feat: remove * and fix crash; refactor useParseRouter hook and add tests#2780
Conversation
- Refactor useParseRouter to improve code organization - Add comprehensive unit tests for useParseRouter hook - Test navigation state parsing, route matching, and edge cases Co-Authored-By: Claude Opus 4.5 <[email protected]>
Summary of ChangesHello @wwwillchen-bot, 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 significantly enhances the modularity and testability of the Highlights
Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
wwwillchen-bot
temporarily deployed
to
ai-bots
GitHub Actions
Inactive
wwwillchen-bot
temporarily deployed
to
ai-bots
GitHub Actions
Inactive
|
@BugBot run |
Greptile SummaryRefactored Key changes:
Testing:
Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 43be00a |
![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
The pull request successfully refactors the useParseRouter hook by extracting the route parsing logic into three well-defined, exported functions: buildRouteLabel, parseRoutesFromRouterFile, and parseRoutesFromNextFiles. This significantly improves the code's organization, readability, and testability. The addition of comprehensive unit tests for these new functions ensures their correctness and covers various scenarios, including edge cases for both React Router and Next.js file-based routing. This is a positive change for the maintainability of the codebase. The specific comments address minor improvements in regex robustness and code conciseness.
|
|
||
| try { | ||
| const parsedRoutes: ParsedRoute[] = []; | ||
| const routePathsRegex = /<Route\s+(?:[^>]*\s+)?path=["']([^"']+)["']/g; |
There was a problem hiding this comment.
The regular expression used to extract path attributes in parseRoutesFromRouterFile might be brittle. If a <Route> element contains other attributes that include the string "path=" or if the element prop itself contains a > character before the path attribute, the current regex might misinterpret the path. A more precise regex that explicitly looks for path as an attribute within the tag could improve robustness and prevent potential parsing errors.
| const routePathsRegex = /<Route\s+(?:[^>]*\s+)?path=["']([^"']+)["']/g; | |
| const routePathsRegex = /<Route\s+[^>]*?path=["']([^"']+)["'][^>]*?>/g; |
| lower === "app/page.tsx" || | ||
| lower === "app/page.jsx" || | ||
| lower === "app/page.js" || | ||
| lower === "app/page.mdx" || | ||
| lower === "app/page.ts" || | ||
| lower === "src/app/page.tsx" || | ||
| lower === "src/app/page.jsx" || | ||
| lower === "src/app/page.js" || | ||
| lower === "src/app/page.mdx" || | ||
| lower === "src/app/page.ts" | ||
| ) { |
There was a problem hiding this comment.
The condition for checking app/page.tsx and its variants is quite verbose. This can be simplified using a regular expression for better readability and maintainability.
if (
/^(?:src\/)?app\/page\.(?:tsx|jsx|js|mdx|ts)$/i.test(lower)
) {![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43be00a6ec
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const path = match[1]; | ||
| // Skip wildcard/catch-all routes like "*" - they are not valid navigation targets | ||
| // and cause 'Invalid URL' TypeError when clicked | ||
| if (path === "*" || path === "/*") continue; |
There was a problem hiding this comment.
The new filter in parseRoutesFromRouterFile drops both "*" and "/*", but only bare "*" is the invalid case for preview navigation (navigateToRoute builds origin + path, so "/*" becomes a valid URL like http://host/*). Excluding "/*" removes the only top-level route in common React Router setups such as <Route path="/*" .../>, leaving the quick-route dropdown empty (it falls back to the “Loading routes...” state) even though parsing succeeded.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Confidence score: 3/5
- Potential runtime throw in
buildRouteLabelwhen paths are only dynamic segments (e.g.,/:id) could break label generation and impact routing UI. - Given a concrete, user-impacting edge case with medium severity, this carries some merge risk despite being localized.
- Pay close attention to
src/hooks/useParseRouter.ts- guard.replaceafter.pop()to avoid undefined crashes.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/hooks/useParseRouter.ts">
<violation number="1" location="src/hooks/useParseRouter.ts:21">
P2: `buildRouteLabel` can throw when a route path contains only dynamic segments (e.g., `/:id`) because the second `.replace` runs on `undefined` after `.pop()` returns nothing. Guard the second replace or fall back earlier to avoid runtime errors when parsing routes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| .filter((segment) => segment && !segment.startsWith(":")) | ||
| .pop() | ||
| ?.replace(/[-_]/g, " ") | ||
| .replace(/^\w/, (c) => c.toUpperCase()) || path; |
There was a problem hiding this comment.
P2: buildRouteLabel can throw when a route path contains only dynamic segments (e.g., /:id) because the second .replace runs on undefined after .pop() returns nothing. Guard the second replace or fall back earlier to avoid runtime errors when parsing routes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/useParseRouter.ts, line 21:
<comment>`buildRouteLabel` can throw when a route path contains only dynamic segments (e.g., `/:id`) because the second `.replace` runs on `undefined` after `.pop()` returns nothing. Guard the second replace or fall back earlier to avoid runtime errors when parsing routes.</comment>
<file context>
@@ -7,6 +7,127 @@ export interface ParsedRoute {
+ .filter((segment) => segment && !segment.startsWith(":"))
+ .pop()
+ ?.replace(/[-_]/g, " ")
+ .replace(/^\w/, (c) => c.toUpperCase()) || path;
+}
+
</file context>
| .replace(/^\w/, (c) => c.toUpperCase()) || path; | |
| ?.replace(/^\w/, (c) => c.toUpperCase()) || path; |
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. This is a clean refactoring PR that extracts internal hook functions into testable utilities with good test coverage. The only behavioral change (filtering wildcard routes) is an improvement that fixes a TypeError. Issues SummaryNo HIGH or MEDIUM issues found. 🟢 Low Priority Notes (4 items)
🚫 Dropped False Positives (5 items)
Generated by Dyadbot multi-agent code review |
github-actions
bot
added
the
needs-human:review-issue
wwwillchen
changed the title
🎭 Playwright Test Results✅ All tests passed!
Total: 724 tests passed (6 flaky) (230 skipped)
|
2 participants
Summary
useParseRouterhook to improve code organization and maintainabilityuseParseRouterhook covering navigation state parsing, route matching, and edge casesTest plan
npm testto verify all 842 tests passsrc/__tests__/useParseRouter.test.tscover various navigation scenarios🤖 Generated with Claude Code