Revert "fix: solve #3028 — forward DA1 query responses during shell init"#3127
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request removes tests validating headless emulator response forwarding during shell initialization and modifies the session logic to skip forwarding when shell readiness is "pending". Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 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! 🎉 |
Greptile SummaryThis PR reverts #3030, which had forwarded headless-emulator terminal query responses (DA1, DSR) to the subprocess during shell init. The revert adds an early-return guard in the Key concerns:
Confidence Score: 2/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Fish shell starts\nSends DA1 query ESC-c] --> B{attachedClients.size === 0?}
B -- No --> C[Renderer xterm handles DA1\nForwards response to shell]
B -- Yes --> D[HeadlessEmulator processes DA1\nGenerates response via onData]
D --> E{shellReadyState === pending?\nNEW GUARD added by this revert}
E -- Yes --> F[🚫 Response DROPPED\nFish waits up to 10s for reply\nIssue 3028 reintroduced]
E -- No --> G[sendWriteToSubprocess\nResponse forwarded to shell]
F --> H[After 10s timeout:\nFish disables cursor-shape\nand reflow detection]
Reviews (1): Last reviewed commit: "Revert "fix: solve #3028 — forward DA1 q..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // The headless emulator responds to terminal queries (e.g. DA) | ||
| // when no renderer client is attached. During shell init we drop | ||
| // these — they'd land in the pre-ready stdin queue and appear as | ||
| // typed text like "?62;4;9;22c" once flushed. After a client | ||
| // attaches the renderer's xterm handles all terminal queries. |
There was a problem hiding this comment.
sendWriteToSubprocess bypasses the preReadyStdinQueue
The comment justifies dropping headless-emulator responses during pending by saying they'd "land in the pre-ready stdin queue and appear as typed text like ?62;4;9;22c once flushed." This is factually incorrect.
The onData callback calls this.sendWriteToSubprocess(data) directly, not this.write(data). Only write() routes through preReadyStdinQueue. The emulator's response would be sent straight to the subprocess's stdin — it would never touch the pre-ready queue. If the real motivation is something else (e.g. the headless emulator generates an incorrect or incomplete DA1 response that confuses fish/zsh), the comment should say so.
| this.subprocess && | ||
| this.subprocessReady | ||
| ) { | ||
| if (this.shellReadyState === "pending") return; |
There was a problem hiding this comment.
By returning early when shellReadyState === "pending", this change drops all headless-emulator responses (DA1, DSR, etc.) during the critical shell-init window. Fish shell sends a DA1 query (\x1b[c) at startup and waits up to 10 seconds for a response before disabling cursor-shape changes and reflow detection.
The SHELL_READY_TIMEOUT_MS is 15 seconds, so when a session starts without a renderer client attached, fish's 10-second DA1 timeout will always fire before the shell-ready marker arrives, degrading the shell experience every single headless startup.
The original PR #3030 was specifically introduced to fix this. If this revert is needed because #3030 introduced a separate regression, that regression should be documented here (and ideally addressed separately) so the trade-off is clear.
…ing shell init (superset-sh#3030)" (superset-sh#3127) This reverts commit 3194568.
cherry-pick方式で内容を取り込み済みの14コミットをgit履歴上もマージ済みにする。 取り込み済み (cherry-pick / 手動移植): - be22b46 superset-sh#3125 — スキップ (下記参照) - 88bc7fb superset-sh#3127 — Revert DA1 ✓ - 92d0ff9 superset-sh#3054 — DA1 fix ✓ - c48450e superset-sh#3093 — file viewer pane fix ✓ - fffa8db superset-sh#3128 — version 1.4.7 ✓ - 589a7c7 superset-sh#3136 — fuzzy scorer (ハイブリッド方式) ✓ - ceb8c81 superset-sh#3150 — Electron 40.8.5 ✓ - 8922b94 superset-sh#3137 — terminalId分離 ✓ - c7508e5 superset-sh#3152 — GitHub無料化 ✓ - 2b91f11 superset-sh#3155 — v2 terminal theme ✓ - b8b11af superset-sh#3154 — TUI dimension fix ✓ - 7599ace superset-sh#3149 — v2 sidebar file tree (手動統合) ✓ - 4d7c612 superset-sh#3174 — DnD重複削除 ✓ - 864977d superset-sh#3157 — Host Service分離 ✓ 意図的にスキップ: - be22b46 superset-sh#3125 (GitHub polling簡素化) フォーク独自のGitHubSyncService (バックエンド集中ポーリング) と 設計が異なるため不採用。upstreamはフロントエンドhover駆動、フォークは バックエンドキャッシュウォーマー方式。詳細は githubQueryPolicy.ts と github-sync-service.ts のFORK NOTEを参照。 ゴースト・マージ復元 (revert 134cfd5 で消失した内容): - 538f306 superset-sh#3120 — Patch vuln ✓ - 1588d20 superset-sh#3108 — terminal lifecycle分離 ✓ - 59426f6 superset-sh#3122 — file tree + FilePane + Alert refactor (手動統合) ✓ - 10d9a5d superset-sh#3097 — tiptap line-height ✓ - 337a9ae superset-sh#3121 — Codex hooks削除 ✓
1 participant
Reverts #3030
Summary by cubic
Reverts the change that forwarded headless emulator DA/DSR responses during shell init, restoring the previous behavior of dropping them while the shell is pending. This avoids queued query replies appearing as typed text; once a client attaches, the renderer handles terminal queries.
Written for commit fd4d431. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests