Version management, platform validation and fallback colors#447
Open
felipecastrosales wants to merge 35 commits intoStacDev:devfrom
Open
Version management, platform validation and fallback colors#447felipecastrosales wants to merge 35 commits intoStacDev:devfrom
felipecastrosales wants to merge 35 commits intoStacDev:devfrom
Conversation
… app version registration
…sion satisfaction check
Feature/version
…orts in stac_registry.dart and stac.dart
Feature/flutter35
feat: add platform validation for widget support in Stac class
|
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. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds build-number propagation and gating, platform-aware version checks, a custom color parsing hook, new StacVersion utilities and tests, additional parser/widget re-exports, a StacTextStyle.copyWith extension, and a few pubspec dependency adjustments. Changes
Sequence DiagramsequenceDiagram
participant App as App
participant Stac as Stac
participant Service as StacService
participant Registry as StacRegistry
participant Json as JsonData
participant Version as StacVersion
App->>Stac: initialize(buildNumber: 100)
Stac->>Service: initialize(buildNumber: 100)
Service->>Registry: registerBuildNumber(100)
Registry-->>Service: ack
App->>Service: fromJson(versionConfig)
Service->>Registry: buildNumber (getter)
Registry-->>Service: 100
Service->>Json: parse StacVersion config
Json-->>Version: {buildNumber:120, condition:"greaterThan"}
Service->>Version: isSatisfied(100)
Version-->>Service: false
Service-->>App: null (feature gated)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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 beforestac_default_bottom_navigation_controller(line 24) andstac_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 betweenstac_selectable_textandstac_set_value, but alphabetically "text" sorts after "set_value". Move this export to after thestac_tab_bar_view/stac_table_cellblock (around line 83-84) where otherstac_text_*exports reside, to keep the barrel consistently sorted.packages/stac/lib/src/framework/stac_registry.dart (1)
18-18:parseCustomColoris a public mutable field on a singleton — consider using a setter for consistency.Other registry state (
_buildNumber) is encapsulated behind a method and getter. ExposingparseCustomColoras a bare public field breaks that pattern and allows any caller to overwrite it without logging or validation.StacService.setParseCustomColoralready 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:buildNumberparameter 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 aversionconstraint will be checked against this value.
Author
|
@coderabbitai enhance my description on pr. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 493
OverviewThis PR introduces version management, platform validation, and several improvements to the STAC framework. ✨ FeaturesVersion Management System
Build Number Support
Platform Validation
Custom Color Parsing
🔧 Improvements
✅ Tests
🧹 Chores
🔄 Migration NotesIf you're using await Stac.initialize(
json: json,
buildNumber: 123, // Optional: enables version-aware widgets
);To use custom color parsing: StacService.setParseCustomColor((colorString) {
// Your custom color parsing logic
return Colors.purple;
});To update the PR description:
Let me know if you'd like any adjustments to the description! |
- Introduced a new method to normalize and validate platform identifiers against a predefined list of supported platforms. - Updated platform checking logic to improve validation and logging of unsupported platforms. - Removed unused imports and cleaned up the code for better readability. - Added missing export for layout parsers in foundation.dart.
…d version management - Removed the deprecated platform identifier normalization logic from stac_service.dart. - Updated platform checks to utilize a new utility function for better consistency. - Simplified the StacVersion class constructor and adjusted platform-specific JSON handling to use the current platform string. - Added export for stac_platforms.dart in utils.dart to support the new platform handling logic.
- Changed the test to return true when the app version is null, indicating that the constraint is satisfied in this case. - Renamed a test for clarity, focusing on build number equality handling. - Updated comments for better understanding of platform-specific JSON handling.
felipecastrosales
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/stac/test/src/utils/version/stac_version_test.dart (2)
17-20: Remove the ineffective registry reset in test setup.
registerBuildNumber(null)is intentionally ignored, so the “reset” comment is misleading and can hide state assumptions between tests.Based on learnings: In `packages/stac/lib/src/framework/stac_registry.dart`, null assignment in `registerBuildNumber` is intentionally guarded and resetting build number is not part of the design.🧹 Suggested cleanup
group('StacVersion', () { - setUp(() { - // Reset the app version before each test - StacRegistry.instance.registerBuildNumber(null); - }); - test('creates StacVersion instance correctly', () {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac/test/src/utils/version/stac_version_test.dart` around lines 17 - 20, The test setup is calling an ineffective reset via StacRegistry.instance.registerBuildNumber(null) which is intentionally ignored; remove that call and its "Reset the app version before each test" comment from the setUp block, and if tests require deterministic registry state replace it with an explicit action that the registry supports (e.g., a real reset method or registering a specific known build number) rather than registerBuildNumber(null).
226-239:⚠️ Potential issue | 🟠 MajorMake this fallback-condition test platform-agnostic.
Using
condition_windowsas the “different platform” key makes the test fail when run on Windows.🧪 Proposed stable test key
test('falls back to generic condition when platform-specific not available', () { final json = { 'buildNumber': 1000, 'condition': '<', - 'condition_windows': '!=', // Different platform + 'condition_fuchsia': '!=', // Non-matching key for standard test hosts };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac/test/src/utils/version/stac_version_test.dart` around lines 226 - 239, The test fails on Windows because it uses a hardcoded platform key 'condition_windows'; update the test JSON to use a non-existent platform-specific key (e.g., 'condition_other' or 'condition_other_platform') so it won't match any real platform and the code path in StacVersion.fromJson falls back to the generic 'condition'. Keep the rest of the test the same and assert that version.condition equals StacConditionVersion.lessThan.
🧹 Nitpick comments (4)
packages/stac_core/lib/foundation/text/stac_text_style/stac_text_style.dart (1)
733-781: Consider aswitchexpression for exhaustive dispatch.The cascading
if (this is …)checks work correctly, but aswitchexpression with Dart 3 type patterns would be more idiomatic, self-documenting, and gives you a compile-time exhaustiveness check if you ever markStacTextStyleassealed:♻️ Suggested refactor
- if (this is StacCustomTextStyle) { - return (this as StacCustomTextStyle).copyWith( - inherit: inherit, - color: color, - backgroundColor: backgroundColor, - fontSize: fontSize, - fontWeight: fontWeight, - fontStyle: fontStyle, - letterSpacing: letterSpacing, - wordSpacing: wordSpacing, - textBaseline: textBaseline, - height: height, - leadingDistribution: leadingDistribution, - decorationColor: decorationColor, - decorationStyle: decorationStyle, - decorationThickness: decorationThickness, - debugLabel: debugLabel, - fontFamily: fontFamily, - fontFamilyFallback: fontFamilyFallback, - package: package, - overflow: overflow, - ); - } - if (this is StacThemeTextStyle) { - return (this as StacThemeTextStyle).copyWith( - inherit: inherit, - color: color, - backgroundColor: backgroundColor, - fontSize: fontSize, - fontWeight: fontWeight, - fontStyle: fontStyle, - letterSpacing: letterSpacing, - wordSpacing: wordSpacing, - textBaseline: textBaseline, - height: height, - leadingDistribution: leadingDistribution, - decorationColor: decorationColor, - decorationStyle: decorationStyle, - decorationThickness: decorationThickness, - debugLabel: debugLabel, - fontFamily: fontFamily, - fontFamilyFallback: fontFamilyFallback, - package: package, - overflow: overflow, - ); - } - throw StateError( - 'copyWith only supported on StacCustomTextStyle and StacThemeTextStyle', - ); + return switch (this) { + final StacCustomTextStyle custom => custom.copyWith( + inherit: inherit, + color: color, + backgroundColor: backgroundColor, + fontSize: fontSize, + fontWeight: fontWeight, + fontStyle: fontStyle, + letterSpacing: letterSpacing, + wordSpacing: wordSpacing, + textBaseline: textBaseline, + height: height, + leadingDistribution: leadingDistribution, + decorationColor: decorationColor, + decorationStyle: decorationStyle, + decorationThickness: decorationThickness, + debugLabel: debugLabel, + fontFamily: fontFamily, + fontFamilyFallback: fontFamilyFallback, + package: package, + overflow: overflow, + ), + final StacThemeTextStyle theme => theme.copyWith( + inherit: inherit, + color: color, + backgroundColor: backgroundColor, + fontSize: fontSize, + fontWeight: fontWeight, + fontStyle: fontStyle, + letterSpacing: letterSpacing, + wordSpacing: wordSpacing, + textBaseline: textBaseline, + height: height, + leadingDistribution: leadingDistribution, + decorationColor: decorationColor, + decorationStyle: decorationStyle, + decorationThickness: decorationThickness, + debugLabel: debugLabel, + fontFamily: fontFamily, + fontFamilyFallback: fontFamilyFallback, + package: package, + overflow: overflow, + ), + _ => throw StateError( + 'copyWith only supported on StacCustomTextStyle and StacThemeTextStyle', + ), + };This eliminates the redundant
ascasts (the pattern binds a typed variable directly) and makes it immediately clear that the branches are mutually exclusive. IfStacTextStyleis later madesealed, the_wildcard arm becomes a compile-time error, prompting updates when new subtypes are added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_core/lib/foundation/text/stac_text_style/stac_text_style.dart` around lines 733 - 781, Replace the cascading type-check ifs in the copyWith implementation with a Dart 3 switch expression using type patterns to match StacCustomTextStyle and StacThemeTextStyle, binding the matched value to a typed variable and calling its copyWith (e.g., case StacCustomTextStyle s: return s.copyWith(...); case StacThemeTextStyle s: return s.copyWith(...);) so you remove the redundant casts and get exhaustiveness checking; keep the existing parameters passed into copyWith and keep the StateError fallback (or let the switch be exhaustive once StacTextStyle is sealed).packages/stac/lib/src/parsers/foundation/foundation.dart (1)
72-72: Remove redundant layout parser exports to maintain a single source of truth.Lines 62-71 export 10 individual layout parsers. Line 72 exports
layout/parsers.dart, which re-exports all of those same files. This creates duplicate entries in the public API surface. Choose one approach: either export individual files directly or use the aggregator; do not do both.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac/lib/src/parsers/foundation/foundation.dart` at line 72, foundation.dart currently exports individual layout parser files and also re-exports them via layout/parsers.dart, creating duplicate public exports; remove the redundancy by choosing one approach—either delete the single-line export 'layout/parsers.dart' so only the individual layout parser exports remain, or remove the individual layout parser exports and keep only export 'layout/parsers.dart'—and update foundation.dart accordingly (reference symbols: export 'layout/parsers.dart' and the individual layout parser export statements in foundation.dart).packages/stac/lib/src/utils/version/stac_version.dart (1)
151-156: Prefer explicitbuildNumberparsing to avoid opaque runtime errors.
toPlatformJson(...).toInt()on dynamic can throwNoSuchMethodErrorfor malformed payloads (e.g. missing or string values), which is hard to diagnose.♻️ Proposed parsing hardening
factory StacVersion.fromJson(Map<String, dynamic> json) { + final rawBuildNumber = toPlatformJson(json, 'buildNumber'); + if (rawBuildNumber is! num) { + throw const FormatException( + 'Invalid or missing "buildNumber" in version config', + ); + } + return StacVersion( - buildNumber: toPlatformJson(json, 'buildNumber').toInt(), + buildNumber: rawBuildNumber.toInt(), condition: (toPlatformJson(json, 'condition') as String?) .toStacConditionVersion(), ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac/lib/src/utils/version/stac_version.dart` around lines 151 - 156, In StacVersion.fromJson replace the fragile toPlatformJson(json, 'buildNumber').toInt() usage with explicit parsing: retrieve the dynamic value via toPlatformJson(json, 'buildNumber'), check for null, handle int values directly, handle numeric strings with int.tryParse, and otherwise throw a clear FormatException (or return a default) naming the field and payload; keep the rest of the factory (including condition parsing via toStacConditionVersion()) unchanged so callers of StacVersion.fromJson get a descriptive error for malformed buildNumber values.packages/stac/lib/src/framework/stac_service.dart (1)
228-249: Consider fail-closed behavior whenplatformis provided but all identifiers are invalid.With
platform: ['andriod'](typo),validatedPlatformsis empty and the widget is currently rendered, which bypasses the intended platform constraint.🛡️ Suggested guard
final invalid = rawList .where((s) => !supportedPlatformStrings.contains(s)) .toList(); if (invalid.isNotEmpty) { Log.w( 'Unknown platform identifier(s) in "platform": $invalid. Supported: $supportedPlatformStrings', ); } + + if (validatedPlatforms.isEmpty && rawList.isNotEmpty) { + Log.w( + 'Ignoring widget because all provided platform identifiers are invalid: $rawList', + ); + return null; + } final isPlatformSupported = validatedPlatforms.contains( currentPlatform, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac/lib/src/framework/stac_service.dart` around lines 228 - 249, The current logic lets a widget render when a `platform` list was provided but all entries are invalid (e.g., typo "andriod") because `validatedPlatforms` becomes empty; change to fail-closed by detecting when `rawList` (or the original provided list) is non-empty and `validatedPlatforms.isEmpty`, log the unknown identifiers using `invalid`/`supportedPlatformStrings` and return null; keep the existing warnings for unknown identifiers and the existing `isPlatformSupported` check otherwise (references: validatedPlatforms, rawList, supportedPlatformStrings, invalid, isPlatformSupported, currentPlatform).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/stac/lib/src/utils/stac_platforms.dart`:
- Around line 14-27: The docstring for currentPlatformString() claims it returns
one of supportedPlatformStrings but the function can return 'unknown'; fix this
mismatch by either adding 'unknown' to the supportedPlatformStrings set/const or
updating the docstring to state that the function may return 'unknown' for
unrecognized platforms; update the declaration of supportedPlatformStrings (or
the doc comment above currentPlatformString) and ensure the switch/default
branch remains or is adjusted accordingly so the contract and implementation
match (refer to currentPlatformString() and supportedPlatformStrings).
In `@packages/stac/test/src/utils/version/stac_version_test.dart`:
- Around line 46-64: The tests currently assert unknown/null inputs map to
StacConditionVersion.greaterThanOrEqual, but the implementation of
toStacConditionVersion() in stac_version.dart intentionally returns
StacConditionVersion.notEqual for unknown or null inputs; update the test
expectations: change the map entry for 'invalid' to expect
StacConditionVersion.notEqual and change the nullString expectation to
StacConditionVersion.notEqual so the tests align with the
toStacConditionVersion() behavior.
---
Duplicate comments:
In `@packages/stac/test/src/utils/version/stac_version_test.dart`:
- Around line 17-20: The test setup is calling an ineffective reset via
StacRegistry.instance.registerBuildNumber(null) which is intentionally ignored;
remove that call and its "Reset the app version before each test" comment from
the setUp block, and if tests require deterministic registry state replace it
with an explicit action that the registry supports (e.g., a real reset method or
registering a specific known build number) rather than
registerBuildNumber(null).
- Around line 226-239: The test fails on Windows because it uses a hardcoded
platform key 'condition_windows'; update the test JSON to use a non-existent
platform-specific key (e.g., 'condition_other' or 'condition_other_platform') so
it won't match any real platform and the code path in StacVersion.fromJson falls
back to the generic 'condition'. Keep the rest of the test the same and assert
that version.condition equals StacConditionVersion.lessThan.
---
Nitpick comments:
In `@packages/stac_core/lib/foundation/text/stac_text_style/stac_text_style.dart`:
- Around line 733-781: Replace the cascading type-check ifs in the copyWith
implementation with a Dart 3 switch expression using type patterns to match
StacCustomTextStyle and StacThemeTextStyle, binding the matched value to a typed
variable and calling its copyWith (e.g., case StacCustomTextStyle s: return
s.copyWith(...); case StacThemeTextStyle s: return s.copyWith(...);) so you
remove the redundant casts and get exhaustiveness checking; keep the existing
parameters passed into copyWith and keep the StateError fallback (or let the
switch be exhaustive once StacTextStyle is sealed).
In `@packages/stac/lib/src/framework/stac_service.dart`:
- Around line 228-249: The current logic lets a widget render when a `platform`
list was provided but all entries are invalid (e.g., typo "andriod") because
`validatedPlatforms` becomes empty; change to fail-closed by detecting when
`rawList` (or the original provided list) is non-empty and
`validatedPlatforms.isEmpty`, log the unknown identifiers using
`invalid`/`supportedPlatformStrings` and return null; keep the existing warnings
for unknown identifiers and the existing `isPlatformSupported` check otherwise
(references: validatedPlatforms, rawList, supportedPlatformStrings, invalid,
isPlatformSupported, currentPlatform).
In `@packages/stac/lib/src/parsers/foundation/foundation.dart`:
- Line 72: foundation.dart currently exports individual layout parser files and
also re-exports them via layout/parsers.dart, creating duplicate public exports;
remove the redundancy by choosing one approach—either delete the single-line
export 'layout/parsers.dart' so only the individual layout parser exports
remain, or remove the individual layout parser exports and keep only export
'layout/parsers.dart'—and update foundation.dart accordingly (reference symbols:
export 'layout/parsers.dart' and the individual layout parser export statements
in foundation.dart).
In `@packages/stac/lib/src/utils/version/stac_version.dart`:
- Around line 151-156: In StacVersion.fromJson replace the fragile
toPlatformJson(json, 'buildNumber').toInt() usage with explicit parsing:
retrieve the dynamic value via toPlatformJson(json, 'buildNumber'), check for
null, handle int values directly, handle numeric strings with int.tryParse, and
otherwise throw a clear FormatException (or return a default) naming the field
and payload; keep the rest of the factory (including condition parsing via
toStacConditionVersion()) unchanged so callers of StacVersion.fromJson get a
descriptive error for malformed buildNumber values.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/stac/lib/src/framework/stac_service.dartpackages/stac/lib/src/parsers/foundation/foundation.dartpackages/stac/lib/src/utils/stac_platforms.dartpackages/stac/lib/src/utils/utils.dartpackages/stac/lib/src/utils/version/stac_version.dartpackages/stac/lib/stac.dartpackages/stac/test/src/utils/version/stac_version_test.dartpackages/stac_core/lib/foundation/text/stac_text_style/stac_text_style.dartpubspec.yaml
💤 Files with no reviewable changes (1)
- pubspec.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/stac/pubspec.yaml (1)
26-31:⚠️ Potential issue | 🟠 MajorGit dependency still blocks
pub publish— same root issue as the prior path dependency.Git dependencies are not allowed as dependencies for packages uploaded to pub.dev. Swapping the local path for a git reference resolves the local-machine portability issue but does not unblock publishing to pub.dev. The comment on line 26 (
# when approved, use stac_core: ^1.3.x) confirms the intent, so this must be reverted to a hosted version constraint before any release cut.🔧 Revert to a hosted version constraint for publishability
- # when approved, use stac_core: ^1.3.x - stac_core: - git: - url: https://github.com/SuaMusica/stac.git - path: packages/stac_core - ref: devall + stac_core: ^1.3.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac/pubspec.yaml` around lines 26 - 31, The pubspec currently pins stac_core as a git dependency (the git block under stac_core) which prevents publishing to pub.dev; revert this to a hosted version constraint (e.g., replace the entire git: url/path/ref block with a version entry like "stac_core: ^1.3.x" per the existing comment) so the package depends on a published release before cutting a release.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/stac_webview/pubspec.yaml`:
- Around line 22-27: The pubspec currently declares stac_core as a git
dependency (stac_core: git: url: ... ref: devall); replace that with a hosted
constraint (e.g., stac_core: ^1.3.0) so the package can be published to pub.dev,
or if you need a temporary non-pub.dev integration move the git entry out of
pubspec and configure a custom hosted repository instead; update the dependency
declaration for stac_core accordingly and remove the git block.
In `@packages/stac/pubspec.yaml`:
- Line 31: Replace the mutable git ref by pinning to a specific commit SHA:
locate the dependency entry that currently reads "ref: devall" in the pubspec
(the git dependency for this package) and change the ref value to the exact
commit SHA you want to lock (e.g., ref: <commit-sha>), then run dart pub get to
verify the resolved revision; ensure you commit the updated pubspec.yaml so the
dependency is reproducibly pinned.
---
Duplicate comments:
In `@packages/stac/pubspec.yaml`:
- Around line 26-31: The pubspec currently pins stac_core as a git dependency
(the git block under stac_core) which prevents publishing to pub.dev; revert
this to a hosted version constraint (e.g., replace the entire git: url/path/ref
block with a version entry like "stac_core: ^1.3.x" per the existing comment) so
the package depends on a published release before cutting a release.
Comment on lines
+22
to
+27
| # when approved, use stac_core: ^1.3.x | ||
| stac_core: | ||
| git: | ||
| url: https://github.com/SuaMusica/stac.git | ||
| path: packages/stac_core | ||
| ref: devall |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
git dependencies pub.dev Dart Flutter package publishing rules
💡 Result:
Rules for publishing a Dart/Flutter package to pub.dev when using git dependencies
- You cannot publish to pub.dev if your package has any
git:dependencies inpubspec.yaml(this includes dependency overrides). Pub.dev requires dependencies to be hosted on the default pub server (pub.dev) (plus SDK deps likesdk: flutter). [1], [2] - You also cannot publish to pub.dev if your package has any
path:dependencies. [2] - Pub.dev likewise doesn’t allow hosted dependencies from custom package repositories (non-default hosted sources). [1]
What to do instead
- Publish the dependency package to pub.dev, then change your dependency to a normal hosted version constraint (e.g.,
^1.2.3) and publish your main package. [2] - If you must depend on git/private/custom-hosted packages, publish to a custom package repository using
publish_to:(not pub.dev). [1]
Relevant docs
- Dart “Package dependencies” (git/path restrictions): [2]
- Dart “Publishing packages” (dependency requirement: hosted on default server): [3]
- Dart “Custom package repositories” (pub.dev disallows git/custom-hosted deps): [1]
Sources:
[1] https://dart.dev/tools/pub/custom-package-repositories
[2] https://dart.dev/tools/pub/dependencies
[3] https://dart.dev/tools/pub/publishing
Remove git dependency before publishing to pub.dev.
This package cannot be published to pub.dev with a git dependency. Pub.dev strictly requires all dependencies to be hosted on the default pub.dev server. Switch line 22-27 to a hosted version constraint like stac_core: ^1.3.0.
If temporary integration is required, move this dependency to a custom package repository configuration instead of pub.dev.
Proposed change
- # when approved, use stac_core: ^1.3.x
- stac_core:
- git:
- url: https://github.com/SuaMusica/stac.git
- path: packages/stac_core
- ref: devall
+ stac_core: ^1.3.0📝 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
| # when approved, use stac_core: ^1.3.x | |
| stac_core: | |
| git: | |
| url: https://github.com/SuaMusica/stac.git | |
| path: packages/stac_core | |
| ref: devall | |
| stac_core: ^1.3.0 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/stac_webview/pubspec.yaml` around lines 22 - 27, The pubspec
currently declares stac_core as a git dependency (stac_core: git: url: ... ref:
devall); replace that with a hosted constraint (e.g., stac_core: ^1.3.0) so the
package can be published to pub.dev, or if you need a temporary non-pub.dev
integration move the git entry out of pubspec and configure a custom hosted
repository instead; update the dependency declaration for stac_core accordingly
and remove the git block.
Contributor
There was a problem hiding this comment.
@felipecastrosales, understood! Since this is for development on the devall branch and you've already noted the intent to switch to a hosted version when approved, the git dependency is fine for now. 👍
🧠 Learnings used
Learnt from: akhil-ge0rge
Repo: StacDev/stac PR: 421
File: packages/stac_core/lib/widgets/sliver_padding/stac_sliver_padding.dart:51-72
Timestamp: 2026-01-27T19:05:50.669Z
Learning: In the stac_core package (packages/stac_core), explicit_to_json: true is configured globally in build.yaml for json_serializable. Therefore, individual JsonSerializable annotations do not need explicitToJson: true specified.
| git: | ||
| url: https://github.com/SuaMusica/stac.git | ||
| path: packages/stac_core | ||
| ref: devall |
Contributor
There was a problem hiding this comment.
Pin ref to a specific commit SHA rather than a mutable branch name.
ref: devall resolves to whatever HEAD of that branch is at fetch time. The dart pub tool doesn't support resolving versions against a git repository; it just fetches a specific revision of the git repository. Any push to devall will silently change the resolved code, making builds non-reproducible and introducing supply-chain risk.
🔧 Pin to a specific commit SHA
- ref: devall
+ ref: <full-commit-sha> # e.g. a1b2c3d4e5f6...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/stac/pubspec.yaml` at line 31, Replace the mutable git ref by
pinning to a specific commit SHA: locate the dependency entry that currently
reads "ref: devall" in the pubspec (the git dependency for this package) and
change the ref value to the exact commit SHA you want to lock (e.g., ref:
<commit-sha>), then run dart pub get to verify the resolved revision; ensure you
commit the updated pubspec.yaml so the dependency is reproducibly pinned.
Contributor
There was a problem hiding this comment.
@felipecastrosales, understood! I've noted your decision to keep the mutable branch reference for now.
Author
|
hey @divyanshub024 can u review this? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR introduces version management, platform validation, and several improvements to the STAC framework.
✨ Features
Version Management System
Build Number Support
buildNumberparameter toStac.initialize()StacRegistryPlatform Validation
Custom Color Parsing
parseCustomColorhook inStacRegistryStacService.setParseCustomColor()🔧 Improvements
StacDoubleandStacTextparsersstac_coreto local path dependency for development✅ Tests
StacVersion(277 lines)🧹 Chores
🔄 Migration Notes
If you're using
Stac.initialize(), you can now optionally pass abuildNumber:To use custom color parsing:
Summary by CodeRabbit
New Features
Tests