Fix drag-handoff bugs: gate FX re-resolve on outside-panel, cache unresolvable OS-drag verdict, block double FX-add retry

src/core/ui/drag_out.cpp

@@ -50,8 +50,7 @@ PathList assemblePathList(const std::vector<ResolvedSample>& resolved) {
 
 OsHandoff decideOsHandoff(const std::vector<std::string>& paths) {
     OsHandoff out;
-    out.startOsDrag = !paths.empty();
+    out.handOffToOs = !paths.empty();
-    out.releaseInternalDrag = out.startOsDrag;
     return out;
 }
 

src/core/ui/drag_out.h

@@ -71,13 +71,15 @@ PathList assemblePathList(const std::vector<ResolvedSample>& resolved);
 
 // --- OS hand-off ordering -----------------------------------------------------
 
-// The two side effects the shell performs when a drag crosses out of REAPER. They are ONE
+// Whether an empty/unresolvable payload should hand off to the OS at all. This decides ONLY
-// decision, not two: winding the internal drag down (release capture, clear drag state) for a
+// the empty-payload third of the failure space — an unresolvable payload must leave the
-// hand-off that then cannot happen consumes the gesture — the user sees a drag that silently
+// internal drag live rather than winding it down (release capture, clear drag state) for a
-// did nothing and drags again. Never release without starting.
+// hand-off that then never happens, which reads to the user as "the drag did nothing, try
+// again". A resolved-but-OS-not-ready hand-off (OLE unavailable, HDROP build failure) is a
+// separate, shell-side readiness gate (drag_out_win::canInitiateDragOut) checked BEFORE the
+// shell tears down internal drag state — this struct does not model that path.
 struct OsHandoff {
-    bool startOsDrag = false;         // hand `paths` to the OS drag initiator
+    bool handOffToOs = false;  // true: wind down internal drag state, then start the OS drag
-    bool releaseInternalDrag = false; // first wind down mouse capture + panel drag state
 };
 
 // Decides the hand-off from the assembled path list. Empty (everything stale/unresolvable)

src/shell/actions/drag_out_win.cpp

@@ -64,10 +64,11 @@ HGLOBAL buildHDrop(const std::vector<std::string>& paths) {
 
 // COM reference counts MUST be interlocked. A CF_HDROP target is free to marshal the data
 // object into another apartment and finish the copy on a background thread AFTER DoDragDrop
-// has returned (Explorer's async file copy does exactly this). A plain ++/-- there races the
+// has returned; Explorer's async file copy is suspected to do exactly this, though that
-// source thread's post-DoDragDrop Release: one lost increment destroys the object — and with
+// specific behavior is not confirmed by experiment. A plain ++/-- there races the source
-// it the source HGLOBAL — before the target reads it, and the drop lands with no file. That
+// thread's post-DoDragDrop Release: one lost increment destroys the object — and with it the
-// race is intermittent and a retry usually wins it; do not "simplify" these back.
+// source HGLOBAL — before the target reads it, and the drop lands with no file. That race is
+// intermittent and a retry usually wins it; do not "simplify" these back.
 inline ULONG comAddRef(volatile LONG& refs) {
     return static_cast<ULONG>(InterlockedIncrement(&refs));
 }
@@ -234,6 +235,15 @@ bool initiateDragOut(HWND__* /*panelHwnd*/, const std::vector<std::string>& abso
     return hr == DRAGDROP_S_DROP && effect == DROPEFFECT_COPY;
 }
 
+bool canInitiateDragOut(const std::vector<std::string>& absolutePaths) {
+    if (absolutePaths.empty()) return false;
+    if (!ensureOleForThisThread()) return false;
+    HGLOBAL hdrop = buildHDrop(absolutePaths);
+    if (!hdrop) return false;
+    GlobalFree(hdrop);  // probe only — initiateDragOut builds its own on the real attempt
+    return true;
+}
+
 }  // namespace reasampler
 
 #else  // ---- macOS / Linux (SWELL) -----------------------------------------------------
@@ -260,6 +270,13 @@ bool initiateDragOut(HWND__* panelHwnd, const std::vector<std::string>& absolute
     return true;  // fire-and-forget; SWELL owns the drag from here (no accept/cancel return)
 }
 
+// SWELL exposes no readiness probe ahead of SWELL_InitiateDragDropOfFileList (which itself
+// reports no accept/cancel outcome) — the non-empty check is the only thing knowable in
+// advance on this platform.
+bool canInitiateDragOut(const std::vector<std::string>& absolutePaths) {
+    return !absolutePaths.empty();
+}
+
 }  // namespace reasampler
 
 #endif

src/shell/actions/drag_out_win.h

@@ -26,4 +26,13 @@ namespace reasampler {
 // (DROPEFFECT_COPY); the return is advisory — a failed drag surfaces no error.
 bool initiateDragOut(HWND__* panelHwnd, const std::vector<std::string>& absolutePaths);
 
+// Cheap, side-effect-free readiness probe: true iff initiateDragOut would actually be able to
+// START a drag for `absolutePaths` right now. Call this BEFORE tearing down internal drag
+// state (release capture, clear drag fields) — an OS that isn't ready (OLE unavailable, or the
+// path list can't build a CF_HDROP) must not consume the gesture the same way an empty payload
+// would. On Windows this re-runs the same OLE-init + HDROP-build checks initiateDragOut does,
+// freeing the probe HGLOBAL immediately; SWELL exposes no such probe, so macOS/Linux reduces to
+// the non-empty check alone.
+bool canInitiateDragOut(const std::vector<std::string>& absolutePaths);
+
 } // namespace reasampler

src/shell/actions/instrument_drop_win.cpp

@@ -104,16 +104,23 @@ bool loadInstrumentOntoTrack(MediaTrack* track, const std::vector<std::uint8_t>&
     const std::string fxName = "VST3:" + vstPluginName();
 
     // An EXPLICIT top-level insertion position (instantiate <= -1000 IS the position, -1000
-    // = first in chain), not the bare -1. Both always create a new instance; the bare form
+    // = first in chain), not the bare -1 — this form is documented in the SDK header. Both
-    // additionally leaves placement to REAPER's ambient FX-chain insert point, which a drop
+    // always create a new instance; the bare form additionally leaves placement to REAPER's
-    // onto an FX container/chain-window moves — so the index handed to TrackFX_SetPreset and
+    // ambient FX-chain insert point, which a drop onto an FX container/chain-window is
-    // the instance just created stop denoting the same FX and the capture never lands. The
+    // suspected (unconfirmed by experiment) to move — if so, the index handed to
-    // bare-form retry keeps the reference path alive if the positional form is ever refused.
+    // TrackFX_SetPreset and the instance just created would stop denoting the same FX and the
+    // capture would never land. The bare-form retry keeps the reference path alive if the
+    // positional form is ever refused.
     // recFX = false: a normal track FX chain instance, not a record/monitoring FX.
     const int insertPos = TrackFX_GetCount(track);
     int fxIndex = TrackFX_AddByName(track, fxName.c_str(), /*recFX=*/false,
                                     /*instantiate=*/-1000 - insertPos);
-    if (fxIndex < 0)
+    // Only retry with the bare form when the chain is PROVABLY unchanged (count still
+    // insertPos): a negative return with the count grown means the positional add DID create
+    // an instance and just reported -1 — retrying then would add a SECOND instance, leaving
+    // the first orphaned (no preset applied, unreachable for rollback), which is exactly the
+    // all-or-nothing violation this contract forbids.
+    if (fxIndex < 0 && TrackFX_GetCount(track) == insertPos)
         fxIndex = TrackFX_AddByName(track, fxName.c_str(), /*recFX=*/false, /*instantiate=*/-1);
 
     // u8string() gives UTF-8 bytes on MSVC (not ACP-converted), so a temp dir under

src/shell/panel/panel_drag.cpp

@@ -297,14 +297,32 @@ void onMouseMove(int x, int y) {
         g_panel.instrumentDropTrack = nullptr;
 
         if (gesture == DragGesture::OsDrag) {
+            if (g_panel.dragOsHandoffBlocked) {
+                // Already known un-hand-off-able for this gesture (empty/unresolvable payload,
+                // or the OS wasn't ready) — skip the fs::exists work and the readiness probe on
+                // every move; keep the drag alive with no drop-target highlight.
+                g_panel.dropKind = DropKind::None;
+                g_panel.dropBankId.clear();
+                invalidatePanel();
+                return;
+            }
+
             // Resolve the payload to existing on-disk paths BEFORE tearing down internal
             // drag state (the resolver reads dragSourceBankId / dragSampleIds), then let the
             // pure rule couple the two side effects: an unresolvable payload must leave the
             // internal drag intact rather than wind it down for a hand-off that never runs —
             // a half-torn-down drag reads to the user as "the drag did nothing, try again".
+            // canInitiateDragOut extends the same coupling to the OS-readiness failure modes
+            // (OLE unavailable, HDROP build failure) that the pure decision cannot see.
             const std::vector<std::string> paths = resolveDragPathsForOs();
             const ui::OsHandoff handoff = ui::decideOsHandoff(paths);
-            if (!handoff.releaseInternalDrag) return;
+            if (!handoff.handOffToOs || !canInitiateDragOut(paths)) {
+                g_panel.dragOsHandoffBlocked = true;
+                g_panel.dropKind = DropKind::None;
+                g_panel.dropBankId.clear();
+                invalidatePanel();
+                return;
+            }
 
             // DoDragDrop runs its own modal loop and takes over mouse capture, so the internal
             // drag must be fully wound down first.
@@ -318,8 +336,7 @@ void onMouseMove(int x, int y) {
             g_panel.dragPrimaryId.clear();
             invalidatePanel();
 
-            if (handoff.startOsDrag)
+            initiateDragOut(g_panel.hwnd, paths);  // COPY-ONLY; blocking on Windows
-                initiateDragOut(g_panel.hwnd, paths);  // COPY-ONLY; blocking on Windows
             return;
         }
         // Inside the client: classify the in-grid gesture (reorder/replace vs move/copy) and
@@ -372,6 +389,7 @@ void resetDragState() {
     g_panel.dragTargetSlot = -1;
     g_panel.dragPrimaryId.clear();
     g_panel.instrumentDropTrack = nullptr;
+    g_panel.dragOsHandoffBlocked = false;
 }
 
 // Commits (or abandons) a drag on button-up. The resolved pure CardGesture decides:
@@ -388,12 +406,19 @@ void onLBtnUp(int x, int y) {
         //
         // Re-resolve at the RELEASE point rather than trusting only the hover-tracked target:
         // WM_MOUSEMOVE is coalesced, so a fast drag onto a dense surface (an FX chain row, a
-        // container) can release over a hotspot no processed move ever reported. Strictly
+        // container) can release over a hotspot no processed move ever reported. Gated on
-        // additive — a release-point miss falls back to the tracked target, so the
+        // !inside exactly like onMouseMove's live resolve — GetThingFromPoint can return a
-        // hover-then-release-on-the-FX-button path is untouched.
+        // track+FX hit at a release point that is still inside the panel's own client rect, and
+        // an in-grid release must always go through the reorder/replace/move/copy path below,
+        // never be reinterpreted as an FX add. Strictly additive otherwise — a release-point
+        // miss falls back to the tracked target, so the hover-then-release-on-the-FX-button
+        // path is untouched.
         const bool singleCapture = g_panel.dragSampleIds.size() == 1;
         MediaTrack* dropTrack = g_panel.instrumentDropTrack;
-        if (singleCapture) {
+        RECT cr{};
+        GetClientRect(g_panel.hwnd, &cr);
+        const bool inside = (x >= cr.left && x < cr.right && y >= cr.top && y < cr.bottom);
+        if (!inside && singleCapture) {
             POINT sp{x, y};
             ClientToScreen(g_panel.hwnd, &sp);
             const FxDropTarget fx = resolveFxDropTarget(sp.x, sp.y);

src/shell/panel/panel_state.h

@@ -320,6 +320,12 @@ struct PanelState {
     // when the pointer is not over an FX button.
     MediaTrack* instrumentDropTrack = nullptr;
 
+    // Latched once an OsDrag hand-off attempt for THIS gesture resolves to "cannot hand off"
+    // (empty/unresolvable payload, or the OS isn't ready) — skips re-running
+    // resolveDragPathsForOs (an fs::exists per sample) and the readiness probe on every
+    // subsequent move while the drag stays alive. Cleared by resetDragState.
+    bool dragOsHandoffBlocked = false;
+
     // Authoritative tail setting lives in ReaSamplerSession, not here; panel reads it for
     // drawing and mutates via footer click / scroll-wheel. bankPanelTailSetting is the
     // capture actions' read seam.

tests/test_drag_out.cpp

@@ -223,39 +223,26 @@ static void testMixedTallies() {
 
 // --- OS hand-off ordering -----------------------------------------------------
 //
-// The drag-out-lands-without-audio regression: the shell used to wind its internal drag down
+// The empty-payload teardown regression: the shell used to wind its internal drag down
 // (release capture, clear drag state) BEFORE checking whether the payload had resolved to any
-// on-disk file. A payload that resolved to nothing therefore consumed the gesture — the drag
+// on-disk file. For an unresolvable payload, initiateDragOut is never reached — no OS drop
-// died half-torn-down and the user saw a drop with no audio and dragged again. The two side
+// happens at all — but the teardown ran anyway, so the drag just silently stopped mid-gesture
-// effects are one decision; these pin that.
+// with no highlight and no drop. The user has to press and start an entirely new drag; retrying
+// the SAME stale selection resolves to the same empty payload and fails identically. These pin
+// the fix: the two side effects (teardown, hand-off) are one decision.
 
 // Nothing draggable -> hand off nothing AND keep the internal drag alive.
 static void testEmptyPathsHandsOffNothingAndKeepsInternalDrag() {
     const OsHandoff h = decideOsHandoff({});
-    CHECK(!h.startOsDrag);
+    CHECK(!h.handOffToOs);
-    CHECK(!h.releaseInternalDrag);
 }
 
-// A resolvable payload -> release the internal drag, then start the OS drag.
+// A resolvable payload -> hand off (release the internal drag, then start the OS drag).
 static void testResolvablePayloadHandsOff() {
     const OsHandoff one = decideOsHandoff({"C:/proj/bank/a.wav"});
-    CHECK(one.startOsDrag);
+    CHECK(one.handOffToOs);
-    CHECK(one.releaseInternalDrag);
     const OsHandoff many = decideOsHandoff({"a.wav", "b.wav", "c.wav"});
-    CHECK(many.startOsDrag);
+    CHECK(many.handOffToOs);
-    CHECK(many.releaseInternalDrag);
-}
-
-// The coupling itself: releasing without starting is the defect, so it must be unreachable
-// for every input the assembler can produce.
-static void testNeverReleasesWithoutStarting() {
-    const std::vector<std::vector<std::string>> inputs = {
-        {}, {"a.wav"}, {"a.wav", "b.wav"}, {""},
-    };
-    for (const std::vector<std::string>& in : inputs) {
-        const OsHandoff h = decideOsHandoff(in);
-        CHECK(!(h.releaseInternalDrag && !h.startOsDrag));
-    }
 }
 
 // End to end through the assembler: a selection whose every entry is stale/unresolvable
@@ -263,8 +250,7 @@ static void testNeverReleasesWithoutStarting() {
 static void testAllSkippedSelectionKeepsInternalDrag() {
     const PathList l = assemblePathList({missing("g1.wav"), unresolved()});
     const OsHandoff h = decideOsHandoff(l.paths);
-    CHECK(!h.startOsDrag);
+    CHECK(!h.handOffToOs);
-    CHECK(!h.releaseInternalDrag);
 }
 
 int main() {
@@ -292,7 +278,6 @@ int main() {
 
     testEmptyPathsHandsOffNothingAndKeepsInternalDrag();
     testResolvablePayloadHandsOff();
-    testNeverReleasesWithoutStarting();
     testAllSkippedSelectionKeepsInternalDrag();
 
     if (g_fail == 0) std::printf("All tests passed.\n");

tests/test_instrument_drop.cpp

@@ -237,29 +237,15 @@ static void testEmbedStripIsNotHotspot() {
 // --- Drop surface parity: container vs FX button ------------------------------
 //
 // The container-drop regression (instance loads, capture does not): the two surfaces must be
-// one code path carrying one payload. The payload is a function of the capture alone, so the
+// one code path carrying one payload. buildInstrumentDropPreset takes only sampleId, so the
-// surface cannot influence it — and both surfaces must classify as hotspots so both reach it.
+// payload side of that claim is already proven once by testPresetRoundTripsThroughInstrumentReader
-
+// and every "fx_"/"tcp.fx"/"mcp.fx" surface classifying as a hotspot is proven by
-// Every surface a drop can land on admits the SAME capture and yields a payload that decodes
+// testTcpMcpFxFamilyIsHotspot / testFxWindowStillHotspot. A loop that reruns both against a
-// back to it. The payload takes the capture and nothing else, so the surface cannot influence
+// fixed sampleId per surface string can't distinguish the surfaces (the loop body is identical
-// it — walking the surfaces here is what pins that the container family is not second-class.
+// every iteration) — it isn't a stronger test than those two, so there is no separate test here.
-static void testEveryDropSurfaceCarriesTheCapture() {
+// The one thing that DOES vary by surface — the shell's TrackFX_AddByName `instantiate` value
-    const char* surfaces[] = {
+// picked for a container drop vs. a bare FX-button drop — lives in instrument_drop_win.cpp and
-        "fx_chain",  // FX chain window / container list — the reported failing surface
+// is untestable without a live DAW (GetThingFromPoint/TrackFX_AddByName have no pure model).
-        "fx_0",      // floating FX window
-        "tcp.fx",    // TCP FX button — the reference path
-        "mcp.fx",    // MCP FX button
-    };
-    const std::string sampleId = "cap-7f3a";
-    for (const char* info : surfaces) {
-        CHECK(infoNamesFxHotspot(info));
-        const ParsedPreset p = parsePreset(buildInstrumentDropPreset(sampleId));
-        CHECK(p.ok);
-        CHECK(p.classId == vstClassIdHex());
-        const ComponentState cs = deserializeComponentState(p.compChunk, kRate);
-        CHECK(cs.selectionId == sampleId);
-    }
-}
 
 // --- All-or-nothing rollback --------------------------------------------------
 
@@ -314,7 +300,6 @@ int main() {
     testNonFxSurfacesAreNotHotspot();
     testEmbedStripIsNotHotspot();
 
-    testEveryDropSurfaceCarriesTheCapture();
     testAddFailureLeavesNothingToRollBack();
     testPresetFailureRollsBackTheCreatedIndex();
     testSuccessKeepsTheInstance();