[codex] fix v1 split pane startup sizing#3416
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR introduces a terminal session readiness system that allows callers to wait for a terminal pane's session to be fully mounted before executing commands. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant LaunchCommand as launchCommandInPane
participant SessionReady as waitForTerminalSessionReady
participant Lifecycle as useTerminalLifecycle
participant WriteCmd as writeCommandInPane
Caller->>LaunchCommand: launchCommandInPane({..., waitForMountedSession: true})
LaunchCommand->>SessionReady: waitForTerminalSessionReady(paneId)
activate SessionReady
note over SessionReady: Waits for signal (returns promise)
Lifecycle->>Lifecycle: ensureTerminalAttached + attach flow
Lifecycle->>SessionReady: markTerminalSessionReady(paneId)
deactivate SessionReady
SessionReady-->>LaunchCommand: Promise resolves
LaunchCommand->>WriteCmd: writeCommandInPane(paneId, command)
WriteCmd-->>LaunchCommand: Command written
LaunchCommand-->>Caller: Complete
alt On Error
Lifecycle->>SessionReady: rejectTerminalSessionReady(paneId, error)
SessionReady-->>LaunchCommand: Promise rejects
LaunchCommand-->>Caller: Error propagates
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 |
Greptile SummaryThis PR fixes a v1 terminal race condition where a split-pane's initial command could be written before the new pane had finished mounting and fitting its xterm instance, causing TUIs to initialise at fallback dimensions. The fix introduces a Changes at a glance:
Issues found:
Confidence Score: 4/5Safe to merge — the race condition fix is correct and all main failure paths are covered; remaining issues are non-blocking P2s. The core fix is sound: the waiter queue is properly initialised, resolved on success, rejected on the two attach-error paths and on dispose. The three P2 findings (no timeout, undocumented ignored params, missing tests) do not affect correctness in normal operation. The only realistic risk is the pathological cancel-loop scenario with no timeout, but since all launched commands are fire-and-forget (void + .catch), the worst outcome is a silently dropped command rather than a crash or hang visible to the user. v1-terminal-cache.ts (timeout concern) and launch-command.test.ts (new path untested) Important Files Changed
Sequence DiagramsequenceDiagram
participant UTP as useTabsWithPresets
participant LCP as launchCommandInPane
participant Cache as v1-terminal-cache
participant TLC as useTerminalLifecycle
UTP->>UTP: storeSplitPane*(tabId, ...)
UTP->>LCP: launchCommandInPane({waitForMountedSession:true})
LCP->>Cache: waitForStreamReady(paneId)
Note over Cache: Returns pending Promise,<br/>adds waiter to streamReadyWaiters
par React mounts new pane
TLC->>TLC: createOrAttach.mutateAsync(...)
alt attach success
TLC->>Cache: setStreamReady(paneId)
Cache->>Cache: resolveStreamReadyWaiters(paneId)
Cache-->>LCP: Promise resolved
LCP->>LCP: writeCommandInPane(paneId, command)
else attach error (killed / general)
TLC->>Cache: failStreamReady(paneId, error)
Cache->>Cache: rejectStreamReadyWaiters(paneId)
Cache-->>LCP: Promise rejected
LCP-->>UTP: .catch → log error
else pane disposed
TLC->>Cache: dispose(paneId)
Cache->>Cache: rejectStreamReadyWaiters(paneId)
Cache-->>LCP: Promise rejected
end
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/v1-terminal-cache.ts (1)
346-363:⚠️ Potential issue | 🟠 MajorReject stream-ready waiters even when the cache entry does not exist.
Line 348 returns before waiter rejection, so promises created by
waitForStreamReady(when called beforegetOrCreate) can hang forever if the pane is disposed before mounting.💡 Proposed fix
export function dispose(paneId: string): void { + rejectStreamReadyWaiters( + paneId, + new Error("Terminal disposed before reaching stream-ready state"), + ); + const entry = cache.get(paneId); if (!entry) return; @@ entry.cleanupCreation(); entry.xterm.dispose(); cache.delete(paneId); - rejectStreamReadyWaiters( - paneId, - new Error("Terminal disposed before reaching stream-ready state"), - ); }🤖 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 346 - 363, The dispose function currently returns immediately if cache.get(paneId) is falsy which prevents calling rejectStreamReadyWaiters and can leave promises from waitForStreamReady hanging; change dispose (function dispose) so it still calls rejectStreamReadyWaiters(paneId, new Error(...)) when no cache entry exists (i.e., move or duplicate the rejectStreamReadyWaiters call before the early return or invoke it in the !entry branch) while preserving the existing behavior when entry exists (disconnect resizeObserver, unsubscribe subscription, call cleanupCreation, dispose xterm, delete cache) and keeping the DEBUG_TERMINAL log.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/v1-terminal-cache.ts`:
- Around line 346-363: The dispose function currently returns immediately if
cache.get(paneId) is falsy which prevents calling rejectStreamReadyWaiters and
can leave promises from waitForStreamReady hanging; change dispose (function
dispose) so it still calls rejectStreamReadyWaiters(paneId, new Error(...)) when
no cache entry exists (i.e., move or duplicate the rejectStreamReadyWaiters call
before the early return or invoke it in the !entry branch) while preserving the
existing behavior when entry exists (disconnect resizeObserver, unsubscribe
subscription, call cleanupCreation, dispose xterm, delete cache) and keeping the
DEBUG_TERMINAL log.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbc4189e-37f7-4c08-9839-35386ed829aa
📒 Files selected for processing (4)
apps/desktop/src/renderer/lib/terminal/launch-command.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/v1-terminal-cache.tsapps/desktop/src/renderer/stores/tabs/useTabsWithPresets.ts
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
* fix v1 split pane startup sizing * Handle background
* fix v1 split pane startup sizing * Handle background
…ssion-readiness [PR4/5] fix(upstream): v1 split pane 起動時のサイジング修正（ターミナル Session Readiness 機構）(superset-sh#3416)
All 9 upstream commits have been individually cherry-picked via PR#159~#163: | Upstream | Our PR | Description | |---|---|---| | d656b7e (superset-sh#3415) | #159 (PR#1) | terminal clipboard handling | | 31fcf19 (superset-sh#3416) | #162 (PR#4) | v1 split pane startup sizing fix | | 039edf2 (superset-sh#3403) | #161 (PR#3) | Cmd+Alt+Arrow spatial pane focus | | b18a00c (superset-sh#3421) | #159 (PR#1) | v2 right sidebar toggle reactive | | 3dd1de2 (superset-sh#3420) | #161 (PR#3) | v2 diff viewer + tab title resolution | | b42a114 (superset-sh#3418) | #159 (PR#1) | CodeMirror hotkey enablement | | c925f4d (superset-sh#3422) | #160 (PR#2) | unbound defaults + restore prev/next tab/workspace | | bb12c09 (superset-sh#3419) | #163 (PR#5) | version bump 1.5.3 | | 47efa73 (superset-sh#3432) | #159 (PR#1) | pending/update-required error selectable | Fork-specific features preserved: - auto-updater (IS_FORK, GitHub Releases API) - QuitMode/cleanupMainWindowResources lifecycle - GitHubSyncService, SpreadsheetViewer - BROWSER_RELOAD / BROWSER_HARD_RELOAD / SEARCH_IN_FILES hotkeys - HotkeyCategory "Browser" - v1 deep-link navigation (useSearch/WorkspaceSearchParams) - v1 tRPC-based PREV/NEXT_WORKSPACE handlers - v1 CLOSE_TERMINAL/CLOSE_TAB hotkey handlers - v2 extra state (rightSidebarOpenViewWidth, showPresetsBar)
1 participant
Summary
Fixes a v1 terminal race where opening a split pane and launching an initial command at the same time could start the terminal session at fallback dimensions, causing TUIs to initialize with the wrong size.
Root Cause
The helper-side launch path could create or attach the daemon session before the newly-created split pane had mounted and finished its first real attach. In that case the backend could start from fallback sizing, and the initial command would run before the mounted terminal had established the correct dimensions.
What Changed
streamReadywaiter to the v1 terminal cache so callers can wait for the mounted terminal attach to complete.launchCommandInPaneto optionally wait for the mounted session instead of issuing a helper-side attach first.Impact
Validation
bunx biome check apps/desktop/src/renderer/lib/terminal/launch-command.ts apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/v1-terminal-cache.ts apps/desktop/src/renderer/stores/tabs/useTabsWithPresets.tsbun test apps/desktop/src/renderer/lib/terminal/launch-command.test.tsNotes
I also checked the v2 terminal path for inspiration. V2 gates initial commands on shell readiness, but the v1 bug here was specifically about pane sizing, so the correct fix was to wait on v1's mounted-session readiness instead of copying the v2 shell-ready flow directly.
Summary by cubic
Fixes a race in v1 split panes where the initial command ran before the pane mounted, causing fallback terminal sizes. Launches now wait for the mounted, fitted session before sending startup commands; background tabs still use the helper-side attach.
waitForTerminalSessionReadywithmark/clear/reject) and integrated it into the v1 terminal lifecycle.waitForMountedSessionoption tolaunchCommandInPane; preset and split-pane flows use it only when panes mount in the active tab.Written for commit b28c69b. Summary will update on new commits.
Summary by CodeRabbit
Release Notes