Added Astro Compatibility#5
Open
fufulog wants to merge 8 commits into
Open
Conversation
…of outputting raw which sometimes breaks display
…component preview. Astro unfortunately precompiles to static html meaning that its not as simple to handle the offsets when finding where to take the screenshot/ display in the editor on hover. I am sure there are issues still with respect to offsets. However i have it working in our production repository fine. The main thing this means is that we cannot take advantage of vite injection and have to manually do injection and parsing to determine the offsets for astro. To resolve some of the more tenacious issues, this filters out script tags completely. So while it will display lightbox and other javascript components if you preview an outer div. It may fail to preview the lightbox itself. Not 100% sure how to fix that. But its very workable as i can always hover an outer div. With respect to raw html style nested elements it works without issues (as far as i can tell). fixes: fixed an issues with the screenshot pipeline, if a resize falls outside the size parameter, it does not get re-compressed and instead just shows a giant image data blob. Now it correctly compresses the fallback to display an image.
📝 WalkthroughWalkthroughAdds Astro support across the extension and vite plugin, plus Astro-aware dev-server detection, route resolution, hover/onboarding updates, and screenshot capture timeout handling. ChangesAstro Support Implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant vitePlugin as vite-plugin index.ts
participant transformer as transformAstro
participant fs as filesystem
participant parser as Astro parser
participant injector as injectCompiledAstro
vitePlugin->>transformer: transform(code, sourceId=".astro")
transformer->>fs: read original .astro source
fs-->>transformer: source content or read error
transformer->>parser: parse(source, position: true)
parser-->>transformer: AST
transformer->>transformer: walk AST, compute injection points
alt compiled code detected
transformer->>injector: injectCompiledAstro(code, points, sourceId)
injector-->>transformer: code + sourcemap
else non-compiled code
transformer->>transformer: injectPreviewAttributes(code, points)
end
transformer-->>vitePlugin: TransformResult or null
sequenceDiagram
participant renderer as renderFromDevServer
participant detector as detectDevServer
participant routeUtil as resolveRouteForFile
participant page as Playwright Page
renderer->>detector: detectDevServer(workspaceRoot)
detector->>detector: check astro.config.*, package.json, and localhost candidates
detector-->>renderer: devServerUrl
renderer->>routeUtil: resolveRouteForFile(workspaceRoot, filePath)
routeUtil-->>renderer: route
renderer->>page: goto(devServerUrl + route, waitUntil: "load")
page-->>renderer: navigation complete
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/screenshotPipeline.ts (1)
63-70: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winScreenshot timeouts aren't soft-handled, unlike the scroll call.
scrollIntoViewIfNeedederrors are swallowed (Line 60), but the per-quality-steptarget.screenshot(...)/img.screenshot(...)calls (Lines 66, 86) have no try/catch. If any single quality step exceeds the 1000ms timeout, the exception propagates out ofcaptureAdaptiveJpegentirely — discarding any already-captured valid buffer (buf/fallbackBuf) from a prior successful iteration and forcing the whole hover preview to fail (buildErrorHover()upstream) instead of returning a usable, if lower-quality, screenshot. This also contradicts the stated fallback behavior of "returning the last captured buffer" when no quality step fitsMAX_BYTES— that guarantee only holds if none of the intermediate calls throw.🔧 Proposed fix: soft-handle per-iteration screenshot errors
try { let buf: Buffer = Buffer.alloc(0); for (const quality of QUALITY_STEPS) { - buf = await target.screenshot({ type: "jpeg", quality, animations: "disabled", timeout: 1000 }); - if (buf.length <= MAX_BYTES) { - return buf; + try { + buf = await target.screenshot({ type: "jpeg", quality, animations: "disabled", timeout: 1000 }); + if (buf.length <= MAX_BYTES) { + return buf; + } + } catch { + continue; } }and similarly for the fallback
img.screenshotloop.Also applies to: 84-91
🤖 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 `@src/screenshotPipeline.ts` around lines 63 - 70, Soft-handle per-iteration screenshot failures in captureAdaptiveJpeg so a timeout on one quality step does not abort the whole hover preview. Add try/catch around the target.screenshot loop and the img.screenshot fallback loop, similar to scrollIntoViewIfNeeded, and continue to the next QUALITY_STEPS entry while preserving and returning the last successful buf/fallbackBuf when later attempts fail.src/devServerRenderer.ts (1)
227-240: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSerialize access to the shared dev page
renderFromDevServernow re-navigates the module-globaldevPageper file, so concurrent hover requests for different routes can clobber each other betweengoto, adapter detection, element lookup, and screenshot capture. Add a lock/queue here or use a per-request page to avoid wrong-route renders and detached-page errors.🤖 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 `@src/devServerRenderer.ts` around lines 227 - 240, renderFromDevServer is using the shared module-global devPage without any concurrency control, so overlapping requests can overwrite each other’s navigation and break adapter detection, element lookup, or screenshot capture. Add serialization around the full render path in renderFromDevServer, or switch it to a per-request page instance, so only one route render runs at a time for a given dev page. Keep the protection around getDevPage, the page.goto navigation, and the downstream render steps that depend on the same page state.
🧹 Nitpick comments (3)
packages/vite-plugin-component-preview/src/transformAstro.ts (1)
20-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
@astrojs/compiler/utils's built-inwalk/ishelpers over a hand-rolled traversal.
@astrojs/compilerships an officialwalk/walkAsync/isutility from@astrojs/compiler/utilsfor traversing the AST and type-guarding tag nodes. Using it instead of this custom recursivewalk(and the manualINJECTABLE_TYPES/is.tag-style check inisInjectableAstroNode) keeps this transformer aligned with upstream AST-shape changes and reduces custom code to maintain.♻️ Suggested direction
-import { parse } from "`@astrojs/compiler`"; +import { parse } from "`@astrojs/compiler`"; +import { walk, is } from "`@astrojs/compiler/utils`";Then replace the local
walkfunction's call sites with the imported one, and useis.tag(node)inisInjectableAstroNodeinstead of the manualINJECTABLE_TYPESset check.🤖 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/vite-plugin-component-preview/src/transformAstro.ts` around lines 20 - 33, Replace the custom AST traversal in transformAstro.ts with the official `@astrojs/compiler/utils` helpers. Update the local walk function call sites to use the imported walk (or walkAsync if needed), and change isInjectableAstroNode to use is.tag(node) instead of the manual INJECTABLE_TYPES membership check. Keep the logic centered around walk, isInjectableAstroNode, and INJECTABLE_TYPES so the transformer follows upstream AST utilities.src/devServerDetector.ts (1)
218-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate logic with
candidatesFromViteConfig.
candidatesFromAstroConfig(lines 218-243) is structurally identical tocandidatesFromViteConfig(lines 191-216) — same read/regex/fallback flow, differing only in the file-name list and default port. Consider extracting a shared helper to avoid drift between the two config parsers.♻️ Suggested consolidation
+async function candidatesFromConfigFile( + workspaceRoot: string, + names: string[], + defaultPort: number, +): Promise<string[]> { + const candidates: string[] = []; + for (const name of names) { + const filePath = path.join(workspaceRoot, name); + try { + const content = await detectorDeps.readFileUtf8(filePath); + const match = content.match(/\bport\s*:\s*(\d{2,5})\b/); + candidates.push(`http://localhost:${match?.[1] ?? defaultPort}`); + } catch { + // Optional hint file. + } + } + return candidates; +} + async function candidatesFromViteConfig(workspaceRoot: string): Promise<string[]> { - const names = [...]; - const candidates: string[] = []; - for (const name of names) { ... } - return candidates; + return candidatesFromConfigFile( + workspaceRoot, + ["vite.config.ts", "vite.config.js", "vite.config.mjs", "vite.config.cjs"], + 5173, + ); } async function candidatesFromAstroConfig(workspaceRoot: string): Promise<string[]> { - const names = [...]; - const candidates: string[] = []; - for (const name of names) { ... } - return candidates; + return candidatesFromConfigFile( + workspaceRoot, + ["astro.config.ts", "astro.config.js", "astro.config.mjs", "astro.config.cjs"], + 4321, + ); }🤖 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 `@src/devServerDetector.ts` around lines 218 - 243, `candidatesFromAstroConfig` duplicates the same read/regex/fallback flow as `candidatesFromViteConfig`, so consolidate the shared parsing logic into a helper and parameterize only the config file names and default port. Keep the config-specific wrappers (`candidatesFromAstroConfig` and `candidatesFromViteConfig`) thin, and reuse the extracted helper for the `detectorDeps.readFileUtf8`/port extraction behavior to prevent future drift.src/devServerRenderer.ts (1)
62-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate of
pluginOnboarding.ts'sisPluginOnlyFrameworkFile.
fileIsPluginOnlyhere uses the exact same regex (/\.(vue|svelte|astro)$/i) asisPluginOnlyFrameworkFileinpluginOnboarding.ts. Consider importing and reusing that function instead of maintaining two copies that could silently drift.♻️ Suggested fix
-function fileIsPluginOnly(filePath: string): boolean { - return /\.(vue|svelte|astro)$/i.test(filePath); -} +import { isPluginOnlyFrameworkFile as fileIsPluginOnly } from "./pluginOnboarding";🤖 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 `@src/devServerRenderer.ts` around lines 62 - 64, The file-only framework check is duplicated between fileIsPluginOnly and pluginOnboarding.ts’s isPluginOnlyFrameworkFile, so reuse the existing helper instead of keeping two identical regex copies. Update devServerRenderer to import and call isPluginOnlyFrameworkFile directly, and remove the local fileIsPluginOnly implementation so the logic stays centralized and won’t drift.
🤖 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/vite-plugin-component-preview/src/transformAstro.ts`:
- Around line 539-543: Remove the leftover debug file writes from
transformAstro’s `sourceId.includes("about-us.astro")` branches, including the
`fs.writeFileSync` calls and empty `try/catch` blocks. Keep `transformAstro`
focused on its intended transform behavior and eliminate any hardcoded `/tmp`
I/O that dumps `code` content during matching transforms, including the
duplicate block referenced in the comment.
- Around line 280-304: The fixed 5-node lookahead in transformAstro’s
$$renderComponent and component/custom-element matching can miss the intended
element in dense ASTs, causing preview metadata injection to be skipped. Update
the scan logic in transformAstro (the block handling $$renderComponent and the
matcher around point.type checks) to search beyond five points or advance
strictly in source order until the next component/custom-element is found, then
apply the data-cp-* injection using that matched point.
In `@src/devServerRenderer.ts`:
- Line 84: The dev-server navigation calls using devPage.goto with waitUntil:
"load" do not set an explicit timeout, so a stalled page load can hang rendering
indefinitely. Update the navigation in src/devServerRenderer.ts at both call
sites to pass a finite timeout option alongside waitUntil. Keep the change
scoped to the devPage.goto invocations used during hover/render setup so the
renderer fails fast instead of waiting forever.
In `@src/pathUtils.ts`:
- Around line 25-71: The route resolver in resolveRouteForFile currently ignores
workspaceRoot, so it matches against the full raw file path and can pick up
unrelated nested app/pages folders. Update resolveRouteForFile to first compute
a workspace-relative path from workspaceRoot and filePath, then run the existing
pattern matching against that relative path; keep the current route
normalization logic in place after the match.
---
Outside diff comments:
In `@src/devServerRenderer.ts`:
- Around line 227-240: renderFromDevServer is using the shared module-global
devPage without any concurrency control, so overlapping requests can overwrite
each other’s navigation and break adapter detection, element lookup, or
screenshot capture. Add serialization around the full render path in
renderFromDevServer, or switch it to a per-request page instance, so only one
route render runs at a time for a given dev page. Keep the protection around
getDevPage, the page.goto navigation, and the downstream render steps that
depend on the same page state.
In `@src/screenshotPipeline.ts`:
- Around line 63-70: Soft-handle per-iteration screenshot failures in
captureAdaptiveJpeg so a timeout on one quality step does not abort the whole
hover preview. Add try/catch around the target.screenshot loop and the
img.screenshot fallback loop, similar to scrollIntoViewIfNeeded, and continue to
the next QUALITY_STEPS entry while preserving and returning the last successful
buf/fallbackBuf when later attempts fail.
---
Nitpick comments:
In `@packages/vite-plugin-component-preview/src/transformAstro.ts`:
- Around line 20-33: Replace the custom AST traversal in transformAstro.ts with
the official `@astrojs/compiler/utils` helpers. Update the local walk function
call sites to use the imported walk (or walkAsync if needed), and change
isInjectableAstroNode to use is.tag(node) instead of the manual INJECTABLE_TYPES
membership check. Keep the logic centered around walk, isInjectableAstroNode,
and INJECTABLE_TYPES so the transformer follows upstream AST utilities.
In `@src/devServerDetector.ts`:
- Around line 218-243: `candidatesFromAstroConfig` duplicates the same
read/regex/fallback flow as `candidatesFromViteConfig`, so consolidate the
shared parsing logic into a helper and parameterize only the config file names
and default port. Keep the config-specific wrappers (`candidatesFromAstroConfig`
and `candidatesFromViteConfig`) thin, and reuse the extracted helper for the
`detectorDeps.readFileUtf8`/port extraction behavior to prevent future drift.
In `@src/devServerRenderer.ts`:
- Around line 62-64: The file-only framework check is duplicated between
fileIsPluginOnly and pluginOnboarding.ts’s isPluginOnlyFrameworkFile, so reuse
the existing helper instead of keeping two identical regex copies. Update
devServerRenderer to import and call isPluginOnlyFrameworkFile directly, and
remove the local fileIsPluginOnly implementation so the logic stays centralized
and won’t drift.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ce858ea-5c9b-4529-84ea-7f83533a4bcb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
package.jsonpackages/vite-plugin-component-preview/package.jsonpackages/vite-plugin-component-preview/src/index.tspackages/vite-plugin-component-preview/src/transformAstro.tspackages/vite-plugin-component-preview/src/types.tspackages/vite-plugin-component-preview/test/transformAstro.test.tspnpm-workspace.yamlsrc/devServerDetector.tssrc/devServerDetector.unit.test.tssrc/devServerRenderer.tssrc/extension.tssrc/hoverProvider.tssrc/pathUtils.tssrc/pathUtils.unit.test.tssrc/pluginOnboarding.tssrc/screenshotPipeline.ts
…erlapping page usage
…tils walkAsync for AST traversal
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/screenshotPipeline.unit.test.ts (1)
119-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for total fallback failure and newPage/waitFor rejection.
These tests cover successful/oversized fallback paths and total target-screenshot failure, but there's no test for the case where every fallback
img.screenshotattempt rejects (which currently returns an empty buffer and discards the validbufper the issue flagged inscreenshotPipeline.ts), nor forctx.newPage()/img.waitFor()throwing. Adding these would have caught the regression noted in the implementation file.🤖 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 `@src/screenshotPipeline.unit.test.ts` around lines 119 - 172, Add unit coverage around captureAdaptiveJpeg for the missing failure paths: one test where the target screenshot succeeds but every fallback img.screenshot call rejects, and another where ctx.newPage() or mockImg.waitFor() throws. Use the existing captureAdaptiveJpeg, mockCtx.newPage, and mockImg.waitFor/screenshot setup to assert the intended return value when fallback creation or capture fails, so the regression in screenshotPipeline.ts is covered.
🤖 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 `@src/screenshotPipeline.ts`:
- Around line 93-105: The fallback path in screenshotPipeline currently returns
an empty buffer if every quality attempt in the fallback loop fails, instead of
preserving the original oversized screenshot. Update the fallback logic around
the QUALITY_STEPS loop so it starts from or falls back to the already captured
buf, and only replaces it when a smaller valid screenshot is produced. Keep the
soft-handled errors in the img.screenshot attempts, but ensure the function
returns the original buf when no fallback succeeds.
- Around line 82-92: The resize fallback path in captureAdaptiveJpeg is too
brittle because ctx.newPage() and img.waitFor() can throw before the already
captured buffer is returned. Wrap the resizePage setup and image wait in a
soft-fail fallback inside screenshotPipeline.ts so any failure in that block
skips the resize attempt and returns the original buffer/preview instead of
propagating an error; use the captureAdaptiveJpeg and resizePage flow to locate
the change.
---
Nitpick comments:
In `@src/screenshotPipeline.unit.test.ts`:
- Around line 119-172: Add unit coverage around captureAdaptiveJpeg for the
missing failure paths: one test where the target screenshot succeeds but every
fallback img.screenshot call rejects, and another where ctx.newPage() or
mockImg.waitFor() throws. Use the existing captureAdaptiveJpeg, mockCtx.newPage,
and mockImg.waitFor/screenshot setup to assert the intended return value when
fallback creation or capture fails, so the regression in screenshotPipeline.ts
is covered.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22578dc4-e7d6-47aa-8fae-92696480921f
📒 Files selected for processing (7)
packages/vite-plugin-component-preview/src/transformAstro.tssrc/devServerRenderer.tssrc/devServerRenderer.unit.test.tssrc/pathUtils.tssrc/pathUtils.unit.test.tssrc/screenshotPipeline.tssrc/screenshotPipeline.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/pathUtils.unit.test.ts
- src/pathUtils.ts
- packages/vite-plugin-component-preview/src/transformAstro.ts
Comment on lines
82
to
92
| const resizePage = await ctx.newPage(); | ||
| try { | ||
| const b64 = buf.toString("base64"); | ||
| await resizePage.setContent( | ||
| `<!DOCTYPE html><html><body style="margin:0;padding:0;overflow:hidden">` + | ||
| `<img src="data:image/jpeg;base64,${b64}" ` + | ||
| `style="max-width:${MAX_CAPTURE_WIDTH}px;max-height:${MAX_CAPTURE_HEIGHT}px;display:block;object-fit:contain">` + | ||
| `</body></html>`, | ||
| `<img src="data:image/jpeg;base64,${b64}" ` + | ||
| `style="max-width:${MAX_CAPTURE_WIDTH}px;max-height:${MAX_CAPTURE_HEIGHT}px;display:block;object-fit:contain">` + | ||
| `</body></html>`, | ||
| ); | ||
| const img = resizePage.locator("img"); | ||
| await img.waitFor({ state: "visible", timeout: 5000 }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== screenshotPipeline excerpt ==\n'
sed -n '1,180p' src/screenshotPipeline.ts | cat -n
printf '\n== devServerRenderer excerpt ==\n'
sed -n '1,220p' src/devServerRenderer.ts | cat -n
printf '\n== captureAdaptiveJpeg usages ==\n'
rg -n "captureAdaptiveJpeg\(" srcRepository: rioredwards/component-preview
Length of output: 15533
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== devServerRenderer around captureAdaptiveJpeg ==\n'
sed -n '280,360p' src/devServerRenderer.ts | cat -n
printf '\n== renderer.ts around captureAdaptiveJpeg ==\n'
sed -n '1,140p' src/renderer.ts | cat -nRepository: rioredwards/component-preview
Length of output: 6302
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== renderFromDevServer usages ==\n'
rg -n "renderFromDevServer\(" src
printf '\n== renderElement / compressImageFile usages ==\n'
rg -n "renderElement\(|compressImageFile\(" src
printf '\n== devServerRenderer exports/consumers context ==\n'
sed -n '1,120p' src/index.ts 2>/dev/null | cat -n || true
sed -n '1,220p' src/main.ts 2>/dev/null | cat -n || trueRepository: rioredwards/component-preview
Length of output: 1068
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== renderFromDevServer usages ==\n'
rg -n "renderFromDevServer\(" src
printf '\n== renderElement / compressImageFile usages ==\n'
rg -n "renderElement\(|compressImageFile\(" src
printf '\n== module entrypoints that may catch render failures ==\n'
rg -n "catch\\(|try \\{" src/main.ts src/index.ts src/*.tsRepository: rioredwards/component-preview
Length of output: 4056
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== hoverProvider around renderFromDevServer ==\n'
sed -n '80,170p' src/hoverProvider.ts | cat -n
printf '\n== hoverProvider around renderElement ==\n'
sed -n '250,310p' src/hoverProvider.ts | cat -nRepository: rioredwards/component-preview
Length of output: 6070
Soft-handle the resize fallback setup. If ctx.newPage() or img.waitFor() throws here, captureAdaptiveJpeg exits before returning the already captured buffer, so the caller drops to an error hover/null instead of the best available preview. src/screenshotPipeline.ts:82-92
🤖 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 `@src/screenshotPipeline.ts` around lines 82 - 92, The resize fallback path in
captureAdaptiveJpeg is too brittle because ctx.newPage() and img.waitFor() can
throw before the already captured buffer is returned. Wrap the resizePage setup
and image wait in a soft-fail fallback inside screenshotPipeline.ts so any
failure in that block skips the resize attempt and returns the original
buffer/preview instead of propagating an error; use the captureAdaptiveJpeg and
resizePage flow to locate the change.
Comment on lines
+93
to
+105
| let fallbackBuf: Buffer = Buffer.alloc(0); | ||
| for (const quality of QUALITY_STEPS) { | ||
| try { | ||
| const nextFallbackBuf = await img.screenshot({ type: "jpeg", quality, animations: "disabled", timeout: 1000 }); | ||
| fallbackBuf = nextFallbackBuf; | ||
| if (fallbackBuf.length <= MAX_BYTES) { | ||
| return fallbackBuf; | ||
| } | ||
| } catch (error) { | ||
| // Soft-handle fallback screenshot timeout/failure. | ||
| } | ||
| } | ||
| return fallbackBuf; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fallback returns empty buffer instead of falling back to the valid oversized buf.
If every img.screenshot attempt in the fallback loop throws (all soft-caught), fallbackBuf stays at its initial Buffer.alloc(0) and is returned as-is — discarding the perfectly valid (if oversized) screenshot already captured in buf from the primary loop. Returning nothing is strictly worse than returning an oversized-but-viewable image, and this scenario isn't covered by the test suite either.
🐛 Proposed fix: fall back to the original buffer if resize completely fails
- let fallbackBuf: Buffer = Buffer.alloc(0);
+ let fallbackBuf: Buffer = buf;
for (const quality of QUALITY_STEPS) {
try {
const nextFallbackBuf = await img.screenshot({ type: "jpeg", quality, animations: "disabled", timeout: 1000 });
fallbackBuf = nextFallbackBuf;
if (fallbackBuf.length <= MAX_BYTES) {
return fallbackBuf;
}
} catch (error) {
// Soft-handle fallback screenshot timeout/failure.
}
}
return fallbackBuf;📝 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
| let fallbackBuf: Buffer = Buffer.alloc(0); | |
| for (const quality of QUALITY_STEPS) { | |
| try { | |
| const nextFallbackBuf = await img.screenshot({ type: "jpeg", quality, animations: "disabled", timeout: 1000 }); | |
| fallbackBuf = nextFallbackBuf; | |
| if (fallbackBuf.length <= MAX_BYTES) { | |
| return fallbackBuf; | |
| } | |
| } catch (error) { | |
| // Soft-handle fallback screenshot timeout/failure. | |
| } | |
| } | |
| return fallbackBuf; | |
| let fallbackBuf: Buffer = buf; | |
| for (const quality of QUALITY_STEPS) { | |
| try { | |
| const nextFallbackBuf = await img.screenshot({ type: "jpeg", quality, animations: "disabled", timeout: 1000 }); | |
| fallbackBuf = nextFallbackBuf; | |
| if (fallbackBuf.length <= MAX_BYTES) { | |
| return fallbackBuf; | |
| } | |
| } catch (error) { | |
| // Soft-handle fallback screenshot timeout/failure. | |
| } | |
| } | |
| return fallbackBuf; |
🤖 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 `@src/screenshotPipeline.ts` around lines 93 - 105, The fallback path in
screenshotPipeline currently returns an empty buffer if every quality attempt in
the fallback loop fails, instead of preserving the original oversized
screenshot. Update the fallback logic around the QUALITY_STEPS loop so it starts
from or falls back to the already captured buf, and only replaces it when a
smaller valid screenshot is produced. Keep the soft-handled errors in the
img.screenshot attempts, but ensure the function returns the original buf when
no fallback succeeds.
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.
Feat: Add Astro Compatibility & Optimize Screenshot Pipeline
Summary
This PR adds support for Astro components (
.astrofiles) to the extension's live hover previews. It also introduces automatic route-matching behavior (navigating to the page matching the file being edited) and enhances the robustness of the Playwright-based screenshot capturing pipeline (adding timeout boundaries and a compression quality fallback loop).Key Changes
Astro Support & Extension Integration
astrounder VS Code document selectors inextension.tsandhoverProvider.ts.hoverProvider.tsanddevServerRenderer.ts.Route-Specific Navigation
resolveRouteForFile()inpathUtils.tsto translate file system paths (e.g.,/src/pages/about/index.astro,/src/app/dashboard/page.tsx) into their matching URL path routes.devServerRenderer.tsto dynamically compare the current URL and navigate to the resolved route corresponding to the edited file.Screenshot Pipeline Robustness
timeout: 1000) and soft catches toscrollIntoViewIfNeededand screenshot generation insidescreenshotPipeline.ts.screenshotPipeline.tsto loop through theQUALITY_STEPSquality levels just like the primary branch, rather than outputting a raw/single-quality file which could occasionally exceedMAX_BYTES.Verification & Tests
pathUtils.unit.test.tscovering route mappings:mypage.astro->/mypage)about/index.astro->/about)dashboard/page.tsx->/dashboard)/for non-page items.Summary by CodeRabbit
New Features
Bug Fixes
Chores