Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the Agents window CodeReviewService PR-review integration to better manage GitHub PR model polling lifecycles and session cleanup behavior.
Changes:
- Updates PR-review initialization to return an
IDisposableso polling can be stopped viaautorundisposal. - Removes eager PR-review initialization/cleanup tied to session change events and instead ties PR polling to the active session.
- Expands session-change cleanup to dispose PR review data alongside regular code review state resets.
Show a summary per file
| File | Description |
|---|---|
src/vs/sessions/contrib/codeReview/browser/codeReviewService.ts |
Adjusts PR review initialization and polling lifecycle management; refactors session-change cleanup logic. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 2
| return; | ||
| } | ||
| })); | ||
|
|
There was a problem hiding this comment.
The autorun only reacts to activeSession changes. After removing the onDidChangeSessions hook, PR review initialization will not re-run when activeSession.gitHubInfo (and its pullRequest) becomes available later, so PR review can remain stuck in None for the active session. Consider reading activeSession.gitHubInfo (and any other needed observables) inside this autorun, or reintroduce a reactive/session-change trigger for PR review initialization.
| const gitHubInfo = activeSession.gitHubInfo.read(reader); | |
| gitHubInfo?.pullRequest.read(reader); |
| private _ensurePRReviewInitialized(sessionResource: URI): IDisposable { | ||
| const data = this._getOrCreatePRReviewData(sessionResource); | ||
| if (data.initialized) { | ||
| return; | ||
| return Disposable.None; | ||
| } |
There was a problem hiding this comment.
_ensurePRReviewInitialized returns Disposable.None once data.initialized is true, but the caller now disposes the polling handle via reader.store when the active session changes. This means switching away from a session stops PR polling, and switching back later will not restart polling (because the method short-circuits). Suggest separating “wiring initialized” from “polling active” (e.g., keep the model on data and always call startPolling() on activation, returning a disposable that stopPolling()s), so polling reliably resumes when a session becomes active again.
No description provided.