fix: give ExtensionPoint's SSR placeholder its own tree-path context - #687
Open
iago1501 wants to merge 2 commits into
Open
fix: give ExtensionPoint's SSR placeholder its own tree-path context#687iago1501 wants to merge 2 commits into
iago1501 wants to merge 2 commits into
Conversation
Loading resolves its extension via useExtension(), which reads whatever tree path is currently in context. The bare <Loading /> passed as NoSSR's onSSR fallback rendered outside any TreePathContextProvider for this block (ComponentLoader only installs the provider inside the loaded content), so it read the *parent's* tree path and drew the *parent's* preview instead of its own (or none, if it has none). On pages where a page-level block declares a large preview (e.g. a 1400px box) and has client-rendered children with no preview of their own, SSR emits the parent's oversized placeholder for each such child, which then collapses to its real (often much smaller, sometimes zero) height on hydration -- a large, page-load-time layout shift. Measured on Olympikus's home page: CLS 0.1071 -> 0.0000 (5/5 runs, live A/B on production, mobile/throttled), FCP/LCP/extension count/DOM node count unchanged. Verified independently at the unit level through the real ReactDOMServer renderer (jsdom can't reach this branch, since NoSSR's layout effect fires there and renders children instead): pre-fix SSR emits the parent's 460px loading-preview marker; post-fix output is empty, as expected for a child with no preview of its own. Adds a regression test mirroring the existing 'no extension for current tree path' test. tsc --noEmit is clean. Known toolchain caveat: the local test suite has 4 pre-existing failures unrelated to this change (pinned @babel/parser 7.7.7 chokes on 'import type' in already-committed code), so the new test could not be executed locally -- verified via the compiled SSR harness instead; CI should run it. Also flags a second, uncharacterized CLS source for follow-up: LazyRender's hardcoded height=400 placeholder causes shifts on scroll for below-fold content of a different height (scroll-arm CLS 0.527 -> 0.399 was noise- dominated with only 3 runs, not treated as a solid result here). Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖 Please select which version do you want to release:
And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.
|
There was a problem hiding this comment.
🟡 Changes recommended
The new SSR test uses renderToString() under Jest/JsDOM where window exists, which can make NoSSR select useLayoutEffect and produce ReactDOMServer warnings/noise unless the test simulates a real SSR environment.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes a CLS issue in ExtensionPoint SSR by ensuring the client-rendered (render: "client") loading placeholder (<Loading />) resolves preview sizing from the block’s own tree-path context instead of inheriting the parent’s tree path (and thus the parent’s preview dimensions).
Changes:
- Wrap
NoSSR’sonSSRplaceholder withTreePathContextProviderscoped tonewTreePath. - Add a regression test that server-renders an
ExtensionPointto ensure a client-rendered child with no preview does not render the parent preview placeholder. - Document the fix in the changelog.
File summaries
| File | Description |
|---|---|
| react/components/ExtensionPoint/index.tsx | Wraps the SSR placeholder in TreePathContextProvider using the block’s newTreePath to prevent inheriting parent preview size. |
| react/components/ExtensionPoint/ExtensionPoint.test.tsx | Adds an SSR regression test verifying parent preview isn’t rendered for a client-only child with no preview. |
| CHANGELOG.md | Adds an Unreleased “Fixed” entry describing the SSR placeholder tree-path scoping fix and its CLS impact. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+68
to
+78
| /* Has to go through the server renderer: NoSSR only takes its onSSR branch there, | ||
| * because under jsdom the layout effect runs and the children render instead. */ | ||
| const html = renderToString( | ||
| <RenderContextProvider | ||
| runtime={{ extensions: mockExtensions, getSettings: () => ({}) } as any} | ||
| > | ||
| <TreePathContextProvider treePath="store.search#category/search-result#category"> | ||
| <ExtensionPoint treePath="" id="search-title" /> | ||
| </TreePathContextProvider> | ||
| </RenderContextProvider> | ||
| ) |
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.
What does this PR do? *
Fixes a CLS bug in
ExtensionPoint: its SSR loading placeholder (<Loading />) rendered outside its own tree-path context, so it inherited the parent block's preview size instead of its own. On pages where a page-level block declares a large preview (e.g. a 1400pxbox) withclient-rendered children that have no preview of their own, SSR emitted the parent's oversized placeholder for each child — which then collapsed to its real height on hydration, causing a large layout shift.Fix: wrap the placeholder in a
TreePathContextProviderscoped to the block's own tree path.📊 Results — tested live on 4 real store accounts
CLS fully eliminated on the two stores that actually have the bug pattern; correctly no-ops on the two that don't. FCP/LCP/DOM node count unchanged everywhere.
How to test it? *
ExtensionPoint.test.tsx;tsc --noEmitclean.vtex linkagainst a dedicated dev workspace on each store's account (baseline vs. fix, same workspace) — never against live production traffic.ReactDOMServer(jsdom can't reach this branch): pre-fix SSR emits the parent's 460px preview marker; post-fix output is empty, as expected.Known toolchain caveat
The local suite has 4 pre-existing failures unrelated to this change (pinned
@babel/parser7.7.7 can't parseimport typein already-committed code), so the new test couldn't run locally — verified instead via the compiled SSR harness. CI should run it for real.Describe alternatives you've considered, if any. *
None — straightforward context-scoping fix to existing
NoSSR/Loadingwiring, not a design trade-off.Related to / Depends on *
Follow-up candidate (not part of this PR):
LazyRender's hardcodedheight=400placeholder can also shift layout for below-fold content of a different real height. Not solidly measured yet (only 3 noisy runs).