new replace, split-horizontal, and split-vertical layout actions... hook up cmd-d and cmd-shift-d... #1931
Conversation
…ook up cmd-d and cmd-shift-d...
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request implements enhancements to block and layout management across multiple areas of the codebase. In the command-line interface, a new string variable and flag are added to allow a replacement block to be specified when opening a web widget. The front-end now includes asynchronous functions for creating new blocks by splitting existing ones both horizontally and vertically, alongside new key bindings to manage terminal blocks. The layout model and associated tree logic are updated with additional action types—replace, split horizontal, and split vertical—and corresponding methods to manipulate the layout tree. Data structures and type definitions are modified to include optional fields for target block identifiers and positioning details. On the server side, command logic for block creation is refactored to handle various target actions such as replacing a block or splitting it in specified directions, with enhanced error handling for invalid inputs. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
frontend/app/store/global.ts (2)
383-405: Consider adding error handling for the block creation call.
While the function properly throws if targetNodeId is missing, you may want to handle potential errors from ObjectService.CreateBlock more gracefully (e.g., providing a user-friendly error message or a fallback action).
407-429: Ensure consistent code usage between horizontal and vertical splits.
Both functions look structurally similar. Consider extracting common logic (like validating targetBlockId and calling ObjectService.CreateBlock) into a helper method for dryness.frontend/app/store/keymodel.ts (1)
229-253: Refactor to reduce code duplication.The function is nearly identical to
handleSplitHorizontal. Consider extracting the common logic into a shared helper function.+async function createTerminalBlockDef(): Promise<BlockDef> { + const termBlockDef: BlockDef = { + meta: { + view: "term", + controller: "shell", + }, + }; + const layoutModel = getLayoutModelForStaticTab(); + const focusedNode = globalStore.get(layoutModel.focusedNode); + if (focusedNode != null) { + const blockAtom = WOS.getWaveObjectAtom<Block>(WOS.makeORef("block", focusedNode.data?.blockId)); + const blockData = globalStore.get(blockAtom); + if (blockData?.meta?.view == "term") { + if (blockData?.meta?.["cmd:cwd"] != null) { + termBlockDef.meta["cmd:cwd"] = blockData.meta["cmd:cwd"]; + } + } + if (blockData?.meta?.connection != null) { + termBlockDef.meta.connection = blockData.meta.connection; + } + } + return termBlockDef; +} -async function handleSplitHorizontal() { - // split horizontally - const termBlockDef: BlockDef = { - meta: { - view: "term", - controller: "shell", - }, - }; - const layoutModel = getLayoutModelForStaticTab(); - const focusedNode = globalStore.get(layoutModel.focusedNode); - if (focusedNode == null) { - return; - } - const blockAtom = WOS.getWaveObjectAtom<Block>(WOS.makeORef("block", focusedNode.data?.blockId)); - const blockData = globalStore.get(blockAtom); - if (blockData?.meta?.view == "term") { - if (blockData?.meta?.["cmd:cwd"] != null) { - termBlockDef.meta["cmd:cwd"] = blockData.meta["cmd:cwd"]; - } - } - if (blockData?.meta?.connection != null) { - termBlockDef.meta.connection = blockData.meta.connection; - } - await createBlockSplitHorizontally(termBlockDef, focusedNode.data.blockId, "after"); +async function handleSplitHorizontal() { + const layoutModel = getLayoutModelForStaticTab(); + const focusedNode = globalStore.get(layoutModel.focusedNode); + if (focusedNode == null) { + return; + } + const termBlockDef = await createTerminalBlockDef(); + await createBlockSplitHorizontally(termBlockDef, focusedNode.data.blockId, "after"); } -async function handleSplitVertical() { - // split horizontally - const termBlockDef: BlockDef = { - meta: { - view: "term", - controller: "shell", - }, - }; - const layoutModel = getLayoutModelForStaticTab(); - const focusedNode = globalStore.get(layoutModel.focusedNode); - if (focusedNode == null) { - return; - } - const blockAtom = WOS.getWaveObjectAtom<Block>(WOS.makeORef("block", focusedNode.data?.blockId)); - const blockData = globalStore.get(blockAtom); - if (blockData?.meta?.view == "term") { - if (blockData?.meta?.["cmd:cwd"] != null) { - termBlockDef.meta["cmd:cwd"] = blockData.meta["cmd:cwd"]; - } - } - if (blockData?.meta?.connection != null) { - termBlockDef.meta.connection = blockData.meta.connection; - } - await createBlockSplitVertically(termBlockDef, focusedNode.data.blockId, "after"); +async function handleSplitVertical() { + const layoutModel = getLayoutModelForStaticTab(); + const focusedNode = globalStore.get(layoutModel.focusedNode); + if (focusedNode == null) { + return; + } + const termBlockDef = await createTerminalBlockDef(); + await createBlockSplitVertically(termBlockDef, focusedNode.data.blockId, "after"); }frontend/layout/lib/layoutTree.ts (2)
458-501: Consider preserving relative sizes during split.Currently, the function rebalances sizes equally after splitting. Consider preserving the relative sizes of existing nodes and only adjusting the new node's size.
- parent.children.forEach((child) => (child.size = 1)); + const existingTotalSize = parent.children.reduce((sum, child) => sum + child.size, 0); + const newSize = 1; + const scaleFactor = (existingTotalSize - newSize) / existingTotalSize; + parent.children.forEach((child, index) => { + if (index === insertIndex) { + child.size = newSize; + } else { + child.size *= scaleFactor; + } + });
505-545: Refactor to reduce code duplication.The function is nearly identical to
splitHorizontal. Consider extracting the common logic into a shared helper function that takes the flex direction as a parameter.+function splitNode( + layoutState: LayoutTreeState, + targetNodeId: string, + newNode: LayoutNode, + position: string, + flexDirection: FlexDirection, + focused?: boolean +) { + const targetNode = findNode(layoutState.rootNode, targetNodeId); + if (!targetNode) { + console.error(`split${flexDirection}: Target node not found`, targetNodeId); + return; + } + + const parent = findParent(layoutState.rootNode, targetNodeId); + if (parent && parent.flexDirection === flexDirection) { + const index = parent.children.findIndex((child) => child.id === targetNodeId); + if (index === -1) { + console.error(`split${flexDirection}: Target node not found in parent's children`, targetNodeId); + return; + } + const insertIndex = position === "before" ? index : index + 1; + parent.children.splice(insertIndex, 0, newNode); + const existingTotalSize = parent.children.reduce((sum, child) => sum + child.size, 0); + const newSize = 1; + const scaleFactor = (existingTotalSize - newSize) / existingTotalSize; + parent.children.forEach((child, idx) => { + if (idx === insertIndex) { + child.size = newSize; + } else { + child.size *= scaleFactor; + } + }); + } else { + const groupNode = newLayoutNode(flexDirection, targetNode.size, [targetNode], undefined); + groupNode.children = position === "before" ? [newNode, targetNode] : [targetNode, newNode]; + groupNode.children.forEach((child) => (child.size = 1)); + if (parent) { + const index = parent.children.findIndex((child) => child.id === targetNodeId); + if (index === -1) { + console.error(`split${flexDirection} (wrap): Target node not found in parent's children`, targetNodeId); + return; + } + parent.children[index] = groupNode; + } else { + layoutState.rootNode = groupNode; + } + } + if (focused) { + layoutState.focusedNodeId = newNode.id; + } + layoutState.generation++; +} -export function splitHorizontal(layoutState: LayoutTreeState, action: LayoutTreeSplitHorizontalAction) { - // ... existing implementation ... +export function splitHorizontal(layoutState: LayoutTreeState, action: LayoutTreeSplitHorizontalAction) { + const { targetNodeId, newNode, position, focused } = action; + splitNode(layoutState, targetNodeId, newNode, position, FlexDirection.Row, focused); } -export function splitVertical(layoutState: LayoutTreeState, action: LayoutTreeSplitVerticalAction) { - // ... existing implementation ... +export function splitVertical(layoutState: LayoutTreeState, action: LayoutTreeSplitVerticalAction) { + const { targetNodeId, newNode, position, focused } = action; + splitNode(layoutState, targetNodeId, newNode, position, FlexDirection.Column, focused); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmd/wsh/cmd/wshcmd-web.go(3 hunks)frontend/app/store/global.ts(3 hunks)frontend/app/store/keymodel.ts(3 hunks)frontend/layout/lib/layoutModel.ts(4 hunks)frontend/layout/lib/layoutTree.ts(2 hunks)frontend/layout/lib/types.ts(2 hunks)frontend/types/gotypes.d.ts(2 hunks)pkg/waveobj/wtype.go(1 hunks)pkg/wcore/layout.go(1 hunks)pkg/wshrpc/wshrpctypes.go(2 hunks)pkg/wshrpc/wshserver/wshserver.go(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/layout/lib/layoutModel.ts
[error] 508-534: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 535-561: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
🔇 Additional comments (20)
frontend/app/store/global.ts (2)
13-13: Import statements look good.
These imports cleanly reference the needed layout action types from "@/layout/lib/types".
734-735: Export lines appear correct.
Exporting these new split functions makes them easily accessible for layout logic throughout the application.pkg/wshrpc/wshserver/wshserver.go (1)
211-264: Verify correctness of optional target actions.
The overall structure for handling "replace", "splitright", "splitleft", "splitup", and "splitdown" is clear, and falling back to an insert action when TargetBlockId is empty is sensible. However, consider these points:
• For replace: calling DeleteBlock with the second parameter as false intentionally avoids recursively deleting sub-blocks. Confirm that’s desired in your workflow.
• For split actions: ensure any ephemeral or magnified states are handled (if relevant).
• For insert action: the code properly handles Magnified and Ephemeral fields.Everything else looks consistent with the existing code conventions.
frontend/layout/lib/layoutModel.ts (3)
20-23: Imports for replaceNode, splitHorizontal, and splitVertical
Importing these new functions expands the layout model’s actions effectively. Keep verifying that naming and usage remain consistent across your codebase.
388-395: New case statements for ReplaceNode, SplitHorizontal, and SplitVertical.
The calls to the respective helper functions (replaceNode, splitHorizontal, splitVertical) are properly integrated into the reducer logic and each ends with break statements, preventing fallthrough.
490-508: ReplaceNode event handling logic.
Here, you correctly retrieve a target node by blockId, construct a replace action, and invoke treeReducer. The final break statement ensures no unintended fallthrough.cmd/wsh/cmd/wshcmd-web.go (4)
42-42: LGTM!The variable declaration follows Go naming conventions and is appropriately scoped.
46-46: LGTM!The flag is correctly defined with a clear description and appropriate short and long forms.
103-113: LGTM!The error handling is robust and the validation logic is correct:
- Properly resolves block ID using
resolveSimpleId.- Appropriate error message for invalid block ID.
- Prevents conflicting flag combinations (
--replaceand--magnified).
123-126: LGTM!The target block ID and action are correctly set in the command data when a replacement block is specified.
pkg/wcore/layout.go (1)
21-23: LGTM!The new layout action type constants are well-defined:
- Follow consistent naming pattern.
- Values are descriptive and match their purpose.
- Maintain backward compatibility with existing constants.
pkg/waveobj/wtype.go (1)
204-214: LGTM!The new fields in
LayoutActionDataare well-defined:
TargetBlockIdfor specifying the target block.Positionfor specifying the split position.- Both fields are optional with
omitemptyJSON tags.frontend/layout/lib/types.ts (2)
73-75: LGTM!The new action types in
LayoutTreeActionTypeare well-defined and follow the existing pattern.
193-219: LGTM!The new interfaces are well-structured and documented:
- Clear comments explaining each action's purpose.
- Appropriate field types and optional flags.
- Position field uses a union type for type safety.
frontend/app/store/keymodel.ts (1)
203-227: LGTM! Clean implementation of horizontal split.The function correctly inherits the working directory and connection from the focused block, maintaining context when splitting.
frontend/layout/lib/layoutTree.ts (1)
432-454: LGTM! Clean implementation of node replacement.The function correctly handles both root and non-root node replacement while preserving the node size.
pkg/wshrpc/wshrpctypes.go (2)
44-50: LGTM! Well-defined constants for block actions.The constants follow a consistent naming pattern and use descriptive values.
348-355: LGTM! Well-documented struct fields.The new fields are properly documented with clear comments indicating valid values for
TargetAction.frontend/types/gotypes.d.ts (2)
156-157: LGTM! Type definitions match Go struct.The optional fields correctly mirror the Go struct fields in
wshrpctypes.go.
485-486: LGTM! Type definitions support new layout actions.The optional fields properly support the new replace and split layout actions.
frontend/layout/lib/layoutModel.ts
Outdated
| case LayoutTreeActionType.SplitVertical: { | ||
| const targetNode = this?.getNodeByBlockId(action.targetblockid); | ||
| if (!targetNode) { | ||
| console.error( | ||
| "Cannot apply eventbus layout action SplitVertical, could not find target node with blockId", | ||
| action.targetblockid | ||
| ); | ||
| break; | ||
| } | ||
| if (action.position != "before" && action.position != "after") { | ||
| console.error( | ||
| "Cannot apply eventbus layout action SplitVertical, invalid position", | ||
| action.position | ||
| ); | ||
| break; | ||
| } | ||
| const newNode = newLayoutNode(undefined, action.nodesize, undefined, { | ||
| blockId: action.blockid, | ||
| }); | ||
| const splitAction: LayoutTreeSplitVerticalAction = { | ||
| type: LayoutTreeActionType.SplitVertical, | ||
| targetNodeId: targetNode.id, | ||
| newNode: newNode, | ||
| position: action.position, | ||
| }; | ||
| this.treeReducer(splitAction, false); | ||
| } |
There was a problem hiding this comment.
Avoid unintended fallthrough from SplitVertical to subsequent cases.
Similarly, add a “break;” statement inside this block to prevent any unexpected continuation.
Proposed fix:
this.treeReducer(splitAction, false);
+ break;
}
default:
console.warn("unsupported layout action", action);📝 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.
| case LayoutTreeActionType.SplitVertical: { | |
| const targetNode = this?.getNodeByBlockId(action.targetblockid); | |
| if (!targetNode) { | |
| console.error( | |
| "Cannot apply eventbus layout action SplitVertical, could not find target node with blockId", | |
| action.targetblockid | |
| ); | |
| break; | |
| } | |
| if (action.position != "before" && action.position != "after") { | |
| console.error( | |
| "Cannot apply eventbus layout action SplitVertical, invalid position", | |
| action.position | |
| ); | |
| break; | |
| } | |
| const newNode = newLayoutNode(undefined, action.nodesize, undefined, { | |
| blockId: action.blockid, | |
| }); | |
| const splitAction: LayoutTreeSplitVerticalAction = { | |
| type: LayoutTreeActionType.SplitVertical, | |
| targetNodeId: targetNode.id, | |
| newNode: newNode, | |
| position: action.position, | |
| }; | |
| this.treeReducer(splitAction, false); | |
| } | |
| case LayoutTreeActionType.SplitVertical: { | |
| const targetNode = this?.getNodeByBlockId(action.targetblockid); | |
| if (!targetNode) { | |
| console.error( | |
| "Cannot apply eventbus layout action SplitVertical, could not find target node with blockId", | |
| action.targetblockid | |
| ); | |
| break; | |
| } | |
| if (action.position != "before" && action.position != "after") { | |
| console.error( | |
| "Cannot apply eventbus layout action SplitVertical, invalid position", | |
| action.position | |
| ); | |
| break; | |
| } | |
| const newNode = newLayoutNode(undefined, action.nodesize, undefined, { | |
| blockId: action.blockid, | |
| }); | |
| const splitAction: LayoutTreeSplitVerticalAction = { | |
| type: LayoutTreeActionType.SplitVertical, | |
| targetNodeId: targetNode.id, | |
| newNode: newNode, | |
| position: action.position, | |
| }; | |
| this.treeReducer(splitAction, false); | |
| break; | |
| } | |
| default: | |
| console.warn("unsupported layout action", action); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 535-561: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
There was a problem hiding this comment.
@sawka seems like a good suggestion
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
frontend/layout/lib/layoutModel.ts
Outdated
| const targetNode = this?.getNodeByBlockId(action.targetblockid); | ||
| if (!targetNode) { | ||
| console.error( | ||
| "Cannot apply eventbus layout action SplitHorizontal, could not find target node with blockId", | ||
| action.targetblockid | ||
| ); | ||
| break; | ||
| } | ||
| if (action.position != "before" && action.position != "after") { | ||
| console.error( | ||
| "Cannot apply eventbus layout action SplitHorizontal, invalid position", | ||
| action.position | ||
| ); | ||
| break; | ||
| } | ||
| const newNode = newLayoutNode(undefined, action.nodesize, undefined, { | ||
| blockId: action.blockid, | ||
| }); | ||
| const splitAction: LayoutTreeSplitHorizontalAction = { | ||
| type: LayoutTreeActionType.SplitHorizontal, | ||
| targetNodeId: targetNode.id, | ||
| newNode: newNode, | ||
| position: action.position, | ||
| }; | ||
| this.treeReducer(splitAction, false); | ||
| } |
There was a problem hiding this comment.
Avoid unintended fallthrough from SplitHorizontal to the next case.
A break or return is missing at the end of this block. Add a “break;” statement inside the curly braces to prevent falling into the SplitVertical case.
Apply the following diff to insert the break:
this.treeReducer(splitAction, false);
+ break;
}
case LayoutTreeActionType.SplitVertical: {📝 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.
| const targetNode = this?.getNodeByBlockId(action.targetblockid); | |
| if (!targetNode) { | |
| console.error( | |
| "Cannot apply eventbus layout action SplitHorizontal, could not find target node with blockId", | |
| action.targetblockid | |
| ); | |
| break; | |
| } | |
| if (action.position != "before" && action.position != "after") { | |
| console.error( | |
| "Cannot apply eventbus layout action SplitHorizontal, invalid position", | |
| action.position | |
| ); | |
| break; | |
| } | |
| const newNode = newLayoutNode(undefined, action.nodesize, undefined, { | |
| blockId: action.blockid, | |
| }); | |
| const splitAction: LayoutTreeSplitHorizontalAction = { | |
| type: LayoutTreeActionType.SplitHorizontal, | |
| targetNodeId: targetNode.id, | |
| newNode: newNode, | |
| position: action.position, | |
| }; | |
| this.treeReducer(splitAction, false); | |
| } | |
| const targetNode = this?.getNodeByBlockId(action.targetblockid); | |
| if (!targetNode) { | |
| console.error( | |
| "Cannot apply eventbus layout action SplitHorizontal, could not find target node with blockId", | |
| action.targetblockid | |
| ); | |
| break; | |
| } | |
| if (action.position != "before" && action.position != "after") { | |
| console.error( | |
| "Cannot apply eventbus layout action SplitHorizontal, invalid position", | |
| action.position | |
| ); | |
| break; | |
| } | |
| const newNode = newLayoutNode(undefined, action.nodesize, undefined, { | |
| blockId: action.blockid, | |
| }); | |
| const splitAction: LayoutTreeSplitHorizontalAction = { | |
| type: LayoutTreeActionType.SplitHorizontal, | |
| targetNodeId: targetNode.id, | |
| newNode: newNode, | |
| position: action.position, | |
| }; | |
| this.treeReducer(splitAction, false); | |
| break; | |
| } | |
| case LayoutTreeActionType.SplitVertical: { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 508-534: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
There was a problem hiding this comment.
same here, was this lack of break intentional?
There was a problem hiding this comment.
@esimkowitz No, this wasn't intentional. The missing break statement is a bug that could cause unintended behavior by falling through to the SplitVertical case. The previous suggestion to add a break statement is correct and should be applied.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
No description provided.