Repository navigation
Open
Conversation
matt2e
marked this pull request as ready for review
November 4, 2026 22:42
kalvinnchau
reviewed
Nov 4, 2026
kalvinnchau
left a comment
kalvinnchau
left a comment
Contributor
There was a problem hiding this comment.
🤖 One image-state regression found: a sibling failure can remove an already-loaded viewer image.
kalvinnchau
reviewed
Nov 5, 2026
kalvinnchau
left a comment
kalvinnchau
left a comment
Contributor
There was a problem hiding this comment.
🤖 The direct loaded-viewer regression is repaired. One narrower back-navigation preservation gap remains, reproduced in a component test; real-browser timing is unverified.
kalvinnchau
reviewed
Nov 5, 2026
kalvinnchau
left a comment
kalvinnchau
left a comment
Contributor
There was a problem hiding this comment.
🤖 [P2] Supply media responses in the thread-window fixture — tests/browser/thread-window.spec.mjs:277
The fixture server lacks the /api/relay/media handler used by the other image browser specs. Its two images now render the new error statuses, so the zero-status assertion fails with Expected: 0, Received: 2. Exact-head Chromium job 127024133184 and WebKit job 127024133188 fail; the Chromium case also reproduces locally with npx playwright test --config tests/browser/playwright.config.mjs tests/browser/thread-window.spec.mjs:252 --project=chromium --retries=0. Add the media middleware to this spec's createServer rather than weakening the assertion. The unchanged fixture/spec passed on base b95f9df.
The prior after-return sibling-error case is repaired. One additional departing-image error gap is detailed inline.
… the viewer A message image whose original failed to load stayed blurred with no way to recover, and opening it showed a broken image in the viewer. This reverses the earlier "failure retains the preview" rule for load errors. A failed original now keeps its reserved frame and blurred preview under a scrim with "Couldn't load image" and Retry. One ImageLoadError component serves the row and the viewer, and a container query on its own size picks the layout. Thresholds are in rem, so larger text steps down a layout instead of overflowing: - under 5.5rem wide or 4rem tall: a round Retry named and titled "Couldn't load image. Retry" - under 10rem by 6rem: icon and a compact Retry, with the status kept for screen readers - otherwise: icon, "Couldn't load image" and Retry Failed frames grow to at least 2.5rem so the round Retry fits. The failed frame is no longer the viewer link, so Retry is not nested in a link; a "+N" tile stays the gallery link. Retry stops propagation, is busy until the fresh request settles, and ignores repeated presses. Another failure returns to the error. Success restores the link, and focus that was on Retry moves to it. Failures are shared by media source across mounted views, so the row and the viewer agree and Retry in either recovers both. A view whose original already decoded keeps its pixels. A failure is forgotten once no view of its source is mounted, so remounting requests the original again, as before. The viewer lays out the same recovery above its toolbar, disables zoom, and hides copy, download and open until the image loads. Decode rejection still keeps the preview. There is no automatic retry or cache-busting. Signed-off-by: mtoohey <mtoohey@squareup.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…und Retry Follow-up to b89d13a after review. The viewer withheld its image only while failed, so during a retry the image took the stage's first grid row. The size-contained error then collapsed in the auto row, dropped to the round layout under the toolbar, and hid the focused Retry. The image and the error now share grid area 1 / 1 with the toolbar in row 2, so Retry keeps its place, layout and focus while busy. A failure switched the row frame between a span and the link, so React remounted it. The blurhash canvas went blank until it repainted, and the image Retry loaded was discarded and requested again. The frame is now one stable anchor that drops href, label and click handling while failed. Without href it is no link, so Retry is not nested in one. A successful Retry shows the image it loaded, with one request. Focus follows the frame's control both ways. A focused link that fails hands focus to Retry, recorded in ref cleanup before React edits the frame. In the viewer, focus that a failure or recovery removes or disables moves to Retry or the stage. Closing a viewer whose opener can no longer take focus focuses the opener's first available control, so a row that failed meanwhile gets its Retry. Images in a group name their position, e.g. "Retry image 2 of 4" and "Couldn't load image 2 of 4. Retry", in the row and the viewer gallery. The modal already hides the row's status while the viewer is open, so a viewer Retry is announced once. A browser test now asserts this. Unit tests now check: - the same frame, canvas and image through failure, retry and success - the focus handoffs - the positional names A new Chromium and WebKit spec covers: - a Retry that succeeds with one more request and focus on the link - a delayed viewer Retry that keeps focus inside the stage above the toolbar, then closing to the row's Retry Unchanged: - a decode failure may still be served from HTTP cache - the latest event wins between the row and the viewer - failed frames still grow to at least 2.5rem Signed-off-by: mtoohey <mtoohey@squareup.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… viewer toolbar still Follow-up to 557a78c. A frame that loaded and then failed was still a link to Chromium. React sets `onclick = noop` on any element with an onClick prop (Safari's trapClickOnNonInteractiveElement workaround) and never clears it when the prop goes away. Chromium exposes an anchor with a click listener as a link even without href, so screen readers heard a link named "Retry image" wrapping Retry. A frame that mounted already failed was generic. The frame now clears the leftover handler while failed and takes role="none", so it is never a link whichever way it failed. The viewer removed Copy, Download and Open while its image was failed. The centred toolbar narrowed and Next moved about 44px, so a repeated Next press could land on the counter, and a successful Retry moved it back. Those actions now stay in place, disabled. Copy and native Download are disabled buttons. The Download and Open links are aria-disabled and drop href. The toolbar keeps its width and centring. Focus on an action that becomes aria-disabled moves to Retry, as it already did for disabled buttons. MediaDownloadAction gains a `disabled` prop for this. Zoom and gallery buttons faded into their disabled colours over the buttons' 150ms colour transition, so they looked usable for a moment after the gallery reached a failed image. In the viewer toolbar, a control's disabled state now applies at once. Tests: - A unit test covers the click trap and role while failed, when mounted already failed, and after recovery. - Unit tests cover the disabled-in-place actions, including native Download and Open in browser, and focus moving to Retry from an aria-disabled action. - A Chromium-only browser test reads the accessibility tree over CDP and checks that no link wraps Retry while failed and that the link returns on recovery. - A Chromium and WebKit test checks that the toolbar, the navigation, zoom, copy and download boxes match between a failed and a loaded image, and that zoom greys out with no running transition. Signed-off-by: mtoohey <mtoohey@squareup.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… fails Follow-up to 3e80fa7 after review. The viewer read only the shared load state for its source. After it had loaded an image, an error from a row showing the same source made it drop its pixels, show "Couldn't load image" and disable zoom, copy, download and open. That broke the rule that a view whose original already decoded keeps it. The viewer now remembers that its current source loaded, as the row does with its decoded dimensions. While it has, it ignores a failed or retrying shared state for its image, the error overlay, the focus handoff and the toolbar. Its own load error clears this, so the viewer's own failure still shows. Navigating to another image, or back to this one, starts over. Tests: - A new test mirrors the row one: a loaded viewer keeps its image, no status and enabled zoom, copy and download while a row with the same source shows its error. - The viewer-recovery focus test now reopens the viewer over the failed row, since a viewer that already loaded no longer offers Retry there. Signed-off-by: mtoohey <mtoohey@squareup.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…arting image Follow-up to a31765d after review. Navigating resets the viewer's record that its image loaded. Returning to an image while its departing copy is still mounted (during the slide, or while the next image loads) makes React reuse that element. Its source is unchanged, so no new load event fires and the viewer never marked it ready again. A later failure from a row showing the same source then removed the visible image, showed "Couldn't load image" and disabled the toolbar. After a source change, the viewer now also checks the image it actually rendered. If that image is complete and has a natural width, it counts as loaded at once. A broken image is also complete, but it has no natural width, so it doesn't count. The viewer's own load error still clears readiness and shows the error. Tests: - A regression in image-load.test.tsx holds the slide open, returns to the first image both after the next image arrives and while it is still loading, checks that the same element is reused, and then fails the row. The viewer keeps its image, shows no status and leaves zoom enabled. Both cases fail without the fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: mtoohey <mtoohey@squareup.com>
… departing Follow-up to 07f5ede after review. Opening a slow image, moving on before it settled and returning while its departing copy was still mounted reused that element. If the request had failed in the meantime, the departing copy had no error handler, and the readiness check only counted a complete image with a natural width. So the viewer showed a broken image with zoom, copy, download and open enabled, and offered no Retry. When the viewer returns to an element it already showed and finds it complete with no natural width, it now treats that as its own load failure, as its onError does: it clears readiness, marks the source failed, and shows "Couldn't load image" and Retry with the actions disabled. A newly rendered element is left to report its own load or error, so a cached image without intrinsic size isn't mistaken for a broken one. Tests: - A regression in image-load.test.tsx holds the slide open, fails the first image while it departs and returns before the next one loads. The viewer shows the error with Retry and disabled zoom and copy; Retry requests the image again, and a later error of its own still shows. It fails without the fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: mtoohey <mtoohey@squareup.com>
The messages fixture's thread carries a two-image root. thread-window.spec served no media, so both images failed. A failed image now shows a "Couldn't load image" status, and "newest window positions immediately" counts statuses in thread history to check that nothing is loading, so it found 2 and failed in both engines. Move messages.spec's fixture media plugin into fixture-media.mjs and use it for every thread-window server, so only a test that marks an image missing sees a failed one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.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
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.
Summary
When a message image's original failed to load, the row kept showing the blurred preview with no way to recover. Opening it showed a broken image in the viewer.
A failed image now keeps its frame and blurred preview, with a "Couldn't load image" message and a Retry button on top. The overlay chooses one of three layouts based on its own size:
The viewer shows the same error and Retry. Zoom, gallery, copy, download and open controls stay where they are but are disabled, so the toolbar doesn't shift. The row and the viewer share retry state: a Retry in either one recovers both. A successful Retry makes exactly one extra request.
Judgment calls
Testing
pnpm lintandpnpm check(biome, typecheck, design checks)image-failure,image-scroll,image-strip,image-zoom,image-copy-focus,gallery-theme,media-controls,message-gallery-polishandmessages.Before / after
staged-image-f87fa1b3-7530-4056-8920-e14863b885c7.mp4
After
🤖 Generated with Claude Code