codeant-ai-for-open-source[bot] commented on code in PR #45522: URL: https://github.com/apache/superset/pull/45522#discussion_r4389068049
########## superset-frontend/src/core/pages/index.ts: ########## @@ -0,0 +1,326 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * @fileoverview Host-side registry for extension-contributed pages. + * + * Mirrors `src/core/views`: module-level state, a `useSyncExternalStore` + * subscription so a registration arriving after first render still lands, and + * an `ExtensionPlaceholder` for a page whose component has not arrived. + * + * The one thing this adds is holding an extension to its manifest. A page's + * path is declared there, which is what the server reads when it registers a + * URL rule and checks the path against every route the deployment serves. A + * component registered against a path the manifest does not declare has no + * rule behind it: it would mount on client navigation and 404 on a cold load. + * Rejecting it here keeps the two halves from disagreeing. + */ + +import React, { + ComponentType, + createContext, + useCallback, + useContext, + useSyncExternalStore, +} from 'react'; +import { useHistory, useLocation } from 'react-router-dom'; +import type { pages as pagesApi } from '@apache-superset/core'; +import { logging } from '@apache-superset/core/utils'; +import PageBoundary from './PageBoundary'; +import ExtensionPlaceholder from 'src/extensions/ExtensionPlaceholder'; +import { Disposable } from '../models'; +import { createEventEmitter } from '../utils'; + +type Page = pagesApi.Page; +type ResolvedPage = pagesApi.ResolvedPage; + +/** + * A page as the server reported it: what the manifest declares, plus whether + * this user may open it. Bound per extension by the loader. + */ +export interface DeclaredPage { + path: string; + /** False when the server refused the path and registered no rule for it. */ + registered?: boolean; + permitted?: boolean; + /** Everything else the manifest declared about the page. */ + [field: string]: unknown; +} + +/** A registration that did not take, so a caller can dispose it harmlessly. */ +const INERT = new Disposable(() => {}); + +/** + * Mirrors `PAGE_PATH_PATTERN` in `superset_core.extensions.constants`. + * + * Checked again here because this is the last point before a path is handed + * to the router, and because the message a developer sees for a malformed + * path should arrive in the console they are already watching. + */ +const PAGE_PATH_PATTERN = /^(\/[a-z]([a-z0-9]*(-[a-z0-9]+)*)?)+$/; + +const pageRegistry: Map< + string, + { page: ResolvedPage; component: ComponentType } +> = new Map(); + +const syncListeners = new Set<() => void>(); +const subscribe = (listener: () => void) => { + syncListeners.add(listener); + return () => syncListeners.delete(listener); +}; + +const registerEmitter = createEventEmitter<ResolvedPage>(); +const unregisterEmitter = createEventEmitter<ResolvedPage>(); + +// `useSyncExternalStore` requires a referentially stable snapshot between +// notifications, so the array is rebuilt only when the registry changes. +let snapshot: ResolvedPage[] = []; +const refresh = () => { + snapshot = Array.from(pageRegistry.values(), entry => entry.page); + syncListeners.forEach(listener => listener()); +}; + +/** + * Registers a page on behalf of a specific extension. + * + * Not part of the public API: the loader binds `extensionId` and the paths + * that extension's manifest declares before handing the namespace to + * extension code, so an extension can neither claim another extension's id + * nor register a path it did not declare. See `ExtensionsLoader.loadModule`. + */ +export const registerPageFor = ( + extensionId: string, + declared: readonly DeclaredPage[], + page: Page, + component: ComponentType, +): Disposable => { + // Nothing here throws. An extension calls `registerPage` at module scope, + // so a throw takes down the whole bundle -- its other pages, its views, its + // commands -- over one page. Worse, one of these conditions is not the + // author's doing at all: the server refuses a path it has already given to + // something else, and the author cannot see that from their own source. The + // page is dropped, loudly, and the extension carries on. + if (!page?.path || !PAGE_PATH_PATTERN.test(page.path)) { + logging.error( + `Extension "${extensionId}" registered a page at ` + + `"${page?.path ?? '<missing>'}", which is not an absolute path of ` + + `lowercase segments such as "/review-queue". The page was ignored.`, + ); + return INERT; + } + + const declaration = declared.find(entry => entry.path === page.path); + if (!declaration) { + logging.error( + `Extension "${extensionId}" registered a page at "${page.path}", which ` + + `its manifest does not declare. Add it to contributions.pages and ` + + `rebuild: the server registers URL rules from the manifest, so this ` + + `page would work on client navigation and 404 on a cold load. ` + + `Declared: ${declared.map(entry => entry.path).join(', ') || '(none)'}.`, + ); + return INERT; + } + + if (declaration.registered === false) { + // Two causes, and from here they look the same: the path is held by + // Superset, an operator's blueprint or another extension, or the page was + // added to the manifest after this server started, since rules are + // registered at boot while the extensions API re-reads the manifest every + // request. Naming only the conflict sent an author who had just added a + // page looking for one that does not exist, so the message names both and + // says where the answer is. + logging.error( + `Extension "${extensionId}" declares a page at "${page.path}", but the ` + + `server registered no route for it, so the page was ignored. Either ` + + `the path is already served -- the server log names what holds it, ` + + `and the manifest needs a different path -- or the page was added ` + + `after the server started, in which case restart it.`, + ); + return INERT; + } + + const id = `${extensionId}:${page.path}`; + // Registered whether or not the user may open it, so that the component is + // there to render a refusal rather than a blank screen, and so an + // extension's own bundle never has to branch on permissions. + // + // The manifest's own fields come first, so a page that declares `nav` or + // `name` there does not have to repeat them in code to be seen. They are + // only defaults: anything the registration passes wins, because that is the + // call the author wrote. + const { + path: _path, + registered: _registered, + permitted, + ...declaredFields + } = declaration; + const resolved: ResolvedPage = { + ...(declaredFields as Partial<Page>), + ...page, + extensionId, + permitted: permitted !== false, + }; + + const replaced = pageRegistry.get(id); + if (replaced) { + // Within one extension only -- ids carry the extension, and the server + // has already refused a path two extensions both declare. Silent + // replacement would make the first registration stop rendering with + // nothing said about it. + logging.warn( + `Page "${id}" is already registered; it has been replaced and the ` + + `earlier component no longer renders.`, + ); + } + + const entry = { page: resolved, component }; + pageRegistry.set(id, entry); + refresh(); + registerEmitter.fire(resolved); + + return new Disposable(() => { + // Only while this registration still holds the id: disposing one that has + // since been replaced would remove the replacement instead. + if (pageRegistry.get(id) !== entry) { + return; + } + pageRegistry.delete(id); + refresh(); + unregisterEmitter.fire(resolved); + }); +}; + +export const getPages = (): ResolvedPage[] => snapshot; Review Comment: **Suggestion:** `getPages()` exposes the cached array and page objects; a caller can mutate them and make route lookup lose or misroute pages still in the registry. **Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Cache` [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6627653b2ba24d51a0dc3d307a328c14&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6627653b2ba24d51a0dc3d307a328c14&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent ๐ค </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/core/pages/index.ts **Line:** 210:210 **Comment:** *Cache: `getPages()` exposes the cached array and page objects; a caller can mutate them and make route lookup lose or misroute pages still in the registry. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45522&comment_hash=43343d501fdc099b9e92d3083e5f38930616d85106778b281f984fcc79db0465&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45522&comment_hash=43343d501fdc099b9e92d3083e5f38930616d85106778b281f984fcc79db0465&reaction=dislike'>๐</a> ########## superset-frontend/src/core/pages/index.ts: ########## @@ -0,0 +1,326 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * @fileoverview Host-side registry for extension-contributed pages. + * + * Mirrors `src/core/views`: module-level state, a `useSyncExternalStore` + * subscription so a registration arriving after first render still lands, and + * an `ExtensionPlaceholder` for a page whose component has not arrived. + * + * The one thing this adds is holding an extension to its manifest. A page's + * path is declared there, which is what the server reads when it registers a + * URL rule and checks the path against every route the deployment serves. A + * component registered against a path the manifest does not declare has no + * rule behind it: it would mount on client navigation and 404 on a cold load. + * Rejecting it here keeps the two halves from disagreeing. + */ + +import React, { + ComponentType, + createContext, + useCallback, + useContext, + useSyncExternalStore, +} from 'react'; +import { useHistory, useLocation } from 'react-router-dom'; +import type { pages as pagesApi } from '@apache-superset/core'; +import { logging } from '@apache-superset/core/utils'; +import PageBoundary from './PageBoundary'; +import ExtensionPlaceholder from 'src/extensions/ExtensionPlaceholder'; +import { Disposable } from '../models'; +import { createEventEmitter } from '../utils'; + +type Page = pagesApi.Page; +type ResolvedPage = pagesApi.ResolvedPage; + +/** + * A page as the server reported it: what the manifest declares, plus whether + * this user may open it. Bound per extension by the loader. + */ +export interface DeclaredPage { + path: string; + /** False when the server refused the path and registered no rule for it. */ + registered?: boolean; + permitted?: boolean; + /** Everything else the manifest declared about the page. */ + [field: string]: unknown; +} + +/** A registration that did not take, so a caller can dispose it harmlessly. */ +const INERT = new Disposable(() => {}); + +/** + * Mirrors `PAGE_PATH_PATTERN` in `superset_core.extensions.constants`. + * + * Checked again here because this is the last point before a path is handed + * to the router, and because the message a developer sees for a malformed + * path should arrive in the console they are already watching. + */ +const PAGE_PATH_PATTERN = /^(\/[a-z]([a-z0-9]*(-[a-z0-9]+)*)?)+$/; + +const pageRegistry: Map< + string, + { page: ResolvedPage; component: ComponentType } +> = new Map(); + +const syncListeners = new Set<() => void>(); +const subscribe = (listener: () => void) => { + syncListeners.add(listener); + return () => syncListeners.delete(listener); +}; + +const registerEmitter = createEventEmitter<ResolvedPage>(); +const unregisterEmitter = createEventEmitter<ResolvedPage>(); + +// `useSyncExternalStore` requires a referentially stable snapshot between +// notifications, so the array is rebuilt only when the registry changes. +let snapshot: ResolvedPage[] = []; +const refresh = () => { + snapshot = Array.from(pageRegistry.values(), entry => entry.page); + syncListeners.forEach(listener => listener()); +}; + +/** + * Registers a page on behalf of a specific extension. + * + * Not part of the public API: the loader binds `extensionId` and the paths + * that extension's manifest declares before handing the namespace to + * extension code, so an extension can neither claim another extension's id + * nor register a path it did not declare. See `ExtensionsLoader.loadModule`. + */ +export const registerPageFor = ( + extensionId: string, + declared: readonly DeclaredPage[], + page: Page, + component: ComponentType, +): Disposable => { + // Nothing here throws. An extension calls `registerPage` at module scope, + // so a throw takes down the whole bundle -- its other pages, its views, its + // commands -- over one page. Worse, one of these conditions is not the + // author's doing at all: the server refuses a path it has already given to + // something else, and the author cannot see that from their own source. The + // page is dropped, loudly, and the extension carries on. + if (!page?.path || !PAGE_PATH_PATTERN.test(page.path)) { + logging.error( + `Extension "${extensionId}" registered a page at ` + + `"${page?.path ?? '<missing>'}", which is not an absolute path of ` + + `lowercase segments such as "/review-queue". The page was ignored.`, + ); + return INERT; + } + + const declaration = declared.find(entry => entry.path === page.path); + if (!declaration) { + logging.error( + `Extension "${extensionId}" registered a page at "${page.path}", which ` + + `its manifest does not declare. Add it to contributions.pages and ` + + `rebuild: the server registers URL rules from the manifest, so this ` + + `page would work on client navigation and 404 on a cold load. ` + + `Declared: ${declared.map(entry => entry.path).join(', ') || '(none)'}.`, + ); + return INERT; + } + + if (declaration.registered === false) { + // Two causes, and from here they look the same: the path is held by + // Superset, an operator's blueprint or another extension, or the page was + // added to the manifest after this server started, since rules are + // registered at boot while the extensions API re-reads the manifest every + // request. Naming only the conflict sent an author who had just added a + // page looking for one that does not exist, so the message names both and + // says where the answer is. + logging.error( + `Extension "${extensionId}" declares a page at "${page.path}", but the ` + + `server registered no route for it, so the page was ignored. Either ` + + `the path is already served -- the server log names what holds it, ` + + `and the manifest needs a different path -- or the page was added ` + + `after the server started, in which case restart it.`, + ); + return INERT; + } + + const id = `${extensionId}:${page.path}`; + // Registered whether or not the user may open it, so that the component is + // there to render a refusal rather than a blank screen, and so an + // extension's own bundle never has to branch on permissions. + // + // The manifest's own fields come first, so a page that declares `nav` or + // `name` there does not have to repeat them in code to be seen. They are + // only defaults: anything the registration passes wins, because that is the + // call the author wrote. + const { + path: _path, + registered: _registered, + permitted, + ...declaredFields + } = declaration; + const resolved: ResolvedPage = { + ...(declaredFields as Partial<Page>), + ...page, + extensionId, + permitted: permitted !== false, + }; + + const replaced = pageRegistry.get(id); + if (replaced) { + // Within one extension only -- ids carry the extension, and the server + // has already refused a path two extensions both declare. Silent + // replacement would make the first registration stop rendering with + // nothing said about it. + logging.warn( + `Page "${id}" is already registered; it has been replaced and the ` + + `earlier component no longer renders.`, + ); + } + + const entry = { page: resolved, component }; + pageRegistry.set(id, entry); + refresh(); + registerEmitter.fire(resolved); Review Comment: **Suggestion:** A throwing `onDidRegisterPage` listener escapes after registration, aborting module evaluation and leaving the page registered without returning its disposable. **Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Possible bug` [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=41b05967986043a18bc2bc89d408ee7e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=41b05967986043a18bc2bc89d408ee7e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent ๐ค </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/core/pages/index.ts **Line:** 196:196 **Comment:** *Possible Bug: A throwing `onDidRegisterPage` listener escapes after registration, aborting module evaluation and leaving the page registered without returning its disposable. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45522&comment_hash=c6d9e28bbb3102dd8d4a72dad683032d4510c1724f6826febdc0a17bf33c273e&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45522&comment_hash=c6d9e28bbb3102dd8d4a72dad683032d4510c1724f6826febdc0a17bf33c273e&reaction=dislike'>๐</a> -- 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]
