update snapshot restore and sandbox snapshot APIs - #13
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 40 minutes and 22 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe changes reorganize snapshot-related functionality by moving snapshot creation from the client-level snapshots API to the sandbox instance method, renaming the resume operation to restore across the snapshots service, and migrating the CreateSnapshotParams schema definition from the snapshot module to the sandbox module with corresponding type export updates. Changes
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 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: 1
🧹 Nitpick comments (2)
src/client/sandbox.ts (1)
221-226: Add explicit return type for API consistency.Other instance methods on
Sandboxdeclare explicit return types (e.g.,pause(): Promise<this>,addMount(...): Promise<ObjectStorageMountSummary>).createSnapshotrelies on inference, which leaks theSandboxesClient.createSnapshotreturn type through and is inconsistent with the surrounding style. Consider annotating: Promise<SnapshotData>(importingSnapshotDatafrom@/models/index.js).♻️ Proposed change
async createSnapshot( params: CreateSnapshotParams = {}, options?: { timeout?: number }, - ) { + ): Promise<SnapshotData> { return this.client.sandboxes.createSnapshot(this.id, params, options); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/sandbox.ts` around lines 221 - 226, The createSnapshot method on the Sandbox class lacks an explicit return type; update its signature to return Promise<SnapshotData> and add an import for SnapshotData from "@/models/index.js" so the method matches the style of other instance methods (e.g., pause(): Promise<this>, addMount(): Promise<ObjectStorageMountSummary>) and no longer leaks the SandboxesClient.createSnapshot inferred type.tests/services/snapshots.test.ts (1)
27-36: Consider asserting the HTTP methods too.The path/body coverage is good; adding method checks would catch regressions where restore/delete accidentally switch verbs.
Test hardening diff
assert.equal(calls[0]?.path, "/v1/snapshot/restore"); + assert.equal(calls[0]?.init.method, "POST"); assert.deepEqual(jsonOf(calls[0]!), { snapshot_name: "snap-c", auto_pause: true, timeout: 12, }); assert.equal(restored.templateId, "tpl-1"); assert.equal(calls[1]?.path, "/v1/snapshot/snap-1"); + assert.equal(calls[1]?.init.method, "DELETE");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/services/snapshots.test.ts` around lines 27 - 36, The test currently asserts request paths and bodies for client.restore and client.delete but not HTTP methods; update the assertions around restored = await client.restore(...) and await client.delete(...) to also check calls[0]?.method is "POST" (or the expected verb used by SnapshotService.restore) and calls[1]?.method is "DELETE" (or the expected verb used by SnapshotService.delete") so the test verifies both path/body and HTTP verb for calls[0] and calls[1].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/models/index.ts`:
- Around line 33-39: The module re-exports listSnapshots schemas but forgot to
re-export restoreSnapshotParamsSchema alongside the RestoreSnapshotParams type;
update src/models/index.ts to export restoreSnapshotParamsSchema from
"@/models/snapshot.js" (similar to listSnapshotsParamsSchema) so consumers and
services (e.g., code using restoreSnapshotParamsSchema and
RestoreSnapshotParams) can import the validation schema and type consistently.
---
Nitpick comments:
In `@src/client/sandbox.ts`:
- Around line 221-226: The createSnapshot method on the Sandbox class lacks an
explicit return type; update its signature to return Promise<SnapshotData> and
add an import for SnapshotData from "@/models/index.js" so the method matches
the style of other instance methods (e.g., pause(): Promise<this>, addMount():
Promise<ObjectStorageMountSummary>) and no longer leaks the
SandboxesClient.createSnapshot inferred type.
In `@tests/services/snapshots.test.ts`:
- Around line 27-36: The test currently asserts request paths and bodies for
client.restore and client.delete but not HTTP methods; update the assertions
around restored = await client.restore(...) and await client.delete(...) to also
check calls[0]?.method is "POST" (or the expected verb used by
SnapshotService.restore) and calls[1]?.method is "DELETE" (or the expected verb
used by SnapshotService.delete") so the test verifies both path/body and HTTP
verb for calls[0] and calls[1].
🪄 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: 36cdf0bc-a1a9-436f-a27b-3c91d88eafb7
📒 Files selected for processing (11)
examples/snapshots.tssrc/client/sandbox.tssrc/index.tssrc/models/index.tssrc/models/sandbox.tssrc/models/snapshot.tssrc/services/sandboxes.tssrc/services/snapshots.tstests/client/client-sandbox.test.tstests/services/sandboxes-client.test.tstests/services/snapshots.test.ts
1 participant
Summary by CodeRabbit
killSandboxAfterparameter for snapshot creationresumetorestore