🐛 analyzer and genai config fixes #727
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: David Zager <[email protected]>
djzager
force-pushed
the
analyzer-startup
branch
from
c427f1b to
5f48b8f
Compare
Walkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vscode/src/utilities/configuration.ts (1)
156-158: Remove duplicate/typo’d API: updateConfigActiveProfileIdfileId.
This duplicates functionality (correct version exists at Lines 174-176) and risks accidental use.Apply this diff:
-export const updateConfigActiveProfileIdfileId = async (id: string) => { - return updateConfigValue("activeProfileId", id, vscode.ConfigurationTarget.Workspace); -};
🧹 Nitpick comments (6)
vscode/src/utilities/configuration.ts (1)
104-106: Optional: provide a disable counterpart for symmetry and tests.
Handy for tests and UI toggles.export const enableGenAI = async (): Promise<void> => { await updateConfigValue("genai.enabled", true, vscode.ConfigurationTarget.Workspace); }; + +export const disableGenAI = async (): Promise<void> => { + await updateConfigValue("genai.enabled", false, vscode.ConfigurationTarget.Workspace); +};shared/src/types/actions.ts (1)
28-55: Nit: WEBVIEW_READY appears twice in the union.
Harmless but noisy; consider deduping.| typeof OPEN_GENAI_SETTINGS | typeof CONFIGURE_CUSTOM_RULES - | typeof WEBVIEW_READY | typeof SET_ACTIVE_PROFILEwebview-ui/src/components/AnalysisPage/ConfigAlerts.tsx (1)
32-35: Avoid conflicting inline margins on CardYou set both marginTop and margin; margin can override the top. Simplify to a single margin shorthand.
- style={{ maxWidth: "600px", marginTop: "1rem", margin: "0 auto" }} + style={{ maxWidth: "600px", margin: "1rem auto 0" }}And similarly below:
- <Card isCompact style={{ maxWidth: "600px", marginTop: "1rem", margin: "0 auto" }}> + <Card isCompact style={{ maxWidth: "600px", margin: "1rem auto 0" }}>Also applies to: 62-63
vscode/src/commands.ts (1)
794-803: Improve error rendering to avoid “[object Object]”Show a meaningful message when enabling GenAI fails.
- } catch (error) { - logger.error("Error enabling GenAI:", error); - window.showErrorMessage(`Failed to enable GenAI: ${error}`); + } catch (error) { + logger.error("Error enabling GenAI:", { error }); + const msg = error instanceof Error ? error.message : String(error); + window.showErrorMessage(`Failed to enable GenAI: ${msg}`); }webview-ui/src/components/AnalysisPage/WalkthroughDrawer/WalkthroughDrawer.tsx (1)
53-57: Solid UX for disabled GenAI; address lint nitsLogic for genaiDisabled title/status/button is correct. Fix formatting per linter.
- const genaiDisabled = state.configErrors.some( - (error) => error.type === "genai-disabled", - ); + const genaiDisabled = state.configErrors.some((error) => error.type === "genai-disabled");- <Button - variant="link" - onClick={() => dispatch(enableGenAI())} - > + <Button variant="link" onClick={() => dispatch(enableGenAI())}> Enable GenAI </Button>Also applies to: 79-90, 181-194
vscode/src/paths.ts (1)
61-93: Respect user-configured analyzer path; add file-type guard (optional)Good early return on valid user path. Consider verifying it’s a regular file to avoid directories with execute bit.
- const isValid = await checkIfExecutable(userAnalyzerPath); + const isValid = await checkIfExecutable(userAnalyzerPath); + if (isValid) { + try { + const { stat } = await import("node:fs/promises"); + const s = await stat(userAnalyzerPath); + if (!s.isFile()) { + logger.warn(`Analyzer path is not a file: ${userAnalyzerPath}`); + // Treat as invalid + throw new Error("Path is not a file"); + } + } catch { + // fall through to invalid path branch + } + } if (!isValid) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
shared/src/types/actions.ts(2 hunks)vscode/src/commands.ts(2 hunks)vscode/src/extension.ts(4 hunks)vscode/src/paths.ts(2 hunks)vscode/src/utilities/configuration.ts(1 hunks)vscode/src/webviewMessageHandler.ts(2 hunks)webview-ui/src/components/AnalysisPage/AnalysisPage.tsx(1 hunks)webview-ui/src/components/AnalysisPage/ConfigAlerts.tsx(2 hunks)webview-ui/src/components/AnalysisPage/WalkthroughDrawer/WalkthroughDrawer.tsx(5 hunks)webview-ui/src/components/GetSolutionDropdown.tsx(0 hunks)webview-ui/src/hooks/actions.ts(1 hunks)
💤 Files with no reviewable changes (1)
- webview-ui/src/components/GetSolutionDropdown.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: abrugaro
PR: konveyor/editor-extensions#657
File: tests/global.setup.ts:22-27
Timestamp: 2025-08-05T16:16:56.005Z
Learning: In the Playwright test framework for konveyor/editor-extensions, the global setup in tests/global.setup.ts is designed to handle only essential initialization like extension installation verification. Individual tests are responsible for configuring GenAI when needed, rather than doing it globally. The openAnalysisView() method is used as a fast verification mechanism to ensure the extension was successfully installed, which is more efficient than configuring GenAI globally for all tests.
📚 Learning: 2025-08-01T13:53:20.774Z
Learnt from: djzager
PR: konveyor/editor-extensions#645
File: vscode/src/webviewMessageHandler.ts:575-577
Timestamp: 2025-08-01T13:53:20.774Z
Learning: The TOGGLE_AGENT_MODE action handler in vscode/src/webviewMessageHandler.ts correctly calls toggleAgentMode() without parameters, as the toggleAgentMode function in vscode/src/utilities/configuration.ts has been updated to internally handle reading and toggling the agent mode configuration value.
Applied to files:
vscode/src/utilities/configuration.tsvscode/src/webviewMessageHandler.ts
📚 Learning: 2025-08-05T16:16:56.005Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#657
File: tests/global.setup.ts:22-27
Timestamp: 2025-08-05T16:16:56.005Z
Learning: In the Playwright test framework for konveyor/editor-extensions, the global setup in tests/global.setup.ts is designed to handle only essential initialization like extension installation verification. Individual tests are responsible for configuring GenAI when needed, rather than doing it globally. The openAnalysisView() method is used as a fast verification mechanism to ensure the extension was successfully installed, which is more efficient than configuring GenAI globally for all tests.
Applied to files:
webview-ui/src/components/AnalysisPage/WalkthroughDrawer/WalkthroughDrawer.tsx
📚 Learning: 2025-08-15T20:32:49.398Z
Learnt from: djzager
PR: konveyor/editor-extensions#685
File: agentic/src/clients/solutionServerClient.ts:62-68
Timestamp: 2025-08-15T20:32:49.398Z
Learning: The SolutionServerClient constructor in vscode/src/extension.ts has been correctly updated to include all 5 parameters: serverUrl, enabled, authEnabled, insecure, and logger, matching the new constructor signature in the authentication PR.
Applied to files:
vscode/src/extension.ts
🧬 Code graph analysis (9)
webview-ui/src/hooks/actions.ts (2)
vscode/src/utilities/configuration.ts (1)
enableGenAI(104-106)shared/src/types/actions.ts (2)
WebviewAction(56-59)WebviewActionType(28-55)
vscode/src/utilities/configuration.ts (1)
webview-ui/src/hooks/actions.ts (1)
enableGenAI(38-41)
webview-ui/src/components/AnalysisPage/ConfigAlerts.tsx (2)
vscode/src/utilities/configuration.ts (1)
enableGenAI(104-106)webview-ui/src/hooks/actions.ts (1)
enableGenAI(38-41)
webview-ui/src/components/AnalysisPage/WalkthroughDrawer/WalkthroughDrawer.tsx (1)
webview-ui/src/hooks/actions.ts (1)
enableGenAI(38-41)
vscode/src/paths.ts (2)
vscode/src/utilities/configuration.ts (2)
getConfigAnalyzerPath(19-19)updateAnalyzerPath(96-98)vscode/src/utilities/fileUtils.ts (1)
checkIfExecutable(25-51)
vscode/src/commands.ts (3)
vscode/src/utilities/constants.ts (1)
EXTENSION_NAME(17-17)vscode/src/client/tracer.ts (1)
logger(60-65)vscode/src/utilities/configuration.ts (1)
enableGenAI(104-106)
vscode/src/webviewMessageHandler.ts (2)
shared/src/types/actions.ts (1)
ENABLE_GENAI(6-6)vscode/src/commands.ts (1)
executeExtensionCommand(61-63)
shared/src/types/actions.ts (1)
vscode/src/webviewMessageHandler.ts (1)
ENABLE_GENAI(241-243)
vscode/src/extension.ts (2)
vscode/src/utilities/configuration.ts (2)
getConfigSolutionServerEnabled(22-23)getConfigSolutionServerAuth(24-25)vscode/src/paths.ts (1)
ensureKaiAnalyzerBinary(61-182)
🪛 GitHub Check: Lint
webview-ui/src/components/AnalysisPage/WalkthroughDrawer/WalkthroughDrawer.tsx
[warning] 53-53:
Replace ⏎····(error)·=>·error.type·===·"genai-disabled",⏎·· with (error)·=>·error.type·===·"genai-disabled"
[warning] 181-181:
Replace ⏎····························variant="link"⏎····························onClick={()·=>·dispatch(enableGenAI())}⏎·························· with ·variant="link"·onClick={()·=>·dispatch(enableGenAI())}
⏰ 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). (1)
- GitHub Check: Build (windows)
🔇 Additional comments (14)
vscode/src/utilities/configuration.ts (2)
104-106: Enable function is fine; keep as-is.
Sets the workspace-scoped flag and aligns with existing setters.
67-68: Default GenAI enablement must be false.
Apply:-export const getConfigGenAIEnabled = (): boolean => - getConfigValue<boolean>("genai.enabled") ?? true; +export const getConfigGenAIEnabled = (): boolean => + getConfigValue<boolean>("genai.enabled") ?? false;And verify your
package.jsonundercontributes.configuration.properties.genai.enabledis declared with"default": false.webview-ui/src/components/AnalysisPage/AnalysisPage.tsx (1)
221-221: LGTM: formatting-only callback inline.
No behavioral change; consistent with surrounding dispatch usage.vscode/src/webviewMessageHandler.ts (2)
25-26: Importing ENABLE_GENAI is correct and scoped.
Matches the shared constant; avoids stringly-typed action names.
241-243: Verify enableGenAI registration and state refresh
No registration for${EXTENSION_NAME}.enableGenAIwas found; ensure you’ve addedcontext.subscriptions.push( vscode.commands.registerCommand( `${EXTENSION_NAME}.enableGenAI`, /* handler that enables GenAI and dispatches a webview state update to clear “genai-disabled” */ ) )in your extension activation (e.g.
extension.ts), and that its handler signals the webview to clear the “genai-disabled” error.webview-ui/src/hooks/actions.ts (1)
38-41: New action creator is consistent and typed.
Aligns with existing action creators and shared types.shared/src/types/actions.ts (2)
6-6: New ENABLE_GENAI constant added correctly.
Name and style consistent with existing constants.
34-34: Union type update is correct.
Ensures type safety for the new action across the app.webview-ui/src/components/AnalysisPage/ConfigAlerts.tsx (1)
4-4: Good integration: add “Enable GenAI” action for genai-disabledHooking ENABLE_GENAI through the UI is correct and consistent with actions.ts.
Also applies to: 45-48
vscode/src/commands.ts (2)
33-35: Import enableGenAI is correctly wiredBringing enableGenAI in from configuration.ts matches the new command handler.
65-71: Verified:enableGenAIis registered exactly once and correctly wired from the webview handler.vscode/src/extension.ts (3)
231-247: Correctly gate credential prompts on both togglesPrompting only when solution server and auth are enabled prevents unnecessary dialogs.
282-300: Connect to solution server only when enabledGood: avoids unnecessary connection attempts and updates state on success/failure.
403-446: Clearing GenAI-related config errors on setting changes is correctThe filter ensures stale errors don’t linger when enabling/configuring GenAI.
Comment on lines
+490
to
+519
| if (event.affectsConfiguration("konveyor.analyzerPath")) { | ||
| this.state.logger.info("Analyzer path configuration modified!"); | ||
|
|
||
| // Check if server is currently running | ||
| const wasServerRunning = this.state.analyzerClient.isServerRunning(); | ||
|
|
||
| // Stop server if it's running | ||
| if (wasServerRunning) { | ||
| this.state.logger.info("Stopping analyzer server for binary path change..."); | ||
| await this.state.analyzerClient.stop(); | ||
| } | ||
|
|
||
| // Re-ensure the binary (this will validate and reset if needed) | ||
| await ensureKaiAnalyzerBinary(this.context, this.state.logger); | ||
|
|
||
| // Restart server if it was running before | ||
| if (wasServerRunning) { | ||
| this.state.logger.info("Restarting analyzer server after binary path change..."); | ||
| try { | ||
| if (await this.state.analyzerClient.canAnalyzeInteractive()) { | ||
| await this.state.analyzerClient.start(); | ||
| } | ||
| } catch (error) { | ||
| this.state.logger.error("Error restarting analyzer server:", error); | ||
| vscode.window.showErrorMessage( | ||
| `Failed to restart analyzer server after binary path change: ${error}`, | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden analyzer restart flow on analyzerPath changes
Wrap stop/ensure/start with defensive error handling so a failure doesn’t leave the server stuck.
- if (event.affectsConfiguration("konveyor.analyzerPath")) {
+ if (event.affectsConfiguration("konveyor.analyzerPath")) {
this.state.logger.info("Analyzer path configuration modified!");
// Check if server is currently running
const wasServerRunning = this.state.analyzerClient.isServerRunning();
// Stop server if it's running
- if (wasServerRunning) {
- this.state.logger.info("Stopping analyzer server for binary path change...");
- await this.state.analyzerClient.stop();
- }
+ if (wasServerRunning) {
+ this.state.logger.info("Stopping analyzer server for binary path change...");
+ try {
+ await this.state.analyzerClient.stop();
+ } catch (error) {
+ this.state.logger.error("Error stopping analyzer server:", { error });
+ }
+ }
// Re-ensure the binary (this will validate and reset if needed)
- await ensureKaiAnalyzerBinary(this.context, this.state.logger);
+ try {
+ await ensureKaiAnalyzerBinary(this.context, this.state.logger);
+ } catch (error) {
+ this.state.logger.error("Failed to ensure analyzer binary after path change:", {
+ error,
+ });
+ vscode.window.showErrorMessage(
+ `Failed to validate analyzer binary after path change: ${
+ error instanceof Error ? error.message : String(error)
+ }`,
+ );
+ return; // Don't attempt restart if ensure failed
+ }
// Restart server if it was running before
if (wasServerRunning) {
this.state.logger.info("Restarting analyzer server after binary path change...");
try {
if (await this.state.analyzerClient.canAnalyzeInteractive()) {
await this.state.analyzerClient.start();
}
} catch (error) {
- this.state.logger.error("Error restarting analyzer server:", error);
+ this.state.logger.error("Error restarting analyzer server:", { error });
vscode.window.showErrorMessage(
- `Failed to restart analyzer server after binary path change: ${error}`,
+ `Failed to restart analyzer server after binary path change: ${
+ error instanceof Error ? error.message : String(error)
+ }`,
);
}
}
}📝 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.
Suggested change
| if (event.affectsConfiguration("konveyor.analyzerPath")) { | |
| this.state.logger.info("Analyzer path configuration modified!"); | |
| // Check if server is currently running | |
| const wasServerRunning = this.state.analyzerClient.isServerRunning(); | |
| // Stop server if it's running | |
| if (wasServerRunning) { | |
| this.state.logger.info("Stopping analyzer server for binary path change..."); | |
| await this.state.analyzerClient.stop(); | |
| } | |
| // Re-ensure the binary (this will validate and reset if needed) | |
| await ensureKaiAnalyzerBinary(this.context, this.state.logger); | |
| // Restart server if it was running before | |
| if (wasServerRunning) { | |
| this.state.logger.info("Restarting analyzer server after binary path change..."); | |
| try { | |
| if (await this.state.analyzerClient.canAnalyzeInteractive()) { | |
| await this.state.analyzerClient.start(); | |
| } | |
| } catch (error) { | |
| this.state.logger.error("Error restarting analyzer server:", error); | |
| vscode.window.showErrorMessage( | |
| `Failed to restart analyzer server after binary path change: ${error}`, | |
| ); | |
| } | |
| } | |
| } | |
| if (event.affectsConfiguration("konveyor.analyzerPath")) { | |
| this.state.logger.info("Analyzer path configuration modified!"); | |
| // Check if server is currently running | |
| const wasServerRunning = this.state.analyzerClient.isServerRunning(); | |
| // Stop server if it's running | |
| if (wasServerRunning) { | |
| this.state.logger.info("Stopping analyzer server for binary path change..."); | |
| try { | |
| await this.state.analyzerClient.stop(); | |
| } catch (error) { | |
| this.state.logger.error("Error stopping analyzer server:", { error }); | |
| } | |
| } | |
| // Re-ensure the binary (this will validate and reset if needed) | |
| try { | |
| await ensureKaiAnalyzerBinary(this.context, this.state.logger); | |
| } catch (error) { | |
| this.state.logger.error("Failed to ensure analyzer binary after path change:", { error }); | |
| vscode.window.showErrorMessage( | |
| `Failed to validate analyzer binary after path change: ${ | |
| error instanceof Error ? error.message : String(error) | |
| }`, | |
| ); | |
| return; // Don't attempt restart if ensure failed | |
| } | |
| // Restart server if it was running before | |
| if (wasServerRunning) { | |
| this.state.logger.info("Restarting analyzer server after binary path change..."); | |
| try { | |
| if (await this.state.analyzerClient.canAnalyzeInteractive()) { | |
| await this.state.analyzerClient.start(); | |
| } | |
| } catch (error) { | |
| this.state.logger.error("Error restarting analyzer server:", { error }); | |
| vscode.window.showErrorMessage( | |
| `Failed to restart analyzer server after binary path change: ${ | |
| error instanceof Error ? error.message : String(error) | |
| }`, | |
| ); | |
| } | |
| } | |
| } |
4 tasks
hhpatel14
pushed a commit
that referenced
this pull request
Aug 29, 2025
This addresses a few issues: 1. #720 - updated the ensureKaiAnalyzerBinary to handle the user configured and throw vscode error if invalid (and reset)...use it for startup and config changes 2. Only prompt for ss stuff if auth is enabled AND solution server is enabled 3. Fix UX for disabled gen ai case. Fixes #720 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - New Features - Enable GenAI directly from alerts, the walkthrough step, or via a new command. - Improvements - Clear “GenAI is disabled” status with guided actions to enable. - Analyzer path setting now validates user-provided paths, falls back to bundled binary if invalid, and restarts the analyzer when the path changes. - Solution server connects only when enabled, reducing noise. - Bug Fixes - Prevents unnecessary credential prompts when the solution server is disabled. - Chores - Minor UI cleanup and formatting; removed an unused import. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: David Zager <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses a few issues:
Fixes The server can be started with a non existing analyzer binary #720
Summary by CodeRabbit