feat: add A/B test for default new session mode#306532
feat: add A/B test for default new session mode#306532pierceboggan merged 3 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a first-class setting (chat.newSession.defaultMode) whose default value is driven by a TAS treatment, and wires ChatInputPart to use this setting instead of the previous one-off experiment logic.
Changes:
- Add
ChatConfiguration.DefaultNewSessionModeand registerchat.newSession.defaultModedynamically viaChatAgentSettingContributionusing a TAS treatment. - Update
ChatInputPart._setEmptyModelStateto apply the configured default mode (including case-insensitive matching for custom mode names). - Remove the old experiment plumbing in
ChatInputPartand delete unused chat-mode validation helpers.
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/common/constants.ts | Adds new configuration key and removes unused chat mode validation helpers. |
| src/vs/workbench/contrib/chat/browser/widget/input/chatInputPart.ts | Switches default-mode selection from experiment-based to configuration-based and removes assignment service dependency. |
| src/vs/workbench/contrib/chat/browser/chat.contribution.ts | Dynamically registers the new setting with a TAS-driven default. |
andysharman
force-pushed
the
feature/default-new-session-mode-experiment
branch
2 times, most recently
from
5502c6d to
4882d82
Compare
src/vs/workbench/contrib/chat/browser/widget/input/chatInputPart.ts
Outdated
Show resolved
Hide resolved
| private registerDefaultModeSetting(): void { | ||
| this.experimentService.getTreatment<string>('chatDefaultNewSessionMode').then(value => { | ||
| const node: IConfigurationNode = { | ||
| id: 'chatSidebar', | ||
| title: nls.localize('interactiveSessionConfigurationTitle', "Chat"), | ||
| type: 'object', | ||
| properties: { | ||
| [ChatConfiguration.DefaultNewSessionMode]: { | ||
| type: 'string', | ||
| description: nls.localize('chat.newSession.defaultMode', "The default mode for new chat sessions. When empty, the chat view's default mode is used."), | ||
| default: typeof value === 'string' ? value : '', | ||
| } | ||
| } | ||
| }; | ||
| configurationRegistry.updateConfigurations({ add: [node], remove: [] }); | ||
| }); |
There was a problem hiding this comment.
ChatAgentSettingContribution is registered for WorkbenchPhase.AfterRestored, and registerDefaultModeSetting() waits on an async TAS treatment before registering the chat.newSession.defaultMode schema/default. If a chat widget/session is restored or created before this runs, _setEmptyModelState() may read undefined and miss applying the experiment-driven default for that first session. Consider registering this contribution earlier (e.g. WorkbenchPhase.BlockRestore) or ensuring the setting is registered synchronously with a default, then updating it when the treatment arrives.
There was a problem hiding this comment.
i think you can take this one if you want - i believe you can register the contribution in chat.contributions
Replace the temporary chat.defaultMode experiment with a proper configuration setting chat.newSession.defaultMode whose default is driven by TAS treatment chatDefaultNewSessionMode. - Add DefaultNewSessionMode to ChatConfiguration enum - Register setting dynamically via ChatAgentSettingContribution following the registerMaxRequestsSetting pattern - Rewrite _setEmptyModelState to sync read the config setting with case-insensitive custom mode name matching - Remove unused validateChatMode, isChatMode, and getDefaultModeExperimentStorageKey - Remove IWorkbenchAssignmentService from ChatInputPart (moved to ChatAgentSettingContribution) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
andysharman
force-pushed
the
feature/default-new-session-mode-experiment
branch
from
4882d82 to
74fcb3b
Compare
| [ChatConfiguration.DefaultNewSessionMode]: { | ||
| type: 'string', | ||
| description: nls.localize('chat.newSession.defaultMode', "The default mode for new chat sessions. When empty, the chat view's default mode is used."), | ||
| default: '', | ||
| }, |
There was a problem hiding this comment.
chat.newSession.defaultMode is registered in the static registerConfiguration block and then re-registered again via configurationRegistry.updateConfigurations() inside registerDefaultModeSetting(). While the later registration will override the schema, it also adds another configuration contributor and makes the registration path harder to reason about. Prefer registering the property schema once, and use configuration defaults overrides (e.g. configurationRegistry.registerDefaultConfigurations/deregisterDefaultConfigurations) or a tracked lastNode+remove pattern to update only the default value from TAS.
…-session-mode-experiment # Conflicts: # src/vs/workbench/contrib/chat/browser/widget/input/chatInputPart.ts
andysharman
force-pushed
the
feature/default-new-session-mode-experiment
branch
from
113062d to
669bdb0
Compare
Replace the temporary chat.defaultMode experiment with a proper configuration setting chat.newSession.defaultMode whose default is driven by TAS treatment chatDefaultNewSessionMode.