Skip to content

test(storybook): wait for plus-menu popups to settle - #4897

Merged
Astro-Han merged 1 commit into
apache:mainfrom
orangeCatDeveloper:test/storybook-plus-menu-visibility
Sep 6, 2026
Merged

test(storybook): wait for plus-menu popups to settle#4897
Astro-Han merged 1 commit into
apache:mainfrom
orangeCatDeveloper:test/storybook-plus-menu-visibility

Conversation

@orangeCatDeveloper

Copy link
Copy Markdown
Contributor

Summary

The Storybook smoke intermittently reads the composer's + menu as hidden right after it opens, although it never closed. jest-dom's toBeVisible walks up to the Astryx layer and reads its computed opacity; the layer's entry keyframes use animation-fill-mode: backwards, so until the first frame ticks the animation that value is 0, and on a loaded CI runner the assertion lands inside that window. #4889 already retries the one read in the slash-menu story; this covers the same read in the app-shell sibling and the two findByRole('listbox') reads that follow, using the same waitFor idiom.

The app-shell harness also held its Skill refresh on setTimeout(…, 150). A slow runner outlives that before the story clicks the row, so the click activates the row for real and the menu closes. The harness now settles on an explicit release the story calls once the busy-row assertions are done, the way the slash-menu story already latches its projection.

Fixes #4896

Verification

format:check and node --test scripts/storybook-visual-smoke.test.mjs pass. typecheck:stories reports only the four rightCollapsed errors main already carries from #4694, which #4895 fixes; nothing in this diff. The stories were looped in mcr.microsoft.com/playwright:v1.62.1-noble at --cpus=2 with four concurrent pages, which is the only environment where this reproduces (never on macOS in 44 runs).

Before (origin/main d2d7efe), slash-menu story, 40 rounds, with getComputedStyle/showPopover/animation events hooked:

pass=35 fail=5
1110ms showPopover div#_r_4_ inner=menu/添加上下文
1200ms click div role=menuitem label=选择技能 busy=true
1330ms gcsHit div#_r_4_ display=block opacity=0 visibility=visible popoverOpen=true :: window.getComputedStyle < Jl < Xl < Xl < Xl < Object.Zl
1343ms animationstart x1ahk7ht-B div#_r_4_ inner=menu/添加上下文

Before, app-shell story, 40 rounds (18 of the toBeVisible failures end with "editor": "/" and the Skill listbox open, the timer having expired before the click):

pass=15 fail=25
  17 HL: expect(element).toBeVisible()
   8 HL: expect(element).toHaveAttribute("aria-busy", "true")

After, same container, same loops:

### slash x40
pass=40 fail=0
### appshell x40
pass=40 fail=0

Root cause

See #4896. Emulating prefers-reduced-motion in the smoke script would remove the animation but also switches scroll-motion-policy.ts to instant scrolling, and two app-shell scroll stories fail by design under it, so the fix stays in the stories.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code (Fable 5.1) ran the CI-log triage, built the Linux repro and instrumentation, identified the mechanism, and authored the story changes; commit carries Generated-by: Claude Code.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/S Under 100 readable lines label Sep 6, 2026

@Astro-Han Astro-Han 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.

Reviewed current head 3956ab444e879caeb68d7c4617c0d6eb58ad233f (OPEN, MERGEABLE/BLOCKED awaiting human review). Technical GO — no P0–P3. But the integration gate is not closed: current test fails on the unmodified Electron E2E budget check (session-workbar.spec.ts budget 5, actual 6), before Storybook smoke — not attributed to this PR, but not green either.

What it does

Two Storybook files only (app-shell.stories.tsx:1494-1563, composer-slash-menu.stories.tsx:423-457): popup visibility waits and controlled projection release. Real Composer loading behavior, Plan boundaries, menu hold/close, and settled-listbox paths checked (composer.tsx:1979-1989,2015-2030, desktop wiring app-shell.tsx:3019-3028). Historical CI reproduces both target stories' toBeVisible() timing failures, and the fix matches the failure points.

Scope and limits

Merge-tree/diff check clean. Desktop build/typecheck/Storybook smoke/real Electron run not performed locally (no usable deps/dist in this worktree).


Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

简体中文

评审结论来自自动化审查流程;发布者没有读这份 diff,核的是当前 head 有没有漂移、以及 exact-head 的门禁状态。当前 head 是 3956ab4,未关闭。技术上无阻断问题;但门禁没闭合(E2E 预算红,与本改动无关)。合并由人类定。

jest-dom's toBeVisible reads the Astryx layer's computed opacity, which
stays 0 until the first frame ticks its backwards-filled entry
animation. On a loaded CI runner that frame lands after the assertion
and a popup that never closed reads as hidden. Retry the visibility
reads the way the slash-menu story already does, and let the refresh
harness settle on an explicit release instead of a 150ms timer that
a slow runner outlives before the click.

Generated-by: Claude Code
@orangeCatDeveloper
orangeCatDeveloper force-pushed the test/storybook-plus-menu-visibility branch from 3956ab4 to c65ef8b Compare September 6, 2026 06:51

@Astro-Han Astro-Han 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.

Re-reviewed at c65ef8b against main (e226ac532), +14/−8 over 2 story files, CI green.

The gate that held this yesterday is closed: #4895 fixed the budget entry on main, and this head is green. main itself now fails on exactly the story this PR fixes (plus-menu-during-skill-refresh, run 34017497207), so this is the fix main needs.

The three waitFor wraps are the same idiom #4889 used for the same layer-opacity read, and the explicit release replaces the only clock in the harness. Nothing else changes. #4829 carries a narrower version of the harness change; it will rebase onto this.

AI-assisted review: drafted with Maka; I verified the three reads and the release path myself.

@Astro-Han
Astro-Han merged commit 90014db into apache:main Sep 6, 2026
1 check passed
@orangeCatDeveloper
orangeCatDeveloper deleted the test/storybook-plus-menu-visibility branch September 6, 2026 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/S Under 100 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Storybook smoke: toBeVisible reads a freshly opened Astryx layer as hidden while its entry animation is pending

2 participants