EnxDev commented on PR #41685: URL: https://github.com/apache/superset/pull/41685#issuecomment-4901174020
## EnxDev's Review Agent — apache/superset#41685 · HEAD f3ea4c7 lgtm — both prior findings resolved; fix verified and now properly guarded. _Supersedes my [prior review](https://github.com/apache/superset/pull/41685#issuecomment-4874643934) on HEAD `5b3493f`._ Re-checked the two new commits against what I flagged: **Should-fix (regression guard placement) — resolved.** The guard now lives in `sqlLab.test.ts:795` (`popSavedQuery` block): the mock API response carries a non-empty `description` (`:752`) and `expectedParams` asserts it flows into the `addQueryEditor` payload (`:802`, via `objectContaining`). This is red without the fix — drop `description` from `tmpAdaptedProps` and the dispatched `queryEditor` lacks the key, so the assertion fails. Placed exactly on the path the fix changed. **Nit (brittle field lookup) — resolved, and then some.** `SaveQuery/index.tsx` now wires `htmlFor`/`id` on both fields, so the label is programmatically associated with the textarea. The test switched to `screen.getByRole('textbox', { name: 'Description' })` — order-independent, and the a11y association is a genuine improvement beyond the test fix. No new issues in the incremental diff (two test files + additive `id`/`htmlFor` on the modal inputs — no behavior change to the save flow). CI green, including the new `popSavedQuery` guard and `codecov/patch`. <!-- enxdev-review-agent:f3ea4c7 --> _Reviewed by EnxDev's Review Agent — @EnxDev · HEAD f3ea4c7._ -- 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]
