codeant-ai-for-open-source[bot] commented on PR #37007: URL: https://github.com/apache/superset/pull/37007#issuecomment-3727632193
## 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/37007/files#diff-ccabdc663027824fa3745601b4ed5e3fbdf9935833572553d7e7702470c08980R64-R72'><strong>Stale pendingValueRef</strong></a><br>The ref `pendingValueRef` is initialized from the incoming `value` prop but never updated when `value` changes. If the parent updates `value` (e.g., reset from Redux), the ref will be stale and a subsequent blur may dispatch an outdated value. Confirm whether `pendingValueRef` should be synced with prop changes.<br> - [ ] <a href='https://github.com/apache/superset/pull/37007/files#diff-ccabdc663027824fa3745601b4ed5e3fbdf9935833572553d7e7702470c08980R82-R85'><strong>Controlled vs local edit display</strong></a><br>The input is still rendered as a controlled input with `value={value}` while onChange only mutates a ref. Because the external `value` only updates on blur, user edits won't be reflected in the input (or will conflict with control behavior). Decide whether to make the input uncontrolled (use defaultValue) or maintain local component state to reflect edits while deferring propagation to onBlur.<br> - [ ] <a href='https://github.com/apache/superset/pull/37007/files#diff-fae6123e350416eacca50005aa7f0371de5126a2800dda08f5cf8e22d464465fR58-R83'><strong>Inconsistent async usage across tests</strong></a><br>Some tests are declared async but do not await userEvent calls (and some are not async yet use async helpers). Standardize on using userEvent.setup() and awaiting interactions, or consistently await userEvent.* calls. Also consider using waitFor when asserting side-effectful outcomes.<br> - [ ] <a href='https://github.com/apache/superset/pull/37007/files#diff-fae6123e350416eacca50005aa7f0371de5126a2800dda08f5cf8e22d464465fR34-R44'><strong>Async test flakiness</strong></a><br>Multiple tests call userEvent.type and userEvent.tab without awaiting their async promises and then assert synchronously on mocks. This can lead to flaky tests where assertions run before events complete. Use the async userEvent API or await calls and/or wrap assertions in waitFor.<br> - [ ] <a href='https://github.com/apache/superset/pull/37007/files#diff-fae6123e350416eacca50005aa7f0371de5126a2800dda08f5cf8e22d464465fR46-R56'><strong>Weak / ambiguous assertions</strong></a><br>The test for typing a value exceeding max only asserts that onChange was called, not what value was dispatched. That makes the intent unclear and allows regressions in clamping/validation behavior. Prefer asserting the exact expected value or properties (e.g., called once and the value is numeric and <= max).<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]
