Repository navigation
fix(messages): retain image geometry across virtualized remounts - #1271
Conversation
Co-authored-by: murderbot <3754f8729004d95654c46dbab3129e4ab9ef05cc2534e2a3fbfc155983bd637b@buzz.block.builderlab.xyz> Signed-off-by: Bradley Axen <baxen@squareup.com>
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Combined agent review from am and peon: approve at 8316df944b8460ea215a7301628c1798b1b150c3 against 35ae259acb5c95ef86df778d1ae9b2880d3f6e71. No blocking findings.
Both reviews checked stable session ownership, deterministic resolved sources, the 512-entry bound, stale decode cleanup, and MessageRow wiring. Cached dimensions reserve geometry without bypassing the current image node’s decode or pixel readiness.
Optional simplification from peon: AttachmentImage.tsx:43–63 tracks session identity in local decoded state as well as cache ownership. Current live session changes remount the workspace, and cached sessions provide no image source before handoff. Removing the redundant local session bookkeeping could simplify this path. No reachable failure was found; this is not required for approval.
Validation at this head:
- am:
bin/pnpm typecheckpassed. - am: full
vitest run --maxWorkers=4passed: 647 files, 10,034 tests; 2 files and 9 tests skipped. - am: complete
image-scroll.spec.mjspassed in Chromium and WebKit: 14 cases at this head, including retained geometry during actual virtualizer remounts with decoding held pending. - peon: AttachmentImage and MessageRow tests passed, 121 tests. Reverting the AttachmentImage implementation change made both remount regression tests fail while the other three new tests passed.
Independent scores:
| Reviewer | Minimalness | Elegance | Correctness |
|---|---|---|---|
| am | 10/10 | 10/10 | 10/10 |
| peon | 9/10 | 9/10 | 10/10 |
Peon’s minimalness and elegance deductions reflect the optional redundant session bookkeeping above.
Packaged native app, live relay behavior, and the complete browser suite were not exercised. Browser validation covers the complete affected image-scroll file in both engines.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account.
No actionable findings; no changes requested. Source assessment: 9/10 for minimalness, elegance, and correctness.
Reviewed head 8316df944b8460ea215a7301628c1798b1b150c3 against base 35ae259acb5c95ef86df778d1ae9b2880d3f6e71. The five-file change keeps decoded dimensions bounded per session and exact source while preserving each mount’s own pixel-readiness gate. Reviewed stale decode cleanup, source/session changes, failed decode and unavailable-source paths, caller wiring, and regression-test assertions. The existing attachment renderer retains its geometry ownership and interaction/focus behavior. Public PR description and commit message inspected; no attached images or actionable disclosure findings.
Independent review coverage: Rocket (systems/performance/lifecycle), Mantis (correctness/security/concurrency), Groot (product/accessibility), and Star Lord integration. All lanes completed.
Optional documentation note: docs/channels.md:1354 still says “Virtualized remounts start with loading geometry again.” Consider updating that sentence to describe reuse of retained session/source dimensions, with loading geometry as the cache-miss fallback.
Validation is source-only: no tests, builds, app launches, or PR code execution. The reported test results were not independently reproduced; CI, live/native rendering, scroll stability, keyboard/focus behavior, both themes, narrow layouts, enlarged text, performance, and human acceptance remain unverified. This COMMENT review is not approval or merge authorization.
* origin/main: Add native Huddles with call controls and conversation panels (#489) fix(claude-code): keep sessions alive while Claude's background work runs (#1102) Rename Agent setup "Providers" to "Harness defaults" (#1348) feat(plugins): feature core plugins and branded catalog browsing (#1246) Improve AI defaults and add Goose configuration (#1266) feat(sidebar): filter channel sections by unread and activity (#1297) feat(plugins): organize settings with guides and focused controls (#1176) Unify Solo chat menus, rename workspaces, quieter membership notices (#1326) Add identity restore UI (#1101) Add native identity backup recovery (#1107) Back up desktop identities only with saved consent (#1106) Add native identity backup consent hooks (#1137) remove the Channel Usage plugin (#1335) fix(messages): retain image geometry across virtualized remounts (#1271) fix(channels): read metadata for a labeled channel whose roster came first (#1289) fix(channel-members): allow adding members to system channels (#1288) fix(unread): recover dropped thread roots for unread thread activity (#1291) test(voice-notes): fix the draft-card race that turned main red (#1315) ci: check dependency policy with cargo-deny (#1321) Signed-off-by: jtennant <jtennant@squareup.com> # Conflicts: # src/bundled/inbox/InboxDetail.test.tsx # src/bundled/inbox/InboxDetail.tsx
…x-session-preview-live * origin/jt/inbox-page-tabs: (25 commits) refactor(inbox): open conversations through conversation.ui.Tabs fix(inbox): retire a Huddle room's retained tabs and keep their focus test(inbox): stop depending on Channels having loaded a thread's channel fix(inbox): close an opened conversation once its room becomes a Huddle Add native Huddles with call controls and conversation panels (#489) fix(inbox): offer no Archive shortcut while Drafts is shown fix(inbox): keep a restored tab's row selected after leaving Inbox fix(claude-code): keep sessions alive while Claude's background work runs (#1102) Rename Agent setup "Providers" to "Harness defaults" (#1348) feat(plugins): feature core plugins and branded catalog browsing (#1246) Improve AI defaults and add Goose configuration (#1266) feat(sidebar): filter channel sections by unread and activity (#1297) feat(plugins): organize settings with guides and focused controls (#1176) Unify Solo chat menus, rename workspaces, quieter membership notices (#1326) Add identity restore UI (#1101) Add native identity backup recovery (#1107) Back up desktop identities only with saved consent (#1106) Add native identity backup consent hooks (#1137) remove the Channel Usage plugin (#1335) fix(messages): retain image geometry across virtualized remounts (#1271) ...
Problem
When an image has missing or inaccurate size metadata, it forgets the dimensions it decoded once the virtualizer unmounts its row. On remount the image shows the placeholder again (up to 320px), then shrinks after decoding. The list's measurements change, which can unmount and remount nearby rows, so the cycle repeats. Near a scroll boundary this shows up as the placeholder flickering back and forth.
Fix
image-dimensions.ts: a small bounded cache of decoded dimensions, isolated per relay session and source. It holds dimensions only, no pixels and no decode-ready state.AttachmentImagesizes its frame from the cached dimensions on remount. On first load it still sizes the image as it decodes, as before.MessageRowpasses the session scope through.No changes to the scroll controller, the virtualizer, or persistence.
Verification
AttachmentImage.test.tsxandimage-scroll.spec.mjs. Before the fix they fail in Chromium and WebKit (the image remounts at 320px instead of the 10px it already learned); with the fix they pass.pnpm typecheckpasses,vitest runpasses (10,034 passed, 9 skipped), and all 26 tests inimage-scroll.spec.mjspass across Chromium and WebKit.