update emain-window to do repositioning in a simpler way #3087
Conversation
… doesn't buy us anything)
WalkthroughThis PR updates two files. In electron.vite.config.ts the Vite dev-server watch ignore pattern changes from Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| const workspaceList = await WorkspaceService.ListWorkspaces(); | ||
|
|
||
| const workspaceHasWindow = !!workspaceList.find((wse) => wse.workspaceid === workspaceId)?.windowid; | ||
| const _workspaceHasWindow = !!workspaceList.find((wse) => wse.workspaceid === workspaceId)?.windowid; |
There was a problem hiding this comment.
WARNING: Unused variable - _workspaceHasWindow is computed but never used in the function. The variable was previously named workspaceHasWindow (without underscore prefix), suggesting it may have had usage that was accidentally removed. Either remove this variable or restore its intended usage.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 files)
The PR simplifies tab repositioning by replacing async Reviewed by minimax-m2.5-20260211 · 1,277,254 tokens |
![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)
emain/emain-window.ts (1)
483-510:⚠️ Potential issue | 🟡 MinorRemove the unused
repositionTabsSlowlymethod (lines 483-510).This method is no longer called anywhere in the codebase and has become dead code from the refactoring. It should be deleted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@emain/emain-window.ts` around lines 483 - 510, Delete the dead private method repositionTabsSlowly(delayMs: number) from the class: remove its entire implementation (the code referencing activeTabView, getContentBounds(), delay(...) and calling finalizePositioning()) since it is no longer referenced anywhere; ensure there are no remaining references to repositionTabsSlowly and run a quick compile to confirm no usages remain.
🧹 Nitpick comments (3)
emain/emain-window.ts (3)
465-466: MisleadingawaitonwebContents.send()which returnsvoid.
webContents.send()is a fire-and-forget IPC call that returnsvoid, not a Promise. Theawaithere does nothing and may mislead readers into thinking it waits for the message to be processed.This creates an asymmetry: new tabs wait for
waveReadyPromisebeforefinalizePositioning(), but reused tabs callfinalizePositioning()immediately. If this is intentional (reused tabs are already ready), consider removing theawaitfor clarity.♻️ Suggested fix
} else { console.log("reusing an existing tab, calling wave-init", tabView.waveTabId); - await tabView.webContents.send("wave-init", tabView.savedInitOpts); // reinit + tabView.webContents.send("wave-init", tabView.savedInitOpts); // reinit this.finalizePositioning(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@emain/emain-window.ts` around lines 465 - 466, The await on tabView.webContents.send("wave-init", ...) is misleading because webContents.send returns void; either remove the await to make the fire-and-forget behavior explicit, or (if you intended to wait for the renderer to be ready like for new tabs) replace the await on send with awaiting the renderer-ready promise used elsewhere (e.g., await tabView.waveReadyPromise or whatever waveReadyPromise is named) before calling this.finalizePositioning(); locate the send call and finalizePositioning() in emain-window.ts and apply one of these two fixes so the code’s behavior and intent are consistent.
546-550: Parameter_delayMsis unused and the function uses a hardcoded value.The parameter is marked unused but the timeout always uses
1000regardless of what would have been passed. If the parameter was intended to be used, consider:♻️ Option: Use the parameter or remove it from signature
- private removeTabViewLater(tabId: string, _delayMs: number) { + private removeTabViewLater(tabId: string, delayMs: number) { setTimeout(() => { this.removeTabView(tabId, false); - }, 1000); + }, delayMs); }Or if
1000msis intentional, remove the parameter:- private removeTabViewLater(tabId: string, _delayMs: number) { + private removeTabViewLater(tabId: string) { setTimeout(() => { this.removeTabView(tabId, false); }, 1000); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@emain/emain-window.ts` around lines 546 - 550, removeTabViewLater currently ignores its _delayMs parameter and always uses a hardcoded 1000ms; either use the passed value or drop the unused parameter. To fix, update removeTabViewLater to pass the delay to setTimeout (replace the literal 1000 with the delay parameter and consider renaming _delayMs to delayMs) so the call to this.removeTabView(tabId, false) runs after the requested delay, or remove the _delayMs parameter from the method signature and keep the 1000ms literal if that fixed delay is intentional.
821-821: Consider removing the unused variable entirely.
_workspaceHasWindowis computed but never used. The underscore prefix acknowledges this, but the variable could be removed entirely to simplify the code.♻️ Remove unused variable
const workspaceList = await WorkspaceService.ListWorkspaces(); - const _workspaceHasWindow = !!workspaceList.find((wse) => wse.workspaceid === workspaceId)?.windowid; - const choice = dialog.showMessageBoxSync(this, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@emain/emain-window.ts` at line 821, Remove the unused variable declaration _workspaceHasWindow — the computed value const _workspaceHasWindow = !!workspaceList.find((wse) => wse.workspaceid === workspaceId)?.windowid is never referenced, so delete that line (and any leftover underscore-prefixed placeholder) from emain-window.ts to simplify the code and avoid dead variables.
🤖 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 `@emain/emain-window.ts`:
- Around line 483-510: Delete the dead private method
repositionTabsSlowly(delayMs: number) from the class: remove its entire
implementation (the code referencing activeTabView, getContentBounds(),
delay(...) and calling finalizePositioning()) since it is no longer referenced
anywhere; ensure there are no remaining references to repositionTabsSlowly and
run a quick compile to confirm no usages remain.
---
Nitpick comments:
In `@emain/emain-window.ts`:
- Around line 465-466: The await on tabView.webContents.send("wave-init", ...)
is misleading because webContents.send returns void; either remove the await to
make the fire-and-forget behavior explicit, or (if you intended to wait for the
renderer to be ready like for new tabs) replace the await on send with awaiting
the renderer-ready promise used elsewhere (e.g., await tabView.waveReadyPromise
or whatever waveReadyPromise is named) before calling
this.finalizePositioning(); locate the send call and finalizePositioning() in
emain-window.ts and apply one of these two fixes so the code’s behavior and
intent are consistent.
- Around line 546-550: removeTabViewLater currently ignores its _delayMs
parameter and always uses a hardcoded 1000ms; either use the passed value or
drop the unused parameter. To fix, update removeTabViewLater to pass the delay
to setTimeout (replace the literal 1000 with the delay parameter and consider
renaming _delayMs to delayMs) so the call to this.removeTabView(tabId, false)
runs after the requested delay, or remove the _delayMs parameter from the method
signature and keep the 1000ms literal if that fixed delay is intentional.
- Line 821: Remove the unused variable declaration _workspaceHasWindow — the
computed value const _workspaceHasWindow = !!workspaceList.find((wse) =>
wse.workspaceid === workspaceId)?.windowid is never referenced, so delete that
line (and any leftover underscore-prefixed placeholder) from emain-window.ts to
simplify the code and avoid dead variables.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1091022b-ecd7-4b80-8f64-4913276d9c06
📒 Files selected for processing (2)
electron.vite.config.tsemain/emain-window.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@emain/emain-window.ts`:
- Around line 396-398: The new WaveTabView (tabView) is being added at full
window bounds before initialization finishes, making it topmost and obscuring
the current tab; set the tab off-screen before adding it and only let it move
on-screen after initialization completes. Specifically, call
tabView.setBounds(...) with off-screen coordinates (e.g., x: -9999, y: 0, width:
winBounds.width, height: winBounds.height or similar off-screen x) before
this.contentView.addChildView(tabView), then wait for waveReadyPromise and call
tabView.finalizePositioning() (or the existing positioning helper) to set the
real on-screen bounds and z-order once init succeeds; use getContentBounds(),
tabView.setBounds, this.contentView.addChildView, waveReadyPromise, and
tabView.finalizePositioning() to implement this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4bc1bc7e-7d2f-49dd-b121-d4ae4b9b91a3
📒 Files selected for processing (1)
emain/emain-window.ts
| const winBounds = this.getContentBounds(); | ||
| tabView.setBounds({ x: 0, y: 0, width: winBounds.width, height: winBounds.height }); | ||
| this.contentView.addChildView(tabView); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check file size and structure
wc -l emain/emain-window.tsRepository: wavetermdev/waveterm
Length of output: 89
🏁 Script executed:
# Read the relevant section around lines 396-398 with context
sed -n '380,420p' emain/emain-window.tsRepository: wavetermdev/waveterm
Length of output: 1801
🏁 Script executed:
# Look for the initializeTab method definition
rg -n "initializeTab" emain/emain-window.ts -B 2 -A 15Repository: wavetermdev/waveterm
Length of output: 1805
🏁 Script executed:
# Search for waveReadyPromise usage
rg -n "waveReadyPromise" emain/emain-window.ts -B 2 -A 2Repository: wavetermdev/waveterm
Length of output: 338
🏁 Script executed:
# Look for finalizePositioning method
rg -n "finalizePositioning" emain/emain-window.ts -B 2 -A 10Repository: wavetermdev/waveterm
Length of output: 1898
🏁 Script executed:
# Find the WaveTabView class and understand positionTabOnScreen/positionTabOffScreen
rg -n "positionTabOnScreen|positionTabOffScreen" emain/emain-window.ts -B 2 -A 5Repository: wavetermdev/waveterm
Length of output: 1568
🏁 Script executed:
# Check what happens before and after initializeTab is called - look at setTabViewIntoWindow
rg -n "setTabViewIntoWindow" emain/emain-window.ts -B 5 -A 30Repository: wavetermdev/waveterm
Length of output: 3304
🏁 Script executed:
# Search for where activeTabView is set to understand which tab becomes active
rg -n "activeTabView\s*=" emain/emain-window.ts -B 2 -A 2Repository: wavetermdev/waveterm
Length of output: 971
🏁 Script executed:
# Look for any other view positioning or visibility logic around initialization
rg -n "addChildView|removeChildView|setVisible" emain/emain-window.ts -B 1 -A 1Repository: wavetermdev/waveterm
Length of output: 506
🏁 Script executed:
# Search for the WaveTabView class definition and positionTabOnScreen implementation
rg -n "class WaveTabView" emain/ -A 30Repository: wavetermdev/waveterm
Length of output: 2124
🏁 Script executed:
# Look for positionTabOnScreen and positionTabOffScreen in the codebase
rg -n "positionTabOnScreen\s*(" emain/ -A 10Repository: wavetermdev/waveterm
Length of output: 160
🏁 Script executed:
# Check if there's any initialization of the WaveTabView that might affect visibility
rg -n "new WaveTabView" emain/ -B 5 -A 10Repository: wavetermdev/waveterm
Length of output: 1277
🏁 Script executed:
# Check what getOrCreateWebViewForTab does and if it initializes the view
rg -n "getOrCreateWebViewForTab" emain/ -A 20Repository: wavetermdev/waveterm
Length of output: 4365
🏁 Script executed:
# Search for positionTabOnScreen and positionTabOffScreen definitions
rg -n "positionTabOnScreen|positionTabOffScreen" emain/emain-tabview.ts -A 15Repository: wavetermdev/waveterm
Length of output: 981
🏁 Script executed:
# Check if there's any visibility or z-order related code in WaveTabView
rg -n "setVisible|zIndex|isVisible" emain/emain-tabview.ts -B 2 -A 5Repository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
# Look for any initialization that might set the view's initial position
rg -n "setBounds" emain/emain-tabview.ts -B 3 -A 3Repository: wavetermdev/waveterm
Length of output: 402
🏁 Script executed:
# Check if there's any logic that positions the old tab off-screen BEFORE adding the new tab
rg -n "positionTabOffScreen|removeChildView" emain/emain-window.ts -B 3 -A 2Repository: wavetermdev/waveterm
Length of output: 755
🏁 Script executed:
# Look for any code that might position tabs off-screen when they lose focus or become inactive
rg -n "isActiveTab\s*=" emain/emain-window.ts -B 5 -A 5Repository: wavetermdev/waveterm
Length of output: 715
🏁 Script executed:
# Check the full context of where finalizePositioning is called relative to setTabViewIntoWindow
sed -n '448,475p' emain/emain-window.tsRepository: wavetermdev/waveterm
Length of output: 1349
🏁 Script executed:
# Final verification: Check if there are any other similar patterns or related code
rg -n "addChildView" emain/emain-window.ts -B 5 -A 2Repository: wavetermdev/waveterm
Length of output: 591
🏁 Script executed:
# Verify if positionTabOffScreen is the intended pattern
rg -n "positionTabOffScreen\(" emain/emain-window.tsRepository: wavetermdev/waveterm
Length of output: 121
Attach the new WaveTabView off-screen until initialization completes.
Lines 397–398 position the tab at full window bounds before waveReadyPromise resolves. Since addChildView appends to the child list and Electron uses child order for z-layer ordering, this uninitialized tab becomes topmost and obscures the current tab during slow initialization or workspace switches. Move it off-screen here and let finalizePositioning() bring it on-screen only after init succeeds.
💡 Suggested change
const winBounds = this.getContentBounds();
- tabView.setBounds({ x: 0, y: 0, width: winBounds.width, height: winBounds.height });
+ tabView.setBounds({ x: -15000, y: -15000, width: winBounds.width, height: winBounds.height });
this.contentView.addChildView(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.
| const winBounds = this.getContentBounds(); | |
| tabView.setBounds({ x: 0, y: 0, width: winBounds.width, height: winBounds.height }); | |
| this.contentView.addChildView(tabView); | |
| const winBounds = this.getContentBounds(); | |
| tabView.setBounds({ x: -15000, y: -15000, width: winBounds.width, height: winBounds.height }); | |
| this.contentView.addChildView(tabView); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@emain/emain-window.ts` around lines 396 - 398, The new WaveTabView (tabView)
is being added at full window bounds before initialization finishes, making it
topmost and obscuring the current tab; set the tab off-screen before adding it
and only let it move on-screen after initialization completes. Specifically,
call tabView.setBounds(...) with off-screen coordinates (e.g., x: -9999, y: 0,
width: winBounds.width, height: winBounds.height or similar off-screen x) before
this.contentView.addChildView(tabView), then wait for waveReadyPromise and call
tabView.finalizePositioning() (or the existing positioning helper) to set the
real on-screen bounds and z-order once init succeeds; use getContentBounds(),
tabView.setBounds, this.contentView.addChildView, waveReadyPromise, and
tabView.finalizePositioning() to implement this change.
1 participant
the old way doesn't buy us anything, so we should just do it in a straightforward way.