Conversation
d04640f to
270630e
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a welcome banner widget for signed-out users prompting them to try GitHub Copilot for free. The banner appears in the chat input area when specific conditions are met and can be permanently dismissed by the user.
Changes:
- New
ChatNoAuthWidgetclass that displays a welcome message with a "Try Copilot" button and dismiss functionality - Configuration setting
chat.noAuthWidget.enabledwith experiment support - CSS styling for the new dismiss button
- Telemetry tracking for widget interactions
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/vs/workbench/contrib/chat/browser/widget/input/chatNoAuthWidget.ts |
New widget implementation with constructor-based initialization, dismiss functionality, telemetry, and button click handler |
src/vs/workbench/contrib/chat/browser/widget/input/media/chatStatusWidget.css |
CSS styles for the dismiss button (X) including hover states |
src/vs/workbench/contrib/chat/browser/chat.contribution.ts |
Added import for the new widget and registered the experimental configuration setting |
Comments suppressed due to low confidence (1)
src/vs/workbench/contrib/chat/browser/widget/input/chatNoAuthWidget.ts:108
- The dismiss button is not keyboard accessible. It should have tabindex="0" to make it focusable and keyboard event handlers for Enter and Space keys. Alternatively, consider using an actual button element instead of a div.
const dismissButton = $('.chat-no-auth-dismiss');
dismissButton.appendChild(renderIcon(Codicon.close));
dismissButton.title = localize('chat.noAuth.dismiss', "Dismiss");
this._register(dom.addDisposableListener(dismissButton, 'click', (e) => {
e.stopPropagation();
this.telemetryService.publicLog2<WorkbenchActionExecutedEvent, WorkbenchActionExecutedClassification>('workbenchActionExecuted', {
id: 'chatNoAuthWidget.dismiss',
from: 'chatNoAuthWidget'
});
this.dismiss();
}));
| // Dismiss button (X) | ||
| const dismissButton = $('.chat-no-auth-dismiss'); | ||
| dismissButton.appendChild(renderIcon(Codicon.close)); | ||
| dismissButton.title = localize('chat.noAuth.dismiss', "Dismiss"); |
There was a problem hiding this comment.
According to the coding guidelines, tooltips should use IHoverService instead of the native title attribute. This provides a more consistent user experience across VS Code. You should inject IHoverService in the constructor and use it to setup the hover for the dismiss button.
| if (!this.chatEntitlementService.anonymous && this.chatWidgetService.lastFocusedWidget) { | ||
| this.chatWidgetService.lastFocusedWidget.focusInput(); | ||
| } else { | ||
| await this.commandService.executeCommand(CHAT_SETUP_ACTION_ID); | ||
| } |
There was a problem hiding this comment.
The button click logic appears to be inverted. For anonymous users who can already use chat, focusing the input makes sense. However, for signed-out users who are not anonymous, they should be prompted to sign in via the setup flow. The current condition checks if NOT anonymous, which means signed-out non-anonymous users will only get their input focused instead of being prompted to sign in. Consider reversing the condition to check if the user IS anonymous before focusing the input.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/vs/workbench/contrib/chat/browser/widget/input/chatNoAuthWidget.ts:75
- The initial visibility logic leaves
domNodevisible with the.chat-status-widgetstyling while the asyncgetHistorySessionItems()check is pending, which can render an empty banner and affect layout height briefly. Consider initializing withdisplay: none(and_isVisible = false) and only showing the node oncecreateWidgetContent()decides to render.
this.domNode = $('.chat-status-widget');
// Check if dismissed in this profile
const isDismissed = this.storageService.getBoolean(DISMISS_STORAGE_KEY, StorageScope.PROFILE, false);
if (isDismissed) {
this.domNode.style.display = 'none';
this._isVisible = false;
} else {
// Controlled by experiment via chat.noAuthWidget.enabled setting
const enabled = this.configurationService.getValue<boolean>('chat.noAuthWidget.enabled');
if (enabled) {
// Don't show if user has prior chat history (they already know about Copilot)
this.chatService.getHistorySessionItems().then(history => {
if (history.length > 0) {
this.domNode.style.display = 'none';
this._isVisible = false;
} else {
this.createWidgetContent();
this.telemetryService.publicLog2<WorkbenchActionExecutedEvent, WorkbenchActionExecutedClassification>('workbenchActionExecuted', {
| // Don't show if user has prior chat history (they already know about Copilot) | ||
| this.chatService.getHistorySessionItems().then(history => { | ||
| if (history.length > 0) { | ||
| this.domNode.style.display = 'none'; | ||
| this._isVisible = false; | ||
| } else { | ||
| this.createWidgetContent(); | ||
| this.telemetryService.publicLog2<WorkbenchActionExecutedEvent, WorkbenchActionExecutedClassification>('workbenchActionExecuted', { | ||
| id: 'chatNoAuthWidget.shown', | ||
| from: 'chatNoAuthWidget' | ||
| }); | ||
| } | ||
| }); | ||
| } else { |
There was a problem hiding this comment.
getHistorySessionItems() can throw (e.g. storage errors) and this code uses .then(...) without a .catch(...), which can cause an unhandled promise rejection and leave the widget in a partially initialized state. Please handle errors (e.g. catch and hide the widget or assume empty history) and also avoid updating DOM/telemetry if the widget has been disposed by the time the promise settles.
This issue also appears on line 57 of the same file.
| // If Copilot is already enabled (user is signed in), just focus the input. | ||
| // The banner will hide automatically when the user sends a message (chatSessionIsEmpty becomes false). | ||
| // Otherwise, trigger the setup flow. | ||
| if (!this.chatEntitlementService.anonymous && this.chatWidgetService.lastFocusedWidget) { | ||
| this.chatWidgetService.lastFocusedWidget.focusInput(); |
There was a problem hiding this comment.
The "Try Copilot" click handler appears to invert the anonymous vs signed-out behavior. This widget is shown for signed-out or anonymous users, but the code focuses the input when anonymous === false (which is the typical signed-out case) and only triggers CHAT_SETUP_ACTION_ID when anonymous === true. If the intent is to run setup for signed-out users and just focus for anonymous-access users, the condition should be flipped (and ideally not depend on lastFocusedWidget being set).
| // If Copilot is already enabled (user is signed in), just focus the input. | |
| // The banner will hide automatically when the user sends a message (chatSessionIsEmpty becomes false). | |
| // Otherwise, trigger the setup flow. | |
| if (!this.chatEntitlementService.anonymous && this.chatWidgetService.lastFocusedWidget) { | |
| this.chatWidgetService.lastFocusedWidget.focusInput(); | |
| // If Copilot is already enabled for anonymous access, just focus the input. | |
| // The banner will hide automatically when the user sends a message (chatSessionIsEmpty becomes false). | |
| // Otherwise (signed-out case), trigger the setup flow. | |
| if (this.chatEntitlementService.anonymous) { | |
| if (this.chatWidgetService.lastFocusedWidget) { | |
| this.chatWidgetService.lastFocusedWidget.focusInput(); | |
| } |
| this._register(this.hoverService.setupManagedHover(getDefaultHoverDelegate('mouse'), dismissButton, localize('chat.noAuth.dismiss', "Dismiss"))); | ||
| this._register(dom.addDisposableListener(dismissButton, 'click', (e) => { | ||
| e.stopPropagation(); | ||
| this.telemetryService.publicLog2<WorkbenchActionExecutedEvent, WorkbenchActionExecutedClassification>('workbenchActionExecuted', { | ||
| id: 'chatNoAuthWidget.dismiss', | ||
| from: 'chatNoAuthWidget' | ||
| }); | ||
| this.dismiss(); | ||
| })); | ||
|
|
There was a problem hiding this comment.
The dismiss control is rendered as a plain element with only a click listener. As-is it is not keyboard-accessible, and the CSS :focus styling won’t apply unless the element can receive focus. Please make it a proper button (or add role="button", tabIndex=0, an accessible name, and Enter/Space key handling) so screen reader and keyboard users can dismiss the banner.
| this._register(this.hoverService.setupManagedHover(getDefaultHoverDelegate('mouse'), dismissButton, localize('chat.noAuth.dismiss', "Dismiss"))); | |
| this._register(dom.addDisposableListener(dismissButton, 'click', (e) => { | |
| e.stopPropagation(); | |
| this.telemetryService.publicLog2<WorkbenchActionExecutedEvent, WorkbenchActionExecutedClassification>('workbenchActionExecuted', { | |
| id: 'chatNoAuthWidget.dismiss', | |
| from: 'chatNoAuthWidget' | |
| }); | |
| this.dismiss(); | |
| })); | |
| const dismissAriaLabel = localize('chat.noAuth.dismiss', "Dismiss"); | |
| dismissButton.setAttribute('role', 'button'); | |
| dismissButton.tabIndex = 0; | |
| dismissButton.setAttribute('aria-label', dismissAriaLabel); | |
| this._register(this.hoverService.setupManagedHover(getDefaultHoverDelegate('mouse'), dismissButton, dismissAriaLabel)); | |
| const handleDismiss = (e: Event): void => { | |
| e.stopPropagation(); | |
| this.telemetryService.publicLog2<WorkbenchActionExecutedEvent, WorkbenchActionExecutedClassification>('workbenchActionExecuted', { | |
| id: 'chatNoAuthWidget.dismiss', | |
| from: 'chatNoAuthWidget' | |
| }); | |
| this.dismiss(); | |
| }; | |
| this._register(dom.addDisposableListener(dismissButton, 'click', e => { | |
| handleDismiss(e); | |
| })); | |
| this._register(dom.addDisposableListener(dismissButton, 'keydown', e => { | |
| const keyboardEvent = e as KeyboardEvent; | |
| if (keyboardEvent.key === 'Enter' || keyboardEvent.key === ' ' || keyboardEvent.key === 'Spacebar' || keyboardEvent.code === 'Space') { | |
| keyboardEvent.preventDefault(); | |
| handleDismiss(keyboardEvent); | |
| } | |
| })); |
| .interactive-session .interactive-input-part > .chat-input-widgets-container .chat-status-widget .chat-no-auth-dismiss:focus { | ||
| opacity: 1; | ||
| background-color: var(--vscode-toolbar-hoverBackground); | ||
| outline: 1px solid var(--vscode-focusBorder); | ||
| outline-offset: 1px; | ||
| } |
There was a problem hiding this comment.
The .chat-no-auth-dismiss:focus rule assumes the dismiss element is focusable, but the current implementation renders it as a non-focusable element. Either make the dismiss control keyboard-focusable (preferred) or update the styling to match the actual element semantics so focus indication works reliably.
| * Shown when the user is signed out and hasn't exceeded any quota (different from ChatStatusWidget). | ||
| * Can be dismissed by clicking the X button. | ||
| */ | ||
| export class ChatNoAuthWidget extends Disposable implements IChatInputPartWidget { |
There was a problem hiding this comment.
can you not reuse the chatStatusWidget to check for no-auth and signed out entitlements?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (2)
src/vs/workbench/contrib/chat/browser/widget/input/chatStatusWidget.ts:88
- In the
getHistorySessionItems()resolution path, the code only checkshistory.length === 0before showing the welcome banner. If entitlement/quota state changes while the promise is pending (e.g. quota becomes exceeded, user signs in, banner gets dismissed), the banner can still show even though the visibility conditions are no longer met. Re-check the gating conditions inside thethenblock before rendering/logging.
this.chatService.getHistorySessionItems().then(history => {
if (history.length === 0) {
this.createWidgetContent('anonymousWelcome');
this.domNode.style.display = '';
src/vs/workbench/contrib/chat/browser/widget/input/chatStatusWidget.ts:87
- The PR description says the banner should show when the current chat session is empty, but the implementation additionally requires
getHistorySessionItems().length === 0(i.e. no prior chat history). If the extra "first-time user" constraint is intended, please update the PR description/visibility conditions; otherwise, remove the history gate and rely onchatSessionIsEmpty.
this.chatService.getHistorySessionItems().then(history => {
if (history.length === 0) {
this.createWidgetContent('anonymousWelcome');
| // Show for free/anonymous users on empty sessions | ||
| ChatInputPartWidgetsRegistry.register( | ||
| ChatStatusWidget.ID, | ||
| ChatStatusWidget, | ||
| ContextKeyExpr.and( |
There was a problem hiding this comment.
ChatStatusWidget is instantiated/initialized only once (constructor calls initializeIfEnabled()), but the when clause no longer includes chatQuotaExceeded. If quota info arrives later via onDidChangeQuotaExceeded/onDidChangeQuotaRemaining, the widget won't re-evaluate and may never show the quota-exceeded banner. Consider restoring ChatContextKeys.chatQuotaExceeded in the when expression (and/or listening to entitlement/quota change events and re-rendering accordingly).
| // Show for free/anonymous users on empty sessions | |
| ChatInputPartWidgetsRegistry.register( | |
| ChatStatusWidget.ID, | |
| ChatStatusWidget, | |
| ContextKeyExpr.and( | |
| // Show for free/anonymous users on empty sessions when quota is exceeded | |
| ChatInputPartWidgetsRegistry.register( | |
| ChatStatusWidget.ID, | |
| ChatStatusWidget, | |
| ContextKeyExpr.and( | |
| ChatContextKeys.chatQuotaExceeded, |
|
|
||
| const $ = dom.$; | ||
|
|
||
| const DISMISS_STORAGE_KEY = 'chat.statusWidget.dismissed'; |
There was a problem hiding this comment.
The dismissal storage key name (chat.statusWidget.dismissed) is now used specifically for the anonymous welcome banner. Rename it to something banner-specific (e.g. chat.noAuthWidget.dismissed) to avoid confusion and future collisions if the quota status widget ever needs its own dismissal state.
| const DISMISS_STORAGE_KEY = 'chat.statusWidget.dismissed'; | |
| const DISMISS_STORAGE_KEY = 'chat.noAuthWidget.dismissed'; |
bd7478a to
520a4a8
Compare
… widget transparency
Adopting for configuration-editing, emmet, grunt, jake, and npm
520a4a8 to
1aa0ede
Compare
…gunt Adopt esbuild instead of webpack for a few more extensions
…download-fix Add retries for target download logic in sanity tests
…prevent-ad-redirects Prevent tracking redirects in fetch tool
* rename * tests * fixes * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix test * Update src/vs/workbench/contrib/chat/common/promptSyntax/service/promptsService.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…5070) * Support prompt file slash commands in backgrround agents * Update tests * Update comments
We should be using cjs here still. Only webviews use esm
…icrosoft#293628) * fixfix: scope remote terminal env vars to correct workspace folder Remote terminals used last active editor workspace instead of the terminal's actual CWD, activating the wrong Python environment in multi-root workspaces. Fixes microsoft/vscode-python-environments#986 * Simpler and more reliable fix Add tests to getWorkspaceForTerminal
188e734 to
33fb41f
Compare
* Pass pixel dimension through terminal resize * Update src/vs/workbench/contrib/terminal/browser/terminalInstance.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This reverts commit 9b64d3b.
Fix restoring non-local chat session options
Revert "Use sequence as default terminal tab title"
Refactor chat status widget for no auth scenario
* fix sending new chat request in sessions window * sessions - handle deleted active session by switching to the next available session * sessions - ensure session switch only occurs for deleted known agent sessions * refactor: rename ISessionsWorkbenchService to ISessionsManagementService and update references - Replaced all instances of ISessionsWorkbenchService with ISessionsManagementService across multiple files. - Updated the implementation of the active session management to reflect the new service name. - Removed the old sessionsWorkbenchService.ts file and created a new sessionsManagementService.ts file with the updated logic. * fix: update variable name from agentSessionsService to sessionsManagementService for consistency * feat: make sidebar title area draggable and prevent dragging of interactive elements * feat: add draggable title area for sidebar to enable window movement * fix: update draggable area for interactive elements in sidebar title * fix: update draggable area for interactive elements in sidebar title * fix: update layout for chat widget extension pickers and sorting logic * feat: implement dynamic target configuration for chat widget based on workspace context * fix: remove local agent session from allowed targets in NewChatWidget * feat: add local folder picker for workspace selection in NewChatWidget * feat: enhance folder selection with recent folders in NewChatWidget * feat: integrate action widget service for improved folder selection in NewChatWidget * feat: implement custom folder dropdown for recent folder selection in NewChatWidget * feat: replace custom folder dropdown with DropdownMenu for improved folder selection in NewChatWidget * feat: update computeAllowedTargets to conditionally include Local agent session provider * feat: replace DropdownMenu with SelectBox for improved folder selection in NewChatWidget * feat: enhance folder selection with context menu and improved button UI in NewChatWidget
…soft#295838) * Initial plan * Add lock group feature when opening integrated browser side-by-side Co-authored-by: jruales <1588988+jruales@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: jruales <1588988+jruales@users.noreply.github.com>
…293566) Instead of always showing the `eyeClosed` icon, show the `eye` or `eyeClosed` icon depending on the current visibility.
…als (microsoft#295073) * Support multi-root external terminal selection * Dont use any * Can I get rid of unknown? * unknown to any * edit tests * try fix test * better types? * Use service directly
* Fix tsgo ext build problem matching * Update .vscode/tasks.json Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Fix double timestamp --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Fixes an issue where stopping a chat session would fail to cancel the request when a message was queued. The problem occurred because queued messages could replace the pending request before the finally block attempted to delete it. The fix stores a reference to the CancellableRequest and checks that it matches before deleting, preventing race conditions between queued messages and request cancellation. Fixes microsoft#295022 (Commit message generated by Copilot)
…rmouse Remove dead code
* revamp model picker * implement new model picker and enable it in sessions window * cleanup * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update src/vs/workbench/contrib/chat/browser/widget/input/chatModelPicker.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * feedback * fix color * revert --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…oft#295866) * queuing: interrupt when messages are sent during confirmation Got this feedback from a few people that steering/queuing during a tool call confirmation should just cancel the confirmation and send the message. This does that. Also cleans up the opt-in setting I had added initially that was only partially working, now that the feature has shipped. * comments
* improved yaml parser * Update src/vs/workbench/contrib/chat/common/promptSyntax/promptFileParser.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Try to align `scanBuiltinExtensions` with new esbuild changes
…og (microsoft#293837) * fix: return undefined when user canceled input * fix: resolve eslint warning local/code-no-any-casts
e88917c to
88a3a94
Compare
…try Copilot
Extends ChatStatusWidget to handle a third case ('anonymousWelcome') alongside
the existing 'free' and 'anonymous' quota-exceeded banners.
When the experiment setting chat.noAuthWidget.enabled is true, anonymous users
with no chat history see a 'GitHub Copilot is now enabled.' banner with a
dismiss (X) button. Clicking the message focuses the chat input; dismissing
persists via profile storage.
Changes:
- New setting chat.noAuthWidget.enabled (experiment-controlled)
- Refactored chat.statusWidget.sku enum to chat.statusWidget.anonymous boolean
- Added explicit quota check before showing quota-exceeded banners
- Updated when clause to show widget for anonymous users even without quota exceeded
- Telemetry for welcomeShown, welcomeClick, welcomeDismiss actions
- Keyboard accessible dismiss button with IHoverService tooltip
- Focus styles and link styling for the welcome message
88a3a94 to
487ddc9
Compare
Summary
Adds a welcome banner in the chat input area prompting signed-out users to try GitHub Copilot for free.
Issue link here: https://github.com/microsoft/vscode-internalbacklog/issues/6518
Changes
ChatNoAuthWidgetdisplaying "GitHub Copilot is now enabled to try for free." with a "Try Copilot" CTAchat.noAuthWidget.enabledwith experiment support (experiment: { mode: 'auto' })shown,dismiss, andtryCopilotactionsVisibility Conditions
Banner shows when ALL are true:
chat.noAuthWidget.enabledistrue(controlled via experiment)Testing
chat.noAuthWidget.enabled: truein settings