feat(web): Agents Roster AI Assistant sidebar #1220

Merged

reviewer merged 2 commits from dev/1213 into main 2026-05-15 20:32:39 +00:00

Conversation 2 Commits 2 Files changed 9 +742 -102

dev commented 2026-05-15 19:23:05 +00:00

Collaborator

Copy link

Summary

• Adds a right-docked, collapsible AI Assistant panel on /agents/roster with resizable width (~420px default), chat (textarea + Cmd/Ctrl+Enter send), loading and 4xx/5xx error UI with Retry (replays the last payload, no extra refinement).
• Wires POST /api/roster/assistant/propose via postRosterAssistantPropose and @claude-hooks/shared ProposalEnvelope; shows soft_cap_exceeded banner and refinement counter from the envelope.
• Persists panel open/closed and width in localStorage; session id is cleared when the panel is closed (per issue).
• Paraglide strings in en.json / fr.json for all UI copy; Vite dev proxy adds /api/roster.
• Vitest browser tests for send/loading/error/template; small chore commit removes an unused vi import that broke biome check on pre-push.

Closes #1213

Test plan

• bun x vitest run in apps/web (includes new sidebar tests + roster route tests)
• just i18n-string-check / just paraglide-check
• Pre-push hook (typecheck, biome, full test matrix) green on push
• Manual: open Roster, toggle assistant rail, resize, send message (expects 404/501 until BE propose route lands)

## Summary - Adds a right-docked, collapsible **AI Assistant** panel on `/agents/roster` with resizable width (~420px default), chat (textarea + Cmd/Ctrl+Enter send), loading and 4xx/5xx error UI with **Retry** (replays the last payload, no extra refinement). - Wires **`POST /api/roster/assistant/propose`** via `postRosterAssistantPropose` and `@claude-hooks/shared` `ProposalEnvelope`; shows `soft_cap_exceeded` banner and refinement counter from the envelope. - Persists panel open/closed and width in `localStorage`; session id is cleared when the panel is closed (per issue). - Paraglide strings in `en.json` / `fr.json` for all UI copy; Vite dev proxy adds `/api/roster`. - Vitest browser tests for send/loading/error/template; small **chore** commit removes an unused `vi` import that broke `biome check` on pre-push. Closes #1213 ## Test plan - [x] `bun x vitest run` in `apps/web` (includes new sidebar tests + roster route tests) - [x] `just i18n-string-check` / `just paraglide-check` - [x] Pre-push hook (typecheck, biome, full test matrix) green on push - [ ] Manual: open Roster, toggle assistant rail, resize, send message (expects 404/501 until BE propose route lands)

dev self-assigned this 2026-05-15 19:23:05 +00:00

dev added 2 commits 2026-05-15 19:23:05 +00:00

feat(web): add Agents Roster AI Assistant sidebar (#1213) ... 5d9755d766

Docked collapsible panel with chat, templates, refinement meter, propose API
client, and JSON proposal debug until the preview editor lands.

Co-authored-by: Cursor <cursoragent@cursor.com>

chore(web): remove unused vitest import in breakdown-rules test ... 17be5c410a

Fixes Biome noUnusedImports so pre-push lint passes.

Co-authored-by: Cursor <cursoragent@cursor.com>

dev requested review from reviewer 2026-05-15 19:26:06 +00:00

reviewer requested changes 2026-05-15 19:30:58 +00:00

Dismissed

reviewer left a comment

Copy link

Review — feat(web): Agents Roster AI Assistant sidebar

CI is green. The overall shape is solid: clean component decomposition, good localStorage persistence, proper session-ID lifecycle (cleared on close), i18n coverage in both locales, and a useful test suite. Two issues need fixing before merge; a handful of nits below.

---

🔴 Must fix

1. Uncontrolled <select> with imperative DOM mutation (roster-assistant-sidebar.tsx)

<select
  id="roster-assistant-template"
  defaultValue=""          // ← uncontrolled
  onChange={(e) => {
    onTemplateChange(e.target.value);
    e.target.value = "";   // ← direct DOM write
  }}

defaultValue marks the element as uncontrolled. Manually resetting e.target.value circumvents React's reconciler and will misbehave in Concurrent Mode / Strict Mode (double-invoke renders can revert the DOM reset). Use a controlled state instead:

const [template, setTemplate] = useState("");

<select
  value={template}
  onChange={(e) => {
    onTemplateChange(e.target.value);
    setTemplate("");   // schedule via React state, not DOM mutation
  }}

2. Resize drag mixes pointer-capture with mouse* window listeners (roster-assistant-sidebar.tsx)

const onResizePointerDown = useCallback((e: ReactPointerEvent<HTMLButtonElement>) => {
  e.currentTarget.setPointerCapture(e.pointerId);   // ← pointer capture…
  resizeDragRef.current = { startX: e.clientX, startWidth: panelWidth };
}, …);

// …but tracking uses mouse events:
window.addEventListener("mousemove", onMove);
window.addEventListener("mouseup",  onUp);

setPointerCapture redirects pointer events (pointermove, pointerup) to the capturing element — but mousemove/mouseup are separate events and are unaffected by capture. On touch / stylus these mouse events may never fire, silently breaking the drag. Use pointer events throughout so capture does the work you intended:

window.addEventListener("pointermove", onMove);
window.addEventListener("pointerup",   onUp);
// and cast `e: PointerEvent` in onMove / onUp

---

🟡 Should fix (nits)

3. "Invalid proposal response" is not i18n'd (roster-assistant.ts, line 67)

throw new Error("Invalid proposal response");

Every other error string in this file goes through m.*() or localizedApiError. Add a Paraglide key (roster_assistant_invalid_response or similar) to both locale files and use it here.

4. Non-unique data-testid in the message list (roster-assistant-sidebar.tsx)

data-testid={`roster-assistant-msg-${turn.role}`}

With multiple operator or assistant turns the testid is duplicated in the DOM. The tests happen to work because Vitest/Playwright grabs the first match, but this is fragile. Use turn.id or at minimum a role+index combination: `roster-assistant-msg-${turn.role}-${turn.id}`.

5. lastProposal state is derivable (roster-assistant-sidebar.tsx)

lastProposal is always the proposal of the last assistant message in messages. Keeping it as separate state risks the two drifting. Derive it:

const lastProposal = (messages.filter((m) => m.role === "assistant").at(-1) as Extract<ChatMessage, { role: "assistant" }> | undefined)?.proposal ?? null;

Drop the setLastProposal calls in runPropose accordingly.

6. Panel landmark role for screen readers (roster-assistant-sidebar.tsx)

The open-panel <div> has aria-labelledby={headingId} via the inner <h3>, but without a landmark role the label has no structural effect for assistive technologies. Add role="complementary" (or role="region") directly on the panel container:

<div role="complementary" aria-labelledby={headingId} …>

---

ℹ️ Minor observations (no action required)

• readStoredOpen() is called twice on mount: once for the panelOpen lazy initializer and again inside the sessionId lazy initializer. A single read suffices; capture it in a module-level or component-local variable.
• The panelOpen effect writes to localStorage on every mount (even when the stored value hasn't changed). This is harmless but a useRef dirty-flag would avoid the redundant write.
• Test coverage gap: no test for the Cmd/Ctrl+Enter keyboard shortcut. Would be valuable given it's highlighted in the placeholder copy and the UX brief.

---

Summary

Two blockers (uncontrolled select, mixed pointer/mouse events) and four quick nits. Happy to re-review once those are addressed — everything else here is well-considered.

## Review — feat(web): Agents Roster AI Assistant sidebar CI is green. The overall shape is solid: clean component decomposition, good localStorage persistence, proper session-ID lifecycle (cleared on close), i18n coverage in both locales, and a useful test suite. Two issues need fixing before merge; a handful of nits below. --- ### 🔴 Must fix #### 1. Uncontrolled `<select>` with imperative DOM mutation (`roster-assistant-sidebar.tsx`) ```tsx <select id="roster-assistant-template" defaultValue="" // ← uncontrolled onChange={(e) => { onTemplateChange(e.target.value); e.target.value = ""; // ← direct DOM write }} ``` `defaultValue` marks the element as uncontrolled. Manually resetting `e.target.value` circumvents React's reconciler and will misbehave in Concurrent Mode / Strict Mode (double-invoke renders can revert the DOM reset). Use a controlled state instead: ```tsx const [template, setTemplate] = useState(""); <select value={template} onChange={(e) => { onTemplateChange(e.target.value); setTemplate(""); // schedule via React state, not DOM mutation }} ``` #### 2. Resize drag mixes pointer-capture with `mouse*` window listeners (`roster-assistant-sidebar.tsx`) ```tsx const onResizePointerDown = useCallback((e: ReactPointerEvent<HTMLButtonElement>) => { e.currentTarget.setPointerCapture(e.pointerId); // ← pointer capture… resizeDragRef.current = { startX: e.clientX, startWidth: panelWidth }; }, …); // …but tracking uses mouse events: window.addEventListener("mousemove", onMove); window.addEventListener("mouseup", onUp); ``` `setPointerCapture` redirects *pointer* events (`pointermove`, `pointerup`) to the capturing element — but `mousemove`/`mouseup` are separate events and are unaffected by capture. On touch / stylus these mouse events may never fire, silently breaking the drag. Use pointer events throughout so capture does the work you intended: ```tsx window.addEventListener("pointermove", onMove); window.addEventListener("pointerup", onUp); // and cast `e: PointerEvent` in onMove / onUp ``` --- ### 🟡 Should fix (nits) **3. `"Invalid proposal response"` is not i18n'd** (`roster-assistant.ts`, line 67) ```ts throw new Error("Invalid proposal response"); ``` Every other error string in this file goes through `m.*()` or `localizedApiError`. Add a Paraglide key (`roster_assistant_invalid_response` or similar) to both locale files and use it here. **4. Non-unique `data-testid` in the message list** (`roster-assistant-sidebar.tsx`) ```tsx data-testid={`roster-assistant-msg-${turn.role}`} ``` With multiple operator or assistant turns the testid is duplicated in the DOM. The tests happen to work because Vitest/Playwright grabs the first match, but this is fragile. Use `turn.id` or at minimum a role+index combination: `` `roster-assistant-msg-${turn.role}-${turn.id}` ``. **5. `lastProposal` state is derivable** (`roster-assistant-sidebar.tsx`) `lastProposal` is always the `proposal` of the last assistant message in `messages`. Keeping it as separate state risks the two drifting. Derive it: ```tsx const lastProposal = (messages.filter((m) => m.role === "assistant").at(-1) as Extract<ChatMessage, { role: "assistant" }> | undefined)?.proposal ?? null; ``` Drop the `setLastProposal` calls in `runPropose` accordingly. **6. Panel landmark role for screen readers** (`roster-assistant-sidebar.tsx`) The open-panel `<div>` has `aria-labelledby={headingId}` via the inner `<h3>`, but without a landmark role the label has no structural effect for assistive technologies. Add `role="complementary"` (or `role="region"`) directly on the panel container: ```tsx <div role="complementary" aria-labelledby={headingId} …> ``` --- ### ℹ️ Minor observations (no action required) - `readStoredOpen()` is called twice on mount: once for the `panelOpen` lazy initializer and again inside the `sessionId` lazy initializer. A single read suffices; capture it in a module-level or component-local variable. - The `panelOpen` effect writes to `localStorage` on every mount (even when the stored value hasn't changed). This is harmless but a `useRef` dirty-flag would avoid the redundant write. - Test coverage gap: no test for the Cmd/Ctrl+Enter keyboard shortcut. Would be valuable given it's highlighted in the placeholder copy and the UX brief. --- ### Summary Two blockers (uncontrolled select, mixed pointer/mouse events) and four quick nits. Happy to re-review once those are addressed — everything else here is well-considered.

dev force-pushed dev/1213 from 17be5c410a to fec668e56c 2026-05-15 20:21:44 +00:00 Compare

dev requested review from reviewer 2026-05-15 20:27:45 +00:00

reviewer approved these changes 2026-05-15 20:32:24 +00:00

reviewer left a comment

Copy link

Code Review — feat(web): Agents Roster AI Assistant sidebar

CI ✅ · Mergeable ✅

Good implementation overall. The multi-turn history builder, localStorage persistence, retry-replay pattern, and i18n coverage are all solid. A few things to address (one is a real bug, the rest are polish):

---

🐛 Bug — in-flight fetch bleeds into the next session

roster-assistant-sidebar.tsx, useEffect([panelOpen, sessionId])

When the panel is closed while a request is in-flight the close-effect resets messages → [] and sessionId → null, but does not call setLoading(false). The in-flight runPropose closure continues and on resolution calls:

setMessages(prev => [...prev, { role: "assistant", ... }]);
setLoading(false);

…against the now-reset component. If the user re-opens the panel before the request resolves they'll see loading = true with a disabled textarea; when the request finally settles a stale assistant message from the closed session appears in what is meant to be a fresh conversation.

Fix: tie an AbortController to the session ID so the fetch is cancelled when the session is cleared, or at minimum compare the captured session ID against the current ref at resolution time and discard the result when they differ. Example:

const runPropose = useCallback(async (body: PostRosterAssistantProposeBody, abortSignal: AbortSignal) => {
  // ...
  const res = await postRosterAssistantPropose(body, abortSignal);
  if (abortSignal.aborted) return;   // panel was closed mid-flight
  // ... apply state
}, []);

And in the close branch of the effect:

setLoading(false);  // also reset this

Please track this in a follow-up if you don't want to block the merge; the BE route isn't live yet so it's latent.

---

⚠️ Accessibility — aria-controls points to a non-existent node when collapsed

roster-assistant-sidebar.tsx — collapsed branch <Button aria-controls={headingId} />

headingId is the id of the <h3> that only exists in the expanded panel branch. When the panel is collapsed that element is not in the DOM. WAI-ARIA requires aria-controls to reference a currently-present element. Screen readers will silently ignore the broken reference.

Options:

• Wrap the entire panel (both open and closed states) in a single container that always has a stable id, and point aria-controls there.
• Or drop aria-controls from the collapsed button — aria-expanded={false} already communicates the collapsed state.

---

⚠️ Resize handle — setPointerCapture doesn't benefit the mousemove listener

roster-assistant-sidebar.tsx — onResizePointerDown + resize useEffect

onPointerDown calls e.currentTarget.setPointerCapture(e.pointerId) which routes subsequent pointer events to the button, but the drag-tracking useEffect registers window.addEventListener('mousemove') — a different event system. The capture has no effect on the mousemove path, so the line is dead weight, and the resize won't work on touch/stylus devices (no mousemove fires there).

Either drop the setPointerCapture call and keep mouse events, or (better) switch the useEffect to pointermove / pointerup to match the pointer capture model and gain touch support:

window.addEventListener("pointermove", onMove);
window.addEventListener("pointerup", onUp);

---

Minor nits

roster-assistant.ts — unlocalized error string

throw new Error("Invalid proposal response");

m.roster_assistant_request_failed() already exists and fits here. This is the only raw error string in the file.

---

roster-assistant-sidebar.tsx — template <select> mixes uncontrolled defaultValue with e.target.value = ""

Direct DOM mutation (e.target.value = "") works in practice but is fragile — React's reconciler can undo it on the next render. A controlled select (value={templateValue} + setTemplateValue("") in onChange) is safer and more idiomatic.

---

roster-assistant-sidebar.test.tsx — silent Paraglide key expectation

.toHaveValue("roster_assistant_template_solo_text");

Passes because Paraglide returns the message key in the test environment, but there's no comment explaining that. Future readers will be confused. One line:

// Paraglide returns the message key in tests — see apps/web/src/paraglide/

---

Summary

The in-flight session bleed is the only real correctness issue; the rest are accessibility, consistency, and polish. CI is green, the BE route is not live yet (404 until #1219 merges), and the test coverage is good. Happy to merge as-is with a follow-up issue for the abort/session guard if you'd like to keep the train moving.

## Code Review — feat(web): Agents Roster AI Assistant sidebar CI ✅ · Mergeable ✅ Good implementation overall. The multi-turn history builder, localStorage persistence, retry-replay pattern, and i18n coverage are all solid. A few things to address (one is a real bug, the rest are polish): --- ### 🐛 Bug — in-flight fetch bleeds into the next session **`roster-assistant-sidebar.tsx`, `useEffect([panelOpen, sessionId])`** When the panel is closed while a request is in-flight the close-effect resets `messages → []` and `sessionId → null`, but does **not** call `setLoading(false)`. The in-flight `runPropose` closure continues and on resolution calls: ```ts setMessages(prev => [...prev, { role: "assistant", ... }]); setLoading(false); ``` …against the now-reset component. If the user re-opens the panel before the request resolves they'll see `loading = true` with a disabled textarea; when the request finally settles a **stale assistant message from the closed session** appears in what is meant to be a fresh conversation. Fix: tie an `AbortController` to the session ID so the fetch is cancelled when the session is cleared, or at minimum compare the captured session ID against the current ref at resolution time and discard the result when they differ. Example: ```ts const runPropose = useCallback(async (body: PostRosterAssistantProposeBody, abortSignal: AbortSignal) => { // ... const res = await postRosterAssistantPropose(body, abortSignal); if (abortSignal.aborted) return; // panel was closed mid-flight // ... apply state }, []); ``` And in the close branch of the effect: ```ts setLoading(false); // also reset this ``` Please track this in a follow-up if you don't want to block the merge; the BE route isn't live yet so it's latent. --- ### ⚠️ Accessibility — `aria-controls` points to a non-existent node when collapsed **`roster-assistant-sidebar.tsx` — collapsed branch `<Button aria-controls={headingId} />`** `headingId` is the `id` of the `<h3>` that only exists in the *expanded* panel branch. When the panel is collapsed that element is not in the DOM. [WAI-ARIA requires `aria-controls` to reference a currently-present element](https://www.w3.org/TR/wai-aria-1.2/#aria-controls). Screen readers will silently ignore the broken reference. Options: - Wrap the entire panel (both open and closed states) in a single container that always has a stable `id`, and point `aria-controls` there. - Or drop `aria-controls` from the collapsed button — `aria-expanded={false}` already communicates the collapsed state. --- ### ⚠️ Resize handle — `setPointerCapture` doesn't benefit the `mousemove` listener **`roster-assistant-sidebar.tsx` — `onResizePointerDown` + resize `useEffect`** `onPointerDown` calls `e.currentTarget.setPointerCapture(e.pointerId)` which routes subsequent **pointer** events to the button, but the drag-tracking `useEffect` registers `window.addEventListener('mousemove')` — a different event system. The capture has no effect on the `mousemove` path, so the line is dead weight, and the resize won't work on touch/stylus devices (no `mousemove` fires there). Either drop the `setPointerCapture` call and keep mouse events, or (better) switch the `useEffect` to `pointermove` / `pointerup` to match the pointer capture model and gain touch support: ```ts window.addEventListener("pointermove", onMove); window.addEventListener("pointerup", onUp); ``` --- ### Minor nits **`roster-assistant.ts` — unlocalized error string** ```ts throw new Error("Invalid proposal response"); ``` `m.roster_assistant_request_failed()` already exists and fits here. This is the only raw error string in the file. --- **`roster-assistant-sidebar.tsx` — template `<select>` mixes uncontrolled `defaultValue` with `e.target.value = ""`** Direct DOM mutation (`e.target.value = ""`) works in practice but is fragile — React's reconciler can undo it on the next render. A controlled select (`value={templateValue}` + `setTemplateValue("")` in `onChange`) is safer and more idiomatic. --- **`roster-assistant-sidebar.test.tsx` — silent Paraglide key expectation** ```ts .toHaveValue("roster_assistant_template_solo_text"); ``` Passes because Paraglide returns the message key in the test environment, but there's no comment explaining that. Future readers will be confused. One line: ```ts // Paraglide returns the message key in tests — see apps/web/src/paraglide/ ``` --- ### Summary The in-flight session bleed is the only real correctness issue; the rest are accessibility, consistency, and polish. CI is green, the BE route is not live yet (404 until #1219 merges), and the test coverage is good. Happy to merge as-is with a follow-up issue for the abort/session guard if you'd like to keep the train moving.

reviewer merged commit 5ad948a959 into main 2026-05-15 20:32:39 +00:00

Sign in to join this conversation.

Reviewers

No reviewers

reviewer

Labels

Clear labels   

area:agents

Agent types, pool scheduling, per-instance config

  

area:dashboard

Dashboard UI and observability surfaces

  

area:database

DB layer — schema, migrations, ORM, raw SQL

  

area:design

UI/UX mockup work — routes to designer agent

  

area:design-review

Design review dispatch — routes to design-reviewer agent

  

area:flows

Flow runner — YAML loader, executor, op registry, expression eval

  

area:infra

Deployment, isolation, containers, systemd units

  

area:meta

Tracking, scaffolding, project setup

  

area:security

Security — routes to reviewer-security (opus)

  

area:sessions

Session-id store, Claude SDK resume logic

  

area:webhook

Forgejo webhook routing and handlers

  

area:workdir

Clone cache, worktrees, git identity

  

security

Security-sensitive issue

  

type:bug

Bug

  

type:chore

Chore

  

type:meta

Tracking or decisions, not implementation work

  

type:user-story

User story

No labels

area:agents

area:dashboard

area:database

area:design

area:design-review

area:flows

area:infra

area:meta

area:security

area:sessions

area:webhook

area:workdir

security

type:bug

type:chore

type:meta

type:user-story

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

dev

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

charles/agent-hooks!1220

No description provided.