Skip to content

fix(tests): finish the public build before closing the popup; widen the messages loading-state wait - #14595

Merged
erichare merged 1 commit into
mainfrom
fix/nightly-31907290063-shards
Aug 15, 2026
Merged

fix(tests): finish the public build before closing the popup; widen the messages loading-state wait#14595
erichare merged 1 commit into
mainfrom
fix/nightly-31907290063-shards

Conversation

@erichare

@erichare erichare commented Aug 15, 2026

Copy link
Copy Markdown
Member

Summary

Nightly 31907290063 (main @ b40b405, first nightly after #14589) failed exactly two Playwright shards. Both are test-side, both diagnosed from the blob-report traces.

Windows 24/70 — messages.a11y.spec.ts "scans the named loading state"

expect(getByRole('status', {name: 'Loading'})).toBeVisible() right after page.goto("/settings/messages") used the default 5s. The trace shows goto returning at load (18–24s Vite module storm), then auto_login (1.6–3.0s) → whoamiconfig → the lazy settings route; the messages query mounted 7.0s / 7.5s after goto — 1.4s / 1.7s after the expect gave up. The aria snapshot at failure was the app-level Loading... boot page, not SessionView's status. Now uses TIMEOUTS.standard, which the identical held-response loading scan in knowledge-bases.a11y already uses.

Linux 41/70 — publish-flow.spec.ts

The spec sent a message in the shareable-playground popup and called newPage.close() 30ms later, while the public build was still in flight. Aborting that request mid-write made the backend terminate its aiosqlite connections under cancellation, and the trace + backend log show a ~60s window where every SQLite writer stalled while reads kept answering in ms:

  • the un-publish PATCH /flows/{id} sent 0.5s after the close never answered (→ toBeChecked({checked:false}) failed);
  • the retry's GET /auto_login hung 34s+ so the app never booted (→ mainpage_title timeout);
  • the sibling worker's fresh start playground build started 0.2s earlier took 71s vs 0.66s for the identical send moments before;
  • backend: Exception terminating connection <aiosqlite …> × 3 with CancelledError … via BaseHTTPMiddleware call_next, then Task was destroyed but it is pending! for _terminate_graceful_close() at 20:51:25 — right when the stall cleared (GC).

Fix: run the message through the shared sendPlaygroundMessage(newPage, "Hello", { surface: "shareable" }), which waits for the Stop button to appear and clear before returning, so the popup is only closed after the build completes. That removes the abort-mid-write trigger from this spec and also proves the published playground actually completes a run rather than merely starting one.

Not addressed here (backend follow-up): the underlying leak looks like a SQLAlchemy terminate() ↔ aiosqlite stop()/close() ordering race under persistent cancellation — the shielded graceful-close task nulls _connection before the busy worker thread reaches close_and_stop, which then skips sqlite3.close(), leaving an open write transaction alive until gen-2 GC. Real users closing a tab mid-build can hit the same stall; happy to write that up as an issue.

Both files are identical on release-1.12.0, so this cherry-picks cleanly.

Test plan

  • npx biome check clean on both specs; tsc clean over both specs (temp config — tsconfig.json's include skips tests/)
  • Local full-stack Playwright run: messages.a11y "scans the named loading state" ✓ 34.8s, publish-flow "user should be able to publish a flow" ✓ 43.7s
  • CI shards green

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Tests
    • Improved accessibility test reliability by allowing additional time for loading states during initialization.
    • Streamlined publish-flow test coverage for the shareable playground experience.

…he messages loading-state wait

Nightly 31907290063 (main @ b40b405) failed exactly two Playwright shards.

Windows 24/70 - messages.a11y "scans the named loading state": the expect
after `page.goto("/settings/messages")` used the default 5s. The trace shows
goto returning at `load`, then auto_login (1.6-3.0s) -> whoami -> config ->
the lazy settings route; the messages query mounted 7.0s / 7.5s after goto,
1.4s / 1.7s after the expect gave up. The aria snapshot at failure was the
app-level "Loading..." page, not SessionView's status. Use TIMEOUTS.standard,
which the identical held-response loading scan in knowledge-bases.a11y
already uses.

Linux 41/70 - publish-flow: the spec sent a message in the shareable
playground popup and closed it 30ms later, while the public build was still
in flight. Aborting that request mid-write made the backend terminate its
aiosqlite connections under cancellation; the trace + backend log show a
~60s window where every SQLite writer stalled (the un-publish PATCH never
answered, the retry's auto_login hung 34s+, the sibling worker's build took
71s instead of 0.66s) while reads kept answering in ms. Wait for the build
to finish (Stop visible -> hidden via the shared sendPlaygroundMessage
helper) before closing the popup, which also proves the published playground
completes a run rather than merely starting one.

Verified locally against the full Playwright stack: both tests pass.
@coderabbitai

coderabbitai Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 92915f4b-ae94-44b3-bda9-20d9de109e60

📥 Commits

Reviewing files that changed from the base of the PR and between b40b405 and 517765c.

📒 Files selected for processing (2)
  • src/frontend/tests/a11y/messages.a11y.spec.ts
  • src/frontend/tests/core/features/publish-flow.spec.ts

Walkthrough

The PR updates two frontend tests. The accessibility test uses an explicit standard timeout. The publish-flow test uses sendPlaygroundMessage for the shareable playground flow.

Changes

Frontend test updates

Layer / File(s) Summary
Loading-state timeout
src/frontend/tests/a11y/messages.a11y.spec.ts
The loading-status assertion uses TIMEOUTS.standard and documents delayed initialization.
Shareable playground message flow
src/frontend/tests/core/features/publish-flow.spec.ts
The test replaces manual message interaction with sendPlaygroundMessage and removes the unused TEXTS import.

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

Merge Risk: ⚪ Minimal · up to 51776

The tests now wait for the messages loading state and for the public build to finish before closing the popup, reducing flaky failures without changing product behavior. No actionable merge-blocking risk remains beyond normal checks.

Possibly related PRs

Suggested labels: test

Suggested reviewers: cristhianzl

🚥 Pre-merge checks | ✅ 9
✅ Passed checks (9 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and specifically summarizes both test fixes: completing the public build before closing the popup and extending the messages loading-state wait.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Test Coverage For New Implementations ✅ Passed The PR updates two existing Playwright regression specs with real loading-state and completed shareable-build assertions; no new product functionality requires additional unit or integration tests.
Test Quality And Coverage ✅ Passed The PR updates only frontend Playwright tests: the loading scan uses a 30s wait, and publish-flow waits for Stop to clear before asserting unpublish and redirect behavior.
Test File Naming And Structure ✅ Passed The PR modifies existing Playwright tests using the repository’s established .spec.ts convention; descriptive names, fixtures, setup, and positive/negative assertions remain intact. No structure fa...
Excessive Mock Usage Warning ✅ Passed The diff adds no mocks. Existing API route stubs in messages.a11y.spec.ts predate the change and isolate external responses; publish-flow now exercises the real playground helper.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/nightly-31907290063-shards

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

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the bug Something isn't working label Aug 15, 2026
@github-actions

Copy link
Copy Markdown
Contributor

✅ Test Coverage Advisor

No source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉

Advisory check only — never blocks merge.

@github-actions github-actions Bot added bug Something isn't working and removed bug Something isn't working labels Aug 15, 2026
@erichare
erichare merged commit 48aac8b into main Aug 15, 2026
101 checks passed
@erichare
erichare deleted the fix/nightly-31907290063-shards branch August 15, 2026 21:50
@github-actions

Copy link
Copy Markdown
Contributor

Frontend Unit Test Coverage Report

Coverage Summary

Lines Statements Branches Functions
Coverage: 53%
53.1% (78256/147367) 70.92% (11081/15623) 48.74% (1830/3754)

Unit Test Results

Tests Skipped Failures Errors Time
6138 0 💤 0 ❌ 0 🔥 22m 4s ⏱️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant