Fix docs homepage slide preview when the bundled PDF is an LFS pointer#41540
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
|
Great work on this fix! 🎉 The LFS pointer fallback for the docs homepage slide deck is a sharp catch — The PR is well-structured:
This looks ready for review. 🟢
|
There was a problem hiding this comment.
Pull request overview
This PR hardens the docs homepage slide preview by ensuring the bundled slide PDF is a real PDF (not a Git LFS pointer) before the Astro dev server/tests run, preventing the homepage from getting stuck on “Loading slides...” due to InvalidPDFException.
Changes:
- Add a Node helper (
scripts/ensure-docs-slide-pdf.js) that detects Git LFS pointer content and downloads the real PDF from GitHub media. - Replace the prior inline
build:slidesshell check with the reusable helper and run it before starting the Playwright web server. - Extend the Playwright slide preview test to assert the page does not emit
InvalidPDFExceptionin console errors.
Show a summary per file
| File | Description |
|---|---|
| scripts/ensure-docs-slide-pdf.js | New helper to validate/copy the slide PDF and download a real PDF when the checked-out file is an LFS pointer. |
| docs/package.json | Wire build:slides to the new Node helper. |
| docs/playwright.config.ts | Ensure slide asset preparation runs before the dev server starts for browser tests. |
| docs/tests/slide-preview.spec.ts | Add a regression assertion that InvalidPDFException is not logged during slide rendering. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
| if (!bytes.toString("utf8").startsWith(LFS_POINTER_PREFIX)) { | ||
| throw new Error(`${SOURCE_PATH} is neither a PDF nor a Git LFS pointer.`); | ||
| } |
| console.warn(`Detected Git LFS pointer at ${SOURCE_PATH}; downloading ${url}`); | ||
|
|
||
| const response = await fetch(url); | ||
| if (!response.ok) { | ||
| throw new Error(`Failed to download slide deck PDF: ${response.status} ${response.statusText}`); | ||
| } |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Go: 0 ( Scoring breakdown:
📝 Recommendation — Unit test coverage for new production script
The E2E Playwright test covers the happy path (PDF renders without This is a recommendation, not a deduction under the scoring rubric. Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — requesting changes on two reliability gaps and one test coverage gap.
📋 Key Themes & Highlights
Issues to address
- No fetch timeout (
ensure-docs-slide-pdf.js:68): the unboundedfetch()call will hang the entire Playwright test run ifmedia.githubusercontent.comis slow or unreachable. A one-liner fix:{ signal: AbortSignal.timeout(30_000) }. - No output-file cache (
ensure-docs-slide-pdf.js:81):build:slidesis now part of thewebServercommand inplaywright.config.ts, so it runs before every localnpm test. Without a cache check onOUTPUT_PATH, contributors without LFS will download the PDF on every run. - Missing source-file guard (
ensure-docs-slide-pdf.js:53): a missingSOURCE_PATHproduces a crypticENOENT— a short existence check with a helpful hint would improve DX.
Suggestions (non-blocking)
- No unit tests for
isPdf()andgetRepositoryPath()— ascripts/ensure-docs-slide-pdf.test.jsfile would follow the established pattern. InvalidPDFExceptionassertion is only in one test; a dedicated test or spreading it to all slide-dependent tests would make a future regression more diagnosable.
Positive highlights
- ✅ Excellent root-cause diagnosis and clear PR description.
- ✅
getGitRef()correctly prefersGITHUB_SHAin CI over localgit rev-parse. - ✅ Downloaded PDF is validated (
isPdf()) before being written — no silent corrupt-file risk. - ✅ Fallback to
'github/gh-aw'ingetRepositoryPath()is a safe default for forks without a detectable remote. - ✅ Regression test wired into the Playwright setup so future CI runs exercise the real homepage behaviour.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 61.8 AIC · ⌖ 9.99 AIC · ⊞ 6.5K
|
|
||
| console.warn(`Detected Git LFS pointer at ${SOURCE_PATH}; downloading ${url}`); | ||
|
|
||
| const response = await fetch(url); |
There was a problem hiding this comment.
[/diagnose] No timeout on fetch() — if media.githubusercontent.com is slow or unreachable, build:slides (and every Playwright test run via webServer) will hang indefinitely.
💡 Suggested fix
Use AbortSignal.timeout() (available in Node 22):
const response = await fetch(url, { signal: AbortSignal.timeout(30_000) });This is especially important because playwright.config.ts now runs build:slides before every dev server start — a hung fetch blocks all test runs.
| return downloadedBytes; | ||
| } | ||
|
|
||
| async function main() { |
There was a problem hiding this comment.
[/diagnose] main() always re-downloads from GitHub media when the source is an LFS pointer, even if OUTPUT_PATH already has a valid cached PDF. Since playwright.config.ts now runs build:slides before every dev server start, contributors who lack LFS will trigger a network fetch on every local npm test run.
💡 Suggested optimisation
async function main() {
// Skip re-download if the output is already a valid PDF.
if (fs.existsSync(OUTPUT_PATH) && isPdf(fs.readFileSync(OUTPUT_PATH))) {
console.log(`✓ Slide PDF already ready at ${OUTPUT_PATH}`);
return;
}
const pdfBytes = await readPdfBytes();
fs.mkdirSync(path.dirname(OUTPUT_PATH), { recursive: true });
fs.writeFileSync(OUTPUT_PATH, pdfBytes);
console.log(`✓ Slide PDF ready at ${OUTPUT_PATH}`);
}This keeps the fast path fast and avoids unnecessary network calls in repeated runs.
| const LFS_POINTER_PREFIX = "version https://git-lfs.github.com/spec/v1"; | ||
|
|
||
| function isPdf(buffer) { | ||
| return buffer.subarray(0, 5).toString("utf8") === "%PDF-"; |
There was a problem hiding this comment.
[/tdd] The pure functions isPdf() and getRepositoryPath() have no unit tests. Other scripts in this directory follow the pattern of a sibling .test.js file (e.g. scripts/generate-agent-factory.test.js), but there is no scripts/ensure-docs-slide-pdf.test.js.
💡 Suggested coverage
Minimal cases worth testing:
// isPdf
assert(isPdf(Buffer.from('%PDF-1.4 ...')));
assert(!isPdf(Buffer.from('version (gitlfs/redacted)')));
// getRepositoryPath
// Verify the regex handles HTTPS and SSH formats, and .git suffix
assert.equal(parseRemote('https://github.com/owner/repo.git'), 'owner/repo');
assert.equal(parseRemote('git@github.com:owner/repo'), 'owner/repo');The regex at line 29 is also worth a test: a malformed remote should fall through to the 'github/gh-aw' default.
|
|
||
| expect(width).toBeGreaterThan(0); | ||
| expect(height).toBeGreaterThan(0); | ||
| expect(consoleErrors.some((message) => message.includes('InvalidPDFException'))).toBe(false); |
There was a problem hiding this comment.
[/tdd] The InvalidPDFException guard is only asserted in the "renders a visible canvas" test, but consoleErrors is tracked across all tests in the describe block. Consider moving the assertion to its own focused test, or at minimum adding it to every test that depends on successful slide rendering — so a PDF regression anywhere surfaces a clear failure message.
💡 Suggested approach
A dedicated test makes the intent explicit:
test('slide preview loads without PDF errors', () => {
expect(
consoleErrors.some((m) => m.includes('InvalidPDFException'))
).toBe(false);
});This separates the regression assertion from the canvas-dimension check, making both failures individually diagnosable.
| } | ||
|
|
||
| async function readPdfBytes() { | ||
| const bytes = fs.readFileSync(SOURCE_PATH); |
There was a problem hiding this comment.
[/diagnose] If SOURCE_PATH doesn't exist, readFileSync throws a raw ENOENT error. This gives a developer no hint about what to do next.
💡 Suggested guard
async function readPdfBytes() {
if (!fs.existsSync(SOURCE_PATH)) {
throw new Error(
`Expected ${SOURCE_PATH} to exist as a PDF or a Git LFS pointer.\n` +
`Run \`git lfs install && git lfs pull\` or ensure the file was committed.`
);
}
const bytes = fs.readFileSync(SOURCE_PATH);
...
}The original inline shell script implicitly relied on grep returning a non-zero exit if the file was absent; the Node rewrite loses that guard.
There was a problem hiding this comment.
Fix the fetch timeout before merging.
The new LFS-fallback path in scripts/ensure-docs-slide-pdf.js makes a live network request with no deadline. Because that script now runs as part of the Playwright webServer command, a stalled media.githubusercontent.com connection will hang every CI test run indefinitely — a reliability regression relative to the previous pure-local build:slides step.
📋 Findings summary
Blocking
- Missing fetch timeout (
ensure-docs-slide-pdf.js:68) —fetch(url)has noAbortController; CI hangs indefinitely on network stalls now thatbuild:slidesgates Playwright startup.
Non-blocking
- Silent wrong-repo fallback (
ensure-docs-slide-pdf.js:37) — whengitsucceeds but the remote URL doesn't match the regex, the code silently falls back togithub/gh-awwith no warning; produces a confusing404on non-standard remotes with no diagnostic hint.
🔎 Code quality review by PR Code Quality Reviewer · 102.2 AIC · ⌖ 7.42 AIC · ⊞ 5.2K
|
|
||
| console.warn(`Detected Git LFS pointer at ${SOURCE_PATH}; downloading ${url}`); | ||
|
|
||
| const response = await fetch(url); |
There was a problem hiding this comment.
CI hang risk: fetch(url) has no timeout or AbortController; if media.githubusercontent.com stalls, build:slides blocks indefinitely — and since build:slides now gates the Playwright webServer startup (see playwright.config.ts), every test run can hang until the runner's global job timeout.
💡 Suggested fix
const DOWNLOAD_TIMEOUT_MS = 30_000;
const controller = new AbortController();
const timer = setTimeout(
() => controller.abort(new Error(`Timed out after ${DOWNLOAD_TIMEOUT_MS}ms downloading slide PDF`)),
DOWNLOAD_TIMEOUT_MS
);
try {
const response = await fetch(url, { signal: controller.signal });
if (!response.ok) {
throw new Error(`Failed to download slide deck PDF: ${response.status} ${response.statusText}`);
}
const downloadedBytes = Buffer.from(await response.arrayBuffer());
if (!isPdf(downloadedBytes)) {
throw new Error(`Downloaded slide deck from ${url} is not a real PDF.`);
}
return downloadedBytes;
} finally {
clearTimeout(timer);
}Node.js v18+ fetch honours AbortController.signal. Without a deadline, a slow or unresponsive GitHub media server leaves CI spinning indefinitely — previously build:slides was a pure local file-copy step with no such risk.
| // Fall back to the canonical public repository path. | ||
| } | ||
|
|
||
| return "github/gh-aw"; |
There was a problem hiding this comment.
Silent wrong-repo fallback: when execFileSync succeeds but the remote URL doesn't match the regex, the function falls through to return "github/gh-aw" with no warning logged. The catch comment explains the git-not-available case, but a successful-git/unrecognized-URL case is qualitatively different and produces a confusing 404 Not Found with no indication that the wrong repository path was used.
💡 Suggested fix
function getRepositoryPath() {
try {
const remote = execFileSync("git", ["config", "--get", "remote.origin.url"], {
cwd: ROOT,
encoding: "utf8",
}).trim();
const match = remote.match(/github\.com[:\/](?<owner>[^\/]+)\/(?<repo>[^\/.]+?)(?:\.git)?$/);
if (match?.groups?.owner && match.groups.repo) {
return `${match.groups.owner}/${match.groups.repo}`;
}
// git ran but the URL format wasn't recognised — warn so it's diagnosable.
console.warn(
`Could not parse repository path from remote URL "${remote}"; falling back to github/gh-aw`
);
} catch {
// git not available; fall back to the canonical public repository path.
}
return "github/gh-aw";
}Scenarios where the regex silently misses: GitHub Enterprise with a custom domain ((ghe.mycompany.com/redacted), any remote URL that omits the github.com` token. Without the warning, the failure manifests only as a 404 during the PDF download step, with no clue that the wrong owner/repo was used.
|
@copilot run pr-finisher skill |
|
@copilot review all comments and address unresolved review feedback.
|
|
Please refresh the branch and summarize the remaining blockers, especially the fetch-timeout follow-up.
|
The docs homepage hero slide deck could get stuck on Loading slides... across all viewports because the bundled PDF resolved to a Git LFS pointer instead of a real PDF, causing
InvalidPDFExceptionin the browser. This change hardens slide asset preparation so the homepage preview can render even when the checkout does not contain hydrated LFS content.Slide asset fallback
docs/slides/github-agentic-workflows.pdfis a Git LFS pointer.docs/public/slides/.Docs build / test wiring
build:slidesshell check with the reusable Node helper.Regression coverage
InvalidPDFException.