Repository navigation
Fix race condition in modal editor part singleton creation - #327123
Merged
Benjamin Christopher Simmonds (benibenj) merged 1 commit intoJul 23, 2026
Merged
Benjamin Christopher Simmonds (benibenj) merged 1 commit into
Benjamin Christopher Simmonds (benibenj) merged 1 commit into
Conversation
createModalEditorPart awaited the ModalEditorPart creation before assigning the singleton reference, so concurrent callers could each pass the "already exists" check and create duplicate modal parts/groups. Track the in-flight creation promise and have concurrent callers await and reuse it, applying their own options to the shared instance once it resolves. Adds a regression test that invokes createModalEditorPart concurrently via Promise.all and asserts a single instance/group is returned and only one group-registration event fires. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Benjamin Christopher Simmonds (benibenj)
enabled auto-merge (squash)
July 23, 2026 13:12
Copilot started reviewing on behalf of
Benjamin Christopher Simmonds (benibenj)
July 23, 2026 13:12
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Prevents duplicate modal editor parts during concurrent singleton creation.
Changes:
- Tracks in-flight modal editor part creation.
- Adds concurrent singleton creation coverage.
Show a summary per file
| File | Description |
|---|---|
editorParts.ts |
Reuses an in-flight modal creation promise. |
modalEditorGroup.test.ts |
Tests concurrent creation and event count. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Medium
Comment on lines
+204
to
+208
| const [modalPart1, modalPart2, modalPart3] = await Promise.all([ | ||
| parts.createModalEditorPart(), | ||
| parts.createModalEditorPart(), | ||
| parts.createModalEditorPart() | ||
| ]); |
Comment on lines
+211
to
+216
| const createPromise = this.doCreateModalEditorPart(options).finally(() => { | ||
| this.modalEditorPartCreatePromise = undefined; | ||
| }); | ||
| this.modalEditorPartCreatePromise = createPromise; | ||
|
|
||
| return createPromise; |
Martin Aeschlimann (aeschli)
approved these changes
Jul 23, 2026
Benjamin Christopher Simmonds (benibenj)
merged commit Jul 23, 2026
b8d3a33
into
main
30 checks passed
Benjamin Christopher Simmonds (benibenj)
deleted the
fix/modal-editor-singleton-race
branch
July 23, 2026 13:30
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Summary
Fixes a race condition in modal editor part singleton creation identified while reviewing #326885.
EditorParts.createModalEditorPart awaited the ModalEditorPart creation (�wait this.instantiationService.createInstance(ModalEditorPart, this).create(...)) before assigning his.modalEditorPart. If createModalEditorPart was called concurrently (e.g. two callers racing to open a modal editor), both calls could pass the "reuse existing instance" check and each create its own ModalEditorPart, resulting in duplicate modal editor parts/groups and duplicate onDidAddGroup events.
Fix
This does not include the separate stable-ID change from #326885 — that should land separately.
Testing
pm run typecheck-client - 0 errors.