Repository navigation
fix(sandbox): move SpritesClient to apps/web IO boundary (fixes ERR_REQUIRE_ESM) - #1667
Merged
Merged
Conversation
@pagespace/lib compiles to CommonJS (tsconfig.build.json: module=commonjs),
so the static `import { SpritesClient } from '@fly/sprites'` in sprites.ts
was compiled to `require('@fly/sprites')` in the dist. Node.js rejects
CJS require() of an ESM-only package with ERR_REQUIRE_ESM, causing every
terminal command and every agent bash/writeFile/readFile tool call to fail
silently (caught by the route's outer try/catch, returned as 500) — no
traffic ever reached the Sprites API.
The previous fix (adding @fly/sprites to serverExternalPackages) put the
package in standalone node_modules but the docs confirm serverExternalPackages
uses native Node.js require() at runtime, which fails identically.
Fix: pure-core / IO-shell split.
- Remove the static SpritesClient import from @pagespace/lib (type-only
import type { NetworkPolicy } replaces it; erased at compile time).
- Remove defaultSdk() from lib; make sdk required in
createSpritesSandboxClient({ sdk }) so the call site must provide it.
- Add apps/web/src/lib/sandbox/sprites-client.ts: the only file that
imports @fly/sprites. Lives in apps/web where Next.js bundles it as
a server-side async webpack chunk (ESM → CJS-compatible bundle) rather
than emitting require(). SpritesClient is lazily instantiated and
cached for the process lifetime.
- Add @fly/sprites as a direct dependency of apps/web so webpack can
resolve and bundle it, and TypeScript can find its types.
- Remove @fly/sprites from serverExternalPackages (wrong fix: forces
require() which fails for ESM-only packages; webpack bundling is correct).
- Update both call sites (terminal route, sandbox-tools-runtime) to import
createProductionSpritesSandboxClient from the new app-boundary module.
- Fix stale Node 22 comment in sandbox-tools-runtime (web image is 24.x).
All 23 sprites unit tests pass. Tests inject a fake sdk and are unaffected
by the interface change.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X2ogJhXAnbtKnbGikjZz31
Contributor
📝 WalkthroughWalkthroughThe ChangesFly Sprites ESM Interop Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 004207f2e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const [store, { createProductionSpritesSandboxClient }] = await Promise.all([ | ||
| createDbTerminalSessionStore(), | ||
| import('@pagespace/lib/services/sandbox/sandbox-client/sprites'), | ||
| import('@/lib/sandbox/sprites-client'), |
There was a problem hiding this comment.
The terminal route tests still mock/import @pagespace/lib/services/sandbox/sandbox-client/sprites (checked apps/web/src/app/api/pages/[pageId]/terminal/execute/__tests__/route.test.ts), so after this import moved to @/lib/sandbox/sprites-client, the post-slot tests instantiate the real SpritesClient and happy-path cases call the real Sprites API instead of the fake client. Update the tests to mock the new app-boundary module, otherwise the web test suite becomes dependent on real Sprites credentials/network and fails or flakes.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Fixed in commits 60dd74b8b and 34a8d1a9a:
fix(terminal-tests): mock new sprites-client app boundary instead of lib path— updatedroute.test.tsto mock@/lib/sandbox/sprites-clientinstead of the old@pagespace/lib/...path, and changed frommockReturnValue(sync) tomockResolvedValue(async) to match the newawait createProductionSpritesSandboxClient()call in the route. All 21 tests in the file now pass.fix(sprites-client): correct comments — webpack bundles, not serverExternalPackages— updated both comment blocks insprites-client.tsto accurately describe the mechanism: webpack bundles@fly/spritesas a server async chunk because it is a direct dep ofapps/web(not inserverExternalPackages).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/lib/sandbox/sprites-client.ts (1)
23-39: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winMemoize in-flight SDK initialization to avoid duplicate client creation under concurrency.
Between Line 26 and Line 38, simultaneous first calls can race and create multiple
SpritesClientinstances. Memoize the initialization promise in addition tocachedSdk.Suggested refactor
let cachedSdk: SpritesSdk | null = null; +let cachedSdkPromise: Promise<SpritesSdk> | null = null; async function getSpritesSDK(): Promise<SpritesSdk> { if (cachedSdk) return cachedSdk; + if (cachedSdkPromise) return cachedSdkPromise; - // Dynamic import: `@fly/sprites` is in serverExternalPackages so webpack emits - // a native import() here rather than require() — the only form that works for - // an ESM-only package from a CJS runtime. - const { SpritesClient } = await import('`@fly/sprites`'); - const client = new SpritesClient(resolveSpritesToken()); - cachedSdk = { - getSprite: (name) => client.getSprite(name) as unknown as Promise<SpriteInstanceLike>, - createSprite: (name, config) => - client.createSprite(name, config) as unknown as Promise<SpriteInstanceLike>, - deleteSprite: (name) => client.deleteSprite(name), - }; - return cachedSdk; + cachedSdkPromise = (async () => { + const { SpritesClient } = await import('`@fly/sprites`'); + const client = new SpritesClient(resolveSpritesToken()); + cachedSdk = { + getSprite: (name) => client.getSprite(name) as unknown as Promise<SpriteInstanceLike>, + createSprite: (name, config) => + client.createSprite(name, config) as unknown as Promise<SpriteInstanceLike>, + deleteSprite: (name) => client.deleteSprite(name), + }; + return cachedSdk; + })().finally(() => { + cachedSdkPromise = null; + }); + return cachedSdkPromise; }🤖 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 `@apps/web/src/lib/sandbox/sprites-client.ts` around lines 23 - 39, The getSpritesSDK function has a race condition where concurrent calls can bypass the cachedSdk check and create multiple SpritesClient instances simultaneously. In addition to the cachedSdk variable, introduce another variable to memoize the in-flight initialization promise. Check if initialization is already in progress and return that promise, otherwise start a new initialization only when both cachedSdk is null and no in-flight promise exists. This ensures that concurrent callers will await the same initialization promise rather than creating duplicate clients.
🤖 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 `@apps/web/src/lib/sandbox/sprites-client.ts`:
- Around line 4-9: The comments in the sprites-client.ts file contain
contradictory guidance about the serverExternalPackages configuration. Update
the comment block starting at line 4 (which discusses `@fly/sprites` being
ESM-only and mentions serverExternalPackages) and the comment block at line
27-29 to correctly reflect that `@fly/sprites` should NOT be included in
serverExternalPackages, matching the actual configuration in the next.config.ts
file. Remove any references that suggest `@fly/sprites` should be added to
serverExternalPackages and clarify the current runtime model that handles the
ESM import correctly without it.
---
Nitpick comments:
In `@apps/web/src/lib/sandbox/sprites-client.ts`:
- Around line 23-39: The getSpritesSDK function has a race condition where
concurrent calls can bypass the cachedSdk check and create multiple
SpritesClient instances simultaneously. In addition to the cachedSdk variable,
introduce another variable to memoize the in-flight initialization promise.
Check if initialization is already in progress and return that promise,
otherwise start a new initialization only when both cachedSdk is null and no
in-flight promise exists. This ensures that concurrent callers will await the
same initialization promise rather than creating duplicate clients.
🪄 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: 54437d03-872d-4f11-8585-d955fe200191
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
apps/web/next.config.tsapps/web/package.jsonapps/web/src/app/api/pages/[pageId]/terminal/execute/route.tsapps/web/src/lib/ai/tools/sandbox-tools-runtime.tsapps/web/src/lib/sandbox/sprites-client.tspackages/lib/src/services/sandbox/sandbox-client/sprites.ts
…ternalPackages Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2ogJhXAnbtKnbGikjZz31
…lib path Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2ogJhXAnbtKnbGikjZz31
This was referenced Jun 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Terminal commands and agent bash/writeFile/readFile tools were silently failing — Sprites API saw zero traffic.
Root cause:
@pagespace/libcompiles to CommonJS (module: commonjsintsconfig.build.json). The staticimport { SpritesClient } from '@fly/sprites'insprites.tscompiled torequire('@fly/sprites')in the dist. Node.js rejectsrequire()of an ESM-only package withERR_REQUIRE_ESM. The route's outertry/catchcaught it and returned 500 before Sprites was ever called.The previous fix (#1665 —
serverExternalPackages: ["@fly/sprites"]) changed the error from "Cannot find package" toERR_REQUIRE_ESMbut didn't fix the underlying cause. The Next.js docs confirmserverExternalPackagesuses native Node.jsrequire()at runtime — wrong for an ESM-only package.Fix: pure-core / IO-shell split
@pagespace/libis the pure core — it should hold interfaces and transformation logic only, no IO side effects from its own imports. InstantiatingSpritesClientis an IO effect and belongs at the application boundary (apps/web), where Next.js bundles it as an ESM-aware server-side webpack chunk instead of emittingrequire().Changes
packages/lib/.../sprites.ts—import { SpritesClient }→import type { NetworkPolicy }(type-only, erased at compile time). RemovedefaultSdk(). Makesdkrequired:createSpritesSandboxClient({ sdk: SpritesSdk }).apps/web/src/lib/sandbox/sprites-client.ts(new) — the only file that imports@fly/sprites. Lazily creates and caches theSpritesClient, returns anExecSandboxClientvia the lib factory. Next.js/webpack bundles this as a server async chunk (ESM → CJS-compatible).@fly/spritesmust NOT be inserverExternalPackages— that forcesrequire()which fails for ESM-only packages.apps/web/package.json— add@fly/spritesas a direct dep so webpack can resolve and bundle it and TypeScript can find its types.next.config.ts— remove@fly/spritesfromserverExternalPackages(wrong fix: forcesrequire()at runtime). Add a comment explaining why it must not be listed there.terminal/execute/route.tsandsandbox-tools-runtime.ts— importcreateProductionSpritesSandboxClientfrom the new app-boundary module instead of from@pagespace/lib.sandbox-tools-runtime.ts(web image has been 24.x).Tests
sdkand are unaffected by the interface change.terminal/execute/route.test.tsto mock@/lib/sandbox/sprites-client(the new import path) and stubcreateProductionSpritesSandboxClientasmockResolvedValue(async, matching the route'sawaitcall). All 21 terminal route tests pass.🤖 Generated with Claude Code
https://claude.ai/code/session_01X2ogJhXAnbtKnbGikjZz31