Turns app:globalhotkey into a dedicated quake mode#3151
Turns app:globalhotkey into a dedicated quake mode#3151Midnight145 wants to merge 7 commits intowavetermdev:mainfrom
Conversation
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughA module-level nullable Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Notes:
Reviewed by minimax-m2.5-20260211 · 349,508 tokens Reviewed by minimax-m2.5-20260211 · 184,370 tokens |
![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
🧹 Nitpick comments (1)
emain/emain-window.ts (1)
964-971: Consider documenting or externalizing the transition delays.The hardcoded delays (120ms, 80ms) for fullscreen transitions are environment-dependent and may not be sufficient on all window managers or systems. A brief comment explaining why these values were chosen would help maintainability. The double
moveWindowToDisplaycall (before show and after delay) could also use a comment explaining why it's necessary.📝 Suggested documentation
+ // Delay needed for WM to process fullscreen exit before move quakeWindow.setFullScreen(false); await delay(120); moveWindowToDisplay(quakeWindow, targetDisplay); quakeWindow.show(); if (wasFullscreen) { + // Re-apply move after show to handle WMs that reset position await delay(80); moveWindowToDisplay(quakeWindow, targetDisplay); quakeWindow.setFullScreen(true); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@emain/emain-window.ts` around lines 964 - 971, Replace the magic numbers and unexplained duplicate move call with named constants and a short comment: extract the delays (120 and 80 ms) into descriptively named constants (e.g., FULLSCREEN_PRE_SHOW_DELAY_MS and FULLSCREEN_POST_SHOW_DELAY_MS) and add a comment near moveWindowToDisplay(quakeWindow, targetDisplay) + quakeWindow.show() explaining why the window is moved before showing and again after the small delay (to handle timing/race conditions with some window managers), and reference the wasFullscreen branch where quakeWindow.setFullScreen(true) is called so future maintainers know these values control the fullscreen transition timing and where to tweak them.
🤖 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 952-987: The hotkey handler can run concurrently causing flicker;
add a guard (e.g., a module-scoped boolean like isTogglingQuake or a simple
mutex) around the globalShortcut.register callback to return immediately if an
operation is already in progress, set the guard true before starting the async
sequence inside fireAndForget and ensure it is cleared in a finally block after
operations that reference quakeWindow, createNewWaveWindow, moveWindowToDisplay,
getDisplayForQuakeToggle, etc.; this prevents overlapping executions while
preserving existing behavior.
---
Nitpick comments:
In `@emain/emain-window.ts`:
- Around line 964-971: Replace the magic numbers and unexplained duplicate move
call with named constants and a short comment: extract the delays (120 and 80
ms) into descriptively named constants (e.g., FULLSCREEN_PRE_SHOW_DELAY_MS and
FULLSCREEN_POST_SHOW_DELAY_MS) and add a comment near
moveWindowToDisplay(quakeWindow, targetDisplay) + quakeWindow.show() explaining
why the window is moved before showing and again after the small delay (to
handle timing/race conditions with some window managers), and reference the
wasFullscreen branch where quakeWindow.setFullScreen(true) is called so future
maintainers know these values control the fullscreen transition timing and where
to tweak them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 601a194c-9623-42cf-8c88-03309cf094d8
📒 Files selected for processing (1)
emain/emain-window.ts
…ition with keybind spam
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 714-718: Currently createBrowserWindow() assigns the global
quakeWindow based on creation order; instead stop setting quakeWindow inside
createBrowserWindow() and add an explicit way for callers to designate the quake
window (e.g., accept a designateAsQuakeWindow boolean or expose a
setQuakeWindow(bwin) helper). Update relaunchBrowserWindows() to call
createBrowserWindow(...) for each window but pass designateAsQuakeWindow = true
when windowId === primaryWindowId (so the primary restored window becomes
quakeWindow), and update the hotkey path that creates a replacement quake window
to also set designateAsQuakeWindow = true or call setQuakeWindow(bwin) after
creation; ensure the global quakeWindow variable is only set by that explicit
mechanism (referencing quakeWindow, createBrowserWindow(),
relaunchBrowserWindows(), primaryWindowId).
- Around line 965-986: The toggle path reads quakeWindow, then awaits delays and
calls methods on it; capture the current window into a local (e.g., const win =
quakeWindow) before performing any awaits and, after each await (after
PRE_QUAKE_FULLSCREEN_DELAY_MS and POST_QUAKE_FULLSCREEN_DELAY_MS), validate that
win is still present and not destroyed (guard with if (!win ||
win.isDestroyed()) return; or abort the toggle flow) before calling
moveWindowToDisplay(win, ...), win.show(), win.setFullScreen(...), win.focus(),
and accessing win.activeTabView?.webContents.focus(); apply the same
local-capture-and-guard pattern around both await points to avoid dereferencing
a nulled/destroyed quakeWindow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 17e130fe-1b5a-4e2d-878f-791ba3057b99
📒 Files selected for processing (1)
emain/emain-window.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
emain/emain-window.ts (1)
714-718:⚠️ Potential issue | 🟠 MajorDon't pick the quake window by creation order.
relaunchBrowserWindows()restoreswindowIdsin reverse order, so this still designates the last restored window instead ofprimaryWindowId. After a relaunch, the hotkey can end up hiding/showing the wrong restored window. Please make the caller designate the quake window explicitly instead of inferring it here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@emain/emain-window.ts` around lines 714 - 718, Currently this file auto-assigns quakeWindow by creation order (quakeWindow, bwin, bwin.waveWindowId), which conflicts with relaunchBrowserWindows restoring windowIds in reverse; remove the automatic designation here so the caller explicitly sets quakeWindow (e.g., after relaunchBrowserWindows using primaryWindowId). Concretely: delete or disable the block that assigns quakeWindow = bwin (and the console.log), and rely on the caller to set quakeWindow (or pass an explicit flag/primaryWindowId to this module) rather than inferring from creation order.
🤖 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 926-946: moveWindowToDisplay currently only clamps x/y offsets so
when curBounds.width/height exceed targetDisplay.workArea the window stays too
large and extends off-screen; update moveWindowToDisplay to clamp
curBounds.width/height to targetArea.width/height (and adjust x/y after
clamping) before calling win.setBounds, e.g., compute newWidth =
Math.min(curBounds.width, targetArea.width) and newHeight =
Math.min(curBounds.height, targetArea.height), recalc maxXOffset/maxYOffset and
nextX/nextY using the clamped sizes, then call win.setBounds with {
...curBounds, width: newWidth, height: newHeight, x: nextX, y: nextY } so the
window fits fully inside targetArea.
---
Duplicate comments:
In `@emain/emain-window.ts`:
- Around line 714-718: Currently this file auto-assigns quakeWindow by creation
order (quakeWindow, bwin, bwin.waveWindowId), which conflicts with
relaunchBrowserWindows restoring windowIds in reverse; remove the automatic
designation here so the caller explicitly sets quakeWindow (e.g., after
relaunchBrowserWindows using primaryWindowId). Concretely: delete or disable the
block that assigns quakeWindow = bwin (and the console.log), and rely on the
caller to set quakeWindow (or pass an explicit flag/primaryWindowId to this
module) rather than inferring from creation order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 12db5393-d493-4011-8d62-c766216e32cd
📒 Files selected for processing (1)
emain/emain-window.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
emain/emain-window.ts (1)
989-992:⚠️ Potential issue | 🟡 MinorAdd a destroyed check before final focus operations.
After
show()and the optional fullscreen restore, the code accesseswindow.focus()andwindow.activeTabViewwithout a final destroyed check. While unlikely, the window could close during the show/fullscreen sequence.🛡️ Suggested hardening
- window.focus(); - if (window.activeTabView?.webContents) { - window.activeTabView.webContents.focus(); - } + if (!window.isDestroyed()) { + window.focus(); + window.activeTabView?.webContents?.focus(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@emain/emain-window.ts` around lines 989 - 992, After the show()/fullscreen sequence, guard the final focus operations with a destroyed check: verify the BrowserWindow instance is not destroyed (e.g., using window.isDestroyed()) before calling window.focus(), and then only call window.activeTabView.webContents.focus() if window.activeTabView and its webContents still exist; update the code around window.focus() and window.activeTabView?.webContents.focus() to early-return or skip these calls when window.isDestroyed() is true or when activeTabView/webContents are missing.
🧹 Nitpick comments (1)
emain/emain-window.ts (1)
936-947: Bounds clamping works correctly, but could be clearer.The width/height clamping was added (good!), but
maxXOffset/maxYOffsetstill referencecurBounds.width/curBounds.heightrather than the clampednextWidth/nextHeight. The current code happens to work because whencurBoundsexceedstargetArea, the max offset clamps to 0 anyway, but using the clamped values would make the intent clearer.♻️ Optional cleanup
const nextHeight = Math.min(curBounds.height, targetArea.height); const nextWidth = Math.min(curBounds.width, targetArea.width); -const maxXOffset = Math.max(0, targetArea.width - curBounds.width); -const maxYOffset = Math.max(0, targetArea.height - curBounds.height); +const maxXOffset = Math.max(0, targetArea.width - nextWidth); +const maxYOffset = Math.max(0, targetArea.height - nextHeight);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@emain/emain-window.ts` around lines 936 - 947, The maxXOffset and maxYOffset calculations still use curBounds.width/curBounds.height instead of the clamped nextWidth/nextHeight, which is misleading; update maxXOffset = Math.max(0, targetArea.width - nextWidth) and maxYOffset = Math.max(0, targetArea.height - nextHeight) so the offset clamping uses the actual clamped dimensions, then keep sourceXOffset/sourceYOffset and nextX/nextY calculations as-is and call win.setBounds with the updated nextWidth/nextHeight.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@emain/emain-window.ts`:
- Around line 989-992: After the show()/fullscreen sequence, guard the final
focus operations with a destroyed check: verify the BrowserWindow instance is
not destroyed (e.g., using window.isDestroyed()) before calling window.focus(),
and then only call window.activeTabView.webContents.focus() if
window.activeTabView and its webContents still exist; update the code around
window.focus() and window.activeTabView?.webContents.focus() to early-return or
skip these calls when window.isDestroyed() is true or when
activeTabView/webContents are missing.
---
Nitpick comments:
In `@emain/emain-window.ts`:
- Around line 936-947: The maxXOffset and maxYOffset calculations still use
curBounds.width/curBounds.height instead of the clamped nextWidth/nextHeight,
which is misleading; update maxXOffset = Math.max(0, targetArea.width -
nextWidth) and maxYOffset = Math.max(0, targetArea.height - nextHeight) so the
offset clamping uses the actual clamped dimensions, then keep
sourceXOffset/sourceYOffset and nextX/nextY calculations as-is and call
win.setBounds with the updated nextWidth/nextHeight.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a76773a2-6634-4973-a521-f09c5d53277e
📒 Files selected for processing (1)
emain/emain-window.ts
2 participants
closes #3138, closes #2128