Repository navigation
Merge revamp slice 2: clusters and the single-screen merge - #28
Merged
Merged
Conversation
bashar-qassis
changed the title
bashar-qassis
force-pushed
the
merge-revamp-slice-2
branch
from
August 19, 2026 21:37
252818f to
a1205d0
Compare
bashar-qassis
force-pushed
the
merge-revamp-slice-2
branch
from
August 19, 2026 22:00
a1205d0 to
252818f
Compare
…terminism 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
…ue writes, id tiebreak
…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.
…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.
…d last uncheck, fix alias id casting
…d first-met-through
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`.
bashar-qassis
force-pushed
the
merge-revamp-slice-2
branch
from
August 19, 2026 22:13
0e0e6f1 to
31026a3
Compare
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.
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.
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.
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.
…tory 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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Slice 2 of the duplicate-contact merge revamp. Replaces the pairwise duplicates queue and the three-step merge wizard with a cluster list and a single-screen cluster merge, built on slice 1's resolver and engine.
Stacked on #27 — review that one first; this diff sits on top of it.
Spec:
docs/superpowers/specs/2026-08-17-duplicate-merge-revamp-design.mdPlan:
docs/superpowers/plans/2026-08-17-merge-revamp-slice-2-clusters-screen.mdWhat's here
list_clusters/2computes components from candidate pairs on every read, so they cannot drift from the pairs that justify them. The build is greedy: pending pairs sorted by{-score, contact_id, duplicate_contact_id}, and a pair unions its endpoints only if the result contains no dismissed pair — dismissals are negative edges, so a match the user rejected can never reappear by transitivity through a third contact.MergeSummary— the multi-valued records and history counts the screen shows, with duplicates marked rather than removed so the screen can show what the merge will collapse./contacts/duplicates/cluster/:id— member strip with checkboxes and a primary chip, an Identity section where every mergeable field is visible and editable in place, Contact details as prunable checklists, history as counts, and one Immich row listing linked members by name.Spec scenarios
A1–A6, B1–B10, C1–C5, F1–F3, F5–F6, G2. The two end-to-end scenarios §7 asks for by name — the handled-cluster scenario and the two-people scenario — are their own tests in
test/kith/duplicate_merge_scenarios_test.exs, driven through the real detection worker, resolver and engine rather than hand-placed state.Notable decisions
Each was a conflict between the plan and either the spec or the code; the spec won.
MergeSummarycarries a dedupe-equivalence key for exactly this; the leak is cross-owner (clicked row on a loser, equivalent on the survivor) and there is a test constructed to fail without the expansion.MergeFields.non_clearable/0now unions the changeset'svalidate_requiredlist with theNOT NULLcolumns read frominformation_schema, because "Leave empty" onbirthdate_year_unknownproduced a23502out of the engine, outside its documented error contract. Derived rather than hardcoded, so a futurenull: falsecolumn is covered on its own; cached in:persistent_term, so a nullability migration needs a restart to take effect.immich_status: "unlinked"rather than nil — slice 1 exempts that group from held-by-member validation, so a nil would hit the CHECK constraint as an unhandled error.first_met_through_idrow, since the engine coerces a self-reference to empty and the user would otherwise pick a value that silently vanishes.Verification
mix precommitgreen (compile--warnings-as-errors,deps.unlock --unused, format,credo --strict, test).Before merging
.github/workflows/ci.ymltriggers only on PRs targetingmain/master, and this one targets the slice 1 branch. Only the bottom of the stack is gated.first_met_through_idrow mixing a member and an outsider renders an unhighlighted candidate that submits as cleared.🤖 Generated with Claude Code
https://claude.ai/code/session_01DCMNiVE7SKJeMusAn7DiMd