-
Notifications
You must be signed in to change notification settings - Fork 0
feat(terminal): interactive shell terminal — Phase 1 #394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
b350d3f
5829205
2c25d80
4a5170e
56a896a
b5b1ff8
78699fe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ import { CalendarView } from './pages/CalendarView'; | |
| import { TodoView } from './pages/TodoView'; | ||
| import { TodoDetailView } from './pages/TodoDetailView'; | ||
| import { TaskBoard } from './pages/TaskBoard'; | ||
| import { TerminalView } from './pages/TerminalView'; | ||
| import { ErrorBoundary } from './components/ErrorBoundary'; | ||
| import { MobileShell } from './components/MobileShell'; | ||
| import { DesktopShell } from './components/DesktopShell'; | ||
|
|
@@ -168,6 +169,22 @@ export function App() { | |
| </ProtectedRoute> | ||
| } | ||
| /> | ||
| <Route | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 regressions: The terminal routes lack an
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 regressions: Terminal routes are not wrapped in |
||
| path="/terminal" | ||
| element={ | ||
| <ProtectedRoute> | ||
| <TerminalView /> | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 style: Terminal routes are not wrapped in |
||
| </ProtectedRoute> | ||
| } | ||
| /> | ||
| <Route | ||
| path="/terminal/:sessionId" | ||
| element={ | ||
| <ProtectedRoute> | ||
| <TerminalView /> | ||
| </ProtectedRoute> | ||
| } | ||
| /> | ||
| </Routes> | ||
| </MobileShell> | ||
| </BrowserRouter> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,159 @@ | ||
| /** Terminal — xterm.js wrapper with mobile keyboard toolbar. */ | ||
|
|
||
| import { useRef, useEffect, useCallback, useState } from 'react'; | ||
| import { Terminal as XTerm } from '@xterm/xterm'; | ||
| import { FitAddon } from '@xterm/addon-fit'; | ||
| import { WebLinksAddon } from '@xterm/addon-web-links'; | ||
| import '@xterm/xterm/css/xterm.css'; | ||
|
|
||
| interface TerminalProps { | ||
| onData: (data: string) => void; | ||
| onResize: (cols: number, rows: number) => void; | ||
| /** Ref callback — parent calls write() to push PTY output into the terminal. */ | ||
| termRef: React.MutableRefObject<{ write: (data: string) => void } | null>; | ||
| } | ||
|
|
||
| export function Terminal({ onData, onResize, termRef }: TerminalProps) { | ||
| const containerRef = useRef<HTMLDivElement>(null); | ||
| const xtermRef = useRef<XTerm | null>(null); | ||
| const fitRef = useRef<FitAddon | null>(null); | ||
| const [ctrlActive, setCtrlActive] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| if (!containerRef.current) return; | ||
|
|
||
| const xterm = new XTerm({ | ||
| cursorBlink: true, | ||
| fontSize: 14, | ||
| fontFamily: 'Menlo, Monaco, "Courier New", monospace', | ||
| theme: { | ||
| background: '#1a1a2e', | ||
| foreground: '#e0e0e0', | ||
| cursor: '#e0e0e0', | ||
| selectionBackground: '#3a3a5e', | ||
| black: '#1a1a2e', | ||
| red: '#ff6b6b', | ||
| green: '#51cf66', | ||
| yellow: '#ffd43b', | ||
| blue: '#74c0fc', | ||
| magenta: '#cc5de8', | ||
| cyan: '#66d9e8', | ||
| white: '#e0e0e0', | ||
| brightBlack: '#4a4a6e', | ||
| brightRed: '#ff8787', | ||
| brightGreen: '#69db7c', | ||
| brightYellow: '#ffe066', | ||
| brightBlue: '#91d5ff', | ||
| brightMagenta: '#da77f2', | ||
| brightCyan: '#99e9f2', | ||
| brightWhite: '#ffffff', | ||
| }, | ||
| allowProposedApi: true, | ||
| scrollback: 5000, | ||
| }); | ||
|
|
||
| const fit = new FitAddon(); | ||
| fitRef.current = fit; | ||
| xterm.loadAddon(fit); | ||
| xterm.loadAddon(new WebLinksAddon()); | ||
|
|
||
| xterm.open(containerRef.current); | ||
|
|
||
| // Delay fit to ensure container has layout dimensions | ||
| requestAnimationFrame(() => { | ||
| fit.fit(); | ||
| onResize(xterm.cols, xterm.rows); | ||
| }); | ||
|
|
||
| xterm.onData((data) => { | ||
| onData(data); | ||
| }); | ||
|
|
||
| // Expose write method to parent | ||
| termRef.current = { | ||
| write: (data: string) => xterm.write(data), | ||
| }; | ||
|
|
||
| xtermRef.current = xterm; | ||
|
|
||
| const handleResize = () => { | ||
| fit.fit(); | ||
| onResize(xterm.cols, xterm.rows); | ||
| }; | ||
|
|
||
| window.addEventListener('resize', handleResize); | ||
|
|
||
| // Also re-fit when keyboard opens/closes on mobile | ||
| const resizeObserver = new ResizeObserver(() => { | ||
| requestAnimationFrame(() => { | ||
| fit.fit(); | ||
| onResize(xterm.cols, xterm.rows); | ||
| }); | ||
| }); | ||
| if (containerRef.current) { | ||
| resizeObserver.observe(containerRef.current); | ||
| } | ||
|
|
||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 style: The |
||
| return () => { | ||
| window.removeEventListener('resize', handleResize); | ||
| resizeObserver.disconnect(); | ||
| termRef.current = null; | ||
| xterm.dispose(); | ||
| }; | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 style: The eslint-disable-next-line comment suppresses react-hooks/exhaustive-deps for the entire effect. The props (onData, onResize, termRef) are intentionally excluded because they're expected to be stable refs/callbacks. This is a known pattern but could break silently if a caller passes unstable callbacks. Consider documenting this contract in the TerminalProps interface. |
||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 style: The
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 style: The
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 unsafe_assumptions: The |
||
| }, []); | ||
|
|
||
| const sendKey = useCallback( | ||
| (key: string) => { | ||
| if (ctrlActive) { | ||
| // Convert to control character (Ctrl+A = \x01, Ctrl+C = \x03, etc.) | ||
| const code = key.toUpperCase().charCodeAt(0) - 64; | ||
| if (code > 0 && code < 32) { | ||
| onData(String.fromCharCode(code)); | ||
| } | ||
| setCtrlActive(false); | ||
| } else { | ||
| onData(key); | ||
| } | ||
| xtermRef.current?.focus(); | ||
| }, | ||
| [ctrlActive, onData], | ||
| ); | ||
|
|
||
| const toolbarKeys = [ | ||
| { label: 'Esc', key: '\x1b' }, | ||
| { label: 'Tab', key: '\t' }, | ||
| { label: 'Ctrl', key: '__ctrl__' }, | ||
| { label: '\u2191', key: '\x1b[A' }, | ||
| { label: '\u2193', key: '\x1b[B' }, | ||
| { label: '\u2190', key: '\x1b[D' }, | ||
| { label: '\u2192', key: '\x1b[C' }, | ||
| { label: '|', key: '|' }, | ||
| { label: '/', key: '/' }, | ||
| { label: '~', key: '~' }, | ||
| ]; | ||
|
|
||
| return ( | ||
| <div className="terminal-container"> | ||
| <div className="terminal-xterm" ref={containerRef} /> | ||
| <div className="terminal-toolbar"> | ||
| {toolbarKeys.map((tk) => ( | ||
| <button | ||
| key={tk.label} | ||
| className={`terminal-toolbar-key${tk.key === '__ctrl__' && ctrlActive ? ' terminal-toolbar-key--active' : ''}`} | ||
| onPointerDown={(e) => { | ||
| e.preventDefault(); // Don't steal focus from xterm | ||
| if (tk.key === '__ctrl__') { | ||
| setCtrlActive((v) => !v); | ||
| } else { | ||
| sendKey(tk.key); | ||
| } | ||
| }} | ||
| > | ||
| {tk.label} | ||
| </button> | ||
| ))} | ||
| </div> | ||
| </div> | ||
| ); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 style: The
/terminalroutes are not wrapped in<PageRoute>unlike all other protected pages (/inbox,/calendar,/todos,/tasks,/files). On desktop, the terminal view will not render inside theDesktopShelllayout, losing the nav sidebar. This may be intentional for a full-screen terminal experience, but is inconsistent with the other pages.[fixable]