Conversation
WalkthroughReworks how version, hash, and selection are consumed and propagated: adds a VersionProvider and version hook, extracts hash logic to a hook, replaces LocationContext usage in Note components with an onSelectNote callback, memoizes CodeSync context, and adjusts NoteLink/NoteList APIs and wiring. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as NotesList / Note UI
participant NoteMgr as NoteManager
participant LocProv as LocationProvider
participant VersionProv as VersionProvider
participant NoteLink as NoteLink
UI->>NoteMgr: onSelectNote(note)
NoteMgr->>LocProv: setLocationParams({ version, selectionStart, selectionEnd })
NoteMgr-->>UI: note becomes active / re-render
NoteLink->>VersionProv: useVersionContext() -> version
NoteLink->>LocProv: getHashFromLocationParams(...) via hook
NoteLink-->>UI: render href with computed hash
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)
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 |
✅ Deploy Preview for graypaper-reader ready!
To edit notification comments on pull requests, go to your Netlify project 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: 1
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)
107-111: Handler not updated to match click behavior.This keyboard handler still uses
note.original.*values, whilehandleWholeNoteClick(lines 86-90) now usesnote.current.*. This means clicking and keyboard activation will restore different selection states, which is likely unintended.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/NoteManager/components/Note.tsx(1 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/NoteManager/components/Note.tsx
⏰ 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). (4)
- GitHub Check: Redirect rules - graypaper-reader
- GitHub Check: Header rules - graypaper-reader
- GitHub Check: Pages changed - graypaper-reader
- GitHub Check: visual-tests
Visual Regression Test Report ✅ PassedGithub run id: 19408751054 🔗 Artifacts: Download |
chmurson
force-pushed
the
fix-selection-notes-problem
branch
from
2c55af1 to
ff507fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/App.tsx(2 hunks)src/components/CodeSyncProvider/CodeSyncProvider.tsx(2 hunks)src/components/LocationProvider/LocationProvider.tsx(3 hunks)src/components/LocationProvider/VersionProvider.tsx(1 hunks)src/components/LocationProvider/hooks/useGetLocationParamsToHash.ts(1 hunks)src/components/MetadataProvider/MetadataProvider.tsx(2 hunks)src/components/NoteManager/NoteManager.tsx(4 hunks)src/components/NoteManager/components/Note.tsx(4 hunks)src/components/NoteManager/components/NoteLayout.tsx(1 hunks)src/components/NoteManager/components/NoteLink.tsx(3 hunks)src/components/NoteManager/components/NotesList.tsx(2 hunks)src/main.tsx(1 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/VersionProvider.tsxsrc/components/LocationProvider/hooks/useGetLocationParamsToHash.tssrc/components/NoteManager/components/Note.tsxsrc/App.tsxsrc/components/NoteManager/components/NoteLayout.tsxsrc/components/NoteManager/components/NoteLink.tsxsrc/components/NoteManager/NoteManager.tsxsrc/components/LocationProvider/LocationProvider.tsxsrc/main.tsxsrc/components/NoteManager/components/NotesList.tsxsrc/components/MetadataProvider/MetadataProvider.tsxsrc/components/CodeSyncProvider/CodeSyncProvider.tsx
🧠 Learnings (5)
📓 Common learnings
Learnt from: chmurson
Repo: FluffyLabs/graypaper-reader PR: 330
File: src/components/Outline/OutlineLink.tsx:17-21
Timestamp: 2025-10-22T20:36:10.440Z
Learning: This repo targets React 19+; prefer passing refs as a normal prop to function components and avoid React.forwardRef in new code. File context: src/components/Outline/OutlineLink.tsx.
📚 Learning: 2025-10-22T20:36:10.440Z
Learnt from: chmurson
Repo: FluffyLabs/graypaper-reader PR: 330
File: src/components/Outline/OutlineLink.tsx:17-21
Timestamp: 2025-10-22T20:36:10.440Z
Learning: This repo targets React 19+; prefer passing refs as a normal prop to function components and avoid React.forwardRef in new code. File context: src/components/Outline/OutlineLink.tsx.
Applied to files:
src/components/LocationProvider/VersionProvider.tsxsrc/components/NoteManager/components/Note.tsxsrc/App.tsxsrc/components/NoteManager/components/NoteLayout.tsxsrc/components/NoteManager/components/NoteLink.tsxsrc/components/NoteManager/NoteManager.tsxsrc/components/LocationProvider/LocationProvider.tsxsrc/main.tsxsrc/components/NoteManager/components/NotesList.tsxsrc/components/MetadataProvider/MetadataProvider.tsxsrc/components/CodeSyncProvider/CodeSyncProvider.tsx
📚 Learning: 2025-10-22T20:36:10.440Z
Learnt from: chmurson
Repo: FluffyLabs/graypaper-reader PR: 330
File: src/components/Outline/OutlineLink.tsx:17-21
Timestamp: 2025-10-22T20:36:10.440Z
Learning: Project targets React 19+; prefer passing refs as a regular prop in function components and avoid React.forwardRef in new code. File: src/components/Outline/OutlineLink.tsx.
Applied to files:
src/components/NoteManager/components/Note.tsxsrc/components/NoteManager/components/NoteLayout.tsxsrc/components/NoteManager/components/NoteLink.tsxsrc/components/NoteManager/NoteManager.tsxsrc/components/NoteManager/components/NotesList.tsxsrc/components/CodeSyncProvider/CodeSyncProvider.tsx
📚 Learning: 2025-10-22T20:36:10.440Z
Learnt from: chmurson
Repo: FluffyLabs/graypaper-reader PR: 330
File: src/components/Outline/OutlineLink.tsx:17-21
Timestamp: 2025-10-22T20:36:10.440Z
Learning: Tailwind CSS v4 is used; avoid non-default width utilities like border-b-1 unless configured. Prefer border-b (1px) or border-b-[1px]. File context: src/components/Outline/OutlineLink.tsx.
Applied to files:
src/components/NoteManager/components/NoteLayout.tsxsrc/components/NoteManager/components/NoteLink.tsx
📚 Learning: 2025-10-28T20:41:06.887Z
Learnt from: chmurson
Repo: FluffyLabs/graypaper-reader PR: 333
File: src/components/PdfProvider/PdfProvider.tsx:34-37
Timestamp: 2025-10-28T20:41:06.887Z
Learning: In the graypaper-reader codebase, for text layer render tracking in PdfProvider, use RefObject<number[]> approach (textLayerRenderedRef) without adding reactive signals or version numbers. Consumers should check the ref imperatively when needed rather than reacting to changes.
Applied to files:
src/components/CodeSyncProvider/CodeSyncProvider.tsx
🧬 Code graph analysis (11)
src/components/LocationProvider/VersionProvider.tsx (1)
src/components/LocationProvider/LocationProvider.tsx (1)
useLocationContext(29-35)
src/components/LocationProvider/hooks/useGetLocationParamsToHash.ts (3)
src/components/MetadataProvider/MetadataProvider.tsx (1)
useMetadataContext(37-43)src/components/LocationProvider/types.ts (1)
ILocationParams(3-7)src/components/LocationProvider/utils/locationParamsToHash.ts (1)
locationParamsToHash(11-38)
src/components/NoteManager/components/Note.tsx (2)
src/components/NotesProvider/types/DecoratedNote.ts (1)
IDecoratedNote(4-19)src/components/NoteManager/components/NoteLink.tsx (1)
NoteLink(18-105)
src/App.tsx (1)
src/components/LocationProvider/VersionProvider.tsx (1)
useVersionContext(6-14)
src/components/NoteManager/components/NoteLayout.tsx (2)
src/components/NoteManager/components/NoteContext.tsx (1)
useNoteContext(19-25)src/components/NoteManager/components/NoteLink.tsx (1)
NoteLink(18-105)
src/components/NoteManager/components/NoteLink.tsx (8)
src/components/NotesProvider/types/DecoratedNote.ts (1)
IDecoratedNote(4-19)src/components/CodeSyncProvider/CodeSyncProvider.tsx (2)
CodeSyncContext(31-31)ICodeSyncContext(11-22)src/components/LocationProvider/VersionProvider.tsx (1)
useVersionContext(6-14)src/components/LocationProvider/hooks/useGetLocationParamsToHash.ts (1)
useGetLocationParamsToHash(6-18)src/components/LocationProvider/LocationProvider.tsx (2)
LocationContext(27-27)ILocationContext(16-21)src/components/NoteManager/components/NoteContext.tsx (1)
useNoteContext(19-25)src/components/SelectionProvider/SelectionProvider.tsx (2)
SelectionContext(35-35)ISelectionContext(21-29)shared/links-metadata/src/utils.ts (1)
isSameBlock(3-9)
src/components/NoteManager/NoteManager.tsx (2)
src/components/LocationProvider/LocationProvider.tsx (2)
LocationContext(27-27)ILocationContext(16-21)src/components/NotesProvider/types/DecoratedNote.ts (1)
IDecoratedNote(4-19)
src/components/LocationProvider/LocationProvider.tsx (1)
src/components/LocationProvider/hooks/useGetLocationParamsToHash.ts (1)
useGetLocationParamsToHash(6-18)
src/main.tsx (3)
src/components/LocationProvider/LocationProvider.tsx (1)
LocationProvider(37-183)src/components/LocationProvider/VersionProvider.tsx (1)
VersionProvider(16-27)src/App.tsx (1)
App(19-55)
src/components/NoteManager/components/NotesList.tsx (3)
src/components/NoteManager/components/Note.tsx (1)
Note(24-195)src/components/NotesProvider/types/DecoratedNote.ts (1)
IDecoratedNote(4-19)src/components/NotesProvider/types/StorageNote.ts (1)
IStorageNote(5-5)
src/components/CodeSyncProvider/CodeSyncProvider.tsx (1)
shared/links-metadata/src/migrate.ts (1)
migrateSelection(107-160)
⏰ 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 (13)
src/main.tsx (1)
21-23: LGTM! Provider hierarchy is correct.The VersionProvider is properly positioned as a child of LocationProvider, which is necessary since VersionProvider reads
locationParams.versionviauseLocationContext().src/components/NoteManager/components/NoteLayout.tsx (1)
21-34: LGTM! Component API updated correctly.The changes align with the refactored NoteLink component that now accepts an
activeprop instead ofonEditNote. The SelectedText component correctly passesactive={true}to indicate an active note state.src/components/MetadataProvider/MetadataProvider.tsx (1)
37-43: LGTM! Standard context hook pattern.The new
useMetadataContexthook follows the same pattern as other context hooks in the codebase (useLocationContext,useVersionContext), providing runtime safety by throwing an error when used outside the provider.src/components/LocationProvider/LocationProvider.tsx (1)
62-62: LGTM! Hash generation logic properly extracted.The refactoring to use
useGetLocationParamsToHashhook centralizes the hash generation logic and maintains the same public API surface through the context.src/components/CodeSyncProvider/CodeSyncProvider.tsx (1)
199-216: LGTM! Context value properly memoized.Memoizing the context object prevents unnecessary re-renders of consumers when the provider re-renders but the context value hasn't changed. The dependency array correctly includes all six callback functions.
src/components/LocationProvider/hooks/useGetLocationParamsToHash.ts (1)
1-18: LGTM! Clean extraction of hash generation logic.The hook properly encapsulates the location-to-hash conversion logic, uses
useMetadataContextfor metadata access, and correctly memoizes the hash function withmetadataas the dependency.src/components/NoteManager/components/NotesList.tsx (2)
1-6: LGTM! Proper use of React.memo.Memoizing the Note component prevents unnecessary re-renders when the NotesList re-renders but individual note props haven't changed, which is a reasonable optimization for a list of moderately complex components.
13-34: LGTM! onSelectNote callback properly propagated.The
onSelectNoteprop is correctly added to the component interface and passed down to eachMemoizedNote, enabling the selection behavior described in the PR objectives.src/components/LocationProvider/VersionProvider.tsx (1)
1-27: LGTM! Well-structured provider following established patterns.The VersionProvider correctly:
- Extracts version from LocationProvider via
useLocationContext()- Memoizes the context value based on
locationParams.version- Provides a runtime-safe hook with clear error messaging
- Follows the same pattern as other context providers in the codebase
src/App.tsx (1)
9-9: App now depends onVersionProviderforversionSwitching to
useVersionContextand threadingversionintoPdfProviderlooks good and aligns with the new provider model. Just ensureAppis always rendered under<VersionProvider>(likely inmain.tsx), otherwise this hook will throw at runtime.Also applies to: 20-20, 28-28
src/components/NoteManager/components/Note.tsx (1)
16-22: UnifiedonSelectNotecallback fixes click/keyboard divergenceRouting both
handleWholeNoteClickandhandleNoteEnterthroughonSelectNote(note)gives consistent behavior for mouse and keyboard activation and removes the previous direct LocationContext coupling from this component. The prop-based approach is also in line with the project’s React 19 “no forwardRef for new code” guideline.Also applies to: 24-24, 72-85, 87-102
src/components/NoteManager/components/NoteLink.tsx (2)
107-135: UpdateVersionLink migration flow looks correct and nicely encapsulatedThe new
UpdateVersionLinkcleanly:
- Reads the current selection from
LocationContext.- Compares it to
note.current.selectionStart/EndusingisSameBlockand prompts if changed.- Writes back via
onEditNoteusingversionfromuseVersionContext.This encapsulates migration logic away from the main Note/NoteLink components and reuses existing contexts appropriately.
15-18: The review comment is based on incomplete code analysis and reaches an incorrect conclusion.The review missed a critical call site:
NoteLayout.tsxline 28 renders<NoteLink note={note} active={true} />within theSelectedTextcomponent. This meansUpdateVersionLinkis properly reachable whenmigrationFlag && isEditable && activeconditions are met. The code is correct as-is and does not contain unreachable logic.The review's suggestion to remove the
activeguard would break the intended behavior by showing the update link in contexts where it should not appear.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/NoteManager/components/NoteLink.tsx (1)
57-65: Past review comment addressed: currentVersionLink now correctly usesnote.current.The link now uses
note.current.selectionStartandnote.current.selectionEndinstead ofnote.original, ensuring that for migrated notes the "current version" link points to the migrated location. This aligns the link target with the displayed page number and section titles.
🧹 Nitpick comments (1)
src/components/NoteManager/components/NoteLink.tsx (1)
137-148: Consider usingdisabledattribute for better accessibility.The link uses a "disabled" CSS class when
selectedBlocks.length === 0but remains clickable. While the handler guards against missing selection (line 117), using a<button disabled={...}>or preventing the click event would provide clearer semantics and better accessibility.Example:
return ( - <a + <button + type="button" + disabled={selectedBlocks.length === 0} href="#" onClick={handleMigrateClick} data-tooltip-id="note-link" data-tooltip-content="Make sure the selection is accurate or adjust it in the current version and update the note." data-tooltip-place="top" - className={`default-link ${selectedBlocks.length === 0 ? "disabled update" : "update"}`} + className={`default-link update`} > (update version) - </a> + </button> );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/LocationProvider/LocationProvider.tsx(2 hunks)src/components/NoteManager/NoteManager.tsx(4 hunks)src/components/NoteManager/components/NoteLink.tsx(3 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/LocationProvider.tsxsrc/components/NoteManager/components/NoteLink.tsxsrc/components/NoteManager/NoteManager.tsx
🧠 Learnings (4)
📓 Common learnings
Learnt from: chmurson
Repo: FluffyLabs/graypaper-reader PR: 330
File: src/components/Outline/OutlineLink.tsx:17-21
Timestamp: 2025-10-22T20:36:10.440Z
Learning: This repo targets React 19+; prefer passing refs as a normal prop to function components and avoid React.forwardRef in new code. File context: src/components/Outline/OutlineLink.tsx.
📚 Learning: 2025-10-22T20:36:10.440Z
Learnt from: chmurson
Repo: FluffyLabs/graypaper-reader PR: 330
File: src/components/Outline/OutlineLink.tsx:17-21
Timestamp: 2025-10-22T20:36:10.440Z
Learning: This repo targets React 19+; prefer passing refs as a normal prop to function components and avoid React.forwardRef in new code. File context: src/components/Outline/OutlineLink.tsx.
Applied to files:
src/components/LocationProvider/LocationProvider.tsxsrc/components/NoteManager/components/NoteLink.tsxsrc/components/NoteManager/NoteManager.tsx
📚 Learning: 2025-10-22T20:36:10.440Z
Learnt from: chmurson
Repo: FluffyLabs/graypaper-reader PR: 330
File: src/components/Outline/OutlineLink.tsx:17-21
Timestamp: 2025-10-22T20:36:10.440Z
Learning: Project targets React 19+; prefer passing refs as a regular prop in function components and avoid React.forwardRef in new code. File: src/components/Outline/OutlineLink.tsx.
Applied to files:
src/components/NoteManager/components/NoteLink.tsxsrc/components/NoteManager/NoteManager.tsx
📚 Learning: 2025-10-22T20:36:10.440Z
Learnt from: chmurson
Repo: FluffyLabs/graypaper-reader PR: 330
File: src/components/Outline/OutlineLink.tsx:17-21
Timestamp: 2025-10-22T20:36:10.440Z
Learning: Tailwind CSS v4 is used; avoid non-default width utilities like border-b-1 unless configured. Prefer border-b (1px) or border-b-[1px]. File context: src/components/Outline/OutlineLink.tsx.
Applied to files:
src/components/NoteManager/components/NoteLink.tsx
🧬 Code graph analysis (3)
src/components/LocationProvider/LocationProvider.tsx (1)
src/components/LocationProvider/hooks/useGetLocationParamsToHash.ts (1)
useGetLocationParamsToHash(6-18)
src/components/NoteManager/components/NoteLink.tsx (8)
src/components/NotesProvider/types/DecoratedNote.ts (1)
IDecoratedNote(4-19)src/components/CodeSyncProvider/CodeSyncProvider.tsx (2)
CodeSyncContext(31-31)ICodeSyncContext(11-22)src/components/LocationProvider/VersionProvider.tsx (1)
useVersionContext(6-14)src/components/LocationProvider/hooks/useGetLocationParamsToHash.ts (1)
useGetLocationParamsToHash(6-18)src/components/LocationProvider/LocationProvider.tsx (2)
LocationContext(27-27)ILocationContext(16-21)src/components/NoteManager/components/NoteContext.tsx (1)
useNoteContext(19-25)src/components/SelectionProvider/SelectionProvider.tsx (2)
SelectionContext(35-35)ISelectionContext(21-29)shared/links-metadata/src/utils.ts (1)
isSameBlock(3-9)
src/components/NoteManager/NoteManager.tsx (2)
src/components/LocationProvider/LocationProvider.tsx (2)
LocationContext(27-27)ILocationContext(16-21)src/components/NotesProvider/types/DecoratedNote.ts (1)
IDecoratedNote(4-19)
⏰ 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 (2)
src/components/LocationProvider/LocationProvider.tsx (1)
5-5: LGTM! Clean extraction to a dedicated hook.The hash computation logic is now properly extracted into
useGetLocationParamsToHash, which memoizes the function based on metadata. This improves separation of concerns and makes the logic reusable across components.Also applies to: 61-61
src/components/NoteManager/NoteManager.tsx (1)
71-80: Ref pattern now works correctly; stale value issue resolved.The ref is now updated on every render (line 72), which addresses the stale value concern from the previous review. This pattern—keeping the callback stable with an empty dependency array while accessing fresh values via
ref.current—is a valid React optimization to preventMemoizedNotesListfrom re-rendering whenlocationParams.versionchanges.While the more idiomatic approach suggested in the previous review would work, it would cause the callback to be recreated on every version change, potentially impacting the memoization benefit. The current solution is a reasonable trade-off for performance.
Based on learnings
1 participant
Summary by CodeRabbit
Bug Fixes
Performance
New Features