Make edit-suggestion approval atomic + idempotent (server-side callable)

[roiguri]

Problem

Edit-suggestion approval (EditSuggestionReview.handleApprove, src/lib/recipes/suggest_edit_modal/edit-suggestion-review.js) orchestrates a multi-resource, privileged mutation from the browser with no atomicity:

1. RecipeService.update() — writes the recipe doc + migrates/deletes Storage images
2. RecipeEditSuggestionService.approve() — stamps the suggestion approved and supersedes sibling pendings (more writes + Storage cleanup)

There is no transaction, and every await is a partial-failure window. Concrete failure modes:

• Apply succeeds, stamp fails → recipe is updated but the suggestion stays pending. The generic error toast invites the manager to click Approve again, which re-applies the same snapshot (re-running image migration/deletion) and never supersedes siblings.
• Concurrent managers → two managers open the same pending suggestion; the second's in-memory status is a stale pending (the list comes from listPending()), so their approve applies a second full-recipe snapshot, reverting the first and potentially deleting its images.

This shares a root cause with #315 (recipe field schema/allowlist): the client orchestrates a privileged write whose scope and sequencing it shouldn't own.

Severity / why deferred

At current scale this is rare (single active manager, low concurrency) and the re-apply is largely idempotent for content. A client-side guard was prototyped (re-read status before applying + split error handling) but cannot fully close the tail: if the stamp genuinely fails the doc stays pending, so a later approve still re-applies. The durable fix is server-side, so we're tracking it rather than shipping a partial client-side guard.

Recommended long-term solution

Move approval into a callable Cloud Function (approveEditSuggestion({ suggestionId })) — the project already runs Cloud Functions (functions/index.js). Inside:

1. Verify manager role server-side (rules become defense-in-depth, not the only gate).
2. Firestore transaction for the doc writes: read the suggestion inside the transaction, assert status === 'pending', then write the recipe doc + stamp the suggestion + supersede siblings atomically. This fully closes the double-apply / revert tail — re-invocation reads a non-pending doc and no-ops (idempotent).
3. Apply the canonical recipe field schema from Enforce recipe field schema/allowlist in the service layer (RecipeService.update + edit-suggestion create) #315 server-side, so a console-injected rogue field can never reach the recipe. (Folds Enforce recipe field schema/allowlist in the service layer (RecipeService.update + edit-suggestion create) #315 and this fix into one enforcement point.)
4. Storage ops are idempotent, best-effort, post-commit: Storage can't join the Firestore transaction, so commit the doc transaction first (source of truth), then migrate/delete images; sweep orphans via a Storage-triggered cleanup/reconciliation (same pattern as the existing Storage-triggered WebP resizing). Never block the user-visible result on Storage cleanup.

Chose callable over a Firestore-status-trigger for the synchronous manager UX (immediate confirmation after they've already reviewed the diff); the transaction provides the idempotency a trigger would otherwise need anyway.

Migration path

1. (now) keep the current client flow as-is.
2. build the canonical schema module (Enforce recipe field schema/allowlist in the service layer (RecipeService.update + edit-suggestion create) #315) — useful standalone.
3. add the callable approveEditSuggestion using that schema + transaction; point the dashboard at it; retire client-side orchestration in handleApprove.
4. extend the same treatment to reject (Storage cleanup) and the shared manager edit path.

Tradeoffs

• Adds cold-start latency to approve (acceptable — rare manager action, not a hot path).
• Adds firebase deploy --only functions to the release flow (outward, hard-to-reverse — confirm before running).
• Storage remains non-transactional by nature; reconciliation is the accepted answer.

Acceptance criteria

• Approval is a single server-side operation; manager role verified server-side.
• Doc writes (recipe + suggestion stamp + sibling supersede) are transactional and idempotent — a duplicate/concurrent approve cannot double-apply or revert.
• Field set applied is constrained by the Enforce recipe field schema/allowlist in the service layer (RecipeService.update + edit-suggestion create) #315 canonical schema.
• Storage cleanup is idempotent with orphan reconciliation; user-visible result does not depend on it.
• Client handleApprove no longer orchestrates the multi-step mutation.

References

• src/lib/recipes/suggest_edit_modal/edit-suggestion-review.js (handleApprove)
• src/js/services/recipes/recipe-edit-suggestion-service.js (approve, supersedePending, reject)
• src/js/services/recipes/recipe-service.js (update)
• Related: Enforce recipe field schema/allowlist in the service layer (RecipeService.update + edit-suggestion create) #315 (recipe field schema/allowlist — shares the enforcement point)
• Found during review of feat(recipes): Suggest an Edit with manager approval (Milestone 1) #307 (Suggest an Edit, Milestone 1).