🌱 generalize errors held in extension state #582
Conversation
- Hold an array of ConfigErrors on the extension data for better flexibility - Log errors (and leave earlier) in solution server client Signed-off-by: David Zager <[email protected]>
WalkthroughThis change refactors configuration validation throughout the codebase, replacing boolean flags with a new error-object-based model ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant VSCodeExtension
participant ConfigurationModule
participant UI
User->>VSCodeExtension: Change settings / profile
VSCodeExtension->>ConfigurationModule: updateConfigErrors()
ConfigurationModule->>VSCodeExtension: configErrors[]
VSCodeExtension->>UI: Provide configErrors[]
UI->>UI: Render warnings/errors based on configErrors[]
sequenceDiagram
participant Client
participant SolutionServerClient
participant RemoteServer
Client->>SolutionServerClient: Call public async method
SolutionServerClient->>SolutionServerClient: Check connection and client ID
alt Not connected or client ID missing
SolutionServerClient->>Client: Log error, return default/empty result
else Connected and client ID present
SolutionServerClient->>RemoteServer: Proceed with remote call
RemoteServer-->>SolutionServerClient: Response
SolutionServerClient-->>Client: Return result
end
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
webview-ui/src/components/AnalysisPage/AnalysisPage.tsx (1)
58-59: Remove unused import.The
PageHeaderimport is not used in the component and should be removed to clean up the imports.-import { Incident } from "@editor-extensions/shared"; -import { PageHeader } from "@patternfly/react-component-groups"; +import { Incident } from "@editor-extensions/shared";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
agentic/src/clients/solutionServerClient.ts(7 hunks)shared/src/types/types.ts(1 hunks)vscode/src/extension.ts(5 hunks)vscode/src/utilities/configuration.ts(4 hunks)vscode/src/webviewMessageHandler.ts(6 hunks)webview-ui/src/components/AnalysisPage/AnalysisPage.tsx(4 hunks)webview-ui/src/components/AnalysisPage/WalkthroughDrawer/WalkthroughDrawer.tsx(1 hunks)webview-ui/src/context/ExtensionStateContext.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
webview-ui/src/components/AnalysisPage/WalkthroughDrawer/WalkthroughDrawer.tsx (1)
Learnt from: ibolton336
PR: konveyor/editor-extensions#558
File: vscode/src/webviewMessageHandler.ts:228-229
Timestamp: 2025-06-30T15:26:08.027Z
Learning: Provider configuration status should be determined by actual provider settings (API keys, credentials, etc.) and should not be tied to active analysis profiles. Provider configuration is global and independent of which analysis profile is currently active.
webview-ui/src/context/ExtensionStateContext.tsx (1)
Learnt from: ibolton336
PR: konveyor/editor-extensions#558
File: vscode/src/webviewMessageHandler.ts:228-229
Timestamp: 2025-06-30T15:26:08.027Z
Learning: Provider configuration status should be determined by actual provider settings (API keys, credentials, etc.) and should not be tied to active analysis profiles. Provider configuration is global and independent of which analysis profile is currently active.
vscode/src/extension.ts (1)
Learnt from: ibolton336
PR: konveyor/editor-extensions#558
File: vscode/src/webviewMessageHandler.ts:228-229
Timestamp: 2025-06-30T15:26:08.027Z
Learning: Provider configuration status should be determined by actual provider settings (API keys, credentials, etc.) and should not be tied to active analysis profiles. Provider configuration is global and independent of which analysis profile is currently active.
vscode/src/webviewMessageHandler.ts (1)
Learnt from: ibolton336
PR: konveyor/editor-extensions#558
File: vscode/src/webviewMessageHandler.ts:228-229
Timestamp: 2025-06-30T15:26:08.027Z
Learning: Provider configuration status should be determined by actual provider settings (API keys, credentials, etc.) and should not be tied to active analysis profiles. Provider configuration is global and independent of which analysis profile is currently active.
webview-ui/src/components/AnalysisPage/AnalysisPage.tsx (1)
Learnt from: ibolton336
PR: konveyor/editor-extensions#558
File: vscode/src/webviewMessageHandler.ts:228-229
Timestamp: 2025-06-30T15:26:08.027Z
Learning: Provider configuration status should be determined by actual provider settings (API keys, credentials, etc.) and should not be tied to active analysis profiles. Provider configuration is global and independent of which analysis profile is currently active.
vscode/src/utilities/configuration.ts (1)
Learnt from: ibolton336
PR: konveyor/editor-extensions#558
File: vscode/src/webviewMessageHandler.ts:228-229
Timestamp: 2025-06-30T15:26:08.027Z
Learning: Provider configuration status should be determined by actual provider settings (API keys, credentials, etc.) and should not be tied to active analysis profiles. Provider configuration is global and independent of which analysis profile is currently active.
🧬 Code Graph Analysis (2)
vscode/src/webviewMessageHandler.ts (1)
shared/src/types/types.ts (2)
ExtensionData(143-165)createConfigError(180-210)
vscode/src/utilities/configuration.ts (1)
shared/src/types/types.ts (2)
ExtensionData(143-165)createConfigError(180-210)
🪛 GitHub Check: Lint
webview-ui/src/components/AnalysisPage/AnalysisPage.tsx
[warning] 59-59:
'PageHeader' is defined but never used
[warning] 59-59:
'PageHeader' is defined but never used
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build (windows)
- GitHub Check: Build (linux)
- GitHub Check: Build (macos)
🔇 Additional comments (25)
agentic/src/clients/solutionServerClient.ts (6)
143-148: Good defensive programming - consistent error handling for disconnected state.The connection validation provides clear error messages and appropriate early returns when the client is not connected. This prevents undefined behavior and makes debugging easier.
237-247: Excellent addition of client ID validation.The check for
currentClientIdis crucial for incident creation operations. The early return with-1provides a consistent error indicator that callers can handle appropriately.
304-322: Comprehensive error handling for bulk operations.The validation logic properly handles both connection state and client ID requirements. The return structure maintains consistency with the expected interface while clearly indicating failure states.
392-402: Consistent validation pattern applied to solution creation.The dual validation (connection + client ID) follows the same pattern as incident creation, maintaining consistency across the API.
460-465: Appropriate handling for optional operations.Returning
undefinedfor hint retrieval when disconnected is semantically correct since hints are optional enhancements rather than required operations.
525-530: File operations properly guarded.The connection checks for accept/reject file operations prevent attempting file operations when the server is unavailable, avoiding potential errors.
Also applies to: 559-564
shared/src/types/types.ts (4)
161-161: Excellent architectural improvement - from boolean flags to structured errors.Replacing
analysisConfigwithconfigErrorsprovides much more flexibility and better user experience through detailed error information instead of just boolean flags.
167-173: Well-designed error type enumeration.The
ConfigErrorTypeunion covers all the essential configuration error scenarios and provides clear, semantic error categories.
175-178: Clean and simple error interface.The
ConfigErrorinterface strikes the right balance between simplicity and expressiveness with just type and message fields.
180-210: Excellent factory pattern implementation.The
createConfigErrorobject provides consistent error creation with standardized messages. This ensures uniform error messaging across the application and makes maintenance easier.webview-ui/src/context/ExtensionStateContext.tsx (1)
24-24: Correct default state alignment with new error structure.The empty
configErrorsarray is the appropriate default value for the new error-based configuration system.vscode/src/extension.ts (3)
59-59: Consistent state initialization with new error model.The initialization with an empty
configErrorsarray aligns perfectly with the new error-based configuration validation approach.
117-117: Proper function name updates for error-based validation.The calls to
updateConfigErrors(renamed fromupdateAnalysisConfig) maintain the same validation functionality while aligning with the new error-centric approach.Also applies to: 143-143
291-298: Insightful demonstration of new error model flexibility.The commented code elegantly shows how the new approach could handle edge cases (like no workspace) more gracefully by storing error state instead of throwing exceptions. This could provide better user experience in the future.
webview-ui/src/components/AnalysisPage/WalkthroughDrawer/WalkthroughDrawer.tsx (1)
46-52: Clean adaptation to new error-based configuration model.The logic correctly derives provider configuration status from the presence of specific error types in the
configErrorsarray. Usingsome()to check for error presence is efficient and readable.vscode/src/webviewMessageHandler.ts (3)
33-33: LGTM: Import added for error factory functions.The addition of
createConfigErrorimport aligns with the refactoring to use error objects instead of boolean flags.
69-69: LGTM: Function calls updated consistently.All calls to the renamed function
updateConfigErrorsFromActiveProfileare correctly updated throughout the file.Also applies to: 87-87, 116-116, 130-130
227-255: LGTM: Well-structured error-based configuration validation.The refactored function properly implements the error-object-based approach:
- Selective error clearing: Only removes profile-related errors (
no-active-profile,invalid-label-selector,no-custom-rules) while preserving other error types- Comprehensive validation: Checks for active profile, label selector, and custom rules configuration
- Early return pattern: Returns early when no active profile is found to avoid unnecessary checks
- Consistent error creation: Uses the
createConfigErrorfactory functions for type-safe error creationThe logic correctly handles all the configuration validation scenarios identified in the PR objectives.
webview-ui/src/components/AnalysisPage/AnalysisPage.tsx (3)
71-71: LGTM: State destructuring updated for error-based validation.The change from
analysisConfigtoconfigErrorscorrectly aligns with the refactoring to use error objects instead of boolean flags.
149-150: LGTM: Improved warning message aggregation.The warning logic now properly aggregates multiple error messages by joining them with newlines, providing better visibility into all configuration issues at once.
201-206: LGTM: Simplified and consistent error display.The conditional rendering now uses
configErrors.length > 0for better clarity, and the alert provides a consistent message directing users to review their configuration. This is more user-friendly than the previous dynamic message approach.vscode/src/utilities/configuration.ts (4)
10-10: LGTM: Import added for error factory functions.The addition of
createConfigErrorimport supports the refactoring to use error objects instead of boolean flags.
222-252: LGTM: Comprehensive error-based configuration validation.The refactored
updateConfigErrorsfunction provides significant improvements:
- Complete error reset: Clears all existing errors before validation to ensure fresh state
- Comprehensive validation: Checks active profile, label selector, custom rules, and provider configuration
- Early return pattern: Returns early when no active profile is found to avoid unnecessary checks
- Provider validation integration: Incorporates GenAI provider status checking with appropriate error types
- Consistent error creation: Uses the
createConfigErrorfactory functions for type-safe error creationThe logic correctly handles all configuration validation scenarios and provides better granularity than the previous boolean flag approach.
304-304: LGTM: Configuration change listener updated.The call to
updateConfigErrorsis correctly updated in the configuration change listener to maintain consistency with the refactoring.
321-333: LGTM: Profile validity function updated for error-based approach.The
updateActiveProfileValidityfunction correctly implements the error-based approach:
- Selective error filtering: Only removes profile-related errors (
invalid-label-selector,no-custom-rules) while preserving others- Validation logic: Checks label selector and custom rules configuration
- Consistent error creation: Uses the
createConfigErrorfactory functionsThe function maintains the same validation logic while adapting to the new error-based model.
Signed-off-by: David Zager <[email protected]>
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
webview-ui/src/components/AnalysisPage/AnalysisPage.tsx (1)
201-215: Improve error rendering implementation.The error rendering has several issues that need attention:
- Use stable keys: Using array index as React key can cause rendering issues when the array changes
- Style conflicts:
marginTopis applied but then overridden bymargin- Inconsistent with past feedback: The title format doesn't match the suggested format from previous reviews
Apply this diff to address these issues:
- {configErrors.length > 0 && ( - <PageSection padding={{ default: "noPadding" }}> - {configErrors.map((error, index) => ( - <Card - isCompact - style={{ maxWidth: "600px", marginTop: "1rem", margin: "0 auto" }} - key={index} - > - <Alert variant="warning" title={error.message}> - {error.error?.message && error.error.message} - </Alert> - </Card> - ))} - </PageSection> - )} + {configErrors.length > 0 && ( + <PageSection padding={{ default: "noPadding" }}> + {configErrors.map((configError) => ( + <Card + isCompact + style={{ maxWidth: "600px", margin: "1rem auto 0" }} + key={`${configError.message}-${configError.error?.slice(0, 50) || 'no-error'}`} + > + <Alert + variant="warning" + title={`${configError.message} - ${configError.error?.slice(0, 150) || ''}`} + > + {configError.error && configError.error.length > 150 && ( + <div>{configError.error.slice(150)}</div> + )} + </Alert> + </Card> + ))} + </PageSection> + )}
🧹 Nitpick comments (1)
webview-ui/src/components/AnalysisPage/AnalysisPage.tsx (1)
59-59: Remove unused import.The
PageHeaderimport is not used anywhere in the component.-import { PageHeader } from "@patternfly/react-component-groups";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
shared/src/types/types.ts(1 hunks)webview-ui/src/components/AnalysisPage/AnalysisPage.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- shared/src/types/types.ts
🧰 Additional context used
🧠 Learnings (1)
webview-ui/src/components/AnalysisPage/AnalysisPage.tsx (1)
Learnt from: ibolton336
PR: konveyor/editor-extensions#558
File: vscode/src/webviewMessageHandler.ts:228-229
Timestamp: 2025-06-30T15:26:08.027Z
Learning: Provider configuration status should be determined by actual provider settings (API keys, credentials, etc.) and should not be tied to active analysis profiles. Provider configuration is global and independent of which analysis profile is currently active.
🪛 GitHub Check: Lint
webview-ui/src/components/AnalysisPage/AnalysisPage.tsx
[warning] 59-59:
'PageHeader' is defined but never used
[warning] 59-59:
'PageHeader' is defined but never used
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build (windows)
- GitHub Check: Build (macos)
- GitHub Check: Build (linux)
🔇 Additional comments (1)
webview-ui/src/components/AnalysisPage/AnalysisPage.tsx (1)
149-150: Configuration warning logic updated correctly.The change from checking a single configuration flag to using
configErrors.length > 0aligns well with the new error handling approach. The hardcoded warning message provides clear guidance to users.
Summary by CodeRabbit