fix: use React Router for frontend SPA navigation#3134
Open
adityathebe wants to merge 2 commits into
Open
Conversation
Sidebar and internal UI navigation used a mix of manual history updates, raw anchors, and window.location.href. That forced document reloads and remounted the frontend when users navigated between sidebar routes. Wire the frontend through react-router-dom, define /ui routes declaratively, and convert internal navigation to Link/NavLink/AppLink so route changes update content and active state without rebuilding the shell.
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR completes the migration from a custom history-based routing scheme to React Router v6/v7 across the frontend. It adds the ChangesReact Router Migration
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
ui/frontend/src/TypeView.tsx (1)
259-272: 💤 Low valueConsider preserving link semantics for row navigation.
Switching from
getRowHreftoonRowClickremoves standard link behaviors—users can no longer middle-click or right-click → "Open in new tab" to open config items in separate tabs. If theDataTablecomponent supports bothgetRowHref(for the anchor element) andonRowClick(for SPA interception), combining them would preserve both browser link affordances and client-side navigation.If the component doesn't support that pattern or this UX trade-off is intentional, feel free to disregard.
🤖 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 `@ui/frontend/src/TypeView.tsx` around lines 259 - 272, The row navigation currently uses only onRowClick in ConfigTable, which loses native link semantics; add a getRowHref prop to the DataTable that returns `/item/${encodeURIComponent(String(row.id))}` (matching getRowId and using the same encoding) so each row renders a real anchor, and keep the existing onRowClick handler (navigate(...)) for SPA interception—this preserves middle-click/right-click behavior while retaining client-side navigation via navigate in onRowClick.ui/frontend/src/main.tsx (1)
4-4: ⚡ Quick winAvoid duplicating the UI base path constant.
basename="/ui"duplicatesUI_BASEand can silently drift from link-generation helpers.Diff suggestion
import { BrowserRouter } from "react-router-dom"; +import { UI_BASE } from "./navigation"; @@ - <BrowserRouter basename="/ui"> + <BrowserRouter basename={UI_BASE}> <App /> </BrowserRouter>Also applies to: 49-51
🤖 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 `@ui/frontend/src/main.tsx` at line 4, The Router is hardcoding basename="/ui" which duplicates the existing UI_BASE constant and can drift from link helpers; update main.tsx to import and use the shared UI_BASE constant instead of the literal string wherever basename is set (replace the BrowserRouter basename="/ui" usage and the other occurrences around the 49-51 area), and ensure any link-generation helpers continue to reference the same UI_BASE to keep routing consistent.
🤖 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 `@ui/frontend/src/App.tsx`:
- Around line 141-147: The route components are double-decoding URL params by
calling the decodeParam helper on values already returned decoded by useParams;
update ItemRoute and the TypeRoute/TypeView usage to stop calling decodeParam on
params from useParams (pass id and configType directly from useParams into
ItemView/TypeView), and either remove/rename or change the decodeParam helper to
a no-op for values already from useParams so other call sites aren’t
double-decoded (refer to ItemRoute, TypeView usage, and the decodeParam helper).
In `@ui/frontend/src/navigation.tsx`:
- Around line 6-10: routerPathFromHref currently only treats exact UI_BASE and
UI_BASE + '/' as internal, so URLs like "/ui?x=..." or "/ui#hash" fall through;
update routerPathFromHref to treat any href that equals UI_BASE or starts with
UI_BASE followed by '/', '?' or '#' as internal: if href === UI_BASE return "/";
else if href.startsWith(UI_BASE) and the next character (href[UI_BASE.length])
is '/', '?' or '#' return href.slice(UI_BASE.length) (ensuring leading '/' when
slicing yields empty); adjust the same logic used around lines 16-18 if there is
a symmetric helper to keep behavior consistent.
In `@ui/frontend/src/playbooks/PlaybookBrowser.tsx`:
- Line 1420: The navigate call currently uses
navigate(`/playbooks/runs/${encodeURIComponent(response.run_id)}`) which misses
the required "/ui" route prefix; update this invocation in PlaybookBrowser.tsx
to use the same routing pattern as other occurrences by changing the path to
`/ui/playbooks/runs/${encodeURIComponent(response.run_id)}` so navigation
matches routes used elsewhere (search for the navigate(...) call referencing
response.run_id to locate it).
- Around line 1899-1902: Update the Link in PlaybookBrowser.tsx that currently
navigates to "/playbooks/runs" so it uses the same UI route prefix as the rest
of the component (change the route to "/ui/playbooks/runs"); locate the Link
element near the Clear button (the JSX containing <Link ...> with Icon
name="lucide:x") and modify its to prop to include the "/ui" prefix to match
other references in this file.
---
Nitpick comments:
In `@ui/frontend/src/main.tsx`:
- Line 4: The Router is hardcoding basename="/ui" which duplicates the existing
UI_BASE constant and can drift from link helpers; update main.tsx to import and
use the shared UI_BASE constant instead of the literal string wherever basename
is set (replace the BrowserRouter basename="/ui" usage and the other occurrences
around the 49-51 area), and ensure any link-generation helpers continue to
reference the same UI_BASE to keep routing consistent.
In `@ui/frontend/src/TypeView.tsx`:
- Around line 259-272: The row navigation currently uses only onRowClick in
ConfigTable, which loses native link semantics; add a getRowHref prop to the
DataTable that returns `/item/${encodeURIComponent(String(row.id))}` (matching
getRowId and using the same encoding) so each row renders a real anchor, and
keep the existing onRowClick handler (navigate(...)) for SPA interception—this
preserves middle-click/right-click behavior while retaining client-side
navigation via navigate in onRowClick.
🪄 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: b82a66d5-eece-4663-a866-3c66789e61e2
⛔ Files ignored due to path filters (1)
ui/frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
ui/frontend/package.jsonui/frontend/src/App.tsxui/frontend/src/CatalogSidebar.tsxui/frontend/src/TypeView.tsxui/frontend/src/access/AccessBrowser.tsxui/frontend/src/components/ConfigItemSelector.tsxui/frontend/src/config-detail/ConfigItemDetail.tsxui/frontend/src/layout/DetailPageLayout.tsxui/frontend/src/main.tsxui/frontend/src/navigation.tsxui/frontend/src/playbooks/PlaybookBrowser.tsxui/frontend/src/settings/SettingsBrowser.tsx
React Router v7 already decodes values returned by useParams. Remove the extra decodeURIComponent helper from route wrappers so IDs and config types containing literal percent-encoded sequences are passed through unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sidebar navigation in
ui/frontendwas using a mix of manual history handling, raw anchors, andwindow.location.href, causing full document reloads.\n\nReplace the custom route state layer withreact-router-dom, wire/uiroutes throughBrowserRouter, and convert internal navigation toLink/NavLink.\n\nThe sidebar now stays mounted during navigation; only active selection/content changes update.Summary by CodeRabbit
Release Notes
New Features
Refactor