fix(desktop): Codex CLI terminal の入力遅延と描画不安定を改善#382
Merged
Conversation
PTY→IPC間の二重16msバッファを除去。renderer側のrAF（≈16ms）が 律速として残るため描画上限は変わらず、体感遅延のみ改善される。
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 19 minutes and 9 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughデスクトップ端末の不安定性を解決するため、出力フラッシュ間隔をゼロに変更し、端末のオープンを延期実行に切り替え、RAF書き込みバッファリングを削除し、端末クエリ抑制を強化しました。 Changes
Sequence Diagram(s)sequenceDiagram
participant App as アプリケーション
participant Terminal as Terminal.tsx
participant Helpers as helpers.ts
participant Cache as v1-terminal-cache.ts
participant XTerm as XTerm
App->>Terminal: Terminal コンポーネント初期化
Terminal->>Helpers: createTerminalInWrapper() 呼び出し
Helpers->>Cache: 端末エントリ作成
Cache->>XTerm: xterm インスタンス作成
Helpers-->>Terminal: openOnce() メソッド返却
Terminal->>Helpers: openOnce() 実行
Helpers->>XTerm: xterm.open(wrapper) 実行
XTerm-->>Helpers: オープン完了
Helpers->>Helpers: WebGL初期化スケジュール
Terminal->>Helpers: useEffect でxtermオプション適用
Helpers->>XTerm: xterm.options 設定
App->>Cache: ストリームデータ受信
Cache->>XTerm: write(data) 直接実行
XTerm-->>App: 出力レンダリング
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
- terminal.ts: resize に追加した不要なデバッグログを削除 - daemon-manager.ts: このPRで誤って削除した既存ログを全て復元 (DEBUG_TERMINAL インポート / データ受信 / supersede / createOrAttach呼び出し / reset開始・完了) - session.ts: 調査用に追加した定数・関数・メソッド・呼び出し箇所を全削除 (DEBUG_TERMINAL_FLOW 等の定数 / summarizeTerminalData / shouldDebugFlow / debugFlow / subprocess env vars の過剰な伝播 / 各所の debugFlow 呼び出し) BROADCAST_COALESCE_INTERVAL_MS=0 の変更は維持
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/main/terminal-host/session.ts (1)
486-510:⚠️ Potential issue | 🟠 MajorDEBUG 無効時のホットパス集計を避けてください。
debugFlow()の中で gate していても、引数のsummarizeTerminalData()/JSON.stringify()は呼び出し前に評価されます。PTY data/broadcast の hot path なので、debug 有効時だけ summary を作ってください。修正案
case PtySubprocessIpcType.Data: { if (payload.length === 0) break; let data = payload.toString("utf8"); - const rawSummary = summarizeTerminalData(data); + const shouldDebug = this.shouldDebugFlow(); + const rawSummary = shouldDebug ? summarizeTerminalData(data) : null; @@ this.enqueueEmulatorWrite(data); this.queueBroadcastData(data); - this.debugFlow("subprocess-data", { - raw: rawSummary, - postStrip: summarizeTerminalData(data), - emulatorQueuedBytes: this.emulatorWriteQueuedBytes, - pendingBroadcastBytes: this.pendingBroadcastBytes, - clientCount: this.attachedClients.size, - }); + if (shouldDebug) { + this.debugFlow("subprocess-data", { + raw: rawSummary, + postStrip: summarizeTerminalData(data), + emulatorQueuedBytes: this.emulatorWriteQueuedBytes, + pendingBroadcastBytes: this.pendingBroadcastBytes, + clientCount: this.attachedClients.size, + }); + } break; } @@ private queueBroadcastData(data: string): void { if (data.length === 0) return; - this.debugFlow("queue-broadcast-data", { - data: summarizeTerminalData(data), - coalesceEnabled: BROADCAST_COALESCE_ENABLED, - pendingChunks: this.pendingBroadcastChunks.length, - pendingBytes: this.pendingBroadcastBytes, - }); + if (this.shouldDebugFlow()) { + this.debugFlow("queue-broadcast-data", { + data: summarizeTerminalData(data), + coalesceEnabled: BROADCAST_COALESCE_ENABLED, + pendingChunks: this.pendingBroadcastChunks.length, + pendingBytes: this.pendingBroadcastBytes, + }); + } @@ - this.debugFlow("flush-pending-broadcast-data", { - data: summarizeTerminalData(merged), - }); + if (this.shouldDebugFlow()) { + this.debugFlow("flush-pending-broadcast-data", { + data: summarizeTerminalData(merged), + }); + } @@ - this.debugFlow("broadcast-event", { - eventType, - clientCount: this.attachedClients.size, - messageBytes: Buffer.byteLength(message, "utf8"), - payloadType: payload.type, - payloadBytes: - payload.type === "data" - ? Buffer.byteLength(payload.data, "utf8") - : undefined, - }); + if (this.shouldDebugFlow()) { + this.debugFlow("broadcast-event", { + eventType, + clientCount: this.attachedClients.size, + messageBytes: Buffer.byteLength(message, "utf8"), + payloadType: payload.type, + payloadBytes: + payload.type === "data" + ? Buffer.byteLength(payload.data, "utf8") + : undefined, + }); + }Also applies to: 1284-1370
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/terminal-host/session.ts` around lines 486 - 510, The hot path is calling summarizeTerminalData (and the JSON work inside debugFlow args) unconditionally; wrap the expensive summary work so it only runs when debugging is enabled by checking the same debug flag used inside debugFlow before calling summarizeTerminalData or building the debug object. Specifically, in session.ts around the PtySubprocessIpcType.Data handling, compute rawSummary and postStrip only if debugFlow is enabled (e.g., call a helper like this.isDebugEnabled() or expose debugFlowEnabled) and otherwise pass nothing/skip building the debug payload; keep the existing behavior of enqueueEmulatorWrite(data), queueBroadcastData(data), scanForShellReady(this.scanState, data), and this.resolveShellReady as-is. Ensure any variables referenced by debugFlow are defined only when the debug branch runs to avoid unnecessary allocations.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/v1-terminal-cache.ts (1)
359-429:⚠️ Potential issue | 🟠 MajorDEBUG 無効時の write hot path 集計を避けてください。
debugRendererFlow()側で return しても、summarizeAnsi()を含む引数オブジェクトは先に評価されます。directxterm.write化した hot path なので、debug 有効時だけ summary/hex を作る形にしてください。修正案
export function scheduleWrite(paneId: string, data: string): void { const entry = cache.get(paneId); if (!entry) return; - const incomingSummary = summarizeAnsi(data); - debugRendererFlow(paneId, "schedule-write", { - incoming: incomingSummary, - activeBufferType: entry.xterm.buffer.active.type, - }); - debugRendererHex(paneId, "schedule-write-chunk", data, { - activeBufferType: entry.xterm.buffer.active.type, - }); + if (shouldDebugPane(FLOW_DEBUG_PANE_ID_KEY, paneId)) { + debugRendererFlow(paneId, "schedule-write", { + incoming: summarizeAnsi(data), + activeBufferType: entry.xterm.buffer.active.type, + }); + } + if (shouldDebugPane(HEX_DEBUG_PANE_ID_KEY, paneId)) { + debugRendererHex(paneId, "schedule-write-chunk", data, { + activeBufferType: entry.xterm.buffer.active.type, + }); + } @@ if (entry.eventHandler) { - debugRendererFlow(paneId, "route-event-to-handler", { - eventType: event.type, - dataBytes: event.type === "data" ? summarizeAnsi(event.data).bytes : 0, - }); + if (shouldDebugPane(FLOW_DEBUG_PANE_ID_KEY, paneId)) { + debugRendererFlow(paneId, "route-event-to-handler", { + eventType: event.type, + dataBytes: event.type === "data" ? summarizeAnsi(event.data).bytes : 0, + }); + } entry.eventHandler(event); return; } @@ if (event.type === "data") { - debugRendererFlow(paneId, "route-event-hidden-data", { - data: summarizeAnsi(event.data), - }); + if (shouldDebugPane(FLOW_DEBUG_PANE_ID_KEY, paneId)) { + debugRendererFlow(paneId, "route-event-hidden-data", { + data: summarizeAnsi(event.data), + }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/v1-terminal-cache.ts` around lines 359 - 429, The hot path is accidentally computing ANSI summaries because debugRendererFlow/debugRendererHex are called with objects that evaluate summarizeAnsi() even when DEBUG_TERMINAL is false; update scheduleWrite and routeEvent so that any summarizeAnsi()/debugRendererHex()/debugRendererFlow calls (and the dataBytes calculation in the entry.eventHandler branch) are only executed when debugging is enabled (e.g., wrap those debug calls and their summarizeAnsi() / summarizeHex logic in if (DEBUG_TERMINAL) or similar), leaving entry.xterm.write(data) and scheduleWrite/flushWrite behavior unchanged.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.ts (1)
49-49: OSC 4 の query 判定をより厳密にすることも検討可能です。
data.includes("?")は OSC 4 の set payload (Ps;rgb:rr/gg/bb) に?が含まれていないことを前提にしています。実運用では set 形式に?は現れないため通常は問題になりませんが、複数要素を連結した4;0;rgb:..;1;?のような混在パターンではリクエスト全体が抑止され set 部分が落ちます。気になるなら;?を末尾/セグメント単位で判定するか、/;\?(?:;|$)/のような正規表現に寄せると意図がより明確になります。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.ts` at line 49, The current OSC 4 handler registered via parser.registerOscHandler(4, ...) uses data.includes("?") which can falsely treat mixed payloads like "4;0;rgb:..;1;?" as a query and suppress the whole sequence; update the predicate to detect a standalone query segment instead (e.g., test for a "?"/";?" at segment boundaries using a regex like /(?:^|;)\\?(?:;|$)/ or split data on ';' and check for any element === '?' ) so that set payloads containing '?' inside values are not misclassified; keep the handler registration (parser.registerOscHandler) and disposal logic (disposables.push(...)) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/terminal-host/pty-subprocess.ts`:
- Line 11: When writing the raw PTY log use a secure file mode and don't let
write failures break the subprocess data path: wrap all uses of appendFileSync
and fsWrite in try/catch (locations where raw log files are opened/written in
pty-subprocess, including the other write sites mentioned) and on error log the
failure but do not close or drop the subprocess streams; when creating the raw
log file ensure it is created with permissions 0o600 (use the mode option or
open the file with that mode before writing) so secrets/inputs are protected.
- Around line 131-132: Replace the regex literals that trigger the lint rule in
the object properties scrollRegion and cursorMove: instead of /\x1b\[[0-9;]*r/
and /\x1b\[[0-9;]*H/, construct them with new RegExp(String.raw`...\`) using the
same patterns (e.g. new RegExp(String.raw`\x1b\[[0-9;]*r`) and new
RegExp(String.raw`\x1b\[[0-9;]*H`); update the properties (scrollRegion and
cursorMove) to use these RegExp instances so the lint rule
noControlCharactersInRegex is satisfied.
In `@apps/desktop/src/main/terminal-host/session.ts`:
- Around line 138-167: The Biome regex lint errors come from using control
characters in regex literals inside summarizeTerminalData; replace the three
problematic patterns (the data.match(/\x1b/g) call and the /\x1b\[[0-9;]*r/ and
/\x1b\[[0-9;]*H/ literals) with RegExp constructions that use String.raw to
encode the escape sequence, e.g. use new RegExp(String.raw`\x1b`, "g") for the
global match and new RegExp(String.raw`\x1b\[[0-9;]*r`) and new
RegExp(String.raw`\x1b\[[0-9;]*H`) for the two tests so the control character is
not embedded directly in a regex literal.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/config.ts`:
- Around line 17-20: The DEBUG_TERMINAL constant reads localStorage at module
import, which can throw in some environments; wrap the localStorage access
inside a try/catch in the DEBUG_TERMINAL evaluation so the exception is
swallowed and a safe fallback is used (false) if access fails. Specifically,
update the DEBUG_TERMINAL initialization to first check typeof localStorage in a
try block and catch any errors from localStorage.getItem, returning false on
error, so the module import never throws when computing DEBUG_TERMINAL.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalConnection.ts`:
- Around line 41-67: Declare an ESC constant at the top (e.g., const ESC =
"\x1b"), extract shared regexes into reusable RegExp instances using the RegExp
constructor with String.raw (e.g., CPR_RESPONSE_PATTERN = new
RegExp(String.raw`${ESC}\[[0-9]+;[0-9]+R`) and KITTY_KEYBOARD_PATTERN = new
RegExp(String.raw`${ESC}\[[0-9;:]+u`), update summarizeTerminalInput to use
data.split(ESC).length - 1 instead of data.match(/\x1b/g), replace all literal
regexes in summarizeTerminalInput and isInterestingTerminalInput with the new
CPR_RESPONSE_PATTERN and KITTY_KEYBOARD_PATTERN and other patterns constructed
via RegExp + String.raw (for focusIn/focusOut/bracketed paste use
String.raw`${ESC}[I` etc.), and reorder imports at the top to satisfy Biome
ordering rules.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/v1-terminal-cache.ts`:
- Around line 92-95: The helper shouldDebugPane currently reads
SUPERSET_TERMINAL_DEBUG_PANE_ID/HEX from localStorage via readDebugPaneId
without checking the master DEBUG_TERMINAL flag, so stored pane ids can
re-enable flow/hex logging even when DEBUG_TERMINAL is false; update
shouldDebugPane (and the similar logic around lines 142-164) to first check the
DEBUG_TERMINAL flag and immediately return false if it's falsy, then proceed to
call readDebugPaneId and compare to paneId (or "*") only when DEBUG_TERMINAL is
true; reference the function shouldDebugPane and the constants/keys
SUPERSET_TERMINAL_DEBUG_PANE_ID, HEX and the helper readDebugPaneId while making
this change.
- Around line 103-139: The summarizeAnsi function uses regex literals that
include the control character \x1b and trigger the noControlCharactersInRegex
lint rule; replace each regex literal containing \x1b with a RegExp created from
String.raw so the control char is not in a literal. Concretely, update patterns
in summarizeAnsi such as /\x1b\[[0-9;]*K/, /\x1b\[[0-9;]*r/, /\x1b\[[0-9;]*H/,
/\x1b\[[0-9;]*A/, /\x1b\[[0-9;]*B/, /\x1b\[[0-9;]*C/, /\x1b\[[0-9;]*D/,
/\x1b\[[0-9]+;[0-9]+R/, /\x1b\[[0-9;:]+u/ and the global match /\x1b/g (used in
escapes) to use new RegExp(String.raw`...`, flags) (e.g. new
RegExp(String.raw`\x1b\[[0-9;]*K`)) preserving any flags like 'g'. Ensure all
replacements keep the same semantics and flags.
In `@knowledge.md`:
- Around line 51-64: Add a one-line clarification in the section about hidden
tab stacks: state that the existing PersistentTabRenderer implementation
persisted only tabs containing persistent views (webview, vscode-extension,
reference-graph, file-viewer) while terminal-only tabs had hasPersistentView ===
false and were unmounted when inactive; reference PersistentTabRenderer and
TabsContent/PersistentTabRenderer.tsx so readers know the “hidden terminal”
behavior only applies when a terminal is embedded alongside a persistent pane.
---
Outside diff comments:
In `@apps/desktop/src/main/terminal-host/session.ts`:
- Around line 486-510: The hot path is calling summarizeTerminalData (and the
JSON work inside debugFlow args) unconditionally; wrap the expensive summary
work so it only runs when debugging is enabled by checking the same debug flag
used inside debugFlow before calling summarizeTerminalData or building the debug
object. Specifically, in session.ts around the PtySubprocessIpcType.Data
handling, compute rawSummary and postStrip only if debugFlow is enabled (e.g.,
call a helper like this.isDebugEnabled() or expose debugFlowEnabled) and
otherwise pass nothing/skip building the debug payload; keep the existing
behavior of enqueueEmulatorWrite(data), queueBroadcastData(data),
scanForShellReady(this.scanState, data), and this.resolveShellReady as-is.
Ensure any variables referenced by debugFlow are defined only when the debug
branch runs to avoid unnecessary allocations.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/v1-terminal-cache.ts`:
- Around line 359-429: The hot path is accidentally computing ANSI summaries
because debugRendererFlow/debugRendererHex are called with objects that evaluate
summarizeAnsi() even when DEBUG_TERMINAL is false; update scheduleWrite and
routeEvent so that any summarizeAnsi()/debugRendererHex()/debugRendererFlow
calls (and the dataBytes calculation in the entry.eventHandler branch) are only
executed when debugging is enabled (e.g., wrap those debug calls and their
summarizeAnsi() / summarizeHex logic in if (DEBUG_TERMINAL) or similar), leaving
entry.xterm.write(data) and scheduleWrite/flushWrite behavior unchanged.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.ts`:
- Line 49: The current OSC 4 handler registered via parser.registerOscHandler(4,
...) uses data.includes("?") which can falsely treat mixed payloads like
"4;0;rgb:..;1;?" as a query and suppress the whole sequence; update the
predicate to detect a standalone query segment instead (e.g., test for a
"?"/";?" at segment boundaries using a regex like /(?:^|;)\\?(?:;|$)/ or split
data on ';' and check for any element === '?' ) so that set payloads containing
'?' inside values are not misclassified; keep the handler registration
(parser.registerOscHandler) and disposal logic (disposables.push(...))
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83d8de65-5b41-41b3-8ee4-72d2e366ebfb
📒 Files selected for processing (15)
apps/desktop/electron.vite.config.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/main/lib/terminal/daemon/daemon-manager.tsapps/desktop/src/main/terminal-host/pty-subprocess.tsapps/desktop/src/main/terminal-host/session.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/config.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalConnection.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalStream.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/v1-terminal-cache.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxknowledge.md
💤 Files with no reviewable changes (1)
- apps/desktop/src/main/lib/terminal/daemon/daemon-manager.ts
Comment on lines
+17
to
+20
| export const DEBUG_TERMINAL = | ||
| typeof localStorage !== "undefined" && | ||
| localStorage.getItem("SUPERSET_TERMINAL_DEBUG") === "1"; | ||
| process.env.SUPERSET_TERMINAL_DEBUG === "1" || | ||
| (typeof localStorage !== "undefined" && | ||
| localStorage.getItem("SUPERSET_TERMINAL_DEBUG") === "1"); |
There was a problem hiding this comment.
localStorage 読み取りを try/catch で保護してください。
この module は import 時に評価されるため、storage access が例外を投げる環境だと terminal 画面全体が初期化に失敗します。
修正案
+function isLocalStorageDebugEnabled(): boolean {
+ try {
+ return (
+ typeof localStorage !== "undefined" &&
+ localStorage.getItem("SUPERSET_TERMINAL_DEBUG") === "1"
+ );
+ } catch {
+ return false;
+ }
+}
+
export const DEBUG_TERMINAL =
process.env.SUPERSET_TERMINAL_DEBUG === "1" ||
- (typeof localStorage !== "undefined" &&
- localStorage.getItem("SUPERSET_TERMINAL_DEBUG") === "1");
+ isLocalStorageDebugEnabled();📝 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.
Suggested change
| export const DEBUG_TERMINAL = | |
| typeof localStorage !== "undefined" && | |
| localStorage.getItem("SUPERSET_TERMINAL_DEBUG") === "1"; | |
| process.env.SUPERSET_TERMINAL_DEBUG === "1" || | |
| (typeof localStorage !== "undefined" && | |
| localStorage.getItem("SUPERSET_TERMINAL_DEBUG") === "1"); | |
| function isLocalStorageDebugEnabled(): boolean { | |
| try { | |
| return ( | |
| typeof localStorage !== "undefined" && | |
| localStorage.getItem("SUPERSET_TERMINAL_DEBUG") === "1" | |
| ); | |
| } catch { | |
| return false; | |
| } | |
| } | |
| export const DEBUG_TERMINAL = | |
| process.env.SUPERSET_TERMINAL_DEBUG === "1" || | |
| isLocalStorageDebugEnabled(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/config.ts`
around lines 17 - 20, The DEBUG_TERMINAL constant reads localStorage at module
import, which can throw in some environments; wrap the localStorage access
inside a try/catch in the DEBUG_TERMINAL evaluation so the exception is
swallowed and a safe fallback is used (false) if access fails. Specifically,
update the DEBUG_TERMINAL initialization to first check typeof localStorage in a
try block and catch any errors from localStorage.getItem, returning false on
error, so the module import never throws when computing DEBUG_TERMINAL.
Comment on lines
+41
to
+67
| function summarizeTerminalInput(data: string): Record<string, unknown> { | ||
| return { | ||
| bytes: new TextEncoder().encode(data).length, | ||
| chars: data.length, | ||
| escapes: (data.match(/\x1b/g) || []).length, | ||
| cprResponse: /\x1b\[[0-9]+;[0-9]+R/.test(data), | ||
| focusIn: data.includes("\x1b[I"), | ||
| focusOut: data.includes("\x1b[O"), | ||
| kittyKeyboard: /\x1b\[[0-9;:]+u/.test(data), | ||
| bracketedPasteStart: data.includes("\x1b[200~"), | ||
| bracketedPasteEnd: data.includes("\x1b[201~"), | ||
| preview: | ||
| JSON.stringify(data).length > 220 | ||
| ? `${JSON.stringify(data).slice(0, 220)}…` | ||
| : JSON.stringify(data), | ||
| }; | ||
| } | ||
|
|
||
| function isInterestingTerminalInput(data: string): boolean { | ||
| return ( | ||
| /\x1b\[[0-9]+;[0-9]+R/.test(data) || | ||
| data.includes("\x1b[I") || | ||
| data.includes("\x1b[O") || | ||
| /\x1b\[[0-9;:]+u/.test(data) || | ||
| data.includes("\x1b[200~") || | ||
| data.includes("\x1b[201~") | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file
fd -t f "useTerminalConnection.ts" apps/desktopRepository: MocA-Love/superset
Length of output: 189
🏁 Script executed:
# Check if file exists and view the relevant lines
if [ -f "apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalConnection.ts" ]; then
echo "=== File found, checking lines 41-67 ==="
sed -n '41,67p' "apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalConnection.ts"
else
echo "File not found at expected path"
fiRepository: MocA-Love/superset
Length of output: 969
🏁 Script executed:
# Check Biome configuration
if [ -f "biome.json" ] || [ -f ".biomerc.json" ]; then
echo "=== Biome config ==="
cat biome.json 2>/dev/null || cat .biomerc.json 2>/dev/null
fiRepository: MocA-Love/superset
Length of output: 44
🏁 Script executed:
# Test the specific file with Biome to see if lint error occurs
if command -v biome &> /dev/null; then
echo "=== Running Biome lint on the file ==="
biome check "apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalConnection.ts" 2>&1 | head -100
else
echo "Biome not available in PATH"
fiRepository: MocA-Love/superset
Length of output: 4767
Biome の noControlCharactersInRegex エラーを解消してください。
regex 形式の \x1b 制御文字が lint/suspicious/noControlCharactersInRegex として 6 箇所で検出されています（45, 46, 49, 61, 64 行目）。Biome は制御文字を含む regex literals を意図しない入力として拒否するため、提案の RegExp コンストラクタ + String.raw パターンで対応してください。
修正時に以下の点を確認してください:
- ESC定数を上部で宣言
- CPR_RESPONSE_PATTERN と KITTY_KEYBOARD_PATTERN を両関数で共用可能な形で抽出
data.match(/\x1b/g)をdata.split(ESC).length - 1に変更（機能同等で効率的）- Import 順序も Biome の要求に合わせて修正
修正案
+const ESC = "\x1b";
+const CPR_RESPONSE_PATTERN = new RegExp(
+ String.raw`\x1b\[[0-9]+;[0-9]+R`,
+);
+const KITTY_KEYBOARD_PATTERN = new RegExp(String.raw`\x1b\[[0-9;:]+u`);
+
function summarizeTerminalInput(data: string): Record<string, unknown> {
return {
bytes: new TextEncoder().encode(data).length,
chars: data.length,
- escapes: (data.match(/\x1b/g) || []).length,
- cprResponse: /\x1b\[[0-9]+;[0-9]+R/.test(data),
+ escapes: data.split(ESC).length - 1,
+ cprResponse: CPR_RESPONSE_PATTERN.test(data),
focusIn: data.includes("\x1b[I"),
focusOut: data.includes("\x1b[O"),
- kittyKeyboard: /\x1b\[[0-9;:]+u/.test(data),
+ kittyKeyboard: KITTY_KEYBOARD_PATTERN.test(data),
bracketedPasteStart: data.includes("\x1b[200~"),
bracketedPasteEnd: data.includes("\x1b[201~"),
@@
function isInterestingTerminalInput(data: string): boolean {
return (
- /\x1b\[[0-9]+;[0-9]+R/.test(data) ||
+ CPR_RESPONSE_PATTERN.test(data) ||
data.includes("\x1b[I") ||
data.includes("\x1b[O") ||
- /\x1b\[[0-9;:]+u/.test(data) ||
+ KITTY_KEYBOARD_PATTERN.test(data) ||
data.includes("\x1b[200~") ||
data.includes("\x1b[201~")
);🧰 Tools
🪛 Biome (2.4.11)
[error] 45-45: Unexpected control character in a regular expression.
(lint/suspicious/noControlCharactersInRegex)
[error] 46-46: Unexpected control character in a regular expression.
(lint/suspicious/noControlCharactersInRegex)
[error] 49-49: Unexpected control character in a regular expression.
(lint/suspicious/noControlCharactersInRegex)
[error] 61-61: Unexpected control character in a regular expression.
(lint/suspicious/noControlCharactersInRegex)
[error] 64-64: Unexpected control character in a regular expression.
(lint/suspicious/noControlCharactersInRegex)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalConnection.ts`
around lines 41 - 67, Declare an ESC constant at the top (e.g., const ESC =
"\x1b"), extract shared regexes into reusable RegExp instances using the RegExp
constructor with String.raw (e.g., CPR_RESPONSE_PATTERN = new
RegExp(String.raw`${ESC}\[[0-9]+;[0-9]+R`) and KITTY_KEYBOARD_PATTERN = new
RegExp(String.raw`${ESC}\[[0-9;:]+u`), update summarizeTerminalInput to use
data.split(ESC).length - 1 instead of data.match(/\x1b/g), replace all literal
regexes in summarizeTerminalInput and isInterestingTerminalInput with the new
CPR_RESPONSE_PATTERN and KITTY_KEYBOARD_PATTERN and other patterns constructed
via RegExp + String.raw (for focusIn/focusOut/bracketed paste use
String.raw`${ESC}[I` etc.), and reorder imports at the top to satisfy Biome
ordering rules.
pty-subprocess.ts:
- appendFileSync インポート削除
- DEBUG_FLOW/DEBUG_RAW_PTY 等の調査用定数を削除
- utf8BytesToHex/previewText/summarizeAnsi/appendDebugRecord/debugFlowLog 関数を削除
- flushOutput/handleSpawn/onData/onExit/handleWrite/handleResize 内の
appendDebugRecord・debugFlowLog 呼び出しを削除
- handleSpawn 内の env dump (process.stderr.write) を削除
v1-terminal-cache.ts:
- FLOW_DEBUG_PANE_ID_KEY 等の定数・デバッグ関数群を削除
- attachToContainer/ResizeObserver/detachFromContainer/scheduleWrite 内の
debugRendererFlow・debugRendererHex 呼び出しを削除
useTerminalConnection.ts:
- shouldLogPaneDebug 等のデバッグ関数群を削除
- write 時・resize 時のデバッグログを削除
useTerminalLifecycle.ts:
- このPRで誤って削除した DEBUG_TERMINAL ログ4箇所を復元
(Mount / isReattach / createOrAttach start / Unmount)
knowledge.md: 調査ノートを削除
調査用に追加した変更で本PRの目的（バッファ0ms化）と無関係なため除去。 DEBUG_TERMINAL は元の localStorage のみの判定に戻す。
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/v1-terminal-cache.ts (2)
145-160:resizeDebounceTimerを実際の debounce に接続するか削除してください。追加された timer は clear されるだけで set されておらず、
ResizeObserver内のfitAddon.fit()/onResizeは同期実行のままです。連続 resize を一括発火する意図なら timer を使い、意図しないなら field とコメントを削除しておくと読み違いを防げます。debounce する場合の修正例
const observer = new ResizeObserver(() => { if (container.clientWidth === 0 || container.clientHeight === 0) return; - const prevCols = entry.lastCols; - const prevRows = entry.lastRows; - entry.fitAddon.fit(); - entry.lastCols = entry.xterm.cols; - entry.lastRows = entry.xterm.rows; + if (entry.resizeDebounceTimer) { + clearTimeout(entry.resizeDebounceTimer); + } + entry.resizeDebounceTimer = setTimeout(() => { + entry.resizeDebounceTimer = null; + if (container.clientWidth === 0 || container.clientHeight === 0) return; - if (entry.lastCols !== prevCols || entry.lastRows !== prevRows) { - onResize?.(); - } + const prevCols = entry.lastCols; + const prevRows = entry.lastRows; + entry.fitAddon.fit(); + entry.lastCols = entry.xterm.cols; + entry.lastRows = entry.xterm.rows; + + if (entry.lastCols !== prevCols || entry.lastRows !== prevRows) { + onResize?.(); + } + }, 0); });この案を採る場合は、
detachFromContainer()/dispose()でも同じ timer clear を入れてください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/v1-terminal-cache.ts` around lines 145 - 160, The entry.resizeDebounceTimer is never set — either implement a real debounce around the ResizeObserver callback or remove the field/comments to avoid confusion. If you choose debounce, wrap the body of the ResizeObserver (the fitAddon.fit() and onResize call) in a setTimeout that resets entry.resizeDebounceTimer and clearTimeout on subsequent events so rapid resizes coalesce; ensure you set entry.resizeDebounceTimer when scheduling and clear it in the existing clearTimeout branch. If you choose removal, delete the resizeDebounceTimer field and the clearTimeout call and leave the observer callback synchronous. If implementing debounce, also clear the timer in detachFromContainer() and dispose() to avoid leaks. Use the symbols entry.resizeDebounceTimer, ResizeObserver, entry.fitAddon.fit(), onResize, detachFromContainer(), and dispose() to locate changes.
230-243: write 周りのコメントを direct write 化後の実装に合わせてください。
scheduleWrite()は logging しておらず、flushWrite()も no-op なので、コメントの “logging” や古い rAF buffer 前提が残ると次の修正で誤解されやすいです。修正例
-// --- rAF write buffer --- +// --- Terminal writes --- /** - * Thin wrapper around xterm.write so callers share cache lookup and logging. + * Thin wrapper around xterm.write so callers share cache lookup. * Keep the PTY -> xterm path as close to VS Code/xterm's recommended * integration as possible. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/v1-terminal-cache.ts` around lines 230 - 243, コメントが古い実装（logging や rAF バッファ前提）を前提にしているため、scheduleWrite と flushWrite の説明を現行の direct write 実装に合わせて更新してください: scheduleWrite(paneId, data) はキャッシュ.lookup 後に直接 entry.xterm.write を呼ぶことを明記し、logging を行わない点を除去または明確にし、flushWrite(_paneId) は後方互換の no-op であることを明記して古いバッファ動作の説明を削除してください。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/v1-terminal-cache.ts`:
- Around line 119-120: The HMR-restored entries from hot.data.v1TerminalCache
may lack the openOnce method and calling entry.openOnce() causes a TypeError;
update the restore/attach logic in v1-terminal-cache to guard the call or patch
missing methods by either checking if typeof entry.openOnce === "function"
before invoking it, or when hydrating entries from hot.data.v1TerminalCache add
a no-op openOnce to entries that don't have one so
container.appendChild(entry.wrapper); entry.openOnce(); is safe after HMR.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/v1-terminal-cache.ts`:
- Around line 145-160: The entry.resizeDebounceTimer is never set — either
implement a real debounce around the ResizeObserver callback or remove the
field/comments to avoid confusion. If you choose debounce, wrap the body of the
ResizeObserver (the fitAddon.fit() and onResize call) in a setTimeout that
resets entry.resizeDebounceTimer and clearTimeout on subsequent events so rapid
resizes coalesce; ensure you set entry.resizeDebounceTimer when scheduling and
clear it in the existing clearTimeout branch. If you choose removal, delete the
resizeDebounceTimer field and the clearTimeout call and leave the observer
callback synchronous. If implementing debounce, also clear the timer in
detachFromContainer() and dispose() to avoid leaks. Use the symbols
entry.resizeDebounceTimer, ResizeObserver, entry.fitAddon.fit(), onResize,
detachFromContainer(), and dispose() to locate changes.
- Around line 230-243: コメントが古い実装（logging や rAF バッファ前提）を前提にしているため、scheduleWrite と
flushWrite の説明を現行の direct write 実装に合わせて更新してください: scheduleWrite(paneId, data)
はキャッシュ.lookup 後に直接 entry.xterm.write を呼ぶことを明記し、logging
を行わない点を除去または明確にし、flushWrite(_paneId) は後方互換の no-op
であることを明記して古いバッファ動作の説明を削除してください。
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0bb2b29-87a5-40f0-b447-3e1d36609c54
📒 Files selected for processing (4)
.gitignoreapps/desktop/src/main/terminal-host/pty-subprocess.tsapps/desktop/src/main/terminal-host/session.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/v1-terminal-cache.ts
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- apps/desktop/src/main/terminal-host/pty-subprocess.ts
- apps/desktop/src/main/terminal-host/session.ts
Comment on lines
119
to
+120
| container.appendChild(entry.wrapper); | ||
| entry.openOnce(); |
There was a problem hiding this comment.
HMR 復元済み cache の openOnce 欠落を吸収してください。
hot.data.v1TerminalCache から復元される旧 shape の entry は openOnce を持たないため、HMR 後の再 attach で entry.openOnce() が TypeError になります。旧 entry は既に open 済みのはずなので、呼び出し側か HMR 復元時に runtime guard / no-op 補完を入れるのが安全です。
修正例
container.appendChild(entry.wrapper);
- entry.openOnce();
+ const maybeOpenOnce = (
+ entry as Partial<Pick<CachedTerminal, "openOnce">>
+ ).openOnce;
+ maybeOpenOnce?.();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/v1-terminal-cache.ts`
around lines 119 - 120, The HMR-restored entries from hot.data.v1TerminalCache
may lack the openOnce method and calling entry.openOnce() causes a TypeError;
update the restore/attach logic in v1-terminal-cache to guard the call or patch
missing methods by either checking if typeof entry.openOnce === "function"
before invoking it, or when hydrating entries from hot.data.v1TerminalCache add
a no-op openOnce to entries that don't have one so
container.appendChild(entry.wrapper); entry.openOnce(); is safe after HMR.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Desktop の terminal で Codex CLI を使ったときに起きていた入力遅延と描画不安定をまとめて改善します。
knowledge.mdに記録するRelated Issues
なし
Type of Change
Testing
自動テストは未実施です。
手動確認手順:
cd apps/desktopSUPERSET_WORKSPACE_NAME=superset SKIP_ENV_VALIDATION=1 DESKTOP_VITE_PORT=5222 bun run devScreenshots (if applicable)
なし
Additional Notes
xterm.open()のタイミングと renderer 側 resize 経路を整理していますknowledge.mdに残していますSummary by CodeRabbit
リリースノート
新機能
バグ修正
ドキュメント