IEP-1754 Run ESP-IDF version detection asynchronously with progress reporting#1445
Merged
Conversation
📝 WalkthroughWalkthroughReplaced CompletableFuture-based refresh with an Eclipse Job that reports progress via IProgressMonitor, computes per-IDF rows using a VirtualThreadPerTaskExecutor, moves UI updates into a single Display.asyncExec block, changes exception handling to in-job try/catch returning IStatus, and removed the stored toolInitializer field and EIM JSON refresh. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "UI / Display"
participant Job as "Eclipse Job"
participant Executor as "VirtualThreadPerTaskExecutor"
participant Task as "Per-IDF Task"
participant Model as "Table Model"
UI->>Job: schedule(refresh)
Job->>Job: run(IProgressMonitor)
Job->>Executor: submit per-IDF tasks
Executor->>Task: start detection (parallel)
Task-->>Executor: row data (future)
Executor->>Job: join all futures -> rows
Job->>Model: set new input (background)
Job->>UI: asyncExec(update input/selection, reset install node, updateButtonState)
UI-->>Job: UI updated
Job-->>UI: return IStatus (OK/ERROR)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 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 (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (1)
96-96: Remove or retain theToolInitializerintentionally.This instance is immediately discarded, and the referenced constructor only initializes instance fields, so this line currently has no observable effect. Either remove it or restore a field if later logic should use
ToolInitializer.🤖 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/manager/pages/ESPIDFMainTablePage.java` at line 96, The ToolInitializer instance created on its own in ESPIDFMainTablePage (new ToolInitializer(InstanceScope.INSTANCE.getNode(UIPlugin.PLUGIN_ID))) has no effect because it is immediately discarded; either remove that statement entirely or persist the instance to a field (e.g., add a private final ToolInitializer toolInitializer and assign it in the constructor) so the initializer's state is retained and can be used later; update the class constructor and any references to use toolInitializer if kept, otherwise simply delete the line to avoid a no-op side effect.
🤖 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.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java`:
- Around line 438-467: Rows are built from the local newJson but the shared
field eimJson remains stale, causing performToolsSetup() (and SetupToolsInIde
construction) to operate on old data; inside the
Display.getDefault().asyncExec(...) block, assign the freshly parsed newJson to
the eimJson field (e.g., eimJson = newJson) before calling
tableViewer.setInput(finalRows) or any code that may trigger
performToolsSetup()/new SetupToolsInIde, ensuring UI-thread synchronization of
the configuration state.
- Around line 478-480: The finally block formatting must follow the project
brace style: move the `finally` keyword to its own line so the closing brace of
the try/catch ends on one line and `finally` begins the next, e.g. change the
sequence that currently reads like `} finally {` (around the block that calls
`monitor.done()`) to `}\nfinally\n{`, ensuring the block that contains
`monitor.done()` retains its contents unchanged.
---
Nitpick comments:
In
`@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java`:
- Line 96: The ToolInitializer instance created on its own in
ESPIDFMainTablePage (new
ToolInitializer(InstanceScope.INSTANCE.getNode(UIPlugin.PLUGIN_ID))) has no
effect because it is immediately discarded; either remove that statement
entirely or persist the instance to a field (e.g., add a private final
ToolInitializer toolInitializer and assign it in the constructor) so the
initializer's state is retained and can be used later; update the class
constructor and any references to use toolInitializer if kept, otherwise simply
delete the line to avoid a no-op side effect.
🪄 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: bf0b69c2-08c1-44a1-9f5f-a41a750edc3e
📒 Files selected for processing (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java
Comment on lines
+438
to
+467
| var newJson = configParser.getEimJson(true); | ||
| List<IdfRow> rows = List.of(); | ||
|
|
||
| if (newJson != null && newJson.getIdfInstalled() != null) | ||
| { | ||
| var gitPath = newJson.getGitPath(); | ||
|
|
||
| monitor.subTask("Detecting ESP-IDF versions..."); | ||
|
|
||
| try (var executor = Executors.newVirtualThreadPerTaskExecutor()) | ||
| { | ||
| var futures = newJson.getIdfInstalled().stream().map(idf -> CompletableFuture.supplyAsync( | ||
| () -> new IdfRow(idf, ToolsUtility.isIdfInstalledActive(idf), | ||
| ToolsUtility.getIdfVersion(idf, gitPath), idf.getName(), idf.getPath()), | ||
| executor)).toList(); | ||
|
|
||
| rows = futures.stream().map(CompletableFuture::join).toList(); | ||
| } | ||
| } | ||
|
|
||
| final List<IdfRow> finalRows = rows; | ||
|
|
||
| Display.getDefault().asyncExec(() -> { | ||
| if (container.isDisposed()) | ||
| return; | ||
|
|
||
| var currentSelection = tableViewer.getSelection(); | ||
| currentInstallingNode = null; | ||
|
|
||
| tableViewer.setInput(finalRows); |
There was a problem hiding this comment.
Keep eimJson synchronized with the refreshed configuration.
Rows are rebuilt from newJson, but performToolsSetup() still uses the stale eimJson field when creating SetupToolsInIde. Refresh the field on the UI thread before exposing the new rows.
🐛 Proposed fix
var newJson = configParser.getEimJson(true);
List<IdfRow> rows = List.of();
@@
}
+ final EimJson finalJson = newJson;
final List<IdfRow> finalRows = rows;
Display.getDefault().asyncExec(() -> {
if (container.isDisposed())
return;
var currentSelection = tableViewer.getSelection();
+ this.eimJson = finalJson;
currentInstallingNode = null;
tableViewer.setInput(finalRows);🤖 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/manager/pages/ESPIDFMainTablePage.java`
around lines 438 - 467, Rows are built from the local newJson but the shared
field eimJson remains stale, causing performToolsSetup() (and SetupToolsInIde
construction) to operate on old data; inside the
Display.getDefault().asyncExec(...) block, assign the freshly parsed newJson to
the eimJson field (e.g., eimJson = newJson) before calling
tableViewer.setInput(finalRows) or any code that may trigger
performToolsSetup()/new SetupToolsInIde, ensuring UI-thread synchronization of
the configuration state.
Comment on lines
+478
to
+480
| } finally | ||
| { | ||
| monitor.done(); |
There was a problem hiding this comment.
Match the project brace style for finally.
The repository formatter requires block braces on the next line; split finally from the preceding closing brace. As per coding guidelines, “Place opening brace on the next line for types, methods, constructors, blocks, and switch statements.”
🎨 Proposed formatting fix
- } finally
+ }
+ finally
{
monitor.done();📝 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
| } finally | |
| { | |
| monitor.done(); | |
| } | |
| finally | |
| { | |
| monitor.done(); |
🤖 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/manager/pages/ESPIDFMainTablePage.java`
around lines 478 - 480, The finally block formatting must follow the project
brace style: move the `finally` keyword to its own line so the closing brace of
the try/catch ends on one line and `finally` begins the next, e.g. change the
sequence that currently reads like `} finally {` (around the block that calls
`monitor.done()`) to `}\nfinally\n{`, ensuring the block that contains
`monitor.done()` retains its contents unchanged.
sigmaaa
changed the title
Collaborator
|
@sigmaaa hi ! The original delay on Windows machines was improved. LGTM 👍 |
AndriiFilippov
approved these changes
Apr 23, 2026
sigmaaa
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (2)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (2)
478-480:⚠️ Potential issue | 🟡 MinorMatch the project brace style for
finally.
finallyshould start on its own line here to stay formatter-clean.🎨 Proposed formatting fix
- } finally + } + finally { monitor.done();As per coding guidelines, "Place opening brace on the next line for types, methods, constructors, blocks, and
switchstatements".🤖 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/manager/pages/ESPIDFMainTablePage.java` around lines 478 - 480, The finally keyword is not placed on its own line per project brace style; in the ESPIDFMainTablePage class's finally block move the `finally` so it appears on its own line and ensure its opening brace is on the next line (i.e., change the current "} finally {" sequence to "}" then a newline with "finally" and a newline with "{", leaving the monitor.done() call inside).
438-469:⚠️ Potential issue | 🟠 MajorRefresh
eimJsontogether with the new rows.Rows are rebuilt from
newJson, butperformToolsSetup()still reads the stale field at Line 419. After a refresh, activate/reinstall can run against outdated config.🐛 Proposed fix
var newJson = configParser.getEimJson(true); List<IdfRow> rows = List.of(); @@ - final List<IdfRow> finalRows = rows; + final EimJson finalJson = newJson; + final List<IdfRow> finalRows = rows; Display.getDefault().asyncExec(() -> { if (container.isDisposed()) return; var currentSelection = tableViewer.getSelection(); + eimJson = finalJson; currentInstallingNode = null; tableViewer.setInput(finalRows);🤖 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/manager/pages/ESPIDFMainTablePage.java` around lines 438 - 469, The rows are built from configParser.getEimJson(true) into local newJson but the shared eimJson used by performToolsSetup() remains stale; update the shared state by assigning the refreshed newJson to the instance field (e.g., eimJson) before UI actions so performToolsSetup(), activate/reinstall handlers, and updateButtonState() see the new config. Concretely, after creating newJson and before calling tableViewer.setInput(finalRows) (inside the Display.asyncExec block) set the instance eimJson = newJson (or call the class method that refreshes it) so all subsequent code (performToolsSetup(), updateButtonState(), etc.) uses the refreshed configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java`:
- Around line 478-480: The finally keyword is not placed on its own line per
project brace style; in the ESPIDFMainTablePage class's finally block move the
`finally` so it appears on its own line and ensure its opening brace is on the
next line (i.e., change the current "} finally {" sequence to "}" then a newline
with "finally" and a newline with "{", leaving the monitor.done() call inside).
- Around line 438-469: The rows are built from configParser.getEimJson(true)
into local newJson but the shared eimJson used by performToolsSetup() remains
stale; update the shared state by assigning the refreshed newJson to the
instance field (e.g., eimJson) before UI actions so performToolsSetup(),
activate/reinstall handlers, and updateButtonState() see the new config.
Concretely, after creating newJson and before calling
tableViewer.setInput(finalRows) (inside the Display.asyncExec block) set the
instance eimJson = newJson (or call the class method that refreshes it) so all
subsequent code (performToolsSetup(), updateButtonState(), etc.) uses the
refreshed configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22b8c70b-e7c1-49a7-8a3d-ec2bc02b761c
📒 Files selected for processing (3)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/Messages.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties
✅ Files skipped from review due to trivial changes (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/messages.properties
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
Currently, the refreshEditorUI method runs the version detection command synchronously for every configured ESP-IDF environment. If a user has multiple environments, this significantly prolongs the refresh process and freezes the Eclipse UI.
Fix: Offload the version detection commands to run concurrently in the background using Virtual Threads.
Wrapped the process in an Eclipse Job so the UI remains completely responsive, and the user sees a progress monitor.
Fixes # (IEP-1754)
Type of change
Please delete options that are not relevant.
How has this been tested?
Install multiple ESP-IDF versions → open the ESP-IDF Manager and track how long it takes to display all ESP-IDF versions.
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit