terminal multi-input for tab #1643
Conversation
WalkthroughThe pull request updates the Wave Terminal application to implement multi-input functionality for terminal blocks. It includes modifications across several files, with the roadmap reflecting the completion of the "Multi-Input between terminal blocks on the same tab" feature. Key changes involve the addition of a global state atom Specifically, new methods facilitate data transmission to multiple terminal instances, a global key binding is introduced to toggle multi-input mode, and the terminal view is enhanced to support this functionality. The overall changes provide a structured approach for users to interact with multiple terminal blocks simultaneously, thereby increasing the application's flexibility and usability. 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: 0
🧹 Nitpick comments (3)
frontend/app/view/term/term.tsx (2)
190-201: Toggle multi-input feature
The UI logic for a "Multi Input ON" button is straightforward. For better UX, consider adding a quick visual indicator or icon to make the feature state even clearer.
682-695:getAllBasicTermModelsfunction
Solid approach to filter out non-basic terminals. If usage grows, consider caching results if performance is critical.frontend/app/block/blockframe.tsx (1)
293-293: Consider providing a fallback for thetitleprop.
Ifelem.titleis undefined or null, you might display a default value or omit the property entirely to avoid an empty tooltip in some browsers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ROADMAP.md(1 hunks)frontend/app/block/blockframe.tsx(1 hunks)frontend/app/store/global.ts(3 hunks)frontend/app/store/keymodel.ts(3 hunks)frontend/app/view/term/term.tsx(9 hunks)frontend/app/view/term/termwrap.ts(5 hunks)frontend/types/custom.d.ts(3 hunks)
🔇 Additional comments (24)
frontend/app/view/term/term.tsx (9)
15-15: Import reassurance
No issues: this import statement is consistent with the usage below for retrieving block models.
28-28: Confirm each utility usage
Both boundNumber and stringToBase64 appear properly utilized.
133-133: Initialize header elements array
Good approach: an empty array is a clean starting point for building dynamic UI.
318-329: isBasicTerm method
The method concisely checks if the terminal is in a simple (non-vdom, non-cmd) state. No issues identified.
330-330: Blank line
No specific comment.
331-341: multiInputHandler method
The logic for forwarding data to all other terminals is clear and correct. Optional: add debug logging in case multi-input data distribution fails or to track usage.
343-346: sendDataToController method
Straightforward data encoding and sending. Consider catching errors in case the RPC call fails for any reason.
845-847: Consistency with isBasicTerm
Defining isBasicTerm directly in the component is good, but ensure it stays consistent with the method in TermViewModel.
913-924: React.useEffect for multi-input callback
Clear logic: sets up or clears the callback depending on focus and multi-input state. No issues found.
frontend/app/view/term/termwrap.ts (6)
43-43: Added optional sendDataHandler
Allows external injection of data handling logic. This pattern increases flexibility.
59-60: New properties in TermWrap: multiInputCallback and sendDataHandler
Enhances extensibility for multi-input features. Implementation is straightforward.
70-70: Assigning sendDataHandler
Ensures we can delegate data sending externally; no concerns.
144-144: onKeyHandler registration
Positions key event handling in one place. Good approach to maintain clarity for multi-input usage.
175-177: handleTermData now calling this.sendDataHandler?.(data)
Provides a neat injection point to avoid hardcoded logic. Good usage of optional chaining.
178-181: onKeyHandler triggers multiInputCallback
This effectively routes keystroke data to multi-input. If keys become more complex, ensure key data is sanitized or validated.
frontend/types/custom.d.ts (3)
30-30: Declare isTermMultiInput in GlobalAtomsType
Accurately reflects new global state. Compatible with newly introduced multi-input features.
171-171: Added optional title in HeaderTextButton
Expands UI flexibility by allowing tooltips or extra info. No issues found.
245-247: Added isBasicTerm in ViewModel
Consistent with the terminal’s logic. Marking it optional is sensible for other view models.
frontend/app/store/keymodel.ts (3)
8-8: Import getAllBlockComponentModels
Import aligns with multi-input logic that needs to iterate over terminal blocks.
229-240: New countTermBlocks function
Effectively counts basic terminals. Quick check: ensure logic is consistent with isBasicTerm usage.
324-332: Key binding for Ctrl+Shift+i
Smart guard ensures multi-input toggles only if at least two basic terminal blocks exist. Clear user experience.
frontend/app/store/global.ts (2)
167-167: Added isTermMultiInput atom
Aligned with multi-input feature. Initializing to false is a sensible default.
500-502: New function getAllBlockComponentModels
Facilitates enumerating block models in the store. Implementation is straightforward.
ROADMAP.md (1)
27-27: Roadmap status update looks correct.
This line accurately reflects the completion of the feature in question.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/types/custom.d.ts (1)
180-180: Good addition of title property!The optional title property enhances button accessibility. Consider using ARIA labels for even better accessibility.
Consider adding aria-label support:
type HeaderTextButton = { elemtype: "textbutton"; text: string; className?: string; title?: string; + ariaLabel?: string; onClick?: (e: React.MouseEvent<any>) => void; };frontend/app/view/term/term.tsx (1)
321-332: Consider extracting duplicated logic!The
isBasicTermimplementation is correct, but the logic is duplicated inTerminalView. Consider extracting this to a shared utility function.+const isBasicTerm = (termMode: string, controller: string): boolean => { + return termMode !== "vdom" && controller !== "cmd"; +}; isBasicTerm(getFn: jotai.Getter): boolean { - const termMode = getFn(this.termMode); - if (termMode == "vdom") { - return false; - } - const blockData = getFn(this.blockAtom); - if (blockData?.meta?.controller == "cmd") { - return false; - } - return true; + return isBasicTerm( + getFn(this.termMode), + getFn(this.blockAtom)?.meta?.controller + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ROADMAP.md(1 hunks)frontend/app/block/blockframe.tsx(1 hunks)frontend/app/store/keymodel.ts(3 hunks)frontend/app/view/term/term.tsx(9 hunks)frontend/app/view/term/termwrap.ts(5 hunks)frontend/types/custom.d.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ROADMAP.md
- frontend/app/block/blockframe.tsx
- frontend/app/store/keymodel.ts
🔇 Additional comments (9)
frontend/app/view/term/termwrap.ts (4)
44-44: Well-structured type and property additions!The new optional
sendDataHandlerinTermWrapOptionsand the corresponding properties inTermWrapclass follow good design patterns by:
- Using dependency injection for flexible data handling
- Making the handler optional for backward compatibility
- Properly typing the callbacks for type safety
Also applies to: 61-62
74-74: Clean constructor initialization!The
sendDataHandleris properly initialized from the provided options.
193-194: Clean data handling delegation!The method elegantly delegates data handling using optional chaining, making it both safe and flexible.
196-199: Well-implemented key event handling!The method properly checks for callback existence before forwarding key events, ensuring safe operation.
frontend/types/custom.d.ts (2)
30-30: Appropriate global state type definition!The
isTermMultiInputatom is correctly typed as a primitive boolean atom in the global state.
265-267: Clean interface extension!The
isBasicTermmethod is well-defined with appropriate types and helpful documentation.frontend/app/view/term/term.tsx (3)
334-344: Well-implemented multi-input handling!The method correctly:
- Filters out self to prevent echo
- Early returns if no other terminals exist
- Properly forwards data to all other basic terminals
689-702: Clean implementation of terminal model collection!The function correctly filters and collects basic terminal models using appropriate type checks and filtering.
995-1005: Well-implemented React effect for multi-input setup!The effect correctly:
- Sets up the callback based on all required conditions
- Cleans up by removing the callback when needed
- Uses appropriate dependencies
| sendDataToController(data: string) { | ||
| const b64data = stringToBase64(data); | ||
| RpcApi.ControllerInputCommand(TabRpcClient, { blockid: this.blockId, inputdata64: b64data }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for RPC call!
While the implementation is correct, it should handle potential RPC failures.
sendDataToController(data: string) {
const b64data = stringToBase64(data);
- RpcApi.ControllerInputCommand(TabRpcClient, { blockid: this.blockId, inputdata64: b64data });
+ RpcApi.ControllerInputCommand(TabRpcClient, { blockid: this.blockId, inputdata64: b64data })
+ .catch(error => {
+ console.error('Failed to send data to controller:', error);
+ // Consider notifying the user or retrying
+ });
}📝 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.
| sendDataToController(data: string) { | |
| const b64data = stringToBase64(data); | |
| RpcApi.ControllerInputCommand(TabRpcClient, { blockid: this.blockId, inputdata64: b64data }); | |
| } | |
| sendDataToController(data: string) { | |
| const b64data = stringToBase64(data); | |
| RpcApi.ControllerInputCommand(TabRpcClient, { blockid: this.blockId, inputdata64: b64data }) | |
| .catch(error => { | |
| console.error('Failed to send data to controller:', error); | |
| // Consider notifying the user or retrying | |
| }); | |
| } |
No description provided.