feat: implement context menu for terminal actions #167
Conversation
- Added context menu with options to copy, paste, select all, and clear terminal content. - Integrated keyboard shortcuts for copy (Ctrl/Cmd+C), paste (Ctrl/Cmd+V), and select all (Ctrl/Cmd+A). - Enhanced platform detection for Mac users to adjust key bindings accordingly. - Implemented functionality to handle context menu actions and close the menu on outside clicks or key events.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a keyboard‑navigable in-app right‑click terminal context menu (copy, paste, select all, clear), clipboard integration via navigator.clipboard, macOS modifier detection, focus/keyboard navigation, and wiring pasted input into the existing WebSocket/input path. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant TerminalComponent
participant Clipboard as navigator.clipboard
participant WebSocket as WS/Input
Note over User,TerminalComponent: User triggers context menu or shortcut
User ->> TerminalComponent: right-click / keyboard shortcut
TerminalComponent ->> TerminalComponent: open menu / resolve action
alt Copy
TerminalComponent ->> TerminalComponent: read selection
TerminalComponent ->> Clipboard: writeText(selection)
Clipboard -->> TerminalComponent: success/failure
else Paste
TerminalComponent ->> Clipboard: readText()
Clipboard -->> TerminalComponent: clipboardText
TerminalComponent ->> WebSocket: send input(clipboardText)
else Select All
TerminalComponent ->> TerminalComponent: selectAll()
else Clear
TerminalComponent ->> TerminalComponent: clearTerminal()
end
TerminalComponent ->> User: update focus / close menu / visual feedback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40–60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @SuperComboGamer, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience of the terminal by introducing a right-click context menu and standard keyboard shortcuts for common text manipulation tasks such as copying, pasting, selecting all, and clearing content. It also includes platform-aware key binding adjustments to provide a more intuitive interaction, particularly for Mac users. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully implements a context menu for the terminal, adding useful actions like copy, paste, select all, and clear, along with their corresponding keyboard shortcuts. The implementation is well-structured, particularly the logic for handling context menu visibility and actions.
My review includes two main suggestions for improvement:
- Refactoring the platform detection logic to remove duplication and improve maintainability.
- Enhancing the accessibility of the new context menu, for example by using a dedicated library component, which would provide essential features like keyboard navigation.
Addressing these points will make the new functionality more robust and accessible.
![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)
apps/app/src/components/views/terminal-view/terminal-panel.tsx (3)
76-79: Consider usinguserAgentDatawith fallback for platform detection.
navigator.platformis deprecated. While it still works across browsers, you may want to future-proof this with a fallback pattern.🔎 Suggested improvement
// Detect platform on mount useEffect(() => { - setIsMac(navigator.platform.toUpperCase().indexOf("MAC") >= 0); + const checkIsMac = () => { + // Modern API (Chromium-based browsers) + if ('userAgentData' in navigator && navigator.userAgentData?.platform) { + return navigator.userAgentData.platform.toUpperCase().includes('MAC'); + } + // Fallback for other browsers + return /mac/i.test(navigator.platform || navigator.userAgent); + }; + setIsMac(checkIsMac()); }, []);
348-350: Duplicate platform detection logic.This duplicates the platform detection from line 78. While using inline detection here avoids potential stale closure issues with the
isMacstate, consider extracting this to a shared utility function for consistency.🔎 Extract to utility
Create a utility function that can be used in both places:
// In a utils file or at the top of this file const getIsMac = () => /mac/i.test(navigator.platform || '');Then use
getIsMac()in both the useEffect and the key handler.
849-889: Add viewport boundary checking and accessibility attributes.Two improvements to consider:
Viewport boundaries: The menu could render off-screen if the user right-clicks near the viewport edge.
Accessibility: Add ARIA roles for screen reader support.
🔎 Suggested improvements
{/* Context menu */} {contextMenu && ( <div className="fixed z-50 min-w-[160px] rounded-md border border-border bg-popover p-1 shadow-md animate-in fade-in-0 zoom-in-95" - style={{ left: contextMenu.x, top: contextMenu.y }} + style={{ + left: Math.min(contextMenu.x, window.innerWidth - 180), + top: Math.min(contextMenu.y, window.innerHeight - 200), + }} + role="menu" + aria-label="Terminal context menu" onClick={(e) => e.stopPropagation()} > <button className="flex w-full items-center gap-2 rounded-sm px-2 py-1.5 text-sm text-popover-foreground hover:bg-accent hover:text-accent-foreground cursor-default" onClick={() => handleContextMenuAction("copy")} + role="menuitem" >Apply
role="menuitem"to all button elements in the menu.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/app/src/components/views/terminal-view/terminal-panel.tsx(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (4)
apps/app/src/components/views/terminal-view/terminal-panel.tsx (4)
114-156: LGTM!The clipboard operations are well-implemented with proper null checks, error handling, and WebSocket state validation before sending paste data.
163-181: LGTM!Clean action dispatcher with type-safe union, proper async handling, and good UX by refocusing the terminal after actions complete.
359-370: LGTM!The Ctrl+C behavior correctly distinguishes between copy (when text is selected) and SIGINT (when no selection), matching standard terminal conventions.
671-690: LGTM!Proper event listener management with cleanup. Using capture phase for scroll events ensures the menu closes regardless of which element is scrolled.
SuperComboGamer
added
the
Do Not Merge
- Improved context menu functionality by adding keyboard navigation support for actions (copy, paste, select all, clear). - Utilized refs to manage focus on menu items and updated platform detection for Mac users. - Ensured context menu closes on outside clicks and handles keyboard events effectively.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/app/src/components/views/terminal-view/terminal-panel.tsx (2)
683-730: Optimize event listener management.Including
focusedMenuIndexin the dependencies causes all event listeners to be removed and re-registered on every arrow key press. To improve performance, consider using a ref to track the focused index or restructuring the handler to avoid needing this in the dependency array.🔎 Proposed optimization using ref pattern
+ const focusedMenuIndexRef = useRef(0); // Close context menu on click outside or scroll, handle keyboard navigation useEffect(() => { if (!contextMenu) return; // Reset focus index and focus menu when opened setFocusedMenuIndex(0); + focusedMenuIndexRef.current = 0; requestAnimationFrame(() => { const firstButton = contextMenuRef.current?.querySelector<HTMLButtonElement>('[role="menuitem"]'); firstButton?.focus(); }); const handleClick = () => closeContextMenu(); const handleScroll = () => closeContextMenu(); const handleKeyDown = (e: KeyboardEvent) => { switch (e.key) { case "Escape": e.preventDefault(); closeContextMenu(); break; case "ArrowDown": e.preventDefault(); - setFocusedMenuIndex((prev) => (prev + 1) % menuActions.length); + setFocusedMenuIndex((prev) => { + const next = (prev + 1) % menuActions.length; + focusedMenuIndexRef.current = next; + return next; + }); break; case "ArrowUp": e.preventDefault(); - setFocusedMenuIndex((prev) => (prev - 1 + menuActions.length) % menuActions.length); + setFocusedMenuIndex((prev) => { + const next = (prev - 1 + menuActions.length) % menuActions.length; + focusedMenuIndexRef.current = next; + return next; + }); break; case "Enter": case " ": e.preventDefault(); - handleContextMenuAction(menuActions[focusedMenuIndex]); + handleContextMenuAction(menuActions[focusedMenuIndexRef.current]); break; case "Tab": e.preventDefault(); closeContextMenu(); break; } }; document.addEventListener("click", handleClick); document.addEventListener("scroll", handleScroll, true); document.addEventListener("keydown", handleKeyDown); return () => { document.removeEventListener("click", handleClick); document.removeEventListener("scroll", handleScroll, true); document.removeEventListener("keydown", handleKeyDown); }; - }, [contextMenu, closeContextMenu, focusedMenuIndex, handleContextMenuAction]); + }, [contextMenu, closeContextMenu, handleContextMenuAction]);
897-958: Good accessibility improvements; consider a component library for robustness.The context menu now includes keyboard navigation (arrow keys, Enter, Escape) and proper ARIA attributes, which significantly addresses the previous accessibility concerns. However, the previous suggestion to use a dedicated library like Radix UI or Headless UI still has merit, as they provide additional features like Home/End key support, typeahead navigation, and more comprehensive screen reader announcements.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/app/src/components/views/terminal-view/terminal-panel.tsx(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/app/src/components/views/terminal-view/terminal-panel.tsx (1)
apps/app/src/lib/utils.ts (1)
cn(5-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (9)
apps/app/src/components/views/terminal-view/terminal-panel.tsx (9)
12-15: LGTM!The new icon imports are appropriate for the context menu actions.
73-77: LGTM!The state declarations appropriately support the context menu feature. The use of both
isMacstate andisMacRefcorrectly addresses the previous review concern about stale closures in event handlers.
79-88: LGTM!The platform detection correctly uses the modern
userAgentDataAPI with a fallback tonavigator.platform, addressing the previous review comment about deprecation. Setting both state and ref ensures UI updates and prevents stale closures in event handlers.
107-108: LGTM!Using refs to store the latest function versions prevents stale closures in the keyboard event handler.
123-138: LGTM!The copy implementation correctly uses the Clipboard API with proper error handling and returns a boolean to indicate success.
142-154: LGTM!The paste implementation correctly integrates the Clipboard API with the WebSocket input path and includes appropriate error handling.
157-165: LGTM!Clean wrappers around terminal operations.
167-190: LGTM!The context menu action handler follows a clean pattern: close menu, execute action, and restore terminal focus.
357-392: LGTM!The keyboard shortcuts correctly use
isMacRef.currentto avoid stale closures, addressing the previous review concern. The Ctrl+C behavior intelligently copies when text is selected and sends SIGINT otherwise, which is standard terminal UX.
- Enhanced context menu positioning with boundary checks to prevent overflow on screen edges. - Updated platform detection logic for Mac users to utilize modern userAgentData API with a fallback to deprecated navigator.platform. - Ensured context menu opens correctly within viewport limits, improving user experience.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/app/src/components/views/terminal-view/terminal-panel.tsx (1)
127-159: Consider adding user feedback for clipboard operation failures.The clipboard operations use the modern
navigator.clipboardAPI correctly, but failures are only logged to console. Users won't receive feedback if copy or paste fails (e.g., due to clipboard permissions).Consider showing a brief toast notification or terminal message when these operations fail, especially since the context menu actions are user-initiated and users expect feedback.
Example: Add visual feedback for clipboard failures
You could add a toast notification system or write a message to the terminal:
const copySelection = useCallback(async (): Promise<boolean> => { const terminal = xtermRef.current; if (!terminal) return false; const selection = terminal.getSelection(); if (!selection) return false; try { await navigator.clipboard.writeText(selection); return true; } catch (err) { console.error("[Terminal] Copy failed:", err); + terminal.write("\r\n\x1b[31m[Copy failed: clipboard access denied]\x1b[0m\r\n"); return false; } }, []);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/app/src/components/views/terminal-view/terminal-panel.tsx(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (7)
apps/app/src/components/views/terminal-view/terminal-panel.tsx (7)
12-15: Icon imports look good.The new icon imports are appropriate for the context menu functionality.
73-77: Good state management for context menu.The combination of state and refs is appropriate. The
isMacRefpattern correctly avoids the stale closure issue that would occur when accessing platform detection from the keyboard handler.
79-92: Platform detection implementation is solid.Good use of the modern
userAgentDataAPI with an appropriate fallback. The ref pattern ensures keyboard handlers always have access to the current platform without stale closures.
161-194: Terminal operations are well-structured.The action handler properly orchestrates clipboard operations, closes the menu, and refocuses the terminal. Good UX decisions throughout.
361-396: Keyboard shortcuts are well-implemented.The shortcut logic correctly uses
isMacRef.currentto avoid the platform detection duplication issue mentioned in previous reviews. The Ctrl+C behavior (copy when text is selected, otherwise send SIGINT) matches standard terminal conventions.
743-772: Boundary checking implementation looks good.The context menu positioning correctly handles viewport edges and prevents off-screen rendering. This addresses the concern raised in previous reviews.
One minor maintainability note: the menu height is hardcoded at line 750 (
menuHeight = 152). If menu items are added or removed in the future, this value would need manual updating. Consider dynamically measuring the menu dimensions after render if this becomes an issue.
924-987: Context menu UI has good accessibility features.The menu implementation includes proper ARIA roles, keyboard navigation support, and platform-specific shortcut hints. This addresses the accessibility concerns raised in previous reviews about missing keyboard navigation.
The Tab key behavior (closes the menu rather than cycling through items) differs from some menu implementations, but it's a reasonable choice that allows users to quickly exit the menu and continue keyboard navigation.
…ard navigation - Introduced a ref to keep the focused menu index in sync with state, enhancing keyboard navigation within the terminal context menu. - Updated event handlers to utilize the ref for managing focus, ensuring consistent behavior during menu interactions. - Simplified dependencies in the effect hook for better performance and clarity.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/app/src/components/views/terminal-view/terminal-panel.tsx (2)
755-784: LGTM! Context menu positioning with boundary checks.This implementation addresses the previous feedback about the menu rendering off-screen near viewport edges. The boundary checking correctly:
- Calculates position relative to window dimensions
- Adjusts for right and bottom edges
- Ensures minimum padding
The approximate menu height (152px at line 762) works for the current 4 items + separator. If menu items become dynamic in the future, consider measuring the actual rendered height using
getBoundingClientRect()after the menu is rendered.
936-999: LGTM! Context menu rendering is accessible and well-structured.The context menu implementation properly addresses previous accessibility feedback with:
- ARIA roles (
role="menu",role="menuitem")- ARIA labels (
aria-label="Terminal context menu")- Proper
tabIndexmanagement for keyboard navigation- Visual feedback for focused items
- Platform-specific keyboard hints
Optional UX improvements (non-blocking):
Disable "Copy" when no selection exists
+ const hasSelection = xtermRef.current?.hasSelection() ?? false; <button role="menuitem" tabIndex={focusedMenuIndex === 0 ? 0 : -1} + disabled={!hasSelection} className={cn( "flex w-full items-center gap-2 rounded-sm px-2 py-1.5 text-sm text-popover-foreground cursor-default outline-none", + !hasSelection && "opacity-50 cursor-not-allowed", focusedMenuIndex === 0 ? "bg-accent text-accent-foreground" : "hover:bg-accent hover:text-accent-foreground" )} onClick={() => handleContextMenuAction("copy")} >Show all keyboard shortcut variants in hint
The hint shows only
⌘Cfor copy, but⌘⇧Calso works. Consider showing both variants:- <span className="text-xs text-muted-foreground">{isMac ? "⌘C" : "Ctrl+C"}</span> + <span className="text-xs text-muted-foreground">{isMac ? "⌘C / ⌘⇧C" : "Ctrl+C / Ctrl+Shift+C"}</span>(Though this might make the hint too long)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/app/src/components/views/terminal-view/terminal-panel.tsx(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/app/src/components/views/terminal-view/terminal-panel.tsx (1)
apps/app/src/lib/utils.ts (1)
cn(5-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (11)
apps/app/src/components/views/terminal-view/terminal-panel.tsx (11)
12-15: LGTM! Icon imports are appropriate for context menu actions.The imported icons match their respective actions (Copy, ClipboardPaste, CheckSquare for select all, Trash2 for clear) and follow the existing pattern in the file.
73-78: LGTM! State and ref management is well-structured.The dual state+ref pattern for
isMacandfocusedMenuIndexcorrectly avoids stale closures in event handlers while maintaining reactivity. This addresses the previous performance concern about effects re-running on every keystroke.
80-93: LGTM! Platform detection addresses previous feedback.The implementation now uses the modern
navigator.userAgentData.platformAPI with a graceful fallback tonavigator.platform, addressing the previous concern about using deprecated APIs. The intentional comment on line 88 shows awareness of the fallback's deprecation status.
162-170: LGTM! Terminal operations are clean wrappers.The
selectAllandclearTerminalfunctions are straightforward wrappers around the xterm API. These operations are safe and don't require additional error handling.
177-195: LGTM! Context menu action handler is well-structured.The centralized handler properly:
- Closes the menu before executing actions
- Routes to appropriate operations
- Refocuses the terminal after actions (good UX)
- Uses stable callbacks in dependencies
The implementation is clean and maintainable.
362-397: LGTM! Keyboard shortcuts are properly implemented.The platform-aware keyboard shortcuts correctly:
- Use
isMacRef.currentto avoid closure issues (addresses previous feedback)- Implement terminal conventions (Ctrl+Shift+C always copies, Ctrl+C sends SIGINT when no selection)
- Handle both Ctrl+V and Ctrl+Shift+V for paste
- Clear selection after copy (line 378) for good UX
- Use
event.codefor keyboard-layout-independent detection- Use refs to access latest callback implementations
684-690: LGTM! Menu actions array and ref synchronization.The readonly
menuActionsarray defines a clear order for keyboard navigation, and the effect keepsfocusedMenuIndexRefsynchronized for use in event handlers.
692-746: LGTM! Context menu lifecycle addresses all previous feedback.This implementation successfully addresses previous concerns:
- Performance: Uses
focusedMenuIndexRef.currentin the event handler to avoid re-running the effect on every arrow key press (previous feedback)- Keyboard navigation: Fully implements Arrow keys, Enter, Space, Escape, and Tab (previous feedback about missing accessibility)
- Focus management: Uses
requestAnimationFrameto ensure DOM is ready before focusing (line 699)- Cleanup: Properly removes all event listeners
The
updateFocusIndexhelper (lines 707-710) keeps the ref and state synchronized, solving the stale closure problem elegantly.
748-753: LGTM! Focus update effect correctly applies keyboard navigation.This effect cleanly separates concerns: the keyboard handler updates the focus index, and this effect applies the visual focus to the appropriate menu button.
929-934: LGTM! Context menu correctly scoped to terminal area.The
onContextMenuhandler is properly attached to the terminal container (line 933), ensuring the context menu only appears when right-clicking within the terminal viewport rather than the entire panel (including header). This is the correct scope for terminal actions.
128-160: Error handling is appropriate; no permission check needed.The current try-catch implementation correctly handles clipboard failures, including permission denials. Modern browsers automatically prompt for permission when clipboard methods are called, and the promise rejection on denial is caught by the existing error handlers. The suggested
navigator.clipboardcheck only verifies API existence, not permissions, making it redundant.If this terminal component runs in an iframe, ensure the iframe element includes
allow="clipboard-read; clipboard-write"to enable clipboard access.
SuperComboGamer
removed
the
Do Not Merge
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.