Refactor application/ — LibraryContext + decomposed module

[justinf555]

Summary

• Introduces LibraryContext, a pure-Rust pub(in crate::application) container for the domain + infrastructure (Library, RenderPipeline, Tokio handle, purge-task handle). UI code physically cannot name the type — the architectural boundary is now enforced by the type system, not by convention.
• Decomposes the 898-line application/mod.rs god-object into seven sibling files with clear roles (context.rs, startup.rs, lifecycle.rs, library_loader.rs, import.rs, actions.rs, plus the existing keyring.rs). mod.rs drops to 369 lines — essentially the GObject subclass machinery + public accessor API.
• Replaces 13 RefCell<Option<T>> fields on MomentsApplication with OnceCell<T> for clients and LibraryContext-owned shapes for domain state. Tightens client constructor signatures to declare minimal sub-service dependencies via a build() convention enforced inside application/.

Triggered by an external engineering review (Downloads/moments_repository_engineering_review.md) which flagged "Application State Container Is Trending Toward a God Object" as the highest-severity architectural finding.

Full design doc landed as the first commit of this PR: docs/design-library-context.md.

Architectural property achieved

• LibraryContext is pub(in crate::application) — unspellable in ui/, client/, library/. UI code cannot reach the domain except through a Client.
• Clients are constructed via Client::build(...) inside application/ only. UI code cannot construct a Client because it cannot acquire the constructor arguments (sub-services live behind pub(in crate::application) accessors on LibraryContext).
• All client storage is OnceCell<T> (post-startup contract), with Option<&SyncClient> exposed for the Immich-only sync client.
• Startup is four typed phase functions (phase1_domain, phase2_background_services, phase3_clients, phase4_install) — compiler enforces ordering.
• SyncEngine has its own typed identity on Application; SyncClient holds an Arc<SyncEngine> for UI-driven engine control.

Per-step breakdown (11 commits)

Step	Commit	What
Doc	97dcb41	docs/design-library-context.md — the design
1	7ea02bc	Introduce LibraryContext; populate alongside legacy fields
2	05b18a1	Switch client construction to build() taking concrete deps
3	85cc084	Migrate client fields to OnceCell<T>
4	395e2e8	Remove deprecated accessors + legacy domain fields
5	0f84c33	Split startup into typed phase functions (startup.rs)
6	8d4abb0	Reshape SyncHandle → SyncEngine top-level service
7	98c018d	Extract LibraryLoader from duplicated wizard/open paths
8	cac9c2c	Extract actions.rs + import.rs from mod.rs
9	c97b226	Extract library-open lifecycle into lifecycle.rs
10	3923cf5	Tighten Album/People clients to minimal sub-services

Steps 7–10 emerged during implementation as architecturally-adjacent cleanups (Step 7 is documented in the design doc; Steps 8–10 are pure decomposition / dependency-tightening). They were bundled into this PR because they live in application/ and the cost of doing them separately later would be higher.

application/ module shape after

mod.rs            369  — GObject subclass + public accessors + shutdown/activate
startup.rs        503  — 4 typed startup phases
lifecycle.rs      186  — setup wizard + open-library orchestration
library_loader.rs 157  — shared bundle-open + keyring pipeline (+ 4 unit tests)
keyring.rs        144  — token resolution (unchanged)
import.rs         124  — import dialog + flow
actions.rs        108  — app actions + about/shortcuts/prefs dialogs
context.rs        106  — LibraryContext (domain owner)

Notable deviations from the design doc

Two deliberate deviations are documented inline in the code:

1. library_context field is RefCell<Option<Arc<LibraryContext>>>, not the doc's sketched OnceCell<Arc<...>>. GObject shutdown takes &self, but OnceCell::take requires &mut self. The existing shutdown path needs to clear the Arc<Library> (and the SqlitePool it wraps) before main() drops the Tokio runtime. Converting requires a shutdown-ordering redesign — out of scope for this refactor. Documented at the field declaration in mod.rs.

2. MediaClientV2 and ImportClient keep Arc<Library> rather than splitting into N sub-services. Both genuinely touch 6+ services plus Library-level cross-service methods; splitting would harm readability without architectural benefit. The doc explicitly allowed this ("If a client uses the whole Arc<Library> because it touches many services, that's acceptable"). AlbumClientV2, PeopleClientV2, and SyncClient were tightened (Step 10).

Out of scope / deferred

These were flagged in the design doc as future work:

• BackgroundContext — anticipated holder for long-running background services (SyncEngine, PurgeTask) once a coordinated operation (pause_all, shutdown_in_order, apply_config) is needed. Trigger condition: ≥3 background-task fields or first concrete coordination need.
• ClientContext — speculative, depends on D-Bus design. Deferred until cross-cutting policy actually differs between callers.
• AppConfig — typed config struct read once from gio::Settings. Open question in the design doc.

Test plan

• make lint clean (cargo fmt + clippy -D warnings, both default and --features dhat-heap)
• make test — 615 passed, 0 failed (baseline was 611; added 4 LibraryLoader unit tests in Step 7)
• make typos clean
• make test-integration passes
• Manual: launch via make run-dev, verify Local-backend setup flow (new library creation) — reviewer to confirm
• Manual: launch via make run-dev, verify Immich-backend setup flow and sync — reviewer to confirm
• Manual: launch with stale GSettings library path (delete the library dir, then start) — verify recovery into setup wizard with the stale-path dialog
• Manual: trigger an import — verify the import dialog and run flow

🤖 Generated with Claude Code

[github-actions]

Claude finished @justinf555's task in 3m 27s —— View job

---

PR Review: Refactor application/ — LibraryContext + decomposed module

Tasks

• Gather context and understand the PR
• Read core new files (context.rs, startup.rs, lifecycle.rs, library_loader.rs)
• Review mod.rs changes
• Review client changes (album, people, sync)
• Review sync/mod.rs changes
• Check UI-side changes for regressions
• Post review findings

---

Overall Assessment

This is a well-executed architectural refactor. The pub(in crate::application) visibility enforcement is a legitimate compile-time barrier against UI code reaching domain objects directly, the four-phase typed startup meaningfully enforces ordering, and the transition from RefCell<Option<T>> to OnceCell<T> for clients is clean. The design doc deviations are honestly documented and justified. No regressions visible in the UI-side changes — they're all simple accessor renames/removals following the new client build() convention.

Four issues worth addressing before merge:

---

Issues

1. ImmichClient::new failure is silently swallowed — startup.rs:222–224

let immich_client = immich_info
    .as_ref()
    .and_then(|info| ImmichClient::new(&info.server_url, &info.access_token).ok());

If ImmichClient::new fails (malformed URL, etc.), immich_client is None. Downstream, recorder becomes NoOpRecorder and resolver becomes LocalResolver — the library opens successfully but mutation recording and original-file resolution are silently wrong for a user who configured Immich. There's no warn! log, no error surface. At minimum this deserves a warn! before the .ok():

let immich_client = immich_info.as_ref().and_then(|info| {
    match ImmichClient::new(&info.server_url, &info.access_token) {
        Ok(c) => Some(c),
        Err(e) => {
            warn!("failed to build Immich HTTP client: {e}; continuing without sync");
            None
        }
    }
});

Fix this →

---

2. thumbnails_dir_for_sync duplicates paths.thumbnails_dir — startup.rs:86–92, start()

DomainArtifacts carries both paths: ImportPaths (with thumbnails_dir: PathBuf) and thumbnails_dir_for_sync: PathBuf. Both are populated from bundle.thumbnails.clone() in start(). phase2_background_services uses domain.thumbnails_dir_for_sync but it could use domain.paths.thumbnails_dir. Removing thumbnails_dir_for_sync would shrink DomainArtifacts and eliminate the question of "why are these the same path stored twice?".

Fix this →

---

3. PeopleClientV2::insert_into_models lacks the idempotency guard added to AlbumClientV2 — client/people/client_v2.rs:295–307

AlbumClientV2::insert_into_models (line 628) skips an insert when the album ID is already present — this guards against the command path and AlbumEvent::AlbumAdded both firing for the same ID. PeopleClientV2::insert_into_models has no equivalent guard, so a FacesEvent::PersonAdded arriving after a successful create-command could produce a duplicate row. The pattern to match:

// In insert_into_models, before store.append:
if find_by_id(&store, id.as_str()).is_some() {
    continue; // already present — skip
}

Fix this →

---

4. library_context() accessor is dead but suppressed — mod.rs:278–281

#[allow(dead_code)]
pub(in crate::application) fn library_context(&self) -> Option<Arc<LibraryContext>> {
    self.imp().library_context.borrow().clone()
}

shutdown() accesses the RefCell field directly (.borrow_mut().take()), never through this accessor. Either route shutdown through the accessor (making #[allow(dead_code)] unnecessary) or remove the accessor. Suppressing a dead-code warning with #[allow] is a maintenance hazard since it masks future true dead code.

---

Minor notes (no action required)

• library_context field comment (mod.rs:52–57) says clearing it drops Arc<Library> before the runtime. That's partially true — it drops the LibraryContext's clone — but MediaClientV2 and ImportClient in OnceCell<T> also hold Arc<Library> and can't be cleared from &self. The comment deeper in shutdown() correctly acknowledges this; the field-level doc is a bit misleading. Worth a one-line clarification.

• SyncClient outbox methods reach for MomentsApplication::default().tokio_handle() rather than storing the Tokio handle during build(). This is consistent with the rest of the client surface, so no change needed — just worth documenting if the design doc is updated.

• Phase structure is clean. The typed DomainArtifacts / BackgroundServices / BuiltClients result types make the data-flow between phases readable and the compile-time ordering guarantee is real.

[github-actions]

📊 Code Complexity Report

Complexity    SLOC  Function
──────────  ──────  ──────────────────────────────────────────────────
        34     167  sync/providers/immich/pull.rs::run_sync
        32      47  ui/preferences_dialog/mod.rs::spawn_library_stats
        32      37  ui/preferences_dialog/mod.rs::<anonymous>
        25      66  client/people/client_v2.rs::listen
        25      65  client/album/client_v2.rs::listen
        24      59  client/media/client_v2.rs::listen_media
        23      36  client/media/client_v2.rs::on_media_changed
        22      85  ui/viewer/info_panel/camera_section.rs::set_item
        20      47  renderer/format/detect.rs::detect_from_bytes
        19     114  renderer/edits.rs::apply_pixel_adjustments
        19      70  ui/sidebar/mod.rs::build_pinned_section
        19      69  ui/album_grid/selection.rs::wire_batch_delete
        19      52  ui/album_grid/selection.rs::<anonymous>
        18     193  ui/photo_grid/mod.rs::setup
        17     184  ui/photo_grid/mod.rs::set_store
        17      28  ui/preferences_dialog/mod.rs::spawn_server_stats
        17      20  ui/preferences_dialog/mod.rs::<anonymous>
        16      92  ui/viewer/edit_panel/filter_section.rs::setup
        16      72  ui/people_grid/factory.rs::build_factory
        16      71  ui/viewer/edit_panel/mod.rs::save_to_db

_{Top 20 functions by cognitive complexity · job log}

[github-actions]

🔒 Security Report

Check	Status
Cargo audit	❌ Fail
Cargo deny	❌ Fail

_{job log}

[github-actions]

CI Summary

Check	Status
Typo check	✅ Pass
Build & test (fmt + clippy + unit + integration)	✅ Pass

_{full run log}

[github-actions]

Claude finished @justinf555's task in 51s —— View job

---

Re-review: PR #663 review findings addressed

Tasks

• Gather context
• Check latest commit addressing review findings
• Verify each of the 4 issues from prior review
• Post updated assessment

---

All four issues flagged in the prior review are cleanly resolved in 3dbec5e:

1. ImmichClient::new failure logged — startup.rs ✅
The silent .ok() is replaced with a match that emits warn!("failed to build Immich HTTP client: {e}; continuing without sync"). Correct.

2. thumbnails_dir_for_sync removed — startup.rs ✅
Field dropped from DomainArtifacts, removed from start() extraction, and phase2_background_services now reads domain.paths.thumbnails_dir.clone(). Cleaner than before.

3. PeopleClientV2::insert_into_models idempotency guard ✅
Guard added matching the album-side pattern, plus a unit test (insert_into_models_is_idempotent) covering the exact double-insert race scenario. Good coverage.

4. Dead library_context() accessor removed ✅
Accessor and its #[allow(dead_code)] suppression are gone. shutdown continues to access the RefCell field directly, which is the right call given the OnceCell::take/&self constraint documented in the field comment.

---

No new issues found. The PR is in good shape — the architectural goals are achieved, the deviations are honestly documented, CI is green, and all prior review findings are resolved. Ready to merge pending manual smoke-test sign-off (Local setup flow, Immich flow, stale-path recovery, import dialog) as noted in the test plan.