Fix completed progress notification dismissal - #315184
Conversation
Treat completed progress as non-active for notification affordances and refresh notification rows on progress completion so close and clear-all actions return. Adds model and UI regression coverage, and documents terminal done() contract for notification progress. Fixes microsoft#314963
There was a problem hiding this comment.
Pull request overview
Fixes a workbench notifications UX bug where notifications that previously showed progress could become non-dismissible after progress completion, by treating completed progress as inactive and forcing a row refresh at the completion transition.
Changes:
- Update
NotificationViewItem.hasProgressto returnfalseonce progress isdone(). - Refresh notification list rows in Notification Center and Toasts when progress transitions to completed so close/clear affordances re-render.
- Add regression coverage for
hasProgress/stickysemantics and Notification Center clear affordances; document thedone()terminal contract.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/common/notifications.ts | Makes hasProgress reflect active (non-done) progress state. |
| src/vs/workbench/browser/parts/notifications/notificationsToasts.ts | Forces a toast row re-render when progress completes to restore affordances. |
| src/vs/workbench/browser/parts/notifications/notificationsCenter.ts | Forces a center row re-render when progress completes so toolbar/clear-all recalculates. |
| src/vs/platform/notification/common/notification.ts | Documents done() as a terminal call in the notification progress API. |
| src/vs/workbench/test/common/notifications.test.ts | Adds assertions validating hasProgress/sticky behavior across done(). |
| src/vs/workbench/test/browser/notificationsList.test.ts | Adds browser regression test ensuring completed-progress notifications can be cleared and show close action again. |
@microsoft-github-policy-service agree |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Distinguish active progress from progress-handle presence so completed notifications regain dismissal without changing notification deduplication. Refresh rows only when activity changes and cover start, completion, restart, and uniqueness. Fixes microsoft#314963 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: Benjamin Christopher Simmonds (@benibenj)Matched files:
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixes #314963
Summary
Completed progress notifications could remain visible but non-dismissible in Notification Center:
notifications.clearAlland the focused-notification clear command skipped themFix
hasProgressas progress-handle presence so progress notifications retain their existing deduplication behavior.Validation
npm run typecheck-clientnode build\hygiene.ts <8 changed notification files>scripts\test.bat src\vs\workbench\test\common\notifications.test.ts src\vs\workbench\test\browser\notificationsList.test.ts— 14 passing