Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTool initialization now centralizes EIM executable resolution via a new resolver that prefers an ChangesEIM Path Resolution System
Sequence Diagram(s)sequenceDiagram
participant Startup as EspressifToolStartup
participant Resolver as ToolInitializer
participant Finder as SystemExecutableFinder
participant FS as FileSystem
participant Env as Environment
participant Default as DefaultPath
Startup->>Resolver: resolveEimExecutablePath(eimJson)
Resolver->>Finder: locate "eim" on PATH
alt found on PATH
Finder-->>Resolver: path
Resolver-->>Startup: resolved path
else not found
Finder-->>Resolver: not found
Resolver->>FS: check eimJson.eimPath (if provided)
alt eimJson path exists
FS-->>Resolver: path
Resolver-->>Startup: resolved path
else not found
FS-->>Resolver: not found
Resolver->>Env: read EIM_PATH env var
alt env var set and exists
Env-->>Resolver: path
Resolver-->>Startup: resolved path
else not found
Env-->>Resolver: not set/missing
Resolver->>Default: getDefaultEimPath()
alt default exists
Default-->>Resolver: path
Resolver-->>Startup: resolved path
else none
Default-->>Resolver: none
Resolver-->>Startup: empty (resolution failed)
end
end
end
end
Startup->>Startup: set EIM_PATH only if resolved path non-empty
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java (1)
158-170:⚠️ Potential issue | 🟡 MinorError message and flow depend on resolver returning empty.
StringUtil.isEmpty(resolved)is the only protection against invoking export with a fictional default path, but the resolver (seeToolInitializer.resolveEimExecutablePath) rarely returns empty today. In practice, when EIM isn't installed,resolvedbecomes the default path,Paths.get(resolved)succeeds, and the real error is produced insideexportOldConfig("Cannot Convert EIM is not installed"), which gets logged but the user-facingMessages.OldConfigExportCompleteFailMsgis written — the more specific "could not be resolved" branch here is unreachable. This will resolve itself once the resolver is fixed; just flagging so the two changes land together.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java` around lines 158 - 170, startExportOldConfig currently trusts toolInitializer.resolveEimExecutablePath(resolved) even when it returns a default non-installed path; add a validation step after Paths.get(resolved) to ensure the path points to an existing, executable EIM binary (use Files.exists(...) and Files.isExecutable(...)) and if that check fails log a clear, specific error via Logger.log and write the appropriate user-facing message instead of falling back to Messages.OldConfigExportCompleteFailMsg; additionally, when calling exportOldConfig(...) propagate or inspect its returned error/exception so that if exportOldConfig reports "EIM is not installed" you surface that specific message to the console (use the exportOldConfig return/exception handling in startExportOldConfig to select the correct message).
🧹 Nitpick comments (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java (2)
50-69: Consider unifyingisEimInstalled()withresolveEimExecutablePath().The two methods now encode overlapping but subtly different resolution rules (this one consults
EIM_PATHenv var and requires the default to exist; the resolver consultseim_idf.jsonand currently ignores default existence). A single source of truth would avoid drift — e.g.:public boolean isEimInstalled() { return !StringUtil.isEmpty(resolveEimExecutablePath(null)); }(Only viable once the default-path existence check above is applied, otherwise this would always return
true.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java` around lines 50 - 69, Unify the resolution logic by making isEimInstalled() delegate to resolveEimExecutablePath(null) so there is a single source of truth; update resolveEimExecutablePath(String) to include the same default-path existence check currently in isEimInstalled() (i.e., after checking env var and eim_idf.json, verify any returned default path actually exists before returning it) so resolveEimExecutablePath returns null when no valid executable path exists and isEimInstalled can simply test for a non-empty result.
77-81: Tests for new resolution logic.
findEimOnSystemPath()andresolveEimExecutablePath(EimJson)encode non-trivial priority logic (PATH →eim_idf.json→ default). Please add JUnit 5 coverage incom.espressif.idf.core.testcovering at minimum: found-on-PATH wins, missing-on-PATH + valid JSON path, missing-on-PATH + non-existent JSON path, and no-source fallback. Based on learnings, tests in com.espressif.idf.core.test (JUnit 5) should be added when changing core/plugin logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java` around lines 77 - 81, Add JUnit 5 tests in package com.espressif.idf.core.test that exercise the resolution priority implemented by findEimOnSystemPath() and resolveEimExecutablePath(EimJson): create a ToolInitializerTest (or similarly named) with four test methods asserting: (1) when an "eim" is present on PATH the finder returns the PATH location (found-on-PATH wins), (2) when "eim" is not on PATH but a valid eim_idf.json is provided resolveEimExecutablePath(...) returns the JSON-specified path, (3) when not on PATH and the JSON path is non-existent resolveEimExecutablePath(...) falls back appropriately (assert expected empty/default), and (4) when no sources exist the default fallback is returned; use temporary files/dirs and mock or manipulate SystemExecutableFinder behavior and EimJson inputs to isolate each scenario and assert the exact returned strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java`:
- Around line 90-109: The resolveEimExecutablePath method currently returns the
default path unconditionally because getDefaultEimPath() always returns a Path;
change resolveEimExecutablePath (used by findAndSetEimPath and startup code) to
verify the default Path actually exists with Files.exists(defaultEimPath) before
returning it and otherwise return StringUtil.EMPTY; this makes the “nothing
resolvable” case behave like isEimInstalled (which checks existence) and
prevents writing a bogus EIM_PATH that downstream callers (e.g.,
EimButtonLaunchListener → eimLoader.launchEimWithResult) will attempt to
execute.
In
`@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java`:
- Around line 118-123: ToolInitializer.resolveEimExecutablePath currently
returns getDefaultEimPath().toString() unconditionally which causes EIM_PATH to
be set to a non-existent default; update resolveEimExecutablePath to check
whether the resolved path (system PATH result or the default returned by
getDefaultEimPath()) actually exists and is executable, and return an empty
Optional/string (or null as the project style uses) when no valid executable is
found so callers (e.g., EspressifToolStartup handling EIM_PATH and
EimButtonLaunchListener calling eimLoader.launchEimWithResult) will fall back to
the “EIM missing” branch instead of storing a bogus default.
---
Outside diff comments:
In
`@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java`:
- Around line 158-170: startExportOldConfig currently trusts
toolInitializer.resolveEimExecutablePath(resolved) even when it returns a
default non-installed path; add a validation step after Paths.get(resolved) to
ensure the path points to an existing, executable EIM binary (use
Files.exists(...) and Files.isExecutable(...)) and if that check fails log a
clear, specific error via Logger.log and write the appropriate user-facing
message instead of falling back to Messages.OldConfigExportCompleteFailMsg;
additionally, when calling exportOldConfig(...) propagate or inspect its
returned error/exception so that if exportOldConfig reports "EIM is not
installed" you surface that specific message to the console (use the
exportOldConfig return/exception handling in startExportOldConfig to select the
correct message).
---
Nitpick comments:
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java`:
- Around line 50-69: Unify the resolution logic by making isEimInstalled()
delegate to resolveEimExecutablePath(null) so there is a single source of truth;
update resolveEimExecutablePath(String) to include the same default-path
existence check currently in isEimInstalled() (i.e., after checking env var and
eim_idf.json, verify any returned default path actually exists before returning
it) so resolveEimExecutablePath returns null when no valid executable path
exists and isEimInstalled can simply test for a non-empty result.
- Around line 77-81: Add JUnit 5 tests in package com.espressif.idf.core.test
that exercise the resolution priority implemented by findEimOnSystemPath() and
resolveEimExecutablePath(EimJson): create a ToolInitializerTest (or similarly
named) with four test methods asserting: (1) when an "eim" is present on PATH
the finder returns the PATH location (found-on-PATH wins), (2) when "eim" is not
on PATH but a valid eim_idf.json is provided resolveEimExecutablePath(...)
returns the JSON-specified path, (3) when not on PATH and the JSON path is
non-existent resolveEimExecutablePath(...) falls back appropriately (assert
expected empty/default), and (4) when no sources exist the default fallback is
returned; use temporary files/dirs and mock or manipulate SystemExecutableFinder
behavior and EimJson inputs to isolate each scenario and assert the exact
returned strings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4401adbb-833a-4fa3-a066-450bd2e97791
📒 Files selected for processing (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java
| // Set EimPath on every startup: system PATH first, then eim_idf.json, then default locations | ||
| String resolvedEimPath = toolInitializer.resolveEimExecutablePath(eimJson); | ||
| if (!StringUtil.isEmpty(resolvedEimPath)) | ||
| { | ||
| idfEnvironmentVariables.addEnvVariable(IDFEnvironmentVariables.EIM_PATH, eimJson.getEimPath()); | ||
| } | ||
| else | ||
| { | ||
| // Fail-safe call to ensure if the eim is in Applications or user.home it is setup in env vars | ||
| toolInitializer.findAndSetEimPath(); | ||
| idfEnvironmentVariables.addEnvVariable(IDFEnvironmentVariables.EIM_PATH, resolvedEimPath); | ||
| } |
There was a problem hiding this comment.
EIM_PATH may be overwritten with a non-existent default on every startup.
Because ToolInitializer.resolveEimExecutablePath() currently returns getDefaultEimPath().toString() without checking existence, this branch will almost always execute and set EIM_PATH to a path like ~/.espressif/eim_gui/eim[.exe] even when EIM isn't installed. Callers such as EimButtonLaunchListener (reads EIM_PATH and invokes eimLoader.launchEimWithResult(...)) will then fail at launch time with an IOException instead of taking the "EIM missing" branch.
The root-cause fix belongs in ToolInitializer.resolveEimExecutablePath (see the comment there). Once the resolver returns empty on a non-existent default, the guard here behaves as the comment implies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.java`
around lines 118 - 123, ToolInitializer.resolveEimExecutablePath currently
returns getDefaultEimPath().toString() unconditionally which causes EIM_PATH to
be set to a non-existent default; update resolveEimExecutablePath to check
whether the resolved path (system PATH result or the default returned by
getDefaultEimPath()) actually exists and is executable, and return an empty
Optional/string (or null as the project style uses) when no valid executable is
found so callers (e.g., EspressifToolStartup handling EIM_PATH and
EimButtonLaunchListener calling eimLoader.launchEimWithResult) will fall back to
the “EIM missing” branch instead of storing a bogus default.
sigmaaa
left a comment
sigmaaa
left a comment
There was a problem hiding this comment.
LGTM! Please take a look at the method I mentioned in the PR. Looks like a dead code
| public void findAndSetEimPath() | ||
| { | ||
| Path defaultEimPath = getDefaultEimPath(); | ||
|
|
||
| if (defaultEimPath != null) | ||
| setEimPathInEnvVar(defaultEimPath.toString()); | ||
| String resolved = resolveEimExecutablePath(null); | ||
| if (!StringUtil.isEmpty(resolved)) | ||
| { | ||
| setEimPathInEnvVar(resolved); | ||
| } | ||
| } |
There was a problem hiding this comment.
It seems that this is dead code.
kolipakakondal
left a comment
kolipakakondal
left a comment
There was a problem hiding this comment.
Hi @alirana01 Thanks for the PR. Currrent changes looks to me. However, we should do few checks to make sure it works with the GUI/CLI launch as well. Please refer to the VSCode PR here and make the necessary changes.
|
@alirana01 Please check the PR comments |
- resolveEimExecutablePath now validates default path exists before returning it, preventing bogus EIM_PATH when EIM is not installed - Added EIM_PATH env variable as fallback step in resolution - Unified isEimInstalled() to delegate to resolveEimExecutablePath - Removed dead findAndSetEimPath() and setEimPathInEnvVar() methods
Hi @kolipakakondal i have resolved some of the blocking comments on this PR and the other thing you mentioned for the way VSCode is handling it I will create a new PR for that so this doesn't become a blocker for release as that may require proper testing on various platforms The ticket is logged here IEP-1762 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java`:
- Around line 50-53: isEimInstalled() currently passes null to
resolveEimExecutablePath(), skipping the eim_idf.json branch and returning false
when EIM is only recorded in eim_idf.json; change isEimInstalled() to load the
local eim_idf.json configuration (using the same loader used elsewhere in this
class) and pass that config into resolveEimExecutablePath(config) so
resolveEimExecutablePath can detect a custom path in eim_idf.json and return
true when appropriate, preventing EimButtonLaunchListener from triggering
unnecessary downloads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d42f1964-0138-4d6c-94f0-cef5691a9c1e
📒 Files selected for processing (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java
| public boolean isEimInstalled() | ||
| { | ||
| return !StringUtil.isEmpty(resolveEimExecutablePath(null)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all call sites of isEimInstalled() across the plugin bundles
rg -n --type=java -B2 -A2 '\bisEimInstalled\s*\('Repository: espressif/idf-eclipse-plugin
Length of output: 1898
🏁 Script executed:
# Read the resolveEimExecutablePath method to understand the logic branches
cd bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools
cat -n ToolInitializer.java | sed -n '75,110p'Repository: espressif/idf-eclipse-plugin
Length of output: 1248
🏁 Script executed:
# Read the isEimIdfJsonPresent method
cd bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools
cat -n ToolInitializer.java | sed -n '107,115p'Repository: espressif/idf-eclipse-plugin
Length of output: 404
🏁 Script executed:
# Read the EimButtonLaunchListener.java context around line 72 to understand the impact
cat -n bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EimButtonLaunchListener.java | sed -n '65,90p'Repository: espressif/idf-eclipse-plugin
Length of output: 836
🏁 Script executed:
# Read loadEimJson() to understand the exception it declares
cd bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools
cat -n ToolInitializer.java | sed -n '114,130p'Repository: espressif/idf-eclipse-plugin
Length of output: 465
🏁 Script executed:
# Also check if there are any other methods in ToolInitializer that catch EimVersionMismatchException
rg -n 'EimVersionMismatchException' bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.javaRepository: espressif/idf-eclipse-plugin
Length of output: 220
isEimInstalled() returns false when EIM exists only in eim_idf.json, triggering unnecessary downloads.
Passing null to resolveEimExecutablePath() skips the JSON-path branch (lines 83–90). If EIM is located only at a custom path recorded in eim_idf.json — not on the system PATH, not in EIM_PATH, and not at the default location — isEimInstalled() returns false. This causes EimButtonLaunchListener.java:72 to unconditionally trigger a download job even though EIM is already installed.
The simplest fix is to load the JSON locally and pass it through:
🔧 Proposed fix
public boolean isEimInstalled()
{
- return !StringUtil.isEmpty(resolveEimExecutablePath(null));
+ EimJson eimJson = null;
+ try
+ {
+ eimJson = loadEimJson();
+ }
+ catch (EimVersionMismatchException e)
+ {
+ Logger.log(e);
+ }
+ return !StringUtil.isEmpty(resolveEimExecutablePath(eimJson));
}📝 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.
| public boolean isEimInstalled() | |
| { | |
| return !StringUtil.isEmpty(resolveEimExecutablePath(null)); | |
| } | |
| public boolean isEimInstalled() | |
| { | |
| EimJson eimJson = null; | |
| try | |
| { | |
| eimJson = loadEimJson(); | |
| } | |
| catch (EimVersionMismatchException e) | |
| { | |
| Logger.log(e); | |
| } | |
| return !StringUtil.isEmpty(resolveEimExecutablePath(eimJson)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java`
around lines 50 - 53, isEimInstalled() currently passes null to
resolveEimExecutablePath(), skipping the eim_idf.json branch and returning false
when EIM is only recorded in eim_idf.json; change isEimInstalled() to load the
local eim_idf.json configuration (using the same loader used elsewhere in this
class) and pass that config into resolveEimExecutablePath(config) so
resolveEimExecutablePath can detect a custom path in eim_idf.json and return
true when appropriate, preventing EimButtonLaunchListener from triggering
unnecessary downloads.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
@alirana01 hi ! Windows 11: Close EIM, delete EIM using |
yes because ide has that in env variables any way i will try to see if this can be fixed |
|
Fixed ✅ |
4 participants
Description
EIM Path Discovery should be improved now first the system path is checked for eim and then we check eim_idf.json
Summary by CodeRabbit