Fix terminal state loss when switching workspaces #2717
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
WalkthroughAdds a per-window tab-view cache ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
emain/emain-window.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-15T03:18:52.647Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2433
File: emain/emain-window.ts:811-828
Timestamp: 2025-10-15T03:18:52.647Z
Learning: In emain/emain-window.ts, within the relaunchBrowserWindows function, ClientService.GetClientData() is guaranteed to return a valid client object and never null/undefined. The backend ensures a client is initialized before startup, so no null-guard is needed when accessing clientData.windowids.
Applied to files:
emain/emain-window.ts
🔇 Additional comments (5)
emain/emain-window.ts (5)
142-142: LGTM!The cache field is well-documented and properly initialized alongside the existing
allLoadedTabViewsmap. Using aMap<string, WaveTabView>provides efficient O(1) lookup by tab ID.Also applies to: 222-222
357-361: LGTM!Proper cleanup of cached views on window teardown. Destroying each cached view before clearing the map prevents memory leaks.
586-610: Good defensive handling for rapid workspace switching.The edge case handling at lines 594-598 correctly destroys any existing cached view before caching the new one, preventing memory leaks during rapid workspace switches. The decision to keep views as children of
contentView(positioned off-screen) rather than removing them is sound for preserving WebView/xterm.js state.
615-623: LGTM!The cache-first lookup correctly reuses existing tab views, and setting
tabInitialized = trueensures the view bypasses full reinitialization, preserving terminal state. Removing from cache on reuse prevents double-ownership.
679-685: The function is correctly designed for active tabs only, not cached ones.
getWaveWindowByTabIdonly checksallLoadedTabViewsby design. It's only called viagetWaveWindowByWebContentsId, which retrieves tabs fromwcIdToWaveTabMap—a map containing only active tabs with running web content. Active tabs always exist inallLoadedTabViewsand never inallTabViewsCache(cached tabs lack active webContents). While similar operations likeremoveTabViewdefensively check both maps, the current behavior is intentional and safe for this lookup's usage pattern.
stevenvo
force-pushed
the
fix/preserve-terminal-state-workspace-switch
branch
from
562732e to
978443c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emain/emain-window.ts (1)
357-361: MissingremoveChildViewfor cached views before destroy.Cached views remain as children of
contentView(just positioned off-screen). For consistency with the loop above (lines 351-356) and to avoid relying on implicit cleanup during window destruction, consider removing them fromcontentViewbefore destroying.🔎 Proposed fix
// Also destroy any cached views for (const tabView of this.allTabViewsCache.values()) { + if (!this.isDestroyed()) { + this.contentView.removeChildView(tabView); + } tabView?.destroy(); } this.allTabViewsCache.clear();
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
emain/emain-window.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-15T03:18:52.647Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2433
File: emain/emain-window.ts:811-828
Timestamp: 2025-10-15T03:18:52.647Z
Learning: In emain/emain-window.ts, within the relaunchBrowserWindows function, ClientService.GetClientData() is guaranteed to return a valid client object and never null/undefined. The backend ensures a client is initialized before startup, so no null-guard is needed when accessing clientData.windowids.
Applied to files:
emain/emain-window.ts
🧬 Code graph analysis (1)
emain/emain-window.ts (1)
emain/emain-tabview.ts (2)
WaveTabView(106-225)getOrCreateWebViewForTab(295-349)
🔇 Additional comments (5)
emain/emain-window.ts (5)
142-142: LGTM!The cache declaration is well-typed and the comment clearly documents its purpose for preserving tab views across workspace switches.
222-222: LGTM!Consistent initialization alongside
allLoadedTabViews.
586-610: Good handling of workspace switch with proper edge case protection.The implementation correctly:
- Preserves terminal state by caching instead of destroying
- Handles rapid workspace switching by destroying stale cached views
- Documents the lifecycle clearly
The design choice to keep cached views as children of
contentView(just off-screen) is valid and simpler than removing/re-adding.
615-625: LGTM!Cache-first lookup pattern is correctly implemented. Cached views are properly removed from the cache and will be re-added to
allLoadedTabViewsviasetTabViewIntoWindow.
655-673: Good fix - addresses the previous review comment.The
removeChildViewcall is now correctly placed after the if-else block (lines 668-671), ensuring cached views are removed fromcontentViewbefore being destroyed. This properly handles the case where cached views remain as children ofcontentView(just positioned off-screen).
|
Closing temporarily for additional debugging and testing. Will reopen after issues are resolved. |
|
Fixed the gibberish screen issue! The problem was keeping cached views as children of contentView (positioned off-screen), causing multiple terminals to render on top of each other. Changes in latest commit:
The display corruption should now be gone. Ready for review! |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
emain/emain-window.ts (1)
686-692:getWaveWindowByTabIdmisses cached tabs—can returnundefinedfor valid tab IDs.When a tab is moved to
allTabViewsCacheduring workspace switches, its WebContents remains active. If that cached tab sends an IPC event, the lookup chaingetWaveWindowByWebContentsId→getWaveWindowByTabIdwill fail becausegetWaveWindowByTabIdonly searchesallLoadedTabViews, notallTabViewsCache. This causes handlers likeset-active-tab,switch-workspace, and others to receiveundefinedand may silently fail or throw errors.Extend
getWaveWindowByTabIdto also checkallTabViewsCache:Suggested fix
export function getWaveWindowByTabId(tabId: string): WaveBrowserWindow { for (const ww of waveWindowMap.values()) { if (ww.allLoadedTabViews.has(tabId) || ww.allTabViewsCache.has(tabId)) { return ww; } } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emain/emain-window.ts
🧰 Additional context used
🧬 Code graph analysis (1)
emain/emain-window.ts (1)
emain/emain-tabview.ts (2)
WaveTabView(106-225)getOrCreateWebViewForTab(295-349)
🔇 Additional comments (5)
emain/emain-window.ts (5)
142-142: LGTM: Cache field properly declared and initialized.The
allTabViewsCachefield is well-documented and correctly initialized alongsideallLoadedTabViews.Also applies to: 222-222
357-361: LGTM: Cached view cleanup is correct.Cached views don't need
removeChildViewhere because they were already removed fromcontentViewwhen cached (line 590). The code correctly destroys and clears them during window teardown.
586-609: Well-designed caching with defensive edge-case handling.The code properly:
- Removes views from
contentViewbefore caching to avoid DOM conflicts- Destroys any pre-existing cached view for the same tabId (lines 593-596) to handle rapid workspace switching
- Documents the indefinite cache lifetime clearly (lines 606-609)
This preserves terminal state as intended.
662-675: LGTM: Tab removal correctly handles both loaded and cached views.The dual-path logic is correct:
- Cached tabs (lines 665-671): Delete from cache without calling
removeChildView(already removed at line 590)- Loaded tabs (lines 672-674): Remove from
contentViewand delete from mapBoth paths converge to
destroy()at line 676.
619-630: Cache reuse logic is well-designed with state preservation already validated.The flow correctly reuses cached views:
- Check cache first (line 620)
- Remove from cache and re-add to
contentView(lines 624-626)- Treat as initialized (line 621) since the view was previously set up
The codebase already documents the state preservation mechanism: "Cached views are kept alive indefinitely and only destroyed when: (1) the tab is explicitly closed by the user, (2) the window is closed. This matches how traditional terminal apps work and prevents terminal state loss." The design intentionally removes views from the view hierarchy during workspace switches while keeping them alive in the cache, then re-attaches them when needed—this approach preserves Electron's WebContentsView internal state including alternate screen mode and scrollback history. Edge cases are handled (e.g., rapid workspace switching).
Fixes issue where TUI applications (vim, htop, opencode, etc.) would lose terminal state when switching between workspaces. This caused inability to scroll and display corruption. Root cause: Workspace switching was destroying all tab views including terminals, then recreating them from cache. This destroyed xterm.js instances and lost their state. Solution: Cache tab views across workspace switches instead of destroying them. Tab views are positioned off-screen but kept alive, preserving: - Terminal buffer state (normal and alternate screen modes) - Scrollback history and scrolling capability - Running processes and their output - Cursor position and all terminal modes Memory management: Cached views kept alive until tab closed or window closed. Note: This PR includes the StreamCancelFn type fix from wavetermdev#2716 to ensure the branch builds correctly.
Cached tab views were left as children of contentView (positioned off-screen), causing multiple terminals to render on top of each other. Changes: - Remove cached views from contentView during workspace switch - Re-add when reusing from cache - Simplify removal logic (cached vs loaded)
When cached tabs send IPC events, lookup was failing because getWaveWindowByTabId only searched allLoadedTabViews. Now also checks allTabViewsCache to handle cached tabs.
stevenvo
force-pushed
the
fix/preserve-terminal-state-workspace-switch
branch
from
7575db4 to
ec88c71
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emain/emain-window.ts (1)
142-142: Consider monitoring memory usage of cached tab views in production.The indefinite caching strategy (1–5 MB per cached terminal) is well-suited to preserving terminal state across workspace switches. For typical usage this overhead is negligible, but power users who frequently switch between many workspaces with numerous tabs may accumulate cached views over extended sessions.
Consider adding telemetry to monitor:
- Number of cached views per window
- Total memory consumed by cached views
- Cache hit rate when returning to workspaces
This data would help validate the current "cache indefinitely" policy and inform future decisions about cache eviction strategies if needed.
Also applies to: 586-610
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emain/emain-window.ts
🧰 Additional context used
🧬 Code graph analysis (1)
emain/emain-window.ts (1)
emain/emain-tabview.ts (2)
WaveTabView(106-225)getOrCreateWebViewForTab(295-349)
🔇 Additional comments (6)
emain/emain-window.ts (6)
142-142: Cache field properly declared and initialized.The
allTabViewsCachefield is correctly typed and initialized. The documentation clearly explains its purpose for preserving terminal state across workspace switches.Also applies to: 222-222
357-361: Cached views properly destroyed on window teardown.The cleanup logic correctly destroys all cached views when the window closes. Since cached views are removed from
contentViewbefore being cached, noremoveChildViewcall is needed here.
586-610: Workspace switch correctly preserves tab views in cache.This is the core fix that prevents terminal state loss. The implementation correctly:
- Removes views from
contentViewbefore caching (prevents display corruption)- Handles edge cases with duplicate cache entries
- Documents the cache lifetime clearly
- Preserves terminal state (scrollback, alternate screen, cursor position) across workspace switches
619-630: Cache lookup enables seamless tab view reuse.The logic correctly prioritizes cached views during tab restoration:
- Checks cache first to reuse existing views
- Properly transfers views from cache back to
contentView- Sets
tabInitialized = truefor cached views (since they're already initialized)- Falls back to creating new views only when necessary
This ensures preserved terminal state is correctly restored when returning to a workspace.
662-675: Tab removal correctly handles both loaded and cached views.The logic properly distinguishes between:
- Loaded tabs: removed from
contentViewandallLoadedTabViewsbefore destroying- Cached tabs: only removed from cache before destroying (no
removeChildViewneeded since they were already removed fromcontentViewduring workspace switch)This correctly implements the fix for the display corruption issue mentioned in the PR objectives.
688-688: Window lookup correctly considers cached tabs.The updated logic checks both
allLoadedTabViewsandallTabViewsCache, ensuring IPC events from cached tabs are correctly routed to their parent window. This is essential for maintaining communication with cached terminal views.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
emain/emain-window.ts (2)
585-613: Consider resettingisActiveTabon cached views.When views are moved to the cache, their
isActiveTabproperty remains in its previous state. WhilesetTabViewIntoWindowdoes manage this property when views are reused, having staleisActiveTab = trueon multiple cached views could cause confusion during debugging or if this property is ever queried directly.🔎 Proposed fix
for (const [tabId, tabView] of this.allLoadedTabViews.entries()) { if (!this.isDestroyed()) { this.contentView.removeChildView(tabView); } + tabView.isActiveTab = false; // If tabId already in cache (edge case with rapid workspace switching), destroy the old cached view first const existingCachedView = this.allTabViewsCache.get(tabId); if (existingCachedView) { existingCachedView.destroy(); } this.allTabViewsCache.set(tabId, tabView); }
605-608: Consider adding a safeguard for extreme cache sizes.The indefinite retention is intentional and well-documented. However, for power users with many workspaces and tabs, memory usage could grow significantly. Consider:
- Adding telemetry/logging when the cache exceeds a threshold (e.g., 20+ tabs) to understand real-world usage patterns.
- Optionally, a future enhancement could add an LRU eviction policy or user-configurable limit.
This is not a blocker—the current approach correctly prioritizes state preservation over memory, matching traditional terminal behavior.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emain/emain-window.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-15T03:18:52.647Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2433
File: emain/emain-window.ts:811-828
Timestamp: 2025-10-15T03:18:52.647Z
Learning: In emain/emain-window.ts, within the relaunchBrowserWindows function, ClientService.GetClientData() is guaranteed to return a valid client object and never null/undefined. The backend ensures a client is initialized before startup, so no null-guard is needed when accessing clientData.windowids.
Applied to files:
emain/emain-window.ts
🔇 Additional comments (5)
emain/emain-window.ts (5)
141-141: LGTM!The cache field declaration and initialization follow the same pattern as
allLoadedTabViews. Clean and consistent.Also applies to: 221-221
356-360: LGTM!Proper cleanup of cached views during window teardown. Since cached views are already removed from
contentViewwhen they're moved to the cache (line 589), onlydestroy()is needed here.
618-629: LGTM!The cache-first lookup pattern is correct. Cached views are properly removed from the cache and re-added to
contentViewbefore being activated. ThetabInitialized = trueensures the reused view follows the "reusing an existing tab" path insetTabViewIntoWindow.
661-674: LGTM!The extended logic correctly handles removal from both loaded views and the cache. Since cached views are already removed from
contentViewduring the workspace switch (line 589), noremoveChildViewcall is needed when destroying them from the cache.
685-691: LGTM!Essential update to ensure IPC handlers can locate windows by tab ID for both active and cached tabs.
1 participant
Summary
Fixes terminal state loss and display corruption when switching between workspaces in WaveTerminal.
Problem
When switching workspaces, TUI applications (vim, htop, opencode, etc.) would:
Root Cause
The
switchworkspaceoperation calledremoveAllChildViews()which destroyed all tab views. When recreated, xterm.js terminal instances lost their state even though backend shell processes continued running.Tab switching vs Workspace switching:
Solution
Cache tab views across workspace switches instead of destroying them:
Implementation Details
Key Changes
Bug Fixes (commits 2-4)
getWaveWindowByTabIdWhat This Preserves
Memory Management
Testing
Tested with opencode (TUI app):
Behavior Match
Now matches macOS Terminal, iTerm2, and other terminal emulators where terminal state is preserved across workspace/tab switches.