feat: Preference option for configuring eim_idf.json file#1444
Open
kolipakakondal wants to merge 4 commits intomasterfrom
Open
feat: Preference option for configuring eim_idf.json file#1444kolipakakondal wants to merge 4 commits intomasterfrom
kolipakakondal wants to merge 4 commits intomasterfrom
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (16)
📝 WalkthroughWalkthroughThis PR introduces a centralized Changes
Sequence DiagramsequenceDiagram
participant User
participant PreferencesPage as Preferences UI
participant PrefStore as Preference Store
participant PathResolver as EimIdfJsonPathResolver
participant WatchService as EimJsonWatchService
User->>PreferencesPage: Set custom EIM JSON path
PreferencesPage->>PreferencesPage: Validate filename
PreferencesPage->>PrefStore: Save path to EIM_IDF_JSON_PATH
PreferencesPage->>WatchService: restartAfterEimIdfPathChange()
WatchService->>WatchService: Stop current instance
WatchService->>PathResolver: resolveEimIdfJsonFile()
PathResolver->>PrefStore: Read EIM_IDF_JSON_PATH preference
PathResolver-->>WatchService: Return resolved path
WatchService->>WatchService: Create new instance with path
WatchService->>WatchService: Re-attach prior listeners
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimLoader.java (1)
138-174:⚠️ Potential issue | 🟠 MajorKeep pause/unpause balanced on the same watcher instance.
The code pauses one
getInstance()result and later unpauses another. If the watcher is restarted/recreated while EIM is open, or ifcallback.run()throws, listeners can remain paused on the original watcher.Suggested fix
waitJob.addJobChangeListener(new JobChangeAdapter() { + private EimJsonWatchService pausedWatchService; + `@Override` public void aboutToRun(IJobChangeEvent event) { - EimJsonWatchService ws = EimJsonWatchService.getInstance(); - if (ws != null) + pausedWatchService = EimJsonWatchService.getInstance(); + if (pausedWatchService != null) { - ws.pauseListeners(); + pausedWatchService.pauseListeners(); } } @@ public void done(IJobChangeEvent event) { - Display.getDefault().asyncExec(() -> { - try - { - standardConsoleStream.write("EIM has been closed.\n"); //$NON-NLS-1$ - } - catch (IOException e) - { - Logger.log(e); - } - }); - - if (callback != null) + try { - callback.run(); + Display.getDefault().asyncExec(() -> { + try + { + standardConsoleStream.write("EIM has been closed.\n"); //$NON-NLS-1$ + } + catch (IOException e) + { + Logger.log(e); + } + }); + + if (callback != null) + { + callback.run(); + } } - - EimJsonWatchService ws2 = EimJsonWatchService.getInstance(); - if (ws2 != null) + finally { - ws2.unpauseListeners(); + if (pausedWatchService != null) + { + pausedWatchService.unpauseListeners(); + pausedWatchService = null; + } } } });🤖 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/EimLoader.java` around lines 138 - 174, The aboutToRun/done listener currently pauses EimJsonWatchService.getInstance() into local ws and later calls getInstance() again (ws2) and runs callback, risking an unmatched unpause if the watcher is recreated or callback throws; capture the watcher instance once (e.g., a final variable like pausedWatcher) when aboutToRun runs or before adding the JobChangeAdapter and use that same reference in done, and ensure done wraps callback.run() in try/finally so pausedWatcher.unpauseListeners() is always invoked on the same instance even if callback throws.bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonStateChecker.java (1)
23-64:⚠️ Potential issue | 🟠 MajorTrack the resolved path with the stored timestamp.
Line 64 makes the watched file configurable, but
LAST_MODIFIED_PREF_KEYstill stores a timestamp without the path it belongs to. After switching locations, a newly selectedeim_idf.jsoncan be treated as unchanged if its mtime is older than the previous file’s timestamp.Suggested fix
private static final String LAST_MODIFIED_PREF_KEY = "lastEimJsonModified"; //$NON-NLS-1$ + private static final String LAST_PATH_PREF_KEY = "lastEimJsonPath"; //$NON-NLS-1$ @@ public boolean wasModifiedSinceLastRun() { - File jsonFile = new File(getEimJsonPath()); + String eimJsonPath = getEimJsonPath(); + File jsonFile = new File(eimJsonPath); if (!jsonFile.exists()) { return false; } long lastModified = jsonFile.lastModified(); long lastSeen = preferences.getLong(LAST_MODIFIED_PREF_KEY, 0L); @@ return false; } + String lastPath = preferences.get(LAST_PATH_PREF_KEY, eimJsonPath); + if (!eimJsonPath.equals(lastPath)) + { + return true; + } + return lastModified > lastSeen; } @@ public void updateLastSeenTimestamp() { - File jsonFile = new File(getEimJsonPath()); + String eimJsonPath = getEimJsonPath(); + File jsonFile = new File(eimJsonPath); if (jsonFile.exists()) { preferences.putLong(LAST_MODIFIED_PREF_KEY, jsonFile.lastModified()); + preferences.put(LAST_PATH_PREF_KEY, eimJsonPath); } }🤖 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/watcher/EimJsonStateChecker.java` around lines 23 - 64, The stored timestamp under LAST_MODIFIED_PREF_KEY in EimJsonStateChecker doesn't account for the resolved file path from getEimJsonPath(), so switching to a different eim_idf.json can be misdetected; update wasModifiedSinceLastRun and updateLastSeenTimestamp to also persist and compare the resolved path (e.g., store a companion preference like lastEimJsonPath or include the path in the pref key) before using the stored timestamp—if the saved path differs from the current getEimJsonPath() result treat it as a first-seen file (or reset the timestamp) and then save the current path together with the file's lastModified value so future calls to wasModifiedSinceLastRun check both path equality and mtime.
🤖 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/EimIdfJsonPathResolver.java`:
- Around line 30-40: The resolver currently returns any existing path; update
resolveEimIdfJsonFileFromPreferenceString to validate that the provided custom
path has the expected basename (compare
Paths.get(custom).getFileName().toString() to EimConstants.EIM_JSON) and that it
is a regular file (use Files.isRegularFile) before returning it; otherwise fall
back to getDefaultEimIdfJsonFile(). Keep the existing null/empty check and use
the same Path p variable and Files.exists check logic but replace the
exists-only check with the basename + isRegularFile guards so the watcher/UI
contract is honored.
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.java`:
- Around line 63-66: The current isEimIdfJsonPresent() uses Files.exists(...)
which can return true for directories or non-regular paths; change the check to
verify the resolved path from new
EimIdfJsonPathResolver().resolveEimIdfJsonFile() is a regular readable file (use
Files.isRegularFile(path) and/or Files.isReadable(path) instead of Files.exists)
so the method only returns true for an actual file that can be read.
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonWatchService.java`:
- Around line 92-114: restartAfterEimIdfPathChange currently stops the old
watcher first which can leave INSTANCE null if constructing the new
EimJsonWatchService fails; change the order in restartAfterEimIdfPathChange so
you first create the new EimJsonWatchService instance (new
EimJsonWatchService()), register the copied listeners on that fresh instance via
fresh.addEimJsonChangeListener(...), then atomically assign INSTANCE = fresh,
and only after the swap call old.requestStop(); keep the try/catch around the
construction so if construction throws you log the IOException and leave the
original INSTANCE running (do not set INSTANCE to null).
In
`@tests/com.espressif.idf.core.test/src/com/espressif/idf/core/tools/test/EimIdfJsonPathResolverTest.java`:
- Around line 50-55: The test nonexistent_custom_falls_back_to_default assumes a
literal filesystem path that might exist on the host; make it deterministic by
using JUnit's `@TempDir`. Change the test to accept a `@TempDir` Path (e.g. Path
tempDir) and pass a guaranteed-missing child like
tempDir.resolve("missing").resolve("eim_idf.json").toString() into
EimIdfJsonPathResolver.resolveEimIdfJsonFileFromPreferenceString, keeping the
assertion against r.getDefaultEimIdfJsonFile(); reference the test method name
and the EimIdfJsonPathResolver class to locate where to update the parameter and
call.
---
Outside diff comments:
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimLoader.java`:
- Around line 138-174: The aboutToRun/done listener currently pauses
EimJsonWatchService.getInstance() into local ws and later calls getInstance()
again (ws2) and runs callback, risking an unmatched unpause if the watcher is
recreated or callback throws; capture the watcher instance once (e.g., a final
variable like pausedWatcher) when aboutToRun runs or before adding the
JobChangeAdapter and use that same reference in done, and ensure done wraps
callback.run() in try/finally so pausedWatcher.unpauseListeners() is always
invoked on the same instance even if callback throws.
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonStateChecker.java`:
- Around line 23-64: The stored timestamp under LAST_MODIFIED_PREF_KEY in
EimJsonStateChecker doesn't account for the resolved file path from
getEimJsonPath(), so switching to a different eim_idf.json can be misdetected;
update wasModifiedSinceLastRun and updateLastSeenTimestamp to also persist and
compare the resolved path (e.g., store a companion preference like
lastEimJsonPath or include the path in the pref key) before using the stored
timestamp—if the saved path differs from the current getEimJsonPath() result
treat it as a first-seen file (or reset the timestamp) and then save the current
path together with the file's lastModified value so future calls to
wasModifiedSinceLastRun check both path equality and mtime.
🪄 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: 0c910487-d1c4-4aae-9ef8-a3cb4e454c42
📒 Files selected for processing (14)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfConfiguratinParser.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimIdfJsonPathResolver.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/EimLoader.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/ToolInitializer.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonStateChecker.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/tools/watcher/EimJsonWatchService.javabundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFTerminalProcessConnector.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/Messages.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/messages.propertiesbundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/messages_zh.propertiesbundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/EspressifToolStartup.javatests/com.espressif.idf.core.test/src/com/espressif/idf/core/tools/test/EimIdfJsonPathResolverTest.java
sigmaaa
reviewed
Apr 24, 2026
Collaborator
sigmaaa
left a comment
sigmaaa
left a comment
There was a problem hiding this comment.
LGTM. Just a one small nit about small UX improvement
| eimGroup.setLayout(new GridLayout(3, false)); | ||
|
|
||
| Label eimPathLabel = new Label(eimGroup, SWT.None); | ||
| eimPathLabel.setText(Messages.EspresssifPreferencesPage_EimIdfJsonPathLabel); |
Collaborator
There was a problem hiding this comment.
NIT: We can also use eimIdfJsonPathText.setMessage("Leave empty to use the default value."); to make users aware that a default value exists.
kolipakakondal
force-pushed
the
fix_accept_eimidfjson_location
branch
from
9c43397 to
dbf78f1
Compare
Collaborator
|
@kolipakakondal hi ! Tested under: Windows 11: Mac arm64: |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Description
Preference option for configuring eim_idf.json file from the custom location.
Fixes # IEP-1753
Type of change
New feature (non-breaking change which adds functionality)
How has this been tested?
Test Configuration:
Dependent components impacted by this PR:
Checklist