amaannawab923 commented on code in PR #45522:
URL: https://github.com/apache/superset/pull/45522#discussion_r4401283677
##########
superset-frontend/src/features/home/Menu.tsx:
##########
@@ -199,6 +200,109 @@ const StyledImage = styled(Image)`
const { useBreakpoint } = Grid;
+/**
+ * What the two helpers below need of a contributed page, and no more, so the
+ * menu does not take a dependency on the pages registry. One shape shared by
+ * both, so a field one of them reads cannot go missing from the other.
+ */
+type ContributedPage = {
+ name: string;
+ path: string;
+ nav?: { label?: string; category?: string };
+ permitted?: boolean;
+};
+
+/**
+ * Folds pages contributed by extensions into the server-rendered menu.
+ *
+ * The navbar is built on the server from FAB's menu, which an extension
+ * cannot write to: an entry there needs a `menu_access` permission created
+ * at boot, and extensions register in the browser. A page that opted in with
+ * `nav` is therefore added here, client-side, so that a contributed page has
+ * somewhere a user could find it at all.
+ *
+ * A page without `nav.category` sits at the top level. One that names a
+ * category goes into that group of the Settings menu instead, which is
+ * where an administrative screen belongs -- see `withExtensionSettings`.
+ */
+export function withExtensionPages(
+ menu: MenuObjectProps[],
+ pages: ContributedPage[],
+): MenuObjectProps[] {
+ // A page the user may not open is left out rather than shown and refused:
+ // the server answers 403 for it, so an entry here would only advertise a
+ // dead end. The page stays registered, so a direct link still explains
+ // itself rather than rendering nothing.
+ const contributed = pages.filter(
+ page => page.nav && !page.nav.category && page.permitted !== false,
+ );
+ if (!contributed.length) {
+ return menu;
+ }
+
+ return [
+ ...menu,
+ ...contributed.map(
+ page =>
+ ({
+ label: page.nav?.label ?? page.name,
+ // `name` is the React key for the entry, and the label is the
+ // extension author's to choose -- a page labelled "Charts" would
+ // collide with the host's own entry. The path cannot collide,
+ // because the server refused any page that shared one.
+ name: `extension:${page.path}`,
+ url: page.path,
Review Comment:
Fixed in b65a24e, and you were right that it is the convention that is
broken rather than one call site.
Every other `url` in this menu is already rooted -- the backend emits host
entries through `url_for` -- and `buildMenuItem` strips the root again before
handing the path to `NavLink`. The contributed entry carried the bare manifest
path, which survived that round trip only because `stripAppRoot` leaves an
unrooted path alone. Both the navbar entry and the settings variant now carry
the root like their neighbours.
That alone does not fix the case you described, which is worth spelling out.
Where a page's own first segment *is* the application root, "already rooted"
and "starts with the root segment" are indistinguishable, so `ensureAppRoot`
leaves such a path alone by construction -- exactly as it must for every other
caller. So `/analytics/review` under `/analytics` still cannot be linked
correctly from the client, whatever the menu does.
It is refused at registration instead, beside the existing route-collision
check, with the real URL named in the log. That makes it deployment-dependent
-- a manifest fine at the domain root is refused under a subdirectory -- but
`register_extension_pages` already makes that trade for route collisions, and a
loud refusal at boot beats a nav entry that silently 404s.
Tests are where you asked for them: `withExtensionPages.test.ts` under a
mocked `/analytics` root, covering both the navbar and the Settings child, that
the root is applied exactly once, and that a domain-root deployment is
unaffected; plus backend tests that the shadowing path is refused, that
`/analytics-archive` is not caught by a partial segment match, and that
`APPLICATION_ROOT = "/"` refuses nothing.
--
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]
