Conversation
…nimal settings (unchanged core) - Add mouse bindings: drag to select, release to copy to clipboard and exit copy-mode - Leave unrelated settings untouched to keep diff minimal - No renderer/xterm option changes
WalkthroughAdds renderer wheel-driven tmux history scrolling via a new IPC channel, implements tmux-manager methods to enter/exit copy-mode and bound scrolling, moves server-level history-limit application earlier, and sanitizes mouse-report sequences in emitted data. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Mouse Wheel / Keyboard)
participant Renderer as Renderer (Terminal.tsx)
participant IPC as Main Process IPC
participant TmuxMgr as Tmux Manager
User->>Renderer: wheel event
Renderer->>Renderer: check alt-buffer & drag-selection
alt eligible for history scroll
Renderer->>IPC: terminal-scroll-lines { id, amount }
IPC->>TmuxMgr: scrollLines(sid, amount)
TmuxMgr->>TmuxMgr: enter copy-mode & send-keys (bounded)
TmuxMgr-->>Renderer: (emits data/events as usual)
else not eligible
Renderer-->>User: default scroll handling
end
%% input arriving should finish scroll/copy-mode before being written
User->>Renderer: keyboard input
Renderer->>IPC: terminal-input { id, ... }
IPC->>TmuxMgr: scrollFinish(sid) %% called before writing input
TmuxMgr->>TmuxMgr: exit copy-mode
IPC->>TmuxMgr: write input to tmux
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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). (4)
Comment |
…election bindings
…stom wheel -> tmux copy-mode scroll; tmux mouse off
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/tmux-manager.ts (1)
183-228: Consider aligning session and server history limits.The session sets
history-limitto200000(line 192), butapplyServerSettings()sets both server and global limits to1000000. Since tmux's hierarchy means session settings take precedence for that session, the server limit of 1M won't be effective if sessions cap at 200k. Consider either:
- Increasing the session limit to match the server (1000000), or
- Documenting why different limits are intentional
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/main/lib/terminal-ipcs.ts(2 hunks)apps/desktop/src/main/lib/tmux-manager.ts(3 hunks)apps/desktop/src/renderer/screens/main/components/MainContent/Terminal.tsx(2 hunks)apps/desktop/src/shared/ipc-channels/terminal.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/desktop/src/shared/ipc-channels/terminal.ts (2)
apps/desktop/src/shared/ipc-channels/types.ts (1)
NoResponse(22-22)apps/desktop/src/shared/ipc-channels/index.ts (1)
NoResponse(21-21)
🔇 Additional comments (7)
apps/desktop/src/shared/ipc-channels/terminal.ts (1)
43-47: LGTM!The new IPC channel follows the established pattern and the type signature matches the implementation in both the IPC handler and the frontend caller.
apps/desktop/src/main/lib/terminal-ipcs.ts (1)
87-98: LGTM!The handler correctly invokes the
tmuxManager.scrollLines()method with the provided terminal ID and scroll amount.apps/desktop/src/main/lib/tmux-manager.ts (4)
96-98: LGTM!Applying server-wide settings before loading sessions ensures global configuration is in place before any session-specific operations.
230-261: LGTM!The server settings are applied correctly with appropriate error handling. The try-catch blocks ensure that failures in setting global configuration don't crash the initialization process.
648-677: LGTM!The mouse sequence sanitization correctly filters out X11 mouse reporting enable codes while preserving other terminal sequences. The try-catch ensures robustness if the regex processing fails.
679-701: LGTM!The scroll implementation correctly:
- Bounds the scroll amount to a reasonable range (1-100 lines)
- Uses tmux's copy-mode for history navigation
- Efficiently repeats the scroll command with the
-Nflag- Handles errors gracefully without crashing
apps/desktop/src/renderer/screens/main/components/MainContent/Terminal.tsx (1)
164-166: No issues found — xterm.js options verified as valid in v5.5.0.Both
macOptionClickForcesSelectionandrightClickSelectsWordare validITerminalOptionsin @xterm/xterm 5.5.0. The options are correctly implemented to provide native macOS-like terminal selection behavior.
| export function registerTerminalIPCs(window: BrowserWindowType) { | ||
| // Initialize tmux manager (restore sessions) only once | ||
| if (!ipcHandlersRegistered) { | ||
| if (!ipcHandlersRegistered) { |
There was a problem hiding this comment.
Fix indentation to use tabs consistently.
The indentation uses spaces instead of tabs, inconsistent with the rest of the file.
Apply this diff:
- if (!ipcHandlersRegistered) {
+ if (!ipcHandlersRegistered) {📝 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.
| if (!ipcHandlersRegistered) { | |
| if (!ipcHandlersRegistered) { |
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/terminal-ipcs.ts around line 13, the indentation
for the "if (!ipcHandlersRegistered) {" line uses spaces instead of the file's
expected tabs; change the leading spaces to a single tab (or match the project's
tab-level) so the line is indented with tabs consistently with the rest of the
file.
| // Attach custom wheel handler to scroll tmux history when in normal buffer | ||
| try { | ||
| term.attachCustomWheelEventHandler((ev: WheelEvent) => { | ||
| // If alternate buffer (full-screen app), let xterm transform wheel to keys | ||
| const isAlternate = (term as any)?.buffer?.active?.type === "alternate"; | ||
| if (isAlternate) return true; // allow default handling | ||
|
|
||
| // If user is selecting (mouse button down), don't hijack | ||
| if ((ev.buttons & 1) === 1) return true; | ||
|
|
||
| // Convert delta to line count (heuristic) | ||
| const lines = Math.max(1, Math.abs(Math.round(ev.deltaY / 40))); | ||
| const amount = ev.deltaY > 0 ? lines : -lines; | ||
| if (terminalIdRef.current && amount !== 0) { | ||
| window.ipcRenderer.send("terminal-scroll-lines", { | ||
| id: terminalIdRef.current, | ||
| amount, | ||
| }); | ||
| // Prevent default so viewport/xterm doesn't fight with tmux copy-mode | ||
| return false; | ||
| } | ||
| return true; | ||
| }); | ||
| } catch {} |
There was a problem hiding this comment.
🧩 Analysis chain
Verify behavior across platforms and document internal API usage.
The wheel handler implementation looks correct, but note:
-
Internal API access (line 264): Accessing
(term as any)?.buffer?.active?.typerelies on xterm.js internals that may change between versions. While the optional chaining provides safety, consider documenting this dependency or exploring public APIs liketerm.modesif available. -
Delta-to-line conversion (line 271): The heuristic of 40 pixels per line may vary across different platforms, mice, and trackpads. Consider testing on Windows and Linux to ensure consistent scrolling behavior.
Run the following to check if xterm.js 5.5.0 provides a public API for buffer type detection:
🏁 Script executed:
#!/bin/bash
# Search for public buffer-related APIs in xterm typings
rg -nP --type=ts -C3 'buffer.*alternate|modes.*application' node_modules/@xterm/xterm/Length of output: 66
Then I'll search the web for xterm.js documentation to verify the API concerns.
🏁 Script executed:
#!/bin/bash
# Find xterm version in package.json
fd -n "package.json" --exec grep -l "xterm" {} \; | head -5 | xargs cat | grep -A2 -B2 "@xterm/xterm"Length of output: 230
🌐 Web query:
xterm.js 5.x public API alternate buffer detection
💡 Result:
Yes — in xterm.js 5.x you can detect the alternate buffer via the public Terminal.buffers API:
- Check current buffer: terminal.buffers.active.type === 'alternate'
- Listen for changes: terminal.buffers.onBufferChange(listener) and inspect the passed buffer.type
Minimal examples:
-
synchronous check:
const isAlt = terminal.buffers.active.type === 'alternate'; -
subscribe to changes:
const disp = terminal.buffers.onBufferChange(buf => {
const isAlt = buf.type === 'alternate';
// ...
});
Docs: IBufferNamespace / IBuffer (active, alternate, type, onBufferChange). [1][2]
Sources
[1] xterm.js API — IBufferNamespace (active, alternate, onBufferChange).
[2] xterm.js API — IBuffer (type: "normal" | "alternate").
Use xterm.js public API instead of internal buffer access.
The concern about internal API usage is valid. xterm.js 5.x provides a public Terminal.buffers API for detecting the alternate buffer, accessible via terminal.buffers.active.type, eliminating the need for type casting.
Update line 264:
const isAlternate = term.buffers.active?.type === "alternate";This replaces the internal (term as any)?.buffer?.active?.type access with the public API.
Delta-to-line heuristic (line 271): The 40 pixels-per-line conversion needs platform-specific testing on Windows and Linux to validate consistency across different input devices—this cannot be verified in the sandbox environment.
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/screens/main/components/MainContent/Terminal.tsx
around lines 260 to 283, replace the internal buffer access `(term as
any)?.buffer?.active?.type` with the xterm.js public API
`term.buffers.active?.type` to detect the alternate buffer; update the condition
at line ~264 to use that public property and keep the rest of the wheel handler
logic intact; do not introduce any new internal casts and ensure TypeScript
types align with xterm's Terminal type (add a narrow type assertion only if
absolutely required).
…ative xterm selection\n\n- mouse off in tmux; strip mouse-enable seqs to prevent xterm mouse mode\n- display-time 0 to suppress position overlay\n- add terminal-scroll-finish IPC; debounce exit from copy-mode\n- preserve deep scrollback via server/global settings
…ut instead of wheel timeout
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/lib/tmux-manager.ts (1)
639-645: Sanitize mouse-enable sequences for cached history as well, not just live data — This issue is valid and confirmed.The code shows the gap:
addTerminalMessagestores rawdatadirectly insession.outputHistoryemitMessagesanitizes withsanitizeMouseSeqonly for live emission to windowsgetHistoryreturns unsanitizedoutputHistoryOn reattach after restart, any replayed history containing escape sequences like
\x1b[?1000hwill reach xterm without filtering, potentially enabling mouse mode. The suggested fix—extractingsanitizeMouseSeqto a class-level helper and sanitizing before storing in history—correctly addresses the root cause.
♻️ Duplicate comments (1)
apps/desktop/src/renderer/screens/main/components/MainContent/Terminal.tsx (1)
260-294: Avoid internal xterm buffer access in the wheel handlerThe wheel-to-tmux scrolling flow looks solid, but:
const isAlternate = (term as any)?.buffer?.active?.type === "alternate";depends on xterm internals andany. xterm 5.x exposes a publicbuffersAPI that you can use instead.- The empty
catch {}will silently hide any failure to attach the wheel handler, which could make debugging harder.You can rely on the public API and keep the logic otherwise unchanged:
- term.attachCustomWheelEventHandler((ev: WheelEvent) => { - // If alternate buffer (full-screen app), let xterm transform wheel to keys - const isAlternate = (term as any)?.buffer?.active?.type === "alternate"; + term.attachCustomWheelEventHandler((ev: WheelEvent) => { + // If alternate buffer (full-screen app), let xterm transform wheel to keys + const isAlternate = term.buffers?.active?.type === "alternate"; if (isAlternate) return true; // allow default handling @@ - }); - } catch {} + }); + } catch (e) { + console.warn("Failed to attach wheel handler:", e); + }This removes the internal access and gives you at least a lightweight log if the handler cannot be attached.
🧹 Nitpick comments (2)
apps/desktop/src/main/lib/tmux-manager.ts (2)
183-231: Per-sessionhistory-limitlikely overrides the new server/global limitIn
applySessionSettingsyou still set:["history-limit", "200000"],while
applyServerSettingssets both server and globalhistory-limitto1000000. In tmux, a session-specifichistory-limitwill override the global setting, so effective history will remain at 200k for these sessions.If your goal is “maximized scrollback via server/global limit”, consider either:
- Dropping the per-session
history-limitentirely, or- Bumping it to match the server/global cap and adjusting the comment so it’s clear which one is authoritative.
For example, to delegate entirely to server/global:
- ["history-limit", "200000"],This keeps the per-session config focused on invisibility (status, titles, mouse, display-time, etc.) while letting the higher history limit do its job.
681-712: Scroll/cancel implementation looks solid; just keep an eye on spawnSync frequencyThe
scrollLines/scrollFinishpair is nicely constrained:
amountis clamped to 1–100 lines and truncated, which prevents pathological scroll values.- Positive vs negative mapping (
scroll-down/scroll-up) matches how you deriveamountfrom the wheel event.copy-mode -eis idempotent andcancelis safe to send even when not in copy-mode.The only thing to watch is that each wheel event triggers two
spawnSync("tmux", …)calls from the main process. If you notice UI stutter under heavy wheel usage, a future optimization could be to batch/coalesce scroll amounts or switch to an asyncspawn-based flow, but this is reasonable as a first implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/main/lib/terminal-ipcs.ts(2 hunks)apps/desktop/src/main/lib/tmux-manager.ts(3 hunks)apps/desktop/src/renderer/screens/main/components/MainContent/Terminal.tsx(2 hunks)apps/desktop/src/shared/ipc-channels/terminal.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/main/lib/terminal-ipcs.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/desktop/src/shared/ipc-channels/terminal.ts (2)
apps/desktop/src/shared/ipc-channels/index.ts (1)
NoResponse(21-21)apps/desktop/src/shared/ipc-channels/types.ts (1)
NoResponse(22-22)
🔇 Additional comments (2)
apps/desktop/src/renderer/screens/main/components/MainContent/Terminal.tsx (1)
164-172: Xterm selection options match the “native-feeling” goal
macOptionClickForcesSelection: trueandrightClickSelectsWord: trueare a good fit for macOS-style selection and Cmd+C copy. Just be aware thatrightClickSelectsWordmeans xterm will consume right-click instead of a context menu, so double-check that’s the intended UX in this terminal view.apps/desktop/src/shared/ipc-channels/terminal.ts (1)
43-53: Scroll IPC channel contracts look consistentThe
terminal-scroll-linesandterminal-scroll-finishchannel shapes match the tmux-manager methods (id + amount / id-only) and are consistent with existing string-based requests liketerminal-detach. No issues from a typing or API-shape perspective.
| async initialize(): Promise<void> { | ||
| // Ensure server-level settings (affect all sessions/panes) | ||
| this.applyServerSettings(); | ||
|
|
||
| const savedSessions = this.loadSessionsFromDisk(); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Make initialize synchronous again and ensure server settings apply with minimal config
Two related points here:
initializeisasyncbut neverawaits
The body is entirely synchronous. Marking itasyncchanges the public type toPromise<void>without a behavioral need and may force call sites to be updated. Unless you plan to addawaithere, it’s safer to keep this method synchronous:
- async initialize(): Promise<void> {
+ initialize(): void {
// Ensure server-level settings (affect all sessions/panes)
this.applyServerSettings();
@@
- const savedSessions = this.loadSessionsFromDisk();
+ const savedSessions = this.loadSessionsFromDisk();applyServerSettingsmay not do what you expect on a fresh server and may break “tmux invisibility”- If no server is running on
TMUX_SOCKET,tmux set … history-limitcan fail with “no server running” (non‑zero exit) and be silently ignored sincespawnSyncdoesn’t throw. - If
setdoes start a server, it will use the default config (user’s~/.tmux.conf), whereas everywhere else you intentionally use-f /dev/nullto keep tmux invisible and unconfigured.
- If no server is running on
Consider explicitly starting the server with your minimal config and checking exit codes:
- private applyServerSettings(): void {
- try {
- // Server-level hard cap for history size
- spawnSync("tmux", [
- "-L",
- this.TMUX_SOCKET,
- "set",
- "-s",
- "history-limit",
- "1000000",
- ]);
- } catch (e) {
- console.warn("[TmuxManager] Failed to set server history-limit", e);
- }
-
- try {
- // Global default for future windows/panes
- spawnSync("tmux", [
- "-L",
- this.TMUX_SOCKET,
- "set",
- "-g",
- "history-limit",
- "1000000",
- ]);
- } catch (e) {
- console.warn("[TmuxManager] Failed to set global history-limit", e);
- }
- }
+ private applyServerSettings(): void {
+ // Ensure server exists and uses our minimal config
+ const start = spawnSync("tmux", [
+ "-f",
+ "/dev/null",
+ "-L",
+ this.TMUX_SOCKET,
+ "start-server",
+ ]);
+ if (start.status !== 0) {
+ console.warn(
+ "[TmuxManager] Failed to start tmux server:",
+ start.stderr?.toString?.() ?? start.status,
+ );
+ return;
+ }
+
+ const server = spawnSync("tmux", [
+ "-L",
+ this.TMUX_SOCKET,
+ "set",
+ "-s",
+ "history-limit",
+ "1000000",
+ ]);
+ if (server.status !== 0) {
+ console.warn(
+ "[TmuxManager] Failed to set server history-limit:",
+ server.stderr?.toString?.() ?? server.status,
+ );
+ }
+
+ const globalOpt = spawnSync("tmux", [
+ "-L",
+ this.TMUX_SOCKET,
+ "set",
+ "-g",
+ "history-limit",
+ "1000000",
+ ]);
+ if (globalOpt.status !== 0) {
+ console.warn(
+ "[TmuxManager] Failed to set global history-limit:",
+ globalOpt.stderr?.toString?.() ?? globalOpt.status,
+ );
+ }
+ }This keeps tmux on the custom socket using /dev/null config and guarantees the history limits are actually applied.
Also applies to: 232-263
I'll verify the review comment by examining the actual code. Let me gather the necessary information.
🏁 Script executed:
#!/bin/bash
# Examine the tmux-manager.ts file to verify the concerns
# 1. Show the initialize method around lines 96-101
echo "=== initialize() method (lines 96-101) ==="
sed -n '90,110p' apps/desktop/src/main/lib/tmux-manager.ts
echo ""
echo "=== applyServerSettings() method (lines 232-263) ==="
sed -n '225,270p' apps/desktop/src/main/lib/tmux-manager.ts
echo ""
echo "=== Check for initialize method signature ==="
grep -n "initialize\s*(" apps/desktop/src/main/lib/tmux-manager.ts | head -5
echo ""
echo "=== Search for await statements in initialize ==="
awk '/^\s*(async\s+)?initialize\s*\(/,/^\s*\}/' apps/desktop/src/main/lib/tmux-manager.ts | grep -i awaitLength of output: 1740
🏁 Script executed:
#!/bin/bash
# Find all call sites of initialize() method
echo "=== Call sites of .initialize() ==="
rg -n "\.initialize\s*\(" apps/desktop/src/ --type ts --type tsx -C 2
echo ""
echo "=== Check for async/await usage with initialize ==="
rg -n "await.*\.initialize|initialize\s*\(\)" apps/desktop/src/ --type ts --type tsx -C 1Length of output: 144
🏁 Script executed:
#!/bin/bash
# Find call sites of initialize() using simpler pattern
echo "=== Call sites of .initialize() ==="
rg -n "\.initialize\s*\(" apps/desktop/src/ --type ts -C 2
echo ""
echo "=== Check if initialize is awaited ==="
rg -n "await.*\.initialize" apps/desktop/src/ --type ts
echo ""
echo "=== Look for tmuxManager instantiation and usage ==="
rg -n "new.*TmuxManager|tmuxManager\s*=" apps/desktop/src/ --type ts -C 3Length of output: 4014
🏁 Script executed:
#!/bin/bash
# Check the full initialize method for any await calls
echo "=== Full initialize method body ==="
sed -n '96,130p' apps/desktop/src/main/lib/tmux-manager.tsLength of output: 847
Remove async from initialize() and fix applyServerSettings() to check exit codes and use proper config
Two confirmed issues:
initialize()markedasyncbut contains noawaitstatements
The method body is entirely synchronous. The call site atapps/desktop/src/main/lib/terminal-ipcs.ts:14uses.catch()but does notawait, leaving the promise unhandled. Remove the async/Promise wrapper:
- async initialize(): Promise<void> {
+ initialize(): void {
// Ensure server-level settings (affect all sessions/panes)
this.applyServerSettings();
const savedSessions = this.loadSessionsFromDisk();applyServerSettings()silently ignoresspawnSyncexit codes and lacks config isolation
The try/catch only catches exceptions, not command failures.spawnSyncreturns its status in the.statusfield, which is never checked. Additionally, without-f /dev/null, a missing server will load user config (~/.tmux.conf). Update to explicitly start the server with minimal config and validate all exit codes:
private applyServerSettings(): void {
- try {
- // Server-level hard cap for history size
- spawnSync("tmux", [
- "-L",
- this.TMUX_SOCKET,
- "set",
- "-s",
- "history-limit",
- "1000000",
- ]);
- } catch (e) {
- console.warn("[TmuxManager] Failed to set server history-limit", e);
- }
-
- try {
- // Global default for future windows/panes
- spawnSync("tmux", [
- "-L",
- this.TMUX_SOCKET,
- "set",
- "-g",
- "history-limit",
- "1000000",
- ]);
- } catch (e) {
- console.warn("[TmuxManager] Failed to set global history-limit", e);
- }
+ // Ensure server exists with minimal config
+ const start = spawnSync("tmux", [
+ "-f",
+ "/dev/null",
+ "-L",
+ this.TMUX_SOCKET,
+ "start-server",
+ ]);
+ if (start.status !== 0) {
+ console.warn(
+ "[TmuxManager] Failed to start tmux server:",
+ start.stderr?.toString?.() ?? start.status,
+ );
+ return;
+ }
+
+ const server = spawnSync("tmux", [
+ "-L",
+ this.TMUX_SOCKET,
+ "set",
+ "-s",
+ "history-limit",
+ "1000000",
+ ]);
+ if (server.status !== 0) {
+ console.warn(
+ "[TmuxManager] Failed to set server history-limit:",
+ server.stderr?.toString?.() ?? server.status,
+ );
+ }
+
+ const globalOpt = spawnSync("tmux", [
+ "-L",
+ this.TMUX_SOCKET,
+ "set",
+ "-g",
+ "history-limit",
+ "1000000",
+ ]);
+ if (globalOpt.status !== 0) {
+ console.warn(
+ "[TmuxManager] Failed to set global history-limit:",
+ globalOpt.stderr?.toString?.() ?? globalOpt.status,
+ );
+ }
}Committable suggestion skipped: line range outside the PR's diff.
Kitenite
force-pushed
the
feat/tmux-mouse-selection
branch
3 times, most recently
from
4f9f3c3 to
2e962f0
Compare
1 participant
Summary by CodeRabbit