fix: broken initial selection of text on first page#333
Conversation
✅ Deploy Preview for graypaper-reader ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds a new hook Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant PDFJS as pdfjs-dist
participant Hook as useTextLayerRendered
participant Provider as PdfProvider
participant Context as PdfContext
participant Selection as SelectionRenderer
Note over PDFJS,Hook: text layer rendered events
PDFJS->>Hook: "textlayerrendered"(pageNumber)
Hook->>Hook: add unique pageNumber to textLayerRenderedRef.current
Hook->>Provider: provide textLayerRenderedRef
Provider->>Context: include textLayerRenderedRef in context value
Selection->>Context: read textLayerRenderedRef
Selection->>Selection: check textLayerRenderedRef.current.includes(pageNumber)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Visual Regression Test Report ✅ PassedGithub run id: 18889516699 🔗 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/PdfProvider/PdfProvider.tsx (1)
231-245: IncludetextLayerRenderedVersionin context value and memo deps.The ref identity won’t change on events; the version ensures subscribers can update.
pageOffsets, downloadPdfWithTheme, - textLayerRenderedRef, + textLayerRenderedRef, + textLayerRenderedVersion, }), - [theme, viewer, visiblePages, services, scale, downloadPdfWithTheme, textLayerRenderedRef], + [theme, viewer, visiblePages, services, scale, downloadPdfWithTheme, textLayerRenderedRef, textLayerRenderedVersion],
🧹 Nitpick comments (3)
src/components/PdfViewer/SelectionRenderer/SelectionRenderer.tsx (1)
33-40: Retry timer: use functional setState and align delay with comment.Avoid stale closure on
retryScrollingand fix the “in a second” comment vs 100ms delay mismatch.- // we don't have an offset yet, so we will retry in a second or so. + // we don't have an offset yet, so retry shortly. setTimeout(() => { - setRetryScrolling(retryScrolling + 1); - }, 100); + setRetryScrolling((n) => n + 1); + }, 200);src/components/PdfProvider/hooks/useTextLayerRendered.ts (2)
4-4: Naming consistency: file vs hook export.Exported name
useTextLayerdoesn’t match fileuseTextLayerRendered.ts. Consider renaming touseTextLayerRenderedfor clarity and searchability.
13-15: Avoid noisy logs in production paths.Remove
console.logor gate behindNODE_ENV !== "production".
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/PdfProvider/PdfProvider.tsx(4 hunks)src/components/PdfProvider/hooks/useTextLayerRendered.ts(1 hunks)src/components/PdfViewer/SelectionRenderer/SelectionRenderer.tsx(3 hunks)src/components/PdfViewer/utils.ts(0 hunks)
💤 Files with no reviewable changes (1)
- src/components/PdfViewer/utils.ts
🧰 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/PdfProvider/hooks/useTextLayerRendered.tssrc/components/PdfViewer/SelectionRenderer/SelectionRenderer.tsxsrc/components/PdfProvider/PdfProvider.tsx
🧬 Code graph analysis (2)
src/components/PdfViewer/SelectionRenderer/SelectionRenderer.tsx (1)
src/components/PdfProvider/PdfProvider.tsx (2)
PdfContext(13-13)IPdfContext(27-37)
src/components/PdfProvider/PdfProvider.tsx (1)
src/components/PdfProvider/hooks/useTextLayerRendered.ts (1)
useTextLayer(4-25)
⏰ 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/PdfProvider/PdfProvider.tsx (1)
34-35: No fixes required; null-safety is already properly handled.The review comment raised concerns about null-safety when accessing
pageOffsets.current[page]. However, verification shows this is already correctly implemented:
- SelectionRenderer guards with
if (!pageOffset)checks before using the value- NoteRenderer passes the value to HighlightNote, which declares
pageOffsetas an optional prop (pageOffset?: DOMRect) and explicitly checksif (!pageOffset || ...)before renderingThe type definition (
RefObject<DOMRect[]>) is correct, and all consumers properly handle the undefined case. Optional chaining is unnecessary here; the current pattern of defensive checks is appropriate.src/components/PdfViewer/SelectionRenderer/SelectionRenderer.tsx (1)
110-114: Incorrect review comment - type already prevents nullcurrent.The type
RefObject<DOMRect[]>guarantees thatpageOffsets.currentis always aDOMRect[]array, never null. It's initialized asuseRef([])and cannot have a nullcurrentproperty. The concern about guarding against nullcurrentis unfounded.Array elements may be
undefineduntil populated, but this is already handled by existing checks (line 33 guards the element value, line 110 checks bothpageNumberand validates the result, and line 42 passes to an optional prop). No changes needed.Likely an incorrect or invalid review comment.
chmurson
force-pushed
the
fix-initial-pdf-selection
branch
from
900392e to
6f72f15
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/PdfProvider/hooks/useTextLayerRendered.ts (1)
7-23: Reset the ref on EventBus change/clear to avoid stale pages across documents.If a new PDF loads (or EventBus becomes undefined),
textLayerRenderedRef.currentretains old page numbers. Consumers checkingincludes(pageNumber)may get false positives for the new doc (e.g., page 1). Clear the ref wheneventBuschanges; this preserves your ref‑only design.Apply this minimal diff:
useEffect(() => { - if (!eventBus) { - return; - } + // New document or cleared EventBus → reset tracked pages + textLayerRenderedRef.current = []; + if (!eventBus) return; const handleTextLayerRendered = (e: { pageNumber: number }) => { if (!textLayerRenderedRef.current.includes(e.pageNumber)) { textLayerRenderedRef.current.push(e.pageNumber); } }; eventBus.on("textlayerrendered", handleTextLayerRendered); return () => { eventBus.off("textlayerrendered", handleTextLayerRendered); }; }, [eventBus]);Optional: If your viewer reuses the same
EventBusacross documents, also reset on a doc lifecycle event (e.g.,pagesinit/pagesloaded) within this effect.
🧹 Nitpick comments (1)
src/components/PdfProvider/hooks/useTextLayerRendered.ts (1)
12-16: Optional: streamline dedup with a Set one‑liner.Current
includes+pushworks; a Set keeps entries unique more idiomatically. Keeps ref‑only approach intact.- const handleTextLayerRendered = (e: { pageNumber: number }) => { - if (!textLayerRenderedRef.current.includes(e.pageNumber)) { - textLayerRenderedRef.current.push(e.pageNumber); - } - }; + const handleTextLayerRendered = (e: { pageNumber: number }) => { + // Avoid duplicates in O(1) using Set, keep outward type as number[] + textLayerRenderedRef.current = Array.from( + new Set([...textLayerRenderedRef.current, e.pageNumber]), + ); + };Based on learnings.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/PdfProvider/hooks/useTextLayerRendered.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:
src/components/PdfProvider/hooks/useTextLayerRendered.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: chmurson
PR: FluffyLabs/graypaper-reader#333
File: src/components/PdfProvider/PdfProvider.tsx:34-37
Timestamp: 2025-10-28T20:41:06.862Z
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.
📚 Learning: 2025-10-28T20:41:06.862Z
Learnt from: chmurson
PR: FluffyLabs/graypaper-reader#333
File: src/components/PdfProvider/PdfProvider.tsx:34-37
Timestamp: 2025-10-28T20:41:06.862Z
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/PdfProvider/hooks/useTextLayerRendered.ts
⏰ 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 (1)
src/components/PdfProvider/hooks/useTextLayerRendered.ts (1)
1-6: Design choice (ref‑only) acknowledged.Using a ref without re‑render ticks matches the project’s intent; consumers can poll
textLayerRenderedRef.currentas needed. No further changes required here.Based on learnings.
1 participant
What
Why?
It was obscure regression that appeard after "Selected text" was moved inside of Notes tab. It used to be above component.
Summary by CodeRabbit