Sessions: Report cloud session pull requests#327393
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds pull request reporting for Copilot Cloud sessions in the Agents window.
Changes:
- Resolves PRs from provider metadata or repository branch data.
- Shares live PR icon polling and caching behavior.
- Adds documentation and provider tests.
Show a summary per file
| File | Description |
|---|---|
copilotChatSessionsProvider.test.ts |
Tests cloud PR resolution and icon updates. |
COPILOT_CHAT_SESSIONS_PROVIDER.md |
Documents cloud PR behavior. |
copilotChatSessionsProvider.ts |
Adds cloud PR inference and live icons. |
baseAgentHostSessionsProvider.ts |
Uses the shared icon helper. |
pullRequestIconStatus.ts |
Centralizes live and cached PR icon resolution. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Medium
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@benibenj |
| ...info, | ||
| pullRequest: { | ||
| ...pullRequest, | ||
| icon: computeSessionPullRequestIcon(reader, this._gitHubService, this._pullRequestIconCache, info) |
There was a problem hiding this comment.
AI Review: Could pullRequestUrl be the source of truth for the PR number here? _extractPullRequestNumber currently returns metadata.pullRequestNumber before parsing the URL, while the URI and icon cache continue to use the URL. With conflicting metadata, the live model can load one PR while the reported URI/cache refer to another.
| return { | ||
| ...info, | ||
| pullRequest: { | ||
| ...pullRequest, |
There was a problem hiding this comment.
AI Review: Similarly, _extractOwnerRepo prefers owner/name, repositoryNwo, and the workspace URI before pullRequestUrl. If those disagree, this live lookup can poll one repository while the reported PR URL points to another. Should owner/repo be parsed from pullRequestUrl whenever it is present?
| - Constructs with initial values from the agent session's metadata and timing | ||
| - `update(session)` performs a batched observable transaction to update all reactive properties | ||
| - Extracts workspace info, changes, description, and GitHub info from session metadata | ||
| - Treats a cloud provider's `pullRequestUrl` as authoritative and falls back to resolving the PR from `owner`/`name`/`branch` when the URL is absent |
There was a problem hiding this comment.
AI Review: Does URL-only metadata satisfy this contract? For cloud sessions, _buildWorkspace constructs a path from metadata.owner and metadata.name; when only pullRequestUrl is supplied, that becomes /undefined/undefined/..., and _extractOwnerRepo accepts the resulting workspace URI before reaching the URL fallback. Could the URL be checked first, or workspace construction guard missing owner/name?
| owner: 'owner', | ||
| name: 'repo', | ||
| branch: 'feature', | ||
| pullRequestUrl: prUri.toString(), |
There was a problem hiding this comment.
AI Review: Could this test deliberately conflict the scalar metadata with pullRequestUrl and add a URL-only case? The current tests use agreeing repository metadata and no conflicting pullRequestNumber, so they do not exercise the precedence paths above or the /undefined/undefined/ case.
Summary
Testing