Conversation
There was a problem hiding this comment.
Pull request overview
This PR integrates AWS Bedrock as an alternative AI model provider alongside GitHub Copilot, allowing users to choose between different language models. The implementation introduces a factory pattern for model selection and adds UI indicators for ONTAP login status.
Key changes:
- Created a model factory pattern to support multiple AI providers (Copilot and Bedrock)
- Added visual indicators in the file system tree to show which filesystems have ONTAP login details configured
- Introduced new configuration options for Bedrock model selection and region
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/extension.ts | Added new command registration for updating ONTAP login details and passed refresh callback to addOntapLoginDetails function |
| src/copilot_herlper.ts | Refactored to use the new model factory pattern, replacing direct vscode.LanguageModelChat calls with abstracted Model interface |
| src/commands.ts | Updated addOntapLoginDetails to accept a refresh callback parameter and invoke it after successful credential storage |
| src/chat/models/copilot.ts | New file implementing the Model interface for GitHub Copilot integration |
| src/chat/models/bedrock.ts | New file implementing the Model interface for AWS Bedrock integration |
| src/chat/modelFactory.ts | New file defining the Model interface and factory function for selecting between AI providers |
| src/TreeItems.ts | Enhanced FileSystemsItem to track and display ONTAP login details status with different icons |
| src/FileSystemsTree.ts | Modified to check for stored credentials and pass login status when creating FileSystemsItem instances |
| package.json | Added Bedrock configuration options, new command definitions, and AWS SDK dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { executeOntapCommands, OntapExecutorResult } from './ontap_executor'; | ||
| import { state } from './state'; | ||
| import { copilot_filesystem_question, copilot_question } from './telemetryReporter'; | ||
| import { getModel } from './chat/modelFactory'; |
There was a problem hiding this comment.
Corrected spelling of 'herlper' to 'helper' in filename.
| const content = [{ | ||
| role: ConversationRole.USER, |
There was a problem hiding this comment.
The messageType parameter is not being used. The role is hardcoded to ConversationRole.USER, but the function receives a messageType parameter that should determine whether the role is 'user' or 'assistant'. This inconsistency means all messages will be sent as user messages regardless of the intended role.
| const content = [{ | |
| role: ConversationRole.USER, | |
| // Map MessageType to ConversationRole | |
| const role = messageType === "user" ? ConversationRole.USER : ConversationRole.ASSISTANT; | |
| const content = [{ | |
| role: role, |
| await model.sendRequest([testMessage], {}, new vscode.CancellationTokenSource().token); | ||
| return model; |
There was a problem hiding this comment.
A new CancellationTokenSource is created but never disposed. This could lead to resource leaks. Consider storing the token source in a variable and calling dispose() after the request, or reuse the existing token passed to the constructor.
| await model.sendRequest([testMessage], {}, new vscode.CancellationTokenSource().token); | |
| return model; | |
| const cts = new vscode.CancellationTokenSource(); | |
| try { | |
| await model.sendRequest([testMessage], {}, cts.token); | |
| return model; | |
| } finally { | |
| cts.dispose(); | |
| } |
| return Promise.all(fileSystems.map(async fs => { | ||
| const name = fs.Tags?.find(tag => tag.Key === 'Name')?.Value || fs.FileSystemId; | ||
| const fsItem = new FileSystemsItem(name || '', fs.FileSystemId || '', fs, element.id || 'us-east-1', vscode.TreeItemCollapsibleState.Collapsed); | ||
| const sshInfoStr = await state.context.secrets.get(`sshKey-${fs.FileSystemId}-${element.id || 'us-east-1'}`); |
There was a problem hiding this comment.
The secrets.get call is made inside a map operation for each filesystem, resulting in N sequential async calls. Consider batching these secret retrievals or caching them to improve performance when rendering many filesystems.
| const bedrockModel = vscode.workspace.getConfiguration('netapp-fsx-ontap').get('bedrockmodel',""); | ||
| if (bedrockModel) { | ||
| const model = new BedrockModel(bedrockModel as string); |
There was a problem hiding this comment.
The empty string default for bedrockmodel is used as a boolean check on line 20. This is unclear - consider using a null/undefined default or explicitly checking for empty string to make the intent clearer.
| const bedrockModel = vscode.workspace.getConfiguration('netapp-fsx-ontap').get('bedrockmodel',""); | |
| if (bedrockModel) { | |
| const model = new BedrockModel(bedrockModel as string); | |
| const bedrockModel = vscode.workspace.getConfiguration('netapp-fsx-ontap').get<string>('bedrockmodel', undefined); | |
| if (typeof bedrockModel === 'string' && bedrockModel.trim() !== "") { | |
| const model = new BedrockModel(bedrockModel); |
No description provided.