Claude finished @Yeraze's task —— View job

PR Review: feat(auto-ack): message-type × hop-distance 2×2 matrix

• Gathering context and reading changed files
• Review server-side logic (autoAckDecision.ts, meshtasticManager.ts)
• Review migration (093_autoack_matrix.ts)
• Review frontend (AutoAcknowledgeSection.tsx, AutomationContext.tsx)
• Review settings constants
• Review test coverage
• Post final review

This is a well-structured refactor that cleanly replaces a tangled legacy design with an explicit 2×2 matrix model. The separation of concerns — pure decision logic extracted to autoAckDecision.ts, typed model in autoAckMatrix.ts, idempotent migration — is all solid. A few items worth discussing:

Potential Bugs / Correctness

1. Message template selection uses raw hopsTraveled === 0 instead of the matrix's isZeroHop logic

In meshtasticManager.ts:9190, the direct-vs-multihop template selection reads:

const autoAckMessage = (hopsTraveled === 0 && autoAckMessageDirect)
  ? autoAckMessageDirect
  : autoAckMessageStandard;

This diverges from the matrix's cell classification, which uses autoAckIsZeroHop(hopsTraveled, message.viaMqtt). An MQTT-relayed packet at 0 hops falls into the MultiHop cell (correctly), but will still use the direct message template, producing a mismatch between the cell chosen and the template rendered. The fix would be to use the already-computed isZeroHop variable here instead of re-checking hopsTraveled === 0.

2. Migration skips prefixes that have only autoAckRegex/autoAckChannels set

From computeMigrationInserts in 093_autoack_matrix.ts:68–71:

const hasBehaviorKey = LEGACY_SUFFIXES.some(
  (s) => s !== 'autoAckEnabled' && legacy[s] !== undefined,
);
if (legacy.autoAckEnabled !== 'true' && !hasBehaviorKey) continue;

autoAckRegex and autoAckChannels are not in LEGACY_SUFFIXES, so a source that has autoAckEnabled: 'false' (never explicitly 'true') plus those keys gets skipped entirely. This means a user who enabled auto-ack, used it, then disabled it (setting autoAckEnabled: 'false') would have their behavior toggles zeroed. This is arguably correct behavior (disabled → all off), but the test in line 94–101 explicitly documents this and the comment says "no enable, no behavior keys". Worth making the boundary explicit in a code comment for the legacy.autoAckEnabled !== 'true' case, since the subtle case is 'false' (explicitly disabled) vs absent.

3. replyId handling when packetId is 0

In autoAckDecision.ts:62:

replyId: cellReplyDmEnabled && !isDirectMessage ? undefined : packetId,

packetId is typed as number | undefined. The manager passes meshPacket.id which can legally be 0. If packetId === 0, replyId will be 0 (falsy but not undefined). The comment in meshtasticManager.ts:9157 already calls this out for tapbacks: "packetId can be 0 (valid unsigned integer), so check explicitly". The routing helper handles this correctly since it checks packetId only for undefined, not falsiness — this is fine, but the packetId != null guard at meshtasticManager.ts:9157 and meshtasticManager.ts:9082 correctly handles this. No bug, but worth a note.

Code Quality

4. computeMatrixValues — onDefaultTrue semantics should be explicitly documented

In 093_autoack_matrix.ts:87:

const onDefaultTrue = (v: string | undefined) => v === undefined ? true : v !== 'false';

This means '0', 'yes', '1', etc. all count as true. In the legacy code this was intentional (saving false is the only way to turn something off), but it's a non-obvious semantic that could surprise a future maintainer. A short inline comment on this line would help.

5. The skipped test suite is a meaningful gap

AutoAcknowledgeSection.test.tsx:42 marks the entire legacy suite as describe.skip(...) with a comment noting it's a "Tier 2 follow-up". The legacy tests covered save-flow, 403 handling, regex validation, token buttons, and sample preview — all behaviors that still exist in the new component. The new active suite (AutoAcknowledgeSection — response matrix) only covers the 2×2 checkbox rendering and the "Respond via DM disabled until Message checked" rule. The save flow, error handling, and channel toggle paths are now untested in this file. Suggest tracking the rewrite as a follow-up issue if not doing it now.

6. autoAckChannels parsing has diverged between server and context

meshtasticManager.ts:9023–9025 parses autoAckChannels as a comma-separated string:

const enabledChannels = autoAckChannels
  ? autoAckChannels.split(',').map(c => parseInt(c.trim())).filter(n => !isNaN(n))
  : [];

AutomationContext.tsx:173–184 parses it first as JSON, then CSV. Both paths should arrive at the same result in practice, but there's a subtle difference: if somehow a JSON array string like "[0,1]" was saved (e.g. by a legacy path), the server would return an empty array while the context would parse it correctly. This is a pre-existing inconsistency and not introduced by this PR, but worth noting in context of the broader settings cleanup.

Performance

7. Six sequential getSettingForSource calls per auto-ack message

After the matrix cell is determined, the code makes three sequential DB calls (cellReplyEnabled, cellTapbackEnabled, cellReplyDmEnabled) on top of the earlier lookups. Combined with the existing per-message DB reads (autoAckEnabled, autoAckChannels, etc.), each auto-ack trigger makes 8–10 sequential async DB calls. For SQLite this is fine at low volumes, but under load (burst channel messages) this creates queuing. This is a pre-existing issue not specific to this PR, but the new matrix adds 3 more sequential reads on the hot path. A future optimization would be to batch-read all autoAck settings in one query.

Minor Nits

• AUTOACK_CELLS[0].label is 'Channel · Direct (0 hops)' while AUTOACK_CELLS[2].label is 'Direct Message · 0 hops' — the formatting is inconsistent (the first uses · with (0 hops) in parentheses, the second uses · without). Worth standardizing.
• autoAckDirectZeroHopReplyDmEnabled and autoAckDirectMultiHopReplyDmEnabled are always set to 'true' by computeMatrixValues (direct replies are inherently DMs). Since these keys are now in both VALID_SETTINGS_KEYS and PER_SOURCE_SETTINGS_KEYS, the UI still allows saving 'false' to them for direct cells. The UI renders them as checked + disabled, which is correct, but there's no server-side validation enforcing the invariant. Not a security issue, just a consistency note.
• The docs/internal/dev-notes/AUTOACK_2X2_PLAN.md design doc was added per CLAUDE.md instructions, but CLAUDE.md says "NEVER proactively create documentation files unless explicitly requested" — this may be intentional if the doc was requested, but it's worth confirming it was desired as a committed artifact vs a working note.

Summary

The core design is sound. The most actionable items are:

1. Bug: Fix the message template selector to use isZeroHop instead of hopsTraveled === 0 to keep MQTT 0-hop packets consistent across cell selection and template choice.
2. Test debt: The skipped legacy test suite should be tracked as a follow-up issue.
3. Minor: Label string consistency, inline comment on onDefaultTrue semantics.