fix: address Copilot review comments on PR #1219 (pause regression tests)#1221
fix: address Copilot review comments on PR #1219 (pause regression tests)#1221passionworkeer wants to merge 5 commits intoe2b-dev:mainfrom
Conversation
When using Sandbox.connect() with an apiKey, the pause() method was not passing the apiKey to the API call, causing auth failures. This fix ensures pause() (and deprecated betaPause()) use the same pattern as kill() - merging connectionConfig with provided opts. Fixes: e2b-dev#1215
…tion - Update @returns JSDoc: pause() returns boolean (true=paused, false=already paused) matching SandboxApi.pause semantics, not a sandbox ID - Add regression tests for apiKey propagation in Sandbox.connect(): - pause() succeeds when E2B_API_KEY is unset but apiKey is passed to connect() - pause() returns false when sandbox is already paused - pause() works on connected sandbox with apiKey in connectionConfig Refs: Copilot PR e2b-dev#1218 review comments
- Remove unused template import - Change savedApiKey truthy check to explicit undefined check - Add await before Sandbox.connect() calls (P1 - async Promise fix) - Add explicit non-null assertion after toBeDefined() for TypeScript
… Copilot review) - Remove unused 'expect' import from vitest - Use explicit if-check + throw instead of expect(apiKey).toBeDefined() - This properly narrows TypeScript type for apiKey without relying on assertion return type
|
There was a problem hiding this comment.
Pull request overview
This PR follows up on the “pause regression tests” work by tightening the Vitest regression tests around apiKey handling and updating Sandbox.pause()/betaPause() to consistently forward the instance’s connectionConfig into the underlying API calls.
Changes:
- Updated
Sandbox.pause()andSandbox.betaPause()to mergethis.connectionConfiginto the options passed toSandboxApi. - Updated
pauseregression tests to remove an unusedexpectimport and replacetoBeDefined()/non-null assertions with explicit runtime guards.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/js-sdk/tests/sandbox/pause.test.ts | Adjusts pause regression tests to avoid unused imports and use explicit apiKey guards for TS narrowing. |
| packages/js-sdk/src/sandbox/index.ts | Ensures pause()/betaPause() use the sandbox instance’s connectionConfig when calling SandboxApi. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const apiKey = process.env.E2B_API_KEY || savedApiKey | ||
| if (apiKey === undefined) { | ||
| throw new Error('apiKey must be defined at this point') | ||
| } | ||
| const finalApiKey = apiKey |
There was a problem hiding this comment.
After deleting process.env.E2B_API_KEY, reading it again here is redundant (it will always be undefined). This makes the intent harder to follow; consider just using savedApiKey (and/or capture the value before deleting) so the source of finalApiKey is explicit.
| const apiKey = process.env.E2B_API_KEY || savedApiKey | |
| if (apiKey === undefined) { | |
| throw new Error('apiKey must be defined at this point') | |
| } | |
| const finalApiKey = apiKey | |
| const finalApiKey = savedApiKey | |
| if (finalApiKey === undefined) { | |
| throw new Error('apiKey must be defined at this point') | |
| } |
| /** | ||
| * Pause a sandbox by its ID. | ||
| * |
There was a problem hiding this comment.
This is an instance method, but the docstring says "Pause a sandbox by its ID", which reads like a static/ID-based API. Consider updating the wording to reflect that it pauses the current sandbox instance (e.g., "Pause this sandbox").
| async pause(opts?: ConnectionOpts): Promise<boolean> { | ||
| return await SandboxApi.pause(this.sandboxId, opts) | ||
| return await SandboxApi.pause(this.sandboxId, { ...this.connectionConfig, ...opts }) | ||
| } | ||
|
|
||
| /** | ||
| * @deprecated Use {@link Sandbox.pause} instead. | ||
| */ | ||
| async betaPause(opts?: ConnectionOpts): Promise<boolean> { | ||
| return await SandboxApi.betaPause(this.sandboxId, opts) | ||
| return await SandboxApi.betaPause(this.sandboxId, { ...this.connectionConfig, ...opts }) | ||
| } |
There was a problem hiding this comment.
The PR description lists only test-only changes, but this diff also changes runtime behavior in Sandbox.pause() / betaPause() by merging this.connectionConfig into the options passed to SandboxApi. Please update the PR description (or split the change) so reviewers are aware of the SDK behavior change.
- Remove redundant process.env.E2B_API_KEY read after env deletion (savedApiKey is always the source after delete, per Copilot feedback) - Update docstring: 'Pause a sandbox by its ID' -> 'Pause this sandbox instance' (pause() is an instance method, not a static ID-based API)
Summary
Addresses Copilot review feedback on PR #1219:
Removed unused expect import - The expect import from vitest was only used for toBeDefined() assertions which don't narrow TypeScript types. Removed since it's no longer needed.
Replaced expect(apiKey).toBeDefined() with explicit type guard - Runtime assertions don't help TypeScript's type narrowing. Used explicit if (apiKey === undefined) throw new Error(...) pattern which properly narrows the type and is more explicit about the failure mode.
Remove redundant apiKey read in test - After deleting process.env.E2B_API_KEY, reading it again is always undefined. Use savedApiKey directly to make intent explicit.
Fix JSDoc for pause() - Changed "Pause a sandbox by its ID" (sounds static) to "Pause this sandbox" to correctly describe the instance method.
Changes
Runtime behavior
Test-only
Copilot issues addressed
Closes #1219 (as a follow-up fix)