Add LLM-Based Recommendations for Broken Pipeline Runs #57
Conversation
…I keys with the VSCode SecretsStorage API
…ggest Fix is clicked, then closes when the document opens
… OpenAI Co-authored-by: Morgan-Summer-Davis <Morgan-Summer-Davis@users.noreply.github.com>
…de changes directly
…n response Co-authored-by: Morgan Davis <Morgan-Summer-Davis@users.noreply.github.com>
…s located in it, and applies the suggested edit to it
…er, rather than a custom solution
merge in getFileByContent search function
add typescript types for ArtifactData and StepData
…de-zenml into prompt-engineering
…st arguments from user choices when choosing an LLM model, add getter methods for supported providers/models, public method to setModel
…ndefined when the key was not found
WalkthroughThe pull request introduces significant enhancements to a Visual Studio Code extension focused on AI interactions. Key changes include a transition to an array-based configuration format, the addition of AI model selection capabilities, and the implementation of new commands for managing AI suggestions. A custom file system provider is established to facilitate these interactions, while new dependencies support the expanded functionality. Changes
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 39
🧹 Outside diff range and nitpick comments (21)
src/commands/ai/cmds.ts (1)
14-17: Consider reordering imports for consistency.While the imports are correct, it's a good practice to group them logically. Consider reordering them as follows:
- External libraries (vscode)
- Internal modules (SaveAIChangeEmitter and AIStepFixer)
This improves readability and follows common TypeScript conventions.
-import { SaveAIChangeEmitter } from '../pipelines/StepFixerFs'; - import * as vscode from 'vscode'; + +import { SaveAIChangeEmitter } from '../pipelines/StepFixerFs'; import AIStepFixer from '../pipelines/AIStepFixer';src/commands/secrets/registry.ts (3)
14-17: LGTM: Imports are appropriate and well-organized.The necessary modules are imported correctly. Consider grouping imports by their source (e.g., third-party libraries, local modules) for better readability.
You could organize imports like this:
import { ExtensionContext, commands } from 'vscode'; import { registerCommand } from '../../common/vscodeapi'; import { ZenExtension } from '../../services/ZenExtension'; import { secretsCommands } from './cmds';
19-23: LGTM: Well-documented function with clear JSDoc comments.The function documentation is clear and informative. To further improve it, consider adding a @returns tag to explicitly state that the function doesn't return a value (void).
You could enhance the documentation like this:
/** * Registers secrets related commands for the extension. * * @param {ExtensionContext} context - The context in which the extension operates, used for registering commands and managing their lifecycle. * @returns {void} */
24-43: LGTM: Well-structured function with proper error handling.The implementation is solid, with good use of try-catch and context management. Here are a few suggestions for improvement:
- Consider using a more descriptive variable name than
cmdin the forEach loop.- The error logging could be more informative by including the error message.
- You might want to consider returning a boolean to indicate success or failure.
Here's a suggested refactoring:
export const registerSecretsCommands = (context: ExtensionContext): boolean => { try { const registeredCommands = [ registerCommand( 'zenml.registerLLMAPIKey', async () => await secretsCommands.registerLLMAPIKey(context) ), ]; registeredCommands.forEach(command => { context.subscriptions.push(command); ZenExtension.commandDisposables.push(command); }); commands.executeCommand('setContext', 'secretsCommandsRegistered', true); return true; } catch (error) { console.error('Error registering secrets commands:', error instanceof Error ? error.message : String(error)); commands.executeCommand('setContext', 'secretsCommandsRegistered', false); return false; } };This refactoring improves error logging, uses a more descriptive variable name, and returns a boolean to indicate success or failure.
src/commands/ai/registry.ts (2)
14-15: Refine TODO comments and consider separation of concerns.The TODO comments suggest including secret command registration in this file. However, this might lead to mixing concerns. Consider the following:
- Create a separate file for secret-related command registration to maintain a clear separation of concerns.
- Refine the TODO comments to include specific action items or reference relevant issue tickets.
- If the TODO is addressing an immediate concern, consider implementing it now rather than leaving it as a comment.
17-27: LGTM: Imports and function signature are appropriate.The imports, function signature, and JSDoc comment are well-structured. A minor suggestion to improve the JSDoc:
Consider adding a
@returnstag to the JSDoc comment to explicitly state that the function doesn't return a value (void). This enhances the documentation's completeness:/** * Registers GenAI related commands for the extension. * * @param {ExtensionContext} context - The context in which the extension operates, used for registering commands and managing their lifecycle. * @returns {void} */src/common/vscodeapi.ts (1)
74-83: Good implementation, with room for minor improvementsThe
getSecretfunction is well-implemented and follows the single responsibility principle. However, there are a few suggestions to enhance it:
Add an explicit return type for better type safety:
export async function getSecret(context: ExtensionContext, key: string): Promise<string | undefined> { // ... existing implementation }Consider a more flexible error handling approach. Instead of logging to the console, you might want to throw an error or return a result object that includes both the secret and an error status. This would allow the caller to decide how to handle the error.
Minor refactoring for improved readability:
export async function getSecret(context: ExtensionContext, key: string): Promise<string | undefined> { const secret = await context.secrets.get(key); if (secret === undefined) { console.error(`The requested secret with key '${key}' does not exist.`); } return secret; }This refactoring removes the unnecessary
return undefinedstatement, asundefinedis implicitly returned whensecretisundefined.src/types/PipelineTypes.ts (2)
36-53: LGTM with a suggestion for improvementThe
StepDatainterface is well-structured and follows TypeScript naming conventions. It provides a comprehensive set of properties to describe a pipeline step. However, consider the following suggestion:Consider changing the
sourceCodeproperty to use a more appropriate type for potentially large code blocks. For example:sourceCode: { content: string; language?: string; size?: number; };This structure would allow for better handling of large code snippets and provide additional metadata about the source code.
55-78: LGTM with suggestions for enhancementThe
ArtifactDatatype is well-structured and provides a comprehensive representation of artifact data. Here are some suggestions for further improvement:
- Consider making the
shapeproperty more flexible to accommodate multi-dimensional data:shape: number[];
- If possible, define a more specific type for
dtypeto improve type safety:dtype: Record<string, 'int' | 'float' | 'string' | 'boolean'>;
- Add optional properties for version control information:
versionControl?: { commitHash?: string; branch?: string; repository?: string; };These changes would make the
ArtifactDatatype more robust and flexible for various use cases.resources/dag-view/dag.css (1)
166-169: Consider adding some padding to the ul element.While removing the default list styling is good, consider adding a small padding to the
ulelement to prevent menu items from touching the edge of the menu.You could add padding like this:
#context-menu ul { list-style: none; - padding: 0px; + padding: 5px 0; }package.json (1)
66-162: LGTM! Configuration structure improved and LLM model selection added.The new configuration structure improves organization and the addition of LLM model selection aligns well with the PR objectives. The variety of models from different providers offers good flexibility to users.
Consider adding a description for the
zenml.llm-modelproperty to provide users with context about its purpose and usage."zenml.llm-model": { + "description": "Select the LLM model to use for generating recommendations for broken pipeline runs.", "enum": [ "anthropic.claude-3-5-sonnet-20240620", "anthropic.claude-3-opus-20240229", ... ] }src/commands/pipelines/StepFixerFs.ts (3)
140-142: Remove unusedMinFSErrorconstant.After updating the unimplemented methods to throw specific
FileSystemErrorinstances, theMinFSErrorconstant is no longer used and can be removed to clean up the code.Apply this diff to remove the unused constant:
-const MinFSError = new Error( - 'Invalid operation attempted by minimal file system which only supports basic reading and writing to files' -);
145-160: Explicitly declare access modifiers in theFileclass.For better code clarity and to align with TypeScript best practices, explicitly declare access modifiers (
public,private, orprotected) for class properties. This makes the code more readable and maintainable.Apply this diff to add access modifiers:
class File implements vscode.FileStat { - type: vscode.FileType; - ctime: number; - mtime: number; - size: number; - name: string; - data?: Uint8Array; + public type: vscode.FileType; + public ctime: number; + public mtime: number; + public size: number; + public name: string; + public data?: Uint8Array; constructor(name: string) { // ... } }
162-179: Explicitly declare access modifiers in theDirectoryclass.Similarly, adding access modifiers to the
Directoryclass properties enhances code readability and follows best practices.Apply this diff to add access modifiers:
class Directory implements vscode.FileStat { - type: vscode.FileType; - ctime: number; - mtime: number; - size: number; - name: string; - entries: Map<string, File | Directory>; + public type: vscode.FileType; + public ctime: number; + public mtime: number; + public size: number; + public name: string; + public entries: Map<string, File | Directory>; constructor(name: string) { // ... } }src/common/utilities.ts (3)
145-146: Address the TODO comments in the codeThere are TODO comments indicating pending enhancements:
- Line 119: Ensure that when multiple copies of the file are present in the git log, the function returns only one copy.
- Lines 145-146: Modify the function to search for the nearest git repository from each root, rather than assuming the repository is in the root workspace directory.
Do you want me to help implement these improvements or open a GitHub issue to track these tasks?
Also applies to: 119-119
151-181: Improve variable naming for better readabilityIn the
searchGitCacheByFileContentfunction, some variable names may not accurately reflect their contents, which can make the code harder to understand:
- The variable
matchingLinesactually holds file paths, not lines.- The variable
linesrepresents the split content lines for searching.Consider renaming variables for clarity:
- Rename
matchingLinestomatchingFilePathsorcurrentMatchingFilePaths.- Rename
linestocontentLinesorsearchLines.This will make the code more readable and maintainable.
93-117: Consider adding unit tests forfindFirstLineNumberThe
findFirstLineNumberfunction is non-trivial and could benefit from unit tests to verify its correctness, especially with various edge cases like empty strings, no matches, and multi-line substrings.Would you like assistance in creating unit tests for this function to ensure its reliability?
src/services/aiService.ts (2)
113-132: Improve user experience when no AI model is selectedWhen no AI model is selected, the method prompts the user to select one but also shows an error message. This might be confusing. Consider guiding the user through the selection process without showing an error.
You could refactor the code to prompt the user to select an AI model and return early without showing an error message:
if (!this.provider || !this.model) { await AIStepFixer.getInstance().selectLLM(); - vscode.window.showErrorMessage( - `No AI model selected. Please select one through the command palette above and try again.` - ); return; }
141-157: Clarify the assistant's prompt messagesThe prompt provided to the AI assistant could be adjusted for clarity and conciseness. Ensuring clear instructions can lead to better responses from the AI.
Consider simplifying and clarifying as follows:
content: `You are an advanced AI programming assistant tasked with troubleshooting ZenML pipeline runs. Construct an explanation that: - Focuses on the possible causes of the error, explaining the 'why' behind it. - Avoids making assumptions or adding unsupported details. - Provides the full corrected source code if proposing changes. - Responds using Markdown syntax only.`,src/commands/pipelines/DagRender.ts (2)
131-145: Consider removing unnecessary null checks forWebviewBase.contextIn the new cases added (
'stepFix','selectLLM','getLLM'), the checkif (!WebviewBase.context) return;might be unnecessary becauseWebviewBase.contextis already validated in the constructor where an error is thrown if it'snull. Removing these redundant checks can simplify the code.Here’s the suggested change:
case 'stepFix': - if (!WebviewBase.context) return; AIStepFixer.getInstance().suggestFixForStep(message.id, node); break; case 'selectLLM': - if (!WebviewBase.context) return; AIStepFixer.getInstance().selectLLM(); break; case 'getLLM': - if (!WebviewBase.context) return; panel.webview.postMessage( `model: ${AIService.getInstance(WebviewBase.context).model || 'none'}` ); break;
181-181: Remove debuggingconsole.logstatementThe
console.log('artifact data', artifactData);statement appears to be for debugging purposes. Consider removing it to keep the console output clean.Apply this diff to remove the console log:
); - console.log('artifact data', artifactData);
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
🔇 Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
- package.json (4 hunks)
- resources/dag-view/dag.css (1 hunks)
- resources/dag-view/dag.js (4 hunks)
- resources/dag-view/icons/1487.gif:Zone.Identifier (1 hunks)
- src/commands/ai/cmds.ts (1 hunks)
- src/commands/ai/registry.ts (1 hunks)
- src/commands/pipelines/AIStepFixer.ts (1 hunks)
- src/commands/pipelines/DagRender.ts (4 hunks)
- src/commands/pipelines/MultiStepInput.ts (1 hunks)
- src/commands/pipelines/StepFixerFs.ts (1 hunks)
- src/commands/secrets/cmds.ts (1 hunks)
- src/commands/secrets/registry.ts (1 hunks)
- src/common/utilities.ts (2 hunks)
- src/common/vscodeapi.ts (2 hunks)
- src/services/ZenExtension.ts (3 hunks)
- src/services/aiService.ts (1 hunks)
- src/types/PipelineTypes.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- resources/dag-view/icons/1487.gif:Zone.Identifier
🧰 Additional context used
📓 Path-based instructions (14)
resources/dag-view/dag.js (1)
Pattern
**/*.js: Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations.src/commands/ai/cmds.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.src/commands/ai/registry.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.src/commands/pipelines/AIStepFixer.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.src/commands/pipelines/DagRender.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.src/commands/pipelines/MultiStepInput.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.src/commands/pipelines/StepFixerFs.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.src/commands/secrets/cmds.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.src/commands/secrets/registry.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.src/common/utilities.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.src/common/vscodeapi.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.src/services/ZenExtension.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.src/services/aiService.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.src/types/PipelineTypes.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
Biome
resources/dag-view/dag.js
[error] 159-159: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 160-160: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 164-164: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 165-165: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
src/commands/pipelines/MultiStepInput.ts
[error] 2-7: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 9-9: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 46-46: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments not posted (17)
src/commands/ai/cmds.ts (2)
1-12: LGTM: Copyright and license information is correct.The copyright notice and Apache License 2.0 terms are properly included at the beginning of the file.
37-40: LGTM: Exported object is correctly implemented.The
aiCommandsobject is properly exported, making the functions available for use in other parts of the application. This approach allows for easy import and use of these commands elsewhere in the codebase.src/commands/secrets/registry.ts (1)
1-12: LGTM: Appropriate license and copyright notice.The Apache License 2.0 notice is correctly included with up-to-date copyright information.
src/commands/ai/registry.ts (1)
1-12: LGTM: Proper license and copyright notice.The file includes the correct Apache 2.0 license information and copyright notice.
src/commands/secrets/cmds.ts (1)
1-15: LGTM: File header and imports are appropriate.The copyright notice, license information, and import statements are correctly formatted and suitable for the file's purpose.
src/common/vscodeapi.ts (1)
21-21: Import change approvedThe addition of
ExtensionContextto the import list is correct and necessary for the newgetSecretfunction. This change ensures that theExtensionContexttype is available for use in the function signature.resources/dag-view/dag.css (2)
159-164: LGTM: Context menu styling looks good.The styling for the context menu is well-defined. The absolute positioning, white background, box-shadow, and rounded corners create a visually distinct and appealing menu.
171-176: LGTM: Menu item styling is well-defined.The styling for individual menu items is appropriate. The font weight, size, padding, and cursor style create a clear and interactive appearance for the menu items.
src/services/ZenExtension.ts (3)
20-20: LGTM: New import for secrets commands.The import for
registerSecretsCommandsis consistent with the existing pattern and naming convention. It's properly organized within the codebase structure.
47-47: LGTM: New import for AI commands aligns with PR objectives.The import for
registerAICommandsis consistent with the existing pattern and naming convention. It's properly organized within the codebase structure and aligns with the PR objective of introducing AI-related functionality.
78-79: LGTM: New command registrations added correctly.The additions of
registerSecretsCommandsandregisterAICommandsto theregistriesarray are consistent with the existing pattern. These changes ensure that the new command sets for secrets management and AI features will be properly registered, aligning with the PR objectives.package.json (4)
321-343: LGTM! New commands added for LLM integration and code recommendations.The new commands align well with the PR objectives of integrating LLM-based recommendations. The "when" conditions for the code recommendation commands ensure they're only available in the appropriate context, which is a good practice.
516-527: LGTM! Editor title menu updated with code recommendation actions.The new menu items for displaying the next code recommendation and accepting code recommendations are well-integrated. The "when" conditions ensure these actions are only available in the appropriate context, which is a good user experience design.
Line range hint
1-569: Overall, excellent implementation of LLM-based recommendations feature.The changes in this file comprehensively support the new LLM-based recommendation feature for broken pipeline runs. The modifications to the configuration, commands, menus, and dependencies all align well with the PR objectives. The implementation appears well-structured and consistent.
563-566: New dependencies added, but clarification needed.The addition of "simple-git" aligns with the PR objective of searching for step code in the nearest git repository. However, the purpose of "token.js" is not clear from the context provided.
Could you please clarify the purpose of adding the "token.js" dependency? How is it used in the context of this PR?
✅ Verification successful
'token.js' dependency usage confirmed.
The
token.jsdependency is utilized insrc/services/aiService.ts:import { TokenJS } from 'token.js';🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of token.js in the codebase rg --type typescript 'token\.js'Length of output: 69
Script:
#!/bin/bash # Search for usage of token.js in the codebase without specifying file type rg 'token\.js'Length of output: 749
src/commands/pipelines/StepFixerFs.ts (1)
1-181: Ensure consistent event firing after file operations.In the
writeFilemethod, events are fired using_fireSoon, and in thereadFilemethod, an event is fired when a file is created or changed. Verify that all file operations that modify the file system state appropriately fire events so that any observers are correctly notified.Run the following script to check event firing consistency:
✅ Verification successful
Event firing in file operations is consistent.
All file system modifications appropriately trigger
_fireSoonevents.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for invocations of `_fireSoon` in file system methods. # Test: Ensure that all methods modifying the file system state call `_fireSoon`. # Expected: Methods like `writeFile`, `createDirectory`, `delete`, etc., should invoke `_fireSoon`. rg --type ts '_fireSoon' src/commands/pipelines/StepFixerFs.ts -A 5 -B 5Length of output: 1178
src/services/aiService.ts (1)
85-86: Ensure error messages do not expose sensitive informationThe error message reveals the provider's name, which could be sensitive. It's best practice to avoid exposing any potentially sensitive information in error messages presented to the user.
[security]
Consider revising the error message:
-throw new Error( - `No ${this.provider} API key configured. Please add an environment variable or save a key through the command palette above and try again.` -); +throw new Error( + `No API key configured for the selected AI provider. Please set the API key through environment variables or save it securely via the command palette and try again.` +);
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/commands/pipelines/MultiStepInput.ts (2)
73-131: LGTM with a minor suggestion: Robust showQuickPick implementationThe
showQuickPickmethod is well-implemented, providing a comprehensive quick pick experience with proper handling of button triggers, selection changes, and input hiding. The use of disposables for cleanup is a good practice.However, there's a minor type safety concern:
Consider improving type safety in the button trigger handler:
- resolve(<any>item); + resolve(item as P extends { buttons: (infer I)[] } ? I : never);This change aligns the resolved type with the return type of the promise, enhancing type safety.
133-208: LGTM with a minor suggestion: Comprehensive showInputBox implementationThe
showInputBoxmethod is well-implemented, providing a robust input box experience with proper handling of button triggers, value acceptance, value changes, and input hiding. The use of disposables for cleanup and the implementation of real-time validation are good practices.However, there's a minor type safety concern:
Consider improving type safety in the button trigger handler:
- resolve(<any>item); + resolve(item as P extends { buttons: (infer I)[] } ? I : never);This change aligns the resolved type with the return type of the promise, enhancing type safety.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
- package.json (5 hunks)
- resources/dag-view/dag.css (1 hunks)
- resources/dag-view/dag.js (4 hunks)
- src/commands/ai/cmds.ts (1 hunks)
- src/commands/pipelines/AIStepFixer.ts (1 hunks)
- src/commands/pipelines/DagRender.ts (4 hunks)
- src/commands/pipelines/MultiStepInput.ts (1 hunks)
- src/commands/pipelines/StepFixerFs.ts (1 hunks)
- src/commands/secrets/cmds.ts (1 hunks)
- src/common/utilities.ts (2 hunks)
- src/services/aiService.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- package.json
- resources/dag-view/dag.css
- resources/dag-view/dag.js
- src/commands/ai/cmds.ts
- src/commands/pipelines/AIStepFixer.ts
- src/commands/pipelines/DagRender.ts
- src/commands/pipelines/StepFixerFs.ts
- src/commands/secrets/cmds.ts
- src/common/utilities.ts
- src/services/aiService.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/commands/pipelines/MultiStepInput.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
🔇 Additional comments (5)
src/commands/pipelines/MultiStepInput.ts (5)
3-7: LGTM: Efficient implementation of InputFlowActionThe
InputFlowActionobject is well-implemented using symbols. This approach addresses the previous review comment and provides a clean, efficient way to represent unique action identifiers.
9-9: LGTM: Correct use of undefined in InputStep typeThe
InputSteptype alias correctly usesundefinedinstead ofvoidin the union type, addressing the previous review comment. This provides better clarity on the possible return types of the input step function.
11-21: LGTM: Well-structured QuickPickParameters interfaceThe
QuickPickParametersinterface is well-defined and provides a clear structure for the parameters used in theshowQuickPickmethod. It extendsvscode.QuickPickItemappropriately and includes all necessary properties for a comprehensive quick pick implementation.
23-34: LGTM: Comprehensive InputBoxParameters interfaceThe
InputBoxParametersinterface is well-defined and provides a clear structure for the parameters used in theshowInputBoxmethod. It includes all necessary properties for a robust input box implementation, including validation and resume functionality.
36-71: LGTM: Well-structured MultiStepInput classThe
MultiStepInputclass is well-designed with a clear separation of concerns:
- The static
runmethod provides a convenient entry point for initiating the multi-step input process.- The private
stepThroughmethod handles the core logic of stepping through input steps, including error handling for back, resume, and cancel actions.- The use of a stack (
stepsarray) for managing the input flow is an effective approach for supporting back navigation.The implementation effectively manages the state of the current input and handles various flow control scenarios.
…torage to single source of truth for API keys. TokenJS receives keys as a config option
… before inserting into the code
There was a problem hiding this comment.
A couple of nits attached, but two bigger issues I found while testing:
- registering an API key for Anthropic models never worked for me. I would get to the point where I selected Anthropic and then it would immediately close down the box where I was meant to input the key. So something buggy there.
- I was also never able to test the code fixing since whenever I'd click to get the fixes, I'd get this error:
Happy to send over whatever logs etc you need to help you to reproduce, but this didn't work at all for me.
| "gemini.gemini-1.5-pro", | ||
| "gemini.gemini-1.5-flash", | ||
| "gemini.gemini-1.0-pro", | ||
| "openai.gpt-4o", |
There was a problem hiding this comment.
| "openai.gpt-4o", | |
| "openai.gpt-4o", | |
| "openai.gpt-4", |
| 'claude-3-haiku-20240307', | ||
| ] as const; | ||
| const supportedGeminiModels = ['gemini-1.5-pro', 'gemini-1.5-flash', 'gemini-1.0-pro'] as const; | ||
| const supportedOpenAIModels = ['gpt-4o', 'gpt-4o-mini', 'gpt-4-turbo', 'gpt-3.5-turbo'] as const; |
There was a problem hiding this comment.
| const supportedOpenAIModels = ['gpt-4o', 'gpt-4o-mini', 'gpt-4-turbo', 'gpt-3.5-turbo'] as const; | |
| const supportedOpenAIModels = ['gpt-4o', 'gpt-4', 'gpt-4o-mini', 'gpt-4-turbo', 'gpt-3.5-turbo'] as const; |
|
@sklarfox @Morgan-Summer-Davis some feedback. Happy to discuss more on Slack as needed. |
Summary
This pull request integrates with the DAG-visualization feature to allow users to request an LLM to suggest fixes for a broken step in one of their pipelines, including suggesting inline code changes to the step’s code, if local.
UI Changes
Adds a context-menu to the DAG-visualization webview that supports already-extant functionality, as well as a “Suggest Fix” option and one to select a default LLM for suggestions.
vscode.diffinterface. The intention is that it should look to the user as if they are live-editing the source file, but the changes are quarantined so as to not accidentally autosave over the user’s original code. Manually saving or clicking “Accept Changes” in the file header will overwrite the source code.Backend Changes
AIServicesingleton that supports Anthropic, Gemini, and OpenAI modelsfixMyPipelineRequestmethod which creates the LLM chat completion and returns a formatted response along with any python code snippets generated by the LLMsecretStorageAPI. Environment variables take precedence oversecretStorageSupportedLLMModelsandSupportedLLMProvidersSummary by CodeRabbit
New Features
Bug Fixes
Documentation