Repository navigation
feat(webui): redesign agent workspace - #684
Conversation
Add the right-side operational rail, checkpoint and result views, memory review controls, expanded timeline events, and responsive workspace layout. Keep asynchronous session updates scoped to their originating session and cover the new behavior with focused UI tests.
Poll the result endpoint for a bounded window after prompt streams close so terminal details persisted asynchronously are not missed. Cover the eventual-consistency window with an App regression test and verify it through the local production WebUI path.
📝 WalkthroughWalkthroughThe WebUI adds responsive session navigation, right-rail panels, session results, runtime settings, checkpoint management, memory review actions, approval scopes, timeline enhancements, and guards against stale asynchronous updates. ChangesWebUI session workspace
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant AgentClient
participant RightRail
User->>App: select a session
App->>AgentClient: load session history and result
AgentClient-->>App: return session data
App->>RightRail: show the selected panel
User->>RightRail: open diff, memory, checkpoints, or settings
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
coding-agent/webui/app/src/components/Timeline.tsx (1)
277-315: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClient clock skew can hide the approval controls while the request is still pending.
timedOutderives only from the browser clock against a deadline computed frompromptedAt. The server owns the real auto-deny. If the browser clock runs ahead of the server clock, or ifpromptedAtis skewed, the card renders "timed out (auto-denied)" and removes the Approve, Always allow, and Deny buttons while the server still waits for a decision. The user then cannot respond, and the request stalls until the server timeout.The authoritative signal is the
approval_resultevent, which already setsitem.resolved. Consider keeping the controls available after the local countdown reaches zero, and showing the timed-out label only whenitem.resolvedarrives. A grace period is a lighter alternative.🛡️ Proposed change: keep controls until the server resolves
- const timedOut = remaining !== null && remaining <= 0; - const done = !!item.resolved || timedOut; + const timedOut = remaining !== null && remaining <= 0; + // The server owns the auto-deny; only `resolved` closes the card so clock + // skew cannot strand a still-pending request. + const done = !!item.resolved;Then render the expired hint next to the still-active controls:
) : ( <div className="mt-3 flex flex-wrap items-center gap-2"> + {timedOut && ( + <span className="text-xs text-muted"> + deadline passed · awaiting server decision + </span> + )} <buttonNote that
Timeline.test.tsxlines 172-209 assert the current behavior. Update those tests with whichever option you choose.🤖 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 `@coding-agent/webui/app/src/components/Timeline.tsx` around lines 277 - 315, Update the approval-card logic around timedOut, done, and the conditional controls so the local deadline never marks a pending request as resolved or hides Approve, Always allow, or Deny actions. Treat item.resolved, populated by the authoritative approval_result event, as the only completion signal; retain the countdown/expired hint as appropriate while controls remain available until that signal arrives. Update the affected Timeline.test.tsx assertions to match the new pending-after-local-expiry behavior.
🧹 Nitpick comments (10)
coding-agent/webui/app/src/components/Timeline.tsx (1)
192-205: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExpose the tool status to assistive technology.
The status indicator uses only color, shape, and a
titleattribute. Screen readers do not reliably announcetitleon aspan. Add visually hidden text or anaria-labelso the running, failed, and done states are available without vision.♿ Proposed change
{running ? ( <span className="h-2 w-2 shrink-0 animate-pulse rounded-full bg-accent" - title="running" + title="running" + role="img" + aria-label="running" /> ) : item.isError ? ( - <span className="shrink-0 text-xs text-err" title="failed"> + <span className="shrink-0 text-xs text-err" role="img" aria-label="failed" title="failed"> ✗ </span> ) : ( - <span className="shrink-0 text-xs text-ok" title="done"> + <span className="shrink-0 text-xs text-ok" role="img" aria-label="done" title="done"> ✓ </span> )}🤖 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 `@coding-agent/webui/app/src/components/Timeline.tsx` around lines 192 - 205, Add accessible status text to the three indicator spans in the Timeline component’s running/item.isError conditional, ensuring running, failed, and done states are announced to assistive technology without relying on title, while preserving the existing visual indicators.coding-agent/webui/app/src/components/RightRail.tsx (1)
16-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Escape-to-close for the mobile drawer.
Below
mdthe panel is an overlay drawer with a dimming backdrop. The backdrop closes the panel on click only. Keyboard and screen reader users have no equivalent dismissal, and the backdrop still allows Tab into the obscured main content. Add an Escape key handler so keyboard users can dismiss the drawer.♿ Proposed change
-import type { ReactNode } from "react"; +import { useEffect, type ReactNode } from "react";export default function RightRail({ panel, onToggle, children }: Props) { + useEffect(() => { + if (!panel) return; + const onKeyDown = (e: KeyboardEvent) => { + if (e.key === "Escape") onToggle(panel); + }; + document.addEventListener("keydown", onKeyDown); + return () => document.removeEventListener("keydown", onKeyDown); + }, [panel, onToggle]); + return (🤖 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 `@coding-agent/webui/app/src/components/RightRail.tsx` around lines 16 - 30, Update the mobile drawer rendering in RightRail to handle Escape key presses by calling onToggle(panel), while preserving the existing backdrop click behavior. Limit the keyboard dismissal behavior to the open mobile panel and ensure the handler is attached to the drawer or its overlay without affecting desktop rendering.coding-agent/webui/app/src/components/CheckpointsPanel.test.tsx (1)
77-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for the empty label.
This test always types a label.
CheckpointsPanelpasses the rawlabelstate, andAgentClient.captureCheckpointomits a blank label so the body becomes{}. That branch is the default flow when the user creates a checkpoint without typing a label. Assert that the captured body is{}in that case.🤖 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 `@coding-agent/webui/app/src/components/CheckpointsPanel.test.tsx` around lines 77 - 108, Add a separate test case near the existing labeled checkpoint test that creates a checkpoint without changing the “checkpoint label” input. Assert that the capture callback receives an empty body object ({}) while preserving the list refresh and capture callback expectations for the unlabeled flow.coding-agent/webui/app/src/components/ReviewQueue.tsx (1)
131-185: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComplete the ARIA tab pattern.
The buttons declare
role="tab"inside arole="tablist", but no element declaresrole="tabpanel". Screen readers then announce tabs that control nothing. Wrap the records list in atabpaneland link it witharia-controlsandid.♿ Proposed change
{TABS.map((t) => ( <button key={t.id} role="tab" + id={`review-tab-${t.id}`} + aria-controls="review-tabpanel" aria-selected={tab === t.id}- {loading ? ( + <div id="review-tabpanel" role="tabpanel" aria-labelledby={`review-tab-${tab}`}> + {loading ? ( <div className="px-4 py-3 text-sm text-muted">Loading reviews…</div>Close the new wrapper after the records list at line 185.
🤖 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 `@coding-agent/webui/app/src/components/ReviewQueue.tsx` around lines 131 - 185, Complete the ARIA tab pattern in the TABS rendering and records content: add a stable id to the records container, set each tab button’s aria-controls to that id, and wrap the loading/empty/records conditional in a role="tabpanel" element referencing the active tab with aria-labelledby. Keep the existing content and behavior unchanged.coding-agent/webui/app/src/components/ReviewQueue.test.tsx (1)
63-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd coverage for the failure branches.
The suite exercises only successful responses.
ReviewQueuerenders two distinct error messages:reviews load failed:fromloadandreview update failed:fromtransition. Add a test that returns a non-2xx response for the list request and one for the transition request, then assert the rendered error text and that the rows stay actionable.🤖 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 `@coding-agent/webui/app/src/components/ReviewQueue.test.tsx` around lines 63 - 96, Add failure-branch tests for ReviewQueue: cover a non-2xx list response from the load flow and assert the rendered “reviews load failed:” message, then cover a non-2xx transition response and assert “review update failed:” while the candidate row and its action button remain available. Reuse the existing stubFetch, record, and client setup patterns, and verify each failure preserves an actionable row.coding-agent/webui/app/src/components/Timeline.test.tsx (1)
27-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStub
scrollIntoViewwithvi.spyOnso cleanup restores the prototype.Directly assigning
Element.prototype.scrollIntoViewcreates a non-spied prototype property, sovi.restoreAllMocks()does not revert it. Add a native-like no-op first if needed, then spy on it so the per-test stub is removed inafterEach.🤖 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 `@coding-agent/webui/app/src/components/Timeline.test.tsx` around lines 27 - 35, Update the Timeline test setup around beforeEach so Element.prototype.scrollIntoView is initialized with a native-like no-op when unavailable, then stub it via vi.spyOn instead of direct assignment. Keep afterEach’s vi.restoreAllMocks cleanup so the prototype is restored between tests.coding-agent/webui/app/src/components/Header.tsx (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
PROVIDERSto a shared module.
SettingsPanel.tsximportsPROVIDERSfrom./Header(line 4). This makes a panel component depend on a sibling presentation component only for a data constant. Move the list to../lib/typesor a newlib/providers.ts, then import it in both components. This keeps the dependency direction pointing at the library layer.🤖 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 `@coding-agent/webui/app/src/components/Header.tsx` at line 15, Move the PROVIDERS constant out of Header into a shared library module such as lib/types or lib/providers.ts, then update both Header and SettingsPanel imports to use that module. Remove the presentation-layer export and preserve the existing provider list unchanged.coding-agent/webui/app/src/App.tsx (1)
152-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the
localStoragewrite out of the state updater.React state updater functions must be pure. This updater writes to
localStorageas a side effect. React can call an updater more than once (for example underStrictModein development), so the write can repeat. The file already has the correct pattern forthemeat lines 139-146: persist in auseEffect.♻️ Proposed refactor
+ useEffect(() => { + try { + localStorage.setItem(THINKING_LS_KEY, showThinking ? "1" : "0"); + } catch { + /* ignore */ + } + }, [showThinking]); + const toggleThinking = useCallback(() => { - setShowThinking((v) => { - try { - localStorage.setItem(THINKING_LS_KEY, v ? "0" : "1"); - } catch { - /* ignore */ - } - return !v; - }); + setShowThinking((v) => !v); }, []);Note: the effect also writes on mount. If you must avoid the initial write, gate it with a ref.
🤖 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 `@coding-agent/webui/app/src/App.tsx` around lines 152 - 161, Update toggleThinking in App.tsx so its state updater only returns the toggled value and performs no localStorage writes. Add a useEffect tied to showThinking that persists the current value using THINKING_LS_KEY, following the existing theme persistence pattern.coding-agent/webui/app/src/App.test.tsx (2)
1243-1273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
/resultbranch to the fetch stub.
loadSessioncallsrefreshResult, which issuesGET /sessions/{id}/result.stubRailFetchhas no branch for that path, so the stub reachesthrow new Error(\unexpected fetch ${url}`)on every session selection. The tests still pass only becauserefreshResult` swallows the rejection. This hides a real request from the stub and makes future failures hard to read. Add an explicit branch that returns an empty result.♻️ Proposed addition
if (url.includes(`/sessions/${active.session_id}/memory/reviews`)) { return Promise.resolve(jsonResponse([])); } + if (url.endsWith(`/sessions/${active.session_id}/result`)) { + return Promise.resolve( + jsonResponse({ + session_id: active.session_id, + status: "completed", + turn_status: "idle", + final_answer: null, + verification_summary: null, + failure_details: null, + }), + ); + }🤖 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 `@coding-agent/webui/app/src/App.test.tsx` around lines 1243 - 1273, Add an explicit `/sessions/${active.session_id}/result` branch in `stubRailFetch`, returning an empty result response before the unexpected-URL error. Keep the existing session, runs, memory, diff, and patch branches unchanged.
1915-1930: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the bare
setTimeout(0)flush withawait act(async () => {}).Lines 1927, 2005, and 2257 flush pending work with
await new Promise((resolve) => setTimeout(resolve, 0));. Two problems follow. First, the state updates that resolve afterwards run outsideact(), so React can emit act warnings and the assertion can read a stale DOM. Second, each of these flushes is followed by a negative assertion (queryByText(...)is null). If one macrotask tick does not drain the promise chain, the assertion passes for the wrong reason and stops guarding the race.await act(async () => {})drains the microtask queue inside React's batching boundary.♻️ Proposed change (apply at lines 1927, 2005, and 2257)
- await new Promise((resolve) => setTimeout(resolve, 0)); + await act(async () => undefined); expect(screen.getByText("b-file.ts")).toBeTruthy(); expect(screen.queryByText("a-file.ts")).toBeNull();Import
actfrom@testing-library/react, asSettingsPanel.test.tsxalready does.🤖 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 `@coding-agent/webui/app/src/App.test.tsx` around lines 1915 - 1930, In the affected race-condition tests, replace each setTimeout(0) promise flush with an empty await act(async () => {}) call so pending promise-driven state updates are drained inside React’s act boundary before assertions. Add the act import from `@testing-library/react`, and update all three flush sites near the stale-fetch assertions, preserving the existing assertions and test flow.
🤖 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 `@coding-agent/webui/app/src/components/MemoryPanel.tsx`:
- Around line 15-19: Update the prop comment above client, sessionId, and
onReviewsChanged to state that the interactive review queue is added alongside
the static accepted-memories list, not that it replaces it; preserve the
existing rendering behavior.
In `@coding-agent/webui/app/src/components/ReviewQueue.tsx`:
- Around line 63-79: Reset ReviewQueue’s initializedRef and clear its loaded
review records whenever sessionId changes, so the first-load effect runs for the
new session and stale records are not displayed. Update the session-aware logic
around the ReviewQueue effect and state, or alternatively key the ReviewQueue
child in MemoryPanel by sessionId to force a remount; preserve the existing
loading behavior for each session.
In `@coding-agent/webui/app/src/components/SettingsPanel.tsx`:
- Around line 26-27: In SettingsPanel, add an effect that updates both provider
and model state from providerName and modelName whenever those props change,
preserving empty-string fallbacks for undefined values and keeping the existing
local state initialization.
In `@coding-agent/webui/app/src/index.css`:
- Line 16: Update the --font-mono declaration to quote the SFMono-Regular and
Menlo font-family names, while leaving the generic ui-monospace and monospace
keywords unquoted.
---
Outside diff comments:
In `@coding-agent/webui/app/src/components/Timeline.tsx`:
- Around line 277-315: Update the approval-card logic around timedOut, done, and
the conditional controls so the local deadline never marks a pending request as
resolved or hides Approve, Always allow, or Deny actions. Treat item.resolved,
populated by the authoritative approval_result event, as the only completion
signal; retain the countdown/expired hint as appropriate while controls remain
available until that signal arrives. Update the affected Timeline.test.tsx
assertions to match the new pending-after-local-expiry behavior.
---
Nitpick comments:
In `@coding-agent/webui/app/src/App.test.tsx`:
- Around line 1243-1273: Add an explicit `/sessions/${active.session_id}/result`
branch in `stubRailFetch`, returning an empty result response before the
unexpected-URL error. Keep the existing session, runs, memory, diff, and patch
branches unchanged.
- Around line 1915-1930: In the affected race-condition tests, replace each
setTimeout(0) promise flush with an empty await act(async () => {}) call so
pending promise-driven state updates are drained inside React’s act boundary
before assertions. Add the act import from `@testing-library/react`, and update
all three flush sites near the stale-fetch assertions, preserving the existing
assertions and test flow.
In `@coding-agent/webui/app/src/App.tsx`:
- Around line 152-161: Update toggleThinking in App.tsx so its state updater
only returns the toggled value and performs no localStorage writes. Add a
useEffect tied to showThinking that persists the current value using
THINKING_LS_KEY, following the existing theme persistence pattern.
In `@coding-agent/webui/app/src/components/CheckpointsPanel.test.tsx`:
- Around line 77-108: Add a separate test case near the existing labeled
checkpoint test that creates a checkpoint without changing the “checkpoint
label” input. Assert that the capture callback receives an empty body object
({}) while preserving the list refresh and capture callback expectations for the
unlabeled flow.
In `@coding-agent/webui/app/src/components/Header.tsx`:
- Line 15: Move the PROVIDERS constant out of Header into a shared library
module such as lib/types or lib/providers.ts, then update both Header and
SettingsPanel imports to use that module. Remove the presentation-layer export
and preserve the existing provider list unchanged.
In `@coding-agent/webui/app/src/components/ReviewQueue.test.tsx`:
- Around line 63-96: Add failure-branch tests for ReviewQueue: cover a non-2xx
list response from the load flow and assert the rendered “reviews load failed:”
message, then cover a non-2xx transition response and assert “review update
failed:” while the candidate row and its action button remain available. Reuse
the existing stubFetch, record, and client setup patterns, and verify each
failure preserves an actionable row.
In `@coding-agent/webui/app/src/components/ReviewQueue.tsx`:
- Around line 131-185: Complete the ARIA tab pattern in the TABS rendering and
records content: add a stable id to the records container, set each tab button’s
aria-controls to that id, and wrap the loading/empty/records conditional in a
role="tabpanel" element referencing the active tab with aria-labelledby. Keep
the existing content and behavior unchanged.
In `@coding-agent/webui/app/src/components/RightRail.tsx`:
- Around line 16-30: Update the mobile drawer rendering in RightRail to handle
Escape key presses by calling onToggle(panel), while preserving the existing
backdrop click behavior. Limit the keyboard dismissal behavior to the open
mobile panel and ensure the handler is attached to the drawer or its overlay
without affecting desktop rendering.
In `@coding-agent/webui/app/src/components/Timeline.test.tsx`:
- Around line 27-35: Update the Timeline test setup around beforeEach so
Element.prototype.scrollIntoView is initialized with a native-like no-op when
unavailable, then stub it via vi.spyOn instead of direct assignment. Keep
afterEach’s vi.restoreAllMocks cleanup so the prototype is restored between
tests.
In `@coding-agent/webui/app/src/components/Timeline.tsx`:
- Around line 192-205: Add accessible status text to the three indicator spans
in the Timeline component’s running/item.isError conditional, ensuring running,
failed, and done states are announced to assistive technology without relying on
title, while preserving the existing visual indicators.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b18d70cb-17f6-4afa-b04d-49d616eb8783
📒 Files selected for processing (24)
coding-agent/webui/app/src/App.test.tsxcoding-agent/webui/app/src/App.tsxcoding-agent/webui/app/src/components/CheckpointsPanel.test.tsxcoding-agent/webui/app/src/components/CheckpointsPanel.tsxcoding-agent/webui/app/src/components/Composer.tsxcoding-agent/webui/app/src/components/DiffPanel.tsxcoding-agent/webui/app/src/components/Header.tsxcoding-agent/webui/app/src/components/MemoryPanel.tsxcoding-agent/webui/app/src/components/ResultPanel.test.tsxcoding-agent/webui/app/src/components/ResultPanel.tsxcoding-agent/webui/app/src/components/ReviewQueue.test.tsxcoding-agent/webui/app/src/components/ReviewQueue.tsxcoding-agent/webui/app/src/components/RightRail.tsxcoding-agent/webui/app/src/components/SessionList.test.tsxcoding-agent/webui/app/src/components/SessionList.tsxcoding-agent/webui/app/src/components/SettingsPanel.test.tsxcoding-agent/webui/app/src/components/SettingsPanel.tsxcoding-agent/webui/app/src/components/Timeline.test.tsxcoding-agent/webui/app/src/components/Timeline.tsxcoding-agent/webui/app/src/index.csscoding-agent/webui/app/src/lib/api.tscoding-agent/webui/app/src/lib/timeline.test.tscoding-agent/webui/app/src/lib/timeline.tscoding-agent/webui/app/src/lib/types.ts
| // When provided, the static accepted list is replaced by the interactive | ||
| // review queue (status tabs + accept/reject transitions). | ||
| client?: AgentClient; | ||
| sessionId?: string; | ||
| onReviewsChanged?: () => void; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the prop comment: the accepted list is not replaced.
The comment states that the static accepted list is replaced by the review queue. The static "Accepted memories" section at lines 91-130 still renders whenever ReviewQueue renders, so accepted memories appear twice. Either update the comment to describe the queue as an addition, or skip the static section when the queue is active.
📝 Proposed comment fix
- // When provided, the static accepted list is replaced by the interactive
- // review queue (status tabs + accept/reject transitions).
+ // When provided, the interactive review queue (status tabs plus
+ // accept/reject transitions) renders above the static accepted list.
client?: AgentClient;📝 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.
| // When provided, the static accepted list is replaced by the interactive | |
| // review queue (status tabs + accept/reject transitions). | |
| client?: AgentClient; | |
| sessionId?: string; | |
| onReviewsChanged?: () => void; | |
| // When provided, the interactive review queue (status tabs plus | |
| // accept/reject transitions) renders above the static accepted list. | |
| client?: AgentClient; | |
| sessionId?: string; | |
| onReviewsChanged?: () => void; |
🤖 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 `@coding-agent/webui/app/src/components/MemoryPanel.tsx` around lines 15 - 19,
Update the prop comment above client, sessionId, and onReviewsChanged to state
that the interactive review queue is added alongside the static
accepted-memories list, not that it replaces it; preserve the existing rendering
behavior.
| useEffect(() => { | ||
| if (!expanded || initializedRef.current) return; | ||
| initializedRef.current = true; | ||
| let cancelled = false; | ||
| void (async () => { | ||
| const candidates = await load("candidate"); | ||
| if (cancelled || candidates === null) return; | ||
| if (candidates.length === 0) { | ||
| tabRef.current = "accepted"; | ||
| setTab("accepted"); | ||
| await load("accepted"); | ||
| } | ||
| })(); | ||
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [expanded, load]); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reset the first-load guard when sessionId changes.
initializedRef is never reset. load is recreated when sessionId changes, so this effect reruns, but the guard on line 64 returns immediately. The panel then keeps showing the review records of the previous session, and the accept and reject actions post against the new sessionId.
MemoryPanel renders ReviewQueue without a key, so the component stays mounted across a session switch.
🐛 Proposed fix: clear the guard and the records on session change
const tabRef = useRef<ReviewStatus>(tab);
+
+ // A session switch keeps this component mounted; drop the loaded records so
+ // the first expand of the new session refetches instead of showing stale rows.
+ useEffect(() => {
+ initializedRef.current = false;
+ setRecords([]);
+ setError(null);
+ setRejectingId(null);
+ setReason("");
+ tabRef.current = "candidate";
+ setTab("candidate");
+ }, [sessionId]);An alternative is to key the child in coding-agent/webui/app/src/components/MemoryPanel.tsx at line 89 with key={sessionId}, which remounts it and resets all local state.
📝 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.
| useEffect(() => { | |
| if (!expanded || initializedRef.current) return; | |
| initializedRef.current = true; | |
| let cancelled = false; | |
| void (async () => { | |
| const candidates = await load("candidate"); | |
| if (cancelled || candidates === null) return; | |
| if (candidates.length === 0) { | |
| tabRef.current = "accepted"; | |
| setTab("accepted"); | |
| await load("accepted"); | |
| } | |
| })(); | |
| return () => { | |
| cancelled = true; | |
| }; | |
| }, [expanded, load]); | |
| useEffect(() => { | |
| initializedRef.current = false; | |
| setRecords([]); | |
| setError(null); | |
| setRejectingId(null); | |
| setReason(""); | |
| tabRef.current = "candidate"; | |
| setTab("candidate"); | |
| }, [sessionId]); | |
| useEffect(() => { | |
| if (!expanded || initializedRef.current) return; | |
| initializedRef.current = true; | |
| let cancelled = false; | |
| void (async () => { | |
| const candidates = await load("candidate"); | |
| if (cancelled || candidates === null) return; | |
| if (candidates.length === 0) { | |
| tabRef.current = "accepted"; | |
| setTab("accepted"); | |
| await load("accepted"); | |
| } | |
| })(); | |
| return () => { | |
| cancelled = true; | |
| }; | |
| }, [expanded, load]); |
🤖 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 `@coding-agent/webui/app/src/components/ReviewQueue.tsx` around lines 63 - 79,
Reset ReviewQueue’s initializedRef and clear its loaded review records whenever
sessionId changes, so the first-load effect runs for the new session and stale
records are not displayed. Update the session-aware logic around the ReviewQueue
effect and state, or alternatively key the ReviewQueue child in MemoryPanel by
sessionId to force a remount; preserve the existing loading behavior for each
session.
| const [provider, setProvider] = useState(providerName ?? ""); | ||
| const [model, setModel] = useState(modelName ?? ""); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Sync provider and model when the props change.
useState(providerName ?? "") and useState(modelName ?? "") read the props only on mount. App.tsx keys SettingsPanel by sessionId (line 715), so the panel does not remount while the same session stays active. A checkpoint restore rewinds session runtime settings, as the warning in CheckpointsPanel.tsx states, and App.tsx then reloads the session summary. The new providerName and modelName props arrive, but these two fields keep showing the pre-restore values. Add an effect that resyncs both fields when the props change.
🔧 Proposed fix
const [provider, setProvider] = useState(providerName ?? "");
const [model, setModel] = useState(modelName ?? "");
+
+ // Runtime settings can change outside this panel (for example a checkpoint
+ // restore), so track the reported session values.
+ useEffect(() => {
+ setProvider(providerName ?? "");
+ }, [providerName]);
+ useEffect(() => {
+ setModel(modelName ?? "");
+ }, [modelName]);📝 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.
| const [provider, setProvider] = useState(providerName ?? ""); | |
| const [model, setModel] = useState(modelName ?? ""); | |
| const [provider, setProvider] = useState(providerName ?? ""); | |
| const [model, setModel] = useState(modelName ?? ""); | |
| // Runtime settings can change outside this panel (for example a checkpoint | |
| // restore), so track the reported session values. | |
| useEffect(() => { | |
| setProvider(providerName ?? ""); | |
| }, [providerName]); | |
| useEffect(() => { | |
| setModel(modelName ?? ""); | |
| }, [modelName]); |
🤖 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 `@coding-agent/webui/app/src/components/SettingsPanel.tsx` around lines 26 -
27, In SettingsPanel, add an effect that updates both provider and model state
from providerName and modelName whenever those props change, preserving
empty-string fallbacks for undefined values and keeping the existing local state
initialization.
| --color-ok: #3fb950; | ||
| --color-err: #f85149; | ||
| --color-warn: #d29922; | ||
| --font-mono: ui-monospace, SFMono-Regular, Menlo, monospace; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the Stylelint value-keyword-case failure.
Stylelint reports two errors on this line for SFMono-Regular and Menlo. Quote the multi-word and proper-name families. The rule ignores quoted strings, and quoting is the recommended form for font family names that are not generic keywords.
🔧 Proposed fix
- --font-mono: ui-monospace, SFMono-Regular, Menlo, monospace;
+ --font-mono: ui-monospace, "SFMono-Regular", "Menlo", monospace;📝 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.
| --font-mono: ui-monospace, SFMono-Regular, Menlo, monospace; | |
| --font-mono: ui-monospace, "SFMono-Regular", "Menlo", monospace; |
🧰 Tools
🪛 Stylelint (17.14.1)
[error] 16-16: Expected "SFMono-Regular" to be "sfmono-regular" (value-keyword-case)
(value-keyword-case)
[error] 16-16: Expected "Menlo" to be "menlo" (value-keyword-case)
(value-keyword-case)
🤖 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 `@coding-agent/webui/app/src/index.css` at line 16, Update the --font-mono
declaration to quote the SFMono-Regular and Menlo font-family names, while
leaving the generic ui-monospace and monospace keywords unquoted.
Source: Linters/SAST tools
三栏工作台重构(承接已关闭的 #683,该 PR 因分支 force-push 历史无法重开,以新 PR 交付同一分支):
布局与视觉
turn 流与审批
功能补齐(服务端早已支持,前端首次接线)
质量过程
Summary by CodeRabbit
New Features
Bug Fixes
Tests