Repository navigation
fix(guide): render a probe's open before its settle wait starts - #24249
Conversation
A guide's ui.find probe opens a registered popover, menu or local tab through that primitive's own state, waits settleMs on a timer, searches what the open revealed, then restores it. The open was a plain state update from outside React, so it rendered on React's next scheduler task. On a busy machine the settle timer can come due first. The probe then saw the container still closed and concluded "none", and its restore read the not-yet-updated open ref and did nothing, so the open landed afterwards and stayed on the person's page. This is the one-off CI failure of "opens a popover without its autofocus taking focus from the person" (expected 'none' to be 'found'). Run each open and each restore inside flushSync, so the container has rendered before the settle wait starts. A new test per primitive ends the settle wait before React's task can run; all three fail on main with the CI signature and pass with the fix. Closes kirodotdev#24248
github-actions
Bot
added
fork
UX Review (Opus 5.5, fork) — ✅ PASSUX-level review of UX-Verdict: PASS Nothing to check. This is a render-timing fix inside the guide's probe and its test; no control, string or layout changes, so it needs no screenshot. [UX-REVIEWED] 3d62e6e Description read[DESCRIPTION-READ] c09f63c9a3c862174757ca904a4891cc4c4f4c66b1270d31f02a5c4e249bbe31 That sha256 names everything this verdict read: the PR description, plus the 3 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with |
Design Review (Opus 5.5, fork) — ✅ PASSDesign-level review of Design-Verdict: PASS Nothing blocks: the fix addresses the root cause (the open and the restore now render before the settle wait starts), and its only caller runs from a timer, never from a render or an effect, so [DESIGN-REVIEWED] 3d62e6e Description read[DESCRIPTION-READ] d5ede72900695dcc45cd99fe42f609c32919a71ffc71d2b741efe8ffc11ce3d9 That sha256 names everything this verdict read: the PR description, plus the 1 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with |
First Principles Review (Opus 5.5, fork) — ✅ PASSPremise-level review of First-Principles-Verdict: PASS Nothing blocks. This is a fix to the mechanism that causes the defect: the open and the restore now render before the settle wait starts. The fix is in the probe's only caller of What this change shipsInventory (2 items) — 2 justifiedIntent: a guide probe finds a control inside a popover, menu or local tab and closes it again, even on a slow machine. This is a FIX (#24248, plus a new test that fails on base).
[FIRST-PRINCIPLES-REVIEWED] 3d62e6e Description read[DESCRIPTION-READ] b6e79c515bd66fedce238da0a368e7f6e0c4ee8a8a525c3c60849b0d5e8412f9 That sha256 names the PR description this verdict read. Recompute it with |
Opus 5.5 Review (fork) — ✅ no blocking findingsReviewed |
GPT 6.1 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
github-actions
Bot
added
readiness: passed
KiroCrewCaptain
left a comment
KiroCrewCaptain
left a comment
There was a problem hiding this comment.
Tier 1 auto-approve: fix (2 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, security checklist all-NO, no reviewer-lane findings, readiness signal fresh for this head SHA, declared category matches the diff. Category: the guide probe renders a container's open and restore synchronously before its settle wait (closes #24248), with tests.
github-actions
Bot
removed
the
readiness: passed
Problem / Motivation
Goal: A guide's
ui.findprobe finds a control inside a popover, menu or local tab, and puts that container back, even on a busy machine.On a loaded machine, the probe can open a registered popover, menu or local tab, decide that nothing is inside it, and leave it open on the person's page. CI caught it once as a flake in
website/src/guide/findByName.test.tsx> probeForName > "opens a popover without its autofocus taking focus from the person" (run 40192491275, Frontend Tests (1):expected 'none' to be 'found'). That run was on #23937, which has nowebsite/changes.Why it matters
When the guide points someone at a control inside a menu, a missed probe means the guide asks them to open the menu themselves, even though the guide could have found the control. A probe that leaves the menu open changes the page the person is looking at, and the probe's contract is that it never does that. The failure also shows up as a cross-PR frontend flake.
Not a goal
settleMs, or the order it tries containers in.What changed (motivation → approach → change)
nonefor a control that is inside the popover.probeOnceopens a container through the primitive's own state (setStatefrom outside React), then waitssettleMson a timer. React renders that update on its next scheduler task. When the settle timer comes due first,entry.isOpen()is still false, so the probe does not search. The restore then reads the open ref, which has not updated yet, and does nothing. The open lands afterwards and stays. The 1500 ms budget is not involved: the test has one container, and the budget is checked only before each open. The same failure reproduces with a frozennow.website/src/guide/findByName.tsruns eachentry.open()and eachrestore()insideflushSync. The container has rendered before the settle wait starts, whatever order the timer and React's task run in. This covers all three primitives, because the probe is the only caller of a probe entry'sopen.Backwards compatibility
Compatible: the probe opens and restores the same containers through the same calls, and the render now happens before the settle wait instead of during it.
probeForNameis only called from a timer callback (useFindProbe), never during a render or effect, soflushSyncis valid there.Removes nothing: no path, record, validator or key is removed.
Tests
Proof: not provable -- frontend test (prove.py runs Python only)
New test in
website/src/guide/findByName.test.tsx: "finds inside %s and puts it back when its settle wait ends before the page renders". It runs once each for a popover, a menu and a local tab. Each settle wait ends at the next microtask instead of on a timer, so it always ends before React's render task. The test checks that the probe returnsfoundand that the container is closed (or the original tab is selected again) afterwards. There are no sleeps, retries or real-clock waits.Red/green evidence (manual):
origin/mainproduct codeexpected 'none' to be 'found'(the CI signature)now: () => 0(budget can never run out; scratch, not committed)setImmediatecallback, and the runner stalls 20 ms before it returns to the event loop (scratch, not committed)findByName.test.tsx(75 tests), 6 vitest processes in parallel × 5 runs for loadLocal gates:
vitest run src/guide src/components/ui(11 files, 416 tests) passed. So didChatPage.views.test.tsxandMembersPage.owners.test.tsx, the other suites that drive the guide.eslintis clean on both changed files,tsc -p tsconfig.app.jsonis clean, andtest/test_flake_pattern_ratchet.pypassed (60 tests).Manual verification
N/A — unit coverage sufficient: the defect is the order of one timer and one React scheduler task. The new test pins that order, and a click-through cannot reliably reproduce it.
Screenshots / video
Why no screenshot: No layout or styling change; the probe opens and closes the same containers, only the render timing relative to its wait changes.
Related Issues
Closes #24248
Sibling sweep
Swept:
git grep -nE "\.open\(\)|useProbeTarget\(" -- website/srcSibling: website/src/components/ui/popover.tsx:51 -- not affected -- its probe open and restore now run inside the probe's flushSync at the single caller; covered by the "a popover" case
Sibling: website/src/components/ui/dropdown-menu.tsx:82 -- not affected -- same single caller; covered by the "a menu" case
Sibling: website/src/components/ui/tabs.tsx:148 -- not affected -- same single caller; covered by the "a local tab" case
Pattern harvest
Rule candidate: review-prompt
Pattern: "React state set from outside React (timer or async code), then a fixed setTimeout before reading the rendered DOM or a ref assigned during render: the render can land after the timer, so flush it with flushSync or wait for the commit"
Checklist
feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)Contribution License Agreement
Contributions are accepted under the project's Apache-2.0 license; see the Licensing section of CONTRIBUTING.md.