fix(lens): group variants by the element label, not the matcher - #676
Open
filip131311 wants to merge 1 commit into
Open
fix(lens): group variants by the element label, not the matcher#676filip131311 wants to merge 1 commit into
filip131311 wants to merge 1 commit into
Conversation
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.
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
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.
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 onelement. Because that default equals the label, calls that omitmatchaccumulate correctly — which is exactly why this stayed hidden.Reported direction — supplying
matchforks a second card:Unreported direction — and this one is worse. The key is symmetric, so two different labels sharing a matcher value merge. Verified deliberately:
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 carryingmatch: {text: SUBMIT}landed onel-qa-probe-button-2.The fix
Identity is the
elementlabel, compared ignoring case and surrounding whitespace — which keeps it coherent with the id slug, that already folds both.matchbecomes 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:Silently dropping it would have been the same class of bug as the one being fixed.
One UI change was required.
index.htmllocks 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.goneis false underrevealOffscreen, 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
variantCountwas per-proposal, so an element's second variant could be calledv5— 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
totalElements: 1, counts 1 → 2 → 3Known 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.mdnow states the label is the identity.Checks
element_countper round will read lower where labels previously forked, so pre/post Lens funnel numbers aren't strictly comparable.argent-lensflag. Skills gate 10.0; extract-tools 46/46; prettier, eslint, both typechecks clean; lock untouched.