Repository navigation
Conversation
dborup
left a comment
There was a problem hiding this comment.
Review against codex/my-mesh-activity-304:
[P2] Preserve the route-backfill uncertainty on the My Mesh card. The existing node-detail include=advertRoutes response includes advertCounts.route_mask_backfill and node-adverts.js warns when its status is not complete. This PR forwards only advertIntervals to health. Until route-mask backfill completes, legacy adverts can be classified from their first-stored route, so a normal-looking zero-hop/flood cadence on the card can be provisional. Please carry the existing backfill status through the opt-in health response and show the same caveat (or suppress the estimate) when it is not complete. Add a backend/browser regression for that state.
The rest of the approach looks sound: it reuses #245 rather than duplicating interval logic, keeps the cold scan opt-in and cached, and applies the identity-hidden gate before the lookup. I ran the full server, ingestor and frontend suites plus the My Mesh Chromium tests; all passed after the existing Kpa-clawbot#2027 fixture was updated.
|
Review feedback addressed (commit
|
dborup
left a comment
There was a problem hiding this comment.
Follow-up review of commit 1f54e5a4 against the stacked base: the route-backfill finding is resolved. The opt-in health response now forwards the existing typed backfill status behind the identity-hidden gate; the card warns for pending, backfilling, and unknown status, while a complete status stays quiet. Backend and desktop/mobile browser regressions cover the distinction. I found no further actionable issues in this follow-up.
Local validation is green (full Go server and ingestor suites, 227 frontend test files, both My Mesh Chromium tests). This stacked PR has no automatic PR check because the workflow filters pull requests to master; a manual branch run is being used for CI validation. No merge or deployment is implied.
Review — CS-MacBook PR#352 — head 1f54e5aDom: APPROVE with nits Independent, read-only review of this PR's own diff on top of its stacked base Findings
No correctness, privacy, or XSS issues found. Answers to the review points1. Separate zero-hop & flood intervals, reuse of #245/#247/#292 (DRY), values vs node detail. 2. Provisional route-classification warning — shown in the right cases? 3. cmd/server read-only, no new untyped maps, hardcoded colours, XSS. 4. Fork-guards / closing keywords / commit author. Acceptance criteria → tests
Tests & mutants (merged tree
|
Bring #351 rounds 2-3 (merged as 1f423e0) under #352's advert cadence. Conflicts: - public/home.js: keep master's clipped-name disclosure (title + revealClippedObserverButtons) and add #352's cadence block after the observer row. - test-issue-304-my-mesh-e2e.js: keep master's eight mixed-role cards, wide/clipped names and the escapeAttr quote test; carry #352's advertIntervals/advertRouteBackfill fixtures on repeaters 0-2 and the opt-in health URL. Also pin that the no-estimate repeater shows no provisional badge and a non-repeater shows no cadence. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…che (#352) #352 moved the card's health request to ?include=advertIntervals but loadHealth still fetched the plain URL, so a card click became a second network request instead of a client-cache hit. CI's home-coverage E2E caught it as a race (the panel was still "Loading…"). Pin that a card click renders from the card's own URL, and that a node outside My Mesh keeps the plain URL (no advert scan). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…352) One healthPath() helper builds the card request and the panel request. For a claimed node the panel asks for the same ?include=advertIntervals URL the card already fetched, so the client cache answers it as before #352. Nodes outside My Mesh keep the plain URL and skip the advert scan. Update the Kpa-clawbot#2027 harness comment that still said Full health used the plain URL. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rapport — CS-pve-agent3 PR#352 master-sync — head a5c782aReview feedback addressed (commit
Tests
Mutants (each applied alone, then reverted; tree clean afterwards)
The merge has no new production logic, so M1–M8 serve as its red runs. The new finding has a real red-before-fix commit ( CI per jobHead
Head Leftovers
|
|
Non-blocking review note on head |
Summary
Add separate estimated zero-hop and flood advert intervals to repeater cards on My Mesh (part of #304).
This reuses the existing #245 estimator and its cached, route-aware advert scan. The My Mesh card opts in on its existing node-health request (
/api/nodes/{pk}/health?include=advertIntervals). It does not make another HTTP request per card or duplicate the interval algorithm. For a claimed node, the health panel (card click / Full health) reuses the same URL through onehealthPath()helper, so the client cache answers it. Nodes outside My Mesh keep the plain/healthURL. Non-repeater and default health responses do not pay for the advert lookup. Hidden identities fail closed before the lookup.The card distinguishes estimated, too few, none observed, irregular, and unavailable states. It shows the sample count and latest advert when present, and says the values are observed estimates from at most the newest 20 adverts per class, not device configuration. Missed receptions may skew the estimate. While route-mask backfill is not
complete(or its status is missing), the card marks the route classes provisional and forwards the existing typedadvertRouteBackfillstatus. The full node page remains the detailed view.Base
The base is
master. #351 (activity chart and compact observer list) merged as1f423e06. This branch now mergesorigin/masterwith a normal merge commit (no rebase or force-push). The merge keeps #351's rounds 2–3:title, plusFull names →revealed byrevealClippedObserverButtons);escapeAttrquote/XSS regression intest-issue-304-my-mesh-e2e.js.#352's cadence block renders after the observer row. The E2E fixture keeps #351's eight mixed-role cards and adds the cadence data to repeaters 0–2.
Validation
cmd/server:go test -race ./...passed.cmd/ingestor:go test ./...passed.sh test-all.sh(234/234) andnode test-frontend-helpers.js(712) passed. One unrelated timing flake intest-channels-client-state-152.js(unchanged from master) was seen once under load and passed on re-run.test-issue-304-my-mesh-e2e.js(desktop 1280 and mobile 375),test-issue-2027-my-mesh-node-page-e2e.js(25/25) andtest-home-coverage-e2e.js(12/12, 5 runs) passed with a local Go server on the CI-preparede2e-fixture.db./health?include=advertIntervalsand/nodes/{pk}?include=advertRoutesreturn identical intervals and backfill status for the feat(nodes): estimated advert intervals (flood / zero-hop) per node, then mesh-wide settings analytics #245 seed node. Default/healthreturns neither field.--diff origin/masterand--file public/home.js),node --check, andgit diff --checkpassed.Performance and privacy
The existing
nodeAdvertRoutescache has a 30-second TTL, per-node invalidation, a 128-entry cap, and singleflight. A backend test checks that three opt-in health calls perform only one scan, and that default and non-repeater calls perform none. A cold scan still costs more than ordinary health, so the field is opt-in. The same identity-hidden gate as node detail runs before the cached breakdown is accessed.No merge or deployment is requested here.
🤖 Generated with Claude Code