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:
📝 WalkthroughWalkthroughAlways prefer an environment-derived ESP‑IDF setup (extension custom vars or process env), validate filesystem and tooling presence (IDF, tools, git, esptool), persist the chosen setup to workspace config, simplify tool-info APIs and tooling checks, add contextual access labels, allow skipping esptool per-port probes, adjust new‑project flow, update docs/tests, and remove unused classes/files. Changes
Sequence Diagram(s)sequenceDiagram
participant Env as Env / idf.customExtraVars
participant Ext as Extension (loadIdfSetup)
participant FS as Filesystem
participant Tools as IdfToolsManager
participant WS as Workspace config
Ext->>Env: read customExtraVars / process.env
Ext->>FS: pathExists(IDF_PATH), pathExists(IDF_TOOLS_PATH), isBinInPath("git")
alt env-defined paths exist and valid
Ext->>Tools: getRequiredToolsInfo(pathToTools, ["cmake","ninja"])
Tools-->>Ext: tools env + versions
Ext->>WS: write idf.currentSetup (env-derived)
Ext-->>Env: return env-defined setup
else fallback to workspace setups
Ext->>WS: read idf.setups list
Ext->>FS: pathExists(setup.idfPath) for configured currentSetup
alt found matching setup
Ext->>WS: write idf.currentSetup (selected)
Ext-->>Env: return workspace setup
else
Ext-->>Env: notify "Current ESP-IDF setup is not found" or "No ESP-IDF Setups found"
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and user experience of the ESP-IDF extension by focusing on more reliable environment setup, optimized serial port detection, and clearer error diagnostics. The changes aim to streamline the extension's operation, particularly for non-EIM setups and the New Project Wizard, by ensuring that necessary paths and tools are correctly identified and validated, leading to a more stable and predictable development environment. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces several improvements to the ESP-IDF extension's setup and tool management. Key changes include enhancing error handling for Git version retrieval, optimizing serial port listing by allowing esptool calls to be skipped for performance, and making IDF setup loading more robust by validating paths with pathExists. The IdfToolsManager was refactored to consolidate tool information retrieval, reducing code duplication. Environment variable handling was improved by prioritizing user-defined custom variables and dynamically detecting the Git executable path. Additionally, the new project wizard's initialization flow was optimized by deferring serial port and board list loading until after IDF setup selection, and pathExists checks were added before executing idf_tools.py to prevent errors from missing paths. Error reporting for ESP-IDF requirements checks was also made more detailed.
| try { | ||
| const rawGitVersion = await execChildProcess( | ||
| reportedResult.configurationSettings.gitPath, | ||
| ["--version"], | ||
| context.extensionPath | ||
| ); | ||
| } else { | ||
| reportedResult.gitVersion.result = "Not found"; | ||
| reportedResult.gitVersion.output = rawGitVersion; | ||
| const versionMatches = rawGitVersion.match(GIT_VERSION_REGEX); | ||
| if (versionMatches && versionMatches.length) { | ||
| reportedResult.gitVersion.result = versionMatches[0].replace( | ||
| /git\sversion\s/g, | ||
| "" | ||
| ); | ||
| } else { | ||
| reportedResult.gitVersion.result = "Not found"; | ||
| } | ||
| } catch (error) { | ||
| reportedResult.gitVersion.output = | ||
| error instanceof Error ? error.message : String(error); | ||
| reportedResult.gitVersion.result = "Error: " + reportedResult.gitVersion.output; | ||
| reportedResult.latestError = error; | ||
| } |
There was a problem hiding this comment.
Wrapping the execChildProcess call in a try...catch block for getGitVersion is a critical improvement. It ensures that failures in retrieving the Git version are gracefully handled, and the reportedResult is populated with detailed error information, enhancing the overall robustness and diagnostic capabilities of the extension.
| public async getListArray( | ||
| workspaceFolder: vscode.Uri, | ||
| skipEsptoolCall: boolean = false | ||
| ) { | ||
| return await this.list(workspaceFolder, skipEsptoolCall); |
There was a problem hiding this comment.
Adding skipEsptoolCall as a parameter to getListArray and list methods is a significant improvement. This allows callers, like the New Project Wizard, to bypass the potentially slow esptool call when chip ID detection is not immediately necessary, improving performance and user experience.
| let existingIdfSetups = await Promise.all( | ||
| idfSetups.map(async (setup) => { | ||
| return (await pathExists(setup.idfPath)) ? setup : null; | ||
| }) | ||
| ).then((results) => results.filter((setup) => setup !== null)); |
There was a problem hiding this comment.
Filtering idfSetups with pathExists to create existingIdfSetups before passing them to getNewProjectArgs is a good robustness improvement. It ensures that only valid and accessible IDF paths are considered, preventing potential errors from stale or incorrect configurations.
src/eim/loadIdfSetup.ts
Outdated
| const gitPath = "/usr/bin/git"; | ||
| const idfSetupId = getIdfMd5sum(idfPath); | ||
| const idfVersion = await getEspIdfFromCMake(idfPath); | ||
| const gitPath = await isBinInPath("git", process.env); |
There was a problem hiding this comment.
Changing gitPath = "/usr/bin/git" to gitPath = await isBinInPath("git", process.env) makes the Git executable detection more dynamic and robust. This avoids hardcoding a path that might not be valid across different operating systems or user installations, improving portability and reliability.
src/eim/loadIdfSetup.ts
Outdated
| if (customVars["IDF_PYTHON_ENV_PATH"] || process.env.IDF_PYTHON_ENV_PATH) { | ||
| venvPythonPath = join( | ||
| process.env.IDF_PYTHON_ENV_PATH || customVars["IDF_PYTHON_ENV_PATH"], | ||
| customVars["IDF_PYTHON_ENV_PATH"] || process.env.IDF_PYTHON_ENV_PATH, |
There was a problem hiding this comment.
Similar to IDF_PATH, prioritizing customVars["IDF_PYTHON_ENV_PATH"] for the Python virtual environment path is a good improvement. It ensures consistency in respecting user-defined custom variables.
src/idfToolsManager.ts
Outdated
| pathToVerify?: string, | ||
| onReqPkgs?: string[], | ||
| logToChannel: boolean = true | ||
| ) { | ||
| let versions: { [key: string]: string } = {}; | ||
| if (pathToVerify) { | ||
| versions = await this.verifyPackages( | ||
| pathToVerify, | ||
| onReqPkgs, | ||
| logToChannel | ||
| ); | ||
| } | ||
| const packages = await this.getPackageList(onReqPkgs); | ||
| const idfToolsList = packages.map((pkg) => { | ||
| const pkgVersionsForPlatform = pkg.versions.filter((version) => { | ||
| return ( | ||
| Object.getOwnPropertyNames(version).indexOf( | ||
| this.platformInfo.platformToUse | ||
| ) > -1 || | ||
| Object.getOwnPropertyNames(version).indexOf( | ||
| this.platformInfo.fallbackPlatform | ||
| ) > -1 | ||
| ); | ||
| }); | ||
| const expectedVersions = pkgVersionsForPlatform.map((p) => p.name); | ||
| let isToolVersionCorrect = | ||
| expectedVersions.indexOf(versions[pkg.name]) > -1 || | ||
| (versions[pkg.name] && versions[pkg.name] === "No command version"); | ||
| const versionToUse = this.getVersionToUse(pkg); | ||
| let pkgExportedPath: string = ""; | ||
| let pkgVars = pkg.export_vars; | ||
| if (basePath) { | ||
| pkgExportedPath = pkg.binaries | ||
| ? path.join(basePath, pkg.name, versionToUse, ...pkg.binaries) | ||
| : path.join(basePath, pkg.name, versionToUse); | ||
| pkgVars = this.exportVarsForPkg(pkg, basePath); | ||
| } | ||
| return { | ||
| actual: versions[pkg.name] || "", | ||
| description: pkg.description, | ||
| doesToolExist: isToolVersionCorrect, | ||
| env: pkgVars, | ||
| expected: expectedVersions.join(","), | ||
| name: pkg.name, | ||
| path: pkgExportedPath, | ||
| } as IEspIdfTool; | ||
| }); | ||
| return idfToolsList; | ||
| } | ||
|
|
||
| public async getEIMToolsInfo( | ||
| pathToVerify: string, | ||
| onReqPkgs?: string[], | ||
| logToChannel: boolean = true | ||
| ) { | ||
| let versions: { [key: string]: string } = await this.verifyPackages( | ||
| const versions = await this.verifyPackages( | ||
| pathToVerify, | ||
| onReqPkgs, | ||
| logToChannel | ||
| ); |
There was a problem hiding this comment.
The consolidation of getRequiredToolsInfo and getEIMToolsInfo into a single getRequiredToolsInfo method is a significant refactoring improvement. It simplifies the API, reduces code duplication, and makes the tool verification logic more consistent.
| progress.report({ increment: 10, message: "Loading serial ports..." }); | ||
| let serialPortList: Array<string> = ["detect"]; | ||
| try { | ||
| const serialPortListDetails = await SerialPort.shared().getListArray( | ||
| workspace, | ||
| true | ||
| ); | ||
| serialPortList.push(...serialPortListDetails.map((p) => p.comName)); | ||
| } catch (error) { | ||
| const msg = error.message | ||
| ? error.message | ||
| : "Error looking for serial ports."; | ||
| Logger.infoNotify(msg); | ||
| Logger.error(msg, error, "getNewProjectArgs getSerialPort"); | ||
| serialPortList = ["no port"]; | ||
| } | ||
| progress.report({ increment: 10, message: "Loading ESP-IDF Boards list..." }); | ||
| const openOcdScriptsPath = await getOpenOcdScripts(workspace); | ||
| let espBoards = await getBoards(openOcdScriptsPath); |
There was a problem hiding this comment.
Moving the serial port and board list loading logic to after the IDF setup selection is a logical improvement. These operations might depend on the chosen IDF setup, ensuring that the correct environment is used. Additionally, calling getListArray with true for skipEsptoolCall is a key performance optimization for the New Project Wizard.
| const doesIdfPathExist = await pathExists(espIdfPath); | ||
| if (!doesIdfPathExist) { | ||
| return idfToolsDict; | ||
| } | ||
| const doesIdfToolsPyExist = await pathExists(idfToolsPyPath); | ||
| if (!doesIdfToolsPyExist) { | ||
| return idfToolsDict; | ||
| } |
There was a problem hiding this comment.
Adding pathExists checks for espIdfPath and idfToolsPyPath before attempting to execute idf_tools.py significantly improves the robustness of getEnvVarsFromIdfTools. This prevents errors from being thrown if these critical paths are missing, leading to more graceful failure handling.
| reportedResult.idfCheckRequirements.output = | ||
| error instanceof Error ? error.message : String(error); | ||
| reportedResult.idfCheckRequirements.result = "Error: " + reportedResult.idfCheckRequirements.output; |
There was a problem hiding this comment.
Capturing the actual error message and including it in both output and result fields provides more detailed and actionable information when checkEspIdfRequirements fails. This significantly improves error reporting.
| await writeParameter( | ||
| "idf.currentSetup", | ||
| idfSetupToUse.idfPath, | ||
| ConfigurationTarget.WorkspaceFolder, | ||
| workspaceFolder | ||
| ); |
There was a problem hiding this comment.
Moving the writeParameter("idf.currentSetup", ...) call to this block ensures that the idf.currentSetup is consistently updated after idfSetupToUse has been determined, regardless of whether it was found from existing setups or defaulted. This improves the reliability of the configuration.
There was a problem hiding this comment.
Pull request overview
This PR improves extension activation and “New Project” wizard behavior for non-EIM / legacy ESP-IDF setups by making IDF setup selection more robust, reducing reliance on esptool.py for serial-port listing, and simplifying tool verification/error reporting.
Changes:
- Skip
esptool.py-based chip-id probing when listing serial ports for the New Project wizard to avoid failures in legacy/non-EIM setups. - Refactor/streamline tool verification (
IdfToolsManager.getRequiredToolsInfo) and improve error reporting in support diagnostics. - Add additional validation of IDF setups/paths during activation and setup loading.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/views/nvs-partition-table/App.vue | Ensures first row deleteability check is numeric-safe. |
| src/utils.ts | Extends canAccessFile logging details; updates isBinInPath typing/docs. |
| src/support/gitVersion.ts | Wraps git version detection in try/catch and records error details. |
| src/support/configurationAccess.ts | Passes context labels into canAccessFile for better diagnostics. |
| src/support/checkEspIdfTools.ts | Updates to new getRequiredToolsInfo signature. |
| src/support/checkEspIdfRequirements.ts | Improves requirements-path error message and captures error output. |
| src/pythonManager.ts | Adds existence checks for IDF_PATH/idf_tools.py before exporting vars. |
| src/newProject/newProjectInit.ts | Moves serial-port loading later and skips esptool-based probing for wizard. |
| src/idfToolsManager.ts | Consolidates tool-info methods into a single getRequiredToolsInfo. |
| src/flash/flashTask.ts | Improves file-access logging context for flash script and section binaries. |
| src/flash/eraseFlashTask.ts | Improves file-access logging context for esptool.py. |
| src/extension.ts | Filters out invalid IDF setups (missing idfPath) before launching wizard. |
| src/espIdf/serial/serialPort.ts | Adds skipEsptoolCall option to avoid esptool-based chip-id requests. |
| src/eim/verifySetup.ts | Switches to consolidated getRequiredToolsInfo for EIM/non-EIM paths. |
| src/eim/loadSettings.ts | Removes unused import cleanup. |
| src/eim/loadIdfSetup.ts | Adjusts setup selection and env-var based setup creation (IDF_PATH priority, git path lookup). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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 (2)
src/utils.ts (1)
1079-1083:⚠️ Potential issue | 🟠 MajorKeep scanning PATH when the first hit is outside
containerDir.Returning here drops the rest of the PATH search. If a system binary appears before the ESP-IDF-managed one, the function reports “not found” even when a later entry matches the requested container.
🔧 Suggested fix
if (containerDir && containerDir.length) { const resultContainerPath = containerDir.join(path.sep); if (binaryPath.indexOf(resultContainerPath) === -1) { - return ""; + continue; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils.ts` around lines 1079 - 1083, The current check inside the PATH scan returns "" as soon as a found binaryPath does not include the resultContainerPath (containerDir.join(path.sep)), which aborts further PATH entries and can miss a later valid container-managed binary; change the logic so that when binaryPath.indexOf(resultContainerPath) === -1 you skip this candidate and continue scanning the rest of PATH (e.g., replace the early return with continuing the loop or otherwise only return "" after the entire PATH has been exhausted), keeping the use of containerDir, resultContainerPath and binaryPath to locate valid matches.src/idfToolsManager.ts (1)
26-34:⚠️ Potential issue | 🟠 MajorRemove the type cast and either populate
pathor remove it from the interface.The mapper omits the required
path: stringproperty but casts toIEspIdfTool, hiding a contract break. Theenvis assigned rawpkg.export_vars, which contains unresolved${TOOL_PATH}placeholders. While current consumers inverifySetup.tsonly readdoesToolExistandname, any code attempting to accesstool.pathwill getundefinedat runtime. Additionally,reportedResult.espIdfToolsVersionsexpectsidfToolResult[](4 props:actual,doesToolExist,expected,name), but this function returns 6-property objects includingdescriptionandenv, creating a hidden type mismatch.Either:
- Include
pathin the mapper (callingexportVarsForPkg()to resolve placeholders if needed), or- Update
IEspIdfToolandidfToolResultto match what the function actually returns and remove the cast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/idfToolsManager.ts` around lines 26 - 34, The mapper is casting package objects to IEspIdfTool but omits path and leaves env with unresolved placeholders, causing a contract break with IEspIdfTool and idfToolResult; either populate path and resolve env by calling exportVarsForPkg() inside the mapper so each returned object includes path: string and a resolved env, or change the types (IEspIdfTool and idfToolResult) to match the actual returned shape and remove the unsafe cast; update references such as reportedResult.espIdfToolsVersions and verifySetup.ts to use the corrected shape.
🧹 Nitpick comments (3)
src/views/nvs-partition-table/App.vue (1)
80-80: Unnecessary type conversion and unrelated to PR objectives.The
Number(i)wrapper is redundant becauseifrom Vue'sv-forarray iteration is already a numeric index. The originali !== 0was correct and type-safe. Additionally, this NVS partition table UI change appears unrelated to the PR's stated objectives (fixing path resolution and EIM setup issues). Consider reverting this change to keep the PR focused.♻️ Revert to original expression
- :canDeleteRow="Number(i) !== 0" + :canDeleteRow="i !== 0"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/nvs-partition-table/App.vue` at line 80, The prop binding :canDeleteRow was changed to use Number(i) but the v-for index i is already a number; revert the expression to the original i !== 0 to remove the unnecessary type conversion. Update the template where :canDeleteRow is set (the binding using the loop variable i) to use i !== 0 so the UI logic remains unchanged and unrelated PR scope stays focused.src/espIdf/serial/serialPort.ts (1)
335-363: Prefer a named option overskipEsptoolCall.Threading a bare boolean through
getListArray()andlist()already produces call sites likegetListArray(workspace, true), which are hard to read and easy to invert later. An options object or a dedicated helper would make the new behavior self-documenting.As per coding guidelines "Prefer self-documenting code (names + small helpers) over comments; write comments for intent/why/constraints/edge cases instead of narrating what the code already says".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/espIdf/serial/serialPort.ts` around lines 335 - 363, Replace the bare boolean flag skipEsptoolCall on getListArray and list with a small options object (e.g., { skipEsptoolCall?: boolean } or more explicit { useEsptool?: boolean }) so callers use named properties instead of positional booleans; update the signatures of getListArray(workspaceFolder, options) and list(workspaceFolder, options) and all call sites that call getListArray(..., true/false) to pass a descriptive object (e.g., getListArray(workspace, { skipEsptoolCall: true })) and adjust any internal uses of skipEsptoolCall inside list to read options.skipEsptoolCall.src/support/gitVersion.ts (1)
44-49: Consider usingLogger.errorfor caught errors.Per coding guidelines, caught errors should use
Logger.error(message, error, "scope identifier"). While the error is captured inreportedResult.latestError, adding explicit logging would improve observability.Suggested addition
} catch (error) { + Logger.error("Failed to get git version", error, "support gitVersion"); reportedResult.gitVersion.output = error instanceof Error ? error.message : String(error); reportedResult.gitVersion.result = "Error: " + reportedResult.gitVersion.output; reportedResult.latestError = error; }Note: This would require importing
Loggerfrom"../logger/logger".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/support/gitVersion.ts` around lines 44 - 49, The catch block currently sets reportedResult.gitVersion.output/result and reportedResult.latestError but doesn't log the error; import Logger from "../logger/logger" and call Logger.error with a clear message, the caught error, and a scope identifier (e.g., "gitVersion:getGitVersion" or similar) inside the catch so that errors for reportedResult.gitVersion and reportedResult.latestError are recorded via Logger.error(errorMessage, error, "scope").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/eim/loadIdfSetup.ts`:
- Around line 46-57: loadIdfSetup can return undefined (early returns and failed
find), but callers assume a non-null value and access properties directly;
update loadIdfSetup (symbols: loadIdfSetup, idfSetupToUse, idfSetups,
idfConfigurationName) to guarantee a valid return or fail fast: after computing
idfSetupToUse, if it's undefined pick a safe default (e.g., idfSetups[0]) or
throw a descriptive Error; alternatively, keep the current nullable return but
update all callers that dereference its result to check for undefined and handle
cancellation/invalid-setup cases before accessing properties. Ensure the find()
result is explicitly handled so callers at extension call sites never
dereference an undefined value.
In `@src/extension.ts`:
- Around line 2345-2349: After filtering idfSetups into existingIdfSetups (in
the block that calls pathExists), add a guard that checks if existingIdfSetups
is empty and handle it before calling getNewProjectArgs or opening the wizard:
either show a user-facing message (e.g., via showInformationMessage or
showErrorMessage) explaining no valid frameworks were found, or fall back to
idfSetups/unfiltered options if that makes sense. Update both occurrences (the
block creating existingIdfSetups and the similar block around lines 2355-2358)
to avoid calling getNewProjectArgs with an empty list and to return/abort the
flow cleanly when no valid setups exist.
---
Outside diff comments:
In `@src/idfToolsManager.ts`:
- Around line 26-34: The mapper is casting package objects to IEspIdfTool but
omits path and leaves env with unresolved placeholders, causing a contract break
with IEspIdfTool and idfToolResult; either populate path and resolve env by
calling exportVarsForPkg() inside the mapper so each returned object includes
path: string and a resolved env, or change the types (IEspIdfTool and
idfToolResult) to match the actual returned shape and remove the unsafe cast;
update references such as reportedResult.espIdfToolsVersions and verifySetup.ts
to use the corrected shape.
In `@src/utils.ts`:
- Around line 1079-1083: The current check inside the PATH scan returns "" as
soon as a found binaryPath does not include the resultContainerPath
(containerDir.join(path.sep)), which aborts further PATH entries and can miss a
later valid container-managed binary; change the logic so that when
binaryPath.indexOf(resultContainerPath) === -1 you skip this candidate and
continue scanning the rest of PATH (e.g., replace the early return with
continuing the loop or otherwise only return "" after the entire PATH has been
exhausted), keeping the use of containerDir, resultContainerPath and binaryPath
to locate valid matches.
---
Nitpick comments:
In `@src/espIdf/serial/serialPort.ts`:
- Around line 335-363: Replace the bare boolean flag skipEsptoolCall on
getListArray and list with a small options object (e.g., { skipEsptoolCall?:
boolean } or more explicit { useEsptool?: boolean }) so callers use named
properties instead of positional booleans; update the signatures of
getListArray(workspaceFolder, options) and list(workspaceFolder, options) and
all call sites that call getListArray(..., true/false) to pass a descriptive
object (e.g., getListArray(workspace, { skipEsptoolCall: true })) and adjust any
internal uses of skipEsptoolCall inside list to read options.skipEsptoolCall.
In `@src/support/gitVersion.ts`:
- Around line 44-49: The catch block currently sets
reportedResult.gitVersion.output/result and reportedResult.latestError but
doesn't log the error; import Logger from "../logger/logger" and call
Logger.error with a clear message, the caught error, and a scope identifier
(e.g., "gitVersion:getGitVersion" or similar) inside the catch so that errors
for reportedResult.gitVersion and reportedResult.latestError are recorded via
Logger.error(errorMessage, error, "scope").
In `@src/views/nvs-partition-table/App.vue`:
- Line 80: The prop binding :canDeleteRow was changed to use Number(i) but the
v-for index i is already a number; revert the expression to the original i !== 0
to remove the unnecessary type conversion. Update the template where
:canDeleteRow is set (the binding using the loop variable i) to use i !== 0 so
the UI logic remains unchanged and unrelated PR scope stays focused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87277780-922f-4a3d-a70a-acad43478098
📒 Files selected for processing (16)
src/eim/loadIdfSetup.tssrc/eim/loadSettings.tssrc/eim/verifySetup.tssrc/espIdf/serial/serialPort.tssrc/extension.tssrc/flash/eraseFlashTask.tssrc/flash/flashTask.tssrc/idfToolsManager.tssrc/newProject/newProjectInit.tssrc/pythonManager.tssrc/support/checkEspIdfRequirements.tssrc/support/checkEspIdfTools.tssrc/support/configurationAccess.tssrc/support/gitVersion.tssrc/utils.tssrc/views/nvs-partition-table/App.vue
💤 Files with no reviewable changes (2)
- src/eim/loadSettings.ts
- src/support/checkEspIdfTools.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
docs_espressif/zh_CN/installation.rst (1)
75-75: Minor: Missing blank line before list item continuation.In reStructuredText, when continuing content after a code block within a numbered list, a blank line is typically needed for proper rendering.
} + 3. 或者，也可使用 PATH（Windows 下为 Path）列出要追加到 PATH 的工具路径：🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs_espressif/zh_CN/installation.rst` at line 75, Add a blank line before the continuation of the numbered list item that begins with "3. 或者，也可使用 PATH（Windows 下为 Path）列出要追加到 PATH 的工具路径：" so the reStructuredText list renders correctly after the preceding code block; locate that exact list item text and insert a single empty line before its content continuation.src/support/checkIdfSetups.ts (1)
36-40: Useconstinstead ofletforenvVars.The variable is not reassigned after initialization.
- let envVars: { [key: string]: string } = await getEnvVariables(idfSetup); + const envVars: { [key: string]: string } = await getEnvVariables(idfSetup);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/support/checkIdfSetups.ts` around lines 36 - 40, Change the mutable declaration of envVars to an immutable one: replace the current "let envVars" with "const envVars" where getEnvVariables(idfSetup) is awaited (in the block that then calls isIdfSetupValid), since envVars is never reassigned; update the declaration near the calls to getEnvVariables and isIdfSetupValid and keep the rest (extendedIdfSetup.isValid / .reason assignment) unchanged.src/eim/loadIdfSetup.ts (1)
203-214: RedundantisValidcheck.The
if (isValid)check on line 203 is redundant since the function already returns at line 163 when!isValid. The code inside this block will always execute when this point is reached.♻️ Suggested simplification
- if (isValid) { - ESP.ProjectConfiguration.store.set( - ESP.ProjectConfiguration.CURRENT_IDF_CONFIGURATION, - customVars - ); - await writeParameter( - "idf.currentSetup", - envDefinedIdfSetup.idfPath, - ConfigurationTarget.WorkspaceFolder, - workspaceFolder - ); - return envDefinedIdfSetup; - } + ESP.ProjectConfiguration.store.set( + ESP.ProjectConfiguration.CURRENT_IDF_CONFIGURATION, + customVars + ); + await writeParameter( + "idf.currentSetup", + envDefinedIdfSetup.idfPath, + ConfigurationTarget.WorkspaceFolder, + workspaceFolder + ); + return envDefinedIdfSetup;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/eim/loadIdfSetup.ts` around lines 203 - 214, The check "if (isValid)" around the block that stores CURRENT_IDF_CONFIGURATION, writes "idf.currentSetup" and returns envDefinedIdfSetup is redundant because the function already returns when !isValid earlier; remove the surrounding if (isValid) { ... } and unindent the inner statements so they always execute at that point, keeping the same calls to ESP.ProjectConfiguration.store.set, writeParameter("idf.currentSetup", envDefinedIdfSetup.idfPath, ConfigurationTarget.WorkspaceFolder, workspaceFolder), and the return envDefinedIdfSetup to preserve behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/eim/loadIdfSetup.ts`:
- Around line 138-149: The current case-insensitive lookup using pathNameInEnv =
Object.keys(process.env).find(k => k.toUpperCase() == "PATH") can yield
undefined or return a differently cased key and then use undefined as an index
into customVars; update the lookup to first attempt to find an exact "PATH" key,
otherwise find a case-insensitive match and if none exists default to the
canonical "PATH" string, then use that resolved key (e.g., pathKey) when reading
process.env and when indexing customVars; apply this change in the block around
pathNameInEnv, customVars and the call to
IdfToolsManager.createIdfToolsManager/exportPathsInString so you never use
undefined as a key.
- Around line 118-122: The code computes idfPath = customVars["IDF_PATH"] ||
process.env.IDF_PATH then calls pathExists(idfPath) without checking for
undefined; add a guard before calling pathExists to handle a missing IDF_PATH
(e.g., if idfPath is falsy, log/return/throw as appropriate) so pathExists is
only invoked with a valid string. Update the block around idfPath in
loadIdfSetup.ts to check idfPath (referencing the idfPath variable and
pathExists function) and exit early or provide a clear error when IDF_PATH is
undefined.
---
Nitpick comments:
In `@docs_espressif/zh_CN/installation.rst`:
- Line 75: Add a blank line before the continuation of the numbered list item
that begins with "3. 或者，也可使用 PATH（Windows 下为 Path）列出要追加到 PATH 的工具路径：" so the
reStructuredText list renders correctly after the preceding code block; locate
that exact list item text and insert a single empty line before its content
continuation.
In `@src/eim/loadIdfSetup.ts`:
- Around line 203-214: The check "if (isValid)" around the block that stores
CURRENT_IDF_CONFIGURATION, writes "idf.currentSetup" and returns
envDefinedIdfSetup is redundant because the function already returns when
!isValid earlier; remove the surrounding if (isValid) { ... } and unindent the
inner statements so they always execute at that point, keeping the same calls to
ESP.ProjectConfiguration.store.set, writeParameter("idf.currentSetup",
envDefinedIdfSetup.idfPath, ConfigurationTarget.WorkspaceFolder,
workspaceFolder), and the return envDefinedIdfSetup to preserve behavior.
In `@src/support/checkIdfSetups.ts`:
- Around line 36-40: Change the mutable declaration of envVars to an immutable
one: replace the current "let envVars" with "const envVars" where
getEnvVariables(idfSetup) is awaited (in the block that then calls
isIdfSetupValid), since envVars is never reassigned; update the declaration near
the calls to getEnvVariables and isIdfSetupValid and keep the rest
(extendedIdfSetup.isValid / .reason assignment) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dcd7bb2b-3c7c-46d6-afc3-9aab47fb838e
📒 Files selected for processing (9)
docs_espressif/en/installation.rstdocs_espressif/zh_CN/installation.rstsrc/PyReqLog.tssrc/eim/loadIdfSetup.tssrc/eim/migrationTool.tssrc/eim/verifySetup.tssrc/packageWebError.tssrc/pythonManager.tssrc/support/checkIdfSetups.ts
💤 Files with no reviewable changes (2)
- src/PyReqLog.ts
- src/packageWebError.ts
✅ Files skipped from review due to trivial changes (1)
- docs_espressif/en/installation.rst
🚧 Files skipped from review as they are similar to previous changes (2)
- src/eim/verifySetup.ts
- src/pythonManager.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/support/checkEspIdfRequirements.ts (1)
54-60: LGTM - Robust error handling with proper type narrowing.The
instanceof Errorcheck correctly extracts the message, with aString(error)fallback for non-Error values. The "Error: " prefix in the result provides better context in diagnostics.Consider adding a
Logger.errorcall for additional visibility during debugging, per coding guidelines:Logger.error(reportedResult.idfCheckRequirements.output, error, "checkEspIdfRequirements");This is optional since the error is already captured in
reportedResult.latestErrorfor the diagnostic report.
,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/support/checkEspIdfRequirements.ts` around lines 54 - 60, Add a Logger.error call inside the catch block of the checkEspIdfRequirements function to log the extracted error message and the original error for visibility: after setting reportedResult.idfCheckRequirements.output and reportedResult.latestError, call Logger.error with the output message, the caught error object, and the string "checkEspIdfRequirements" so logs include context; reference the reportedResult.idfCheckRequirements, reportedResult.latestError, and the catch error variable when adding the call.src/extension.ts (1)
2345-2356: Good fix for the empty setups issue; minor improvements possible.The validation and early-return guard address the prior concern about the wizard receiving an empty list. A couple of refinements:
- The
!existingIdfSetupscheck is redundant sincePromise.all(...).then(...)always returns an array.- Consider using
Logger.warnNotifyfor consistency with similar warnings elsewhere, and provide a more descriptive message explaining the paths were invalid.💡 Suggested refinement
let existingIdfSetups = await Promise.all( idfSetups.map(async (setup) => { return (await pathExists(setup.idfPath)) ? setup : null; }) ).then((results) => results.filter((setup) => setup !== null)); - if (!existingIdfSetups || existingIdfSetups.length === 0) { - vscode.window.showInformationMessage( - vscode.l10n.t("No ESP-IDF Setups found") + if (existingIdfSetups.length === 0) { + Logger.warnNotify( + vscode.l10n.t( + "No valid ESP-IDF setups were found. Check the configured ESP-IDF paths and try again." + ) ); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/extension.ts` around lines 2345 - 2356, The check for !existingIdfSetups is redundant because existingIdfSetups (result of Promise.all(...).then(...)) is always an array; remove that nullity check and only test length, and replace the vscode.window.showInformationMessage call with the project's standard Logger.warnNotify to keep messaging consistent (use a more descriptive message like "No valid ESP-IDF setups found; all configured paths are invalid" when existingIdfSetups.length === 0). Locate the logic around idfSetups, pathExists, and existingIdfSetups in the extension activation flow and update the early-return branch to use Logger.warnNotify instead of vscode.window.showInformationMessage and drop the redundant null check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/extension.ts`:
- Around line 2345-2356: The check for !existingIdfSetups is redundant because
existingIdfSetups (result of Promise.all(...).then(...)) is always an array;
remove that nullity check and only test length, and replace the
vscode.window.showInformationMessage call with the project's standard
Logger.warnNotify to keep messaging consistent (use a more descriptive message
like "No valid ESP-IDF setups found; all configured paths are invalid" when
existingIdfSetups.length === 0). Locate the logic around idfSetups, pathExists,
and existingIdfSetups in the extension activation flow and update the
early-return branch to use Logger.warnNotify instead of
vscode.window.showInformationMessage and drop the redundant null check.
In `@src/support/checkEspIdfRequirements.ts`:
- Around line 54-60: Add a Logger.error call inside the catch block of the
checkEspIdfRequirements function to log the extracted error message and the
original error for visibility: after setting
reportedResult.idfCheckRequirements.output and reportedResult.latestError, call
Logger.error with the output message, the caught error object, and the string
"checkEspIdfRequirements" so logs include context; reference the
reportedResult.idfCheckRequirements, reportedResult.latestError, and the catch
error variable when adding the call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 035019ee-d5e3-490a-819b-f99b6be5c168
📒 Files selected for processing (8)
docs_espressif/zh_CN/installation.rstsrc/eim/loadIdfSetup.tssrc/eim/verifySetup.tssrc/extension.tssrc/idfToolsManager.tssrc/support/checkEspIdfRequirements.tssrc/support/gitVersion.tssrc/test/doctor.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/test/doctor.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/support/gitVersion.ts
- src/eim/verifySetup.ts
- docs_espressif/zh_CN/installation.rst
- src/eim/loadIdfSetup.ts
|
@brianignacio5 hi ! Tested under: I was able to verify that an external manual ESP-IDF v5.2.1 setup works in an existing workspace when configured explicitly. However, Observed behavior:
settings.json: and does not contain: So the wizard did persist the selected setup partially, but not enough for external/manual setup to be usable in the new workspace. Doctor Command output file: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/espIdf/serial/serialPort.ts (2)
455-464: Redundant existence checks; prefer a singlestatcall.
pathExistsfollowed byvscode.workspace.fs.statperforms two filesystem round-trips. A singlestatcall suffices—catchFileNotFounderrors and checkstat.type.Also, minor grammar fix: "does not exists" → "does not exist".
Proposed simplification
- const esptoolPathExists = await pathExists(esptoolPath); - if (!esptoolPathExists) { - throw new Error(`esptool.py does not exists in ${esptoolPath}`); - } - const stat = await vscode.workspace.fs.stat( - vscode.Uri.file(esptoolPath) - ); - if (stat.type !== vscode.FileType.File) { - // esptool.py does not exists - throw new Error(`esptool.py in ${esptoolPath} is not a file`); - } + let stat: vscode.FileStat; + try { + stat = await vscode.workspace.fs.stat(vscode.Uri.file(esptoolPath)); + } catch { + throw new Error(`esptool.py does not exist at ${esptoolPath}`); + } + if (stat.type !== vscode.FileType.File) { + throw new Error(`esptool.py at ${esptoolPath} is not a file`); + }This also removes the need for the
fs-extraimport at line 32 ifpathExistsis no longer used elsewhere in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/espIdf/serial/serialPort.ts` around lines 455 - 464, Remove the redundant pathExists call and replace it with a single vscode.workspace.fs.stat wrapped in try/catch: call vscode.workspace.fs.stat(vscode.Uri.file(esptoolPath)), catch the file-not-found error and throw a new Error with corrected grammar ("esptool.py does not exist in ${esptoolPath}"), then verify stat.type === vscode.FileType.File and if not throw "esptool.py in ${esptoolPath} is not a file"; remove the now-unused pathExists/esptoolPathExists variable and drop the fs-extra import if it is no longer used elsewhere in this file.
383-387: Move Python/IDF_PATH retrieval after theskipEsptoolCallguard.
getVirtualEnvPythonPath()and the IDF_PATH lookup execute even whenskipEsptoolCallistrue. Per the PR objectives, the New Project Wizard usesskipEsptoolCall=trueprecisely because IDF may not be fully configured yet—calling these functions can fail or produce misleading errors.Consider deferring them inside the
elseblock at line 447.Suggested refactor
- const pythonBinPath = await getVirtualEnvPythonPath(); - const currentEnvVars = ESP.ProjectConfiguration.store.get<{ - [key: string]: string; - }>(ESP.ProjectConfiguration.CURRENT_IDF_CONFIGURATION, {}); - const idfPath = currentEnvVars["IDF_PATH"]; const enableSerialPortChipIdRequest = idfConf.readParameter( "idf.enableSerialPortChipIdRequest", workspaceFolder ) as boolean; ... if (!enableSerialPortChipIdRequest) { return resolve(choices); } - async function processPorts(serialPort: SerialPortDetails, esptoolPath: string) { + + if (skipEsptoolCall) { + resolve(choices); + } else { + const pythonBinPath = await getVirtualEnvPythonPath(); + const currentEnvVars = ESP.ProjectConfiguration.store.get<{ + [key: string]: string; + }>(ESP.ProjectConfiguration.CURRENT_IDF_CONFIGURATION, {}); + const idfPath = currentEnvVars["IDF_PATH"]; + + async function processPorts(serialPort: SerialPortDetails, esptoolPath: string) { ... } + const esptoolPath = join( + idfPath, ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/espIdf/serial/serialPort.ts` around lines 383 - 387, The retrieval of the virtualenv Python binary and IDF_PATH happens before the skipEsptoolCall guard; move the calls to getVirtualEnvPythonPath() and the ESP.ProjectConfiguration.store.get<CURRENT_IDF_CONFIGURATION>() lookup that sets idfPath into the branch that runs when skipEsptoolCall is false (i.e., inside the else branch that performs esptool actions) so these functions are only invoked when esptool is actually called; update any variables (pythonBinPath, idfPath) to be declared/initialized there and remove their pre-guard usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/newProject/utils.ts`:
- Around line 54-70: The code assigns properties on
settingsJson["idf.customExtraVars"] without ensuring it exists; before setting
IDF_PATH/IDF_TOOLS_PATH/IDF_PYTHON_ENV_PATH when customExtraVars is truthy,
initialize settingsJson["idf.customExtraVars"] =
settingsJson["idf.customExtraVars"] || {} (or equivalent) so subsequent
assignments in the block (references: settingsJson, customExtraVars, idfSetup)
do not attempt to write properties on undefined.
---
Nitpick comments:
In `@src/espIdf/serial/serialPort.ts`:
- Around line 455-464: Remove the redundant pathExists call and replace it with
a single vscode.workspace.fs.stat wrapped in try/catch: call
vscode.workspace.fs.stat(vscode.Uri.file(esptoolPath)), catch the file-not-found
error and throw a new Error with corrected grammar ("esptool.py does not exist
in ${esptoolPath}"), then verify stat.type === vscode.FileType.File and if not
throw "esptool.py in ${esptoolPath} is not a file"; remove the now-unused
pathExists/esptoolPathExists variable and drop the fs-extra import if it is no
longer used elsewhere in this file.
- Around line 383-387: The retrieval of the virtualenv Python binary and
IDF_PATH happens before the skipEsptoolCall guard; move the calls to
getVirtualEnvPythonPath() and the
ESP.ProjectConfiguration.store.get<CURRENT_IDF_CONFIGURATION>() lookup that sets
idfPath into the branch that runs when skipEsptoolCall is false (i.e., inside
the else branch that performs esptool actions) so these functions are only
invoked when esptool is actually called; update any variables (pythonBinPath,
idfPath) to be declared/initialized there and remove their pre-guard usage.
🪄 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: b6ed441e-d90f-4539-be9f-83b0a34ed9cf
📒 Files selected for processing (2)
src/espIdf/serial/serialPort.tssrc/newProject/utils.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
src/eim/verifySetup.ts:76
failedToolsResultwill always be empty here becausetoolsInfoonly containscmakeandninja(passed togetRequiredToolsInfo), but the filter explicitly excludes those names. This makesisIdfSetupValidincorrectly succeed even whencmake/ninjaare missing. Remove the name exclusion or broaden the package list so the filter logic matches the intended validation.
const failedToolsResult = toolsInfo.filter(
(tInfo) =>
!tInfo.doesToolExist && ["cmake", "ninja"].indexOf(tInfo.name) === -1
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (2)
src/eim/verifySetup.ts:81
- The missing-tools filter excludes
cmakeandninja(["cmake", "ninja"].indexOf(tInfo.name) === -1). SincegetRequiredToolsInfois called with only["cmake", "ninja"], this makesfailedToolsResultalways empty and allows setups to be treated as valid even when those tools are missing. The filter condition should include (not exclude) the required tools.
let toolsInfo: IEspIdfTool[] = await idfToolsManager.getRequiredToolsInfo(
envVars[pathNameInEnv],
["cmake", "ninja"],
logToChannel
);
const failedToolsResult = toolsInfo.filter(
(tInfo) =>
!tInfo.doesToolExist && ["cmake", "ninja"].indexOf(tInfo.name) === -1
);
if (failedToolsResult.length) {
const missingTools = failedToolsResult.map((t) => t.name).join(", ");
return [false, `Missing required tools: ${missingTools}`];
}
src/utils.ts:1072
isBinInPathusesObject.keys(process.env)to pick a PATH key and then assumesenv[pathNameInEnv]exists. IfpathNameInEnvis undefined (no PATH inprocess.env) or if the caller’senvuses a different casing (e.g.PATHvsPath),env[pathNameInEnv].split(...)will throw. Prefer resolving the PATH key from the providedenv(case-insensitive) and falling back to "PATH" when not found.
let pathNameInEnv: string = Object.keys(process.env).find(
(k) => k.toUpperCase() == "PATH"
);
const pathDirs = env[pathNameInEnv].split(path.delimiter);
for (const pathDir of pathDirs) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@brianignacio5 hi ! the error message is improved ✅ |
|
issue for both Mac arm64 / Windows 11 the new issue appears after latest changes: I did add Git explicitly to my but my external IDF is: Doctor comman even minimal |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/utils.ts:1072
isBinInPathderives the PATH key name fromprocess.envinstead of the providedenvobject, and then immediately doesenv[pathNameInEnv].split(...). If the passedenvuses a different PATH key casing (or is a partial env without that key), this will throw at runtime. Prefer detecting the PATH key fromObject.keys(env)(with a default of "PATH") and guard when the PATH value is missing.
let pathNameInEnv: string = Object.keys(process.env).find(
(k) => k.toUpperCase() == "PATH"
);
const pathDirs = env[pathNameInEnv].split(path.delimiter);
for (const pathDir of pathDirs) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -98,6 +85,15 @@ export async function isIdfSetupValid( | |||
| const missingTools = failedToolsResult.map((t) => t.name).join(", "); | |||
| return [false, `Missing required tools: ${missingTools}`]; | |||
There was a problem hiding this comment.
failedToolsResult currently filters out the requested tools (cmake, ninja) due to indexOf(tInfo.name) === -1, which means missing cmake/ninja will never be reported and the setup can be marked valid even when these tools are absent. Adjust the filter to include the requested tool names (or remove the name check entirely) so missing required tools are correctly detected.
| "idf.currentSetup", | ||
| workspaceFolder | ||
| ) as string; | ||
| const doesIdfSetupToUseExist = await pathExists(idfConfigurationName); |
There was a problem hiding this comment.
idfConfigurationName is read from settings and then passed directly to pathExists(...). If the setting is unset, readParameter(...) can yield undefined/empty, which can throw or produce misleading results. Guard for a falsy value before calling pathExists (and only attempt to match idfSetups when it’s a non-empty path).
| const doesIdfSetupToUseExist = await pathExists(idfConfigurationName); | |
| let doesIdfSetupToUseExist = false; | |
| if (idfConfigurationName) { | |
| doesIdfSetupToUseExist = await pathExists(idfConfigurationName); | |
| } |
| const idfToolsManager = new IdfToolsManager( | ||
| toolsJson, | ||
| platformInfo, | ||
| OutputChannel.init(), | ||
| reportedResult.configurationSettings.espIdfPath | ||
| ); | ||
| const verifiedPkgs = await idfToolsManager.getRequiredToolsInfo( | ||
| reportedResult.configurationSettings.toolsPath, | ||
| reportedResult.configurationSettings.customExtraPaths | ||
| ); |
There was a problem hiding this comment.
getRequiredToolsInfo(...) now expects a PATH-like string, but reportedResult.configurationSettings.customExtraPaths can be undefined when no CURRENT_IDF_CONFIGURATION is set (see getConfigurationSettings, which assigns it from currentEnvVars["PATH"]). Passing undefined can lead to incorrect PATH mutation (e.g., prefixing with "undefined") and false tool verification failures. Consider falling back to reportedResult.configurationSettings.systemEnvPath (or an empty string) when customExtraPaths is not set.
| @@ -111,7 +111,7 @@ suite("Doctor Command tests", () => { | |||
| test("Wrong esp-idf py requirements", async () => { | |||
| reportObj.configurationSettings.pythonBinPath = "/my/wrong/python/path"; | |||
There was a problem hiding this comment.
This test relies on reportObj.configurationSettings.espIdfPath having been set by earlier tests (it isn’t set in this test before calling checkEspIdfRequirements). That makes the assertion order-dependent and harder to maintain. Set espIdfPath explicitly in this test (and/or reset reportObj in setup) so the test is self-contained.
| reportObj.configurationSettings.pythonBinPath = "/my/wrong/python/path"; | |
| reportObj.configurationSettings.pythonBinPath = "/my/wrong/python/path"; | |
| reportObj.configurationSettings.espIdfPath = "/some/non-existing-path"; |
| if ( | ||
| pathToPigweed && | ||
| !modifiedEnv[pathNameInEnv].split(delimiter).includes(pathToPigweed) | ||
| ) { | ||
| modifiedEnv[pathNameInEnv] += delimiter + pathToPigweed; | ||
| } |
There was a problem hiding this comment.
At the Pigweed PATH injection, modifiedEnv[pathNameInEnv].split(...) is called without first ensuring the PATH value exists. If PATH is missing/empty in the environment (or was removed by earlier processing), this will throw. Add a guard (e.g., ensure modifiedEnv[pathNameInEnv] is a string before splitting/appending).
Description
Fixes for non EIM setups that rely on old ESP-IDF setups. Fix New Project Wizard so it doesn't need esptool.py to get serial ports and only rely that IDF_PATH exists.
Fixes #1794
Fixes #1795
This pull request introduces several improvements and refactors to the ESP-IDF extension, focusing on enhancing robustness, simplifying code, and improving error handling. The most important changes include stricter validation of IDF setups, improved serial port listing logic, consolidation and simplification of tool verification methods, and more informative error reporting throughout the extension.
IDF Setup and Environment Validation
src/eim/loadIdfSetup.ts,src/extension.ts,src/pythonManager.ts, [1] [2] [3]src/eim/loadIdfSetup.ts, src/eim/loadIdfSetup.tsL125-R135)Serial Port Listing Improvements
src/espIdf/serial/serialPort.ts, [1] [2] [3] [4] [5] [6]Tool Verification Refactor
getRequiredToolsInfoandgetEIMToolsInfomethods inIdfToolsManagerinto a single streamlined method, reducing code duplication and simplifying tool verification logic. (src/idfToolsManager.ts, [1] [2]src/eim/verifySetup.ts, [1]src/support/checkEspIdfTools.ts, [2]Error Handling and Reporting
src/support/checkEspIdfRequirements.ts, [1] [2] [3]src/flash/eraseFlashTask.ts, [1]src/flash/flashTask.ts, [2]Miscellaneous Refactors
src/newProject/newProjectInit.ts, [1] [2]src/eim/loadSettings.ts, src/eim/loadSettings.tsL19)These changes collectively improve reliability, maintainability, and user experience across the extension.
Type of change
Please delete options that are not relevant.
Steps to test this pull request
ESP-IDF: New Projectwizard which should allow users to select from EIM ESP-IDF setups without any opened directory.Expected behaviour:
Expected output:
How has this been tested?
Manual test
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
Bug Fixes
Refactor
Documentation
Tests