TT-7761 TT-7762 TT-7763 TT-7765 cut paste buttons disabled properly

[sarahentzel]

No description provided.

[Copilot]

🟡 Changes recommended

Paste remains available during replacement-audio loading and can discard the incoming audio.

1 open finding

• Block paste until the player is ready · New

What changed in this PR

Updates waveform editing controls so cut, copy, and paste respect selection and busy states.

Changes:

• Tracks region selection and active paste operations.
• Disables editing during playback, saving, or duplicate paste attempts.
• Adds hotkey and button-state tests.

File	Description
web/​src/​components/​WSAudioPlayer.tsx	Updates waveform edit guards and control states.
web/​src/​components/​WSAudioPlayer.hotkeys.test.tsx	Tests selection, loading, playback, saving, and repeated paste behavior.

🧠 Review effort: Balanced

---

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

[gtryus]

I reviewed the four commits that touch just WSAudioPlayer.tsx and its hotkey test.

• The hasRegion → regionCount rename is cosmetic.

Smaller points, in the spirit of the article

1. regionSelected duplicates existing state. regionSelectedRef.current always equals currentSegmentRef.current !== undefined, except right after the blob-reset effect, which clears one but not the other. The article's "avoid redundant state" idea applies here. You could clear currentSegmentRef in the reset and have the handlers read currentSegmentRef.current directly. You would then need only the regionSelected state, which the render uses, and not the extra ref. As written, the two values can disagree after a blob change.

2. The new line in the blob effect is acceptable. setRegionSelected(false) is the article's "adjusting state when a prop changes" pattern. Here it sits inside an effect that already syncs with an external system (wsLoad / wsClear), so it adds no extra render pass. If wavesurfer already sends onCurrentSegment(undefined) when it loads or clears, the line is redundant, but I didn't check whether it does.

3. The enable rules are written twice and no longer match. This is the biggest practical issue. It isn't an effect problem, but it is the article's "compute it once during render" point:

   	Hotkey handler checks	Button disabled checks
   Cut	readyRef, regionSelectedRef	regionSelected (no ready check)
   Copy	regionSelectedRef (no ready check)	regionSelected
   Paste	readyRef, wsHasClipboard(), playingRef, mediaSaveInProgress, pasteBusyRef	playing, mediaSaveInProgress, pasteBusy, with canPaste hiding the button; no ready check

   For example, one commit blocks pasting by hotkey while the player isn't ready, but the Paste button stays enabled during that time. Clicking it does nothing, because handlePaste returns early. A fix would be to compute canCut, canCopy and canPaste once per render, use them for disabled, and have the handlers read the same values, either through a ref updated during render or by passing ready into the handler's dependencies.

4. The state-plus-ref pairs are justified. regionSelected/regionSelectedRef and pasteBusy/pasteBusyRef each have a combined setter. The state drives rendering and the ref lets hotkey callbacks stay stable. That matches existing code like setReady, so it's consistent, even though the article would prefer reading state directly.

Bottom line

The branch adds no effects and puts the new state changes in handlers, as the article advises. The one change worth making before merging is item 3: compute each button's enabled state once so the buttons and hotkeys agree. Item 1 is an optional cleanup.

[gtryus]

@sarahentzel were these the kinds of changes we talked about changing?

[sarahentzel]

only if the state variables are unnecessary and could be computed on the fly

[gtryus]

Both reviews are done. The two that matter most: Save stays enabled after a failed edit, and the selection-reset code writes a ref during render again. I checked both in the source.

Standards

The localization part follows docs/ai/localization.md. wsAudioPlayer.changeFailed is in alphabetical order, has an empty <target/> and a sourcefile context, was generated by the updater, and is read with useSelector.

Hard violation

• Ref written during render (WSAudioPlayer.tsx:612-616, §Effects / React purity). The blob/reset check runs during render and now calls setCurrentSegment(undefined). That helper sets currentSegmentRef.current, which goes against the previous commit a20c2c2 ("do not write refs on render"). The fix is to call setCurrentSegmentx(undefined) there and update the ref somewhere outside render.

Likely bugs

• Save is enabled after a failed edit (WSAudioPlayer.tsx:1576-1582). The catch branch calls setBlobReady(hasAudio) and setChanged(hasAudio), but onBlobReady never gets a new blob. The parent still has the pre-edit blob, or none after setMyMediaId(''). Saving would store audio that doesn't match the waveform, and the new test asserts that Save is enabled, which locks this in.
• wsDuration() can throw inside the catch. If it does, handleChanged rejects, and publishChanged voids that on the assumption that it never rejects.
• Untranslated error text (useAudioAi.ts:177, §Localized strings). The raw English 'offline' / 'no token' text reaches the user. Use ts.mustBeOnline or another localized string instead.

Judgement calls

• setCurrentSegmentx is an unclear name (§Naming, Mysterious Name). Something like setCurrentSegmentState would be clearer.
• Keeping both state and a mirroring ref for the current segment is justified, but it needs a comment saying why both exist (§State).
• WSAudioPlayer.tsx is about 3050 lines (§Modules). handleChanged and publishChanged could move into a hook.
• The "reports a failed edit" test resets the wsDuration mock on its last lines. If an assertion fails first, the reset never runs and the mock leaks into later tests; move it to afterEach (§Tests).
• Duplicated Code: the try and catch branches repeat the duration / hasAudio / setChanged block. Compute it once after the try/catch.
• Middle Man: publishChanged only wraps void handleChanged().
• Primitive Obsession: errors are told apart by message strings ('offline', 'no token', 'canceled'). An error code would be sturdier.
• Already there before this commit: no guard against stale results after await wsBlob(), and the mock repeats useWaveSurfer's 25 positional parameters (known debt).

Spec

There is no issue or spec file, so this review checked the code against the three intents in the commit message.

(a) Missing or partial

• "call AI cb with an error if offline/no token"
  • requestAudioAi already reported offline, so in practice this commit only adds the no-token case.
  • The non-S3 axiosPostFile branch has no token check, though that branch is currently unused.
  • No test covers the offline branch inside s3request.
  • No WSAudioPlayer test shows that a callback error clears the busy state.
• "handle or ignore errors from handleChanged"
  • All six call sites are covered.
  • Because handleChanged now catches its own errors, the outer try/catch around :1434 can never fire.
• "currentSegment needed state as well as ref"
  • Every write goes through setCurrentSegment, so state and ref can't drift apart.

(c) Implemented, but looks wrong

• The same three issues as above: Save enabled with a stale blob, the ref written during render, and the raw "no token"/"offline" text reaching the user (here through failAi).
• Tests cover only the cut path. Undo, paste, AI replace and a failed AI callback inside WSAudioPlayer are untested.

(b) Not asked for

• The new changeFailed string is needed for the new error report, and it went through the XLIFF pipeline correctly.
• The typed mockUseWaveSurfer and the mockGetGlobal typing are test refactors outside the three intents.
• The extended "disables copy and cut" test does belong to intent 2.

---

Summary: Standards has 1 hard violation, 3 likely bugs and 9 judgement calls; the worst is Save being enabled with a stale blob after a failed edit. Spec has 5 gaps, 3 questionable implementations and 2 test-only additions outside the stated intents; the worst is again Save after a failed edit, plus untranslated error text reaching the user.

[sarahentzel]

wsDuration() can throw inside the catch. If it does, handleChanged rejects, and publishChanged voids that on the assumption that it never rejects.
NO - this is the code for wsDuration: durationRef.current || 0; Not much chance of a throw.