feat(resume): static PDF download, one-page CSS enforcement, and e2e reliability#50
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplace client-side print button with a direct PDF download link, switch resume data to a local JSON (with a dev override), add a Bun+Playwright PDF generator and end-to-end PDF test, and update Playwright timeouts, a few e2e waits, print CSS, and related configs. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (5)
e2e/resume-pdf.spec.ts (2)
3-6: Consider describe-level browser skip for idiomatic Playwright style.The in-body
test.skip(condition, reason)works correctly here since there are no hooks, but the Playwright-recommended pattern for Chromium-only suites uses a fixture callback at thedescribelevel — e.g.,test.skip(({ browserName }) => browserName !== 'chromium', 'Chromium only!')— which also prevents any futurebeforeAll/afterAllhooks from running on non-Chromium browsers.♻️ Proposed refactor
-test.describe("Resume PDF", () => { - test("fits on exactly one page", async ({ page, browserName }) => { - // page.pdf() is only available in Chromium - test.skip(browserName !== "chromium", "PDF generation requires Chromium"); +test.describe("Resume PDF", () => { + test.skip( + ({ browserName }) => browserName !== "chromium", + "PDF generation requires Chromium", + ); + + test("fits on exactly one page", async ({ page }) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/resume-pdf.spec.ts` around lines 3 - 6, Move the browser skip from inside the test body to the describe-level by invoking test.skip as a fixture callback for the test.describe block (use test.describe("Resume PDF", () => { test.skip(({ browserName }) => browserName !== 'chromium', 'Chromium only!'); ... })). Replace the in-test call to test.skip in the "fits on exactly one page" test so the describe-level skip prevents the entire suite (including any future beforeAll/afterAll) from running on non-Chromium browsers; keep the existing test name "fits on exactly one page" and other test logic unchanged.
22-23:Buffer.from(pdfBuffer)is a redundant copy.
page.pdf()"returns the PDF buffer", sopdfBufferis already aBuffer. CallingBuffer.from(pdfBuffer)allocates an unnecessary copy.♻️ Proposed fix
- const pdfStr = Buffer.from(pdfBuffer).toString("latin1"); + const pdfStr = pdfBuffer.toString("latin1");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/resume-pdf.spec.ts` around lines 22 - 23, The code makes an unnecessary copy by calling Buffer.from(pdfBuffer) before converting to string; instead use the existing Buffer returned by page.pdf() directly. Replace the pdfStr assignment so it calls pdfBuffer.toString("latin1") (keeping the same variable names pdfBuffer and pdfStr) and leave the subsequent pageMatches regex unchanged.scripts/generate-resume-pdf.ts (3)
108-108:Buffer.from(pdfBuffer)creates an unnecessary copy on every iteration.
page.pdf()"returns the PDF buffer" —pdfBufferis already aBuffer, socountPdfPagescan be called directly.♻️ Proposed fix
- const pageCount = countPdfPages(Buffer.from(pdfBuffer)); + const pageCount = countPdfPages(pdfBuffer);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate-resume-pdf.ts` at line 108, The call is making an unnecessary copy by wrapping pdfBuffer in Buffer.from before counting pages; change the code that calls countPdfPages(Buffer.from(pdfBuffer)) to call countPdfPages(pdfBuffer) directly (ensure pdfBuffer returned from page.pdf() is treated as a Buffer and passed unchanged), so the countPdfPages function receives the existing Buffer and you avoid duplicating the PDF buffer on every iteration.
26-28: Silent drift risk:INITIAL_FONT_PT/INITIAL_SPACE_PTmust stay in sync withresume.cssmanually.Because the script always injects its own CSS override — including on attempt 1 — it never exercises the natural CSS state. If
resume.cssis updated independently and these constants are not kept in sync:
INITIAL_FONT_PT> actual CSS value → attempt 1 renders with an artificially large font, potentially spilling to page 2 and triggering a spurious "scaled down" warning, producing a smaller-than-needed PDF.INITIAL_FONT_PT< actual CSS value → script starts below the actual CSS baseline, producing an unnecessarily small PDF with no warning.The e2e test at
e2e/resume-pdf.spec.tsdoes not inject any CSS override, so it tests the real CSS values — meaning the two can produce different results when these constants drift.Consider either reading the initial values from the CSS file at runtime, or skipping the CSS override entirely on the first attempt (let Chromium use the natural print CSS, then only inject overrides on subsequent scale-down attempts):
♻️ Sketch of "natural-CSS-first" approach
+ // Attempt 1: test the natural CSS (no override injected). + // Only inject overrides on scale-down iterations. for (let attempt = 1; attempt <= MAX_ITERATIONS; attempt++) { - await page.evaluate( - ({ font, space }) => { /* inject style */ }, - { font: fontPt, space: spacePt }, - ); + if (scaledDown) { + await page.evaluate( + ({ font, space }: { font: number; space: number }) => { + /* inject / update `#resume-pdf-scale` style */ + }, + { font: fontPt, space: spacePt }, + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate-resume-pdf.ts` around lines 26 - 28, The script's hardcoded INITIAL_FONT_PT and INITIAL_SPACE_PT in generate-resume-pdf.ts can drift from resume.css and cause wrong first-attempt rendering; update the logic so the first rendering attempt uses the page's natural print CSS (do not inject the override CSS on attempt 1) and only apply the injected CSS overrides on subsequent scale-down attempts, or alternatively read the initial font/spacing values directly from resume.css at runtime; change the CSS-injection branch that currently runs unconditionally to skip injection when attempt === 1 (or implement a CSS-parsing helper to derive INITIAL_FONT_PT/INITIAL_SPACE_PT from resume.css) and ensure e2e/resume-pdf.spec.ts behavior remains unchanged.
66-161: Consolidate browser cleanup intotry/finallyto guard all exit paths.
browser.close()is called explicitly in the 4 controlled exit paths, but any unexpected throw (e.g.,writeFileerror,page.evaluaterejection,page.gotorejection) falls through tomain().catch()at line 163-166 which callsprocess.exit(1)without closing the browser. While the OS will reclaim the Chromium subprocess on process exit, the explicit close calls are now scattered across four branches — atry/finallyboth removes that duplication and makes the intent unambiguous.♻️ Proposed refactor
const browser = await chromium.launch(); - const page = await browser.newPage(); - await page.goto(`${baseUrl}/resume`, { waitUntil: "networkidle" }); + try { + const page = await browser.newPage(); + await page.goto(`${baseUrl}/resume`, { waitUntil: "networkidle" }); let fontPt = INITIAL_FONT_PT; let spacePt = INITIAL_SPACE_PT; let scaledDown = false; for (let attempt = 1; attempt <= MAX_ITERATIONS; attempt++) { // ... (loop body unchanged, remove individual browser.close() calls) ... if (pageCount === 1) { const outputPath = resolve(output); await writeFile(outputPath, pdfBuffer); console.log(`\n✅ Resume PDF saved to ${output}`); // ... scaling warning log ... - await browser.close(); return; } if (pageCount === 0) { console.error("Error: could not detect any pages in the generated PDF."); - await browser.close(); process.exit(1); } // ... scale-down logic ... if (fontPt < MIN_FONT_PT) { console.error(`\n❌ Font size (${fontPt}pt) fell below the minimum (${MIN_FONT_PT}pt).`); console.error(" Consider reducing the resume content instead."); - await browser.close(); process.exit(1); } } console.error(`\n❌ Could not fit resume on one page after ${MAX_ITERATIONS} attempts.`); - await browser.close(); process.exit(1); + } finally { + await browser.close(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate-resume-pdf.ts` around lines 66 - 161, Wrap the work done after launching Chromium (the const browser = await chromium.launch(); and subsequent page interactions including page.goto, page.evaluate, page.pdf, writeFile, and the loop that checks pageCount) in a try/finally so browser.close() runs in the finally block; remove the four explicit await browser.close() calls and any duplicated exit logic inside the loop, ensuring you still call process.exit(1) where appropriate after closing (or rethrow/errors as needed) so all exit paths cleanly close the browser instance referenced by browser and the page variable before exit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/resume-pdf.spec.ts`:
- Around line 3-6: Move the browser skip from inside the test body to the
describe-level by invoking test.skip as a fixture callback for the test.describe
block (use test.describe("Resume PDF", () => { test.skip(({ browserName }) =>
browserName !== 'chromium', 'Chromium only!'); ... })). Replace the in-test call
to test.skip in the "fits on exactly one page" test so the describe-level skip
prevents the entire suite (including any future beforeAll/afterAll) from running
on non-Chromium browsers; keep the existing test name "fits on exactly one page"
and other test logic unchanged.
- Around line 22-23: The code makes an unnecessary copy by calling
Buffer.from(pdfBuffer) before converting to string; instead use the existing
Buffer returned by page.pdf() directly. Replace the pdfStr assignment so it
calls pdfBuffer.toString("latin1") (keeping the same variable names pdfBuffer
and pdfStr) and leave the subsequent pageMatches regex unchanged.
In `@scripts/generate-resume-pdf.ts`:
- Line 108: The call is making an unnecessary copy by wrapping pdfBuffer in
Buffer.from before counting pages; change the code that calls
countPdfPages(Buffer.from(pdfBuffer)) to call countPdfPages(pdfBuffer) directly
(ensure pdfBuffer returned from page.pdf() is treated as a Buffer and passed
unchanged), so the countPdfPages function receives the existing Buffer and you
avoid duplicating the PDF buffer on every iteration.
- Around line 26-28: The script's hardcoded INITIAL_FONT_PT and INITIAL_SPACE_PT
in generate-resume-pdf.ts can drift from resume.css and cause wrong
first-attempt rendering; update the logic so the first rendering attempt uses
the page's natural print CSS (do not inject the override CSS on attempt 1) and
only apply the injected CSS overrides on subsequent scale-down attempts, or
alternatively read the initial font/spacing values directly from resume.css at
runtime; change the CSS-injection branch that currently runs unconditionally to
skip injection when attempt === 1 (or implement a CSS-parsing helper to derive
INITIAL_FONT_PT/INITIAL_SPACE_PT from resume.css) and ensure
e2e/resume-pdf.spec.ts behavior remains unchanged.
- Around line 66-161: Wrap the work done after launching Chromium (the const
browser = await chromium.launch(); and subsequent page interactions including
page.goto, page.evaluate, page.pdf, writeFile, and the loop that checks
pageCount) in a try/finally so browser.close() runs in the finally block; remove
the four explicit await browser.close() calls and any duplicated exit logic
inside the loop, ensuring you still call process.exit(1) where appropriate after
closing (or rethrow/errors as needed) so all exit paths cleanly close the
browser instance referenced by browser and the page variable before exit.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/resume.css (1)
413-413:--print-base-font: 16ptwill always trigger auto-scaling — consider a more realistic starting value.At 16pt body text, the derived name heading becomes 36pt. This is far too large for a single letter-size page, so the script will always iterate and always emit the "scaling was required" warning — the CSS default is never used directly.
The previous 12pt was closer to a working print size. A value in the 10–11pt range would mean fewer iterations and the CSS file would converge toward the empirically correct value after the first guided run.
♻️ Suggested starting point
- --print-base-font: 16pt; + --print-base-font: 10pt;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/resume.css` at line 413, The CSS variable --print-base-font is set to an unrealistic 16pt which forces the print-scaling loop to always run (the derived name heading becomes ~36pt); change the initial value of --print-base-font in app/resume.css to a more realistic print starting size (suggest 10pt–11pt, e.g. 10.5pt or 11pt) so the script converges faster and avoids the "scaling was required" warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/resume/page.tsx`:
- Line 22: The filename construction in the const filename line uses
resume.basics.label ?? "Resume", which causes a duplicated "_Resume_Resume.pdf"
when label is missing; update filename generation to omit the label segment when
resume.basics.label is null/undefined (or use a short semantic fallback like
"Profile") by conditionally including the label part (e.g., build an array of
parts using resume.basics.name?.replace(...) and only push
resume.basics.label?.replace(...) when present, then join with "_") so the
suffix becomes "_Resume.pdf" only once and no incorrect "Resume_Resume" occurs;
update the code referencing filename in page.tsx accordingly.
- Around line 43-44: The href is hardcoded to "/Rio_Edwards_Resume.pdf" while
the download attribute uses the dynamic filename variable, which can get out of
sync and fails if the PDF isn't generated; fix by either (A) updating the build
script to run "bun generate:resume" before the Next build (e.g., change the
"build" script to include bun generate:resume && next build) so the file always
exists, or (B) derive the link href from the same filename variable used in
download (use "/" + filename for the href) so href and download stay in sync;
locate the href and download usage in page.tsx (the element with
href="/Rio_Edwards_Resume.pdf" and download={filename}) and apply one or both
fixes.
---
Nitpick comments:
In `@app/resume.css`:
- Line 413: The CSS variable --print-base-font is set to an unrealistic 16pt
which forces the print-scaling loop to always run (the derived name heading
becomes ~36pt); change the initial value of --print-base-font in app/resume.css
to a more realistic print starting size (suggest 10pt–11pt, e.g. 10.5pt or 11pt)
so the script converges faster and avoids the "scaling was required" warning.
app/resume/page.tsx
Outdated
| href="/Rio_Edwards_Resume.pdf" | ||
| download={filename} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the static PDF exists in public/
fd "Rio_Edwards_Resume.pdf" --type fRepository: rioredwards/portfolio
Length of output: 94
🏁 Script executed:
# Read the resume page to understand the context
cat -n app/resume/page.tsx | head -60Repository: rioredwards/portfolio
Length of output: 2392
🏁 Script executed:
# Check for build/deploy configuration
cat package.json | grep -A 10 "scripts"Repository: rioredwards/portfolio
Length of output: 760
Hardcoded PDF path decoupled from dynamic filename and build pipeline not configured to regenerate.
The file /Rio_Edwards_Resume.pdf currently exists in public/, but the build script ("next build") does not include bun generate:resume as a pre-step. This creates two problems:
-
Pipeline fragility: If the committed file is deleted or not present in a fresh clone before the build runs, the download link will fail with a 404 despite build success.
-
Filename coupling: The
href="/Rio_Edwards_Resume.pdf"is hardcoded whiledownload={filename}is dynamically constructed fromresume.basics.nameandresume.basics.label. If the PDF is regenerated with a different filename (e.g., when label changes), the href and download attribute will diverge, causing the browser to serve the wrong file or mismatch expectations.
To address this:
- Add
bun generate:resumeto the build script ("build": "bun generate:resume && next build") to ensure the file is always current, OR - Derive the
hreffrom thefilenamevariable to keep them in sync instead of hardcoding it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/resume/page.tsx` around lines 43 - 44, The href is hardcoded to
"/Rio_Edwards_Resume.pdf" while the download attribute uses the dynamic filename
variable, which can get out of sync and fails if the PDF isn't generated; fix by
either (A) updating the build script to run "bun generate:resume" before the
Next build (e.g., change the "build" script to include bun generate:resume &&
next build) so the file always exists, or (B) derive the link href from the same
filename variable used in download (use "/" + filename for the href) so href and
download stay in sync; locate the href and download usage in page.tsx (the
element with href="/Rio_Edwards_Resume.pdf" and download={filename}) and apply
one or both fixes.
…reliability
Download link:
- Delete PrintResumeButton (window.print() client component)
- Resume page now has a plain <a download> link to /Rio_Edwards_Resume.pdf
- Update resume.css print block to hide the download link element during printing
One-page PDF enforcement:
- Tune --print-base-font to 10.5pt and --print-base-space to 4.25pt so the resume
fits on one page with static CSS (no dynamic scaling script needed)
- Add e2e/resume-pdf.spec.ts: Chromium-only test that generates a PDF via
page.pdf() and asserts it is exactly one page
- Regenerate public/Rio_Edwards_Resume.pdf with the corrected values
E2E reliability:
- Switch networkidle -> domcontentloaded in lightbox tests and home-page POM
- Add explicit timeouts to lightbox modal and hydration waits
- Skip skip-link test on Mobile Safari WebKit (programmatic focus unreliable)
- Use getByRole("heading") for EXPERIENCE and EDUCATION section assertions
- Add transpilePackages: ["next-mdx-remote"] to next.config.ts
- Switch playwright webServer from npm to bun, add webServer timeout (120s)
- Add global test timeout (60s) and navigationTimeout (45s) to playwright.config.ts
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rioredwards
force-pushed
the
feat/resume-pdf-one-page-guard
branch
from
46c4fa2 to
94177a4
Compare
rioredwards
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e/resume-pdf.spec.ts (1)
22-24: Regex-based PDF page counting is fragile — usepdf-parseinstead.Parsing the PDF as a raw
latin1string and grepping for/Type /Pageis an implementation-level detail of Chromium's current PDF output. Libraries likepdfjsexpose PDF properties such as page count directly, which can then be asserted with Playwright'sexpect. If Chromium starts compressing page-object dictionaries intoObjStmstreams in a future version, these entries would be inside aFlateDecodeblob and invisible in the raw string, silently returningpageCount = 0(the test would then always fail rather than report a real overflow).Additionally,
Buffer.from(pdfBuffer)is a redundant allocation —page.pdf()already returns the PDF buffer directly — so.toString("latin1")can be called onpdfBufferdirectly.♻️ Proposed refactor using
pdf-parseInstall the dependency:
bun add -D pdf-parse `@types/pdf-parse`- // Count pages by matching /Type /Page dictionary entries (not /Type /Pages) - const pdfStr = Buffer.from(pdfBuffer).toString("latin1"); - const pageMatches = pdfStr.match(/\/Type\s*\/Page\b(?!s)/g); - const pageCount = pageMatches ? pageMatches.length : 0; + const pdfData = await import("pdf-parse").then((m) => m.default(pdfBuffer)); + const pageCount = pdfData.numpages;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/resume-pdf.spec.ts` around lines 22 - 24, The test currently converts pdfBuffer to a latin1 string and counts "/Type /Page" matches via pdfStr/pageMatches/pageCount which is fragile; instead install and use the pdf-parse library and call its parseBuffer/ pdf(...) on pdfBuffer to read the authoritative numpages property and assert that with Playwright's expect, and remove the redundant Buffer.from(pdfBuffer) allocation and the regex logic (pdfStr, pageMatches, pageCount).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@playwright.config.ts`:
- Around line 61-64: The webServer health-check URL and Playwright baseURL use
"http://localhost:3100" while the dev server is bound to 127.0.0.1 (see command:
"bun run dev --hostname 127.0.0.1 --port 3100"), which can cause IPv6 localhost
resolution failures; change the URL values (the url property in the webServer
config and the baseURL in the use config) from "http://localhost:3100" to
"http://127.0.0.1:3100" and search for any other occurrences of "localhost:3100"
in the Playwright config to replace so health checks and requests target the
same IPv4-bound address.
---
Duplicate comments:
In `@app/resume/page.tsx`:
- Line 22: The filename template can produce a duplicated "Resume" because
resume.basics.label falls back to "Resume" inside the template while the literal
already appends "_Resume.pdf"; change the logic for filename (the const
filename) to avoid a duplicate by treating resume.basics.label as optional (use
an empty-string fallback or build a labelPart that's only prefixed with "_" when
label exists) and then append the single "_Resume.pdf" suffix; reference the
filename constant and the resume.basics.name / resume.basics.label when making
this change.
- Around line 43-44: The anchor currently hardcodes
href="/Rio_Edwards_Resume.pdf" while using the dynamic download={filename},
causing mismatches and 404s; update the anchor in app/resume/page.tsx to use the
same filename variable for href (e.g., href={filename}) or otherwise derive both
href and download from the same source (the filename constant/prop) and add a
guard so the link is not rendered (or a fallback message) when the generated PDF
is missing to avoid build-time 404s; refer to the filename identifier and the
anchor element where download={filename} is set.
---
Nitpick comments:
In `@e2e/resume-pdf.spec.ts`:
- Around line 22-24: The test currently converts pdfBuffer to a latin1 string
and counts "/Type /Page" matches via pdfStr/pageMatches/pageCount which is
fragile; instead install and use the pdf-parse library and call its parseBuffer/
pdf(...) on pdfBuffer to read the authoritative numpages property and assert
that with Playwright's expect, and remove the redundant Buffer.from(pdfBuffer)
allocation and the regex logic (pdfStr, pageMatches, pageCount).
| command: "bun run dev --hostname 127.0.0.1 --port 3100", | ||
| url: "http://localhost:3100", | ||
| reuseExistingServer: !process.env.CI, | ||
| timeout: 120_000, |
There was a problem hiding this comment.
localhost in health-check URL / baseURL may not resolve to 127.0.0.1.
The dev server starts with --hostname 127.0.0.1, binding exclusively to the IPv4 loopback. Both webServer.url (http://localhost:3100) and baseURL (http://localhost:3100) rely on the OS resolving localhost. On Node.js 18+ / Linux systems (including many GitHub Actions runners), localhost can resolve to ::1 first; an HTTP request to http://localhost:3100 then hits IPv6, which the server doesn't serve, causing the Playwright readiness poll to time out after 120 s.
Align the URLs to the bound address:
🐛 Proposed fix
webServer: {
command: "bun run dev --hostname 127.0.0.1 --port 3100",
- url: "http://localhost:3100",
+ url: "http://127.0.0.1:3100",
reuseExistingServer: !process.env.CI,
timeout: 120_000,
},And in use:
- baseURL: "http://localhost:3100",
+ baseURL: "http://127.0.0.1:3100",📝 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.
| command: "bun run dev --hostname 127.0.0.1 --port 3100", | |
| url: "http://localhost:3100", | |
| reuseExistingServer: !process.env.CI, | |
| timeout: 120_000, | |
| command: "bun run dev --hostname 127.0.0.1 --port 3100", | |
| url: "http://127.0.0.1:3100", | |
| reuseExistingServer: !process.env.CI, | |
| timeout: 120_000, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playwright.config.ts` around lines 61 - 64, The webServer health-check URL
and Playwright baseURL use "http://localhost:3100" while the dev server is bound
to 127.0.0.1 (see command: "bun run dev --hostname 127.0.0.1 --port 3100"),
which can cause IPv6 localhost resolution failures; change the URL values (the
url property in the webServer config and the baseURL in the use config) from
"http://localhost:3100" to "http://127.0.0.1:3100" and search for any other
occurrences of "localhost:3100" in the Playwright config to replace so health
checks and requests target the same IPv4-bound address.
…ling - Add content/resume.json as the canonical resume data source (seeded from Gist) - Simplify lib/resume.ts to import the local JSON instead of fetching remotely. RESUME_LOCAL_PATH still works in dev for generating variant PDFs. - Resume page is now fully static at build time (no network fetch, no revalidation) - Fix download attribute coupling: hardcode download="Rio_Edwards_Resume.pdf" to match the href rather than building a dynamic filename from resume data - Add scripts/generate-pdf.ts: a simple script (no auto-scaling loop) to regenerate public/Rio_Edwards_Resume.pdf when resume content changes. Run with bun generate:pdf while dev server is running. - Add resume-variants/ to .gitignore for storing tailored job-application PDFs locally - Update .env.example to clarify RESUME_LOCAL_PATH is for variant generation only Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/generate-pdf.ts (1)
43-43:writeFilewill throw if the parent directory of--outputdoesn't exist.The default path
public/always exists, but a custom--output ./resume-variants/company.pdfwill fail ifresume-variants/hasn't been created yet. Consider adding amkdirbefore the write.🛡️ Proposed fix
+import { writeFile, mkdir } from "fs/promises"; -import { writeFile } from "fs/promises"; ... + await mkdir(new URL(".", `file://${resolve(output)}`), { recursive: true }); await writeFile(resolve(output), pdf);Or more simply:
+import { writeFile, mkdir } from "fs/promises"; import { resolve, dirname } from "path"; ... + await mkdir(dirname(resolve(output)), { recursive: true }); await writeFile(resolve(output), pdf);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate-pdf.ts` at line 43, The writeFile call in generate-pdf.ts (await writeFile(resolve(output), pdf)) will throw if the parent directory of output doesn't exist; before writing, ensure the directory exists by resolving the parent path (e.g., using dirname(resolve(output))) and calling mkdir with { recursive: true } to create any missing directories, then proceed to writeFile as before so custom outputs like ./resume-variants/company.pdf won't fail.lib/resume.ts (1)
75-75: Moveimportto the top of the file.The module-level import at line 75 is syntactically valid but sits after 73 lines of interface declarations, violating the common TypeScript convention of all imports at the file header. Readers scan the top for dependencies.
♻️ Proposed refactor
+import canonicalResume from "@/content/resume.json"; + // JSON Resume schema types (subset used by this portfolio) // Full schema: https://jsonresume.org/schema export interface ResumeBasics { ... -import canonicalResume from "@/content/resume.json"; - export async function getResume(): Promise<Resume> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/resume.ts` at line 75, Move the module import for canonicalResume to the top of lib/resume.ts so all imports are grouped before any declarations; specifically relocate the line importing canonicalResume (import canonicalResume from "@/content/resume.json") above the interface/type declarations in this file so dependencies are declared at the file header per TypeScript convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/resume.ts`:
- Around line 83-86: Wrap the dev local-path read/parse logic (the dynamic
import of fs/promises, resolve(localPath), readFile and JSON.parse) in a
try/catch inside the function that loads the resume so read failures or JSON
parse errors are caught; in the catch, throw or return a new Error with a
descriptive message that includes the offending localPath and the original error
message (preserve the original error as cause if supported) so callers don’t get
an opaque 500 (look for the code that performs readFile/JSON.parse in
lib/resume.ts to update).
In `@scripts/generate-pdf.ts`:
- Around line 33-44: Declare the Chromium browser variable before launching
(e.g., let browser) and wrap the navigation/pdf/write steps in a try/finally so
browser.close() always runs; specifically, call chromium.launch() and await
page.goto(...)/page.pdf(...)/writeFile(...) inside the try block, and in the
finally block check if browser is truthy then await browser.close().catch(() =>
{}) to swallow close errors—update the logic around chromium.launch, page.goto,
page.pdf, writeFile, and browser.close to ensure no orphaned Chromium process.
---
Nitpick comments:
In `@lib/resume.ts`:
- Line 75: Move the module import for canonicalResume to the top of
lib/resume.ts so all imports are grouped before any declarations; specifically
relocate the line importing canonicalResume (import canonicalResume from
"@/content/resume.json") above the interface/type declarations in this file so
dependencies are declared at the file header per TypeScript convention.
In `@scripts/generate-pdf.ts`:
- Line 43: The writeFile call in generate-pdf.ts (await
writeFile(resolve(output), pdf)) will throw if the parent directory of output
doesn't exist; before writing, ensure the directory exists by resolving the
parent path (e.g., using dirname(resolve(output))) and calling mkdir with {
recursive: true } to create any missing directories, then proceed to writeFile
as before so custom outputs like ./resume-variants/company.pdf won't fail.
| const { readFile } = await import("fs/promises"); | ||
| const { resolve } = await import("path"); | ||
| const json = await readFile(resolve(localPath), "utf-8"); | ||
| return JSON.parse(json); |
There was a problem hiding this comment.
Missing error handling for the dev local-path override.
If RESUME_LOCAL_PATH points to a missing or malformed file, readFile / JSON.parse throw and the RSC crashes with an opaque 500. A simple catch with a descriptive message would save debugging time.
🛡️ Proposed fix
- const json = await readFile(resolve(localPath), "utf-8");
- return JSON.parse(json);
+ let json: string;
+ try {
+ json = await readFile(resolve(localPath), "utf-8");
+ } catch {
+ throw new Error(`RESUME_LOCAL_PATH="${localPath}" could not be read. Check the path in .env.local.`);
+ }
+ try {
+ return JSON.parse(json);
+ } catch {
+ throw new Error(`RESUME_LOCAL_PATH="${localPath}" contains invalid JSON.`);
+ }📝 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 { readFile } = await import("fs/promises"); | |
| const { resolve } = await import("path"); | |
| const json = await readFile(resolve(localPath), "utf-8"); | |
| return JSON.parse(json); | |
| const { readFile } = await import("fs/promises"); | |
| const { resolve } = await import("path"); | |
| let json: string; | |
| try { | |
| json = await readFile(resolve(localPath), "utf-8"); | |
| } catch { | |
| throw new Error(`RESUME_LOCAL_PATH="${localPath}" could not be read. Check the path in .env.local.`); | |
| } | |
| try { | |
| return JSON.parse(json); | |
| } catch { | |
| throw new Error(`RESUME_LOCAL_PATH="${localPath}" contains invalid JSON.`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/resume.ts` around lines 83 - 86, Wrap the dev local-path read/parse logic
(the dynamic import of fs/promises, resolve(localPath), readFile and JSON.parse)
in a try/catch inside the function that loads the resume so read failures or
JSON parse errors are caught; in the catch, throw or return a new Error with a
descriptive message that includes the offending localPath and the original error
message (preserve the original error as cause if supported) so callers don’t get
an opaque 500 (look for the code that performs readFile/JSON.parse in
lib/resume.ts to update).
| const browser = await chromium.launch(); | ||
| const page = await browser.newPage(); | ||
| await page.goto(`${baseUrl}/resume`, { waitUntil: "networkidle" }); | ||
|
|
||
| const pdf = await page.pdf({ | ||
| format: "Letter", | ||
| margin: { top: "0.5in", right: "0.55in", bottom: "0.5in", left: "0.55in" }, | ||
| printBackground: true, | ||
| }); | ||
|
|
||
| await writeFile(resolve(output), pdf); | ||
| await browser.close(); |
There was a problem hiding this comment.
Browser process leaks if any step throws before browser.close().
If the dev server isn't running, page.goto() throws (connection refused), browser.close() is never reached, and a zombie Chromium process is left behind. Wrap the logic in try/finally.
🛡️ Proposed fix
const browser = await chromium.launch();
-const page = await browser.newPage();
-await page.goto(`${baseUrl}/resume`, { waitUntil: "networkidle" });
-
-const pdf = await page.pdf({
- format: "Letter",
- margin: { top: "0.5in", right: "0.55in", bottom: "0.5in", left: "0.55in" },
- printBackground: true,
-});
-
-await writeFile(resolve(output), pdf);
-await browser.close();
-
-console.log(`PDF saved to ${output}`);
+try {
+ const page = await browser.newPage();
+ await page.goto(`${baseUrl}/resume`, { waitUntil: "networkidle" });
+
+ const pdf = await page.pdf({
+ format: "Letter",
+ margin: { top: "0.5in", right: "0.55in", bottom: "0.5in", left: "0.55in" },
+ printBackground: true,
+ });
+
+ await writeFile(resolve(output), pdf);
+ console.log(`PDF saved to ${output}`);
+} finally {
+ await browser.close();
+}📝 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 browser = await chromium.launch(); | |
| const page = await browser.newPage(); | |
| await page.goto(`${baseUrl}/resume`, { waitUntil: "networkidle" }); | |
| const pdf = await page.pdf({ | |
| format: "Letter", | |
| margin: { top: "0.5in", right: "0.55in", bottom: "0.5in", left: "0.55in" }, | |
| printBackground: true, | |
| }); | |
| await writeFile(resolve(output), pdf); | |
| await browser.close(); | |
| const browser = await chromium.launch(); | |
| try { | |
| const page = await browser.newPage(); | |
| await page.goto(`${baseUrl}/resume`, { waitUntil: "networkidle" }); | |
| const pdf = await page.pdf({ | |
| format: "Letter", | |
| margin: { top: "0.5in", right: "0.55in", bottom: "0.5in", left: "0.55in" }, | |
| printBackground: true, | |
| }); | |
| await writeFile(resolve(output), pdf); | |
| console.log(`PDF saved to ${output}`); | |
| } finally { | |
| await browser.close(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/generate-pdf.ts` around lines 33 - 44, Declare the Chromium browser
variable before launching (e.g., let browser) and wrap the navigation/pdf/write
steps in a try/finally so browser.close() always runs; specifically, call
chromium.launch() and await page.goto(...)/page.pdf(...)/writeFile(...) inside
the try block, and in the finally block check if browser is truthy then await
browser.close().catch(() => {}) to swallow close errors—update the logic around
chromium.launch, page.goto, page.pdf, writeFile, and browser.close to ensure no
orphaned Chromium process.
Co-Authored-By: Warp <agent@warp.dev>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.claude/settings.local.json (1)
1-19:⚠️ Potential issue | 🟠 Major
.local.jsonshould not be committed to the repository.
settings.local.jsonis the per-developer, machine-local override file for Claude Code. It is explicitly distinguished fromsettings.json(the team-shared, VCS-tracked file). Committing it forces every contributor's Claude agent to inherit these permissions, which is the opposite of what.localfiles are for, and it will generate noise in PRs whenever any developer changes their personal allow-list.Add it to
.gitignoreand commit onlysettings.jsonfor shared defaults.🛠️ Proposed fix
# .gitignore +.claude/settings.local.jsonMove any permissions that should apply to the whole team into
.claude/settings.jsoninstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/settings.local.json around lines 1 - 19, The committed per-developer override file (settings.local.json) contains a personal permissions.allow list and must not be tracked; remove that file from the repo, add it to .gitignore, and ensure only team-wide defaults live in the tracked settings.json. Specifically, delete or untrack the file containing the "permissions": { "allow": [...] } block (remove it from the index with git rm --cached or delete and commit), add an entry to .gitignore to prevent future commits of settings.local.json, and move any permissions that should be shared into the tracked settings.json so only shared defaults remain under "permissions.allow".
🧹 Nitpick comments (2)
.claude/settings.local.json (1)
11-15:Bash(bun run:*)makesBash(bun run build:*)on Line 11 redundant and broadens scope.
bun run:*matches every script inpackage.json, includingdeploy,clean, etc. The earlier, narrowerbun run build:*entry is now dead. If the intent is only to allow builds, keep the specific entry and drop the wildcard; if broader access is acceptable, remove the duplicate.♻️ Proposed cleanup (narrow intent)
- "Bash(bun run build:*)", "WebSearch", "Bash(true:*)", "Bash(git add:*)", - "Bash(bun run:*)", "Bash(git commit:*)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/settings.local.json around lines 11 - 15, The entries "Bash(bun run build:*)" and "Bash(bun run:*)" conflict because the wildcard "Bash(bun run:*)" makes the specific "Bash(bun run build:*)" redundant; decide narrow intent and remove the broader wildcard: delete the "Bash(bun run:*)" line from the settings so only "Bash(bun run build:*)" remains (or conversely, if you want all scripts allowed, remove "Bash(bun run build:*)" and keep "Bash(bun run:*)"), ensuring you update the list to contain only the intended permission.e2e/keyboard-navigation.spec.ts (1)
64-72: LGTM — idiomatic Playwright conditional skip.Destructuring
browserNameand usingtest.skip(condition, reason)inside the test body is the standard Playwright approach when the skip decision depends on a runtime fixture. The rationale (Safari's default Tab-focus behaviour excludes links) is accurate.One optional note: because
test.skip()is placed inside the test body rather than before the hook chain, thebeforeEach(goto+waitForHydration) will still run for WebKit before the test is skipped. If the navigation cost matters in CI, you could hoist the guard to atest.beforeEachscoped inside a nestedtest.describeor use a project-level filter, but for a single lightweight skip this is perfectly fine as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/keyboard-navigation.spec.ts` around lines 64 - 72, No change required: the conditional runtime skip inside the test using the browserName fixture is correct and idiomatic; leave the test body as-is where test.skip(browserName === "webkit", "Safari skips links during Tab navigation") is called (references: browserName, test.skip in the "Tab navigates through interactive elements" test). If you do want to avoid running beforeEach for WebKit in CI, move the same guard into a test.beforeEach within a nested test.describe or apply a project-level filter, but that's optional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/settings.local.json:
- Around line 14-16: The settings currently grant broad autonomous git
staging/commit rights via the entries "Bash(git add:*)" and "Bash(git
commit:*)", which lets the agent make arbitrary commits; remove or tighten these
entries in .claude/settings.local.json by deleting "Bash(git add:*)" and
"Bash(git commit:*)" (or replace them with narrowly scoped alternatives such as
explicit allowed file patterns like "Bash(git add: <safe-file-patterns>)" and a
restricted commit pattern that disallows flags, e.g., only "Bash(git commit:
-m:*)"), and keep any safe command like "Bash(bun run:*)" if needed; prefer
removing both git entries entirely so developers perform git add/commit manually
after reviewing diffs.
---
Outside diff comments:
In @.claude/settings.local.json:
- Around line 1-19: The committed per-developer override file
(settings.local.json) contains a personal permissions.allow list and must not be
tracked; remove that file from the repo, add it to .gitignore, and ensure only
team-wide defaults live in the tracked settings.json. Specifically, delete or
untrack the file containing the "permissions": { "allow": [...] } block (remove
it from the index with git rm --cached or delete and commit), add an entry to
.gitignore to prevent future commits of settings.local.json, and move any
permissions that should be shared into the tracked settings.json so only shared
defaults remain under "permissions.allow".
---
Nitpick comments:
In @.claude/settings.local.json:
- Around line 11-15: The entries "Bash(bun run build:*)" and "Bash(bun run:*)"
conflict because the wildcard "Bash(bun run:*)" makes the specific "Bash(bun run
build:*)" redundant; decide narrow intent and remove the broader wildcard:
delete the "Bash(bun run:*)" line from the settings so only "Bash(bun run
build:*)" remains (or conversely, if you want all scripts allowed, remove
"Bash(bun run build:*)" and keep "Bash(bun run:*)"), ensuring you update the
list to contain only the intended permission.
In `@e2e/keyboard-navigation.spec.ts`:
- Around line 64-72: No change required: the conditional runtime skip inside the
test using the browserName fixture is correct and idiomatic; leave the test body
as-is where test.skip(browserName === "webkit", "Safari skips links during Tab
navigation") is called (references: browserName, test.skip in the "Tab navigates
through interactive elements" test). If you do want to avoid running beforeEach
for WebKit in CI, move the same guard into a test.beforeEach within a nested
test.describe or apply a project-level filter, but that's optional.
.claude/settings.local.json
Outdated
| "Bash(git add:*)", | ||
| "Bash(bun run:*)", | ||
| "Bash(git commit:*)" |
There was a problem hiding this comment.
git add:* + git commit:* grant the Claude agent autonomous commit capability — supply-chain risk.
With both permissions in place, the agent can stage any file and then commit with arbitrary flags (--amend, -a, --allow-empty, --no-verify, etc.) without any human checkpoint. On a project with CI that auto-deploys from main, a single misdirected agent session could silently push unreviewed code.
Prefer tightly scoped alternatives or, at minimum, restrict commit flags rather than using a wildcard:
- "Bash(git add:*)",
- "Bash(git commit:*)"
+ "Bash(git add -p:*)",
+ "Bash(git commit -m:*)"Even better, remove both and let the developer perform git operations manually after reviewing the agent's diff.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/settings.local.json around lines 14 - 16, The settings currently
grant broad autonomous git staging/commit rights via the entries "Bash(git
add:*)" and "Bash(git commit:*)", which lets the agent make arbitrary commits;
remove or tighten these entries in .claude/settings.local.json by deleting
"Bash(git add:*)" and "Bash(git commit:*)" (or replace them with narrowly scoped
alternatives such as explicit allowed file patterns like "Bash(git add:
<safe-file-patterns>)" and a restricted commit pattern that disallows flags,
e.g., only "Bash(git commit: -m:*)"), and keep any safe command like "Bash(bun
run:*)" if needed; prefer removing both git entries entirely so developers
perform git add/commit manually after reviewing diffs.
…m front-end variant) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Playwright's page.pdf() renders at full paper width then clips the margin areas, causing content to be cut off. The fix: declare margins via CSS @page so the layout engine knows the actual content area before rendering. Removed margin from page.pdf() calls in both the generation script and the e2e test, which was overriding the @page rule and triggering the clipping. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nt skills Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Commit 47475e8 used CSS @page margins to own the content area, but Playwright ignores @page when page.pdf() passes its own margin option. The actual fix: set padding on html in @media print so the layout engine shrinks the content area before rendering. Playwright margin is now explicitly zero in both the generate script and the e2e test. Also sets background:white on html to prevent the site theme color from bleeding through in the generated PDF. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ng logic Updated the ResumeContent component to conditionally render job highlight titles only if they are provided. This change ensures that empty titles are not displayed, enhancing the clarity of the resume content. Additionally, modified the ResumeWorkProject interface to make the title property optional.
- lib/resume.ts: move import to top of file - e2e/resume-pdf.spec.ts: move test.skip to describe level (idiomatic Playwright); remove redundant Buffer.from() wrapping on pdfBuffer - scripts/generate-pdf.ts: mkdir parent dir before writeFile so resume-variants/ is created automatically on first use - playwright.config.ts: use 127.0.0.1 instead of localhost to avoid IPv6 resolution mismatches with the server bound to 127.0.0.1 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Per-developer local settings should not be committed. The file remains on disk but is now gitignored. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Summary
Static PDF download: Replaces the
window.print()button with a plain<a download>link pointing to/Rio_Edwards_Resume.pdf. Deletes the now-unusedPrintResumeButtonclient component.One-page CSS enforcement: Instead of an auto-scaling script that iteratively shrinks font/spacing until the PDF fits, the print CSS variables are tuned once (
--print-base-font: 10.5pt,--print-base-space: 4.25pt) so the resume fits on one page statically. When content grows and the test fails, the fix is to adjust those two values inresume.css.Hard e2e guard:
e2e/resume-pdf.spec.tsgenerates a PDF via Playwright'spage.pdf()and asserts it is exactly one page. Fails with an actionable message pointing to the CSS variables to tune.E2E reliability: Switches
networkidle->domcontentloadedwhere appropriate, adds explicit timeouts, skips an unreliable skip-link test on Mobile Safari, improves section heading assertions, and updatesplaywright.config.tsto usebunwith a properwebServertimeout.Test plan
playwright test --project=chromiumpasses (31/31)resume-pdf.spec.tsconfirms PDF is exactly 1 pagebun run buildpasses clean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests
Chores