Keep image review downloads and external opens distinct by zrmarley · Pull Request #144 · block/buzz-app

zrmarley · 2026-09-22T20:14:36Z

Summary

Keeps proxied relay image review sources on the existing Download image path.
Routes direct HTTPS image review sources through the shared host onOpenLink plumbing and labels them Open image in browser instead of presenting them as downloads.
Extracts the existing attachment source policy so file attachments and the image review viewer share the same proxy-vs-external decision.
Corrects the browser fixture so its synthetic SVG image still renders through the app's local media proxy instead of being treated as a direct external image.

Linear: https://linear.app/squareup/issue/BOT-1930/view-images-and-galleries

Source-derived risk

This is a narrow policy/wiring fix, not full gallery validation.
Download image for relay proxy media and external Open image in browser intentionally remain distinct shared-policy cases.
ChannelsPage.openLink can return false for ordinary HTTPS by design, so this PR does not claim verified native system-browser external opener behavior. The _blank fallback attributes are established; real browser/package navigation remains pending.
BOT-1930 remains incomplete for package protected read/native saving work tracked with BOT-2016/BOT-1884. Copy image work is separate in BOT-2022.

Design notes

Read ~/goose artifacts/buzz-design-system-pack/AGENTS.md, DESIGN.md, and components/registry.json; IconButton is listed with status proposed.
Reuses the existing repo IconButton and the Phosphor gateway export for the existing ArrowSquareOutIcon.
No new visual tokens, styles, media files, recordings, assets, media/binary fixtures, downloads, or real conversation captures were added. tests/fixtures/messages.tsx changed only as code fixture data.
The hosted browser failure produced CI's ordinary synthetic Playwright failure screenshots/traces for this fixture. The fixture correction does not commit or download those artifacts; synthetic screenshots are now explicitly approved for this CI context.
This follows the existing repository icon policy; the registry/IconButton packaging design conflict remains noted, not changed here.

CI root cause and fixture fix

Prior hosted CI on 76bd45a failed only in Browser journeys (chromium, 1/2) and Browser journeys (webkit, 1/2) for tests/browser/messages.spec.mjs:8.
Root cause: the fixture's image attachment source was changed from the local proxy path to an external data SVG. That made the narrow-control browser journey look for the prior proxied attachment link that no longer existed.
Fix: ca3c911 keeps the fixture synthetic SVG data in memory and serves it from the existing browser-test local media proxy middleware, preserving the app path under test without fetching remote media or weakening assertions.

Tests

bin/pnpm typecheck
bin/pnpm exec biome check --error-on-warnings src/bundled/channels/ChannelsPage.tsx src/features/messages/FileAttachment.tsx src/features/messages/ImageReviewStage.tsx src/features/messages/MediaReviewViewer.tsx src/features/messages/attachment-source.ts src/features/messages/MediaReviewViewer.test.tsx src/features/messages/icon-labels.test.tsx tests/fixtures/messages.tsx
bin/pnpm exec vitest run src/features/messages/MediaReviewViewer.test.tsx src/features/messages/icon-labels.test.tsx — 2 files, 10 tests
bin/pnpm design:check
Push hook on ca3c911: TypeScript plus related Vitest selection — 17 files, 220 tests; design-system guards passed
Slopaganda Panda review: PASS, 2/10 smell, no blockers

2026-09-23 final verification at head `4ee7c25`

Hosted CI is green at exact head 4ee7c254b33db48bf3a672fb4bc703d374757f82: required CI required passed, all non-skipped hosted checks passed, and both Chromium/WebKit browser journey shards passed. Windows native validation was skipped as expected.
Local browser journeys were not rerun after the rebase/fixture correction; browser coverage for the final head is from hosted CI.
User manual dev-desktop confirmation on Sept. 23: external Markdown embedded-image viewer Open image in browser opens the browser and leaves Buzz open; relay-uploaded image Download image saves an actual image and opens it.
The user-reported manual result was not independently byte/hash checked here, and installed-package/native behavior remains unvalidated.
The existing inline bare-URL link behavior that does not use the image viewer remains unchanged by this PR.
No full just scan was run.

2026-09-23 main integration

Rebasing onto origin/main 60ed428 picked up the upstream audio attachment work and FileAttachment helper export.
Conflict resolution kept the PR's shared attachment-source helper as the single owner, left FileAttachment importing it directly, and changed MessageRow to import isProxySource from attachment-source while preserving the component import from FileAttachment.
No behavior beyond that mechanical helper/caller integration was added.

2026-09-23 rebase validation

bin/pnpm typecheck
bin/pnpm exec biome check --error-on-warnings src/features/messages/FileAttachment.tsx src/features/messages/MessageRow.tsx src/features/messages/attachment-source.ts src/features/messages/ImageReviewStage.tsx src/features/messages/MediaReviewViewer.tsx src/features/messages/MediaReviewViewer.test.tsx src/features/messages/icon-labels.test.tsx src/features/messages/AudioAttachment.tsx tests/fixtures/messages.tsx
bin/pnpm exec vitest run src/features/messages/MediaReviewViewer.test.tsx src/features/messages/icon-labels.test.tsx src/features/messages/MessageRow.test.tsx src/features/messages/AudioAttachment.test.tsx — 4 files, 73 tests
bin/pnpm design:check
Push hook on 4ee7c25: TypeScript plus related Vitest selection — 17 files, 223 tests; design-system guards passed

2026-09-23 final review

Slopaganda Panda general code review: PASS, no blockers, reviewed the full current diff including the browser fixture correction and the mechanical origin/main/PR Play audio and voice-note attachments #133 helper integration.
PR is ready for human review; no merge or review approval is requested from the agent.

zrmarley · 2026-09-23T13:06:39Z

🤖 Pushed fixture correction ca3c911. The prior hosted failures were the same messages media-review journey in Chromium/WebKit after the synthetic SVG fixture stopped going through the local media proxy; ca3c911 keeps the SVG in-memory but serves it from the existing local proxy middleware, with no remote fetches, real conversation captures, downloaded artifacts, or assertion weakening. Local push hook passed; fresh CI is pending on the new head. PR remains draft.

Signed-off-by: Zach Marley <zmarley@squareup.com>

zrmarley · 2026-09-23T13:33:01Z

🤖 Ready for human review at head 4ee7c254b33db48bf3a672fb4bc703d374757f82. Hosted CI is green (CI required passed; all non-skipped checks passed; Chromium/WebKit browser journeys passed; Windows native validation skipped as expected). PR body records Sept. 23 user manual dev-desktop evidence: external Markdown embedded-image viewer Open image in browser opens the browser and leaves Buzz open; relay-uploaded image Download image saves an actual image and opens it. Remaining gaps: user-reported manual result was not independently byte/hash checked here, installed-package/native behavior is not validated, local browser journeys were not rerun, and no full just scan was run.

wesbillman

Pinky here, reviewing on Wes’s behalf.

No blocking findings at 4ee7c25; one non-blocking fixture caller omission noted inline. The shared helpers retain their existing behavior, and the viewer preserves proxy downloads versus HTTPS host/fallback opens.

Hosted CI passed: 2,223 Vitest tests and all Chromium/WebKit journey shards, including all four messages.spec.mjs cases in both engines. CI merge 9222c57 has the same tree as the reviewed head. No local suites or native launches performed. Packaged-native saving/opening remains unvalidated; the PR’s reported dev-desktop check is not package acceptance.

Approval and merge remain with the human reviewers.

wesbillman · 2026-09-23T23:15:10Z

  messageId: string;
  initialTime: number;
  restoreFocus?: RefObject<HTMLElement | null>;
+  onOpenLink(url: string): boolean;


Pinky here, reviewing on Wes’s behalf.

P3 — update the remaining fixture caller. tests/fixtures/dialog-gallery/Scene.tsx:67–76 still mounts MediaReviewViewer without this newly required prop. That fixture is outside tsconfig.json’s include set, so the green typecheck does not cover this omission. Its data-URL source currently hides the external-open action, making this non-blocking rather than a current click failure. Please pass onOpenLink={() => false} there too, keeping the existing caller consistent with the component contract.

wesbillman

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

Review clear: no actionable production blocker found. Reviewed head 4ee7c254b33db48bf3a672fb4bc703d374757f82 against base 60ed428b016ea67cfadbff03c56b812990e170a9, including independent image-viewer interaction review.

The shared attachment policy retains proxied downloads and separates direct HTTPS host-opener/browser fallback actions. Checked production wiring, source producers, file/audio helper consumers, gallery selection, unavailable sources and modified-click behavior. The unchanged exact-session surface does not gain a viewer entry point here; expanding that feature is outside this narrow fix.

Validation: read-only source review on Blox; no PR code or tests executed. Exact-head hosted CI passed JavaScript, Rust/tool integration, browser measurements and all four Chromium/WebKit journey shards. Windows native validation was skipped. Direct-HTTPS fallback is asserted at the component layer, not exercised by the image browser fixture. Add a representative browser case as follow-up coverage; no concrete broken interaction was established, and real mounted-component tests already exercise the anchor/handler composition. Packaged-native saving/opening remains unvalidated. The author-reported dev-desktop check is not independent package acceptance.

Non-blocking: the dialog-gallery fixture caller still omits the new required onOpenLink prop, already noted in the existing review. Update that fixture alongside the component contract; this does not block the production wiring.

COMMENTED, not approval. Human/required maintainer approval remains before merge.

…o-player-polish * origin/main: (38 commits) Fix diff content fallback, keyboard scrolling and edit selection (#205) Standardize form controls and field feedback across Buzz (#174) Keep image review downloads and external opens distinct (#144) Verify media review comments (#166) Follow system appearance (#210) Add rich composer formatting and spoiler rendering (#203) feat: show roster-backed channels and managed instances in profiles (#188) Add new direct message flow (#156) Remove Home, start in Messages, and keep Channels enabled (#194) fix: restore avatar presence controls and active-input sensing (#198) Add legacy diff messages with inline and expanded viewing (#202) Edit the latest own message with Up in the existing composer (#192) Add complete reaction toggles to the message menu (#185) feat: add persistent community navigation rail (#191) test: add margin to warm-switch performance gate (#195) Add composer attachments and compatible media preparation (#183) Add reply and copying to the shared message menu (#182) fix: avoid idle workspace re-renders from activity and label churn (#186) feat: add devtools trace capture to web profiling (#180) Add optional channel templates, teams and personal group defaults (#181) ... Signed-off-by: Zach Marley <zmarley@squareup.com>

…-content-compat * origin/main: (38 commits) Fix diff content fallback, keyboard scrolling and edit selection (#205) Standardize form controls and field feedback across Buzz (#174) Keep image review downloads and external opens distinct (#144) Verify media review comments (#166) Follow system appearance (#210) Add rich composer formatting and spoiler rendering (#203) feat: show roster-backed channels and managed instances in profiles (#188) Add new direct message flow (#156) Remove Home, start in Messages, and keep Channels enabled (#194) fix: restore avatar presence controls and active-input sensing (#198) Add legacy diff messages with inline and expanded viewing (#202) Edit the latest own message with Up in the existing composer (#192) Add complete reaction toggles to the message menu (#185) feat: add persistent community navigation rail (#191) test: add margin to warm-switch performance gate (#195) Add composer attachments and compatible media preparation (#183) Add reply and copying to the shared message menu (#182) fix: avoid idle workspace re-renders from activity and label churn (#186) feat: add devtools trace capture to web profiling (#180) Add optional channel templates, teams and personal group defaults (#181) ... Signed-off-by: Zach Marley <zmarley@squareup.com> # Conflicts: # docs/channels.md

…rs-support * origin/main: Add Messages design gallery and tighten message layout (#158) Fix diff content fallback, keyboard scrolling and edit selection (#205) Standardize form controls and field feedback across Buzz (#174) Keep image review downloads and external opens distinct (#144) Verify media review comments (#166) Follow system appearance (#210) Add rich composer formatting and spoiler rendering (#203) feat: show roster-backed channels and managed instances in profiles (#188) Add new direct message flow (#156) Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz>

zrmarley added 2 commits September 23, 2026 09:09

Fix image review save and open policy

b2fc401

Signed-off-by: Zach Marley <zmarley@squareup.com>

Fix messages media fixture proxy source

4ee7c25

Signed-off-by: Zach Marley <zmarley@squareup.com>

zrmarley force-pushed the zmarley/bot-1930-image-save-policy branch from ca3c911 to 4ee7c25 Compare September 23, 2026 13:10

zrmarley marked this pull request as ready for review September 23, 2026 13:33

zrmarley requested review from a team, comp615 and wesbillman as code owners September 23, 2026 13:33

wesbillman mentioned this pull request Sep 23, 2026

Add composer attachments and compatible media preparation #183

Merged

wesbillman reviewed Sep 23, 2026

View reviewed changes

wesbillman reviewed Sep 24, 2026

View reviewed changes

wesbillman approved these changes Sep 24, 2026

View reviewed changes

wesbillman merged commit ec8ea07 into main Sep 24, 2026
12 checks passed

wesbillman deleted the zmarley/bot-1930-image-save-policy branch September 24, 2026 13:20

kalvinnchau mentioned this pull request Sep 24, 2026

fix: repair vitest baseline broken by composer and media review merges #215

Closed

Navigation Menu

Keep image review downloads and external opens distinct - #144

Keep image review downloads and external opens distinct#144
wesbillman merged 2 commits into
mainfrom
zmarley/bot-1930-image-save-policy

zrmarley commented Sep 22, 2026 •

edited

Loading

Uh oh!

zrmarley commented Sep 23, 2026

Uh oh!

zrmarley commented Sep 23, 2026

Uh oh!

wesbillman left a comment

Uh oh!

wesbillman Sep 23, 2026

Uh oh!

wesbillman left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

zrmarley commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Source-derived risk

Design notes

CI root cause and fixture fix

Tests

2026-09-23 final verification at head 4ee7c25

2026-09-23 main integration

2026-09-23 rebase validation

2026-09-23 final review

Uh oh!

zrmarley commented Sep 23, 2026

Uh oh!

zrmarley commented Sep 23, 2026

Uh oh!

wesbillman left a comment

Choose a reason for hiding this comment

Uh oh!

wesbillman Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

wesbillman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zrmarley commented Sep 22, 2026 •

edited

Loading

2026-09-23 final verification at head `4ee7c25`