Skip to content

fix(auto-favorite): scope status fetch to active source (#2826)#2828

Merged
Yeraze merged 1 commit intomainfrom
fix/auto-favorite-respect-active-source
Apr 27, 2026
Merged

fix(auto-favorite): scope status fetch to active source (#2826)#2828
Yeraze merged 1 commit intomainfrom
fix/auto-favorite-respect-active-source

Conversation

@Yeraze
Copy link
Copy Markdown
Owner

@Yeraze Yeraze commented Apr 27, 2026

Summary

Fixes #2826 — Auto Favorite always read status (role/firmware/managed nodes) from the first registered source, even after switching to a different one. Multi-source users with one Client Base + one Client Mute saw the false warning "Your node role is 'Client Mute' — Auto Favorite requires Router, Router Late, or Client Base" on the perfectly-configured Client Base source.

Root cause

AutoFavoriteSection.tsx fetched /api/auto-favorite/status without the sourceQuery fragment. The backend route at server.ts:1345 falls back to the legacy meshtasticManager singleton when no ?sourceId= is supplied — and that singleton is whichever source was registered first at startup. The settings fetch in the same component already passed sourceQuery correctly; the status fetch was an oversight.

Fix

  • Append sourceQuery to the /api/auto-favorite/status URL.
  • Add sourceQuery to the fetchData useCallback deps so switching sources actually re-fetches.

Test plan

  • Two new tests in AutoFavoriteSection.test.tsx assert the URL carries the active sourceId, and that the no-source legacy path still works.
  • npx vitest run src/components/AutoFavoriteSection.test.tsx — 6/6 pass.
  • Manual repro per the issue: two sources (client_base + client_mute), select the client_base source, confirm warning is gone.

🤖 Generated with Claude Code

The Auto Favorite UI fetched `/api/auto-favorite/status` without the
active sourceId, so the backend fell through to the legacy singleton
manager — which is whichever source was registered first. Users with
multiple sources saw the role/firmware/managed-nodes status of the
*first* source even after switching away from it, producing false
"Your node role is X — Auto Favorite requires …" warnings on
correctly-configured sources.

- Append `sourceQuery` to the status fetch URL so the backend resolves
  the manager via `sourceManagerRegistry.getManager(sourceId)`.
- Add `sourceQuery` to the `fetchData` useCallback deps so switching
  sources actually re-fetches.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 27, 2026

Claude finished @Yeraze's task —— View job


PR Review Complete ✅

Reviewing pull request #2828: fix(auto-favorite): scope status fetch to active source

Todo List:

  • Read and analyze the changed files
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security concerns
  • Review test coverage
  • Provide comprehensive feedback

Code Review Findings

Code Quality & Best Practices

  • Clean implementation: The fix is minimal and surgical - only adds sourceQuery to the status fetch URL and dependency array (AutoFavoriteSection.tsx:49, 65)
  • Consistent patterns: The fix follows the existing pattern already used for settings fetch
  • Proper dependency management: sourceQuery correctly added to useCallback dependencies to trigger re-fetch when source changes
  • Good separation of concerns: Logic properly contained within the existing component structure

Bug Fix Analysis

  • Root cause correctly identified: The issue was that /api/auto-favorite/status didn't include sourceQuery, causing fallback to legacy singleton
  • Proper solution: Adding ${sourceQuery} to line 49 ensures the correct source is queried
  • No regression risk: The change is backward compatible - when no source is active, sourceQuery will be empty

Performance Considerations

  • Minimal impact: Only adds a query parameter to an existing API call
  • Efficient re-fetching: sourceQuery in dependency array ensures status updates when switching sources without unnecessary calls
  • No additional network overhead: Uses existing parallel fetch pattern

Security Assessment

  • No security concerns: The sourceQuery comes from a trusted hook and follows existing patterns
  • Input validation: The backend handles sourceId validation as shown in the PR description
  • No injection risks: Query parameter construction is safe

Test Coverage

Excellent test additions:

  • Regression test: AutoFavoriteSection.test.tsx:108-121 specifically tests the fix by verifying sourceId is passed to the status endpoint
  • Edge case coverage: Lines 123-130 test the legacy behavior (no sourceId) to ensure backward compatibility
  • Clear test descriptions: Test names clearly explain the scenarios being verified
  • Proper mocking: Uses appropriate mocks and waitFor patterns for async behavior

Recommendations

Approval Recommended

This is a high-quality fix that:

  • Solves the reported issue with minimal, targeted changes
  • Includes comprehensive test coverage for both new and legacy behavior
  • Follows established code patterns and conventions
  • Has no security, performance, or breaking change concerns

Optional Enhancements (Not blocking)

  1. Additional error handling test: Consider adding a test for status API failure scenarios
  2. Documentation: The inline comments in the test file effectively document the regression being prevented

Summary

This PR effectively fixes issue #2826 with a clean, well-tested solution. The implementation correctly ensures auto-favorite status fetching respects the active source selection, preventing the confusing role warnings experienced by multi-source users. The regression tests will prevent this issue from reoccurring.

Recommended action: ✅ Approve and merge


@Yeraze Yeraze merged commit 816a1c9 into main Apr 27, 2026
19 checks passed
@Yeraze Yeraze deleted the fix/auto-favorite-respect-active-source branch April 27, 2026 20:37
@Yeraze Yeraze mentioned this pull request Apr 27, 2026
4 tasks
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.

[BUG] Autofavorite displays warning even when node is client_base.

1 participant