Skip to content

Devall#447

Draft
felipecastrosales wants to merge 27 commits intoStacDev:devfrom
SuaMusica:devall
Draft

Devall#447
felipecastrosales wants to merge 27 commits intoStacDev:devfrom
SuaMusica:devall

Conversation

@felipecastrosales
Copy link

@felipecastrosales felipecastrosales commented Feb 12, 2026

Summary by CodeRabbit

  • New Features
    • Added version and build number-based feature gating to control widget availability across app releases.
    • Introduced platform-aware version constraints that evaluate build requirements for iOS, Android, and other platforms.
    • Added custom color parsing support for enhanced theming flexibility.

lstonussi and others added 25 commits May 19, 2025 14:24
feat: add platform validation for widget support in Stac class
@CLAassistant
Copy link

CLAassistant commented Feb 12, 2026

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 3 committers have signed the CLA.

❌ lstonussi
❌ atrope
❌ felipecastrosales


lstonussi seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 12, 2026

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR introduces build number version gating and platform-aware feature constraints. It adds buildNumber parameter propagation through Stac initialization, version validation logic in StacService, a new StacVersion module for managing version conditions and comparisons, custom color parsing support, and additional parser/widget exports.

Changes

Cohort / File(s) Summary
Pubspec Configuration
examples/counter_example/pubspec.yaml, examples/movie_app/pubspec.yaml, pubspec.yaml, packages/stac/pubspec.yaml
Updated string quoting style from single to double quotes; removed empty lines; replaced stac_core version constraint with local path dependency.
Framework Core
packages/stac/lib/src/framework/stac.dart, packages/stac/lib/src/framework/stac_service.dart, packages/stac/lib/src/framework/stac_registry.dart
Added buildNumber parameter to Stac.initialize and propagated through to StacService. Extended StacRegistry with parseCustomColor field, buildNumber storage, and registerBuildNumber method. Enhanced StacService with version/platform validation logic in fromJson and added setParseCustomColor public setter.
Version Management
packages/stac/lib/src/utils/version/stac_version.dart
Introduced new StacVersion class with platform-aware buildNumber configuration, StacConditionVersion enum (greaterThan, lessThan, equal, etc.), and isSatisfied logic for version constraint evaluation with extensions for string parsing and platform-specific field resolution.
Version Testing
packages/stac/test/src/utils/version/stac_version_test.dart
Comprehensive test suite covering StacVersion construction, JSON deserialization, condition parsing, isSatisfied logic across all conditions, platform-specific behavior, and edge cases.
Color Utilities
packages/stac/lib/src/utils/color_utils.dart
Added custom color parsing fallback via StacRegistry.parseCustomColor when standard color types do not match.
Parser & Widget Exports
packages/stac/lib/src/parsers/foundation/layout/parsers.dart, packages/stac/lib/src/parsers/widgets/widgets.dart, packages/stac/lib/src/utils/utils.dart, packages/stac/lib/stac.dart
New layout parser aggregation file; added exports for stac_double and stac_text widgets; exposed stac_version utility in public API; extended main library barrel with layout parser exports.

Sequence Diagram(s)

sequenceDiagram
    participant App as Application
    participant Stac as Stac.initialize()
    participant StacSvc as StacService.initialize()
    participant Registry as StacRegistry
    participant FromJson as StacService.fromJson()

    App->>Stac: initialize(buildNumber: 100)
    Stac->>StacSvc: initialize(buildNumber: 100)
    StacSvc->>Registry: registerBuildNumber(100)
    Registry->>Registry: _buildNumber = 100
    Note over StacSvc,Registry: Initialization complete

    App->>FromJson: fromJson(widgetJson with version)
    rect rgba(100, 150, 200, 0.5)
    FromJson->>Registry: get buildNumber
    FromJson->>FromJson: Check StacVersion.isSatisfied(100)
    end
    FromJson->>FromJson: Check platform constraints
    alt Version/Platform satisfied
        FromJson-->>App: Return widget instance
    else Unsupported
        FromJson-->>App: Return null
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested labels

feature

Suggested reviewers

  • divyanshub024
  • Potatomonsta

Poem

🐰 Hops of versioning flow,
Build numbers now glow,
Platform gates guard the way,
Colors parse custom, hooray!
Features gated with care,
Version logic laid bare

🚥 Pre-merge checks | ✅ 3 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Devall' is vague and does not clearly convey what changes are being made in this pull request. Provide a more descriptive title that summarizes the main changes, such as 'Add version management and custom color parsing' or 'Add StacVersion and platform validation support'.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into dev

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

🤖 Fix all issues with AI agents
In `@packages/stac/lib/src/framework/stac_registry.dart`:
- Around line 75-79: The registerBuildNumber method currently ignores null and
prevents resetting the stored value; update registerBuildNumber(int?
buildNumber) to allow null assignments by removing the null guard and directly
setting _buildNumber = buildNumber so tests that call
StacRegistry.instance.registerBuildNumber(null) actually clear the value, or
alternatively add a new clearBuildNumber() that sets _buildNumber = null if you
want to preserve the guard—refer to registerBuildNumber and the _buildNumber
field when making the change.

In `@packages/stac/lib/src/framework/stac_service.dart`:
- Around line 226-248: The platform gating in stac_service.dart currently uses
dart:io's Platform.operatingSystem (see the block referencing
Platform.operatingSystem, isPlatformSupported, supportedPlatforms and Log.w),
which will fail on Flutter Web; update the check to use Flutter's platform APIs
(use kIsWeb from package:flutter/foundation.dart to short-circuit web and/or
defaultTargetPlatform to compare TargetPlatform values) and map incoming
`platform` values to TargetPlatform (or handle a special "web" token) instead of
directly comparing strings; additionally define and validate against a canonical
set of supported identifiers (an enum or const List like SupportedPlatform /
supportedPlatformStrings) before using contains so typos are caught and your
Log.w can list only validated entries.

In `@packages/stac/lib/src/utils/version/stac_version.dart`:
- Around line 224-226: The current match maps unknown/null condition strings
(the '_' arm next to the "'!=' => StacConditionVersion.notEqual" case) to
StacConditionVersion.notEqual which is surprising; change the default branch to
produce a pass-through condition instead: return
StacConditionVersion.greaterThanOrEqual (or the enum variant representing >=)
and ensure any factory/constructor/path that creates a condition from a string
sets its buildNumber to 0 (i.e. a greaterThanOrEqual with buildNumber 0), so
unrecognized or missing condition strings behave as a no-op rather than silently
becoming notEqual.
- Line 1: The current stac_version.dart imports dart:io which breaks Flutter
Web; split the platform-specific code into two files: create
stac_version_io.dart containing the implementation that uses
Platform/Platform.operatingSystem and any logic currently in stac_version.dart,
and create stac_version_web.dart as a web-safe stub implementation (no dart:io
usage). Replace the direct import in stac_version.dart with conditional
exports/imports that select stac_version_io.dart when dart.library.io is
available and stac_version_web.dart when dart.library.html is available, and
remove the unconditional import of dart:io. Also update any tests that import
stac_version.dart to use the same conditional import pattern or to import the
web/io stubs so tests don’t pull in dart:io on web. Ensure identifiers and
public APIs from the original file remain exported unchanged so callers continue
to use the same symbols.

In `@packages/stac/lib/stac.dart`:
- Line 7: Remove the redundant export of
'package:stac/src/parsers/foundation/layout/parsers.dart' from stac.dart: the
layout parsers are already re-exported via parsers.dart ->
foundation/foundation.dart, so delete the export line "export
'package:stac/src/parsers/foundation/layout/parsers.dart';" from the file to
avoid duplicate exports and keep the public API minimal.

In `@packages/stac/pubspec.yaml`:
- Around line 26-28: The pubspec.yaml currently uses a local path dependency for
stac_core (stac_core: path: ../stac_core) which will block publishing; revert
this to the version constraint (e.g., stac_core: ^1.3.0) before merging or
ensure CI prevents path deps. Update the dependency entry in pubspec.yaml from
the local path back to the versioned form and/or add a CI check (dart pub
publish --dry-run or a lint) to fail the build if any dependency uses a path:
spec so stac_core isn’t published with a local path.

In `@packages/stac/test/src/utils/version/stac_version_test.dart`:
- Around line 188-202: The test for StacVersion.fromJson is brittle because it
assumes the test host is not iOS; change the JSON so the platform-specific key
cannot match the running platform (e.g., replace 'buildNumber_ios' with a
deliberately different key like 'buildNumber_fuchsia' or programmatically
compute a platform key that differs from Platform.operatingSystem) so
StacVersion.fromJson will be forced to use the generic buildNumber; update
assertions against version.buildNumber and leave the condition check for
StacConditionVersion.greaterThanOrEqual unchanged.
- Around line 144-153: The test named "handles version with non-numeric
components" is misleading because StacVersion.buildNumber is an int; update the
test to reflect that by renaming it (e.g., to "handles build number equality" or
"succeeds when build numbers are equal") and remove or replace the comment
"Non-numeric components should be treated as 0"; leave the logic that registers
the build number via StacRegistry.instance.registerBuildNumber(2000) and the
assertion expect(version.isSatisfied(...), true) unchanged so the test still
verifies StacVersion equality semantics.
- Around line 66-73: The test "returns false when app version is null" currently
calls StacRegistry.instance.registerBuildNumber(null) but due to a separate
null-guard bug that leaves the registry value unchanged the test doesn't
exercise the null case; update the test to explicitly assert the intended
behavior of StacVersion.isSatisfied when the app version is null by either (A)
ensuring the registry is actually set to null (fix/registerBuildNumber to allow
setting null) and then changing the expectation to true, or (B) bypassing the
registry and call expect(version.isSatisfied(null), true) directly; reference
StacRegistry.instance.registerBuildNumber and StacVersion.isSatisfied when
making the change.
🧹 Nitpick comments (4)
packages/stac/lib/src/parsers/widgets/widgets.dart (2)

23-23: Export is out of alphabetical order.

stac_double (line 23) is placed before stac_default_bottom_navigation_controller (line 24) and stac_default_tab_controller (line 25), but alphabetically "double" sorts after "default" and "divider". Move this export to after line 26 (stac_divider) to maintain consistent ordering.


62-62: Export is out of alphabetical order.

stac_text (line 62) is placed between stac_selectable_text and stac_set_value, but alphabetically "text" sorts after "set_value". Move this export to after the stac_tab_bar_view / stac_table_cell block (around line 83-84) where other stac_text_* exports reside, to keep the barrel consistently sorted.

packages/stac/lib/src/framework/stac_registry.dart (1)

18-18: parseCustomColor is a public mutable field on a singleton — consider using a setter for consistency.

Other registry state (_buildNumber) is encapsulated behind a method and getter. Exposing parseCustomColor as a bare public field breaks that pattern and allows any caller to overwrite it without logging or validation. StacService.setParseCustomColor already acts as the intended entry point, so making the field private with a setter would be more consistent.

This is minor since the registry is internal, but worth aligning.

packages/stac/lib/src/framework/stac.dart (1)

191-203: buildNumber parameter not documented in the method's doc comment.

The doc comment (lines 134–180) describes all other parameters but omits buildNumber. Consider adding a brief description, e.g.:

- [buildNumber]: Optional build number for version gating. When provided, JSON widget definitions with a version constraint will be checked against this value.

Comment on lines +75 to +79
void registerBuildNumber(int? buildNumber) {
if (buildNumber != null) {
_buildNumber = buildNumber;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

registerBuildNumber silently ignores null, preventing reset.

Once a build number is set, there is no way to clear it back to null. The test setUp calls StacRegistry.instance.registerBuildNumber(null) expecting a reset, but this is a no-op due to the null guard. The isSatisfied group's "returns false when app version is null" test (line 66 of test file) accidentally passes because _buildNumber retains the previous non-null value (2000), not because it was actually reset to null.

If the intent is to protect against accidental null assignment, consider a separate clearBuildNumber() method. Otherwise, simply remove the guard:

🐛 Proposed fix
  void registerBuildNumber(int? buildNumber) {
-   if (buildNumber != null) {
-     _buildNumber = buildNumber;
-   }
+   _buildNumber = buildNumber;
  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
void registerBuildNumber(int? buildNumber) {
if (buildNumber != null) {
_buildNumber = buildNumber;
}
}
void registerBuildNumber(int? buildNumber) {
_buildNumber = buildNumber;
}
🤖 Prompt for AI Agents
In `@packages/stac/lib/src/framework/stac_registry.dart` around lines 75 - 79, The
registerBuildNumber method currently ignores null and prevents resetting the
stored value; update registerBuildNumber(int? buildNumber) to allow null
assignments by removing the null guard and directly setting _buildNumber =
buildNumber so tests that call StacRegistry.instance.registerBuildNumber(null)
actually clear the value, or alternatively add a new clearBuildNumber() that
sets _buildNumber = null if you want to preserve the guard—refer to
registerBuildNumber and the _buildNumber field when making the change.

Comment on lines +226 to +248
if (platform != null) {
final currentPlatform = Platform.operatingSystem;
bool isPlatformSupported = false;
List<String> supportedPlatforms = [];

// Check if platform is a list or a single string
if (platform is List) {
supportedPlatforms = platform.map((e) => e.toString()).toList();
isPlatformSupported = supportedPlatforms.contains(currentPlatform);
} else if (platform is String) {
supportedPlatforms.add(platform);
isPlatformSupported = platform == currentPlatform;
}

// If platform is not supported, log and return null
if (!isPlatformSupported) {
final platformsStr = supportedPlatforms.join(', ');
Log.w(
'Widget not supported on platform [$currentPlatform]. Only available for: $platformsStr',
);
return null;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Platform gating uses dart:io Platform — same Flutter Web concern.

Platform.operatingSystem on line 227 will fail on Flutter Web (same root cause as flagged in stac_version.dart). If web support is needed, consider using defaultTargetPlatform from package:flutter/foundation.dart or kIsWeb to handle web gracefully.

Additionally, the platform check accepts arbitrary strings (e.g., "ios", "android", "macos"), but there's no validation or documentation of accepted values. Consider defining an enum or constant list of supported platform identifiers to prevent typos in JSON configs.

🤖 Prompt for AI Agents
In `@packages/stac/lib/src/framework/stac_service.dart` around lines 226 - 248,
The platform gating in stac_service.dart currently uses dart:io's
Platform.operatingSystem (see the block referencing Platform.operatingSystem,
isPlatformSupported, supportedPlatforms and Log.w), which will fail on Flutter
Web; update the check to use Flutter's platform APIs (use kIsWeb from
package:flutter/foundation.dart to short-circuit web and/or
defaultTargetPlatform to compare TargetPlatform values) and map incoming
`platform` values to TargetPlatform (or handle a special "web" token) instead of
directly comparing strings; additionally define and validate against a canonical
set of supported identifiers (an enum or const List like SupportedPlatform /
supportedPlatformStrings) before using contains so typos are caught and your
Log.w can list only validated entries.

@@ -0,0 +1,281 @@
import 'dart:io';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the actual file to understand the Platform usage
cat -n packages/stac/lib/src/utils/version/stac_version.dart | head -200

Repository: StacDev/stac

Length of output: 7481


🏁 Script executed:

#!/bin/bash
# Check for dart:io imports across the codebase
rg "import 'dart:io'" --type=dart

Repository: StacDev/stac

Length of output: 549


🏁 Script executed:

#!/bin/bash
# Check for web-specific handling patterns
rg "defaultTargetPlatform|kIsWeb|dart:io|conditional" --type=dart | head -30

Repository: StacDev/stac

Length of output: 2484


🏁 Script executed:

#!/bin/bash
# Check if Flutter is a dependency
fd pubspec.yaml | xargs cat

Repository: StacDev/stac

Length of output: 16405


🏁 Script executed:

#!/bin/bash
# Check if there's a conditional import pattern for stac_version
fd "stac_version" --type f | head -20

Repository: StacDev/stac

Length of output: 169


🏁 Script executed:

#!/bin/bash
# Look at stac_image_parser to see how conditional imports are handled
cat -n packages/stac/lib/src/parsers/widgets/stac_image/stac_image_parser.dart | head -50

Repository: StacDev/stac

Length of output: 431


🏁 Script executed:

#!/bin/bash
# Check if stac_core package uses dart:io (since it's a pure Dart package)
rg "import 'dart:io'" packages/stac_core/ --type=dart

Repository: StacDev/stac

Length of output: 38


🏁 Script executed:

#!/bin/bash
# Check the stac package pubspec.yaml to see if web is mentioned
cat packages/stac/pubspec.yaml | grep -i "web\|environment\|flutter" -A 5

Repository: StacDev/stac

Length of output: 769


🏁 Script executed:

#!/bin/bash
# Check if there's a stub or web implementation file for stac_version
fd "stac_version" -x ls -la {} \;

Repository: StacDev/stac

Length of output: 271


🏁 Script executed:

#!/bin/bash
# Look at how stac_image_parser_io and stac_image_parser_web differ
head -30 packages/stac/lib/src/parsers/widgets/stac_image/stac_image_parser_io.dart
head -30 packages/stac/lib/src/parsers/widgets/stac_image/stac_image_parser_web.dart

Repository: StacDev/stac

Length of output: 1814


🏁 Script executed:

#!/bin/bash
# Verify that stac_version_test.dart would also be affected
grep -n "Platform\|dart:io" packages/stac/test/src/utils/version/stac_version_test.dart

Repository: StacDev/stac

Length of output: 414


dart:io breaks Flutter Web — requires conditional imports.

Platform.operatingSystem from dart:io is unavailable in web environments. This file will fail to compile or run on Flutter Web. The codebase already uses conditional exports elsewhere (stac_image_parser.dart); apply the same pattern here:

  1. Create stac_version_io.dart containing the platform-aware implementation
  2. Create stac_version_web.dart as a stub (or without platform-specific logic)
  3. Update imports to use conditional exports based on dart.library.io and dart.library.html
  4. Update the test file similarly to avoid importing dart:io unconditionally
🤖 Prompt for AI Agents
In `@packages/stac/lib/src/utils/version/stac_version.dart` at line 1, The current
stac_version.dart imports dart:io which breaks Flutter Web; split the
platform-specific code into two files: create stac_version_io.dart containing
the implementation that uses Platform/Platform.operatingSystem and any logic
currently in stac_version.dart, and create stac_version_web.dart as a web-safe
stub implementation (no dart:io usage). Replace the direct import in
stac_version.dart with conditional exports/imports that select
stac_version_io.dart when dart.library.io is available and stac_version_web.dart
when dart.library.html is available, and remove the unconditional import of
dart:io. Also update any tests that import stac_version.dart to use the same
conditional import pattern or to import the web/io stubs so tests don’t pull in
dart:io on web. Ensure identifiers and public APIs from the original file remain
exported unchanged so callers continue to use the same symbols.

Comment on lines +224 to +226
'!=' => StacConditionVersion.notEqual,
_ => StacConditionVersion.notEqual,
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Defaulting unknown/null condition to notEqual is surprising.

When a condition string is unrecognized or null, it silently becomes notEqual. This can cause widgets to be unexpectedly hidden (or shown) when the JSON config has a typo in the condition field. A more defensive approach would be to default to a "pass-through" condition (like greaterThanOrEqual with buildNumber 0) or throw a parse error to surface misconfigurations early.

🤖 Prompt for AI Agents
In `@packages/stac/lib/src/utils/version/stac_version.dart` around lines 224 -
226, The current match maps unknown/null condition strings (the '_' arm next to
the "'!=' => StacConditionVersion.notEqual" case) to
StacConditionVersion.notEqual which is surprising; change the default branch to
produce a pass-through condition instead: return
StacConditionVersion.greaterThanOrEqual (or the enum variant representing >=)
and ensure any factory/constructor/path that creates a condition from a string
sets its buildNumber to 0 (i.e. a greaterThanOrEqual with buildNumber 0), so
unrecognized or missing condition strings behave as a no-op rather than silently
becoming notEqual.

export 'package:stac/src/parsers/parsers.dart';
export 'package:stac/src/services/services.dart';
export 'package:stac/src/utils/utils.dart';
export 'package:stac/src/parsers/foundation/layout/parsers.dart';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, locate and examine the main parsers.dart file
find packages/stac/lib/src/parsers -name "parsers.dart" -type f

# Then examine its contents
cat packages/stac/lib/src/parsers/parsers.dart

# Also check what's in the foundation directory
ls -la packages/stac/lib/src/parsers/foundation/

# Check if there's a foundation.dart barrel file
ls -la packages/stac/lib/src/parsers/foundation/layout/

Repository: StacDev/stac

Length of output: 2478


🏁 Script executed:

cat packages/stac/lib/src/parsers/foundation/foundation.dart

Repository: StacDev/stac

Length of output: 5546


🏁 Script executed:

cat packages/stac/lib/src/parsers/foundation/layout/parsers.dart

Repository: StacDev/stac

Length of output: 452


Remove this redundant export — the layout parsers are already available through the existing exports.

All parsers in layout/parsers.dart are already re-exported by foundation/foundation.dart, which is itself exported by parsers.dart at line 4. This direct export at line 7 is unnecessary and should be removed.

🤖 Prompt for AI Agents
In `@packages/stac/lib/stac.dart` at line 7, Remove the redundant export of
'package:stac/src/parsers/foundation/layout/parsers.dart' from stac.dart: the
layout parsers are already re-exported via parsers.dart ->
foundation/foundation.dart, so delete the export line "export
'package:stac/src/parsers/foundation/layout/parsers.dart';" from the file to
avoid duplicate exports and keep the public API minimal.

Comment on lines +26 to +28
# stac_core: ^1.3.0
stac_core:
path: ../stac_core
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Local path dependency must not be published — ensure it's reverted before release.

Switching stac_core to a local path is fine for development, but path dependencies are rejected by pub publish. The commented-out version constraint suggests this is intentional for now, but leaving it in a PR targeting dev risks it being forgotten when cutting a release.

Consider either:

  • Restoring the version constraint before merge, or
  • Adding a CI check (e.g., dart pub publish --dry-run) to catch path dependencies before release.
🤖 Prompt for AI Agents
In `@packages/stac/pubspec.yaml` around lines 26 - 28, The pubspec.yaml currently
uses a local path dependency for stac_core (stac_core: path: ../stac_core) which
will block publishing; revert this to the version constraint (e.g., stac_core:
^1.3.0) before merging or ensure CI prevents path deps. Update the dependency
entry in pubspec.yaml from the local path back to the versioned form and/or add
a CI check (dart pub publish --dry-run or a lint) to fail the build if any
dependency uses a path: spec so stac_core isn’t published with a local path.

Comment on lines +66 to +73
test('returns false when app version is null', () {
StacRegistry.instance.registerBuildNumber(null);
final version = StacVersion(
buildNumber: 1000,
condition: StacConditionVersion.equal,
);
expect(version.isSatisfied(StacRegistry.instance.buildNumber), false);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Test "returns false when app version is null" does not actually test a null build number.

Due to the registerBuildNumber null-guard bug (flagged in stac_registry.dart), line 67 is a no-op — _buildNumber remains 2000 from the group setUp. The assertion passes because 2000 != 1000 (equal condition fails), not because the build number is null.

If the null-guard is fixed, isSatisfied(null) returns true per the implementation (line 267-269 of stac_version.dart), so the expectation of false would need to be updated to true — or the intended behavior reconsidered.

🤖 Prompt for AI Agents
In `@packages/stac/test/src/utils/version/stac_version_test.dart` around lines 66
- 73, The test "returns false when app version is null" currently calls
StacRegistry.instance.registerBuildNumber(null) but due to a separate null-guard
bug that leaves the registry value unchanged the test doesn't exercise the null
case; update the test to explicitly assert the intended behavior of
StacVersion.isSatisfied when the app version is null by either (A) ensuring the
registry is actually set to null (fix/registerBuildNumber to allow setting null)
and then changing the expectation to true, or (B) bypassing the registry and
call expect(version.isSatisfied(null), true) directly; reference
StacRegistry.instance.registerBuildNumber and StacVersion.isSatisfied when
making the change.

Comment on lines +144 to +153
test('handles version with non-numeric components', () {
StacRegistry.instance.registerBuildNumber(2000);

final version = StacVersion(
buildNumber: 2000,
condition: StacConditionVersion.equal,
);
// Non-numeric components should be treated as 0
expect(version.isSatisfied(StacRegistry.instance.buildNumber), true);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Misleading test — "non-numeric components" don't apply to int build numbers.

StacVersion.buildNumber is typed as int, so there are no non-numeric components possible. The comment on line 151 ("Non-numeric components should be treated as 0") and the test name are holdovers from a string-based version model. This test is equivalent to the basic equal-condition test and should be renamed or removed to avoid confusion.

🤖 Prompt for AI Agents
In `@packages/stac/test/src/utils/version/stac_version_test.dart` around lines 144
- 153, The test named "handles version with non-numeric components" is
misleading because StacVersion.buildNumber is an int; update the test to reflect
that by renaming it (e.g., to "handles build number equality" or "succeeds when
build numbers are equal") and remove or replace the comment "Non-numeric
components should be treated as 0"; leave the logic that registers the build
number via StacRegistry.instance.registerBuildNumber(2000) and the assertion
expect(version.isSatisfied(...), true) unchanged so the test still verifies
StacVersion equality semantics.

Comment on lines +188 to +202
test(
'falls back to generic buildNumber when platform-specific not available',
() {
final json = {
'buildNumber': 1500,
'buildNumber_ios': 2500, // Different platform
'condition': '>=',
};

final version = StacVersion.fromJson(json);

// Should use generic buildNumber since `platform`-specific is not available
expect(version.buildNumber, 1500);
expect(version.condition, StacConditionVersion.greaterThanOrEqual);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Platform-dependent test is fragile — will fail if run on iOS.

This test hardcodes 'buildNumber_ios': 2500 as a "different platform" key and expects the generic buildNumber to be used. If the test suite ever runs on an iOS device/simulator, Platform.operatingSystem would be 'ios', and the assertion on line 200 would fail (2500 instead of 1500).

Consider using a platform key that is guaranteed to differ from the test host (e.g., an invented key like 'buildNumber_fuchsia'), or make the test explicitly choose a platform different from Platform.operatingSystem.

🤖 Prompt for AI Agents
In `@packages/stac/test/src/utils/version/stac_version_test.dart` around lines 188
- 202, The test for StacVersion.fromJson is brittle because it assumes the test
host is not iOS; change the JSON so the platform-specific key cannot match the
running platform (e.g., replace 'buildNumber_ios' with a deliberately different
key like 'buildNumber_fuchsia' or programmatically compute a platform key that
differs from Platform.operatingSystem) so StacVersion.fromJson will be forced to
use the generic buildNumber; update assertions against version.buildNumber and
leave the condition check for StacConditionVersion.greaterThanOrEqual unchanged.

@felipecastrosales felipecastrosales marked this pull request as draft February 12, 2026 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants