fix: consolidate tab drag-and-drop to native HTML5 API#74
Merged
Conversation
📝 WalkthroughWalkthroughReplaces Framer Motion reordering with a native drag-and-drop tab reorder flow: adds before/after drop positions, visual drop indicators, per-tab drag handlers and state, ReorderPreview APIs in the pane DnD hook, and expanded tests covering multi-tab drag scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Tab as Tab Component
participant TabBar as WorkspaceTabBar
participant PaneDnd as PaneDnd Hook
participant Store as Workspace Store
User->>Tab: dragstart
Tab->>TabBar: onDragStart(payload)
TabBar->>PaneDnd: startTabDrag(payload)
PaneDnd->>PaneDnd: set drag state
User->>Tab: dragover target tab
Tab->>TabBar: onDragOver(event)
TabBar->>TabBar: computeTabPosition(cursor)
TabBar->>PaneDnd: setReorderPreview(paneId,targetTabId,position)
PaneDnd->>PaneDnd: update reorderPreview
Tab->>Tab: render drop indicator (before/after)
User->>Tab: drop
Tab->>TabBar: onDrop(event)
TabBar->>PaneDnd: handleTabReorder(sourcePaneId,targetTabId,position)
PaneDnd->>Store: reorderTabsInPane(...)
Store->>Store: update tab order
PaneDnd->>PaneDnd: clearReorderPreview()
Tab->>Tab: clear dragging visuals
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (5)
src/renderer/components/workspace/WorkspaceTabBar.tsx (3)
388-431: Consider extracting duplicated position calculation.The position calculation logic (getting bounding rect, computing half-width, determining
'before'/'after') is duplicated inhandleTabDragOverandhandleTabDrop. Consider extracting to a helper function for maintainability.♻️ Proposed refactor
+ const getDropPosition = useCallback((e: React.DragEvent): 'before' | 'after' => { + const target = e.currentTarget as HTMLElement + const rect = target.getBoundingClientRect() + const x = e.clientX - rect.left + return x < rect.width / 2 ? 'before' : 'after' + }, []) + const handleTabDragOver = useCallback( (tabId: string, e: React.DragEvent) => { e.preventDefault() e.dataTransfer.dropEffect = 'move' if (!dragPayload || dragPayload.type !== 'tab') return if (dragPayload.sourcePaneId !== paneId) return - // Calculate position based on mouse X within the tab - const target = e.currentTarget as HTMLElement - const rect = target.getBoundingClientRect() - const x = e.clientX - rect.left - const halfWidth = rect.width / 2 - - const position: 'before' | 'after' = x < halfWidth ? 'before' : 'after' + const position = getDropPosition(e) setReorderPreview(paneId, tabId, position) }, - [dragPayload, paneId, setReorderPreview] + [dragPayload, paneId, setReorderPreview, getDropPosition] )Apply similar change to
handleTabDrop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/workspace/WorkspaceTabBar.tsx` around lines 388 - 431, The duplicated logic that computes the drop position in handleTabDragOver and handleTabDrop should be extracted to a small helper (e.g., getTabPositionFromMouse or computeTabPosition) which accepts the target element (or DragEvent/currentTarget) and clientX and returns 'before' | 'after'; replace the rect/getBoundingClientRect/halfWidth/x comparisons in both handleTabDragOver and handleTabDrop with calls to that helper and keep all existing early-return checks and subsequent calls to setReorderPreview and handleTabReorder unchanged.
58-58: Remove unusedtabRef.The
tabRefis created and attached to the div but never read or used for any measurements or imperative actions. Consider removing it to reduce unnecessary code.🧹 Proposed fix
const inputRef = useRef<HTMLInputElement>(null) - const tabRef = useRef<HTMLDivElement>(null)And remove
ref={tabRef}from the div at line 88.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/workspace/WorkspaceTabBar.tsx` at line 58, Remove the unused ref by deleting the tabRef declaration (const tabRef = useRef<HTMLDivElement>(null)) and removing the ref={tabRef} prop on the corresponding div in WorkspaceTabBar.tsx; since tabRef is never read or used for measurements/imperative actions, eliminate the import/useRef usage if it becomes unused to keep the file clean.
474-501: IIFE pattern in JSX is unusual.The IIFE
(() => { ... })()for rendering terminal tabs works but is unconventional. Consider extracting to a small inline component or computing aterminalMapoutside the render loop for cleaner code.♻️ Alternative approach with pre-computed map
Add before the return statement:
const terminalMap = new Map(terminalStoreTerminals.map(t => [t.id, t]))Then simplify the rendering:
- {tab.type === 'terminal' ? ( - (() => { - const terminal = terminalStoreTerminals.find((t) => t.id === tab.terminalId) - if (!terminal) return null - return ( - <TerminalTabInline + {tab.type === 'terminal' ? ( + terminalMap.get(tab.terminalId) ? ( + <TerminalTabInline + terminal={terminalMap.get(tab.terminalId)!} ... - /> - ) - })() + /> + ) : null ) : (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/workspace/WorkspaceTabBar.tsx` around lines 474 - 501, The IIFE used to render terminal tabs is unconventional; compute a lookup (e.g., terminalMap = new Map(terminalStoreTerminals.map(t => [t.id, t]))) once before the component return and replace the IIFE with a simple conditional that does const terminal = terminalMap.get(tab.terminalId) and returns the <TerminalTabInline> only when terminal exists. Keep all existing props and handlers (isActive check using activeTabId, setActiveTab, setActivePane, onCloseTerminal, onRenameTerminal, handleTabDragStart, handleTabDragOver, handleTabDragLeave, handleTabDrop) unchanged so behavior remains identical.src/renderer/components/workspace/WorkspaceTabBar.test.tsx (2)
321-329: Inconsistent approach for settingclientXin drag events.The first dragover test (lines 269-274) uses
createEventwithObject.definePropertyto setclientX, while this test usesfireEvent.dragOverwithclientXin options. The latter approach may not reliably set the event'sclientXproperty depending on the testing library internals.Consider using the same
createEvent+Object.definePropertyapproach for consistency and reliability:♻️ Proposed fix for consistency
- // Drag over right half (x = 150, which is > 100) - fireEvent.dragOver(targetTab, { - clientX: 150, - clientY: 20, - dataTransfer: { dropEffect: null } - }) + // Drag over right half (x = 150, which is > 100) + const dragEvent = createEvent.dragOver(targetTab, { + dataTransfer: { dropEffect: null } + }) + Object.defineProperty(dragEvent, 'clientX', { value: 150, writable: false }) + Object.defineProperty(dragEvent, 'clientY', { value: 20, writable: false }) + fireEvent(targetTab, dragEvent)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/workspace/WorkspaceTabBar.test.tsx` around lines 321 - 329, The test uses fireEvent.dragOver with a clientX option which can be unreliable; change this dragover simulation to use createEvent('dragover') and set clientX via Object.defineProperty on the created event (the same pattern used earlier), dispatch it on targetTab, and then assert mockSetReorderPreview was called with ('pane-a','tab-2','after'); update the test that currently references targetTab and mockSetReorderPreview to use the createEvent + Object.defineProperty approach for consistency and reliability.
373-380: SameclientXreliability concern in drop test.Similar to the dragover test, consider using
createEvent+Object.definePropertyfor settingclientXreliably:♻️ Proposed fix
- // Drop on right half - fireEvent.drop(targetTab, { - clientX: 150, - clientY: 20 - }) + // Drop on right half + const dropEvent = createEvent.drop(targetTab) + Object.defineProperty(dropEvent, 'clientX', { value: 150, writable: false }) + Object.defineProperty(dropEvent, 'clientY', { value: 20, writable: false }) + fireEvent(targetTab, dropEvent)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/workspace/WorkspaceTabBar.test.tsx` around lines 373 - 380, The drop test in WorkspaceTabBar.test.tsx uses fireEvent.drop with clientX which can be unreliable; instead create a drop event via createEvent('drop') for targetTab, use Object.defineProperty to set event.clientX (and clientY if needed) to 150/20, then dispatch it on targetTab (e.g. targetTab.dispatchEvent) so mockHandleTabReorder('pane-a','tab-2','after') is called deterministically; replace the existing fireEvent.drop call with this createEvent + defineProperty + dispatch approach referencing targetTab and mockHandleTabReorder.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/components/workspace/WorkspaceTabBar.tsx`:
- Around line 167-172: The "Kill Process" menu item currently reuses onClose;
change it to call a distinct handler (e.g., onKillProcess) by adding an optional
onKillProcess prop to the WorkspaceTabBar component props and replacing the
onClick for the menu entry with onKillProcess (or, if you prefer, onKillProcess
?? onClose to preserve behavior when not provided); update the component's prop
type/interface and all call sites where WorkspaceTabBar is instantiated to pass
a forceful termination handler (or omit to fall back), and ensure any referenced
function name is unique (onKillProcess) and wired to the terminal kill logic
rather than the regular close flow.
---
Nitpick comments:
In `@src/renderer/components/workspace/WorkspaceTabBar.test.tsx`:
- Around line 321-329: The test uses fireEvent.dragOver with a clientX option
which can be unreliable; change this dragover simulation to use
createEvent('dragover') and set clientX via Object.defineProperty on the created
event (the same pattern used earlier), dispatch it on targetTab, and then assert
mockSetReorderPreview was called with ('pane-a','tab-2','after'); update the
test that currently references targetTab and mockSetReorderPreview to use the
createEvent + Object.defineProperty approach for consistency and reliability.
- Around line 373-380: The drop test in WorkspaceTabBar.test.tsx uses
fireEvent.drop with clientX which can be unreliable; instead create a drop event
via createEvent('drop') for targetTab, use Object.defineProperty to set
event.clientX (and clientY if needed) to 150/20, then dispatch it on targetTab
(e.g. targetTab.dispatchEvent) so mockHandleTabReorder('pane-a','tab-2','after')
is called deterministically; replace the existing fireEvent.drop call with this
createEvent + defineProperty + dispatch approach referencing targetTab and
mockHandleTabReorder.
In `@src/renderer/components/workspace/WorkspaceTabBar.tsx`:
- Around line 388-431: The duplicated logic that computes the drop position in
handleTabDragOver and handleTabDrop should be extracted to a small helper (e.g.,
getTabPositionFromMouse or computeTabPosition) which accepts the target element
(or DragEvent/currentTarget) and clientX and returns 'before' | 'after'; replace
the rect/getBoundingClientRect/halfWidth/x comparisons in both handleTabDragOver
and handleTabDrop with calls to that helper and keep all existing early-return
checks and subsequent calls to setReorderPreview and handleTabReorder unchanged.
- Line 58: Remove the unused ref by deleting the tabRef declaration (const
tabRef = useRef<HTMLDivElement>(null)) and removing the ref={tabRef} prop on the
corresponding div in WorkspaceTabBar.tsx; since tabRef is never read or used for
measurements/imperative actions, eliminate the import/useRef usage if it becomes
unused to keep the file clean.
- Around line 474-501: The IIFE used to render terminal tabs is unconventional;
compute a lookup (e.g., terminalMap = new Map(terminalStoreTerminals.map(t =>
[t.id, t]))) once before the component return and replace the IIFE with a simple
conditional that does const terminal = terminalMap.get(tab.terminalId) and
returns the <TerminalTabInline> only when terminal exists. Keep all existing
props and handlers (isActive check using activeTabId, setActiveTab,
setActivePane, onCloseTerminal, onRenameTerminal, handleTabDragStart,
handleTabDragOver, handleTabDragLeave, handleTabDrop) unchanged so behavior
remains identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2dd45a4a-5da7-4acd-bbca-b236838e579a
📒 Files selected for processing (4)
src/renderer/components/workspace/WorkspaceTabBar.test.tsxsrc/renderer/components/workspace/WorkspaceTabBar.tsxsrc/renderer/hooks/use-pane-dnd.tsxsrc/renderer/types/workspace.types.ts
Comment on lines
+167
to
+172
| { | ||
| label: 'Kill Process', | ||
| icon: <Skull size={14} />, | ||
| onClick: onClose, | ||
| variant: 'danger' | ||
| } |
There was a problem hiding this comment.
"Kill Process" action is identical to "Close".
Both "Kill Process" and "Close" call onClose. If "Kill Process" is meant to forcefully terminate the terminal process without prompts, it should invoke a different handler (e.g., onKillProcess). Otherwise, consider removing the redundant menu item.
Would you like me to help design a separate onKillProcess callback that forcefully terminates the terminal?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/workspace/WorkspaceTabBar.tsx` around lines 167 -
172, The "Kill Process" menu item currently reuses onClose; change it to call a
distinct handler (e.g., onKillProcess) by adding an optional onKillProcess prop
to the WorkspaceTabBar component props and replacing the onClick for the menu
entry with onKillProcess (or, if you prefer, onKillProcess ?? onClose to
preserve behavior when not provided); update the component's prop type/interface
and all call sites where WorkspaceTabBar is instantiated to pass a forceful
termination handler (or omit to fall back), and ensure any referenced function
name is unique (onKillProcess) and wired to the terminal kill logic rather than
the regular close flow.
gnoviawan
force-pushed
the
fix/tab-drag-reorder-conflict
branch
from
d67be9f to
08fa5f4
Compare
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 `@src/renderer/components/workspace/WorkspaceTabBar.tsx`:
- Around line 408-411: The container's dragleave handler is clearing
reorderPreview too eagerly—modify the drag leave logic (referencing
handleTabDragLeave and wherever setReorderPreview/reorderPreview is cleared) to
only clear the preview when the pointer has actually left the tab strip
container (use the event's relatedTarget / relatedElement check or test
e.currentTarget.contains(e.relatedTarget) in the dragleave handler) so moves
between child tabs don't remove the indicator; apply the same fix in the other
dragleave location noted (the second handler around line 458).
- Around line 413-423: The handler handleTabDrop currently calls
e.preventDefault() and e.stopPropagation() immediately, blocking pane-level drop
logic for cross-pane tab drags or file drops; change it so the event is only
prevented/stopped after confirming this is a same-pane tab reorder: check
dragPayload && dragPayload.type === 'tab' && dragPayload.sourcePaneId === paneId
first, and only then call e.preventDefault()/e.stopPropagation(),
computeTabPosition(...) and call handleTabReorder(paneId, tabId, position); this
preserves bubbling for cross-pane moves and file drops while keeping tab-reorder
behavior intact.
In `@src/renderer/types/workspace.types.ts`:
- Line 22: The DropPosition union must not include 'before'|'after'; introduce a
new TabReorderPosition = 'before'|'after' and remove those two members from
DropPosition; then update any APIs and usages that are tab-specific to accept
TabReorderPosition instead of DropPosition (notably the functions and types
referenced in use-pane-dnd.tsx and workspace-store.ts) so pane-drop logic only
sees 'left'|'right'|'top'|'bottom'|'center' while tab-reorder code uses
TabReorderPosition for exhaustive typing and proper rejection of invalid pane
drops.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61d7678a-ce32-4856-ac62-cf2df57983a5
📒 Files selected for processing (4)
src/renderer/components/workspace/WorkspaceTabBar.test.tsxsrc/renderer/components/workspace/WorkspaceTabBar.tsxsrc/renderer/hooks/use-pane-dnd.tsxsrc/renderer/types/workspace.types.ts
| export type PaneNode = SplitNode | LeafNode | ||
|
|
||
| export type DropPosition = 'left' | 'right' | 'top' | 'bottom' | 'center' | ||
| export type DropPosition = 'left' | 'right' | 'top' | 'bottom' | 'center' | 'before' | 'after' |
There was a problem hiding this comment.
Keep pane drop zones and tab reorder positions as separate types.
Expanding DropPosition here makes 'before'/'after' legal anywhere pane-drop positions are accepted. That now typechecks through src/renderer/hooks/use-pane-dnd.tsx and src/renderer/stores/workspace-store.ts, where those values fall through as vertical/trailing splits instead of being rejected. A separate TabReorderPosition keeps the pane-drop paths exhaustively typed.
Possible shape
-export type DropPosition = 'left' | 'right' | 'top' | 'bottom' | 'center' | 'before' | 'after'
+export type PaneDropPosition = 'left' | 'right' | 'top' | 'bottom' | 'center'
+export type TabReorderPosition = 'before' | 'after'
+export type DropPosition = PaneDropPosition📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export type DropPosition = 'left' | 'right' | 'top' | 'bottom' | 'center' | 'before' | 'after' | |
| export type PaneDropPosition = 'left' | 'right' | 'top' | 'bottom' | 'center' | |
| export type TabReorderPosition = 'before' | 'after' | |
| export type DropPosition = PaneDropPosition |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/types/workspace.types.ts` at line 22, The DropPosition union
must not include 'before'|'after'; introduce a new TabReorderPosition =
'before'|'after' and remove those two members from DropPosition; then update any
APIs and usages that are tab-specific to accept TabReorderPosition instead of
DropPosition (notably the functions and types referenced in use-pane-dnd.tsx and
workspace-store.ts) so pane-drop logic only sees
'left'|'right'|'top'|'bottom'|'center' while tab-reorder code uses
TabReorderPosition for exhaustive typing and proper rejection of invalid pane
drops.
08fa5f4 to
42cd048
Compare
Remove Framer Motion Reorder component which conflicted with native drag events. Implement single native HTML5 drag system for both: - Intra-pane tab reordering (drag tabs left/right within tab bar) - Cross-pane tab movement (drag tabs to different panes/splits) Changes: - Extended DropPosition type with 'before'/'after' for tab-relative drops - Added reorderPreview state and handleTabReorder to PaneDndContext - Replaced Reorder.Group/Item with plain divs and native drag handlers - Added visual drop indicator line for tab reorder preview - Added CSS transitions for smooth tab movement - Updated tests to remove Framer Motion mocks and test native drag Co-Authored-By: Claude <noreply@anthropic.com>
gnoviawan
force-pushed
the
fix/tab-drag-reorder-conflict
branch
from
42cd048 to
20f4bb9
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/renderer/components/workspace/WorkspaceTabBar.tsx (1)
174-179:⚠️ Potential issue | 🟡 MinorWire "Kill Process" to a distinct action.
Line 177 still calls
onClose, so "Kill Process" behaves exactly like the normal close action. This menu item is still misleading unless it invokes a dedicated kill handler.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/workspace/WorkspaceTabBar.tsx` around lines 174 - 179, The "Kill Process" menu item is wired to onClose (so it just closes the tab); change it to call a dedicated kill handler instead: add a new prop like onKillProcess (or onKill) to the WorkspaceTabBar component's props/interface, replace the onClick for the menu item (label: 'Kill Process') to call that new handler, and update all parent usages to pass a proper kill implementation; if no handler is supplied, make the menu item disabled or fall back to onClose only after explicit confirmation to avoid surprise behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/renderer/components/workspace/WorkspaceTabBar.tsx`:
- Around line 174-179: The "Kill Process" menu item is wired to onClose (so it
just closes the tab); change it to call a dedicated kill handler instead: add a
new prop like onKillProcess (or onKill) to the WorkspaceTabBar component's
props/interface, replace the onClick for the menu item (label: 'Kill Process')
to call that new handler, and update all parent usages to pass a proper kill
implementation; if no handler is supplied, make the menu item disabled or fall
back to onClose only after explicit confirmation to avoid surprise behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ebe07e28-2375-4bd8-ac2e-93ab84f439ec
📒 Files selected for processing (4)
src/renderer/components/workspace/WorkspaceTabBar.test.tsxsrc/renderer/components/workspace/WorkspaceTabBar.tsxsrc/renderer/hooks/use-pane-dnd.tsxsrc/renderer/types/workspace.types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/components/workspace/WorkspaceTabBar.test.tsx
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
Reordercomponent which conflicted with native HTML5 drag eventsProblem Solved
Users could not reliably reorder tabs within a pane because two competing drag systems (Framer Motion Reorder and native HTML5 drag) fired simultaneously on the same tab elements.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests