codeant-ai-for-open-source[bot] commented on PR #37059: URL: https://github.com/apache/superset/pull/37059#issuecomment-3739156598
## 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/37059/files#diff-8a676fb002920d1d142fa9b19ded1ecb7b8b8d682cf9b9fe80f01b1a2314c86dR570-R587'><strong>Cached data mutation</strong></a><br>The code mutates theme dictionaries obtained from the cached bootstrap payload (via `payload.get("common")`). Because `cached_common_bootstrap_data` is memoized, modifying those dicts in-place can persist changes across requests and users, causing cross-request contamination. Confirm that mutations are performed on a copy instead of the cached object.<br> - [ ] <a href='https://github.com/apache/superset/pull/37059/files#diff-8a676fb002920d1d142fa9b19ded1ecb7b8b8d682cf9b9fe80f01b1a2314c86dR578-R592'><strong>Token not persisted</strong></a><br>The code reads `theme_config.get("token", {})` and mutates the returned dict, but if the `token` key did not previously exist this mutating dict is not written back into `theme_config`. As a result the fallback `brandAppName` may not be persisted into the theme structure sent to templates/bootstrap payload.<br> - [ ] <a href='https://github.com/apache/superset/pull/37059/files#diff-c99ae4b2b09b756ab2189a99a9685229f9d12633fc2616c368ea869770f603bfR780-R782'><strong>Backward compatibility</strong></a><br>THEME_DEFAULT["token"]["brandAppName"] is set at module import time to the current value of `APP_NAME`. User overrides from a later-imported `superset_config` (applied at the end of this file) will update `APP_NAME` but will not update THEME_DEFAULT, so the theme token may not reflect user-specified `APP_NAME`. Verify that the intended 3-tier fallback and backward compatibility work when `APP_NAME` is overridden in a local config.<br> - [ ] <a href='https://github.com/apache/superset/pull/37059/files#diff-449931f1bf449596ce5e3ab49828ee91c5fc3532cf191e4dbd44fff4ba779c61R121-R123'><strong>Breaking Type Change</strong></a><br>Making `brandAppName` a required property on `SupersetSpecificTokens` means the combined `SupersetTheme` type now requires this field. Existing theme objects or serialized theme payloads that don't provide `brandAppName` will fail type-checking or cause runtime assumptions elsewhere. Verify that all theme producers (bootstrap payload, default themes, theme editor exports) populate this token or that the codebase provides a typed/ runtime fallback.<br> - [ ] <a href='https://github.com/apache/superset/pull/37059/files#diff-449931f1bf449596ce5e3ab49828ee91c5fc3532cf191e4dbd44fff4ba779c61R112-R130'><strong>Backward compatibility risk</strong></a><br>The new token must be consistently applied across runtime code paths. If some code expects `brandAppName` to exist (because it's required by the type) but theme merge/serialization doesn't include it, the UI title fallback logic may be incorrect. Ensure either the token is optional in the type OR every theme-default/bootstrap path injects the resolved value (brandAppName → APP_NAME → "Superset").<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]
