feat: support Vercel sandbox resources#723
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 selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds Vercel Sandbox resource support: computes requested vCPUs from ChangesVercel Sandbox Resource Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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 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 |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
![open-inspect[bot]](https://avatars.githubusercontent.com/u/2492022?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
PR #723 (feat: support Vercel sandbox resources) by @ColeMurray adds Vercel sandbox resource serialization plus provider mapping from cpuCores/memoryMib to resources.vcpus. The implementation is mostly clean and well-tested for normal supported sizes, but one edge case can send unsupported Vercel resource values.
Files changed: 6, +78/-14.
Critical Issues
- [Functionality]
packages/control-plane/src/sandbox/providers/vercel/provider.ts:638- Requests above Vercel's documented maximum can serialize unsupportedresources.vcpusvalues such as9, instead of rejecting clearly or using a supported shape. I left an inline comment with an example and suggested fixes.
Suggestions
- Add a test covering resource requests above the maximum supported Vercel size so the intended behavior is explicit.
Nitpicks
- None.
Positive Feedback
- The create and restore paths both use the same resource mapping, which keeps behavior consistent.
- The client test verifies
resourcesis serialized through the REST payload. - The docs were updated to explain the provider-specific CPU/memory mapping and default behavior.
Questions
- None beyond the inline comment on the above-max resource behavior.
Verdict
Request Changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/control-plane/src/sandbox/providers/vercel/provider.ts (1)
625-639: 💤 Low valueConsider removing redundant
?? undefinedon lines 628-629.The expressions
sandboxSettings?.cpuCores ?? undefinedandsandboxSettings?.memoryMib ?? undefinedare redundant because when the left-hand side is nullish (includingnull), the result is alreadyundefinedwithout the explicit coalescing. You could simplify to:const requestedCpuCores = sandboxSettings?.cpuCores; const requestedMemoryMib = sandboxSettings?.memoryMib;This doesn't change behavior since both are later coalesced to
0on line 634, but it's slightly cleaner.The overall logic correctly handles all edge cases:
- Fractional
cpuCoresvalues- Combined CPU and memory constraints (using the higher derived vCPU count)
- Explicit
nullvalues (treated as unset, returningundefinedto omitresources)- Out-of-range requests (fallback to
Math.ceil, letting Vercel enforce limits per the documented design)🤖 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/control-plane/src/sandbox/providers/vercel/provider.ts` around lines 625 - 639, In resolveVercelResources, remove the redundant "?? undefined" on the sandboxSettings?.cpuCores and sandboxSettings?.memoryMib assignments and simply assign requestedCpuCores = sandboxSettings?.cpuCores and requestedMemoryMib = sandboxSettings?.memoryMib; this preserves existing semantics (they later coalesce to defaults) and cleans up the code while leaving the rest of the function (vcpusForMemory calculation, requestedVcpus, supportedVcpus lookup, and return) unchanged.
🤖 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.
Nitpick comments:
In `@packages/control-plane/src/sandbox/providers/vercel/provider.ts`:
- Around line 625-639: In resolveVercelResources, remove the redundant "??
undefined" on the sandboxSettings?.cpuCores and sandboxSettings?.memoryMib
assignments and simply assign requestedCpuCores = sandboxSettings?.cpuCores and
requestedMemoryMib = sandboxSettings?.memoryMib; this preserves existing
semantics (they later coalesce to defaults) and cleans up the code while leaving
the rest of the function (vcpusForMemory calculation, requestedVcpus,
supportedVcpus lookup, and return) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45e622ac-6377-4293-846c-2e1206f4e9e4
📒 Files selected for processing (6)
docs/VERCEL_SANDBOX_PROVIDER.mdpackages/control-plane/src/sandbox/providers/vercel/client.test.tspackages/control-plane/src/sandbox/providers/vercel/client.tspackages/control-plane/src/sandbox/providers/vercel/provider.test.tspackages/control-plane/src/sandbox/providers/vercel/provider.tspackages/shared/src/types/integrations.ts
Terraform Validation Results
Pushed by: @open-inspect[bot], Action: |
1 participant
Summary
cpuCoresandmemoryMibsettings to Vercelresources.vcpuson create and restore.resourcesthrough the REST client and cover the provider/client behavior with tests.Testing
npm test -w @open-inspect/control-plane -- src/sandbox/providers/vercel/provider.test.ts src/sandbox/providers/vercel/client.test.tsnpm run build -w @open-inspect/sharednpm run typecheck -w @open-inspect/control-planeCreated with Open-Inspect
Summary by CodeRabbit
New Features
Tests
Documentation