Conversation
✅ Deploy Preview for dart-spry canceled.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (4)
📝 WalkthroughWalkthroughThis PR upgrades the routing system to Roux 0.5.x, replacing single-segment wildcard paths ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 3-4: The changelog entries fail to mark the path-syntax migration
as breaking: update the release notes to add a BREAKING change and a short
migration note explaining that the public string-path API changed from "/*" to
"/**" (affecting consumer code that constructs Spry, MiddlewareRoute, and
ErrorRoute instances manually); explicitly call out the symbols Spry,
MiddlewareRoute, ErrorRoute and instruct consumers to replace occurrences of
"/*" with "/**" in their route strings and to test route matching after
upgrading to roux 0.5.x.
In `@lib/src/builder/generator.dart`:
- Around line 81-82: The closure returned around the route parameter lookup
currently prefers event.params.wildcard before the named param, which causes
named captures (e.g., [...slug]) to be shadowed by the generic 'wildcard' key;
change the lookup in the returned function so it reads the named value first via
event.params.get(name) and only falls back to event.params.wildcard
(RouteParams.wildcard) if that returns null/empty, updating the logic in the
closure that computes final wildcard value.
In `@lib/src/builder/scanner.dart`:
- Around line 300-304: The code determines isTerminal and routeShape using
rawSegments including a trailing "index" file, so catch-all dirs like
[...slug]/index.dart are misclassified; before calling _parseSegment compute an
effectiveSegments list that strips a single trailing index file (match "index"
or filename with basename "index" like "index.dart") and then use isTerminal: i
== effectiveSegments.length - 1 and routeShape: effectiveSegments.join('/') when
calling _parseSegment (update the same logic at the other occurrence around
lines 311-313). Ensure you only strip one trailing index and leave other
segments unchanged.
- Around line 292-307: The scanner currently accumulates parsed.paramNames into
the route-local paramNames map but only checks for name collisions across
routes; update the loop that processes rawSegments so that after calling
_parseSegment and before adding parsed.paramNames into paramNames you detect
duplicates within the same normalized path: check for repeated names inside
parsed.paramNames and for any overlap between parsed.paramNames and the existing
paramNames for this route, and if found throw a scan/validation error (with a
clear message referencing the duplicate param) so routes like `[id]/[id].dart`
or `[name].[name].dart` are rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34603120-94d0-433c-8f04-42afbd92b513
📒 Files selected for processing (16)
CHANGELOG.mdlib/src/builder/generator.dartlib/src/builder/scanner.dartpubspec.yamlsites/spry.medz.dev/guide/routing.mdtest/app_test.darttest/fixtures/scanner/expressive/routes/archive/[[...rest]].darttest/fixtures/scanner/expressive/routes/assets/[...path+].darttest/fixtures/scanner/expressive/routes/docs/[[section]].darttest/fixtures/scanner/expressive/routes/files/[name].[ext].darttest/fixtures/scanner/expressive/routes/posts/post-[id].json.get.darttest/fixtures/scanner/expressive/routes/users/[_].darttest/fixtures/scanner/expressive/routes/users/[id([0-9]+)].darttest/generator_test.darttest/routing_test.darttest/scanner_test.dart
Summary by CodeRabbit
New Features
Documentation
Breaking / Migration