refactor: streamline session option group selection logic and improve test coverage#308743
refactor: streamline session option group selection logic and improve test coverage#308743DonJayamanne wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors Copilot CLI session option-group selection behavior (repo/branch/isolation) to produce more consistent defaults—especially in welcome-view (MRU) scenarios—and expands unit test coverage around these flows. It also starts returning locked option values for existing sessions so the session UI can reflect the resolved/locked selections.
Changes:
- Auto-select the first MRU repository in welcome view (and fall back to the first item when a previous selection is no longer present).
- When branch selection is locked (workspace isolation), ignore any previous branch selection and snap back to HEAD; adjust locked branch group items accordingly.
- For existing sessions, populate
ChatSession.optionswith locked option values derived from the existing-session input-state groups.
Show a summary per file
| File | Description |
|---|---|
| extensions/copilot/src/extension/chatSessions/vscode-node/test/sessionOptionGroupBuilder.spec.ts | Updates/extends tests to cover new selection/locking behavior for MRU repo + branch groups. |
| extensions/copilot/src/extension/chatSessions/vscode-node/sessionOptionGroupBuilder.ts | Refines welcome-view repository selection and locked-branch selection behavior. |
| extensions/copilot/src/extension/chatSessions/vscode-node/copilotCLIChatSessions.ts | Adds locked option values to existing session content and removes an unused no-op handler. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 2
| expect(result!.items[0].locked).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('resets to HEAD branch when locked with workspace isolatuion even if previous selection was different', () => { |
There was a problem hiding this comment.
Test name has a spelling typo: "isolatuion" → "isolation". This shows up in test output and makes the intent harder to read/search.
| it('resets to HEAD branch when locked with workspace isolatuion even if previous selection was different', () => { | |
| it('resets to HEAD branch when locked with workspace isolation even if previous selection was different', () => { |
| const [history, title, optionGroups] = await Promise.all([ | ||
| this.getSessionHistory(copilotcliSessionId, folderRepo, token), | ||
| this.customSessionTitleService.getCustomSessionTitle(copilotcliSessionId), | ||
| this._optionGroupBuilder.buildExistingSessionInputStateGroups(resource, token), |
There was a problem hiding this comment.
provideChatSessionContentForExistingSession now calls folderRepositoryManager.getFolderRepository(...), but buildExistingSessionInputStateGroups(...) also calls getFolderRepository internally. This duplicates the same lookup on every session open; consider plumbing the already-fetched folder/repo info into the option-group builder (or splitting out a helper that takes the resolved folderRepo) to avoid extra IO/work.
| this._optionGroupBuilder.buildExistingSessionInputStateGroups(resource, token), | |
| this._optionGroupBuilder.buildExistingSessionInputStateGroups(resource, token, folderRepo), |
No description provided.