docs(immich): build spec for edit sync (#224)

[justinf555]

Summary

Captures the design conversation that produced the v0.4.0 plan for syncing non-destructive edits with Immich. New file docs/design-immich-edit-sync.md; section 5 of design-photo-editing.md shrinks to a 7-line pointer at the new doc.

The original design proposed PUT /assets/{id}/original to overwrite the user's original on the server. That endpoint was removed in current Immich main. Empirical probing of v2.7.5 (running locally) confirmed the only viable mutation surfaces are /assets/{id}/edits (crop/rotate/mirror only) and POST /assets + POST /stacks for everything else.

What's in the spec

• Two mechanisms by edit family: native /assets/{id}/edits API for crop/rotate/mirror, render+upload+stack+tag+XMP for exposure/colour/vignette/etc.
• Originals are never modified. Local edits table stays the source of truth; server-side artefacts are projections.
• Recovery via custom XMP (namespace urn:moments:edits:1.0) embedded in the rendered JPEG. Carries originalAssetId, originalContentHash, editVersion, renderedAt, editJson. Lazy parse on edit-open if local row is missing — supports new-install / second-machine scenarios.
• Server-side discovery via a moments-edit tag on rendered assets. Verified on v2.7.5: invisible in the photo info panel, surfaces only via sidebar / search.
• Stacks join the sync: SyncResetV1 misclassifies every local row as orphan and deletes the library #628 heartbeat reconciliation as the fifth reconciled table.

Phased plan

Four phases, sequenced for incremental review:

Phase	Scope	Depends on
A	Stacks schema + sync wiring + grid filter	—
B	Geometric edits via /assets/{id}/edits	—
C	Pixel render+stack+tag+XMP	A
D	Recovery from server state	C

A and B are independent and can land in parallel. Each phase has explicit acceptance criteria in §9 of the spec.

Open questions

Six listed in §11 of the spec. The blocker before Phase B implementation:

• §11.1 — re-probe PUT /assets/{id}/edits with the corrected nested-parameters payload. The first probe used a flat shape and saw 500s; the OpenAPI spec on main confirms {action, parameters: {…}} is the correct form.

Knock-on effects

• Issue Editing prep: sync-conflict schema for edits table (server vs local) #641 is superseded. Its premise ("server bytes diverged from local") doesn't apply in either of the two new mechanisms. Only the "which Immich asset holds our render" column survives, covered in §3.2.
• Issue Editing: Immich render-and-upload (Phase 6) #224 scope grows beyond size/L — the spec splits it into four phases.
• Issue Editing: polish and integration (Phase 7) #225 (polish) depends on this design landing.

Test plan

• make lint (no code changes; doc-only)
• Spec reviewed end-to-end before commit
• Re-probe PUT /assets/{id}/edits with the corrected payload (§11.1) before Phase B starts
• Verify Immich preserves embedded XMP on POST /assets round-trip (§11.2) before Phase C starts

🤖 Generated with Claude Code

[github-actions]

Claude finished @justinf555's task in 2m 42s —— View job

---

PR Review: docs(immich): build spec for edit sync (#224)

Tasks

• Gather context and understand the request
• Read docs/design-immich-edit-sync.md (new file)
• Read docs/design-photo-editing.md (modified)
• Cross-reference with existing architecture (migration count, Mutation/outbox pattern, pull.rs types, ImmichClient methods)
• Provide review feedback

---

Summary

This is a well-structured design document that fixes a fundamental premise error (the now-absent PUT /assets/{id}/original) and replaces it with a credible two-mechanism hybrid. The phased approach is sound and the dependency ordering is correct. Cross-referencing against the codebase shows the migration numbers, id_by_external_id method name, delete_with_body HTTP method, and outbox storage pattern are all consistent with the existing implementation.

Several spec-level issues need resolution before Phase B or C implementation begins — one is a definite copy-paste bug, two are design gaps that would cause ambiguity for the implementer.

---

Issues

Bug — copy-paste error in §4.2 AssetEditHandler

// §4.2 step 1:
let media_id = ctx.library.media().id_by_external_id(face.asset_id).await?;

face.asset_id is copied from AssetFaceHandler. In this handler the variable should come from the deserialized edit payload (e.g. payload.asset_id). Using face here won't compile and reveals a copy-paste from the wrong handler. Fix this →

---

Design gap — §7.4 Recovery SQL is internally contradicted

Step 1 of the recovery flow debates itself mid-paragraph and never resolves:

SELECT id FROM media WHERE stack_id = ? AND id != ?
(the non-primary; for an edited photo the primary IS the rendered)
Actually: ... That's the rendered. (Wait: the rendered IS primary in our model.)
Equivalent: find the Moments-tagged member of the stack the user opened.

The spec correctly establishes in §7.2 that StackCreated { primary_asset_id: rendered, original_asset_id: original } makes the rendered asset the primary. That means Phase D should open the editor when the user opens the original (the non-primary sibling), not the primary. The recovery query should be:

-- User is on the original (non-primary). Find the rendered sibling (the primary):
SELECT s.primary_asset_id
FROM stacks s
JOIN media m ON m.id = ? AND m.stack_id = s.id

The current text leaves this ambiguous enough to produce two different implementations. Fix this →

---

Design gap — Mutation enum serialization not specified

§4.3 shows Mutation enum variants with Rust struct syntax but doesn't specify how they serialize to the (entity_type, action, payload) outbox tuple. The existing pattern (OutboxRow { entity_type, entity_id, action, payload }) requires every new variant to implement to_outbox_rows(). Implementers will need to invent the serialization — which risks divergence from the pull-side from_row decoders.

A table like the following would close this gap:

Mutation variant	entity_type	action	payload
AssetEditsApplied { id, actions }	"asset"	"apply_edits"	{"actions": [...]}
AssetEditsCleared { id }	"asset"	"clear_edits"	—
StackCreated { primary_asset_id, original_asset_id }	"stack"	"create"	{"primary_id": "...", "original_id": "..."}
...

Fix this →

---

Design gap — §7.2 step 8 ordering dependency unspecified

8. After push completes and external_id is stamped on the new media row: record StackCreated and AssetTaggedMomentsEdit outbox entries.

The outbox drains via PushManager::push_one sequentially, but the new media row for the rendered asset doesn't have its external_id set until the upload succeeds. The spec doesn't say how StackCreated finds the rendered asset's external_id — does AssetImported push return the new id synchronously? Does push_one stamp it before returning? Or does server_rendered_asset_id need to be set via a callback?

This is subtle but important: if StackCreated is queued before the rendered asset's external_id is available, lookup_media_external_id in the push handler will fail. The spec should say explicitly whether StackCreated is queued after AssetImported completes in-process, or whether the push handler for AssetImported is responsible for enqueueing it.

---

Minor — straighten_degrees not classified

EditState.TransformState in design-photo-editing.md includes straighten_degrees: f64 (freeform ±45°). §5.3's action mapping only covers crop/rotate (multiples of 90°)/mirror and is silent on straighten. The spec's EditState::is_geometric_only() predicate needs to explicitly classify straighten — it's almost certainly a pixel adjustment (requires interpolation), not a geometric one, but the spec doesn't say so. If it's misclassified as geometric, Phase B would silently try to send a float angle to Immich's rotate action, which only accepts 90/180/270.

---

Minor — §11.6 open question already resolved by this PR

§11.6 asks "What about section 5 of design-photo-editing.md?" — this PR already answers it (shrinks to a 7-line pointer). The question can be crossed off or removed from the open-questions list. Fix this →

---

Minor — persist_stack_locally helper undefined

§4.4 StackCreated push handler calls self.persist_stack_locally(...) but this helper is not specified anywhere. At minimum the spec should note it as a new method on PushManager and describe what it does (insert into stacks, update media.stack_id for both members).

---

Minor — hardcoded local path in §10.2

API key at `/home/justin/.config/immich-test/api_key`

The make test-immich target should read this from an env var (IMMICH_TEST_API_KEY_FILE or similar) so CI can supply it without the path being tied to a developer's home directory.

---

Nits

• §4.2, step 3: editing().upsert_edits() — verify this method exists or is planned; the current LibraryEditing trait in design-photo-editing.md exposes save_edit_state, not upsert_edits. Alignment between the trait name and the handler call would reduce friction at implementation time.
• quick-xml dependency will need to be added to Cargo.toml for src/renderer/xmp.rs. Worth noting in the Phase C checklist.
• §5.2 originalContentHash is documented as "SHA-1, base64" — confirm this matches Moments' existing media.content_hash format (the CLAUDE.md doesn't mention the hash algorithm). If they differ, the fallback lookup in §7.4 step 4 will silently fail to match.

---

Overall: Approve the direction and the phased plan. The two critical items (copy-paste face.asset_id and the contradicted recovery SQL) should be fixed before the spec is used to drive Phase B/C implementation — the rest are gaps that can be clarified at implementation time but would be cleaner to resolve now.

[github-actions]

📊 Code Complexity Report

Complexity    SLOC  Function
──────────  ──────  ──────────────────────────────────────────────────
        52     215  ui/viewer/mod.rs::setup_signals
        34     157  sync/providers/immich/pull.rs::run_sync
        33      48  ui/preferences_dialog/mod.rs::spawn_library_stats
        32      37  ui/preferences_dialog/mod.rs::<anonymous>
        28     144  ui/video_viewer/mod.rs::setup_signals
        27     188  ui/photo_grid/mod.rs::set_store
        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     154  ui/album_picker_dialog/dialog.rs::connect_signals
        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      72  ui/sidebar/mod.rs::build_pinned_section
        19      70  ui/album_grid/selection.rs::wire_batch_delete
        19      53  ui/album_grid/selection.rs::<anonymous>
        18     193  ui/photo_grid/mod.rs::setup
        18      29  ui/preferences_dialog/mod.rs::spawn_server_stats
        17      20  ui/preferences_dialog/mod.rs::<anonymous>

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

[github-actions]

🔒 Security Report

Check	Status
Cargo audit	✅ Pass
Cargo deny	✅ Pass

_{job log}

[justinf555]

Sidebar doesn't have Tags as a subsection anymore

[github-actions]

CI Summary

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

_{full run log}