Chat with your pipelines functionality #56
Conversation
…I keys with the VSCode SecretsStorage API
Co-authored-by: Nathaniel Xu <[email protected]> Co-authored-by: Alan Cho <[email protected]>
Added functionality to the chat window in the sidebar (activity bar). User input is now sent to the Gemini API using TokenJS, and the response from the API is displayed in the chat window. Co-authored-by: Nathaniel Xu <[email protected]> Co-authored-by: Alan Cho <[email protected]>
Co-authored-by: Nathaniel-Xu <[email protected]> Co-authored-by: William Yennie <[email protected]>
Co-authored-by: Will Yennie <[email protected]> Co-authored-by: Alan Cho <[email protected]>
Co-authored-by: William Yennie <[email protected]> Co-authored-by: Nathaniel Xu <[email protected]>
Co-authored-by: William Yennie <[email protected]> Co-authored-by: Nathaniel Xu <[email protected]>
Integrated `marked` library to convert Markdown text to HTML. Updated `ChatViewProvider` to process and render Markdown in chat message. Modified `renderChatLog` to parse Markdown before displaying message.
…re files Renamed ChatViewProvider to ChatDataProvider for consistency with other files Moved ChatDataProvider class from extension.ts to src/activityBar/chatView Moved chat.js, chat.html, and chat.css to resources/chat-view
removed typescript error Co-authored-by: William Yennie <[email protected]>
Added missing semicolons and formatted code Fixed TypeScript linting issues with type checks and type guards Improved error handling and logging
…e/chatbot merge: feature/secrets into feature/chatbot Integrates updates from the `feature/secrets` branch in sklar-fork into `feature/chatbot`. Includes changes related to chatbot functionality improvements.
- Replaced Event constructor with CustomEvent for better compatibility in VSCode webview environment. - Wrapped case body in braces to prevent variable hoisting issues in switch-case statements.
- Added error handling, logging, and optional watch flag to build_css. - Removed the redundant dev function.
- Replaced `display: contents;` with `display: flex;` for the `<form>` element to maintain accessibility. - Changed hardcoded `color: black !important;` to `color: var(--vscode-editor-foreground);` for `ul` elements to ensure theme consistency. - Removed redundant color properties from `#chatMessages` child elements to allow inheritance and simplify the CSS.
|
@wyennie @alan-cho @Nathaniel-Xu A few things here...
Happy to dive more into this feedback on Slack etc as needed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
🧹 Outside diff range and nitpick comments (14)
resources/chat-view/chat.html (4)
37-37: Add accessibility attributes to loader elementThe loader element should have appropriate ARIA attributes for screen readers.
- <div id="loader" class="mb-4 mt-4"></div> + <div id="loader" class="mb-4 mt-4" role="status" aria-label="Loading response"></div>
55-65: Enhance SVG button accessibilityWhile the SVG has aria-label, the button itself lacks proper accessibility attributes.
<button type="submit" id="sendMessage" + aria-label="Send message" class="p-2 text-zenml hover:text-purple-700 focus:outline-none" >
72-82: Improve context dropdown accessibilityThe context button and dropdown need proper ARIA attributes for accessibility.
<button id="contextButton" class="context-button" + aria-expanded="false" + aria-controls="optionsDropdown" title="Send ZenML context with message" > Context </button> -<div id="optionsDropdown" class="context-dropdown"> +<div id="optionsDropdown" class="context-dropdown" role="menu" aria-labelledby="contextButton">
161-161: Remove commented out codeRemove commented out Tailwind configuration and script tag. If needed, move the configuration to a separate file.
Also applies to: 165-180
package.json (1)
431-435: Consider adding a condition for chat menu visibility.The LLM API key menu item is well-defined, but the chat menu item could benefit from a condition to control its visibility (e.g., when the server is connected).
{ "when": "view == chatPanelView", "command": "zenml.openChat", "group": "navigation" } +{ + "when": "serverCommandsRegistered && view == chatPanelView", + "command": "zenml.openChat", + "group": "navigation" +}Also applies to: 489-493
src/views/chatView/chatMessageHandler.ts (3)
63-74: Consider reviewing the necessity of the try-catch block inshowInfohandlerIf
chatDataProvider.showInfoMessageis unlikely to throw exceptions, you might consider removing thetry-catchblock to simplify the code and improve readability.
75-86: Consider reviewing the necessity of the try-catch block inupdateProviderhandlerIf
chatDataProvider.updateProviderdoes not throw exceptions, removing thetry-catchblock could streamline the code without affecting functionality.
87-98: Consider reviewing the necessity of the try-catch block inupdateModelhandlerIf
chatDataProvider.updateModelis a synchronous operation that doesn't throw exceptions, you might simplify the code by omitting thetry-catchblock.src/views/chatView/chatRenderer.ts (5)
15-15: Nitpick: Remove unused import of 'Tokens'.The
Tokensimport from themarkedlibrary is not used in this file. Removing it can help clean up the code.Apply this diff to remove the unused import:
-import { marked, Renderer, Tokens } from 'marked'; +import { marked, Renderer } from 'marked';
115-115: Remove unnecessary type assertion in 'marked.use' call.Since
rendereris already typed asPartial<Renderer>, you can directly pass it tomarked.usewithout asserting its type.Apply this diff:
marked.use({ renderer: renderer as Renderer }); +// Simplify to: +marked.use({ renderer });
123-126: Improve accessibility by using semantic HTML elements.Currently, the messages are wrapped in
<div>elements with<p>tags for the role name and the content directly inserted as HTML. Consider using more semantic elements, such as<article>for each message and headings for the role names.Example:
<article class="p-4 assistant"> <h2 class="font-semibold text-zenml">ZenML Assistant</h2> <div class="message-content"> <!-- sanitized content --> </div> </article>This enhances the semantic structure and can improve accessibility for users with assistive technologies.
165-165: Limit the use of checkboxes to enhance user experience.The condition
level < 2for displaying checkboxes means that checkboxes will only appear for the top two levels of the tree. Consider whether this limitation is necessary or if it would be more intuitive to allow checkboxes at all levels.Evaluate the user interface requirements and adjust the condition as needed.
181-192: Simplify HTML generation by using template literals effectively.The current HTML strings have inconsistent indentation and could be made more readable by using template literals and maintaining consistent formatting.
For example, reformat the return value:
return ` <div class="tree-item"> <div class="tree-item-wrapper"> <div class="tree-item-content" style="padding-left: ${level * 16}px;"> <span class="tree-item-icon">${iconSvg}</span> ${prevPageEl} <span class="tree-item-name" title="${title}">${item.name}</span> ${nextPageEl} ${checkboxEl} </div> ${childrenEl} </div> </div> `;This enhances readability and maintainability.
src/views/chatView/utils/ContextUtils.ts (1)
255-286: Improve Error Logging ingetPipelineRunNodesThe current error message in the catch block is generic. Enhancing it with more context will aid in debugging and troubleshooting.
Modify the error logging:
} catch (error) { - console.error('Failed to retrieve pipeline run nodes:', error); + console.error(`Failed to retrieve pipeline run nodes for pipelineRunId: ${pipelineRunId || 'All IDs'}`, error); return []; }This change provides additional information about which pipeline run IDs were involved when the error occurred.
📜 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 (13)
package.json(8 hunks)postcss.config.js(1 hunks)resources/chat-view/chat.html(1 hunks)resources/chat-view/chat.js(1 hunks)src/types/ChatTypes.ts(1 hunks)src/views/chatView/ChatDataProvider.ts(1 hunks)src/views/chatView/chatMessageHandler.ts(1 hunks)src/views/chatView/chatRenderer.ts(1 hunks)src/views/chatView/utils/ContextUtils.ts(1 hunks)src/views/chatView/utils/CustomErrors.ts(1 hunks)src/views/chatView/utils/PipelineUtils.ts(1 hunks)src/views/chatView/utils/TokenUtils.ts(1 hunks)tailwind.config.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tailwind.config.js
🚧 Files skipped from review as they are similar to previous changes (2)
- postcss.config.js
- resources/chat-view/chat.js
🧰 Additional context used
📓 Path-based instructions (8)
src/types/ChatTypes.ts (1)
Pattern **/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
src/views/chatView/ChatDataProvider.ts (1)
Pattern **/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
src/views/chatView/chatMessageHandler.ts (1)
Pattern **/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
src/views/chatView/chatRenderer.ts (1)
Pattern **/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
src/views/chatView/utils/ContextUtils.ts (1)
Pattern **/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
src/views/chatView/utils/CustomErrors.ts (1)
Pattern **/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
src/views/chatView/utils/PipelineUtils.ts (1)
Pattern **/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
src/views/chatView/utils/TokenUtils.ts (1)
Pattern **/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
🔇 Additional comments (20)
src/views/chatView/utils/CustomErrors.ts (1)
1-12: LGTM! License header is properly formatted.
The Apache 2.0 license header is correctly included with all required information.
src/types/ChatTypes.ts (2)
27-42: LGTM!
The TreeItem interface is well-structured with appropriate types and clear documentation.
47-56: LGTM!
The ContextItem interface is well-defined with appropriate property requirements.
package.json (5)
53-53: LGTM: Format script update is appropriate.
The format script now correctly includes .js and .html files, which is necessary for the new chat interface files.
283-288: LGTM: Chat command is well-defined.
The command is properly categorized and uses an appropriate icon for chat functionality.
357-360: LGTM: Chatbot view is properly configured.
The view is correctly defined as a webview, which is appropriate for rendering a rich chat interface.
517-517: LGTM: CSS processing dependencies are appropriate.
The addition of Tailwind CSS and its required dependencies (autoprefixer, postcss) is appropriate for styling the chat interface.
Also applies to: 520-520, 523-523
307-311: Verify secure storage of LLM API keys.
While the command is properly defined, ensure that the API keys are stored securely using VSCode's secret storage API.
✅ Verification successful
API keys are securely stored using VSCode's built-in secrets storage
The implementation in src/commands/secrets/cmds.ts shows that LLM API keys are properly stored using VSCode's built-in secrets.store() API, which is the recommended secure storage mechanism for sensitive data in VSCode extensions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if VSCode's secret storage API is used for storing API keys
# Test: Look for SecretStorage usage in TypeScript files
echo "Checking for secure API key storage:"
rg --type typescript -A 5 "SecretStorage|getPassword|setPassword"
Length of output: 253
Script:
#!/bin/bash
# Search for files related to the LLM API key command implementation
rg -g "*.ts" -g "*.js" "registerLLMAPIKey" -A 10
# Also check for any secret storage related code
rg -g "*.ts" -g "*.js" -i "secret.*storage|store.*secret|vscode.*secret" -A 5
Length of output: 2595
src/views/chatView/utils/PipelineUtils.ts (1)
55-62: Ensure HTML tags in 'name' property are rendered correctly
The 'name' property of TreeItem includes HTML tags (e.g., <b>). Verify that the UI component rendering these items supports HTML content. If not, the tags may be displayed as plain text. Consider using a separate property for formatted content if necessary.
src/views/chatView/chatMessageHandler.ts (6)
18-22: Well-defined CommandHandler type with appropriate parameters
The CommandHandler type is correctly defined with appropriate parameters and a Promise-based return type, ensuring consistency across command handlers.
25-50: sendMessage handler is well-implemented with robust error handling
The sendMessage handler properly validates the presence of message.text and handles specific errors like NetworkError and ValidationError. The user receives clear and actionable messages, enhancing the user experience.
51-62: clearChat handler effectively clears the chat log with proper error handling
The clearChat handler correctly clears the chat history and provides specific feedback in case of a StorageError, ensuring the user is informed of any issues.
99-107: prevPage handler correctly implements pagination with proper error handling
The prevPage handler effectively navigates to the previous page and refreshes the webview, providing clear feedback to the user in case of errors.
108-116: nextPage handler correctly implements pagination with proper error handling
The nextPage handler successfully moves to the next page and refreshes the webview, ensuring a smooth user experience even when errors occur.
119-130: handleWebviewMessage function effectively dispatches messages to appropriate handlers
The handleWebviewMessage function correctly identifies and invokes the corresponding handler for each command, and logs a warning for unknown commands, aiding in debugging.
src/views/chatView/chatRenderer.ts (4)
120-120: Avoid potential XSS by sanitizing rendered markdown content.
As previously mentioned, directly using marked.parse without sanitization can introduce security risks. Ensure that all markdown content is sanitized before rendering.
Refer to the earlier comment on sanitizing user input in lines 55-68.
131-135: Ensure streaming messages are properly sanitized and rendered.
Similar to previous comments, make sure that the streamingMessage content is sanitized to prevent any security issues.
Refer to the earlier comment on sanitizing user input in lines 55-68.
147-192:
Fix inconsistent button class attributes in pagination controls.
In the convertTreeDataToHtml function, buttons are created with class attributes that may be inconsistent or incorrect. Ensure that the class attribute is properly formatted with an = sign.
Apply this diff to correct the button elements:
if (item.firstPage) {
- nextPageEl = `<button class"tree-item-button" id="nextPage">Next</button>`;
- prevPageEl = `<button class"tree-item-button" id="prevPage" style="visibility: hidden;">Prev</button>`;
+ nextPageEl = `<button class="tree-item-button" id="nextPage">Next</button>`;
+ prevPageEl = `<button class="tree-item-button" id="prevPage" style="visibility: hidden;">Prev</button>`;
} else if (item.lastPage) {
- prevPageEl = `<button class"tree-item-button" id="prevPage">Prev</button>`;
- nextPageEl = `<button class"tree-item-button" id="nextPage" style="visibility: hidden;">Next</button>`;
+ prevPageEl = `<button class="tree-item-button" id="prevPage">Prev</button>`;
+ nextPageEl = `<button class="tree-item-button" id="nextPage" style="visibility: hidden;">Next</button>`;
} else {
- nextPageEl = `<button class"tree-item-button" id="nextPage">Next</button>`;
- prevPageEl = `<button class"tree-item-button" id="prevPage">Prev</button>`;
+ nextPageEl = `<button class="tree-item-button" id="nextPage">Next</button>`;
+ prevPageEl = `<button class="tree-item-button" id="prevPage">Prev</button>`;
}This fixes the missing = signs in the class attributes.
107-113:
Correct the parameter typing in the custom 'marked' renderer.
The code method in your custom renderer uses destructured parameters with the Tokens.Code type, but this may cause type incompatibility with the expected signature in marked. The code method should have the signature:
code(code: string, language: string | undefined, isEscaped: boolean): string;Apply this diff to correct the method signature:
function renderChatLog(
messages: ChatMessage[],
streamingMessage: ChatMessage | null = null
): string {
const renderer: Partial<Renderer> = {
- code({ text, lang, escaped }: Tokens.Code) {
- const formattedCode = text.replace(/\n$/, '') + (escaped ? '' : '\n');
+ code(code: string, language: string | undefined, isEscaped: boolean) {
+ const formattedCode = code.replace(/\n$/, '') + (isEscaped ? '' : '\n');
return isEscaped
? `<code>${formattedCode}</code>`
: '<pre><code>' + formattedCode + '</code></pre>\n';
},
};This should resolve any TypeScript type errors and ensure compatibility with the marked library.
⛔ Skipped due to learnings
Learnt from: wyennie
PR: zenml-io/vscode-zenml#56
File: src/views/chatView/chatRenderer.ts:0-0
Timestamp: 2024-09-26T19:11:09.263Z
Learning: In this codebase, the `marked` library's `code` method accepts an object with properties `text`, `lang`, and `escaped` as parameters, instead of separate arguments.
Learnt from: wyennie
PR: zenml-io/vscode-zenml#56
File: src/views/chatView/chatRenderer.ts:0-0
Timestamp: 2024-10-08T15:45:17.501Z
Learning: In this codebase, the `marked` library's `code` method accepts an object with properties `text`, `lang`, and `escaped` as parameters, instead of separate arguments.
src/views/chatView/utils/ContextUtils.ts (1)
239-243: Approve: Proper Filtering of Null Logs
The implementation correctly filters out null values from the logs array, ensuring that only valid log data is processed downstream.
| export class NetworkError extends Error { | ||
| constructor(message: string) { | ||
| super(message); | ||
| this.name = 'NetworkError'; | ||
| } | ||
| } | ||
|
|
||
| export class ValidationError extends Error { | ||
| constructor(message: string) { | ||
| super(message); | ||
| this.name = 'ValidationError'; | ||
| } | ||
| } | ||
|
|
||
| export class StorageError extends Error { | ||
| constructor(message: string) { | ||
| super(message); | ||
| this.name = 'StorageError'; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing error classes with additional features.
While the basic implementation is correct, consider these improvements for better error handling:
- Add JSDoc documentation
- Preserve stack traces
- Prevent inheritance
- Add custom properties for context
Here's an improved implementation:
+/** Custom error for network-related issues */
+export class NetworkError extends Error {
+ /** @param {string} message - The error message */
+ constructor(message: string) {
+ super(message);
this.name = 'NetworkError';
+ // Maintains proper stack trace
+ Object.setPrototypeOf(this, NetworkError.prototype);
}
}
+/** Custom error for validation-related issues */
+export class ValidationError extends Error {
+ /** @param {string} message - The error message */
+ constructor(message: string) {
+ super(message);
this.name = 'ValidationError';
+ Object.setPrototypeOf(this, ValidationError.prototype);
}
}
+/** Custom error for storage-related issues */
+export class StorageError extends Error {
+ /** @param {string} message - The error message */
+ constructor(message: string) {
+ super(message);
this.name = 'StorageError';
+ Object.setPrototypeOf(this, StorageError.prototype);
}
}Optional: If you need to add context to these errors, consider adding custom properties:
export class NetworkError extends Error {
constructor(
message: string,
public readonly statusCode?: number,
public readonly endpoint?: string
) {
super(message);
this.name = 'NetworkError';
Object.setPrototypeOf(this, NetworkError.prototype);
}
}
src/types/ChatTypes.ts
Outdated
| export type AIModel = | ||
| | 'gemini-1.5-pro' | ||
| | 'gemini-1.5-flash' | ||
| | 'gemini-1.0-pro' | ||
| | 'gpt-4o' | ||
| | 'gpt-4o-mini' | ||
| | 'gpt-4o-2024-05-13' | ||
| | 'gpt-4-turbo' | ||
| | 'gpt-4-turbo-2024-04-09' | ||
| | 'gpt-4-0125-preview' | ||
| | 'gpt-4-turbo-preview' | ||
| | 'gpt-4-1106-preview' | ||
| | 'gpt-4-vision-preview' | ||
| | 'gpt-4' | ||
| | 'gpt-4-0314' | ||
| | 'gpt-4-0613' | ||
| | 'gpt-4-32k' | ||
| | 'gpt-4-32k-0314' | ||
| | 'gpt-4-32k-0613' | ||
| | 'gpt-3.5-turbo' | ||
| | 'gpt-3.5-turbo-16k' | ||
| | 'gpt-3.5-turbo-0301' | ||
| | 'gpt-3.5-turbo-0613' | ||
| | 'gpt-3.5-turbo-1106' | ||
| | 'gpt-3.5-turbo-0125' | ||
| | 'gpt-3.5-turbo-16k-0613' | ||
| | 'claude-3-5-sonnet-20240620' | ||
| | 'claude-3-opus-20240229' | ||
| | 'claude-3-sonnet-20240229' | ||
| | 'claude-3-haiku-20240307' | ||
| | 'claude-2.1' | ||
| | 'claude-2.0' | ||
| | 'claude-instant-1.2'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider organizing models by provider for better maintainability.
The model list could be better organized by grouping them by provider.
const OPENAI_MODELS = {
GPT4: {
DEFAULT: 'gpt-4',
TURBO: 'gpt-4-turbo',
// ... other GPT-4 variants
},
GPT35: {
DEFAULT: 'gpt-3.5-turbo',
// ... other GPT-3.5 variants
}
} as const;
const ANTHROPIC_MODELS = {
CLAUDE3: {
OPUS: 'claude-3-opus-20240229',
// ... other Claude-3 variants
},
CLAUDE2: {
DEFAULT: 'claude-2.1',
// ... other Claude-2 variants
}
} as const;
const GEMINI_MODELS = {
V15: {
PRO: 'gemini-1.5-pro',
// ... other 1.5 variants
},
V10: {
PRO: 'gemini-1.0-pro'
}
} as const;
export type AIModel =
| typeof OPENAI_MODELS[keyof typeof OPENAI_MODELS][keyof typeof OPENAI_MODELS[keyof typeof OPENAI_MODELS]]
| typeof ANTHROPIC_MODELS[keyof typeof ANTHROPIC_MODELS][keyof typeof ANTHROPIC_MODELS[keyof typeof ANTHROPIC_MODELS]]
| typeof GEMINI_MODELS[keyof typeof GEMINI_MODELS][keyof typeof GEMINI_MODELS[keyof typeof GEMINI_MODELS]];
src/types/ChatTypes.ts
Outdated
| export interface ChatMessage { | ||
| /** The role of the message sender (e.g., user, assistant). */ | ||
| role: string; | ||
| /** The content of the message. */ | ||
| content: string; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a union type for the role property.
The role property could be more strictly typed to prevent invalid values.
export interface ChatMessage {
- role: string;
+ role: 'user' | 'assistant' | 'system';
content: string;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface ChatMessage { | |
| /** The role of the message sender (e.g., user, assistant). */ | |
| role: string; | |
| /** The content of the message. */ | |
| content: string; | |
| } | |
| export interface ChatMessage { | |
| /** The role of the message sender (e.g., user, assistant). */ | |
| role: 'user' | 'assistant' | 'system'; | |
| /** The content of the message. */ | |
| content: string; | |
| } |
| <svg | ||
| width="15" | ||
| height="15" | ||
| viewBox="0 0 15 15" | ||
| fill="none" | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| class="hidden md:block size-6 text-gray" | ||
| > | ||
| <path | ||
| d="M7.5 2C7.77614 2 8 2.22386 8 2.5L8 12.5C8 12.7761 7.77614 13 7.5 13C7.22386 13 7 12.7761 7 12.5L7 2.5C7 2.22386 7.22386 2 7.5 2Z" | ||
| fill="currentColor" | ||
| fill-rule="evenodd" | ||
| clip-rule="evenodd" | ||
| ></path> | ||
| </svg> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce SVG code duplication
The separator SVG is duplicated. Consider extracting it into a reusable template variable.
Move the SVG definition to the JavaScript code and inject it where needed:
const separatorSvg = `<svg width="15" height="15" viewBox="0 0 15 15" fill="none" xmlns="http://www.w3.org/2000/svg" class="hidden md:block size-6 text-gray">
<path d="M7.5 2C7.77614 2 8 2.22386 8 2.5L8 12.5C8 12.7761 7.77614 13 7.5 13C7.22386 13 7 12.7761 7 12.5L7 2.5C7 2.22386 7.22386 2 7.5 2Z" fill="currentColor" fill-rule="evenodd" clip-rule="evenodd"></path>
</svg>`;Also applies to: 128-142
resources/chat-view/chat.html
Outdated
| <div | ||
| id="chatMessages" | ||
| class="space-y-4 mb-4 mt-4 overflow-y-auto flex-grow" | ||
| style="max-height: calc(100vh - 300px)" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace fixed height calculation with a more flexible solution
Using a fixed calc() value might cause layout issues on different screen sizes.
- style="max-height: calc(100vh - 300px)"
+ class="max-h-[calc(100vh-300px)] md:max-h-[calc(100vh-200px)]"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| style="max-height: calc(100vh - 300px)" | |
| class="max-h-[calc(100vh-300px)] md:max-h-[calc(100vh-200px)]" |
| type ContextType = | ||
| | 'serverContext' | ||
| | 'environmentContext' | ||
| | 'pipelineContext' | ||
| | 'stackContext' | ||
| | 'stackComponentsContext' | ||
| | 'logsContext' | ||
| | 'pipelineRunContext' | ||
| | string; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor ContextType to Enhance Type Safety
Including string in the ContextType union undermines type safety, allowing any string value and potentially leading to unexpected behavior. To enforce stricter type checking and prevent invalid context types, consider removing | string from the ContextType definition.
Apply this diff to improve type safety:
type ContextType =
| 'serverContext'
| 'environmentContext'
| 'pipelineContext'
| 'stackContext'
| 'stackComponentsContext'
| 'logsContext'
| 'pipelineRunContext';
- | string;This change ensures that only predefined context types are accepted, allowing TypeScript to catch invalid values at compile time.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type ContextType = | |
| | 'serverContext' | |
| | 'environmentContext' | |
| | 'pipelineContext' | |
| | 'stackContext' | |
| | 'stackComponentsContext' | |
| | 'logsContext' | |
| | 'pipelineRunContext' | |
| | string; | |
| type ContextType = | |
| | 'serverContext' | |
| | 'environmentContext' | |
| | 'pipelineContext' | |
| | 'stackContext' | |
| | 'stackComponentsContext' | |
| | 'logsContext' | |
| | 'pipelineRunContext'; |
| await lsClient.sendLsClientRequest<ZenmlGlobalConfigResp>('getGlobalConfig'); | ||
| const apiToken = globalConfig.store.api_token; | ||
| const dashboardUrl = globalConfig.store.url; | ||
|
|
||
| if (!apiToken) { | ||
| throw new Error('API Token is not available in global configuration'); | ||
| } |
There was a problem hiding this comment.
Add Null Checks for globalConfig.store
The code assumes that globalConfig.store is always defined. In cases where it's undefined, accessing its properties will cause runtime errors. Adding null checks ensures robust error handling.
Apply this diff:
const globalConfig =
await lsClient.sendLsClientRequest<ZenmlGlobalConfigResp>('getGlobalConfig');
+ if (!globalConfig.store) {
+ throw new Error('Global configuration store is undefined');
+ }
const apiToken = globalConfig.store.api_token;
const dashboardUrl = globalConfig.store.url;
- if (!apiToken) {
+ if (!apiToken || !dashboardUrl) {
throw new Error('API Token or Dashboard URL is not available in global configuration');
}This change prevents potential crashes due to accessing properties of undefined.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await lsClient.sendLsClientRequest<ZenmlGlobalConfigResp>('getGlobalConfig'); | |
| const apiToken = globalConfig.store.api_token; | |
| const dashboardUrl = globalConfig.store.url; | |
| if (!apiToken) { | |
| throw new Error('API Token is not available in global configuration'); | |
| } | |
| const globalConfig = | |
| await lsClient.sendLsClientRequest<ZenmlGlobalConfigResp>('getGlobalConfig'); | |
| if (!globalConfig.store) { | |
| throw new Error('Global configuration store is undefined'); | |
| } | |
| const apiToken = globalConfig.store.api_token; | |
| const dashboardUrl = globalConfig.store.url; | |
| if (!apiToken || !dashboardUrl) { | |
| throw new Error('API Token or Dashboard URL is not available in global configuration'); | |
| } |
| type Step = { | ||
| id: string; | ||
| [key: string]: any; | ||
| }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine the Step Type Definition for Clarity
The current Step type includes an index signature, which allows any additional properties and may mask type errors. Defining specific expected properties improves readability and type safety.
Update the Step type:
type Step = {
id: string;
+ type: string;
+ name: string;
+ status: string;
- [key: string]: any;
};By specifying the expected properties, you help TypeScript catch potential errors and make the code more self-documenting.
Committable suggestion skipped: line range outside the PR's diff.
| if (context.startsWith('Pipeline Run:')) { | ||
| try { | ||
| let runData = JSON.parse(context.replace('Pipeline Run:', '')); | ||
| let logs = await getPipelineRunLogs(runData.id); | ||
| let nodeData = await getPipelineRunNodes('step', runData.id); | ||
| systemMessage += `Pipeline Run: ${JSON.stringify(runData)}\n`; | ||
| systemMessage += `Logs: ${logs}\n`; | ||
| systemMessage += `Step Data: ${JSON.stringify(nodeData)}\n`; | ||
| } catch (error) { | ||
| console.error('Failed to parse pipeline run data from context:', error); | ||
| systemMessage += 'Failed to parse pipeline run data from context.\n'; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify Context Handling and Reduce Complexity
The current implementation checks if the context starts with 'Pipeline Run:' and then parses JSON data from it. This approach mixes control flow and data parsing, potentially leading to errors. Refactoring the code to handle pipeline run contexts explicitly in the switch statement enhances readability and maintainability.
Refactor the code as follows:
- if (context.startsWith('Pipeline Run:')) {
- try {
- let runData = JSON.parse(context.replace('Pipeline Run:', ''));
- let logs = await getPipelineRunLogs(runData.id);
- let nodeData = await getPipelineRunNodes('step', runData.id);
- systemMessage += `Pipeline Run: ${JSON.stringify(runData)}\n`;
- systemMessage += `Logs: ${logs}\n`;
- systemMessage += `Step Data: ${JSON.stringify(nodeData)}\n`;
- } catch (error) {
- console.error('Failed to parse pipeline run data from context:', error);
- systemMessage += 'Failed to parse pipeline run data from context.\n';
- }
- } else {
switch (context) {
+ case 'pipelineRunContext':
+ try {
+ const runData = await getSelectedPipelineRunData();
+ const logs = await getPipelineRunLogs(runData.id);
+ const nodeData = await getPipelineRunNodes('step', runData.id);
+ systemMessage += `Pipeline Run: ${JSON.stringify(runData)}\n`;
+ systemMessage += `Logs: ${JSON.stringify(logs)}\n`;
+ systemMessage += `Step Data: ${JSON.stringify(nodeData)}\n`;
+ } catch (error) {
+ console.error('Failed to retrieve pipeline run data:', error);
+ systemMessage += 'Failed to retrieve pipeline run data.\n';
+ }
+ break;
case 'serverContext':
systemMessage += getServerData();
break;This refactoring moves the pipeline run context handling into the switch statement, aligning it with other context types and improving code structure.
Committable suggestion skipped: line range outside the PR's diff.
| if (nodeType === 'all' || node.type === nodeType) { | ||
| return await lsClient.sendLsClientRequest<JsonObject>('getPipelineRunStep', [node.id]); | ||
| } | ||
| return null; | ||
| }) |
There was a problem hiding this comment.
Handle Artifact Nodes in getPipelineRunNodes
Currently, the function only retrieves step data even when nodeType is 'artifact' or 'all'. To fully support these node types, implement logic to handle artifacts.
Update the node retrieval logic:
const filteredNodes = await Promise.all(
dag.nodes.map(async (node: DagArtifact | DagStep) => {
if (nodeType === 'all' || node.type === nodeType) {
- return await lsClient.sendLsClientRequest<JsonObject>('getPipelineRunStep', [node.id]);
+ if (node.type === 'step') {
+ return await lsClient.sendLsClientRequest<JsonObject>('getPipelineRunStep', [node.id]);
+ } else if (node.type === 'artifact') {
+ return await lsClient.sendLsClientRequest<JsonObject>('getPipelineRunArtifact', [node.id]);
+ }
}
return null;
})
);This modification ensures that artifact nodes are correctly fetched when requested.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (nodeType === 'all' || node.type === nodeType) { | |
| return await lsClient.sendLsClientRequest<JsonObject>('getPipelineRunStep', [node.id]); | |
| } | |
| return null; | |
| }) | |
| if (nodeType === 'all' || node.type === nodeType) { | |
| if (node.type === 'step') { | |
| return await lsClient.sendLsClientRequest<JsonObject>('getPipelineRunStep', [node.id]); | |
| } else if (node.type === 'artifact') { | |
| return await lsClient.sendLsClientRequest<JsonObject>('getPipelineRunArtifact', [node.id]); | |
| } | |
| } | |
| return null; | |
| }) |
…d have the errors preserve the stack trace
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (10)
src/types/ChatTypes.ts (4)
24-42: Consider grouping related properties and adding type validation.While the interface is well-documented, consider these improvements:
- Group pagination-related properties using a nested interface
- Add type validation for the
valuepropertyexport interface TreeItem { name: string; - value?: string | number; + value?: string | number | null; children?: TreeItem[]; title?: string; hidden?: boolean; - firstPage?: boolean; - lastPage?: boolean; + pagination?: { + isFirstPage: boolean; + isLastPage: boolean; + }; }
44-56: Consider using inheritance for better type organization.The
ContextIteminterface shares common properties withTreeItem. Consider using inheritance to reduce duplication and improve maintainability.-export interface ContextItem { +type BaseItem = { name: string; + title?: string; + children?: TreeItem[]; +}; + +export interface ContextItem extends BaseItem { value: string; title: string; - children?: TreeItem[]; }
86-92: Improve readability of the AIModel type using type helpers.The current type definition is correct but hard to read. Consider using type helpers to improve maintainability.
+type ModelValues<T> = T[keyof T][keyof T[keyof T]]; + export type AIModel = - | (typeof OPENAI_MODELS)[keyof typeof OPENAI_MODELS][keyof (typeof OPENAI_MODELS)[keyof typeof OPENAI_MODELS]] - | (typeof ANTHROPIC_MODELS)[keyof typeof ANTHROPIC_MODELS][keyof (typeof ANTHROPIC_MODELS)[keyof typeof ANTHROPIC_MODELS]] - | (typeof GEMINI_MODELS)[keyof typeof GEMINI_MODELS][keyof (typeof GEMINI_MODELS)[keyof typeof GEMINI_MODELS]]; + | ModelValues<typeof OPENAI_MODELS> + | ModelValues<typeof ANTHROPIC_MODELS> + | ModelValues<typeof GEMINI_MODELS>;
94-115: Ensure consistency between providers and model constants.Consider deriving the provider type from the model constants to maintain consistency and prevent mismatches.
+type Provider = keyof typeof MODELS_MAP; + +const MODELS_MAP = { + openai: OPENAI_MODELS, + anthropic: ANTHROPIC_MODELS, + gemini: GEMINI_MODELS, +} as const; + export interface WebviewMessage { command: 'sendMessage' | 'clearChat' | 'showInfo' | 'updateProvider' | 'updateModel' | 'prevPage' | 'nextPage'; text?: string; context?: string[]; - provider?: 'openai' | 'anthropic' | 'gemini'; + provider?: Provider; model?: AIModel; }src/views/chatView/utils/TokenUtils.ts (4)
28-29: Enhance type safety for the config objectThe config object type could be more specific than
Record<string, string>. Consider creating a dedicated interface for the configuration.interface TokenJSConfig { apiKey: string; // Add other potential config properties } const config: TokenJSConfig = { apiKey: apiKey };
41-78: Extract template to a separate constantThe template string is quite long and could be moved to a separate constant or configuration file. This would improve maintainability and make the code more modular.
// At the top of the file or in a separate config file const CHAT_RESPONSE_TEMPLATE = `...`; // In the function const template = CHAT_RESPONSE_TEMPLATE;
95-98: Improve type safety for message rolesInstead of type casting, consider defining a proper type for the message role.
type MessageRole = 'system' | 'user' | 'assistant'; interface ChatMessageWithRole extends ChatMessage { role: MessageRole; } messages: fullMessages.map(msg => ({ role: msg.role, content: msg.content, } as ChatMessageWithRole)),
102-106: Add type safety for stream responseThe stream response structure could benefit from proper type definitions.
interface ChatChoice { delta: { content?: string; }; } interface StreamResponse { choices: ChatChoice[]; } for await (const part of stream as AsyncIterable<StreamResponse>) { if (part.choices[0]?.delta?.content) { yield part.choices[0].delta.content; } }src/views/chatView/utils/ContextUtils.ts (2)
13-25: Remove unused importerrorfrom consoleThe
errorimport from 'console' on line 24 is not used anywhere in the code.-import { error } from 'console';
148-200: Consider implementing rate limiting for API callsThe log retrieval functions make multiple API calls in parallel without any rate limiting. This could potentially overwhelm the server or hit API rate limits.
Consider implementing a rate limiter utility:
class RateLimiter { private queue: Array<() => Promise<any>> = []; private processing = false; constructor(private requestsPerSecond: number) {} async add<T>(fn: () => Promise<T>): Promise<T> { return new Promise((resolve, reject) => { this.queue.push(async () => { try { resolve(await fn()); } catch (error) { reject(error); } }); this.process(); }); } private async process() { if (this.processing) return; this.processing = true; while (this.queue.length > 0) { const batch = this.queue.splice(0, this.requestsPerSecond); await Promise.all(batch.map(fn => fn())); await new Promise(resolve => setTimeout(resolve, 1000)); } this.processing = false; } }Then use it in the log fetching functions:
const rateLimiter = new RateLimiter(5); // 5 requests per second // In getLogData: const logs = await Promise.all( pipelineRunSteps[0].map(step => rateLimiter.add(() => fetchStepLogs(step)) ) );Also applies to: 222-256
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (8)
resources/chat-view/chat.html(1 hunks)src/types/ChatTypes.ts(1 hunks)src/views/chatView/ChatDataProvider.ts(1 hunks)src/views/chatView/chatRenderer.ts(1 hunks)src/views/chatView/utils/ContextUtils.ts(1 hunks)src/views/chatView/utils/CustomErrors.ts(1 hunks)src/views/chatView/utils/PipelineUtils.ts(1 hunks)src/views/chatView/utils/TokenUtils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- resources/chat-view/chat.html
- src/views/chatView/ChatDataProvider.ts
- src/views/chatView/chatRenderer.ts
- src/views/chatView/utils/CustomErrors.ts
🧰 Additional context used
📓 Path-based instructions (4)
src/types/ChatTypes.ts (1)
Pattern **/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
src/views/chatView/utils/ContextUtils.ts (1)
Pattern **/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
src/views/chatView/utils/PipelineUtils.ts (1)
Pattern **/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
src/views/chatView/utils/TokenUtils.ts (1)
Pattern **/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
🔇 Additional comments (3)
src/types/ChatTypes.ts (2)
14-22: LGTM! Well-documented interface with proper type constraints.
The ChatMessage interface is well-structured with clear JSDoc comments and appropriate type constraints for the role property.
58-84: Verify the AI model versions are up-to-date.
The model organization is good, but some versions might need updating:
- Claude models: Verify if these are the latest versions
- GPT models: Check if newer versions are available
✅ Verification successful
Let me gather more information about model version references in configuration or documentation files.
Model versions are up-to-date and aligned with recent changes
Based on the git history and codebase analysis, the current model versions are correct and reflect recent updates:
- Claude models: Using latest versions (claude-3-5-sonnet-20240620, claude-3-opus-20240229)
- GPT models: Using current versions (gpt-4o-mini, gpt-3.5-turbo)
- Gemini models: Using latest versions (gemini-1.5-pro, gemini-1.5-flash)
The model versions were recently updated as part of a larger refactoring that consolidated and simplified the model selection, removing older versions and keeping only the current ones.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for model version references in the codebase
echo "Searching for model version references..."
rg -i "claude-.*-\d{8}|gpt-\d|gemini-\d" --type ts
Length of output: 779
Script:
#!/bin/bash
# Search for model version references in config and documentation
echo "Searching in config and docs..."
rg -i "model.*version|supported.*model" --type yaml --type md --type json
# Check for any recent changes related to models
echo -e "\nChecking recent model-related changes..."
git log --since="3 months ago" -p | rg -i "model.*version|claude-|gpt-|gemini"
Length of output: 21925
src/views/chatView/utils/PipelineUtils.ts (1)
52-52: Ensure sensitive data is not exposed when serializing run object
Using JSON.stringify(run) may inadvertently expose sensitive information contained within the run object. Please verify that all properties of run are safe to be displayed to the user.
Run the following script to list all properties of the PipelineRun interface:
This will help confirm that no sensitive data is included when serializing the run object.
✅ Verification successful
No sensitive data exposed in PipelineRun serialization
Based on the PipelineRun interface definition found in src/types/PipelineTypes.ts, the object only contains basic metadata:
- id, name, status
- stack name
- start/end times
- OS information (os, osVersion)
- Python version
All these fields are non-sensitive operational metadata that are safe to display to users.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find the definition of the PipelineRun interface and list its properties.
# Search for the PipelineRun interface definition in TypeScript files.
rg -A 20 'interface PipelineRun' --type typescript
Length of output: 87
Script:
#!/bin/bash
# Let's try different approaches to find the interface definition
# Search for files containing "PipelineRun" in their content
rg "PipelineRun" -l
# Search for the actual interface definition with more context
rg -A 20 "interface.*PipelineRun"
# Look for type definitions or interfaces related to pipeline runs
rg -A 10 "type.*PipelineRun|interface.*Pipeline"
# Search for the usage context of the run object
rg -B 5 -A 5 'JSON\.stringify\(run\)'
Length of output: 4633
| const response = await axios.get(`${dashboardUrl}/api/v1/steps/${step.id}/logs`, { | ||
| headers: { | ||
| Authorization: `Bearer ${apiToken}`, | ||
| accept: 'application/json', | ||
| }, | ||
| }); | ||
| return response.data; | ||
| } catch (error) { | ||
| console.error(`Failed to get logs for step with id ${step.id}`, error); | ||
| return null; |
There was a problem hiding this comment.
Enhance security in error handling and URL construction
- API tokens should not be logged in error messages
- Step IDs should be sanitized before being used in URLs to prevent potential injection attacks
Apply these security enhancements:
try {
+ const sanitizedStepId = encodeURIComponent(stepId);
- const response = await axios.get(`${dashboardUrl}/api/v1/steps/${stepId}/logs`, {
+ const response = await axios.get(`${dashboardUrl}/api/v1/steps/${sanitizedStepId}/logs`, {
headers: {
Authorization: `Bearer ${apiToken}`,
accept: 'application/json',
},
});
return response.data;
} catch (error) {
- console.error(`Failed to get logs for step with id ${stepId}`, error);
+ console.error(`Failed to get logs for step`, { stepId, error: error.message });
return null;
}Also applies to: 209-218
| export async function addContext(requestedContext: ContextType[]): Promise<string> { | ||
| let systemMessage = 'Context:\n'; | ||
| for (let context of requestedContext) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for requestedContext array
The function should validate that the input array is not empty or null before processing.
export async function addContext(requestedContext: ContextType[]): Promise<string> {
+ if (!requestedContext?.length) {
+ return 'Context:\nNo context types requested.\n';
+ }
let systemMessage = 'Context:\n';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function addContext(requestedContext: ContextType[]): Promise<string> { | |
| let systemMessage = 'Context:\n'; | |
| for (let context of requestedContext) { | |
| export async function addContext(requestedContext: ContextType[]): Promise<string> { | |
| if (!requestedContext?.length) { | |
| return 'Context:\nNo context types requested.\n'; | |
| } | |
| let systemMessage = 'Context:\n'; | |
| for (let context of requestedContext) { |
| { name: `<b>run name:</b> ${run.name}` }, | ||
| { name: `<b>status:</b> ${run.status}` }, | ||
| { name: `<b>stack:</b> ${run.stackName}` }, | ||
| { name: `<b>start time:</b> ${formattedStartTime}` }, | ||
| { name: `<b>end time:</b> ${formattedEndTime}` }, | ||
| { name: `<b>os:</b> ${run.os} ${run.osVersion}` }, | ||
| { name: `<b>python version:</b> ${run.pythonVersion}` }, | ||
| ], |
There was a problem hiding this comment.
Sanitize user inputs to prevent XSS vulnerabilities
When injecting user-supplied data like run.name, run.status, etc., into HTML content, ensure that these values are properly sanitized or escaped to prevent Cross-Site Scripting (XSS) attacks.
Consider using a utility function or library to escape HTML characters. Here's how you might adjust the code:
+import { escape } from 'lodash';
const treeItem: TreeItem = {
name: run.name,
value: stringValue,
title: 'Includes all code, logs, and metadata for a specific pipeline run with message',
children: [
- { name: `<b>run name:</b> ${run.name}` },
- { name: `<b>status:</b> ${run.status}` },
- { name: `<b>stack:</b> ${run.stackName}` },
- { name: `<b>start time:</b> ${formattedStartTime}` },
- { name: `<b>end time:</b> ${formattedEndTime}` },
- { name: `<b>os:</b> ${run.os} ${run.osVersion}` },
- { name: `<b>python version:</b> ${run.pythonVersion}` },
+ { name: `<b>run name:</b> ${escape(run.name)}` },
+ { name: `<b>status:</b> ${escape(run.status)}` },
+ { name: `<b>stack:</b> ${escape(run.stackName)}` },
+ { name: `<b>start time:</b> ${escape(formattedStartTime)}` },
+ { name: `<b>end time:</b> ${escape(formattedEndTime)}` },
+ { name: `<b>os:</b> ${escape(run.os)} ${escape(run.osVersion)}` },
+ { name: `<b>python version:</b> ${escape(run.pythonVersion)}` },
],
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { name: `<b>run name:</b> ${run.name}` }, | |
| { name: `<b>status:</b> ${run.status}` }, | |
| { name: `<b>stack:</b> ${run.stackName}` }, | |
| { name: `<b>start time:</b> ${formattedStartTime}` }, | |
| { name: `<b>end time:</b> ${formattedEndTime}` }, | |
| { name: `<b>os:</b> ${run.os} ${run.osVersion}` }, | |
| { name: `<b>python version:</b> ${run.pythonVersion}` }, | |
| ], | |
| { name: `<b>run name:</b> ${escape(run.name)}` }, | |
| { name: `<b>status:</b> ${escape(run.status)}` }, | |
| { name: `<b>stack:</b> ${escape(run.stackName)}` }, | |
| { name: `<b>start time:</b> ${escape(formattedStartTime)}` }, | |
| { name: `<b>end time:</b> ${escape(formattedEndTime)}` }, | |
| { name: `<b>os:</b> ${escape(run.os)} ${escape(run.osVersion)}` }, | |
| { name: `<b>python version:</b> ${escape(run.pythonVersion)}` }, | |
| ], |
| const { treeItems } = getPipelineData(); | ||
| const paginatedTreeItems = [...treeItems]; | ||
| const pagination = PipelineDataProvider.getInstance().pagination; | ||
| const paginatedTreeItem = { | ||
| title: 'pagination', | ||
| name: `${pagination.currentPage} of ${pagination.totalPages}`, | ||
| firstPage: pagination.currentPage === 1, | ||
| lastPage: pagination.currentPage === pagination.totalPages, | ||
| }; | ||
|
|
||
| if (pagination.totalItems > pagination.itemsPerPage) { | ||
| paginatedTreeItems.push(paginatedTreeItem); | ||
| } | ||
|
|
||
| return paginatedTreeItems; |
There was a problem hiding this comment.
Implement actual pagination logic in getPaginatedTreeData
Currently, the getPaginatedTreeData function does not apply pagination to the treeItems. All items are returned regardless of the current page or items per page. To correctly implement pagination, slice treeItems based on pagination.currentPage and pagination.itemsPerPage.
Apply this diff to fix the pagination logic:
export function getPaginatedTreeData(): TreeItem[] {
try {
const { treeItems } = getPipelineData();
- const paginatedTreeItems = [...treeItems];
const pagination = PipelineDataProvider.getInstance().pagination;
+ const startIndex = (pagination.currentPage - 1) * pagination.itemsPerPage;
+ const endIndex = startIndex + pagination.itemsPerPage;
+ const paginatedTreeItems = treeItems.slice(startIndex, endIndex);
const paginatedTreeItem = {
title: 'pagination',
name: `${pagination.currentPage} of ${pagination.totalPages}`,
firstPage: pagination.currentPage === 1,
lastPage: pagination.currentPage === pagination.totalPages,
};
if (pagination.totalItems > pagination.itemsPerPage) {
paginatedTreeItems.push(paginatedTreeItem);
}
return paginatedTreeItems;
} catch (error) {
console.error('Error fetching paginated tree data:', error);
return [];
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { treeItems } = getPipelineData(); | |
| const paginatedTreeItems = [...treeItems]; | |
| const pagination = PipelineDataProvider.getInstance().pagination; | |
| const paginatedTreeItem = { | |
| title: 'pagination', | |
| name: `${pagination.currentPage} of ${pagination.totalPages}`, | |
| firstPage: pagination.currentPage === 1, | |
| lastPage: pagination.currentPage === pagination.totalPages, | |
| }; | |
| if (pagination.totalItems > pagination.itemsPerPage) { | |
| paginatedTreeItems.push(paginatedTreeItem); | |
| } | |
| return paginatedTreeItems; | |
| const { treeItems } = getPipelineData(); | |
| const pagination = PipelineDataProvider.getInstance().pagination; | |
| const startIndex = (pagination.currentPage - 1) * pagination.itemsPerPage; | |
| const endIndex = startIndex + pagination.itemsPerPage; | |
| const paginatedTreeItems = treeItems.slice(startIndex, endIndex); | |
| const paginatedTreeItem = { | |
| title: 'pagination', | |
| name: `${pagination.currentPage} of ${pagination.totalPages}`, | |
| firstPage: pagination.currentPage === 1, | |
| lastPage: pagination.currentPage === pagination.totalPages, | |
| }; | |
| if (pagination.totalItems > pagination.itemsPerPage) { | |
| paginatedTreeItems.push(paginatedTreeItem); | |
| } | |
| return paginatedTreeItems; |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
resources/chat-view/styles.css (3)
11-13: Remove redundant color declarationThe
colorproperty onulelements is redundant as it will inherit from thebodyelement. This improves maintainability and reduces CSS specificity issues.-ul { - color: var(--vscode-editor-foreground); -}
149-156: Improve context dropdown positioningThe absolute positioning with
left: 0; right: 0;might cause issues in smaller viewports. Consider using a more robust positioning strategy:.context-dropdown { - position: absolute; - left: 0; - right: 0; + position: fixed; + min-width: 200px; + max-width: 100%; bottom: 100%; margin-bottom: 0.25rem; z-index: 10; + max-height: 80vh; + overflow-y: auto; }
177-183: Enhance code block readabilityConsider adding syntax highlighting support and improving code block contrast:
#chatMessages pre code { display: block; background-color: var(--vscode-textCodeBlock-background); color: var(--vscode-editor-foreground); padding: 1em; border-radius: 5px; + font-family: var(--vscode-editor-font-family); + font-size: var(--vscode-editor-font-size); + line-height: 1.5; + tab-size: 2; + -moz-tab-size: 2; }resources/chat-view/chat.js (3)
18-82: Add error handling for localStorage operationsThe state management functions should include try-catch blocks to handle potential localStorage errors and type checking for stored values.
function saveState() { + try { const provider = document.querySelector('#provider-dropdown').value; const model = document.querySelector('#model-dropdown').value; // ... existing code ... localStorage.setItem('selectedProvider', provider); localStorage.setItem('selectedModel', model); localStorage.setItem('selectedContexts', JSON.stringify(state)); + } catch (error) { + console.error('Failed to save state:', error); + vscode.postMessage({ + command: 'showInfo', + text: 'Failed to save preferences' + }); + } }
291-320: Add error handling for clipboard operationsThe clipboard operations should include error handling to provide better user feedback.
if (content) { - navigator.clipboard.writeText(content).then(() => { - vscode.postMessage({ command: 'showInfo', text: 'Message copied to clipboard' }); - }); + navigator.clipboard.writeText(content) + .then(() => { + vscode.postMessage({ command: 'showInfo', text: 'Message copied to clipboard' }); + }) + .catch(error => { + console.error('Failed to copy:', error); + vscode.postMessage({ command: 'showInfo', text: 'Failed to copy message' }); + }); }
336-358: Add error handling for unknown sample message typesThe switch statement should handle unknown button values with an appropriate error message.
default: { + console.error('Unknown sample message type:', buttonValue); + vscode.postMessage({ + command: 'showInfo', + text: 'Invalid sample message type' + }); break; }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
resources/chat-view/chat.html(1 hunks)resources/chat-view/chat.js(1 hunks)resources/chat-view/styles.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/chat-view/chat.html
🧰 Additional context used
📓 Path-based instructions (1)
resources/chat-view/chat.js (1)
Pattern **/*.js: Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations.
🪛 Biome
resources/chat-view/chat.js
[error] 99-99: 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] 102-102: 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)
🪛 eslint
resources/chat-view/styles.css
[error] 2-2: Parsing error: Declaration expected.
🔇 Additional comments (1)
resources/chat-view/chat.js (1)
474-494: LGTM! Well-implemented textarea handling
The textarea event handler properly implements multi-line support with Shift+Enter and handles form submission correctly using CustomEvent.
| form { | ||
| display: flex; | ||
| } | ||
|
|
||
| #chatForm { | ||
| background: var(--vscode-input-background); | ||
| border: 1px solid var(--vscode-input-border); | ||
| border-radius: 0.375rem 0.375rem 0 0; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance form accessibility
While the form styling looks good, consider adding focus styles for better keyboard navigation and accessibility:
form {
display: flex;
+ &:focus-within {
+ outline: 2px solid var(--vscode-focusBorder);
+ outline-offset: -1px;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| form { | |
| display: flex; | |
| } | |
| #chatForm { | |
| background: var(--vscode-input-background); | |
| border: 1px solid var(--vscode-input-border); | |
| border-radius: 0.375rem 0.375rem 0 0; | |
| } | |
| form { | |
| display: flex; | |
| &:focus-within { | |
| outline: 2px solid var(--vscode-focusBorder); | |
| outline-offset: -1px; | |
| } | |
| } | |
| #chatForm { | |
| background: var(--vscode-input-background); | |
| border: 1px solid var(--vscode-input-border); | |
| border-radius: 0.375rem 0.375rem 0 0; | |
| } |
| .copy-button { | ||
| position: absolute; | ||
| bottom: 8px; | ||
| right: 8px; | ||
| background-color: var(--vscode-button-background); | ||
| color: var(--vscode-button-foreground); | ||
| border: none; | ||
| border-radius: 4px; | ||
| padding: 4px 8px; | ||
| font-size: 12px; | ||
| cursor: pointer; | ||
| } | ||
|
|
||
| .copy-button:hover { | ||
| background-color: var(--vscode-button-hoverBackground); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility features to copy button
The copy button needs proper focus states and ARIA labels:
.copy-button {
/* existing styles */
+ &:focus {
+ outline: 2px solid var(--vscode-focusBorder);
+ outline-offset: 2px;
+ }
+ &:focus:not(:focus-visible) {
+ outline: none;
+ }
+ &[aria-pressed="true"] {
+ background-color: var(--vscode-button-secondaryBackground);
+ }
}Also, ensure the HTML includes:
aria-label="Copy code"aria-pressedstate for feedback
Committable suggestion skipped: line range outside the PR's diff.
| .tree-view { | ||
| background-color: var(--vscode-editor-background); | ||
| color: var(--vscode-editor-foreground); | ||
| width: 256px; | ||
| max-height: 300px; | ||
| overflow: auto; | ||
| border: 2px var(--vscode-editor-foreground) solid; | ||
| border-radius: 0.375rem; | ||
| box-shadow: | ||
| 0 4px 6px -1px rgba(0, 0, 0, 0.1), | ||
| 0 2px 4px -1px rgba(0, 0, 0, 0.06); | ||
| } | ||
|
|
||
| .tree-item { | ||
| display: flex; | ||
| align-items: center; | ||
| cursor: pointer; | ||
| } | ||
|
|
||
| .tree-item:hover { | ||
| background-color: var(--vscode-list-background); | ||
| } | ||
|
|
||
| .tree-item-content { | ||
| display: flex; | ||
| align-items: center; | ||
| flex-grow: 1; | ||
| padding: 2px 8px; | ||
| } | ||
|
|
||
| .tree-item-icon { | ||
| width: 16px; | ||
| height: 16px; | ||
| display: inline-flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| } | ||
|
|
||
| .tree-item-name { | ||
| font-size: 14px; | ||
| color: var(--vscode-editor-foreground); | ||
| margin-left: 4px; | ||
| flex-grow: 1; | ||
| } | ||
|
|
||
| .tree-item-children { | ||
| display: none; | ||
| width: 100%; | ||
| } | ||
|
|
||
| .tree-item-children.open { | ||
| display: block; | ||
| } | ||
|
|
||
| .tree-item-wrapper { | ||
| display: flex; | ||
| flex-direction: column; | ||
| width: 100%; | ||
| } | ||
|
|
||
| .tree-item-checkbox { | ||
| width: 16px; | ||
| height: 16px; | ||
| margin-left: 8px; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve tree view accessibility
The tree view component needs ARIA attributes and keyboard navigation support. Consider adding these CSS properties to support accessibility features:
.tree-view {
/* existing styles */
+ &:focus-within {
+ outline: 2px solid var(--vscode-focusBorder);
+ outline-offset: -1px;
+ }
}
.tree-item {
/* existing styles */
+ &:focus {
+ outline: 2px solid var(--vscode-focusBorder);
+ outline-offset: -1px;
+ }
}
.tree-item-checkbox {
/* existing styles */
+ &:focus {
+ outline: 2px solid var(--vscode-focusBorder);
+ outline-offset: 2px;
+ }
}Also, ensure corresponding HTML elements have proper ARIA attributes:
role="tree"on the tree viewrole="treeitem"on tree itemsaria-expandedfor expandable items
Committable suggestion skipped: line range outside the PR's diff.
| case 'showInfo': { | ||
| vscode.window.showInformationMessage({ | ||
| command: 'showInfoToExtension', | ||
| text: message.text, | ||
| }); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Fix incorrect usage of vscode.window in webview context
The webview context doesn't have direct access to vscode.window. Instead, send a message to the extension host.
case 'showInfo': {
- vscode.window.showInformationMessage({
- command: 'showInfoToExtension',
- text: message.text,
- });
+ vscode.postMessage({
+ command: 'showInfoToExtension',
+ text: message.text
+ });
break;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case 'showInfo': { | |
| vscode.window.showInformationMessage({ | |
| command: 'showInfoToExtension', | |
| text: message.text, | |
| }); | |
| break; | |
| } | |
| case 'showInfo': { | |
| vscode.postMessage({ | |
| command: 'showInfoToExtension', | |
| text: message.text | |
| }); | |
| break; | |
| } |
| // Sends a message to the LLM | ||
| function sendMessage(event) { | ||
| event.preventDefault(); | ||
| if (isInputDisabled) { | ||
| return; | ||
| } | ||
|
|
||
| const message = messageInput.value.trim(); | ||
| const selectedProvider = localStorage.getItem('selectedProvider'); | ||
| const selectedModel = localStorage.getItem('selectedModel'); | ||
| const context = JSON.parse(localStorage.getItem('selectedContexts')) || []; | ||
|
|
||
| if (message) { | ||
| vscode.postMessage({ | ||
| command: 'sendMessage', | ||
| text: message, | ||
| context: context, | ||
| provider: selectedProvider, | ||
| model: selectedModel, | ||
| }); | ||
|
|
||
| event.target.reset(); | ||
| saveState(); // Save state before refresh | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix variable hoisting and improve message validation
- The
isInputDisabledvariable is used before its declaration at line 201. - The message validation could be more robust.
+let isInputDisabled = false;
+
function sendMessage(event) {
event.preventDefault();
if (isInputDisabled) {
return;
}
const message = messageInput.value.trim();
+ if (!message || message.length > 4000) { // Add reasonable limit
+ vscode.postMessage({
+ command: 'showInfo',
+ text: message.length > 4000 ? 'Message too long' : 'Message cannot be empty'
+ });
+ return;
+ }
const selectedProvider = localStorage.getItem('selectedProvider');
const selectedModel = localStorage.getItem('selectedModel');
const context = JSON.parse(localStorage.getItem('selectedContexts')) || [];
- if (message) {
vscode.postMessage({
command: 'sendMessage',
text: message,
context: context,
provider: selectedProvider,
model: selectedModel,
});
event.target.reset();
saveState();
- }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Sends a message to the LLM | |
| function sendMessage(event) { | |
| event.preventDefault(); | |
| if (isInputDisabled) { | |
| return; | |
| } | |
| const message = messageInput.value.trim(); | |
| const selectedProvider = localStorage.getItem('selectedProvider'); | |
| const selectedModel = localStorage.getItem('selectedModel'); | |
| const context = JSON.parse(localStorage.getItem('selectedContexts')) || []; | |
| if (message) { | |
| vscode.postMessage({ | |
| command: 'sendMessage', | |
| text: message, | |
| context: context, | |
| provider: selectedProvider, | |
| model: selectedModel, | |
| }); | |
| event.target.reset(); | |
| saveState(); // Save state before refresh | |
| } | |
| } | |
| let isInputDisabled = false; | |
| // Sends a message to the LLM | |
| function sendMessage(event) { | |
| event.preventDefault(); | |
| if (isInputDisabled) { | |
| return; | |
| } | |
| const message = messageInput.value.trim(); | |
| if (!message || message.length > 4000) { // Add reasonable limit | |
| vscode.postMessage({ | |
| command: 'showInfo', | |
| text: message.length > 4000 ? 'Message too long' : 'Message cannot be empty' | |
| }); | |
| return; | |
| } | |
| const selectedProvider = localStorage.getItem('selectedProvider'); | |
| const selectedModel = localStorage.getItem('selectedModel'); | |
| const context = JSON.parse(localStorage.getItem('selectedContexts')) || []; | |
| vscode.postMessage({ | |
| command: 'sendMessage', | |
| text: message, | |
| context: context, | |
| provider: selectedProvider, | |
| model: selectedModel, | |
| }); | |
| event.target.reset(); | |
| saveState(); | |
| } |
resources/chat-view/chat.js
Outdated
| function toggleTreeItemBoxes() { | ||
| const pipelineRunsBox = document.querySelector( | ||
| 'input[type="checkbox"][value="pipelineContext"]' | ||
| ); | ||
|
|
||
| const pipelineRunBoxes = document.querySelectorAll( | ||
| 'input[type="checkbox"][value*="Pipeline Run:"]' | ||
| ); | ||
|
|
||
| // Condition: If the main checkbox is clicked, all children checkboxes should be checked. | ||
| if (pipelineRunsBox.checked) { | ||
| pipelineRunBoxes.forEach(checkbox => (checkbox.checked = true)); | ||
| console.log('checked boxes: ', pipelineRunBoxes); | ||
| } else { | ||
| pipelineRunBoxes.forEach(checkbox => (checkbox.checked = false)); | ||
| } | ||
|
|
||
| // Condition: If any of the children checkboxes are unchecked, then the main checkbox should be checked. | ||
| function updateMainCheckbox() { | ||
| const anyUnchecked = Array.from(pipelineRunBoxes).some(checkbox => !checkbox.checked); | ||
| pipelineRunsBox.checked = !anyUnchecked; | ||
| } | ||
|
|
||
| pipelineRunBoxes.forEach(checkbox => { | ||
| checkbox.addEventListener('change', updateMainCheckbox); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor checkbox assignments and fix event listener memory leak
- The assignments in expressions should be moved out for better readability.
- The event listeners are added repeatedly without cleanup, which could cause memory leaks.
function toggleTreeItemBoxes() {
const pipelineRunsBox = document.querySelector(
'input[type="checkbox"][value="pipelineContext"]'
);
const pipelineRunBoxes = document.querySelectorAll(
'input[type="checkbox"][value*="Pipeline Run:"]'
);
if (pipelineRunsBox.checked) {
- pipelineRunBoxes.forEach(checkbox => (checkbox.checked = true));
+ pipelineRunBoxes.forEach(checkbox => {
+ checkbox.checked = true;
+ });
} else {
- pipelineRunBoxes.forEach(checkbox => (checkbox.checked = false));
+ pipelineRunBoxes.forEach(checkbox => {
+ checkbox.checked = false;
+ });
}
function updateMainCheckbox() {
const anyUnchecked = Array.from(pipelineRunBoxes).some(checkbox => !checkbox.checked);
pipelineRunsBox.checked = !anyUnchecked;
}
+ // Remove existing listeners before adding new ones
+ pipelineRunBoxes.forEach(checkbox => {
+ checkbox.removeEventListener('change', updateMainCheckbox);
+ });
pipelineRunBoxes.forEach(checkbox => {
checkbox.addEventListener('change', updateMainCheckbox);
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function toggleTreeItemBoxes() { | |
| const pipelineRunsBox = document.querySelector( | |
| 'input[type="checkbox"][value="pipelineContext"]' | |
| ); | |
| const pipelineRunBoxes = document.querySelectorAll( | |
| 'input[type="checkbox"][value*="Pipeline Run:"]' | |
| ); | |
| // Condition: If the main checkbox is clicked, all children checkboxes should be checked. | |
| if (pipelineRunsBox.checked) { | |
| pipelineRunBoxes.forEach(checkbox => (checkbox.checked = true)); | |
| console.log('checked boxes: ', pipelineRunBoxes); | |
| } else { | |
| pipelineRunBoxes.forEach(checkbox => (checkbox.checked = false)); | |
| } | |
| // Condition: If any of the children checkboxes are unchecked, then the main checkbox should be checked. | |
| function updateMainCheckbox() { | |
| const anyUnchecked = Array.from(pipelineRunBoxes).some(checkbox => !checkbox.checked); | |
| pipelineRunsBox.checked = !anyUnchecked; | |
| } | |
| pipelineRunBoxes.forEach(checkbox => { | |
| checkbox.addEventListener('change', updateMainCheckbox); | |
| }); | |
| } | |
| function toggleTreeItemBoxes() { | |
| const pipelineRunsBox = document.querySelector( | |
| 'input[type="checkbox"][value="pipelineContext"]' | |
| ); | |
| const pipelineRunBoxes = document.querySelectorAll( | |
| 'input[type="checkbox"][value*="Pipeline Run:"]' | |
| ); | |
| if (pipelineRunsBox.checked) { | |
| pipelineRunBoxes.forEach(checkbox => { | |
| checkbox.checked = true; | |
| }); | |
| console.log('checked boxes: ', pipelineRunBoxes); | |
| } else { | |
| pipelineRunBoxes.forEach(checkbox => { | |
| checkbox.checked = false; | |
| }); | |
| } | |
| function updateMainCheckbox() { | |
| const anyUnchecked = Array.from(pipelineRunBoxes).some(checkbox => !checkbox.checked); | |
| pipelineRunsBox.checked = !anyUnchecked; | |
| } | |
| // Remove existing listeners before adding new ones | |
| pipelineRunBoxes.forEach(checkbox => { | |
| checkbox.removeEventListener('change', updateMainCheckbox); | |
| }); | |
| pipelineRunBoxes.forEach(checkbox => { | |
| checkbox.addEventListener('change', updateMainCheckbox); | |
| }); | |
| } |
🧰 Tools
🪛 Biome
[error] 99-99: 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] 102-102: 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)
Summary
This pull request introduces a comprehensive set of features and improvements to the chatbot functionality within the VSCode ZenML extension. The updates enhance user interaction, context management, and overall user experience.
Chat Interface Enhancements:
Context Management:
Pipeline Integration:
UI and UX Improvements:
Contributors
@wyennie
@Nathaniel-Xu
@alan-cho
Summary by CodeRabbit
New Features
Bug Fixes
Style
Documentation
Tests