Repository navigation
feat(extensions): let extensions contribute top-level pages - #45522
Open
amaannawab923 wants to merge 44 commits into
Open
amaannawab923 wants to merge 44 commits into
amaannawab923 wants to merge 44 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #45522 +/- ##
==========================================
+ Coverage 85.17% 85.20% +0.02%
==========================================
Files 3082 3086 +4
Lines 210541 210884 +343
Branches 49624 49713 +89
==========================================
+ Hits 179327 179676 +349
+ Misses 27821 27815 -6
Partials 3393 3393
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
Bot
added
requires:rebase
amaannawab923
force-pushed
the
feat/extension-pages
branch
from
October 21, 2026 13:24
39e5e31 to
a9432e7
Compare
amaannawab923
marked this pull request as ready for review
October 21, 2026 13:47
amaannawab923
requested review from
geido,
michael-s-molina,
msyavuz,
rusackas,
sadpandajoe and
villebro
as code owners
October 21, 2026 13:47
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
github-actions
Bot
added
i18n
sadpandajoe
reviewed
Oct 21, 2026
geido
reviewed
Oct 22, 2026
geido
left a comment
geido
left a comment
Member
There was a problem hiding this comment.
I tested this hands-on: PR head 10057d7412 merged locally with current master (aabddf5b3f), with ENABLE_EXTENSIONS on. I used two extensions scaffolded with this branch's CLI (one has a permission-gated page, one collides with it) and users admin, gamma and alpha. Videos, stills and the test extensions are in this gist: https://gist.github.com/geido/6de6b5435657f4d5b8065274de1f8b2a
A lot of it works well (list below). There are also several real bugs, CI is red, and the design differs from SIP-235, which is still being discussed.
Relationship to SIP-235 (#45248)
SIP-235 covers the same feature (it also takes in #44101) and was revised today after @michael-s-molina's review. This PR doesn't reference it, and it takes the other side on most of its decisions:
| SIP-235 (as revised today) | This PR | |
|---|---|---|
| Registration | views.registerView(view, 'global.page', C); a separate registerPage API is listed under rejected alternatives |
New pages namespace (registerPage, getPages, useSubPath, useNavigate, events) |
| URL | One namespaced shell route /extensions/pages/<pageId>; list/pages become reserved publisher names |
Flat paths chosen by the author, plus collision detection at boot |
| Navbar | menus.registerMenuItem({ page, icon, order }, 'global.navbar' | 'global.settingsMenu'), backed by core.navigation.openPage |
nav: { label, category } in the manifest; no icon or order |
| Manifest | Contributions stay docs/tooling-only; having the server read them is deferred | contributions.pages is authoritative and read by the server |
| RBAC | Out of scope; per-page permissions are a "possible follow-up" (a backend declaration keyed on the page id) | Per-page FAB permissions, automatic Admin grant, _is_admin_only change, EXTENSIONS_SYNC_PAGE_PERMISSIONS |
| Sub-routing | Out of scope (one route depth) | A page owns its subtree (useSubPath / useNavigate) |
| Navigation | navigateTo, 'extension' in the Page union, active-tab highlighting |
None of these |
| Dependencies | None | @superset-ui/core added to the Module Federation share scope |
The first commit here (fd96a9da99) served pages from a single namespaced rule. Its message argues against per-page routes, which need "collision detection and path validation for author-supplied paths, neither of which this needs". Several of the issues below come from moving to flat paths.
This adds public contract that's hard to take back: the @apache-superset/core API, the extension.json schema, the share scope, and a security-manager rule. I'd suggest settling the shape on the SIP thread first. Some parts of this PR would make good follow-ups there: page declarations the server reads (so cold loads resolve and nav entries don't paint late), per-page permissions, the sub-path hooks, and PageBoundary.
Verified working
- Navbar and Settings → Manage entries open with client-side transitions.
useSubPath/useNavigate, the Back button, and cold loads of deep sub-paths.SupersetClientGET and POST (with CSRF) through the shared package.PageBoundarycontains a crash and recovers.- At boot,
/welcome,/api/v1/datasource/table(converter rule) and a path duplicated across extensions are refused. - Undeclared or refused
registerPagecalls are rejected. - Anonymous users are redirected to login.
- Gamma: entry hidden, 403 on direct load, "Not authorized" alert on in-app arrival.
- Granting the permission in the Roles UI unlocks the page.
superset initkeeps page permissions off Alpha and Gamma.tests/unit_tests/extensions/(319) and the PR's Jest suites (99) pass.
Happy path (admin)
Path conflicts refused at boot
Permission-gated page (gamma), granted in the Roles UI
CI
CI is red, and all of these still reproduce with master merged in:
- mypy errors in
test_pages_view.py(see @sadpandajoe's thread). test_init_extensions_loggingfails withAttributeError: ... 'superset_app'. It passes on master.- end-of-file-fixer fails on
messages.pot.
The branch also conflicts with master now, only in messages.pot.
Inline comments cover the rest. Where I could confirm an existing thread, I added the repro there.
github-actions
Bot
added
requires:rebase
Three gate failures, no behaviour change. - `oxfmt` rewraps six frontend files. Its output, taken as given. - `ensure_page_permissions` rolls the session back by hand, which the transaction checker reads as a unit of work wanting `@transaction`. It cannot have one: the decorator would wrap the whole loop, so one permission that fails to write would discard the ones already created, and every worker runs this at boot where losing a race is expected. Disabled with that reason recorded, as 178 other call sites in `superset/` do. - `Object.keys(...)[0]` becomes array destructuring, which is what `prefer-destructuring` asks for and reads better anyway.
The eight strings the page boundary and the extension-page screen introduce were never in the template, so no translator could reach them and `check_pot_drift` failed. Only those eight entries are added. Running `babel_update.sh` whole would have been the obvious move and the wrong one: it rewrites the template's header and re-runs `pybabel update` over all 30 catalogs, which on current master alone produces a 3,520-line diff and drops existing translations -- none of which the drift check looks at, since it compares msgid sets in `messages.pot` and nothing else.
Behaviour: - Render the shell with a 403 rather than aborting, so a cold load or a bookmark of a page the user may not open gets the same "not authorized" screen as an in-app arrival. `show_http_exception` serves HTML only for 404 and 500, so aborting answered a browser with a JSON body carrying the generic "unexpected error" code for a refusal that is neither. - Key a settings child by `name` in `RightMenu`, which keys them by label: a contributed page labelled like a host entry gave two children the same React key. - Make `PageNav.label` optional, matching the TypeScript type, the docs and the host's existing fallback to the page's name. It was required, so a manifest declaring only `nav.category` failed validation. Messages and comments that said something untrue: - A page with no rule now reports both reasons there may be none: the path is already served, or the page was added after the server started. Naming only the conflict sent an author who had just added a page looking for one that does not exist. - The grant logs and docstring name the prefixed view menu the permission actually hangs from, not the bare extension id. - `EXTENSIONS_SYNC_PAGE_PERMISSIONS = False` leaves the declared permissions uncreated: `superset init` syncs the permissions of FAB-registered views and these pages are plain Flask rules, so it does not reach them. The comment said init would create them, and now says what an operator has to create instead. - Dropped a stale comment claiming the extensions API filters refused pages out; it reports every declared page with what became of it, as the comment below it already said. CI: - `test_init_extensions_logging` builds its initializer with `object.__new__`, so give it an app and stub the page registration `init_extensions` now runs before the entrypoint. - Type the page-view test's fake as `SimpleNamespace`, which is what it builds, so the attribute reads check and three `type: ignore`s go. - One trailing newline in messages.pot.
What a contributed page requires was read twice. The view closed over the permission its manifest declared when the rule was registered, while `permitted_page_paths` -- which decides whether the API advertises the page, and so whether a navigation entry appears -- re-read the manifest on every request. `get_extensions` really does re-read the bundle each time, so the two could disagree, and the disagreement fails open on the side that enforces: a page that gains a permission without a restart is hidden from the navigation while its route still admits any signed-in user. Both now read a table written when the rule is registered, keyed by extension and path, held on the Flask app rather than this module so the several apps a test run builds cannot see each other's pages. What is enforced and what is advertised cannot drift apart, because they are the same entry. The table also answers whether a page exists at all, which the view needs because a missing entry and an entry of `None` mean opposite things: a page may legitimately declare no permission and be open to any signed-in user. `_page_requirement` returns both facts so an unrecorded page cannot read as the most permissive kind there is -- it is refused. Entries are never cleared. A Flask rule cannot be unregistered, so a page that was registered goes on being served for the life of the process and its requirement has to last exactly as long; dropping it would leave a live route with nothing recorded against it, which the view now refuses. Re-registering a page overwrites its own entry, so the last registration wins, as it already does for the rule. `permitted_page_paths` keeps answering about the pages of the manifest it is given -- only where it reads the requirement from has changed. Three tests: the unrecorded rule is refused though the permission is granted, and both directions of the drift. The last two fail against the previous code.
`navigation.getPage()` answered `'home'` while a contributed page was open. A page sits behind a catch-all route, so no entry in `PAGE_ROUTES` can match one and the fallback claimed the welcome screen. The event was the worse half: `notifyLocationChanged` returns early when the derived page has not changed, so moving from Home to a contributed page derived `'home'` twice and fired nothing at all, while moving there from a dashboard fired a confident `'home'`. `derivePage` now falls through to the page registry rather than to `'home'`. Asked of the registry rather than matched here, so there is one definition of which paths a page owns -- `isFrontendRoute` and `ExtensionPage` already read that same one, and a fourth representation of it would be the thing most likely to disagree later. The registry answers live, because extensions register long after this module is evaluated, so a registration has to be able to change the answer. That needs the location it is being judged against: a registration is not a navigation and carries no pathname, and the router's lives in `useNavigationTracker`'s ref, out of reach here. Hence `lastPathname` and `rederive`, wired to registration and to unregistration both -- without the second, a disposed page would leave `getPage()` naming a surface nothing serves any more. `'extension'` joins the `Page` union. Nothing in the tree switches exhaustively over `getPage()` or keys a `Record` by `Page`, so it is additive here and a compile-time change only for a published consumer. What this does not fix: one contributed page to another fires nothing, since both derive `'extension'`. That is the union carrying a surface rather than an identity, and it wants an accessor that names the page -- on the `pages` namespace, which already owns `ResolvedPage`, rather than on `navigation`, which is deliberately payload-free. Left alone here rather than half-done. Seven tests. Five fail against the previous code; the two that do not are guards on behaviour that was already right -- a path merely sharing a prefix with a page, and a host route keeping precedence at a path a page claims.
promising an import that will not compile
Two findings from review, both small.
**The navbar entry was never highlighted while its own page was open.**
`activeTabs` is a four-case switch over the host's own screens, so a
contributed entry -- keyed `extension:<path>` -- fell to the default and
cleared the selection. The lookup now asks the page registry, which keeps one
definition of which paths a page owns: `isFrontendRoute` and `derivePage`
read the same one, and it answers by longest match on segment boundaries, so
`/reports/archive` wins over `/reports` and `/reports-weekly` belongs to
neither.
It is asked before the host cases, not in the default branch, because three
of those cases match by unbounded prefix: a page at `/dashboard-review` would
otherwise underline Dashboards. Ordering it first cannot cost a host screen
its own highlight, since the server refuses a page whose path it already
serves and a refused page is never registered. `contributedPages` joins the
dependency array, or a cold load onto a contributed page would never
highlight anything, there being no navigation to re-run the effect.
A Settings-menu entry is untouched and stays unhighlighted: `RightMenu`
renders `selectable={false}` and passes no `selectedKeys`, so no Settings
child is highlighted, contributed or otherwise.
**Three places told authors to import `@superset-ui/core` as though it would
compile.** A `shared` entry is a runtime arrangement that TypeScript never
sees, so the import fails to resolve until the package is installed locally
too -- the project's own rule, in `quick-start.md`, already says a shared
package belongs in both `peerDependencies` and `devDependencies`. The text in
the published `.d.ts`, in `contribution-types.md` and in `UPDATING.md` now
says so.
The same section also warns which types you get: the copy on npm and the copy
this host runs carry the same version number with about two years between
them, and neither side of the share declares a `requiredVersion`, so a symbol
the published types export but the host no longer does compiles and is
`undefined` at runtime.
`architecture.md` and `quick-start.md` were still describing the share set
without `@superset-ui/core`, which this branch added to the CLI template;
both now list it, and `architecture.md` says plainly that only
`@apache-superset/core` is rebound per extension.
Six tests, five of which fail against the previous code. Not fixed here: the
scaffold still does not declare the dependency, because whether it should
depends on whether the share survives at all.
…st what was only asserted `useNavigate` concatenated the page root with the caller's sub-path and pushed the result. `history.push` keeps `..` verbatim while the browser's URL parser resolves it out of the address bar on `pushState`, so a target with a parent segment left the router and the URL disagreeing -- and enough of them climbed past the router's basename. The target is now parsed with the same algorithm the browser applies, which covers `%2E%2E` with it, and one that resolves outside the page is logged and dropped. Query and fragment are re-appended, since `history.push` parses them off the whole string. Not a boundary: extension code runs in the host realm and reaches `window.history` directly. The public doc said a page "cannot navigate out of its namespace by accident", which was the part that was untrue, and now says what happens instead. `onDidRegisterPage` and `onDidUnregisterPage` are typed `Event<T>`, whose subscribe signature takes `(listener, thisArgs?)`, but the wrappers forwarded only the listener -- so the idiom the type invites, `onDidRegisterPage(controller.addPage, controller)`, ran `addPage` unbound. Forwarded, as `navigation` and `editors` already do. Contributed navbar entries are sorted by path. Remote entries are injected in parallel and register as they finish, so the registry's order was the order the network happened to return and the bar could come out differently on the next reload. Path rather than label: the server refuses a second page claiming a path, so it needs no tiebreak, and it is not translated, so the order does not move with the reader's locale. Three tests for behaviour that already held but rested on nothing. The `registerPageFor` binding is what stops one extension mounting a component on another's declared path, and only the registry's own tests touched it, with hand-picked arguments -- so a closure forwarding the wrong id stayed green. A manifest declaring one path twice is refused on the second, and what the frontend is told now comes from the rule's recorded requirement, so a looser duplicate cannot advertise a page the server answers 403 for. And the rollback fixture counted reads of `.session` rather than calls to `rollback()`, so its assertion passed whether or not the failed insert was rolled back. The permission example in the page docs named the grant as `can_review on acme.tools`; the host creates and checks it against `extension:acme.tools`.
…ble page registry `fire` notified listeners with a bare `forEach`, so the first one to throw took the rest of the notification with it. Listeners come from extensions as well as the host, they share one set per event, and `fire` is called after the operation it announces has taken effect -- a page is already in the registry when `onDidRegisterPage` fires. So one bad listener stopped its peers being told and surfaced inside whichever extension happened to trigger the event, aborting that module's evaluation and every registration below it. Each listener is now isolated and a throw is logged. This is `createEventEmitter`, not the pages registry, because the same emitter backs navigation, menus, views, editors and chat. `getPages` returned the module-level array that backs the registry, and it is public API, so an extension could rewrite a page's `path` and point a live route somewhere else. Frozen at the point the snapshot is rebuilt rather than copied on read: `getPages` is also the `getSnapshot` for `useSyncExternalStore`, which compares by reference, so a fresh array per call would hand React a new snapshot on every render and never settle. The published signature says `readonly` now, which is what it always meant; `withExtensionPages` and `withExtensionSettings` take a readonly list to match, neither having mutated it. The doc for `getPages` also claimed registration order without saying that across extensions this is the order their bundles finished loading.
…h that shadows the app root Every other `url` in the menu is already rooted -- the backend emits host entries through `url_for` -- and `buildMenuItem` strips the root again before handing the path to `NavLink`. A contributed entry carried the bare manifest path, which survived that round trip only because `stripAppRoot` leaves an unrooted path alone. It now carries the root like its neighbours, so the convention holds rather than happening to work. One path cannot be fixed that way. Where a page's own first segment is the application root, "already rooted" and "starts with the root segment" are indistinguishable, so `ensureAppRoot` leaves it alone and the entry points at a URL the server does not serve: with the app at `/analytics`, a page declared at `/analytics/review` really answers at `/analytics/analytics/review`. The menu lookup misses the registry, the entry degrades from an SPA link to a plain one, and the click 404s. The client cannot tell the two apart, so the path is refused at registration instead, beside the existing route-collision check. Deployment-dependent, like that check: a manifest that is fine at the domain root is refused under a subdirectory, which is the trade `register_extension_pages` already makes. The root is matched whole, so `/analytics-archive` is unaffected, and a deployment at `/` refuses nothing.
A page is half a contribution: the manifest declares the path, the bundle supplies the component. With no frontend there is no remote entry, so nothing can ever register one -- the rules would be installed, the nav would advertise the pages, and every one of them would answer the shell and then resolve to nothing. Refused as a set, because the cause is the manifest rather than any one path. The test fixtures declared pages without a frontend, which is a shape the loader never produces, so they now carry one.
…haring @superset-ui/core Sharing `@superset-ui/core` through Module Federation made the whole surface of a large internal package into extension API by accident. Nothing said which parts an extension could rely on, the package moves between releases, and its published copy and the one in this repo carry the same version number with about two years between them -- so an extension could type-check against one package and run against another. What extensions actually needed from it was `SupersetClient`: reaching this Superset's REST API with the session and a CSRF token on writes. That is now `http` on `@apache-superset/core` -- `get`, `post`, `put`, `delete`, each taking an `endpoint` and returning the parsed `json`, the `status` and the response `headers`. The host side is a thin adapter over `SupersetClient`, so the transport is unchanged and only the contract is new: a named surface the host can keep stable while the package behind it moves. An endpoint must be a path under this Superset. An absolute URL is refused, and so is a protocol-relative one, which resolves to another origin while still beginning with a slash -- the call carries the session and the CSRF token, and neither belongs to anyone else. The share is dropped from the host and from the scaffold, and the docs no longer tell authors to import `SupersetClient`. An extension that imports `@superset-ui/core` now bundles its own copy, which is never configured, so its calls go out without a CSRF token; UPDATING says so and points at `http`.
… one dropdown The bar has no overflow menu -- it is rendered with antd's `disabledOverflow` -- so entries that do not fit wrap onto a second row under the logo, and the header, being `z-index: 10`, paints that row over the page beneath it. Nothing displaced the host's own items, since contributed entries are appended, but the damage above them was real: measured against the real cascade the first row holds roughly five contributed entries at 1440px and three at 1280px, and the host's own four already fill it at 820px with no extensions at all. Contributed entries are now capped, the dropdown counting against the cap, so the bar holds the same number whether one extension is installed or thirty. The fold reuses what the menu already does: an item with children and no `url` renders as a dropdown, and `isFrontendRoute` is computed per render for children as well as for items, so a folded page is still an SPA link. Its key is unchanged, so a page inside the dropdown still highlights while it is open. Nothing is dropped -- every contributed page is either on the bar or in the dropdown. This bounds the growth rather than promising the bar never wraps; no single number can, across that range. Making the bar itself responsive -- or giving antd's overflow back -- is the complete answer, and it belongs with the host's own chrome rather than with extensions.
…e current one `navigation.getPage()` names the surface, and every contributed page is `'extension'` there -- so moving from one contributed page to another is not a change in that value and fires nothing. That is correct for what the `Page` union means, and useless to an extension that contributes two pages and needs to know which one the user is looking at. `pages.getCurrentPage()` answers the finer question, and `pages.onDidChangeCurrentPage` reports it changing. It fires on a move between two contributed pages, on leaving for a host screen (with `undefined`), and when an extension registers a page for the URL the user is already sitting on -- which is the normal case rather than the exception, the bundle landing after the route. A move within one page's sub-paths is not a change and fires nothing. The registry is told where the user is by the navigation tracker, which already had the pathname, and recomputes on registration too, from the rebuilt snapshot so the lookup sees the registration that caused it. Observing only: an extension reads the current page, it does not set it. A test in `navigation` pins the other half -- that the surface deliberately does not change on that move -- so the two cannot drift into agreeing.
Two CI failures, both mine and both from running the wrong command locally: `oxlint` without `--config oxlint.json` reports nothing, and the formatter is `oxfmt`, which I had not run at all. `findPageByPath` is now declared above the current-page tracking that calls it. No behaviour change -- nothing ran before module evaluation finished -- but `no-use-before-define` was right that the reference read backwards, and the helper belongs beside the snapshot it reads. The five test files are reformatted by `oxfmt`, untouched otherwise.
`build_extension_data` pairs `registered` with `permitted`, and that pair decides the navigation entry, whether a component may be registered, and which refusal the page shows. Both halves were tested separately and the serializer that joins them was not, so swapping the two would have passed everything. It now has a test for all three outcomes -- an open page, a permission-gated page the user cannot open, and a path a host route already serves -- and swapping the flags fails exactly that test, out of the 329 in this suite. `useSubPath` was only ever exercised with a page root that shares no segment with the app root, so a second `stripAppRoot` would have gone unnoticed: `/review-queue/42` is untouched by one. With the app at `/analytics` and a page at `/analytics/review-queue`, a second strip eats the page's own first segment and the page reads its sub-path as `/`. Covered now for `useSubPath` and `useNavigate` alike. The server refuses such a path at registration, so it should not arise in a deployment -- but that guard is the server's, and these hooks have to be right on their own.
The production build failed on `export { del as delete }`: `declare
function del` is type-only and erases, so the aliased export was left
bound to nothing and the bundler refused the module. `tsc --noEmit` and
jest both pass it -- neither bundles this package's source -- which is why
every local check was green while bundle-size, docker-build and playwright
were red.
The module is type-only stubs by design, so giving `del` a real binding is
not an option, and `delete` cannot be a declaration name. The verb is `del`
in the published declaration and in the host namespace that implements it,
with the reason stated where someone will reach for `delete` and wonder.
Verified against the thing that failed: a full production build now
compiles 11,605 modules with no errors, where CI reported one.
A page whose feature is switched off should not sit in the Settings menu leading to a screen about something that is not running. Everything needed was already here: page visibility is computed per request and per user through `permitted_page_paths`, and the menu filters on `permitted !== false`. Only the condition was missing. `Page` takes an optional `featureFlag`, recorded alongside the permission when the rule is registered. A page whose flag is off is left out of what the frontend is told about, and its route answers 404 rather than rendering a shell for a feature that is not there. 404 and not 403, and checked before the permission: with the flag off there is no page, which is not a question about who is asking, so an administrator must not see it either. Additive and backward compatible -- a page that declares no flag behaves exactly as before. `_page_requirement` now returns a `PageRequirement` rather than a bare permission, so the two conditions travel together instead of a second lookup having to find the flag somewhere else.
amaannawab923
force-pushed
the
feat/extension-pages
branch
from
November 8, 2026 13:29
287d263 to
d8f02b8
Compare
Three ways the adapter did something other than what its types say, all invisible to the existing tests because they assert against a mocked client rather than what goes out. `del` dropped a `jsonPayload`. `callApi` turns a payload into a body for POST, PATCH and PUT alone, so a delete carrying one sent no body and no content type, and an endpoint reading `request.get_json()` refused the request instead of performing it. Only `get` is documented as ignoring a payload, and a body on DELETE is a legal request, so it is serialized here rather than quietly lost. `searchParams` replaced the endpoint's own query instead of joining it, because `callApi` builds the URL by assigning `url.search`. An endpoint carrying a rison `q=(...)` filter -- which is how Superset's API is asked for anything but the default page -- lost it the moment a caller passed any search param. The query is now split off and merged, with a key given in both taken from `searchParams`. A 204 was rejected. The default `parseMethod` calls `response.json()`, and Werkzeug sends a 204 with no body at all, so the usual answer to a successful delete threw a SyntaxError carrying neither status nor headers: a request that succeeded arrived as an error with nothing to say about itself. The body is parsed here instead, and `json` is typed as possibly undefined to say so. A non-2xx still rejects with the response. Tested through the real client rather than a mock: `index.wire.test.ts` asserts the request that actually goes out and the response that comes back, and four of its cases fail without this change.
…not open `permitted` is one answer to two questions. The server leaves a page out of `permitted_page_paths` both when the user lacks its permission and when its `featureFlag` is off, and the client read every `permitted: false` as a refusal. So with the flag off, SPA navigation -- a bookmark inside the app, a configured logo target -- told even an Admin to ask an administrator for a permission the page may never have declared, while a cold load of the same URL answered 404. The flag is re-read on the client from the same resolved `FEATURE_FLAGS` map the server computed `page_available` from and served in the same bootstrap, so the two ends cannot disagree about a flag, and a flag-off page now renders what its URL answers: not found. Also adds the case the flag tests were missing. Every one of them built a `PageRequirement` by hand, so nothing exercised the path from a manifest's `featureFlag` to the route or to `permitted_page_paths`: dropping `feature_flag=page.featureFlag` in `register_extension_pages` left all of them green while a flagged-off page stayed open and in the nav. The new case registers a manifest page both ways round, and fails if that wiring is removed.
…t client `featureFlag` was accepted by the manifest schema and described nowhere: absent from the contribution-types page and from the `Page` type, so an author could not discover it and `getPages()[0].featureFlag` did not type-check. It is now on `Page` and has a section of its own, and the line promising that a page without `permission` is open to any signed-in user is qualified, which stopped being true for a page that declares a flag. The page API also pointed authors at `@superset-ui/core` for `SupersetClient`, offered "in the share scope". webpack deliberately no longer shares it -- sharing it had made that whole internal package extension API by accident -- so an author following this bundled a second, unconfigured client, and a write through it went out without the host's CSRF token and failed validation. It points at the `http` namespace instead, and drops the instruction to declare the package in the extension's own `package.json`.
`localeCompare` collates in the runtime's default locale -- the reader's, in a browser -- however ASCII the input, so a Czech reader sorts `ch` after `h` and a Danish one `aa` after `z`. On the navbar that order does more than arrange: it decides which entries keep a slot on the bar and which fold into the dropdown, so two readers sorting differently saw different navigation rather than the same navigation differently arranged. Compared by code unit instead, which `PAGE_PATH_PATTERN` makes the plain alphabetical order anyway, and which is that in every runtime. `withExtensionSettings` now sorts the same way, for the same reason minus the fold. The message for a page the server registered no rule for named two causes out of four. A path can also be refused at boot for naming a reserved `permission`, or for a first segment equal to the application root, and the server logs which -- while a page added to the manifest after the server started was not refused at all and is the one cause the log has nothing to say about. Enumerating a subset sent an author looking for a conflict that did not exist, so it points at the log and names only what the log cannot.
The flag fix reproduced the hole it was fixing. `permitted` is read on the
client by consulting the page's own `featureFlag`, which only works while
`build_extension_data` keeps emitting it -- and dropping that field left
345 tests green while an Admin with the flag off was told again to ask for
a permission. The same complaint sadpandajoe made about the route wiring,
one file over. `test_build_extension_data_carries_a_pages_feature_flag`
pins the field, and fails when it is dropped.
Three things the http change did that nobody asked for:
The endpoint's query was round-tripped through `URLSearchParams` whether
or not anything had to be merged into it, so `?q=(page_size:1)` went out
as `?q=%28page_size%3A1%29`. Werkzeug reads them the same, but that
literal URL is what `callApi` keys its ETag cache on and what access logs
and proxy rules match, and nobody asked for the spelling to change. The
endpoint is now passed through untouched unless there is something to
merge.
A caller's `Content-Type` overrode the adapter's on a `del` body, so
`headers: {'Content-Type': 'text/plain'}` sent a JSON body under a type no
endpoint would parse -- while the published contract says the host
supplies the content type. The adapter's now wins for the body it
serialized.
A valueless key gained an `=` on the merge path. Unchanged behaviour for
Flask, mentioned here because it is still a change.
Also the 204 fix only ever covered an empty body: a 2xx whose body is not
JSON -- a sign-in page served to an expired session, a proxy's error page
-- still threw a bare SyntaxError with neither status nor headers, which
is the failure geido described. It now carries both, plus enough of the
body to recognise.
Two published claims were left false by the flag work. `ResolvedPage
.permitted` still said it is "always true for a page that declares no
permission", which stopped being true the moment a flag could turn it off,
and the client comment said the two ends "cannot disagree" about a flag.
They can, for as long as a tab stays open: `window.featureFlags` is baked
into the bootstrap at load while `permitted` comes from a per-request
call, so a flag switched off mid-session leaves the stale map saying it is
on until the page is reloaded.
The server and the browser disagree about what "already served" means. `_first_claimant` asks Werkzeug, whose rules match exactly, so a page at `/welcome/review` is accepted at boot -- `/welcome/` does not serve its descendants. The client mounts its routes without `exact`, and `<Switch>` takes the first match in array order, so the host's `/welcome/` claims the path by prefix and the contributed catch-all below is never reached. The page registered, appeared in the navigation, and opened Home. Measured across the route table, 30 of 31 such paths rendered a host screen instead of the page. Neither of the obvious fixes is right. Matching contributed pages ahead of host routes inverts the precedence the server enforces and would let an extension take `/sqllab`. Refusing nested paths on the server means Flask reproducing React Router's prefix semantics -- a second route table that has to agree with the first on every host route, forever. So the check runs where the matcher already lives: at registration, the declared path goes through `matchPath` against `RoutePaths`, and a page any host route claims is refused with the route named. `RoutePaths` rather than the assembled `routes` array because it is the same set of host paths, it is a plain constants module, and importing the route table here would close a cycle through `ExtensionPage`. This does not make those paths work. It turns thirty silent failures into thirty loud refusals, which is the difference between a bug an author can see and one their users report. Two comments asserted the invariant this disproves -- the catch-all's, which presented host precedence as a guarantee without saying the page is then unreachable, and one in `core/navigation/index.test.ts` claiming the server refuses these. Both corrected.
SIP-235 lists a separate page-registration API under rejected
alternatives: pages are views at a reserved location. Both reviewers
arrived at that from different symptoms, and the SIP thread settled it.
This is that shape.
A page registers as `views.registerView({ id, name }, PAGE_LOCATION,
Component)` with `PAGE_LOCATION` as `global.page`, and the manifest
declares an id rather than a path. The host serves every page at
`/extensions/pages/<id>`: an extension names its page, it does not choose
where the page is served from.
Most of this change is deletion. With no author-written path there is no
path to validate, nothing for two extensions to collide over, and nothing
that can land on a route Superset already answers -- so the path pattern,
the boot-time collision check and the rule-reaches-into scan all go, along
with the `/:extensionPagePath+` catch-all and the registration guard that
stood in front of it. `isFrontendRoute` becomes a prefix test on the
host's own namespace instead of a lookup into a registry that fills in
after module evaluation.
That also answers the route-shadowing finding directly rather than
guarding it. A page declared beneath a host screen's path was accepted at
boot -- Werkzeug's rules match exactly, so `/welcome/` does not serve
`/welcome/review` -- while the client matches on a prefix and rendered the
host screen instead, measured at 30 of 31 such paths. There is no longer a
path to declare, so the class is gone.
The review fixes already on this branch are kept: the http namespace
contract, the feature flag told apart from a permission, `featureFlag`
typed and documented, and the contributed navbar entries ordered by code
unit rather than the reader's locale -- keyed on the page id now that
there is no path.
msyavuz
reviewed
Nov 9, 2026
|
|
||
| const seen: unknown[] = []; | ||
| disposables.push(onDidChangeCurrentPage(page => seen.push(page))); | ||
| notifyPathnameChanged('/review-queue/42'); |
Member
There was a problem hiding this comment.
This test was left over from the old path scheme: /review-queue/42 is outside the page, so recomputeCurrentPage fires undefined, seen becomes [undefined] instead of [], and the test fails. It should move to /extensions/pages/acme.tools.review-queue/42.
Contributor
Author
There was a problem hiding this comment.
Fixed in 8fc5271. notifyPathnameChanged('/extensions/pages/acme.tools.review-queue/42'), so the move is actually inside the page and seen is empty for the reason the test name gives.
The same vacuity was in navigation/index.test.ts — /approvalsomething was testing nothing once the prefix moved, so that one now pushes /extensions/pagesomething, which is the prefix-without-a-segment-boundary case that still exists.
| for extension in get_extensions().values(): | ||
| manifest = extension.manifest | ||
| for page in manifest.contributions.pages: | ||
| if page.id == page_id: |
Member
There was a problem hiding this comment.
The server never checks that a page id starts with its extension's id (the Page.id pattern doesn't either), so if two extensions declare the same id, the route enforces whichever manifest get_extensions() returns first; another extension declaring acme.tools.queue with no permission or flag can open that shell route even though acme.tools gates the page with a permission.
Contributor
Author
There was a problem hiding this comment.
Confirmed, and fixed in 8fc5271. owns_page(extension_id, page_id) is now the one rule, applied in the three places that read a manifest's pages:
_declared_requirement— the enforcement path. A declaration the extension does not own is skipped, so the owning extension's requirement is the one the route reads.ensure_page_permissions— no permission row is created for a page an extension does not own, and the reason is logged where the author sees it at load.permitted_page_ids— not advertised, sinceregisterPageForwould refuse to register it and a nav entry would point at "page not found".
registerPageFor and the published Page.id already stated the rule; the server was the half that did not hold to it.
test_the_owning_extension_is_the_one_that_guards_a_page builds exactly the case you describe — squatter first in the dict, declaring neither a permission nor a flag — and it reports other.ext without the check.
| @pytest.mark.parametrize("permission", ["Can_Review", "can review", "can-review", ""]) | ||
| def test_a_malformed_permission_is_rejected_by_the_manifest(permission: str) -> None: | ||
| with pytest.raises(ValidationError, match="should match pattern"): | ||
| Page(path="acme.tools.a", name="A", permission=permission) |
Member
There was a problem hiding this comment.
Page(path=...) still uses the removed field, so id is missing and validation fails for that reason; the should match pattern match only passes because pydantic lists every error, so this test would stay green even if the permission pattern stopped working.
Contributor
Author
There was a problem hiding this comment.
Fixed in 8fc5271 — Page(id="acme.tools.a", ...), with a note on why the field matters here: with id missing, pydantic reports that too and match="should match pattern" passes off the wrong error.
| import { pages } from '@apache-superset/core'; | ||
| import ReviewQueue from './ReviewQueue'; | ||
|
|
||
| pages.registerPage( |
Member
There was a problem hiding this comment.
The Pages section still documents the model d3006da removed: a manifest path, pages.registerPage, choosing a path, server-side collision checks and the 403 wording. An author copying it writes a call that no longer exists, and it contradicts the id-based example at line 342 of the same page.
Contributor
Author
There was a problem hiding this comment.
Rewritten in 8fc5271. "Choosing a path" is now "Choosing an id" and says the host forms the address at /extensions/pages/<id>; the manifest example uses id, and the registration example is views.registerView({ id }, views.PAGE_LOCATION, Component), which is what the code actually offers. The sub-path example shows the real address. The 403 wording now matches what happens — the permission is refused at startup and logged, so the page is not openable.
Also removed the "Drawing a chart" section in the same pass: it documented charts.AssetChart, and charts is not part of this change.
| * // "contributions": { "pages": [{ "path": "/review-queue", "name": "Queue" }] } | ||
| * import { pages } from '@apache-superset/core'; | ||
| * | ||
| * pages.registerPage({ path: '/review-queue', name: 'Queue' }, ReviewQueue); |
Member
There was a problem hiding this comment.
The published file overview and the useSubPath/useNavigate docs still show pages.registerPage({ path: ... }) and author-chosen paths like /review-queue. That is the API this commit deleted, so the shipped typings tell extension authors to call a function that doesn't exist.
Contributor
Author
There was a problem hiding this comment.
Rewritten in 8fc5271. The fileoverview now describes declaring an id and registering the component at views.PAGE_LOCATION, with the host forming the address; the useSubPath and useNavigate docs use /extensions/pages/acme.tools.review-queue/42 rather than an author-chosen /review-queue.
Same prose was in the host-side registry, so src/core/pages/index.ts got the same treatment — its fileoverview still said the server registers a URL rule and checks the path against every route the deployment serves.
| import * as supersetCore from '@apache-superset/core'; | ||
| import { | ||
| authentication, | ||
| charts, |
Member
There was a problem hiding this comment.
charts is imported from @apache-superset/core and from src/core (Namespaces.ts too), but no charts module exists in either package, so this fails to compile.
Contributor
Author
There was a problem hiding this comment.
Gone as of c796470 — the charts import went out with the view-slot scope cut, so ExtensionsStartup no longer references a module neither package provides. Namespaces.ts has no charts entry either.
| "types": "./lib/authentication/index.d.ts", | ||
| "default": "./lib/authentication/index.js" | ||
| }, | ||
| "./charts": { |
Member
There was a problem hiding this comment.
There is no src/charts in the package, so the ./charts export points at a lib/charts that the build never produces, and a published @apache-superset/core ships a broken subpath.
Contributor
Author
There was a problem hiding this comment.
Removed in 8fc5271. There is no src/charts, so the subpath resolved to a lib/charts the build never produces; with charts out of this PR's scope the export had nothing left to point at.
| #: Publisher names the host reserves, because `/extensions/list/` and | ||
| #: `/extensions/pages/<id>` are its own routes under the same prefix an | ||
| #: extension's API is served from. | ||
| RESERVED_PUBLISHERS = frozenset({"list", "pages"}) |
Member
There was a problem hiding this comment.
RESERVED_PUBLISHERS is defined but never checked anywhere, so an extension published as pages or list still gets an API prefix that collides with the host's /extensions/pages/<id> and /extensions/list/ routes.
Contributor
Author
There was a problem hiding this comment.
Now enforced, in 8fc5271, rather than deleted — the collision you describe is real.
A field_validator on BaseExtension.publisher covers both ExtensionConfig and Manifest, which is what the host actually reads. validate_publisher in the CLI refuses it as well, so an author is told at superset-extensions init rather than at load. Tests on both paths.
| * the path is taken -- so it is sent here as the authoritative list of | ||
| * paths the extension is allowed to register a component against. | ||
| */ | ||
| contributions?: { |
Member
There was a problem hiding this comment.
This comment still says the server registers a URL rule per page and refuses taken paths, which the id-based model removed, so it misdescribes what contributions.pages now means.
Contributor
Author
There was a problem hiding this comment.
Rewritten in 8fc5271. It now says what contributions.pages is: the ids this extension may register against, and what the server reads to decide whether to answer /extensions/pages/<id> and who may open it — plus what permitted is for.
…change Taking the page addressing across brought the list-view pages and the extension namespace registration with it, and those belong to the view-locations work: `ChartList`, `DashboardList` and `DatasetList` referenced a `ViewListExtension` this branch does not carry, and `Namespaces` and `ExtensionsStartup` imported a `charts` namespace it does not export. Neither is needed for a page to be a view at a reserved location, and tsc said so. They go back to what this PR already had.
…ntract
Four CI failures, one cause and two strays.
The cause: realigning pages onto SIP-235 was done by porting from a branch
that also carries the view-slot work, so material belonging to that change
came along -- `core/viewLocations.ts`, a Home test for `home.section`, and a
registry hook for `root.context.provider`. The implementation side was then
cut from this branch, leaving the rest referring to things that are not here:
- `Home.sections.test.tsx` asked the Home page for a contributed section.
Nothing on this branch mounts one, so five tests looked for a button that
was never going to be rendered.
- `useRegisteredExtension` subscribed to the extensions registry.
`TypedRegistry` has no `subscribe` -- its own comment says listener
methods were never added -- so the hook threw at mount and took
`RootContextProviders` with it. Reverted to the bare `get` it had.
- `core/viewLocations.ts` named slots in SqlLab, Explore, the dashboard and
the list views. Nothing on this branch reads it.
The strays:
- `PanelToolbar` called `commands.getCommand(item.command)` on a `MenuItem`
whose `command` this change made optional, a page item having somewhere to
go rather than something to do. A toolbar renders commands, so an item
without one is skipped, and the type now says so at both call sites.
- The http adapter dropped the `json as T` its predecessor had when it moved
to parsing the body itself. The published type is `json: T`, and a 204 or
an empty 200 has nothing to parse, so the cast is restored with a note
saying which case it covers.
…prose Review on #45522 found eleven things the id-based page model left behind. Two more it flagged were already gone with the view-slot scope cut. The one that mattered: the server never checked that a page id belongs to the extension declaring it. `registerPageFor` has always refused a registration whose id does not begin with its extension's own id, and `Page.id` says so, but `_declared_requirement` matched on the id alone and returned whichever manifest `get_extensions()` listed first -- so a second extension declaring `acme.tools.queue` with no permission and no feature flag answered for a page `acme.tools` had gated. `owns_page` now settles it in all three places that read a manifest's pages: the route's requirement lookup, the permissions created at load, and what is advertised to the frontend. A page an extension does not own is ignored, loudly, the way the frontend already ignores it. `RESERVED_PUBLISHERS` was defined and never read, so an extension published as `pages` or `list` still got an API prefix shadowed by the host's own `/extensions/pages/<id>` and `/extensions/list/`. Refused now, in the manifest schema and at `superset-extensions init`. `HttpResponse.json` was typed `T` while the adapter returns `undefined` for a 204 or an empty body, which type-checked and then threw on `json.result`. It is `T | undefined`, which is what it always was, and the host's namespace is checked against the published type instead of cast through `unknown`. The published `@apache-superset/core` exported a `./charts` subpath with no `src/charts` behind it, so the subpath resolved to a `lib/charts` the build never produces. The rest was prose and tests describing the model this branch replaced: `pages.registerPage({ path })` and "choosing a path" in the contribution docs and in the shipped typings, a `charts.AssetChart` section for a namespace this branch does not add, a `Page(path=...)` in a test that passed on the wrong half of a pydantic error, four tests exercising author-chosen paths at addresses no page can reach any more, and two comments spliced from both models -- including the claim that `derivePage`'s register/unregister re-derives still did something, which they have not since the id prefix replaced the registry lookup.
9 tasks
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Lets an extension contribute a top-level page — a screen of its own, at its own URL — as opposed to a view occupying a slot in a screen Superset already ships.
An extension declares the page in its manifest and registers a component for it at runtime:
Both halves are needed: the manifest declaration is what the server reads to register a URL rule, so a cold load resolves; the runtime registration supplies the component. Registering a component for a path the manifest does not declare is refused, so the two cannot drift.
Paths are checked, not namespaced
The path is the author's to choose, but not theirs to take. Before registering a rule, the server checks the path against the routes the deployment actually serves. Two checks, because neither subsumes the other:
/oauth-authorized/<provider>never string-matches the request it would answer, so a page could take an OAuth callback. Asking the router instead catches it, along with redirects (strict_slashes) and rules answering only other methods./reportwould shadow a core/report/list. Matching cannot find those — there is no single URL to probe that an arbitrary descendant is guaranteed to answer — so the rules are read directly. The comparison is segment-wise rather than textual, because a converter matches text that does not resemble it:/api/v1/datasource/<datasource_type>/...does not start with/api/v1/datasource/table/, yet it answers/api/v1/datasource/table/42. A converter matches any single segment;pathspans the rest.A page owns its subtree exclusively, so any rule reaching into it is a conflict — including ones Werkzeug's precedence would have resolved in the host's favour. Reimplementing that precedence here would be wrong in a way nobody notices, and "this path is in use, pick another" is a better error than a route that works until the arithmetic shifts.
Registration runs after operator blueprints, the FAB auth views and every core view, so the map it checks is complete. It re-runs on every boot, which is the part that holds over time: routes Superset registers conditionally — the OAuth callback exists only under
AUTH_TYPE = AUTH_OAUTH— are absent where the feature is off, so a page may legitimately take that path there. Enabling the feature later puts the real rule in the map first, and the next boot refuses the page loudly rather than leaving an extension in front of the callback.A refused page is skipped, not fatal. One extension claiming a taken path should not stop the server starting.
Two further layers back this up. The client route is a catch-all pushed last, so
<Switch>gives every host screen precedence the same way Werkzeug gives its static rules precedence — pinned by a test that derives host paths from the route table, so a route added in the wrong place is covered the day it is added. And the extensions API reports every declared page together with whether it got a rule, read back from the URL map, so a refused page is told apart from one never declared -- and neither the menu nor a component registration accepts one that has no rule.Permissions
A page may name a permission. The host creates it against the extension when the extension loads — not during
superset init, because an extension is installed long after a deployment is initialised and an administrator cannot grant a permission that does not exist. It reads ascan review on extension:acme.toolsin the Roles screen, so an extension's permissions group together and two extensions naming the same permission stay separate. Theextension:prefix is what lets_is_admin_onlyrecognise these without a registry of installed extensions:_is_gamma_pvmis a denylist, so an unrecognised view menu would be granted to stock Gamma and Alpha and the declared permission would stop restricting anything after the nextsuperset init. Superset hit this withUserRegistrationsRestAPI; a view menu carrying an extension id can never be listed inADMIN_ONLY_VIEW_MENUSthe way that one is, so it is recognised by shape instead.Omitting the field leaves the page open to any signed-in user, which is what every page did before this field existed.
A user without the permission gets 403 rather than FAB's
has_accessredirect to the login page — they are already signed in, so sending them to log in again says nothing true. Login still comes first, so an anonymous visitor is redirected rather than refused; 403 would disclose that the page exists to someone who has not identified themselves.The Admin role is given these permissions on creation, and the grant is re-checked each boot -- but only while no role at all holds the permission, so an operator who moves it to a role of their own keeps it there. A permission no role holds gates nothing and leaves the page reachable by nobody, which is the state a failed or racing first boot produces, so that is the one repaired. This is not a new policy:
sync_role_definitionsbuilds Admin from every permission that is not one of the four object-level ones, sosuperset initwould grant it anyway — just not until someone next ran init, which installing an extension does not otherwise require. Correspondingly, a page naming one ofdatabase_access,catalog_access,schema_accessordatasource_accessis refused, since those are exactly what Superset excludes when deciding Admin, and such a page would have its grant stripped at the nextsuperset initwith nothing in the logs to explain it.Also included
useSubPath()anduseNavigate(), so a page can own sub-paths (/review-queue/42/edit) and navigate within itself as a client-side transition. These exist becausereact-router-domis not in the Module Federation share scope — an extension importing it gets a second module instance whose context the host's<Router>never populates.@superset-ui/coreis added to the share scope, so a page getsSupersetClientwith working CSRF rather than hand-rolling fetches.Open question for reviewers
Paths are flat and author-chosen. The alternative is a host-derived namespace (
/extensions/<publisher>.<name>/<slug>), which makes collisions impossible by construction rather than by inspection, at the cost of long URLs. Grafana (/a/<plugin-id>/*) and Mattermost (/plug/<id>) namespace; Kibana (/app/<id>) does not.Flat paths are deliberate here, but they carry three costs worth naming:
/approvalsreads as part of Superset, with nothing saying third-party code serves it.Happy to move to a namespaced prefix if reviewers prefer it — the shape is set in
pagePathand the manifest's path pattern.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
After: a contributed page at
/approvalswith its navigation entry, alongside Dashboards / Charts / Datasets. A page whose permission the user lacks is absent from the navigation bar and answers 403 on a direct load.TESTING INSTRUCTIONS
Automated:
Manually, with
ENABLE_EXTENSIONSon and an extension declaring two pages (one with apermission):/review-queue/42/edit) — it renders, anduseSubPath()reports/42/edit./welcome. On restart the log names the conflict, the page is not registered, and/welcomestill serves Superset's own screen./api/v1/datasource/table, and confirm it is refused at boot and that/api/v1/datasource/table/42/column/x/values/still reaches the datasource API rather than the app shell.ADDITIONAL INFORMATION
ENABLE_EXTENSIONSNo migration: the permissions these pages use are created through the security manager when an extension loads, as FAB permissions are elsewhere.