Claude finished @Yeraze's task —— View job

PR Review: fix(unified): append-only message feed

Tasks:

• Read changed files
• Analyze code quality and best practices
• Check for potential bugs
• Review performance considerations
• Check security concerns
• Evaluate test coverage
• Post review feedback

Overall Assessment

This is a well-designed structural fix for a real disappearing-message bug. The core idea — making the live feed append-only via a persistent Map keyed by dedupKey — is sound, and the implementation is clean with appropriate guardrails (upsert semantics, cap, reset on channel change). The PR is production-ready with only minor observations worth noting.

src/utils/unifiedMessageAccumulator.ts

Positives:

• Pure function with no side-effects on the outside world — easy to test and reason about in isolation.
• Generic <T extends AccumulableMessage> keeps the utility reusable for other paginated feeds without requiring changes.
• The mutation-in-place design (acc is mutated) is intentional and documented, which is important since the caller relies on the ref surviving re-renders.

Observation — sort stability on equal createdAt:
The sort a.createdAt - b.createdAt is unstable when two messages share the same millisecond timestamp (e.g. two MQTT bridged messages batched in the same poll). Different JS engines may produce different orderings for equal elements. This is pre-existing behaviour, but since the accumulator now persists order across polls, a transient inconsistency on one poll becomes permanent. Worth a follow-up if message ordering with identical createdAt turns out to matter.

Observation — cap threshold triggers re-rebuild every render above 5000:
When out.length > cap, the function clears and re-populates acc in O(cap). That's fine. However, if the caller passes a cap lower than the current acc.size, every subsequent call will do a clear+rebuild even if no new messages were added (because acc will be at cap after the rebuild, then get 0 new messages but out.length will equal cap, not exceed it — actually this is fine). No real issue here; just confirmed the logic is correct.

src/pages/UnifiedMessagesPage.tsx (lines 287–303)

Concern — ref mutation inside useMemo violates React rules:

// lines 294–303
const accumulatorRef = useRef<Map<string, UnifiedMessage>>(new Map());
const accChannelRef = useRef<string>(selectedChannel);
const allMessages = useMemo<UnifiedMessage[]>(() => {
  // Reset synchronously when the channel changes so feeds never blend.
  if (accChannelRef.current !== selectedChannel) {
    accChannelRef.current = selectedChannel;
    accumulatorRef.current = new Map();   // ← ref mutation inside useMemo
  }
  return foldUnifiedMessagePages(accumulatorRef.current, data?.pages);
}, [data?.pages, selectedChannel]);

React's rules of hooks state that useMemo must be a pure computation — it must not produce observable side-effects. Mutating a useRef value is technically a side-effect (it modifies external mutable state), and React is free to discard and recompute useMemo callbacks multiple times in Strict Mode / Concurrent Mode. If React re-runs this memo before committing, acc gets doubled-up entries because foldUnifiedMessagePages mutates acc in place.

A safer pattern would be to use useEffect for the reset and a separate useMemo that only reads from acc after the effect has run, or — more pragmatically for a ref-based accumulator — handle the full fold in a useEffect that sets a useState snapshot:

const [allMessages, setAllMessages] = useState<UnifiedMessage[]>([]);
useEffect(() => {
  if (accChannelRef.current !== selectedChannel) {
    accChannelRef.current = selectedChannel;
    accumulatorRef.current = new Map();
  }
  setAllMessages(foldUnifiedMessagePages(accumulatorRef.current, data?.pages));
}, [data?.pages, selectedChannel]);

This keeps the mutation entirely inside an effect (no Strict Mode double-run hazard) and gives React a proper state-driven re-render signal. The current implementation may work fine in production builds, but will produce subtle bugs in React 18+ Strict Mode dev where effects/memos intentionally double-invoke.

Minor — refetch() on manual refresh button doesn't reset the accumulator:
Line 494 calls refetch() which re-fetches only page 1. The accumulator retains all prior messages, which is the desired behaviour. However if the user specifically wants to see a "clean" feed (e.g. they suspect stale data), there's no way to flush the accumulator without switching channels. A reset-on-explicit-refresh option might be worth a future ticket.

src/utils/unifiedMessageAccumulator.test.ts

Positives:

• The core regression test ("retains a message that is gone from a later poll") directly captures the bug scenario — excellent.
• Good coverage of: sort order, intra/cross-page dedup, upsert-wins, undefined pages, cap behaviour (both snapshot length and acc.size trimmed).

Gap — concurrent channel blending (the reset path):
The test suite doesn't cover the channel-reset guard. Specifically: if selectedChannel changes between two foldUnifiedMessagePages calls, does the test confirm that old-channel messages don't bleed into the new channel's feed? The reset happens in UnifiedMessagesPage.tsx, not in foldUnifiedMessagePages itself, so the accumulator utility doesn't know about channels. A test (or comment) clarifying this responsibility split would be helpful for future maintainers.

Gap — empty page in multi-page response:
No test covers pages = [[], [msg('a', 100)]] (an empty first page). This is an edge case the server might return under some query conditions.

Security

No concerns. The accumulator is a pure client-side data structure; there's no XSS risk beyond what already exists in renderMessageWithLinks. The dedupKey is used only as a Map key, not rendered directly.

Summary

The useMemo-with-ref-mutation pattern is the one item I'd flag for a fix before merging to main, given that React Strict Mode double-invocation will mutate accumulatorRef twice per poll in development and could surface confusing duplicate-entry bugs during dev testing.