feat: lcp-logo-eager-loading - #3377
Conversation
Set Logo default loading to eager and route PLP critical image preload through a dedicated server head helper to improve LCP resource discovery. Add focused tests for logo loading defaults and preload metadata generation. Co-authored-by: Cursor <cursoragent@cursor.com>
WalkthroughExtracts LCP image preload logic into getCriticalProductImagePreload with tests, integrates it into ProductListingPage (renders preload via object spread), sets Navbar logos to eager, adds Logo loading tests, and reduces the useTTI timeout. ChangesLCP Preload Refactoring and Eager Loading Optimization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/src/sdk/head/useServerHeadComponents.tsx (1)
4-10: ⚡ Quick win
useServerHeadComponentsis named like a Hook but isn't one.It calls no hooks and returns JSX, so the
useprefix is misleading —eslint-plugin-react-hookstreatsuse*as Hooks and callers will expect Hook semantics. Either make it a real component or rename to a plain helper/function form.As per coding guidelines: "Custom hooks should follow React hooks conventions".♻️ Option: expose as a component
-export default function useServerHeadComponents(components: ReactNode) { +export default function ServerHeadComponents({ + components, +}: { + components: ReactNode +}) { if (!components) { return null } return <Head>{components}</Head> }🤖 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 `@packages/core/src/sdk/head/useServerHeadComponents.tsx` around lines 4 - 10, The export useServerHeadComponents is named like a React Hook but isn't one; rename or convert it: either (A) turn it into a proper component (e.g. export default function ServerHeadComponents({ components }: { components: ReactNode }) { if (!components) return null; return <Head>{components}</Head>; }) or (B) rename the helper to a non-Hook name (e.g. renderServerHeadComponents or getServerHeadElements) and keep its current signature but remove the `use` prefix; update all call sites to the new name and adjust the export accordingly. Ensure the identifier change is applied to imports/usages so ESLint no longer treats it as a Hook.packages/core/src/sdk/head/getCriticalProductImagePreload.ts (1)
20-24: ⚡ Quick winQuality alignment looks correct; remaining risk is width (320 vs 360)
getCriticalProductImagePreloadhardcodesquality: 75, andProductCard’s<Image>doesn’t passquality, sonext/image’s defaultquality(75) should reachfaststoreLoaderfor the rendered<img>as well. That means the loader’s VTEXqualityoutput (derived from 75 →customQuality) should match, so quality itself shouldn’t cause a double-fetch.The preload uses
width: 320whileProductCarddefaultswidthto360; if LCP ends up picking a different generated width/srcset entry, you could still see two requests—consider aligning the preload width with the product card’s effective rendered width for the LCP scenario.🤖 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 `@packages/core/src/sdk/head/getCriticalProductImagePreload.ts` around lines 20 - 24, getCriticalProductImagePreload currently hardcodes width: 320 which can mismatch ProductCard's default width (360) and cause LCP double-fetches; update getCriticalProductImagePreload to use the same effective width as ProductCard (e.g., change the hardcoded width to 360 or derive it from the same prop/source ProductCard uses) so the preload src matches the image's rendered/srcset entry; reference getCriticalProductImagePreload, ProductCard, and faststoreLoader to ensure the preload width and faststoreLoader call use the identical width value.
🤖 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 `@packages/core/src/sdk/head/getCriticalProductImagePreload.ts`:
- Around line 20-24: getCriticalProductImagePreload currently hardcodes width:
320 which can mismatch ProductCard's default width (360) and cause LCP
double-fetches; update getCriticalProductImagePreload to use the same effective
width as ProductCard (e.g., change the hardcoded width to 360 or derive it from
the same prop/source ProductCard uses) so the preload src matches the image's
rendered/srcset entry; reference getCriticalProductImagePreload, ProductCard,
and faststoreLoader to ensure the preload width and faststoreLoader call use the
identical width value.
In `@packages/core/src/sdk/head/useServerHeadComponents.tsx`:
- Around line 4-10: The export useServerHeadComponents is named like a React
Hook but isn't one; rename or convert it: either (A) turn it into a proper
component (e.g. export default function ServerHeadComponents({ components }: {
components: ReactNode }) { if (!components) return null; return
<Head>{components}</Head>; }) or (B) rename the helper to a non-Hook name (e.g.
renderServerHeadComponents or getServerHeadElements) and keep its current
signature but remove the `use` prefix; update all call sites to the new name and
adjust the export accordingly. Ensure the identifier change is applied to
imports/usages so ESLint no longer treats it as a Hook.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0404a940-342a-4bd4-b21f-9a3e3043f591
📒 Files selected for processing (6)
packages/core/src/components/templates/ProductListingPage/ProductListingPage.tsxpackages/core/src/components/ui/Logo/Logo.tsxpackages/core/src/sdk/head/getCriticalProductImagePreload.tspackages/core/src/sdk/head/useServerHeadComponents.tsxpackages/core/test/components/ui/Logo.browser.test.tsxpackages/core/test/sdk/head/getCriticalProductImagePreload.test.ts
Keep Logo default lazy and apply eager loading only in navbar contexts while preserving PLP critical image preload. Remove the unnecessary head wrapper and colocate preload logic with image loader plus stronger VTEX URL coverage tests. Co-authored-by: Cursor <cursoragent@cursor.com>
@faststore/api
@faststore/cli
@faststore/components
@faststore/core
@faststore/diagnostics
@faststore/lighthouse
@faststore/sdk
@faststore/ui
commit: |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/test/components/ui/Logo.browser.test.tsx (1)
19-39: 💤 Low valueConsider verifying other props are passed through.
The tests correctly verify the
loadingprop behavior. To strengthen coverage, you could also assert thataltandsrcprops flow through to the Image component.✨ Optional enhancement
it('uses lazy loading by default', () => { render(<Logo alt="FastStore" src="/logo.svg" />) expect(imageSpy).toHaveBeenCalledTimes(1) expect(imageSpy).toHaveBeenCalledWith( expect.objectContaining({ + alt: 'FastStore', + src: '/logo.svg', loading: 'lazy', }) ) })🤖 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 `@packages/core/test/components/ui/Logo.browser.test.tsx` around lines 19 - 39, Add assertions in Logo.browser.test.tsx to verify that the alt and src props are forwarded to the underlying Image by updating the two tests that render <Logo ... /> (the "uses lazy loading by default" and "keeps explicit loading overrides" cases) to also expect that imageSpy was called with an object containing the corresponding alt and src values; reference the Logo component render and the imageSpy mock when adding expect(imageSpy).toHaveBeenCalledWith(expect.objectContaining({ alt: 'FastStore', src: '/logo.svg', loading: 'lazy' })) and similarly for the eager case with loading: 'eager'.packages/core/src/components/templates/ProductListingPage/ProductListingPage.tsx (1)
124-136: ⚡ Quick winStale comment: the "320" it justifies now lives in the helper.
This block reasons about "Using 320 here," but the width moved into
getCriticalProductImagePreload. Anyone tuning 320 will look in the helper and miss this rationale. Consider trimming this to the PLP-specific "why preload the first product image" note and moving the srcset/width math next to thewidth: 320ingetCriticalProductImagePreload.ts.🤖 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 `@packages/core/src/components/templates/ProductListingPage/ProductListingPage.tsx` around lines 124 - 136, Trim the lengthy srcset/width math in ProductListingPage.tsx so the comment only explains why we preload the first product image (i.e., LCP reasoning) and remove the "Using 320 here" justification; move the detailed DPI/viewport/srcset width calculation and rationale to the helper getCriticalProductImagePreload (next to the literal width: 320) so anyone tuning that constant sees the full reasoning where it lives; update or keep the lcpImagePreload call unchanged.
🤖 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
`@packages/core/src/components/templates/ProductListingPage/ProductListingPage.tsx`:
- Around line 124-136: Trim the lengthy srcset/width math in
ProductListingPage.tsx so the comment only explains why we preload the first
product image (i.e., LCP reasoning) and remove the "Using 320 here"
justification; move the detailed DPI/viewport/srcset width calculation and
rationale to the helper getCriticalProductImagePreload (next to the literal
width: 320) so anyone tuning that constant sees the full reasoning where it
lives; update or keep the lcpImagePreload call unchanged.
In `@packages/core/test/components/ui/Logo.browser.test.tsx`:
- Around line 19-39: Add assertions in Logo.browser.test.tsx to verify that the
alt and src props are forwarded to the underlying Image by updating the two
tests that render <Logo ... /> (the "uses lazy loading by default" and "keeps
explicit loading overrides" cases) to also expect that imageSpy was called with
an object containing the corresponding alt and src values; reference the Logo
component render and the imageSpy mock when adding
expect(imageSpy).toHaveBeenCalledWith(expect.objectContaining({ alt:
'FastStore', src: '/logo.svg', loading: 'lazy' })) and similarly for the eager
case with loading: 'eager'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bbf4f84a-d4e4-47cf-80a5-8cf29a29a030
📒 Files selected for processing (6)
packages/core/src/components/navigation/Navbar/Navbar.tsxpackages/core/src/components/navigation/NavbarSlider/NavbarSlider.tsxpackages/core/src/components/templates/ProductListingPage/ProductListingPage.tsxpackages/core/src/components/ui/Image/getCriticalProductImagePreload.tspackages/core/test/components/ui/Logo.browser.test.tsxpackages/core/test/sdk/head/getCriticalProductImagePreload.test.ts
✅ Files skipped from review due to trivial changes (2)
- packages/core/src/components/navigation/NavbarSlider/NavbarSlider.tsx
- packages/core/src/components/navigation/Navbar/Navbar.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/test/sdk/head/getCriticalProductImagePreload.test.ts
Lower the TTI timeout constant from 5000ms to 1000ms to mark interactivity sooner in the performance hook behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@packages/core/src/sdk/performance/useTTI.ts`:
- Line 3: The inline comment for the TTI_TIMEOUT constant is stale (it says "5
seconds" while TTI_TIMEOUT is 1000 ms); update the comment in useTTI.ts next to
the TTI_TIMEOUT constant to reflect the actual threshold (e.g., "1 second
without long tasks as a criterion for Time To Interactive") so the text matches
the numeric value 1000 and includes the existing web.dev reference.
- Line 3: The TTI_TIMEOUT constant used by useTTI is set to 1000 but the comment
and web.dev guidance indicate a 5s quiet window; update the TTI_TIMEOUT constant
from 1000 to 5000 and revise the inline comment to reflect "5 seconds without
long tasks" (or, alternatively, if 1s is intentional, add a clear comment in
useTTI explaining why a 1000ms approximation is chosen and how long-task-only
detection differs from full TTI guidance); modify only the TTI_TIMEOUT
definition and its comment and ensure references to TTI_TIMEOUT in the useTTI
logic remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f513362-201f-44a8-b758-10daf268c9e3
📒 Files selected for processing (1)
packages/core/src/sdk/performance/useTTI.ts
| import { useEffect, useState } from 'react' | ||
|
|
||
| const TTI_TIMEOUT = 5000 // 5 seconds without long tasks as a criterion for Time To Interactive - https://web.dev/articles/tti | ||
| const TTI_TIMEOUT = 1000 // 5 seconds without long tasks as a criterion for Time To Interactive - https://web.dev/articles/tti |
There was a problem hiding this comment.
Update the stale comment.
The comment still says "5 seconds" but the constant is now 1000ms (1 second). This misleads readers about the actual TTI detection threshold.
📝 Proposed fix
-const TTI_TIMEOUT = 1000 // 5 seconds without long tasks as a criterion for Time To Interactive - https://web.dev/articles/tti
+const TTI_TIMEOUT = 1000 // 1 second without long tasks as a criterion for Time To Interactive - https://web.dev/articles/tti📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const TTI_TIMEOUT = 1000 // 5 seconds without long tasks as a criterion for Time To Interactive - https://web.dev/articles/tti | |
| const TTI_TIMEOUT = 1000 // 1 second without long tasks as a criterion for Time To Interactive - https://web.dev/articles/tti |
🤖 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 `@packages/core/src/sdk/performance/useTTI.ts` at line 3, The inline comment
for the TTI_TIMEOUT constant is stale (it says "5 seconds" while TTI_TIMEOUT is
1000 ms); update the comment in useTTI.ts next to the TTI_TIMEOUT constant to
reflect the actual threshold (e.g., "1 second without long tasks as a criterion
for Time To Interactive") so the text matches the numeric value 1000 and
includes the existing web.dev reference.
🧩 Analysis chain
🌐 Web query:
What is the recommended quiet window duration for Time to Interactive (TTI) measurement according to web.dev and web performance standards?
💡 Result:
According to web.dev and associated web performance documentation, the recommended quiet window duration for measuring Time to Interactive (TTI) is at least five seconds [1][2]. A quiet window is defined as a period of time during which there are no long tasks (tasks exceeding 50 milliseconds) and no more than two in-flight network GET requests [1][3]. It is important to note that TTI has been deprecated in modern performance auditing tools like Lighthouse (removed as of version 10) due to its high variability and sensitivity to outlier network requests and long tasks [1][4]. Official guidance now recommends using metrics such as Interaction to Next Paint (INP), Total Blocking Time (TBT), and Largest Contentful Paint (LCP) as more reliable indicators of page performance and responsiveness [1][4].
Citations:
- 1: https://web.dev/articles/tti
- 2: https://www.onely.com/blog/time-to-interactive-tti/
- 3: https://csswizardry.com/2026/06/front-ends-missing-metric-the-tbt-window/
- 4: https://developer.chrome.com/docs/lighthouse/performance/interactive
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/core/src/sdk/performance/useTTI.ts"
if [ ! -f "$FILE" ]; then
echo "Missing $FILE"
exit 1
fi
echo "---- $FILE (first 120 lines) ----"
sed -n '1,120p' "$FILE" | cat -n
echo "---- $FILE (lines around TTI_TIMEOUT) ----"
rg -n "TTI_TIMEOUT|Time To Interactive|TTI|long task|quiet" "$FILE" || trueRepository: vtex/faststore
Length of output: 2387
Fix TTI_TIMEOUT in packages/core/src/sdk/performance/useTTI.ts to match TTI quiet-window guidance
useTTIsetsTTI_TIMEOUT = 1000(1s) as the quiet period since the last long task, but the inline comment says “5 seconds”, and web.dev’s TTI guidance uses an at-least-5s quiet window (plus constraints like limited in-flight GETs).- Update
TTI_TIMEOUT(likely to5000) and the comment, or clearly document why a 1s threshold is intentional and how the approximation (long tasks only) fits your use case. - If this measurement is for performance monitoring, consider whether modern proxies (INP/TBT/LCP) better match current standards since TTI is deprecated in tools like Lighthouse.
🤖 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 `@packages/core/src/sdk/performance/useTTI.ts` at line 3, The TTI_TIMEOUT
constant used by useTTI is set to 1000 but the comment and web.dev guidance
indicate a 5s quiet window; update the TTI_TIMEOUT constant from 1000 to 5000
and revise the inline comment to reflect "5 seconds without long tasks" (or,
alternatively, if 1s is intentional, add a clear comment in useTTI explaining
why a 1000ms approximation is chosen and how long-task-only detection differs
from full TTI guidance); modify only the TTI_TIMEOUT definition and its comment
and ensure references to TTI_TIMEOUT in the useTTI logic remain unchanged.
|
0 New Issues
0 Fixed Issues
0 Accepted Issues
renatomaurovtex
left a comment
renatomaurovtex
left a comment
There was a problem hiding this comment.
Review — feat: lcp-logo-eager-loading
Nice, focused LCP work. The getCriticalProductImagePreload extraction is clean, the object-spread render in ProductListingPage is tidy, and both new test files cover the right cases (I verified quality=8 is the correct loader output for quality: 75, since faststoreLoader does ceil(min(q,100)/10)).
One change looks out of scope and needs a decision before merge — see the inline comment on useTTI.ts.
PR summary vs. implementation
The summary says "Set Logo default loading to eager". But Logo.tsx still defaults to loading = 'lazy' (and the new test asserts exactly that) — eager is opted into explicitly at the Navbar/NavbarSlider call sites. The implementation is actually the better choice (no global eager-loading of every logo). Just tweak the summary so it matches: explicit opt-in at the navbar, default stays lazy.
Nit: test colocation
test/sdk/head/getCriticalProductImagePreload.test.ts tests a helper living in src/components/ui/Image/. Moving it under test/components/ui/Image/ (next to the new Logo.browser.test.tsx) keeps the mapping obvious. Non-blocking.
Verdict: Changes requested
Blocking:
- Revert (or split out + justify, and fix the stale comment) the
TTI_TIMEOUT5000 -> 1000 change inuseTTI.ts— out of scope and a possible CWV regression.
Non-blocking:
- Align the PR summary with the actual implementation (explicit eager at call sites, default stays
lazy). - Move
getCriticalProductImagePreload.test.tsnext to its source undertest/components/ui/Image/.
Checks to confirm before merge: pnpm lint · build · the two new test files · pnpm size (touches core).
| import { useEffect, useState } from 'react' | ||
|
|
||
| const TTI_TIMEOUT = 5000 // 5 seconds without long tasks as a criterion for Time To Interactive - https://web.dev/articles/tti | ||
| const TTI_TIMEOUT = 1000 // 5 seconds without long tasks as a criterion for Time To Interactive - https://web.dev/articles/tti |
There was a problem hiding this comment.
Unrelated TTI_TIMEOUT change with a now-stale comment.
This isn't mentioned in the PR summary/deviations and doesn't relate to logo/LCP preload. Two issues:
- The comment still says "5 seconds" while the value is now
1000(1s) — they contradict each other. - Dropping the idle window 5s → 1s makes
useTTIflipisInteractivetotruemuch earlier, so anything gated behind it mounts sooner — potentially adding work inside the critical loading window and regressing LCP/CWV. Performance is NON-NEGOTIABLE here.
Was this committed by accident? If intentional, split it into its own PR with a rationale (and fix the comment); otherwise revert:
| const TTI_TIMEOUT = 1000 // 5 seconds without long tasks as a criterion for Time To Interactive - https://web.dev/articles/tti | |
| const TTI_TIMEOUT = 5000 // 5 seconds without long tasks as a criterion for Time To Interactive - https://web.dev/articles/tti |
| import { useEffect, useState } from 'react' | ||
|
|
||
| const TTI_TIMEOUT = 5000 // 5 seconds without long tasks as a criterion for Time To Interactive - https://web.dev/articles/tti | ||
| const TTI_TIMEOUT = 1000 // 5 seconds without long tasks as a criterion for Time To Interactive - https://web.dev/articles/tti |
There was a problem hiding this comment.
pelo que lembro os 5 segundos sao intencionais. 1 segundo tem risco de long task ainda serem carregadas apos esse periodo.
2 participants
Summary
Logodefault loading toeagerin@faststore/core, preserving explicit loading overrides.getCriticalProductImagePreloadto centralize preload metadata for the first PLP product image.useServerHeadComponentsand updatedProductListingPageto inject critical image preload link through the server head helper.Tests
pnpm --filter @faststore/core exec vitest run test/components/ui/Logo.browser.test.tsx test/sdk/head/getCriticalProductImagePreload.test.tsDeviations
specs/lcp-logo-eager-loading.mdis located under a symlink target outside this git repository, so its status update (Approved->Done) could not be included in this branch commit.Spec
specs/lcp-logo-eager-loading.mdMade with Cursor
Summary by CodeRabbit
New Features
Tests