Copy an image from the media viewer to the clipboard

[zrmarley]

Summary

• Copy the currently displayed image from the media viewer to the system clipboard, as a distinct action from copying a link.
• Scope is browser/dev-broker only; native clipboard support remains deferred to a separate change, since packaged native cannot display protected media today.
• Reuses the already-loaded same-origin authorized <img> from the media viewer, drawing it to a canvas and writing PNG to the clipboard with zero new network requests and no CSP change.
• Clipboard output is PNG-normalized; this does not claim byte-identical copying of the original media bytes.
• Copy and Download are grouped into one toolbar action cell, and success/failure is reported by a stage-scoped notice laid out directly above the viewer's floating image controls.

Privacy and platform notes

• Copying places decrypted protected media on the system clipboard, comparable to the existing Download image action.
• WebKit requires navigator.clipboard.write to be called synchronously inside the user gesture. The helper passes a promise-valued ClipboardItem immediately and lets canvas encoding settle afterward. Two regression tests pin this: one asserts clipboard.write runs before the encode promise settles, and a component-level test asserts it runs synchronously within the click.
• Design reuses the existing IconButton, Phosphor CopyIcon, existing semantic tokens, and the viewer's overlay context, with no new shared tokens or components.

Review feedback addressed

• Notice/toolbar overlap. The notice and toolbar now share a positioned stack, so the notice is laid out relative to the toolbar's actual rendered bounds instead of computed token arithmetic. Copy and Download are grouped into one cell, so a multi-image view no longer wraps Download to a second row at narrow widths.
• Keyboard focus during a pending copy. The Copy button now uses the shared loading contract rather than native disabled, which keeps it focusable, sets aria-busy, and still suppresses duplicate activation. The copyBusy guard is retained.
• Late-completion coverage. Added tests for a resolved copy, a rejected copy, and settling after unmount, each after the selected image changed, so the completion guards are protected.
• Sign-off history. The branch was rebased onto current main: linear history, no merge commit, every commit signed off, and the earlier no-op commit pair dropped.

Test plan

• Focused unit: bin/pnpm exec vitest run src/features/messages/image-copy.test.ts src/features/messages/icon-labels.test.tsx — 22 passed.
• Full unit suite (CI-equivalent command) — 392 files, 4441 tests, 0 failed.
• bin/just iterate — passed.
• tests/browser/messages.spec.mjs in Chromium and WebKit via --no-deps — 7 passed each. Includes new geometry coverage asserting the notice stays above the toolbar for success and failure feedback at wide and narrow (multi-image) widths and with enlarged text, comparing real bounding boxes.
• Fail-then-pass was confirmed for both the overlap fix and the focus fix against their pre-fix behavior.

Browser keyboard-focus regression evidence

• Added tests/browser/image-copy-focus.spec.mjs, a real-browser regression case for keeping keyboard focus on Copy during and after an image copy. This belongs in browser coverage because native disabled-button blur/focus behavior is not modeled by jsdom.
• Fail-then-pass: with Copy temporarily changed to disabled={!canCopyImage || copying} and no loading, the new spec failed in Chromium and WebKit at expect(copy).toBeFocused() while the deferred clipboard write was pending; after restoring ImageReviewStage.tsx, the spec passed in both engines.
• Commands run:
  • bin/pnpm exec playwright test --config tests/browser/playwright.config.mjs tests/browser/image-copy-focus.spec.mjs --project=chromium --project=webkit --no-deps — Chromium and WebKit passed.
  • bin/pnpm exec playwright test --config tests/browser/playwright.config.mjs tests/browser/messages.spec.mjs --project=chromium --project=webkit --no-deps -g "copy" — Chromium and WebKit passed, confirming the fixture change remains backward compatible.
  • bin/pnpm exec biome format --write tests/fixtures/messages.tsx tests/browser/image-copy-focus.spec.mjs and git diff --check — passed.

Deferred / not run

• bin/just scan was not run.
• Native/packaged clipboard behavior is unverified and out of scope here.
• Real Safari clipboard copy was manually verified on 2026-09-25, along with the notice placement; automated clipboard writes cannot be granted reliably in WebKit, so that manual check is the browser-only evidence for the gesture path.

[wesbillman]

Star Lord's automated source review via Wes's account. This is non-blocking COMMENT feedback, not an approval or merge authorization.

Reviewed head 127667185859342e84255a1ee13ddbb4f310b98a against base/merge-base 70687b624446d6df2f8c40822be79ae35d450d57 (five changed files).

Findings

• P2 — Copy feedback overlaps the narrow multi-image toolbar. See the inline comment on Messages.module.css; the new fourth toolbar child creates a second row, but the notice remains positioned for one row.
• P2 — One PR commit lacks the required DCO sign-off. Merge commit 3c872ac142bc7e0ecaec5bae106896b94d89bc20 has no Signed-off-by trailer in either its raw commit or the GitHub Git commit response. AGENTS.md:151–161 requires a sign-off on every commit against the PR base, and this commit is in that range (the other five have trailers). Have the responsible author repair that certification using their verified identity, then verify the hosted DCO Check at the resulting head; do not add another person's certification on their behalf.
• Optional P3 — Public-description hygiene. The Summary still includes an internal tracker URL and internal issue identifiers. Replace these with a self-contained public explanation or public issue reference; the browser-only scope and native deferral can be described without internal identifiers.

Scope and limits

Traced the viewer/gallery caller, existing authorized image source, synchronous clipboard-write timing, busy/selection/unmount handling, shared controls and responsive CSS; inspected the added tests and existing browser journey. The helper reuses the loaded image and adds no fetch or CSP change. Native clipboard support remains explicitly out of scope.

Source only: no tests, builds, app/clipboard operations or PR code execution. Extracted files matched the pinned Git blobs and the base-to-head whitespace check passed. The PR's unit/browser/Safari results and inherited CI-failure explanation are author reports, not independently validated here; current CI was not assessed. Rendered layout, browser/OS clipboard behavior, accessibility announcements and human visual acceptance remain unverified. The linked issue could not be read (access denied), so review intent is based on the PR's stated scope and repository documents.

[wesbillman]

P2 — Anchor the notice above the full toolbar, including its second row. At max-width: 650px, .imageReviewToolbar still has three grid columns, but a multi-image proxy view now has four direct children: switcher, zoom group, Copy and Download (ImageReviewStage.tsx:225–305). Download therefore auto-flows to a second row. This offset counts only one 2rem control: with the current tokens it places the notice bottom 3.25rem above the stage bottom, inside the first control row rather than above the toolbar. Its opaque background and higher z-index obscure the centered zoom controls for the success/error notice duration. This is derived from the pinned markup/CSS, not a rendered observation.

Group Copy/Download into one toolbar action cell to preserve the existing grid, and position the notice relative to the toolbar's full outer height (including padding/border), rather than duplicating a one-row height constant. Check a <=650px multi-image view after success and failure; the current browser journey checks Next/Download visibility but never shows a copy notice.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Reviewed head 127667185859342e84255a1ee13ddbb4f310b98a against base 70687b624446d6df2f8c40822be79ae35d450d57.

Changes required: fix the feedback/toolbar overlap and keyboard-focus loss described inline, and address this publication issue:

[P2] Remove the employee-only tracker reference from the public PR description. The first Summary bullet links to the internal issue tracker. Replace it with a self-contained feature description or a public issue reference; remove internal ticket identifiers from the deferred-native note as well. This is a public-repository privacy issue, not a request to change implementation scope.

Validation: independently exercised real Chromium image-copy success, PNG clipboard readback, zero additional media requests, and permission-denied recovery using the production viewer fixture. Hosted tests for the changed files pass; browser journeys and Rust/tool checks pass. The sole hosted JavaScript failure is inherited from the base and has an upstream fix in #299. CI tested merge snapshot f6e082a4, not the current base combination. Safari success is author-reported, not independently repeated. Human visual acceptance remains deferred in the PR.

Exit criteria: notice stays visibly above the toolbar at desktop/narrow widths and enlarged text, with browser geometry regression coverage; keyboard activation retains focus through pending and settled copy states; PR description contains no employee-only references. No native clipboard expansion is requested.

[wesbillman]

[P2] Anchor copy feedback to the actual toolbar height

The offset assumes the toolbar is one --size-control-sm tall, but it also has padding/borders and can grow or wrap. The new notice therefore renders on top of the zoom controls, not above them. At this head, a real Chromium run of the production messages fixture at 1280×800 measured toolbar y=634–744, notice y=674–700, and slider y=681–697: the notice hides the slider for the feedback duration. At 390px, adding Copy also moves Download into a second grid row while the notice stays inside the toolbar. Enlarged text further changes its height.

Position the notice relative to the toolbar's actual bounds (for example, a shared positioned stack), rather than an assumed control height. Add Chromium/WebKit geometry coverage that triggers success/failure feedback and asserts no overlap with the toolbar at wide/narrow widths and enlarged text. The pre-existing reset-label wrapping is not this finding and need not become part of this fix.

[wesbillman]

[P2] Preserve keyboard focus while the clipboard write is pending

Setting native disabled on the focused Copy button blurs it in Chromium. Independently reproduced in the production messages fixture using the real clipboard: focus Copy, press Enter, wait for Image copied; document.activeElement changes from the Copy BUTTON to BODY and never returns. Keyboard users lose their focus indicator and cannot immediately repeat the action with Enter. The same transition happens while a permission prompt/write is pending.

Use the shared busy-state contract, e.g. disabled={!canCopyImage} plus loading={copying}, or preserve focus equivalently. Button already keeps loading controls focusable, suppresses duplicate activation, and sets aria-busy. Retain the existing busy guard and add a keyboard regression that checks focus during and after the operation.

[kalvinnchau]

🤖 One non-blocking regression-coverage finding below. Existing toolbar-overlap feedback is not duplicated. Full frontend validation was not green (4051 passed, 12 failed, 1 skipped; failure baseline unestablished). Isolated Chromium/WebKit helper writes succeeded, but that does not validate the rendered viewer, clipboard contents, or native WebViews. Not approval.

[kalvinnchau]

🤖 [P3] Protect pending-copy retirement with a regression test

The current switch test resolves copying before changing images, so it does not protect this late-completion guard. Removing both selected-URL completion guards and the mounted check in showCopyNotice still leaves all 12 icon-labels.test.tsx tests passing. A future regression could therefore show success/failure feedback on image B after a copy initiated on A.

Hold clipboard.write pending, switch the selected image, then settle it and assert that no status/alert appears for the retired attempt; cover rejection and unmount as well. This is a demonstrated coverage gap, not a claim that the current guard fails.

[wesbillman]

Star Lord's automated source follow-up via Wes's account. Non-blocking COMMENT feedback only—not an approval or merge authorization.

Reviewed head 65a1e9ae189e365c6b0febb9f895b0924aaf845f against base/merge-base 70687b624446d6df2f8c40822be79ae35d450d57, focusing on the three-file delta since 127667185859342e84255a1ee13ddbb4f310b98a and the existing review exit criteria.

Prior fixes remain incomplete. The new jsdom cases cover synchronous clipboard dispatch and rejection of a late success notice after selection changes. They do not establish toolbar geometry or keyboard-focus retention.

• P2 — Notice placement still assumes one toolbar row. The inline comment explains why adding padding to the fixed offset does not fix the narrow multi-image case. Anchor feedback to the toolbar's actual bounds, preserving the existing wide/narrow and enlarged-text acceptance criteria.
• P2 — Keyboard-focus fix is unchanged: src/features/messages/ImageReviewStage.tsx:294 still passes disabled={!canCopyImage || copying}. Button.tsx:44–47 preserves disabled-state focus for loading, not this explicit disabled path. Reuse disabled={!canCopyImage} with loading={copying} (keeping the busy guard), and check keyboard focus during and after a held write. This is follow-up to the existing focus finding, not a newly reproduced browser result.
• Existing DCO item remains: the current seven-commit PR range still includes 3c872ac142bc7e0ecaec5bae106896b94d89bc20 without Signed-off-by; its Git commit response confirms the omission. Have the responsible author repair their certification and verify the resulting DCO check, as required by the pinned AGENTS.md:151–161.
• Existing publication item remains: the PR description still contains the employee-only tracker reference and internal identifiers noted in the prior review. Replace them with a self-contained public description without changing the browser-only/native-deferred scope.

Source-only follow-up: no tests, builds, app launches, clipboard operations, CI reads, or PR-code execution. Immutable source extracts were Git-blob/SHA256 verified; no working-tree source inputs. The browser journey inspected at tests/browser/messages.spec.mjs checks Next/Download visibility but does not trigger Copy; no browser cases were added by this follow-up. Current rendered geometry, browser/OS clipboard behavior, accessibility announcements, CI, and human visual acceptance remain unverified. The linked internal issue was not independently read in this cycle; intent comes from the stated PR scope, repository/vision docs, and prior review criteria. No new unrelated findings or native scope expansion.

[wesbillman]

P2 — The revised offset still leaves feedback inside a two-row toolbar. At max-width: 650px, the toolbar remains a three-column grid (Messages.module.css:892–901), while a multi-image proxy view supplies four direct children: switcher, zoom group, Copy, Download (ImageReviewStage.tsx:227–305). Download therefore occupies a second row. This calculation adds padding but still counts only one --size-control-sm row; with the pinned tokens it puts the notice bottom at 4.25rem from the stage bottom, inside the first control row, rather than above the whole toolbar. Enlarged text can increase the toolbar height further.

Keep the prior fix at its owning layout: position the notice above the toolbar's actual bounds (for example, in a shared positioned stack), not another fixed row-height estimate. Preserve the agreed browser regression for success/failure, wide/narrow multi-image views and enlarged text. This is source-derived follow-up to the existing overlap finding, not a new rendered measurement.

[wesbillman]

Star Lord's automated source follow-up via Wes's account. Non-blocking COMMENT feedback only—not an approval or merge authorization.

Reviewed head 738ffc972c04871fc379661771cfb82420ff932d against base 70687b624446d6df2f8c40822be79ae35d450d57, focusing on the five-file delta since 65a1e9ae189e365c6b0febb9f895b0924aaf845f and the existing review exit criteria.

Addressed in source

• The notice and toolbar now share a bottom-anchored flex-column stack; notice placement no longer estimates a control-row height. Copy/Download share one action cell in the narrow three-column grid. The new Chromium/WebKit journey checks success/error notice geometry at 1280px/640px, including enlarged root text.
• Copy now uses disabled={!canCopyImage} plus loading={copying}, retaining the synchronous busy guard and reusing the shared focusable loading contract. I am not carrying forward the prior production-code focus defect as unchanged.
• The public description is now self-contained, with the internal tracker references removed; browser-only scope and native deferral remain explicit.

Remaining prior-review items

• P2 — Complete the keyboard-focus regression at the browser layer. The inline comment identifies why the new jsdom assertion does not establish the previously requested pending-and-settled keyboard behavior. This is an outstanding regression-coverage criterion, not a newly observed runtime failure.
• P2 — The merge commit still lacks the required DCO trailer. The complete eight-commit PR range still contains 3c872ac142bc7e0ecaec5bae106896b94d89bc20 without Signed-off-by; its Git commit response independently confirms the omission. The other seven commits have trailers. AGENTS.md:151–161 requires certification for every commit against the PR base. Have the responsible contributor repair only a certification they can legitimately make, preserving actual authorship, then re-audit the range. The hosted DCO Check is currently successful; this finding is about the explicit repository rule, not a claim that CI reports failure.

Evidence and limits

Source-only: 26 app blobs and two vision-document blobs hash-verified; no dirty checkout inputs, local tests, builds, app launches, clipboard operations or PR-code execution. No additional actionable production defect found in the fix delta.

One read-only check snapshot associated with this head showed JavaScript and DCO successful, browser journeys and Rust/tool integration still running, and Windows native validation skipped. This is not a completed CI verdict or proof of the browser acceptance criteria. The PR's local browser/Safari passes remain author-reported. Rendered layout, native keyboard focus, real clipboard behavior, announcements and human visual acceptance were not independently exercised here; the new geometry fixture mocks clipboard behavior.

[wesbillman]

P2 — Keep the existing keyboard-focus exit criterion in a real-browser regression. The production switch to loading={copying} is the requested repair, but this new assertion is in a jsdom file (:1), calls button.focus() / fireEvent.click() rather than keyboard activation, and checks focus only while pending. After settlement it checks the notice and aria-busy, not focus. jsdom cannot establish the native blur behavior that caused the prior finding.

The new browser case only clicks Copy and checks geometry; messagesFixture.imageClipboard resolves/rejects immediately, so it cannot hold the pending state either. Extend that existing fixture with a deferred clipboard-write gate: keyboard-activate Copy, observe write start, assert focus while pending, release in finally, and assert focus after settlement in Chromium and WebKit. Keep the unit test for the busy guard. This closes the already-agreed regression requirement; it is not evidence that the repaired production code still loses focus.

[wesbillman]

Star Lord automated source follow-up (via Wes’s account)

No new actionable findings. The two remaining prior-review items are addressed. This is non-blocking COMMENT feedback, not approval or merge authorization.

• Head: b5f06b24bd6ab561abb25873fc5b04d32792cfac
• API base: 1d19153276b93ef733fe5e8ead888fc528fc00e3
• Merge base: 85d6bf82c54d1c8d930d58444597a1fe31cc8975

Scope is the existing exit criteria from the previous review, the new browser regression/fixture, and relevant rebase integration—not a new audit of unrelated main-branch changes.

Prior items addressed

• Browser keyboard-focus regression: tests/browser/image-copy-focus.spec.mjs:68–110 exercises the actual viewer and shared Copy control using Tab/Enter. It waits for a recorded clipboard write, checks focus and busy state while the explicit gate remains held, checks duplicate activation, releases in finally, then checks focus and cleared busy state after both success and failure. tests/fixtures/messages.tsx:253–275 supplies the deferred external clipboard boundary; existing immediate-mode geometry coverage remains intact. The new case runs in both configured browser engines. This closes the requested browser-layer coverage gap; it does not pretend the mocked clipboard is an OS clipboard test.
• DCO history: the fully paginated current PR range contains six commits. Their Git commit responses all contain Signed-off-by trailers; the previously identified unsigned merge commit is absent. The hosted DCO Check is successful.

The earlier production repairs remain present: Copy uses the shared focusable loading contract, and the notice/toolbar share a bottom-anchored stack with Copy/Download grouped in one action cell. The public description remains self-contained and keeps native clipboard support deferred. No additional production defect was found in the scoped follow-up.

Evidence and limitations

Source-only: 20 extracted app blobs hash-verified, clean-checkout provenance recorded, no dirty source inputs. No tests/builds, app launches, clipboard operations or PR-code execution. The added journey’s browser-specific justification and Chromium/WebKit fail-before/pass-after evidence are documented by the author; I did not independently execute them. One new browser journey (two engine cases), with existing coverage retained.

One read-only hosted check snapshot associated with this head showed CI required, JavaScript, all six browser shards, Rust/tool integration and DCO successful; Windows native validation was skipped. This is check-status evidence, not a live clipboard or visual-acceptance observation. Native/packaged clipboard behavior remains out of scope; Safari copy is author-reported. Real OS clipboard contents, current rendered geometry, assistive announcements and fresh human acceptance were not independently exercised here.