Skip to content

Conversation

@connor-tyndall
Copy link
Contributor

@connor-tyndall connor-tyndall commented Jan 9, 2026

Summary

Addresses #25 - Edit Existing Features in The UI

This PR adds the ability to edit and delete features through both the UI and assistant chat:

  • UI Edit Button: Feature modal now shows Edit/Delete buttons for all features (pending and completed)
  • Assistant Chat: Commands like "Update feature 25 description to..." or "Delete feature 123"
  • Completed Feature Workflow: For completed features, deletion removes from tracking only; the assistant suggests creating a "removal" feature if code deletion is also needed

Changes

Backend

  • Add FeatureUpdate schema for partial updates
  • Add PATCH /features/{id} endpoint
  • Add feature_create, feature_update, feature_delete MCP tools
  • Enable feature management tools in assistant chat with updated system prompt

Frontend

  • Add Edit mode to FeatureModal with form for category, name, description, steps
  • Allow Edit/Delete on completed features (previously only pending)
  • Add useUpdateFeature mutation hook
  • Add friendly tool descriptions in assistant chat (e.g., "Updating feature Edit Existing Features in The UI #25")

Test plan

  • Click a pending feature → verify Edit/Skip/Delete buttons appear
  • Click a completed feature → verify Edit/Delete buttons appear (no Skip)
  • Edit a feature → modify fields → Save → verify changes persist
  • Delete a completed feature → verify confirmation shows "code remains" note
  • Open assistant chat → say "Update feature 1 description to test" → verify it works
  • Open assistant chat → say "Delete feature 2" → verify deletion and helpful response

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • In-app feature editing and deletion: edit category, name, description, and test steps with add/remove, validation, save/cancel, and delete confirmation.
    • API and client support for partial feature updates and deletions; assistant can now update and delete features via chat.
  • Bug Fixes

    • Improved validation and clearer error messages for update/create/delete flows.
  • Documentation

    • README expanded with Project Assistant, feature-management workflows, and registry guidance.

✏️ Tip: You can customize this high-level summary in your review settings.

Addresses leonvanzyl#25 - Edit Existing Features in The UI

## Backend
- Add FeatureUpdate schema for partial updates (PATCH)
- Add PATCH endpoint for updating features
- Add feature_create, feature_update, feature_delete MCP tools
- Enable feature management tools in assistant chat

## Frontend
- Add Edit mode to FeatureModal with form for category, name, description, steps
- Allow Edit/Delete on completed features (not just pending)
- Add useUpdateFeature mutation hook
- Add friendly tool descriptions in assistant chat

## Workflow
- UI: Click feature → Edit/Delete buttons for all features
- Assistant: Commands like "Update feature 25 description to..." or "Delete feature 123"
- For completed features, deletion removes from tracking only; suggests creating
  a "removal" feature if code deletion is also needed

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@coderabbitai
Copy link

coderabbitai bot commented Jan 9, 2026

📝 Walkthrough

Walkthrough

Adds partial-update feature support end-to-end: new FeatureUpdate schema, MCP tools feature_update/feature_delete (duplicated in the module), a PATCH router for partial updates (duplicate handler present), frontend API/hook and UI edit flow, and assistant tooling exposing update/delete.

Changes

Cohort / File(s) Summary
Backend Schema & Routing
server/schemas.py, server/routers/features.py
Adds FeatureUpdate (all fields optional) and a PATCH /projects/{project_name}/features/{feature_id} endpoint that applies only provided fields. A duplicate PATCH handler for the same path is present.
Backend MCP Tools
mcp_server/feature_mcp.py
Adds feature_update and feature_delete tools (validation, DB session handling, JSON responses). Both functions are duplicated in the file, resulting in redefinitions.
Assistant Service
server/services/assistant_chat_session.py
Expands FEATURE_MANAGEMENT_TOOLS to include mcp__features__feature_update and mcp__features__feature_delete, and updates prompts/guidance to reference update/delete capabilities.
Frontend API & Types
ui/src/lib/api.ts, ui/src/lib/types.ts
Exports new FeatureUpdate type and updateFeature(projectName, featureId, update: FeatureUpdate) API; formatting/type literal adjustments elsewhere.
Frontend Hooks
ui/src/hooks/useProjects.ts, ui/src/hooks/useAssistantChat.ts
Adds useUpdateFeature(projectName) hook (PATCH mutation, invalidation). useAssistantChat gains tool-call description helpers and more robust WebSocket lifecycle/streaming handling.
Frontend Component
ui/src/components/FeatureModal.tsx
Reworks FeatureModal to support edit mode (category, name, description, steps), dynamic step add/remove, validation, save/cancel/delete flows, and integration with update hook.
Docs
README.md
Documents Project Assistant and new feature management commands; adds registry concept and updates examples/project structure.

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant UI as FeatureModal (UI)
    participant Hook as useUpdateFeature (Client Hook)
    participant API as ui API (updateFeature)
    participant Server as Backend Router
    participant DB as Database

    User->>UI: Open modal, edit fields
    UI->>UI: Validate input
    User->>UI: Click Save
    UI->>Hook: mutate({ featureId, update })
    Hook->>API: PATCH /projects/{projectName}/features/{featureId}
    API->>Server: Forward FeatureUpdate payload
    Server->>Server: Validate project & feature, filter provided fields
    Server->>DB: Apply updates
    DB-->>Server: Persist confirmation
    Server-->>API: Return updated FeatureResponse
    API-->>Hook: Resolve with updated feature
    Hook->>Hook: Invalidate queries (features list)
    Hook-->>UI: Return updated feature
    UI->>User: Render updated feature
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Poem

🐇 I hopped through code with eager paws,
Poked at fields and fixed the flaws,
Patch a name, delete a tale,
DB nods as tests set sail,
A rabbit cheers: the backlog claws.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 32.84% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding feature editing and deletion capabilities across the codebase.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e8c45a9 and 02988b8.

📒 Files selected for processing (7)
  • mcp_server/feature_mcp.py
  • server/routers/features.py
  • server/schemas.py
  • server/services/assistant_chat_session.py
  • ui/src/hooks/useAssistantChat.ts
  • ui/src/lib/api.ts
  • ui/src/lib/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • server/schemas.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Use SQLite with SQLAlchemy ORM for database access and feature management

Files:

  • server/services/assistant_chat_session.py
  • server/routers/features.py
  • mcp_server/feature_mcp.py
ui/src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

ui/src/**/*.{ts,tsx}: Use TypeScript for React components in the UI (React 18 with TypeScript)
Use React Query (TanStack Query) for API calls and data fetching in the UI
Use Radix UI for component primitives in the React UI
Run ESLint to lint React UI code

Files:

  • ui/src/hooks/useAssistantChat.ts
  • ui/src/lib/types.ts
  • ui/src/lib/api.ts
server/routers/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Use FastAPI for REST API endpoints in the server/routers/ directory

Files:

  • server/routers/features.py
mcp_server/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Use MCP (Model Context Protocol) servers for agent tools: feature_mcp.py for feature management, with tools: feature_get_stats, feature_get_next, feature_get_for_regression, feature_mark_passing, feature_skip, feature_create_bulk

Files:

  • mcp_server/feature_mcp.py
🧠 Learnings (1)
📚 Learning: 2026-01-10T08:23:04.012Z
Learnt from: CR
Repo: leonvanzyl/autocoder PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T08:23:04.012Z
Learning: Applies to mcp_server/**/*.py : Use MCP (Model Context Protocol) servers for agent tools: feature_mcp.py for feature management, with tools: feature_get_stats, feature_get_next, feature_get_for_regression, feature_mark_passing, feature_skip, feature_create_bulk

Applied to files:

  • server/services/assistant_chat_session.py
  • mcp_server/feature_mcp.py
🧬 Code graph analysis (3)
server/routers/features.py (6)
ui/src/lib/types.ts (2)
  • FeatureUpdate (85-90)
  • Feature (60-69)
server/schemas.py (2)
  • FeatureUpdate (90-95)
  • FeatureResponse (98-106)
server/routers/projects.py (1)
  • validate_project_name (78-85)
server/utils/validation.py (1)
  • validate_project_name (10-28)
server/routers/expand_project.py (1)
  • _get_project_path (35-43)
api/database.py (1)
  • Feature (19-44)
ui/src/lib/types.ts (1)
server/schemas.py (22)
  • ProjectSummary (42-47)
  • ProjectStats (34-39)
  • ProjectDetail (50-56)
  • DriveInfo (242-246)
  • DirectoryEntry (249-256)
  • DirectoryListResponse (259-264)
  • PathValidationResponse (267-274)
  • ProjectPrompts (59-63)
  • FeatureListResponse (109-113)
  • FeatureCreate (85-87)
  • FeatureUpdate (90-95)
  • AgentStatus (146-152)
  • AgentActionResponse (155-159)
  • SetupStatus (166-171)
  • WSProgressMessage (178-183)
  • WSFeatureUpdateMessage (186-190)
  • WSLogMessage (193-197)
  • WSAgentStatusMessage (200-203)
  • ImageAttachment (214-235)
  • ModelsResponse (302-305)
  • ModelInfo (290-293)
  • SettingsUpdate (308-318)
mcp_server/feature_mcp.py (4)
server/services/assistant_chat_session.py (1)
  • get_session (399-402)
server/services/spec_chat_session.py (1)
  • get_session (441-444)
ui/src/lib/types.ts (1)
  • Feature (60-69)
api/database.py (2)
  • Feature (19-44)
  • to_dict (33-44)
🔇 Additional comments (18)
ui/src/lib/types.ts (1)

85-90: LGTM! FeatureUpdate interface aligns with server schema.

The interface correctly defines all fields as optional to support partial updates, matching the server-side FeatureUpdate schema.

server/routers/features.py (1)

293-338: LGTM! Well-structured update endpoint with proper validation.

The endpoint correctly implements partial updates using exclude_unset=True, validates input, and follows the same error-handling and session-management patterns as other endpoints in this router.

mcp_server/feature_mcp.py (2)

481-544: LGTM! Well-implemented feature_update tool.

The implementation correctly handles partial updates by collecting only non-None parameters, validates input appropriately, and follows the established pattern of other MCP tools in this file.


546-593: LGTM! feature_delete tool with helpful UX for completed features.

The implementation correctly handles feature deletion and includes a thoughtful note for completed features, reminding users that the code remains and suggesting a new feature if code removal is needed.

server/services/assistant_chat_session.py (3)

54-61: LGTM! Tool list correctly expanded with update and delete capabilities.

The updated FEATURE_MANAGEMENT_TOOLS list appropriately includes the new feature_update and feature_delete tools while maintaining the separation of mark_passing as an agent-only operation.


90-138: LGTM! System prompt clearly communicates new capabilities.

The updated system prompt effectively describes the assistant's new update and delete capabilities, with clear and concise tool descriptions.


155-180: LGTM! Excellent documentation with clear examples.

The new sections provide helpful guidance and examples for updating and deleting features. The note about completed features is particularly valuable, ensuring users understand that code remains after deletion and can create a removal feature if needed.

ui/src/lib/api.ts (1)

144-156: LGTM! updateFeature function correctly implemented.

The function follows the established pattern of other API functions, uses proper URL encoding, and correctly sends a PATCH request with the FeatureUpdate payload.

ui/src/hooks/useAssistantChat.ts (10)

30-45: LGTM!

These type-safe helper functions properly handle unknown input types and provide sensible fallbacks. Good defensive programming for handling dynamic tool inputs.


47-83: LGTM!

The centralized tool description function improves maintainability and provides a consistent user-friendly experience. The prefix stripping logic elegantly handles both mcp__features__* and direct tool names.


101-115: LGTM!

The connectTimeoutRef cleanup ensures pending connection timeouts are properly cleared on unmount, preventing potential memory leaks and state updates on unmounted components.


141-159: LGTM!

The connection identity guards (wsRef.current !== ws) prevent race conditions where callbacks from stale WebSocket instances could incorrectly update state. Clearing the previous ping interval before starting a new one prevents accumulating multiple intervals.


161-180: LGTM!

The stale connection guard and exponential backoff reconnection logic are well-implemented. The backoff delays (2s, 4s, 8s capped at 10s) provide reasonable retry behavior without overwhelming the server.


182-188: LGTM!

The stale connection guard ensures error events from previous connection attempts don't incorrectly update the current connection status.


229-244: LGTM!

The tool call handling cleanly integrates with getToolDescription to provide user-friendly feedback. The type cast to Record<string, unknown> is appropriate for the dynamic nature of tool inputs from the WebSocket.


252-274: LGTM!

The reverse search correctly handles scenarios where tool_call/system messages are interleaved after a streaming assistant message. The immutable update pattern with spread operators is appropriate for React state updates.


304-353: LGTM!

The retry-limited polling mechanism (50 × 100ms = 5s max) prevents infinite loops while allowing sufficient time for connection establishment. All exit paths properly clear the timeout ref and provide meaningful error feedback.


386-414: LGTM!

The disconnect function comprehensively cleans up all resources: pending connection polling, reconnection attempts, keep-alive pings, and the WebSocket itself. Setting reconnectAttempts to max before cleanup correctly prevents the onclose handler from scheduling unwanted reconnections.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @ui/src/hooks/useAssistantChat.ts:
- Around line 263-287: The checkAndSend polling in the start callback can spin
forever while wsRef.current.readyState stays CONNECTING; add a retry limit or
overall timeout to stop polling and handle failure: in the start function
(inside useAssistantChat) augment checkAndSend to track attempts or a deadline
(e.g., maxRetries or startTime + timeoutMs), stop retrying when exceeded, clear
any pending timers, setIsLoading(false) and surface an error (or call an onError
handler) when the socket never opens; ensure you still handle the OPEN and
CONNECTING cases and that wsRef, connect, and setConversationId logic remain
unchanged.
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a195d6d and bbccaf1.

📒 Files selected for processing (9)
  • mcp_server/feature_mcp.py
  • server/routers/features.py
  • server/schemas.py
  • server/services/assistant_chat_session.py
  • ui/src/components/FeatureModal.tsx
  • ui/src/hooks/useAssistantChat.ts
  • ui/src/hooks/useProjects.ts
  • ui/src/lib/api.ts
  • ui/src/lib/types.ts
🧰 Additional context used
🧬 Code graph analysis (6)
ui/src/hooks/useProjects.ts (3)
server/schemas.py (3)
  • FeatureCreate (85-87)
  • FeatureUpdate (90-95)
  • SettingsUpdate (296-306)
ui/src/lib/types.ts (4)
  • FeatureCreate (77-83)
  • FeatureUpdate (85-90)
  • Settings (326-329)
  • SettingsUpdate (331-334)
registry.py (1)
  • Settings (90-96)
ui/src/hooks/useAssistantChat.ts (1)
ui/src/lib/types.ts (2)
  • ChatMessage (236-244)
  • AssistantChatServerMessage (304-310)
server/schemas.py (1)
ui/src/lib/types.ts (1)
  • FeatureUpdate (85-90)
server/routers/features.py (2)
server/schemas.py (2)
  • FeatureUpdate (90-95)
  • FeatureResponse (98-106)
api/database.py (1)
  • Feature (19-44)
ui/src/components/FeatureModal.tsx (3)
ui/src/lib/types.ts (1)
  • Feature (60-69)
ui/src/lib/api.ts (3)
  • skipFeature (130-140)
  • deleteFeature (118-128)
  • updateFeature (142-154)
ui/src/hooks/useProjects.ts (3)
  • useSkipFeature (101-110)
  • useDeleteFeature (89-99)
  • useUpdateFeature (112-127)
mcp_server/feature_mcp.py (2)
ui/src/lib/types.ts (1)
  • Feature (60-69)
api/database.py (2)
  • Feature (19-44)
  • to_dict (33-44)
🔇 Additional comments (17)
server/schemas.py (1)

90-96: LGTM!

The FeatureUpdate schema is well-designed for partial updates with appropriate validation constraints. The optional fields with min_length validators ensure that when provided, values are non-empty, which aligns with the corresponding TypeScript interface in ui/src/lib/types.ts.

ui/src/lib/types.ts (1)

85-90: LGTM!

The FeatureUpdate interface correctly defines optional fields for partial updates, maintaining consistency with the backend FeatureUpdate schema in server/schemas.py.

server/services/assistant_chat_session.py (2)

39-49: LGTM!

The separation of feature management tools from read-only tools is clean, and combining them into ASSISTANT_FEATURE_TOOLS provides a clear, maintainable configuration for the assistant's capabilities.


75-167: Well-structured system prompt with clear capability boundaries.

The updated system prompt clearly distinguishes between codebase analysis (read-only) and feature management (CRUD) capabilities. The example interactions for updating and deleting features provide good guidance for the assistant.

ui/src/hooks/useAssistantChat.ts (1)

30-66: LGTM!

The getToolDescription helper provides user-friendly descriptions for tool calls, improving the UX by displaying meaningful messages instead of raw tool identifiers.

server/routers/features.py (1)

301-345: LGTM!

The update_feature endpoint correctly implements partial updates using model_dump(exclude_unset=True), which distinguishes between fields that were not provided vs. explicitly set to None. The validation and error handling patterns are consistent with other endpoints in the file.

mcp_server/feature_mcp.py (3)

418-466: LGTM!

The feature_create tool is well-implemented with proper priority calculation, input validation via Pydantic Field annotations, and consistent error handling with rollback on failure.


469-531: LGTM!

The feature_update tool correctly handles partial updates by only applying provided fields. The validation for "no fields to update" case and the consistent response format with success/message/feature are well done.


534-580: LGTM!

The feature_delete tool includes a thoughtful UX detail: when deleting a completed feature, it adds a note explaining that the code remains and suggests creating a removal feature if needed. This aligns well with the PR objectives.

ui/src/components/FeatureModal.tsx (3)

37-73: LGTM!

The edit mode state management is well-structured. Using useId() to generate stable step IDs is a good approach, and the useEffect cleanup ensures the form resets properly when exiting edit mode or when the feature prop changes.


114-136: Verify: Clearing all steps is not possible with current logic.

When filteredSteps is empty (all steps removed or empty), steps is set to undefined which means "don't update". This prevents validation errors (backend requires min_length=1), but users cannot intentionally remove all steps from a feature.

If this is the intended behavior, consider adding a hint in the UI when the user tries to remove the last step. If clearing steps should be supported, the backend validation would need adjustment.


396-446: LGTM!

The action buttons correctly differentiate between completed features (Edit/Delete) and pending features (Edit/Skip/Delete). The conditional rendering and loading states are properly handled, and the delete confirmation flow includes a helpful note about code remaining for completed features.

ui/src/hooks/useProjects.ts (3)

5-13: LGTM! Import changes support new feature update capability.

The addition of FeatureUpdate to the type imports is necessary for the new useUpdateFeature hook and aligns with the backend schema.


112-127: LGTM! Well-structured hook following established patterns.

The useUpdateFeature hook correctly:

  • Accepts a properly typed FeatureUpdate payload
  • Calls the corresponding API function
  • Invalidates the features query cache on success
  • Follows the same pattern as other feature mutation hooks

34-51: All callers have been properly updated to use the new object syntax.

The mutationFn signature change to destructured parameters has been correctly implemented. Both calls in NewProjectModal.tsx (lines 92 and 108) pass an object with the expected properties {name, path, specMethod}, which matches the hook's new signature.

ui/src/lib/api.ts (2)

27-44: LGTM! Improved error handling prevents unhandled rejections.

The addition of .catch() when parsing error responses (line 37-39) provides a defensive fallback when the server returns a non-JSON error response, improving reliability.


142-154: LGTM! Correctly implements partial feature updates.

The updateFeature function:

  • Uses PATCH method appropriate for partial updates
  • Accepts FeatureUpdate with optional fields
  • Returns the updated Feature object
  • Follows the same URL pattern as other feature endpoints

Addresses CodeRabbit AI feedback - the checkAndSend function could
poll indefinitely if the WebSocket never opened. Added:

- Maximum 50 retries (5 seconds max wait)
- Proper error handling for timeout case
- Error handling for closed/error WebSocket states
- Added onError to useCallback dependency array

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Fix all issues with AI agents
In @ui/src/hooks/useAssistantChat.ts:
- Around line 100-152: Old WebSocket handlers can act on stale connections
because wsRef.current is overwritten; capture the newly created socket in a
local constant (the existing ws variable) and in each handler (ws.onopen,
ws.onclose, ws.onerror, ping interval logic) first verify that wsRef.current ===
ws before mutating shared refs (pingIntervalRef, reconnectTimeoutRef,
reconnectAttempts) or calling setConnectionStatus/onError; also ensure you clear
any previous ping interval before starting a new one and only schedule
reconnects from the handler when the closing socket is the current one (use
maxReconnectAttempts/reconnectAttempts as before).
- Around line 215-233: The response_done handler can leave an assistant message
stuck with isStreaming: true when the last message is a tool_call/system
message; update the setMessages logic in the "response_done" case to find the
most recent message with role === "assistant" and isStreaming === true (not just
prev[prev.length-1]) and set its isStreaming to false; modify the setMessages
callback used in the response_done case to locate that latest streaming
assistant message by scanning prev from the end and replace it with {
...message, isStreaming: false } while preserving other messages.
- Around line 335-347: The disconnect() function fails to clear a scheduled
reconnect, so a pending reconnectTimeoutRef.current may still fire and call
connect() after a user-initiated disconnect; update disconnect() to clear any
pending timeout by checking reconnectTimeoutRef.current, calling
clearTimeout(reconnectTimeoutRef.current), and setting
reconnectTimeoutRef.current = null in the same cleanup block that clears
pingIntervalRef and closes wsRef, while still setting reconnectAttempts.current
= maxReconnectAttempts and setConnectionStatus("disconnected").
- Around line 30-66: getToolDescription reads properties from input: name,
feature_id, features, file_path and pattern which are typed as unknown and can
be undefined, causing strict-TS errors and "#undefined" outputs; update each
switch case to safely narrow and guard those properties before interpolation
(e.g., use typeof input.name === "string" ? input.name : "new feature"; typeof
input.feature_id === "number" || typeof input.feature_id === "string" ?
input.feature_id : "unknown"; Array.isArray(input.features) ?
input.features.length : "multiple"; typeof input.file_path === "string" ?
input.file_path : "file"; typeof input.pattern === "string" ? input.pattern :
"pattern"), or use small local type-guard helpers, and keep the existing
Array.isArray check for features to avoid unsafe property access.
🧹 Nitpick comments (1)
ui/src/hooks/useAssistantChat.ts (1)

263-302: Tighten start() polling + conversation id check.

  • if (existingConversationId) skips valid IDs like 0; safer is existingConversationId != null.
  • Consider storing/clearing the polling timeout to avoid multiple overlapping start() calls.
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bbccaf1 and ca52497.

📒 Files selected for processing (1)
  • ui/src/hooks/useAssistantChat.ts

- Add type-safe helpers (getStringValue, getFeatureId) for tool description
  to prevent "#undefined" outputs and strict-TS errors

- Fix stale WebSocket handler issue: handlers now verify wsRef.current === ws
  before mutating shared refs; clear previous ping interval before starting new

- Fix response_done to find most recent streaming assistant message by scanning
  from end (not just last message), prevents stuck isStreaming when tool_call
  follows assistant text

- Fix disconnect() to clear pending reconnect timeout, preventing reconnect
  after user-initiated disconnect

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @ui/src/hooks/useAssistantChat.ts:
- Around line 297-336: The polling in start (checkAndSend) uses setTimeout
recursively but never stores the timeout ID, so timeouts keep firing if the
component unmounts; fix by adding a ref (e.g., connectTimeoutRef) to store the
ID returned by setTimeout whenever you call setTimeout in checkAndSend/start,
clear that timeout in your component's cleanup effect (and when you hit the
maxRetries or succeed) and ensure you null out the ref after clearing so no
stale timers remain; update references to wsRef, start, and checkAndSend to use
this timeout-ref-based cancellation.
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ca52497 and 8165d5d.

📒 Files selected for processing (1)
  • ui/src/hooks/useAssistantChat.ts
🧰 Additional context used
🧬 Code graph analysis (1)
ui/src/hooks/useAssistantChat.ts (1)
ui/src/lib/types.ts (2)
  • ChatMessage (236-244)
  • AssistantChatServerMessage (304-310)
🔇 Additional comments (13)
ui/src/hooks/useAssistantChat.ts (13)

1-24: LGTM: Clean type definitions and imports.

The type definitions and imports are well-structured. The ConnectionStatus union type provides clear connection states, and the interface definitions are comprehensive.


26-45: Excellent type-safe helpers.

The helper functions effectively prevent the "#undefined" issues mentioned in the commit messages. Good use of type guards to ensure type safety.


47-83: LGTM: Well-designed tool description mapping.

The switch statement provides clear, user-friendly descriptions for all tool types. Good use of the type-safe helpers to avoid undefined values in descriptions.


85-100: LGTM: Proper state and ref management.

The state and ref setup is appropriate for managing WebSocket connections and chat messages. Good separation of state (for UI updates) and refs (for connection management).


102-115: LGTM: Proper cleanup on unmount.

The cleanup effect correctly clears all timers and closes the WebSocket connection to prevent memory leaks.


135-153: Excellent connection lifecycle management.

The connection guards (wsRef.current === ws) on lines 137, 149 effectively prevent stale handler issues. Clearing the previous ping interval before starting a new one (lines 142-145) prevents multiple timers, addressing the issue mentioned in the commit messages.


155-174: Solid reconnection logic with exponential backoff.

The close handler correctly validates the current connection before acting (line 157) and implements proper exponential backoff. The backoff calculation (lines 168-171) correctly doubles the delay with each attempt, capped at 10 seconds.


189-220: Correct streaming message handling.

The logic correctly handles streaming text by appending to the existing streaming message or creating a new one. The immutable update pattern is properly implemented.


223-239: Nice user experience improvement with friendly tool descriptions.

The tool_call handler now shows user-friendly descriptions via getToolDescription instead of raw tool names. This is a good UX enhancement.


246-267: Smart backward search for streaming messages.

The backward search (lines 253-263) correctly handles the case where tool_call or system messages may follow the streaming assistant message. This prevents the stuck isStreaming state mentioned in the commit messages.


338-367: LGTM: Standard message sending pattern.

The sendMessage function correctly checks connection state, updates UI state, and sends the message. Good error handling for disconnected state.


369-391: Excellent disconnect implementation.

The disconnect function properly prevents reconnection by setting reconnectAttempts to max and clearing the pending reconnect timeout (lines 373-376). This addresses the issue mentioned in the commit messages about preventing reconnect after user-initiated disconnect.


393-408: LGTM: Clean hook interface.

The clearMessages function and return statement are straightforward and complete.

Added connectTimeoutRef to track the setTimeout IDs used in the start
function's checkAndSend polling. The timeout is now:

- Cleared on component unmount via cleanup effect
- Cleared when connection succeeds or fails
- Cleared when maxRetries is reached
- Cleared on disconnect()
- Cleared before starting a new connection attempt

Prevents stale timeouts from firing after component unmounts.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
ui/src/hooks/useAssistantChat.ts (1)

317-348: Consider using a Promise-based approach for WebSocket readiness.

The polling mechanism (checkAndSend with 100ms intervals up to 5 seconds) works but is somewhat inelegant. Consider wrapping WebSocket connection in a Promise that resolves on onopen or rejects on timeout/error, which would eliminate polling and make the async flow more explicit.

♻️ Example Promise-based approach
const waitForConnection = (ws: WebSocket, timeoutMs: number = 5000): Promise<void> => {
  return new Promise((resolve, reject) => {
    const timeout = setTimeout(() => {
      reject(new Error('Connection timeout'));
    }, timeoutMs);

    if (ws.readyState === WebSocket.OPEN) {
      clearTimeout(timeout);
      resolve();
      return;
    }

    const handleOpen = () => {
      clearTimeout(timeout);
      ws.removeEventListener('open', handleOpen);
      ws.removeEventListener('error', handleError);
      resolve();
    };

    const handleError = () => {
      clearTimeout(timeout);
      ws.removeEventListener('open', handleOpen);
      ws.removeEventListener('error', handleError);
      reject(new Error('Connection failed'));
    };

    ws.addEventListener('open', handleOpen, { once: true });
    ws.addEventListener('error', handleError, { once: true });
  });
};

Then in start():

try {
  await waitForConnection(wsRef.current);
  // Send start message
} catch (error) {
  onError?.(error.message);
}
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8165d5d and a190d25.

📒 Files selected for processing (1)
  • ui/src/hooks/useAssistantChat.ts
🧰 Additional context used
🧬 Code graph analysis (1)
ui/src/hooks/useAssistantChat.ts (1)
ui/src/lib/types.ts (2)
  • ChatMessage (236-244)
  • AssistantChatServerMessage (304-310)
🔇 Additional comments (2)
ui/src/hooks/useAssistantChat.ts (2)

30-83: LGTM! Type-safe helpers and user-friendly tool descriptions.

The helper functions (getStringValue, getFeatureId) provide safe type guards for extracting values from unknown inputs, and getToolDescription maps tool calls to clear, user-friendly messages. The tool name normalization (removing the mcp__features__ prefix) ensures consistent handling regardless of how tools are invoked.


104-120: LGTM! Comprehensive timeout cleanup prevents resource leaks.

The timeout management is thorough:

  • connectTimeoutRef is cleared on unmount, at the start of start(), in all branches of the polling logic (success/timeout/error), and in disconnect()
  • reconnectTimeoutRef and pingIntervalRef are similarly cleaned up across all relevant lifecycle hooks

This addresses the polling timeout leak mentioned in the commit message.

Also applies to: 302-351, 384-412

- Add Project Assistant to Web UI feature list
- Document feature_create, feature_update, feature_delete MCP tools
- Add Project Assistant section with capabilities and persistence info
- Add assistant.db to generated project structure

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @README.md:
- Line 59: Update the README to reflect that project paths are registry-driven
by registry.py and that assistant.db is created at the registered project path
(i.e., <registered_project_path>/assistant.db) rather than a fixed
generations/my_project/ location; remove or correct the claim about a "New Chat"
button in the Project Assistant section (lines referencing the AssistantChat
component) since AssistantChat opens one conversation per panel session and has
no UI control to start/reset a conversation; keep the notes that feature_create,
feature_update, and feature_delete are implemented and that conversation
persistence works as designed.
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a190d25 and d218a88.

📒 Files selected for processing (1)
  • README.md
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: leonvanzyl/autocoder PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T08:23:04.012Z
Learning: Applies to mcp_server/**/*.py : Use MCP (Model Context Protocol) servers for agent tools: feature_mcp.py for feature management, with tools: feature_get_stats, feature_get_next, feature_get_for_regression, feature_mark_passing, feature_skip, feature_create_bulk

…ence

- Update Project Assistant section to note assistant.db is in registered path
- Remove "New Chat" button reference (not present in AssistantChat)
- Add registry.py to project structure
- Rename "Generated Project Structure" to "Project Registry and Structure"
- Explain registry-driven project paths (~/.autocoder/registry.db)
- Update path examples to use <registered_project_path> instead of fixed path

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
README.md (1)

201-212: Specify language for code block to comply with markdown linting.

The code block starting at line 201 should have a language identifier (e.g., ```) for consistency with markdown standards. This helps with syntax highlighting and linting compliance.

♻️ Proposed fix
-```
-<registered_project_path>/
+```
+<registered_project_path>/
🧹 Nitpick comments (1)
README.md (1)

193-212: Clarify registry reference in path examples.

While the new "Project Registry and Structure" section clearly explains the registry concept, consider explicitly mentioning in the "Running the Generated Application" section (around line 221) that <registered_project_path> refers to the project's registered path from the registry. This helps users understand the relationship between the registry and the project directory structure you've documented.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d218a88 and 71bdd0c.

📒 Files selected for processing (1)
  • README.md
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: leonvanzyl/autocoder PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T08:23:04.012Z
Learning: Applies to mcp_server/**/*.py : Use MCP (Model Context Protocol) servers for agent tools: feature_mcp.py for feature management, with tools: feature_get_stats, feature_get_next, feature_get_for_regression, feature_mark_passing, feature_skip, feature_create_bulk
📚 Learning: 2026-01-10T08:23:04.012Z
Learnt from: CR
Repo: leonvanzyl/autocoder PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T08:23:04.012Z
Learning: Two-agent pattern: Initializer Agent reads app spec and creates features in first session; Coding Agent implements features one by one in subsequent sessions

Applied to files:

  • README.md
📚 Learning: 2026-01-10T08:23:04.012Z
Learnt from: CR
Repo: leonvanzyl/autocoder PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T08:23:04.012Z
Learning: Applies to registry.py : Store project registry using SQLite with POSIX path format (forward slashes) for cross-platform compatibility

Applied to files:

  • README.md
📚 Learning: 2026-01-10T08:23:04.012Z
Learnt from: CR
Repo: leonvanzyl/autocoder PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T08:23:04.012Z
Learning: Applies to mcp_server/**/*.py : Use MCP (Model Context Protocol) servers for agent tools: feature_mcp.py for feature management, with tools: feature_get_stats, feature_get_next, feature_get_for_regression, feature_mark_passing, feature_skip, feature_create_bulk

Applied to files:

  • README.md
📚 Learning: 2026-01-10T08:23:04.012Z
Learnt from: CR
Repo: leonvanzyl/autocoder PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T08:23:04.012Z
Learning: Applies to autonomous_agent_demo.py : Use absolute paths or registered project names when running the agent via: python autonomous_agent_demo.py --project-dir <path_or_name>

Applied to files:

  • README.md
🪛 markdownlint-cli2 (0.18.1)
README.md

201-201: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🔇 Additional comments (1)
README.md (1)

107-123: Documentation updates integrate feature management capabilities well.

The additions clearly document the new MCP tools (feature_create, feature_update, feature_delete) and the Project Assistant functionality with conversation persistence. The explanation of how features can be managed via natural language in the assistant aligns well with the PR objectives. The note about assistant.db storage in the registered project directory provides useful operational context.

connor-tyndall and others added 2 commits January 11, 2026 14:36
- Add 'text' language identifier to project structure code block
- Clarify that path refers to registered project path in Running section

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Resolved conflicts:
- mcp_server/feature_mcp.py: Combined feature_update/delete from HEAD with threading lock from master
- server/routers/features.py: Combined update_feature endpoint from HEAD with create_features_bulk from master
- server/services/assistant_chat_session.py: Combined feature management docs from HEAD with master changes
- ui/src/hooks/useAssistantChat.ts: Kept HEAD's robust connection handling (guard checks, retry limits, getToolDescription helper)
- ui/src/lib/api.ts: Combined FeatureUpdate import from HEAD with bulk create types from master

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant