[codex] Allow pane drops into new tabs#3809
Conversation
📝 WalkthroughWalkthroughThis pull request adds drag-and-drop support for moving panes to different tab indices. The store's Changes
Sequence DiagramsequenceDiagram
actor User
participant TabBar
participant Workspace
participant Store
User->>TabBar: Drag pane & drop at index
activate TabBar
TabBar->>TabBar: Detect drop item type & position
alt Drop item is pane
TabBar->>Workspace: onMovePaneToNewTab(paneId, toIndex)
activate Workspace
Workspace->>Store: movePaneToNewTab({paneId, toIndex})
activate Store
Store->>Store: Create new tab at adjusted index
Store->>Store: Transfer pane to new tab
Store->>Store: Update activeTabId & layout
Store-->>Workspace: Updated state
deactivate Store
Workspace-->>TabBar: State updated
deactivate Workspace
end
TabBar->>TabBar: Clear insertion indicator
deactivate TabBar
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR enables pane dragging onto the v2 tab bar to create a new tab at the drop position. The Confidence Score: 5/5Safe to merge — all findings are P2 style suggestions with no correctness impact The index-adjustment logic is correct and well-covered by two targeted tests. The drop-handler type-guarding is sound (both getItemType() and 'paneId' in item), and the visual indicator is properly cleared before any early returns. Both remaining comments are P2 organizational suggestions. No files require special attention
|
| Filename | Overview |
|---|---|
| packages/panes/src/core/store/store.ts | Extended movePaneToNewTab to accept an optional toIndex; added index-tracking loop and offset-adjustment logic to handle source-tab removal correctly |
| packages/panes/src/core/store/store.test.ts | Added two movePaneToNewTab test cases covering mid-list insertion and source-tab removal with index adjustment |
| packages/panes/src/react/components/Workspace/Workspace.tsx | Wires onMovePaneToNewTab prop through to the store's movePaneToNewTab action |
| packages/panes/src/react/components/Workspace/components/TabBar/TabBar.tsx | Expanded drop zone to the root div (was OverflowFadeContainer), added PANE_DRAG_TYPE to accepted drag types, dispatches movePaneToNewTab on pane drop with the computed insert index |
Sequence Diagram
sequenceDiagram
participant User
participant TabBar
participant DnD as react-dnd
participant Store as WorkspaceStore
User->>DnD: drag pane over tab bar
DnD->>TabBar: hover(PaneDragItem, monitor)
TabBar->>TabBar: computeInsertIndex → insertIndexRef
TabBar->>TabBar: setInsertIndex (show indicator)
User->>DnD: drop pane
DnD->>TabBar: drop(PaneDragItem, monitor)
TabBar->>TabBar: read insertIndexRef (idx)
TabBar->>TabBar: clear insertIndexRef + setInsertIndex(null)
TabBar->>TabBar: check monitor.getItemType() === PANE_DRAG_TYPE
TabBar->>Store: movePaneToNewTab({ paneId, toIndex: idx })
Store->>Store: find sourceTab + sourceTabIndex
Store->>Store: removePaneFromLayout → nextSourceLayout
Store->>Store: compute adjustedIndex (offset if source tab removed before toIndex)
Store->>Store: splice newTab into nextTabs at insertIndex
Store->>Store: return { tabs: nextTabs, activeTabId: newTab.id }
Comments Outside Diff (1)
-
packages/panes/src/react/components/Workspace/components/TabBar/TabBar.tsx, line 165-176 (link)Pane drop silently no-ops when
tabs.length === 0In the empty-state branch (
tabs.length === 0),tabsTrackRef.currentisnull(the track div isn't rendered). Thehoverhandler short-circuits immediately, leavinginsertIndexRef.current === null, so a pane dropped here returns early without creating a new tab. In practice this is unreachable — a pane always lives in some tab sotabs.lengthis at least 1 — but it may be worth an early exit or a comment for clarity.Prompt To Fix With AI
This is a comment left during a code review. Path: packages/panes/src/react/components/Workspace/components/TabBar/TabBar.tsx Line: 165-176 Comment: **Pane drop silently no-ops when `tabs.length === 0`** In the empty-state branch (`tabs.length === 0`), `tabsTrackRef.current` is `null` (the track div isn't rendered). The `hover` handler short-circuits immediately, leaving `insertIndexRef.current === null`, so a pane dropped here returns early without creating a new tab. In practice this is unreachable — a pane always lives in some tab so `tabs.length` is at least 1 — but it may be worth an early exit or a comment for clarity. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/panes/src/react/components/Workspace/components/TabBar/TabBar.tsx
Line: 18
Comment:
**Cross-component coupling for `PANE_DRAG_TYPE`**
`TabBar` now imports `PANE_DRAG_TYPE` from a deeply nested peer component's barrel (`../Tab/components/Pane/components/PaneHeader`). `TabItem.tsx` already does the same, but the PR adds a second consumer, making the coupling more visible. Consider hoisting this constant to a shared `dnd-types.ts` file (e.g., alongside `TAB_DRAG_TYPE` in `TabBar/components/TabItem`) so both `TabBar` and `TabItem` can import from a common location rather than from a UI component that happens to export it.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/panes/src/react/components/Workspace/components/TabBar/TabBar.tsx
Line: 165-176
Comment:
**Pane drop silently no-ops when `tabs.length === 0`**
In the empty-state branch (`tabs.length === 0`), `tabsTrackRef.current` is `null` (the track div isn't rendered). The `hover` handler short-circuits immediately, leaving `insertIndexRef.current === null`, so a pane dropped here returns early without creating a new tab. In practice this is unreachable — a pane always lives in some tab so `tabs.length` is at least 1 — but it may be worth an early exit or a comment for clarity.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Allow pane drops into new tabs" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } from "react"; | ||
| import { useDrop } from "react-dnd"; | ||
| import type { Tab } from "../../../../../types"; | ||
| import { PANE_DRAG_TYPE } from "../Tab/components/Pane/components/PaneHeader"; |
There was a problem hiding this comment.
PANE_DRAG_TYPE
TabBar now imports PANE_DRAG_TYPE from a deeply nested peer component's barrel (../Tab/components/Pane/components/PaneHeader). TabItem.tsx already does the same, but the PR adds a second consumer, making the coupling more visible. Consider hoisting this constant to a shared dnd-types.ts file (e.g., alongside TAB_DRAG_TYPE in TabBar/components/TabItem) so both TabBar and TabItem can import from a common location rather than from a UI component that happens to export it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/panes/src/react/components/Workspace/components/TabBar/TabBar.tsx
Line: 18
Comment:
**Cross-component coupling for `PANE_DRAG_TYPE`**
`TabBar` now imports `PANE_DRAG_TYPE` from a deeply nested peer component's barrel (`../Tab/components/Pane/components/PaneHeader`). `TabItem.tsx` already does the same, but the PR adds a second consumer, making the coupling more visible. Consider hoisting this constant to a shared `dnd-types.ts` file (e.g., alongside `TAB_DRAG_TYPE` in `TabBar/components/TabItem`) so both `TabBar` and `TabItem` can import from a common location rather than from a UI component that happens to export it.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/panes/src/core/store/store.ts (1)
794-856: LGTM — insertion math handles the source-removal case correctly.I traced the cases (source at index 0 / middle / last, source kept vs. removed,
toIndexat 0 / equal to source / past end / undefined) and the result lines up with the tab-bar's "drop where the cursor is" intent in every case. ThesourceTabIndex < args.toIndexguard is the right condition: it only decrements when the removed source tab sat before the requested insertion point in the pre-removal array.Minor optional readability tweak — the dual
requestedIndex/adjustedIndexternary is a bit dense for what is effectively "subtract 1 if the removed source tab was to the left of the insertion point, then clamp":♻️ Optional refactor
- const requestedIndex = args.toIndex ?? nextTabs.length; - const adjustedIndex = - args.toIndex !== undefined && - !nextSourceLayout && - sourceTabIndex < args.toIndex - ? args.toIndex - 1 - : requestedIndex; - const insertIndex = Math.max( - 0, - Math.min(adjustedIndex, nextTabs.length), - ); - - nextTabs.splice(insertIndex, 0, newTab); + let insertIndex = args.toIndex ?? nextTabs.length; + // If the source tab was removed and sat before the requested + // insertion point, the trailing tabs have shifted left by one. + if ( + args.toIndex !== undefined && + !nextSourceLayout && + sourceTabIndex < args.toIndex + ) { + insertIndex -= 1; + } + insertIndex = Math.max(0, Math.min(insertIndex, nextTabs.length)); + + nextTabs.splice(insertIndex, 0, newTab);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/panes/src/core/store/store.ts` around lines 794 - 856, The insertion-index math in movePaneToNewTab is correct but a bit dense; replace the requestedIndex/adjustedIndex ternary with a single, clearer computation that computes a baseIndex = args.toIndex ?? nextTabs.length, subtracts 1 when (args.toIndex !== undefined && !nextSourceLayout && sourceTabIndex < args.toIndex), then clamps the result between 0 and nextTabs.length to produce insertIndex; update references to requestedIndex/adjustedIndex accordingly and keep the existing conditions using sourceTabIndex and nextSourceLayout.packages/panes/src/core/store/store.test.ts (1)
601-641: LGTM — both critical paths (source tab kept vs. removed) are covered.The "stable insertion" test is the right one to lock in: it pins the adjustment-on-removal behavior in
movePaneToNewTaband would catch a regression to a naivesplice(args.toIndex, 0, newTab)afternextTabsfilter.Optional additional cases worth pinning down later if you want belt-and-suspenders on the clamp/append paths:
toIndexomitted → new tab appended at the end.toIndexlarger thantabs.length→ clamped to end.- Source tab is the last tab and gets removed with
toIndexat/after end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/panes/src/core/store/store.test.ts` around lines 601 - 641, Tests already cover the two critical paths for movePaneToNewTab (source tab kept vs removed); add three extra unit cases in store.test.ts (using makeStore and tp helpers) to harden edge behavior: (1) omit toIndex to assert the new tab is appended, (2) set toIndex larger than tabs.length to assert it clamps/appends to the end, and (3) have the source tab be the last tab and removed while toIndex is at/after end to assert insertion position remains correct; keep assertions for tabs order, newTab.panes.<id>, newTab.activePaneId, layout, and store.getState().activeTabId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/panes/src/core/store/store.test.ts`:
- Around line 601-641: Tests already cover the two critical paths for
movePaneToNewTab (source tab kept vs removed); add three extra unit cases in
store.test.ts (using makeStore and tp helpers) to harden edge behavior: (1) omit
toIndex to assert the new tab is appended, (2) set toIndex larger than
tabs.length to assert it clamps/appends to the end, and (3) have the source tab
be the last tab and removed while toIndex is at/after end to assert insertion
position remains correct; keep assertions for tabs order, newTab.panes.<id>,
newTab.activePaneId, layout, and store.getState().activeTabId.
In `@packages/panes/src/core/store/store.ts`:
- Around line 794-856: The insertion-index math in movePaneToNewTab is correct
but a bit dense; replace the requestedIndex/adjustedIndex ternary with a single,
clearer computation that computes a baseIndex = args.toIndex ?? nextTabs.length,
subtracts 1 when (args.toIndex !== undefined && !nextSourceLayout &&
sourceTabIndex < args.toIndex), then clamps the result between 0 and
nextTabs.length to produce insertIndex; update references to
requestedIndex/adjustedIndex accordingly and keep the existing conditions using
sourceTabIndex and nextSourceLayout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 770de858-b43a-4844-89e9-705ae61b24a2
📒 Files selected for processing (4)
packages/panes/src/core/store/store.test.tspackages/panes/src/core/store/store.tspackages/panes/src/react/components/Workspace/Workspace.tsxpackages/panes/src/react/components/Workspace/components/TabBar/TabBar.tsx
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/panes/src/react/components/Workspace/components/TabBar/TabBar.tsx">
<violation number="1" location="packages/panes/src/react/components/Workspace/components/TabBar/TabBar.tsx:95">
P2: Pane drop-to-new-tab is a no-op when the target workspace has zero tabs because insert index is never computed in the empty-tab-bar path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| hover: (_item: TabDragItem | PaneDragItem, monitor) => { | ||
| const track = tabsTrackRef.current; | ||
| const offset = monitor.getClientOffset(); | ||
| if (!track || !offset) return; |
There was a problem hiding this comment.
P2: Pane drop-to-new-tab is a no-op when the target workspace has zero tabs because insert index is never computed in the empty-tab-bar path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/panes/src/react/components/Workspace/components/TabBar/TabBar.tsx, line 95:
<comment>Pane drop-to-new-tab is a no-op when the target workspace has zero tabs because insert index is never computed in the empty-tab-bar path.</comment>
<file context>
@@ -85,8 +91,8 @@ export function TabBar<TData>({
- accept: TAB_DRAG_TYPE,
- hover: (_item, monitor) => {
+ accept: [TAB_DRAG_TYPE, PANE_DRAG_TYPE],
+ hover: (_item: TabDragItem | PaneDragItem, monitor) => {
const track = tabsTrackRef.current;
const offset = monitor.getClientOffset();
</file context>
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
Root Cause
The shared v2 tab bar only accepted tab drag items. Pane drags could switch to existing tabs through tab items or split onto panes, but drops on the top bar itself were ignored.
Validation
Summary by cubic
Allow dropping a pane onto the v2 tab bar to create a new tab at the drop position. The new tab is activated and the insertion index stays stable even if the source tab is removed.
onMovePaneToNewTabtoTabBarand wire it throughWorkspace.movePaneToNewTab({ paneId, toIndex? })and add tests inpackages/panes.Written for commit e9b2d5d. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Tests