{{ message }}
chore(dashboard): fix some more DashboardController races#40375
Open
Skn0tt wants to merge 10 commits intomicrosoft:mainfrom
Open
chore(dashboard): fix some more DashboardController races#40375Skn0tt wants to merge 10 commits intomicrosoft:mainfrom
Skn0tt wants to merge 10 commits intomicrosoft:mainfrom
Conversation
- 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.
This comment has been minimized.
This comment has been minimized.
- 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.
11103ca to
3d845e0
Compare
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.
This comment has been minimized.
This comment has been minimized.
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>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Contributor
This comment has been minimized.
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).
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

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 indashboard.spec.ts.Concrete example — the screencast/annotate flake:
When a client ran
playwright cli show --annotate, the controller wouldvoid-fire "new tab", "engage annotate mode" and "screen frame" without sequencing them. The order on the wire was often:But annotate locks onto a specific frame, so it needs:
Other races fixed along the way:
playwright cli show --annotatefor different session names would silently coalesce (only one would win)._handleAttachedPageClosecould briefly emit a tabless state before swapping in the new attached page._pushTabsre-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:
revealSessionruns through a single reveal loop; identical concurrent calls dedup, distinct ones supersede via a per-reveal cancel signal._emitTabs()(reads from a_tabDetailscache) and an async per-page_updateDetails(page).BrowserTrackerfires per-page events so only the affected page is re-fetched, and_emitTabs()being sync makes ordering with other emissions trivial._handleAttachedPageCloseawaits the swap before re-emitting.SessionModelindirection in the frontend; the sidebar subscribes directly to the channel.Result:
dashboard.spec.tsno longer flakes (verified locally with--repeat-each=20 --workers=8on webkit, plus full pass across all projects).