From 933bc863446d48c8e9af67a9ac61d01d25b8d1cc Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Tue, 18 Aug 2026 11:11:53 +0300 Subject: [PATCH 01/27] feat(duplicates): derive clusters with dismissed pairs as negative edges --- lib/kith/duplicate_detection.ex | 128 ++++++++++++++++++++++++ lib/kith/duplicate_detection/cluster.ex | 19 ++++ test/kith/duplicate_detection_test.exs | 127 +++++++++++++++++++++++ 3 files changed, 274 insertions(+) create mode 100644 lib/kith/duplicate_detection/cluster.ex diff --git a/lib/kith/duplicate_detection.ex b/lib/kith/duplicate_detection.ex index 6ae52641..401ad2fa 100644 --- a/lib/kith/duplicate_detection.ex +++ b/lib/kith/duplicate_detection.ex @@ -1,7 +1,9 @@ defmodule Kith.DuplicateDetection do import Ecto.Query, warn: false import Kith.Scope + alias Kith.Contacts.Contact alias Kith.Contacts.DuplicateCandidate + alias Kith.DuplicateDetection.Cluster alias Kith.Repo @default_page_size 20 @@ -198,4 +200,130 @@ defmodule Kith.DuplicateDetection do |> Repo.insert!() end end + + @doc """ + Lists derived duplicate clusters, highest confidence first. + + Options: `:limit` (default #{@default_page_size}), `:offset` (default 0). + """ + def list_clusters(account_id, opts \\ []) do + limit = Keyword.get(opts, :limit, @default_page_size) + offset = Keyword.get(opts, :offset, 0) + + account_id + |> build_clusters() + |> Enum.drop(offset) + |> Enum.take(limit) + end + + @doc "How many clusters are pending review." + def cluster_count(account_id), do: account_id |> build_clusters() |> length() + + defp build_clusters(account_id) do + candidates = + DuplicateCandidate + |> scope_to_account(account_id) + |> where([d], d.status in ["pending", "dismissed"]) + |> Repo.all() + + {pending, dismissed} = Enum.split_with(candidates, &(&1.status == "pending")) + + negatives = + MapSet.new(dismissed, fn d -> {d.contact_id, d.duplicate_contact_id} end) + + pending + # Fully deterministic: scores come from a small fixed set, so ties are the + # common case. Without the id tiebreak, which edge wins would depend on + # arbitrary row order and clustering would vary between identical runs. + |> Enum.sort_by(&{-&1.score, &1.contact_id, &1.duplicate_contact_id}) + |> Enum.reduce(%{groups: %{}, of: %{}, next: 0}, &union_pair(&2, &1, negatives)) + |> to_clusters(pending) + end + + defp union_pair(acc, pair, negatives) do + {acc, group_a} = ensure_group(acc, pair.contact_id) + {acc, group_b} = ensure_group(acc, pair.duplicate_contact_id) + + cond do + group_a == group_b -> acc + blocked?(acc, group_a, group_b, negatives) -> acc + true -> merge_groups(acc, group_a, group_b) + end + end + + defp ensure_group(acc, contact_id) do + case Map.fetch(acc.of, contact_id) do + {:ok, group_id} -> + {acc, group_id} + + :error -> + group_id = acc.next + + acc = %{ + acc + | groups: Map.put(acc.groups, group_id, MapSet.new([contact_id])), + of: Map.put(acc.of, contact_id, group_id), + next: group_id + 1 + } + + {acc, group_id} + end + end + + # A dismissed pair is a negative edge: the user already reviewed those two + # contacts and rejected the match, so no third contact may reunite them by + # transitivity. + defp blocked?(acc, group_a, group_b, negatives) do + members_a = Map.fetch!(acc.groups, group_a) + members_b = Map.fetch!(acc.groups, group_b) + + Enum.any?(negatives, fn {one, two} -> + (MapSet.member?(members_a, one) and MapSet.member?(members_b, two)) or + (MapSet.member?(members_a, two) and MapSet.member?(members_b, one)) + end) + end + + defp merge_groups(acc, group_a, group_b) do + members_b = Map.fetch!(acc.groups, group_b) + members = MapSet.union(Map.fetch!(acc.groups, group_a), members_b) + + of = Enum.reduce(members_b, acc.of, &Map.put(&2, &1, group_a)) + + %{acc | groups: acc.groups |> Map.put(group_a, members) |> Map.delete(group_b), of: of} + end + + defp to_clusters(acc, pending) do + contacts = + acc.of + |> Map.keys() + |> then(fn ids -> from(c in Contact, where: c.id in ^ids) |> Repo.all() end) + |> Map.new(&{&1.id, &1}) + + acc.groups + |> Enum.filter(fn {_id, members} -> MapSet.size(members) >= 2 end) + |> Enum.map(fn {_id, members} -> build_cluster(members, pending, contacts) end) + |> Enum.sort_by(&{-&1.max_score, &1.key}) + end + + defp build_cluster(members, pending, contacts) do + pairs = + Enum.filter(pending, fn p -> + MapSet.member?(members, p.contact_id) and + MapSet.member?(members, p.duplicate_contact_id) + end) + + member_contacts = + members + |> Enum.map(&Map.get(contacts, &1)) + |> Enum.reject(&is_nil/1) + |> Enum.sort_by(& &1.id) + + %Cluster{ + key: members |> Enum.min(), + contacts: member_contacts, + pairs: pairs, + max_score: pairs |> Enum.map(& &1.score) |> Enum.max(fn -> 0.0 end), + reasons: pairs |> Enum.flat_map(& &1.reasons) |> Enum.uniq() + } + end end diff --git a/lib/kith/duplicate_detection/cluster.ex b/lib/kith/duplicate_detection/cluster.ex new file mode 100644 index 00000000..6e4fde92 --- /dev/null +++ b/lib/kith/duplicate_detection/cluster.ex @@ -0,0 +1,19 @@ +defmodule Kith.DuplicateDetection.Cluster do + @moduledoc """ + A derived group of contacts that detection believes are the same person. + + Clusters have no database row — they are computed from `duplicate_candidates` + on every read, so they cannot drift from the pairs that justify them. `key` is + the lowest member contact id and is used for routing. + """ + + defstruct [:key, :contacts, :pairs, :max_score, :reasons] + + @type t :: %__MODULE__{ + key: integer(), + contacts: [Kith.Contacts.Contact.t()], + pairs: [Kith.Contacts.DuplicateCandidate.t()], + max_score: float(), + reasons: [String.t()] + } +end diff --git a/test/kith/duplicate_detection_test.exs b/test/kith/duplicate_detection_test.exs index 6660c840..34931b5a 100644 --- a/test/kith/duplicate_detection_test.exs +++ b/test/kith/duplicate_detection_test.exs @@ -441,4 +441,131 @@ defmodule Kith.DuplicateDetectionTest do assert Enum.all?(merged_rows, fn {one, two} -> one != nil or two != nil end) end end + + describe "list_clusters/2" do + setup do + Kith.ContactsFixtures.seed_reference_data!() + user = Kith.AccountsFixtures.user_fixture() + account_id = user.account_id + + contacts = + Map.new([a: "Ann", b: "Bea", c: "Cal", d: "Dee", e: "Eve"], fn {key, name} -> + {key, Kith.ContactsFixtures.contact_fixture(account_id, %{first_name: name})} + end) + + %{user: user, account_id: account_id, contacts: contacts} + end + + defp candidate!(account_id, one, two, opts \\ []) do + {low, high} = if one.id < two.id, do: {one, two}, else: {two, one} + + Repo.insert!(%Kith.Contacts.DuplicateCandidate{ + account_id: account_id, + contact_id: low.id, + duplicate_contact_id: high.id, + score: Keyword.get(opts, :score, 0.9), + reasons: Keyword.get(opts, :reasons, ["email_match"]), + status: Keyword.get(opts, :status, "pending"), + detected_at: DateTime.utc_now(:second) + }) + end + + defp member_ids(cluster), do: cluster.contacts |> Enum.map(& &1.id) |> Enum.sort() + + test "transitive pairs collapse into one cluster", ctx do + %{a: a, b: b, c: c} = ctx.contacts + candidate!(ctx.account_id, a, b) + candidate!(ctx.account_id, b, c) + + assert [cluster] = Kith.DuplicateDetection.list_clusters(ctx.account_id) + assert member_ids(cluster) == Enum.sort([a.id, b.id, c.id]) + end + + test "disjoint pairs stay separate", ctx do + %{a: a, b: b, d: d, e: e} = ctx.contacts + candidate!(ctx.account_id, a, b) + candidate!(ctx.account_id, d, e) + + clusters = Kith.DuplicateDetection.list_clusters(ctx.account_id) + + assert length(clusters) == 2 + end + + test "clusters are ordered by their highest score", ctx do + %{a: a, b: b, d: d, e: e} = ctx.contacts + candidate!(ctx.account_id, a, b, score: 0.6) + candidate!(ctx.account_id, d, e, score: 0.95) + + [first, second] = Kith.DuplicateDetection.list_clusters(ctx.account_id) + + assert first.max_score == 0.95 + assert second.max_score == 0.6 + end + + test "a cluster carries the union of its pair reasons", ctx do + %{a: a, b: b, c: c} = ctx.contacts + candidate!(ctx.account_id, a, b, reasons: ["email_match"]) + candidate!(ctx.account_id, b, c, reasons: ["phone_match", "email_match"]) + + assert [cluster] = Kith.DuplicateDetection.list_clusters(ctx.account_id) + assert Enum.sort(cluster.reasons) == ["email_match", "phone_match"] + end + + test "the key is the lowest member id", ctx do + %{a: a, b: b} = ctx.contacts + candidate!(ctx.account_id, a, b) + + assert [cluster] = Kith.DuplicateDetection.list_clusters(ctx.account_id) + assert cluster.key == Enum.min([a.id, b.id]) + end + + test "a dismissed pair blocks a transitive union", ctx do + %{a: a, b: b, c: c} = ctx.contacts + # A and C were reviewed and rejected; B links to both. + candidate!(ctx.account_id, a, c, status: "dismissed") + candidate!(ctx.account_id, a, b, score: 0.9) + candidate!(ctx.account_id, b, c, score: 0.6) + + clusters = Kith.DuplicateDetection.list_clusters(ctx.account_id) + + assert [cluster] = clusters + assert member_ids(cluster) == Enum.sort([a.id, b.id]) + refute c.id in member_ids(cluster) + end + + test "the stronger edge wins when a dismissal blocks the weaker", ctx do + %{a: a, b: b, c: c} = ctx.contacts + candidate!(ctx.account_id, a, c, status: "dismissed") + candidate!(ctx.account_id, a, b, score: 0.6) + candidate!(ctx.account_id, b, c, score: 0.9) + + assert [cluster] = Kith.DuplicateDetection.list_clusters(ctx.account_id) + assert member_ids(cluster) == Enum.sort([b.id, c.id]) + end + + test "tied scores cluster deterministically", ctx do + %{a: a, b: b, c: c} = ctx.contacts + candidate!(ctx.account_id, a, b, score: 0.85) + candidate!(ctx.account_id, b, c, score: 0.85) + + first = Kith.DuplicateDetection.list_clusters(ctx.account_id) |> Enum.map(&member_ids/1) + + for _ <- 1..5 do + assert Enum.map(Kith.DuplicateDetection.list_clusters(ctx.account_id), &member_ids/1) == + first + end + end + + test "paginates over clusters, not pairs", ctx do + %{a: a, b: b, d: d, e: e} = ctx.contacts + candidate!(ctx.account_id, a, b, score: 0.95) + candidate!(ctx.account_id, d, e, score: 0.6) + + assert [one] = Kith.DuplicateDetection.list_clusters(ctx.account_id, limit: 1) + assert one.max_score == 0.95 + + assert [two] = Kith.DuplicateDetection.list_clusters(ctx.account_id, limit: 1, offset: 1) + assert two.max_score == 0.6 + end + end end From 07286ca5669cc7670e6fabb923a06ee5129da131 Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Tue, 18 Aug 2026 11:17:11 +0300 Subject: [PATCH 02/27] fix(duplicates): tenancy-scope cluster contact lookup, fix vacuous determinism test - to_clusters/3 now scopes the contact lookup by account_id (defence in depth; the ids it reads were already account-scoped upstream) - move the >= 2 member-count gate to after contact resolution so it can't disagree with the reject(&is_nil/1) that follows it - the determinism test previously asserted nothing about the id tiebreak (no dismissal meant both tied edges unioned regardless of order); it now dismisses a third pair so the tied edges compete, and inserts them in reverse id order so DB row order can't accidentally produce the result - add cross-account isolation, cluster_count/1, and key-ascending tiebreak tests --- lib/kith/duplicate_detection.ex | 10 +++--- test/kith/duplicate_detection_test.exs | 49 +++++++++++++++++++++++++- 2 files changed, 54 insertions(+), 5 deletions(-) diff --git a/lib/kith/duplicate_detection.ex b/lib/kith/duplicate_detection.ex index 401ad2fa..31191c11 100644 --- a/lib/kith/duplicate_detection.ex +++ b/lib/kith/duplicate_detection.ex @@ -237,7 +237,7 @@ defmodule Kith.DuplicateDetection do # arbitrary row order and clustering would vary between identical runs. |> Enum.sort_by(&{-&1.score, &1.contact_id, &1.duplicate_contact_id}) |> Enum.reduce(%{groups: %{}, of: %{}, next: 0}, &union_pair(&2, &1, negatives)) - |> to_clusters(pending) + |> to_clusters(account_id, pending) end defp union_pair(acc, pair, negatives) do @@ -292,16 +292,18 @@ defmodule Kith.DuplicateDetection do %{acc | groups: acc.groups |> Map.put(group_a, members) |> Map.delete(group_b), of: of} end - defp to_clusters(acc, pending) do + defp to_clusters(acc, account_id, pending) do contacts = acc.of |> Map.keys() - |> then(fn ids -> from(c in Contact, where: c.id in ^ids) |> Repo.all() end) + |> then(fn ids -> + Contact |> scope_to_account(account_id) |> where([c], c.id in ^ids) |> Repo.all() + end) |> Map.new(&{&1.id, &1}) acc.groups - |> Enum.filter(fn {_id, members} -> MapSet.size(members) >= 2 end) |> Enum.map(fn {_id, members} -> build_cluster(members, pending, contacts) end) + |> Enum.filter(fn cluster -> length(cluster.contacts) >= 2 end) |> Enum.sort_by(&{-&1.max_score, &1.key}) end diff --git a/test/kith/duplicate_detection_test.exs b/test/kith/duplicate_detection_test.exs index 34931b5a..93decdab 100644 --- a/test/kith/duplicate_detection_test.exs +++ b/test/kith/duplicate_detection_test.exs @@ -545,8 +545,17 @@ defmodule Kith.DuplicateDetectionTest do test "tied scores cluster deterministically", ctx do %{a: a, b: b, c: c} = ctx.contacts - candidate!(ctx.account_id, a, b, score: 0.85) + # A and C were dismissed, so (a,b) and (b,c) compete for B: only one can + # win. With equal scores, the id-based tiebreak in the sort key decides + # which one — (a,b) sorts first because a.id < b.id. Inserted in + # reverse (b,c) then (a,b) order so DB/insertion order alone cannot + # produce that result — only the explicit id tiebreak can. + candidate!(ctx.account_id, a, c, status: "dismissed") candidate!(ctx.account_id, b, c, score: 0.85) + candidate!(ctx.account_id, a, b, score: 0.85) + + assert [cluster] = Kith.DuplicateDetection.list_clusters(ctx.account_id) + assert member_ids(cluster) == Enum.sort([a.id, b.id]) first = Kith.DuplicateDetection.list_clusters(ctx.account_id) |> Enum.map(&member_ids/1) @@ -567,5 +576,43 @@ defmodule Kith.DuplicateDetectionTest do assert [two] = Kith.DuplicateDetection.list_clusters(ctx.account_id, limit: 1, offset: 1) assert two.max_score == 0.6 end + + test "clusters never cross accounts", ctx do + %{a: a, b: b} = ctx.contacts + candidate!(ctx.account_id, a, b) + + other_user = Kith.AccountsFixtures.user_fixture() + other_account_id = other_user.account_id + + other_x = Kith.ContactsFixtures.contact_fixture(other_account_id, %{first_name: "Xan"}) + other_y = Kith.ContactsFixtures.contact_fixture(other_account_id, %{first_name: "Yan"}) + candidate!(other_account_id, other_x, other_y) + + clusters = Kith.DuplicateDetection.list_clusters(ctx.account_id) + + assert [cluster] = clusters + assert member_ids(cluster) == Enum.sort([a.id, b.id]) + refute other_x.id in member_ids(cluster) + refute other_y.id in member_ids(cluster) + end + + test "cluster_count/1 counts derived clusters", ctx do + %{a: a, b: b, d: d, e: e} = ctx.contacts + candidate!(ctx.account_id, a, b) + candidate!(ctx.account_id, d, e) + + assert Kith.DuplicateDetection.cluster_count(ctx.account_id) == 2 + end + + test "equal max_score breaks the tie by ascending key", ctx do + %{a: a, b: b, d: d, e: e} = ctx.contacts + candidate!(ctx.account_id, a, b, score: 0.8) + candidate!(ctx.account_id, d, e, score: 0.8) + + [first, second] = Kith.DuplicateDetection.list_clusters(ctx.account_id) + + assert first.max_score == second.max_score + assert first.key < second.key + end end end From b422e335f4770c870e67d1b56ae4129b0dcbbdca Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Tue, 18 Aug 2026 15:13:03 +0300 Subject: [PATCH 03/27] feat(duplicates): add cluster lookup, primary selection and clique dismissal --- lib/kith/duplicate_detection.ex | 144 +++++++++++++++++++++ test/kith/duplicate_detection_test.exs | 168 +++++++++++++++++++++++++ 2 files changed, 312 insertions(+) diff --git a/lib/kith/duplicate_detection.ex b/lib/kith/duplicate_detection.ex index 31191c11..24ba1303 100644 --- a/lib/kith/duplicate_detection.ex +++ b/lib/kith/duplicate_detection.ex @@ -224,6 +224,10 @@ defmodule Kith.DuplicateDetection do DuplicateCandidate |> scope_to_account(account_id) |> where([d], d.status in ["pending", "dismissed"]) + |> join(:inner, [d], one in Contact, on: one.id == d.contact_id) + |> join(:inner, [d, _one], two in Contact, on: two.id == d.duplicate_contact_id) + |> where([d, one, two], is_nil(one.deleted_at) and is_nil(two.deleted_at)) + |> select([d], d) |> Repo.all() {pending, dismissed} = Enum.split_with(candidates, &(&1.status == "pending")) @@ -328,4 +332,144 @@ defmodule Kith.DuplicateDetection do reasons: pairs |> Enum.flat_map(& &1.reasons) |> Enum.uniq() } end + + @doc """ + Finds the cluster containing `contact_id`, or `nil`. + + Takes any member id rather than only the cluster key, so a bookmark survives + the key shifting when a lower-id member joins. + """ + def get_cluster(account_id, contact_id) do + account_id + |> build_clusters() + |> Enum.find(fn cluster -> + Enum.any?(cluster.contacts, &(&1.id == contact_id)) + end) + end + + @count_schemas [ + Kith.Contacts.Note, + Kith.Contacts.Address, + Kith.Contacts.ContactField, + Kith.Contacts.Document, + Kith.Contacts.Photo, + Kith.Activities.Call, + Kith.Activities.LifeEvent + ] + + @doc """ + The member that should survive by default: the one holding the most attached + records, tie-broken by earliest creation. + + Moving the fewest rows is the cheap part; the real reason is that the richest, + oldest record is the id external clients (CardDAV, Immich) are already pinned + to. + """ + def default_primary(contacts) do + ids = Enum.map(contacts, & &1.id) + counts = attached_counts(ids) + + Enum.max_by(contacts, fn contact -> + {Map.get(counts, contact.id, 0), -DateTime.to_unix(contact.inserted_at)} + end) + end + + defp attached_counts(ids) do + schema_counts = + Enum.reduce(@count_schemas, %{}, fn schema, acc -> + from(r in schema, + where: r.contact_id in ^ids, + group_by: r.contact_id, + select: {r.contact_id, count(r.id)} + ) + |> Repo.all() + |> Enum.reduce(acc, fn {id, n}, acc -> Map.update(acc, id, n, &(&1 + n)) end) + end) + + from(ac in "activity_contacts", + where: ac.contact_id in ^ids, + group_by: ac.contact_id, + select: {ac.contact_id, count()} + ) + |> Repo.all() + |> Enum.reduce(schema_counts, fn {id, n}, acc -> Map.update(acc, id, n, &(&1 + n)) end) + end + + @doc """ + Records that the selected members are not duplicates of each other. + + Writes the full clique over `selected_ids` rather than only the pairs + detection happened to produce: a cluster is often a chain (A–B, B–C), and + dismissing only those leaves no negative edge between A and C. Pairs between + two unchecked members are never touched — the user excluded them, and + excluding is not a statement about them. + + `selected_ids` and `unchecked_ids` are caller-supplied, so every id is + verified to belong to `account_id` before anything is written — the + candidate row's foreign key only proves the contact exists, not that it's + this account's, and a bad caller could otherwise write a dismissal row + referencing another account's contact. Ids that don't resolve to this + account are silently dropped from their list rather than raising, since + this is a bulk action driven by UI selection state, not a single-resource + lookup. + """ + def dismiss_selection(account_id, selected_ids, unchecked_ids) do + now = DateTime.utc_now() |> DateTime.truncate(:second) + + selected_ids = owned_ids(account_id, selected_ids) + unchecked_ids = owned_ids(account_id, unchecked_ids) + + cliques = + for one <- selected_ids, two <- selected_ids, one < two, do: {one, two} + + crosses = + for one <- selected_ids, two <- unchecked_ids, one != two do + if one < two, do: {one, two}, else: {two, one} + end + + (cliques ++ crosses) + |> Enum.uniq() + |> Enum.each(fn {low, high} -> upsert_dismissed(account_id, low, high, now) end) + + :ok + end + + defp owned_ids(_account_id, []), do: [] + + defp owned_ids(account_id, ids) do + Contact + |> scope_to_account(account_id) + |> where([c], c.id in ^ids) + |> select([c], c.id) + |> Repo.all() + end + + defp upsert_dismissed(account_id, low, high, now) do + case Repo.one( + from(d in DuplicateCandidate, + where: + d.account_id == ^account_id and d.contact_id == ^low and + d.duplicate_contact_id == ^high + ) + ) do + nil -> + %DuplicateCandidate{account_id: account_id} + |> DuplicateCandidate.changeset(%{ + contact_id: low, + duplicate_contact_id: high, + score: 0.0, + reasons: ["user_rejected"], + status: "dismissed", + detected_at: now, + resolved_at: now + }) + |> Repo.insert!() + + %DuplicateCandidate{status: "merged"} = existing -> + existing + + existing -> + existing |> DuplicateCandidate.dismiss_changeset() |> Repo.update!() + end + end end diff --git a/test/kith/duplicate_detection_test.exs b/test/kith/duplicate_detection_test.exs index 93decdab..14c39171 100644 --- a/test/kith/duplicate_detection_test.exs +++ b/test/kith/duplicate_detection_test.exs @@ -615,4 +615,172 @@ defmodule Kith.DuplicateDetectionTest do assert first.key < second.key end end + + describe "get_cluster/2 and trashed members" do + setup do + Kith.ContactsFixtures.seed_reference_data!() + user = Kith.AccountsFixtures.user_fixture() + + contacts = + Map.new([a: "Ann", b: "Bea", c: "Cal"], fn {key, name} -> + {key, Kith.ContactsFixtures.contact_fixture(user.account_id, %{first_name: name})} + end) + + %{user: user, account_id: user.account_id, contacts: contacts} + end + + test "any member id resolves to the cluster", ctx do + %{a: a, b: b, c: c} = ctx.contacts + candidate!(ctx.account_id, a, b) + candidate!(ctx.account_id, b, c) + + for member <- [a, b, c] do + cluster = Kith.DuplicateDetection.get_cluster(ctx.account_id, member.id) + assert cluster + assert member.id in Enum.map(cluster.contacts, & &1.id) + end + end + + test "a contact in no cluster returns nil", ctx do + assert Kith.DuplicateDetection.get_cluster(ctx.account_id, ctx.contacts.a.id) == nil + end + + test "a trashed member is excluded and a one-member cluster disappears", ctx do + %{a: a, b: b} = ctx.contacts + candidate!(ctx.account_id, a, b) + + Repo.update_all(from(c in Kith.Contacts.Contact, where: c.id == ^b.id), + set: [deleted_at: DateTime.utc_now(:second)] + ) + + assert Kith.DuplicateDetection.list_clusters(ctx.account_id) == [] + assert Kith.DuplicateDetection.get_cluster(ctx.account_id, a.id) == nil + end + + test "a trashed member is dropped but a three-member cluster survives", ctx do + %{a: a, b: b, c: c} = ctx.contacts + candidate!(ctx.account_id, a, b) + candidate!(ctx.account_id, b, c) + + Repo.update_all(from(x in Kith.Contacts.Contact, where: x.id == ^c.id), + set: [deleted_at: DateTime.utc_now(:second)] + ) + + assert [cluster] = Kith.DuplicateDetection.list_clusters(ctx.account_id) + assert Enum.map(cluster.contacts, & &1.id) |> Enum.sort() == Enum.sort([a.id, b.id]) + end + end + + describe "default_primary/1" do + setup do + Kith.ContactsFixtures.seed_reference_data!() + user = Kith.AccountsFixtures.user_fixture() + %{user: user, account_id: user.account_id} + end + + test "picks the member with the most attached records", ctx do + thin = Kith.ContactsFixtures.contact_fixture(ctx.account_id, %{first_name: "Thin"}) + rich = Kith.ContactsFixtures.contact_fixture(ctx.account_id, %{first_name: "Rich"}) + + Kith.ContactsFixtures.note_fixture(rich, ctx.user.id) + Kith.ContactsFixtures.note_fixture(rich, ctx.user.id) + Kith.ContactsFixtures.address_fixture(rich) + Kith.ContactsFixtures.note_fixture(thin, ctx.user.id) + + assert Kith.DuplicateDetection.default_primary([thin, rich]).id == rich.id + end + + test "breaks ties toward the earliest created", ctx do + older = Kith.ContactsFixtures.contact_fixture(ctx.account_id, %{first_name: "Older"}) + newer = Kith.ContactsFixtures.contact_fixture(ctx.account_id, %{first_name: "Newer"}) + + Repo.update_all(from(c in Kith.Contacts.Contact, where: c.id == ^older.id), + set: [inserted_at: ~U[2020-01-01 00:00:00Z]] + ) + + older = Repo.get!(Kith.Contacts.Contact, older.id) + + assert Kith.DuplicateDetection.default_primary([newer, older]).id == older.id + end + end + + describe "dismiss_selection/3" do + setup do + Kith.ContactsFixtures.seed_reference_data!() + user = Kith.AccountsFixtures.user_fixture() + + contacts = + Map.new([a: "Ann", b: "Bea", c: "Cal", d: "Dee", e: "Eve"], fn {key, name} -> + {key, Kith.ContactsFixtures.contact_fixture(user.account_id, %{first_name: name})} + end) + + %{account_id: user.account_id, contacts: contacts} + end + + test "writes the full clique over the selected members", ctx do + %{a: a, b: b, c: c} = ctx.contacts + # A chain: only two pairs exist, but all three combinations must end up + # dismissed or a later contact could reunite A and C by transitivity. + candidate!(ctx.account_id, a, b) + candidate!(ctx.account_id, b, c) + + :ok = Kith.DuplicateDetection.dismiss_selection(ctx.account_id, [a.id, b.id, c.id], []) + + assert status_of(ctx.account_id, a, b) == "dismissed" + assert status_of(ctx.account_id, b, c) == "dismissed" + assert status_of(ctx.account_id, a, c) == "dismissed" + end + + test "dismisses selected-to-unchecked but never unchecked-to-unchecked", ctx do + %{a: a, b: b, d: d, e: e} = ctx.contacts + candidate!(ctx.account_id, a, b) + candidate!(ctx.account_id, a, d) + candidate!(ctx.account_id, d, e) + + :ok = + Kith.DuplicateDetection.dismiss_selection(ctx.account_id, [a.id, b.id], [d.id, e.id]) + + assert status_of(ctx.account_id, a, d) == "dismissed" + assert status_of(ctx.account_id, d, e) == "pending" + end + + test "rejects a selected id belonging to another account and writes nothing for it", ctx do + %{a: a, b: b} = ctx.contacts + other_user = Kith.AccountsFixtures.user_fixture() + foreign = Kith.ContactsFixtures.contact_fixture(other_user.account_id, %{first_name: "Fox"}) + + candidate!(ctx.account_id, a, b) + + :ok = + Kith.DuplicateDetection.dismiss_selection(ctx.account_id, [a.id, b.id, foreign.id], []) + + count = + Repo.aggregate( + from(d in Kith.Contacts.DuplicateCandidate, + where: d.contact_id == ^foreign.id or d.duplicate_contact_id == ^foreign.id + ), + :count + ) + + assert count == 0 + end + + test "rejects an unchecked id belonging to another account and writes nothing for it", ctx do + %{a: a, b: b} = ctx.contacts + other_user = Kith.AccountsFixtures.user_fixture() + foreign = Kith.ContactsFixtures.contact_fixture(other_user.account_id, %{first_name: "Fox"}) + + :ok = Kith.DuplicateDetection.dismiss_selection(ctx.account_id, [a.id, b.id], [foreign.id]) + + count = + Repo.aggregate( + from(d in Kith.Contacts.DuplicateCandidate, + where: d.contact_id == ^foreign.id or d.duplicate_contact_id == ^foreign.id + ), + :count + ) + + assert count == 0 + end + end end From 8c6e29b8cfbc439d9584ee0873a74e172bbb1f8c Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Tue, 18 Aug 2026 15:17:56 +0300 Subject: [PATCH 04/27] fix(duplicates): cover merged-preservation branch, transactional clique writes, id tiebreak --- lib/kith/duplicate_detection.ex | 26 ++++++++++++++++++++------ test/kith/duplicate_detection_test.exs | 10 ++++++++++ 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/lib/kith/duplicate_detection.ex b/lib/kith/duplicate_detection.ex index 24ba1303..78e1a071 100644 --- a/lib/kith/duplicate_detection.ex +++ b/lib/kith/duplicate_detection.ex @@ -359,18 +359,24 @@ defmodule Kith.DuplicateDetection do @doc """ The member that should survive by default: the one holding the most attached - records, tie-broken by earliest creation. + records, tie-broken by earliest creation, then by lowest id. Moving the fewest rows is the cheap part; the real reason is that the richest, oldest record is the id external clients (CardDAV, Immich) are already pinned - to. + to. The id tiebreak makes the choice deterministic on its own — without it, + a tie on both count and `inserted_at` (easy with second-precision timestamps + and a bulk import) would resolve by whatever order `contacts` happened to be + passed in, an accident of the caller rather than a rule. + + `contacts` must be non-empty; callers only ever pass cluster members, and a + cluster always has at least two. """ def default_primary(contacts) do ids = Enum.map(contacts, & &1.id) counts = attached_counts(ids) Enum.max_by(contacts, fn contact -> - {Map.get(counts, contact.id, 0), -DateTime.to_unix(contact.inserted_at)} + {Map.get(counts, contact.id, 0), -DateTime.to_unix(contact.inserted_at), -contact.id} end) end @@ -412,6 +418,12 @@ defmodule Kith.DuplicateDetection do account are silently dropped from their list rather than raising, since this is a bulk action driven by UI selection state, not a single-resource lookup. + + The whole batch of pairs is written inside one transaction. Without it, a + mid-loop failure (e.g. a unique-constraint race against a concurrent + dismissal) would leave the clique half-written — some rejected matches + recorded, others not — which for negative edges reopens exactly the + transitivity hole the clique exists to close. """ def dismiss_selection(account_id, selected_ids, unchecked_ids) do now = DateTime.utc_now() |> DateTime.truncate(:second) @@ -427,9 +439,11 @@ defmodule Kith.DuplicateDetection do if one < two, do: {one, two}, else: {two, one} end - (cliques ++ crosses) - |> Enum.uniq() - |> Enum.each(fn {low, high} -> upsert_dismissed(account_id, low, high, now) end) + Repo.transaction(fn -> + (cliques ++ crosses) + |> Enum.uniq() + |> Enum.each(fn {low, high} -> upsert_dismissed(account_id, low, high, now) end) + end) :ok end diff --git a/test/kith/duplicate_detection_test.exs b/test/kith/duplicate_detection_test.exs index 14c39171..2077b0c4 100644 --- a/test/kith/duplicate_detection_test.exs +++ b/test/kith/duplicate_detection_test.exs @@ -741,9 +741,19 @@ defmodule Kith.DuplicateDetectionTest do Kith.DuplicateDetection.dismiss_selection(ctx.account_id, [a.id, b.id], [d.id, e.id]) assert status_of(ctx.account_id, a, d) == "dismissed" + assert status_of(ctx.account_id, b, d) == "dismissed" assert status_of(ctx.account_id, d, e) == "pending" end + test "leaves a merged pair alone rather than downgrading it to dismissed", ctx do + %{a: a, b: b} = ctx.contacts + candidate!(ctx.account_id, a, b, status: "merged") + + :ok = Kith.DuplicateDetection.dismiss_selection(ctx.account_id, [a.id, b.id], []) + + assert status_of(ctx.account_id, a, b) == "merged" + end + test "rejects a selected id belonging to another account and writes nothing for it", ctx do %{a: a, b: b} = ctx.contacts other_user = Kith.AccountsFixtures.user_fixture() From c838223d1b502626176a4737fc450cef94bb6042 Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Tue, 18 Aug 2026 15:22:18 +0300 Subject: [PATCH 05/27] feat(contacts): summarise multi-valued records and history for a merge --- lib/kith/contacts/merge_summary.ex | 147 ++++++++++++++++++++++ test/kith/contacts/merge_summary_test.exs | 97 ++++++++++++++ 2 files changed, 244 insertions(+) create mode 100644 lib/kith/contacts/merge_summary.ex create mode 100644 test/kith/contacts/merge_summary_test.exs diff --git a/lib/kith/contacts/merge_summary.ex b/lib/kith/contacts/merge_summary.ex new file mode 100644 index 00000000..39d89b83 --- /dev/null +++ b/lib/kith/contacts/merge_summary.ex @@ -0,0 +1,147 @@ +defmodule Kith.Contacts.MergeSummary do + @moduledoc """ + Gathers the multi-valued records and history counts a merge screen shows. + + Read-only. Duplicates are marked rather than removed so the screen can show + what the merge will collapse instead of silently dropping it. + """ + + import Ecto.Query, warn: false + + alias Kith.Contacts.{Address, ContactField, Document, Note, Photo} + alias Kith.Repo + + @history [ + notes: Note, + documents: Document, + photos: Photo, + calls: Kith.Activities.Call, + life_events: Kith.Activities.LifeEvent, + reminders: Kith.Reminders.Reminder, + gifts: Kith.Contacts.Gift, + debts: Kith.Contacts.Debt, + pets: Kith.Contacts.Pet, + tasks: Kith.Tasks.Task, + conversations: Kith.Conversations.Conversation, + relationships: Kith.Contacts.Relationship + ] + + @doc """ + Builds the summary for `members`. + + Every `contact_fields`/`addresses`/`tags`/`aliases` entry also carries a + `key` — the value `mark_duplicates/1` grouped it by to decide `duplicate?`. + This is not part of the documented entry shape but is kept intentionally: + `Kith.Contacts.Merge`'s `:dedupe_owned` step runs before `:apply_drop`, so a + survivor row and a loser row with an equal, normalized value are + equivalent — dropping only the one the user clicked can leave its + equivalent behind and the value comes back. Callers building a drop list + must expand a dropped entry to every entry sharing its `key`, not just the + one row shown as a duplicate. + """ + def build(members) do + ids = Enum.map(members, & &1.id) + + %{ + contact_fields: contact_fields(ids), + addresses: addresses(ids), + tags: tags(ids), + aliases: aliases(members), + history: history(ids) + } + end + + defp contact_fields(ids) do + from(f in ContactField, + where: f.contact_id in ^ids, + join: t in assoc(f, :contact_field_type), + order_by: [asc: f.id], + select: {f, t.name} + ) + |> Repo.all() + |> Enum.map(fn {field, name} -> + %{ + id: field.id, + label: name, + value: field.value, + owner_id: field.contact_id, + key: {field.contact_field_type_id, normalize(field.value)} + } + end) + |> mark_duplicates() + end + + defp addresses(ids) do + from(a in Address, where: a.contact_id in ^ids, order_by: [asc: a.id]) + |> Repo.all() + |> Enum.map(fn address -> + %{ + id: address.id, + label: address.label || "Address", + value: Enum.join(Enum.reject([address.line1, address.city], &is_nil/1), ", "), + owner_id: address.contact_id, + key: {normalize(address.line1), normalize(address.postal_code)} + } + end) + |> mark_duplicates() + end + + defp tags(ids) do + from(ct in "contact_tags", + join: t in Kith.Contacts.Tag, + on: t.id == ct.tag_id, + where: ct.contact_id in ^ids, + order_by: [asc: t.name], + select: {ct.contact_id, t.id, t.name} + ) + |> Repo.all() + |> Enum.map(fn {owner_id, tag_id, name} -> + %{id: tag_id, label: "Tag", value: name, owner_id: owner_id, key: tag_id} + end) + |> mark_duplicates() + end + + # Aliases are unioned into a single set, unlike contact_fields/addresses/tags + # (which keep duplicate rows, marked, since each row is independently + # droppable). An alias array has no per-row identity to drop, so repeats + # across members are folded away here rather than surfaced as duplicate?. + defp aliases(members) do + members + |> Enum.flat_map(fn member -> + Enum.map( + member.aliases || [], + &%{id: &1, label: "Alias", value: &1, owner_id: member.id, key: &1} + ) + end) + |> mark_duplicates() + |> Enum.reject(& &1.duplicate?) + end + + defp history(ids) do + counts = + Map.new(@history, fn {key, schema} -> + {key, Repo.aggregate(from(r in schema, where: r.contact_id in ^ids), :count)} + end) + + activities = + from(ac in "activity_contacts", where: ac.contact_id in ^ids, select: count()) + |> Repo.one() + + Map.put(counts, :activities, activities) + end + + # The first occurrence of a key wins; later ones are what the merge collapses. + # `key` is kept on the returned entry — see the moduledoc on `build/1`. + defp mark_duplicates(entries) do + {marked, _seen} = + Enum.map_reduce(entries, MapSet.new(), fn entry, seen -> + duplicate? = MapSet.member?(seen, entry.key) + {Map.put(entry, :duplicate?, duplicate?), MapSet.put(seen, entry.key)} + end) + + marked + end + + defp normalize(nil), do: nil + defp normalize(value) when is_binary(value), do: value |> String.trim() |> String.downcase() +end diff --git a/test/kith/contacts/merge_summary_test.exs b/test/kith/contacts/merge_summary_test.exs new file mode 100644 index 00000000..ea33fc5e --- /dev/null +++ b/test/kith/contacts/merge_summary_test.exs @@ -0,0 +1,97 @@ +defmodule Kith.Contacts.MergeSummaryTest do + use Kith.DataCase, async: false + + import Ecto.Query + + alias Kith.Contacts.MergeSummary + alias Kith.{AccountsFixtures, ContactsFixtures} + + setup do + ContactsFixtures.seed_reference_data!() + user = AccountsFixtures.user_fixture() + + email_type = + Repo.one!( + from(t in Kith.Contacts.ContactFieldType, where: like(t.protocol, "mailto%"), limit: 1) + ) + + a = ContactsFixtures.contact_fixture(user.account_id, %{first_name: "Sarah"}) + b = ContactsFixtures.contact_fixture(user.account_id, %{first_name: "Sarah"}) + + %{user: user, account_id: user.account_id, email_type: email_type, a: a, b: b} + end + + test "combines contact fields and marks the repeat as a duplicate", ctx do + ContactsFixtures.contact_field_fixture(ctx.a, ctx.email_type.id, %{"value" => "s@example.com"}) + + ContactsFixtures.contact_field_fixture(ctx.b, ctx.email_type.id, %{"value" => "s@example.com"}) + + ContactsFixtures.contact_field_fixture(ctx.b, ctx.email_type.id, %{ + "value" => "other@example.com" + }) + + summary = MergeSummary.build([ctx.a, ctx.b]) + + assert length(summary.contact_fields) == 3 + assert Enum.count(summary.contact_fields, & &1.duplicate?) == 1 + + kept = Enum.reject(summary.contact_fields, & &1.duplicate?) + assert Enum.sort(Enum.map(kept, & &1.value)) == ["other@example.com", "s@example.com"] + end + + test "the first occurrence is kept and the later one marked duplicate", ctx do + first = + ContactsFixtures.contact_field_fixture(ctx.a, ctx.email_type.id, %{ + "value" => "s@example.com" + }) + + second = + ContactsFixtures.contact_field_fixture(ctx.b, ctx.email_type.id, %{ + "value" => "s@example.com" + }) + + summary = MergeSummary.build([ctx.a, ctx.b]) + + assert Enum.find(summary.contact_fields, &(&1.id == first.id)).duplicate? == false + assert Enum.find(summary.contact_fields, &(&1.id == second.id)).duplicate? == true + end + + test "addresses dedupe on line1 and postal code", ctx do + ContactsFixtures.address_fixture(ctx.a, %{"line1" => "1 Main St", "postal_code" => "94110"}) + + ContactsFixtures.address_fixture(ctx.b, %{"line1" => " 1 main st ", "postal_code" => "94110"}) + + summary = MergeSummary.build([ctx.a, ctx.b]) + + assert Enum.count(summary.addresses, & &1.duplicate?) == 1 + end + + test "aliases are unioned across members", ctx do + Repo.update_all(from(c in Kith.Contacts.Contact, where: c.id == ^ctx.a.id), + set: [aliases: ["Sarah K."]] + ) + + Repo.update_all(from(c in Kith.Contacts.Contact, where: c.id == ^ctx.b.id), + set: [aliases: ["Sarah K.", "SK"]] + ) + + a = Repo.get!(Kith.Contacts.Contact, ctx.a.id) + b = Repo.get!(Kith.Contacts.Contact, ctx.b.id) + + summary = MergeSummary.build([a, b]) + + assert Enum.map(summary.aliases, & &1.value) |> Enum.sort() == ["SK", "Sarah K."] + end + + test "history counts every type across every member", ctx do + ContactsFixtures.note_fixture(ctx.a, ctx.user.id) + ContactsFixtures.note_fixture(ctx.b, ctx.user.id) + ContactsFixtures.document_fixture(ctx.b) + + summary = MergeSummary.build([ctx.a, ctx.b]) + + assert summary.history.notes == 2 + assert summary.history.documents == 1 + assert summary.history.calls == 0 + end +end From 24ff3828409089a66ea6c26cf6d4679a7b3ecf77 Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Tue, 18 Aug 2026 15:26:31 +0300 Subject: [PATCH 06/27] fix(contacts): guard blank-key addresses in merge summary from false-collapsing Mirror Kith.Contacts.Merge's dedupe_owned_step/2 blank-key guard: an address with neither line1 nor postal_code must not be treated as a duplicate of another blank-key address, since they can differ by city/region/country. --- lib/kith/contacts/merge_summary.ex | 21 ++++++++++++++++++++- test/kith/contacts/merge_summary_test.exs | 18 ++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/lib/kith/contacts/merge_summary.ex b/lib/kith/contacts/merge_summary.ex index 39d89b83..c541b623 100644 --- a/lib/kith/contacts/merge_summary.ex +++ b/lib/kith/contacts/merge_summary.ex @@ -80,12 +80,31 @@ defmodule Kith.Contacts.MergeSummary do label: address.label || "Address", value: Enum.join(Enum.reject([address.line1, address.city], &is_nil/1), ", "), owner_id: address.contact_id, - key: {normalize(address.line1), normalize(address.postal_code)} + key: address_key(address) } end) |> mark_duplicates() end + # Mirrors the blank-key guard in Kith.Contacts.Merge's dedupe_owned_step/2: + # an address with neither line1 nor postal_code isn't a duplicate of + # another address in the same shape (they may differ by city/region/ + # country, e.g. "Paris" vs "Tokyo"), so give each a key that can't collide. + defp address_key(address) do + line1 = normalize(address.line1) + postal_code = normalize(address.postal_code) + + if blank?(line1) and blank?(postal_code) do + {:no_key, address.id} + else + {line1, postal_code} + end + end + + defp blank?(nil), do: true + defp blank?(""), do: true + defp blank?(_), do: false + defp tags(ids) do from(ct in "contact_tags", join: t in Kith.Contacts.Tag, diff --git a/test/kith/contacts/merge_summary_test.exs b/test/kith/contacts/merge_summary_test.exs index ea33fc5e..1b0896cc 100644 --- a/test/kith/contacts/merge_summary_test.exs +++ b/test/kith/contacts/merge_summary_test.exs @@ -66,6 +66,24 @@ defmodule Kith.Contacts.MergeSummaryTest do assert Enum.count(summary.addresses, & &1.duplicate?) == 1 end + test "addresses with no line1 or postal code are never collapsed into each other", ctx do + ContactsFixtures.address_fixture(ctx.a, %{ + "line1" => nil, + "postal_code" => nil, + "city" => "Paris" + }) + + ContactsFixtures.address_fixture(ctx.b, %{ + "line1" => nil, + "postal_code" => nil, + "city" => "Tokyo" + }) + + summary = MergeSummary.build([ctx.a, ctx.b]) + + assert Enum.count(summary.addresses, & &1.duplicate?) == 0 + end + test "aliases are unioned across members", ctx do Repo.update_all(from(c in Kith.Contacts.Contact, where: c.id == ^ctx.a.id), set: [aliases: ["Sarah K."]] From 8ce0a518959776d14f5f5465b9cac07bff1417bf Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Tue, 18 Aug 2026 15:29:49 +0300 Subject: [PATCH 07/27] feat(contacts): expose candidate values for any mergeable field --- lib/kith/contacts/merge_resolution.ex | 14 +++++++++++ test/kith/contacts/merge_resolution_test.exs | 26 ++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/lib/kith/contacts/merge_resolution.ex b/lib/kith/contacts/merge_resolution.ex index f3d696d5..816e76f6 100644 --- a/lib/kith/contacts/merge_resolution.ex +++ b/lib/kith/contacts/merge_resolution.ex @@ -139,6 +139,20 @@ defmodule Kith.Contacts.MergeResolution do end end + @doc """ + Every distinct value `members` hold for `field`, most-held first. + + Unlike `conflicts`, this is populated even when the members agree — the screen + uses it to open an already-resolved row as a choice. + """ + def candidates_for(members, field) do + members + |> Enum.map(fn member -> {member.id, normalize(Map.fetch!(member, field))} end) + |> Enum.reject(fn {_id, value} -> is_nil(value) end) + |> candidates() + |> Enum.sort_by(& &1.count, :desc) + end + # Returns {resolved_value, attribution, conflict_candidates} defp resolve_scalar(members, field) do held = diff --git a/test/kith/contacts/merge_resolution_test.exs b/test/kith/contacts/merge_resolution_test.exs index 91fb6924..5dca036c 100644 --- a/test/kith/contacts/merge_resolution_test.exs +++ b/test/kith/contacts/merge_resolution_test.exs @@ -320,4 +320,30 @@ defmodule Kith.Contacts.MergeResolutionTest do refute Enum.any?([a, b], &(&1.immich_status == "unlinked")) end end + + describe "candidates_for/2" do + test "a resolved field still offers its single value", ctx do + a = contact(ctx.account_id, %{first_name: "Sarah"}) + b = contact(ctx.account_id, %{first_name: "Sarah"}) + + assert [%{value: "Sarah", count: 2}] = + MergeResolution.candidates_for([a, b], :first_name) + end + + test "an empty field offers nothing", ctx do + a = contact(ctx.account_id, %{first_name: "Sarah", occupation: nil}) + b = contact(ctx.account_id, %{first_name: "Sarah", occupation: nil}) + + assert MergeResolution.candidates_for([a, b], :occupation) == [] + end + + test "a contested field offers every value, most-held first", ctx do + a = contact(ctx.account_id, %{first_name: "Sarah", company: "Figma"}) + b = contact(ctx.account_id, %{first_name: "Sarah", company: "Stripe"}) + c = contact(ctx.account_id, %{first_name: "Sarah", company: "Stripe"}) + + assert [%{value: "Stripe", count: 2}, %{value: "Figma", count: 1}] = + MergeResolution.candidates_for([a, b, c], :company) + end + end end From 7d580588d500c710cf90ca18adfa806282dd09b8 Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Tue, 18 Aug 2026 15:34:05 +0300 Subject: [PATCH 08/27] fix(contacts): break candidates_for/2 ties on lowest member id --- lib/kith/contacts/merge_resolution.ex | 6 ++++-- test/kith/contacts/merge_resolution_test.exs | 13 +++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/lib/kith/contacts/merge_resolution.ex b/lib/kith/contacts/merge_resolution.ex index 816e76f6..0f30aff5 100644 --- a/lib/kith/contacts/merge_resolution.ex +++ b/lib/kith/contacts/merge_resolution.ex @@ -140,7 +140,9 @@ defmodule Kith.Contacts.MergeResolution do end @doc """ - Every distinct value `members` hold for `field`, most-held first. + Every distinct value `members` hold for `field`, most-held first, ties broken + by the lowest member id holding that value. Callers may rely on this order + being stable across renders. Unlike `conflicts`, this is populated even when the members agree — the screen uses it to open an already-resolved row as a choice. @@ -150,7 +152,7 @@ defmodule Kith.Contacts.MergeResolution do |> Enum.map(fn member -> {member.id, normalize(Map.fetch!(member, field))} end) |> Enum.reject(fn {_id, value} -> is_nil(value) end) |> candidates() - |> Enum.sort_by(& &1.count, :desc) + |> Enum.sort_by(&{-&1.count, Enum.min(&1.member_ids)}) end # Returns {resolved_value, attribution, conflict_candidates} diff --git a/test/kith/contacts/merge_resolution_test.exs b/test/kith/contacts/merge_resolution_test.exs index 5dca036c..eb10f2ad 100644 --- a/test/kith/contacts/merge_resolution_test.exs +++ b/test/kith/contacts/merge_resolution_test.exs @@ -345,5 +345,18 @@ defmodule Kith.Contacts.MergeResolutionTest do assert [%{value: "Stripe", count: 2}, %{value: "Figma", count: 1}] = MergeResolution.candidates_for([a, b, c], :company) end + + test "a tie on count breaks toward the lowest member id, not value ordering", ctx do + # a is created (and so gets the lower id) first but holds the + # alphabetically-later value — this makes term/map iteration order + # disagree with id order, so the assertion only passes if the sort + # genuinely breaks ties on member id rather than falling out of + # candidates/1's incidental ordering. + a = contact(ctx.account_id, %{first_name: "Sarah", company: "Zeta"}) + b = contact(ctx.account_id, %{first_name: "Sarah", company: "Alpha"}) + + assert [%{value: "Zeta", count: 1}, %{value: "Alpha", count: 1}] = + MergeResolution.candidates_for([a, b], :company) + end end end From 7e1c0d67ede6af87d232559f35c197d94f4a082e Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Tue, 18 Aug 2026 15:38:11 +0300 Subject: [PATCH 09/27] feat(duplicates): list clusters instead of pairs --- lib/kith_web/live/contact_live/index.ex | 43 +++---- .../live/contact_live/index.html.heex | 109 ++++++------------ lib/kith_web/router.ex | 1 + lib/kith_web/user_auth.ex | 2 +- .../contact_live/duplicates_index_test.exs | 62 ++++++++++ 5 files changed, 114 insertions(+), 103 deletions(-) create mode 100644 test/kith_web/live/contact_live/duplicates_index_test.exs diff --git a/lib/kith_web/live/contact_live/index.ex b/lib/kith_web/live/contact_live/index.ex index 491b9b4b..c6e32b52 100644 --- a/lib/kith_web/live/contact_live/index.ex +++ b/lib/kith_web/live/contact_live/index.ex @@ -32,9 +32,9 @@ defmodule KithWeb.ContactLive.Index do |> assign(:contacts, []) |> assign(:meta, nil) |> assign(:tags, Contacts.list_tags(account_id)) - |> assign(:candidates, []) - |> assign(:duplicates_total, 0) - |> assign(:duplicates_has_more, false) + |> assign(:clusters, []) + |> assign(:clusters_total, 0) + |> assign(:clusters_has_more, false) |> assign(:trashed_contacts, [])} end @@ -61,14 +61,14 @@ defmodule KithWeb.ContactLive.Index do defp apply_action(socket, :duplicates, _params) do account_id = socket.assigns.account_id - candidates = DuplicateDetection.list_candidates(account_id, limit: @duplicates_page_size) - total_count = DuplicateDetection.pending_count(account_id) + clusters = DuplicateDetection.list_clusters(account_id, limit: @duplicates_page_size) + total = DuplicateDetection.cluster_count(account_id) socket |> assign(:page_title, "Duplicate Contacts") - |> assign(:candidates, candidates) - |> assign(:duplicates_total, total_count) - |> assign(:duplicates_has_more, length(candidates) >= @duplicates_page_size) + |> assign(:clusters, clusters) + |> assign(:clusters_total, total) + |> assign(:clusters_has_more, length(clusters) >= @duplicates_page_size) end defp apply_action(socket, :trash, _params) do @@ -247,36 +247,19 @@ defmodule KithWeb.ContactLive.Index do # ── Duplicates events ────────────────────────────────────────────────── - def handle_event("dismiss", %{"id" => id}, socket) do - candidate = - DuplicateDetection.get_candidate!(socket.assigns.account_id, String.to_integer(id)) - - {:ok, _} = DuplicateDetection.dismiss_candidate(candidate) - - candidates = Enum.reject(socket.assigns.candidates, &(&1.id == candidate.id)) - total = socket.assigns.duplicates_total - 1 - - {:noreply, - socket - |> assign(:candidates, candidates) - |> assign(:duplicates_total, total) - |> assign(:pending_duplicates_count, total) - |> put_flash(:info, "Duplicate dismissed.")} - end - def handle_event("load_more_duplicates", _params, socket) do - offset = length(socket.assigns.candidates) + account_id = socket.assigns.account_id more = - DuplicateDetection.list_candidates(socket.assigns.account_id, + DuplicateDetection.list_clusters(account_id, limit: @duplicates_page_size, - offset: offset + offset: length(socket.assigns.clusters) ) {:noreply, socket - |> assign(:candidates, socket.assigns.candidates ++ more) - |> assign(:duplicates_has_more, length(more) >= @duplicates_page_size)} + |> assign(:clusters, socket.assigns.clusters ++ more) + |> assign(:clusters_has_more, length(more) >= @duplicates_page_size)} end def handle_event("scan", _params, socket) do diff --git a/lib/kith_web/live/contact_live/index.html.heex b/lib/kith_web/live/contact_live/index.html.heex index 3db183fc..af292da4 100644 --- a/lib/kith_web/live/contact_live/index.html.heex +++ b/lib/kith_web/live/contact_live/index.html.heex @@ -379,11 +379,9 @@ <%!-- Duplicates panel --%> <%= if @live_action == :duplicates do %>
-
-

- {@duplicates_total} potential duplicate{if @duplicates_total != 1, do: "s"} found -

-
+

+ {@clusters_total} possible duplicate{if @clusters_total != 1, do: "s"} found +

- <%= if @candidates == [] do %> + <%= if @clusters == [] do %> <% else %>
- <%= for candidate <- @candidates do %> -
-
-
- = 0.8 && - "bg-red-100 text-red-700 dark:bg-red-900/30 dark:text-red-400", - candidate.score >= 0.5 && candidate.score < 0.8 && - "bg-yellow-100 text-yellow-700 dark:bg-yellow-900/30 dark:text-yellow-400", - candidate.score < 0.5 && - "bg-blue-100 text-blue-700 dark:bg-blue-900/30 dark:text-blue-400" - ]}> - {Float.round(candidate.score * 100, 0)}% match - - - {Enum.join(candidate.reasons, ", ")} - -
+ <.link + :for={cluster <- @clusters} + navigate={~p"/contacts/duplicates/cluster/#{cluster.key}"} + class="block rounded-[var(--radius-lg)] border border-[var(--color-border)] bg-[var(--color-surface-elevated)] shadow-sm p-4 hover:border-[var(--color-border-focus)] transition-colors" + > +
+
+ + {round(cluster.max_score * 100)}% match + - Detected + {Enum.join(cluster.reasons, ", ")}
- -
-
- <.link - navigate={~p"/contacts/#{candidate.contact.id}"} - class="text-sm font-semibold text-[var(--color-accent)] hover:underline" - > - {candidate.contact.display_name} - -
-
- <.link - navigate={~p"/contacts/#{candidate.duplicate_contact.id}"} - class="text-sm font-semibold text-[var(--color-accent)] hover:underline" - > - {candidate.duplicate_contact.display_name} - -
-
- -
- <.link - navigate={ - ~p"/contacts/#{candidate.contact.id}/merge?with=#{candidate.duplicate_contact.id}&candidate_id=#{candidate.id}" - } - class="inline-flex items-center gap-1.5 rounded-[var(--radius-md)] bg-[var(--color-accent)] text-[var(--color-accent-foreground)] px-3 py-1.5 text-xs font-medium hover:bg-[var(--color-accent-hover)] transition-colors" - > - <.icon name="hero-arrows-right-left" class="size-4" /> Merge - - -
+ + {length(cluster.contacts)} contacts +
- <% end %> - <%= if @duplicates_has_more do %> -
- + + + {contact.display_name} + +
- <% end %> + + +
+ +
<% end %> <% end %> diff --git a/lib/kith_web/router.ex b/lib/kith_web/router.ex index 79f81b10..c1327612 100644 --- a/lib/kith_web/router.ex +++ b/lib/kith_web/router.ex @@ -350,6 +350,7 @@ defmodule KithWeb.Router do live "/contacts/archived", ContactLive.Index, :archived live "/contacts/trash", ContactLive.Index, :trash live "/contacts/duplicates", ContactLive.Index, :duplicates + live "/contacts/duplicates/cluster/:id", ContactLive.ClusterMerge, :show live "/contacts/new", ContactLive.New, :new live "/contacts/:id", ContactLive.Show, :show live "/contacts/:id/edit", ContactLive.Edit, :edit diff --git a/lib/kith_web/user_auth.ex b/lib/kith_web/user_auth.ex index 06df9ca3..31bc3c23 100644 --- a/lib/kith_web/user_auth.ex +++ b/lib/kith_web/user_auth.ex @@ -231,7 +231,7 @@ defmodule KithWeb.UserAuth do Phoenix.Component.assign( socket, :pending_duplicates_count, - DuplicateDetection.pending_count(account_id) + DuplicateDetection.cluster_count(account_id) )} else socket = diff --git a/test/kith_web/live/contact_live/duplicates_index_test.exs b/test/kith_web/live/contact_live/duplicates_index_test.exs new file mode 100644 index 00000000..8a6c13e4 --- /dev/null +++ b/test/kith_web/live/contact_live/duplicates_index_test.exs @@ -0,0 +1,62 @@ +defmodule KithWeb.ContactLive.DuplicatesIndexTest do + use KithWeb.ConnCase + + import Phoenix.LiveViewTest + import Ecto.Query + + alias Kith.{AccountsFixtures, ContactsFixtures} + + setup %{conn: conn} do + ContactsFixtures.seed_reference_data!() + user = AccountsFixtures.user_fixture() + %{conn: log_in_user(conn, user), user: user, account_id: user.account_id} + end + + defp candidate!(account_id, one, two, score \\ 0.9) do + {low, high} = if one.id < two.id, do: {one, two}, else: {two, one} + + Kith.Repo.insert!(%Kith.Contacts.DuplicateCandidate{ + account_id: account_id, + contact_id: low.id, + duplicate_contact_id: high.id, + score: score, + reasons: ["email_match"], + status: "pending", + detected_at: DateTime.utc_now(:second) + }) + end + + test "four duplicates render as one cluster entry", ctx do + [a, b, c, d] = + for name <- ~w(Ann Bea Cal Dee) do + ContactsFixtures.contact_fixture(ctx.account_id, %{first_name: name}) + end + + candidate!(ctx.account_id, a, b) + candidate!(ctx.account_id, b, c) + candidate!(ctx.account_id, c, d) + + {:ok, _live, html} = live(ctx.conn, ~p"/contacts/duplicates") + + assert html =~ "1 possible duplicate" + assert html =~ "4 contacts" + + for name <- ~w(Ann Bea Cal Dee), do: assert(html =~ name) + end + + test "each cluster links to its cluster screen", ctx do + a = ContactsFixtures.contact_fixture(ctx.account_id, %{first_name: "Ann"}) + b = ContactsFixtures.contact_fixture(ctx.account_id, %{first_name: "Bea"}) + candidate!(ctx.account_id, a, b) + + {:ok, _live, html} = live(ctx.conn, ~p"/contacts/duplicates") + + assert html =~ "/contacts/duplicates/cluster/#{min(a.id, b.id)}" + end + + test "shows the empty state when nothing is pending", ctx do + {:ok, _live, html} = live(ctx.conn, ~p"/contacts/duplicates") + + assert html =~ "No duplicates found" + end +end From f6b2361672d2a964e6101f8dddc60f527eb9d4db Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Tue, 18 Aug 2026 15:48:19 +0300 Subject: [PATCH 10/27] perf(duplicates): index dismissed pairs and derive clusters once per page blocked?/4 was scanning every dismissed candidate on each union check, making cluster derivation O(pending x dismissed); dismissed rows are never deleted, so that term grows for the account's lifetime while pending stays small. Index dismissed pairs as contact_id => MapSet of partner ids and walk the smaller of the two merging groups instead. Also scope the candidate query's contact joins to the account so resolvability is enforced in SQL, give cluster_count/1 a contacts-free counting path, add list_clusters_page/2 so the duplicates page derives clusters once instead of twice, and fix a has-more false positive when the total lands exactly on a page boundary. Test gaps closed: load_more_duplicates exercised at the LiveView level, and a dismissed pair verified to keep its two contacts out of the same rendered cluster. --- lib/kith/duplicate_detection.ex | 105 +++++++++++++++--- lib/kith_web/live/contact_live/index.ex | 13 ++- .../contact_live/duplicates_index_test.exs | 50 ++++++++- 3 files changed, 143 insertions(+), 25 deletions(-) diff --git a/lib/kith/duplicate_detection.ex b/lib/kith/duplicate_detection.ex index 78e1a071..ca51445d 100644 --- a/lib/kith/duplicate_detection.ex +++ b/lib/kith/duplicate_detection.ex @@ -216,32 +216,92 @@ defmodule Kith.DuplicateDetection do |> Enum.take(limit) end - @doc "How many clusters are pending review." - def cluster_count(account_id), do: account_id |> build_clusters() |> length() + @doc """ + Lists a page of derived clusters together with the total cluster count, from + a single cluster derivation. + + Prefer this over separate `list_clusters/2` + `cluster_count/1` calls when + both are needed for the same request (e.g. rendering a page and its total), + since each derivation re-runs the candidate query and the union-find pass. + + Options: `:limit` (default #{@default_page_size}), `:offset` (default 0). + """ + def list_clusters_page(account_id, opts \\ []) do + limit = Keyword.get(opts, :limit, @default_page_size) + offset = Keyword.get(opts, :offset, 0) + + clusters = build_clusters(account_id) + + %{clusters: clusters |> Enum.drop(offset) |> Enum.take(limit), total: length(clusters)} + end + + @doc """ + How many clusters are pending review. + + Counts groups directly from the union-find derivation rather than building + full `%Cluster{}` structs, so it never queries contacts. + """ + def cluster_count(account_id) do + {acc, _pending} = derive_groups(account_id) + Enum.count(acc.groups, fn {_id, members} -> MapSet.size(members) >= 2 end) + end defp build_clusters(account_id) do + {acc, pending} = derive_groups(account_id) + to_clusters(acc, account_id, pending) + end + + defp derive_groups(account_id) do candidates = DuplicateCandidate |> scope_to_account(account_id) |> where([d], d.status in ["pending", "dismissed"]) - |> join(:inner, [d], one in Contact, on: one.id == d.contact_id) - |> join(:inner, [d, _one], two in Contact, on: two.id == d.duplicate_contact_id) + |> join(:inner, [d], one in Contact, + on: one.id == d.contact_id and one.account_id == ^account_id + ) + |> join(:inner, [d, one], two in Contact, + on: two.id == d.duplicate_contact_id and two.account_id == ^account_id + ) |> where([d, one, two], is_nil(one.deleted_at) and is_nil(two.deleted_at)) |> select([d], d) |> Repo.all() {pending, dismissed} = Enum.split_with(candidates, &(&1.status == "pending")) - negatives = - MapSet.new(dismissed, fn d -> {d.contact_id, d.duplicate_contact_id} end) + negatives = index_negatives(dismissed) + + acc = + pending + # Fully deterministic: scores come from a small fixed set, so ties are + # the common case. Without the id tiebreak, which edge wins would + # depend on arbitrary row order and clustering would vary between + # identical runs. + |> Enum.sort_by(&{-&1.score, &1.contact_id, &1.duplicate_contact_id}) + |> Enum.reduce(%{groups: %{}, of: %{}, next: 0}, &union_pair(&2, &1, negatives)) + + {acc, pending} + end - pending - # Fully deterministic: scores come from a small fixed set, so ties are the - # common case. Without the id tiebreak, which edge wins would depend on - # arbitrary row order and clustering would vary between identical runs. - |> Enum.sort_by(&{-&1.score, &1.contact_id, &1.duplicate_contact_id}) - |> Enum.reduce(%{groups: %{}, of: %{}, next: 0}, &union_pair(&2, &1, negatives)) - |> to_clusters(account_id, pending) + # Dismissed pairs indexed as contact_id => the set of contact ids it was + # dismissed against, so `blocked?/4` can look up membership instead of + # scanning every dismissed row. Dismissed rows are permanent negative edges + # (never deleted), so that set grows for the life of the account while the + # pending set stays small — without this index `blocked?/4` was the + # dominant cost of every cluster derivation. + defp index_negatives(dismissed) do + Enum.reduce(dismissed, %{}, fn d, acc -> + acc + |> Map.update( + d.contact_id, + MapSet.new([d.duplicate_contact_id]), + &MapSet.put(&1, d.duplicate_contact_id) + ) + |> Map.update( + d.duplicate_contact_id, + MapSet.new([d.contact_id]), + &MapSet.put(&1, d.contact_id) + ) + end) end defp union_pair(acc, pair, negatives) do @@ -276,14 +336,25 @@ defmodule Kith.DuplicateDetection do # A dismissed pair is a negative edge: the user already reviewed those two # contacts and rejected the match, so no third contact may reunite them by - # transitivity. + # transitivity. Walks the smaller of the two member sets, doing a map + # lookup + set-disjoint check per member against the larger set — O(min(|a|, + # |b|)) instead of scanning every dismissed pair in the account. defp blocked?(acc, group_a, group_b, negatives) do members_a = Map.fetch!(acc.groups, group_a) members_b = Map.fetch!(acc.groups, group_b) - Enum.any?(negatives, fn {one, two} -> - (MapSet.member?(members_a, one) and MapSet.member?(members_b, two)) or - (MapSet.member?(members_a, two) and MapSet.member?(members_b, one)) + {small, large} = + if MapSet.size(members_a) <= MapSet.size(members_b) do + {members_a, members_b} + else + {members_b, members_a} + end + + Enum.any?(small, fn id -> + case Map.fetch(negatives, id) do + {:ok, partners} -> not MapSet.disjoint?(partners, large) + :error -> false + end end) end diff --git a/lib/kith_web/live/contact_live/index.ex b/lib/kith_web/live/contact_live/index.ex index c6e32b52..98a5646b 100644 --- a/lib/kith_web/live/contact_live/index.ex +++ b/lib/kith_web/live/contact_live/index.ex @@ -61,14 +61,15 @@ defmodule KithWeb.ContactLive.Index do defp apply_action(socket, :duplicates, _params) do account_id = socket.assigns.account_id - clusters = DuplicateDetection.list_clusters(account_id, limit: @duplicates_page_size) - total = DuplicateDetection.cluster_count(account_id) + + %{clusters: clusters, total: total} = + DuplicateDetection.list_clusters_page(account_id, limit: @duplicates_page_size) socket |> assign(:page_title, "Duplicate Contacts") |> assign(:clusters, clusters) |> assign(:clusters_total, total) - |> assign(:clusters_has_more, length(clusters) >= @duplicates_page_size) + |> assign(:clusters_has_more, total > length(clusters)) end defp apply_action(socket, :trash, _params) do @@ -256,10 +257,12 @@ defmodule KithWeb.ContactLive.Index do offset: length(socket.assigns.clusters) ) + clusters = socket.assigns.clusters ++ more + {:noreply, socket - |> assign(:clusters, socket.assigns.clusters ++ more) - |> assign(:clusters_has_more, length(more) >= @duplicates_page_size)} + |> assign(:clusters, clusters) + |> assign(:clusters_has_more, socket.assigns.clusters_total > length(clusters))} end def handle_event("scan", _params, socket) do diff --git a/test/kith_web/live/contact_live/duplicates_index_test.exs b/test/kith_web/live/contact_live/duplicates_index_test.exs index 8a6c13e4..3bbc3ac8 100644 --- a/test/kith_web/live/contact_live/duplicates_index_test.exs +++ b/test/kith_web/live/contact_live/duplicates_index_test.exs @@ -2,7 +2,6 @@ defmodule KithWeb.ContactLive.DuplicatesIndexTest do use KithWeb.ConnCase import Phoenix.LiveViewTest - import Ecto.Query alias Kith.{AccountsFixtures, ContactsFixtures} @@ -12,7 +11,7 @@ defmodule KithWeb.ContactLive.DuplicatesIndexTest do %{conn: log_in_user(conn, user), user: user, account_id: user.account_id} end - defp candidate!(account_id, one, two, score \\ 0.9) do + defp candidate!(account_id, one, two, status \\ "pending", score \\ 0.9) do {low, high} = if one.id < two.id, do: {one, two}, else: {two, one} Kith.Repo.insert!(%Kith.Contacts.DuplicateCandidate{ @@ -21,7 +20,8 @@ defmodule KithWeb.ContactLive.DuplicatesIndexTest do duplicate_contact_id: high.id, score: score, reasons: ["email_match"], - status: "pending", + status: status, + resolved_at: if(status != "pending", do: DateTime.utc_now(:second)), detected_at: DateTime.utc_now(:second) }) end @@ -59,4 +59,48 @@ defmodule KithWeb.ContactLive.DuplicatesIndexTest do assert html =~ "No duplicates found" end + + test "load_more_duplicates loads the next page of clusters", ctx do + pairs = + for i <- 1..21 do + a = ContactsFixtures.contact_fixture(ctx.account_id, %{first_name: "A#{i}"}) + b = ContactsFixtures.contact_fixture(ctx.account_id, %{first_name: "B#{i}"}) + candidate!(ctx.account_id, a, b) + {a, b} + end + + {last_a, _last_b} = List.last(pairs) + + {:ok, live, html} = live(ctx.conn, ~p"/contacts/duplicates") + + assert html =~ "21 possible duplicates" + assert html =~ "Load more" + refute html =~ last_a.display_name + + html = live |> element("button", "Load more") |> render_click() + + assert html =~ last_a.display_name + refute html =~ "Load more" + end + + test "a dismissed pair keeps two contacts out of the same rendered cluster", ctx do + a = ContactsFixtures.contact_fixture(ctx.account_id, %{first_name: "Ann"}) + b = ContactsFixtures.contact_fixture(ctx.account_id, %{first_name: "Bea"}) + c = ContactsFixtures.contact_fixture(ctx.account_id, %{first_name: "Cal"}) + + # Ann and Cal were reviewed and rejected as a match; Bea links to both, so + # the dismissal must block Bea's group from reuniting them by + # transitivity — Cal is left as an unrendered singleton. + candidate!(ctx.account_id, a, c, "dismissed") + candidate!(ctx.account_id, a, b) + candidate!(ctx.account_id, b, c) + + {:ok, _live, html} = live(ctx.conn, ~p"/contacts/duplicates") + + assert html =~ "1 possible duplicate" + assert html =~ "2 contacts" + assert html =~ a.display_name + assert html =~ b.display_name + refute html =~ c.display_name + end end From 39d209718f1df68e27128f9a7645c5f748740d83 Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Tue, 18 Aug 2026 15:54:32 +0300 Subject: [PATCH 11/27] feat(duplicates): render the cluster merge screen --- .../live/contact_live/cluster_merge.ex | 380 ++++++++++++++++++ .../live/contact_live/cluster_merge_test.exs | 101 +++++ 2 files changed, 481 insertions(+) create mode 100644 lib/kith_web/live/contact_live/cluster_merge.ex create mode 100644 test/kith_web/live/contact_live/cluster_merge_test.exs diff --git a/lib/kith_web/live/contact_live/cluster_merge.ex b/lib/kith_web/live/contact_live/cluster_merge.ex new file mode 100644 index 00000000..22129815 --- /dev/null +++ b/lib/kith_web/live/contact_live/cluster_merge.ex @@ -0,0 +1,380 @@ +defmodule KithWeb.ContactLive.ClusterMerge do + @moduledoc """ + Reviews and merges a cluster of duplicate contacts on one screen. + + All state is derived from the member selection: unchecking a member re-runs + `MergeResolution` over the remaining members and discards any explicit field + choices, so what is on screen is always a resolution of exactly the checked + set. + """ + + use KithWeb, :live_view + + alias Kith.Contacts.{MergeFields, MergeResolution, MergeSummary} + alias Kith.DuplicateDetection + alias Kith.Policy + + @impl true + def mount(_params, _session, socket), do: {:ok, assign(socket, :error, nil)} + + @impl true + def handle_params(%{"id" => id}, _uri, socket) do + scope = socket.assigns.current_scope + + cond do + not Policy.can?(scope.user, :update, :contact) -> + {:noreply, + socket + |> put_flash(:error, "You don't have permission to merge contacts") + |> push_navigate(to: ~p"/contacts")} + + cluster = find_cluster(scope.account.id, id) -> + {:noreply, load_cluster(socket, cluster)} + + true -> + {:noreply, + socket + |> put_flash(:error, "No duplicate cluster found for that contact") + |> push_navigate(to: ~p"/contacts")} + end + end + + # `id` comes straight from the URL and is not guaranteed numeric — a bad + # value (e.g. "/contacts/duplicates/cluster/abc") must land on the same + # "no cluster found" redirect as an unknown id rather than raising. + defp find_cluster(account_id, id) do + case Integer.parse(id) do + {contact_id, ""} -> DuplicateDetection.get_cluster(account_id, contact_id) + _ -> nil + end + end + + defp load_cluster(socket, cluster) do + members = cluster.contacts + primary = DuplicateDetection.default_primary(members) + + socket + |> assign(:page_title, "Merge duplicates") + |> assign(:cluster, cluster) + |> assign(:members, members) + |> assign(:selected_ids, MapSet.new(members, & &1.id)) + |> assign(:primary_id, primary.id) + |> assign(:overrides, %{}) + |> assign(:dropped, MapSet.new()) + |> assign(:error, nil) + |> recompute() + end + + # Everything downstream of the selection is derived, never patched in place. + defp recompute(socket) do + selected = selected_members(socket) + primary_id = ensure_primary(socket, selected) + + socket + |> assign(:primary_id, primary_id) + |> assign(:resolution, MergeResolution.resolve(selected, primary_id)) + |> assign(:summary, MergeSummary.build(selected)) + end + + defp selected_members(socket) do + Enum.filter(socket.assigns.members, &MapSet.member?(socket.assigns.selected_ids, &1.id)) + end + + defp ensure_primary(socket, selected) do + if Enum.any?(selected, &(&1.id == socket.assigns.primary_id)) do + socket.assigns.primary_id + else + case selected do + [] -> nil + members -> DuplicateDetection.default_primary(members).id + end + end + end + + # Temporary — Task 7 adds the real event handlers (toggle-member, set-primary, + # choose-field, toggle-value, not-duplicates, merge). Remove this clause then. + @impl true + def handle_event(_event, _params, socket), do: {:noreply, socket} + + # ── Rendering helpers ────────────────────────────────────────────────── + + defp effective(assigns, field) do + Map.get(assigns.overrides, field, Map.get(assigns.resolution.fields, field)) + end + + defp conflict?(assigns, field), do: Map.has_key?(assigns.resolution.conflicts, field) + + defp conflict_count(assigns) do + Enum.count(MergeFields.choice_fields(), &conflict?(assigns, &1)) + end + + defp candidates(assigns, field) do + MergeResolution.candidates_for(selected_from_assigns(assigns), field) + end + + defp selected_from_assigns(assigns) do + Enum.filter(assigns.members, &MapSet.member?(assigns.selected_ids, &1.id)) + end + + defp attribution_text(assigns, field) do + total = MapSet.size(assigns.selected_ids) + + case Map.get(assigns.resolution.attributions, field) do + :all_agree -> "all #{total} agree" + {:only, id} -> "only #{member_name(assigns, id)} has this" + {:some, n} -> "#{n} of #{total}" + _ -> nil + end + end + + defp member_name(assigns, id) do + case Enum.find(assigns.members, &(&1.id == id)) do + nil -> "unknown" + contact -> contact.display_name + end + end + + defp humanize(field) do + field |> Atom.to_string() |> String.replace("_id", "") |> String.replace("_", " ") + end + + defp display(nil), do: nil + defp display(:clear), do: nil + defp display(%Date{} = date), do: Date.to_iso8601(date) + defp display(%DateTime{} = at), do: DateTime.to_iso8601(at) + defp display(true), do: "Yes" + defp display(false), do: "No" + defp display(list) when is_list(list), do: Enum.join(list, ", ") + defp display(value), do: to_string(value) + + @impl true + def render(assigns) do + ~H""" + +
+
+ {@error} + <.link navigate={~p"/contacts/duplicates/cluster/#{@cluster.key}"} class="underline ms-2"> + Reload + +
+ + +
+
+

+ {length(@members)} possible duplicates · {MapSet.size(@selected_ids)} selected +

+

+ Matched on {Enum.join(@cluster.reasons, ", ")}. Uncheck anyone who isn't this person. +

+
+
+ +
+
+ + +
+
+
+ +
0} + class="rounded-[var(--radius-lg)] border border-[var(--color-border)] bg-[var(--color-surface-elevated)]" + > + + + Identity + + {length(MergeFields.choice_fields())} fields · click any value to change it + + 0} + class="rounded-full bg-[var(--color-danger-subtle)] text-[var(--color-danger)] px-2 py-0.5 text-xs font-medium" + > + {conflict_count(assigns)} need{if conflict_count(assigns) == 1, do: "s"} a decision + + + + +
+
+ {humanize(field)} +
+ +
+ +
+ +
+ + {display(effective(assigns, field))} + + + Not set on any contact + + + {attribution_text(assigns, field)} + +
+
+
+ +
+ + + Contact details + + {length(@summary.contact_fields)} fields, {length(@summary.addresses)} addresses, {length( + @summary.tags + )} tags combined + + + + +
+

{label}

+ +

None

+
+
+ +
+ + + Carried over as-is + + {@summary.history |> Map.values() |> Enum.sum()} records move to the primary + + + +
+ 0} + class="rounded-[var(--radius-sm)] border border-[var(--color-border-subtle)] bg-[var(--color-surface-sunken)] px-2.5 py-1 text-xs" + > + {count} {String.replace(to_string(key), "_", " ")} + +
+
+ + +
+

+ {MapSet.size(@selected_ids) - 1} contact(s) move to trash and stay recoverable for 30 days. + + Unchecked contacts will be marked as not duplicates and won't be suggested again. + +

+
+ Not duplicates + + Merge {MapSet.size(@selected_ids)} contacts + +
+
+
+
+
+ """ + end +end diff --git a/test/kith_web/live/contact_live/cluster_merge_test.exs b/test/kith_web/live/contact_live/cluster_merge_test.exs new file mode 100644 index 00000000..779bef17 --- /dev/null +++ b/test/kith_web/live/contact_live/cluster_merge_test.exs @@ -0,0 +1,101 @@ +defmodule KithWeb.ContactLive.ClusterMergeTest do + use KithWeb.ConnCase + + import Phoenix.LiveViewTest + + alias Kith.{AccountsFixtures, ContactsFixtures} + + setup %{conn: conn} do + ContactsFixtures.seed_reference_data!() + user = AccountsFixtures.user_fixture() + account_id = user.account_id + + a = + ContactsFixtures.contact_fixture(account_id, %{ + first_name: "Sarah", + last_name: "Kim", + company: "Figma" + }) + + b = + ContactsFixtures.contact_fixture(account_id, %{ + first_name: "Sarah", + last_name: "Kim", + company: "Stripe", + middle_name: "Jiyoung" + }) + + candidate!(account_id, a, b) + + %{conn: log_in_user(conn, user), user: user, account_id: account_id, a: a, b: b} + end + + defp candidate!(account_id, one, two) do + {low, high} = if one.id < two.id, do: {one, two}, else: {two, one} + + Kith.Repo.insert!(%Kith.Contacts.DuplicateCandidate{ + account_id: account_id, + contact_id: low.id, + duplicate_contact_id: high.id, + score: 0.9, + reasons: ["email_match"], + status: "pending", + detected_at: DateTime.utc_now(:second) + }) + end + + defp cluster_path(a, b), do: "/contacts/duplicates/cluster/#{min(a.id, b.id)}" + + test "renders every member with a checkbox", ctx do + {:ok, _live, html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + assert html =~ "Merge 2 contacts" + assert html =~ ~s(phx-value-id="#{ctx.a.id}") + assert html =~ ~s(phx-value-id="#{ctx.b.id}") + end + + test "renders an agreed field with its attribution and no choice", ctx do + {:ok, _live, html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + assert html =~ "all 2 agree" + assert html =~ "Sarah" + end + + test "renders a gap-filled field attributed to its only source", ctx do + {:ok, _live, html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + assert html =~ "Jiyoung" + assert html =~ "only" + end + + test "renders a contested field as a choice and flags the section", ctx do + {:ok, _live, html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + assert html =~ "Figma" + assert html =~ "Stripe" + assert html =~ "1 needs a decision" + end + + test "a section with a conflict is open, one without is folded", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + assert has_element?(live, "details#section-identity[open]") + refute has_element?(live, "details#section-contact-details[open]") + end + + test "required fields offer no Leave empty option", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + refute has_element?(live, "button[phx-value-field='first_name'][phx-value-index='clear']") + end + + test "an unknown contact id renders not found", ctx do + assert {:error, {:live_redirect, %{to: "/contacts"}}} = + live(ctx.conn, "/contacts/duplicates/cluster/0") + end + + test "a non-numeric contact id redirects instead of raising", ctx do + assert {:error, {:live_redirect, %{to: "/contacts"}}} = + live(ctx.conn, "/contacts/duplicates/cluster/abc") + end +end From 027c6c148d911e19d7665ccaa87b2d3b6c0e79c5 Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Tue, 18 Aug 2026 16:00:27 +0300 Subject: [PATCH 12/27] fix(duplicates): honor Leave empty carve-out, resolved-state badge, footer count --- .../live/contact_live/cluster_merge.ex | 33 +++++++++++++++- .../live/contact_live/cluster_merge_test.exs | 38 +++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/lib/kith_web/live/contact_live/cluster_merge.ex b/lib/kith_web/live/contact_live/cluster_merge.ex index 22129815..9a5c4104 100644 --- a/lib/kith_web/live/contact_live/cluster_merge.ex +++ b/lib/kith_web/live/contact_live/cluster_merge.ex @@ -147,6 +147,14 @@ defmodule KithWeb.ContactLive.ClusterMerge do defp display(list) when is_list(list), do: Enum.join(list, ", ") defp display(value), do: to_string(value) + defp trash_summary(assigns) do + case MapSet.size(assigns.selected_ids) - 1 do + n when n <= 0 -> "Nothing moves to trash." + 1 -> "1 contact moves to trash and stays recoverable for 30 days." + n -> "#{n} contacts move to trash and stay recoverable for 30 days." + end + end + @impl true def render(assigns) do ~H""" @@ -233,6 +241,12 @@ defmodule KithWeb.ContactLive.ClusterMerge do > {conflict_count(assigns)} need{if conflict_count(assigns) == 1, do: "s"} a decision + + all resolved + @@ -266,6 +280,23 @@ defmodule KithWeb.ContactLive.ClusterMerge do {display(candidate.value)} {candidate.count} record(s) +
@@ -360,7 +391,7 @@ defmodule KithWeb.ContactLive.ClusterMerge do

- {MapSet.size(@selected_ids) - 1} contact(s) move to trash and stay recoverable for 30 days. + {trash_summary(assigns)} Unchecked contacts will be marked as not duplicates and won't be suggested again. diff --git a/test/kith_web/live/contact_live/cluster_merge_test.exs b/test/kith_web/live/contact_live/cluster_merge_test.exs index 779bef17..bb9f2b95 100644 --- a/test/kith_web/live/contact_live/cluster_merge_test.exs +++ b/test/kith_web/live/contact_live/cluster_merge_test.exs @@ -89,6 +89,12 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do refute has_element?(live, "button[phx-value-field='first_name'][phx-value-index='clear']") end + test "clearable contested fields do offer a Leave empty option", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + assert has_element?(live, "button[phx-value-field='company'][phx-value-index='clear']") + end + test "an unknown contact id renders not found", ctx do assert {:error, {:live_redirect, %{to: "/contacts"}}} = live(ctx.conn, "/contacts/duplicates/cluster/0") @@ -98,4 +104,36 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do assert {:error, {:live_redirect, %{to: "/contacts"}}} = live(ctx.conn, "/contacts/duplicates/cluster/abc") end + + test "a resolved section says so instead of just staying folded", ctx do + c = + ContactsFixtures.contact_fixture(ctx.account_id, %{first_name: "Priya", last_name: "Nair"}) + + d = + ContactsFixtures.contact_fixture(ctx.account_id, %{first_name: "Priya", last_name: "Nair"}) + + candidate!(ctx.account_id, c, d) + + {:ok, _live, html} = live(ctx.conn, cluster_path(c, d)) + + assert html =~ "all resolved" + end + + test "a viewer hitting the cluster route is redirected to /contacts", ctx do + viewer = AccountsFixtures.user_fixture(%{role: "viewer"}) + viewer_conn = log_in_user(build_conn(), viewer) + + assert {:error, {:live_redirect, %{to: "/contacts"}}} = + live(viewer_conn, cluster_path(ctx.a, ctx.b)) + end + + test "a member id from another account does not resolve", ctx do + other_user = AccountsFixtures.user_fixture() + + other_contact = + ContactsFixtures.contact_fixture(other_user.account_id, %{first_name: "Other"}) + + assert {:error, {:live_redirect, %{to: "/contacts"}}} = + live(ctx.conn, "/contacts/duplicates/cluster/#{other_contact.id}") + end end From 172bc9689e218c21c73da6e13110ba678f689707 Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Tue, 18 Aug 2026 16:08:26 +0300 Subject: [PATCH 13/27] feat(duplicates): wire cluster merge screen interactions --- .../live/contact_live/cluster_merge.ex | 80 +++++++++- .../live/contact_live/cluster_merge_test.exs | 141 ++++++++++++++++++ 2 files changed, 218 insertions(+), 3 deletions(-) diff --git a/lib/kith_web/live/contact_live/cluster_merge.ex b/lib/kith_web/live/contact_live/cluster_merge.ex index 9a5c4104..ca8202bd 100644 --- a/lib/kith_web/live/contact_live/cluster_merge.ex +++ b/lib/kith_web/live/contact_live/cluster_merge.ex @@ -91,10 +91,84 @@ defmodule KithWeb.ContactLive.ClusterMerge do end end - # Temporary — Task 7 adds the real event handlers (toggle-member, set-primary, - # choose-field, toggle-value, not-duplicates, merge). Remove this clause then. @impl true - def handle_event(_event, _params, socket), do: {:noreply, socket} + def handle_event("toggle-member", %{"id" => id}, socket) do + id = String.to_integer(id) + + selected = + if MapSet.member?(socket.assigns.selected_ids, id) do + MapSet.delete(socket.assigns.selected_ids, id) + else + MapSet.put(socket.assigns.selected_ids, id) + end + + if MapSet.size(selected) == 0 do + # `MergeResolution.resolve/2` requires a non-empty member list, and there + # is no meaningful "merge nothing" state to render — refuse to uncheck + # the last remaining member instead of leaving a selection that would + # crash the next recompute. + {:noreply, socket} + else + {:noreply, + socket + |> assign(:selected_ids, selected) + # Selection changed, so every derived value is stale — including choices + # the user made, which may no longer be held by any selected member. + |> assign(:overrides, %{}) + |> assign(:dropped, MapSet.new()) + |> recompute()} + end + end + + def handle_event("set-primary", %{"id" => id}, socket) do + {:noreply, socket |> assign(:primary_id, String.to_integer(id)) |> recompute()} + end + + # "Leave empty" posts the literal index "clear" (spec B7) rather than a + # candidate position — route it to the override directly instead of + # `String.to_integer/1`, which would raise on this value. + def handle_event("choose-field", %{"field" => field, "index" => "clear"}, socket) do + field = String.to_existing_atom(field) + {:noreply, assign(socket, :overrides, Map.put(socket.assigns.overrides, field, :clear))} + end + + def handle_event("choose-field", %{"field" => field, "index" => index}, socket) do + field = String.to_existing_atom(field) + index = String.to_integer(index) + + # Must resolve the clicked index the same way it was rendered — against + # `candidates_for/2` over the current selection, never against + # `@resolution.conflicts[field]`, which is built by a different, unsorted + # helper and could pick a different value than the button the user clicked. + case Enum.at(MergeResolution.candidates_for(selected_members(socket), field), index) do + nil -> + {:noreply, socket} + + candidate -> + {:noreply, + assign(socket, :overrides, Map.put(socket.assigns.overrides, field, candidate.value))} + end + end + + def handle_event("toggle-value", %{"type" => type, "id" => id}, socket) do + key = {type, cast_entry_id(id)} + + dropped = + if MapSet.member?(socket.assigns.dropped, key) do + MapSet.delete(socket.assigns.dropped, key) + else + MapSet.put(socket.assigns.dropped, key) + end + + {:noreply, assign(socket, :dropped, dropped)} + end + + defp cast_entry_id(id) do + case Integer.parse(id) do + {int, ""} -> int + _ -> id + end + end # ── Rendering helpers ────────────────────────────────────────────────── diff --git a/test/kith_web/live/contact_live/cluster_merge_test.exs b/test/kith_web/live/contact_live/cluster_merge_test.exs index bb9f2b95..99ea319b 100644 --- a/test/kith_web/live/contact_live/cluster_merge_test.exs +++ b/test/kith_web/live/contact_live/cluster_merge_test.exs @@ -1,6 +1,7 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do use KithWeb.ConnCase + import Ecto.Query import Phoenix.LiveViewTest alias Kith.{AccountsFixtures, ContactsFixtures} @@ -136,4 +137,144 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do assert {:error, {:live_redirect, %{to: "/contacts"}}} = live(ctx.conn, "/contacts/duplicates/cluster/#{other_contact.id}") end + + describe "interactions" do + test "unchecking a member recomputes the result", ctx do + {:ok, live, html0} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + # "Jiyoung" (b's middle name) appears both in b's own member chip and in + # the resolved middle_name row's value + attribution — the chip stays + # (unchecked members are dimmed, not removed) but the resolved row must + # drop it once b is no longer part of the selection. + occurrences_before = html0 |> String.split("Jiyoung") |> length() + + html = + live + |> element("input[phx-click='toggle-member'][phx-value-id='#{ctx.b.id}']") + |> render_click() + + occurrences_after = html |> String.split("Jiyoung") |> length() + + assert html =~ "Merge 1 contacts" or html =~ "1 selected" + assert occurrences_after < occurrences_before + end + + test "choosing a conflicting value marks it selected", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + html = + live + |> element("button[phx-value-field='company'][phx-value-index='1']") + |> render_click() + + assert html =~ "Figma" + end + + test "changing the selection discards an explicit choice", ctx do + c = + ContactsFixtures.contact_fixture(ctx.account_id, %{ + first_name: "Sarah", + last_name: "Kim", + company: "Stripe" + }) + + candidate!(ctx.account_id, ctx.b, c) + + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + live + |> element("button[phx-value-field='company'][phx-value-index='1']") + |> render_click() + + html = + live + |> element("input[phx-click='toggle-member'][phx-value-id='#{c.id}']") + |> render_click() + + # Back to the computed default rather than the override. + assert html =~ "Stripe" + end + + test "unchecking a value removes it from the merge", ctx do + email_type = + Kith.Repo.one!( + from(t in Kith.Contacts.ContactFieldType, where: like(t.protocol, "mailto%"), limit: 1) + ) + + field = + ContactsFixtures.contact_field_fixture(ctx.b, email_type.id, %{"value" => "x@y.com"}) + + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + html = + live + |> element("input[phx-click='toggle-value'][phx-value-id='#{field.id}']") + |> render_click() + + assert html =~ "x@y.com" + end + + test "making another member primary moves the badge", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + html = + live + |> element("button[phx-click='set-primary'][phx-value-id='#{ctx.b.id}']") + |> render_click() + + assert html =~ "primary" + end + + test "choosing Leave empty on a clearable contested field records a :clear override", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + html = + live + |> element("button[phx-value-field='company'][phx-value-index='clear']") + |> render_click() + + # The clear button itself now reads as the selected choice for the row. + assert html =~ ~s(phx-value-field="company") + + assert has_element?( + live, + "button[phx-value-field='company'][phx-value-index='clear'][class*='bg-[var(--color-accent)]']" + ) + + refute has_element?( + live, + "button[phx-value-field='company'][phx-value-index='0'][class*='bg-[var(--color-accent)]']" + ) + + refute has_element?( + live, + "button[phx-value-field='company'][phx-value-index='1'][class*='bg-[var(--color-accent)]']" + ) + end + + test "a genuinely contested required field offers candidates but no Leave empty option", + ctx do + c = + ContactsFixtures.contact_fixture(ctx.account_id, %{ + first_name: "David", + last_name: "Kim" + }) + + d = + ContactsFixtures.contact_fixture(ctx.account_id, %{ + first_name: "Priya", + last_name: "Kim" + }) + + candidate!(ctx.account_id, c, d) + + {:ok, live, html} = live(ctx.conn, cluster_path(c, d)) + + assert html =~ "David" + assert html =~ "Priya" + assert has_element?(live, "button[phx-value-field='first_name'][phx-value-index='0']") + assert has_element?(live, "button[phx-value-field='first_name'][phx-value-index='1']") + refute has_element?(live, "button[phx-value-field='first_name'][phx-value-index='clear']") + end + end end From 9fa0692b10f71df64bb1e76d05326bb2ced08d5e Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Tue, 18 Aug 2026 21:38:58 +0300 Subject: [PATCH 14/27] fix(duplicates): pin C5 discard, cover toggle-value/set-primary, guard last uncheck, fix alias id casting --- .../live/contact_live/cluster_merge.ex | 20 +++- .../live/contact_live/cluster_merge_test.exs | 103 ++++++++++++++---- 2 files changed, 99 insertions(+), 24 deletions(-) diff --git a/lib/kith_web/live/contact_live/cluster_merge.ex b/lib/kith_web/live/contact_live/cluster_merge.ex index ca8202bd..6329944e 100644 --- a/lib/kith_web/live/contact_live/cluster_merge.ex +++ b/lib/kith_web/live/contact_live/cluster_merge.ex @@ -106,11 +106,15 @@ defmodule KithWeb.ContactLive.ClusterMerge do # `MergeResolution.resolve/2` requires a non-empty member list, and there # is no meaningful "merge nothing" state to render — refuse to uncheck # the last remaining member instead of leaving a selection that would - # crash the next recompute. - {:noreply, socket} + # crash the next recompute. Re-assigning an equal `selected_ids` would + # not itself produce a diff (`assign/3` skips equal values), which would + # leave the browser's checkbox showing unchecked while the server still + # holds that member selected — flash a notice so a patch is always sent. + {:noreply, put_flash(socket, :info, "At least one contact must stay selected")} else {:noreply, socket + |> clear_flash(:info) |> assign(:selected_ids, selected) # Selection changed, so every derived value is stale — including choices # the user made, which may no longer be held by any selected member. @@ -151,7 +155,7 @@ defmodule KithWeb.ContactLive.ClusterMerge do end def handle_event("toggle-value", %{"type" => type, "id" => id}, socket) do - key = {type, cast_entry_id(id)} + key = {type, cast_entry_id(type, id)} dropped = if MapSet.member?(socket.assigns.dropped, key) do @@ -163,7 +167,15 @@ defmodule KithWeb.ContactLive.ClusterMerge do {:noreply, assign(socket, :dropped, dropped)} end - defp cast_entry_id(id) do + # `MergeSummary.build/1` gives "Fields"/"Addresses"/"Tags" entries an + # integer row id, but "Aliases" entries are keyed by the alias string + # itself (there is no per-row id to drop). Casting by the value's shape + # rather than by category would corrupt a numeric-looking alias like + # "007" — `Integer.parse/1` would turn it into `7`, which can never match + # the `{"Aliases", "007"}` key the render side looks up. + defp cast_entry_id("Aliases", id), do: id + + defp cast_entry_id(_type, id) do case Integer.parse(id) do {int, ""} -> int _ -> id diff --git a/test/kith_web/live/contact_live/cluster_merge_test.exs b/test/kith_web/live/contact_live/cluster_merge_test.exs index 99ea319b..e444832a 100644 --- a/test/kith_web/live/contact_live/cluster_merge_test.exs +++ b/test/kith_web/live/contact_live/cluster_merge_test.exs @@ -162,12 +162,23 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do test "choosing a conflicting value marks it selected", ctx do {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) - html = - live - |> element("button[phx-value-field='company'][phx-value-index='1']") - |> render_click() + # candidates_for/2, sorted, ties broken by lowest member id: index 0 is + # a's "Figma", index 1 is b's "Stripe" — so clicking index 1 must move + # the accent styling onto the Stripe button specifically, not merely + # cause "Figma" to still appear (it's a candidate label either way). + live + |> element("button[phx-value-field='company'][phx-value-index='1']") + |> render_click() - assert html =~ "Figma" + assert has_element?( + live, + "button[phx-value-field='company'][phx-value-index='1'][class*='bg-[var(--color-accent)]']" + ) + + refute has_element?( + live, + "button[phx-value-field='company'][phx-value-index='0'][class*='bg-[var(--color-accent)]']" + ) end test "changing the selection discards an explicit choice", ctx do @@ -175,24 +186,31 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do ContactsFixtures.contact_fixture(ctx.account_id, %{ first_name: "Sarah", last_name: "Kim", - company: "Stripe" + company: "Notion" }) candidate!(ctx.account_id, ctx.b, c) {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + # Three members, one value each (Figma/Stripe/Notion): candidates_for/2 + # sorts by lowest member id on a count tie, so index 2 is c's "Notion" — + # an explicit choice that is nobody's computed default, so it can only + # still be selected after the toggle below if the override survived. live - |> element("button[phx-value-field='company'][phx-value-index='1']") + |> element("button[phx-value-field='company'][phx-value-index='2']") |> render_click() - html = - live - |> element("input[phx-click='toggle-member'][phx-value-id='#{c.id}']") - |> render_click() + live + |> element("input[phx-click='toggle-member'][phx-value-id='#{c.id}']") + |> render_click() - # Back to the computed default rather than the override. - assert html =~ "Stripe" + # Back to the computed default (a's "Figma", the surviving lowest-id + # member) rather than the discarded override. + assert has_element?( + live, + "button[phx-value-field='company'][phx-value-index='0'][class*='bg-[var(--color-accent)]']" + ) end test "unchecking a value removes it from the merge", ctx do @@ -206,23 +224,68 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) - html = - live - |> element("input[phx-click='toggle-value'][phx-value-id='#{field.id}']") - |> render_click() + assert has_element?( + live, + "input[phx-click='toggle-value'][phx-value-id='#{field.id}'][checked]" + ) + + live + |> element("input[phx-click='toggle-value'][phx-value-id='#{field.id}']") + |> render_click() - assert html =~ "x@y.com" + refute has_element?( + live, + "input[phx-click='toggle-value'][phx-value-id='#{field.id}'][checked]" + ) end test "making another member primary moves the badge", ctx do {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + assert has_element?(live, "button[phx-click='set-primary'][phx-value-id='#{ctx.b.id}']") + refute has_element?(live, "button[phx-click='set-primary'][phx-value-id='#{ctx.a.id}']") + + live + |> element("button[phx-click='set-primary'][phx-value-id='#{ctx.b.id}']") + |> render_click() + + # The badge (and its "make primary" button) moved to a, off of b. + refute has_element?(live, "button[phx-click='set-primary'][phx-value-id='#{ctx.b.id}']") + assert has_element?(live, "button[phx-click='set-primary'][phx-value-id='#{ctx.a.id}']") + end + + test "unchecking the last selected member is refused, not stranded unchecked", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + live + |> element("input[phx-click='toggle-member'][phx-value-id='#{ctx.b.id}']") + |> render_click() + html = live - |> element("button[phx-click='set-primary'][phx-value-id='#{ctx.b.id}']") + |> element("input[phx-click='toggle-member'][phx-value-id='#{ctx.a.id}']") |> render_click() - assert html =~ "primary" + assert html =~ "At least one contact must stay selected" + + assert has_element?( + live, + "input[phx-click='toggle-member'][phx-value-id='#{ctx.a.id}'][checked]" + ) + end + + test "a numeric-looking alias can still be dropped", ctx do + {:ok, member} = Kith.Contacts.update_contact(ctx.a, %{"aliases" => ["007"]}) + + {:ok, live, _html} = live(ctx.conn, cluster_path(member, ctx.b)) + + assert has_element?(live, "input[phx-click='toggle-value'][phx-value-id='007'][checked]") + + live + |> element("input[phx-click='toggle-value'][phx-value-id='007']") + |> render_click() + + refute has_element?(live, "input[phx-click='toggle-value'][phx-value-id='007'][checked]") end test "choosing Leave empty on a clearable contested field records a :clear override", ctx do From 41aaa2c783a0056bc512e250435744fc09d4833f Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Tue, 18 Aug 2026 21:46:52 +0300 Subject: [PATCH 15/27] feat(duplicates): submit cluster merges and rejections --- .../live/contact_live/cluster_merge.ex | 138 +++++++++++++++- .../live/contact_live/cluster_merge_test.exs | 151 ++++++++++++++++++ 2 files changed, 286 insertions(+), 3 deletions(-) diff --git a/lib/kith_web/live/contact_live/cluster_merge.ex b/lib/kith_web/live/contact_live/cluster_merge.ex index 6329944e..d7f44fe1 100644 --- a/lib/kith_web/live/contact_live/cluster_merge.ex +++ b/lib/kith_web/live/contact_live/cluster_merge.ex @@ -10,6 +10,7 @@ defmodule KithWeb.ContactLive.ClusterMerge do use KithWeb, :live_view + alias Kith.Contacts alias Kith.Contacts.{MergeFields, MergeResolution, MergeSummary} alias Kith.DuplicateDetection alias Kith.Policy @@ -74,6 +75,9 @@ defmodule KithWeb.ContactLive.ClusterMerge do |> assign(:primary_id, primary_id) |> assign(:resolution, MergeResolution.resolve(selected, primary_id)) |> assign(:summary, MergeSummary.build(selected)) + # A merge error describes the resolution that was submitted. Anything that + # changes the resolution makes it stale, so it never outlives one. + |> assign(:error, nil) end defp selected_members(socket) do @@ -133,7 +137,11 @@ defmodule KithWeb.ContactLive.ClusterMerge do # `String.to_integer/1`, which would raise on this value. def handle_event("choose-field", %{"field" => field, "index" => "clear"}, socket) do field = String.to_existing_atom(field) - {:noreply, assign(socket, :overrides, Map.put(socket.assigns.overrides, field, :clear))} + + {:noreply, + socket + |> assign(:overrides, Map.put(socket.assigns.overrides, field, :clear)) + |> assign(:error, nil)} end def handle_event("choose-field", %{"field" => field, "index" => index}, socket) do @@ -150,7 +158,9 @@ defmodule KithWeb.ContactLive.ClusterMerge do candidate -> {:noreply, - assign(socket, :overrides, Map.put(socket.assigns.overrides, field, candidate.value))} + socket + |> assign(:overrides, Map.put(socket.assigns.overrides, field, candidate.value)) + |> assign(:error, nil)} end end @@ -164,9 +174,131 @@ defmodule KithWeb.ContactLive.ClusterMerge do MapSet.put(socket.assigns.dropped, key) end - {:noreply, assign(socket, :dropped, dropped)} + {:noreply, socket |> assign(:dropped, dropped) |> assign(:error, nil)} + end + + def handle_event("merge", _params, socket) do + survivor_id = socket.assigns.primary_id + + loser_ids = + socket |> selected_members() |> Enum.map(& &1.id) |> Enum.reject(&(&1 == survivor_id)) + + resolution = %{ + fields: Map.merge(socket.assigns.resolution.fields, socket.assigns.overrides), + drop: build_drop(socket), + # Every member the screen showed but the user excluded, so the engine can + # dismiss exactly the pairs the user reviewed and rejected. Leaving these + # out would leave those pairs `pending` and they would be suggested again. + unchecked_ids: unchecked_ids(socket) + } + + resolution = drop_aliases(resolution, socket.assigns.dropped) + + case Contacts.merge_cluster(socket.assigns.current_scope, survivor_id, loser_ids, resolution) do + {:ok, survivor} -> + {:noreply, + socket + |> put_flash(:info, "Merged #{length(loser_ids) + 1} contacts") + |> redirect(to: ~p"/contacts/#{survivor.id}")} + + {:error, reason} -> + {:noreply, assign(socket, :error, error_message(reason))} + end end + def handle_event("not-duplicates", _params, socket) do + DuplicateDetection.dismiss_selection( + socket.assigns.current_scope.account.id, + MapSet.to_list(socket.assigns.selected_ids), + unchecked_ids(socket) + ) + + {:noreply, + socket + |> put_flash(:info, "Marked as not duplicates") + |> redirect(to: ~p"/contacts/duplicates")} + end + + defp unchecked_ids(socket) do + socket.assigns.members + |> Enum.map(& &1.id) + |> Enum.reject(&MapSet.member?(socket.assigns.selected_ids, &1)) + end + + # The screen groups dropped values by its own section labels; the engine wants + # them keyed by entity type. Aliases are deliberately absent: they are a + # whole-array field carried in `fields`, with no row to drop. + defp build_drop(socket) do + socket.assigns.dropped + |> Enum.reduce(%{}, fn {label, id}, acc -> + case drop_key(label) do + nil -> + acc + + key -> + ids = equivalent_ids(socket, key, id) + Map.update(acc, key, ids, &(ids ++ &1)) + end + end) + |> Map.new(fn {key, ids} -> {key, Enum.uniq(ids)} end) + end + + # `Kith.Contacts.Merge`'s contract: its `:dedupe_owned` step collapses equal + # rows *before* `:apply_drop` runs, keeping whichever row has the lower id. + # Naming only the row the user clicked would therefore leave its equivalent + # standing on the survivor and the excluded value would come back. Each + # `MergeSummary` entry carries the dedupe-equivalence `key` for exactly this: + # expand a dropped entry to every entry sharing its key. + defp equivalent_ids(socket, section, id) do + entries = Map.fetch!(socket.assigns.summary, section) + + case Enum.find(entries, &(&1.id == id)) do + nil -> [id] + entry -> for other <- entries, other.key == entry.key, do: other.id + end + end + + defp drop_key("Fields"), do: :contact_fields + defp drop_key("Addresses"), do: :addresses + defp drop_key("Tags"), do: :tags + defp drop_key(_label), do: nil + + # An alias has no backing row — the alias id *is* the string — so unchecking + # one is a subtraction from the resolved array rather than a `drop` entry + # (design spec §3). + defp drop_aliases(resolution, dropped) do + dropped_aliases = for {"Aliases", value} <- dropped, do: value + + case resolution.fields do + %{aliases: list} when is_list(list) and dropped_aliases != [] -> + put_in(resolution.fields[:aliases], list -- dropped_aliases) + + _fields -> + resolution + end + end + + defp error_message(reason) when reason in [:not_found, :trashed], + do: "One of these contacts changed since you opened this page." + + defp error_message({:unknown_value, field}), + do: "The #{humanize(field)} you picked changed since you opened this page." + + defp error_message({:not_clearable, field}), + do: "#{String.capitalize(humanize(field))} can't be left empty." + + defp error_message({:unknown_drop, _key}), + do: "One of the values you unchecked changed since you opened this page." + + defp error_message({:invalid_fields, _changeset}), + do: "The values you picked can't be saved to one contact." + + # `:different_accounts`, `:survivor_in_losers`, `:no_losers` and anything the + # engine grows later. None is reachable from this screen's own UI, and a raw + # reason tuple on the page reads as a crash — say something true instead. + defp error_message(_reason), + do: "This merge couldn't be completed. Reload the page and try again." + # `MergeSummary.build/1` gives "Fields"/"Addresses"/"Tags" entries an # integer row id, but "Aliases" entries are keyed by the alias string # itself (there is no per-row id to drop). Casting by the value's shape diff --git a/test/kith_web/live/contact_live/cluster_merge_test.exs b/test/kith_web/live/contact_live/cluster_merge_test.exs index e444832a..fb33a004 100644 --- a/test/kith_web/live/contact_live/cluster_merge_test.exs +++ b/test/kith_web/live/contact_live/cluster_merge_test.exs @@ -340,4 +340,155 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do refute has_element?(live, "button[phx-value-field='first_name'][phx-value-index='clear']") end end + + describe "submitting" do + test "merging redirects to the survivor and trashes the rest", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + assert {:error, {:redirect, %{to: path}}} = + live |> element("button[phx-click='merge']") |> render_click() + + survivor_id = path |> String.split("/") |> List.last() |> String.to_integer() + loser_id = if survivor_id == ctx.a.id, do: ctx.b.id, else: ctx.a.id + + assert Kith.Repo.get!(Kith.Contacts.Contact, loser_id).deleted_at != nil + assert Kith.Repo.get!(Kith.Contacts.Contact, survivor_id).deleted_at == nil + end + + test "merging with a member unchecked dismisses that pair", ctx do + c = + ContactsFixtures.contact_fixture(ctx.account_id, %{first_name: "Sarah", last_name: "Kim"}) + + candidate!(ctx.account_id, ctx.b, c) + + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + live + |> element("input[phx-click='toggle-member'][phx-value-id='#{c.id}']") + |> render_click() + + assert {:error, {:redirect, _}} = + live |> element("button[phx-click='merge']") |> render_click() + + # The pair the user rejected is dismissed. Its `b` endpoint is a loser, so + # `resolve_after_merge/4` also repoints the row onto the survivor — assert + # on every surviving pair that touches `c` rather than on `(b, c)`, which + # no longer exists by that key. + statuses = + Kith.Repo.all( + from(d in Kith.Contacts.DuplicateCandidate, + where: d.contact_id == ^c.id or d.duplicate_contact_id == ^c.id, + select: d.status + ) + ) + + assert statuses == ["dismissed"] + + # An unchecked member is not a loser — it must still be an active contact. + assert Kith.Repo.get!(Kith.Contacts.Contact, c.id).deleted_at == nil + end + + # Ruling S5: the engine dedupes the survivor's own rows *before* it applies + # `drop`, so naming only the row the user clicked is not enough. Here the + # loser's row is the lower id (the one the screen renders as checkable) and + # the survivor holds the equivalent higher-id row: dropping only the clicked + # id would leave the survivor's row untouched and the excluded value would + # come back on the merged contact. + test "unchecking a value shared by two members drops every row backing it", ctx do + email_type = + Kith.Repo.one!( + from(t in Kith.Contacts.ContactFieldType, where: like(t.protocol, "mailto%"), limit: 1) + ) + + # Inserted on the loser first, so its row id is below the survivor's. + loser_field = + ContactsFixtures.contact_field_fixture(ctx.b, email_type.id, %{ + "value" => "sarah@example.com" + }) + + survivor_field = + ContactsFixtures.contact_field_fixture(ctx.a, email_type.id, %{ + "value" => "sarah@example.com" + }) + + assert loser_field.id < survivor_field.id + + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + # Only the first row of an equal-value group is checkable; the later one + # renders disabled as "duplicate · dropped". + live + |> element( + "input[phx-click='toggle-value'][phx-value-type='Fields'][phx-value-id='#{loser_field.id}']" + ) + |> render_click() + + assert {:error, {:redirect, %{to: path}}} = + live |> element("button[phx-click='merge']") |> render_click() + + assert path == "/contacts/#{ctx.a.id}" + + values = + Kith.Repo.all( + from(f in Kith.Contacts.ContactField, + where: f.contact_id == ^ctx.a.id, + select: f.value + ) + ) + + refute "sarah@example.com" in values + end + + test "not duplicates dismisses the cluster and returns to the list", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + assert {:error, {:redirect, %{to: "/contacts/duplicates"}}} = + live |> element("button[phx-click='not-duplicates']") |> render_click() + + assert Kith.DuplicateDetection.list_clusters(ctx.account_id) == [] + assert Kith.Repo.get!(Kith.Contacts.Contact, ctx.b.id).deleted_at == nil + end + + test "a member merged elsewhere produces an inline error, not a partial merge", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + # Another session trashes a member behind our back. + Kith.Repo.update_all(from(c in Kith.Contacts.Contact, where: c.id == ^ctx.b.id), + set: [deleted_at: DateTime.utc_now(:second)] + ) + + html = live |> element("button[phx-click='merge']") |> render_click() + + assert html =~ "changed since you opened" + assert Kith.Repo.get!(Kith.Contacts.Contact, ctx.a.id).deleted_at == nil + + # The screen stays usable after a failed merge. + assert has_element?(live, "button[phx-click='merge']") + end + + test "changing the resolution clears a stale merge error", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + Kith.Repo.update_all(from(c in Kith.Contacts.Contact, where: c.id == ^ctx.b.id), + set: [deleted_at: DateTime.utc_now(:second)] + ) + + assert live |> element("button[phx-click='merge']") |> render_click() =~ + "changed since you opened" + + html = + live + |> element("button[phx-value-field='company'][phx-value-index='0']") + |> render_click() + + refute html =~ "changed since you opened" + end + + test "a viewer cannot open the screen", ctx do + viewer = AccountsFixtures.user_fixture(%{role: "viewer"}) + + assert {:error, {:live_redirect, %{to: "/contacts"}}} = + live(log_in_user(build_conn(), viewer), cluster_path(ctx.a, ctx.b)) + end + end end From 6391a7c73cad76afe371015e10327bfb3c1ff225 Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Tue, 18 Aug 2026 21:54:11 +0300 Subject: [PATCH 16/27] fix(duplicates): cover alias drops, align address dedupe key with the engine --- lib/kith/contacts/merge_summary.ex | 18 ++++-- test/kith/contacts/merge_summary_test.exs | 21 +++++++ .../live/contact_live/cluster_merge_test.exs | 57 +++++++++++++++++-- 3 files changed, 85 insertions(+), 11 deletions(-) diff --git a/lib/kith/contacts/merge_summary.ex b/lib/kith/contacts/merge_summary.ex index c541b623..eced518c 100644 --- a/lib/kith/contacts/merge_summary.ex +++ b/lib/kith/contacts/merge_summary.ex @@ -90,20 +90,26 @@ defmodule Kith.Contacts.MergeSummary do # an address with neither line1 nor postal_code isn't a duplicate of # another address in the same shape (they may differ by city/region/ # country, e.g. "Paris" vs "Tokyo"), so give each a key that can't collide. + # + # The engine compares these columns through `coalesce(col, '')`, so a NULL + # and an empty string are the same value to it. Collapsing both to `""` here + # keeps this key in step: a caller expanding a dropped entry to everything + # sharing its key (see `build/1`) would otherwise miss the other side of a + # nil/"" pair the engine is about to dedupe, and the dropped value would come + # back on the survivor. defp address_key(address) do - line1 = normalize(address.line1) - postal_code = normalize(address.postal_code) + line1 = blank_to_empty(normalize(address.line1)) + postal_code = blank_to_empty(normalize(address.postal_code)) - if blank?(line1) and blank?(postal_code) do + if line1 == "" and postal_code == "" do {:no_key, address.id} else {line1, postal_code} end end - defp blank?(nil), do: true - defp blank?(""), do: true - defp blank?(_), do: false + defp blank_to_empty(nil), do: "" + defp blank_to_empty(value), do: value defp tags(ids) do from(ct in "contact_tags", diff --git a/test/kith/contacts/merge_summary_test.exs b/test/kith/contacts/merge_summary_test.exs index 1b0896cc..28976e7d 100644 --- a/test/kith/contacts/merge_summary_test.exs +++ b/test/kith/contacts/merge_summary_test.exs @@ -84,6 +84,27 @@ defmodule Kith.Contacts.MergeSummaryTest do assert Enum.count(summary.addresses, & &1.duplicate?) == 0 end + # Kith.Contacts.Merge compares these columns through `coalesce(col, '')`, so + # a NULL line1 and an empty-string line1 are the same value to `:dedupe_owned`. + # The summary key has to agree, or a caller expanding a dropped entry by key + # would miss the other side of the pair and the dropped value would survive. + # The empty string is written directly: `Address`'s changeset casts `""` to + # nil, so a blank line1 only reaches the column through a bulk insert. + test "an address with a nil line1 keys the same as one with an empty line1", ctx do + ContactsFixtures.address_fixture(ctx.a, %{"line1" => nil, "postal_code" => "94110"}) + blank = ContactsFixtures.address_fixture(ctx.b, %{"line1" => nil, "postal_code" => "94110"}) + + Repo.update_all(from(a in Kith.Contacts.Address, where: a.id == ^blank.id), + set: [line1: ""] + ) + + summary = MergeSummary.build([ctx.a, ctx.b]) + + assert [first, second] = summary.addresses + assert first.key == second.key + assert Enum.count(summary.addresses, & &1.duplicate?) == 1 + end + test "aliases are unioned across members", ctx do Repo.update_all(from(c in Kith.Contacts.Contact, where: c.id == ^ctx.a.id), set: [aliases: ["Sarah K."]] diff --git a/test/kith_web/live/contact_live/cluster_merge_test.exs b/test/kith_web/live/contact_live/cluster_merge_test.exs index fb33a004..98c3cb86 100644 --- a/test/kith_web/live/contact_live/cluster_merge_test.exs +++ b/test/kith_web/live/contact_live/cluster_merge_test.exs @@ -439,6 +439,37 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do refute "sarah@example.com" in values end + # Aliases are a whole-array field carried in `fields`, never in `drop`, so + # this is the only path that proves the subtraction reaches the database. + # "007" is deliberately numeric-looking: it also pins + # `cast_entry_id("Aliases", id)` end to end, which a render-only test can't. + test "unchecking an alias keeps it off the merged contact", ctx do + c = + ContactsFixtures.contact_fixture(ctx.account_id, %{ + first_name: "Sarah", + last_name: "Kim", + aliases: ["007", "Bond"] + }) + + candidate!(ctx.account_id, ctx.a, c) + + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + live + |> element("input[phx-click='toggle-value'][phx-value-type='Aliases'][phx-value-id='007']") + |> render_click() + + assert {:error, {:redirect, %{to: path}}} = + live |> element("button[phx-click='merge']") |> render_click() + + assert path == "/contacts/#{ctx.a.id}" + + # Asserted against the persisted array, not the payload: the subtraction + # reads `:aliases` off the already-override-merged map, so it is only + # correct because it runs after that merge. + assert Kith.Repo.get!(Kith.Contacts.Contact, ctx.a.id).aliases == ["Bond"] + end + test "not duplicates dismisses the cluster and returns to the list", ctx do {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) @@ -467,6 +498,16 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do end test "changing the resolution clears a stale merge error", ctx do + email_type = + Kith.Repo.one!( + from(t in Kith.Contacts.ContactFieldType, where: like(t.protocol, "mailto%"), limit: 1) + ) + + field = + ContactsFixtures.contact_field_fixture(ctx.a, email_type.id, %{ + "value" => "sarah@example.com" + }) + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) Kith.Repo.update_all(from(c in Kith.Contacts.Contact, where: c.id == ^ctx.b.id), @@ -476,12 +517,18 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do assert live |> element("button[phx-click='merge']") |> render_click() =~ "changed since you opened" - html = - live - |> element("button[phx-value-field='company'][phx-value-index='0']") - |> render_click() + refute live + |> element("button[phx-value-field='company'][phx-value-index='0']") + |> render_click() =~ "changed since you opened" - refute html =~ "changed since you opened" + assert live |> element("button[phx-click='merge']") |> render_click() =~ + "changed since you opened" + + refute live + |> element( + "input[phx-click='toggle-value'][phx-value-type='Fields'][phx-value-id='#{field.id}']" + ) + |> render_click() =~ "changed since you opened" end test "a viewer cannot open the screen", ctx do From bf3304458a7061437e3daf2bb17adf727de78b9d Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Tue, 18 Aug 2026 21:58:25 +0300 Subject: [PATCH 17/27] feat(duplicates): render immich links as one editable row --- .../live/contact_live/cluster_merge.ex | 111 +++++++++++++++++- .../live/contact_live/cluster_merge_test.exs | 94 +++++++++++++++ 2 files changed, 204 insertions(+), 1 deletion(-) diff --git a/lib/kith_web/live/contact_live/cluster_merge.ex b/lib/kith_web/live/contact_live/cluster_merge.ex index d7f44fe1..145185df 100644 --- a/lib/kith_web/live/contact_live/cluster_merge.ex +++ b/lib/kith_web/live/contact_live/cluster_merge.ex @@ -184,7 +184,9 @@ defmodule KithWeb.ContactLive.ClusterMerge do socket |> selected_members() |> Enum.map(& &1.id) |> Enum.reject(&(&1 == survivor_id)) resolution = %{ - fields: Map.merge(socket.assigns.resolution.fields, socket.assigns.overrides), + fields: + socket.assigns.resolution.fields + |> Map.merge(Map.delete(socket.assigns.overrides, :__immich__)), drop: build_drop(socket), # Every member the screen showed but the user excluded, so the engine can # dismiss exactly the pairs the user reviewed and rejected. Leaving these @@ -206,6 +208,44 @@ defmodule KithWeb.ContactLive.ClusterMerge do end end + # Ruling S7: `immich_status` is `null: false` with a check constraint + # restricting it to "unlinked"/"needs_review"/"linked", so unlike the other + # three (nullable) Immich fields it cannot be set to `:clear` — that maps + # to `nil` in the engine and would raise a `Postgrex.Error`. This mirrors + # `MergeResolution.resolve_immich/3`'s own handling of "no member linked". + def handle_event("choose-immich", %{"id" => "none"}, socket) do + overrides = + MergeFields.immich_fields() + |> Enum.reduce(socket.assigns.overrides, fn + :immich_status, acc -> Map.put(acc, :immich_status, "unlinked") + field, acc -> Map.put(acc, field, :clear) + end) + |> Map.put(:__immich__, :none) + + {:noreply, assign(socket, :overrides, overrides)} + end + + def handle_event("choose-immich", %{"id" => id}, socket) do + id = String.to_integer(id) + + case Enum.find(socket.assigns.members, &(&1.id == id)) do + nil -> + {:noreply, socket} + + member -> + # All four columns move together or the survivor ends up with one + # record's person id and another's sync timestamp. + overrides = + MergeFields.immich_fields() + |> Enum.reduce(socket.assigns.overrides, fn field, acc -> + Map.put(acc, field, Map.fetch!(member, field) || :clear) + end) + |> Map.put(:__immich__, id) + + {:noreply, assign(socket, :overrides, overrides)} + end + end + def handle_event("not-duplicates", _params, socket) do DuplicateDetection.dismiss_selection( socket.assigns.current_scope.account.id, @@ -365,6 +405,32 @@ defmodule KithWeb.ContactLive.ClusterMerge do defp display(list) when is_list(list), do: Enum.join(list, ", ") defp display(value), do: to_string(value) + defp linked_members(assigns) do + assigns + |> selected_from_assigns() + |> Enum.filter(&(not is_nil(&1.immich_person_id))) + end + + # Which member's Immich group is currently winning, so the row can highlight + # it without exposing the opaque person id. + defp immich_source_id(assigns) do + case Map.get(assigns.overrides, :__immich__, :default) do + :default -> + chosen = Map.get(assigns.resolution.fields, :immich_person_id) + + case Enum.find(linked_members(assigns), &(&1.immich_person_id == chosen)) do + nil -> :none + member -> member.id + end + + other -> + other + end + end + + defp immich_sync_label(nil), do: "never synced" + defp immich_sync_label(at), do: "synced #{Calendar.strftime(at, "%d %b %Y")}" + defp trash_summary(assigns) do case MapSet.size(assigns.selected_ids) - 1 do n when n <= 0 -> "Nothing moves to trash." @@ -532,6 +598,49 @@ defmodule KithWeb.ContactLive.ClusterMerge do

+ +
+
Photo library
+
+ + +
+
element("button[phx-click='choose-immich'][phx-value-id='#{ctx.b.id}']") + |> render_click() + + assert {:error, {:redirect, %{to: path}}} = + live |> element("button[phx-click='merge']") |> render_click() + + survivor_id = path |> String.split("/") |> List.last() |> String.to_integer() + survivor = Kith.Repo.get!(Kith.Contacts.Contact, survivor_id) + + assert survivor.immich_person_id == "person-b" + assert survivor.immich_status == "linked" + end + + # Controller ruling S7: `immich_status` is `null: false` with a check + # constraint, so "Not linked" must set it to the string "unlinked" (as + # `MergeResolution.resolve_immich/3` already does when no member is + # linked), not `:clear` — which the engine maps to `nil` and would raise + # a `Postgrex.Error` at the database. This must MERGE, not just render, + # to actually reach that constraint. + test "choosing not linked persists immich_status unlinked, not a crash", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + live + |> element("button[phx-click='choose-immich'][phx-value-id='none']") + |> render_click() + + assert {:error, {:redirect, %{to: path}}} = + live |> element("button[phx-click='merge']") |> render_click() + + survivor_id = path |> String.split("/") |> List.last() |> String.to_integer() + survivor = Kith.Repo.get!(Kith.Contacts.Contact, survivor_id) + + assert survivor.immich_status == "unlinked" + assert is_nil(survivor.immich_person_id) + end + + # `:__immich__` is UI bookkeeping and must never reach `merge_cluster/4` + # as a field — `Contact.update_changeset/2` uses `cast/3` with an + # explicit allowlist, which silently drops unknown keys rather than + # raising, so an unstripped key would not fail loudly; only a successful + # merge after choosing an Immich source proves it was stripped cleanly. + test "choosing an immich source still allows the merge to submit", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + live + |> element("button[phx-click='choose-immich'][phx-value-id='#{ctx.a.id}']") + |> render_click() + + assert {:error, {:redirect, %{to: _path}}} = + live |> element("button[phx-click='merge']") |> render_click() + end + end end From 56439863cc3ec45b9bda0c5d2daa5d246c1b61ea Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Tue, 18 Aug 2026 22:04:04 +0300 Subject: [PATCH 18/27] fix(duplicates): guard immich choice against deselected members, fix strip rationale --- .../live/contact_live/cluster_merge.ex | 15 ++- .../live/contact_live/cluster_merge_test.exs | 123 +++++++++++++++++- 2 files changed, 133 insertions(+), 5 deletions(-) diff --git a/lib/kith_web/live/contact_live/cluster_merge.ex b/lib/kith_web/live/contact_live/cluster_merge.ex index 145185df..d01c153f 100644 --- a/lib/kith_web/live/contact_live/cluster_merge.ex +++ b/lib/kith_web/live/contact_live/cluster_merge.ex @@ -184,6 +184,14 @@ defmodule KithWeb.ContactLive.ClusterMerge do socket |> selected_members() |> Enum.map(& &1.id) |> Enum.reject(&(&1 == survivor_id)) resolution = %{ + # `:__immich__` is UI bookkeeping, not a contact field, and MUST be + # stripped here. `Merge.validate_fields/2` reduces over every entry in + # `fields` and dispatches non-computed, non-`:clear` values to + # `held_by_member?/3`, whose first act is `Map.fetch!(member, field)` + # on a `Contact` struct — `:__immich__` is neither `:clear` nor a known + # computed field, so leaving it in raises an unhandled `KeyError` + # outside the engine's documented error contract, not a silently + # dropped key. fields: socket.assigns.resolution.fields |> Map.merge(Map.delete(socket.assigns.overrides, :__immich__)), @@ -228,7 +236,12 @@ defmodule KithWeb.ContactLive.ClusterMerge do def handle_event("choose-immich", %{"id" => id}, socket) do id = String.to_integer(id) - case Enum.find(socket.assigns.members, &(&1.id == id)) do + # Must resolve against the *selected* members, not `socket.assigns.members` + # — the row only renders buttons for selected linked members, but a + # crafted event carrying a deselected member's id would otherwise adopt + # that member's Immich group unchallenged, since slice 1 exempts the + # Immich group from `held_by_member?/3`. + case Enum.find(selected_members(socket), &(&1.id == id)) do nil -> {:noreply, socket} diff --git a/test/kith_web/live/contact_live/cluster_merge_test.exs b/test/kith_web/live/contact_live/cluster_merge_test.exs index 583a2214..90d770c0 100644 --- a/test/kith_web/live/contact_live/cluster_merge_test.exs +++ b/test/kith_web/live/contact_live/cluster_merge_test.exs @@ -617,10 +617,13 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do end # `:__immich__` is UI bookkeeping and must never reach `merge_cluster/4` - # as a field — `Contact.update_changeset/2` uses `cast/3` with an - # explicit allowlist, which silently drops unknown keys rather than - # raising, so an unstripped key would not fail loudly; only a successful - # merge after choosing an Immich source proves it was stripped cleanly. + # as a field. `Merge.validate_fields/2` reduces over every entry in + # `fields` and dispatches non-`:clear`, non-computed values to + # `held_by_member?/3`, whose first act is `Map.fetch!(member, field)` on + # a `Contact` struct — `:__immich__` holds an integer id or `:none`, so + # leaving it in would raise an unhandled `KeyError`, not silently drop. + # Only a successful merge after choosing an Immich source proves it was + # stripped cleanly. test "choosing an immich source still allows the merge to submit", ctx do {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) @@ -631,5 +634,117 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do assert {:error, {:redirect, %{to: _path}}} = live |> element("button[phx-click='merge']") |> render_click() end + + # Adopting all four columns together, checked on every field including + # `immich_person_url` (never set by the fixtures above) and + # `immich_last_synced_at` (never read back above) — asserting only + # `immich_person_id`/`immich_status` would leave half the unit-move + # guarantee unverified. + test "choosing a member adopts all four immich columns, not just two", ctx do + Kith.Repo.update_all(from(c in Kith.Contacts.Contact, where: c.id == ^ctx.b.id), + set: [immich_person_url: "https://immich.example/person/person-b"] + ) + + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + live + |> element("button[phx-click='choose-immich'][phx-value-id='#{ctx.b.id}']") + |> render_click() + + assert {:error, {:redirect, %{to: path}}} = + live |> element("button[phx-click='merge']") |> render_click() + + survivor_id = path |> String.split("/") |> List.last() |> String.to_integer() + survivor = Kith.Repo.get!(Kith.Contacts.Contact, survivor_id) + + assert survivor.immich_person_id == "person-b" + assert survivor.immich_person_url == "https://immich.example/person/person-b" + assert survivor.immich_status == "linked" + assert survivor.immich_last_synced_at == ~U[2026-02-01 00:00:00Z] + end + + # Unreachable through the rendered UI (the row only offers buttons for + # selected linked members) but not unreachable through a crafted + # `phx-click` payload — and if it were adopted, it would sail through + # `validate_fields/2` unchallenged, since slice 1 exempts the Immich + # group from `held_by_member?/3`. The handler must re-check membership + # against the selection, not `socket.assigns.members`. A three-member + # cluster is needed here: deselecting down to one member disables the + # merge button, and re-selecting a member resets `overrides` wholesale + # (see `handle_event("toggle-member", ...)`), erasing the evidence — so + # the deselected member must stay deselected while two others remain + # selected and mergeable. + test "choosing a deselected member's id does not adopt their immich group", ctx do + c = + ContactsFixtures.contact_fixture(ctx.account_id, %{ + first_name: "Sarah", + last_name: "Kim", + company: "Deselected Co" + }) + + candidate!(ctx.account_id, ctx.a, c) + + Kith.Repo.update_all(from(ct in Kith.Contacts.Contact, where: ct.id == ^c.id), + set: [ + immich_person_id: "person-c", + immich_status: "linked", + immich_last_synced_at: ~U[2026-03-01 00:00:00Z] + ] + ) + + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + live + |> element("input[phx-click='toggle-member'][phx-value-id='#{c.id}']") + |> render_click() + + render_click(live, "choose-immich", %{"id" => to_string(c.id)}) + + assert {:error, {:redirect, %{to: path}}} = + live |> element("button[phx-click='merge']") |> render_click() + + survivor_id = path |> String.split("/") |> List.last() |> String.to_integer() + survivor = Kith.Repo.get!(Kith.Contacts.Contact, survivor_id) + + assert survivor.immich_person_id != "person-c" + end + + test "the row is hidden when no member is linked", ctx do + Kith.Repo.update_all(from(c in Kith.Contacts.Contact, where: c.id == ^ctx.a.id), + set: [immich_person_id: nil, immich_status: "unlinked", immich_last_synced_at: nil] + ) + + Kith.Repo.update_all(from(c in Kith.Contacts.Contact, where: c.id == ^ctx.b.id), + set: [immich_person_id: nil, immich_status: "unlinked", immich_last_synced_at: nil] + ) + + {:ok, _live, html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + refute html =~ "Photo library" + end + + test "not linked shows selected after being clicked, and can switch back to a member", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + live + |> element("button[phx-click='choose-immich'][phx-value-id='none']") + |> render_click() + + assert live + |> element("button[phx-click='choose-immich'][phx-value-id='none']") + |> render() =~ "border-[var(--color-accent)]" + + live + |> element("button[phx-click='choose-immich'][phx-value-id='#{ctx.a.id}']") + |> render_click() + + refute live + |> element("button[phx-click='choose-immich'][phx-value-id='none']") + |> render() =~ "border-[var(--color-accent)]" + + assert live + |> element("button[phx-click='choose-immich'][phx-value-id='#{ctx.a.id}']") + |> render() =~ "border-[var(--color-accent)]" + end end end From f8e9c5532cfbed42e9f9d8a3914b6b75ba2f5bf6 Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Tue, 18 Aug 2026 22:31:07 +0300 Subject: [PATCH 19/27] fix(duplicates): make resolved rows editable, show policy flags, guard first-met-through --- lib/kith/contacts.ex | 41 +++ lib/kith/contacts/merge.ex | 4 + lib/kith/contacts/merge_summary.ex | 14 +- .../live/contact_live/cluster_merge.ex | 313 ++++++++++++++---- test/kith/contacts/merge_summary_test.exs | 31 ++ test/kith/duplicate_merge_scenarios_test.exs | 220 ++++++++++++ .../live/contact_live/cluster_merge_test.exs | 263 ++++++++++++++- 7 files changed, 813 insertions(+), 73 deletions(-) create mode 100644 test/kith/duplicate_merge_scenarios_test.exs diff --git a/lib/kith/contacts.ex b/lib/kith/contacts.ex index c58bb248..247b5404 100644 --- a/lib/kith/contacts.ex +++ b/lib/kith/contacts.ex @@ -1706,6 +1706,47 @@ defmodule Kith.Contacts do Merge.run(scope, survivor_id, loser_ids, resolution) end + @doc """ + Names for the association ids `members` hold, as `%{field => %{id => name}}`. + + Design spec §3, "Association ids": a merge screen renders the associated + record's name, never the raw id. Covers only the three association columns + in `Kith.Contacts.MergeFields.choice_fields/0`; ids with no surviving row + are simply absent, and the caller falls back to the id. + + `first_met_through_id` may point at any contact in the account, not only a + cluster member, so it is looked up rather than read off `members`. + """ + def merge_association_labels([]), do: empty_association_labels() + + def merge_association_labels([%Contact{} | _] = members) do + account_id = hd(members).account_id + + %{ + gender_id: label_map(Gender, ids_held(members, :gender_id), :name), + currency_id: label_map(Currency, ids_held(members, :currency_id), :name), + first_met_through_id: + Contact + |> where([c], c.account_id == ^account_id) + |> label_map(ids_held(members, :first_met_through_id), :display_name) + } + end + + defp empty_association_labels, + do: %{gender_id: %{}, currency_id: %{}, first_met_through_id: %{}} + + defp ids_held(members, field) do + members |> Enum.map(&Map.fetch!(&1, field)) |> Enum.reject(&is_nil/1) |> Enum.uniq() + end + + defp label_map(_queryable, [], _name_field), do: %{} + + defp label_map(queryable, ids, name_field) do + from(r in queryable, where: r.id in ^ids, select: {r.id, field(r, ^name_field)}) + |> Repo.all() + |> Map.new() + end + @doc """ Merges `non_survivor_id` into `survivor_id`. diff --git a/lib/kith/contacts/merge.ex b/lib/kith/contacts/merge.ex index 5986a3c8..e8ec74e5 100644 --- a/lib/kith/contacts/merge.ex +++ b/lib/kith/contacts/merge.ex @@ -335,6 +335,10 @@ defmodule Kith.Contacts.Merge do # case/whitespace-only differences still collapse. Tags and photos # are deduped by their own remap steps above already, via # `dedupe_and_move/6`; nothing to repeat here. + # + # `Kith.Contacts.MergeSummary.normalize/1` mirrors this key so the screen + # marks exactly the rows this step collapses. `btrim/1` strips spaces only — + # keep the two in step if either side changes. defp dedupe_owned_step(repo, survivor_id) do repo.query!( """ diff --git a/lib/kith/contacts/merge_summary.ex b/lib/kith/contacts/merge_summary.ex index eced518c..a118f0fa 100644 --- a/lib/kith/contacts/merge_summary.ex +++ b/lib/kith/contacts/merge_summary.ex @@ -167,6 +167,18 @@ defmodule Kith.Contacts.MergeSummary do marked end + # Pinned to the SQL in `Kith.Contacts.Merge`'s `dedupe_owned_step/2`, which + # keys on `lower(btrim(col))`. One-argument `btrim` strips SPACES ONLY, while + # `String.trim/1` strips every Unicode whitespace character — so a value with + # a leading tab would read as "duplicate · dropped" on screen while the engine + # kept both rows, and a caller expanding a dropped entry over everything + # sharing its key (see `build/1`) would then drop a value the user never + # excluded. `String.trim/2` with `" "` is byte-for-byte what `btrim` does. + # + # `lower()` vs `String.downcase/1` still differ for locale-specific casing + # (Postgres folds per the database collation; Elixir folds per Unicode + # default). Both are identical over ASCII, which is what these keys are in + # practice; the whitespace half is the one that had a reachable divergence. defp normalize(nil), do: nil - defp normalize(value) when is_binary(value), do: value |> String.trim() |> String.downcase() + defp normalize(value) when is_binary(value), do: value |> String.trim(" ") |> String.downcase() end diff --git a/lib/kith_web/live/contact_live/cluster_merge.ex b/lib/kith_web/live/contact_live/cluster_merge.ex index d01c153f..6f72e874 100644 --- a/lib/kith_web/live/contact_live/cluster_merge.ex +++ b/lib/kith_web/live/contact_live/cluster_merge.ex @@ -15,6 +15,33 @@ defmodule KithWeb.ContactLive.ClusterMerge do alias Kith.DuplicateDetection alias Kith.Policy + # The value sections, keyed on the identifier the drop payload and the + # summary both use. The display label is a rendering detail hanging off that + # key, never the key itself: `drop_key/1` used to match on the label, so a + # copy edit or an i18n pass would have made it return `nil` for everything + # and every unchecked value would have silently come back on the survivor. + @sections [ + {:contact_fields, "Fields"}, + {:addresses, "Addresses"}, + {:tags, "Tags"}, + {:aliases, "Aliases"} + ] + + # Rendered instead of an attribution when every `first_met_through_id` + # candidate was a member of this merge. `Merge.clear_member_self_reference/2` + # coerces such a value to `:clear`, so offering it would be offering a value + # the engine silently discards. + @self_reference_note "a contact cannot be met through a record it just absorbed" + + # Read-only rows (design spec §3: resolved by policy, not by choice) and the + # rule that produced each one. + @policy_notes %{ + favorite: "favorite if any of these contacts is", + is_archived: "archived only if every contact is archived", + deceased: "deceased if any of these contacts is", + deceased_at: "the earliest date among the deceased contacts" + } + @impl true def mount(_params, _session, socket), do: {:ok, assign(socket, :error, nil)} @@ -63,6 +90,13 @@ defmodule KithWeb.ContactLive.ClusterMerge do |> assign(:overrides, %{}) |> assign(:dropped, MapSet.new()) |> assign(:error, nil) + # `
` carrying an id but no `open` attribute loses a user-set + # `open` on every morphdom patch, and contact details is the only place + # value pruning happens — unchecking N values would cost 2N clicks. + # Tracked in assigns rather than fenced off with `phx-update="ignore"`, + # which would also freeze the checkbox state inside these sections. + |> assign(:open_sections, MapSet.new()) + |> assign(:labels, Contacts.merge_association_labels(members)) |> recompute() end @@ -149,10 +183,10 @@ defmodule KithWeb.ContactLive.ClusterMerge do index = String.to_integer(index) # Must resolve the clicked index the same way it was rendered — against - # `candidates_for/2` over the current selection, never against + # `visible_candidates/2` over the current selection, never against # `@resolution.conflicts[field]`, which is built by a different, unsorted # helper and could pick a different value than the button the user clicked. - case Enum.at(MergeResolution.candidates_for(selected_members(socket), field), index) do + case Enum.at(visible_candidates(selected_members(socket), field), index) do nil -> {:noreply, socket} @@ -164,6 +198,21 @@ defmodule KithWeb.ContactLive.ClusterMerge do end end + # `
` is server-state here (see `load_cluster/2`): the summary + # reports the toggle so the assign and the browser's own toggle agree. + def handle_event("toggle-section", %{"section" => section}, socket) do + section = String.to_existing_atom(section) + + open = + if MapSet.member?(socket.assigns.open_sections, section) do + MapSet.delete(socket.assigns.open_sections, section) + else + MapSet.put(socket.assigns.open_sections, section) + end + + {:noreply, assign(socket, :open_sections, open)} + end + def handle_event("toggle-value", %{"type" => type, "id" => id}, socket) do key = {type, cast_entry_id(type, id)} @@ -178,42 +227,7 @@ defmodule KithWeb.ContactLive.ClusterMerge do end def handle_event("merge", _params, socket) do - survivor_id = socket.assigns.primary_id - - loser_ids = - socket |> selected_members() |> Enum.map(& &1.id) |> Enum.reject(&(&1 == survivor_id)) - - resolution = %{ - # `:__immich__` is UI bookkeeping, not a contact field, and MUST be - # stripped here. `Merge.validate_fields/2` reduces over every entry in - # `fields` and dispatches non-computed, non-`:clear` values to - # `held_by_member?/3`, whose first act is `Map.fetch!(member, field)` - # on a `Contact` struct — `:__immich__` is neither `:clear` nor a known - # computed field, so leaving it in raises an unhandled `KeyError` - # outside the engine's documented error contract, not a silently - # dropped key. - fields: - socket.assigns.resolution.fields - |> Map.merge(Map.delete(socket.assigns.overrides, :__immich__)), - drop: build_drop(socket), - # Every member the screen showed but the user excluded, so the engine can - # dismiss exactly the pairs the user reviewed and rejected. Leaving these - # out would leave those pairs `pending` and they would be suggested again. - unchecked_ids: unchecked_ids(socket) - } - - resolution = drop_aliases(resolution, socket.assigns.dropped) - - case Contacts.merge_cluster(socket.assigns.current_scope, survivor_id, loser_ids, resolution) do - {:ok, survivor} -> - {:noreply, - socket - |> put_flash(:info, "Merged #{length(loser_ids) + 1} contacts") - |> redirect(to: ~p"/contacts/#{survivor.id}")} - - {:error, reason} -> - {:noreply, assign(socket, :error, error_message(reason))} - end + if authorized?(socket), do: run_merge(socket), else: {:noreply, refuse(socket)} end # Ruling S7: `immich_status` is `null: false` with a check constraint @@ -260,6 +274,14 @@ defmodule KithWeb.ContactLive.ClusterMerge do end def handle_event("not-duplicates", _params, socket) do + if authorized?(socket) do + dismiss_selection(socket) + else + {:noreply, refuse(socket)} + end + end + + defp dismiss_selection(socket) do DuplicateDetection.dismiss_selection( socket.assigns.current_scope.account.id, MapSet.to_list(socket.assigns.selected_ids), @@ -272,6 +294,57 @@ defmodule KithWeb.ContactLive.ClusterMerge do |> redirect(to: ~p"/contacts/duplicates")} end + # Spec G2 is "open **or** submit": `handle_params/3` only covers the open. + # A role downgrade while this page is open, or a reused socket, leaves a + # mounted screen whose submit would otherwise never be re-checked. + defp authorized?(socket), + do: Policy.can?(socket.assigns.current_scope.user, :update, :contact) + + defp refuse(socket) do + socket + |> put_flash(:error, "You don't have permission to merge contacts") + |> push_navigate(to: ~p"/contacts") + end + + defp run_merge(socket) do + survivor_id = socket.assigns.primary_id + + loser_ids = + socket |> selected_members() |> Enum.map(& &1.id) |> Enum.reject(&(&1 == survivor_id)) + + resolution = %{ + # `:__immich__` is UI bookkeeping, not a contact field, and MUST be + # stripped here. `Merge.validate_fields/2` reduces over every entry in + # `fields` and dispatches non-computed, non-`:clear` values to + # `held_by_member?/3`, whose first act is `Map.fetch!(member, field)` + # on a `Contact` struct — `:__immich__` is neither `:clear` nor a known + # computed field, so leaving it in raises an unhandled `KeyError` + # outside the engine's documented error contract, not a silently + # dropped key. + fields: + socket.assigns.resolution.fields + |> Map.merge(Map.delete(socket.assigns.overrides, :__immich__)), + drop: build_drop(socket), + # Every member the screen showed but the user excluded, so the engine can + # dismiss exactly the pairs the user reviewed and rejected. Leaving these + # out would leave those pairs `pending` and they would be suggested again. + unchecked_ids: unchecked_ids(socket) + } + + resolution = drop_aliases(resolution, socket.assigns.dropped) + + case Contacts.merge_cluster(socket.assigns.current_scope, survivor_id, loser_ids, resolution) do + {:ok, survivor} -> + {:noreply, + socket + |> put_flash(:info, "Merged #{length(loser_ids) + 1} contacts") + |> redirect(to: ~p"/contacts/#{survivor.id}")} + + {:error, reason} -> + {:noreply, assign(socket, :error, error_message(reason))} + end + end + defp unchecked_ids(socket) do socket.assigns.members |> Enum.map(& &1.id) @@ -311,16 +384,19 @@ defmodule KithWeb.ContactLive.ClusterMerge do end end - defp drop_key("Fields"), do: :contact_fields - defp drop_key("Addresses"), do: :addresses - defp drop_key("Tags"), do: :tags - defp drop_key(_label), do: nil + # Keyed on the section identifier the template renders as `phx-value-type`, + # never on its display label — see `@sections`. Aliases deliberately fall + # through: they are a whole-array field carried in `fields`, not a `drop`. + defp drop_key("contact_fields"), do: :contact_fields + defp drop_key("addresses"), do: :addresses + defp drop_key("tags"), do: :tags + defp drop_key(_type), do: nil # An alias has no backing row — the alias id *is* the string — so unchecking # one is a subtraction from the resolved array rather than a `drop` entry # (design spec §3). defp drop_aliases(resolution, dropped) do - dropped_aliases = for {"Aliases", value} <- dropped, do: value + dropped_aliases = for {"aliases", value} <- dropped, do: value case resolution.fields do %{aliases: list} when is_list(list) and dropped_aliases != [] -> @@ -352,13 +428,13 @@ defmodule KithWeb.ContactLive.ClusterMerge do defp error_message(_reason), do: "This merge couldn't be completed. Reload the page and try again." - # `MergeSummary.build/1` gives "Fields"/"Addresses"/"Tags" entries an - # integer row id, but "Aliases" entries are keyed by the alias string + # `MergeSummary.build/1` gives contact_fields/addresses/tags entries an + # integer row id, but alias entries are keyed by the alias string # itself (there is no per-row id to drop). Casting by the value's shape # rather than by category would corrupt a numeric-looking alias like # "007" — `Integer.parse/1` would turn it into `7`, which can never match - # the `{"Aliases", "007"}` key the render side looks up. - defp cast_entry_id("Aliases", id), do: id + # the `{"aliases", "007"}` key the render side looks up. + defp cast_entry_id("aliases", id), do: id defp cast_entry_id(_type, id) do case Integer.parse(id) do @@ -380,14 +456,48 @@ defmodule KithWeb.ContactLive.ClusterMerge do end defp candidates(assigns, field) do - MergeResolution.candidates_for(selected_from_assigns(assigns), field) + visible_candidates(selected_from_assigns(assigns), field) + end + + # Design spec §3 and the constraint slice 2 carried over from slice 1: never + # offer a member's own id as a `first_met_through_id` value. + # `Merge.clear_member_self_reference/2` coerces such a choice to `:clear`, so + # the merge would succeed and the survivor's first-met-through would come + # back empty with no error and nothing on screen having said why. + defp visible_candidates(selected, :first_met_through_id) do + member_ids = MapSet.new(selected, & &1.id) + + selected + |> MergeResolution.candidates_for(:first_met_through_id) + |> Enum.reject(&MapSet.member?(member_ids, &1.value)) end + defp visible_candidates(selected, field), + do: MergeResolution.candidates_for(selected, field) + defp selected_from_assigns(assigns) do Enum.filter(assigns.members, &MapSet.member?(assigns.selected_ids, &1.id)) end - defp attribution_text(assigns, field) do + # True when the only first-met-through values on offer were members of this + # merge, so the row renders as cleared with the reason rather than as an + # attribution for a value nobody can pick. + defp self_reference_only?(assigns) do + selected = selected_from_assigns(assigns) + + MergeResolution.candidates_for(selected, :first_met_through_id) != [] and + visible_candidates(selected, :first_met_through_id) == [] + end + + defp attribution_text(assigns, :first_met_through_id = field) do + if self_reference_only?(assigns), + do: @self_reference_note, + else: resolved_attribution(assigns, field) + end + + defp attribution_text(assigns, field), do: resolved_attribution(assigns, field) + + defp resolved_attribution(assigns, field) do total = MapSet.size(assigns.selected_ids) case Map.get(assigns.resolution.attributions, field) do @@ -398,6 +508,10 @@ defmodule KithWeb.ContactLive.ClusterMerge do end end + defp policy_note(field), do: Map.fetch!(@policy_notes, field) + + defp sections, do: @sections + defp member_name(assigns, id) do case Enum.find(assigns.members, &(&1.id == id)) do nil -> "unknown" @@ -409,6 +523,16 @@ defmodule KithWeb.ContactLive.ClusterMerge do field |> Atom.to_string() |> String.replace("_id", "") |> String.replace("_", " ") end + # Design spec §3, "Association ids": the UI renders the associated record's + # name. Without this a contested gender renders as a button reading "3". + # An id with no row behind it (deleted between load and render) falls back + # to the plain value rather than blanking the row. + defp display(assigns, field, value) + when field in [:gender_id, :currency_id, :first_met_through_id], + do: assigns.labels |> Map.fetch!(field) |> Map.get(value) || display(value) + + defp display(_assigns, _field, value), do: display(value) + defp display(nil), do: nil defp display(:clear), do: nil defp display(%Date{} = date), do: Date.to_iso8601(date) @@ -549,6 +673,7 @@ defmodule KithWeb.ContactLive.ClusterMerge do
{humanize(field)}
- -
+ +
+ + {attribution_text(assigns, field)} +
-
+
+ + Not set on any contact + + + {attribution_text(assigns, field)} + +
+
+ +
+
+ {humanize(field)} +
+
{display(effective(assigns, field))} @@ -604,10 +764,10 @@ defmodule KithWeb.ContactLive.ClusterMerge do :if={is_nil(display(effective(assigns, field)))} class="italic text-[var(--color-text-disabled)]" > - Not set on any contact + Not set - {attribution_text(assigns, field)} + {policy_note(field)}
@@ -658,9 +818,14 @@ defmodule KithWeb.ContactLive.ClusterMerge do
- + Contact details @@ -672,24 +837,24 @@ defmodule KithWeb.ContactLive.ClusterMerge do

{label}

-
- + Carried over as-is diff --git a/test/kith/contacts/merge_summary_test.exs b/test/kith/contacts/merge_summary_test.exs index 28976e7d..06c8b3e9 100644 --- a/test/kith/contacts/merge_summary_test.exs +++ b/test/kith/contacts/merge_summary_test.exs @@ -133,4 +133,35 @@ defmodule Kith.Contacts.MergeSummaryTest do assert summary.history.documents == 1 assert summary.history.calls == 0 end + + # `Kith.Contacts.Merge`'s dedupe runs `lower(btrim(value))`, and one-argument + # `btrim` strips SPACES ONLY. `String.trim/1` strips every Unicode whitespace + # character, so a tab-prefixed value used to read as "duplicate · dropped" on + # screen while the engine kept both rows — and a caller expanding a dropped + # entry over everything sharing its key would then drop a value the user + # never excluded. + test "a tab-prefixed value is not a duplicate, matching the engine's btrim", ctx do + ContactsFixtures.contact_field_fixture(ctx.a, ctx.email_type.id, %{"value" => "s@example.com"}) + + ContactsFixtures.contact_field_fixture(ctx.b, ctx.email_type.id, %{ + "value" => "\ts@example.com" + }) + + summary = MergeSummary.build([ctx.a, ctx.b]) + + assert Enum.count(summary.contact_fields, & &1.duplicate?) == 0 + assert summary.contact_fields |> Enum.map(& &1.key) |> Enum.uniq() |> length() == 2 + end + + test "a space-padded value is still a duplicate, matching the engine's btrim", ctx do + ContactsFixtures.contact_field_fixture(ctx.a, ctx.email_type.id, %{"value" => "s@example.com"}) + + ContactsFixtures.contact_field_fixture(ctx.b, ctx.email_type.id, %{ + "value" => " s@example.com " + }) + + summary = MergeSummary.build([ctx.a, ctx.b]) + + assert Enum.count(summary.contact_fields, & &1.duplicate?) == 1 + end end diff --git a/test/kith/duplicate_merge_scenarios_test.exs b/test/kith/duplicate_merge_scenarios_test.exs new file mode 100644 index 00000000..6ee61d2d --- /dev/null +++ b/test/kith/duplicate_merge_scenarios_test.exs @@ -0,0 +1,220 @@ +defmodule Kith.DuplicateMergeScenariosTest do + @moduledoc """ + The two end-to-end scenarios design spec §7 asks for as their own tests: the + handled cluster (F1–F3) and two people in one cluster (F5). + + Every other test in this area covers one step in isolation. These re-run + `DuplicateDetectionWorker` *after* a real merge, which is the only thing that + ties F1's "A–D is not recreated" to the merge that dismissed it — without + them, a change to `resolve_after_merge/4`'s repointing could break F1 with a + green suite. + """ + use Kith.DataCase, async: true + use Oban.Testing, repo: Kith.Repo + + import Kith.Factory + import Kith.ContactsFixtures + + alias Kith.Accounts.Scope + alias Kith.Contacts + alias Kith.Contacts.{Contact, DuplicateCandidate, MergeResolution} + alias Kith.DuplicateDetection + alias Kith.Workers.DuplicateDetectionWorker + + setup do + seed_reference_data!() + {account, user} = setup_account() + + email_type = + Repo.one!( + from(t in Kith.Contacts.ContactFieldType, where: t.protocol == "mailto:", limit: 1) + ) + + %{account: account, scope: Scope.for_user(user), email_type: email_type} + end + + # Shared email is what makes the worker propose the pairs, so a pair that is + # *not* recreated later is a pair the dismissal actually suppressed. + defp contact_with_email(ctx, name, email) do + contact = + insert(:contact, + account: ctx.account, + first_name: name, + last_name: "Rivera", + display_name: "#{name} Rivera" + ) + + insert(:contact_field, + contact: contact, + account: ctx.account, + contact_field_type: ctx.email_type, + value: email + ) + + contact + end + + defp run_worker(ctx), do: perform_job(DuplicateDetectionWorker, %{account_id: ctx.account.id}) + + # Exactly what `ClusterMerge` submits: a resolution over the checked members + # only, plus the ids the user unchecked. + defp merge_checked(ctx, survivor, other_checked, unchecked) do + members = [survivor | other_checked] + resolution = MergeResolution.resolve(members, survivor.id) + + Contacts.merge_cluster(ctx.scope, survivor.id, Enum.map(other_checked, & &1.id), %{ + fields: resolution.fields, + drop: %{}, + unchecked_ids: Enum.map(unchecked, & &1.id) + }) + end + + defp cluster_member_ids(ctx) do + ctx.account.id + |> DuplicateDetection.list_clusters() + |> Enum.map(fn cluster -> cluster.contacts |> Enum.map(& &1.id) |> Enum.sort() end) + end + + defp pair_status(a, b) do + {low, high} = if a.id < b.id, do: {a.id, b.id}, else: {b.id, a.id} + + Repo.one( + from(d in DuplicateCandidate, + where: d.contact_id == ^low and d.duplicate_contact_id == ^high, + select: d.status + ) + ) + end + + # Scores are the clustering input under test here; producing an exact 0.90 / + # 0.60 out of the worker's signal arithmetic is a different test's job (see + # `Kith.Workers.DuplicateDetectionWorkerTest`), so E's two edges are written + # with the scores the scenario names. + defp pending_pair(ctx, one, two, score) do + {low, high} = if one.id < two.id, do: {one, two}, else: {two, one} + + insert(:duplicate_candidate, + account: ctx.account, + contact: low, + duplicate_contact: high, + score: score, + status: "pending", + reasons: ["name_match"] + ) + end + + describe "the handled-cluster scenario (F1–F3)" do + setup ctx do + a = contact_with_email(ctx, "Ana", "shared@example.com") + b = contact_with_email(ctx, "Ana", "shared@example.com") + c = contact_with_email(ctx, "Ana", "shared@example.com") + d = contact_with_email(ctx, "Ana", "shared@example.com") + + run_worker(ctx) + + cluster = DuplicateDetection.get_cluster(ctx.account.id, a.id) + + assert Enum.map(cluster.contacts, & &1.id) |> Enum.sort() == + Enum.sort([a.id, b.id, c.id, d.id]) + + {:ok, _survivor} = merge_checked(ctx, a, [b, c], [d]) + + Map.merge(ctx, %{a: a, b: b, c: c, d: d}) + end + + test "merging A+B+C with D unchecked dismisses A–D and repoints B–D and C–D", ctx do + assert pair_status(ctx.a, ctx.d) == "dismissed" + + # B–D and C–D were repointed onto the survivor, so their rows are gone by + # their old keys and every row still touching D names A. + assert pair_status(ctx.b, ctx.d) == nil + assert pair_status(ctx.c, ctx.d) == nil + + touching_d = + Repo.all( + from(p in DuplicateCandidate, + where: p.contact_id == ^ctx.d.id or p.duplicate_contact_id == ^ctx.d.id, + select: {p.contact_id, p.duplicate_contact_id, p.status} + ) + ) + + {low, high} = if ctx.a.id < ctx.d.id, do: {ctx.a.id, ctx.d.id}, else: {ctx.d.id, ctx.a.id} + assert touching_d == [{low, high, "dismissed"}] + end + + test "re-running the worker does not recreate A–D or recluster them", ctx do + run_worker(ctx) + + assert pair_status(ctx.a, ctx.d) == "dismissed" + + refute Enum.any?(cluster_member_ids(ctx), fn ids -> + ctx.a.id in ids and ctx.d.id in ids + end) + + # D is a live contact still — it was excluded, not merged. + assert Repo.get!(Contact, ctx.d.id).deleted_at == nil + end + + test "a new contact matching A at 0.90 and D at 0.60 clusters as {A, E}", ctx do + run_worker(ctx) + e = contact_with_email(ctx, "Elena", "elena@example.com") + + pending_pair(ctx, ctx.a, e, 0.90) + pending_pair(ctx, ctx.d, e, 0.60) + + assert cluster_member_ids(ctx) == [Enum.sort([ctx.a.id, e.id])] + + # The weaker edge is not consumed, just not allowed to reunite A and D. + assert pair_status(ctx.d, e) == "pending" + end + + test "reversing the scores puts E with D instead, and A is excluded", ctx do + run_worker(ctx) + e = contact_with_email(ctx, "Elena", "elena@example.com") + + pending_pair(ctx, ctx.a, e, 0.60) + pending_pair(ctx, ctx.d, e, 0.90) + + assert cluster_member_ids(ctx) == [Enum.sort([ctx.d.id, e.id])] + assert pair_status(ctx.a, e) == "pending" + end + end + + describe "the two-people scenario (F5)" do + test "D–E survive the merge and derive as their own cluster", ctx do + a = contact_with_email(ctx, "Ana", "shared@example.com") + b = contact_with_email(ctx, "Ana", "shared@example.com") + c = contact_with_email(ctx, "Ana", "shared@example.com") + d = contact_with_email(ctx, "Ana", "shared@example.com") + e = contact_with_email(ctx, "Ana", "shared@example.com") + + run_worker(ctx) + + cluster = DuplicateDetection.get_cluster(ctx.account.id, a.id) + + assert Enum.map(cluster.contacts, & &1.id) |> Enum.sort() == + Enum.sort([a.id, b.id, c.id, d.id, e.id]) + + {:ok, _survivor} = merge_checked(ctx, a, [b, c], [d, e]) + + # Pairs between two unchecked members are left alone (spec F6's rule, + # exercised here by the merge path). + assert pair_status(d, e) == "pending" + assert pair_status(a, d) == "dismissed" + assert pair_status(a, e) == "dismissed" + + run_worker(ctx) + + assert cluster_member_ids(ctx) == [Enum.sort([d.id, e.id])] + + # And that cluster merges on its own, without ever reclustering with A. + {:ok, _} = merge_checked(ctx, d, [e], []) + + run_worker(ctx) + + assert cluster_member_ids(ctx) == [] + assert Repo.get!(Contact, e.id).deleted_at != nil + assert Repo.get!(Contact, d.id).deleted_at == nil + end + end +end diff --git a/test/kith_web/live/contact_live/cluster_merge_test.exs b/test/kith_web/live/contact_live/cluster_merge_test.exs index 90d770c0..bf59aac6 100644 --- a/test/kith_web/live/contact_live/cluster_merge_test.exs +++ b/test/kith_web/live/contact_live/cluster_merge_test.exs @@ -419,7 +419,7 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do # renders disabled as "duplicate · dropped". live |> element( - "input[phx-click='toggle-value'][phx-value-type='Fields'][phx-value-id='#{loser_field.id}']" + "input[phx-click='toggle-value'][phx-value-type='contact_fields'][phx-value-id='#{loser_field.id}']" ) |> render_click() @@ -456,7 +456,7 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) live - |> element("input[phx-click='toggle-value'][phx-value-type='Aliases'][phx-value-id='007']") + |> element("input[phx-click='toggle-value'][phx-value-type='aliases'][phx-value-id='007']") |> render_click() assert {:error, {:redirect, %{to: path}}} = @@ -526,7 +526,7 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do refute live |> element( - "input[phx-click='toggle-value'][phx-value-type='Fields'][phx-value-id='#{field.id}']" + "input[phx-click='toggle-value'][phx-value-type='contact_fields'][phx-value-id='#{field.id}']" ) |> render_click() =~ "changed since you opened" end @@ -747,4 +747,261 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do |> render() =~ "border-[var(--color-accent)]" end end + + # Spec B6: "Resolved rows show the value plus attribution and are + # click-to-change, opening in place as a segmented control including 'Leave + # empty'." Before this, the non-conflict branch rendered two inert spans + # while the section header claimed every value was clickable. + describe "resolved rows are editable (B6/B7)" do + test "a resolved row exposes its candidates as a segmented control", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + # middle_name is held by b alone, so it resolves without a conflict. + assert has_element?(live, "button[phx-value-field='middle_name'][phx-value-index='0']") + + assert live + |> element("button[phx-value-field='middle_name'][phx-value-index='0']") + |> render() =~ "Jiyoung" + end + + test "a resolved clearable row offers Leave empty and clears through to the merge", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + assert has_element?( + live, + "button[phx-value-field='middle_name'][phx-value-index='clear']" + ) + + live + |> element("button[phx-value-field='middle_name'][phx-value-index='clear']") + |> render_click() + + assert {:error, {:redirect, %{to: path}}} = + live |> element("button[phx-click='merge']") |> render_click() + + survivor_id = path |> String.split("/") |> List.last() |> String.to_integer() + assert Kith.Repo.get!(Kith.Contacts.Contact, survivor_id).middle_name == nil + end + + # B7's actual scenario: first_name is normally *resolved*, so this is the + # rule the existing contested-first_name test only covers as a carve-out. + test "the resolved first_name row offers candidates but no Leave empty", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + assert has_element?(live, "button[phx-value-field='first_name'][phx-value-index='0']") + refute has_element?(live, "button[phx-value-field='first_name'][phx-value-index='clear']") + end + end + + # Spec B1: every mergeable scalar is on screen, "including … and the flags". + describe "field coverage (B1)" do + test "every mergeable field outside the Immich group has a row", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + shown = Kith.Contacts.MergeFields.all() -- Kith.Contacts.MergeFields.immich_fields() + + for field <- shown do + assert has_element?(live, "[data-field='#{field}']"), + "no row rendered for mergeable field #{field}" + end + end + + test "the policy flags state the resolved value, not a control", ctx do + Kith.Repo.update_all(from(c in Kith.Contacts.Contact, where: c.id == ^ctx.b.id), + set: [is_archived: true, favorite: true] + ) + + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + # "the most engaged interpretation wins": favorite if any, archived only + # if every member is archived. + assert live |> element("[data-field='favorite']") |> render() =~ "Yes" + assert live |> element("[data-field='is_archived']") |> render() =~ "No" + + refute has_element?(live, "[data-field='favorite'] button") + refute has_element?(live, "[data-field='is_archived'] button") + end + end + + describe "association rows (§3)" do + setup ctx do + woman = + Kith.Repo.insert!(%Kith.Contacts.Gender{name: "Woman", account_id: ctx.account_id}) + + man = Kith.Repo.insert!(%Kith.Contacts.Gender{name: "Man", account_id: ctx.account_id}) + + Map.merge(ctx, %{woman: woman, man: man}) + end + + test "a contested gender renders the gender's name, not its id", ctx do + Kith.Repo.update_all(from(c in Kith.Contacts.Contact, where: c.id == ^ctx.a.id), + set: [gender_id: ctx.woman.id] + ) + + Kith.Repo.update_all(from(c in Kith.Contacts.Contact, where: c.id == ^ctx.b.id), + set: [gender_id: ctx.man.id] + ) + + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + rendered = + live |> element("button[phx-value-field='gender_id'][phx-value-index='0']") |> render() + + assert rendered =~ "Woman" or rendered =~ "Man" + refute rendered =~ to_string(ctx.woman.id) + refute rendered =~ to_string(ctx.man.id) + end + + # The engine's `clear_member_self_reference/2` coerces a member id to + # `:clear`, so offering one is offering a value that silently disappears. + test "a member id is never offered as a first-met-through candidate", ctx do + outsider = + ContactsFixtures.contact_fixture(ctx.account_id, %{ + first_name: "Mutual", + last_name: "Friend" + }) + + Kith.Repo.update_all(from(c in Kith.Contacts.Contact, where: c.id == ^ctx.a.id), + set: [first_met_through_id: ctx.b.id] + ) + + Kith.Repo.update_all(from(c in Kith.Contacts.Contact, where: c.id == ^ctx.b.id), + set: [first_met_through_id: outsider.id] + ) + + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + # Exactly one candidate survives the filter — the non-member — and it + # renders as that contact's name. + assert live + |> element("button[phx-value-field='first_met_through_id'][phx-value-index='0']") + |> render() =~ "Mutual Friend" + + refute has_element?( + live, + "button[phx-value-field='first_met_through_id'][phx-value-index='1']" + ) + end + + test "a row whose only candidate was a member renders cleared with the reason", ctx do + Kith.Repo.update_all(from(c in Kith.Contacts.Contact, where: c.id == ^ctx.a.id), + set: [first_met_through_id: ctx.b.id] + ) + + {:ok, live, html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + refute has_element?( + live, + "button[phx-value-field='first_met_through_id'][phx-value-index='0']" + ) + + assert html =~ "a contact cannot be met through a record it just absorbed" + end + end + + describe "section state" do + # Contact details starts folded and is the only place value pruning + # happens; a `
` with an id and no `open` attribute loses a + # user-set `open` on every LiveView patch, so pruning N values would cost + # 2N clicks. + test "an opened section stays open across an unrelated re-render", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + refute has_element?(live, "details#section-contact-details[open]") + + live + |> element("summary[phx-value-section='contact_details']") + |> render_click() + + assert has_element?(live, "details#section-contact-details[open]") + + live + |> element("button[phx-click='set-primary'][phx-value-id='#{ctx.b.id}']") + |> render_click() + + assert has_element?(live, "details#section-contact-details[open]") + end + end + + describe "drop payload keys" do + # `drop_key/1` used to match the literal display labels the template + # supplied ("Fields"/"Addresses"/"Tags"). A copy edit or an i18n pass would + # have made it return nil for everything, `build_drop/1` return `%{}`, and + # every unchecked value silently come back on the survivor. + test "value rows are keyed on section identifiers, not display copy", ctx do + email_type = + Kith.Repo.one!( + from(t in Kith.Contacts.ContactFieldType, where: like(t.protocol, "mailto%"), limit: 1) + ) + + ContactsFixtures.contact_field_fixture(ctx.a, email_type.id, %{"value" => "x@y.com"}) + {:ok, _} = Kith.Contacts.update_contact(ctx.b, %{"aliases" => ["Sass"]}) + + {:ok, _live, html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + # The rows that exist are keyed on the section identifier… + assert html =~ ~s(phx-value-type="contact_fields") + assert html =~ ~s(phx-value-type="aliases") + + # …and every section, populated or not, is identified the same way. + for {key, label} <- [ + {"contact_fields", "Fields"}, + {"addresses", "Addresses"}, + {"tags", "Tags"}, + {"aliases", "Aliases"} + ] do + assert html =~ ~s(data-field="#{key}") + refute html =~ ~s(phx-value-type="#{label}") + # The label is still displayed — it is just not the key. + assert html =~ ">#{label}

" + end + end + end + + # Spec G2 is "open **or** submit". A viewer cannot mount, so reaching the + # submit needs a role downgrade against an already-open screen (or a reused + # socket) — driven here by handing the handler that exact socket. + describe "permissions on submit (G2)" do + setup ctx do + viewer_scope = Kith.Accounts.Scope.for_user(%{ctx.user | role: "viewer"}) + members = [ctx.a, ctx.b] + + socket = %Phoenix.LiveView.Socket{ + assigns: %{ + __changed__: %{}, + flash: %{}, + current_scope: viewer_scope, + members: members, + selected_ids: MapSet.new([ctx.a.id, ctx.b.id]), + primary_id: ctx.a.id, + overrides: %{}, + dropped: MapSet.new(), + resolution: Kith.Contacts.MergeResolution.resolve(members, ctx.a.id), + error: nil + } + } + + Map.put(ctx, :socket, socket) + end + + test "a viewer submitting a merge is refused, not merged", ctx do + assert {:noreply, socket} = + KithWeb.ContactLive.ClusterMerge.handle_event("merge", %{}, ctx.socket) + + assert {:live, :redirect, %{to: "/contacts"}} = socket.redirected + assert Kith.Repo.get!(Kith.Contacts.Contact, ctx.b.id).deleted_at == nil + assert Kith.Repo.get!(Kith.Contacts.Contact, ctx.a.id).deleted_at == nil + end + + test "a viewer submitting not duplicates is refused, not dismissed", ctx do + assert {:noreply, socket} = + KithWeb.ContactLive.ClusterMerge.handle_event("not-duplicates", %{}, ctx.socket) + + assert {:live, :redirect, %{to: "/contacts"}} = socket.redirected + + assert Kith.Repo.all(from(d in Kith.Contacts.DuplicateCandidate, select: d.status)) == [ + "pending" + ] + end + end end From 9f10da03480aa9ca2fe3376a51aa2c5e31aaa7d1 Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Wed, 19 Aug 2026 10:10:11 +0300 Subject: [PATCH 20/27] fix(duplicates): treat not-null columns as non-clearable, keep identity section open --- lib/kith/contacts/merge_fields.ex | 63 +++++++++++++++++-- .../live/contact_live/cluster_merge.ex | 8 ++- test/kith/contacts/merge_fields_test.exs | 16 ++++- test/kith/contacts_merge_test.exs | 54 ++++++++++++++++ .../live/contact_live/cluster_merge_test.exs | 53 ++++++++++++++++ 5 files changed, 185 insertions(+), 9 deletions(-) diff --git a/lib/kith/contacts/merge_fields.ex b/lib/kith/contacts/merge_fields.ex index 875271a9..126ae4b1 100644 --- a/lib/kith/contacts/merge_fields.ex +++ b/lib/kith/contacts/merge_fields.ex @@ -47,13 +47,66 @@ defmodule Kith.Contacts.MergeFields do def all, do: @choice_fields ++ @policy_fields ++ @array_fields ++ @immich_fields @doc """ - Fields that cannot be set to `:clear`, derived from the contact update - changeset's `validate_required/2` list rather than maintained by hand. + Fields that cannot be set to `:clear`. + + Two sources, neither maintained by hand: + + * the contact update changeset's `validate_required/2` list, and + * every `contacts` column declared `NOT NULL`. + + The second half is not optional. `:clear` becomes `nil` at the database, so a + `NOT NULL` column raises a `Postgrex.Error` (SQLSTATE 23502) outside the + merge engine's documented `{:error, reason}` contract — and the changeset's + required list does not cover them: `birthdate_year_unknown` and + `first_met_year_unknown` are `null: false` booleans that no + `validate_required/2` names, so they were offered as clearable and two clicks + on the cluster merge screen crashed the LiveView. + + Read from `information_schema` rather than restated here so a column added + with `null: false` later is covered on its own. """ def non_clearable do - %Contact{} - |> Contact.update_changeset(%{}) - |> Map.fetch!(:required) + changeset_required = + %Contact{} + |> Contact.update_changeset(%{}) + |> Map.fetch!(:required) + + Enum.uniq(changeset_required ++ not_null_fields()) + end + + @doc """ + Mergeable fields backed by a `NOT NULL` column on `contacts`. + + Cached in `:persistent_term` — the answer is a property of the deployed + schema, and `non_clearable?/1` is called once per field per render and again + for every field inside the merge transaction. + """ + def not_null_fields do + key = {__MODULE__, :not_null_fields} + + case :persistent_term.get(key, nil) do + nil -> + fields = query_not_null_fields() + :persistent_term.put(key, fields) + fields + + fields -> + fields + end + end + + # Compared as strings and filtered against the registry, so no atom is + # created from a database identifier. + defp query_not_null_fields do + %{rows: rows} = + Kith.Repo.query!(""" + SELECT column_name FROM information_schema.columns + WHERE table_name = 'contacts' AND is_nullable = 'NO' + """) + + names = rows |> List.flatten() |> MapSet.new() + + Enum.filter(all(), &MapSet.member?(names, Atom.to_string(&1))) end @doc "Whether `field` may be cleared." diff --git a/lib/kith_web/live/contact_live/cluster_merge.ex b/lib/kith_web/live/contact_live/cluster_merge.ex index 6f72e874..8128db8f 100644 --- a/lib/kith_web/live/contact_live/cluster_merge.ex +++ b/lib/kith_web/live/contact_live/cluster_merge.ex @@ -647,10 +647,14 @@ defmodule KithWeb.ContactLive.ClusterMerge do
0} + open={conflict_count(assigns) > 0 or MapSet.member?(@open_sections, :identity)} class="rounded-[var(--radius-lg)] border border-[var(--color-border)] bg-[var(--color-surface-elevated)]" > - + Identity diff --git a/test/kith/contacts/merge_fields_test.exs b/test/kith/contacts/merge_fields_test.exs index cec61b18..9c5758cf 100644 --- a/test/kith/contacts/merge_fields_test.exs +++ b/test/kith/contacts/merge_fields_test.exs @@ -57,13 +57,25 @@ defmodule Kith.Contacts.MergeFieldsTest do end describe "non_clearable" do - test "matches the contact update changeset's required fields" do + # Two sources, and neither alone is enough: the changeset's required list + # misses `NOT NULL` columns nothing validates (which reach the database as + # `nil` and raise 23502), and the not-null set misses fields that are + # nullable but still required by the changeset. + test "covers the contact update changeset's required fields" do required = %Kith.Contacts.Contact{} |> Kith.Contacts.Contact.update_changeset(%{}) |> Map.fetch!(:required) - assert Enum.sort(MergeFields.non_clearable()) == Enum.sort(required) + assert Enum.all?(required, &(&1 in MergeFields.non_clearable())) + end + + test "covers every mergeable field backed by a not-null column" do + assert Enum.all?(MergeFields.not_null_fields(), &MergeFields.non_clearable?/1) + + assert MergeFields.non_clearable?(:birthdate_year_unknown) + assert MergeFields.non_clearable?(:first_met_year_unknown) + assert MergeFields.non_clearable?(:immich_status) end test "first_name cannot be cleared" do diff --git a/test/kith/contacts_merge_test.exs b/test/kith/contacts_merge_test.exs index 74ddbf2c..cd19a451 100644 --- a/test/kith/contacts_merge_test.exs +++ b/test/kith/contacts_merge_test.exs @@ -451,6 +451,60 @@ defmodule Kith.Contacts.MergeTest do ) end + # `:clear` becomes `nil` at the database, so a `NOT NULL` column raises + # SQLSTATE 23502 — outside this engine's `{:error, reason}` contract. + # `birthdate_year_unknown` is `null: false` but named by no + # `validate_required/2`, so only the schema itself can rule it out. The + # screen is one caller; the contract is what slice 3 and the API lean on. + test "rejects clearing a not-null column that no changeset marks required", ctx do + assert {:error, {:not_clearable, :birthdate_year_unknown}} = + Contacts.merge_cluster( + ctx.scope, + ctx.contact_a.id, + [ctx.contact_b.id], + resolution(%{birthdate_year_unknown: :clear}) + ) + + assert {:error, {:not_clearable, :first_met_year_unknown}} = + Contacts.merge_cluster( + ctx.scope, + ctx.contact_a.id, + [ctx.contact_b.id], + resolution(%{first_met_year_unknown: :clear}) + ) + + # Rejected before anything is written. + assert Kith.Repo.get!(Kith.Contacts.Contact, ctx.contact_b.id).deleted_at == nil + end + + # Guards the derivation itself: every mergeable field backed by a NOT NULL + # column must be non-clearable, so a column added with `null: false` later + # cannot quietly become clearable. + test "every not-null contacts column is reported non-clearable" do + %{rows: rows} = + Kith.Repo.query!(""" + SELECT column_name FROM information_schema.columns + WHERE table_name = 'contacts' AND is_nullable = 'NO' + """) + + names = rows |> List.flatten() |> MapSet.new() + + not_null_mergeable = + Enum.filter(Kith.Contacts.MergeFields.all(), &MapSet.member?(names, Atom.to_string(&1))) + + assert :birthdate_year_unknown in not_null_mergeable + assert :first_met_year_unknown in not_null_mergeable + + for field <- not_null_mergeable do + assert Kith.Contacts.MergeFields.non_clearable?(field), + "#{field} is NOT NULL in the database but is offered as clearable" + end + + # Nullable fields are still clearable — this is not a blanket refusal. + refute Kith.Contacts.MergeFields.non_clearable?(:occupation) + refute Kith.Contacts.MergeFields.non_clearable?(:deceased_at) + end + test "rejects a value no member holds", ctx do assert {:error, {:unknown_value, :company}} = Contacts.merge_cluster( diff --git a/test/kith_web/live/contact_live/cluster_merge_test.exs b/test/kith_web/live/contact_live/cluster_merge_test.exs index bf59aac6..493214eb 100644 --- a/test/kith_web/live/contact_live/cluster_merge_test.exs +++ b/test/kith_web/live/contact_live/cluster_merge_test.exs @@ -1004,4 +1004,57 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do ] end end + + describe "not-null columns are never offered as clearable" do + # `birthdate_year_unknown` is `null: false` but named by no + # `validate_required/2`. Every member holds `false` for it, so once resolved + # rows became segmented controls the clear button rendered on essentially + # every merge screen — and clicking it drove the engine into a 23502. + test "a not-null boolean row renders no Leave empty button", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + assert has_element?(live, "[data-field='birthdate_year_unknown']") + + refute has_element?( + live, + "button[phx-value-field='birthdate_year_unknown'][phx-value-index='clear']" + ) + + refute has_element?( + live, + "button[phx-value-field='first_met_year_unknown'][phx-value-index='clear']" + ) + end + end + + describe "identity section state" do + # Identity opens itself while a conflict is outstanding, so this only bites + # on a fully-resolved cluster — which is exactly where B6's click-to-change + # rows are the reason to open it at all. + test "identity stays open across an edit on a fully resolved cluster", ctx do + c = + ContactsFixtures.contact_fixture(ctx.account_id, %{first_name: "Priya", last_name: "Nair"}) + + d = + ContactsFixtures.contact_fixture(ctx.account_id, %{first_name: "Priya", last_name: "Nair"}) + + candidate!(ctx.account_id, c, d) + + {:ok, live, _html} = live(ctx.conn, cluster_path(c, d)) + + refute has_element?(live, "details#section-identity[open]") + + live + |> element("summary[phx-value-section='identity']") + |> render_click() + + assert has_element?(live, "details#section-identity[open]") + + live + |> element("button[phx-value-field='first_name'][phx-value-index='0']") + |> render_click() + + assert has_element?(live, "details#section-identity[open]") + end + end end From 31026a30a92513d15930b9d84815e494d3cebd0a Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Thu, 20 Aug 2026 01:07:38 +0300 Subject: [PATCH 21/27] fix(contacts): declare t/0 on the schemas Cluster's typespec references Dialyzer failed on this branch with two unknown_type errors: `Kith.Contacts.Contact.t/0` and `Kith.Contacts.DuplicateCandidate.t/0`. `Kith.DuplicateDetection.Cluster`'s `@type t` names both, but an Ecto schema does not define `t/0` on its own, so neither existed. Declared as `%__MODULE__{}`, matching `Kith.Accounts.User`. --- lib/kith/contacts/contact.ex | 2 ++ lib/kith/contacts/duplicate_candidate.ex | 2 ++ 2 files changed, 4 insertions(+) diff --git a/lib/kith/contacts/contact.ex b/lib/kith/contacts/contact.ex index 4729cd00..594468f5 100644 --- a/lib/kith/contacts/contact.ex +++ b/lib/kith/contacts/contact.ex @@ -9,6 +9,8 @@ defmodule Kith.Contacts.Contact do default_limit: 20, max_limit: 100} + @type t :: %__MODULE__{} + schema "contacts" do field :first_name, :string field :last_name, :string diff --git a/lib/kith/contacts/duplicate_candidate.ex b/lib/kith/contacts/duplicate_candidate.ex index bc26d8eb..23f83e40 100644 --- a/lib/kith/contacts/duplicate_candidate.ex +++ b/lib/kith/contacts/duplicate_candidate.ex @@ -4,6 +4,8 @@ defmodule Kith.Contacts.DuplicateCandidate do @statuses ~w(pending merged dismissed) + @type t :: %__MODULE__{} + schema "duplicate_candidates" do field :score, :float field :reasons, {:array, :string}, default: [] From 9e4bd4be646c2496c1e593f62d1506609df12677 Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Fri, 28 Aug 2026 01:54:20 +0300 Subject: [PATCH 22/27] test: align cluster merge field coverage with coupled year-unknown flags Merging the base branch brought in MergeFields.coupled_fields/0, which moved birthdate_year_unknown and first_met_year_unknown out of choice_fields/0. They are derived from whichever member supplied their date, so the cluster merge screen renders no control for them and two tests written against the old independent-flag shape failed. Replace them with assertions over the coupled design: flags get no row and are unreachable as a clear target, and the not-null guard now runs over every non-clearable choice field with a non-vacuity check. --- .../live/contact_live/cluster_merge_test.exs | 65 ++++++++++++++----- 1 file changed, 48 insertions(+), 17 deletions(-) diff --git a/test/kith_web/live/contact_live/cluster_merge_test.exs b/test/kith_web/live/contact_live/cluster_merge_test.exs index 493214eb..bc798d5b 100644 --- a/test/kith_web/live/contact_live/cluster_merge_test.exs +++ b/test/kith_web/live/contact_live/cluster_merge_test.exs @@ -793,12 +793,16 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do end end - # Spec B1: every mergeable scalar is on screen, "including … and the flags". + # Spec B1: every mergeable scalar is on screen. The Immich group moves as one + # unit and has its own control, and a coupled year-unknown flag is derived + # from whichever member supplied its date — neither is a row of its own. describe "field coverage (B1)" do - test "every mergeable field outside the Immich group has a row", ctx do + test "every mergeable field outside the Immich and coupled groups has a row", ctx do {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) - shown = Kith.Contacts.MergeFields.all() -- Kith.Contacts.MergeFields.immich_fields() + shown = + Kith.Contacts.MergeFields.all() -- + (Kith.Contacts.MergeFields.immich_fields() ++ Kith.Contacts.MergeFields.coupled_flags()) for field <- shown do assert has_element?(live, "[data-field='#{field}']"), @@ -806,6 +810,17 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do end end + test "a coupled year-unknown flag gets no row of its own", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + # Picking the flag independently of its date is what produces a contact + # claiming the placeholder year 1900 is a known birth year. + for flag <- Kith.Contacts.MergeFields.coupled_flags() do + refute has_element?(live, "[data-field='#{flag}']"), + "coupled flag #{flag} rendered as an independent control" + end + end + test "the policy flags state the resolved value, not a control", ctx do Kith.Repo.update_all(from(c in Kith.Contacts.Contact, where: c.id == ^ctx.b.id), set: [is_archived: true, favorite: true] @@ -1006,24 +1021,40 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do end describe "not-null columns are never offered as clearable" do - # `birthdate_year_unknown` is `null: false` but named by no - # `validate_required/2`. Every member holds `false` for it, so once resolved - # rows became segmented controls the clear button rendered on essentially - # every merge screen — and clicking it drove the engine into a 23502. - test "a not-null boolean row renders no Leave empty button", ctx do + # `:clear` becomes `nil` at the database, so a `NOT NULL` column raises a + # 23502 outside the engine's `{:error, reason}` contract. The year-unknown + # flags used to reach the screen as their own rows and made this trivially + # reachable; they are coupled fields now and render no control at all, so + # the guard that still has to hold is over the choice fields. + test "no non-clearable choice field renders a Leave empty button", ctx do {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) - assert has_element?(live, "[data-field='birthdate_year_unknown']") + non_clearable = + Enum.filter( + Kith.Contacts.MergeFields.choice_fields(), + &Kith.Contacts.MergeFields.non_clearable?/1 + ) - refute has_element?( - live, - "button[phx-value-field='birthdate_year_unknown'][phx-value-index='clear']" - ) + # Guards the assertion below against silently becoming vacuous. + assert non_clearable != [] - refute has_element?( - live, - "button[phx-value-field='first_met_year_unknown'][phx-value-index='clear']" - ) + for field <- non_clearable do + assert has_element?(live, "[data-field='#{field}']") + + refute has_element?( + live, + "button[phx-value-field='#{field}'][phx-value-index='clear']" + ), + "non-clearable field #{field} offered a Leave empty button" + end + end + + test "coupled year-unknown flags are never reachable as a clear target", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + for flag <- Kith.Contacts.MergeFields.coupled_flags() do + refute has_element?(live, "button[phx-value-field='#{flag}'][phx-value-index='clear']") + end end end From 985acdf58ce9e0f012dfe2c5b52f6767b48c60e6 Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Fri, 28 Aug 2026 02:00:40 +0300 Subject: [PATCH 23/27] fix: close the dismissal race and validate crafted merge-screen payloads Three review findings on the cluster merge screen, all reachable from ordinary use. upsert_dismissed/4 read the candidate row and then inserted it. The detection worker writes the same pairs, so a scan landing between the two raised on the [:account_id, :contact_id, :duplicate_contact_id] unique index, rolled the whole clique back and took the LiveView down with it. The insert now carries ON CONFLICT DO UPDATE guarded on status != "merged", which preserves the same exemption the read path applies. A guard that excludes the row updates nothing and surfaces as Ecto.StaleEntryError, which is the intended outcome and is rescued rather than propagated. dismiss_selection/3 discarded the Repo.transaction/1 result and always returned :ok, so a rolled-back clique reported "Marked as not duplicates" with nothing written. It now returns {:error, reason} and the LiveView keeps the selection and flashes an error instead of redirecting. Every phx-value payload on the screen went straight into String.to_existing_atom/1 or String.to_integer/1. Any existing atom that is not a Contact key -- :page_title, :flash, an assign name -- reached Map.fetch!/2 and killed the process with a KeyError; a non-numeric id raised ArgumentError. Payloads are now cast against the list the render side drew from and an unrecognised one is dropped, matching how find_cluster/2 already treats an unparseable URL id. --- lib/kith/duplicate_detection.ex | 66 +++-- .../live/contact_live/cluster_merge.ex | 239 ++++++++++++------ test/kith/duplicate_detection_test.exs | 24 ++ .../live/contact_live/cluster_merge_test.exs | 55 ++++ 4 files changed, 291 insertions(+), 93 deletions(-) diff --git a/lib/kith/duplicate_detection.ex b/lib/kith/duplicate_detection.ex index ca51445d..78f2d669 100644 --- a/lib/kith/duplicate_detection.ex +++ b/lib/kith/duplicate_detection.ex @@ -490,11 +490,19 @@ defmodule Kith.DuplicateDetection do this is a bulk action driven by UI selection state, not a single-resource lookup. - The whole batch of pairs is written inside one transaction. Without it, a - mid-loop failure (e.g. a unique-constraint race against a concurrent - dismissal) would leave the clique half-written — some rejected matches + The whole batch of pairs is written inside one transaction, so a mid-loop + failure cannot leave the clique half-written — some rejected matches recorded, others not — which for negative edges reopens exactly the transitivity hole the clique exists to close. + + The transaction bounds a failure but does not prevent one: `DuplicateCandidate` + rows for the same pair are also written by `Kith.Workers.DuplicateDetectionWorker`, + and a scan inserting the pair between this function's read and its write would + otherwise violate the `[:account_id, :contact_id, :duplicate_contact_id]` + unique index. Each pair is written with an upsert instead, so the racing row is + dismissed rather than colliding. + + Returns `:ok`, or `{:error, reason}` if the transaction failed. """ def dismiss_selection(account_id, selected_ids, unchecked_ids) do now = DateTime.utc_now() |> DateTime.truncate(:second) @@ -515,8 +523,10 @@ defmodule Kith.DuplicateDetection do |> Enum.uniq() |> Enum.each(fn {low, high} -> upsert_dismissed(account_id, low, high, now) end) end) - - :ok + |> case do + {:ok, _} -> :ok + {:error, reason} -> {:error, reason} + end end defp owned_ids(_account_id, []), do: [] @@ -538,17 +548,7 @@ defmodule Kith.DuplicateDetection do ) ) do nil -> - %DuplicateCandidate{account_id: account_id} - |> DuplicateCandidate.changeset(%{ - contact_id: low, - duplicate_contact_id: high, - score: 0.0, - reasons: ["user_rejected"], - status: "dismissed", - detected_at: now, - resolved_at: now - }) - |> Repo.insert!() + insert_dismissed(account_id, low, high, now) %DuplicateCandidate{status: "merged"} = existing -> existing @@ -557,4 +557,38 @@ defmodule Kith.DuplicateDetection do existing |> DuplicateCandidate.dismiss_changeset() |> Repo.update!() end end + + # A detection scan can insert this pair between the read above and this + # write. `ON CONFLICT DO UPDATE` dismisses that row rather than colliding on + # the unique index and rolling the whole clique back. The `status != "merged"` + # guard preserves the same exemption the read path applies: a pair already + # resolved by an actual merge is not reopened as a dismissal. + # + # When that guard excludes the row, Postgres updates nothing and returns no + # row, which Ecto surfaces as `Ecto.StaleEntryError`. That is the intended + # outcome — the merged row stands — so it is not a failure of the clique. + defp insert_dismissed(account_id, low, high, now) do + on_conflict = + from(d in DuplicateCandidate, + where: d.status != "merged", + update: [set: [status: "dismissed", resolved_at: ^now, updated_at: ^now]] + ) + + %DuplicateCandidate{account_id: account_id} + |> DuplicateCandidate.changeset(%{ + contact_id: low, + duplicate_contact_id: high, + score: 0.0, + reasons: ["user_rejected"], + status: "dismissed", + detected_at: now, + resolved_at: now + }) + |> Repo.insert!( + on_conflict: on_conflict, + conflict_target: [:account_id, :contact_id, :duplicate_contact_id] + ) + rescue + Ecto.StaleEntryError -> :ok + end end diff --git a/lib/kith_web/live/contact_live/cluster_merge.ex b/lib/kith_web/live/contact_live/cluster_merge.ex index 8128db8f..28fff0be 100644 --- a/lib/kith_web/live/contact_live/cluster_merge.ex +++ b/lib/kith_web/live/contact_live/cluster_merge.ex @@ -27,6 +27,12 @@ defmodule KithWeb.ContactLive.ClusterMerge do {:aliases, "Aliases"} ] + # The collapsible `
` blocks, whose open/closed state lives in + # `@open_sections`. Distinct from `@sections` above, which names the + # categories of droppable owned records. Listed so `toggle-section` has + # something to validate its payload against. + @toggle_sections ~w(identity contact_details history)a + # Rendered instead of an attribution when every `first_met_through_id` # candidate was a member of this merge. `Merge.clear_member_self_reference/2` # coerces such a value to `:clear`, so offering it would be offering a value @@ -131,86 +137,52 @@ defmodule KithWeb.ContactLive.ClusterMerge do @impl true def handle_event("toggle-member", %{"id" => id}, socket) do - id = String.to_integer(id) - - selected = - if MapSet.member?(socket.assigns.selected_ids, id) do - MapSet.delete(socket.assigns.selected_ids, id) - else - MapSet.put(socket.assigns.selected_ids, id) - end - - if MapSet.size(selected) == 0 do - # `MergeResolution.resolve/2` requires a non-empty member list, and there - # is no meaningful "merge nothing" state to render — refuse to uncheck - # the last remaining member instead of leaving a selection that would - # crash the next recompute. Re-assigning an equal `selected_ids` would - # not itself produce a diff (`assign/3` skips equal values), which would - # leave the browser's checkbox showing unchecked while the server still - # holds that member selected — flash a notice so a patch is always sent. - {:noreply, put_flash(socket, :info, "At least one contact must stay selected")} + with {:ok, id} <- cast_member_id(id) do + toggle_member(socket, id) else - {:noreply, - socket - |> clear_flash(:info) - |> assign(:selected_ids, selected) - # Selection changed, so every derived value is stale — including choices - # the user made, which may no longer be held by any selected member. - |> assign(:overrides, %{}) - |> assign(:dropped, MapSet.new()) - |> recompute()} + :error -> {:noreply, socket} end end def handle_event("set-primary", %{"id" => id}, socket) do - {:noreply, socket |> assign(:primary_id, String.to_integer(id)) |> recompute()} + case cast_member_id(id) do + {:ok, id} -> {:noreply, socket |> assign(:primary_id, id) |> recompute()} + :error -> {:noreply, socket} + end end # "Leave empty" posts the literal index "clear" (spec B7) rather than a # candidate position — route it to the override directly instead of # `String.to_integer/1`, which would raise on this value. def handle_event("choose-field", %{"field" => field, "index" => "clear"}, socket) do - field = String.to_existing_atom(field) + case cast_choice_field(field) do + {:ok, field} -> + {:noreply, + socket + |> assign(:overrides, Map.put(socket.assigns.overrides, field, :clear)) + |> assign(:error, nil)} - {:noreply, - socket - |> assign(:overrides, Map.put(socket.assigns.overrides, field, :clear)) - |> assign(:error, nil)} + :error -> + {:noreply, socket} + end end def handle_event("choose-field", %{"field" => field, "index" => index}, socket) do - field = String.to_existing_atom(field) - index = String.to_integer(index) - - # Must resolve the clicked index the same way it was rendered — against - # `visible_candidates/2` over the current selection, never against - # `@resolution.conflicts[field]`, which is built by a different, unsorted - # helper and could pick a different value than the button the user clicked. - case Enum.at(visible_candidates(selected_members(socket), field), index) do - nil -> - {:noreply, socket} - - candidate -> - {:noreply, - socket - |> assign(:overrides, Map.put(socket.assigns.overrides, field, candidate.value)) - |> assign(:error, nil)} + with {:ok, field} <- cast_choice_field(field), + {:ok, index} <- cast_member_id(index) do + choose_field(socket, field, index) + else + :error -> {:noreply, socket} end end # `
` is server-state here (see `load_cluster/2`): the summary # reports the toggle so the assign and the browser's own toggle agree. def handle_event("toggle-section", %{"section" => section}, socket) do - section = String.to_existing_atom(section) - - open = - if MapSet.member?(socket.assigns.open_sections, section) do - MapSet.delete(socket.assigns.open_sections, section) - else - MapSet.put(socket.assigns.open_sections, section) - end - - {:noreply, assign(socket, :open_sections, open)} + case cast_section(section) do + {:ok, section} -> {:noreply, assign(socket, :open_sections, toggled(socket, section))} + :error -> {:noreply, socket} + end end def handle_event("toggle-value", %{"type" => type, "id" => id}, socket) do @@ -248,8 +220,79 @@ defmodule KithWeb.ContactLive.ClusterMerge do end def handle_event("choose-immich", %{"id" => id}, socket) do - id = String.to_integer(id) + case cast_member_id(id) do + {:ok, id} -> choose_immich(socket, id) + :error -> {:noreply, socket} + end + end + + def handle_event("not-duplicates", _params, socket) do + if authorized?(socket) do + dismiss_selection(socket) + else + {:noreply, refuse(socket)} + end + end + + defp toggle_member(socket, id) do + selected = + if MapSet.member?(socket.assigns.selected_ids, id) do + MapSet.delete(socket.assigns.selected_ids, id) + else + MapSet.put(socket.assigns.selected_ids, id) + end + if MapSet.size(selected) == 0 do + # `MergeResolution.resolve/2` requires a non-empty member list, and there + # is no meaningful "merge nothing" state to render — refuse to uncheck + # the last remaining member instead of leaving a selection that would + # crash the next recompute. Re-assigning an equal `selected_ids` would + # not itself produce a diff (`assign/3` skips equal values), which would + # leave the browser's checkbox showing unchecked while the server still + # holds that member selected — flash a notice so a patch is always sent. + {:noreply, put_flash(socket, :info, "At least one contact must stay selected")} + else + {:noreply, + socket + |> clear_flash(:info) + |> assign(:selected_ids, selected) + # Selection changed, so every derived value is stale — including choices + # the user made, which may no longer be held by any selected member. + |> assign(:overrides, %{}) + |> assign(:dropped, MapSet.new()) + |> recompute()} + end + end + + defp choose_field(socket, field, index) do + # Must resolve the clicked index the same way it was rendered — against + # `visible_candidates/2` over the current selection, never against + # `@resolution.conflicts[field]`, which is built by a different, unsorted + # helper and could pick a different value than the button the user clicked. + case Enum.at(visible_candidates(selected_members(socket), field), index) do + nil -> + {:noreply, socket} + + candidate -> + {:noreply, + socket + |> assign(:overrides, Map.put(socket.assigns.overrides, field, candidate.value)) + |> assign(:error, nil)} + end + end + + defp toggled(socket, section) do + open = + if MapSet.member?(socket.assigns.open_sections, section) do + MapSet.delete(socket.assigns.open_sections, section) + else + MapSet.put(socket.assigns.open_sections, section) + end + + open + end + + defp choose_immich(socket, id) do # Must resolve against the *selected* members, not `socket.assigns.members` # — the row only renders buttons for selected linked members, but a # crafted event carrying a deselected member's id would otherwise adopt @@ -273,25 +316,24 @@ defmodule KithWeb.ContactLive.ClusterMerge do end end - def handle_event("not-duplicates", _params, socket) do - if authorized?(socket) do - dismiss_selection(socket) - else - {:noreply, refuse(socket)} - end - end - defp dismiss_selection(socket) do - DuplicateDetection.dismiss_selection( - socket.assigns.current_scope.account.id, - MapSet.to_list(socket.assigns.selected_ids), - unchecked_ids(socket) - ) + case DuplicateDetection.dismiss_selection( + socket.assigns.current_scope.account.id, + MapSet.to_list(socket.assigns.selected_ids), + unchecked_ids(socket) + ) do + :ok -> + {:noreply, + socket + |> put_flash(:info, "Marked as not duplicates") + |> redirect(to: ~p"/contacts/duplicates")} - {:noreply, - socket - |> put_flash(:info, "Marked as not duplicates") - |> redirect(to: ~p"/contacts/duplicates")} + # A half-written clique is rolled back, so the screen must not claim the + # dismissal stuck. Staying put keeps the selection intact for a retry. + {:error, _reason} -> + {:noreply, + put_flash(socket, :error, "Could not mark these as not duplicates. Please try again.")} + end end # Spec G2 is "open **or** submit": `handle_params/3` only covers the open. @@ -443,6 +485,49 @@ defmodule KithWeb.ContactLive.ClusterMerge do end end + # ── Casting client-supplied event payloads ───────────────────────────── + + # `phx-value-*` is whatever the client sends, not whatever the template + # rendered. Casting it with `String.to_existing_atom/1` alone is not enough: + # it narrows the input to atoms this node has already created, and plenty of + # those (`:page_title`, `:flash`, every other assign name) are not `Contact` + # struct keys, so they reach `Map.fetch!/2` downstream and take the LiveView + # process down with a `KeyError`. Each cast below therefore ends in a + # membership check against the list the render side actually drew from, and + # an unrecognised payload is dropped rather than raised on — matching how + # `find_cluster/2` already treats an unparseable id in the URL. + + defp cast_choice_field(field) when is_binary(field) do + cast_known_atom(field, MergeFields.choice_fields()) + end + + defp cast_choice_field(_field), do: :error + + defp cast_section(section) when is_binary(section) do + cast_known_atom(section, @toggle_sections) + end + + defp cast_section(_section), do: :error + + defp cast_known_atom(string, allowed) do + atom = String.to_existing_atom(string) + if atom in allowed, do: {:ok, atom}, else: :error + rescue + # No atom by this name exists, so it cannot be in `allowed` either. + ArgumentError -> :error + end + + # Also used for the candidate index in `choose-field`, which is a position in + # a rendered list and so is bounded by the same non-negative-integer shape. + defp cast_member_id(id) when is_binary(id) do + case Integer.parse(id) do + {int, ""} when int >= 0 -> {:ok, int} + _ -> :error + end + end + + defp cast_member_id(_id), do: :error + # ── Rendering helpers ────────────────────────────────────────────────── defp effective(assigns, field) do diff --git a/test/kith/duplicate_detection_test.exs b/test/kith/duplicate_detection_test.exs index 2077b0c4..93172741 100644 --- a/test/kith/duplicate_detection_test.exs +++ b/test/kith/duplicate_detection_test.exs @@ -754,6 +754,30 @@ defmodule Kith.DuplicateDetectionTest do assert status_of(ctx.account_id, a, b) == "merged" end + test "is idempotent over a pair it has already dismissed", ctx do + %{a: a, b: b} = ctx.contacts + + :ok = Kith.DuplicateDetection.dismiss_selection(ctx.account_id, [a.id, b.id], []) + :ok = Kith.DuplicateDetection.dismiss_selection(ctx.account_id, [a.id, b.id], []) + + assert status_of(ctx.account_id, a, b) == "dismissed" + + assert Repo.aggregate( + from(d in Kith.Contacts.DuplicateCandidate, + where: d.account_id == ^ctx.account_id + ), + :count + ) == 1 + end + + # Blind spot: these tests all run on one sandboxed connection, so none of + # them reaches the `ON CONFLICT` branch in `insert_dismissed/4`. That branch + # fires only when a detection scan inserts the pair between this function's + # read and its write, which needs two real connections to provoke. What is + # covered here is that both outcomes the branch has to preserve — a fresh + # pair becomes "dismissed", a merged pair stays "merged" — hold on the read + # path, and that a repeat call does not duplicate a row. + test "rejects a selected id belonging to another account and writes nothing for it", ctx do %{a: a, b: b} = ctx.contacts other_user = Kith.AccountsFixtures.user_fixture() diff --git a/test/kith_web/live/contact_live/cluster_merge_test.exs b/test/kith_web/live/contact_live/cluster_merge_test.exs index bc798d5b..d00ce5a1 100644 --- a/test/kith_web/live/contact_live/cluster_merge_test.exs +++ b/test/kith_web/live/contact_live/cluster_merge_test.exs @@ -1088,4 +1088,59 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do assert has_element?(live, "details#section-identity[open]") end end + + describe "crafted event payloads" do + # `phx-value-*` is client-controlled. Before these guards, `choose-field` + # ran `String.to_existing_atom/1` on it and handed the result to + # `Map.fetch!/2`: any atom the node had already created but that is not a + # Contact key — `:page_title`, `:flash`, an assign name — killed the + # LiveView process. The id handlers had the same shape via + # `String.to_integer/1`, which raises on anything non-numeric. + test "an unknown field name is ignored rather than crashing the view", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + for field <- ["page_title", "flash", "definitely_not_an_atom_anywhere", ""] do + render_click(live, "choose-field", %{"field" => field, "index" => "0"}) + render_click(live, "choose-field", %{"field" => field, "index" => "clear"}) + end + + assert has_element?(live, "[data-field='first_name']") + end + + test "a policy or coupled field cannot be driven through choose-field", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + # These resolve by policy or by coupling, never by choice — accepting an + # override for them would let the client bypass that resolution. + for field <- + Kith.Contacts.MergeFields.policy_fields() ++ + Kith.Contacts.MergeFields.coupled_flags() do + render_click(live, "choose-field", %{"field" => to_string(field), "index" => "clear"}) + end + + assert has_element?(live, "[data-field='first_name']") + end + + test "a non-numeric id is ignored by every id-carrying handler", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + for id <- ["abc", "1x", "", "-1", "9999999999999999999999"] do + render_click(live, "toggle-member", %{"id" => id}) + render_click(live, "set-primary", %{"id" => id}) + render_click(live, "choose-immich", %{"id" => id}) + end + + assert has_element?(live, "[data-field='first_name']") + end + + test "an unknown section name is ignored by toggle-section", ctx do + {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) + + for section <- ["page_title", "nope_not_a_section", "contact_fields", ""] do + render_click(live, "toggle-section", %{"section" => section}) + end + + assert has_element?(live, "[data-field='first_name']") + end + end end From f4aa6f81ef51547ce4c0ad47d771acf573f1c2a5 Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Fri, 28 Aug 2026 02:09:24 +0300 Subject: [PATCH 24/27] perf: bound the cluster derivation and cache the duplicates badge cluster_count/1 backs the sidebar badge, which UserAuth resolves in an on_mount hook -- so it ran for every authenticated LiveView, twice per navigation, on screens with nothing to do with duplicates. Each run loaded every pending *and* dismissed candidate row for the account and re-ran the union-find pass. dismiss_selection/3 writes a full clique per dismissal and those rows are never deleted, so that load grew for the life of the account. Two changes. derive_groups/1 now loads only the dismissed pairs with both endpoints in the pending set: ensure_group/2 is called with nothing but pending endpoints, so every group member is one, and blocked?/4 only looks up ids drawn from two groups -- a negative edge touching a contact no pending pair mentions can never be consulted. That bounds the derivation by the work actually pending rather than by the account's whole dismissal history, and leaves every derived cluster identical. cluster_count/1 is then cached per account in :kith_cache, with every write path that can change the answer dropping the entry and the TTL left as a backstop. MergeFields caches both halves of non_clearable/0, not just the query half: the changeset half built a %Contact{} and ran update_changeset/2 on every call, and non_clearable?/1 runs once per choice field per render -- 17 changesets a render -- and again for every field inside the merge transaction. query_not_null_fields/0 scopes information_schema.columns to current_schema(). It spans every schema in the database, so any other contacts table contributed its NOT NULL columns to the union, and a column nullable here but NOT NULL there would have been reported non-clearable and hidden the "Leave empty" button. MergeSummary.history/1 counts relationships through related_contact_id as well as contact_id. remap_relationships/4 rewrites both, so a member that is only the target of relationships contributed 0 and the "records move to the primary" line understated the merge. --- lib/kith/contacts/merge_fields.ex | 55 ++++++--- lib/kith/contacts/merge_summary.ex | 18 ++- lib/kith/duplicate_detection.ex | 106 ++++++++++++++---- .../live/contact_live/cluster_merge.ex | 5 +- test/kith/contacts/merge_summary_test.exs | 29 +++++ test/kith/duplicate_detection_test.exs | 29 +++++ 6 files changed, 199 insertions(+), 43 deletions(-) diff --git a/lib/kith/contacts/merge_fields.ex b/lib/kith/contacts/merge_fields.ex index c80ca00f..691179a5 100644 --- a/lib/kith/contacts/merge_fields.ex +++ b/lib/kith/contacts/merge_fields.ex @@ -88,42 +88,63 @@ defmodule Kith.Contacts.MergeFields do with `null: false` later is covered on its own. """ def non_clearable do - changeset_required = - %Contact{} - |> Contact.update_changeset(%{}) - |> Map.fetch!(:required) - - Enum.uniq(changeset_required ++ not_null_fields()) + cached(:non_clearable, fn -> + changeset_required = + %Contact{} + |> Contact.update_changeset(%{}) + |> Map.fetch!(:required) + + Enum.uniq(changeset_required ++ not_null_fields()) + end) end @doc """ Mergeable fields backed by a `NOT NULL` column on `contacts`. - - Cached in `:persistent_term` — the answer is a property of the deployed - schema, and `non_clearable?/1` is called once per field per render and again - for every field inside the merge transaction. """ def not_null_fields do - key = {__MODULE__, :not_null_fields} + cached(:not_null_fields, &query_not_null_fields/0) + end + + # Both halves of `non_clearable/0` are cached, not just the query: the + # changeset half builds a `%Contact{}` and runs `update_changeset/2` over it, + # and `non_clearable?/1` is called once per choice field per render — 17 + # changesets a render — and again for every field inside the merge + # transaction. + # + # `:persistent_term` because both answers are properties of the deployed + # schema and the compiled changeset, so they cannot change while the node is + # running. A migration that relaxes a nullability constraint is picked up on + # the next restart, which is when the migration's own deploy happens anyway; + # there is no purge path because nothing would call it. + defp cached(name, fun) do + key = {__MODULE__, name} case :persistent_term.get(key, nil) do nil -> - fields = query_not_null_fields() - :persistent_term.put(key, fields) - fields + value = fun.() + :persistent_term.put(key, value) + value - fields -> - fields + value -> + value end end # Compared as strings and filtered against the registry, so no atom is # created from a database identifier. + # + # Scoped to `current_schema()`: `information_schema.columns` spans every + # schema in the database, so without it any other `contacts` table — a + # leftover, a second tenant schema — contributes its `NOT NULL` columns to + # the union, and a column nullable here but `NOT NULL` there would be + # reported non-clearable and hide the "Leave empty" button. defp query_not_null_fields do %{rows: rows} = Kith.Repo.query!(""" SELECT column_name FROM information_schema.columns - WHERE table_name = 'contacts' AND is_nullable = 'NO' + WHERE table_name = 'contacts' + AND table_schema = current_schema() + AND is_nullable = 'NO' """) names = rows |> List.flatten() |> MapSet.new() diff --git a/lib/kith/contacts/merge_summary.ex b/lib/kith/contacts/merge_summary.ex index a118f0fa..70ff00fe 100644 --- a/lib/kith/contacts/merge_summary.ex +++ b/lib/kith/contacts/merge_summary.ex @@ -144,8 +144,22 @@ defmodule Kith.Contacts.MergeSummary do defp history(ids) do counts = - Map.new(@history, fn {key, schema} -> - {key, Repo.aggregate(from(r in schema, where: r.contact_id in ^ids), :count)} + Map.new(@history, fn + # Relationships are the one history table a member can be on either end + # of, and `Merge`'s `remap_relationships/4` rewrites both `contact_id` + # and `related_contact_id`. Counting only the first endpoint reports 0 + # for a member that is purely the *target* of relationships, so the + # screen understated what the merge moves. A row with both endpoints in + # the cluster is still one row, so `or` counts it once. + {:relationships, schema} -> + {:relationships, + Repo.aggregate( + from(r in schema, where: r.contact_id in ^ids or r.related_contact_id in ^ids), + :count + )} + + {key, schema} -> + {key, Repo.aggregate(from(r in schema, where: r.contact_id in ^ids), :count)} end) activities = diff --git a/lib/kith/duplicate_detection.ex b/lib/kith/duplicate_detection.ex index 78f2d669..918fab16 100644 --- a/lib/kith/duplicate_detection.ex +++ b/lib/kith/duplicate_detection.ex @@ -31,13 +31,17 @@ defmodule Kith.DuplicateDetection do end def dismiss_candidate(%DuplicateCandidate{} = candidate) do + invalidate_cluster_count(candidate.account_id) candidate |> DuplicateCandidate.dismiss_changeset() |> Repo.update() end def mark_merged(%DuplicateCandidate{} = candidate) do + invalidate_cluster_count(candidate.account_id) candidate |> DuplicateCandidate.merge_changeset() |> Repo.update() end + @cluster_count_ttl :timer.minutes(5) + def pending_count(account_id) do DuplicateCandidate |> scope_to_account(account_id) @@ -161,6 +165,8 @@ defmodule Kith.DuplicateDetection do end defp merge_or_insert(account_id, low, high, candidate) do + invalidate_cluster_count(account_id) + existing = Repo.one( from(d in DuplicateCandidate, @@ -240,35 +246,49 @@ defmodule Kith.DuplicateDetection do Counts groups directly from the union-find derivation rather than building full `%Cluster{}` structs, so it never queries contacts. + + Cached per account. This is the sidebar badge, which `KithWeb.UserAuth` + resolves in an `on_mount` hook — so it runs for every authenticated + LiveView, twice per navigation (disconnected then connected mount), on a + screen that may have nothing to do with duplicates. Every write path that + can change the answer drops the entry, so the TTL is only a backstop against + a change made outside this module. """ def cluster_count(account_id) do + case Cachex.fetch(:kith_cache, cluster_count_key(account_id), fn _key -> + {:commit, compute_cluster_count(account_id), ttl: @cluster_count_ttl} + end) do + {:ok, count} -> count + {:commit, count} -> count + # A cache that is down or erroring must not take the badge — and with it + # every authenticated page — down with it. + _ -> compute_cluster_count(account_id) + end + end + + defp compute_cluster_count(account_id) do {acc, _pending} = derive_groups(account_id) Enum.count(acc.groups, fn {_id, members} -> MapSet.size(members) >= 2 end) end + defp cluster_count_key(account_id), do: {:duplicate_cluster_count, account_id} + + # Called from every path that inserts, updates or resolves a candidate row. + # Placed on the low-level writes rather than the public entry points so a new + # caller cannot route around it. + defp invalidate_cluster_count(account_id) do + Cachex.del(:kith_cache, cluster_count_key(account_id)) + :ok + end + defp build_clusters(account_id) do {acc, pending} = derive_groups(account_id) to_clusters(acc, account_id, pending) end defp derive_groups(account_id) do - candidates = - DuplicateCandidate - |> scope_to_account(account_id) - |> where([d], d.status in ["pending", "dismissed"]) - |> join(:inner, [d], one in Contact, - on: one.id == d.contact_id and one.account_id == ^account_id - ) - |> join(:inner, [d, one], two in Contact, - on: two.id == d.duplicate_contact_id and two.account_id == ^account_id - ) - |> where([d, one, two], is_nil(one.deleted_at) and is_nil(two.deleted_at)) - |> select([d], d) - |> Repo.all() - - {pending, dismissed} = Enum.split_with(candidates, &(&1.status == "pending")) - - negatives = index_negatives(dismissed) + pending = pending_candidates(account_id) + negatives = pending |> blocking_dismissed(account_id) |> index_negatives() acc = pending @@ -282,12 +302,55 @@ defmodule Kith.DuplicateDetection do {acc, pending} end + defp pending_candidates(account_id) do + DuplicateCandidate + |> scope_to_account(account_id) + |> where([d], d.status == "pending") + |> join(:inner, [d], one in Contact, + on: one.id == d.contact_id and one.account_id == ^account_id + ) + |> join(:inner, [d, one], two in Contact, + on: two.id == d.duplicate_contact_id and two.account_id == ^account_id + ) + |> where([d, one, two], is_nil(one.deleted_at) and is_nil(two.deleted_at)) + |> select([d], d) + |> Repo.all() + end + + # Only dismissed pairs with *both* endpoints in the pending set can change + # the outcome: `ensure_group/2` is called with nothing but pending endpoints, + # so every group member is one, and `blocked?/4` only ever looks up ids drawn + # from two groups. A negative edge touching a contact no pending pair + # mentions can never be consulted. + # + # This is what keeps the derivation bounded. Dismissed rows are permanent and + # `dismiss_selection/3` writes a full clique over the selection, so the + # account's dismissed set grows without limit — loading all of it made every + # derivation proportional to the account's whole dismissal history rather + # than to the work actually pending. + # + # The endpoints came from the join above, so they are already known live and + # account-scoped; the contacts join is not repeated here. + defp blocking_dismissed([], _account_id), do: [] + + defp blocking_dismissed(pending, account_id) do + ids = + Enum.reduce(pending, MapSet.new(), fn d, acc -> + acc |> MapSet.put(d.contact_id) |> MapSet.put(d.duplicate_contact_id) + end) + |> MapSet.to_list() + + DuplicateCandidate + |> scope_to_account(account_id) + |> where([d], d.status == "dismissed") + |> where([d], d.contact_id in ^ids and d.duplicate_contact_id in ^ids) + |> select([d], d) + |> Repo.all() + end + # Dismissed pairs indexed as contact_id => the set of contact ids it was # dismissed against, so `blocked?/4` can look up membership instead of - # scanning every dismissed row. Dismissed rows are permanent negative edges - # (never deleted), so that set grows for the life of the account while the - # pending set stays small — without this index `blocked?/4` was the - # dominant cost of every cluster derivation. + # scanning them. Only the pairs `blocking_dismissed/2` kept reach here. defp index_negatives(dismissed) do Enum.reduce(dismissed, %{}, fn d, acc -> acc @@ -505,6 +568,7 @@ defmodule Kith.DuplicateDetection do Returns `:ok`, or `{:error, reason}` if the transaction failed. """ def dismiss_selection(account_id, selected_ids, unchecked_ids) do + invalidate_cluster_count(account_id) now = DateTime.utc_now() |> DateTime.truncate(:second) selected_ids = owned_ids(account_id, selected_ids) diff --git a/lib/kith_web/live/contact_live/cluster_merge.ex b/lib/kith_web/live/contact_live/cluster_merge.ex index 28fff0be..d069d6c3 100644 --- a/lib/kith_web/live/contact_live/cluster_merge.ex +++ b/lib/kith_web/live/contact_live/cluster_merge.ex @@ -137,9 +137,8 @@ defmodule KithWeb.ContactLive.ClusterMerge do @impl true def handle_event("toggle-member", %{"id" => id}, socket) do - with {:ok, id} <- cast_member_id(id) do - toggle_member(socket, id) - else + case cast_member_id(id) do + {:ok, id} -> toggle_member(socket, id) :error -> {:noreply, socket} end end diff --git a/test/kith/contacts/merge_summary_test.exs b/test/kith/contacts/merge_summary_test.exs index 06c8b3e9..4da66c9e 100644 --- a/test/kith/contacts/merge_summary_test.exs +++ b/test/kith/contacts/merge_summary_test.exs @@ -134,6 +134,35 @@ defmodule Kith.Contacts.MergeSummaryTest do assert summary.history.calls == 0 end + # `Merge`'s `remap_relationships/4` rewrites `related_contact_id` as well as + # `contact_id`, so a member that is only ever the target of relationships + # still has rows moved onto the survivor. Counting the first endpoint alone + # reported 0 for it and understated the "records move to the primary" line. + test "history counts relationships pointing at a member as well as from it", ctx do + [friend_type | _] = Repo.all(from(rt in "relationship_types", select: rt.id, limit: 1)) + + outsider = + ContactsFixtures.contact_fixture(ctx.user.account_id, %{first_name: "Outsider"}) + + # Neither member is the `contact_id` of either row. + ContactsFixtures.relationship_fixture(outsider, ctx.a, friend_type) + ContactsFixtures.relationship_fixture(outsider, ctx.b, friend_type) + + summary = MergeSummary.build([ctx.a, ctx.b]) + + assert summary.history.relationships == 2 + end + + test "history counts a relationship between two members once", ctx do + [friend_type | _] = Repo.all(from(rt in "relationship_types", select: rt.id, limit: 1)) + + ContactsFixtures.relationship_fixture(ctx.a, ctx.b, friend_type) + + summary = MergeSummary.build([ctx.a, ctx.b]) + + assert summary.history.relationships == 1 + end + # `Kith.Contacts.Merge`'s dedupe runs `lower(btrim(value))`, and one-argument # `btrim` strips SPACES ONLY. `String.trim/1` strips every Unicode whitespace # character, so a tab-prefixed value used to read as "duplicate · dropped" on diff --git a/test/kith/duplicate_detection_test.exs b/test/kith/duplicate_detection_test.exs index 93172741..95401778 100644 --- a/test/kith/duplicate_detection_test.exs +++ b/test/kith/duplicate_detection_test.exs @@ -604,6 +604,35 @@ defmodule Kith.DuplicateDetectionTest do assert Kith.DuplicateDetection.cluster_count(ctx.account_id) == 2 end + # cluster_count/1 is the sidebar badge and is cached per account, so every + # write path has to drop the entry. Without that, dismissing a cluster + # leaves the badge claiming it is still waiting for review. + test "cluster_count/1 reflects a dismissal made after it was first read", ctx do + %{a: a, b: b, d: d, e: e} = ctx.contacts + candidate!(ctx.account_id, a, b) + candidate!(ctx.account_id, d, e) + + assert Kith.DuplicateDetection.cluster_count(ctx.account_id) == 2 + + :ok = Kith.DuplicateDetection.dismiss_selection(ctx.account_id, [a.id, b.id], []) + + assert Kith.DuplicateDetection.cluster_count(ctx.account_id) == 1 + end + + # A dismissed pair only matters if both its endpoints are in the pending + # set, which is what `blocking_dismissed/2` relies on to stay bounded. + test "a dismissal against a contact with no pending pair changes nothing", ctx do + %{a: a, b: b, d: d} = ctx.contacts + candidate!(ctx.account_id, a, b) + candidate!(ctx.account_id, a, d, status: "dismissed") + + before = Kith.DuplicateDetection.list_clusters(ctx.account_id) + + assert Kith.DuplicateDetection.cluster_count(ctx.account_id) == 1 + assert [cluster] = before + assert Enum.sort(member_ids(cluster)) == Enum.sort([a.id, b.id]) + end + test "equal max_score breaks the tie by ascending key", ctx do %{a: a, b: b, d: d, e: e} = ctx.contacts candidate!(ctx.account_id, a, b, score: 0.8) From 68ebb90c9f4706d58d01ad1332d2444df509fbc6 Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Fri, 28 Aug 2026 02:17:10 +0300 Subject: [PATCH 25/27] test: stop the ToS flag leaking between concurrent async cases maybe_require_tos/1 reads :require_tos_acceptance from Application.get_env/3, which is node-global. TosAcceptanceTest and RegistrationTest both flipped it while running async: true, so the setting leaked into every case running at the same time. Any concurrent test building a registration changeset without tos_accepted failed on the acceptance validation, and the two modules clobbered each other's expectations in return. That is what made settings_multitenancy_test.exs:324 flaky: it passes raw attrs to accept_invitation/2 rather than going through AccountsFixtures, which always sets tos_accepted: true. Running those three files together reproduced it in 2 of 5 runs, failing 3 tests each time; 6 of 6 runs are clean now, as are three full suite runs. Both modules are async: false, which is already what every other env-mutating test module here does -- ExUnit runs sync cases serially and never alongside async ones. --- test/kith/accounts/tos_acceptance_test.exs | 8 +++++++- test/kith_web/live/user_live/registration_test.exs | 4 +++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/test/kith/accounts/tos_acceptance_test.exs b/test/kith/accounts/tos_acceptance_test.exs index c5a58769..1bf12e10 100644 --- a/test/kith/accounts/tos_acceptance_test.exs +++ b/test/kith/accounts/tos_acceptance_test.exs @@ -1,5 +1,11 @@ defmodule Kith.Accounts.TosAcceptanceTest do - use Kith.DataCase, async: true + # async: false — the ToS requirement is read from `Application.get_env/3`, + # which is node-global. Flipping it inside an async case leaks into every + # other case running at the same time: any concurrent test building a + # registration changeset without `tos_accepted` fails, and this module's own + # expectations get clobbered in return. ExUnit runs sync cases serially and + # never alongside async ones, which is what makes the mutation safe here. + use Kith.DataCase, async: false alias Kith.Accounts.User diff --git a/test/kith_web/live/user_live/registration_test.exs b/test/kith_web/live/user_live/registration_test.exs index dba5cd76..7b36796f 100644 --- a/test/kith_web/live/user_live/registration_test.exs +++ b/test/kith_web/live/user_live/registration_test.exs @@ -1,5 +1,7 @@ defmodule KithWeb.UserLive.RegistrationTest do - use KithWeb.ConnCase, async: true + # async: false — this module flips :require_tos_acceptance, which is + # node-global. See the note in Kith.Accounts.TosAcceptanceTest. + use KithWeb.ConnCase, async: false import Phoenix.LiveViewTest import Kith.AccountsFixtures From 5343a52c6ed0204311c5b23cd137855c6b375786 Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Fri, 28 Aug 2026 02:20:06 +0300 Subject: [PATCH 26/27] docs: state merge-screen comment rationale without narrating code history Ten comments across the cluster merge screen, MergeFields, MergeSummary and their tests explained a constraint by describing what the code did before it held -- "used to match on the label", "were offered as clearable", "before these guards". A reader cannot check that against the code in front of them, and it dates the comment. Each now states the same constraint in the present, keeping the reasoning that made it worth writing: what breaks if the invariant is dropped, not what broke once. No behaviour change. The commented-out block in user_auth.ex is left alone -- it is the phx_gen_auth generator's illustrative example, not dead code, and this branch does not touch it. --- lib/kith/contacts/merge_fields.ex | 4 +-- lib/kith/contacts/merge_summary.ex | 6 ++-- lib/kith/duplicate_detection.ex | 6 ++-- .../live/contact_live/cluster_merge.ex | 6 ++-- test/kith/contacts/merge_summary_test.exs | 10 +++---- .../live/contact_live/cluster_merge_test.exs | 28 +++++++++---------- 6 files changed, 29 insertions(+), 31 deletions(-) diff --git a/lib/kith/contacts/merge_fields.ex b/lib/kith/contacts/merge_fields.ex index 691179a5..c77c11f9 100644 --- a/lib/kith/contacts/merge_fields.ex +++ b/lib/kith/contacts/merge_fields.ex @@ -81,8 +81,8 @@ defmodule Kith.Contacts.MergeFields do merge engine's documented `{:error, reason}` contract — and the changeset's required list does not cover them: `birthdate_year_unknown` and `first_met_year_unknown` are `null: false` booleans that no - `validate_required/2` names, so they were offered as clearable and two clicks - on the cluster merge screen crashed the LiveView. + `validate_required/2` names, so the column check is the only thing standing + between them and a crash on the cluster merge screen. Read from `information_schema` rather than restated here so a column added with `null: false` later is covered on its own. diff --git a/lib/kith/contacts/merge_summary.ex b/lib/kith/contacts/merge_summary.ex index 70ff00fe..fc6e4e75 100644 --- a/lib/kith/contacts/merge_summary.ex +++ b/lib/kith/contacts/merge_summary.ex @@ -148,9 +148,9 @@ defmodule Kith.Contacts.MergeSummary do # Relationships are the one history table a member can be on either end # of, and `Merge`'s `remap_relationships/4` rewrites both `contact_id` # and `related_contact_id`. Counting only the first endpoint reports 0 - # for a member that is purely the *target* of relationships, so the - # screen understated what the merge moves. A row with both endpoints in - # the cluster is still one row, so `or` counts it once. + # for a member that is purely the *target* of relationships, which + # understates what the merge moves. A row with both endpoints in the + # cluster is still one row, so `or` counts it once. {:relationships, schema} -> {:relationships, Repo.aggregate( diff --git a/lib/kith/duplicate_detection.ex b/lib/kith/duplicate_detection.ex index 918fab16..7f918446 100644 --- a/lib/kith/duplicate_detection.ex +++ b/lib/kith/duplicate_detection.ex @@ -325,9 +325,9 @@ defmodule Kith.DuplicateDetection do # # This is what keeps the derivation bounded. Dismissed rows are permanent and # `dismiss_selection/3` writes a full clique over the selection, so the - # account's dismissed set grows without limit — loading all of it made every - # derivation proportional to the account's whole dismissal history rather - # than to the work actually pending. + # account's dismissed set grows without limit; loading all of it would make + # every derivation proportional to the account's whole dismissal history + # rather than to the work actually pending. # # The endpoints came from the join above, so they are already known live and # account-scoped; the contacts join is not repeated here. diff --git a/lib/kith_web/live/contact_live/cluster_merge.ex b/lib/kith_web/live/contact_live/cluster_merge.ex index d069d6c3..62d45d62 100644 --- a/lib/kith_web/live/contact_live/cluster_merge.ex +++ b/lib/kith_web/live/contact_live/cluster_merge.ex @@ -17,9 +17,9 @@ defmodule KithWeb.ContactLive.ClusterMerge do # The value sections, keyed on the identifier the drop payload and the # summary both use. The display label is a rendering detail hanging off that - # key, never the key itself: `drop_key/1` used to match on the label, so a - # copy edit or an i18n pass would have made it return `nil` for everything - # and every unchecked value would have silently come back on the survivor. + # key, never the key itself: were `drop_key/1` to match on the label, a copy + # edit or an i18n pass would make it return `nil` for everything, and every + # unchecked value would silently come back on the survivor. @sections [ {:contact_fields, "Fields"}, {:addresses, "Addresses"}, diff --git a/test/kith/contacts/merge_summary_test.exs b/test/kith/contacts/merge_summary_test.exs index 4da66c9e..2b06f4d5 100644 --- a/test/kith/contacts/merge_summary_test.exs +++ b/test/kith/contacts/merge_summary_test.exs @@ -137,7 +137,7 @@ defmodule Kith.Contacts.MergeSummaryTest do # `Merge`'s `remap_relationships/4` rewrites `related_contact_id` as well as # `contact_id`, so a member that is only ever the target of relationships # still has rows moved onto the survivor. Counting the first endpoint alone - # reported 0 for it and understated the "records move to the primary" line. + # reports 0 for it and understates the "records move to the primary" line. test "history counts relationships pointing at a member as well as from it", ctx do [friend_type | _] = Repo.all(from(rt in "relationship_types", select: rt.id, limit: 1)) @@ -165,10 +165,10 @@ defmodule Kith.Contacts.MergeSummaryTest do # `Kith.Contacts.Merge`'s dedupe runs `lower(btrim(value))`, and one-argument # `btrim` strips SPACES ONLY. `String.trim/1` strips every Unicode whitespace - # character, so a tab-prefixed value used to read as "duplicate · dropped" on - # screen while the engine kept both rows — and a caller expanding a dropped - # entry over everything sharing its key would then drop a value the user - # never excluded. + # character. Trimming with it would make a tab-prefixed value read as + # "duplicate · dropped" on screen while the engine keeps both rows, and a + # caller expanding a dropped entry over everything sharing its key would then + # drop a value the user never excluded. test "a tab-prefixed value is not a duplicate, matching the engine's btrim", ctx do ContactsFixtures.contact_field_fixture(ctx.a, ctx.email_type.id, %{"value" => "s@example.com"}) diff --git a/test/kith_web/live/contact_live/cluster_merge_test.exs b/test/kith_web/live/contact_live/cluster_merge_test.exs index d00ce5a1..ab782e55 100644 --- a/test/kith_web/live/contact_live/cluster_merge_test.exs +++ b/test/kith_web/live/contact_live/cluster_merge_test.exs @@ -750,8 +750,8 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do # Spec B6: "Resolved rows show the value plus attribution and are # click-to-change, opening in place as a segmented control including 'Leave - # empty'." Before this, the non-conflict branch rendered two inert spans - # while the section header claimed every value was clickable. + # empty'." The section header tells the user every value is clickable, so the + # non-conflict branch has to render controls rather than inert spans. describe "resolved rows are editable (B6/B7)" do test "a resolved row exposes its candidates as a segmented control", ctx do {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) @@ -939,10 +939,10 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do end describe "drop payload keys" do - # `drop_key/1` used to match the literal display labels the template - # supplied ("Fields"/"Addresses"/"Tags"). A copy edit or an i18n pass would - # have made it return nil for everything, `build_drop/1` return `%{}`, and - # every unchecked value silently come back on the survivor. + # `drop_key/1` must not match the literal display labels the template + # supplies ("Fields"/"Addresses"/"Tags"). If it did, a copy edit or an i18n + # pass would make it return nil for everything, `build_drop/1` return `%{}`, + # and every unchecked value silently come back on the survivor. test "value rows are keyed on section identifiers, not display copy", ctx do email_type = Kith.Repo.one!( @@ -1023,9 +1023,8 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do describe "not-null columns are never offered as clearable" do # `:clear` becomes `nil` at the database, so a `NOT NULL` column raises a # 23502 outside the engine's `{:error, reason}` contract. The year-unknown - # flags used to reach the screen as their own rows and made this trivially - # reachable; they are coupled fields now and render no control at all, so - # the guard that still has to hold is over the choice fields. + # flags are coupled fields and render no control at all, so the guard that + # has to hold on this screen is over the choice fields. test "no non-clearable choice field renders a Leave empty button", ctx do {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) @@ -1090,12 +1089,11 @@ defmodule KithWeb.ContactLive.ClusterMergeTest do end describe "crafted event payloads" do - # `phx-value-*` is client-controlled. Before these guards, `choose-field` - # ran `String.to_existing_atom/1` on it and handed the result to - # `Map.fetch!/2`: any atom the node had already created but that is not a - # Contact key — `:page_title`, `:flash`, an assign name — killed the - # LiveView process. The id handlers had the same shape via - # `String.to_integer/1`, which raises on anything non-numeric. + # `phx-value-*` is client-controlled, so nothing stops a crafted payload + # naming an atom that exists on the node but is not a Contact key — + # `:page_title`, `:flash`, an assign name. Reaching `Map.fetch!/2` with one + # kills the LiveView process, as does a non-numeric id reaching + # `String.to_integer/1`. test "an unknown field name is ignored rather than crashing the view", ctx do {:ok, live, _html} = live(ctx.conn, cluster_path(ctx.a, ctx.b)) From da29e671da4f4abd3d8a7147fa39ef2054a208ed Mon Sep 17 00:00:00 2001 From: Bashar Qassis <23612682+bashar-qassis@users.noreply.github.com> Date: Fri, 28 Aug 2026 19:12:28 +0300 Subject: [PATCH 27/27] fix: keep merge-screen spec comments out of the rendered HTML Two comment blocks in the cluster merge template used which HEEx treats as markup, so they compiled into the response body and shipped the internal design-spec references to anyone viewing page source. <%!-- --%> is the compile-time form and stays in the source file. --- lib/kith_web/live/contact_live/cluster_merge.ex | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/kith_web/live/contact_live/cluster_merge.ex b/lib/kith_web/live/contact_live/cluster_merge.ex index 62d45d62..f36d4cc4 100644 --- a/lib/kith_web/live/contact_live/cluster_merge.ex +++ b/lib/kith_web/live/contact_live/cluster_merge.ex @@ -770,11 +770,11 @@ defmodule KithWeb.ContactLive.ClusterMerge do
{humanize(field)}
- + --%>
- + --%>