fix: keep default docs title the same for every type of ingestion#2118
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe PR propagates page titles into default-document metadata, makes URL ingestion filenames optional, and expands E2E coverage to validate fetching with Langflow ingestion disabled and enabled. ChangesDefault document ingestion
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant FetchLatestDocsTest
participant Settings
participant KnowledgePage
FetchLatestDocsTest->>Settings: Disable Langflow ingestion
FetchLatestDocsTest->>KnowledgePage: Fetch latest docs
KnowledgePage-->>FetchLatestDocsTest: Verify default document
FetchLatestDocsTest->>Settings: Enable Langflow ingestion
FetchLatestDocsTest->>KnowledgePage: Fetch latest docs
KnowledgePage-->>FetchLatestDocsTest: Verify default document
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
mpawlow
left a comment
There was a problem hiding this comment.
Code Review 1
- ✅ Approved / LGTM 🚀
- See Minor PR comments: (1a), (1c) for optional consideration
| knowledge, | ||
| settings, | ||
| }) => { | ||
| test.setTimeout(180000); // 3 minutes timeout |
There was a problem hiding this comment.
(1a) [Minor] E2E test timeout not increased despite doubling the test's workload
Problem
- The test was extended from a single ingestion pass to two full sequential phases (Langflow-disabled, then Langflow-enabled), each of which:
- Toggles a setting and waits for a save-confirmation toast (up to 120s per
saveIngestSettings()call). - Triggers
fetchLatestDocs(). - Polls
verifyDocumentActive(), which has its own internal 5-minute (300000ms) deadline waiting for OpenSearch indexing to complete.
- Toggles a setting and waits for a save-confirmation toast (up to 120s per
- Despite the workload roughly doubling,
test.setTimeout(180000)(3 minutes) was left unchanged. - Even a single call to
verifyDocumentActive()can internally poll for up to 5 minutes, which already exceeds the current 3-minute outer test timeout — this pre-existing mismatch is now compounded by running two full phases back-to-back.
Code References
frontend/tests/core/fetch_latest_docs.spec.ts:11(test.setTimeout(180000), unchanged)frontend/tests/core/fetch_latest_docs.spec.ts:20-46(two sequential phases added)frontend/tests/pages/Settings.ts:197-203(saveIngestSettings(), up to 120s wait)frontend/tests/pages/Knowledge.ts:717(verifyDocumentActive(), 300000ms internal deadline)
Potential Solution
- Increase
test.setTimeout(...)to comfortably cover two full phases, e.g.360000(6 minutes) or more, matching realistic worst-case indexing + save-toast latency for both phases combined.
Alternative Solutions
- Split into two separate
test(...)cases (Langflow-disabled, Langflow-enabled) each with its own 3-minute budget, so a slow phase doesn't consume budget needed by the other and failures are individually attributable.
There was a problem hiding this comment.
(1c) [Minor] Empty <title></title> produces an empty original_filename
Problem
materialize_url_as_text_filefalls back to"OpenRAG"only when the<title>tag is entirely absent (title_matchisNone).- If the tag is present but empty (
<title></title>) or whitespace-only,title_match.group(1).strip()evaluates to"", andtitle = ""is used verbatim — no fallback is applied. - This empty string then becomes
original_filename=""in_ingest_default_documents_url, producing a default document with an empty display name.
Code References
src/utils/url_content_fetcher.py:51src/services/default_docs_service.py:344(original_filename=title)
Potential Solution
- Fall back to
"OpenRAG"when the extracted title is falsy after stripping, not only when the regex fails to match:title = html.unescape(title_match.group(1).strip()) if title_match else "" title = title or "OpenRAG"
This pull request enhances the "Fetch Latest Docs" feature by improving end-to-end test coverage for both Langflow-enabled and traditional ingestion modes, and refactors backend ingestion logic to better handle document metadata. The main changes include updating tests to toggle Langflow ingestion, modifying ingestion services to use document titles from URLs, and improving type safety for optional filenames throughout the codebase.
Testing improvements:
fetch_latest_docs.spec.tsnow verifies "Fetch Latest Docs" works both with Langflow ingestion enabled and disabled, toggling the setting and validating the active document in each mode.Settingstest page object adds a method to toggle Langflow ingestion and locates the relevant UI element for the test. [1] [2]Backend ingestion and metadata handling:
materialize_url_as_text_filenow returns both the temporary file path and the document title, allowing ingestion services to use the actual title as theoriginal_filenameinstead of a hardcoded value. [1] [2] [3] [4]filenameparameter in document ingestion contexts and callbacks is now optional (str | None), improving type safety and flexibility for documents without explicit filenames. [1] [2] [3]Summary by CodeRabbit
Improvements
Bug Fixes