diff --git a/surface/server/src/event-types.ts b/surface/server/src/event-types.ts index 1fc70f65..79b6e6e1 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 f1c52fd5..8df91416 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 f85b6500..d8b3c0b4 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 f65ec51f..e69aabd4 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 e9ec0e17..7413a450 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 62bc7ae3..b563e23e 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 396e5b4c..45045e10 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 f8e8b8d7..14ddf1d8 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 a5d2c8ab..d32f24a6 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( () => () => {