Add first-page PDF cover thumbnails on document search cards - #1661
Conversation
Generate compact non-searchable cover_page images at ingest, surface coverImageId on search matches, and render real page-1 previews in the document result cards (with placeholder fallback). Demo corpus includes static covers; add an operator backfill script for existing live PDFs. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in:45 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe PR adds first-page PDF cover extraction, stores covers as non-searchable preview images, propagates cover image IDs through search data, displays signed covers in document cards, and provides a dry-run backfill script for existing indexed PDFs. ChangesDocument cover thumbnails
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant PDFExtractor
participant IndexingWorker
participant SearchAPI
participant DocumentPreview
PDFExtractor->>IndexingWorker: provide first-page cover artifact
IndexingWorker->>IndexingWorker: persist non-searchable cover image
IndexingWorker-->>SearchAPI: provide cover image ID
SearchAPI-->>DocumentPreview: provide coverImageId
DocumentPreview->>DocumentPreview: request signed cover URL
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
scripts/backfill-document-covers.mjs (2)
40-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve
worker/pythonfrom the script location, not the working directory.
process.cwd()requires the operator to run the command from the repository root. Any other working directory produces an import failure forextract_pdf_assets. Derive the path fromimport.meta.urlinstead.♻️ Proposed refactor
Add the import:
import path from "node:path"; +import { fileURLToPath } from "node:url";Then resolve the directory relative to the script:
-sys.path.insert(0, ${JSON.stringify(path.join(process.cwd(), "worker/python"))})+sys.path.insert(0, ${JSON.stringify(+ path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..", "worker", "python"),+)})🤖 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 `@scripts/backfill-document-covers.mjs` at line 40, Update the Python path setup in the backfill script to resolve worker/python relative to the script’s import.meta.url instead of process.cwd(). Use the script-location URL/path conversion and preserve the existing sys.path insertion so extract_pdf_assets imports work from any working directory.
201-206: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winExit with a non-zero code when documents fail.
The script currently exits 0 even when every document fails. Any wrapper script or scheduled job then reports success. Set the exit code from the
failedcounter.♻️ Proposed refactor
if (!apply) { console.log("Re-run with --apply to write covers."); } + if (failed > 0) {+ process.exitCode = 1;+ } }🤖 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 `@scripts/backfill-document-covers.mjs` around lines 201 - 206, Update the completion flow in the backfill script after the summary logging to set the process exit code based on the failed counter, returning a non-zero code whenever failed is greater than zero while preserving the existing zero-success behavior.
🤖 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 `@scripts/backfill-document-covers.mjs`:
- Around line 181-189: Re-read the document’s current metadata immediately
before the update in the cover patch flow, then merge the new cover_image_id
into that refreshed object so concurrent metadata changes are preserved. In the
existing-cover skip path around the cover lookup, reuse the existing cover row’s
id when metadata.cover_image_id is absent, and patch the document metadata
instead of permanently skipping it.
- Around line 108-119: Move the document_images lookup and its coverError
handling into the per-document try block so a single lookup failure is recorded
as that document’s failure and processing continues. Preserve the existing skip
behavior for documents with an existing cover and ensure the run reaches the
final summary.
- Around line 49-70: Update the child-process promise around spawn in the
document-rendering helper to listen for the child’s error event and reject with
that error, preserving the caller’s per-document error handling. Add a render
timeout that terminates the spawned Python process and rejects when exceeded,
while clearing the timer on successful close or spawn failure so normal renders
and cleanup remain unaffected.
- Around line 37-46: Update the embedded Python script near
extractor.save_cover_page to set the extraction budget’s page count from the
opened document before rendering the cover. Keep using ExtractionBudget and
preserve the existing render and artifact-slot arguments; only ensure
set_page_count is applied so maxPages is enforced.
In `@src/components/clinical-dashboard/document-search-results.tsx`:
- Around line 802-833: Update the document page preview image flow around
showCover and coverLoaded so the skeleton remains visible until the current
signed URL has successfully loaded. Track which coverUrl has loaded, add image
error handling that invokes markFailed, and remove the useEffect reset of load
state; ensure stale URL state cannot reveal a transparent or failed image.
In `@worker/python/extract_pdf_assets.py`:
- Around line 1070-1075: Update the extraction flow around save_cover_page and
normal image extraction to process all searchable artifacts before attempting
the optional cover. Then call save_cover_page for the first page only with the
remaining budget, catch ExtractionBudgetExceeded, and skip the cover while
adding a warning; preserve successful cover insertion into images without
allowing cover budget usage to fail indexing.
In `@worker/python/test_extract_pdf_assets_cover.py`:
- Around line 32-33: Remove the no-op assertion in the test method containing
the clinical-crops comment, including the `or True` condition. If the fixture is
required to include a non-cover artifact, replace it with an explicit assertion
for that expected artifact; otherwise remove the assertion entirely.
---
Nitpick comments:
In `@scripts/backfill-document-covers.mjs`:
- Line 40: Update the Python path setup in the backfill script to resolve
worker/python relative to the script’s import.meta.url instead of process.cwd().
Use the script-location URL/path conversion and preserve the existing sys.path
insertion so extract_pdf_assets imports work from any working directory.
- Around line 201-206: Update the completion flow in the backfill script after
the summary logging to set the process exit code based on the failed counter,
returning a non-zero code whenever failed is greater than zero while preserving
the existing zero-success behavior.
🪄 Autofix
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: ea94a577-19bd-4694-8292-8c8ffe51610b
⛔ Files ignored due to path filters (3)
public/demo-documents/synthetic-clozapine-monitoring-cover.pngis excluded by!**/*.pngpublic/demo-documents/synthetic-lithium-monitoring-cover.pngis excluded by!**/*.pngpublic/demo-documents/synthetic-risk-flow-cover.pngis excluded by!**/*.png
📒 Files selected for processing (13)
scripts/backfill-document-covers.mjssrc/app/api/search/route.tssrc/components/clinical-dashboard/document-search-results.tsxsrc/lib/demo-data.tssrc/lib/document-enrichment.tssrc/lib/image-filtering.tssrc/lib/types.tstests/document-cover-thumbnails.test.tstests/document-enrichment-visual-counts.test.tstests/worker-visual-capture.test.tsworker/main.tsworker/python/extract_pdf_assets.pyworker/python/test_extract_pdf_assets_cover.py
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Track cover load state by comparing the loaded URL to the current signed URL, and drop the unused catch binding in the backfill script. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Keep document search cover ids on the existing document_images round trip so the offline answer-path budget stays at 13. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Apply set_page_count before cover render, harden spawn error/timeout handling, keep per-document failures from aborting the backfill, re-read metadata before patching cover_image_id, keep the skeleton until the signed URL loads with onError, extract the optional cover after searchable artifacts, and drop the no-op test assertion. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Keep cover thumbnails out of the demo document-viewer image list, regenerate the design-system adoption manifest for Skeleton usage, and select uncovered PDFs for backfill instead of re-scanning the most recently updated set. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Keep uncovered-candidate selection, CodeRabbit backfill hardening, cover preview skeleton/onError behaviour, and demo/adoption fixes. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Empty commit to re-fire pull_request synchronize for the current tip after stale/queued runs were cancelled during the Actions major outage. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Empty commit to fire pull_request synchronize after head had no Actions runs. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
The cover-page test pointed at public/demo-documents, which Dockerfile.worker does not COPY into the runner image. Mirror the page-edge crop pattern and keep the fixture under worker/python/fixtures. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Summary
cover_pagePNG thumbnails during PDF extraction (PyMuPDF) and persist them as non-searchable display artifacts.coverImageId/cover_image_idon document search matches and render real page-1 previews inDocumentPagePreview(placeholder fallback retained).scripts/backfill-document-covers.mjsto backfill existing live PDFs without a full reindex.document_imagesvisual enrichment query (no extra round trip / offline budget stays 13).RAG impact: no retrieval behaviour change — covers are
searchable: false, excluded from caption budget / chunk attachments /isClinicalImageEvidence, and are display-only for search cards.Verification
document-cover-thumbnails,document-enrichment-visual-counts,rag-round-trip-budget,demo-data— passedpython3 -m unittest test_extract_pdf_assets_cover— OK--apply(operator-approved): ~513 / 2065 indexed PDFs already have covers mid-runRisk and rollout
metadata.cover_image_idvia service role (scripts/backfill-document-covers.mjs --apply). Ingest path writes covers on future index jobs.Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
image_type=unclear+source_kind=cover_page(no DB CHECK migration required).--applyexplicitly.coverImageIdis present.Summary by CodeRabbit
New Features
Bug Fixes
Tests