Skip to content

fix(live): restore the Multibyte only toggle before init awaits - #88

Merged
adminopenclaw8-sketch merged 5 commits into
masterfrom
codex/fix-live-multibyte-reload-race
Sep 24, 2026
Merged

adminopenclaw8-sketch merged 5 commits into
masterfrom
codex/fix-live-multibyte-reload-race

Conversation

@adminopenclaw8-sketch

@adminopenclaw8-sketch adminopenclaw8-sketch commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

This fixes a race on the Live page's Multibyte only toggle. It was a real bug users could hit, not just a flaky test: while Live was starting up, the box showed the wrong state, and a click made then was lost.

test-live-multibyte-only-e2e.js caught it by chance. CI run 35874411524 failed in its reload step and passed on one re-run.

The race

public/live.js on master does this. Line numbers are from 4f59b993; live.js is unchanged on the current master 08716f43:

  1. At script load (line 54), multibyteOnly is read from localStorage['live-multibyte-only']. Feed filtering is therefore correct from the start.
  2. init() renders the controls with app.innerHTML. #liveMultibyteToggle has no checked attribute, so it renders OFF.
  3. init() then awaits fetch('/api/config/map') (line 1283) and loadNodes() (line 1570).
  4. Only after both awaits (line 1639) did it set multibyteToggle.checked = multibyteOnly and attach the change listener.

Between steps 2 and 4 the controls panel is open, and the box is visible and clickable. Holding /api/config/map with Playwright request interception reproduces this deterministically on master:

While init is held Master This branch
Saved ON, what the box shows OFF, while filtering is already ON ON
Unsaved, the user clicks the box box turns ON, nothing saved (stored=null) ON, saved true at once
After init finishes click reverted to OFF ON, still saved

Without the hold, the window was about 30–35 ms locally. It grows with network latency and with the node count behind loadNodes(). CI serves Istanbul-instrumented JS, which makes it slower still, and that is where the test failed.

Reload and SPA hash navigation both go through registerPage('live').init → init(app), so both were affected.

Fix

Production: in public/live.js, the existing 6-line restore-and-listener block now runs right after app.innerHTML, before the first await. The code itself is unchanged, and so are the storage key, the default and the filter behaviour.

Every mount renders a new element, so the listener is still attached exactly once. rebuildFeedList() was already callable during init through theme-refresh. It returns early without #liveFeed and touches neither the map nor nodes.

Test: test-live-multibyte-only-e2e.js keeps its place in the CI sequence. There is no new registration.

Why the new test is deterministic

  • It holds /api/config/map, the first await in Live init, using page.route(..., { times: 1 }). That freezes init right after the controls are rendered, and all toggle checks run inside that window.
  • Afterwards it waits until Live has subscribed to packets, i.e. window._liveWSHandler() returns a handler. connectWS() sets it after loadNodes(), in the same synchronous run of init as the old toggle wiring. destroy() clears it, so it marks a finished init on every load and SPA mount.
  • The test asserts there is no handler while init is held, so the post-init checks can't run too early.
  • Timeouts are safety nets only, each with its own message. There are no sleeps and no retries.

Coverage:

  • the original steps: default OFF, and the ON/OFF packet filter;
  • saved true, false and unset, each shown correctly while init is held (saved true cannot pass on the OFF default);
  • a click during init is saved at once, with exactly one write, and init does not revert it;
  • the feed itself: in each step below, a 1-byte and a 2-byte path-hash packet are pushed through _liveBufferPacket both while init is held and after it, and the test checks which one renders (see "Feed-filter assertions" below);
  • a real page.reload();
  • 3 SPA round trips to #/packets and back, with the setting kept and exactly one write per click (no piled-up listeners);
  • no page errors, same-origin console errors or unhandled rejections. External map-tile errors are ignored, because the tile CDN is outside this app.

Evidence

All results are fresh from this branch on macOS arm64 with Playwright 1.58.2 and Chromium 1208. The server is the local Go server with the CI fixture (freshen → CI seed SQL → migrate). No staging, demo or production system was contacted.

Target test

  • Plain: 20/20, then 10/10 after the first review fixes and 5/5 after the second, plus 10/10 after the feed-filter assertions (b8c1d719).
  • Istanbul-instrumented build (as CI serves it): 10/10, then 10/10 after the first review fixes and 5/5 after the second, plus 5/5 after the feed-filter assertions.
  • Every run holds init at least 7 times, so every run is a controlled slow-init run.
  • Separate scratch repro, 5 runs plain and 5 instrumented: while held, the box shows the saved ON and a click is saved and kept.

Red on master: the new test fails with master's live.js on "a click during init must save the setting at once (stored=false)" and "toggle must show ON while init is still running, got OFF".

Mutations (each fails 3/3 with the intended message; M1–M7 were re-run after each review round):

Mutant Result
M1: master live.js (fix reverted) fails: click not saved; saved ON shown as OFF during init
M2: _liveWSHandler always null fails: "Live init did not finish …"
M3: restore early, listener late fails: click during init not saved
M4: listener early, restore late fails: saved ON shown as OFF during init
M5: listener attached twice fails: writes=2
M6: fix reverted + template default checked fails: saved false / unset shown as ON (not passing on a lucky default)
M7: init no longer fetches /api/config/map fails: "… the init hold point this test relies on has moved"
M10: same-origin console.error during init fails the error gate
Probe: every external image (tiles) answered 503 passes 3/3 (19 ignored "Failed to load resource")

Other checks

  • node --check passes on both changed files, and git diff --check is clean.
  • CI's "Lint CSS variables" step and all 72 "Run JS unit tests" commands pass. The steps were extracted mechanically from deploy.yml.
  • scripts/check-xss-sinks.sh --diff origin/master: clean.
  • eslint public/*.js: 0 errors. live.js has 13 warnings on both master and this branch.

CI-like Playwright sequence: all 109 commands of "Run Playwright E2E tests" and the slide-over gate, in CI order, against the instrumented build.

  • All 13 Live-related E2E tests pass.
  • The other failures were local-environment issues. A fixture freshened about an hour earlier had left the packets page's 15-minute window, and three tests hardcode /usr/bin/chromium. With a fresh fixture and CHROMIUM_PATH set, they pass on both master and this branch.
  • test-reach-rank-e2e.js was flaky on master at the time: 2/8 failures on master, 0/8 on this branch. That flake is now fixed on master by test(reach-rank): wait for the remounted board before typing; prove Back restores the search #90, and this branch includes it (see "Synced with master").

Browser check: Chromium with real clicks, at 1400×900 desktop and 375×812 mobile (touch, mobile UA). Covered: the default, a click, a reload with a frame taken mid-init (the box already shows the saved ON), SPA away and back, and console errors (none).

Feed-filter assertions (merge-review finding, b8c1d719)

A later merge review found that the test proved the checkbox, localStorage and readiness, but not that the setting filters the feed. Two realistic regressions passed:

  • rA: the internal filter no longer follows the saved value, but the box still shows it. The UI says ON while the feed does not filter.
  • rD: a click during init is saved, but the filter is applied only after init.

The commit is test-only; public/live.js is byte-identical to a786621e.

How the probe works:

  • probeFeed() buffers one packet with a 1-byte and one with a 2-byte path hash through window._liveBufferPacket, then reads the rendered feed DOM in the same evaluate. In LIVE mode, bufferPacket → renderPacketTree → addFeedItem is synchronous, so nothing waits and nothing retries.
  • assertFeedFilter() requires three things:
    • the 2-byte packet renders, which proves the feed was live;
    • the 1-byte packet is hidden or shown exactly as the setting says;
    • MC_packetHashSize reports sizes 1 and 2.
  • Probe hashes are unique per call, so earlier DOM cannot match.
  • A failure reads e.g. "setting is ON but the feed filter behaves as OFF: the single-byte packet was shown (click during init, init still held)".

Where it probes, both while init is held and after it:

  • a click during init;
  • a reload with saved ON;
  • fresh loads with the setting saved true, false and unset;
  • each SPA round trip, plus the OFF click that follows.

Probe header byte: it changes from 0x10 to 0x15.

  • 0x10 is route 00 (transport flood) with payload 4 (ADVERT).
  • 0x15 is route 01 (FLOOD) with payload 5 (GRP_TXT), which matches makePkt's route_type: 1 and GRP_TXT (firmware/docs/packet_format.md).
  • Path-hash sizes are unchanged: …00 gives 1 and …40 gives 2.

Mutations (isolated copies of public/, each run 2×):

Mutant Result
rA red on the feed assertion (reload saved ON, SPA round trip 1, fresh saved true), all while init is held
rD1: saved during init, applied only once Live is ready red: "click during init, init still held"
rD2: saved during init, filter applied after loadNodes() red: same assertion, so late application is caught
rS: destroy() drops the filter state, and the box is set from storage red: "SPA round trip 1, init still held" (added after the fresh review)
R1: master live.js red on the checkbox/storage assertions, as before
doubled listener still red
forged readiness still red
same-origin console.error still red
external tile images answered 503 still green, 2/2
PR code green

Fresh verification of b8c1d719 (macOS arm64, local Go server, CI fixture; no staging or production):

  • The target test passed 10/10 on plain files and 5/5 on the Istanbul-instrumented build.
  • Every run holds init at least 7 times.
  • The 14 other Live E2E tests pass.
  • Chromium browser check at 1400 px desktop and 375 px mobile passed: click, reload mid-init, SPA away and back, with no page, window or console errors and no unhandled rejections.
  • node --check, git diff --check, and the XSS diff gate are clean.
  • eslint reports 0 errors; live.js has 13 warnings on both this branch and master e51272d9.
  • Four local unit tests fail: test-frontend-helpers.js, test-issue-1485-live-anim-z.js, test-live-anims.js and test-panel-corner.js. They fail identically on a clean e51272d9 checkout and are not run in CI.

A fresh reviewer that did not write this change reviewed the test-only patch. Its first round flagged the missing SPA feed probe (rS) and the header byte; both are fixed. Its re-check found no blockers.

Independent review

A separate reviewer agent that did not write the change confirmed the race, the init ordering and the readiness signal. It built its own mutants and found no blockers in either round.

  • First round, addressed in bfbcd861:
    • tile-CDN console errors that could flake the new error gate;
    • swallowed waits;
    • steps depending on each other's state;
    • a missing finally.
  • Re-check, addressed in a786621e:
    • a carried-over re-check that could not fail, now removed and replaced by an honest comment;
    • a setup assertion that hid the SPA round-trip checks.
  • The reviewer also probed the origin filter. Same-origin /api/* failures and the app's own console.error calls are still counted; only external CDN resource errors are dropped.

Synced with master (75f90845)

The branch was brought up to date with a regular merge commit, 75f90845. Its parents are the previous head b8c1d719 and master 08716f43; there was no rebase, squash or force. The merge was conflict-free, and its tree df269886 equals the synthetic merge tree that was verified before the sync.

Master now includes:

None of them touch public/live.js, test-live-multibyte-only-e2e.js or the Live init, toggle, feed or WebSocket code. The diff against master is still exactly those two files, with this PR's blobs, and the multibyte test is registered once in deploy.yml.

Fresh verification on the merge tree (local Go server, CI fixture; no staging or production):

  • Multibyte test: 10/10 plain and 5/5 instrumented. Every run holds init, so every run is a controlled slow-init run.

  • Mutants, each run 2× and failing on its intended assertion:

    • rA, rD1, rD2 and rS fail on "setting is ON but the feed filter behaves as OFF";
    • the master placement fails on the checkbox and storage checks;
    • a doubled listener fails on writes=2;
    • forged readiness fails on the readiness self-check;
    • a same-origin console.error fails on the error gate.

    The external tile-503 probe stays green, 2/2.

  • Live E2E: all 14 other tests pass.

  • Reach Rank (test(reach-rank): wait for the remounted board before typing; prove Back restores the search #90): test-reach-rank-e2e.js passed 10/10, with the Back/Forward search step green every time. CI positions 91–97 passed 3/3 per test.

  • Static and workflow checks:

    • CI's unit-test block (73 tests) passes, and the Go fork-guard/workflow tests pass (5 tests).
    • All six workflows parse as YAML.
    • node --check, eslint 8 (13 live.js warnings, as on master), the XSS diff gate and git diff --check are all clean.
  • Browser check: Chromium at 1400 px desktop and 375 px mobile passed: real click, reload with init held, and SPA away and back, with no page, window or console errors and no unhandled rejections.

Independent delta review: a fresh reviewer that did not write this PR or the sync found no blockers.

Known limitations

Files

  • public/live.js: moves the restore and listener block (+13/−8)
  • test-live-multibyte-only-e2e.js: deterministic persistence, race and feed-filter coverage

🤖 Generated with Claude Code

Openclaw and others added 4 commits September 23, 2026 18:03
Live init renders its controls, then awaits /api/config/map and
loadNodes(). The "Multibyte only" box got its saved state and change
listener only after both awaits. The feed, though, already filtered on
the saved value from script load.

While init ran, the box was visible and clickable but showed OFF even
when saved ON. A click in that window was neither saved nor applied, and
init reset the box when it caught up. The reload check in
test-live-multibyte-only-e2e.js read the box as soon as it existed, so it
failed whenever init was slow (CI serves instrumented JS).

The fix moves the existing restore and listener to straight after the
controls are rendered, before the first await. Nothing else changes:
storage key, default and filter behaviour are the same. Each mount
renders a new element, so the listener is still attached once.

The test now holds /api/config/map to freeze init at its first await,
and checks the box while init is held. It covers the saved true, false
and unset states, a click during init, a real reload, and three SPA
round trips with a single write per click. It then waits for Live's
packet subscription (window._liveWSHandler()) and fails on page errors,
console errors or unhandled rejections. It uses no sleeps or retries.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…pendent

Fixes for the independent review's findings:

- Only console errors from the app's own origin count. External map
  tile failures (19 "Failed to load resource" errors when the tile CDN
  answers 503) made the error gate flaky in CI. Page errors, unhandled
  rejections and same-origin console errors still fail the test.
- The packet feed waits now throw a clear message instead of being
  swallowed.
- Each persistence step sets the saved value it needs, so one failure
  no longer cascades into later setup failures.
- The reload step re-checks that the click from the previous step
  survived after that step's init settled. The comment notes that a
  revert deferred by an arbitrary timer is out of reach for a
  deterministic test.
- Fresh-context steps close their context in a finally block.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…on setup

Fixes for the re-review's two low findings:

- Removed the carried-over re-check in the reload step. It ran with no
  gap after the previous step's own read, so it could not catch
  anything. The click step now says plainly that "kept" means kept
  after init finished, and that a timer-deferred revert is out of reach
  for a deterministic test.
- The SPA step's setup reload now waits for Live init without asserting
  mid-init. Its round-trip and single-listener checks therefore always
  run. With master's live.js the step now fails on the round-trip
  check itself.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review found that the test checked only the checkbox and localStorage.
Two realistic regressions passed:
- rA: the internal filter no longer follows the saved value, while the
  box still shows it.
- rD: a click during init is saved, but the filter is applied only
  after init.

probeFeed() buffers one 1-byte and one 2-byte path-hash packet through
window._liveBufferPacket and reads the rendered feed in the same
evaluate. In LIVE mode, bufferPacket -> renderPacketTree -> addFeedItem
is synchronous, so nothing waits.

assertFeedFilter() requires three things:
- the 2-byte packet renders, so the feed was live;
- the 1-byte packet is hidden or shown exactly as the setting says;
- both packets have the expected hash sizes.

Each of these steps now probes the feed while init is held and after it
finishes: the click during init, the reload with saved ON, the three
fresh loads, and the SPA round trips plus the OFF click that follows.

The probe header byte changes from 0x10 (transport flood, ADVERT) to
0x15 (FLOOD, GRP_TXT) to match route_type 1 and payloadTypeName.

rA, both rD variants and an SPA state-loss mutant now fail on
"setting is ON but the feed filter behaves as OFF". public/live.js is
unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings in #86 (Relay Airtime Share), #87 (blacklist QA hardening) and #90
(Reach Rank test stabilisation). None of them touch public/live.js or
test-live-multibyte-only-e2e.js. The merge was conflict-free, and its tree
equals the verified synthetic merge tree.

Co-Authored-By: Claude Opus 5.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