codeant-ai-for-open-source[bot] commented on PR #35810: URL: https://github.com/apache/superset/pull/35810#issuecomment-3605516652
## 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/35810/files#diff-4072161ccde25adebf125fe61e17e86dc27c80ba3ba455c7fd4eada9a1ee9feaR912-R1007'><strong>Async Error Handling</strong></a><br>`fetchUsageData` now throws on `AbortError`, but at least one call site (`componentDidMount`) does not observe or handle the returned promise. If a usage request is aborted during unmount, this can result in an unhandled promise rejection. The reviewer should validate that either all callers handle rejected promises (including aborts) or that `fetchUsageData` treats aborts as a non-error condition.<br> - [ ] <a href='https://github.com/apache/superset/pull/35810/files#diff-4072161ccde25adebf125fe61e17e86dc27c80ba3ba455c7fd4eada9a1ee9feaR768-R799'><strong>API Contract Change</strong></a><br>`onQueryFormat` now calls `this.props.formatQuery(datasource.sql, { signal })` and `mapDispatchToProps` forwards this to the `formatQuery` action creator. The reviewer should confirm that the action/middleware signature supports the new `options` argument (and propagates the `AbortSignal`) while still returning a response with `response.json.result`, to avoid runtime errors or silently ignored abort signals.<br> - [ ] <a href='https://github.com/apache/superset/pull/35810/files#diff-269fa2cc1bf85b4a03ec172be71d110d8a6ff85c0e306f92d7d9636b096fb19aR183-R186'><strong>Possible Bug</strong></a><br>The global `afterEach` now unconditionally calls `jest.runOnlyPendingTimers()` before `jest.useRealTimers()`. If a test never switched to fake timers (the default in this suite appears to be real timers), Jest will throw a "Timers are not mocked" error, potentially breaking the whole test file. This should be guarded or removed, or tests should consistently enable fake timers first.<br> - [ ] <a href='https://github.com/apache/superset/pull/35810/files#diff-d83c6762e1f0a765b474a66662c8eed8aab198a145e15375429224853a87ed2dR462-R471'><strong>Weak Assertion</strong></a><br>The `allows simultaneous different async operations` test only asserts that `props.datasource` is defined and never actually triggers or validates concurrent async operations. This offers little confidence that per-request controllers don't interfere with each other. Consider strengthening or removing this 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]
