feat(desktop): pane context menus + binary tree layout#3196
Conversation
- Add data-driven context menu system to @superset/panes with ContextMenuActionConfig type and recursive rendering - Wire up context menus for all v2 pane types (terminal, chat, browser, file) with pane-specific actions (copy/paste/clear/scroll for terminal) - Add terminal hotkeys (CLEAR_TERMINAL, SCROLL_TO_BOTTOM) and scroll-to-bottom button to v2 terminal pane - Add movePaneToTab/movePaneToNewTab store actions for Move to Tab submenu - Rewrite layout model from N-ary splits (children[]/weights[]) to strict binary tree (first/second/splitPercentage) matching react-mosaic's model - Replace split node IDs with path-based addressing (SplitPath) - Equalize now uses leaf-count-weighted splitPercentage recursively - Pane removal uses sibling promotion (only neighbor affected)
📝 WalkthroughWalkthroughAdds a hierarchical, customizable pane context menu system; refactors split layouts from array-based weights to binary tree (first/second) with path-based APIs; extends terminal runtime registry with selection, clear, scroll-to-bottom, paste, and terminal access methods; and plumbs context-menu actions through Workspace/Tab/Pane components. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as Pane (Tab/Workspace)
participant Registry as TerminalRuntimeRegistry
participant Terminal
participant Clipboard
participant Store
User->>UI: Right-click pane (open context menu)
UI->>UI: resolve workspace + pane contextMenuActions
User->>UI: Select "Paste"
UI->>Clipboard: readText()
Clipboard-->>UI: text
UI->>Registry: paste(terminalId, text)
Registry->>Terminal: paste text / insertText()
User->>UI: Select "Copy"
UI->>Registry: getSelection(terminalId)
Registry-->>UI: selectedText
UI->>Clipboard: writeText(selectedText)
User->>UI: Select "Clear Terminal"
UI->>Registry: clear(terminalId)
Registry->>Terminal: clear()
User->>UI: Press hotkey EQUALIZE_PANE_SPLITS
UI->>Store: equalizeTab({ tabId })
Store->>Store: equalizeAllSplits()
Store-->>UI: updated layout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 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 |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 21 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="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx:89">
P2: `terminal` is memoized with `[terminalId]`, so it can stay `null` after attach and break the scroll-to-bottom button wiring. Recompute it each render (or include a dependency that changes after attach).</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx:126">
P2: Handle clipboard write failures instead of firing `writeText()` without rejection handling.</violation>
<violation number="2" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx:139">
P2: Avoid empty `catch` here; log or otherwise surface clipboard read failures so paste issues are diagnosable.
(Based on your team's feedback about handling async failures explicitly and not silently swallowing errors.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| try { | ||
| const text = await navigator.clipboard.readText(); | ||
| if (text) terminalRuntimeRegistry.paste(terminalId, text); | ||
| } catch { |
There was a problem hiding this comment.
P2: Avoid empty catch here; log or otherwise surface clipboard read failures so paste issues are diagnosable.
(Based on your team's feedback about handling async failures explicitly and not silently swallowing errors.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx, line 139:
<comment>Avoid empty `catch` here; log or otherwise surface clipboard read failures so paste issues are diagnosable.
(Based on your team's feedback about handling async failures explicitly and not silently swallowing errors.) </comment>
<file context>
@@ -78,13 +98,84 @@ export function usePaneRegistry(
+ try {
+ const text = await navigator.clipboard.readText();
+ if (text) terminalRuntimeRegistry.paste(terminalId, text);
+ } catch {
+ // Clipboard access denied
+ }
</file context>
| onSelect: (ctx) => { | ||
| const { terminalId } = ctx.pane.data as TerminalPaneData; | ||
| const text = terminalRuntimeRegistry.getSelection(terminalId); | ||
| if (text) navigator.clipboard.writeText(text); |
There was a problem hiding this comment.
P2: Handle clipboard write failures instead of firing writeText() without rejection handling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx, line 126:
<comment>Handle clipboard write failures instead of firing `writeText()` without rejection handling.</comment>
<file context>
@@ -78,13 +98,84 @@ export function usePaneRegistry(
+ onSelect: (ctx) => {
+ const { terminalId } = ctx.pane.data as TerminalPaneData;
+ const text = terminalRuntimeRegistry.getSelection(terminalId);
+ if (text) navigator.clipboard.writeText(text);
+ },
+ },
</file context>
![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
🧹 Nitpick comments (4)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts (1)
235-239: Add a focused regression test for tab-wide equalization.This hotkey now drives
equalizeTab, so it’s worth locking in the nested-split behavior and the single-pane no-op path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts around lines 235 - 239, Add a focused regression test that exercises the tab-wide equalization hotkey behavior: simulate invoking the useHotkey handler for "EQUALIZE_PANE_SPLITS" (or invoke the exported function that triggers it) and assert two scenarios—(1) when the active tab contains nested splits, equalizeTab is called with the active tab id and panes are redistributed as expected, and (2) when the active tab has a single pane, equalizeTab is a no-op (no state change). Locate the hotkey wiring in useWorkspaceHotkeys (useHotkey call) and the equalizeTab action on the store (store.getState().equalizeTab) to invoke/mock the store and active tab (getActiveTab) accordingly; use mocks/stubs for store.getState and equalizeTab to assert the call and state-change/no-op behavior.packages/panes/src/react/components/Workspace/components/Tab/components/Pane/components/PaneContextMenu/PaneContextMenu.tsx (1)
13-16: Consider using a package-level import instead of deep relative path.The import path
"../../../../../../../../types"is fragile and hard to maintain. Since this is within the@superset/panespackage, consider adding a shorter alias or re-exporting from a more accessible location.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/panes/src/react/components/Workspace/components/Tab/components/Pane/components/PaneContextMenu/PaneContextMenu.tsx` around lines 13 - 16, Replace the fragile deep-relative import of ContextMenuActionConfig and RendererContext in PaneContextMenu.tsx with a package-level import by re-exporting those types from the `@superset/panes` package (or the package's top-level types entry) and then importing them like import type { ContextMenuActionConfig, RendererContext } from "@superset/panes"; — update the import statement to reference the package-level module and ensure the package index exports those types.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx (1)
34-36:navigator.platformis deprecated.While still widely supported,
navigator.platformis deprecated. Consider usingnavigator.userAgentData?.platformwith a fallback, or a utility that already handles platform detection elsewhere in the codebase.#!/bin/bash # Check if there's an existing platform detection utility in the codebase rg -n "userAgentData|platform.*mac|isMac" --type=ts apps/desktop/src/renderer/ -l | head -10🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx around lines 34 - 36, Replace the deprecated navigator.platform usage in the MOD_KEY constant with a safe detection that prefers navigator.userAgentData?.platform and falls back to navigator.platform (or a shared utility if one exists), e.g., read platform = navigator.userAgentData?.platform ?? navigator.platform, normalize with .toLowerCase(), then check includes("mac") to choose "⌘" vs "Ctrl+"; update the MOD_KEY definition in usePaneRegistry.tsx accordingly and reuse any existing isMac/isPlatform utility if present.packages/panes/src/core/store/store.test.ts (1)
474-549: Add tests for the two new tab-move mutations.This suite covers
movePaneToSplit, butmovePaneToTabandmovePaneToNewTab— the new state transitions behind the submenu work in this PR — still have no direct assertions here. A nested-layout case would be especially useful to lock down source-tab collapse and focus behavior.🤖 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 474 - 549, Add unit tests in store.test.ts for the two new mutations movePaneToTab and movePaneToNewTab: create scenarios that mirror the existing movePaneToSplit tests (within-tab, across-tab, and no-op on self) and include at least one nested-layout case to verify the source tab collapses correctly and focus/activeTabId is updated as expected; specifically exercise the store.getState().movePaneToTab and store.getState().movePaneToNewTab calls, then assert tabs[].panes, tabs[].layout, activePaneId, and store.getState().activeTabId reflect the intended post-move state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useDefaultContextMenuActions/useDefaultContextMenuActions.tsx:
- Line 29: Replace the terminal-specific hotkey with the generic pane hotkey:
change the useHotkeyDisplay call that sets closePaneShortcut from
useHotkeyDisplay("CLOSE_TERMINAL") to useHotkeyDisplay("CLOSE_PANE") so the
closePaneShortcut variable uses the registry's "Close Pane" binding; update any
references to closePaneShortcut in the same hook/component to ensure they now
reflect the generic CLOSE_PANE hotkey.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx:
- Around line 89-93: The memoized terminal is only keyed by terminalId so it
never re-computes when connectionState changes; update the useMemo dependency
array to include connectionState (i.e., useMemo(() =>
terminalRuntimeRegistry.getTerminal(terminalId), [terminalId, connectionState]))
so terminal is re-derived after attach, and remove or update the biome-ignore
comment accordingly; reference useMemo, terminalRuntimeRegistry.getTerminal,
terminalId, and connectionState to locate the code.
In `@packages/panes/src/core/store/store.ts`:
- Around line 32-47: buildBalancedTree constructs the binary split shape but
doesn't normalize splitPercentage so three panes become 25/25/50; after creating
the split tree call equalizeAllSplits(node) before returning to distribute
splitPercentage by leaf counts. Update buildBalancedTree to compute the tree as
it currently does and then run equalizeAllSplits on the resulting LayoutNode
(and make the same change in the analogous function at the other location
referenced by lines 55-69) so new multi-pane tabs get evenly sized
splitPercentage values.
- Around line 738-758: The active-pane fallback uses
findFirstPaneId(nextSourceLayout) which picks the leftmost pane instead of the
sibling that was promoted when removePaneFromLayout removed the moved pane;
update removePaneFromLayout (or its caller) to return the promotedPaneId (or
expose it via nextSourcePanes result) and change the activePaneId fallback in
the block that builds nextTabs (the branch handling t.id === sourceTab.id where
it currently does activePaneId: t.activePaneId === args.paneId ?
findFirstPaneId(nextSourceLayout) : t.activePaneId) to use the promotedPaneId
when available, falling back to findFirstPaneId only if no promotedPaneId is
provided; apply the same change to the analogous code around the other
occurrence (the similar activePaneId logic mentioned for the 792-803 region).
In `@packages/panes/src/react/components/Workspace/components/Tab/Tab.tsx`:
- Around line 52-53: The mounted panel sizes aren’t updated when
node.splitPercentage changes because defaultSize is only used on mount; add a
useEffect that watches node.splitPercentage, recomputes firstSize =
node.splitPercentage ?? 50 and secondSize = 100 - firstSize, and calls
groupRef.current?.setLayout([firstSize, secondSize]) to sync the live layout;
also check the double-click handler that calls
groupRef.current?.setLayout([50,50]) (and the equalizeTab() flow) and if that
setLayout does not trigger the onLayout callback, follow it with a call to
resizeSplit() (or otherwise invoke the same onLayout handler) so the equalized
sizes persist to the store.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx:
- Around line 34-36: Replace the deprecated navigator.platform usage in the
MOD_KEY constant with a safe detection that prefers
navigator.userAgentData?.platform and falls back to navigator.platform (or a
shared utility if one exists), e.g., read platform =
navigator.userAgentData?.platform ?? navigator.platform, normalize with
.toLowerCase(), then check includes("mac") to choose "⌘" vs "Ctrl+"; update the
MOD_KEY definition in usePaneRegistry.tsx accordingly and reuse any existing
isMac/isPlatform utility if present.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts:
- Around line 235-239: Add a focused regression test that exercises the tab-wide
equalization hotkey behavior: simulate invoking the useHotkey handler for
"EQUALIZE_PANE_SPLITS" (or invoke the exported function that triggers it) and
assert two scenarios—(1) when the active tab contains nested splits, equalizeTab
is called with the active tab id and panes are redistributed as expected, and
(2) when the active tab has a single pane, equalizeTab is a no-op (no state
change). Locate the hotkey wiring in useWorkspaceHotkeys (useHotkey call) and
the equalizeTab action on the store (store.getState().equalizeTab) to
invoke/mock the store and active tab (getActiveTab) accordingly; use mocks/stubs
for store.getState and equalizeTab to assert the call and state-change/no-op
behavior.
In `@packages/panes/src/core/store/store.test.ts`:
- Around line 474-549: Add unit tests in store.test.ts for the two new mutations
movePaneToTab and movePaneToNewTab: create scenarios that mirror the existing
movePaneToSplit tests (within-tab, across-tab, and no-op on self) and include at
least one nested-layout case to verify the source tab collapses correctly and
focus/activeTabId is updated as expected; specifically exercise the
store.getState().movePaneToTab and store.getState().movePaneToNewTab calls, then
assert tabs[].panes, tabs[].layout, activePaneId, and
store.getState().activeTabId reflect the intended post-move state.
In
`@packages/panes/src/react/components/Workspace/components/Tab/components/Pane/components/PaneContextMenu/PaneContextMenu.tsx`:
- Around line 13-16: Replace the fragile deep-relative import of
ContextMenuActionConfig and RendererContext in PaneContextMenu.tsx with a
package-level import by re-exporting those types from the `@superset/panes`
package (or the package's top-level types entry) and then importing them like
import type { ContextMenuActionConfig, RendererContext } from "@superset/panes";
— update the import statement to reference the package-level module and ensure
the package index exports those types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 142e5eae-14a5-44cd-af15-db444f1bd8bd
📒 Files selected for processing (21)
apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useDefaultContextMenuActions/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useDefaultContextMenuActions/useDefaultContextMenuActions.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxpackages/panes/src/core/store/store.test.tspackages/panes/src/core/store/store.tspackages/panes/src/core/store/utils/index.tspackages/panes/src/core/store/utils/utils.test.tspackages/panes/src/core/store/utils/utils.tspackages/panes/src/index.tspackages/panes/src/react/components/Workspace/Workspace.tsxpackages/panes/src/react/components/Workspace/components/Tab/Tab.tsxpackages/panes/src/react/components/Workspace/components/Tab/components/Pane/Pane.tsxpackages/panes/src/react/components/Workspace/components/Tab/components/Pane/components/PaneContextMenu/PaneContextMenu.tsxpackages/panes/src/react/components/Workspace/components/Tab/components/Pane/components/PaneContextMenu/index.tspackages/panes/src/react/index.tspackages/panes/src/react/types.tspackages/panes/src/types.ts
| const equalizePaneSplitsShortcut = useHotkeyDisplay( | ||
| "EQUALIZE_PANE_SPLITS", | ||
| ).text; | ||
| const closePaneShortcut = useHotkeyDisplay("CLOSE_TERMINAL").text; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's a separate CLOSE_PANE hotkey defined
rg -n "CLOSE_PANE|CLOSE_TERMINAL" --type=ts -C2 apps/desktop/src/renderer/hotkeys/Repository: superset-sh/superset
Length of output: 874
🏁 Script executed:
sed -n '25,35p' apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/\$workspaceId/hooks/useDefaultContextMenuActions/useDefaultContextMenuActions.tsxRepository: superset-sh/superset
Length of output: 437
Use CLOSE_PANE hotkey instead of CLOSE_TERMINAL for the generic close pane action.
Line 29 uses useHotkeyDisplay("CLOSE_TERMINAL") for closePaneShortcut, but a dedicated CLOSE_PANE hotkey is defined in the registry (with label "Close Pane"). The "Close Pane" action is generic and applies to all pane types, not just terminals, so it should reference CLOSE_PANE to match the semantic intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useDefaultContextMenuActions/useDefaultContextMenuActions.tsx
at line 29, Replace the terminal-specific hotkey with the generic pane hotkey:
change the useHotkeyDisplay call that sets closePaneShortcut from
useHotkeyDisplay("CLOSE_TERMINAL") to useHotkeyDisplay("CLOSE_PANE") so the
closePaneShortcut variable uses the registry's "Close Pane" binding; update any
references to closePaneShortcut in the same hook/component to ensure they now
reflect the generic CLOSE_PANE hotkey.
| function buildBalancedTree( | ||
| panes: LayoutNode[], | ||
| direction: "horizontal" | "vertical" = "vertical", | ||
| ): LayoutNode { | ||
| if (panes.length === 1) return panes[0]!; | ||
|
|
||
| const mid = Math.ceil(panes.length / 2); | ||
| const nextDirection = direction === "vertical" ? "horizontal" : "vertical"; | ||
|
|
||
| return { | ||
| type: "split", | ||
| direction, | ||
| first: buildBalancedTree(panes.slice(0, mid), nextDirection), | ||
| second: buildBalancedTree(panes.slice(mid), nextDirection), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Equalize multi-pane tabs when constructing them.
buildBalancedTree() only gives you the binary shape. Because the renderer falls back missing splitPercentage values to 50/50 at every branch, addTab({ panes: [a, b, c] }) now comes out as 25/25/50 instead of evenly sized panes. Running equalizeAllSplits() here keeps new multi-pane tabs consistent with the leaf-count-based layout model used everywhere else in this PR.
♻️ Proposed fix
- layout: buildBalancedTree(leaves),
+ layout: equalizeAllSplits(buildBalancedTree(leaves)),Also applies to: 55-69
🤖 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 32 - 47,
buildBalancedTree constructs the binary split shape but doesn't normalize
splitPercentage so three panes become 25/25/50; after creating the split tree
call equalizeAllSplits(node) before returning to distribute splitPercentage by
leaf counts. Update buildBalancedTree to compute the tree as it currently does
and then run equalizeAllSplits on the resulting LayoutNode (and make the same
change in the analogous function at the other location referenced by lines
55-69) so new multi-pane tabs get evenly sized splitPercentage values.
| const nextTabs = s.tabs | ||
| .map((t) => { | ||
| if (t.id === sourceTab.id) { | ||
| if (!nextSourceLayout) return null; | ||
| return { | ||
| ...t, | ||
| layout: nextSourceLayout, | ||
| panes: nextSourcePanes, | ||
| activePaneId: | ||
| t.activePaneId === args.paneId | ||
| ? findFirstPaneId(nextSourceLayout) | ||
| : t.activePaneId, | ||
| }; | ||
| } | ||
| if (t.id === targetTab.id) { | ||
| return { | ||
| ...t, | ||
| layout: nextTargetLayout, | ||
| panes: { ...t.panes, [pane.id]: pane }, | ||
| activePaneId: pane.id, | ||
| }; |
There was a problem hiding this comment.
Keep source-tab focus on the promoted sibling after a move.
If the source layout is [p1, [p2, p3]] and active p3 is moved away, removePaneFromLayout() promotes p2, but findFirstPaneId(nextSourceLayout) still picks p1. That makes focus jump to the leftmost pane instead of the pane that inherited the moved pane’s slot. Please carry the promoted pane id out of the removal step and use that fallback here.
Also applies to: 792-803
🤖 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 738 - 758, The
active-pane fallback uses findFirstPaneId(nextSourceLayout) which picks the
leftmost pane instead of the sibling that was promoted when removePaneFromLayout
removed the moved pane; update removePaneFromLayout (or its caller) to return
the promotedPaneId (or expose it via nextSourcePanes result) and change the
activePaneId fallback in the block that builds nextTabs (the branch handling
t.id === sourceTab.id where it currently does activePaneId: t.activePaneId ===
args.paneId ? findFirstPaneId(nextSourceLayout) : t.activePaneId) to use the
promotedPaneId when available, falling back to findFirstPaneId only if no
promotedPaneId is provided; apply the same change to the analogous code around
the other occurrence (the similar activePaneId logic mentioned for the 792-803
region).
| const firstSize = node.splitPercentage ?? 50; | ||
| const secondSize = 100 - firstSize; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In react-resizable-panels (the library behind @superset/ui/resizable), does changing a Panel's defaultSizeafter mount update the live layout, and doesPanelGroup.setLayout()fireonLayout so external state stays in sync?
💡 Result:
No, changing a Panel's defaultSize after mount does not update the live layout. The defaultSize prop is only used for the initial size on first render/mount; subsequent changes to it are ignored. This is confirmed in GitHub issue #125 where the maintainer states: "The defaultSize param is only used once– the first time the Panel renders, to set the initial size. After that, any changes to that property will not affect anything." Yes, PanelGroup.setLayout (via groupRef) updates the live layout and fires onLayoutChanged (the recommended callback for layout changes, including for external state sync like persistence), keeping external state in sync if using that prop. The changelog confirms setLayout persists to the in-memory cache (4.5.9), and docs position onLayoutChanged for post-change notifications suitable for saving state. onLayoutChange (continuous during drag) may or may not fire depending on implementation, but onLayoutChanged is the one for final sync.
Citations:
- 1: Conditional Panel defaultSize based on window size bvaughn/react-resizable-panels#125
- 2: Direct control of sizes bvaughn/react-resizable-panels#58
- 3: https://www.npmjs.com/package/react-resizable-panels/v/2.0.8
- 4: https://github.com/bvaughn/react-resizable-panels/blob/main/CHANGELOG.md
- 5: https://cdn.jsdelivr.net/npm/react-resizable-panels@4.7.6/README.md
🏁 Script executed:
cd /dev/null && find . -type f -name "Tab.tsx" | grep -i workspace | head -5Repository: superset-sh/superset
Length of output: 113
🏁 Script executed:
rg -l "SplitView|groupRef" --type tsx | head -10Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
find . -path "*/Workspace/components/Tab/Tab.tsx" -o -path "*/panes/src/react/components/Workspace/components/Tab/Tab.tsx"Repository: superset-sh/superset
Length of output: 134
🏁 Script executed:
find . -name "Tab.tsx" | xargs wc -l 2>/dev/null | sort -rn | headRepository: superset-sh/superset
Length of output: 138
🏁 Script executed:
rg "packages/panes" --files --type tsx | head -20Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
fd "Tab.tsx" --type fRepository: superset-sh/superset
Length of output: 766
🏁 Script executed:
fd "packages/panes" --type dRepository: superset-sh/superset
Length of output: 425
🏁 Script executed:
find . -type f -path "*packages/panes/src/react/components/Workspace/components/Tab*"Repository: superset-sh/superset
Length of output: 2374
🏁 Script executed:
git ls-files | grep -i "tab.tsx" | grep -i workspaceRepository: superset-sh/superset
Length of output: 429
🏁 Script executed:
git ls-files | grep "panes/src"Repository: superset-sh/superset
Length of output: 2970
🏁 Script executed:
cat -n ./packages/panes/src/react/components/Workspace/components/Tab/Tab.tsx | head -100Repository: superset-sh/superset
Length of output: 3238
🏁 Script executed:
rg "ResizablePanelGroup" --type tsx -A 5 | grep -E "(onLayout|onLayoutChanged)" | head -10Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
find . -path "*superset/ui/resizable*" -o -path "*@superset/ui*resizable*" 2>/dev/null | head -20Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
git ls-files | grep -i resizable | head -20Repository: superset-sh/superset
Length of output: 265
🏁 Script executed:
cat -n packages/ui/src/components/ui/resizable.tsx | head -50Repository: superset-sh/superset
Length of output: 2261
Sync store-driven split percentage changes to the panel group layout.
The defaultSize prop is only evaluated on initial render. When node.splitPercentage updates in the store (e.g., from equalizeTab()), the mounted panels will not reflect the new values because defaultSize changes after mount are ignored. Add a useEffect to sync updates: when node.splitPercentage changes, call groupRef.current?.setLayout([firstSize, secondSize]) to apply the new layout to the live component. Also verify that the double-click handler calling setLayout([50, 50]) on line 70 correctly triggers the onLayout callback so the equalization persists to the store; if the layout change does not trigger onLayout, explicitly call resizeSplit() after setLayout().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/panes/src/react/components/Workspace/components/Tab/Tab.tsx` around
lines 52 - 53, The mounted panel sizes aren’t updated when node.splitPercentage
changes because defaultSize is only used on mount; add a useEffect that watches
node.splitPercentage, recomputes firstSize = node.splitPercentage ?? 50 and
secondSize = 100 - firstSize, and calls groupRef.current?.setLayout([firstSize,
secondSize]) to sync the live layout; also check the double-click handler that
calls groupRef.current?.setLayout([50,50]) (and the equalizeTab() flow) and if
that setLayout does not trigger the onLayout callback, follow it with a call to
resizeSplit() (or otherwise invoke the same onLayout handler) so the equalized
sizes persist to the store.
Greptile SummaryThis PR rewrites the pane layout model from N-ary flat splits to a strict binary tree and adds a comprehensive context menu system. The binary tree utilities are well-implemented and tested. Two P1 bugs need fixing: terminal useMemo returns null permanently, and equalizeTab doesn't visually move panels. Confidence Score: 3/5Not safe to merge — two P1 bugs break features explicitly in the test plan (ScrollToBottomButton and equalize). The binary tree layout model, context menu architecture, and store actions are well-designed and thoroughly tested. However two P1 bugs break primary user paths: terminal useMemo always returns null so ScrollToBottomButton never shows, and equalizeTab has no visual effect because defaultSize is uncontrolled. Once fixed the PR should reach 4-5. TerminalPane.tsx (terminal useMemo missing connectionState dep) and Tab.tsx (defaultSize uncontrolled so equalizeTab has no visual effect) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Right-click Pane] --> B[PaneContextMenu]
B --> C[resolve actions]
C --> D[workspace contextMenuActions]
D -->|defaults| E[PaneDefinition.contextMenuActions]
E -->|final list| F[ContextMenuItems]
F --> G{action type?}
G -->|separator| H[ContextMenuSeparator]
G -->|has children| I[ContextMenuSub recursive]
G -->|item| J[ContextMenuItem + shortcut]
K[equalizeTab] --> L[splitPercentage updated in store]
L --> M[SplitView re-renders with new defaultSize]
M --> N[react-resizable-panels ignores defaultSize after mount]
N --> O[Panels stay in place - visual bug]
P[LayoutNode] --> Q{type}
Q -->|pane| R[Pane component]
Q -->|split| S[SplitView]
S --> T[ResizablePanel defaultSize=first%]
S --> U[ResizablePanel defaultSize=second%]
T --> V[onLayout fires resizeSplit to store]
W[closePane] --> X[removePaneFromLayout]
X --> Y{sibling?}
Y -->|yes| Z[promote sibling]
Y -->|no| AA[remove tab]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| </> | ||
| ); | ||
| })} | ||
| <ResizablePanel defaultSize={firstSize}> |
There was a problem hiding this comment.
ResizablePanel uses the uncontrolled defaultSize prop, which react-resizable-panels only applies on initial mount. When equalizeTab updates splitPercentage in the store, re-renders pass new defaultSize values, but mounted panels ignore them and stay put. The test plan item “Equalize pane splits — all panes get equal visual space” will fail. The existing double-click handler uses groupRef.current?.setLayout([50, 50]) specifically because defaultSize cannot move an already-mounted panel; the same imperative approach is needed after equalizeTab.
| return ( | ||
| <div className="flex h-full w-full flex-col p-2"> | ||
| <div | ||
| ref={containerRef} | ||
| className="min-h-0 flex-1 overflow-hidden" | ||
| style={{ backgroundColor: appearance.background }} | ||
| /> | ||
| <div className="relative min-h-0 flex-1 overflow-hidden"> | ||
| <div | ||
| ref={containerRef} |
There was a problem hiding this comment.
terminal is permanently null — ScrollToBottomButton never works
The useMemo only lists [terminalId] as dependencies. On the first render the terminal isn’t yet in the registry (attachment happens inside a useEffect that runs after render), so getTerminal(terminalId) returns null and that result is cached. When connectionState later changes and triggers a re-render, useMemo finds terminalId unchanged and returns the cached null — it does not re-run the factory.
The biome-ignore comment claims “connectionState triggers re-derive after attach”, but this is incorrect: useMemo only re-runs when a listed dependency changes, not on every re-render.
| return ( | |
| <div className="flex h-full w-full flex-col p-2"> | |
| <div | |
| ref={containerRef} | |
| className="min-h-0 flex-1 overflow-hidden" | |
| style={{ backgroundColor: appearance.background }} | |
| /> | |
| <div className="relative min-h-0 flex-1 overflow-hidden"> | |
| <div | |
| ref={containerRef} | |
| const terminal = useMemo( | |
| () => terminalRuntimeRegistry.getTerminal(terminalId), | |
| [terminalId, connectionState], | |
| ); |
| kind: "chat", | ||
| data: { sessionId: null } as ChatPaneData, | ||
| }); | ||
| }, |
There was a problem hiding this comment.
CLOSE_TERMINAL shortcut displayed for all pane types
closePaneShortcut is derived from useHotkeyDisplay("CLOSE_TERMINAL") and used for the generic “Close Pane” action shown in every pane’s context menu. If CLOSE_TERMINAL is terminal-specific, right-clicking a browser or chat pane will show a misleading shortcut hint. Consider a generic CLOSE_PANE hotkey ID or omit the shortcut from the default and let each pane-kind override add the correct binding.
| }, | |
| const closePaneShortcut = useHotkeyDisplay("CLOSE_PANE").text; |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/panes/src/core/store/store.ts (1)
733-739: Consider equalizing the target tab after inserting the moved pane.The new split wrapper is created without a
splitPercentage, defaulting to 50/50. If the target tab has multiple existing panes, the moved pane will take 50% of the entire tab space regardless of existing pane count. This may not match user expectations for "Move to Tab."Consider calling
equalizeAllSplits(nextTargetLayout)to distribute space proportionally by leaf count, consistent with the equalization model elsewhere.♻️ Proposed fix
const paneLeaf: LayoutNode = { type: "pane", paneId: pane.id }; - const nextTargetLayout: LayoutNode = { + const nextTargetLayout: LayoutNode = equalizeAllSplits({ type: "split", direction: "horizontal", first: targetTab.layout, second: paneLeaf, - }; + });🤖 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 733 - 739, The split wrapper for moving a pane is created as nextTargetLayout (with paneLeaf referencing pane.id and using targetTab.layout as first/second) but no splitPercentage is set, so the moved pane always gets a 50/50 size; call equalizeAllSplits(nextTargetLayout) after constructing nextTargetLayout and before applying it to the store so the split percentages are distributed by leaf count consistent with the equalization model used elsewhere.
🤖 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.ts`:
- Around line 733-739: The split wrapper for moving a pane is created as
nextTargetLayout (with paneLeaf referencing pane.id and using targetTab.layout
as first/second) but no splitPercentage is set, so the moved pane always gets a
50/50 size; call equalizeAllSplits(nextTargetLayout) after constructing
nextTargetLayout and before applying it to the store so the split percentages
are distributed by leaf count consistent with the equalization model used
elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4493b213-b419-4b6d-825e-16e80dbb0d5d
📒 Files selected for processing (3)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsxpackages/panes/src/core/store/store.tspackages/panes/src/core/store/utils/utils.ts
) * feat(desktop): add pane context menus + rewrite layout to binary tree - Add data-driven context menu system to @superset/panes with ContextMenuActionConfig type and recursive rendering - Wire up context menus for all v2 pane types (terminal, chat, browser, file) with pane-specific actions (copy/paste/clear/scroll for terminal) - Add terminal hotkeys (CLEAR_TERMINAL, SCROLL_TO_BOTTOM) and scroll-to-bottom button to v2 terminal pane - Add movePaneToTab/movePaneToNewTab store actions for Move to Tab submenu - Rewrite layout model from N-ary splits (children[]/weights[]) to strict binary tree (first/second/splitPercentage) matching react-mosaic's model - Replace split node IDs with path-based addressing (SplitPath) - Equalize now uses leaf-count-weighted splitPercentage recursively - Pane removal uses sibling promotion (only neighbor affected) * fix: resolve lint warnings (non-null assertions, unused suppression)
cherry-pick済み: - e728ebd feat(desktop): wire up missing hotkeys for v2 workspace (superset-sh#3190) - 1eddeb3 feat(desktop): git changes sidebar with resource-oriented API (superset-sh#3177) - 11ed4f8 V2 terminal env (superset-sh#3184) - 0c52ecc feat(desktop): pane context menus + binary tree layout (superset-sh#3196) - 5578746 fix(desktop): resolve file icons from origin instead of href (superset-sh#3199) - 5a1e5d1 feat(panes): prefer sibling pane when closing active pane (superset-sh#3198) - d670c4a V2 top bar: right sidebar toggle, org dropdown in sidebar, unified open-in button (superset-sh#3197) - 2573fa2 fix(desktop): remove macOS background-to-tray quit interception (superset-sh#3205) - 4a29342 feat: Superset CLI + CLI framework + Better Auth 1.5.6 (superset-sh#3194) - 700cd65 fix(desktop): revert broken file icon origin fix + bundle all icon sources (superset-sh#3218) フォーク独自対応: - cleanupMainWindowResources()をexit pathに移動維持 (#3205対応) - BROWSER_HARD_RELOAD/SEARCH_IN_FILESをv2 workspaceに配線 - BROWSER_RELOAD/HARD_RELOADのuseHotkey配線修正（リマップ対応） - ansi_up依存維持
1 participant
Summary
@superset/panes— right-click any pane for split/move/close actions, terminal panes get copy/paste/clear/scrollchildren[]/weights[]) to strict binary tree (first/second/splitPercentage) matching react-mosaic's proven modelmovePaneToTab/movePaneToNewTabstore actions for "Move to Tab" submenuCLEAR_TERMINAL,SCROLL_TO_BOTTOM) and scroll-to-bottom button to v2 terminal paneKey changes
Context menus
ContextMenuActionConfig<TData>type with recursive children support for submenuscontextMenuActionsprop on<Workspace>PaneDefinition.contextMenuActions(same pattern aspaneActionsfor header buttons)Binary tree layout (matching react-mosaic)
LayoutNodesplit variant:first/second/splitPercentage?instead ofid/children[]/weights[]SplitPath = ("first" | "second")[]) replaces split node IDssplitPercentagerecursively (all panes get equal visual space)Test plan
Summary by cubic
Adds data-driven pane context menus and rewrites
@superset/paneslayout to a react-mosaic–style binary tree with path-based resizing and equalize. Improves terminal UX with copy/paste/clear/scroll actions, hotkeys, and a scroll-to-bottom button.New Features
contextMenuActionsand per‑panePaneDefinition.contextMenuActionsto customize menus; newPaneContextMenucomponent renders items, submenus, separators, and shortcuts viaContextMenuActionConfig.movePaneToTabandmovePaneToNewTabstore actions.Migration
children[]/weights[]/idtofirst/second/splitPercentage?; split IDs removed in favor ofSplitPath.resizeSplit({ tabId, path, splitPercentage })replacesresizeSplit({ splitId, weights }).equalizeSplit({ tabId, path })and newequalizeTab({ tabId })replace split‑ID–based equalize.movePaneToTab({ paneId, targetTabId }),movePaneToNewTab({ paneId }).updateSplitInLayout→updateAtPath; addedequalizeAllSplits,getNodeAtPath,positionToDirection.Written for commit 6a5c725. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements