Settle a docked dialog's request when a layout load drops its tab - #2828
Merged
Merged
Conversation
rolfheij-sil
marked this pull request as ready for review
September 16, 2026 15:52
rolfheij-sil
requested review from
irahopkinson,
lyonsil and
tjcouch-sil
as code owners
September 16, 2026 15:52
rolfheij-sil
force-pushed
the
dialog-request-survives-layout-load
branch
from
September 18, 2026 11:04
0dc8eaf to
effee11
Compare
lyonsil
reviewed
Sep 18, 2026
lyonsil
left a comment
lyonsil
left a comment
Member
There was a problem hiding this comment.
Automated review of the dialog-request settling change on this PR, checked against head effee11. There is one finding; it is posted inline on the line it lands on.
The severity on the comment is the severity this review settled on, and checked and confirmed means the finding was verified against the code rather than inferred from the diff.
(AI-assisted, with my guidance)
…ps its tab A programmatic loadLayout call (e.g. a Simple/Power mode switch) replaces the dock's entire contents without running rc-dock's per-tab remove callback. Only web views were noticed: emitCloseEventsForWebViewsRemovedByLayoutLoad diffed web view definitions against the loaded layout's web view tabs, so a docked, non-modal dialog dropped by the same load left its entry in dialogRequests forever and its requester's promise never settled. web-view.service-shard.ts generalizes the layout-info tab collector to every tab type (not just web views) and, after each load, emits the surviving tab ids on a new module-level event (onLayoutLoadTabIds). The web-view shard cannot import the dialog shard (the dialog shard already imports the web-view shard), so the event only reports what survived; it keeps no record of what non-web-view tabs existed before the load, leaving each subscriber to compare the survivors against the ids it was itself tracking. dialog.service-shard.ts subscribes in startDialogServiceShard() and resolves any pending request whose tab id is missing from the survivors as though the user canceled (undefined, this service's cancellation value) without trying to close the tab (false — it is already gone). This is distinct from the shutdown sweep, which rejects rather than resolves and never deletes the request from dialogRequests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
…event Starting the dialog shard now subscribes to onLayoutLoadTabIds; the two sibling suites mocked the web-view shard without it and failed at start-up in CI. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
rolfheij-sil
force-pushed
the
dialog-request-survives-layout-load
branch
from
September 21, 2026 10:28
effee11 to
7a7f2bf
Compare
The layout-load sweep in startDialogServiceShard() resolved every dialogRequests entry whose id was missing from the survived-tab set. A request is registered synchronously before showDialog awaits addTab, so a layout load landing in that window between registration and the tab actually reaching the dock was indistinguishable from a load that dropped an already-docked tab: the sweep wrongly resolved the request as a user cancellation, the dialog then docked and showed anyway, and the user's eventual OK/Cancel hit resolveDialogRequest with no request left to resolve, throwing and leaving the tab open. Track whether each request's tab has actually been placed in the dock (set once addTab resolves for it, looked up fresh from the map rather than the closure variable to naturally skip a request already removed by then) and have the sweep only settle requests that are marked docked. Requests still waiting on addTab now survive the sweep, while the original defend-against-a-dropped-docked-tab behavior is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
emitFn runs subscribers through a plain, non-isolating forEach, so a synchronously-throwing onDidCloseWebView subscriber would abort the rest of the loop and anything scheduled after it. Emitting the surviving tab ids first means a misbehaving close subscriber cannot suppress the non-web-view sweep (e.g. the dialog service shard's docked-request settling) that depends on onLayoutLoadTabIds having fired. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
showDialog registered a dialog request into dialogRequests before awaiting addTab, then marked isTabDocked = true one microtask after addTab actually resolved. A whole-layout load's tab-drop sweep runs synchronously inside loadLayout, so a load landing in that one-tick gap between the tab really being placed and isTabDocked being set would find the tab already gone but the request still reading as undocked — the sweep would skip it, leaving the request unsettled forever. Reachable only for an untracked load (one that began against an empty dock), since a tracked load holds showDialog out entirely. addTab now takes an optional onDocked callback, invoked synchronously right after addTabToDock places the tab — strictly before addTab's own promise resolves. showDialog marks isTabDocked from that callback instead of from its own post-await continuation, closing the gap without depending on rc-dock's dock-still-shows-pre-load-tabs quirk and without settling a request whose tab was never placed (a load landing before placement still leaves isTabDocked false, so that request is correctly left alone). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lyonsil
approved these changes
Sep 22, 2026
lyonsil
left a comment
lyonsil
left a comment
Member
There was a problem hiding this comment.
@lyonsil reviewed 7 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on rolfheij-sil).
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
2 participants
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.
What
A programmatic
loadLayoutcall (e.g. a Simple/Powerplatform.interfaceModeswitch) replaces the dock's entire contents without running rc-dock's per-tab remove callback. Only web views were noticed when this happens — a docked, non-modal dialog dropped by the same load left its entry indialogRequestsforever, and the requester's promise never settled.web-view.service-shard.tsgeneralizes its layout-info tab collector to every tab type (not just web views) and, after each load, emits the surviving tab ids on a new module-level event,onLayoutLoadTabIds. The web-view shard can't import the dialog shard (the dialog shard already imports the web-view shard), so the event reports only what survived and leaves each subscriber to compare that against the ids it was itself tracking.dialog.service-shard.tssubscribes instartDialogServiceShard()and resolves any pending request whose tab id is missing from the survivors as though the user canceled (undefined, this service's cancellation value) without attempting to close the tab (false— it's already gone). This is distinct from the existing shutdown sweep, which rejects rather than resolves and never deletes the request.Why
Surfaced during the review of #2821 (per-pane content zoom): with a docked About dialog open, switching interface mode leaked its request, and the zoom feature's dialog gate then stayed shut for the rest of the session. The gate is fixed on that PR; the leak is a pre-existing dialog-lifecycle bug and is fixed here on its own, off
main.Test plan
npm run typecheck,npm run build:types(papi.d.ts unchanged),npm run build:main,npx eslint,npm run format:checkall cleandialog.service-shard.layout-load.test.ts), a control proving a kept tab's request is untouched (dialog.service-shard.test.ts), and a regression guard confirming web-view close events still fire on a layout-load removal (web-view.service-shard.test.ts)AI-assisted — session
🤖 Generated with Claude Code
This change is