feat(desktop): add userTitle field for user-renamed tabs#289
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds user-settab titles via a new optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Terminal as Terminal Component
participant PasteH as Paste Handler
participant Store as TabsStore
participant Display as Tab Display (getTabDisplayName)
Note over User,Terminal: User types or pastes into terminal
User->>Terminal: type chars / paste text
Terminal->>PasteH: setupPasteHandler(onPaste)
PasteH-->>Terminal: onPaste(pastedText)
Terminal->>Terminal: append to commandBufferRef
Terminal->>Terminal: onKey Enter -> sanitizeForTitle(buffer)
alt sanitized non-null
Terminal->>Store: setTabAutoTitle(tabId, sanitized)
Store->>Store: update tab.name = sanitized
else sanitized null
Note right of Terminal: no auto-title update
end
Note over Display,Store: Rendering precedence when showing tab title
Display->>Store: read tab
Store-->>Display: return { name, userTitle }
Display->>Display: show userTitle if non-empty else name
Display-->>User: render tab title
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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! 🎉 |
Separates auto-generated tab names from user-set titles. The auto-generated name (e.g., "Terminal 1") is preserved for consistent tab numbering while userTitle stores custom names set via rename. User title takes precedence when non-empty; clearing it reverts to the auto-generated name. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Kitenite
force-pushed
the
courageous-opossum-0bc099
branch
from
f75ba37 to
28915fe
Compare
![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 (1)
apps/desktop/src/renderer/stores/tabs/store.ts (1)
161-167: Implementation is correct and aligns with PR objectives.The mutation correctly updates
userTitleinstead ofname, preserving the auto-generated name for consistent numbering while storing the user's custom title.For improved clarity, consider renaming the parameter to reflect that it sets
userTitle:-renameTab: (tabId, newName) => { +renameTab: (tabId, newTitle) => { set((state) => ({ tabs: state.tabs.map((t) => - t.id === tabId ? { ...t, userTitle: newName } : t, + t.id === tabId ? { ...t, userTitle: newTitle } : t, ), })); },Also update the type signature in
types.ts:-renameTab: (tabId: string, newName: string) => void; +renameTab: (tabId: string, newTitle: string) => void;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx(2 hunks)apps/desktop/src/renderer/stores/tabs/store.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/types.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsxapps/desktop/src/renderer/stores/tabs/store.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsxapps/desktop/src/renderer/stores/tabs/store.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Co-locate component dependencies (utils, hooks, constants, config, tests, stories) next to the file using them
Avoidanytype in TypeScript unless absolutely necessary
Files:
apps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsxapps/desktop/src/renderer/stores/tabs/store.ts
apps/desktop/src/{renderer,shared,preload}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in renderer process code or shared code in the desktop app
Files:
apps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsxapps/desktop/src/renderer/stores/tabs/store.ts
apps/desktop/src/{main,renderer,shared}/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use type-safe IPC system for communication between Electron main and renderer processes
Files:
apps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/stores/tabs/utils.tsapps/desktop/src/renderer/stores/tabs/store.ts
**/{components,pages}/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
**/{components,pages}/**/*.tsx: Use one folder per component with structureComponentName/ComponentName.tsxplusindex.tsfor barrel export
Create one component per file - do not use multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx
🧬 Code graph analysis (1)
apps/desktop/src/renderer/stores/tabs/utils.ts (1)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
Tab(25-32)
⏰ 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). (1)
- GitHub Check: Build
🔇 Additional comments (4)
apps/desktop/src/renderer/stores/tabs/utils.ts (1)
15-21: LGTM! Precedence logic is well-implemented.The
userTitleprecedence logic correctly handles all edge cases: optional chaining safely handlesundefined,trim()ensures whitespace-only titles fall through to the auto-generated name, and the falsy check on the trimmed string properly catches empty strings.apps/desktop/src/renderer/stores/tabs/types.ts (1)
27-28: LGTM! Clear type definition with helpful documentation.The optional
userTitlefield is properly typed, and the inline comments clearly explain the distinction between the auto-generatednameand the user-setuserTitlewith its precedence behavior.apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx (2)
79-87: LGTM! Rename initialization correctly prioritizesuserTitle.The initialization logic properly uses
userTitlewhen available, falling back tonameand thendisplayName. The nullish coalescing chain ensures the input always has a value to display.
89-97: LGTM! Rename submission logic correctly handles clearinguserTitle.The comparison against
currentUserTitle(trimmed or empty string) properly supports all rename scenarios:
- Setting a custom title updates
userTitle- Clearing the input (empty string) clears
userTitle, revealing the auto-generated name- No-op when the value hasn't changed
Tracks terminal input and updates the tab's auto-generated title when the user submits a command (presses Enter). Handles backspace, Ctrl+C, and Ctrl+U for accurate command tracking. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx (1)
79-86: Consider treating empty/whitespaceuserTitleas “no custom title” when initializing rename
tab.userTitle ?? tab.name ?? displayNamewill useuserTitleeven when it’s an empty string or all-whitespace, so the rename input can appear blank while the tab label shows the auto-generated name. To better match the “empty string means cleared” semantics and user expectations, you could normalize here and fall back to the auto name whenuserTitleis empty/whitespace:- const startRename = () => { - setRenameValue(tab.userTitle ?? tab.name ?? displayName); + const startRename = () => { + const initialTitle = tab.userTitle?.trim() || tab.name || displayName; + setRenameValue(initialTitle); setIsRenaming(true); setTimeout(() => { inputRef.current?.focus(); inputRef.current?.select(); }, 0); };This keeps the behavior for existing non-empty titles while avoiding the “blank input but non-blank label” edge case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx(2 hunks)apps/desktop/src/renderer/stores/tabs/store.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/types.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/renderer/stores/tabs/utils.ts
- apps/desktop/src/renderer/stores/tabs/store.ts
- apps/desktop/src/renderer/stores/tabs/types.ts
🧰 Additional context used
📓 Path-based instructions (5)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx
**/{components,pages}/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
**/{components,pages}/**/*.tsx: Use one folder per component with structureComponentName/ComponentName.tsxplusindex.tsfor barrel export
Create one component per file - do not use multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Co-locate component dependencies (utils, hooks, constants, config, tests, stories) next to the file using them
Avoidanytype in TypeScript unless absolutely necessary
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx
apps/desktop/src/{renderer,shared,preload}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in renderer process code or shared code in the desktop app
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx
⏰ 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). (1)
- GitHub Check: Build
🔇 Additional comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx (1)
88-95: Rename submission logic correctly targetsuserTitleand supports clearingThe
submitRenameflow looks consistent with the newuserTitlesemantics:
- It trims the input and compares against the trimmed existing
tab.userTitle, avoiding no-op writes and whitespace-only changes.- It calls
renameTab(tab.id, trimmedValue)even whentrimmedValueis"", enabling the “clear to revert to auto-generated name” behavior described in the PR.- It does not touch the underlying
namefield, so auto-generated numbering can remain stable.This aligns well with the intent of keeping
namefor numbering and usinguserTitlepurely as an override.
Moves command input processing to commandBuffer.ts with comprehensive tests for enter, backspace, and cancel key handling. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Use strip-ansi library for ANSI escape sequence removal - Add lodash debounce (100ms) to prevent rapid title updates - Limit title length to 32 characters - Add tests for sanitization 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove strip-ansi dependency and use simple regex to keep only alphanumeric, space, underscore, dash, dot, and slash characters. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
25-32: Clarify contract betweenname,userTitle, andsetTabAutoTitleThe new
userTitle?: stringfield andsetTabAutoTitleAPI fit the described behavior (auto-generated vs user-provided titles) and look type-safe. To prevent misuse, I’d suggest tightening the comments aroundTab/TabsStoreto spell out that:
nameis the persisted auto-generated title (updated viasetTabAutoTitleonly).userTitleis an optional user-renamed display title, written viarenameTab.- UI display helpers should always prefer a non-empty
userTitleovername.This will make the intended separation obvious to future callers.
Also applies to: 48-54
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.test.ts (1)
1-137: Good behavioral coverage; consider aligning with “one assertion per test” guidelineThese tests exercise the command buffer and sanitization paths well (enter/cancel/backspace flows, printable filtering, ANSI stripping, truncation, null handling) and are readable/independent.
However, many
itblocks contain multipleexpectcalls, which diverges from the repo guideline that tests should have one assert per test forapps/desktop/**/*.test.{ts,tsx,js,jsx}. If you want strict alignment with that style guide, consider:
- Grouping expectations into a single structural assertion per test (e.g.,
expect(result).toMatchObject({ buffer: "...", submittedCommand: ... })), or- Splitting some cases (e.g., buffer vs submittedCommand) into separate
itblocks.Functionally this is fine as-is, so this can be a follow-up cleanup rather than a blocker.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
31-31: Auto-title integration looks good; refine debounce lifecycle to avoid stray timersThe new flow—tracking a per-terminal
commandBufferRef, running input throughprocessCommandInput, and debounced-callingsetTabAutoTitleonsubmittedCommandtied toparentTabIdRef—is a nice separation of concerns and keeps PTY writes (writeRef.current({ tabId: paneId, data })) unchanged.One improvement: the debounced function is recreated on every render:
useRef(debounce(setTabAutoTitle, 100))initializes once, butdebouncedSetTabAutoTitleRef.current = debounce(setTabAutoTitle, 100);assigns a new debounced instance every render, and- There’s no
cancel()on unmount.That can (a) weaken debouncing guarantees across re-renders and (b) leave pending timers that can still fire after unmount.
You can stabilize and clean this up by initializing once in an effect and cancelling on teardown, e.g.:
- const parentTabId = pane?.tabId; + const parentTabId = pane?.tabId; … - const parentTabIdRef = useRef(parentTabId); - parentTabIdRef.current = parentTabId; - - const debouncedSetTabAutoTitleRef = useRef(debounce(setTabAutoTitle, 100)); - debouncedSetTabAutoTitleRef.current = debounce(setTabAutoTitle, 100); + const parentTabIdRef = useRef(parentTabId); + parentTabIdRef.current = parentTabId; + + const debouncedSetTabAutoTitleRef = + useRef<ReturnType<typeof debounce> | null>(null); + + useEffect(() => { + const debounced = debounce(setTabAutoTitle, 100); + debouncedSetTabAutoTitleRef.current = debounced; + return () => debounced.cancel(); + }, [setTabAutoTitle]); @@ - if (result.submittedCommand && parentTabIdRef.current) { - debouncedSetTabAutoTitleRef.current( - parentTabIdRef.current, - result.submittedCommand, - ); - } + if ( + result.submittedCommand && + parentTabIdRef.current && + debouncedSetTabAutoTitleRef.current + ) { + debouncedSetTabAutoTitleRef.current( + parentTabIdRef.current, + result.submittedCommand, + ); + }This keeps the debounce identity stable across renders and guarantees any queued callbacks are cancelled when the terminal unmounts.
If you want to double-check
lodash/debouncetypings/behavior with this pattern in your current version, please verify against the library docs or type definitions.Also applies to: 38-39, 42-42, 73-78, 225-243
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.ts (1)
1-56: Command buffer & title sanitization are solid; optional edge-case refinementsThe
sanitizeForTitleandprocessCommandInputimplementations are clear, side-effect free, and align with the tests:
- ANSI is stripped via
strip-ansi, non-printables removed, whitespace trimmed, and titles truncated to 32 chars withnullfor empty/whitespace-only results.- The buffer state machine (ENTER → submit & clear, BACKSPACE → drop last char, CANCEL → clear, otherwise append printable chars including tab) is easy to reason about.
Two optional considerations:
- Control-char precedence: You currently prioritize ENTER, then BACKSPACE, then CANCEL based on
input.includes(...). If xterm ever emits multiple control characters in a single chunk, BACKSPACE would win over CANCEL. If you’d prefer “clear line” semantics to always win, you could checkhasCancelbeforehasBackspace.- Non-ASCII commands: The
[^\x20-\x7e]filter strips all non-ASCII characters from titles. If you later want to support non-Latin commands in tab titles, you might loosen this to only strip control characters (Unicode category\p{C}) while keeping printable non-ASCII.These aren’t blockers; the current behavior is consistent with the tests and the PR’s goals.
Since this relies on
strip-ansi’s behavior and your expected xtermonDatasemantics, please confirm against your terminal/strip-ansi versions if you change the control-char or character-class handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
apps/desktop/package.json(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx(4 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.test.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/store.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/types.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/stores/tabs/store.ts
🧰 Additional context used
📓 Path-based instructions (7)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.test.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Co-locate component dependencies (utils, hooks, constants, config, tests, stories) next to the file using them
Avoidanytype in TypeScript unless absolutely necessary
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.test.ts
apps/desktop/src/{renderer,shared,preload}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in renderer process code or shared code in the desktop app
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.test.ts
apps/desktop/src/{main,renderer,shared}/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use type-safe IPC system for communication between Electron main and renderer processes
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.tsapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.test.ts
**/{components,pages}/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
**/{components,pages}/**/*.tsx: Use one folder per component with structureComponentName/ComponentName.tsxplusindex.tsfor barrel export
Create one component per file - do not use multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
apps/desktop/**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.test.{ts,tsx,js,jsx}: Tests should have one assert per test
Tests should be readable
Tests should be fast
Tests should be independent
Tests should be repeatable
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.test.ts
🧠 Learnings (3)
📚 Learning: 2025-12-06T18:44:13.290Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T18:44:13.290Z
Learning: Applies to apps/desktop/src/{main,renderer,shared}/**/*.ts : Use type-safe IPC system for communication between Electron main and renderer processes
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.ts
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be readable
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.test.ts
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be repeatable
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.test.ts
🧬 Code graph analysis (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.test.ts (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.ts (2)
processCommandInput(22-56)sanitizeForTitle(13-20)
⏰ 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). (1)
- GitHub Check: Build
🔇 Additional comments (1)
apps/desktop/package.json (1)
84-84: strip-ansi dependency addition matches new terminal title sanitizationAdding
strip-ansias a runtime dependency aligns with its use incommandBuffer.tsfor cleaning terminal input before deriving tab titles; keeping it independencies(notdevDependencies) is appropriate.Please confirm your bundler/tsconfig is configured for default ESM-style imports for
strip-ansi(as used incommandBuffer.ts), or adjust the import to match your module settings if needed.
Kitenite
force-pushed
the
courageous-opossum-0bc099
branch
from
2edf07c to
33a5398
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
31-42: Consider cancelling debounced tab-title updates on unmountUsing refs for
parentTabIdandsetTabAutoTitleplus a debounced updater is a solid pattern to avoid effect churn and stale closures. One minor improvement: the debounced function can keep a pending timeout after this component unmounts. It’s harmless since it only hits the store, but cancelling it in the cleanup would avoid stray work and make behavior more predictable (especially in tests):return () => { + debouncedSetTabAutoTitle.cancel?.(); isUnmounted = true; inputDisposable.dispose(); cleanupKeyboard(); cleanupFocus?.(); cleanupResize(); cleanupPaste(); cleanupQuerySuppression(); detachRef.current({ tabId: paneId }); setSubscriptionEnabled(false); xterm.dispose(); xtermRef.current = null; searchAddonRef.current = null; };Also applies to: 73-84, 320-327
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx(5 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.test.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.test.ts
🧰 Additional context used
📓 Path-based instructions (6)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.ts
**/{components,pages}/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
**/{components,pages}/**/*.tsx: Use one folder per component with structureComponentName/ComponentName.tsxplusindex.tsfor barrel export
Create one component per file - do not use multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Co-locate component dependencies (utils, hooks, constants, config, tests, stories) next to the file using them
Avoidanytype in TypeScript unless absolutely necessary
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.ts
apps/desktop/src/{renderer,shared,preload}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in renderer process code or shared code in the desktop app
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.ts
apps/desktop/src/{main,renderer,shared}/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use type-safe IPC system for communication between Electron main and renderer processes
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.ts
🧠 Learnings (1)
📚 Learning: 2025-12-06T18:44:13.290Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T18:44:13.290Z
Learning: Applies to apps/desktop/src/{main,renderer,shared}/**/*.ts : Use type-safe IPC system for communication between Electron main and renderer processes
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.ts
⏰ 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). (1)
- GitHub Check: Build
🔇 Additional comments (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (2)
5-12: Command-buffer and debounce imports are cleanly scopedBringing in
lodash/debounceand localprocessCommandInputkeeps the terminal component lean, and there are no renderer/main boundary issues here. Looks good.
231-249: handleTerminalInput restart + auto-title plumbing looks correctResetting
commandBufferRefon restart and early-returning avoids leaking old input into a new PTY session, and routing all non-exit input throughprocessCommandInputbefore callingwriteRefcleanly isolates auto-title logic from the actual terminal I/O. No issues spotted here.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.ts (1)
11-19: sanitizeForTitle is a solid, bounded sanitizerCapping input length before sanitizing, then trimming and enforcing a final max length with a conservative allowlist is a good balance between robustness and readable titles. Returning
nullfor empty output keeps callers’ logic simple.
| export function processCommandInput( | ||
| currentBuffer: string, | ||
| input: string, | ||
| ): CommandBufferResult { | ||
| const hasEnter = ENTER.some((char) => input.includes(char)); | ||
| const hasBackspace = BACKSPACE.some((char) => input.includes(char)); | ||
| const hasCancel = CANCEL.some((char) => input.includes(char)); | ||
|
|
||
| if (hasEnter) { | ||
| return { | ||
| buffer: "", | ||
| submittedCommand: sanitizeForTitle(currentBuffer), | ||
| }; | ||
| } | ||
|
|
||
| if (hasBackspace) { | ||
| return { | ||
| buffer: currentBuffer.slice(0, -1), | ||
| submittedCommand: null, | ||
| }; | ||
| } | ||
|
|
||
| if (hasCancel) { | ||
| return { | ||
| buffer: "", | ||
| submittedCommand: null, | ||
| }; | ||
| } | ||
|
|
||
| const printableChars = input.replace(/[^\x20-\x7e\t]/g, ""); | ||
| return { | ||
| buffer: currentBuffer + printableChars, | ||
| submittedCommand: null, | ||
| }; | ||
| } |
There was a problem hiding this comment.
processCommandInput may mis-track buffer for multi-character input chunks
The current logic treats input as if it contains at most one special key: on hasEnter/hasBackspace/hasCancel, it ignores any printable characters in the same chunk and only looks at currentBuffer. For example:
- If
input === "ls\r"andcurrentBuffer === "", you’ll submitsanitizeForTitle("")(i.e.null) rather than"ls". - If
input === "d\b"andcurrentBuffer === "abc", the new buffer becomes"ab"instead of the expected"abc"(from"abcd".slice(0, -1)).
This is fine as long as upstream always calls it once per keystroke, but it will drift from what the user sees if input is ever batched (e.g., some paste scenarios or repeated keys).
You could make this more robust by processing input character-by-character, applying the same rules in sequence:
export function processCommandInput(
currentBuffer: string,
input: string,
): CommandBufferResult {
let buffer = currentBuffer;
let submittedCommand: string | null = null;
for (const ch of input) {
if (ENTER.includes(ch)) {
submittedCommand = sanitizeForTitle(buffer);
buffer = "";
} else if (BACKSPACE.includes(ch)) {
buffer = buffer.slice(0, -1);
} else if (CANCEL.includes(ch)) {
buffer = "";
} else if (/^[\x20-\x7e\t]$/.test(ch)) {
buffer += ch;
}
}
return { buffer, submittedCommand };
}That keeps behavior the same for single-key inputs while handling batched ones correctly.
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.ts
around lines 21-55, the function treats the entire input chunk as one event
(enter/backspace/cancel) and ignores printable characters in the same chunk,
causing incorrect buffer/submission for multi-character inputs; fix by iterating
over each character in input in order, updating a local buffer and
submittedCommand for each char (enter -> submit sanitized current buffer and
clear buffer, backspace -> slice last char from buffer, cancel -> clear buffer,
printable -> append to buffer), then return the final buffer and
submittedCommand.
Track keystrokes via xterm.onKey instead of parsing PTY data from onData. This avoids TUI escape sequence pollution in tab titles since onKey captures actual keyboard events rather than shell output. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Pass onPaste callback to setupPasteHandler so pasted text is appended to the command buffer, not just typed keystrokes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Keep most characters in tab titles, only strip ANSI escape sequences. This preserves uppercase, special chars like @, etc. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Read current line from xterm buffer when Enter is pressed instead of tracking keystrokes. This captures autocompleted text from shells like zsh with powerlevel10k or zsh-autosuggestions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Use refs for paneName, terminalTheme, and debouncedSetTabAutoTitle to avoid triggering effect re-runs - Theme changes handled by separate sync effect, not terminal recreation - Revert to keystroke tracking (onKey + paste) for command buffer - Add sanitizeForTitle to strip ANSI sequences from tab titles 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
apps/desktop/package.json(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx(9 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.test.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.test.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.ts
🧰 Additional context used
📓 Path-based instructions (6)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
**/{components,pages}/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
**/{components,pages}/**/*.tsx: Use one folder per component with structureComponentName/ComponentName.tsxplusindex.tsfor barrel export
Create one component per file - do not use multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Co-locate component dependencies (utils, hooks, constants, config, tests, stories) next to the file using them
Avoidanytype in TypeScript unless absolutely necessary
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
apps/desktop/src/{renderer,shared,preload}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in renderer process code or shared code in the desktop app
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
apps/desktop/src/{main,renderer,shared}/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use type-safe IPC system for communication between Electron main and renderer processes
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
🧬 Code graph analysis (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (2)
createTerminalInstance(52-128)setupPasteHandler(156-187)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.ts (1)
sanitizeForTitle(5-9)
⏰ 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). (1)
- GitHub Check: Build
🔇 Additional comments (4)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (1)
137-173: LGTM! Clean extension for paste tracking.The addition of the
PasteHandlerOptionsinterface and optionalonPastecallback is well-designed:
- Backward compatible with default parameter
- Properly invokes the callback before calling
xterm.paste()- Enables command buffer tracking without changing existing behavior
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (2)
46-47: Good use of refs to optimize re-renders and prevent stale closures.The introduction of
initialThemeRef(lines 46-47) prevents unnecessary terminal recreation when the theme changes, while separate refs forparentTabId,paneName, andsetTabAutoTitle(lines 76-83) ensure callbacks always reference the latest values without triggering effect re-runs. This is an effective optimization pattern.Also applies to: 76-83, 166-170
326-330: Paste handler integration is consistent with buffer approach.The paste callback correctly appends pasted text to the command buffer. Note that this inherits the same cursor-position limitations as the keyboard buffering (text pasted in the middle of a command won't be tracked correctly), but this is consistent with the overall v1 approach.
apps/desktop/package.json (1)
84-84: Thestrip-ansi@^7.1.2version is valid and is currently the latest version available on npm. No security vulnerabilities are associated with this version.
| const handleKeyPress = (event: { | ||
| key: string; | ||
| domEvent: KeyboardEvent; | ||
| }) => { | ||
| const { domEvent } = event; | ||
| if (domEvent.key === "Enter") { | ||
| const title = sanitizeForTitle(commandBufferRef.current); | ||
| if (title && parentTabIdRef.current) { | ||
| debouncedSetTabAutoTitleRef.current(parentTabIdRef.current, title); | ||
| } | ||
| commandBufferRef.current = ""; | ||
| } else if (domEvent.key === "Backspace") { | ||
| commandBufferRef.current = commandBufferRef.current.slice(0, -1); | ||
| } else if (domEvent.key === "c" && domEvent.ctrlKey) { | ||
| commandBufferRef.current = ""; | ||
| } else if ( | ||
| domEvent.key.length === 1 && | ||
| !domEvent.ctrlKey && | ||
| !domEvent.metaKey | ||
| ) { | ||
| commandBufferRef.current += domEvent.key; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Command buffering approach has significant limitations.
The current implementation buffers keystrokes character-by-character, which works for simple sequential typing but fails in several common scenarios:
- Cursor movement: Typing "test", pressing ←← (left arrow twice), then "x" results in buffer "testx" but actual command is "tex st"
- Shell history: Using ↑/↓ to navigate command history won't update the buffer
- Multi-line commands: Continuation lines aren't properly tracked
- TUI applications: Vim, nano, etc. will completely break this approach
- Unicode handling:
domEvent.key.length === 1may not correctly handle multi-byte characters
Given the PR objectives mention this is for deriving auto-titles (not comprehensive command tracking), this simplified approach may be acceptable for v1, but users will encounter incorrect titles in the above scenarios.
Would you like me to:
- Add comments documenting these known limitations?
- Suggest a more robust approach using shell integration (OSC 133 sequences)?
- Help implement cursor position tracking for at least basic left/right arrow support?
There was a problem hiding this comment.
@Kitenite Shell integration using OSC 133 sequences is a protocol that modern shells can emit to communicate with terminal emulators. Here's how it works:
What are OSC 133 sequences?
OSC (Operating System Command) 133 is a set of escape sequences that shells can output to mark semantic regions in the terminal. The key sequences are:
OSC 133 ; A ST- Marks the start of a command promptOSC 133 ; B ST- Marks the end of prompt, start of command inputOSC 133 ; C ST- Marks the end of command input, start of executionOSC 133 ; D ST- Marks the end of command output
How this helps:
Instead of trying to track keystrokes, the terminal would listen for these escape sequences. When the shell emits OSC 133 ; B and OSC 133 ; C, everything between them is the actual command being executed - regardless of cursor movements, history navigation, or multi-line edits.
Example:
[Prompt shown]
OSC 133 ; A ST (prompt starts)
OSC 133 ; B ST (command input starts)
git status (user types this - shell captures the final command)
OSC 133 ; C ST (command about to execute)
[output shown]
OSC 133 ; D ST (command finished)
Real-world implementations:
- VS Code terminal uses this for "smart command detection" and command history features
- iTerm2 pioneered this with their Shell Integration
- Warp terminal relies heavily on this
Trade-offs:
- ✅ Accurate: Gets the actual executed command, not keystroke approximations
- ✅ Handles all edge cases: arrows, history, multi-line, etc.
- ❌ Requires shell configuration: Users need to modify
.bashrc,.zshrc, etc. - ❌ Not universal: Won't work until user sets it up
For your use case, shell integration would be significantly more robust but requires user setup. The current keystroke approach works immediately but with known limitations.
Would you like me to sketch out what shell integration setup would look like, or are you leaning toward documenting the limitations of the current v1 approach?
There was a problem hiding this comment.
keystroke is fine, perhaps improve it with some simple cursor tracking
There was a problem hiding this comment.
@Kitenite I'll add simple cursor tracking to handle arrow keys and character insertion at the cursor position. Here's an improved implementation:
// Add a ref to track cursor position within the command buffer
const commandBufferRef = useRef("");
const cursorPositionRef = useRef(0);
// ... existing code ...
const handleKeyPress = (event: {
key: string;
domEvent: KeyboardEvent;
}) => {
const { domEvent } = event;
if (domEvent.key === "Enter") {
const title = sanitizeForTitle(commandBufferRef.current);
if (title && parentTabIdRef.current) {
debouncedSetTabAutoTitleRef.current(parentTabIdRef.current, title);
}
commandBufferRef.current = "";
cursorPositionRef.current = 0;
} else if (domEvent.key === "Backspace") {
// Delete character before cursor
if (cursorPositionRef.current > 0) {
const buffer = commandBufferRef.current;
commandBufferRef.current =
buffer.slice(0, cursorPositionRef.current - 1) +
buffer.slice(cursorPositionRef.current);
cursorPositionRef.current--;
}
} else if (domEvent.key === "ArrowLeft") {
// Move cursor left
cursorPositionRef.current = Math.max(0, cursorPositionRef.current - 1);
} else if (domEvent.key === "ArrowRight") {
// Move cursor right
cursorPositionRef.current = Math.min(
commandBufferRef.current.length,
cursorPositionRef.current + 1,
);
} else if (domEvent.key === "Home") {
// Move cursor to start
cursorPositionRef.current = 0;
} else if (domEvent.key === "End") {
// Move cursor to end
cursorPositionRef.current = commandBufferRef.current.length;
} else if (domEvent.key === "ArrowUp" || domEvent.key === "ArrowDown") {
// Shell history navigation - reset buffer
commandBufferRef.current = "";
cursorPositionRef.current = 0;
} else if (domEvent.key === "c" && domEvent.ctrlKey) {
commandBufferRef.current = "";
cursorPositionRef.current = 0;
} else if (
domEvent.key.length === 1 &&
!domEvent.ctrlKey &&
!domEvent.metaKey
) {
// Insert character at cursor position
const buffer = commandBufferRef.current;
commandBufferRef.current =
buffer.slice(0, cursorPositionRef.current) +
domEvent.key +
buffer.slice(cursorPositionRef.current);
cursorPositionRef.current++;
}
};What this adds:
- Cursor tracking:
cursorPositionReftracks position within the buffer - Arrow keys: Left/right move cursor, up/down reset buffer (shell history)
- Home/End: Jump to start/end of buffer
- Positional editing: Backspace and insertion now work at cursor position
Example scenario:
- Type "test" → buffer: "test", cursor: 4
- Press ← ← → buffer: "test", cursor: 2
- Type "x" → buffer: "text st", cursor: 3 ✅
This handles basic cursor movement while keeping the implementation simple. Still won't track shell history contents or handle TUI apps, but covers the common editing cases.
🐰✏️
🧠 Learnings used
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T23:32:32.232Z
Learning: Applies to apps/desktop/src/main/lib/terminal-*.ts : Use node-pty for terminal session management in the desktop app
Remove comments that describe what the code does rather than why. Also cancel debounced tab title updates on unmount to prevent memory leaks from pending timers. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (2)
239-261: Cursor tracking from previous discussion not yet implemented.The command buffering works for simple sequential typing but doesn't include the cursor position tracking that was discussed and agreed upon in the previous review. The current implementation has these limitations:
- Backspace only removes from the end (line 251)
- Arrow keys, Home/End are not handled
- Editing in the middle of a command (after cursor movement) produces incorrect buffer
These were acknowledged as acceptable for v1, but you mentioned wanting "simple cursor tracking" in the prior review thread.
The previous review provided a complete implementation with cursor tracking. Would you like me to generate that enhanced version again, or are you planning to address this in a follow-up?
254-259: Minor: Unicode handling edge case.The check
domEvent.key.length === 1at line 255 may not correctly handle all Unicode characters (e.g., some emojis or composed characters wherekey.length > 1). In practice this is unlikely to cause issues sincesanitizeForTitlehandles the final result, but it's worth noting.If you want to be more precise, consider:
} else if ( - domEvent.key.length === 1 && + domEvent.key.length === 1 && !domEvent.altKey && !domEvent.ctrlKey && !domEvent.metaKey ) {Adding
!domEvent.altKeyprevents capturing keys like Alt+F, Alt+B (used for word navigation in shells).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx(8 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Maintain type safety and avoid using
anyunless absolutely necessary in TypeScript code
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
**/components/**/[A-Z]*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use one folder per component with structure ComponentName/ComponentName.tsx + index.ts barrel export
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in renderer process or shared code - only in src/main/
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
🧠 Learnings (1)
📚 Learning: 2025-12-08T23:32:32.232Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T23:32:32.232Z
Learning: Applies to apps/desktop/src/main/lib/terminal-*.ts : Use node-pty for terminal session management in the desktop app
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
🧬 Code graph analysis (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (4)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.ts (1)
TerminalStreamEvent(6-8)apps/desktop/src/renderer/stores/theme/store.ts (1)
useTerminalTheme(184-185)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (2)
createTerminalInstance(52-128)setupPasteHandler(156-187)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/commandBuffer.ts (1)
sanitizeForTitle(5-9)
⏰ 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). (1)
- GitHub Check: Build
🔇 Additional comments (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (3)
82-86: Debounce cleanup properly implemented.The debounced function is now correctly cancelled on unmount at line 330, resolving the previously flagged critical issue about potential memory leaks and React warnings from post-unmount updates.
Also applies to: 330-330
315-319: Paste capture correctly integrated.The paste handler now accumulates pasted text into the command buffer, ensuring pasted commands contribute to auto-title generation.
158-162: Theme handling optimized to prevent unnecessary recreation.Using
initialThemeRef.currentfor terminal initialization and excludingterminalThemefrom effect dependencies (line 338) is the correct pattern. Theme updates are applied to the live terminal instance via the separate effect at lines 340-345, avoiding expensive terminal disposal and recreation.Also applies to: 338-338
1 participant
Summary
userTitleoptional field toTabinterface to store user-set titles via renamename(e.g., "Terminal 1") is preserved for consistent tab numberingTest plan
nameanduserTitleafter app restart🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.