feat(website): SVG optimization bypass and page block observability#1514
Open
feat(website): SVG optimization bypass and page block observability#1514
Conversation
- Skip raster optimization for SVG images (vector graphics) - Add matchedPageBlock to state for x-deco-page header support
Contributor
📝 WalkthroughWalkthroughPR adds SVG-specific image handling that bypasses optimization when source ends with .svg, and introduces observability state tracking in the Fresh handler to expose matched page block names for monitoring purposes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Tagging OptionsShould a new tag be published when this PR is merged?
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@website/components/Image.tsx`:
- Around line 258-270: The SVG branch in the Image component short-circuits
before handling the preload prop, so when isSvg(props.src) is true any
preload={true} is ignored; update the SVG handling in Image.tsx to run the same
preload/link-registration logic as non‑SVG images (i.e., check props.preload and
emit the Head preload link or call the existing preload helper) before returning
the <img> for SVGs, or remove the early return and render the SVG path after
executing the preload logic so props.preload is honored while keeping
data-fresh-disable-lock, loading, ref and src usage unchanged.
In `@website/handlers/fresh.ts`:
- Around line 235-242: The CI fails deno fmt --check due to quoting and trailing
whitespace in the block that sets matched page block; update the string literals
to use double quotes and remove the trailing whitespace: in the
isFreshCtx<DecoState>(ctx) branch where you set (ctx.state as
any).matchedPageBlock from page.metadata.component, change the regex string
delimiters from single to double quotes (e.g., "^site/" and "\\.tsx$") and
ensure there is no trailing whitespace or blank characters after the closing
brace so the file passes deno fmt.
🧹 Nitpick comments (1)
website/components/Image.tsx (1)
253-254: Consider handling query strings in SVG detection.The current implementation won't detect SVG files with query parameters (e.g.,
image.svg?v=1orimage.svg?token=abc). This could cause SVGs with cache-busting or CDN parameters to still go through raster optimization.🔧 Suggested fix
-const isSvg = (src: string) => src?.toLowerCase().endsWith(".svg"); +const isSvg = (src: string) => { + if (!src) return false; + const pathWithoutQuery = src.split('?')[0]; + return pathWithoutQuery.toLowerCase().endsWith('.svg'); +};
Comment on lines
+258
to
+270
| // SVGs are vector graphics and don't need raster optimization | ||
| if (isSvg(props.src)) { | ||
| return ( | ||
| <img | ||
| {...props} | ||
| data-fresh-disable-lock | ||
| preload={undefined} | ||
| src={props.src} | ||
| loading={loading} | ||
| ref={ref} | ||
| /> | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
SVG path ignores the preload prop.
When preload={true} is passed for an SVG image, the preload <Head> link is not rendered since the early return bypasses that logic. This could regress LCP for preloaded SVG hero images.
🔧 Suggested fix to support preload for SVG
// SVGs are vector graphics and don't need raster optimization
if (isSvg(props.src)) {
return (
- <img
- {...props}
- data-fresh-disable-lock
- preload={undefined}
- src={props.src}
- loading={loading}
- ref={ref}
- />
+ <>
+ {preload && (
+ <Head>
+ <link as="image" rel="preload" href={props.src} />
+ </Head>
+ )}
+ <img
+ {...props}
+ data-fresh-disable-lock
+ preload={undefined}
+ src={props.src}
+ loading={loading}
+ ref={ref}
+ />
+ </>
);
}📝 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.
Suggested change
| // SVGs are vector graphics and don't need raster optimization | |
| if (isSvg(props.src)) { | |
| return ( | |
| <img | |
| {...props} | |
| data-fresh-disable-lock | |
| preload={undefined} | |
| src={props.src} | |
| loading={loading} | |
| ref={ref} | |
| /> | |
| ); | |
| } | |
| // SVGs are vector graphics and don't need raster optimization | |
| if (isSvg(props.src)) { | |
| return ( | |
| <> | |
| {preload && ( | |
| <Head> | |
| <link as="image" rel="preload" href={props.src} /> | |
| </Head> | |
| )} | |
| <img | |
| {...props} | |
| data-fresh-disable-lock | |
| preload={undefined} | |
| src={props.src} | |
| loading={loading} | |
| ref={ref} | |
| /> | |
| </> | |
| ); | |
| } |
🤖 Prompt for AI Agents
In `@website/components/Image.tsx` around lines 258 - 270, The SVG branch in the
Image component short-circuits before handling the preload prop, so when
isSvg(props.src) is true any preload={true} is ignored; update the SVG handling
in Image.tsx to run the same preload/link-registration logic as non‑SVG images
(i.e., check props.preload and emit the Head preload link or call the existing
preload helper) before returning the <img> for SVGs, or remove the early return
and render the SVG path after executing the preload logic so props.preload is
honored while keeping data-fresh-disable-lock, loading, ref and src usage
unchanged.
Comment on lines
+235
to
+242
| // Set matched page block name for observability headers | ||
| if (isFreshCtx<DecoState>(ctx) && page?.metadata?.component) { | ||
| // deno-lint-ignore no-explicit-any | ||
| (ctx.state as any).matchedPageBlock = page.metadata.component | ||
| .replace(/^site\//, '') | ||
| .replace(/\.tsx$/, ''); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Fix formatting to resolve pipeline failure.
The CI is failing due to deno fmt --check. The issues are:
- Single quotes should be double quotes (Deno default)
- Trailing whitespace on line 242
🔧 Proposed fix
// Set matched page block name for observability headers
if (isFreshCtx<DecoState>(ctx) && page?.metadata?.component) {
// deno-lint-ignore no-explicit-any
(ctx.state as any).matchedPageBlock = page.metadata.component
- .replace(/^site\//, '')
- .replace(/\.tsx$/, '');
+ .replace(/^site\//, "")
+ .replace(/\.tsx$/, "");
}
- 📝 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.
Suggested change
| // Set matched page block name for observability headers | |
| if (isFreshCtx<DecoState>(ctx) && page?.metadata?.component) { | |
| // deno-lint-ignore no-explicit-any | |
| (ctx.state as any).matchedPageBlock = page.metadata.component | |
| .replace(/^site\//, '') | |
| .replace(/\.tsx$/, ''); | |
| } | |
| // Set matched page block name for observability headers | |
| if (isFreshCtx<DecoState>(ctx) && page?.metadata?.component) { | |
| // deno-lint-ignore no-explicit-any | |
| (ctx.state as any).matchedPageBlock = page.metadata.component | |
| .replace(/^site\//, "") | |
| .replace(/\.tsx$/, ""); | |
| } |
🧰 Tools
🪛 GitHub Actions: ci
[error] 239-242: deno fmt --check reported: Found 1 not formatted file in 2083 files.
🤖 Prompt for AI Agents
In `@website/handlers/fresh.ts` around lines 235 - 242, The CI fails deno fmt
--check due to quoting and trailing whitespace in the block that sets matched
page block; update the string literals to use double quotes and remove the
trailing whitespace: in the isFreshCtx<DecoState>(ctx) branch where you set
(ctx.state as any).matchedPageBlock from page.metadata.component, change the
regex string delimiters from single to double quotes (e.g., "^site/" and
"\\.tsx$") and ensure there is no trailing whitespace or blank characters after
the closing brace so the file passes deno fmt.
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
1 participant
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.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.