Conversation
Compute interleaved stereo min/max peaks (Lmax, Lmin, Rmax, Rmin) and render left/right channels as separate filled SVG paths stacked vertically. Closes #680 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Grid lines switch from solid to dashed while Meta/Command is pressed, providing visual feedback for multi-select mode. Resets on blur. Closes #679 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Clicking a clip now also selects its parent track and seeks the playhead to the click position. Multi-select (Cmd+click) preserves playhead position. Closes #678 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add soft dark shadow to playhead and selected-track cursor for a floating effect. Hover seek line on clips provides click-to-seek visual feedback. Closes #681 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR polishes the timeline UX by enhancing waveform visualization and interaction feedback, adding stereo-aware rendering and modifier-key UI affordances.
Changes:
- Update waveform peak computation to produce interleaved stereo min/max peaks and render dual-channel stacked SVG waveforms.
- Add a
useMetaKeyDownhook and use it to toggle timeline grid lines between solid and dashed while Meta/Command is held. - Improve timeline interactions/visuals: clip click selects parent track + seeks, playhead cursor shadow, and a faint hover seek line on clips.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/utils/waveformPeaks.ts |
Changes peak format to interleaved stereo min/max and exports PEAK_STRIDE. |
src/components/timeline/ClipWaveform.tsx |
Renders dual-channel filled paths + center divider using the new peak stride format. |
src/hooks/useMetaKeyDown.ts |
Adds global key tracking hook for Meta/Command press state. |
src/components/timeline/GridOverlay.tsx |
Switches grid line styling to dashed while Meta is held. |
src/components/timeline/ClipBlock.tsx |
Makes clip click also select track + seek; adds hover seek line rendering. |
src/components/timeline/Playhead.tsx |
Adds subtle box shadow to playhead/cursor. |
tests/unit/waveformPeaks.test.ts |
Updates unit tests for new stereo min/max peak format. |
tests/unit/clipWaveform.test.tsx |
Updates waveform rendering tests to assert dual-channel SVG paths/divider. |
tests/unit/useMetaKeyDown.test.ts |
Adds tests for Meta/blur behavior of the new hook. |
tests/unit/bounceInPlaceService.test.ts |
Adjusts expected waveform peak length for new stride format. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function computeWaveformPeaks( | ||
| audioBuffer: AudioBuffer, | ||
| numPeaks: number, | ||
| startSample: number = 0, | ||
| endSample?: number, | ||
| ): number[] { | ||
| const channelData = audioBuffer.getChannelData(0); | ||
| const regionEnd = endSample ?? channelData.length; | ||
| const leftData = audioBuffer.getChannelData(0); | ||
| const rightData = audioBuffer.numberOfChannels >= 2 | ||
| ? audioBuffer.getChannelData(1) | ||
| : leftData; | ||
|
|
||
| const regionEnd = endSample ?? leftData.length; | ||
| const regionLength = regionEnd - startSample; | ||
| const samplesPerPeak = Math.floor(regionLength / numPeaks); | ||
| if (samplesPerPeak <= 0) return new Array(numPeaks).fill(0); | ||
| if (samplesPerPeak <= 0) return new Array(numPeaks * 4).fill(0); | ||
|
|
||
| const peaks: number[] = new Array(numPeaks); | ||
| const peaks: number[] = new Array(numPeaks * 4); | ||
|
|
There was a problem hiding this comment.
computeWaveformPeaks now returns interleaved min/max values (including negatives) and a 4 * numPeaks-length array, which is a breaking contract change for existing consumers that assume numPeaks non-negative magnitudes (e.g. asset mini-waveform components that do peaks.map(p => ...)). Consider either (a) introducing a new function for the stereo min/max format and keeping computeWaveformPeaks backward-compatible, or (b) updating all call sites/renderers and persisting a version/migration so older stored waveforms still render correctly.
| const regionEnd = endSample ?? leftData.length; | ||
| const regionLength = regionEnd - startSample; | ||
| const samplesPerPeak = Math.floor(regionLength / numPeaks); | ||
| if (samplesPerPeak <= 0) return new Array(numPeaks).fill(0); | ||
| if (samplesPerPeak <= 0) return new Array(numPeaks * 4).fill(0); | ||
|
|
||
| const peaks: number[] = new Array(numPeaks); | ||
| const peaks: number[] = new Array(numPeaks * 4); | ||
|
|
||
| for (let i = 0; i < numPeaks; i++) { | ||
| let max = 0; | ||
| let lMax = 0; | ||
| let lMin = 0; | ||
| let rMax = 0; | ||
| let rMin = 0; | ||
| const start = startSample + i * samplesPerPeak; | ||
| const end = Math.min(start + samplesPerPeak, regionEnd); | ||
| for (let j = start; j < end; j++) { | ||
| const abs = Math.abs(channelData[j]); | ||
| if (abs > max) max = abs; | ||
| const lSample = leftData[j]; | ||
| if (lSample > lMax) lMax = lSample; | ||
| if (lSample < lMin) lMin = lSample; | ||
| const rSample = rightData[j]; | ||
| if (rSample > rMax) rMax = rSample; | ||
| if (rSample < rMin) rMin = rSample; | ||
| } | ||
| peaks[i] = max; | ||
| const idx = i * 4; | ||
| peaks[idx] = lMax; | ||
| peaks[idx + 1] = lMin; | ||
| peaks[idx + 2] = rMax; | ||
| peaks[idx + 3] = rMin; |
There was a problem hiding this comment.
The stride value is hard-coded as 4 in several places inside computeWaveformPeaks even though PEAK_STRIDE is exported. Using the constant (and ideally defining it above the function) reduces the risk of the implementation and tests drifting out of sync if the format changes again.
| const logicalPeakCount = Math.floor(peaks.length / PEAK_STRIDE); | ||
| if (logicalPeakCount === 0) { | ||
| // Legacy mono fallback: old peaks with no stride structure | ||
| return null; | ||
| } | ||
|
|
||
| const peakSlice = getVisiblePeakSlice(logicalPeakCount, audioDuration, audioOffset, getClipSourceSpan(clipWindow)); | ||
| if (peakSlice.numBars === 0) return null; |
There was a problem hiding this comment.
logicalPeakCount = Math.floor(peaks.length / PEAK_STRIDE) will be non-zero for legacy mono peak arrays (e.g. 1024 old-style peaks), so this code will silently interpret legacy data as interleaved min/max and render an incorrect waveform. If you need backward compatibility with existing saved projects/assets, add an explicit format check (e.g. peaks.length === CLIP_WAVEFORM_PEAK_COUNT or a stored version) and convert legacy magnitudes into the new [max,min,max,min] structure before rendering.
| function getVisiblePeakSlice( | ||
| logicalPeakCount: number, | ||
| audioDuration: number, | ||
| audioOffset: number, | ||
| sourceSpan: number, | ||
| ) { | ||
| if (logicalPeakCount === 0 || audioDuration <= 0) { | ||
| return { startPeakIdx: 0, numBars: 0 }; | ||
| } | ||
|
|
||
| const startPeakIdx = Math.floor((audioOffset / audioDuration) * logicalPeakCount); | ||
| const visibleAudioSec = Math.min(sourceSpan, Math.max(0, audioDuration - audioOffset)); | ||
| const endPeakIdx = Math.min( | ||
| Math.ceil(((audioOffset + visibleAudioSec) / audioDuration) * logicalPeakCount), | ||
| logicalPeakCount, | ||
| ); | ||
|
|
||
| return { | ||
| startPeakIdx, | ||
| numBars: Math.max(0, endPeakIdx - startPeakIdx), | ||
| }; | ||
| } |
There was a problem hiding this comment.
getVisiblePeakSlice doesn’t clamp audioOffset into [0, audioDuration], so startPeakIdx can become negative or exceed logicalPeakCount (e.g. from rounding or invalid clip state). That can lead to out-of-range peak indices and empty/incorrect rendering. Clamp audioOffset (and the derived startPeakIdx) to valid bounds before computing the slice.
| }, [clip.id, selectClip]); | ||
| const isMultiSelect = e.metaKey || e.ctrlKey; | ||
| selectClip(clip.id, isMultiSelect); | ||
| useUIStore.getState().selectTrack(track.id, isMultiSelect); |
There was a problem hiding this comment.
Calling selectTrack() here will set lastSelectionContext to 'tracks' (see uiStore.selectTrack), even though the user clicked a clip. That can change subsequent shortcut behavior (e.g. Cmd+A selecting tracks instead of clips). Consider selecting the parent track in a way that preserves the clip selection context (e.g. a dedicated helper that updates selectedTrackIds without touching lastSelectionContext, or re-setting the context back to 'clips' after selecting the track).
| useUIStore.getState().selectTrack(track.id, isMultiSelect); | |
| useUIStore.getState().selectTrack(track.id, isMultiSelect); | |
| // Preserve clip selection context even though we also select the parent track | |
| useUIStore.setState({ lastSelectionContext: 'clips' }); |
| expect(result.audioKey).toBe('bounce-audio-key'); | ||
| expect(result.duration).toBe(3); | ||
| expect(result.waveformPeaks).toHaveLength(1024); | ||
| expect(result.waveformPeaks).toHaveLength(1024 * 4); |
There was a problem hiding this comment.
This assertion bakes in both the peak-count (1024) and the stride (4). To make the test resilient to future format tweaks, consider asserting against CLIP_WAVEFORM_PEAK_COUNT * PEAK_STRIDE (importing the constants) instead of raw literals.
Summary
Closes #678, #679, #680, #681
Test plan
🤖 Generated with Claude Code