Open
Conversation
…nalyzer has a cleaner type guard
aram356
requested changes
Mar 10, 2026
Collaborator
aram356
left a comment
There was a problem hiding this comment.
Summary
Well-executed refactoring that replaces N independent appendChild/insertBefore prototype patches with a single shared dispatcher. The JS design is sound and thoroughly tested. The Rust-side GPT proxy rewrite introduces two behavioral changes that need attention before merge.
Blocking
🔧 wrench
- Redirect following widens proxy trust boundary:
build_proxy_configinheritsfollow_redirects: true, allowing the GPT proxy to follow up to 4 redirects to any host. The old code was single-hop. GPT static assets don't redirect, sofollow_redirects = falseis the correct setting. (gpt.rs:128) - Error handling semantics changed silently: Old code returned
Erron non-2xx upstream status; new code passes through the response body. This is arguably better behavior but is a semantic change that should be documented or reverted. (gpt.rs:160)
Non-blocking
🤔 thinking
- Vary header normalization dropped: Old
vary_with_accept_encodinglogic removed; relying on upstream Google CDN to always send correct Vary headers. Low practical risk but removes a defensive layer. (gpt.rs:147) - Privacy-scrubbing relies on header set-last-wins:
copy_proxy_forward_headersforwards Referer/X-Forwarded-For beforewith_headeroverrides blank them. Correct today, fragile if the proxy helper ever appends instead of sets. (gpt.rs:134) - Stale dispatcher state replaced without teardown: Version mismatch path overwrites the global without restoring old prototype wrappers. No production risk (single page load), but can stack wrappers in HMR/dev. (
dom_insertion_dispatcher.ts:81)
♻️ refactor
handleJSDoc ambiguous about node insertion: "Consumed" implies the node might not be inserted, but the dispatcher always inserts. Clarify that return value only controls handler chain short-circuiting. (dom_insertion_dispatcher.ts:28)
👍 praise
- Single shared prototype patch: The core dispatcher design eliminates wrapper stacking, makes ordering deterministic, and the test coverage (priority ordering, external patch preservation, repeated cycles, error resilience) is thorough.
- GPT auto-install on pre-set flag: Closes a real race condition between inline bootstrap and bundle evaluation. Both orderings tested.
- Test cleanup delegated to dispatcher teardown: Tests validate that teardown works rather than masking broken cleanup with manual prototype restoration.
CI Status
- cargo fmt: PASS
- cargo test: PASS
- vitest: PASS
- format-typescript: PASS
- format-docs: PASS
- CodeQL: PASS
ChristianPavilonis
requested changes
Mar 12, 2026
Collaborator
There was a problem hiding this comment.
Key items to address:
- Keep proxy redirect handling constrained in
crates/common/src/integrations/gpt.rs(do not broadly follow redirects without explicit allow-listing/trust guarantees). - Clarify and stabilize GPT proxy error semantics in
crates/common/src/integrations/gpt.rs(the non-2xx passthrough change needs either rollback or explicit documentation/tests). - Restore explicit cache/header handling in
crates/common/src/integrations/gpt.rs(Varynormalization and deterministic privacy header scrubbing should not rely on set order). - In
crates/js/lib/src/shared/dom_insertion_dispatcher.ts, ensure stale dispatcher replacement does best-effort teardown in version mismatch paths and tightenhandle()JSDoc return semantics.
Once these are addressed (or explicitly accepted with rationale), I am happy to re-review.
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
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.
Summary
appendChild/insertBeforewrappers.Changes
crates/common/src/integrations/gpt.rsAccept-Encoding, updated bootstrap comments, and refreshed proxy tests/fixtures.crates/js/lib/src/shared/dom_insertion_dispatcher.tsappendChild/insertBeforepatch, orders handlers deterministically, and restores baselines safely.crates/js/lib/src/shared/script_guard.tscrates/js/lib/src/integrations/gpt/script_guard.tscrates/js/lib/src/integrations/gpt/index.tscrates/js/lib/src/integrations/datadome/script_guard.tscrates/js/lib/src/integrations/google_tag_manager/script_guard.tscrates/js/lib/src/integrations/lockr/nextjs_guard.tscrates/js/lib/src/integrations/permutive/script_guard.tscrates/js/lib/test/shared/dom_insertion_dispatcher.test.tscrates/js/lib/test/integrations/gpt/index.test.tscrates/js/lib/test/integrations/gpt/script_guard.test.tscrates/js/lib/test/integrations/datadome/script_guard.test.tscrates/js/lib/test/integrations/google_tag_manager/script_guard.test.tscrates/js/lib/test/integrations/lockr/nextjs_guard.test.tsCloses
Closes #402
Test plan
cargo test --workspacecargo clippy --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest runcd crates/js/lib && npm run formatcd docs && npm run formatcargo build --bin trusted-server-fastly --release --target wasm32-wasip1fastly compute servecd crates/js/lib && npm run buildChecklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)