Don't depend on sessionResource in a few places#299110
Conversation
Removes some antipaterns and unused variables.
There was a problem hiding this comment.
Pull request overview
This PR removes an anti-pattern where widget state (dynamic variables and tool selections) was fetched via sessionResource-based service lookups (IChatVariablesService.getDynamicVariables(sessionResource)) in places that already had a direct IChatWidget reference. It introduces helper functions getDynamicVariablesForWidget and getSelectedToolAndToolSetsForWidget that work directly with a widget, and adds parseChatRequestWithReferences to ChatRequestParser to accept pre-fetched data instead of going through the service.
Changes:
- Extracts
getDynamicVariablesForWidgetandgetSelectedToolAndToolSetsForWidgetas standalone exported helper functions inchatVariables.ts, and refactors existing call sites to use them directly - Adds
parseChatRequestWithReferencestoChatRequestParseraccepting pre-computed variable/tool data, withparseChatRequestnow delegating to it; removes unusedsessionResourceparameter fromChatInputPart.getAttachedContextandgetAttachedAndImplicitContext - Adds unit tests for the new helper functions
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
chatVariables.ts (browser) |
Extracts getDynamicVariablesForWidget and getSelectedToolAndToolSetsForWidget as public functions; service methods now delegate to them |
chatRequestParser.ts |
Adds parseChatRequestWithReferences accepting direct data; original parseChatRequest delegates to it |
chatInputPart.ts |
Removes unused sessionResource parameter from getAttachedContext and getAttachedAndImplicitContext |
chatWidget.ts |
Updates two parseChatRequest calls and getAttachedContext/getAttachedAndImplicitContext calls |
chatInputEditorContrib.ts |
Uses parseChatRequestWithReferences with direct widget data |
chatPasteProviders.ts |
Removes IChatVariablesService dependency; uses getDynamicVariablesForWidget |
chatContinueInAction.ts |
Updates to use parseChatRequestWithReferences and parameterless context methods |
mainThreadChatAgents2.ts |
Updates to use parseChatRequestWithReferences |
chatVariables.test.ts |
New test file for getDynamicVariablesForWidget and getSelectedToolAndToolSetsForWidget |
Comments suppressed due to low confidence (1)
src/vs/workbench/contrib/chat/test/browser/attachments/chatVariables.test.ts:163
- The test
sets isDirectory for directory attachmentsuses multipleassert.strictEqualcalls to verify different properties of the result. Per the project's coding guidelines, prefer a singleassert.deepStrictEqualover multiple precise assertions when checking properties of the same object.
assert.strictEqual(result.length, 1);
assert.strictEqual(result[0].isFile, false);
assert.strictEqual(result[0].isDirectory, true);
You can also share your feedback on Copilot code review. Take the survey.
src/vs/workbench/contrib/chat/test/browser/attachments/chatVariables.test.ts
Show resolved
Hide resolved
src/vs/workbench/contrib/chat/browser/widget/input/editor/chatInputEditorContrib.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Removes some antipaterns and unused variables.