fix: don't blindly load esm scripts#47
Conversation
📝 WalkthroughWalkthroughThis PR refactors bundled UnicornStudio SDK loading to use blob URLs and import maps for extension scripts. A new CDN URL constant is exported, blob URLs are created for bundled extensions, the model-renderer import is rewritten, and an import map redirects the model-renderer URL to the rewritten blob. The core script is injected separately. Comprehensive tests validate the new behavior. Build config enables esbuild support. ChangesBundled SDK Loading Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/shared/sdk-loader.ts (1)
129-137: Make the model-renderer import rewrite slightly more defensive (optional)
- The bundled
model-renderercontent usesfrom "./three-bundle.js"(with whitespace), so the current/from\s+["']\.\/three-bundle\.js["']/pattern matches.- Optional: switching to
\s*would still handle any future bundling/minification that removes the whitespace.🔧 Proposed fix to handle varying whitespace
return modelRendererContent.replace( - /from\s+["']\.\/three-bundle\.js["']/, + /from\s*["']\.\/three-bundle\.js["']/, `from "${threeBundleUrl}"`, );🤖 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 `@src/shared/sdk-loader.ts` around lines 129 - 137, The regex in rewriteModelRendererImport is fragile to variations in whitespace; update the pattern used in rewriteModelRendererImport to allow zero-or-more whitespace between from and the module path (use \s* instead of \s+) so minified/bundled output without a space still matches, keeping the rest of the pattern (including the escaped dot and quote handling) the same; ensure you update the replace call that uses that regex so modelRendererContent.replace(...) still substitutes with `threeBundleUrl`.
🤖 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 `@src/shared/sdk-loader.ts`:
- Around line 129-137: The regex in rewriteModelRendererImport is fragile to
variations in whitespace; update the pattern used in rewriteModelRendererImport
to allow zero-or-more whitespace between from and the module path (use \s*
instead of \s+) so minified/bundled output without a space still matches,
keeping the rest of the pattern (including the escaped dot and quote handling)
the same; ensure you update the replace call that uses that regex so
modelRendererContent.replace(...) still substitutes with `threeBundleUrl`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4711cb5-3541-42c0-aba8-c8223370e67c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
pnpm-workspace.yamlsrc/__tests__/sdk-loader.test.tssrc/shared/constants.tssrc/shared/sdk-bundle.tssrc/shared/sdk-loader.ts
|
Gonna check, sorry for the delay, my father in law passed away. |
|
Sorry to hear @diegopeixoto ! Im a little over my ski's here but it LGTM |
Addresses: #46
Summary by CodeRabbit
Tests
Chores