feat: async + sandboxed image loading via glycin with GDK fallback#3
Conversation
Replace the synchronous Texture::from_file in load_file with async decoding off the main thread (implements ADR-0004, NFR-001/NFR-002): - new decode module: glycin sandboxed loader process when installed, in-process GDK decoding on a worker thread otherwise - backend chosen once per session by probing installed glycin loaders; choice is logged at first decode - fallback is strictly session-wide, never per-file: a file glycin rejects is a failed load and is never re-fed to the unsandboxed GDK decoder - decode_job_id guard (same monotonic pattern as OCR) drops stale decodes during fast navigation; FileInfo emission and OCR kickoff move into the finish_decode completion handler - previous image stays visible under the busy spinner until the new decode completes; spinner carries through into OCR - remove the unused glycin cargo feature (crate is pure Rust and now always compiled) - packaging: add glycin-loaders module to the Flatpak manifest (GNOME runtime does not ship it); update PKGBUILD optdepends - docs: ADR-0004 implementation notes, PHASED_PLAN Phase 7, CLAUDE.md current state, README/DEPENDENCIES/DECISIONS wording
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
Summary by CodeRabbit
WalkthroughMakes glycin always compiled, adds a session-cached decode backend with a GDK fallback, wires async decoding into the viewer with job-id gating, and updates docs, packaging, CI, and advisory settings to match the new decode path. ChangesAsync decode backend and viewer flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 49243c29bf
ℹ️ 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".
| glycin = { version = "3", optional = true } | ||
| # Sandboxed image decoding. Always compiled (pure Rust client, no system build | ||
| # deps); whether it is used is decided at runtime by probing installed loaders. | ||
| glycin = { version = "3", features = ["gdk4"] } |
There was a problem hiding this comment.
Making glycin unconditional means clean source/package builds now need glycin's native libraries available at build time, before the runtime GDK fallback can ever run. In environments that have the existing GTK/Tesseract setup but not packages such as libseccomp/lcms2/fontconfig development files, cargo build can fail even though the docs and setup paths still present glycin as optional; please either add those native dependencies to the setup/package recipes or keep this behind a feature.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified and fixed in b8e81aa — cargo tree confirms the glycin client links libseccomp, lcms2, and fontconfig (via libseccomp-sys, lcms2-sys, yeslogic-fontconfig-sys), needed at build/link time regardless of which backend runs. They are now declared in the PKGBUILD depends, the README install line, and DEPENDENCIES.md (with the Debian/Ubuntu -dev package names), and the inaccurate "pure Rust, no system deps" wording was corrected in Cargo.toml and ADR-0004. Keeping the crate unconditional rather than feature-gated: these libraries are present on effectively every GTK-capable system, and a single build recipe avoids per-distro variants.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/quickview-ui/src/decode.rs (1)
77-93: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffUnbounded thread spawning per decode with no cancellation.
Each
decode_gdkcall spawns a fresh OS thread that keeps running to completion even if the caller's future is dropped (e.g. via thedecode_job_idguard on a superseded load). Rapid navigation through many images could pile up several concurrent decode threads doing wasted work. Consider a bounded worker pool (or check the job id before/while decoding) to cap concurrent decode threads.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/quickview-ui/src/decode.rs` around lines 77 - 93, The decode_gdk helper currently spawns a new OS thread for every decode and never cancels it, so superseded loads can still consume work. Update decode_gdk to avoid per-call thread spawning by using a bounded worker pool or another shared execution path, and wire in cancellation/job validation so a dropped caller future (such as one invalidated by decode_job_id) can stop or skip work. Keep the fix localized around decode_gdk and its caller-side job guard so concurrent image decodes are capped.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/quickview-ui/src/decode.rs`:
- Around line 77-93: The decode_gdk helper currently spawns a new OS thread for
every decode and never cancels it, so superseded loads can still consume work.
Update decode_gdk to avoid per-call thread spawning by using a bounded worker
pool or another shared execution path, and wire in cancellation/job validation
so a dropped caller future (such as one invalidated by decode_job_id) can stop
or skip work. Keep the fix localized around decode_gdk and its caller-side job
guard so concurrent image decodes are capped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b091e9cc-b57a-43a5-b06d-5e41a363c022
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.claude/CLAUDE.mdREADME.mdadrs/ADR-0004-Image-Decoding.mdcrates/quickview-ui/Cargo.tomlcrates/quickview-ui/src/decode.rscrates/quickview-ui/src/lib.rscrates/quickview-ui/src/windows/shared.rsdocs/DECISIONS.mddocs/DEPENDENCIES.mddocs/PHASED_PLAN.mdpackaging/arch/PKGBUILDpackaging/flatpak/com.example.QuickView.json
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: build-test
- GitHub Check: lint
🧰 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/lib.rscrates/quickview-ui/src/decode.rscrates/quickview-ui/src/windows/shared.rs
🔇 Additional comments (12)
crates/quickview-ui/Cargo.toml (1)
7-8: LGTM!Also applies to: 21-23
crates/quickview-ui/src/lib.rs (1)
9-9: LGTM!crates/quickview-ui/src/windows/shared.rs (1)
44-45: LGTM!Also applies to: 65-65, 106-177
.claude/CLAUDE.md (1)
47-61: LGTM!packaging/flatpak/com.example.QuickView.json (1)
25-31: 🔒 Security & PrivacyVerify pinned
glycin-loadersarchive checksum.
The manifest pins a specific tarball andsha256; this needs a manual check against the official GNOME source release before merging.crates/quickview-ui/src/decode.rs (1)
77-93: 🩺 Stability & AvailabilityDrop this warning:
gdk4::Texture::from_filenameis thread-safe ingdk4 0.10.0, andgdk4::TextureisSend + Sync. The worker-thread fallback does not have the compile/panic issue described here.> Likely an incorrect or invalid review comment.adrs/ADR-0004-Image-Decoding.md (1)
23-36: LGTM!docs/DECISIONS.md (1)
19-22: LGTM!docs/DEPENDENCIES.md (1)
16-24: 📐 Maintainability & Code QualityDisambiguate the installable package name.
The section uses
glycinfor both the optional system loader entry and the always-compiled Rust crate below, which makes the install path ambiguous. The provided context only confirms a Flatpak module namedglycin-loaders; please verify the distro package name before publishing.docs/PHASED_PLAN.md (1)
147-159: LGTM!README.md (1)
56-56: LGTM!packaging/arch/PKGBUILD (1)
18-22: 📐 Maintainability & Code QualityVerify the Arch optdepend name.
The optdepend uses
glycin, but the runtime dependency here is the loader package rather than the always-compiled Rust crate. Please confirm the Arch package name so users aren't told to install the wrong artifact.
Review follow-ups for PR #3: - Codex: the glycin client crate links libseccomp, lcms2, and fontconfig (sandbox seccomp filter, ICC, font paths) — needed at build/link time regardless of the runtime backend. Declare them in PKGBUILD depends, the README install line, and DEPENDENCIES.md, and correct the inaccurate 'pure Rust, no system deps' wording in Cargo.toml and ADR-0004. - CI: cargo-deny failed on RUSTSEC-2024-0436 (paste is unmaintained). paste is a build-time proc-macro pulled in transitively by glycin's deps with no fixed release; add a scoped, documented ignore.
|
@coderabbitai Re the |
|
Tip For best results, initiate chat on the files or code changes.
(=^･ω･^=) noted, makes sense ✏️ Learnings added
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8e81aab7f
ℹ️ 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".
| # Sandboxed image decoding. Always compiled; whether it is used is decided at | ||
| # runtime by probing installed loaders. Links libseccomp, lcms2, and | ||
| # fontconfig (see docs/DEPENDENCIES.md). | ||
| glycin = { version = "3", features = ["gdk4"] } |
There was a problem hiding this comment.
With glycin now always compiled, the clean Arch CI jobs need its native pkg-config dependencies before cargo clippy, cargo test, and cargo build run. I checked both install blocks in .github/workflows/ci.yml; they still install only GTK/Tesseract/layer-shell deps and omit the newly documented libseccomp, lcms2, and fontconfig, so the workflow can fail during dependency build/link before the runtime GDK fallback is ever reached. Fresh evidence beyond the earlier comment is that the CI workflow dependency lists were not updated in this commit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 3b16770 — the three packages are now explicit in both install blocks. Worth noting the premise was partially off: CI passed on the reviewed commit because on Arch gtk4's dependency closure already pulls fontconfig, lcms2, and libseccomp (verified with pactree), and Arch ships headers/.pc files in the main packages. So the workflow could not actually fail as described — but depending on a transitive dep tree for directly-linked libraries is fragile, hence the explicit declaration.
- Add libseccomp/lcms2/fontconfig to both install blocks. They were already present transitively via gtk4's dependency closure (which is why CI passed), but the workflow shouldn't rely on another package's dep tree for libraries we link directly. - Replace 'cargo install cargo-deny' with the pinned prebuilt musl binary: the source build cost ~100s per run whenever the cache missed or upstream released; the download takes ~2s. Drop the now unneeded ~/.cargo/bin cache path.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/ci.yml:
- Around line 94-101: The Install cargo-deny step currently pipes the downloaded
tarball straight into tar without verification. Update the workflow in the
Install cargo-deny job to fetch the matching cargo-deny-${DENY_VERSION} .sha256
checksum from the release, validate the tarball against it before extraction,
and only then unpack it; keep the change localized to this step so the existing
cargo-deny install flow remains the same but with checksum verification added.
🪄 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: 87f75936-e95a-44e0-966f-048d627d17bc
📒 Files selected for processing (1)
.github/workflows/ci.yml
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: build-test
🔇 Additional comments (2)
.github/workflows/ci.yml (2)
34-35: Native dependency additions align with packaging manifest.
gtk4-layer-shell,libseccomp,lcms2, andfontconfigmatch the runtime deps declared inpackaging/arch/PKGBUILD, needed for glycin's sandbox and now-always-compiled build.Also applies to: 78-80
83-93: Cache key rework is consistent with the new install method.Dropping
/root/.cargo/binfrom the cache path is correct sincecargo-denyis no longer installed viacargo install.
Checksum is hardcoded in the workflow rather than fetched from the release, so a swapped artifact fails the build instead of running.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90806e75f9
ℹ️ 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".
| # Sandboxed image decoding. Always compiled; whether it is used is decided at | ||
| # runtime by probing installed loaders. Links libseccomp, lcms2, and | ||
| # fontconfig (see docs/DEPENDENCIES.md). | ||
| glycin = { version = "3", features = ["gdk4"] } |
There was a problem hiding this comment.
Fresh evidence beyond the earlier dependency thread is that scripts/bootstrap_arch.sh, the Quickstart setup path, still installs only GTK/Tesseract/layer-shell deps and leaves glycin as an optional comment; with glycin now compiled unconditionally here, a clean Arch machine following that script can hit the native link/build requirements before any runtime GDK fallback is possible. Please add the same native packages documented elsewhere (at least the glycin client link deps such as libseccomp) to the bootstrap script too.
Useful? React with 👍 / 👎.
1 participant
Summary
Replaces the synchronous
Texture::from_fileinload_filewith fully async decoding off the GTK main thread. Implements ADR-0004 and closes the NFR-001 (no UI freeze on decode) / NFR-002 (sandboxed decoding of untrusted images) gaps — the first Phase 7 task.How it works
decodemodule (crates/quickview-ui/src/decode.rs) with two backends:glib::MainContext::spawn_local(no extra thread)Texture::from_filenameon a worker thread + async-channel (same plumbing as OCR)Loader::supported_mime_types()is probed once per session and the chosen backend is logged at first decode (visible underRUST_LOG=info). One build recipe works across Arch / Ubuntu 24.04+ / Debian 13+ and degrades gracefully elsewhere.decode_job_idmirrors the existing OCR job-id pattern; fast arrow-key navigation abandons superseded decodes.load_filealso invalidates in-flight OCR up front so a late OCR result can't clear the busy spinner mid-decode. FileInfo emission and OCR kickoff moved into thefinish_decodecompletion handler.Packaging / docs
glycin-loaders2.1.5 meson module (loaders only) — the GNOME 49 runtime does not ship the loadersglycin-gtk4(the C bindings aren't used by the Rust crate)Testing
cargo fmt/cargo clippy --all-targets --all-features -- -D warnings/cargo test --allall green (core crate untouched)