fix: proxy Snapchat bundled config loads#823
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
📝 WalkthroughWalkthroughChangesThis PR adds two new SdkPatch variants and implements AST rewrite logic for host pinning and script-loader URL rewriting. The snapchatPixel registry now applies these patches in bundle and proxy modes. Tests were expanded with new unit coverage for the rewrite logic, an integration test for bundled proxy patching, a transform test for forced bundle cache bypass, and e2e updates for the renamed bundle-patched cache and same-origin request tracking. Related runtime typing, a Google Maps model type, and fixture files were also updated. Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant NuxtScriptBundleTransformer
participant rewriteScriptUrlsAST
participant Snapchat SDK
participant Nuxt proxy (/_scripts/p)
NuxtScriptBundleTransformer->>rewriteScriptUrlsAST: bundled script + sdkPatches
rewriteScriptUrlsAST->>rewriteScriptUrlsAST: collect scriptLoaderFunctionKeys
rewriteScriptUrlsAST->>rewriteScriptUrlsAST: apply replace-new-url-host
rewriteScriptUrlsAST->>rewriteScriptUrlsAST: apply replace-script-loader-url
rewriteScriptUrlsAST-->>NuxtScriptBundleTransformer: patched script source
NuxtScriptBundleTransformer-->>Snapchat SDK: serve patched script
Snapchat SDK->>Nuxt proxy (/_scripts/p): request config via /config path
Nuxt proxy (/_scripts/p)-->>Snapchat SDK: proxied response
Related issues: Suggested labels: enhancement, bug Suggested reviewers: — 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/unit/bundle-sdk-patches.test.ts (1)
151-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded
snapchatPixelconfig duplicates registry.ts; risks drift.This inline
proxyConfigs.snapchatPixelmirrors the actual registry entry (packages/script/src/registry.ts) but isn't sourced from it, unlike the analogous suite intest/unit/rewrite-ast.test.tswhich callsgetProxyConfigs(). If the registry'ssdkPatches/domainsforsnapchatPixelchange, this test will keep passing against a stale snapshot instead of failing.♻️ Suggested refactor: derive from registry
- proxyConfigs: { - snapchatPixel: { - domains: ['sc-static.net', 'tr.snapchat.com', 'pixel.tapad.com'], - sdkPatches: [ - { type: 'replace-new-url-host', host: 'sc-static.net' }, - { type: 'replace-script-loader-url', fromDomain: 'tr.snapchat.com', pathPrefix: '/config' }, - ], - } as any, - }, + proxyConfigs: { + snapchatPixel: (await buildProxyConfigsFromRegistry(await registry())).snapchatPixel as any, + },🤖 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 `@test/unit/bundle-sdk-patches.test.ts` around lines 151 - 182, The Snapchat proxy test is hardcoding a stale `proxyConfigs.snapchatPixel` snapshot instead of using the shared registry data. Update the `applies snapchat config host patches to bundled proxy scripts` test to derive `proxyConfigs` from the registry source used elsewhere (for example via the same helper pattern as `rewrite-ast.test.ts` and `getProxyConfigs()`), so `sdkPatches` and `domains` stay aligned with `registry.ts` and future registry changes are caught.test/unit/transform.test.ts (1)
645-687: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the
hashmock or assert the cache key.vi.mocked(hash).mockImplementationOnce(() => 'scevent.min')only affects the intermediate source filename/cache key; the emitted asset URL still comes from the fetched content hash. If this test isn’t checking cache naming, the mock is unnecessary.🤖 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 `@test/unit/transform.test.ts` around lines 645 - 687, The `transform` test for `useScriptSnapchatPixel` is over-mocking `hash` in a way that only affects the intermediate cache key, not the emitted asset URL. Remove the `vi.mocked(hash).mockImplementationOnce(...)` stub unless the test explicitly asserts cache-key behavior, or update the expectations to verify the cache key produced by the `bundle` path and its interaction with `mockBundleStorage`. Keep the assertions focused on the actual `transform` output and the fetch bypass behavior.test/fixtures/first-party/nuxt.config.ts (1)
4-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGrammatically broken comment.
"Pages that need the real SDK load provide an explicit trigger" doesn't parse. Likely meant "Pages that need the real SDK should provide an explicit trigger...".
✏️ Suggested wording fix
// trigger: 'manual' prevents the auto-generated plugin from loading all 18 -// scripts globally on every page. Pages that need the real SDK load provide an -// explicit trigger in their composable call, keeping cross-provider noise low. +// scripts globally on every page. Pages that need the real SDK should provide +// an explicit trigger in their composable call, keeping cross-provider noise low.🤖 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 `@test/fixtures/first-party/nuxt.config.ts` around lines 4 - 5, The inline comment in the Nuxt config is grammatically broken and should be rewritten for clarity. Update the descriptive text near the existing explanatory comment so that the sentence about pages needing the real SDK reads naturally, using the same surrounding context in the configuration block. Keep the meaning intact while fixing the wording in the comment text itself.
🤖 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 `@test/fixtures/first-party/nuxt.config.ts`:
- Around line 4-5: The inline comment in the Nuxt config is grammatically broken
and should be rewritten for clarity. Update the descriptive text near the
existing explanatory comment so that the sentence about pages needing the real
SDK reads naturally, using the same surrounding context in the configuration
block. Keep the meaning intact while fixing the wording in the comment text
itself.
In `@test/unit/bundle-sdk-patches.test.ts`:
- Around line 151-182: The Snapchat proxy test is hardcoding a stale
`proxyConfigs.snapchatPixel` snapshot instead of using the shared registry data.
Update the `applies snapchat config host patches to bundled proxy scripts` test
to derive `proxyConfigs` from the registry source used elsewhere (for example
via the same helper pattern as `rewrite-ast.test.ts` and `getProxyConfigs()`),
so `sdkPatches` and `domains` stay aligned with `registry.ts` and future
registry changes are caught.
In `@test/unit/transform.test.ts`:
- Around line 645-687: The `transform` test for `useScriptSnapchatPixel` is
over-mocking `hash` in a way that only affects the intermediate cache key, not
the emitted asset URL. Remove the `vi.mocked(hash).mockImplementationOnce(...)`
stub unless the test explicitly asserts cache-key behavior, or update the
expectations to verify the cache key produced by the `bundle` path and its
interaction with `mockBundleStorage`. Keep the assertions focused on the actual
`transform` output and the fetch bypass behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bd56888e-76f0-4664-a05b-f77b2325e04d
📒 Files selected for processing (9)
packages/script/src/plugins/rewrite-ast.tspackages/script/src/registry.tspackages/script/src/runtime/types.tstest/e2e-dev/first-party.test.tstest/fixtures/first-party/nuxt.config.tstest/fixtures/first-party/pages/snap.vuetest/unit/bundle-sdk-patches.test.tstest/unit/rewrite-ast.test.tstest/unit/transform.test.ts
1 participant
🔗 Linked issue
Resolves #822
❓ Type of change
📚 Description
Bundled
scevent.min.jswas deriving its config host from the local asset URL, then loading/config/...from the app origin. This adds scoped SDK patches fornew URL(...).hostand script-loader URL arguments so Snapchat config loads go through thetr.snapchat.comproxy without depending on current minified names.The Snapchat first-party e2e now forces a fresh upstream bundle and fails if
/config/...is requested from the app origin. The bundled SDK regression test also derives Snapchat proxy settings from the registry so domains and patches cannot drift silently.✅ Verification
pnpm lintpasses with the existing docs passive-voice warningpnpm typecheckpnpm buildpnpm vitest run test/unit/transform.test.ts test/unit/rewrite-ast.test.ts test/unit/bundle-sdk-patches.test.ts test/unit/proxy-configs.test.tspnpm vitest run --project e2e-dev test/e2e-dev/first-party.test.ts -t snapchatPixel