Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds an exported resetZoomLevel(webContents) and a non-exported broadcastZoomFactorChanged helper in emain-util.ts. Replaces direct zoom-reset logic and per-webContents Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)
Files Reviewed (3 files)
Note: The PR successfully centralizes zoom handling through the new |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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-tabview.ts`:
- Around line 168-170: The "zoom-changed" handler currently sends
"zoom-factor-change" only to this.webContents, bypassing the new app-wide
broadcast; replace the local send in the this.webContents.on("zoom-changed",
...) handler with a call into the shared zoom sync/broadcast path (i.e., invoke
the central zoom broadcast/sync function used elsewhere in the refactor rather
than this.webContents.send("zoom-factor-change", ...)) and pass the current zoom
factor from this.webContents.getZoomFactor() so all tabs receive the update.
In `@emain/emain-util.ts`:
- Around line 15-21: broadcastZoomFactorChanged currently only sends an IPC
message ("zoom-factor-change") which leaves other WebContents' internal zoom
state stale; update the function (broadcastZoomFactorChanged) to also call
wc.setZoomFactor(newZoomFactor) for each non-destroyed, app-owned
electron.webContents (in addition to wc.send) so getZoomFactor() reflects the
new value, or alternatively implement/consume a single shared zoom source that
new WebContents hydrate from; ensure you only call setZoomFactor on appropriate
WebContents (skip destroyed or external/guest contents).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b46512af-794f-4616-88d6-3d340903f70e
📒 Files selected for processing (3)
emain/emain-menu.tsemain/emain-tabview.tsemain/emain-util.ts
emain/emain-tabview.ts
Outdated
| this.webContents.on("zoom-changed", (_event, _zoomDirection) => { | ||
| this.webContents.send("zoom-factor-change", this.webContents.getZoomFactor()); | ||
| }); |
There was a problem hiding this comment.
zoom-changed still bypasses the new broadcast path.
This handler only sends "zoom-factor-change" back to this.webContents, so any zoom change that comes through Electron's native zoom-changed flow remains local even after this refactor. If zoom notifications are supposed to be app-wide now, this should route through the same shared zoom sync/broadcast path instead of doing a per-tab send.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@emain/emain-tabview.ts` around lines 168 - 170, The "zoom-changed" handler
currently sends "zoom-factor-change" only to this.webContents, bypassing the new
app-wide broadcast; replace the local send in the
this.webContents.on("zoom-changed", ...) handler with a call into the shared
zoom sync/broadcast path (i.e., invoke the central zoom broadcast/sync function
used elsewhere in the refactor rather than
this.webContents.send("zoom-factor-change", ...)) and pass the current zoom
factor from this.webContents.getZoomFactor() so all tabs receive the update.
| function broadcastZoomFactorChanged(newZoomFactor: number): void { | ||
| for (const wc of electron.webContents.getAllWebContents()) { | ||
| if (wc.isDestroyed()) { | ||
| continue; | ||
| } | ||
| wc.send("zoom-factor-change", newZoomFactor); | ||
| } |
There was a problem hiding this comment.
Broadcasting only the IPC leaves other WebContents on a stale zoom factor.
Right now only the initiating webContents gets setZoomFactor(...); every other renderer just receives "zoom-factor-change". That makes the CSS update propagate, but getZoomFactor() in those other windows/tabs still returns the old value, so newly initialized views and the next +/- step start from the wrong baseline. This helper needs to synchronize the actual Electron zoom state for each app-owned WebContents, or the app needs a single shared zoom source that new tabs hydrate from.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@emain/emain-util.ts` around lines 15 - 21, broadcastZoomFactorChanged
currently only sends an IPC message ("zoom-factor-change") which leaves other
WebContents' internal zoom state stale; update the function
(broadcastZoomFactorChanged) to also call wc.setZoomFactor(newZoomFactor) for
each non-destroyed, app-owned electron.webContents (in addition to wc.send) so
getZoomFactor() reflects the new value, or alternatively implement/consume a
single shared zoom source that new WebContents hydrate from; ensure you only
call setZoomFactor on appropriate WebContents (skip destroyed or external/guest
contents).
1 participant
No description provided.