perf: improve Core Web Vitals across iot-hub, pricing, blog, and docs#536
perf: improve Core Web Vitals across iot-hub, pricing, blog, and docs#536rusikv wants to merge 19 commits into
Conversation
The site has no View Transitions/ClientRouter, so astro:page-load never fires. Each setup already inits on DOMContentLoaded (or immediately) and is idempotent, so the extra listener was dead registration.
…nk rewrite
MailerLite universal.js + ml('load') now load on first interaction via __deferOnInteraction instead of at parse; UtmTracker's document.links rewrite runs at requestIdleCallback instead of DOMContentLoaded. Moves third-party/main-thread work out of the load/LCP window.
Re-encode reporting_12-1.webp at q80 (same 2400x1512 dims) — the LCP element on the 4.2 release post. Visually identical for a UI screenshot.
…walk Hero highlight (3s) and gallery autoplay (5s) setInterval timers start only when their element scrolls into view (IntersectionObserver), so they never run off-screen; FAQ deep-link hash walk + scrollIntoView deferred to requestIdleCallback.
GTM already defers to first interaction via __deferOnInteraction; this raises the no-interaction fallback so tag-manager + injected pixels bootstrap after the LCP window for idle visitors. Trade-off: analytics for non-interacting visitors fires up to ~2.5s later.
InstallButton now imports a tiny boot module that wires one delegated click listener and dynamic-imports the dialog on first trigger click. The dialog module no longer self-initializes; openFor() reads the stored local base on first open. Drops the iot-hub model chunk (~77KB raw) from the eager graph on detail and solution-template pages: eager JS 96.6->16.5KB raw (29.9->7.5KB gz), verified against the built output.
The 3s fallback assumed GTM's old 2.5s; bump to 5.5s so the intended GTM-first ordering holds for non-interacting visitors.
Same path and format (webp q80 / png palette), dimensions unchanged, visually verified: 7.6MB -> 1.8MB total. Two files that carried PNG bytes under a .webp extension are now real WebP. The remaining >300KB files were measured and skipped (recompression gains <20% or negative).
Blog-post body images rendered as bare <img> with no dimensions and eager loading. New rehype plugin (scoped to src/content/blog/) injects intrinsic width/height read from public/ at build time, keeps the first body image eager (usual LCP element), and lazy-loads the rest with decoding=async. Verified in the built output: docs pages byte-identical, every post <=1 eager image.
The header mega-menu inlined 50+ icon SVGs (~30KB gz) ahead of <main> on every page. Icons now live in one cached /nav-sprite.svg referenced via <use>; currentColor theming and per-icon aspect ratios are preserved. Regenerate with pnpm generate:nav-sprite after editing icons.
DocImage and ImageGallery hardcoded loading=lazy on every image, including a page's first content image — its likely LCP element. A per-render Astro.locals flag now marks the first image eager + fetchpriority=high (dark-theme twins load eager without the priority boost); everything after stays lazy.
196KB -> 180KB at q80, same dimensions.
Each of ~500 FAQ rows inlined two identical tabler SVGs (chevron + copy-link). Both are now shared CSS mask definitions colored via currentColor — about 170KB of markup and ~2,000 DOM nodes off /pricing/ with identical rendering, including the copied state.
Same rationale as the iot-hub batch: no ClientRouter, the event never fires; both scripts already init on DOMContentLoaded or immediately.
vvlladd28
left a comment
vvlladd28
left a comment
There was a problem hiding this comment.
Review summary
Reviewed 49 changed files in perf: improve Core Web Vitals across iot-hub, pricing, blog, and docs. Left 17 comment(s) inline.
The overall approach is solid and well-documented — dead astro:page-load listener removal is safe (no ClientRouter anywhere), the sprite covers all 37 nav icons with currentColor theming intact, and the defer-tier ordering (GTM 5s < GitHub button 5.5s < YourGPT 10s) is consistent. The one change I'd treat as a blocker is the MailerLite deferral in DemoRequestForm.astro: the DOMContentLoaded listener inside the deferred callback almost never fires, which silently breaks GTM demo-form tracking for first-touch visitors. The rest are smaller robustness and maintainability points.
This review was auto-generated. Findings may contain errors — please verify before applying changes.
| } | ||
|
|
||
| document.addEventListener('DOMContentLoaded', waitForForm); | ||
| document.addEventListener('DOMContentLoaded', waitForForm); |
There was a problem hiding this comment.
Now that this whole block runs inside __deferOnInteraction, it executes on first interaction or the 2.5s fallback — in both cases DOMContentLoaded has almost always already fired, so a listener registered here never runs. That means waitForForm() never executes, the MailerLite form never gets id={gtmFormId} / the gtm_form class, and GTM form tracking silently breaks — precisely for first-touch visitors landing directly on the request-demo pages (same-session navigations still work because the engaged flag makes the callback run synchronously at parse time, before DCL).
Suggest replacing the listener with a readyState gate:
if (document.readyState === 'loading') {
document.addEventListener('DOMContentLoaded', waitForForm);
} else {
waitForForm();
}There was a problem hiding this comment.
Sharp catch — this was a real regression (form tracking would silently break for first-touch visitors). Fixed with the readyState gate as suggested, and made the 2500ms fallback explicit while there (0a8d4c8).
| // never lost to a page navigation. | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| void import('@components/IotHub/install-dialog').then((m) => |
There was a problem hiding this comment.
Two rapid clicks before the import resolves both reach here (the dialog isn't open yet to block the second one), so openFor() runs twice and the second dialog.showModal() throws InvalidStateError. The old synchronous path couldn't hit this — the first click opened the modal before a second click could land on the trigger. An if (dialog.open) return guard in open() (or an in-flight flag here) would cover it.
Related: if the dynamic import rejects (flaky network), the click is silently swallowed — navigation was already prevented and there's no .catch. Worth at least a console.error, or falling back to letting the navigation proceed.
There was a problem hiding this comment.
Both fixed (e207098): open() now returns early when the dialog is already open, and a rejected import logs a console.error instead of swallowing the click.
| if (!src.startsWith('/') || src.startsWith('//')) continue; | ||
| if (props.width != null || props.height != null) continue; | ||
|
|
||
| const cleanPath = decodeURIComponent(src.split(/[?#]/)[0] ?? ''); |
There was a problem hiding this comment.
decodeURIComponent throws URIError on malformed percent-encoding — e.g. a literal % in a filename like /images/blog/50%_off.png — and since the transformer is async, that rejection fails the whole build with an unhelpful stack. Everything else in this plugin degrades gracefully; worth giving this the same treatment (try/catch → skip dimensions for that image).
There was a problem hiding this comment.
Fixed (aa4a437) — decode failure now degrades to skipping dimensions for that image, same as the other failure paths.
| // pnpm generate:nav-sprite | ||
|
|
||
| const SRC_DIR = resolve('src/assets/images/landings/nav'); | ||
| const OUT_FILE = resolve('public/nav-sprite.svg'); |
There was a problem hiding this comment.
The sprite is a committed build artifact with no freshness guard: if someone adds or edits an icon under src/assets/images/landings/nav/ and forgets pnpm generate:nav-sprite, the <use> reference silently renders an empty box — no build error, no lint failure. Since the generator is fast (37 files), could it just run as a prebuild step (and the output be gitignored), or alternatively get a CI check that regenerates and diffs, the way redirects output is kept in sync? A <!-- generated by scripts/generate-nav-sprite.mjs — do not edit --> marker inside the output would also help, matching the auto-generated-marker convention used in public/_redirects.
There was a problem hiding this comment.
Went with a middle path (473d967): the generator now runs at the start of every build script (fast — 37 files), and both outputs carry do-not-edit markers. Kept the sprite committed so pnpm dev works without a build step; a stale commit can no longer ship since builds always regenerate.
| const ariaAttrs = alt ? ` role="img" aria-label="${attr(alt)}"` : ' aria-hidden="true"'; | ||
| return `<svg ${cleaned} class="nav-icon" width="${size}"${ariaAttrs}>`; | ||
| }); | ||
| viewBox = icons[src].match(/viewBox="([^"]+)"/)?.[1]; |
There was a problem hiding this comment.
There's a subtle split-brain here: the viewBox comes from the source SVG (via the raw glob) while the geometry comes from the committed sprite, and the nav-${name} id convention is duplicated between this file and generate-nav-sprite.mjs. A stale sprite therefore renders with a fresh viewBox but old paths (or nothing at all). Consider having the generator also emit a small manifest (symbol id → viewBox) that this component imports — that makes viewBox and geometry atomically consistent, removes the id-convention duplication, drops the per-render regex over raw SVG text, and would even let you emit a content-hashed sprite filename so browsers can't serve a stale cached sprite after an icon update (public/ assets aren't fingerprinted like /_astro ones).
There was a problem hiding this comment.
Done (473d967): the generator emits a manifest (symbol id + viewBox) that NavIcon imports, so geometry and viewBox come from the same generation pass and the id convention lives in one place. Skipped the content-hashed filename for now — icons change rarely and builds regenerate the sprite — but it's a natural next step if cache staleness ever bites.
| if (typeof window !== 'undefined' && !window.__tbInstallDialogInit) { | ||
| window.__tbInstallDialogInit = true; | ||
| if (document.readyState === 'loading') { | ||
| document.addEventListener('DOMContentLoaded', boot, { once: true }); |
There was a problem hiding this comment.
The readyState gate doesn't buy anything for a document-level delegated listener — unlike the old module (which needed the DOM for readLocalBase timing), boot() here only calls document.addEventListener, which is safe at any parse stage. Registering the click listener immediately is simpler and wires the handler a moment earlier; the whole if (readyState === 'loading') branch can collapse to a single document.addEventListener('click', onTriggerClick).
There was a problem hiding this comment.
Collapsed (e207098) — the delegated listener registers directly, no readyState branch.
| if (!sourcePath.includes('/src/content/blog/')) return; | ||
|
|
||
| // Collect in document order first; async work inside a visitor is unsafe. | ||
| const images: { properties: NonNullable<Record<string, unknown>> }[] = []; |
There was a problem hiding this comment.
A few small things in an otherwise tidy plugin: NonNullable<Record<string, unknown>> is a no-op (Record is already non-nullable) — the wrapper just adds noise; the '56 posts' figure in the cache comment will silently rot as posts are added ('~dozens of posts' ages better); and since dimsCache is module-level and never invalidated, editing an image in pnpm dev serves stale width/height until a server restart — worth a one-line note in the cache comment so nobody debugs that as a bug.
There was a problem hiding this comment.
All three addressed (aa4a437): dropped the no-op NonNullable, reworded the count to age better, and documented the dev-mode cache staleness.
|
|
||
| // First content image on the page is the likely LCP element — load it | ||
| // eagerly at high priority; everything after stays lazy. | ||
| const isFirst = claimFirstContentImage(Astro.locals); |
There was a problem hiding this comment.
The claim is by render order, not viewport position: on long text-first docs pages the first DocImage can sit several screens below the fold, and fetchpriority="high" there competes for bandwidth with the resources that actually paint the fold (fonts, CSS, the text LCP). Eager-loading the first image is a reasonable heuristic, but was the high priority boost validated against pages where the first image is deep in the content? If results were mixed, a softer variant — eager loading without the fetchpriority bump — captures most of the win with none of the contention risk.
There was a problem hiding this comment.
Fair challenge — the boost wasn't validated against text-first pages, and the contention risk there is real. Adopted your softer variant (9a7b913): first image stays eager (fixing the lazy-LCP bug, which is the bulk of the win), no fetchpriority; the tradeoff is documented in the component.
| transition: color 0.15s ease; | ||
| } | ||
|
|
||
| // Drawn as a CSS mask so ~500 FAQ rows share one icon definition instead |
There was a problem hiding this comment.
Each of the two icon data-URIs is pasted twice verbatim (mask + -webkit-mask), so an icon tweak has to be applied in two places per icon and any divergence is invisible until it renders. An SCSS variable per icon ($chevron-svg: url("data:...")) or a tiny masked-icon($svg, $size) mixin would keep one copy each — and would give the same treatment an obvious home for other pages that still inline the identical tabler chevron dozens of times (e.g. src/pages/partners/affiliate.astro renders tabler:chevron-down per FAQ row today).
There was a problem hiding this comment.
Deduped into SCSS variables — one definition per icon (9a7b913). The affiliate/partners FAQ chevrons are a good candidate for the same treatment but belong to the planned FaqSection consolidation, so leaving them for that follow-up.
| width: 18px; | ||
| height: 18px; | ||
| color: var(--color-text-secondary); | ||
| background-color: currentColor; |
There was a problem hiding this comment.
One end-user regression to be aware of with the mask approach: under Windows High Contrast / forced-colors: active, background-color is overridden by the system palette in a way that can make mask-drawn glyphs invisible, whereas the previous inline SVGs got forced stroke/fill colors and stayed visible. A small @media (forced-colors: active) { .faq-chevron, .faq-copy-link::before { background-color: CanvasText; } } rule keeps the chevron and copy-link icons legible for those users.
There was a problem hiding this comment.
Good catch — added the forced-colors rule pinning both glyphs to CanvasText (9a7b913).
The deferred callback almost always runs after DOMContentLoaded, so a DCL listener registered inside it never fired and the form never received its GTM id/class for first-touch visitors. Also make the 2500ms fallback explicit: the visible demo form deliberately embeds ahead of GTM's 5s tier.
Two rapid clicks racing the dynamic import both reached showModal(), which throws on an open dialog; open() now returns early. A rejected import is logged instead of silently swallowing the click. Also simplify: reading the local base at module scope is already first-click-lazy (the boot module's import() is the only consumer), and a document-level delegated listener needs no DOM-ready gate.
decodeURIComponent throws on a literal % in a filename and would fail the whole build; degrade to skipping dimensions like every other failure path. Plus comment/type tidying (dev-mode cache staleness note, drop a no-op NonNullable).
The generator now also writes a manifest (symbol id + viewBox) that NavIcon imports, so reference and geometry come from the same generation pass instead of NavIcon re-deriving them from raw sources. Build scripts run the generator (fast, 37 files) so a stale committed sprite can't ship; outputs carry do-not-edit markers. Sprite stays committed so dev works without a build.
- DeferredLoadTrigger now carries the authoritative idle-fallback schedule; GTM and GitHubButton comments point at it instead of cross-referencing each other. - FAQ hash deep-link and UTM link rewrite get idle timeouts (500ms/1000ms) so a busy main thread can't unboundedly delay a deep-link scroll or lose click attribution. - Drop the fetchpriority boost from first content images (eager stays): on text-first docs pages a below-fold image shouldn't compete with resources painting the fold. - Gallery autoplay: the never-cleared timer already makes start one-shot; drop the redundant flag and cross-reference the two visibility-gate copies. - FAQ mask data-URIs deduped into SCSS variables and pinned to CanvasText under forced-colors so the glyphs stay visible in high-contrast modes.
2 participants
Addresses the Lighthouse/PSI performance audit (132 pages scoring 57–89; drivers: main-thread JS blocking and heavy/late images; CLS already 0 site-wide). 14 commits, grouped below.
Main-thread JS (TBT)
astro:page-loadlisteners removed (site has no ClientRouter — the event never fires).Images (LCP)
src/content/blog/): injects intrinsicwidth/height, keeps the first body image eager, lazy-loads the rest. Verified in built output: every post ≤1 eager image; sampled docs pages byte-identical (scoping proof).fetchpriority=high(DocImage/ImageGalleryhardcoded lazy on everything, including likely LCP elements); dark-theme twins load without the priority boost.HTML weight
<main>on every page; now one cached/nav-sprite.svgvia<use>,currentColortheming preserved (all 37 icons verified in both themes). Regenerate withpnpm generate:nav-sprite./pricing/208 → 166 KB gz (−20%), docs pages −41 KB raw, home 330 KB raw.Verification
astro check,eslint, and full builds green; link graph untouched.localhost, and simulated throttling degenerates against a local server). Please judge via PSI/Lighthouse on the Cloudflare preview URL and post-merge PSI. A pre-merge PSI baseline of production has been captured for before/after comparison.Not in this PR (follow-ups)