Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR transitions Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
Signed-off-by: yaacov <kobi.zamir@gmail.com>
![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 (1)
scripts/stage-npm-flavor.js (1)
56-58: Add existence check for.d.tsfiles for consistency.Unlike
rootFiles(lines 43-45), the.d.tsfile copy loop doesn't verify file existence before copying. If any.d.tsfile is missing,fs.copyFileSyncwill throw a crypticENOENTerror rather than a clear diagnostic message.♻️ Proposed fix for consistent error handling
for (const f of dtsFiles) { + const src = path.join(root, f); + if (!fs.existsSync(src)) { + throw new Error(`Missing required .d.ts file: ${f}`); + } - fs.copyFileSync(path.join(root, f), path.join(destDir, f)); + fs.copyFileSync(src, path.join(destDir, f)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/stage-npm-flavor.js` around lines 56 - 58, The .d.ts copy loop lacks an existence check and will raise an opaque ENOENT if a file is missing; update the loop that iterates over dtsFiles to mirror the rootFiles behavior by checking fs.existsSync(path.join(root, f)) before calling fs.copyFileSync, and if the file is missing, throw or log a clear diagnostic referencing the file and destDir so callers get a helpful error instead of a raw ENOENT; keep using the same variables (dtsFiles, root, destDir, fs.copyFileSync, path.join) for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/stage-npm-flavor.js`:
- Around line 28-37: In copyPublishTree, the dirs array used to build the
package tree (in function copyPublishTree) is missing the "worker" directory
which causes apis/worker.js (which requires "../worker/index") to fail at
runtime; update the dirs array (in the same function) to include "worker" so
that the worker/index files are copied into the published package and the
require("./apis/worker") from index.js resolves correctly.
---
Nitpick comments:
In `@scripts/stage-npm-flavor.js`:
- Around line 56-58: The .d.ts copy loop lacks an existence check and will raise
an opaque ENOENT if a file is missing; update the loop that iterates over
dtsFiles to mirror the rootFiles behavior by checking
fs.existsSync(path.join(root, f)) before calling fs.copyFileSync, and if the
file is missing, throw or log a clear diagnostic referencing the file and
destDir so callers get a helpful error instead of a raw ENOENT; keep using the
same variables (dtsFiles, root, destDir, fs.copyFileSync, path.join) for
consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93d6d11d-ab6e-4aa4-9647-534228d1a0cb
📒 Files selected for processing (9)
.gitignoreModbusRTU.d.tsREADME.mdServerSerial.d.tsindex.d.tspackage.jsonscripts/clean.jsscripts/package-no-serial-port.jsonscripts/stage-npm-flavor.js
| function copyPublishTree(destDir) { | ||
| const dirs = ["apis", "ports", "servers", "utils"]; | ||
| const rootFiles = ["index.js", "README.md", "LICENSE"]; | ||
| const dtsFiles = [ | ||
| "index.d.ts", | ||
| "ModbusRTU.d.ts", | ||
| "ServerTCP.d.ts", | ||
| "ServerSerial.d.ts", | ||
| "TestPort.d.ts" | ||
| ]; |
There was a problem hiding this comment.
Critical: Missing worker directory will break published packages at runtime.
The dirs array omits the worker directory, but apis/worker.js requires ../worker/index (see apis/worker.js:1). Since index.js loads this API via require("./apis/worker"), both published package flavors will fail at runtime with a "module not found" error when the Worker class is accessed.
🐛 Proposed fix to include the worker directory
function copyPublishTree(destDir) {
- const dirs = ["apis", "ports", "servers", "utils"];
+ const dirs = ["apis", "ports", "servers", "utils", "worker"];
const rootFiles = ["index.js", "README.md", "LICENSE"];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function copyPublishTree(destDir) { | |
| const dirs = ["apis", "ports", "servers", "utils"]; | |
| const rootFiles = ["index.js", "README.md", "LICENSE"]; | |
| const dtsFiles = [ | |
| "index.d.ts", | |
| "ModbusRTU.d.ts", | |
| "ServerTCP.d.ts", | |
| "ServerSerial.d.ts", | |
| "TestPort.d.ts" | |
| ]; | |
| function copyPublishTree(destDir) { | |
| const dirs = ["apis", "ports", "servers", "utils", "worker"]; | |
| const rootFiles = ["index.js", "README.md", "LICENSE"]; | |
| const dtsFiles = [ | |
| "index.d.ts", | |
| "ModbusRTU.d.ts", | |
| "ServerTCP.d.ts", | |
| "ServerSerial.d.ts", | |
| "TestPort.d.ts" | |
| ]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/stage-npm-flavor.js` around lines 28 - 37, In copyPublishTree, the
dirs array used to build the package tree (in function copyPublishTree) is
missing the "worker" directory which causes apis/worker.js (which requires
"../worker/index") to fail at runtime; update the dirs array (in the same
function) to include "worker" so that the worker/index files are copied into the
published package and the require("./apis/worker") from index.js resolves
correctly.
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores