Repository navigation
fix(pdf): make the render scale and page ceiling reachable from generate() - #1301
Merged
Merged
Conversation
Contributor
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPDF fallback rendering now supports per-request ChangesPDF fallback controls
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Caller
participant messageBuilder
participant pdfProcessor
participant Provider
Caller->>messageBuilder: submit PDF with scale and maxPages
messageBuilder->>pdfProcessor: validate and convert PDF pages
pdfProcessor-->>messageBuilder: return page images
messageBuilder->>Provider: send provider-formatted content
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/continuous-test-suite-bugfixes.ts (1)
1830-1881: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the advanced
input.contentPDF path.This test only calls
input.pdfFiles. Add a case that supplies a PDF ininput.contentwithpdfOptions. That path constructsMultimodalPdfEntryseparately insrc/lib/utils/messageBuilder.tsLine 1879.🤖 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 `@test/continuous-test-suite-bugfixes.ts` around lines 1830 - 1881, The test case in “MessageBuilder `#297`: the real conversion path honors the default scale and caller overrides” only exercises input.pdfFiles; extend it to also invoke buildMultimodalMessagesArray with a PDF supplied through input.content and pdfOptions, then assert the captured scale/maxPages values so the separate MultimodalPdfEntry construction path 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/lib/utils/messageBuilder.ts`:
- Around line 2403-2413: Update the PDF processing flow around
enforceAggregatePdfLimits() to retain its resolved page count before conversion,
including when input.content pageCount is null because metadata.pages is
omitted. Use that resolved value in the requestedPages check for the truncation
warning, while preserving the existing warning message and behavior for
non-truncated conversions.
---
Nitpick comments:
In `@test/continuous-test-suite-bugfixes.ts`:
- Around line 1830-1881: The test case in “MessageBuilder `#297`: the real
conversion path honors the default scale and caller overrides” only exercises
input.pdfFiles; extend it to also invoke buildMultimodalMessagesArray with a PDF
supplied through input.content and pdfOptions, then assert the captured
scale/maxPages values so the separate MultimodalPdfEntry construction path is
covered.
🪄 Autofix
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: 8450db66-d93b-4d21-acdc-8d9c4e8d7cdd
📒 Files selected for processing (5)
src/lib/types/file.tssrc/lib/types/generate.tssrc/lib/types/stream.tssrc/lib/utils/messageBuilder.tstest/continuous-test-suite-bugfixes.ts
Comment on lines
+2403
to
+2413
| const requestedPages = pdf.pageCount; | ||
| if ( | ||
| typeof requestedPages === "number" && | ||
| requestedPages > conversionResult.pageCount | ||
| ) { | ||
| logger.warn( | ||
| `[PDF→Image] ${safeBasename(pdf.filename)} has ${requestedPages} pages but only ` + | ||
| `${conversionResult.pageCount} were converted (maxPages limit). The model will not ` + | ||
| `see the remaining pages — raise pdfOptions.maxPages or split the document.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the resolved page count for the truncation warning.
For input.content, pageCount is null when metadata.pages is omitted. enforceAggregatePdfLimits() resolves the actual count from the PDF bytes, but does not retain it. Line 2405 then skips this warning when conversion truncates the document at maxPages.
Return or store the resolved page count before conversion. Use that value for this warning.
🤖 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/lib/utils/messageBuilder.ts` around lines 2403 - 2413, Update the PDF
processing flow around enforceAggregatePdfLimits() to retain its resolved page
count before conversion, including when input.content pageCount is null because
metadata.pages is omitted. Use that resolved value in the requestedPages check
for the truncation warning, while preserving the existing warning message and
behavior for non-truncated conversions.
There was a problem hiding this comment.
Pull request overview
This PR makes the PDF→image fallback’s render scale and maxPages configurable from generate()/stream() options, ensuring PDF_LIMITS.DEFAULT_SCALE and the page ceiling are actually reachable in the real conversion path, and adds a warning when a PDF is truncated by the page ceiling.
Changes:
- Thread
pdfOptions.scaleandpdfOptions.maxPagesthrough bothinput.pdfFilesandinput.contentpaths into the PDF→image conversion call. - Replace the inline PDF entry shape in
convertMultimodalToProviderFormatwith the canonicalMultimodalPdfEntry. - Add a regression test verifying the real conversion path receives defaults and per-request overrides.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/continuous-test-suite-bugfixes.ts | Adds a regression test to confirm the real PDF→image converter receives default and overridden scale/maxPages. |
| src/lib/utils/messageBuilder.ts | Threads scale/page-ceiling options into the conversion call and warns when documents are truncated by maxPages. |
| src/lib/types/stream.ts | Exposes pdfOptions.scale and pdfOptions.maxPages on streaming options. |
| src/lib/types/generate.ts | Exposes pdfOptions.scale and pdfOptions.maxPages on generate options (but needs alignment with TextGenerationOptions). |
| src/lib/types/file.ts | Extends MultimodalPdfEntry to carry scale/maxPages through the message-building pipeline. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
2381
to
2383
| const conversionResult = await PDFImageConverter.convertToImages( | ||
| pdf.buffer, | ||
| { |
Comment on lines
+2400
to
+2413
| // The renderer stops at maxPages, so a longer document is silently | ||
| // truncated — say so rather than letting the model answer from a | ||
| // partial document as though it had the whole thing. | ||
| const requestedPages = pdf.pageCount; | ||
| if ( | ||
| typeof requestedPages === "number" && | ||
| requestedPages > conversionResult.pageCount | ||
| ) { | ||
| logger.warn( | ||
| `[PDF→Image] ${safeBasename(pdf.filename)} has ${requestedPages} pages but only ` + | ||
| `${conversionResult.pageCount} were converted (maxPages limit). The model will not ` + | ||
| `see the remaining pages — raise pdfOptions.maxPages or split the document.`, | ||
| ); | ||
| } |
Comment on lines
+1841
to
+1851
| const original = | ||
| PDFImageConverter.convertToImages.bind(PDFImageConverter); | ||
| ( | ||
| PDFImageConverter as unknown as Record<string, unknown> | ||
| ).convertToImages = async ( | ||
| buffer: Buffer, | ||
| opts: { scale?: number; maxPages?: number }, | ||
| ) => { | ||
| seen.push({ scale: opts?.scale, maxPages: opts?.maxPages }); | ||
| return original(buffer, opts); | ||
| }; |
Comment on lines
152
to
+158
| /** PDF processing options (#258). */ | ||
| pdfOptions?: { | ||
| /** Password for an encrypted PDF (image-conversion fallback path). */ | ||
| password?: string; | ||
| /** Max rendered-canvas pixels per page (#260 memory guard); oversized pages auto-downscale. */ | ||
| maxCanvasPixels?: number; | ||
| /** |
Contributor
Review Summary for PR #1301Decision: ✅ APPROVEDSummary of ChangesThis PR adds two new optional configuration options for PDF image fallback processing (#297):
Files Changed:
Impact Analysis
Key Improvements
Verification
RecommendationSafe to merge - This is a non-breaking enhancement that addresses a real issue with PDF processing memory costs while giving users control over quality vs resource usage. The implementation is clean, well-tested, and follows existing patterns. |
Tara-ag
approved these changes
Aug 7, 2026
Tara-ag
approved these changes
Aug 7, 2026
…ate() PDF_LIMITS.DEFAULT_SCALE was lowered to 1.5 to cut the image-fallback memory cost, but the only PDF-to-image call the product actually makes hardcoded `scale: 2.0` and `maxPages: 20`, so nothing downstream ever saw the new default. GenerateOptions.pdfOptions exposed neither knob either, so no caller could reach it. The 776MB-for-100-pages figure in #297 was unchanged in practice for every provider without native PDF support. The conversion now defaults to PDF_LIMITS.DEFAULT_SCALE / DEFAULT_MAX_PAGES and both are settable per request via pdfOptions.scale / pdfOptions.maxPages, carried through the input.pdfFiles and input.content paths alike. Verified against the real path: default {scale:1.5,maxPages:20}, and an override of either reaches the converter. Truncation is no longer silent: when a document has more pages than maxPages, the pages past the ceiling were dropped without a word and the model answered from a partial document as though it were whole. That now warns. convertMultimodalToProviderFormat took a fourth inline copy of the PDF entry shape, which is what stopped the knobs one frame short of the conversion; it now uses MultimodalPdfEntry (#309). Not addressed here: PDFProcessor.convertToImagesStream (#302) is still reachable from nowhere. Wiring it into this path would not lower peak memory on its own, because the message builder collects every page into one array before building the message — that needs a streaming-aware pipeline, not a call-site swap, so it stays open. Closes #297
Contributor
Author
|
All five review findings addressed — all were valid.
The test now also asserts the validation rejections, so a regression on any of |
murdore
force-pushed
the
fix/pdf-scale-reachable
branch
from
August 7, 2026 19:41
f7b3c8f to
eaed8dd
Compare
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Contributor
Review SummaryDecision: APPROVED ✅ This PR successfully addresses issue #297 by making PDF rendering scale and page limits actually configurable from What was fixed:
Impact Analysis:
Quality Checks Passed:✅ Security: No secrets or vulnerabilities found Test Coverage:
Files Changed:
All checks passed. Ready to merge. 🚀 |
Contributor
|
✅ Approved — This PR makes PDF render scale (1.5) and page ceiling (20) configurable via Summary of changes:
Impact assessment:
Key validations performed:
Recommendation: Safe to merge. The implementation correctly exposes previously hardcoded rendering knobs as configurable options, improving memory management for large PDFs while maintaining backward compatibility. |
Contributor
|
👍 Approved — No issues found. The implementation correctly exposes PDF render scale and page ceiling as configurable options while maintaining backward compatibility. All validations, tests, and documentation are in place. |
Contributor
|
🎉 This PR is included in version 10.10.9 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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
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.
Closes #297 · leaves #302 open with a reason
The gap
PDF_LIMITS.DEFAULT_SCALEwas lowered to 1.5 to cut the image-fallback memory cost. It never took effect.The only PDF→image call the product actually makes hardcoded
scale: 2.0andmaxPages: 20, andGenerateOptions.pdfOptionsexposed neither knob — so no caller could reach the new default either. The 776MB-for-100-pages figure in the issue was unchanged in practice for every provider without native PDF support (Azure, Mistral, Ollama, …).The constant was lowered; the product kept rendering at 2.0.
The fix
Verified against the real path by intercepting the converter:
{scale: 1.5, maxPages: 20}pdfOptions: { scale: 0.8 }{scale: 0.8, maxPages: 20}pdfOptions: { maxPages: 3 }{scale: 1.5, maxPages: 3}Both knobs are now settable per request and carried through the
input.pdfFilesandinput.contentpaths alike.Silent truncation also fixed. Pages past
maxPageswere dropped without a word — the model answered from a partial document as though it had the whole thing. That now warns with the page counts and how to raise the ceiling.The root cause of the knobs stopping one frame short:
convertMultimodalToProviderFormatheld a fourth inline copy of the PDF entry shape. It now usesMultimodalPdfEntry(#309).What I did not do — #302
PDFProcessor.convertToImagesStreamis still reachable from nowhere (grep: zero references outside its own file and the docs). I left it that way deliberately.Wiring it into this call site would not lower peak memory:
buildMultimodalMessagesArraycollects every page into oneimagesarray before building the message, so the pages end up resident regardless of how they arrive. Making streaming pay off needs a streaming-aware message pipeline, which is a redesign rather than a call-site swap. Claiming #302 here would have been closing it on paper.Verification
Confirmed the new test catches a regression — restoring the hardcoded
scale: 2.0makes it report✗and exit non-zero.tsc --noEmit --strictclean.Summary by CodeRabbit
New Features
Bug Fixes