Repository navigation
fix(pdf): reject empty PDFs with 0 pages instead of returning success - #780
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 a runtime validation in PDF conversion to reject empty PDFs by throwing "PDF has 0 pages. Cannot convert empty PDF to images." after page conversion. Added unit tests that mock page conversion for 0- and 1-page PDFs and validate error messaging. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 Full review and check if the changes are according to the issue #335 |
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
🧩 Analysis chain
🏁 Script executed: Length of output: 1846 🏁 Script executed: Length of output: 8432 🏁 Script executed: Length of output: 2315 🏁 Script executed: Length of output: 2873 🏁 Script executed: Length of output: 2374 🏁 Script executed: Length of output: 7626
✅ Alignment with Issue
|
|
@murdore Please review and merge |
![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 414-417: Replace the plain throw new Error(...) in the PDF page
check with ErrorFactory.invalidParameters to follow project error patterns:
where images.length === 0 currently throws, construct and throw
ErrorFactory.invalidParameters("pdf-processor", new Error("PDF has 0 pages.
Cannot convert empty PDF to images.")); locate this in the pdfProcessor.ts
function that processes PDF pages and ensure the new ErrorFactory call mirrors
the message and type used elsewhere (or add a dedicated
ErrorFactory.pdfProcessing(...) helper if you expect reuse).
In `@test/unit/utils/pdfProcessor.test.ts`:
- Around line 6-31: Add vi.resetModules() before calling vi.doMock() in this
test to clear the module cache so importing
"../../../src/lib/utils/pdfProcessor.js" picks up the mocked "pdf-to-img";
specifically, insert vi.resetModules() immediately before
vi.doMock("pdf-to-img", ...) in the test that imports MockedPDFProcessor and
asserts on MockedPDFProcessor.convertToImages, and apply the same change to the
other two tests in this suite that use vi.doMock()/vi.doUnmock() so each test
re-imports a fresh module instance.
🧹 Nitpick comments (1)
test/unit/utils/pdfProcessor.test.ts (1)
58-85: Test validates error message clarity as required.This test fulfills the acceptance criterion: "Error message explicitly mentions 0 pages and is actionable." The try/catch pattern works, though you could also use
rejects.toThrow()combined with checking the error object:♻️ Alternative using rejects pattern
it("should provide clear error message mentioning 0 pages", async () => { // ... setup code ... - try { - await MockedPDFProcessor.convertToImages(validPdfHeader); - expect.fail("Should have thrown an error"); - } catch (error) { - const errorMessage = (error as Error).message; - expect(errorMessage).toContain("0 pages"); - expect(errorMessage).toContain("empty PDF"); - } + await expect( + MockedPDFProcessor.convertToImages(validPdfHeader), + ).rejects.toSatisfy((error: Error) => { + expect(error.message).toContain("0 pages"); + expect(error.message).toContain("empty PDF"); + return true; + }); vi.doUnmock("pdf-to-img"); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/utils/pdfProcessor.tstest/unit/utils/pdfProcessor.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
src/lib/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
src/lib/**/*.ts: Use MessageBuilder (src/lib/utils/messageBuilder.ts) as the central component for constructing all messages with multimodal support (text, images, PDFs, CSV)
Wrap async operations with withTimeout utility for timeout handling
Use ErrorFactory for creating typed errors instead of generic Error objects
thinkingLevel option supports "minimal", "low", "medium" (default), and "high" values for extended thinking on supported models (Anthropic Claude, Gemini 2.5+, Gemini 3)
Files:
src/lib/utils/pdfProcessor.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain strict TypeScript type safety across all modules with no any types unless justified
Files:
src/lib/utils/pdfProcessor.ts
🧠 Learnings (1)
📚 Learning: 2025-08-19T05:02:34.555Z
Learnt from: coder-dodo
Repo: juspay/neurolink PR: 87
File: test/provider-edge-cases.test.ts:16-21
Timestamp: 2025-08-19T05:02:34.555Z
Learning: When using vi.mock() in Vitest, mocking the same import path that contains the system under test replaces the actual implementation with a mock, even if the physical file has a different extension (.ts vs .js). TypeScript's NodeNext module resolution treats .js imports as references to .ts files.
Applied to files:
test/unit/utils/pdfProcessor.test.ts
🧬 Code graph analysis (1)
test/unit/utils/pdfProcessor.test.ts (1)
src/lib/utils/logger.ts (1)
error(239-241)
🔇 Additional comments (1)
test/unit/utils/pdfProcessor.test.ts (1)
33-56: LGTM!Good positive test case verifying that a 1-page PDF succeeds. The assertions for
pageCountandimages.lengthproperly validate the conversion result.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Check for empty PDF (0 pages) | ||
| if (images.length === 0) { | ||
| throw new Error("PDF has 0 pages. Cannot convert empty PDF to images."); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if ErrorFactory exists and how it's used
echo "=== Checking for ErrorFactory usage ==="
rg -n "ErrorFactory" --type ts -A 3
echo ""
echo "=== Checking file path and content around the target lines ==="
if [ -f "src/lib/utils/pdfProcessor.ts" ]; then
echo "File exists, checking content around lines 414-417:"
sed -n '410,445p' src/lib/utils/pdfProcessor.ts | cat -n
else
echo "File not found, searching for pdfProcessor:"
fd pdfProcessor
fiRepository: juspay/neurolink
Length of output: 15532
🏁 Script executed:
# Check error patterns in src/lib/utils
echo "=== Errors in src/lib/utils with ErrorFactory vs generic Error ==="
rg -n "throw" src/lib/utils/ --type ts -B 1 -A 1 | head -50Repository: juspay/neurolink
Length of output: 2533
🏁 Script executed:
# Search for all methods in ErrorFactory to see available options
echo "=== ErrorFactory method definitions ==="
sed -n '117,400p' src/lib/utils/errorHandling.ts | grep -E "^\s+(static\s+)?[a-zA-Z]+\(" | head -30Repository: juspay/neurolink
Length of output: 717
🏁 Script executed:
# Check if there's a generic processingError or conversionError method
rg -n "conversionError|processingError|fileProcessing" src/lib/utils/errorHandling.tsRepository: juspay/neurolink
Length of output: 42
Replace generic Error with ErrorFactory to comply with coding guidelines.
The throw new Error() on line 416 should use ErrorFactory instead, per the coding guidelines for src/lib/**/*.ts. The most appropriate option is ErrorFactory.invalidParameters("pdf-processor", new Error(...)) to match the validation pattern used elsewhere in the codebase. Alternatively, consider adding a dedicated PDF processing error method to ErrorFactory if PDF-specific error handling is expected to be reused.
The error re-wrapping in the catch block (line 434) is intentional and correct—the original message is preserved within the new context.
🤖 Prompt for AI Agents
In `@src/lib/utils/pdfProcessor.ts` around lines 414 - 417, Replace the plain
throw new Error(...) in the PDF page check with ErrorFactory.invalidParameters
to follow project error patterns: where images.length === 0 currently throws,
construct and throw ErrorFactory.invalidParameters("pdf-processor", new
Error("PDF has 0 pages. Cannot convert empty PDF to images.")); locate this in
the pdfProcessor.ts function that processes PDF pages and ensure the new
ErrorFactory call mirrors the message and type used elsewhere (or add a
dedicated ErrorFactory.pdfProcessing(...) helper if you expect reuse).
| it("should throw error for PDF with 0 pages", async () => { | ||
| // Create a valid PDF header buffer that will pass initial validation | ||
| const validPdfHeader = Buffer.from( | ||
| "%PDF-1.4\n1 0 obj<</Type/Catalog/Pages 2 0 R>>endobj 2 0 obj<</Type/Pages/Count 0/Kids[]>>endobj\nxref\n0 3\n0000000000 65535 f\n0000000009 00000 n\n0000000058 00000 n\ntrailer<</Size 3/Root 1 0 R>>\nstartxref\n150\n%%EOF", | ||
| ); | ||
|
|
||
| // Mock pdf-to-img to return an empty async iterator (0 pages) | ||
| vi.doMock("pdf-to-img", () => ({ | ||
| pdf: vi.fn().mockResolvedValue({ | ||
| [Symbol.asyncIterator]: async function* () { | ||
| // Empty iterator - no pages | ||
| }, | ||
| }), | ||
| })); | ||
|
|
||
| // Re-import to get the mocked version | ||
| const { PDFProcessor: MockedPDFProcessor } = await import( | ||
| "../../../src/lib/utils/pdfProcessor.js" | ||
| ); | ||
|
|
||
| await expect( | ||
| MockedPDFProcessor.convertToImages(validPdfHeader), | ||
| ).rejects.toThrow("PDF has 0 pages. Cannot convert empty PDF to images."); | ||
|
|
||
| vi.doUnmock("pdf-to-img"); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Vitest vi.doMock vi.resetModules module isolation best practices
💡 Result:
Short best-practices for module isolation with vi.doMock / vi.resetModules (Vitest)
- Know hoisting vs non-hoisting
- vi.mock is hoisted (runs before imports). Use it when you want the module mocked for all imports in the file. (v3.vitest.dev)
- Use vi.doMock when you need the mock to run at runtime
- vi.doMock is NOT hoisted and will only affect subsequent dynamic imports. Use it when the mock needs to reference variables defined in the test scope. (vitest.dev)
- Reset module cache to isolate module-local state
- vi.resetModules() clears the module cache so subsequent dynamic imports are re-evaluated; call it in beforeEach or inside vi.hoisted/setup to ensure fresh modules per test. Top-level static imports cannot be re-evaluated. (vitest.dev)
- Reset mocks separate from module cache
- vi.resetModules() does NOT clear mock registry. Use vi.unmock / vi.doUnmock or vi.resetAllMocks()/vi.restoreAllMocks() as appropriate to clear mocks between tests. (vitest.dev)
- Prefer dynamic import after reset/mock when you need isolation
- Pattern: vi.resetModules(); vi.doMock(...) (if non-hoisted mocking needed); then await import('./module') so the import picks up the intended mocked or fresh implementation. (vitest.dev)
- Avoid importing the target module earlier (including in setup files)
- Modules imported in setup files or earlier in the file will be cached and not affected by later doMock/resetModules; if you mock in setup, ensure you manage cache (vi.resetModules()) or avoid pre-importing. Real-world reports show unexpected failures when setup imports block later mocks. (v1.vitest.dev)
- For Jest parity / advanced isolation
- Jest’s isolateModules/isolateModulesAsync provides per-callback registries; Vitest relies on resetModules + dynamic imports and doMock/hoisting semantics instead. Use the dynamic-import pattern for similar isolation. (jestjs.io)
Minimal example pattern
- beforeEach(() => { vi.resetModules(); vi.resetAllMocks(); })
- vi.doMock('./dep', () => ({ fn: vi.fn(()=>'x') })) // if non-hoisted needed
- const mod = await import('./module') // will see mocked dep / fresh module. (vitest.dev)
If you want, I can convert these into copy-paste test examples (Common pitfalls + working examples).
Citations:
- 1: https://v3.vitest.dev/api/vi?utm_source=openai
- 2: https://vitest.dev/api/vi?utm_source=openai
- 3: https://vitest.dev/api/vi?utm_source=openai
- 4: https://vitest.dev/api/vi?utm_source=openai
- 5: https://vitest.dev/api/vi?utm_source=openai
- 6: https://v1.vitest.dev/api/vi.html?utm_source=openai
- 7: https://jestjs.io/docs/jest-object?utm_source=openai
- 8: https://vitest.dev/api/vi?utm_source=openai
Add vi.resetModules() to ensure proper test isolation with dynamic mocks.
The test correctly verifies the exact error message for the 0-page case. However, vi.doUnmock() clears only the mock registry, not the module cache. Without vi.resetModules(), subsequent tests may reuse a cached module from the previous test rather than reimporting with the new mock applied. Add vi.resetModules() before each vi.doMock() to ensure each test gets a fresh module instance.
🔧 Required fix for test isolation
it("should throw error for PDF with 0 pages", async () => {
// Create a valid PDF header buffer that will pass initial validation
const validPdfHeader = Buffer.from(
"%PDF-1.4\n1 0 obj<</Type/Catalog/Pages 2 0 R>>endobj 2 0 obj<</Type/Pages/Count 0/Kids[]>>endobj\nxref\n0 3\n0000000000 65535 f\n0000000009 00000 n\n0000000058 00000 n\ntrailer<</Size 3/Root 1 0 R>>\nstartxref\n150\n%%EOF",
);
+ // Reset module registry to ensure clean state
+ vi.resetModules();
+
// Mock pdf-to-img to return an empty async iterator (0 pages)
vi.doMock("pdf-to-img", () => ({Apply the same pattern to the other two tests in this suite.
📝 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.
| it("should throw error for PDF with 0 pages", async () => { | |
| // Create a valid PDF header buffer that will pass initial validation | |
| const validPdfHeader = Buffer.from( | |
| "%PDF-1.4\n1 0 obj<</Type/Catalog/Pages 2 0 R>>endobj 2 0 obj<</Type/Pages/Count 0/Kids[]>>endobj\nxref\n0 3\n0000000000 65535 f\n0000000009 00000 n\n0000000058 00000 n\ntrailer<</Size 3/Root 1 0 R>>\nstartxref\n150\n%%EOF", | |
| ); | |
| // Mock pdf-to-img to return an empty async iterator (0 pages) | |
| vi.doMock("pdf-to-img", () => ({ | |
| pdf: vi.fn().mockResolvedValue({ | |
| [Symbol.asyncIterator]: async function* () { | |
| // Empty iterator - no pages | |
| }, | |
| }), | |
| })); | |
| // Re-import to get the mocked version | |
| const { PDFProcessor: MockedPDFProcessor } = await import( | |
| "../../../src/lib/utils/pdfProcessor.js" | |
| ); | |
| await expect( | |
| MockedPDFProcessor.convertToImages(validPdfHeader), | |
| ).rejects.toThrow("PDF has 0 pages. Cannot convert empty PDF to images."); | |
| vi.doUnmock("pdf-to-img"); | |
| }); | |
| it("should throw error for PDF with 0 pages", async () => { | |
| // Create a valid PDF header buffer that will pass initial validation | |
| const validPdfHeader = Buffer.from( | |
| "%PDF-1.4\n1 0 obj<</Type/Catalog/Pages 2 0 R>>endobj 2 0 obj<</Type/Pages/Count 0/Kids[]>>endobj\nxref\n0 3\n0000000000 65535 f\n0000000009 00000 n\n0000000058 00000 n\ntrailer<</Size 3/Root 1 0 R>>\nstartxref\n150\n%%EOF", | |
| ); | |
| // Reset module registry to ensure clean state | |
| vi.resetModules(); | |
| // Mock pdf-to-img to return an empty async iterator (0 pages) | |
| vi.doMock("pdf-to-img", () => ({ | |
| pdf: vi.fn().mockResolvedValue({ | |
| [Symbol.asyncIterator]: async function* () { | |
| // Empty iterator - no pages | |
| }, | |
| }), | |
| })); | |
| // Re-import to get the mocked version | |
| const { PDFProcessor: MockedPDFProcessor } = await import( | |
| "../../../src/lib/utils/pdfProcessor.js" | |
| ); | |
| await expect( | |
| MockedPDFProcessor.convertToImages(validPdfHeader), | |
| ).rejects.toThrow("PDF has 0 pages. Cannot convert empty PDF to images."); | |
| vi.doUnmock("pdf-to-img"); | |
| }); |
🤖 Prompt for AI Agents
In `@test/unit/utils/pdfProcessor.test.ts` around lines 6 - 31, Add
vi.resetModules() before calling vi.doMock() in this test to clear the module
cache so importing "../../../src/lib/utils/pdfProcessor.js" picks up the mocked
"pdf-to-img"; specifically, insert vi.resetModules() immediately before
vi.doMock("pdf-to-img", ...) in the test that imports MockedPDFProcessor and
asserts on MockedPDFProcessor.convertToImages, and apply the same change to the
other two tests in this suite that use vi.doMock()/vi.doUnmock() so each test
re-imports a fresh module instance.
df3c9c9 to
717ef2a
Compare
|
🎉 This PR is included in version 8.35.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
What does this PR do?
Adds validation to detect and reject PDFs with 0 pages during image conversion, providing clear error feedback instead of silently returning an empty result.
Related Issues
Fixes #335
Type of Change
Motivation and Context
Why is this change needed? What problem does it solve?
Previously, when
PDFProcessor.convertToImages()processed a PDF with 0 pages, the page iteration loop would never execute, resulting in an empty array being returned with a success message. This was confusing for users who expected either valid results or a clear error.Impact of the bug:
Changes Made
convertToImages()method"PDF has 0 pages. Cannot convert empty PDF to images."Files modified:
src/lib/utils/pdfProcessor.ts- Added empty PDF validationtest/unit/utils/pdfProcessor.test.ts- Added 3 new test casesBreaking Changes
Testing
Test Coverage:
New tests added:
should throw error for PDF with 0 pages- Verifies rejection of empty PDFsshould succeed for PDF with at least 1 page- Verifies normal PDFs still workshould provide clear error message mentioning 0 pages- Verifies error message is actionableManual Testing Steps:
pnpm test -- --run test/unit/utils/pdfProcessor.test.tsCode Quality
Documentation
Commit Message Format
type(scope): descriptionSummary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.