fix: close round-3 review findings — smallest-target-first, residue test fix, extraction

Waveform overlay now resolves node/tab/marker click collisions by target area instead of check order; residue test now uses a distinguishing fixture; Gate-unavailable-while-drawn logic extracted to one pure helper shared by resolvePlay and applyControl.

src/core/instrument/engine/play_params.h

@@ -195,6 +195,17 @@ bool splineActive(const Play& p) {
            (p.filter.enabled && p.filterSpline.mode == EnvMode::Spline);
 }
 
+// The one enforcement of the rule above: forces `p` to Trigger the instant it goes
+// splineActive. Every path that can leave `p` in a state splineActive() would newly return true
+// for — resolving stored params toward the engine, or an editor control edit that can flip a
+// mode/enable toggle — routes through this, so none of them can reopen the Gate+spline hole
+// splineActive's own doc comment describes. Header-inline and allocation-free: play_params.h
+// sits on the per-voice-per-sample include path.
+template <class Play>
+void enforceGateUnavailableWhileDrawn(Play& p) {
+    if (splineActive(p)) p.playMode = PlayMode::Trigger;
+}
+
 // [start, end) frames, half-open. A zero-length loop (start == end) is the "no sustain loop"
 // marker — a held note past the sample end goes silent rather than looping a zero span.
 struct SampleLoop {

src/core/instrument/engine/velocity_curve.h

@@ -232,13 +232,14 @@ public:
     // True once the cursor has settled on the contour's LAST segment. On its own this does NOT
     // make a 0 read here a terminus: the final segment's LEFT endpoint can also be 0 (a 2-point
     // contour is nothing but a single "final" segment starting at frame 0), which would read 0
-    // while about to rise. Voice::tickAmplitude pairs this with terminalValue() == 0 — the
+    // while about to rise. Voice::tickAmplitude pairs this with segmentEndValue() == 0 — the
     // segment's RIGHT endpoint, i.e. the whole contour's true end — before calling a 0 read the
     // note's genuine permanent terminus.
     bool onFinalSegment() const { return seg_ + 2 == n_; }
-    // The current segment's right endpoint — the whole contour's terminal Y only when paired
-    // with onFinalSegment() (see there).
-    double terminalValue() const { return y1_; }
+    // The CURRENT SEGMENT's right endpoint — not a contour-level concept despite the name's
+    // shape; it is the whole contour's terminal Y only when paired with onFinalSegment() (see
+    // there). Named for what it returns, not for its one call site's use of it.
+    double segmentEndValue() const { return y1_; }
 
     // `phase` is normalized position over the contour's whole span, [0,1]; out-of-range clamps
     // to the terminal values (a note past its span holds the contour's last level).

src/core/instrument/engine/voice.h

@@ -184,18 +184,11 @@ private:
         // entirely — release() then has no envelope to end, and an active sustain loop rings
         // forever.
         if (ampSplineCur_.active() && playMode_ == PlayMode::Trigger) {
-            // A contour covers the sample end to end, so the head leaving the span IS the end of
-            // the note — the exhaustion path in advanceFrame is what frees the voice. A contour
-            // whose TERMINAL value (the final segment's right endpoint) is 0 reaches a genuine
-            // permanent terminus early, the spline analogue of a staged AHD's finished(). Gating
-            // on the terminal value, not just the segment index, matters because a 2-point
-            // contour IS a single "final" segment from frame 0 — checking onFinalSegment() alone
-            // would call a contour that STARTS at 0 (e.g. a fade-in) over before it ever rises.
-            // Mid-contour dips through 0 still don't free early, since the spline is deliberately
-            // not globally monotone.
+            // Early-free at a genuine permanent terminus (the spline analogue of a staged AHD's
+            // finished()) — onFinalSegment()/segmentEndValue()'s own doc comments own the why.
             amp = ampSplineCur_.eval(splinePhase());
             if (amp == 0.0 && ampSplineCur_.onFinalSegment() &&
-                ampSplineCur_.terminalValue() == 0.0) {
+                ampSplineCur_.segmentEndValue() == 0.0) {
                 amplitudeDone_ = true;
             }
         } else if (playMode_ == PlayMode::Gate) {

src/core/instrument/map/sample_map.cpp

@@ -262,10 +262,10 @@ PlayParams resolvePlay(const PlaySeconds& stored, int sampleRate) {
     out.ampSpline    = stored.ampSpline;
     out.pitchSpline  = stored.pitchSpline;
     out.filterSpline = stored.filterSpline;
-    // Gate is unavailable while any EG is drawn — see splineActive (play_params.h) for why.
-    // The editor refuses the Gate segment for the same reason; enforcing it HERE as well is
-    // what keeps a hand-edited or downgraded blob from reaching the engine as Gate + spline.
-    if (splineActive(stored)) out.playMode = PlayMode::Trigger;
+    // Every field splineActive reads on `out` is already copied from `stored` above, so this
+    // enforces the same rule enforceGateUnavailableWhileDrawn's doc comment (play_params.h)
+    // describes — the editor's applyControl is the other caller, so the two cannot drift.
+    enforceGateUnavailableWhileDrawn(out);
     return out;
 }
 

src/core/instrument/ui/CMakeLists.txt

@@ -24,8 +24,10 @@ reasampler_pure_library(waveform_view
     SOURCES waveform_view.cpp
     LINK PUBLIC editor_geometry peaks PRIVATE sample_bands)
 # sample_bands is linked directly here because the test exercises the lane metrics that
-# waveform_view does not re-export.
-reasampler_test(waveform_view LINK waveform_view sample_bands)
+# waveform_view does not re-export. velocity_curve is linked for the smallest-target-first
+# arbitration tests, which pin the geometric facts editor_input_waveform.cpp's node/tab/marker
+# resolution depends on (the shell itself has no test target).
+reasampler_test(waveform_view LINK waveform_view sample_bands velocity_curve)
 
 reasampler_pure_library(browser_scroll
     SOURCES browser_scroll.cpp

src/shell/instrument/editor_controls.cpp

@@ -274,8 +274,8 @@ void ReaSamplerEditor::applyControl(int id, PlaySeconds& play, double value,
     // (above) or an enable toggle (kPitchEnvEnable/kFilterEnable), whose enabling can make an
     // already-Spline pitch/filter envelope newly active. Applying it once here, rather than at
     // each site that could cause the flip, is what keeps a future such control from reopening
-    // the same hole.
-    if (splineActive(play)) play.playMode = PlayMode::Trigger;
+    // the same hole. `resolvePlay` (sample_map.cpp) is the other caller of the shared helper.
+    enforceGateUnavailableWhileDrawn(play);
 }
 
 double ReaSamplerEditor::liveSampleRate() const {

src/shell/instrument/editor_input_waveform.cpp

@@ -12,7 +12,8 @@
 #include <cstdint>
 #include <vector>
 
-#include "core/instrument/engine/loop/loop_span.h"  // maxCrossfade (the shared drag-clamp bound)
+#include "core/instrument/engine/loop/loop_span.h"     // maxCrossfade (the shared drag-clamp bound)
+#include "core/instrument/engine/velocity_curve.h"      // kCurveNodeGrabRadius (target-size arbitration)
 #include "core/instrument/ui/envelope_edit.h"   // nodeAtPoint / resolveNodeDrag
 #include "core/instrument/ui/spline_edit.h"     // the shared point-editing grammar
 #include "core/instrument/ui/waveform_view.h"   // waveformOverlayArea / markerAtPoint / snap
@@ -24,6 +25,7 @@ namespace reasampler::vst {
 using namespace reasampler::ui;
 using namespace reasampler::instrument::ui;
 using instrument::engine::loop::maxCrossfade;
+using instrument::engine::kCurveNodeGrabRadius;
 
 bool ReaSamplerEditor::mouseDownWaveform(const FaceLayout& fl, int x, int y) {
     const std::vector<AudioSample>& pcm = monoPcmFor(selectedId_);
@@ -59,33 +61,65 @@ bool ReaSamplerEditor::mouseDownWaveform(const FaceLayout& fl, int x, int y) {
             return true;  // node moves once the cursor drags
         }
     }
-    // An existing contour node's grab/toggle/delete runs BEFORE the markers — mirroring
-    // markerHandleRect's tab-vs-column split (below): a coincident pixel (the default contour
-    // endpoint sits at the same x as the default start marker) is resolved by asking the
-    // NARROWER target first. pointAtPixel's pick radius is a small box around the node's own
-    // (x, y), not a full-height column, so this claims only genuine node hits — the marker's
-    // column stays grabbable at every other y along the same x. Never add here (kAdd is only
-    // tried once the markers have also passed on the click, below).
-    if (splineLive && splineOverlayClick(overlay, x, y, gesture, /*addOnEmptySpace=*/false)) {
-        return true;
+    const SetupMarkers m = pickedMarkers(frames);
+    const std::int64_t markerFrames[3] = {m.start, m.loopStart, m.loopEnd};
+
+    // Three affordances can claim the same pixel: a contour node (a small fixed pick box), the
+    // crossfade tab (a small clipped top-strip tab), and a marker's full-height grab column
+    // (waveform_view.h's tab-vs-column split already keeps the tab apart from ITS OWN column;
+    // this is the cross-affordance case on top of that). Resolving by any fixed check order
+    // shadows whichever one loses the tie — this seam regressed twice from exactly that fix.
+    // Instead measure each claimant's own target area and let the SMALLEST hit win: the marker
+    // column is the odd one out (its target is the whole band height), so it only wins where
+    // nothing narrower also claims the pixel. Never add here (kAdd is only tried once nothing
+    // else has claimed the click, below).
+    struct Candidate {
+        bool hit = false;
+        std::int64_t area = 0;
+    };
+
+    Candidate node;
+    if (splineLive) {
+        const VelocityCurve::Box box = splineOverlayBox(overlay);
+        // Also require strict in-box, matching splineOverlayClick's own narrowing (spline_edit.h's
+        // grammar note) — otherwise this candidate could "win" the arbitration below for a click
+        // splineOverlayClick would then refuse, silently swallowing it instead of falling through
+        // to the tab/marker checks.
+        if (contains(overlay.rect, x, y) && splineFor(overlayEnv_).pointAtPixel(box, x, y) >= 0) {
+            node.hit = true;
+            constexpr std::int64_t side = 2 * kCurveNodeGrabRadius + 1;
+            node.area = side * side;
+        }
     }
 
-    const SetupMarkers m = pickedMarkers(frames);
-    // The crossfade handle first, and only when there IS a loop to fade: at a zero fade it
-    // sits exactly on the loop start, so it can only stay reachable by owning the top strip
-    // (waveform_view.h's handle-vs-column split) and being asked first. The same ambiguity
-    // recurs whenever ANY marker's frame lands on loopStart - crossfade (most plausibly the
-    // start marker dragged up against the fade edge), so this check has to run before the
-    // marker array below regardless of which marker the collision is with.
-    if (m.hasLoop &&
-        contains(markerHandleRect(overlay, frames, m.loopStart - m.crossfade), x, y)) {
+    Candidate tab;
+    const Rect tabRect =
+        m.hasLoop ? markerHandleRect(overlay, frames, m.loopStart - m.crossfade) : Rect{};
+    if (m.hasLoop && contains(tabRect, x, y)) {
+        tab.hit = true;
+        tab.area = static_cast<std::int64_t>(tabRect.width) * tabRect.height;
+    }
+
+    const int markerHit = markerAtPoint(overlay, frames, markerFrames, 3, x, y);
+    Candidate marker;
+    if (markerHit >= 0) {
+        marker.hit = true;
+        marker.area = static_cast<std::int64_t>(2 * kMarkerGrabWidth + 1) * overlay.rect.height;
+    }
+
+    // Node checked before marker on an area tie so a degenerate (zero-height) overlay still
+    // prefers the node — unreachable in practice (sample_bands floors the band well above it),
+    // kept only so this arbitration has one well-defined answer for every input, not just the
+    // ones the current geometry constants happen to produce.
+    if (node.hit && (!tab.hit || node.area <= tab.area) && (!marker.hit || node.area <= marker.area)) {
+        return splineOverlayClick(overlay, x, y, gesture, /*addOnEmptySpace=*/false);
+    }
+    if (tab.hit && (!marker.hit || tab.area <= marker.area)) {
         beginMarkerDrag(WaveMarker::kLoopXfade, m, frames, x);
         return true;
     }
-    const std::int64_t markerFrames[3] = {m.start, m.loopStart, m.loopEnd};
-    const int hit = markerAtPoint(overlay, frames, markerFrames, 3, x, y);
-    if (hit >= 0) {
-        beginMarkerDrag(static_cast<WaveMarker>(hit), m, frames, x);
+    if (marker.hit) {
+        beginMarkerDrag(static_cast<WaveMarker>(markerHit), m, frames, x);
         return true;
     }
     // Nothing else wanted the click: now the drawn contour may take the empty space.
@@ -98,10 +132,7 @@ bool ReaSamplerEditor::splineOverlayClick(const OverlayArea& waveArea, int x, in
     const VelocityCurve::Box box = splineOverlayBox(waveArea);
     VelocityCurve& contour = splineFor(overlayEnv_);
     SplineEdit edit = resolveSplineEdit(contour, box, gesture, x, y);
-    // resolveSplineEdit's outside-box grab/delete/toggle allowance (pointAtPixel's radius has
-    // no box check of its own) was designed for the popup's inset ring; the overlay box has NO
-    // inset (splineOverlayBox), so honoring it here would extend the grab halo 6px into the
-    // inter-band pad. kAdd already requires in-box (resolveSplineEdit's own check).
+    // The overlay's outside-box narrowing — see spline_edit.h's grammar note for why.
     if (edit.kind != SplineEditKind::kNone && edit.kind != SplineEditKind::kAdd &&
         !(x >= box.left && x < box.left + box.width && y >= box.top && y < box.top + box.height)) {
         edit = SplineEdit{};