Polish media viewers, playback controls, and galleries - #341
Conversation
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
…fades Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fda59b978c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const outsideTabStops = | ||
| !container.contains(active) || |
There was a problem hiding this comment.
When a user opens the emoji picker from a media-review reaction control, Popover.Portal places the focused picker/search outside this backdrop. This condition therefore treats every Tab press inside the picker as escaped focus, prevents the event, and moves focus to the first or last control in the underlying dialog, so the picker cannot be navigated by keyboard. Include modal-owned portal content in the focus scope, or defer to the nested popover's focus management while it is open.
AGENTS.md reference: src/shared/design-system/AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord’s automated source review via Wes’s account.
Reviewed head fda59b978c74848bdc89308f92a28d6bbc233938 against base 85d6bf82c54d1c8d930d58444597a1fe31cc8975.
One actionable keyboard-focus regression is detailed inline. The gallery navigation should preserve focus on a still-enabled Next/Previous button so consecutive keyboard activations continue working. No additional actionable findings from the playback, comment routing, media styling, or motion-lifecycle source review.
Validation limits: source-only review of the pinned snapshot and changed tests, with independent playback and image/motion review lanes. No PR code, tests, builds, or app workflows executed. A single hosted-check snapshot showed JavaScript, Rust/tool integration, DCO, and some browser jobs successful, with other browser jobs still running and Windows native validation skipped; this is not an all-green or runtime-validation claim. The author reports native/human testing, but this review did not independently observe it. Native geometry, gesture timing, and browser rendering remain unverified here.
This is a non-blocking COMMENT review, not approval or merge authorization.
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ca24b74d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| replies={replies} | ||
| limited={snapshot.limited} | ||
| timecodesSeekable={videoUrls.size === 1} | ||
| timecodesSeekable={props.attachment.kind === "video"} |
There was a problem hiding this comment.
When a thread contains more than one video, this now marks every parsed timecode as seekable merely because the currently open attachment is a video. Since timecode replies do not identify an attachment, all of those replies become timeline markers for—and seek—the currently selected clip, potentially sending users to the wrong frame. Retain the previous unique-video check or encode which attachment each timecode targets.
Useful? React with 👍 / 👎.
| } | ||
| /* Shared dark media review and compact video surfaces. */ | ||
| .mediaPreview[data-video-preview] { | ||
| width: min(384px, 100%, calc(var(--media-ratio, 16 / 9) * 180px)); |
There was a problem hiding this comment.
For a portrait attachment such as 9:16, this formula makes the preview only about 101px wide. The new control row contains non-shrinking time labels, speed and volume groups, 28px buttons, and a timeline with a 20px minimum, while .mediaPreview clips overflow; consequently most playback controls are visually clipped and cannot be clicked. Add a compact responsive control layout or keep the preview wide enough for the controls.
Useful? React with 👍 / 👎.
| .controls :focus-visible, | ||
| .center:focus-visible { | ||
| outline: 2px solid var(--text-standard); | ||
| outline-offset: 3px; |
There was a problem hiding this comment.
These unlayered CSS-module rules override the host's layered pointer-focus suppression. In browsers that retain :focus-visible after clicking or programmatically focusing a media slider/control, a pointer interaction therefore shows the keyboard outline. Prefix these authored rings with the app's html[data-keyboard-navigation] modality owner so pointer and keyboard focus remain distinct.
Useful? React with 👍 / 👎.
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord’s automated source follow-up review via Wes’s account.
Head: 6ca24b74d68c23434e956039167ab964e5dd6a40
Base: 85d6bf82c54d1c8d930d58444597a1fe31cc8975
Incremental scope: the 12-file delta since reviewed head fda59b978c74848bdc89308f92a28d6bbc233938, the prior gallery-focus finding, and the changed behavior’s callers/lifecycle.
No actionable source findings in this follow-up.
- The prior P2 gallery-focus regression is addressed in source:
ImageReviewStageretains focus on an enabled Next/Previous button and falls back to the gallery at the endpoint. Added Enter/Space tests cover repeated navigation in both directions and endpoint focus. - Reviewed the new Space/arrow shortcuts, editor/slider/menu exclusions, initial modal focus, and shared relative-seek helper. Also traced immediate dismissal pause, pending gesture guards, late-play handling during exit, detached-video cleanup, and reaction/feedback fading. The emoji changes leave button geometry unchanged.
- The new regression tests were read, not run. No browser cases were added or removed in this incremental delta.
Validation limits: source-only; no PR code, tests, builds, or app workflows executed. One hosted-check snapshot showed JavaScript and DCO successful, Rust/tool integration and browser journeys still running, and Windows native validation skipped. It is not an all-green result. Author-reported browser/native testing was not independently observed. The PR description explicitly leaves the latest human confirmation pending; the repository’s ready-for-review checklist therefore remains incomplete, and this source result does not clear that gate. Native rendering, real playback timing, and current human acceptance remain unverified here.
Non-blocking COMMENT review only—not approval or merge authorization.
Signed-off-by: Codex <codex@openai.com>
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord’s automated source follow-up review via Wes’s account (wesbillman).
Head: 57561a14e25a57cf8c2c18c1530598dcb495f21f
Base: a2927b44b1ef426c2bc7b98369139a6479b45d40
Previous reviewed head: 6ca24b74d68c23434e956039167ab964e5dd6a40
No new actionable findings in this bounded merge/integration follow-up.
- Of the 52 files in the current PR delta, 48 have identical Git blobs to the previous reviewed revision. The four overlaps are shared button CSS,
IconButton, component metadata, and its design specimen. All other head-to-head file changes exactly match the new base. - The merge preserves both the base’s
avatarand this PR’smediavariant. Both continue through the existing Button behavior; hover/pressed exclusions retain both treatments, and the media-glass recipe and media caller remain intact. The registry and specimens keep both variants. - The previous gallery-focus fix and its Enter/Space regression tests are unchanged. No tests or browser journeys were added or removed by the media-side integration changes.
This does not reopen or independently revalidate the unchanged feature. Source assessment of this integration meets the 9/10 minimalness/elegance/correctness bar; no new owner or abstraction was introduced.
Validation limits: immutable, Git-blob-verified source only; no dirty checkout inputs, PR code execution, tests, builds, app/native workflows, or CI checks. The description’s test evidence is for the older 6ca24b74 revision, and its latest human confirmation remains pending. Rendering, playback, merged-state test results, and human acceptance are not established here. This COMMENT is not approval, checklist clearance, or merge authorization.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57561a14e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| type="button" | ||
| aria-label={`Playback speed: ${rate}x`} | ||
| aria-expanded={menu} | ||
| onClick={() => setMenu(!menu)} |
There was a problem hiding this comment.
When the speed menu opens, focus remains on this button, but the Escape handler is attached only to the sibling fieldset. Pressing Escape immediately therefore leaves the menu open in an inline preview; inside the review modal it bubbles to useModalBoundary and closes the entire viewer. Handle Escape from the shared speed wrapper/trigger or move focus into the menu when it opens.
AGENTS.md reference: src/shared/design-system/AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
* origin/main: Refine presence labels and status colors (#323) Polish media viewers, playback controls, and galleries (#341) perf(mentions): name each choice set once and skip closed choosers (#310) Fix clipped emoji in reaction pills (#339) Delay message timestamp tooltips by 500 ms (#321) Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz>
Photo and video previews now expand smoothly into a shared dark viewer with fading controls and an animated comments sidebar. Adds photo gallery navigation, trackpad zoom/pan, animated GIF playback, and custom video controls with saved speed. No new dependencies.
Video gestures support click-to-play/pause, double-click ±10-second seeking, and temporary 2× while holding, with brief visual feedback. Closing stops playback immediately and fades the reaction tray with the other controls. Space toggles play/pause and Left/Right arrow keys seek 10 seconds without gesture overlays; editing and slider keys keep their normal behavior. Timestamp comments seek the selected video and appear on its timeline; opening from a reply preserves the same conversation. Chat preview glass matches the existing media treatment and keeps white controls in either app theme.
Validation: independent review, all 1,675 related tests, and local type/design checks pass at
6ca24b74. All 49 focused video/motion/gesture tests pass, including dismissal, delayed playback, and StrictMode autoplay coverage. Browser checks verified gallery focus, keyboard playback/seeking, comment-editing isolation, and paused playback with the tray faded during exit. Earlier native flows were human-tested; the latest gallery, keyboard, emoji, and dismissal follow-ups await human confirmation. DCO passes; hosted CI is pending.Snapshots (sample content)
Chat preview
Video viewer
Photo gallery
Gesture feedback