feat(nitrogen_cli): migrate to unrouter 0.13#1
Conversation
📝 WalkthroughWalkthroughThe routing infrastructure in the nitrogen CLI has been migrated from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request primarily refactors the application's routing mechanism by migrating from nocterm_unrouter to the unrouter package. This involved updating nitrogen.dart to use the new unrouter API, defining routes with Inlets, managing navigation via a global router instance, and removing the custom NitroRoute sealed class. The changes also include a major dependency update in pubspec.yaml and pubspec.lock to support the new routing library and other related packages. A new test file, router_migration_test.dart, was added to verify the core routing functionality after the migration. Additionally, minor stylistic code reformatting was applied across several files, and an unused variable was removed in link_command.dart.
![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 (2)
packages/nitrogen_cli/bin/nitrogen.dart (1)
640-697:⚠️ Potential issue | 🟠 MajorLeaving
ProcessViewdoes not stop the spawned process.
Process.start()is held only in a local variable, but ESC and‹ Backimmediately route home. If the user exits while_runningis true, the child process keeps running off-screen and another command can be launched on top of it. Either block navigation until completion or keep aProcesshandle and terminate/await it on exit.🛠️ Minimal containment fix
onKeyEvent: (event) { - if (event.logicalKey == LogicalKey.escape || - event.logicalKey == LogicalKey.arrowLeft) { - router.replace('/'); + if (!_running && + (event.logicalKey == LogicalKey.escape || + event.logicalKey == LogicalKey.arrowLeft)) { + unawaited(router.replace('/')); return true; } return false; }, @@ HoverButton( label: '‹ Back', - onTap: () => router.replace('/'), + onTap: () { + if (!_running) { + unawaited(router.replace('/')); + } + }, color: Colors.cyan, ),Also applies to: 705-708, 790-793
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nitrogen_cli/bin/nitrogen.dart` around lines 640 - 697, The spawned Process created in _start() is stored only in a local variable so leaving ProcessView doesn't stop the child; store the Process in a state field (e.g. add a _process: Process? on the State) and assign the result of Process.start(...) to it, then implement cleanup in dispose() (or on navigation) to terminate/kill the process and await its exitCode (use _process!.kill(...) and await _process!.exitCode or call _process!.kill(ProcessSignal.sigterm) then fallback to kill if needed), also cancel _pulseTimer and any stream subscriptions; ensure _start() checks for an existing running _process before starting another.packages/nitrogen_cli/pubspec.yaml (1)
17-24:⚠️ Potential issue | 🔴 CriticalRaise the declared Dart SDK floor to match these dependency bumps.
nocterm 0.6.0requires Dart 3.5 andunrouter 0.13.0requires Dart 3.10, sosdk: ^3.4.0advertises toolchains that cannot resolve this pubspec. Tighten the package SDK constraint to at least^3.10.0before landing this.🛠️ Suggested fix
environment: - sdk: ^3.4.0 + sdk: ^3.10.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nitrogen_cli/pubspec.yaml` around lines 17 - 24, The pubspec's SDK lower bound (sdk: ^3.4.0) is too low for the bumped dependencies; update the environment sdk constraint in pubspec.yaml to at least ^3.10.0 so it satisfies nocterm 0.6.0 (requires 3.5) and unrouter 0.13.0 (requires 3.10); edit the "environment: sdk" entry to "^3.10.0" to tighten the package SDK constraint before landing.
🤖 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/nitrogen_cli/bin/nitrogen.dart`:
- Around line 148-163: The route currently calls _getProjectInfo() then always
returns a ProcessView that will run build_runner even when info is null; change
the Inlet view builder to check the result of _getProjectInfo() and, if it
returns null, return an explicit “not in a Nitro project” error view (e.g., a
NotInNitroProjectView or simple ErrorView with a clear message) instead of
returning ProcessView; only construct the ProcessView (with workingDirectory:
info.directory.path and args for flutter pub run build_runner) when info is
non-null so build_runner is never started outside a Nitro project.
In `@packages/nitrogen_cli/lib/commands/link_command.dart`:
- Line 210: The _failed flag is incorrectly declared final and never updated;
change it to a mutable bool or compute it from actual step states (e.g., derive
from LinkStepState.failed) so reporting works: update the declaration of _failed
in link_command.dart to be writable (or replace with a computed value like
steps.any((s) => s.failed)), ensure places that set component.result.success =
!_failed and call shutdownApp(_failed ? 1 : 0) use that real value, and if the
code updates individual LinkStepState instances, set _failed = true when any
LinkStepState.failed becomes true (or recompute before UI branch checks such as
the if (!_failed) that controls the failure UI).
In `@packages/nitrogen_cli/test/router_migration_test.dart`:
- Around line 8-25: The test currently conditionally resets the singleton
app.router and doesn't verify that replace('/') actually returns to the
dashboard; modify the test so the final app.router.replace('/') runs
unconditionally (e.g., in a finally block or after a try/catch) and add an
assertion after that replace to confirm the dashboard is rendered (check
tester.terminalState contains 'Nitrogen CLI'); locate calls to
app.router.replace('/') and app.router.push('/init') and ensure RouterView is
pumped again before the final assertion.
---
Outside diff comments:
In `@packages/nitrogen_cli/bin/nitrogen.dart`:
- Around line 640-697: The spawned Process created in _start() is stored only in
a local variable so leaving ProcessView doesn't stop the child; store the
Process in a state field (e.g. add a _process: Process? on the State) and assign
the result of Process.start(...) to it, then implement cleanup in dispose() (or
on navigation) to terminate/kill the process and await its exitCode (use
_process!.kill(...) and await _process!.exitCode or call
_process!.kill(ProcessSignal.sigterm) then fallback to kill if needed), also
cancel _pulseTimer and any stream subscriptions; ensure _start() checks for an
existing running _process before starting another.
In `@packages/nitrogen_cli/pubspec.yaml`:
- Around line 17-24: The pubspec's SDK lower bound (sdk: ^3.4.0) is too low for
the bumped dependencies; update the environment sdk constraint in pubspec.yaml
to at least ^3.10.0 so it satisfies nocterm 0.6.0 (requires 3.5) and unrouter
0.13.0 (requires 3.10); edit the "environment: sdk" entry to "^3.10.0" to
tighten the package SDK constraint before landing.
🪄 Autofix (Beta)
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
Run ID: d26c30bf-baf5-48d3-811f-3999e6c81701
⛔ Files ignored due to path filters (1)
packages/nitrogen_cli/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
packages/nitrogen_cli/bin/nitrogen.dartpackages/nitrogen_cli/lib/commands/link_command.dartpackages/nitrogen_cli/pubspec.yamlpackages/nitrogen_cli/test/link_command_test.dartpackages/nitrogen_cli/test/router_migration_test.dart
💤 Files with no reviewable changes (1)
- packages/nitrogen_cli/test/link_command_test.dart
ROOT CAUSE #1 — Record params: void* passed as jobject (CRASH) C bridge passed Dart's `void*` (Pointer<Uint8>) directly to JNI CallStaticObjectMethod as the jbyteArray argument. JVM interpreted the raw heap address as a Java object reference → invalid object → SIGSEGV/abort. This crashed the process mid-test, causing ALL tests from §6 (@HybridRecord) onwards to fail. Fix in jni_method_emitter.dart: // Read Dart's [4B len][payload] format: int32_t value_len = *((const int32_t*)value); jbyteArray j_value = env->NewByteArray(value_len); env->SetByteArrayRegion(j_value, 0, value_len, (const jbyte*)((const uint8_t*)value + 4)); // skip 4-byte prefix CallStaticObjectMethod(..., j_value); ROOT CAUSE #2 — JVM descriptor mismatches (silent wrong values) GetStaticMethodID returns null when JVM type doesn't match. C bridge returns default (0/false/null) — no exception, no crash. Fixes in kotlin_generator.dart: a) Enum params in _call: TcStatus → Long (J, not LTcStatus;) callParamsResolved decodes: Long → TcStatus.fromNative(value) b) Nullable enum params in _call: TcStatus? → Long (J not LTcStatus;) sentinel: val xArg = if (x < 0L) null else EnumType.fromNative(x) c) Nullable bool? in _call: Boolean → Int (I not Z) sentinel: val xArg = if (x < 0) null else (x != 0) (Int supports -1; jboolean only 0/1 → null now detectable on Android!) d) Nullable property getters: non-nullable primitives match C (J/D/Z) double? → Double + (?: Double.NaN); int? → Long + (?: -1L) e) Nullable property setters: same non-nullable primitive types decodes: NaN→null, -1L→null 2928 generator tests passing. dart analyze — no issues. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Summary by CodeRabbit