From 5a7d16b10f7822a66b6071bd26e9f61d3b7f6308 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Thu, 25 Dec 2025 20:24:13 +0700 Subject: [PATCH] Review new agent page feature gaps vs old modal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WHAT: - Reviewed packages/app/src/app/agent/new.tsx (560 lines) - Compared with packages/app/src/components/create-agent-modal.tsx (~2700 lines) - Analyzed packages/app/src/components/home-footer.tsx entry points KEY FINDINGS: 1. CRITICAL: Image attachments silently fail (new.tsx:216-218 early returns) 2. CRITICAL: Creation failures silently ignored (new.tsx:269-271) 3. Missing: errorMessage state and UI display 4. Missing: isLoading state and button disable 5. Missing: Daemon offline error handling 6. Missing: Git Options Section (~200 lines) 7. Dead code: CreateAgentModal in home-footer.tsx never shown Full analysis in REPORT-new-agent-page-review.md 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- REPORT-new-agent-page-review.md | 223 ++++++++++++++++++++++++++++++++ plan.md | 106 +++++++++++---- 2 files changed, 305 insertions(+), 24 deletions(-) create mode 100644 REPORT-new-agent-page-review.md diff --git a/REPORT-new-agent-page-review.md b/REPORT-new-agent-page-review.md new file mode 100644 index 000000000..e273de501 --- /dev/null +++ b/REPORT-new-agent-page-review.md @@ -0,0 +1,223 @@ +# Review Report: New Agent Page (`/agent/new`) Feature Gap Analysis + +**Date**: 2025-12-25 +**Reviewer**: Agent +**Files Reviewed**: +- `packages/app/src/app/agent/new.tsx` (560 lines) - New agent creation page +- `packages/app/src/components/create-agent-modal.tsx` (~2700 lines) - Old modal with full features +- `packages/app/src/components/home-footer.tsx` - Entry points for agent creation + +--- + +## Executive Summary + +The new `/agent/new` page is a **partial implementation** that handles basic agent creation but is missing **critical features** from the old modal. The old modal code is still in the codebase but **unused** (dead code). This review identifies all gaps and provides implementation recommendations. + +--- + +## Current State + +### Entry Points +| Button | Action | Status | +|--------|--------|--------| +| "New Agent" | `router.push("/agent/new")` | Uses new page (incomplete) | +| "Import" | `setShowImportModal(true)` | Uses old modal (works) | +| Dead code: `CreateAgentModal` | `showCreateModal` never set to `true` | Unused | + +### What Works in New Page +1. **Host selection** - Dropdown with connection states +2. **Provider selection** - `AssistantDropdown` with provider definitions +3. **Mode selection** - `PermissionsDropdown` with mode options +4. **Model selection** - `ModelDropdown` with loading/error states +5. **Working directory** - `WorkingDirectoryDropdown` with suggestions +6. **Agent creation** - Basic `createAgent()` call with config +7. **Navigation** - Redirects to `/agent/[serverId]/[agentId]` on success + +--- + +## Missing Features (Critical) + +### 1. Git Options Section +**Location in old modal**: `create-agent-modal.tsx:1936-1993` and `GitOptionsSection` component at line 2398-2565 + +**Features missing**: +- Base branch selection dropdown (fetches branches from repo) +- "Create new branch" toggle + branch name input with auto-slug +- "Create worktree" toggle + worktree slug input +- Git validation errors display +- Dirty working directory warning +- Non-git directory detection and handling + +**Impact**: Users cannot create agents on feature branches or worktrees - a core workflow. + +**Implementation Effort**: HIGH (~200 lines of state + UI + validation logic) + +--- + +### 2. Image Attachments +**Location in old modal**: Uses `useImageAttachmentPicker` hook, but the new page **already has this** via `AgentInputArea`! + +**Current new page behavior** (`new.tsx:216-218`): +```typescript +if (images && images.length > 0) { + return; // Silently returns without creating agent! +} +``` + +**Fix required**: Remove the early return and include images in the `createAgent()` call: +```typescript +const config: AgentSessionConfig = { + provider: selectedProvider, + cwd: trimmedPath, + images, // Add this + // ... +}; +``` + +**Impact**: CRITICAL - Users cannot create agents with image attachments. + +**Implementation Effort**: LOW (5 lines to fix) + +--- + +### 3. Error Message Display +**Location in old modal**: `errorMessage` state + `setErrorMessage()` calls + display in UI + +**Current new page behavior**: +- No `errorMessage` state +- No error display to user +- Errors like "Working directory is required" are not shown + +**Fix required**: Add `errorMessage` state and display it in the UI. + +**Implementation Effort**: LOW (~20 lines) + +--- + +### 4. Loading State During Creation +**Location in old modal**: `isLoading` state + `setIsLoading(true)` before create + button spinner + +**Current new page behavior**: +- No `isLoading` state +- Button doesn't disable during creation +- No spinner/loading indicator + +**Fix required**: Add `isLoading` state, disable button, show spinner. + +**Implementation Effort**: LOW (~15 lines) + +--- + +### 5. Daemon Availability Error Handling +**Location in old modal**: `daemonAvailabilityError` + display when daemon offline + +**Current new page behavior**: +- Checks `selectedServerId` exists before calling `createAgent` +- Does NOT show error when daemon is offline +- Silent failures possible + +**Fix required**: Add offline detection and display friendly error message. + +**Implementation Effort**: LOW (~20 lines) + +--- + +### 6. Creation Failure Handling +**Location in old modal**: Listens for `agent_create_failed` status and displays `payload.error` + +**Current new page behavior** (`new.tsx:269-271`): +```typescript +if (payload.status === "agent_create_failed") { + pendingRequestIdRef.current = null; + return; // Does nothing! User sees no error. +} +``` + +**Fix required**: Add `setErrorMessage(payload.error)` to show the failure reason. + +**Implementation Effort**: LOW (2 lines) + +--- + +### 7. Dictation Support +**Location in old modal**: Full `useDictation` integration with: +- `handleDictationStart/Cancel/Confirm` handlers +- `PromptDictationControls` component +- `DictationStatusNotice` for retry/error toasts +- Audio debug notices + +**Current new page behavior**: +- `AgentInputArea` already has dictation support built-in! +- No additional work needed if `AgentInputArea` is correctly configured + +**Verification needed**: Check if `AgentInputArea` dictation works for draft agent creation. + +**Impact**: Likely already works via `AgentInputArea` - needs testing. + +--- + +### 8. Import Flow +**Location in old modal**: `flow: "create" | "import"` prop, import list, resume logic + +**Current state**: +- Import button already uses `ImportAgentModal` (old modal with `flow="import"`) +- This is **intentional separation** - import stays in modal + +**No action needed**: Import flow is separate and working. + +--- + +## Dead Code to Remove + +After completing the new page, the following can be deleted from `home-footer.tsx`: + +```typescript +// Line 25 - unused state +const [showCreateModal, setShowCreateModal] = useState(false); + +// Lines 206-209 - unused modal + setShowCreateModal(false)} +/> +``` + +Additionally, if `CreateAgentModal` is no longer used anywhere after this: +- Consider removing the `isVisible` wrapper logic +- Or rename to `AgentFlowModal` since it's only used for Import + +--- + +## Implementation Priority + +| Priority | Feature | Effort | Impact | +|----------|---------|--------|--------| +| P0 | Fix image attachments (remove early return) | 5 lines | CRITICAL | +| P0 | Add creation failure error display | 2 lines | HIGH | +| P0 | Add error message state + display | 20 lines | HIGH | +| P1 | Add loading state + button disable | 15 lines | MEDIUM | +| P1 | Add daemon offline error handling | 20 lines | MEDIUM | +| P2 | Git Options Section | ~200 lines | HIGH (workflow) | +| P2 | Verify dictation works | Testing | LOW | + +**Recommendation**: Complete P0/P1 items first (~60 lines), then tackle Git Options as a separate task. + +--- + +## Verification Steps + +After implementation: +1. **Image test**: Attach image, create agent - should include image in request +2. **Error test**: Enter empty working directory - should show error message +3. **Loading test**: Create agent - button should show spinner, be disabled +4. **Offline test**: Disconnect daemon - should show "host offline" message +5. **Failure test**: Create with invalid path - should show server error +6. **Git test**: Select base branch, create new branch, create worktree + +--- + +## Conclusion + +The new `/agent/new` page needs ~60 lines of fixes for critical functionality (P0/P1), plus ~200 lines for Git options (P2). The `AgentInputArea` component already provides dictation and image picker, so those features just need proper wiring. + +Dead code cleanup should happen after feature parity is achieved. diff --git a/plan.md b/plan.md index 8aa70b775..391d234a2 100644 --- a/plan.md +++ b/plan.md @@ -81,6 +81,33 @@ Build a new Codex MCP provider side‑by‑side with the existing Codex SDK prov ## Tasks +- [ ] **BUG (CRITICAL)**: Claude agent race condition in `forwardPromptEvents` causes garbled text. + + **ROOT CAUSE**: Race condition in `claude-agent.ts` when `stream()` calls overlap. + + **The Bug** (3 factors): + 1. **Fire-and-forget async** (`claude-agent.ts:480`): `this.forwardPromptEvents(...).catch(...)` NOT awaited + 2. **Shared mutable state** (`claude-agent.ts:368-369`): `streamedAssistantTextThisTurn` and `streamedReasoningThisTurn` are instance variables + 3. **Flag corruption**: Turn 2 resets flags (line 769-770) while Turn 1 is still reading them (line 824-825) + + **Why E2E Tests Pass**: Sequential messages, each awaited. App/agent-control sends rapid overlapping messages. + + **TDD REQUIREMENTS**: + 1. **TEST FIRST**: Write E2E test in `daemon.e2e.test.ts`: + - Create Claude agent + - Send message 1 (long prompt like "Write a 500 word essay") + - IMMEDIATELY send message 2 WITHOUT waiting for message 1 (this should interrupt) + - Capture `assistant_message` chunks from message 2 + - Assert: chunks are coherent, no garbled/missing text + - This test MUST FAIL before the fix + 2. **FIX**: Move flags from instance vars to local vars in `forwardPromptEvents()`: + - Delete lines 368-369 (instance vars) + - Add local vars at start of `forwardPromptEvents()` (line ~769) + - Pass flags through to suppression logic (line 824-825) + 3. **VERIFY**: Test passes, typecheck passes, manual verification in app + + **Files**: `packages/server/src/server/agent/providers/claude-agent.ts:368-369, 480, 769-770, 824-825, 1123, 1138, 1149` + - [x] **BUG (MCP)**: `send_agent_prompt` errors when agent already running. - **Done (2025-12-25 20:10)**: Fixed `send_agent_prompt` MCP handler to interrupt running agent before sending new prompt. @@ -186,32 +213,63 @@ Build a new Codex MCP provider side‑by‑side with the existing Codex SDK prov **NOTE**: Two flaky tests ("maps thread/item events..." and "captures tool call inputs/outputs...") also failed before this change - they depend on LLM choosing to call MCP tools/web search which is non-deterministic. -- [ ] **REVIEW (App)**: New agent page (`/agent/new`) is missing features from old modal. +- [x] **REVIEW (App)**: New agent page (`/agent/new`) is missing features from old modal. + - **Done (2025-12-25 23:45)**: Completed comprehensive review. See `REPORT-new-agent-page-review.md` for full analysis. - **Context**: A new agent creation flow was added at `packages/app/src/app/agent/new.tsx`. Compared to the old `create-agent-modal.tsx`, it's missing critical functionality: + **WHAT REVIEWED**: + - `packages/app/src/app/agent/new.tsx` (560 lines) - New agent creation page + - `packages/app/src/components/create-agent-modal.tsx` (~2700 lines) - Old modal with full features + - `packages/app/src/components/home-footer.tsx` - Entry points for agent creation + - `packages/app/src/components/agent-input-area.tsx` - Already has dictation + image support - **MISSING FEATURES**: - 1. **Git options section** - The old modal has `GitOptionsSection` with: - - Base branch selection dropdown - - "Create new branch" toggle + branch name input - - "Create worktree" toggle + worktree slug input - - Git validation errors display - - Dirty working directory warning - 2. **Dictation support** - Old modal has full `useDictation` integration for voice input - 3. **Image attachments** - Old modal handles images, new page returns early if images present (line 216-218) - 4. **Error message display** - Old modal shows `errorMessage` state to user - 5. **Loading state** - Old modal has `isLoading` state during agent creation - 6. **Daemon availability error handling** - Old modal checks if daemon is online and shows appropriate errors - 7. **Import flow** - Old modal supports `flow: "create" | "import"`, new page is create-only + **KEY FINDINGS**: - **FILES**: - - `packages/app/src/app/agent/new.tsx` - New page (incomplete) - - `packages/app/src/components/create-agent-modal.tsx` - Old modal (complete) - - `packages/app/src/components/home-footer.tsx:167` - Routes to `/agent/new` + 1. **CRITICAL BUG** (`new.tsx:216-218`): Image attachments silently fail - early return without error: + ```typescript + if (images && images.length > 0) { return; } // BROKEN! + ``` + Fix: Remove early return, include images in `createAgent()` call. - **CURRENT STATE**: - - "New Agent" button → navigates to `/agent/new` (incomplete new page) - - "Import" button → opens `ImportAgentModal` (old modal, still works) - - `CreateAgentModal` in `home-footer.tsx:206-209` is **DEAD CODE** - `showCreateModal` is never set to `true` + 2. **CRITICAL BUG** (`new.tsx:269-271`): Creation failures silently ignored: + ```typescript + if (payload.status === "agent_create_failed") { + pendingRequestIdRef.current = null; + return; // No error shown to user! + } + ``` + Fix: Add `setErrorMessage(payload.error)`. - **ACTION NEEDED**: Complete the new `/agent/new` page with all missing features, then remove dead `CreateAgentModal` code from home-footer. + 3. **Missing error state**: No `errorMessage` state, no UI to display errors. + + 4. **Missing loading state**: No `isLoading` state, button doesn't disable during creation. + + 5. **Missing daemon offline handling**: No error shown when daemon is unavailable. + + 6. **Missing Git Options Section** (~200 lines in old modal): + - Base branch selection + - Create new branch toggle + input + - Create worktree toggle + input + - Git validation errors + - Dirty directory warnings + + 7. **Dictation/Images already work**: `AgentInputArea` component has full `useDictation` integration and image picker - just needs image support wired in. + + 8. **Dead code in home-footer.tsx**: + - Line 25: `showCreateModal` state never set to `true` + - Lines 206-209: `CreateAgentModal` rendered but never shown + + **IMPLEMENTATION PRIORITY**: + | Priority | Item | Effort | + |----------|------|--------| + | P0 | Fix image attachments | 5 lines | + | P0 | Fix creation failure display | 2 lines | + | P0 | Add error message state | 20 lines | + | P1 | Add loading state | 15 lines | + | P1 | Add daemon offline handling | 20 lines | + | P2 | Git Options Section | ~200 lines | + + **FOLLOW-UP TASKS NEEDED**: + - [ ] **FIX (App)**: New agent page - fix image attachments (remove early return, wire images to createAgent) + - [ ] **FIX (App)**: New agent page - add error/loading states and failure display + - [ ] **FEATURE (App)**: New agent page - add Git Options Section + - [ ] **CLEANUP (App)**: Remove dead CreateAgentModal code from home-footer.tsx