fix(viewer): mount/unmount zoom viewer instead of Activity hide/show (2nd-open crash)#509
Merged
Merged
Conversation
The full-screen WebGL zoom viewer was wrapped in `<Activity mode=...>` to show/
hide it. React `<Activity>` preserves the child's DOM (the same `<canvas>`) and
state, but runs the child's effect cleanup on hide and re-runs effects on show.
So closing the zoom fires WebGLImageViewer's effect cleanup → the FU-13
`destroy()` → `loseContext()`, which permanently loses the (reused) canvas's
WebGL context and nulls the engine ref. On the second open the effect re-runs,
but `isInitialized` state is preserved across Activity hide/show, so
`setUpWebGLEngine`'s `if (isInitialized) return` guard skips rebuilding the
engine — leaving a null engine on a canvas whose context is dead. The engine
(when it does run on the lost context) gets a null program from createProgram()
and throws in attachShader → blank image + full-page crash on the 2nd zoom.
Reproduced via devtools: open#1 context alive → close `lost=1` → open#2 creates
no new context and `ctxAlive=false`.
Fix: conditionally MOUNT/UNMOUNT the viewer (`{showFullScreenViewer && ...}`)
instead of toggling `<Activity>`. Every open is a fresh WebGLImageViewer → fresh
`<canvas>` → fresh context → fresh `isInitialized` → the engine builds normally.
Every close is a real unmount, so the FU-13 `destroy()`/`loseContext()` runs on a
canvas that is being discarded — correct, and the leak fix is fully preserved
(open/close still create+release exactly one context each). The only trade-off
is that zoom/pan position resets on each open, which is expected for a lightbox.
Regression introduced by FU-13 (#503), whose 10-cycle test only covered the
album→detail→zoom→navigate-away (true unmount) path, not detail-page
open→close→reopen (Activity hide/show, reused canvas).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Zheaoli
added a commit
that referenced
this pull request
May 31, 2026
fix(viewer): keep zoom viewer mounted + CSS-toggle (restore context reuse/state, follow-up to #509)
pull Bot
pushed a commit
to candies404/PicImpact
that referenced
this pull request
May 31, 2026
…t/state Follow-up to besscroft#509. besscroft#509 fixed the 2nd-open crash by conditionally mounting/ unmounting the full-screen viewer, but that regressed UX: every open rebuilt the WebGL engine (re-creation cost) and lost the zoom/pan state. The original design deliberately kept one engine + context alive across open/close within the modal, preserving zoom/pan and avoiding rebuilds. Root cause of the crash was that the viewer was toggled with `<Activity>`, which runs the child's effect cleanup on hide — firing FU-13's destroy()/loseContext() on every close, so the 2nd open landed on a permanently-lost context (crash). The destroy was bound to the wrong lifecycle event (zoom close instead of detail unmount). Fix C: keep the viewer MOUNTED once opened and toggle it with CSS `visibility` instead of `<Activity>` / conditional unmount: - Staying mounted means the effect cleanup never runs on close → one engine + WebGL context stays alive across open/close → context reused, zoom/pan state preserved, zero rebuild cost (restores the original UX). - destroy()/loseContext() now only runs when ProgressiveImage unmounts (leaving the detail page) — via the WebGLImageViewer's own unmount cleanup — so the FU-13 leak fix is fully preserved (one context created+released per detail). - `visibility:hidden` (not display:none) keeps the canvas sized, so re-showing doesn't drive the engine through a 0×0 resize that would reset the zoom; pointer-events are disabled while hidden so the invisible overlay never blocks the page underneath. - `hasOpenedFullScreen` latches on first open so the engine is still built lazily (not before the user zooms). The WebGL engine and FU-13's destroy()/loseContext() are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
@Zheaoli : on the image detail page, the second time you click into the WebGL zoom, the whole page crashes and the image won't display.
Root cause (devtools-reproduced + code, confirmed by Design + Impl)
The full-screen viewer was wrapped in
<Activity mode={showFullScreenViewer ? 'visible' : 'hidden'}>. React<Activity>preserves the child DOM (the same<canvas>) and state, but runs the child's effect cleanup on hide and re-runs effects on show. So:destroy()→loseContext()permanently loses α + nulls the engine ref; canvas C is preserved by Activity.setUpWebGLEngine(), butisInitializedstate was preserved (true) →if (isInitialized) returnskips rebuilding → null engine on a dead-context canvas →createProgram()returns null →attachShader(null, …)throws → blank image + full-page crash.devtools evidence: open#1 context alive → close
lost=1→ open#2 no new context created,ctxAlive=false.This is a regression from FU-13 (#503) — its 10-cycle test only exercised album→detail→zoom→navigate-away (true unmount, fresh canvas next time), not detail-page open→close→reopen (Activity hide/show reusing the same canvas).
Fix
Conditionally mount/unmount the viewer (
{showFullScreenViewer && ( … )}) instead of toggling<Activity>:WebGLImageViewer→ fresh<canvas>→ fresh context → freshisInitialized→ engine builds normally.destroy()/loseContext()runs on a canvas that's being discarded — correct, and the leak fix is fully preserved (each open/close still creates + releases exactly one context).No engine change; FU-13's
loseContext()is untouched (correct under the mount/unmount model).Verification
pnpm exec tsc --noEmit— no new errors;pnpm lint— 0 errors.ctxAlive=true). Will re-run the reproduction harness after deploy.Release-blocker.
🤖 Generated with Claude Code