Paragraph for active note #331
Conversation
# Conflicts: # src/components/NoteManager/components/Note.tsx
✅ Deploy Preview for graypaper-reader ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughReworks Note to provide a memoized noteLayoutContext to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User/UI
participant NoteComp as Note component
participant NoteLayout as NoteLayout.Root
participant NLContext as noteLayoutContext (memoized)
participant Selection as SelectionContext
participant Handlers as Note handlers
User->>NoteComp: render(note, active)
NoteComp->>NLContext: build memoized context (note, isEditable, isEditing, handlers)
NoteComp->>NoteLayout: provide(NLContext)
alt note is active
NoteLayout->>Selection: request current selection
Selection-->>NoteLayout: selectionString
NoteLayout->>User: render NoteLayout.SelectedText (header, selectionString, Edit button)
User->>Handlers: click Edit
Handlers-->>NLContext: set isEditing = true
NoteLayout->>User: render editing UI (textarea) or view UI (text)
else not active
NoteLayout->>User: render NoteLayout.Text / NoteLabels
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 18733218597 🔗 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: 3
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)
126-133: Keyboard handler logic inverted; fix activation keys and preventDefault only for Enter/Space.Currently prevents default on all keys except “Enter”/“Space”, breaking keyboard UX. Also, KeyboardEvent.key for space is " " (single space), not "Space".
const handleNoteEnter = (e: React.KeyboardEvent<HTMLDivElement>) => { if (e.target instanceof Element && (e.target.closest("input") || e.target.closest("textarea"))) { return; } - - if (e.key !== "Enter" && e.key !== "Space") { - e.preventDefault(); - } + const isActivationKey = e.key === "Enter" || e.key === " " || e.code === "Space"; + if (!isActivationKey) return; + e.preventDefault(); if (active) { return; } setLocationParams({
🧹 Nitpick comments (2)
src/components/NoteManager/components/Note.tsx (2)
35-41: Context looks good; add a displayName for better DevTools debugging.Optional but helpful when multiple providers are nested.
const noteContext = createContext<{ note: IDecoratedNote; isEditable: boolean; handleEditClick: () => void; onEditNote: INotesContext["handleUpdateNote"]; isEditing: boolean; } | null>(null); + +noteContext.displayName = "NoteLayoutContext";
183-189: Drop redundant isEditing check inside an already guarded branch.You’re already in
active && !isEditing; the extra ternary is unnecessary.{active && !isEditing && ( <> <NoteLayout.SelectedText /> <NoteLayout.Text /> - {!isEditing ? <NoteLabels note={note} /> : null} + <NoteLabels note={note} /> </> )}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/NoteManager/components/Note.tsx(9 hunks)src/components/NoteManager/components/NoteLink.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/NoteLink.tsxsrc/components/NoteManager/components/Note.tsx
🧠 Learnings (2)
📚 Learning: 2025-10-22T20:36:10.394Z
Learnt from: chmurson
PR: FluffyLabs/graypaper-reader#330
File: src/components/Outline/OutlineLink.tsx:17-21
Timestamp: 2025-10-22T20:36:10.394Z
Learning: Project targets React 19+; prefer passing refs as a regular prop in function components and avoid React.forwardRef in new code. File: src/components/Outline/OutlineLink.tsx.
Applied to files:
src/components/NoteManager/components/NoteLink.tsx
📚 Learning: 2025-10-22T20:36:10.394Z
Learnt from: chmurson
PR: FluffyLabs/graypaper-reader#330
File: src/components/Outline/OutlineLink.tsx:17-21
Timestamp: 2025-10-22T20:36:10.394Z
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/NoteLink.tsx
🧬 Code graph analysis (1)
src/components/NoteManager/components/Note.tsx (5)
src/components/NotesProvider/types/DecoratedNote.ts (1)
IDecoratedNote(4-19)src/components/NotesProvider/NotesProvider.tsx (1)
INotesContext(20-40)src/components/NoteManager/components/NoteLabels.tsx (1)
NoteLabels(9-17)src/components/SelectionProvider/SelectionProvider.tsx (2)
SelectionContext(35-35)ISelectionContext(21-29)src/components/NoteManager/components/NoteLink.tsx (1)
NoteLink(16-131)
⏰ 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 (3)
src/components/NoteManager/components/Note.tsx (3)
151-160: Good useMemo for stable context value.Dependencies are complete; avoids unnecessary consumer re-renders.
163-176: Provider wrap + interactive card LGTM.Structure is clear; Tailwind 4.x arbitrary values (bg-[var(...)]/shadow-[...]) are valid.
If “Activate label” (Line 173) refers to activating a note, consider “Activate note” for clarity.
261-262: API surface of NoteLayout is tidy.Exposing Root/Text/SelectedText keeps call sites clean.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/NoteManager/components/Note.tsx (1)
233-258: SelectedText: aria-label added; token usage OK.
The edit button now has an accessible name;bg-sidebarandborder-brand-primarytokens align with shared UI theme.
🧹 Nitpick comments (9)
src/components/NoteManager/components/Note.tsx (4)
35-41: Context shape looks good; add a displayName for better DevTools traces.
Optional but helpful in debugging.} | null>(null); + +// Improves React DevTools readability +noteContext.displayName = "NoteContext";
62-69: Avoid mutating state object when updating labels.
Use functional setState to prevent accidental stale reads and side effects.- const handleEditLabels = useCallback( - (labels: UnPrefixedLabel[]) => { - noteDirty.labels = [...new Set(labels)]; - setNoteDirty({ ...noteDirty }); - }, - [noteDirty], - ); + const handleEditLabels = useCallback( + (labels: UnPrefixedLabel[]) => { + const unique = [...new Set(labels)]; + setNoteDirty((prev) => ({ ...prev, labels: unique })); + }, + [], + );
167-169: Accessible name: clarify and omit when inactive.
Use a meaningful label; avoid empty string when active.- role={!active ? "button" : undefined} - tabIndex={!active ? 0 : undefined} - aria-label={!active ? "Activate label" : ""} + role={!active ? "button" : undefined} + tabIndex={!active ? 0 : undefined} + aria-label={!active ? "Open note" : undefined}Also applies to: 171-173
222-229: Minor semantics (optional): wrap author in a .
Purely presentational; improves markup semantics.- return ( - <blockquote className="whitespace-pre-wrap"> - {note.original.author} - <NoteContent content={note.original.content} /> - </blockquote> - ); + return ( + <blockquote className="whitespace-pre-wrap"> + <cite>{note.original.author}</cite> + <NoteContent content={note.original.content} /> + </blockquote> + );tools/snapshot-tests/tests/basic-snapshots.spec.ts (5)
23-31: Close the shared context to avoid resource leaks.
Add afterAll to close the BrowserContext created in beforeAll.test.beforeAll(async ({ browser }) => { context = await browser.newContext(getCommonContext()); page = await context.newPage(); }); + + test.afterAll(async () => { + await context.close(); + });
53-62: Remove unusedbrowserfixture in this test.
Keeps the signature clean.- test("notes tab - initial state", async ({ browser }) => { + test("notes tab - initial state", async () => {
103-110: Use locator by placeholder with case-insensitive match.
CSS substring match is case-sensitive and brittle.- const searchInput = page.locator('input[placeholder*="search"]'); - await searchInput.first().fill("protocol"); + const searchInput = page.getByPlaceholder(/search/i); + await searchInput.fill("protocol"); await page.locator(".search-results:not(.search-loading)").waitFor({ state: "visible" }); - await expect(page).toHaveScreenshot("search-with-query.png", { fullPage: true }); - await searchInput.first().fill("blockchain"); + await expect(page).toHaveScreenshot("search-with-query.png", { fullPage: true }); + await searchInput.fill("blockchain");
140-154: Ensure ad-hoc contexts are closed in tests.
Prevent leaks and speed up runs.- test("notes dropdown - initial state", async ({ browser }) => { - const context = await browser.newContext(getCommonContext()); - const page = await context.newPage(); - await page.goto(hostname, { waitUntil: "networkidle" }); - await page.evaluate(() => document.fonts.ready); + test("notes dropdown - initial state", async ({ browser }) => { + const context = await browser.newContext(getCommonContext()); + const page = await context.newPage(); + try { + await page.goto(hostname, { waitUntil: "networkidle" }); + await page.evaluate(() => document.fonts.ready); - const notesDropdown = page.locator( - '[data-testid="notes-dropdown"], button:has-text("Notes"), [aria-label*="notes"], .notes-dropdown', - ); - if ((await notesDropdown.count()) > 0) { - await notesDropdown.first().click(); + const notesDropdown = page.locator( + '[data-testid="notes-dropdown"], button:has-text("Notes"), [aria-label*="notes"], .notes-dropdown', + ); + if ((await notesDropdown.count()) > 0) { + await notesDropdown.first().click(); - await expect(page).toHaveScreenshot("topbar-notes-dropdown.png", { fullPage: true }); - } + await expect(page).toHaveScreenshot("topbar-notes-dropdown.png", { fullPage: true }); + } + } finally { + await context.close(); + } });
156-179: Close context in “selecting note on and off” as well.
Same rationale: cleanup and stability.- test("selecting note on and off", async ({ browser }) => { - const context = await browser.newContext(getCommonContext()); - const page = await context.newPage(); - await page.goto(hostname, { waitUntil: "networkidle" }); - await page.evaluate(() => document.fonts.ready); + test("selecting note on and off", async ({ browser }) => { + const context = await browser.newContext(getCommonContext()); + const page = await context.newPage(); + try { + await page.goto(hostname, { waitUntil: "networkidle" }); + await page.evaluate(() => document.fonts.ready); - const notesDropdown = page.locator( - '[data-testid="notes-dropdown"], button:has-text("Notes"), [aria-label*="notes"], .notes-dropdown', - ); - if ((await notesDropdown.count()) > 0) { - await notesDropdown.first().click(); + const notesDropdown = page.locator( + '[data-testid="notes-dropdown"], button:has-text("Notes"), [aria-label*="notes"], .notes-dropdown', + ); + if ((await notesDropdown.count()) > 0) { + await notesDropdown.first().click(); - const noteOptions = page.locator('[type="checkbox"], .note-option, [role="menuitemcheckbox"]'); - if ((await noteOptions.count()) > 0) { - await noteOptions.first().click(); + const noteOptions = page.locator('[type="checkbox"], .note-option, [role="menuitemcheckbox"]'); + if ((await noteOptions.count()) > 0) { + await noteOptions.first().click(); - await expect(page).toHaveScreenshot("topbar-notes-selected-on.png", { fullPage: true }); + await expect(page).toHaveScreenshot("topbar-notes-selected-on.png", { fullPage: true }); - await noteOptions.first().click(); + await noteOptions.first().click(); - await expect(page).toHaveScreenshot("topbar-notes-selected-off.png", { fullPage: true }); - } - } + await expect(page).toHaveScreenshot("topbar-notes-selected-off.png", { fullPage: true }); + } + } + } finally { + await context.close(); + } });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (34)
tools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/homepage-outline-dark-mode-darwin.pngis excluded by!**/*.pngtools/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-darwin.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-activation-dark-mode-darwin.pngis excluded by!**/*.pngtools/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-darwin.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-darwin.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-darwin.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-initial-dark-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/notes-tab-initial-light-mode-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/search-initial-dark-mode-darwin.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-darwin.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-darwin.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-darwin.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-darwin.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-darwin.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-darwin.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-darwin.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-darwin.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-darwin.pngis excluded by!**/*.pngtools/snapshot-tests/tests/basic-snapshots.spec.ts-snapshots/topbar-github-option-hover-light-mode-linux.pngis excluded by!**/*.png
📒 Files selected for processing (2)
src/components/NoteManager/components/Note.tsx(8 hunks)tools/snapshot-tests/tests/basic-snapshots.spec.ts(2 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/Note.tsx
🧬 Code graph analysis (1)
src/components/NoteManager/components/Note.tsx (5)
src/components/NotesProvider/types/DecoratedNote.ts (1)
IDecoratedNote(4-19)src/components/NotesProvider/NotesProvider.tsx (1)
INotesContext(20-40)src/components/NoteManager/components/NoteLabels.tsx (1)
NoteLabels(9-17)src/components/SelectionProvider/SelectionProvider.tsx (2)
SelectionContext(35-35)ISelectionContext(21-29)src/components/NoteManager/components/NoteLink.tsx (1)
NoteLink(16-131)
⏰ 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 (4)
src/components/NoteManager/components/Note.tsx (4)
8-8: LGTM: imports and SelectionContext usage are correct.Also applies to: 21-21
150-159: LGTM: memoized NoteLayout context and Provider wiring.
This reduces renders and clarifies composition boundaries.Also applies to: 162-162
184-186: SelectedText usage in active states looks correct.
Consistent header + selection renders in both view and edit modes.Also applies to: 192-193
263-264: Export surface LGTM.
Exposing SelectedText via NoteLayout is coherent and keeps API tidy.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/NoteManager/components/Note.tsx (1)
233-259: SelectedText: consider safer context access pattern.Line 234 uses type assertion (
as ISelectionContext) which bypasses null-checking. While consistent with existing code (e.g.,NoteLink.tsx), it risks runtime errors ifSelectionProvideris missing.Consider creating a
useSelectionContexthook similar touseNoteContext(lines 43-49) for consistency and better error messages:const useSelectionContext = () => { const context = useContext(SelectionContext); if (!context) { throw new Error("useSelectionContext must be used within a SelectionProvider"); } return context; };Then use it:
const SelectedText = () => { - const { selectionString } = useContext(SelectionContext) as ISelectionContext; + const { selectionString } = useSelectionContext(); const { handleEditClick, isEditable, note, onEditNote, isEditing } = useNoteContext();This would provide consistent error handling across all context usage in the component.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/NoteManager/components/Note.tsx(8 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
🧬 Code graph analysis (1)
src/components/NoteManager/components/Note.tsx (5)
src/components/NotesProvider/types/DecoratedNote.ts (1)
IDecoratedNote(4-19)src/components/NotesProvider/NotesProvider.tsx (1)
INotesContext(20-40)src/components/NoteManager/components/NoteLabels.tsx (1)
NoteLabels(9-17)src/components/SelectionProvider/SelectionProvider.tsx (2)
SelectionContext(35-35)ISelectionContext(21-29)src/components/NoteManager/components/NoteLink.tsx (1)
NoteLink(16-131)
⏰ 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 (4)
src/components/NoteManager/components/Note.tsx (4)
35-41: Well-structured context definition.The context shape is clear and includes all necessary properties for the NoteLayout components to function. This refactor improves maintainability and enables better composition.
124-143: Keyboard accessibility correctly implemented!This properly addresses the past review concern. The handler now:
- Only responds to Enter/Space (no longer blocks Tab/arrow navigation)
- Uses robust Space detection (
e.key === " "ORe.code === "Space")- Only prevents default for activation keys to stop page scroll
- Guards inputs/textareas correctly
151-160: Good optimization with useMemo.Memoizing the context value prevents unnecessary re-renders of descendant components. The dependency array is complete and correct.
222-231: Clean refactor to use context.Consistent with the component composition pattern. The
useNoteContexthook provides a safe way to access the context with built-in error handling.
|
@chmurson FYI: Something weird happens when selecting text at the very beginning of the document: Initially I only saw section details (no text at all), but after selecting some text further down the document the text was displayed correctly on the right (and the note I created displays it when active), but selecting something back at the beginning of the document leaves the previously selected text. |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Style
Tests