fix(config): store local-node module config responses (handler was remote-only) by Yeraze · Pull Request #3460 · Yeraze/meshmonitor

Yeraze · 2026-06-14T18:00:25Z

Summary

The getModuleConfigResponse handler in processAdminMessage (src/server/meshtasticManager.ts) was entirely wrapped in if (isRemoteNode). For the local node, the response was logged and then discarded — it never merged into this.actualModuleConfig. Only the initial wantConfig download stream ever populated the local node's module config, so explicit refreshes (requestModuleConfig / requestAllModuleConfigs / refreshModuleConfigs) of the local node were effectively no-ops. The Proto3-empty fallback that records an all-default module under its key was also remote-only.

This is the contributing bug found while fixing #3457 (Traffic Management shown unsupported on firmware v2.7.24). That PR resolved the support-detection symptom via firmware-version gating; this PR makes explicit local module-config refreshes actually persist.

Changes

Added a local-node branch to the getModuleConfigResponse handler that merges the response into actualModuleConfig (preserving existing keys).
For an empty (all-default, Proto3-omitted) local response, records the module under the pending request's key — without clobbering an already-populated config.
requestModuleConfig() now tracks the pending key (keyed by the local node number) so the empty-response fallback can resolve it.
Added LOCAL_MODULE_CONFIG_TYPE_KEYS map (ModuleConfigType enum → decoded ModuleConfig oneof key).

Issues Resolved

Relates to #3457 (contributing bug; not user-visible for support detection after #3457).

Documentation Updates

No documentation changes needed — internal config-sync logic.

Testing

Unit tests pass (full suite: 7001 tests, 0 failures)
TypeScript compiles cleanly (tsc --noEmit)
New meshtasticManager.localModuleConfig.test.ts (6 cases): non-empty merge preserves existing keys; empty response records pending key; from: 0 local-origin case; no-clobber of a populated config; remote responses don't leak into local config; requestModuleConfig pending-key tracking
Manual: on a local v2.7.22+ device, trigger a config refresh and confirm an all-default Traffic Management / StatusMessage module persists in actualModuleConfig

Note: the parallel getConfigResponse (device config) handler is also remote-only; left out of scope here to keep this focused on module config.

🤖 Generated with Claude Code

…mote-only) The getModuleConfigResponse handler in processAdminMessage was entirely wrapped in `if (isRemoteNode)`, so explicit local module-config refreshes (requestModuleConfig / requestAllModuleConfigs / refreshModuleConfigs) were logged and then discarded — only the initial wantConfig download stream ever populated the local node's actualModuleConfig. The Proto3-empty fallback that records an all-default module under its key was also remote-only. Add a local-node branch that merges the response into actualModuleConfig and, for an empty (all-default, Proto3-omitted) response, records the module under the pending request's key without clobbering an already-populated config. requestModuleConfig now tracks that pending key (via LOCAL_MODULE_CONFIG_TYPE_KEYS, keyed by the local node number) so the fallback can resolve it. Discovered while fixing #3457 (Traffic Management shown unsupported on 2.7.24). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

github-actions · 2026-06-14T18:00:44Z

Claude finished @Yeraze's task —— View job

Reviewing PR #3460

I've thoroughly reviewed this pull request that fixes the local module config response handling bug. Here's my comprehensive analysis:

Todo List:

Read and analyze the modified meshtasticManager.ts file
Review the new test file for coverage adequacy
Examine related codebase context for module config handling
Provide comprehensive review feedback

🟢 Overall Assessment: APPROVED

This is a well-implemented bug fix with excellent test coverage. The changes are focused, safe, and address a real functional issue.

✅ Code Quality & Best Practices

Excellent implementation following established patterns:

Consistent with existing code style: The local-node handling mirrors the existing remote-node logic structure
Proper logging: Added appropriate debug/info logs matching the project's logging patterns
Defensive programming: Proper null checks and initialization of actualModuleConfig
Clear comments: The added comments clearly explain the bug and solution

Code organization:

The LOCAL_MODULE_CONFIG_TYPE_KEYS constant is well-placed and properly documented
Logic is cleanly separated between remote and local handling paths

✅ Bug Analysis & Correctness

Problem correctly identified and fixed:

Root cause: Local node responses were being logged but discarded due to if (isRemoteNode) wrapper
Solution: Added parallel local-node branch that merges responses into actualModuleConfig
Edge case handled: Empty Proto3 responses (all-default configs) are properly recorded using pending request tracking

Implementation correctness:

src/server/meshtasticManager.ts:11751-11784 - The local node handling correctly:
- Initializes actualModuleConfig if null
- Filters out payloadVariant and undefined fields (consistent with remote logic)
- Merges rather than replaces existing config (preserves other modules)
- Handles empty responses via pending request tracking
- Uses proper fallback for from: 0 packets

✅ Performance Considerations

No performance concerns:

Changes only affect the response handling path (not hot path)
No additional loops or expensive operations
Memory usage unchanged (same data structures, just different storage location)
Pending request tracking adds minimal overhead (Map operations)

✅ Security Analysis

No security issues identified:

No user input processing or validation changes
Configuration data handling remains the same
No new attack vectors introduced
Follows existing patterns for config storage

✅ Test Coverage

Comprehensive test suite (meshtasticManager.localModuleConfig.test.ts):

Non-empty response merging - Verifies new config merges with existing without replacement
Empty response handling - Tests Proto3 default handling via pending request tracking
Edge case: from: 0 - Handles local-origin packets correctly
No-clobber protection - Prevents overwriting populated configs with empty defaults
Remote isolation - Ensures remote responses don't leak into local config
Request tracking - Verifies requestModuleConfig properly sets pending keys

Test quality:

Good use of mocking to isolate the unit under test
Covers both success and edge case scenarios
Tests the actual bug scenario (empty responses being stored)

🟡 Minor Suggestions

Line 11771-11772: The dual fallback lookup (this.pendingModuleConfigRequests.get(localNodeKey) ?? this.pendingModuleConfigRequests.get(fromNum)) is clever but could use a brief comment explaining when each case applies.
Testing: Consider adding a test case for the scenario where localNodeInfo is null (though this may not be possible in practice).

✅ Documentation & Context

Well-documented change:

Clear PR description explaining the bug and fix
Relates to issue fix(config): gate Traffic Management/StatusMessage support on firmware version #3457 appropriately
Inline comments explain the logic clearly
No external documentation needed for internal fix

🎯 Issues Resolved

This fix resolves the core issue where:

requestModuleConfig(), requestAllModuleConfigs(), and refreshModuleConfigs() were no-ops for local nodes
Only the initial wantConfig stream populated local module config
Module support detection could fail due to missing configs

Recommendation: MERGE ✅

This is a solid, well-tested bug fix that:

Addresses a real functional issue
Follows established code patterns
Has comprehensive test coverage
Introduces no new risks
Will improve the reliability of module config refresh operations

Job Run

Bump version to 4.10.3 across package.json, package-lock.json, helm/meshmonitor/Chart.yaml, desktop/package.json, and desktop/src-tauri/tauri.conf.json. Finalize the CHANGELOG: promote [Unreleased] to [4.10.3] and add the entries merged after the section was last written — MQTT NodeInfo clobber fix (#3456), Traffic Management/StatusMessage firmware-version gate (#3457), local-node module config storage (#3460), phantom channel swap fix (#3453), and the TX power help-text clarification (#3459). Add a release blog post highlighting the Traffic Management detection fix and the bug-fix round. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Yeraze merged commit 304ffbc into main Jun 14, 2026
19 checks passed

Yeraze mentioned this pull request Jun 14, 2026

release: v4.10.3 #3461

Merged

Yeraze deleted the fix/local-module-config-response branch June 14, 2026 20:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(config): store local-node module config responses (handler was remote-only)#3460

fix(config): store local-node module config responses (handler was remote-only)#3460
Yeraze merged 1 commit into
mainfrom
fix/local-module-config-response

Yeraze commented Jun 14, 2026

Uh oh!

github-actions Bot commented Jun 14, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

Yeraze commented Jun 14, 2026

Summary

Changes

Issues Resolved

Documentation Updates

Testing

Uh oh!

github-actions Bot commented Jun 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewing PR #3460

Todo List:

🟢 Overall Assessment: APPROVED

✅ Code Quality & Best Practices

✅ Bug Analysis & Correctness

✅ Performance Considerations

✅ Security Analysis

✅ Test Coverage

🟡 Minor Suggestions

✅ Documentation & Context

🎯 Issues Resolved

Recommendation: MERGE ✅

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

github-actions Bot commented Jun 14, 2026 •

edited

Loading