fix(routing): replace the drifted route tables with a single registry - #583
Conversation
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR centralizes SPA route definitions and access control in ChangesRouting and build validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant App
participant routeRegistry
participant AppRoutes
participant PageComponent
Browser->>App: pathname
App->>routeRegistry: resolvePath(currentRoutePath())
routeRegistry-->>App: page and data
App->>AppRoutes: currentPage and pageData
AppRoutes->>routeRegistry: checkAccess(user, currentPage)
routeRegistry-->>AppRoutes: access result
AppRoutes->>PageComponent: render selected component
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
github-actions
Bot
added
documentation
![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
🧹 Nitpick comments (4)
src/routes/routeRegistry.js (2)
277-285: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
checkAccessfails open for unknown pages. An unregistered page key returnsallowed: true.AppRoutercurrently guards withgetRoutefirst, so this isn't exploitable today, but any other caller (e.g. a nav-link visibility check) would silently grant access. Prefer default-deny here.🛡️ Proposed fix
export function checkAccess(user, page) { const route = getRoute(page); - if (!route || route.access === PUBLIC) { + if (!route) { + return { allowed: false, reason: "unknown-route" }; + } + if (route.access === PUBLIC) { return { allowed: true, reason: null }; }🤖 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 `@src/routes/routeRegistry.js` around lines 277 - 285, Update checkAccess so an unknown page from getRoute(page) returns allowed: false rather than entering the public-route success path. Preserve allowed access for registered routes whose access is PUBLIC, and leave the existing unauthenticated handling unchanged.
192-198: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winComment promises an ordering guarantee the code doesn't provide.
filterpreserves registry declaration order; nothing sorts longest-prefix-first. It happens to be safe today (no parameterised slug is a prefix of another), but the invariant is unenforced. Either sort explicitly or drop the claim.♻️ Proposed fix
-const PARAMETERISED_ROUTES = ROUTES.filter((route) => route.param); +const PARAMETERISED_ROUTES = ROUTES.filter((route) => route.param).sort( + (a, b) => Math.max(...b.slugs.map((s) => s.length)) - Math.max(...a.slugs.map((s) => s.length)) +);🤖 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 `@src/routes/routeRegistry.js` around lines 192 - 198, Update PARAMETERISED_ROUTES so its construction enforces the documented longest-prefix-first ordering by sorting parameterised routes by descending path-prefix length; otherwise remove the ordering claim from the comment. Preserve the existing route filtering and matching behavior.src/App.jsx (1)
21-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a plain prefix strip over a constructed
RegExp.BASE_PATHis a trusted module constant so the static-analysis ReDoS hint is a false positive, but interpolating an unescaped string intoRegExpis a footgun the moment the base path gains a regex metacharacter (.,+). A case-insensitivestartsWithis equivalent and cheaper.♻️ Proposed fix
-const currentRoutePath = () => - window.location.pathname - .replace(new RegExp(`^${BASE_PATH}`, "i"), "") - .replace(/^\/+|\/+$/g, ""); +const currentRoutePath = () => { + const { pathname } = window.location; + const bare = pathname.toLowerCase().startsWith(BASE_PATH.toLowerCase()) + ? pathname.slice(BASE_PATH.length) + : pathname; + return bare.replace(/^\/+|\/+$/g, ""); +};🤖 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 `@src/App.jsx` around lines 21 - 24, Update currentRoutePath to remove BASE_PATH using a case-insensitive startsWith check and string slicing instead of constructing an interpolated RegExp, while preserving the existing slash trimming behavior.Source: Linters/SAST tools
src/routes/AppRoutes.jsx (1)
45-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
reasondoubles as both a sentinel and a user-facing string.checkAccessreturns the magic value"unauthenticated"for one branch and a rendered message for another, so a wording change in the registry silently breaks this comparison. Consider a discriminated result, e.g.{ allowed, code, message }, and branch oncode.🤖 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 `@src/routes/AppRoutes.jsx` around lines 45 - 55, Update checkAccess and its caller in AppRoutes so access results use a stable discriminant such as code for authentication status, while carrying the user-facing denial text separately as message. Change the unauthenticated branch to compare the discriminant and pass the message to UnauthorizedPage, preserving the existing LoginPage and denial behavior.
🤖 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 `@scripts/check-routes.js`:
- Around line 137-153: Update the duplicate-slug validation around seenSlugs so
parameterized routes are checked in their own map instead of being skipped.
Maintain separate maps for static and parameterized slugs, allowing the same
slug across maps while calling fail when duplicated within either map; preserve
the existing conflict message and route-page reporting.
In `@src/routes/AppRoutes.jsx`:
- Around line 47-49: Update src/routes/AppRoutes.jsx lines 47-49 to redirect
unauthenticated users via onNavigate("login") from an effect instead of
rendering LoginPage inline, keeping page state, URL, and layout chrome
synchronized. Also update src/App.jsx lines 62-64 to evaluate hasChrome using
the router’s effective rendered page if needed, so substituted LoginPage or
NotFoundPage layouts do not inherit chrome from the requested currentPage.
In `@src/routes/routeRegistry.js`:
- Around line 13-15: Update the comments in routeRegistry.js, including the
entry near the route registry and the one around line 200, to reference
scripts/check-routes.js as the build-time validator instead of the non-existent
routeRegistry.test.js; preserve the existing explanation of the validation
checks.
---
Nitpick comments:
In `@src/App.jsx`:
- Around line 21-24: Update currentRoutePath to remove BASE_PATH using a
case-insensitive startsWith check and string slicing instead of constructing an
interpolated RegExp, while preserving the existing slash trimming behavior.
In `@src/routes/AppRoutes.jsx`:
- Around line 45-55: Update checkAccess and its caller in AppRoutes so access
results use a stable discriminant such as code for authentication status, while
carrying the user-facing denial text separately as message. Change the
unauthenticated branch to compare the discriminant and pass the message to
UnauthorizedPage, preserving the existing LoginPage and denial behavior.
In `@src/routes/routeRegistry.js`:
- Around line 277-285: Update checkAccess so an unknown page from getRoute(page)
returns allowed: false rather than entering the public-route success path.
Preserve allowed access for registered routes whose access is PUBLIC, and leave
the existing unauthenticated handling unchanged.
- Around line 192-198: Update PARAMETERISED_ROUTES so its construction enforces
the documented longest-prefix-first ordering by sorting parameterised routes by
descending path-prefix length; otherwise remove the ordering claim from the
comment. Preserve the existing route filtering and matching behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bfca85ae-7d83-43ae-a8b5-b570cd72a535
📒 Files selected for processing (7)
docs/frontend-build.mdpackage.jsonscripts/check-routes.jssrc/App.jsxsrc/components/auth/EnterpriseSecurityCenter.jsxsrc/routes/AppRoutes.jsxsrc/routes/routeRegistry.js
| if (!allowed && reason === "unauthenticated") { | ||
| return <LoginPage onNavigate={onNavigate} />; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Layout chrome can disagree with the page actually rendered. AppRouter may substitute LoginPage or NotFoundPage for the requested route, but App.jsx decides navbar/footer visibility from currentPage alone — so a signed-out hit on /equipment renders the full-bleed login layout wrapped in Navbar + Footer.
src/routes/AppRoutes.jsx#L47-L49: instead of renderingLoginPagein place, redirect throughonNavigate("login")(in an effect) so page state, URL, and chrome all agree.src/App.jsx#L62-L64: alternatively (or additionally), have the router surface the effective page sohasChromeis evaluated against what is rendered rather than what was requested.
📍 Affects 2 files
src/routes/AppRoutes.jsx#L47-L49(this comment)src/App.jsx#L62-L64
🤖 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 `@src/routes/AppRoutes.jsx` around lines 47 - 49, Update
src/routes/AppRoutes.jsx lines 47-49 to redirect unauthenticated users via
onNavigate("login") from an effect instead of rendering LoginPage inline,
keeping page state, URL, and layout chrome synchronized. Also update src/App.jsx
lines 62-64 to evaluate hasChrome using the router’s effective rendered page if
needed, so substituted LoginPage or NotFoundPage layouts do not inherit chrome
from the requested currentPage.
Route information lived in three hand-maintained places - the routeMap object in App.jsx, the switch in AppRoutes.jsx, and the import list above it - with nothing keeping them consistent. The accumulated drift: - KeyVaultSecurityPage and MicrosegmentationPage were rendered but never imported, an uncaught ReferenceError that ErrorBoundary turns into a blank screen on /keyvault and /microsegmentation - case 'keyvault-security' and case 'microsegmentation' each appeared twice, so the second was dead and /microsegmentation silently rendered the ZTNA page - routeMap declared help twice, and microsegmentation twice with different targets; in an object literal the last silently wins - fourteen security consoles existed as page components with no route - UnauthorizedPage accepted a message prop that nothing ever passed, so every denial showed the same generic sentence One registry entry per page removes the drift by construction. Access level, layout chrome and the dynamic-segment prop name are declared alongside the component, so App.jsx no longer keeps its own noLayoutPages list and AppRoutes no longer hard-codes which prop each page expects. Unknown paths now resolve to the 404 page. The previous routeMap[path] || 'landing' fallback rendered the landing page for any mistyped URL, which hid exactly this class of bug. package.json gains the eslintConfig field it never had. Without it react-scripts runs its ESLint plugin against eslint-config-react-app/base only and never loads the ruleset where no-undef, no-dupe-keys and no-duplicate-case are errors - which is why none of the above was ever reported. Enabling it caught two further defects: Network and Award used as icons in EnterpriseSecurityCenter.jsx without being imported. scripts/check-routes.js runs from prebuild and fails on an unimported component, a duplicate page key or slug, or an unregistered console. Verified by re-injecting all three original defects. Refs kRamu81#575
Three findings, all valid: - scripts/check-routes.js skipped parameterised routes entirely when checking for duplicate slugs, so two dynamic routes could claim the same prefix and resolvePath would always pick whichever came first in ROUTES, leaving the other permanently unreachable. Static and parameterised slugs now get their own map, so a slug may appear once as each but not twice within either kind. Verified by injecting a second route on the 'apply' prefix. - Layout chrome was decided from the requested page while AppRoutes may render a different one. A signed-out visit to /equipment rendered the full-bleed login screen wrapped in the navbar and footer. Added resolveEffectivePage to the registry and both AppRoutes and App.jsx now derive the substitution from it, so the two cannot disagree about what is on screen. - Two comments in routeRegistry.js referenced routeRegistry.test.js, which does not exist; the validator is scripts/check-routes.js. Refs kRamu81#575
7e77694 to
66d48a8
Compare
|
Thanks @coderabbitai — all three findings were valid and are fixed in the latest push. 1. Fixed with separate maps, as suggested — a slug may appear once as static and once as parameterised ( 2. Layout chrome disagreeing with the page actually rendered. Good catch, and it is a real visual defect — a signed-out visit to I took the second option you offered rather than the redirect, because redirecting from an effect would change the URL on a bookmarked deep link and lose the destination the user was trying to reach. Instead the registry now exports 3. Stale
|
|
Tip For best results, initiate chat on the files or code changes.
🐇✨ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
|
@MOHITKOURAV01 bro you can join discord community only 5 prs per day |
|
@kRamu81 Yes bro, yesterday I raised 5 PRs, so I am raising 5 today as well. |
|
@MOHITKOURAV01 today you got all spam because i was merged your prs yesterday 3 only today 7 so all prs going to spam make everyday i am checking prs at 9 to 11.30 pm |
2 participants
Description
Fixes #575.
Route information lived in three hand-maintained places that had to be edited together:
routeMapobject insrc/App.jsx— URL slug → page keyswitchinsrc/routes/AppRoutes.jsx— page key → component + role guardNothing kept them consistent, and they had drifted badly.
What was broken
Two components rendered without ever being imported.
AppRoutes.jsximportedSecurityKeyVaultPagebut renderedKeyVaultSecurityPage;MicrosegmentationPagewas notimported under any name. Both are uncaught
ReferenceErrors that theErrorBoundaryturns into ablank screen on
/keyvaultand/microsegmentation.Four route keys declared twice, so the second was dead.
case "microsegmentation"appeared once under the ZTNA group and again under the SDP group. Thefirst wins, so
MicrosegmentationPagewas unreachable and/microsegmentationsilently renderedZeroTrustNetworkPage.In
App.jsx'srouteMap,helpwas declared twice andmicrosegmentationtwice with differenttargets (
"ztna"and"microsegmentation"). In an object literal the last wins silently, so/microsegmentationmapped to the page whosecasewas itself dead.Fourteen security consoles had no route at all —
CspmPage,SbomPage,SoarPage,SecurityPosturePage,SecurityGovernancePage,SecurityObservabilityPage,SecurityPlaybookPage,SecurityThreatPage,SecurityVulnerabilityPage,SamlIdentityProviderPage,ThreatIntelligencePage,ComplianceEvidencePage,ComplianceReportingPage,IncidentResponsePlaybookPage.SecurityPosturePagewas even importedand then never referenced.
UnauthorizedPageaccepted amessageprop that nothing ever passed, so every denial showedthe same generic sentence.
Why none of it was reported
package.jsonhad noeslintConfigfield.react-scriptstherefore ran its ESLint pluginagainst
eslint-config-react-app/baseonly, never loading the ruleset whereno-undef,no-dupe-keysandno-duplicate-caseare errors. I verified this by building with both componentsstill undefined —
Compiled successfully.(I originally wrote in #575 that these were build failures. They were not, and I have corrected the
issue.)
The fix
src/routes/routeRegistry.jsbecomes the single source of truth — one entry per page carrying itscanonical slug, aliases, component, access level, layout flag and dynamic-segment prop name:
Both
App.jsxandAppRoutes.jsxnow derive from it. Three consequences worth calling out:App.jsxno longer keeps its ownnoLayoutPageslist. Whether a page shows the navbar andfooter is a property of the route, declared once (
chrome: false).AppRoutes.jsxno longer hard-codes which prop each page expects. The registry records thatedit-equipmenttakesequipmentIdwhileorderstatustakesorder.routeMap[path] || "landing"fallbackrendered the landing page for any mistyped URL — which is precisely what hid this class of bug.
Denials now name the roles permitted on the route, so
messagefinally carries something.Enabling ESLint caught two more real defects
Adding
eslintConfigimmediately surfaced:Both are used as tab icons. Fixed by importing them.
Six rule families are explicitly switched off:
no-unused-vars,react-hooks/exhaustive-deps,eqeqeq, and threejsx-a11yrules. Roughly 40 files carry pre-existing violations of them.react-scriptspromotes every warning to a build error underCI=true, so leaving them on wouldfail the build for reasons unrelated to this change. Each is worth fixing — as its own PR, where the
diff is reviewable, rather than burying a real regression in ~200 lines of noise. This is recorded
in
docs/frontend-build.md.Verification
scripts/check-routes.jsruns fromprebuildand fails when a referenced component is notimported, an imported module does not exist, a page key or slug is claimed twice, or a
src/pages/auth/*Page.jsxis unregistered.I verified it catches the original defects by re-injecting all three:
It is static analysis rather than a runtime import, so it needs no React runtime, no JSDOM and no
test framework, and finishes in well under a second. It also exits
2if it parses zero routes, soa change to the registry's shape cannot make the checks vacuously pass.
Type of change
Checklist
scripts/check-routes.js, verified againstre-injected defects)
Summary by CodeRabbit
New Features
Bug Fixes
Quality Improvements
Documentation