QoL and Layout Fixes for Windows #2661
Conversation
sawka
commented
Dec 11, 2025
sawka
commented
- remove title bar on windows windows
- re-integrate native controls into our tab bar
- remove menu completely (weird "Alt" activation)
- actually fix Ctrl-V in terminal
- clamp zoom levels better (0.4 - 2.6)
- simplify macos / windows drag regions (keep responsive to zoom)
- fix settings schemas for windows
WalkthroughThis PR adds a telemetry field Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Areas to pay extra attention to: platform-specific window appearance and transparency/blur fallbacks, drag-region sizing and visual regressions, telemetry correctness for Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
emain/emain-menu.ts (1)
252-252: Consider extracting zoom bounds to named constants.The zoom bounds (min: 0.4, max: 2.6) are hardcoded in four locations. While the values are now consistent with the PR objective to clamp zoom levels, consider extracting them to named constants at the top of the file for better maintainability.
Apply this refactor to reduce duplication:
+const MIN_ZOOM_FACTOR = 0.4; +const MAX_ZOOM_FACTOR = 2.6; + function makeEditMenu(fullConfig?: FullConfigType): Electron.MenuItemConstructorOptions[] {Then update the four locations:
- const newZoom = Math.min(2.6, wc.getZoomFactor() + 0.2); + const newZoom = Math.min(MAX_ZOOM_FACTOR, wc.getZoomFactor() + 0.2);- const newZoom = Math.max(0.4, wc.getZoomFactor() - 0.2); + const newZoom = Math.max(MIN_ZOOM_FACTOR, wc.getZoomFactor() - 0.2);Also applies to: 265-265, 280-280, 293-293
frontend/app/view/term/term-model.ts (1)
500-517: Logic is correct; consider making method private.The platform-aware Ctrl-V paste handling logic is well-implemented:
- macOS correctly excluded (uses Cmd-V)
- Setting
app:ctrlvpasteis properly respected- Sensible platform defaults (Windows=true, Linux=false)
- Null vs. explicit false distinction handled correctly
Since this method appears to only be used within
handleTerminalKeydown(line 550), consider making it private unless it needs to be exposed for testing or external use.Apply this diff to make the method private:
- shouldHandleCtrlVPaste(): boolean { + private shouldHandleCtrlVPaste(): boolean {emain/emain-ipc.ts (2)
15-15: IPC wiring for terminal command telemetry looks good; double‑check async behaviorThe new
increment-term-commandsIPC channel andincrementTermCommandsRunimport are wired correctly and keep the handler minimal. IfincrementTermCommandsRunis (or may become) async, consider wrapping it infireAndForget(or adding explicit error handling) so any rejected promise doesn’t surface as an unhandled rejection:- electron.ipcMain.on("increment-term-commands", () => { - incrementTermCommandsRun(); - }); + electron.ipcMain.on("increment-term-commands", () => { + // Wrap in fireAndForget if incrementTermCommandsRun returns a Promise + fireAndForget(incrementTermCommandsRun); + });Also verify the frontend uses the same
"increment-term-commands"channel name so this path is actually hit.Also applies to: 410-412
315-338: Config‑gated overlay update matches intent; consider making it more robustThe new guard:
const fullConfig = await RpcApi.GetFullConfigCommand(ElectronWshClient); if (fullConfig.settings["window:nativetitlebar"] && unamePlatform !== "win32") return;correctly skips overlay updates on non‑Windows when the native title bar is enabled, while still allowing overlay behavior on Windows as desired for this PR.
Two robustness nits you may want to address:
- Defensive access to settings
If
fullConfig.settingscan ever benull/undefined, this will throw. Using optional chaining is safer:- if (fullConfig.settings["window:nativetitlebar"] && unamePlatform !== "win32") return; + if (fullConfig.settings?.["window:nativetitlebar"] && unamePlatform !== "win32") return;
- Guard against GetFullConfig/overlay failures
This handler now awaits a remote config call in addition to
capturePage. If either rejects, you may get unhandled rejections since Electron doesn’t await the listener. Wrapping the body intry/catchand logging/early‑returning would make this more resilient:- electron.ipcMain.on("update-window-controls-overlay", async (event, rect: Dimensions) => { - const fullConfig = await RpcApi.GetFullConfigCommand(ElectronWshClient); - if (fullConfig.settings?.["window:nativetitlebar"] && unamePlatform !== "win32") return; - const zoomFactor = event.sender.getZoomFactor(); + electron.ipcMain.on("update-window-controls-overlay", async (event, rect: Dimensions) => { + try { + const fullConfig = await RpcApi.GetFullConfigCommand(ElectronWshClient); + if (fullConfig.settings?.["window:nativetitlebar"] && unamePlatform !== "win32") { + return; + } + const zoomFactor = event.sender.getZoomFactor(); // ... rest of logic unchanged ... - const overlay = await event.sender.capturePage(electronRect); + const overlay = await event.sender.capturePage(electronRect); + // ... + } catch (err) { + console.error("update-window-controls-overlay failed", err); + } });This keeps the new behavior while avoiding crashes if config or capture fail.
frontend/app/workspace/widgets.tsx (1)
527-534: Inconsistentdev-labelclass usage.The visible dev label element (line 483) no longer uses the
dev-labelclass, but this measurement div still references it. Since the.dev-labelstyling was removed fromtabbar.scss, this class has no effect. Consider removing it for consistency:- <div - className="dev-label flex justify-center items-center w-full py-1 text-accent text-[30px]" + <div + className="flex justify-center items-center w-full py-1 text-accent text-[30px]" title="Running Wave Dev Build" >frontend/app/tab/tabbar.tsx (1)
662-680: Simplify redundant else branches and consider edge case.The else branches assign the same value as the if branches, making them redundant. Also, consider what happens if
zoomFactoris exactly 0 (though unlikely):// Calculate window drag left width based on platform and state let windowDragLeftWidth = 10; if (isMacOS() && !isFullScreen) { - if (zoomFactor > 0) { - windowDragLeftWidth = 74 / zoomFactor; - } else { - windowDragLeftWidth = 74; - } + windowDragLeftWidth = zoomFactor > 0 ? 74 / zoomFactor : 74; } // Calculate window drag right width let windowDragRightWidth = 6; if (isWindows()) { - if (zoomFactor > 0) { - windowDragRightWidth = 139 / zoomFactor; - } else { - windowDragRightWidth = 139; - } + windowDragRightWidth = zoomFactor > 0 ? 139 / zoomFactor : 139; }emain/emain-window.ts (1)
162-204: Consider extracting common transparency/background logic.The transparency and background color handling is duplicated across all three platform branches. While each platform has different blur implementations, the structure could be simplified.
Consider extracting a helper function:
function applyTransparencySettings( winOpts: BaseWindowConstructorOptions, platform: NodeJS.Platform, isTransparent: boolean, isBlur: boolean ) { if (isTransparent) { winOpts.transparent = true; } else if (isBlur) { if (platform === "darwin") { winOpts.vibrancy = "fullscreen-ui"; } else if (platform === "win32") { winOpts.backgroundMaterial = "acrylic"; } // Linux doesn't support blur } else { winOpts.backgroundColor = "#222222"; } }Then call it after each platform's specific settings:
if (opts.unamePlatform === "darwin") { winOpts.titleBarStyle = "hiddenInset"; winOpts.titleBarOverlay = false; winOpts.autoHideMenuBar = !settings?.["window:showmenubar"]; applyTransparencySettings(winOpts, opts.unamePlatform, isTransparent, isBlur); } // ... etcemain/emain-tabview.ts (2)
16-82: Windows accelerator handler looks solid; consider minor robustness tweaksThis centralizes Windows-specific shortcuts cleanly and matches the PR goals (new window, reload, paste, zoom clamping, fullscreen, workspace switching, devtools). Logic and bounds for zoom (0.4–2.6) look correct.
Two small, non-blocking robustness ideas:
- For
Ctrl+Shift+N,createNewWaveWindow()is async; if your runtime treats unhandled rejections strictly, you might want to eithervoid createNewWaveWindow();with an internaltry/catch, or attach.catchhere to log failures.- In the workspace-switch block,
waveWindowis captured before the RPC completes. If the window is closed beforeWorkspaceListCommandresolves,switchWorkspacecould run on a stale window. Re-resolving the window inside the.then(viagetWaveWindowById(tabView.waveWindowId)) before callingswitchWorkspacewould make this more resilient.These are polish-level; behavior as written is reasonable.
320-327:before-input-eventintegration is correct; small readability nitThe ordering here is good: chord-mode handling runs first, then Windows-only accelerators on keyDown, with
e.preventDefault()when handled. That should prevent events from leaking into the renderer when a shortcut is consumed.If you want a tiny readability tweak, you could early-return for non-Windows or non-
keyDownevents (e.g., guard clause at the top of the handler) to reduce nesting, but that’s purely stylistic.
📜 Review details
Configuration used: CodeRabbit 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 (20)
cmd/server/main-server.go(2 hunks)emain/emain-ipc.ts(3 hunks)emain/emain-menu.ts(7 hunks)emain/emain-tabview.ts(2 hunks)emain/emain-window.ts(3 hunks)emain/emain.ts(3 hunks)frontend/app/element/windowdrag.scss(0 hunks)frontend/app/element/windowdrag.tsx(0 hunks)frontend/app/modals/messagemodal.scss(1 hunks)frontend/app/modals/modal.tsx(4 hunks)frontend/app/store/global.ts(3 hunks)frontend/app/tab/tabbar.scss(1 hunks)frontend/app/tab/tabbar.tsx(13 hunks)frontend/app/view/codeeditor/schemaendpoints.ts(1 hunks)frontend/app/view/term/term-model.ts(3 hunks)frontend/app/view/waveconfig/waveconfig.tsx(1 hunks)frontend/app/workspace/widgets.tsx(2 hunks)frontend/tailwindsetup.css(1 hunks)frontend/types/custom.d.ts(1 hunks)pkg/telemetry/telemetrydata/telemetrydata.go(1 hunks)
💤 Files with no reviewable changes (2)
- frontend/app/element/windowdrag.tsx
- frontend/app/element/windowdrag.scss
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T05:09:26.916Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2465
File: frontend/app/onboarding/onboarding-upgrade.tsx:13-21
Timestamp: 2025-10-21T05:09:26.916Z
Learning: In the waveterm codebase, clientData is loaded and awaited in wave.ts before React runs, ensuring it is always available when components mount. This means atoms.client will have data on first render.
Applied to files:
frontend/app/store/global.ts
🧬 Code graph analysis (7)
frontend/app/view/term/term-model.ts (1)
frontend/app/store/global.ts (2)
getSettingsKeyAtom(855-855)getApi(846-846)
emain/emain-window.ts (2)
docs/src/components/platformcontext.tsx (1)
Platform(7-7)emain/emain-platform.ts (1)
getElectronAppBasePath(272-272)
emain/emain-tabview.ts (5)
pkg/vdom/vdom_types.go (1)
WaveKeyboardEvent(233-247)emain/emain-window.ts (2)
getWaveWindowById(656-658)createNewWaveWindow(804-834)frontend/util/keyutil.ts (1)
checkKeyPressed(327-327)emain/emain-wsh.ts (1)
ElectronWshClient(116-116)emain/emain-platform.ts (1)
unamePlatform(283-283)
emain/emain-ipc.ts (1)
emain/emain-platform.ts (1)
unamePlatform(283-283)
frontend/app/tab/tabbar.tsx (1)
frontend/util/platformutil.ts (2)
isWindows(16-18)isMacOS(12-14)
emain/emain-menu.ts (1)
emain/emain-platform.ts (1)
unamePlatform(283-283)
cmd/server/main-server.go (2)
pkg/telemetry/telemetrydata/telemetrydata.go (1)
TEventUserProps(64-90)pkg/telemetry/telemetry.go (1)
AutoUpdateChannel(107-110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (29)
emain/emain-menu.ts (3)
126-130: LGTM — Formatting change only.The function signature formatting has been updated, but there's no behavioral change.
387-396: LGTM — Windows menu guard aligns with PR objectives.The function rename from
makeAppMenutomakeAndSetAppMenuand the Windows platform guard correctly implement the PR objective to remove the application menu on Windows to avoid Alt activation behavior.
400-400: LGTM — Event handler updated consistently.The event handler correctly references the renamed function.
emain/emain.ts (1)
26-26: LGTM — Import and call sites updated consistently.The import statement and both call sites have been updated to use the renamed
makeAndSetAppMenufunction. The changes are consistent with the modifications inemain/emain-menu.ts, and the Windows platform guard in that function ensures the menu is not set on Windows as intended.Also applies to: 321-321, 363-363
frontend/app/modals/modal.tsx (3)
25-40: Modal parameter destructuring refactor looks good.The multi-line destructuring formatting improves readability without changing semantics. Parameter order and ref handling are correct for React's forwardRef pattern.
92-99: ModalFooter destructuring refactor is clean.The expanded formatting with default values is idiomatic React. The
cancelLabel = "Cancel"andokLabel = "Ok"defaults are preserved correctly.
122-127: FlexiModalComponent interface formatting is correct.The multi-line generic extends clause is valid TypeScript and improves readability. Content and Footer type references are properly maintained.
frontend/app/view/term/term-model.ts (2)
29-29: LGTM! Platform utilities imported correctly.The platform detection imports are appropriate for the new Ctrl-V paste handling logic.
548-556: LGTM! Ctrl-V paste integration is correct.The integration into
handleTerminalKeydownfollows the existing pattern for Ctrl-Shift-V paste (lines 557-562):
- Properly prevents default behavior and stops propagation
- Calls the appropriate API method (
nativePaste())- Positioned logically in the event handler flow
The platform-aware check ensures Ctrl-V paste works as expected on Windows by default while respecting user settings.
emain/emain-ipc.ts (1)
29-38: Builder window lookup refactor preserves behaviorUsing
.findwith a normalizedappIdis clear and equivalent to the previous logic, and keeps the fast path (focusandreturn) unchanged. No issues here.pkg/telemetry/telemetrydata/telemetrydata.go (1)
84-89: LGTM!The new
SettingsTransparentfield follows the established naming conventions and JSON tag patterns. Theomitemptytag is appropriate for this boolean telemetry field.frontend/types/custom.d.ts (1)
25-25: LGTM!The
zoomFactorAtomtype declaration correctly usesjotai.PrimitiveAtom<number>and is appropriately placed alongside other UI-related atoms.frontend/app/tab/tabbar.scss (1)
39-46: LGTM!The simplified
.tab-bar-rightstyling withmargin-left: autocorrectly pushes the right-side elements to the edge in the flexbox layout. The removal of the.window-dragblock aligns with the new inline platform-aware drag handles intabbar.tsx.cmd/server/main-server.go (2)
231-241: Consider addingSettingsTransparentto telemetry counts for consistency.The startup activity update (line 330) populates
SettingsTransparent, but the telemetry counts update here does not. If the window transparency setting can change during runtime, the counts telemetry should also track it.If the transparency setting can only be applied at startup and cannot be changed at runtime, this is fine. Otherwise, consider adding:
props.UserSet = &telemetrydata.TEventUserProps{ SettingsCustomWidgets: customWidgets, SettingsCustomAIPresets: customAIPresets, SettingsCustomSettings: customSettings, SettingsCustomAIModes: customAIModes, + SettingsTransparent: fullConfig.Settings.WindowTransparent, }
318-331: LGTM!The
fullConfigretrieval andSettingsTransparentpopulation follow the established pattern for telemetry enrichment.frontend/app/workspace/widgets.tsx (2)
8-8: LGTM!Good cleanup removing the unused
getApiimport.
481-488: LGTM!The dev label styling has been simplified to use inline Tailwind classes.
frontend/app/store/global.ts (3)
79-87: LGTM!The
zoomFactorAtominitialization follows the established pattern with proper error handling. The default value of1.0(100% zoom) is sensible, and theonZoomFactorChangecallback keeps the atom synchronized with the Electron main process.
75-77: Good improvement adding error logging.The added error logging for
isFullScreenAtom,onMenuItemAbout, andupdaterStatusAtominitialization failures improves debuggability without changing behavior.Also applies to: 93-95, 148-150
200-200: LGTM!The
zoomFactorAtomis correctly exposed in the atoms object, making it available throughout the application.frontend/app/tab/tabbar.tsx (4)
22-24: LGTM!The constant renames to PascalCase (
TabDefaultWidth,TabMinWidth,OSOptions) follow JavaScript/TypeScript conventions for constants better than the previous SCREAMING_CASE style.
45-64: LGTM!Good extraction of
WaveAIButtoninto a memoized component. The styling correctly uses Tailwind classes and theWebkitAppRegion: "no-drag"ensures the button is clickable within the drag region.
691-705: LGTM!The inline drag handles with platform-aware widths and
WebkitAppRegion: "drag"correctly replace the removedWindowDragcomponent. The conditional rendering for the app menu button based on platform is clean.
738-742: LGTM!The right drag area for Windows native controls integration is correctly implemented with the calculated width based on zoom factor.
frontend/app/view/waveconfig/waveconfig.tsx (1)
275-283: Config editorfileNamechange aligns with new schema pathsSwitching to
fileName={WAVECONFIGPATH/${selectedFile.path}}matches the new staticWAVECONFIGPATH-based schema fileMatch patterns and avoids OS-specific path issues, especially on Windows. Looks good and consistent with the schema refactor.frontend/app/view/codeeditor/schemaendpoints.ts (1)
12-18: StaticWAVECONFIGPATHglobs simplify cross‑platform schema matchingThe move to static
*/WAVECONFIGPATH/...fileMatch entries is a good simplification and should avoid Windows path separator issues that dynamic configDir-based paths had. One thing to double‑check end‑to‑end: that the URI/filename that Monaco (or your JSON language service) sees for these models actually includes/WAVECONFIGPATH/<relativePath>(e.g. via thefileNamepassed fromWaveConfigView), so patterns like*/WAVECONFIGPATH/settings.jsondo in fact match the models you create.emain/emain-window.ts (2)
44-47: LGTM: Formatting improvement.The reformatting of the conditional improves readability without changing behavior.
207-210: LGTM: Windows menu removal aligns with PR objectives.Removing the menu on Windows post-construction addresses the "odd Alt activation behavior" mentioned in the PR description. This approach is correct since Electron doesn't have a window option to prevent the default menu during construction—it must be removed explicitly after the window is created.
emain/emain-tabview.ts (1)
5-13: New imports are consistent with usageThe additional imports (
checkKeyPressed,createNewWaveWindow,unamePlatform) match their usage below, and there are no apparent circular or unused-import issues in this file.
|
|
||
| export type WindowOpts = { | ||
| unamePlatform: string; | ||
| unamePlatform: NodeJS.Platform; |
There was a problem hiding this comment.
Consider adding exhaustive platform handling or constraining the type.
The type change from string to NodeJS.Platform improves type safety. However, NodeJS.Platform includes values beyond darwin, linux, and win32 (e.g., aix, android, freebsd, openbsd, etc.). The window initialization logic at lines 165-204 only handles these three platforms explicitly, leaving other platforms without proper window configuration.
Option 1: Add a default case with a warning
export type WindowOpts = {
unamePlatform: NodeJS.Platform;
isPrimaryStartupWindow?: boolean;
foregroundWindow?: boolean;
};Then in the constructor (after line 204):
} else if (opts.unamePlatform === "win32") {
winOpts.frame = true;
winOpts.titleBarStyle = "hidden";
winOpts.titleBarOverlay = {
color: "#222222",
symbolColor: "#c3c8c2",
height: 32,
};
if (isTransparent) {
winOpts.transparent = true;
} else if (isBlur) {
winOpts.backgroundMaterial = "acrylic";
} else {
winOpts.backgroundColor = "#222222";
}
+ } else {
+ console.warn(`Unsupported platform: ${opts.unamePlatform}, using default window options`);
+ winOpts.backgroundColor = "#222222";
}Option 2: Constrain the type to supported platforms
-export type WindowOpts = {
- unamePlatform: NodeJS.Platform;
+export type SupportedPlatform = "darwin" | "linux" | "win32";
+
+export type WindowOpts = {
+ unamePlatform: SupportedPlatform;
isPrimaryStartupWindow?: boolean;
foregroundWindow?: boolean;
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In emain/emain-window.ts around line 26, the unamePlatform property is typed as
NodeJS.Platform but the window initialization logic (lines ~165-204) only
handles darwin, linux and win32, leaving other NodeJS.Platform values unhandled;
either (A) add a default branch in the platform switch that logs a warning and
falls back to a safe, generic window configuration so unknown platforms get
reasonable behavior, or (B) constrain the property type to the supported union
('darwin' | 'linux' | 'win32') and validate/throw if process.platform is outside
that set; implement one of these fixes and ensure any warning or thrown error
includes clear context for debugging.
| footer { | ||
| padding: 10px; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all footer-related styles in SCSS/CSS files
echo "=== Footer-related styles in SCSS/CSS ==="
rg -n '\bfooter\b.*\{|\.footer|\.modal-footer' --type=scss --type=css -B 1 -A 3
echo ""
echo "=== Footer element usage in modal components ==="
rg -n '<footer' --type=tsx --type=ts frontend/app/modals/ -B 2 -A 2
echo ""
echo "=== All messagemodal.scss content ==="
cat -n frontend/app/modals/messagemodal.scssRepository: wavetermdev/waveterm
Length of output: 499
🏁 Script executed:
#!/bin/bash
# Search for footer styles without type restrictions
echo "=== Footer-related styles in SCSS files ==="
find frontend -name "*.scss" -type f | xargs grep -n "footer" 2>/dev/null
echo ""
echo "=== Footer element in TypeScript/TSX files ==="
find frontend -name "*.ts" -o -name "*.tsx" | xargs grep -n "<footer" 2>/dev/null | head -20
echo ""
echo "=== Search for modal-footer class references ==="
find frontend -type f \( -name "*.scss" -o -name "*.tsx" -o -name "*.ts" \) | xargs grep -n "modal-footer" 2>/dev/nullRepository: wavetermdev/waveterm
Length of output: 1457
🏁 Script executed:
#!/bin/bash
# Check the .modal-footer styles in both modal.scss files
echo "=== frontend/app/modals/modal.scss .modal-footer styles ==="
sed -n '60,75p' frontend/app/modals/modal.scss
echo ""
echo "=== frontend/app/element/modal.scss .modal-footer styles ==="
sed -n '49,65p' frontend/app/element/modal.scss
echo ""
echo "=== Full messagemodal.scss context ==="
cat frontend/app/modals/messagemodal.scssRepository: wavetermdev/waveterm
Length of output: 847
Use .modal-footer class selector instead of footer tag.
The footer element in modal.tsx (line 101) has className="modal-footer", not just a bare tag. The current selector .message-modal footer targets the tag by name, but the .modal-footer class styles defined in modal.scss have higher specificity and will override the 10px padding. Change line 7 from footer { to .modal-footer { to properly target the footer and ensure the padding applies.
🤖 Prompt for AI Agents
In frontend/app/modals/messagemodal.scss around lines 7 to 9, the rule uses the
footer tag selector which is being overridden by the existing .modal-footer
class styles; change the selector from the footer tag to .modal-footer so the
padding targets the element with className="modal-footer" in modal.tsx, ensuring
the 10px padding is applied and specificity matches the modal styles.
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)
frontend/app/tab/tabbar.tsx (1)
326-331: Window resize listener is never removed due to new arrow function instancesThe
useEffectis adding and removing different function instances:window.addEventListener("resize", () => handleResizeTabs()); // ... window.removeEventListener("resize", () => handleResizeTabs());Because the arrow functions are recreated each time, the
removeEventListenernever detaches the original handler, so resize listeners accumulate and keep stale closures alive.Consider storing the handler in a stable function:
- useEffect(() => { - window.addEventListener("resize", () => handleResizeTabs()); - return () => { - window.removeEventListener("resize", () => handleResizeTabs()); - }; - }, [handleResizeTabs]); + useEffect(() => { + const onResize = () => handleResizeTabs(); + window.addEventListener("resize", onResize); + return () => { + window.removeEventListener("resize", onResize); + }; + }, [handleResizeTabs]);This keeps only a single, up-to-date listener attached.
♻️ Duplicate comments (2)
emain/emain-window.ts (1)
26-26: Add default platform handling or constrain the type to supported platforms.The
unamePlatformproperty acceptsNodeJS.Platform, which includes platforms beyonddarwin,linux, andwin32(e.g.,aix,android,freebsd,openbsd,sunos). The window initialization logic at lines 165-203 only handles these three platforms explicitly, leaving other platforms without proper window configuration.Consider one of the approaches suggested in the previous review:
Option 1: Add a default case with fallback configuration after line 202:
} else { winOpts.backgroundColor = "#222222"; } + } else { + console.warn(`Unsupported platform: ${opts.unamePlatform}, using default window options`); + winOpts.backgroundColor = "#222222"; }Option 2: Constrain the type to supported platforms:
-export type WindowOpts = { - unamePlatform: NodeJS.Platform; +export type SupportedPlatform = "darwin" | "linux" | "win32"; + +export type WindowOpts = { + unamePlatform: SupportedPlatform; isPrimaryStartupWindow?: boolean; foregroundWindow?: boolean; };Also applies to: 165-203
frontend/app/tab/tabbar.tsx (1)
488-517: Remove or gate debugconsole.logcalls and avoid mutating state in-place during tab reorderingThere are several dev-style logs and some non-idiomatic state mutation in the drag/pin flow:
console.log("setting updated tabs", …)andconsole.log("updated pinnedTabIds", …)insetUpdatedTabsDebounced.console.log("handleDragStart", …)inhandleDragStart.console.log("handlePinChange", …)inhandlePinChange.These will be noisy in production and were previously flagged for a similar case (
zoomFactorlogging), so they should either be removed or switched to a structured logger behind a debug flag.In the same block,
tabIdsandpinnedTabIdsare mutated in-place (splice,add,delete) and then reused. While this currently “works” because other state updates (setDraggingTab) drive re-renders, it’s brittle and fights React’s expectation of immutable state.A safer pattern would be:
- let pinnedTabCount = pinnedTabIds.size; - const draggedTabId = draggingTabDataRef.current.tabId; - const isPinned = pinnedTabIds.has(draggedTabId); + let pinnedTabCount = pinnedTabIds.size; + const draggedTabId = draggingTabDataRef.current.tabId; + const isPinned = pinnedTabIds.has(draggedTabId); @@ - if (!isPinned && nextTabId && pinnedTabIds.has(nextTabId)) { - pinnedTabIds.add(draggedTabId); - } else if (isPinned && prevTabId && !pinnedTabIds.has(prevTabId)) { - pinnedTabIds.delete(draggedTabId); - } - if (pinnedTabCount != pinnedTabIds.size) { - console.log("updated pinnedTabIds", pinnedTabIds, tabIds); - setPinnedTabIds(pinnedTabIds); + const newPinned = new Set(pinnedTabIds); + if (!isPinned && nextTabId && newPinned.has(nextTabId)) { + newPinned.add(draggedTabId); + } else if (isPinned && prevTabId && !newPinned.has(prevTabId)) { + newPinned.delete(draggedTabId); + } + if (pinnedTabCount !== newPinned.size) { + setPinnedTabIds(newPinned); pinnedTabCount = pinnedTabIds.size; }And similarly, prefer cloning
tabIdswhen reordering rather than mutating the existing array.Also applies to: 561-585, 629-635
🧹 Nitpick comments (1)
frontend/app/tab/tabbar.tsx (1)
71-73: Config error UI refinements are safe layout changesThe updated
ConfigErrorMessagecopy/layout and theConfigErrorIconbutton className change are purely presentational and don’t alter the underlying logic for detecting and displaying config errors. Assuming thered/Tailwind classes exist in your styles, this all looks fine.Also applies to: 80-82, 89-91, 115-115
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emain/emain-window.ts(3 hunks)frontend/app/tab/tabbar.tsx(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
emain/emain-window.ts (2)
docs/src/components/platformcontext.tsx (1)
Platform(7-7)emain/emain-platform.ts (1)
getElectronAppBasePath(272-272)
frontend/app/tab/tabbar.tsx (1)
frontend/util/platformutil.ts (2)
isWindows(16-18)isMacOS(12-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
emain/emain-window.ts (3)
44-47: LGTM: Formatting improvement.The multi-line condition formatting enhances readability without changing behavior.
165-203: LGTM: Platform-specific window configuration is well-structured.The explicit per-platform blocks for
darwin,linux, andwin32improve clarity and maintainability. The Windows configuration correctly usestitleBarStyle: "hidden"withtitleBarOverlay(without explicitframesetting), following Electron's recommended patterns.
207-209: LGTM: Windows menu removal aligns with PR objectives.Calling
setMenu(null)for Windows after window construction correctly implements the PR objective to remove the application menu on Windows to avoid Alt activation issues.frontend/app/tab/tabbar.tsx (2)
9-9: Platform-specific constants and drag-region sizing look consistent with PR goalsThe switch to
isMacOS/isWindows, theTabDefaultWidth/TabMinWidth/OSOptionsconstants, and the zoom-aware left/right drag widths are wired throughsetSizeAndPositionand the JSX consistently. The non-tab width budget (drag regions, app menu, AI, workspace switcher, banners) is summed once and then used to clamp the per-tab width, which is solid and matches the Windows/macOS layout objectives.Also applies to: 22-24, 247-257, 270-271, 301-302, 432-436, 655-676, 686-691, 733-737
45-63: WaveAIButton extraction and integration look goodPulling the AI toggle out into a memoized
WaveAIButtonand dropping it into the tab bar keeps the render tree cleaner and reuses the existingWorkspaceLayoutModelAPI correctly. No functional issues from what’s shown.Also applies to: 701-701
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emain/emain-tabview.ts (1)
41-44: Consider extracting zoom reset logic to a helper function.The zoom reset logic here duplicates the pattern in
emain-menu.ts(lines 240-241) and is inconsistent with the use ofincreaseZoomLevelanddecreaseZoomLevelhelpers. Consider creating aresetZoomLevelutility inemain-util.tsfor consistency.Example implementation in
emain-util.ts:export function resetZoomLevel(webContents: electron.WebContents): void { webContents.setZoomFactor(1); webContents.send("zoom-factor-change", 1); }Then use it here and in
emain-menu.ts:- if (checkKeyPressed(waveEvent, "Ctrl:0")) { - tabView.webContents.setZoomFactor(1); - tabView.webContents.send("zoom-factor-change", 1); - return true; - } + if (checkKeyPressed(waveEvent, "Ctrl:0")) { + resetZoomLevel(tabView.webContents); + return true; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
emain/emain-ipc.ts(4 hunks)emain/emain-menu.ts(6 hunks)emain/emain-tabview.ts(2 hunks)emain/emain-util.ts(1 hunks)frontend/app/workspace/widgets.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/app/workspace/widgets.tsx
- emain/emain-ipc.ts
🧰 Additional context used
🧬 Code graph analysis (2)
emain/emain-tabview.ts (4)
emain/emain-window.ts (2)
getWaveWindowById(655-657)createNewWaveWindow(803-833)frontend/util/keyutil.ts (1)
checkKeyPressed(327-327)emain/emain-util.ts (2)
increaseZoomLevel(15-19)decreaseZoomLevel(21-25)emain/emain-platform.ts (1)
unamePlatform(283-283)
emain/emain-menu.ts (2)
emain/emain-util.ts (2)
increaseZoomLevel(15-19)decreaseZoomLevel(21-25)emain/emain-platform.ts (1)
unamePlatform(283-283)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (6)
emain/emain-util.ts (2)
11-13: LGTM! Zoom constants align with PR objectives.The constants properly implement the zoom clamping range (0.4–2.6) mentioned in the PR description, and the delta of 0.2 provides reasonable granularity.
15-25: LGTM! Zoom helper functions are well-implemented.The functions correctly clamp zoom levels to the defined bounds and emit events to keep the renderer synchronized. The implementation centralizes zoom control logic effectively.
emain/emain-tabview.ts (2)
23-85: LGTM! Windows accelerator handler comprehensively addresses menu removal.The function correctly implements Windows-specific keyboard shortcuts to compensate for the removed application menu (per PR objective). The Ctrl+V paste handling specifically addresses the terminal paste fix mentioned in the PR description.
326-331: LGTM! Clean integration of Windows accelerator handling.The Windows-specific accelerator handling is properly gated by platform check and integrated into the existing event flow. The
preventDefaultis correctly called only when a shortcut is handled.emain/emain-menu.ts (2)
250-251: LGTM! Zoom utilities are used correctly and consistently.The zoom helper functions are properly integrated into the menu handlers with appropriate null checks. This ensures consistent zoom behavior across the application.
Also applies to: 260-261, 272-273, 282-283
377-385: LGTM! Windows menu removal properly implements PR objective.The guard correctly prevents the application menu from being set on Windows, addressing the "odd Alt activation behavior" mentioned in the PR description. This coordinates well with the Windows-specific accelerator handling implemented in
emain-tabview.ts.
