Conversation
✅ Deploy Preview for graypaper-reader ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughReplaces inline add/edit UI with a NewNote-driven component and inactive skeletons; introduces NoteContainer and useLatestCallback; stabilizes add/update/delete via ref-backed callbacks and keepShowingNewNote; tightens async effects with cancellation guards; relaxes areSelectionsEqual to accept optional selections. Changes
Sequence Diagram(s)(omitted — changes are UI/component additions and defensive async guards; no new multi-component sequential control flow requiring a diagram) Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{js,jsx,ts,tsx}⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (3)📚 Learning: 2025-10-22T20:36:10.440ZApplied to files:
📚 Learning: 2025-10-22T20:36:10.440ZApplied to files:
📚 Learning: 2025-10-28T20:41:06.887ZApplied to files:
🧬 Code graph analysis (1)src/components/NoteManager/components/NoteLayout.tsx (3)
⏰ 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)
🔇 Additional comments (2)
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: 20519577623 🔗 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 (6)
src/hooks/useLatestCallback.ts (1)
4-7: Avoid mutating refs during render phase.Assigning to
ref.currentduring render (line 6) can cause issues with React's concurrent rendering features. React 19 may invoke the render function multiple times before committing, leading to unexpected behavior or stale closures.Consider using
useInsertionEffect(oruseLayoutEffect) to safely update the ref after render commits:🔎 Proposed fix
-import { type RefObject, useRef } from "react"; +import { type RefObject, useInsertionEffect, useRef } from "react"; // biome-ignore lint/suspicious/noExplicitAny: <explanation> export function useLatestCallback<T extends (...args: any[]) => any>(callback: T): RefObject<T> { const ref = useRef(callback); - ref.current = callback; + useInsertionEffect(() => { + ref.current = callback; + }); return ref; }src/components/NoteManager/components/InactiveNoteSkeleton.tsx (1)
6-14: Consider removing duplicate Tailwind classes.Several classes in line 11 (
note,rounded-xl,p-4,bg-[var(--inactive-note-bg)]) are already applied byNoteContainer(see lines 14-16 ofNoteContainer.tsx). Whilecn()handles duplicates gracefully, removing them improves readability.🔎 Proposed simplification
<NoteContainer active={false} aria-hidden="true" className={cn( - "note relative rounded-xl p-4 bg-[var(--inactive-note-bg)] border border-[var(--border)]/60 select-none animate-pulse", + "border border-[var(--border)]/60 select-none animate-pulse", className, )} >src/components/NoteManager/components/NewNote.tsx (1)
26-41: Simplify callback handling.Since
useLatestCallbackreturns a stable ref, wrapping calls inuseCallbackwith the ref as a dependency provides no additional stability benefit. The callbacks could invoke the props directly:🔎 Proposed simplification
- const latestOnCancel = useLatestCallback(onCancel); - const latestOnSave = useLatestCallback(onSave); - - const handleCancelClick = useCallback(() => { - latestOnCancel.current(); - }, [latestOnCancel]); - - const handleSaveClick = useCallback(() => { + const handleCancelClick = useCallback(() => { + onCancel(); + }, [onCancel]); + + const handleSaveClick = useCallback(() => { const mathValidationError = validateMath(noteContent); if (mathValidationError) { setNoteContentError(mathValidationError); return; } - latestOnSave.current({ noteContent }); - }, [noteContent, latestOnSave]); + onSave({ noteContent }); + }, [noteContent, onSave]);Alternatively, if the intent is to avoid re-renders when parent callbacks change, the current pattern works but the
useLatestCallbackhook should be fixed per the earlier comment.src/components/NoteManager/NoteManager.tsx (2)
34-34: Consider resettingkeepShowingNewNoteafter notes become ready.The ref is set on save but never cleared. While the current guards prevent issues, resetting it (e.g., when
notesReadytransitions totrueor when selection changes) would make the intent clearer and prevent potential stale state in future refactors.
119-119: Consider renaming for clarity.The variable
isActiveNotesis a boolean, but the name suggests a collection. ConsiderhasActiveNoteorhasActiveNotesfor better readability.🔎 Suggested rename
- const isActiveNotes = notes.some((note) => activeNotes.includes(note)); + const hasActiveNote = notes.some((note) => activeNotes.includes(note));Then update the usage on line 128:
- !isActiveNotes && + !hasActiveNote &&src/components/NotesProvider/utils/areSelectionsEqual.ts (1)
15-17: Document theundefinedequality semantics.When both points are
undefined, this returnstrue(sinceundefined === undefined). This is reasonable ("both empty means equal"), but could surprise future callers who might expectfalsefor missing data.Consider adding a brief JSDoc to clarify:
/** * Compares two selection points. Returns true if both are undefined * or if both have matching pageNumber and index values. */ export const areSelectionPointsEqual = (point1?: ISynctexBlockId, point2?: ISynctexBlockId): boolean => { return point1?.pageNumber === point2?.pageNumber && point1?.index === point2?.index; };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/components/NoteManager/NoteManager.tsx(4 hunks)src/components/NoteManager/components/InactiveNoteSkeleton.tsx(1 hunks)src/components/NoteManager/components/NewNote.tsx(1 hunks)src/components/NoteManager/components/Note.tsx(4 hunks)src/components/NoteManager/components/NotesList.tsx(0 hunks)src/components/NoteManager/components/SimpleComponents/NoteContainer.tsx(1 hunks)src/components/NotesProvider/NotesProvider.tsx(1 hunks)src/components/NotesProvider/hooks/useRemoteNotes.ts(1 hunks)src/components/NotesProvider/utils/areSelectionsEqual.ts(1 hunks)src/hooks/useLatestCallback.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/NoteManager/components/NotesList.tsx
🧰 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/NotesProvider/NotesProvider.tsxsrc/hooks/useLatestCallback.tssrc/components/NotesProvider/utils/areSelectionsEqual.tssrc/components/NoteManager/components/InactiveNoteSkeleton.tsxsrc/components/NoteManager/NoteManager.tsxsrc/components/NoteManager/components/SimpleComponents/NoteContainer.tsxsrc/components/NoteManager/components/NewNote.tsxsrc/components/NoteManager/components/Note.tsxsrc/components/NotesProvider/hooks/useRemoteNotes.ts
🧠 Learnings (2)
📚 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/hooks/useLatestCallback.tssrc/components/NoteManager/NoteManager.tsxsrc/components/NoteManager/components/SimpleComponents/NoteContainer.tsxsrc/components/NoteManager/components/NewNote.tsxsrc/components/NoteManager/components/Note.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/hooks/useLatestCallback.tssrc/components/NoteManager/NoteManager.tsxsrc/components/NoteManager/components/NewNote.tsxsrc/components/NoteManager/components/Note.tsx
🧬 Code graph analysis (6)
src/components/NotesProvider/utils/areSelectionsEqual.ts (1)
shared/links-metadata/src/types.ts (1)
ISynctexBlockId(1-4)
src/components/NoteManager/components/InactiveNoteSkeleton.tsx (1)
src/components/NoteManager/components/SimpleComponents/NoteContainer.tsx (1)
NoteContainer(10-24)
src/components/NoteManager/NoteManager.tsx (5)
shared/links-metadata/src/types.ts (1)
ISynctexBlockId(1-4)src/components/NotesProvider/types/StorageNote.ts (1)
IStorageNote(5-5)src/components/NotesProvider/consts/labels.ts (1)
LABEL_LOCAL(1-1)src/components/NotesProvider/utils/areSelectionsEqual.ts (1)
areSelectionsEqual(8-13)src/components/NoteManager/components/NewNote.tsx (1)
NewNote(20-135)
src/components/NoteManager/components/NewNote.tsx (6)
src/hooks/useLatestCallback.ts (1)
useLatestCallback(4-8)src/utils/validateMath.ts (1)
validateMath(3-14)src/components/NotesProvider/types/DecoratedNote.ts (1)
IDecoratedNote(4-19)src/components/NoteManager/components/NoteContext.tsx (1)
ISingleNoteContext(6-22)src/components/NoteManager/components/NoteLayout.tsx (1)
NoteLayout(66-73)src/components/NoteManager/components/SimpleComponents/NoteContainer.tsx (1)
NoteContainer(10-24)
src/components/NoteManager/components/Note.tsx (1)
src/components/NoteManager/components/SimpleComponents/NoteContainer.tsx (1)
NoteContainer(10-24)
src/components/NotesProvider/hooks/useRemoteNotes.ts (3)
src/components/NotesProvider/types/StorageNote.ts (1)
IStorageNote(5-5)src/components/NotesProvider/utils/notesImportExport.ts (1)
importNotesFromJson(49-80)src/components/NotesProvider/consts/labels.ts (1)
LABEL_REMOTE(2-2)
⏰ 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/NotesProvider/NotesProvider.tsx (1)
82-96: LGTM! Well-implemented cancellation pattern.The cancellation guard correctly prevents state updates after the component unmounts or when dependencies change, avoiding the "can't perform a React state update on an unmounted component" warning. This aligns well with the similar pattern in
useRemoteNotes.ts.src/components/NotesProvider/hooks/useRemoteNotes.ts (2)
29-58: LGTM! Proper cancellation handling for async operations.Good use of early-exit checks after
fetchanddata.text()to avoid unnecessary processing when the effect has been cancelled. The pattern correctly prevents state updates after unmount.
61-74: Consistent cancellation pattern applied.The decoration effect mirrors the cancellation pattern from the loading effect, ensuring state updates are skipped when the component unmounts during async decoration.
src/components/NoteManager/components/SimpleComponents/NoteContainer.tsx (1)
1-24: LGTM! Clean reusable container component.Good extraction of the note card styling into a reusable component. The use of CSS variables for theming and proper TypeScript typing with
DetailedHTMLPropsallows full HTML attribute forwarding. This reduces duplication acrossNote,NewNote, andInactiveNoteSkeleton.src/components/NoteManager/components/Note.tsx (1)
227-293: LGTM! Clean adoption of NoteContainer.The refactor properly delegates root element styling to
NoteContainerwhile preserving all existing interactive behavior (click, keyboard, hover, focus, dropdown). Therefforwarding works correctly via the spread props pattern.src/components/NoteManager/components/NewNote.tsx (1)
114-134: LGTM! Well-structured new note component.The component correctly integrates with the
NoteLayoutcontext system and reusesNoteContainerfor consistent styling. The validation flow and memoization strategy are appropriate.src/components/NoteManager/NoteManager.tsx (4)
1-14: LGTM!Imports are correctly updated to support the new component-driven architecture.
43-57: LGTM!The delete and cancel handlers correctly clear the selection, with proper dependency arrays.
59-88: LGTM!The save handler properly validates prerequisites, constructs the note with all required fields, and captures the selection for the UI transition. The error thrown on missing selection is appropriate for catching programming errors early.
121-159: LGTM!The render logic correctly handles all states:
- Loading with selection (NewNote + faded UI)
- Loading without selection (skeletons)
- Ready with no notes (empty message)
- Ready with notes (list)
The opacity/pointer-events approach effectively prevents interaction during loading while maintaining visual continuity.
src/components/NotesProvider/utils/areSelectionsEqual.ts (1)
3-13: LGTM!The relaxed typing with optional chaining enables broader usage patterns while maintaining correct comparison semantics for the NoteManager use case.
|
there is issue when adding labels to a while created a new - does not work yet. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/components/NoteManager/NoteManager.tsx (2)
41-47: Remove stable ref from dependency array.The
latestDeleteNoteref is stable across renders and doesn't need to be in the dependency array. Including it is unnecessary and could cause the callback to be recreated more often than needed.🔎 Proposed fix
const memoizedHandleDeleteNote = useCallback( (note: IDecoratedNote) => { latestDeleteNote.current(note); handleClearSelection(); }, - [handleClearSelection, latestDeleteNote], + [handleClearSelection], );
49-54: Remove stable ref from dependency array.The
latestUpdateNoteref is stable across renders and doesn't need to be in the dependency array.🔎 Proposed fix
const memoizedHandleUpdateNote = useCallback( (note: IDecoratedNote, newNote: IStorageNote) => { latestUpdateNote.current(note, newNote); }, - [latestUpdateNote], + [], );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/NoteManager/NoteManager.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/NoteManager/NoteManager.tsx
🧠 Learnings (2)
📚 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/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/NoteManager.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). (1)
- GitHub Check: visual-tests
🔇 Additional comments (5)
src/components/NoteManager/NoteManager.tsx (5)
1-15: LGTM! Clean import organization.The new imports support the redesigned note-adding flow and are all utilized in the component.
37-39: LGTM! Proper use of useLatestCallback pattern.The ref-backed callbacks provide stable references to the latest handlers, which is useful for avoiding stale closures.
56-58: LGTM! Clean cancellation handler.The callback correctly clears the selection when a new note is cancelled.
122-161: Well-structured conditional rendering flow.The rendering logic cleanly handles multiple states:
- Loading state with skeleton components
- Empty state with helpful message
- Active state with notes list
- New note creation when selection exists
The opacity and pointer-events styling on line 125 provides good visual feedback during loading.
127-138: No action required—areSelectionsEqualsafely handles undefined parameters.The function signature accepts optional parameters and uses optional chaining throughout, so passing
undefined(whenkeepShowingNewNote.currentis undefined) will not cause runtime errors. The comparison will correctly evaluate tofalse.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/NoteManager/NoteManager.tsx (3)
35-39: Consider adding logic to clear keepShowingNewNote.The
keepShowingNewNoteref is set when a note is saved (lines 83-86) but is never cleared. This serves as a UX optimization to prevent flickering during the async save operation, which is good.However, if the user later returns to the same selection after the note has been deleted or is no longer active, the condition on line 132
areSelectionsEqual(locationParams, keepShowingNewNote.current)could allowNewNoteto render even whennotesReadyis false, which might not be the intended behavior.If this is a concern, consider clearing
keepShowingNewNote.currentwhen appropriate (e.g., when a note becomes active, or when selection changes to a different range).
123-126: Consider using Tailwind classes instead of inline styles.The inline styles for
opacityandpointerEventscould be replaced with Tailwind 4.x utility classes for consistency and better maintainability.🔎 Proposed refactor using Tailwind classes
- <div - className="note-manager flex flex-col gap-2.5" - style={{ opacity: notesReady ? 1.0 : 0.3, pointerEvents: notesReady ? "auto" : "none" }} - > + <div + className={twMerge( + "note-manager flex flex-col gap-2.5", + notesReady ? "opacity-100 pointer-events-auto" : "opacity-30 pointer-events-none" + )} + >As per coding guidelines, prefer Tailwind 4.x conventions.
127-140: Consider extracting the complex condition to improve readability.The condition for rendering
NewNotespans multiple lines and includes several checks. Extracting this to a well-named variable would improve code clarity and make the intent more explicit.🔎 Proposed refactor for better readability
+ const shouldShowNewNote = + locationParams.selectionEnd && + locationParams.selectionStart && + pageNumber !== null && + selectedBlocks.length > 0 && + !isActiveNotes && + (notesReady || areSelectionsEqual(locationParams, keepShowingNewNote.current)); + return ( <div className={twMerge( "note-manager flex flex-col gap-2.5", notesReady ? "opacity-100 pointer-events-auto" : "opacity-30 pointer-events-none" )} > - {locationParams.selectionEnd && - locationParams.selectionStart && - pageNumber !== null && - selectedBlocks.length > 0 && - !isActiveNotes && - (notesReady || areSelectionsEqual(locationParams, keepShowingNewNote.current)) && ( + {shouldShowNewNote && ( <NewNote
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/NoteManager/NoteManager.tsx
🧰 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/NoteManager.tsx
🧠 Learnings (3)
📚 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/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: 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/NoteManager/NoteManager.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/NoteManager/NoteManager.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). (1)
- GitHub Check: visual-tests
🔇 Additional comments (5)
src/components/NoteManager/NoteManager.tsx (5)
1-27: LGTM!The imports and component setup are clean. The use of
memofor NotesList is appropriate for performance optimization.Based on learnings, this repo targets React 19+.
41-58: LGTM!The memoized handlers correctly use the latest callback references and have appropriate dependencies. Clearing the selection after delete (line 44) provides good UX.
60-89: Add type annotation for the function parameter.The
noteContentparameter should have a type annotation for better type safety and developer experience.🔎 Proposed type annotation
- const handleAddNoteClick = useCallback( - ({ noteContent }: { noteContent: string }) => { + const handleAddNoteClick = useCallback( + ({ noteContent }: { noteContent: string }) => {Wait, actually looking at line 61, the type annotation
{ noteContent: string }is already present. Let me recheck...Actually, the annotation IS there on line 61:
({ noteContent }: { noteContent: string }). So this is fine.Actually, reviewing line 61 again, the type annotation is present:
{ noteContent: string }. This is correct.
91-118: LGTM!The
locationRefpattern correctly provides access to the latest location context without causing re-renders, andmemoizedHandleSelectNoteproperly handles the three selection modes.
142-161: LGTM!The conditional rendering logic for skeletons, empty state, and notes list is well-structured and handles all the necessary states appropriately.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/NoteManager/components/NewNote.tsx (1)
20-50: Solid state management and validation logic.The use of
dateReffor a deterministic timestamp anduseLatestCallbackfor stable parent callbacks is excellent. Math validation before save provides good user feedback.Optional: Move link constants outside component
Lines 44-45 define empty string constants inside the component body. While this works fine (strings are primitives), moving them outside would make the intent clearer:
+const EMPTY_VERSION_LINK = ""; + export const NewNote = ({ selectionEnd, selectionStart, version, onCancel, onSave }: NewNoteProps) => { const dateRef = useRef(new Date().getTime()); // ... state declarations ... - const currentVersionLink = ""; - const originalVersionLink = ""; + const currentVersionLink = EMPTY_VERSION_LINK; + const originalVersionLink = EMPTY_VERSION_LINK;src/components/NoteManager/NoteManager.tsx (1)
119-161: Thoughtful render logic with excellent state handling.The conditional rendering correctly handles all states:
- Shows
NewNotewhen there's a valid selection and no active notes (lines 126-139)- Uses
areSelectionsEqualcleverly to keepNewNotevisible after save untilnotesReady(line 131)- Displays loading skeletons while notes are being fetched (lines 141-148)
- Shows a clear empty state message when appropriate (line 150)
- Renders the notes list with memoized callbacks when notes exist (lines 152-160)
Optional: Replace inline styles with CSS classes
Lines 122-125 use inline
styleprops foropacityandpointer-events. For better separation of concerns and maintainability, consider adding CSS classes:<div className="note-manager flex flex-col gap-2.5" - style={{ opacity: notesReady ? 1.0 : 0.3, pointerEvents: notesReady ? "auto" : "none" }} + className={cn( + "note-manager flex flex-col gap-2.5", + !notesReady && "opacity-30 pointer-events-none" + )} >(Requires importing
cnutility if not already available)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/NoteManager/NoteManager.tsxsrc/components/NoteManager/components/NewNote.tsx
🧰 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/NewNote.tsxsrc/components/NoteManager/NoteManager.tsx
🧠 Learnings (3)
📚 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/NoteManager/components/NewNote.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/NewNote.tsxsrc/components/NoteManager/NoteManager.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/NoteManager/NoteManager.tsx
🧬 Code graph analysis (2)
src/components/NoteManager/components/NewNote.tsx (6)
shared/links-metadata/src/types.ts (1)
ISynctexBlockId(1-4)src/hooks/useLatestCallback.ts (1)
useLatestCallback(4-8)src/utils/validateMath.ts (1)
validateMath(3-14)src/components/NoteManager/components/NoteContext.tsx (1)
ISingleNoteContext(6-22)src/components/NoteManager/components/NoteLayout.tsx (1)
NoteLayout(66-73)src/components/NoteManager/components/SimpleComponents/NoteContainer.tsx (1)
NoteContainer(10-24)
src/components/NoteManager/NoteManager.tsx (6)
shared/links-metadata/src/types.ts (1)
ISynctexBlockId(1-4)src/hooks/useLatestCallback.ts (1)
useLatestCallback(4-8)src/components/NotesProvider/types/StorageNote.ts (1)
IStorageNote(5-5)src/components/NotesProvider/utils/areSelectionsEqual.ts (1)
areSelectionsEqual(8-13)src/components/NoteManager/components/NewNote.tsx (1)
NewNote(20-136)src/components/NoteManager/components/InactiveNoteSkeleton.tsx (1)
InactiveNoteSkeleton(6-26)
⏰ 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 (7)
src/components/NoteManager/components/NewNote.tsx (3)
1-18: LGTM! Clean imports and type definition.The imports are well-organized and the
NewNotePropstype clearly defines the component's interface.
52-113: Well-structured context and memo usage.The memoized objects correctly model the new note state:
noteDirtycaptures the current editing statenoteprovides the decorated note structure with empty original content (since this is unsaved)noteLayoutContextproperly configures UI state withactive: true,isEditing: true, and appropriate callbacksThe no-op handlers (
handleEditClick,onEditNote,handleSelectNote) are appropriate for a new, unsaved note context.
115-136: Clean render logic with proper error handling.The UI correctly displays validation errors conditionally and provides clear Cancel/Save actions with test IDs for testability.
src/components/NoteManager/NoteManager.tsx (4)
1-13: LGTM! Clean integration of new dependencies.The new imports for
useLatestCallback,areSelectionsEqual,InactiveNoteSkeleton, andNewNoteare well-organized and necessary for the refactored functionality.
34-38: Excellent use of refs and latest callbacks pattern.The
keepShowingNewNoteref cleverly preserves the selection for a newly created note, allowing the UI to continue showingNewNoteuntilnotesReadycompletes. Wrapping handlers withuseLatestCallbackprevents stale closures while maintaining stable callback identity—a solid pattern for avoiding unnecessary re-renders.
40-57: Well-designed handler orchestration.The handlers appropriately compose the latest callbacks with UI state updates:
memoizedHandleDeleteNoteclears the selection after deletion (good UX)memoizedHandleUpdateNoteforwards to the latest callbackhandleNewNoteCancelclears the selection when canceling new note creationDependency arrays are correct for all three.
59-88: Robust note creation logic with proper validation.The refactored
handleAddNoteClickcorrectly:
- Validates all required selection data before proceeding (lines 61-68)
- Constructs a complete
IStorageNotewith all metadata (lines 70-79)- Updates
keepShowingNewNoteto maintain UI state continuity (lines 82-85)The validation guards will throw an error if preconditions aren't met, which is appropriate since the render logic ensures these conditions before showing
NewNote.Based on learnings from past review: including
latestHandleAddNotein the dependency array satisfies linter requirements.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/NoteManager/NoteManager.tsx (1)
128-131: Consider using Tailwind classes for conditional opacity and pointer-events.The inline styles work correctly, but could be replaced with Tailwind classes for consistency with the rest of the styling approach.
🔎 Proposed refactor using Tailwind classes
<div - className="note-manager flex flex-col gap-2.5" - style={{ opacity: notesReady ? 1.0 : 0.3, pointerEvents: notesReady ? "auto" : "none" }} + className={twMerge( + "note-manager flex flex-col gap-2.5", + notesReady ? "opacity-100 pointer-events-auto" : "opacity-30 pointer-events-none" + )} >
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
tools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/notes-tab-after-note-activation-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/notes-tab-after-note-activation-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/notes-tab-after-note-edit-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/notes-tab-after-note-edit-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/notes-tab-after-opening-dropdown-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/notes-tab-after-opening-dropdown-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/notes-tab-initial-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/notes-tab-initial-light-mode-linux.pngis excluded by!**/*.png
📒 Files selected for processing (1)
src/components/NoteManager/NoteManager.tsx
🧰 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/NoteManager.tsx
🧠 Learnings (3)
📚 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/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: 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/NoteManager/NoteManager.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/NoteManager/NoteManager.tsx
🧬 Code graph analysis (1)
src/components/NoteManager/NoteManager.tsx (5)
src/hooks/useLatestCallback.ts (1)
useLatestCallback(4-8)src/components/NotesProvider/types/StorageNote.ts (1)
IStorageNote(5-5)src/components/NotesProvider/utils/areSelectionsEqual.ts (1)
areSelectionsEqual(8-13)src/components/NoteManager/components/NewNote.tsx (1)
NewNote(20-136)src/components/NoteManager/components/InactiveNoteSkeleton.tsx (1)
InactiveNoteSkeleton(6-26)
⏰ 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 (5)
src/components/NoteManager/NoteManager.tsx (5)
1-14: LGTM!Imports are clean and all appear to be utilized. The addition of
useLatestCallback,areSelectionsEqual, and the new component imports (NewNote,InactiveNoteSkeleton) align well with the redesigned note-adding flow.
36-53: Good use ofuseLatestCallbackfor stable callback references.The pattern correctly wraps context handlers to provide stable references while always executing the latest implementation. The
memoizedHandleDeleteNoteappropriately clears the selection after deletion, ensuring consistent UI state.
55-88: LGTM!The note creation flow correctly validates required state before saving and stores the selection range in
keepShowingNewNoteto maintain UI continuity while notes refresh. The guard conditions align with the render conditions forNewNote.
147-166: LGTM!The render logic correctly handles all states: loading skeletons when not ready, empty state message when ready with no notes, and the memoized notes list when notes exist. The use of
MemoizedNotesListwith stable callback references ensures efficient re-renders.
119-119: The code correctly handles reference equality. Bothnotes(which isfilteredNotes) andactiveNotesare derived from the same source through filtering operations that preserve object references. SinceactiveNotesis filtered fromfilteredNotes, all elements inactiveNotesmaintain the same object references as those innotes. Therefore,Array.includes()will correctly identify matching notes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/NoteManager/components/NewNote.tsx (1)
32-40: Consider validating for empty note content.The validation currently only checks for math syntax errors. If empty notes should not be allowed, consider adding a check before validation.
🔎 Optional enhancement to prevent empty notes
const handleSaveClick = useCallback(() => { + if (noteContent.trim() === "") { + setNoteContentError("Note content cannot be empty"); + return; + } + const mathValidationError = validateMath(noteContent); if (mathValidationError) { setNoteContentError(mathValidationError); return; } latestOnSave.current({ noteContent, labels }); }, [noteContent, latestOnSave, labels]);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/NoteManager/NoteManager.tsxsrc/components/NoteManager/components/NewNote.tsx
🧰 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/NoteManager.tsxsrc/components/NoteManager/components/NewNote.tsx
🧠 Learnings (3)
📚 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/NoteManager.tsxsrc/components/NoteManager/components/NewNote.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/NoteManager/NoteManager.tsxsrc/components/NoteManager/components/NewNote.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/NoteManager/NoteManager.tsx
🧬 Code graph analysis (2)
src/components/NoteManager/NoteManager.tsx (6)
shared/links-metadata/src/types.ts (1)
ISynctexBlockId(1-4)src/hooks/useLatestCallback.ts (1)
useLatestCallback(4-8)src/components/NotesProvider/types/StorageNote.ts (1)
IStorageNote(5-5)src/components/NotesProvider/utils/areSelectionsEqual.ts (1)
areSelectionsEqual(8-13)src/components/NoteManager/components/NewNote.tsx (1)
NewNote(20-88)src/components/NoteManager/components/InactiveNoteSkeleton.tsx (1)
InactiveNoteSkeleton(6-26)
src/components/NoteManager/components/NewNote.tsx (7)
shared/links-metadata/src/types.ts (1)
ISynctexBlockId(1-4)src/hooks/useLatestCallback.ts (1)
useLatestCallback(4-8)src/utils/validateMath.ts (1)
validateMath(3-14)src/components/NoteManager/components/NoteLayout.tsx (1)
NoteLayout(66-73)src/components/NoteManager/components/SimpleComponents/NoteContainer.tsx (1)
NoteContainer(10-24)src/components/NotesProvider/types/DecoratedNote.ts (1)
IDecoratedNote(4-19)src/components/NoteManager/components/NoteContext.tsx (1)
ISingleNoteContext(6-22)
⏰ 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 (12)
src/components/NoteManager/components/NewNote.tsx (6)
1-18: LGTM! Clean imports and type definitions.The component structure is well-organized with appropriate type safety for the note creation flow.
20-30: LGTM! State and callback initialization is correct.The use of
useLatestCallbackensures stable callback references while always invoking the latest version, and the default "local" label is a sensible starting point for new notes.
42-48: LGTM! Empty version links are appropriate for new notes.New notes don't have version history yet, so empty strings are correct. The error clearing on content change provides good user experience.
50-88: LGTM! Well-structured component render and layout context.The helper hooks properly memoize derived state, and the JSX composition using
NoteLayoutcomponents provides a clean, consistent UI. The conditional error message display enhances user feedback.
90-119: LGTM! Correct structure for new note representation.The memoized note object correctly represents an empty, local note with appropriate default values for a new note that hasn't been saved yet.
121-198: LGTM! Correct dirty state tracking and layout context.The dirty note object properly tracks the current editing state, and the layout context correctly provides all required handlers and metadata. The empty/no-op functions for unused handlers are appropriate for the new note creation flow.
src/components/NoteManager/NoteManager.tsx (6)
1-15: LGTM! Clean import additions for the redesigned note flow.The new imports properly support the refactored architecture with
NewNotecomponent, skeleton UI, and callback stabilization.
35-39: LGTM! Proper ref initialization and callback stabilization.The
keepShowingNewNoteref correctly tracks the selection for post-save UI continuity, and wrapping the note handlers withuseLatestCallbackensures stable references while always invoking the latest versions.
41-58: LGTM! Well-designed handler memoization with appropriate side effects.The delete handler correctly clears the selection to prevent showing a deleted note's selection. The cancel handler also appropriately clears the selection when abandoning note creation.
60-89: LGTM! Robust save handler with proper validation and UX continuity.The callback correctly validates all required selection state before saving, constructs the note with proper metadata, and uses
keepShowingNewNoteto maintain UI consistency during the async save/reload cycle. The dependency array is correct per your linter requirements.
120-126: LGTM! Correct state tracking and cleanup logic.The
isActiveNotesflag properly prevents concurrent editing, and the effect correctly resets thekeepShowingNewNoteref once notes have loaded, completing the save cycle.
129-165: LGTM! Well-designed conditional rendering with excellent UX.The render logic elegantly handles multiple states:
- Loading state with skeleton placeholders
- Empty state with clear messaging
- New note creation with selection validation
- Existing notes list
The
areSelectionsEqual(locationParams, keepShowingNewNote.current)condition is particularly clever—it keeps theNewNoteform visible during the reload cycle after saving, providing seamless UX continuity.
1 participant
What?
Adding new note has been redesigned:
Extras:
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.