codeant-ai-for-open-source[bot] commented on PR #36486: URL: https://github.com/apache/superset/pull/36486#issuecomment-3635110197
## 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/36486/files#diff-b14ca673750ade06cbc648f5dcd0c700b97241b0265bb6a639b483e8783fa939R121-R145'><strong>Promise error handling</strong></a><br>`getTimezoneOptionsAsync()` stores a global `computePromise` but does not handle rejection. If the async computation throws/rejects, `computePromise` will remain set to a rejected promise and subsequent callers may never recover. Consider clearing or replacing `computePromise` on rejection.<br> - [ ] <a href='https://github.com/apache/superset/pull/36486/files#diff-b14ca673750ade06cbc648f5dcd0c700b97241b0265bb6a639b483e8783fa939R182-R190'><strong>setState after unmount</strong></a><br>The async flow started in `handleOpenChange` calls `getTimezoneOptionsAsync().then(setTimezoneOptions).finally(() => setIsLoadingOptions(false))`. If the component unmounts before the promise settles, the resolved callbacks will call state setters on an unmounted component and can cause React warnings or memory leaks. Add a mounted check or cleanup to avoid calling setState after unmount.<br> - [ ] <a href='https://github.com/apache/superset/pull/36486/files#diff-b14ca673750ade06cbc648f5dcd0c700b97241b0265bb6a639b483e8783fa939R69-R74'><strong>Offset key ambiguity</strong></a><br>`getOffsetKey` concats January and July offsets without a separator (e.g. `utcOffset().toString() + utcOffset().toString()`), producing keys like `60120` or `00`. This relies on matching keys in `offsetsToName`, but the concatenation may be ambiguous for certain offsets. Consider a clear delimiter to avoid accidental collisions.<br> - [ ] <a href='https://github.com/apache/superset/pull/36486/files#diff-b14ca673750ade06cbc648f5dcd0c700b97241b0265bb6a639b483e8783fa939R173-R175'><strong>Unused prop</strong></a><br>The `minWidth` prop has a default value but is never applied to the rendered <Select>. This is misleading and means the default has no effect — callers relying on that default will not get the expected behavior.<br> - [ ] <a href='https://github.com/apache/superset/pull/36486/files#diff-65e4188666ca6e3d65c21462b0f1713161d0d60ecc5bd0abb9af7eabf751c730R41-R59'><strong>Timer / microtask flakiness</strong></a><br>The test uses fake timers and calls `jest.runAllTimers()` to trigger async work that in implementation is scheduled via a combination of `queueMicrotask` and `setTimeout(0)`. Microtasks (queueMicrotask / Promise microtasks) are not always flushed by timer APIs and this can lead to intermittent flakes where the timezone options never materialize in time. Consider ensuring microtasks are flushed after advancing timers (e.g. awaiting a resolved promise or using React's act).<br> - [ ] <a href='https://github.com/apache/superset/pull/36486/files#diff-7dc3633607d66f62b7c5a2c6703ba56acfe6643cb320a582edc24f9a54f132b1R56-R66'><strong>Flaky timers / microtasks</strong></a><br>The tests rely on calling `jest.runAllTimers()` after triggering the dropdown open to flush async work. The component under test uses a mix of `queueMicrotask` and `setTimeout(0)` (per PR description). Jest's fake timer APIs may not run microtasks the same way as real runtime which can lead to intermittent failures or tests that hang. Verify the tests reliably flush both microtasks and timers (use `act`, a dedicated flush helper, or adjust the scheduling in the component/tests).<br> - [ ] <a href='https://github.com/apache/superset/pull/36486/files#diff-7dc3633607d66f62b7c5a2c6703ba56acfe6643cb320a582edc24f9a54f132b1R74-R122'><strong>Repeated test setup / duplication</strong></a><br>The new tests add repeated steps to trigger timezone data loading (click combobox, run timers, wait for loading to finish) across many tests. This increases maintenance burden and risk of inconsistent behavior between tests. Consider extracting a small helper (e.g. `triggerTimezoneLoad`) to centralize this flow and ensure consistent ordering of microtask/timer flushing.<br> - [ ] <a href='https://github.com/apache/superset/pull/36486/files#diff-65e4188666ca6e3d65c21462b0f1713161d0d60ecc5bd0abb9af7eabf751c730R60-R63'><strong>Brittle loading text assertion</strong></a><br>The test waits for "Loading..." to disappear. The component's UX mentions "Loading timezones..." and labeling could change; asserting exact text may make the test brittle. Prefer a more robust selector (regex, role, aria-label, or a spinner class) so translations/wording changes don't break the test.<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]
