fix: Windows ARM64 version#29
Merged
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 45 minutes and 13 seconds. ⌛ 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 selected for processing (1)
📝 WalkthroughWalkthroughThis PR bumps the extension version to 0.20.1 and adds comprehensive serial communication logging. It introduces OutputChannel support to SerialPortManager, enhances error handling with improved event propagation, adds connection/error logging to WebviewPanel, and updates package.json with a patched serialport bindings fork to fix Windows ARM64 issues. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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: 1
🧹 Nitpick comments (4)
fork-template/README.md (1)
52-58: Version in example doesn't match actual usage.The example shows
^13.0.0, but the actualpackage.jsonin the repository root uses^13.0.2. Update this example to match for consistency:{ "overrides": { - "@serialport/bindings-cpp": "npm:`@jason2866/serialport-bindings-cpp`@^13.0.0" + "@serialport/bindings-cpp": "npm:`@jason2866/serialport-bindings-cpp`@^13.0.2" } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fork-template/README.md` around lines 52 - 58, The version shown in the README JSON example is inconsistent with package.json: update the override value for "@serialport/bindings-cpp" from "^13.0.0" to "^13.0.2" in the README's JSON example so it matches the repository's actual dependency version; locate the JSON code block in README.md that contains "overrides" and change the version string accordingly.package.json (1)
203-205: Update documentation example to match the override version.The forked package exists and version 13.0.2 is available on npm. However, there's a documentation inconsistency:
- The override uses
^13.0.2- The example in
fork-template/README.mdshows^13.0.0Update the README example to reference
^13.0.2for consistency with the actual version pinned in package.json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 203 - 205, Update the example in fork-template/README.md so the documented override version matches package.json: change the referenced override for `@serialport/bindings-cpp` from ^13.0.0 to ^13.0.2 to reflect the actual pinned override ("@serialport/bindings-cpp": "npm:`@jason2866/serialport-bindings-cpp`@^13.0.2") and keep docs consistent with package.json.src/webviewPanel.ts (1)
74-79: Also surface serial errors in the monitor UI, not only output channel.Right now errors are appended to the output channel only. Posting a webview
errormessage here would keep user-visible feedback consistent with other failure paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/webviewPanel.ts` around lines 74 - 79, The serial error handler currently only writes to the output channel via this.log.appendLine inside the this.serialManager.onError callback; update that handler to also send an error notification to the monitor webview so the UI shows the failure. Locate the onError subscription (this.serialManager.onError) and add a webview message post (e.g., this.panel.webview.postMessage or this.webview.postMessage depending on the class field used) sending a payload like { type: 'error', message: err.message } alongside the existing this.log.appendLine call so both the output channel and the webview receive the error.src/serialPortManager.ts (1)
113-127: Prefer extension output-channel logging over rawconsole.login connect path.These logs bypass the extension’s existing output-channel trail and can become noisy in production. Consider routing through a shared logger/output channel (or a debug flag) for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/serialPortManager.ts` around lines 113 - 127, The connect() method currently uses raw console.log calls (e.g., inside connect(), around this._isConnected, this._selectedPath, this._baudRate and when no port is selected) which bypass the extension’s output channel; replace those console.log calls with the shared extension logger/output-channel API (or gate them behind a debug flag) so logs flow through the extension’s output trail — update messages in connect(), the selected-port branch (where selectPort() is called), and before/after await this.disconnect() to call the shared logger (preserving the same content) and ensure any async/error paths still return the same boolean behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/serialPortManager.ts`:
- Around line 151-157: The 'close' handler currently always calls
this._onConnectionChange.fire(false), causing duplicate disconnect events
because disconnect() also emits false; modify the close handler to only fire the
connection change when the connection state actually transitions (e.g., check
this._isConnected or a similar flag before calling
this._onConnectionChange.fire(false)), or remove the emission from the 'close'
handler and let disconnect() be the single place that emits; update the logic
around this.port.on('close', ...) and the disconnect() method to ensure
_onConnectionChange.fire(false) is emitted exactly once per disconnect and that
this._isConnected is set consistently.
---
Nitpick comments:
In `@fork-template/README.md`:
- Around line 52-58: The version shown in the README JSON example is
inconsistent with package.json: update the override value for
"@serialport/bindings-cpp" from "^13.0.0" to "^13.0.2" in the README's JSON
example so it matches the repository's actual dependency version; locate the
JSON code block in README.md that contains "overrides" and change the version
string accordingly.
In `@package.json`:
- Around line 203-205: Update the example in fork-template/README.md so the
documented override version matches package.json: change the referenced override
for `@serialport/bindings-cpp` from ^13.0.0 to ^13.0.2 to reflect the actual
pinned override ("@serialport/bindings-cpp":
"npm:`@jason2866/serialport-bindings-cpp`@^13.0.2") and keep docs consistent with
package.json.
In `@src/serialPortManager.ts`:
- Around line 113-127: The connect() method currently uses raw console.log calls
(e.g., inside connect(), around this._isConnected, this._selectedPath,
this._baudRate and when no port is selected) which bypass the extension’s output
channel; replace those console.log calls with the shared extension
logger/output-channel API (or gate them behind a debug flag) so logs flow
through the extension’s output trail — update messages in connect(), the
selected-port branch (where selectPort() is called), and before/after await
this.disconnect() to call the shared logger (preserving the same content) and
ensure any async/error paths still return the same boolean behavior.
In `@src/webviewPanel.ts`:
- Around line 74-79: The serial error handler currently only writes to the
output channel via this.log.appendLine inside the this.serialManager.onError
callback; update that handler to also send an error notification to the monitor
webview so the UI shows the failure. Locate the onError subscription
(this.serialManager.onError) and add a webview message post (e.g.,
this.panel.webview.postMessage or this.webview.postMessage depending on the
class field used) sending a payload like { type: 'error', message: err.message }
alongside the existing this.log.appendLine call so both the output channel and
the webview receive the error.
🪄 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: 03a598bc-fa27-41c0-9a78-b7d9ba1a2ca6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.vscodeignorefork-template/.github/workflows/prebuild.ymlfork-template/README.mdpackage.jsonsrc/serialPortManager.tssrc/webviewPanel.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/serialPortManager.ts (1)
153-160:⚠️ Potential issue | 🟡 MinorClear
this.portin theclosehandler to avoid stale closed handles.On Line 153, the
closepath updates_isConnectedbut keepsthis.portnon-null. For unexpected closes (e.g., device removal), this leaves stale state until a later reconnect.♻️ Proposed fix
this.port.on('close', (disconnectError?: Error | null) => { if (disconnectError) { this._onError.fire(disconnectError); } - if (this._isConnected) { - this._isConnected = false; - this._onConnectionChange.fire(false); - } + const wasConnected = this._isConnected; + this._isConnected = false; + this.port = null; + if (wasConnected) { + this._onConnectionChange.fire(false); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/serialPortManager.ts` around lines 153 - 160, The close handler currently sets _isConnected and fires _onConnectionChange/_onError but leaves this.port referencing the closed SerialPort; modify the close callback on this.port to clear the stale handle (set this.port = null) and remove/cleanup any listeners so the manager no longer holds a closed port reference. Ensure you do this inside the existing this.port.on('close', ...) block alongside setting this._isConnected = false and firing _onConnectionChange, referencing this.port, _isConnected, _onConnectionChange, and _onError.
🧹 Nitpick comments (1)
src/serialPortManager.ts (1)
33-35: Track OutputChannel ownership for proper disposal.If no channel is injected (Line 35), this class creates one but never disposes it in
dispose().♻️ Proposed refactor
export class SerialPortManager extends vscode.Disposable { private port: SerialPort | null = null; @@ private readonly log: vscode.OutputChannel; + private readonly ownsLog: boolean; @@ constructor(outputChannel?: vscode.OutputChannel) { super(() => this.dispose()); - this.log = outputChannel || vscode.window.createOutputChannel('ESP Decoder'); + this.ownsLog = !outputChannel; + this.log = outputChannel || vscode.window.createOutputChannel('ESP Decoder'); @@ dispose(): void { @@ this._onData.dispose(); this._onError.dispose(); this._onConnectionChange.dispose(); + if (this.ownsLog) { + this.log.dispose(); + } } }Also applies to: 272-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/serialPortManager.ts` around lines 33 - 35, The class creates an OutputChannel in the constructor when none is injected but never disposes it; add an ownership flag (e.g., createdOutputChannel or ownsOutputChannel) set true when you call vscode.window.createOutputChannel in the constructor(s) and false when an external channel is provided, then update the dispose() method to call this.log.dispose() only when that flag is true; apply the same change to the other constructor/creation site referenced around the 272-279 block so only internally-created channels are disposed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/serialPortManager.ts`:
- Around line 153-160: The close handler currently sets _isConnected and fires
_onConnectionChange/_onError but leaves this.port referencing the closed
SerialPort; modify the close callback on this.port to clear the stale handle
(set this.port = null) and remove/cleanup any listeners so the manager no longer
holds a closed port reference. Ensure you do this inside the existing
this.port.on('close', ...) block alongside setting this._isConnected = false and
firing _onConnectionChange, referencing this.port, _isConnected,
_onConnectionChange, and _onError.
---
Nitpick comments:
In `@src/serialPortManager.ts`:
- Around line 33-35: The class creates an OutputChannel in the constructor when
none is injected but never disposes it; add an ownership flag (e.g.,
createdOutputChannel or ownsOutputChannel) set true when you call
vscode.window.createOutputChannel in the constructor(s) and false when an
external channel is provided, then update the dispose() method to call
this.log.dispose() only when that flag is true; apply the same change to the
other constructor/creation site referenced around the 272-279 block so only
internally-created channels are disposed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10267b32-c6b3-4c84-8635-d38489adc56d
📒 Files selected for processing (3)
src/extension.tssrc/serialPortManager.tssrc/webviewPanel.ts
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/serialPortManager.ts`:
- Around line 196-203: The current close callback clears this.port and emits a
disconnected state even when this.port.close reports an error; instead, only
clear this.port and update this._isConnected / call
this._onConnectionChange.fire(false) when err is falsy so the existing 'close'
event handler remains the sole place that cleans up on an actual close; leave
the port reference intact and do not change connection state on error
(optionally log or propagate the err) in the this.port.close callback.
🪄 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: bd8514cf-bca6-4a4e-a506-277b13c8414f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
CHANGELOG.mdpackage.jsonsrc/crashDecoder.tssrc/extension.tssrc/serialPortManager.tssrc/webviewPanel.ts
Remove redundant assignment of port to null on close.
Jason2866
added a commit
that referenced
this pull request
Apr 16, 2026
fix: Windows ARM64 version (#29)
Merged
This was referenced May 5, 2026
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.
Summary by CodeRabbit
Bug Fixes
Improvements
Chores