feat(settings): read Node Display settings per-source (#4412 Phase 2)

[Yeraze]

Phase 2 of the per-source Node Display epic (#4412). Converts every backend read of the 10 Node Display keys from global to per-source. Frontend is Phase 3; MeshCore filtering is Phase 4.

Plan: docs/internal/dev-notes/PER_SOURCE_NODE_DISPLAY_EPIC.md
Spec: docs/internal/dev-notes/PER_SOURCE_NODE_DISPLAY_PHASE2_SPEC.md
Builds on Phase 1 (#4417), which seeded source:{id}:{key} for every existing source via migration 131.

⚠️ User-visible behavior change — release notes

GET /api/neighbor-info read the settings key 'maxNodeAge', which has never existed in VALID_SETTINGS_KEYS. It always resolved null and always fell back to 24 hours. The endpoint has been silently pinned at a 24-hour window forever. It now honors the configured maxNodeAgeHours per source, so anyone who tuned that setting will see their neighbor-info link set change on upgrade.

What changed

New shared foundation. There is no runtime global fallback (#2839/#2840), so an unset source falls through to a hardcoded default — which meant 24 was about to scatter across a dozen read sites and rot independently.

• src/constants/nodeDisplayDefaults.ts — the values. Isomorphic and import-free so Phase 3 can consume the same runtime values on the frontend (src/server/constants/settings.ts was rejected: the frontend imports only type from src/server/**). Also carries NODE_DISPLAY_RANGES, ending the duplicated validation literals between settingsRoutes.ts and server.ts.
• src/server/services/nodeDisplaySettings.ts — the accessor, via reader injection copying the existing ManagerSettingsDb pattern. It must not import databaseService; that graph imports into this one.
• SettingsRepository.getSettingForSources(ids, key) — one indexed IN() lookup. The unifiedRoutes dashboard fan-out reads once and broadcasts to every source; converting it naively would have made a hot path N+1. Deliberately not built on getSourceSettings, which is a full-table scan ([BUG] Per-source settings POST compares against global current-values; getSourceSettings does a full-table scan #4419).

~15 read sites converted, including three in database.ts (getNodeNeedingTracerouteAsync, getNodesNeedingRemoteLocalStatsAsync, getNodeNeedingRemoteAdminCheckAsync) that were reading the global value despite already having sourceId threaded through from every caller. Verified: zero global reads of any of the 10 keys remain outside tests and migrations.

Inactive-node service restructured to one timer with per-source config — not timer-per-source, which was rejected for lifecycle coupling. Per-source nextRunAt, due-ness computed before the users query, nextRunAt set before the work so a throw can't hot-loop, an explicit running flag (getStatus() previously derived it from the timer handle and would report false during the 60s warm-up), and pruning on deregistration. restartInactiveNodeService(t,c,cd) → rescheduleInactiveNodeService(sourceId?).

Two tests were pinning bugs rather than catching them

Both are why this phase found what it did:

• server.neighbor-info-position.test.ts re-implemented the neighbor-info route body inside the test — including the same dead 'maxNodeAge' key. It could not catch the bug because it copied it. Deleted; real-router coverage now lives in neighborInfoRoutes.test.ts (converted to createRouteTestApp()) and the existing nodeEnhancer.position-override.test.ts. This is the anti-pattern CLAUDE.md names: a fake that re-implements the logic under test cannot catch regressions in that logic.
• inactiveNodeNotificationService.test.ts asserted "no eligible users does not advance nextRunAt — the source is retried on the very next tick", pinning the regression below as intended behavior.

Regression caught in review, fixed in 707a6d87

The restructured scheduler's zero-users branch left every due source permanently due, so getUsersWithInactiveNodeNotifications() and the zero-eligible diagnostic ran on every 60s tick instead of at each source's configured interval — a 60× increase, and zero-users is the default state on a fresh install. Same failure class as #4399/#4413.

The reasoning in the original comment ("nothing was checked, so retry") does not hold: "no users have notifications enabled" is a global condition, not per-source, and cannot become true faster because you re-queried 59 seconds later.

The entire suite passed with this bug present — nothing tested scheduler cadence. The fix ships with a fake-timer test asserting one query across five ticks.

Verification

• Full suite: 12,506 passing, 0 failing, success: true via JSON reporter. 109 skips, unchanged from the Phase 1 baseline, so nothing silently stopped running.
• PostgreSQL (5433) and MySQL (3307) containers live for the run.
• tsc --noEmit clean. lint:ci ratchet clean.
• New *.perSource.test.ts files prove two sources with different maxNodeAgeHours get different results, and that two sources with different check intervals each fire on their own schedule.

Still not fixed

Per-source permission scoping (#4416) remains open — a source-A-scoped settings:write grant still authorizes source-B writes. Do not read this PR as "per-source settings are now secure."

Refs #4412

🤖 Generated with Claude Code

https://claude.ai/code/session_01L9NzRtqE8eSMS8tvAeodUB

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: feat(settings): read Node Display settings per-source (#4412 Phase 2)

• Gather context and understand PR scope
• Review new foundation files (nodeDisplayDefaults.ts, nodeDisplaySettings.ts)
• Review repository changes (settings.ts)
• Review inactiveNodeNotificationService.ts refactor
• Review route changes (neighbor-info, traceroute, unified, settings)
• Review test coverage
• Post final review

---

This is a well-engineered, thoroughly documented phase. The PR body is unusually transparent about its own bugs (the scheduler hot-loop regression, the pinned test anti-pattern) and shows genuine ownership of code quality. The observations below are mostly minor; none are blockers given the stated Phase 3/4 scope.

Architecture & Design

Excellent decisions:

• nodeDisplayDefaults.ts being isomorphic (zero imports, lives in src/constants/) is the right call for Phase 3 frontend reuse. The precedent citation (hopEmoji.ts) is appropriate.
• The NodeDisplayBatchReader interface being separate from NodeDisplaySettingsReader keeps the batched IN() path opt-in. Only the hot unified-dashboard fan-out needs it; other callers aren't forced to implement it.
• The "no global fallback" decision is correctly applied throughout: getSettingForSources() leaves absent ids out of its result, and getMaxNodeAgeHoursForSources() fills them from hardcoded defaults rather than a second query.
• The getInactiveNodeConfig() using Promise.all for the three independent reads is the right pattern here.

Minor design observation:

tracerouteRoutes.ts:22 — GET /traceroutes/recent computes maxNodeAgeHours per-source via getMaxNodeAgeHours(databaseService.settings, recentSourceId ?? null). When recentSourceId is null/undefined (no ?sourceId filter), this reads the bare maxNodeAgeHours key (the legacy global row). That is technically correct per the "no global fallback" spec — getSettingForSource(null, key) reads the un-namespaced key. But it means a global save to maxNodeAgeHours (from the global settings tab) still gates the cross-source /traceroutes/recent? response, while per-source saves don't. This is a known Phase 3/4 boundary concern and not a bug in this PR — just worth flagging as something to revisit when the frontend is updated.

Code Quality

settingsRoutes.ts:953-958 (global save path, inactive-node keys):

if (INACTIVE_NODE_KEYS.some((key) => key in filteredSettings)) {
  callbacks.rescheduleInactiveNodeService?.(null);
  logger.debug('✅ Inactive node check rescheduled for all sources');
}

The comment correctly explains why null (= all sources) is right here: global saves of these keys must invalidate every source's nextRunAt. The approach is sound.

settingsRoutes.ts:1034 (global save success response):

res.json({ success: true, settings: filteredSettings });

The per-source branch at line 825 uses ok(res, { ignoredKeys }) (the shared envelope helper), but this global branch hand-rolls the response. The CLAUDE.md response envelope notes ok() should be used for handlers but also notes this is an "opportunistic" conversion — no issue here, just noting the asymmetry.

inactiveNodeNotificationService.ts:141-144 (pruning nextRunAt while iterating):

for (const id of this.nextRunAt.keys()) {
  if (!activeIds.has(id)) this.nextRunAt.delete(id);
}

Iterating a Map while deleting from it is safe in JavaScript (the spec guarantees the iterator yields entries that existed when the iteration began, deleted entries are skipped). This is correct, just worth noting for readers unfamiliar with the JS Map spec.

getSettingForSources — empty sourceIds with duplicates:
The method deduplicates input via new Set(sourceIds). This is good hygiene, but the caller getMaxNodeAgeHoursForSources doesn't deduplicate its sourceIds input first. The result Map is keyed by sourceId, so duplicates in the input produce correct values — the database never sees duplicates, and the result Map is iterated over the original sourceIds array. Safe, but slightly asymmetric (the dedup is at the repo layer, not the caller).

Bug / Potential Issues

parseNodeDisplayNumber and nodeDimmingStartHours/nodeDimmingMinOpacity (no range):

src/constants/nodeDisplayDefaults.ts:100-105 documents that keys without a NODE_DISPLAY_RANGES entry are parsed but not bounds-checked, and parseNodeDisplayNumber('nodeDimmingStartHours', '-5') returns -5. This is intentional for now (Phase 3 adds ranges from the SettingsTab inputs). The test at nodeDisplayDefaults.test.ts:103-104 pins this behavior explicitly, which is good.

inactiveNodeNotificationService.ts:143-144 (map cleanup and newly deregistered source):

After pruning stale nextRunAt entries, if a source was deregistered mid-tick (after managers was captured but before the pruning loop), it could appear in due even after the prune — since due was computed from the captured managers slice before pruning. In practice this is a non-issue (deregistration and tick run on the same JS event loop, so there's no true race), but it's a subtle ordering dependency worth a short comment.

Performance

sourceDashboardData.ts:231 — buildSourceNeighborInfo resolves maxNodeAgeHours per-source when maxNodeAgeHours is not provided:

const resolvedMaxAge = maxNodeAgeHours ?? await getMaxNodeAgeHours(databaseService.settings, source.id);

When called from buildSourceDashboard via the per-source /api/sources/:id/dashboard route (not the unified route), this triggers one getSetting DB query per dashboard call. The unified route correctly batches this with getMaxNodeAgeHoursForSources. The per-source single-source path was already one-query-per-source before this PR (it was a global read), so the performance is unchanged — just noted for Phase 3 awareness.

unifiedRoutes.ts:1300-1303 — The batched getMaxNodeAgeHoursForSources call correctly uses a single IN() query for the entire source selection. Well done.

Test Coverage

Strengths:

• nodeDisplayDefaults.test.ts pins every invariant that matters: key count, no duplicates, subset-of-VALID_SETTINGS_KEYS, byte-identity with migration 131's seed, '0'/'1' boolean convention, parseNodeDisplayNumber edge cases (0-below-min, out-of-range, unranged keys), parseNodeDisplayBoolean edge cases.
• nodeDisplaySettings.test.ts isolates the accessor service with a stub reader and explicitly tests the "no global fallback" invariant (a global row present, no per-source override → hardcoded default). This is the exact regression being prevented.
• inactiveNodeNotificationService.perSource.test.ts is exemplary: fake timers, independent cadences between two sources, independent cooldowns, and bounded-map growth on deregistration — four distinct scheduling behaviors, each in its own it block.
• The zero-users scheduler fix test (advancing five ticks with getUsersWithInactiveNodeNotifications called exactly once per tick, not five times) directly pins the regression caught in review.

One gap — getSettingForSources with null value from DB:

settings.test.ts tests the batched repo method, but it would be useful to also assert what happens when a row has a NULL value (i.e., the key exists in the table but value is null). The type signature Promise<Map<string, string>> suggests values are always non-null strings, but the DB select({ value: settings.value }) could return null for a null-value row. The cast at line 238 (as Array<{ key: string; value: string }>) would silently pass null through as a string to the caller. This is an edge case that's unlikely in practice (migration 131 seeds string values) but worth a defensive cast or note.

Security

• The existing SSRF protections in settingsRoutes.ts (Apprise probe URL blocklist, IMDS range check) are unchanged and correct.
• The auditSettingsWrite function now covers both per-source and global saves — the PR body explicitly calls out this was a bug ([FEAT] Meschore - last advert time filtering #4412 bug feat: telemetry parsing, direct messages, danger zone, and UX improvements #6). Correct.
• The setSourceSettings key validation (/^[A-Za-z0-9_.-]+$/.test(k)) is still present and correct.
• Note: The PR correctly calls out that per-source permission scoping ([BUG] Two divergent SOURCEY_RESOURCES lists — per-source permission scoping silently ignored for settings, dashboard, info, audit, security #4416) is still open. This is not a regression introduced here, just a reminder that a settings:write grant on source A can still write to source B's namespace.

Minor Style

• src/server/routes/settingsRoutes.ts:255 — GET /settings error at line 255 hand-rolls res.status(500).json({ error: '...' }) while some surrounding handlers use fail(). The opportunistic-conversion note in CLAUDE.md covers this.
• The settingsRoutes.ts file is now 1,835 lines. This is pre-existing, but Phase 3's frontend changes may be a natural point to consider splitting the Node Display section into its own sub-router if it grows further.

Summary

The PR achieves its stated goal cleanly: zero global reads of the 10 Node Display keys remain in backend business logic, the scheduler regression is caught and fixed with a pinning test, and two test files that were masking bugs rather than catching them are correctly replaced. The nodeDisplayDefaults.ts / nodeDisplaySettings.ts foundation is a solid, reusable pattern for Phase 3.

Verdict: Ready to merge with no blocking issues. The observations above are informational for Phase 3/4 planning.

[Yeraze]

Thanks — review triaged. One suggestion applied, one answered with evidence, the rest noted for Phase 3.

Applied

tick() prune/due ordering dependency. Agreed it deserved a comment rather than being left implicit. Added one stating that the prune and the due filter both read the same captured managers slice, that a source deregistered after the snapshot gets one final pass and is pruned next tick, and that re-reading the registry between the two would let them disagree. Comment-only, no behavior change.

Answered — getSettingForSources cannot receive a NULL value

Good instinct, but the case is impossible at the schema level: value is .notNull() on all three backends (src/db/schema/settings.ts:12, :20, :28). There is no way for the table to hold a null-value row, so the cast at settings.ts:238 cannot smuggle null through as a string.

I'd rather not add a defensive branch for a state the schema forbids — it would be unreachable code implying the invariant is weaker than it is. If the NOT NULL constraint were ever relaxed, that is the change that should carry the null handling.

Noted, no action

• Unranged nodeDimming* keys — intentional, and the behavior is pinned by test at nodeDisplaySettings/nodeDisplayDefaults. Phase 3 adds the ranges from the SettingsTab inputs, which is where the authoritative bounds live.
• buildSourceNeighborInfo single-source query — as you note, unchanged from before this PR (it was one global read, now one scoped read). The fan-out path is the one that mattered and it is batched.
• fail() at settingsRoutes.ts:255 — opportunistic conversion per CLAUDE.md; converting untouched handlers would widen this diff without improving it.
• settingsRoutes.ts at 1,835 lines — agreed, and Phase 3 is the natural moment to look at it. Logged as a Phase 3 consideration rather than done here, since splitting a router under a behavior change makes both harder to review.

Reminder carried forward

Reiterating your security note so it does not get lost: per-source permission scoping (#4416) is still open. A settings:write grant on source A can still write source B's namespace. This PR does not change that, and the PR body says so.

[github-actions]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: feat(settings): read Node Display settings per-source (#4412 Phase 2)

• Gather context and understand PR scope
• Review new foundation files (nodeDisplayDefaults.ts, nodeDisplaySettings.ts)
• Review repository changes (settings.ts)
• Review inactiveNodeNotificationService.ts refactor
• Review route changes (neighbor-info, traceroute, unified, settings)
• Review test coverage
• Post final review

---

This is a substantial, well-architected phase. The PR body is unusually self-aware about the bugs it found and fixed, and the review history shows the author engaged with all prior feedback constructively. My observations below are mostly confirmatory, with a few new findings.

Architecture & Design

Strong decisions:

• nodeDisplayDefaults.ts being zero-import / isomorphic is the correct call for Phase 3 reuse. The hopEmoji.ts precedent cited in the file header is apt.
• The NodeDisplayBatchReader interface being additive over NodeDisplaySettingsReader (not folded into one) is correct — only the hot unifiedRoutes dashboard fan-out needs batching, and the interface boundary makes that opt-in explicit.
• The getSettingForSources() reverse-map approach (prefixed key → sourceId rather than string-splitting) at settings.ts:227-229 is correct and defensively documented: a : in a sourceId would corrupt a split. Good.
• tick() comment at inactiveNodeNotificationService.ts:141-148 correctly documents the ordering dependency between the prune step and the due filter, and why re-reading the registry in between would break them. This directly addresses the previous review's observation.

Code Quality

neighborInfoRoutes.ts:19-22 — silent null→undefined coercion:

const maxNodeAgeHours = await getMaxNodeAgeHours(
  databaseService.settings,
  neighborInfoSourceId ?? null,
);

neighborInfoSourceId is typed string | undefined; ?? null converts undefined → null. getMaxNodeAgeHours then calls getSettingForSource(null, key) which reads the un-namespaced global row. This is correct per the spec, but the type signature mismatch between the query param (string | undefined) and getSettingForSource's string | null | undefined means the ?? null is load-bearing — if it were removed, the behavior would be identical but the intent less clear. A comment on the ?? null would help the next reader understand this is not a defensive no-op.

tracerouteRoutes.ts:21 — getSetting without async wrapper:

const tracerouteIntervalMinutes = parseInt(
  await databaseService.settings.getSetting('tracerouteIntervalMinutes') || '5'
);

getSetting returns Promise<string | null>. The || '5' fallback is applied to the resolved value — correct. But this line reads the global, un-namespaced tracerouteIntervalMinutes even when recentSourceId is defined. This was pre-existing behavior (not introduced by this PR) and is mentioned in the previous review as a Phase 3/4 concern, so no action required now — just confirming it's known.

inactiveNodeNotificationService.ts:143-151 — per-due-source config reads are serial:

for (const manager of due) {
  try {
    const cfg = await getInactiveNodeConfig(databaseService.settings, manager.sourceId);
    this.nextRunAt.set(manager.sourceId, now + cfg.checkIntervalMinutes * 60_000);
    // ... work ...
  }
}

Inside the zero-users branch, each due source's config is fetched with a separate await inside the for-loop. With N due sources this is N×3 sequential DB queries (three reads per getInactiveNodeConfig). The same pattern exists in the non-zero branch. Since the minimum check interval is 1 minute and most deployments will have few sources, this is not a practical problem — but a Promise.all(due.map(...)) for the config reads would be safer for large deployments. Not blocking; noting for Phase 3.

settingsRoutes.ts:1034 — global save response inconsistency:

res.json({ success: true, settings: filteredSettings });

The per-source save at line 825 uses ok(res, { ignoredKeys }). The global path at 1034 hand-rolls { success: true, settings }. As noted in the prior review, this is an opportunistic-conversion concern per CLAUDE.md — not blocking, but the asymmetry remains.

Potential Bugs

parseNodeDisplayNumber — integer: true in NODE_DISPLAY_RANGES is unused:

src/constants/nodeDisplayDefaults.ts:87-94 defines ranges with an integer field:

maxNodeAgeHours: { min: 1, max: 168, integer: true },

But parseNodeDisplayNumber never reads range.integer (line 116: only n < range.min || n > range.max). So parseNodeDisplayNumber('maxNodeAgeHours', '1.5') returns 1.5, not clamped to default and not truncated to 1. The field is presumably intended for Phase 3 UI validation. This is intentional and not a bug in this PR — but the integer field is dead code today and may cause confusion. A brief comment noting it's Phase 3's responsibility would help.

getSettingForSources — inArray with a large sourceId list:

At settings.ts:236: inArray(settings.key, [...prefixedToSourceId.keys()]). SQLite's IN (...) has a default maximum of 999 parameters (compile-time SQLITE_LIMIT_VARIABLE_NUMBER, default 999 in older builds, 32766 in newer). With N sources each needing one prefixed key, a deployment with >999 sources would fail silently. In practice no deployment will have >999 sources, but this limit is worth a code comment if the method is later extended to multi-key batching.

Performance

unifiedRoutes.ts:1300-1303 — getMaxNodeAgeHoursForSources correctly batches into a single IN() for the dashboard fan-out. This was the hot-path concern and it's handled correctly.

getInactiveNodeConfig at zero-users branch — three DB reads per due source, sequential. As noted above, acceptable for current scale.

Security

• setSourceSettings key validation (/^[A-Za-z0-9_.-]+$/.test(k)) at settings.ts:263 is unchanged and correct.
• The auditSettingsWrite at both the per-source (line 824) and global (line 1027) paths means every settings write is audited — the PR body calls this out correctly.
• Per-source permission scoping ([BUG] Two divergent SOURCEY_RESOURCES lists — per-source permission scoping silently ignored for settings, dashboard, info, audit, security #4416) is still open. A settings:write grant on source A can still write source B's namespace. The PR correctly documents this and does not regress it.

Test Coverage

Strengths (directly observed):

• nodeDisplayDefaults.test.ts is thorough: key count, no duplicates, subset-of-valid-keys, byte-identity with migration 131 seed, '0'/'1' convention, parseNodeDisplayNumber edge cases (0-below-min, out-of-range, unranged keys), parseNodeDisplayBoolean round-trips.
• nodeDisplaySettings.test.ts pins the "no global fallback" invariant with explicit counter-example: global row present + no per-source override → hardcoded default, not the global row. This is the regression being prevented.
• settings.test.ts adds six new getSettingForSources tests including one that monkey-patches db.select to count actual round-trips — the "issues exactly one SELECT" assertion is unusually rigorous.
• inactiveNodeNotificationService.perSource.test.ts uses fake timers to prove four independent scheduling behaviors: single timer, independent cadences, independent cooldowns, and bounded-map growth on deregistration.
• database.maxNodeAge.perSource.test.ts is an integration test against a real SQLite instance (not just mocks), proving the full stack from settings write through DB query to actual node selection.

One minor gap — zero-users scheduler fix test:

inactiveNodeNotificationService.test.ts is supposed to contain the "one query per tick, five ticks" test for the zero-users hot-loop regression (per the PR body). I didn't read that file in full. If that test is there, the coverage is complete. The perSource test file does not cover the zero-users scheduler cadence directly — it relies on having at least one eligible user (mocked makeRow).

nodeDimmingStartHours/nodeDimmingMinOpacity — unranged behavior is pinned:

nodeDisplayDefaults.test.ts:100-109 explicitly pins that unranged keys accept out-of-range values including negatives. Good — this is the intentional behavior, and pinning it prevents an accidental future range addition from silently breaking stored configs.

Summary

The PR is well-executed. The three main goals — zero global reads of the 10 Node Display keys, zero-users scheduler hot-loop fixed, and two bug-masking tests replaced — are all achieved. The nodeDisplayDefaults.ts / nodeDisplaySettings.ts foundation is reusable for Phase 3 without modification. The batch read path for the unified dashboard is correct and efficient.

The items above are informational for Phase 3/4 planning. No blocking issues found.

Verdict: Ready to merge.