Catalog: give the query selector an accessible name

[nl0]

Description

Two defects in the shared query selector used by the Athena and ElasticSearch
query screens.

It has no accessible name. Its InputLabel sits beside the Select rather
than naming it. In MUI v4 the focusable node is the role="button" display div,
which takes its name from labelId — so a keyboard or screen-reader user hears
an unnamed button where a sighted user reads "Select query".

It claims a query is loaded when the load failed. renderValue fell back to
"Custom" — the label for a hand-written query — whenever value was null.
Callers null the value on a failed load, so the field asserted "Custom" directly
beside its own helper text saying the query list could not be loaded. It now
renders blank on the error path and keeps "Custom" for the case it was written
for.

Review findings addressed

None folded in; the review raised nothing against this unit's code. Recorded:

• f42 — the compensation for a caller's lossy null encoding sits inside the
  shared QuerySelect rather than at the caller. Left as is deliberately: the
  trigger does not exist yet (ElasticSearch.tsx passes no error prop), so
  there is no second call site to disagree with.
• f18 — worth naming across two PRs in this stack rather than in neither.
  The same MUI v4 accessible-name problem is fixed twice, in two different
  ways: labelId here, SelectDisplayProps in the Search sidebar (PR 3).
  Both are correct for their call site; the inconsistency is the finding. A third
  case is still unfixed at Queries/Athena/Workgroups.tsx:64-66. Unifying this
  in the shared Select wrapper is a follow-up, not something either PR should
  reach across into.

Verification

cd catalog && npx vitest run app/containers/Queries/QuerySelect.spec.tsx

8 tests, including the accessible name and the blank-under-error branch.

Position in the stack

PR 2 of 9, based on
stack/5217-1-data-products-lazy-adapter.

This is part of the split of #5217 asked for in
f27. PRs 2
and 3 are the two smallest, uncontested accessibility fixes in that set and share
no symbol with anything above them — they are early in the stack so a release
gets them even if the contested units above stall.

TODO

• Unit tests
• Security: Confirm that this change meets security best practices and does not violate the security model
• Open: Confirm that this change doesn't break the Open variant
• Changelog entry

🤖 Generated with Claude Code

Greptile Summary

This PR gives the shared query selector an accessible name and changes its null display during errors.

• Connects InputLabel to the MUI Select display node through labelId.
• Blanks the selector for an error with no selected query, while retaining “Custom” for ordinary null selections.
• Adds focused accessibility and error-display tests plus updated snapshots and changelog text.

Confidence Score: 4/5

The ElasticSearch custom-query regression should be fixed before merging because validation errors now blank a selector whose active mode is still “Custom.”

The accessible-name change is sound, but the shared display condition conflates Athena’s query-list load failure with ElasticSearch’s query-body validation error and hides a reachable active selection.

Files Needing Attention: catalog/app/containers/Queries/QuerySelect.tsx and catalog/app/containers/Queries/QuerySelect.spec.tsx

Important Files Changed

Filename	Overview
catalog/app/containers/Queries/QuerySelect.tsx	Correctly adds accessible labeling, but the broad error-based blanking also hides ElasticSearch’s active custom-query state during validation errors.
catalog/app/containers/Queries/QuerySelect.spec.tsx	Adds useful accessible-name and display-state coverage, but does not exercise the existing ElasticSearch meaning of the error prop.
catalog/app/containers/Queries/snapshots/QuerySelect.spec.tsx.snap	Records the expected label ID and aria-labelledby relationship.
catalog/CHANGELOG.md	Documents the intended accessibility and failed-load display fixes.

_{Reviews (1): Last reviewed commit: "docs(changelog): entry for the query sel..." | Re-trigger Greptile}

Greptile also left 1 inline comment on this PR.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 34.92%. Comparing base (41a0243) to head (0e69c8f).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5260      +/-   ##
==========================================
- Coverage   34.92%   34.92%   -0.01%     
==========================================
  Files         741      741              
  Lines       23802    23804       +2     
  Branches     6429     6429              
==========================================
  Hits         8314     8314              
- Misses      13744    13746       +2     
  Partials     1744     1744              

Flag	Coverage Δ
catalog	34.92% <100.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nl0]

Local code review — findings and dispositions

Reviewed as this layer's diff (stack/5217-1-data-products-lazy-adapter..stack/5217-2-query-select-accessible-name), not against master. Three findings: two accepted, one declined.

1. Accepted — aria-labelledby dropped the selected query from the accessible name

This one was a regression the branch introduced. labelId on its own names the display div after the label, and because aria-labelledby wins over an element's contents, the selected query stopped reaching assistive tech altogether. MUI builds the attribute as labelId buttonId and only emits buttonId when the Select is given an id (or a name) — SelectInput derives it as SelectDisplayProps.id || (name ? 'mui-component-select-' + name : undefined).

Measured with a query selected, reading the accessible name off the role="button" node:

state	accessible name
before this branch	the query's name — label missing
labelId only	Select a query — selection missing
labelId + id	Select a query name1

So the field traded one omission for another: a screen-reader user could no longer tell which query was loaded, though the div still renders that name as text. Fixed in ae806fb by giving the Select an id, pinned by a test that fails when the id is dropped.

2. Accepted — the useId mock handed out a fresh id per call, not per component instance

utils/useId memoizes through React.useState(makeId); the spec's bare counter did not, so rendering the component twice moved the label id from test-id-2 to test-id-4. The committed snapshots held only because each snapshot test renders exactly once and QuerySelect happens to call useId twice before the id that lands in them — any later rerender, or an interaction that opens the menu, would renumber every id.

useId already takes its generator as a parameter, so the mock now delegates to the real hook and makes only the generation deterministic (bbb9319). The ids in the snapshots are unchanged.

3. Declined — "blanking on error assumes helperText is always present"

Correct that the two props are independent optionals, so an error whose message is empty would render a field blank with no explanation. But the suggested remedy — falling back to a placeholder when there is no helper — would put "Custom" back in the one place this branch exists to remove it from. A blank field under a red underline is the honest rendering of "nothing is loaded"; "Custom" asserts that a hand-written query is loaded. Left as is: if an error arrives with no message, that is the caller's contract to fix, not something the selector should paper over by claiming a query it does not have.

Also in this pass

Swept this layer's comments for narrative rather than constraint (2e05d7e): dropped "Blank under error" sitting above the branch that blanks under error, and two test comments retelling the labelId mechanism the component already states where a reader lands. What each keeps is the part the code cannot show.

The CHANGELOG entry is unchanged, deliberately. Across the whole PR the label is what newly reaches assistive tech — the selected query was announced before this branch and is announced again after the fix — so the entry as written already describes the net change.

Verification

npx vitest run app/containers/Queries (112 pass, from 111), npx tsc --noEmit, and oxfmt --check / oxlint on the touched files. Layers 3–9 were rebased onto the updated chain and force-pushed; each layer's own diff is byte-identical after the rebase, and every PR in the stack still shows only its own layer.