feat(meshcore): per-source node-age filtering and Node Display settings (closes #4412)

[Yeraze]

Closes #4412. Final phase of the per-source Node Display epic. Builds on Phase 1 (#4417), Phase 2 (#4425), Phase 3 (#4431).

Plan: docs/internal/dev-notes/PER_SOURCE_NODE_DISPLAY_EPIC.md
Spec: docs/internal/dev-notes/PER_SOURCE_NODE_DISPLAY_PHASE4_SPEC.md

The bug this fixes first — MeshCore alerts you could not configure

inactiveNodeNotificationService.ts:266 already branches on sourceType === 'meshcore' and queries getInactiveMeshcoreNodes. So the three inactive-node settings — threshold, check interval, cooldown — have been driving inactive-node alerts on MeshCore sources with no UI anywhere to configure them. A MeshCore source had no Node Display settings surface at all.

That is fixed here. It was found by tracing consumers to decide which settings belong on the MeshCore surface, rather than guessing.

What the reporter asked for

"Can you set up Node Display menu for the map/list based on last advert time, same as for Meshtastic?"

Two things were missing, not one:

1. The settings surface. SettingsTab mounts only at GlobalSettingsPage.tsx:89 and App.tsx:3640 (the Meshtastic source route); main.tsx:86 routes MeshCore to MeshCoreSourcePage, which never rendered it. New MeshCoreNodeDisplaySection composes into the existing MeshCoreSettingsView.

Mounting SettingsTab was considered and rejected: it needs ~50 props (~24 of them no-op callbacks — a seam that type-checks and lies), and SOURCE_SECTIONS would drag firmware update, packet monitor, telemetry, solar and sorting onto a MeshCore page. MeshCore-shaped UI, shared storage — it persists through the existing /api/settings?sourceId=, not a new route, because forking one setting across two endpoints is the drift #4416 exists to fix.

Four fields, not ten: max age + the inactive trio. The other six have Meshtastic-view-only consumers.

2. The filter. MeshCoreNodesView had no age filter whatsoever. Now filters on the reconciled meshcore_nodes.lastHeard (prefer lastSeen, fall back to lastAdvert * 1000) with favorites and the local node exempt, at parity with useProcessedNodes — otherwise a favorited MeshCore node silently vanishes.

Units: one place knows the rule

nodes.lastHeard is seconds, meshcore_nodes.lastHeard is milliseconds, firmware lastAdvert is seconds. New src/utils/meshcoreAge.ts owns the conversion and three existing hand-rolled copies were collapsed onto it. Grep-verified: no * 1000 / / 1000 arithmetic on these fields remains outside the helper.

No lastAdvert column was added — the interview chose the reconciled lastHeard, which works for rows already in the DB.

Retired: an unreachable branch from Phase 3

Phase 3 added an isMeshCoreSource branch to SettingsTab hiding two fields, on my assumption that Phase 4 would mount SettingsTab on MeshCore. It doesn't, so the branch is unreachable by construction. Removed, along with its 2 tests — which built a <SourceProvider sourceType="meshcore"> no route constructs and would have passed even if the MeshCore surface showed every Meshtastic knob. Replacement coverage is MeshCoreNodeDisplaySection.test.tsx. The epic doc's now-wrong note is corrected in place rather than deleted, so nobody "restores" the branch believing coverage was lost.

Test fixtures that would have hidden the filter

All 17 MeshCoreNodesView tests used lastHeard: 1000/2000/3000 — epoch 1970. Every fixture falls outside any real cutoff, so the suite would have gone red for the wrong reason and tempted someone into weakening the filter. Rebased to Date.now()-relative ms; no assertion text changed.

Browser validation (live MeshCore source, deployed worktree build)

• The Node Display section exists on a MeshCore source — it did not before.
• Counts match ground truth computed from the raw API at every window: 10 @ 1h (9 in-window + exempt local node), 34 @ 24h, 48 @ 168h (47 + local). Stable across 10-15s sampling.
• Changes apply without a page reload — the cache-invalidation path (spec R3), the one defect a fully green suite could not catch.
• Per-source isolation: MC-Sandbox at 168 while MC-BLESandbox stayed 24.
• Only console error is a pre-existing locales/en-US.json 404.

Data-quality note: of 192 stored MeshCore nodes, 185 have plausible ms timestamps; 4 are future-dated and 2 near-epoch — devices with unset RTCs. The filter treats them as reported. Not a defect, but worth knowing before reading a node count as a bug.

Verification

• Full suite 12,696 passing, 0 failing, success: true via JSON reporter, PG + MySQL live, 109 skips unchanged.
• tsc --noEmit clean; lint:ci ratchet clean.
• Six test files owned by no work package passed unedited — the guardrail that a refactor must preserve the behavior they pin.

Still open

Per-source permission scoping (#4416): a source-A-scoped settings:write grant still authorizes source-B writes. This PR does not change that. Also #4419.

Closes #4412

🤖 Generated with Claude Code

https://claude.ai/code/session_01L9NzRtqE8eSMS8tvAeodUB

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: feat(meshcore): per-source node-age filtering and Node Display settings

Tasks

• Gather context and understand the PR
• Read key changed files
• Review code quality and best practices
• Check for potential bugs or issues
• Review performance considerations
• Check security concerns
• Review test coverage
• Post final review

---

This is a well-engineered PR. The self-documentation is thorough, the architecture decisions are sound, and the tests are unusually good. Here's what I found:

Findings

Bug / Logic Gap

isAgeExempt relies on name.includes('(local)') — a string-matching heuristic

MeshCoreNodesView.tsx:280:

const isAgeExempt = useCallback((r: MergedRow): boolean =>
  r.isFavorite || r.name.includes('(local)'), []);

The local-node detection in visibleContacts at line 306 uses c.advName?.includes('(local)'), and both match by how the server sets advName: \${localNode.name} (local)`inmeshcoreDeviceRoutes.tsandmeshcoreContactsRoutes.ts`.

This is an established cross-codebase convention (MeshCoreMap.tsx:local, MeshCoreMessageRouteModal.tsx:localContact all share it), so no functional regression here. However, the MergedRow.name field is populated from c.advName || c.name || 'Unknown' — if a user legitimately names their device something containing the string (local), their node would be accidentally exempted from age filtering. Given this is an existing convention rather than new code, it's worth a // NOTE: relies on server-side naming convention comment or a future issue to migrate to an isLocal boolean flag, but is not a blocker.

Potential Edge Case in mergeNodesAndContacts

MeshCoreNodesView.tsx:91:

existing.lastHeard = existing.lastHeard ?? meshcoreLastHeardMs(c) ?? undefined;

This correctly prefers the already-reconciled DB lastHeard from the MeshCoreNode over the raw contact timestamps. However, there's no test that exercises a node where existing.lastHeard is 0 (the "unknown" sentinel). In meshcoreLastHeardMs, 0 is treated as falsy/null and falls through to the next source. But in mergeNodesAndContacts, existing.lastHeard = 0 is a truthy value for the ?? operator, so a node row with lastHeard: 0 would not fall through to the contact's lastSeen/lastAdvert. This means a node with lastHeard: 0 in the DB would have no resolvable timestamp in the merged row, even if the contact has a valid recent one — and would be excluded by the age filter.

Whether lastHeard: 0 can reach the frontend depends on whether the backend ever writes it. Based on meshcoreManager.ts:459, it writes lastSeen preferring non-zero values, so this is likely a theoretical edge case. Still, aligning the merge logic to treat 0 the same as null (i.e. (existing.lastHeard || null) ?? meshcoreLastHeardMs(c) ?? undefined) would close the gap.

Input Validation Gap in MeshCoreNodeDisplaySection

MeshCoreNodeDisplaySection.tsx:174:

onChange={(e) => update('maxNodeAgeHours', parseInt(e.target.value, 10))}

All four inputs parse via parseInt. If the user clears the field (empty string), parseInt('') returns NaN, which is stored in the draft and serialized to "NaN" in the POST body. The server likely rejects/ignores this (since parseNodeDisplayNumber on the server side would produce the default for an invalid parse), but the UI will show NaN in the input. The Meshtastic SettingsTab has the same pattern (not introduced here), so this is consistent but worth noting. A guard like || 0 or falling back to the current value on isNaN would improve UX.

Minor: Translation Key Reuse

MeshCoreNodeDisplaySection.tsx:113:

showToast(t('automation.insufficient_permissions', 'Insufficient permissions'), 'error');

This borrows an automation.* i18n key for a settings context. This works because the fallback string is always used in tests, and the en.json file presumably has this key, but it conflates two different UI domains. A settings.insufficient_permissions key would be cleaner; this is a minor style concern only.

Similarly at line 126:

showToast(t('automation.settings_saved', 'Settings saved'), 'success');

Strengths

meshcoreAge.ts is the right abstraction. Centralizing all MeshCore timestamp normalization into one module with an explicit LAST_ADVERT_MS_THRESHOLD guard eliminates a whole class of future unit-confusion bugs. The module-level comment explaining all three time fields and their units is exactly what a future maintainer needs.

Test (7) — cache invalidation — is correctly marked mandatory. This is exactly the kind of integration test that catches a class of bugs no unit test can catch: the filter applies the stale cutoff until a queryClient.invalidateQueries call propagates. Making it explicit with a comment explaining what would break without it is excellent practice.

Fixture rebase is methodical. Moving from lastHeard: 1000/2000/3000 (epoch 1970) to Date.now()-relative values while preserving sort order/assertions was the right call — epoch-1970 fixtures hide filter regressions by design.

Favorites and local-node exemption parity with Meshtastic. Explicitly matching useProcessedNodes.ts:194 semantics in the comment and in the implementation reduces the chance of asymmetric behavior surprises between source types.

satisfies binding in MESHCORE_NODE_DISPLAY_KEYS. Using satisfies readonly NodeDisplaySettingKey[] means a deleted or renamed constant key in nodeDisplayDefaults.ts causes a compile error here rather than a silent runtime divergence.

visibleContacts for the map includes advName?.includes('(local)') as a fallback (MeshCoreNodesView.tsx:306). This prevents the map's polar-grid origin from disappearing if the local node happens to have no matching MeshCoreNode row — a subtle but correct edge case.

SaveBarProvider wrapping. MeshCorePage.tsx:202-214 correctly wraps the settings view (including the new section) in <SaveBarProvider>/<SaveBarGroup>/<SaveBar>, so the save bar lifecycle works without a separate provider registration.

Summary

The implementation is production-quality. The two most actionable findings are:

1. The lastHeard: 0 edge case in mergeNodesAndContacts (could cause a node to be incorrectly filtered out rather than falling through to the contact's timestamp).
2. The parseInt → NaN draft state when a settings input is cleared (minor UX issue, consistent with existing pattern).

Neither is a blocker, but (1) is worth a follow-up issue if lastHeard: 0 is observable in practice.

[Yeraze]

Thanks — four findings, all applied in 06370d32.

The one real bug: lastHeard: 0

Good catch, and it mattered more than "theoretical." meshcoreLastHeardMs already treats 0 as absent, but mergeNodesAndContacts used ??, which treats 0 as present. Two functions in the same feature disagreeing about whether 0 means "unknown" is precisely the kind of inconsistency that surfaces later as a node mysteriously missing from the list.

Fixed by aligning the merge to the helper's rule ((existing.lastHeard || null) ?? …). New test: a node row with lastHeard: 0 whose contact carries a 2-hour-old lastSeen now resolves to the contact's timestamp and appears. Previously it resolved to 0, was read as "known and ancient," and was filtered out.

NaN guard

Applied at the single update() setter rather than the four call sites, so clearing any field falls back to the last-known-good draft value. New test clears the field, asserts the input still shows 48 rather than NaN, saves, and asserts no value in the POST body is the string "NaN".

Agreed that SettingsTab having the same pattern isn't a reason to ship it in new code.

(local) heuristic

Behavior left alone — as you note, it's an established convention the server sets and three other components match. Added NOTE: comments at both match sites recording the dependency and that an explicit isLocal flag is the robust fix. I did not implement the flag here: it spans two server route files and three components, so it belongs in its own change rather than riding along in the PR that closes #4412. It's on the epic's follow-up list.

i18n keys

Added settings.node_display.* keys and switched the calls over. One note: you named two borrowed automation.* keys, but there was a third — automation.settings_save_failed in the same catch block, same problem. Converted it too rather than leaving one inconsistent case behind.

Verification

Full suite 12,698 passing / 0 failing, success: true via JSON reporter, PG + MySQL live, 109 skips unchanged. tsc clean, lint:ci clean. The six test files owned by no work package still pass unedited.

On your note about test (7): that framing is right, and it earned its keep. Browser validation showed the filter updating without a reload — which is the behavior that test pins, and the one thing a fully green unit suite genuinely could not have told us.

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: feat(meshcore): per-source node-age filtering and Node Display settings

Tasks

• Gather context and understand the PR
• Read key changed files
• Verify fixes for previously identified issues
• Review code quality and new additions
• Check test coverage
• Post final review

---

This is a follow-up review of the updated PR after Yeraze addressed all four findings from the initial review in commit 06370d3. Here's my assessment of each fix and any new observations.

Verification of the Four Fixed Issues

✅ lastHeard: 0 merge gap — Fixed correctly

MeshCoreNodesView.tsx:96:

existing.lastHeard = (existing.lastHeard || null) ?? meshcoreLastHeardMs(c) ?? undefined;

The fix correctly aligns mergeNodesAndContacts with meshcoreLastHeardMs's own treatment of 0 as absent. The inline comment referencing the PR number is a good trace for future readers. The regression test at MeshCoreNodesView.test.tsx:331 is thorough: a node with lastHeard: 0 in the DB whose contact has a 2-hour-old lastSeen now resolves correctly and survives the filter.

✅ parseInt → NaN draft state — Fixed correctly

MeshCoreNodeDisplaySection.tsx:97-103:

const update = (key: typeof MESHCORE_NODE_DISPLAY_KEYS[number], value: number) => {
  setDraft((prev) => ({ ...prev, [key]: Number.isNaN(value) ? prev[key] : value }));
};

The guard is applied at the single update() setter rather than duplicated at four call sites — the right level of abstraction. Test (6b) at MeshCoreNodeDisplaySection.test.tsx:226 covers both the UI symptom (input still shows last valid value) and the serialization outcome (no "NaN" in POST body), which is exactly the right test structure.

✅ (local) naming convention — Documented, not changed

Both match sites in MeshCoreNodesView.tsx now carry NOTE: comments (lines 286-292 and 319-321) explaining the server-side origin of the convention, the caveat for user-chosen names, and why the fix belongs in a separate change. This is the right call — documenting the boundary rather than creating scope creep in a PR that was already complete.

✅ i18n keys — Fixed and then some

All three borrowed automation.* keys (including the catch-block settings_save_failed that wasn't flagged) were replaced with proper settings.node_display.* keys. The en.json entries (settings.node_display.insufficient_permissions, settings.node_display.settings_saved, settings.node_display.settings_save_failed) are present and correctly mapped. Catching the third key the review missed is exactly the kind of attention to detail that makes patches like this trustworthy.

New Observations

No regressions introduced. The six test files owned by no work package in the PR description are plausibly unaffected — the changes are additive (new component + test) or narrowly scoped (merge logic fix, NaN guard, i18n key rename).

MESHCORE_NODE_DISPLAY_KEYS → satisfies binding is clean. The compile-time guard at MeshCoreNodeDisplaySection.tsx:38 means a rename or deletion in nodeDisplayDefaults.ts breaks the build here rather than silently drifting. This is the right pattern for a constant that drives both UI and POST body shape.

nodeDisplaySettingsQueryKey export is correctly shared. The section invalidates the exact same cache key the Nodes view and map read from (['settings', 'node-display', sourceId]), guaranteed by the shared export rather than a duplicated string literal. This is what makes spec requirement R3 (cache invalidation propagates without reload) structurally enforced rather than test-only.

SaveBarProvider wrapping is correct. MeshCorePage.tsx:202-214 wraps MeshCoreSettingsView (which contains the new section) in <SaveBarProvider>/<SaveBarGroup>/<SaveBar>, so the save bar lifecycle works without a separate registration step in the section itself.

Minor: visibleContacts local-node fallback at MeshCoreNodesView.tsx:324 still uses advName?.includes('(local)') without a corresponding NOTE: comment, while isAgeExempt above it now does. Given that both the NOTE: comments in the PR explicitly call out visibleContacts as sharing the same convention, this is worth a one-line comment for consistency — but it's a cosmetic nit, not a functional gap.

Summary

All four previously identified issues are resolved correctly. The fixes are precise, each is covered by a new test that targets the specific failure mode, and one finding was extended to catch a related inconsistency the initial review missed. The implementation is in solid shape for merge.