Terminal notification support #294703
Terminal notification support #294703
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for displaying VS Code notifications triggered by terminal OSC 99 sequences, integrating it as a terminal contribution and exposing a user setting to enable/disable the feature.
Changes:
- Introduces an
Osc99NotificationHandler+ terminal contribution that parses OSC 99 metadata/payloads and shows notifications with actions/reports. - Adds
terminal.integrated.enableNotificationssetting (and wires it through config/types). - Adds a browser test suite covering core OSC 99 notification behaviors.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/terminalContrib/oscNotifications/browser/terminal.oscNotifications.contribution.ts | Implements OSC 99 parsing, notification creation/update, action handling, and terminal contribution registration. |
| src/vs/workbench/contrib/terminalContrib/oscNotifications/test/browser/terminalOscNotifications.test.ts | Adds unit tests validating notification creation, chunking, reporting, querying, and urgency mapping. |
| src/vs/workbench/contrib/terminal/terminal.all.ts | Registers the new OSC notifications terminal contribution. |
| src/vs/workbench/contrib/terminal/common/terminalConfiguration.ts | Adds configuration schema for terminal.integrated.enableNotifications. |
| src/vs/workbench/contrib/terminal/common/terminal.ts | Extends ITerminalConfiguration with enableNotifications. |
| src/vs/platform/terminal/common/terminal.ts | Adds TerminalSettingId.EnableNotifications. |
| src/vs/workbench/contrib/terminal/browser/terminalInstance.ts | Introduces an extra blank line (formatting-only change). |
...vs/workbench/contrib/terminalContrib/notifications/browser/terminal.notifications.handler.ts
Show resolved
Hide resolved
...vs/workbench/contrib/terminalContrib/notifications/browser/terminal.notifications.handler.ts
Show resolved
Hide resolved
...s/workbench/contrib/terminalContrib/notifications/test/browser/terminalNotifications.test.ts
Show resolved
Hide resolved
| if (state.id) { | ||
| const existing = this._osc99ActiveNotifications.get(state.id); | ||
| if (existing) { | ||
| existing.handle.updateMessage(message); | ||
| existing.handle.updateSeverity(severity); | ||
| existing.handle.updateActions(actions); | ||
| existing.actionStore.dispose(); | ||
| existing.actionStore = actionStore; | ||
| existing.focusOnActivate = state.focusOnActivate; | ||
| existing.reportOnActivate = state.reportOnActivate; | ||
| existing.reportOnClose = state.reportOnClose; | ||
| existing.autoCloseDisposable?.dispose(); | ||
| existing.autoCloseDisposable = this._scheduleOsc99AutoClose(existing.handle, state.autoCloseMs); | ||
| return; |
There was a problem hiding this comment.
When updating an existing active notification (existing branch), all actions created above still reference handleRef.current, but handleRef.current is never set to existing.handle before returning. This means the updated notification’s button/focus/dismiss/disable actions won’t close the notification (and may not behave as expected). Set handleRef.current = existing.handle before calling updateActions(...) (or restructure action creation to use the actual handle directly) so actions work for updates as well as initial creation.
| private _showOsc99Notification(state: IOsc99NotificationState): void { | ||
| const message = this._getOsc99NotificationMessage(state); | ||
| if (!message) { | ||
| return; | ||
| } | ||
|
|
||
| const severity = state.urgency === 2 ? Severity.Warning : Severity.Info; | ||
| const priority = this._getOsc99NotificationPriority(state.urgency); | ||
| const source = { |
There was a problem hiding this comment.
OSC 99 payload text is untrusted, but notification messages support markdown-style links including command: (see parseLinkedText), and the notifications UI opens links with { allowCommands: true }. As a result, a terminal could present a clickable [label](command:...) link that executes a command when clicked. The handler should sanitize/neutralize command: links (and ideally all markdown link syntax) in title/body before passing message to notify, and add a test that verifies command: cannot be triggered via OSC 99 content.
| ) { | ||
| super(); | ||
| this._handler = this._register(new Osc99NotificationHandler({ | ||
| isEnabled: () => this._configurationService.getValue(TerminalOscNotificationsSettingId.EnableNotifications), |
There was a problem hiding this comment.
IConfigurationService.getValue(...) is used without a type parameter here, which effectively makes isEnabled return any and loses boolean type safety. Use getValue<boolean>(...) === true (or otherwise coerce to boolean) to ensure the host implementation matches IOsc99NotificationHost.isEnabled(): boolean.
| isEnabled: () => this._configurationService.getValue(TerminalOscNotificationsSettingId.EnableNotifications), | |
| isEnabled: () => this._configurationService.getValue<boolean>(TerminalOscNotificationsSettingId.EnableNotifications) === true, |
1 participant
Fixes #294247