Repository navigation
fix(studio): make clip peak warnings plain and hoverable - #5099
Conversation
terencecho
left a comment
There was a problem hiding this comment.
At 80e8889f285db6fa86808154dccd56faf5dbc157: the Before/After captures show the new label clearing the fx badge, and the video sound-strip z-index rule (packages/studio/src/styles/components.css:317–325) keeps its compact warning above the waveform. I found one audio-path blocker and one accessibility regression:
-
Blocker — the audio waveform can paint over the new warning (
packages/studio/src/player/components/ClipPeakMarks.tsx:49). Moving “Too loud” from the top tobottom-1places it inside the audio waveform canvas area.AudioWaveform.tsx:238–245paints a full-width canvas in a layer with z-index 10; the audio badge has no z-index, while only the video badge gets z-index 20. On a sustained loud clip, near-full-height waveform bars cover the bottom-positioned red text; before this change the warning sat at the top, outside the canvas region. A representative Chromium paint reproduction with near-full-height bars obscured the text, while the same badge without canvas paint was legible; the attached PR capture does not exercise that case. Give the audio warning a paint layer above the waveform (while staying below trim handles) and add a high-amplitude visual regression case. This is a paint-order issue, not a hover hit-test issue: the production content wrapper haspointer-events: noneand the badge opts back into pointer input. -
Important — keyboard-only users lose the numeric peak (
packages/studio/src/player/components/ClipPeakMarks.tsx:48–54). The former dBFS text was visible; it now exists only in a nativetitleon a non-focusable span. Keyboard focus lands on the outer clip button (TimelineClip.tsx:132–159), whose own label/title do not include the peak, so the number is not available without hover. Preserve an accessible focus path to the detail (e.g. focusable warning with a focus-triggered tooltip or a clip-level description). I am not claiming the pre-existing screen-reader label omission was introduced here.
Verdict: REQUEST CHANGES
Reasoning: The audio warning can be visually obscured precisely on sustained loud clips, which defeats this change's primary message; the numeric detail also loses its formerly visible keyboard-only path.
— Review by tai (pr-review)
Edit accuracy: accurate 2055 (base branch 2055), smooth 1634 of thoseThe gate passes. Quarantined, measured but not gated (0) |
|
Addressed both points in a92e8d0:
The prior SVG fixture missed the production canvas's paint order. The replacement uses the real AudioWaveform component with deterministic peak responses; it does not claim media-decoding or persisted-edit coverage. Updated captures are in the PR body. Validation: component tests 3/3 across three consecutive runs, shared Tooltip 5/5, TimelineClip 12/12, three consecutive Chromium passes, repository lint and Studio typecheck. All changed source files match the remotely executed files byte-for-byte. |
terencecho
left a comment
There was a problem hiding this comment.
Re-review at a92e8d0d2a8ddde3a23adaf1cca199b742d34e24 of my prior CHANGES_REQUESTED review at 80e8889f:
- Audio paint-order blocker resolved (
packages/studio/src/player/components/ClipPeakMarks.tsx:44–64): the badge now hasz-20inside the isolated clip content, above the actualAudioWaveform.tsx:238–245layer at z-index 10. The production waveform remains a child ofClipPeakMarks(useRenderClipContent.ts:80–101). This is code-path/stacking-order verification, not a new Chromium paint capture. - Keyboard numeric-detail regression resolved (
ClipPeakMarks.tsx:34–42,ClipPeakTooltip.tsx:13–19,TimelineClip.tsx:132–160,260): the numeric dBFS detail reaches a tooltip on the same focusable clip button;Tooltip.tsx:36–60opens on focus and setsaria-describedby. The focused-clip and quiet-transition assertions inClipPeakMarks.test.tsx:48–79pass.
Important — update two stale test assumptions: Linux and Windows Studio Test jobs fail on AudibleVideoClipContent.test.tsx:51–64 (a bare test badge omits the production component's new z-20 class after the redundant CSS rule was removed) and Timeline.test.ts:90–95,741–750 (the test's parent-walking helper recognizes inline style.display === "contents" but not the new tooltip wrapper's className="contents"). A focused local run passed 91 tests and failed those same two; I found no corresponding production row-height or interaction regression. These tests should be brought into line with the rendered component and DOM shape, but CI status alone is not a code-merit hold.
Verdict: APPROVE
Reasoning: Both previous user-visible defects are fixed on the live head, and the remaining red assertions reflect test-fixture/selector drift rather than a confirmed production regression.
— Review by tai (pr-review)
|
Follow-up at 80c2a2a fixes the two CI fixture regressions:
The affected suites pass 2/2 and 83/83 across three consecutive runs. Real-waveform paint, keyboard and trim checks still pass; deliberate paint/focus/quiet mutations fail and are restored. Repository lint and Studio typecheck pass. No timing allowances were increased. |
terencecho
left a comment
There was a problem hiding this comment.
Re-review at 80c2a2af665fff13d7c5149b8f7c3bddcbf97e0a after my approval at a92e8d0d and the earlier changes-requested review:
- The audio warning still paints above the waveform and the numeric peak remains available on keyboard focus of the existing clip button. This push moves the badge's z-index 20 from the Tailwind class to the shared
[data-peak-badge]rule (components.css:317–329), retaining the stacking order over the actual waveform layer at z-index 10. The production clip/tooltip path is otherwise unchanged. This is source/stacking-order verification at this head, not a new full-Studio browser capture. - The two stale Studio assertions identified in my prior review are repaired: the bare video-badge fixture now receives the shared badge CSS, and
Timeline.test.tsfinds the semantic[role="gridcell"]instead of assuming an inlinedisplay: contentswrapper chain. The Studio Test run is green (686 suites / 7,627 tests); the affected badge and Timeline tests pass.
No new code-merit blocker found. At the time of this review the 20 Studio edit-accuracy shards are still running, so this approval is not a claim that CI or the merge gate has finished.
Verdict: APPROVE.
— Review by tai (pr-review)
What
Show “Too loud” on audio clips that reach the export ceiling. Keep the numeric peak available on hover and keyboard focus. Video sound strips retain their compact warning triangle.
Why
The technical readout was hard to interpret. Moving it below the effects row also needs an explicit paint layer above the waveform, and replacing visible numbers must preserve a keyboard path to those details.
How
The warning paints above the waveform inside the clip's isolated stacking context, beneath trim controls. The peak component owns the description; the existing clip button exposes it through Studio's shared tooltip. Quiet clips disable the tooltip without replacing the focused button. Peak detection, gain, and export behavior are unchanged.
Test plan
The browser fixture uses the real timeline, waveform, warning, and sound-strip components with deterministic peak responses and callback counters. It proves paint, focus, and trim routing, not media decoding or persisted edits.
Before
Original technical readout:
Review reproduction with a sustained high-amplitude waveform:
After