Repository navigation
fix(health): judge favorites and home status cards by the node's role - #2215
Conversation
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>
fd3f596 to
40f7c0c
Compare
efiten
left a comment
There was a problem hiding this comment.
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, whosenodeis the DB row (rolekey) or a stub withrole: "unknown"(cmd/server/store.go:9966). The ingestor writes lowercase roles, so the comparison ingetHealthThresholds(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 fordegradedMs,silentMs,3600000,86400000,getHealthThresholdsandgetNodeStatus. The remaining hits are observer thresholds or time formatting. - Config overrides:
healthThresholdsoverrides now reach these views, becausegetHealthThresholdsreadsHEALTH_THRESHOLDSon every call. - Tests run:
- The new test passes 7/7 on the head and fails 5/7 on master.
test-frontend-helpers.jspasses 769/769.test-all.shpasses, apart from the XSS-gate fixtures that need Python, which is not installed on my machine.
Requested change
-
The call-site tests check source text, not behaviour (
tests/unit/test-health-card-role-thresholds.js:44-72). The twovmtests check values inroles.jsthat already held before this PR. The other five look forgetHealthThresholds(between string markers. They would still pass in these cases:- the thresholds are swapped (
th.silentMswhereth.degradedMsbelongs); - 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 examplegetHealthStatus(role, lastHeardIso, now)returning'healthy' | 'degraded' | 'silent' | null. Use it at the three call sites and test it invmfor 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/healthfor 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 outsideroles.js, is fine as a guard against regressing. - the thresholds are swapped (
Notes, not blocking
- The
age < degradedMs ? healthy : age < silentMs ? degraded : silentchain now appears in the three call sites (public/app.js:1751-1754,public/home.js:298-300and457-464), and also inpublic/analytics.js:2665andpublic/live.js:2613-2616. Option (a) above would fold these into one helper. - These cards judge
stats.lastHeardfrom the packet store with nolast_seenfallback, while My Repeaters (analytics.js:2659) useslast_heard || last_seen. That is not introduced here. With the defaultretentionHours: 168the 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 tonode.last_seenwhenstats.lastHeardis null. Your call. - "Not heard recently" (
home.js:458) is shown whenstats.lastHeardis 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>
|
Review feedback addressed (commit
Not changed: the live node panel still judges by |
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>
|
CI fix (commit |
efiten
left a comment
There was a problem hiding this comment.
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
- Behaviour tests.
getHealthStatus(public/roles.js:452-459) runs invmintests/unit/test-health-status.js(14 passed), andtests/e2e/test-health-status-e2e.jspassed 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 of40f7c0c9("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 atapp.js(E2E: "dot class status-warn, want status-ok"). - I also tried 12 more:
<=at the boundary, room not counted as infra, the droppedlast_seenfallback, the swapped dot class, an undefined role at each call site, andn.last_heardalone atanalytics.js:2460. 11 were caught. The survivor (dropping the empty-string check) is equivalent, sincenew Date('')is invalid and the helper returns null either way.
- The E2E fails with the frontend of
- Duplication. All six copies call the helper:
app.js:1753,home.js:300,home.js:459,analytics.js:2460,analytics.js:2654andlive.js:2613.nodes.js:194usesnodeSilentMsonly for the tooltip text, as you said. last_seenfallback 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>
|
Merged master (commit
|
The problem
A starred repeater can show two different health statuses. The My Repeaters tab (#1761) and
getNodeStatusjudge 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.If you set
healthThresholdsinconfig.json, every view follows your values.The change
Three call sites now use
getHealthThresholds(role), the helper the My Repeaters tab already uses:renderFavDropdowninapp.jshome.jshome.jsThe 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
getNodeStatusdoes). 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 totest-all.sh. It checks that infra roles get looser thresholds than nodes, and that each of the three call sites goes throughgetHealthThresholds(role)with no node-only comparison left. On currentmaster5 of its 7 assertions fail; with this change all pass.sh test-all.shpasses. It prints aSTALE ALLOWLISTwarning from an a11y self-test, which it also does onmaster.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)inroles.js. It returnshealthy,degradedorsilentby the role's thresholds, ornullwhen 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/silentMsnow call it:app.js);home.js);analytics.js);live.js).Each keeps its own labels and timestamp, so the last three show the same as before.
analytics.jsandlive.jsno longer guard withtypeof getHealthThresholds.roles.jsloads before both, and both already used it without a guard elsewhere: the old network status calledgetHealthThresholdsdirectly, andlive.jsreadsROLE_COLORS.The favorites dropdown and both home cards now fall back to the node's
last_seenwhen 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.
retentionHoursunset (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
getNodeStatusgetNodeStatushas two states (active / stale) and counts a repeater that relays traffic as alive (#1598). Using it here would change more than the thresholds.getHealthStatuskeeps these views' three states.Scope
last_seenalone, 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.jsrunsroles.jsinvm. It covers:healthThresholdsoverride through/api/config/client.One source guard remains: no age-vs-threshold comparison outside
roles.js.tests/unit/test-my-repeaters-dashboard.jsnow 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.jsmocks/api/nodes/<pk>/healthfor 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 onmasterand 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.jsonwithinfraDegradedHours: 2andinfraSilentHours: 6in Chromium. Repeaters heard 3 h and 7 h ago show Degraded and Silent.sh test-all.shpasses. It prints aSTALE ALLOWLISTwarning, 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