codeant-ai-for-open-source[bot] commented on PR #36927: URL: https://github.com/apache/superset/pull/36927#issuecomment-3715568628
## 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/36927/files#diff-77354112fba06049e1dcd4e5386eee2ae66fd18ee58e771ead1dd5ffc69a5a69R203-R216'><strong>Changed status-condition semantics</strong></a><br>The new condition makes `shouldRecalculate` true whenever `prevChartStatus !== 'success'`, independent of whether any meaningful chart/filter state changed. This is different from the previous control flow (where recalculation only happened when `prevChartStatus !== 'success'` AND one of the comparisons changed). Verify this is intentional — it can cause extra work or incorrect indicator updates.<br> - [ ] <a href='https://github.com/apache/superset/pull/36927/files#diff-0d3f1ec9aeea3a44207273ccf038e3d714778532407b311a168a26786caafa49R207-R221'><strong>Redundant dispatch risk</strong></a><br>The new auto-apply logic dispatches updateDataMask when `needsAutoApply` is true. If `appliedDataMask` and the incoming `dataMask` are deeply equal this dispatch is redundant and may trigger unnecessary re-renders, url updates, or network activity. Confirm a deep-equality check is performed before dispatching to avoid extra work or update loops.<br> - [ ] <a href='https://github.com/apache/superset/pull/36927/files#diff-77354112fba06049e1dcd4e5386eee2ae66fd18ee58e771ead1dd5ffc69a5a69R203-R213'><strong>Reference vs deep equality</strong></a><br>The new effect uses reference equality (e.g., `dataMask !== prevDataMask`, `nativeFilters !== prevNativeFilters`, `chartLayoutItems !== prevChartLayoutItems`) to decide when to recalculate indicators. Complex objects are often recreated even when semantically equal which can cause unnecessary recalculations, or conversely, deep changes masked by stable references could be missed. Consider using a deep-equality check for these objects or a more targeted change detector.<br> - [ ] <a href='https://github.com/apache/superset/pull/36927/files#diff-ba4a3f5d4f2a2c527dc695d66412685f793d6dc9637198cc496cdb484f70774aR355-R410'><strong>Missing assertion</strong></a><br>The new test "auto-applies filter when extraFormData is empty in applied state" creates a spy on `updateDataMask` but never asserts it was called. The test advances timers and only checks UI presence, so it can pass without actually verifying the intended auto-apply behavior. Add explicit assertions to ensure the auto-apply side effect happened.<br> - [ ] <a href='https://github.com/apache/superset/pull/36927/files#diff-ba4a3f5d4f2a2c527dc695d66412685f793d6dc9637198cc496cdb484f70774aR357-R410'><strong>Potential side effects from spying original action</strong></a><br>The test uses `jest.spyOn(dataMaskActions, 'updateDataMask')` without mocking the implementation. If `updateDataMask` triggers real dispatches or other side effects, the test may become flaky or impact global state. Consider mocking the implementation to a no-op action object or safe stub to isolate the unit 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]
