feat(architecture): refine motion systems#253
Open
Aditya948351 wants to merge 4 commits into
Open
Conversation
|
@Aditya948351 is attempting to deploy a commit to the Dot_NotSam's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR replaces the single ChangesMulti-display overlay architecture and delta-time audio smoothing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
SamXop123
added
gssoc26
SamXop123
requested changes
Jun 19, 2026
SamXop123
left a comment
SamXop123
left a comment
Owner
There was a problem hiding this comment.
This PR is unsafe to merge and will crash the application on startup, disable visualizer responses, and break settings persistence. Please address the following issues:
ReferenceErrorinmain.js: IncreateOverlayForDisplay(display), you updated the initialization toconst win = new BrowserWindow({...})but did not change the subsequent references (e.g.overlayWindow.setAlwaysOnTop,overlayWindow.loadFile, etc.) which still use the removed globaloverlayWindowvariable.- Missing
resizeAllOverlaysfunction: You callresizeAllOverlays()ondisplay-metrics-changedandsecond-instanceevents, but the function is not defined anywhere in the codebase. - Broken Settings / IPC Control:
sendVisualizerSettings()andsendFocusModeOpacity()still check and use the globaloverlayWindow, meaning settings and opacity changes will not be broadcast to the newoverlayWindowsMap.showCustomContextMenuand theset-ignore-mouse-eventsIPC handler still checkoverlayWindow.
- Settings Sanitization in
settingsStore.js: The newmonitorssettings configuration is missing fromsanitizeSettingsandDEFAULT_SETTINGS, meaning any changes saved viaupdateSettingsare immediately stripped and discarded. - Frozen Renderer Physics: In
renderer.js(renderFrame),lastFrameAt = now;is executed beforeupdateAudioLevel(now). InsideupdateAudioLevel,now - lastFrameAtevaluates to0, makingtimeScale = 0. As a result, the audio reactivity is frozen and won't respond to incoming levels.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
main.js (4)
399-408:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove stale reference to
overlayWindow.Line 407 references the old single-window variable after the loop already reloads all windows in
overlayWindows. This line is both redundant and will cause a runtime error sinceoverlayWindowno longer exists.🐛 Proposed fix
function reloadVisualizer() { for (const win of overlayWindows.values()) { if (!win.isDestroyed()) { win.webContents.reloadIgnoringCache(); } } stopSimulatedAudioFallback(); - overlayWindow.webContents.reloadIgnoringCache(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.js` around lines 399 - 408, In the reloadVisualizer function, remove the stale reference to the overlayWindow variable on the last line that calls win.webContents.reloadIgnoringCache(). This line is redundant because all windows are already being reloaded in the for loop that iterates through overlayWindows.values(), and overlayWindow no longer exists in the codebase, which will cause a runtime error. Simply delete this line to resolve the issue.
1622-1643:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUpdate
showCustomContextMenuto multi-display model.This function still references
overlayWindowin multiple places. For multi-display support, it should determine which overlay window contains the cursor and send the context menu command to that window.🔧 Proposed fix
function showCustomContextMenu() { - if (!overlayWindow || overlayWindow.isDestroyed()) { + if (overlayWindows.size === 0) { return; } const cursorPoint = screen.getCursorScreenPoint(); + const targetDisplay = screen.getDisplayNearestPoint(cursorPoint); + const win = overlayWindows.get(targetDisplay.id); + + if (!win || win.isDestroyed()) { + return; + } // Force Windows to refresh the window's z-order relative to other topmost windows // (like the tray overflow panel) by toggling setAlwaysOnTop and calling moveTop() - overlayWindow.setAlwaysOnTop(false); - overlayWindow.setAlwaysOnTop(true, "screen-saver"); - overlayWindow.moveTop(); + win.setAlwaysOnTop(false); + win.setAlwaysOnTop(true, "screen-saver"); + win.moveTop(); - const primaryDisplay = screen.getPrimaryDisplay(); - const localX = cursorPoint.x - primaryDisplay.bounds.x; - const localY = cursorPoint.y - primaryDisplay.bounds.y; + const localX = cursorPoint.x - targetDisplay.bounds.x; + const localY = cursorPoint.y - targetDisplay.bounds.y; - overlayWindow.webContents.send("show-context-menu", { + win.webContents.send("show-context-menu", { x: localX, y: localY }); - overlayWindow.setIgnoreMouseEvents(false); + win.setIgnoreMouseEvents(false); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.js` around lines 1622 - 1643, The showCustomContextMenu function currently assumes a single overlayWindow and sends the context menu to it regardless of which display the cursor is on. To support multi-display, refactor this function to determine which display contains the cursor using the cursorPoint coordinates and screen.getAllDisplays(), then identify the corresponding overlay window for that display. Instead of using the global overlayWindow reference, find and use the correct overlay window for the display containing the cursor before sending the show-context-menu message to overlayWindow.webContents.send().
475-480:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUpdate
sendFocusModeOpacityto multi-display model.This function still references the old
overlayWindowvariable. It should iterate overoverlayWindowslikesendAudioLeveldoes.🐛 Proposed fix
function sendFocusModeOpacity(opacity) { - if (!overlayWindow || overlayWindow.isDestroyed()) { - return; - } - overlayWindow.webContents.send("focus-mode-opacity", { opacity }); + for (const win of overlayWindows.values()) { + if (!win.isDestroyed()) { + win.webContents.send("focus-mode-opacity", { opacity }); + } + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.js` around lines 475 - 480, The sendFocusModeOpacity function currently operates on a single overlayWindow variable but needs to be updated to support multiple displays. Refactor this function to iterate over the overlayWindows collection (similar to how sendAudioLevel handles multiple windows) and send the focus-mode-opacity message to each non-destroyed window in the collection, removing the single overlayWindow check and replacing it with a loop that validates each window before sending.
265-286:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: References to undefined
overlayWindowinstead ofwin.The window is created as
winon line 241, but lines 265-286 operate onoverlayWindowwhich is the old single-window variable that no longer exists in this multi-display architecture. This will cause runtime errors—the new window is never configured, shown, or loaded.Additionally, line 285 sets
overlayWindow = nullin theclosedhandler, which is incorrect for multi-display—the window should be removed from the Map instead.🐛 Proposed fix
- overlayWindow.setAlwaysOnTop(true, "screen-saver"); - overlayWindow.setVisibleOnAllWorkspaces(true, { visibleOnFullScreen: true }); - overlayWindow.setIgnoreMouseEvents(true, { forward: true }); - overlayWindow.setBounds(bounds); - overlayWindow.showInactive(); - overlayWindow.moveTop(); - overlayWindow.loadFile("index.html"); + win.setAlwaysOnTop(true, "screen-saver"); + win.setVisibleOnAllWorkspaces(true, { visibleOnFullScreen: true }); + win.setIgnoreMouseEvents(true, { forward: true }); + win.setBounds(bounds); + win.showInactive(); + win.moveTop(); + win.loadFile("index.html"); - overlayWindow.webContents.on("did-finish-load", () => { + win.webContents.on("did-finish-load", () => { setTimeout(() => { sendVisualizerSettings(); }, 100); }); - overlayWindow.on("close", () => { + win.on("close", () => { stopSimulatedAudioFallback(); }); - overlayWindow.on("closed", () => { + win.on("closed", () => { stopSimulatedAudioFallback(); - overlayWindow = null; + overlayWindows.delete(id); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.js` around lines 265 - 286, Replace all references to `overlayWindow` with `win` throughout this code block since the window is created as `win` on line 241. Additionally, in the `closed` event handler where the code currently sets `overlayWindow = null`, this should instead remove the window from the Map data structure that stores windows for the multi-display architecture (likely using a key identifier like the display ID or window ID to locate and delete the entry from the Map).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@renderer.js`:
- Around line 596-598: The smoothing factor calculation in the smoothedLevel
update line can exceed 1 during frame stalls, causing overshoot and jitter.
Bound the product of response and timeScale to a maximum of 1 by wrapping the
multiplication factor (response * timeScale) with Math.min to prevent
smoothedLevel from overshooting its normalized bounds and ensure stable
frame-rate independent smoothing even when frames are delayed.
---
Outside diff comments:
In `@main.js`:
- Around line 399-408: In the reloadVisualizer function, remove the stale
reference to the overlayWindow variable on the last line that calls
win.webContents.reloadIgnoringCache(). This line is redundant because all
windows are already being reloaded in the for loop that iterates through
overlayWindows.values(), and overlayWindow no longer exists in the codebase,
which will cause a runtime error. Simply delete this line to resolve the issue.
- Around line 1622-1643: The showCustomContextMenu function currently assumes a
single overlayWindow and sends the context menu to it regardless of which
display the cursor is on. To support multi-display, refactor this function to
determine which display contains the cursor using the cursorPoint coordinates
and screen.getAllDisplays(), then identify the corresponding overlay window for
that display. Instead of using the global overlayWindow reference, find and use
the correct overlay window for the display containing the cursor before sending
the show-context-menu message to overlayWindow.webContents.send().
- Around line 475-480: The sendFocusModeOpacity function currently operates on a
single overlayWindow variable but needs to be updated to support multiple
displays. Refactor this function to iterate over the overlayWindows collection
(similar to how sendAudioLevel handles multiple windows) and send the
focus-mode-opacity message to each non-destroyed window in the collection,
removing the single overlayWindow check and replacing it with a loop that
validates each window before sending.
- Around line 265-286: Replace all references to `overlayWindow` with `win`
throughout this code block since the window is created as `win` on line 241.
Additionally, in the `closed` event handler where the code currently sets
`overlayWindow = null`, this should instead remove the window from the Map data
structure that stores windows for the multi-display architecture (likely using a
key identifier like the display ID or window ID to locate and delete the entry
from the Map).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 08ffb67e-7825-489b-9bed-6501fd622e2e
📒 Files selected for processing (2)
main.jsrenderer.js
Aditya948351
force-pushed
the
refinemotion
branch
from
June 19, 2026 14:04
3e645fc to
f9ea0f3
Compare
Contributor
Author
|
@SamXop123 Requesting your review again |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #238
Summary by CodeRabbit
Release Notes
New Features
Improvements