fix(web): count unseen messages by rendered block, not raw message - #1255
Conversation
The "N new messages" pill counted raw DecryptedMessages while the timeline renders folded blocks, so the two never agreed. A subagent run is dozens of sidechain messages but a single Task card; a tool_use and its tool_result are two messages and one card; consecutive tools collapse into one group. The pill could read "47 new messages" when scrolling down revealed two new rows. collectNewUnseenIds never inspected isSidechain, and it could not: the reducer's grouping is stateful (it needs the Task tool_use before it can map parentToolUseId), so a per-message predicate in the store cannot reproduce it. Adding an isSidechain check there would also invert the error for orphan sidechain messages, which tracer.ts falls back to emitting at the top level. Instead, drop the store's unseen bookkeeping entirely and count what the renderer actually produced. Watermark the visible blocks when the user scrolls away from the tail, then count the blocks past the last one they had seen. The count is anchor-based rather than timestamp-based because the blocks array is not monotonic in createdAt: messages sort by invokedAt ?? createdAt, so a queued message carries an old createdAt while sitting at the end. Anchoring also makes prepended history free, since older blocks land before the anchor. Known limit, documented at the call site: once the history window fills up, mergeIntoWindow trims incoming messages off the tail, so the pill reports 0 instead of a count. Under-reporting is preferable here, and returning to the tail force-refetches the latest page anyway.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
-
[Major] Preserve the stable local identity across optimistic-to-stored replacement - the watermark snapshots only
block.idatweb/src/chat/unseenBlocks.ts:13. HAPI replaces an optimistic user row with a stored row that has a differentidbut the samelocalId(web/src/lib/messages.ts:69), while the rendered user block uses the changing message ID (web/src/chat/reducerTimeline.ts:693). If the user scrolls into history after the consumed optimistic row becomes visible but before its stored echo arrives, the replacement is no longer recognized; the scan anchors on the previous block and increments the pill even though no new rendered row appeared.Suggested fix:
export type UnseenWatermark = { ids: Set<string> localIds: Set<string> } export function createUnseenWatermark(blocks: readonly VisibleChatBlock[]): UnseenWatermark { return { ids: new Set(blocks.map((block) => block.id)), localIds: new Set(blocks.flatMap((block) => 'localId' in block && block.localId ? [block.localId] : [] )) } } function isKnownBlock(block: VisibleChatBlock, watermark: UnseenWatermark): boolean { if (watermark.ids.has(block.id)) return true if ('localId' in block && block.localId && watermark.localIds.has(block.localId)) return true // existing tool-group identity handling... }
Summary
Review mode: initial. One Major correctness issue found in optimistic-message reconciliation.
Testing
Not run (automation; PR content reviewed statically and not executed). Missing regression coverage for a visible optimistic user block being replaced by a stored block with the same localId and a different id after watermark capture.
HAPI Bot
| } | ||
|
|
||
| export function createUnseenWatermark(blocks: readonly VisibleChatBlock[]): UnseenWatermark { | ||
| return { ids: new Set(blocks.map((block) => block.id)) } |
There was a problem hiding this comment.
[MAJOR] Preserve localId when recognizing seen blocks.
This snapshot stores only the current block IDs, but mergeMessages replaces an optimistic user row (id === localId) with the stored echo's different id while preserving localId (web/src/lib/messages.ts:69). User blocks are keyed from msg.id (web/src/chat/reducerTimeline.ts:693), so if the watermark is captured before that replacement, the same rendered row stops matching and is counted as a new message.
Suggested fix:
type UnseenWatermark = {
ids: Set<string>
localIds: Set<string>
}
const localIds = new Set(blocks.flatMap((block) =>
'localId' in block && block.localId ? [block.localId] : []
))
if (
watermark.ids.has(block.id)
|| ('localId' in block
&& block.localId
&& watermark.localIds.has(block.localId))
) {
return true
}Please add a regression test that captures a watermark with { id: localId, localId }, replaces it with { id: serverId, localId }, and asserts the unseen count remains zero.
The watermark snapshotted only block.id, but that id is not stable for the user's own messages: mergeMessages replaces an optimistic row with a stored row that keeps localId under a new server id, and the user block renders with the message id. Scrolling into history while an own message was still optimistic meant its echo anchored one block earlier and bumped the pill by one, with no new rendered row. Track localId alongside id in the watermark and match on either. Reported by HAPI Bot on tiann#1255.
|
Thanks — confirmed and fixed in 4bbd40e. I verified the two code facts the finding rests on before changing anything:
Then wrote the regression test first. My initial attempt passed against the unfixed code, which was misleading: with a single block in the window, the changed id made the scan find no anchor at all and hit the "everything was trimmed" fallback that returns 0. It only reproduces when the optimistic block sits after another seen block, so the anchor visibly retreats: That version failed with Applied essentially your suggested shape, with one adjustment:
|
There was a problem hiding this comment.
Findings
- [Major] One inline finding: the counter still counts pre-join assistant blocks rather than the final rows rendered by assistant-ui.
Questions
- None.
Summary
Review mode: follow-up after new commits. The prior localId reconciliation issue is addressed, but one Major correctness issue remains in the counting unit.
Testing
Not run (automation; PR content reviewed statically and not executed). Missing regression coverage for multiple adjacent assistant-role blocks that assistant-ui joins into one rendered card.
HAPI Bot
| } | ||
| for (let index = blocks.length - 1; index >= 0; index -= 1) { | ||
| if (isKnownBlock(blocks[index], watermark)) { | ||
| return blocks.length - 1 - index |
There was a problem hiding this comment.
[MAJOR] Count the final joined assistant rows.
visibleBlocks is still not one-to-one with what the thread renders. The runtime explicitly joins adjacent assistant-role blocks into one card (web/src/lib/assistant-runtime.ts:107 and web/src/lib/assistant-runtime.ts:637), but this return counts every block after the anchor. A response containing reasoning + text + a tool call therefore reports 3 new messages even though ThreadPrimitive.Messages shows one new card; appending another assistant block to an existing response card also increments the pill without adding a row.
Suggested fix:
function renderedRole(block: VisibleChatBlock): 'user' | 'assistant' | 'system' {
if (block.kind === 'user-text') return 'user'
if (block.kind === 'agent-event') return 'system'
if (block.kind === 'cli-output' && block.source === 'user') return 'user'
return 'assistant'
}
function countRenderedRowsAfter(blocks: readonly VisibleChatBlock[], anchor: number): number {
let count = 0
let previousRole = renderedRole(blocks[anchor])
for (let index = anchor + 1; index < blocks.length; index += 1) {
const role = renderedRole(blocks[index])
if (role !== 'assistant' || previousRole !== 'assistant') count += 1
previousRole = role
}
return count
}
// Inside the known-anchor branch:
return countRenderedRowsAfter(blocks, index)Add a regression test where a watermark ends at a user block and the next response produces adjacent reasoning/text/tool blocks; the expected count is 1.
visibleBlocks is still not one-to-one with rendered rows: assistant-ui joins a run of adjacent assistant-role blocks into a single card, so a response made of reasoning + text + a tool call was reported as three new messages instead of one, and appending another block to an in-flight response bumped the pill without adding a row. Walk the blocks after the anchor and only start a new row where the assistant run breaks. Role assignment is the part that would drift, so rather than restating it, visibleBlockRole moves from assistant-runtime.ts to toolGroups.ts (next to the VisibleChatBlock definition it describes) and both the runtime and the counter import the one copy. Reported by HAPI Bot on tiann#1255.
|
Confirmed and fixed in e102ed1 — you're right that One deviation from the suggested patch, and it's the point of this PR: a local
The row-walking loop itself matches your shape. Regression coverage added:
Also had to fix a fixture of my own:
|
There was a problem hiding this comment.
Findings
- No findings.
Questions
- None.
Summary
Review mode: follow-up after new commits. No correctness, security, regression, data-loss, performance, or maintainability issues found in the current diff. The previous Major about counting adjacent assistant-role blocks separately is addressed. Residual risk: no browser-level test exercises the real scroll transition, assistant-ui conversion, and indicator together; the added pure-count and React-hook tests cover the core behavior.
Testing
Not run (automation; PR content was reviewed statically and not executed). GitHub test and pr-review checks were pending at review time.
HAPI Bot
2 participants
Problem
The "N new messages" pill counts raw
DecryptedMessages, but the timeline renders folded blocks. The two units never agreed:tool_use+tool_result= +2So the pill could read "47 new messages" while scrolling down revealed two new rows.
Why not just skip sidechain messages in the store
collectNewUnseenIdsnever inspectedisSidechain, and it can't meaningfully: the reducer's grouping is stateful — it needs to have seen the Tasktool_usebeforeparentToolUseIdresolves to a group. A per-message predicate in the store cannot reproduce that.A naive
if (isSidechain) continuewould also invert the error for orphan sidechain messages, whichtracer.ts:139deliberately falls back to emitting at the top level — they'd be visible but uncounted. And it leaves thetool_result2× inflation untouched.Approach
Delete the store's unseen bookkeeping entirely and count what the renderer actually produced.
web/src/chat/unseenBlocks.ts— watermark the visible blocks when the user scrolls away from the tail, then count blocks past the last one they had seenweb/src/hooks/useUnseenBlockCount.ts— captures the watermark during render (not in an effect), so the first frame after leaving the tail reports 0 rather than flashing a stale countmessage-window-store.ts— dropsunseenIds,unseenCount,collectNewUnseenIds,unseenIdentity, thecountUnseenoption and its three call sites (net −44 lines).viewModenow flows down the props path thatunseenCountused, keeping it in the sameuseSyncExternalStoresnapshot asmessagesso the two can't tear.Also removed a dead conjunct in
activateMessageWindow:viewMode === 'tail' && unseenIds.size > 0was unreachable, since every path into tail mode cleared the set.Anchor-based, not timestamp-based
countUnseenBlocksfinds the last block the watermark recognizes and counts what follows, rather than comparingcreatedAt. The blocks array is not monotonic increatedAt—compareMessagessorts byinvokedAt ?? createdAt, so a queued message sits at the end while carrying an oldcreatedAt. Anchoring also makes prepended history free (older blocks land before the anchor) and is immune to optimistic rows changing both id and timestamp when the server row replaces them.Group ids shift when a lone tool-call is absorbed into a group (
createToolGroupId), so the anchor check also matches onfirstToolId/lastToolId.Behaviour notes
tool_resultcompleting a card, a subagent working inside a Task card) no longer bump the count — only genuinely new top-level blocks do.HISTORY_WINDOW_SIZE),mergeIntoWindowtrims incoming messages off the tail, so the pill reports 0 instead of a count. Under-reporting beats the old inflation, and entering tail mode force-refetches the latest page anyway.Testing
web/src/chat/unseenBlocks.test.tsbuilds blocks through the realreduceChatBlocks+buildVisibleChatBlockspipeline rather than hand-rolling them — covering the 30-message subagent run, tool grouping,tool_resultcompletion, history pagination, group-id churn, and a trimmed-out watermark. The subagent case assertstaskCard.children.length === 30so it can't pass by messages being silently dropped upstream.web/src/hooks/useUnseenBlockCount.test.tscovers what only appears under React: watermark capture timing, convergence of the render-phasesetState, and that re-opening a session left in history mode doesn't capture a watermark the user never saw.bun run typecheckclean;bun run buildsucceeds.