Switch to isolated-vm and event based automation scripts - Attempt 3 and final one#80
Conversation
|
Warning Review limit reached
More reviews will be available in 58 minutes and 13 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughRefactors ScriptRuntime from vm2 to isolated-vm with an injected BOOTSTRAP_SCRIPT, makes lifecycle methods async with serialized event dispatch, updates controller to await load, adjusts device test constructors and TypeScript test config, bumps Node engine/CI versions, adds esbuild, and narrows health OS types. ChangesScriptRuntime isolated-vm refactoring and integration
Device test constructor and TypeScript configuration updates
Environment, dependencies, and type updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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: 3
🧹 Nitpick comments (1)
src/automation/scriptRuntime.ts (1)
74-77: 💤 Low value
setAttributeis fire-and-forget; script won't observe failures.The isolate's
setAttributeusesapplySyncand immediately returnsPromise.resolve()(lines 74-77), while the host-side handler (lines 213-218) catches errors but doesn't propagate them back. If the underlying device rejects, the script will never know.If this is intentional for performance, consider documenting it in the bootstrap JSDoc. Otherwise, propagate errors back to the isolate.
Also applies to: 213-218
🤖 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/automation/scriptRuntime.ts` around lines 74 - 77, The setAttribute implementation is currently fire-and-forget: it calls __setAttribute.applySync(...) and always returns Promise.resolve(), so failures on the host never reach the script; fix by propagating errors: change setAttribute to call __setAttribute.applySync(...) inside a try/catch and return a resolved promise on success and a rejected promise (or rethrow) with the caught error on failure, and update the host-side attribute handler (the function that currently catches errors when handling attribute sets) to not swallow errors but to rethrow or return the error to the isolate so applySync/applyAsync can surface the failure to setAttribute.
🤖 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 @.github/workflows/test.yml:
- Line 14: CI Node test matrix excludes Node 22 while package.json allows
>=22.0.0, so either add "22.x" back into the node-version matrix entry in the
GitHub Actions workflow (the node-version array in .github/workflows/test.yml)
to test 22,24,26, or update the package.json engines.node range to ">=24.0.0
<27.0.0" to match the current matrix; pick one and make the corresponding change
so the declared engines and CI matrix are aligned.
In `@src/automation/scriptRuntime.ts`:
- Around line 259-269: In load(), the await on the lifecycle handler (via
this.pendingLifecycleDone and this.lifecycleRef!.apply('start')) can reject and
leak the created isolate, context, logWriter and runningSince; wrap that await
in a try/catch and perform a cleanup/partial-teardown when it fails: catch the
rejection from the pendingLifecycleDone promise, call the existing teardown/stop
helper (or add a small private method like partialTeardown that releases
isolate, context, clears logWriter and runningSince), rethrow the original error
so callers still see the failure, and ensure pendingLifecycleDone is cleared;
this ensures failure inside the onStart handler won’t leave resources allocated.
- Around line 316-339: runForEvent pushes a closure that dereferences
this.dispatchRef at execution time, which can race with stop() setting
dispatchRef to null; capture the reference at enqueue time (e.g. const dispatch
= this.dispatchRef) and use that captured variable inside the Promise instead of
this.dispatchRef and avoid the non-null assertion, or alternatively add a
null-check inside the queued task before calling apply; update runForEvent (and
the queued closure that calls dispatch.apply) to use the captured symbol
(dispatch) and keep existing pendingEventDone/resolve/reject behavior so
processQueue and eventQueue semantics remain unchanged.
---
Nitpick comments:
In `@src/automation/scriptRuntime.ts`:
- Around line 74-77: The setAttribute implementation is currently
fire-and-forget: it calls __setAttribute.applySync(...) and always returns
Promise.resolve(), so failures on the host never reach the script; fix by
propagating errors: change setAttribute to call __setAttribute.applySync(...)
inside a try/catch and return a resolved promise on success and a rejected
promise (or rethrow) with the caught error on failure, and update the host-side
attribute handler (the function that currently catches errors when handling
attribute sets) to not swallow errors but to rethrow or return the error to the
isolate so applySync/applyAsync can surface the failure to setAttribute.
🪄 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: 31fe3e2d-1bbf-4f72-8e61-b8d71aa0e9e3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
.github/workflows/release.yml.github/workflows/test.ymlpackage.jsonsrc/automation/scriptRuntime.tssrc/controller/automation/runScriptController.tssrc/health/healthMetricsCollector.tstests/tsconfig.jsontests/unit/automation/scriptRuntime.spec.tstests/unit/device/protocol/buttplugIo/buttplugIoDevice.spec.tstests/unit/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.spec.tstests/unit/device/protocol/virtual/display/displayVirtualDevice.spec.tstests/unit/device/testDeviceProvider.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@package.json`:
- Line 62: The engines.node range in package.json currently allows Node 24/25
but the repo depends on isolated-vm ^7.0.0 which requires Node 26.x; update the
engines.node field (package.json -> "engines.node") to restrict to Node 26 only
(for example ">=26.0.0 <27.0.0") or alternatively bump/downgrade the isolated-vm
dependency to match the Node versions you intend to support (reference
isolated-vm ^7.0.0 and the package.json "engines.node" entry when making the
change).
🪄 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: 844a1ed3-9510-4d6c-b885-324f4a965dda
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.jsonsrc/automation/scriptRuntime.ts
1 participant
Fixes #17
Summary by CodeRabbit
Chores
New Features
Tests
Health