Skip to content

feat: lazy loading for Genie conversations#163

Open
calvarjorge wants to merge 15 commits intomainfrom
jorge.calvar/genie_sequential_loading
Open

feat: lazy loading for Genie conversations#163
calvarjorge wants to merge 15 commits intomainfrom
jorge.calvar/genie_sequential_loading

Conversation

@calvarjorge
Copy link
Contributor

@calvarjorge calvarjorge commented Mar 5, 2026

Summary

  • When loading a Genie conversation, only the most recent page of messages is fetched initially (configurable via initialPageSize, default 20)
  • Older messages are loaded on demand when the user scrolls to the top
  • Adds a history_info SSE event that communicates pagination state (nextPageToken) to the frontend
  • Scroll position is preserved when older messages are prepended

Load only the most recent page of messages when opening a conversation,
and fetch older messages on demand when the user scrolls to the top.

- Add `history_info` SSE event with pagination token
- Add REST endpoint GET /:alias/conversations/:conversationId/messages
- Frontend auto-triggers loading via IntersectionObserver with scroll
  position preservation
- Consolidate to single `listConversationMessages` method in connector

Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com>
Load only the most recent page of messages when opening a conversation,
and fetch older messages on demand when the user scrolls to the top.

- Add `history_info` SSE event with pagination token to existing
  getConversation endpoint (no new endpoints)
- Frontend auto-triggers loading via IntersectionObserver with scroll
  position preservation using the same SSE endpoint with pageToken
- Consolidate to single `listConversationMessages` method in connector

Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com>
- Extract shared fetchConversationPage for loadHistory and loadOlderMessages
- Rename processEvent to processStreamEvent (only used for sendMessage)
- Pass stable React setters directly instead of recreating paginationCallbacks
- Add abort signal to loadOlderMessages to prevent leaks on unmount

Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com>
Reference the constant directly instead of hardcoding the value,
preventing test failures when the default is changed.

Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com>
Extract useScrollManagement and useLoadOlderOnScroll into custom hooks,
inline StreamingIndicator, leaving the component as clean JSX.

Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com>
- useLoadOlderOnScroll owns its sentinel ref internally
- Simplify message rendering with filter+map

Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com>
- Merge ref-sync effects into one
- Extract fetchPage helper shared by loadHistory and loadOlderMessages
- Simplify query_result handler with .map() instead of imperative loop

Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com>
Comment on lines +426 to +427
hasOlderMessages,
loadOlderMessages,
Copy link
Member

Choose a reason for hiding this comment

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

maybe we could rename hasOlderMessages to hasPreviousPage and loadOlderMessages to fetchPreviousPage? To use a naming known from e.g. tanstack query: https://tanstack.com/query/latest/docs/framework/react/reference/useInfiniteQuery?from=reactQueryV3

What. do you think?

BTW maybe isFetchingPreviousPage would be also helpful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I made the renames and added isFetchingPreviousPage

}

const includeQueryResults = req.query.includeQueryResults !== "false";
const pageToken = req.query.pageToken as string | undefined;
Copy link
Member

@pkosiec pkosiec Mar 6, 2026

Choose a reason for hiding this comment

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

Express query params can be [key: string]: undefined | string | ParsedQs | (string | ParsedQs)[];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a check to ensure they're of type string

convId: string,
options?: { pageToken?: string; errorMessage?: string },
) => {
abortControllerRef.current?.abort();
Copy link
Member

Choose a reason for hiding this comment

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

pagination should have separate abortControllerRef - if a user scrolls to the top during an active stream (status "streaming"), loadOlderMessages -> fetchPage will abort the in-progress streaming connection

Copy link
Contributor Author

Choose a reason for hiding this comment

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

separated into 2 abort controllers so that the scroll won't stop sending a message

});
});

describe("getConversation with pageToken", () => {
Copy link
Member

Choose a reason for hiding this comment

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

I think we're missing a test case for pagination request failure - can you add it pls?

- Rename hasOlderMessages/loadOlderMessages to hasPreviousPage/
  fetchPreviousPage following TanStack Query conventions, add
  isFetchingPreviousPage
- Use separate AbortController for pagination to avoid aborting
  in-progress streams
- Fix Express query param type safety (typeof check vs as cast)
- Add test for paginated request failure

Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com>
@calvarjorge calvarjorge requested a review from pkosiec March 9, 2026 08:05
@pkosiec
Copy link
Member

pkosiec commented Mar 9, 2026

@calvarjorge ran /ce:review (https://github.com/EveryInc/compound-engineering-plugin) and got those results

  • confirmed with a validation run on Cursor: could you please check it? Especially the P1 and P2 parts. Thanks!
 ---
  🔴 P1 — Critical (Blocks Merge)

  1. useScrollManagement yanks user to bottom on status change
  - File: packages/appkit-ui/src/react/genie/genie-chat-message-list.tsx
  - Agents: Frontend Races (Critical), Performance (3.2)
  - The useLayoutEffect depends on [messages.length, status]. When status changes to "loading-older", the effect fires before any messages are prepended, detects no prepend (first message
   ID unchanged), and scrolls to bottom — yanking the user away from where they deliberately scrolled.
  - Fix: Remove status from the dependency array for scroll-to-bottom logic, or split into two effects: one for append (scroll to bottom) and one for prepend (preserve position). Only
  scroll to bottom when messages are appended or on initial load.
  - Repro: Load a conversation with many messages. Scroll up. Watch scroll position reset when "loading older" spinner appears.

  2. Race between fetchPreviousPage and concurrent sendMessage
  - File: packages/appkit-ui/src/react/genie/use-genie-chat.ts
  - Agents: Frontend Races (High), TypeScript (Critical)
  - If user sends a message while older messages are loading: sendMessage sets status to "streaming", then the pagination promise resolves and stomps status back to "idle", hiding the
  streaming indicator. Old messages also get prepended during streaming, causing visual chaos.
  - Fix: Either (a) abort paginationAbortRef at the start of sendMessage, or (b) guard fetchPreviousPage's .then() to only apply results when status is still "loading-older":
  setStatus((current) => current === "loading-older" ? "idle" : current);

  3. IntersectionObserver rapid-fire pagination loop
  - File: packages/appkit-ui/src/react/genie/genie-chat-message-list.tsx
  - Agents: Frontend Races (Medium), Performance (3.3)
  - After a page loads and status returns to "idle", shouldObserve becomes true again, recreating the IntersectionObserver. But the sentinel is still visible at the top (user hasn't
  scrolled down), so the observer immediately fires again, loading the next page, and the next, defeating lazy loading entirely.
  - Fix: Add a debounce flag that suppresses the first intersection event after a page load, OR ensure useScrollManagement's scroll adjustment pushes the sentinel below the viewport fold
  after prepend. If issue #1 is fixed (no scroll-to-bottom on status change), the scroll preservation should naturally push the sentinel off-screen.
  - Repro: Load a conversation with many pages. Scroll to top once. Count network requests — if multiple fire without further scrolling, the loop is happening.

  ---
  🟡 P2 — Important (Should Fix)

  4. connectSSE retries can cause duplicate messages during pagination
  - File: packages/appkit-ui/src/react/genie/use-genie-chat.ts (fetchConversationPage)
  - Agent: Frontend Races (High)
  - connectSSE has built-in retry logic. The items array in fetchConversationPage accumulates across retries in the same closure. If attempt 1 partially succeeds then fails, attempt 2
  succeeds fully — items contains duplicates.
  - Fix: Either disable retries for pagination (maxRetries: 0), or clear the items array at the start of each onMessage sequence. For user-initiated "load more", showing a retryable error
   is better UX than silent retries.

  5. Error status not set for server-sent error events during pagination
  - Files: use-genie-chat.ts (fetchConversationPage + fetchPreviousPage)
  - Agents: TypeScript (Critical #1), Architecture (4c)
  - The onError callback in fetchConversationPage calls setError() but does NOT call setStatus("error"). Only onConnectionError sets error status. When a server-sent { type: "error" }
  event arrives during pagination, error message is set but status transitions to "idle" when the stream completes — the error flashes then disappears.
  - Fix: Have the onError callback also set status, or check error state in .then() before transitioning to "idle".

  6. Unsafe MutableRefObject cast in fetchPage
  - File: use-genie-chat.ts, lines 327-329
  - Agents: TypeScript (#3), Pattern Recognition (#1), Simplicity (#4)
  - fetchPage accepts React.RefObject<AbortController | null> then casts to MutableRefObject to write .current. The parameter type should be React.MutableRefObject<AbortController | null>
   directly, or use { current: AbortController | null }.

  7. query_result handler regression: O(n) full-array scan + copy on every event
  - File: use-genie-chat.ts (processStreamEvent)
  - Agents: Performance (2.1), Frontend Races (#6)
  - The refactored query_result handler uses .map() over the entire messages array for every event, creating a new array each time. The old code used reverse scan with break. With K query
   results and N messages, this is O(N*K) with K full array allocations, each triggering React reconciliation.
  - Fix: Restore the reverse-scan-with-break pattern, or build an attachmentId → messageIndex lookup.

  8. Scroll height reference stale with async content
  - File: genie-chat-message-list.tsx (useScrollManagement)
  - Agent: Frontend Races (#5)
  - prevScrollHeightRef is captured at the end of the layout effect. If images or async content inside messages load between renders, scrollHeight shifts, making the next prepend's delta
  calculation wrong.
  - Fix: Consider capturing scroll height before the prepend (in fetchPreviousPage before setMessages), or use a ResizeObserver on the viewport to keep the reference fresh.

  ---
  🔵 P3 — Nice-to-Have (Enhancements)

  9. Remove unused fields from history_info event
  - Files: shared/src/genie.ts, connectors/genie/client.ts
  - Agents: Simplicity (#1), Architecture (#4e)
  - conversationId, spaceId, and loadedCount are emitted but never consumed by the frontend. Slim history_info to { type: "history_info"; nextPageToken: string | null }.

  10. Remove unused isFetchingPreviousPage from return type
  - Files: types.ts, use-genie-chat.ts
  - Agent: Simplicity (#2)
  - Exposed in UseGenieChatReturn but never consumed. It's trivially derived from status === "loading-older". YAGNI.

  11. filter().map() creates intermediate array on every render
  - File: genie-chat-message-list.tsx
  - Agent: Performance (3.1)
  - The previous inline-null-return pattern avoided this allocation. Either revert to .map() with null return, or wrap in useMemo.

  12. Scroll state leaks across conversation switches
  - File: genie-chat-message-list.tsx (useScrollManagement)
  - Agent: Performance (3.2)
  - prevFirstMessageIdRef and prevScrollHeightRef persist across conversation changes. On reset() + loadHistory() returning the same number of messages, the hook may incorrectly detect a
  "prepend".
  - Fix: Pass conversationId as a key prop on GenieChatMessageList or reset refs when conversation changes.

  13. loadedCount JSDoc says "initial load" but applies to all pages
  - File: shared/src/genie.ts
  - Agent: TypeScript (#7)
  - Comment says "Total messages returned in this initial load" but it's emitted on every page fetch. Rename to pageMessageCount or update the comment.

  14. Programmatic plugin API lacks pagination
  - Files: genie.ts (plugin exports), client.ts
  - Agent: Agent-Native (#1, #2)
  - GeniePlugin.getConversation() fetches all pages. No way for server-side agents to paginate. Consider exposing a listMessages(alias, convId, { pageToken?, pageSize? }) export.

  15. No frontend tests for scroll management hooks
  - File: genie-chat-message-list.tsx
  - Agent: TypeScript (#5)
  - useScrollManagement and useLoadOlderOnScroll have non-trivial DOM interaction logic with no test coverage.

  16. Silent SSE parse error swallowing
  - File: use-genie-chat.ts
  - Agents: TypeScript (#6), Frontend Races (#7)
  - catch { // Malformed SSE data } — if history_info event is malformed, pagination silently stops working. Add console.warn in development.

  17. options parameter name shadows outer scope in fetchPage
  - File: use-genie-chat.ts
  - Agent: TypeScript (#8)
  - Inner options parameter in fetchPage shadows the outer options: UseGenieChatOptions. Consider renaming to pageOptions.

Consistent with getConversation handler — avoids unsafe as string cast
for Express query params.

Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com>
- useScrollManagement no longer fires on status-only changes, preventing
  scroll-to-bottom yank when "loading-older" spinner appears
- sendMessage aborts in-flight pagination to prevent status stomping
- fetchPreviousPage guards setStatus to not overwrite concurrent streams
- IntersectionObserver skips initial callback after re-subscription to
  prevent rapid-fire pagination loop

Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com>
- Server-sent error events during pagination now set error status
  (hadError flag prevents status flashing to idle)
- Replace unsafe MutableRefObject cast with plain object type in fetchPage
- Restore reverse-scan-with-break for query_result in processStreamEvent
- Add ResizeObserver to keep scroll height ref fresh for async content

Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com>
- Set error status directly in onError callback instead of threading
  hadError flag through return value
- Remove freeze/unfreeze scroll momentum plumbing (not effective enough)
- Clean up unused code

Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com>
- loadHistory aborts in-flight pagination to prevent stale messages
  from old conversation being prepended
- Replace initialFire boolean with requestAnimationFrame arming so
  short conversations where sentinel is genuinely visible still
  trigger loading automatically

Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com>
Add MIN_PREVIOUS_PAGE_LOAD_MS to ensure scroll inertia fully settles
before fetchPreviousPage prepends older messages to the chat.

Signed-off-by: Jorge Calvar <jorge.calvar@databricks.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