From d2670751275f3a0c53e706f079095ee159cb290b Mon Sep 17 00:00:00 2001 From: Jack Kingsman Date: Sat, 25 Jul 2026 19:35:40 -0700 Subject: [PATCH] Add testing harness and fix up a few niggling bugs --- AGENTS.md | 4 +- app/AGENTS.md | 5 +- app/repository/messages.py | 50 +++++-- frontend/AGENTS.md | 20 ++- frontend/harness/drive.mjs | 131 +++++++++++++++++ frontend/harness/index.html | 11 ++ frontend/harness/main.tsx | 136 ++++++++++++++++++ frontend/harness/vite.config.ts | 15 ++ frontend/src/App.tsx | 57 +++++++- frontend/src/components/MessageList.tsx | 52 ++++++- frontend/src/hooks/useUnreadCounts.ts | 48 +++++-- frontend/src/test/appFavorites.test.tsx | 1 + frontend/src/test/appSearchJump.test.tsx | 1 + frontend/src/test/appStartupHash.test.tsx | 1 + .../src/test/unreadMarkerResolution.test.ts | 56 ++++++++ frontend/src/test/useUnreadCounts.test.ts | 70 +++++++++ tests/test_api.py | 48 +++++++ 17 files changed, 664 insertions(+), 42 deletions(-) create mode 100644 frontend/harness/drive.mjs create mode 100644 frontend/harness/index.html create mode 100644 frontend/harness/main.tsx create mode 100644 frontend/harness/vite.config.ts create mode 100644 frontend/src/test/unreadMarkerResolution.test.ts diff --git a/AGENTS.md b/AGENTS.md index 9fca0cb..e9a5552 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -375,7 +375,7 @@ All endpoints are prefixed with `/api` (e.g., `/api/health`). | POST | `/api/packets/region-backfill` | Re-resolve region scope for stored channel messages with retained raw packets | | POST | `/api/packets/decrypt/historical` | Decrypt stored packets | | POST | `/api/packets/maintenance` | Delete old packets and vacuum | -| GET | `/api/read-state/unreads` | Server-computed unread counts, mentions, last message times, and `last_read_ats` boundaries | +| GET | `/api/read-state/unreads` | Server-computed unread counts, mentions, last message times, `last_read_ats` boundaries, and `first_unread_ids` (unread-divider anchor) | | POST | `/api/read-state/mark-all-read` | Mark all conversations as read | | GET | `/api/settings` | Get app settings | | PATCH | `/api/settings` | Update app settings | @@ -440,7 +440,7 @@ Read state (`last_read_at`) is tracked **server-side** for consistency across de - Stored as Unix timestamp in `contacts.last_read_at` and `channels.last_read_at` - Updated via `POST /api/contacts/{public_key}/mark-read` and `POST /api/channels/{key}/mark-read` - Bulk update via `POST /api/read-state/mark-all-read` -- Aggregated counts via `GET /api/read-state/unreads` (server-side computation of counts, mention flags, `last_message_times`, and `last_read_ats`) +- Aggregated counts via `GET /api/read-state/unreads` (server-side computation of counts, mention flags, `last_message_times`, `last_read_ats`, and `first_unread_ids`) **State Tracking Keys (Frontend)**: Generated by `getStateKey()` for message times (sidebar sorting): - Channels: `channel-{channel_key}` diff --git a/app/AGENTS.md b/app/AGENTS.md index d5d6a47..55685ef 100644 --- a/app/AGENTS.md +++ b/app/AGENTS.md @@ -124,7 +124,8 @@ app/ ### Read/unread state - Server is source of truth (`contacts.last_read_at`, `channels.last_read_at`). -- `GET /api/read-state/unreads` returns counts, mention flags, `last_message_times`, and `last_read_ats`. +- `GET /api/read-state/unreads` returns counts, mention flags, `last_message_times`, `last_read_ats`, and `first_unread_ids`. +- `first_unread_ids` maps stateKey -> id of the oldest unread message, so the client can anchor the unread divider (and jump to it) without paging back through history. It is computed with `ROW_NUMBER() OVER (PARTITION BY type, conversation_key ORDER BY received_at, id)` — deliberately not `MIN(received_at)` with a bare id, because sender timestamps are whole seconds and same-second ties are routine, and not `MIN(id)`, because historical decryption inserts old messages with new ids. ### DM ingest + ACKs @@ -270,7 +271,7 @@ Web Push is a standalone subsystem in `app/push/`, separate from the fanout modu - `POST /packets/maintenance` ### Read state -- `GET /read-state/unreads` — counts, mention flags, `last_message_times`, and `last_read_ats` +- `GET /read-state/unreads` — counts, mention flags, `last_message_times`, `last_read_ats`, and `first_unread_ids` - `POST /read-state/mark-all-read` ### Settings diff --git a/app/repository/messages.py b/app/repository/messages.py index 0338993..e394d84 100644 --- a/app/repository/messages.py +++ b/app/repository/messages.py @@ -727,11 +727,7 @@ class MessageRepository: mention_flags: dict[str, bool] = {} last_message_times: dict[str, int] = {} last_read_ats: dict[str, int | None] = {} - # id of the oldest unread message per conversation. Rides the aggregate - # queries below via SQLite's bare-column rule: with exactly one MIN() in - # the query, bare columns come from the row that produced the minimum. - # Deliberately not MIN(id) — historical decryption inserts old messages - # with new ids, so id order and received_at order can disagree. + # id of the oldest unread message per conversation. first_unread_ids: dict[str, int | None] = {} mention_token = f"@[{name}]" if name else None @@ -760,9 +756,7 @@ class MessageRepository: SUM(CASE WHEN ? <> '' AND INSTR(LOWER(m.text), LOWER(?)) > 0 THEN 1 ELSE 0 - END) > 0 as has_mention, - MIN(m.received_at) as first_unread_at, - m.id as first_unread_id + END) > 0 as has_mention FROM messages m JOIN channels c ON m.conversation_key = c.key WHERE m.type = 'CHAN' AND m.outgoing = 0 @@ -777,7 +771,6 @@ class MessageRepository: for row in rows: state_key = f"channel-{row['conversation_key']}" counts[state_key] = row["unread_count"] - first_unread_ids[state_key] = row["first_unread_id"] if mention_token and row["has_mention"]: mention_flags[state_key] = True @@ -789,9 +782,7 @@ class MessageRepository: SUM(CASE WHEN ? <> '' AND INSTR(LOWER(m.text), LOWER(?)) > 0 THEN 1 ELSE 0 - END) > 0 as has_mention, - MIN(m.received_at) as first_unread_at, - m.id as first_unread_id + END) > 0 as has_mention FROM messages m LEFT JOIN contacts ct ON m.conversation_key = ct.public_key WHERE m.type = 'PRIV' AND m.outgoing = 0 @@ -805,7 +796,6 @@ class MessageRepository: for row in rows: state_key = f"contact-{row['conversation_key']}" counts[state_key] = row["unread_count"] - first_unread_ids[state_key] = row["first_unread_id"] if mention_token and row["has_mention"]: mention_flags[state_key] = True @@ -829,6 +819,40 @@ class MessageRepository: for row in rows: last_read_ats[f"contact-{row['public_key']}"] = row["last_read_at"] + # Oldest unread message per conversation. ROW_NUMBER rather than + # MIN(received_at) with a bare id: sender timestamps are whole seconds + # (a protocol constraint, see AGENTS.md), so several unread messages + # routinely share the oldest second and SQLite's bare-column rule only + # promises *a* row holding the minimum. Ordering by (received_at, id) + # picks the same message the client's own ordering does. + async with conn.execute( + f""" + WITH ranked AS ( + SELECT m.type, m.conversation_key, m.id, + ROW_NUMBER() OVER ( + PARTITION BY m.type, m.conversation_key + ORDER BY m.received_at ASC, m.id ASC + ) AS rn + FROM messages m + LEFT JOIN channels c ON m.type = 'CHAN' AND m.conversation_key = c.key + LEFT JOIN contacts ct ON m.type = 'PRIV' AND m.conversation_key = ct.public_key + WHERE m.outgoing = 0 + AND m.received_at > COALESCE( + CASE WHEN m.type = 'CHAN' THEN c.last_read_at ELSE ct.last_read_at END, + 0 + ) + AND (m.type <> 'CHAN' OR COALESCE(c.muted, 0) = 0) + {blocked_sql} + ) + SELECT type, conversation_key, id FROM ranked WHERE rn = 1 + """, + blocked_params, + ) as cursor: + rows = await cursor.fetchall() + for row in rows: + prefix = "channel" if row["type"] == "CHAN" else "contact" + first_unread_ids[f"{prefix}-{row['conversation_key']}"] = row["id"] + async with conn.execute( f""" SELECT type, conversation_key, MAX(received_at) as last_message_time diff --git a/frontend/AGENTS.md b/frontend/AGENTS.md index 966e196..3cc289f 100644 --- a/frontend/AGENTS.md +++ b/frontend/AGENTS.md @@ -243,7 +243,7 @@ High-level state is delegated to hooks: - `useConversationNavigation`: search target, conversation selection reset, and info-pane state - `useConversationActions`: send/resend/trace/path-discovery/block handlers and channel override updates - `useConversationMessages`: conversation switch loading, embedded conversation-scoped cache, jump-target loading, pagination, dedup/update helpers, reconnect reconciliation, and pending ACK buffering -- `useUnreadCounts`: unread counters, mention tracking, recent-sort timestamps, and server `last_read_ats` boundaries +- `useUnreadCounts`: unread counters, mention tracking, recent-sort timestamps, server `last_read_ats`, and `first_unread_ids` (the unread-divider anchor) - `useRealtimeAppState`: typed WS event application, reconnect recovery, cache/unread coordination - `useRepeaterDashboard`: repeater dashboard state (login, pane data/retries, console, actions) @@ -292,6 +292,16 @@ High-level state is delegated to hooks: - Packet feed/visualizer render keys and dedup logic should use `observation_id` (fallback to `id` only for older payloads). - The dedicated raw packet feed view now includes a frontend-only stats drawer. It tracks a separate lightweight per-observation session history for charts/rankings, so its windows are not limited by the visible packet list cap. Coverage messaging should stay honest when detailed in-memory stats history has been trimmed or the selected window predates the current browser session. +### Virtualization (`MessageList`) + +The message list is windowed with `@tanstack/react-virtual`; only the visible rows are mounted, so render cost no longer scales with conversation length. Three details are load-bearing and easy to break: + +- **`scrollMargin`** is measured from the virtual spacer's offset within the scroll container, because the container carries `p-4` and can show an "older messages" banner above the rows. Without it every `scrollToIndex` with `start`/`center` lands 16–48px high, and the error shifts as the banner appears during pagination. Rows must subtract it back out in their `translateY`. +- **The bottom-pin is deferred and re-asserted** across a bounded run of frames rather than performed once, because row heights start as estimates and a single `scrollToIndex` gets undone as they converge (completely so under StrictMode's double-invoked effects). It is cancelled by a pending `targetMessageId` and by any deliberate scroll gesture. +- **`getItemKey` returns a string sentinel** for indices past the end of a shrunken list; a bare index would collide with the numeric message-id keyspace and poison the measurement cache. + +jsdom has no layout engine, so none of this is observable from the vitest suite — it needs a real browser. + ### Radio settings behavior - `SettingsRadioSection.tsx` surfaces `path_hash_mode` only when `config.path_hash_mode_supported` is true. @@ -375,7 +385,11 @@ Note: MQTT, bot, and community MQTT settings were migrated to the `fanout_config `RawPacket.decrypted_info` includes `channel_key` and `contact_key` for MQTT topic routing. -`UnreadCounts` includes `counts`, `mentions`, `last_message_times`, and `last_read_ats`. The unread-boundary/jump-to-unread behavior uses the server-provided `last_read_ats` map keyed by `getStateKey(...)`. +`UnreadCounts` includes `counts`, `mentions`, `last_message_times`, `last_read_ats`, and `first_unread_ids`. + +The unread divider is anchored to `first_unread_ids` — the id of the oldest unread message per conversation — not to a timestamp. `MessageList` locates it with `findIndex(msg.id === unreadMarkerMessageId)`, which returns `-1` when that message is not in the loaded window; that is the signal to offer "Jump to unread" (routed through the `targetMessageId`/`getMessagesAround` path) rather than render a divider. Locating by timestamp instead would return index 0 whenever the boundary sits further back than the loaded window, silently placing the divider on the wrong message. + +Counts are incremented live over WebSocket while `first_unread_ids` only arrives with a full `/read-state/unreads` fetch, so `useUnreadCounts.incrementUnread` seeds the boundary itself on the read→unread transition. A channel going unread while the app is open would otherwise have a count but no boundary, and no divider at all. ## Contact Info Pane @@ -435,7 +449,7 @@ The `SearchView` component (`components/SearchView.tsx`) provides full-text sear - **State**: `targetMessageId` is shared between `useConversationNavigation` and `useConversationMessages`. When a search result is clicked, `handleNavigateToMessage` sets the target ID and switches to the target conversation. - **Same-conversation clear**: when `targetMessageId` is cleared after the target is reached, the hook preserves the around-loaded mid-history view instead of replacing it with the latest page. - **Persistence**: `SearchView` stays mounted after first open using the same `hidden` class pattern as `CrackerPanel`, preserving search state when navigating to results. -- **Jump-to-message**: `useConversationMessages` handles optional `targetMessageId` by calling `api.getMessagesAround()` instead of the normal latest-page fetch, loading context around the target message. `MessageList` scrolls to the target via `data-message-id` attribute and applies a `message-highlight` CSS animation. +- **Jump-to-message**: `useConversationMessages` handles optional `targetMessageId` by calling `api.getMessagesAround()` instead of the normal latest-page fetch, loading context around the target message. `MessageList` resolves the target to an index and calls `virtualizer.scrollToIndex(...)`, then applies a `message-highlight` CSS animation. A pending target suppresses the bottom-pin (see Virtualization below), since the around-load clears the list first and would otherwise be yanked to the newest message. - **Bidirectional pagination**: After jumping mid-history, `hasNewerMessages` enables forward pagination via `fetchNewerMessages`. The scroll-to-bottom button calls `jumpToBottom` (re-fetches latest page) instead of just scrolling. - **WS message suppression**: When `hasNewerMessages` is true, incoming WS messages for the active conversation are not added to the message list (the user is viewing historical context, not the latest page). diff --git a/frontend/harness/drive.mjs b/frontend/harness/drive.mjs new file mode 100644 index 0000000..1789ad0 --- /dev/null +++ b/frontend/harness/drive.mjs @@ -0,0 +1,131 @@ +/** + * Minimal CDP driver: launches the Playwright-cached Chromium, loads the harness, + * and evaluates assertions in-page. No dependencies — Node 22+ ships fetch/WebSocket. + * + * Usage: node harness/drive.mjs + */ +import { spawn } from 'node:child_process'; +import { setTimeout as sleep } from 'node:timers/promises'; + +const URL_ARG = process.argv[2] ?? 'http://localhost:5199/'; +const CHROME = + process.env.CHROME_BIN ?? + `${process.env.HOME}/.cache/ms-playwright/chromium-1223/chrome-linux64/chrome`; +const PORT = 9333; + +const chrome = spawn( + CHROME, + [ + '--headless=new', + `--remote-debugging-port=${PORT}`, + '--no-sandbox', + '--disable-gpu', + '--disable-dev-shm-usage', + '--window-size=1000,700', + // Start blank so the driver can enable Runtime before anything renders, + // otherwise early console output is missed. + 'about:blank', + ], + { stdio: 'ignore' } +); + +let ws; +let nextId = 1; +const pending = new Map(); + +function send(method, params = {}) { + const id = nextId++; + ws.send(JSON.stringify({ id, method, params })); + return new Promise((resolve, reject) => pending.set(id, { resolve, reject })); +} + +async function evaluate(expression) { + const res = await send('Runtime.evaluate', { + expression, + awaitPromise: true, + returnByValue: true, + }); + if (res.exceptionDetails) { + throw new Error( + res.exceptionDetails.exception?.description ?? JSON.stringify(res.exceptionDetails) + ); + } + return res.result.value; +} + +async function connect() { + for (let i = 0; i < 100; i++) { + try { + const list = await fetch(`http://127.0.0.1:${PORT}/json/list`).then((r) => r.json()); + const page = list.find((t) => t.type === 'page' && t.webSocketDebuggerUrl); + if (page) return page.webSocketDebuggerUrl; + } catch { + /* not up yet */ + } + await sleep(100); + } + throw new Error('Chromium did not expose a debugging target'); +} + +async function main() { + const wsUrl = await connect(); + ws = new WebSocket(wsUrl); + await new Promise((resolve, reject) => { + ws.addEventListener('open', resolve, { once: true }); + ws.addEventListener('error', reject, { once: true }); + }); + ws.addEventListener('message', (ev) => { + const msg = JSON.parse(ev.data); + if (msg.id && pending.has(msg.id)) { + const { resolve, reject } = pending.get(msg.id); + pending.delete(msg.id); + msg.error ? reject(new Error(JSON.stringify(msg.error))) : resolve(msg.result); + } + }); + + const consoleLines = []; + ws.addEventListener('message', (ev) => { + const msg = JSON.parse(ev.data); + if (msg.method === 'Runtime.consoleAPICalled') { + consoleLines.push(msg.params.args.map((a) => a.value ?? a.description).join(' ')); + } + }); + globalThis.__consoleLines = consoleLines; + + await send('Runtime.enable'); + await send('Page.enable'); + await send('Page.navigate', { url: URL_ARG }); + + // Wait for the harness to mount and settle. + for (let i = 0; i < 100; i++) { + const ready = await evaluate( + `!!document.querySelector('[data-message-id]') && !!window.__setHarness` + ).catch(() => false); + if (ready) break; + await sleep(100); + } + await evaluate(`new Promise(r => requestAnimationFrame(() => requestAnimationFrame(r)))`); + + const script = process.env.HARNESS_SCRIPT; + // A wedged page (render loop) would otherwise leave evaluate pending forever. + const out = await Promise.race([ + evaluate(script), + sleep(20000).then(() => { + throw new Error('page did not respond within 20s (main thread wedged?)'); + }), + ]); + console.log(JSON.stringify(out, null, 2)); + if (process.env.HARNESS_CONSOLE === '1') { + console.log('--- page console ---'); + for (const line of globalThis.__consoleLines) console.log(' ' + line); + } + + ws.close(); + chrome.kill(); +} + +main().catch((err) => { + console.error('DRIVER ERROR:', err.message); + chrome.kill(); + process.exit(1); +}); diff --git a/frontend/harness/index.html b/frontend/harness/index.html new file mode 100644 index 0000000..4106610 --- /dev/null +++ b/frontend/harness/index.html @@ -0,0 +1,11 @@ + + + + + MessageList layout harness + + +
+ + + diff --git a/frontend/harness/main.tsx b/frontend/harness/main.tsx new file mode 100644 index 0000000..5c2b6d2 --- /dev/null +++ b/frontend/harness/main.tsx @@ -0,0 +1,136 @@ +/** + * Real-layout harness for MessageList. + * + * jsdom has no layout engine, so windowing and scroll anchoring cannot be + * verified there. This mounts the real component in a real browser at a fixed + * viewport and exposes a control surface on `window` for a CDP driver. + */ +import { createRoot } from 'react-dom/client'; +import { StrictMode, useMemo, useState } from 'react'; +import { MessageList } from '../src/components/MessageList'; +import type { Message } from '../src/types'; +import '../src/index.css'; + +const CHANNEL_KEY = 'AA'.repeat(16); + +function makeMessage(i: number, opts: { long?: boolean } = {}): Message { + const body = opts.long + ? `message ${i} ` + 'wrapped filler text to force multi-line layout '.repeat(4) + : `message ${i}`; + return { + id: i + 1, + type: 'CHAN', + conversation_key: CHANNEL_KEY, + text: `Alice: ${body}`, + sender_timestamp: 1700000000 + i, + received_at: 1700000000 + i, + paths: null, + txt_type: 0, + signature: null, + sender_key: null, + outgoing: false, + acked: 0, + sender_name: 'Alice', + }; +} + +function buildMessages(count: number, longEvery = 0): Message[] { + return Array.from({ length: count }, (_, i) => + makeMessage(i, { long: longEvery > 0 && i % longEvery === 0 }) + ); +} + +interface HarnessState { + count: number; + longEvery: number; + hasOlderMessages: boolean; + loadingOlder: boolean; + targetMessageId: number | null; + unreadMarkerLastReadAt: number | null | undefined; + prepended: number; +} + +function Harness() { + const q = new URLSearchParams(location.search); + const [state, setState] = useState({ + count: Number(q.get('count') ?? 500), + longEvery: Number(q.get('longEvery') ?? 0), + hasOlderMessages: q.get('hasOlder') === '1', + loadingOlder: false, + targetMessageId: null, + // ?unread=1 puts the unread boundary before all loaded history, like a channel + // with thousands unread. + unreadMarkerLastReadAt: q.get('unread') === '1' ? 1600000000 : undefined, + prepended: 0, + }); + + // Prepending shifts ids negative so existing rows keep their identity, which is + // what the real app does when older history loads in. Memoized so the array + // identity is stable across unrelated re-renders, matching the real app. + const messages = useMemo(() => { + const base = buildMessages(state.count, state.longEvery); + const older = Array.from({ length: state.prepended }, (_, i) => { + const m = makeMessage(-(state.prepended - i)); + m.id = -(state.prepended - i); + m.received_at = 1700000000 - (state.prepended - i); + m.sender_timestamp = m.received_at; + return m; + }); + return [...older, ...base]; + }, [state.count, state.longEvery, state.prepended]); + + (window as unknown as Record).__setHarness = (next: Partial) => + setState((prev) => ({ ...prev, ...next })); + (window as unknown as Record).__messages = messages; + + return ( +
+ +
+ ); +} + +const params = new URLSearchParams(location.search); + +// Churn recorder. Installed before the first render so it captures the initial +// mount, which is where a large history does its flickering. +{ + const churn = { rowEvents: 0, windows: new Set(), frames: 0 }; + (window as unknown as Record).__churn = churn; + const obs = new MutationObserver((recs) => { + for (const r of recs) churn.rowEvents += r.addedNodes.length + r.removedNodes.length; + }); + obs.observe(document.getElementById('root')!, { childList: true, subtree: true }); + const t0 = performance.now(); + const sample = () => { + churn.frames++; + const ids = [...document.querySelectorAll('[data-message-id]')].map((e) => + e.getAttribute('data-message-id') + ); + if (ids.length) churn.windows.add(ids[0] + ':' + ids[ids.length - 1]); + if (performance.now() - t0 < 4000) requestAnimationFrame(sample); + else obs.disconnect(); + }; + requestAnimationFrame(sample); +} + +// ?strict=0 isolates StrictMode's double-mount (dev-only) from real behavior. +const useStrict = params.get('strict') !== '0'; +createRoot(document.getElementById('root')!).render( + useStrict ? ( + + + + ) : ( + + ) +); diff --git a/frontend/harness/vite.config.ts b/frontend/harness/vite.config.ts new file mode 100644 index 0000000..6e1a3b6 --- /dev/null +++ b/frontend/harness/vite.config.ts @@ -0,0 +1,15 @@ +import { defineConfig } from 'vite'; +import react from '@vitejs/plugin-react'; +import path from 'path'; + +export default defineConfig({ + root: path.resolve(__dirname), + plugins: [react()], + resolve: { alias: { '@': path.resolve(__dirname, '../src') } }, + server: { + port: 5199, + fs: { allow: [path.resolve(__dirname, '..')] }, + // git checkout/stash swaps inodes and loses the default watcher. + watch: { usePolling: true, interval: 300 }, + }, +}); diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index d658cae..19b8eb6 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -27,7 +27,13 @@ import { RichPayloadProvider } from './contexts/RichPayloadContext'; import { usePush } from './contexts/PushSubscriptionContext'; import { messageContainsMention } from './utils/messageParser'; import { getStateKey } from './utils/conversationState'; -import type { BulkCreateHashtagChannelsResult, Channel, Conversation, RawPacket } from './types'; +import type { + BulkCreateHashtagChannelsResult, + Channel, + Conversation, + Message, + RawPacket, +} from './types'; import { CONTACT_TYPE_REPEATER, CONTACT_TYPE_ROOM } from './types'; import { shouldAutoFocusInput } from './utils/autoFocusInput'; @@ -43,6 +49,34 @@ interface NewMessagePrefillRequest { nonce: number; } +/** + * Which message the unread divider should sit on. + * + * Normally the server's first-unread id. The exception is a channel that has + * never been read: its true boundary is the first message ever sent there, so + * offering to jump would haul the reader to the start of history for no gain. + * Everything loaded is unread in that case, so the divider belongs at the top of + * the window — which is what the pre-id behaviour did, and it is genuinely the + * more useful answer. + */ +export function resolveUnreadMarkerId( + boundaryId: number | null, + lastReadAt: number | null, + messages: Message[] +): number | null { + if (boundaryId === null) return null; + if (lastReadAt !== null) return boundaryId; + if (messages.length === 0) return boundaryId; + if (messages.some((msg) => msg.id === boundaryId)) return boundaryId; + + const oldestLoaded = messages.reduce((oldest, msg) => { + if (msg.received_at < oldest.received_at) return msg; + if (msg.received_at === oldest.received_at && msg.id < oldest.id) return msg; + return oldest; + }, messages[0]); + return oldestLoaded.id; +} + export function App() { const quoteSearchOperatorValue = useCallback((value: string) => { return `"${value.replace(/\\/g, '\\\\').replace(/"/g, '\\"')}"`; @@ -311,6 +345,7 @@ export function App() { unreadCounts, mentions, lastMessageTimes, + unreadLastReadAts, firstUnreadIds, recordMessageEvent, renameConversationState, @@ -344,17 +379,23 @@ export function App() { const activeChannelId = activeConversation.id; const activeChannelUnreadCount = unreadCounts[getStateKey('channel', activeChannelId)] ?? 0; + const boundaryId = firstUnreadIds[getStateKey('channel', activeChannelId)] ?? null; + setChannelUnreadMarker((prev) => { if (prev?.channelId === activeChannelId) { + // Same channel: hold the marker steady so it does not move under the + // reader, except to fill in a boundary we did not have yet. A marker + // created before /unreads resolved would otherwise stay blank for as long + // as the user stays put. + if (prev.messageId === null && boundaryId !== null) { + return { channelId: activeChannelId, messageId: boundaryId }; + } return prev; } if (activeChannelUnreadCount <= 0) { return null; } - return { - channelId: activeChannelId, - messageId: firstUnreadIds[getStateKey('channel', activeChannelId)] ?? null, - }; + return { channelId: activeChannelId, messageId: boundaryId }; }); }, [activeConversation, unreadCounts, firstUnreadIds]); @@ -539,7 +580,11 @@ export function App() { unreadMarkerMessageId: activeConversation?.type === 'channel' && channelUnreadMarker?.channelId === activeConversation.id - ? channelUnreadMarker.messageId + ? resolveUnreadMarkerId( + channelUnreadMarker.messageId, + unreadLastReadAts[getStateKey('channel', activeConversation.id)] ?? null, + messages + ) : undefined, onNavigateToUnread: (messageId: number) => setTargetMessageId(messageId), targetMessageId, diff --git a/frontend/src/components/MessageList.tsx b/frontend/src/components/MessageList.tsx index 3f28898..59a7733 100644 --- a/frontend/src/components/MessageList.tsx +++ b/frontend/src/components/MessageList.tsx @@ -422,6 +422,11 @@ export function MessageList({ // fired once and marked done. const pendingBottomScrollRef = useRef(false); const [bottomScrollNonce, setBottomScrollNonce] = useState(0); + const virtualSpacerRef = useRef(null); + // Distance from the scroll container's content origin down to the first row. + // Non-zero because the container carries p-4 and can show a loading/older + // banner above the rows; see the scrollMargin note on the virtualizer. + const [scrollMargin, setScrollMargin] = useState(0); const [showScrollToBottom, setShowScrollToBottom] = useState(false); const [selectedPath, setSelectedPath] = useState<{ paths: MessagePath[]; @@ -528,7 +533,16 @@ export function MessageList({ count: sortedMessages.length, getScrollElement: () => listRef.current, estimateSize: () => ESTIMATED_MESSAGE_HEIGHT, - getItemKey: (index) => sortedMessages[index]?.id ?? index, + // Rows do not start at the scroll container's origin: the container has p-4 + // padding and may render an "older messages" banner above them. Without this + // the virtualizer's offsets are short by that distance, so every + // scrollToIndex with 'start'/'center' lands high by 16-48px — and the error + // moves as the banner appears and disappears during pagination. + scrollMargin, + // String sentinel for the transient window past the end of a shrunken list: + // a bare index would share the keyspace with message ids and poison the + // measurement cache for whichever message happens to have that id. + getItemKey: (index) => sortedMessages[index]?.id ?? `__idx:${index}`, overscan: 8, // A row that measures zero has not really been laid out yet (hidden pane, images // still loading). Keep the estimate instead, or the window balloons to compensate. @@ -551,6 +565,20 @@ export function MessageList({ }); const virtualRows = virtualizer.getVirtualItems(); + // Re-measured whenever something above the rows can change height. + useLayoutEffect(() => { + const spacer = virtualSpacerRef.current; + const list = listRef.current; + if (!spacer || !list) return; + // Relative to the scroll container's *content* origin, so it is independent + // of the current scroll position. offsetTop is not usable here: the two + // elements can resolve to different offsetParents. + const next = Math.round( + spacer.getBoundingClientRect().top - list.getBoundingClientRect().top + list.scrollTop + ); + setScrollMargin((prev) => (prev === next ? prev : next)); + }, [loadingOlder, hasOlderMessages, messages.length]); + const scrollToIndex = useCallback( (index: number, align: 'start' | 'center' | 'end') => { if (index < 0) return; @@ -613,7 +641,15 @@ export function MessageList({ if ((isInitialLoadRef.current || conversationChanged) && messages.length > 0) { // Initial load or conversation switch - pin to the newest message. Requested // rather than performed here; see pendingBottomScrollRef. - requestBottomScroll(); + // + // Unless we are loading *at* a specific message: jump-to-message and + // jump-to-unread clear the list before fetching a window around their + // target, which trips both the initial-load and conversation-changed + // branches. Pinning to the bottom here would then discard the target scroll + // a frame later, stranding the user at the newest message instead. + if (!targetMessageId) { + requestBottomScroll(); + } isInitialLoadRef.current = false; } else if (messagesAdded > 0 && prevMessagesLengthRef.current > 0) { if (scrollStateRef.current.wasNearTop) { @@ -629,7 +665,7 @@ export function MessageList({ } prevMessagesLengthRef.current = messages.length; - }, [messages, sortedMessages.length, scrollToIndex, requestBottomScroll]); + }, [messages, sortedMessages.length, scrollToIndex, requestBottomScroll, targetMessageId]); // Scroll to target message and highlight it useLayoutEffect(() => { @@ -637,8 +673,10 @@ export function MessageList({ const targetIndex = sortedMessages.findIndex((msg) => msg.id === targetMessageId); if (targetIndex === -1) return; - // Prevent the initial-load layout effect from overriding our scroll + // Prevent the initial-load layout effect from overriding our scroll, and drop + // any bottom pin already queued by an earlier pass over the same commit. isInitialLoadRef.current = false; + pendingBottomScrollRef.current = false; scrollToIndex(targetIndex, 'center'); setHighlightedMessageId(targetMessageId); targetScrolledRef.current = true; @@ -1036,6 +1074,7 @@ export function MessageList({ )}
@@ -1146,7 +1185,10 @@ export function MessageList({ data-index={index} ref={virtualizer.measureElement} className="absolute left-0 top-0 flex w-full flex-col pb-0.5" - style={{ transform: `translateY(${virtualRow.start}px)` }} + // start is measured from the scroll container's origin, which + // scrollMargin accounts for; the spacer already sits that far + // down, so subtract it back out when positioning within it. + style={{ transform: `translateY(${virtualRow.start - scrollMargin}px)` }} > {unreadMarkerIndex === index && (onDismissUnreadMarker ? ( diff --git a/frontend/src/hooks/useUnreadCounts.ts b/frontend/src/hooks/useUnreadCounts.ts index fafd655..df1d46d 100644 --- a/frontend/src/hooks/useUnreadCounts.ts +++ b/frontend/src/hooks/useUnreadCounts.ts @@ -164,18 +164,29 @@ export function useUnreadCounts( } }, [activeConversation]); - const incrementUnread = useCallback((stateKey: string, hasMention?: boolean) => { - setUnreadCounts((prev) => ({ - ...prev, - [stateKey]: (prev[stateKey] || 0) + 1, - })); - if (hasMention) { - setMentions((prev) => ({ + const incrementUnread = useCallback( + (stateKey: string, messageId: number, hasMention?: boolean) => { + setUnreadCounts((prev) => ({ ...prev, - [stateKey]: true, + [stateKey]: (prev[stateKey] || 0) + 1, })); - } - }, []); + // Counts move live over the socket, but first_unread_ids only arrives with a + // full /unreads fetch. Without seeding it here, a conversation that goes from + // read to unread while the app is open has a count but no boundary, and the + // divider silently never renders. Only the transition matters: once a + // boundary exists, later messages are not the *first* unread. + setFirstUnreadIds((prev) => + prev[stateKey] != null ? prev : { ...prev, [stateKey]: messageId } + ); + if (hasMention) { + setMentions((prev) => ({ + ...prev, + [stateKey]: true, + })); + } + }, + [] + ); const recordMessageEvent = useCallback( ({ @@ -205,7 +216,7 @@ export function useUnreadCounts( setLastMessageTimes(updated); if (!isActiveConversation && !msg.outgoing && isNewMessage) { - incrementUnread(stateKey, hasMention); + incrementUnread(stateKey, msg.id, hasMention); } }, [incrementUnread] @@ -230,6 +241,14 @@ export function useUnreadCounts( return next; }); + setFirstUnreadIds((prev) => { + if (!(oldStateKey in prev)) return prev; + const next = { ...prev }; + next[newStateKey] = next[newStateKey] ?? next[oldStateKey]; + delete next[oldStateKey]; + return next; + }); + setLastMessageTimes(renameConversationTimeKey(oldStateKey, newStateKey)); }, []); @@ -246,6 +265,12 @@ export function useUnreadCounts( delete next[stateKey]; return next; }); + setFirstUnreadIds((prev) => { + if (!(stateKey in prev)) return prev; + const next = { ...prev }; + delete next[stateKey]; + return next; + }); setUnreadLastReadAts((prev) => { if (!(stateKey in prev)) return prev; const next = { ...prev }; @@ -261,6 +286,7 @@ export function useUnreadCounts( setUnreadCounts({}); setMentions({}); setUnreadLastReadAts({}); + setFirstUnreadIds({}); // Persist to server with single bulk request api.markAllRead().catch((err) => { diff --git a/frontend/src/test/appFavorites.test.tsx b/frontend/src/test/appFavorites.test.tsx index b8746b6..ad7c545 100644 --- a/frontend/src/test/appFavorites.test.tsx +++ b/frontend/src/test/appFavorites.test.tsx @@ -104,6 +104,7 @@ vi.mock('../hooks', async (importOriginal) => { mentions: {}, lastMessageTimes: {}, unreadLastReadAts: {}, + firstUnreadIds: {}, recordMessageEvent: mocks.hookFns.recordMessageEvent, renameConversationState: vi.fn(), markAllRead: mocks.hookFns.markAllRead, diff --git a/frontend/src/test/appSearchJump.test.tsx b/frontend/src/test/appSearchJump.test.tsx index 9345005..259799a 100644 --- a/frontend/src/test/appSearchJump.test.tsx +++ b/frontend/src/test/appSearchJump.test.tsx @@ -53,6 +53,7 @@ vi.mock('../hooks', async (importOriginal) => { mentions: {}, lastMessageTimes: {}, unreadLastReadAts: {}, + firstUnreadIds: {}, recordMessageEvent: vi.fn(), renameConversationState: vi.fn(), markAllRead: vi.fn(), diff --git a/frontend/src/test/appStartupHash.test.tsx b/frontend/src/test/appStartupHash.test.tsx index 8a9a1bd..12d2a3a 100644 --- a/frontend/src/test/appStartupHash.test.tsx +++ b/frontend/src/test/appStartupHash.test.tsx @@ -47,6 +47,7 @@ vi.mock('../hooks', async (importOriginal) => { mentions: {}, lastMessageTimes: {}, unreadLastReadAts: {}, + firstUnreadIds: {}, recordMessageEvent: vi.fn(), renameConversationState: vi.fn(), markAllRead: vi.fn(), diff --git a/frontend/src/test/unreadMarkerResolution.test.ts b/frontend/src/test/unreadMarkerResolution.test.ts new file mode 100644 index 0000000..6baaae7 --- /dev/null +++ b/frontend/src/test/unreadMarkerResolution.test.ts @@ -0,0 +1,56 @@ +/** + * The unread divider is anchored to a message id from the server. These cover the + * one case where that id is deliberately overridden: a channel that has never + * been read, whose true boundary is the start of history and therefore a useless + * jump target. + */ +import { describe, expect, it } from 'vitest'; + +import { resolveUnreadMarkerId } from '../App'; +import type { Message } from '../types'; + +function msg(id: number, receivedAt: number): Message { + return { + id, + type: 'CHAN', + conversation_key: 'CHAN1', + text: `Alice: m${id}`, + sender_timestamp: receivedAt, + received_at: receivedAt, + paths: null, + txt_type: 0, + signature: null, + sender_key: null, + outgoing: false, + acked: 0, + sender_name: 'Alice', + }; +} + +describe('resolveUnreadMarkerId', () => { + const loaded = [msg(50, 1700000050), msg(51, 1700000051), msg(52, 1700000052)]; + + it('uses the server boundary when the channel has been read before', () => { + expect(resolveUnreadMarkerId(9, 1700000000, loaded)).toBe(9); + }); + + it('uses the server boundary when it is inside the loaded window', () => { + expect(resolveUnreadMarkerId(51, null, loaded)).toBe(51); + }); + + it('anchors a never-read channel to the top of the loaded window', () => { + // Boundary 1 is the first message ever sent; jumping there would dump the + // reader at the start of history. Everything loaded is unread, so the top of + // the window is both true and useful. + expect(resolveUnreadMarkerId(1, null, loaded)).toBe(50); + }); + + it('picks the oldest loaded message regardless of array order', () => { + expect(resolveUnreadMarkerId(1, null, [loaded[2], loaded[0], loaded[1]])).toBe(50); + }); + + it('passes through when there is no boundary or no messages', () => { + expect(resolveUnreadMarkerId(null, null, loaded)).toBeNull(); + expect(resolveUnreadMarkerId(7, null, [])).toBe(7); + }); +}); diff --git a/frontend/src/test/useUnreadCounts.test.ts b/frontend/src/test/useUnreadCounts.test.ts index ab9745f..13f2ed4 100644 --- a/frontend/src/test/useUnreadCounts.test.ts +++ b/frontend/src/test/useUnreadCounts.test.ts @@ -479,4 +479,74 @@ describe('useUnreadCounts', () => { expect(result.current.lastMessageTimes[getStateKey('contact', CONTACT_KEY)]).toBe(1700002000); expect(result.current.lastMessageTimes[getStateKey('channel', CHANNEL_KEY)]).toBe(1700002001); }); + + it('seeds the first-unread boundary when a conversation goes unread over the socket', async () => { + // Counts move live over WS but first_unread_ids only arrives with a full + // fetch. Without seeding here, a channel that goes unread while the app is + // open has a count but no boundary, so the divider never renders. + const mocks = await getMockedApi(); + mocks.getUnreads.mockResolvedValue({ + counts: {}, + mentions: {}, + last_message_times: {}, + first_unread_ids: {}, + last_read_ats: {}, + }); + + const { result } = renderWith({ channels: [makeChannel(CHANNEL_KEY, 'Test')] }); + await act(async () => { + await vi.waitFor(() => expect(mocks.getUnreads).toHaveBeenCalled()); + }); + + const key = getStateKey('channel', CHANNEL_KEY); + act(() => { + result.current.recordMessageEvent({ + msg: makeMessage({ id: 4711, type: 'CHAN', conversation_key: CHANNEL_KEY }), + activeConversation: false, + isNewMessage: true, + }); + }); + expect(result.current.firstUnreadIds[key]).toBe(4711); + + // A later message must not move the boundary — it is not the *first* unread. + act(() => { + result.current.recordMessageEvent({ + msg: makeMessage({ id: 4712, type: 'CHAN', conversation_key: CHANNEL_KEY }), + activeConversation: false, + isNewMessage: true, + }); + }); + expect(result.current.firstUnreadIds[key]).toBe(4711); + expect(result.current.unreadCounts[key]).toBe(2); + }); + + it('drops first-unread boundaries on mark-all-read', async () => { + const mocks = await getMockedApi(); + mocks.getUnreads.mockResolvedValue({ + counts: {}, + mentions: {}, + last_message_times: {}, + first_unread_ids: {}, + last_read_ats: {}, + }); + + const { result } = renderWith({ channels: [makeChannel(CHANNEL_KEY, 'Test')] }); + await act(async () => { + await vi.waitFor(() => expect(mocks.getUnreads).toHaveBeenCalled()); + }); + + act(() => { + result.current.recordMessageEvent({ + msg: makeMessage({ id: 99, type: 'CHAN', conversation_key: CHANNEL_KEY }), + activeConversation: false, + isNewMessage: true, + }); + }); + expect(result.current.firstUnreadIds[getStateKey('channel', CHANNEL_KEY)]).toBe(99); + + await act(async () => { + await result.current.markAllRead(); + }); + expect(result.current.firstUnreadIds).toEqual({}); + }); }); diff --git a/tests/test_api.py b/tests/test_api.py index 6090163..c1cc6b2 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -1022,6 +1022,54 @@ class TestReadStateEndpoints: ) assert result["first_unread_ids"][f"contact-{contact_key}"] == first_dm.id + @pytest.mark.asyncio + async def test_first_unread_id_breaks_same_second_ties_by_id(self, test_db): + """Sender timestamps are whole seconds, so the oldest unread second is + routinely shared. The boundary must be the first of those messages, not + an arbitrary one.""" + chan_key = "BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB2" + await ChannelRepository.upsert(key=chan_key, name="Busy") + await ChannelRepository.update_last_read_at(chan_key, 1000) + + # Three unread messages all landing in the same second. + ids = [] + for text in ("Bob: first", "Bob: second", "Bob: third"): + ids.append( + await MessageRepository.create( + msg_type="CHAN", + text=text, + received_at=1001, + conversation_key=chan_key, + sender_timestamp=1001, + ) + ) + + result = await MessageRepository.get_unread_counts(None) + + assert result["counts"][f"channel-{chan_key}"] == 3 + assert result["first_unread_ids"][f"channel-{chan_key}"] == min(ids) + + @pytest.mark.asyncio + async def test_first_unread_id_ignores_muted_and_outgoing(self, test_db): + """Muted channels are excluded from unread counts, so they must not + report a boundary either.""" + chan_key = "CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC3" + await ChannelRepository.upsert(key=chan_key, name="Muted") + await ChannelRepository.update_last_read_at(chan_key, 1000) + await ChannelRepository.set_muted(chan_key, True) + await MessageRepository.create( + msg_type="CHAN", + text="Bob: unread but muted", + received_at=1001, + conversation_key=chan_key, + sender_timestamp=1001, + ) + + result = await MessageRepository.get_unread_counts(None) + + assert f"channel-{chan_key}" not in result["counts"] + assert result["first_unread_ids"].get(f"channel-{chan_key}") is None + @pytest.mark.asyncio async def test_get_unreads_no_name_skips_mentions(self, test_db): """Unreads without a radio name returns counts but no mention flags."""