Skip to content

feat(ui): Cross-instance tab attach via TabDragBroker (Phase 4)#12

Merged
joshuapare merged 3 commits into
mainfrom
feat/tabs-dnd-3
Mar 8, 2026
Merged

feat(ui): Cross-instance tab attach via TabDragBroker (Phase 4)#12
joshuapare merged 3 commits into
mainfrom
feat/tabs-dnd-3

Conversation

@joshuapare
Copy link
Copy Markdown
Contributor

@joshuapare joshuapare commented Mar 8, 2026

Summary

  • Adds TabDragBrokerProvider context that coordinates drag sessions across multiple EditorTabs instances, enabling detach-from-one / drop-onto-another workflows
  • Extends useTabDetach with horizontal threshold detection (left/right edge escape) and an onDetachArmed callback so the broker takes over mid-drag while the pointer is still down
  • Adds useTabAttach hook for drop zone registration, hover detection, and insert index computation from pointer position
  • New detachToBroker opt-in prop on EditorTabs (default false) — standalone Phase 3 detach is unaffected
  • Visual polish: drop target highlight, insertion indicator, suppressed sortable transition during detach

Test plan

  • pnpm typecheck passes
  • All 282 unit tests pass (pnpm test)
  • CrossWindowAttach story: detach tab → FakeWindow spawns; drag from FakeWindow over main strip → highlight + indicator; release → tab attaches at position; release elsewhere → tab returns to window
  • SplitPaneAttach story: drag tab horizontally or vertically between left/right panes; dropped tab becomes active; source pane selects neighbor after detach
  • Existing reorder, keyboard nav, close, context menu stories all still work
  • Reduced motion respected (no transition animations)

Summary by CodeRabbit

  • New Features

    • Cross-window and split-pane tab attach/detach via drag-and-drop
    • Visual drop-target highlight and vertical insertion indicator during drags
  • Bug Fixes

    • Prevents tabs from snapping back during detach operations for smoother animations
  • Tests

    • Added comprehensive tests for drag broker, attach flows, insert-index calculations, and detach thresholds

Introduce a TabDragBroker context that coordinates drag sessions across
multiple EditorTabs instances on the same page. Tabs can be detached
from one strip and dropped onto another with visual insertion indicators.

- Add TabDragBrokerProvider with pointer tracking, drop zone registry,
  and floating ghost rendering via portal
- Add useTabAttach hook for drop zone registration and insert index
  computation
- Extend useTabDetach with horizontal threshold detection and
  onDetachArmed callback for mid-drag broker handoff
- Add detachToBroker opt-in prop to EditorTabs (default false)
- Add drop target highlight and insertion indicator CSS
- Suppress sortable transition during detach to prevent visual glitch
- Add CrossWindowAttach and SplitPaneAttach stories
- Add unit tests for broker, attach hook, and horizontal detach
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 8, 2026

📝 Walkthrough

Walkthrough

A TabDragBroker-based cross-window/pane attach system is added. EditorTabs integrates broker lifecycle, attach/drop detection, and insert indicators; useTabAttach and TabDragBroker manage drop zones and ghost rendering. CSS and EditorTabItem transitions are adjusted to suppress snap-back during detach-armed drags.

Changes

Cohort / File(s) Summary
Tab Drag Broker System
packages/base-ui/src/components/editor-tabs/context/TabDragBroker.tsx, packages/base-ui/src/components/editor-tabs/context/TabDragBroker.test.tsx
New provider + hook exposing session lifecycle, pointer tracking, drop-zone registration, hit-testing, and ghost-tab portal rendering; comprehensive tests for session, hit-testing, and commit behavior.
Drop Zone Attachment Hook
packages/base-ui/src/components/editor-tabs/hooks/useTabAttach.ts, packages/base-ui/src/components/editor-tabs/hooks/useTabAttach.test.ts
New hook registering a viewport as a drop zone, computing insertIndex from broker.pointerX and tab layout, exposing isDropTarget/insertIndex and committing attaches.
Detach Detection & Callbacks
packages/base-ui/src/components/editor-tabs/hooks/useTabDetach.ts, packages/base-ui/src/components/editor-tabs/hooks/useTabDetach.test.ts
Adds horizontal escape detection and optional onDetachArmed/onDetachReverted callbacks; tests updated to assert horizontal-threshold behavior.
Editor Tabs Integration
packages/base-ui/src/components/editor-tabs/EditorTabs.tsx, packages/base-ui/src/components/editor-tabs/EditorTabsViewport.tsx, packages/base-ui/src/components/editor-tabs/context/EditorTabsContext.tsx
Adds instanceId/onAttachTab/detachToBroker props, broker session lifecycle, shared CSS-var snapshotting for ghost styling, exposes isAttachDropTarget & attachInsertIndex via context, and renders AttachDropIndicator in viewport.
Types & Public Exports
packages/base-ui/src/components/editor-tabs/types.ts, packages/base-ui/src/components/editor-tabs/index.ts
Introduces AttachCommit type and re-exports TabDragBrokerProvider and useTabDragBroker from the package index.
Animation & Visuals
packages/base-ui/src/components/editor-tabs/EditorTabItem.tsx, packages/base-ui/src/components/editor-tabs/EditorTabs.module.css
Suppresses transition when dragMode is detach-armed to avoid snap-back; adds .AttachDropIndicator styles and viewport drop-target highlight with reduced-motion handling.
Stories & Tests
packages/base-ui/src/components/editor-tabs/EditorTabs.stories.tsx, packages/base-ui/src/components/editor-tabs/EditorTabs.test.tsx
Adds cross-window and split-pane stories demonstrating detach/attach flows and tests validating onAttachTab acceptance and conditional drop-target rendering.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant EditorTabs as EditorTabs<br/>(Main)
    participant Broker as TabDragBroker<br/>(Provider)
    participant Ghost as DetachGhost<br/>(Portal)
    participant EditorTabs2 as EditorTabs<br/>(Target Pane)

    User->>EditorTabs: pointerdown on tab
    EditorTabs->>EditorTabs: enter reorder mode
    User->>EditorTabs: move pointer beyond horizontal threshold
    EditorTabs->>EditorTabs: transition to detach-armed
    EditorTabs->>Broker: beginSession(tab, clientX, clientY)
    Broker->>Ghost: render ghost at pointer pos
    User->>Broker: pointerMove events
    Broker->>Broker: hit-test drop zones, compute insertIndex
    Broker->>EditorTabs2: set hover / isAttachDropTarget
    EditorTabs2->>EditorTabs2: render AttachDropIndicator at insertIndex
    User->>Broker: pointerup over EditorTabs2
    Broker->>EditorTabs2: onAttach(commit {tab, sourceInstanceId, insertIndex})
    EditorTabs2->>EditorTabs2: insert tab at index
    Broker->>Ghost: clear session and unmount ghost
Loading
sequenceDiagram
    participant Hook as useTabAttach
    participant Broker as TabDragBroker
    participant Viewport as EditorTabsViewport

    rect rgba(100,150,200,0.5)
        Hook->>Broker: registerDropZone({instanceId, getRect, getElement, onAttach})
        Broker->>Broker: add zone to registry
    end

    rect rgba(150,200,100,0.5)
        Broker->>Broker: pointerMove -> hitTest zones -> set hoverInstanceId, pointerX
        Broker->>Hook: cause re-render via context
    end

    rect rgba(200,100,100,0.5)
        Hook->>Hook: compute insertIndex from tab midpoints and broker.pointerX
        Viewport->>Viewport: render AttachDropIndicator at computed left
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

Poem

🐰 I hopped a ghostly tab tonight,

across panes lit with guiding light,
Brokers hummed and zones did glow,
I placed the tab where it should go —
a tiny hop, a perfect flight. ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly summarizes the main change: adding cross-instance tab attach coordination via TabDragBroker (Phase 4), which is the primary feature delivered by this changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/tabs-dnd-3

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/base-ui/src/components/editor-tabs/context/TabDragBroker.tsx`:
- Around line 83-96: The cleanup function currently only runs on
pointerup/cancel and leaves document-level listeners if the provider unmounts;
add a React unmount effect in TabDragBrokerProvider that calls cleanup() (or
cancelSession() if you need onCancel semantics) to remove pointermove/pointerup
listeners, cancelAnimationFrame, clear sessionRef, and reset state when the
provider unmounts; locate the cleanup function in TabDragBroker.tsx and add a
useEffect with a return handler that invokes cleanup() so listeners and stale
closures are torn down on unmount.
- Around line 22-25: The broker currently seeds its pointerX/pointerY from
screen coordinates causing the fixed-position ghost to be misaligned; change the
TabDragBroker API so beginSession(session: TabDragSession, clientX: number,
clientY: number) (and any related calls) use client coordinates end-to-end:
update the TabDragBrokerValue interface, the beginSession implementation in
TabDragBroker, and all callers (notably useTabDetach) to pass clientX/clientY
instead of screenX/screenY, and ensure subsequent pointer updates and
cancelSession still operate on client-space pointerX/pointerY so the ghost is
positioned correctly immediately on beginSession.
- Around line 98-125: computeInsertIndex is scanning the entire document for
'[data-tab-id]' which can include tabs from other viewports causing wrong
hit-testing and insert indexes; change it to query only within the matched
DropZoneRegistration/viewport returned by zone.getRect() (or a zone-provided
element) instead of document.querySelectorAll, and use that zone-scoped element
for both the intersection check and the zoneTabs sort/midpoint logic; update any
other similar logic (the other function at lines ~144-176) that uses
document-wide queries to instead use zone-scoped queries and/or a registered
viewport element from dropZonesRef.current so the computed index only considers
tabs inside the target drop zone (refer to computeInsertIndex,
dropZonesRef.current, zone.getRect, and the other drop-handling function
mentioned).

In `@packages/base-ui/src/components/editor-tabs/EditorTabs.stories.tsx`:
- Around line 786-791: The BrokerFakeWindow story registers a fake attach target
by passing onAttachTab but handleAttachTab is a no-op, causing tabs to be
removed via onDetachCommit and never inserted into the target; either implement
the attach logic in handleAttachTab to update the fake window's tab state
(matching the detach flow) or stop passing onAttachTab to BrokerFakeWindow so it
isn't advertised as a drop target; update the same pattern for the second
occurrence (lines 860-863) and remove the unused handleAttachTab to clear the
ESLint error if you choose to stop advertising attach support.

In `@packages/base-ui/src/components/editor-tabs/hooks/useTabAttach.test.ts`:
- Around line 5-33: Add a broker-backed test around useTabAttach by wrapping the
hook with TabDragBrokerProvider; create a viewportRef containing two sibling tab
elements (with known boundingClientRect widths/positions) and render the hook
via renderHook({ wrapper: ({ children }) =>
<TabDragBrokerProvider>{children}</TabDragBrokerProvider> }). Use the
broker-backed flow to simulate hover: dispatch pointer/mouse events (inside act)
with clientX placed just left of the midpoint and assert
result.current.insertIndex === leftIndex, then dispatch with clientX just right
of the midpoint and assert insertIndex === rightIndex; keep instanceId and
viewportRef parameters the same as existing tests and ensure events target the
viewport/tab elements so hover detection and midpoint-based insertIndex logic in
useTabAttach are exercised.

In `@packages/base-ui/src/components/editor-tabs/hooks/useTabDetach.ts`:
- Around line 11-12: The onDetachArmed callback currently forwards
screenX/screenY to the broker which breaks insertIndex calculations in
useTabAttach.ts that compare broker.pointerX to element.getBoundingClientRect()
midpoints; update the handoff for onDetachArmed in useTabDetach.ts to pass
clientX/clientY (not screenX/screenY) to the broker so broker.pointerX/ptr align
with getBoundingClientRect() coordinates, while keeping onDetachCommit unchanged
to continue using screenX/screenY; locate usages of onDetachArmed/onDetachCommit
and the broker assignment so you adjust only the onDetachArmed path and leave
onDetachCommit as-is.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: dcbb1677-75f0-44a9-9580-cd1c57eb8618

📥 Commits

Reviewing files that changed from the base of the PR and between ad6bc04 and 887072c.

📒 Files selected for processing (15)
  • packages/base-ui/src/components/editor-tabs/EditorTabItem.tsx
  • packages/base-ui/src/components/editor-tabs/EditorTabs.module.css
  • packages/base-ui/src/components/editor-tabs/EditorTabs.stories.tsx
  • packages/base-ui/src/components/editor-tabs/EditorTabs.test.tsx
  • packages/base-ui/src/components/editor-tabs/EditorTabs.tsx
  • packages/base-ui/src/components/editor-tabs/EditorTabsViewport.tsx
  • packages/base-ui/src/components/editor-tabs/context/EditorTabsContext.tsx
  • packages/base-ui/src/components/editor-tabs/context/TabDragBroker.test.tsx
  • packages/base-ui/src/components/editor-tabs/context/TabDragBroker.tsx
  • packages/base-ui/src/components/editor-tabs/hooks/useTabAttach.test.ts
  • packages/base-ui/src/components/editor-tabs/hooks/useTabAttach.ts
  • packages/base-ui/src/components/editor-tabs/hooks/useTabDetach.test.ts
  • packages/base-ui/src/components/editor-tabs/hooks/useTabDetach.ts
  • packages/base-ui/src/components/editor-tabs/index.ts
  • packages/base-ui/src/components/editor-tabs/types.ts

Comment thread packages/base-ui/src/components/editor-tabs/context/TabDragBroker.tsx Outdated
Comment thread packages/base-ui/src/components/editor-tabs/EditorTabs.stories.tsx Outdated
Comment thread packages/base-ui/src/components/editor-tabs/hooks/useTabDetach.ts Outdated
- Add useEffect cleanup in TabDragBrokerProvider to remove document
  listeners on unmount
- Change broker API from screen to client coordinates so the ghost
  positions correctly with position:fixed
- Scope computeInsertIndex to zone element via getElement() instead of
  querying all [data-tab-id] in the document
- Remove no-op onAttachTab from BrokerFakeWindow so it doesn't
  silently swallow drops
- Add broker-backed tests for useTabAttach verifying drop zone
  registration and insert index computation at various pointer positions
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/base-ui/src/components/editor-tabs/hooks/useTabDetach.ts (1)

1-1: ⚠️ Potential issue | 🟠 Major

Add unmount cleanup for the document-level drag listener.

cleanup() only runs on drag end/cancel. If an EditorTabs instance unmounts while a drag is active, the document pointermove handler and pending RAF persist with stale closures, preventing proper teardown. Mirror the unmount cleanup pattern from TabDragBrokerProvider (line 119 in TabDragBroker.tsx).

Patch sketch
-import { useCallback, useRef, useState } from 'react';
+import { useCallback, useEffect, useRef, useState } from 'react';
@@
   const cleanup = useCallback(() => {
     if (listenerRef.current) {
       document.removeEventListener('pointermove', listenerRef.current);
       listenerRef.current = null;
@@
     stripRectRef.current = null;
     activeIdRef.current = null;
   }, []);
+
+  useEffect(() => cleanup, [cleanup]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/base-ui/src/components/editor-tabs/hooks/useTabDetach.ts` at line 1,
The document-level pointermove handler and pending requestAnimationFrame in
useTabDetach (cleanup only runs on drag end/cancel) need teardown on unmount to
avoid stale closures; update the hook (useTabDetach) to register a useEffect
cleanup that calls the existing cleanup function when the component unmounts,
and ensure that cleanup removes the document pointermove listener and cancels
any pending RAF (mirror the unmount cleanup pattern used in
TabDragBrokerProvider). Locate the cleanup logic inside useTabDetach and make
the effect call it on unmount so event listeners and RAF are always cleared even
if a drag is active during unmount.
♻️ Duplicate comments (2)
packages/base-ui/src/components/editor-tabs/hooks/useTabAttach.test.ts (1)

51-199: 🧹 Nitpick | 🔵 Trivial

These tests still miss the live hover path.

Both broker-backed cases only validate the commit payload produced by pointerup; they never drive pointermove or assert result.current.attach.isDropTarget / insertIndex. A regression in the visual attach-indicator path of useTabAttach() would still pass this suite.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/base-ui/src/components/editor-tabs/hooks/useTabAttach.test.ts`
around lines 51 - 199, Tests only assert commit payload on pointerup and miss
exercising the live hover path; update the two broker-backed tests that use
useTabAttach and useTabDragBroker to also simulate pointermove events between
beginSession and pointerup, find and call the registered 'pointermove' handler
(like you did for 'pointerup'), and assert result.current.attach.isDropTarget
becomes true and that result.current.attach.insertIndex updates to the expected
index during hover before calling the pointerup commit; ensure you reference the
broker.beginSession, attach.isDropTarget, and attach.insertIndex symbols when
locating the code to change.
packages/base-ui/src/components/editor-tabs/context/TabDragBroker.tsx (1)

137-145: ⚠️ Potential issue | 🟠 Major

Resolve the topmost hit zone, not the first registered one.

Both loops stop at the first matching entry in dropZonesRef.current. In packages/base-ui/src/components/editor-tabs/EditorTabs.stories.tsx Lines 952-966, the fake windows are draggable fixed overlays, so when a window overlaps the main strip the broker can still commit to whichever zone registered first. Use getElement() together with document.elementsFromPoint() (or equivalent) so the broker picks the visually topmost intersecting zone before computing insertIndex.

Also applies to: 154-162

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/base-ui/src/components/editor-tabs/context/TabDragBroker.tsx` around
lines 137 - 145, The current loop over dropZonesRef.current picks the first
registered matching zone; change it to resolve the visually topmost matching
zone by: collect all zones where hitTestZone(zone.getRect(), e.clientX,
e.clientY) is true (and skip sessionRef.current?.sourceInstanceId), then use
each zone's DOM element (zone.getElement()) and call
document.elementsFromPoint(e.clientX, e.clientY) to pick the first element in
that returned list that matches one of the zone elements; set foundId to that
zone's id and proceed to compute insertIndex. Apply this change to both places
in TabDragBroker.tsx where you iterate dropZonesRef.current (the initial
hit-test loop and the subsequent similar loop) so the broker always commits to
the visually topmost intersecting zone.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/base-ui/src/components/editor-tabs/hooks/useTabAttach.ts`:
- Around line 21-37: The effect in useTabAttach currently depends on the whole
broker context so pointer-state changes retrigger unregister/register; to fix,
pull the stable callbacks off the broker (e.g. const { registerDropZone,
unregisterDropZone } = broker || {}) and use those in the useEffect dependency
array instead of broker, then call registerDropZone({ instanceId, getRect: ...,
getElement: ..., onAttach: onAttachTab }) and return () =>
unregisterDropZone(instanceId); also guard the effect to return early if
registerDropZone or unregisterDropZone or onAttachTab are falsy and keep
instanceId and viewportRef in deps.

In `@packages/base-ui/src/components/editor-tabs/hooks/useTabDetach.ts`:
- Around line 89-99: The hysteresis re-entry path currently skips when
onDetachArmed is present, making broker drags irreversible; change the logic in
useTabDetach so that when currentMode === 'detach-armed' you still compute
withinX/withinY and allow updateMode('reorder') to run regardless of
onDetachArmed; when performing that transition also call the broker recovery to
clear the session (i.e., invoke broker.clearSession() or the appropriate clear
method used by detachToBroker/EditorTabs) so the EditorTabs onDetachCommit path
no longer fires and broker state is reset. Ensure onDetachArmed/onDetachCommit
semantics are preserved and only the hysteresis re-entry gains
broker.clearSession() on the reorder transition.

---

Outside diff comments:
In `@packages/base-ui/src/components/editor-tabs/hooks/useTabDetach.ts`:
- Line 1: The document-level pointermove handler and pending
requestAnimationFrame in useTabDetach (cleanup only runs on drag end/cancel)
need teardown on unmount to avoid stale closures; update the hook (useTabDetach)
to register a useEffect cleanup that calls the existing cleanup function when
the component unmounts, and ensure that cleanup removes the document pointermove
listener and cancels any pending RAF (mirror the unmount cleanup pattern used in
TabDragBrokerProvider). Locate the cleanup logic inside useTabDetach and make
the effect call it on unmount so event listeners and RAF are always cleared even
if a drag is active during unmount.

---

Duplicate comments:
In `@packages/base-ui/src/components/editor-tabs/context/TabDragBroker.tsx`:
- Around line 137-145: The current loop over dropZonesRef.current picks the
first registered matching zone; change it to resolve the visually topmost
matching zone by: collect all zones where hitTestZone(zone.getRect(), e.clientX,
e.clientY) is true (and skip sessionRef.current?.sourceInstanceId), then use
each zone's DOM element (zone.getElement()) and call
document.elementsFromPoint(e.clientX, e.clientY) to pick the first element in
that returned list that matches one of the zone elements; set foundId to that
zone's id and proceed to compute insertIndex. Apply this change to both places
in TabDragBroker.tsx where you iterate dropZonesRef.current (the initial
hit-test loop and the subsequent similar loop) so the broker always commits to
the visually topmost intersecting zone.

In `@packages/base-ui/src/components/editor-tabs/hooks/useTabAttach.test.ts`:
- Around line 51-199: Tests only assert commit payload on pointerup and miss
exercising the live hover path; update the two broker-backed tests that use
useTabAttach and useTabDragBroker to also simulate pointermove events between
beginSession and pointerup, find and call the registered 'pointermove' handler
(like you did for 'pointerup'), and assert result.current.attach.isDropTarget
becomes true and that result.current.attach.insertIndex updates to the expected
index during hover before calling the pointerup commit; ensure you reference the
broker.beginSession, attach.isDropTarget, and attach.insertIndex symbols when
locating the code to change.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: f3041717-35c0-4c44-802e-2f232562a68a

📥 Commits

Reviewing files that changed from the base of the PR and between 887072c and a7b7e39.

📒 Files selected for processing (7)
  • packages/base-ui/src/components/editor-tabs/EditorTabs.stories.tsx
  • packages/base-ui/src/components/editor-tabs/EditorTabs.tsx
  • packages/base-ui/src/components/editor-tabs/context/TabDragBroker.test.tsx
  • packages/base-ui/src/components/editor-tabs/context/TabDragBroker.tsx
  • packages/base-ui/src/components/editor-tabs/hooks/useTabAttach.test.ts
  • packages/base-ui/src/components/editor-tabs/hooks/useTabAttach.ts
  • packages/base-ui/src/components/editor-tabs/hooks/useTabDetach.ts

Comment thread packages/base-ui/src/components/editor-tabs/hooks/useTabAttach.ts Outdated
Comment on lines +89 to 99
if (currentMode === 'reorder' && (above || below || pastLeft || pastRight)) {
updateMode('detach-armed');
} else if (currentMode === 'detach-armed') {
// Hysteresis: must come back closer than half threshold to revert
const withinHysteresis =
onDetachArmed?.(activeIdRef.current!, e.clientX, e.clientY);
} else if (currentMode === 'detach-armed' && !onDetachArmed) {
// Hysteresis: must come back within half-threshold on BOTH axes to revert.
const withinY =
e.clientY >= rect.top - hysteresis && e.clientY <= rect.bottom + hysteresis;
if (withinHysteresis) {
const withinX =
e.clientX >= rect.left - hysteresis && e.clientX <= rect.right + hysteresis;
if (withinX && withinY) {
updateMode('reorder');
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Broker mode can’t return to reorder once it arms.

Line 92 disables the hysteresis path whenever onDetachArmed is provided. In detachToBroker mode that makes the handoff irreversible: dragging back over the source strip still ends in onDetachCommit, and the broker.clearSession() recovery path in packages/base-ui/src/components/editor-tabs/EditorTabs.tsx Lines 197-200 never becomes reachable. Keep the reorder re-entry path for broker drags and clear the broker session when that transition happens.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/base-ui/src/components/editor-tabs/hooks/useTabDetach.ts` around
lines 89 - 99, The hysteresis re-entry path currently skips when onDetachArmed
is present, making broker drags irreversible; change the logic in useTabDetach
so that when currentMode === 'detach-armed' you still compute withinX/withinY
and allow updateMode('reorder') to run regardless of onDetachArmed; when
performing that transition also call the broker recovery to clear the session
(i.e., invoke broker.clearSession() or the appropriate clear method used by
detachToBroker/EditorTabs) so the EditorTabs onDetachCommit path no longer fires
and broker state is reset. Ensure onDetachArmed/onDetachCommit semantics are
preserved and only the hysteresis re-entry gains broker.clearSession() on the
reorder transition.

- Stabilize useTabAttach effect deps by destructuring registerDropZone
  and unregisterDropZone from broker instead of depending on the whole
  context object which is recreated every render
- Allow hysteresis revert even when onDetachArmed is present so broker
  drags are reversible; add onDetachReverted callback wired to
  broker.clearSession() in EditorTabs
- Add useEffect unmount cleanup in useTabDetach to remove document
  pointermove listener and cancel pending rAF on unmount
- Add live hover assertions (isDropTarget, insertIndex) to
  broker-backed useTabAttach tests via rAF stub
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/base-ui/src/components/editor-tabs/EditorTabs.tsx (1)

197-210: ⚠️ Potential issue | 🟠 Major

Guard the legacy detach commit when the broker already consumed the drop.

With detachToBroker enabled, a target strip can already handle the pointerup via onAttachTab, but this branch still calls detach.handleDetachDragEnd() for every detach-armed end. That means the same drag can both attach through the broker and re-fire the source onDetachCommit. Please plumb the broker’s end-state into this branch and only run the legacy detach path when the broker session finishes without an attach.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/base-ui/src/components/editor-tabs/EditorTabs.tsx` around lines 197
- 210, When handling DragEnd in handleDragEnd, guard the legacy detach commit so
it only runs if the broker did not consume/complete an attach: inside the
detach.dragModeRef.current === 'detach-armed' branch, after calling
reorder.handleDragCancel(), check brokerSessionStarted.current and broker and
whether the broker reports a completed attach/consumption (add/use a small query
API such as broker.wasAttached() or broker.sessionConsumed flag); only call
detach.handleDetachDragEnd(event) when that broker query indicates no attach
occurred—otherwise skip detach.handleDetachDragEnd to avoid double-committing;
keep detach.handleDetachDragCancel() in the other branch as-is and ensure
brokerSessionStarted.current is reset afterward.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@packages/base-ui/src/components/editor-tabs/EditorTabs.tsx`:
- Around line 197-210: When handling DragEnd in handleDragEnd, guard the legacy
detach commit so it only runs if the broker did not consume/complete an attach:
inside the detach.dragModeRef.current === 'detach-armed' branch, after calling
reorder.handleDragCancel(), check brokerSessionStarted.current and broker and
whether the broker reports a completed attach/consumption (add/use a small query
API such as broker.wasAttached() or broker.sessionConsumed flag); only call
detach.handleDetachDragEnd(event) when that broker query indicates no attach
occurred—otherwise skip detach.handleDetachDragEnd to avoid double-committing;
keep detach.handleDetachDragCancel() in the other branch as-is and ensure
brokerSessionStarted.current is reset afterward.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8ca0214f-480c-4f0f-9539-6f2e52db607f

📥 Commits

Reviewing files that changed from the base of the PR and between a7b7e39 and f90a233.

📒 Files selected for processing (4)
  • packages/base-ui/src/components/editor-tabs/EditorTabs.tsx
  • packages/base-ui/src/components/editor-tabs/hooks/useTabAttach.test.ts
  • packages/base-ui/src/components/editor-tabs/hooks/useTabAttach.ts
  • packages/base-ui/src/components/editor-tabs/hooks/useTabDetach.ts

@joshuapare joshuapare merged commit 6043989 into main Mar 8, 2026
1 check 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.

1 participant