Skip to content

fix(chat-summary-history): join summary-batch-heartbeat protocol to dedupe polling (#334)#371

Open
MrAladdin wants to merge 8 commits into
Mininglamp-OSS:mainfrom
MrAladdin:fix/issue-334-chat-summary-history-heartbeat
Open

fix(chat-summary-history): join summary-batch-heartbeat protocol to dedupe polling (#334)#371
MrAladdin wants to merge 8 commits into
Mininglamp-OSS:mainfrom
MrAladdin:fix/issue-334-chat-summary-history-heartbeat

Conversation

@MrAladdin

Copy link
Copy Markdown

Summary

Fixes #334POST /summary/api/v1/summaries/batch-status is hit redundantly when the conversation sidebar's summary history is shown after the standalone Smart Summary tab has been visited. The reporter (jeff-wilson2010) traced the root cause to three independent pollers running with no cross-coordination; the maintainer triage (lml2468) confirmed the diagnosis and recommended #2 minimal patch — this PR — with #1 root-fix as a follow-up (now filed as #370).

Root Cause (verified)

The codebase already has a 3-event window.dispatchEvent heartbeat protocol coordinating SummaryListPage (dispatcher) and SummaryDetailPage (listener):

  • summary-batch-heartbeatSummaryListPage:152 dispatches per poll; SummaryDetailPage:120 listens → stops 15s fallback.
  • summary-status-change — same, dispatched on status change → reload detail.
  • summary-list-unmountSummaryListPage:84 on cleanup → restart fallback if needed.

ChatSummaryHistory (sidebar 5s poller) is completely outside this protocol:

$ grep -rn 'summary-batch-heartbeat' packages/ apps/
packages/dmworksummary/src/pages/SummaryDetailPage.tsx:120: addEventListener
packages/dmworksummary/src/pages/SummaryListPage.tsx:152:   dispatchEvent

When the user has visited the Smart Summary tab once, Main keeps SummaryListPage mounted under display:none and its 2 s timer continues polling. Opening the sidebar starts ChatSummaryHistory's 5 s timer on the same taskIds. The two are mutually unaware — same task IDs hit at both 2 s and 5 s cadence.

Approach (Minimal — Approach A per spec)

Bring ChatSummaryHistory into the existing 3-event protocol:

  1. Listener: on summary-batch-heartbeat, if payload's taskIds ⊇ our active set (subset check), set peerActive=true with a 15 s freshness window. The 5 s tick consults this flag and skips a tick without stopping the timer.
  2. Dispatcher: after a successful own poll, broadcast summary-batch-heartbeat with our taskIds, so peers can defer to us symmetrically.
  3. summary-status-change listener refreshes loadHistory() when payload taskIds intersects current items; ignored otherwise.
  4. summary-list-unmount listener clears peerActive so the next tick polls.
  5. Self-echo guard: window.dispatchEvent is synchronous, so our own dispatch re-enters our own listener. A transient isDispatchingOwnHeartbeat flag in doPoll (set true before dispatch, cleared in finally) suppresses the re-entry — without it, solo polling silently degrades from 5 s → ~15 s (caught in Phase 3.5 adversarial verify before any code was written).

Subset semantics (in containsAllTaskIds helper): we skip ONLY when every one of our active IDs is in the heartbeat payload. Partial-overlap payloads must not suppress, or the uncovered IDs would never get a status update.

Existing coordination preserved: ChatSummaryHistory.paused prop (sibling coordination within the same ChatSummaryPanel), chat-summary-created / chat-summary-deleted handlers, all existing pre-existing tests — untouched.

Files Changed

 docs/superpowers/specs/2026-06-10-issue-334-summary-batch-status-coordination-design.md | +264
 packages/dmworksummary/src/components/ChatSummaryHistory.tsx                            |  +89
 packages/dmworksummary/src/components/__tests__/ChatSummaryHistory.test.tsx             | +348
 packages/dmworksummary/src/pages/SummaryDetailPage.tsx                                  |   +1  -2
 packages/dmworksummary/src/utils/heartbeatCoverage.ts                                   |  +27
 packages/dmworksummary/src/utils/__tests__/heartbeatCoverage.test.ts                    |  +37
 6 files changed, 766 insertions(+), 2 deletions(-)

Commit history (9 commits, fix → refactor cleanup):

d83c617c test(chat-summary-history): extract mountHistory helper (#334)
9eb00b00 refactor(chat-summary-history): dedupe self-echo guard comment (#334)
bba604d2 refactor(summary): share COVERING_HEARTBEAT_WINDOW_MS across heartbeat consumers (#334)
aa8f12ad fix(chat-summary-history): skip status-change refresh while paused (#334)
fb5b0958 fix(chat-summary-history): listen for summary-status-change and summary-list-unmount (#334)
afda5a31 fix(chat-summary-history): dispatch summary-batch-heartbeat after own poll (#334)
b3053db3 fix(chat-summary-history): join summary-batch-heartbeat protocol as listener (#334)
658d6030 fix(summary): add containsAllTaskIds heartbeat-coverage helper (#334)
9ddebcd7 docs(superpowers): add #334 design spec + implementation plan

Test Plan

Unit tests

  • containsAllTaskIds helper — 7/7 pass (covers superset, equal, partial-overlap, disjoint, empty-active triplet, empty/undefined-payload-with-active, large-payload O(1) lookup).
  • ChatSummaryHistory heartbeat coordination — 9 new cases:
    • Listener: covering heartbeat suppresses next tick
    • Listener: polls again after 15 s freshness expires
    • Listener: non-covering (partial-overlap) heartbeat does not suppress
    • Listener: empty heartbeat with active items does not suppress
    • Dispatcher: own poll broadcasts heartbeat with our active IDs
    • Self-echo guard regression: solo polling stays at 5 s cadence across 3 consecutive ticks (would fail at "called 2 times" without guard)
    • summary-status-change refreshes when payload intersects
    • summary-status-change ignored when no intersection
    • summary-list-unmount clears suppression
    • (also does not fetch on summary-status-change while paused from quality-reviewer follow-up)
  • Full dmworksummary suite: 198 total / 194 pass / 4 pre-existing failures (no delta from main baseline — failures live in an unrelated Object.defineProperty(window, 'innerWidth', ...) test file).

Real-browser runtime verification

Local dev (octo-dev script), setup:

  1. Registered test_user_a + test_user_b, made them mutual friends via DB, created Space TestSpace334, created Group TestGroup334 (members: both users), and INSERT'd one summary task with status=WAITING_CONFIRM (long-lived active state — worker doesn't process it without user confirmation, so the polling window is bounded only by confirm_deadline).
  2. Logged in as test_user_a → visited Smart Summary tab (mounts SummaryListPage, starts 2 s poll) → switched back to chat → opened TestGroup334 → opened sidebar 智能总结 panel (mounts ChatSummaryHistory, would start 5 s poll).
  3. Installed an XMLHttpRequest.prototype.open/send hook in the page console to count /summaries/batch-status requests over a 60 s window.
Scenario Requests / 60 s (extrapolated from sample) Cadence distribution
PRE-FIX (git checkout origin/main -- ChatSummaryHistory.tsx) ~42 51 requests in 73 s: 29 ~2s intervals + 14 <1s intervals (the 2s and 5s streams interleaving) + 7 other
POST-FIX (this PR) ~30 38 requests in 75 s: all intervals 1924–2005 ms — pure 2 s stream, ChatSummaryHistory's 5 s tick fully suppressed by SummaryListPage's heartbeat
Delta −12 req / 60 s (~28% reduction) 0 requests at the 5 s cadence, confirming the listener's subset-and-freshness check works

The 60s post-fix figure (~30) exactly matches the theoretical SummaryListPage-only cadence (60 / 2 = 30 ticks). The pre-fix figure (~42) matches the theoretical sum (30 + 12 = 42, where 12 = 60/5).

No regression scenarios (also verified in unit tests):

  • History alone, no peer broadcasting → keeps its 5 s cadence (self-echo guard regression test).
  • paused === true while a status-change event arrives → no fetch (covered by does not fetch on summary-status-change while paused).
  • summary-list-unmount arrives → next 5s tick polls normally (covered).

Code review

  • superpowers:subagent-driven-development per task: implementer → spec reviewer → code-quality reviewer (5 tasks total, all pass).
  • /simplify 4 reviewers (reuse / simplification / efficiency / altitude) in parallel; applied:
    • F2/F6 — exported COVERING_HEARTBEAT_WINDOW_MS from utils/heartbeatCoverage.ts and replaced SummaryDetailPage's inline 15000 literal so the protocol's freshness window lives in one place.
    • F3 — extracted mountHistory() test helper to dedupe render + advanceTimers boilerplate across the new heartbeat tests.
    • F5 — collapsed duplicate self-echo guard comment in doPoll to a one-line reference to the authoritative explanation on handleBatchHeartbeat.
    • F1 / F7 / F8 deferred to follow-up issues (Approach C refactor(summary): extract summaryPollingService singleton (#334 follow-up — Approach C) #370 absorbs F7/F8; canCancel/isInFlight consolidation belongs to its own cleanup).

Adversarial Verification (Phase 3.5)

Before any subagent dispatch, ran a plan-stage adversarial check (Critical Rule 5 Layer 1) — caught one would-have-shipped regression:

V8 — self-echo loop: dispatcher's own window.dispatchEvent re-enters its own listener on the same call stack; listener flips peerActive=true from our own broadcast; next 5 s tick gets suppressed; we never re-dispatch (no peer was ever broadcasting); freshness window expires at 15 s → solo polling cadence silently degrades from 5 s to ~15 s.

Fix patched into plan + added a dedicated regression test (does not self-suppress: solo polling stays at 5s cadence) before any implementation work — see commit afda5a31's tests and the isDispatchingOwnHeartbeat field in b3053db3.

Risks & Mitigations

Risk Mitigation
Subset check correctness on large payloads containsAllTaskIds uses a Set for O(1) payload lookup; tested at scale (5000 elements).
paused + listener interaction (setState while invisible) paused guard added at top of handleStatusChangeEvent (commit aa8f12ad); handleBatchHeartbeat only flips instance fields, never setState.
Stale heartbeat after channel switch The freshness check is re-evaluated every tick with the current getActiveTaskIds(); a no-longer-covering peer stops being suppressive within at most 15 s.
window.dispatchEvent is synchronous (self-echo) isDispatchingOwnHeartbeat flag in doPoll (try/finally) — listener returns early during own dispatch. Tested.
Tests miss something unit can't see (the #256 Task E lesson) Real-browser pre/post-fix run completed; numbers above.
Approach C (singleton service) is the right altitude Out of scope for this minimal patch; filed as #370. The self-echo guard added here makes the case for Approach C concrete and citable.

Out-of-scope Follow-ups (each tracked separately)

Rollback

git revert d83c617c 9eb00b00 bba604d2 aa8f12ad fb5b0958 afda5a31 b3053db3 658d6030
# Spec doc (9ddebcd7) is harmless prose; can be kept or reverted independently.

Closes #334

🤖 Generated with Claude Code

@MrAladdin

Copy link
Copy Markdown
Author

✅ Phase 5 Runtime Verification — Two-Gate Compliance Confirmed

Per project rule Must Reproduce Every Issue (PRE-FIX repro + POST-FIX verify in real browser are both mandatory before PR), this PR has both gates documented in the PR body's "Test Plan → Real-browser runtime verification" section:

✅ Gate 1 — PRE-FIX repro (real browser)

Test fixture: test_user_a + TestSpace334 + TestGroup334 + DB-inserted WAITING_CONFIRM summary task.
Scenario: user has visited Smart Summary tab → sidebar 智能总结 panel open on the chat with an active task.
Observed in localhost:28080 Chrome DevTools (XHR hook on XMLHttpRequest.prototype):

51 batch-status requests over a 73s window. Interval distribution: 29 × ~2 s + 14 × <1 s overlap + 7 other — 2 s and 5 s streams interleaving. Reproduces the reporter's "duplicate polling" symptom 1:1.
Extrapolated to 60 s: ~42 requests.

✅ Gate 2 — POST-FIX verify (real browser, same setup)

Same fixture, same UI state, same 60 s window — fix applied:

38 requests over a 75 s window. All intervals 1924–2005 ms — pure 2 s stream, the ChatSummaryHistory 5 s tick is fully suppressed by the SummaryListPage heartbeat (0 requests in any 5 s cadence bucket).
Extrapolated to 60 s: ~30 requests.

Net: −12 requests / 60 s (−28%)ChatSummaryHistory's 5 s polling is 100% suppressed by the protocol while a peer (SummaryListPage) is broadcasting. No regression in solo-polling cadence (covered by the self-echo guard regression test).

(Full data + timeline + XHR hook installation are in the PR body's Test Plan section.)

…glamp-OSS#334)

Subset check shared by all summary-batch-heartbeat consumers. A heartbeat
is treated as covering iff every id in the listener's active set is in
the broadcast payload — partial-overlap payloads must not suppress
polling, or the uncovered ids would never receive status updates.

Used in the next commit to bring ChatSummaryHistory into the existing
heartbeat protocol.

Refs Mininglamp-OSS#334
…istener (Mininglamp-OSS#334)

Add a window 'summary-batch-heartbeat' listener that flips an in-instance
'peerActive' flag with a 15s freshness window when the broadcast covers
our entire active-task set. The existing 5s tick now consults this flag
and skips a tick (without stopping the timer) when a covering peer poll
is fresh.

Subset semantics (containsAllTaskIds): partial-overlap or disjoint
payloads must not suppress — the uncovered ids would otherwise never
receive status updates.

Refs Mininglamp-OSS#334
… poll (Mininglamp-OSS#334)

After every successful batchStatus call, broadcast a heartbeat carrying
our own active-task ids. This lets peers in the same protocol —
SummaryListPage, SummaryDetailPage's fallback, or another
ChatSummaryHistory instance — defer their own tick symmetrically when
we are already covering their interest set.

Mirrors SummaryListPage.doBatchPoll's existing dispatch (line 152) with
the same event name and payload shape.

Refs Mininglamp-OSS#334
…ry-list-unmount (Mininglamp-OSS#334)

* summary-status-change: refresh history from server when a peer
  surfaces a status flip for any currently-visible item. Ignored when
  taskIds do not intersect — avoids unrelated-channel refreshes.
* summary-list-unmount: clear peerActive suppression so the next 5s
  tick polls — we may now be the only poller covering our active set.

Handler named handleStatusChangeEvent to mirror SummaryDetailPage and
to avoid collision with SummaryListPage.handleStatusChange (a different
class's filter-dropdown callback).

Closes the listener half of the protocol (Task 2 already added the
batch-heartbeat listener; Task 3 added the dispatcher).

Refs Mininglamp-OSS#334
…ininglamp-OSS#334)

When the conversation summary panel switches to detail view it passes
paused=true to ChatSummaryHistory, which stops the 5s tick via the
existing maybeStartPoll guard. The new handleStatusChangeEvent handler
(commit fb5b095) did not honor paused — a peer's status-change event
would still trigger a full loadHistory network round-trip + setState
even though the list is hidden behind the detail view.

Mirror maybeStartPoll's paused guard at the top of the handler so a
hidden panel does no fetches.

Refs Mininglamp-OSS#334
…t consumers (Mininglamp-OSS#334)

The 15s freshness window for the summary-batch-heartbeat protocol now
lives once in utils/heartbeatCoverage.ts and is imported by both
ChatSummaryHistory (added in this PR) and SummaryDetailPage (where it
was inline as a bare 15000 literal). Tuning the window touches one site.

Pure refactor: no behavior change. Test suite (198 total) unchanged.

Refs Mininglamp-OSS#334
…glamp-OSS#334)

doPoll's comment now references handleBatchHeartbeat's authoritative
explanation instead of restating it. Pure documentation cleanup; no
behavior change.

Refs Mininglamp-OSS#334
…SS#334)

The 5 new heartbeat tests with identical setup now use a single helper.
Reduces the file's await-act boilerplate count without changing test
semantics — assertions and mocks per-test still live inline. Helper
opts in via the paused param when needed.

Refs Mininglamp-OSS#334

Gate-1-Verified: PRE-FIX 51 batch-status req/73s (2s+5s interleave) — see PR Mininglamp-OSS#371 comment

Gate-2-Verified: POST-FIX 38 req/75s pure-2s, ChatSummaryHistory 5s tick fully suppressed — see PR Mininglamp-OSS#371 comment
@MrAladdin MrAladdin force-pushed the fix/issue-334-chat-summary-history-heartbeat branch from d83c617 to 57b0a55 Compare June 11, 2026 05:44
@github-actions github-actions Bot added size/L PR size: L and removed size/XL PR size: XL labels Jun 11, 2026
@github-actions

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity. Please add an update or it will be closed.

@github-actions github-actions Bot added the stale label Jun 25, 2026
@lml2468 lml2468 added review:running:qa qa-engineer review in progress review:running:security security-engineer review in progress review:running:code code-reviewer review in progress labels Jun 27, 2026
@lml2468

lml2468 commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

QA Verdict: PASS

Embedded qa-engineer persona (review-lead, tick bb5bd690-6fae-4e69-a0b9-0e7d014262c2). Read → CI evidence verify → review.

Coverage

  • Unit (heartbeatCoverage.test.ts) — 7 cases against containsAllTaskIds: strict-superset, equal, partial-overlap → false, disjoint → false, empty myActiveIds triplet (covered/payload-empty/payload-undefined), empty-payload-with-active → false, 5000-element O(1) lookup. Symmetric "true / false" pairs around the subset predicate; no asymmetry I can see.
  • Unit (ChatSummaryHistory.test.tsx heartbeat block) — 10 cases covering every event the component now listens to:
    • covering heartbeat suppresses next tick ✓
    • 15 s freshness expiry resumes polling ✓
    • non-covering (partial overlap) does not suppress ✓
    • empty payload with active ids does not suppress ✓
    • dispatcher broadcasts our active ids ✓
    • self-echo guard regression (3 consecutive 5 s ticks; would fail at tick 2 without isDispatchingOwnHeartbeat) ✓
    • summary-status-change intersect → refresh ✓ / disjoint → ignore ✓ / paused → ignore ✓
    • summary-list-unmount clears suppression ✓

Edge / boundary

  • Subset semantics are pinned by tests on both sides of the line: an active id missing from the payload must keep polling — caught.
  • Freshness window value lives in one place (COVERING_HEARTBEAT_WINDOW_MS = 15_000, exported from utils/heartbeatCoverage.ts and consumed by both ChatSummaryHistory and SummaryDetailPage:353). No risk of the two sides drifting.
  • Cleanup is symmetric: all 3 new addEventListener calls have matching removeEventListener in componentWillUnmount (verified in diff lines 49-51 vs 59-61).

Flakiness risk

  • Tests use vi.useFakeTimers() + act consistently; no real setTimeout / Date.now() racing.
  • The 15 s-expiry test deliberately walks past the boundary (+15500 ms after first +5000) to avoid the equality edge — good.

Verify completeness (real-browser evidence)

  • PR body documents a manual instrumented run: 51 reqs / 73 s pre-fix vs 38 reqs / 75 s post-fix, every post-fix interval 1924–2005 ms (pure 2 s SummaryListPage cadence, no surviving 5 s ticks). The post-fix 30 req/60 s matches the theoretical SummaryListPage-only rate exactly. The 28 % drop is the headline bug: sidebar smart-summary batch-status duplicate polling (uncoordinated multi-poller) #334 user-visible win.
  • I did not re-run locally (CI sprint-gate failure is non-code — see note below — and the unit deltas + the real-browser narrative are reproducible from the diff). Marking this verdict on documented evidence + diff inspection.

Existing-suite delta

  • PR claims 194/198 pass, 4 pre-existing failures, baseline-equal. The 4 stale failures sit in an unrelated Object.defineProperty(window, 'innerWidth', ...) file. I cannot independently re-baseline from this tick (no checkout), but the claim is testable on next local run and the new tests are additive (no edits to other test files in the diff).

Risk noted (not a block)

Verdict

PASS — comprehensive unit coverage on both the helper and the protocol integration, an explicit regression test for the adversarial finding (self-echo loop), real-browser pre/post measurement that confirms the intended cadence drop, and no test edits to pre-existing files (additive only). The sprint-board CI red is an unrelated process gate.

— qa persona drain complete (case-A first-fanout, 1/3).

@lml2468 lml2468 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA Verdict: PASS — see verdict comment above for coverage, edge-case, flakiness, real-browser evidence, and the unrelated check-sprint sprint-board CI red note. (qa-engineer persona, tick bb5bd690)

@lml2468 lml2468 added review:done:qa:approve qa-engineer PASS and removed review:running:qa qa-engineer review in progress labels Jun 27, 2026
@lml2468

lml2468 commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Security Verdict: CLEARED

Embedded security-engineer persona (review-lead, tick bb5bd690). Diff-level review against STRIDE / OWASP.

Attack surface delta

Surface Pre Post Notes
Network endpoints POST /summary/api/v1/summaries/batch-status (existing) unchanged No new HTTP paths; this PR reduces request volume (~28 % per #334 measurement).
New event listeners summary-batch-heartbeat, summary-status-change, summary-list-unmount on window window.dispatchEvent is same-origin only; no cross-origin / postMessage introduced.
New globals / persistence none All state is instance-level (this.peerActive, this.lastCoveringEventTime, this.isDispatchingOwnHeartbeat).
Dependencies unchanged unchanged No dependencies-changed label; SBOM delta = 0.
AuthN / AuthZ unchanged unchanged loadHistory() and batchStatus() reuse pre-existing auth-bearing API client.
Crypto unchanged unchanged None added.

STRIDE

  • S (Spoofing)CustomEvent payloads come from same-origin JS. An attacker who can window.dispatchEvent(new CustomEvent('summary-batch-heartbeat', {detail: {taskIds: […all-ids…]}})) already has script execution and a worse capability surface (fetch to anywhere, DOM manipulation). Spoofing the heartbeat can only suppress the component's own polling (DoS-on-self, see below); cannot inject server data into the UI.
  • T (Tampering) — Payload is consumed read-only via containsAllTaskIds (subset check). No state mutation derived from peer payload other than peerActive boolean + timestamp. handleStatusChangeEvent calls loadHistory() which re-fetches authoritatively from the server — peer event cannot inject UI content.
  • R (Repudiation) — No audit log changes; no behavior here is auditable.
  • I (Info disclosure) — Heartbeat payload contains numeric taskIds only. Same task IDs are already rendered in DOM and present in network traces of the component itself. No PII added to event bus.
  • D (Denial of Service) — Two angles:
    1. Peer can suppress polling: a forged covering heartbeat suppresses the next 5 s tick for up to 15 s. The freshness window auto-recovers; effect is bounded and self-healing. Attacker with script execution can already clearInterval directly.
    2. Self-echo loop: caught and fixed via isDispatchingOwnHeartbeat flag with try/finally. Verified by the dedicated regression test (does not self-suppress: solo polling stays at 5s cadence).
  • E (Elevation) — No privilege boundary touched.

Input validation

  • containsAllTaskIds guards undefined/empty payloads explicitly (utils/heartbeatCoverage.ts:14-15). Set lookup is structurally safe (numeric IDs).
  • handleStatusChangeEvent checks taskIds length + paused prop before doing any work, then intersects with our items. No SQLi/XSS surface (no string interpolation, no innerHTML).

Code hygiene flags

  • try/finally around isDispatchingOwnHeartbeat ensures the flag clears even if dispatchEvent throws — correct.
  • componentWillUnmount cleanly removes all 3 new listeners — no leak.

Risk

  • None blocking. The single conceptual risk (peer suppression DoS) requires same-origin script execution, which strictly dominates the gained attack capability.

Verdict

CLEARED — no auth/crypto/SBOM/network-surface delta; same-origin event bus only; defensive try/finally around dispatcher; payload validation present; self-echo regression captured in tests.

— security persona drain complete (case-A first-fanout, 2/3).

@lml2468 lml2468 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security Verdict: CLEARED — see verdict comment for STRIDE walkthrough, attack-surface delta (no new HTTP / dep / auth / crypto), and the same-origin DoS bound. (security-engineer persona, tick bb5bd690)

@lml2468 lml2468 added review:done:security:approve security-engineer CLEARED and removed review:running:security security-engineer review in progress labels Jun 27, 2026
@lml2468

lml2468 commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Code Review Verdict: APPROVED

Embedded code-reviewer persona (review-lead, tick bb5bd690). Diff-level inspection across the 5 changed files.

Correctness

  • Self-echo guard (ChatSummaryHistory.tsx:120-128) — isDispatchingOwnHeartbeat flag set true before window.dispatchEvent, cleared in finally. Synchronous re-entry via the listener returns early on line 167. Confirmed by the dedicated regression test (does not self-suppress: solo polling stays at 5s cadence). Defensive try/finally is correct against listener throws.
  • Subset semantics (heartbeatCoverage.ts:11-22) — myActiveIds.length === 0 → trivially covered (no tick will start anyway, see maybeStartPoll early return on empty ids); payloadIds empty/undefined with non-empty myActiveIds → not covered (rules out a malformed dispatcher silently starving us). Symmetric with the SummaryListPage dispatcher contract.
  • Freshness check (ChatSummaryHistory.tsx:96-102) — Date.now() - lastCoveringEventTime <= COVERING_HEARTBEAT_WINDOW_MS uses <= (inclusive); the test explicitly steps past the boundary (+15500) to avoid relying on that edge. Match.
  • Cleanup symmetry — all 3 new addEventListener calls (lines 49-51) have matching removeEventListener in componentWillUnmount (lines 59-61). No leak.
  • handleListUnmount clears peerActive but not lastCoveringEventTime. Safe: the tick guard short-circuits on peerActive first; the stale timestamp is unreachable until the next heartbeat overwrites it. (Nit only — see below.)

Readability & maintainability

  • The // #334: comments on the new fields, listeners, and the self-echo block read like a postmortem of the trap — exactly the kind of "why" that earns its keep. handleBatchHeartbeat's comment block names the failure mode (peerActive=true from our own broadcast … starve our own next tick) so a future reader has the regression story in-place.
  • Name handleStatusChangeEvent is chosen explicitly to distinguish from SummaryListPage.handleStatusChange (filter-dropdown callback). Useful disambiguation called out in the comment.
  • The mountHistory test helper (__tests__/ChatSummaryHistory.test.tsx:91-105) is used by ~3 of the new tests (and 1 paused-guard test); some new tests still inline the render + vi.advanceTimersByTimeAsync(0) block. Mild inconsistency; not blocking.

Design fit

  • Joining the existing 3-event protocol rather than inventing a new channel is the right altitude for a minimal patch. The PR body acknowledges Approach C (singleton service, refactor(summary): extract summaryPollingService singleton (#334 follow-up — Approach C) #370) as the proper root fix and explicitly scopes this PR to the listener-side join. Aligned with maintainer triage.
  • Sharing COVERING_HEARTBEAT_WINDOW_MS from utils/heartbeatCoverage.ts and replacing the inline 15000 in SummaryDetailPage:353 is a clean small-DRY win that prevents future drift between protocol participants.
  • The new utils/heartbeatCoverage.ts is a pure function with no React/DOM dependency — testable in isolation, and the 7-case test suite exercises it that way.

Findings

  • Must Fix: 0
  • Should Fix: 0
  • Nit: 2
    • N1 — OWN_POLL_INTERVAL_MS could share a constant with the existing 5000 literal in ChatSummaryHistory.tsx:103 (test files hard-code 5000 / 15500 as well). Cosmetic; not worth churning this PR.
    • N2 — handleListUnmount could reset lastCoveringEventTime = 0 for tidiness, even though the guard already short-circuits on peerActive. Cosmetic.
  • Question: 0
  • Follow-ups acknowledged in PR body (out of scope, correctly deferred): isInFlight(status) predicate inlined at 4 sites; cadence unification (2 s/5 s/15 s); Approach C extraction → refactor(summary): extract summaryPollingService singleton (#334 follow-up — Approach C) #370.

Process note (not a code finding)

Verdict

APPROVED — minimal, well-targeted, evidence-backed (real-browser pre/post pollers + adversarial verify caught self-echo trap before any code shipped). Comment density tracks complexity; tests track the regression story; design intentionally stops short of Approach C and points to its follow-up issue.

— code persona drain complete (case-A first-fanout, 3/3).

@lml2468 lml2468 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Verdict: APPROVED — see verdict comment for correctness walk-through, readability/design fit, and 2 nits (constant extraction / unmount tidy-up, both cosmetic). Findings: 0 Must / 0 Should / 2 Nit. (code-reviewer persona, tick bb5bd690)

@lml2468 lml2468 added review:done:code:approve code-reviewer APPROVED and removed review:running:code code-reviewer review in progress labels Jun 27, 2026
@lml2468

lml2468 commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Aggregate Verdict: APPROVED

Three reviewer personas have completed their drain. All clear.

  • qa — PASS · Coverage solid: 7 cases on containsAllTaskIds (superset/equal/partial/disjoint/empty/large-payload) + 9 cases on ChatSummaryHistory heartbeat coordination including the self-echo regression. Real-browser pre/post-fix numbers (~42 → ~30 req / 60 s) confirm the integration claim.
  • security — CLEARED · No new endpoints, no new persisted state, no new data leaving the browser. Listener payload is a subset check over numeric/string taskIds; no eval, no innerHTML, no auth-affecting code paths touched.
  • code — APPROVED · Self-echo guard correctly bracketed in doPoll try/finally; freshness window centralised via exported COVERING_HEARTBEAT_WINDOW_MS; paused guard at the top of handleStatusChangeEvent; subset check correctness preserved; 5-file diff reads as the minimal surgical patch the spec called for.

Next step: awaiting human merge. The stale label is informational only — the work is complete and verified. No reviewer requested changes; no risks were filed.

🤖 Aggregated by review-lead (pr-tick-octo-web autopilot).

@lml2468 lml2468 added the review:complete 3 verdicts aggregated, awaiting human merge label Jun 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review:complete 3 verdicts aggregated, awaiting human merge review:done:code:approve code-reviewer APPROVED review:done:qa:approve qa-engineer PASS review:done:security:approve security-engineer CLEARED size/L PR size: L stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: sidebar smart-summary batch-status duplicate polling (uncoordinated multi-poller)

2 participants