Skip to content

Fix flaky WebKit menu focus browser test - #409

Merged
kalvinnchau merged 1 commit into
mainfrom
larry/menu-dismiss-hold
Sep 29, 2026
Merged

kalvinnchau merged 1 commit into
mainfrom
larry/menu-dismiss-hold

Conversation

@loganj

@loganj loganj commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

🤖

Summary

  • This is a flaky test, not a product bug. The browser test for "closing a menu keeps focus where the user moved it" (added in Keep focus where the user moved it when a menu finishes closing #355) fails only some of the time, and only in WebKit on CI. Today it failed on main and on several unrelated PRs.
  • The cause is timing in the test itself. This change makes the test hold the closing menu reliably. Product code does not change.

Details

  • The test pauses the menu's short closing animation so it can move focus while the menu is still closing. It started the pause from the browser's transitionrun event.
  • In the failed CI run, the menu was fully gone about 70 ms after Escape, before the pause took effect. WebKit can deliver transitionrun after a short animation has already finished, so the pause never happened.
  • The test now starts the pause as soon as the menu is marked as closing (a MutationObserver on the data-ending-style attribute). That runs before the next frame, so the closing animation cannot finish first.
  • I could not reproduce the CI failure locally, including with injected main-thread load. The old and new versions both pass 30 of 30 local runs in Chromium and WebKit, so CI is the real check for this fix.

WebKit in CI can report transitionrun after the short menu exit
transition has already finished. The test then never holds the exit,
the menu unmounts, and the first held() poll times out. Pause the
exit animations from a MutationObserver on data-ending-style instead;
it runs before the next frame.

Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
@loganj
loganj marked this pull request as ready for review September 29, 2026 15:58
@loganj
loganj requested review from a team, comp615 and wesbillman as code owners September 29, 2026 15:58
@loganj loganj changed the title Hold a closing menu reliably in the menu focus browser test Fix flaky WebKit menu focus browser test Sep 29, 2026

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Star Lord — automated source review via Wes’s account

No actionable findings. Reviewed head f13f9f9287fa064ace415fc7d56f2064e8da6e7f; target base 14a2e7ed585b130315629ded39d1b83b05eb2a53.

  • The single-file change in tests/browser/menu-dismiss.spec.mjs:10–40 moves the hold to the closing-attribute mutation without changing product code, assertions, case count, retries, or timeouts. It retains the entrance-settled barrier, observed paused state, explicit release, and finally cleanup.
  • Traced the sidebar → shared Menu → final-focus path and Base UI 1.8.0’s close lifecycle. The mutation observer pauses the exit before the next frame; Base UI’s completion path subsequently waits on the animation finished promises. The existing test still checks composer focus, ordinary collapsed-content focus, and normal focus return to the row after dismissal.
  • Failure/retry scope: assertion failures retain release cleanup. This is dismissal synchronization, not an asynchronous product mutation: it neither changes nor establishes error/retry focus behavior in save/delete dialogs.
  • Public-material check: inspected the public PR description, changed test, and sole commit message. No actionable privacy finding in those surfaces; the description contains no attached images. Preserved legitimate attribution/DCO metadata.

Read-only CI evidence: run 36593011196 succeeded for this PR revision. Browser artifacts record clean merge commit 5427437a473acafc83ab33bf7ca3677ac2813215, with the reviewed base/head as parents—not a standalone-head run. Its only source difference from the head is the base’s two-line thread-summary CSS adjustment. The menu test passed with retry 0 in Chromium (2.508 s) and WebKit (5.735 s). Each containing functional shard (4/6) passed 72/72: wall time 254.456/424.904 s and summed test execution 476.052/814.461 s respectively. The slowest test was the mute/read retry journey (27.904/38.255 s); the slowest file was navigation-sidebar (99.428/159.911 s), not this test.

Limits: source review plus existing CI artifacts only; I ran no tests, builds, installs, or app workflows. No matched before/after cost baseline was inspected. As the description notes, the old and new helper both passed locally, so this green hosted run does not prove the intermittent failure is eliminated. No approval or merge authorization is implied.

@kalvinnchau
kalvinnchau merged commit 32f4dd3 into main Sep 29, 2026
20 checks passed
@kalvinnchau
kalvinnchau deleted the larry/menu-dismiss-hold branch September 29, 2026 17:26
johnmatthewtennant pushed a commit that referenced this pull request Sep 29, 2026
…t-update-drafts

* commit '0a4982797f38164d75e3e8f48e58fabb9dd59e66': (66 commits)
  Show saved local and relay inventory while retaining existing import controls (#286)
  feat(channels): edit channel details with confirmed saves (#369)
  test(channels): discover the hoverable width for activity corners (#416)
  Fix flaky WebKit menu focus browser test (#409)
  Test Goose connections and fix Pi test false failures (#383)
  feat: open threads with verified newest-first windows (#154)
  Add agent conversation context selection (#382)
  test: keep behavioral coverage without cosmetic matrices (#410)
  Fix reading position and composer caret on channel return (#411)
  fix(channels): prevent clipped activity rows and remove separators (#377)
  ci: publish signed macOS updater artifacts in prereleases (#387)
  feat(messages): add jump to latest controls (#374)
  Align reply summaries with message content (#408)
  Add centered thinking pills to agent avatars (#351)
  Keep focus where the user moved it when a menu finishes closing (#355)
  Browse legacy identities without a destination and review text before cloning (#285)
  Show separate identity cards and prevent duplicate imports (#225)
  Polish message and thread spacing, grouping, and typography (#364)
  Remove the Away avatar badge stroke (#395)
  fix(profiles): hide activity on human profiles (#391)
  ...

Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz>
johnmatthewtennant pushed a commit that referenced this pull request Sep 29, 2026
* origin/main: (25 commits)
  Show saved local and relay inventory while retaining existing import controls (#286)
  feat(channels): edit channel details with confirmed saves (#369)
  test(channels): discover the hoverable width for activity corners (#416)
  Fix flaky WebKit menu focus browser test (#409)
  Test Goose connections and fix Pi test false failures (#383)
  feat: open threads with verified newest-first windows (#154)
  Add agent conversation context selection (#382)
  test: keep behavioral coverage without cosmetic matrices (#410)
  Fix reading position and composer caret on channel return (#411)
  fix(channels): prevent clipped activity rows and remove separators (#377)
  ci: publish signed macOS updater artifacts in prereleases (#387)
  feat(messages): add jump to latest controls (#374)
  Align reply summaries with message content (#408)
  Add centered thinking pills to agent avatars (#351)
  Keep focus where the user moved it when a menu finishes closing (#355)
  Browse legacy identities without a destination and review text before cloning (#285)
  Show separate identity cards and prevent duplicate imports (#225)
  Polish message and thread spacing, grouping, and typography (#364)
  Remove the Away avatar badge stroke (#395)
  fix(profiles): hide activity on human profiles (#391)
  ...

Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz>

# Conflicts:
#	src/bundled/agents/AgentsPage.test.tsx
#	src/bundled/agents/AgentsPage.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants