Repository navigation
Conversation
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Arjun Mahanti <arjun@squareup.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security), and Groot (product/accessibility) reviews.
One P2 finding inline: extreme video aspect-ratio metadata can now reserve an enormous timeline row. The intended readable 16rem portrait sizing and aspect-ratio preservation should remain; the requested repair is a bounded fallback for extreme metadata. Source assessment: 8/10 until that boundary is handled.
Reviewed head: e0814a8. Base tip: 8dff9ac; diff merge-base: a9e82d1. All three changed files and supported callers were inspected. Existing MediaAttachment/VideoControls owners are reused. No public-description disclosure issue found; no attachments were present to inspect.
Validation: source only; no tests, builds, apps, or exploit reproduction run. One hosted CI snapshot showed completed checks successful, two browser shards still running, and Windows native/Vitest flake lanes skipped. Browser geometry, live scrolling, native playback, error/retry focus and human acceptance were not independently exercised. The new browser case has an appropriate real-layout justification; the author’s fail-then-pass evidence was not reproduced. This COMMENT is advisory, not an approval or merge authorization.
kalvinnchau
left a comment
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 One non-blocking test coverage finding inline; the existing height-bound finding remains open.
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Arjun Mahanti <arjun@squareup.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.
Changes requested: one blocker. The sizing direction looks right, but the global object-fit: contain change also reaches collapsed GitHub description thumbnails, and required CI is red on it in both engines.
Merge criteria: keep cropped thumbnails at cover (inline thread), leave the existing github-conversation.spec.mjs:226 assertion as is, and get required CI green.
Validation gaps: source review plus existing CI only, no live run. The new sizing tests don't cover collapsed thumbnails. The separate WebKit new-message.spec.mjs:305 fill timeout has no source link to this diff we could find.
| width: 100%; | ||
| height: 100%; | ||
| object-fit: cover; | ||
| object-fit: contain; |
There was a problem hiding this comment.
🤖 Blocker. This shared rule also styles the image inside collapsed GitHub description thumbnails. Those tiles are meant to be cropped squares, so contain letterboxes non-square screenshots.
Evidence: tests/browser/github-conversation.spec.mjs:226 expects object-fit: cover and gets contain in both Chromium and WebKit (run 39628331358). The thumbnail owner's override at src/bundled/github/GitHub.module.css:635 (.thumbnail [data-image-preview] > img) never applied, because MediaAttachment nests the <img> inside a span. Before this change, cover came from this rule.
Fix: scope the crop to the thumbnail owner by making the GitHub.module.css selector reach the nested image (for example .thumbnail [data-image-preview] img), and keep contain here for message and body media. Keep the existing assertion.
There was a problem hiding this comment.
🤖 Addressed in ad04ddc: the thumbnail-owned selector reaches the nested image and restores cover; the existing assertion was unchanged. All 42 focused GitHub conversation, media sizing/controls, and new-message tests passed across Chromium and WebKit on that fix.
Current head e4bd191 also integrates main: upstream c0451f2 (#1325) had replaced the old GitHub conversation UI and removed its thumbnail owner and test. The integration preserves that upstream removal rather than resurrecting obsolete CSS. Bounded body media still uses contain. All 38 surviving/current GitHub body/checklist, media sizing/controls, and new-message tests pass across both engines; mandatory hooks passed. The prior composer timeout did not reproduce, and no speculative composer/test change was added.
DCO is green; hosted CI is still queued in https://github.com/block/buzz-app/actions/runs/39795729317. Human/native acceptance is not claimed. Leaving this thread unresolved for reviewer assessment.
|
🤖 Review follow-up audit of current head Required CI remains red in run 39628331358. The current source still has WebKit also times out filling the Avery Chen composer at |
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Preserve the upstream replacement of the old GitHub conversation thumbnails. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Arjun Mahanti <arjun@squareup.com>
|
🤖 CI follow-up for pushed head Hosted completion is currently blocked by GitHub's active Actions Results incident, which reports delays processing workflow and step results. Run 39795729317 still reports queued; other repository runs show the same delay. No rerun was requested, and the existing run remains intact. CI is not yet verified green. The prior thumbnail blocker is addressed as explained in the review-thread reply; reviewer approval and human/native acceptance remain outstanding. |
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security), and Groot (product/accessibility) reviews.
No actionable defects found in this follow-up and current integration. The prior extreme-height finding is addressed by bounded frames and contained media. The subsequent thumbnail-cropping finding concerns an old GitHub thumbnail implementation removed by the merged upstream changes; that caller is absent from the reviewed revision. The current GitHubBody → MediaAttachment path retains bounded, uncropped media. Source assessment: 9/10.
Reviewed head: e4bd191. Base and merge-base: 3d23acb. Inspected the five-file diff, prior covered revisions, supported callers, sizing/resize lifecycle, error states and modal focus paths. Existing MediaAttachment, AudioAttachment and VideoControls owners remain in place. The public description and four commit messages showed no disclosure finding; no image attachments were present.
Optional wording cleanup: describe the increased portrait-preview height allowance and update the fixture’s “without a height clamp” wording. The description explicitly attributes its test results to 1c291c3; refresh that summary when validation of the merged head finishes.
Validation limits: source only; no tests, builds or app execution. Three added browser cases (none removed by this PR) have a real-layout/decoded-media/focus justification; reported fail-then-pass results were not reproduced. A single 14:34 UTC hosted-check snapshot had many checks running or queued, so current-head CI completion, native playback, rendered geometry, keyboard/screen-reader behavior, error/retry focus and human acceptance remain unverified. This COMMENT is advisory, not an approval or merge authorization.
loganj
left a comment
loganj
left a comment
There was a problem hiding this comment.
Larry and Rbot: an independent adversarial review of e4bd191.
The video fix itself is correct. Rbot traced the video, poster, thread and GitHub-image callers. [data-video-preview] at line 2243-2244 is the real owner of video size, and it has its own floor and height cap. One consequence: the 16rem floor and the max-height on the base .mediaPreview and .mediaPreviewThread rules now apply to images only.
Should fix: the image-only floor puts dark side bars on ordinary phone screenshots. It also caused the tall-image problem that the max-height and contain were then added to correct. A smaller change fixes both (inline at line 1033).
Non-blocking:
- Audio has no regression test for the outgoing collapse. This one is from Rbot (inline).
- The narrow-screen image cap changed, and this is not mentioned in the PR (inline).
- The portrait video now uses much more of the timeline. This is a product call (inline).
- The portrait fixture puts portrait metadata over the landscape sample asset. It proves the frame size, not portrait pixels, so the PR text should not claim more than that.
Evidence: source review, plus a standalone Chromium probe of the base-rule CSS (numbers inline). We did not run the repo suites.
| collapsing to their byline. max-width still lets narrow panes shrink. */ | ||
| width: min( | ||
| 32rem, | ||
| max(16rem, calc(min(28rem, 56vh) * var(--media-ratio, 16 / 9))) |
There was a problem hiding this comment.
Should fix: the floor letterboxes common portrait images. This rule and .mediaPreviewThread are overridden for video by [data-video-preview] (2243-2244), so this floor now affects only images. Any image narrower than 9:16 gets a 16rem frame. The frame is then capped by max-height, and with contain (1065) and the img's --surface-inverse background, the space beside the image paints as dark side bars.
I copied this rule into a standalone Chromium page and loaded an iPhone screenshot ratio (1179×2556) inside a fit-content body:
| viewport height | frame | image width | bar on each side |
|---|---|---|---|
| 950 (this PR) | 256×448 | 207 | 25px |
| 700 (this PR) | 256×392 | 181 | 38px |
| 950, no floor | 207×448 | 207 | 0 |
| 700, no floor | 181×392 | 181 | 0 |
Before this PR, these images had no bars. The floor also caused the 1000×6000 problem: at a 16rem width that image is 96rem tall. Without the floor, calc(cap * ratio) gives about 4.7rem × 28rem, which is already bounded.
The collapse came from the 100% term inside min(), not from the missing floor. calc(cap * ratio) is already a definite width. So the simpler fix for the base and thread rules is:
width: min(32rem, calc(min(28rem, 56vh) * var(--media-ratio, 16 / 9)));
max-width: 100%;That means no floor and no max-height. Keep the floor, the height cap and contain only in the video owner ([data-video-preview], and .mediaVideo / .mediaPoster), and restore cover for .mediaPreviewPixels > img. Images keep their exact ratio, so cover does not crop them. The tall-image test should still pass, because 4.7rem × 28rem is 448px or less.
There was a problem hiding this comment.
🤖 Fixed in a1feb3d. The base and thread image widths now derive directly from aspect ratio, with no 16rem floor or independent height clamp; image pixels use cover. The video owner retains its readable width floor, bounded height, and contained video/poster pixels.
The existing image regression now checks decoded 1179×2556 phone screenshots and 1000×6000 images through GitHub and thread modes. It failed before the fix in both Chromium and WebKit on the frame/image aspect-ratio mismatch. Afterward, all 30 cases in the complete sizing, media-controls, and GitHub-body files passed across both engines, including wide/narrow/shorter viewports, light/dark, enlarged text, fullscreen, Escape, and focus return. Mandatory commit/push hooks passed. Current-head CI is running: https://github.com/block/buzz-app/actions/runs/39812925630 . Native/human acceptance remains unverified.
| .mediaPreview { | ||
| max-height: 18rem; | ||
| } | ||
| .audioAttachment { |
There was a problem hiding this comment.
Two narrow-screen points (max-width 40.625rem):
- This PR deleted
.mediaPreview { max-height: 18rem; }from this block. On a 390×844 phone, image previews can now be up tomin(28rem, 56vh)≈ 448px tall, instead of 288px. Please confirm this is intended and mention it in the PR, or restore a narrow cap. If you take the no-floor suggestion at line 1033, the cap can go on the basecalc()instead of onmax-height, so it does not letterbox. - This
.audioAttachment { max-width: 100%; }is now redundant: the base rule (line 987) sets the same value. It can be removed.
There was a problem hiding this comment.
🤖 In a1feb3d, restored the narrow image height bound to 18rem through the ratio-derived width calculation in both base and thread modes, so it does not introduce side bars. The extended image case checks the bound and decoded aspect ratio at 390×844 with 16px and 20px root text in both engines; all 30 focused checks passed. The PR description now records this behavior. The redundant audio declaration is unchanged; this pass leaves optional cleanup aside.
|
|
||
| // Browser layout is required: percentage widths inside an outgoing fit-content | ||
| // message can collapse even though the media has a valid aspect ratio. | ||
| test("video-only outgoing messages keep a readable, proportional preview", async ({ |
There was a problem hiding this comment.
(From Rbot.) Non-blocking coverage gap. The audio change at line 986-987 fixes the same percentage/fit-content collapse, but only video has a regression test. audio-attachment.spec.mjs:77 cannot catch the collapse: its fixture (tests/fixtures/audio-attachment.tsx:12) gives the player a fixed 14rem/24rem parent. Please add an outgoing audio-only row to the message-gallery fixture, and check a minimum width against the independent [data-layout] row, as this video test does.
| max-height: 16rem; | ||
| /* Ordinary 9:16 portraits still fit at the readable 16rem width. */ | ||
| max-height: min(32rem, 70vh); | ||
| width: min(24rem, max(16rem, calc(var(--media-ratio, 16 / 9) * 11.25rem))); |
There was a problem hiding this comment.
Product question: a 9:16 portrait video preview grows from about 6.3rem × 11.25rem to 16rem × 28.4rem, which is about 455px of timeline height at 16px text. That size makes the controls easier to use. If it is larger than intended, a smaller height cap here would keep the 16rem width floor and still bound the height.
There was a problem hiding this comment.
🤖 This follow-up preserves the existing readable portrait-video size; a1feb3d changes image sizing only. The PR description now explicitly records the approximately 16rem × 28.4rem 9:16 frame and that the portrait fixture validates metadata-driven frame geometry over a landscape sample, not portrait pixels. The existing portrait-geometry and video-control checks passed in Chromium and WebKit among the 30 focused cases. Human acceptance of the larger video frame remains outstanding; no product-size approval is claimed.
Remove the image-only floor, preserve narrow image height bounds through proportional widths, and cover decoded portrait/tall images in base and thread modes. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Arjun Mahanti <arjun@squareup.com>
|
🤖 Logan-only follow-up is pushed as Current-head CI is not green: Chromium shard 3 fails |
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security), and Groot (product/accessibility) reviews.
No actionable findings in this follow-up. The image repair addresses the portrait side bars and restores the narrow-screen height bound through proportional widths. The more-specific video rule retains its readable width floor and bounded height; video/poster pixels remain contained. The production GitHub image caller supplies no dimension metadata, so its image ratio comes from decoded pixels. Existing MediaAttachment, VideoControls and modal/resize owners are preserved. Source assessment: 9/10.
Reviewed head: a1feb3d. Base and merge-base: 3d23acb. Compared the three-file follow-up against previously reviewed e4bd191 and checked its integration with the five-file PR diff and supported callers. Public description, discussion and commit messages showed no new disclosure finding; no attached images were present.
Validation limits: source only; no tests, builds or app execution. No browser cases were added or removed in this follow-up; the expanded image matrix has a real-layout/decoded-image justification, and its thread fixture exercises renderer mode rather than the complete app thread journey. Reported fail-before/pass-after results were not reproduced. The author reports a current-head Settings scroll-restoration CI failure; CI status and cause were not independently assessed here. Rendered pixels, live resize/scrolling, native playback, keyboard/screen-reader behavior, error/retry focus and human acceptance remain unverified. This COMMENT is advisory, not an approval or merge authorization.
Co-authored-by: bmo <a7b504d9a2bfe905589e9ac237d7d8d023b8d10547060ec9618e6487b55d756c@buzz.block.builderlab.xyz> Signed-off-by: Arjun Mahanti <arjun@squareup.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/performance/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) reviews.
No new actionable findings. Source assessment: 9/10. This follow-up checks the upstream merge since reviewed head a1feb3d. The five-file feature patch is unchanged apart from offsets/context metadata, with no merge-conflict resolution. The incoming compact media-grid path uses attachmentImage rather than mediaPreview, so the standalone sizing rule does not resize its square tiles. Existing MediaAttachment, VideoControls and modal owners remain responsible for playback, failure handling and focus.
Reviewed head: 245d333. Base and merge-base: c2210b4. Public description and feature commit messages inspected; no attached images or new disclosure finding found.
Validation limits: source only; no tests, builds or app execution. Three geometry/decoded-image browser cases belong to the original feature; this merge adds no feature cases. The description’s 30-pass evidence applies to a1feb3d, not this head, and was not reproduced. One 11:28 UTC hosted-check snapshot had browser, JavaScript, Rust and measurement jobs still running; completed checks had succeeded, with Windows native and selected Vitest flake checks skipped. Rendered geometry, native playback, portrait pixels, keyboard/screen-reader and error/retry focus behavior, performance and human acceptance remain unverified. This COMMENT is advisory, not an approval or merge authorization.
Video-only outgoing messages could shrink their previews to the byline width because percentage sizing fed back into the message's fit-content layout. Use a definite preferred width with a 16rem floor and an available-width cap for videos, and a definite width for audio players.
Image previews use definite ratio-derived widths without a minimum-width floor or an independent height clamp. Phone screenshots and tall GitHub body images retain their aspect ratio without side bars. Narrow images retain the previous 18rem height bound through the width calculation. Video and poster content remains contained inside bounded frames for extreme metadata.
The ordinary 9:16 video frame intentionally retains this PR's readable 16rem width (about 28.4rem high at 16px root text), subject to the existing viewport cap. The portrait-video fixture uses portrait metadata over a landscape sample: its checks prove frame geometry, not portrait video pixels. Human acceptance of the larger portrait frame remains outstanding. Existing MediaAttachment, VideoControls, and message CSS remain the owners.
Validation
Current checked revision:
a1feb3d348d5e91e633f4de62606b702048167a0.e4bd191d6passed hosted CI and 38 focused browser checks. Its upstream integration retained GitHub integration: gh-backed panel with checklist and merge, Solo PR link, and watch_pr #1325's replacement of the old GitHub conversation thumbnails; the old thumbnail assertion passed onad04ddc43before that integration.settings.spec.mjs:1140(plugin-detail catalog scroll expected 1505, received 1791); other jobs are still running. This Settings flow is outside the Logan-only repair scope. DCO is green.Native live visual confirmation and human acceptance remain deferred. Earlier native build succeeded, but community loading could not be verified. No new native acceptance, portrait-pixel validation, or screenshots are claimed by this follow-up.