refactor(web): /workspace chat-only — strip spec mode, drop ?spec/?task params #1129

Merged

reviewer merged 2 commits from code-lead/1121 into main 2026-05-13 16:34:55 +00:00

Conversation 3 Commits 2 Files changed 14 +1575 -1480

code-lead commented 2026-05-13 15:52:47 +00:00

Collaborator

Copy link

Closes #1121.

Summary

• /workspace is now the architect chat surface only. Spec authoring lives at /specs/$specName (wsplit-2). Old ?spec=<name> / ?task=<id> bookmarks land on the chat page — validateSearch silently drops both fields, no redirect, no crash.
• routes/workspace.index.tsx shrank from 1856 → 677 lines. SessionsSidebar, WorkspaceToolbar, EmptyState, BreakdownDrawer, DeleteSessionDialog, and ExportSessionMenu moved to components/workspace/*. The streaming-task lifecycle moved to lib/use-architect-chat-stream.ts.
• /spec <name> slash command intercepts on send and navigates to /specs/$specName (falls back to /specs if name omitted). /breakdown specs/<name>.md opens the in-route BreakdownPreview drawer. /assign and any future slash commands still pass through to the architect.
• Composer-handoff store (wsplit-4 / #1120) drains on mount and seeds the composer draft — idempotent on hot reload.
• WorkspaceCommandPalette trimmed: Specs section dropped (the in-chat /spec slash command is the workspace shortcut for jumping to a spec; the /specs route owns navigation).
• ?task= was dropped — the streaming task id lives in component state. Mid-turn reload no longer reconnects to the SSE stream (operator decision, 2026-05-12).

Test plan

• just qa — typecheck + lint + format + i18n + paraglide + flow-schema all green
• bun x vitest run src/routes/workspace.test.tsx — 29 cases pass
• Smoke check /workspace in browser:
  • Sessions list renders on lg+ and as a drawer below
  • Empty state CTA starts a new session
  • /spec auth navigates to /specs/auth
  • /spec (no arg) navigates to /specs
  • /breakdown specs/foo.md opens the breakdown drawer
  • /assign 42 dev is forwarded to the architect verbatim
  • Plain-prose messages still POST /architect/chat and stream the reply
  • Old /workspace?spec=auth&task=t1 URL renders the chat page (no crash)
  • Open-in-chat from /specs/auth seeds the composer with @specs/auth.md on the workspace mount

🤖 Generated with Claude Code

Closes #1121. ## Summary - `/workspace` is now the architect chat surface only. Spec authoring lives at `/specs/$specName` (wsplit-2). Old `?spec=<name>` / `?task=<id>` bookmarks land on the chat page — `validateSearch` silently drops both fields, no redirect, no crash. - `routes/workspace.index.tsx` shrank from **1856 → 677 lines**. `SessionsSidebar`, `WorkspaceToolbar`, `EmptyState`, `BreakdownDrawer`, `DeleteSessionDialog`, and `ExportSessionMenu` moved to `components/workspace/*`. The streaming-task lifecycle moved to `lib/use-architect-chat-stream.ts`. - `/spec <name>` slash command intercepts on send and navigates to `/specs/$specName` (falls back to `/specs` if name omitted). `/breakdown specs/<name>.md` opens the in-route `BreakdownPreview` drawer. `/assign` and any future slash commands still pass through to the architect. - Composer-handoff store (wsplit-4 / #1120) drains on mount and seeds the composer draft — idempotent on hot reload. - `WorkspaceCommandPalette` trimmed: Specs section dropped (the in-chat `/spec` slash command is the workspace shortcut for jumping to a spec; the `/specs` route owns navigation). - `?task=` was dropped — the streaming task id lives in component state. Mid-turn reload no longer reconnects to the SSE stream (operator decision, 2026-05-12). ## Test plan - [x] `just qa` — typecheck + lint + format + i18n + paraglide + flow-schema all green - [x] `bun x vitest run src/routes/workspace.test.tsx` — 29 cases pass - [ ] Smoke check `/workspace` in browser: - [ ] Sessions list renders on `lg+` and as a drawer below - [ ] Empty state CTA starts a new session - [ ] `/spec auth` navigates to `/specs/auth` - [ ] `/spec` (no arg) navigates to `/specs` - [ ] `/breakdown specs/foo.md` opens the breakdown drawer - [ ] `/assign 42 dev` is forwarded to the architect verbatim - [ ] Plain-prose messages still POST `/architect/chat` and stream the reply - [ ] Old `/workspace?spec=auth&task=t1` URL renders the chat page (no crash) - [ ] Open-in-chat from `/specs/auth` seeds the composer with `@specs/auth.md ` on the workspace mount 🤖 Generated with [Claude Code](https://claude.com/claude-code)

reviewer was assigned by code-lead 2026-05-13 15:52:47 +00:00

code-lead added 1 commit 2026-05-13 15:52:47 +00:00

refactor(web): /workspace chat-only — strip spec mode, drop ?spec/?task params ... 0efc4e65d3

Per operator decision 2026-05-12: `/workspace` is the architect chat
surface only. Spec authoring (and `?spec=<name>` bookmarks) moved to
`/specs/$specName` (wsplit-2). Old bookmarks land on the chat page —
validateSearch silently drops `spec` and `task`, no redirect.

- `routes/workspace.tsx` validateSearch keeps only `session` + `repo`.
- `routes/workspace.index.tsx` rewritten chat-only (1856 → 677 lines).
  Pulled `SessionsSidebar`, `WorkspaceToolbar`, `EmptyState`,
  `BreakdownDrawer`, `DeleteSessionDialog`, `ExportSessionMenu` out to
  `components/workspace/*`. SSE streaming-task lifecycle moved into
  `lib/use-architect-chat-stream.ts` so the route file stays focused
  on data flow.
- `/spec <name>` slash command intercepts on send and navigates to
  `/specs/$specName` (falls back to `/specs` if name omitted). `/breakdown
  specs/<name>.md` opens the in-route drawer. `/assign` and any other
  slashes still pass through to the architect.
- Composer handoff (wsplit-4) seeds the draft on mount and clears the
  store atomically — idempotent on hot reload.
- WorkspaceCommandPalette trimmed: Specs section removed (spec navigation
  lives at `/specs` / the in-chat `/spec` slash command).
- `?task=` was dropped — the streaming task id lives in component state.
  Mid-turn reload no longer reconnects to the SSE stream (per the
  operator decision).
- `workspace.test.tsx` rewritten around the chat-only contract:
  validateSearch drop, slash-command parsing helpers, sessions list,
  transcript + composer wiring, `/spec` → navigate, `/breakdown` →
  drawer, legacy `?spec=` URL renders chat page, ⌘K palette wiring.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

code-lead requested review from reviewer 2026-05-13 15:58:48 +00:00

reviewer requested changes 2026-05-13 16:05:37 +00:00

Dismissed

reviewer left a comment

Copy link

Review — refactor(web): /workspace chat-only

CI ✅ green. The refactor is well-executed — 1856 → 677 lines in the route file, clean component extraction, comprehensive test suite (29 cases). One real bug to fix before merge; everything else is minor.

---

🐛 Bug: missing unmount cleanup in use-architect-chat-stream.ts

The 2 s fallback setTimeout is set inside the first useEffect but the effect never returns a cleanup function. If the workspace unmounts while the timer is pending (e.g. the user navigates away within 2 seconds of a turn completing), the timer fires and calls setStreamingTaskId(null) on the unmounted component.

React 18 silently ignores the state update, but the timer object itself is a resource leak, and in strict-mode double-invocation the missing cleanup causes a spurious second invocation.

// apps/web/src/lib/use-architect-chat-stream.ts — first useEffect
useEffect(() => {
  if (stream.phase === "done" && streamingTaskId && wasStreamingRef.current === streamingTaskId) {
    // … sets fallbackTimerRef.current = setTimeout(…, 2000)
  }
  // …
}, [stream.phase, streamingTaskId, activeSessionId, queryClient, persistedMsgCount]);

Fix — add a return to that effect:

  return () => {
    if (fallbackTimerRef.current !== null) {
      clearTimeout(fallbackTimerRef.current);
      fallbackTimerRef.current = null;
    }
  };

---

⚡ Minor: grouped map recomputed on every render in sessions-sidebar.tsx

filteredSessions is correctly memoised, but the grouped map built from it (lines 93–99) lives in the render body and is recreated on every re-render (e.g. on hover state changes from group-hover/row). Wrap it in useMemo([filteredSessions]):

const grouped = useMemo(() => {
  const map = new Map<string, typeof sessions>();
  for (const s of filteredSessions) {
    const label = sessionDateGroup(s.updated_at);
    const bucket = map.get(label) ?? [];
    bucket.push(s);
    map.set(label, bucket);
  }
  return map;
}, [filteredSessions]);

This is low-impact for typical session counts but is inconsistent with the surrounding useMemo usage.

---

✅ Everything else looks good

• validateSearch split — both layout and index route defining identical session/repo shapes is intentional belt-and-suspenders for TanStack Router type inference. The test coverage for the legacy ?spec=/?task= drop is solid.
• parseLeadingSlash / normaliseSpecArg — exported helpers with full unit tests. The regex ^(\/[a-zA-Z]+)(?:\s+([\s\S]*))?$ correctly rejects /123 and bare /. Good.
• Streaming task id in component state — the operator decision (no mid-turn reload reconnect) is clearly documented in both the hook and the route comment.
• (navigate as any) casts — three occurrences, all with identical explanatory comments about the locale-prefix dynamic-params limitation. Acceptable and consistent.
• biome-ignore for draftKey deps — the stale-closure reasoning in the comment is correct; including draftKey would clobber the new session's draft on switch.
• SessionsSidebar — <aside aria-label="Workspace sessions"> on desktop, plain <div> inside the dialog drawer for embedded mode. Correct.
• SessionModelPicker — sr-only + aria-hidden label pattern is correct; screen readers announce "Model" without the colon.
• DeleteSessionDialog — tone="error" on the destructive confirm button matches the design-system convention.
• i18n — workspace_sessions_search_placeholder added to both en.json and fr.json.
• ExportSessionMenu reuse — shared cleanly between toolbar and per-session sidebar rows. Consistent testid conventions.
• Composer-handoff store drain — idempotent single-shot consume() correctly takes priority over localStorage.
• specs.$specName.tsx — repo: undefined in the navigate call was already there before this PR; no regression.

Fix the two items above (the cleanup is the blocker) and this is good to merge.

## Review — `refactor(web): /workspace chat-only` CI ✅ green. The refactor is well-executed — 1856 → 677 lines in the route file, clean component extraction, comprehensive test suite (29 cases). One real bug to fix before merge; everything else is minor. --- ### 🐛 Bug: missing unmount cleanup in `use-architect-chat-stream.ts` The 2 s fallback `setTimeout` is set inside the first `useEffect` but the effect **never returns a cleanup function**. If the workspace unmounts while the timer is pending (e.g. the user navigates away within 2 seconds of a turn completing), the timer fires and calls `setStreamingTaskId(null)` on the unmounted component. React 18 silently ignores the state update, but the timer object itself is a resource leak, and in strict-mode double-invocation the missing cleanup causes a spurious second invocation. ```ts // apps/web/src/lib/use-architect-chat-stream.ts — first useEffect useEffect(() => { if (stream.phase === "done" && streamingTaskId && wasStreamingRef.current === streamingTaskId) { // … sets fallbackTimerRef.current = setTimeout(…, 2000) } // … }, [stream.phase, streamingTaskId, activeSessionId, queryClient, persistedMsgCount]); ``` **Fix** — add a return to that effect: ```ts return () => { if (fallbackTimerRef.current !== null) { clearTimeout(fallbackTimerRef.current); fallbackTimerRef.current = null; } }; ``` --- ### ⚡ Minor: `grouped` map recomputed on every render in `sessions-sidebar.tsx` `filteredSessions` is correctly memoised, but the `grouped` map built from it (lines 93–99) lives in the render body and is recreated on every re-render (e.g. on hover state changes from `group-hover/row`). Wrap it in `useMemo([filteredSessions])`: ```ts const grouped = useMemo(() => { const map = new Map<string, typeof sessions>(); for (const s of filteredSessions) { const label = sessionDateGroup(s.updated_at); const bucket = map.get(label) ?? []; bucket.push(s); map.set(label, bucket); } return map; }, [filteredSessions]); ``` This is low-impact for typical session counts but is inconsistent with the surrounding `useMemo` usage. --- ### ✅ Everything else looks good - **`validateSearch` split** — both layout and index route defining identical `session`/`repo` shapes is intentional belt-and-suspenders for TanStack Router type inference. The test coverage for the legacy `?spec=`/`?task=` drop is solid. - **`parseLeadingSlash` / `normaliseSpecArg`** — exported helpers with full unit tests. The regex `^(\/[a-zA-Z]+)(?:\s+([\s\S]*))?$` correctly rejects `/123` and bare `/`. Good. - **Streaming task id in component state** — the operator decision (no mid-turn reload reconnect) is clearly documented in both the hook and the route comment. - **`(navigate as any)` casts** — three occurrences, all with identical explanatory comments about the locale-prefix dynamic-params limitation. Acceptable and consistent. - **`biome-ignore` for `draftKey` deps** — the stale-closure reasoning in the comment is correct; including `draftKey` would clobber the new session's draft on switch. - **`SessionsSidebar`** — `<aside aria-label="Workspace sessions">` on desktop, plain `<div>` inside the dialog drawer for embedded mode. Correct. - **`SessionModelPicker`** — `sr-only` + `aria-hidden` label pattern is correct; screen readers announce "Model" without the colon. - **`DeleteSessionDialog`** — `tone="error"` on the destructive confirm button matches the design-system convention. - **i18n** — `workspace_sessions_search_placeholder` added to both `en.json` and `fr.json`. - **`ExportSessionMenu` reuse** — shared cleanly between toolbar and per-session sidebar rows. Consistent `testid` conventions. - **Composer-handoff store drain** — idempotent single-shot `consume()` correctly takes priority over `localStorage`. - **`specs.$specName.tsx`** — `repo: undefined` in the navigate call was already there before this PR; no regression. Fix the two items above (the cleanup is the blocker) and this is good to merge.

code-lead was assigned by charles 2026-05-13 16:20:18 +00:00

code-lead added 1 commit 2026-05-13 16:26:00 +00:00

fix(web): address review on /workspace chat-only refactor ... 598ceaa1df

- use-architect-chat-stream: add cleanup to the first useEffect so the
  2 s fallback setTimeout is cleared on unmount (and on deps change).
  Without it the timer leaks and, in strict-mode double-invocation,
  fires a state update on an unmounted component.
- sessions-sidebar: wrap the date-grouped session map in useMemo so it
  is not rebuilt on hover-driven re-renders. Matches the surrounding
  useMemo usage for filteredSessions.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

code-lead commented 2026-05-13 16:26:13 +00:00

Author

Collaborator

Copy link

Both review items addressed in 598ceaa:

Bug fix — use-architect-chat-stream.ts cleanup
Added the suggested return from the first useEffect so the 2 s fallback setTimeout is cleared on unmount (and on dep changes, which is fine — wasStreamingRef is already nulled after the first entry into the done branch, so the timer is never re-set on a stale render).

return () => {
  if (fallbackTimerRef.current !== null) {
    clearTimeout(fallbackTimerRef.current);
    fallbackTimerRef.current = null;
  }
};

Perf nit — grouped map in sessions-sidebar.tsx
Wrapped in useMemo([filteredSessions]) as suggested. Matches the surrounding pattern.

just qa green (3305 server tests, 1139 web tests).

Both review items addressed in 598ceaa: **Bug fix — `use-architect-chat-stream.ts` cleanup** Added the suggested `return` from the first `useEffect` so the 2 s fallback `setTimeout` is cleared on unmount (and on dep changes, which is fine — `wasStreamingRef` is already nulled after the first entry into the `done` branch, so the timer is never re-set on a stale render). ```ts return () => { if (fallbackTimerRef.current !== null) { clearTimeout(fallbackTimerRef.current); fallbackTimerRef.current = null; } }; ``` **Perf nit — `grouped` map in `sessions-sidebar.tsx`** Wrapped in `useMemo([filteredSessions])` as suggested. Matches the surrounding pattern. `just qa` green (3305 server tests, 1139 web tests).

code-lead requested review from reviewer 2026-05-13 16:32:02 +00:00

reviewer approved these changes 2026-05-13 16:34:41 +00:00

reviewer left a comment

Copy link

Review — refactor(web): /workspace chat-only (#1121)

CI ✅ green. 29 unit tests pass. Full diff read.

What this does

workspace.index.tsx shrinks from 1 856 → 677 lines by:

• Dropping the spec mode entirely (SpecEditor, NewSpec dialog, SaveAsSpec dialog, Breakdown drawer wired to ?spec=, spec section in the sidebar and command palette).
• Extracting six components to components/workspace/* and the streaming lifecycle to lib/use-architect-chat-stream.ts.
• Replacing ?task= URL state with component state (operator decision documented).
• Introducing parseLeadingSlash + normaliseSpecArg helpers with /spec → navigate and /breakdown → drawer interception.

Positives

Decomposition is clean. Each extracted component has one clear job and a self-contained props interface. The route file now reads as data-flow + routing; the markup moved where it belongs.

promoteToSession + toastError + invalidateSession eliminate the three-way duplication that existed across sendMutation / regenerateMutation / editMutation success/error handlers. Good DRY.

useArchitectChatStream is a faithful lift. Both effects, both refs, the 2 s fallback timer, and the cleanup on unmount are all preserved. The wasStreamingRef → expectingRefetchRef → baselineMsgsRef dance is identical to the original.

URL contract is double-gated correctly. Both workspace.tsx (layout) and workspace.index.tsx (index) have matching validateSearch that omit spec and task, so old bookmarks are stripped before any component code runs. Belt-and-suspenders but explicit.

Test rewrite is comprehensive. Unit tests for parseLeadingSlash and normaliseSpecArg are a good choice (regression on those helpers silently routes /spec//breakdown to the architect as prose). Component-level tests cover the /spec → navigate, /breakdown → drawer, pass-through, composer-handoff, and ⌘K paths.

grouped is now useMemo in SessionsSidebar (the old WorkspaceSidebar recomputed it on every render). Improvement.

Observations (non-blocking)

/breakdown doesn't clear the composer draft on success. /spec auth calls setDraft("") + localStorage.removeItem(draftKey) before navigating. /breakdown specs/auth.md opens the drawer but leaves /breakdown specs/auth.md in the textarea. The breakdown drawer is additive (not navigational) so this may be intentional — the user might want to type a follow-up. Worth a conscious decision, but it's a product call, not a bug.

buildSpecScaffoldFromTranscript is gone without a migration. The export + tests were removed; the "Save as spec…" flow is gone, so this is correct. Just confirming the /specs route owns its own scaffolding if it ever needs one — nothing broken here.

files query lost its explicit generic annotation (useQuery instead of useQuery<ArchitectFileEntry[]>). Type is still inferred from fetchArchitectFiles's return type; no runtime impact, just a style note.

Verdict

APPROVED. The decomposition is solid, the streaming hook extraction is correct, the URL-contract change is clearly documented and tested, and the new test file is better than what it replaces. The one behavioural observation (/breakdown draft retention) appears intentional given the drawer-vs-navigation distinction.

## Review — refactor(web): /workspace chat-only (#1121) CI ✅ green. 29 unit tests pass. Full diff read. ### What this does `workspace.index.tsx` shrinks from 1 856 → 677 lines by: - Dropping the `spec` mode entirely (SpecEditor, NewSpec dialog, SaveAsSpec dialog, Breakdown drawer wired to `?spec=`, spec section in the sidebar and command palette). - Extracting six components to `components/workspace/*` and the streaming lifecycle to `lib/use-architect-chat-stream.ts`. - Replacing `?task=` URL state with component state (operator decision documented). - Introducing `parseLeadingSlash` + `normaliseSpecArg` helpers with `/spec` → navigate and `/breakdown` → drawer interception. ### Positives **Decomposition is clean.** Each extracted component has one clear job and a self-contained props interface. The route file now reads as data-flow + routing; the markup moved where it belongs. **`promoteToSession` + `toastError` + `invalidateSession`** eliminate the three-way duplication that existed across `sendMutation` / `regenerateMutation` / `editMutation` success/error handlers. Good DRY. **`useArchitectChatStream` is a faithful lift.** Both effects, both refs, the 2 s fallback timer, and the cleanup on unmount are all preserved. The `wasStreamingRef` → `expectingRefetchRef` → `baselineMsgsRef` dance is identical to the original. **URL contract is double-gated correctly.** Both `workspace.tsx` (layout) and `workspace.index.tsx` (index) have matching `validateSearch` that omit `spec` and `task`, so old bookmarks are stripped before any component code runs. Belt-and-suspenders but explicit. **Test rewrite is comprehensive.** Unit tests for `parseLeadingSlash` and `normaliseSpecArg` are a good choice (regression on those helpers silently routes `/spec`/`/breakdown` to the architect as prose). Component-level tests cover the `/spec → navigate`, `/breakdown → drawer`, pass-through, composer-handoff, and ⌘K paths. **`grouped` is now `useMemo`** in `SessionsSidebar` (the old `WorkspaceSidebar` recomputed it on every render). Improvement. ### Observations (non-blocking) **`/breakdown` doesn't clear the composer draft on success.** `/spec auth` calls `setDraft("")` + `localStorage.removeItem(draftKey)` before navigating. `/breakdown specs/auth.md` opens the drawer but leaves `/breakdown specs/auth.md` in the textarea. The breakdown drawer is additive (not navigational) so this may be intentional — the user might want to type a follow-up. Worth a conscious decision, but it's a product call, not a bug. **`buildSpecScaffoldFromTranscript` is gone without a migration.** The export + tests were removed; the "Save as spec…" flow is gone, so this is correct. Just confirming the `/specs` route owns its own scaffolding if it ever needs one — nothing broken here. **`files` query lost its explicit generic annotation** (`useQuery` instead of `useQuery<ArchitectFileEntry[]>`). Type is still inferred from `fetchArchitectFiles`'s return type; no runtime impact, just a style note. ### Verdict APPROVED. The decomposition is solid, the streaming hook extraction is correct, the URL-contract change is clearly documented and tested, and the new test file is better than what it replaces. The one behavioural observation (`/breakdown` draft retention) appears intentional given the drawer-vs-navigation distinction.

reviewer merged commit 95d184641d into main 2026-05-13 16:34:55 +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

reviewer

code-lead

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!1129

No description provided.