Skip to content

fix(health): judge favorites and home status cards by the node's role - #2215

Merged
efiten merged 4 commits into
Kpa-clawbot:masterfrom
nullrouten0:pr/role-aware-health-cards
Oct 17, 2026
Merged

efiten merged 4 commits into
Kpa-clawbot:masterfrom
nullrouten0:pr/role-aware-health-cards

Conversation

@nullrouten0

@nullrouten0 nullrouten0 commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

The problem

A starred repeater can show two different health statuses. The My Repeaters tab (#1761) and getNodeStatus judge it by the infra thresholds (degraded after 24 h, silent after 72 h). The favorites dropdown and the home page cards judge every node by the node thresholds (degraded after 1 h, silent after 24 h), because they never look at the node's role.

repeater last heard favorites / home cards (before) My Repeaters tab after this PR
5 h ago degraded healthy healthy
30 h ago silent degraded degraded
80 h ago silent silent silent

If you set healthThresholds in config.json, every view follows your values.

The change

Three call sites now use getHealthThresholds(role), the helper the My Repeaters tab already uses:

  • renderFavDropdown in app.js
  • the "My Nodes" grid card in home.js
  • the hero search-result card in home.js

The search card's fallback text, "Not heard in 24+ hours", was only true for non-infra nodes, so it now reads "Not heard recently".

Scope

  • I kept the existing three-state healthy / degraded / silent logic and changed only which thresholds apply.
  • Not included: counting a repeater that is relaying traffic as alive (Active nodes are disappearing from the map #1598, which getNodeStatus does). That is a bigger behavior change and would be its own PR.

Risk

Low. It is 9 lines in two files, and the helper and thresholds already exist and are used elsewhere.

Tests

New tests/unit/test-health-card-role-thresholds.js, added to test-all.sh. It checks that infra roles get looser thresholds than nodes, and that each of the three call sites goes through getHealthThresholds(role) with no node-only comparison left. On current master 5 of its 7 assertions fail; with this change all pass.

sh test-all.sh passes. It prints a STALE ALLOWLIST warning from an a11y self-test, which it also does on master.

Update after review (commit 0f2376df)

The text above is the description efiten reviewed on 2026-10-15. This section describes the code as it is now.

The change

  • New getHealthStatus(role, heard, now) in roles.js. It returns healthy, degraded or silent by the role's thresholds, or null when there is no usable timestamp. It only applies the cut-offs; each caller picks the timestamp.

  • All six places that compared an age against degradedMs / silentMs now call it:

    • the favorites dropdown (app.js);
    • the "My Nodes" card and the home health card (home.js);
    • the analytics network status and My Repeaters (analytics.js);
    • the live node panel (live.js).

    Each keeps its own labels and timestamp, so the last three show the same as before.

    analytics.js and live.js no longer guard with typeof getHealthThresholds. roles.js loads before both, and both already used it without a guard elsewhere: the old network status called getHealthThresholds directly, and live.js reads ROLE_COLORS.

  • The favorites dropdown and both home cards now fall back to the node's last_seen when the packet store has no packet for it. My Repeaters (last_heard || last_seen) and the node page (nodes.js:729) already do this. Without it, with a packet store window under 72 h, a repeater could be silent on the cards and degraded on My Repeaters. The "ago" text uses the same timestamp.

    With the default thresholds, the fallback changes a status only when the packet store holds less than 72 h of packets. retentionHours unset (unlimited) or at the example's 168 h holds more than that. Otherwise a node with no packet in the window is silent either way, and the views now show how long ago it was last seen instead of "never" or nothing.

  • With no timestamp at all, the home health card said "Not heard in 24+ hours". It now says "No packets in the current window".

Why a new helper and not getNodeStatus

getNodeStatus has two states (active / stale) and counts a repeater that relays traffic as alive (#1598). Using it here would change more than the thresholds. getHealthStatus keeps these views' three states.

Scope

  • Not included: counting a repeater that relays traffic as alive (Active nodes are disappearing from the map #1598) in these views. That is a bigger behavior change and would be its own PR.
  • The live node panel still judges by last_seen alone, and the node page stays relay-aware. Both now use the shared thresholds; aligning their timestamps would also be its own change.

Tests

  • tests/unit/test-health-status.js runs roles.js in vm. It covers:

    • repeater, room, companion, sensor, an unknown role and a missing role, at ages either side of 1 h, 24 h and 72 h;
    • ISO and epoch input;
    • null, empty and invalid timestamps;
    • a healthThresholds override through /api/config/client.

    One source guard remains: no age-vs-threshold comparison outside roles.js.

  • tests/unit/test-my-repeaters-dashboard.js now uses the real helper instead of a stub, which gave repeaters the node thresholds. It also checks that a repeater heard 30 h ago is Degraded.

  • New tests/e2e/test-health-status-e2e.js mocks /api/nodes/<pk>/health for six nodes. It checks five views: the favorites dropdown, the "My Nodes" card, the home health card, the analytics network status and the live node panel. It fails on master and on this PR's first commit. I put it on shard 2, which was the shortest shard in the last CI run.

  • Mutation check: I made 33 deliberate breaks. In the helper they cover each comparison and boundary. At each of the six call sites they cover the timestamp field, the role argument and at least one label mapping. Each one makes the suite fail.

  • I checked a config.json with infraDegradedHours: 2 and infraSilentHours: 6 in Chromium. Repeaters heard 3 h and 7 h ago show Degraded and Silent.

  • sh test-all.sh passes. It prints a STALE ALLOWLIST warning, which comes from a deliberate fixture in the a11y self-test.

Cost

The analytics network status loops over every node. In headless Chromium, 30,000 synthetic nodes take 9.0 to 9.2 ms before this change and 9.1 ms after (median of 21 runs, on one machine).

Risk

Low. The change is frontend only, with no API, config or data change. Rolling back means reverting the commits.


🤖 Generated with Claude Code

renderFavDropdown (app.js) and the two health cards in home.js ("My Nodes"
and the hero search result) compared every node against the node
thresholds (1 h degraded, 24 h silent) and never looked at its role. A
starred repeater or room was therefore held to limits meant for companions:
last heard 5 h ago it showed "degraded" and at 30 h "silent", while the
My Repeaters tab (Kpa-clawbot#1761) and getNodeStatus in roles.js, which both use
getHealthThresholds(role), call those healthy and degraded (infra limits:
24 h / 72 h).

These three call sites now use getHealthThresholds(role) as well. The
search card's "Not heard in 24+ hours" fallback text was only true for
nodes, so it now reads "Not heard recently".

The new test fails on the old code (5 of 7 assertions) and passes now.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@nullrouten0
nullrouten0 force-pushed the pr/role-aware-health-cards branch from fd3f596 to 40f7c0c Compare October 15, 2026 06:31

@efiten efiten left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The change itself is right; requesting changes for the test only.

What I checked

  • Role field: the favorites dropdown and both home cards read /api/nodes/{pk}/health, whose node is the DB row (role key) or a stub with role: "unknown" (cmd/server/store.go:9966). The ingestor writes lowercase roles, so the comparison in getHealthThresholds (public/roles.js:439) matches.
  • Missing role: h.node && h.node.role (public/app.js:1751) is null-safe. An unknown or missing role falls back to the node thresholds, as before.
  • No node-only comparison left: after this PR none remains in public/. I grepped for degradedMs, silentMs, 3600000, 86400000, getHealthThresholds and getNodeStatus. The remaining hits are observer thresholds or time formatting.
  • Config overrides: healthThresholds overrides now reach these views, because getHealthThresholds reads HEALTH_THRESHOLDS on every call.
  • Tests run:
    • The new test passes 7/7 on the head and fails 5/7 on master.
    • test-frontend-helpers.js passes 769/769.
    • test-all.sh passes, apart from the XSS-gate fixtures that need Python, which is not installed on my machine.

Requested change

  1. The call-site tests check source text, not behaviour (tests/unit/test-health-card-role-thresholds.js:44-72). The two vm tests check values in roles.js that already held before this PR. The other five look for getHealthThresholds( between string markers. They would still pass in these cases:

    • the thresholds are swapped (th.silentMs where th.degradedMs belongs);
    • a comparison is reversed;
    • the wrong field is passed at app.js:1751 (that test accepts any argument).

    They would also break on a harmless rename of a marker. Two ways to fix it:

    • (a) A pure helper in roles.js, for example getHealthStatus(role, lastHeardIso, now) returning 'healthy' | 'degraded' | 'silent' | null. Use it at the three call sites and test it in vm for repeater, room, companion, sensor, an unknown role and a null timestamp. This also removes the duplication in note 1 below.
    • (b) An E2E test that mocks /api/nodes/X/health for a repeater last heard 30 h ago, and asserts the home card shows Degraded and not Silent.

    Keeping one source check, that no HEALTH_THRESHOLDS.node* is used outside roles.js, is fine as a guard against regressing.

Notes, not blocking

  1. The age < degradedMs ? healthy : age < silentMs ? degraded : silent chain now appears in the three call sites (public/app.js:1751-1754, public/home.js:298-300 and 457-464), and also in public/analytics.js:2665 and public/live.js:2613-2616. Option (a) above would fold these into one helper.
  2. These cards judge stats.lastHeard from the packet store with no last_seen fallback, while My Repeaters (analytics.js:2659) uses last_heard || last_seen. That is not introduced here. With the default retentionHours: 168 the two agree. With a store window under 72 h, a repeater heard 50 h ago can still differ between the cards and My Repeaters. Either mention it as out of scope, or fall back to node.last_seen when stats.lastHeard is null. Your call.
  3. "Not heard recently" (home.js:458) is shown when stats.lastHeard is null, so when there is no packet in the store window at all. "No packets in the current window" says that more exactly. Not blocking: it is already more accurate than the old "24+ hours" text.

Review feedback on Kpa-clawbot#2215: the tests checked source text, so a swapped
threshold, a reversed comparison or the wrong field at a call site still
passed.

getHealthStatus(role, heard, now) in roles.js returns healthy / degraded /
silent by the role's thresholds, or null when there is no usable
timestamp. The six places that compared an age against degradedMs /
silentMs now call it: the favorites dropdown, both home cards, the
analytics network status, My Repeaters and the live node panel. Each keeps
its own timestamp and labels.

The favorites dropdown and both home cards now fall back to the node's
last_seen when the packet store has no packet for it, like My Repeaters
(last_heard || last_seen) and the node page (nodes.js). Before, with a
store window under 72 h a repeater could be silent on the cards and
degraded on My Repeaters. The "ago" text uses the same timestamp, and the
search card says "No packets in the current window" when there is none.

Tests:
- test-health-status.js runs roles.js in vm: every role at ages either
  side of 1 h, 24 h and 72 h, ISO and epoch input, missing and invalid
  timestamps, and a healthThresholds override through /api/config/client.
  One source guard: no threshold comparison outside roles.js.
- test-my-repeaters-dashboard.js uses the real helper instead of a stub,
  and checks that a repeater heard 30 h ago is Degraded.
- New test-health-status-e2e.js mocks the health API for six nodes and
  checks all five views in Chromium. It fails on master and on the
  previous commit of this PR.

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

nullrouten0 commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor Author

Review feedback addressed (commit 0f2376df). The PR description keeps the text you reviewed, with an "Update after review" section added below it.

  1. The tests now check behavior. I went with your option (a): getHealthStatus(role, heard, now) in roles.js. tests/unit/test-health-status.js runs it in vm. It covers repeater, room, companion, sensor, an unknown role and a missing role, at ages either side of 1 h, 24 h and 72 h. It also covers ISO and epoch input, null, empty and invalid timestamps, and a healthThresholds override through /api/config/client.

    I also added (b): tests/e2e/test-health-status-e2e.js on shard 2. It mocks /api/nodes/<pk>/health for six nodes and checks the favorites dropdown, the My Nodes card and the home health card, plus the analytics network status and the live node panel. It fails on master and on 40f7c0c9.

    The three mutations you listed now fail the suite (swapped thresholds, a reversed comparison, the wrong field at app.js). So do 30 more: each comparison and boundary in the helper, and at each call site the timestamp field, the role argument and at least one label mapping.

    I kept one source guard, but as "no age-vs-threshold comparison outside roles.js". "No HEALTH_THRESHOLDS.node* outside roles.js" would flag nodes.js:194, which reads nodeSilentMs only for the tooltip text.

  2. Duplication (note 1): all six copies now call the helper. That is the five you listed plus the analytics network status at analytics.js:2466. Each keeps its own labels and timestamp. The My Repeaters test now uses the real helper; its stub had been giving repeaters the node thresholds.

  3. last_seen (note 2): the dropdown and both home cards fall back to node.last_seen, as My Repeaters (last_heard || last_seen) and the node page (nodes.js:729) do. The "ago" text uses the same timestamp.

  4. Fallback text (note 3): it now says "No packets in the current window".

Not changed: the live node panel still judges by last_seen alone, and the node page stays relay-aware (#1598). Both now use the shared thresholds.

The helper added in 0f2376d is defined in roles.js and used from
app.js, home.js, analytics.js and nodes.js; .eslintrc.json did not list
it, so CI's eslint no-undef step failed on six call sites.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@nullrouten0

Copy link
Copy Markdown
Contributor Author

CI fix (commit 5d5446eb). The lint step failed on 0f2376df because getHealthStatus, the helper added in that commit, was not declared in .eslintrc.json, so eslint reported its six call sites as undefined. One line added next to getHealthThresholds; no code change. Lint and test-all.sh pass locally and CI is green on this head.

@efiten efiten left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review feedback is addressed, thanks. Approving. One thing before it can merge: the branch now conflicts with master.

Conflict. .github/workflows/deploy.yml conflicts with master since #2209 and #2211 merged today. Both added a suite 2 line next to yours (test-issue-748-node-analytics-coverage-e2e.js and test-issue-1268-rf-params-e2e.js). Please rebase or merge master and keep all three suite 2 lines, including test-health-status-e2e.js. .eslintrc.json, scripts/non-unit-tests.json and test-all.sh merge without conflict.

What I checked at 5d5446eb

  1. Behaviour tests. getHealthStatus (public/roles.js:452-459) runs in vm in tests/unit/test-health-status.js (14 passed), and tests/e2e/test-health-status-e2e.js passed 8/8 against a local server on the e2e fixture.
    • The E2E fails with the frontend of 1ae4c749 ("E2E Rptr 5h: card class degraded, want healthy") and of 40f7c0c9 ("E2E Rptr DB 30h: card class silent, want degraded").
    • The three mutations from my review are caught: swapped thresholds (10 unit failures), reversed comparison (11 unit failures, 4 in test-my-repeaters-dashboard) and the wrong field at app.js (E2E: "dot class status-warn, want status-ok").
    • I also tried 12 more: <= at the boundary, room not counted as infra, the dropped last_seen fallback, the swapped dot class, an undefined role at each call site, and n.last_heard alone at analytics.js:2460. 11 were caught. The survivor (dropping the empty-string check) is equivalent, since new Date('') is invalid and the helper returns null either way.
  2. Duplication. All six copies call the helper: app.js:1753, home.js:300, home.js:459, analytics.js:2460, analytics.js:2654 and live.js:2613. nodes.js:194 uses nodeSilentMs only for the tooltip text, as you said.
  3. last_seen fallback and the "No packets in the current window" text are in place and asserted in the E2E.

eslint on the touched files: 0 errors, and the warnings are identical to the base.

Minor, no action needed

  • The source guard (test-health-status.js:61-65) catches a comparison against a property (age < th.degradedMs), not one after destructuring or against a literal. The unit and E2E tests carry the behaviour, so this is fine as a tripwire.

Resolve the deploy.yml conflict from Kpa-clawbot#2209 and Kpa-clawbot#2211: keep all three
suite 2 lines (748 node analytics, health status, 1268 rf params).

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

Copy link
Copy Markdown
Contributor Author

Merged master (commit 5bf4df16); no rebase, so 5d5446eb and the commits before it are unchanged.

  • Conflict: .github/workflows/deploy.yml keeps all three suite 2 lines: test-issue-748-node-analytics-coverage-e2e.js, test-health-status-e2e.js and test-issue-1268-rf-params-e2e.js. .eslintrc.json, scripts/non-unit-tests.json and test-all.sh merged without conflict.
  • No code change: master did not touch roles.js, app.js, home.js, analytics.js, live.js or the three test files since this branch's base, so they are identical to what you approved at 5d5446eb.
  • Checked on the merged head: npm run lint 0 errors; test-all.sh passes; test-health-status.js 14 passed; test-health-status-e2e.js 8/8 against a local server on the migrated e2e fixture. CI is green on this head.

@efiten
efiten merged commit 127dd51 into Kpa-clawbot:master Oct 17, 2026
12 checks passed
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.

2 participants