Skip to content

feat(ui): sort sessions by recent activity#1738

Open
officialasishkumar wants to merge 4 commits intokagent-dev:mainfrom
officialasishkumar:feat/sort-sessions-by-last-activity
Open

feat(ui): sort sessions by recent activity#1738
officialasishkumar wants to merge 4 commits intokagent-dev:mainfrom
officialasishkumar:feat/sort-sessions-by-last-activity

Conversation

@officialasishkumar
Copy link
Copy Markdown
Contributor

Summary

  • Sort session lists by updated_at descending so active long-running chats stay visible
  • Touch session activity when events or tasks are stored
  • Group and display sidebar sessions by last activity, with story coverage for older sessions updated today

Fixes #1556

Testing

  • go test ./core/internal/database -run 'TestListSessionsOrdersByRecentActivity|TestStoreEventTouchesSessionActivity|TestStoreTaskTouchesSessionActivity' -count=1\n- npm run lint -- --quiet

@github-actions github-actions Bot added the enhancement New feature or request label Apr 23, 2026
Copy link
Copy Markdown
Contributor

@jsonmp-k8 jsonmp-k8 left a comment

Choose a reason for hiding this comment

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

Good feature — sorting by activity is the right UX. A few issues to address:

1. StoreTask uses TouchSession (no user_id check) while StoreEvents uses TouchSessionForUser (with user_id check)

TouchSession updates any session matching the ID regardless of who owns it. TouchSessionForUser scopes to the user. Since protocol.Task doesn't carry a user_id, using TouchSession is understandable, but it means any task upsert can bump another user's session timestamp. Consider whether this matters for multi-tenant scenarios, or add a comment explaining the choice.

2. Fake client StoreTask touch logic iterates all sessions

for _, session := range c.sessions {
    if session.ID == task.ContextID {
        session.UpdatedAt = now
    }
}

This scans every session to find one by ID. The sessions map is keyed by sessionKey(id, userID), so you can't do a direct lookup without the user_id. This is fine for a fake/test client, but worth noting — a break after the match would be cleaner since session IDs are unique.

3. sortSessionsByActivity can use cmp.Compare instead of manual if-chains

The time.Time.Compare method returns -1/0/1 directly. You can simplify:

func sortSessionsByActivity(sessions []database.Session) {
    slices.SortStableFunc(sessions, func(i, j database.Session) int {
        if c := j.UpdatedAt.Compare(i.UpdatedAt); c != 0 {
            return c
        }
        if c := j.CreatedAt.Compare(i.CreatedAt); c != 0 {
            return c
        }
        return strings.Compare(i.ID, j.ID)
    })
}

Note the j before i to get descending order.

4. UI: createdAt prop name is now misleading

In SessionGroup.tsx, the createdAt prop now receives session.updated_at || session.created_at. The prop name createdAt no longer reflects what it contains. Consider renaming it to lastActivity or activityAt to avoid confusion for future readers.

Overall the backend changes (SQL, transaction wrapping, tests) look solid. The test coverage is thorough.

Copy link
Copy Markdown
Contributor

@jsonmp-k8 jsonmp-k8 left a comment

Choose a reason for hiding this comment

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

.

@officialasishkumar
Copy link
Copy Markdown
Contributor Author

Addressed the review feedback in fe08838: added the task touch comment, broke the fake-client loop on match, simplified the sort comparator, and renamed the UI timestamp prop to activityAt.

@iplay88keys iplay88keys self-assigned this Apr 28, 2026
@officialasishkumar officialasishkumar force-pushed the feat/sort-sessions-by-last-activity branch from fe08838 to c062577 Compare April 28, 2026 18:44
Comment on lines +154 to +169
if err := q.InsertEvent(ctx, dbgen.InsertEventParams{
ID: e.ID,
UserID: e.UserID,
SessionID: sessionID,
Data: e.Data,
}); err != nil {
return fmt.Errorf("failed to store event %s: %w", e.ID, err)
}
if sessionID != nil {
if err := q.TouchSessionForUser(ctx, dbgen.TouchSessionForUserParams{
ID: *sessionID,
UserID: e.UserID,
}); err != nil {
return fmt.Errorf("failed to touch session %s: %w", *sessionID, err)
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This feels ugly, maybe the InsertEvent query should also touch the session automatically?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved the session activity update into the InsertEvent SQL path in 4699816.

SessionID: strPtrIfNotEmpty(task.ContextID),
return c.withTx(ctx, func(q *dbgen.Queries) error {
sessionID := strPtrIfNotEmpty(task.ContextID)
if err := q.UpsertTask(ctx, dbgen.UpsertTaskParams{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same comment here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Handled the same way for task upserts in 4699816 by updating the session in the SQL CTE.

@officialasishkumar officialasishkumar force-pushed the feat/sort-sessions-by-last-activity branch from 05c3b27 to 4699816 Compare April 29, 2026 21:07
@EItanya
Copy link
Copy Markdown
Contributor

EItanya commented Apr 30, 2026

Hey there @officialasishkumar I deleted the fake db client in a recent PR, please resolve the conflicts and then I will merge

@officialasishkumar officialasishkumar force-pushed the feat/sort-sessions-by-last-activity branch from 4699816 to 639ed77 Compare April 30, 2026 18:12
EItanya
EItanya previously approved these changes Apr 30, 2026
@officialasishkumar
Copy link
Copy Markdown
Contributor Author

Rebased on current main and resolved the conflict by keeping the upstream fake-client deletion. go test ./core/internal/database passes from the go/ directory.

Copilot AI review requested due to automatic review settings May 1, 2026 12:56
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Implements “recent activity” session ordering across the backend and UI so long-running active chats stay visible, aligning with the requested behavior in #1556.

Changes:

  • Update session list queries to sort by updated_at (desc) with created_at as a tie-breaker.
  • Touch session.updated_at when storing events or tasks.
  • Update sidebar grouping/sorting and timestamps to use last activity time (updated_at fallback to created_at), with Storybook coverage for an older-but-recently-active session.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
ui/src/components/sidebars/SessionGroup.tsx Passes last-activity timestamp to each chat row.
ui/src/components/sidebars/GroupedChats.tsx Groups/sorts sessions by last activity date.
ui/src/components/sidebars/GroupedChats.stories.tsx Adds story covering an older session updated today.
ui/src/components/sidebars/ChatItem.tsx Renames timestamp prop to activityAt and displays it.
ui/src/components/sidebars/ChatItem.stories.tsx Updates stories to use activityAt.
go/core/internal/database/queries/tasks.sql Touches session updated_at on task upsert.
go/core/internal/database/queries/sessions.sql Orders session lists by updated_at desc.
go/core/internal/database/queries/events.sql Touches session updated_at on event insert.
go/core/internal/database/gen/tasks.sql.go Regenerates sqlc output for updated task query.
go/core/internal/database/gen/sessions.sql.go Regenerates sqlc output for updated session list ordering.
go/core/internal/database/gen/events.sql.go Regenerates sqlc output for updated event insert query.
go/core/internal/database/client_test.go Adds DB tests for ordering + activity touch behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +42 to +43
const getActivityDate = (session: Session) => new Date(session.updated_at || session.created_at);

Copy link

Copilot AI May 1, 2026

Choose a reason for hiding this comment

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

getActivityDate() allocates a new Date each time it’s called, and the sort comparator calls it repeatedly (O(n log n)). Consider computing a numeric activity timestamp once per session (e.g., Date.parse(updated_at ?? created_at)) and sorting on that to reduce allocations and repeated parsing.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in b7a3418 by computing one numeric activity timestamp per session and sorting on that cached value.

UPDATE session
SET updated_at = NOW()
FROM inserted_event
WHERE session.id = inserted_event.session_id
Copy link

Copilot AI May 1, 2026

Choose a reason for hiding this comment

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

This UPDATE always runs even when the inserted event has a NULL session_id (common when StoreEvents passes an empty session). Adding a inserted_event.session_id IS NOT NULL predicate (or guarding in the CTE) would avoid doing an unnecessary UPDATE step.

Suggested change
WHERE session.id = inserted_event.session_id
WHERE inserted_event.session_id IS NOT NULL
AND session.id = inserted_event.session_id

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in b7a3418 by adding the inserted_event.session_id IS NOT NULL guard before touching the session.

UPDATE session
SET updated_at = NOW()
FROM upserted_task
WHERE session.id = upserted_task.session_id
Copy link

Copilot AI May 1, 2026

Choose a reason for hiding this comment

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

Similar to events, this UPDATE step will still be planned/executed even if the upserted task’s session_id is NULL. Consider adding upserted_task.session_id IS NOT NULL to the UPDATE’s WHERE clause to skip the session touch work when there’s no session associated.

Suggested change
WHERE session.id = upserted_task.session_id
WHERE upserted_task.session_id IS NOT NULL
AND session.id = upserted_task.session_id

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in b7a3418 by adding the upserted_task.session_id IS NOT NULL guard before touching the session.

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
@officialasishkumar officialasishkumar force-pushed the feat/sort-sessions-by-last-activity branch from a28660d to 1817503 Compare May 5, 2026 08:22
Comment on lines +262 to +266
_, err = db.Exec(ctx, `
UPDATE session
SET created_at = $1, updated_at = $2
WHERE id = $3 AND user_id = $4
`, s.createdAt, s.updatedAt, s.id, userID)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In general tests like this shouldn't execute raw SQL. Rather we should use the new queries to prove the actual behavior

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] Sort session history by most recent activity (instead of creation)

5 participants