Conversation
Behaviorally it was working, but it wasn't showing up in the settings. We also hadn't mapped it into our (pointless IMHO) `Settings` class, or put it under the "correct" section of `integratedConsole`. To be honest, it's open work to migrate away from `getSettings()` and to just using the VS Code APIs directly, but we shouldn't do it now.
a2819f2 to
30d7f1a
Compare
|
Updates #5380. Surprised Copilot didn't point out the issue. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR fixes the visibility and wiring of the “suppress terminal stopped notification” setting by properly mapping it into the extension’s Settings model and aligning the setting key with the integratedConsole grouping.
Changes:
- Add
suppressTerminalStoppedNotificationtoIntegratedConsoleSettingsso it’s represented ingetSettings(). - Update runtime read/write paths to use
integratedConsole.suppressTerminalStoppedNotification. - Rename the contributed VS Code setting ID to
powershell.integratedConsole.suppressTerminalStoppedNotificationso it appears in the intended settings section.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/settings.ts | Adds the missing integrated console setting field to the settings model. |
| src/session.ts | Switches notification suppression logic and “Don’t Show Again” persistence to the integratedConsole setting path. |
| package.json | Renames the contributed setting ID to the integratedConsole namespace so it shows under the correct grouping. |
| if ( | ||
| getSettings().integratedConsole.suppressTerminalStoppedNotification | ||
| ) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
This change drops support for the previous setting key (powershell.suppressTerminalStoppedNotification). Users who already set the old key will start seeing notifications again after updating. Consider adding a backward-compatible fallback (read both old + new keys), and/or a one-time migration that copies the old value into integratedConsole.suppressTerminalStoppedNotification.
| if ( | |
| getSettings().integratedConsole.suppressTerminalStoppedNotification | |
| ) { | |
| return; | |
| } | |
| const suppressTerminalStoppedNotification = | |
| getSettings().integratedConsole | |
| .suppressTerminalStoppedNotification; | |
| const legacySuppressTerminalStoppedNotification = vscode.workspace | |
| .getConfiguration("powershell") | |
| .get<boolean>("suppressTerminalStoppedNotification"); | |
| if ( | |
| !suppressTerminalStoppedNotification && | |
| legacySuppressTerminalStoppedNotification | |
| ) { | |
| await changeSetting( | |
| "integratedConsole.suppressTerminalStoppedNotification", | |
| true, | |
| true, | |
| this.logger, | |
| ); | |
| return; | |
| } | |
| if ( | |
| suppressTerminalStoppedNotification || | |
| legacySuppressTerminalStoppedNotification | |
| ) { | |
| return; | |
| } |
Behaviorally it was working, but it wasn't showing up in the settings. We also hadn't mapped it into our (pointless IMHO)
Settingsclass, or put it under the "correct" section ofintegratedConsole.To be honest, it's open work to migrate away from
getSettings()and to just using the VS Code APIs directly, but we shouldn't do it now.