feat(web-ui): /sessions list page + sidebar navigation entry (#508)#519
Conversation
Add TypeScript types for the sessions feature (#508): - SessionState: 'active' | 'ended' | 'paused' - Session: id, state, workspace_path, model, timestamps, cost, agent_name - SessionListResponse: sessions array with total count
Add sessions API client with getAll, create, and end methods following the existing API client pattern (#508).
SessionCard displays session state dot, short ID, workspace name, model, cost, relative time, and action buttons (Resume/View/End). 16 tests covering all display and interaction scenarios.
Dialog modal for creating new sessions with workspace path input, model selector (sonnet/opus/haiku), and loading state handling. 7 tests covering render, defaults, submit, and loading behavior.
Main sessions page content with SWR data fetching, loading skeletons, empty state, error state, search filtering, sorting (active first), and NewSessionModal integration. 7 tests covering all states.
Thin page wrapper following tasks/page.tsx pattern with workspace hydration guard, no-workspace fallback, and SessionListView render.
Add Sessions link between Execution and Blockers in sidebar navigation with CommandLineIcon. Active session count badge (blue) polled every 30s. 3 new tests + updated nav count test (6→8 items).
- handleEnd: wrap in try/catch, surface error banner, always mutate in finally - handleCreate: rename param to avoid shadowing useSWR data var - NewSessionModal: reset form on open to prevent stale workspace path - AppSidebar/SessionListView: encode workspacePath in SWR cache keys - Update AppSidebar test keys to match encodeURIComponent output
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds a Sessions feature: new /sessions pages and detail stub, SessionListView, SessionCard, NewSessionModal, types and sessionsApi, sidebar nav entry with active-session badge, and comprehensive tests for the new UI and sidebar behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Page as Sessions Page
participant List as SessionListView
participant SWR as SWR Layer
participant API as sessionsApi
participant Backend as /api/v2/sessions
User->>Page: Navigate to /sessions
Page->>List: render with workspacePath
List->>SWR: useSWR(getAll workspacePath)
SWR->>API: sessionsApi.getAll(workspacePath)
API->>Backend: GET /api/v2/sessions?workspace_path=...
Backend-->>API: sessions[]
API-->>SWR: SessionListResponse
SWR-->>List: { sessions, isLoading, error }
List-->>User: render session cards (sorted, filtered)
User->>List: Click "+ New Session"
List->>User: Open NewSessionModal
User->>Modal: submit {workspace_path, model}
Modal->>API: sessionsApi.create(data)
API->>Backend: POST /api/v2/sessions
Backend-->>API: Session
API-->>Modal: Session
Modal->>List: close + trigger mutate()
List->>SWR: mutate() -> refetch
SWR->>API: getAll(...)
Backend-->>SWR: Updated sessions[]
SWR-->>List: updated list
List-->>User: show new session
User->>Card: Click "End"
Card->>User: window.confirm()
User-->>Card: confirm true
Card->>API: sessionsApi.end(id)
API->>Backend: DELETE /api/v2/sessions/{id}
Backend-->>API: success
API-->>List: (void)
List->>SWR: mutate()
SWR-->>List: refreshed sessions
List-->>User: updated UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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)
Comment |
Code Review — Sessions List Page + Sidebar Navigation (#508)Good overall implementation. The TDD approach with 112 tests is well-executed, and the component architecture (types → api → card → list → page) follows the existing patterns cleanly. A few things worth addressing: Bug:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-ui/__tests__/components/layout/AppSidebar.test.tsx (1)
53-56:⚠️ Potential issue | 🟡 MinorReset
mockSWRDatainbeforeEachto prevent cross-test leakage.
jest.clearAllMocks()does not clear entries inmockSWRData, so data set in one test can affect later tests.🔧 Suggested fix
beforeEach(() => { jest.clearAllMocks(); mockUsePathname.mockReturnValue('/'); + Object.keys(mockSWRData).forEach((key) => { + delete mockSWRData[key]; + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/__tests__/components/layout/AppSidebar.test.tsx` around lines 53 - 56, The tests leak SWR state because mockSWRData isn't cleared between runs; update the test suite's beforeEach (the existing beforeEach that calls jest.clearAllMocks() and mockUsePathname.mockReturnValue('/')) to also reset or reinitialize mockSWRData (e.g., set it to an empty object or call its reset method) so each test starts with a clean SWR mock state; reference mockSWRData and the beforeEach block when making the change.
🧹 Nitpick comments (2)
web-ui/__tests__/components/sessions/SessionListView.test.tsx (1)
142-153: Exercise the actual retry action, not just button visibility.This test currently verifies the Retry button is rendered but not that clicking it calls
mutate. Adding that assertion would close the loop on retry behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/__tests__/components/sessions/SessionListView.test.tsx` around lines 142 - 153, The test shows the Retry button but doesn't assert its behavior; update the 'shows error state with retry button' test to simulate a user click on the Retry button and assert that the mocked mutate function is called. Keep using the existing mockUseSWR return value (with const mutate = jest.fn()) and SessionListView render, then use userEvent.click or fireEvent.click on screen.getByRole('button', { name: /retry/i }) and expect(mutate).toHaveBeenCalled();.web-ui/src/components/sessions/SessionListView.tsx (1)
158-158: Use ShadcnButtonfor the dismiss action.Line 158 uses a raw
<button>inweb-ui/src, which diverges from the UI component rule used elsewhere in this file.🎯 Suggested change
- <button className="ml-2 underline" onClick={() => setEndError(null)}>Dismiss</button> + <Button + variant="link" + size="sm" + className="ml-2 h-auto p-0" + onClick={() => setEndError(null)} + > + Dismiss + </Button>As per coding guidelines: "
web-ui/src/**/*.{ts,tsx}: Web UI must use Shadcn/UI (Nova preset) with gray color scheme and Hugeicons (@hugeicons/react); never use lucide-react".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/components/sessions/SessionListView.tsx` at line 158, Replace the raw HTML <button> used for the dismiss action in SessionListView.tsx with the Shadcn Button component (e.g., Button from your shadcn/ui bundle) and wire the same onClick handler (setEndError(null)); remove the underline class and instead use the appropriate Button variant/props (e.g., a link/ghost style or gray color scheme) plus the ml-2 spacing so visual layout is preserved, and add the Button import to the top of the file if missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/src/components/layout/AppSidebar.tsx`:
- Around line 126-129: The Sessions badge currently hardcodes a blue token
("bg-blue-500") in the JSX where label === 'Sessions' and activeSessionCount is
rendered; update the className for the badge (the span that renders
{activeSessionCount}) to use the project's Shadcn/UI Nova gray theme tokens
instead of bg-blue-500 (e.g., replace blue token with the corresponding gray
background and matching gray/white text tokens from the Nova preset such as
bg-gray-500/600 and text-gray-900 or text-white as appropriate) so it follows
the gray color scheme used across web-ui components.
In `@web-ui/src/components/sessions/NewSessionModal.tsx`:
- Line 33: Extract the inline payload type used by NewSessionModal's onSubmit
((data: { workspace_path: string; model: string }) => Promise<void>) into a
shared exported interface named SessionCreateRequest in
web-ui/src/types/index.ts, export it, then import and use that type in
NewSessionModal (replace the inline shape on onSubmit) and in SessionListView so
both components consume the same SessionCreateRequest type; update any affected
function signatures (e.g., onSubmit) and imports accordingly.
In `@web-ui/src/components/sessions/SessionCard.tsx`:
- Around line 51-55: The current nesting of Link > Button creates an interactive
element inside another; change to Button asChild wrapping Link so Button renders
the Link element instead. In SessionCard, replace the Link wrapping Button with
Button having the prop asChild and put <Link
href={`/sessions/${session.id}`}>...</Link> as its child, preserving Button
props (size="sm" variant="ghost" className="h-7 gap-1 px-2 text-xs") and the
label that uses isActive ? 'Resume' : 'View' + " →".
In `@web-ui/src/components/sessions/SessionListView.tsx`:
- Around line 50-58: handleEnd currently sets endError in the catch but never
clears it on success; update the handleEnd function so that after a successful
await sessionsApi.end(id) you call setEndError('') (or null) to clear the stale
error before calling mutate(), and optionally capture the caught error in catch
(e) to log or ignore; reference: handleEnd, setEndError, sessionsApi.end,
mutate.
- Line 64: Add the missing dynamic session detail route for the /sessions/:id
path by creating a route component that accepts the route parameter id (e.g.,
export default async function SessionPage({ params }) { ... }) and fetches the
session by params.id, rendering the session details (or a not-found state) so
router.push(`/sessions/${session.id}`) resolves; ensure the component handles
create/resume/view flows, returns appropriate JSX/ReactNode, and exports
metadata or error handling as needed to match existing sessions list behavior.
In `@web-ui/src/lib/api.ts`:
- Around line 634-645: sessionsApi.getAll currently assumes a { sessions, total
} shape but the backend returns a list response; update getAll and its types so
the returned value matches the backend contract: adjust the SessionListResponse
type to the actual list response shape returned by GET /api/v2/sessions (e.g.,
an array or ListResponse<T> as used elsewhere) and return response.data exactly
(or map response.data.results to the expected shape) from getAll instead of
fabricating { sessions, total }; update any callers expecting
sessionData.sessions to use the corrected property name (e.g., the array itself
or .results) so runtime undefined errors are resolved.
---
Outside diff comments:
In `@web-ui/__tests__/components/layout/AppSidebar.test.tsx`:
- Around line 53-56: The tests leak SWR state because mockSWRData isn't cleared
between runs; update the test suite's beforeEach (the existing beforeEach that
calls jest.clearAllMocks() and mockUsePathname.mockReturnValue('/')) to also
reset or reinitialize mockSWRData (e.g., set it to an empty object or call its
reset method) so each test starts with a clean SWR mock state; reference
mockSWRData and the beforeEach block when making the change.
---
Nitpick comments:
In `@web-ui/__tests__/components/sessions/SessionListView.test.tsx`:
- Around line 142-153: The test shows the Retry button but doesn't assert its
behavior; update the 'shows error state with retry button' test to simulate a
user click on the Retry button and assert that the mocked mutate function is
called. Keep using the existing mockUseSWR return value (with const mutate =
jest.fn()) and SessionListView render, then use userEvent.click or
fireEvent.click on screen.getByRole('button', { name: /retry/i }) and
expect(mutate).toHaveBeenCalled();.
In `@web-ui/src/components/sessions/SessionListView.tsx`:
- Line 158: Replace the raw HTML <button> used for the dismiss action in
SessionListView.tsx with the Shadcn Button component (e.g., Button from your
shadcn/ui bundle) and wire the same onClick handler (setEndError(null)); remove
the underline class and instead use the appropriate Button variant/props (e.g.,
a link/ghost style or gray color scheme) plus the ml-2 spacing so visual layout
is preserved, and add the Button import to the top of the file if missing.
🪄 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: e21ba614-a186-45d5-8580-27d3cc930a47
📒 Files selected for processing (11)
web-ui/__tests__/components/layout/AppSidebar.test.tsxweb-ui/__tests__/components/sessions/NewSessionModal.test.tsxweb-ui/__tests__/components/sessions/SessionCard.test.tsxweb-ui/__tests__/components/sessions/SessionListView.test.tsxweb-ui/src/app/sessions/page.tsxweb-ui/src/components/layout/AppSidebar.tsxweb-ui/src/components/sessions/NewSessionModal.tsxweb-ui/src/components/sessions/SessionCard.tsxweb-ui/src/components/sessions/SessionListView.tsxweb-ui/src/lib/api.tsweb-ui/src/types/index.ts
| {label === 'Sessions' && activeSessionCount > 0 && ( | ||
| <span className="ml-auto flex h-5 min-w-5 items-center justify-center rounded-full bg-blue-500 px-1.5 text-[10px] font-bold text-white"> | ||
| {activeSessionCount} | ||
| </span> |
There was a problem hiding this comment.
Use gray theme tokens for the Sessions badge instead of hardcoded blue.
Line 127 introduces bg-blue-500, which diverges from the UI gray color scheme requirement in this codebase.
🎨 Suggested fix
- <span className="ml-auto flex h-5 min-w-5 items-center justify-center rounded-full bg-blue-500 px-1.5 text-[10px] font-bold text-white">
+ <span className="ml-auto flex h-5 min-w-5 items-center justify-center rounded-full bg-muted px-1.5 text-[10px] font-bold text-foreground">
{activeSessionCount}
</span>As per coding guidelines web-ui/src/**/*.{ts,tsx}: Web UI must use Shadcn/UI (Nova preset) with gray color scheme and Hugeicons (@hugeicons/react); never use lucide-react.
📝 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.
| {label === 'Sessions' && activeSessionCount > 0 && ( | |
| <span className="ml-auto flex h-5 min-w-5 items-center justify-center rounded-full bg-blue-500 px-1.5 text-[10px] font-bold text-white"> | |
| {activeSessionCount} | |
| </span> | |
| {label === 'Sessions' && activeSessionCount > 0 && ( | |
| <span className="ml-auto flex h-5 min-w-5 items-center justify-center rounded-full bg-muted px-1.5 text-[10px] font-bold text-foreground"> | |
| {activeSessionCount} | |
| </span> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/components/layout/AppSidebar.tsx` around lines 126 - 129, The
Sessions badge currently hardcodes a blue token ("bg-blue-500") in the JSX where
label === 'Sessions' and activeSessionCount is rendered; update the className
for the badge (the span that renders {activeSessionCount}) to use the project's
Shadcn/UI Nova gray theme tokens instead of bg-blue-500 (e.g., replace blue
token with the corresponding gray background and matching gray/white text tokens
from the Nova preset such as bg-gray-500/600 and text-gray-900 or text-white as
appropriate) so it follows the gray color scheme used across web-ui components.
| open: boolean; | ||
| onOpenChange: (open: boolean) => void; | ||
| defaultWorkspacePath: string; | ||
| onSubmit: (data: { workspace_path: string; model: string }) => Promise<void>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract session-create payload type to shared types.
{ workspace_path: string; model: string } should be a shared exported type from web-ui/src/types/index.ts and imported here (and in SessionListView) to satisfy repo typing rules and avoid drift.
💡 Suggested refactor
- onSubmit: (data: { workspace_path: string; model: string }) => Promise<void>;
+ onSubmit: (data: SessionCreateRequest) => Promise<void>;// web-ui/src/types/index.ts
export interface SessionCreateRequest {
workspace_path: string;
model: string;
}+ import type { SessionCreateRequest } from '@/types';As per coding guidelines: "TypeScript types must be defined in web-ui/src/types/index.ts and API client functionality in web-ui/src/lib/api.ts".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/components/sessions/NewSessionModal.tsx` at line 33, Extract the
inline payload type used by NewSessionModal's onSubmit ((data: { workspace_path:
string; model: string }) => Promise<void>) into a shared exported interface
named SessionCreateRequest in web-ui/src/types/index.ts, export it, then import
and use that type in NewSessionModal (replace the inline shape on onSubmit) and
in SessionListView so both components consume the same SessionCreateRequest
type; update any affected function signatures (e.g., onSubmit) and imports
accordingly.
| const session = await sessionsApi.create(createData); | ||
| setModalOpen(false); | ||
| mutate(); | ||
| router.push(`/sessions/${session.id}`); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether a dynamic sessions detail route exists.
fd -a -p 'page.tsx' web-ui/src/app/sessions | sed 's#^\./##'Repository: frankbria/codeframe
Length of output: 114
Add the missing dynamic session detail route.
Line 64 calls router.push(\/sessions/${session.id}`), but the dynamic page web-ui/src/app/sessions/[id]/page.tsxdoes not exist. Only the static/sessions` page is present. This breaks all navigation to session details (create, resume, view). Create the missing dynamic route file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/components/sessions/SessionListView.tsx` at line 64, Add the
missing dynamic session detail route for the /sessions/:id path by creating a
route component that accepts the route parameter id (e.g., export default async
function SessionPage({ params }) { ... }) and fetches the session by params.id,
rendering the session details (or a not-found state) so
router.push(`/sessions/${session.id}`) resolves; ensure the component handles
create/resume/view flows, returns appropriate JSX/ReactNode, and exports
metadata or error handling as needed to match existing sessions list behavior.
| getAll: async ( | ||
| workspacePath: string, | ||
| params?: { state?: SessionState } | ||
| ): Promise<SessionListResponse> => { | ||
| const response = await api.get<SessionListResponse>('/api/v2/sessions', { | ||
| params: { | ||
| workspace_path: workspacePath, | ||
| ...(params?.state ? { state: params.state } : {}), | ||
| }, | ||
| }); | ||
| return response.data; | ||
| }, |
There was a problem hiding this comment.
sessionsApi.getAll response type does not match backend contract.
Line 638 currently assumes { sessions, total }, but backend GET /api/v2/sessions is defined as a list response. This will cause sessionData.sessions to be undefined at runtime.
🔧 Suggested fix
- ): Promise<SessionListResponse> => {
- const response = await api.get<SessionListResponse>('/api/v2/sessions', {
+ ): Promise<SessionListResponse> => {
+ const response = await api.get<Session[]>('/api/v2/sessions', {
params: {
workspace_path: workspacePath,
...(params?.state ? { state: params.state } : {}),
},
});
- return response.data;
+ return {
+ sessions: response.data,
+ total: response.data.length,
+ };
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/lib/api.ts` around lines 634 - 645, sessionsApi.getAll currently
assumes a { sessions, total } shape but the backend returns a list response;
update getAll and its types so the returned value matches the backend contract:
adjust the SessionListResponse type to the actual list response shape returned
by GET /api/v2/sessions (e.g., an array or ListResponse<T> as used elsewhere)
and return response.data exactly (or map response.data.results to the expected
shape) from getAll instead of fabricating { sessions, total }; update any
callers expecting sessionData.sessions to use the corrected property name (e.g.,
the array itself or .results) so runtime undefined errors are resolved.
- Add SessionCreateRequest type to src/types/index.ts
- Fix sessionsApi.getAll: backend returns Session[] array, wrap as { sessions, total }
- SessionCard: Button asChild > Link (fix interactive element nesting a11y)
- SessionCard: format cost_usd with .toFixed(4) to avoid float representation issues
- NewSessionModal: use SessionCreateRequest type; reset isSubmitting on open
- SessionListView: clear endError at start of handleEnd before retry
- AppSidebar: Sessions badge uses bg-muted/text-foreground (gray theme, not blue)
- AppSidebar: align SWR cache key with actual API endpoint
- Create sessions/[id]/page.tsx (missing dynamic route — broke all detail navigation)
- Fix AppSidebar tests: reset mockSWRData in beforeEach, update SWR key + badge class
PR Review — Sessions List Page + Sidebar Navigation (#508)This is a solid, well-structured piece of UI work. TDD approach with 112 tests, clear component decomposition, proper SWR patterns, and good handling of the hydration guard — the fundamentals are right. A few issues worth addressing before merging: Issues1.
const handleSubmit = async () => {
setIsSubmitting(true);
try {
await onSubmit({ workspace_path: workspacePath, model });Add a guard: 2.
const handleCreate = useCallback(async (createData: SessionCreateRequest) => {
const session = await sessionsApi.create(createData); // ← unhandled rejection
setModalOpen(false);
mutate();
router.push(`/sessions/${session.id}`);
}, [mutate, router]);Wrap in try/catch and surface the error the same way 3. {label === 'Sessions' && activeSessionCount > 0 && (
{label === 'Blockers' && openBlockerCount > 0 && (This will silently break if the label text ever changes. Consider adding an optional const navItemsWithBadges = NAV_ITEMS.map((item) => ({
...item,
badge: item.href === '/sessions' ? activeSessionCount
: item.href === '/blockers' ? openBlockerCount
: 0,
}));Then the render loop becomes 4. The detail page is clearly a stub, but
If there's a tracking issue for the detail page, linking it here would help. 5. SWR cache key vs actual API parameter naming The SWR key in But the actual API call in params: { workspace_path: workspacePath, ... }The SWR key is just an identifier so this isn't a bug, but it creates a confusing mismatch when debugging (e.g., comparing DevTools network requests vs SWR cache). Consider aligning the key to match the actual query string. Minor
What's working well
Items 1 and 2 are the most user-visible gaps. Item 3 is a maintainability improvement worth making while the pattern is fresh. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web-ui/src/components/sessions/SessionListView.tsx (1)
26-27: Align the SWR key with the actual API query param (workspace_path).At Line 26, using
pathin the key while the request usesworkspace_pathcan create cache-key drift and make shared invalidation harder across components.Suggested diff
- `/api/v2/sessions?path=${encodeURIComponent(workspacePath)}`, + `/api/v2/sessions?workspace_path=${encodeURIComponent(workspacePath)}`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/components/sessions/SessionListView.tsx` around lines 26 - 27, The SWR key string in SessionListView.tsx uses `path` while the actual request and backend expect `workspace_path`, causing cache-key drift; update the key used with sessionsApi.getAll to `/api/v2/sessions?workspace_path=${encodeURIComponent(workspacePath)}` (or switch to a tuple key like ['/api/v2/sessions', workspacePath]) so it matches the API query param and consistent invalidation across components; check usages around sessionsApi.getAll and other components to ensure they use the same `workspace_path` key format.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-ui/src/components/sessions/SessionListView.tsx`:
- Around line 157-160: Replace the raw HTML <button> in SessionListView that
dismisses endError with the Shadcn Button component used across the app: import
the Button from the shared UI components and change the element that calls
setEndError(null) to use <Button ... onClick={() =>
setEndError(null)}>Dismiss</Button> (choose the appropriate variant/size to
match the current styling, e.g., variant="link" or size="sm"); ensure the Button
import is added to the top of the SessionListView file and preserve
accessibility (aria-label) if needed and any existing className styling.
---
Nitpick comments:
In `@web-ui/src/components/sessions/SessionListView.tsx`:
- Around line 26-27: The SWR key string in SessionListView.tsx uses `path` while
the actual request and backend expect `workspace_path`, causing cache-key drift;
update the key used with sessionsApi.getAll to
`/api/v2/sessions?workspace_path=${encodeURIComponent(workspacePath)}` (or
switch to a tuple key like ['/api/v2/sessions', workspacePath]) so it matches
the API query param and consistent invalidation across components; check usages
around sessionsApi.getAll and other components to ensure they use the same
`workspace_path` key format.
🪄 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: 7765890c-4a44-4fe7-bff7-406e0325806a
📒 Files selected for processing (9)
web-ui/__tests__/components/layout/AppSidebar.test.tsxweb-ui/__tests__/components/sessions/SessionCard.test.tsxweb-ui/src/app/sessions/[id]/page.tsxweb-ui/src/components/layout/AppSidebar.tsxweb-ui/src/components/sessions/NewSessionModal.tsxweb-ui/src/components/sessions/SessionCard.tsxweb-ui/src/components/sessions/SessionListView.tsxweb-ui/src/lib/api.tsweb-ui/src/types/index.ts
✅ Files skipped from review due to trivial changes (2)
- web-ui/src/components/sessions/SessionCard.tsx
- web-ui/tests/components/sessions/SessionCard.test.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- web-ui/src/components/layout/AppSidebar.tsx
- web-ui/src/lib/api.ts
- web-ui/src/components/sessions/NewSessionModal.tsx
- web-ui/tests/components/layout/AppSidebar.test.tsx
- web-ui/src/types/index.ts
| <div className="rounded-lg border border-destructive/50 bg-destructive/10 px-4 py-2 text-sm text-destructive"> | ||
| {endError} | ||
| <button className="ml-2 underline" onClick={() => setEndError(null)}>Dismiss</button> | ||
| </div> |
There was a problem hiding this comment.
Use Shadcn Button instead of raw <button> for dismiss action.
At Line 159, this bypasses the shared UI system used elsewhere in the file.
Suggested diff
- <button className="ml-2 underline" onClick={() => setEndError(null)}>Dismiss</button>
+ <Button
+ variant="link"
+ size="sm"
+ className="ml-2 h-auto p-0 text-destructive underline"
+ onClick={() => setEndError(null)}
+ >
+ Dismiss
+ </Button>As per coding guidelines, web-ui/src/**/*.{ts,tsx} must use Shadcn/UI (Nova preset) and Hugeicons in Web UI components.
📝 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.
| <div className="rounded-lg border border-destructive/50 bg-destructive/10 px-4 py-2 text-sm text-destructive"> | |
| {endError} | |
| <button className="ml-2 underline" onClick={() => setEndError(null)}>Dismiss</button> | |
| </div> | |
| <div className="rounded-lg border border-destructive/50 bg-destructive/10 px-4 py-2 text-sm text-destructive"> | |
| {endError} | |
| <Button | |
| variant="link" | |
| size="sm" | |
| className="ml-2 h-auto p-0 text-destructive underline" | |
| onClick={() => setEndError(null)} | |
| > | |
| Dismiss | |
| </Button> | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/src/components/sessions/SessionListView.tsx` around lines 157 - 160,
Replace the raw HTML <button> in SessionListView that dismisses endError with
the Shadcn Button component used across the app: import the Button from the
shared UI components and change the element that calls setEndError(null) to use
<Button ... onClick={() => setEndError(null)}>Dismiss</Button> (choose the
appropriate variant/size to match the current styling, e.g., variant="link" or
size="sm"); ensure the Button import is added to the top of the SessionListView
file and preserve accessibility (aria-label) if needed and any existing
className styling.
Code Review: Sessions List Page + Sidebar Navigation (#508)Good overall implementation — the component structure is clean, the test coverage is thorough, and the workspace hydration guard pattern is followed correctly. A few things worth addressing: Issues1. Wrong icon for empty state ( <Loading03Icon className="mx-auto mb-3 h-10 w-10 text-muted-foreground" />
<p className="text-sm font-medium text-foreground">No sessions yet</p>
2. Fragile label-string badge dispatch in {label === 'Sessions' && activeSessionCount > 0 && (
{label === 'Blockers' && openBlockerCount > 0 && (This ties badge rendering to the human-readable label string. If either label is renamed, the badge silently disappears with no compile-time error. The interface NavItem {
href: string;
label: string;
id?: 'sessions' | 'blockers'; // or badge data
...
}3. Silent failure on session creation error (
Consider adding an 4. if (window.confirm('End this session? This will stop the active agent.')) {
5. const response = await api.get<Session[]>('/api/v2/sessions', { ... });
return { sessions: response.data, total: response.data.length };This assumes the backend returns a flat Minor
Tests112 tests is solid coverage. The SWR mock reset in The |
1 participant
Summary
Implements #508 — adds the
/sessionspage and sidebar navigation entry so users can discover and manage interactive agent sessions.Session,SessionState,SessionListResponseadded tosrc/types/index.tsgetAll,create,endmethods added tosrc/lib/api.ts/sessionspage: workspace hydration guard following existing page patternAcceptance Criteria
/sessionspage lists sessions from API, sorted active first/sessions/[id]/sessionsTest Plan
Implementation Notes
handleEndshows an inline error banner on failure and always re-syncs SWRNewSessionModalresets workspace path and model on each open (prevents stale state across workspace switches)encodeURIComponent(workspacePath)to handle paths with special characters correctlyCloses #508
Summary by CodeRabbit
New Features
Tests
Documentation