Comfy-Org / Comfy-Org/ComfyUI_frontend

ADR 0003's per-store collision-contract table is wrong in four places

Open
#15,743 0 comments 0 reactions 1 assignee Claimed by @DrJKL View on GitHub
documentation
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

ADR 0003's per-store collision-contract table (`docs/adr/0003-crdt-based-layout-system.md:227-247`, merged in #15707 at 2026-08-23T07:07:28Z) is wrong in four places, verified against the stores at the same commit `f1bfb313d6`. #15701, filed eight minutes earlier, documents the store behaviour correctly; the ADR contradicts it. The decision the table supports (merge-boundary reconciliation, stores as commitment layer) is correct and unaffected — only the table's description of current behaviour is wrong. Same class as #15618: a governing ADR misdescribing implemented contracts, one day after the last one.

## The four errors

**1. `nodeDataStore.registerNode` is not "identity-keyed (Set membership)".** It is id-keyed: `existingBucket?.byId.get(state.id)` on a `Map` (`src/stores/nodeDataStore.ts:49`), with an identity *escape*: same raw object and same `graphId` returns the incumbent (`:50-56`). A different `NodeState` with the same id is rejected with `return undefined` (`:57`) — a rejection path the ADR does not describe at all. "Re-adding the same `NodeState` object is a no-op" is the escape hatch, not the keying.

**2. `linkStore.registerLink` is not "first-registration-wins per target input slot" as its primary rule.** It rejects on duplicate **link id** first (`src/stores/linkStore.ts:208-214`, root-scoped bucket, `console.error`), and the per-slot rule applies only to non-floating topologies: `hasUniqueTarget(topology) = !isFloatingTopology(topology)` (`:90-92`) guards the slot check, so floating links can share an input slot by design. The ADR states the secondary rule as the whole contract and omits the floating exemption.

**3. `rerouteStore` does not "log a warning".** `src/stores/rerouteStore.ts:131` is `console.error`.

**4. "discard the other without surfacing a diagnostic" is wrong for two of four stores, and mischaracterises a third.** At `f1bfb313d6`: `linkStore` errors on both collision paths (`:210` duplicate id, `:223` occupied slot); `rerouteStore` errors (`:131`); `nodeDataStore` is the only silent rejector (until #15720 lands, which adds `console.error` there); and `widgetValueStore` does not "keep whichever arrived first" on type mismatch — it overwrites, keeping the newcomer (`src/stores/widgetValueStore.ts:103-107`; its `console.warn:94` is on the un-keyable-id path, not collision). The sentence is accurate only for `nodeDataStore`, and #15720 removes that case.

## Suggested correction

Replace the four bullets and the closing sentence with per-store rows stating: key type (id-keyed Map vs structural key), rejection vs overwrite semantics, exact diagnostic (`console.error` / silent / n/a), and the floating-link exemption for `linkStore`. #15701's table is already correct and can be lifted.

Cross-refs: #15701 (correct table), #15707 (introduced the ADR text), #15720 (changes the nodeDataStore diagnostic), #15618 (same defect class in ADR 0008).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.