Repository navigation
fix(studio): allow a local GSAP dependency mirror - #6247
Conversation
Preview approval requiredThis pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Thanks for this, it's a clean fix for #6205. Reviewed at 3e45ee9dad2d. There were no earlier reviews on the PR.
What I liked:
- The base is validated once at startup and an invalid value fails loudly instead of quietly falling back to jsDelivr (
packages/core/src/gsapCdn.ts:6-17,packages/studio-server/src/routes/preview.ts:446-455). - The mirror reaches the in-editor plugin loaders through a server-written meta tag injected at the start of
<head>, so an authored meta with the same name can't take over. There's a test for that. - Root and sub-composition ETags carry the mirror, so switching mirrors can't serve stale HTML.
I checked the validation with a few inputs: file://, a Windows folder path, a backslash URL and javascript: are all rejected. .. segments are normalized and quotes and < in the path are percent-encoded. The value only comes from the server's environment, never from a request or a project file, and the server doesn't proxy anything new.
Two small things, neither blocking:
- Renders don't follow the mirror.
packages/producer/src/services/htmlCompiler.ts:1955-1967still rewrites a missing localgsap/dist/*script to jsDelivr. If someone sets the mirror because they're offline and a vendored GSAP file is missing, the preview works but an export points the script at jsDelivr and loses GSAP. Passing the same setting through, or one docs sentence saying the setting covers preview only, would avoid the surprise. - Plain HTTP on a remote host.
http://is accepted for any host. Scripts from the mirror run in the editor's page, so for anything other than localhost it would be worth either allowinghttp:only for loopback hosts or a docs line recommending HTTPS.
Nits: subComposition.ts:357 escapes the URL inline while preview.ts:84 has escapeGsapUrl; one shared helper would keep them in step. The docs could also say the mirror has to be served over HTTP, because a folder path is refused, which Windows users are likely to try first.
I relied on CI for the test runs (all green at this head) and didn't repeat the browser walk.
Verdict: APPROVE
Reasoning: The override is validated at the trust boundary, set only by whoever starts the server, covered through the real preview routes, and CI is green. The two remaining points are small follow-ups.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Thanks for this. The mirror plumbing is careful, and I have no correctness concerns at this head (3e45ee9). Miguel's review already covers the details. I agree with his two follow-ups (renders still rewrite a missing local GSAP script to jsDelivr, and plain http is accepted for remote hosts) and won't repeat them here.
Security pass for the merge queue: the change touches only source, tests and docs. There are no workflow, package.json, lockfile, install-script or dependency changes, it is a single commit, and the new tests call the routes in memory with no real network requests.
One change we'd like before merging: make local the default. As written, every Studio session still loads GSAP plugins from jsDelivr unless the user knows to set HYPERFRAMES_GSAP_BASE_URL. So both concerns in #6205 (offline use, and no third-party requests from a local editor) remain for anyone on the default. The shape we have in mind:
- The CLI ships the three files Studio adds (
gsap.min.js,CustomEase.min.js,MotionPathPlugin.min.js).packages/cli/scripts/build-copy.mjsalready copies browser assets intodist, and gsap 3.15.0 is already installed in the workspace, so this needs no new runtime dependency. - The CLI server serves them from a same-origin route using your
<root>/gsap@<version>/dist/<file>layout, next to the existing/api/runtime.jsroute, and passes that root toregisterPreviewRoutesthrough the adapter, the wayruntimeUrlis passed. - Resolution order:
HYPERFRAMES_GSAP_BASE_URLif set (unchanged, still no public fallback), then the local root, then jsDelivr only when the host supplies no local root or the requested version is not the bundled one. Other hosts that embed studio-server without the files keep today's behavior. - Tests for the default document pointing at the local root with no jsDelivr URL, and for the version-mismatch fallback, plus a docs update for the new default.
If that's more than you want to take on here, say so and we can decide between landing this as is and following up ourselves. The Before/After captures in the description are exactly what we need, thank you for those.
— Rames
| @@ -1,11 +1,27 @@ | |||
| const GSAP_CDN_VERSION = "3.15.0"; | |||
| const GSAP_CDN_BASE_URL = "https://cdn.jsdelivr.net/npm"; | |||
There was a problem hiding this comment.
This stays the default, so a user who hasn't set the env var still fetches MotionPathPlugin from jsDelivr on every Studio open, including for compositions that never use a motion path (the preload in NLEContext.tsx runs on each iframe load). Could the default come from the host instead? See the note on preview.ts for the shape. The gsap version locked in bun.lock is 3.15.0, the same as GSAP_CDN_VERSION, so files bundled with the CLI line up with the version used when none can be inferred.
| export function registerPreviewRoutes(api: Hono, adapter: PreviewApiAdapter): void { | ||
| let gsapBaseUrl: string; | ||
| try { | ||
| gsapBaseUrl = normalizeGsapBaseUrl(process.env.HYPERFRAMES_GSAP_BASE_URL); |
There was a problem hiding this comment.
This is where the local default would plug in. Suggestion: an optional GSAP root on the adapter next to runtimeUrl, resolved as env var if set, else the adapter's root, else jsDelivr. The CLI adapter in packages/cli/src/server/studioServer.ts would point it at a route that serves the bundled dist files, the way /api/runtime.js works. Your ETag salt already covers a change of root, so cached previews stay correct. One catch: normalizeGsapBaseUrl rejects root-relative paths, which is right for the env var, but a same-origin adapter value needs to pass, and the browser loaders in gsapSoftReload.ts run the meta value through the same function.
|
Update: we're landing this as is (it's in the merge queue). No need to rework it for the local-by-default suggestion above; we'll pick that up in a follow-up, along with Miguel's two notes. Thanks again. — Rames |
Studio eagerly loads MotionPathPlugin from jsDelivr even when a composition vendors GSAP locally. In networks where jsDelivr is unavailable, that generated request fails before the first motion-path edit.
Add
HYPERFRAMES_GSAP_BASE_URLfor a local or private npm-style mirror. The preview server validates and normalizes it once at API startup, then passes it through preview metadata to both iframe plugin loaders. Generated GSAP, CustomEase, MotionPathPlugin, and fallback requests use the selected mirror; root and subcomposition cache keys include that selection.The default remains jsDelivr. Runtime-selected plugin versions and existing server-side version inference stay intact. Authored resource URLs and GSAP bootstrap scripts saved into project source remain unchanged; offline projects must vendor those resources separately. Missing mirror files do not trigger a public-CDN fallback. The CLI and Studio-server docs explain the mirror layout and restart requirements.
Closes #6205.
Validation:
hyperframes lintand the browser check.Before
With GSAP vendored locally and external requests blocked, Studio reports the generated MotionPathPlugin CDN failure.
After
With
HYPERFRAMES_GSAP_BASE_URLpointing to the local mirror, the first motion-path edit seeks correctly. The browser made zero external requests.