👻 Notify user when java and/or mvn not installed #919
Conversation
WalkthroughThe change refactors vscode/src/client/analyzerClient.ts to centralize analyzer RPC server lifecycle handling inside startAnalysisServer, adds a stderrBuffer to collect server stderr, and inspects that buffer on process exit to detect Java- or Maven-missing errors. When detected, it shows targeted user prompts (e.g., open Red Hat OpenJDK download or Maven install guide); otherwise it falls back to the existing generic error flow. Close/error handlers and server state transitions (stopped, startFailed) and logging are preserved and applied consistently during startup and exit. No public/exported API changes. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
vscode/src/commands.ts (1)
104-132: Fix misleading success copy; consider small UX tweak
- If JAVA_HOME is undefined you show “(auto-detected)”, which is backwards. Say it's not set but Java was found via PATH.
- Optional: wrap the check in withProgress since validation runs sync and may momentarily block the UI.
Apply this minimal copy fix:
- const message = validation.javaHome - ? `✅ Java environment is valid!\n\nJAVA_HOME: ${validation.javaHome}` - : `✅ Java environment is valid!\n\nJAVA_HOME: (auto-detected)`; + const message = validation.javaHome + ? `✅ Java environment is valid!\n\nJAVA_HOME: ${validation.javaHome}` + : `✅ Java environment is valid!\n\nJAVA_HOME is not set (using PATH)`;vscode/package.json (1)
142-147: Add activation event for the new command (discoverability)Without an
onCommand:activation event, the extension may not activate when a user runs this command from the palette in a fresh workspace.Also add this under
activationEvents:"activationEvents": [ "onFileSystem:konveyorMemFs", - "onFileSystem:konveyorReadOnly" + "onFileSystem:konveyorReadOnly", + "onCommand:konveyor.testJavaEnvironment" ],Please confirm the command appears and runs from the Command Palette in a new workspace without prior activation.
vscode/src/client/analyzerClient.ts (3)
176-195: Duplicate onUnhandledNotification registrationYou register
onUnhandledNotificationtwice. Keep one.Apply:
- this.analyzerRpcConnection.onUnhandledNotification((e) => { - this.logger.warn(`Unhandled notification: ${e.method}`); - });
635-689: Make Java detection portable; add version check
- macOS:
readlink -fisn’t available; preferfs.realpathSyncand/usr/libexec/java_home -v 11+.- Validate major version ≥ 11 to match your error text.
Patch the Unix/macOS branch and version parsing:
- // Try to run java -version to check if java is available - execSync("java -version", { stdio: "pipe" }); + // Verify Java presence and version (stderr carries version on many JDKs) + const versionOut = execSync("java -version 2>&1", { encoding: "utf8", shell: true }); + const m = versionOut.match(/version\s+"(\d+(?:\.\d+)*)/); + const major = m ? parseInt(m[1].split(".")[0], 10) : undefined; + if (major !== undefined && major < 11) { + return { + isValid: false, + error: + "Java 11+ is required for the analyzer server. Please install a supported JDK and try again.", + }; + } ... - const javaPath = execSync("which java", { encoding: "utf8" }).trim(); - if (javaPath) { - // Follow symlinks and get the actual Java installation path - const realPath = execSync(`readlink -f "${javaPath}"`, { encoding: "utf8" }).trim(); - // Go up the directory structure to find JAVA_HOME (typically ../.. from bin/java) - javaHome = realPath.replace(/\/bin\/java$/, ""); - } + const javaPath = execSync("which java", { encoding: "utf8" }).trim(); + if (javaPath) { + // Resolve symlinks portably + const realPath = fs.realpathSync(javaPath); + javaHome = realPath.replace(/\/bin\/java$/, ""); // parent of bin + } + // macOS helper (preferred when available) + if (process.platform === "darwin" && !javaHome) { + try { + javaHome = execSync("/usr/libexec/java_home -v 11+", { encoding: "utf8" }).trim(); + } catch { /* ignore */ } + }
694-733: Strengthen Windows JAVA_HOME detection; avoid fs shadowing
- Handle the common “javapath” case from
where javaand split lines with CRLF.- Use the already-imported
fs-extrainstead ofrequire("fs").Apply:
- const javaPath = execSync("where java", { encoding: "utf8" }).trim().split("\n")[0]; - if (javaPath && javaPath.includes("\\bin\\java.exe")) { - return javaPath.replace(/\\bin\\java\.exe$/, ""); - } + const candidates = execSync("where java", { encoding: "utf8" }) + .trim() + .split(/\r?\n/) + .map((s) => s.trim()); + const binJava = candidates.find((p) => /\\bin\\java\.exe$/i.test(p)); + if (binJava) { + return binJava.replace(/\\bin\\java\.exe$/i, ""); + } ... - const fs = require("fs"); for (const basePath of commonPaths) { try { if (fs.existsSync(basePath)) { const jdkDirs = fs .readdirSync(basePath) .filter((dir: string) => dir.startsWith("jdk") || dir.startsWith("java-")); if (jdkDirs.length > 0) { const javaHome = `${basePath}\\${jdkDirs[0]}`; if (fs.existsSync(`${javaHome}\\bin\\java.exe`)) { return javaHome; } } } } catch (fsError) { // Continue to next path } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
vscode/package.json(1 hunks)vscode/src/client/analyzerClient.ts(2 hunks)vscode/src/commands.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-30T18:00:23.969Z
Learnt from: djzager
PR: konveyor/editor-extensions#885
File: vscode/src/commands.ts:18-18
Timestamp: 2025-09-30T18:00:23.969Z
Learning: In the konveyor/editor-extensions repository, the commands konveyor.configureLabelSelector and konveyor.configureSourcesTargets were previously removed along with their handlers. Any PR that removes their definitions from vscode/package.json is cleanup work, not adding new functionality.
Applied to files:
vscode/package.json
🧬 Code graph analysis (1)
vscode/src/commands.ts (1)
vscode/src/utilities/constants.ts (1)
EXTENSION_NAME(18-18)
⏰ 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: Lint
🔇 Additional comments (1)
vscode/src/client/analyzerClient.ts (1)
228-248: Targeted user guidance on Java failures looks goodNice UX: specific error text and quick action to install OpenJDK, with generic fallback for non-Java errors.
ibolton336
force-pushed
the
java-home-check
branch
from
a486ac0 to
447f669
Compare
Signed-off-by: Ian Bolton <[email protected]>
ibolton336
force-pushed
the
java-home-check
branch
from
447f669 to
61937f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
vscode/src/client/analyzerClient.ts (1)
335-339: Consider consolidating cleanup logic (optional).The close handler duplicates state management from the exit handler (Lines 331-332). While this defensive approach ensures cleanup, you could extract the common logic into a private method to reduce duplication:
private cleanupServerProcess() { this.fireServerStateChange("stopped"); this.analyzerRpcServer = null; }Then call it from both handlers. However, the current implementation is safe and the duplication is minimal.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vscode/src/client/analyzerClient.ts(1 hunks)
⏰ 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 (3)
vscode/src/client/analyzerClient.ts (3)
291-299: LGTM! Stderr buffering implementation is sound.The buffering approach correctly preserves logging while capturing stderr for error detection. For startup error scenarios, the unbounded buffer is acceptable.
341-348: LGTM! Error handler correctly manages spawn failures.The handler appropriately distinguishes spawn errors (state: "startFailed") from exit/close events (state: "stopped"), ensuring proper state transitions throughout the server lifecycle.
302-333: Excellent Java error detection with clear user guidance. Verify that the analyzer server actually emits “java is not installed or not on the path” (case-insensitive) when Java is missing and adjust the matching string if necessary.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
vscode/src/client/analyzerClient.ts (2)
118-123: Remove non-existent socket event listeners.The
connectionAttemptandconnectionAttemptFailedevents do not exist onnet.Socketand will never fire.As previously suggested, replace with the standard
connectevent:- socket.addListener("connectionAttempt", () => { - this.logger.info("Attempting to establish connection..."); - }); - socket.addListener("connectionAttemptFailed", () => { - this.logger.info("Connection attempt failed"); - }); + socket.on("connect", () => { + this.logger.info("Connected to analyzer RPC server"); + });
180-184: Forward all command arguments safely.Only the first argument is forwarded, which drops additional arguments that some JDT commands require. Additionally, the force unwrap operator
!will crash ifargumentsis undefined.Apply this diff to forward all arguments safely:
- const result = await vscode.commands.executeCommand( - "java.execute.workspaceCommand", - params.command, - params.arguments![0], - ); + const args = Array.isArray(params.arguments) ? params.arguments : []; + const result = await vscode.commands.executeCommand( + "java.execute.workspaceCommand", + params.command, + ...args, + );
🧹 Nitpick comments (1)
vscode/src/client/analyzerClient.ts (1)
302-333: Consider consistent state management for error conditions.When the analyzer exits with a non-zero code, the state is set to
stopped(line 331), but theerrorevent handler at line 343 sets it tostartFailed. This inconsistency could affect downstream logic that depends on distinguishing between clean shutdown and startup failure.Consider setting the state to
startFailedfor non-zero exit codes to maintain consistency:} else { // Show generic error as before vscode.window.showErrorMessage( `Analyzer RPC server failed. Status code: ${code}. Please see the output channel for details.`, ); } } - this.fireServerStateChange("stopped"); + this.fireServerStateChange("startFailed"); this.analyzerRpcServer = null; });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vscode/src/client/analyzerClient.ts(1 hunks)
⏰ 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). (2)
- GitHub Check: Build (windows)
- GitHub Check: Build (macos)
🔇 Additional comments (2)
vscode/src/client/analyzerClient.ts (2)
291-299: LGTM!Buffering stderr for post-exit error analysis is a clean approach that enables targeted error detection without duplicating error-handling logic.
335-348: LGTM!The
closeanderrorhandlers properly manage server lifecycle and state transitions. The error handler correctly usesstartFailedstate for process errors.
| "Please install Red Hat OpenJDK (JDK 11 or later) and ensure the 'java' " + | ||
| "command is available in your system PATH.", | ||
| "Install Red Hat OpenJDK", | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the Java error message across platforms.
The detection relies on an exact string match (case-insensitive) for "java is not installed or not on the path". Error message text can vary across operating systems, Java implementations, or analyzer versions.
Run this script to search for the error message generation in the analyzer codebase:
Consider using a regex pattern or checking for multiple known Java error indicators (e.g., "java.*not found", "JAVA_HOME.*not set", "command not found.*java") to make the detection more robust across different environments.
🏁 Script executed:
#!/bin/bash
# Search for where this error message is generated in the analyzer
rg -i "java is not installed" -A 2 -B 2
rg -i "not on the path" -A 2 -B 2
rg -i "java.*not.*found" -A 2 -B 2Length of output: 278104
Improve Java‐not‐available detection
Relying on a single text check for “java is not installed or not on the path” is brittle. Update this to match multiple known error patterns (e.g. /java.*not found/, /command not found.*java/, /JAVA_HOME.*not set/) or consolidate into one regex to catch variations across platforms.
Location: vscode/src/client/analyzerClient.ts:307
🤖 Prompt for AI Agents
In vscode/src/client/analyzerClient.ts around line 307, the current check using
stderrBuffer.toLowerCase().includes("java is not installed or not on the path")
is too brittle; replace it with a single case-insensitive regex that matches
multiple known error patterns (examples: "java.*not found", "command not
found.*java", "java: command not found", "JAVA_HOME.*not set") and use
regex.test(stderrBuffer) after normalizing the buffer (trim/ensure string).
Update the condition to use that consolidated RegExp so variations across
platforms are caught reliably.
Signed-off-by: Ian Bolton <[email protected]>
ibolton336
changed the title
ibolton336
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
vscode/src/client/analyzerClient.ts (3)
118-123: Remove non-existent socket event listeners.These socket event listeners (
connectionAttemptandconnectionAttemptFailed) don't exist onnet.Socketand will never fire. This was flagged in a previous review.Replace with standard socket events:
- socket.addListener("connectionAttempt", () => { - this.logger.info("Attempting to establish connection..."); - }); - socket.addListener("connectionAttemptFailed", () => { - this.logger.info("Connection attempt failed"); - }); + socket.on("connect", () => { + this.logger.info("Connected to analyzer RPC server"); + });
180-184: Forward all command arguments safely.Only the first argument is forwarded (
params.arguments![0]), which drops additional arguments and can crash whenargumentsis undefined. This was flagged in a previous review.Apply this diff:
- const result = await vscode.commands.executeCommand( - "java.execute.workspaceCommand", - params.command, - params.arguments![0], - ); + const args = Array.isArray(params.arguments) ? params.arguments : []; + const result = await vscode.commands.executeCommand( + "java.execute.workspaceCommand", + params.command, + ...args, + );
307-307: Make Java error detection more robust.The current check using
.includes("java is not installed")is too brittle and won't catch variations across platforms. This concern was raised in a previous review.Use a regex pattern to match multiple known error indicators:
- if (stderrBuffer.toLowerCase().includes("java is not installed")) { + const javaErrorPattern = /java.*not\s+(found|installed)|command\s+not\s+found.*java|java_home.*not\s+set/i; + if (javaErrorPattern.test(stderrBuffer)) { // Show user-friendly Java error with action button
🧹 Nitpick comments (2)
vscode/src/client/analyzerClient.ts (2)
291-299: Consider bounding the stderr buffer.The
stderrBufferaccumulates all stderr output without size limits, which could consume excessive memory if the server produces verbose error output before exiting.Consider limiting the buffer size:
// Buffer to collect stderr for error analysis -let stderrBuffer = ""; +let stderrBuffer = ""; +const MAX_STDERR_BUFFER_SIZE = 10000; // characters analyzerRpcServer.stderr.on("data", (data) => { const asString: string = data.toString().trimEnd(); this.logger.error(`${asString}`); // Collect stderr for potential Java/Maven error detection - stderrBuffer += asString + "\n"; + if (stderrBuffer.length < MAX_STDERR_BUFFER_SIZE) { + stderrBuffer += asString + "\n"; + } });
301-365: LGTM with optional refinement for handler coordination.The exit, close, and error handlers correctly manage lifecycle and state transitions. The exit handler's stderr-based error detection is a good approach for providing user-friendly diagnostics.
Optional: Consider preventing state transitions in
closehandler iferroralready fired, to maintain the more specific "startFailed" state:analyzerRpcServer.on("close", (code, signal) => { this.logger.info(`Analyzer RPC server closed [signal: ${signal}, code: ${code}]`); - this.fireServerStateChange("stopped"); + // Only transition to stopped if not already in a failed state + if (this.serverState !== "startFailed") { + this.fireServerStateChange("stopped"); + } this.analyzerRpcServer = null; });
| "Please install Maven (version 3.6 or later) and ensure the 'mvn' " + | ||
| "command is available in your system PATH.", | ||
| "Install Maven", | ||
| "Maven Setup Guide", |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Make Maven error detection more robust.
Similar to the Java check, this Maven detection using .includes("maven is not installed") won't catch platform variations like "mvn: command not found" or "Maven not found".
Use a regex pattern:
- } else if (stderrBuffer.toLowerCase().includes("maven is not installed")) {
+ const mavenErrorPattern = /maven.*not\s+(found|installed)|command\s+not\s+found.*(mvn|maven)|mvn.*not\s+recognized/i;
+ } else if (mavenErrorPattern.test(stderrBuffer)) {
// Show user-friendly Maven error with action buttonCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In vscode/src/client/analyzerClient.ts around line 323, the current Maven
detection uses stderrBuffer.toLowerCase().includes("maven is not installed")
which misses common variants; replace that check with a case-insensitive regex
test that matches patterns like "mvn: command not found", "command not found:
mvn", "maven not found", or "maven.*not.*installed" (e.g. /mvn: command not
found|command not found: mvn|maven not found|maven.*not.*installed/i) and use
RegExp.test(stderrBuffer) so the detection is robust across platforms and
message variations.
2 participants
Screen.Recording.2025-10-08.at.2.56.30.PM.mov
Screen.Recording.2025-10-08.at.3.20.40.PM.mov
Summary by CodeRabbit
New Features
Bug Fixes