Skip to content

fix: P3 follow-ups from the #203/#204/#206 reviews (#208) - #214

Merged
dborup merged 15 commits into
masterfrom
codex/issue-208-review-followups
Oct 4, 2026
Merged

dborup merged 15 commits into
masterfrom
codex/issue-208-review-followups

Conversation

@dborup-agent

@dborup-agent dborup-agent commented Oct 4, 2026 •

Copy link
Copy Markdown
Collaborator

Relates to #208

P3 follow-ups from the reviews of #203, #204 and #206. Each item is fixed with a test that fails before and passes after (or pins a test gap with a mutant that survived before), except the Hash Stats column sort in item 6, which stays local for the reason below. One test commit followed by one fix commit per item; items 2 and 4 are test-only gaps.

Items

# From Outcome Change Test (red before → green after) Mutants
1 #203 Fixed writeNodeNotFound logs a failed lookupMissingNode: the first at once, then at most once per 10 min with the count suppressed in between (log-first-then-interval, as in schema_wait.go). A client-cancelled request is not logged; the requested key is not in the line. Still the bare 404. TestNodeDetail404LookupErrorIsLoggedOnce (schema drift: inactive_nodes without role; 3 requests → 1 line), TestNodeDetail404CancelledLookupIsNotLogged, TestMissingNodeLookupLogThrottle M1a no log, M1b no throttle, M1c no cancel guard, M1d suppressed count never reset, M1e < → <=: all killed
2 #203 Test gap closed None in code: the unit test and the E2E now read the card's <dt>/<dd> rows and assert that the Last advert row shows the inactive last_seen. test-issue-199-missing-node.js, test-issue-199-inactive-observer-e2e.js M8 (row dropped) survived before; killed now by unit and E2E
3 #203 Fixed "No advert heard since <date>; this device is inactive." → "…; this node is listed as inactive." The observer's current status is already on the card as the Last upload as observer row. unit + E2E assert the new wording and the absence of "this device is inactive" M3 (old wording back): killed by unit and E2E
4 #204 Test gap closed None in code: a unit case focuses the modal's close button, and the E2E focuses #packetPathClose and #packetPathCopyLink, then presses Escape. test-packet-path-map.js, test-issue-180-packets-url-modal-e2e.js M4 (overlay.contains(el) removed) survived before; killed now by unit and E2E
5 #206 Fixed _writeViewParams also records the resolved values in the entry's history.state (analyticsView, other keys kept). Without a URL value, restoreViewParams uses that record before sessionStorage. URLs are unchanged. test-analytics-subtab-deeplinks-205.js (Back/Forward modelled in the vm, Scopes and Wardriving), E2E with real goBack/goForward in Chromium M5a record ignored, M5b record not written, M5c written only on a URL change, M5d other state keys dropped, M5e record not updated: all killed
6 #206 Filter fixed; sort reasoned The multi-byte adopters filter is deep-linked as mbf= (all/confirmed/suspected/unknown), URL only. The column sort stays local (reason below). unit: URL → active button and rows, hostile values → All, default URL untouched, nothing stored, tab switch drops it. E2E: click writes, reload keeps, All drops, tab switch drops, hostile value → All M6a click does not write the URL (E2E), M6b key not in TAB_URL_PARAMS, M6c URL ignored, M6d raw URL value used unvalidated: all killed
7 #206 Fixed (section); bytes reasoned TAB_URL_PARAMS.collisions = ['section'], and the comment now says why bytes is not listed (see below) unit: leaving Hash Issues with bytes=2&section=… gives #/analytics?tab=topology&bytes=2&window=24h; a Hash Issues → Hash Stats → Hash Issues round-trip keeps bytes=2 M7a (section not listed), M7b (bytes listed again): both killed by unit; M7b also by the Kpa-clawbot#1914 E2E

Item 5: how history entries are told apart

A default view leaves its key out of the URL, so the URL alone cannot tell "this entry was the default" from "a plain visit of the tab". The entry's history.state can:

  • A rendered entry carries the record, so Back/Forward to it restores its own view, including a default.
  • A new entry (a link, location.hash = …) has no state, so it still opens the stored value, as analytics: deep-link inner views (Scopes sub-tabs + window, audit other tabs' local view state) #205 intends.
  • A tab switch (_updateAnalyticsUrl) already writes a null state, so clicking a tab back within the same entry still brings the stored view back.
  • Only values that are strings are read from the record, and they go through resolveViewParam: a value is compared with === against the allowed list, and an unknown one gives the default. A garbled or foreign state never reaches the view.
  • replaceState runs only when the URL or the record changes, so re-renders do not add calls (Safari throttles them).

Item 7: why bytes= stays across a tab switch

My first fix (249351f1) listed both keys, and CI's test-issue-1306-collisions-terminology-e2e.js failed: step "Kpa-clawbot#1914: time-window, tab and theme refreshes retain the chosen byte size" pins that Hash Issues → Hash Stats → Hash Issues keeps bytes=2. Hash Issues has no stored fallback for the byte size, so the URL is where it is remembered across a tab switch. 0f77c974 narrows the fix:

Item 6: why the column sort stays local

  • The sort is a one-shot DOM reorder. It has no state to link: no column or direction variable, no indicator, ascending only, and the next filter click discards it. Deep-linking it would mean building sort state first, which is new behaviour beyond this item.
  • It also has a pre-existing defect. Its colIdx map is off by one against the six columns: Role is missing, Status sorts the Role cells, Hash Size sorts the Status cells, Adverts sorts the Hash Size cells, and Last Seen sorts the Adverts cells. Linking it now would put a broken order into shared links. Listed under follow-ups below.

The filter follows the #194/#205 pattern: renderHashSizes reads mbf= through restoreViewParams on every render, and a click writes it with setViewParam. The value is never put into a selector or markup, an unknown value gives All, and All keeps the URL as it was. The filter is not stored in sessionStorage, because it had no stored state before; a spec without a storageKey now skips storage, so a plain visit still opens on All.

Performance

  • Item 1 runs only on the 404 error path: one mutex and one time.Now(). There are no new queries.
  • Item 5 adds, per tab render or click, a copy of the small history.state object (a few keys). replaceState is only called on a change.
  • Item 6 resolves one parameter per Hash Stats render. Nothing touches a hot path (packet rendering, ingest, WS broadcast).

Invariants

  • cmd/server stays read-only: no new query and no write.
  • No new map[string]interface{}; the throttle is a named struct.
  • scripts/check-xss-sinks.sh --diff is clean.
  • No hardcoded colours are added. The touched Hash Stats button lines keep their existing var(--x, #fallback) values; only class changed.
  • No per-item API calls.
  • Workflows are untouched (fork guards: 9). The new E2E steps sit in files CI already runs.

Tests run locally

  • cd cmd/server && go test -timeout 20m ./...: ok (1172 s, run alone)
  • cd cmd/ingestor && go test -timeout 60m ./...: ok (883 s; untouched by this PR. Two earlier local runs hit the 10 and 20 min timeouts while sharing the CPU, with no failed test)
  • sh test-all.sh: 214 files passed, 0 failed
  • node test-frontend-helpers.js: 707 passed, 0 failed
  • E2E against a local Go server on test-fixtures/e2e-fixture.db, prepared as in CI (freshen, inline seed, corescope-migrate, seed-2073, seed-199):
    • test-issue-199-inactive-observer-e2e.js: 3/3
    • test-issue-180-packets-url-modal-e2e.js: 12/12
    • test-issue-205-analytics-subtab-deeplinks-e2e.js: 15/15, three runs
    • Against the pre-fix analytics.js, the two new Back/Forward steps fail.
    • test-issue-1306-collisions-terminology-e2e.js: 23/23 after 0f77c974 (it failed on 249351f1 locally and in CI)
    • The whole CI E2E step, run locally (127 files, minus test-channel-proposals-e2e.js, which starts its own server and ingestor): 122 passed. The 5 failures were "no packets row" and timeouts from a fixture freshened two hours earlier. With a re-freshened fixture all 5 pass, on this branch and on origin/master.

Follow-ups (not in this PR)

  • Hash Stats adopters sort: the colIdx off-by-one above, and its lack of state. Fixing it is a behaviour change, so it should get its own issue.

🤖 Generated with Claude Code

dborup and others added 15 commits October 4, 2026 08:20
A failed lookupMissingNode (schema drift on inactive_nodes) answers the
bare 404 without a trace in the log. Pin that it is logged exactly once
for repeated requests, carries the error but not the requested key, and
that a request cancelled by its client is not logged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
writeNodeNotFound still answers the bare 404 when lookupMissingNode
fails, but now logs the error: the first failure at once, later ones at
most every 10 minutes with the number suppressed in between (the
log-first-then-interval shape of schema_wait.go). A request its client
cancelled is not logged, and the requested key stays out of the line.
Read-only: no new query, no new write.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The last advert date also appears in the explanation sentence, so a
card without the "Last advert" row (mutant M8) passed both the unit test
and the E2E. Both now read the card's <dt>/<dd> rows and assert the
"Last advert" row shows the inactive row's last_seen.

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

An observer can be uploading right now while its node row is still in
inactive_nodes (retired before #203, or while the observer was quiet),
so "this device is inactive" can contradict the "Last upload as
observer" row next to it. Pin the wording to the record instead.

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

"No advert heard since <date>; this device is inactive" becomes "...;
this node is listed as inactive", which stays true while the observer
of the same key is uploading. The observer's current status is already
on the card as the "Last upload as observer" row.

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

No test focused an element inside the modal, so dropping the
overlay.contains(el) check in focusInLayerAbove (mutant M4) survived:
the position:fixed .modal-overlay is then taken for a layer drawn over
the modal and Escape on its close button does nothing. Add a unit case
and an E2E step for the close and copy-link buttons.

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

TAB_URL_PARAMS is "the hash keys each tab owns", but Hash Issues'
bytes= and section= are not in it, so they ride along to the next tab:
#/analytics?tab=topology&bytes=2&section=hashMatrixSection.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#208)

A tab switch away from Hash Issues now drops its byte-size selector
(bytes=) and section link (section=) keys, like the other tabs' keys.
Hash Issues itself is unchanged: it reads and writes both as before.

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

The #206 audit missed the All / Confirmed / Suspected / Unknown filter
of the Multi-Byte Hash Adopters card. Pin the #194/#205 pattern for it
as mbf=: a URL value selects and filters, an unknown or hostile value
is All and is dropped from the URL, All keeps the URL as it is, nothing
goes to sessionStorage, and leaving the tab drops the key.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…bf= (#208)

The All / Confirmed / Suspected / Unknown filter of the Multi-Byte Hash
Adopters card now follows the #194/#205 pattern: renderHashSizes reads
mbf= on every render through restoreViewParams, a click writes it with
setViewParam, a value is only compared with === against the allowed
list (never put in a selector or markup), an unknown one is All, and
All keeps the URL as it was. Leaving the tab drops mbf=.

URL only: a spec without a storageKey now skips sessionStorage, so a
plain visit still opens on All as before.

The column sort of the same table stays local (see the PR): it is a
one-shot DOM reorder with no state to link.

The test now derives each fixture adopter's status the way the card
does (capability row by pubkey, else unknown) instead of assuming it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
In Chromium against the fixture server: a cold load of
?tab=hashsizes&mbf=confirmed selects Confirmed; a click writes mbf=, a
reload keeps it, All drops it and nothing is stored; a tab switch drops
it; a hostile value is All without a page error. The click path is not
reachable from the vm unit test (the card wires its handler in a
setTimeout), so this is what kills a click that does not write the URL.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…iew (#208)

A default view leaves its key out of the URL and a missing key falls
back to sessionStorage, so Back to #/analytics?tab=scopes after a later
entry stored sub=regions opens Regions, not that entry's Overview (and
the same for Wardriving's window). The vm's fake history now keeps a
state, and new entries and Back/Forward are modelled. Also pinned: a
new entry without the key still opens the stored value, a tab switch
inside an entry still brings it back, a garbled entry state never
reaches the view, and other history.state keys are kept.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…one (#208)

restoreViewParams leaves a default out of the URL and fell back to
sessionStorage for a missing key, so Back to #/analytics?tab=scopes
after a later entry stored sub=regions opened Regions and rewrote that
entry's URL. _writeViewParams now also records the resolved values in
the entry's history.state (analyticsView, other state keys kept), and
restoreViewParams uses that record before sessionStorage when the URL
has no value. A new entry has no record, so a plain visit still opens
the stored view; a tab switch writes a null state, so clicking a tab
back in the same entry also still does. URLs are unchanged, and
replaceState only runs when the URL or the record changes.

E2E: real Back/Forward in Chromium for Scopes and Wardriving; Regions
has no window buttons, and expectScopes now waits for a given window.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…1914), drops section= (#208)

CI's Kpa-clawbot#1914 E2E (test-issue-1306-collisions-terminology-e2e.js) pins
that Hash Issues -> Hash Stats -> Hash Issues keeps the chosen byte
size: bytes= has no stored fallback, so the URL is where the tab
remembers it. Dropping it on a tab switch (249351f) broke that. Only
section=, a one-shot scroll anchor, belongs to the tab's dropped keys.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Narrows 249351f: TAB_URL_PARAMS.collisions is ['section']. bytes= stays
in the URL across a tab switch, as Kpa-clawbot#1914 intends (no stored fallback),
and the comment now says why it is not listed, which was the mismatch
the #208 review flagged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup-agent

Copy link
Copy Markdown
Collaborator Author

Rapport — CS-pve-agent3 PR#214 #208 — head 0f77c97

Status: All 7 items are handled. Items 1, 3 and 5 are fixed, items 2 and 4 close their test gaps, item 6 deep-links the filter and keeps the sort local with a recorded reason, and item 7 drops section= but keeps bytes= per Kpa-clawbot#1914. CI is green on 0f77c974. The PR stays a draft.

Evidence tags: [T] test or run output, [K] checked in code or diff, [A] assessment or inference.

Items

# Outcome Commits (test → fix) Test Mutants Evidence
1 Fixed: a failed lookupMissingNode is logged, the first at once, then at most every 10 min with a suppressed count. A cancelled request is not logged, and the key is not in the line. Still a bare 404. be868cff → d1cfe9d1 TestNodeDetail404LookupErrorIsLoggedOnce (schema drift, 3 requests → 1 line), TestNodeDetail404CancelledLookupIsNotLogged, TestMissingNodeLookupLogThrottle M1a–M1e killed [T] red before (0 lines logged), green after; [K] named struct, no new query
2 Test gap closed: the Last advert <dt>/<dd> row is asserted in the unit test and the E2E 2b1b2916 test-issue-199-missing-node.js, test-issue-199-inactive-observer-e2e.js M8 survived before; killed now by unit and E2E [T]
3 Fixed: "…; this node is listed as inactive." The observer's current status is the existing Last upload as observer row. fa639baf → ee7a9032 unit + E2E wording M3 killed by unit and E2E [T]; [T] screenshot of the card checked
4 Test gap closed: Escape with focus on #packetPathClose / #packetPathCopyLink closes the modal 0c0cadf0 test-packet-path-map.js (unit), test-issue-180-packets-url-modal-e2e.js (2 steps) M4 survived before; killed now by unit and E2E [T]
5 Fixed: the entry's resolved view is recorded in history.state.analyticsView and used before sessionStorage when the URL has no key. A new entry and a tab switch keep the stored fallback. URLs are unchanged. d7b5e19d → 0022501b vm Back/Forward model (Scopes, Wardriving, garbled state, other keys kept); E2E real goBack/goForward M5a–M5e killed [T] the E2E steps fail on the pre-fix analytics.js and pass after
6 Filter deep-linked as mbf= (URL only). Sort kept local: a stateless one-shot DOM reorder whose colIdx is off by one against the six columns. 8eb4398a → 9deaa258, E2E b992cfaf unit (URL → button/rows, hostile → All, no storage, tab switch drops it); E2E (click, reload, All, tab switch, hostile) M6a (E2E), M6b–M6d (unit) killed [T]; [K] off-by-one in the sort map; [T] screenshot checked
7 section= is dropped on a tab switch; bytes= is kept, and the comment says why e322ea82 → 249351f1, then defbc59c → 0f77c974 unit: leaving the tab drops section and keeps bytes; a Hash Issues → Hash Stats → Hash Issues round-trip keeps bytes=2 M7a, M7b killed (unit); M7b also by the Kpa-clawbot#1914 E2E [T] see the note below

Note on item 7. The first fix (249351f1) also dropped bytes=, and CI's test-issue-1306-collisions-terminology-e2e.js failed on step "Kpa-clawbot#1914: time-window, tab and theme refreshes retain the chosen byte size" (run 37191941496). Hash Issues has no stored fallback for the byte size, so the URL is where Kpa-clawbot#1914 remembers it across a tab switch. 0f77c974 narrows the fix to section= only. [T] reproduced locally, then 23/23 after the change.

Tests

  • [T] cd cmd/server && go test -timeout 20m ./...: ok.
  • [T] cd cmd/ingestor && go test -timeout 60m ./...: ok (883 s). The package is untouched. Two earlier local runs hit the 10 and 20 min timeouts while sharing the CPU with other suites, with no failed test.
  • [T] sh test-all.sh: 214 files passed, 0 failed. node test-frontend-helpers.js: 707 passed, 0 failed.
  • [T] E2E against a local Go server on e2e-fixture.db, prepared as in CI (freshen, inline seed, corescope-migrate, seed-2073, seed-199). The server was stopped by its port-owner pid.
    • test-issue-199: 3/3. test-issue-180: 12/12. test-issue-205: 15/15, three runs. test-issue-1306: 23/23.
    • The whole CI E2E step was also run locally: 127 files, minus test-channel-proposals-e2e.js, which starts its own server and ingestor. 122 passed. The 5 failures (1188, 1062, touch-gestures, 1692, 1122) came from a fixture freshened two hours earlier: no packets in the default window. With a re-freshened fixture all 5 pass, on this branch and on origin/master.
  • [T] scripts/check-xss-sinks.sh --diff: clean (exit 0).
  • [K] No new map[string]interface{}, and cmd/server stays read-only. No new hardcoded colours: the touched Hash Stats lines keep their existing var(--x, #fallback). No per-item API calls.
  • [K] Workflows are untouched; the fork guard count is still 9.

CI (run 37194646048, head 0f77c974)

Job Result
Go Build & Test pass [T]
Playwright E2E Tests pass [T]
Build & Publish Docker Image pass [T]; a local image build only, no push
Release Artifacts / Deploy Staging / Publish Badges & Summary skipped (PR) [T]

The previous run on 0022501b (37191941496) failed on the Kpa-clawbot#1914 E2E step described above.

Remaining

  • [A] The Hash Stats adopters column sort has the colIdx off-by-one and no state. Fixing it is a behaviour change outside follow-ups from #203, #204 and #206 reviews (P3: test gaps, inactive-card wording, history defaults, Hash Stats audit) #208 and fits a separate issue.
  • [A] Item 5 relies on _updateAnalyticsUrl writing a null state on a tab switch, so a tab clicked back within the same entry keeps the stored fallback. This is pinned by the unit test "a tab switch inside an entry still brings back the stored view".
  • [A] The branch has 15 commits, test then fix per item, including the item-7 narrowing as follow-up commits (no amend or force-push).

@adminopenclaw8-sketch

Copy link
Copy Markdown
Collaborator

Review — CS-Minimax PR#214 review-followups — head 0f77c97

Dom: APPROVE med nits

Independent, read-only review of head 0f77c974 and of its merge into origin/master a0086bdd (merged tree 31208321, from git merge-tree --write-tree). Both trees, and master, were unpacked with git archive into scratch.

  • E2E runs used a local Go server built from the merged tree. It ran on e2e-fixture.db, prepared as in CI: freshen, the inline seed from deploy.yml, corescope-migrate, seed-2073, seed-199.
  • Each server was stopped by its port-owner pid.
  • Platform: go1.27.0 darwin/arm64, Playwright 1.58.2 Chromium.

Evidence tags: [T] run here, [A] analysis of the source, [K] taken from the author's report or CI, not re-run.

Findings

# Severity Finding Evidence
F1 follow-up, pre-existing, outside this PR The Hash Stats adopters column sort is broken, as the author says.
- colIdx = { name: 0, status: 1, hashSize: 2, packets: 3, lastSeen: 4 } has no entry for Role, so every later key reads the cell one column to its left. This dates from 45623672, where the Role column and this map were added together.
- In the browser on the fixture, clicking Adverts leaves the Adverts column as 2, 1, 1, …, and clicking Role does nothing.
- It deserves its own issue (proposed below). Keeping the sort local in this PR is right.
[T][A]
F2 nit, optional The card's headline is still Inactive node, a device-state claim, even when the Last upload as observer row a few lines below is minutes old. The explanation is now worded as a listing; a headline such as "Listed as inactive" would match it. [T]
F3 nit, cosmetic The first lookup-failure line always says (0 more suppressed since the last report; next report in 10m0s at the earliest). Leaving out the count when it is 0 would read better. [T]
F5 info, pre-existing, unrelated My one full -race run of cmd/server failed on TestIssue1008_HandlerReturns503WhileSubpathIndexLoading (status = 200, want 503), while E2E probes loaded the CPU.
  • The test checks the not-ready window right after Load(), which races the background index build on its 6-transmission fixture.
  • Under CPU load, a pre-compiled -race binary fails it 3 of 600 times on master and 2 of 600 on the merged tree; unloaded it is 0 of 100 on both.
  • The PR touches neither the store nor the index code. There is no issue for this flake in this repo yet; filing one is worth considering. | [T][A] |
    | F4 | info | Because bytes= is kept (item 7), it now shows in other tabs' URLs, for example #/analytics?tab=hashsizes&bytes=2. No other tab reads bytes, so this is harmless and matches Allow to link to Mesh Analytics with a path hash byte size set via url Kpa-clawbot/CoreScope#1914. | [T][A] |

Item by item

1. writeNodeNotFound lookup error (cmd/server/node_not_found.go)

  • Rate limit. The first failure is logged at once; later ones at most once per missingNodeLogEvery (10 min), with the count suppressed since the previous line. time.Now() keeps its monotonic reading, so a wall-clock step cannot stretch the window. [A]
  • Cancelled requests. A request whose context is already done (r.Context().Err() != nil) is not logged. The server has no timeout middleware that would put a deadline on r.Context(), so a slow but genuine failure cannot be silenced as "cancelled". [A]
  • Key not in the line. The pubkey is bound with ? and SQLite errors do not echo bound values. The test asserts that the key is absent. [T][A]
  • Still a 404, with the bare {"error":"Not found"} body. [T]
  • Goroutine safety and size.
    • The limiter is one mutex-guarded struct per Server (a time and an int), so its size is fixed. [A]
    • A probe that fires 64 concurrent failing lookups under -race, run 3 times, gave one log line and 64 × 404 each time: missing-node lookup failed, answering a bare 404: SQL logic error: no such column: role (1) (0 more suppressed …). [T]

2. Last advert row

  • The unit test and the E2E now read the <dt>/<dd> rows.
  • M8 (row dropped) passes master's test-issue-199-missing-node.js (8/8, survives). [T]
  • On this PR it fails both the unit test (no "Last advert" row; rows: Name, Role, Last upload as observer) and test-issue-199-inactive-observer-e2e.js (2 passed, 1 failed), so it is killed. [T]

3. Card wording

  • The rendered card for the seeded inactive observer reads "No advert heard since 2026-09-24 …; this node is listed as inactive." Its rows are Name, Role, Last advert and Last upload as observer: 2026-10-04 …. [T] (screenshot checked)
  • The explanation no longer contradicts the observer row. Only the headline still reads as a state (F2). [A]

4. Escape inside the View Path modal

  • M4 (overlay.contains(el) removed) passes master's test-packet-path-map.js (64/64, survives). [T]
  • On this PR it fails the unit test (Escape with focus inside the modal … modal still open) and both new E2E steps (#packetPathClose, #packetPathCopyLink; 10 passed, 2 failed), so it is killed. [T]

5. Back/forward in Analytics

  • Real Back/Forward, multi-tab probe. A Playwright probe, not committed, built six history entries:

    • E1: Scopes, then a click to Hop depth;
    • E2: Wardriving, then a click to 7d;
    • E3: a link to Scopes, which opens the stored Hop depth; then a click to Overview, a tab switch to Wardriving and back;
    • E4: sub=regions;
    • E5: Hash Stats, then a click to Confirmed;
    • E6: Hash Issues bytes=2&section=…, a tab switch to Hash Stats and back.

    The probe then went Back to E1 and Forward to E6. [T]

    • PR: every entry showed its own view in both directions. Back to E3 showed Overview with URL #/analytics?tab=scopes.
    • master (same server, master's public/): Back to E3 showed Regions and rewrote that entry's URL to …&sub=regions. Forward to E3 did the same. Back to E5 showed All, because master has no mbf.
  • Deep links from fix(analytics): escape-safe ?tab= lookup and withQuery contract (#193) #194/feat(analytics): deep-link Scopes sub-tabs and window (#205) #206 still win.

    • Cold loads of sub=hygiene&swin=7d, wdwin=1h and tab=collisions&bytes=3 showed those values with the URL unchanged.
    • A hostile sub=<img> fell back to Overview with no page error. [T]
  • Old history.state without the field.

    • {legacy:1}, then a reload: the stored value is used, and legacy:1 is kept next to the new analyticsView.
    • A garbled record {analyticsView:{sub:7, swin:'<x>'}, other:'k'}: the non-string sub is ignored (the stored value is used), <x> resolves to the default 24h, and other is kept. [T]
  • Precedence is URL, then the entry record, then sessionStorage, then the default. Every value goes through resolveViewParam's === allow-list, and nothing from the URL or state reaches a selector or markup. [A]

6. Hash Stats

  • mbf= deep link. The filter is deep-linked as mbf=, URL only.
    • A click writes it; a reload keeps it; All removes it.
    • A tab switch drops it, because it is in TAB_URL_PARAMS.hashsizes.
    • A hostile value gives All and a canonical URL.
    • These are the PR's E2E steps, green here (15/15). [T]
    • The value is validated twice: by restoreViewParams and again in renderMultiByteAdopters. [A]
  • Sort. It stays local. The colIdx off-by-one is a real, pre-existing defect; see F1 and the proposed issue below. [T][A]

7. section= dropped, bytes= kept

Proposed follow-up issue (not created)

Title: fix(analytics): Hash Stats multi-byte adopters column sort reads the wrong column and has no state

Summary. In public/analytics.js (renderMultiByteAdopters, the [data-sort] click handler), colIdx is { name: 0, status: 1, hashSize: 2, packets: 3, lastSeen: 4 }, but the table has six columns: Node, Role, Status, Hash Size, Adverts, Last Seen. The Role column and this map were added together in 45623672. So:

  • Role does nothing;
  • Status reads the Role cell, so every row gets weight 2 and the order is unchanged;
  • Hash Size reads the Status text;
  • Adverts reads the Hash Size cell;
  • Last Seen reads the Adverts cell, as a string.

Even with the right index, Last Seen would compare timeAgo text ("5m ago") as strings. The sort is ascending only, keeps no state or indicator, and is discarded by the next filter click.

Evidence. On the E2E fixture, clicking Adverts leaves the Adverts column 2, 1, 1, ….

Proposed fix.

Acceptance criteria.

  • A unit test per column that asserts the order of that column's own cells.
  • The Last Seen order follows timestamps, not text.
  • The filter and mbf= behaviour are unchanged.

Relates to #208.

Cross-cutting checks

  • No behaviour change beyond the 7 items. [A]
    • _writeViewParams skips storage only for a spec without a storageKey, and only the new mbf spec lacks one.
    • replaceState now carries a state object instead of null on analytics view writes. Only analytics.js reads analyticsView; packets.js and packet-path-map.js use their own history handling on other routes.
    • The router does not read history.state.
  • Deep-link pattern. No URL value reaches a selector or HTML. Unknown values give the default for sub, swin, wdwin and mbf. [T][A]
  • scripts/check-xss-sinks.sh --diff. Exit 0, both against a0086bdd and against the merge-base c91a793f. It was run in a local --shared clone checked out detached at the head. [T]
  • Colours. The diff adds no new colour value. The three hex values in added lines are the existing var(--x, #fallback) fallbacks on the Hash Stats buttons, carried over unchanged with only class edited. [T]
  • API calls. No new fetch/api( in the frontend diff, so no per-item calls. [T]
  • cmd/server read-only. The non-test hunks add no Exec, INSERT, UPDATE, DELETE or file write. [T]
  • No new map[string]interface{} (0 added lines). [T]
  • Fork guards. deploy.yml 9 and release-fast-path.yml 1, identical to master; .github is unchanged. [T]
  • No closing keywords in the title, body or the 15 commit messages. [T]
  • Commit identity. All 15 commits have author and committer dborup <kontakt@meshview.dk>. [T]

Tests

Run (merged tree unless noted) Result
cd cmd/server && go test -race -count=1 ./... FAIL (551 s): exactly one failing test, TestIssue1008_HandlerReturns503WhileSubpathIndexLoading, a pre-existing timing flake that also fails on master (F5); no DATA RACE [T]
sh test-all.sh 214 passed, 0 failed [T]
node test-frontend-helpers.js 707 passed, 0 failed [T]
test-issue-199-inactive-observer-e2e.js 3 passed, 0 failed [T]
test-issue-180-packets-url-modal-e2e.js 12 passed, 0 failed [T]
test-issue-205-analytics-subtab-deeplinks-e2e.js 15 passed, 0 failed [T]
test-issue-1306-collisions-terminology-e2e.js 23 passed, 0 failed [T]
Back/forward, deep-link and legacy-state probe as described under item 5; no page errors [T]
CI on head (Go Build & Test, Playwright, Docker) success [K]

Mutants

Each mutant was applied to a copy of the merged tree. The E2Es ran against a fresh server on that copy, stopped by its port-owner pid after each mutant.

# Mutant Result
MuA history state ignored (entry = null in restoreViewParams) killed: unit (96 passed, 4 failed) and E2E 205 (13 passed, 2 failed: both Back/Forward steps) [T]
MuB section kept (collisions: []) killed: unit switching from Hash Issues to another tab drops section=, keeps bytes= and window= [T]. test-issue-1306 stays 23/23 with this mutant, as expected. [T]
MuC rate limit removed (if false && … in note) killed: TestNodeDetail404LookupErrorIsLoggedOnce (3 lines for 3 requests) and TestMissingNodeLookupLogThrottle [T]
M8 Last advert row dropped survives master's tests; killed on the PR by unit and E2E 199 [T]
M4 overlay.contains(el) removed survives master's tests; killed on the PR by unit and both E2E 180 Escape steps [T]

The author's M1a–M7b were not re-run. [K]

Not verified

  • Browsers other than Chromium. Safari's replaceState throttling is covered only by the existing try/catch. [A]
  • The whole CI E2E step; I ran the four files above plus my own probes.
  • cmd/ingestor, which the PR does not touch.
  • A second full -race run of cmd/server. As instructed there was only one, so the suite is green only with F5 discounted. CI's Go job on this head is green. [K]
  • Staging and prod.

The head was 0f77c97499fe484321ab9b8a6a147109d1afc870 before this review (git ls-remote) and still 0f77c97499fe484321ab9b8a6a147109d1afc870 after it. The PR is still a draft and was not modified.


Generated by Claude Code

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.

3 participants