chore: refactor and centralize app menu access COMPASS-9976 #7493

addaleax · 2025-10-23T13:52:29Z

(Split out from #7494 since I think this part is a bit clearer in that I think we want to do it for sure, and it's meaningful to review this separately)

Instead of having the logic for accessing the Electron application menu from the collection tabs spread out through the codebase, bundle it in a single interface that gives all Compass plugins access to the menu as needed.

Description

Checklist

New tests and/or benchmarks are included
Documentation is changed or added
If this change updates the UI, screenshots/videos are added and a design review is requested
I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Motivation and Context

Bugfix
New feature
Dependency update
Misc

Open Questions

Dependents

Types of changes

Backport Needed
Patch (non-breaking change which fixes an issue)
Minor (non-breaking change which adds functionality)
Major (fix or feature that would cause existing functionality to change)

Instead of having the logic for accessing the Electron application menu from the collection tabs spread out through the codebase, bundle it in a single interface that gives all Compass plugins access to the menu as needed.

Copilot

Pull Request Overview

This PR refactors how Compass plugins access the Electron application menu by introducing a centralized ApplicationMenuProvider interface. Instead of having collection-specific menu logic spread across multiple files, it consolidates menu management into a single, reusable pattern.

Introduces ApplicationMenuProvider interface and useApplicationMenu hook for managing application menus from any component
Replaces direct IPC calls for collection menu visibility with a more flexible menu registration system
Moves collection menu logic from main process into the collection tab component itself

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
packages/compass/src/main/menu.ts	Refactored to support dynamic menu registration via `additionalMenus` and `roleListeners` instead of hardcoded collection submenu
packages/compass/src/main/menu.spec.ts	Updated tests to reflect new menu state structure and removed tests for deprecated collection submenu methods
packages/compass/src/app/components/home.tsx	Removed collection submenu callbacks, added `ApplicationMenuContextProvider`
packages/compass/src/app/application.tsx	Implemented `ApplicationMenu` class providing menu provider interface, removed old IPC listeners for schema/import/export
packages/compass-workspaces/src/index.ts	Removed global app registry listeners for menu actions (now handled locally)
packages/compass-workspaces/src/application-menu.tsx	New file defining menu provider interface, context, and `useApplicationMenu` hook
packages/compass-workspaces/package.json	Added new `application-menu` export entry point
packages/compass-collection/src/components/collection-tab.tsx	Integrated `useApplicationMenu` hook to register collection menu when component is mounted

Comments suppressed due to low confidence (2)

packages/compass-workspaces/src/application-menu.tsx:1

The dependency array performs JSON.stringify and transformAppMenu on every render, which could be expensive for large menus. Consider memoizing these computed values using useMemo to avoid recalculating them unless menu or roles actually change.

import React, { useContext, useEffect } from 'react';

packages/compass-collection/src/components/collection-tab.tsx:1

[nitpick] The empty object passed as the fourth argument to globalAppRegistry.emit appears unnecessary. If this parameter is unused, consider removing it or adding a comment explaining its purpose for clarity.

import React, { useCallback, useEffect } from 'react';

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

addaleax · 2025-10-23T14:32:17Z

packages/compass-electron-menu/src/application-menu.tsx

Also wanted to call out this entry point in particular, if there's a better place for this or we maybe should even put it into a separate package that would seem fine to me

Ah, left a comment about it before seeing yours: yeah, I'd probably suggest to move this to its own place, the logic is pretty contained and doesn't need to be tied to our compass plugins

gribnoysup

Looks good! Very minor notes, but nothing blocking. Finally I can close #5818

gribnoysup · 2025-10-23T13:57:49Z

packages/compass-electron-menu/src/application-menu.tsx

This can probably be just completely in its own package

@gribnoysup Done – obviously that's a fairly large change to this PR, so please feel free to re-review 🙂

packages/compass-workspaces/src/application-menu.tsx

packages/compass/src/app/application.tsx

gribnoysup · 2025-10-23T14:44:01Z

packages/compass/src/main/menu.ts

-  return menu;
+  return [
+    darwinCompassSubMenu(menuState, app),
+    connectSubMenu(false, app),


Would be nice to move all feature specific menus to their locations in the app, but that's already a good start

addaleax · 2025-10-23T14:52:40Z

Looks good! Very minor notes, but nothing blocking. Finally I can close #5818

Oh damn, I wasn't aware of that one! Will address your feedback today

gribnoysup · 2025-10-23T15:15:08Z

Haha, yeah, your implementation is solving a similar problem in a pretty similar way so I think it's a great replacement for this unfinished experiment I did! I tried to bake a bit more features in, but I don't think it's really required at the end of the day, definitely feel free to use it for inspiration if you feel like it though 😄

paula-stacho · 2025-10-23T15:32:33Z

scripts/check-peer-deps.js

+    const shouldSkip = (path) =>
+      path.node.leadingComments?.some((comment) =>
+        comment.value?.includes('compass-peer-deps-ignore')
+      );


I actually wasn't aware that we're doing this via a custom script, cool! Looks like ImportDeclaration node has a prop importKind?: "type" | "typeof" | "value" | null;. Maybe we could ignore the type imports automatically?

@gribnoysup I assume we intentionally included type dependencies here ... if that isn't the case, then yeah, excluding them entirely makes sense to me

I mean, the main purpose here is to create a clear picture of what packages actually depend on via a set of straightforward rules to follow. Type imports should be part of the direct dependencies list, if they are not, you are potentially breaking consumers of your package

I see that the exception is made for electron, I guess this will go away if we move the application menu code to its own package. Can we remove the ability to make exceptions for the rules if we will not be using them?

Type imports should be part of the direct dependencies list, if they are not, you are potentially breaking consumers of your package

True. That's an implicit requirement that this package adds to its dependents.

I see that the exception is made for electron, I guess this will go away if we move the application menu code to its own package. Can we remove the ability to make exceptions for the rules if we will not be using them?

The same logic applies even now that it's in a separate package, unfortunately :(

I'm maybe missing something, but I'd like to clarify this. So this is a real type dependency for this package, this is a type-only dependency, so even though it will end up in a transitive "production" dependency tree of packages that use it and are not bundled, it will not end up in the actual compass-web bundle or its production dependencies (only in dev ones as this is build time), so what's preventing us from adding this as a prod dependency in this package if this should be a prod dependency? 🙂

@gribnoysup I've undone that in 07da41f.

I don't think this is the right choice in general for npm packages – yes, it being a type-only dependency means that it's something that will require the electron package to be present in dependent packages. But that requirement only exists at build time, so for "normal" npm packages I would never add it as a production dependency – there would just be an implicit requirement to have electron available as a devDependency in consuming packages as well. (If npm provided something like "peer dev dependency" packages, that would be the right category.)

You're right that we can ignore that concern because we're targeting only Compass here. Personally, I still like to follow what I'd consider a best practice for our packages even if our target context doesn't really require that best practice to be followed. But this is definitely not a hill I am prepared to die on in this case 🙂

…menu

gribnoysup · 2025-10-24T08:49:34Z

packages/compass-electron-menu/src/ipc-provider-renderer.ts

+import { uuid } from './util';
+import createDebug from 'debug';
+const debug = createDebug('compass-electron-menu:ipc-provider-renderer');
+export interface HadronIpcRenderer {


Curious why are you not just adding a dependency on hadron-ipc here, types and all (or at least for types), the package is safe to import in browser environment

Two reasons, although they're not exactly huge ones:

We get type safety for the events by using the event map type here for the events/calls defined in this package

It's easier to provide a typesafe mock interface in the test if we just restrict ourselves to what we use

Aha, gotcha, makes sense! Makes me think that maybe it's worth extending the HadronIpcRenderer / HadronIpcMain class to take type-safe event map optionally, but doesn't need to be something for this PR 🙂

gribnoysup

Still looks good, but I'd really wouldn't want us to introduce any exceptions to the peer dep check rule, this was causing so much confusion in the past, was so misaligned across all packages, I really would want us to stick to the defined rules here, especially as it doesn't look like there's a reason to make an exception

packages/compass-electron-menu/src/application-menu.spec.tsx

packages/compass/package.json

gribnoysup · 2025-10-24T09:15:13Z

packages/compass/src/app/application.tsx

          )}
-          showCollectionSubMenu={this.showCollectionSubMenu.bind(this)}
-          hideCollectionSubMenu={this.hideCollectionSubMenu.bind(this)}
+          applicationMenuProvider={this.menuProvider}


You can probably keep the whole provider out of the entrypoint instead of passing the instance as a prop here and then passing it to provider component (but not that important, we need to do a big clean-up of entrypoints, so it can stay as is)

@gribnoysup So, moving the provider component into the root render tree here? No strong feelings, but I do want to check that I understand your suggestion correctly 🙂

Yep! Wrapping the whole here in your provider and passing it the ipc

chore: refactor and centralize app menu access

a95c2a4

Instead of having the logic for accessing the Electron application menu from the collection tabs spread out through the codebase, bundle it in a single interface that gives all Compass plugins access to the menu as needed.

addaleax requested a review from a team as a code owner October 23, 2025 13:52

addaleax requested review from Copilot and paula-stacho October 23, 2025 13:52

addaleax mentioned this pull request Oct 23, 2025

feat(data-modeling): add undo/redo application menu interactions COMPASS-9976 #7494

Merged

Copilot AI reviewed Oct 23, 2025

View reviewed changes

addaleax commented Oct 23, 2025

View reviewed changes

gribnoysup approved these changes Oct 23, 2025

View reviewed changes

fixup: check + test

79cc9be

gribnoysup mentioned this pull request Oct 23, 2025

chore(react-electron-menu): implement React based rendering for the various electron menus in the app 🦨 #5818

Closed

3 tasks

paula-stacho reviewed Oct 23, 2025

View reviewed changes

addaleax added 2 commits October 24, 2025 01:19

fixup: separate package

e98afa3

fixup: compass-data-modeling does not yet depend on compass-electron-…

500892a

…menu

gribnoysup reviewed Oct 24, 2025

View reviewed changes

gribnoysup approved these changes Oct 24, 2025

View reviewed changes

addaleax added 4 commits October 24, 2025 11:21

fixup: remove peer dep check workaround

07da41f

fixup: cr

020d08e

fixup: cr

299d026

Merge remote-tracking branch 'origin/main' into 9976-dev-part-p1

49936b6

addaleax merged commit 45b67d8 into main Oct 24, 2025
58 of 60 checks passed

addaleax deleted the 9976-dev-part-p1 branch October 24, 2025 13:52

chore: refactor and centralize app menu access COMPASS-9976 #7493

chore: refactor and centralize app menu access COMPASS-9976 #7493

Uh oh!

Conversation

addaleax commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Motivation and Context

Open Questions

Dependents

Types of changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribnoysup left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Oct 23, 2025

Uh oh!

gribnoysup commented Oct 23, 2025

Uh oh!

paula-stacho Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribnoysup Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribnoysup left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

addaleax commented Oct 23, 2025 •

edited

Loading

paula-stacho Oct 23, 2025 •

edited

Loading

gribnoysup Oct 24, 2025 •

edited

Loading