Add experiment for sign in button in title bar#305723
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an experimental configuration/experiment to surface a “Sign In” affordance in the custom title bar area for signed-out Copilot users, aligning with existing title bar indicator patterns.
Changes:
- Introduces
chat.signInTitleBar.enabledas an experimental, auto-enrolled experiment-backed setting. - Adds a new
TitleBarAdjacentCenteraction + custom action view item to render a “Sign In” button styled like the update indicator. - Updates the chat status bar “Signed out” label to “Sign In”.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/common/constants.ts | Adds new ChatConfiguration.SignInTitleBarEnabled setting key constant. |
| src/vs/workbench/contrib/chat/browser/chatStatus/chatStatusEntry.ts | Changes signed-out status label text to “Sign In”. |
| src/vs/workbench/contrib/chat/browser/chatSetup/chatSetupContributions.ts | Registers title-bar-adjacent sign-in action + custom view item via IActionViewItemService. |
| src/vs/workbench/contrib/chat/browser/chat.contribution.ts | Registers the new experimental setting with experiment metadata. |
Comments suppressed due to low confidence (1)
src/vs/workbench/contrib/chat/browser/chatSetup/chatSetupContributions.ts:845
- This entry relies on the
.update-indicatorCSS that’s currently imported by the update contribution (updateTitleBarEntry.ts). That creates a hidden cross-feature dependency: if the update contribution/CSS isn’t loaded (or the class names/styles change), the sign-in button styling will break. Consider adding chat-owned styling (new CSS class) or explicitly importing the required stylesheet so the dependency is intentional and stable.
const content = dom.append(container, dom.$('.update-indicator.prominent'));
content.setAttribute('role', 'button');
content.setAttribute('aria-label', this.action.label);
const label = dom.append(content, dom.$('.indicator-label'));
label.textContent = this.action.label;
| // Signed out | ||
| else if (this.chatEntitlementService.entitlement === ChatEntitlement.Unknown) { | ||
| const signedOutWarning = localize('notSignedIn', "Signed out"); | ||
| const signedOutWarning = localize('notSignedIn', "Sign In"); |
There was a problem hiding this comment.
The localization key notSignedIn previously corresponded to “Signed out”, but now maps to “Sign In”. Reusing the same key will keep existing translations for “Signed out” in non-English builds, resulting in incorrect UI text. Use a new localization key (e.g. signIn) when changing the meaning of a string so translators get updated source text.
| const signedOutWarning = localize('notSignedIn', "Sign In"); | |
| const signedOutWarning = localize('signIn', "Sign In"); |
| const content = dom.append(container, dom.$('.update-indicator.prominent')); | ||
| content.setAttribute('role', 'button'); | ||
| content.setAttribute('aria-label', this.action.label); |
There was a problem hiding this comment.
SignInTitleBarEntry.render sets role="button" and aria-label on the inner .update-indicator element, but the actual actionable/focusable element is the action view item container (BaseActionViewItem wires events/focus on container). This can lead to confusing semantics for assistive tech (nested button-like element that isn’t focusable). Prefer applying accessible labeling/role to the view item container (or follow the update indicator pattern and avoid adding a separate role on a child element).
This issue also appears on line 841 of the same file.
| const content = dom.append(container, dom.$('.update-indicator.prominent')); | |
| content.setAttribute('role', 'button'); | |
| content.setAttribute('aria-label', this.action.label); | |
| container.setAttribute('aria-label', this.action.label); | |
| const content = dom.append(container, dom.$('.update-indicator.prominent')); |
| this._register(actionViewItemService.register( | ||
| MenuId.TitleBarAdjacentCenter, | ||
| 'workbench.action.chat.signInIndicator', | ||
| (action, options) => new SignInTitleBarEntry(action, options) |
There was a problem hiding this comment.
Avoid duplicating the action id string; ChatSetupSignInTitleBarAction already defines static readonly ID. Reusing the constant here prevents future mismatches if the id changes.
| class SignInTitleBarEntry extends BaseActionViewItem { | ||
|
|
||
| constructor( | ||
| action: IAction, | ||
| options: IBaseActionViewItemOptions, | ||
| ) { | ||
| super(undefined, action, options); | ||
| } | ||
|
|
||
| public override render(container: HTMLElement) { | ||
| super.render(container); | ||
|
|
||
| container.setAttribute('aria-label', this.action.label); | ||
|
|
||
| const content = dom.append(container, dom.$('.update-indicator.prominent')); | ||
| const label = dom.append(content, dom.$('.indicator-label')); | ||
| label.textContent = this.action.label; | ||
| } |
There was a problem hiding this comment.
SignInTitleBarEntry extends BaseActionViewItem but doesn’t override updateLabel / updateEnabled. Since BaseActionViewItem wires onDidChange and calls these methods, label/enablement changes won’t be reflected and the UI may stay clickable even if the action becomes disabled. Consider implementing these overrides (and storing the created label/content elements) or extending an action view item class that already handles label/disabled rendering, similar to UpdateTitleBarEntry.
| const content = dom.append(container, dom.$('.update-indicator.prominent')); | ||
| const label = dom.append(content, dom.$('.indicator-label')); | ||
| label.textContent = this.action.label; |
There was a problem hiding this comment.
This title bar entry reuses the .update-indicator/.indicator-label styling classes, but those styles are defined in workbench/contrib/update CSS (updateTitleBarEntry.css) and aren’t imported here. That creates a hidden cross-feature dependency and risks unstyled UI if the update contribution/CSS changes or isn’t loaded. Consider adding chat-owned styling (or explicitly importing the required CSS in this feature) instead of relying on update’s internal classes.
| class ChatSetupSignInTitleBarAction extends Action2 { | ||
|
|
||
| static readonly ID = SIGN_IN_TITLE_BAR_ACTION_ID; | ||
|
|
||
| constructor() { | ||
| super({ | ||
| id: ChatSetupSignInTitleBarAction.ID, | ||
| title: localize('signInIndicatorTitleBarAction', 'Sign In'), | ||
| f1: false, | ||
| menu: [{ | ||
| id: MenuId.TitleBarAdjacentCenter, | ||
| order: 0, // same position as the update button | ||
| when: ContextKeyExpr.and( | ||
| ContextKeyExpr.has(`config.${ChatConfiguration.SignInTitleBarEnabled}`), | ||
| ChatContextKeys.Entitlement.signedOut, | ||
| ChatContextKeys.Setup.hidden.negate(), | ||
| ContextKeyExpr.has('updateTitleBar').negate() | ||
| ), | ||
| }] | ||
| }); |
There was a problem hiding this comment.
ChatSetupSignInTitleBarAction is registered unconditionally, but the associated view-item registration is gated to Electron-only (isWeb early return). In web builds, this action can still contribute a menu item to MenuId.TitleBarAdjacentCenter (with default rendering) even though the feature is intended to be Electron-only. Consider skipping registerAction2(ChatSetupSignInTitleBarAction) (or gating the menu when) when isWeb is true so the action doesn’t surface in web.
Adds setting for the following experimental behavior: