chat: make creation of the edit session sync#277924
Conversation
Uses the session state to wait in cases where it's needed Refs #277318
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @jriekenMatched files:
@bpaseroMatched files:
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the chat editing session creation from asynchronous to synchronous, using the session's state observable to wait for initialization where needed. The change simplifies the codebase by removing ObservablePromise wrappers and async method signatures throughout the editing service chain.
Key changes:
- Made
IChatEditingServicemethods synchronous (removedasync/Promisereturns) - Moved session initialization from async
init()method to constructor parameter and private_init() - Introduced
chatEditingSessionIsReady()helper to wait for session state to transition fromInitial - Updated all usage sites to access
editingSessiondirectly instead of awaitingeditingSessionObs?.promise
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| chatEditingService.ts | Changed interface methods to synchronous, added chatEditingSessionIsReady() helper function |
| chatModel.ts | Removed ObservablePromise wrapper, changed editingSession to direct property, made startEditingSession() synchronous |
| chatEditingServiceImpl.ts | Removed async methods, removed _restoringEditingSession field, made session creation synchronous |
| chatEditingSession.ts | Moved init() to private _init() called from constructor, added transferFrom constructor parameter, made entries observable state-aware |
| inlineChatSessionServiceImpl.ts | Updated to use direct editingSession property with null-safe access |
| inlineChatController.ts | Removed awaits on editingSessionObs?.promise, updated to use direct editingSession |
| chatWidget.ts | Updated to use chatEditingSessionIsReady() helper when waiting for session |
| chatMarkdownContentPart.ts | Simplified observable logic, removed nested observables |
| chatChangesSummaryPart.ts | Removed nested promise result checks, simplified to direct session access |
| chatService.test.ts | Updated mock to return synchronous session |
| chatEditingService.test.ts | Removed awaits on session creation, updated to use direct editingSession property |
| autorunSelfDisposable(reader => { | ||
| const state = session.state.read(reader); | ||
| if (state !== ChatEditingSessionState.Initial) { | ||
| reader.dispose(); | ||
| resolve(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The autorunSelfDisposable is never cleaned up if the session is disposed while still in Initial state, leading to a potential memory leak. Consider checking if the session is disposed or handling the onDidDispose event to reject/resolve the promise.
| autorunSelfDisposable(reader => { | |
| const state = session.state.read(reader); | |
| if (state !== ChatEditingSessionState.Initial) { | |
| reader.dispose(); | |
| resolve(); | |
| } | |
| }); | |
| let resolved = false; | |
| const autorunDisposable = autorunSelfDisposable(reader => { | |
| const state = session.state.read(reader); | |
| if (state !== ChatEditingSessionState.Initial) { | |
| if (!resolved) { | |
| resolved = true; | |
| reader.dispose(); | |
| autorunDisposable.dispose(); | |
| if (sessionDisposable) { | |
| sessionDisposable.dispose(); | |
| } | |
| resolve(); | |
| } | |
| } | |
| }); | |
| // Listen for session disposal if possible | |
| let sessionDisposable: IDisposable | undefined; | |
| if ('onDidDispose' in session && typeof (session as any).onDidDispose === 'function') { | |
| sessionDisposable = (session as any).onDidDispose(() => { | |
| if (!resolved) { | |
| resolved = true; | |
| autorunDisposable.dispose(); | |
| resolve(); | |
| } | |
| }); | |
| } |
| initialPosition: editor.getSelection().getStartPosition().delta(-1), /* one line above selection start */ | ||
| chatModel, | ||
| editingSession, | ||
| editingSession: chatModel.editingSession!, |
There was a problem hiding this comment.
Non-null assertion on chatModel.editingSession is unsafe. The chat model doesn't start an editing session automatically; it requires calling startEditingSession() first. This will throw at runtime if editingSession is undefined. Either add a call to chatModel.startEditingSession() before this line or handle the case where it might be undefined.
| public readonly entries: IObservable<readonly IModifiedFileEntry[]> = derived(reader => { | ||
| const state = this._state.read(reader); | ||
| if (state === ChatEditingSessionState.Disposed || state === ChatEditingSessionState.Initial) { | ||
| return []; | ||
| } else { | ||
| return this._entriesObs.read(reader); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The entries observable returns an empty array during the Initial state, which means consumers cannot observe entries until initialization completes. This could cause issues if entries are accessed before _init() finishes. Consider documenting this behavior or ensuring consumers use chatEditingSessionIsReady() before accessing entries.
Uses the session state to wait in cases where it's needed Refs #277318
Uses the session state to wait in cases where it's needed
Refs #277318