Skip to content

Rewrite ES module scripts for replay - #95

Draft
posthog[bot] wants to merge 1 commit into
tnafrom
posthog-self-driving/fixnrs-replay-rewrite-module-scripts-914816
Draft

posthog[bot] wants to merge 1 commit into
tnafrom
posthog-self-driving/fixnrs-replay-rewrite-module-scripts-914816

Conversation

@posthog

@posthog posthog Bot commented Sep 11, 2026

Copy link
Copy Markdown

Description

  • Teach the HTML and content rewriters to replay <script type="module">. The change is additive and gated on type="module", so non-module scripts keep the exact behavior they have today.
  • pywb/rewrite/regex_rewriters.py: add JSModuleRewriter, which reuses the existing URL and location rewriting but does not wrap the code in the wombat proxy block scope, and rewrites URLs with a new esm_ modifier so imported chunks replay as modules too.
  • pywb/rewrite/html_rewriter.py: rewrite inline module bodies with the module rewriter, and rewrite a module src and <link rel="modulepreload"> with esm_. Module state rides on a new _wb_parse_module flag; the parse-context name stays script so the </script> end tag still closes it.
  • pywb/rewrite/content_rewriter.py and pywb/rewrite/default_rewriter.py: content served with the esm_ modifier resolves to the module rewriter and a text/javascript MIME type (browsers reject a module served with a non-JS type).

Motivation and Context

  • A preservation client reports pages replaying poorly site-wide on the NRS QA host even though capture succeeded, and a re-crawl did not help — the shape of a replay defect, not a capture defect.
  • Root cause: every script is wrapped by JSWombatProxyRewriter in a { let window = ...; ... } block. import/export are only valid at a module's top level, so modules cannot be wrapped and were excluded from rewriting entirely (_allow_js_type rejects type="module"). Their import URLs were therefore never rewritten to the archive, so the browser fetched them live and the site content-policy header blocked them.
  • This gap exists in upstream webrecorder/pywb too (checked 2.10.0b1), so there was no upstream implementation to port.
  • The two configuration amplifiers named in the report (a narrow QA csp-header, and limit_one_per_day drift between environments) live in the mirrorweb/pfe-nrs-replay deployment repo and are handled in the companion PR mirrorweb/pfe-nrs-replay#43.

Refs WO-1734.

Screenshots (if appropriate):

Types of changes

  • Replay fix (fixes a replay specific issue)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added or updated tests to cover my changes.
  • All new and existing tests passed.

Created with PostHog Desktop from this inbox report, addressing WO-1734.

Teach the HTML and content rewriters about `<script type="module">`.

Module scripts were excluded from rewriting because the wombat proxy wraps
every script in a block scope, which is invalid around top-level
import/export. As a result module imports were never rewritten to archive
URLs and failed on replay.

Add a module path gated on `type="module"`:
- A new `esm_` URL modifier and `JSModuleRewriter` that rewrites URLs and
  location without the block-scope wrapper.
- Inline module bodies, module `src`, and `rel=modulepreload` use it.
- Served content with the `esm_` modifier resolves to the module rewriter
  and a JavaScript MIME type.

Non-module scripts keep the existing behavior.

Generated-By: PostHog Desktop
Task-Id: a1580f14-6987-4b24-a7cf-a452ceefe10d
@coderabbitai

coderabbitai Bot commented Sep 11, 2026

Copy link
Copy Markdown

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 544462b4-20b4-47d5-8fff-1babb3878cac

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@posthog

posthog Bot commented Sep 11, 2026

Copy link
Copy Markdown
Author

CI status note

The red CI / unit-tests checks are a pre-existing, repo-wide infrastructure issue, not a result of this change:

  • .github/workflows/ci.yaml runs the matrix ['3.7', '3.8', '3.9', '3.10', '3.11'] via actions/setup-python@v1. GitHub's current runner images no longer provide Python 3.7/3.8/3.9, so those jobs fail at the Set up Python step (Version 3.7 with arch x64 not found) before any test runs, and fail-fast then cancels 3.10/3.11.
  • The base branch tna shows the same failure conclusion on its own recent pushes, confirming this predates and is independent of this PR.

This change itself passes the rewrite suite locally on a supported interpreter (Python 3.12): pytest pywb/rewrite/test/ → all pass except test_brotli_not_accepted_auto_decode, which also fails on an unmodified tna checkout (an optional-dependency issue unrelated to this change).

Getting CI green needs a workflow update (drop the end-of-life Python versions and/or bump setup-python). That is a repo-wide CI-policy change outside the scope of this PR, so I have not made it here — it is a maintainer decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants