feat: add status/author filters and sortable columns to sessions page#1518
Open
quay-devel wants to merge 12 commits intoambient-code:mainfrom
Open
feat: add status/author filters and sortable columns to sessions page#1518quay-devel wants to merge 12 commits intoambient-code:mainfrom
quay-devel wants to merge 12 commits intoambient-code:mainfrom
Conversation
…sions list Add phase filter dropdown (Active/Completed/Failed), "My sessions" toggle using current user identity, and clickable sortable Created column header. Filter params flow through PaginationParams -> API layer -> query keys for proper cache isolation. Includes 4 new tests for filter param passthrough. Ref: ambient-code#1515 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ist endpoint Add server-side filtering by phase and userId, plus configurable sort direction to the ListSessions handler, supporting the sessions page filter/sort UI (ambient-code#1515). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove unused sortSessionsByCreationTime() function (replaced by sortSessions()) - Add sortDirection to offset reset dependency array for correct UX on sort toggle Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
Contributor
📝 WalkthroughWalkthroughAdds server-side session filtering and configurable sorting. Backend extends pagination params and updates ListSessions to filter by phase and userId and to sort by requested column/direction. Frontend adds phase dropdown, "My sessions" toggle, sortable Name/Created headers, and forwards new query params to the API. ChangesSessions Filtering & Sorting
Sequence DiagramsequenceDiagram
participant User as User
participant Frontend as SessionsSection
participant API as API Client
participant Backend as ListSessions Handler
participant K8s as K8s API
User->>Frontend: Select phase, toggle "My sessions", set sort
Frontend->>Frontend: Update state (phase, mySessionsOnly, sortBy, sortDirection)
Frontend->>API: listSessionsPaginated({phase, userId, sortBy, sortDirection})
API->>Backend: GET /sessions?phase=...&userId=...&sortBy=...&sortDirection=...
Backend->>K8s: List all AgenticSession CRs
K8s-->>Backend: All sessions
Backend->>Backend: Apply search filter
Backend->>Backend: Apply phase filter
Backend->>Backend: Apply userId filter
Backend->>Backend: Sort by requested column/direction
Backend-->>API: Filtered & sorted sessions
API-->>Frontend: Sessions data
Frontend->>Frontend: Render table with chips and sort indicators
Frontend-->>User: Display results
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Microsoft Presidio Analyzer (2.2.362)components/backend/handlers/sessions.goMicrosoft Presidio Analyzer failed to scan this file 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 |
Make the Name column header clickable for sorting, matching the existing Created column behavior. Clicking the active sort column toggles direction; clicking an inactive column switches to it with a sensible default (asc for name, desc for created). Arrow icons only show on the active sort column. The backend already supports sortBy=name, so no backend changes needed. Refs ambient-code#1515
![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: 3
🧹 Nitpick comments (1)
components/backend/handlers/sessions_test.go (1)
342-386: ⚡ Quick winAdd coverage for
sortBy=namepathCurrent additions validate
sortDirection, but not the"name"branch itself. A regression insortBy=namewouldn’t be caught by this suite.🤖 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 `@components/backend/handlers/sessions_test.go` around lines 342 - 386, Add a test case that exercises the "sortBy=name" branch by calling the ListSessions handler with the query param sortBy=name (and both sortDirection=asc and the default/desc case) using httpUtils.CreateTestGinContext and the existing createTestSessionWithOptions("alpha-"+randomName...) / createTestSessionWithOptions("beta-"+randomName...) setup; assert HTTP 200, parse response items, and verify ordering by metadata.name (alpha before beta for asc, beta before alpha for desc) so the sortBy=name code path is covered.
🤖 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 `@components/backend/handlers/sessions.go`:
- Around line 803-816: The comparator used by sort.Slice currently compares
sessions[i].Metadata["name"] but the UI displays spec.displayName ||
metadata.name; update the "name" case in the comparator to compute vi and vj by
first checking the session's spec.displayName (type-assert to string) and using
it if non-empty, otherwise falling back to sessions[].Metadata["name"]; then
lowercase both values for comparison. Modify the comparator inside sort.Slice
(the "name" case) to read spec.displayName for sessions[i] and sessions[j],
fallback to metadata.name, and set vi/vj accordingly before returning vi < vj
(leaving the default case using getSessionCreationTimestamp unchanged).
In `@components/frontend/src/components/workspace-sections/sessions-section.tsx`:
- Around line 329-362: The sortable TableHead elements (the ones toggling sort
for 'name' and 'created') are currently click-only; make them
keyboard-accessible by ensuring they are focusable and respond to Enter/Space:
add tabIndex={0} (or use a semantic <button>), a role="button" if not a native
button, and an onKeyDown handler that invokes the same logic used in the onClick
(toggling setSortDirection or setting setSortBy and default sortDirection). Also
add ARIA state like aria-sort on those headers to reflect sortBy/sortDirection
for screen readers (use 'none'/'ascending'/'descending' based on sortBy and
sortDirection). Ensure handlers reference the same setSortBy/setSortDirection
logic currently in the TableHead click handlers for 'name' and 'created'.
- Around line 118-120: The request builder is using mySessionsOnly while
currentUser may still be unresolved, causing userId to be undefined and
returning unfiltered results; update the logic in the SessionsSection where
userId is set (userId: mySessionsOnly ? currentUser?.userId : undefined) to only
include userId when currentUser?.userId exists (e.g., userId: mySessionsOnly &&
currentUser?.userId ? currentUser.userId : undefined) so the API is not queried
as "my sessions" with an undefined id, and also disable the "My sessions" toggle
control (the UI toggle bound to mySessionsOnly) until currentUser is loaded so
the user cannot enable the filter before currentUser is known.
---
Nitpick comments:
In `@components/backend/handlers/sessions_test.go`:
- Around line 342-386: Add a test case that exercises the "sortBy=name" branch
by calling the ListSessions handler with the query param sortBy=name (and both
sortDirection=asc and the default/desc case) using
httpUtils.CreateTestGinContext and the existing
createTestSessionWithOptions("alpha-"+randomName...) /
createTestSessionWithOptions("beta-"+randomName...) setup; assert HTTP 200,
parse response items, and verify ordering by metadata.name (alpha before beta
for asc, beta before alpha for desc) so the sortBy=name code path is covered.
🪄 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: Enterprise
Run ID: 02606c11-9e03-4797-9a17-67b53f51995f
📒 Files selected for processing (7)
components/backend/handlers/sessions.gocomponents/backend/handlers/sessions_test.gocomponents/backend/types/common.gocomponents/frontend/src/components/workspace-sections/sessions-section.tsxcomponents/frontend/src/services/api/sessions.tscomponents/frontend/src/services/queries/__tests__/use-sessions.test.tscomponents/frontend/src/types/api/common.ts
- Sort-by-name now uses displayName with fallback to metadata.name, matching what the UI displays - "My sessions" toggle disabled while currentUser is loading, and userId guard prevents unfiltered queries when user is unresolved - Sortable table headers are keyboard-accessible (tabIndex, role, onKeyDown, aria-sort) - Added sortBy=name test coverage (ascending and descending) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
components/frontend/src/components/workspace-sections/sessions-section.tsx (1)
330-359: ⚡ Quick winKeep
<th>semantics and move click/keyboard handlers to an inner<button>
aria-sortbelongs on the column header, but these cells also setrole="button", which can drop header semantics for assistive tech. KeepTableHeadas a header (aria-sortthere), and render an inner button for interaction.Also applies to: 362-391
🤖 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 `@components/frontend/src/components/workspace-sections/sessions-section.tsx` around lines 330 - 359, The TableHead currently has interactive semantics (role="button" and keyboard handlers) which overrides header semantics; keep the aria-sort on TableHead but remove role="button" and onClick/onKeyDown/tabIndex from TableHead, and instead render an inner <button> (or a Button-like element) inside the TableHead that contains the Name label and the ArrowUp/ArrowDown icons; move the click and keyboard logic there to call setSortBy('name') and setSortDirection(...) and ensure the inner button has accessible aria-label/aria-pressed if needed; apply the same change to the other similar TableHead block that handles sorting (the one that uses setSortBy/setSortDirection for the other column).
🤖 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 `@components/backend/handlers/sessions.go`:
- Around line 803-829: The comparator used in sort.Slice (inside sortSessions)
lacks a deterministic tie-breaker so when vi == vj the order is
non-deterministic; update the comparator in the sort.Slice call to, after
computing vi and vj (the primary keys from Spec.DisplayName or
getSessionCreationTimestamp), check if vi == vj and then compare a stable
secondary key such as sessions[i].Metadata["name"] (string) or another unique
identifier (e.g., a session ID field) to break ties; ensure the secondary
comparison respects ascending/descending by inverting the result when ascending
is false so pagination becomes deterministic.
In `@components/frontend/src/components/workspace-sections/sessions-section.tsx`:
- Line 85: The "My sessions" toggle can be enabled when currentUser.userId is
undefined; update the SessionsSection component to guard the toggle by checking
currentUser?.userId and isCurrentUserLoading: disable (or make the onChange a
no-op) when currentUser?.userId is falsy or while isCurrentUserLoading is true,
and ensure any filter logic that uses currentUser.userId (the code around the
toggle handler and the filtering logic referenced near lines 118 and 269-278)
only applies the userId when it exists (i.e., don't set or send userId undefined
to the request); useCurrentUser and its isLoading flag should determine the
toggle's disabled state and whether the onChange sets the user filter.
---
Nitpick comments:
In `@components/frontend/src/components/workspace-sections/sessions-section.tsx`:
- Around line 330-359: The TableHead currently has interactive semantics
(role="button" and keyboard handlers) which overrides header semantics; keep the
aria-sort on TableHead but remove role="button" and onClick/onKeyDown/tabIndex
from TableHead, and instead render an inner <button> (or a Button-like element)
inside the TableHead that contains the Name label and the ArrowUp/ArrowDown
icons; move the click and keyboard logic there to call setSortBy('name') and
setSortDirection(...) and ensure the inner button has accessible
aria-label/aria-pressed if needed; apply the same change to the other similar
TableHead block that handles sorting (the one that uses
setSortBy/setSortDirection for the other column).
🪄 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: Enterprise
Run ID: 3fe254f0-9fc7-4aec-8bce-e4b817786b3b
📒 Files selected for processing (3)
components/backend/handlers/sessions.gocomponents/backend/handlers/sessions_test.gocomponents/frontend/src/components/workspace-sections/sessions-section.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- components/backend/handlers/sessions_test.go
Comment on lines
803
to
829
| sort.Slice(sessions, func(i, j int) bool { | ||
| ts1 := getSessionCreationTimestamp(sessions[i]) | ||
| ts2 := getSessionCreationTimestamp(sessions[j]) | ||
| // Sort descending (newest first) - RFC3339 timestamps sort lexicographically | ||
| return ts1 > ts2 | ||
| var vi, vj string | ||
| switch sortBy { | ||
| case "name": | ||
| ni := sessions[i].Spec.DisplayName | ||
| if strings.TrimSpace(ni) == "" { | ||
| if name, ok := sessions[i].Metadata["name"].(string); ok { | ||
| ni = name | ||
| } | ||
| } | ||
| nj := sessions[j].Spec.DisplayName | ||
| if strings.TrimSpace(nj) == "" { | ||
| if name, ok := sessions[j].Metadata["name"].(string); ok { | ||
| nj = name | ||
| } | ||
| } | ||
| vi = strings.ToLower(ni) | ||
| vj = strings.ToLower(nj) | ||
| default: // "created" or any unrecognized value | ||
| vi = getSessionCreationTimestamp(sessions[i]) | ||
| vj = getSessionCreationTimestamp(sessions[j]) | ||
| } | ||
| if ascending { | ||
| return vi < vj | ||
| } | ||
| return vi > vj | ||
| }) |
Contributor
There was a problem hiding this comment.
Add deterministic tie-breaker in sortSessions to avoid pagination jitter
When vi == vj, the comparator returns false both ways. With sort.Slice and pagination, equal-key rows can reorder between requests, causing duplicate/missing sessions across pages. Add a stable secondary key (e.g., metadata.name) before returning.
Suggested fix
sort.Slice(sessions, func(i, j int) bool {
var vi, vj string
switch sortBy {
case "name":
ni := sessions[i].Spec.DisplayName
if strings.TrimSpace(ni) == "" {
if name, ok := sessions[i].Metadata["name"].(string); ok {
ni = name
}
}
nj := sessions[j].Spec.DisplayName
if strings.TrimSpace(nj) == "" {
if name, ok := sessions[j].Metadata["name"].(string); ok {
nj = name
}
}
vi = strings.ToLower(ni)
vj = strings.ToLower(nj)
default:
vi = getSessionCreationTimestamp(sessions[i])
vj = getSessionCreationTimestamp(sessions[j])
}
+ if vi == vj {
+ ni, _ := sessions[i].Metadata["name"].(string)
+ nj, _ := sessions[j].Metadata["name"].(string)
+ if ascending {
+ return strings.ToLower(ni) < strings.ToLower(nj)
+ }
+ return strings.ToLower(ni) > strings.ToLower(nj)
+ }
if ascending {
return vi < vj
}
return vi > vj
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| sort.Slice(sessions, func(i, j int) bool { | |
| ts1 := getSessionCreationTimestamp(sessions[i]) | |
| ts2 := getSessionCreationTimestamp(sessions[j]) | |
| // Sort descending (newest first) - RFC3339 timestamps sort lexicographically | |
| return ts1 > ts2 | |
| var vi, vj string | |
| switch sortBy { | |
| case "name": | |
| ni := sessions[i].Spec.DisplayName | |
| if strings.TrimSpace(ni) == "" { | |
| if name, ok := sessions[i].Metadata["name"].(string); ok { | |
| ni = name | |
| } | |
| } | |
| nj := sessions[j].Spec.DisplayName | |
| if strings.TrimSpace(nj) == "" { | |
| if name, ok := sessions[j].Metadata["name"].(string); ok { | |
| nj = name | |
| } | |
| } | |
| vi = strings.ToLower(ni) | |
| vj = strings.ToLower(nj) | |
| default: // "created" or any unrecognized value | |
| vi = getSessionCreationTimestamp(sessions[i]) | |
| vj = getSessionCreationTimestamp(sessions[j]) | |
| } | |
| if ascending { | |
| return vi < vj | |
| } | |
| return vi > vj | |
| }) | |
| sort.Slice(sessions, func(i, j int) bool { | |
| var vi, vj string | |
| switch sortBy { | |
| case "name": | |
| ni := sessions[i].Spec.DisplayName | |
| if strings.TrimSpace(ni) == "" { | |
| if name, ok := sessions[i].Metadata["name"].(string); ok { | |
| ni = name | |
| } | |
| } | |
| nj := sessions[j].Spec.DisplayName | |
| if strings.TrimSpace(nj) == "" { | |
| if name, ok := sessions[j].Metadata["name"].(string); ok { | |
| nj = name | |
| } | |
| } | |
| vi = strings.ToLower(ni) | |
| vj = strings.ToLower(nj) | |
| default: // "created" or any unrecognized value | |
| vi = getSessionCreationTimestamp(sessions[i]) | |
| vj = getSessionCreationTimestamp(sessions[j]) | |
| } | |
| if vi == vj { | |
| ni, _ := sessions[i].Metadata["name"].(string) | |
| nj, _ := sessions[j].Metadata["name"].(string) | |
| if ascending { | |
| return strings.ToLower(ni) < strings.ToLower(nj) | |
| } | |
| return strings.ToLower(ni) > strings.ToLower(nj) | |
| } | |
| if ascending { | |
| return vi < vj | |
| } | |
| return vi > vj | |
| }) |
🤖 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 `@components/backend/handlers/sessions.go` around lines 803 - 829, The
comparator used in sort.Slice (inside sortSessions) lacks a deterministic
tie-breaker so when vi == vj the order is non-deterministic; update the
comparator in the sort.Slice call to, after computing vi and vj (the primary
keys from Spec.DisplayName or getSessionCreationTimestamp), check if vi == vj
and then compare a stable secondary key such as sessions[i].Metadata["name"]
(string) or another unique identifier (e.g., a session ID field) to break ties;
ensure the secondary comparison respects ascending/descending by inverting the
result when ascending is false so pagination becomes deterministic.
|
|
||
| // Reset offset when search changes | ||
| // Current user for "My sessions" filter | ||
| const { data: currentUser, isLoading: isCurrentUserLoading } = useCurrentUser(); |
Contributor
There was a problem hiding this comment.
Guard “My sessions” when currentUser.userId is unavailable
If user loading finishes without a userId, the toggle can still be enabled and shows an active filter while the request is unfiltered (userId stays undefined). Disable (or no-op) the toggle unless currentUser?.userId exists.
As per coding guidelines, "Verify loading/error states and error handling in React Query hooks."
Also applies to: 118-118, 269-278
🤖 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 `@components/frontend/src/components/workspace-sections/sessions-section.tsx`
at line 85, The "My sessions" toggle can be enabled when currentUser.userId is
undefined; update the SessionsSection component to guard the toggle by checking
currentUser?.userId and isCurrentUserLoading: disable (or make the onChange a
no-op) when currentUser?.userId is falsy or while isCurrentUserLoading is true,
and ensure any filter logic that uses currentUser.userId (the code around the
toggle handler and the filtering logic referenced near lines 118 and 269-278)
only applies the userId when it exists (i.e., don't set or send userId undefined
to the request); useCurrentUser and its isLoading flag should determine the
toggle's disabled state and whether the onChange sets the user filter.
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
PaginationParamswithphase,userId,sortBy,sortDirectionquery params. AddedfilterSessionsByPhase(),filterSessionsByUserID(), andsortSessions()functions to theListSessionshandler pipeline (applied after search, before pagination). Supports comma-separated phase values, configurable sort direction, and sorting by name or creation time.useCurrentUser()hook), and sortable "Created" and "Name" column headers with direction indicators. Clicking the active sort column toggles direction; clicking an inactive column switches to it with a sensible default (asc for name, desc for created). Active filters shown as dismissable chips. Pagination offset resets on any filter/sort change.Fixes #1515
Files Changed
components/backend/types/common.goPaginationParamsstruct with 4 new fieldscomponents/backend/handlers/sessions.gocomponents/backend/handlers/sessions_test.gocreateTestSessionWithOptionshelper + 5 test contextscomponents/frontend/src/types/api/common.tsPaginationParamstypecomponents/frontend/src/services/api/sessions.tslistSessionsPaginated()components/frontend/src/components/workspace-sections/sessions-section.tsxcomponents/frontend/src/services/queries/__tests__/use-sessions.test.tsTest plan
?phase=Runningreturns only Running sessions?phase=Running,Pendingreturns sessions in either phase?userId=<id>returns only that user's sessions?sortBy=name&sortDirection=ascsorts alphabetically?sortBy=created&sortDirection=descsorts newest-first (default)go veton backend changesnpx vitest runon frontend tests🤖 Generated with Claude Code
Summary by CodeRabbit