Repository navigation
feat: improve agent session list pagination - #2126
Conversation
📝 WalkthroughWalkthroughThis PR implements cursor-based pagination for agent session listing across backend and frontend, replacing page-based polling. Sessions are now ordered by last-updated descending, session titles auto-derive from the first user message, and the frontend session list updates in real-time from stream responses. ChangesCursor-based pagination and session list improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/src/features/agent/components/SessionSidebar.tsx (2)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing GPL v3 license header.
The file has no license header. Per coding guidelines, all
*.tsxsource files must carry a GPL v3 license header managed viamake addlicense.+// Copyright (C) 2024 The Dagu Authors +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see <https://www.gnu.org/licenses/>. + import { useEffect, useMemo, useRef } from 'react';As per coding guidelines: "Apply GPL v3 license headers on source files, managed via
make addlicense".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/features/agent/components/SessionSidebar.tsx` at line 1, Add the missing GPL v3 license header to the SessionSidebar.tsx source file by running the repository tooling: execute make addlicense (or apply the standard GPL v3 header the tool uses) so the file gains the required license comment at the top; ensure the header format matches other .tsx files in the repo and commit the updated file.
45-56:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
onLoadMorecan fire multiple times while a fetch is in-flight.The IntersectionObserver in this component triggers
onLoadMorerepeatedly if the sentinel stays visible (e.g., when threshold: 0.1 detects visibility). TheloadMoreSessionsfunction inuseAgentChatonly checkshasMoreSessionsandsessionCursorbefore callingfetchSessionsPage—there is no guard against concurrent invocations. IffetchSessionsPageis still executing when the observer fires again, duplicate requests will be sent to the backend.To fix this, accept an
isLoadingMoreprop and skip observing while a load is pending:Guard using an `isLoadingMore` prop
type Props = { ... hasMore: boolean; + isLoadingMore?: boolean; }; export function SessionSidebar({ ... hasMore, + isLoadingMore, }: Props): ReactElement | null { ... useEffect(() => { const el = sentinelRef.current; - if (!el || !hasMore) return; + if (!el || !hasMore || isLoadingMore) return; const observer = new IntersectionObserver(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/features/agent/components/SessionSidebar.tsx` around lines 45 - 56, Add a guard to avoid concurrent loads: accept an isLoadingMore boolean prop into the component and in the useEffect that creates the IntersectionObserver (which uses sentinelRef, hasMore, onLoadMore) return early when isLoadingMore is true so the observer is not created/triggered while a fetch is in-flight; additionally ensure the loadMoreSessions implementation in useAgentChat (which calls fetchSessionsPage and checks hasMoreSessions and sessionCursor) exposes and sets this loading state (isLoadingMore) around the async fetch to prevent concurrent invocations.
🧹 Nitpick comments (3)
internal/service/frontend/api/v1/agent_sessions_test.go (1)
264-269: ⚡ Quick winMake the follow-up cursor request explicitly use cursor mode.
The second request reuses
Cursorbut omitsPaginationMode. SettingPaginationModeCursorthere too will make the test unambiguous and resilient to future default-mode changes.Suggested test tweak
resp, err = setup.api.ListAgentSessions(sessionAdminCtx(), apigen.ListAgentSessionsRequestObject{ Params: apigen.ListAgentSessionsParams{ + PaginationMode: &mode, Cursor: listResp.NextCursor, PerPage: new(2), }, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/frontend/api/v1/agent_sessions_test.go` around lines 264 - 269, The follow-up ListAgentSessions call reuses the Cursor but doesn't set PaginationMode, which can make the test brittle; update the second call to ListAgentSessions (using ListAgentSessionsRequestObject and ListAgentSessionsParams) to explicitly set PaginationMode to PaginationModeCursor along with Cursor and PerPage so the request unambiguously uses cursor mode.internal/agent/api.go (1)
1672-1681: 💤 Low valueConsider
slices.SortStableFuncoversort.SliceStable
sort.SliceStablewith an index-based closure is the pre-1.21 idiom.slices.SortStableFunc(available since Go 1.21) is more idiomatic and avoids capturing the slice by index:♻️ Proposed refactor
+import "slices" +import "cmp" // already imported + func sortSessionsNewestFirst(sessions []SessionWithState) { - sort.SliceStable(sessions, func(i, j int) bool { - left := sessions[i].Session - right := sessions[j].Session - if !left.UpdatedAt.Equal(right.UpdatedAt) { - return left.UpdatedAt.After(right.UpdatedAt) - } - return left.ID > right.ID - }) + slices.SortStableFunc(sessions, func(a, b SessionWithState) int { + if c := b.Session.UpdatedAt.Compare(a.Session.UpdatedAt); c != 0 { + return c + } + return cmp.Compare(b.Session.ID, a.Session.ID) + }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agent/api.go` around lines 1672 - 1681, Replace the index-based sort in sortSessionsNewestFirst with the Go 1.21 idiomatic slices.SortStableFunc: use slices.SortStableFunc over the sessions slice and provide a comparator that directly compares two SessionWithState values (accessing their Session.UpdatedAt and Session.ID) to order by UpdatedAt descending then by ID, avoiding capturing by index; update imports to include "slices" and remove reliance on sort.SliceStable.ui/src/features/agent/hooks/useAgentChat.ts (1)
609-637: ⚡ Quick win
sessionPageinfetchSessionsPagedeps causes stale-closure reads and unnecessary callback re-creations
setSessionPage(sessionPage + 1)at line 614 capturessessionPagefrom the closure. BecausesessionPageis in the dependency array (line 631),fetchSessionsPage(and thusloadMoreSessions) is recreated after every page load. Using the functional-update form avoids the stale read and allowssessionPageto be removed from the deps, eliminating the chained re-creations:♻️ Proposed fix
- if (!cursor) { + if (!cursor) { setSessions(converted); - setSessionPage(1); + setSessionPage(1); // reset } else { appendSessions(converted); - setSessionPage(sessionPage + 1); + setSessionPage((p) => p + 1); }[ client, remoteNode, - sessionPage, setSessions, appendSessions, setHasMoreSessions, setSessionPage, setSessionCursor, ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/src/features/agent/hooks/useAgentChat.ts` around lines 609 - 637, fetchSessionsPage captures sessionPage causing stale-closure reads and recreations; change the increment to use the functional updater (call setSessionPage(prev => prev + 1) inside fetchSessionsPage where setSessionPage(sessionPage + 1) is used) and then remove sessionPage from the fetchSessionsPage dependency array so fetchSessionsPage and loadMoreSessions won't be needlessly re-created on every page change; keep other deps (client, remoteNode, setSessions, appendSessions, setHasMoreSessions, setSessionCursor) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/v1/api.yaml`:
- Around line 6351-6360: Update the operation description for listAgentSessions
to explicitly document pagination precedence: state that when
paginationMode=cursor or an AgentSessionCursor value is provided the server will
use cursor pagination and ignore Page/PerPage parameters (and return
nextCursor), otherwise it will use page/perPage; also document the server
validation rule for conflicting inputs (e.g., if both cursor and page are
supplied and no paginationMode, the server prefers cursor) and return a 400 only
for malformed/invalid values. Reference operationId "listAgentSessions" and
parameters AgentSessionPaginationMode, AgentSessionCursor, Page, PerPage,
RemoteNode when adding this sentence so clients know exactly how the server
resolves mixed pagination inputs.
In `@internal/agent/api.go`:
- Around line 1628-1661: ListSessionsCursor currently calls
appendPersistedSessions which invokes getStoredSessionCost for every persisted
session, causing an O(n) DB fan-out; change ListSessionsCursor to mirror
ListSessionsPaginated by inlining the persisted-session collection loop (do not
call appendPersistedSessions) so you only gather persisted session metadata and
update activeIDs, then sort and slice, and finally call
loadVisibleStoredSessionCosts only for the visible items; ensure you reference
and preserve activeIDs handling, use
sessionIsAfterCursor/encodeSessionListCursor as before, and remove any
preemptive calls to getStoredSessionCost when building the full sessions list.
In `@internal/service/frontend/api/v1/agent_sessions.go`:
- Line 163: The JSON currently emits an empty string instead of null because
ptrOf(result.NextCursor) returns a non-nil pointer for ""—change to return nil
for empty cursors: add or use a helper like emptyToNilString that takes
result.NextCursor and returns nil if s=="" otherwise &s, and use that helper
when populating the NextCursor field (replace ptrOf(result.NextCursor) with
emptyToNilString(result.NextCursor) in agent_sessions.go).
- Around line 176-190: cursorPagination currently sets TotalRecords to the
page-size passed as count, which is incorrect for cursor-based pagination;
change cursorPagination (function name cursorPagination) to set TotalRecords to
0 (or a sentinel like -1) to indicate the total is unknown rather than using
count, and update any call-sites that expect TotalRecords to reflect this
semantic change (e.g., where cursorPagination is called to populate
api.Pagination in agent session responses) so UI/consumers don't display the
page-size as the total.
---
Outside diff comments:
In `@ui/src/features/agent/components/SessionSidebar.tsx`:
- Line 1: Add the missing GPL v3 license header to the SessionSidebar.tsx source
file by running the repository tooling: execute make addlicense (or apply the
standard GPL v3 header the tool uses) so the file gains the required license
comment at the top; ensure the header format matches other .tsx files in the
repo and commit the updated file.
- Around line 45-56: Add a guard to avoid concurrent loads: accept an
isLoadingMore boolean prop into the component and in the useEffect that creates
the IntersectionObserver (which uses sentinelRef, hasMore, onLoadMore) return
early when isLoadingMore is true so the observer is not created/triggered while
a fetch is in-flight; additionally ensure the loadMoreSessions implementation in
useAgentChat (which calls fetchSessionsPage and checks hasMoreSessions and
sessionCursor) exposes and sets this loading state (isLoadingMore) around the
async fetch to prevent concurrent invocations.
---
Nitpick comments:
In `@internal/agent/api.go`:
- Around line 1672-1681: Replace the index-based sort in sortSessionsNewestFirst
with the Go 1.21 idiomatic slices.SortStableFunc: use slices.SortStableFunc over
the sessions slice and provide a comparator that directly compares two
SessionWithState values (accessing their Session.UpdatedAt and Session.ID) to
order by UpdatedAt descending then by ID, avoiding capturing by index; update
imports to include "slices" and remove reliance on sort.SliceStable.
In `@internal/service/frontend/api/v1/agent_sessions_test.go`:
- Around line 264-269: The follow-up ListAgentSessions call reuses the Cursor
but doesn't set PaginationMode, which can make the test brittle; update the
second call to ListAgentSessions (using ListAgentSessionsRequestObject and
ListAgentSessionsParams) to explicitly set PaginationMode to
PaginationModeCursor along with Cursor and PerPage so the request unambiguously
uses cursor mode.
In `@ui/src/features/agent/hooks/useAgentChat.ts`:
- Around line 609-637: fetchSessionsPage captures sessionPage causing
stale-closure reads and recreations; change the increment to use the functional
updater (call setSessionPage(prev => prev + 1) inside fetchSessionsPage where
setSessionPage(sessionPage + 1) is used) and then remove sessionPage from the
fetchSessionsPage dependency array so fetchSessionsPage and loadMoreSessions
won't be needlessly re-created on every page change; keep other deps (client,
remoteNode, setSessions, appendSessions, setHasMoreSessions, setSessionCursor)
intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 446cc028-e916-4175-9846-da609c402dcf
📒 Files selected for processing (15)
api/v1/api.gen.goapi/v1/api.yamlinternal/agent/api.gointernal/agent/api_test.gointernal/agent/session.gointernal/agent/session_test.gointernal/agent/store.gointernal/service/frontend/api/v1/agent_sessions.gointernal/service/frontend/api/v1/agent_sessions_test.goui/src/api/v1/schema.tsui/src/features/agent/components/SessionSidebar.tsxui/src/features/agent/components/__tests__/SessionSidebar.test.tsxui/src/features/agent/context/AgentChatContext.tsxui/src/features/agent/hooks/__tests__/useAgentChat.test.tsxui/src/features/agent/hooks/useAgentChat.ts
| description: "Lists sessions for the current user. Use `paginationMode=cursor` and pass `cursor` to load the next page returned by `nextCursor`; `page` remains available for compatibility." | ||
| operationId: "listAgentSessions" | ||
| tags: | ||
| - "agent" | ||
| parameters: | ||
| - $ref: "#/components/parameters/RemoteNode" | ||
| - $ref: "#/components/parameters/AgentSessionPaginationMode" | ||
| - $ref: "#/components/parameters/AgentSessionCursor" | ||
| - $ref: "#/components/parameters/Page" | ||
| - $ref: "#/components/parameters/PerPage" |
There was a problem hiding this comment.
Document precedence when both pagination styles are provided.
Please specify how the server resolves requests that include both paginationMode=cursor/cursor and page/perPage (e.g., reject with 400 or prefer cursor). This avoids client-side ambiguity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/v1/api.yaml` around lines 6351 - 6360, Update the operation description
for listAgentSessions to explicitly document pagination precedence: state that
when paginationMode=cursor or an AgentSessionCursor value is provided the server
will use cursor pagination and ignore Page/PerPage parameters (and return
nextCursor), otherwise it will use page/perPage; also document the server
validation rule for conflicting inputs (e.g., if both cursor and page are
supplied and no paginationMode, the server prefers cursor) and return a 400 only
for malformed/invalid values. Reference operationId "listAgentSessions" and
parameters AgentSessionPaginationMode, AgentSessionCursor, Page, PerPage,
RemoteNode when adding this sentence so clients know exactly how the server
resolves mixed pagination inputs.
| func (a *API) ListSessionsCursor(ctx context.Context, userID string, cursor string, perPage int) (SessionCursorResult, error) { | ||
| pg := exec.NewPaginator(1, perPage) | ||
|
|
||
| activeIDs := make(map[string]struct{}) | ||
| sessions := a.collectActiveSessions(userID, activeIDs) | ||
| sessions = a.appendPersistedSessions(ctx, userID, activeIDs, sessions) | ||
| sortSessionsNewestFirst(sessions) | ||
|
|
||
| start := 0 | ||
| if cursor != "" { | ||
| decoded, err := decodeSessionListCursor(cursor) | ||
| if err != nil { | ||
| return SessionCursorResult{}, err | ||
| } | ||
| start = len(sessions) | ||
| for i, sess := range sessions { | ||
| if sessionIsAfterCursor(sess.Session, decoded) { | ||
| start = i | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| end := min(start+pg.Limit(), len(sessions)) | ||
| items := sessions[start:end] | ||
| a.loadVisibleStoredSessionCosts(ctx, activeIDs, items) | ||
|
|
||
| var nextCursor string | ||
| if end < len(sessions) && len(items) > 0 { | ||
| nextCursor = encodeSessionListCursor(items[len(items)-1].Session) | ||
| } | ||
|
|
||
| return SessionCursorResult{Items: items, NextCursor: nextCursor}, nil | ||
| } |
There was a problem hiding this comment.
ListSessionsCursor negates its own cost-loading optimization by using appendPersistedSessions
appendPersistedSessions (line 1633) calls getStoredSessionCost — which does GetMessages + ListSubSessions — for every persisted session. loadVisibleStoredSessionCosts (line 1653) then loads the same cost a second time for visible-page items. The O(n) DB fan-out occurs regardless of page size.
ListSessionsPaginated avoids this correctly by inlining the persisted-session loop without calling getStoredSessionCost and relying entirely on loadVisibleStoredSessionCosts for the visible slice. ListSessionsCursor should mirror that pattern:
🐛 Proposed fix
func (a *API) ListSessionsCursor(ctx context.Context, userID string, cursor string, perPage int) (SessionCursorResult, error) {
pg := exec.NewPaginator(1, perPage)
activeIDs := make(map[string]struct{})
sessions := a.collectActiveSessions(userID, activeIDs)
- sessions = a.appendPersistedSessions(ctx, userID, activeIDs, sessions)
+ if a.store != nil {
+ persisted, err := a.store.ListSessions(ctx, userID)
+ if err != nil {
+ a.logger.Warn("Failed to list persisted sessions", "error", err)
+ } else {
+ for _, sess := range persisted {
+ if _, exists := activeIDs[sess.ID]; exists {
+ continue
+ }
+ if sess.ParentSessionID != "" {
+ continue
+ }
+ sessions = append(sessions, SessionWithState{
+ Session: *sess,
+ Working: false,
+ })
+ }
+ }
+ }
sortSessionsNewestFirst(sessions)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/agent/api.go` around lines 1628 - 1661, ListSessionsCursor currently
calls appendPersistedSessions which invokes getStoredSessionCost for every
persisted session, causing an O(n) DB fan-out; change ListSessionsCursor to
mirror ListSessionsPaginated by inlining the persisted-session collection loop
(do not call appendPersistedSessions) so you only gather persisted session
metadata and update activeIDs, then sort and slice, and finally call
loadVisibleStoredSessionCosts only for the visible items; ensure you reference
and preserve activeIDs handling, use
sessionIsAfterCursor/encodeSessionListCursor as before, and remove any
preemptive calls to getStoredSessionCost when building the full sessions list.
| return api.ListAgentSessions200JSONResponse{ | ||
| Sessions: toAPISessions(result.Items), | ||
| Pagination: cursorPagination(len(result.Items), result.NextCursor), | ||
| NextCursor: ptrOf(result.NextCursor), |
There was a problem hiding this comment.
ptrOf(result.NextCursor) emits "" instead of null when there is no next page.
ptrOf("") produces a non-nil pointer, so the JSON response contains "nextCursor": "" rather than "nextCursor": null when the result set is exhausted. Clients that test nextCursor != null will never see the end-of-list signal; only clients relying on the JS falsy evaluation of "" will work correctly. The cleaner contract is null when exhausted.
🐛 Proposed fix
- NextCursor: ptrOf(result.NextCursor),
+ NextCursor: emptyToNilString(result.NextCursor),Add a small helper (or inline the check):
// emptyToNilString returns nil for an empty string, otherwise a pointer to the value.
func emptyToNilString(s string) *string {
if s == "" {
return nil
}
return &s
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/frontend/api/v1/agent_sessions.go` at line 163, The JSON
currently emits an empty string instead of null because ptrOf(result.NextCursor)
returns a non-nil pointer for ""—change to return nil for empty cursors: add or
use a helper like emptyToNilString that takes result.NextCursor and returns nil
if s=="" otherwise &s, and use that helper when populating the NextCursor field
(replace ptrOf(result.NextCursor) with emptyToNilString(result.NextCursor) in
agent_sessions.go).
| func cursorPagination(count int, nextCursor string) api.Pagination { | ||
| totalPages := 1 | ||
| nextPage := 1 | ||
| if nextCursor != "" { | ||
| totalPages = 2 | ||
| nextPage = 2 | ||
| } | ||
| return api.Pagination{ | ||
| CurrentPage: 1, | ||
| NextPage: nextPage, | ||
| PrevPage: 1, | ||
| TotalPages: totalPages, | ||
| TotalRecords: count, | ||
| } | ||
| } |
There was a problem hiding this comment.
TotalRecords reflects page-size, not total session count.
cursorPagination receives count = len(result.Items) and assigns it to TotalRecords. This is the number of items in the current page, not the total number of sessions. Any consumer that renders a "Total: N records" label from this field will display the wrong value. For cursor-based pagination where the total is unknown, the honest choice is 0 (or to omit / use -1 as a sentinel) rather than silently misusing the field with the page size.
🐛 Proposed fix
-func cursorPagination(count int, nextCursor string) api.Pagination {
+func cursorPagination(nextCursor string) api.Pagination {
totalPages := 1
nextPage := 1
if nextCursor != "" {
totalPages = 2
nextPage = 2
}
return api.Pagination{
CurrentPage: 1,
NextPage: nextPage,
PrevPage: 1,
TotalPages: totalPages,
- TotalRecords: count,
+ TotalRecords: 0, // total unknown in cursor mode
}
}And update the call-site:
- Pagination: cursorPagination(len(result.Items), result.NextCursor),
+ Pagination: cursorPagination(result.NextCursor),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/frontend/api/v1/agent_sessions.go` around lines 176 - 190,
cursorPagination currently sets TotalRecords to the page-size passed as count,
which is incorrect for cursor-based pagination; change cursorPagination
(function name cursorPagination) to set TotalRecords to 0 (or a sentinel like
-1) to indicate the total is unknown rather than using count, and update any
call-sites that expect TotalRecords to reflect this semantic change (e.g., where
cursorPagination is called to populate api.Pagination in agent session
responses) so UI/consumers don't display the page-size as the total.
Summary
Improve the AI agent session list so it uses cursor pagination, loads additional sessions incrementally, and shows compact first-user-message labels instead of date-only rows.
Changes
Related Issues
N/A
Checklist