Skip to content

fix(lens): group variants by the element label, not the matcher - #676

Open
filip131311 wants to merge 1 commit into
mainfrom
filip/variant-identity-by-element
Open

fix(lens): group variants by the element label, not the matcher#676
filip131311 wants to merge 1 commit into
mainfrom
filip/variant-identity-by-element

Conversation

@filip131311

Copy link
Copy Markdown
Collaborator

Fixes #624.

The bug, and a worse half the issue doesn't mention

Identity was keyed on match (defaulted to {by:"text", value: element}), never on element. Because that default equals the label, calls that omit match accumulate correctly — which is exactly why this stayed hidden.

Reported direction — supplying match forks a second card:

element "QA probe button", no match      → el-qa-probe-button-1
element "QA probe button", match SUBMIT  → el-qa-probe-button-2   ← forked, totalElements: 2

Unreported direction — and this one is worse. The key is symmetric, so two different labels sharing a matcher value merge. Verified deliberately:

element "Header logo",  match {text: SHARED} → el-header-logo-4
element "Footer link",  match {text: SHARED} → el-header-logo-4
                                               and the response came back
                                               element: "Header logo"

I asked to stage a variant for Footer link and it was filed under Header logo, with the response naming the wrong element and nothing indicating another element had absorbed it. A human picker card would show one component's variants under another's name. I hit this by accident first: a fresh "Checkout CTA" call carrying match: {text: SUBMIT} landed on el-qa-probe-button-2.

The fix

Identity is the element label, compared ignoring case and surrounding whitespace — which keeps it coherent with the id slug, that already folds both.

match becomes a locator. A matcher the agent supplies replaces the label-derived default (that default is a synthesized placeholder; a real matcher is strictly better). Two different explicit matchers for one label is ambiguous — the agent may have meant two elements — so the first is kept and the second is reported:

Kept the matcher this element already had (identifier=buy-top) and ignored identifier=buy-bottom
— variants group by the `element` label, so give a different label if these are different elements.

Silently dropping it would have been the same class of bug as the one being fixed.

One UI change was required. index.html locks a card's anchor against the matcher it first resolved and never invalidates it, so an upgraded locator would leave the card homed on the old node. It now re-homes when the matcher changes.

I initially wanted to avoid touching the UI by never mutating the stored matcher — the critique showed that was the wrong trade. A stale anchor yields an unanchored but revealable card (rec.gone is false under revealOffscreen, and there's an explicit off-screen prompt), whereas keeping a fuzzy first matcher leaves a card confidently pointing at the wrong element forever. And the UI ships inside the tool-server bundle, so there's no version skew to avoid.

Variant ids are now per-element. They came from a store-wide counter while variantCount was per-proposal, so an element's second variant could be called v5 — that mismatch is how the reporter first noticed something was wrong. Safe: reset() already zeroed the counter each round, so store-wide uniqueness was never a property anything could rely on, and every consumer (store lookups, buildOutcome, the UI staged map, drag payload, submit) is elementId-scoped.

Verified live

sequence result
the issue's three calls one card, totalElements: 1, counts 1 → 2 → 3
two labels sharing a matcher two cards, each response naming its own element
conflicting explicit matchers first kept, second reported in the hint

Known consequence

Two genuinely different elements that share a label now merge — nothing forced distinct labels before. That's the flip side of making the label the identity, it's what the docs already promised, and the hint names the remedy when it's detectable. SKILL.md now states the label is the identity.

Checks

  • 3092 tests pass. 6 new, all 6 failing against the pre-fix source; the 54 existing variant tests pass unmodified.
  • Telemetry note: element_count per round will read lower where labels previously forked, so pre/post Lens funnel numbers aren't strictly comparable.
  • Behind the argent-lens flag. Skills gate 10.0; extract-tools 46/46; prettier, eslint, both typechecks clean; lock untouched.

propose_variant keyed an element's identity on "match", defaulted to
{by:'text', value: element}. Because the default equals the label, calls that
omitted "match" appeared to accumulate correctly and hid the problem — but
supplying it, which the docs recommend, produced a second picker card, so the
human was asked to choose between halves of one set.

The key was symmetric, so it failed the other way too, and that direction is
worse: two different labels sharing a matcher value merged. Staging a variant
for 'Footer link' filed it under 'Header logo' and returned that other label in
the response, with nothing to indicate a different element had absorbed it. A
picker card would have shown one component's variants under another's name.

Identity is now the label, compared ignoring case and surrounding whitespace so
it agrees with the id slug, which already folds both.

"match" becomes a locator. One the agent supplies replaces the label-derived
default, since that default is a synthesized placeholder and a real matcher is
strictly better. Two different explicit matchers for one label is ambiguous —
the agent may have meant two elements — so the first is kept, and the second is
reported in the result and named in the hint together with the remedy, because
dropping it silently would be the same class of bug as the one being fixed.

The preview window locks a card's anchor against the matcher it first resolved
and never invalidates it, so an upgraded locator would have left the card homed
on the old node. It now re-homes when the matcher changes.

Variant ids were drawn from a store-wide counter while variantCount was
per-proposal, so an element's second variant could be called v5. That mismatch
is how the reporter first noticed something was wrong. Ids are per-element now;
the counter was already reset every round, so store-wide uniqueness was never a
property anything could rely on.

Both tools stay behind the argent-lens flag.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

propose_variant: passing match forks a second element instead of accumulating variants on the same one

1 participant