Skip to content

test(mirror-fold): wait for the phone header instead of sampling it - #3204

Merged
devswha merged 1 commit into
mainfrom
fix/main-mirror-fold
Nov 6, 2026
Merged

devswha merged 1 commit into
mainfrom
fix/main-mirror-fold

Conversation

@devswha

@devswha devswha commented Nov 6, 2026

Copy link
Copy Markdown
Owner

Summary

scripts/mirror-fold-demo-regression.ts fails on main (6dfcc17) in CI and on old-desktop alone: choose() → hover: Timeout 10000ms exceeded on .tab-strip, "element was detached from the DOM".

Cause: choose() decided phone vs desktop with a one-shot isVisible() on .app-header .context-tabs, right after the page was resized from a desktop-wide window (844/915 px) to 390 px. The fold state it waits for first is already true at 844, so nothing waited for the phone layout. Since #3156 the desktop tab row lives in the header; at that moment the header still held it, so the test took the desktop branch and hovered a strip that the phone layout then removed.

Diagnostic run on old-desktop (plain main, instrumented choose):

CHOOSE Original width {"w":390,"ctx":0,"strips":[["header-tabs",32,52]]}
LATER  Original width {"w":390,"ctx":1,"strips":[]}   (3 s later)

The app ends in the right layout; only the test sampled it too early. Not a product bug.

Fix (test only): callers say which layout they expect; the phone branch waits for the phone's title (locator.waitFor()) before opening More. No sleeps, no retries.

Validation

  • scripts/mirror-fold-demo-regression.ts 20/20 PASS on old-desktop (ovis-remote-check, where plain main fails), FAILS=0.
  • Same script passes locally (P620), also 8 concurrent runs.
  • bun run check fast: green (typecheck, build, unit 12806 pass / 0 fail).

choose() sampled `.app-header .context-tabs` with isVisible() right after the
page was resized from a desktop-wide window to 390 px. The header still held
the desktop tab row at that moment (#3156 moved it into the header), so the
test took the desktop branch and hovered a strip the phone layout then took
away: "hover: Timeout ... element was detached from the DOM".

Callers now say which layout they expect, and the phone branch waits for the
phone's title before opening More.
@coderabbitai

coderabbitai Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Explain Change →

No actionable comments were generated in the recent review.

Summary

Summary by CodeRabbit

  • Tests
    • Improved regression checks for tab navigation across phone and desktop layouts. Phone scenarios now account for the phone tab layout and its header menu, while desktop scenarios continue to verify the desktop tab menu. This helps ensure the appropriate navigation experience is exercised across screen sizes during automated browser testing.

The regression helper now takes an explicit phone argument to select the phone or desktop menu. The affected test calls pass the corresponding layout value.

Changes: Regression menu selection

Layer / File(s) Summary
Select menus by device layout
scripts/mirror-fold-demo-regression.ts
The helper waits for phone context tabs before opening More, or opens the desktop tab menu. Phone and desktop test calls pass the matching argument.

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Other

Possibly related PRs

  • devswha/ovis#1961: Adds the mirror-fold regression and the Original width and Wrap to screen actions used by this test.
  • devswha/ovis#1963: Extends the fold regression to attached panes watched on phones.
  • devswha/ovis#3099: Changes the phone menu interaction that the regression helper now selects.
Merge Risk: Minimal · up to 2de8c

The change replaces timing-sensitive layout detection with explicit menu selection. No actionable merge-blocking risk is identified; merge after normal checks.

Pre-merge checks: Passed 5

Check name Status Explanation
Title check Passed The title describes the test change that waits for the phone header.
Description check Passed The description explains the failure, fix, and validation results. It uses Summary instead of Change and does not list skipped checks.
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files.

Finishing Touches

Generate docstrings

  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)

  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 6dfcc17 and 2de8c35.

Files selected for processing: 1

  • scripts/mirror-fold-demo-regression.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

@devswha
devswha merged commit db3c54d into main Nov 6, 2026
12 checks passed
@devswha

devswha commented Nov 6, 2026

Copy link
Copy Markdown
Owner Author

Merge note (merge pass) at head 2de8c35:

  • Test-only fix for main's red mirror-fold-demo-regression.ts:209 (phone callers wait for the phone header after the 844 -> 390 resize).
  • omo review receipt: CLEAR at this exact head. Unresolved review threads: 0 (GraphQL). CodeRabbit: success.
  • Verified: the script passes alone twice on main 6dfcc17 + this head on a second PC (old-desktop), where it failed on plain main; Fast checks are green. Merged ahead of GitHub's browser shards to turn main green.

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.

1 participant