Add PDF upload support with size validation and tex…#95
Conversation
📝 WalkthroughWalkthrough
ChangesPDF upload flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant User
participant FileUpload
participant pdfjs_dist
participant PDFWorker
User->>FileUpload: select PDF
FileUpload->>pdfjs_dist: dynamically import library
FileUpload->>PDFWorker: configure worker
FileUpload->>pdfjs_dist: load document and extract page text
pdfjs_dist-->>FileUpload: return extracted text
FileUpload-->>User: update upload status and content
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
🤖 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 `@apps/web-ui/src/app/components/FileUpload.tsx`:
- Around line 64-70: Update the PDF extraction workflow in FileUpload so
PDF_TIMEOUT_MS enforces a single 60-second deadline for the entire upload,
rather than restarting around page.getTextContent() for each page. Create and
reuse one shared deadline or wrap the complete extraction/parsing workflow in
one timeout, while preserving the existing per-page extraction behavior within
that overall limit.
- Around line 19-26: Update the PDF error catch handler in the file upload flow
to check that its request ID still matches requestIdRef.current before updating
status, matching the existing stale-request guard used for successful results
and text failures. Ensure errors from older uploads cannot overwrite the status
of a newer upload.
- Around line 57-62: Update the PDF loading flow around loadingTask and pdf so
cleanup always runs in a finally block: destroy loadingTask unconditionally, and
call pdf.cleanup() when a document was successfully created. Preserve the
existing timeout and parsing error behavior while ensuring cleanup also occurs
after timeout or page-processing failures.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7741014e-fbd4-4229-af05-f9412f6fe8d4
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yaml
📒 Files selected for processing (2)
apps/web-ui/package.jsonapps/web-ui/src/app/components/FileUpload.tsx
…PDFs The previous implementation used Promise.race for the 60s timeout, which meant that on expiry the underlying extraction kept running to completion in the background — pdf.cleanup() and loadingTask.destroy() were never reached, and the worker kept processing pages nobody would ever read. This commit replaces the Promise.race pattern with an explicit timeout flag and a setTimeout handle. On expiry we call loadingTask.destroy() to release the worker; on success we run pdf.cleanup() and loadingTask.destroy() in a finally block. Both cleanup calls are best-effort (errors are swallowed) so they cannot mask the original extraction error. Also addressed while here: - Resolve the pdfjs worker URL once at module scope (PDF_WORKER_URL) instead of constructing it on every PDF drop. - Fetch pages concurrently via Promise.all. pdfjs's worker handles pages in parallel and caches them, so this is significantly faster than the previous sequential loop on multi-page resumes. - Surface scanned-image PDFs: pdfjs returns an empty string silently when a PDF has no text layer, so we now detect that case and tell the user instead of accepting a blank resume for evaluation. - Consolidate the stale-request check into an isStale(requestId) helper used by both the text and pdf branches.
|
Hey — pushed one fix commit on top of #95: Bug fixed: the Replaced with an explicit While I was there:
Verified:
Not done (worth a follow-up, but out of scope for this PR):
Happy to split up / rework any of this if you'd prefer separate commits. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@apps/web-ui/src/app/components/FileUpload.tsx`:
- Around line 90-102: Update the page extraction logic in FileUpload’s
Promise.all block to bound concurrency instead of starting extraction for every
page simultaneously. Process pages sequentially or through a small
worker/concurrency limit, while preserving page order and the existing text
extraction and joining behavior.
- Around line 95-99: Update the text extraction logic in FileUpload to preserve
PDF line breaks indicated by each item's hasEOL metadata instead of joining all
items with spaces. Keep spaces between items on the same line, insert newline
separators after items marked hasEOL, and retain the existing empty-string
handling for items without text.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ba8d0541-c888-4059-a782-248558ebb780
📒 Files selected for processing (1)
apps/web-ui/src/app/components/FileUpload.tsx
| const pageTexts = await Promise.all( | ||
| Array.from({ length: doc.numPages }, (_, i) => i + 1).map( | ||
| async (pageNumber) => { | ||
| const page = await doc.getPage(pageNumber); | ||
| const content = await page.getTextContent(); | ||
| return content.items | ||
| .map((item) => | ||
| "str" in item && item.str != null ? String(item.str) : "" | ||
| ) | ||
| .join(" "); | ||
| } | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound concurrent page extraction.
Promise.all starts every page extraction at once. A page-heavy PDF can create excessive worker activity and memory pressure despite the 50 MB file limit. Process pages sequentially or use a small concurrency limit.
Proposed bounded extraction
- const pageTexts = await Promise.all(
- Array.from({ length: doc.numPages }, (_, i) => i + 1).map(
- async (pageNumber) => {
- const page = await doc.getPage(pageNumber);
- const content = await page.getTextContent();
- return content.items
- .map((item) =>
- "str" in item && item.str != null ? String(item.str) : ""
- )
- .join(" ");
- }
- )
- );
+ const pageTexts: string[] = [];
+ for (let pageNumber = 1; pageNumber <= doc.numPages; pageNumber++) {
+ const page = await doc.getPage(pageNumber);
+ const content = await page.getTextContent();
+ pageTexts.push(
+ content.items
+ .map((item) =>
+ "str" in item && item.str != null ? String(item.str) : ""
+ )
+ .join(" ")
+ );
+ }📝 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 pageTexts = await Promise.all( | |
| Array.from({ length: doc.numPages }, (_, i) => i + 1).map( | |
| async (pageNumber) => { | |
| const page = await doc.getPage(pageNumber); | |
| const content = await page.getTextContent(); | |
| return content.items | |
| .map((item) => | |
| "str" in item && item.str != null ? String(item.str) : "" | |
| ) | |
| .join(" "); | |
| } | |
| ) | |
| ); | |
| const pageTexts: string[] = []; | |
| for (let pageNumber = 1; pageNumber <= doc.numPages; pageNumber++) { | |
| const page = await doc.getPage(pageNumber); | |
| const content = await page.getTextContent(); | |
| pageTexts.push( | |
| content.items | |
| .map((item) => | |
| "str" in item && item.str != null ? String(item.str) : "" | |
| ) | |
| .join(" ") | |
| ); | |
| } |
🤖 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 `@apps/web-ui/src/app/components/FileUpload.tsx` around lines 90 - 102, Update
the page extraction logic in FileUpload’s Promise.all block to bound concurrency
instead of starting extraction for every page simultaneously. Process pages
sequentially or through a small worker/concurrency limit, while preserving page
order and the existing text extraction and joining behavior.
| return content.items | ||
| .map((item) => | ||
| "str" in item && item.str != null ? String(item.str) : "" | ||
| ) | ||
| .join(" "); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby context.
FILE="apps/web-ui/src/app/components/FileUpload.tsx"
wc -l "$FILE"
sed -n '1,180p' "$FILE"
# Find any usage of PDF.js text items and line-break handling in the repo.
rg -n "hasEOL|TextItem|getTextContent|pdfjs" apps/web-ui/src/app -SRepository: TechImmigrants/cv-builder
Length of output: 7535
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the pdfjs-dist version and any local type definitions if present.
rg -n '"pdfjs-dist"|pdfjs-dist' package.json apps/web-ui/package.json pnpm-lock.yaml yarn.lock package-lock.json -S || trueRepository: TechImmigrants/cv-builder
Length of output: 449
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Pull authoritative docs for TextItem.hasEOL semantics.
python3 - <<'PY'
import sys, urllib.request
urls = [
"https://mozilla.github.io/pdf.js/api/draft/module-pdfjsLib.html",
"https://github.com/mozilla/pdf.js/blob/master/src/display/api.js",
]
for url in urls:
try:
with urllib.request.urlopen(url, timeout=20) as r:
data = r.read(2000).decode("utf-8", "ignore")
print(f"URL: {url}\n{data[:1200]}\n---")
except Exception as e:
print(f"URL: {url}\nERROR: {e}\n---")
PYRepository: TechImmigrants/cv-builder
Length of output: 592
🌐 Web query:
PDF.js TextItem hasEOL getTextContent line breaks documentation
💡 Result:
In PDF.js, the hasEOL property is a boolean field within the TextItem object that indicates whether a specific text content item is followed by a line break [1][2][3]. This property was formally added to the TextItem API documentation to assist with identifying line endings, which are not explicitly defined in the underlying PDF specification [4]. Because PDF files represent text as glyphs positioned at absolute coordinates rather than as structured lines or paragraphs, there is no inherent concept of "lines" or "line breaks" in the file format [5][6][7]. While hasEOL provides a signal for line breaks, developers often rely on additional techniques when extracting text because the accuracy of these markers can vary depending on how the PDF was generated and how PDF.js processes it [8][5][9]. Common strategies include: 1. Using transform matrix data: The transform property of a TextItem contains a matrix where the fifth element (index 5) typically represents the vertical (Y) coordinate [6]. By comparing the Y-coordinate of consecutive items, you can programmatically detect line changes [6][7][10]. 2. Combining with disableCombineTextItems: By default, PDF.js attempts to combine text items into larger strings to improve text selection [9][11]. If you need more granular control over text positioning or line detection, setting disableCombineTextItems: true in the getTextContent parameters can prevent the engine from merging these items, giving you access to the raw, uncombined text chunks [3][9][11]. Because of the variable nature of PDF structures, there is no single guaranteed method to extract "raw lines" that works perfectly for every document; detection logic often involves heuristics based on coordinate proximity, font metadata, and the hasEOL property [5][6][10].
Citations:
- 1: https://mozilla.github.io/pdf.js/api/draft/module-pdfjsLib.html
- 2: https://mozilla.github.io/pdf.js/api/draft/api.js.html
- 3: https://alist-org.github.io/pdf.js/api/draft/api.js.html
- 4: Add
hasEOLto theTextItemtypedef in the API (PR 13257 follow-up) mozilla/pdf.js#13489 - 5: How to get the raw lines' text content? mozilla/pdf.js#9732
- 6: https://stackoverflow.com/questions/54645206/pdfjs-get-raw-text-from-pdf-with-correct-newline-withespace
- 7: https://stackoverflow.com/questions/44376415/display-line-breaks-as-n-in-pdf-to-text-conversion-using-pdf-js
- 8: getTextContent is breaking up text in unexpected ways - very inconsistent mozilla/pdf.js#18201
- 9: getTextContent strings returning more than single characters mozilla/pdf.js#12934
- 10: Add a PDF2HTML nodejs example using textlayers mozilla/pdf.js#12607
- 11: Extract Individual Words mozilla/pdf.js#13047
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how FileUpload content is consumed to judge impact.
rg -n "onContentLoaded|FileUpload" apps/web-ui/src -S
# If there's a parent component, print its relevant section.
PARENT="$(rg -l "onContentLoaded" apps/web-ui/src | head -n 1)"
echo "PARENT=$PARENT"
[ -n "${PARENT:-}" ] && sed -n '1,220p' "$PARENT"Repository: TechImmigrants/cv-builder
Length of output: 3718
Preserve PDF line breaks in FileUpload.tsx:95-99. Joining every text item with " " drops hasEOL line breaks, flattening headings, lists, and section boundaries before evaluation.
🤖 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 `@apps/web-ui/src/app/components/FileUpload.tsx` around lines 95 - 99, Update
the text extraction logic in FileUpload to preserve PDF line breaks indicated by
each item's hasEOL metadata instead of joining all items with spaces. Keep
spaces between items on the same line, insert newline separators after items
marked hasEOL, and retain the existing empty-string handling for items without
text.
2 participants
What does this PR do?
Adds PDF upload support to the Web UI. Users can now drag-and-drop or select .pdf resume files, which are parsed client-side using pdfjs-dist and extracted to plain text for evaluation.
Previously, PDF uploads were detected but rejected with a "not supported yet" message.
Implementation details:
Uses dynamic import("pdfjs-dist") to avoid SSR issues with DOMMatrix during static export
PDF worker loaded locally from node_modules (no CDN dependency)
50 MB file size limit and 60-second timeout per operation
Request counter prevents stale uploads from overwriting newer ones
Worker/document resources cleaned up via pdf.cleanup() after extraction
Text extracted page-by-page with null-safe text item handling
Related issue
Closes #96
Type of change
Checklist
Screenshots (if UI change)
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes