feat(app): sort sidebar sessions by user activity#476
Merged
Conversation
Astro-Han
added
enhancement
Contributor
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR adds activity-based session ordering end-to-end: backend list endpoints and cursors now support an "activity" sort (based on last user-message timestamps), session rows expose activity metadata, and frontend window/state and sidebar time logic are updated to consume and prefer activity timestamps when selecting and rendering the initial Pawwork window. ChangesActivity-Based Session Ordering (single cross-stack DAG)
sequenceDiagram
autonumber
participant Frontend as Client (UI)
participant API as Server (opencode)
participant DB as Database
rect rgba(100,150,240,0.5)
Frontend->>API: GET /sessions?sort=activity&limit=N
API->>DB: SELECT ... activityAtExpr, lastUserMessageAtExpr ... ORDER BY activityAt DESC, id DESC
DB-->>API: rows with activityAt, lastUserMessageAt
API->>API: encodeActivitySessionCursor(nextRow.activityAt, nextRow.id)
API-->>Frontend: 200 OK (rows), x-next-cursor: encodedActivityCursor
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 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 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new "activity" sorting mode for sessions, which prioritizes the time of the latest user message over the session creation time. Changes include backend SQL updates to calculate activity timestamps, API route enhancements for sorting and cursor-based pagination, and frontend updates to utilize this new sorting logic in the session layout. Feedback suggests improving the robustness of cursor validation for updated-time sorting and simplifying the logic for mapping user message timestamps in the session list.
Astro-Han
force-pushed
the
codex/i473-sidebar-activity
branch
from
40203a3 to
622fb2f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/app/src/pages/layout.tsx (2)
596-609:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPinned/active fallback loads drop the activity timestamp.
Anything not already present in the
sort: "activity"page is fetched throughsession.get(), sopinned/activeentries here can enter the window withoutactivityAt. Those rows then fall back to cache ortime.created, which breaks the new “same activity source for ordering and row timestamps” rule for exactly the sessions this block backfills.🤖 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 `@packages/app/src/pages/layout.tsx` around lines 596 - 609, The pinned/active fallbacks lose the activity timestamp because sessions fetched via loadSessionByID (or session.get()) may lack time.activityAt; when building pinned and active from loaded.get(id) ?? (await loadSessionByID(id)) preserve or inject the activity timestamp from the original response-loaded session: use the loaded Map (loaded) entry's time.activityAt (from response.data / PawworkWindowSession) to set time.activityAt on the fallback session if it is missing. Apply the same preservation when constructing active (params.id) so loadSessionByID results inherit activityAt from loaded.get(params.id) if present.
646-659:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve activity metadata when applying
session.updated.
infois a plainSession, so replacing an existingPawworkWindowSessionwith it dropsactivityAtandlastUserMessageAt. After anysession.updatedevent, that row can immediately fall back to creation-time ordering until the next full reload.Suggested fix
const upsertPawworkWindowSession = (info: Session) => { if (info.parentID || info.time?.archived) return + const mergeWindowSession = (current: PawworkWindowSession[]) => { + const existing = current.find((session) => session.id === info.id) + const next: PawworkWindowSession = { + ...info, + activityAt: existing?.activityAt, + lastUserMessageAt: existing?.lastUserMessageAt, + } + return sortPawworkSessionWindowSessions([...current.filter((session) => session.id !== info.id), next]) + } batch(() => { - setPawworkSessionWindowState("normal", (current) => - sortPawworkSessionWindowSessions([...current.filter((session) => session.id !== info.id), info]), - ) + setPawworkSessionWindowState("normal", mergeWindowSession) if (store.pawworkPinnedSessions.includes(info.id)) { - setPawworkSessionWindowState("pinned", (current) => - sortPawworkSessionWindowSessions([...current.filter((session) => session.id !== info.id), info]), - ) + setPawworkSessionWindowState("pinned", mergeWindowSession) } if (params.id === info.id) { - setPawworkSessionWindowState("active", info) + setPawworkSessionWindowState("active", (current) => + current?.id === info.id + ? { + ...info, + activityAt: current.activityAt, + lastUserMessageAt: current.lastUserMessageAt, + } + : info, + ) } }) }🤖 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 `@packages/app/src/pages/layout.tsx` around lines 646 - 659, upsertPawworkWindowSession is replacing existing PawworkWindowSession entries with the plain Session `info`, which drops derived fields like activityAt and lastUserMessageAt; change the insertion logic in setPawworkSessionWindowState calls (and the active/pinned branches) to merge `info` with any existing session object (lookup by id) so you preserve existing activityAt and lastUserMessageAt when updating (i.e., find existing session from `current` before filter and use { ...existing, ...info } or similar) so session.updated does not wipe those metadata fields.packages/app/src/pages/layout/pawwork-session-source.ts (1)
72-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't let a stale
activityAthide fresher loaded user input.Line 73 turns
activityAtinto a hard override, but that value is only as fresh as the last global-list fetch. If the current session gets a newer local user message before the window reloads, the sidebar will keep sorting/displaying the older timestamp instead of the latest known user activity.Suggested fix
export function pawworkSidebarSessionTime(session: SessionTimeLike, messages?: MessageTimeLike[]) { - if (isFiniteNumber(session.activityAt)) return session.activityAt + let latestLoadedUserAt: number | undefined for (let i = (messages?.length ?? 0) - 1; i >= 0; i--) { const message = messages?.[i] if (message?.role !== "user") continue const created = message.time?.created - if (isFiniteNumber(created)) return created + if (isFiniteNumber(created)) { + latestLoadedUserAt = created + break + } } + if (isFiniteNumber(session.activityAt)) { + return latestLoadedUserAt === undefined ? session.activityAt : Math.max(session.activityAt, latestLoadedUserAt) + } + if (latestLoadedUserAt !== undefined) return latestLoadedUserAt const sessionCreated = session.time?.created return isFiniteNumber(sessionCreated) ? sessionCreated : 0 }🤖 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 `@packages/app/src/pages/layout/pawwork-session-source.ts` around lines 72 - 81, The function pawworkSidebarSessionTime currently returns session.activityAt unconditionally if it's finite, which can hide a newer local user message; change the logic to compare timestamps: find the latest user message timestamp (iterate messages for message.role === "user" and check message.time?.created), then if session.activityAt is finite and >= that latest user timestamp return session.activityAt, otherwise return the latest user timestamp if finite, else fallback to session.time?.created (if finite) or 0; keep using isFiniteNumber for all timestamp checks to locate and update the logic around session.activityAt, messages, message.time?.created, and session.time?.created in pawworkSidebarSessionTime.
🤖 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 `@packages/opencode/src/session/session.ts`:
- Around line 1037-1052: sessionOrder currently treats any non-"created" sort as
"updated", causing list({ sort: "activity" }) to be silently wrong; update
sessionOrder to explicitly handle the "activity" case instead of defaulting to
updated: add a branch for sort === "activity" that returns the correct ordering
(e.g. [desc(SessionTable.activity_at), desc(SessionTable.id)]) so Session.list()
and listGlobal() behave as advertised by ListSort, or if the activity column is
not present, throw a clear error when sort === "activity" so callers get
explicit feedback.
---
Outside diff comments:
In `@packages/app/src/pages/layout.tsx`:
- Around line 596-609: The pinned/active fallbacks lose the activity timestamp
because sessions fetched via loadSessionByID (or session.get()) may lack
time.activityAt; when building pinned and active from loaded.get(id) ?? (await
loadSessionByID(id)) preserve or inject the activity timestamp from the original
response-loaded session: use the loaded Map (loaded) entry's time.activityAt
(from response.data / PawworkWindowSession) to set time.activityAt on the
fallback session if it is missing. Apply the same preservation when constructing
active (params.id) so loadSessionByID results inherit activityAt from
loaded.get(params.id) if present.
- Around line 646-659: upsertPawworkWindowSession is replacing existing
PawworkWindowSession entries with the plain Session `info`, which drops derived
fields like activityAt and lastUserMessageAt; change the insertion logic in
setPawworkSessionWindowState calls (and the active/pinned branches) to merge
`info` with any existing session object (lookup by id) so you preserve existing
activityAt and lastUserMessageAt when updating (i.e., find existing session from
`current` before filter and use { ...existing, ...info } or similar) so
session.updated does not wipe those metadata fields.
In `@packages/app/src/pages/layout/pawwork-session-source.ts`:
- Around line 72-81: The function pawworkSidebarSessionTime currently returns
session.activityAt unconditionally if it's finite, which can hide a newer local
user message; change the logic to compare timestamps: find the latest user
message timestamp (iterate messages for message.role === "user" and check
message.time?.created), then if session.activityAt is finite and >= that latest
user timestamp return session.activityAt, otherwise return the latest user
timestamp if finite, else fallback to session.time?.created (if finite) or 0;
keep using isFiniteNumber for all timestamp checks to locate and update the
logic around session.activityAt, messages, message.time?.created, and
session.time?.created in pawworkSidebarSessionTime.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 58b11065-a2b6-4982-a44f-1e021018fe12
⛔ Files ignored due to path filters (2)
packages/sdk/js/src/v2/gen/sdk.gen.tsis excluded by!**/gen/**packages/sdk/js/src/v2/gen/types.gen.tsis excluded by!**/gen/**
📒 Files selected for processing (8)
packages/app/src/pages/layout.tsxpackages/app/src/pages/layout/pawwork-session-source.test.tspackages/app/src/pages/layout/pawwork-session-source.tspackages/app/src/pages/layout/pawwork-session-window.test.tspackages/app/src/pages/layout/pawwork-session-window.tspackages/opencode/src/server/instance/experimental.tspackages/opencode/src/session/session.tspackages/opencode/test/server/global-session-activity-list.test.ts
Astro-Han
force-pushed
the
codex/i473-sidebar-activity
branch
5 times, most recently
from
d6910f5 to
df70d17
Compare
Astro-Han
force-pushed
the
codex/i473-sidebar-activity
branch
from
df70d17 to
5ecc0d9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/opencode/src/session/session.ts (1)
1055-1094: Indexes are already in place; consider a CTE optimization for future performance tuning.
lastUserMessageAtExpris a correlated scalar subquery with nestedNOT EXISTSclauses againstPartTable. Because Drizzle inlines SQL-fragment objects at every call site, the activity-sort path with a cursor evaluates this subquery five times per session row: twice inSELECT(foractivityAtandlastUserMessageAt), twice in theWHEREOR-branches (insidelt(…)andeq(…)which both wrapactivityAtExpr), and once inORDER BY.The required indexes are already present in the schema:
MessageTable: composite index on(session_id, time_created, id)— which is even more specific than the minimum neededPartTable: indexes on(message_id, id)and(session_id)covering both NOT EXISTS pathsTo further optimize if this becomes a bottleneck, consider wrapping the correlated computation in a SQL CTE or subquery (using Drizzle's
.as()or a rawWITHclause) so SQLite computesactivityAtonce per row and reuses it by name inORDER BYandWHERE, avoiding redundant evaluations.🤖 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 `@packages/opencode/src/session/session.ts` around lines 1055 - 1094, The correlated scalar subquery lastUserMessageAtExpr is being inlined multiple times (for activityAtExpr and in WHERE/ORDER BY), causing redundant evaluations; refactor to compute it once via a CTE or named subquery and reference that name for activityAt/activityAtExpr/lastUserMessageAt in activitySelect and in any lt(...)/eq(...) or ORDER BY usage so SQLite reuses the single computed value per Session row (use Drizzle's .as() or a raw WITH to create the CTE that selects session id plus lastUserMessageAt from MessageTable/PartTable, then join/consume that CTE when building activitySelect).
🤖 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 `@packages/app/src/pages/layout.tsx`:
- Around line 629-632: Pinned/active sessions fetched via loaded.get or
loadSessionByID are missing activity metadata (activityAt/lastUserMessageAt),
causing ordering issues; update the logic so IDs not present in response.data
are hydrated through an activity-aware endpoint or a per-session fetch that
returns the same metadata instead of plain Session: change calls to
loadSessionByID to use (or implement) a loadSessionWithActivityByID or a
fetchSessionsActivityByIDs and then pass that enriched session into
mergePawworkWindowSessionMetadata (and likewise update the identical block at
637-640) so existing.get(id) is merged with full activity-aware session objects.
In `@packages/app/src/pages/layout/pawwork-session-source.ts`:
- Around line 94-97: The no-activityAt fallback currently picks the newest
loaded user message without checking part-based eligibility, so update the
fallback logic that selects the newest user message to also call
partsForMessage(message.id) and pass that into isActivityEligibleUserMessage; if
the check returns false, skip/continue and try the next message. Apply the same
change to the other fallback occurrence that uses the newest loaded message (the
second block that mirrors this logic) so both selection sites honor
requireEligibility by consulting partsForMessage and
isActivityEligibleUserMessage before promoting a message.
---
Nitpick comments:
In `@packages/opencode/src/session/session.ts`:
- Around line 1055-1094: The correlated scalar subquery lastUserMessageAtExpr is
being inlined multiple times (for activityAtExpr and in WHERE/ORDER BY), causing
redundant evaluations; refactor to compute it once via a CTE or named subquery
and reference that name for activityAt/activityAtExpr/lastUserMessageAt in
activitySelect and in any lt(...)/eq(...) or ORDER BY usage so SQLite reuses the
single computed value per Session row (use Drizzle's .as() or a raw WITH to
create the CTE that selects session id plus lastUserMessageAt from
MessageTable/PartTable, then join/consume that CTE when building
activitySelect).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 03d7a065-fe0a-40a3-a70d-aa0c1a35e810
⛔ Files ignored due to path filters (2)
packages/sdk/js/src/v2/gen/sdk.gen.tsis excluded by!**/gen/**packages/sdk/js/src/v2/gen/types.gen.tsis excluded by!**/gen/**
📒 Files selected for processing (9)
packages/app/src/pages/layout.tsxpackages/app/src/pages/layout/pawwork-session-source.test.tspackages/app/src/pages/layout/pawwork-session-source.tspackages/app/src/pages/layout/pawwork-session-window.test.tspackages/app/src/pages/layout/pawwork-session-window.tspackages/opencode/src/server/instance/experimental.tspackages/opencode/src/session/session.tspackages/opencode/test/server/global-session-activity-list.test.tspackages/opencode/test/server/global-session-list.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
sort=activityto the global experimental session list contract.Why
The PawWork sidebar first window was still selected by session creation time. PR #471 fixed timestamps for rows already loaded into the sidebar, but an older session with recent user activity could still be hidden outside the initial window. This PR moves the selection contract to the backend/API layer so the first sidebar window represents conversations the user most recently typed into.
Related Issue
Closes #473
Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
Please check the activity-order cursor semantics in
Session.listGlobal, especially same-time pagination and the rule that assistant-only messages,session.time.updated, compaction messages, and synthetic-only user messages do not promote a session. Real user messages that also carry synthetic reminder parts should still count as user activity.Risk Notes
Medium. This adds a new global session list sort mode and uses correlated message/part queries for activity ordering. Existing
updatedandcreatedsort modes remain covered by tests and unchanged for existing clients. No persistence migration, dependency, packaging, updater, signing, or permission changes are included.How To Verify
Screenshots or Recordings
Not captured. This changes sidebar data ordering, not layout or copy. The ordering contract is covered by backend route tests and frontend helper tests; Electron startup was verified.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
New Features
Refactor
Tests