[codex] Add PDF file upload ingest - #24
Conversation
|
Too much diff to scan? Review this PR in Change Stack to start with the highest-impact changes. Warning Review limit reached
More reviews will be available in 1 minute and 26 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR introduces a complete multipart file upload feature for the ingest API ( ChangesFile Upload Ingestion Flow
Retrieval and Citation Improvements
Query Planning and Frontend
Tests and Documentation
🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
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 unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
backend/app/chat/service.py (1)
145-145: 💤 Low valueConsider extracting the structural heading token threshold.
The
<= 12token limit for detecting structural headings is a magic number. While acceptable for this heuristic, extracting it as a named constant (e.g.,_STRUCTURAL_HEADING_MAX_TOKENS = 12) would improve maintainability.🤖 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 `@backend/app/chat/service.py` at line 145, Extract the magic number 12 into a named module-level constant like _STRUCTURAL_HEADING_MAX_TOKENS = 12 and replace the literal in the structural heading check (the return in the function using _support_tokens) with a comparison against that constant (e.g., return len(_support_tokens(text)) <= _STRUCTURAL_HEADING_MAX_TOKENS) so the threshold is easy to find and adjust.docs/data-model/er-diagram.md (1)
29-29: 💤 Low valueConsider alphabetizing the source type enum.
The source type values are currently listed in insertion order rather than alphabetically. While this doesn't affect functionality, consistent alphabetical ordering improves maintainability.
📝 Suggested alphabetical order
- text type "enum: notes_folder|github|rss|pdf_upload|file_upload|bookmark|research_note|manual" + text type "enum: bookmark|file_upload|github|manual|notes_folder|pdf_upload|research_note|rss"🤖 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 `@docs/data-model/er-diagram.md` at line 29, The enum listing for the source/type field is not alphabetized; reorder the enum values in the type declaration (the string: "enum: notes_folder|github|rss|pdf_upload|file_upload|bookmark|research_note|manual") into alphabetical order (by value name) so the enum reads consistently and is easier to maintain; update the enum string accordingly wherever the type/source enum is defined in the ER diagram.backend/tests/unit/test_upload_parsers.py (1)
8-39: 💤 Low valueConsider extracting PDF test helpers to shared test utilities.
The
_sample_pdf_bytes()helper is duplicated betweentest_ingest.pyandtest_upload_parsers.py. While this duplication is minor for test code, extracting it to a shared test utility module (e.g.,tests/helpers.py) would improve maintainability if more tests need to generate test PDFs.🤖 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 `@backend/tests/unit/test_upload_parsers.py` around lines 8 - 39, The _sample_pdf_bytes() helper is duplicated; extract it into a shared test utility (e.g., create a tests.helpers module) by moving the function (rename to sample_pdf_bytes or keep same name) into that module and then update test_upload_parsers.py and test_ingest.py to import the helper from tests.helpers instead of defining it inline; remove the duplicate definitions in both test files and run tests to ensure imports and references to _sample_pdf_bytes() (or the chosen name) are updated accordingly.
🤖 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.
Nitpick comments:
In `@backend/app/chat/service.py`:
- Line 145: Extract the magic number 12 into a named module-level constant like
_STRUCTURAL_HEADING_MAX_TOKENS = 12 and replace the literal in the structural
heading check (the return in the function using _support_tokens) with a
comparison against that constant (e.g., return len(_support_tokens(text)) <=
_STRUCTURAL_HEADING_MAX_TOKENS) so the threshold is easy to find and adjust.
In `@backend/tests/unit/test_upload_parsers.py`:
- Around line 8-39: The _sample_pdf_bytes() helper is duplicated; extract it
into a shared test utility (e.g., create a tests.helpers module) by moving the
function (rename to sample_pdf_bytes or keep same name) into that module and
then update test_upload_parsers.py and test_ingest.py to import the helper from
tests.helpers instead of defining it inline; remove the duplicate definitions in
both test files and run tests to ensure imports and references to
_sample_pdf_bytes() (or the chosen name) are updated accordingly.
In `@docs/data-model/er-diagram.md`:
- Line 29: The enum listing for the source/type field is not alphabetized;
reorder the enum values in the type declaration (the string: "enum:
notes_folder|github|rss|pdf_upload|file_upload|bookmark|research_note|manual")
into alphabetical order (by value name) so the enum reads consistently and is
easier to maintain; update the enum string accordingly wherever the type/source
enum is defined in the ER diagram.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aca293bb-ab54-41b4-abdc-f1ba3addafc3
📒 Files selected for processing (24)
backend/app/agentic_rag/service.pybackend/app/api/ingest.pybackend/app/chat/service.pybackend/app/config.pybackend/app/db/models.pybackend/app/ingest/parsers.pybackend/app/retrieval/hybrid.pybackend/migrations/versions/0006_file_upload_source_type.pybackend/requirements.prod.txtbackend/requirements.txtbackend/tests/integration/test_ingest.pybackend/tests/integration/test_retrieval.pybackend/tests/unit/test_agentic_rag.pybackend/tests/unit/test_api_auth.pybackend/tests/unit/test_chat_citation_support.pybackend/tests/unit/test_config.pybackend/tests/unit/test_upload_parsers.pydocs/PROGRESS.mddocs/USAGE.mddocs/data-model/er-diagram.mddocs/implementation-notes.mdfrontend/app/ingest/page.tsxfrontend/lib/api/client.tsfrontend/lib/api/types.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
What changed
POST /ingest/uploadfor.pdf,.txt, and.mduploads, including local PDF text extraction withpypdf.file_uploadsource type support, migration0006, upload parser tests, API auth/config coverage, and frontend upload mode with selected-file handling.Why
Users could select
pdf_uploadmetadata but had no actual multipart upload path. After upload worked, PDF questions still failed in chat when strict full-text missed typo-adjacent queries or citation validation rejected translated English answers over Vietnamese PDF chunks.Validation
SECOND_BRAIN_TEST_DATABASE_URL=postgresql+psycopg://second_brain:second_brain@localhost:5433/second_brain SECOND_BRAIN_LLM_PROVIDER=fake SECOND_BRAIN_AGENTIC_RAG_ENABLED=false pytest->279 passed, 8 warningsnpm run lintnpm run build(passes; existing Next multiple-lockfile warning remains)npm audit --audit-level=highpython -m pip checkdocker compose config --quietgit diff --cached --check/ingest/uploadsmoke accepted an empty-password encrypted PDF and smoke source was deleted/chatprobes passed twice for normal RAG and twice for Agentic RAG on the uploaded PDF questionNotes
Summary by CodeRabbit
New Features
Bug Fixes