fix(loaders): accumulate all pages in CustomDocLoader (#376) - #400
Merged
EnjoyBacon7 merged 5 commits intoMay 21, 2026
Merged
Conversation
Previously the loop body used '=' so each iteration overwrote 's', returning only the final page. Switch to '+=' so all pages are concatenated as intended. Fixes #376
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCustomDocLoader.aload_document now concatenates each cleaned page's text ( ChangesMulti-page Document Content Preservation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openrag/components/indexer/loaders/CustomDocLoader.py (1)
22-39:⚠️ Potential issue | 🔴 CriticalAdd a unit test for CustomDocLoader that verifies multi-page document accumulation.
The PR objectives require a unit test loading a multi-page fixture (≥3 pages) and asserting that text from each page is present in the returned content, but no such test exists in the repository. The implementation in
aload_document()correctly accumulates pages with[PAGE_N]markers, but this critical functionality is untested.🤖 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 `@openrag/components/indexer/loaders/CustomDocLoader.py` around lines 22 - 39, Add a new async unit test that exercises CustomDocLoader.aload_document by loading a multi-page fixture (≥3 pages) and asserting the returned Document.page_content contains the text from each page and the corresponding "[PAGE_1]", "[PAGE_2]", "[PAGE_3]" markers; instantiate or reference the same loader registration used by CustomDocLoader.doc_loaders for the fixture suffix, call await CustomDocLoader().aload_document(fixture_path, metadata=...) (or the correct class/method used in your code), and use pytest/pytest-asyncio to run the coroutine and assert that each page's unique text and all PAGE_N markers appear in the combined content. Ensure the test uses a deterministic multi-page fixture and cleans up any temp files if created.
🧹 Nitpick comments (1)
openrag/components/indexer/loaders/CustomDocLoader.py (1)
35-37: ⚡ Quick winConsider using list + join for string concatenation.
The current implementation using
+=in a loop creates a new string object on each iteration (since strings are immutable in Python). For documents with many pages, accumulating parts in a list and joining once at the end is more efficient.⚡ Performance optimization using list + join
- s = "" + parts = [] for page_num, p in enumerate(pages, start=1): - s += p.page_content.strip() + f"\n[PAGE_{page_num}]\n" + parts.append(p.page_content.strip()) + parts.append(f"\n[PAGE_{page_num}]\n") + s = "".join(parts) return Document(page_content=s, metadata=metadata)🤖 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 `@openrag/components/indexer/loaders/CustomDocLoader.py` around lines 35 - 37, The string accumulation using s += inside the loop (variable s built from pages, page_num and p.page_content) is inefficient; change it to collect fragments in a list (append each p.page_content.strip() plus the page marker like "\n[PAGE_{page_num}]\n") and after the loop call "".join(fragments) to produce the final string, using the same page marker format so behavior of the code (and any function/class in this file that uses s) remains identical.
🤖 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.
Outside diff comments:
In `@openrag/components/indexer/loaders/CustomDocLoader.py`:
- Around line 22-39: Add a new async unit test that exercises
CustomDocLoader.aload_document by loading a multi-page fixture (≥3 pages) and
asserting the returned Document.page_content contains the text from each page
and the corresponding "[PAGE_1]", "[PAGE_2]", "[PAGE_3]" markers; instantiate or
reference the same loader registration used by CustomDocLoader.doc_loaders for
the fixture suffix, call await CustomDocLoader().aload_document(fixture_path,
metadata=...) (or the correct class/method used in your code), and use
pytest/pytest-asyncio to run the coroutine and assert that each page's unique
text and all PAGE_N markers appear in the combined content. Ensure the test uses
a deterministic multi-page fixture and cleans up any temp files if created.
---
Nitpick comments:
In `@openrag/components/indexer/loaders/CustomDocLoader.py`:
- Around line 35-37: The string accumulation using s += inside the loop
(variable s built from pages, page_num and p.page_content) is inefficient;
change it to collect fragments in a list (append each p.page_content.strip()
plus the page marker like "\n[PAGE_{page_num}]\n") and after the loop call
"".join(fragments) to produce the final string, using the same page marker
format so behavior of the code (and any function/class in this file that uses s)
remains identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d5522239-e9c0-4eb0-a31a-714ce611f6a1
📒 Files selected for processing (1)
openrag/components/indexer/loaders/CustomDocLoader.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openrag/components/indexer/loaders/test_customdocloader.py (1)
23-34: ⚡ Quick winVerify mocked loader interactions to avoid false positives.
The test should also assert the mocked loader path was actually used and awaited once, so it fails if
aload_documentstops calling the patched loader.Suggested interaction assertions
with patch.dict(CustomDocLoader.doc_loaders, {".docx": fake_loader_cls}, clear=True): # BaseLoader.__init__ pulls a config; we bypass it with object.__new__ loader = object.__new__(CustomDocLoader) result = await loader.aload_document(str(file_path), metadata={"src": "x"}) + fake_loader_cls.assert_called_once() + fake_loader_instance.aload.assert_awaited_once() + assert "page-one" in result.page_content🤖 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 `@openrag/components/indexer/loaders/test_customdocloader.py` around lines 23 - 34, Add assertions to verify the patched loader was invoked and its async load was awaited: after creating loader and calling await loader.aload_document(...), assert that fake_loader_cls was called once with the file path string (the loader factory was used) and assert that fake_loader_instance.aload was awaited exactly once with the expected metadata argument (e.g. metadata={"src":"x"}) so the test fails if CustomDocLoader.aload_document stops calling the patched loader.
🤖 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 `@openrag/components/indexer/loaders/test_customdocloader.py`:
- Around line 38-39: The test currently only asserts "[PAGE_1]" and "[PAGE_3]"
in result.page_content, missing the middle marker; update the test in
test_customdocloader.py (around the assertions for result.page_content) to also
assert that "[PAGE_2]" is present so the intermediate page marker is validated
(i.e., add an assertion that result.page_content contains "[PAGE_2]").
---
Nitpick comments:
In `@openrag/components/indexer/loaders/test_customdocloader.py`:
- Around line 23-34: Add assertions to verify the patched loader was invoked and
its async load was awaited: after creating loader and calling await
loader.aload_document(...), assert that fake_loader_cls was called once with the
file path string (the loader factory was used) and assert that
fake_loader_instance.aload was awaited exactly once with the expected metadata
argument (e.g. metadata={"src":"x"}) so the test fails if
CustomDocLoader.aload_document stops calling the patched loader.
🪄 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
Run ID: f34a8441-e730-4605-916c-751298c89f0c
📒 Files selected for processing (1)
openrag/components/indexer/loaders/test_customdocloader.py
This was referenced Jun 15, 2026
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
2 participants
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.
Summary
openrag/components/indexer/loaders/CustomDocLoader.py:37useds = p.page_content.strip() + ...inside the page loop, overwritingson every iteration. Result: every multi-page legacy.doc/.docx/.odtdocument indexed via this loader silently dropped every page except the last, with no error visible in logs.This PR changes the assignment to
s +=so all pages are accumulated.Test plan
.docfile with three or more pages and confirm all pages are present in the returned contentFixes #376
Summary by CodeRabbit
Bug Fixes
Tests