feat: wire up on-disk OCR cache#4
Conversation
Cache OCR results as JSON under ~/.cache/quickview/ocr/, keyed by blake3(path + lang + mtime + size) so edited files are simply misses. Revises ADR-0009's in-memory-first decision: Quick Preview spawns a fresh process per invocation, so only a persistent cache helps it. - cache.rs: load_ocr/store_ocr taking an explicit cache root (testable without ProjectDirs); atomic temp-file + rename writes since concurrent QuickView processes are a designed use case - start_ocr: cache check/write inside the existing worker-thread closure — main thread never touches disk, and hits flow through the same channel so the OCR job-id guard applies unchanged - empty results are cached (text-free images don't re-run tesseract); failures are not, so transient errors retry on next open - no eviction in v1 (entries are a few KB); documented in ADR-0009 implementation notes along with the future settings-in-key rule - tests: key sensitivity (lang/path/size/mtime), round-trip, corrupt and absent entries as misses
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughImplements an on-disk OCR cache keyed by path, language, mtime, and size. The cache module now resolves entry paths from an explicit root and can load/store JSON cache files atomically. The OCR worker consults the cache before Tesseract, and docs were updated. ChangesOn-disk OCR cache
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ViewerController
participant CacheModule
participant Filesystem
participant Tesseract
ViewerController->>CacheModule: load_ocr(cache_dir, file, lang)
CacheModule->>Filesystem: read <cache_dir>/ocr/<key>.json
alt cache hit
Filesystem-->>CacheModule: cached JSON
CacheModule-->>ViewerController: OcrResult
else cache miss
Filesystem-->>CacheModule: missing/error
CacheModule-->>ViewerController: None
ViewerController->>Tesseract: run_tesseract_tsv
Tesseract-->>ViewerController: TSV output
ViewerController->>ViewerController: parse_tesseract_tsv
ViewerController->>CacheModule: store_ocr(cache_dir, file, lang, result)
CacheModule->>Filesystem: write temp file, rename
end
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d492353b83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// incompatible older schema) is treated as a miss: OCR re-runs and the entry | ||
| /// is overwritten. | ||
| pub fn load_ocr(cache_root: &Path, file: &Path, lang: &str) -> Option<OcrResult> { | ||
| let path = ocr_cache_path(cache_root, file, lang); |
There was a problem hiding this comment.
This lookup trusts ocr_cache_path, but that key only hashes modified().as_secs() plus the file size. In workflows that rewrite the same image path within the same second with an unchanged byte length (common for rapid screenshot/editor saves), the cache key stays identical and this new on-disk cache can return OCR from the previous contents instead of re-running Tesseract. Include subsecond mtime data (or another content/version check) before treating the entry as a hit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — fixed in f886e19: the key now hashes the full nanosecond mtime (as_nanos()), so a same-second rewrite with an unchanged byte length produces a new key. Added a test asserting a 1ns mtime change (same second, same size) changes the key.
| // re-run tesseract); failures are not, so transient errors | ||
| // retry on the next open. | ||
| if let Some(root) = &cache_root { | ||
| if let Err(err) = cache::store_ocr(root, &path, &lang, &parsed) { |
There was a problem hiding this comment.
Because store_ocr recomputes the cache path from the file's current metadata after Tesseract has already read and parsed the image, an edit that lands while OCR is running can write OCR for the old contents under the new file's metadata key. Subsequent opens of the edited image then get a cache hit with stale word boxes/text. Snapshot the metadata/key before running Tesseract and either store under that key or skip the store if the file changed during OCR.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — fixed in f886e19: the cache key is now snapshotted before tesseract runs (load_ocr/store_ocr take the entry path derived once up front). A file edited mid-OCR stores its stale result under the old key, which the edited file then correctly misses.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/quickview-core/src/cache.rs`:
- Around line 63-83: The temp file cleanup in store_ocr only handles the rename
failure path, so a failed std::fs::write can leave an orphaned json.tmp-* file
behind. Update store_ocr to remove the temp file whenever the write step fails
as well, using the existing tmp path and the same cleanup behavior already used
after std::fs::rename errors.
In `@crates/quickview-ui/src/windows/shared.rs`:
- Around line 227-247: The OCR cache write path in store_ocr still uses a shared
json.tmp-<pid> temp file, so concurrent OCR jobs can overwrite each other and
lose a cache write. Update the cache write flow called from shared.rs to
generate a unique temp file per store_ocr call (or otherwise serialize writes)
while keeping the existing cache key behavior, and use the unique temp path
during the atomic replace step so overlapping jobs cannot clobber one another.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba828bd0-a69e-4a1d-a3d2-945037a2588a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.claude/CLAUDE.mdadrs/ADR-0009-Caching.mdcrates/quickview-core/Cargo.tomlcrates/quickview-core/src/cache.rscrates/quickview-ui/src/windows/shared.rsdocs/DECISIONS.mddocs/DEPENDENCIES.mddocs/PHASED_PLAN.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use rustfmt and clippy for code formatting and linting, as tracked in rust-toolchain.toml
Run clippy with all targets and features enabled with -D warnings flag to treat warnings as errors
Files:
crates/quickview-ui/src/windows/shared.rscrates/quickview-core/src/cache.rs
🔇 Additional comments (10)
crates/quickview-core/src/cache.rs (4)
1-14: LGTM!
24-45: LGTM!Explicit
cache_rootparameterization is a clean improvement for testability, and the hash inputs (path, lang, mtime, size) reasonably invalidate on file changes.
47-56: LGTM!
85-187: 📐 Maintainability & Code QualityNo MSRV issue here The crate targets Rust 1.83 and the pinned toolchain is stable, so
Option::is_none_oris available here.> Likely an incorrect or invalid review comment.crates/quickview-core/Cargo.toml (1)
14-20: LGTM!adrs/ADR-0009-Caching.md (1)
38-62: LGTM!docs/DECISIONS.md (1)
32-32: LGTM!Also applies to: 132-140
docs/DEPENDENCIES.md (1)
37-37: LGTM!docs/PHASED_PLAN.md (1)
160-164: LGTM!.claude/CLAUDE.md (1)
54-61: LGTM!
- hash nanosecond mtime: whole seconds aliased a same-second rewrite of the same path with an unchanged byte length (Codex) - snapshot the cache key before tesseract runs: a file edited mid-OCR now stores its stale result under the old key, which the edited file correctly misses, instead of under the new metadata's key (Codex); load_ocr/store_ocr now take the entry path derived once up front - unique temp file per store (pid + per-process counter) so concurrent same-key writes cannot clobber each other's temp file (CodeRabbit) - remove the temp file when the write itself fails, not just the rename — v1 has no eviction pass to sweep orphans (CodeRabbit) - test: subsecond mtime change (same second, same size) changes the key
There was a problem hiding this comment.
💡 Codex Review
Include the cwd in relative cache keys
When the viewer is launched with a relative path, this hashes only the literal path string. I checked crates/quickview/src/main.rs, and CLI/stdin input is stored as PathBuf::from(...) without canonicalization, so quickview image.png run from two different directories can reuse the same cache entry when the two files also share lang, size, and mtime, causing the second image to load OCR boxes/text from the first. Canonicalize or otherwise make the keyed path absolute before consulting the on-disk cache.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The OCR cache key hashes the literal path string, so a relative
invocation ('quickview image.png') keyed on the cwd-dependent spelling:
two different files could collide on one entry (given equal lang, size,
and mtime), and one file opened via different spellings fragmented into
several. Relative paths also broke directory navigation, since
"image.png".parent() is empty.
Canonicalize once in resolve_input_path so every downstream consumer
sees one absolute, symlink-resolved form. Missing files fall through
unchanged, preserving the graceful load-failed state.
|
Re: Codex's relative-path cache-key finding on |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4c327d673
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| static WRITE_SEQ: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); | ||
| let seq = WRITE_SEQ.fetch_add(1, std::sync::atomic::Ordering::Relaxed); | ||
| let tmp = entry.with_extension(format!("json.tmp-{}-{seq}", std::process::id())); | ||
| if let Err(err) = std::fs::write(&tmp, &json) { |
There was a problem hiding this comment.
With a normal 022 umask, std::fs::write creates this temp/cache file as 0644 before it is renamed. In environments where $XDG_CACHE_HOME or the home directory is traversable (shared machines or custom cache roots), the new persistent cache exposes recognized text from screenshots/images to other local users. Please create the temp file with private permissions (for example 0600, and ideally a 0700 cache dir) before writing/renaming.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — fixed in 4d22e87. Confirmed empirically first: entries were landing 0644 in 0755 dirs under umask 022, relying on the home directory for privacy — and this cache is the first place QuickView persists OCR text, so private-by-default is the right posture. Temp files are now created with mode 0600 via OpenOptionsExt (create_new; the rename preserves the mode on the final entry) and cache directories with mode 0700 via DirBuilderExt. Creation-time only — pre-existing entries keep their old modes; ADR-0009 documents that clearing ~/.cache/quickview/ocr resets everything. Verified on disk: fresh run yields a 0700 dir and 0600 entry, and the round-trip test now asserts both modes.
Cache entries hold recognized text from the user's images (screenshots often contain emails, tokens, passwords) and are the first place QuickView persists OCR text; with umask 022 they landed world-readable (0644 in 0755 dirs), relying on the home directory for privacy. Create the temp file with mode 0600 (rename preserves it) and cache directories with mode 0700. Creation-time only; pre-existing entries keep their modes — documented in ADR-0009 with the cache-clear reset.
1 participant
Summary
Wires up the previously-unused
cache.rsmodule as an on-disk OCR cache: results are stored as JSON under~/.cache/quickview/ocr/, keyed by blake3 ofpath + lang + mtime + size. Closes out the Phase 7 caching task ("re-opening the same image is faster due to caching").This revises ADR-0009's "in-memory first" decision (rationale recorded in new implementation notes in the ADR): Quick Preview spawns a fresh process per invocation, so only a persistent cache helps the flagship workflow — and it also covers in-session arrow-key revisits, since re-reading a few-KB JSON is effectively instant.
Design points
rm -rf ~/.cache/quickview/ocr. Real eviction arrives with Phase 8's SQLite cache. Tesseract is currently invoked with no psm/oem flags, solangis the only setting and it is in the key; the ADR notes that future configurable settings must join the key.Testing
cache.rs: key sensitivity (lang/path/size/mtime each change the key), store/load round-trip (incl. no leftover temp files), corrupt entry → miss, absent entry → miss.cargo fmt/clippy --all-targets --all-features -D warnings/cargo test --allclean.OCR cache hitwith the correct text;touchon the image causes a miss + second entry;--quick-previewin a fresh process gets the hit.