Skip to content

Fix CustomCode scriptsClientOnly not re-running scripts after hydration - #4848

Open
Manish-Builder-io wants to merge 6 commits into
mainfrom
ai_main_11d532f23f2448839fcf
Open

Manish-Builder-io wants to merge 6 commits into
mainfrom
ai_main_11d532f23f2448839fcf

Conversation

@Manish-Builder-io

@Manish-Builder-io Manish-Builder-io commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes a regression where CustomCode blocks with scriptsClientOnly: true fail to restore SSR-stripped <script> tags after hydration.

Root cause: shouldComponentUpdate in CustomCodeComponent only compared nextProps.code !== this.props.code. This meant the setState({ hydrated: true }) call made in componentDidMount (which is meant to trigger a one-time re-render to re-insert the scripts that were stripped for SSR) was blocked, since only state.hydrated changed and not props.code.

Fix:

  • Added a proper State type ({ hydrated: boolean }) and used it as the generic parameter on React.Component<Props, State> instead of leaving the component untyped for state.
  • Updated shouldComponentUpdate to also accept nextState: State and return true when nextState.hydrated !== this.state.hydrated, in addition to the existing code prop comparison. This preserves the original perf optimization (skipping re-renders when code is unchanged) while no longer suppressing the one-time hydration re-render.

Tests:

  • Added packages/react/test/custom-code.test.tsx covering:
    • Scripts are correctly stripped from SSR output when scriptsClientOnly is true.
    • Scripts are restored in the DOM and executed after hydration (verifying the fix).
    • CustomCode still skips unnecessary re-renders when neither code nor hydration state change (verifying no perf regression).
  • Updated packages/react/test/setupTests.ts to polyfill TextEncoder/TextDecoder in the jsdom test environment, which react-dom/server's renderToString requires but jsdom 19 doesn't provide.
  • Added a changeset documenting the patch fix for @builder.io/react.

Link to JIRA ticket:
https://builder-io.atlassian.net/browse/SUPP-2847


Edit in Builder  Preview


To clone this PR locally use the Github CLI with command gh pr checkout 4848

You can tag me at @BuilderIO for anything you want me to fix or change


Note

Low Risk
Narrow lifecycle fix in CustomCode with regression tests; behavior change is restoring intended client-only script execution after hydration.

Overview
Fixes a regression where Custom Code blocks with scriptsClientOnly stripped <script> tags on SSR but never put them back or executed them on the client.

shouldComponentUpdate only compared props.code, so the one-time setState({ hydrated: true }) in componentDidMount did not trigger a re-render. The change types component state as { hydrated: boolean } and allows updates when hydrated changes, while still skipping re-renders when only unrelated parent props change.

Adds jsdom tests for SSR script stripping, post-hydration script restore/execution, and a render-count guard. Test setup polyfills TextEncoder/TextDecoder for renderToString. Patch changeset for @builder.io/react.

Reviewed by Cursor Bugbot for commit 274496e. Bugbot is set up for automated code reviews on this repo. Configure here.

@builder-io-integration builder-io-integration Bot changed the title Update from the Builder.io agent Fix CustomCode scriptsClientOnly not re-running scripts after hydration Sep 7, 2026
@nx-cloud

nx-cloud Bot commented Sep 7, 2026

Copy link
Copy Markdown

View your CI Pipeline Execution ↗ for commit 80f6050

Command Status Duration Result
nx test @e2e/svelte ✅ Succeeded 7m 3s View ↗
nx test @e2e/react-sdk-next-14-app ✅ Succeeded 9m 31s View ↗
nx test @e2e/solid ✅ Succeeded 8m 32s View ↗
nx test @e2e/nextjs-sdk-next-app ✅ Succeeded 10m 57s View ↗
nx test @e2e/react ✅ Succeeded 7m 13s View ↗
nx test @e2e/angular-17 ✅ Succeeded 11m 9s View ↗
nx test @e2e/react-sdk-next-15-app ✅ Succeeded 10m 1s View ↗
nx test @e2e/react-native-76-fabric ✅ Succeeded 7m 31s View ↗
Additional runs (38) ✅ Succeeded ... View ↗

💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗


☁️ Nx Cloud last updated this comment at 2026-09-16 12:50:15 UTC

builder-io-integration[bot]

This comment was marked as outdated.

@Manish-Builder-io

Copy link
Copy Markdown
Contributor Author

@builderio-bot Review all unresolved PR comments and latest review feedback.

Be skeptical and verify each suggestion against the code and requirements before making changes.

For every comment:

  • Fix it if you agree and reply with what changed.
  • If you disagree, reply with a concise rationale.
  • Resolve the comment once addressed.
  • Leave it open only if human input is required.

Process every comment and finish with a summary of fixed, rejected, and blocked items.

@builder-io-integration

Copy link
Copy Markdown
Contributor

All checks pass. Summary of this round:

Fixed (1)

react-dom/client import in the new test — valid finding, agreed. I verified it before changing anything: packages/react/node_modules/@types/react-dom is 16.9.24 and has no client.d.ts. My typecheck was only passing because resolution fell through to the hoisted root @types/react-dom@18.2.17 — exactly the accident the bot flagged.

  • Replaced createRoot/hydrateRoot with @testing-library/react (14.1.2, already a direct devDependency): render(tree, { container, hydrate: true }) for hydration, rerender(...) for the re-render guard.
  • Dropped the now-unneeded IS_REACT_ACT_ENVIRONMENT global declaration.
  • tsc --traceResolution confirms every react-dom* specifier now resolves to the local 16.9.24 copy. react-dom/server stays (exists in @types/react-dom@16, already used by test/basic.test.tsx).

Replied on the thread and resolved it.

Found while verifying (not raised by any reviewer)

CI runs yarn g:nx test @builder.io/react -- packages/react/src, so nothing under packages/react/test/ ever executes in CI — my regression test was guarding nothing. Moved it to packages/react/src/blocks/custom-code.test.tsx and confirmed with the exact CI command (nx test @builder.io/react -- packages/react/src → 4 suites, 41 tests, pass).

Rejected: none. Blocked: none.

Verification: exact CI command passes; full package suite 48 passed; tsc --module commonjs clean (exit 0); prettier clean. Regression still proven both ways — reverting only the nextState.hydrated clause makes the hydration test fail. The two failing suites (test/basic.test.tsx, test/image.test.tsx) are pre-existing Cannot find module 'src/functions/extract-localized-values' errors from a baseUrl-style import at HEAD, outside CI's scope and unrelated.

One thing I left alone deliberately: the merge from main brought in test/variants-ab-hydration.test.tsx, which has its own local TextEncoder/TextDecoder polyfill now duplicated by the guarded one I added to test/setupTests.ts. Harmless, but I didn't touch a file that just landed — say the word if you'd like it de-duplicated.

@changeset-bot

changeset-bot Bot commented Sep 7, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 13f418b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@builder.io/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

builder-io-integration[bot]

This comment was marked as outdated.

@Manish-Builder-io
Manish-Builder-io requested review from a team and sanyamkamat and removed request for a team September 7, 2026 12:18

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this needed?

@@ -0,0 +1,5 @@
---

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do a dev release first

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sanyamkamat can we do the dev release?

@Manish-Builder-io Manish-Builder-io self-assigned this Sep 15, 2026
builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

@Manish-Builder-io

Copy link
Copy Markdown
Contributor Author

@builderio-bot Re-investigate the Custom Code scriptsClientOnly: true hydration regression from the ground up using the reported real Next.js App Router reproduction, and do not assume the proposed componentDidMount change is correct. Confirm why state.hydrated remains false, why originalRef is null in the real webpack/Next.js runtime, and how the module-scope globalReplaceNodes / document.querySelectorAll(".builder-custom-code") initialization interacts with React hydration and chunk evaluation order. Compare the real runtime behavior with the existing test and determine why the test passes while the production scenario fails. Trace the complete SSR, hydrateRoot, React hydration mismatch, componentDidMount, setState, re-render, and findAndRunScripts() flow, and identify the minimal reliable lifecycle change that ensures an SSR-stripped Custom Code block is restored and its script executes after hydration without depending on originalRef being captured. If the investigation confirms that isHydrating/originalRef is incorrectly gating the required state transition, implement the smallest targeted fix, preferably one that works in real bundled React/Next.js environments while preserving the intended behavior for non-hydration renders and existing replaceNodes logic. Add a regression test that actually exercises the problematic hydration sequence as closely as the repository can support, and strengthen the test if the current test setup masks the production issue. Validate that the fix restores the <script> after hydration and executes it, while ensuring existing Custom Code behavior is unchanged. Do not make unrelated refactors, and if the investigation disproves the proposed direction, report the confirmed root cause and implement only the evidence-backed fix.

@builder-io-integration

Copy link
Copy Markdown
Contributor

Re-investigated from scratch as requested, without assuming the original diagnosis. The reviewer's hypothesis is confirmedisHydrating/originalRef was the production trigger, and shouldComponentUpdate was a second, latent blocker. Both are load-bearing; neither alone is sufficient.

Confirmed root cause

isHydrating was return !isShopify && this.originalRef. originalRef is only populated from globalReplaceNodes, which is filled at module-evaluation time by document.querySelectorAll('.builder-custom-code') at the top of CustomCode.tsx. Next.js App Router loads the SDK chunk via an async script, so that scan can run before the streamed body containing the block has been parsed. globalReplaceNodes is then empty, originalRef stays null, and isHydrating is false.

With isHydrating === false, the code getter — (Builder.isServer || (this.isHydrating && this.firstLoad)) && scriptsClientOnly — returns the full code on the first client render, while the server rendered the stripped code. That is a dangerouslySetInnerHTML hydration mismatch, and React does not repair those: it warns and keeps the server DOM. I reproduced this exactly in jsdom:

Warning: Prop `dangerouslySetInnerHTML` did not match.
  Server: "<div class=\"cc-body\">hello</div>"
  Client: "<div class=\"cc-body\">hello</div><script>..."

componentDidMount then takes the else branch and calls findAndRunScripts() synchronously against a DOM that still holds the stripped SSR markup — zero <script> elements found. No further render is scheduled, so the scripts are never restored. That is the reported failure.

Why state.hydrated stays false: on this path setState is never reached at all, because the isHydrating branch is not taken. The shouldComponentUpdate issue only surfaces on the other path, where originalRef is captured — there setState({hydrated: true}) does fire, but the props-only sCU from #3572 drops the re-render.

Why the previous test passed while production failed: the test inserted the SSR markup into document and only then required the module, so the module-scope scan always found the node and originalRef was always captured. It exercised the second failure mode and structurally could not hit the first.

Fix

One targeted change, in isHydrating:

get isHydrating() {
  return !isShopify && Boolean(this.originalRef || this.props.scriptsClientOnly);
}

scriptsClientOnly unconditionally strips scripts on the server, so the first client render must match that regardless of whether the node was captured. This removes the dependency on originalRef entirely and routes these blocks through the intended strip → mount → restore → run sequence. The hydration-mismatch warning also disappears.

The sCU clause (nextState.hydrated !== this.state.hydrated) is retained and still required — it is what lets the mount-time setState actually re-render.

Regression test

Added restores and runs the scripts when the module loads before the SSR markup exists, which loads the module before the SSR markup is in the document, matching the production chunk-evaluation order. Verified it fails on the old isHydrating and passes on the new one. beforeEach now clears document.body so DOM state cannot leak between the two orderings.

Both fixes proven load-bearing by reverting each independently:

reverted result
isHydrating clause new ordering test fails (querySelector('script')null)
sCU nextState clause both hydration tests fail

Behavior preserved

  • Non-scriptsClientOnly: isHydrating reduces to Boolean(this.originalRef) — identical to before.
  • Shopify: isShopify short-circuits first; unchanged.
  • replaceNodes: componentDidMount guards the whole branch behind if (!this.replaceNodes), and render() omits dangerouslySetInnerHTML when replaceNodes is set, so the code getter result is unused there. Unchanged.
  • fix[gen1-react]: SUPP-685 embedded iframe reloads when page orientation changes #3572 (iframe reloads on orientation/resize): those re-renders carry unchanged props.code and no state change, so sCU still returns false.
  • Idempotency: scriptsInserted/scriptsRun still prevent double execution.
  • Only delta: a purely client-rendered scriptsClientOnly block now paints its scripts on the re-render right after mount rather than the first render, and runs them on Builder.nextTick. Same commit, before paint; no observable change.

Validation

  • npx jest src (the exact CI scope) → 42 passed, 4 suites.
  • npx tsc --module commonjs over src + test → clean, exit 0.
  • npx tslint src/blocks/CustomCode.tsx → clean; prettier --check on all touched files → clean.
  • No as any, no unrelated refactors.

@builder-io-integration

Copy link
Copy Markdown
Contributor

The reviewer was right, and the original diagnosis was incomplete.

Confirmed root cause: isHydrating was !isShopify && this.originalRef. originalRef comes from globalReplaceNodes, populated by a document.querySelectorAll('.builder-custom-code') scan at module-evaluation time. Next.js loads the SDK chunk with an async script, so that scan often runs before the streamed body is parsed — originalRef stays null, isHydrating is false, and the code getter returns the full code on the first client render while the server rendered the stripped code. I reproduced the resulting React warning in jsdom:

Warning: Prop `dangerouslySetInnerHTML` did not match.
  Server: "<div class="cc-body">hello</div>"
  Client: "<div class="cc-body">hello</div><script>..."

React does not repair dangerouslySetInnerHTML mismatches — it warns and keeps the server DOM. componentDidMount then takes the non-hydrating branch and runs findAndRunScripts() against markup that has zero scripts. setState({hydrated: true}) is never even reached, which is why state.hydrated stays false.

The old test inserted the SSR markup before requiring the module, so originalRef was always captured — it could only ever exercise the second failure mode (shouldComponentUpdate dropping the state re-render).

Fix — one line in packages/react/src/blocks/CustomCode.tsx:108:

return !isShopify && Boolean(this.originalRef || this.props.scriptsClientOnly);

The shouldComponentUpdate nextState.hydrated clause is kept; I verified by reverting each independently that both are load-bearing (without the isHydrating clause the new ordering test fails; without the sCU clause both hydration tests fail). Non-scriptsClientOnly, Shopify, and replaceNodes paths are provably unaffected, and the #3572 iframe-reload protection still holds.

Added restores and runs the scripts when the module loads before the SSR markup exists, which loads the module before the markup is in the document. CI-scoped jest src → 42 passed; tsc clean; tslint and prettier clean. Findings posted to PR #4848.

One thing to flag: the working tree also contained unrelated version downgrades in packages/react/package.json and the eight packages/sdks/output/*/package.json files, reverting the HEAD commit's 9.4.7-0 bump. I restored those to HEAD rather than let them land in this PR.

@builder-io-integration builder-io-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builder reviewed your changes — looks good ✅

Review Details

Incremental Code Review Summary

The latest update expands the CustomCode fix to cover asynchronous SDK/chunk loading, where the module-level SSR node scan can occur before markup is present. isHydrating now also treats scriptsClientOnly as sufficient to keep the initial client render script-free, allowing the hydration-state transition to restore and execute scripts afterward. The added regression test explicitly exercises module-before-markup loading, alongside the existing SSR, hydration, and render-bailout coverage.

Two parallel reviewers found no new confirmed issues. The focused four-test suite passes, the package build succeeds, and the version/changeset updates remain consistent with the repository's prerelease flow. Existing unrelated full-suite module-resolution failures were not attributed to this PR.

Risk remains standard.

🧪 Browser testing: Skipped — dependency setup is still running and no development server or proxy is available; will retry on the next incremental review.

// module was evaluated. Bundlers load the SDK chunk asynchronously, so that ordering is not
// guaranteed. `scriptsClientOnly` always strips scripts on the server, so the first client
// render has to match that regardless of whether the node was captured.
return !isShopify && Boolean(this.originalRef || this.props.scriptsClientOnly);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change kinda makes sense. I will do another dev release for this one tomorrow

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sanyamkamat Could you please do a dev release today for testing.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants