fix docs navigation stability and search filter interactions#772
Merged
tannerlinsley merged 1 commit intomainfrom Mar 17, 2026
Merged
fix docs navigation stability and search filter interactions#772tannerlinsley merged 1 commit intomainfrom
tannerlinsley merged 1 commit intomainfrom
Conversation
✅ Deploy Preview for tanstack ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThis PR addresses multiple bugs and improvements: fixes Chrome sidebar navigation through menu state management refactoring in DocsLayout, resolves dropdown reopening issues in the search modal by removing modal context blocking, adds proper error handling for missing libraries via notFound() guards across route files, and adds ESLint suppression comments for react-hooks compatibility in admin routes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 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)
📝 Coding Plan
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}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/admin/npm-stats.tsx (1)
271-290:⚠️ Potential issue | 🟡 MinorMissing ESLint suppression on second
useReactTablecall.The suppression comment is added for
libraryTableat line 272, but thepackagesTableat line 286 also usesuseReactTableand lacks the same suppression. If thereact-hooks/incompatible-libraryrule triggers onuseReactTablecalls, both instances should be suppressed for consistency.🔧 Proposed fix
// eslint-disable-next-line react-hooks/incompatible-library const libraryTable = useReactTable({ data: libraryStats ?? [], columns: libraryColumns, getCoreRowModel: getCoreRowModel(), }) const packages = useMemo( () => [...(packagesData?.packages ?? [])].sort( (a, b) => (b.downloads ?? 0) - (a.downloads ?? 0), ), [packagesData?.packages], ) + // eslint-disable-next-line react-hooks/incompatible-library const packagesTable = useReactTable({ data: packages, columns: packageColumns, getCoreRowModel: getCoreRowModel(), })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/admin/npm-stats.tsx` around lines 271 - 290, The second use of useReactTable (creating packagesTable) is missing the ESLint suppression present for libraryTable; add the same disable comment (eslint-disable-next-line react-hooks/incompatible-library) immediately above the packagesTable declaration so both calls to useReactTable (libraryTable and packagesTable) consistently suppress the react-hooks/incompatible-library rule.
🧹 Nitpick comments (3)
src/components/DocsLayout.tsx (2)
669-674: Non-collapsible wrapper has unnecessary summary-targeting CSS classes.The
[&>summary]:before:mr-1 [&>summary]:marker:text-[0.8em] [&>summary]:marker:leading-4classes only apply to a<summary>child, but non-collapsible groups render a<div>child inside this wrapper. These selectors will never match.Consider extracting shared classes or removing the summary-specific selectors from the non-collapsible case:
♻️ Proposed fix
) : ( <div key={`group-${i}`} - className="[&>summary]:before:mr-1 [&>summary]:marker:text-[0.8em] [&>summary]:marker:leading-4 relative select-none" + className="relative select-none" > {groupContent} </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DocsLayout.tsx` around lines 669 - 674, The wrapper div rendering non-collapsible groups contains summary-specific CSS selectors ("[&>summary]:before:mr-1 [&>summary]:marker:text-[0.8em] [&>summary]:marker:leading-4") that will never match because it renders a <div> child (see the wrapper with key={`group-${i}`} and variable groupContent); remove those summary-targeting classes from this non-collapsible wrapper or extract the shared, non-summary classes into a common variable and apply only summary-specific selectors on the collapsible wrapper/element that actually contains a <summary>, so update the className on the non-collapsible wrapper to omit the [&>summary] rules and keep shared styles centralized.
537-550: Consider using a more stable key thanString(group.label)for ReactNode labels.When
group.labelis a React element (e.g., the GitHub/YouTube/Discord menu items),String(group.label)returns"[object Object]". The index prefix prevents collisions, but if menu items are conditionally included/excluded, the same group could get different keys across renders, causing unexpected open/close state changes.Consider adding a stable
idfield toMenuItemtype, or using a hash of the label string when available:const key = typeof group.label === 'string' ? `${index}:${group.label}` : `${index}:group-${index}`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DocsLayout.tsx` around lines 537 - 550, The current key generation in groupInitialOpenState uses String(group.label) which is unstable for ReactNode labels; update the key logic to use a stable identifier: prefer an explicit id property on the group (e.g., group.id) if present, otherwise use the label when it's a string, and as a final fallback produce a deterministic fallback based on the group's index (e.g., `group-${index}`); update the key assignment in the reduce callback (the variable currently named key) and ensure any type/interface for menuConfig/MenuItem supports an optional id so state remains consistent across conditional renders.src/routes/$libraryId/$version.tsx (1)
13-19: Consider centralizing the repeatedfindLibrary/notFound()guard.The same lookup/404 block now lives in
beforeLoad,loader, andRouteForm, with the same pattern repeated in the sibling routes in this PR. A small shared helper would make future changes to library resolution or 404 behavior much easier to keep consistent.♻️ Helper sketch
+function requireLibrary(libraryId: string) { + const library = findLibrary(libraryId) + if (!library) { + throw notFound() + } + return library +} + beforeLoad: (ctx) => { const { libraryId, version } = ctx.params - const library = findLibrary(libraryId) - - if (!library) { - throw notFound() - } + const library = requireLibrary(libraryId)Also applies to: 31-35, 53-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`$libraryId/$version.tsx around lines 13 - 19, The repeated pattern using findLibrary(...) followed by throw notFound() should be centralized: add a small helper (e.g., ensureLibraryOrThrow(libraryId) or resolveLibraryOr404) that calls findLibrary and throws notFound() when missing, then replace the duplicate blocks in beforeLoad, loader, and the RouteForm component with calls to that helper (use the helper name in each location to make intent clear and keep behavior consistent across sibling routes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/routes/admin/npm-stats.tsx`:
- Around line 271-290: The second use of useReactTable (creating packagesTable)
is missing the ESLint suppression present for libraryTable; add the same disable
comment (eslint-disable-next-line react-hooks/incompatible-library) immediately
above the packagesTable declaration so both calls to useReactTable (libraryTable
and packagesTable) consistently suppress the react-hooks/incompatible-library
rule.
---
Nitpick comments:
In `@src/components/DocsLayout.tsx`:
- Around line 669-674: The wrapper div rendering non-collapsible groups contains
summary-specific CSS selectors ("[&>summary]:before:mr-1
[&>summary]:marker:text-[0.8em] [&>summary]:marker:leading-4") that will never
match because it renders a <div> child (see the wrapper with key={`group-${i}`}
and variable groupContent); remove those summary-targeting classes from this
non-collapsible wrapper or extract the shared, non-summary classes into a common
variable and apply only summary-specific selectors on the collapsible
wrapper/element that actually contains a <summary>, so update the className on
the non-collapsible wrapper to omit the [&>summary] rules and keep shared styles
centralized.
- Around line 537-550: The current key generation in groupInitialOpenState uses
String(group.label) which is unstable for ReactNode labels; update the key logic
to use a stable identifier: prefer an explicit id property on the group (e.g.,
group.id) if present, otherwise use the label when it's a string, and as a final
fallback produce a deterministic fallback based on the group's index (e.g.,
`group-${index}`); update the key assignment in the reduce callback (the
variable currently named key) and ensure any type/interface for
menuConfig/MenuItem supports an optional id so state remains consistent across
conditional renders.
In `@src/routes/`$libraryId/$version.tsx:
- Around line 13-19: The repeated pattern using findLibrary(...) followed by
throw notFound() should be centralized: add a small helper (e.g.,
ensureLibraryOrThrow(libraryId) or resolveLibraryOr404) that calls findLibrary
and throws notFound() when missing, then replace the duplicate blocks in
beforeLoad, loader, and the RouteForm component with calls to that helper (use
the helper name in each location to make intent clear and keep behavior
consistent across sibling routes).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5e7c8b8-b9ee-45ea-b19d-ce6edca840fb
📒 Files selected for processing (11)
src/components/CopyPageDropdown.tsxsrc/components/DocsLayout.tsxsrc/components/SearchModal.tsxsrc/routes/$libraryId/$version.docs.tsxsrc/routes/$libraryId/$version.index.tsxsrc/routes/$libraryId/$version.tsxsrc/routes/admin/audit.tsxsrc/routes/admin/github-stats.tsxsrc/routes/admin/logins.tsxsrc/routes/admin/npm-stats.tsxsrc/routes/admin/roles.$roleId.tsx
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
2 participants
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.
Summary
notFound()behavior instead of throwing runtime errorsIssues addressed
Details
Docs navigation
The docs sidebar was relying on render-time
<details open>behavior instead of explicit React state, which could break collapsible group interaction in Chrome. This change makes group open/close state controlled and keeps the active section expanded.Sidebar link preloading is also disabled in this path to reduce the chance of hover-triggered instability on heavy docs pages.
Search modal
The search palette library/framework selectors were using dropdown trigger composition that could interfere with focus and reopening behavior inside the dialog. This change switches them to proper button triggers and uses non-modal dropdown behavior inside the modal.
Invalid route handling
Several
/$libraryId/...routes now usefindLibrary(...)withnotFound()so invalid library IDs render a proper not-found state instead of throwing.Tooling stability
The smoke test now allows more time for the dev server to start while
tscandeslintrun in parallel, and it prints recent server output if startup times out.Admin route
useReactTable(...)call sites now explicitly suppress the React Compilerincompatible-librarywarning, which keeps lint output clean without changing table behavior.Validation
pnpm run lintpnpm testSummary by CodeRabbit
Bug Fixes
Improvements