fix(viewer): keep zoom viewer mounted + CSS-toggle (restore context reuse/state, follow-up to #509)#510
Merged
Merged
Conversation
…t/state Follow-up to #509. #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>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Context
Follow-up to #509 (already merged). @Zheaoli pointed out that #509's approach (conditionally mount/unmount the full-screen viewer) fixed the 2nd-open crash but 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 (recap)
The viewer was toggled with
<Activity>, which runs the child's effect cleanup on hide → fired FU-13'sdestroy()/loseContext()on every close → the 2nd open landed on a permanently-lost context → crash. The destroy was bound to the wrong lifecycle (zoom-close instead of detail-unmount).Fix C
Keep the viewer mounted once opened and toggle it with CSS
visibilityinstead of<Activity>/ conditional unmount:destroy()/loseContext()now runs only when ProgressiveImage unmounts (leaving the detail page), via the WebGLImageViewer's own unmount cleanup → FU-13 leak fix fully preserved (one context created+released per detail visit,created==lost).visibility:hidden(notdisplay:none) keeps the canvas sized, so re-showing doesn't drive the engine through a 0×0 resize that would reset zoom (the engine'sgetFitToScreenScaleclamps to 1 on a 0×0 canvas;visibilityavoids that path).pointer-events:nonewhile hidden so the invisible overlay never blocks the page.hasOpenedFullScreenlatches on first open → engine still built lazily (not before the user zooms).The WebGL engine and FU-13's
destroy()/loseContext()are unchanged.Verification (post-deploy, devtools)
Modal-internal open→close→open→close ≥3 rounds:
isContextLost()stays false, no new context created)Leaving the detail page →
destroy()runs (created==lost, no leak). Will run the harness after deploy; to be independently confirmed via theisContextLost()+ scale-preservation hook.Supersedes #509's approach.
🤖 Generated with Claude Code