fix: selecting migrated notes #341
Conversation
* fix: selecting migrated notes * perf: reduce re-rendering of notes when activating one * chore: apply tiny fixes * fix: name of handler
✅ Deploy Preview for graypaper-reader ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThis PR refactors context management by introducing new context hooks ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
Visual Regression Test Report ✅ PassedGithub run id: 19409449434 🔗 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: 0
🧹 Nitpick comments (5)
src/components/MetadataProvider/MetadataProvider.tsx (1)
35-43: useMetadataContext hook looks good; consider adopting it at call sites for consistencyThe guard + error message make context usage safer. Since this file now exposes a typed hook, it would be cleaner to use
useMetadataContext()instead ofuseContext(MetadataContext) as IMetadataContextin consumers likeAppandLocationProvider, so all access goes through one well-typed, guarded path.src/components/LocationProvider/hooks/useGetLocationParamsToHash.ts (1)
1-18: Hashing hook correctly centralizes logic; only minor polish possibleThe hook cleanly binds
locationParamsToHashto currentmetadataand memoizes the result; behavior looks correct and side‑effect free.Two optional refinements you might consider:
- Rename
metaDataContext→metadataContextfor consistency with theIMetadataContextname.- Since you only return
getHashFromLocationParams, you could either:
return getHashFromLocationParams;, or- keep the object return but memoize it with
useMemoto avoid a new object each render if consumers ever depend on the wrapper object identity.Both are non‑blocking style tweaks.
src/App.tsx (1)
9-22: Switch to useVersionContext is appropriate; consider also using useMetadataContext hereReading
{ version }fromuseVersionContext()aligns App with the newVersionProviderand decouples it from the full location state, which is a nice simplification.Given you now have
useMetadataContext, it would be consistent (and avoid the manual cast) to replace:const { urlGetters } = useContext(MetadataContext) as IMetadataContext;with:
const { urlGetters } = useMetadataContext();This would also eliminate the need for the raw
useContextimport in this file.src/components/NoteManager/components/NotesList.tsx (1)
1-7: Memoization + onSelectNote wiring look correct; only minor ergonomics to considerWrapping
NoteinMemoizedNoteand threadingonSelectNotethrough ensures selection is handled externally without regressing performance; theactiveandnoteprops still participate correctly in the memo comparison.If this component’s API is used in multiple places, you might consider extracting the inline props object into a named
NotesListPropstype/interface so the newonSelectNoteprop is easier to reuse and evolve.Also applies to: 13-20, 28-35
src/components/LocationProvider/LocationProvider.tsx (1)
5-5: Delegating getHashFromLocationParams to the hook is fine; consider unifying metadata accessUsing
useGetLocationParamsToHash()to providegetHashFromLocationParamskeeps hashing logic centralized and reusable, whilehandleSetLocationParamsstill callslocationParamsToHashdirectly withmetadata, so behavior remains consistent.The only downside is that this component now reads
MetadataContexttwice (lines 38/40 and indirectly insideuseGetLocationParamsToHash). To reduce duplication and tighten the abstraction, you could:
- Replace the direct
useContext(MetadataContext) as IMetadataContextusages withuseMetadataContext(); and/or- Expose a variant of
useGetLocationParamsToHashthat acceptsmetadataas an argument so this provider can share the already-read metadata instead of re-reading the context.These are cleanup suggestions; current logic is sound.
Also applies to: 37-41, 52-59, 61-75, 164-175
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
shared/links-metadata/src/migrate.ts(1 hunks)src/App.tsx(2 hunks)src/components/CodeSyncProvider/CodeSyncProvider.tsx(2 hunks)src/components/LocationProvider/LocationProvider.tsx(2 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/hooks/useGetLocationParamsToHash.tssrc/components/MetadataProvider/MetadataProvider.tsxsrc/main.tsxsrc/App.tsxsrc/components/NoteManager/components/NotesList.tsxshared/links-metadata/src/migrate.tssrc/components/NoteManager/components/Note.tsxsrc/components/LocationProvider/VersionProvider.tsxsrc/components/NoteManager/components/NoteLink.tsxsrc/components/NoteManager/components/NoteLayout.tsxsrc/components/CodeSyncProvider/CodeSyncProvider.tsxsrc/components/NoteManager/NoteManager.tsxsrc/components/LocationProvider/LocationProvider.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/MetadataProvider/MetadataProvider.tsxsrc/main.tsxsrc/App.tsxsrc/components/NoteManager/components/NotesList.tsxsrc/components/NoteManager/components/Note.tsxsrc/components/LocationProvider/VersionProvider.tsxsrc/components/NoteManager/components/NoteLink.tsxsrc/components/NoteManager/components/NoteLayout.tsxsrc/components/CodeSyncProvider/CodeSyncProvider.tsxsrc/components/NoteManager/NoteManager.tsxsrc/components/LocationProvider/LocationProvider.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/App.tsxsrc/components/NoteManager/components/NotesList.tsxshared/links-metadata/src/migrate.tssrc/components/NoteManager/components/Note.tsxsrc/components/NoteManager/components/NoteLink.tsxsrc/components/NoteManager/components/NoteLayout.tsxsrc/components/CodeSyncProvider/CodeSyncProvider.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 (11)
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/main.tsx (4)
src/components/MetadataProvider/MetadataProvider.tsx (1)
MetadataProvider(45-83)src/components/LocationProvider/LocationProvider.tsx (1)
LocationProvider(37-182)src/components/LocationProvider/VersionProvider.tsx (1)
VersionProvider(16-27)src/App.tsx (1)
App(19-55)
src/App.tsx (1)
src/components/LocationProvider/VersionProvider.tsx (1)
useVersionContext(6-14)
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/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/components/LocationProvider/VersionProvider.tsx (1)
src/components/LocationProvider/LocationProvider.tsx (1)
useLocationContext(29-35)
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/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/CodeSyncProvider/CodeSyncProvider.tsx (2)
shared/links-metadata/src/migrate.ts (1)
migrateSelection(107-160)shared/links-metadata/index.ts (1)
migrateSelection(3-3)
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)
⏰ 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 (23)
shared/links-metadata/src/migrate.ts (1)
94-94: LGTM! Good defensive programming.The addition of optional chaining and nullish coalescing prevents potential runtime errors when
sourceLines[sourceBlock.line - 1]doesn't exist, ensuring the migration logic gracefully falls back to single-line matching instead of throwing an error.src/components/CodeSyncProvider/CodeSyncProvider.tsx (2)
3-3: LGTM!The
useMemoimport is correctly added to support the context memoization below.
199-216: Excellent optimization for context stability.Memoizing the context object prevents unnecessary re-renders of consumers by ensuring the context value reference remains stable when the underlying functions haven't changed. Since all six functions are already memoized with
useCallback, thisuseMemoprovides the final layer of optimization by stabilizing the object itself.src/main.tsx (1)
13-13: LGTM! Provider hierarchy is correct.The VersionProvider is correctly positioned inside LocationProvider (which it depends on via useLocationContext) and outside App (which consumes it via useVersionContext). The import path follows the project's conventions.
Also applies to: 21-23
src/components/NoteManager/NoteManager.tsx (4)
1-1: LGTM! Imports are necessary for the new functionality.Both useRef and IDecoratedNote are properly used in the handleSelectNote callback implementation.
Also applies to: 9-9
31-31: LGTM! Context destructuring aligns with interface.The addition of setLocationParams destructuring matches the ILocationContext interface and is required for the new handleSelectNote callback.
71-80: LGTM! Ref-based callback pattern prevents unnecessary re-renders.The locationRef approach correctly avoids including locationParams/setLocationParams in the useCallback dependencies, preventing the callback from being recreated on every location change. The implementation preserves the current version when navigating to a note's selection, which aligns with the migrated note behavior (note.current contains the selection in the current version).
112-112: LGTM! Callback wiring is complete.The onSelectNote prop correctly wires up the note selection flow from NotesList to individual Note components.
src/components/NoteManager/components/Note.tsx (4)
21-21: LGTM! Prop signature is correctly defined.The onSelectNote callback prop is properly typed and integrated into the component signature, completing the migration from direct LocationContext manipulation to callback-based navigation.
Also applies to: 24-24
72-85: LGTM! Click handler correctly delegates to onSelectNote.The handleWholeNoteClick logic properly guards against clicks on interactive elements and only calls onSelectNote when the note is not already active.
87-102: LGTM! Keyboard handler maintains accessibility.The keyboard activation handler correctly implements Enter/Space key support with proper guards against form elements and already-active notes.
154-154: LGTM! Explicit active prop improves clarity.While the default value for active is false, explicitly passing it here makes the intent clear in the conditional rendering logic.
src/components/NoteManager/components/NoteLayout.tsx (1)
23-23: LGTM! Context usage aligned with refactored NoteLink API.The removal of onEditNote from the destructuring is correct since NoteLink now obtains it internally via useNoteContext. The active={true} prop correctly indicates this is always displaying in active/selected state.
Also applies to: 28-28
src/components/LocationProvider/VersionProvider.tsx (3)
1-4: LGTM! Context creation follows React best practices.The context type correctly uses undefined for the uninitialized state, and imports are minimal and appropriate.
6-14: LGTM! Hook implementation follows established patterns.The runtime guard ensures the hook is only used within a VersionProvider, and the error message follows the same convention as useLocationContext.
16-27: LGTM! Provider implementation is optimized and correct.The VersionProvider correctly depends on LocationProvider (via useLocationContext), properly memoizes the context value on version changes only, and follows React context provider best practices.
src/components/NoteManager/components/NoteLink.tsx (7)
6-7: LGTM! API surface simplified with new context hooks.The component now obtains version and hash generation via dedicated hooks, and the active prop replacement for onEditNote improves the component's API clarity. The default value active = false is appropriate.
Also applies to: 11-11, 15-15, 18-18
28-29: LGTM! Hook usage follows React rules.Both hooks are correctly called at the top level and provide necessary data for link generation.
34-36: LGTM! Correctly uses migrated note data.Using note.current.selectionStart.pageNumber is appropriate since it reflects the migrated position in the current version.
38-48: LGTM! Effect correctly responds to note changes.The useEffect properly uses note.current data and depends on the entire note object, ensuring it updates when any aspect of the note changes.
57-65: LGTM! Link generation uses current version context.The currentVersionLink correctly combines the current version from context with the note's migrated selection coordinates, and the useMemo dependencies are appropriate.
101-101: LGTM! Conditional rendering logic is sound.The UpdateVersionLink is appropriately shown only for migrated, editable notes that are currently active, allowing users to update the note to the current version.
107-149: LGTM! UpdateVersionLink implements migration logic correctly.The component properly:
- Gathers all necessary context (location, note, version, selection)
- Guards against missing selection data
- Uses isSameBlock utility to detect selection changes
- Prompts the user before updating if the selection differs
- Updates the note with current selection coordinates and version
- Provides appropriate visual feedback via disabled state
The implementation handles the complex migration flow with good error prevention and user experience.
1 participant
No description provided.