Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (41)
📝 WalkthroughWalkthroughAdjusted Modbus response parsing (radix and unsigned/signed fixes), added FC23 write input validation, improved port lifecycle listener handling, introduced ESLint flat config and lint scripts, updated CI/Gulp/tooling, removed legacy lint artifacts, and applied small formatting/whitespace tweaks across examples/tests and tcp port buffering line. Changes
Sequence Diagram(s)(Skipped — changes are parsing/validation, lint/config, CI, and minor formatting edits; no new multi-component sequential feature introduced.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.js`:
- Around line 275-277: The byte-count for FC23 is being read as a signed integer
in _readFC23 (data.readInt8(2)), which yields negative values for counts >127;
change the parser to read an unsigned byte (use data.readUInt8(2)) and ensure
the subsequent loop that builds values[] uses that unsigned length to iterate
the correct number of bytes when decoding registers in _readFC23.
- Around line 1269-1307: The code currently checks only types for valuesToWrite;
add a length validation before allocating the request buffer: if
Buffer.isBuffer(valuesToWrite) ensure valuesToWrite.length >= writeByteCount,
and if Array.isArray(valuesToWrite) ensure valuesToWrite.length >=
numWriteRegisters; if the check fails call next(new Error("valuesToWrite length
does not match numWriteRegisters")) (or return the same callback pattern used
earlier) and return. Place this validation just after computing
writeByteCount/numWriteRegisters (before using valuesToWrite in the Buffer.copy
or writeUInt16BE loop) and keep existing symbols (writeByteCount,
numWriteRegisters, valuesToWrite, next) to locate where to add the check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0abf21d-1467-4ac8-a7e8-64e827242429
📒 Files selected for processing (3)
index.jspackage.jsonports/tcpport.js
yaacov
force-pushed
the
fix-lint
branch
2 times, most recently
from
e4f3513 to
a17feae
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.js`:
- Around line 1264-1291: The code currently uses numReadRegisters and
numWriteRegisters without validation which can cause synchronous throws; add
validation before any buffer/size calculations (before computing writeByteCount
and responseLength) to ensure both numReadRegisters and numWriteRegisters are
defined, are numbers, are integers, and are within acceptable ranges (e.g., >0
and <= max registers you support—use the same protocol limit you enforce
elsewhere, such as 125); if validation fails call next(new Error(...)) or
next(new BadAddressError()) as appropriate and return. Reference the variables
numReadRegisters and numWriteRegisters and the logic around computing
writeByteCount and responseLength so the checks run prior to Buffer.alloc(),
writeUInt8(), writeUInt16BE(), or any size math.
- Around line 1421-1423: Replace the optional catch binding with a traditional
catch parameter to retain compatibility with older Node versions: change the two
try/catch blocks that load optional native deps (the one requiring
"./ports/rtubufferedport" which assigns module.exports.RTUBufferedPort and the
other similar optional require) from "catch { }" to "catch (err) { }" (you can
ignore or log err inside the block); do not change README in this PR so runtime
syntax remains Node 4/6/8 compatible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3199f3e-993e-4bc0-bbf5-6951c1ed7722
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (40)
.eslintrc.json.github/workflows/ci.yml.gitignore.vscode/launch.jsonREADME.mdeslint.config.mjsexamples/async_server.jsexamples/buffer.jsexamples/buffered_server.jsexamples/buffertcp.jsexamples/debug.jsexamples/device-identification.jsexamples/logger.jsexamples/logger_complete.jsexamples/polling_RTU.jsexamples/polling_TCP.jsexamples/polling_UDP.jsexamples/server.jsexamples/server_enron.jsexamples/server_enron_serial.jsexamples/simple.jsexamples/simple_ble.jsexamples/simple_enron.jsexamples/simple_enron_serial.jsexamples/tcp_client_abort_signal.jsexamples/write.jsexamples/write_complete.jsgulpfile.jsindex.jsnpm-update.shnpm-upgrade.shpackage.jsonports/bleport.jsports/tcpport.jstest/Lint/test.jstest/servers/serverserial.test.jstest/servers/servertcp.enron.test.jstest/servers/servertcp.test.jstest/servers/servertcpCallback.test.jstest/servers/servertcpPromise.test.js
💤 Files with no reviewable changes (6)
- npm-update.sh
- .vscode/launch.json
- npm-upgrade.sh
- test/Lint/test.js
- .eslintrc.json
- ports/bleport.js
✅ Files skipped from review due to trivial changes (27)
- examples/async_server.js
- examples/simple_enron_serial.js
- examples/buffertcp.js
- examples/write_complete.js
- examples/server_enron_serial.js
- examples/simple_enron.js
- examples/simple_ble.js
- test/servers/servertcpCallback.test.js
- .gitignore
- examples/tcp_client_abort_signal.js
- examples/buffer.js
- ports/tcpport.js
- test/servers/servertcp.test.js
- README.md
- examples/buffered_server.js
- examples/polling_UDP.js
- examples/server_enron.js
- test/servers/servertcpPromise.test.js
- examples/simple.js
- test/servers/serverserial.test.js
- examples/server.js
- examples/device-identification.js
- examples/write.js
- test/servers/servertcp.enron.test.js
- examples/polling_TCP.js
- examples/polling_RTU.js
- examples/logger.js
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- package.json
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
Chores
Style