rusackas commented on code in PR #44101:
URL: https://github.com/apache/superset/pull/44101#discussion_r4118786338
##########
superset-frontend/src/features/home/RightMenu.tsx:
##########
@@ -521,6 +527,31 @@ const RightMenu = ({
}
});
+ const extensionSettingsItems = (settingsMenuExtension?.primary ?? [])
+ .map(menuItem => {
+ const command = commands.getCommand(menuItem.command);
+ if (!command) {
+ return null;
+ }
Review Comment:
Traced it, and I don't think this is a real gap so much as an existing one:
`commands` (`src/core/commands/index.ts`) has no reactivity at all, it's a
plain Map with no subscribe/event mechanism, so `getCommand` was never going to
be "live" regardless of where it's called from. `PanelToolbar` calls it the
exact same way for its own secondary actions.
`settingsMenuExtension` itself *is* reactive (`useMenu`, in the deps array),
so a menu item registering later re-renders and re-checks fine. The actual edge
case is specifically "menu item registers before its command does," which would
need the extension author to activate in that order, backwards from the natural
one (my own test in this PR, and the notifications extension I verified this
against, both register the command first). Not going to bolt reactivity onto
`commands` as part of this PR, that's a bigger change affecting `PanelToolbar`
too, better as its own follow-up if it's worth doing at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
