Repository navigation
fix(pdf-processor): enforce page limits by default with actionable alternatives - #801
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughAdded an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PDFProcessor
participant Provider
participant Logger
participant ErrorFactory
Client->>PDFProcessor: submit PDF
PDFProcessor->>Provider: estimatePages(pdf)
Provider-->>PDFProcessor: estimatedPages
alt estimatedPages > provider.maxPages and enforceLimits = true
PDFProcessor->>ErrorFactory: pdfPageLimitExceeded(estimatedPages, maxPages, provider)
ErrorFactory-->>PDFProcessor: NeuroLinkError
PDFProcessor->>Client: throw NeuroLinkError
else estimatedPages > provider.maxPages and enforceLimits = false
PDFProcessor->>Logger: warn("bypassing page limits", details)
PDFProcessor->>PDFProcessor: continue processing
PDFProcessor->>Client: return result
else within limits
PDFProcessor->>PDFProcessor: proceed with conversion
PDFProcessor->>Client: return result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 Trigger a Full review on this PR and check whether the changes are according to the #263 |
🧩 Analysis chain
🏁 Script executed: Length of output: 2215 🏁 Script executed: Length of output: 2412
✅ Changes Match Issue RequirementsThe PR implementation aligns very well with the issue specifications: 1. Core Functionality ✓
2. Error Enforcement ✓The code now throws an error with exactly 5 actionable alternatives:
3. Bypass Mode ✓When
4. No Breaking Changes ✓
|
There was a problem hiding this comment.
Pull request overview
This pull request changes PDF page limit validation from logging a warning to throwing an error by default. It adds an enforceLimits option to PDFProcessorOptions (defaulting to true) that allows users to bypass the limit with an explicit opt-in.
Changes:
- Added
enforceLimits?: booleanfield toPDFProcessorOptionstype with JSDoc documentation - Modified page limit validation logic to throw an error with actionable alternatives when limits are exceeded (default behavior)
- Added bypass mechanism with
enforceLimits: falsethat logs a prominent warning instead of throwing
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/lib/types/fileTypes.ts | Added enforceLimits optional boolean field to PDFProcessorOptions with JSDoc explaining default behavior |
| src/lib/utils/pdfProcessor.ts | Updated page limit validation to enforce limits by default, throwing an error with 5 actionable alternatives, or logging a warning when bypassed |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/lib/utils/pdfProcessor.ts`:
- Around line 203-215: The new thrown Error in the PDF page-limit check should
use the SDK's ErrorFactory to create a typed error instead of raw Error; locate
the conditional that checks metadata.estimatedPages, config.maxPages and
options?.enforceLimits in pdfProcessor (the throw inside the block that
references provider and PDF_PROVIDER_CONFIGS) and replace the new Error(...)
with a call to ErrorFactory (constructing a clear error code/message and
including details like estimatedPages, maxPages, provider, and the suggested
alternatives in the error metadata or message) so the error is typed and
consistent with the rest of the SDK.
- Around line 203-222: Add unit tests for the new enforceLimits behavior in
pdfProcessor: create tests that (1) verify a PDF with metadata.estimatedPages <=
config.maxPages processes normally (under-limit), (2) assert that when
metadata.estimatedPages > config.maxPages and options.enforceLimits is not false
the function throws an Error whose message includes the detected pages,
config.maxPages and the Google AI Studio maxPages string from
PDF_PROVIDER_CONFIGS["google-ai-studio"].maxPages, and (3) assert that when
metadata.estimatedPages > config.maxPages but options.enforceLimits === false
the function does not throw and emits a logger.warn containing the bypass
warning text and page counts; use the same entry point that reads
metadata.estimatedPages/config.maxPages/options.enforceLimits and stub/mock
logger.warn to capture warnings and any network/API calls to isolate behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lib/utils/pdfProcessor.ts`:
- Around line 207-215: The error message in the PDF page-limit throw is
misleading because it suggests Google AI Studio has a higher page limit; update
the thrown Error constructed in the PDF page check (the throw new Error block
that references metadata.estimatedPages, provider, config.maxPages and
PDF_PROVIDER_CONFIGS["google-ai-studio"].maxPages) to either remove the “use
Google AI Studio for higher page limits” suggestion or replace it with an
accurate note about Google AI Studio's larger file-size allowance (e.g., mention
~2000MB size limit) and keep the other alternatives and the enforceLimits hint
intact so users get correct guidance.
🧹 Nitpick comments (1)
src/lib/utils/pdfProcessor.ts (1)
206-215: Use ErrorFactory with a new method for PDF validation errors.While the coding guidelines specify using ErrorFactory for typed errors across the SDK, there's currently no PDF-specific error method in ErrorFactory (unlike the pattern used for PPT and image validation). Consider creating a new static method like
ErrorFactory.invalidPDFPageLimit(estimatedPages, maxPages, provider)to handle this case, following the same pattern asinvalidPPTPagesandimageTooLarge. This would both align with SDK conventions and improve error classification downstream.
b9cfdb7 to
2326e9c
Compare
|
@coderabbitai Trigger a Full review on this PR and check whether the changes are according to the #263 |
|
✅ Validation ResultsThe PR implementation matches issue
|
|
@coderabbitai Can you check again as I have already added the test files |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 7412 🏁 Script executed: Length of output: 56493
✅ Test Coverage ValidationYour test suite ( Core Requirements from Issue
|
|
@murdore please review and merge |
2326e9c to
90611f4
Compare
…ternatives - Add enforceLimits option to PDFProcessorOptions (default: true) - Throw error with 5 actionable alternatives when page limit exceeded - Allow bypass with enforceLimits: false (logs prominent warning) Fixes: Page limit check only logged warning but did not prevent processing
90611f4 to
89064a8
Compare
|
🎉 This PR is included in version 9.0.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Pull Request
Description
What does this PR do?
Enforces PDF page limits by default, throwing an actionable error instead of only logging a warning. Previously, a 10,000-page PDF would be sent to the API despite exceeding limits, causing rejection, token errors, or unexpected costs.
Related Issues
Fixes #(issue number for "Page limit check only logs a warning but doesn't prevent processing")
Type of Change
Please select the type of change:
Motivation and Context
Why is this change needed? What problem does it solve?
pdfProcessor.tsonly calledlogger.warn()when page limit was exceeded. Processing continued and oversized PDFs were sent to provider APIs.enforceLimitsoption (default: true) that throws an error with actionable alternatives when limits are exceededChanges Made
What specific changes were made?
enforceLimits?: booleanoption toPDFProcessorOptionsinsrc/lib/types/fileTypes.ts(default: true)src/lib/utils/pdfProcessor.tsto checkoptions?.enforceLimits !== falseenforceLimits: false): logs prominent warning about bypass risks{ enforceLimits: false }(not recommended)Breaking Changes
Does this PR introduce breaking changes?
This is technically a behavior change (error instead of warning), but it prevents invalid API calls that would fail anyway. Users who intentionally want to bypass can use
enforceLimits: false.Testing
How has this been tested?
Test Coverage
Required tests (per acceptance criteria):
enforceLimits: falseworksManual Testing Steps
{ enforceLimits: false }→ should log warning and continueCode Quality
Have you followed code quality standards?
Documentation
Have you updated documentation?
Commit Message Format
Does your commit follow semantic commit conventions?
type(scope): descriptionCommit:
fix(pdf-processor): enforce page limits by default with actionable alternativesDependencies
Does this PR add, update, or remove dependencies?
Performance Impact
Does this change affect performance?
Security Considerations
Are there any security implications?
This change improves cost control by preventing unintended large API calls.
Deployment Notes
Special deployment instructions?
Screenshots / Videos
N/A - No UI changes
Reviewer Checklist
For reviewers:
Additional Notes
Any additional information for reviewers:
type:bug,priority:critical,component:pdf-processor,modality:pdfPre-submission Checklist
Before submitting, ensure you have:
pnpm testpnpm buildpnpm run validate:alland all checks passThank you for contributing to NeuroLink!
Summary by CodeRabbit