Skip to content

Fix stuck tab ghost when drag input restarts - #692

Merged
kshivang merged 10 commits into
devfrom
fix/tab-drag-ghost-cleanup
Sep 25, 2026
Merged

kshivang merged 10 commits into
devfrom
fix/tab-drag-ghost-cleanup

Conversation

@kshivang

@kshivang kshivang commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

Extracts an owned drag-session abstraction used by the host's shared pointer handler. Cancellation, callback failures and stale handlers can clear only their own drag, and a refused second start cannot claim an existing gesture.

This branch includes #627's stable tab identity, live callbacks and current-index drop resolution. Merge #627 first, then this follow-up; both retain dev as their target so the normal PR CI runs. Title/index changes now preserve the intended drag through release, rather than cancelling it. The original implementation, investigation and contributor history are retained.

Adds real mouse coverage for chip reindexing/removal, full-button removal, component replacement, and starting another drag after interruption. Session tests cover coroutine teardown, exceptions, replacement ownership, refused starts and cross-pane release. Native-release cleanup already on dev is preserved.

Validation at 589146374: 36 focused tests pass with zero failures/skips; common-main and desktop-test ktlint plus composeApp detekt pass. Current-head GitHub checks must also pass. No application was launched; manual dragging over the hardware browser surface remains unverified.

Related: #690, #627, #1614. Investigation: docs/investigations/tab-drag-ghost-690.md.

@supabase

supabase Bot commented Sep 15, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project pcnwqamqdnsadranufjv because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@github-actions

Copy link
Copy Markdown
Contributor

Claude diff review of 6030e00 (no tests executed).

Review of PR #692 @ 6030e00

Diff-only review; I did not verify TabDraggableComponent, detectDragGestures call sites outside the diff, or that the new tests compile/pass.


Confirmed defects (reasoning is closed over the diff text itself)

1. session.start() can silently no-op while the caller is told a drag began

TabDragSession.kt:21 — if (component.isDragging) return leaves owned = null, so update/end/cancel all become no-ops for the whole gesture. But in BossTabButton.kt the surrounding code is unconditional:

session.start(tab = tabInfo, panelId = panelId, index = tabIndex, position = absolutePosition)
onDragStart()                     // fires even when start() bailed out
...
val result = session.end()        // returns null
onDragEnd(result)

So the parent gets onDragStart() with no drag behind it and later onDragEnd(null). Any caller state keyed off onDragStart (placeholder gap, "tab is dragging" styling) is entered without a corresponding drag. start() should return Boolean (or throw/ignore the gesture) and the callbacks should be gated on it.

2. Nothing in the new design can recover from foreign/stale drag state

Before this PR, two paths could clear state that some other surface left behind: the unconditional tabDragComponent.cancelDrag() in onDragCancel, and the DisposableEffect in BossTabButton (deleted, old lines 291–301). After this PR:

  • start refuses to start while isDragging (TabDragSession.kt:21),
  • update/end/cancel are all gated on isOwner (:27, :31, :37),
  • withDragSession's finally calls cancel(), which is also isOwner-gated (:48 → :37).

Net effect: if draggingTab is ever left set by a path whose owning coroutine is still alive (or whose cleanup never ran), every tab surface becomes permanently undraggable rather than recovering by overriding. The failure mode changes from "stuck ghost" to "drag silently dead", with no escape hatch in the diff. This is a real regression in robustness, and it is not covered by any test (see §7).

3. end() drops ownership before the risky call, re-opening the hole the PR claims to close

fun end(): TabDropResult? {
    if (!isOwner) return null
    owned = null                  // TabDragSession.kt:32
    return component.endDrag()    // :33
}

If endDrag() throws (drop-target resolution, listener dispatch, etc.), the exception unwinds through detectDragGestures into withDragSession's finally, where cancel() now sees owned == null and does nothing — exactly the stuck-ghost state the PR's own comment in TabFaviconChip.kt ("a result that throws must not leave a ghost stuck to the pointer") says must not happen. Clear owned after a successful endDrag(), or wrap in try { … } finally { owned = null }.

Minor companion: the non-owner branch at :31 returns without clearing owned, leaving a dangling reference until cancel() runs. Harmless today, inconsistent with cancel()'s unconditional owned = null at :38.

4. Cleanup resets the component but never notifies the composable callbacks

withDragSession (TabDragSession.kt:45-49) only calls component.cancelDrag(). On a pointerInput restart or disposal, neither onDragEnd nor onDragCancel (BossTabButton) nor onDragEnd (TabFaviconChip, TabFaviconChip.kt:266) is invoked. Combined with §1/§4, an interrupted gesture leaves caller-side state that was set in onDragStart() with no termination signal. The session should expose an "interrupted" hook, or withDragSession should take the cancel callback.

5. The root cause is still there; the fix converts a stuck ghost into a silently aborted drop

Both surfaces still key on the index: BossTabButton.kt Modifier.pointerInput(tabInfo, panelId, tabIndex) and TabFaviconChip.kt:247 pointerInput(tab, panelId, tabIndex). The investigation doc itself names this as the cause ("A changed index restarts its coroutine", docs/investigations/tab-drag-ghost-690.md). With the fix, a mid-gesture index change now runs finally → cancel() and aborts the user's in-flight drag — the drop never happens, and per §4 the caller is never told. Any live reorder preview, or a sibling tab closing in the same pane during a drag, hits this. A fix that survives the restart (key on tab.id/panelId, read tabIndex via rememberUpdatedState) would address the reported behaviour rather than only the visual symptom. As written, the PR should at least state that the drag is now cancelled rather than completed.


Uncertain observations (depend on code not in the diff)

  1. Identity-based ownership is fragile. isOwner compares with === (TabDragSession.kt:13) and owned is captured by reading component.draggingTab immediately after startDragging (:23). Two unverified assumptions: (a) startDragging publishes draggingTab synchronously — otherwise owned stays null and the session is inert for its whole lifetime, silently restoring the original bug; (b) DraggingTabInfo is never replaced by a copy during updateDrag — if position tracking is ever moved into that object (draggingTab = draggingTab.copy(...)), ownership breaks after the first update() and the drag freezes mid-gesture with a live ghost. The separate component.dragDelta assertions in TabDragSessionTest suggest (b) holds today, but the coupling is invisible from DraggingTabInfo's side; a monotonic session id or an explicit owner token would be safer.

  2. Cleanup is now dispatcher-dependent rather than synchronous. The deleted DisposableEffect.onDispose ran inline during composition disposal; withDragSession's finally runs only when the cancelled pointer coroutine is resumed on its dispatcher. For a pane/window teardown where that dispatcher stops pumping, the cleanup may never run. Narrow, but it is a behavioural change introduced by deleting the disposal hook.

  3. registerTabBarBounds in TabDragSessionTest (:95) is never unregistered. If bounds live in instance state this is fine; if any part of that registry is shared/companion-scoped, this test leaks a "target" pane into other tests and makes ordering matter.

  4. TabChipDragCancellationTest compile/robustness risk. The call site passes tab/isActive/onClick/size/tabDragComponent/panelId/tabIndex (:59-67) with no windowPosition; TabFaviconChip's signature isn't in the diff, so I can't confirm it matches. The assertion also depends on rule.waitForIdle() flushing the cancelled pointer coroutine's finally before runOnIdle (:73-75) — plausible with the test clock, but it is a cross-dispatcher ordering assumption and a likely flake source if it is wrong.


Test gaps

  1. No coverage for BossTabButton, which is the file where safety code was removed. Both new tests exercise TabFaviconChip only (TabChipDragCancellationTest.kt:59). The deleted DisposableEffect covered "composable disposed mid-drag" for the button; nothing replaces that assertion for that surface, and the onDragStart/onDragEnd pairing problem in §1/§4 lives exclusively in that file.
  2. No test that a drag can start after an interrupted one. This is the §2 lockout. TabChipDragCancellationTest.interruptedDrag stops at assertFalse(component.isDragging) (:75); it never re-presses and re-drags to prove the surface is still usable.
  3. handler that never started a drag cannot cancel another handler (TabDragSessionTest.kt:81-92) is mis-named and under-asserts. The handler does call session.start(...); it is the isDragging guard that no-ops. The test asserts only that the pre-existing drag is intact — not the user-visible consequence that the second surface's gesture is now inert and yet its caller-side onDragStart would have fired.
  4. No assertion that end() survives a throwing endDrag() (§3), and none that an interrupted gesture clears dropTarget/dragStartPosition in the Compose test (only the pure-coroutine test at TabDragSessionTest.kt:44-47 checks those fields).
  5. The investigation doc concedes manual cross-pane verification over the hardware browser surface "has not been reported" (docs/investigations/tab-drag-ghost-690.md, Validation section) — worth resolving before merge given §5 changes real drop behaviour.

Non-blocking

  • AGENTS.md:1256-1261 asserts withDragSession's finally "clears an interrupted gesture even when pointer input is cancelled or restarted" — accurate for the owner path, but overstated given §3 (owner already released) and §7 (dispatcher-dependent). Worth softening so the next reader does not rely on a guarantee the code doesn't make.
  • No security-relevant surface in this diff (no I/O, IPC, deserialization, or privilege changes); the only state touched is in-process drag state.

Recommendation: address §1, §3 and §4 before merge (small, local changes), decide explicitly on §2's no-recovery policy, and either fix or document §5.

@kshivang
kshivang changed the base branch from main to dev September 15, 2026 01:36
@kshivang

Copy link
Copy Markdown
Contributor Author

@kshivang Thanks for the concrete mouse-input reproduction of #690, the exact-instance session ownership, and the clear distinction between the reproduced restart and the unproven live incident. I reviewed head 6030e005f334700b200883d3ea8e0cbc669a84a9 and retargeted this PR to dev.

I am moving this to draft for focused reconciliation and acceptance coverage:

  1. Direct overlap with Stop a tab drag being stranded by the tab changing under it #627: its current head fc86cbeab changes these same two gesture handlers, adds finally/owner cleanup, and preserves title/index changes with live inputs and current-index drop resolution. This PR instead deliberately cancels on index/title restart. Its index test explicitly expects cancellation, so it does not establish that the user's intended drop completes. Please converge these approaches before shipping: preserve Stop a tab drag being stranded by the tab changing under it #627's live-input/current-index behavior and bring over this PR's shared session abstraction, refusal to acquire an already active drag, non-owner regression, and investigation evidence. Stop a tab drag being stranded by the tab changing under it #627 is outside this review's assignment and remains untouched; this is a consolidation recommendation, not a claim that either PR fully subsumes the other or that Stop a tab drag being stranded by the tab changing under it #627 is validated for merge.
  2. Full-button lifecycle coverage is missing: both new Compose tests render only TabFaviconChip, while BossTabButton loses its disposal hook. Add real button mouse tests for source removal/identity replacement, index/title change, and a second successful drag after interruption. Include replacement of tabDragComponent: the button's pointerInput keys omit that component even though the removed disposal effect keyed on it. Ensure old component state is cleared and subsequent gestures use the replacement. Confirm ordinary drops return the expected tab/index and stale handlers cannot cancel a replacement drag.
  3. Issue acceptance remains open: record cross-pane release/cancel/source-removal checks over the hardware browser surface for both chip and full-tab UI. No app was launched or stopped in this review, and that manual check is not claimed complete.

Claude review reconciliation: the claimed end() exception leak is not supported by the actual implementation—endDrag() resets state before dropResultFor. startDragging publishes synchronously, updates retain the same DraggingTabInfo, and bounds registries are instance-local. Refusing to steal an active drag is useful ownership protection, not evidence of a new permanent lockout. The unconditional start callback / missing cancellation callback is an API pairing concern, but I found no custom host onDragStart consumer proving the asserted visible state leak; do not treat that speculation as an established regression. The missing button/restart recovery coverage and continued index-key cancellation are real gaps to reconcile.

Validation: all 10 reported Build & Test checks pass on the submitted head (Supabase preview skipped). I downloaded macOS run 34912735876's XML and independently confirmed 22 targeted tests, zero failures/errors/skips: chip cancellation 2, session 5, reorder 15. git diff --check passes. These results predate retargeting and are not a fresh dev-integration run. CI/infrastructure is not an author defect. No source repairs or shared-branch changes were made in this review. Host placement is appropriate: these are host-owned tab gestures, with no plugin API or persisted-data changes.

@kshivang
kshivang marked this pull request as draft September 15, 2026 01:38
@kshivang
kshivang force-pushed the dev branch 5 times, most recently from 75ebc71 to 628f152 Compare September 18, 2026 02:25
@kshivang
kshivang force-pushed the dev branch 5 times, most recently from 56b3e1b to 4eb36a4 Compare September 24, 2026 22:21
@kshivang
kshivang marked this pull request as ready for review September 24, 2026 22:48
@github-actions

Copy link
Copy Markdown
Contributor

Claude diff review of a027cc1 (no tests executed).

Summary

The change replaces per-handler drag bookkeeping with a TabDragSession owned by the pointer-input coroutine, stabilises the pointerInput keys on both tab surfaces, removes the tab-id-based DisposableEffect in BossTabButton, and adds an optional sourceIndex refresh to endDrag. The core ownership model (identity comparison against component.draggingTab, cleanup in finally) is sound and is the right fix for the restart-without-callback class of bug. My concerns are concentrated in two places: the new work endDrag does at release time, and the fact that onEnd(null) is now delivered on cancellation/teardown paths where it previously was not.

I reviewed only the diff; I have not run anything and cannot see the unchanged call sites of onDragEnd, updateDropTarget, isDragging, or BossTabButton's windowPosition declaration.


Confirmed defects / issues

1. endDrag(sourceIndex) replaces draggingTab with a copy, which silently revokes the session's ownership

TabDraggableComponent.kt, new block in endDrag:

if (sourceIndex != null && draggingTab != null) {
    draggingTab = draggingTab?.copy(sourceIndex = sourceIndex)
    updateDropTarget()
}

TabDragSession.end() (TabDragSession.kt) checks isOwner before the call, then owned = null after it. Between those two points component.draggingTab becomes a different instance than owned. If anything after the copy throws — most plausibly updateDropTarget(), but also anything in the result computation before the documented "state is cleared before the result is worked out" point — then:

  • session.end() never clears owned,
  • the finally in detectTabDragGestures calls session.cancel(), which evaluates isOwner as false because identity no longer matches,
  • component.draggingTab is left non-null with no owner able to clear it.

That is precisely the stuck-ghost state the PR exists to eliminate, reintroduced on the exception path. Low probability, but it is a real hole in the ownership invariant the PR advertises. Safest fix is to have endDrag mutate the copy after capturing val dragging = draggingTab, or to have TabDragSession.end() clear owned before delegating (owned = null first, then component.endDrag(...)).

2. The drop target is now always recomputed at release, and that behaviour is keyed off an unrelated parameter

Both production call sites always pass a non-null sourceIndex (BossTabButton.kt: sourceIndex = { currentTabIndex }; TabFaviconChip.kt: sourceIndex = tabIndex). So in production updateDropTarget() now runs on every release, where previously the drop used whatever target the last updateDrag computed. Consequences that the diff does not address:

  • If a panel's bounds were unregistered mid-drag (pane closed, layout collapsed, a pane that spring-loaded and then re-laid-out), the previously-resolved target may now resolve to null and the drop is silently discarded.
  • Conversely, if bounds shifted under a stationary pointer, the drop can land somewhere the user never hovered.

Mixing "refresh the source index" and "re-resolve the target" behind one nullable parameter also means the default-argument path (endDrag(), used by TabDragSessionTest.ordinary cross pane drop) exercises different end-of-drag semantics than production. The test suite therefore never covers the production combination of "target resolved during drag" vs "target re-resolved at release" disagreeing. I'd split this into endDrag(sourceIndex: Int?) doing only the index refresh, or make the recompute unconditional and cover it.

3. Contract change: onEnd(null) now fires on cancel and on teardown

TabDragGesture.kt:

onDragCancel = { if (session.cancel()) onEnd(null) },
...
} finally {
    if (session.cancel()) onEnd(null)
}

Previously onDragCancel called only component.cancelDrag() and the finally notified nobody. The diff confirms the change by flipping TabDragGestureTest from assertEquals(0, drops) to assertEquals(1, drops). Two things follow that the diff does not show being handled:

  • Every existing onDragEnd consumer needs auditing. Any handler that treats invocation as "a drag concluded" (analytics, animation reset, focus restore, re-enabling input) now also fires for pure cancels and for pointerInput key restarts. None of those call sites are in the diff, so I cannot confirm they are null-safe in behaviour, only in type.
  • The finally path runs during node detach / composition exit. currentOnDragEnd(...) is invoked while the modifier node is being disposed, i.e. from a cancelled coroutine. If any consumer's handler writes to composition-scoped state or navigates, it now does so at disposal time. This is a new timing, not a new type.

The test file changed the assertion but I cannot see the test's name from the diff — if it is still named for "cancel does not report a drop" it now contradicts itself.

4. Double cleanup is dead-but-asymmetric

withDragSession ends with finally { session.cancel() }, and detectTabDragGestures already ends with finally { if (session.cancel()) onEnd(null) } inside it. The outer cancel can only ever observe owned == null, so it is dead for this caller — but it is the only cleanup for any future withDragSession user, and it cancels without notifying. That asymmetry (one path notifies, the other doesn't) is exactly the kind of thing the next reader will get wrong. Worth either dropping the outer finally or documenting that callers own notification.


Uncertain observations (need checking against code not in the diff)

5. BossTabButton's windowPosition may now be stale

BossTabButton.kt: startPosition = windowPosition + offset. TabFaviconChip deliberately threads this through a lambda (windowPosition = { windowPosition }) and the diff's own KDoc explains why: the value is only correct when read at drag start. BossTabButton reads it directly from the gesture lambda. That is fine if windowPosition is a delegated var ... by remember { mutableStateOf(...) } (the delegate is captured, the read is live). If it is a plain captured value, the widened pointerInput keys (tabInfo.id, panelId, tabDragComponent instead of tabInfo, panelId, tabIndex) mean the lambda is now recreated far less often, so a previously self-correcting staleness becomes permanent for the tab's lifetime. The declaration is not in the diff — please confirm.

6. No lower bound on sourceIndex at release

startDragging is gated at start by tabIndex >= 0 in BossTabButton.isDragEnabled. At release, session.end(sourceIndex()) passes whatever the current index is. For BossTabButton a negative index would flip isDragEnabled and detach the modifier first, so the gesture cancels — probably safe. For TabFaviconChip the enabling condition at the if (...) Modifier else Modifier.tabChipDrag(...) site is not in the diff, so I cannot confirm tabIndex is gated there. If it is not, a -1 can reach draggingTab.copy(sourceIndex = -1) and produce a Reorder with a negative from-index.

7. TabDragSession.start(onStart:) adopts whatever drag exists, not the one onStart created

if (component.isDragging) return false
try { onStart() } finally { owned = component.draggingTab }

The isDragging guard makes this benign today (nothing to adopt). But note BossTabButton's onStart can bail out via val info = currentTabInfo ?: return@detectTabDragGestures without starting anything; the finally still assigns. Correctness here rests entirely on isDragging being exactly draggingTab != null, which is not in the diff. If they can ever diverge, a handler that started nothing claims ownership of someone else's drag.


Test gaps

  • No assertion on onEnd call count in the removal/teardown paths. TabChipDragCancellationTest only asserts component.isDragging. Given issue ⬆️ actions:(deps): Bump actions/setup-go from 6 to 7 #3 (cancel now notifies) and the two nested finallys, a test pinning "exactly one onEnd(null) when the source is removed mid-drag" would be the highest-value addition. Only TabDragSurvivesTabChangeTest.changing a button identity cancels only its old gesture asserts a count (assertEquals(1, cancellations)), and only for the id-change restart.
  • No coverage of panelId changing — it is one of the three remaining keys and is untested on both surfaces.
  • No coverage of the end-time updateDropTarget() disagreeing with the in-drag target (see ⬆️ actions:(deps): Bump actions/setup-java from 4 to 5 #2): e.g. register bounds, drag over them, unregister them, release. That is the behavioural risk this parameter introduces and it is currently unexercised.
  • TabDragSessionTest exercises the 4-arg start(tab, panelId, index, position) overload, which no production code calls. The production overload is start(onStart: () -> Unit); its distinctive finally { owned = ... } semantics (including the throwing-onStart case) are untested.
  • TabDragSurvivesTabChangeTest.reindexed source is the tab reordered on release calls component.endDrag(sourceIndex = 1) directly, bypassing the session; no test asserts that a gesture that survived a reindex produces a correct Reorder through session.end other than the one composition test, which registers bounds manually after the drag started.
  • Possible dispatch-ordering flake: teardown cleanup depends on a cancelled coroutine's finally running before the assertion. The tests rely on waitForIdle()/runOnIdle to sequence this. Compose cancels the old pointerInput job and launches the replacement without awaiting the old one, so ordering between "new handler installed" and "old handler's finally" is not guaranteed. The identity check makes the logic order-independent (good), but assertions like assertFalse(component.isDragging, "source interruption must dispose the ghost") immediately after one waitForIdle() could be timing-sensitive on CI.

Non-issues / notes

  • The removal of the DisposableEffect in BossTabButton is correct and the AGENTS.md rule added alongside it accurately states why (a tab id can be on multiple surfaces). The two "idle copy does not cancel another surface's gesture" tests pin it on both surfaces.
  • return@detectTabDragGestures inside the named onStart argument resolves to the lambda's own implicit label, so it returns from onStart rather than the enclosing function — correct, though it reads as if it aborts the whole gesture setup. A named run { }/explicit guard would be clearer.
  • The KDoc in TabFaviconChip ("Seven parameters plus the receiver") matches the new signature.
  • No security surface here — all state is process-local UI state; no untrusted input, IO, or serialization is touched.
  • docs/investigations/tab-drag-ghost-690.md explicitly records that "manual verification of cross-pane drops over the hardware browser surface has not been reported." Given issue ⬆️ actions:(deps): Bump actions/setup-java from 4 to 5 #2, that is exactly the scenario I would want manually exercised before merge. (I treat the doc's claims about which Gradle tasks passed as unverified narrative, not evidence.)

@github-actions

Copy link
Copy Markdown
Contributor

Claude diff review of 088da0c (no tests executed).

Review — PR #692 @ 088da0c

Scope note: this is a diff-only read. I have not seen TabDraggableComponent, BossTabButton, TabFaviconChip, DraggingTabInfo, or any existing tests, so statements about them are inferences, flagged as such.

The core idea — ownership keyed on the exact DraggingTabInfo instance plus a finally-based teardown — is sound and is a real improvement over the hand-rolled ownedDrag bookkeeping it replaces. The issues below are mostly narrow.


1. Confirmed defects

1.1 end() leaks stale ownership state when the session is not the owner — TabDragSession.kt:36-41

fun end(sourceIndex: Int? = null): TabDropResult? {
    if (!isOwner) return null      // <- `owned` is left pointing at a dead DraggingTabInfo
    ...
    owned = null

This is a behavioural divergence from the code being deleted. The old handler cleared ownership unconditionally at the end of both callbacks (TabDragGesture.kt removed lines: onDragEnd ended with a bare ownedDrag = null outside the if, same for onDragCancel). The new onDragEnd (TabDragGesture.kt, onDragEnd = { if (session.isOwner) onEnd(session.end(sourceIndex())) }) never clears it on the non-owner path, so a session can survive a gesture holding a non-null owned that no longer matches component.draggingTab.

Impact is low only because isOwner uses reference identity and a fresh DraggingTabInfo is presumably allocated per startDragging. But the invariant "owned != null implies this session started that drag in the current gesture" is no longer true, and any future code path that re-publishes the same instance (restore/undo, pooling, a remembered instance) turns this into a spurious cancel of somebody else's drag. Your own test exercises the leak without asserting on it: TabDragSessionTest "old coroutine cleanup leaves replacement drag intact" calls session.end() as a non-owner, after which owned is still the stale instance until withDragSession's finally clears it.

Fix: if (!isOwner) { owned = null; return null }, or mirror cancel()'s shape (val ended = isOwner; ...; owned = null).

1.2 The behaviour the docs assert is not present in the diff — AGENTS.md:1378-1382, docs/investigations/tab-drag-ghost-690.md

AGENTS.md now states as a standing rule that "Both BossTabButton and TabFaviconChip run inside withDragSession", and the investigation doc says "The old tab-id-only disposal hook is removed because another surface for that tab could otherwise cancel a drag it did not start." No change to either composable, and no removal of any disposal hook, appears in this diff. Either (a) they reach withDragSession transitively via detectTabDragGestures and the removal already landed in the stacked #627, in which case the doc overstates what this PR does, or (b) the diff is incomplete. Since withDragSession is introduced in this diff, any direct caller must be in this diff — and the only direct callers here are detectTabDragGestures and tests. Please reconcile the prose with what actually ships, because AGENTS.md is load-bearing guidance for future changes.

1.3 TabDragSession.start(tab, panelId, index, position) has no production caller in this diff — TabDragSession.kt:15-20

The four-argument overload is referenced only by TabDragSessionTest. If the composables call detectTabDragGestures (which uses the start(onStart) overload), this is dead API kept alive by tests. Either wire it up or drop it; a convenience overload whose only consumer is its own test is a maintenance trap, and it is also the overload most exposed to issue 2.1 below.


2. Uncertain observations (need the out-of-diff definitions to confirm)

2.1 Ownership is inferred, not captured — depends on isDragging ≡ draggingTab != null — TabDragSession.kt:22-30

if (component.isDragging) return false
try { onStart() } finally { owned = component.draggingTab }

If isDragging is anything other than exactly draggingTab != null (e.g. it also requires a slop threshold, a non-zero delta, or a dropTarget), then a session whose onStart starts nothing can still latch onto a DraggingTabInfo owned by another handler — and its finally will then cancel that foreign drag. That is precisely the failure class this PR exists to eliminate. The deleted code had the same shape, so this is not a regression, but the new class generalises the pattern and hands it out as a reusable API.

Deterministic alternative that does not depend on isDragging at all:

val before = component.draggingTab
try { onStart() } finally {
    owned = component.draggingTab?.takeIf { it !== before }
}

2.2 Double teardown path — TabDragGesture.kt inner finally + TabDragSession.kt:56-58

detectTabDragGestures cancels in its own finally, then withDragSession cancels again. Idempotent as written (cancel() returns false the second time, so onEnd fires once), which matches the comment "notify once". I could not verify that component.cancelDrag() is itself idempotent/safe when no drag is active — if it unconditionally writes state or emits an event, the second call is a silent no-op only by luck. Worth a glance.

2.3 onEnd(null) is invoked from a finally during coroutine cancellation — TabDragGesture.kt, inner finally

Pre-existing, not introduced here, but the consolidation makes it the single cleanup path for both surfaces: if any onEnd implementation launches into a scope that is already cancelled, or writes state during disposal, the throw escapes the pointer-input coroutine. The added tests only use onEnd implementations supplied by the real composables (behaviour unasserted) or none at all.

2.4 owned has no memory-visibility annotation — TabDragSession.kt:10

Fine if every mutation and read happens on the single pointer-input/main dispatcher, which I'd expect but cannot confirm from the diff. Worth confirming that withDragSession's finally cannot run on a different thread than block.


3. Test gaps

3.1 The onEnd-exactly-once contract is untested. The stated purpose of the session.cancel() return value in TabDragGesture.kt is "notify once", yet no test drives detectTabDragGestures with an instrumented onEnd counter through (a) onDragCancel → finally, (b) onDragEnd → finally, (c) onStart throwing. TabDragSessionTest tests TabDragSession/withDragSession directly and never touches the adapter; TabChipDragCancellationTest goes through the composables and only observes isDragging. The single most regression-prone line in the diff has no direct coverage.

3.2 The reindex scenario asserts nothing about the outcome. TabChipDragCancellationTest.interruptedDrag sets index.value = 1 and asserts only component.isDragging before and !isDragging after release. The doc claims "release reads the current source index" (docs/investigations/tab-drag-ghost-690.md), i.e. that sourceIndex() resolves to 1, not 0 — that is the actual user-visible behaviour of the fix and it is never asserted. A wrong source index would produce a silently wrong reorder and every assertion in this test would still pass.

3.3 Two assertions are vacuous in three of four parameterisations. In the remove and replaceComponent branches the drag is already cleared before rule.onNodeWithTag("source").performMouseInput { release() }, so assertFalse(component.isDragging, "release must clear the ghost") tests nothing; likewise the final assertFalse(currentComponent.value.isDragging). Only the reindex branch gives that line meaning.

3.4 No composable-level test of the motivating multi-surface case. AGENTS.md:1380 justifies identity ownership with "a tab can appear on multiple surfaces". TabDragSessionTest covers it at unit level ("handler that never started a drag cannot cancel another handler"), but no test composes BossTabButton and TabFaviconChip for the same tab id simultaneously and verifies that disposing one does not kill the other's drag — which is the exact regression the removed disposal hook caused.

3.5 Possible brittleness in the mouse-injection sequence — TabChipDragCancellationTest.startMouseDrag + the later release(). The test presses in one performMouseInput block and releases in a different block after the subtree under the tag has been removed/recomposed/re-keyed. This relies on injection state being retained across invocations and on release() finding a pressed button; in the replaceComponent branch a brand-new pointerInput coroutine also starts with the pointer already down, which is not a state detectDragGestures normally observes. I can't confirm flakiness without running it, but it is the kind of sequence that breaks on Compose test-infra updates. Consider a single performMouseInput block with rule.mainClock control, or at minimum a comment explaining the retained-pointer assumption.

3.6 Not covered: session.update / session.end after the owning drag was ended by the owner itself (double-end), and start() returning true/false for the four-arg overload when another drag is live (only the closure overload's false path is exercised).


4. Security

Nothing security-relevant in this diff: no I/O, no deserialisation, no privilege or network surface. The only data crossing a boundary is TabInfo.id/title already in the process. The added doc contains an external issue link only.


Suggested minimum before merge

  1. Clear owned on the non-owner path of end() (1.1).
  2. Reconcile AGENTS.md / the investigation doc with what the diff actually changes, or include the composable changes (1.2).
  3. Add an onEnd-invocation-count test against detectTabDragGestures (3.1) and assert the resolved source index in the reindex case (3.2).
  4. Decide whether the four-arg start overload ships or goes (1.3), and consider the before/after identity capture in start (2.1).

@kshivang
kshivang merged commit 57553e3 into dev Sep 25, 2026
9 checks passed
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.

Tab drag ghost remains stuck to pointer after moving tabs between panes

1 participant