test(#1356): assert the rendered label, not where identifiers sit in map.js

[TeTeHacko]

Follow-up to the review on #1912, where this assertion cost a round trip. Independent of that PR — this branch is off current master and touches no code path it changes.

The problem

#1356 V3.e, V3.f and V3.g all describe what makeRepeaterLabelIcon produces, but all three assert it by grepping public/map.js. V3.e also bounds the distance between two identifiers:

assert(/MB_GLYPHS\[[^\]]+\][\s\S]{0,200}shortHash|shortHash[\s\S]{0,200}MB_GLYPHS\[/.test(mapSrc),
  'makeRepeaterLabelIcon prepends MB_GLYPHS glyph to the hash text');

Three separate failure modes, all observed:

1. It fails on edits that change nothing. #1912 inserts one variable declaration in that function; the markup is byte-identical and the build went red.

2. It cannot tell code from prose about code. My first attempt at fixing #1912 added a comment explaining the constraint — and the comment mentioned both identifiers, so it satisfied the grep by itself. With that comment present I moved the hash assignment away from the glyph, reintroducing the exact defect, and the test still reported green. A check that a comment can satisfy is worse than one that is merely brittle.

3. It does not assert the thing it is named after. On master the match is not the declaration order at all. It is MB_GLYPHS[...] reaching the later shortHash inside ariaStatus, 212 characters downstream. Whether the glyph is actually prepended to the hash is incidental to whether this passes.

That third point also corrects something I said on #1912, and it corrects it against myself: both the 312 you quoted and the 299 I "corrected" it to are the distance between the two declarations, which is not the distance the regex uses. Measuring the one it does use:

tree	MB_GLYPHS[ → next shortHash	assertion
master	212	pass
#1912 before the fix	277	fail
moving unknownWidth below the glyph	343	fail
moving shortHash below the glyph	54	pass

So moving unknownWidth down does not merely fall short — it makes the gap worse, because it lands between the glyph and ariaStatus. My earlier "233, still 33 over" was the wrong metric on the wrong pair. Apologies; the conclusion happened to hold but the reasoning did not.

What this does

Loads map.js in the same DOM-less vm sandbox test-map-clustering.js already uses, exposes makeRepeaterLabelIcon through the existing window.__meshcoreMapInternals hook, and asserts the emitted markup:

• glyph, U+2009 thin space, hash — in that order and adjacent;
• no glyph and no thin space when there is no multi-byte status;
• aria-label exactly multi-byte <status>, hash <ID>, and repeater hash <ID> without one;
• the visible span carries aria-hidden.

No browser, so it stays in the JS-unit-tests step rather than moving to Playwright.

Mutation-tested, not eyeballed

mutation	old V3.e/f/g	new
glyph moved after the hash	all silent	caught
plain space instead of U+2009	all silent	caught
span loses aria-hidden	V3.g caught	caught
aria-label loses its comma	all silent	caught
200 chars inserted between the identifiers (no behaviour change)	V3.e fails	passes

Full JS unit list from .github/workflows/deploy.yml: 65/65.

Notes for review

• V3.a–V3.d (MB_GLYPHS definitions, CSS variables, the border rule) are left as source/CSS greps. The glyph values are now covered implicitly by the rendered-output assertions, but converting the CSS ones needs a different approach and did not belong here.
• The sandbox loader has no try/catch that warns and continues. If map.js stops loading, the suite must fail rather than silently skip every assertion below it.
• If this lands, the ordering comment in fix(map): render an unobserved hash size as unknown, not as 1 byte #1912 becomes obsolete and I will drop it there. I deliberately did not touch it from this branch so the two do not conflict textually.

[efiten]

Reviewing this properly shortly; flagging the CI result first so you are not left guessing.

It failed on something unrelated to what this PR changes:

✗ setting persists across reload: multibyte toggle should restore checked=true from localStorage

That is a customizer persistence check, not one of the three a11y assertions you are replacing. Two things worth knowing before you go looking:

1. This repo has a documented flake family around focus and reload timing in headless Chromium (bug(slide-over): focus-restore to originating row is CI-flaky on X-click and resize-cleanup paths #1172, hard-fix: slide-over close path must not transition through focused-but-hidden state (supersedes #1172) #1616), and a localStorage-restore-after-reload assertion is the shape those take. feat(packets): add filter to All Observer dropdown #1884 failed on a swatch-focus test this morning and passed on the rerun with no change.
2. Master itself was red for part of this morning from an unrelated two-PR interaction (fix(#1784): ship pathTrust default 1, not 2 #1929 plus feat(#1784): gate ingestor neighbor-edge creation on the path-trust threshold (rebase of #1863) #1930, fixed in fix: unbreak master — decouple the pathTrust builder test from the default #1932), so runs started in that window tested a broken base. Yours started at 09:31, which is around when the fix landed, so it may or may not have picked it up.

Worth one rerun before treating it as real. If it fails the same way twice, it is worth a look and I will help chase it.

On the PR itself: replacing a source grep with a check on what the function renders is the right direction, and the mutation table showing what each version catches is exactly the evidence I would want. Full review to follow.

[efiten]

Recycling both of your PRs, and it is worth saying why, because it is a trap that has cost this queue a lot of time today.

Both of your runs are attempt=2, so they were re-run rather than recycled. A re-run does not help here. GitHub re-runs against the same commit the original run used, which for a pull_request event is the merge commit as it stood when that run was first created. It never sees a newer master.

That fully explains #1912. Its re-run at 11:44 still tests a base from before #1932, which is the fix for the master breakage I caused this morning by merging #1929 and #1930 together. So it keeps failing on TestNeighborEdgesBuilderPathTrustExcludesOneByte, which has nothing to do with your change and is already fixed on master.

For #1933 it is less clear-cut. Its run started at 09:31, which is the same minute #1932 merged, so its base may or may not have included the fix. The failure there was setting persists across reload: multibyte toggle should restore checked=true from localStorage, in test-live-multibyte-only-e2e.js.

I said I would look properly if it failed twice rather than send you chasing it, so: I read your public/map.js change and it cannot cause that failure. It adds makeRepeaterLabelIcon to the existing window.__meshcoreMapInternals object and reformats the literal across lines. Nothing in it touches the live view, the multibyte toggle or localStorage. Since it has only genuinely run once, a real fresh run is the right next step before anyone goes digging.

Closing and reopening both, which forces a new merge ref against current master. Nothing needed from you.

[efiten]

Settled, and in your favour: your PR was not the cause.

I re-ran the master pipeline for 859173f1 a few minutes ago, with none of your code in it. It failed on:

✗ setting persists across reload: multibyte toggle should restore checked=true from localStorage

The same assertion that failed on your PR, on plain master. That is as clean a control as we are going to get.

I said earlier that I had read your public/map.js change and could not see how adding makeRepeaterLabelIcon to __meshcoreMapInternals could touch a live-view localStorage toggle. This confirms it.

The cause is in the test: it waits for the toggle element to exist and then reads .checked, but the element existing does not mean the code that restores it from localStorage has run yet. Fixed in #1939 along with a second step of the same shape in the colour-picker suite.

Your #1933 stands on its own merits and I still like it: replacing a source grep with a check on what the function renders is the right direction, and it is what would have spared us both the 312-versus-277-character detour this morning.