feat(ui): Tab detach on vertical drag (Phase 3)#10
Merged
Conversation
Drag a tab vertically beyond a threshold to detach it, firing onDetachCommit with screen coordinates and payload for the host app to spawn a new window. - useTabDetach hook with native pointermove tracking and hysteresis - createDetachAwareModifier replacing restrictToHorizontalAxis - DragOverlay with CSS variable snapshot for detach ghost - DetachGhostTab lightweight clone component - Detachable/DetachDisabled stories with fake window demo - 15 new tests for detach hook, modifier, and integration
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a detach-capable drag flow to EditorTabs: new useTabDetach hook and DragMode/DetachCommit types, a detach-aware modifier, DetachGhostTab UI with CSS and DragOverlay integration, EditorTabs and EditorTabItem updates to expose/use dragMode, storybook demos, and comprehensive tests for detach behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Editor as EditorTabs
participant Hook as useTabDetach
participant Overlay as DragOverlay
participant Parent as ParentComponent
User->>Editor: pointerdown + drag start
Editor->>Hook: handleDetachDragStart(event)
Hook->>Hook: set dragMode='reorder'\nattach pointermove listener
User->>Hook: pointermove (beyond threshold)
Hook->>Hook: compute delta\nset dragMode='detach-armed'
Hook->>Editor: update context.dragMode
Editor->>Overlay: render DetachGhostTab (DragOverlay)
User->>Editor: release pointer (drag end)
Editor->>Hook: handleDetachDragEnd(event)
alt dragMode == 'detach-armed'
Hook->>Parent: onDetachCommit({ id, payload, screenX, screenY })
Parent->>Parent: spawn detached window\nremove tab from main bar
end
Hook->>Hook: cleanup() -> remove listener, set dragMode='idle'
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/DetachGhostTab.tsx`:
- Around line 10-15: The ghost clone rendered by DetachGhostTab is decorative
and duplicates the live tab label for assistive tech; update the JSX returned by
the DetachGhostTab component to include aria-hidden="true" on the root div (the
element with className using styles.Tab and styles.DetachGhost) so screen
readers ignore this cloned content while dragging.
In `@packages/base-ui/src/components/editor-tabs/EditorTabs.stories.tsx`:
- Around line 611-623: The close button in the EditorTabs story currently lacks
an explicit type which can cause unintended form submissions; update the button
element (the one rendering LuX and calling onClose) to include type="button" so
it behaves as a non-submit control across contexts—locate the button with
onClick={onClose} inside EditorTabs.stories (the close button JSX) and add the
type attribute.
In `@packages/base-ui/src/components/editor-tabs/EditorTabs.test.tsx`:
- Around line 165-193: Add pointer drag/drop integration tests that exercise the
EditorTabs detach API instead of only mount smoke tests: simulate a pointerdown
on the tab element (use screen.getByRole('tab', { name: 'index.ts' })) then move
the pointer enough to cross the component's detach threshold and fire pointerup
to trigger the detach flow, and assert that the onDetachCommit mock receives an
object with { id, payload, screenX, screenY } matching the tab and pointer
coordinates; also add a test that renders <EditorTabs detachable={false} ... />
and performs the same pointer drag sequence and assert that onDetachCommit is
not called and no [data-detach-source] is added to the document.
In `@packages/base-ui/src/components/editor-tabs/EditorTabs.tsx`:
- Around line 105-106: Replace the useMemo-created mutable containers with
useRef to be idiomatic: change rootElRef (currently defined via useMemo(() => ({
current: null as HTMLDivElement | null }), [])) to useRef<HTMLDivElement |
null>(null) and change cssVarSnapshotRef (currently useMemo(() => ({ current:
null as React.CSSProperties | null }), [])) to useRef<React.CSSProperties |
null>(null); also add useRef to the React import. This preserves the same types
and behavior while communicating intent more clearly.
In
`@packages/base-ui/src/components/editor-tabs/modifiers/createDetachAwareModifier.test.ts`:
- Around line 45-61: Add a symmetric test that verifies the lower-bound clamp
for X in reorder mode by mirroring the existing upper-bound case: call
createDetachAwareModifier with modeRef.current = 'reorder', use makeModifierArgs
with a transform.x negative enough to push the node's left past the
ancestor.left, and assert the returned x is clamped so the node.left equals
ancestor.left (and y clamped to 0 as in the other test). Reference the existing
test pattern and helper functions createDetachAwareModifier and makeModifierArgs
to construct the new "clamps X to ancestor left edge in reorder mode" test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aec51ab6-3b6b-4d27-86a4-bc00a1b860d1
📒 Files selected for processing (13)
packages/base-ui/src/components/editor-tabs/DetachGhostTab.tsxpackages/base-ui/src/components/editor-tabs/EditorTabItem.tsxpackages/base-ui/src/components/editor-tabs/EditorTabs.module.csspackages/base-ui/src/components/editor-tabs/EditorTabs.stories.tsxpackages/base-ui/src/components/editor-tabs/EditorTabs.test.tsxpackages/base-ui/src/components/editor-tabs/EditorTabs.tsxpackages/base-ui/src/components/editor-tabs/context/EditorTabsContext.tsxpackages/base-ui/src/components/editor-tabs/hooks/useTabDetach.test.tspackages/base-ui/src/components/editor-tabs/hooks/useTabDetach.tspackages/base-ui/src/components/editor-tabs/index.tspackages/base-ui/src/components/editor-tabs/modifiers/createDetachAwareModifier.test.tspackages/base-ui/src/components/editor-tabs/modifiers/createDetachAwareModifier.tspackages/base-ui/src/components/editor-tabs/types.ts
Comment on lines
+165
to
+193
| it('renders normally with detachable={false}', () => { | ||
| renderWithTheme( | ||
| <EditorTabs tabs={baseTabs} activeId="file1" detachable={false} />, | ||
| ); | ||
|
|
||
| expect(screen.getByRole('tab', { name: 'index.ts' })).toBeInTheDocument(); | ||
| expect(screen.getByRole('tab', { name: 'App.tsx' })).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('does not apply data-detach-source during normal render', () => { | ||
| renderWithTheme(<EditorTabs tabs={baseTabs} activeId="file1" />); | ||
|
|
||
| const detachSource = document.querySelector('[data-detach-source]'); | ||
| expect(detachSource).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('accepts onDetachCommit prop without error', () => { | ||
| const onDetachCommit = vi.fn(); | ||
| renderWithTheme( | ||
| <EditorTabs | ||
| tabs={baseTabs} | ||
| activeId="file1" | ||
| onDetachCommit={onDetachCommit} | ||
| />, | ||
| ); | ||
|
|
||
| expect(screen.getByRole('tab', { name: 'index.ts' })).toBeInTheDocument(); | ||
| expect(onDetachCommit).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Exercise the detach API, not just mount-time smoke paths.
These assertions only prove the component renders. They will not catch regressions in the detachable={false} gate or the onDetachCommit wiring. Please add at least one pointer drag/drop test here that crosses the detach threshold and asserts the emitted { id, payload, screenX, screenY }, plus one that proves detachable={false} suppresses it.
🤖 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.test.tsx` around lines
165 - 193, Add pointer drag/drop integration tests that exercise the EditorTabs
detach API instead of only mount smoke tests: simulate a pointerdown on the tab
element (use screen.getByRole('tab', { name: 'index.ts' })) then move the
pointer enough to cross the component's detach threshold and fire pointerup to
trigger the detach flow, and assert that the onDetachCommit mock receives an
object with { id, payload, screenX, screenY } matching the tab and pointer
coordinates; also add a test that renders <EditorTabs detachable={false} ... />
and performs the same pointer drag sequence and assert that onDetachCommit is
not called and no [data-detach-source] is added to the document.
- Add aria-hidden to DetachGhostTab overlay clone - Replace useMemo ref containers with idiomatic useRef - Add type="button" to story close button - Add left-edge X clamp test for modifier - Add detach integration tests (pointermove listener, prop wiring)
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/EditorTabs.tsx`:
- Around line 108-115: The dependency array for the useCallback creating
rootRefCb includes rootElRef even though rootElRef is a stable ref from useRef
and never changes; update the useCallback dependencies to only include ref
(i.e., change [ref, rootElRef] to [ref]) so rootRefCb won't be recreated
unnecessarily; keep the function body as-is referencing rootElRef.current.
- Around line 143-147: The snapshot currently uses Record<string,string> then is
double-cast to React.CSSProperties; instead declare snapshot as a combined type
that satisfies both shapes so you can assign directly to
cssVarSnapshotRef.current: e.g. const snapshot: Partial<Record<string,string>> &
React.CSSProperties = {}; then populate it with computed.getPropertyValue(key)
for each key from CSS_VAR_SNAPSHOT_KEYS and set cssVarSnapshotRef.current =
snapshot; this uses symbols CSS_VAR_SNAPSHOT_KEYS, computed.getPropertyValue,
and cssVarSnapshotRef to locate the change.
- Around line 138-153: The useCallback for handleDragStart includes stable refs
rootElRef and cssVarSnapshotRef in its dependency array even though their
.current identities never change; update the dependency array of handleDragStart
to remove rootElRef and cssVarSnapshotRef and only include the actual changing
dependencies (e.g., reorder and detach) so the callback isn't needlessly
recreated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1bd505fb-5e8a-4949-8ca8-42126c406ef6
📒 Files selected for processing (5)
packages/base-ui/src/components/editor-tabs/DetachGhostTab.tsxpackages/base-ui/src/components/editor-tabs/EditorTabs.stories.tsxpackages/base-ui/src/components/editor-tabs/EditorTabs.test.tsxpackages/base-ui/src/components/editor-tabs/EditorTabs.tsxpackages/base-ui/src/components/editor-tabs/modifiers/createDetachAwareModifier.test.ts
- Remove stable refs from useCallback dependency arrays - Use intersection type for CSS var snapshot to avoid double cast
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/EditorTabs.tsx`:
- Line 139: Replace the verbose Parameters<...> type usage in the DndContext
drag handlers with the direct types exported from `@dnd-kit/core`: import
DragStartEvent and DragEndEvent and use those types for the onDragStart and
onDragEnd handler parameters (the current anonymous parameter at the location
using Parameters<NonNullable<React.ComponentProps<typeof
DndContext>['onDragStart']>>[0] and the corresponding onDragEnd signature around
the other handler). This simplifies the signatures in EditorTabs.tsx and keeps
the handler parameter types clear and explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 54f50623-af5c-4586-9bdc-ca90acc11507
📒 Files selected for processing (1)
packages/base-ui/src/components/editor-tabs/EditorTabs.tsx
Replace verbose Parameters<NonNullable<...>> type extraction with direct imports from @dnd-kit/core.
This was referenced Mar 8, 2026
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
detachThresholdPx, default 18px)onDetachCommit({ id, payload, screenX, screenY })when a tab is dropped in detach mode, enabling the host app (Wails) to spawn a new windowdetachableprop (defaulttrue) can disable detach entirelyKey implementation details
useTabDetachhook — nativepointermovelistener for pointer tracking with hysteresis to prevent flicker near the threshold boundarycreateDetachAwareModifier— single modifier replacingrestrictToHorizontalAxis+restrictToVisibleScrollArea, mode-aware (horizontal-only in reorder, free movement in detach)DragOverlaywith CSS variable snapshot — snapshots computed--_ov-*variables at drag start and injects as inline styles on the portal, solving the CSS variable loss issueDetachGhostTab— lightweight non-interactive tab clone for the overlaypointermoveevents)New files
hooks/useTabDetach.ts+ testmodifiers/createDetachAwareModifier.ts+ testDetachGhostTab.tsxStories
detachable={false}, vertical drag locked to horizontalTest plan
pnpm typecheckpassespnpm testpasses (263 tests, 52 files)detachable={false}locks to horizontal onlySummary by CodeRabbit
New Features
Style
Tests
Chores