qafix: improve click behaviour of single note #335
Conversation
✅ Deploy Preview for graypaper-reader ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughCentralizes URL-hash construction into a new Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant NoteLink as NoteLink Component
participant LocationContext as LocationContext
participant Utils as Hash Utilities
participant LocationProvider as LocationProvider
App->>NoteLink: Render (locationParams)
NoteLink->>LocationContext: getHashFromLocationParams(params)
LocationContext->>Utils: locationParamsToHash(params, metadata)
Utils->>Utils: encode version & selection, serialize search params
Utils-->>LocationContext: hash string
LocationContext-->>NoteLink: return hash
NoteLink->>NoteLink: set href (memo)
App->>NoteLink: User clicks link
NoteLink->>NoteLink: handleNoteLinkClick (reads href)
NoteLink->>App: window.location.hash = href
App->>LocationProvider: hash change
LocationProvider->>LocationProvider: extractSearchParams -> update locationParams
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
Visual Regression Test Report ✅ PassedGithub run id: 18890908522 🔗 Artifacts: Download |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/NoteManager/components/Note.tsx (1)
105-143: Inconsistent version usage between mouse and keyboard handlers.
handleWholeNoteClick(line 118) useslocationParams.versionwhilehandleNoteEnter(line 139) usesnote.original.version. This creates inconsistent behavior: clicking the note navigates to the current context version, but pressing Enter/Space navigates to the note's original version.Apply this diff to align keyboard navigation with mouse navigation:
setLocationParams({ - version: note.original.version, + version: locationParams.version, selectionStart: note.original.selectionStart, selectionEnd: note.original.selectionEnd, });
🧹 Nitpick comments (2)
src/components/NoteManager/components/NoteLink.tsx (1)
76-94: Optimize memoization dependencies for better performance.The
currentVersionLinkdependency array includes entirelocationParamsandnoteobjects, causing re-computation when unrelated properties change. Consider depending only on the specific properties used.Apply this diff to optimize dependencies:
const currentVersionLink = useMemo( () => getHashFromLocationParams({ version: locationParams.version, selectionStart: note.original.selectionStart, selectionEnd: note.original.selectionEnd, }), - [locationParams, note, getHashFromLocationParams], + [locationParams.version, note.original.selectionStart, note.original.selectionEnd, getHashFromLocationParams], ); const originalLink = useMemo( () => getHashFromLocationParams({ version: note.original.version, selectionStart: note.original.selectionStart, selectionEnd: note.original.selectionEnd, }), - [note, getHashFromLocationParams], + [note.original.version, note.original.selectionStart, note.original.selectionEnd, getHashFromLocationParams], );src/components/LocationProvider/LocationProvider.tsx (1)
51-58: Consider aligning type signature with runtime check.The function signature accepts
ILocationParamsbut line 53 checks forundefined. While this defensive check is good for safety, the type signature could explicitly allowundefinedto match the runtime behavior.Apply this diff if you want the type to match the implementation:
- const handleSetLocationParams = useCallback( - (newParams?: ILocationParams) => { + const handleSetLocationParams = useCallback( + (newParams: ILocationParams | undefined) => { if (!newParams) return; const hash = locationParamsToHash(newParams, metadata); window.location.hash = hash; }, [metadata], );Alternatively, if
undefinedshould never be passed, remove the check and keep the current signature.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/components/LocationProvider/LocationProvider.tsx(3 hunks)src/components/LocationProvider/types.ts(1 hunks)src/components/LocationProvider/utils/constants.ts(1 hunks)src/components/LocationProvider/utils/encodePageNumberAndIndex.ts(1 hunks)src/components/LocationProvider/utils/locationParamsToHash.ts(1 hunks)src/components/NoteManager/components/Note.tsx(2 hunks)src/components/NoteManager/components/NoteLink.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
⚙️ CodeRabbit configuration file
When reviewing Tailwind CSS classes, ensure they follow Tailwind 4.x conventions and suggest modern 4.x alternatives for deprecated patterns.
Files:
src/components/LocationProvider/utils/encodePageNumberAndIndex.tssrc/components/NoteManager/components/Note.tsxsrc/components/LocationProvider/utils/constants.tssrc/components/NoteManager/components/NoteLink.tsxsrc/components/LocationProvider/utils/locationParamsToHash.tssrc/components/LocationProvider/types.tssrc/components/LocationProvider/LocationProvider.tsx
🧬 Code graph analysis (5)
src/components/NoteManager/components/Note.tsx (1)
src/components/LocationProvider/LocationProvider.tsx (1)
useLocationContext(28-34)
src/components/NoteManager/components/NoteLink.tsx (2)
src/components/LocationProvider/LocationProvider.tsx (2)
LocationContext(26-26)ILocationContext(15-20)src/components/Outline/OutlineLink.tsx (1)
OutlineLink(4-54)
src/components/LocationProvider/utils/locationParamsToHash.ts (4)
src/components/LocationProvider/types.ts (2)
ILocationParams(3-7)SearchParams(9-14)src/components/MetadataProvider/MetadataProvider.tsx (1)
IMetadataContext(21-29)src/components/LocationProvider/utils/constants.ts (4)
SHORT_COMMIT_HASH_LENGTH(5-5)VERSION_SEGMENT_INDEX(1-1)SELECTION_SEGMENT_INDEX(2-2)SEGMENT_SEPARATOR(3-3)src/components/LocationProvider/utils/encodePageNumberAndIndex.ts (1)
encodePageNumberAndIndex(1-4)
src/components/LocationProvider/types.ts (1)
shared/links-metadata/src/types.ts (1)
ISelectionParams(30-33)
src/components/LocationProvider/LocationProvider.tsx (3)
src/components/LocationProvider/types.ts (1)
ILocationParams(3-7)shared/links-metadata/src/types.ts (2)
ISynctexBlock(21-28)ISelectionParams(30-33)src/components/LocationProvider/utils/locationParamsToHash.ts (1)
locationParamsToHash(11-37)
⏰ 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: visual-tests
🔇 Additional comments (11)
src/components/LocationProvider/types.ts (1)
1-14: LGTM! Clean type definitions.The type definitions are well-structured and provide a clear contract for location parameters and search params. The use of
Partial<ISelectionParams>for optional selection data is appropriate.src/components/NoteManager/components/Note.tsx (1)
58-58: LGTM! Proper context usage.Extracting
locationParamsalongsidesetLocationParamsenables version access throughout the component, aligning with the centralized location state management.src/components/LocationProvider/utils/locationParamsToHash.ts (1)
39-48: LGTM! Clean search params serialization.The helper properly filters out falsy values and constructs the query string only when parameters exist.
src/components/LocationProvider/utils/constants.ts (1)
1-6: LGTM! Well-defined constants.The constants are appropriately named and the values are correct. The
SELECTION_DECOMPOSE_PATTERNproperly matches 6-character hex segments, and theSHORT_COMMIT_HASH_LENGTHaligns with git's default behavior.src/components/NoteManager/components/NoteLink.tsx (3)
25-25: LGTM! Updated to use hash-based navigation.Correctly extracts
locationParamsandgetHashFromLocationParamsfrom context, aligning with the new centralized hash generation approach.
45-50: LGTM! Consolidated click handler.The unified handler properly prevents default navigation and updates the hash programmatically, enabling smooth client-side navigation.
96-134: LGTM! Proper integration of hash-based navigation.The computed links are correctly used in href attributes and the click handlers properly handle navigation through hash updates.
src/components/LocationProvider/LocationProvider.tsx (4)
5-13: LGTM! Clean import organization.All necessary types, constants, and utilities are properly imported to support the centralized hash generation approach.
15-20: LGTM! Extended context interface.Adding
getHashFromLocationParamsto the public API enables consumers to compute hashes without duplicating encoding logic, promoting consistency across the application.
60-66: LGTM! Proper memoization and delegation.The
getHashFromLocationParamsfunction is correctly memoized and delegates to the centralizedlocationParamsToHashutility.
169-180: LGTM! Context properly updated.The context value correctly includes
getHashFromLocationParamsand the dependency array is properly updated to include it.
1 participant
What?
hrefvalue and works nciely with "open in new tab" or copy url.Summary by CodeRabbit
Refactor
UX