Fix terminal state loss when switching workspaces #2714
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughA tab-view caching mechanism was added to WaveBrowserWindow in emain/emain-window.ts: a new public field Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🔇 Additional comments (5)
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
🧬 Code graph analysis (1)
emain/emain-window.ts (1)
emain/emain-tabview.ts (2)
WaveTabView(106-225)getOrCreateWebViewForTab(295-349)
🔇 Additional comments (4)
emain/emain-window.ts (4)
142-142: LGTM: Cache field properly declared and initialized.The
allTabViewsCachefield mirrors the pattern ofallLoadedTabViewsand is correctly initialized in the constructor.Also applies to: 222-222
357-361: LGTM: Proper cache cleanup on window teardown.Cached views are correctly destroyed and cleared when the window closes, preventing memory leaks.
610-618: LGTM: Cache-first lookup enables seamless tab reuse.The logic correctly:
- Checks the cache first for existing tab views
- Sets
tabInitialized=truefor cached views (already initialized)- Removes from cache when reused (will be added to
allLoadedTabViewsinsetTabViewIntoWindow)- Falls back to
getOrCreateWebViewForTabfor uncached tabsSince cached views remain children of
contentView(only positioned off-screen), they're correctly repositioned when reused.
586-604: Well-designed caching strategy for workspace switches.The approach of caching views off-screen rather than destroying them correctly preserves terminal state. The comments clearly document the lifecycle expectations.
Tab IDs are globally unique (generated as UUIDs independent of workspace context), so the cache key design is safe from collisions across workspaces.
| let tabView = this.allLoadedTabViews.get(tabId); | ||
| if (tabView == null) { | ||
| console.log("removeTabView -- tabView not found", tabId, this.waveWindowId); | ||
| // the tab was never loaded, so just return | ||
| return; | ||
| // Check cache - tab might be from a different workspace | ||
| tabView = this.allTabViewsCache.get(tabId); | ||
| if (tabView == null) { | ||
| console.log("removeTabView -- tabView not found in loaded or cache", tabId, this.waveWindowId); | ||
| return; | ||
| } | ||
| console.log("removeTabView -- removing from cache", tabId, this.waveWindowId); | ||
| this.allTabViewsCache.delete(tabId); | ||
| } else { | ||
| this.contentView.removeChildView(tabView); | ||
| this.allLoadedTabViews.delete(tabId); | ||
| } | ||
| this.contentView.removeChildView(tabView); | ||
| this.allLoadedTabViews.delete(tabId); | ||
| tabView.destroy(); |
There was a problem hiding this comment.
Potential issue: Missing contentView.removeChildView for cached tabs.
When removing a cached tab (lines 652-659), contentView.removeChildView(tabView) is not called before destroy(). However, cached tabs were never removed from contentView during the workspace switch—they were only positioned off-screen.
This could potentially cause issues when destroying a view that's still attached to its parent. Consider adding the removal:
🔎 Proposed fix
tabView = this.allTabViewsCache.get(tabId);
if (tabView == null) {
console.log("removeTabView -- tabView not found in loaded or cache", tabId, this.waveWindowId);
return;
}
console.log("removeTabView -- removing from cache", tabId, this.waveWindowId);
+ if (!this.isDestroyed()) {
+ this.contentView.removeChildView(tabView);
+ }
this.allTabViewsCache.delete(tabId);
} else {
this.contentView.removeChildView(tabView);📝 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.
| let tabView = this.allLoadedTabViews.get(tabId); | |
| if (tabView == null) { | |
| console.log("removeTabView -- tabView not found", tabId, this.waveWindowId); | |
| // the tab was never loaded, so just return | |
| return; | |
| // Check cache - tab might be from a different workspace | |
| tabView = this.allTabViewsCache.get(tabId); | |
| if (tabView == null) { | |
| console.log("removeTabView -- tabView not found in loaded or cache", tabId, this.waveWindowId); | |
| return; | |
| } | |
| console.log("removeTabView -- removing from cache", tabId, this.waveWindowId); | |
| this.allTabViewsCache.delete(tabId); | |
| } else { | |
| this.contentView.removeChildView(tabView); | |
| this.allLoadedTabViews.delete(tabId); | |
| } | |
| this.contentView.removeChildView(tabView); | |
| this.allLoadedTabViews.delete(tabId); | |
| tabView.destroy(); | |
| let tabView = this.allLoadedTabViews.get(tabId); | |
| if (tabView == null) { | |
| // Check cache - tab might be from a different workspace | |
| tabView = this.allTabViewsCache.get(tabId); | |
| if (tabView == null) { | |
| console.log("removeTabView -- tabView not found in loaded or cache", tabId, this.waveWindowId); | |
| return; | |
| } | |
| console.log("removeTabView -- removing from cache", tabId, this.waveWindowId); | |
| if (!this.isDestroyed()) { | |
| this.contentView.removeChildView(tabView); | |
| } | |
| this.allTabViewsCache.delete(tabId); | |
| } else { | |
| this.contentView.removeChildView(tabView); | |
| this.allLoadedTabViews.delete(tabId); | |
| } | |
| tabView.destroy(); |
🤖 Prompt for AI Agents
In emain/emain-window.ts around lines 650 to 664, when a tabView is found in the
cache you delete it from allTabViewsCache and immediately call
tabView.destroy(), but you never remove it from contentView; before destroying a
cached tab you should call this.contentView.removeChildView(tabView) (and then
delete it from this.allTabViewsCache) so the view is detached from its parent
prior to destroy to avoid destroying an attached view.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "devDependencies": { | ||
| "@eslint/js": "^8.57.0", | ||
| "@rollup/plugin-node-resolve": "^16.0.3", | ||
| "@tailwindcss/vite": "^4.1.18", | ||
| "@tailwindcss/vite": "^4.1.17", | ||
| "@types/color": "^4.2.0", |
There was a problem hiding this comment.
The root package-lock now pins the Tailwind build tooling to 4.1.17 (@tailwindcss/vite and tailwindcss), while package.json still requires ^4.1.18. Because npm verifies that the lockfile satisfies the declared ranges, npm ci/install will now fail with an out-of-sync lockfile error until the lock is regenerated with the 4.1.18 range expected by package.json (and the tsunami/frontend workspace, which also still requests 4.1.18).
Useful? React with 👍 / 👎.
stevenvo
force-pushed
the
fix/preserve-terminal-state-workspace-switch
branch
from
2745ac1 to
d625a37
Compare
Fixes issue where TUI applications (vim, htop, 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. Solution: Cache tab views across workspace switches instead of destroying them. Tab views are positioned off-screen but kept alive, preserving all terminal state.
stevenvo
force-pushed
the
fix/preserve-terminal-state-workspace-switch
branch
from
d625a37 to
887aa79
Compare
|
Closing temporarily for additional testing. Will reopen after validation. |
1 participant
Summary
Fixes issue where TUI applications (vim, htop, opencode, etc.) would lose terminal state when switching between workspaces, causing inability to scroll and display corruption.
Problem
When switching workspaces, all tab views were being destroyed and recreated. This caused:
Root Cause
The switchworkspace operation in emain/emain-window.ts called removeAllChildViews() which destroyed all tab views including their xterm.js terminal instances. When recreated from cache, the terminal state was not properly restored.
Solution
Cache tab views across workspace switches instead of destroying them:
Changes
Testing
Behavior Match
Now matches macOS Terminal, iTerm2, and other terminal emulators where terminals remain active across tab/workspace switches.