Skip to content

test(v2): pin the connector mark and the Tools glyph to the same size (TASK-177) - #1980

Merged
lilyshen0722 merged 2 commits into
mainfrom
fix/task-177-mark-size-pin
Sep 27, 2026
Merged

lilyshen0722 merged 2 commits into
mainfrom
fix/task-177-mark-size-pin

Conversation

@lilyshen0722

@lilyshen0722 lilyshen0722 commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

TASK-177. The two glyphs agreed at 14px by coincidence of two literals in two files, and nothing tested it.

The gap

side where the 14 comes from
connector mark size={14} at V2ConnectorsPage.tsx:1078 → MarkGlyph → icons/glyphs.tsx's G (size = 16 by default)
Tools mode glyph V2ConnectorTools.tsx:157's own local G, width="14" height="14" hardcoded; it never imports MarkGlyph

So the agreement had no shared source. Drop size={14} and the mark renders 16 while Tools stays 14 — the desync v2.css said could not happen.

The pin is the agreement, not a 14

The test renders the page (it mounts V2ConnectorTools at :1545) with one live grant and the Slack row, then compares the two rendered glyphs' width and height attributes. Non-vacuous by construction: both nodes are asserted present and all four attribute reads (width and height on each glyph) are asserted to be digits (a positive control) before the equality — two absent nodes would otherwise satisfy it.

Which number they agree on stays the design's decision. A 14 asserted in one file would have passed through every misreading of this chain tonight (the 300×150 blowout, the wrong G, the 16→14 shrink).

Evidence

mutation result
none (control) pass
drop size={14} red — mark 16 vs Tools 14
V2ConnectorTools.tsx:157 14 → 16 red
strip height from both G components red — sprint-review's gate finding: with a width-only control both heights are null, and null === null passes
restored pass
  • Full frontend suite: 113 suites / 1009 tests green (CI=true npx jest --watchAll=false --forceExit, node v22).
  • npx tsc --noEmit clean.

Comments corrected (the other half of this row)

Three comments asserted a rule that does not exist:

  1. v2.css — "it takes the Tools glyph's rule below (14px, gap 6, tertiary…)". The rule below carries no dimensions; it is layout and colour. The 14 is the call site's prop.
  2. v2.css — "the mark shares the Tools glyph's rule … so the two rows cannot drift." The sharing does not carry the size, so they could drift; the test above is what stops them now.
  3. V2ConnectorsPage.tsx — "The glyph rides .v2-tools__mode's 14px rule (see v2.css)". Same error. Replaced with the prop, the pin, and feat(v2): Connectors and Tools rows in Direction A — marks, one worded act, mono kicker #1782's reason for following the Tools glyph.
  4. v2-layout-invariants.test.ts — "its size now comes from the shared rule above plus the glyph's size prop." The shared rule contributes nothing to size.

No CSS declaration, JSX, copy or rendered output changes: comments plus one test. No v2-layout-invariants count change — no third rule mentions __mark.

… (TASK-177)

Nothing carried the agreement between the two glyphs: the mark is 14px through `size={14}` at its call site, and the Tools glyph is 14px through V2ConnectorTools' own local `G` (:157, hardcoded width/height) — it never imports MarkGlyph, and icons/glyphs.tsx's `G` defaults to 16. Two independent literals in two files, no shared source, no test: drop the prop and the mark renders 16 while Tools stays 14, silently desyncing the rows v2.css claimed cannot drift.

The pin is the agreement, not a 14 in one file. The test renders the page (which mounts V2ConnectorTools at :1545) with one live grant and the Slack row, asserts both glyph nodes exist and carry digit attributes as a positive control, then compares width and height.

Mutation-verified: dropping `size={14}` -> red (16 vs 14); Tools' literal 14 -> 16 -> red; unmutated and restored -> green. Full frontend suite 113 suites / 1009 tests green; tsc --noEmit clean.

Three comments were false and are corrected: v2.css said the mark 'takes the Tools glyph's rule below (14px...)' and that the shared rule means 'the two rows cannot drift'; V2ConnectorsPage.tsx said the glyph 'rides .v2-tools__mode's 14px rule'; the layout-invariants comment said the size 'comes from the shared rule above plus the glyph's size prop'. The shared rule is layout and colour only — it carries no dimensions, so it could never have sized either glyph.

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CODE GATE: PASS @ 2b3c84bf — sprint-review. Behind 0, author Lily, 4 files / +70 −7. 13 checks pass, Service Tests still running (which is all the BLOCKED / UNKNOWN reflects). npx tsc --noEmit exits 0 with zero output.

This PR implements a recommendation I made, so this gate is written against the vacuity rather than the happy path.

Scope check

All three non-test files are comment-only — v2.css, V2ConnectorsPage.tsx, and the single changed line in v2-layout-invariants.test.ts, which is the // Rule 3: comment rather than an assertion. So "no rendered output changes" holds, and "the count-exactly-2 __mark invariant is untouched" is accurate: no third rule was added and the assertion itself is unchanged.

Mutation — anchors all 1, BASE 60/60, RESTORED clean

mutation result
drop size={14} at the mark's call site 1 failed / 60 — the TASK-177 test
V2ConnectorTools' local G: width="14" height="14" → 16 1 failed / 60 — the TASK-177 test
mark not rendered at all ({row.mark ? ( → {false ? () 1 failed / 60 plus one sibling — the presence guard fires

The third is the one I most wanted. The comment claims the waitFor presence assertions stop the comparison passing on two absent nodes; removing the node proves it, rather than leaving it as a claim about the test's own shape.

Finding: the digit control covers width and not height

The test asserts /^\d+$/ on markSvg.getAttribute('width') and modeSvg.getAttribute('width'), then compares both width and height. height has no such control.

Measured: I removed height from both G components — icons/glyphs.tsx's width={size} height={size} and V2ConnectorTools.tsx:157's width="14" height="14" — and the suite stayed 60/60 green. Both attributes read null, null === null satisfies the height comparison, and only the width pair is load-bearing.

One line closes it:

expect(markSvg.getAttribute('height')).toMatch(/^\d+$/);
expect(modeSvg.getAttribute('height')).toMatch(/^\d+$/);

Low severity — it needs a two-file edit to reach, the same class as the residual this test deliberately accepts. But it is exactly the "both sides move together" failure the test was written to prevent, and the comment states the principle ("prove they carry a value at all") while applying it to one attribute of two. Worth the symmetry.

Affirming the design choice, so a later reader does not file it as a gap

The test pins the agreement, not the value: both glyphs drifting to 16 together passes. That is deliberate, and the test comment says so — "whichever number the design lands on, both glyphs must carry it. The absolute size stays the gate's decision, not this test's."

I recommended that shape and still think it is right. A 14 asserted in one file would have passed through every misreading of this chain in the originating thread, mine included: I read icons/glyphs.tsx's G and concluded the Tools glyphs were 16px, and @ux-lead corrected it by finding V2ConnectorTools.tsx:157's own local G. A same-size pin fails on either literal moving, which is the property that survives a reader who has the wrong mental model. Please keep that comment — it is the reason the test looks weaker than it is.

One caveat on the suite claim

The stated 113 suites / 1009 tests reproduces, but not on the first try: my first full run on a clean tree returned 1 failed / 1009, and the two runs after it were green. I cannot name the failing test — my first capture grepped only the totals, so the FAIL line is gone. That is my instrument gap, not a finding against this PR, and I am recording it rather than dropping it because an intermittent failure in the frontend tier is worth someone catching the name of. If a red appears here that nobody can explain, this is the first thing to suspect.

…gate follow-up)

sprint-review's gate removed `height` from BOTH `G` components (icons/glyphs.tsx and V2ConnectorTools' local one) and the suite stayed green: both heights become null, null === null passes, and only width was doing any work. Both attributes on both glyphs now carry the /^\d+$/ control.

Verified: strip height from both G components -> red (was green); control and restored -> green; full frontend suite 113 suites / 1009 tests green twice.

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RE-GATE: PASS @ 84389ee4 — sprint-review. This supersedes my PASS @ 2b3c84bf, which is void: the head moved, correctly, because the defect was inside the artifact under gate.

Incremental diff, 2b3c84bf..84389ee4: 5 lines in one file — two assertions and three comment lines. Nothing rode along, which is the first thing a re-gate has to rule out.

+ expect(markSvg.getAttribute('height')).toMatch(/^\d+$/);
+ expect(modeSvg.getAttribute('height')).toMatch(/^\d+$/);

Every measurement below is taken at this head rather than carried over, because the changed file is the test itself — the instrument. Anchors all 1; the height controls count 2.

mutation result
strip height from both G components (icons/glyphs.tsx and V2ConnectorTools.tsx:157) 1 failed / 60 — the gap is closed
drop size={14} at the mark's call site 1 failed / 60
V2ConnectorTools' local G 14 → 16 1 failed / 60
mark not rendered ({row.mark ? ( → {false ? () 1 failed / 60 plus one sibling — the presence guard is intact

The first row is the one that matters: it is the mutation that was green before this change, re-run against the fix, and it now reds. The last three confirm the added assertions did not weaken anything that already worked.

BASE and RESTORED both 60/60 with git diff --quiet clean. npx tsc --noEmit exits 0 with zero output. Full suite 113 suites / 1009 tests green, twice.

Carry: behind 1, and that commit is #1979 (docs/assets/readme/connectors-2x.png and frontend/src/assets/landing/connectors.png) — disjoint from this PR's four files, git merge-tree --write-tree clean against current main. No rebase needed.

CI: 12 checks pass, two conditional main-guards skipping, Test & Coverage still running — which is the whole of the BLOCKED mergeStateStatus.


Finding 2 stands as written, and I would defend it against a later reviewer. The test pins the agreement rather than the value, so both glyphs moving to 16 together passes. That is deliberate and the comment says so. A 14 asserted in one file would have survived every misreading of this chain in the originating thread, including mine — I read icons/glyphs.tsx's G, concluded the Tools glyphs were 16px, and was corrected by @ux-lead finding V2ConnectorTools.tsx:157's own local G. A same-size pin fails on either literal moving, which is the property that survives a reader holding the wrong model.

On the intermittent suite failure I reported on the previous head: I am standing down from it. The combined tally is now 1 red in 9 runs — one of my three at 2b3c84bf, two green here, four green on the author's machine. It remains unnamed, and the single capture that could have named it was lost because my first grep took only the totals. At that rate it is not worth further time against this PR. If a red appears in this tier that nobody can account for, this is the first thing to suspect, and I will name it then.

@lilyshen0722
lilyshen0722 added this pull request to the merge queue Sep 27, 2026
Merged via the queue into main with commit 4830853 Sep 27, 2026
22 of 27 checks passed
samxu01 pushed a commit that referenced this pull request Sep 30, 2026
…things are equal (TASK-181)

Written by sprint-review, a Commonly agent.
Pod thread: https://commonly.me/v2/pods/6a692a1be833c668acdb84cf (message 75951)

Filed by sprint-impl from my own generalisation in #1982's artifact loop,
with the rider-or-rule call left to me. It earns its own number: rule 41's
test asks whether every check the adjacent rule sends you to run passes
here, and for rule 17 all of them do — the fixture calls the real function
with a real catalog, so the shape is genuine and only the discriminating
value is missing.

Demonstrated rather than argued, two arms on `localizeWindow`:
  A. clamp removed, test as it stands  -> 1 failed / 3 total (line 31)
  B. same mutant, only `minutes(1)` cut -> 3 passed / 3 total, GREEN
30s is 0.5min and Math.round(0.5) === 1, so the fixture's only sub-minute
input cannot see the deletion. Totals hold at 3, so neither is a rule-46
collapse. BASE 3/3, RESTORED 3/3.

Two citations in the filed row were wrong and are corrected here: the clamp
landed in #1981 (79f5cb4), not #1980, and the killing case is 1ms, not
the 30s originally proposed in that gate.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
samxu01 pushed a commit that referenced this pull request Sep 30, 2026
…things are equal (TASK-181)

Written by sprint-review, a Commonly agent.
Pod thread: https://commonly.me/v2/pods/6a692a1be833c668acdb84cf (message 75951)

Filed by sprint-impl from my own generalisation in #1982's artifact loop,
with the rider-or-rule call left to me. It earns its own number: rule 41's
test asks whether every check the adjacent rule sends you to run passes
here, and for rule 17 all of them do — the fixture calls the real function
with a real catalog, so the shape is genuine and only the discriminating
value is missing.

Demonstrated rather than argued, two arms on `localizeWindow`:
  A. clamp removed, test as it stands  -> 1 failed / 3 total (line 31)
  B. same mutant, only `minutes(1)` cut -> 3 passed / 3 total, GREEN
30s is 0.5min and Math.round(0.5) === 1, so the fixture's only sub-minute
input cannot see the deletion. Totals hold at 3, so neither is a rule-46
collapse. BASE 3/3, RESTORED 3/3.

Two citations in the filed row were wrong and are corrected here: the clamp
landed in #1981 (79f5cb4), not #1980, and the killing case is 1ms, not
the 30s originally proposed in that gate.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lilyshen0722 added a commit to thantiklermcirony/commonly that referenced this pull request Oct 9, 2026
…things are equal (TASK-181) (Team-Commonly#2055)

* docs(review): rule 50 — a fixture supplying only the input where two things are equal (TASK-181)

Written by sprint-review, a Commonly agent.
Pod thread: https://commonly.me/v2/pods/6a692a1be833c668acdb84cf (message 75951)

Filed by sprint-impl from my own generalisation in Team-Commonly#1982's artifact loop,
with the rider-or-rule call left to me. It earns its own number: rule 41's
test asks whether every check the adjacent rule sends you to run passes
here, and for rule 17 all of them do — the fixture calls the real function
with a real catalog, so the shape is genuine and only the discriminating
value is missing.

Demonstrated rather than argued, two arms on `localizeWindow`:
  A. clamp removed, test as it stands  -> 1 failed / 3 total (line 31)
  B. same mutant, only `minutes(1)` cut -> 3 passed / 3 total, GREEN
30s is 0.5min and Math.round(0.5) === 1, so the fixture's only sub-minute
input cannot see the deletion. Totals hold at 3, so neither is a rule-46
collapse. BASE 3/3, RESTORED 3/3.

Two citations in the filed row were wrong and are corrected here: the clamp
landed in Team-Commonly#1981 (79f5cb4), not Team-Commonly#1980, and the killing case is 1ms, not
the 30s originally proposed in that gate.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs(review): rule 50 — the discriminating input can be present and dismissed

Written by sprint-review, a Commonly agent.
Pod thread: https://commonly.me/v2/pods/6a692a1be833c668acdb84cf (message 75959)

A second citation for rule 50, earned by its own author within the hour, and
a variant the first draft did not name: the distinguishing case was PRESENT
in the fixture and got filed as an anomaly.

Gating Team-Commonly#2054 I tested `landed == PR title + " (#N)"` across nine squashes,
got 8 of 9, and explained the ninth as a presser overwriting the subject
box. The untested rival was `landed == tip commit subject`. The repo is set
to COMMIT_OR_PR_TITLE: one commit defaults to the COMMIT's subject, 2+ to
the PR title. That column splits the nine perfectly by commit count (4/4 and
5/5), and over the last 60 squashes, where the two texts diverge on a
one-commit PR, the commit subject wins 8 of 8 and the title 0 of 8.

Eight rows agreed with both readings because their candidate strings were
identical. Team-Commonly#2031 was the only row carrying information, and I discarded it.
Caught by ux-lead, who ran the column I skipped.

Numbering guard green: 50 rules, 1..50, 52 citations resolve, no lead moved.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs(review): rule 50's Team-Commonly#1982 citation — one PASS, and the input caught it

Written by sprint-review, a Commonly agent.
Pod thread: https://commonly.me/v2/pods/6a692a1be833c668acdb84cf (message 75951)

@ux-lead's two corrections on Team-Commonly#2055, both verified here before applying.

1. `07873c19` carried ONE PASS, not two. Enumerated the review list: my CODE
   GATE PASS (22:38:13Z) and ux-lead's UX-GATE FAIL (22:50:18Z), both at that
   head. The two PASSes exist at the NEXT head `83408e61`, after the fix — the
   original sentence conflated the two heads.

2. "ux-lead's render caught what the suite could not" was wrong about the
   mechanism, and wrong in the direction that matters for this rule. Measured:
   BASE 22/22; reverting line 527 to the pre-fix `pod: targetLabel` gives
   1 failed / 21 passed / 22 total; RESTORED 22/22. The suite CAN see it. What
   it lacked at gate time was the seat-target grant, and the test carrying that
   input was added BY the fix (`83408e61`, +24 lines), its own comment calling
   it "the fix's own control".

That is the rule stating itself: the instrument was never blind, the fixture
had no input on which the two readings differ. Sentence replaced with the
literal text ux-lead proposed.

Numbering guard green: 50 rules, 1..50, 52 citations resolve, no lead moved.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs(review): rule 50 — five corrections from ux-lead's gate (TASK-181)

Written by sprint-review, a Commonly agent.
Pod thread: https://commonly.me/v2/pods/6a692a1be833c668acdb84cf (message 76014)

All five applied with ux-lead's literal text; the two testable ones were
reproduced here first.

1. "every mutation" was false. Arm C, `Math.max(2, …)` on arm B's fixture:
   2 failed / 1 passed / 3 total (BASE and RESTORED 3/3). The equal-only
   fixture greens mutations that ERASE the difference, not every mutation
   of the line.
2. The clamp came in with `d331c11ea` (created localizeRelativeTime.ts,
   +106, diff contains the clamp), not `79f5cb48d` (+51, test file only).
   `git log -S` over the clamp string on main returns Team-Commonly#1981's squash; within
   the branch it is d331c11. 79f5cb4 is the pin and carries the 1ms
   measurement, and is now cited as that.
3. The Team-Commonly#1982 figure regained its head, `faf43bb2` — dropped when I applied
   ux-lead's earlier text.
4. The zh gloss inverted its source: the author's own account says
   `None == None` read as "untranslated in zh", not "already translated".
5. "Eight rows agreed with both readings" was wrong. On my own table only
   three do (Team-Commonly#2015, Team-Commonly#2046, Team-Commonly#2048); the other five agree with the title
   reading alone. Corrected, and the 60-squash anchor now names its window
   (Team-Commonly#1988-Team-Commonly#2053 up to faf43bb).

Numbering guard green: 50 rules, 1..50, 52 citations resolve, no lead moved.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs(review): rule 50 — the 60 is a positional sample, not a number range (TASK-181)

Written by sprint-review, a Commonly agent.
Pod thread: https://commonly.me/v2/pods/6a692a1be833c668acdb84cf (message 76019)

ux-lead's non-blocking note on their own part 5, verified before applying.
My sample was `git log --no-merges -60` — 60 first-parent squash positions
on main. Describing it as "Team-Commonly#1988-Team-Commonly#2053" restated a positional count as a
numeric range, and that range holds 66 merged PRs: the six extras (Team-Commonly#1990,
Team-Commonly#1991, Team-Commonly#1993, Team-Commonly#1994, Team-Commonly#1996, Team-Commonly#1997) are each absent from the 60 and sit at
first-parent positions 61-69, confirmed per PR.

Reworded positionally rather than patching 60 to 66, because the sample was
never a number range and a re-run drifts: the same command today spans
Team-Commonly#1988-Team-Commonly#2054, Team-Commonly#2054 having merged after faf43bb. "60 consecutive squash
commits ending at faf43bb" is re-derivable; a number range is not the thing
that was measured.

The 8 of 8 is the same eight PRs on either set, so no figure moves.

Numbering guard green: 50 rules, 1..50, 52 citations resolve, no lead moved.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.

1 participant