MM-69578: fix: allow closing windows other than the main window with cmd + w on macOS - #3878
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:
📝 WalkthroughWalkthroughDevTools focus/close listeners were added to three windows—MattermostWebContentsView, MainWindow, and CallsWidgetWindow—each emitting an UPDATE_SHORTCUT_MENU IPC event via ipcMain when devtools-focused or devtools-closed fires. Corresponding unit tests were added or extended, and a null-safety test was added for the app menu. ChangesDevTools shortcut menu refresh
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant WebContents
participant Window as MainWindow / MattermostWebContentsView / CallsWidgetWindow
participant ipcMain
WebContents->>Window: devtools-focused event
Window->>ipcMain: emit UPDATE_SHORTCUT_MENU
WebContents->>Window: devtools-closed event
Window->>ipcMain: emit UPDATE_SHORTCUT_MENU
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
svelle
changed the title
![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)
src/app/menus/appMenu/view.ts (1)
10-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlace
common/communicationbefore the othercommon/*imports.The added import is out of the ESLint-enforced alphabetical order for the internal alias group.
Proposed import order
-import Config from 'common/config'; -import ServerManager from 'common/servers/serverManager'; import {UPDATE_SHORTCUT_MENU} from 'common/communication'; +import Config from 'common/config'; +import ServerManager from 'common/servers/serverManager';As per coding guidelines, “Follow import order enforced by ESLint: builtins → external →
@mattermost/*→ internal aliases (app,common,main,renderer) →types→ siblings/parent/index, with groups separated by blank lines and alphabetized within groups.”🤖 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 `@src/app/menus/appMenu/view.ts` around lines 10 - 12, The import block in view.ts is out of ESLint’s required alphabetical order for internal aliases. Reorder the `common/*` imports so `common/communication` is placed before `common/config` and `common/servers/serverManager`, keeping the internal alias group alphabetized and preserving the existing import grouping rules.Source: Coding guidelines
🤖 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 `@src/app/views/MattermostWebContentsView.ts`:
- Around line 328-335: The DevTools shortcut-menu refresh listeners in
MattermostWebContentsView are being removed by the macOS close-and-reopen path,
so the reopened DevTools no longer gets updates. Update the
devtools-closed/devtools-focused handling around the webContentsView.webContents
listeners to re-register the refresh listener after the synthetic close path,
and ensure UPDATE_SHORTCUT_MENU still fires for the reopened DevTools state.
---
Nitpick comments:
In `@src/app/menus/appMenu/view.ts`:
- Around line 10-12: The import block in view.ts is out of ESLint’s required
alphabetical order for internal aliases. Reorder the `common/*` imports so
`common/communication` is placed before `common/config` and
`common/servers/serverManager`, keeping the internal alias group alphabetized
and preserving the existing import grouping rules.
🪄 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: 77d17f5f-3745-427a-8013-3e219e685dc2
📒 Files selected for processing (2)
src/app/menus/appMenu/view.tssrc/app/views/MattermostWebContentsView.ts
github-actions
Bot
added
E2E/Run
github-actions
Bot
added
E2E/Run
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/app/menus/appMenu/view.test.js`:
- Around line 252-312: The two `createViewMenu` tests are sharing `ipcMain.emit`
call history, so the `devtools-closed` assertion can pass from a previous
`devtools-focused` emission. Isolate mock state per test by clearing
`ipcMain.emit` in the test setup (for example in `beforeEach`) along with the
other menu-related mocks, so each case verifies only its own
`onDevToolsFocused`/`onDevToolsClosed` flow.
🪄 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: 90b2b07e-4648-4c5b-8ccb-bc6317830191
📒 Files selected for processing (1)
src/app/menus/appMenu/view.test.js
devinbinnie
left a comment
devinbinnie
left a comment
There was a problem hiding this comment.
Good QoL improvement, I have some suggestions though.
Also, should we account for the calls widget window as well? That also has a dev tools option.
| })(), | ||
| click() { | ||
| MainWindow.get()?.webContents.openDevTools({mode: 'detach'}); | ||
| const wc = MainWindow.get()?.webContents; |
There was a problem hiding this comment.
Can we do this registration and listener setup in mainWindow.ts where the other listeners for the main window are? Feels weird to be adding it here in the click handler.
| this.webContentsView.webContents.off('devtools-focused', onDevToolsFocused); | ||
| ipcMain.emit(UPDATE_SHORTCUT_MENU); | ||
| }; | ||
| this.webContentsView.webContents.once('devtools-closed', onDevToolsClosed); |
There was a problem hiding this comment.
Since this is just firing when the devtools is being interacted with, do we really need the on and off here? I think we can just register the listener to run permanently like we do with the other listeners per web contents. I don't think we need to toggle the listener on/off dependent on whether the devtools is open, since it should only fire these events when the devtools is open. And in any case, it probably shouldn't matter if it updates the shortcut menu more often.
The same should apply for the main window as well.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/app/callsWidgetWindow.ts`:
- Around line 213-214: CallsWidgetWindow is registering DevTools event handlers
on its webContents, but the test coverage does not verify that behavior. Update
src/app/callsWidgetWindow.test.js to assert that CallsWidgetWindow (via onShow()
or the relevant setup path) registers both devtools-focused and devtools-closed
listeners on the widget/popout webContents, and that those listeners emit
UPDATE_SHORTCUT_MENU through ipcMain when triggered. Use CallsWidgetWindow,
onShow(), and UPDATE_SHORTCUT_MENU to locate the behavior.
🪄 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: 746ef186-e6b3-4884-96e2-c273bc8f517d
📒 Files selected for processing (6)
src/app/callsWidgetWindow.tssrc/app/mainWindow/mainWindow.test.jssrc/app/mainWindow/mainWindow.tssrc/app/menus/appMenu/view.test.jssrc/app/views/MattermostWebContentsView.test.jssrc/app/views/MattermostWebContentsView.ts
Documentation Impact Analysis — no longer neededA previous automated documentation impact comment exists, but the latest analysis determined that no documentation changes are needed. The |
devinbinnie
left a comment
devinbinnie
left a comment
There was a problem hiding this comment.
Couple nits, but much better. Can we make sure this is QAed or tested somehow especially with the changes to the calls widget?
Thanks @svelle :)
| this.win.browserWindow.on('blur', this.onBlur); | ||
| this.win.browserWindow.contentView.on('bounds-changed', this.handleBoundsChanged); | ||
| this.win.browserWindow.webContents.on('before-input-event', this.onBeforeInputEvent); | ||
| this.win.browserWindow.webContents.on('devtools-focused', () => ipcMain.emit(UPDATE_SHORTCUT_MENU)); |
There was a problem hiding this comment.
Nit: Make these a member function of the MainWindow class so we don't add multiple unnecessary functions.
| }); | ||
| this.webContentsView.webContents.on('did-navigate-in-page', () => this.handlePageTitleUpdated(this.webContentsView.webContents.getTitle())); | ||
| this.webContentsView.webContents.on('page-title-updated', (_, newTitle) => this.handlePageTitleUpdated(newTitle)); | ||
| this.webContentsView.webContents.on('devtools-focused', () => ipcMain.emit(UPDATE_SHORTCUT_MENU)); |
| this.win.webContents.on('will-navigate', this.onNavigate); | ||
| this.win.webContents.on('did-start-navigation', this.onNavigate); | ||
| this.win.webContents.on('devtools-focused', () => ipcMain.emit(UPDATE_SHORTCUT_MENU)); | ||
| this.win.webContents.on('devtools-closed', () => ipcMain.emit(UPDATE_SHORTCUT_MENU)); |
devinbinnie
added
4: Reviews Complete
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
DevTools windows are not BrowserWindows, so BrowserWindow.getFocusedWindow() returns null when they're focused. The app menu was only rebuilt on main window focus (MAIN_WINDOW_FOCUSED) and popout window focus, so Cmd+W with a DevTools window in focus would still fire the stale handler from when the main window was focused — closing the main window instead of the DevTools window. Fix by listening to 'devtools-focused' on the WebContents whose DevTools opened, and emitting UPDATE_SHORTCUT_MENU to trigger a menu rebuild. The rebuilt menu has role:'close' with CmdOrCtrl+W (no focused BrowserWindow), so macOS sends performClose: to the frontmost window (the DevTools window). Also listen to 'devtools-closed' to remove the listener and rebuild again. Mirrors the pattern used by popout windows, which call MenuManager.refreshMenu() in their BrowserWindow 'focus' handler. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uerg9bXjuH1nnSbjvwMaMM
…acOS reopen - Move common/communication import before common/config to satisfy ESLint import/order rule - Extract DevTools menu refresh listener setup into registerDevToolsMenuRefresh() so the macOS close-and-reopen workaround path also re-registers the listeners after the synthetic devtools-closed consumes them (per CodeRabbit review) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uerg9bXjuH1nnSbjvwMaMM
Update existing test mock to include isDevToolsOpened, on, once, and off methods required by the new click handler. Add tests covering: early return when MainWindow is null, guard preventing duplicate listener registration when DevTools is already open, devtools-focused emitting UPDATE_SHORTCUT_MENU, and devtools-closed removing the focus listener and emitting UPDATE_SHORTCUT_MENU.
…isteners Cover the registerDevToolsMenuRefresh logic added to openDevTools: listener registration, devtools-focused/devtools-closed handlers emitting UPDATE_SHORTCUT_MENU, the guard against double-registration, and the macOS workaround path that re-registers listeners after the synthetic close-and-reopen. Also clear all mocks in beforeEach of view.test.js to prevent shared ipcMain call history between devtools-focused and devtools-closed assertions.
…n each openDevTools call Per reviewer feedback: - Move devtools-focused/devtools-closed listeners into constructors/init methods (MattermostWebContentsView constructor, MainWindow.init, CallsWidgetWindow.init and onPopOutCreate) so they are registered once and stay active permanently. - Simplify view.ts click handler to just call openDevTools directly; listener setup now lives in mainWindow.ts where the other main-window listeners are. - Also cover the calls widget window and its popout (as suggested by reviewer). - Simplify openDevTools in MattermostWebContentsView to remove the conditional registerDevToolsMenuRefresh helper; only the macOS reopen workaround remains. - Update all affected tests to reflect the new architecture. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uerg9bXjuH1nnSbjvwMaMM
Fix broken onPopOutCreate test where the will-redirect listener was being overwritten by the new devtools-focused/devtools-closed listeners. Add 8 new tests covering devtools listener registration and UPDATE_SHORTCUT_MENU emission for both the widget window (init) and the popout (onPopOutCreate).
Replace inline () => ipcMain.emit(UPDATE_SHORTCUT_MENU) arrow functions passed to webContents.on() with a named private emitShortcutMenuUpdate member on each class, avoiding unnecessary repeated function allocations.
…md+W Electron's BrowserWindow.getFocusedWindow() returns the owner window when that window's DevTools is focused, so rebuilding the menu alone still left Close Tab bound to Cmd+W for Main Window DevTools. Exclude the isDevToolsFocused case when deciding close accelerators. Co-authored-by: Sven Hüster <svelle@users.noreply.github.com>
8c2b931 to
3ba7a87
Compare
github-actions
Bot
added
E2E/Run
|
@devinbinnie thanks for your reviews on this. I think I addressed it all now. I also finally went and actually tested it (claude opened the PR before I was ready for it to be reviewed 😅 ) it's working like a charm now, even with the calls pop-out widget and dev tools. |
svelle
changed the title
7 participants
Summary
This PR fixes the behavior of keyboard shortcuts (particularly Cmd+W) when DevTools is open. When DevTools gains focus, the app menu is now rebuilt so that Cmd+W targets the DevTools window instead of the main window. Similarly, when DevTools loses focus, the menu is rebuilt again to restore normal behavior.
The fix adds event listeners for
devtools-focusedanddevtools-closedevents on web contents, emittingUPDATE_SHORTCUT_MENUto trigger menu reconstruction. This ensures consistent behavior with pop-out windows, which already handle focus changes via their BrowserWindowfocusevent.Changes
src/app/menus/appMenu/view.tsto add DevTools event listeners when opening DevTools from the View menusrc/app/views/MattermostWebContentsView.tsto add the same DevTools event listeners when opening DevTools from the web viewipcMainfrom electron andUPDATE_SHORTCUT_MENUconstant from common/communicationChecklist
npm run lint:jsfor proper code formattingE2E/RunRelease Note
https://claude.ai/code/session_01Uerg9bXjuH1nnSbjvwMaMM
Change Impact: 🟡 Medium
Regression Risk: The change is fairly targeted, but it touches multiple window/webContents initialization paths and global shortcut-menu refresh behavior. Risk is moderate because DevTools focus/close handling is now wired into several components, and regressions could affect macOS shortcut behavior or menu updates across windows.
QA Recommendation: Recommended manual QA on macOS for DevTools focus/close transitions in the main window, calls widget window, and popout window, with a quick sanity check on menu shortcut behavior after switching focus.
Generated by CodeRabbitAI