codeant-ai-for-open-source[bot] commented on code in PR #38253: URL: https://github.com/apache/superset/pull/38253#discussion_r2854566021
########## docs/scripts/generate-if-changed.mjs: ########## @@ -0,0 +1,306 @@ +/** + * 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. + */ + +/** + * Smart generator wrapper: only runs generators whose input files have changed. + * + * Computes a hash of each generator's input files (stories, engine specs, + * openapi.json, and the generator scripts themselves). Compares against a + * stored cache. Skips generators whose inputs and outputs are unchanged. + * + * Usage: + * node scripts/generate-if-changed.mjs # smart mode (default) + * node scripts/generate-if-changed.mjs --force # force regenerate all + */ + +import { createHash } from 'crypto'; +import { execSync, spawn } from 'child_process'; +import fs from 'fs'; +import path from 'path'; +import { fileURLToPath } from 'url'; + +const __filename = fileURLToPath(import.meta.url); +const __dirname = path.dirname(__filename); +const DOCS_DIR = path.resolve(__dirname, '..'); +const ROOT_DIR = path.resolve(DOCS_DIR, '..'); +const CACHE_FILE = path.join(DOCS_DIR, '.docusaurus', 'generator-hashes.json'); + +const FORCE = process.argv.includes('--force'); + +// Ensure local node_modules/.bin is on PATH (needed for docusaurus CLI) +const localBin = path.join(DOCS_DIR, 'node_modules', '.bin'); +process.env.PATH = `${localBin}${path.delimiter}${process.env.PATH}`; + +// --------------------------------------------------------------------------- +// Generator definitions +// --------------------------------------------------------------------------- + +const GENERATORS = [ + { + name: 'superset-components', + command: 'node scripts/generate-superset-components.mjs', + inputs: [ + { + type: 'glob', + base: path.join(ROOT_DIR, 'superset-frontend/packages/superset-ui-core/src/components'), + pattern: '**/*.stories.tsx', + }, + { + type: 'glob', + base: path.join(ROOT_DIR, 'superset-frontend/packages/superset-core/src'), + pattern: '**/*.stories.tsx', + }, + { type: 'file', path: path.join(DOCS_DIR, 'scripts/generate-superset-components.mjs') }, + { type: 'file', path: path.join(DOCS_DIR, 'src/components/StorybookWrapper.jsx') }, + ], + outputs: [ + path.join(DOCS_DIR, 'developer_portal/components/index.mdx'), + path.join(DOCS_DIR, 'static/data/components.json'), + path.join(DOCS_DIR, 'src/types/apache-superset-core/index.d.ts'), + ], + }, + { + name: 'database-docs', + command: 'node scripts/generate-database-docs.mjs', + inputs: [ + { + type: 'glob', + base: path.join(ROOT_DIR, 'superset/db_engine_specs'), + pattern: '**/*.py', + }, + { type: 'file', path: path.join(DOCS_DIR, 'scripts/generate-database-docs.mjs') }, + ], + outputs: [ + path.join(DOCS_DIR, 'src/data/databases.json'), + path.join(DOCS_DIR, 'docs/databases/supported'), + ], + }, + { + name: 'api-docs', + command: + 'python3 scripts/fix-openapi-spec.py && docusaurus gen-api-docs superset && node scripts/convert-api-sidebar.mjs && node scripts/generate-api-index.mjs && node scripts/generate-api-tag-pages.mjs', + inputs: [ + { type: 'file', path: path.join(DOCS_DIR, 'static/resources/openapi.json') }, + { type: 'file', path: path.join(DOCS_DIR, 'scripts/fix-openapi-spec.py') }, + { type: 'file', path: path.join(DOCS_DIR, 'scripts/convert-api-sidebar.mjs') }, + { type: 'file', path: path.join(DOCS_DIR, 'scripts/generate-api-index.mjs') }, + { type: 'file', path: path.join(DOCS_DIR, 'scripts/generate-api-tag-pages.mjs') }, + ], + outputs: [ + path.join(DOCS_DIR, 'docs/api.mdx'), + ], + }, +]; + +// --------------------------------------------------------------------------- +// Hashing utilities +// --------------------------------------------------------------------------- + +function walkDir(dir, pattern) { + const results = []; + if (!fs.existsSync(dir)) return results; + + const regex = globToRegex(pattern); + function walk(currentDir) { + for (const entry of fs.readdirSync(currentDir, { withFileTypes: true })) { + const fullPath = path.join(currentDir, entry.name); + if (entry.isDirectory()) { + if (entry.name === 'node_modules' || entry.name === '__pycache__') continue; + walk(fullPath); + } else { + const relativePath = path.relative(dir, fullPath); + if (regex.test(relativePath)) { + results.push(fullPath); + } + } Review Comment: **Suggestion:** The glob-based hashing assumes POSIX-style `/` path separators, but `walkDir` feeds OS-native paths into the regex; on Windows this means no files match the glob patterns, so the hash never reflects real file changes and generators can be incorrectly treated as up-to-date after the first run. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Smart docs generation skips updates on Windows environments. - ⚠️ Superset components and database docs can become stale locally. - ⚠️ Windows contributors may unknowingly commit outdated generated files. ``` </details> ```suggestion } else { // Normalize to forward slashes so glob patterns work on Windows as well const relativePath = path .relative(dir, fullPath) .split(path.sep) .join('/'); if (regex.test(relativePath)) { results.push(fullPath); } } ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. On a Windows machine (where `path.sep` is `\`), run `cd docs && yarn build` or `yarn start`, which invokes `yarn generate:smart` (script in `docs/package.json:9,13`) and executes `docs/scripts/generate-if-changed.mjs` before Docusaurus. 2. On this first run, each generator in `GENERATORS` at `docs/scripts/generate-if-changed.mjs:54-109` executes because their outputs (e.g., `developer_portal/components/index.mdx`, `static/data/components.json`, `src/data/databases.json`) do not yet exist, and their input hashes are computed in `computeInputHash()` at `155-168` using `walkDir()` at `115-136`. 3. Modify any generator input file that should be matched by a glob, such as a story file under `superset-frontend/packages/superset-ui-core/src/components` (pattern `**/*.stories.tsx` at `60-62`) or an engine spec under `superset/db_engine_specs` (pattern `**/*.py` at `82-85`). 4. Run `cd docs && yarn build` or `yarn start` again; on Windows, `walkDir()` at `115-136` builds `relativePath` using `path.relative(dir, fullPath)` (line `127`), which contains backslashes, while `globToRegex()` at `138-145` converts patterns like `**/*.stories.tsx` into regexes expecting `/` separators, so `regex.test(relativePath)` at `128-129` never matches and `files` stays empty; `computeInputHash()` therefore returns the same hash as before, causing `main()` at `222-224` and `266-267` to treat generators as "no changes, skipping," leaving the generated docs and metadata stale despite edited inputs. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** docs/scripts/generate-if-changed.mjs **Line:** 127:131 **Comment:** *Logic Error: The glob-based hashing assumes POSIX-style `/` path separators, but `walkDir` feeds OS-native paths into the regex; on Windows this means no files match the glob patterns, so the hash never reflects real file changes and generators can be incorrectly treated as up-to-date after the first run. 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. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38253&comment_hash=612590a3874f7205820b9d50c0490f04db732df578f09eaf1ec8227c046ee809&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38253&comment_hash=612590a3874f7205820b9d50c0490f04db732df578f09eaf1ec8227c046ee809&reaction=dislike'>👎</a> ########## docs/scripts/generate-superset-components.mjs: ########## @@ -1332,6 +1335,123 @@ ${sections} `; } +/** + * Build metadata for a component (for JSON output) + */ +function buildComponentMetadata(component, storyContent) { + const { componentName, description, category, sourceConfig, resolvedImportPath, extensionCompatible } = component; + const { args, controls, gallery, liveExample } = extractArgsAndControls(storyContent, componentName); + const labels = CATEGORY_LABELS[category] || { + title: category.charAt(0).toUpperCase() + category.slice(1).replace(/-/g, ' '), + }; + + return { + name: componentName, + category, + categoryLabel: labels.title || category, + description: description || '', + importPath: resolvedImportPath || sourceConfig.importPrefix, + package: sourceConfig.docImportPrefix, + extensionCompatible: Boolean(extensionCompatible), + propsCount: Object.keys(args).length, + controlsCount: controls.length, + hasGallery: Boolean(gallery && gallery.sizes && gallery.styles), + hasLiveExample: Boolean(liveExample), + docPath: `developer_portal/components/${category}/${componentName.toLowerCase()}`, + storyFile: component.relativePath, + }; +} + +/** + * Extract type and component export declarations from a component source file. + * Used to generate .d.ts type declarations for extension-compatible components. + */ +function extractComponentTypes(componentPath) { + if (!fs.existsSync(componentPath)) return null; + const content = fs.readFileSync(componentPath, 'utf-8'); + + const types = []; + for (const match of content.matchAll(/export\s+type\s+(\w+)\s*=\s*([^;]+);/g)) { + types.push({ name: match[1], definition: match[2].trim() }); + } + + const components = []; + for (const match of content.matchAll(/export\s+const\s+(\w+)\s*[=:]/g)) { + components.push(match[1]); + } + + return { types, components }; +} + +/** + * Generate TypeScript type declarations for extension-compatible components. + * Produces a .d.ts file that downstream consumers can reference. + */ +function generateExtensionTypeDeclarations(extensionComponents) { + const imports = new Set(); + const typeDeclarations = []; + const componentDeclarations = []; + + for (const comp of extensionComponents) { + const componentDir = path.dirname(comp.filePath); + const componentFile = path.join(componentDir, 'index.tsx'); + const extracted = extractComponentTypes(componentFile); + if (!extracted) continue; + + for (const type of extracted.types) { + if (type.definition.includes('AntdAlertProps') || type.definition.includes('AlertProps')) { + imports.add("import type { AlertProps as AntdAlertProps } from 'antd/es/alert';"); + } + if (type.definition.includes('PropsWithChildren') || type.definition.includes('FC')) { + imports.add("import type { PropsWithChildren, FC } from 'react';"); + } + typeDeclarations.push(`export type ${type.name} = ${type.definition};`); + } + + for (const name of extracted.components) { + const propsType = `${name}Props`; + const hasPropsType = extracted.types.some(t => t.name === propsType); + componentDeclarations.push( + hasPropsType + ? `export const ${name}: FC<${propsType}>;` + : `export const ${name}: FC<Record<string, unknown>>;` + ); + } + } + + return `/** + * 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 Review Comment: **Suggestion:** The generated `index.d.ts` for extension-compatible components always declares component exports as `FC<...>`, but the code only adds a React import when a type definition string contains `PropsWithChildren` or `FC`; if a component has no such type definitions, the declarations will still reference `FC` without importing it, causing downstream TypeScript consumers of `@apache-superset/core/ui` to see an unresolved `FC` type error. [type error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Generated @apache-superset/core/ui .d.ts references undefined FC. - ⚠️ Docs TypeScript tooling reports unresolved identifier FC errors. - ⚠️ Extension projects reusing index.d.ts see TS compile failures. ``` </details> ```suggestion // Ensure FC is imported whenever component declarations reference it, // even if no type definition string itself mentions FC/PropsWithChildren. if (componentDeclarations.length > 0) { const hasReactImport = Array.from(imports).some(line => line.includes("from 'react'")); if (!hasReactImport) { imports.add("import type { FC } from 'react';"); } } ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the generator that produces extension component types by executing `node docs/scripts/generate-superset-components.mjs` (or `yarn generate:superset-components`), which calls `generateExtensionTypeDeclarations()` from `docs/scripts/generate-superset-components.mjs:1390-1452` inside `main()` at `docs/scripts/generate-superset-components.mjs:1458-1580` and writes the result to `TYPES_OUTPUT_PATH` (`docs/scripts/generate-superset-components.mjs:63-64, 1559-1561` → `docs/src/types/apache-superset-core/index.d.ts`). 2. Observe that at least one extension-compatible component exists: the story `superset-frontend/packages/superset-core/src/ui/components/Alert/Alert.stories.tsx:19-22,32-35` imports `{ Alert, type AlertProps }` from `'.'` and sets `title: 'Extension Components/Alert'`, so `parseStoryFile()` in `docs/scripts/generate-superset-components.mjs:236-352` marks it as category `extension` and `extensionCompatible: true`, causing `components.filter(c => c.extensionCompatible)` in `main()` (around `docs/scripts/generate-superset-components.mjs:1553-1555`) to pass at least one item into `generateExtensionTypeDeclarations()`. 3. For such a component, `extractComponentTypes()` at `docs/scripts/generate-superset-components.mjs:1369-1384` reads its `index.tsx`, collects `export type ...` aliases and `export const ...` component names, and `generateExtensionTypeDeclarations()` then unconditionally emits declarations of the form ``export const Alert: FC<AlertProps>;`` or ``export const Alert: FC<Record<string, unknown>>;`` in the `componentDeclarations` array (`docs/scripts/generate-superset-components.mjs:1411-1418`), while only adding a React import when a *type definition string* includes `'PropsWithChildren'` or `'FC'` (`docs/scripts/generate-superset-components.mjs:1401-1407`); if none of the collected `export type` definitions mention those substrings (e.g., `AlertProps` is an alias of Ant Design `AlertProps` and only triggers the Antd import branch), the returned template (`docs/scripts/generate-superset-components.mjs:1422-1451`) will reference `FC` in `componentDeclarations` without any `import type { FC } from 'react';` line in the `imports` section. 4. In the docs project, TypeScript module resolution for `@apache-superset/core/ui` is configured in `docs/tsconfig.json:15-21` to point at `./src/types/apache-superset-core`, so any TS/TSX file under `docs/src` that imports from `@apache-superset/core/ui` will consume the generated `index.d.ts`; running a type check (e.g., `cd docs && yarn tsc --noEmit` using `docs/tsconfig.json:1-34`) or using an editor that respects this tsconfig will surface a `Cannot find name 'FC'` error originating from `src/types/apache-superset-core/index.d.ts`, because `FC` is used in the component declarations but never imported in that file. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** docs/scripts/generate-superset-components.mjs **Line:** 1426:1426 **Comment:** *Type Error: The generated `index.d.ts` for extension-compatible components always declares component exports as `FC<...>`, but the code only adds a React import when a type definition string contains `PropsWithChildren` or `FC`; if a component has no such type definitions, the declarations will still reference `FC` without importing it, causing downstream TypeScript consumers of `@apache-superset/core/ui` to see an unresolved `FC` type error. 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. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38253&comment_hash=878f5a125d3c9b99e16bc010282555ba31729bf870b29c309e6120c240779ab4&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38253&comment_hash=878f5a125d3c9b99e16bc010282555ba31729bf870b29c309e6120c240779ab4&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]
