feat(stac_cli): Open-sourcing Stac CLI#456
Merged
divyanshub024 merged 7 commits intodevfrom Mar 1, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (4)
📝 WalkthroughWalkthroughAdds a new stac_cli package (CLI entrypoint, commands, services, models, utils) implementing OAuth, project management, build/deploy, and upgrade flows. Also updates example imports from Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as stac_cli (bin)
participant Browser
participant Google
participant Firebase
participant Config as ConfigService
User->>CLI: stac login
CLI->>CLI: start local callback server, generate PKCE
CLI->>Browser: open auth URL (state, code_challenge)
Browser->>Google: user authorizes
Google->>Browser: redirect with auth code
Browser->>CLI: callback POST with code
CLI->>Google: exchange code + verifier -> tokens
Google-->>CLI: access_token / id_token
CLI->>Firebase: sign-in with id_token
Firebase-->>CLI: firebase tokens (refresh, id)
CLI->>Config: store AuthToken
Config-->>CLI: persisted
CLI-->>User: login success
sequenceDiagram
participant User
participant CLI as stac_cli
participant BuildSvc as BuildService
participant FS as FileSystem
participant DartVM
participant Writer as JSONWriter
User->>CLI: stac build
CLI->>BuildSvc: build(projectPath)
BuildSvc->>FS: read lib/default_stac_options.dart
FS-->>BuildSvc: options
BuildSvc->>BuildSvc: scan source for annotated artifacts
BuildSvc->>DartVM: generate & run wrapper per artifact
DartVM-->>BuildSvc: stdout JSON
BuildSvc->>Writer: clean & normalize JSON
Writer-->>FS: write stac/.build/screens|themes/*.json
FS-->>BuildSvc: files persisted
BuildSvc-->>CLI: build completed
CLI-->>User: summary
sequenceDiagram
participant User
participant CLI as stac_cli
participant BuildSvc
participant DeploySvc
participant FS as FileSystem
participant HTTP as HttpClient
participant API as CloudAPI
User->>CLI: stac deploy (--skip-build?)
CLI->>BuildSvc: build(...) [if not skipped]
BuildSvc-->>CLI: artifacts ready
CLI->>DeploySvc: deploy(projectPath)
DeploySvc->>FS: read projectId from lib/default_stac_options.dart
FS-->>DeploySvc: projectId
DeploySvc->>FS: list stac/.build/screens & themes
FS-->>DeploySvc: file list
loop for each artifact
DeploySvc->>HTTP: POST /projects/{id}/screens|themes (payload)
HTTP->>API: request
API-->>HTTP: response
HTTP-->>DeploySvc: result
end
DeploySvc-->>CLI: deploy summary
CLI-->>User: deployment result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 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: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/movie_app/lib/default_stac_options.dart (1)
3-12:⚠️ Potential issue | 🟡 MinorDocumentation example uses outdated import path.
Line 3 correctly imports
stac_core.dart, but the doc example on line 12 still references the oldstac/stac.dartpath. This inconsistency may confuse developers copying the example.📝 Proposed fix
/// Example: /// ```dart /// import 'package:flutter/material.dart'; -/// import 'package:stac/stac.dart'; +/// import 'package:stac/stac_core.dart'; /// import 'default_stac_options.dart';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/movie_app/lib/default_stac_options.dart` around lines 3 - 12, Update the documentation example import to match the actual API surface: replace the outdated doc import "package:stac/stac.dart" with "package:stac/stac_core.dart" in the comment block of default_stac_options.dart so the example that shows initializing StacOptions/runApp is consistent with the real import; ensure the comment still references StacOptions and default_stac_options.dart correctly.
🟡 Minor comments (9)
packages/stac_cli/pubspec.yaml-7-8 (1)
7-8:⚠️ Potential issue | 🟡 MinorReconcile SDK version constraint discrepancy.
The
pubspec.yamlspecifiessdk: ^3.8.1, butpubspec.lockrestricts it todart: ">=3.10.0 <4.0.0". These constraints should be aligned. Updatepubspec.yamlto match the lock file's requirement (minimum 3.10.0) or regenerate the lock file if 3.8.1 compatibility is intended for users of this CLI tool.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/pubspec.yaml` around lines 7 - 8, The SDK constraint in pubspec.yaml (the environment: sdk field) conflicts with pubspec.lock's dart ">=3.10.0 <4.0.0"; either update pubspec.yaml's sdk constraint to ">=3.10.0 <4.0.0" to match the lock file, or regenerate pubspec.lock (flutter pub get / dart pub get) and CI artifacts if you intend to support ^3.8.1—modify the environment: sdk line in pubspec.yaml (or regenerate the lock) accordingly so both files align.packages/stac_cli/lib/src/commands/auth/login_command.dart-18-30 (1)
18-30:⚠️ Potential issue | 🟡 MinorThe
--interactiveflag is defined but never used.The flag is registered in the argument parser but
execute()doesn't readargResults['interactive']or pass it to_authService.login(). This appears to be incomplete implementation or dead code.🔧 Suggested fix if flag should control behavior
`@override` Future<int> execute() async { - await _authService.login(); + final interactive = argResults?['interactive'] as bool? ?? true; + await _authService.login(interactive: interactive); return 0; }This assumes
AuthService.login()accepts aninteractiveparameter. If non-interactive login isn't supported yet, consider removing the flag until it's implemented.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/lib/src/commands/auth/login_command.dart` around lines 18 - 30, The registered '--interactive' flag on LoginCommand is never read; update execute() to read argResults['interactive'] and pass that value into _authService.login(...) (e.g., call _authService.login(interactive: interactive)) so the flag controls behavior, or if AuthService.login does not accept such a parameter yet, remove the argParser.addFlag('interactive', ...) from the LoginCommand constructor to avoid dead code; ensure references are to LoginCommand, argParser, execute(), argResults['interactive'], and _authService.login.packages/stac_cli/lib/src/utils/file_utils.dart-10-15 (1)
10-15:⚠️ Potential issue | 🟡 MinorEmpty string fallback creates invalid paths.
When
HOMEorUSERPROFILEis unset, returning empty string causesconfigDirectoryto produce paths like/.stacor/AppData/Local/stac_cli, which are invalid or system-level directories. Consider throwing an exception or returning null to force callers to handle this edge case.🛡️ Suggested fix
static String get homeDirectory { if (Platform.isWindows) { - return Platform.environment['USERPROFILE'] ?? ''; + final home = Platform.environment['USERPROFILE']; + if (home == null || home.isEmpty) { + throw StateError('USERPROFILE environment variable is not set'); + } + return home; } - return Platform.environment['HOME'] ?? ''; + final home = Platform.environment['HOME']; + if (home == null || home.isEmpty) { + throw StateError('HOME environment variable is not set'); + } + return home; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/lib/src/utils/file_utils.dart` around lines 10 - 15, The current static getter homeDirectory returns an empty string when HOME/USERPROFILE is unset, producing invalid paths; change the implementation of the static getter homeDirectory to throw a descriptive runtime error (e.g., StateError) when the environment variable is missing instead of returning ''—include which variable was missing and the current Platform in the error message so callers (like configDirectory) must handle the missing-home case; update the getter signature/implementation in file_utils.dart (static String get homeDirectory) accordingly.packages/stac_cli/lib/src/utils/file_utils.dart-68-82 (1)
68-82:⚠️ Potential issue | 🟡 MinorMissing exception handling for malformed YAML.
loadYamlthrowsFormatExceptionon invalid YAML syntax. This would propagate up unexpectedly rather than returningnulllike other error cases.🛡️ Suggested fix
static Future<Map<String, dynamic>?> readYamlFile(String filePath) async { if (!await fileExists(filePath)) { return null; } - final content = await readFile(filePath); - final yaml = loadYaml(content); - - if (yaml is Map) { - return Map<String, dynamic>.from(yaml); + try { + final content = await readFile(filePath); + final yaml = loadYaml(content); + if (yaml is Map) { + return Map<String, dynamic>.from(yaml); + } + return null; + } on FormatException catch (e) { + ConsoleLogger.debug('Failed to parse YAML file $filePath: $e'); + return null; } - - return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/lib/src/utils/file_utils.dart` around lines 68 - 82, The readYamlFile method currently calls loadYaml and lets FormatException propagate; wrap the loadYaml(content) call (and the subsequent Map conversion) in a try-catch that catches FormatException (and optionally other parsing exceptions) and return null on error so malformed YAML behaves like other missing/error cases; update function readYamlFile to call fileExists and readFile as-is but protect loadYaml and Map<String, dynamic>.from(...) with a try { final yaml = loadYaml(content); if (yaml is Map) return Map<String, dynamic>.from(yaml); } on FormatException { return null; } (you can also log the exception if desired).packages/stac_cli/lib/src/commands/init_command.dart-217-219 (1)
217-219:⚠️ Potential issue | 🟡 MinorGenerated usage example references an outdated import path.
Line 218 still shows
package:stac/stac.dartwhile the generated file itself usesstac_core.dart. Aligning this avoids setup confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/lib/src/commands/init_command.dart` around lines 217 - 219, The generated example import uses the outdated package path "package:stac/stac.dart" whereas the rest of the generated file expects "stac_core.dart"; update the import line in the usage template to import "stac_core.dart" (replace the string "package:stac/stac.dart" with the correct "package:stac_core/stac_core.dart" or the local "stac_core.dart" used elsewhere), and ensure any other generated import lines (e.g., default_stac_options.dart) match the actual filenames used by InitCommand's template so the example compiles without manual edits.packages/stac_cli/lib/src/services/project_service.dart-82-87 (1)
82-87:⚠️ Potential issue | 🟡 MinorSlug normalization can produce edge hyphens.
Project names with surrounding whitespace can produce slugs like
-my-project-, which is risky for URL/slug validation.🔧 Proposed fix
static String _generateSlug(String name) { final slug = name + .trim() .toLowerCase() .replaceAll(_nonAlphanumericRegex, '') .replaceAll(_whitespaceRegex, '-') .replaceAll(_multipleHyphensRegex, '-') - .trim(); + .replaceAll(_edgeHyphensRegex, ''); if (slug.isEmpty) { throw StacException( 'Project name must contain at least one alphanumeric character', ); } return slug; } static final _nonAlphanumericRegex = RegExp(r'[^a-z0-9\s-]'); static final _whitespaceRegex = RegExp(r'\s+'); static final _multipleHyphensRegex = RegExp(r'-+'); + static final _edgeHyphensRegex = RegExp(r'^-+|-+$');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/lib/src/services/project_service.dart` around lines 82 - 87, The current slug build (variable slug using _nonAlphanumericRegex, _whitespaceRegex, _multipleHyphensRegex) can leave leading/trailing hyphens for names with surrounding whitespace; fix by either trimming the input before replacements or by adding a final step that strips leading/trailing hyphens (e.g. apply name = name.trim() before the replace chain or add a final replace using a regex like ^-+|-+$ to remove edge hyphens) so the resulting slug has no leading or trailing '-' characters.packages/stac_cli/lib/src/models/project/project.dart-48-79 (1)
48-79:⚠️ Potential issue | 🟡 Minor
copyWithcannot clear nullable fields tonull.Using
??for nullable properties (slug,description,deletedAt,subscription,uiLoads, etc.) prevents explicit nulling. Consider a sentinel-basedcopyWithpattern so callers can intentionally clear values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/lib/src/models/project/project.dart` around lines 48 - 79, The current Project.copyWith uses `??` so callers cannot explicitly set nullable fields (e.g., slug, description, deletedAt, subscription, uiLoads) to null; change the signature to use sentinel defaults (e.g., Object? sentinel = _undefined) for nullable params in Project.copyWith and update the body to check identity against the sentinel for each field (if param === sentinel then keep this.field else use the provided value, allowing provided null to clear the field). Add a private sentinel constant (e.g., const _undefined = Object()) in the file and apply this pattern for all nullable parameters (slug, description, deletedAt, subscription, uiLoads, etc.) so callers can intentionally clear values.packages/stac_cli/lib/src/models/project/subscription.dart-171-201 (1)
171-201:⚠️ Potential issue | 🟡 Minor
copyWithcannot explicitly null nullable properties.Because nullable fields use
param ?? this.field, callers cannot clear values likeupdatedAt,status, oralertThreshold. Please consider a sentinel-based approach for nullable params.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/lib/src/models/project/subscription.dart` around lines 171 - 201, copyWith currently uses the `param ?? this.field` pattern which prevents callers from explicitly nulling nullable properties (e.g., `updatedAt`, `status`, `alertThreshold`); change `Subscription.copyWith` to use a sentinel-based approach (or explicit wrapper/Optional) for nullable params so you can detect "parameter provided and set to null" vs "parameter omitted" — introduce a private sentinel object and make params typed as `Object?` (or use a dedicated wrapper) then check against the sentinel to decide whether to assign the new value (including null) or keep `this.updatedAt`/`this.status`/`this.alertThreshold` when the sentinel is passed; update the call site/constructor usage to accept the possibly-null values accordingly while still defaulting to existing fields when the sentinel is used.packages/stac_cli/lib/src/commands/build_command.dart-26-30 (1)
26-30:⚠️ Potential issue | 🟡 Minor
--validateis currently a no-op.The flag is declared but never read in
execute(), so--no-validatewon’t change behavior. Please either wire it intoBuildService.build(...)or remove the flag until supported.Also applies to: 42-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/lib/src/commands/build_command.dart` around lines 26 - 30, The 'validate' flag declared via argParser.addFlag('validate') is never read; update execute() to read the flag from argResults (e.g., final validate = argResults['validate'] as bool) and pass it into BuildService.build(...) (e.g., BuildService.build(..., validate: validate)) so --no-validate takes effect, or if validation support isn't implemented remove the argParser.addFlag('validate') declaration; apply the same fix for the other addFlag(...) in this file (the second flag declaration) so all declared flags are actually consumed by execute().
🧹 Nitpick comments (11)
packages/stac_cli/lib/src/utils/string_utils.dart (1)
23-31: Consider handling consecutive uppercase letters (acronyms).The current implementation splits each uppercase letter individually, which produces unexpected results for acronyms:
"HTTPClient"→"h_t_t_p_client"(instead of"http_client")"getURL"→"get_u_r_l"(instead of"get_url")If the CLI only processes standard camelCase identifiers without acronyms, this is fine. Otherwise, consider a regex that handles runs of uppercase letters:
♻️ Proposed fix for acronym handling
/// Converts a camelCase/PascalCase string to snake_case. static String toSnakeCase(String value) { return value .replaceAllMapped( - RegExp(r'[A-Z]'), - (match) => '_${match.group(0)!.toLowerCase()}', + RegExp(r'([A-Z]+)([A-Z][a-z])|([a-z])([A-Z])'), + (match) { + if (match.group(1) != null) { + return '${match.group(1)!.toLowerCase()}_${match.group(2)!.toLowerCase()}'; + } + return '${match.group(3)}_${match.group(4)!.toLowerCase()}'; + }, ) - .replaceFirst(RegExp(r'^_'), ''); + .toLowerCase(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/lib/src/utils/string_utils.dart` around lines 23 - 31, toSnakeCase currently inserts an underscore before every single uppercase letter, breaking acronyms (e.g. "HTTPClient" → "h_t_t_p_client"); update the toSnakeCase implementation to treat consecutive uppercase letters as a single acronym run: detect runs of uppercase letters (optionally followed by a lowercase) and runs that start after a lowercase/digit, insert a single underscore before the whole run and lowercase the result. Modify the RegExp used in toSnakeCase (and its replaceAllMapped logic) to match uppercase sequences rather than individual letters (for example using lookarounds such as '(?<=[a-z0-9])([A-Z]+)' and '([A-Z]+)(?=[A-Z][a-z]|$)') and transform matches to '_'+match.toLowerCase(), then trim any leading underscore as before.packages/stac_cli/pubspec.yaml (1)
33-33: Consider removing or clarifying the trailing comment.The comment
# stac_cli/pubspec_overrides.yamlappears to be an orphaned artifact, possibly from melos setup or development notes. If it's intended to reference the overrides file for developers, consider adding context (e.g.,# See pubspec_overrides.yaml for local development overrides) or remove it if unnecessary.🔧 Suggested fix
- -# stac_cli/pubspec_overrides.yamlOr, if keeping it for developer reference:
-# stac_cli/pubspec_overrides.yaml +# Note: See pubspec_overrides.yaml for melos-managed local path overrides during development🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/pubspec.yaml` at line 33, The trailing orphan comment "# stac_cli/pubspec_overrides.yaml" should be removed or clarified; either delete the line if it's unnecessary or replace it with a descriptive comment such as "See pubspec_overrides.yaml for local development overrides" so its intent is clear; locate the comment string "# stac_cli/pubspec_overrides.yaml" in the pubspec.yaml and update accordingly.packages/stac_cli/README.md (1)
35-35: Clarify environment configuration for end users.The reference to
lib/src/config/env.dartpoints to an internal implementation file. For users who install the CLI globally viadart pub global activate, this path won't be directly accessible. Consider clarifying whether this is intended for contributors/developers only, or provide the actual mechanism for users to switch environments (e.g., a CLI flag or environment variable).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/README.md` at line 35, The README currently points users to the internal file lib/src/config/env.dart and the symbol currentEnvironment; update the documentation to clarify intent: indicate that lib/src/... and currentEnvironment are for contributors/developers only, and then document the user-facing mechanism to change environments (e.g., describe the CLI flag or environment variable to use when running the installed CLI, or explain how to rebuild with a different environment if applicable). Mention the exact symbol currentEnvironment and the file lib/src/config/env.dart so maintainers can find the developer-only option, and add an example command showing the supported user method (CLI flag or ENV var) for global installs.packages/stac_cli/lib/src/models/auth_token.dart (1)
35-42: Add defensive null checks infromJsonfor clearer error messages.If the stored token JSON is corrupted or missing required fields, the current implementation will throw unclear cast errors. Consider validating required fields before construction.
♻️ Proposed enhancement
factory AuthToken.fromJson(Map<String, dynamic> json) { + final accessToken = json['access_token']; + final expiresAt = json['expires_at']; + final scopes = json['scopes']; + + if (accessToken is! String) { + throw FormatException('Invalid or missing access_token'); + } + if (expiresAt is! String) { + throw FormatException('Invalid or missing expires_at'); + } + if (scopes is! List) { + throw FormatException('Invalid or missing scopes'); + } + return AuthToken( - accessToken: json['access_token'] as String, + accessToken: accessToken, refreshToken: json['refresh_token'] as String?, - expiresAt: DateTime.parse(json['expires_at'] as String), - scopes: List<String>.from(json['scopes'] as List), + expiresAt: DateTime.parse(expiresAt), + scopes: List<String>.from(scopes), ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/lib/src/models/auth_token.dart` around lines 35 - 42, Add defensive null/type checks in AuthToken.fromJson: validate required keys access_token, expires_at, and scopes exist and are of expected types before calling the AuthToken constructor; parse expires_at with try/catch to throw a clear FormatException if invalid; allow refresh_token to be optional but validate its type if present; on missing/invalid required fields throw descriptive exceptions (e.g., "Missing or invalid access_token in AuthToken.fromJson") to replace unclear cast errors.packages/stac_cli/lib/src/exceptions/stac_exception.dart (1)
9-10: Consider includingcauseintoString()for better debugging.When a
causeis present, including it in the string representation would help with debugging and error logs.♻️ Proposed enhancement
`@override` - String toString() => 'StacException: $message'; + String toString() => + cause != null ? 'StacException: $message (cause: $cause)' : 'StacException: $message';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/lib/src/exceptions/stac_exception.dart` around lines 9 - 10, The StacException.toString() currently only returns 'StacException: $message' and should include the optional cause for better debugging; update the StacException class so its toString() method (toString()) appends the cause when present (e.g., "StacException: $message; cause: $cause" or similar), ensuring null-safe handling of the cause and preserving the existing message formatting.packages/stac_cli/lib/src/models/project/project_access.dart (1)
38-40:isEqualmethod is redundant for Dart enums.Dart enums can be compared directly with
==, soisEqualprovides no additional value over standard equality comparison. Consider removing it to reduce API surface.// Current usage: ProjectAccess.admin.isEqual(ProjectAccess.admin) // true // Simpler: ProjectAccess.admin == ProjectAccess.admin // true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/lib/src/models/project/project_access.dart` around lines 38 - 40, Remove the redundant isEqual method from the ProjectAccess enum and update any call sites to use Dart's native equality (==) instead; search for the symbol isEqual in the codebase, delete the isEqual(ProjectAccess other) => value == other.value implementation from the ProjectAccess declaration, and replace usages like someAccess.isEqual(otherAccess) with someAccess == otherAccess so the enum relies on built-in enum comparison.packages/stac_cli/lib/src/config/env.dart (2)
7-10:currentEnvironmentappears unused.The
Environmentenum andcurrentEnvironmentconstant are defined but not referenced anywhere in the configuration logic. If this is a placeholder for future environment switching, consider adding a TODO comment or removing it to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/lib/src/config/env.dart` around lines 7 - 10, The enum Environment and constant currentEnvironment are defined but unused; either remove the unused enum/constant (Environment, currentEnvironment) to avoid dead code, or keep them and add a TODO comment explaining intended future use and link it to configuration switching (e.g., note which config functions should read currentEnvironment) so reviewers know it’s deliberate—update the declaration accordingly to reflect chosen approach.
25-30: Clarify intent:requiredon nullable field.Using
requiredwithString?means the caller must explicitly pass the parameter (even if null), but doesn't enforce a non-null value. This works correctly with how_valueis called, but consider whether making it a positional optional parameter would better express the optionality.💡 Alternative constructor signature
const EnvConfig({ required this.baseApiUrl, required this.googleOAuthClientId, - required this.googleOAuthClientSecret, + this.googleOAuthClientSecret, required this.firebaseWebApiKey, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/lib/src/config/env.dart` around lines 25 - 30, The constructor currently declares nullable fields (baseApiUrl, googleOAuthClientId, googleOAuthClientSecret, firebaseWebApiKey) but marks them as required in EnvConfig, which forces callers to pass explicit nulls; decide intended semantics and update the constructor accordingly: if these values are truly optional, remove the required keyword from those named parameters (or convert them to an optional positional parameter list) so callers may omit them; if they must be present, change the types to non-nullable String and enforce/validate in the EnvConfig constructor (or assert) so callers cannot pass null. Update references to EnvConfig and any usage of _value to match the chosen signature.packages/stac_cli/lib/src/utils/console_logger.dart (1)
122-126: Misleading function name:_leftPaddingForCenterdoesn't center content.The function divides by 4 (placing content ~25% from left) rather than by 2 (true centering). If this is intentional for visual aesthetics, consider renaming to
_leftPaddingForBanneror similar to avoid confusion.🔧 Suggested rename
- static int _leftPaddingForCenter(int contentLength, int totalWidth) { + static int _leftPaddingForBanner(int contentLength, int totalWidth) { final remaining = totalWidth - contentLength; if (remaining <= 0) return 0; return remaining ~/ 4; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/lib/src/utils/console_logger.dart` around lines 122 - 126, The function _leftPaddingForCenter currently computes left padding as remaining ~/ 4 which yields a ~25% offset; either fix the logic to actually center by returning remaining ~/ 2, or if the 25% offset was intentional, rename the function (e.g., _leftPaddingForBanner or _leftOffsetForAesthetic) and update any callers and comments accordingly so the name matches behavior; modify the return expression in _leftPaddingForCenter (or the renamed function) and adjust references to the function name across the file.packages/stac_cli/lib/src/models/project/ui_loads.dart (1)
65-85:copyWithcannot clear nullable fields.Using
??for nullable properties prevents explicit reset-to-null updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/lib/src/models/project/ui_loads.dart` around lines 65 - 85, The copyWith currently uses `??` which prevents setting nullable fields to null; update `UiLoads.copyWith` to accept sentinel-based parameters (e.g. change each parameter type to `Object? param = _unset` using a private const `_unset = Object()`), then inside the constructor use a ternary that checks `param == _unset ? this.field : param as <type>?` for each field (reference the parameters/currentPeriodUiLoadCount, lastUiLoadCountFlushed, lastUiLoadsFlushedDelta, lastUiLoadsCountFlushedAt, lastUiLoadsUploadError, lifetimeUiLoadCount and the `UiLoads` constructor) so callers can explicitly pass null to clear a nullable property.packages/stac_cli/lib/src/services/upgrade_service.dart (1)
96-99:upgradeToacceptsversionbut never uses it.This makes the API misleading. Either validate that
downloadUrlmatchesversion, or removeversionfrom the method signature.Also applies to: 106-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/lib/src/services/upgrade_service.dart` around lines 96 - 99, The upgradeTo method accepts a version parameter but never uses it; either remove the unused version parameter from upgradeTo (and from all its callers) to keep the API accurate, or implement explicit validation inside upgradeTo (e.g., parse the version from downloadUrl or fetch the package metadata and compare it to the provided version) and throw/return an error when they don't match; update the upgradeTo signature and all call sites (and the related duplicate occurrence) accordingly so the method and its callers stay consistent.
🤖 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_cli/lib/src/commands/base_command.dart`:
- Around line 48-51: The catch currently maps every StacException to
NotAuthenticatedException; instead, inspect the caught StacException (symbol:
StacException) for signs of authentication failure (e.g., status code == 401,
error code/message indicating "unauth"/"unauthorized", or a dedicated
AuthException subclass) and only throw NotAuthenticatedException when that
condition is met; for all other StacException cases preserve the original error
and stacktrace (use rethrow or throw the original exception) so unrelated
operational failures are not masked (update the catch block where StacException
is handled and replace the unconditional throw const NotAuthenticatedException()
with a conditional throw based on the exception’s auth indicators).
In `@packages/stac_cli/lib/src/commands/init_command.dart`:
- Around line 62-65: The code treats a user-initiated "skip project" (when
_selectOrCreateProjectInteractively() returns null) as an error by returning
exit code 1; change those return values to 0 to indicate successful intentional
skip. Update each branch where Project? project = await
_selectOrCreateProjectInteractively(); followed by if (project == null) return
1; (including occurrences around _selectOrCreateProjectInteractively at the
locations indicated and similar checks at the other noted spots) so they return
0 instead of 1, preserving normal flow when the user chooses to not set up a
default project.
- Around line 205-231: The generated dartConfig embeds project.name,
project.description, and project.id directly into single-quoted Dart string
literals which breaks when values contain quotes or backslashes; update the
generation in init_command.dart to escape these values before interpolation
(e.g., replace backslashes and single quotes and normalize newlines) or use a
safe Dart string-encoding helper so the StacOptions literal for
defaultStacOptions is always valid; locate the dartConfig template and the
interpolation sites for '${project.name}', '${project.description}', and
'${project.id}' and apply the escaping helper there.
In `@packages/stac_cli/lib/src/commands/project/list_command.dart`:
- Around line 1-3: The JSON flag currently prints Dart Map/List via toString(),
which is not valid JSON; update the code paths that output results (e.g., the
ListCommand run/execute method and the other occurrences around lines 35-38) to
import dart:convert and use jsonEncode(...) when --json is set instead of
calling toString(), ensuring any top-level object is serializable (convert
Maps/Lists/primitive types) before printing.
In `@packages/stac_cli/lib/src/commands/upgrade_command.dart`:
- Around line 16-19: The upgrade command's option registration using
argParser.addOption for 'version' currently sets abbr: 'v', which conflicts with
the global --verbose flag; remove the abbr (or change it to an unused short
flag) so the option becomes argParser.addOption('version', help: ...) (or use a
different letter not used globally), ensuring calls to the upgrade command
(e.g., in upgrade_command.dart where 'version' is defined) no longer collide
with the global --verbose defined in stac_cli.dart.
In `@packages/stac_cli/lib/src/services/auth_service.dart`:
- Around line 133-143: The HTTP refresh/login calls in auth_service.dart use
client.postUrl(uri), request.close(), and response.transform(...).join() with no
timeouts, so flows can hang; set a sensible connection timeout on the HttpClient
(client.connectionTimeout = Duration(...)) and wrap the async waits for
client.postUrl(uri), request.close(), and
response.transform(utf8.decoder).join() with Future.timeout(Duration(...)) (or
catch TimeoutException) so stalled Google/Firebase endpoints will error fast;
update the refresh/token/status code paths that use client, request.close(), and
responseBody retrieval to handle and propagate timeout errors appropriately.
- Around line 134-140: The token refresh request currently sets
request.headers.contentType to JSON and writes a JSON payload (payload with
'grant_type' and 'refresh_token'), but Firebase requires
application/x-www-form-urlencoded; change the content type to
ContentType('application', 'x-www-form-urlencoded') and send the body as a
form-encoded string (e.g., URL-encoded key=value pairs derived from the payload
map) instead of json.encode; update the code around the request write call that
constructs and sends the payload (the variables `request`, `payload`, and
`refreshToken`) so the server receives form-encoded data.
In `@packages/stac_cli/lib/src/services/build_service.dart`:
- Around line 84-90: The artifact conversion is recalculating the project root
from Directory.current, which breaks builds that pass build(projectPath: ...);
update the code so the project root is threaded into the conversion functions
instead of recomputing it: modify _convertCallableToJson to accept a projectPath
(or projectRoot) parameter and use Directory(projectPath) when resolving the
root, then change all call sites that invoke _convertCallableToJson (including
where _processArtifacts calls it) to pass the build(projectPath: ...) value
through (e.g., add projectPath argument to _processArtifacts and its callers
such as the screens/artifacts handling and the other listed locations) so
conversions run in the correct project context. Ensure function signatures and
downstream usages (_processArtifacts, _convertCallableToJson, any callers of
build(projectPath: ...)) are updated accordingly.
In `@packages/stac_cli/lib/src/services/deploy_service.dart`:
- Around line 30-31: The deploy code currently hardcodes the build directory via
buildDirPath and buildDir; change it to read the configured build output path
from the project's STAC options (the value defined in default_stac_options.dart)
instead of using 'stac/.build'. Locate where buildDirPath/buildDir is created in
DeployService (or the deploy method) and replace the hardcoded path with the
configured option (e.g., stacOptions.buildOutput or similar), falling back to
'stac/.build' only if the option is absent; ensure you import/use the project
STAC options provider or accept the build output path as a constructor/parameter
so the deploy service uses the configured build output.
- Around line 112-120: The deployment currently only logs warnings on partial
failures and returns success; update the logic after computing totalFailures
(the block using totalFailures, screenSuccess/screenFail,
themeSuccess/themeFail) so that when totalFailures > 0 the command exits with a
non-zero status instead of returning normally — e.g., log an error via
ConsoleLogger.error and then call exit(1) (from dart:io) or throw a specific
exception so the CLI returns a failure code; ensure this change is applied in
the same function where totalFailures is computed to guarantee `stac deploy`
exits non-zero on any upload failures.
In `@packages/stac_cli/lib/src/services/upgrade_service.dart`:
- Around line 329-339: Stage the new binary to a temporary path first instead of
deleting the old one: copy sourcePath to a temp file (e.g., "${destPath}.tmp" or
use File.createTemp) and verify the copy succeeded, then atomically replace the
existing binary by renaming/moving the temp file to destPath (use File.rename or
platform-appropriate replace) so the swap is atomic; update the logic around
isWindows/oldFile/destPath/sourcePath to stop deleting oldFile before staging
and only remove or overwrite via the atomic rename after successful staging, and
ensure file permissions (executable bit) are preserved or set before the final
rename.
- Around line 195-203: Check for a null return from detectOsArch() before
iterating assets and fail fast: call detectOsArch(), if it returns null throw
StacException('Unsupported platform') rather than force-unwrapping osArch in the
for-loop; then proceed to iterate and use _isMatchingAsset(name, osArch) and
assign downloadUrl/binaryAssetName only when osArch is non-null.
- Around line 248-252: The current _isMatchingAsset(String assetName,
Map<String, String> osArch) is too permissive (contains checks) and can match
checksum/signature assets; tighten it by requiring both os and arch to match
case-insensitively AND the filename to have an allowed binary archive/installer
extension (or not have a disallowed extension). Update _isMatchingAsset to
normalize assetName and os/arch to lower-case, check assetName.contains(os) &&
assetName.contains(arch), and additionally require
assetName.match(RegExp(r'\.(tar\.gz|tar\.bz2|zip|gz|exe|dmg|deb|rpm|tgz)$')) or
ensure it does not endWith('.sha256')/'.sig'/' .asc' (pick one approach) so
checksum/signature files are excluded.
In `@packages/stac_cli/lib/src/utils/date_time_utils.dart`:
- Around line 31-33: The Firestore timestamp reconstruction uses
DateTime.fromMillisecondsSinceEpoch without specifying UTC, causing local-time
shifts; update the DateTime construction in the conversion code (the
DateTime.fromMillisecondsSinceEpoch(...) call in date_time_utils.dart) to pass
isUtc: true so the returned DateTime reflects the original UTC instant (i.e.,
DateTime.fromMillisecondsSinceEpoch(<ms>, isUtc: true)).
In `@packages/stac_cli/lib/src/utils/http_client.dart`:
- Around line 93-108: The post method currently catches any non-DioException in
a generic catch and wraps it into a new StacException, which re-wraps existing
StacException instances and loses original details; update the post(String path,
{dynamic data}) method so that in the final catch block you detect if the caught
error is already a StacException and rethrow it unchanged, otherwise wrap
non-StacException errors into a new StacException (matching the behavior used by
get/put/delete); locate the post method and alter the final catch to check `if
(e is StacException) rethrow;` before throwing a new StacException for other
errors.
---
Outside diff comments:
In `@examples/movie_app/lib/default_stac_options.dart`:
- Around line 3-12: Update the documentation example import to match the actual
API surface: replace the outdated doc import "package:stac/stac.dart" with
"package:stac/stac_core.dart" in the comment block of default_stac_options.dart
so the example that shows initializing StacOptions/runApp is consistent with the
real import; ensure the comment still references StacOptions and
default_stac_options.dart correctly.
---
Minor comments:
In `@packages/stac_cli/lib/src/commands/auth/login_command.dart`:
- Around line 18-30: The registered '--interactive' flag on LoginCommand is
never read; update execute() to read argResults['interactive'] and pass that
value into _authService.login(...) (e.g., call _authService.login(interactive:
interactive)) so the flag controls behavior, or if AuthService.login does not
accept such a parameter yet, remove the argParser.addFlag('interactive', ...)
from the LoginCommand constructor to avoid dead code; ensure references are to
LoginCommand, argParser, execute(), argResults['interactive'], and
_authService.login.
In `@packages/stac_cli/lib/src/commands/build_command.dart`:
- Around line 26-30: The 'validate' flag declared via
argParser.addFlag('validate') is never read; update execute() to read the flag
from argResults (e.g., final validate = argResults['validate'] as bool) and pass
it into BuildService.build(...) (e.g., BuildService.build(..., validate:
validate)) so --no-validate takes effect, or if validation support isn't
implemented remove the argParser.addFlag('validate') declaration; apply the same
fix for the other addFlag(...) in this file (the second flag declaration) so all
declared flags are actually consumed by execute().
In `@packages/stac_cli/lib/src/commands/init_command.dart`:
- Around line 217-219: The generated example import uses the outdated package
path "package:stac/stac.dart" whereas the rest of the generated file expects
"stac_core.dart"; update the import line in the usage template to import
"stac_core.dart" (replace the string "package:stac/stac.dart" with the correct
"package:stac_core/stac_core.dart" or the local "stac_core.dart" used
elsewhere), and ensure any other generated import lines (e.g.,
default_stac_options.dart) match the actual filenames used by InitCommand's
template so the example compiles without manual edits.
In `@packages/stac_cli/lib/src/models/project/project.dart`:
- Around line 48-79: The current Project.copyWith uses `??` so callers cannot
explicitly set nullable fields (e.g., slug, description, deletedAt,
subscription, uiLoads) to null; change the signature to use sentinel defaults
(e.g., Object? sentinel = _undefined) for nullable params in Project.copyWith
and update the body to check identity against the sentinel for each field (if
param === sentinel then keep this.field else use the provided value, allowing
provided null to clear the field). Add a private sentinel constant (e.g., const
_undefined = Object()) in the file and apply this pattern for all nullable
parameters (slug, description, deletedAt, subscription, uiLoads, etc.) so
callers can intentionally clear values.
In `@packages/stac_cli/lib/src/models/project/subscription.dart`:
- Around line 171-201: copyWith currently uses the `param ?? this.field` pattern
which prevents callers from explicitly nulling nullable properties (e.g.,
`updatedAt`, `status`, `alertThreshold`); change `Subscription.copyWith` to use
a sentinel-based approach (or explicit wrapper/Optional) for nullable params so
you can detect "parameter provided and set to null" vs "parameter omitted" —
introduce a private sentinel object and make params typed as `Object?` (or use a
dedicated wrapper) then check against the sentinel to decide whether to assign
the new value (including null) or keep
`this.updatedAt`/`this.status`/`this.alertThreshold` when the sentinel is
passed; update the call site/constructor usage to accept the possibly-null
values accordingly while still defaulting to existing fields when the sentinel
is used.
In `@packages/stac_cli/lib/src/services/project_service.dart`:
- Around line 82-87: The current slug build (variable slug using
_nonAlphanumericRegex, _whitespaceRegex, _multipleHyphensRegex) can leave
leading/trailing hyphens for names with surrounding whitespace; fix by either
trimming the input before replacements or by adding a final step that strips
leading/trailing hyphens (e.g. apply name = name.trim() before the replace chain
or add a final replace using a regex like ^-+|-+$ to remove edge hyphens) so the
resulting slug has no leading or trailing '-' characters.
In `@packages/stac_cli/lib/src/utils/file_utils.dart`:
- Around line 10-15: The current static getter homeDirectory returns an empty
string when HOME/USERPROFILE is unset, producing invalid paths; change the
implementation of the static getter homeDirectory to throw a descriptive runtime
error (e.g., StateError) when the environment variable is missing instead of
returning ''—include which variable was missing and the current Platform in the
error message so callers (like configDirectory) must handle the missing-home
case; update the getter signature/implementation in file_utils.dart (static
String get homeDirectory) accordingly.
- Around line 68-82: The readYamlFile method currently calls loadYaml and lets
FormatException propagate; wrap the loadYaml(content) call (and the subsequent
Map conversion) in a try-catch that catches FormatException (and optionally
other parsing exceptions) and return null on error so malformed YAML behaves
like other missing/error cases; update function readYamlFile to call fileExists
and readFile as-is but protect loadYaml and Map<String, dynamic>.from(...) with
a try { final yaml = loadYaml(content); if (yaml is Map) return Map<String,
dynamic>.from(yaml); } on FormatException { return null; } (you can also log the
exception if desired).
In `@packages/stac_cli/pubspec.yaml`:
- Around line 7-8: The SDK constraint in pubspec.yaml (the environment: sdk
field) conflicts with pubspec.lock's dart ">=3.10.0 <4.0.0"; either update
pubspec.yaml's sdk constraint to ">=3.10.0 <4.0.0" to match the lock file, or
regenerate pubspec.lock (flutter pub get / dart pub get) and CI artifacts if you
intend to support ^3.8.1—modify the environment: sdk line in pubspec.yaml (or
regenerate the lock) accordingly so both files align.
---
Nitpick comments:
In `@packages/stac_cli/lib/src/config/env.dart`:
- Around line 7-10: The enum Environment and constant currentEnvironment are
defined but unused; either remove the unused enum/constant (Environment,
currentEnvironment) to avoid dead code, or keep them and add a TODO comment
explaining intended future use and link it to configuration switching (e.g.,
note which config functions should read currentEnvironment) so reviewers know
it’s deliberate—update the declaration accordingly to reflect chosen approach.
- Around line 25-30: The constructor currently declares nullable fields
(baseApiUrl, googleOAuthClientId, googleOAuthClientSecret, firebaseWebApiKey)
but marks them as required in EnvConfig, which forces callers to pass explicit
nulls; decide intended semantics and update the constructor accordingly: if
these values are truly optional, remove the required keyword from those named
parameters (or convert them to an optional positional parameter list) so callers
may omit them; if they must be present, change the types to non-nullable String
and enforce/validate in the EnvConfig constructor (or assert) so callers cannot
pass null. Update references to EnvConfig and any usage of _value to match the
chosen signature.
In `@packages/stac_cli/lib/src/exceptions/stac_exception.dart`:
- Around line 9-10: The StacException.toString() currently only returns
'StacException: $message' and should include the optional cause for better
debugging; update the StacException class so its toString() method (toString())
appends the cause when present (e.g., "StacException: $message; cause: $cause"
or similar), ensuring null-safe handling of the cause and preserving the
existing message formatting.
In `@packages/stac_cli/lib/src/models/auth_token.dart`:
- Around line 35-42: Add defensive null/type checks in AuthToken.fromJson:
validate required keys access_token, expires_at, and scopes exist and are of
expected types before calling the AuthToken constructor; parse expires_at with
try/catch to throw a clear FormatException if invalid; allow refresh_token to be
optional but validate its type if present; on missing/invalid required fields
throw descriptive exceptions (e.g., "Missing or invalid access_token in
AuthToken.fromJson") to replace unclear cast errors.
In `@packages/stac_cli/lib/src/models/project/project_access.dart`:
- Around line 38-40: Remove the redundant isEqual method from the ProjectAccess
enum and update any call sites to use Dart's native equality (==) instead;
search for the symbol isEqual in the codebase, delete the isEqual(ProjectAccess
other) => value == other.value implementation from the ProjectAccess
declaration, and replace usages like someAccess.isEqual(otherAccess) with
someAccess == otherAccess so the enum relies on built-in enum comparison.
In `@packages/stac_cli/lib/src/models/project/ui_loads.dart`:
- Around line 65-85: The copyWith currently uses `??` which prevents setting
nullable fields to null; update `UiLoads.copyWith` to accept sentinel-based
parameters (e.g. change each parameter type to `Object? param = _unset` using a
private const `_unset = Object()`), then inside the constructor use a ternary
that checks `param == _unset ? this.field : param as <type>?` for each field
(reference the parameters/currentPeriodUiLoadCount, lastUiLoadCountFlushed,
lastUiLoadsFlushedDelta, lastUiLoadsCountFlushedAt, lastUiLoadsUploadError,
lifetimeUiLoadCount and the `UiLoads` constructor) so callers can explicitly
pass null to clear a nullable property.
In `@packages/stac_cli/lib/src/services/upgrade_service.dart`:
- Around line 96-99: The upgradeTo method accepts a version parameter but never
uses it; either remove the unused version parameter from upgradeTo (and from all
its callers) to keep the API accurate, or implement explicit validation inside
upgradeTo (e.g., parse the version from downloadUrl or fetch the package
metadata and compare it to the provided version) and throw/return an error when
they don't match; update the upgradeTo signature and all call sites (and the
related duplicate occurrence) accordingly so the method and its callers stay
consistent.
In `@packages/stac_cli/lib/src/utils/console_logger.dart`:
- Around line 122-126: The function _leftPaddingForCenter currently computes
left padding as remaining ~/ 4 which yields a ~25% offset; either fix the logic
to actually center by returning remaining ~/ 2, or if the 25% offset was
intentional, rename the function (e.g., _leftPaddingForBanner or
_leftOffsetForAesthetic) and update any callers and comments accordingly so the
name matches behavior; modify the return expression in _leftPaddingForCenter (or
the renamed function) and adjust references to the function name across the
file.
In `@packages/stac_cli/lib/src/utils/string_utils.dart`:
- Around line 23-31: toSnakeCase currently inserts an underscore before every
single uppercase letter, breaking acronyms (e.g. "HTTPClient" →
"h_t_t_p_client"); update the toSnakeCase implementation to treat consecutive
uppercase letters as a single acronym run: detect runs of uppercase letters
(optionally followed by a lowercase) and runs that start after a
lowercase/digit, insert a single underscore before the whole run and lowercase
the result. Modify the RegExp used in toSnakeCase (and its replaceAllMapped
logic) to match uppercase sequences rather than individual letters (for example
using lookarounds such as '(?<=[a-z0-9])([A-Z]+)' and
'([A-Z]+)(?=[A-Z][a-z]|$)') and transform matches to '_'+match.toLowerCase(),
then trim any leading underscore as before.
In `@packages/stac_cli/pubspec.yaml`:
- Line 33: The trailing orphan comment "# stac_cli/pubspec_overrides.yaml"
should be removed or clarified; either delete the line if it's unnecessary or
replace it with a descriptive comment such as "See pubspec_overrides.yaml for
local development overrides" so its intent is clear; locate the comment string
"# stac_cli/pubspec_overrides.yaml" in the pubspec.yaml and update accordingly.
In `@packages/stac_cli/README.md`:
- Line 35: The README currently points users to the internal file
lib/src/config/env.dart and the symbol currentEnvironment; update the
documentation to clarify intent: indicate that lib/src/... and
currentEnvironment are for contributors/developers only, and then document the
user-facing mechanism to change environments (e.g., describe the CLI flag or
environment variable to use when running the installed CLI, or explain how to
rebuild with a different environment if applicable). Mention the exact symbol
currentEnvironment and the file lib/src/config/env.dart so maintainers can find
the developer-only option, and add an example command showing the supported user
method (CLI flag or ENV var) for global installs.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/stac_cli/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (49)
.gitignoreexamples/movie_app/lib/default_stac_options.dartexamples/movie_app/lib/widgets/movie_carousel/movie_carousel.dartexamples/movie_app/pubspec.yamlpackages/stac_cli/.gitignorepackages/stac_cli/LICENSEpackages/stac_cli/README.mdpackages/stac_cli/analysis_options.yamlpackages/stac_cli/bin/stac_cli.dartpackages/stac_cli/lib/src/commands/auth/login_command.dartpackages/stac_cli/lib/src/commands/auth/logout_command.dartpackages/stac_cli/lib/src/commands/auth/status_command.dartpackages/stac_cli/lib/src/commands/base_command.dartpackages/stac_cli/lib/src/commands/build_command.dartpackages/stac_cli/lib/src/commands/deploy_command.dartpackages/stac_cli/lib/src/commands/init_command.dartpackages/stac_cli/lib/src/commands/project/create_command.dartpackages/stac_cli/lib/src/commands/project/list_command.dartpackages/stac_cli/lib/src/commands/project_command.dartpackages/stac_cli/lib/src/commands/upgrade_command.dartpackages/stac_cli/lib/src/config/env.dartpackages/stac_cli/lib/src/exceptions/auth_exception.dartpackages/stac_cli/lib/src/exceptions/build_exception.dartpackages/stac_cli/lib/src/exceptions/stac_exception.dartpackages/stac_cli/lib/src/models/auth_token.dartpackages/stac_cli/lib/src/models/project/project.dartpackages/stac_cli/lib/src/models/project/project.g.dartpackages/stac_cli/lib/src/models/project/project_access.dartpackages/stac_cli/lib/src/models/project/subscription.dartpackages/stac_cli/lib/src/models/project/subscription.g.dartpackages/stac_cli/lib/src/models/project/ui_loads.dartpackages/stac_cli/lib/src/models/project/ui_loads.g.dartpackages/stac_cli/lib/src/models/stac_dsl_artifact.dartpackages/stac_cli/lib/src/services/auth_service.dartpackages/stac_cli/lib/src/services/build_service.dartpackages/stac_cli/lib/src/services/config_service.dartpackages/stac_cli/lib/src/services/deploy_service.dartpackages/stac_cli/lib/src/services/project_service.dartpackages/stac_cli/lib/src/services/upgrade_service.dartpackages/stac_cli/lib/src/utils/console_logger.dartpackages/stac_cli/lib/src/utils/date_time_utils.dartpackages/stac_cli/lib/src/utils/file_utils.dartpackages/stac_cli/lib/src/utils/http_client.dartpackages/stac_cli/lib/src/utils/oauth_pkce.dartpackages/stac_cli/lib/src/utils/string_utils.dartpackages/stac_cli/lib/src/version.dartpackages/stac_cli/lib/stac_cli.dartpackages/stac_cli/pubspec.yamlpackages/stac_cli/pubspec_overrides.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/stac_cli/lib/src/utils/file_utils.dart (1)
56-58: Make writes resilient by creating the parent directory.
writeFilecurrently assumes the parent path already exists. Creating it here prevents avoidableFileSystemExceptionfailures.♻️ Proposed change
static Future<void> writeFile(String filePath, String content) async { - await File(filePath).writeAsString(content); + final file = File(filePath); + await file.parent.create(recursive: true); + await file.writeAsString(content); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/lib/src/utils/file_utils.dart` around lines 56 - 58, The writeFile method assumes the parent directory exists; update writeFile to create the parent directory before writing by resolving the parent via File(filePath).parent and calling create(recursive: true) (handle async) prior to invoking writeAsString; keep using the same writeFile signature and await semantics so File(filePath).writeAsString(content) runs after the parent directory creation.packages/stac_cli/lib/src/config/env.dart (1)
9-10: Avoid compile-time environment switching for dev/prod.
currentEnvironmentis pinned to prod (Line 10), so local/dev routing requires source edits. Prefer runtime selection (for exampleSTAC_ENV) with a safe default to reduce accidental production targeting.♻️ Proposed change
-// Flip this to switch environments. -const Environment currentEnvironment = Environment.prod; +// Runtime-selectable environment. Defaults to prod. +Environment get currentEnvironment { + switch (_value('STAC_ENV')?.toLowerCase()) { + case 'dev': + return Environment.dev; + case 'prod': + default: + return Environment.prod; + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/lib/src/config/env.dart` around lines 9 - 10, The code currently pins the compile-time const currentEnvironment to Environment.prod; change this to a runtime-resolved value so developers can switch without editing source: replace the const currentEnvironment with a runtime-initialized final (or getter) that reads Platform.environment['STAC_ENV'] (or similar), map values like "prod"/"production" to Environment.prod and default to Environment.dev (safe default to avoid accidental production targeting), and keep the Environment enum unchanged; update any code that relied on currentEnvironment being const if necessary.
🤖 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_cli/lib/src/utils/file_utils.dart`:
- Around line 10-24: The homeDirectory getter currently returns an empty string
when HOME/USERPROFILE is missing which allows configDirectory to resolve to a
relative path; change homeDirectory (the static getter) to fail fast by throwing
a descriptive StateError (or similar) if Platform.environment['HOME'] /
['USERPROFILE'] is null, so callers like the static configDirectory getter
cannot proceed with a bogus empty path and will surface a clear error instead.
---
Nitpick comments:
In `@packages/stac_cli/lib/src/config/env.dart`:
- Around line 9-10: The code currently pins the compile-time const
currentEnvironment to Environment.prod; change this to a runtime-resolved value
so developers can switch without editing source: replace the const
currentEnvironment with a runtime-initialized final (or getter) that reads
Platform.environment['STAC_ENV'] (or similar), map values like
"prod"/"production" to Environment.prod and default to Environment.dev (safe
default to avoid accidental production targeting), and keep the Environment enum
unchanged; update any code that relied on currentEnvironment being const if
necessary.
In `@packages/stac_cli/lib/src/utils/file_utils.dart`:
- Around line 56-58: The writeFile method assumes the parent directory exists;
update writeFile to create the parent directory before writing by resolving the
parent via File(filePath).parent and calling create(recursive: true) (handle
async) prior to invoking writeAsString; keep using the same writeFile signature
and await semantics so File(filePath).writeAsString(content) runs after the
parent directory creation.
…r proper serialization
…tifacts and _convertCallableToJson methods
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
packages/stac_cli/lib/src/commands/upgrade_command.dart (3)
6-7: Consider constructor injection for testability.Instantiating
UpgradeServicedirectly makes unit testing difficult. Constructor injection allows mocking the service in tests.♻️ Proposed refactor for dependency injection
class UpgradeCommand extends BaseCommand { - final UpgradeService _upgradeService = UpgradeService(); + final UpgradeService _upgradeService; + + UpgradeCommand({UpgradeService? upgradeService}) + : _upgradeService = upgradeService ?? UpgradeService() { + argParser.addOption( + 'version', + help: 'Specific version to install (e.g., 1.2.0)', + ); + argParser.addFlag( + 'force', + abbr: 'f', + negatable: false, + help: 'Force upgrade even if already on latest version', + ); + } - UpgradeCommand() { - argParser.addOption( - 'version', - help: 'Specific version to install (e.g., 1.2.0)', - ); - argParser.addFlag( - 'force', - abbr: 'f', - negatable: false, - help: 'Force upgrade even if already on latest version', - ); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/lib/src/commands/upgrade_command.dart` around lines 6 - 7, The UpgradeCommand currently instantiates UpgradeService directly (final UpgradeService _upgradeService = UpgradeService()), which hinders unit testing; change UpgradeCommand to accept an UpgradeService via its constructor (e.g., a required or optional parameter in UpgradeCommand(...)) and assign it to the _upgradeService field so tests can inject a mock or fake UpgradeService instance; update any call sites that create UpgradeCommand to pass the real UpgradeService and adjust visibility of _upgradeService if necessary.
29-31: Consider validating version format early.The
specificVersionis passed directly to the service without format validation. Invalid input likestac upgrade --version foocould result in unclear error messages from the network call.final specificVersion = argResults?['version'] as String?; if (specificVersion != null && !RegExp(r'^\d+\.\d+\.\d+').hasMatch(specificVersion)) { ConsoleLogger.error('Invalid version format. Expected semver (e.g., 1.2.0)'); return 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/lib/src/commands/upgrade_command.dart` around lines 29 - 31, In execute(), validate the user-supplied specificVersion (argResults?['version']) before passing it to the service: if specificVersion is non-null ensure it matches a semver pattern (e.g., /^\d+\.\d+\.\d+$/) and on mismatch call ConsoleLogger.error with a clear message like "Invalid version format. Expected semver (e.g., 1.2.0)" and return a non-zero exit code (e.g., 1) to avoid sending invalid input to the network call.
48-61: Consider consolidating redundant early-exit conditions.When
specificVersion == null,targetVersionequalslatestVersion. The first condition (line 49) checks string equalitycurrentVersion == targetVersion, while the second (line 56) checks!updateAvailable. These overlap when no specific version is requested.If
updateAvailableis computed via semantic version comparison (which may differ from string equality), both checks provide defense-in-depth. Otherwise, consider consolidating:♻️ Proposed consolidation
// Check if upgrade is needed - if (!force && checkResult.currentVersion == targetVersion) { - ConsoleLogger.success( - '✓ Already on the latest version (${checkResult.currentVersion})', - ); - return 0; - } - - if (!force && !checkResult.updateAvailable && specificVersion == null) { + final alreadyOnTarget = checkResult.currentVersion == targetVersion; + final noUpdateNeeded = !checkResult.updateAvailable && specificVersion == null; + + if (!force && (alreadyOnTarget || noUpdateNeeded)) { ConsoleLogger.success( '✓ Already on the latest version (${checkResult.currentVersion})', ); return 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/lib/src/commands/upgrade_command.dart` around lines 48 - 61, Consolidate the redundant early-exit checks into a single condition: when not forcing and no specificVersion is requested, combine the existing equality and update-available guards into one branch that logs the success and returns 0. Update the logic around checkResult, targetVersion, specificVersion, updateAvailable and force (used by ConsoleLogger.success) so you only have one early return for the “already up-to-date” case (e.g. !force && specificVersion == null && (checkResult.currentVersion == targetVersion || !checkResult.updateAvailable)); alternatively if updateAvailable is the authoritative semantic comparison, use that alone with !force && specificVersion == null to decide the early exit.packages/stac_cli/lib/src/commands/init_command.dart (1)
273-277: Command suggestion may be invalid if name/description contain spaces or special characters.The printed command suggestion doesn't quote the arguments. If
nameis "My Project", the suggestion becomesstac project create -n My Project -d ...which would fail. Consider quoting the values.💡 Suggested improvement
ConsoleLogger.info( - 'You can also run "stac project create -n ${name.trim()} -d ${description.trim()}" and then re-run "stac init".', + 'You can also run "stac project create -n \'${name.trim()}\' -d \'${description.trim()}\'" and then re-run "stac init".', );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac_cli/lib/src/commands/init_command.dart` around lines 273 - 277, The printed suggestion in init_command.dart uses ConsoleLogger.info with unquoted name and description, which will break if name/description contain spaces or special characters; update the suggestion to properly quote and escape the values (e.g., wrap name.trim() and description.trim() in quotes or shell-escape them) when building the command string passed to ConsoleLogger.info (refer to the block around ConsoleLogger.error("Failed to create project: \$e") and the subsequent ConsoleLogger.info call that uses name and description).
🤖 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_cli/lib/src/commands/init_command.dart`:
- Around line 138-144: The inner catch block currently uses catch
(ProcessException) but logs ConsoleLogger.error('Failed to manually add
dependency: $e') which references the outer exception variable; update the inner
catch to capture its exception (e.g., catch (ProcessException inner) or catch
(ProcessException e)) and log that captured variable in ConsoleLogger.error so
the actual dart failure is reported (adjust the log message to include the inner
exception variable). Ensure you update only the inner catch signature and the
ConsoleLogger.error call (symbols: the inner catch clause and
ConsoleLogger.error).
In `@packages/stac_cli/lib/src/services/build_service.dart`:
- Around line 165-169: The hidden-directory filter using
entity.path.contains('/.') fails on Windows because it only checks for forward
slashes; update the check around entity.path (the code that decides whether to
add to dartFiles) to detect hidden directories cross-platform by either using
package:path to split path segments and checking any segment.startsWith('.') or
by using a regex that matches either slash or backslash (e.g., one that looks
for '(^|[\\/])\\.'). Also ensure the '.build' check similarly inspects path
segments rather than searching for substring '.build' so it works on Windows
paths; update the condition that currently references entity.path to use these
platform-agnostic checks.
- Around line 594-609: The _cleanJson function currently uses reference equality
(cleanedValue != [] / {}), which never detects empty collections; update the
checks to test for emptiness by type instead — e.g. after computing
cleanedValue, treat it as invalid if it is null or (cleanedValue is List &&
cleanedValue.isEmpty) or (cleanedValue is Map && cleanedValue.isEmpty), and only
add to cleanedMap when it passes these checks; likewise when building
cleanedList filter out items that are null or empty by using the same type-based
(is List/Map) isEmpty tests. Ensure returns remain returning null for empty
map/list results.
- Around line 520-532: The current use of Process.run with .timeout lets the
Dart subprocess keep running if the timeout fires; replace Process.run with
Process.start to obtain a Process object (use the same args: 'dart' and ['run',
path.basename(scriptFile.path)] with workingDirectory: projectDir and
runInShell: Platform.isWindows), capture stdout/stderr into strings, then await
process.exitCode with a timeout and on timeout call process.kill() (and
optionally process.kill(ProcessSignal.sigkill) if still running) before
throwing; update subsequent uses of result.exitCode, result.stdout,
result.stderr to use the Process object's exitCode, captured stdout, and
captured stderr variables instead.
---
Nitpick comments:
In `@packages/stac_cli/lib/src/commands/init_command.dart`:
- Around line 273-277: The printed suggestion in init_command.dart uses
ConsoleLogger.info with unquoted name and description, which will break if
name/description contain spaces or special characters; update the suggestion to
properly quote and escape the values (e.g., wrap name.trim() and
description.trim() in quotes or shell-escape them) when building the command
string passed to ConsoleLogger.info (refer to the block around
ConsoleLogger.error("Failed to create project: \$e") and the subsequent
ConsoleLogger.info call that uses name and description).
In `@packages/stac_cli/lib/src/commands/upgrade_command.dart`:
- Around line 6-7: The UpgradeCommand currently instantiates UpgradeService
directly (final UpgradeService _upgradeService = UpgradeService()), which
hinders unit testing; change UpgradeCommand to accept an UpgradeService via its
constructor (e.g., a required or optional parameter in UpgradeCommand(...)) and
assign it to the _upgradeService field so tests can inject a mock or fake
UpgradeService instance; update any call sites that create UpgradeCommand to
pass the real UpgradeService and adjust visibility of _upgradeService if
necessary.
- Around line 29-31: In execute(), validate the user-supplied specificVersion
(argResults?['version']) before passing it to the service: if specificVersion is
non-null ensure it matches a semver pattern (e.g., /^\d+\.\d+\.\d+$/) and on
mismatch call ConsoleLogger.error with a clear message like "Invalid version
format. Expected semver (e.g., 1.2.0)" and return a non-zero exit code (e.g., 1)
to avoid sending invalid input to the network call.
- Around line 48-61: Consolidate the redundant early-exit checks into a single
condition: when not forcing and no specificVersion is requested, combine the
existing equality and update-available guards into one branch that logs the
success and returns 0. Update the logic around checkResult, targetVersion,
specificVersion, updateAvailable and force (used by ConsoleLogger.success) so
you only have one early return for the “already up-to-date” case (e.g. !force &&
specificVersion == null && (checkResult.currentVersion == targetVersion ||
!checkResult.updateAvailable)); alternatively if updateAvailable is the
authoritative semantic comparison, use that alone with !force && specificVersion
== null to decide the early exit.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/stac_cli/lib/src/commands/init_command.dartpackages/stac_cli/lib/src/commands/project/list_command.dartpackages/stac_cli/lib/src/commands/upgrade_command.dartpackages/stac_cli/lib/src/services/build_service.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stac_cli/lib/src/commands/project/list_command.dart
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
1 participant
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.
Description
This PR introduces the official Stac CLI and open-sources it as part of the Stac monorepo.
The CLI enables developers to manage Stac SDUI projects end to end from the terminal, including authentication, project setup, building JSON artifacts, and deploying to the cloud.
✨ What’s Included
login,logout,status)stac initfor bootstrapping Stac projectsstac buildto convert Dart widget definitions into JSONstac deployto upload artifacts to Stac Cloud~/.stac/.envThis lays the foundation for Stac Cloud workflows and improves the overall developer experience around Server Driven UI with Flutter.
Type of Change
Summary by CodeRabbit
New Features
Documentation
Other