Claude finished @Yeraze's task —— View job

PR Review: fix: prevent messages from vanishing in multi-source unified feed

• Gathering context
• Reading changed files
• Analyzing code quality, bugs, performance, security, and test coverage
• Posting review feedback

Summary

This PR fixes two compounding bugs causing messages to vanish from the unified multi-source feed. The fixes are well-targeted and the code quality is high. Here's my detailed review:

Bug 1 — fetchLimit starvation fix (limit × 5)

src/server/routes/unifiedRoutes.ts:458

const fetchLimit = limit * 5;

Assessment: ✅ Correct and well-reasoned.

The PR description explains the math clearly (~167 s headroom at 3 msg/s vs a 10 s poll interval). The comment in code mirrors the PR description well.

Minor observation: The /packets endpoint on line 1022 still uses limit * 2:

const fetchLimit = limit * 2;  // /packets endpoint, unchanged

This is intentional — that endpoint is an RF-level monitor with no dedup, so starvation works differently (each packet is unique per source). The asymmetry is defensible, but a brief comment at the packets-endpoint fetchLimit line explaining why it doesn't need 5x (no cross-source dedup) would help future readers avoid "fixing" it unnecessarily.

Bug 2 — _dbchan / _radio suffix stripping

src/server/routes/unifiedRoutes.ts:214-234

const SERVER_COPY_SUFFIXES = new Set(['dbchan', 'radio']);

export function extractPacketIdFromRowId(rowId: unknown): number | null {
  ...
  const tailIdx = SERVER_COPY_SUFFIXES.has(parts[parts.length - 1])
    ? parts.length - 2
    : parts.length - 1;
  if (tailIdx < 1) return null;
  ...
}

Assessment: ✅ Correct logic, good defensive bounds check.

The tailIdx < 1 guard correctly rejects degenerate cases like a_dbchan (only one non-suffix segment, which would be the source prefix, not a numeric packet ID).

Potential edge case (low risk): If a future suffix is added to SERVER_COPY_SUFFIXES and a row ID happens to have two consecutive known suffixes (e.g., src_123_456_radio_dbchan), only the outermost suffix is stripped and _radio would be treated as the packet ID segment, returning null. This is unlikely in practice but worth noting if the suffix list grows. A while loop stripping suffixes would be more robust, though overkill for the current two-value set.

Naming clarity: The constant SERVER_COPY_SUFFIXES is well-named and the JSDoc comment block clearly explains the contract and history.

Code Quality

• ✅ The comment block before extractPacketIdFromRowId (lines 134–162) is excellent — it explains the key format contract, why this field is load-bearing, and exactly what happens if the format drifts.
• ✅ No raw SQL introduced; the fix is entirely in routing/dedup logic.
• ✅ The SERVER_COPY_SUFFIXES constant is defined at module scope (not inside the hot path), so Set.has() is O(1) per call with no allocation per message.
• ✅ The existing JSDoc on extractPacketIdFromRowId was updated inline with the new behavior.

Test Coverage

src/server/routes/unifiedRoutes.test.ts

New tests added:

• strips _dbchan suffix before extracting packetId (#3719) — ✅
• strips _radio suffix before extracting packetId (#3719) — ✅
• returns null when only a suffix segment remains after stripping — ✅ (covers the tailIdx < 1 guard)
• caps limit at 500 (fetches 2500 per source for dedup headroom) — ✅ (updated from 2x → 5x assertion)

One gap: The channel filter path's fetchLimit assertion at line 641 checks for 250 (limit=50 × 5), which is correct. The no-filter path at line 592 checks for 2500 (limit capped to 500 × 5). Both are validated. ✅

Suggestion (non-blocking): There's no test for double-suffix stripping (e.g., src_a_1234_567890_radio_dbchan), but given the edge case is unlikely and the current behavior (returning null) is safe, this is not a blocker.

Performance

• The suffix check is a single Set.has() on the last element of an already-split array — negligible overhead per message.
• Raising fetchLimit from 2x to 5x increases DB row fetches by 2.5x for high-traffic deployments. This is an intentional tradeoff for correctness and is clearly documented. At the default limit of 100, this is 500 rows per source per 10-second poll — well within normal SQLite/Postgres capacity.

Security

No security concerns. The rowId input is validated for type, length, digit-only content, and numeric range before use. The DB fetch limit increase is bounded (capped at 500 * 5 = 2500).

Verdict

LGTM. Both fixes are correct, well-tested, and well-documented. The PR description's root-cause analysis is accurate and matches the code. No blocking issues found.

Claude finished @Yeraze's task —— View job

PR Review: fix: prevent messages from vanishing in multi-source unified feed

• Gathering context
• Reading changed files
• Analyzing code quality, bugs, performance, security, and test coverage
• Posting review feedback

Summary

This PR fixes two compounding bugs causing messages to vanish from the unified multi-source feed. Both fixes are well-targeted and the code quality is high. Here is my detailed review:

Bug 1 — fetchLimit starvation fix (limit × 5)

src/server/routes/unifiedRoutes.ts:458

const fetchLimit = limit * 5;

Assessment: ✅ Correct and well-reasoned.

The PR description explains the math clearly (~167 s headroom at 3 msg/s vs a 10 s poll interval). The comment in code mirrors the PR description well.

Minor observation: The /packets endpoint on line 1022 still uses limit * 2:

const fetchLimit = limit * 2;  // /packets endpoint, unchanged

This is intentional — that endpoint is an RF-level monitor with no dedup, so starvation works differently. However, a brief comment at that line explaining why it doesn't use 5x (no cross-source dedup needed) would help future readers avoid unnecessarily "fixing" it.

Bug 2 — _dbchan / _radio suffix stripping

src/server/routes/unifiedRoutes.ts:208-234

const SERVER_COPY_SUFFIXES = new Set(['dbchan', 'radio']);

export function extractPacketIdFromRowId(rowId: unknown): number | null {
  ...
  const tailIdx = SERVER_COPY_SUFFIXES.has(parts[parts.length - 1])
    ? parts.length - 2
    : parts.length - 1;
  if (tailIdx < 1) return null;
  ...
}

Assessment: ✅ Correct logic, good defensive bounds check.

The tailIdx < 1 guard correctly rejects degenerate cases like a_dbchan (only one non-suffix segment, which would be the source prefix, not a numeric packet ID). The Set.has() call is O(1) and the Set is module-level so there's no allocation in the hot path.

Low-risk edge case: If a future suffix is added to SERVER_COPY_SUFFIXES and a row ID happens to have two consecutive known suffixes (e.g., src_123_456_radio_dbchan), only the outermost suffix is stripped. This is unlikely in practice and the safe fallback (null → text+timestamp key) means it degrades gracefully rather than corrupting data.

MeshCoreNativeBackend changes

src/server/meshcoreNativeBackend.ts / meshcoreNativeBackend.discovery.test.ts

The diff shows the request_regions tag guard was dropped (matching commit d73539a) and the discovery tests were expanded to cover the full 0x8E ControlData frame-parsing path. These look correct and are well covered by the new test cases:

• Frame layout ([55, 0x80, filter, tag(LE)]) is validated byte-for-byte in the test
• Tag-mismatch guard is tested (non-matching tag → ignored)
• Prefix-only short-key response correctly skips addOrUpdateContact

Code Quality

• ✅ The comment block before extractPacketIdFromRowId is excellent — it explains the key format contract, the history, and exactly what happens if the format drifts.
• ✅ No raw SQL introduced; the fix is entirely in routing/dedup logic.
• ✅ SERVER_COPY_SUFFIXES is defined at module scope (not inside the hot path), so Set.has() is O(1) per call with no allocation per message.
• ✅ JSDoc on extractPacketIdFromRowId was updated inline with the new behavior.

Test Coverage

New tests added in unifiedRoutes.test.ts:

• strips _dbchan suffix before extracting packetId (#3719) — ✅
• strips _radio suffix before extracting packetId (#3719) — ✅
• returns null when only a suffix segment remains after stripping — ✅ (covers tailIdx < 1 guard)
• caps limit at 500 (fetches 2500 per source for dedup headroom) — ✅ (updated from 2x → 5x assertion)
• Channel filter path fetchLimit assertion at line 641 checks 250 (limit=50 × 5) — ✅

Discovery test coverage in meshcoreNativeBackend.discovery.test.ts:

• Correct frame construction validated byte-by-byte — ✅
• Tag-echo matching — ✅
• Prefix-only short key → no auto-add — ✅

Performance

• The suffix check is a single Set.has() on the last element of an already-split array — negligible overhead per message.
• Raising fetchLimit from 2x to 5x increases DB row fetches by 2.5x for high-traffic deployments. This is an intentional correctness tradeoff. At the default limit of 100, this is 500 rows per source per 10-second poll — well within normal SQLite/PostgreSQL/MySQL capacity.

Security

No security concerns. The rowId input is validated for type, length, digit-only content, and numeric range before use. The DB fetch limit increase is bounded (capped at 500 × 5 = 2500).

Verdict

LGTM. Both fixes are correct, well-tested, and well-documented. The PR description's root-cause analysis is accurate and matches the code. No blocking issues found. The one optional suggestion (comment at the /packets fetchLimit line) is non-blocking.

Claude finished @Yeraze's task —— View job

PR Review: fix: prevent messages vanishing in multi-source unified feed

• Gathering context
• Reading changed files
• Analyzing code quality, bugs, performance, security, and test coverage
• Posting review feedback

Summary

This PR carries three commits fixing two independent bugs. The analysis below covers the final state of the branch (including the mid-PR revert in commit 4713a21).

Fix 1 — _dbchan / _radio dedup key fix (unifiedRoutes.ts)

src/server/routes/unifiedRoutes.ts:208–234

const SERVER_COPY_SUFFIXES = new Set(['dbchan', 'radio']);

export function extractPacketIdFromRowId(rowId: unknown): number | null {
  ...
  const tailIdx = SERVER_COPY_SUFFIXES.has(parts[parts.length - 1])
    ? parts.length - 2
    : parts.length - 1;
  if (tailIdx < 1) return null;
  ...
}

Assessment: ✅ Correct logic with appropriate defensive guards.

• The tailIdx < 1 guard correctly rejects degenerate single-segment cases like a_dbchan where there's no numeric packet ID after the suffix is stripped.
• SERVER_COPY_SUFFIXES defined at module scope — Set.has() is O(1) with no per-message allocation in the hot path.
• The comment block above the function is well-documented with historical context and the key format contract.

Minor edge case (low risk, non-blocking): If a row ID ever has two consecutive known suffixes (e.g., src_123_456_radio_dbchan), only the outermost is stripped. This degrades safely to null (fallback to text+timestamp dedupKey, causing duplicate entries rather than data corruption). Unlikely enough that adding a while loop would be overkill.

Naming: SERVER_COPY_SUFFIXES is well-named and immediately clear to future readers.

Fix 1 — fetchLimit evolution (two-step in this PR)

The original commit (d98f36c) raised fetchLimit from limit × 2 to limit × 5. A subsequent commit (4713a21) then reverted it back to limit × 2 with a clear rationale: the client-side append-only accumulator introduced in #3738 makes messages persist regardless of the server window, making the 5x multiplier redundant overhead.

Assessment: ✅ Well-handled. The revert commit message is explicit about the reasoning and preserves the _dbchan/_radio fix. The test comments were also updated to stay consistent (1000 not 2500).

One inconsistency to flag: The test description at line 586 reads:

it('caps limit at 500 (fetches 1000 per source for dedup headroom)', ...)

But the assertion that was reverted on line 593 left the comment // fetchLimit = limit * 2 = 1000 which is correct. However, the channel-filter test comment at line ~641 reads // fetchLimit = limit*2 but the value there is 100 (limit=50 × 2), which is consistent. ✅

Fix 2 — Region discovery timeout (meshcoreNativeBackend.ts)

src/server/meshcoreNativeBackend.ts — request_regions handler

const onResp = (r: any) => {
  if (!sentReceived) return;
  // No tag check: CMD_SEND_ANON_REQ (57) uses sendToRadioFrame, and
  // the Sent ack's expectedAckCrc does NOT match the BinaryResponse's
  // tag (ANON_REQ uses a different tagging scheme than SendBinaryReq (50)).
  // The runExclusiveRadioOp lock serializes all radio ops so the first
  // BinaryResponse after Sent is guaranteed to be ours (#3734).
  cleanup();
  resolve((r?.responseData ?? new Uint8Array()) as Uint8Array);
};

Assessment: ✅ Correct fix with a sound security model.

The root cause analysis in the PR description is accurate: CMD_SEND_ANON_REQ (57) via sendToRadioFrame does not echo the CRC in BinaryResponse.tag unlike SendBinaryReq (50). Removing the tag check and relying on runExclusiveRadioOp for serialization is the correct solution since that lock already guarantees only one radio op listens at a time.

The existing sentReceived gate is correctly preserved — it prevents a BinaryResponse from a previous operation (arriving during our Sent window) from being accepted before our own Sent ack arrives.

Comment quality: The inline comments clearly document the why, including the cross-reference to #3734 and the comparison to SendBinaryReq (50). The runExclusiveRadioOp JSDoc block is comprehensive.

Test Coverage

unifiedRoutes.test.ts — new tests for the dedup-key fix:

• strips _dbchan suffix before extracting packetId (#3719) — ✅
• strips _radio suffix before extracting packetId (#3719) — ✅
• returns null when only a suffix segment remains after stripping — ✅ (covers tailIdx < 1 guard)
• caps limit at 500 (fetches 1000 per source for dedup headroom) — ✅ (reverted to 2x)

meshcoreNativeBackend.discovery.test.ts — new tests for the region fix:

• request_regions sends [57, pubkey, 0x01, 0x00] and parses clock + region names — ✅ byte-by-byte frame validation
• request_regions succeeds when Sent ack carries no expectedAckCrc — ✅ edge case for r=undefined path
• request_regions resolves when expectedAckCrc from Sent does not match BinaryResponse tag (#3734) — ✅ exact regression scenario

The regression test at line 313 precisely models the firmware behavior described in the root cause: expectedAckCrc: 0xcafe1234 in Sent with tag: 0 in BinaryResponse. This is excellent regression test craftsmanship.

Code Quality

• ✅ No raw SQL introduced anywhere.
• ✅ All three backends (SQLite/PostgreSQL/MySQL) unaffected — fix is in routing/dedup and MeshCore backend only.
• ✅ runExclusiveRadioOp docblock clearly explains the serialization contract and why tag-matching isn't needed.
• ✅ r?.responseData ?? new Uint8Array() safely handles a missing responseData field in the BinaryResponse event.

Performance

• Suffix check: single Set.has() per message — negligible.
• fetchLimit is back to limit × 2 (reverted from 5x), keeping the per-poll DB load unchanged from pre-PR behavior.
• No new blocking paths introduced in the request_regions handler; the change only removes a check rather than adding one.

Security

No concerns. The rowId input validation (type check, length cap, digit-only regex, 32-bit range check) remains intact. The tag-check removal in request_regions is safe within the serialized radio-op lock — the comment documents the reasoning clearly.

Verdict

LGTM. The two underlying bugs are correctly identified and fixed, the test coverage for both regressions is solid, and the mid-PR revert of the 5x window-widening is well-justified and cleanly executed. No blocking issues.