Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds server-driven defaults and a RESET command to the config protocol; output now exposes version and defaults; server can send/reset defaults; model tracks default state; UI gets a new renderer and action handler; editor wires reset actions and uses server-reported version to gate behavior. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as Config UI Renderer
participant Editor as SDKConfigurationEditor
participant Server as JsonConfigServer
participant Output as JsonConfigOutput
User->>UI: click reset button / menu reset
UI->>Editor: onResetRequested / onMenuResetRequested
Editor->>Editor: check isResetSupported (output.getVersion() >= 3)
Editor->>Server: resetElementById(id) / resetElementChildren(list)
Server->>Server: build JSON {version: X, reset:[...]}
Server->>Server: execute(..., CommandType.RESET)
Server-->>Output: response JSON
Output->>Output: parse() reads VERSION and DEFAULTS
Editor->>Editor: notifyRequestServed(RESET) -> clear modified entries
Editor->>UI: renderFullMenu (uses defaults/values maps)
UI->>User: controls reflect reset/default values
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 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.sdk.config.core/src/com/espressif/idf/sdk/config/core/server/JsonConfigOutput.java (1)
72-100:⚠️ Potential issue | 🟠 MajorUpdate parsing still drops
defaultsand assumesvisible/rangesalways exist.
SDKConfigurationEditor.update()always callsparse(..., true)for SET/RESET responses, but this branch readsdefaultsJsonand then discards it, sogetDefaultsJsonMap()stays stale after a reset. It also dereferencesvisibleJsonandrangesJsonunconditionally, which will NPE if a minimal update payload is accepted upstream.Possible fix
if (isUpdate) { // newly updated values and visible items JSONObject visibleJson = (JSONObject) jsonObj.get(IJsonServerConfig.VISIBLE); JSONObject valuesJson = (JSONObject) jsonObj.get(IJsonServerConfig.VALUES); JSONObject rangesJson = (JSONObject) jsonObj.get(IJsonServerConfig.RANGES); JSONObject defaultsJson = (JSONObject) jsonObj.get(IJsonServerConfig.DEFAULTS); - // Updated visible items - Set<String> newVisibleKeyset = visibleJson.keySet(); - for (String key : newVisibleKeyset) - { - visibleJsonMap.put(key, visibleJson.get(key)); - } + if (visibleJson != null) + { + Set<String> newVisibleKeyset = visibleJson.keySet(); + for (String key : newVisibleKeyset) + { + visibleJsonMap.put(key, visibleJson.get(key)); + } + } // Updated values Set<String> newValuesKeyset = valuesJson.keySet(); for (String key : newValuesKeyset) { valuesJsonMap.put(key, valuesJson.get(key)); } - // Updated ranges - Set<String> newRangesKeyset = rangesJson.keySet(); - for (String key : newRangesKeyset) - { - rangesJsonMap.put(key, rangesJson.get(key)); - } + if (rangesJson != null) + { + Set<String> newRangesKeyset = rangesJson.keySet(); + for (String key : newRangesKeyset) + { + rangesJsonMap.put(key, rangesJson.get(key)); + } + } + + if (defaultsJson != null) + { + if (defaultsJsonMap == null) + { + defaultsJsonMap = new JSONObject(); + } + Set<String> newDefaultsKeyset = defaultsJson.keySet(); + for (String key : newDefaultsKeyset) + { + defaultsJsonMap.put(key, defaultsJson.get(key)); + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundles/com.espressif.idf.sdk.config.core/src/com/espressif/idf/sdk/config/core/server/JsonConfigOutput.java` around lines 72 - 100, In the isUpdate branch inside JsonConfigOutput.parse(...) ensure you null-check the JSONObject variables before using them (visibleJson, valuesJson, rangesJson, defaultsJson) to avoid NPEs on minimal update payloads, and actually apply the defaultsJson into defaultsJsonMap (similar to how visibleJsonMap/valuesJsonMap/rangesJsonMap are updated) instead of discarding it; update the loops that iterate newVisibleKeyset/newValuesKeyset/newRangesKeyset to only run when the corresponding JSONObject is non-null and add a new loop that copies entries from defaultsJson into defaultsJsonMap when defaultsJson != null.bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java (1)
1185-1196:⚠️ Potential issue | 🔴 Critical
getCurrentBuildFolder()now always returns"".The new call to
IDFUtil.getBuildDir(project)ignores its return value, sorollbackBuildFolder(buildFolder)restores an empty path after the editor opens. That will break the build-folder sdkconfig flow.Possible fix
private String getCurrentBuildFolder() { - String buildFolder = StringUtil.EMPTY; try { - IDFUtil.getBuildDir(project); + return IDFUtil.getBuildDir(project); } catch (CoreException e) { Logger.log(e); } - return buildFolder; + return StringUtil.EMPTY; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java` around lines 1185 - 1196, getCurrentBuildFolder() currently ignores the return value of IDFUtil.getBuildDir(project) so it always returns an empty string; change the method to assign the result of IDFUtil.getBuildDir(project) to buildFolder (e.g., buildFolder = IDFUtil.getBuildDir(project)) and return that value, keeping the existing CoreException handling (Logger.log(e)) so rollbackBuildFolder(buildFolder) receives the actual build folder instead of "".
🤖 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.sdk.config.core/src/com/espressif/idf/sdk/config/core/server/JsonConfigServerRunnable.java`:
- Around line 205-210: The current validation in JsonConfigServerRunnable that
returns only when both IJsonServerConfig.VERSION and IJsonServerConfig.VALUES
are present should be relaxed so VERSION is optional; update the check in the
JSON parsing logic to require only IJsonServerConfig.VALUES (leave
IJsonServerConfig.VERSION optional because JsonConfigOutput falls back to
protocol version 1) so the parsed payload is passed through to
notifyHandler(...); modify the condition that references
IJsonServerConfig.VERSION and IJsonServerConfig.VALUES to only assert the
presence of IJsonServerConfig.VALUES.
In
`@bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java`:
- Around line 1108-1114: The current logic clears modifiedJsonMap for both
CommandType.LOAD and CommandType.RESET, which discards unrelated unsaved text
edits; change it so that for CommandType.LOAD you still call
modifiedJsonMap.clear(), but for CommandType.RESET only remove entries
corresponding to the IDs being reset (e.g. iterate the reset IDs from the
command/payload and call modifiedJsonMap.remove(id) for each). Keep the isDirty
assignment (isDirty = (type == CommandType.RESET)) and the
Display.getDefault().asyncExec(this::editorDirtyStateChanged) call unchanged;
locate this logic around the handler that receives the reset/load command (the
block referencing CommandType.LOAD, CommandType.RESET, modifiedJsonMap, and
editorDirtyStateChanged) and replace the blanket clear with per-id removals for
RESET.
- Around line 668-678: collectAllChildIds currently gathers only descendant ids
for a selectedElement and when childIds is non-empty the code calls
executeResetChildrenCommand(childIds) but skips resetting the selected element
itself (e.g., a menuconfig node with its own id). Fix by ensuring the
selectedElement's id is included before calling executeResetChildrenCommand: if
selectedElement.getId() != null add that id to childIds (or build a reset list
that begins with selectedElement.getId()) so both executeResetChildrenCommand
and executeResetCommand behavior reset the parent id as well; update places
using collectAllChildIds / executeResetChildrenCommand (including the similar
block around lines 720-733) to include the parent id when present.
- Around line 681-699: The per-item reset control in addResetButton currently
creates a Label (resetIcon) with a SWT.MouseUp listener, which is not
keyboard-focusable; replace that Label with a real Button (use SWT.PUSH |
SWT.FLAT) so it is focusable and accessible, set the same icon via
SDKConfigUIPlugin.getImage(ICONS_SDK_RESET_ACTION_PNG), add a SelectionListener
(or use addListener for SWT.Selection) to invoke
executeResetCommand(kConfigMenuItem.getId()) when activated, and keep
applyResetSupportState(resetIconComposite, ..., activeTooltip) to preserve
tooltip/enablement behavior; ensure the new Button is given the same layout and
layout data as resetIconComposite so visual layout remains unchanged.
---
Outside diff comments:
In
`@bundles/com.espressif.idf.sdk.config.core/src/com/espressif/idf/sdk/config/core/server/JsonConfigOutput.java`:
- Around line 72-100: In the isUpdate branch inside JsonConfigOutput.parse(...)
ensure you null-check the JSONObject variables before using them (visibleJson,
valuesJson, rangesJson, defaultsJson) to avoid NPEs on minimal update payloads,
and actually apply the defaultsJson into defaultsJsonMap (similar to how
visibleJsonMap/valuesJsonMap/rangesJsonMap are updated) instead of discarding
it; update the loops that iterate
newVisibleKeyset/newValuesKeyset/newRangesKeyset to only run when the
corresponding JSONObject is non-null and add a new loop that copies entries from
defaultsJson into defaultsJsonMap when defaultsJson != null.
In
`@bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java`:
- Around line 1185-1196: getCurrentBuildFolder() currently ignores the return
value of IDFUtil.getBuildDir(project) so it always returns an empty string;
change the method to assign the result of IDFUtil.getBuildDir(project) to
buildFolder (e.g., buildFolder = IDFUtil.getBuildDir(project)) and return that
value, keeping the existing CoreException handling (Logger.log(e)) so
rollbackBuildFolder(buildFolder) receives the actual build folder instead of "".
🪄 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: 7f8300e7-c690-446a-a923-f331afc0db1d
⛔ Files ignored due to path filters (1)
bundles/com.espressif.idf.sdk.config.ui/icons/reset.pngis excluded by!**/*.png
📒 Files selected for processing (8)
bundles/com.espressif.idf.sdk.config.core/src/com/espressif/idf/sdk/config/core/IJsonServerConfig.javabundles/com.espressif.idf.sdk.config.core/src/com/espressif/idf/sdk/config/core/KConfigMenuItem.javabundles/com.espressif.idf.sdk.config.core/src/com/espressif/idf/sdk/config/core/server/CommandType.javabundles/com.espressif.idf.sdk.config.core/src/com/espressif/idf/sdk/config/core/server/IJsonConfigOutput.javabundles/com.espressif.idf.sdk.config.core/src/com/espressif/idf/sdk/config/core/server/JsonConfigOutput.javabundles/com.espressif.idf.sdk.config.core/src/com/espressif/idf/sdk/config/core/server/JsonConfigServer.javabundles/com.espressif.idf.sdk.config.core/src/com/espressif/idf/sdk/config/core/server/JsonConfigServerRunnable.javabundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java
Comment on lines
205
to
+210
| try | ||
| { | ||
| JSONObject jsonObj = (JSONObject) new JSONParser().parse(jsonOutput); | ||
| if (jsonObj != null) | ||
| { | ||
| if (jsonObj.get(IJsonServerConfig.VISIBLE) != null && jsonObj.get(IJsonServerConfig.VALUES) != null | ||
| && jsonObj.get(IJsonServerConfig.RANGES) != null) | ||
| { | ||
| return true; | ||
| } | ||
| return jsonObj.containsKey(IJsonServerConfig.VERSION) && jsonObj.containsKey(IJsonServerConfig.VALUES); |
There was a problem hiding this comment.
Keep version optional in response validation.
JsonConfigOutput already falls back to protocol version 1 when the field is missing. Making VERSION mandatory here means older config-server payloads never reach notifyHandler(...), so the editor just times out instead of loading.
Possible fix
- return jsonObj.containsKey(IJsonServerConfig.VERSION) && jsonObj.containsKey(IJsonServerConfig.VALUES);
+ return jsonObj.containsKey(IJsonServerConfig.VALUES);📝 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
| try | |
| { | |
| JSONObject jsonObj = (JSONObject) new JSONParser().parse(jsonOutput); | |
| if (jsonObj != null) | |
| { | |
| if (jsonObj.get(IJsonServerConfig.VISIBLE) != null && jsonObj.get(IJsonServerConfig.VALUES) != null | |
| && jsonObj.get(IJsonServerConfig.RANGES) != null) | |
| { | |
| return true; | |
| } | |
| return jsonObj.containsKey(IJsonServerConfig.VERSION) && jsonObj.containsKey(IJsonServerConfig.VALUES); | |
| try | |
| { | |
| JSONObject jsonObj = (JSONObject) new JSONParser().parse(jsonOutput); | |
| if (jsonObj != null) | |
| { | |
| return jsonObj.containsKey(IJsonServerConfig.VALUES); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@bundles/com.espressif.idf.sdk.config.core/src/com/espressif/idf/sdk/config/core/server/JsonConfigServerRunnable.java`
around lines 205 - 210, The current validation in JsonConfigServerRunnable that
returns only when both IJsonServerConfig.VERSION and IJsonServerConfig.VALUES
are present should be relaxed so VERSION is optional; update the check in the
JSON parsing logic to require only IJsonServerConfig.VALUES (leave
IJsonServerConfig.VERSION optional because JsonConfigOutput falls back to
protocol version 1) so the parsed payload is passed through to
notifyHandler(...); modify the condition that references
IJsonServerConfig.VERSION and IJsonServerConfig.VALUES to only assert the
presence of IJsonServerConfig.VALUES.
....espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java
Outdated
Show resolved
Hide resolved
....espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java
Outdated
Show resolved
Hide resolved
....espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
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.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java (1)
822-834:⚠️ Potential issue | 🔴 Critical
getCurrentBuildFolderdiscards the result and always returns empty string.The method calls
IDFUtil.getBuildDir(project)but doesn't assign the result tobuildFolder. This meansrollbackBuildFolderwill always receive an empty string, potentially breaking the build folder restoration logic.Proposed fix
private String getCurrentBuildFolder() { String buildFolder = StringUtil.EMPTY; try { - IDFUtil.getBuildDir(project); + buildFolder = IDFUtil.getBuildDir(project); } catch (CoreException e) { Logger.log(e); } return buildFolder; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java` around lines 822 - 834, The method getCurrentBuildFolder currently ignores the return value of IDFUtil.getBuildDir(project) and always returns an empty string; update getCurrentBuildFolder so it assigns the result of IDFUtil.getBuildDir(project) to the local variable (buildFolder) before returning it, preserving the existing CoreException catch (Logger.log(e)) behavior so rollbackBuildFolder receives the actual build folder string instead of StringUtil.EMPTY.
🤖 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.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/ConfigUIRenderer.java`:
- Around line 279-306: In addResetButton(RenderContext ctx) replace the SWT
Label used as resetIcon with a Button created with SWT.PUSH | SWT.FLAT so the
control can receive keyboard focus; set the button image via
SDKConfigUIPlugin.getImage(ICONS_SDK_RESET_ACTION_PNG), mirror the existing
enabled/tooltip logic (use setEnabled and setToolTipText on the Button), and
replace the SWT.MouseUp listener with a SelectionListener (or
addSelectionListener) that calls actionHandler.onResetRequested(ctx.configKey())
when ctx.configKey() != null; keep using resetIconComposite and isResetSupported
to decide enabled state and tooltip text and preserve the existing title text
via ctx.item().getTitle().
- Around line 270-276: The code currently calls
PlatformUI.getWorkbench().getActiveWorkbenchWindow().getShell() which can NPE if
no workbench window exists; before calling .getShell() retrieve the
IWorkbenchWindow via PlatformUI.getWorkbench().getActiveWorkbenchWindow(), check
it for null and use a safe fallback (e.g., Display.getDefault().getActiveShell()
or null-check and abort/skip showing the dialog) when creating activeShell for
the HelpPopupDialog; update the block that assigns activeShell, constructs new
HelpPopupDialog(...) and opens infoDialog (referencing
PlatformUI.getWorkbench().getActiveWorkbenchWindow(), activeShell,
HelpPopupDialog, infoDialog and ctx.configKey()) so it only uses a non-null
Shell or safely exits.
- Around line 135-139: The current code in ConfigUIRenderer.java casts
ctx.modifiedValue() and ctx.configValue() directly to (long), which can throw
ClassCastException if the JSON value is an Integer or other Number type; change
the logic to treat values as Number: retrieve ctx.modifiedValue() and
ctx.configValue() as Number, pick the non-null Number (prefer modified), call
number.longValue() to obtain a long, and then format with Long.toHexString(...)
for textControl.setText(...); also keep the existing null check
(ctx.configValue() != null) and handle possible ClassCastException by ensuring
you only cast to Number before calling longValue().
- Around line 157-175: renderInt currently sends the raw trimmed string to
actionHandler.onTextModified; change the modify listener in renderInt so it
parses the trimmed text into a numeric value (use Long.decode(trimmed) to accept
decimal and hex) and pass the resulting Long to
actionHandler.onTextModified(ctx.configKey(), parsedValue); catch
NumberFormatException and pass null (or a sensible numeric-empty sentinel) to
preserve type consistency with renderHex and avoid sending plain strings for
integer config values.
In
`@bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java`:
- Around line 669-672: In SDKConfigurationEditor replace the incorrect loop that
iterates jsonObj.entrySet() treating each element as a key: in the block where
jsonObj.entrySet() is iterated (same pattern as the doSave bug) change the
iteration to use Map.Entry (e.g., iterate as Map.Entry<?,?> entry) and call
entry.getKey() and entry.getValue() when populating valuesJsonMap (use the
existing jsonObj and valuesJsonMap symbols to locate the code).
- Around line 461-465: The loop over modifiedJsonMap.entrySet() is treating each
element as a key, causing Map.Entry objects to be used as keys and
modifiedJsonMap.get(key) to return null; change the iteration to unpack entries
and use their key/value (e.g., iterate as Map.Entry<?,?> entry and call
valuesJsonMap.put(entry.getKey(), entry.getValue())) or iterate
modifiedJsonMap.keySet() and use the key to fetch the value; locate this block
in SDKConfigurationEditor where modifiedJsonMap and valuesJsonMap are used and
replace the incorrect for-loop accordingly.
- Around line 610-624: onMenuResetRequested currently collects only descendant
IDs via collectAllChildIds and calls executeResetChildrenCommand(childIds),
which skips the parent menu's own ID when the menu has both an id and children;
modify onMenuResetRequested so that if menu.getId() is non-null/non-empty you
insert the parent's id into childIds (e.g., at front) before calling
executeResetChildrenCommand, ensuring the parent id is included in the reset
call instead of relying on the else branch that only runs when there are no
children.
---
Outside diff comments:
In
`@bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java`:
- Around line 822-834: The method getCurrentBuildFolder currently ignores the
return value of IDFUtil.getBuildDir(project) and always returns an empty string;
update getCurrentBuildFolder so it assigns the result of
IDFUtil.getBuildDir(project) to the local variable (buildFolder) before
returning it, preserving the existing CoreException catch (Logger.log(e))
behavior so rollbackBuildFolder receives the actual build folder string instead
of StringUtil.EMPTY.
🪄 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: 040dfb5c-1117-4450-b854-b0920291e64e
📒 Files selected for processing (3)
bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/ConfigActionHandler.javabundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/ConfigUIRenderer.javabundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java
Comment on lines
+135
to
+139
| if (ctx.configValue() != null) | ||
| { | ||
| long valToFormat = ctx.modifiedValue() != null ? (long) ctx.modifiedValue() : (long) ctx.configValue(); | ||
| textControl.setText("0x" + Long.toHexString(valToFormat).toUpperCase()); | ||
| } |
There was a problem hiding this comment.
Potential ClassCastException when casting config values to long.
The cast (long) ctx.modifiedValue() and (long) ctx.configValue() assumes the JSON values are always Long. If the server returns an Integer or the value was parsed differently, this will throw ClassCastException.
Proposed fix using Number for safer casting
if (ctx.configValue() != null)
{
- long valToFormat = ctx.modifiedValue() != null ? (long) ctx.modifiedValue() : (long) ctx.configValue();
+ Number modified = (Number) ctx.modifiedValue();
+ Number config = (Number) ctx.configValue();
+ long valToFormat = modified != null ? modified.longValue() : config.longValue();
textControl.setText("0x" + Long.toHexString(valToFormat).toUpperCase());
}📝 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 (ctx.configValue() != null) | |
| { | |
| long valToFormat = ctx.modifiedValue() != null ? (long) ctx.modifiedValue() : (long) ctx.configValue(); | |
| textControl.setText("0x" + Long.toHexString(valToFormat).toUpperCase()); | |
| } | |
| if (ctx.configValue() != null) | |
| { | |
| Number modified = (Number) ctx.modifiedValue(); | |
| Number config = (Number) ctx.configValue(); | |
| long valToFormat = modified != null ? modified.longValue() : config.longValue(); | |
| textControl.setText("0x" + Long.toHexString(valToFormat).toUpperCase()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/ConfigUIRenderer.java`
around lines 135 - 139, The current code in ConfigUIRenderer.java casts
ctx.modifiedValue() and ctx.configValue() directly to (long), which can throw
ClassCastException if the JSON value is an Integer or other Number type; change
the logic to treat values as Number: retrieve ctx.modifiedValue() and
ctx.configValue() as Number, pick the non-null Number (prefer modified), call
number.longValue() to obtain a long, and then format with Long.toHexString(...)
for textControl.setText(...); also keep the existing null check
(ctx.configValue() != null) and handle possible ClassCastException by ensuring
you only cast to Number before calling longValue().
Comment on lines
+157
to
+175
| private void renderInt(RenderContext ctx) | ||
| { | ||
| var labelName = new Label(parent, SWT.NONE); | ||
| labelName.setText(ctx.item().getTitle()); | ||
|
|
||
| var textControl = new Text(parent, SWT.SINGLE | SWT.BORDER); | ||
| var gridData = new GridData(); | ||
| gridData.widthHint = 250; | ||
| textControl.setLayoutData(gridData); | ||
| textControl.setToolTipText(ctx.helpInfo()); | ||
|
|
||
| if (ctx.configValue() != null) | ||
| { | ||
| textControl.setText(ctx.modifiedValue() != null ? String.valueOf(ctx.modifiedValue()) | ||
| : String.valueOf(ctx.configValue())); | ||
| } | ||
|
|
||
| textControl.addModifyListener(e -> actionHandler.onTextModified(ctx.configKey(), textControl.getText().trim())); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how int values are handled elsewhere in the codebase
rg -n "INT_TYPE" --type java -A 5 -B 2 | head -60Repository: espressif/idf-eclipse-plugin
Length of output: 3569
🏁 Script executed:
#!/bin/bash
# Get renderHex method to compare parsing behavior
rg -n "private void renderHex" --type java -A 25 bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/ConfigUIRenderer.javaRepository: espressif/idf-eclipse-plugin
Length of output: 1052
🏁 Script executed:
#!/bin/bash
# Check onTextModified handler implementation
rg -n "onTextModified" --type java -B 2 -A 10 bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/ | head -100Repository: espressif/idf-eclipse-plugin
Length of output: 7802
🏁 Script executed:
#!/bin/bash
# Check how values are stored in modifiedJsonMap
rg -n "modifiedJsonMap.put\|modifiedJsonMap.get" --type java -B 2 -A 2 | head -80Repository: espressif/idf-eclipse-plugin
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Find where modifiedJsonMap is used for server communication
rg -n "modifiedJsonMap" --type java -B 2 -A 5 bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java | head -120Repository: espressif/idf-eclipse-plugin
Length of output: 1600
🏁 Script executed:
#!/bin/bash
# Check if there's JSON serialization happening
rg -n "JSONObject\|JSONArray\|toJson" --type java bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java | head -40Repository: espressif/idf-eclipse-plugin
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Look for tests related to int configuration
find . -name "*Test*.java" -o -name "*test*.java" | xargs grep -l "INT_TYPE\|renderInt" 2>/dev/null | head -5Repository: espressif/idf-eclipse-plugin
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Check ConfigServer to see if it handles type conversion
rg -n "ConfigServer\|execute" --type java bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/ | grep -i "class ConfigServer\|public class" | head -10Repository: espressif/idf-eclipse-plugin
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Check if there's any type validation for int values
rg -n "parseInt\|parseLong\|Integer.parse\|Long.parse" --type java bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/ConfigUIRenderer.javaRepository: espressif/idf-eclipse-plugin
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Check how configValue is retrieved and used for int type
rg -n "ctx.configValue\|ctx.modifiedValue" --type java -B 2 -A 2 bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/ConfigUIRenderer.java | grep -A 8 "renderInt" | head -30Repository: espressif/idf-eclipse-plugin
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Check RenderContext to understand value types
find . -name "RenderContext.java" -exec cat {} \;Repository: espressif/idf-eclipse-plugin
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Search for where values come from - valuesJsonMap
rg -n "valuesJsonMap\|get\(" --type java bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/ConfigUIRenderer.java | head -40Repository: espressif/idf-eclipse-plugin
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Get complete renderInt, renderString, and renderHex methods for side-by-side comparison
sed -n '109,122p' bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/ConfigUIRenderer.java
echo "---RENDERSTRING ENDS---"
sed -n '124,155p' bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/ConfigUIRenderer.java
echo "---RENDERHEX ENDS---"
sed -n '157,175p' bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/ConfigUIRenderer.java
echo "---RENDERINT ENDS---"Repository: espressif/idf-eclipse-plugin
Length of output: 2120
Integer values are sent as strings instead of parsed numbers.
In renderInt, user input is passed as a string to onTextModified, unlike renderHex which explicitly parses the input to long before calling onTextModified. Since integer configuration values are expected to be numeric types (like hex values), the string representation should be parsed to maintain type consistency.
Proposed fix to parse integer value
- textControl.addModifyListener(e -> actionHandler.onTextModified(ctx.configKey(), textControl.getText().trim()));
+ textControl.addModifyListener(e -> {
+ String text = textControl.getText().trim();
+ try
+ {
+ long intVal = Long.parseLong(text);
+ actionHandler.onTextModified(ctx.configKey(), intVal);
+ }
+ catch (NumberFormatException ignored)
+ {
+ // Ignore invalid input during typing
+ }
+ });📝 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
| private void renderInt(RenderContext ctx) | |
| { | |
| var labelName = new Label(parent, SWT.NONE); | |
| labelName.setText(ctx.item().getTitle()); | |
| var textControl = new Text(parent, SWT.SINGLE | SWT.BORDER); | |
| var gridData = new GridData(); | |
| gridData.widthHint = 250; | |
| textControl.setLayoutData(gridData); | |
| textControl.setToolTipText(ctx.helpInfo()); | |
| if (ctx.configValue() != null) | |
| { | |
| textControl.setText(ctx.modifiedValue() != null ? String.valueOf(ctx.modifiedValue()) | |
| : String.valueOf(ctx.configValue())); | |
| } | |
| textControl.addModifyListener(e -> actionHandler.onTextModified(ctx.configKey(), textControl.getText().trim())); | |
| } | |
| private void renderInt(RenderContext ctx) | |
| { | |
| var labelName = new Label(parent, SWT.NONE); | |
| labelName.setText(ctx.item().getTitle()); | |
| var textControl = new Text(parent, SWT.SINGLE | SWT.BORDER); | |
| var gridData = new GridData(); | |
| gridData.widthHint = 250; | |
| textControl.setLayoutData(gridData); | |
| textControl.setToolTipText(ctx.helpInfo()); | |
| if (ctx.configValue() != null) | |
| { | |
| textControl.setText(ctx.modifiedValue() != null ? String.valueOf(ctx.modifiedValue()) | |
| : String.valueOf(ctx.configValue())); | |
| } | |
| textControl.addModifyListener(e -> { | |
| String text = textControl.getText().trim(); | |
| try | |
| { | |
| long intVal = Long.parseLong(text); | |
| actionHandler.onTextModified(ctx.configKey(), intVal); | |
| } | |
| catch (NumberFormatException ignored) | |
| { | |
| // Ignore invalid input during typing | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/ConfigUIRenderer.java`
around lines 157 - 175, renderInt currently sends the raw trimmed string to
actionHandler.onTextModified; change the modify listener in renderInt so it
parses the trimmed text into a numeric value (use Long.decode(trimmed) to accept
decimal and hex) and pass the resulting Long to
actionHandler.onTextModified(ctx.configKey(), parsedValue); catch
NumberFormatException and pass null (or a sensible numeric-empty sentinel) to
preserve type consistency with renderHex and avoid sending plain strings for
integer config values.
Comment on lines
+270
to
+276
| var activeShell = PlatformUI.getWorkbench().getActiveWorkbenchWindow().getShell(); | ||
| if (infoDialog != null) | ||
| infoDialog.close(); | ||
|
|
||
| infoDialog = new HelpPopupDialog(activeShell, "Help > " + ctx.configKey(), message); | ||
| infoDialog.open(); | ||
| }); |
There was a problem hiding this comment.
Potential NPE if no active workbench window.
PlatformUI.getWorkbench().getActiveWorkbenchWindow() can return null when no workbench window is active, which would cause a NullPointerException on .getShell().
Proposed fix with null check
- var activeShell = PlatformUI.getWorkbench().getActiveWorkbenchWindow().getShell();
+ var workbenchWindow = PlatformUI.getWorkbench().getActiveWorkbenchWindow();
+ if (workbenchWindow == null)
+ return;
+ var activeShell = workbenchWindow.getShell();
if (infoDialog != null)
infoDialog.close();📝 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
| var activeShell = PlatformUI.getWorkbench().getActiveWorkbenchWindow().getShell(); | |
| if (infoDialog != null) | |
| infoDialog.close(); | |
| infoDialog = new HelpPopupDialog(activeShell, "Help > " + ctx.configKey(), message); | |
| infoDialog.open(); | |
| }); | |
| var workbenchWindow = PlatformUI.getWorkbench().getActiveWorkbenchWindow(); | |
| if (workbenchWindow == null) | |
| return; | |
| var activeShell = workbenchWindow.getShell(); | |
| if (infoDialog != null) | |
| infoDialog.close(); | |
| infoDialog = new HelpPopupDialog(activeShell, "Help > " + ctx.configKey(), message); | |
| infoDialog.open(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/ConfigUIRenderer.java`
around lines 270 - 276, The code currently calls
PlatformUI.getWorkbench().getActiveWorkbenchWindow().getShell() which can NPE if
no workbench window exists; before calling .getShell() retrieve the
IWorkbenchWindow via PlatformUI.getWorkbench().getActiveWorkbenchWindow(), check
it for null and use a safe fallback (e.g., Display.getDefault().getActiveShell()
or null-check and abort/skip showing the dialog) when creating activeShell for
the HelpPopupDialog; update the block that assigns activeShell, constructs new
HelpPopupDialog(...) and opens infoDialog (referencing
PlatformUI.getWorkbench().getActiveWorkbenchWindow(), activeShell,
HelpPopupDialog, infoDialog and ctx.configKey()) so it only uses a non-null
Shell or safely exits.
...es/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/ConfigUIRenderer.java
Show resolved
Hide resolved
....espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java
Outdated
Show resolved
Hide resolved
Comment on lines
+610
to
+624
| @Override | ||
| public void onMenuResetRequested(KConfigMenuItem menu) | ||
| { | ||
| Label labelName = new Label(updateUIComposite, SWT.NONE); | ||
| labelName.setText(kConfigMenuItem.getTitle()); | ||
|
|
||
| Text text = new Text(updateUIComposite, SWT.SINGLE | SWT.BORDER); | ||
| GridData gridData = new GridData(); | ||
| gridData.widthHint = 250; | ||
| text.setLayoutData(gridData); | ||
| text.setToolTipText(helpInfo); | ||
| List<String> childIds = new ArrayList<>(); | ||
| collectAllChildIds(menu, childIds); | ||
|
|
||
| if (configValue != null) | ||
| if (!childIds.isEmpty()) | ||
| { | ||
| text.setText(newConfigValue != null ? String.valueOf(newConfigValue) : String.valueOf(configValue)); | ||
|
|
||
| executeResetChildrenCommand(childIds); | ||
| } | ||
| text.addModifyListener(addModifyListener(configKey, text)); | ||
| addTooltipImage(kConfigMenuItem); | ||
|
|
||
| } | ||
| else if (isVisible && type.equals(IJsonServerConfig.MENU_TYPE)) | ||
| { | ||
| renderMenuItems(kConfigMenuItem); | ||
| } | ||
| else if (type.equals(IJsonServerConfig.CHOICE_TYPE)) | ||
| { | ||
| Logger.logTrace(SDKConfigUIPlugin.getDefault(), | ||
| "Config key >" + configKey + " visiblity status >" + isVisible); //$NON-NLS-1$ //$NON-NLS-2$ | ||
|
|
||
| if (isExist(visibleJsonMap, configKey)) | ||
| else if (menu.getId() != null) | ||
| { | ||
| List<KConfigMenuItem> choiceItems = kConfigMenuItem.getChildren(); | ||
| Label labelName = new Label(updateUIComposite, SWT.NONE); | ||
| labelName.setText(kConfigMenuItem.getTitle()); | ||
|
|
||
| Combo choiceCombo = new Combo(updateUIComposite, SWT.DROP_DOWN | SWT.READ_ONLY); | ||
| choiceCombo.setLayoutData(new GridData(SWT.NONE, SWT.NONE, false, false, 1, 1)); | ||
|
|
||
| GridData gridData = new GridData(); | ||
| gridData.widthHint = 250; | ||
| choiceCombo.setLayoutData(gridData); | ||
| executeResetCommand(menu.getId()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if menu items can have both an ID and children that need resetting
rg -n "menuconfig\|isMenuConfig" --type java -B 2 -A 5 | head -50Repository: espressif/idf-eclipse-plugin
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Find and examine the collectAllChildIds method
rg -n "collectAllChildIds" --type java -B 2 -A 10Repository: espressif/idf-eclipse-plugin
Length of output: 5029
🏁 Script executed:
#!/bin/bash
# Look for KConfigMenuItem class and its structure
rg -n "class KConfigMenuItem" --type java -B 2 -A 20Repository: espressif/idf-eclipse-plugin
Length of output: 2944
🏁 Script executed:
#!/bin/bash
# Check the executeResetChildrenCommand and executeResetCommand methods
rg -n "executeReset(Children)?Command" --type java -B 2 -A 8Repository: espressif/idf-eclipse-plugin
Length of output: 6380
🏁 Script executed:
#!/bin/bash
# Check git log for the mentioned commit
git log --oneline --all | grep -i "reset\|6859a6a" | head -20Repository: espressif/idf-eclipse-plugin
Length of output: 251
Parent menu's config ID is skipped when resetting a menu with both ID and children.
When onMenuResetRequested is called on a menu that has both an id and children, collectAllChildIds populates the list with only descendant IDs (never including the parent). The code then executes executeResetChildrenCommand(childIds), bypassing the parent's own ID. The parent's ID is only reset in the else if branch, which executes only when there are no children. To fix this, add the parent's ID to childIds before calling executeResetChildrenCommand:
if (menu.getId() != null && !menu.getId().isEmpty())
{
childIds.add(0, menu.getId());
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java`
around lines 610 - 624, onMenuResetRequested currently collects only descendant
IDs via collectAllChildIds and calls executeResetChildrenCommand(childIds),
which skips the parent menu's own ID when the menu has both an id and children;
modify onMenuResetRequested so that if menu.getId() is non-null/non-empty you
insert the parent's id into childIds (e.g., at front) before calling
executeResetChildrenCommand, ensuring the parent id is included in the reset
call instead of relying on the else branch that only runs when there are no
children.
....espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java
Outdated
Show resolved
Hide resolved
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)
bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java (1)
821-827:⚠️ Potential issue | 🔴 CriticalCritical:
getCurrentBuildFolder()now always returns empty.
IDFUtil.getBuildDir(project)is called on Line 821 but its return value is ignored, so rollback always restores""and can wipe the project build-dir setting.Suggested fix
private String getCurrentBuildFolder() { String buildFolder = StringUtil.EMPTY; try { - IDFUtil.getBuildDir(project); + buildFolder = IDFUtil.getBuildDir(project); } catch (CoreException e) { Logger.log(e); } return buildFolder; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java` around lines 821 - 827, getCurrentBuildFolder() is ignoring the result of IDFUtil.getBuildDir(project) so buildFolder stays empty; change the call in getCurrentBuildFolder() to assign the return value to the buildFolder variable (e.g., buildFolder = IDFUtil.getBuildDir(project)) and keep the existing CoreException catch/logger so the method returns the actual build directory when available instead of "".
♻️ Duplicate comments (2)
bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java (2)
610-620:⚠️ Potential issue | 🟠 MajorMenu reset still skips the parent item ID when children exist.
In Line 610–620, the list is built from descendants only. For menu nodes that have both
idand children, parent value is not reset.Suggested fix
public void onMenuResetRequested(KConfigMenuItem menu) { List<String> childIds = new ArrayList<>(); + if (menu.getId() != null && !menu.getId().isEmpty()) + { + childIds.add(menu.getId()); + } collectAllChildIds(menu, childIds); if (!childIds.isEmpty()) { executeResetChildrenCommand(childIds); } - else if (menu.getId() != null) + else if (menu.getId() != null) { executeResetCommand(menu.getId()); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java` around lines 610 - 620, The current logic collects only descendant IDs via collectAllChildIds(menu, childIds) and calls executeResetChildrenCommand(childIds), which omits resetting the parent when a menu node has its own id; update the flow so that if menu.getId() != null you include menu.getId() into the childIds list (or otherwise add it as the first element) before calling executeResetChildrenCommand(childIds), ensuring the parent id and all descendants are reset; reference the methods collectAllChildIds, executeResetChildrenCommand, executeResetCommand and the menu.getId() check when making the change.
748-754:⚠️ Potential issue | 🔴 CriticalCritical: RESET still clears unrelated unsaved text edits.
On Line 748–754,
modifiedJsonMap.clear()runs forCommandType.RESET, dropping pending edits not part of the reset request.Suggested fix direction
- if (type == CommandType.LOAD || type == CommandType.RESET) - { - modifiedJsonMap.clear(); - isDirty = (type == CommandType.RESET); - - Display.getDefault().asyncExec(this::editorDirtyStateChanged); - } + if (type == CommandType.LOAD) + { + modifiedJsonMap.clear(); + isDirty = false; + Display.getDefault().asyncExec(this::editorDirtyStateChanged); + } + else if (type == CommandType.RESET) + { + // remove only reset ids (track ids when RESET command is issued) + // e.g., pendingResetIds.forEach(modifiedJsonMap::remove); + isDirty = true; + Display.getDefault().asyncExec(this::editorDirtyStateChanged); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java` around lines 748 - 754, The code currently clears all pending edits by calling modifiedJsonMap.clear() for both CommandType.LOAD and CommandType.RESET in SDKConfigurationEditor, which drops unrelated unsaved text edits; change the logic so modifiedJsonMap.clear() runs only for CommandType.LOAD (or, if RESET should only remove specific keys, remove only those keys from modifiedJsonMap based on the reset payload) and keep the isDirty update and async editorDirtyStateChanged call for RESET; update the conditional around modifiedJsonMap.clear() (referencing modifiedJsonMap, CommandType.LOAD, CommandType.RESET, and the method invoking Display.getDefault().asyncExec(this::editorDirtyStateChanged)) accordingly.
🤖 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.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java`:
- Around line 454-455: The VERSION can become stale because JsonConfigOutput
only updates when the server response contains VERSION; update callers that
build outgoing jsonObject (the places that call
configServer.getOutput().getVersion() and the
jsonObject.put(IJsonServerConfig.VERSION, ...)) to prefer an explicit VERSION
already present in the current jsonObject (e.g.
jsonObject.optInt(IJsonServerConfig.VERSION, ...)) and fall back to
configServer.getOutput().getVersion(); additionally modify JsonConfigOutput
parsing to merge incremental server responses by preserving the existing stored
version when the incoming response omits VERSION so resetElementById(), other
callers, and any SAVE/SET/RESET payloads always use the latest known version.
---
Outside diff comments:
In
`@bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java`:
- Around line 821-827: getCurrentBuildFolder() is ignoring the result of
IDFUtil.getBuildDir(project) so buildFolder stays empty; change the call in
getCurrentBuildFolder() to assign the return value to the buildFolder variable
(e.g., buildFolder = IDFUtil.getBuildDir(project)) and keep the existing
CoreException catch/logger so the method returns the actual build directory when
available instead of "".
---
Duplicate comments:
In
`@bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java`:
- Around line 610-620: The current logic collects only descendant IDs via
collectAllChildIds(menu, childIds) and calls
executeResetChildrenCommand(childIds), which omits resetting the parent when a
menu node has its own id; update the flow so that if menu.getId() != null you
include menu.getId() into the childIds list (or otherwise add it as the first
element) before calling executeResetChildrenCommand(childIds), ensuring the
parent id and all descendants are reset; reference the methods
collectAllChildIds, executeResetChildrenCommand, executeResetCommand and the
menu.getId() check when making the change.
- Around line 748-754: The code currently clears all pending edits by calling
modifiedJsonMap.clear() for both CommandType.LOAD and CommandType.RESET in
SDKConfigurationEditor, which drops unrelated unsaved text edits; change the
logic so modifiedJsonMap.clear() runs only for CommandType.LOAD (or, if RESET
should only remove specific keys, remove only those keys from modifiedJsonMap
based on the reset payload) and keep the isDirty update and async
editorDirtyStateChanged call for RESET; update the conditional around
modifiedJsonMap.clear() (referencing modifiedJsonMap, CommandType.LOAD,
CommandType.RESET, and the method invoking
Display.getDefault().asyncExec(this::editorDirtyStateChanged)) accordingly.
🪄 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: cd533d45-80cc-4199-8055-8f60d38dd914
📒 Files selected for processing (1)
bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java
....espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java
Show resolved
Hide resolved
There was a problem hiding this comment.
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.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java (1)
818-830:⚠️ Potential issue | 🔴 CriticalBug: Return value of
IDFUtil.getBuildDir(project)is discarded.Line 823 calls
IDFUtil.getBuildDir(project)but doesn't assign the result tobuildFolder. The method always returnsStringUtil.EMPTY, which breaks the build folder rollback logic used at lines 172 and 199. This will incorrectly reset the project's build folder to an empty string after the editor initializes.Proposed fix
private String getCurrentBuildFolder() { String buildFolder = StringUtil.EMPTY; try { - IDFUtil.getBuildDir(project); + buildFolder = IDFUtil.getBuildDir(project); } catch (CoreException e) { Logger.log(e); } return buildFolder; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java` around lines 818 - 830, The getCurrentBuildFolder method discards the result of IDFUtil.getBuildDir(project) so it always returns StringUtil.EMPTY; update getCurrentBuildFolder to assign the return value of IDFUtil.getBuildDir(project) to buildFolder (e.g., buildFolder = IDFUtil.getBuildDir(project)) and keep the existing CoreException catch (Logger.log(e)) so that on exception it falls back to StringUtil.EMPTY; ensure the method then returns the assigned buildFolder.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java`:
- Around line 818-830: The getCurrentBuildFolder method discards the result of
IDFUtil.getBuildDir(project) so it always returns StringUtil.EMPTY; update
getCurrentBuildFolder to assign the return value of IDFUtil.getBuildDir(project)
to buildFolder (e.g., buildFolder = IDFUtil.getBuildDir(project)) and keep the
existing CoreException catch (Logger.log(e)) so that on exception it falls back
to StringUtil.EMPTY; ensure the method then returns the assigned buildFolder.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d1f7528-f7f6-4881-9031-cf75339fe325
📒 Files selected for processing (1)
bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java
There was a problem hiding this comment.
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.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java (1)
831-843:⚠️ Potential issue | 🔴 CriticalBug:
getCurrentBuildFolder()discards the result and always returns empty.The result of
IDFUtil.getBuildDir(project)is never assigned tobuildFolder, so this method always returnsStringUtil.EMPTY. Subsequently,rollbackBuildFolder(buildFolder)will set the project's build directory to an empty string instead of restoring the original value.Proposed fix
private String getCurrentBuildFolder() { String buildFolder = StringUtil.EMPTY; try { - IDFUtil.getBuildDir(project); + buildFolder = IDFUtil.getBuildDir(project); } catch (CoreException e) { Logger.log(e); } return buildFolder; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java` around lines 831 - 843, getCurrentBuildFolder() currently calls IDFUtil.getBuildDir(project) but never assigns its result, so it always returns StringUtil.EMPTY and causes rollbackBuildFolder(buildFolder) to restore an empty path; fix by assigning the method result to buildFolder (e.g., buildFolder = IDFUtil.getBuildDir(project)), keep the existing CoreException catch and Logger.log(e), and ensure the method returns the assigned buildFolder (optionally guard against null from IDFUtil.getBuildDir to return StringUtil.EMPTY if needed).
🧹 Nitpick comments (1)
bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java (1)
759-765: Fragile regex parsing for reset key extraction.The regex pattern
"Reset\\s+([A-Za-z0-9_]+)\\s+to default value"assumes a specific server message format. If the server changes this format (e.g., different wording, localization, or structured JSON responses), the pattern won't match and reset keys won't be removed frommodifiedJsonMap, causing stale edits to persist silently.Consider one of these alternatives:
- Parse structured data from the server response if available (e.g., JSON with reset IDs)
- Track the IDs being reset before sending the command and use those IDs directly in
notifyRequestServed- Add logging when no matches are found to aid debugging
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java` around lines 759 - 765, The Pattern/Matcher block that extracts reset keys from serverMessage is fragile; instead obtain reset IDs from structured server responses or your request-tracking context and use those to remove entries from modifiedJsonMap. Specifically, update the code around serverMessage handling to first try parsing a JSON field (e.g., "reset_ids" or similar) and remove each id from modifiedJsonMap, and if the server response has no structured data fall back to using the IDs you tracked when sending the reset command (the request context that calls notifyRequestServed). Also add a debug log when neither structured IDs nor regex matches are found so failures are visible; touch the Pattern/Matcher code only as a fallback, not the primary extraction method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java`:
- Around line 831-843: getCurrentBuildFolder() currently calls
IDFUtil.getBuildDir(project) but never assigns its result, so it always returns
StringUtil.EMPTY and causes rollbackBuildFolder(buildFolder) to restore an empty
path; fix by assigning the method result to buildFolder (e.g., buildFolder =
IDFUtil.getBuildDir(project)), keep the existing CoreException catch and
Logger.log(e), and ensure the method returns the assigned buildFolder
(optionally guard against null from IDFUtil.getBuildDir to return
StringUtil.EMPTY if needed).
---
Nitpick comments:
In
`@bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java`:
- Around line 759-765: The Pattern/Matcher block that extracts reset keys from
serverMessage is fragile; instead obtain reset IDs from structured server
responses or your request-tracking context and use those to remove entries from
modifiedJsonMap. Specifically, update the code around serverMessage handling to
first try parsing a JSON field (e.g., "reset_ids" or similar) and remove each id
from modifiedJsonMap, and if the server response has no structured data fall
back to using the IDs you tracked when sending the reset command (the request
context that calls notifyRequestServed). Also add a debug log when neither
structured IDs nor regex matches are found so failures are visible; touch the
Pattern/Matcher code only as a fallback, not the primary extraction method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b612c503-f4ce-4241-80ee-bc9af0c4eaef
📒 Files selected for processing (1)
bundles/com.espressif.idf.sdk.config.ui/src/com/espressif/idf/sdk/config/ui/SDKConfigurationEditor.java
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
1 participant
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
Please include a summary of the change and which issue is fixed.
Fixes # (IEP-XXX)
Type of change
Please delete options that are not relevant.
How has this been tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
New Features
Bug Fixes