chore(dashboard): fix some more DashboardController races by Skn0tt · Pull Request #40375 · microsoft/playwright · GitHub
Skip to content

chore(dashboard): fix some more DashboardController races#40375

Open
Skn0tt wants to merge 10 commits intomicrosoft:mainfrom
Skn0tt:refactor-dashboard-controller
Open

chore(dashboard): fix some more DashboardController races#40375
Skn0tt wants to merge 10 commits intomicrosoft:mainfrom
Skn0tt:refactor-dashboard-controller

Conversation

@Skn0tt
Copy link
Copy Markdown
Member

@Skn0tt Skn0tt commented Apr 23, 2026

The dashboard controller was sprinkled with void somePromise() calls, which meant event ordering between client and dashboard was effectively undefined. This caused a number of subtle bugs and at least one persistent flake in dashboard.spec.ts.

Concrete example — the screencast/annotate flake:
When a client ran playwright cli show --annotate, the controller would void-fire "new tab", "engage annotate mode" and "screen frame" without sequencing them. The order on the wire was often:

new tab → engage annotate → screen frame

But annotate locks onto a specific frame, so it needs:

new tab → screen frame → engage annotate

Other races fixed along the way:

  • Two clients calling playwright cli show --annotate for different session names would silently coalesce (only one would win).
  • _handleAttachedPageClose could briefly emit a tabless state before swapping in the new attached page.
  • _pushTabs re-fetched title+favicon for every page on every event, with no ordering guarantee on which emission won.

The fix is mostly about making sequencing explicit:

  • revealSession runs through a single reveal loop; identical concurrent calls dedup, distinct ones supersede via a per-reveal cancel signal.
  • Tab emission split into a sync _emitTabs() (reads from a _tabDetails cache) and an async per-page _updateDetails(page). BrowserTracker fires per-page events so only the affected page is re-fetched, and _emitTabs() being sync makes ordering with other emissions trivial.
  • _handleAttachedPageClose awaits the swap before re-emitting.
  • Dropped the SessionModel indirection in the frontend; the sidebar subscribes directly to the channel.

Result: dashboard.spec.ts no longer flakes (verified locally with --repeat-each=20 --workers=8 on webkit, plus full pass across all projects).

- Serialize push/coalesce: replace ad-hoc scheduled/running/rerun booleans
  on _pushTabs/_pushSessions with a deferred (_*Next + _*ResolveNext)
  pattern, so callers can await emissions for ordering.
- Reorder _pushSessions body to emit before _reconcile so the UI doesn't
  wait for a slow BrowserTracker.create.
- Latest-wins coalesce for _switchAttachedTo via _pendingSwitchPage and a
  shared in-flight promise; replaces the .catch().then() chain trick.
- _handleAttachedPageClose: skip fallback attach when the context is
  already closed.
- closeSession reuses the cached browser tracker when present.
- Wrap page.title() and faviconUrl() in a withTimeout helper.
- Async onclose: clean up the recording dir and close+unlink stream files.
@github-actions

This comment has been minimized.

Skn0tt added 2 commits April 23, 2026 10:59
- Replace anyConnection() helper with a hasConnection ManualPromise
  that callers await before fanning out to all current connections.
- triggerAnnotate(signal) now returns the AnnotationResult (or undefined
  when aborted) via a ManualPromise; the caller writes the result to its
  socket. Removes the waitingSockets bookkeeping from the server core.
- Wrap the per-message socket handler in a single try/catch/finally;
  errors are serialized as {error} so the annotate client surfaces them.
- Collapse the createWebSocket connection setup to a single const.
Replace the _runRevealLoop / _pendingReveal / _revealLoopRunning state
machine with a linear async _doReveal task that waits on a re-armable
_sessionsChangedNext signal until the target page exists.

- Dedup identical in-flight reveals.
- Distinct reveals supersede the in-flight one (per-entry cancel
  ManualPromise raced against _sessionsChangedNext).
- _pushSessions just signals; no longer calls into reveal logic.
- onclose rejects the in-flight cancel so any waiter unblocks.
@Skn0tt Skn0tt force-pushed the refactor-dashboard-controller branch from 11103ca to 3d845e0 Compare April 23, 2026 09:15
Skn0tt added 2 commits April 23, 2026 11:26
Make _handleAttachedPageClose async and await _switchAttachedTo before
_pushTabs, so a single broadcast reflects the new attached page instead
of pushing twice with a wrong intermediate state.
Inline the SessionModel wrapper into SessionSidebar via useState/useEffect
on the 'sessions' event, alongside the existing 'tabs' subscription.
Move SessionStatus's BrowserStatus alias away — sessionSidebar now uses
BrowserStatus directly. Hoist client + visibilitychange registration into
App so they have a proper React lifecycle, and delete sessionModel.ts.
@github-actions

This comment has been minimized.

Skn0tt and others added 3 commits April 23, 2026 11:40
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

When switching the attached page, the server emitted the first frame
of the new page before the page-change tabs event. The client clears
liveFrame on page change, so the dropped frame left CLI annotate
stuck in cliAnnotatePending forever (static pages emit no further
frames).

Serialize tabs emit before screencast start so the wire ordering is
always tabs then frame.
@github-actions
Copy link
Copy Markdown
Contributor

@github-actions

This comment has been minimized.

Replace _pushTabs/_runPushTabs (which re-fetched title+favicon for every
page on every event) with:
- _tabDetails cache keyed by Page
- sync _emitTabs() that builds tabs from the cache and prunes orphans
- async _updateDetails(page) that refreshes one page and re-emits

BrowserTracker now fires per-page events (onPageNavigated, onPageClosed)
plus onContextClosed, so only the affected page is re-fetched. Drops the
redundant framenavigated listener in AttachedPage.init.

Tab.title becomes optional in the channel (sidebar already tolerates it).
@Skn0tt Skn0tt changed the title chore(dashboard): clean up DashboardController races chore(dashboard): fix some more DashboardController races Apr 23, 2026
@Skn0tt Skn0tt marked this pull request as ready for review April 23, 2026 12:15
@Skn0tt Skn0tt requested a review from dgozman April 23, 2026 12:27
@github-actions
Copy link
Copy Markdown
Contributor

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