Repository navigation
Merge revamp slice 1: N-way merge engine, shared resolver, and two data-loss fixes - #27
Merged
Merged
Conversation
bashar-qassis
changed the title
3 tasks
bashar-qassis
force-pushed
the
worktree-merge-revamp
branch
2 times, most recently
from
August 19, 2026 22:00
9c82882 to
a70bda4
Compare
…ich candidates on merge
… just vs. survivor Handles loser-vs-loser collisions on contact_tags, activity_contacts and photos (unique indexes), adds the same treatment for immich_candidates, and resolves colliding birthday reminders (partial unique index on reminders) by keeping the survivor's own reminder or the lowest-id loser's. Extracts the collision handling into a shared dedupe_and_move/5 helper.
Addresses were never deduplicated after merge (plan gap across every task); add the design spec's line1+postal_code normalized dedupe. Normalize the contact_fields dedupe key too (case/whitespace-insensitive per spec §3). Add loser-vs-survivor relationship collision regression tests.
Two addresses lacking both line1 and postal_code normalize to the same empty key but can be different places (differ by city/region/country/ label). Only dedupe when the key has actual content.
…ops, drop display-name job
…ment dedupe/drop ordering contract
…n, document unchecked_ids, add loser-collision and account-isolation tests
…w immich contacts
bashar-qassis
force-pushed
the
worktree-merge-revamp
branch
from
August 19, 2026 22:13
a70bda4 to
f1a1602
Compare
…on and field choices
The wizard seeded field_choices with "survivor" for every mergeable field, which apply_legacy_choices/4 reads as a deliberate pin and turns into :clear whenever the survivor's value is nil. Every wizard merge therefore discarded the resolver's gap fill. Seed nothing; compute the radio's selected state.
…dios The old assertion checked whether "Acme" appeared anywhere in the rendered HTML, but both columns render their value unconditionally, so it passed even when the highlight sat on the wrong button. Assert on the accent-subtle class per button instead, and add the converse case (survivor value kept) so the assertion is shown to discriminate in both directions.
Only birthday reminders were deduped, but no unique index covers stay_in_touch, so a merge could leave two active rows on the survivor and Reminders.resolve_stay_in_touch_instance/1 would raise MultipleResultsError on the next logged interaction. Dedupe in the engine and make the lookup total with limit: 1.
Merging a user's me-contact away left the FK naming a trashed row; the purge worker then hard-deleted it and on_delete: :nilify_all silently cleared the link 30 days after the merge that caused it.
merge_cluster/4 is documented to return {:error, reason}; an unrecognised
field key raised inside the transaction and would kill the caller's
LiveView process rather than surfacing a flash.
merge_cluster/4 is the caller-facing function; its own @doc had a separate error-reason list from run/4's and was left out of the prior commit.
Also missing from run/4's list, and genuinely returnable via validate_drop/2. Folded into this doc-fix series per review ruling rather than deferred as a separate change.
resync_birthday_reminder/4 refilled enqueued_oban_job_ids while active stayed false, undoing what deactivation deliberately cleared. Keep tracking next_reminder_date; skip the enqueue.
The round-1 gap-fill test only called merge_contacts/2, the arity the UI never uses, which is why the wizard's default-choices bug survived review. Pair each resolver assertion with a 3-arity run, and add structural tests asserting the engine's registries still cover every contact_id column and every FK into contacts.
…omments Review found two disclosure problems in the Task 6 audit: - merge_invariants_test.exs's two structural tests cannot see a pointer that is both unnamed contact_id and has no FK (import_records.local_entity_id's shape exactly). Document the blind spot in the moduledoc and pin the one known instance with a third, structural test that fails if remap_import_records_step/4 is removed or unwired from build_multi/7 — a behavioural test for it already exists, so this doesn't duplicate a merge. - The Part 2 wizard-path test comments implied they closed the same arity-based gap that let Finding 1 survive review. They don't: apply_legacy_choices/4 resolves each field independently, so that gap cannot occur there. Reworded the comments to state the narrower invariant they actually verify (an unrelated explicit choice must not disturb an untouched field's resolution) without overclaiming.
…opaque Every Ecto.Multi pipeline trips call_without_opaque. The cause is upstream: Ecto.Multi's `names` field is typed MapSet.t(), MapSet.t/0 is opaque, and Dialyzer constant-folds Multi.new/0 to a struct literal holding a structurally built MapSet where an opaque one is expected. Ecto.Multi's own t/0 is not opaque, so this is not internal-field access on our side. No code shape fixes it — routing Multi.new/0 through a `@spec _ :: Multi.t()` boundary only relocates the warning to contract_with_opaque, because Multi.new/0's success typing cannot satisfy Multi.t() itself. Replaces 22 :nowarn_function escapes across six contexts, each of which was muting every check on its function rather than just the opacity warning. All 22 were verified to be this false positive alone: narrowing them to :no_opaque surfaced no new warnings before they were removed entirely. lint_rtl.ex keeps its :nowarn_function — that one is an :error_handling warning on a Mix task, unrelated to Ecto.Multi.
Comments describing superseded versions of the code rather than the code as it stands: the @owned_schemas note counted schemas "the previous two-contact engine silently orphaned" (a count that rots the moment the list is reordered, now stating the exhaustiveness requirement and its invariant test instead), :remap_contact_tags claimed to mirror "the old engine's" step, and resync_birthday_reminder/4 carried a bare note that a function had been renamed in an earlier task.
Rewrite comments that described the edit rather than the code: the import_records note left behind by the removed fixup helper, the wizard's default_field_choices/0 note, and the :no_opaque rationale in mix.exs. Replace the `Task 1`/`Task 3` plan-doc pointers in the merge tests with the behaviour they refer to, and drop the "as originally assumed" narration from the invariant test's known-column list. Rewrap an over-long line in @owned_schemas' comment and remove its duplicate of the MergeInvariantsTest claim already stated at __owned_schemas__.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Slice 1 of the duplicate-contact merge revamp. Replaces the two-contact merge engine with an N-way
merge_cluster/4backed by a shared resolution module, and fixes two data-loss bugs.No user-visible change: the existing merge wizard and
POST /api/contacts/mergekeep working through a thinmerge_contacts/3shim over the new engine. The only behaviour difference is that merging no longer dismisses duplicate suggestions the user never saw.Spec:
docs/superpowers/specs/2026-08-17-duplicate-merge-revamp-design.mdPlan:
docs/superpowers/plans/2026-08-17-merge-revamp-slice-1-engine.mdWhat's here
Kith.Contacts.MergeFields— declares which contact fields are mergeable and how.non_clearable/0derives from the changeset'svalidate_requiredlist rather than being hand-maintained.Kith.Contacts.MergeResolution— a pure resolver: members in, complete field map + conflicts + attributions out. No DB access. One implementation shared by every caller.Kith.Contacts.Merge— the engine. One transaction: lock, validate, apply scalars, remap, deduplicate, apply the drop list, resolve candidate pairs, soft-delete the losers, audit.first_met_throughreferences on the trashed contact.dismiss_candidates_for_contact/2is gone. Pair resolution now only dismisses pairs the user actually reviewed.Spec scenarios covered
D1–D10, F4, G1, G3, Bug 1, Bug 2. D9 (concurrent merges) has its locking implemented but no automated test —
FOR UPDATEcontention needs two connections and the Ecto sandbox gives one, so it needs a manual two-iexcheck before this ships.Notable decisions made during implementation
Each was a conflict between the plan and either the spec or the codebase; the spec won.
SELECT ... FOR UPDATEran beforeRepo.transaction, so the lock was released immediately and D9 would not have held.order_by: c.idwas added for deterministic lock acquisition, since two overlapping merges could otherwise deadlock.immich_candidateshad no guard at all, andremindershas a birthday partial unique index that the commonest merge of all would have hit.DisplayNameRecomputeWorkeris not enqueued. That worker is account-wide and cannot act on one contact, so the job could never succeed.Contact.update_changeset/2already recomputesdisplay_namesynchronously. The spec's §2 step 10 and scenario D10 are annotated with this.first_met_through_idnaming any merged member is coerced tonil, so no caller can produce a contact met through itself.needs_reviewused to fail outright with{:unknown_value, :immich_status}— the normal post-sync state for a duplicate pair, so those pairs were unmergeable.Verification
mix precommitgreen (compile--warnings-as-errors,deps.unlock --unused, format,credo --strict, test).Before merging
iexsessions merging overlapping clusters — the second must block, then fail validation on the now-trashed member rather than merging.test/kith/accounts/tos_acceptance_test.exsrunsasync: truewhile mutating global app env (:require_tos_acceptance), whichUser.maybe_require_tos/1reads. Any concurrent test creating a user then fails with "you must accept the Terms of Service". One-word fix (async: false); untouched here as out of scope.🤖 Generated with Claude Code
https://claude.ai/code/session_01DCMNiVE7SKJeMusAn7DiMd