Refactor/flashing#1815
Open
brianignacio5 wants to merge 22 commits into
Open
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:
📝 WalkthroughWalkthroughRefactors build/flash/erase flows into modular transport-specific subsystems, replaces legacy task and execution wiring with a new TaskManager/addProcessTask API, moves PreCheck into common, adds workspace-folder persistence, many helpers, tests, and four new localization strings. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Extension as "Extension (command)"
participant WithProg as "withProgressWrapper"
participant Build as "buildMain"
participant TaskMgr as "TaskManager / addProcessTask"
participant Flash as "flashMain"
participant Monitor as "Monitor"
User->>Extension: invoke build/flash command
Extension->>WithProg: run pre-checks + progress
WithProg->>Build: buildMain(ws, token, flashType)
Build->>TaskMgr: enqueue compile/build/size tasks
TaskMgr-->>Build: executions/results
alt flash requested
Build->>Flash: buildFinishFlashCmd -> flashMain(...)
Flash->>TaskMgr: enqueue transport-specific flash tasks (UART/DFU/JTAG)
TaskMgr-->>Flash: executions/results
end
Flash->>Monitor: interruptMonitorWithDelay -> createNewIdfMonitor
Monitor-->>User: monitor opened / output
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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
This pull request refactors the build and flash infrastructure by introducing dedicated modules for build execution, DFU operations, and task management. It also cleans up the task execution logic by replacing redundant code with a centralized addProcessTask helper and improves error handling across several modules. I have identified a redundant code block in src/build/buildHelpers.ts that should be removed and a minor inconsistency in error string conversion in src/espIdf/setTarget/setTargetInIdf.ts that should be addressed.
brianignacio5
force-pushed
the
refactor/flashing
branch
from
12c8d01 to
9a0f6a6
Compare
|
Download the artifacts for this pull request: |
brianignacio5
force-pushed
the
refactor/flashing
branch
from
e6f2376 to
e700369
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/espIdf/setTarget/index.ts (2)
225-236:⚠️ Potential issue | 🟡 MinorNormalize caught values before logging.
err as Erroronly changes the TypeScript type; non-Errorthrowables still reach the logger withoutmessage/stack. Wrap them before passing to the logging API.Proposed fix
} catch (err) { + const error = err instanceof Error ? err : new Error(String(err)); const errMsg = - err instanceof Error + error instanceof Error ? err.message : l10n.t("Unknown error occurred while setting IDF target."); if (errMsg.includes("are satisfied")) { Logger.info(errMsg); OutputChannel.appendLine(errMsg); } else { - Logger.errorNotify(errMsg, err as Error, "setIdfTarget"); + Logger.errorNotify(errMsg, error, "setIdfTarget"); OutputChannel.appendLine(errMsg); }As per coding guidelines, use
Logger.error(message, error, "scope identifier")for caught errors, with a consistent scope identifier as the third argument.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/espIdf/setTarget/index.ts` around lines 225 - 236, The catch block in setTarget is passing non-Error throwables directly to the logger and using Logger.errorNotify; normalize the caught value by converting non-Error throwables into an Error (e.g., create a new Error with the stringified value or default message) so it always has message/stack, then call Logger.error(normalizedMessage, normalizedError, "setIdfTarget") instead of Logger.errorNotify; also keep writing errMsg to OutputChannel and use the same "setIdfTarget" scope identifier.
114-128:⚠️ Potential issue | 🟠 MajorFilter out connected boards whose IDF target cannot be resolved.
targetsFromIdf.find(...)can returnundefined, but the cast at Line 127 hides it. Selecting such a board will later dereferenceselectedTarget.idfTarget.targetand fail.Proposed fix
- connectedBoards = parsed.boards.map( - (b: any) => - ({ - label: b.name, - idfTarget: targetsFromIdf.find( - (t) => t.target === b.target - ), - description: b.description, - detail: `Status: CONNECTED${ - b.location ? ` Location: ${b.location}` : "" - }`, - isConnected: true, - boardInfo: b, - } as ISetTargetQuickPickItems) - ); + connectedBoards = parsed.boards.flatMap((b: any) => { + const idfTarget = targetsFromIdf.find( + (t) => t.target === b.target + ); + if (!idfTarget) { + return []; + } + return [ + { + label: b.name, + idfTarget, + description: b.description, + detail: `Status: CONNECTED${ + b.location ? ` Location: ${b.location}` : "" + }`, + isConnected: true, + boardInfo: b, + }, + ]; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/espIdf/setTarget/index.ts` around lines 114 - 128, The mapping that builds connectedBoards uses targetsFromIdf.find(...) which may return undefined but is cast to ISetTargetQuickPickItems; update the construction in the connectedBoards creation so you exclude any board entries where idfTarget is undefined (i.e., only include boards where targetsFromIdf.find(...) returns a value). Locate the mapping that produces ISetTargetQuickPickItems (the connectedBoards = parsed.boards.map(...) block) and either filter parsed.boards first or use a reduce/filter+map so that entries with a missing idfTarget are not added, ensuring later dereferences like selectedTarget.idfTarget.target are safe.src/flash/transports/uart/types/flashModel.ts (1)
20-35:⚠️ Potential issue | 🟠 MajorBreaking rename: verify all consumers were migrated.
The
partitionTablefield was renamed to the string-literal key"partition-table". Any remainingmodel.partitionTableaccess (including JTAG transport code, tests, and fixtures) will silently resolve toundefinedat runtime since no compile-time error is produced when the property is looked up via the old name on a typed object — but worse, plain JS orany-typed call sites will not be caught by the compiler at all. Please verify no stale references exist.Also consider whether a camelCase key (e.g.
partitionTable) would be more idiomatic for a TS interface; the hyphenated key forces bracket access (model["partition-table"]) at every call site. If the hyphenated form is required to mirror the JSON shape fromflasher_args.json, that's fine — otherwise an aliasing step in the builder would keep call sites cleaner.#!/bin/bash # Find any remaining references to the old field name and confirm the new key is used everywhere. echo "=== Old field references (should be zero) ===" rg -nP '\bpartitionTable\b' --type=ts -g '!**/node_modules/**' echo "=== New key references ===" rg -nP '"partition-table"|\[\s*["'\'']partition-table["'\'']\s*\]' --type=ts -g '!**/node_modules/**' echo "=== writeFlashArgs consumers ===" rg -nP '\bwriteFlashArgs\b' --type=ts -g '!**/node_modules/**'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flash/transports/uart/types/flashModel.ts` around lines 20 - 35, The interface FlashModel was changed from partitionTable to the string-literal key "partition-table", which can break callers still using model.partitionTable; search for and replace any remaining uses of partitionTable (including tests, fixtures, JTAG transport code, and any any-typed call sites) to use model["partition-table"] or update them to read from a normalized/canonical field; alternatively add a compatibility alias during model construction (e.g., populate partitionTable from model["partition-table"] or vice versa) so existing call sites keep working; ensure all references reported by ripgrep (the reviewer’s commands) are fixed and run tests to validate no runtime undefineds remain.src/langTools/index.ts (1)
176-185:⚠️ Potential issue | 🟠 MajorSkip local monitor interruption in web UI.
Line 177 runs the local monitor interruption path before the web check. In web mode, delegate directly to
IDFWebCommandKeys.Monitorbefore touching local terminal/monitor state.Move the web branch before local monitor handling
} else if (commandName === "monitor") { - await interruptMonitorWithDelay(workspaceURI); if (vscode.env.uiKind === vscode.UIKind.Web) { vscode.commands.executeCommand(IDFWebCommandKeys.Monitor); return new vscode.LanguageModelToolResult([ new vscode.LanguageModelTextPart( "Redirecting to ESP-IDF Web Monitor command" ), ]); } + await interruptMonitorWithDelay(workspaceURI); const noReset = await shouldDisableMonitorReset(workspaceURI); await createNewIdfMonitor(workspaceURI, noReset);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/langTools/index.ts` around lines 176 - 185, The code currently calls interruptMonitorWithDelay(workspaceURI) before checking for web UI; move the web UI branch ahead of local monitor handling so when vscode.env.uiKind === vscode.UIKind.Web the code immediately calls vscode.commands.executeCommand(IDFWebCommandKeys.Monitor) and returns the LanguageModelToolResult without invoking interruptMonitorWithDelay or touching local terminal state; update the conditional order around commandName === "monitor" to check the web case first and only run interruptMonitorWithDelay for non-web environments.src/taskManager.ts (1)
186-235:⚠️ Potential issue | 🟠 MajorEnsure the task listener is disposed on all paths and wrap task execution in error handling.
The success path at line 224 resolves without disposing
taskDisposable, leaving the listener active after all tasks complete—it will not affect this run but may interfere with future task execution if matching task IDs are reused. Additionally, theawait tasks.executeTask(...)inside the event handler (line 226) is unguarded and can produce an unhandled rejection if task execution fails.Register the listener before starting tasks, wrap all
tasks.executeTask()calls in try/catch, and dispose the listener on both success and failure paths to ensure clean state across multiple runs.🐛 Proposed control-flow fix
public static async runTasks() { - return new Promise<void>(async (resolve, reject) => { + return new Promise<void>((resolve, reject) => { if (TaskManager.tasks.length === 0) { return resolve(); } - let lastExecution = await tasks.executeTask(TaskManager.tasks[0]); + + let lastExecution: Awaited<ReturnType<typeof tasks.executeTask>>; + const cleanup = () => { + taskDisposable.dispose(); + TaskManager.disposables = TaskManager.disposables.filter( + (disposable) => disposable !== taskDisposable + ); + }; + const startNextTask = async () => { + try { + lastExecution = await tasks.executeTask(TaskManager.tasks[0]); + } catch (error) { + cleanup(); + reject(error); + } + }; + const taskDisposable = tasks.onDidEndTaskProcess(async (e) => { if ( e.execution && e.execution.task.definition.taskId === lastExecution.task.definition.taskId @@ if (e.exitCode !== 0) { // Task has already ended (this handler is triggered *after* end), // so terminating here is unnecessary and can produce VS Code errors // like "Task to terminate not found". this.cancelTasks(); - this.disposeListeners(); + cleanup(); return reject( new Error( `Task ${lastExecution.task.name} exited with code ${e.exitCode}` ) ); } if (TaskManager.tasks.length === 0) { TaskManager.tasks = []; + cleanup(); return resolve(); } else { - lastExecution = await tasks.executeTask(TaskManager.tasks[0]); + await startNextTask(); } } }); TaskManager.disposables.push(taskDisposable); + void startNextTask(); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/taskManager.ts` around lines 186 - 235, The runTasks method leaves the task listener (taskDisposable) active on the success path and executes tasks without error handling; register the onDidEndTaskProcess listener (taskDisposable) before calling tasks.executeTask, push it to TaskManager.disposables immediately, and wrap every call to tasks.executeTask (both the initial call and the awaited call inside the handler) in try/catch to convert failures into rejects; ensure the listener is disposed (call taskDisposable.dispose() or remove it from TaskManager.disposables) on all exit paths — success, failure (non-zero exitCode), and exception — before calling resolve() or reject() and before invoking this.cancelTasks()/this.disposeListeners().
🟠 Major comments (22)
src/common/store.ts-45-57 (1)
45-57:⚠️ Potential issue | 🟠 MajorStore the selected workspace folder as a URI string and implement fallback for stale references.
Storing
Uriobjects directly inglobalStatecauses a type mismatch: VS Code's Memento serializesUrias JSON and returns a plainUriComponentsobject (not a revivedUriinstance) on retrieval. Passing this toworkspace.getWorkspaceFolder(), which expects aUritype, will fail at runtime. Additionally, when the stored workspace folder is no longer open, the method returnsundefinedinstead of falling back to the first current workspace folder.Store
Uri.toString()and restore withUri.parse(). When the stored folder is stale or unavailable, fall back to the first current workspace folder.Proposed fix
public getSelectedWorkspaceFolder() { - const storedUri = this.get<Uri>( - ExtensionConfigStore.SELECTED_WORKSPACE_FOLDER, - workspace.workspaceFolders ? workspace.workspaceFolders?.[0].uri : undefined + const fallbackFolder = workspace.workspaceFolders?.[0]; + const storedUriValue = this.get<string>( + ExtensionConfigStore.SELECTED_WORKSPACE_FOLDER ); - if (!storedUri) { - return; + + if (storedUriValue) { + const storedFolder = workspace.getWorkspaceFolder(Uri.parse(storedUriValue)); + if (storedFolder) { + return storedFolder; + } } - return workspace.getWorkspaceFolder(storedUri); + + return fallbackFolder; } public setSelectedWorkspaceFolder(selectedFolder: Uri) { - this.set(ExtensionConfigStore.SELECTED_WORKSPACE_FOLDER, selectedFolder); + this.set( + ExtensionConfigStore.SELECTED_WORKSPACE_FOLDER, + selectedFolder.toString() + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/store.ts` around lines 45 - 57, getSelectedWorkspaceFolder currently retrieves a serialized Uri object and passes it to workspace.getWorkspaceFolder which expects a real Uri instance and also doesn't fall back if the stored folder is stale; change setSelectedWorkspaceFolder to store selectedFolder.toString() under ExtensionConfigStore.SELECTED_WORKSPACE_FOLDER, and change getSelectedWorkspaceFolder to read the stored string, if present call Uri.parse(storedString) and use workspace.getWorkspaceFolder(parsedUri); if that returns undefined (stale or closed folder) return workspace.workspaceFolders?.[0] as the fallback; ensure types reflect string storage in the backing get/set operations.src/common/registerCommand.ts-22-42 (1)
22-42: 🛠️ Refactor suggestion | 🟠 MajorReturn type
numberis misleading; returnvoidor theDisposable.
context.subscriptions.push(...)returns the new array length, not a handle. Any caller treating the returnednumberas a registration ID would be using meaningless data. Either returnvoid(common forregister*helpers that self-register intosubscriptions) or return theDisposableproduced bycommands.registerCommandso callers can dispose it independently if needed.♻️ Suggested change
export function registerIDFCommand( context: ExtensionContext, name: string, callback: (...args: any[]) => any -): number { +): Disposable { const telemetryCallback = async function (this: unknown, ...args: any[]): Promise<any> { const startTime = Date.now(); Logger.info(`Command::${name}::Executed`); try { return await callback.apply(this, args); } catch (error) { Logger.error(`Command::${name}::Failed`, error, `registerIDFCommand ${name}`); throw error; } finally { const timeSpent = Date.now() - startTime; Telemetry.sendEvent("command", { commandName: name }, { timeSpent }); } }; - return context.subscriptions.push( - commands.registerCommand(name, telemetryCallback) - ); + const disposable = commands.registerCommand(name, telemetryCallback); + context.subscriptions.push(disposable); + return disposable; }(Add
Disposableto thevscodeimport.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/registerCommand.ts` around lines 22 - 42, The function registerIDFCommand currently returns a number (the new length of context.subscriptions) which is misleading; change it to return the Disposable from commands.registerCommand so callers can dispose the registration. Update the signature of registerIDFCommand to return Disposable, add Disposable to the vscode imports, store the result of commands.registerCommand(name, telemetryCallback) in a local const (e.g., disposable), push that disposable into context.subscriptions, and return the disposable instead of the numeric push result; keep the telemetryCallback logic and error handling as-is.src/eraseFlash/index.ts-23-28 (1)
23-28:⚠️ Potential issue | 🟠 MajorAdd null check before dereferencing
wsFolder.
wsFoldercan beundefinedeven whenopenFolderCheckpasses. TheopenFolderCheckonly verifies that workspace folders exist (at least one); it does not guarantee thatESP.GlobalConfiguration.store.getSelectedWorkspaceFolder()returns a valid folder. If the stored workspace URI is no longer valid or hasn't been set, dereferencingwsFolder!.uriwill throw an opaque TypeError. Add a null check or handle the undefined case gracefully.Reference for comparison:
src/langTools/index.tssafely uses optional chaining and checks for undefined before proceeding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/eraseFlash/index.ts` around lines 23 - 28, The code dereferences wsFolder with wsFolder!.uri in the async handler which can be undefined; add a null/undefined check for wsFolder (or use optional chaining) before accessing .uri and handle the undefined case by returning early or propagating a clear error/cancel (mirroring the pattern used in src/langTools/index.ts). Update the async callback (the anonymous async (_progress, cancelToken, wsFolder) => { ... }) to validate wsFolder and avoid using the non-null assertion, and then call readParameter("idf.flashType", workspaceFolderUri) only when workspaceFolderUri is present.src/utils.ts-42-42 (1)
42-42:⚠️ Potential issue | 🟠 MajorAwait the relocated flash-encryption check.
The flash command path awaits
isFlashEncryptionEnabled(...); here the promise is used as a condition, which is always truthy and can incorrectly disable monitor reset.Proposed fix
- if (isFlashEncryptionEnabled(workspaceUri)) { + if (await isFlashEncryptionEnabled(workspaceUri)) { const valueReleaseModeEnabled = await getConfigValueFromSDKConfig( "CONFIG_SECURE_FLASH_ENCRYPTION_MODE_RELEASE", workspaceUriAlso applies to: 1230-1236
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils.ts` at line 42, The condition currently uses the promise returned by isFlashEncryptionEnabled(...) without awaiting it, so the check is always truthy and can disable monitor reset; update the flash command path to await isFlashEncryptionEnabled(...) before using its boolean result (e.g., const enabled = await isFlashEncryptionEnabled(...)) and then branch on that boolean to decide whether to reset the monitor; apply the same awaited-fix wherever isFlashEncryptionEnabled is used (notably around the monitor reset logic referenced and also in the occurrences at lines ~1230-1236).src/flash/index.ts-27-33 (1)
27-33:⚠️ Potential issue | 🟠 MajorResolve flash encryption after the command’s workspace is finalized.
This reads the selected workspace before
flash(...)runs its own folder-selection/precheck flow. In multi-root or initially-unselected workspaces, UART flashing can resolveisEncryptedfor the wrong folder or default tofalse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flash/index.ts` around lines 27 - 33, The current flashWithEncryptionResolution reads ESP.GlobalConfiguration.store.getSelectedWorkspaceFolder() and calls isFlashEncryptionEnabled before flash(...) runs its own workspace-selection/precheck, causing wrong folder resolution; instead, stop resolving isEncrypted here and let flash(...) perform workspace selection and then call isFlashEncryptionEnabled against the workspace selected by flash. Update flashWithEncryptionResolution to pass undefined for isEncrypted to flash(explicitFlashType?, partition?) and move the isFlashEncryptionEnabled call into the beginning of flash (or into its workspace-finalization helper) so flash uses ESP.GlobalConfiguration.store.getSelectedWorkspaceFolder() after workspace selection/precheck to determine isEncrypted.src/flash/transports/dfu/helpers.ts-22-29 (1)
22-29:⚠️ Potential issue | 🟠 MajorFix DFU PID parsing before labeling devices.
Line 24 slices the array literal
[0], not the regex match, sopidis always empty and ESP32-S2 devices are labeled as ESP32-S3.Proposed fix
function deviceLabel(selectedDevice: string) { - const regex = new RegExp(/:\d+\]/g); - const pid = selectedDevice.match(regex) ? [0].slice(4, -1) : []; + const pid = Number.parseInt( + selectedDevice.match(/:(\d+)\]/)?.[1] ?? "", + 10 + ); - if (pid && pid[0] === 2) { + if (pid === 2) { return "ESP32-S2"; } return "ESP32-S3"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flash/transports/dfu/helpers.ts` around lines 22 - 29, The deviceLabel function currently slices the literal [0] instead of the regex match, so pid is always empty; update the pid extraction in deviceLabel to get the first match from selectedDevice.match(regex), slice off the leading ':' and trailing ']' (e.g. slice(1, -1)), convert that substring to a number (parseInt or Number) and then compare the numeric pid (e.g. pid === 2) when deciding between "ESP32-S2" and "ESP32-S3"; ensure you reference the deviceLabel function and the pid variable when making this change.src/eraseFlash/transports/uart/task.ts-30-48 (1)
30-48:⚠️ Potential issue | 🟠 MajorMake the erase single-flight guard atomic and rollback-safe.
The guard is checked before two
awaits, so concurrent calls can both pass and enqueue erase tasks. Also, if task creation throws after Line 38,isErasingremains stucktrue.Proposed fix
if (EraseFlashSession.isErasing) { throw new Error("ALREADY_ERASING"); } - const modifiedEnv = await configureEnvVariables(workspace); - const { - pythonPath: pythonBinPath, - esptoolScriptPath, - } = await resolveEsptoolInvocation(modifiedEnv["IDF_PATH"]); EraseFlashSession.isErasing = true; - const args = buildUartEraseFlashArgs(esptoolScriptPath, port); - return addProcessTask( - "Erase Flash", - workspace, - pythonBinPath, - args, - workspace.fsPath || process.cwd(), - modifiedEnv, - { captureOutput } - ); + try { + const modifiedEnv = await configureEnvVariables(workspace); + const { + pythonPath: pythonBinPath, + esptoolScriptPath, + } = await resolveEsptoolInvocation(modifiedEnv["IDF_PATH"]); + const args = buildUartEraseFlashArgs(esptoolScriptPath, port); + return addProcessTask( + "Erase Flash", + workspace, + pythonBinPath, + args, + workspace.fsPath || process.cwd(), + modifiedEnv, + { captureOutput } + ); + } catch (error) { + EraseFlashSession.isErasing = false; + throw error; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/eraseFlash/transports/uart/task.ts` around lines 30 - 48, Make the single-flight guard atomic by doing a synchronous check-and-set on EraseFlashSession.isErasing at function start (before any await) and ensure rollback if any async step fails: replace the current pre-await check with an immediate if (EraseFlashSession.isErasing) throw and then set EraseFlashSession.isErasing = true right away; wrap the subsequent await calls to configureEnvVariables and resolveEsptoolInvocation, plus the call to addProcessTask, in try/catch so that on any thrown error you reset EraseFlashSession.isErasing = false and rethrow; finally, when addProcessTask successfully returns, attach a completion handler (or finally on the returned promise) to clear EraseFlashSession.isErasing when the erase process exits so the flag is not left stuck—apply changes around configureEnvVariables, resolveEsptoolInvocation, buildUartEraseFlashArgs and addProcessTask usage.src/eraseFlash/transports/uart/cmd.ts-55-64 (1)
55-64:⚠️ Potential issue | 🟠 MajorOnly report erase success when the task succeeds, and cleanup in
finally.If
runTasksWithBoolean()returnsfalse, Lines 56-62 still notify “Erase flash done”. If it throws, Line 64 is skipped and the erase session can remain locked.Proposed fix
- const eraseFlashResult = await TaskManager.runTasksWithBoolean(); - if (!cancelToken.isCancellationRequested) { - const msg = "⚡️ Erase flash done"; - OutputChannel.appendLine(msg, "Erase flash"); - Logger.infoNotify(msg); - OutputChannel.appendLine("Flash memory content has been erased."); - Logger.infoNotify("Flash memory content has been erased."); + try { + const eraseFlashResult = await TaskManager.runTasksWithBoolean(); + if (eraseFlashResult && !cancelToken.isCancellationRequested) { + const msg = "⚡️ Erase flash done"; + OutputChannel.appendLine(msg, "Erase flash"); + Logger.infoNotify(msg); + OutputChannel.appendLine("Flash memory content has been erased."); + Logger.infoNotify("Flash memory content has been erased."); + } + return { + continueFlag: eraseFlashResult, + executions: collectExecutions(eraseFlashExecution), + }; + } finally { TaskManager.disposeListeners(); + EraseFlashSession.isErasing = false; } - EraseFlashSession.isErasing = false; - return { - continueFlag: eraseFlashResult, - executions: collectExecutions(eraseFlashExecution), - }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/eraseFlash/transports/uart/cmd.ts` around lines 55 - 64, The current code always logs success and disposes listeners regardless of TaskManager.runTasksWithBoolean() outcome and may skip cleanup if it throws; change the flow so you capture the boolean result from TaskManager.runTasksWithBoolean() and only call OutputChannel.appendLine/Logger.infoNotify success messages when eraseFlashResult is true and cancelToken.isCancellationRequested is false, move TaskManager.disposeListeners() and EraseFlashSession.isErasing = false into a finally block so they always run even if runTasksWithBoolean() throws, and keep failure/cancellation paths from logging the success messages (referencing eraseFlashResult, TaskManager.runTasksWithBoolean, cancelToken.isCancellationRequested, OutputChannel.appendLine, Logger.infoNotify, TaskManager.disposeListeners, and EraseFlashSession.isErasing).src/eraseFlash/main.ts-39-43 (1)
39-43:⚠️ Potential issue | 🟠 MajorGuard
eraseFlashMainwithEraseFlashSession.isErasingbefore async preflight.This function resets
EraseFlashSession.isErasingon errors, but never checks or sets it on entry. Two erase commands can therefore run through method selection/monitor interruption and dispatch erase transports concurrently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/eraseFlash/main.ts` around lines 39 - 43, eraseFlashMain currently performs async preflight (selectFlashMethod, interruptMonitorWithDelay) without checking or setting EraseFlashSession.isErasing, allowing concurrent erase commands; fix by at the very start of eraseFlashMain atomically checking EraseFlashSession.isErasing and if true abort/throw, otherwise set EraseFlashSession.isErasing = true, then proceed to call selectFlashMethod and interruptMonitorWithDelay, and ensure all exit paths (success, error, cancellation) clear EraseFlashSession.isErasing so it cannot remain stuck; reference the eraseFlashMain function and EraseFlashSession.isErasing, and keep the guard around the async preflight and full erase dispatch.src/flash/transports/uart/uartFlashExecution.ts-39-57 (1)
39-57:⚠️ Potential issue | 🟠 MajorSet the flashing guard before any async work and clear it on setup failures.
Two concurrent calls can both pass Line 39 before either reaches Line 45, so duplicate flash tasks can be enqueued. Also, any error after setting the flag leaves
FlashSession.isFlashingstuck until something else resets it.🔒 Proposed fix
if (FlashSession.isFlashing) { throw new Error("ALREADY_FLASHING"); } - assertFlashSectionsReadable(buildDirPath, model); - const { pythonPath: pythonBinPath, esptoolScriptPath } = - await resolveEsptoolInvocation(modifiedEnv["IDF_PATH"]!); FlashSession.isFlashing = true; - const flasherArgs = partitionToUse - ? getSingleBinFlasherArgs(model, esptoolScriptPath, partitionToUse) - : getFlasherArgs(model, esptoolScriptPath, encryptPartitions); - return addProcessTask( - "Flash", - workspace, - pythonBinPath, - flasherArgs, - buildDirPath, - modifiedEnv, - { captureOutput } - ); + try { + assertFlashSectionsReadable(buildDirPath, model); + const { pythonPath: pythonBinPath, esptoolScriptPath } = + await resolveEsptoolInvocation(modifiedEnv["IDF_PATH"]!); + const flasherArgs = partitionToUse + ? getSingleBinFlasherArgs(model, esptoolScriptPath, partitionToUse) + : getFlasherArgs(model, esptoolScriptPath, encryptPartitions); + return addProcessTask( + "Flash", + workspace, + pythonBinPath, + flasherArgs, + buildDirPath, + modifiedEnv, + { captureOutput } + ); + } catch (error) { + FlashSession.isFlashing = false; + throw error; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flash/transports/uart/uartFlashExecution.ts` around lines 39 - 57, Set FlashSession.isFlashing immediately before doing any async work to prevent race conditions, and ensure it is cleared on setup failures: move the assignment to FlashSession.isFlashing = true to just before calling resolveEsptoolInvocation/getFlasherArgs/getSingleBinFlasherArgs and wrap the subsequent async/setup sequence (calls to assertFlashSectionsReadable, resolveEsptoolInvocation, building flasherArgs and calling addProcessTask) in a try/catch that resets FlashSession.isFlashing = false on any thrown error (and rethrows), or use a try/finally to clear the flag on failure but keep it set for a successfully started addProcessTask; reference FlashSession.isFlashing, resolveEsptoolInvocation, assertFlashSectionsReadable, getFlasherArgs/getSingleBinFlasherArgs and addProcessTask when making the change.src/espIdf/menuconfig/saveDefConfig.ts-59-63 (1)
59-63:⚠️ Potential issue | 🟠 MajorDispose task listeners in
finallyand don’t require a token for success notification.Line 60 skips the success notification whenever no
CancellationTokenis passed. Also, ifTaskManager.runTasks()rejects, Line 63 is never reached and task listeners remain registered.🧹 Proposed fix
- await TaskManager.runTasks(); - if (cancelToken && !cancelToken.isCancellationRequested) { - Logger.infoNotify("def-config has been generated"); - } - TaskManager.disposeListeners(); + try { + await TaskManager.runTasks(); + if (!cancelToken?.isCancellationRequested) { + Logger.infoNotify("def-config has been generated"); + } + } finally { + TaskManager.disposeListeners(); + } return saveDefSdkconfigExecution;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/espIdf/menuconfig/saveDefConfig.ts` around lines 59 - 63, Move TaskManager.disposeListeners() into a finally block so listeners are always cleaned up even if TaskManager.runTasks() throws, and change the success-notify condition to not require a CancellationToken by using a check like “if (!cancelToken?.isCancellationRequested)” so Logger.infoNotify("def-config has been generated") runs when tasks complete successfully and the token is not cancelled; update the block around TaskManager.runTasks(), cancelToken, Logger.infoNotify, and TaskManager.disposeListeners accordingly.src/flash/transports/dfu/dfuFlashExecution.ts-34-59 (1)
34-59:⚠️ Potential issue | 🟠 MajorClose the DFU flashing race and reset the flag on preparation errors.
Line 38 awaits before Line 40 sets
FlashSession.isFlashing, so parallel DFU flash requests can both pass the guard. Errors fromdfuFlashingArgsoraddProcessTaskalso leave the flag stuck.🔒 Proposed fix
if (FlashSession.isFlashing) { throw new Error("ALREADY_FLASHING"); } - assertFlashSectionsReadable(buildDirPath, model); - const { pythonPath: pythonBinPath } = - await resolveEsptoolInvocation(modifiedEnv["IDF_PATH"]!); FlashSession.isFlashing = true; - const dfuResult = await dfuFlashingArgs( - pythonBinPath, - modifiedEnv, - model.chip, - buildDirPath - ); - if (!dfuResult) { + try { + assertFlashSectionsReadable(buildDirPath, model); + const { pythonPath: pythonBinPath } = + await resolveEsptoolInvocation(modifiedEnv["IDF_PATH"]!); + const dfuResult = await dfuFlashingArgs( + pythonBinPath, + modifiedEnv, + model.chip, + buildDirPath + ); + if (!dfuResult) { + throw new Error("NO_DFU_DEVICE_SELECTED"); + } + return addProcessTask( + "Flash", + workspace, + dfuResult.cmdToUse, + dfuResult.args, + buildDirPath, + modifiedEnv, + { captureOutput } + ); + } catch (error) { FlashSession.isFlashing = false; - throw new Error("NO_DFU_DEVICE_SELECTED"); + throw error; } - return addProcessTask( - "Flash", - workspace, - dfuResult.cmdToUse, - dfuResult.args, - buildDirPath, - modifiedEnv, - { captureOutput } - ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flash/transports/dfu/dfuFlashExecution.ts` around lines 34 - 59, The guard can race because resolveEsptoolInvocation is awaited before FlashSession.isFlashing is set and any prep error leaves the flag stuck; move setting FlashSession.isFlashing = true to immediately after the initial if (FlashSession.isFlashing) guard, then wrap the preparation steps (resolveEsptoolInvocation and dfuFlashingArgs) in a try/catch that resets FlashSession.isFlashing = false and rethrows on error; keep the flag set when you successfully call and return addProcessTask so concurrent flashes remain blocked until the launched task takes over.src/espIdf/unitTest/configure.ts-101-108 (1)
101-108:⚠️ Potential issue | 🟠 MajorDon’t flash after a failed or canceled unit-test build.
buildTestAppdropsbuildMain(...).continueFlag, sobuildFlashTestAppstill callsflashTestAppwhen the build was aborted. This can flash stale binaries after a failed/canceled build.Proposed fix
export async function buildTestApp( unitTestAppDirPath: Uri, cancelToken: CancellationToken -) { +): Promise<boolean> { let flashType = readParameter( "idf.flashType", unitTestAppDirPath ) as ESP.FlashType; @@ if (!buildCmdResults.continueFlag) { - return; + return false; } + return true; } @@ export async function buildFlashTestApp( unitTestAppDirPath: Uri, cancelToken: CancellationToken ) { - await buildTestApp(unitTestAppDirPath, cancelToken); + const canFlash = await buildTestApp(unitTestAppDirPath, cancelToken); + if (!canFlash) { + return; + } await flashTestApp(unitTestAppDirPath, cancelToken); }Also applies to: 130-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/espIdf/unitTest/configure.ts` around lines 101 - 108, The code is flashing test firmware even when buildMain indicates it was aborted; update buildTestApp and buildFlashTestApp to check buildMain's result.continueFlag (e.g., buildCmdResults.continueFlag) before calling flashTestApp so flashTestApp is only invoked when continueFlag is true; specifically, in the blocks that call buildMain and then flashTestApp (currently using buildCmdResults and later in the buildFlashTestApp path), return early or skip flashTestApp when continueFlag is false to avoid flashing stale binaries.src/flash/shared/errHandling.ts-81-129 (1)
81-129:⚠️ Potential issue | 🟠 MajorReturn the sentinel value described by the handler contract.
The comment says
ALREADY_FLASHINGreturnsfalse, but every branch currently returnsundefined. That makes it unsafe for callers to decide whether to preserve or clear the existing flash session state.Proposed fix
-// Returns false only for ALREADY_FLASHING (caller returns before clearing session flag). export function handleFlashCommandCatch( error: unknown, flashType: ESP.FlashType -) { +): boolean { const errMsg = error instanceof Error ? error.message : String(error); @@ OutputChannel.appendLineAndShow(errStr, "Flash"); Logger.errorNotify(errStr, error as Error, "flashCommand already flashing"); - return; + return false; } @@ if (mapped) { presentMappedFlashCommandError(mapped, error); - return; + return true; } @@ Logger.errorNotify( errStr, error as Error, "uartFlashCommand esptool.py access error" ); - return; + return true; } @@ Logger.errorNotify( errStr, error as Error, `${category} unknown error`, undefined, false ); + return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flash/shared/errHandling.ts` around lines 81 - 129, The handler currently never returns the sentinel described by its contract; update handleFlashCommandCatch to explicitly return false in the ALREADY_FLASHING branch (where it logs "Already one flash process is running!") and ensure all other handled paths return true so callers know the error was handled and can clear the flash session flag — e.g., after presentMappedFlashCommandError(mapped, error) and after the esptool.py ENOENT/SCRIPT_PERMISSION_ERROR branch either return true, or add a single explicit return true at the end of the function after the final Logger.errorNotify; keep the existing logging and calls to OutputChannel/Logger intact.src/eraseFlash/transports/jtag/tclClientCmd.ts-34-72 (1)
34-72:⚠️ Potential issue | 🟠 MajorUse
.once()and add a timeout to prevent listener accumulation and indefinite hangs.The
responseanderrorlisteners are registered via.on()but never removed after the promise resolves. Repeated calls accumulate stale handlers (memory leak), and if the TCL client never emits a response, the promise hangs forever.Since
TCLClientextendsEventEmitter, replace.on()with.once()for single-response commands, or manually remove the listeners after resolution. Add a timeout (similar to TCLClient's internal 5-second pattern) to fail gracefully if no response arrives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/eraseFlash/transports/jtag/tclClientCmd.ts` around lines 34 - 72, The promise in tclClientCmd.ts registers persistent listeners on client for "response" and "error" which causes handler accumulation and potential hangs; change those .on(...) handlers to .once(...) on the TCLClient instance (the "response" and "error" events used in the Promise around sendCommand(fullCommand)) and add a timeout (e.g., mirror TCLClient's ~5s) that rejects/resolves with a failure CapturedTaskOutput when elapsed, ensuring the timeout is cleared on either event and that the Promise resolves only once (continueFlag/execs set accordingly) to prevent memory leaks and indefinite waits.src/espIdf/hints/openocdhint.ts-141-158 (1)
141-158:⚠️ Potential issue | 🟠 MajorStore handler references instead of removing all listeners on dispose.
OpenOCDManageris a singleton; callingremoveAllListeners("data"/"error")removes all listeners on those events, not just the ones registered here. If other extension features or multiple instances register listeners on the same events, they will be broken. Store the handler references and remove only those specific handlers.Proposed fix
const openOCDManager = OpenOCDManager.init(); - // Add event listener to OpenOCDManager to detect when there's new output - openOCDManager.on("data", (data) => { + const onData = (data: Buffer) => { const content = data.toString(); this.processOutput(content); - }); + }; - openOCDManager.on("error", (error, data) => { + const onError = (error: Error, data?: Buffer) => { const content = data ? data.toString() : error.message; this.processOutput(content); - }); + }; + + openOCDManager.on("data", onData); + openOCDManager.on("error", onError); this.openOCDLogWatcher = { dispose: () => { - openOCDManager.removeAllListeners("data"); - openOCDManager.removeAllListeners("error"); + openOCDManager.removeListener("data", onData); + openOCDManager.removeListener("error", onError); }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/espIdf/hints/openocdhint.ts` around lines 141 - 158, The current dispose implementation calls openOCDManager.removeAllListeners("data"/"error") which clears all listeners from the OpenOCDManager singleton; instead capture the handler functions you pass to openOCDManager.on (the data handler used in openOCDManager.on("data", ...) and the error handler used in openOCDManager.on("error", ...)), store them (e.g., const onData = (data) => ...; const onError = (error, data) => ...), register those handlers with openOCDManager.on, and change this.openOCDLogWatcher.dispose to call openOCDManager.removeListener("data", onData) and openOCDManager.removeListener("error", onError) so only these specific handlers are removed.src/flash/transports/jtag/flashTclClient.ts-40-78 (1)
40-78:⚠️ Potential issue | 🟠 MajorAdd timeout and listener cleanup for the OpenOCD response.
If OpenOCD never emits
responseorerror, this promise never settles and the flash flow hangs indefinitely. Also prefer one-shot listeners and remove the opposite listener once settled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flash/transports/jtag/flashTclClient.ts` around lines 40 - 78, The promise created around the `client` response can hang forever; change the `client.on("response", ...)` and `client.on("error", ...)` handlers in flashTclClient.ts to one-shot listeners (e.g., `once`) and add a timeout (setTimeout) that will resolve with a failed CapturedTaskOutput after a configurable delay; on any settlement (response, error, or timeout) clear the timeout and remove the other listener (e.g., removeListener or use once so only the counterpart is removed/ignored) to avoid memory leaks or double-resolve, and ensure `sendCommand(fullCommand)` remains invoked after listeners are registered.src/build/buildMain.ts-120-132 (1)
120-132:⚠️ Potential issue | 🟠 MajorDo not continue the pipeline after cancellation.
The success notification checks
cancelToken, but the returnedcontinueFlagdoes not. If cancellation arrives after tasks finish, callers can still proceed to flash/monitor.Gate the returned result on cancellation
- if (buildResult && !cancelToken.isCancellationRequested) { + const completed = + buildResult && sizeResult && !cancelToken.isCancellationRequested; + if (completed) { updateIdfComponentsTree(workspace); Logger.infoNotify("Build Successful"); const flashCmd = await buildFinishFlashCmd(workspace); if (flashCmd) { OutputChannel.appendLine(flashCmd, "Build"); } } cleanupBuildState(buildTask); return { - continueFlag: buildResult && sizeResult, + continueFlag: completed, executions, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/build/buildMain.ts` around lines 120 - 132, The pipeline may continue after a cancellation because the returned continueFlag only uses buildResult and sizeResult; update the return value so it also checks cancelToken.isCancellationRequested (e.g., set continueFlag to buildResult && sizeResult && !cancelToken.isCancellationRequested) to prevent downstream steps from running when cancelled; keep the existing success notification gating (the earlier if block using cancelToken) and ensure cleanupBuildState(buildTask) still runs, but do not change other call sites like updateIdfComponentsTree or buildFinishFlashCmd.src/flash/transports/jtag/flashTclClient.ts-38-38 (1)
38-38:⚠️ Potential issue | 🟠 MajorEscape TCL arguments before building the command.
Wrapping args in quotes is not enough: a build path or
idf.jtagFlashCommandExtraArgsvalue containing"or\can break the TCL command and inject extra tokens.Escape quoted TCL args
- const fullCommand = `${command} ${args.map((arg) => `"${arg}"`).join(" ")}`; + const quoteTclArg = (arg: string) => + `"${arg.replace(/\\/g, "\\\\").replace(/"/g, '\\"')}"`; + const fullCommand = [command, ...args.map(quoteTclArg)].join(" ");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flash/transports/jtag/flashTclClient.ts` at line 38, The current fullCommand construction in flashTclClient.ts builds fullCommand = `${command} ${args.map((arg) => `"${arg}"`).join(" ")}` which is vulnerable to TCL token injection; before quoting each arg (in the args.map used to build fullCommand) escape TCL-special characters by replacing backslashes with double-backslashes and double quotes with backslash-quote (i.e., implement an escapeTclArg/quoteTclArg helper), then wrap the escaped value in double quotes and join — update the args.map call and use the new helper wherever args are incorporated into the TCL command (referencing fullCommand, command, and args).src/flash/transports/uart/uartFlashCmd.ts-50-100 (1)
50-100:⚠️ Potential issue | 🟠 MajorGuarantee flash cleanup on thrown errors.
If
createFlashModel, task creation, orrunTasksWithBoolean()throws, Line 100 is skipped andFlashSession.isFlashingcan remain true. Put the reset/listener cleanup in afinallyso failed flashes do not block later build/flash commands.Suggested cleanup shape
+ const cleanupFlashState = () => { + TaskManager.disposeListeners(); + FlashSession.isFlashing = false; + }; + cancelToken.onCancellationRequested(() => { TaskManager.cancelTasks(); - TaskManager.disposeListeners(); + cleanupFlashState(); }); - const model = await createFlashModel( + try { + const model = await createFlashModel( flasherArgsJsonPath, port, flashBaudRate - ); + ); ... - FlashSession.isFlashing = false; - return { - continueFlag: flashResult, - executions: collectExecutions( - preFlashExecution, - flashExecution, - postFlashExecution - ), - }; + return { + continueFlag: flashResult, + executions: collectExecutions( + preFlashExecution, + flashExecution, + postFlashExecution + ), + }; + } finally { + cleanupFlashState(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flash/transports/uart/uartFlashCmd.ts` around lines 50 - 100, The code can throw before the final cleanup so FlashSession.isFlashing and TaskManager listeners may not be reset; wrap the main flash workflow (from createFlashModel through runTasksWithBoolean and result handling) in a try/finally where in finally you always set FlashSession.isFlashing = false and call TaskManager.disposeListeners(), and ensure any cancelToken.onCancellationRequested handlers are still registered but the global cleanup happens in the finally block (referencing createFlashModel, CustomTask.addCustomTask, createDfuFlashProcessTask/createUartFlashProcessTask, TaskManager.runTasksWithBoolean, TaskManager.disposeListeners, FlashSession.isFlashing).src/flash/transports/jtag/jtagCmd.ts-67-109 (1)
67-109:⚠️ Potential issue | 🟠 MajorReset
FlashSession.isFlashingon successful and thrown JTAG paths.Line 67 sets the global flash flag, but the success path returns without clearing it. Wrap the JTAG execution flow in
try/finallyso future build/flash operations are not blocked after a successful flash or an exception.Ensure JTAG flash state is always cleared
FlashSession.isFlashing = true; + try { const forceUNIXPathSeparator = readParameter( "openocd.jtag.command.force_unix_path_separator", workspace ); ... return { continueFlag: true, executions: collectExecutions( preFlashExecution, ...flashExecution.executions, postFlashExecution ), }; + } finally { + FlashSession.isFlashing = false; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flash/transports/jtag/jtagCmd.ts` around lines 67 - 109, The code sets FlashSession.isFlashing = true before running the JTAG flow but never clears it on the success return path; wrap the entire JTAG execution sequence (from setting FlashSession.isFlashing through the preFlashExecution, TaskManager.runTasks, jtagFlash call, postFlashExecution and final return) in a try/finally and set FlashSession.isFlashing = false in the finally block so the flag is cleared whether jtagFlash (and subsequent throwCapturedTaskFailure) succeeds or throws; keep existing logic for calling jtagFlash, throwCapturedTaskFailure, CustomTask.addCustomTask and TaskManager.runTasks unchanged, only moving them inside the try and ensuring the finally always resets FlashSession.isFlashing.src/common/PreCheck.ts-147-155 (1)
147-155:⚠️ Potential issue | 🟠 MajorMake
minIdfVersionCheck()fail closed instead of rejecting.Several commands await this helper before
PreCheck.perform()runs. IfIDF_PATHis missing or version resolution throws, the command bypassesopenFolderCheckand the intended user-facing precheck message.🛡️ Proposed fail-closed handling
export async function minIdfVersionCheck(minVersion: string) { const currentEnvVars = ESP.ProjectConfiguration.store.get<{ [key: string]: string; }>(ESP.ProjectConfiguration.CURRENT_IDF_CONFIGURATION, {}); - const currentVersion = await getEspIdfFromCMake(currentEnvVars["IDF_PATH"]); + let currentVersion = ""; + try { + const idfPath = currentEnvVars["IDF_PATH"]; + currentVersion = idfPath ? await getEspIdfFromCMake(idfPath) : ""; + } catch (error) { + Logger.error( + "Failed to resolve ESP-IDF version for precheck.", + error as Error, + "common PreCheck minIdfVersionCheck" + ); + } return [ () => PreCheck.espIdfVersionValidator(minVersion, currentVersion), cmdNeedToolVersionOrHigher("v" + minVersion, "ESP-IDF"), ] as PreCheckInput; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/PreCheck.ts` around lines 147 - 155, minIdfVersionCheck currently lets getEspIdfFromCMake throw which bypasses PreCheck; wrap the call in a try/catch and treat missing IDF_PATH or any exception as an unresolved/old version so the check fails closed. Concretely, read currentEnvVars["IDF_PATH"], and if it's falsy or if getEspIdfFromCMake throws, set currentVersion to a sentinel low version (e.g., "0.0.0") instead of rethrowing; then return the same validators (PreCheck.espIdfVersionValidator(minVersion, currentVersion) and cmdNeedToolVersionOrHigher("v"+minVersion, "ESP-IDF")) so the precheck reports a failure rather than allowing the command to proceed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db4f081e-92d2-4504-8dbf-fa19ebd8c660
📒 Files selected for processing (93)
l10n/bundle.l10n.es.jsonl10n/bundle.l10n.pt.jsonl10n/bundle.l10n.ru.jsonl10n/bundle.l10n.zh-cn.jsonsrc/build/buildCmd.tssrc/build/buildFinishFlashCmd.tssrc/build/buildHelpers.tssrc/build/buildMain.tssrc/build/buildTask.tssrc/build/cmakeConfigure.tssrc/build/dfuExecution.tssrc/build/index.tssrc/build/sizeExecution.tssrc/build/validation.tssrc/buildFlashMonitor/index.tssrc/cdtDebugAdapter/debugConfProvider.tssrc/cdtDebugAdapter/imageViewPanel.tssrc/common/PreCheck.tssrc/common/registerCommand.tssrc/common/store.tssrc/common/withProgressWrapper.tssrc/config.tssrc/customTasks/customTaskProvider.tssrc/eim/getExistingSetups.tssrc/eraseFlash/eraseFlashSession.tssrc/eraseFlash/index.tssrc/eraseFlash/main.tssrc/eraseFlash/transports/jtag/eraseFlashJtagResponse.tssrc/eraseFlash/transports/jtag/jtag.tssrc/eraseFlash/transports/jtag/tclClientCmd.tssrc/eraseFlash/transports/uart/cmd.tssrc/eraseFlash/transports/uart/eraseFlashUartArgs.tssrc/eraseFlash/transports/uart/task.tssrc/espBom/index.tssrc/espIdf/debugAdapter/verifyApp.tssrc/espIdf/hints/openocdhint.tssrc/espIdf/menuconfig/saveDefConfig.tssrc/espIdf/monitor/command.tssrc/espIdf/monitor/interruptMonitorWithDelay.tssrc/espIdf/openOcd/openOcdManager.tssrc/espIdf/partition-table/tree.tssrc/espIdf/reconfigure/task.tssrc/espIdf/setTarget/index.tssrc/espIdf/setTarget/setTargetInIdf.tssrc/espIdf/size/idfSizeTask.tssrc/espIdf/tracing/appTracePanel.tssrc/espIdf/tracing/tree/appTraceArchiveTreeDataProvider.tssrc/espIdf/unitTest/adapter.tssrc/espIdf/unitTest/configure.tssrc/espIdf/unitTest/utils.tssrc/extension.tssrc/flash/eraseFlashJtag.tssrc/flash/eraseFlashTask.tssrc/flash/flashCmd.tssrc/flash/flashProject.tssrc/flash/flashTask.tssrc/flash/index.tssrc/flash/jtag.tssrc/flash/main.tssrc/flash/resolveFlashContext.tssrc/flash/selectFlashMethod.tssrc/flash/shared/errHandling.tssrc/flash/shared/esptool/resolveEsptoolInvocation.tssrc/flash/shared/flashSession.tssrc/flash/shared/verifyFlashBins.tssrc/flash/startFlashing.tssrc/flash/transports/dfu/dfuFlashExecution.tssrc/flash/transports/dfu/getDFUArgs.tssrc/flash/transports/dfu/helpers.tssrc/flash/transports/jtag/assertMinimumOpenOcdVersionForJtag.tssrc/flash/transports/jtag/flashTclClient.tssrc/flash/transports/jtag/jtagCmd.tssrc/flash/transports/uart/flashArgsBuilder.tssrc/flash/transports/uart/flashModelBuilder.tssrc/flash/transports/uart/types/flashModel.tssrc/flash/transports/uart/uartFlashCmd.tssrc/flash/transports/uart/uartFlashExecution.tssrc/flash/uartFlash.tssrc/flash/verify/canFlash.tssrc/flash/verify/flashEncryption.tssrc/langTools/index.tssrc/statusBar/index.tssrc/taskManager.tssrc/taskManager/customExecution.tssrc/test/cursor-clangd-prompt.test.tssrc/test/suite/build.test.tssrc/test/suite/dfu.test.tssrc/test/suite/eraseFlash.test.tssrc/test/suite/flash.test.tssrc/test/suite/taskManager.test.tssrc/utils.tstestFiles/flasher_args_mixed_encryption.jsontestFiles/flasher_args_unencrypted.json
💤 Files with no reviewable changes (10)
- src/config.ts
- src/flash/jtag.ts
- src/flash/eraseFlashJtag.ts
- src/espIdf/size/idfSizeTask.ts
- src/flash/flashCmd.ts
- src/flash/eraseFlashTask.ts
- src/flash/startFlashing.ts
- src/build/buildCmd.ts
- src/flash/uartFlash.ts
- src/flash/flashTask.ts
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/flash/transports/dfu/helpers.ts (1)
39-52:⚠️ Potential issue | 🟠 MajorNormalize
getDfuListto always return an array.
listAvailableDfuDevices()can returnnullwhen no DFU devices match, butsrc/flash/transports/dfu/getDFUArgs.ts:22-28treatsgetDfuList()asstring[]. Returning[]avoids a no-device runtime failure.Proposed fix
-export async function getDfuList(env: { [key: string]: string }) { +export async function getDfuList(env: { [key: string]: string }): Promise<string[]> { const dfuListStr = await execChildProcess( "dfu-util", ["--list"], @@ - const arrayDfuDevices = listAvailableDfuDevices(dfuListStr); + const arrayDfuDevices = listAvailableDfuDevices(dfuListStr) ?? []; return arrayDfuDevices; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flash/transports/dfu/helpers.ts` around lines 39 - 52, getDfuList currently forwards the result of listAvailableDfuDevices which may be null; change getDfuList to always return an array (string[]) by checking the result of listAvailableDfuDevices(dfuListStr) and returning an empty array when it is null/undefined so callers like getDFUArgs that expect string[] won't fail; keep using execChildProcess and OutputChannel.init() as before and update the function signature/return handling in getDfuList accordingly.src/taskManager.ts (1)
207-215:⚠️ Potential issue | 🟡 MinorRace condition:
onDidEndTaskProcesscan fire beforelastExecutionis assigned, stranding the task queue.The listener at line 207 is registered before
tasks.executeTask()(line 267) resolves. For very fast or failed-to-start tasks,onDidEndTaskProcesscan fire before the Promise settles, causing the handler to hit theif (!lastExecution) return;guard at line 208. Since the task has already ended, the handler never fires again for that task, and the queue is left in a waiting state indefinitely.Replace the
lastExecutionguard with a capturedtaskIdfromTaskManager.tasks[0].definition.taskId(taken before execution). Checke.execution.task.definition.taskId === capturedTaskIdinstead of relying onlastExecutionbeing populated—this eliminates the race without blocking execution flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/taskManager.ts` around lines 207 - 215, The listener registered via tasks.onDidEndTaskProcess (taskDisposable) races with tasks.executeTask because it checks lastExecution; instead capture the taskId from TaskManager.tasks[0].definition.taskId (or the queued task's definition.taskId) immediately before calling tasks.executeTask, then in the onDidEndTaskProcess handler compare e.execution.task.definition.taskId to that capturedTaskId instead of checking lastExecution, and remove/replace the if (!lastExecution) return guard so the handler reliably matches and cleans up the queue even if the task ends before lastExecution is assigned.
🧹 Nitpick comments (11)
src/espIdf/hints/openocdhint.ts (1)
67-90: Redundant!versionguard.Line 73 already returns early when
!version, so the additional!versioncheck on line 83 is dead. You can simplify:- // Skip initialization if openOCD version is not supporting hints - const minRequiredVersion = "v0.12.0-esp32-20250226"; - if ( - !version || - !PreCheck.openOCDVersionValidator(minRequiredVersion, version) - ) { + // Skip initialization if openOCD version is not supporting hints + const minRequiredVersion = "v0.12.0-esp32-20250226"; + if (!PreCheck.openOCDVersionValidator(minRequiredVersion, version)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/espIdf/hints/openocdhint.ts` around lines 67 - 90, The initialize() method contains a redundant check for !version before calling PreCheck.openOCDVersionValidator; remove the second !version clause so the version truthiness is only validated once (after OpenOCDManager.version() returns) and rely on the existing early return, keeping the check that calls PreCheck.openOCDVersionValidator(minRequiredVersion, version) and the minRequiredVersion constant intact to skip initialization when version is insufficient.src/common/store.ts (1)
45-68: Method name saysUribut returns aWorkspaceFolder.
getSelectedWorkspaceFolderUri()returns eitherworkspace.workspaceFolders[0]or the result ofworkspace.getWorkspaceFolder(...)— both areWorkspaceFolderobjects, notUri. That's also why every caller in this PR doesws!.uri/selectedWorkspace.uri(seesrc/flash/index.ts:56,src/cdtDebugAdapter/debugConfProvider.ts:49,91,src/statusBar/index.ts:77-79).Consider renaming to
getSelectedWorkspaceFolder()to match the actual return type, or return the.urito match the name. Also worth adding an explicit return type annotation (WorkspaceFolder | undefined) for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/store.ts` around lines 45 - 68, The method getSelectedWorkspaceFolderUri is misnamed: it returns WorkspaceFolder objects, not Uris; update it to either (A) rename the method to getSelectedWorkspaceFolder and add an explicit return type WorkspaceFolder | undefined, or (B) keep the name and return the .uri (Uri | undefined) instead — pick one consistent option and adjust callers accordingly (places that currently use ws!.uri / selectedWorkspace.uri must be updated if you return Uri directly). Ensure the related setter setSelectedWorkspaceFolder and the stored key ExtensionConfigStore.SELECTED_WORKSPACE_FOLDER remain consistent with the chosen convention.src/common/PreCheck.ts (1)
149-166: Silent fallback to0.0.0can mask misconfiguration.In
minIdfVersionCheck, ifgetEspIdfFromCMake(idfPath)throws or ifIDF_PATHis not set in the project configuration, the current version falls back to"0.0.0"which forces the precheck to fail with a generic "needs {minVersion} or higher" message. That makes it hard to distinguish "IDF not configured" from "IDF version too old".Consider logging the swallowed error via
Logger.error(..., "common PreCheck minIdfVersionCheck")so users/maintainers can diagnose setup issues.- } catch { - currentVersion = UNRESOLVED_IDF_VERSION; - } + } catch (error) { + Logger.error( + `Failed to resolve ESP-IDF version from ${idfPath}`, + error as Error, + "common PreCheck minIdfVersionCheck" + ); + currentVersion = UNRESOLVED_IDF_VERSION; + }As per coding guidelines: "Use
Logger.error(message, error, "scope identifier")for caught errors, with a consistent scope identifier as the third argument".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/PreCheck.ts` around lines 149 - 166, The function minIdfVersionCheck silently falls back to UNRESOLVED_IDF_VERSION when IDF_PATH is missing or getEspIdfFromCMake(idfPath) throws; update it to log these situations using Logger.error(message, error, "common PreCheck minIdfVersionCheck") so failures are diagnosable: when idfPath is falsy, call Logger.error with a clear message and no error object (or a new Error describing missing IDF_PATH), and inside the catch capture the caught error and pass it to Logger.error along with a descriptive message; keep the existing return behavior and references to ESP.ProjectConfiguration and PreCheck.espIdfVersionValidator/UNRESOLVED_IDF_VERSION unchanged.src/eraseFlash/transports/jtag/jtag.ts (1)
39-55: Consider adding OpenOCD readiness verification for consistency with JTAG flash.
jtagFlashCommandMainretriesclient.verifyOpenOCDReady()up to 3 times after launching OpenOCD (seesrc/flash/transports/jtag/jtagCmd.tslines 53-65), butjtagEraseFlashCommandonly checks the launch prompt. If OpenOCD is still starting wheneraseFlashTelnetCommandsendshalt; flash erase_sector ..., the command can fail with an unclear error. Consider mirroring the readiness retry loop before callingeraseFlashTelnetCommand.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/eraseFlash/transports/jtag/jtag.ts` around lines 39 - 55, jtagEraseFlashCommand currently only prompts the user to launch OpenOCD but does not wait for the server to be ready, so add the same readiness verification loop used in jtagFlashCommandMain: after creating TCLClient (client) call client.verifyOpenOCDReady() up to 3 times with short delay/retries and abort with the same error handling if it still fails; reference the verifyOpenOCDReady() method and the retry logic in jtagFlashCommandMain (the loop around client.verifyOpenOCDReady()) and then proceed to call eraseFlashTelnetCommand only when verifyOpenOCDReady() succeeds.src/langTools/index.ts (1)
188-221: Consider reusingbuildFlashAndMonitorto avoid duplication.This branch re-implements the same build → flash → monitor pipeline that now lives in
buildFlashAndMonitor(src/buildFlashMonitor/index.ts). Two paths with slightly different semantics (e.g.,resolveFlashTypeForTask/resolvePartitionToUseForTaskvs. directreadParameter, and the ordering ofinterruptMonitorWithDelay) are easy to drift apart. If you can expose a captureOutput variant ofbuildFlashAndMonitorthat returns{ continueFlag, executions }, thiselse ifbranch could simply call it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/langTools/index.ts` around lines 188 - 221, The branch duplicating build → flash → monitor should be replaced by calling a capture-output variant of buildFlashAndMonitor; add or expose a new function (e.g., buildFlashAndMonitorCapture or extend buildFlashAndMonitor in src/buildFlashMonitor/index.ts) that accepts the same inputs used here (workspaceURI, token, flashType/partition resolution results or flags to read parameters) and returns { continueFlag, executions }, then replace this else-if body to call that variant instead of calling buildMain, flashMain, interruptMonitorWithDelay, shouldDisableMonitorReset, and createNewIdfMonitor directly; ensure the new API supports captureOutput=true behavior and preserves the existing semantics for parameter resolution (resolveFlashTypeForTask/resolvePartitionToUseForTask vs readParameter) and monitor reset ordering.src/eraseFlash/transports/jtag/tclClientCmd.ts (1)
37-90: Consider extracting shared TCL command executor and adding a timeout.This function duplicates the promise/settle/listener cleanup structure of
jtagFlashinsrc/flash/transports/jtag/flashTclClient.tsalmost verbatim, differing only in the success predicate and error messages. Consider extracting a shared helper (e.g.,runTclCommand(client, fullCommand, { successPredicate, errorMessages })) to avoid drift between flash and erase paths.Separately, there's no timeout here — if the TCL server neither emits
responsenorerror, the promise hangs indefinitely (blockingthrowCapturedTaskFailuredownstream). Consider adding asetTimeoutthat resolves with a captured-error execution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/eraseFlash/transports/jtag/tclClientCmd.ts` around lines 37 - 90, Extract the duplicated promise/listener/cleanup logic into a shared helper (e.g., runTclCommand) and use it from both this file's erase flow and the existing jtagFlash in flashTclClient.ts: the helper should accept the client, fullCommand, a successPredicate (used instead of isJtagEraseFlashResponseSuccess when called), and customizable error messages for onError and onResponse-failure; move the onResponse/onError cleanup (client.off) and resolve behavior into that helper and call client.sendCommand from it. Also add a configurable timeout inside runTclCommand that, when reached, clears listeners and resolves with a CapturedTaskOutput representing a timed-out failure (continueFlag false, success false, non-zero exitCode) so the promise cannot hang indefinitely. Ensure references to onResponse, onError, client.once/on/off, and createCapturedExecution are handled inside the helper.src/taskManager.ts (2)
82-103:throwCapturedTaskFailureonly surfaces the first failing execution.The loop throws on the first failed capture and discards subsequent failures. If pre-build, build, and post-build all produce captured errors, only the pre-build message is reported. For the current usage (sequential tasks that stop on first failure) this is fine, but consider aggregating messages (e.g.
throw new Error(failures.join("\n"))) to avoid surprising users when multiple independent failures accumulate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/taskManager.ts` around lines 82 - 103, throwCapturedTaskFailure currently throws on the first failing execution and drops later failures; modify throwCapturedTaskFailure to collect all failure messages instead of immediately throwing: iterate executions (using OutputCapturingExecution | ShellOutputCapturingExecution.getOutput()), for each failed execution build a message from executionOutput.stderr || executionOutput.stdout || `Task exited with code ${executionOutput.exitCode}`, push to a failures array, and after the loop if failures.length > 0 throw a single Error(failures.join("\n")) so all captured failures are surfaced together.
295-321:addProcessTaskreturn type widens toProcessExecutioneven withcaptureOutput: true.The return type is the union
OutputCapturingExecution | ProcessExecution, but callers that passcaptureOutput: truewill need to narrow before callinggetOutput(). A conditional return type (overloads) keeps the captured-output path ergonomic.♻️ Proposed overloads
+export function addProcessTask( + name: string, + workspaceUri: Uri, + command: string, + args: string[], + cwd: string, + env: { [key: string]: string }, + options: { captureOutput: true; presentation?: TaskPresentationOptions } +): OutputCapturingExecution; +export function addProcessTask( + name: string, + workspaceUri: Uri, + command: string, + args: string[], + cwd: string, + env: { [key: string]: string }, + options?: { captureOutput?: false; presentation?: TaskPresentationOptions } +): ProcessExecution; export function addProcessTask( name: string, workspaceUri: Uri, command: string, args: string[], cwd: string, env: { [key: string]: string }, options?: { captureOutput?: boolean; presentation?: TaskPresentationOptions; } ): OutputCapturingExecution | ProcessExecution {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/taskManager.ts` around lines 295 - 321, addProcessTask currently returns a union (OutputCapturingExecution | ProcessExecution) which forces callers to narrow even when they pass options.captureOutput = true; change addProcessTask to provide TypeScript overloads: one overload signature where options has captureOutput: true and the return type is OutputCapturingExecution, another overload for captureOutput?: false | undefined returning ProcessExecution (and a general overload for omitted options if desired), then implement the existing function body (single implementation) matching those overloads; reference the addProcessTask function and getTaskProcessExecution along with the OutputCapturingExecution and ProcessExecution types when adding overload signatures so callers who pass captureOutput: true get the precise return type.src/flash/transports/uart/uartFlashCmd.ts (1)
51-56: Merge the two cancellation listeners.Two separate
onCancellationRequestedsubscriptions serve a single logical purpose; consolidate them into one handler to keep the cancel path in a single place and to make it easier to also clean up the returnedDisposableif ever needed.♻️ Proposed fix
- cancelToken.onCancellationRequested(() => { - TaskManager.cancelTasks(); - }); - cancelToken.onCancellationRequested(() => { - FlashSession.isFlashing = false; - }); + cancelToken.onCancellationRequested(() => { + TaskManager.cancelTasks(); + FlashSession.isFlashing = false; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flash/transports/uart/uartFlashCmd.ts` around lines 51 - 56, Merge the two cancellation listeners into a single onCancellationRequested handler: replace the two separate cancelToken.onCancellationRequested callbacks with one callback that calls TaskManager.cancelTasks() and sets FlashSession.isFlashing = false, and return/store the Disposable if you need to dispose/unregister it later; update any existing references to the separate registrations to use this single consolidated handler (look for cancelToken.onCancellationRequested, TaskManager.cancelTasks, and FlashSession.isFlashing).src/flash/transports/jtag/flashTclClient.ts (1)
26-32: Unsafe double-cast on the shim execution.
({ getOutput: async () => output } as unknown) as IdfTaskExecutionbypasses the compiler. IfIdfTaskExecution's member set ever changes (e.g.OutputCapturingExecutiongains a required field consumed bythrowCapturedTaskFailure), this shim will silently compile while failing at runtime. Consider defining a narrow interface (e.g.OutputCapturingExecutionLikewith justgetOutput(): Promise<CapturedTaskOutput>) and broadenMaybeIdfTaskExecution/throwCapturedTaskFailureto accept it, instead of lying to the type system.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flash/transports/jtag/flashTclClient.ts` around lines 26 - 32, Replace the unsafe double-cast in createCapturedExecution by introducing a narrow interface (e.g. OutputCapturingExecutionLike) that exposes only getOutput(): Promise<CapturedTaskOutput>, change createCapturedExecution to return that interface instead of casting to IdfTaskExecution, and update the consumer types/signatures (MaybeIdfTaskExecution and throwCapturedTaskFailure) to accept this narrow interface (or a union/overload) so callers that only need getOutput can use the safe shim without lying to the type system; ensure you remove the "as unknown as IdfTaskExecution" cast and adjust any places that previously expected a full IdfTaskExecution to handle the new interface accordingly.src/build/buildMain.ts (1)
38-48: JSDoc restates the signature; trim or remove.The JSDoc at lines 38-48 mostly restates parameter names and return type that are already self-documenting. Per coding guidelines, prefer comments for why/constraints over narrating what, and avoid redundant JSDoc. Consider trimming to a single sentence about the orchestration contract (mutual exclusion with flashing, post-build size step, DFU append) or removing entirely.
As per coding guidelines: "Avoid redundant JSDoc documentation (e.g., type and function docs restating the same concept)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/build/buildMain.ts` around lines 38 - 48, The JSDoc block above the build function in src/build/buildMain.ts redundantly restates the parameter names and return type; remove the verbose `@param/`@returns lines and replace with a single short sentence summarizing the orchestration contract (e.g., mutual exclusion with flashing, post-build size step, and DFU append) or delete the block entirely; update the comment immediately above the build function (the existing comment block at the top of the file) so it no longer duplicates the function signature.
🤖 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/build/buildMain.ts`:
- Around line 135-151: The catch block in buildMain.ts currently has two
separate if statements causing duplicate notifications for the same error (e.g.,
"ALREADY_BUILDING" logs both Logger.errorNotify and then hits the general else);
change the independent ifs into an if / else if / else chain so the branches are
mutually exclusive: keep the initial cleanupBuildState(buildTask) call, then
check if (error instanceof Error && error.message === "ALREADY_BUILDING") to
call Logger.errorNotify("Already a build is running!", ... , "buildCommand"),
else if (error instanceof Error && error.message === "BUILD_TERMINATED") to call
Logger.warnNotify("Build is Terminated"), else call the generic
Logger.errorNotify("Something went wrong while trying to build the project",
errorInstance, "buildCommand", undefined, false) and then return { continueFlag:
false, executions } as before.
- Around line 117-119: The current logic can report "Build Successful" when
buildResult is true but sizeResult is false because
throwCapturedTaskFailure(executions) may return without throwing if the size
task had no captured output; modify the post-task handling so that when
sizeResult is false you either (A) throw an explicit error after calling
throwCapturedTaskFailure(executions) if it returned normally, or (B) include
sizeResult in the success gate (require buildResult && sizeResult && !cancelled)
before calling updateIdfComponentsTree and notifying success; update the code
around the buildResult/sizeResult checks that call throwCapturedTaskFailure and
the success branch to reference buildResult, sizeResult, cancelled, and the
updateIdfComponentsTree/notification calls so failures in the size task cannot
be silently ignored.
In `@src/buildFlashMonitor/index.ts`:
- Around line 88-93: The progress callback mixes the original workspaceFolderUri
and the resolved folderUri (from taskWsFolder), causing
interruptMonitorWithDelay to read settings from the wrong scope; update the call
to interruptMonitorWithDelay to use the resolved folderUri (the same variable
passed into shouldDisableMonitorReset and createNewIdfMonitor) so all three
calls use the same workspace URI (interruptMonitorWithDelay,
shouldDisableMonitorReset, createNewIdfMonitor) and thus read the same
idf.monitorDelay and reset settings.
In `@src/common/PreCheck.ts`:
- Around line 69-100: Update the OpenOCD version regex in
openOCDVersionValidator to strictly match dotted numeric version parts and allow
multi-digit components: replace /v(\d+.?\d+.?\d)-esp32-(\d+)/ with a pattern
that escapes dots and uses \d+ for each numeric component (e.g.
^v(\d+\.\d+\.\d+)-esp32-(\d+)$) so versions like v0.12.10-esp32-20250226 match;
keep the existing parsing logic that checks
minVersionParsed/currentVersionParsed, compareVersion call, and the error
logging behavior.
In `@src/common/registerCommand.ts`:
- Around line 32-34: The catch block in registerCommand.ts passes a compile-time
cast (error as Error) to Logger.error which fails for non-Error rejections;
update the catch to normalize the caught value into a real Error before logging
and rethrowing (e.g., if (!(err instanceof Error)) wrap with new
Error(String(err)) or preserve message/stack when present) and then call
Logger.error(`Command::${name}::Failed`, normalizedError, `registerIDFCommand
${name}`) and throw the original/normalized error; adjust the catch around the
registerIDFCommand/command callback to use this normalizedError variable.
In `@src/eraseFlash/main.ts`:
- Around line 85-88: In the catch block handling errors for eraseFlashCommand,
replace the use of Logger.errorNotify with Logger.error so the caught error
follows the TypeScript guideline; keep the existing errMsg and cast (error as
Error) and pass the same scope identifier string "eraseFlashCommand" to
Logger.error(errMsg, error as Error, "eraseFlashCommand") to ensure consistent
logging.
In `@src/espIdf/tracing/tree/appTraceArchiveTreeDataProvider.ts`:
- Around line 49-51: The EventEmitter instance type doesn't match the declared
field type: change the instantiation of OnDidChangeTreeData so its generic
includes null (match EventEmitter<AppTraceArchiveItems | null>) to allow
this.OnDidChangeTreeData.fire(null) in refresh() to compile; update the new
EventEmitter<> generic to EventEmitter<AppTraceArchiveItems | null> for the
OnDidChangeTreeData field.
- Around line 78-82: The call to
ESP.GlobalConfiguration.store.getSelectedWorkspaceFolderUri() is misleading
because that method actually returns a WorkspaceFolder; rename the method to
getSelectedWorkspaceFolder() and update all callers accordingly (including this
usage where storedWorkspaceFolder is assigned and .uri.fsPath is read into
baseFolderPath); update the implementation and any interface/type definitions in
ESP.GlobalConfiguration.store to the new name and run a project-wide
search/replace for getSelectedWorkspaceFolderUri to ensure consistency.
In `@src/flash/main.ts`:
- Around line 50-52: The code is inconsistent: flashMain currently prompts the
user via selectFlashMethod when flashType is falsy, while buildFlashMonitor
calls resolveFlashTypeForTask which silently falls back to idf.flashType; pick
one consistent approach and implement it across callers. Either change flashMain
to use resolveFlashTypeForTask(workspaceFolderUri, flashType) when flashType is
falsy (so no UI is shown) or remove the silent fallback and have
resolveFlashTypeForTask return undefined so flashMain always calls
selectFlashMethod; update the callers (e.g., buildFlashMonitor/index.ts) to
match the chosen flow; reference resolveFlashTypeForTask, flashMain, and
selectFlashMethod to locate and align the behavior.
- Around line 131-140: The current flow in the try/catch of src/flash/main.ts
drops the captured executions:
throwCapturedTaskFailure(flashCmdResult.executions) causes an error to be thrown
and the catch returns `{ continueFlag: false, executions: [] }`, losing the
`flashCmdResult.executions` that callers need; fix by preserving
executions—either return `flashCmdResult` directly when
`flashCmdResult.continueFlag` is false instead of throwing, or modify the catch
to extract the executions from the thrown error (or from a rethrown wrapper) and
return them (ensure this logic touches throwCapturedTaskFailure, the catch that
calls handleFlashCommandCatch, and the FlashSession.isFlashing handling so
callers receive the original `executions` array).
In `@src/flash/shared/errHandling.ts`:
- Around line 105-128: In handleFlashCommandCatch, fix the two
logging/notification issues: when errnoCode === "ENOENT" use the computed
category rather than the hardcoded "uartFlashCommand esptool.py access error"
(replace that literal with category so JTAG/DFU/uart are attributed correctly)
and for the unknown-error fallback change OutputChannel.appendLine to
OutputChannel.appendLineAndShow and call Logger.errorNotify with notifyUser
enabled (remove the trailing false) so the Flash output is revealed and the user
receives a notification; reference OutputChannel.appendLineAndShow,
Logger.errorNotify, errnoCode, category, and the handleFlashCommandCatch
function when making these edits.
In `@src/flash/transports/dfu/helpers.ts`:
- Around line 95-98: The current extraction in helpers.ts silently returns an
empty string when the narrow regex fails; change the logic in the function that
uses selectedDfuDevice.detail (the code creating regex, pathValue) to extract
any quoted path using a capturing regex like /path="([^"]+)"/ and return the
captured group if present, otherwise fail explicitly (throw or return an error)
with a clear message including selectedDfuDevice.detail so the caller cannot
proceed with an empty selector; ensure you reference the same variables (regex,
pathValue, selectedDfuDevice.detail) when updating the logic.
In `@src/flash/transports/jtag/flashTclClient.ts`:
- Around line 34-41: The quoteTclArg function currently only escapes backslashes
and double quotes, but must also escape TCL variable and command substitution
characters; update quoteTclArg to additionally escape '$' and '[' (i.e., replace
'$' → '\$' and '[' → '\[') while preserving the correct escaping order (escape
backslashes first, then other characters) so the returned string prevents TCL
variable/command substitution when used by OpenOCD.
In `@src/flash/transports/jtag/jtagCmd.ts`:
- Around line 41-67: Add the same entry guard used in
createDfuFlashProcessTask/createUartFlashProcessTask to jtagFlashCommandMain: at
the start of the function check FlashSession.isFlashing and if true immediately
throw the same "ALREADY_FLASHING" error (or return the same error flow those
functions use) before any async work (before calling OpenOCDManager.init or
creating TCLClient); this ensures FlashSession.isFlashing is set only by the
first caller and prevents concurrent JTAG flash invocations from racing with the
finally block that clears the flag.
In `@src/flash/transports/uart/uartFlashCmd.ts`:
- Around line 38-109: The uartFlashCommandMain function never sets
FlashSession.isFlashing = true before scheduling tasks, which allows
cancellation handlers to clear the flag prematurely; fix this by assigning
FlashSession.isFlashing = true at the start of the try block inside
uartFlashCommandMain (before awaiting createFlashModel and before
creating/adding tasks) so the flag is set as soon as flashing begins, preserving
symmetry with the cancellation handlers and matching the jtag pattern.
In `@src/langTools/index.ts`:
- Around line 376-382: The current code builds errorMessageResult including
error.stack and returns it in a LanguageModelTextPart (see errorMessageResult
and new vscode.LanguageModelTextPart), which leaks stack traces; instead remove
the stack from the returned string, return a sanitized message (e.g., "Failed to
execute command \"<commandName>\": <errorMessage>") and log the full error via
Logger.error(errorMessage, error, "langToolsInvoke") before returning the
LanguageModelToolResult so callers see only the sanitized message while the
stack is recorded in logs.
---
Outside diff comments:
In `@src/flash/transports/dfu/helpers.ts`:
- Around line 39-52: getDfuList currently forwards the result of
listAvailableDfuDevices which may be null; change getDfuList to always return an
array (string[]) by checking the result of listAvailableDfuDevices(dfuListStr)
and returning an empty array when it is null/undefined so callers like
getDFUArgs that expect string[] won't fail; keep using execChildProcess and
OutputChannel.init() as before and update the function signature/return handling
in getDfuList accordingly.
In `@src/taskManager.ts`:
- Around line 207-215: The listener registered via tasks.onDidEndTaskProcess
(taskDisposable) races with tasks.executeTask because it checks lastExecution;
instead capture the taskId from TaskManager.tasks[0].definition.taskId (or the
queued task's definition.taskId) immediately before calling tasks.executeTask,
then in the onDidEndTaskProcess handler compare
e.execution.task.definition.taskId to that capturedTaskId instead of checking
lastExecution, and remove/replace the if (!lastExecution) return guard so the
handler reliably matches and cleans up the queue even if the task ends before
lastExecution is assigned.
---
Nitpick comments:
In `@src/build/buildMain.ts`:
- Around line 38-48: The JSDoc block above the build function in
src/build/buildMain.ts redundantly restates the parameter names and return type;
remove the verbose `@param/`@returns lines and replace with a single short
sentence summarizing the orchestration contract (e.g., mutual exclusion with
flashing, post-build size step, and DFU append) or delete the block entirely;
update the comment immediately above the build function (the existing comment
block at the top of the file) so it no longer duplicates the function signature.
In `@src/common/PreCheck.ts`:
- Around line 149-166: The function minIdfVersionCheck silently falls back to
UNRESOLVED_IDF_VERSION when IDF_PATH is missing or getEspIdfFromCMake(idfPath)
throws; update it to log these situations using Logger.error(message, error,
"common PreCheck minIdfVersionCheck") so failures are diagnosable: when idfPath
is falsy, call Logger.error with a clear message and no error object (or a new
Error describing missing IDF_PATH), and inside the catch capture the caught
error and pass it to Logger.error along with a descriptive message; keep the
existing return behavior and references to ESP.ProjectConfiguration and
PreCheck.espIdfVersionValidator/UNRESOLVED_IDF_VERSION unchanged.
In `@src/common/store.ts`:
- Around line 45-68: The method getSelectedWorkspaceFolderUri is misnamed: it
returns WorkspaceFolder objects, not Uris; update it to either (A) rename the
method to getSelectedWorkspaceFolder and add an explicit return type
WorkspaceFolder | undefined, or (B) keep the name and return the .uri (Uri |
undefined) instead — pick one consistent option and adjust callers accordingly
(places that currently use ws!.uri / selectedWorkspace.uri must be updated if
you return Uri directly). Ensure the related setter setSelectedWorkspaceFolder
and the stored key ExtensionConfigStore.SELECTED_WORKSPACE_FOLDER remain
consistent with the chosen convention.
In `@src/eraseFlash/transports/jtag/jtag.ts`:
- Around line 39-55: jtagEraseFlashCommand currently only prompts the user to
launch OpenOCD but does not wait for the server to be ready, so add the same
readiness verification loop used in jtagFlashCommandMain: after creating
TCLClient (client) call client.verifyOpenOCDReady() up to 3 times with short
delay/retries and abort with the same error handling if it still fails;
reference the verifyOpenOCDReady() method and the retry logic in
jtagFlashCommandMain (the loop around client.verifyOpenOCDReady()) and then
proceed to call eraseFlashTelnetCommand only when verifyOpenOCDReady() succeeds.
In `@src/eraseFlash/transports/jtag/tclClientCmd.ts`:
- Around line 37-90: Extract the duplicated promise/listener/cleanup logic into
a shared helper (e.g., runTclCommand) and use it from both this file's erase
flow and the existing jtagFlash in flashTclClient.ts: the helper should accept
the client, fullCommand, a successPredicate (used instead of
isJtagEraseFlashResponseSuccess when called), and customizable error messages
for onError and onResponse-failure; move the onResponse/onError cleanup
(client.off) and resolve behavior into that helper and call client.sendCommand
from it. Also add a configurable timeout inside runTclCommand that, when
reached, clears listeners and resolves with a CapturedTaskOutput representing a
timed-out failure (continueFlag false, success false, non-zero exitCode) so the
promise cannot hang indefinitely. Ensure references to onResponse, onError,
client.once/on/off, and createCapturedExecution are handled inside the helper.
In `@src/espIdf/hints/openocdhint.ts`:
- Around line 67-90: The initialize() method contains a redundant check for
!version before calling PreCheck.openOCDVersionValidator; remove the second
!version clause so the version truthiness is only validated once (after
OpenOCDManager.version() returns) and rely on the existing early return, keeping
the check that calls PreCheck.openOCDVersionValidator(minRequiredVersion,
version) and the minRequiredVersion constant intact to skip initialization when
version is insufficient.
In `@src/flash/transports/jtag/flashTclClient.ts`:
- Around line 26-32: Replace the unsafe double-cast in createCapturedExecution
by introducing a narrow interface (e.g. OutputCapturingExecutionLike) that
exposes only getOutput(): Promise<CapturedTaskOutput>, change
createCapturedExecution to return that interface instead of casting to
IdfTaskExecution, and update the consumer types/signatures
(MaybeIdfTaskExecution and throwCapturedTaskFailure) to accept this narrow
interface (or a union/overload) so callers that only need getOutput can use the
safe shim without lying to the type system; ensure you remove the "as unknown as
IdfTaskExecution" cast and adjust any places that previously expected a full
IdfTaskExecution to handle the new interface accordingly.
In `@src/flash/transports/uart/uartFlashCmd.ts`:
- Around line 51-56: Merge the two cancellation listeners into a single
onCancellationRequested handler: replace the two separate
cancelToken.onCancellationRequested callbacks with one callback that calls
TaskManager.cancelTasks() and sets FlashSession.isFlashing = false, and
return/store the Disposable if you need to dispose/unregister it later; update
any existing references to the separate registrations to use this single
consolidated handler (look for cancelToken.onCancellationRequested,
TaskManager.cancelTasks, and FlashSession.isFlashing).
In `@src/langTools/index.ts`:
- Around line 188-221: The branch duplicating build → flash → monitor should be
replaced by calling a capture-output variant of buildFlashAndMonitor; add or
expose a new function (e.g., buildFlashAndMonitorCapture or extend
buildFlashAndMonitor in src/buildFlashMonitor/index.ts) that accepts the same
inputs used here (workspaceURI, token, flashType/partition resolution results or
flags to read parameters) and returns { continueFlag, executions }, then replace
this else-if body to call that variant instead of calling buildMain, flashMain,
interruptMonitorWithDelay, shouldDisableMonitorReset, and createNewIdfMonitor
directly; ensure the new API supports captureOutput=true behavior and preserves
the existing semantics for parameter resolution
(resolveFlashTypeForTask/resolvePartitionToUseForTask vs readParameter) and
monitor reset ordering.
In `@src/taskManager.ts`:
- Around line 82-103: throwCapturedTaskFailure currently throws on the first
failing execution and drops later failures; modify throwCapturedTaskFailure to
collect all failure messages instead of immediately throwing: iterate executions
(using OutputCapturingExecution | ShellOutputCapturingExecution.getOutput()),
for each failed execution build a message from executionOutput.stderr ||
executionOutput.stdout || `Task exited with code ${executionOutput.exitCode}`,
push to a failures array, and after the loop if failures.length > 0 throw a
single Error(failures.join("\n")) so all captured failures are surfaced
together.
- Around line 295-321: addProcessTask currently returns a union
(OutputCapturingExecution | ProcessExecution) which forces callers to narrow
even when they pass options.captureOutput = true; change addProcessTask to
provide TypeScript overloads: one overload signature where options has
captureOutput: true and the return type is OutputCapturingExecution, another
overload for captureOutput?: false | undefined returning ProcessExecution (and a
general overload for omitted options if desired), then implement the existing
function body (single implementation) matching those overloads; reference the
addProcessTask function and getTaskProcessExecution along with the
OutputCapturingExecution and ProcessExecution types when adding overload
signatures so callers who pass captureOutput: true get the precise return type.
🪄 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: 2ca8b3ec-e77e-41b4-8c0b-bd9d95566e6a
📒 Files selected for processing (39)
src/build/buildMain.tssrc/buildFlashMonitor/index.tssrc/cdtDebugAdapter/debugConfProvider.tssrc/cdtDebugAdapter/imageViewPanel.tssrc/common/PreCheck.tssrc/common/registerCommand.tssrc/common/store.tssrc/common/withProgressWrapper.tssrc/eraseFlash/index.tssrc/eraseFlash/main.tssrc/eraseFlash/transports/jtag/jtag.tssrc/eraseFlash/transports/jtag/tclClientCmd.tssrc/eraseFlash/transports/uart/cmd.tssrc/eraseFlash/transports/uart/task.tssrc/espIdf/hints/openocdhint.tssrc/espIdf/menuconfig/saveDefConfig.tssrc/espIdf/setTarget/index.tssrc/espIdf/setTarget/setTargetInIdf.tssrc/espIdf/tracing/tree/appTraceArchiveTreeDataProvider.tssrc/espIdf/unitTest/adapter.tssrc/espIdf/unitTest/configure.tssrc/espIdf/unitTest/utils.tssrc/flash/flashProject.tssrc/flash/index.tssrc/flash/main.tssrc/flash/shared/errHandling.tssrc/flash/transports/dfu/dfuFlashExecution.tssrc/flash/transports/dfu/helpers.tssrc/flash/transports/jtag/flashTclClient.tssrc/flash/transports/jtag/jtagCmd.tssrc/flash/transports/uart/flashModelBuilder.tssrc/flash/transports/uart/types/flashModel.tssrc/flash/transports/uart/uartFlashCmd.tssrc/flash/transports/uart/uartFlashExecution.tssrc/langTools/index.tssrc/statusBar/index.tssrc/taskManager.tssrc/test/suite/flash.test.tssrc/utils.ts
✅ Files skipped from review due to trivial changes (2)
- src/flash/transports/uart/types/flashModel.ts
- src/espIdf/unitTest/adapter.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- src/eraseFlash/transports/uart/task.ts
- src/eraseFlash/transports/uart/cmd.ts
- src/eraseFlash/index.ts
- src/flash/flashProject.ts
- src/cdtDebugAdapter/imageViewPanel.ts
- src/flash/transports/uart/uartFlashExecution.ts
- src/common/withProgressWrapper.ts
- src/espIdf/menuconfig/saveDefConfig.ts
- src/flash/transports/uart/flashModelBuilder.ts
- src/espIdf/setTarget/setTargetInIdf.ts
- src/espIdf/unitTest/configure.ts
- src/espIdf/unitTest/utils.ts
- src/espIdf/setTarget/index.ts
- src/test/suite/flash.test.ts
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/flash/transports/dfu/helpers.ts (1)
39-52:⚠️ Potential issue | 🟠 MajorReturn an empty list when no DFU devices match.
listAvailableDfuDevices()returnsnullwhentext.match()finds no matches.getDfuListpropagates this null, butsrc/flash/transports/dfu/getDFUArgs.ts:28unsafely casts it tostring[]and accesses.lengthon line 31, causing a runtime error when no devices are detected. Normalize the return to an empty array using the nullish coalescing operator.Proposed fix
-export async function getDfuList(env: { [key: string]: string }) { +export async function getDfuList(env: { [key: string]: string }): Promise<string[]> { const dfuListStr = await execChildProcess( "dfu-util", ["--list"], process.cwd(), OutputChannel.init(), { env, maxBuffer: 500 * 1024, cwd: process.cwd(), } ); - const arrayDfuDevices = listAvailableDfuDevices(dfuListStr); + const arrayDfuDevices = listAvailableDfuDevices(dfuListStr) ?? []; return arrayDfuDevices; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flash/transports/dfu/helpers.ts` around lines 39 - 52, getDfuList currently returns whatever listAvailableDfuDevices(dfuListStr) returns, but listAvailableDfuDevices can return null when no matches are found; change getDfuList (in helpers.ts) to normalize that result to an empty array (e.g., use the nullish coalescing operator on the call to listAvailableDfuDevices) so callers like getDFUArgs can safely treat the return as string[] and check .length without runtime errors.src/langTools/index.ts (1)
151-208:⚠️ Potential issue | 🟠 MajorHandle
continueFlag === falsebefore returning task output.The delegated flows can stop gracefully with
continueFlag: falseand no executions, but this value is only assigned and never read. For cases like flash validation failures or an already-running task, the LM tool can return an empty result instead of telling the user why the command did not continue.Proposed direction
if (TASK_COMMANDS.has(commandName)) { for (const execution of taskExecutions) { if (execution && "getOutput" in execution) { const output = await (execution as | OutputCapturingExecution | ShellOutputCapturingExecution).getOutput(); outputs.push(new vscode.LanguageModelTextPart(output.stdout)); outputs.push(new vscode.LanguageModelTextPart(output.stderr)); } } + if (!continueFlag && outputs.length === 0) { + outputs.push( + new vscode.LanguageModelTextPart( + `Command "${commandName}" did not complete. Check the ESP-IDF output for details.` + ) + ); + } return new vscode.LanguageModelToolResult(outputs); }Also applies to: 276-286
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/langTools/index.ts` around lines 151 - 208, The delegated flows set continueFlag but the function never checks it before returning outputs, so when buildMain/flashMain/buildFlashAndMonitorCapture/eraseFlashMain etc. return continueFlag === false with no executions the language tool may return an empty result; after each call that assigns continueFlag (e.g., results from buildMain, flashMain, bfmResults, eraseFlashResult) check if continueFlag is false and if so return a vscode.LanguageModelToolResult containing a clear explanatory vscode.LanguageModelTextPart (include any captured output/diagnostics from the result when present) instead of proceeding, ensuring the code references the continueFlag and taskExecutions variables for the check and uses the same result objects from those functions.
🧹 Nitpick comments (7)
src/common/store.ts (1)
45-62: Minor: redundant optional chaining in fallback.
workspace.workspaceFolders ? workspace.workspaceFolders?.[0] : undefined— the truthy-guard makes the inner?.redundant. Also, the entire fallback is already whatgetWorkspaceFolder(Uri.parse(storedUri))conceptually returns on miss, so the helper reads a bit convoluted.♻️ Simplified shape
- public getSelectedWorkspaceFolder() { - const fallbackWorkspaceFolder = workspace.workspaceFolders - ? workspace.workspaceFolders?.[0] - : undefined; - const storedUri = this.get<string>( - ExtensionConfigStore.SELECTED_WORKSPACE_FOLDER, - "" - ); - if (storedUri) { - const storedFolder = workspace.getWorkspaceFolder(Uri.parse(storedUri)); - if (!storedFolder) { - this.clear(ExtensionConfigStore.SELECTED_WORKSPACE_FOLDER); - return fallbackWorkspaceFolder; - } - return storedFolder; - } - return fallbackWorkspaceFolder; - } + public getSelectedWorkspaceFolder(): WorkspaceFolder | undefined { + const fallback = workspace.workspaceFolders?.[0]; + const storedUri = this.get<string>( + ExtensionConfigStore.SELECTED_WORKSPACE_FOLDER, + "" + ); + if (!storedUri) return fallback; + const storedFolder = workspace.getWorkspaceFolder(Uri.parse(storedUri)); + if (!storedFolder) { + this.clear(ExtensionConfigStore.SELECTED_WORKSPACE_FOLDER); + return fallback; + } + return storedFolder; + }Note that adding a
WorkspaceFolder | undefinedreturn annotation also helps every downstream caller (e.g.,src/statusBar/index.ts,src/cdtDebugAdapter/debugConfProvider.ts) get accurate type inference without inspecting the body.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/store.ts` around lines 45 - 62, In getSelectedWorkspaceFolder, remove the redundant optional chaining in the fallback (replace workspace.workspaceFolders ? workspace.workspaceFolders?.[0] : undefined with a simpler expression) and simplify the logic so the fallback is directly workspace.workspaceFolders?.[0]; also add an explicit return type annotation WorkspaceFolder | undefined to the getSelectedWorkspaceFolder method and keep the storedUri handling that uses workspace.getWorkspaceFolder(Uri.parse(storedUri)) and ExtensionConfigStore.SELECTED_WORKSPACE_FOLDER intact so callers in src/statusBar/index.ts and src/cdtDebugAdapter/debugConfProvider.ts get correct types.src/cdtDebugAdapter/debugConfProvider.ts (2)
214-221: Logger scope could be more specific per guideline convention.The
Logger.errorscope is the class name only ("CDTDebugConfigurationProvider"). Per coding guidelines, scope identifiers are more useful when they also name the method/operation, e.g."CDTDebugConfigurationProvider resolveDebugConfiguration", to disambiguate against the othercatchsites in this provider.🔧 Proposed fix
- Logger.error(msg, error as Error, "CDTDebugConfigurationProvider"); + Logger.error( + msg, + error as Error, + "CDTDebugConfigurationProvider resolveDebugConfiguration" + );As per coding guidelines: "Use
Logger.error(message, error, \"scope identifier\")for caught errors, with a consistent scope identifier as the third argument (e.g., "extension activate checkFoo")".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cdtDebugAdapter/debugConfProvider.ts` around lines 214 - 221, Change the Logger.error scope to include the method/operation name to follow the guideline: update the call to Logger.error(...) inside the catch block in the CDTDebugConfigurationProvider (the block handling missing build files) to use a more specific scope string such as "CDTDebugConfigurationProvider resolveDebugConfiguration" instead of just "CDTDebugConfigurationProvider"; keep the message and error arguments unchanged and only modify the third argument to the Logger.error invocation to the new scoped identifier.
186-191:|| 2fallback works, but??is safer and more intent-revealing.
idfTargetWatchpointMap[idfTarget as IdfTarget]will be eitherundefined(unknown target) or a positive number — all current values are ≥ 2, so||is correct today, but it also coerces any legitimate0in the future. Switching to??expresses the "unknown target → default 2" intent precisely and is robust to future additions.- String(idfTargetWatchpointMap[idfTarget as IdfTarget] || 2) + String(idfTargetWatchpointMap[idfTarget as IdfTarget] ?? 2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cdtDebugAdapter/debugConfProvider.ts` around lines 186 - 191, Replace the fallback operator in the mapping that builds config.initCommands so the default watchpoint count uses the nullish coalescing operator instead of logical OR: in the block where config.initCommands is mapped, change the expression that reads String(idfTargetWatchpointMap[idfTarget as IdfTarget] || 2) to use ?? (i.e. String(idfTargetWatchpointMap[idfTarget as IdfTarget] ?? 2)), referencing idfTargetWatchpointMap and idfTarget/IdfTarget in the config.initCommands mapping to make the "unknown target → default 2" intent explicit and avoid accidentally treating 0 as absent.src/taskManager.ts (2)
187-272:runTasksPromise constructor + async executor is an antipattern.
new Promise<void>(async (resolve, reject) => { ... })combines two patterns that cancel each other: theasyncexecutor creates an implicit promise whose rejection is discarded, while the outer promise is driven manually viaresolve/reject. Any throw outside the two explicittry/catchblocks (e.g., fromfindIndex, fromTaskManager.tasks[0]access if the array mutates, or a future refactor) will be swallowed and the outer promise will hang forever. The lifecycle tracking viadisposeTaskListener+lastExecutionis otherwise clean.A safer shape is a plain (non-async) executor with a top-level
try { ... } catch { reject; disposeTaskListener(); }wrapper, or refactoring toasync+await new Promise(...)with a separate listener-scoped promise. Not blocking — flagging because this file is the backbone of the refactored build/flash flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/taskManager.ts` around lines 187 - 272, The runTasks function uses a Promise with an async executor which can swallow synchronous throws; change the executor to a plain (non-async) function and wrap the executor body in a top-level try/catch that calls disposeTaskListener(), TaskManager.cancelTasks(), TaskManager.disposeListeners(), and reject(err) on any thrown error, or alternatively refactor runTasks to be async and await a separate Promise that resolves/rejects from the tasks.onDidEndTaskProcess listener; ensure you remove the async keyword from the Promise executor, keep the existing try/catch blocks around tasks.executeTask calls, and reference the same symbols (runTasks, disposeTaskListener, lastExecution, tasks.onDidEndTaskProcess, TaskManager.tasks) so any synchronous exceptions (e.g., from findIndex or TaskManager.tasks[0]) correctly trigger cleanup and rejection.
82-103:throwCapturedTaskFailuresilently no-ops when no execution captures output.If none of the executions in the list has
getOutput(e.g., plainProcessExecution/ShellExecutionentries), the loop completes without throwing. Callers such assrc/eraseFlash/transports/jtag/jtag.tsawait this after asserting the upstream result already failed — when nothing throws they fall through tocontinueFlag: trueeven though the underlying task reported failure (see related comment on that file).Consider documenting this contract explicitly in JSDoc (e.g., "only throws when at least one capturing execution reports failure") so callers don't treat the
awaitas a guaranteed throw on prior failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/taskManager.ts` around lines 82 - 103, Update throwCapturedTaskFailure by adding JSDoc that explicitly documents its contract: state that the function only throws when at least one execution implements getOutput (i.e., is an OutputCapturingExecution or ShellOutputCapturingExecution) and that it will silently no-op if no executions capture output; describe the exact throw conditions (stderr, stdout, or exitCode). Also note in the doc that callers that require a guaranteed exception on any prior failure must either ensure they pass capturing executions or perform an explicit failure check themselves (i.e., do not rely on await throwCapturedTaskFailure to always throw).src/flash/transports/uart/uartFlashCmd.ts (1)
57-58: MissingALREADY_FLASHINGguard — inconsistent with JTAG path.
jtagFlashCommandMainthrowsALREADY_FLASHINGwhenFlashSession.isFlashingis alreadytrue; this function unconditionally sets it totrue. A second caller would clobber the flag, and thefinallyat line 107 would clear it while the first flash is still running, defeating the mutex. Align with JTAG for consistency.🔒 Proposed fix
try { + if (FlashSession.isFlashing) { + throw new Error("ALREADY_FLASHING"); + } FlashSession.isFlashing = true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flash/transports/uart/uartFlashCmd.ts` around lines 57 - 58, Before setting FlashSession.isFlashing to true in uartFlashCommandMain, check the flag and throw the same ALREADY_FLASHING error as jtagFlashCommandMain if it's already true; then set FlashSession.isFlashing = true only after that check and keep the existing finally block that clears the flag to preserve the mutex semantics. Reference FlashSession.isFlashing and the ALREADY_FLASHING error constant and mirror the guard logic used in jtagFlashCommandMain so concurrent callers are prevented from clobbering the flag.src/flash/transports/jtag/flashTclClient.ts (1)
47-107:jtagFlashhas no timeout — can hang indefinitely.The Promise only resolves on
responseorerrorevents fromTCLClient. If OpenOCD accepts the TCP connection but stalls (crash mid-program, stuck on locked hardware, firewalled reply), neither listener ever fires and the flash flow hangs until the caller cancels the task.TCLClient.verifyOpenOCDReadyalready guards itself with a 5s timeout — applying a similar (longer) deadline here would keep the flow responsive.⏱️ Proposed fix — add a timeout
export async function jtagFlash( client: TCLClient, command: string, ...args: string[] ): Promise<CustomExecutionTaskResult> { const fullCommand = `${command} ${args.map(quoteTclArg).join(" ")}`; return new Promise<CustomExecutionTaskResult>((resolve) => { let settled = false; + const timer = setTimeout(() => { + finish({ + continueFlag: false, + executions: [ + createCapturedExecution({ + stdout: "", + stderr: `JTAG flash timed out waiting for OpenOCD response to: ${command}`, + success: false, + exitCode: -1, + }), + ], + }); + }, 120_000); const finish = (result: CustomExecutionTaskResult) => { if (settled) { return; } settled = true; + clearTimeout(timer); client.off("response", onResponse); client.off("error", onError); resolve(result); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flash/transports/jtag/flashTclClient.ts` around lines 47 - 107, jtagFlash can hang indefinitely because it only resolves on TCLClient "response" or "error"; add a deadline timer inside jtagFlash (e.g., 30s) that calls the same finish flow if the timer fires: create a timeout via setTimeout when starting the promise, store its id, and on timeout call finish with continueFlag: false and a CapturedTaskOutput describing the timeout; ensure the timer is cleared (clearTimeout) inside finish and that finish still removes the client listeners (onResponse/onError) as it already does so; reference jtagFlash, client.once/client.off, onResponse, onError, finish, and createCapturedExecution when implementing.
🤖 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/build/buildMain.ts`:
- Around line 61-71: The current non-atomic check of BuildTask.isBuilding can
race; replace the early read of BuildTask.isBuilding with an atomic reservation
using BuildTask.tryReserveBuild() and if tryReserveBuild() returns false perform
the same early return (logging) path; if you take the reservation and later
early-exit (the block that currently returns { continueFlag: false, executions
}) call BuildTask.releaseBuildReservation() before returning so the reservation
is freed; note that buildTask.build() and cleanupBuildState already flip/release
build state so do not change those flows.
In `@src/buildFlashMonitor/index.ts`:
- Around line 121-132: The call site uses resolveFlashTypeForTask which can
return an empty string at runtime despite its non-nullable type; mirror the
defensive pattern used in flashMain by replacing the direct value usage with a
nullish/coalescing fallback (e.g., use resolveFlashTypeForTask(... ) ??
<defaultFlashType>) before passing it into buildFlashAndMonitorCapture; update
the variable flashType in this block so it guarantees a valid default string
when the config is unset, leaving resolvePartitionToUseForTask and
buildFlashAndMonitorCapture calls unchanged.
In `@src/common/store.ts`:
- Around line 36-38: The change to make Store.get<T>(key: string, defaultValue:
T) mandatory breaks existing callers; revert to making defaultValue optional or
add an overload so callers that omit it still compile (e.g. keep public
get<T>(key: string): T | undefined and public get<T>(key: string, defaultValue:
T): T) and delegate to this.ctx.globalState.get<T>(...), or update every call
site of Store.get<T> (such as calls in projectConfiguration.ts, adapterSerial.ts
and rainmaker client) to pass a proper defaultValue of type T; implement the
overload/optional parameter on the get<T> method to preserve prior semantics and
types.
In `@src/common/withProgressWrapper.ts`:
- Around line 75-85: The wrapper can pass an undefined wsFolder into tasks
(wsFolder is set from options?.workspaceFolder or
ESP.GlobalConfiguration.store.getSelectedWorkspaceFolder()), which lets callers
like buildFlashAndMonitor dereference taskWsFolder!.uri and crash; before
calling window.withProgress/task(progress, token, wsFolder) ensure a concrete
workspace folder is available by checking the selected workspace folder and, if
missing, call the existing openFolderCheck or throw/return early with an
error/prompt so task is never invoked with undefined; update the logic around
wsFolder resolution in withProgressWrapper (the wsFolder variable and the call
site of task) to enforce this precondition.
In `@src/eraseFlash/main.ts`:
- Around line 64-73: The JTAG branch logs success even when
eraseFlashCmdResult.continueFlag is false because throwCapturedTaskFailure may
return without throwing; update the block handling ESP.FlashType.JTAG (symbols:
flashType, ESP.FlashType.JTAG, eraseFlashCmdResult, jtagEraseFlashCommand,
throwCapturedTaskFailure, OutputChannel.appendLine, Logger.infoNotify) to
explicitly return or abort when eraseFlashCmdResult.continueFlag is false (e.g.,
check continueFlag after calling jtagEraseFlashCommand and if false either await
throwCapturedTaskFailure(...) and then return eraseFlashCmdResult or skip the
success OutputChannel.appendLine/Logger.infoNotify), so the "JTAG erase flash
finished" message is only emitted when continueFlag is true.
In `@src/eraseFlash/transports/jtag/jtag.ts`:
- Around line 65-76: The current code in the erase flow (around
eraseFlashTelnetCommand, throwCapturedTaskFailure, collectExecutions in jtag.ts)
returns continueFlag: true even when eraseResult.continueFlag is false because
throwCapturedTaskFailure can silently return; change the control flow so that
after calling await throwCapturedTaskFailure(eraseResult.executions) you check
eraseResult.continueFlag and if it is false return { continueFlag: false,
executions: collectExecutions(...eraseResult.executions) } (i.e., return early
with a failure result) instead of falling through to return success; ensure
references to eraseFlashTelnetCommand, throwCapturedTaskFailure, and
collectExecutions are used to locate and update the logic.
- Around line 48-50: Import and call readParameter through the idfConf namespace
(use idfConf.readParameter) instead of the default import, and add explicit type
assertions to match the TCLConnection signature when constructing TCLClient:
assert host as string and port as number (e.g., const host =
idfConf.readParameter("openocd.tcl.host", workspaceFolder) as string; const port
= idfConf.readParameter("openocd.tcl.port", workspaceFolder) as number) so the
TCLClient({ host, port }) call uses correctly typed values; locate usages around
TCLClient and readParameter in this file to apply the changes.
In `@src/espIdf/unitTest/adapter.ts`:
- Around line 112-118: The catch block is logging an incorrect scope ("unitTest
runHandler configurePytestUnitApp") while the code path calls configureUnityApp;
update the Logger.error call in the catch inside the configureUnityApp call site
so the third argument matches the actual function/scope (e.g., "unitTest
runHandler configureUnityApp"), keep the same message and error argument order,
and ensure the change is applied to the Logger.error invocation that currently
appears in the configureUnityApp try/catch.
In `@src/flash/index.ts`:
- Around line 53-58: The code calls
ESP.GlobalConfiguration.store.getSelectedWorkspaceFolder() and uses a non-null
assertion (ws!) inside registerIDFCommand's callback which can be undefined in
multi-root/fresh setups; update the PreCheck.perform callback in
registerIDFCommand("espIdf.selectFlashMethodAndFlash", ...) to guard the
returned value from getSelectedWorkspaceFolder() (or fall back to the first
workspace folder) before calling selectFlashMethod: retrieve const ws =
ESP.GlobalConfiguration.store.getSelectedWorkspaceFolder(); if ws is undefined,
try a fallback like vscode.workspace.workspaceFolders?.[0]; if still undefined,
show a user-facing error/notification and return; otherwise call await
selectFlashMethod(ws.uri). Ensure references: registerIDFCommand,
PreCheck.perform, openFolderCheck,
ESP.GlobalConfiguration.store.getSelectedWorkspaceFolder, and selectFlashMethod
are updated accordingly.
In `@src/flash/shared/errHandling.ts`:
- Around line 87-90: The current ternary sets category to "jtagFlashCommand" for
any non-UART flashType, which misattributes unmapped DFU errors; update the
category assignment (variable: category) to explicitly handle DFU by checking
flashType === ESP.FlashType.DFU and returning "dfuFlashCommand", otherwise
default to "jtagFlashCommand" (or refactor the ternary into an if/else or switch
in the same scope) so unmapped DFU failures are logged under the correct dfu
logger scope.
In `@src/taskManager.ts`:
- Around line 120-134: TaskManager.addTask silently skips adding a task when the
generated taskId (variable taskId) collides with an existing entry (checked via
existingTaskIndex against TaskManager.tasks), which can drop distinct tasks that
slugify to the same string; update addTask to avoid silent drops by either
appending a monotonic counter or a workspace-scoped suffix/hash to taskId (or
include currentWorkspaceFolder identifier) to guarantee uniqueness, and emit a
warning via the existing logger before skipping (or never skip and create a
unique id instead); locate TaskManager.addTask, the taskId construction, and the
existingTaskIndex check to implement this change and ensure callers like
addProcessTask continue to work.
---
Outside diff comments:
In `@src/flash/transports/dfu/helpers.ts`:
- Around line 39-52: getDfuList currently returns whatever
listAvailableDfuDevices(dfuListStr) returns, but listAvailableDfuDevices can
return null when no matches are found; change getDfuList (in helpers.ts) to
normalize that result to an empty array (e.g., use the nullish coalescing
operator on the call to listAvailableDfuDevices) so callers like getDFUArgs can
safely treat the return as string[] and check .length without runtime errors.
In `@src/langTools/index.ts`:
- Around line 151-208: The delegated flows set continueFlag but the function
never checks it before returning outputs, so when
buildMain/flashMain/buildFlashAndMonitorCapture/eraseFlashMain etc. return
continueFlag === false with no executions the language tool may return an empty
result; after each call that assigns continueFlag (e.g., results from buildMain,
flashMain, bfmResults, eraseFlashResult) check if continueFlag is false and if
so return a vscode.LanguageModelToolResult containing a clear explanatory
vscode.LanguageModelTextPart (include any captured output/diagnostics from the
result when present) instead of proceeding, ensuring the code references the
continueFlag and taskExecutions variables for the check and uses the same result
objects from those functions.
---
Nitpick comments:
In `@src/cdtDebugAdapter/debugConfProvider.ts`:
- Around line 214-221: Change the Logger.error scope to include the
method/operation name to follow the guideline: update the call to
Logger.error(...) inside the catch block in the CDTDebugConfigurationProvider
(the block handling missing build files) to use a more specific scope string
such as "CDTDebugConfigurationProvider resolveDebugConfiguration" instead of
just "CDTDebugConfigurationProvider"; keep the message and error arguments
unchanged and only modify the third argument to the Logger.error invocation to
the new scoped identifier.
- Around line 186-191: Replace the fallback operator in the mapping that builds
config.initCommands so the default watchpoint count uses the nullish coalescing
operator instead of logical OR: in the block where config.initCommands is
mapped, change the expression that reads String(idfTargetWatchpointMap[idfTarget
as IdfTarget] || 2) to use ?? (i.e. String(idfTargetWatchpointMap[idfTarget as
IdfTarget] ?? 2)), referencing idfTargetWatchpointMap and idfTarget/IdfTarget in
the config.initCommands mapping to make the "unknown target → default 2" intent
explicit and avoid accidentally treating 0 as absent.
In `@src/common/store.ts`:
- Around line 45-62: In getSelectedWorkspaceFolder, remove the redundant
optional chaining in the fallback (replace workspace.workspaceFolders ?
workspace.workspaceFolders?.[0] : undefined with a simpler expression) and
simplify the logic so the fallback is directly workspace.workspaceFolders?.[0];
also add an explicit return type annotation WorkspaceFolder | undefined to the
getSelectedWorkspaceFolder method and keep the storedUri handling that uses
workspace.getWorkspaceFolder(Uri.parse(storedUri)) and
ExtensionConfigStore.SELECTED_WORKSPACE_FOLDER intact so callers in
src/statusBar/index.ts and src/cdtDebugAdapter/debugConfProvider.ts get correct
types.
In `@src/flash/transports/jtag/flashTclClient.ts`:
- Around line 47-107: jtagFlash can hang indefinitely because it only resolves
on TCLClient "response" or "error"; add a deadline timer inside jtagFlash (e.g.,
30s) that calls the same finish flow if the timer fires: create a timeout via
setTimeout when starting the promise, store its id, and on timeout call finish
with continueFlag: false and a CapturedTaskOutput describing the timeout; ensure
the timer is cleared (clearTimeout) inside finish and that finish still removes
the client listeners (onResponse/onError) as it already does so; reference
jtagFlash, client.once/client.off, onResponse, onError, finish, and
createCapturedExecution when implementing.
In `@src/flash/transports/uart/uartFlashCmd.ts`:
- Around line 57-58: Before setting FlashSession.isFlashing to true in
uartFlashCommandMain, check the flag and throw the same ALREADY_FLASHING error
as jtagFlashCommandMain if it's already true; then set FlashSession.isFlashing =
true only after that check and keep the existing finally block that clears the
flag to preserve the mutex semantics. Reference FlashSession.isFlashing and the
ALREADY_FLASHING error constant and mirror the guard logic used in
jtagFlashCommandMain so concurrent callers are prevented from clobbering the
flag.
In `@src/taskManager.ts`:
- Around line 187-272: The runTasks function uses a Promise with an async
executor which can swallow synchronous throws; change the executor to a plain
(non-async) function and wrap the executor body in a top-level try/catch that
calls disposeTaskListener(), TaskManager.cancelTasks(),
TaskManager.disposeListeners(), and reject(err) on any thrown error, or
alternatively refactor runTasks to be async and await a separate Promise that
resolves/rejects from the tasks.onDidEndTaskProcess listener; ensure you remove
the async keyword from the Promise executor, keep the existing try/catch blocks
around tasks.executeTask calls, and reference the same symbols (runTasks,
disposeTaskListener, lastExecution, tasks.onDidEndTaskProcess,
TaskManager.tasks) so any synchronous exceptions (e.g., from findIndex or
TaskManager.tasks[0]) correctly trigger cleanup and rejection.
- Around line 82-103: Update throwCapturedTaskFailure by adding JSDoc that
explicitly documents its contract: state that the function only throws when at
least one execution implements getOutput (i.e., is an OutputCapturingExecution
or ShellOutputCapturingExecution) and that it will silently no-op if no
executions capture output; describe the exact throw conditions (stderr, stdout,
or exitCode). Also note in the doc that callers that require a guaranteed
exception on any prior failure must either ensure they pass capturing executions
or perform an explicit failure check themselves (i.e., do not rely on await
throwCapturedTaskFailure to always throw).
🪄 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: d49cc196-3902-4931-bdb5-5605e7d800df
📒 Files selected for processing (27)
src/build/buildMain.tssrc/buildFlashMonitor/index.tssrc/cdtDebugAdapter/debugConfProvider.tssrc/cdtDebugAdapter/imageViewPanel.tssrc/common/PreCheck.tssrc/common/registerCommand.tssrc/common/store.tssrc/common/withProgressWrapper.tssrc/eraseFlash/main.tssrc/eraseFlash/transports/jtag/jtag.tssrc/espIdf/hints/openocdhint.tssrc/espIdf/tracing/tree/appTraceArchiveTreeDataProvider.tssrc/espIdf/unitTest/adapter.tssrc/espIdf/unitTest/utils.tssrc/flash/flashProject.tssrc/flash/index.tssrc/flash/main.tssrc/flash/shared/errHandling.tssrc/flash/transports/dfu/dfuFlashExecution.tssrc/flash/transports/dfu/helpers.tssrc/flash/transports/jtag/flashTclClient.tssrc/flash/transports/jtag/jtagCmd.tssrc/flash/transports/uart/uartFlashCmd.tssrc/flash/transports/uart/uartFlashExecution.tssrc/langTools/index.tssrc/statusBar/index.tssrc/taskManager.ts
✅ Files skipped from review due to trivial changes (1)
- src/common/registerCommand.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/cdtDebugAdapter/imageViewPanel.ts
- src/flash/transports/uart/uartFlashExecution.ts
- src/flash/transports/dfu/dfuFlashExecution.ts
- src/flash/flashProject.ts
- src/espIdf/unitTest/utils.ts
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (11)
src/flash/transports/jtag/flashTclClient.ts (1)
34-37: Stale JSDoc after escaping expansion.The comment still says it only escapes backslashes and double quotes, but the implementation also escapes
$and[(per the earlier security fix). Update the description so the doc matches behavior.✏️ Proposed doc update
/** - * Escape backslashes and double quotes, then wrap for OpenOCD TCL double-quoted - * string arguments (prevents breaking out of the quoted token). + * Escape backslashes, double quotes, `$`, and `[`, then wrap for OpenOCD TCL + * double-quoted string arguments (prevents breaking out of the quoted token + * and blocks TCL variable/command substitution). */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flash/transports/jtag/flashTclClient.ts` around lines 34 - 37, Update the JSDoc that currently states the function only escapes backslashes and double quotes to reflect the actual escaping behavior (it also escapes '$' and '['); edit the comment above the escaping routine (the JSDoc for the escapeArg/escapeTclArg function used to build OpenOCD TCL double-quoted string arguments) to list all escaped characters and purpose (backslash, double quote, dollar sign, and left bracket) so the description matches the implementation.src/build/validation.ts (1)
32-35: Add an explicit return type annotation.
runValidationBeforeBuildis exported and returns{ cmakeBin, ninjaBin }, but both are implicitly typed asstring(sinceisBinInPathresolves to a string). DeclaringPromise<{ cmakeBin: string; ninjaBin: string }>makes the public surface explicit and prevents theanythat the AI summary hints at from leaking into callers.♻️ Proposed refactor
export async function runValidationBeforeBuild( envVariables: NodeJS.ProcessEnv, currentWorkspace: Uri -) { +): Promise<{ cmakeBin: string; ninjaBin: string }> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/build/validation.ts` around lines 32 - 35, The exported function runValidationBeforeBuild should have an explicit return type to make its public API clear; update its signature to return Promise<{ cmakeBin: string; ninjaBin: string }> (instead of relying on inference from isBinInPath) so callers don't get implicit any types, and ensure any internal awaits/returns conform to that shape (refer to runValidationBeforeBuild and the values cmakeBin and ninjaBin).src/cdtDebugAdapter/debugConfProvider.ts (1)
84-88: Consider restoring an explicit return type.
resolveDebugConfigurationcan either returnconfigor implicitly returnundefinedfrom thecatchbranch (Line 220). Without a return type annotation, the inferred type isPromise<DebugConfiguration | undefined>, which is fine, but making that explicit helps document the fact that VS Code may receiveundefined(and will then abort the debug session) and prevents accidental narrowing regressions when this function is edited later.♻️ Proposed refactor
public async resolveDebugConfiguration( folder: WorkspaceFolder | undefined, config: DebugConfiguration, token?: CancellationToken - ) { + ): Promise<DebugConfiguration | undefined> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cdtDebugAdapter/debugConfProvider.ts` around lines 84 - 88, Add an explicit return type to resolveDebugConfiguration: declare it as returning Promise<DebugConfiguration | undefined> so callers and future edits make the optional undefined case explicit; ensure the function signature for resolveDebugConfiguration (the method currently declared async resolveDebugConfiguration(folder, config, token?)) includes this return type and that the success path returns config while the catch path returns undefined (or explicitly returns void) to match the annotated union.src/taskManager.ts (2)
280-285: Redundant self-assignment.
TaskManager.tasks.splice(...)at Lines 262–264 already removes the finished task, and this branch is only entered after that splice leaves the array empty. TheTaskManager.tasks = []on Line 281 is a no-op.♻️ Minor cleanup
if (TaskManager.tasks.length === 0) { - TaskManager.tasks = []; disposeTaskListener(); resolve(); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/taskManager.ts` around lines 280 - 285, Remove the redundant self-assignment TaskManager.tasks = [] inside the empty-array branch; since TaskManager.tasks is already emptied by the preceding splice (and this branch is only reached when length === 0), simply call disposeTaskListener() and resolve() and return. Update the block in taskManager where the code checks if (TaskManager.tasks.length === 0) to eliminate the no-op assignment while keeping disposeTaskListener() and resolve() intact.
83-103: Trim the JSDoc per coding guidelines.The block on Lines 83–103 is ~20 lines of narrative describing the no-op semantics and throw priorities. As per coding guidelines, "Keep JSDoc proportional; if it grows beyond ~8–10 lines, trim it or move narrative to
docs_espressif/when appropriate", and "Prefer self-documenting code (names + small helpers) over comments". The control flow inside the function already makes the throw order obvious; the caller-contract caveat is the only part worth keeping inline.♻️ Proposed refactor
/** - * Inspects task executions that support captured process output and throws if - * any report failure. Executions are considered only when they implement - * {`@link` OutputCapturingExecution.getOutput} / {`@link` ShellOutputCapturingExecution.getOutput} - * (duck-typed via `"getOutput" in execution`); plain `ProcessExecution`, - * `ShellExecution`, etc. are skipped. - * - * **No-op:** If every entry is missing, undefined, or has no `getOutput`, or if - * every `getOutput()` result is missing or has `success: true`, this function - * returns normally without throwing. - * - * **Throw conditions** (first failed `getOutput()` wins, in order): - * 1. If `!executionOutput.success` and `stderr` has non-whitespace content → throws `Error` with that stderr string. - * 2. Else if `stdout` has non-whitespace content → throws `Error` with that stdout string. - * 3. Else → throws `Error` with message ``Task exited with code ${exitCode}``. - * - * Callers that must always surface a prior command failure as an exception - * cannot rely solely on `await throwCapturedTaskFailure(...)`: they must pass - * at least one output-capturing execution, or perform their own check on - * `continueFlag` / task results before or after calling this function. + * Throws the first captured failure among output-capturing executions. + * Non-capturing executions (plain ProcessExecution/ShellExecution/CustomExecution) + * are skipped, so callers must independently check `continueFlag` / task + * results when no capturing execution is guaranteed to be present. */As per coding guidelines: "Keep JSDoc proportional; if it grows beyond ~8–10 lines, trim it or move narrative to
docs_espressif/when appropriate".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/taskManager.ts` around lines 83 - 103, Trim the oversized JSDoc above the throwCapturedTaskFailure function to a concise summary: one-line purpose, one-line behavior note, and keep the caller-contract caveat about callers needing at least one output-capturing execution (or to check continueFlag/task results) — remove the long throw-priority/no-op narrative and move detailed semantics to docs_espressif/ or an internal docs file if needed.src/eraseFlash/transports/jtag/jtag.ts (1)
56-63: Retry loop uses a bareforloop with hidden side effects.Minor readability nit: the
breakonisReadyinside a counter loop interleaves control flow with thesetTimeoutdelay. Extracting a small helper or usingwhile (!isReady && attempt < 3)reads more directly. Also consider logging each failed attempt to aid diagnosis when OpenOCD is slow to come up.for (let attempt = 0; !isReady && attempt < 3; attempt++) { if (attempt > 0) { await new Promise((resolve) => setTimeout(resolve, 1000)); } isReady = await client.verifyOpenOCDReady(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/eraseFlash/transports/jtag/jtag.ts` around lines 56 - 63, The retry loop using a bare for with an internal break makes control flow and the delay order unclear; refactor the check around TCLClient.verifyOpenOCDReady() into a clearer loop (e.g., while(!isReady && attempts < 3) or for with the delay before subsequent attempts) and remove the internal break; also add a per-attempt log via your logger (or console) indicating the attempt number and failure to help diagnose slow OpenOCD startup. Ensure you update references around the existing isReady variable and the TCLClient instance so the behavior and maximum 3 attempts remain unchanged.src/build/dfuExecution.ts (1)
44-55: Target-allowlist duplication.The allowlist
esp32s2/esp32s3is encoded here (Lines 46–47) and also inselectedDFUAdapterIdatsrc/flash/transports/dfu/helpers.tsLines 63–77. If a new DFU-capable target is added (or an existing one is removed), both places must be kept in sync, orselectedDFUAdapterId(adapterTargetName)on Line 70 will throw for an allowed-but-unmapped target / the allow-list here will silently gate a supported target.Consider deriving support from
selectedDFUAdapterId(e.g., wrap it in try/catch or expose aisDFUSupportedTarget(chip)helper) so the list of supported chips has a single source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/build/dfuExecution.ts` around lines 44 - 55, The allowlist for DFU-capable targets is duplicated; replace the hardcoded check of adapterTargetName in dfuExecution.ts with a single source of truth from the DFU helpers: either call selectedDFUAdapterId(adapterTargetName) wrapped in try/catch and treat any error as "unsupported", or add and call a new isDFUSupportedTarget(chip) helper in src/flash/transports/dfu/helpers.ts that encapsulates the allowed targets logic; update dfuExecution.ts to use that helper (or the try/catch around selectedDFUAdapterId) so supported targets are maintained in one place and no longer duplicated.src/flash/index.ts (1)
54-63: Drop the redundant non-null assertion.After the
if (!ws) { … return; }guard on lines 57-60, TypeScript already narrowswsto non-nullable. Thews!on line 61 is noise.♻️ Proposed tweak
- await selectFlashMethod(ws!.uri); + await selectFlashMethod(ws.uri);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flash/index.ts` around lines 54 - 63, The non-null assertion on ws is redundant after the guard; in the registerIDFCommand callback remove the unnecessary `!` when calling selectFlashMethod so call selectFlashMethod(ws.uri) instead of selectFlashMethod(ws!.uri), keeping the existing if (!ws) { Logger.infoNotify(...); return; } guard that narrows ws to non-null before use (symbols: registerIDFCommand, selectFlashMethod, ws).src/build/buildMain.ts (1)
38-48: Fix misleading@returnsand trim trivial JSDoc.The
@returnsline claimstrue/false, butbuildMainreturns aCustomExecutionTaskResultobject ({ continueFlag, executions }). Also, per the repo's JSDoc guideline, individual@paramentries that just restate the parameter name/type ("The workspace folder URI", "The cancellation token") add noise — keep the intent/behavior prose and drop the rest.📝 Proposed tweak
/** - * Build the project with the given parameters. - * - * It will build the project, run the size task, and if flashType is DFU, it will append the DFU execution. - * - * `@param` workspace - The workspace folder URI - * `@param` cancelToken - The cancellation token - * `@param` flashType - The flash type - * `@param` buildType - The build type - * `@returns` true if the build is successful, false otherwise + * Runs the build pipeline: pre-build tasks → build → optional DFU append → post-build → + * optional size task. Gates concurrent execution against `FlashSession.isFlashing` and + * `BuildTask.tryReserveBuild()`. */As per coding guidelines: Avoid redundant JSDoc documentation (e.g., type and function docs restating the same concept).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/build/buildMain.ts` around lines 38 - 48, The JSDoc for buildMain is misleading and verbose: remove trivial `@param` descriptions that merely restate parameter names/types (drop lines like "The workspace folder URI" and "The cancellation token") and replace the `@returns` line to accurately describe the actual return value (CustomExecutionTaskResult object containing continueFlag and executions) rather than "true/false"; update the prose summary to focus on behavior (what buildMain does) and ensure the `@returns` describes the returned object's shape and semantics referencing CustomExecutionTaskResult and fields continueFlag and executions.src/build/index.ts (1)
27-41: Wrap thebuildhandler to avoid forwarding command-invocation args, and drop the needlessasync.
- Line 27:
registerBuildCommandsisasyncbut never awaits — misleading to callers.- Line 28:
registerIDFCommand(context, "espIdf.buildDevice", build)passesbuilddirectly. Any caller doingcommands.executeCommand("espIdf.buildDevice", someArg)will seesomeArgsilently forwarded asflashType, which then propagates throughbuildMainand downstream logic (includingflashType === ESP.FlashType.DFUchecks). All the sibling registrations on lines 29-40 already wrap in() => build(...); align this one too.🛡️ Proposed fix
-export async function registerBuildCommands(context: ExtensionContext) { - registerIDFCommand(context, "espIdf.buildDevice", build); +export function registerBuildCommands(context: ExtensionContext) { + registerIDFCommand(context, "espIdf.buildDevice", () => build()); registerIDFCommand(context, "espIdf.buildDFU", () =>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/build/index.ts` around lines 27 - 41, registerBuildCommands is marked async but never awaits and, more importantly, passes the build function directly to registerIDFCommand which causes any command-invocation args to be forwarded as flashType; change registerBuildCommands to a non-async function and wrap the device registration so it uses a no-arg wrapper like the other registrations (i.e., replace registerIDFCommand(context, "espIdf.buildDevice", build) with a wrapper that calls build() with no parameters), keeping the other wrapped registrations for buildDFU, buildApp, buildBootloader, and buildPartitionTable unchanged.src/flash/transports/uart/uartFlashCmd.ts (1)
54-59: Merge the two cancellation handlers.Two back-to-back
cancelToken.onCancellationRequestedlisteners doing unrelated cleanup are functionally fine but harder to read and each leaks an extraDisposable. Consider a single handler.♻️ Proposed tweak
- cancelToken.onCancellationRequested(() => { - TaskManager.cancelTasks(); - }); - cancelToken.onCancellationRequested(() => { - FlashSession.isFlashing = false; - }); + cancelToken.onCancellationRequested(() => { + TaskManager.cancelTasks(); + FlashSession.isFlashing = false; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flash/transports/uart/uartFlashCmd.ts` around lines 54 - 59, Two adjacent cancelToken.onCancellationRequested listeners should be merged into a single handler to avoid extra Disposables and improve readability; replace the two handlers with one cancelToken.onCancellationRequested callback that calls TaskManager.cancelTasks() and then sets FlashSession.isFlashing = false in the same function, keeping the existing behavior and disposing only one listener (refer to cancelToken.onCancellationRequested, TaskManager.cancelTasks, and FlashSession.isFlashing to locate the code).
🤖 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/build/buildHelpers.ts`:
- Around line 22-55: In applySdkconfigDefaultsAndCcacheArgs: remove the
redundant second disjuncts in the arg existence checks (only use
a.startsWith("-DSDKCONFIG=") and a.startsWith("-DSDKCONFIG_DEFAULTS=")), stop
wrapping injected values in literal single quotes (push
-DSDKCONFIG=${sdkconfigFile} and
-DSDKCONFIG_DEFAULTS=${sdkconfigDefaults.join(";")} without quotes), and drop
the unnecessary args.length guard so -DCCACHE_ENABLE=1 is added whenever
enableCCache is true (use args.indexOf("-DCCACHE_ENABLE=1") as currently
written). Also update the unit tests in build.test.ts to expect unquoted
SDKCONFIG and SDKCONFIG_DEFAULTS values (e.g. "-DSDKCONFIG=/ws/sdkconfig"
instead of "-DSDKCONFIG='/ws/sdkconfig'").
In `@src/buildFlashMonitor/index.ts`:
- Around line 115-117: The progress label passed to withProgressWrapper is
incomplete ("ESP-IDF:") and should be replaced with a full, user-facing title
for the build+flash+monitor sequence; update the call in index.ts (the
withProgressWrapper invocation that wraps openFolderCheck) to use a complete
string such as "ESP-IDF: Flashing project" (or the consistent title used
elsewhere like "ESP-IDF: Building project" if more appropriate) so the progress
UI shows a meaningful description.
In `@src/cdtDebugAdapter/debugConfProvider.ts`:
- Around line 214-221: The catch block in resolveDebugConfiguration currently
sets a misleading default message when the caught value isn't an Error; instead
of claiming missing build files, stringify the unknown thrown value and use that
as the message (e.g., via String(error) or JSON.stringify fallback) so
Logger.error receives the actual data; update the ternary that sets msg to
handle Error instances as before and otherwise produce a clear stringified
representation, then pass that into Logger.error (keep the existing context
string "CDTDebugConfigurationProvider resolveDebugConfiguration").
In `@src/common/store.ts`:
- Around line 55-68: The getSelectedWorkspaceFolder method currently calls
Uri.parse(storedUri) without guarding against malformed storedUri; wrap the
parse in a try/catch so that if Uri.parse throws you call
this.clear(ExtensionConfigStore.SELECTED_WORKSPACE_FOLDER) and return the
fallback (same behavior as when workspace.getWorkspaceFolder(...) returns
undefined). Locate the logic in getSelectedWorkspaceFolder, protect the
Uri.parse(storedUri) call, and ensure any parse error is handled by clearing the
stored key and returning the fallback WorkspaceFolder.
In `@src/flash/shared/errHandling.ts`:
- Around line 39-46: The current mappings in errHandling.ts rely on the exact
string "Task ESP-IDF Flash exited with code 74", which breaks if the task name
changes; update the matching logic to match on the exit code instead (e.g., test
the error message against a regex like /exited with code 74$/) or, better,
detect and use e.exitCode if available from TaskManager.runTasks()/lastExecution
(see TaskManager.runTasks and TaskManager.addTask where lastExecution.task.name
and e.exitCode are produced) and then map exitCode === 74 to the userMessage "No
DFU capable USB device available found" (keep the same loggerScope/notify).
Ensure you replace the exact-map entry with this exitCode-based check so
renaming the task won't break the mapping.
In `@src/flash/transports/jtag/flashTclClient.ts`:
- Around line 73-78: The onResponse handler currently uses
String.prototype.replace with TCLClient.DELIMITER which only removes the first
delimiter byte and can leave stray '\x1a' characters in the response, breaking
the success check in onResponse; change the sanitizer to remove all delimiter
occurrences (e.g., use replaceAll(TCLClient.DELIMITER, "") or a global-regex
replace) so the response string used by onResponse === "0" and any emitted
stdout is clean; ensure this change aligns with sendCommand/flushBuffer behavior
that may accumulate multiple delimiters.
In `@src/taskManager.ts`:
- Around line 136-140: The TaskManager.taskResults declaration doesn't match
what is actually pushed (objects with taskId, exitCode, and taskName); update
the static declaration of taskResults to include exitCode?: number and
taskName?: string (you can also keep output?: any and error?: Error optional) so
the stored shape matches pushes (e.g., change TaskManager.taskResults to Array<{
taskId: string; exitCode?: number; taskName?: string; output?: any; error?:
Error }>).
---
Nitpick comments:
In `@src/build/buildMain.ts`:
- Around line 38-48: The JSDoc for buildMain is misleading and verbose: remove
trivial `@param` descriptions that merely restate parameter names/types (drop
lines like "The workspace folder URI" and "The cancellation token") and replace
the `@returns` line to accurately describe the actual return value
(CustomExecutionTaskResult object containing continueFlag and executions) rather
than "true/false"; update the prose summary to focus on behavior (what buildMain
does) and ensure the `@returns` describes the returned object's shape and
semantics referencing CustomExecutionTaskResult and fields continueFlag and
executions.
In `@src/build/dfuExecution.ts`:
- Around line 44-55: The allowlist for DFU-capable targets is duplicated;
replace the hardcoded check of adapterTargetName in dfuExecution.ts with a
single source of truth from the DFU helpers: either call
selectedDFUAdapterId(adapterTargetName) wrapped in try/catch and treat any error
as "unsupported", or add and call a new isDFUSupportedTarget(chip) helper in
src/flash/transports/dfu/helpers.ts that encapsulates the allowed targets logic;
update dfuExecution.ts to use that helper (or the try/catch around
selectedDFUAdapterId) so supported targets are maintained in one place and no
longer duplicated.
In `@src/build/index.ts`:
- Around line 27-41: registerBuildCommands is marked async but never awaits and,
more importantly, passes the build function directly to registerIDFCommand which
causes any command-invocation args to be forwarded as flashType; change
registerBuildCommands to a non-async function and wrap the device registration
so it uses a no-arg wrapper like the other registrations (i.e., replace
registerIDFCommand(context, "espIdf.buildDevice", build) with a wrapper that
calls build() with no parameters), keeping the other wrapped registrations for
buildDFU, buildApp, buildBootloader, and buildPartitionTable unchanged.
In `@src/build/validation.ts`:
- Around line 32-35: The exported function runValidationBeforeBuild should have
an explicit return type to make its public API clear; update its signature to
return Promise<{ cmakeBin: string; ninjaBin: string }> (instead of relying on
inference from isBinInPath) so callers don't get implicit any types, and ensure
any internal awaits/returns conform to that shape (refer to
runValidationBeforeBuild and the values cmakeBin and ninjaBin).
In `@src/cdtDebugAdapter/debugConfProvider.ts`:
- Around line 84-88: Add an explicit return type to resolveDebugConfiguration:
declare it as returning Promise<DebugConfiguration | undefined> so callers and
future edits make the optional undefined case explicit; ensure the function
signature for resolveDebugConfiguration (the method currently declared async
resolveDebugConfiguration(folder, config, token?)) includes this return type and
that the success path returns config while the catch path returns undefined (or
explicitly returns void) to match the annotated union.
In `@src/eraseFlash/transports/jtag/jtag.ts`:
- Around line 56-63: The retry loop using a bare for with an internal break
makes control flow and the delay order unclear; refactor the check around
TCLClient.verifyOpenOCDReady() into a clearer loop (e.g., while(!isReady &&
attempts < 3) or for with the delay before subsequent attempts) and remove the
internal break; also add a per-attempt log via your logger (or console)
indicating the attempt number and failure to help diagnose slow OpenOCD startup.
Ensure you update references around the existing isReady variable and the
TCLClient instance so the behavior and maximum 3 attempts remain unchanged.
In `@src/flash/index.ts`:
- Around line 54-63: The non-null assertion on ws is redundant after the guard;
in the registerIDFCommand callback remove the unnecessary `!` when calling
selectFlashMethod so call selectFlashMethod(ws.uri) instead of
selectFlashMethod(ws!.uri), keeping the existing if (!ws) {
Logger.infoNotify(...); return; } guard that narrows ws to non-null before use
(symbols: registerIDFCommand, selectFlashMethod, ws).
In `@src/flash/transports/jtag/flashTclClient.ts`:
- Around line 34-37: Update the JSDoc that currently states the function only
escapes backslashes and double quotes to reflect the actual escaping behavior
(it also escapes '$' and '['); edit the comment above the escaping routine (the
JSDoc for the escapeArg/escapeTclArg function used to build OpenOCD TCL
double-quoted string arguments) to list all escaped characters and purpose
(backslash, double quote, dollar sign, and left bracket) so the description
matches the implementation.
In `@src/flash/transports/uart/uartFlashCmd.ts`:
- Around line 54-59: Two adjacent cancelToken.onCancellationRequested listeners
should be merged into a single handler to avoid extra Disposables and improve
readability; replace the two handlers with one
cancelToken.onCancellationRequested callback that calls
TaskManager.cancelTasks() and then sets FlashSession.isFlashing = false in the
same function, keeping the existing behavior and disposing only one listener
(refer to cancelToken.onCancellationRequested, TaskManager.cancelTasks, and
FlashSession.isFlashing to locate the code).
In `@src/taskManager.ts`:
- Around line 280-285: Remove the redundant self-assignment TaskManager.tasks =
[] inside the empty-array branch; since TaskManager.tasks is already emptied by
the preceding splice (and this branch is only reached when length === 0), simply
call disposeTaskListener() and resolve() and return. Update the block in
taskManager where the code checks if (TaskManager.tasks.length === 0) to
eliminate the no-op assignment while keeping disposeTaskListener() and resolve()
intact.
- Around line 83-103: Trim the oversized JSDoc above the
throwCapturedTaskFailure function to a concise summary: one-line purpose,
one-line behavior note, and keep the caller-contract caveat about callers
needing at least one output-capturing execution (or to check continueFlag/task
results) — remove the long throw-priority/no-op narrative and move detailed
semantics to docs_espressif/ or an internal docs file if needed.
🪄 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: 86e83b56-3050-4a57-9394-49362440fe37
📒 Files selected for processing (20)
src/build/buildHelpers.tssrc/build/buildMain.tssrc/build/buildTask.tssrc/build/dfuExecution.tssrc/build/index.tssrc/build/validation.tssrc/buildFlashMonitor/index.tssrc/cdtDebugAdapter/debugConfProvider.tssrc/common/store.tssrc/common/withProgressWrapper.tssrc/eraseFlash/index.tssrc/eraseFlash/main.tssrc/eraseFlash/transports/jtag/jtag.tssrc/espIdf/unitTest/adapter.tssrc/flash/flashProject.tssrc/flash/index.tssrc/flash/shared/errHandling.tssrc/flash/transports/jtag/flashTclClient.tssrc/flash/transports/uart/uartFlashCmd.tssrc/taskManager.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/eraseFlash/index.ts
- src/espIdf/unitTest/adapter.ts
- src/build/buildTask.ts
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)
src/langTools/index.ts (1)
152-362:⚠️ Potential issue | 🟠 MajorDead flash error branches and unused
continueFlag— consider propagating failure status to the LM tool or removing unreachable handlers.Verification confirms:
Flash errors are caught and swallowed inside
flashMain(src/flash/main.ts lines 120+):handleFlashCommandCatchlogs the error but does not re-throw; the function returns{ continueFlag: false, executions: [] }instead. This means the error branches at lines 312 (ALREADY_FLASHING), 319 (NO_DFU_DEVICE_SELECTED), 324 (isFlashRelatedTaskExitCode74), 331 (FLASH_TERMINATED), 336 (SECTION_BIN_FILE_NOT_ACCESSIBLE), and 343–354 (ENOENT/SCRIPT_PERMISSION_ERROR) in langTools will never execute for flash/buildFlashMonitor commands.
ALREADY_ERASINGescapes the try block (src/eraseFlash/main.ts line 40, before the try block at line 43): it reaches the generic error handler at line 355 instead of a purpose-built message likeALREADY_FLASHINGreceives.
continueFlagis never read after assignment (lines 166, 177, 199, 208). The output loop (lines 277–288) unconditionally returns captured stdout/stderr regardless ofcontinueFlagvalue, so the LM tool cannot distinguish between successful and failed task phases.Recommendation: Either (a) remove the flash-specific and unreachable error branches to avoid confusing future maintainers, or (b) propagate
continueFlaginto the tool result (e.g., prepend a status message when!continueFlag) and add anALREADY_ERASINGhandler so the LM consistently receives a failure signal instead of only raw task output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/langTools/index.ts` around lines 152 - 362, The tool currently has unreachable flash-specific error branches because flashMain/buildFlashAndMonitorCapture swallow errors and return continueFlag=false; fix by propagating failure status into the LanguageModelToolResult and adding the missing ALREADY_ERASING handler: after running command helpers (flashMain, buildMain, buildFlashAndMonitorCapture, eraseFlashMain) check continueFlag and if false prepend or return a clear status LanguageModelTextPart (e.g., "Command failed" or specific failure text) instead of returning raw outputs; also add a catch branch for errorMessage === "ALREADY_ERASING" (similar style to ALREADY_FLASHING) so eraseFlashMain failures produce a consistent LM message; finally, ensure the TASK_COMMANDS output loop only returns stdout/stderr when continueFlag is true (or include the failure status when false) so the LM can distinguish success vs failure (update references: continueFlag, taskExecutions, flashMain, buildFlashAndMonitorCapture, eraseFlashMain, ALREADY_ERASING, TASK_COMMANDS).
🧹 Nitpick comments (7)
src/cdtDebugAdapter/debugConfProvider.ts (2)
48-58: Duplicated workspace-folder resolution block.The same 10-line resolution sequence (
getSelectedWorkspaceFolder→showWorkspaceFolderPick→ throw) now appears verbatim in bothresolveDebugConfigurationWithSubstitutedVariablesandresolveDebugConfiguration. Extracting it to a small helper would DRY this up and keep future changes (e.g., adding telemetry or remembering the pick) in a single place.♻️ Suggested refactor
+ private async pickOrResolveWorkspaceFolder( + folder: WorkspaceFolder | undefined + ): Promise<WorkspaceFolder> { + if (folder) return folder; + const stored = ESP.GlobalConfiguration.store.getSelectedWorkspaceFolder(); + if (stored) return stored; + const picked = await window.showWorkspaceFolderPick({ + placeHolder: "Pick a workspace folder to start a debug session.", + }); + if (!picked) { + throw new Error("No folder was selected to start debug session"); + } + return picked; + }Also applies to: 90-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cdtDebugAdapter/debugConfProvider.ts` around lines 48 - 58, The workspace-folder resolution sequence is duplicated in resolveDebugConfigurationWithSubstitutedVariables and resolveDebugConfiguration; extract it into a small async helper (e.g., getOrPickWorkspaceFolder) that calls ESP.GlobalConfiguration.store.getSelectedWorkspaceFolder(), falls back to window.showWorkspaceFolderPick(...) with the same placeholder, and throws the existing error if no folder is returned; replace both occurrences in resolveDebugConfigurationWithSubstitutedVariables and resolveDebugConfiguration with a call to this helper so future changes (telemetry, caching) live in one place.
158-192: Minor:idfTargettype coercion has a hidden fallback path.
getIdfTargetFromSdkconfigreturns an arbitrary string (e.g., whenIDF_TARGETis unset or custom). Theas IdfTargetcast on Line 189 bypasses the compiler and relies solely on the?? 2fallback. Defaulting to2silently for a newly introduced target (e.g., a futureesp32xx) may set a wrong watchpoint limit without any signal.Consider logging a debug message when the target is missing from
idfTargetWatchpointMapso the fallback is traceable during triage:♻️ Suggested refactor
- config.initCommands = config.initCommands.map((cmd: string) => - cmd.replace( - "{IDF_TARGET_CPU_WATCHPOINT_NUM}", - String(idfTargetWatchpointMap[idfTarget as IdfTarget] ?? 2) - ) - ); + const watchpointNum = + idfTargetWatchpointMap[idfTarget as IdfTarget] ?? 2; + if (!(idfTarget in idfTargetWatchpointMap)) { + Logger.info( + `Unknown IDF target "${idfTarget}"; defaulting CPU watchpoint count to ${watchpointNum}.` + ); + } + config.initCommands = config.initCommands.map((cmd: string) => + cmd.replace("{IDF_TARGET_CPU_WATCHPOINT_NUM}", String(watchpointNum)) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cdtDebugAdapter/debugConfProvider.ts` around lines 158 - 192, The code silently falls back to 2 when idfTarget is not a known key; after calling getIdfTargetFromSdkconfig and before replacing tokens in config.initCommands, check idfTargetWatchpointMap for the key (don't just cast with as IdfTarget), compute const watchpointNum = idfTargetWatchpointMap[idfTarget as any]; if watchpointNum is undefined log a debug/warning message including the raw idfTarget and that you're using the default 2 (use the module's existing logger or console.debug if none), then use the fallback value; finally replace "{IDF_TARGET_CPU_WATCHPOINT_NUM}" with String(watchpointNum ?? 2). This ensures getIdfTargetFromSdkconfig, idfTargetWatchpointMap, and the mapping logic are explicitly checked and any unexpected target is traceable.src/flash/transports/jtag/flashTclClient.ts (2)
96-111: Consider logging the raw error for diagnostics.When
erris not anErrorinstance, the original value is replaced with a generic fallback message and discarded. This makes failures in the OpenOCD TCL path hard to debug from logs. Consider forwarding the raw error throughLogger.errorwith a scope like"jtagFlash onError"before producing thestderrstring, so the original context is preserved even though the promise still resolves with a cleanCapturedTaskOutput.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flash/transports/jtag/flashTclClient.ts` around lines 96 - 111, The onError handler currently discards non-Error values and only returns a generic stderr; update onError to log the raw err for diagnostics (e.g., call Logger.error(err, "jtagFlash onError") or the module's logger) before building the CapturedTaskOutput, then keep constructing the same output object and call finish(...) with createCapturedExecution(output) as before; reference the onError function, CapturedTaskOutput, createCapturedExecution and finish so you add only the logging step without changing the returned output shape or control flow.
26-32: Minor: avoid theas unknown as IdfTaskExecutiondouble cast.Not all members of the
IdfTaskExecutionunion type (ShellExecution,ProcessExecution,CustomExecutionfrom VS Code) have agetOutput()method. The created object only satisfies the methods required bythrowCapturedTaskFailure, which explicitly checks"getOutput" in execution. Consider creating a narrower interface (e.g.,CaptureableExecutionwith onlygetOutput()) or restructuring the union to avoid the double cast workaround.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flash/transports/jtag/flashTclClient.ts` around lines 26 - 32, The function createCapturedExecution currently uses a double-cast to IdfTaskExecution to satisfy the union type; instead define and use a narrower interface (e.g., CaptureableExecution) that only includes getOutput(), update createCapturedExecution to return that narrower type, and update any callers (such as throwCapturedTaskFailure) to accept the narrower CaptureableExecution or perform a type guard ("getOutput" in execution) before treating it as IdfTaskExecution; reference createCapturedExecution, IdfTaskExecution, getOutput, and throwCapturedTaskFailure when making these changes.src/taskManager.ts (2)
263-279:cancelTasks()inside end-listener iterates an already-mutated queue.On non-zero exit, the current task has already been spliced out at lines 254–261 (the
tasks.onDidEndTaskProcessfires after the task ended), soTaskManager.cancelTasks()on line 268 only iterates remaining queued tasks that have never been started—eachtasks.taskExecutions.find(...)will returnundefined, making the call effectively a noop that just setsTaskManager.tasks = []. This is fine today but the intent reads as "terminate the still-running task"; adding a brief comment or replacing withTaskManager.tasks = []; TaskManager.disposeListeners();would remove the ambiguity and avoid misleading future readers. The existing comment on lines 264–266 already acknowledges "terminating here is unnecessary" — consider making the cleanup explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/taskManager.ts` around lines 263 - 279, The end-listener currently calls TaskManager.cancelTasks(), which iterates the task queue but the current task was already removed by tasks.onDidEndTaskProcess, making cancelTasks() a noop and confusing to readers; replace that call with an explicit TaskManager.tasks = [] (to clear any remaining queued tasks) and keep TaskManager.disposeListeners() and disposeTaskListener(), or at minimum add a clarifying comment near the dispose/reject block referencing tasks.onDidEndTaskProcess and why cancelTasks() is not used — update the code paths around TaskManager.cancelTasks(), TaskManager.tasks, TaskManager.disposeListeners(), disposeTaskListener(), and the error/reject logic for lastExecution to reflect the explicit cleanup.
184-189: Use??for boolean presentation overrides.
presentationOptions?.showReuseMessage || falseandpresentationOptions?.clear || falsecoerce an explicittruecorrectly but cannot distinguish an explicitfalsefrom an unset value. Today it happens to yield the desired default, but if the defaults ever change totrue, callers passingfalsewill be silently overridden. Prefer nullish coalescing (?? false) so caller intent is preserved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/taskManager.ts` around lines 184 - 189, The boolean presentation overrides in newTask.presentationOptions currently use logical OR which treats an explicit false as unset; update the assignments for showReuseMessage and clear to use nullish coalescing so caller false is preserved (i.e., replace presentationOptions?.showReuseMessage || false and presentationOptions?.clear || false with presentationOptions?.showReuseMessage ?? false and presentationOptions?.clear ?? false) while leaving reveal and panel handling unchanged.src/build/dfuExecution.ts (1)
32-32: PreferIdfTaskExecution[]overExclude<MaybeIdfTaskExecution, undefined>[].
Exclude<MaybeIdfTaskExecution, undefined>resolves exactly toIdfTaskExecutionsinceMaybeIdfTaskExecution = IdfTaskExecution | undefined. Importing and usingIdfTaskExecutiondirectly is clearer and avoids the roundtrip through a utility type.♻️ Proposed refactor
-import { addProcessTask, type MaybeIdfTaskExecution } from "../taskManager"; +import { addProcessTask, type IdfTaskExecution } from "../taskManager"; @@ export async function appendDfuExecution( - executions: Exclude<MaybeIdfTaskExecution, undefined>[], + executions: IdfTaskExecution[], workspace: Uri, captureOutput?: boolean ): Promise<boolean> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/build/dfuExecution.ts` at line 32, The parameter type for "executions" uses Exclude<MaybeIdfTaskExecution, undefined>[] which is equivalent to IdfTaskExecution[]; change the signature to use IdfTaskExecution[] directly, update any imports to reference IdfTaskExecution instead of MaybeIdfTaskExecution, and remove the unnecessary Exclude usage so the function/method (the "executions" parameter in dfuExecution.ts) is clearer and simpler.
🤖 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/build/buildMain.ts`:
- Around line 144-162: The catch block unconditionally calls cleanupBuildState()
which in turn calls BuildTask.releaseBuildReservation(), risking releasing a
reservation we never acquired (e.g., when FlashSession.isFlashing short-circuits
before tryReserveBuild()). Modify the flow to track reservation ownership with a
local flag (e.g., let reserved = false; set reserved = true only after
tryReserveBuild() succeeds) and change cleanup/teardown to only call
cleanupBuildState() or BuildTask.releaseBuildReservation() when reserved ===
true; update references in this function to use the new reserved flag to guard
release logic and ensure existing error branches (ALREADY_BUILDING /
BUILD_TERMINATED / default) preserve their behavior.
In `@src/build/dfuExecution.ts`:
- Around line 36-42: readParameter("idf.buildPath", workspace) can return an
empty string which causes join(buildPath, "flasher_args.json") to point to a
cwd-relative file and produce a misleading missing-file warning; update the
logic around the buildPath variable in dfuExecution (the block using
readParameter, buildPath, pathExists, Logger.warnNotify, and join) to explicitly
validate buildPath (e.g., if (!buildPath) or buildPath.trim() === ""), surface a
clear distinct message such as "idf.buildPath is not configured" (mirroring the
"Build path not found" style used in runSizeTaskIfEnabled), and return false
early instead of checking for flasher_args.json when buildPath is empty.
In `@src/build/index.ts`:
- Around line 51-58: The code unsafely casts readParameter("idf.flashType",
wsFolder) to ESP.FlashType; instead validate the returned value before assigning
to resolvedFlashType: call readParameter into a local variable, check that it is
a non-empty string and that Object.values(ESP.FlashType) includes it (or
otherwise map/parse it to a valid enum member), and if validation fails set
resolvedFlashType to an explicit safe default (or undefined) before calling
buildMain(wsFolder.uri, cancelToken, resolvedFlashType, buildType); update the
assignment around resolvedFlashType/readParameter and ensure buildMain receives
only a valid ESP.FlashType or the chosen explicit default.
In `@src/cdtDebugAdapter/debugConfProvider.ts`:
- Line 88: The caller must handle the new undefined return from
resolveDebugConfiguration: update the call site that invokes
cdtDebugProvider.resolveDebugConfiguration(...) and then
vscode.debug.startDebugging to guard against undefined (the call to
vscode.debug.startDebugging expects a DebugConfiguration); if
resolveDebugConfiguration returns undefined, do not call startDebugging and
instead surface the error (e.g., return early and/or call
vscode.window.showErrorMessage) or rethrow so higher-level error handling runs;
alternatively revert resolveDebugConfiguration to throw on error instead of
returning undefined—locate the resolveDebugConfiguration implementation and the
caller that passes its result directly to vscode.debug.startDebugging to apply
the guard.
In `@src/flash/transports/uart/uartFlashCmd.ts`:
- Around line 51-59: The cancellation handler currently clears the global
FlashSession.isFlashing before this call owns it and stays subscribed beyond
this call; fix by acquiring ownership first and making the handler scoped: set
FlashSession.isFlashing = true before registering
cancelToken.onCancellationRequested OR use a local ownership marker (e.g.,
ownsFlash) that the handler checks before clearing, and store the returned
subscription/disposable from cancelToken.onCancellationRequested so you can
dispose/unsubscribe it in the finally block; ensure TaskManager.cancelTasks and
FlashSession.isFlashing are only touched by the handler if this invocation owns
the flash.
- Around line 107-110: The comment claiming runTasksWithBoolean handles listener
disposal is wrong: runTasksWithBoolean (which wraps runTasks) does not call
TaskManager.disposeListeners(), and runTasks only calls disposeTaskListener() on
the success path; therefore restore/keep the finally block that sets
FlashSession.isFlashing = false and calls TaskManager.disposeListeners(); remove
or update any surrounding comment/assertion that claims disposal is already
handled by runTasksWithBoolean, and reference runTasksWithBoolean, runTasks,
disposeTaskListener, TaskManager.disposeListeners, and FlashSession.isFlashing
when making the change.
In `@src/taskManager.ts`:
- Around line 214-341: The listener for tasks.onDidEndTaskProcess can fire
before lastExecution is assigned, so modify runTasks to capture the current
taskId synchronously before each tasks.executeTask call (e.g., create a local
currentTaskId from TaskManager.tasks[0].definition.taskId), use that
currentTaskId when matching inside the onDidEndTaskProcess listener instead of
lastExecution?.task.definition.taskId, and apply the same pattern for the
follow-up execution block where Promise.resolve(tasks.executeTask(...)) is
called so the listener can reliably detect the finished task even if execution
resolves after the event.
---
Outside diff comments:
In `@src/langTools/index.ts`:
- Around line 152-362: The tool currently has unreachable flash-specific error
branches because flashMain/buildFlashAndMonitorCapture swallow errors and return
continueFlag=false; fix by propagating failure status into the
LanguageModelToolResult and adding the missing ALREADY_ERASING handler: after
running command helpers (flashMain, buildMain, buildFlashAndMonitorCapture,
eraseFlashMain) check continueFlag and if false prepend or return a clear status
LanguageModelTextPart (e.g., "Command failed" or specific failure text) instead
of returning raw outputs; also add a catch branch for errorMessage ===
"ALREADY_ERASING" (similar style to ALREADY_FLASHING) so eraseFlashMain failures
produce a consistent LM message; finally, ensure the TASK_COMMANDS output loop
only returns stdout/stderr when continueFlag is true (or include the failure
status when false) so the LM can distinguish success vs failure (update
references: continueFlag, taskExecutions, flashMain,
buildFlashAndMonitorCapture, eraseFlashMain, ALREADY_ERASING, TASK_COMMANDS).
---
Nitpick comments:
In `@src/build/dfuExecution.ts`:
- Line 32: The parameter type for "executions" uses
Exclude<MaybeIdfTaskExecution, undefined>[] which is equivalent to
IdfTaskExecution[]; change the signature to use IdfTaskExecution[] directly,
update any imports to reference IdfTaskExecution instead of
MaybeIdfTaskExecution, and remove the unnecessary Exclude usage so the
function/method (the "executions" parameter in dfuExecution.ts) is clearer and
simpler.
In `@src/cdtDebugAdapter/debugConfProvider.ts`:
- Around line 48-58: The workspace-folder resolution sequence is duplicated in
resolveDebugConfigurationWithSubstitutedVariables and resolveDebugConfiguration;
extract it into a small async helper (e.g., getOrPickWorkspaceFolder) that calls
ESP.GlobalConfiguration.store.getSelectedWorkspaceFolder(), falls back to
window.showWorkspaceFolderPick(...) with the same placeholder, and throws the
existing error if no folder is returned; replace both occurrences in
resolveDebugConfigurationWithSubstitutedVariables and resolveDebugConfiguration
with a call to this helper so future changes (telemetry, caching) live in one
place.
- Around line 158-192: The code silently falls back to 2 when idfTarget is not a
known key; after calling getIdfTargetFromSdkconfig and before replacing tokens
in config.initCommands, check idfTargetWatchpointMap for the key (don't just
cast with as IdfTarget), compute const watchpointNum =
idfTargetWatchpointMap[idfTarget as any]; if watchpointNum is undefined log a
debug/warning message including the raw idfTarget and that you're using the
default 2 (use the module's existing logger or console.debug if none), then use
the fallback value; finally replace "{IDF_TARGET_CPU_WATCHPOINT_NUM}" with
String(watchpointNum ?? 2). This ensures getIdfTargetFromSdkconfig,
idfTargetWatchpointMap, and the mapping logic are explicitly checked and any
unexpected target is traceable.
In `@src/flash/transports/jtag/flashTclClient.ts`:
- Around line 96-111: The onError handler currently discards non-Error values
and only returns a generic stderr; update onError to log the raw err for
diagnostics (e.g., call Logger.error(err, "jtagFlash onError") or the module's
logger) before building the CapturedTaskOutput, then keep constructing the same
output object and call finish(...) with createCapturedExecution(output) as
before; reference the onError function, CapturedTaskOutput,
createCapturedExecution and finish so you add only the logging step without
changing the returned output shape or control flow.
- Around line 26-32: The function createCapturedExecution currently uses a
double-cast to IdfTaskExecution to satisfy the union type; instead define and
use a narrower interface (e.g., CaptureableExecution) that only includes
getOutput(), update createCapturedExecution to return that narrower type, and
update any callers (such as throwCapturedTaskFailure) to accept the narrower
CaptureableExecution or perform a type guard ("getOutput" in execution) before
treating it as IdfTaskExecution; reference createCapturedExecution,
IdfTaskExecution, getOutput, and throwCapturedTaskFailure when making these
changes.
In `@src/taskManager.ts`:
- Around line 263-279: The end-listener currently calls
TaskManager.cancelTasks(), which iterates the task queue but the current task
was already removed by tasks.onDidEndTaskProcess, making cancelTasks() a noop
and confusing to readers; replace that call with an explicit TaskManager.tasks =
[] (to clear any remaining queued tasks) and keep TaskManager.disposeListeners()
and disposeTaskListener(), or at minimum add a clarifying comment near the
dispose/reject block referencing tasks.onDidEndTaskProcess and why cancelTasks()
is not used — update the code paths around TaskManager.cancelTasks(),
TaskManager.tasks, TaskManager.disposeListeners(), disposeTaskListener(), and
the error/reject logic for lastExecution to reflect the explicit cleanup.
- Around line 184-189: The boolean presentation overrides in
newTask.presentationOptions currently use logical OR which treats an explicit
false as unset; update the assignments for showReuseMessage and clear to use
nullish coalescing so caller false is preserved (i.e., replace
presentationOptions?.showReuseMessage || false and presentationOptions?.clear ||
false with presentationOptions?.showReuseMessage ?? false and
presentationOptions?.clear ?? false) while leaving reveal and panel handling
unchanged.
🪄 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: 664d8969-076d-40b9-8d65-534e1b972c56
📒 Files selected for processing (16)
src/build/buildHelpers.tssrc/build/buildMain.tssrc/build/dfuExecution.tssrc/build/index.tssrc/build/validation.tssrc/buildFlashMonitor/index.tssrc/cdtDebugAdapter/debugConfProvider.tssrc/common/store.tssrc/eraseFlash/transports/jtag/jtag.tssrc/flash/index.tssrc/flash/shared/errHandling.tssrc/flash/transports/jtag/flashTclClient.tssrc/flash/transports/uart/uartFlashCmd.tssrc/langTools/index.tssrc/taskManager.tssrc/test/suite/build.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/build/buildHelpers.ts
- src/common/store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/build/validation.ts
brianignacio5
force-pushed
the
refactor/flashing
branch
from
3cfc929 to
609fb3b
Compare
brianignacio5
force-pushed
the
refactor/flashing
branch
from
609fb3b to
3458a13
Compare
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
This pull request refactors and modularizes the ESP-IDF VSCode extension's build system, splitting the previously monolithic
buildCmd.tsinto focused modules. The changes improve maintainability, clarity, and future extensibility of the build process, while also updating localized strings in multiple languages.Build system refactor and modularization:
buildCmd.tsto a newbuildMain.tsfile, which now orchestrates the build pipeline, handles task reservations, manages cancellation, and coordinates pre/post build tasks, DFU builds, and size reporting. This results in cleaner, more maintainable code.buildHelpers.tsfile, promoting code reuse and separation of concerns.buildFinishFlashCmd.tsmodule, with improvements such as quoting binary file paths and returning an empty string if the flasher args file is missing.buildCmd.tshas been removed, with its responsibilities distributed among the new modules, eliminating duplication and outdated code.Localization updates:
Type of change
Please delete options that are not relevant.
Steps to test this pull request
Click on any "Flash" command. It should work as expected.
Execute action.
Observe results.
Expected behaviour:
All flash commands continue working as expected.
Expected output:
All build output remains the same.
Expected behaviour:
Expected output:
How has this been tested?
Manual tests of Flash commands in extension.
Test Configuration:
Checklist
Summary by CodeRabbit
Localization
New Features
Build & Flash
Tests