Skip to content

Keep react-aria Select collections stable while a field-focus re-render is in flight - #32766

Merged
ShaileshParmar11 merged 1 commit into
mainfrom
claude/testlibrary-select-detach
Sep 7, 2026
Merged

Keep react-aria Select collections stable while a field-focus re-render is in flight#32766
ShaileshParmar11 merged 1 commit into
mainfrom
claude/testlibrary-select-detach

Conversation

@ShaileshParmar11

Copy link
Copy Markdown
Contributor

Problem

Features/DataQuality/TestLibrary.spec.ts › should create, edit, and delete a test definition intermittently times out at 60s in the merge queue and full runs, failing on both attempts when it goes red (run 34031893426, shard chromium-06):

waiting for getByRole('option', { name: 'TABLE', exact: true })
  - locator resolved to <div role="option" data-key="TABLE" ...>
  - attempting click action
  - element was detached from the DOM, retrying

Root cause

Focusing any field runs onFocusCapturehandleActiveFieldonActiveFieldChange, which is the parent's setActiveField (TestDefinitionForm.component.tsx). That re-renders TestDefinitionFormBody, and primaryFields / classificationFields were plain const arrays of fresh object literals — so every re-render handed the react-aria Select a new props object, rebuilding its collection and detaching the option that was open under the cursor.

It races the click, which is why it is intermittent rather than a hard failure.

#32631 hoisted the enum-derived option arrays (ENTITY_TYPE_OPTIONS, …) to module scope. That stabilised the options identity but not the field descriptor identity, which is what actually feeds the collection — so the detach survived.

Fix

Memoize both field arrays. This is the case .claude/rules/frontend-performance.md calls out directly: inline object/array props defeat memoization, hoist constants and wrap the rest.

Verification

  • The field definitions are byte-identical with the useMemo wrapper removed (whitespace/comment-normalised comparison) — the diff is the wrapper plus re-indentation, no behaviour change.
  • TestDefinitionFormBody.test.tsx passes; 55 tests green across the form's suites.
  • ESLint 0 errors / 0 warnings (deps are exactly what react-hooks/exhaustive-deps requires; form comes from useForm() and is a stable RHF reference).
  • tsc --noEmit clean for this file.

Caveat worth stating: the failing test is intermittent, so a single green run does not prove the race is gone — it proves the mechanism that produced the detach no longer exists on any re-render path.

🤖 Generated with Claude Code

… re-render

Focusing a field calls onActiveFieldChange -> setActiveField in the parent,
re-rendering the body and rebuilding the FieldProp arrays. The react-aria
Select then got a new props object and rebuilt its collection, detaching the
open option mid-click -- TestLibrary "should create, edit, and delete a test
definition" timed out on "element was detached from the DOM, retrying".

Hoisting the option arrays fixed the options identity but not the field
descriptor identity, which is what feeds the collection.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 6, 2026 14:22
@ShaileshParmar11 ShaileshParmar11 added skip-pr-checks Bypass PR metadata validation check safe to test Add this label to run secure Github workflows on PRs labels Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

❌ PR checklist incomplete

This PR cannot be merged until the following are addressed on its linked issue:

  • No GitHub issue is linked. Link an issue in the Development section of the PR (or add Fixes #12345 to the description). For a same-org cross-repo issue, add Fixes open-metadata/<repo>#123 to the description.

The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically.

Maintainers can bypass this check by adding the skip-pr-checks label.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added the UI UI specific issues label Sep 6, 2026
@gitar-bot

gitar-bot Bot commented Sep 6, 2026

Copy link
Copy Markdown
Code Review ✅ Approved

Memoizes test-definition field arrays to keep react-aria Select collections stable during field-focus re-renders. This prevents options from detaching mid-click and fixes the intermittent timeout in the TestLibrary test. No issues found.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 69%
69.96% (92771/132598) 54.73% (57353/104780) 55.92% (17432/31169)

@sonarqubecloud

sonarqubecloud Bot commented Sep 6, 2026

Copy link
Copy Markdown

@ShaileshParmar11 ShaileshParmar11 removed the skip-pr-checks Bypass PR metadata validation check label Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

✅ Playwright Results — workflow succeeded

Validated commit 196f7be2fcba012ee78cb97335c53170326e085b in Playwright run 34038984275, attempt 1.

✅ 557 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky

Performance

Blocking targets: ✅ met · Optimization targets: 🟡 in progress

Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting.

🕒 Full workflow signal wall (to summary) 34m 29s

⏱️ Max setup 5m 17s · max shard execution 18m 33s · max shard-job elapsed before upload 21m 39s · reporting 4s

🌐 234.57 requests/attempt · 2.82 app boots/UI scenario · 34.05% common-shard skew

Optimization targets still in progress:

  • Common shard skew was 34.05% (convergence target: at most 15%).
  • Browser traffic was 234.57 requests per attempt (convergence target: fewer than 200).
  • Application boot ratio was 2.82 per UI scenario (1629 boots / 578 scenarios; convergence target: at most 1).
Shard Passed Failed Flaky Skipped Lifecycle failed Lifecycle flaky
✅ Shard chromium-01 142 0 0 0 0 0
✅ Shard chromium-02 133 0 0 0 0 0
✅ Shard chromium-03 124 0 0 0 0 0
✅ Shard data-asset-rules-01 65 0 0 0 0 0
✅ Shard domain-isolation-01 16 0 0 0 0 0
✅ Shard global-state-01 34 0 0 0 0 0
✅ Shard ingestion-01 1 0 0 0 0 0
✅ Shard reindex-01 2 0 0 0 0 0
✅ Shard search-01 11 0 0 0 0 0
✅ Shard search-rbac-01 29 0 0 0 0 0

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@ShaileshParmar11
ShaileshParmar11 added this pull request to the merge queue Sep 7, 2026
Merged via the queue into main with commit 69e6b75 Sep 7, 2026
173 of 178 checks passed
@ShaileshParmar11
ShaileshParmar11 deleted the claude/testlibrary-select-detach branch September 7, 2026 20:07
ShaileshParmar11 added a commit that referenced this pull request Sep 8, 2026
react-aria builds a Select's collection from `items` AND `children`. The
children function was declared inline, so every render of a form gave the
ListBox a fresh identity and rebuilt the collection -- detaching whichever
option was under the cursor.

This is the third and, as far as the chain goes, last unstable identity on
that path. #32631 hoisted the option arrays and #32766 memoized the field
descriptors, which stabilised `items`; both reduced the failure rate of
TestLibrary "should create, edit, and delete a test definition" without
ending it, and the CI log still shows "element is not stable" followed by
"element was detached from the DOM".

The function closes over nothing but its argument, so module scope suffices.
Being in the shared component, this covers every FieldTypes.SELECT.

Not reproducible locally (passes 3/3), so this is not verified red -> green;
it is a mechanism-level fix, and the inline-function-prop hazard it removes is
one .claude/rules/frontend-performance.md calls out independently.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants