Skip to content

fix: guard clipboard/eyedropper handlers against updates after unmount - #35

Merged
DavidHDev merged 1 commit into
DavidHDev:mainfrom
ngXuanHaiNam612:fix/guard-copy-handlers-unmount
Aug 14, 2026
Merged

fix: guard clipboard/eyedropper handlers against updates after unmount#35
DavidHDev merged 1 commit into
DavidHDev:mainfrom
ngXuanHaiNam612:fix/guard-copy-handlers-unmount

Conversation

@ngXuanHaiNam612

@ngXuanHaiNam612 ngXuanHaiNam612 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Fixes #39

What does this PR do?

Guards 6 clipboard/eyedropper handlers (+ preview-video.tsx) against calling setState / scheduling a new timer after the owning component has unmounted, using a new shared useMountedRef() hook.

Type of change

  • Bug fix

Checklist

  • npx tsc --noEmit passes
  • npm run lint passes
  • npm run build passes
  • Tested in Chrome (with the html-in-canvas flag, if relevant)

@DavidHDev

Copy link
Copy Markdown
Owner

Thanks for taking this on, and for being thorough about it.

The catch is that this repo is on React 19, and since React 18 calling setState on an unmounted component is a silent no-op. The old warning was removed and it isn't a leak, so most of these guards (copy-button, copy-menu, playground-actions, color-picker, html-in-canvas-banner) don't change any behaviour.

Two parts of this are genuinely worth keeping though:

  • preview-video.tsx — the cancelled flag is a real fix. Without it, the play().then() callback can pause and rewind the video after the effect has already been cleaned up.
  • demo-controls.tsx — guarding play("bloom") is fair, since a sound firing after unmount is actually perceivable.

Would you be up for trimming this down to just those two and dropping the useMountedRef hook? That keeps the real fix without adding a shared abstraction that would need explaining to the next person who reads it.

@ngXuanHaiNam612

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed explanation — that's a fair point, and you're right that I conflated "state update after unmount" with "side effect after unmount." I hadn't accounted for React 18+ making unmounted setState a silent no-op.

I'll trim this down to just the two real fixes.

…share-link button

Trimmed from the original guard-everywhere approach after review feedback:
since React 18+ makes setState on an unmounted component a silent no-op,
guarding copy-button.tsx, copy-menu.tsx, playground-actions.tsx,
color-picker.tsx, and html-in-canvas-banner.tsx didn't change any
observable behavior. Kept only the two guards that protect real,
perceivable side effects:

- preview-video.tsx: a cancelled flag prevents video.pause()/currentTime
  reset from firing on the DOM node after the effect's cleanup has run.
- demo-controls.tsx (ShareLinkButton): a local mounted ref prevents
  play("bloom") from firing an audible sound after the button is gone.

Dropped the shared useMountedRef() hook since it's no longer needed.
@ngXuanHaiNam612
ngXuanHaiNam612 force-pushed the fix/guard-copy-handlers-unmount branch from d19d3c8 to b64519f Compare August 14, 2026 10:48
@DavidHDev
DavidHDev merged commit 2dd45d7 into DavidHDev:main Aug 14, 2026
1 check passed
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.

Copy-to-clipboard and EyeDropper handlers can update state after their component unmounts

2 participants