Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the issue and pull request overview panels to show the webview UI before the full issue/PR model is resolved from the GitHub API. This improves perceived performance by displaying the panel immediately while data loads asynchronously.
Changes:
- Introduced
UnresolvedIdentityinterface to represent minimal information (owner, repo, number) needed to display the overview panel - Refactored
IssueOverviewPanelandPullRequestOverviewPanelto accept an optional model parameter and resolve it asynchronously if not provided - Updated all call sites to pass the identity along with the model (or just the identity when the model isn't available yet)
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/github/views.ts | Adds new UnresolvedIdentity interface to represent minimal PR/issue identification |
| src/github/issueOverview.ts | Refactors createOrShow and adds updateWithIdentity method to support async model resolution |
| src/github/pullRequestOverview.ts | Extends base class changes and overrides resolveModel for PR-specific resolution |
| src/uriHandler.ts | Updates URI handlers to pass identity, though PR handler still resolves before showing |
| src/commands.ts | Updates command handlers to construct and pass identity objects |
| src/issues/issueFeatureRegistrar.ts | Updates issue creation flow to pass identity |
| src/view/pullRequestCommentController.ts | Updates PR review opening to pass identity |
| src/github/overviewRestorer.ts | Updates webview restoration to pass identity |
| src/test/github/pullRequestOverview.test.ts | Updates tests to work with new API signature |
| const pullRequest = await this._resolvePullRequestFromIdentity(resolved.identity, resolved.folderManager); | ||
| if (!pullRequest) { | ||
| return; | ||
| } | ||
| return PullRequestOverviewPanel.createOrShow(this._telemetry, this._context.extensionUri, resolved.folderManager, resolved.identity, pullRequest); |
There was a problem hiding this comment.
There's an inconsistency between how issues and pull requests are handled via URI. For issues, the webview is shown immediately with just the identity, and the issue is resolved asynchronously inside IssueOverviewPanel (line 134). However, for pull requests, the PR is still fully resolved before showing the webview (lines 163-167). This seems to contradict the PR's purpose of showing the webview before resolution. Consider updating this to match the issue handling pattern by calling PullRequestOverviewPanel.createOrShow with just the identity and letting it resolve the PR asynchronously.
| const pullRequest = await this._resolvePullRequestFromIdentity(resolved.identity, resolved.folderManager); | |
| if (!pullRequest) { | |
| return; | |
| } | |
| return PullRequestOverviewPanel.createOrShow(this._telemetry, this._context.extensionUri, resolved.folderManager, resolved.identity, pullRequest); | |
| return PullRequestOverviewPanel.createOrShow(this._telemetry, this._context.extensionUri, resolved.folderManager, resolved.identity); |
| const identity = { owner: state.owner, repo: state.repo, number: state.number }; | ||
| if (state.isIssue) { | ||
| const issueModel = await repo.getIssue(state.number, true); | ||
| if (!issueModel) { | ||
| webviewPanel.dispose(); | ||
| return; | ||
| } | ||
| return IssueOverviewPanel.createOrShow(this._telemetry, this._extensionUri, folderManager, issueModel, undefined, true, webviewPanel); | ||
| return IssueOverviewPanel.createOrShow(this._telemetry, this._extensionUri, folderManager, identity, issueModel, undefined, true, webviewPanel); | ||
| } else { | ||
| const pullRequestModel = await repo.getPullRequest(state.number, true); | ||
| if (!pullRequestModel) { | ||
| webviewPanel.dispose(); | ||
| return; | ||
| } | ||
| return PullRequestOverviewPanel.createOrShow(this._telemetry, this._extensionUri, folderManager, pullRequestModel, undefined, true, webviewPanel); | ||
| return PullRequestOverviewPanel.createOrShow(this._telemetry, this._extensionUri, folderManager, identity, pullRequestModel, undefined, true, webviewPanel); | ||
| } |
There was a problem hiding this comment.
The overview restorer still resolves the issue/PR model before showing the webview (lines 58 and 65). If the resolution fails, the panel is disposed without showing anything to the user (lines 59-61, 66-68). Consider updating this to match the pattern used in UriHandler._openIssueWebview, where the webview is shown immediately with the identity and the model is resolved asynchronously. This would provide better user feedback by showing the webview while the model is being loaded.
See below for a potential fix:
return IssueOverviewPanel.createOrShow(this._telemetry, this._extensionUri, folderManager, identity, undefined, undefined, true, webviewPanel);
} else {
return PullRequestOverviewPanel.createOrShow(this._telemetry, this._extensionUri, folderManager, identity, undefined, undefined, true, webviewPanel);
| await result; | ||
| // Notify that this PR overview is now active | ||
| PullRequestOverviewPanel._onVisible.fire(pullRequestModel); | ||
| PullRequestOverviewPanel._onVisible.fire(this._item); |
There was a problem hiding this comment.
The _onVisible event is fired unconditionally with this._item, but this._item could potentially be undefined if an error occurred during updateItem. The existing code at line 189-191 shows that the pattern elsewhere is to check if (this._item) before firing this event. Consider adding a similar guard here to ensure the event is only fired when a valid pull request model is available.
| PullRequestOverviewPanel._onVisible.fire(this._item); | |
| if (this._item) { | |
| PullRequestOverviewPanel._onVisible.fire(this._item); | |
| } |
Part of microsoft/vscode#288848