Repository navigation
Unify file attachment cards and file-type artwork - #1220
Conversation
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
|
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Three non-blocking findings below: composer tile framing, semantic color roles, and missing behavioral assertions. No actionable security findings. Approval withheld pending these fixes and browser validation: the prescribed live flow required Keychain access, and the full unit suite did not complete.
| aria-label={`Preview ${item.file.name}`} | ||
| onClick={(event) => preview(event.currentTarget)} | ||
| icon={<FileTextIcon size={20} />} | ||
| icon={<FileTypeIcon name={item.file.name} mime={item.file.type} />} |
There was a problem hiding this comment.
🤖 [P3] Avoid painting a second surface around the file tile
The new 36×36 tile sits inside the existing solid 48×48 IconButton. In the Chromium composer fixture (relay-composer.html?attachments), this produces a 6px black frame around the white text/PDF tile in light mode; dark mode also retains the frame. Hover/pressed styling is confined to that outer ring, unlike the message card's single contained tile. Use a non-painting button variant or make the tile fill the slot. This was measured in the fixture; live-app confirmation remains outstanding.
| .icon[data-kind="code"] { | ||
| background: var(--affordance-selected); | ||
| color: var(--text-link); | ||
| } |
There was a problem hiding this comment.
🤖 [P3] Give file categories their own measured color roles
Sheet/archive/code artwork reuses success, warning, selection and link roles. In particular, text-link on affordance-selected is not a registered contrast pair, so the contrast guard does not measure this combination. Retuning those interaction/status roles will also silently repaint file categories. DESIGN.md's categorical-role guidance applies here as it does to the new PDF pair: register file-artwork pairs for these categories, or keep the glyphs without category hues.
| await expect( | ||
| page.locator('[data-message-id="file-unavailable"] [data-kind="pdf"]'), | ||
| ).toBeVisible(); | ||
| } |
There was a problem hiding this comment.
🤖 [P3] Assert the advertised artwork behavior
This test checks only visibility of text/PDF kinds. Neither the light/dark loop nor its assertions verifies computed colors or pointer/focus behavior, so it passes if the fixed PDF pairing regresses. Sheet/archive/code classification and MIME-versus-extension precedence also lack coverage. Add the classification matrix at the unit/component layer and assert the fixed PDF computed colors in both browser themes so these tests protect the stated behavior.
kalvinnchau
left a comment
There was a problem hiding this comment.
all comments are p3 non-blocking
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account.
No actionable findings. Source-review assessment: 9/10 for minimalness, ownership, and correctness. The shared FileTypeIcon serves both composer drafts and received/unavailable cards; the existing native/proxy/external download paths, accessible action names, preview restoration, and error/retry controls are preserved. The fixed PDF color pair is owned by shared tokens and recorded in the registry. Reviewed all 10 changed files and relevant callers, including a fresh-frame self-review.
Reviewed head be4e99aa20a3cdc5ec638c38d61b4f022f08f302 against base e4bb8ae830ff8e9ad932c61d4f33322688ebe3f8. Inspected the public PR description (no attached images), changed source/tests, and both commit messages; no public-material disclosure finding.
Source-only: no PR code, tests, builds, or app launches executed; hosted CI was not assessed. The author’s browser-test results were not independently reproduced. Native downloads, actual keyboard/error-retry focus, rendered geometry at narrow/wide widths and enlarged text in both themes, and final human acceptance remain unverified. The combined five-PR split-equivalence claim was not independently assessed. This COMMENT is not approval or merge authorization.
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account.
No additional actionable findings in this integration follow-up. Source-review assessment: 9/10 for minimalness, ownership, and correctness. Compared the current revision with the previously reviewed be4e99a: both attachment patches are unchanged, and the new merge incorporates main without a separate merge-resolution change. Re-read the ten-file PR diff, the relevant merged shared styles, attachment callers, preview restoration, and download/upload error and retry paths, followed by a fresh-frame self-review. File artwork stays with FileTypeIcon; the PDF pair remains in shared tokens and the registry.
Reviewed head 152c89ca0a8d4bbf5962cb37b51eeeb31bec8663 against base 47689e46e401204e33f4d32508f0d6407486a70b. Inspected the public PR description (no attached images), all three PR commit messages, and changed publication material; no new disclosure finding on those surfaces. This follow-up does not independently review every unrelated change merged from main or establish the five-PR split-equivalence claim.
Source-only: no PR code, tests, builds, or apps executed; current hosted CI and the author’s reported browser runs were not independently assessed. Native downloads, actual keyboard/error-retry focus, rendered layouts at narrow/wide widths and enlarged text in both themes, test-cost measurements, and final human acceptance remain unverified. This COMMENT is not approval or merge authorization.
loganj
left a comment
There was a problem hiding this comment.
🤖 Larry's performance and responsiveness review, for Logan. Head 152c89ca.
No performance or responsiveness concerns. FileTypeIcon is a pure, cheap classification on each render. The four new Tabler icons are imported by file, so they do not pull in the whole icon set. The card changes are static CSS with no new animation or measurement. All required checks pass.
…ui-polish * buzz-app/main: (43 commits) Unify file attachment cards and file-type artwork (#1220) feat(agents): start agents from relay-synced config copies (#1185) render shared sessions as attachment cards (#1217) fix(channels): keep other channels' permission reads alive across a delete (#1245) feat(channels): mark temporary channels in the sidebar (#1249) test(browser): fix six intermittent browser test failures and a composer caret bug (#1239) Close channel tabs together and make tab shortcuts predictable (#1154) Make message retry controls compact and accessible (#1221) fix(messages): end deferred composer focus on any focus move; stabilise hover-preview probe (#1244) fix(messages): keep restored-channel focus off the composer (#1243) fix(me): record reads in Solo chats opened from the sidebar (#1183) feat(inbox): archive or restore the open conversation with Control-Command-A (#1181) fix: keep panel divider dragging responsive (#1067) feat(agents): find people for agents through the relay's name-only profile search (#1124) fix(agents): explain why Create agent is disabled (#1232) feat(agents): run up to 8 Interests at once for every agent type (#1100) fix(codex): set up Codex from the agent creation card, as Claude does (#1231) Shrink the thread loading mark by a third (#1214) feat(agents): plugins add tools to every agent session (#1216) fix(sidebar): re-measure every drop target when a section resizes mid-drag (#1230) ... Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
File attachments use one contained file-type icon in both the message card and composer preview. Text, code, archives, spreadsheets and PDFs have distinct Tabler artwork; PDFs use red on white in both themes. Downloadable and unavailable file cards share the same geometry and hierarchy.
Split from #1191. Independent of the message layout and agent activity PRs; based directly on main. Existing attachment loading, media-source lifecycle and download behavior are preserved.
Validation: mandatory commit/push hooks passed; both Chromium/WebKit attachment-gallery checks passed. The file-artwork assertions from the original combined gallery test move to message-files-polish.spec.mjs; browser computed rendering is the boundary being checked. No original assertions are dropped.
Ready for review at Arjun’s request; hosted CI and final human acceptance are tracked separately; current-head CI is green and human acceptance remains unconfirmed. No temporary playground is included.
Split set: #1218 layout; #1219 reaction interactions; #1220 file attachments; #1221 delivery retries; #1222 agent activity. Merge #1218 before #1219/#1222; the latter two target its branch and should be retargeted/rebased after the base is squash-merged. #1220/#1221 target main independently.
Final split audit: all five actual branches merge together without conflicts. Their combined production/source/docs match the original #1191 changes exactly on main
e4bb8ae83; only the gallery-test partition files differ, with all original assertions retained. Independent partition review found no remaining blockers. Original #1191 and its branch are preserved.CI follow-up: updated the existing media-controls file-preview assertion from the previous control fill to the intended dark inset surface. The old assertion failed in Chromium and WebKit; after correction, the full media-controls and message-files-polish files passed all 24 tests in both engines. No browser cases or assertions were removed, and no production code changed in this repair. The existing real-browser case continues to verify theme inheritance and hover colors. Native acceptance remains unconfirmed.
Repair follow-up (head
152c89ca0): merged main47689e46ewithout conflicts to incorporate the already-merged hover-preview fix (#1215) and its test follow-up (#1244). No new product edits or test relaxations; the attachment diff against main is unchanged. Fullmedia-controls.spec.mjs,message-files-polish.spec.mjs, andbuzz-links.spec.mjspassed 42/42 in Chromium and WebKit (3.1 minutes). Mandatory push hooks passed, including types/related unit tests (553.09 seconds) and design checks. All three PR commits have DCO sign-offs. Current-head hosted CI: https://github.com/block/buzz-app/actions/runs/38910023242 (SUCCESS: all 18 browser shards, all six browser flake shards, JavaScript, Rust/tool integration, measurements, required CI, DCO, Semgrep and zizmor passed). Native/manual visual acceptance remains unconfirmed; no acceptance marker is claimed.