Claude finished @Yeraze's task —— View job

PR Review

• Gather context
• Review changed file
• Post feedback

Summary

This is a minimal, clean copy-only change. The description text on line 169 was updated from:

"Automatically favorite eligible nodes for zero-cost hop routing."

to:

"Automatically favorite eligible nodes for zero-cost hop and prioritized routing."

Code quality: The change is correct and well-scoped. The updated text is grammatically sound and accurately reflects both benefits of favoriting nodes in Meshtastic.

Potential issues: None. The i18n key (automation.auto_favorite.description) and fallback string are in sync. The "Read more" link and all other UI logic are untouched.

Test coverage: The PR notes that the existing test only checks the "Read more" link URL, not this description string. That's accurate — no test changes are needed for a UX copy tweak, though it would be reasonable to add an assertion for this string if test coverage of copy is desired in the future.

Performance / Security: No concerns — this is a pure string change.

Verdict: LGTM. The change is minimal, accurate, and low-risk.

Claude finished @Yeraze's task —— View job

PR Review

• Gather context
• Review changed files
• Post feedback

Summary

This PR is broader than the title suggests. Beyond the description copy tweak, it includes a substantive logic fix: CLIENT and CLIENT_MUTE nodes are now excluded from auto-favoriting, and a new CLIENT_BASE tip banner is added. All three changes are well-implemented.

src/server/constants/autoFavorite.ts

Logic fix — correct and important.

Before this PR, ZERO_HOP_RELAY_ROLES was apparently including CLIENT/CLIENT_MUTE (based on the issue). Now it correctly contains only ROUTER, ROUTER_LATE, and CLIENT_BASE (line 11–15). The JSDoc comment on isAutoFavoriteEligible (line 32) explicitly documents the rationale: "CLIENT and CLIENT_MUTE never relay, so they are never auto-favorited." This is accurate per Meshtastic protocol behavior.

One minor style note: AUTO_FAVORITE_LOCAL_ROLES and ZERO_HOP_RELAY_ROLES (lines 4–15) contain identical members. If they're intentionally kept separate for semantic clarity (local eligibility vs. relay capability), that's fine, but a comment noting why they differ-in-concept-but-not-in-value would help future maintainers avoid the temptation to deduplicate them incorrectly.

src/components/AutoFavoriteSection.tsx

New CLIENT_BASE tip banner (lines 238–254) — clean implementation, correctly conditioned on status.localNodeRole === DeviceRole.CLIENT_BASE. The sapphire color and informational tone distinguishes it well from the yellow warning banners.

Description text (line 166) — unchanged from the original ('Automatically favorite eligible nodes for zero-cost hop routing.'). The PR title and body describe updating this to mention "prioritized routing", but the file still shows the old string. This may be a commit ordering issue or a rebase artifact — worth verifying that the intended description update landed.

src/server/meshtasticManager.autoFavorite.test.ts

Good coverage of the new exclusions. The new tests at lines 22–27 directly cover the issue #3774 regression:

• returns false for 0-hop CLIENT when local is CLIENT_BASE
• returns false for 0-hop CLIENT_MUTE when local is CLIENT_BASE

The favoriteLocked behavior matrix (lines 86–123) is a useful documentation pattern, though those tests only assert constants rather than testing real behavior — this is acknowledged in the comment on line 87. These are fine as living documentation.

Issues / Questions

1. Description string not updated — AutoFavoriteSection.tsx line 166 still reads 'Automatically favorite eligible nodes for zero-cost hop routing.' without the "and prioritized routing" addition mentioned in the PR body. If this was intentional (e.g., the description change was dropped in favor of just the CLIENT_BASE tip), the PR body should be updated to reflect that.

2. sourceQuery missing from the lock call — Line 321 passes sourceId in the POST body for /api/nodes/:nodeId/favorite-lock, but other API calls in this component append sourceQuery as a query param (lines 48–50, 80). Verify the lock endpoint reads sourceId from the body rather than the query string; if it supports both, this is fine, but it's inconsistent with the rest of the component.

Verdict

The core logic fix (excluding CLIENT/CLIENT_MUTE) is correct and well-tested. The new CLIENT_BASE tip is a good UX addition. The only thing to double-check is whether the description string update (the nominal purpose of this PR) actually landed correctly.

Claude finished @Yeraze's task —— View job

PR Review

• Gather context
• Review changed files
• Post feedback

Summary

This PR correctly fixes issue #3774 by excluding CLIENT and CLIENT_MUTE nodes from auto-favoriting, adds a CLIENT_BASE tip banner, and simplifies getTargetDescription. The changes are clean, well-documented, and well-tested.

src/server/constants/autoFavorite.ts

Logic fix — correct. The ZERO_HOP_RELAY_ROLES check at line 57 now applies uniformly for all eligible local roles, eliminating the old CLIENT_BASE fallthrough to unconditional return true.

Good: The comment block at lines 3–8 explicitly explains why AUTO_FAVORITE_LOCAL_ROLES and ZERO_HOP_RELAY_ROLES are kept separate despite currently having identical members. This addresses the previous review's concern and is a proactive guard against incorrect future deduplication.

Minor nit: The JSDoc for isAutoFavoriteEligible on line 32 says "Local must be ROUTER, ROUTER_LATE, or CLIENT_BASE" — this information is also in AUTO_FAVORITE_LOCAL_ROLES. No action needed, just noting there are two sources of truth for this list; if a new local role is ever added to the set, the JSDoc will need a manual update.

src/components/AutoFavoriteSection.tsx

CLIENT_BASE tip banner (lines 238–254): Correctly conditioned on status.localNodeRole === DeviceRole.CLIENT_BASE and renders independently of firmwareValid/roleValid. One edge case worth noting: the banner appears even when the firmware check fails (firmwareValid === false). Since the feature won't work at all in that case, showing the CLIENT_BASE tip alongside the firmware warning may be slightly confusing. Consider wrapping the tip in {firmwareValid && status.localNodeRole === DeviceRole.CLIENT_BASE && ...} so it only shows when the setup is actually functional.

Description text (line 166): The PR body mentions updating the description to reference "prioritized routing", but the current text reads 'Automatically favorite eligible nodes for zero-cost hop routing.' — identical to before. This appears intentional based on the PR body's "Changes" section, which says it was "reverted to" this shorter text. The PR body and title should be read as describing the end state correctly — this is not a bug.

sourceId in favorite-lock (line 321): The lock call passes sourceId in the POST body (body: JSON.stringify({ locked: true, sourceId })), while other API calls in this component append sourceQuery as a URL query parameter (lines 48, 80). This is the same inconsistency flagged in the prior review. If the route handler reads sourceId from req.body, this works, but it diverges from the rest of the component's API call pattern. Worth verifying the server route handler for POST /api/nodes/:nodeId/favorite-lock reads sourceId from req.body (not req.query).

src/server/meshtasticManager.autoFavorite.test.ts

Good regression coverage. The new tests at lines 22–27 directly close issue #3774:

• CLIENT_BASE local + CLIENT target → false
• CLIENT_BASE local + CLIENT_MUTE target → false

The CLIENT_BASE + CLIENT_BASE → true case (line 34) is also a useful addition that wasn't in the prior suite.

favoriteLocked behavior matrix (lines 86–123): These tests only assert on plain object literals, not on any real function output. They serve as living documentation of expected behavior states, which is fine. No action needed, but a follow-up to convert them into integration-style tests would increase their actual regression value.

Issues

1. CLIENT_BASE tip shown during firmware error (AutoFavoriteSection.tsx:238): The tip renders even when firmwareValid === false. Recommend guarding with firmwareValid && to avoid showing an actionable tip when the feature can't run at all.

2. sourceId body vs. query inconsistency (AutoFavoriteSection.tsx:321): The lock endpoint receives sourceId via POST body while all other API calls in the component use sourceQuery. Please confirm the server route reads sourceId from the correct location.

Verdict

Core fix is correct and important. Test coverage for the regression is solid. The two items above are worth a quick look but neither is blocking.

Thanks for the review — addressing the three points:

1. Description string "not updated" — This is intentional, not a missed change. The PR was originally a copy tweak to "...zero-cost hop and prioritized routing", but per the issue [BUG] 'Auto Favorite automation for zero-cost hop routing#2018' feature favorites literally everybody #3774 discussion the scope changed: the feature now strictly covers zero-cost hop relay roles, so the description was deliberately reverted to "...zero-cost hop routing." The PR title/body have been updated to reflect the new scope. No action needed.

2. Two same-valued role sets — Good call. Added a comment in c8afeaca explaining that AUTO_FAVORITE_LOCAL_ROLES (which local roles may run the feature) and ZERO_HOP_RELAY_ROLES (which target roles actually relay) are distinct concepts that happen to share members today, and should not be deduplicated.

3. sourceQuery missing from the lock call — False positive. The /api/nodes/:nodeId/favorite-lock endpoint reads sourceId from req.body by design (server.ts:1557, with requirePermission(..., { sourceIdFrom: 'body' })), so passing it in the body — not the query string — is correct here. This is also pre-existing code untouched by this PR.