codeant-ai-for-open-source[bot] commented on PR #36929: URL: https://github.com/apache/superset/pull/36929#issuecomment-3720173816
## Nitpicks 🔍 <table> <tr><td>🔒 <strong>No security issues identified</strong></td></tr> <tr><td>⚡ <strong>Recommended areas for review</strong><br><br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-5738343ae0b1e3eae980a6e088fadd9ed2d0da5d2743ff8f537e6bda16f8a00aR20-R20'><strong>API breaking change</strong></a><br>The file previously exported a default language pack. That export was removed and only `logging` is re-exported now. Consumers that imported the language pack or translation utilities from this module will break unless the PR provides a compatibility re-export or all call sites were updated to the new package (`@apache-superset/core`). Verify all imports across the monorepo and extensions are updated, or add a proxy re-export to preserve backward compatibility.<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-0c14b31c4620b815688a3f05ca9ca44323c2058b1d3a12cf0ce8ca4c242e6b4dR19-R20'><strong>Mixed core packages</strong></a><br>The file now imports `t` from `@apache-superset/core` but still imports `DTTM_ALIAS`, `QueryColumn`, and `QueryMode` from `@superset-ui/core`. Mixing two core packages can produce duplicate runtime instances, inconsistent types, or diverging implementations of shared utilities. Confirm these symbols intentionally remain in `@superset-ui/core` and ensure both packages are compatible (same versions / singletons) to avoid subtle runtime or bundling issues.<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-e5b479dd3644699417563491d8257b24c20ba8bc7ec040b9dad41fee4993db9cR54-R66'><strong>ID / key safety</strong></a><br>DOM `id` and React `key` are derived directly from `val` (including `JSON.stringify(val)` and `tab-${val}`). If `val` is an object, array, or contains characters not valid in an id, this can produce invalid/duplicate ids or unpredictable keys. Use a stable stringification or index-based fallback to generate safe ids/keys.<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-e5b479dd3644699417563491d8257b24c20ba8bc7ec040b9dad41fee4993db9cR80-R88'><strong>Translation substitution</strong></a><br>The translated accessibility string uses a label value that can be a ReactNode (not necessarily a string). Passing a non-string as the translation placeholder can yield incorrect output (e.g. "[object Object]") or lose meaningful label text. Ensure the substituted value is converted/sanitized to a user-visible string before calling `t`.<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-b1dc60f07b3d4280f99b64e090b13e54339ccbae170055c935a44411870b27a4R47-R52'><strong>Reset does not clear implicit singleton</strong></a><br>If `getInstance()` is called before `configure()`, a `Translator` is created and assigned to `singleton` while `isConfigured` remains `false`. `resetTranslation()` only clears `singleton` when `isConfigured` is `true`, so the implicitly-created singleton will not be cleared. This can lead to stale state across tests or when trying to reconfigure the translator.<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-24418750126e5ffe6c0754c95b80136e8618951698ba79d78f053646ee3c77a8R19-R19'><strong>Missing runtime dependency risk</strong></a><br>Moving `t` import to `@apache-superset/core` can cause runtime module resolution failures if the new package isn't added to this package's dependencies or isn't present at runtime. Ensure `@apache-superset/core` is a declared dependency (or peerDependency) and is bundled/published where this plugin runs.<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-82f8803e13e6f5acb3c5c769a9118aa756df94dec543b4395f538c4274593a5fR20-R20'><strong>Dependency change</strong></a><br>The translation function `t` is now imported from `@apache-superset/core`. Verify that this package is added to the package.json (or the consuming package's dependencies) and that the monorepo build and bundler configuration are updated so the new package is available at runtime. Missing/incorrect dependency changes will cause runtime/module resolution failures.<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-82f8803e13e6f5acb3c5c769a9118aa756df94dec543b4395f538c4274593a5fR20-R21'><strong>i18n runtime compatibility</strong></a><br>Ensure the `t` implementation exported from `@apache-superset/core` is API-compatible with prior usage (same signature and runtime behavior). Also confirm translations are loaded/initialized correctly in environments that consume this plugin (extensions, tests, storybooks).<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-d987d4addee4ccb4cccae5bb4376bdf7915776d6e8631f9a777abfa98c08c9a0R52-R59'><strong>I18n import change</strong></a><br>The PR moves `t` and `tn` imports to `@apache-superset/core/ui`. Confirm that the new exports behave identically to the previous `@superset-ui/core` implementations (pluralization, interpolation, runtime side-effects). Also ensure there's no mixed usage elsewhere in the repo that would import translations from the old package, which could lead to inconsistent translations at runtime.<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-959ec67ff89359eb84f66b382320c396a36fc397e0edd518a388bef2f08ffeafR19-R20'><strong>Inconsistent imports</strong></a><br>The PR moves translation `t` to `@apache-superset/core` while validators (`validateInteger`, `validateNonEmpty`) remain imported from `@superset-ui/core`. This creates mixed core packages in the same module and can cause runtime/packaging inconsistencies if validators are also moved to the new core or re-exported differently. Verify the canonical source for shared utilities and align imports across the repo.<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-27f4beee3a16ed4fbc4597487901745893024e6881211196cea9d420378eca41R20-R21'><strong>Export / resolution risk</strong></a><br>Ensure `t` is actually exported from `@apache-superset/core` in the version used by the app and that moving it did not create circular dependency or runtime resolution issues. Also verify tree-shaking / bundle size implications and update any consumers that still import `t` from `@superset-ui/core`.<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-386fbc0fea678cabaddf2e8850b476ea104bb2969e6e68d274c599f2f9298d31R24-R26'><strong>Swallowed errors during image fetch</strong></a><br>The new logic catches fetch errors when checking the image, treats a 404 as "not ready", but silently swallows all other errors (no rethrow). That can cause non-404 failures to be treated as if the image was ready (or simply ignored), preventing proper retry/failure handling and logging. Ensure non-404 errors are propagated so retries/failure handling and logging behave as intended.<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-db1bdffa03dc02058c5959fb6a63c839a36246fc91e5aa7421d5f7727db1aa9fR28-R34'><strong>Module initialization side effects</strong></a><br>The code calls `getExtensionsRegistry()` at module scope (top-level) which can run during module evaluation. This may trigger side effects too early (e.g., during server-side rendering, tests, or before runtime initialization). Consider delaying registry resolution to component render or using lazy initialization to avoid ordering issues.<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-ca24d7e8037bd100e6cac18e07ad111132021be4994316b82cea11a03037eb2dR22-R22'><strong>Logging API shape</strong></a><br>The code calls `logging.error(error)`. Confirm that the `logging` export from `@apache-superset/core` exposes an `error` method with the expected signature. If the logging API changed during refactor, adapt the calls or wrap the new logger to preserve behavior.<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-ca24d7e8037bd100e6cac18e07ad111132021be4994316b82cea11a03037eb2dR21-R21'><strong>Feature flag source</strong></a><br>Ensure `FeatureFlag` and `isFeatureEnabled` are still meant to be imported from `@superset-ui/core`. If the "move translations to new core" refactor also relocated feature-flag utilities to `@apache-superset/core`, this import will break at runtime or cause inconsistent behavior. Verify the canonical source and update imports accordingly.<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-b7822e6e4889de9ed070d60e4f892d8020248f1b4ec4b7505bd136496ce39d88R51-R77'><strong>Error surface/visibility</strong></a><br>The catch block clears state but does not surface the error to the user or log it. Consider logging the caught error and/or showing a danger toast so failures to fetch metadata are visible to users and debuggable in logs.<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-b7822e6e4889de9ed070d60e4f892d8020248f1b4ec4b7505bd136496ce39d88R19-R22'><strong>Mixed core package imports</strong></a><br>This file now imports `t` and `logging` from `@apache-superset/core` while `SupersetClient` remains imported from `@superset-ui/core`. Having core utilities split across two packages can introduce duplicated runtime instances (i18n, logging singletons) or incompatible versions. Validate that these packages are intentionally split, that peerDependencies/aliases will dedupe them at build/runtime, and that no re-exports were intended so everything resolves to a single implementation.<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-6178581f7c03d7fccce4bd0231c333f46fa4453b5899b1d9c54a3b46026766daR20-R22'><strong>Mixed core packages</strong></a><br>The PR imports translation (`t`) from `@apache-superset/core` while still importing `SupersetClient` and `getClientErrorObject` from `@superset-ui/core`. This can lead to shipping two overlapping "core" packages, duplicate runtime code, and inconsistent exports across the app. Verify whether `SupersetClient` and `getClientErrorObject` should also be re-exported from `@apache-superset/core` (or vice versa), and ensure all callers are updated consistently to avoid bundle duplication or runtime mismatches.<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-c8e902d52cd894d7d5a3157499b0b1306996da2a806892e963f183482a6258feR19-R23'><strong>Import inconsistency</strong></a><br>The PR moved `t` and `logging` to `@apache-superset/core` but left `SupersetClient` imported from `@superset-ui/core`. This mixed sourcing can cause duplicate/core-version mismatches, subtle runtime differences, or larger bundles. Verify which package is the single source-of-truth for core utilities and make imports consistent.<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-c8e902d52cd894d7d5a3157499b0b1306996da2a806892e963f183482a6258feR19-R23'><strong>Packaging / bundle size risk</strong></a><br>Importing related utilities from two different packages risks pulling both packages into the client bundle (duplicate code). Confirm whether `@apache-superset/core` re-exports `SupersetClient` and, if so, consolidate imports to avoid duplication and ensure consistent runtime behavior.<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-053cd50a9b3592e948a7bb79cda6a0a1bef81cddaf12eabf93e97dda27e649dbR30-R32'><strong>Mixed import sources</strong></a><br>The PR moves `logging` and `t` to `@apache-superset/core` while keeping `SupersetClient` (and many other UI components) imported from `@superset-ui/core`. Mixing the old and new packages for related runtime utilities may cause version mismatches, duplicate instances, or context differences (e.g., different singletons for logging or translation state). Verify the packages are aligned and the same logical implementations are used across the app.<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-053cd50a9b3592e948a7bb79cda6a0a1bef81cddaf12eabf93e97dda27e649dbR32-R32'><strong>Translation import location</strong></a><br>`t` is now imported from `@apache-superset/core/ui`. Ensure the translation registry and message catalogs remain wired properly after the move so translations resolve the same way. Confirm there are no duplicate `t` implementations still referenced elsewhere.<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-acf5af0de344c4ec7c4008ef7c1331146a12b90650e4ec78133497d0b222d9f9R19-R20'><strong>Translation import path</strong></a><br>The `t` function was moved from `@superset-ui/core` to `@apache-superset/core/ui`. Verify that the new import path is correct and that `@apache-superset/core` is present in this package's dependencies (or a top-level workspace) so the module resolves at runtime. Confirm that other plugins were updated consistently to avoid mixed imports.<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-62f5efec37d5e773c89a17b5755a99502279de7b1373439a280e80ef8fd494feR19-R21'><strong>Dependency / build changes required</strong></a><br>Moving `t` to `@apache-superset/core` likely requires changes to repository-level package.json and build config (new dependency, possible storybook/test config updates). Ensure CI and consumers (extensions) add the new package and that rollouts are coordinated to avoid missing import errors.<br> - [ ] <a href='https://github.com/apache/superset/pull/36929/files#diff-acf5af0de344c4ec7c4008ef7c1331146a12b90650e4ec78133497d0b222d9f9R19-R26'><strong>Dependency duplication / compatibility</strong></a><br>Adding `@apache-superset/core` while `@superset-ui/core` remains used for ChartPlugin/ChartMetadata may create two sources of truth for shared utilities. Verify there are no conflicting implementations/version mismatches that could lead to duplicate bundles or inconsistent behavior.<br> </td></tr> </table> -- 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]
