Skip to content

TT-7770 select pasted region after paste is done - #755

Merged
gtryus merged 2 commits into
developfrom
TT-7770
Oct 13, 2026
Merged

gtryus merged 2 commits into
developfrom
TT-7770

Conversation

@sarahentzel

Copy link
Copy Markdown
Collaborator

No description provided.

@sarahentzel
sarahentzel marked this pull request as ready for review October 12, 2026 22:20
@sarahentzel
sarahentzel requested a balanced review from Copilot October 12, 2026 22:20

Copilot AI 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.

🟡 Changes recommended

The visual selection can be serialized and saved as an overlapping segment in multi-region mode.

1 open finding
What changed in this PR

Selects the newly pasted audio span in the waveform.

Changes:

  • Calculates the pasted clip’s resulting time range.
  • Adds and selects a Wavesurfer region for that range.
  • Adds unit tests for selection behavior.
File Description
web/​src/​crud/​audioClipboard.ts Calculates the pasted selection range.
web/​src/​crud/​audioClipboard.test.ts Tests selection-range calculation.
web/​src/​crud/​useWaveSurfer.tsx Selects the pasted span after insertion.
web/​src/​crud/​useWavesurferRegions.tsx Adds the region-selection API.
web/​src/​crud/​useWavesurferRegions.test.tsx Tests pasted-span region selection.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread web/src/crud/useWavesurferRegions.tsx Outdated

@gtryus gtryus 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.

I've read the diff and the region-handling code around it; writing up the review now.

Review: TT-7770 "select pasted region after paste is done"

The branch has one commit touching 5 files (audioClipboard.ts, useWaveSurfer.tsx, useWavesurferRegions.tsx and two test files).

You Might Not Need an Effect: passes

  • No new useEffect. The selection is made in the same flow as the paste: wsPaste awaits the insert, then calls wsSelectSpan (useWaveSurfer.tsx:988-989). The article recommends this ("logic caused by a particular interaction belongs in the event handler"). The alternative it warns against would be an effect that watches the buffer or a "just pasted" flag.
  • Derived values are computed, not stored. pastedSelection is a pure function of span, the clip length and the sample rate. Nothing new goes into state.

Maintainability (docs/claude-maintain.md)

1. wsSelectSpan repeats work the region-created handler already does. This is the main issue.
wavesurfer's addRegion emits region-created before it returns. Your existing handler at useWavesurferRegions.tsx:573-605 already:

  • sets r.drag = singleRegionRef.current
  • in single-region mode, removes the old current region and moves the playhead to r.start
  • after waitForIt, calls setCurrentRegion(r) and onRegion(numRegions(), true), unless loadingRef.current is set

wsSelectSpan (useWavesurferRegions.tsx:1245-1258) then runs drag: singleRegionRef.current, setCurrentRegion(created) and onRegion(...) itself. Results:

  • onRegion fires twice per paste: once right away, once after the async wait. That's harmless today, but a consumer that counts or toggles on it would break.
  • The rules for "a region became the selection" now live in two places. A future change to one (for example the TT-7666 recording lock) can quietly miss the other.

Suggested fix: If loadingRef is reliably false when wsSelectSpan runs, drop the explicit setCurrentRegion, onRegion and drag and let the handler do the work. If you keep them because loadingRef may still be true (it's cleared in the ready handler, useWaveSurfer.tsx:438, and the order relative to await load() isn't obvious), add a one-line comment saying so. That's the "comment why" rule.

2. Risk in multi-region mode. In multi-region mode, wsSelectSpan would add a real segment, and the handler's later setCurrentRegion(findRegion(progress(), true)) could replace the selection. Today paste appears to be reachable only from MediaRecord (allowRecord makes singleRegionOnly true), so this is not a current bug. Consider having wsSelectSpan return early when !singleRegionRef.current so the assumption is explicit.

3. Minor items

  • wsSelectSpan returns a boolean that the only caller ignores. Either use it or return void.
  • pastedSelection declares { start: number; end: number } | undefined inline. pasteSpan's region parameter uses the same shape, so a shared named type (e.g. TimeSpan) would avoid the duplicate.
  • pastedSelection recomputes Math.floor(span.start * sampleRate) and so assumes insertAudioData rounds to a frame the same way. A short note would protect against drift.

4. Done well

  • DRAG_SELECTION_COLOR replaces a magic string and is shared with enableDragSelection.
  • pastedSelection is a small, pure, exported helper with tests for its boundary cases: replace, insert, empty waveform and an empty clip.

5. Tests

  • The pastedSelection tests check behaviour, which is good.
  • The hook test asserts on internal callbacks (onRegion, onCurrentRegion) and uses ws: any with 12 positional arguments. That's consistent with the rest of the file, but it's tightly coupled. The positional argument list is an existing problem, not one this commit added. It also passes onRegion(1, true) without showing whether the double call from item 1 happens, because the mock plugin may not emit region-created.

Verdict: It follows the effects guidance. Before merging, fix item 1: either remove the duplication or document why it's there.

don't reselect region after paste if not in singleRegionMode
@sarahentzel
sarahentzel requested a review from gtryus October 13, 2026 14:37
@gtryus
gtryus merged commit 476158f into develop Oct 13, 2026
2 checks passed
@gtryus
gtryus deleted the TT-7770 branch October 13, 2026 14:44
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.

3 participants