codeant-ai-for-open-source[bot] commented on PR #34565: URL: https://github.com/apache/superset/pull/34565#issuecomment-3658561589
## 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/34565/files#diff-8a676fb002920d1d142fa9b19ded1ecb7b8b8d682cf9b9fe80f01b1a2314c86dR512-R516'><strong>Cache / user mismatch</strong></a><br>The memoized function is keyed by `user_id` (argument) but the body uses `g.user` when building `menu_data`. This can return the wrong menu for a cached entry keyed to a different user id (or an anonymous user) and lead to incorrect UI being served from cache.<br> - [ ] <a href='https://github.com/apache/superset/pull/34565/files#diff-b69349908c886a7fb0924d9c8534d9ffa81a37c09db77a25413916f661d6eaa8R116-R135'><strong>Cleanup helper robustness</strong></a><br>`_cleanup_test_databases` deletes databases in a loop and commits at the end, but a failure while deleting one database may leave others undeleted or the session in an inconsistent state. The helper also retries delete after a rollback but swallows exceptions. Consider logging errors and making deletion atomic or robust to partial failures.<br> - [ ] <a href='https://github.com/apache/superset/pull/34565/files#diff-8a676fb002920d1d142fa9b19ded1ecb7b8b8d682cf9b9fe80f01b1a2314c86dR475-R475'><strong>Locale parsing edge-case</strong></a><br>The new language extraction only splits on underscore (`_`). Locale strings using hyphens (e.g. `de-DE`) or other formats will not be normalized correctly and may lead to incorrect `locale` values in bootstrap data or missed cache separation.<br> - [ ] <a href='https://github.com/apache/superset/pull/34565/files#diff-b69349908c886a7fb0924d9c8534d9ffa81a37c09db77a25413916f661d6eaa8R137-R141'><strong>Broad exception swallowing</strong></a><br>Several added try/except blocks catch `Exception` then silently `pass`, hiding underlying errors (rollback or cleanup failures). Silent swallowing can mask real problems in test setup/teardown and make debugging harder. At minimum, log the exception details; consider re-raising or failing the test when appropriate.<br> - [ ] <a href='https://github.com/apache/superset/pull/34565/files#diff-8a676fb002920d1d142fa9b19ded1ecb7b8b8d682cf9b9fe80f01b1a2314c86dR525-R528'><strong>get_locale() stringification assumptions</strong></a><br>The code converts `get_locale()` to a string (`str(locale)`) to form the cache key. If `str(locale)` ever returns an unexpected representation (different from values used elsewhere), cache fragmentation or collisions could occur. Consider normalizing the locale format consistently for both caching and payload.<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]
