Conversation
|
Warning Review limit reached
Next review available in: 39 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe change adds Node and browser OS platforms, supports environment configuration, connects OS operations to the WebAssembly runtime, and adds subprocess integration tests and runtime documentation. ChangesOS platform support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BallerinaRun
participant WasmBridge
participant BallerinaRuntime
participant OSPlatform
BallerinaRun->>WasmBridge: start run with env
WasmBridge->>OSPlatform: create per-run OS platform
WasmBridge->>BallerinaRuntime: run with OS platform
BallerinaRuntime->>OSPlatform: execute OS operation
OSPlatform-->>BallerinaRuntime: return result
BallerinaRuntime-->>WasmBridge: complete run
WasmBridge-->>BallerinaRun: finish cleanup
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 4
🧹 Nitpick comments (2)
packages/balrun/src/wasm-bridge.ts (1)
21-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the exported
OSPlatformtype directly.
ReturnType<typeof createOSPlatform>resolves toOSPlatform, which./osalready exports. Referencing the named type states the contract directly and does not depend on the factory signature.♻️ Proposed refactor
-import { createOSPlatform } from "./os"; +import { createOSPlatform, type OSPlatform } from "./os";interface WasmPlatform { httpListenerTransport: HTTPListenerTransport; - os: ReturnType<typeof createOSPlatform>; + os: OSPlatform; }🤖 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 `@packages/balrun/src/wasm-bridge.ts` around lines 21 - 24, Update the WasmPlatform.os property type to use the exported OSPlatform type directly, importing it from ./os as needed, instead of deriving it with ReturnType<typeof createOSPlatform>. Preserve the existing interface contract and factory usage elsewhere.packages/balrun/src/react.ts (1)
37-42: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDocument that
envmust use a stable reference.
useMemocompares dependencies withObject.is, so<BallerinaProvider env={new Map()} />creates a new dependency object on every render.optschanges each render,useEffectre-creates theBallerinainstance, and the WASM bridge re-initializes unnecessarily. Note on theenvprop that callers should memoize or hoist the value, or accept a plain record and build theMapinside the provider.🤖 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 `@packages/balrun/src/react.ts` around lines 37 - 42, Document on the `env` prop near the `BallerinaProvider` options that callers must provide a stable reference, recommending memoizing or hoisting the Map; alternatively, accept a plain record and construct the Map inside the provider. Preserve the existing `useMemo` dependency behavior and avoid unrelated changes.
🤖 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 `@packages/ballerina-wasm/os_wasm.go`:
- Around line 139-141: Update palProcess.call to validate p.proxy is a non-null
object and that the requested method property is a JavaScript function before
invoking it, returning the validation error through the existing error path.
Apply the same proxy and kill-function guards to palProcess.Kill, returning
without calling JavaScript when validation fails.
In `@packages/balrun/README.md`:
- Around line 135-138: Update the README example around the Ballerina
constructor to define a local fs filesystem instance before it is passed to new
Ballerina({ fs, env }), or provide the filesystem inline, ensuring the copied
sample has no undefined identifiers.
In `@packages/balrun/src/os.ts`:
- Around line 31-44: BrowserOSPlatform currently mutates the caller-owned
environment map, allowing state to leak between runs. In
packages/balrun/src/os.ts#L31-44, copy the supplied Environment in the
BrowserOSPlatform constructor so setEnv and unsetEnv operate on a per-run map;
packages/balrun/src/ballerina.ts#L44-44 and
packages/balrun/src/wasm-bridge.ts#L80-80 require no direct changes once this
copy is in place.
- Around line 50-60: Update getUsername, getUserHome, and exec to be async
methods so unsupportedOS() produces rejected promises rather than synchronous
throws, preserving the existing return types and unsupported-operation behavior.
---
Nitpick comments:
In `@packages/balrun/src/react.ts`:
- Around line 37-42: Document on the `env` prop near the `BallerinaProvider`
options that callers must provide a stable reference, recommending memoizing or
hoisting the Map; alternatively, accept a plain record and construct the Map
inside the provider. Preserve the existing `useMemo` dependency behavior and
avoid unrelated changes.
In `@packages/balrun/src/wasm-bridge.ts`:
- Around line 21-24: Update the WasmPlatform.os property type to use the
exported OSPlatform type directly, importing it from ./os as needed, instead of
deriving it with ReturnType<typeof createOSPlatform>. Preserve the existing
interface contract and factory usage elsewhere.
🪄 Autofix
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 Plus
Run ID: e36eea1c-5e22-4048-a639-74bf08092631
📒 Files selected for processing (13)
packages/ballerina-wasm/main_wasm.gopackages/ballerina-wasm/os_wasm.gopackages/ballerina-wasm/pal_wasm.gopackages/balrun/README.mdpackages/balrun/src/ballerina-core.tspackages/balrun/src/ballerina.tspackages/balrun/src/index.tspackages/balrun/src/os.tspackages/balrun/src/react.tspackages/balrun/src/wasm-bridge.tspackages/balrun/tests/ballerina.test.tspackages/balrun/tests/fixtures/os.balpackages/balrun/tests/wasm-bridge.test.ts
1 participant
Resolves #42
Summary by CodeRabbit
New Features
Bug Fixes
Tests