Replace CLI Monitor with ESP Decoder Terminal in used target "Upload and Monitor"#52
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughResolve monitor baud from PlatformIO when unspecified and intercept pioarduino's combined "Upload and Monitor" task: run a synthetic upload-only replacement, open the ESP Decoder monitor on success or reacquire the port on failure, and suppress spurious upload events during interception. ChangesUpload and Monitor Task Interception + Baud Resolution
Sequence DiagramsequenceDiagram
participant VSCode
participant Extension
participant Monitor as ESP Decoder Monitor
VSCode->>Extension: Start Upload and Monitor task
Extension->>Extension: Detect combined task
Extension->>VSCode: Terminate original task
Extension->>VSCode: Execute synthetic upload-only task
VSCode->>Extension: Synthetic task completes
alt Exit code 0 (success)
Extension->>Monitor: Open with autoConnect
else Exit code != 0 (failure)
Extension->>VSCode: Reacquire serial port
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/extension.ts`:
- Around line 579-590: The current code calls event.execution.terminate() and
immediately launches vscode.tasks.executeTask(uploadOnly), which can race
because terminate() is fire-and-forget; change this to wait for the original
execution to fully end before calling executeTask. Implement a Promise that
registers a one-time vscode.tasks.onDidEndTask listener filtered to the original
execution (capture the execution in a local origExecution variable or compare
event.execution), resolve when that event fires (or timeout/fallback), await
that Promise after calling event.execution.terminate(), then call
vscode.tasks.executeTask(uploadOnly) and update interceptionInProgress
appropriately; ensure the onDidEndTask disposable is disposed after resolving to
avoid leaks.
- Line 444: The global boolean interceptionInProgress is racy and can get stuck;
replace it with a scoped Set<vscode.TaskExecution> (e.g., syntheticUploads) that
you add to when executeTask(uploadOnlyTask) resolves and remove from in the
onDidEndTask/onDidEndTaskProcess handler (or via a per-execution fallback
timeout) so suppression applies only to that TaskExecution; ensure you await the
original TaskExecution.terminate() before spawning the synthetic upload to avoid
concurrent pio runs, and only clear the TaskExecution from the set after
executeCommand('esp-decoder.openMonitor', …) and reacquireWithRetry complete
(i.e., await openMonitor/reacquire before removing) so the suppression remains
until auto-connect/reacquire finishes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
@coderabbitai is the goal from the PR reached to start ESP Decoder instead of CLI Terminal for the combined target "Upload and Monitor"? Is the implementation solid? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 27923
✅ Goal: AchievedThe interception flow works end-to-end:
🔍 Implementation QualityStrengths:
Potential gaps to be aware of:
None of these are blockers for the primary use case, but gap Summary: The PR successfully replaces the pioarduino CLI monitor with ESP Decoder for the "Upload and Monitor" target. The implementation is well-thought-out, handles the key race conditions correctly, and includes proper fallback handling. The gaps listed above are edge cases that won't affect normal usage. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/pioIntegration.ts (2)
579-589: 💤 Low valuePrefer a static import for consistency with the existing
pioCoreimport.
pioarduino-node-helpersis already statically imported at the top of this file (line 6), so the dynamicawait import(...)here is inconsistent and adds an extra Promise hop on a hot path (called every timeesp-decoder.openMonitorruns without an explicit baud rate). A static import keeps the bundling/tree-shaking story consistent withcore.♻️ Proposed simplification
-import { core as pioCore } from 'pioarduino-node-helpers'; +import { core as pioCore, project as pioProject } from 'pioarduino-node-helpers';export async function getMonitorBaudRate(projectPath: string): Promise<number | undefined> { try { - const { ProjectConfig } = (await import('pioarduino-node-helpers')).project; - const config = new ProjectConfig(projectPath); + const config = new pioProject.ProjectConfig(projectPath); await config.read(); return config.getEnvMonitorSpeed(config.defaultEnv()); } catch { // PIO Core not available or project not configured — ignore } return undefined; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pioIntegration.ts` around lines 579 - 589, The function getMonitorBaudRate currently uses a dynamic await import of 'pioarduino-node-helpers' — change it to use the already statically imported module (the same symbol used where pioCore is imported) and directly reference ProjectConfig instead of calling await import(...). Keep the existing error handling: construct ProjectConfig(projectPath), await config.read(), and return config.getEnvMonitorSpeed(config.defaultEnv()), but remove the dynamic import and any unnecessary Promise hop so calls from esp-decoder.openMonitor use the static ProjectConfig class.
580-588: 💤 Low valueSilent failure makes baud-rate resolution failures invisible.
Any failure (PIO Core missing, project not configured, helper API mismatch, unexpected exception) is swallowed without any diagnostic, so users who expect their
monitor_speedto be honoured will silently fall back to the default baud without any way to know why. Consider logging the error path to an output channel — even at a debug level — to aid troubleshooting, especially since the new ProjectConfig API is from a freshly bumped dependency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pioIntegration.ts` around lines 580 - 588, The try/catch around the ProjectConfig lookup swallows all errors (affecting ProjectConfig, getEnvMonitorSpeed, defaultEnv or import failures) so add logging inside the catch that records the caught error and context; locate the block that imports project.ProjectConfig and calls config.read()/getEnvMonitorSpeed/defaultEnv and in the catch log a concise diagnostic (e.g., using the existing output/logger instance or console.debug/error) including the exception message and that baud-rate resolution failed, then return undefined as before.src/extension.ts (4)
723-732: 💤 Low valueSuppression predicate looks correct; consider logging the suppression source for diagnostics.
The combined check
suppressingCombinedTask || syntheticUploadExecutions.size > 0correctly covers both windows — the brief gap betweenterminate()andexecuteTaskresolving, and the lifetime of the synthetic upload itself. Since this is the place where bugs in the handover would manifest as "weird port behavior after upload", it would help future debugging to log which condition matched:♻️ Optional log refinement
- if (suppressingCombinedTask || syntheticUploadExecutions.size > 0) { - log.appendLine( - `[ESP Decoder] Ignoring onDidUpload (exitCode=${exitCode}) — intercepted upload in progress`, - ); + if (suppressingCombinedTask || syntheticUploadExecutions.size > 0) { + const reason = suppressingCombinedTask + ? 'combined-task termination' + : `synthetic upload (${syntheticUploadExecutions.size} active)`; + log.appendLine( + `[ESP Decoder] Ignoring onDidUpload (exitCode=${exitCode}) — ${reason}`, + ); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/extension.ts` around lines 723 - 732, The suppression log should record which predicate caused the early return to aid diagnostics: update the block in src/extension.ts where the variables suppressingCombinedTask and syntheticUploadExecutions are checked (the onDidUpload handler) to include in the process log whether suppression was due to suppressingCombinedTask being true or syntheticUploadExecutions.size > 0 (and include the actual size value); keep the existing message but append the matching source (e.g., "suppressedBy=suppressingCombinedTask" or "suppressedBy=syntheticUploadExecutions,size=NN") so future debugging can tell which branch fired.
622-629: 💤 Low value
suppressingCombinedTaskreset window on the failure path leaves a small unsuppressed gap.If
vscode.tasks.executeTask(uploadOnly)rejects, the catch on line 626 setssuppressingCombinedTask = falseimmediately. Pioarduino's spuriousonDidUpload(exitCode != 0)for the terminated combined task may arrive after this point (it's an async pipeline of its own), at which point neither flag is set and the suppression check at lines 727 falls through —reacquireWithRetryruns unexpectedly even though the user never actually started a new monitor session.Consider keeping
suppressingCombinedTasktrue for a brief tail (e.g., asetTimeout(..., 1500)cleanup) when the synthetic launch fails, so the trailing pioarduino event can't escape the suppression window.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/extension.ts` around lines 622 - 629, The catch path that handles a rejected vscode.tasks.executeTask(uploadOnly) currently clears suppressingCombinedTask immediately, creating a race where a late pioarduino onDidUpload can bypass suppression; modify the catch block that references syntheticUploadExecutions and suppressingCombinedTask so instead of setting suppressingCombinedTask = false immediately it schedules a short delayed reset (e.g., setTimeout to clear suppressingCombinedTask after ~1500ms) and only clears any temporary state after that delay; ensure syntheticUploadExecutions handling and any immediate cleanup still occur, but keep suppressingCombinedTask true for the brief tail so the later onDidUpload handler and the reacquireWithRetry suppression check remain effective.
102-110: 💤 Low valueSimplify the baud rate resolution IIFE.
The async IIFE on lines 102-107 is harder to read than necessary. A small inline helper or direct conditional is clearer:
♻️ Proposed simplification
- const resolvedBaudRate = typeof opts?.baudRate === 'number' - ? opts.baudRate - : await (async () => { - const folder = vscode.workspace.workspaceFolders?.[0]?.uri.fsPath; - return folder ? await getMonitorBaudRate(folder) : undefined; - })(); + let resolvedBaudRate: number | undefined = opts?.baudRate; + if (typeof resolvedBaudRate !== 'number') { + const folder = vscode.workspace.workspaceFolders?.[0]?.uri.fsPath; + resolvedBaudRate = folder ? await getMonitorBaudRate(folder) : undefined; + } if (typeof resolvedBaudRate === 'number') { serialManager.setBaudRate(resolvedBaudRate); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/extension.ts` around lines 102 - 110, The async IIFE used to compute resolvedBaudRate is unnecessary and hard to read; replace it with a direct conditional that awaits getMonitorBaudRate when no numeric opts?.baudRate is provided. Specifically, compute resolvedBaudRate by checking typeof opts?.baudRate === 'number' and otherwise reading the first workspace folder path and awaiting getMonitorBaudRate(folder) (or leaving undefined if no folder), then call serialManager.setBaudRate(resolvedBaudRate) when the result is a number; update references to resolvedBaudRate, opts?.baudRate, getMonitorBaudRate, and serialManager.setBaudRate accordingly.
504-538: ⚖️ Poor tradeoffShellExecution falls back silently to letting the CLI monitor run.
buildUploadOnlyTaskonly handlesvscode.ProcessExecutionand returnsundefinedfor everything else. When that happens,onDidStartTasklogs a warning to the output channel and returns without terminating the combined task — so on any pioarduino version (or platform) where the "Upload and Monitor" task is constructed withShellExecution, the user silently gets the legacy CLI monitor and the PR's main feature simply doesn't activate.
isPioUploadAndMonitorTaskalready detects the task by name regex (lines 483–484) even whentask.executionisn'tProcessExecution, so the detection→build mismatch is the blocker. Two reasonable mitigations:
- Add
ShellExecutionsupport: clonecommand, strip--target monitorfromargs/commandLine, preserveoptions.- At minimum, surface the situation more visibly (e.g. a one-time
showWarningMessage) rather than only logging to the output channel, so users on the unsupported path realize why the new behavior didn't kick in.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/extension.ts` around lines 504 - 538, The buildUploadOnlyTask currently only handles vscode.ProcessExecution and returns undefined for ShellExecution which causes detection (isPioUploadAndMonitorTask) to succeed but no interception to occur; update buildUploadOnlyTask to also handle vscode.ShellExecution by cloning the ShellExecution's command/commandLine and args, removing the "--target monitor" token (and its following "monitor" literal if present) while preserving execution.options/presentationOptions and other metadata, creating the new task with a new vscode.ShellExecution; if you prefer a fallback instead of full ShellExecution support, add a visible user-facing fallback (vscode.window.showWarningMessage) when you detect a pio upload-and-monitor task whose execution is not ProcessExecution to inform the user the extension could not intercept the monitor.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/extension.ts`:
- Around line 597-620: The current originalTaskEnded Promise uses a fixed
5_000ms setTimeout which can race with a slow origExecution shutdown; change the
implementation of originalTaskEnded (the Promise that registers
vscode.tasks.onDidEndTask and the setTimeout) to poll
vscode.tasks.taskExecutions for the presence of origExecution until it
disappears or a longer deadline (e.g. 10–15s) elapses, and call
processLogger/log.appendLine when the timeout path is taken so the event is
visible; keep disposing the subscription (sub.dispose()) when onDidEndTask fires
or when the polling/timeout completes, and ensure you still await
originalTaskEnded before creating the synthetic upload-only task while leaving
suppressingCombinedTask handling unchanged.
---
Nitpick comments:
In `@src/extension.ts`:
- Around line 723-732: The suppression log should record which predicate caused
the early return to aid diagnostics: update the block in src/extension.ts where
the variables suppressingCombinedTask and syntheticUploadExecutions are checked
(the onDidUpload handler) to include in the process log whether suppression was
due to suppressingCombinedTask being true or syntheticUploadExecutions.size > 0
(and include the actual size value); keep the existing message but append the
matching source (e.g., "suppressedBy=suppressingCombinedTask" or
"suppressedBy=syntheticUploadExecutions,size=NN") so future debugging can tell
which branch fired.
- Around line 622-629: The catch path that handles a rejected
vscode.tasks.executeTask(uploadOnly) currently clears suppressingCombinedTask
immediately, creating a race where a late pioarduino onDidUpload can bypass
suppression; modify the catch block that references syntheticUploadExecutions
and suppressingCombinedTask so instead of setting suppressingCombinedTask =
false immediately it schedules a short delayed reset (e.g., setTimeout to clear
suppressingCombinedTask after ~1500ms) and only clears any temporary state after
that delay; ensure syntheticUploadExecutions handling and any immediate cleanup
still occur, but keep suppressingCombinedTask true for the brief tail so the
later onDidUpload handler and the reacquireWithRetry suppression check remain
effective.
- Around line 102-110: The async IIFE used to compute resolvedBaudRate is
unnecessary and hard to read; replace it with a direct conditional that awaits
getMonitorBaudRate when no numeric opts?.baudRate is provided. Specifically,
compute resolvedBaudRate by checking typeof opts?.baudRate === 'number' and
otherwise reading the first workspace folder path and awaiting
getMonitorBaudRate(folder) (or leaving undefined if no folder), then call
serialManager.setBaudRate(resolvedBaudRate) when the result is a number; update
references to resolvedBaudRate, opts?.baudRate, getMonitorBaudRate, and
serialManager.setBaudRate accordingly.
- Around line 504-538: The buildUploadOnlyTask currently only handles
vscode.ProcessExecution and returns undefined for ShellExecution which causes
detection (isPioUploadAndMonitorTask) to succeed but no interception to occur;
update buildUploadOnlyTask to also handle vscode.ShellExecution by cloning the
ShellExecution's command/commandLine and args, removing the "--target monitor"
token (and its following "monitor" literal if present) while preserving
execution.options/presentationOptions and other metadata, creating the new task
with a new vscode.ShellExecution; if you prefer a fallback instead of full
ShellExecution support, add a visible user-facing fallback
(vscode.window.showWarningMessage) when you detect a pio upload-and-monitor task
whose execution is not ProcessExecution to inform the user the extension could
not intercept the monitor.
In `@src/pioIntegration.ts`:
- Around line 579-589: The function getMonitorBaudRate currently uses a dynamic
await import of 'pioarduino-node-helpers' — change it to use the already
statically imported module (the same symbol used where pioCore is imported) and
directly reference ProjectConfig instead of calling await import(...). Keep the
existing error handling: construct ProjectConfig(projectPath), await
config.read(), and return config.getEnvMonitorSpeed(config.defaultEnv()), but
remove the dynamic import and any unnecessary Promise hop so calls from
esp-decoder.openMonitor use the static ProjectConfig class.
- Around line 580-588: The try/catch around the ProjectConfig lookup swallows
all errors (affecting ProjectConfig, getEnvMonitorSpeed, defaultEnv or import
failures) so add logging inside the catch that records the caught error and
context; locate the block that imports project.ProjectConfig and calls
config.read()/getEnvMonitorSpeed/defaultEnv and in the catch log a concise
diagnostic (e.g., using the existing output/logger instance or
console.debug/error) including the exception message and that baud-rate
resolution failed, then return undefined as before.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d6005af-4eb0-4513-a29c-cb2caeb08947
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonsrc/extension.tssrc/pioIntegration.tssrc/pioarduino-node-helpers.d.ts
|
@coderabbitai is the goal from the PR reached to start ESP Decoder instead of CLI Terminal for the combined target "Upload and Monitor"? Is the implementation solid? |
|
Tip For best results, initiate chat on the files or code changes.
✅ Goal: AchievedThe interception flow works end-to-end:
🔍 Implementation QualitySolid points: no race condition on termination, well-designed suppression of spurious Potential gaps (all non-blocking for normal usage):
Gap #1 is the only one worth monitoring as pioarduino evolves. Everything else is an edge case that won't affect normal "Upload and Monitor" usage. |
|
@coderabbitai you did not take in account latest commits!! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 61042
✅ What the latest commits fixed1.
|
| # | Issue | Severity | Status |
|---|---|---|---|
| 1 | ShellExecution not handled: buildUploadOnlyTask returns undefined and logs a warning; CLI monitor silently takes over |
Medium | Still present |
| 3 | onDidEndTaskProcess may not fire for synthetic tasks cancelled before the process starts — monitor won't open automatically |
Low | Still present |
Gap #1 is the only one worth keeping an eye on as pioarduino evolves. Gap #3 is a rare edge case (task cancelled in the window between executeTask() resolving and the process spawning) and the fallback onDidEndTask listener does clean up syntheticUploadExecutions properly even in that scenario, so state stays consistent.
✅ Updated Summary
The latest commits address the two most actionable gaps from the previous review. The PR now:
- Correctly opens ESP Decoder instead of the CLI terminal for "Upload and Monitor" ✔️
- Forwards the correct baud rate from
platformio.inito the monitor ✔️ - Uses a robust 10 s fallback timeout ✔️
- Handles race conditions and spurious
onDidUploadevents correctly ✔️
The implementation is solid and ready. The two remaining gaps are known limitations that do not affect normal usage.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes