Conversation
…ore cached Root cause (from superset-sh#3348): createOrAttach onSuccess eagerly called v1TerminalCache.startStream / setStreamReady / markTerminalSessionReady before inspecting result.isColdRestore. Cold-restore responses come back without a real backend session, yet the cache was left in streamReady=true state. On the next mount (e.g. after a tab switch unmount from PersistentTabRenderer — terminal panes are not persistent views), useTerminalLifecycle's isReattach fast-path saw streamReady=true + live subscription and skipped createOrAttach entirely. From that moment on every keystroke was forwarded to electronTrpcClient.terminal.write with no backend session, the main process threw, and writeRef (no onError callback) silently swallowed the failure — the user just saw a terminal that displayed output but refused to accept input. Fix: 1. useTerminalLifecycle.ts: defer startStream/setStreamReady/ markTerminalSessionReady until after the cold-restore branches return, so only real backend sessions flip the cache into streamReady=true. 2. useTerminalLifecycle.ts: when coldRestoreState.has(paneId) is true, force isReattach=false so remounts re-enter the full attach path instead of trusting stale streamReady. Belt-and-suspenders against the module-level coldRestoreState map surviving unmount. 3. useTerminalColdRestore.ts: in handleStartShell's onSuccess (which finally spawns a real shell for the restored pane), mark the cache/readiness waiters as ready. Reject readiness waiters on failure so preset paths do not hang forever. FORK NOTE: this is an upstream regression in superset-sh#3348 that superset-sh#3389 only narrowed. Revisit when upstream publishes its own fix.
|
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 46 minutes and 6 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ターミナルセッションの冷復元時の初期化フローが改善されました。 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts (1)
657-659: 重複ロジックのヘルパー関数への抽出を検討してください。ストリーム準備状態のマーキングシーケンス（
startStream→setStreamReady→markTerminalSessionReady）がuseTerminalColdRestore.tsと重複しています。将来的な一貫性のために、v1-terminal-cacheモジュールにヘルパー関数を追加することを検討できます。ただし、PRノートによると、これはアップストリームの修正が利用可能になるまでの一時的なフォークパッチとのことなので、現状のままでも問題ありません。
♻️ オプション：ヘルパー関数の提案
// v1-terminal-cache.ts に追加 export function markSessionReady(paneId: string): void { startStream(paneId); setStreamReady(paneId); markTerminalSessionReady(paneId); }🤖 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/useTerminalLifecycle.ts` around lines 657 - 659, The sequence startStream(paneId) → setStreamReady(paneId) → markTerminalSessionReady(paneId) is duplicated in useTerminalLifecycle (functions: v1TerminalCache.startStream, v1TerminalCache.setStreamReady, markTerminalSessionReady) and useTerminalColdRestore; extract this sequence into a single helper exported from the v1-terminal-cache module (e.g., export function markSessionReady(paneId: string)) and replace the three-call sequence in both useTerminalLifecycle.ts and useTerminalColdRestore.ts with a single v1TerminalCache.markSessionReady(paneId) call to keep logic consistent and maintainable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts`:
- Around line 657-659: The sequence startStream(paneId) → setStreamReady(paneId)
→ markTerminalSessionReady(paneId) is duplicated in useTerminalLifecycle
(functions: v1TerminalCache.startStream, v1TerminalCache.setStreamReady,
markTerminalSessionReady) and useTerminalColdRestore; extract this sequence into
a single helper exported from the v1-terminal-cache module (e.g., export
function markSessionReady(paneId: string)) and replace the three-call sequence
in both useTerminalLifecycle.ts and useTerminalColdRestore.ts with a single
v1TerminalCache.markSessionReady(paneId) call to keep logic consistent and
maintainable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 605b6b80-7e1a-4b40-8e16-a89ac9d12925
📒 Files selected for processing (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalColdRestore.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts
Address CodeRabbit review on #167: the three-call sequence startStream + setStreamReady + markTerminalSessionReady was repeated in useTerminalLifecycle.ts and useTerminalColdRestore.ts. Centralize it in v1-terminal-cache.ts so the cold-restore and normal attach paths stay in lockstep, and drop the now-unused markTerminalSessionReady imports from the consumers.
Owner
Author
CodeRabbit レビュー対応Nitpick 指摘の通り、 変更内容
検証
cold-restore と通常 attach の両方の経路が同じヘルパーを通るようになり、将来的な drift を防げます。 |
MocA-Love
added a commit
that referenced
this pull request
Apr 15, 2026
Codex review (PR#174): the previous unconditional markSessionReady() call in handleRetryConnection.onSuccess broke the PR #167 invariant that the cache must not flip to streamReady=true before a real backend session exists. Cold-restore reconnect responses come back with isColdRestore=true and no backend; setting the cache ready in that state lets a subsequent tab-switch remount take the isReattach fast-path and silently drop user keystrokes — exactly the bug PR #167 was meant to fix. Restore the !result.isColdRestore gate. The cold-restore reconnect path is still wired end-to-end because handleStartShell.onSuccess (in main, after PR #167) calls markSessionReady once the replacement shell spawns.
MocA-Love
added a commit
that referenced
this pull request
Apr 15, 2026
Codex review (PR#174): the previous unconditional markSessionReady() call in handleRetryConnection.onSuccess broke the PR #167 invariant that the cache must not flip to streamReady=true before a real backend session exists. Cold-restore reconnect responses come back with isColdRestore=true and no backend; setting the cache ready in that state lets a subsequent tab-switch remount take the isReattach fast-path and silently drop user keystrokes — exactly the bug PR #167 was meant to fix. Restore the !result.isColdRestore gate. The cold-restore reconnect path is still wired end-to-end because handleStartShell.onSuccess (in main, after PR #167) calls markSessionReady once the replacement shell spawns.
Merged
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
2 participants
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.
症状
v1 ターミナルで文字が打てなくなる致命バグ。最新 dmg でテスト中に発覚。
根本原因
Upstream PR superset-sh#3348 (`feat(desktop): port v2 hide-attach xterm pattern to v1 terminal`) で導入された、v1 terminal cache と cold restore の状態不整合。
PR superset-sh#3389 (`fix(desktop): v1 terminal — emit legacy marker + gate reattach on streamReady`) で isReattach 判定を `streamReady` ベースに厳密化したが、cold restore 経路が誤って `streamReady=true` を立てるので本質的には直っていない。
不整合の流れ
致命度
修正内容
1. `useTerminalLifecycle.ts` createOrAttach onSuccess
cold restore 判定より 後ろ に `startStream` / `setStreamReady` / `markTerminalSessionReady` を移動。実 backend session がある場合のみ cache を ready にマーク。
```diff
onSuccess: (result) => {
if (!isAttachActive()) return;
if (activeAttachRequestId !== requestId) return;
setConnectionError(null);
clearPaneInitialDataRef.current(paneId);
v1TerminalCache.startStream(paneId);
v1TerminalCache.setStreamReady(paneId);
markTerminalSessionReady(paneId);
const storedColdRestore = coldRestoreState.get(paneId);
if (storedColdRestore?.isRestored) { ... return; }
if (result.isColdRestore) { ... return; }
// Real backend session is live — safe to start the stream
// subscription and unblock waiters.
v1TerminalCache.startStream(paneId);
v1TerminalCache.setStreamReady(paneId);
markTerminalSessionReady(paneId);
pendingInitialStateRef.current = result;
```
2. `useTerminalLifecycle.ts` isReattach 判定
`coldRestoreState.has(paneId)` の間は isReattach を強制的に false にし、remount 時に必ず full attach 経路を通すようにする belt-and-suspenders。
```diff
const cachedBeforeCreate = v1TerminalCache.get(paneId);
+const hasPendingColdRestore = coldRestoreState.has(paneId);
const isReattach =
cachedBeforeCreate?.streamReady === true &&
```
3. `useTerminalColdRestore.ts` handleStartShell onSuccess
実シェルが spawn されたタイミングで、cache と session-readiness waiters を ready にマークする。失敗時は reject する。
```diff
onSuccess: (result) => {
pendingInitialStateRef.current = result;
maybeApplyInitialState();
+
// Mark the cache + readiness waiters now that a real shell exists.
v1TerminalCache.startStream(paneId);
v1TerminalCache.setStreamReady(paneId);
markTerminalSessionReady(paneId);
setIsRestoredMode(false);
coldRestoreState.delete(paneId);
...
},
onError: (error) => {
...
rejectTerminalSessionReady(
paneId,
new Error(error.message || "Failed to start shell"),
);
isStreamReadyRef.current = true;
```
FORK NOTE
これは upstream 由来のリグレッションなので、通常フォークの方針では修正せず upstream の fix を待つのが原則ですが、terminal が使えないのは致命的なため一時パッチとして適用します。
upstream が自前で fix を出したら、本 PR の修正を revert → upstream の実装に合わせる形で追従する想定。コミットメッセージと各修正箇所の `FORK NOTE` コメントに経緯を残してあります。
検証
再現手順（修正前）
修正後の期待動作
1〜3 は同じ。4 で入力 → 正常に反映される ✅
影響範囲
合計 2 ファイル、+45 / -8 行。
関連
Summary by CodeRabbit
リリースノート