fix(notifications): show close button after progress notification completes#318319
fix(notifications): show close button after progress notification completes#318319SpencerJung wants to merge 1 commit into
Conversation
…pletes (microsoft#314963) Progress notifications kept hasProgress permanently true after done() was called because _progress was never nulled out. This prevented: - The close button from appearing - Clear All from being enabled - otifications.clearAll from dismissing completed progress notifications - Fix hasProgress to check !_progress.state.done - Re-render notification on PROGRESS change to update toolbar actions Fixes microsoft#314963
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates notification progress handling so completed progress is no longer treated as active, and ensures the notification viewer refreshes state when progress changes.
Changes:
- Treats notifications with
progress.state.done === trueas not having active progress. - Triggers a viewer input refresh when progress content changes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/vs/workbench/common/notifications.ts | Refines hasProgress to exclude completed progress states. |
| src/vs/workbench/browser/parts/notifications/notificationsViewer.ts | Refreshes rendered notification input when progress updates occur. |
|
|
||
| get hasProgress(): boolean { | ||
| return !!this._progress; | ||
| return !!this._progress && !this._progress.state.done; |
| break; | ||
| case NotificationViewItemContentChangeKind.PROGRESS: | ||
| this.renderProgress(notification); | ||
| this.setInput(notification); |
|
@SpencerJung please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
1 similar comment
|
@SpencerJung please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
Related Issue
Closes #314963
Summary
After a progress notification completes (
progress.done()is called), the notification'shasProgressgetter still returnedtruebecause_progresswas never nulled out. This caused three broken behaviors:notifications.clearAllcommand silently skipped the notificationChanges
hasProgressgetter to check!!this._progress && !this._progress.state.done(matching the existingstickygetter logic)this.setInput(notification)call onPROGRESScontent change events so the toolbar actions (including close button visibility) are re-rendered when progress completesVerification Evidence
npm run compile-check-ts-native: PASSED (no TypeScript errors)Checklist