Skip to content

fix(studio): make clip peak warnings plain and hoverable - #5099

Merged
miguel-heygen merged 3 commits into
mainfrom
fix/clip-peak-plain-warning
Oct 6, 2026
Merged

miguel-heygen merged 3 commits into
mainfrom
fix/clip-peak-plain-warning

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

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

  • Red-first Chromium reproduction with the production AudioWaveform canvas and sustained full-height peaks: warning obscured and no keyboard detail.
  • Final component tests pass three consecutive runs: 3/3. Shared Tooltip tests pass 5/5; TimelineClip tests pass 12/12.
  • CI fixture regressions fixed: shared badge CSS assertions pass 2/2 and timeline behavior tests pass 83/83, each across three consecutive runs. The timeline test locates its semantic track cell rather than assuming a fixed wrapper chain.
  • Repository lint, Studio typecheck, and changed TypeScript formatting pass.
  • Final Chromium checks pass three consecutive runs: warning paint, Tab order, linked description, Escape, quiet clips, 32-pixel audio/video clips, and both trim edges.
  • Deliberately removing the paint layer, focus registration, or quiet disabling makes its check fail. Each mutation is restored.
  • Changed source files match the remotely executed source byte-for-byte.

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:

Before: technical peak readout

Review reproduction with a sustained high-amplitude waveform:

Before: real waveform obscures the warning

After

After: warning paints above the real waveform

After: keyboard detail remains visible on a narrow clip

@miguel-heygen
miguel-heygen marked this pull request as ready for review October 6, 2026 01:34

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 to bottom-1 places it inside the audio waveform canvas area. AudioWaveform.tsx:238–245 paints 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 has pointer-events: none and 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 native title on 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)

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Edit accuracy: accurate 2055 (base branch 2055), smooth 1634 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Addressed both points in a92e8d0:

  1. Waveform paint order: the shared warning badge now paints at z-index 20 above the real AudioWaveform canvas, within the existing isolated context that keeps trim handles above it. Removed the redundant video-only z-index. The revised high-amplitude Chromium fixture reproduces the obstruction before the fix and verifies unobscured warning pixels afterward. Both trim edges still work on wide and 32-pixel audio/video clips. Removing the paint fix makes the regression check fail.
  2. Keyboard numeric detail: the existing clip button now opens Studio's shared tooltip on focus, with its numeric peak linked through aria-describedby. There is no new tab stop. The portal remains visible outside a narrow clip; Escape dismisses it. Lowering volume to a quiet level clears the tooltip and description while preserving the same focused button. Removing registration or quiet disabling makes the component tests fail.

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 terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 has z-20 inside the isolated clip content, above the actual AudioWaveform.tsx:238–245 layer at z-index 10. The production waveform remains a child of ClipPeakMarks (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–60 opens on focus and sets aria-describedby. The focused-clip and quiet-transition assertions in ClipPeakMarks.test.tsx:48–79 pass.

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)

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Follow-up at 80c2a2a fixes the two CI fixture regressions:

  • The shared [data-peak-badge] CSS rule now owns z-index 20 for both audio and video, preserving the existing styling contract.
  • The track-expansion test locates the semantic grid cell instead of assuming a fixed chain of parent wrappers. Its height and expansion assertions are unchanged.

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 terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.ts finds the semantic [role="gridcell"] instead of assuming an inline display: contents wrapper 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)

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 6, 2026
Merged via the queue into main with commit 4905b4d Oct 6, 2026
94 checks passed
@miguel-heygen
miguel-heygen deleted the fix/clip-peak-plain-warning branch October 6, 2026 04:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants