From d7ea00a29013f8f03d10189c8782cdd1ec424cdb Mon Sep 17 00:00:00 2001 From: Gary Basin Date: Thu, 16 Jul 2026 21:41:43 -0400 Subject: [PATCH] fix(surface): a deleted message must not strand a channel's unread state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Delete the newest message in a channel and that channel goes unread forever, for everyone who had not already read it. Reloading lands you at the top of the scrollback with a "1 new" pill that points at nothing and a "Jump to latest" button that does nothing. #test-channel has been stuck like this since a QA message was posted and deleted. A deleted message with no replies renders no row: buildTimelineItems skips it. But it stays in `main` — the delete folds onto the posted event rather than dropping it — and everything that asks "what is the newest message?" still answered with it. So `isNewestMessageVisible` looked up a `data-eid` that no element carries, returned false forever, and mark-read never fired. The cursor could never advance past the delete, which is what made every symptom permanent and self-repeating: the landing scrolled to a divider that was never rendered, leaving scrollTop at 0. `isRenderableMessage` is now the single answer to "does this paint?", shared by the items builder, the unread count, the scroll landing, and `newestConfirmedMainEventId`. The read cursor keys off the newest message that actually paints, and a missing row falls back to the scroll position instead of reporting "not visible" forever — the deadlock is no longer representable. Server-side, `latest_event_id` counted the deleted message too, so the sidebar and push badge lit for content that no longer exists and named an id no client could ever reach. It now excludes deleted messages: the counter must only ever return an id a reader can actually see. That rule lived as two copies and had already drifted — the push badge never grew the broadcast clause and counted thread replies invisible in the feed — so both now share one definition. Separately, the "Agent answered" chip fired on every reload. Timeline mounts before the first history page lands, and the "have I seen this answer?" baseline was taken from that empty render, so the whole first page read as having just arrived. It now baselines once history loads. Verified against prod: the fixed counter returns 710 for #test-channel against a stuck cursor of 711, so the channel clears on deploy with no user action. It is the only channel in the database whose counter changes. --- surface/server/src/event-types.ts | 31 ++++++ surface/server/src/events/read.ts | 10 +- surface/server/src/push.ts | 8 +- surface/server/test/readCursors.test.ts | 34 ++++++ surface/shared/src/appState.ts | 8 +- surface/shared/src/timeline.ts | 14 +++ surface/shared/src/util.ts | 4 +- surface/web/src/components/Timeline.test.tsx | 107 ++++++++++++++++++- surface/web/src/components/Timeline.tsx | 40 +++++-- 9 files changed, 227 insertions(+), 29 deletions(-) diff --git a/surface/server/src/event-types.ts b/surface/server/src/event-types.ts index 1fc70f65c..79b6e6e1d 100644 --- a/surface/server/src/event-types.ts +++ b/surface/server/src/event-types.ts @@ -99,3 +99,34 @@ export const SYNC_CATCHUP_RAW_EVENT_TYPES = [ ...CATCHUP_RAW_EVENT_TYPES, ...SYNC_EVENT_TYPES.filter((type) => !TIMELINE_EVENT_TYPE_SET.has(type)), ]; + +// === the unread rule === + +// Events that make a channel unread. A thread reply counts only when it is +// broadcast: the agent's answer is an ordinary channel message and marks the +// channel unread like one, or the very thing you asked for lands below the fold +// with nothing to say it arrived. +const UNREAD_EVENT_TYPES = ['message.posted', 'session.spawned', 'session.replied'] as const; + +/** + * Per-channel `latest_event_id` — the newest event that should draw attention. + * Correlates against `c.id`, so it belongs inside a LATERAL over `channels c`. + * + * ONE definition on purpose. This rule lived as two copies (the channel list and + * the push badge) and they silently drifted: the badge never grew the broadcast + * clause, so it counted thread replies nobody could see in the feed. + * + * Deleted messages are excluded. The content is gone, so it must not light a + * channel — and a deleted message with no replies left renders no row at all, + * which made this counter unreachable: it named an id the client could never + * scroll to, so mark-read could never catch up and the channel stayed unread + * forever. Whatever id this returns, a client must be able to see and read it. + */ +export const CHANNEL_LATEST_EVENT_ID_SQL = ` + SELECT MAX(e.id) AS latest_event_id + FROM events e + LEFT JOIN message_state ms ON ms.event_id = e.id + WHERE e.channel_id = c.id + AND e.type IN ${sqlTypeList(UNREAD_EVENT_TYPES)} + AND (e.thread_root_event_id IS NULL OR (e.payload->>'broadcast')::boolean IS TRUE) + AND NOT COALESCE(ms.is_deleted, false)`; diff --git a/surface/server/src/events/read.ts b/surface/server/src/events/read.ts index f1c52fd53..8df91416e 100644 --- a/surface/server/src/events/read.ts +++ b/surface/server/src/events/read.ts @@ -1,6 +1,7 @@ import type { Db, DbClient } from '../db.js'; import { CATCHUP_RAW_EVENT_TYPES as CATCHUP_RAW_EVENT_TYPE_VALUES, + CHANNEL_LATEST_EVENT_ID_SQL, sqlTypeList, SYNC_CATCHUP_RAW_EVENT_TYPES as SYNC_CATCHUP_RAW_EVENT_TYPE_VALUES, SYNC_EVENT_TYPES as SYNC_EVENT_TYPE_VALUES, @@ -476,14 +477,7 @@ export async function listChannelsFor(pool: Db | DbClient, userId: string): Prom WHERE m.channel_id = c.id ) member_counts ON c.kind IN ('private', 'gdm') LEFT JOIN LATERAL ( - SELECT MAX(e.id) AS latest_event_id - FROM events e - WHERE e.channel_id = c.id - -- The agent's answer is an ordinary channel message, so it marks the - -- channel unread like one. Leaving it out would land the very thing - -- you asked for below the fold with nothing to say it had arrived. - AND e.type IN ('message.posted', 'session.spawned', 'session.replied') - AND (e.thread_root_event_id IS NULL OR (e.payload->>'broadcast')::boolean IS TRUE) + ${CHANNEL_LATEST_EVENT_ID_SQL} ) latest ON true -- === mentions-activity additions === LEFT JOIN LATERAL ( diff --git a/surface/server/src/push.ts b/surface/server/src/push.ts index f85b65002..d8b3c0b4a 100644 --- a/surface/server/src/push.ts +++ b/surface/server/src/push.ts @@ -6,6 +6,7 @@ import type { Db } from './db.js'; import type { WsHub } from './hub.js'; import type { WireEvent } from './events.js'; import { config } from './config.js'; +import { CHANNEL_LATEST_EVENT_ID_SQL } from './event-types.js'; import { memberUserIdsForChannel, resolveDirectMentionUserIds } from './mentions.js'; import { getWebPushSender, @@ -349,12 +350,7 @@ async function unreadChannelCountFor(pool: Db, userId: string): Promise LEFT JOIN channel_mutes mute ON mute.channel_id = c.id AND mute.user_id = $1 LEFT JOIN LATERAL ( - SELECT MAX(e.id) AS latest_event_id - FROM events e - WHERE e.channel_id = c.id - -- Matches the unread rule in events.ts: an agent's answer is a real - -- channel message and counts toward the badge like one. - AND e.type IN ('message.posted', 'session.spawned', 'session.replied') + ${CHANNEL_LATEST_EVENT_ID_SQL} ) latest ON true WHERE mute.user_id IS NULL AND COALESCE(latest.latest_event_id, 0) > COALESCE(rc.last_read_event_id, 0) diff --git a/surface/server/test/readCursors.test.ts b/surface/server/test/readCursors.test.ts index f65ec51f3..e69aabd40 100644 --- a/surface/server/test/readCursors.test.ts +++ b/surface/server/test/readCursors.test.ts @@ -1,3 +1,4 @@ +import { randomUUID } from 'node:crypto'; import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from 'vitest'; import type pg from 'pg'; import { buildApp } from '../src/app.js'; @@ -198,6 +199,39 @@ describe('read cursors', () => { expect(channel.latestEventId).toBe(two.id); }); + // A deleted message renders no row, so no client can scroll to it to mark it + // read. Counting one strands the channel unread forever — every id this + // counter reports has to be an id a reader can actually reach. + it('does not count a deleted message toward latestEventId', async () => { + const { cookie } = await login('alice', 'Alice'); + const kept = await post(cookie, fx.channelId, 'kept'); + const doomed = await post(cookie, fx.channelId, 'doomed'); + + const removed = await app.inject({ + method: 'DELETE', + url: `/api/messages/${doomed.id}`, + headers: { cookie }, + payload: { opId: randomUUID() }, + }); + expect(removed.statusCode).toBe(200); + + const channels = await app.inject({ method: 'GET', url: '/api/channels', headers: { cookie } }); + expect(channels.statusCode).toBe(200); + const channel = channels.json().channels.find((c: any) => c.id === fx.channelId); + expect(channel.latestEventId).toBe(kept.id); + + // The whole point: reading the newest surviving message clears the channel. + await app.inject({ + method: 'POST', + url: `/api/channels/${fx.channelId}/read`, + headers: { cookie }, + payload: { lastReadEventId: kept.id }, + }); + const settled = await app.inject({ method: 'GET', url: '/api/channels', headers: { cookie } }); + const after = settled.json().channels.find((c: any) => c.id === fx.channelId); + expect(after.latestEventId).toBeLessThanOrEqual(after.lastReadEventId); + }); + it('computes latestEventId from main-timeline-visible events only', async () => { const { cookie } = await login('alice', 'Alice'); const root = await postMessage(pool, { diff --git a/surface/shared/src/appState.ts b/surface/shared/src/appState.ts index e9ec0e17c..7413a4506 100644 --- a/surface/shared/src/appState.ts +++ b/surface/shared/src/appState.ts @@ -12,6 +12,7 @@ import { markFailed, mergeHistory, resetToLatest, + isRenderableMessage, mergeThread, removeByClientMsgId, rejectLocalOverlay, @@ -310,7 +311,12 @@ export function newestConfirmedMainEventId(t: ChannelTimeline | undefined): numb if (!t) return 0; for (let index = t.main.length - 1; index >= 0; index--) { const message = t.main[index]; - if (message?.status === 'confirmed' && typeof message.id === 'number') return message.id; + // Skip what never paints: callers compare this against a read cursor to ask + // "has that reader seen everything here?", and a deleted trailing message + // would answer no forever — nobody can read a row that renders nothing. + if (message?.status === 'confirmed' && typeof message.id === 'number' && isRenderableMessage(message)) { + return message.id; + } } return 0; } diff --git a/surface/shared/src/timeline.ts b/surface/shared/src/timeline.ts index 62bc7ae3f..b563e23e4 100644 --- a/surface/shared/src/timeline.ts +++ b/surface/shared/src/timeline.ts @@ -226,6 +226,20 @@ export function isHumanBroadcastReply(m: ChatMessage): boolean { return m.broadcast === true && m.sessionId == null && m.sessionEventType == null; } +/** + * Does this message produce a row in the feed? A deleted message keeps its + * tombstone only to host replies; with none left it renders nothing at all. + * + * It still lives in `main` (the delete folds onto the posted event rather than + * dropping it), so anything that asks "what is the newest message?" must ask + * this first. A row that can never paint can never be scrolled to, so keying + * mark-read or the unread count off one strands the read cursor permanently — + * deleting the newest message in a channel used to leave it unread forever. + */ +export function isRenderableMessage(m: ChatMessage): boolean { + return !(m.deleted === true && m.replyCount === 0); +} + export interface ChannelTimeline { /** Root messages: confirmed sorted by id asc, then pending/failed in send order. */ main: ChatMessage[]; diff --git a/surface/shared/src/util.ts b/surface/shared/src/util.ts index 396e5b4ca..45045e100 100644 --- a/surface/shared/src/util.ts +++ b/surface/shared/src/util.ts @@ -1,5 +1,5 @@ import type { Channel } from './api'; -import type { ChatMessage, UserRef } from './timeline'; +import { isRenderableMessage, type ChatMessage, type UserRef } from './timeline'; /** The person on the other side of a DM (yourself, for a self-DM). */ export function dmPartner(c: Channel, meId: string): UserRef | null { @@ -186,7 +186,7 @@ export function buildTimelineItems(messages: ChatMessage[]): TimelineItem[] { const items: TimelineItem[] = []; let prev: ChatMessage | null = null; for (const m of messages) { - if (m.deleted && m.replyCount === 0) continue; + if (!isRenderableMessage(m)) continue; const d = new Date(m.createdAt); if (!prev || !sameDay(new Date(prev.createdAt), d)) { items.push({ kind: 'day', key: `day-${d.toDateString()}`, label: formatDay(m.createdAt) }); diff --git a/surface/web/src/components/Timeline.test.tsx b/surface/web/src/components/Timeline.test.tsx index f8e8b8d73..14ddf1d8b 100644 --- a/surface/web/src/components/Timeline.test.tsx +++ b/surface/web/src/components/Timeline.test.tsx @@ -85,17 +85,19 @@ function renderTimeline({ unreadDividerAfterId = 1, onReachBottom, sessions = {}, + loaded = true, }: { messages?: ChatMessage[]; unreadDividerAfterId?: number | null; onReachBottom?: () => void; sessions?: Record; + loaded?: boolean; } = {}) { - const renderElement = (nextMessages: ChatMessage[]) => ( + const renderElement = (nextMessages: ChatMessage[], nextLoaded: boolean) => ( ); - const view = render(renderElement(messages)); - return { ...view, rerenderMessages: (nextMessages: ChatMessage[]) => view.rerender(renderElement(nextMessages)) }; + const view = render(renderElement(messages, loaded)); + return { + ...view, + rerenderMessages: (nextMessages: ChatMessage[], nextLoaded = true) => + view.rerender(renderElement(nextMessages, nextLoaded)), + }; } function setScrollMetrics( @@ -404,6 +410,99 @@ describe('Timeline anchored agent answers', () => { expect(HTMLElement.prototype.scrollIntoView).toHaveBeenCalled(); rect.mockRestore(); }); + + // Timeline mounts before the first history page lands (that is what the + // skeleton is for), so the "have I seen an answer before?" baseline must not + // be taken from that empty first render — every reload would replay the chip + // for an answer that arrived days ago. + it('does not show the jump chip for the first history page after an empty mount', async () => { + const rect = vi.spyOn(HTMLElement.prototype, 'getBoundingClientRect').mockImplementation(function ( + this: HTMLElement, + ) { + if (this.getAttribute('role') === 'log') { + return { top: 0, bottom: 200, left: 0, right: 300, width: 300, height: 200, x: 0, y: 0, toJSON: vi.fn() }; + } + if (this.getAttribute('data-eid') === '1') { + return { top: -120, bottom: -80, left: 0, right: 300, width: 300, height: 40, x: 0, y: -120, toJSON: vi.fn() }; + } + return { top: 20, bottom: 60, left: 0, right: 300, width: 300, height: 40, x: 0, y: 20, toJSON: vi.fn() }; + }); + const root = message({ id: 1, text: 'Please ship this carefully' }); + // The real mount order: the skeleton renders with no history, then the + // first page lands with the answer already in it. + const view = renderTimeline({ messages: [], loaded: false, unreadDividerAfterId: null }); + + view.rerenderMessages([{ ...root, replyCount: 1, lastReplyId: 9 }, answer()], true); + + await act(async () => {}); + expect(screen.queryByTestId('agent-answer-jump-chip')).toBeNull(); + rect.mockRestore(); + }); +}); + +// A deleted message with no replies renders no row at all (buildTimelineItems +// skips it), so nothing can scroll to it or mark it read. Every set that drives +// unread, scroll landing, and mark-read has to agree it isn't there — otherwise +// deleting the newest message in a channel strands the read cursor forever. +describe('Timeline deleted tail message', () => { + const deletedTail = () => message({ id: 3, text: '', deleted: true, replyCount: 0 }); + + function mockRowVisibility(visibleEid: string) { + return vi.spyOn(HTMLElement.prototype, 'getBoundingClientRect').mockImplementation(function (this: HTMLElement) { + if (this.getAttribute('role') === 'log') { + return { top: 0, bottom: 200, left: 0, right: 300, width: 300, height: 200, x: 0, y: 0, toJSON: vi.fn() }; + } + if (this.getAttribute('data-eid') === visibleEid) { + return { top: 150, bottom: 190, left: 0, right: 300, width: 300, height: 40, x: 0, y: 150, toJSON: vi.fn() }; + } + return { top: 420, bottom: 460, left: 0, right: 300, width: 300, height: 40, x: 0, y: 420, toJSON: vi.fn() }; + }); + } + + it('marks read when the newest rendered row is visible behind a deleted tail', () => { + const onReachBottom = vi.fn(); + const rect = mockRowVisibility('2'); + + renderTimeline({ + messages: [message({ id: 1, text: 'Message 1' }), message({ id: 2, text: 'Message 2' }), deletedTail()], + unreadDividerAfterId: 1, + onReachBottom, + }); + fireEvent.scroll(screen.getByRole('log', { name: 'Messages' })); + + expect(onReachBottom).toHaveBeenCalled(); + rect.mockRestore(); + }); + + it('does not count a deleted tail message as unread', () => { + renderTimeline({ + messages: [message({ id: 1, text: 'Message 1' }), message({ id: 2, text: 'Message 2' }), deletedTail()], + unreadDividerAfterId: 2, + }); + + const log = screen.getByRole('log', { name: 'Messages' }); + setScrollMetrics(log, { scrollHeight: 1000, clientHeight: 200 }); + log.scrollTop = 0; + fireEvent.scroll(log); + + expect(screen.queryByTestId('jump-to-unread')).toBeNull(); + }); + + it('lands at the bottom when the only unread message is a deleted one', () => { + const rect = mockRowVisibility('2'); + Object.defineProperty(HTMLDivElement.prototype, 'scrollHeight', { configurable: true, value: 1000 }); + Object.defineProperty(HTMLDivElement.prototype, 'clientHeight', { configurable: true, value: 200 }); + + renderTimeline({ + messages: [message({ id: 1, text: 'Message 1' }), message({ id: 2, text: 'Message 2' }), deletedTail()], + unreadDividerAfterId: 2, + }); + + expect(screen.getByRole('log', { name: 'Messages' }).scrollTop).toBe(1000); + Reflect.deleteProperty(HTMLDivElement.prototype, 'scrollHeight'); + Reflect.deleteProperty(HTMLDivElement.prototype, 'clientHeight'); + rect.mockRestore(); + }); }); describe('Timeline human broadcast replies', () => { diff --git a/surface/web/src/components/Timeline.tsx b/surface/web/src/components/Timeline.tsx index a5d2c8ab1..d32f24a63 100644 --- a/surface/web/src/components/Timeline.tsx +++ b/surface/web/src/components/Timeline.tsx @@ -1,7 +1,7 @@ import { memo, useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState } from 'react'; import type { ChatMessage, UserRef } from '@atrium/surface-client'; import type { Session } from '../sessions/types'; -import { buildTimelineItems, isAgentVoiceBroadcast } from '@atrium/surface-client'; +import { buildTimelineItems, isAgentVoiceBroadcast, isRenderableMessage } from '@atrium/surface-client'; import { ChevronDownIcon } from './icons'; import { MessageRow } from './MessageRow'; import type { MentionContext } from './useMentionTypeahead'; @@ -117,8 +117,13 @@ function TimelineImpl({ } const isAnchoredAnnotationEvent = (message: ChatMessage) => isAgentVoiceBroadcast(message) && message.threadRootEventId != null && rootIds.has(message.threadRootEventId); + // `isRenderableMessage` keeps a message that paints nothing (a deleted one + // with no replies left) out of every set derived from the feed — otherwise + // it becomes a "newest message" no scroll can ever reach. return { - visibleMessages: messages.filter((message) => !isAnchoredAnnotationEvent(message)), + visibleMessages: messages.filter( + (message) => isRenderableMessage(message) && !isAnchoredAnnotationEvent(message), + ), answersByRoot: answers, loadedRootIds: rootIds, }; @@ -144,6 +149,7 @@ function TimelineImpl({ const anchoredUnread = messages.find( (message) => (message.id ?? 0) > unreadDividerAfterId && + isRenderableMessage(message) && (message.sessionEventType === 'replied' || message.sessionEventType === 'question_requested' || (message.sessionId != null && message.sessionTask != null)) && @@ -153,9 +159,13 @@ function TimelineImpl({ if (anchoredUnread?.threadRootEventId != null) return anchoredUnread.threadRootEventId; return visibleMessages.find((message) => (message.id ?? 0) > unreadDividerAfterId)?.id ?? null; }, [loadedRootIds, messages, unreadDividerAfterId, visibleMessages]); + // Counted over `messages`, not `visibleMessages`: an anchored answer is + // presented inside its root's cluster, so it is genuinely something new to + // see. A message that paints nothing is not, and counting one strands the + // pill at "1 new" pointing at a row that does not exist. const unreadCount = useMemo(() => { if (unreadDividerAfterId == null || unreadDividerAfterId <= 0) return 0; - return messages.filter((m) => (m.id ?? 0) > unreadDividerAfterId).length; + return messages.filter((m) => isRenderableMessage(m) && (m.id ?? 0) > unreadDividerAfterId).length; }, [messages, unreadDividerAfterId]); const isAtBottom = useCallback((el: HTMLElement) => { @@ -166,15 +176,22 @@ function TimelineImpl({ return el.scrollHeight - el.scrollTop - el.clientHeight < PINNED_BOTTOM_SLOP_PX; }, []); + // `lastMessageId` is the newest message that actually paints, so its row is + // in the DOM whenever the feed has one. If it is missing anyway — nothing + // renderable loaded, or a row that went away between render and measure — + // trust the scroll position rather than reporting "not visible": a lookup + // that can never match would freeze the read cursor for good, which is + // exactly how a deleted trailing message used to make a channel permanently + // unread. Missing row + parked at the bottom means there is nothing below. const isNewestMessageVisible = useCallback(() => { const el = containerRef.current; - if (!el || lastMessageId == null) return false; - const latest = el.querySelector(`[data-eid="${lastMessageId}"]`); - if (!latest) return false; + if (!el) return false; + const latest = lastMessageId != null ? el.querySelector(`[data-eid="${lastMessageId}"]`) : null; + if (!latest) return isAtBottom(el); const latestRect = latest.getBoundingClientRect(); const containerRect = el.getBoundingClientRect(); return latestRect.bottom >= containerRect.top && latestRect.top <= containerRect.bottom; - }, [lastMessageId]); + }, [isAtBottom, lastMessageId]); const isRootVisible = useCallback((rootId: number) => { const container = containerRef.current; @@ -186,6 +203,13 @@ function TimelineImpl({ }, []); useEffect(() => { + // The chip announces an answer that arrives while you are looking. Timeline + // mounts before the first history page lands (that is what the skeleton is + // for), so a baseline taken from that empty render makes the whole first + // page read as "just arrived" — and every reload replayed the chip for an + // answer from days ago. `loaded` is the moment history is actually there: + // baseline from it, and only announce what shows up after. + if (!loaded) return; const latest = messages .filter( (message) => @@ -209,7 +233,7 @@ function TimelineImpl({ setAnswerChip({ answerId: latest.id!, rootId: latest.threadRootEventId, ask }); if (chipTimerRef.current) window.clearTimeout(chipTimerRef.current); chipTimerRef.current = window.setTimeout(() => setAnswerChip(null), 10_000); - }, [answersByRoot, isRootVisible, messages]); + }, [answersByRoot, isRootVisible, loaded, messages]); useEffect( () => () => {