Update terminal suggest status bar UX, change suggest defaults, show suggest in initial hint #285060
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the terminal suggest status bar to add new actions for controlling IntelliSense behavior and adds icon support to the suggest widget status bar. The changes include refactoring the configuration schema to support boolean values for quickSuggestions, changing default values for suggest settings, and adding new menu actions with icons.
Key Changes
- Added icon support to
SuggestWidgetStatusthrough a newISuggestWidgetStatusOptionsinterface - Changed
quickSuggestionsconfiguration to accept boolean or object values, with a new normalization function - Changed configuration defaults:
quickSuggestionsfrom object tofalse,suggestOnTriggerCharactersfromtruetofalse - Reorganized terminal suggest status bar menu with new actions for toggling selection modes and IntelliSense visibility
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
suggestWidgetStatus.ts |
Added ISuggestWidgetStatusOptions interface and updated constructor to support icon display via allowIcons option |
simpleSuggestWidget.ts |
Updated two instances of SuggestWidgetStatus instantiation to pass { allowIcons: true } option |
suggestWidget.ts |
Updated SuggestWidgetStatus instantiation to pass { allowIcons: true } option |
terminalSuggestConfiguration.ts |
Changed quickSuggestions type to accept boolean or object, added normalizeQuickSuggestionsConfig function, changed defaults for quickSuggestions and suggestOnTriggerCharacters to false |
terminal.suggest.ts |
Added six new command IDs for selection mode changes and show/hide IntelliSense actions |
terminalSuggestAddon.ts |
Updated to use normalizeQuickSuggestionsConfig when reading quickSuggestions configuration |
terminal.suggest.contribution.ts |
Refactored status bar actions: added three selection mode toggle actions, two show/hide IntelliSense actions with icons, reorganized existing actions into right group with icons, removed menu entry from AcceptSelectedSuggestion action |
src/vs/workbench/contrib/terminalContrib/suggest/browser/terminal.suggest.contribution.ts
Show resolved
Hide resolved
src/vs/workbench/contrib/terminalContrib/suggest/common/terminalSuggestConfiguration.ts
Show resolved
Hide resolved
src/vs/workbench/contrib/terminalContrib/suggest/common/terminalSuggestConfiguration.ts
Show resolved
Hide resolved
src/vs/workbench/contrib/terminalContrib/suggest/browser/terminal.suggest.contribution.ts
Show resolved
Hide resolved
src/vs/workbench/contrib/terminalContrib/suggest/browser/terminal.suggest.contribution.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/terminalContrib/suggest/browser/terminal.suggest.contribution.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/terminalContrib/suggest/browser/terminal.suggest.contribution.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/terminalContrib/suggest/browser/terminal.suggest.contribution.ts
Outdated
Show resolved
Hide resolved
This comment was marked as spam.
This comment was marked as spam.
| (commandLineHasSpace && quickSuggestions.arguments !== 'off') | ||
| ) { | ||
| if (promptInputState.prefix.match(/[^\s]$/)) { | ||
| sent = this._requestTriggerCharQuickSuggestCompletions(); |
There was a problem hiding this comment.
🟡 MEDIUM - Duplicate nested loop for trigger character checking
Agent: performance
Category: performance
Description:
The same nested loop pattern appears in _sync method (lines 557-570), checking trigger characters on every keystroke.
Suggestion:
Refactor to use a cached trigger character Set. Move trigger character compilation to a dedicated method that only runs when providers change.
Confidence: 70%
Rule: perf_quadratic_loops
Review ID: 34d87b36-1c2b-444c-96bb-3508e6175650
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| export function normalizeQuickSuggestionsConfig(config: ITerminalSuggestConfiguration['quickSuggestions']): ITerminalQuickSuggestionsOptions { | ||
| if (typeof config === 'boolean') { | ||
| return config | ||
| ? { commands: 'on', arguments: 'on', unknown: 'off' } | ||
| : { commands: 'off', arguments: 'off', unknown: 'off' }; | ||
| } | ||
| return config; | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM - Exported function missing JSDoc documentation
Agent: react
Category: docs
Description:
The exported function 'normalizeQuickSuggestionsConfig' lacks JSDoc documentation. Has inline comments (lines 71-76) but no formal JSDoc.
Suggestion:
Convert existing inline comments to proper JSDoc format with @param and @returns tags.
Confidence: 60%
Rule: ts_always_use_jsdoc_for_documentation
Review ID: 34d87b36-1c2b-444c-96bb-3508e6175650
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| return this._lastUserData === '\t'; | ||
| } | ||
|
|
||
| private _sync(promptInputState: IPromptInputModelState): void { |
There was a problem hiding this comment.
🟠 HIGH - Very long method with excessive nesting (4-5 levels)
Agent: refactoring
Category: quality
Description:
_sync is 141 lines long with deeply nested if-statements. Contains a brace scope block (lines 520-595) that increases indentation.
Suggestion:
Extract nested logic into separate methods: (1) _handleCursorMovedRight() for cursor movement logic, (2) _handleCursorMovedLeft() for left cursor logic.
Confidence: 75%
Rule: quality_guard_clauses
Review ID: 34d87b36-1c2b-444c-96bb-3508e6175650
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
Review Summary
Validated 87 issues: 19 kept, 68 filtered Issues Found: 19💬 See 4 individual line comment(s) for details. 📊 10 unique issue type(s) across 19 location(s) 📋 Full issue list (click to expand)🟠 HIGH - Floating promise: async function not awaited (2 occurrences)Agent: nodejs Category: bug 📍 View all locations
Rule: 🟠 HIGH - Unsafe type assertion of potentially null valueAgent: bugs Category: bug File: Description: The code casts e.relatedTarget as HTMLElement without checking if it's null. FocusEvent.relatedTarget can be null when focus moves to nothing. Suggestion: Use 'if (e.relatedTarget instanceof HTMLElement && e.relatedTarget.classList.contains(SuggestDetailsClassName))' for proper type checking. Confidence: 60% Rule: 🟠 HIGH - Very long method with excessive nesting and complexity (2 occurrences)Agent: refactoring Category: quality 📍 View all locations
Rule: 🟠 HIGH - Unused variable 'barrier' declared but never initializedAgent: refactoring Category: bug File: Description: The variable 'barrier' is declared as 'AutoOpenBarrier | undefined' but never initialized. barrier?.open() will never call open() since barrier remains undefined. This appears to be incomplete code. Suggestion: Either initialize the barrier variable with 'barrier = new AutoOpenBarrier(timeoutMs)' before it's used, or remove this block entirely if it's not needed. Confidence: 90% Rule: 🟠 HIGH - Non-null assertion on untrusted get() result (2 occurrences)Agent: typescript Category: bug 📍 View all locations
Rule: 🟡 MEDIUM - Module-level mutable state for configuration (3 occurrences)Agent: nodejs Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Nested loops in trigger character check (2 occurrences)Agent: performance Category: performance 📍 View all locations
Rule: 🟡 MEDIUM - Exported function missing JSDoc documentationAgent: react Category: docs File: Description: The exported function 'normalizeQuickSuggestionsConfig' lacks JSDoc documentation. Has inline comments (lines 71-76) but no formal JSDoc. Suggestion: Convert existing inline comments to proper JSDoc format with @param and @returns tags. Confidence: 60% Rule: 🟡 MEDIUM - Type assertion from function return without type guarantee (4 occurrences)Agent: typescript Category: bug 📍 View all locations
Rule: 🟡 MEDIUM - Missing JSDoc for exported functionAgent: react Category: docs Why this matters: Improves IntelliSense, cross-language usage, and code navigation. File: Description: Exported function 'normalizePathSeparator' lacks JSDoc documentation Suggestion: Add JSDoc comment with @param and @returns tags describing path normalization behavior Confidence: 60% Rule: ℹ️ 15 issue(s) outside PR diff (click to expand)
🟠 HIGH - Floating promise: async function not awaited (2 occurrences)Agent: nodejs Category: bug 📍 View all locations
Rule: 🟠 HIGH - Unsafe type assertion of potentially null valueAgent: bugs Category: bug File: Description: The code casts e.relatedTarget as HTMLElement without checking if it's null. FocusEvent.relatedTarget can be null when focus moves to nothing. Suggestion: Use 'if (e.relatedTarget instanceof HTMLElement && e.relatedTarget.classList.contains(SuggestDetailsClassName))' for proper type checking. Confidence: 60% Rule: 🟠 HIGH - Very long method with excessive nesting and complexityAgent: refactoring Category: quality File: Description: _handleCompletionProviders is 125 lines long with multiple levels of nested conditionals. High cyclomatic complexity due to if-statements handling different scenarios. Suggestion: Extract methods: (1) Create _validateTerminalState() for initial validations, (2) Create _prepareCompletionRequest() for request setup, (3) Create _processCompletions() for completion processing Confidence: 75% Rule: 🟠 HIGH - Unused variable 'barrier' declared but never initializedAgent: refactoring Category: bug File: Description: The variable 'barrier' is declared as 'AutoOpenBarrier | undefined' but never initialized. barrier?.open() will never call open() since barrier remains undefined. This appears to be incomplete code. Suggestion: Either initialize the barrier variable with 'barrier = new AutoOpenBarrier(timeoutMs)' before it's used, or remove this block entirely if it's not needed. Confidence: 90% Rule: 🟠 HIGH - Non-null assertion on untrusted get() result (2 occurrences)Agent: typescript Category: bug 📍 View all locations
Rule: 🟡 MEDIUM - Module-level mutable state for configuration (3 occurrences)Agent: nodejs Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Nested loops in trigger character checkAgent: performance Category: performance File: Description: The _checkProviderTriggerCharacters method uses nested loops to check if a character matches any provider trigger characters. O(n*m) complexity could cause lag with many providers. Suggestion: Build a Set of all trigger characters upfront (in constructor or when providers change), then do O(1) lookups. Confidence: 70% Rule: 🟡 MEDIUM - Unsafe type assertion to access private property (3 occurrences)Agent: typescript Category: bug 📍 View all locations
Rule: 🟡 MEDIUM - Missing JSDoc for exported functionAgent: react Category: docs Why this matters: Improves IntelliSense, cross-language usage, and code navigation. File: Description: Exported function 'normalizePathSeparator' lacks JSDoc documentation Suggestion: Add JSDoc comment with @param and @returns tags describing path normalization behavior Confidence: 60% Rule: Review ID: |
Tyriar
changed the title
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @jriekenMatched files:
|
Part of #284277
Part of #282268
New status bar that is more clear that these things can be clicked to configure:
Left action can be clicked to rotate selection modes (ie. what keybinding is used when it shows):
The eye icon can be used to easily enable quick suggestions and suggest on trigger chars:
The initial hint now and calls out how to show suggestions:
The initial hint will show more often now as it was kind of weird before. Now it will show for all new terminals that have shell integration on the first prompt. Clicking don't show will change a setting and permanently disable.
Terminal suggest is still enabled by default, but quick suggestions and suggest on trigger character are off. What this means is that suggest will never show up unless ctrl+space is pressed explicitly. I expect this to pretty dramatically impact feature usage but it will keep it out of the way for users that don't want it and it will keep terminal input as low latency as possible by default.