feat: Add OCR text extraction and hybrid search#32
Conversation
- Fix database threading bug by adding Lock to cursor context manager
- Fix SSE events race condition with subscriber list lock
- Optimize OCR processing with batch text embeddings (5-10x faster)
- Add GET /screenshots/{id}/ocr endpoint for retrieving OCR text
- Add "View Text" button in web UI lightbox to display extracted text
- Add tests for OCR endpoint (4 new test cases)
- Update documentation with OCR architecture and search modes
- Add `downloaded` field to events/status endpoint for reliable model status when models are downloaded but not loaded (lazy loading) - Add logging to exception handlers in setup routes for debugging - Update frontend to use `downloaded` field as fallback for status - Fix OCR status showing "Not Available" when available - Add benchmark script for OCR and embedding performance testing
📝 WalkthroughWalkthroughAdds OCR extraction with pluggable providers, dual‑size text chunking, 384‑dim text embeddings, a hybrid image+text search pipeline, SSE event streaming, DB schema and API endpoints for OCR/status/migration, processor OCR migration/recompute, frontend search modes and OCR UI, tests, and supporting scripts/tools. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Processor
participant OCRService
participant Chunking
participant TextEmbedding
participant Database
Client->>API: POST /sync/start or POST /sync/ocr/recompute
API->>Processor: start_sync / recompute_ocr / start_ocr_migration
Processor->>Database: fetch pending screenshots
Processor->>OCRService: extract_text(image_path)
OCRService-->>Processor: OCRResult (text, confidence, word_count)
Processor->>Database: add_ocr_text(screenshot_id, ...)
Processor->>Chunking: chunk_ocr_text(full_text)
Chunking-->>Processor: small/large chunks
Processor->>TextEmbedding: get_batch_embeddings(chunks)
TextEmbedding-->>Processor: embeddings[]
Processor->>Database: add_ocr_chunk + add_text_embedding
Processor->>Database: mark_ocr_complete(screenshot_id)
Processor-->>API: emit_sync_progress (via events)
API-->>Client: SSE sync_progress events
sequenceDiagram
participant Client
participant API/Search
participant CLIP
participant TextEmbedding
participant Database
Client->>API/Search: POST /search {query, search_mode}
alt image mode
API/Search->>CLIP: get_image_embedding(query)
CLIP-->>API/Search: image_embedding
API/Search->>Database: search by image similarity
else text_fuzzy mode
API/Search->>Database: search_ocr_fts(query)
else text_semantic mode
API/Search->>TextEmbedding: get_query_embedding(query)
TextEmbedding-->>API/Search: text_embedding
API/Search->>Database: search_text_embeddings(text_embedding)
else auto/hybrid
API/Search->>CLIP: get_image_embedding(query)
CLIP-->>API/Search: image_embedding
API/Search->>TextEmbedding: get_query_embedding(query)
TextEmbedding-->>API/Search: text_embedding
API/Search->>Database: search_hybrid(image_embedding, text_embedding)
end
API/Search-->>Client: results
sequenceDiagram
participant Client
participant API/Events
participant EventHub
participant BackgroundTasks
Client->>API/Events: GET /events/stream
API/Events->>EventHub: subscribe client queue
API/Events->>Client: initial status events
BackgroundTasks->>EventHub: emit_model_download_progress / emit_sync_progress
EventHub-->>API/Events: deliver events to subscriber queues
API/Events-->>Client: SSE messages (model_status, model_download, sync_progress)
Note over API/Events: keepalive pings every 30s
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLAUDE.md (1)
61-64: Docs: API route list should include the new/eventsroutes.
Line 63 lists route prefixes but omits/events.
🤖 Fix all issues with AI agents
In @api/main.py:
- Line 210: The current setup sets uvicorn.access logger to WARNING via
logging.getLogger("uvicorn.access").setLevel(...) but also passes
log_level="warning" into uvicorn.run (symbol: uvicorn.run in main.py), which
sets the root/uvicorn logger level and may unintentionally suppress non-access
INFO logs; decide whether you want to silence only access logs or all uvicorn
logs and update accordingly—either remove or change log_level="warning" to
"info"/None to preserve uvicorn INFO diagnostics, or keep log_level="warning"
and instead explicitly set only "uvicorn.access" to WARNING (and optionally
adjust "uvicorn.error"/root via
logging.getLogger("uvicorn.error"/"uvicorn").setLevel(...)) so behavior matches
your intent (then run a quick dev start to confirm SyncProgress/SSE messages
still appear).
In @api/routes/search.py:
- Around line 100-134: The type error arises because image_embedding and
text_embedding may be None; before calling db.search_similar or
db.search_text_embeddings, explicitly assert or check the embedding is not None
(e.g., if image_embedding is None: raise ValueError("image_embedding required
for image search") or return empty results) to satisfy mypy and avoid runtime
None usage; apply the same pattern for text_embedding prior to
db.search_text_embeddings and keep references to search_mode, db.search_similar,
and db.search_text_embeddings so the checks are colocated with the existing
branches.
In @api/routes/setup.py:
- Around line 202-245: The sync generator download_stream blocks the event loop
by doing synchronous imports and calls (e.g.,
core.embeddings.get_text_embedding, core.text_embeddings.get_text_embedding and
the is_downloaded checks); change download_stream to an async def async
generator and offload blocking operations to the threadpool with
asyncio.get_running_loop().run_in_executor (wrap calls to clip_is_downloaded/
bge_is_downloaded, the import-triggered model initialization and the test
get_text_embedding/get_bge_embedding calls) so you yield SSE events from the
async generator while awaiting run_in_executor for those heavy operations; keep
the same event payloads and return the same StreamingResponse using the async
generator.
In @api/schemas.py:
- Around line 173-176: Change the search_mode field in SearchRequest from a
plain str to the SearchMode enum to enable validation and IDE support: update
the annotation on search_mode to use SearchMode instead of str and keep the
Field default matching the enum value; if SearchMode is declared after
SearchRequest, move the SearchMode enum definition before SearchRequest or use a
forward reference (e.g., a string type hint) so the pydantic model resolves the
enum correctly while preserving the Field description and default.
In @api/tests/test_api.py:
- Around line 678-713: Update the test_search_text_fuzzy_mode mock to match the
real DB call: replace mock_db.search_text_fts.return_value with
mock_db.search_ocr_fts.return_value and return the same list; this aligns the
test with api.routes.search where db.search_ocr_fts() is invoked when
search_mode == "text_fuzzy" so the patched mock_db (from
@patch("api.routes.search.db")) intercepts the call correctly.
In @CLAUDE.md:
- Around line 48-55: Update the documented image embedding dimension in
CLAUDE.md from "512-dim vectors" to "768-dim vectors" to match the code's
EMBEDDING_DIM = 768 (CLIP ViT-L-14); specifically replace the phrase at the
screenshot_embeddings description (around the first occurrence) and the second
occurrence later in the file, leaving the "384-dim" text embedding references
for BGE-small-en-v1.5 unchanged.
In @core/chunking.py:
- Around line 98-108: The character tracking drifts because paragraphs =
re.split(r"\n\n+", text) loses exact separator lengths while char_position is
incremented by a fixed 2; update the loop that builds chunks (variables:
paragraphs, current_chunk, current_start, char_position, chunk_index) to compute
start_char/end_char from the original text instead of assuming two
newlines—e.g., locate each paragraph's start in the original text using a
running search offset (or iterate with re.finditer to capture separators and
their exact lengths) and advance char_position by the actual separator length so
start_char/end_char reflect true positions; apply the same fix to the other
occurrence around line ~162.
In @core/database.py:
- Around line 980-981: The f-string interpolation for chunk_condition is
vulnerable to SQL injection; change chunk_condition to use a parameter
placeholder (e.g., "AND c.chunk_size = %s" or the DB-specific param style)
instead of embedding chunk_size, add chunk_size to an extra_params list/tuple,
and pass that combined params tuple into the DB execute call (where the main
query is executed) so the query is fully parameterized; also optionally validate
chunk_size against allowed values ("small","large") before appending.
In @core/embeddings.py:
- Around line 134-147: Update the is_downloaded() function in core/embeddings.py
to mirror the fallback pattern used in core/text_embeddings.py: when
try_to_load_from_cache(model_id, "config_sentence_transformers.json") returns
None, call try_to_load_from_cache again with "config.json" before concluding the
model is not present. Keep the same model_id
("sentence-transformers/clip-ViT-L-14") and preserve the try/except behavior,
returning True if either config file is found and False only if both checks fail
or an exception occurs.
In @core/ocr.py:
- Around line 131-136: The nested conditional in core/ocr.py checks platform and
provider separately; simplify by combining conditions with logical and to avoid
nested ifs — replace the two blocks that use `if system == "Darwin": if
"apple_vision" in self._providers:` and `elif system == "Windows": if
"tesseract" in self._providers:` with single-line combined checks using `and`
(e.g., `if system == "Darwin" and "apple_vision" in self._providers:` and `elif
system == "Windows" and "tesseract" in self._providers:`) so the method that
selects the provider (the code surrounding the `system` variable and
`self._providers`) returns appropriately without nested ifs.
In @core/text_embeddings.py:
- Around line 39-45: The linter flags the late imports of numpy and torch
(imports "import numpy as np" and "import torch") due to intentional environment
setup above; add "# noqa: E402" at the end of each of those import lines to
suppress the E402 warning so the environment variable/tokens setup can remain
before imports.
In @scripts/benchmark_ocr.py:
- Around line 366-372: The parser in main() currently hardcodes a
developer-specific default for the "--dir" argument
(parser.add_argument("--dir", ...)), so change it to a platform-agnostic
approach: either remove the hardcoded path and make "--dir" required, or set a
generic default (e.g., current working directory or
os.path.expanduser("~/screenshots")) and document it; update the
parser.add_argument call accordingly so the default is not tied to a specific
user's home path.
- Around line 351-360: The code can raise StopIteration or reference an
undefined avg_chunks when chunking is skipped; update the logic around
emb_result, results and avg_chunks so avg_chunks is safely determined with a
default without using next(...) that can raise, e.g. compute avg_chunks = 6 (or
another sensible default) before the conditional and then try to extract it from
results using a safe lookup (e.g. a loop or .get on dicts) and only overwrite
the default if found; ensure emb_time calculation (emb_time = emb_time *
avg_chunks) and the final print statement always use this defined avg_chunks
value so no exception occurs when chunking results are missing or errored.
In @scripts/cleanup_ocr.py:
- Around line 10-18: The top-level import from core.database (db) is causing
E402; move that import into the main() function to avoid top-level path mutation
issues and add an explicit return type hint to main (def main() -> None:). Keep
the sys.path insertion if needed for standalone execution, but ensure any
project imports like "from core.database import db" occur inside main(), and
update any other duplicate top-level imports (referenced around line 20) the
same way.
In @web/src/app/setup/page.tsx:
- Around line 225-228: The migrationProgress calculation can produce NaN when
eventStatus?.ocr_stats.completed + eventStatus?.ocr_stats.pending === 0; update
the logic around migrationProgress to check the sum (e.g., const total =
eventStatus?.ocr_stats?.completed + eventStatus?.ocr_stats?.pending) and only
compute the division when total > 0, otherwise fall back to
status?.migration_status?.progress_percent ?? 0 (or 0); adjust references to
eventStatus, ocr_stats, and migrationProgress accordingly.
- Around line 93-119: The determineStep callback computes textReady but never
uses it, so include it in the readiness check: update the early-if that
currently uses clipReady and downloading flags to also bail to 'models' when
!textReady (and still consider eventStatus?.text_embedding?.downloading); locate
determineStep and variables clipReady/textReady/eventStatus/status and change
the condition to require both clipReady and textReady before proceeding
(returning 'models' otherwise).
🧹 Nitpick comments (24)
pyproject.toml (1)
30-35: Consider moving OCR dependencies to optional extras.
While the code already includes robust availability checking (is_available()verifies both the pytesseract package and the tesseract binary at runtime), keeping OCR as lazy-loaded is good. However,pytesseractis still declared in main dependencies rather than an optional extra, which means it's installed even for users who don't need OCR. Moving it to an optional group (e.g.,ocr = ["pytesseract>=0.3.10", ...]) would reduce the base install footprint while maintaining lazy loading for those who opt in. Note thatocrmacis already properly platform-gated.core/chunking.py (1)
19-34: Duplicate type definitions with api/schemas.py.
ChunkInfoandChunkedTextare defined here as dataclasses and also inapi/schemas.py(lines 511-525) as Pydantic models. Consider importing from a single source to avoid drift between definitions.♻️ Suggested approach
Either:
- Use the Pydantic models from
api/schemas.pythroughout (convert to dict when needed)- Define the dataclasses here and have
api/schemas.pyuse them or mirror them explicitlyThis ensures consistency when the structures evolve.
main.py (1)
54-60: Consider making log level configurable.Setting
log_level="warning"suppresses access logs, which is reasonable for production but may hinder debugging. Consider adding a--verboseflag or environment variable to control this.♻️ Optional enhancement
parser.add_argument("--reload", action="store_true", help="Enable auto-reload for development") + parser.add_argument("--verbose", action="store_true", help="Enable verbose logging") ... uvicorn.run( app, host=args.host, port=args.port, reload=args.reload if not is_frozen() else False, - log_level="warning", # Suppress verbose access logs + log_level="info" if args.verbose else "warning", )api/routes/search.py (1)
60-61: Consider validating search_mode against allowed values.The code calls
.lower()onsearch_modebut doesn't validate it's one of the allowed values ("auto", "image", "text_fuzzy", "text_semantic"). An invalid mode would fall through to the hybrid search case silently.💡 Optional: Add validation for search_mode
search_mode = request.search_mode.lower() + valid_modes = ("auto", "image", "text_fuzzy", "text_semantic") + if search_mode not in valid_modes: + raise HTTPException( + status_code=400, + detail=f"Invalid search_mode. Must be one of: {', '.join(valid_modes)}", + )Alternatively, if
SearchRequest.search_modeis already an enum type, this validation may be handled at the schema level.api/routes/setup.py (1)
142-188: Consider adding response_model for type safety and API documentation.Per coding guidelines, FastAPI route handlers should specify
response_model. The/modelsendpoint returns a dict but lacks a defined response model.💡 Suggested fix
Define a response model in
api/schemas.py:class ModelsStatus(BaseModel): clip: str text_embedding: str ocr: str all_ready: boolThen update the endpoint:
-@router.get("/models") +@router.get("/models", response_model=ModelsStatus) async def get_model_status():core/ocr_providers/apple_vision.py (2)
103-106: Consider using logging instead of print for error handling.Using
print()for error output makes it harder to control log levels and integrate with centralized logging. Consider using Python'sloggingmodule.💡 Suggested change
+import logging + +logger = logging.getLogger(__name__) + # ... in extract_text method: except Exception as e: - print(f"Apple Vision OCR error: {e}") + logger.warning(f"Apple Vision OCR error: {e}") # Return empty result on error rather than raising return OCRResult.empty()
120-131: Redundant import:platformis already imported at module level.The module already imports
platformat line 18. The re-import asplatis unnecessary.💡 Suggested fix
def get_model_info(self) -> dict: """Get information about the Vision framework""" - import platform as plat - return { "engine": "Apple Vision Framework", "platform": "macOS", - "macos_version": plat.mac_ver()[0] if plat.system() == "Darwin" else None, + "macos_version": platform.mac_ver()[0] if platform.system() == "Darwin" else None, "requires_download": False, "memory_overhead": "minimal (uses system APIs)", "supported_languages": ["en", "zh", "ja", "ko", "de", "fr", "it", "pt", "es"], }core/ocr_providers/tesseract.py (2)
109-111: Error handling swallows exceptions silently.Catching all exceptions and returning an empty result could mask underlying issues (e.g., file permission errors, corrupted images). Consider logging at a higher level or re-raising critical exceptions.
Additionally, using
print()for error logging is not ideal for production code. Consider using theloggingmodule for consistent error handling.Suggested improvement
+import logging + +logger = logging.getLogger(__name__) + # In extract_text method: except Exception as e: - print(f"Tesseract OCR error: {e}") + logger.warning("Tesseract OCR error for %s: %s", image_path, e) return OCRResult.empty()
130-138: Bareexceptclause hides specific errors.The bare
exceptat line 137 catches all exceptions includingKeyboardInterruptandSystemExit. Consider catching specific exceptions likesubprocess.TimeoutExpired,subprocess.SubprocessError, orOSError.More specific exception handling
if tesseract_path: try: import subprocess result = subprocess.run([tesseract_path, "--version"], capture_output=True, text=True, timeout=5) # First line contains version version = result.stdout.split("\n")[0] if result.stdout else None - except Exception: + except (subprocess.TimeoutExpired, subprocess.SubprocessError, OSError): passweb/src/app/page.tsx (1)
945-950: Consider adding user feedback for clipboard copy.The
copyOCRTextfunction copies text but provides no visual confirmation to the user. Consider adding a toast notification or temporarily changing the button text.Example with button state feedback
const [copied, setCopied] = useState(false); const copyOCRText = useCallback(() => { if (ocrData?.text) { navigator.clipboard.writeText(ocrData.text); setCopied(true); setTimeout(() => setCopied(false), 2000); } }, [ocrData]); // In the button: // {copied ? 'Copied!' : 'Copy to Clipboard'}web/src/app/setup/page.tsx (1)
168-187: Model download trigger lacks error handling beyond console.error.The fetch call at line 178 handles errors with
.catch(console.error), but if the download endpoint fails, thedownloadTriggeredflag remains true, preventing retry attempts. Consider resetting the flag on error or showing user feedback.Suggested improvement
if (needsClip || needsBge) { setDownloadTriggered(true); // Trigger download endpoint - this will download models fetch('/api/v1/setup/download-models', { method: 'POST' }) .then(async (response) => { + if (!response.ok) { + console.error('Model download failed:', response.status); + setDownloadTriggered(false); // Allow retry + } else { + console.log('Model download triggered'); + } - console.log('Model download triggered'); }) - .catch(console.error); + .catch((err) => { + console.error('Model download error:', err); + setDownloadTriggered(false); // Allow retry + }); }api/routes/events.py (3)
159-161: Missingresponse_modelon status endpoint.Per coding guidelines, FastAPI route handlers should specify
response_model. The/events/statusendpoint returns a complex structure that should have a defined schema.Suggested improvement
# In api/schemas.py, add: class EventsStatus(BaseModel): clip: dict text_embedding: dict ocr: dict sync: dict ocr_stats: dict # Then update the endpoint: @router.get("/status", response_model=EventsStatus) async def get_all_status(): ...
41-43: Events are silently dropped when queue is full.When a subscriber's queue is full (100 items), new events are silently dropped via
contextlib.suppress. This could cause clients to miss important updates. Consider logging when events are dropped for debugging purposes.Add logging for dropped events
+import logging + +logger = logging.getLogger(__name__) + for queue in subscribers: - with contextlib.suppress(asyncio.QueueFull): - queue.put_nowait(event_data) + try: + queue.put_nowait(event_data) + except asyncio.QueueFull: + logger.debug("Event queue full, dropping event: %s", event_type)
183-184: Use logging module instead of print() for error messages.Multiple places in this file use
print()for error logging (lines 184, 204, 216, 242, 253). Using theloggingmodule provides better control over log levels and output formatting.Replace print with logging
+import logging + +logger = logging.getLogger(__name__) + except Exception as e: - print(f"Error getting CLIP status: {e}") + logger.warning("Error getting CLIP status: %s", e)Apply the same pattern to all
print(f"Error ...")statements in this file.api/routes/sync.py (2)
280-287: Consider using FastAPI'sBackgroundTasksinstead of manual threading.The
recompute_ocrendpoint manually creates a daemon thread for background processing. FastAPI providesBackgroundTasksfor this purpose, which integrates better with the framework's lifecycle. The current approach works but is inconsistent with howstart_synccould usebackground_tasks.Using BackgroundTasks
@router.post("/ocr/recompute", response_model=SyncStartResponse) -async def recompute_ocr(): +async def recompute_ocr(background_tasks: BackgroundTasks): """Recompute OCR for all screenshots ... """ if processor_service.is_running: raise HTTPException( status_code=409, detail="Cannot recompute while sync is running", ) if not config.ocr.enabled: raise HTTPException( status_code=400, detail="OCR is disabled. Enable it in settings first.", ) total = db.get_screenshot_count() - # Start recompute in background - import threading - - def run_recompute(): - processor_service.recompute_ocr() - - thread = threading.Thread(target=run_recompute, daemon=True) - thread.start() + background_tasks.add_task(processor_service.recompute_ocr) return SyncStartResponse( success=True, message=f"OCR recompute started for {total} screenshots", unsynced_count=total, )
320-321: Bareexceptclauses hide specific import or service errors.Lines 320 and 330 use bare
exceptwhich catches all exceptions includingSystemExit. Consider catchingImportErrororExceptionwith logging for better debuggability.More specific exception handling
try: from core.text_embeddings import text_embedding_service text_status = text_embedding_service.get_model_status() text_emb = ModelStatus( loaded=text_status["loaded"], device=text_status["device"], idle_seconds=text_status["idle_seconds"], auto_unload_seconds=text_status["auto_unload_seconds"], ) - except Exception: - pass + except (ImportError, Exception) as e: + logger.debug("Text embedding service not available: %s", e)scripts/benchmark_ocr.py (1)
265-267: Accessing private attribute_current_model_nameis fragile.Accessing
text_embedding_service._current_model_namerelies on an internal implementation detail that could change. Consider adding a public method to the service or using a safer fallback.Safer access pattern
print( - f" Model: {text_embedding_service._current_model_name if hasattr(text_embedding_service, '_current_model_name') else 'BGE'}" + f" Model: {getattr(text_embedding_service, 'model_name', None) or 'BGE'}" )Or add a public
get_model_name()method to the text embedding service.core/ocr.py (3)
75-77: Consider adding more specific return type annotation.The return type
dictcould be more specific (e.g.,dict[str, Any]) for better type checking. This applies to other methods likeget_current_provider_infoas well.
88-92: Unused class and instance attributes.
_instance(line 89) and_current_provider(line 92) are defined but never used anywhere in the class. Consider removing them to avoid confusion.♻️ Proposed fix
class OCRService: """ OCR service with pluggable providers. Supports automatic provider selection based on platform and config. Providers are registered and can be switched at runtime. """ _providers: dict[str, type[OCRProvider]] = {} - _instance: OCRProvider | None = None - - def __init__(self): - self._current_provider: OCRProvider | None = None
192-209: Silent failure when providers fail to import.When
ImportErroris raised during provider registration, it's silently suppressed. Consider logging a debug message to aid troubleshooting when expected providers aren't available.♻️ Proposed enhancement
# Import and register Apple Vision provider (macOS only) if platform.system() == "Darwin": try: from core.ocr_providers.apple_vision import AppleVisionOCR OCRService.register("apple_vision", AppleVisionOCR) - except ImportError: - pass # ocrmac not installed + except ImportError as e: + print(f"Apple Vision OCR not available: {e}") # Import and register Tesseract provider (cross-platform) try: from core.ocr_providers.tesseract import TesseractOCR OCRService.register("tesseract", TesseractOCR) - except ImportError: - pass # pytesseract not installed + except ImportError as e: + print(f"Tesseract OCR not available: {e}")core/processor.py (3)
24-28: Simplify empty exception class.The
passstatement after a docstring is redundant; the docstring alone satisfies the class body requirement.♻️ Proposed fix
class OCRProcessingError(Exception): """Raised when OCR processing fails for expected reasons (corrupted image, etc.)""" - - pass
472-494: Unusedoffsetvariable.The
offsetvariable is incremented but never used in the loop logic.get_screenshots_without_ocralready returns only unprocessed screenshots, so offset-based pagination isn't needed.♻️ Proposed fix
# Process all screenshots - offset = 0 batch_size = 10 while not self._cancel_requested: screenshots = db.get_screenshots_without_ocr(limit=batch_size) if not screenshots: break for screenshot in screenshots: if self._cancel_requested: break try: self._process_ocr_for_screenshot(screenshot["id"], screenshot["image_path"]) self._progress.processed += 1 print(f" ✓ {self._progress.processed}/{self._progress.total}") except Exception as e: print(f" ✗ Error: {e}") self._progress.errors += 1 if self._on_progress: self._on_progress(self._progress) - - offset += batch_size
483-489: Inconsistent error handling compared to other loops.In
recompute_ocr, exceptions are caught broadly with a singleexcept Exception, while_process_loopand_process_pending_ocrhave granular handling forOCRProcessingError,TypeError,AttributeError,KeyError,FileNotFoundError, andOSError. Consider applying the same granular error handling here for consistency and to avoid marking code bugs as complete.core/database.py (1)
1299-1319: Consider wrappingreset_all_ocrin a transaction explicitly.While the cursor context manager handles transactions, this operation deletes from multiple tables. If an error occurs mid-way (e.g., after deleting embeddings but before resetting flags), the database could be left in an inconsistent state. The current implementation should be safe due to the context manager's rollback, but consider adding a comment noting transaction semantics for clarity.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
CLAUDE.mdREADME.mdapi/main.pyapi/routes/events.pyapi/routes/screenshots.pyapi/routes/search.pyapi/routes/setup.pyapi/routes/sync.pyapi/schemas.pyapi/tests/test_api.pycore/chunking.pycore/config.pycore/database.pycore/embeddings.pycore/ocr.pycore/ocr_providers/__init__.pycore/ocr_providers/apple_vision.pycore/ocr_providers/tesseract.pycore/processor.pycore/text_embeddings.pymain.pypyproject.tomlscripts/benchmark_ocr.pyscripts/cleanup_ocr.pytray/app.pyweb/src/app/page.tsxweb/src/app/setup/page.tsxweb/src/lib/api.tsweb/src/types/index.ts
🧰 Additional context used
📓 Path-based instructions (9)
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web/src/**/*.{ts,tsx}: Use functional components with hooks in React code
Use Tailwind CSS for styling in React components
Files:
web/src/types/index.tsweb/src/lib/api.tsweb/src/app/setup/page.tsxweb/src/app/page.tsx
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use type hints for function parameters and return types
Use double quotes for strings in Python code
Use private attributes with_leading_underscorenaming convention
Files:
api/routes/screenshots.pymain.pycore/ocr_providers/__init__.pycore/config.pyscripts/benchmark_ocr.pyapi/routes/setup.pycore/chunking.pyapi/main.pyapi/routes/sync.pyapi/routes/events.pycore/ocr_providers/tesseract.pycore/ocr_providers/apple_vision.pycore/processor.pycore/database.pyapi/routes/search.pycore/text_embeddings.pycore/embeddings.pycore/ocr.pyapi/tests/test_api.pytray/app.pyscripts/cleanup_ocr.pyapi/schemas.py
api/routes/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
api/routes/**/*.py: FastAPI routes should use APIRouter with prefix and tags parameters
FastAPI route handlers should include docstrings and specify response_model
Use async/await syntax for FastAPI route handlers
Files:
api/routes/screenshots.pyapi/routes/setup.pyapi/routes/sync.pyapi/routes/events.pyapi/routes/search.py
core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Services should use global singleton instances at module level
Files:
core/ocr_providers/__init__.pycore/config.pycore/chunking.pycore/ocr_providers/tesseract.pycore/ocr_providers/apple_vision.pycore/processor.pycore/database.pycore/text_embeddings.pycore/embeddings.pycore/ocr.py
api/main.py
📄 CodeRabbit inference engine (CLAUDE.md)
FastAPI app should serve REST API and static Next.js build from web/out/
Files:
api/main.py
core/processor.py
📄 CodeRabbit inference engine (CLAUDE.md)
Background sync service for processing screenshots should be implemented in core/processor.py
Files:
core/processor.py
core/database.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use SQLite with sqlite-vec for vector similarity search in database operations
Files:
core/database.py
core/embeddings.py
📄 CodeRabbit inference engine (CLAUDE.md)
CLIP model should implement lazy loading with 5-minute auto-unload timeout
Files:
core/embeddings.py
tray/app.py
📄 CodeRabbit inference engine (CLAUDE.md)
TrayApp should manage system tray menu and spawn backend via subprocess
Files:
tray/app.py
🧠 Learnings (4)
📚 Learning: 2026-01-11T06:00:24.323Z
Learnt from: CR
Repo: VedankPurohit/LiveRecall PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T06:00:24.323Z
Learning: Applies to core/processor.py : Background sync service for processing screenshots should be implemented in core/processor.py
Applied to files:
CLAUDE.mdapi/main.pyapi/routes/sync.pycore/processor.py
📚 Learning: 2026-01-11T06:00:24.323Z
Learnt from: CR
Repo: VedankPurohit/LiveRecall PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T06:00:24.323Z
Learning: Applies to api/main.py : FastAPI app should serve REST API and static Next.js build from web/out/
Applied to files:
api/main.py
📚 Learning: 2026-01-11T06:00:24.323Z
Learnt from: CR
Repo: VedankPurohit/LiveRecall PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T06:00:24.323Z
Learning: Applies to core/database.py : Use SQLite with sqlite-vec for vector similarity search in database operations
Applied to files:
core/database.py
📚 Learning: 2026-01-11T06:00:24.323Z
Learnt from: CR
Repo: VedankPurohit/LiveRecall PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T06:00:24.323Z
Learning: Applies to core/embeddings.py : CLIP model should implement lazy loading with 5-minute auto-unload timeout
Applied to files:
core/text_embeddings.pycore/embeddings.py
🧬 Code graph analysis (21)
web/src/types/index.ts (2)
api/schemas.py (1)
SearchMode(543-549)web/src/app/settings/page.tsx (1)
mode(214-216)
api/routes/screenshots.py (2)
api/schemas.py (1)
ScreenshotOCRResponse(298-314)core/database.py (2)
get_screenshot(285-290)get_ocr_text(782-787)
scripts/benchmark_ocr.py (4)
core/embeddings.py (1)
get_image_embedding(180-193)core/config.py (1)
save(233-274)core/ocr.py (1)
get_provider_name(179-185)core/chunking.py (1)
chunk_ocr_text(247-270)
web/src/lib/api.ts (2)
api/schemas.py (2)
VisibilityFilter(47-52)SearchMode(543-549)web/src/types/index.ts (2)
VisibilityFilter(17-17)SearchMode(143-143)
core/chunking.py (1)
api/schemas.py (2)
ChunkInfo(512-518)ChunkedText(521-525)
web/src/app/setup/page.tsx (2)
api/schemas.py (3)
SetupStatus(468-475)EnhancedSetupStatus(744-766)MigrationStatus(718-741)core/processor.py (1)
SyncProgress(31-63)
api/main.py (2)
core/database.py (1)
get_ocr_pending_count(853-857)core/processor.py (1)
start_ocr_migration(139-158)
api/routes/sync.py (5)
api/schemas.py (4)
AllModelsStatus(626-637)EnhancedSyncStatus(645-676)ModelStatus(378-384)SyncStatus(93-105)web/src/types/index.ts (2)
ModelStatus(33-38)SyncStatus(109-115)core/processor.py (4)
progress(125-126)SyncProgress(31-63)is_running(121-122)percent(60-63)core/database.py (2)
get_unsynced_count(578-582)get_ocr_pending_count(853-857)api/routes/setup.py (1)
get_model_status(143-188)
core/ocr_providers/tesseract.py (1)
core/ocr.py (2)
OCRResult(42-53)empty(51-53)
core/ocr_providers/apple_vision.py (2)
api/schemas.py (1)
OCRResult(503-509)core/ocr.py (2)
OCRResult(42-53)empty(51-53)
core/processor.py (4)
core/embeddings.py (1)
get_image_embedding(180-193)core/database.py (10)
add_embedding(590-605)get_unsynced_count(578-582)get_unsynced_screenshots(569-576)mark_ocr_complete(835-839)add_ocr_text(750-780)add_ocr_chunk(789-819)add_text_embedding(821-833)get_screenshots_without_ocr(841-851)reset_all_ocr(1299-1319)get_screenshot_count(584-588)core/chunking.py (1)
chunk_ocr_text(247-270)core/text_embeddings.py (2)
get_batch_embeddings(269-299)get_batch_embeddings(336-338)
core/database.py (6)
tray/api_client.py (1)
close(43-46)core/processor.py (1)
start(128-137)tray/app.py (1)
start(150-193)core/capture.py (1)
start(34-43)core/compression.py (1)
start(52-75)tray/backend.py (1)
start(66-116)
api/routes/search.py (2)
core/embeddings.py (2)
get_combined_embedding(211-244)get_text_embedding(196-208)core/database.py (4)
search_similar(607-675)search_ocr_fts(859-906)search_text_embeddings(957-1034)search_hybrid(1036-1192)
core/text_embeddings.py (1)
core/embeddings.py (12)
_get_device(31-37)_load_model(40-69)_schedule_auto_unload(72-83)_check_and_unload(86-96)unload_model(99-126)is_loaded(129-131)is_downloaded(134-146)get_model_status(149-158)set_auto_unload_timeout(161-166)_normalize(169-177)get_text_embedding(196-208)cosine_similarity(247-254)
web/src/app/page.tsx (3)
web/src/lib/api.ts (3)
ScreenshotOCR(193-198)search(74-97)getScreenshotOCR(200-202)api/schemas.py (2)
SearchMode(543-549)Screenshot(234-254)web/src/types/index.ts (2)
SearchMode(143-143)Screenshot(3-14)
core/embeddings.py (2)
core/text_embeddings.py (4)
is_downloaded(169-182)is_downloaded(352-354)get_model_status(185-196)get_model_status(340-342)api/routes/setup.py (1)
get_model_status(143-188)
core/ocr.py (3)
api/schemas.py (1)
OCRResult(503-509)core/ocr_providers/apple_vision.py (4)
extract_text(55-106)is_available(108-118)get_model_info(120-131)AppleVisionOCR(26-131)core/ocr_providers/tesseract.py (4)
extract_text(58-111)is_available(113-123)get_model_info(125-147)TesseractOCR(31-156)
api/tests/test_api.py (4)
core/database.py (6)
get_ocr_stats(1241-1266)get_stats(1323-1343)search_similar(607-675)search_hybrid(1036-1192)get_screenshot(285-290)get_ocr_text(782-787)web/src/lib/api.ts (1)
search(74-97)core/text_embeddings.py (2)
get_query_embedding(241-266)get_query_embedding(332-334)api/routes/screenshots.py (1)
get_screenshot(264-271)
tray/app.py (2)
api/tests/test_api.py (1)
api_client(12-27)tray/api_client.py (1)
get_json(138-146)
scripts/cleanup_ocr.py (1)
core/database.py (4)
connect(42-53)cursor(62-75)reset_all_ocr(1299-1319)disconnect(55-59)
api/schemas.py (2)
core/ocr.py (1)
OCRResult(42-53)web/src/types/index.ts (2)
SearchMode(143-143)ModelStatus(33-38)
🪛 GitHub Actions: CI
api/routes/search.py
[error] 103-103: Mypy type error: Argument 1 to 'search_similar' of 'Database' has incompatible type 'list[float] | None'; expected 'list[float]'. (Command: uv run mypy core api tray)
[error] 127-127: Mypy type error: Argument 'query_embedding' to 'search_text_embeddings' of 'Database' has incompatible type 'list[float] | None'; expected 'list[float]'. (Command: uv run mypy core api tray)
core/text_embeddings.py
[error] 44-44: E402 Module level import not at top of file.
[error] 45-45: E402 Module level import not at top of file.
core/ocr.py
[warning] 134-134: SIM102 Use a single if statement instead of nested if statements. Combine if statements using and.
scripts/cleanup_ocr.py
[error] 17-17: E402 Module level import not at top of file.
🔇 Additional comments (33)
api/main.py (1)
16-16: SSE events router wiring looks correct (import + include_router).
Good to see it registered under/api/v1; placement is also before SPA catch-all.Also applies to: 136-136
core/embeddings.py (1)
149-158:downloadedinget_model_status()is a good addition for setup UX.
This aligns with howapi/routes/setup.pytreats “ready” as loaded-or-downloaded.README.md (1)
8-9: LGTM! Well-documented feature additions.The new feature documentation for OCR Text Extraction, Hybrid Search, Search Modes, and OCR Processing is clear and comprehensive. The documentation accurately reflects the new capabilities added in this PR.
Also applies to: 131-148
core/chunking.py (1)
50-52: LGTM! Core chunking logic is well-implemented.The dual-size chunking approach with paragraph and sentence boundary awareness is solid. The
chunk_ocr_textfunction provides a clean API for creating both small and large chunks.Also applies to: 247-270
api/schemas.py (1)
503-509: LGTM! Well-structured schemas with comprehensive documentation.The new OCR, search, and status schemas are well-designed with proper
Fielddescriptions and examples.AllModelsStatusprovides a unified view of all model states.Also applies to: 626-637
core/text_embeddings.py (3)
72-106: LGTM! Lazy loading with 5-minute auto-unload correctly implemented.The model loading follows the established pattern from
core/embeddings.pywith proper thread safety, device fallback, and the 5-minute auto-unload timeout as specified in the project learnings.Also applies to: 109-134
241-266: Good use of BGE query instruction prefix.The instruction prefix
"Represent this sentence for searching relevant passages: "is the correct approach for BGE models to improve retrieval performance. Document embeddings correctly omit this prefix.
321-358: LGTM! Service wrapper follows project conventions.The
TextEmbeddingServiceclass andtext_embedding_servicesingleton follow the coding guidelines for core services. The class-based wrapper provides a consistent interface.web/src/types/index.ts (1)
142-155: LGTM! TypeScript types align with backend schemas.The
SearchModetype correctly mirrors the backendSearchModeenum values, and the optionalsearch_modeproperty inSearchParamsprovides proper type safety for search requests.core/ocr_providers/__init__.py (1)
1-20: LGTM! Well-documented package initialization.The package documentation clearly explains the provider architecture and how to add new OCR providers. The conditional import pattern in
core/ocr.pyis appropriately referenced.tray/app.py (1)
116-146: LGTM! Clean implementation of enhanced setup status handling.The multi-branch logic correctly prioritizes setup triggers: version change first, then CLIP model availability, then OCR migration. The early return on missing status prevents null reference errors, and logging the reason provides good observability.
api/routes/screenshots.py (1)
326-369: LGTM! Well-structured OCR endpoint with proper error handling.The endpoint follows FastAPI best practices with async handler, docstring, and response_model. The three-tier response logic (screenshot not found, OCR not processed, OCR data available) provides clear API semantics. The graceful fallback for missing OCR data (lines 355-362) is a good defensive pattern.
core/ocr_providers/apple_vision.py (1)
55-106: LGTM! Robust text extraction with proper error handling.The
extract_textmethod correctly handles edge cases: missing files raiseFileNotFoundError, empty results returnOCRResult.empty(), and exceptions are caught gracefully. The tuple unpacking at line 86 safely handles variable-length results from ocrmac.web/src/lib/api.ts (2)
74-97: LGTM! Clean integration of search mode parameter.The
searchModeparameter is properly typed withSearchModeand defaults to'auto', matching the backend's default behavior. The parameter is correctly serialized assearch_modein the request body to match the API's snake_case convention.
193-202: LGTM! OCR interface and function are well-defined.The
ScreenshotOCRinterface correctly mirrors the backend'sScreenshotOCRResponseschema. ThegetScreenshotOCRfunction follows the established pattern in this file.core/config.py (3)
64-82: LGTM! Well-documented OCR settings with helpful provider guidance.The docstring clearly documents available and future provider options, including the recompute requirement when switching providers. The defaults (enabled=True, provider="auto") are sensible for most users.
85-119: LGTM! Comprehensive embedding and chunking configuration.The
TextEmbeddingSettingsdocstring clearly warns about dimension compatibility requirements when changing models. TheChunkingSettingsdual-size strategy (512/2048 tokens) aligns with the PR's RRF-based hybrid search approach.
253-266: LGTM! Config persistence correctly extended for new settings.Save and load methods maintain symmetry for the new OCR, text_embedding, and chunking sections. The
.get()pattern with defaults ensures backward compatibility with existing config files that lack these new sections.Also applies to: 308-326
core/ocr_providers/tesseract.py (1)
31-56: LGTM! Well-structured provider with lazy loading.The lazy loading pattern for
pytesseractis appropriate to avoid import errors when the package isn't installed. The class follows the provider interface pattern used by the OCR service.web/src/app/page.tsx (2)
929-950: LGTM! OCR text viewing callbacks are well-implemented.The
viewOCRTextcallback properly manages loading state, error handling, and provides a sensible fallback when OCR fetch fails. ThecopyOCRTextuses the Clipboard API correctly.
1302-1313: LGTM! Search mode selector is well-integrated.The search mode selector is properly styled with Tailwind CSS and the state is correctly passed through to all
search()invocations. The UI options map well to the backendSearchModeenum.api/routes/sync.py (1)
206-209: LGTM! Enhanced sync status endpoint is well-structured.The endpoint properly converts internal progress to the enhanced API schema with OCR tracking details.
api/tests/test_api.py (2)
756-870: LGTM! Comprehensive OCR endpoint test coverage.The tests cover the key scenarios: successful OCR retrieval, unprocessed screenshots, non-existent screenshots (404), and screenshots with OCR done but no text extracted. This provides good confidence in the endpoint behavior.
573-601: Verify mock patch path matches the actual import in the route file.The test patches
core.database.db, but ifdbis imported at the module level inapi/routes/setup.py, the mock path should instead beapi.routes.setup.dbto correctly intercept the reference used by the route handler.core/ocr.py (1)
41-54: LGTM!The overall design is clean with a well-structured pluggable provider pattern. The
OCRProviderprotocol,OCRResultdataclass with theempty()factory method, and the service abstraction are well implemented. Good use of type hints throughout.Also applies to: 56-78, 80-186, 188-229
core/processor.py (3)
270-279: Verify intent: counting OCR-failed screenshots as "processed".When
OCRProcessingErroris caught, botherrorsandprocessedare incremented (lines 273 and 277). This means thepercentproperty will eventually reach 100%, but theerrorscount will be non-zero. Verify this is the intended behavior—typically errors should not also count as processed sinceremaining = total - processed - errors.
96-118: LGTM on lazy loading pattern.Good use of lazy loading to avoid circular imports. The pattern is consistent across all three service dependencies.
315-398: Well-structured OCR pipeline.The
_process_ocr_for_screenshotmethod has clear step separation, proper error classification, and efficient batch embedding generation. The docstring accurately describes the pipeline and error behavior.core/database.py (5)
61-75: LGTM on thread-safe cursor implementation.Good implementation of thread-safe database access using a lock. The context manager properly handles commit/rollback and cursor cleanup.
213-232: Good defensive fallback for FTS5 trigram tokenizer.The try/except fallback to default tokenizer when trigram isn't available (SQLite < 3.34) is good defensive coding for cross-platform compatibility.
770-780:INSERT OR REPLACEmay return unexpectedlastrowid.When using
INSERT OR REPLACE, if a row with the samescreenshot_idexists, SQLite deletes the old row and inserts a new one. Thelastrowidwill be the new row's ID, which differs from the original. If downstream code expects a stable OCR ID across re-processing, this could cause orphaned references. TheON DELETE CASCADEonocr_text_chunksshould handle cleanup, but verify this is the intended behavior.
1036-1192: Well-designed hybrid search with RRF.The Reciprocal Rank Fusion implementation is clean and correctly combines multiple search signals. Good separation of concerns with the source tracking and snippet generation.
169-263: LGTM on OCR table initialization.The OCR schema design is well-structured with proper foreign keys, cascading deletes, and FTS5 triggers for index synchronization.
| # Auto-start OCR migration if needed (for existing screenshots without OCR) | ||
| # This does NOT process new screenshots - those still require manual sync | ||
| from core.config import config | ||
| from core.processor import processor_service | ||
|
|
||
| if config.ocr.enabled: | ||
| ocr_pending = db.get_ocr_pending_count() | ||
| if ocr_pending > 0: | ||
| print(f"🔄 Starting OCR migration for {ocr_pending} existing screenshots...") | ||
| processor_service.start_ocr_migration() | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether OCR migration is safe under multi-process or guarded elsewhere.
rg -n --hidden -S "start_ocr_migration\(|_ocr_migration_loop|has_ocr\s*=\s*0" core api | head -n 80Repository: VedankPurohit/LiveRecall
Length of output: 192
🏁 Script executed:
#!/bin/bash
# Find core/processor.py and examine start_ocr_migration implementation
find . -name "processor.py" -type fRepository: VedankPurohit/LiveRecall
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Look at api/main.py context around lines 40-70 to see full startup logic and error handling
fd -e py "main.py" api | xargs cat -n | sed -n '40,70p'Repository: VedankPurohit/LiveRecall
Length of output: 1335
🏁 Script executed:
#!/bin/bash
# Read core/processor.py to understand start_ocr_migration implementation
wc -l core/processor.pyRepository: VedankPurohit/LiveRecall
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Look for start_ocr_migration definition
rg -n "def start_ocr_migration" core/processor.py -A 20Repository: VedankPurohit/LiveRecall
Length of output: 1022
🏁 Script executed:
#!/bin/bash
# Check for locking or process-safety mechanisms
rg -n "Lock|lock|Semaphore|process_safe|multi.*worker|uvicorn" core/processor.pyRepository: VedankPurohit/LiveRecall
Length of output: 233
🏁 Script executed:
#!/bin/bash
# Check if exception handling exists elsewhere in startup or if crashes block app
rg -n "try:|except|Exception" core/processor.py | head -20Repository: VedankPurohit/LiveRecall
Length of output: 967
🏁 Script executed:
#!/bin/bash
# Look at db.get_ocr_pending_count() to check for process safety
rg -n "def get_ocr_pending_count" core -A 10Repository: VedankPurohit/LiveRecall
Length of output: 669
🏁 Script executed:
#!/bin/bash
# Check if there's any inter-process locking or startup synchronization elsewhere
rg -n "flock|fcntl|multiprocessing.*Lock|db.*lock" core api --hidden | head -10Repository: VedankPurohit/LiveRecall
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Verify if uvicorn/FastAPI has any built-in single-startup mechanism that prevents duplication
rg -n "uvicorn|workers|--workers" . --type py -B2 -A2 | head -20Repository: VedankPurohit/LiveRecall
Length of output: 722
🏁 Script executed:
#!/bin/bash
# Check if there's a startup guard or singleton pattern elsewhere in api/main.py
cat -n api/main.py | sed -n '1,80p'Repository: VedankPurohit/LiveRecall
Length of output: 3175
Add exception handling and guard multi-worker OCR migration duplication.
When uvicorn runs with multiple workers, each process independently calls start_ocr_migration() during startup, causing duplicated OCR work unless the processor/db layer is explicitly process-safe. Additionally, any exception during migration startup will currently crash the API.
Proposed fix
if config.ocr.enabled:
+ try:
ocr_pending = db.get_ocr_pending_count()
if ocr_pending > 0:
+ # NOTE: Ensure this is safe under multi-process deployments (uvicorn workers > 1).
print(f"🔄 Starting OCR migration for {ocr_pending} existing screenshots...")
processor_service.start_ocr_migration()
+ except Exception as e:
+ # Don't take down the API if OCR migration can't start; surface it in logs.
+ logging.getLogger(__name__).exception("Failed to start OCR migration: %s", e)| host="127.0.0.1", | ||
| port=8742, | ||
| reload=True, | ||
| log_level="warning", # Suppress verbose access logs |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the context around line 210 in api/main.py
sed -n '200,220p' api/main.pyRepository: VedankPurohit/LiveRecall
Length of output: 343
🏁 Script executed:
# Let's also see more context to understand what object/function uses log_level
sed -n '190,230p' api/main.pyRepository: VedankPurohit/LiveRecall
Length of output: 665
🏁 Script executed:
# Search for uvicorn.access configuration and logger setup
rg -n "uvicorn|access|logger" api/main.py -A 2 -B 2Repository: VedankPurohit/LiveRecall
Length of output: 556
🏁 Script executed:
# Search for SSE or progress-related logging
rg -n "SSE|progress|stream|info|debug" api/main.pyRepository: VedankPurohit/LiveRecall
Length of output: 118
🏁 Script executed:
# Search for logging configuration in the entire codebase
rg -n "logging\.config|getLogger|uvicorn" --type pyRepository: VedankPurohit/LiveRecall
Length of output: 980
🏁 Script executed:
# Search for SSE, streaming, WebSocket, or progress event implementations
rg -n "sse|streaming|websocket|progress|EventSourceResponse|Server-Sent" --type py -iRepository: VedankPurohit/LiveRecall
Length of output: 50380
🏁 Script executed:
# Search for any logging calls in the app (especially info level)
rg -n "\.info\(|\.debug\(" --type py api/Repository: VedankPurohit/LiveRecall
Length of output: 50
🏁 Script executed:
# Check if there are any custom loggers or logging config that might be affected
rg -n "getLogger|StreamingResponse|yield|event" --type py api/main.py -B 1 -A 1Repository: VedankPurohit/LiveRecall
Length of output: 572
Clarify logging level configuration and confirm it doesn't inadvertently suppress uvicorn's non-access logs.
The explicit logging.getLogger("uvicorn.access").setLevel(logging.WARNING) at line 38 targets only access logs. However, log_level="warning" at line 210 sets uvicorn's root logging level broadly. Depending on uvicorn's logger hierarchy, these may be redundant or redundantly cautious. Since the app tracks progress via SyncProgress callbacks and SSE endpoints rather than logging, the impact is likely minimal, but verify this is intentional and doesn't suppress any other uvicorn INFO-level diagnostics you may need during development.
🤖 Prompt for AI Agents
In @api/main.py at line 210, The current setup sets uvicorn.access logger to
WARNING via logging.getLogger("uvicorn.access").setLevel(...) but also passes
log_level="warning" into uvicorn.run (symbol: uvicorn.run in main.py), which
sets the root/uvicorn logger level and may unintentionally suppress non-access
INFO logs; decide whether you want to silence only access logs or all uvicorn
logs and update accordingly—either remove or change log_level="warning" to
"info"/None to preserve uvicorn INFO diagnostics, or keep log_level="warning"
and instead explicitly set only "uvicorn.access" to WARNING (and optionally
adjust "uvicorn.error"/root via
logging.getLogger("uvicorn.error"/"uvicorn").setLevel(...)) so behavior matches
your intent (then run a quick dev start to confirm SyncProgress/SSE messages
still appear).
| async def download_stream(): | ||
| """Stream download progress as SSE events""" | ||
| import json | ||
|
|
||
| # Download CLIP model | ||
| yield f"data: {json.dumps({'model': 'clip', 'status': 'starting'})}\n\n" | ||
| try: | ||
| from core.embeddings import is_downloaded as clip_is_downloaded | ||
|
|
||
| if not clip_is_downloaded(): | ||
| yield f"data: {json.dumps({'model': 'clip', 'status': 'downloading'})}\n\n" | ||
| # Import will trigger download | ||
| from core.embeddings import get_text_embedding | ||
|
|
||
| # Make a simple call to ensure model is downloaded | ||
| _ = get_text_embedding("test") | ||
| yield f"data: {json.dumps({'model': 'clip', 'status': 'ready'})}\n\n" | ||
| except Exception as e: | ||
| yield f"data: {json.dumps({'model': 'clip', 'status': 'error', 'error': str(e)})}\n\n" | ||
|
|
||
| # Download BGE text embedding model | ||
| yield f"data: {json.dumps({'model': 'text_embedding', 'status': 'starting'})}\n\n" | ||
| try: | ||
| from core.text_embeddings import is_downloaded as bge_is_downloaded | ||
|
|
||
| if not bge_is_downloaded(): | ||
| yield f"data: {json.dumps({'model': 'text_embedding', 'status': 'downloading'})}\n\n" | ||
| from core.text_embeddings import get_text_embedding as get_bge_embedding | ||
|
|
||
| _ = get_bge_embedding("test") | ||
| yield f"data: {json.dumps({'model': 'text_embedding', 'status': 'ready'})}\n\n" | ||
| except Exception as e: | ||
| yield f"data: {json.dumps({'model': 'text_embedding', 'status': 'error', 'error': str(e)})}\n\n" | ||
|
|
||
| yield f"data: {json.dumps({'status': 'complete'})}\n\n" | ||
|
|
||
| return StreamingResponse( | ||
| download_stream(), | ||
| media_type="text/event-stream", | ||
| headers={ | ||
| "Cache-Control": "no-cache", | ||
| "Connection": "keep-alive", | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Blocking operations in sync generator may block the event loop.
The download_stream generator performs synchronous model downloads (lines 214-217, 229-231) which could block the FastAPI event loop during large downloads. Consider using an async generator with run_in_executor for the blocking operations.
🔧 Proposed fix using async generator
- async def download_stream():
+ async def download_stream():
"""Stream download progress as SSE events"""
import json
+ import asyncio
+ loop = asyncio.get_event_loop()
# Download CLIP model
yield f"data: {json.dumps({'model': 'clip', 'status': 'starting'})}\n\n"
try:
from core.embeddings import is_downloaded as clip_is_downloaded
if not clip_is_downloaded():
yield f"data: {json.dumps({'model': 'clip', 'status': 'downloading'})}\n\n"
- # Import will trigger download
- from core.embeddings import get_text_embedding
-
- # Make a simple call to ensure model is downloaded
- _ = get_text_embedding("test")
+ # Run blocking download in thread pool
+ def download_clip():
+ from core.embeddings import get_text_embedding
+ return get_text_embedding("test")
+ await loop.run_in_executor(None, download_clip)
yield f"data: {json.dumps({'model': 'clip', 'status': 'ready'})}\n\n"
except Exception as e:
yield f"data: {json.dumps({'model': 'clip', 'status': 'error', 'error': str(e)})}\n\n"🤖 Prompt for AI Agents
In @api/routes/setup.py around lines 202 - 245, The sync generator
download_stream blocks the event loop by doing synchronous imports and calls
(e.g., core.embeddings.get_text_embedding,
core.text_embeddings.get_text_embedding and the is_downloaded checks); change
download_stream to an async def async generator and offload blocking operations
to the threadpool with asyncio.get_running_loop().run_in_executor (wrap calls to
clip_is_downloaded/ bge_is_downloaded, the import-triggered model initialization
and the test get_text_embedding/get_bge_embedding calls) so you yield SSE events
from the async generator while awaiting run_in_executor for those heavy
operations; keep the same event payloads and return the same StreamingResponse
using the async generator.
| def main(): | ||
| parser = argparse.ArgumentParser(description="Benchmark OCR and embedding performance") | ||
| parser.add_argument( | ||
| "--dir", | ||
| default="/Users/vedank/Library/Application Support/LiveRecall/screenshots", | ||
| help="Directory containing screenshots", | ||
| ) |
There was a problem hiding this comment.
Hardcoded default path is developer-specific.
The default --dir path /Users/vedank/Library/Application Support/LiveRecall/screenshots is specific to one developer's machine. Consider using a platform-agnostic default or making the path required.
Suggested improvement
parser.add_argument(
"--dir",
- default="/Users/vedank/Library/Application Support/LiveRecall/screenshots",
+ default=None,
help="Directory containing screenshots",
)
+ args = parser.parse_args()
+
+ if args.dir is None:
+ # Use platform-appropriate default
+ import platform
+ if platform.system() == "Darwin":
+ args.dir = str(Path.home() / "Library/Application Support/LiveRecall/screenshots")
+ elif platform.system() == "Windows":
+ args.dir = str(Path(os.environ.get("APPDATA", "")) / "LiveRecall/screenshots")
+ else:
+ args.dir = str(Path.home() / ".local/share/LiveRecall/screenshots")🤖 Prompt for AI Agents
In @scripts/benchmark_ocr.py around lines 366 - 372, The parser in main()
currently hardcodes a developer-specific default for the "--dir" argument
(parser.add_argument("--dir", ...)), so change it to a platform-agnostic
approach: either remove the hardcoded path and make "--dir" required, or set a
generic default (e.g., current working directory or
os.path.expanduser("~/screenshots")) and document it; update the
parser.add_argument call accordingly so the default is not tied to a specific
user's home path.
| import sys | ||
| from pathlib import Path | ||
|
|
||
| # Add project root to path | ||
| project_root = Path(__file__).parent.parent | ||
| sys.path.insert(0, str(project_root)) | ||
|
|
||
| from core.database import db | ||
|
|
There was a problem hiding this comment.
Fix CI E402 and add type hint for main().
The sys.path tweak is fine for a standalone script, but you need to suppress E402 (or move the import inside main()).
Minimal diff to unblock CI and follow guidelines
@@
-from core.database import db
+from core.database import db # noqa: E402
@@
-def main():
+def main() -> None:Also applies to: 20-20
🧰 Tools
🪛 GitHub Actions: CI
[error] 17-17: E402 Module level import not at top of file.
🤖 Prompt for AI Agents
In @scripts/cleanup_ocr.py around lines 10 - 18, The top-level import from
core.database (db) is causing E402; move that import into the main() function to
avoid top-level path mutation issues and add an explicit return type hint to
main (def main() -> None:). Keep the sys.path insertion if needed for standalone
execution, but ensure any project imports like "from core.database import db"
occur inside main(), and update any other duplicate top-level imports
(referenced around line 20) the same way.
| // Calculate migration progress | ||
| const migrationProgress = eventStatus?.ocr_stats | ||
| ? (eventStatus.ocr_stats.completed / (eventStatus.ocr_stats.completed + eventStatus.ocr_stats.pending)) * 100 | ||
| : status?.migration_status?.progress_percent ?? 0; |
There was a problem hiding this comment.
Potential division by zero in migration progress calculation.
If both completed and pending are 0, the division will result in NaN. Add a guard for this edge case.
Suggested fix
const migrationProgress = eventStatus?.ocr_stats
- ? (eventStatus.ocr_stats.completed / (eventStatus.ocr_stats.completed + eventStatus.ocr_stats.pending)) * 100
+ ? (eventStatus.ocr_stats.completed + eventStatus.ocr_stats.pending > 0
+ ? (eventStatus.ocr_stats.completed / (eventStatus.ocr_stats.completed + eventStatus.ocr_stats.pending)) * 100
+ : 100)
: status?.migration_status?.progress_percent ?? 0;📝 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.
| // Calculate migration progress | |
| const migrationProgress = eventStatus?.ocr_stats | |
| ? (eventStatus.ocr_stats.completed / (eventStatus.ocr_stats.completed + eventStatus.ocr_stats.pending)) * 100 | |
| : status?.migration_status?.progress_percent ?? 0; | |
| // Calculate migration progress | |
| const migrationProgress = eventStatus?.ocr_stats | |
| ? (eventStatus.ocr_stats.completed + eventStatus.ocr_stats.pending > 0 | |
| ? (eventStatus.ocr_stats.completed / (eventStatus.ocr_stats.completed + eventStatus.ocr_stats.pending)) * 100 | |
| : 100) | |
| : status?.migration_status?.progress_percent ?? 0; |
🤖 Prompt for AI Agents
In @web/src/app/setup/page.tsx around lines 225 - 228, The migrationProgress
calculation can produce NaN when eventStatus?.ocr_stats.completed +
eventStatus?.ocr_stats.pending === 0; update the logic around migrationProgress
to check the sum (e.g., const total = eventStatus?.ocr_stats?.completed +
eventStatus?.ocr_stats?.pending) and only compute the division when total > 0,
otherwise fall back to status?.migration_status?.progress_percent ?? 0 (or 0);
adjust references to eventStatus, ocr_stats, and migrationProgress accordingly.
- Fix web test to expect search_mode parameter in search requests - Fix ruff SIM102: combine nested if statements in ocr.py - Fix ruff E402: add noqa comments for intentional late imports - Fix mypy arg-type: add None checks before embedding function calls
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
api/routes/search.py (2)
190-218: Fixquick_searchoptional param typing + add return type hints.
start_date: str = None/end_date: str = Noneshould bestr | None = None. Also, repo guideline prefers explicit return types.Proposed fix
@router.get("/quick", response_model=SearchResponse) async def quick_search( q: str, limit: int = 20, safe_mode: bool = False, - start_date: str = None, - end_date: str = None, + start_date: str | None = None, + end_date: str | None = None, visibility: VisibilityFilter = VisibilityFilter.VISIBLE_ONLY, -): +) -> SearchResponse:
171-188: UseEnhancedSearchResultinstead ofSearchResultto include computed fields.The database computes
match_sources(list of matched search methods) andocr_snippet(highlighted text excerpt) and includes them in the returned dictionaries, but these fields are dropped because the code createsSearchResultobjects which don't model them.EnhancedSearchResult(defined in the schema) includes these fields and should be used here instead.core/database.py (1)
42-53: Enable SQLite foreign keys on connect (needed for your ON DELETE CASCADE assumptions).
WithoutPRAGMA foreign_keys = ON, deleting screenshots won’t cascade toscreenshot_ocr/chunks, andINSERT OR REPLACEwon’t cascade-clean old rows.Proposed fix
def connect(self): """Connect to database and initialize sqlite-vec""" self.conn = sqlite3.connect(str(self.db_path), check_same_thread=False) self.conn.row_factory = sqlite3.Row + self.conn.execute("PRAGMA foreign_keys = ON")
🤖 Fix all issues with AI agents
In @api/routes/search.py:
- Around line 114-125: The loop handling results from db.search_ocr_fts uses the
wrong key (`fts_rank`) so similarity always falls back to 0.5; change it to read
`relevance_score` from each result (e.g., r.get("relevance_score")) and compute
a normalized similarity in [0,1] (and invert if your BM25 implementation assigns
lower=better) before assigning r["similarity"]; keep a sensible fallback (e.g.,
0.0) if relevance_score is missing and preserve r["match_sources"] =
["text_fts"] as before.
In @core/database.py:
- Around line 859-907: The FTS query in search_ocr_fts currently aliases
bm25(...) as relevance_score but other layers expect fts_rank; update the SELECT
alias to bm25(ocr_text_fts) AS fts_rank in search_ocr_fts and make the same
consistent change for the other FTS queries referenced (lines ~1036-1193) so the
DB row keys match the API/UI consumers that read fts_rank.
- Around line 821-834: The add_text_embedding function currently does a plain
INSERT into ocr_text_embeddings which will raise a constraint error if chunk_id
already exists; make the insert idempotent by using an upsert (e.g., "INSERT OR
REPLACE INTO ocr_text_embeddings (chunk_id, embedding) VALUES (?, ?)" or
equivalent UPSERT syntax) so reprocessing the same chunk_id simply replaces the
embedding; keep the validation using TEXT_EMBEDDING_DIM and serialize_embedding
unchanged, and continue returning True on success from add_text_embedding.
In @core/ocr_providers/apple_vision.py:
- Around line 39-52: Move the per-instance cache to a module-level singleton and
add type hints: add a module-level variable _ocr_class: type[OCR] | None = None,
remove self._ocr from AppleVisionOCR.__init__, and update
AppleVisionOCR._get_ocr to use and set _ocr_class (import OCR lazily) and
declare its signature as def _get_ocr(self) -> type[OCR]; ensure you import any
typing names needed (e.g., from typing import Optional or use PEP 484/PEP
585-style annotations) so _ocr_class and _get_ocr have the requested types and
the method returns the OCR class.
In @core/text_embeddings.py:
- Around line 72-107: The EMBEDDING_DIM constant must match the loaded model to
avoid silent schema/status mismatches: after successfully loading the model in
_load_model use _model.get_sentence_embedding_dimension() to set or validate
EMBEDDING_DIM (either assign EMBEDDING_DIM = ... or raise a clear error if it
differs from the configured value); apply the same assignment/validation in the
other model-load site referenced around lines 185-196 so the database schema and
status always reflect the actual embedding dimensionality.
In @scripts/benchmark_ocr.py:
- Around line 29-52: The temp-file handling in warmup_models is unsafe on
Windows because NamedTemporaryFile remains open while PIL writes/reads and
unlink() can fail; replace the NamedTemporaryFile usage with tempfile.mkstemp to
get a filename, immediately close the returned fd, use that path for Image.save
and get_image_embedding, and then remove the file in a try/finally (or ensure
Path(path).unlink(missing_ok=True)) so the file is closed before deletion;
reference warmup_models and get_image_embedding when making the change.
🧹 Nitpick comments (11)
web/src/__tests__/api.test.ts (2)
162-178: Consider verifyingsearch_modefor consistency.For comprehensive test coverage, consider also asserting that the
search_modefield is included in the request body when usingsearchWithParams, similar to the custom params test above.📝 Suggested enhancement
const body = JSON.parse( (global.fetch as jest.Mock).mock.calls[0][1].body ); expect(body.query).toBe('test query'); expect(body.limit).toBe(100); expect(body.safe_mode).toBe(true); +expect(body.search_mode).toBeDefined();
122-140: Optional: Verify defaultsearch_modevalue.As an optional enhancement, you could also verify that
search_modedefaults to'auto'when using the simplest search call with default parameters.scripts/cleanup_ocr.py (2)
13-18: Prefer removingsys.pathmutation (or make it more robust) + add type hints to match repo guidelines.
sys.path.insert()works, but tends to break when invoked from unexpected CWDs / packaging contexts; alsomain()lacks type hints.Proposed minimal tweak
-from core.database import db # noqa: E402 +from core.database import db # noqa: E402 @@ -def main(): +def main() -> None: print("OCR Data Cleanup Script")
24-104: Consider usingdb.get_ocr_stats()/db.get_screenshot_count()instead of ad-hoc SQL.
You already added stable DB APIs for stats; using them here reduces coupling to table names and makes migrations less painful.api/routes/search.py (1)
60-95: Avoid.lower()onrequest.search_modeunless it’s guaranteed non-null + string-like.
Ifsearch_modeis an enum, prefer.value(or just rely on enum validation and compare to enum values).core/ocr_providers/apple_vision.py (1)
53-105: Avoidprint()for OCR errors in library code; use a logger (or re-raise a typed error).
Silent “empty result” on unexpected exceptions can hide real regressions (e.g., broken dependency / permission issues).core/database.py (1)
61-76: Locking incursor()is a good safety net; considerthreading.RLock()to reduce deadlock risk on nested DB helpers.
If any method holdingwith self.cursor()calls another method that also usesself.cursor(), a non-reentrant Lock will deadlock.scripts/benchmark_ocr.py (1)
366-384: Avoid a machine-specific default screenshot directory.
Consider defaulting to an env var (or require--dir) to make the script usable for other devs/CI.core/ocr.py (2)
88-126: Consider caching the selected provider instance instead of re-instantiating on every call.
This likely reduces per-image overhead and lets providers keep warm state (you already have_current_provider/_instancefields but don’t use them).Also applies to: 146-159
190-212: Optional: add debug logging for unavailable providers to improve setup troubleshooting.The silent ImportError handling is reasonable for optional dependencies, but users struggling with setup would benefit from seeing which providers failed to load. The provider implementations themselves provide helpful error messages (e.g., "pytesseract is required for Tesseract OCR. Install it with: pip install pytesseract"), but these only appear when users try to actually use a provider. Adding a debug-level log at module import time (e.g.,
logger.debug(f"OCR provider '{provider_name}' not available: {e}")) would surface dependency issues earlier in the setup experience without alarming users with errors for legitimately optional dependencies.core/text_embeddings.py (1)
44-46: Movetorchimports into the functions that use them to align with lazy-loading pattern.Since
api/routes/search.pyalready importstext_embedding_servicelazily (line 86), movingimport torchinto_get_device()and thetorch.cuda.empty_cache()call site inunload_model()would defer torch initialization until actually needed. This reduces baseline import overhead for CLI/server applications that may not require text embeddings on every startup.Current code (lines 44-46)
import numpy as np # noqa: E402 import torch # noqa: E402
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
api/routes/search.pycore/database.pycore/ocr.pycore/ocr_providers/apple_vision.pycore/ocr_providers/tesseract.pycore/text_embeddings.pyscripts/benchmark_ocr.pyscripts/cleanup_ocr.pyweb/src/__tests__/api.test.tsweb/tsconfig.tsbuildinfo
🚧 Files skipped from review as they are similar to previous changes (1)
- core/ocr_providers/tesseract.py
🧰 Additional context used
📓 Path-based instructions (5)
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web/src/**/*.{ts,tsx}: Use functional components with hooks in React code
Use Tailwind CSS for styling in React components
Files:
web/src/__tests__/api.test.ts
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use type hints for function parameters and return types
Use double quotes for strings in Python code
Use private attributes with_leading_underscorenaming convention
Files:
api/routes/search.pyscripts/benchmark_ocr.pyscripts/cleanup_ocr.pycore/ocr_providers/apple_vision.pycore/database.pycore/ocr.pycore/text_embeddings.py
api/routes/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
api/routes/**/*.py: FastAPI routes should use APIRouter with prefix and tags parameters
FastAPI route handlers should include docstrings and specify response_model
Use async/await syntax for FastAPI route handlers
Files:
api/routes/search.py
core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Services should use global singleton instances at module level
Files:
core/ocr_providers/apple_vision.pycore/database.pycore/ocr.pycore/text_embeddings.py
core/database.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use SQLite with sqlite-vec for vector similarity search in database operations
Files:
core/database.py
🧠 Learnings (3)
📚 Learning: 2026-01-11T06:00:24.322Z
Learnt from: CR
Repo: VedankPurohit/LiveRecall PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T06:00:24.322Z
Learning: Applies to **/*.py : Use type hints for function parameters and return types
Applied to files:
scripts/cleanup_ocr.py
📚 Learning: 2026-01-11T06:00:24.323Z
Learnt from: CR
Repo: VedankPurohit/LiveRecall PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T06:00:24.323Z
Learning: Applies to core/database.py : Use SQLite with sqlite-vec for vector similarity search in database operations
Applied to files:
core/database.py
📚 Learning: 2026-01-11T06:00:24.323Z
Learnt from: CR
Repo: VedankPurohit/LiveRecall PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T06:00:24.323Z
Learning: Applies to core/embeddings.py : CLIP model should implement lazy loading with 5-minute auto-unload timeout
Applied to files:
core/text_embeddings.py
🧬 Code graph analysis (5)
api/routes/search.py (2)
core/embeddings.py (3)
get_safe_search_embedding(271-279)get_combined_embedding(211-244)get_text_embedding(196-208)core/database.py (4)
search_similar(607-675)search_ocr_fts(859-906)search_text_embeddings(957-1034)search_hybrid(1036-1192)
scripts/benchmark_ocr.py (5)
core/embeddings.py (1)
get_image_embedding(180-193)core/ocr_providers/apple_vision.py (1)
extract_text(53-104)core/ocr_providers/tesseract.py (1)
extract_text(58-111)core/ocr.py (1)
get_provider_name(177-183)core/chunking.py (1)
chunk_ocr_text(247-270)
scripts/cleanup_ocr.py (1)
core/database.py (4)
connect(42-53)cursor(62-75)reset_all_ocr(1299-1319)disconnect(55-59)
core/ocr_providers/apple_vision.py (2)
core/ocr.py (8)
OCRResult(42-53)extract_text(67-69)extract_text(146-158)extract_text(215-217)empty(51-53)is_available(71-73)is_available(169-175)get_model_info(75-77)api/schemas.py (1)
OCRResult(503-509)
core/text_embeddings.py (1)
core/embeddings.py (12)
_get_device(31-37)_load_model(40-69)_schedule_auto_unload(72-83)_check_and_unload(86-96)unload_model(99-126)is_loaded(129-131)is_downloaded(134-146)get_model_status(149-158)set_auto_unload_timeout(161-166)_normalize(169-177)get_text_embedding(196-208)cosine_similarity(247-254)
🔇 Additional comments (1)
web/src/__tests__/api.test.ts (1)
142-160: LGTM! Test correctly reflects the new search mode API.The addition of
search_mode: 'auto'properly validates that the search function defaults to auto mode, aligning with the hybrid search functionality introduced in this PR.
| elif search_mode == "text_fuzzy": | ||
| # Pure FTS5 text search | ||
| results = db.search_ocr_fts( | ||
| query=request.query, | ||
| limit=search_limit, | ||
| visibility=request.visibility.value, | ||
| ) | ||
| # Normalize response format | ||
| for r in results: | ||
| r["similarity"] = r.get("fts_rank", 0.5) # FTS rank as similarity | ||
| r["match_sources"] = ["text_fts"] | ||
|
|
There was a problem hiding this comment.
FTS result key mismatch makes similarity wrong (currently defaults to 0.5).
db.search_ocr_fts() (as shown) returns relevance_score (bm25), not fts_rank. This makes text_fuzzy scoring misleading and can break sorting/UX.
Proposed fix (use relevance_score and normalize/invert)
elif search_mode == "text_fuzzy":
# Pure FTS5 text search
results = db.search_ocr_fts(
query=request.query,
limit=search_limit,
visibility=request.visibility.value,
)
# Normalize response format
for r in results:
- r["similarity"] = r.get("fts_rank", 0.5) # FTS rank as similarity
+ # bm25: lower is better; map to (0,1] for UI sorting
+ score = r.get("relevance_score", None)
+ r["similarity"] = (1.0 / (1.0 + float(score))) if score is not None else 0.0
r["match_sources"] = ["text_fts"]🤖 Prompt for AI Agents
In @api/routes/search.py around lines 114 - 125, The loop handling results from
db.search_ocr_fts uses the wrong key (`fts_rank`) so similarity always falls
back to 0.5; change it to read `relevance_score` from each result (e.g.,
r.get("relevance_score")) and compute a normalized similarity in [0,1] (and
invert if your BM25 implementation assigns lower=better) before assigning
r["similarity"]; keep a sensible fallback (e.g., 0.0) if relevance_score is
missing and preserve r["match_sources"] = ["text_fts"] as before.
| import numpy as np # noqa: E402 | ||
| import torch # noqa: E402 | ||
|
|
||
| # Lazy loading state | ||
| _model = None | ||
| _device: str | None = None | ||
| _last_used: float = 0 | ||
| _auto_unload_timer: threading.Timer | None = None | ||
| _lock = threading.Lock() |
There was a problem hiding this comment.
Prevent auto-unload from racing with active encode() (can crash mid-batch).
Timer thread can call unload_model() while model.encode(...) is running; _last_used is updated before encode and won’t protect long-running calls. Add an “in-use” counter or hold a lock around encode + unload checks.
Sketch of a minimal in-use guard
_lock = threading.Lock()
+_in_use = 0
@@
def _check_and_unload():
@@
- if idle_time >= AUTO_UNLOAD_SECONDS:
+ if idle_time >= AUTO_UNLOAD_SECONDS and _in_use == 0:
print(f"Text embedding model idle for {idle_time:.0f}s, unloading...")
unload_model()
@@
def get_text_embedding(text: str) -> list[float]:
@@
- try:
+ global _in_use
+ try:
+ _in_use += 1
embedding = model.encode(text, convert_to_tensor=False)
return _normalize(embedding)
except Exception as e:
print(f"Error generating text embedding: {e}")
raise
+ finally:
+ _in_use -= 1Also applies to: 124-135, 216-300
| def _load_model(model_name: str | None = None): | ||
| """Load the text embedding model (only called once)""" | ||
| global _model, _device, _last_used, _current_model_name | ||
|
|
||
| if model_name is None: | ||
| model_name = _current_model_name | ||
|
|
||
| with _lock: | ||
| if _model is not None: | ||
| _last_used = time.time() | ||
| return _model | ||
|
|
||
| print(f"Loading text embedding model: {model_name}...") | ||
|
|
||
| from sentence_transformers import SentenceTransformer | ||
|
|
||
| _device = _get_device() | ||
| print(f"Using device: {_device}") | ||
|
|
||
| try: | ||
| _model = SentenceTransformer(model_name, device=_device) | ||
| _current_model_name = model_name | ||
| except Exception as e: | ||
| print(f"Error loading on {_device}, falling back to CPU: {e}") | ||
| _device = "cpu" | ||
| _model = SentenceTransformer(model_name, device="cpu") | ||
| _current_model_name = model_name | ||
|
|
||
| _last_used = time.time() | ||
| print(f"Text embedding model loaded successfully (dim={_model.get_sentence_embedding_dimension()})") | ||
|
|
||
| # Start auto-unload timer | ||
| _schedule_auto_unload() | ||
|
|
||
| return _model | ||
|
|
There was a problem hiding this comment.
EMBEDDING_DIM should reflect the loaded model (or be validated).
If config switches to a 768-dim model, DB schema and status will silently disagree. Consider setting EMBEDDING_DIM = _model.get_sentence_embedding_dimension() after load (or validating and raising with an actionable message).
Also applies to: 185-196
🤖 Prompt for AI Agents
In @core/text_embeddings.py around lines 72 - 107, The EMBEDDING_DIM constant
must match the loaded model to avoid silent schema/status mismatches: after
successfully loading the model in _load_model use
_model.get_sentence_embedding_dimension() to set or validate EMBEDDING_DIM
(either assign EMBEDDING_DIM = ... or raise a clear error if it differs from the
configured value); apply the same assignment/validation in the other model-load
site referenced around lines 185-196 so the database schema and status always
reflect the actual embedding dimensionality.
| def warmup_models(): | ||
| """Pre-load all models so benchmark isn't skewed by loading time""" | ||
| print("\n" + "=" * 60) | ||
| print("WARMING UP MODELS (loading into memory)") | ||
| print("=" * 60) | ||
|
|
||
| # CLIP model | ||
| print(" Loading CLIP model...", end=" ", flush=True) | ||
| start = time.perf_counter() | ||
| # Create a tiny test image to trigger model load | ||
| import tempfile | ||
|
|
||
| from PIL import Image | ||
|
|
||
| from core.embeddings import get_image_embedding | ||
|
|
||
| with tempfile.NamedTemporaryFile(suffix=".jpg", delete=False) as f: | ||
| img = Image.new("RGB", (100, 100), color="white") | ||
| img.save(f.name) | ||
| with contextlib.suppress(Exception): | ||
| get_image_embedding(f.name) | ||
| Path(f.name).unlink() | ||
| print(f"done ({time.perf_counter() - start:.1f}s)") | ||
|
|
There was a problem hiding this comment.
Fix temp-file handling: NamedTemporaryFile + PIL + unlink can break on Windows.
The file is still open when PIL writes/reads, and unlink() can fail on Windows.
Proposed fix (mkstemp)
- import tempfile
+ import tempfile
@@
- with tempfile.NamedTemporaryFile(suffix=".jpg", delete=False) as f:
- img = Image.new("RGB", (100, 100), color="white")
- img.save(f.name)
- with contextlib.suppress(Exception):
- get_image_embedding(f.name)
- Path(f.name).unlink()
+ fd, path = tempfile.mkstemp(suffix=".jpg")
+ os.close(fd)
+ try:
+ img = Image.new("RGB", (100, 100), color="white")
+ img.save(path)
+ with contextlib.suppress(Exception):
+ get_image_embedding(path)
+ finally:
+ with contextlib.suppress(Exception):
+ Path(path).unlink()📝 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.
| def warmup_models(): | |
| """Pre-load all models so benchmark isn't skewed by loading time""" | |
| print("\n" + "=" * 60) | |
| print("WARMING UP MODELS (loading into memory)") | |
| print("=" * 60) | |
| # CLIP model | |
| print(" Loading CLIP model...", end=" ", flush=True) | |
| start = time.perf_counter() | |
| # Create a tiny test image to trigger model load | |
| import tempfile | |
| from PIL import Image | |
| from core.embeddings import get_image_embedding | |
| with tempfile.NamedTemporaryFile(suffix=".jpg", delete=False) as f: | |
| img = Image.new("RGB", (100, 100), color="white") | |
| img.save(f.name) | |
| with contextlib.suppress(Exception): | |
| get_image_embedding(f.name) | |
| Path(f.name).unlink() | |
| print(f"done ({time.perf_counter() - start:.1f}s)") | |
| def warmup_models(): | |
| """Pre-load all models so benchmark isn't skewed by loading time""" | |
| print("\n" + "=" * 60) | |
| print("WARMING UP MODELS (loading into memory)") | |
| print("=" * 60) | |
| # CLIP model | |
| print(" Loading CLIP model...", end=" ", flush=True) | |
| start = time.perf_counter() | |
| # Create a tiny test image to trigger model load | |
| import os | |
| import tempfile | |
| from PIL import Image | |
| from core.embeddings import get_image_embedding | |
| fd, path = tempfile.mkstemp(suffix=".jpg") | |
| os.close(fd) | |
| try: | |
| img = Image.new("RGB", (100, 100), color="white") | |
| img.save(path) | |
| with contextlib.suppress(Exception): | |
| get_image_embedding(path) | |
| finally: | |
| with contextlib.suppress(Exception): | |
| Path(path).unlink() | |
| print(f"done ({time.perf_counter() - start:.1f}s)") |
🤖 Prompt for AI Agents
In @scripts/benchmark_ocr.py around lines 29 - 52, The temp-file handling in
warmup_models is unsafe on Windows because NamedTemporaryFile remains open while
PIL writes/reads and unlink() can fail; replace the NamedTemporaryFile usage
with tempfile.mkstemp to get a filename, immediately close the returned fd, use
that path for Image.save and get_image_embedding, and then remove the file in a
try/finally (or ensure Path(path).unlink(missing_ok=True)) so the file is closed
before deletion; reference warmup_models and get_image_embedding when making the
change.
- Replace bare except/print with proper logging in API routes - Add thread error handling in sync recompute background task - Fix zero vector handling in cosine_similarity - Add provider caching to OCRService - Remove unused offset variable in processor - Fix division by zero in setup page progress calculation - Add SearchMode enum validation in search endpoint - Fix benchmark script hardcoded path and undefined variable
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @core/ocr.py:
- Around line 25-28: The docs incorrectly reference a non-existent
OCRService.recompute_all_ocr(); either remove that line or replace it with the
actual API call(s): update the example to use the existing
OCRService.get_provider() (to inspect current provider) and the appropriate
method to change providers (e.g., a setter or configuration reload routine), or
implement a new OCRService.recompute_all_ocr() that re-runs OCR for stored
items; ensure you reference the OCRService class and the chosen method name
(get_provider() or recompute_all_ocr()) consistently in the doc comment.
🧹 Nitpick comments (8)
core/text_embeddings.py (1)
84-101: Consider using logging instead of print statements.The module uses
print()for status messages (lines 84, 89, 95, 101, 130, 145, 161, etc.) while other files in this PR uselogging. For consistency and production-readiness, consider using theloggingmodule.Example refactor
+import logging + +logger = logging.getLogger(__name__) + def _load_model(model_name: str | None = None): ... - print(f"Loading text embedding model: {model_name}...") + logger.info("Loading text embedding model: %s", model_name) ... - print(f"Using device: {_device}") + logger.info("Using device: %s", _device)api/main.py (1)
214-214: Log level suppresses verbose access logs.Setting
log_level="warning"reduces uvicorn noise. Consider making this configurable via environment variable for debugging scenarios.api/routes/setup.py (2)
146-147: Addresponse_modelfor API documentation.Per coding guidelines, FastAPI routes should specify
response_model. Consider defining a schema for the model status response.Suggested fix
-@router.get("/models") -async def get_model_status(): +@router.get("/models", response_model=dict) # Or define a ModelsStatusResponse schema +async def get_model_status():Or better, create a dedicated response model in
api/schemas.pyand use it here.
294-295: Addresponse_modelfor enhanced status endpoint.Per coding guidelines, the
response_modelshould be specified in the decorator.Suggested fix
-@router.get("/enhanced-status") +@router.get("/enhanced-status", response_model=EnhancedSetupStatus) async def get_enhanced_setup_status():api/routes/events.py (1)
186-188: Use logger instead of print for error handling.The exception handlers use
print()for errors while the module has a logger defined. Uselogger.warning()for consistency.Suggested fix
except Exception as e: - print(f"Error getting CLIP status: {e}") + logger.warning("Error getting CLIP status: %s", e)Apply similar changes to lines 207, 219, 245, and 256.
Also applies to: 206-207, 218-219, 244-245, 255-256
core/processor.py (1)
384-384: Consider usingstrict=Truefor zip.The
zip(..., strict=False)on line 384 silently allows mismatched lengths between chunks and embeddings. Since these should always match (generated from the same text list), usingstrict=Truewould catch bugs where they diverge unexpectedly.Suggested fix
- for (chunk_size, chunk), embedding in zip(all_chunks, all_embeddings, strict=False): + for (chunk_size, chunk), embedding in zip(all_chunks, all_embeddings, strict=True):core/ocr.py (2)
90-92: Unused instance variable_current_provider.The
_current_providerattribute is initialized but never assigned or read elsewhere in the class. If this was intended for tracking the last-used provider, consider either implementing that functionality or removing the unused variable.🔧 Suggested fix
def __init__(self): - self._current_provider: OCRProvider | None = None self._provider_cache: dict[str, OCRProvider] = {}
196-213: Consider adding debug logging for failed provider imports.Silent
passonImportErroris appropriate for optional dependencies, but adding debug-level logging would aid troubleshooting when providers unexpectedly fail to register.🔧 Suggested enhancement
+import logging + +logger = logging.getLogger(__name__) + def _register_providers(): """Register available OCR providers""" # Import and register Apple Vision provider (macOS only) if platform.system() == "Darwin": try: from core.ocr_providers.apple_vision import AppleVisionOCR OCRService.register("apple_vision", AppleVisionOCR) - except ImportError: - pass # ocrmac not installed + except ImportError as e: + logger.debug("Apple Vision OCR not available: %s", e) # Import and register Tesseract provider (cross-platform) try: from core.ocr_providers.tesseract import TesseractOCR OCRService.register("tesseract", TesseractOCR) - except ImportError: - pass # pytesseract not installed + except ImportError as e: + logger.debug("Tesseract OCR not available: %s", e)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
api/main.pyapi/routes/events.pyapi/routes/search.pyapi/routes/setup.pyapi/routes/sync.pyapi/schemas.pycore/database.pycore/embeddings.pycore/ocr.pycore/processor.pycore/text_embeddings.pyscripts/benchmark_ocr.pyscripts/cleanup_ocr.pyweb/src/app/setup/page.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/cleanup_ocr.py
🧰 Additional context used
📓 Path-based instructions (8)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use type hints for function parameters and return types
Use double quotes for strings in Python code
Use private attributes with_leading_underscorenaming convention
Files:
api/routes/setup.pycore/embeddings.pyapi/routes/search.pycore/text_embeddings.pycore/processor.pyapi/routes/events.pyapi/routes/sync.pyapi/schemas.pycore/ocr.pyscripts/benchmark_ocr.pycore/database.pyapi/main.py
api/routes/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
api/routes/**/*.py: FastAPI routes should use APIRouter with prefix and tags parameters
FastAPI route handlers should include docstrings and specify response_model
Use async/await syntax for FastAPI route handlers
Files:
api/routes/setup.pyapi/routes/search.pyapi/routes/events.pyapi/routes/sync.py
core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Services should use global singleton instances at module level
Files:
core/embeddings.pycore/text_embeddings.pycore/processor.pycore/ocr.pycore/database.py
core/embeddings.py
📄 CodeRabbit inference engine (CLAUDE.md)
CLIP model should implement lazy loading with 5-minute auto-unload timeout
Files:
core/embeddings.py
core/processor.py
📄 CodeRabbit inference engine (CLAUDE.md)
Background sync service for processing screenshots should be implemented in core/processor.py
Files:
core/processor.py
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web/src/**/*.{ts,tsx}: Use functional components with hooks in React code
Use Tailwind CSS for styling in React components
Files:
web/src/app/setup/page.tsx
core/database.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use SQLite with sqlite-vec for vector similarity search in database operations
Files:
core/database.py
api/main.py
📄 CodeRabbit inference engine (CLAUDE.md)
FastAPI app should serve REST API and static Next.js build from web/out/
Files:
api/main.py
🧠 Learnings (4)
📚 Learning: 2026-01-11T06:00:24.323Z
Learnt from: CR
Repo: VedankPurohit/LiveRecall PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T06:00:24.323Z
Learning: Applies to core/embeddings.py : CLIP model should implement lazy loading with 5-minute auto-unload timeout
Applied to files:
core/embeddings.pycore/text_embeddings.py
📚 Learning: 2026-01-11T06:00:24.323Z
Learnt from: CR
Repo: VedankPurohit/LiveRecall PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T06:00:24.323Z
Learning: Applies to core/processor.py : Background sync service for processing screenshots should be implemented in core/processor.py
Applied to files:
core/processor.pyapi/routes/sync.pyapi/main.py
📚 Learning: 2026-01-11T06:00:24.323Z
Learnt from: CR
Repo: VedankPurohit/LiveRecall PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T06:00:24.323Z
Learning: Applies to core/database.py : Use SQLite with sqlite-vec for vector similarity search in database operations
Applied to files:
core/database.py
📚 Learning: 2026-01-11T06:00:24.323Z
Learnt from: CR
Repo: VedankPurohit/LiveRecall PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T06:00:24.323Z
Learning: Applies to api/main.py : FastAPI app should serve REST API and static Next.js build from web/out/
Applied to files:
api/main.py
🧬 Code graph analysis (8)
api/routes/setup.py (7)
core/embeddings.py (3)
get_model_status(153-162)is_downloaded(134-150)get_text_embedding(200-212)core/text_embeddings.py (6)
get_model_status(185-196)get_model_status(344-346)is_downloaded(169-182)is_downloaded(356-358)get_text_embedding(216-238)get_text_embedding(332-334)core/ocr.py (2)
is_available(71-73)is_available(175-181)core/ocr_providers/tesseract.py (1)
is_available(113-123)core/ocr_providers/apple_vision.py (1)
is_available(106-116)api/schemas.py (2)
MigrationStatus(718-741)EnhancedSetupStatus(744-766)core/database.py (1)
get_ocr_stats(1258-1283)
core/embeddings.py (2)
core/text_embeddings.py (4)
is_downloaded(169-182)is_downloaded(356-358)get_model_status(185-196)get_model_status(344-346)api/routes/setup.py (1)
get_model_status(147-192)
core/text_embeddings.py (2)
core/embeddings.py (8)
_get_device(31-37)_load_model(40-69)_schedule_auto_unload(72-83)_check_and_unload(86-96)unload_model(99-126)is_loaded(129-131)is_downloaded(134-150)get_model_status(153-162)api/routes/setup.py (1)
get_model_status(147-192)
core/processor.py (5)
core/embeddings.py (1)
get_image_embedding(184-197)core/database.py (11)
get_ocr_pending_count(863-867)add_embedding(590-605)get_unsynced_count(578-582)get_unsynced_screenshots(569-576)mark_ocr_complete(845-849)add_ocr_text(750-789)add_ocr_chunk(798-828)add_text_embedding(830-843)get_screenshots_without_ocr(851-861)reset_all_ocr(1316-1336)get_screenshot_count(584-588)core/ocr.py (3)
extract_text(67-69)extract_text(152-164)extract_text(221-223)core/chunking.py (1)
chunk_ocr_text(247-270)core/text_embeddings.py (2)
get_batch_embeddings(269-299)get_batch_embeddings(340-342)
web/src/app/setup/page.tsx (2)
api/schemas.py (3)
SetupStatus(477-484)EnhancedSetupStatus(744-766)MigrationStatus(718-741)core/processor.py (1)
SyncProgress(31-63)
core/ocr.py (3)
api/schemas.py (1)
OCRResult(512-518)core/ocr_providers/tesseract.py (3)
extract_text(58-111)is_available(113-123)get_model_info(125-147)core/ocr_providers/apple_vision.py (4)
extract_text(53-104)is_available(106-116)get_model_info(118-129)AppleVisionOCR(26-129)
core/database.py (1)
tray/backend.py (1)
start(66-116)
api/main.py (2)
core/database.py (1)
get_ocr_pending_count(863-867)core/processor.py (1)
start_ocr_migration(139-158)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Type Check (MyPy)
🔇 Additional comments (61)
core/embeddings.py (2)
134-150: LGTM! Consistent implementation with text_embeddings module.The download check follows the same pattern as
core/text_embeddings.py, checking bothconfig_sentence_transformers.jsonandconfig.jsonas fallback. Exception handling is appropriate for a status check.
153-162: LGTM! Status enhancement aligns with setup requirements.Adding the
downloadedfield enables the setup page to distinguish between "model downloaded but not loaded" and "model not downloaded", improving the user experience during initial setup.api/schemas.py (3)
168-175: LGTM! Search mode integration is well-structured.The
SearchModeenum and its integration intoSearchRequestprovide clear options for hybrid, image-only, and text-only search modes. Good use of Field descriptions and sensible defaults.Also applies to: 177-206
507-590: LGTM! Comprehensive OCR and search result schemas.The OCR-related schemas (
OCRResult,ChunkInfo,ChunkedText,MatchSource,EnhancedSearchResult) are well-defined with proper type hints and descriptions. The enhanced search result schema effectively combines similarity scores with match source attribution and OCR snippets.
640-677: LGTM! Detailed progress tracking schemas.The
EnhancedSyncStatusandMigrationStatusschemas provide comprehensive progress tracking with phase-level granularity. The examples in the Config classes are helpful for API consumers.Also applies to: 713-765
core/database.py (4)
40-40: LGTM! Thread-safe database access implemented correctly.The addition of
_lock = threading.Lock()and wrapping the cursor context manager ensures safe concurrent access from OCR and text embedding workflows. This is essential for the multi-phase sync process.As per coding guidelines: Using SQLite with sqlite-vec for vector similarity search.
Also applies to: 62-76
169-263: LGTM! Robust OCR table initialization with fallback handling.The OCR table structure is well-designed:
- FTS5 with trigram tokenizer for fuzzy search (with fallback to default tokenizer for older SQLite)
- Triggers keep FTS index synchronized automatically
- 384-dim vector table for text embeddings using sqlite-vec
The try-except fallback for trigram tokenizer (lines 215-232) gracefully handles SQLite versions < 3.34.
As per coding guidelines: Using SQLite with sqlite-vec for vector similarity search.
990-999: LGTM! Good SQL injection prevention.The explicit validation of
chunk_sizeparameter on lines 995-996 prevents SQL injection, even though parameterized queries are used. This defense-in-depth approach is commendable.
1053-1210: LGTM! Well-implemented hybrid search with RRF.The Reciprocal Rank Fusion implementation correctly:
- Combines results from multiple search methods (image, FTS, text semantic small/large)
- Calculates RRF scores with standard k=60 parameter
- Deduplicates by screenshot ID while tracking match sources
- Creates snippets for text matches
The complexity is justified by the sophisticated hybrid search requirements.
scripts/benchmark_ocr.py (4)
31-73: LGTM! Good warmup strategy to isolate benchmark timing.Pre-loading all models prevents the first benchmark from being skewed by loading overhead. The use of
contextlib.suppress(Exception)on line 50 is appropriate for warmup where errors are not critical.
76-89: LGTM! Robust image sampling with clear error messages.The function provides helpful error messages when the directory doesn't exist or contains no images, preventing confusing benchmark failures.
92-121: LGTM! Safe statistics with division-by-zero guard.Line 118 correctly checks
if len(times) > 1before calculating standard deviation, preventing runtime errors with single-sample benchmarks.
341-369: LGTM! Comprehensive performance summary.The total processing time estimation is valuable for capacity planning. The breakdown by phase (CLIP, OCR, chunking, embedding) helps identify bottlenecks.
web/src/app/setup/page.tsx (5)
3-3: LGTM! Proper use of useCallback for stable function references.Memoizing
fetchStatusandfetchEventStatuswithuseCallbackprevents unnecessary re-renders and ensures stable dependencies for the effects that consume them.As per coding guidelines: Using functional components with hooks in React code.
Also applies to: 68-91
226-230: LGTM! Division-by-zero guard prevents runtime errors.The conditional check on line 227 ensures we never divide by zero when calculating migration progress. The ternary operator correctly returns 100% when there are no screenshots to process.
94-119: LGTM! Robust step determination logic.The
determineStepfunction correctly prioritizes setup steps and uses optional chaining to safely access nested status properties. The dual status check (enhanced-status and events-status) provides resilience during model downloads.
169-196: LGTM! Well-coordinated auto-trigger behavior.The auto-download (lines 169-187) and auto-sync (lines 190-196) effects use control flags (
downloadTriggered,syncTriggered) to prevent duplicate API calls. This ensures a smooth background setup experience.As per coding guidelines: Using functional components with hooks in React code.
395-467: LGTM! Comprehensive migration progress UI.The migration step provides clear feedback with:
- Visual progress bar
- Processed/remaining stats
- Current phase indicators
- Estimated time remaining (with guard on line 451 to only show when < 100%)
As per coding guidelines: Using Tailwind CSS for styling in React components.
core/text_embeddings.py (7)
1-30: Well-documented module header with model options.The docstring provides useful guidance on model alternatives and switching procedures.
47-61: Global state follows established patterns.The lazy loading state and configuration constants are consistent with
core/embeddings.py, using private attributes with leading underscores per coding guidelines.
109-135: Auto-unload mechanism correctly implemented.The timer-based auto-unload matches the 5-minute idle timeout pattern from the CLIP embeddings module, with proper cleanup and rescheduling logic.
207-213: Normalization correctly handles zero vectors.The
_normalizefunction properly checks for zero norm before dividing, preventing potential division-by-zero errors.
241-267: Correct BGE query instruction prefix usage.The asymmetric encoding approach (plain text for documents, instruction-prefixed for queries) is the recommended pattern for BGE models and improves retrieval quality.
309-317: Cosine similarity correctly handles edge cases.The zero-norm check prevents division-by-zero when comparing embeddings. This aligns with the commit message mentioning "fix zero-vector handling in cosine similarity."
361-362: Singleton pattern correctly implemented.The global
text_embedding_serviceinstance follows the coding guideline for services to use global singleton instances at module level.api/routes/search.py (5)
1-13: Clear module documentation for search modes.The docstring effectively documents the available search modes and the hybrid RRF approach.
34-51: Route handler follows coding guidelines.The endpoint includes a docstring, uses
response_model, and async/await syntax as required by the coding guidelines.
84-88: Lazy import prevents unnecessary model loading.The lazy import of
text_embedding_serviceensures the BGE model is only loaded when text-based search is actually requested, which is a good memory optimization.
136-138: Verifymatch_sourcesaccuracy for text_semantic mode.The code sets
match_sourcesto both["text_semantic_small", "text_semantic_large"]unconditionally. If the result came from only one chunk size, this may misrepresent the actual source. Consider if the database returns source information that could be used here.
190-217: Quick search endpoint maintains backward compatibility.The GET endpoint delegates to the main search function with default parameters, maintaining a simple API for basic use cases while the POST endpoint offers full control.
api/main.py (3)
16-16: Events router correctly integrated.The
eventsrouter is properly imported and will be mounted at/api/v1providing SSE support for real-time updates.
51-64: OCR migration auto-start is safely implemented.The startup code:
- Guards with
config.ocr.enabledcheck- Only starts if there are pending items
- Catches exceptions to prevent app crash
- Uses proper logging for errors
140-140: Events router mounted correctly.The events router is properly included with the
/api/v1prefix, consistent with other routers.api/routes/setup.py (3)
6-16: Logging properly configured.Module-level logger follows best practices for structured logging.
195-249: Download endpoint streams progress effectively.The SSE streaming approach provides good UI feedback during model downloads. The sequential download order (CLIP first, then BGE) is appropriate.
252-291: Migration status endpoint handles edge cases.The division-by-zero protection at line 270 and the exception fallback returning safe defaults demonstrate good defensive programming.
api/routes/events.py (5)
1-26: Well-documented SSE module with clear event types.The module docstring clearly documents the event types (model_download, sync_progress, model_status) and the implementation uses proper logging.
34-46: Thread-safe broadcasting with graceful queue overflow handling.The implementation correctly:
- Copies the subscriber list under lock to prevent race conditions
- Uses
contextlib.suppress(asyncio.QueueFull)to drop events for slow clients rather than blockingThis is appropriate for SSE where missing occasional events is acceptable.
65-132: Event generator correctly bootstraps and maintains connections.The generator:
- Sends initial status on connect for immediate client state
- Uses 30-second timeout with keepalive pings to prevent connection drops
- Wraps status retrieval in try/except for resilience
139-159: SSE endpoint configured correctly.The endpoint includes proper headers for SSE:
Cache-Control: no-cacheprevents cachingConnection: keep-alivemaintains the streamX-Accel-Buffering: nodisables nginx buffering for real-time delivery
267-314: Helper functions enable modular event emission.The
emit_model_download_progress,emit_model_status, andemit_sync_progresshelpers provide a clean API for other modules to broadcast events without coupling to the internal implementation.core/processor.py (9)
1-12: Module docstring clearly documents the processing phases.The documentation explains the three-phase pipeline and how to reconfigure OCR/embedding models.
24-28: OCRProcessingError exception for expected failures.Good practice to distinguish expected OCR failures (corrupted images) from unexpected errors, allowing appropriate error handling strategies.
30-54: Enhanced SyncProgress with detailed phase tracking.The new fields (
current_phase,embeddings_done,ocr_done,text_embeddings_done,last_error,error_type) enable rich progress reporting for the multi-phase pipeline.
91-118: Lazy loading prevents circular imports.The lazy-loading pattern for OCR, text embedding, and chunking services is appropriate to avoid circular import issues while keeping the processor module focused.
139-181: OCR migration entry point properly isolated.The
start_ocr_migrationmethod:
- Guards against concurrent runs
- Only starts if there's pending work
- Runs in a daemon thread for background processing
This separation allows targeted OCR migration without re-running CLIP embeddings.
270-300: Comprehensive error categorization aids debugging.The differentiated error handling:
OCRProcessingError: Expected failure, marks complete to prevent retry loopsTypeError/AttributeError/KeyError: Code bugs, doesn't mark complete for retry after fixFileNotFoundError/OSError: File issues, skips gracefully- Other exceptions: Logged with context
This approach provides actionable error information while maintaining resilience.
400-441: Pending OCR processing maintains consistent error handling.The error handling mirrors
_process_loop, ensuring consistent behavior and error categorization across different entry points.
443-498: OCR recompute provides clean slate regeneration.The
recompute_ocrmethod correctly:
- Prevents concurrent execution
- Clears existing data before reprocessing
- Processes all screenshots with progress tracking
This is appropriate for model changes requiring full regeneration.
500-502: Global singleton follows coding guidelines.The
processor_servicesingleton at module level follows the coding guideline for core services.api/routes/sync.py (6)
1-9: Module documentation updated for multi-phase sync.The docstring clearly describes the three-phase processing pipeline.
52-65: Enhanced status converter captures OCR phase details.The
progress_to_enhanced_statusfunction correctly maps all new SyncProgress fields to the EnhancedSyncStatus schema.
96-117: Start sync now reflects OCR-inclusive totals.The endpoint correctly aggregates unsynced screenshots and OCR-pending items, providing accurate work estimates in the response message.
210-256: OCR management endpoints provide full CRUD access.The endpoints for OCR status, configuration retrieval, and updates follow REST conventions with proper response models and configuration persistence.
285-294: Background thread for recompute has proper error handling.The thread wraps
recompute_ocrin try/except withlogger.exception, ensuring errors are captured rather than silently lost in the daemon thread.
303-344: All models status endpoint aggregates component states.The endpoint correctly:
- Uses lazy imports to avoid loading unused models
- Handles component failures gracefully with warnings
- Returns a comprehensive status using the
AllModelsStatusschemacore/ocr.py (5)
41-53: LGTM!The
OCRResultdataclass is well-structured with sensible defaults and a usefulempty()factory method. The separation between this internal dataclass and the Pydantic model inapi/schemas.pyis a clean pattern for decoupling core logic from API serialization.
56-77: LGTM!The
OCRProviderProtocol is well-designed withruntime_checkableenabling isinstance checks. The interface is clean and the existing provider implementations incore/ocr_providers/conform to this contract.
192-193: LGTM!Global singleton pattern follows the coding guidelines for services in
core/. This enables consistent state for provider caching across the application.
220-233: LGTM!Convenience functions provide a clean module-level API that delegates to the global singleton. This is idiomatic Python and makes the common use cases accessible without requiring users to interact with the service instance directly.
236-243: LGTM!The
__main__block provides useful diagnostics for verifying OCR setup and is appropriately simple for its purpose.
| To switch provider: | ||
| 1. Update config: config.ocr.provider = "paddleocr" | ||
| 2. Run: ocr_service.recompute_all_ocr() | ||
| ============================================================================= |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "def recompute_all_ocr" --type pyRepository: VedankPurohit/LiveRecall
Length of output: 50
🏁 Script executed:
rg -n "recompute_all_ocr" --type py -B 2 -A 2Repository: VedankPurohit/LiveRecall
Length of output: 340
🏁 Script executed:
cat -n core/ocr.py | head -50Repository: VedankPurohit/LiveRecall
Length of output: 1953
🏁 Script executed:
ast-grep --pattern 'class OCRService {
$$$
}'Repository: VedankPurohit/LiveRecall
Length of output: 50
🏁 Script executed:
sed -n '80,190p' core/ocr.pyRepository: VedankPurohit/LiveRecall
Length of output: 3866
🏁 Script executed:
rg -n "class OCRService" -A 50 --type pyRepository: VedankPurohit/LiveRecall
Length of output: 2677
🏁 Script executed:
rg -n "def " core/ocr.py --type pyRepository: VedankPurohit/LiveRecall
Length of output: 1066
Remove reference to non-existent method from documentation.
Line 27 references ocr_service.recompute_all_ocr(), but this method does not exist in the OCRService class. Either implement this method or update the documentation to reflect the actual API for switching OCR providers (e.g., using get_provider() or another existing method).
🤖 Prompt for AI Agents
In @core/ocr.py around lines 25 - 28, The docs incorrectly reference a
non-existent OCRService.recompute_all_ocr(); either remove that line or replace
it with the actual API call(s): update the example to use the existing
OCRService.get_provider() (to inspect current provider) and the appropriate
method to change providers (e.g., a setter or configuration reload routine), or
implement a new OCRService.recompute_all_ocr() that re-runs OCR for stored
items; ensure you reference the OCRService class and the chosen method name
(get_provider() or recompute_all_ocr()) consistently in the doc comment.
Follow codebase conventions by caching the OCR class at module level instead of instance level. Adds proper type hints for _get_ocr_class().
The database returns BM25 score as 'relevance_score' but code was looking for 'fts_rank', causing text_fuzzy search similarity to always default to fallback values.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @core/ocr_providers/apple_vision.py:
- Around line 124-129: Remove the redundant local import "import platform as
plat" in core/ocr_providers/apple_vision.py and update the small info block to
use the already-imported platform module (replace plat.mac_ver and plat.system
with platform.mac_ver and platform.system), so only the top-level platform
import is used and the duplicate alias is removed.
🧹 Nitpick comments (3)
core/ocr_providers/apple_vision.py (3)
55-55: Consider adding a type hint to the class attribute.For consistency and clarity, consider adding a type annotation:
✨ Suggested enhancement
- name = "apple_vision" + name: str = "apple_vision"
73-73: Prefer lowercase variable name following PEP 8.The variable
OCRuses all-caps naming which is typically reserved for constants. Consider using a lowercase name likeocr_classfor better readability.♻️ Proposed refactor
- OCR = _get_ocr_class() + ocr_class = _get_ocr_class() try: # ocrmac returns list of (text, confidence, bbox) tuples - results = OCR(str(image_path)).recognize() + results = ocr_class(str(image_path)).recognize()
106-106: Use the logging module instead of print().For library/service code, prefer the
loggingmodule overprint()to allow proper log level control and integration with application logging.♻️ Proposed refactor
Add at the top of the file with other imports:
import logging logger = logging.getLogger(__name__)Then replace the print statement:
- print(f"Apple Vision OCR error: {e}") + logger.exception("Apple Vision OCR error: %s", e)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/ocr_providers/apple_vision.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use type hints for function parameters and return types
Use double quotes for strings in Python code
Use private attributes with_leading_underscorenaming convention
Files:
core/ocr_providers/apple_vision.py
core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Services should use global singleton instances at module level
Files:
core/ocr_providers/apple_vision.py
🧬 Code graph analysis (1)
core/ocr_providers/apple_vision.py (1)
core/ocr.py (5)
OCRResult(42-53)extract_text(67-69)extract_text(152-164)extract_text(221-223)empty(51-53)
🔇 Additional comments (3)
core/ocr_providers/apple_vision.py (3)
1-28: LGTM!The module-level singleton pattern and type hints align well with the coding guidelines. The docstring clearly explains the provider's purpose and requirements.
31-41: LGTM!The lazy-loading singleton pattern is correctly implemented with proper type hints and helpful error messaging.
110-120: LGTM!The availability check correctly validates both the platform and library dependency.
| import platform as plat | ||
|
|
||
| return { | ||
| "engine": "Apple Vision Framework", | ||
| "platform": "macOS", | ||
| "macos_version": plat.mac_ver()[0] if plat.system() == "Darwin" else None, |
There was a problem hiding this comment.
Remove redundant import.
The platform module is already imported at line 18. The redundant import with alias is unnecessary.
🔧 Proposed fix
def get_model_info(self) -> dict:
"""Get information about the Vision framework"""
- import platform as plat
return {
"engine": "Apple Vision Framework",
"platform": "macOS",
- "macos_version": plat.mac_ver()[0] if plat.system() == "Darwin" else None,
+ "macos_version": platform.mac_ver()[0] if platform.system() == "Darwin" else None,
"requires_download": False,
"memory_overhead": "minimal (uses system APIs)",
"supported_languages": ["en", "zh", "ja", "ko", "de", "fr", "it", "pt", "es"],
}🤖 Prompt for AI Agents
In @core/ocr_providers/apple_vision.py around lines 124 - 129, Remove the
redundant local import "import platform as plat" in
core/ocr_providers/apple_vision.py and update the small info block to use the
already-imported platform module (replace plat.mac_ver and plat.system with
platform.mac_ver and platform.system), so only the top-level platform import is
used and the duplicate alias is removed.
- Use SearchMode enum members instead of string comparisons in search.py - Add thread-safe double-checked locking to OCRService._provider_cache - Add thread-safe double-checked locking to _get_ocr_class() singleton - Remove unnecessary parentheses in setup page conditional
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @web/src/app/setup/page.tsx:
- Around line 378-390: The Continue button's onClick uses only
status?.migration_status?.needs_migration, causing inconsistent navigation
versus determineStep; update the handler to use the same logic as determineStep
(or call determineStep directly) so it also checks
eventStatus?.ocr_stats?.pending > 0 and any other conditions determineStep uses,
then call setStep('migration') or handleCompleteSetup() based on that result
(referencing determineStep, status?.migration_status?.needs_migration,
eventStatus?.ocr_stats?.pending, setStep, and handleCompleteSetup).
🧹 Nitpick comments (7)
api/routes/search.py (1)
189-216:quick_searchdefaults toautomode but doesn't exposesearch_modeparameter.The
quick_searchendpoint doesn't accept asearch_modeparameter, so it always defaults to theSearchRequestdefault (which isauto). This may be intentional for simplicity, but users cannot use quick search with other modes.Consider whether this is the desired behavior. If so, documenting this in the docstring would help.
💡 Optional: Add search_mode parameter
@router.get("/quick", response_model=SearchResponse) async def quick_search( q: str, limit: int = 20, safe_mode: bool = False, start_date: str = None, end_date: str = None, visibility: VisibilityFilter = VisibilityFilter.VISIBLE_ONLY, + search_mode: SearchMode = SearchMode.AUTO, ): """ Quick search with query parameters (simpler than POST). Safe mode is OFF by default for personal recall apps. Enable it to filter potentially sensitive content from results. Example: /api/v1/search/quick?q=blue+shirt&limit=10 Example with dates: /api/v1/search/quick?q=meeting&start_date=251201000000&end_date=251206235959 Example with visibility: /api/v1/search/quick?q=meeting&visibility=all + Example with mode: /api/v1/search/quick?q=error&search_mode=text_fuzzy """ request = SearchRequest( query=q, limit=limit, safe_mode=safe_mode, start_date=start_date, end_date=end_date, visibility=visibility, + search_mode=search_mode, ) return await search_screenshots(request)core/ocr_providers/apple_vision.py (2)
114-117: Consider using structured logging instead of print statements.Using
print()for error logging is not ideal for production code. Consider using theloggingmodule for better observability and log level control.💡 Suggested improvement
+import logging + +logger = logging.getLogger(__name__) + # ... in extract_text method ... except Exception as e: - print(f"Apple Vision OCR error: {e}") + logger.warning("Apple Vision OCR error: %s", e) # Return empty result on error rather than raising return OCRResult.empty()
131-142: Redundant platform import inget_model_info.
platformis already imported at the module level (line 18), but this method imports it again asplat. Use the existing module-level import for consistency.♻️ Suggested fix
def get_model_info(self) -> dict: """Get information about the Vision framework""" - import platform as plat - return { "engine": "Apple Vision Framework", "platform": "macOS", - "macos_version": plat.mac_ver()[0] if plat.system() == "Darwin" else None, + "macos_version": platform.mac_ver()[0] if platform.system() == "Darwin" else None, "requires_download": False, "memory_overhead": "minimal (uses system APIs)", "supported_languages": ["en", "zh", "ja", "ko", "de", "fr", "it", "pt", "es"], }core/database.py (1)
1316-1336:reset_all_ocrshould also clear the FTS index explicitly.The method deletes from
screenshot_ocr, which triggersocr_fts_deletefor each row. However, this trigger-based deletion may be slower than a direct FTS rebuild. Consider adding an explicit FTS rebuild command for performance on large datasets.💡 Optional: Rebuild FTS after bulk delete
def reset_all_ocr(self) -> int: """Reset all OCR data for reprocessing with new provider. Returns: Number of screenshots that will need reprocessing """ with self.cursor() as cur: # Delete all text embeddings cur.execute("DELETE FROM ocr_text_embeddings") # Delete all chunks cur.execute("DELETE FROM ocr_text_chunks") # Delete all OCR records cur.execute("DELETE FROM screenshot_ocr") + # Rebuild FTS index (faster than individual trigger deletions) + cur.execute("INSERT INTO ocr_text_fts(ocr_text_fts) VALUES('rebuild')") + # Reset all has_ocr flags cur.execute("UPDATE screenshots SET has_ocr = 0") count = cur.rowcount return countcore/ocr.py (1)
204-225: Platform check before importing Apple Vision is good but could mask import errors.The
try/except ImportErrorblocks silently swallow import errors, which is fine for optional dependencies but could hide actual issues (e.g., broken module incore/ocr_providers/).Consider logging at DEBUG level when providers fail to import to aid troubleshooting.
💡 Optional: Add debug logging for failed imports
+import logging + +logger = logging.getLogger(__name__) + def _register_providers(): """Register available OCR providers""" # Import and register Apple Vision provider (macOS only) if platform.system() == "Darwin": try: from core.ocr_providers.apple_vision import AppleVisionOCR OCRService.register("apple_vision", AppleVisionOCR) - except ImportError: - pass # ocrmac not installed + except ImportError as e: + logger.debug("Apple Vision OCR not available: %s", e) # Import and register Tesseract provider (cross-platform) try: from core.ocr_providers.tesseract import TesseractOCR OCRService.register("tesseract", TesseractOCR) - except ImportError: - pass # pytesseract not installed + except ImportError as e: + logger.debug("Tesseract OCR not available: %s", e)web/src/app/setup/page.tsx (2)
225-230: Consider extracting migration progress calculation for clarity.The nested ternary expression correctly guards against division by zero, but the logic is dense and could be more readable if extracted into a helper function or
useMemohook.♻️ Proposed refactor
+ // Calculate migration progress (guard against division by zero) + const migrationProgress = useMemo(() => { + if (eventStatus?.ocr_stats) { + const { completed, pending } = eventStatus.ocr_stats; + const total = completed + pending; + return total > 0 ? (completed / total) * 100 : 100; + } + return status?.migration_status?.progress_percent ?? 0; + }, [eventStatus?.ocr_stats, status?.migration_status?.progress_percent]); - const migrationProgress = eventStatus?.ocr_stats - ? (eventStatus.ocr_stats.completed + eventStatus.ocr_stats.pending > 0 - ? (eventStatus.ocr_stats.completed / (eventStatus.ocr_stats.completed + eventStatus.ocr_stats.pending)) * 100 - : 100) - : status?.migration_status?.progress_percent ?? 0;
333-342: Consider checking both models for the download spinner.The spinner and "Downloading models automatically..." message only displays when CLIP is not ready. If CLIP completes but the Text Embedding model is still downloading, the spinner disappears even though downloads are ongoing. Consider checking both model statuses for a more accurate indicator.
♻️ Proposed fix
<div className="text-center"> - {!(status?.clip_status === 'ready' || eventStatus?.clip?.downloaded) && ( + {(!(status?.clip_status === 'ready' || eventStatus?.clip?.downloaded) || + !(status?.text_embedding_status === 'ready' || eventStatus?.text_embedding?.downloaded)) && ( <> <div className="w-6 h-6 border-2 border-[#86efac]/30 border-t-[#86efac] rounded-full animate-spin mx-auto mb-2" /> <p className="text-xs text-[#555]"> Downloading models automatically... </p> </> )} </div>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
api/routes/search.pycore/database.pycore/ocr.pycore/ocr_providers/apple_vision.pyweb/src/app/setup/page.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use type hints for function parameters and return types
Use double quotes for strings in Python code
Use private attributes with_leading_underscorenaming convention
Files:
api/routes/search.pycore/ocr_providers/apple_vision.pycore/database.pycore/ocr.py
api/routes/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
api/routes/**/*.py: FastAPI routes should use APIRouter with prefix and tags parameters
FastAPI route handlers should include docstrings and specify response_model
Use async/await syntax for FastAPI route handlers
Files:
api/routes/search.py
web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web/src/**/*.{ts,tsx}: Use functional components with hooks in React code
Use Tailwind CSS for styling in React components
Files:
web/src/app/setup/page.tsx
core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Services should use global singleton instances at module level
Files:
core/ocr_providers/apple_vision.pycore/database.pycore/ocr.py
core/database.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use SQLite with sqlite-vec for vector similarity search in database operations
Files:
core/database.py
🧠 Learnings (1)
📚 Learning: 2026-01-11T06:00:24.323Z
Learnt from: CR
Repo: VedankPurohit/LiveRecall PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T06:00:24.323Z
Learning: Applies to core/database.py : Use SQLite with sqlite-vec for vector similarity search in database operations
Applied to files:
core/database.py
🧬 Code graph analysis (4)
api/routes/search.py (4)
api/schemas.py (1)
SearchMode(168-174)web/src/types/index.ts (1)
SearchMode(143-143)core/embeddings.py (3)
get_safe_search_embedding(275-283)get_combined_embedding(215-248)get_text_embedding(200-212)core/database.py (4)
search_similar(607-675)search_ocr_fts(869-916)search_text_embeddings(967-1051)search_hybrid(1053-1209)
web/src/app/setup/page.tsx (2)
api/schemas.py (3)
SetupStatus(477-484)EnhancedSetupStatus(744-766)MigrationStatus(718-741)core/processor.py (1)
SyncProgress(31-63)
core/database.py (4)
core/processor.py (1)
start(128-137)tray/app.py (1)
start(150-193)core/compression.py (1)
start(52-75)tray/backend.py (1)
start(66-116)
core/ocr.py (3)
api/schemas.py (1)
OCRResult(512-518)core/ocr_providers/apple_vision.py (4)
extract_text(66-117)is_available(119-129)get_model_info(131-142)AppleVisionOCR(53-142)core/ocr_providers/tesseract.py (4)
extract_text(58-111)is_available(113-123)get_model_info(125-147)TesseractOCR(31-156)
🔇 Additional comments (26)
api/routes/search.py (3)
35-52: LGTM!The endpoint is well-structured with proper
response_model, async syntax, and comprehensive docstring covering all search modes. The examples are helpful for API consumers.
61-93: LGTM!Good implementation of mode-based embedding generation. The lazy import of
text_embedding_serviceat line 85 avoids loading the BGE model when not needed, which is a smart optimization for memory usage.
99-148: LGTM!Clean mode-based branching with proper enum comparisons using
SearchModemembers. Each mode correctly normalizes its results with appropriatematch_sourcesannotations.core/ocr_providers/apple_vision.py (2)
33-50: LGTM!Thread-safe double-checked locking pattern is correctly implemented for lazy loading the OCR class. The error message provides helpful installation instructions.
66-117: LGTM!The
extract_textmethod handles edge cases well:
- File existence validation
- Empty results return
OCRResult.empty()- Graceful error handling with fallback to empty result
- Proper confidence averaging and word counting
core/database.py (9)
61-75: LGTM!Thread-safe cursor context manager with proper locking, commit on success, and rollback on exception. This ensures safe concurrent access from multiple threads (e.g., background OCR processing).
213-263: LGTM!Good FTS5 setup with:
- Graceful fallback from
trigramtokenizer to default if SQLite < 3.34- Content-sync triggers to keep FTS index consistent with
screenshot_ocrtable- Proper use of sqlite-vec for text embeddings with correct dimensions
584-588: LGTM!New utility method
get_screenshot_count()added for total screenshot count. Follows existing patterns in the codebase.
869-916: LGTM!FTS5 search implementation is solid:
- Uses BM25 scoring via
bm25(ocr_text_fts)- Snippet generation with
<mark>highlighting- Visibility filtering with safe parameterized condition construction
990-1028: Good SQL injection prevention forchunk_size.The explicit validation of
chunk_sizeagainst allowed values ("small","large") before using it in the query is the correct approach for preventing SQL injection when dynamically building queries.
1053-1084: LGTM!The hybrid search method is well-documented with clear explanation of the RRF formula and parameters. The mode-based method selection is clean.
1147-1163: LGTM!RRF implementation is correct: accumulates
1 / (k + rank)contributions from each source and tracks match sources per screenshot. Standard RRF with k=60 is a good default.
1177-1190: Potential issue: Visibility condition already applied in source searches.The hybrid search fetches results from
search_similar,search_ocr_fts, andsearch_text_embeddings, which all apply the visibility filter. Then, visibility is applied again when fetching full screenshot data (lines 1170-1175, 1184).This is correct and defensive (ensures no hidden results leak), but there's a subtle issue: if a screenshot's visibility changes between the individual searches and this final fetch, it could be filtered out unexpectedly. This is a minor edge case given the short time window.
The double-filtering is harmless but worth confirming is intentional for defense-in-depth.
1211-1256: LGTM!The
_create_snippethelper provides a clean fallback for snippet generation when FTS5's built-insnippet()isn't available (e.g., for semantic search results). The markdown bold highlighting (**...**) is appropriate for the UI context.core/ocr.py (6)
42-54: LGTM!Clean dataclass with sensible defaults and a factory method for empty results. The separation from the Pydantic model in
api/schemas.pyis appropriate (internal vs API boundary).
57-78: LGTM!Using
@runtime_checkablewithProtocolis the correct approach for structural subtyping, allowing provider classes to implement the interface without explicit inheritance.
89-99: Class-level_providersdict is shared across all instances.The
_providersclass variable is shared across allOCRServiceinstances, which is fine for the registry pattern. However, combining this with instance-level_provider_cachecreates an interesting design where:
- Registry (
_providers) is global/class-level- Cache (
_provider_cache) is per-instanceSince there's a global singleton
ocr_service, this works correctly. Just noting that creating multipleOCRServiceinstances would have isolated caches but shared registries.
106-139: LGTM!Well-implemented double-checked locking pattern for thread-safe provider instantiation. The pattern correctly:
- Checks cache without lock (fast path)
- Acquires lock and re-checks (prevents double initialization)
- Validates availability before caching
141-158: LGTM!Good platform-aware provider selection with sensible fallback chain:
- Apple Vision on macOS
- Tesseract on Windows (or as fallback)
- First registered provider as last resort
228-241: LGTM!Convenience wrapper functions delegate to the global
ocr_servicesingleton, providing a clean module-level API. This follows the codebase convention of global singleton instances at module level as per coding guidelines.web/src/app/setup/page.tsx (6)
10-53: LGTM! Well-structured type definitions.The type definitions correctly mirror the backend API schemas and provide strong type safety for the component. The separation of concerns between SetupStatus, EnhancedSetupStatus, MigrationStatus, SyncProgress, and EventStatus is clear and maintainable.
59-91: LGTM! Proper use of hooks and error handling.The state management and fetch callbacks follow React best practices. The use of
useCallbackwith appropriate dependencies ensures stable function references, and error handling gracefully degrades when the backend is unavailable.
94-119: LGTM! Robust step determination logic.The
determineStepfunction correctly combines data from both status sources (enhanced-status and events/status endpoints) to reliably determine the current setup step. The use of optional chaining properly handles null eventStatus, and the fallback tostatus.clip_status === 'ready'when eventStatus is unavailable is appropriate.
122-196: LGTM! Well-implemented effects with proper cleanup and dependencies.All useEffect hooks follow React best practices:
- Initial load effect provides clear error feedback
- Polling effect properly cleans up intervals and has correct dependencies
- Auto-trigger effects use flags to prevent duplicate API calls
- Dependencies arrays are complete and accurate
395-467: LGTM! Clear and informative migration progress UI.The migration step provides excellent user feedback with:
- Real-time progress bar synchronized with backend events
- Clear stats showing processed vs remaining items
- Phase-specific status messages during sync
- Estimated time remaining with proper guards against edge cases
- Flexibility to continue in background
The estimated time calculation (line 453) appropriately assumes linear progress and is protected by the conditional on line 451.
469-506: LGTM! Clean completion state and helpful progress indicators.The completion step provides clear success feedback with a call-to-action link. The step indicators at the bottom effectively communicate setup progress through the multi-step flow.
| <button | ||
| onClick={handleCompleteSetup} | ||
| className="flex-1 py-3 px-4 bg-[#86efac] text-black rounded font-medium hover:bg-[#86efac]/90 transition-colors" | ||
| onClick={() => { | ||
| // Check if we should go to migration or complete | ||
| if (status?.migration_status?.needs_migration) { | ||
| setStep('migration'); | ||
| } else { | ||
| handleCompleteSetup(); | ||
| } | ||
| }} | ||
| className="flex-1 py-3 px-4 border border-[#333] text-[#8a8a8a] rounded font-medium hover:bg-[#1e1e1e] transition-colors" | ||
| > | ||
| Done | ||
| Continue | ||
| </button> |
There was a problem hiding this comment.
Inconsistent step navigation logic compared to determineStep.
The inline handler only checks status?.migration_status?.needs_migration, but the determineStep function also checks eventStatus?.ocr_stats?.pending > 0. This could cause the Continue button to skip the migration step when OCR processing is pending but the migration_status hasn't updated yet.
🔧 Proposed fix for consistent navigation
<button
onClick={() => {
- // Check if we should go to migration or complete
- if (status?.migration_status?.needs_migration) {
- setStep('migration');
- } else {
- handleCompleteSetup();
- }
+ // Use determineStep for consistent navigation
+ const nextStep = determineStep(status!, eventStatus);
+ if (nextStep === 'migration') {
+ setStep('migration');
+ } else {
+ handleCompleteSetup();
+ }
}}
className="flex-1 py-3 px-4 border border-[#333] text-[#8a8a8a] rounded font-medium hover:bg-[#1e1e1e] transition-colors"
>
Continue
</button>📝 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.
| <button | |
| onClick={handleCompleteSetup} | |
| className="flex-1 py-3 px-4 bg-[#86efac] text-black rounded font-medium hover:bg-[#86efac]/90 transition-colors" | |
| onClick={() => { | |
| // Check if we should go to migration or complete | |
| if (status?.migration_status?.needs_migration) { | |
| setStep('migration'); | |
| } else { | |
| handleCompleteSetup(); | |
| } | |
| }} | |
| className="flex-1 py-3 px-4 border border-[#333] text-[#8a8a8a] rounded font-medium hover:bg-[#1e1e1e] transition-colors" | |
| > | |
| Done | |
| Continue | |
| </button> | |
| <button | |
| onClick={() => { | |
| // Use determineStep for consistent navigation | |
| const nextStep = determineStep(status!, eventStatus); | |
| if (nextStep === 'migration') { | |
| setStep('migration'); | |
| } else { | |
| handleCompleteSetup(); | |
| } | |
| }} | |
| className="flex-1 py-3 px-4 border border-[#333] text-[#8a8a8a] rounded font-medium hover:bg-[#1e1e1e] transition-colors" | |
| > | |
| Continue | |
| </button> |
🤖 Prompt for AI Agents
In @web/src/app/setup/page.tsx around lines 378 - 390, The Continue button's
onClick uses only status?.migration_status?.needs_migration, causing
inconsistent navigation versus determineStep; update the handler to use the same
logic as determineStep (or call determineStep directly) so it also checks
eventStatus?.ocr_stats?.pending > 0 and any other conditions determineStep uses,
then call setStep('migration') or handleCompleteSetup() based on that result
(referencing determineStep, status?.migration_status?.needs_migration,
eventStatus?.ocr_stats?.pending, setStep, and handleCompleteSetup).
1 participant
Summary
This PR adds OCR (Optical Character Recognition) text extraction capability to LiveRecall, enabling text-based search alongside the existing CLIP image search. The feature extracts text from screenshots and provides multiple search methods.
Key Features
Technical Changes
Files Changed
core/ocr.pycore/ocr_providers/core/text_embeddings.pycore/chunking.pycore/database.pycore/processor.pyapi/routes/web/src/app/Testing
uv run pytestuv run python scripts/benchmark_ocr.pyTest plan
Summary by CodeRabbit
New Features
API
UI
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.