Skip to content

fix(web): paginate conversation list + preserve displayOrder (LUM-1618, LUM-1619)#31472

Merged
ashleeradka merged 1 commit into
mainfrom
claude/lum-1618-1619-conversations-fixes
May 21, 2026
Merged

fix(web): paginate conversation list + preserve displayOrder (LUM-1618, LUM-1619)#31472
ashleeradka merged 1 commit into
mainfrom
claude/lum-1618-1619-conversations-fixes

Conversation

@ashleeradka
Copy link
Copy Markdown
Contributor

Summary

Two related fixes in apps/web/src/domains/chat/api/conversations.ts and the downstream grouping. Both were silent data-loss / UX bugs flagged by Codex during the migration.

LUM-1618 — paginate the conversation list

fetchConversationList sent a single GET with no limit / offset and ignored the daemon's hasMore signal. The daemon defaults to 50 items per page, so users with more than 50 foreground (or background) conversations silently lost older rows from the sidebar.

Loop over pages with limit=50, accumulating until hasMore is false. A 200-page safety cap (10,000 conversations of one type) and an empty-page guard prevent a malformed hasMore: true response from looping forever.

LUM-1619 — preserve displayOrder for pinned / custom-group buckets

parseConversation dropped the daemon-supplied displayOrder field, and listConversations's global lastMessageAt sort then overrode whatever order the user had drag-set. After a reload, pinned conversations appeared newest-first instead of in the user's custom order.

  • Capture displayOrder on Conversation.
  • In groupConversations, sort the pinned bucket and each custom-group bucket by displayOrder ascending. Rows missing displayOrder fall back to lastMessageAt newest-first; ties also break on lastMessageAt. The global listConversations sort by recency is preserved so non-bucketed consumers (logs tab, archive page) keep their current behavior.

Closes LUM-1618, LUM-1619.

Test plan

  • bun run test:ci — 100 / 100 pass (added new tests for pagination loop / empty-page guard / first-page-exit, displayOrder parsing, and pinned + custom-group displayOrder sort)
  • bun run lint — clean (1 preexisting unrelated warning)
  • bunx tsc --noEmit — clean

Generated by Claude Code

…8, LUM-1619)

LUM-1618 — fetchConversationList sent a single GET with no limit/offset
and ignored the daemon's hasMore signal. Users with more than 50
foreground (or background) conversations silently lost older rows from
the sidebar. Loop over pages with limit=50, accumulating until
hasMore is false. A 200-page safety cap and an empty-page guard
prevent a malformed hasMore from looping forever.

LUM-1619 — parseConversation dropped the daemon-supplied displayOrder,
and the global lastMessageAt sort then overrode whatever order the
user had drag-set. Capture displayOrder in the Conversation interface
and sort pinned + custom-group buckets by it in groupConversations
(ascending, with lastMessageAt as tiebreaker; rows without
displayOrder fall back to newest-first).
@linear
Copy link
Copy Markdown

linear Bot commented May 21, 2026

LUM-1618

LUM-1619

@ashleeradka ashleeradka marked this pull request as ready for review May 21, 2026 14:05
Copy link
Copy Markdown
Contributor

@vex-assistant-bot vex-assistant-bot Bot left a comment

Choose a reason for hiding this comment

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

APPROVE

Value: Two silent data-loss bugs fixed — users with more than 50 conversations no longer get a silently truncated sidebar, and users who drag-reordered their pinned / custom-group conversations no longer see that order scrambled on every reload.

What this does: Adds a paginated fetch loop to fetchConversationList (accumulates pages until hasMore is false), captures displayOrder in parseConversation, and adds a compareByDisplayOrder comparator to sort pinned / custom-group buckets by the server-provided drag order while falling back to lastMessageAt for everything else.


Code analysis

Pagination loop (fetchConversationList)

  • ApiError thrown on non-OK responses — correct, matches every other function in the file ✅
  • Safety cap at 200 pages (10,000 conversations per type) — bounded and documented ✅
  • Empty-page guard (if (pageItems.length === 0) break) prevents a malformed hasMore: true from spinning forever ✅
  • hasMore defaulted to false when the field is absent or non-boolean ✅
  • sessions fallback preserved for backward-compat with older daemon versions ✅

listConversations orchestration

  • Promise.allSettled to fetch foreground + background in parallel; background failures degrade gracefully with a Sentry warning and don't break the sidebar ✅
  • Dedup by conversationKey (foreground wins on collision) ✅
  • Global lastMessageAt sort preserved so non-bucketed consumers (logs tab, archive page) are unaffected ✅

parseConversationdisplayOrder field

  • Guards typeof === "number" && Number.isFinite(...) — correctly rejects NaN, Infinity, and string "0"
  • JSDoc on Conversation.displayOrder is precise and explains which buckets consume it ✅

compareByDisplayOrder comparator

  • Ascending by displayOrder, ties break on lastMessageAt newest-first ✅
  • Rows with displayOrder win over rows without ✅
  • Returns a stable sort for equal timestamps ✅

groupConversations

  • Uses slice() before sort on every bucket — never mutates the caller's array ✅
  • scheduled and background buckets intentionally left at lastMessageAt order (not drag-reorderable) ✅
  • Custom-group sort applied per-bucket in a loop — O(k × n log n) which is fine for realistic sidebar sizes ✅

Minor observation (non-blocking)

compareByDisplayOrder: ordered rows win over unordered rows

The comparator puts any row with displayOrder before any row without it. The PR description says "freshly-pinned conversations land near the top until the server assigns them an order" — but the current behavior is the opposite: a freshly-pinned conversation with no displayOrder yet sinks below all drag-reordered ones. This is a transient state (the server should assign displayOrder quickly), but the claim in the description doesn't match the code. If the intended behavior is "new pins appear at the top," the comparator needs to be inverted for the mixed case:

// Current: ordered first, unordered last
if (aOrder != null) return -1;

// Alternative: unordered first (newest pin floats up)
if (aOrder == null) return -1;

Worth confirming intent before the next drag-reorder UX pass.


Test coverage

Pagination: three targeted tests — multi-page accumulation (80 items across 2 pages), early exit on hasMore: false, and the empty-page infinite-loop guard. Mock restores in afterEach via client.get = originalGet — correct mock seam (not globalThis.fetch). displayOrder parsing coverage looks solid for the parseConversation layer; I'd add a test in group-conversations.test.ts specifically for the mixed ordered/unordered sort case once the intent above is confirmed.


Vellum Constitution — Trust-seeking: silent truncation at 50 conversations and invisible reorder resets are exactly the kind of invisible failure that erodes trust in an app you use every day.

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 3 additional findings.

Open in Devin Review

@ashleeradka ashleeradka merged commit df1b316 into main May 21, 2026
7 checks passed
@ashleeradka ashleeradka deleted the claude/lum-1618-1619-conversations-fixes branch May 21, 2026 14:09
siddseethepalli added a commit that referenced this pull request May 24, 2026
…runcate (#31924)

ConversationRestorer single-shot fetched 50 rows from
?conversationType=background and never paginated, so users with many
scheduled conversations saw a small recent-activity slice (e.g. 14 of
1856 in one report). This is the macOS analogue of LUM-1618 which was
fixed for web in #31472.

Add fetchAllConversationPages helper that loops until hasMore is
false (with a 50-page safety cap), used for both the foreground and
background fetches in fetchConversationList. Page size is 200 to match
ConversationListStore.loadAllRemainingConversations and reduce cold-
launch round-trips on macOS, which talks to the daemon over loopback.

Co-authored-by: Vellum Assistant <assistant@vellum.ai>
dvargasfuertes pushed a commit that referenced this pull request May 25, 2026
…l sync fan-out (#32002)

* fix(daemon+web): per-conversation seen-state event replaces list-level sync fan-out

Conversation switches that landed on an unseen conversation were
triggering a full sidebar drain — ~14 paginated /conversations
requests (limit=50, offsets 0..350 × foreground + background) on
every switch, even when the conversation list itself didn't change
shape.

Root cause: `handleRecordSeen` and `handleMarkUnread` published
`conversation_list_invalidated` + a `sync_changed` carrying the
`conversationsList` tag. Web consumed the tag via
`useAssistantSyncStream` and `webSyncRouter` and invoked the
debounced full-list refetch (`fetchConversationList` drains
`hasMore=false`, twice per fetch). Seen state is per-conversation
attention metadata, not list-shaped, so the list-level fan-out was
the wrong primitive.

This swaps both routes onto a new `publishConversationSeenChanged`
that emits a single typed `conversation_seen_changed` event
carrying the canonical post-mutation state (hasUnseen,
latestAssistantMessageAt, lastSeenAssistantMessageAt as epoch ms).
The web's `useAssistantSyncStream` (chat-layout scope, always
mounted) patches the cached conversation row in-place via the new
`applyConversationSeenStateLocal` helper. No list invalidation, no
refetch, no drain.

Originating client's echo is a harmless idempotent re-patch — its
optimistic update (`markConversationSeenLocal`) already wrote the
same state. Sibling tabs/devices receive the typed event and patch.
macOS client doesn't yet handle this event; its sidebar will catch
up on the next unrelated list refresh (acceptable for seen state,
which is purely visual).

Tests:
- New daemon test `conversation-seen-changed-publish.test.ts` covers
  both routes' publish behavior, no-state-change skip, and asserts
  `publishConversationListAndMetadataChanged` is NOT called.
- New web tests for the parser (5 cases incl. timestamp narrowing),
  the cache patcher (4 cases incl. defensive null preservation), and
  the sync-stream handler (2 cases asserting no list invalidation).

Refs: LUM-1618 (PR #31472 introduced the drain pagination loop on web),
PR #31924 (macOS analog using fetchAllConversationPages).

* fix: address CI failures from PR #32002

Three CI failures all caused by the seen-state PR; one design
issue surfaced by the cross-domain import rule, two test-shape
issues from the behavior change.

1) **Lint (web) — cross-domain import**: the new import of
   `applyConversationSeenStateLocal` (in `domains/conversations/`)
   from `domains/chat/hooks/use-assistant-sync-stream.ts` tripped
   `local/no-cross-domain-imports`. The hook was already a
   cross-cutting concern — it routes assistant-global SSE events
   into caches across avatar, identity, conversations, sounds,
   schedules, feature flags, and home-feed — mounted at
   RootLayout and described as 'always mounted on every
   authenticated route' in its own header. Per the conventions
   doc, foundational/cross-cutting concerns 'always top-level,
   even if currently consumed by one domain'. Moved the hook
   (and its test) to `src/hooks/`. Updated the two consumers
   (`root-layout.tsx` import, dynamic import in the test).

2) **Type Check (assistant)**: `mockGetAttentionState` in
   `conversation-seen-changed-publish.test.ts` inferred every
   nullable field of its return shape as the literal type of its
   default implementation (e.g. `lastSeenAssistantMessageId:
   null` → typed as `null`). A `.mockImplementationOnce` that
   returns the same row with non-null fields (the
   already-seen-conversation path) is then incompatible. Added
   an explicit `MockAttentionRow` type mirroring the store's
   `AttentionState` shape and applied it as the mock's return
   annotation so overrides accept the full `string | null`
   union.

3) **Test (assistant) — `conversation-sync-tags.test.ts`**: two
   cases (`record seen`, `mark unread`) still asserted the old
   list-level fan-out — exactly the behavior this PR removed.
   Rewrote both to assert the new contract: a single
   `conversation_seen_changed` typed event carrying the
   canonical post-mutation state, with explicit negative
   assertions that `sync_changed` and
   `conversation_list_invalidated` are NOT emitted. Mirrors the
   shape used by the new `conversation-seen-changed-publish`
   regression suite, but exercised against the real
   `assistantEventHub` rather than mocks.

* refactor(seen-signal): drop list umbrella for content-only reasons; GET-and-patch on web

Replaces the typed `conversation_seen_changed` event (PR #32002 Phase 1-2)
with a per-conversation sync-tag + GET-and-patch loop on web. macOS keeps
the legacy `conversation_list_invalidated` broadcast (now scoped to
`targetInterfaceId: "macos"`) until the Electron cutover.

## Hub (`assistant-event-hub.ts`)

- New `targetInterfaceId?: InterfaceId` option on `publish()` and
  `broadcastMessage()`, composing with `targetClientId`/`targetCapability`/
  `excludeClientId`.
- Inline title-update `conversation_list_invalidated` emission is now
  macOS-scoped with an electron-cutover TODO.

## Sync helpers (`resource-sync-events.ts`)

- New `SHAPE_CHANGING_REASONS = {"created", "deleted", "reordered"}`.
- `publishConversationListAndMetadataChanged` only prepends the
  `conversationsList` umbrella tag for shape-changing reasons.
  Content-only reasons (`seen_changed`, `renamed`) emit per-conversation
  metadata tags only.
- New `broadcastConversationListInvalidatedToMacos()` helper centralizes
  the macOS-only legacy broadcast. Same electron-cutover TODO.
- Removed the typed `publishConversationSeenChanged` publisher and the
  `ConversationSeenChanged` message type.

## Web

- New `fetchConversationDetail(assistantId, conversationId)` +
  `CONVERSATION_NOT_FOUND` sentinel in `chat/api/conversations.ts`.
- New `refreshConversationRow(queryClient, assistantId, conversationId)`
  in `conversation-queries.ts` — replaces / appends / removes the row
  in the cached chat context. One GET per row instead of the legacy
  ~14-request paginated drain (`limit=50&offset=0..N` × foreground +
  background) at a few hundred conversations.
- `useAssistantSyncStream` metadata-tag branch now calls
  `refreshConversationRow` (wrapped in `Sentry.captureException`) instead
  of debounced `invalidateQueries`.
- Dropped the FE `handleConversationListInvalidated` handler; the
  dispatcher case is a documented no-op until the electron cutover.
- Removed `applyConversationSeenStateLocal` + the typed-event parser /
  GLOBAL_STREAM_EVENT_TYPES entries.

## Tests

- Rewrote `conversation-sync-tags.test.ts`: shape-changing reasons emit
  `sync_changed` with the umbrella tag; content-only reasons emit just
  the per-conversation metadata tag. Legacy broadcast asserted
  invisible to process subscribers.
- Rewrote `useAssistantSyncStream` metadata-tag tests via `mock.module`
  on `fetchConversationDetail`: GET fires for the right id, cache is
  patched, untouched rows preserved, 404 removes the row, no list-level
  invalidation fires.
- Removed orphaned tests for the torn-out typed event.

Resolves Vargas's PR #32002 architectural review: keep
`publishConversationListAndMetadataChanged`, carry the `conversationId`
through, single emit path per signal.

---------

Co-authored-by: vellum-apollo-bot[bot] <242025090+vellum-apollo-bot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants