Skip to content

fix: clear textLayerRendered ref on unmount#334

Merged
chmurson merged 1 commit intomainfrom
add-missing-clearing-state
Oct 28, 2025
Merged

fix: clear textLayerRendered ref on unmount#334
chmurson merged 1 commit intomainfrom
add-missing-clearing-state

Conversation

@chmurson
Copy link
Collaborator

@chmurson chmurson commented Oct 28, 2025

Add missing change from previous PR #333 ; add clearing ref value on unmount

Summary by CodeRabbit

  • Bug Fixes
    • Improved memory management when PDF components unmount, ensuring proper resource cleanup.

@netlify
Copy link

netlify bot commented Oct 28, 2025

Deploy Preview for graypaper-reader ready!

Name Link
🔨 Latest commit a033bb2
🔍 Latest deploy log https://app.netlify.com/projects/graypaper-reader/deploys/690136f2c2da280008f2b2b9
😎 Deploy Preview https://deploy-preview-334--graypaper-reader.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai
Copy link

coderabbitai bot commented Oct 28, 2025

Walkthrough

A cleanup improvement to the useTextLayerRendered hook that resets the textLayerRenderedRef to an empty array on component unmount, ensuring stored page numbers are cleared when the component is torn down.

Changes

Cohort / File(s) Summary
Unmount cleanup for text layer ref
src/components/PdfProvider/hooks/useTextLayerRendered.ts
Added cleanup logic to reset textLayerRenderedRef to an empty array on component unmount, clearing stored page numbers without affecting event subscription logic or return value.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

A rabbit hops through code so clean,
No dangling refs left to be seen,
Unmount brings peace, the slate runs bare,
Old page numbers drift through the air,
Tidy closures—that's quite rare! 🐰✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "fix: clear textLayerRendered ref on unmount" directly aligns with the main change described in the raw summary: cleaning up the textLayerRenderedRef on component unmount by resetting it to an empty array. The title is specific and clear about what is being fixed (clearing a ref) and when it occurs (on unmount), avoiding vague terminology and noise. It follows conventional commit style and would help teammates understand the primary change when scanning commit history.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch add-missing-clearing-state

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6502258 and a033bb2.

📒 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)

20-23: LGTM! Proper cleanup of ref state on unmount.

Resetting the ref to an empty array in the cleanup function ensures that stale page numbers don't persist when the component unmounts or when the eventBus changes (e.g., switching PDF documents). This prevents memory leaks and maintains correct state tracking.

Based on learnings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Contributor

Visual Regression Test Report ✅ Passed

Github run id: 18889885335

🔗 Artifacts: Download

@chmurson chmurson merged commit a237ede into main Oct 28, 2025
9 checks passed
@chmurson chmurson deleted the add-missing-clearing-state branch October 28, 2025 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant