fix(code-review): apply #277 review fixes - #278
Conversation
Apply the still-applicable CodeRabbit findings from the merged #277 (zfa setup + init wiring) review: - cli_runner: fix inverted alias help text (init is the alias of initialize) - setup/initialize: propagate failed dependency wiring to a non-zero exit code (retain WireResult, list failed entries, stop before entity scaffolding) - setup: require interactive confirmation before --force deletes the target directory (print absolute path + entry count), revise --force/--org help text - setup: warn when --platforms/--org are passed with --dart - dependency_wirer: build the appended dependency_overrides section at column zero instead of replaceFirst (avoid corrupting earlier text) - dependency_wirer: use 'flutter' pub executable for Flutter projects - dependency_wirer: populate WireResult.skipped so didNothing is accurate; guard the pubspec write and record overrides as added only after a successful write; use package:path joins - initialize: throw UsageException instead of exit(1) for mutually-exclusive flags and missing pubspec - tests: use InitializeCommand.buildParser() (drop duplicated parser), add UsageException coverage for --flutter/--dart exclusion, add ensureProjectStructure tests Skipped: defaultGitRef -> stable tag (no released tag contains the zuraffa_flutter sub-package; pinning would break Flutter wiring).
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe CLI now shares ChangesBootstrap reliability
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SetupCommand
participant DependencyWirer
participant DartOrFlutter
participant ProjectFilesystem
User->>SetupCommand: run setup options
SetupCommand->>DependencyWirer: wire dependencies
DependencyWirer->>DartOrFlutter: run pub add or pub get
DependencyWirer->>ProjectFilesystem: write pubspec and create directories
ProjectFilesystem-->>DependencyWirer: return filesystem results
DependencyWirer-->>SetupCommand: return added, skipped, and failed dependencies
SetupCommand-->>User: report success or throw StateError
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/src/core/dependencies/dependency_wirer.dart (1)
277-282: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn skipped dependencies during dry runs.
When some standard dependencies already exist, this return path drops
skippedNames. This makesWireResultinconsistent between dry-run and real runs. Includeskipped: skippedNames.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/core/dependencies/dependency_wirer.dart` around lines 277 - 282, Update the dryRun return path that constructs WireResult to include skipped: skippedNames, preserving the existing added and dryRun values so dry-run results remain consistent with real runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/src/commands/setup_command.dart`:
- Around line 158-166: Move the existing wireResult failure check immediately
after the DependencyWirer.wire call and before step 3 begins. Preserve its
warning output and StateError behavior so setup stops before creating project
structure or .zfa.json when wiring fails.
In `@lib/src/core/dependencies/dependency_wirer.dart`:
- Around line 334-342: Move the `✅ Added override...` success logging in the
dependency override loop to execute only after
`pubspecFile.writeAsString(newContent)` completes successfully. Keep
`added.addAll` and the existing failure handling aligned with the write result
so a failed write reports only failure.
- Around line 351-359: Update the dependency-wiring flow handling nonzero
`getResult.exitCode` so it records a resolution failure in `WireResult` before
returning, rather than only printing diagnostics. Ensure `WireResult.isSuccess`
becomes false and that `InitializeCommand` and `SetupCommand` receive the failed
result and cannot continue as successful after an unresolved override.
---
Outside diff comments:
In `@lib/src/core/dependencies/dependency_wirer.dart`:
- Around line 277-282: Update the dryRun return path that constructs WireResult
to include skipped: skippedNames, preserving the existing added and dryRun
values so dry-run results remain consistent with real runs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a3551d2f-bcc8-4cbe-85bc-8e934ae42929
📒 Files selected for processing (6)
lib/src/cli/cli_runner.dartlib/src/commands/initialize_command.dartlib/src/commands/setup_command.dartlib/src/core/dependencies/dependency_wirer.darttest/commands/setup_command_test.darttest/core/dependencies/dependency_wirer_test.dart
| if (wireResult != null && !wireResult.isSuccess) { | ||
| print( | ||
| '\n⚠️ Some dependencies could not be wired automatically: ' | ||
| '${wireResult.failed.join(', ')}', | ||
| ); | ||
| print(' Add them manually to pubspec.yaml and re-run `zfa init`.'); | ||
| // Non-zero exit so CI can distinguish a partial bootstrap. | ||
| throw StateError('Some dependencies could not be wired automatically.'); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stop setup immediately after dependency wiring fails.
Steps 3 and 4 run before this check. A failed WireResult therefore still creates project structure and .zfa.json, then prints Setup complete! before throwing. Move this failure check directly after DependencyWirer.wire and before step 3.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/src/commands/setup_command.dart` around lines 158 - 166, Move the
existing wireResult failure check immediately after the DependencyWirer.wire
call and before step 3 begins. Preserve its warning output and StateError
behavior so setup stops before creating project structure or .zfa.json when
wiring fails.
| try { | ||
| await pubspecFile.writeAsString(newContent); | ||
| // Record overrides as added only after the write succeeds. | ||
| added.addAll(overrideSpecs.map((s) => s.name)); | ||
| } catch (e) { | ||
| print( | ||
| ' ⚠️ Failed to write dependency_overrides to pubspec.yaml: $e', | ||
| ); | ||
| failed.addAll(overrideSpecs.map((s) => s.name)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Log override success after the write succeeds.
The loop logs ✅ Added override... before writeAsString completes. If the write fails, the output reports the same override as both added and failed. Move the success log after a successful write.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/src/core/dependencies/dependency_wirer.dart` around lines 334 - 342, Move
the `✅ Added override...` success logging in the dependency override loop to
execute only after `pubspecFile.writeAsString(newContent)` completes
successfully. Keep `added.addAll` and the existing failure handling aligned with
the write result so a failed write reports only failure.
| if (getResult.exitCode != 0) { | ||
| final err = getResult.stderr.toString().trim(); | ||
| if (err.isNotEmpty) { | ||
| print(' ⚠️ dart pub get reported issues after override edit:'); | ||
| print( | ||
| ' ⚠️ $pubExecutable pub get reported issues after override edit:', | ||
| ); | ||
| print(' ${err.split('\n').take(3).join('\n ')}'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make a failed pub get fail the wiring result.
When pubExecutable pub get returns nonzero, this code only prints diagnostics. WireResult.isSuccess remains true, so InitializeCommand and SetupCommand can continue and exit successfully after an unresolved override. Record a resolution failure in WireResult before returning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/src/core/dependencies/dependency_wirer.dart` around lines 351 - 359,
Update the dependency-wiring flow handling nonzero `getResult.exitCode` so it
records a resolution failure in `WireResult` before returning, rather than only
printing diagnostics. Ensure `WireResult.isSuccess` becomes false and that
`InitializeCommand` and `SetupCommand` receive the failed result and cannot
continue as successful after an unresolved override.
|
@coderabbitai autofix |
|
An unexpected error occurred while generating fixes: 14 UNAVAILABLE: Connection dropped |
|
@coderabbitai review |
|
1 participant
Follow-up to the merged #277 (zfa
setupcommand +initdependency wiring): applies the still-applicable CodeRabbit review fixes that were not included at merge time. Each finding was re-verified against the code as merged.Inline findings (6)
initdescribed as canonical,initializeas aliaslib/src/cli/cli_runner.dart—initnow reads "Alias of initialize",initializereads "Wire zuraffa dependencies + scaffold a test entity"setup_command.dartretains theWireResult, listsfailedentries in the step 5 summary and throws (exit 1) whenisSuccessis false;initialize_command.dartthrows before entity scaffolding when wiring is unsuccessful--forcedeleted the target directory without confirmationsetup_command.dart—_createAppnow prints the resolved absolute path + recursive entry count, requires interactive "yes" confirmation when stdin is a terminal (aborts if declined), preserves dry-run, and the--forcehelp text now states the directory is deletedreplaceFirst(' dependency_overrides', …)could corrupt an earlier match in the pubspecdependency_wirer.dart— the append branch now builds thedependency_overridessection with the header at column zero from the outsetdart pub add/pub getused for Flutter projects (cannot resolvesdk: flutter)dependency_wirer.dart— executable selected byisFlutter:flutterfor Flutter projects,dartotherwise--flutter+--dart) untested (the rename part was already addressed in #277)setup_command_test.dart— addedrun() rejects both --flutter and --dart with UsageExceptionNitpick findings (8)
WireResult.skippednever populated →didNothingalways falsedependency_wirer.dart—wire()now reports already-present dependency names asskippeddefaultGitReftracks the movingdevelopmentbranchzuraffa_fluttersub-package (split landed 2026-08-04, after the latest tagv5.7.1of 2026-07-30), so pinning the default would break Flutter project wiring. Ref remains overridable via--git-reffor git dependencies.addedbefore the writedependency_wirer.dart—writeAsStringguarded like the pub-add loop; override names added toaddedonly after a successful writedependency_wirer.dart—path.joinfrompackage:pathforpubspec.yaml,build.yaml, and standard dirsexit(1)insideexecutebypasses the CLI error pathinitialize_command.dart— both--deps-only+--no-depsand missing-pubspec cases throwUsageException(exit 64, usage printed)--platforms/--orgsilently ignored with--dartsetup_command.dart— warning printed when either is passed with--dart;--orghelp extended with "(ignored with --dart)"ArgParsercopyInitializeCommand.buildParser()exposed; tests call it; duplicated_buildInitializeParserremovedensureProjectStructureuntesteddependency_wirer_test.dart— temp-dir tests: createsbuild.yaml+ every standard dir, does not overwrite existing files,dryRun: truecreates nothingVerification
dart analyzeon all changed files: no issues (the repo-wide run still reports ~1956 pre-existing issues inzuraffa_flutter/,examples/, etc. — untouched)dart test test/commands/setup_command_test.dart test/core/dependencies/dependency_wirer_test.dart: 68/68 passeddart test: 1143/1143 passed (one intermediate run showed 2 order-dependent failures —route_golden_test+polymorphic_mock_integration_test— which pass in isolation and in every other run; unrelated to these changes)Summary by CodeRabbit
New Features
Bug Fixes
Tests