Conversation
✅ Deploy Preview for graypaper-reader ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Rate limit exceeded@chmurson has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 59 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 19518352473 🔗 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: 1
🧹 Nitpick comments (5)
tools/snapshot-tests/tests/basic-snapshots.spec.ts (1)
78-86: Dropdown screenshot test may be slightly brittleThe new "notes tab - after opening dropdown" test clicks the dropdown trigger and immediately takes a screenshot. If the dropdown content animates in or renders asynchronously, you may capture a partially-open state. Consider also waiting for a deterministic dropdown content selector (e.g., the menu container) to be visible before taking the screenshot to reduce flakiness.
src/components/NoteManager/NoteManager.tsx (1)
87-114: Consider preserving otherlocationParamsfields when updating selection
memoizedHandleSelectNoterebuildsselectionStart,selectionEnd, andversionand then callssetLocationParamswith a new object containing only those three fields. IflocationParamsever grows additional properties (e.g., page-level metadata), this will drop them on each selection change. To future-proof this, consider spreading the existing params before overriding the selection-specific fields, for example:- locationRef.current.setLocationParams({ - selectionStart, - selectionEnd, - version: version, - }); + locationRef.current.setLocationParams({ + ...locationRef.current.locationParams, + selectionStart, + selectionEnd, + version, + });This keeps the current behavior while avoiding accidental loss of extra state if
locationParamsevolves.src/components/NoteManager/components/NoteDropdown.tsx (1)
20-47: Guard against undefined context links and clipboard failures
currentVersionLinkandnoteOriginalVersionShortcome from context where the types allowundefined. If either is missing,copyLinkwill generate a/#undefinedURL and the “Open in v…“ label can render asvundefined. It’s probably impossible in normal flows, but defensively checking (or narrowing the types inISingleNoteContext) before using them would make this component more robust.
navigator.clipboard.writeTextalso returns a Promise and can reject (permissions, non-secure context). If that happens today, it fails silently while still showing “Link copied!”. Consider adding a.catch(even if it just logs) or handling the rejection to keep behavior consistent with the UI feedback.Also applies to: 81-103
src/components/NoteManager/components/NoteLink.tsx (1)
24-25: Tighten handling of context links and clean up unused propTwo minor points:
currentVersionLink/originalVersionLinkare typed as possiblyundefined, but are interpolated intohref. If either is missing you’ll end up with#undefined. Either narrow the types at the context level for notes that renderNoteLink, or add a small guard/fallback here before rendering the anchors.NoteLinkPropsstill declaresactive?: boolean, but the component no longer uses it. Dropping that prop (and theactive={false}at the call-site) will avoid confusion about behavior.Also applies to: 59-69, 72-77
src/components/NoteManager/components/Note.tsx (1)
34-39: Version-link computation & context wiring are sound; consider small robustness tweaksThe way you derive:
noteOriginalVersionShortfrommetadata.versions[note.original.version]?.name,currentVersionLink/originalLinkviagetHashFromLocationParams, and- pass everything through
noteLayoutContextwithsatisfies ISingleNoteContextis coherent and keeps all link/version data centralized in the context. Two minor refinements to consider:
noteOriginalVersionShortcan beundefined; any consumers that renderv{noteOriginalVersionShort}(e.g., NoteDropdown/NoteLink) will showvundefinedif metadata is missing. Either fall back tonote.original.versionhere or have downstream components guard againstundefined.- The
useMemodeps currently usenote.currentandnote. Ifnoteor its nested selection fields are ever mutated in place, the memos won’t see the change. Depending on howIDecoratedNoteis produced, you may want to depend on the specific selection fields instead (e.g.,note.current.selectionStart,note.current.selectionEnd,note.original.selectionStart,note.original.selectionEnd) to make the memoization more robust.Also applies to: 128-146, 148-184
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (24)
tools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/homepage-outline-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/homepage-outline-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/search-initial-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-initial-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-with-query-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-with-query-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-with-results-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-with-results-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-github-expanded-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-github-expanded-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-github-option-hover-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-github-option-hover-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-more-clicked-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-more-clicked-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-dropdown-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-dropdown-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-selected-off-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-selected-off-light-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-selected-on-dark-mode-linux.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-notes-selected-on-light-mode-linux.pngis excluded by!**/*.png
📒 Files selected for processing (8)
src/components/NoteManager/NoteManager.tsx(4 hunks)src/components/NoteManager/components/Note.tsx(7 hunks)src/components/NoteManager/components/NoteContext.tsx(2 hunks)src/components/NoteManager/components/NoteDropdown.tsx(1 hunks)src/components/NoteManager/components/NoteLayout.tsx(2 hunks)src/components/NoteManager/components/NoteLink.tsx(4 hunks)src/components/NoteManager/components/NotesList.tsx(1 hunks)tools/snapshot-tests/tests/basic-snapshots.spec.ts(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:
tools/snapshot-tests/tests/basic-snapshots.spec.tssrc/components/NoteManager/components/NoteLayout.tsxsrc/components/NoteManager/components/NotesList.tsxsrc/components/NoteManager/components/Note.tsxsrc/components/NoteManager/NoteManager.tsxsrc/components/NoteManager/components/NoteLink.tsxsrc/components/NoteManager/components/NoteDropdown.tsxsrc/components/NoteManager/components/NoteContext.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/components/NotesList.tsxsrc/components/NoteManager/components/Note.tsxsrc/components/NoteManager/NoteManager.tsxsrc/components/NoteManager/components/NoteLink.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/components/NotesList.tsxsrc/components/NoteManager/components/Note.tsxsrc/components/NoteManager/NoteManager.tsxsrc/components/NoteManager/components/NoteLink.tsxsrc/components/NoteManager/components/NoteDropdown.tsxsrc/components/NoteManager/components/NoteContext.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 (7)
src/components/NoteManager/components/NoteLayout.tsx (1)
src/components/NoteManager/components/NoteDropdown.tsx (1)
NoteDropdown(20-105)
src/components/NoteManager/components/NotesList.tsx (1)
src/components/NotesProvider/types/DecoratedNote.ts (1)
IDecoratedNote(4-19)
src/components/NoteManager/components/Note.tsx (7)
src/components/NotesProvider/types/DecoratedNote.ts (1)
IDecoratedNote(4-19)src/components/MetadataProvider/MetadataProvider.tsx (1)
useMetadataContext(37-43)src/components/LocationProvider/VersionProvider.tsx (1)
useVersionContext(6-14)src/components/LocationProvider/hooks/useGetLocationParamsToHash.ts (1)
useGetLocationParamsToHash(6-18)src/components/NoteManager/components/NoteContext.tsx (1)
ISingleNoteContext(6-22)src/components/NoteManager/components/NoteLayout.tsx (1)
NoteLayout(66-73)src/components/NoteManager/components/NoteLink.tsx (1)
NoteLink(14-83)
src/components/NoteManager/NoteManager.tsx (3)
src/components/NotesProvider/types/DecoratedNote.ts (1)
IDecoratedNote(4-19)src/components/NotesProvider/types/StorageNote.ts (1)
IStorageNote(5-5)shared/links-metadata/src/types.ts (1)
ISynctexBlockId(1-4)
src/components/NoteManager/components/NoteLink.tsx (2)
src/components/NotesProvider/types/DecoratedNote.ts (1)
IDecoratedNote(4-19)src/components/NoteManager/components/NoteContext.tsx (1)
useNoteContext(26-32)
src/components/NoteManager/components/NoteDropdown.tsx (1)
src/components/NoteManager/components/NoteContext.tsx (1)
useNoteContext(26-32)
src/components/NoteManager/components/NoteContext.tsx (1)
src/components/NotesProvider/types/DecoratedNote.ts (1)
IDecoratedNote(4-19)
🔇 Additional comments (10)
tools/snapshot-tests/tests/basic-snapshots.spec.ts (1)
88-90: Updated edit button selector matches new dropdown markupSwitching the selector to
[data-testid="edit-note-button"]aligns with theDropdownMenuIteminsrc/components/NoteManager/components/NoteDropdown.tsx, keeping the test in sync with the new dropdown-based edit action.src/components/NoteManager/components/NoteLayout.tsx (1)
6-7: ExposingNoteDropdownviaNoteLayoutlooks goodImporting
NoteDropdownand wiring it asNoteLayout.Dropdownkeeps the note-related components discoverable under a single namespace and avoids ad-hoc imports at call sites. No issues from a typing or structure perspective.Also applies to: 66-73
src/components/NoteManager/NoteManager.tsx (1)
3-3: Ref-backed handlers for add/update/delete are a solid stabilization patternStoring
handleAddNote,handleDeleteNote, andhandleUpdateNotein refs and calling them fromuseCallback-memoized wrappers gives you stable function identities forMemoizedNotesListwhile avoiding stale closures. The updatedhandleAddNoteClickusinghandleAddNoteRef.currentand passing the memoized handlers intoMemoizedNotesListis consistent and should reduce unnecessary re-renders of note items.Also applies to: 33-36, 38-43, 45-52, 72-83, 85-86, 141-147
src/components/NoteManager/components/NoteContext.tsx (1)
6-25: Typed note context plus runtime guard improves safetyDefining
ISingleNoteContextand updatingnoteContexttocreateContext<ISingleNoteContext | null>(null)with a throwinguseNoteContextguard makes misuse (accessing the hook outside a provider) fail fast and keeps all consumers aligned on a single, explicit context shape. The added fields for selection and version links fit well with the dropdown and selection flows introduced elsewhere.src/components/NoteManager/components/NotesList.tsx (1)
14-20: Type contract inconsistency between prop declaration and context exposureThe verification reveals a type mismatch.
NotesList.tsxdeclaresonSelectNotewith a requiredoptsparameter, but internally through the context (NoteContext.tsx), it's exposed ashandleSelectNotewith optional parameters. While the wrapper (memoizedOnSelectNote) provides defaults and always passes both arguments to the parent callback, this creates an inconsistent contract:
- NotesList prop (line 19):
onSelectNote: (note, opts: { type: ... }) => void(required)- NoteContext (line 10):
handleSelectNote: (opts?: { type?: ... }) => void(optional)Call sites like
NoteDropdown.tsx(line 51) andNoteLink.tsx(line 51) callhandleSelectNote()without arguments, which is valid for the context type but obscures the intent.Recommendation: Clarify the type contract—either make the context type match the prop's required signature, or explicitly document why the wrapper provides optional parameters with defaults.
src/components/NoteManager/components/NoteDropdown.tsx (2)
107-152: Delayed alert & Escape-simulation behavior looks solidThe delayed-alert menu item correctly:
- Uses
useEffectwith cleanup to clear the timeout.- Prevents default item behavior while the alert is active.
- Ensures
onClickfires only once per activation and then simulatesEscapeto close the dropdown.No issues from a correctness or UX perspective here.
154-194: Two-step confirmation flow is well-implementedThe two-step confirmation item:
- Uses transient state plus a timeout with proper cleanup.
- Prevents propagation on the first click and lets the second click go through to
onClick.- Applies destructive styling only in the confirmation window.
This matches the intended destructive-action UX and looks correct.
src/components/NoteManager/components/NoteLink.tsx (1)
14-52: Context‑driven navigation wiring looks consistentRouting both the migration badge and the main
OutlineLinkthroughhandleSelectNotecentralizes note selection logic nicely, and the async section/subsection title resolution keeps the component focused on display concerns. This change aligns well with the sharedNoteContextdesign.Also applies to: 72-78
src/components/NoteManager/components/Note.tsx (2)
80-93: Click and keyboard activation behavior for notes looks correctThe root note card:
- Ignores clicks originating from nested buttons/links.
- Avoids re-triggering selection when
activeis already true.- Exposes button-like semantics for inactive notes via
role="button",tabIndex=0, and handling both Enter and Space inhandleNoteEnter.This keeps activation predictable without interfering with inner controls or edit mode.
Also applies to: 95-110, 221-233
118-127: StablehandleSelectNotevia ref + callback is a good patternStoring
onSelectNotein a ref and exposingmemoizedOnSelectNotethrough the context avoids stale-closure issues when the parent callback changes while children hold onto the old version. Combined with thesatisfies ISingleNoteContextcheck, this is a solid and type-safe way to provide the handler to nested components likeNoteDropdownandNoteLink.Also applies to: 148-166
|
Couple of notes:
|
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/components/NoteManager/components/icons/CopyIcon.tsx (1)
1-19: Copy icon implementation looks solidSVG structure and accessibility title look good; no functional issues spotted. If you foresee reuse at different sizes, you could later extend this to accept a
classNameorsizeprop, but what’s here is fine for now.src/components/NoteManager/components/icons/CheckIcon.tsx (1)
1-15: Check icon is fine; consider more specific title textThe checkmark SVG is well-formed and accessible. Since this is used to indicate a successful copy, you might optionally rename the
<title>to something like “Copied” or “Link copied” for slightly clearer screen‑reader output.src/components/NoteManager/components/NoteDropdown.tsx (1)
26-40: Clarify whether “Open” should ever trigger a “close” action when the note is active
handleOpenClosecurrently does:handleSelectNote({ type: active ? "close" : "currentVersion" });but the corresponding menu item is rendered with
disabled={active}, which likely prevents clicks whenactive === true. In that case the"close"path is effectively unreachable.If the intention is:
- “Open” is disabled when the note is already active, and closing happens elsewhere → you can simplify this to always use
"currentVersion"and keepdisabled={active}.- “Open” should toggle between open/close → you probably want to drop
disabled={active}so the item remains clickable, and rely on theactive ? "close" : "currentVersion"logic.Worth double‑checking the desired UX and aligning the implementation to avoid dead branches.
Also applies to: 86-91, 123-163
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/NoteManager/components/NoteDropdown.tsx(1 hunks)src/components/NoteManager/components/NoteLabels.tsx(1 hunks)src/components/NoteManager/components/NoteLayout.tsx(2 hunks)src/components/NoteManager/components/SimpleComponents/DropdownMenuItemCopyButton.tsx(1 hunks)src/components/NoteManager/components/icons/CheckIcon.tsx(1 hunks)src/components/NoteManager/components/icons/CopyIcon.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/NoteLabels.tsxsrc/components/NoteManager/components/icons/CopyIcon.tsxsrc/components/NoteManager/components/NoteLayout.tsxsrc/components/NoteManager/components/SimpleComponents/DropdownMenuItemCopyButton.tsxsrc/components/NoteManager/components/icons/CheckIcon.tsxsrc/components/NoteManager/components/NoteDropdown.tsx
🧠 Learnings (1)
📚 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/NoteLabels.tsxsrc/components/NoteManager/components/NoteDropdown.tsx
🧬 Code graph analysis (3)
src/components/NoteManager/components/NoteLayout.tsx (1)
src/components/NoteManager/components/NoteDropdown.tsx (1)
NoteDropdown(21-121)
src/components/NoteManager/components/SimpleComponents/DropdownMenuItemCopyButton.tsx (2)
src/components/NoteManager/components/icons/CheckIcon.tsx (1)
CheckIcon(1-15)src/components/NoteManager/components/icons/CopyIcon.tsx (1)
CopyIcon(1-19)
src/components/NoteManager/components/NoteDropdown.tsx (2)
src/components/NoteManager/components/NoteContext.tsx (1)
useNoteContext(26-32)src/components/NoteManager/components/SimpleComponents/DropdownMenuItemCopyButton.tsx (1)
DropdownMenuItemCopyButton(6-48)
⏰ 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/NoteManager/components/NoteLabels.tsx (1)
9-9: LGTM! Import path typo corrected.The import path has been correctly updated from
SiimpleComponentstoSimpleComponents, fixing the typo and aligning with the broader SimpleComponents refactor across the codebase.src/components/NoteManager/components/NoteLayout.tsx (1)
6-9: NoteLayout wiring for Dropdown and textarea import looks correctImporting
NoteDropdownand exposing it viaNoteLayout.Dropdown, plus fixing theNoteSimpleTextareaimport path, are consistent with the surrounding structure and should integrate cleanly.Also applies to: 66-73
src/components/NoteManager/components/SimpleComponents/DropdownMenuItemCopyButton.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
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)
80-86: Removee.preventDefault()fromhandleWholeNoteClickto allow normal anchor navigationThe issue is confirmed. The
handleWholeNoteClickhandler at lines 80-86 insrc/components/NoteManager/components/Note.tsxcallse.preventDefault()on clicks that target anchors or buttons. However, the "Open" and "Open in vX" items in the dropdown (insrc/components/NoteManager/components/NoteDropdown.tsx, lines 87 and 96-103) are real<a>elements with href attributes. When users click these links, the click bubbles to the card's click handler, which prevents the default anchor navigation, breaking:
- Normal left-click navigation
- Ctrl/Cmd+click to open in new tab
- Middle-click
- Right-click context menu options
The fix is to remove the
e.preventDefault()call and only return early to short-circuit card activation:const handleWholeNoteClick = (e: React.MouseEvent<HTMLDivElement>) => { const target = e.target; if (target instanceof Element && (target.closest("button") || target.closest("a"))) { - e.preventDefault(); return; } if (active) { return; } onSelectNote(note, { type: "currentVersion" }); };This preserves the intent of ignoring card activation for inner controls while allowing anchors to behave as normal links.
Note: The reference to "lines 289-295" appears to be inaccurate—those lines only render the dropdown component, not a similar handler.
🧹 Nitpick comments (3)
src/components/NoteManager/components/Note.tsx (3)
34-39: Version/metadata link computation and context exposure look consistentUsing
useMetadataContext,useVersionContext, anduseGetLocationParamsToHashto derivenoteOriginalVersionShort,currentVersionLink, andoriginalVersionLinklines up with howNoteLinkconsumes these fields viauseNoteContext. The dependency arrays on bothuseMemohooks and the enclosingnoteLayoutContextmemo look correct and should avoid unnecessary recomputations.If you want to be a bit more explicit, you could narrow the
useMemodependencies to the exact subfields used (e.g.note.originalinstead ofnote) for readability, but this is purely optional.Also applies to: 128-147, 148-166
186-203: Hover/focus/dropdown state resolves the previous keyboard accessibility issueThe new
isHovered/isFocused/isDropdownOpentrio plusnoteRefandhandleIsDropdownOpengive the dropdown a clear visibility model: it’s rendered when the note is hovered, focused, or the dropdown is open, and the focus/mouse tracking on close prevents flicker. This also fixes the prior problem where the dropdown wasn’t reachable via keyboard because it was only shown on hover.If you ever want to trim global listeners, you could switch from a
window‑levelmousemovehandler to pointer‑enter/leave on a slightly larger wrapper around the dropdown trigger, but the current implementation is acceptable and low‑cost.Also applies to: 204-222, 227-250, 289-294
252-288: Minor JSX cleanup: redundant fragment in editing blockIn the
active && isEditingbranch you have nested empty fragments (<>inside another<>) which don’t add structure and slightly increase visual noise:{active && isEditing && ( <> <> <NoteLayout.SelectedText /> ... </> </> )}You can safely collapse this to a single fragment:
- {active && isEditing && ( - <> - <> - <NoteLayout.SelectedText /> - ... - </> - </> - )} + {active && isEditing && ( + <> + <NoteLayout.SelectedText /> + ... + </> + )}Purely cosmetic, but makes the JSX a bit easier to scan.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/NoteManager/components/Note.tsx(7 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
🧠 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: 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.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/components/Note.tsx
🧬 Code graph analysis (1)
src/components/NoteManager/components/Note.tsx (7)
src/components/NotesProvider/types/DecoratedNote.ts (1)
IDecoratedNote(4-19)src/components/MetadataProvider/MetadataProvider.tsx (1)
useMetadataContext(37-43)src/components/LocationProvider/VersionProvider.tsx (1)
useVersionContext(6-14)src/components/LocationProvider/hooks/useGetLocationParamsToHash.ts (1)
useGetLocationParamsToHash(6-18)src/components/NoteManager/components/NoteContext.tsx (1)
ISingleNoteContext(6-22)src/components/NoteManager/components/NoteLayout.tsx (1)
NoteLayout(66-73)src/components/NoteManager/components/NoteLink.tsx (1)
NoteLink(14-83)
⏰ 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/NoteManager/components/Note.tsx (2)
34-39: Validation and error-state handling for note editing looks solid
noteContentErroris consistently cleared on save/cancel/edit and only set fromvalidateMath, so editing flows won’t get stuck in an error state. The conditional rendering of the validation message and error class onNoteLayout.TextAreais coherent with this state.Also applies to: 67-79, 266-283
24-25: Code review approval confirmed through comprehensive inspectionThe
onSelectNotesignature at lines 24–25 and the ref wrapper pattern at lines 118–126 are correctly implemented. All call sites across Note.tsx, NotesList.tsx, and NoteManager.tsx match the signature and type constraints, with proper context wiring to passmemoizedOnSelectNotethrough the layout context. No issues found.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/components/NoteManager/components/SimpleComponents/DropdownMenuItemCopyButton.tsx (1)
43-43: Consider removing negative margin workaround.The
my-[-8px]negative margin suggests a layout issue elsewhere. If possible, adjust the parent container's spacing rather than compensating here.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/NoteManager/components/NoteDropdown.tsx(1 hunks)src/components/NoteManager/components/SimpleComponents/DropdownMenuItemCopyButton.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/NoteManager/components/NoteDropdown.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/SimpleComponents/DropdownMenuItemCopyButton.tsx
🧬 Code graph analysis (1)
src/components/NoteManager/components/SimpleComponents/DropdownMenuItemCopyButton.tsx (2)
src/components/NoteManager/components/icons/CheckIcon.tsx (1)
CheckIcon(1-15)src/components/NoteManager/components/icons/CopyIcon.tsx (1)
CopyIcon(1-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
src/components/NoteManager/components/SimpleComponents/DropdownMenuItemCopyButton.tsx
Outdated
Show resolved
Hide resolved
src/components/NoteManager/components/SimpleComponents/DropdownMenuItemCopyButton.tsx
Show resolved
Hide resolved
src/components/NoteManager/components/SimpleComponents/DropdownMenuItemCopyButton.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/NoteManager/components/NoteDropdown.tsx (1)
60-68: Consider an explicit dropdown close mechanism instead of dispatching synthetic events.While the callback pattern addresses the previous concern, dispatching a synthetic Escape event remains fragile—any component listening for Escape will receive this fabricated event, potentially triggering unintended side effects.
If the dropdown library (DropdownMenu from shared-ui) exposes a programmatic close method or an open state controller, use that instead. For example, if
onOpenChangecan be paired with controlledopenstate, you could manage closing explicitly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/NoteManager/components/NoteDropdown.tsx(1 hunks)src/components/NoteManager/components/SimpleComponents/DropdownMenuItemCopyButton.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/NoteDropdown.tsxsrc/components/NoteManager/components/SimpleComponents/DropdownMenuItemCopyButton.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/components/NoteDropdown.tsxsrc/components/NoteManager/components/SimpleComponents/DropdownMenuItemCopyButton.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/components/NoteDropdown.tsxsrc/components/NoteManager/components/SimpleComponents/DropdownMenuItemCopyButton.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/NoteDropdown.tsx
🧬 Code graph analysis (2)
src/components/NoteManager/components/NoteDropdown.tsx (2)
src/components/NoteManager/components/NoteContext.tsx (1)
useNoteContext(26-32)src/components/NoteManager/components/SimpleComponents/DropdownMenuItemCopyButton.tsx (1)
DropdownMenuItemCopyButton(6-53)
src/components/NoteManager/components/SimpleComponents/DropdownMenuItemCopyButton.tsx (2)
src/components/NoteManager/components/icons/CopyIcon.tsx (1)
CopyIcon(1-19)src/components/NoteManager/components/icons/CheckIcon.tsx (1)
CheckIcon(1-15)
⏰ 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 (6)
src/components/NoteManager/components/NoteDropdown.tsx (3)
1-35: LGTM!The imports, component signature, and context usage are clean and well-structured.
117-128: LGTM!The conditional edit and remove actions are well-structured with proper event handling and the two-step confirmation pattern for the destructive action.
133-173: LGTM!The
TwoStepDropdownMenuItemcomponent provides a clean two-step confirmation pattern with proper timeout cleanup, conditional styling, and event handling for destructive actions.src/components/NoteManager/components/SimpleComponents/DropdownMenuItemCopyButton.tsx (3)
1-9: LGTM!The imports, props definition, and state setup are clean. Using
useReffor the callback is a good pattern to maintain a stable reference.
11-24: LGTM!The useEffect properly manages the timeout lifecycle with cleanup and invokes the callback after the 2-second delay.
48-50: LGTM!The conditional icon rendering provides clear visual feedback for each state (idle, success, error).
src/components/NoteManager/components/SimpleComponents/DropdownMenuItemCopyButton.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/NoteManager/components/NoteDropdown.tsx (1)
96-114: Nested button inside anchor elements.The "Open" and "Open in vX" menu items nest a
<button>(fromDropdownMenuItemCopyButton) inside an<a>element, producing invalid HTML (<a><button>…</button></a>).This was previously discussed in past review comments, and you indicated that you prefer to keep this structure to maintain consistent hover/click areas, accepting the accessibility trade-offs.
🧹 Nitpick comments (1)
src/components/NoteManager/components/NoteDropdown.tsx (1)
60-68: Consider using the dropdown's state management instead of synthetic events.Dispatching a synthetic Escape event to close the dropdown is fragile and couples this component to the dropdown library's keyboard handling implementation. If the library changes how it processes keyboard events, this could break.
A more robust approach would be to control the dropdown's open state directly:
export const NoteDropdown = ({ buttonClassName, onDelete, onOpenChange, }: { buttonClassName?: string; onDelete?: () => void; onOpenChange?: (open: boolean) => void }) => { + const [open, setOpen] = useState(false); const { active, handleSelectNote, noteOriginalVersionShort, note, handleEditClick, currentVersionLink, isEditable, originalVersionLink, } = useNoteContext(); // ... other handlers ... const handleCopyComplete = () => { - const escapeEvent = new KeyboardEvent("keydown", { - key: "Escape", - code: "Escape", - keyCode: 27, - bubbles: true, - }); - document.dispatchEvent(escapeEvent); + setOpen(false); }; return ( - <DropdownMenu onOpenChange={onOpenChange}> + <DropdownMenu open={open} onOpenChange={(isOpen) => { + setOpen(isOpen); + onOpenChange?.(isOpen); + }}>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tools/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!**/*.png
📒 Files selected for processing (2)
src/components/NoteManager/components/NoteDropdown.tsx(1 hunks)src/components/NoteManager/components/SimpleComponents/DropdownMenuItemCopyButton.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/NoteManager/components/SimpleComponents/DropdownMenuItemCopyButton.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/NoteDropdown.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/components/NoteDropdown.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/components/NoteDropdown.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/NoteDropdown.tsx
🧬 Code graph analysis (1)
src/components/NoteManager/components/NoteDropdown.tsx (2)
src/components/NoteManager/components/NoteContext.tsx (1)
useNoteContext(26-32)src/components/NoteManager/components/SimpleComponents/DropdownMenuItemCopyButton.tsx (1)
DropdownMenuItemCopyButton(6-53)
⏰ 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/NoteManager/components/NoteDropdown.tsx (2)
96-127: Verify thatcursor: pointerwas applied to dropdown menu items.The PR comments indicate that the reviewer requested cursor: pointer on dropdown menu elements and you agreed to apply it. However, I don't see any
cursor-pointerTailwind class on theDropdownMenuItemelements in this file (lines 96, 105, 120, and 165).Please confirm:
- Was
cursor: pointeradded to theDropdownMenuItemcomponent definition itself (in the shared-ui library)?- Or should the
cursor-pointerclass be added to the individual menu items in this file?The anchor elements (lines 97, 106) will have pointer cursor by default, but the non-anchor items (lines 120, 165-171) may need explicit styling.
If the style needs to be added here, consider:
- <DropdownMenuItem onClick={editNode} data-testid="edit-note-button"> + <DropdownMenuItem onClick={editNode} data-testid="edit-note-button" className="cursor-pointer"> <span>Edit note</span> </DropdownMenuItem>And similarly for the TwoStepDropdownMenuItem, either in its definition or in its className prop.
52-58: No action required—callinghandleSelectNote()without arguments is valid.The function signature in NoteContext.tsx defines the parameter as optional:
handleSelectNote: (opts?: { type?: "currentVersion" | "originalVersion" | "close" }) => void;The
opts?notation makes the parameter fully optional, so both calling patterns are valid:
handleSelectNote()— uses default behaviorhandleSelectNote({ type: "currentVersion" })— explicitly specifies typeThis pattern is consistent across the codebase (e.g., NoteLink.tsx line 51 also calls
handleSelectNote()without arguments). The code is correct as written.Likely an incorrect or invalid review comment.
2 participants
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.