Skip to content

Commit eee287d

Browse files
alexr00alexr00
authored
Finish unifying notifications (#7971)
- show copilot and pr notifications in the badge - be better about unnecessary refreshes of models when fetching notifications - cache issues
1 parent 7534e31 commit eee287d

File tree

12 files changed

+207
-38
lines changed

12 files changed

+207
-38
lines changed

‎src/common/uri.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,8 @@ export function fromPRUri(uri: vscode.Uri): PRUriParams | undefined {
5353
}
5454

5555
export interface PRNodeUriParams {
56-
prIdentifier: string
56+
prIdentifier: string;
57+
showCopilot?: boolean;
5758
}
5859

5960
export function fromPRNodeUri(uri: vscode.Uri): PRNodeUriParams | undefined {
@@ -501,12 +502,15 @@ export function parsePRNodeIdentifier(identifier: string): { remote: string, prN
501502
}
502503

503504
export function createPRNodeUri(
504-
pullRequest: PullRequestModel | { remote: string, prNumber: number } | string
505+
pullRequest: PullRequestModel | { remote: string, prNumber: number } | string, showCopilot?: boolean
505506
): vscode.Uri {
506507
const identifier = createPRNodeIdentifier(pullRequest);
507508
const params: PRNodeUriParams = {
508509
prIdentifier: identifier,
509510
};
511+
if (showCopilot !== undefined) {
512+
params.showCopilot = showCopilot;
513+
}
510514

511515
const uri = vscode.Uri.parse(`PRNode:${identifier}`);
512516

‎src/github/folderRepositoryManager.ts‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2130,10 +2130,10 @@ export class FolderRepositoryManager extends Disposable {
21302130
useCache: boolean = false,
21312131
): Promise<PullRequestModel | undefined> {
21322132
const githubRepo = await this.resolveItem(owner, repositoryName);
2133-
Logger.appendLine(`Found GitHub repo for pr #${pullRequestNumber}: ${githubRepo ? 'yes' : 'no'}`, this.id);
2133+
Logger.trace(`Found GitHub repo for pr #${pullRequestNumber}: ${githubRepo ? 'yes' : 'no'}`, this.id);
21342134
if (githubRepo) {
21352135
const pr = await githubRepo.getPullRequest(pullRequestNumber, useCache);
2136-
Logger.appendLine(`Found GitHub pr repo for pr #${pullRequestNumber}: ${pr ? 'yes' : 'no'}`, this.id);
2136+
Logger.trace(`Found GitHub pr repo for pr #${pullRequestNumber}: ${pr ? 'yes' : 'no'}`, this.id);
21372137
return pr;
21382138
}
21392139
return undefined;
@@ -2144,10 +2144,14 @@ export class FolderRepositoryManager extends Disposable {
21442144
repositoryName: string,
21452145
pullRequestNumber: number,
21462146
withComments: boolean = false,
2147+
useCache: boolean = false
21472148
): Promise<IssueModel | undefined> {
21482149
const githubRepo = await this.resolveItem(owner, repositoryName);
2150+
Logger.trace(`Found GitHub repo for issue #${pullRequestNumber}: ${githubRepo ? 'yes' : 'no'}`, this.id);
21492151
if (githubRepo) {
2150-
return githubRepo.getIssue(pullRequestNumber, withComments);
2152+
const issue = await githubRepo.getIssue(pullRequestNumber, withComments, useCache);
2153+
Logger.trace(`Found GitHub issue repo for issue #${pullRequestNumber}: ${issue ? 'yes' : 'no'}`, this.id);
2154+
return issue;
21512155
}
21522156
return undefined;
21532157
}

‎src/github/githubRepository.ts‎

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,13 @@ export class GitHubRepository extends Disposable {
177177
value.model.dispose();
178178
}
179179
});
180+
private _issueModelsByNumber: LRUCache<number, { model: IssueModel, disposables: vscode.Disposable[] }> = new LRUCache({
181+
maxAge: 1000 * 60 * 60 * 4 /* 4 hours */, stale: true, updateAgeOnGet: true,
182+
dispose: (_key, value) => {
183+
disposeAll(value.disposables);
184+
value.model.dispose();
185+
}
186+
});
180187
// eslint-disable-next-line rulesdir/no-any-except-union-method-signature
181188
private _queriesSchema: any;
182189
private _areQueriesLimited: boolean = false;
@@ -206,10 +213,18 @@ export class GitHubRepository extends Disposable {
206213
return this._pullRequestModelsByNumber.get(prNumber)?.model;
207214
}
208215

216+
getExistingIssueModel(issueNumber: number): IssueModel | undefined {
217+
return this._issueModelsByNumber.get(issueNumber)?.model;
218+
}
219+
209220
get pullRequestModels(): PullRequestModel[] {
210221
return Array.from(this._pullRequestModelsByNumber.values().map(value => value.model));
211222
}
212223

224+
get issueModels(): IssueModel[] {
225+
return Array.from(this._issueModelsByNumber.values().map(value => value.model));
226+
}
227+
213228
public async ensureCommentsController(): Promise<void> {
214229
try {
215230
if (this.commentsController) {
@@ -1009,6 +1024,19 @@ export class GitHubRepository extends Disposable {
10091024
return model;
10101025
}
10111026

1027+
private createOrUpdateIssueModel(issue: Issue): IssueModel {
1028+
let model = this._issueModelsByNumber.get(issue.number)?.model;
1029+
if (model) {
1030+
model.update(issue);
1031+
} else {
1032+
model = new IssueModel(this.telemetry, this, this.remote, issue);
1033+
// No issue-specific event emitters yet; store empty disposables list for symmetry/cleanup
1034+
const disposables: vscode.Disposable[] = [];
1035+
this._issueModelsByNumber.set(issue.number, { model, disposables });
1036+
}
1037+
return model;
1038+
}
1039+
10121040
private _onPullRequestModelChanged(model: PullRequestModel, change: IssueChangeEvent): void {
10131041
this._onDidChangePullRequests.fire([{ model, event: change }]);
10141042
}
@@ -1094,14 +1122,23 @@ export class GitHubRepository extends Disposable {
10941122
}
10951123

10961124
Logger.debug(`Fetch pull request ${id} - done`, this.id);
1097-
return this.createOrUpdatePullRequestModel(await parseGraphQLPullRequest(data.repository.pullRequest, this));
1125+
const pr = this.createOrUpdatePullRequestModel(await parseGraphQLPullRequest(data.repository.pullRequest, this));
1126+
await pr.getLastUpdateTime(new Date(pr.item.updatedAt));
1127+
return pr;
10981128
} catch (e) {
10991129
Logger.error(`Unable to fetch PR: ${e}`, this.id);
11001130
return;
11011131
}
11021132
}
11031133

1104-
async getIssue(id: number, withComments: boolean = false): Promise<IssueModel | undefined> {
1134+
async getIssue(id: number, withComments: boolean = false, useCache: boolean = false): Promise<IssueModel | undefined> {
1135+
if (useCache) {
1136+
const cached = this._issueModelsByNumber.get(id)?.model;
1137+
if (cached) {
1138+
Logger.debug(`Using cached issue model for ${id}`, this.id);
1139+
return cached;
1140+
}
1141+
}
11051142
try {
11061143
Logger.debug(`Fetch issue ${id} - enter`, this.id);
11071144
const { query, remote, schema } = await this.ensure();
@@ -1121,7 +1158,9 @@ export class GitHubRepository extends Disposable {
11211158
}
11221159
Logger.debug(`Fetch issue ${id} - done`, this.id);
11231160

1124-
return new IssueModel(this.telemetry, this, remote, await parseGraphQLIssue(data.repository.issue, this));
1161+
const issue = this.createOrUpdateIssueModel(await parseGraphQLIssue(data.repository.issue, this));
1162+
await issue.getLastUpdateTime(new Date(issue.item.updatedAt));
1163+
return issue;
11251164
} catch (e) {
11261165
Logger.error(`Unable to fetch issue: ${e}`, this.id);
11271166
return;

‎src/github/interface.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -260,7 +260,7 @@ export interface Notification {
260260
};
261261
reason: string;
262262
unread: boolean;
263-
updatedAd: Date;
263+
updatedAt: Date;
264264
lastReadAt: Date | undefined;
265265
}
266266

‎src/github/issueModel.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,8 @@ export class IssueModel<TItem extends Issue = Issue> extends Disposable {
6565
public body: string;
6666
public bodyHTML?: string;
6767

68+
private _lastCheckedForUpdatesAt?: Date;
69+
6870
private _timelineEvents: readonly TimelineEvent[] | undefined;
6971

7072
protected _onDidChange = this._register(new vscode.EventEmitter<IssueChangeEvent>());
@@ -93,6 +95,10 @@ export class IssueModel<TItem extends Issue = Issue> extends Disposable {
9395
}
9496
}
9597

98+
public get lastCheckedForUpdatesAt(): Date | undefined {
99+
return this._lastCheckedForUpdatesAt;
100+
}
101+
96102
public get isOpen(): boolean {
97103
return this.state === GithubItemStateEnum.Open;
98104
}
@@ -420,6 +426,8 @@ export class IssueModel<TItem extends Issue = Issue> extends Disposable {
420426

421427
async getLastUpdateTime(time: Date): Promise<Date> {
422428
Logger.debug(`Fetch timeline events of issue #${this.number} - enter`, IssueModel.ID);
429+
// Record when we initiated this check regardless of outcome so callers can know staleness.
430+
this._lastCheckedForUpdatesAt = new Date();
423431
const githubRepository = this.githubRepository;
424432
const { query, remote, schema } = await githubRepository.ensure();
425433
try {

‎src/github/utils.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1561,7 +1561,7 @@ export function parseNotification(notification: OctokitCommon.Notification): Not
15611561
lastReadAt: notification.last_read_at ? new Date(notification.last_read_at) : undefined,
15621562
reason: notification.reason,
15631563
unread: notification.unread,
1564-
updatedAd: new Date(notification.updated_at),
1564+
updatedAt: new Date(notification.updated_at),
15651565
};
15661566
}
15671567

‎src/notifications/notificationsManager.ts‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ export class NotificationsManager extends Disposable implements vscode.TreeDataP
6666
this._startPolling();
6767
this._register(vscode.workspace.onDidChangeConfiguration(e => {
6868
if (e.affectsConfiguration(`${PR_SETTINGS_NAMESPACE}.${NOTIFICATION_SETTING}`)) {
69-
if (this._isPRNotificationsOn() && !this._pollingHandler) {
69+
if (this.isPRNotificationsOn() && !this._pollingHandler) {
7070
this._startPolling();
7171
}
7272
}
@@ -182,14 +182,18 @@ export class NotificationsManager extends Disposable implements vscode.TreeDataP
182182
markdown.appendMarkdown(`[${ownerName}](https://github.com/${ownerName}) \n`);
183183
markdown.appendMarkdown(`**${notification.subject.title}** \n`);
184184
markdown.appendMarkdown(`Type: ${notification.subject.type} \n`);
185-
markdown.appendMarkdown(`Updated: ${notification.updatedAd.toLocaleString()} \n`);
185+
markdown.appendMarkdown(`Updated: ${notification.updatedAt.toLocaleString()} \n`);
186186
markdown.appendMarkdown(`Reason: ${notification.reason} \n`);
187187

188188
return markdown;
189189
}
190190

191191
//#endregion
192192

193+
public get prNotifications(): PullRequestModel[] {
194+
return Array.from(this._notifications.values()).filter(notification => notification.notification.subject.type === NotificationSubjectType.PullRequest).map(n => n.model) as PullRequestModel[];
195+
}
196+
193197
public async getNotifications(): Promise<INotificationTreeItems | undefined> {
194198
let pollInterval = this._pollingDuration;
195199
let lastModified = this._pollingLastModified;
@@ -396,15 +400,15 @@ export class NotificationsManager extends Disposable implements vscode.TreeDataP
396400

397401
private _sortNotifications(notifications: NotificationTreeItem[]): NotificationTreeItem[] {
398402
if (this._sortingMethod === NotificationsSortMethod.Timestamp) {
399-
return notifications.sort((n1, n2) => n2.notification.updatedAd.getTime() - n1.notification.updatedAd.getTime());
403+
return notifications.sort((n1, n2) => n2.notification.updatedAt.getTime() - n1.notification.updatedAt.getTime());
400404
} else if (this._sortingMethod === NotificationsSortMethod.Priority) {
401405
return notifications.sort((n1, n2) => Number(n2.priority) - Number(n1.priority));
402406
}
403407

404408
return notifications;
405409
}
406410

407-
private _isPRNotificationsOn() {
411+
public isPRNotificationsOn() {
408412
return (vscode.workspace.getConfiguration(PR_SETTINGS_NAMESPACE).get<NotificationVariants>(NOTIFICATION_SETTING) === 'pullRequests');
409413
}
410414

@@ -422,7 +426,7 @@ export class NotificationsManager extends Disposable implements vscode.TreeDataP
422426
// Adapt polling interval if it has changed.
423427
if (response.pollInterval !== this._pollingDuration) {
424428
this._pollingDuration = response.pollInterval;
425-
if (this._pollingHandler && this._isPRNotificationsOn()) {
429+
if (this._pollingHandler && this.isPRNotificationsOn()) {
426430
Logger.appendLine('Notifications: Clearing interval', NotificationsManager.ID);
427431
clearInterval(this._pollingHandler);
428432
Logger.appendLine(`Notifications: Starting new polling interval with ${this._pollingDuration}`, NotificationsManager.ID);
@@ -437,7 +441,7 @@ export class NotificationsManager extends Disposable implements vscode.TreeDataP
437441
}
438442

439443
private _startPolling() {
440-
if (!this._isPRNotificationsOn()) {
444+
if (!this.isPRNotificationsOn()) {
441445
return;
442446
}
443447
this._pollForNewNotifications();

‎src/notifications/notificationsProvider.ts‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -117,9 +117,22 @@ export class NotificationsProvider extends Disposable {
117117
return undefined;
118118
}
119119
const folderManager = this._repositoriesManager.getManagerForRepository(notification.owner, notification.name) ?? this._repositoriesManager.folderManagers[0];
120-
const model = notification.subject.type === NotificationSubjectType.Issue ?
121-
await folderManager.resolveIssue(notification.owner, notification.name, parseInt(issueOrPrNumber), true) :
122-
await folderManager.resolvePullRequest(notification.owner, notification.name, parseInt(issueOrPrNumber));
120+
let model: IssueModel<Issue> | undefined;
121+
const isIssue = notification.subject.type === NotificationSubjectType.Issue;
122+
123+
model = isIssue
124+
? await folderManager.resolveIssue(notification.owner, notification.name, parseInt(issueOrPrNumber), true, true)
125+
: await folderManager.resolvePullRequest(notification.owner, notification.name, parseInt(issueOrPrNumber), true);
126+
127+
if (model) {
128+
const modelCheckedForUpdates = model.lastCheckedForUpdatesAt;
129+
const notificationUpdated = notification.updatedAt;
130+
if (notificationUpdated.getTime() > (modelCheckedForUpdates?.getTime() ?? 0)) {
131+
model = isIssue
132+
? await folderManager.resolveIssue(notification.owner, notification.name, parseInt(issueOrPrNumber), true, false)
133+
: await folderManager.resolvePullRequest(notification.owner, notification.name, parseInt(issueOrPrNumber), false);
134+
}
135+
}
123136
return model;
124137
}
125138

‎src/view/prStatusDecorationProvider.ts‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { PrsTreeModel } from './prsTreeModel';
88
import { Disposable } from '../common/lifecycle';
99
import { Protocol } from '../common/protocol';
1010
import { NOTIFICATION_SETTING, NotificationVariants, PR_SETTINGS_NAMESPACE } from '../common/settingKeys';
11+
import { EventType } from '../common/timelineEvent';
1112
import { createPRNodeUri, fromPRNodeUri, fromQueryUri, parsePRNodeIdentifier, PRNodeUriParams, Schemes, toQueryUri } from '../common/uri';
1213
import { CopilotRemoteAgentManager } from '../github/copilotRemoteAgent';
1314
import { getStatusDecoration } from '../github/markdownUtils';
@@ -39,18 +40,26 @@ export class PRStatusDecorationProvider extends Disposable implements vscode.Fil
3940
repoItems.add(queryUri.toString());
4041
uris.push(queryUri);
4142
}
42-
uris.push(createPRNodeUri(item));
43+
uris.push(createPRNodeUri(item, true));
4344
}
4445
this._onDidChangeFileDecorations.fire(uris);
4546
}));
4647

48+
const addUriForRefresh = (uris: vscode.Uri[], pullRequest: unknown) => {
49+
if (pullRequest instanceof PullRequestModel) {
50+
uris.push(createPRNodeUri(pullRequest));
51+
if (pullRequest.timelineEvents.some(t => t.event === EventType.CopilotStarted)) {
52+
// The pr nodes in the Copilot category have a different uri so we need to refresh those too
53+
uris.push(createPRNodeUri(pullRequest, true));
54+
}
55+
}
56+
};
57+
4758
this._register(
4859
this._notificationProvider.onDidChangeNotifications(notifications => {
4960
let uris: vscode.Uri[] = [];
5061
for (const notification of notifications) {
51-
if (notification.model instanceof PullRequestModel) {
52-
uris.push(createPRNodeUri(notification.model));
53-
}
62+
addUriForRefresh(uris, notification.model);
5463
}
5564
this._onDidChangeFileDecorations.fire(uris);
5665
})
@@ -61,9 +70,7 @@ export class PRStatusDecorationProvider extends Disposable implements vscode.Fil
6170
if (e.affectsConfiguration(`${PR_SETTINGS_NAMESPACE}.${NOTIFICATION_SETTING}`)) {
6271
const uris: vscode.Uri[] = [];
6372
for (const pr of this._notificationProvider.getAllNotifications()) {
64-
if (pr.model instanceof PullRequestModel) {
65-
uris.push(createPRNodeUri(pr.model));
66-
}
73+
addUriForRefresh(uris, pr.model);
6774
}
6875
this._onDidChangeFileDecorations.fire(uris);
6976
}
@@ -106,6 +113,9 @@ export class PRStatusDecorationProvider extends Disposable implements vscode.Fil
106113
}
107114

108115
private _getCopilotDecoration(params: PRNodeUriParams): vscode.FileDecoration | undefined {
116+
if (!params.showCopilot) {
117+
return;
118+
}
109119
const idParts = parsePRNodeIdentifier(params.prIdentifier);
110120
if (!idParts) {
111121
return;

0 commit comments

Comments
 (0)