Handle links from JAM search #236
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe changes introduce support for Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Sidebar
participant Search
participant Outline
participant LocationProvider
User->>Sidebar: Selects Search Tab
Sidebar->>Search: Renders Search (with onSearchFinished)
Search->>LocationProvider: Reads search param from context
Search->>Search: Performs search
Search->>Sidebar: Calls onSearchFinished
Sidebar->>Outline: Sets searchIsDone = true
Outline->>LocationProvider: Reads section param from context
Outline->>Outline: Scrolls to section if searchIsDone and section present
Assessment against linked issues
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for graypaper-reader ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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
🔭 Outside diff range comments (1)
src/components/Tabs/Tabs.tsx (1)
29-49:⚠️ Potential issueGuard against an invalid
activeTabvalue
activeTabIdxis-1when the suppliedactiveTabstring does not exist intabs.
Accessingtabs[-1]below (tabs[activeTabIdx].render()) throws at runtime, breaking the whole UI.-const activeTabIdx = tabs.map((t) => t.name).indexOf(activeTab); +const activeTabIdx = tabs.findIndex((t) => t.name === activeTab); +const safeIdx = activeTabIdx === -1 ? 0 : activeTabIdx; // fallback to first taband replace the two usages of
activeTabIdxinside the JSX withsafeIdx.This prevents uncaught exceptions while still showing some content to the user.
🧹 Nitpick comments (5)
src/components/Outline/Outline.tsx (1)
20-43: Good implementation of section navigation with some improvement opportunitiesThe section navigation implementation uses a smart approach with recursive searching through the outline tree and coordinating with search completion.
A few suggestions to make this more robust:
- Consider adding some visual feedback when a section is not found
- The current implementation uses
includes()for matching, which might match unintended sections if they share common text. Consider more precise matching or prioritizing exact matches.- Add a check to ensure the outline is loaded before attempting to find items
// scroll to section const section = locationParams.section?.toLowerCase(); useEffect(() => { if (section === undefined || searchIsDone === false) { return; } + if (outline.length === 0) { + return; // Wait until outline is loaded + } const findItem = (outline: TOutline): TOutline[0] | undefined => { for (const item of outline) { - if (item.title.toLowerCase().includes(section)) { + // Try exact match first, fall back to includes + if (item.title.toLowerCase() === section) { return item; + } + + // Fall back to partial match if no exact match found + if (item.title.toLowerCase().includes(section)) { + return item; } const res = findItem(item.items); if (res !== undefined) { return res; } } return undefined; }; const itemToScrollTo = findItem(outline); if (itemToScrollTo?.dest) { linkService?.goToDestination(itemToScrollTo.dest); + } else if (section) { + // Optionally provide feedback that section wasn't found + console.warn(`Section "${section}" not found in outline`); } }, [searchIsDone, section, outline, linkService]);src/components/Tabs/Tabs.tsx (1)
33-38: UnnecessaryFragmentwrapperEach iteration already returns a single root
<div>; the surroundingReact.Fragmentadds one extra element to the reconciliation tree without any benefit. Removing it simplifies the DOM.-<React.Fragment key={tab.name}> - <div …>{tab.render()}</div> -</React.Fragment> +<div key={tab.name} …>{tab.render()}</div>src/components/Search/Search.tsx (1)
100-105: Filtering condition is redundant and slightly misleading
xis a tuple[pageIndex, matchesArray], whose length is always2.
Checkingx.length > 0therefore adds no value and may confuse readers.- .filter((x) => x.length > 0 && x[1].length > 0) + .filter(([, matches]) => matches.length > 0)src/components/LocationProvider/LocationProvider.tsx (2)
65-72: Re-serialising the hash dropssearch/sectionpermanently
handleSetLocationParamsintentionally omitssearchandsectionfrom the URL.
However, when invoked fromhandleHashChange(lines 80-86) in response to an incomplete hash, this causes:
- the browser to immediately rewrite the hash without those parameters;
- a second
hashchangeevent that no longer exposes the originalsearch/section, making them unrecoverable on page reload or share.Ensure that the first rewrite honours the original query parameters to keep the page state shareable.
213-218: Header comment mismatches implementationThe comment says “skip the leading ‘/’”, but the code actually removes the leading ‘#’.
Minor, yet worth fixing to avoid confusion for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/components/LocationProvider/LocationProvider.tsx(5 hunks)src/components/NoteManager/NoteManager.tsx(1 hunks)src/components/Outline/Outline.tsx(1 hunks)src/components/Search/Search.tsx(5 hunks)src/components/Sidebar/Sidebar.tsx(2 hunks)src/components/Tabs/Tabs.css(1 hunks)src/components/Tabs/Tabs.tsx(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/Sidebar/Sidebar.tsx (3)
src/components/Outline/Outline.tsx (1)
Outline(10-61)src/components/Search/Search.tsx (1)
Search(7-33)src/components/Tabs/Tabs.tsx (1)
Tabs(18-50)
🪛 GitHub Check: build
src/components/NoteManager/NoteManager.tsx
[failure] 58-58:
Property 'fullVersion' does not exist on type 'ILocationParams'.
🔇 Additional comments (7)
src/components/Tabs/Tabs.css (1)
39-41: Good addition to support the newalwaysRenderprop!This CSS rule elegantly implements the hiding mechanism for inactive tabs when the new
alwaysRenderprop is used. The rule completely removes hidden elements from the layout flow, which ensures inactive tabs don't affect the UI while still preserving their state in the DOM.src/components/Outline/Outline.tsx (1)
10-12: Good implementation of the searchIsDone prop and LocationContext integrationAdding the
searchIsDoneprop and integrating with LocationContext makes the component properly aware of both search state and location parameters, which is essential for the section navigation feature.src/components/Sidebar/Sidebar.tsx (4)
33-39: Well-implemented search completion state trackingGood use of state and callbacks to track search completion status. The comments clearly explain the purpose of this state, and the useCallback is appropriately used to memoize the callback function.
43-43: Good implementation of passing searchIsDone to OutlineProperly connects the search completion state to the Outline component, enabling section navigation after search completes.
51-51: Good implementation of passing onSearchFinished to SearchProperly provides the callback to the Search component, allowing it to signal when search is complete.
59-59: Great use of alwaysRender prop to maintain component stateUsing the alwaysRender prop for Tabs is essential for this feature to work properly. This ensures that the Outline component stays mounted and can respond to search completion events, even when it's not the active tab.
src/components/LocationProvider/LocationProvider.tsx (1)
48-53: PossibleundefinedinversionName
metadata.versions[fullVersion]?.namecan beundefinedwhen the hash contains an unknown commit.
serializeSearchParamswould then placev=undefinedin the URL, which pollutes history and breaks deep-links.Consider a fallback:
-const versionName = fullVersion ? metadata.versions[fullVersion]?.name : … +const versionName = + fullVersion && metadata.versions[fullVersion] + ? metadata.versions[fullVersion].name + : metadata.versions[metadata.latest]?.name;
2 participants
Also fixes #197