feat(devtools-core): add history tab #5410
Conversation
|
View your CI Pipeline Execution ↗ for commit 326a007
☁️ Nx Cloud last updated this comment at |
WalkthroughIntroduces a three-tab panel state ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Panel as DevtoolsPanel
participant Router as Router
participant Storage as localStorage
note over Panel: Initialize currentTab from Storage (tanstackRouterDevtoolsActiveTab)
Dev->>Panel: Click tab (routes | matches | history)
Panel->>Panel: set currentTab()
Panel->>Storage: persist currentTab
Router-->>Panel: match/pathname change
rect rgba(200,230,255,0.35)
note right of Panel: createEffect (uses untrack) prepends match,\ndeduplicates by location and truncates to HISTORY_LIMIT
Panel->>Panel: update history signal
end
alt currentTab == routes
Panel-->>Dev: render routes view (RouteComp)
else currentTab == matches
Panel-->>Dev: render matches list (NavigateButton + AgeTicker)
else currentTab == history
Panel-->>Dev: render history list (scrollable) with AgeTicker and overflow note
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20-30 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/nitro-v2-vite-plugin
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-ssr-query-core
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-static-server-functions
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx(4 hunks)packages/router-devtools-core/src/useStyles.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-devtools-core/src/useStyles.tsxpackages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx (1)
305-323: Remove duplicate history tracking effect.This entire
createEffectblock should be removed. You've already added the correct history tracking logic at lines 282-303 that properly comparespathnameandsearch. This second effect:
- Creates duplicate history entries on every navigation
- Still uses
match.idcomparison, which was flagged in the previous review as missing same-route navigations- Doesn't guard against empty
matchesarray (line 307 could beundefined)- Would fail at line 315 when accessing
currentPathname.idif matches is emptyApply this diff to remove the duplicate effect:
- createEffect(() => { - const currentPathname = - routerState().matches[routerState().matches.length - 1] - - // Read history WITHOUT tracking it to avoid infinite loops - const lastPathname = untrack(() => { - const h = history() - return h[0] - }) - - if (!lastPathname || lastPathname.id !== currentPathname.id) { - setHistory((prev) => { - const newHistory = [currentPathname, ...prev] - // truncate to ensure we don't overflow too much the ui - newHistory.splice(15) - return newHistory - }) - } - }) -
🧹 Nitpick comments (1)
packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx (1)
578-617: History tab rendering looks good.The implementation correctly:
- Iterates over history entries with
For- Shows current location in green (
index() === 0), past entries in gray- Provides navigation buttons and age tickers
- Displays route ID or pathname
Optional: The
<ul>wrapper at line 580 is unnecessary and can be removed:- <div> - <ul> - <For each={history()}> + <div> + <For each={history()}> {(match, index) => ( <li ... </li> )} - </For> - </ul> - </div> + </For> + </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx
🧬 Code graph analysis (1)
packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx (1)
packages/router-devtools-core/src/NavigateButton.tsx (1)
NavigateButton(9-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (2)
packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx (2)
282-303: LGTM! History tracking logic is correct.The effect properly:
- Guards against empty matches
- Uses
untrackto prevent infinite loops- Compares
pathnameandsearchto detect all navigation changes (including same-route param/search changes)- Handles the initial case when
lastMatchis undefinedThis correctly addresses the concerns from the previous review.
513-516: Verify the third parameter for the History button.The History button passes
falseas the third parameter torouteMatchesToggleBtn, while the Routes and Matches buttons passtrue.Confirm this is intentional (e.g., different styling for the last button) rather than an oversight.
packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx
Outdated
Show resolved
Hide resolved
SeanCassiere
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx (1)
307-323: Clean up the redundant history effectThe new effect above already handles tracking and dedupe, so this second effect never fires (and still hardcodes
15). Dropping it avoids confusion and keeps the truncation logic in one place usingHISTORY_LIMIT.- createEffect(() => { - const currentPathname = - routerState().matches[routerState().matches.length - 1] - - // Read history WITHOUT tracking it to avoid infinite loops - const lastPathname = untrack(() => { - const h = history() - return h[0] - }) - - if (!lastPathname || lastPathname.id !== currentPathname.id) { - setHistory((prev) => { - const newHistory = [currentPathname, ...prev] - // truncate to ensure we don't overflow too much the ui - newHistory.splice(15) - return newHistory - }) - } - })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx
🧬 Code graph analysis (1)
packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx (1)
packages/router-devtools-core/src/NavigateButton.tsx (1)
NavigateButton(9-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
|
I also added an overflow indicator on the panel
I was thinking of doing this in a separate pr, it will require a bit more work and rework of some devtool state |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx(5 hunks)packages/router-devtools-core/src/useStyles.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/router-devtools-core/src/useStyles.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx
🧬 Code graph analysis (1)
packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx (1)
packages/router-devtools-core/src/NavigateButton.tsx (1)
NavigateButton(9-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (5)
packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx (5)
68-69: LGTM!Good extraction of the magic number into a named constant.
273-283: LGTM!State initialization is well-structured with proper types and localStorage persistence.
285-310: LGTM!The history tracking logic correctly:
- Guards against empty matches
- Uses
untrackto prevent infinite reactive loops- Normalizes both sides of the search comparison with
?? {}- Deduplicates by location (pathname + search) rather than route id
- Tracks overflow for UI indication
483-527: LGTM!Tab button implementation is consistent and correctly manages active/disabled states.
534-631: LGTM!Tab content rendering is well-structured:
- Proper use of
Switch/Matchfor exclusive tab rendering- History list uses
Forwith proper key management- Overflow indicator provides good UX feedback
- Navigation buttons and age tickers integrated consistently
packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx
Outdated
Show resolved
Hide resolved
@beaussan could you reduce the text-size of this indicator please? Provided no other changes are required, I'll merge this in after that fix comes in. |
packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx
Outdated
Show resolved
Hide resolved
|
Text size and content changed @SeanCassiere ! I've used the 
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/router-devtools-core/src/useStyles.tsx (1)
407-412: Unused style definition.The
historyContainerstyle is defined but never referenced inBaseTanStackRouterDevtoolsPanel.tsx. The History tab content is rendered inside the sharedroutesContainerwrapper (line 513 in the panel file), which may not provide optimal overflow behavior for the history list.Consider either:
- Removing this unused style definition, or
- Applying it to wrap the History tab content for consistent container styling across tabs:
<Match when={currentTab() === 'history'}> <div class={styles().historyContainer}> <ul> {/* existing history list content */} </ul> </div> </Match>packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx (2)
285-310: Consider deep equality for search comparison.The history deduplication logic correctly tracks location changes and addresses previous review feedback. However,
JSON.stringifycan be brittle for complex objects with different key orders, circular references, or non-JSON-serializable values.For more robust comparison, consider using a deep equality function (many libraries provide this) or document the assumption that search params are simple key-value pairs:
// Option 1: Use a deep equality library import { isEqual } from 'lodash-es' // or similar const sameLocation = lastMatch && lastMatch.pathname === currentMatch.pathname && isEqual(lastMatch.search ?? {}, currentMatch.search ?? {}) // Option 2: Add a comment documenting the constraint // Assumes search params are simple key-value pairs without circular refs const sameLocation = lastMatch && lastMatch.pathname === currentMatch.pathname && JSON.stringify(lastMatch.search ?? {}) === JSON.stringify(currentMatch.search ?? {})That said, the current implementation is likely sufficient for typical router search params.
565-610: History items styled as interactive but lack click handlers.The history list items (lines 570-599) use
matchRowstyling, which includescursor: pointerand other interactive visual cues, but the<li>elements don't haveonClickhandlers orrole="button"attributes. In contrast, the Matches tab items (lines 532-560) correctly pairmatchRowstyling with click handlers.The current implementation works since users can click the
NavigateButtoninside each item. However, for consistency with the Matches tab and to avoid misleading cursor states, consider either:
- Adding click handlers to history items to navigate on item click (matching Matches tab behavior), or
- Using a different style variant that doesn't imply the entire row is clickable:
// Option 1: Add click handlers (matches the Matches tab pattern) <li role="button" aria-label={`Navigate to ${match.pathname}`} onClick={() => router().navigate({ to: match.pathname, params: match.params, search: match.search })} class={cx(styles().matchRow(match === activeMatch()))} > // Option 2: Create a non-interactive row style variant // In useStyles.tsx, add a historyRow style without cursor: pointer historyRow: (active: boolean) => { const base = css` display: flex; border-bottom: 1px solid ${colors.darkGray[400]}; align-items: center; padding: ${size[1]} ${size[2]}; // ... other styles without cursor: pointer ` // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx(5 hunks)packages/router-devtools-core/src/useStyles.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-devtools-core/src/useStyles.tsxpackages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx
🧬 Code graph analysis (1)
packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx (1)
packages/router-devtools-core/src/NavigateButton.tsx (1)
NavigateButton(9-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (4)
packages/router-devtools-core/src/useStyles.tsx (1)
413-416: LGTM!The overflow indicator styling correctly uses a small font size (
xs) as requested in the PR comments, with appropriate padding.packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx (3)
4-14: LGTM!The new imports (
For,Match,Switch,createEffect,untrack) are appropriate for implementing the history tab with Solid.js reactive patterns.
68-68: LGTM!The
HISTORY_LIMITconstant is correctly defined as requested in previous review comments and used consistently throughout the file.
463-507: LGTM!The tab button implementation correctly manages the three-tab state with proper disabled states and border styling.
|
Looks like nx is having a moment right now. I've tested this and it seems to work. Merging this in. |
2 participants
Adds a history panel to the devtools, super useful to be able to track down redirects.
If you don't think it should land, I'm totally ok by building it in userland too.
Ideally, I'd also want to, when I click on a history item, have a side panel that shows the matches for this given pathname, but that's a bigger change that I rather wait until I'm given confirmation it's wanted.
Demo: https://discord.com/channels/719702312431386674/1425569815304863844/1425607969680130058
Summary by CodeRabbit
New Features
Refactor
Style