feat(chat): add welcome banner for anonymous users prompting them to …#295869
feat(chat): add welcome banner for anonymous users prompting them to …#295869andysharman wants to merge 1 commit intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an anonymous-user “welcome” variant to ChatStatusWidget so first-time anonymous users can be prompted to try Copilot Chat (with dismiss + persistence), while tightening quota-exceeded banner conditions and adding coverage.
Changes:
- Extend
ChatStatusWidgetto support ananonymousWelcomebanner (click-to-focus + dismiss persisted in profile storage) and log related telemetry. - Add settings
chat.noAuthWidget.enabledand refactor the anonymous quota experiment setting tochat.statusWidget.anonymous(boolean). - Add a new browser test suite for
ChatStatusWidgetcovering visibility/dismissal/history cases.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/test/browser/widget/input/chatStatusWidget.test.ts | New test coverage for welcome/quota banners and dismissal persistence. |
| src/vs/workbench/contrib/chat/browser/widget/input/media/chatStatusWidget.css | Adds styling for clickable welcome text and the dismiss (X) control. |
| src/vs/workbench/contrib/chat/browser/widget/input/chatStatusWidget.ts | Implements welcome banner logic, new settings/storage/telemetry, and quota check adjustment. |
| src/vs/workbench/contrib/chat/browser/chat.contribution.ts | Registers new/updated experimental settings for anonymous welcome/quota experiences. |
Comments suppressed due to low confidence (1)
src/vs/workbench/contrib/chat/browser/widget/input/media/chatStatusWidget.css:82
- The dismiss control draws an outline in the
:focusstate. In this codebase focus outlines are typically applied with:focus-visibleto avoid showing focus rings on mouse interaction while keeping keyboard accessibility. Consider moving the outline styling to:focus-visible(and keep background/opacity changes on:focusif desired).
.interactive-session .interactive-input-part > .chat-input-widgets-container .chat-status-widget .chat-status-dismiss:focus {
opacity: 1;
background-color: var(--vscode-toolbar-hoverBackground);
outline: 1px solid var(--vscode-focusBorder);
outline-offset: 1px;
}
| @@ -47,78 +63,152 @@ export class ChatStatusWidget extends Disposable implements IChatInputPartWidget | |||
| this.initializeIfEnabled(); | |||
There was a problem hiding this comment.
initializeIfEnabled is async but its Promise is neither awaited nor explicitly ignored. This can trigger no-floating-promises linting and can hide unexpected rejections. Prefix the call with void (or otherwise handle the Promise) to make the intent explicit.
| this.initializeIfEnabled(); | |
| void this.initializeIfEnabled(); |
| const handleClick = () => { | ||
| this.telemetryService.publicLog2<WorkbenchActionExecutedEvent, WorkbenchActionExecutedClassification>('workbenchActionExecuted', { | ||
| id: 'chatStatusWidget.welcomeClick', | ||
| from: 'chatStatusWidget' | ||
| }); | ||
| this.chatWidgetService.lastFocusedWidget?.focusInput(); | ||
| }; |
There was a problem hiding this comment.
The welcome banner click handler focuses chatWidgetService.lastFocusedWidget, which can be a different chat widget than the one containing this banner (clicking the banner itself doesn't necessarily update lastFocusedWidget). This can result in focusing the wrong input when multiple chat widgets are open. Prefer resolving the owning widget (e.g., find the widget whose domNode contains this widget’s domNode) and focus that instead.
| private createWidgetContent(enabledSku: 'free' | 'anonymous' | 'anonymousWelcome'): void { | ||
| const contentContainer = $('.chat-status-content'); | ||
| this.messageElement = $('.chat-status-message'); | ||
| contentContainer.appendChild(this.messageElement); |
There was a problem hiding this comment.
enabledSku is no longer a SKU selector (it also includes 'anonymousWelcome'), so the name is misleading. Consider renaming this parameter to something like mode/bannerType to match its meaning.
See below for a potential fix:
private createWidgetContent(mode: 'free' | 'anonymous' | 'anonymousWelcome'): void {
const contentContainer = $('.chat-status-content');
this.messageElement = $('.chat-status-message');
contentContainer.appendChild(this.messageElement);
let dismissButton: HTMLElement | undefined;
if (mode === 'anonymousWelcome') {
| ContextKeyExpr.or( | ||
| ChatContextKeys.Entitlement.planFree, | ||
| ContextKeyExpr.and(ChatContextKeys.chatQuotaExceeded, ChatContextKeys.Entitlement.planFree), | ||
| ContextKeyExpr.and(ChatContextKeys.chatQuotaExceeded, ChatEntitlementContextKeys.chatAnonymous), |
There was a problem hiding this comment.
The when clause includes ContextKeyExpr.and(chatQuotaExceeded, chatAnonymous) but the following chatAnonymous term already makes the widget visible for anonymous users regardless of quota. This makes the expression harder to read and adds an unnecessary dependency on the chatQuotaExceeded context key (extra updates). Consider simplifying the or(...) branches to only the distinct cases you need.
| ContextKeyExpr.and(ChatContextKeys.chatQuotaExceeded, ChatEntitlementContextKeys.chatAnonymous), |
| .interactive-session .interactive-input-part > .chat-input-widgets-container .chat-status-widget .chat-status-message.chat-status-link { | ||
| color: var(--vscode-textLink-foreground); | ||
| cursor: pointer; | ||
| } | ||
|
|
||
| .interactive-session .interactive-input-part > .chat-input-widgets-container .chat-status-widget .chat-status-message.chat-status-link:hover { | ||
| text-decoration: underline; | ||
| } |
There was a problem hiding this comment.
.chat-status-message.chat-status-link is keyboard-focusable (tabIndex=0) but only has a hover style. Add a visible focus indicator (typically via :focus-visible) so keyboard users can tell when the link is focused.
This issue also appears on line 77 of the same file.
andysharman
force-pushed
the
exp-no-auth-banner
branch
from
33fb41f to
238ada7
Compare
|
@microsoft-github-policy-service agree company="Microsoft" |
andysharman
force-pushed
the
exp-no-auth-banner
branch
from
238ada7 to
e88917c
Compare
| this.domNode.appendChild(actionContainer); | ||
| } | ||
|
|
||
| this._register(this.actionButton.onDidClick(async () => { | ||
| const commandId = this.chatEntitlementService.anonymous | ||
| ? CHAT_SETUP_ACTION_ID | ||
| : 'workbench.action.chat.upgradePlan'; | ||
| this.telemetryService.publicLog2<WorkbenchActionExecutedEvent, WorkbenchActionExecutedClassification>('workbenchActionExecuted', { | ||
| id: commandId, | ||
| from: 'chatStatusWidget' | ||
| }); | ||
| await this.commandService.executeCommand(commandId); | ||
| })); | ||
|
|
||
| this.domNode.appendChild(contentContainer); | ||
| this.domNode.appendChild(actionContainer); | ||
| if (dismissButton) { | ||
| this.domNode.appendChild(dismissButton); | ||
| } |
There was a problem hiding this comment.
In the non-welcome modes, actionContainer is appended to domNode before contentContainer. With the widget styled as display: flex and justify-content: space-between, this reverses the intended layout (action/button on the left, message on the right). Append contentContainer first (and then the action/dismiss elements) to keep the message left-aligned and the action right-aligned.
See below for a potential fix:
}
this.domNode.appendChild(contentContainer);
if (dismissButton) {
this.domNode.appendChild(dismissButton);
}
if (mode !== 'anonymousWelcome') {
const actionContainer = this.actionButton?.element.parentElement;
if (actionContainer) {
this.domNode.appendChild(actionContainer);
}
}
| dismissButton.tabIndex = 0; | ||
| const dismissLabel = localize('chat.anonymousWelcome.dismiss', "Dismiss"); | ||
| dismissButton.setAttribute('aria-label', dismissLabel); | ||
| dismissButton.appendChild(renderIcon(Codicon.close)); |
There was a problem hiding this comment.
renderIcon(Codicon.close) returns a plain <span> without aria-hidden. Since the dismiss control already has an aria-label, the icon should be marked aria-hidden="true" (or otherwise hidden from AT) to avoid screen readers announcing both the label and the icon.
| dismissButton.appendChild(renderIcon(Codicon.close)); | |
| const dismissIcon = renderIcon(Codicon.close); | |
| dismissIcon.setAttribute('aria-hidden', 'true'); | |
| dismissButton.appendChild(dismissIcon); |
| if (this._store.isDisposed) { | ||
| return; | ||
| } | ||
| if (history.length === 0) { |
There was a problem hiding this comment.
initializeIfEnabled captures isAnonymous, enabledBanner, and bannerDismissed before awaiting getHistorySessionItems(). Those values can change while the async call is in flight (e.g. user signs in, setting flips, dismissal is stored), which can lead to the welcome banner being shown when it no longer applies. Re-check the relevant conditions after the await (and before calling createWidgetContent('anonymousWelcome')).
| if (history.length === 0) { | |
| // Re-evaluate conditions that may have changed while waiting for history | |
| const isAnonymousAfter = this.chatEntitlementService.anonymous; | |
| const enabledBannerAfter = this.configurationService.getValue<boolean>('chat.noAuthWidget.enabled'); | |
| const bannerDismissedAfter = this.storageService.getBoolean(DISMISS_STORAGE_KEY, StorageScope.PROFILE, false); | |
| if (history.length === 0 && isAnonymousAfter && enabledBannerAfter && !bannerDismissedAfter) { |
| sentiment: IChatSentiment = {}; | ||
| sentimentObs = observableValue<IChatSentiment>(this, {}); | ||
| quotas: IChatEntitlementService['quotas'] = {}; | ||
|
|
There was a problem hiding this comment.
MockChatEntitlementService declares implements IChatEntitlementService but is missing the required markAnonymousRateLimited() method from the interface. This will fail TypeScript compilation for the test file; add a no-op implementation (or delegate to updating the quota context as appropriate for the tests).
| markAnonymousRateLimited(): void { } |
andysharman
force-pushed
the
exp-no-auth-banner
branch
from
e88917c to
88a3a94
Compare
|
Rather than rendering the whole text in blue can we follow the styling we use for our Tips UI. We render an icon in color and use grey text. The chat input box should already be in focus when the panel is open. |
…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
andysharman
force-pushed
the
exp-no-auth-banner
branch
from
88a3a94 to
487ddc9
Compare
Updated to reflect feedback - moved to a more muted experience, retaining dismiss button. |
| if (history.length === 0 && this.chatEntitlementService.anonymous && | ||
| this.configurationService.getValue<boolean>('chat.noAuthWidget.enabled') && | ||
| !this.storageService.getBoolean(DISMISS_STORAGE_KEY, StorageScope.PROFILE, false)) { |
There was a problem hiding this comment.
These checks duplicate the conditions already evaluated on lines 68-72. The values could have changed during the async getHistorySessionItems call, creating a race condition. Since the disposal check on line 85-87 already guards against stale execution, these redundant checks should be removed to avoid confusion and potential inconsistencies.
| if (history.length === 0 && this.chatEntitlementService.anonymous && | |
| this.configurationService.getValue<boolean>('chat.noAuthWidget.enabled') && | |
| !this.storageService.getBoolean(DISMISS_STORAGE_KEY, StorageScope.PROFILE, false)) { | |
| if (history.length === 0) { |
There was a problem hiding this comment.
These re-checks are intentional as they guard against state changes (e.g. sign-in/dismissal) that can happen during async getHistorySessionItems(). The disposal check only covers widget lifecycle, not business logic changes.
Summary
Extends
ChatStatusWidgetto show a welcome banner for anonymous users who haven't used chat yet, prompting them totry GitHub Copilot chat for the first time - it disappears after they've used chat or dismissed the message.
Added test coverage to chatStatusWidget as there's several experiences depending on it.
Relates to https://github.com/microsoft/vscode-internalbacklog/issues/6518
Changes
anonymousWelcomemode inChatStatusWidget— displays a Copilot icon (in link color) with "GitHub Copilot is now enabled." textchat.noAuthWidget.dismissed)chat.noAuthWidget.enabledwith experiment support (experiment: { mode: 'auto' })chat.statusWidget.sku(string enum) →chat.statusWidget.anonymous(boolean) for the anonymous quota-exceeded experimentpercentRemaining === 0, preventing false positiveswelcomeShownandwelcomeDismissactionsVisibility Conditions
The welcome banner shows when all of the following are true:
chat.noAuthWidget.enabledistrue(controlled via experiment)Testing
chat.noAuthWidget.enabled: truein settings