feat(chat): 支持单模型组成team实现聊天 #268
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces a complete Chat Shell direct-chat streaming system, enabling LLM responses to stream directly without Docker Executor involvement. It includes backend API endpoints for streaming chat, a chat service with LLM integration, Redis-backed session management, model configuration resolution with environment-aware placeholders, a document attachment system with multi-format parsing (PDF, Word, Excel, images), and corresponding frontend API clients with streaming UI components, attachment upload/preview, and hooks for managing streaming state. Changes
Sequence DiagramssequenceDiagram
participant Client as User Browser
participant FrontendAPI as Frontend API<br/>(Next.js)
participant BackendAPI as Backend API
participant ChatService as Chat Service
participant LLM as LLM API
participant Redis as Redis Cache
participant DB as Database
Client->>FrontendAPI: POST /api/chat/stream<br/>(message, team_id, etc.)
FrontendAPI->>BackendAPI: Forward to /api/chat/stream<br/>(with auth header)
BackendAPI->>DB: Validate team, check shell types
BackendAPI->>DB: Create task & subtasks
BackendAPI->>Redis: Initialize chat session
BackendAPI->>ChatService: chat_stream(subtask_id, message)
ChatService->>Redis: Get chat history for task
ChatService->>DB: Resolve bot model config<br/>(with env placeholders)
ChatService->>ChatService: Assemble messages<br/>(history + current)
ChatService->>LLM: POST /v1/chat/completions<br/>(streaming)
loop Stream Chunks
LLM-->>ChatService: SSE chunk (delta content)
ChatService-->>BackendAPI: SSE formatted chunk
BackendAPI-->>FrontendAPI: Chunk (data: {...})
FrontendAPI-->>Client: Parse & render streaming content
end
ChatService->>Redis: Save accumulated response<br/>to history
ChatService->>DB: Update subtask status<br/>(ASSISTANT subtask)
ChatService->>DB: Update task status
ChatService-->>BackendAPI: Stream complete
BackendAPI-->>FrontendAPI: [DONE]
FrontendAPI-->>Client: Streaming complete
sequenceDiagram
participant Client as User (Browser)
participant FileUpload as FileUpload<br/>Component
participant AttachmentAPI as Attachment API
participant Parser as Document<br/>Parser
participant DB as Database
participant ChatArea as ChatArea<br/>Component
Client->>FileUpload: Select/drop file
FileUpload->>FileUpload: Validate extension,<br/>file size
FileUpload->>AttachmentAPI: POST /api/attachments/upload<br/>(file)
AttachmentAPI->>DB: Create SubtaskAttachment<br/>(status: UPLOADING)
AttachmentAPI->>Parser: parse(binary_data,<br/>extension)
alt Image
Parser->>Parser: Extract metadata<br/>& base64 encode
Parser-->>AttachmentAPI: (metadata_text, base64)
else PDF/Document
Parser->>Parser: Extract text from format<br/>(pypdf2, python-docx, etc.)
Parser->>Parser: Enforce MAX_TEXT_LENGTH
Parser-->>AttachmentAPI: (extracted_text, length)
end
AttachmentAPI->>DB: Update SubtaskAttachment<br/>(status: READY,<br/>extracted_text, etc.)
AttachmentAPI-->>FileUpload: AttachmentResponse
FileUpload->>FileUpload: Update UI<br/>(show preview)
Client->>ChatArea: Type message & submit
ChatArea->>ChatArea: Build message with<br/>attachment
ChatArea->>ChatArea: Call useChatStream
Note over ChatArea: Chat flows with attached<br/>document content or<br/>image included in prompt
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (20)
backend/app/services/chat/base.py (1)
86-86: Annotate class attribute with ClassVar.Mutable class attributes should be annotated with
typing.ClassVarto clarify they're shared across all instances.Based on static analysis hints (Ruff RUF012).
Apply this diff:
+from typing import ClassVar, Optional -from typing import Optional - DIRECT_CHAT_SHELL_TYPES = ["Chat"] + DIRECT_CHAT_SHELL_TYPES: ClassVar[list[str]] = ["Chat"]backend/app/services/chat/session_manager.py (1)
59-61: Prefer logging.exception for better error diagnostics.Using
logging.exception()instead oflogging.error()in exception handlers automatically includes stack traces, which aids debugging without changing behavior.Based on static analysis hints (Ruff TRY400).
Example for line 60:
- logger.error(f"Error getting chat history for task {task_id}: {e}") + logger.exception(f"Error getting chat history for task {task_id}: {e}")Apply similar changes to lines 93, 119, 148, and 166.
Also applies to: 92-94, 118-120, 147-149, 165-167
backend/app/services/adapters/team_kinds.py (1)
877-877: Simplify boolean comparison.Prefer truthiness check over explicit equality comparison with
True.Based on static analysis hints (Ruff E712).
Apply this diff:
- PublicShell.is_active == True, + PublicShell.is_active,backend/app/services/chat/__init__.py (1)
1-20: LGTM! Clean package initializer with explicit public API surface.The module correctly exposes the key components for the Chat Shell service. The docstring clearly explains the module's purpose.
One optional improvement based on static analysis:
__all__could be sorted alphabetically for consistency with isort conventions.__all__ = [ "chat_service", - "session_manager", "get_model_config_for_bot", + "session_manager", ]frontend/src/hooks/useChatStream.ts (1)
94-136: Potential race condition:abortRefis set afterawait streamChat.If a user calls
stopStream()whilestreamChatis still initializing (before line 134 executes),abortRef.currentwill benulland the abort will be silently ignored. Consider setting up a local abort mechanism before the async call.const startStream = useCallback(async (request: StreamChatRequest): Promise<number> => { // Reset state setIsStreaming(true); setStreamingContent(''); setError(null); setTaskId(request.task_id || null); setSubtaskId(null); let resolvedTaskId = request.task_id || 0; + + // Create abort controller before starting stream + let localAbort: (() => void) | null = null; + abortRef.current = () => localAbort?.(); const { abort } = await streamChat(request, { onMessage: (data: ChatStreamData) => { // ... }, // ... }); - abortRef.current = abort; + localAbort = abort; + abortRef.current = abort; return resolvedTaskId; }, []);frontend/src/app/api/chat/stream/route.ts (2)
14-15: Consider using a server-side environment variable name.
NEXT_PUBLIC_API_URLis conventionally for client-side exposure. For a server-side API route, consider using a non-prefixed variable likeAPI_URLorBACKEND_URLto follow Next.js conventions and avoid unintentional client-side exposure of the backend URL.
26-33: Consider adding a timeout to the backend fetch request.The fetch call to the backend has no timeout configured. If the backend becomes unresponsive, this proxy could hang indefinitely. Consider using
AbortControllerwith a timeout.+ // Add timeout for backend request + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 120000); // 2 minute timeout + // Forward request to backend const backendResponse = await fetch(`${BACKEND_URL}/api/chat/stream`, { method: 'POST', headers: { 'Content-Type': 'application/json', ...(authHeader && { Authorization: authHeader }), }, body: JSON.stringify(body), + signal: controller.signal, }); + + clearTimeout(timeoutId);frontend/src/features/tasks/service/messageService.ts (1)
15-26: Remove verbose console.log before production.The debug logging is helpful during development but should be removed or guarded behind a debug flag for production to avoid console noise.
export function isChatShell(team: Team | null): boolean { // Case-insensitive comparison since backend may return 'chat' or 'Chat' const result = team?.agent_type?.toLowerCase() === 'chat'; - console.log('[messageService] isChatShell check:', { - teamId: team?.id, - teamName: team?.name, - agentType: team?.agent_type, - agentTypeLower: team?.agent_type?.toLowerCase(), - isChatShell: result, - }); return result; }backend/app/services/chat/chat_service.py (4)
113-129: Uselogging.exceptionfor better stack trace capture.For exception handlers,
logging.exceptionautomatically includes the stack trace, which is more helpful for debugging thanlogging.errorwithexc_info=True.except asyncio.TimeoutError: error_msg = "API call timeout" - logger.error(f"Chat stream timeout for subtask {subtask_id}") + logger.exception(f"Chat stream timeout for subtask {subtask_id}") yield f"data: {json.dumps({'error': error_msg})}\n\n" await self._update_subtask_status(subtask_id, "FAILED", error=error_msg) except httpx.RequestError as e: - error_msg = f"Network error: {str(e)}" - logger.error(f"Chat stream network error for subtask {subtask_id}: {e}") + error_msg = f"Network error: {e!s}" + logger.exception(f"Chat stream network error for subtask {subtask_id}") yield f"data: {json.dumps({'error': error_msg})}\n\n" await self._update_subtask_status(subtask_id, "FAILED", error=error_msg) except Exception as e: error_msg = str(e) - logger.error(f"Chat stream error for subtask {subtask_id}: {e}", exc_info=True) + logger.exception(f"Chat stream error for subtask {subtask_id}") yield f"data: {json.dumps({'error': error_msg})}\n\n" await self._update_subtask_status(subtask_id, "FAILED", error=error_msg)
229-229: Fix implicitOptionaltype hint.PEP 484 prohibits implicit
Optional. Use explicit union syntax.- default_headers: Dict[str, Any] = None + default_headers: Optional[Dict[str, Any]] = NoneOr with modern Python 3.10+ syntax:
- default_headers: Dict[str, Any] = None + default_headers: Dict[str, Any] | None = None
308-308: Fix implicitOptionaltype hint (same issue as line 229).- default_headers: Dict[str, Any] = None + default_headers: Optional[Dict[str, Any]] = None
476-481: Avoid== Truecomparison in SQLAlchemy filter.SQLAlchemy boolean columns should use
is_(True)or just the column for truth checks to avoid potential issues.task = ( db.query(Kind) - .filter(Kind.id == task_id, Kind.kind == "Task", Kind.is_active == True) + .filter(Kind.id == task_id, Kind.kind == "Task", Kind.is_active.is_(True)) .first() )frontend/src/features/tasks/components/ChatArea.tsx (2)
319-325: Consider reducing debug logging in production.These console.log statements are useful during development but may clutter production logs. Consider using a conditional debug flag or removing before merge.
708-768: Consider extracting duplicated button rendering logic.The button rendering for streaming/pending/running/cancelling states is nearly identical in both the "no messages" and "has messages" sections. Extract to a shared component or helper to reduce duplication and maintenance burden.
Example approach:
const SendButtonArea = () => { if (isStreaming) { return ( <Button variant="ghost" size="icon" onClick={stopStream} ...> <CircleStop className="h-5 w-5 text-orange-500" /> </Button> ) } // ... rest of conditions }Also applies to: 863-919
frontend/src/features/tasks/components/MessagesArea.tsx (1)
159-164: Good: Typewriter effect for streaming content.Using
useTypewriterprovides a smooth, engaging display of streaming content rather than jarring text jumps.Note:
isChatShelllogic is duplicated here (Line 164) and inmessageService.ts. Consider importing from the service for consistency.- // Check if team uses Chat Shell (streaming mode, no polling needed) - // Case-insensitive comparison since backend may return 'chat' or 'Chat' - const isChatShell = selectedTeam?.agent_type?.toLowerCase() === 'chat'; + import { isChatShell as checkIsChatShell } from '../service/messageService'; + // ... + const isChatShellTeam = checkIsChatShell(selectedTeam);frontend/src/apis/chat.ts (1)
174-176: Silent parse error handling may hide issues.Ignoring parse errors silently (Line 174-176) could mask malformed server responses. Consider logging in development mode for debugging.
} catch { - // Ignore parse errors for incomplete chunks + // Ignore parse errors for incomplete chunks + if (process.env.NODE_ENV === 'development') { + console.debug('[chat.ts] Skipped unparseable chunk:', data); + } }backend/app/api/endpoints/adapter/chat.py (3)
102-102: Unuseduser_idparameter in_should_use_direct_chat.The function signature includes
user_idbut it's never used - the function usesteam.user_idinstead (Line 118, 131). Remove the unused parameter or use it consistently.-def _should_use_direct_chat(db: Session, team: Kind, user_id: int) -> bool: +def _should_use_direct_chat(db: Session, team: Kind) -> bool:And update the call sites (Lines 389, 542) accordingly.
428-429: Add exception chaining for better debugging.When re-raising the
ValueErrorasHTTPException, usefrom eto preserve the exception chain.except ValueError as e: - raise HTTPException(status_code=400, detail=str(e)) + raise HTTPException(status_code=400, detail=str(e)) from e
438-438: Remove unused variablebot_crd.The variable is assigned but never used (flagged by Ruff F841).
- bot_crd = Bot.model_validate(bot.json) bot_json = bot.json or {}backend/app/services/chat/model_resolver.py (1)
362-362: Remove extraneous f-prefix from string literal.The f-string has no placeholders.
- logger.info(f"[model_resolver] api_key contains placeholder, resolving from env...") + logger.info("[model_resolver] api_key contains placeholder, resolving from env...")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
backend/app/api/api.py(2 hunks)backend/app/api/endpoints/adapter/chat.py(1 hunks)backend/app/core/config.py(1 hunks)backend/app/main.py(2 hunks)backend/app/services/adapters/team_kinds.py(2 hunks)backend/app/services/chat/__init__.py(1 hunks)backend/app/services/chat/base.py(1 hunks)backend/app/services/chat/chat_service.py(1 hunks)backend/app/services/chat/model_resolver.py(1 hunks)backend/app/services/chat/session_manager.py(1 hunks)backend/init_data/02-public-shells.yaml(1 hunks)frontend/src/apis/chat.ts(1 hunks)frontend/src/app/api/chat/stream/route.ts(1 hunks)frontend/src/features/tasks/components/ChatArea.tsx(11 hunks)frontend/src/features/tasks/components/MessagesArea.tsx(7 hunks)frontend/src/features/tasks/service/messageService.ts(4 hunks)frontend/src/hooks/useChatStream.ts(1 hunks)frontend/src/hooks/useTypewriter.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{py,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
All code comments, inline comments, block comments, docstrings, TODO/FIXME annotations, and type hints descriptions MUST be written in English
Files:
frontend/src/hooks/useTypewriter.tsbackend/app/main.pyfrontend/src/features/tasks/service/messageService.tsbackend/app/services/adapters/team_kinds.pyfrontend/src/apis/chat.tsfrontend/src/hooks/useChatStream.tsbackend/app/services/chat/base.pybackend/app/services/chat/chat_service.pybackend/app/services/chat/__init__.pybackend/app/services/chat/session_manager.pyfrontend/src/features/tasks/components/MessagesArea.tsxbackend/app/api/api.pyfrontend/src/app/api/chat/stream/route.tsbackend/app/core/config.pybackend/app/api/endpoints/adapter/chat.pybackend/app/services/chat/model_resolver.pyfrontend/src/features/tasks/components/ChatArea.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: TypeScript code MUST use strict mode with type checking enabled
TypeScript/React code MUST use Prettier formatter with single quotes, no semicolons
TypeScript/React code MUST pass ESLint with Next.js configuration
React component names MUST use PascalCase convention
Files:
frontend/src/hooks/useTypewriter.tsfrontend/src/features/tasks/service/messageService.tsfrontend/src/apis/chat.tsfrontend/src/hooks/useChatStream.tsfrontend/src/features/tasks/components/MessagesArea.tsxfrontend/src/app/api/chat/stream/route.tsfrontend/src/features/tasks/components/ChatArea.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: React components MUST use functional components with hooks instead of class-based components
Useconstoverlet, never usevarin TypeScript/JavaScript code
Files:
frontend/src/hooks/useTypewriter.tsfrontend/src/features/tasks/service/messageService.tsfrontend/src/apis/chat.tsfrontend/src/hooks/useChatStream.tsfrontend/src/features/tasks/components/MessagesArea.tsxfrontend/src/app/api/chat/stream/route.tsfrontend/src/features/tasks/components/ChatArea.tsx
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code MUST be PEP 8 compliant with Black formatter (line length: 88) and isort for import organization
Python code MUST include type hints for all functions and variables
Python functions SHOULD NOT exceed 50 lines (preferred maximum)
Python functions and classes MUST have descriptive names and public functions/classes MUST include docstrings
Python code MUST extract magic numbers to named constants
Files:
backend/app/main.pybackend/app/services/adapters/team_kinds.pybackend/app/services/chat/base.pybackend/app/services/chat/chat_service.pybackend/app/services/chat/__init__.pybackend/app/services/chat/session_manager.pybackend/app/api/api.pybackend/app/core/config.pybackend/app/api/endpoints/adapter/chat.pybackend/app/services/chat/model_resolver.py
**/backend/app/services/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend business logic MUST be placed in
app/services/directory
Files:
backend/app/services/adapters/team_kinds.pybackend/app/services/chat/base.pybackend/app/services/chat/chat_service.pybackend/app/services/chat/__init__.pybackend/app/services/chat/session_manager.pybackend/app/services/chat/model_resolver.py
**/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
React component files MUST use kebab-case naming convention
Files:
frontend/src/features/tasks/components/MessagesArea.tsxfrontend/src/features/tasks/components/ChatArea.tsx
**/src/**/*.{tsx,jsx,css}
📄 CodeRabbit inference engine (AGENTS.md)
Frontend Tailwind CSS MUST use provided CSS variables for color system (e.g., --color-bg-base, --color-text-primary, --color-primary)
Files:
frontend/src/features/tasks/components/MessagesArea.tsxfrontend/src/features/tasks/components/ChatArea.tsx
**/src/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/src/**/*.{tsx,jsx}: Frontend responsive design MUST follow mobile-first approach with Tailwind breakpoints
Frontend React forms MUST use react-hook-form with zod validation schema
Frontend components MUST use shadcn/ui component library fromfrontend/src/components/ui/
Files:
frontend/src/features/tasks/components/MessagesArea.tsxfrontend/src/features/tasks/components/ChatArea.tsx
**/backend/app/api/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend API route handlers MUST be created in
app/api/directory
Files:
backend/app/api/api.pybackend/app/api/endpoints/adapter/chat.py
🧬 Code graph analysis (10)
backend/app/main.py (1)
backend/app/services/chat/base.py (2)
get_http_client(29-58)close_http_client(61-75)
frontend/src/features/tasks/service/messageService.ts (2)
frontend/src/types/api.ts (1)
Team(95-111)frontend/src/apis/chat.ts (2)
StreamChatCallbacks(62-69)chatApis(255-258)
backend/app/services/adapters/team_kinds.py (3)
frontend/src/types/api.ts (1)
Shell(84-93)backend/app/schemas/kind.py (1)
Shell(129-136)backend/app/models/public_shell.py (1)
PublicShell(19-38)
frontend/src/apis/chat.ts (3)
backend/app/api/endpoints/adapter/chat.py (1)
StreamChatRequest(38-51)frontend/src/apis/client.ts (1)
request(22-80)frontend/src/apis/user.ts (1)
getToken(54-59)
backend/app/services/chat/chat_service.py (4)
backend/app/services/chat/base.py (1)
get_http_client(29-58)backend/app/services/chat/session_manager.py (2)
get_chat_history(35-61)append_user_and_assistant_messages(122-149)backend/app/models/subtask.py (1)
Subtask(31-64)backend/app/models/kind.py (1)
Kind(25-42)
backend/app/services/chat/__init__.py (1)
backend/app/services/chat/model_resolver.py (1)
get_model_config_for_bot(205-278)
frontend/src/features/tasks/components/MessagesArea.tsx (2)
frontend/src/hooks/useTypewriter.ts (1)
useTypewriter(17-67)frontend/src/features/tasks/service/messageService.ts (1)
isChatShell(15-26)
frontend/src/app/api/chat/stream/route.ts (1)
frontend/src/apis/client.ts (1)
request(22-80)
backend/app/services/chat/model_resolver.py (4)
shared/utils/crypto.py (1)
decrypt_api_key(222-239)backend/app/models/kind.py (1)
Kind(25-42)backend/app/models/public_model.py (1)
PublicModel(19-38)backend/app/schemas/kind.py (1)
Ghost(47-54)
frontend/src/features/tasks/components/ChatArea.tsx (3)
frontend/src/hooks/useChatStream.ts (1)
useChatStream(75-166)frontend/src/features/tasks/service/messageService.ts (1)
isChatShell(15-26)frontend/src/features/tasks/components/ModelSelector.tsx (1)
DEFAULT_MODEL_NAME(40-40)
🪛 Ruff (0.14.7)
backend/app/services/adapters/team_kinds.py
877-877: Avoid equality comparisons to True; use PublicShell.is_active: for truth checks
Replace with PublicShell.is_active
(E712)
backend/app/services/chat/base.py
86-86: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
backend/app/services/chat/chat_service.py
115-115: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
120-120: Use explicit conversion flag
Replace with conversion flag
(RUF010)
121-121: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
208-208: Avoid specifying long messages outside the exception class
(TRY003)
229-229: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
265-265: Local variable masked_key is assigned to but never used
Remove assignment to unused variable masked_key
(F841)
308-308: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
454-454: Do not catch blind exception: Exception
(BLE001)
455-455: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
479-479: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
524-524: Do not catch blind exception: Exception
(BLE001)
525-525: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
backend/app/services/chat/__init__.py
16-20: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
backend/app/services/chat/session_manager.py
57-57: Consider moving this statement to an else block
(TRY300)
59-59: Do not catch blind exception: Exception
(BLE001)
60-60: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
92-92: Do not catch blind exception: Exception
(BLE001)
93-93: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
118-118: Do not catch blind exception: Exception
(BLE001)
119-119: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
147-147: Do not catch blind exception: Exception
(BLE001)
148-148: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
165-165: Do not catch blind exception: Exception
(BLE001)
166-166: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
backend/app/api/endpoints/adapter/chat.py
68-68: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
75-75: f-string without any placeholders
Remove extraneous f prefix
(F541)
82-82: Avoid equality comparisons to True; use PublicShell.is_active: for truth checks
Replace with PublicShell.is_active
(E712)
90-90: f-string without any placeholders
Remove extraneous f prefix
(F541)
98-98: f-string without any placeholders
Remove extraneous f prefix
(F541)
102-102: Unused function argument: user_id
(ARG001)
122-122: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
128-128: f-string without any placeholders
Remove extraneous f prefix
(F541)
139-139: f-string without any placeholders
Remove extraneous f prefix
(F541)
169-169: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
189-189: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
307-307: f-string without any placeholders
Remove extraneous f prefix
(F541)
330-330: f-string without any placeholders
Remove extraneous f prefix
(F541)
356-356: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
357-357: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
380-380: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
411-411: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
429-429: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
438-438: Local variable bot_crd is assigned to but never used
Remove assignment to unused variable bot_crd
(F841)
520-520: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
521-521: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
534-534: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
556-556: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
backend/app/services/chat/model_resolver.py
101-101: Do not catch blind exception: Exception
(BLE001)
167-167: Do not catch blind exception: Exception
(BLE001)
198-198: Do not catch blind exception: Exception
(BLE001)
270-270: Avoid specifying long messages outside the exception class
(TRY003)
275-275: Avoid specifying long messages outside the exception class
(TRY003)
308-308: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
322-322: Avoid equality comparisons to True; use PublicModel.is_active: for truth checks
Replace with PublicModel.is_active
(E712)
362-362: f-string without any placeholders
Remove extraneous f prefix
(F541)
379-379: Do not catch blind exception: Exception
(BLE001)
396-396: Do not catch blind exception: Exception
(BLE001)
441-441: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-tests
🔇 Additional comments (37)
backend/app/core/config.py (1)
49-54: LGTM! Well-structured configuration additions.The new chat-related configuration fields are properly typed, documented, and have reasonable defaults. The inline comments clearly explain each setting's purpose.
frontend/src/hooks/useTypewriter.ts (1)
7-66: LGTM! Clean implementation with smart adaptive logic.The hook provides a smooth typewriter effect with intelligent catch-up behavior when content updates rapidly. The adaptive speed logic (lines 43-55) elegantly balances smooth typing with responsive updates.
backend/app/services/chat/base.py (1)
29-58: LGTM! Proper double-checked locking pattern.The shared HTTP client initialization correctly uses async locks and configures appropriate timeouts and connection pooling for LLM API calls.
backend/app/main.py (2)
109-114: LGTM! Proper HTTP client initialization.The startup lifecycle correctly initializes the shared chat HTTP client with appropriate logging.
207-214: LGTM! Proper cleanup in shutdown.The shutdown lifecycle correctly closes the shared HTTP client to release resources.
backend/app/services/chat/session_manager.py (1)
35-61: LGTM! Safe fallback handling.The function correctly returns an empty list on errors, ensuring chat can continue even if history retrieval fails.
backend/app/api/api.py (1)
9-9: LGTM! Router registration follows established patterns.The chat router is properly imported and registered with appropriate prefix and tags, consistent with other API modules.
Also applies to: 30-30
backend/init_data/02-public-shells.yaml (1)
47-62: LGTM! Well-defined Chat shell resource.The new Chat shell is properly structured with appropriate metadata, supported models (claude, openai), and direct_chat type label. This aligns with the backend's DIRECT_CHAT_SHELL_TYPES configuration.
backend/app/services/adapters/team_kinds.py (1)
851-895: LGTM! Clean two-tier shell resolution.The refined logic correctly checks user-specific shells first, then falls back to public shells, ensuring the new public Chat Shell is discoverable for agent_type mappings.
frontend/src/hooks/useChatStream.ts (1)
141-165: LGTM! Clean hook design with proper state management.The
stopStreamandresetStreamfunctions are well-implemented. The hook follows React best practices withuseCallbackfor stable function references and proper state isolation.frontend/src/app/api/chat/stream/route.ts (1)
55-72: LGTM! Correct streaming pattern.The async IIFE for piping the stream is the appropriate pattern for Next.js streaming responses. The error handling properly aborts the writer, which will signal the client that the stream ended unexpectedly.
frontend/src/features/tasks/service/messageService.ts (1)
79-101: LGTM! Clean Chat Shell streaming integration.The streaming path correctly:
- Checks both
isChatShell(team)andstreamCallbackspresence- Propagates all necessary parameters to the streaming API
- Returns the abort function for cleanup
- Handles errors with proper fallback message
backend/app/services/chat/chat_service.py (1)
28-41: LGTM! Proper lazy initialization for async semaphore.The lazy initialization pattern for the semaphore is correct for async contexts where the semaphore must be created within an event loop. The thread pool for DB operations is a good approach to avoid blocking the async loop.
frontend/src/features/tasks/components/ChatArea.tsx (5)
78-79: LGTM - Optimistic UI state for Chat Shell streaming.The
pendingUserMessagestate enables immediate feedback when the user sends a message, improving perceived performance.
104-134: Streaming hook setup looks correct.The
useChatStreamintegration properly handles completion (refreshes data, updates URL) and errors (clears optimistic state, shows toast). TheresetStream()call inonCompleteensures clean state for subsequent messages.
145-149: Good:hasMessagesnow correctly accounts for streaming state.Including
pendingUserMessageandisStreamingensures the UI transitions smoothly when starting a new chat via streaming.
336-383: Chat Shell streaming path is well-structured.The optimistic update flow (set pending message → clear input → start stream → create task entry) provides good UX. Error handling correctly reverts the optimistic state.
One minor observation: if
startStreamthrows before the task is created (Lines 373-379), the error toast is shown, which is correct. However,setIsLoading(false)at Line 381 runs regardless of success/failure, which is the expected behavior.
424-431: Consistent use ofmessageinstead oftaskInputMessagefor task fields.Using the trimmed
messagevariable for bothtitleandpromptensures consistency between the Chat Shell and non-Chat Shell paths.frontend/src/features/tasks/components/MessagesArea.tsx (4)
128-147: Props interface extended correctly for streaming support.The new optional props (
streamingContent,isStreaming,pendingUserMessage) are properly typed and documented with JSDoc comments.
166-189: Polling correctly disabled for Chat Shell teams.The early return when
isChatShellis true prevents unnecessary polling since streaming provides real-time updates.
343-343: Scroll dependencies updated for streaming.Including
displayContent,pendingUserMessage, andisStreamingensures auto-scroll triggers appropriately during streaming.
810-860: Streaming UI rendering looks good.The pending user message and streaming AI response blocks are styled consistently with existing messages. The blinking cursor (
▊) provides visual feedback during streaming.Minor note: The blinking cursor at Line 856 is always visible when streaming, even after content stops. This might be intentional but could be confusing if the stream ends without the
doneflag.frontend/src/apis/chat.ts (4)
20-35: Well-defined stream data interface.The
ChatStreamDatainterface covers all expected SSE event fields including error handling and completion state.
40-57: Request interface aligns with backend.The
StreamChatRequestinterface matches the backend'sStreamChatRequestmodel inbackend/app/api/endpoints/adapter/chat.py(Lines 38-51).
138-203: SSE stream processing is correct.The async IIFE pattern for processing the stream is appropriate here since the function needs to return immediately with the abort controller. Buffer handling for incomplete lines and
[DONE]detection are properly implemented.
224-250:checkDirectChatfunction is consistent with streaming approach.Error handling matches the
streamChatpattern. This provides a way to check team capabilities before initiating a stream.backend/app/api/endpoints/adapter/chat.py (5)
38-51: Request model is well-defined.The
StreamChatRequestmodel properly types all parameters with appropriate defaults for optional fields.
54-99: Shell type resolution logic is correct.The function properly checks user shells first, then falls back to public shells. Logging aids debugging.
Minor: Several f-strings have no placeholders (Lines 75, 90, 98). This is a style issue flagged by Ruff but doesn't affect functionality.
143-350: Task and subtask creation is comprehensive.The function correctly:
- Validates existing tasks and their status
- Creates workspace and task with proper JSON structure
- Assigns sequential message_ids for conversation threading
- Creates both USER and ASSISTANT subtasks
Note: The placeholder
completed_atfor the assistant subtask (Line 342) is documented and appropriate given the DB constraint.
483-514: Streaming response implementation is correct.The generator pattern with initial ID message followed by forwarded stream chunks is appropriate. SSE headers are properly set with buffering disabled.
517-566: Direct chat check endpoint is straightforward.Returns the capability flag and shell type for the first bot, which is sufficient for frontend decision-making.
backend/app/services/chat/model_resolver.py (6)
27-54: Environment placeholder resolution is well-implemented.The function correctly handles the
${ENV_VAR}pattern and logs resolution status (without exposing values).
57-102: Data source resolution supports flexible notation.The function handles both
source.pathand plainpathformats, with proper array index support. The broadexcept Exception(Line 101) is acceptable here as it's a parsing utility that should fail gracefully.
129-170: Environment header loading has good fallback chain.Tries
EXECUTOR_ENVJSON first, thenDEFAULT_HEADERS, with proper error handling for malformed JSON.
205-278: Model resolution priority is clearly documented and implemented.The four-level priority (force override → bot binding → modelRef → fallback) is well-documented and correctly implemented.
385-397: API key handling is secure.
- Keys are masked in logs (Lines 386-387, 393-394)
- Decryption is attempted only for non-placeholder values
- Fallback to original value on decryption failure is appropriate for backward compatibility
408-456: System prompt resolution correctly combines Ghost and team member prompts.The function properly looks up the Ghost referenced by the bot and appends any team member prompt with appropriate formatting.
| # Log request details (mask API key for security) | ||
| masked_key = f"{api_key[:8]}...{api_key[-4:]}" if api_key and len(api_key) > 12 else ("EMPTY" if not api_key else "***") | ||
| logger.info(f"Calling OpenAI API: {url}, model: {model_id}") | ||
| logger.info(f"[DEBUG] Full request headers: {list(headers.keys())}") | ||
| for k, v in headers.items(): | ||
| if k.lower() in ['authorization', 'x-api-key', 'api-key']: | ||
| masked_v = f"{v[:20]}..." if len(v) > 20 else "***" | ||
| logger.info(f"[DEBUG] Header {k}: {masked_v}") | ||
| else: | ||
| logger.info(f"[DEBUG] Header {k}: {v}") | ||
| logger.info(f"[DEBUG] Request payload: model={model_id}, messages_count={len(messages)}, stream=True") | ||
|
|
||
| async with client.stream("POST", url, json=payload, headers=headers) as response: | ||
| if response.status_code >= 400: | ||
| # Read error response body for debugging | ||
| error_body = await response.aread() | ||
| logger.error(f"[DEBUG] Claude API error response: status={response.status_code}, body={error_body.decode('utf-8', errors='replace')}") |
There was a problem hiding this comment.
Remove unused variable and fix incorrect comment.
masked_keyis assigned but never used (static analysis flagged this)- Line 280 incorrectly references "Claude API" but this is the OpenAI streaming method
- # Log request details (mask API key for security)
- masked_key = f"{api_key[:8]}...{api_key[-4:]}" if api_key and len(api_key) > 12 else ("EMPTY" if not api_key else "***")
logger.info(f"Calling OpenAI API: {url}, model: {model_id}")
# ... existing logging ...
async with client.stream("POST", url, json=payload, headers=headers) as response:
if response.status_code >= 400:
# Read error response body for debugging
error_body = await response.aread()
- logger.error(f"[DEBUG] Claude API error response: status={response.status_code}, body={error_body.decode('utf-8', errors='replace')}")
+ logger.error(f"[DEBUG] OpenAI API error response: status={response.status_code}, body={error_body.decode('utf-8', errors='replace')}")
response.raise_for_status()🧰 Tools
🪛 Ruff (0.14.7)
265-265: Local variable masked_key is assigned to but never used
Remove assignment to unused variable masked_key
(F841)
🤖 Prompt for AI Agents
In backend/app/services/chat/chat_service.py around lines 264 to 280, remove the
unused assignment to masked_key (delete the line that builds masked_key) and
update the error log text to reference OpenAI rather than "Claude" (change the
logger.error message that currently says "Claude API error response" to "OpenAI
API error response" or similar); keep the rest of the header-masking and
response-reading logic intact.
| loop = asyncio.get_event_loop() | ||
|
|
||
| def _update(): | ||
| from app.db.session import SessionLocal | ||
| from app.models.subtask import Subtask, SubtaskStatus | ||
|
|
||
| db = SessionLocal() | ||
| try: | ||
| subtask = db.query(Subtask).get(subtask_id) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /repo && find . -maxdepth 2 -type f \( -name "pyproject.toml" -o -name "setup.py" -o -name "requirements.txt" -o -name "setup.cfg" -o -name "requirements*.txt" \) | head -20Repository: wecode-ai/Wegent
Length of output: 115
🏁 Script executed:
cd /repo && git ls-files | grep -E "(pyproject|setup|requirements)" | head -20Repository: wecode-ai/Wegent
Length of output: 115
🌐 Web query:
SQLAlchemy 2.0 Query.get() deprecation migration guide
💡 Result:
Short migration guide — replace Query.get()
-
Why: Query.get() is legacy in 2.0; use Session.get() which uses the identity map and is the supported API. (docs.sqlalchemy.org)
-
Basic replacement
- Old (legacy): session.query(MyModel).get(pk)
- New: session.get(MyModel, pk)
Example: my_user = session.get(User, 5). (docs.sqlalchemy.org)
-
Composite primary keys
- Tuple form: session.get(MyModel, (pk1, pk2))
- Dict form: session.get(MyModel, {"id": pk1, "version_id": pk2}). (docs.sqlalchemy.org)
-
Options that Query.get supported
- Use Session.get(..., populate_existing=True) to force a DB load; use the options= and with_for_update= parameters on Session.get for loader/execution options. (docs.sqlalchemy.org)
-
Flask/Model usage
- Replace User.query.get(id) with db.session.get(User, id) or User.query.session.get(User, id) if you must call from the model. (stackoverflow.com)
-
Testing/migration steps
- Turn on RemovedIn20 warnings in 1.4, fix all Query.get usages to Session.get, run tests, then upgrade to 2.0. See the 1.4→2.0 migration guide. (docs.sqlalchemy.org)
If you want, I can scan your codebase for Query.get usages and produce an automated replacement diff.
Citations:
- 1: https://docs.sqlalchemy.org/20/orm/queryguide/query.html?utm_source=openai
- 2: https://docs.sqlalchemy.org/20/orm/queryguide/query.html?utm_source=openai
- 3: https://docs.sqlalchemy.org/20/orm/session_api.html?utm_source=openai
- 4: https://docs.sqlalchemy.org/20/orm/session_api.html?utm_source=openai
- 5: https://stackoverflow.com/questions/75365194/sqlalchemy-2-0-version-of-user-query-get1-in-flask-sqlalchemy?utm_source=openai
- 6: https://docs.sqlalchemy.org/20/changelog/migration_20.html?utm_source=openai
🏁 Script executed:
pwd && ls -la | head -20Repository: wecode-ai/Wegent
Length of output: 1245
🏁 Script executed:
find . -maxdepth 2 -type f \( -name "pyproject.toml" -o -name "setup.py" -o -name "requirements.txt" \) 2>/dev/null | head -20Repository: wecode-ai/Wegent
Length of output: 261
🏁 Script executed:
find . -type f -name "chat_service.py" 2>/dev/null | head -5Repository: wecode-ai/Wegent
Length of output: 103
🏁 Script executed:
cat ./backend/pyproject.tomlRepository: wecode-ai/Wegent
Length of output: 585
🏁 Script executed:
cat ./backend/requirements.txtRepository: wecode-ai/Wegent
Length of output: 1371
🏁 Script executed:
sed -n '420,440p' ./backend/app/services/chat/chat_service.pyRepository: wecode-ai/Wegent
Length of output: 762
🏁 Script executed:
sed -n '400,445p' ./backend/app/services/chat/chat_service.pyRepository: wecode-ai/Wegent
Length of output: 1726
🏁 Script executed:
sed -n '409,500p' ./backend/app/services/chat/chat_service.pyRepository: wecode-ai/Wegent
Length of output: 3239
🏁 Script executed:
rg -A 5 "_update_subtask_status" ./backend/app/services/chat/chat_service.py | head -50Repository: wecode-ai/Wegent
Length of output: 1966
Update SQLAlchemy Query pattern only; keep asyncio.get_event_loop().
The db.query(Subtask).get(subtask_id) pattern is deprecated in SQLAlchemy 2.0. Use db.get(Subtask, subtask_id) instead.
However, asyncio.get_event_loop() is correct here—it retrieves the event loop from the current async context to schedule the synchronous _update() function in a thread pool via run_in_executor(). Using asyncio.get_running_loop() would fail because the thread pool callback runs outside the event loop context.
loop = asyncio.get_event_loop()
def _update():
from app.db.session import SessionLocal
from app.models.subtask import Subtask, SubtaskStatus
db = SessionLocal()
try:
- subtask = db.query(Subtask).get(subtask_id)
+ subtask = db.get(Subtask, subtask_id)🤖 Prompt for AI Agents
In backend/app/services/chat/chat_service.py around lines 427 to 435, the code
uses the deprecated SQLAlchemy pattern db.query(Subtask).get(subtask_id);
replace that call with db.get(Subtask, subtask_id) to use the SQLAlchemy 2.0
API, leaving the existing asyncio.get_event_loop() usage unchanged so the
synchronous _update() can still be scheduled with run_in_executor; ensure any
necessary imports remain and update any similar query.get(...) usages in this
scope to the new db.get(Model, pk) form.
| if (!response.ok) { | ||
| const errorText = await response.text(); | ||
| let errorMsg = errorText; | ||
| try { | ||
| const json = JSON.parse(errorText); | ||
| if (json && typeof json.detail === 'string') { | ||
| errorMsg = json.detail; | ||
| } | ||
| } catch { | ||
| // Not JSON, use original text | ||
| } | ||
| throw new Error(errorMsg); | ||
| } |
There was a problem hiding this comment.
Missing 401 authentication handling.
Unlike the existing client.ts (lines 21-79 in snippets), this error handling doesn't redirect to login on 401 responses. Users with expired tokens will see a generic error instead of being redirected.
Consider adding 401 handling consistent with the existing API client:
if (!response.ok) {
+ if (response.status === 401) {
+ // Handle auth failure - redirect to login
+ removeToken();
+ if (typeof window !== 'undefined') {
+ window.location.href = '/login';
+ }
+ throw new Error('Authentication failed');
+ }
const errorText = await response.text();
// ... rest of error handling
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/apis/chat.ts around lines 115 to 127, the current error handling
parses response text but does not treat HTTP 401 specially; update this block to
detect response.status === 401 and handle authentication failure the same way as
client.ts (clear stored auth/token and redirect to the login page or call the
centralized auth-failure handler), otherwise continue parsing the error body as
now and throw the constructed error; ensure the 401 branch runs before JSON
parsing so expired tokens trigger an immediate logout/redirect.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
frontend/src/apis/chat.ts (1)
117-129: Missing 401 authentication handling.Unlike the existing
client.ts, this error handling doesn't redirect to login on 401 responses. Users with expired tokens will see a generic error instead of being redirected.
🧹 Nitpick comments (21)
frontend/src/apis/attachments.ts (1)
17-39: Consider importing shared types from@/types/apito avoid duplication.
AttachmentStatusis already defined infrontend/src/types/api.ts(line 396), andAttachmentResponse/AttachmentDetailResponselargely overlap with theAttachmentinterface there. Importing these shared types would ensure consistency and reduce maintenance burden.-/** - * Attachment status enum - */ -export type AttachmentStatus = 'uploading' | 'parsing' | 'ready' | 'failed' - -/** - * Attachment response from API - */ -export interface AttachmentResponse { - id: number - filename: string - file_size: number - mime_type: string - status: AttachmentStatus - text_length?: number | null - error_message?: string | null -} - -/** - * Detailed attachment response - */ -export interface AttachmentDetailResponse extends AttachmentResponse { - subtask_id?: number | null - file_extension: string - created_at: string -} +import type { Attachment, AttachmentStatus } from '@/types/api' + +// Re-export for convenience +export type { AttachmentStatus } + +// Use Attachment as the response type +export type AttachmentResponse = Pick<Attachment, 'id' | 'filename' | 'file_size' | 'mime_type' | 'status' | 'text_length' | 'error_message'> +export type AttachmentDetailResponse = Attachmentfrontend/src/features/tasks/components/AttachmentPreview.tsx (1)
31-37: Download failures are silently logged without user feedback.When the download fails, the error is only logged to
console.error. Consider surfacing this error to the user via a toast notification or similar mechanism, as download failures are actionable events the user should be aware of.+import { toast } from '@/components/ui/use-toast' // or your toast library const handleDownload = useCallback(async () => { try { await downloadAttachment(attachment.id, attachment.filename) } catch (err) { console.error('Failed to download attachment:', err) + toast({ + title: '下载失败', + description: '附件下载失败，请重试', + variant: 'destructive', + }) } }, [attachment.id, attachment.filename])backend/app/services/attachment/attachment_service.py (2)
31-115: Uselogger.exceptioninstead oflogger.errorfor better stack traces.When logging errors in exception handlers,
logger.exceptionautomatically includes the full traceback, which is more helpful for debugging.- except DocumentParseError as e: - logger.error(f"Document parsing failed for attachment {attachment.id}: {e}") + except DocumentParseError as e: + logger.exception(f"Document parsing failed for attachment {attachment.id}") attachment.status = AttachmentStatus.FAILED attachment.error_message = str(e) db.commit() raise
74-89: Consider combining the two flush operations.The status transitions from
UPLOADINGtoPARSINGhappen in quick succession without any meaningful work in between. This double flush adds overhead. TheUPLOADINGstatus is immediately overwritten before any parsing begins.# Create attachment record with PARSING status (upload is synchronous) attachment = SubtaskAttachment( user_id=user_id, original_filename=filename, file_extension=extension, file_size=file_size, mime_type=mime_type, binary_data=binary_data, - status=AttachmentStatus.UPLOADING, + status=AttachmentStatus.PARSING, ) db.add(attachment) db.flush() # Get the ID - - # Update status to PARSING - attachment.status = AttachmentStatus.PARSING - db.flush()If the
UPLOADINGstatus is intended to be visible to the frontend during an async upload, consider whether this synchronous implementation matches that design.backend/alembic/versions/add_subtask_attachments_table.py (1)
50-50: Redundant index on primary key.The
ix_subtask_attachments_idindex on theidcolumn is unnecessary since primary keys are automatically indexed by the database engine.- op.create_index('ix_subtask_attachments_id', 'subtask_attachments', ['id'])Also remove the corresponding drop in the downgrade:
- op.drop_index('ix_subtask_attachments_id', table_name='subtask_attachments')frontend/src/hooks/useAttachment.ts (1)
106-123: Consider using server-providedfile_extensionandcreated_at.The
file_extension(line 117) is derived client-side fromfile.name, andcreated_at(line 118) uses the client's timestamp. If the server response includes these values, prefer using them for consistency with server state.attachment: { id: attachment.id, filename: attachment.filename, file_size: attachment.file_size, mime_type: attachment.mime_type, status: attachment.status, text_length: attachment.text_length, error_message: attachment.error_message, subtask_id: null, - file_extension: file.name.substring(file.name.lastIndexOf('.')), - created_at: new Date().toISOString(), + file_extension: attachment.file_extension ?? file.name.substring(file.name.lastIndexOf('.')), + created_at: attachment.created_at ?? new Date().toISOString(), },backend/app/services/attachment/parser.py (2)
123-128: Preserve exception chain usingraise ... from e.When re-raising exceptions, use
raise ... from eto preserve the original traceback. This applies to multiple locations flagged by static analysis (lines 127, 153, 191, 233, 271, 293, 306).Example fix for line 127:
except Exception as e: logger.error(f"Error parsing document: {e}", exc_info=True) - raise DocumentParseError(f"Failed to parse document: {str(e)}") + raise DocumentParseError(f"Failed to parse document: {e!s}") from eApply the same pattern (
from eand{e!s}instead of{str(e)}) to:
- Line 153 (
_parse_pdf)- Line 191 (
_parse_word)- Line 233 (
_parse_powerpoint)- Line 271 (
_parse_excel)- Line 293 (
_parse_csv)- Line 306 (
_parse_text)
41-52: Annotate mutable class attribute withClassVar.Per Ruff RUF012, mutable class attributes should be annotated with
typing.ClassVarto indicate they belong to the class, not instances.+from typing import ClassVar, Tuple -from typing import Tuple class DocumentParser: # Supported file extensions and their MIME types - SUPPORTED_EXTENSIONS = { + SUPPORTED_EXTENSIONS: ClassVar[dict[str, str]] = { ".pdf": "application/pdf", ... }frontend/src/features/tasks/components/ChatArea.tsx (2)
60-61: Remove debug console.log statements before production.Multiple
console.logstatements are present throughout the component (lines 60, 176, 181, 186-189, 195-199, 278, 287, 342-348, 351). Consider removing or replacing with a proper logging utility that can be disabled in production.- if (initialTeamIdRef.current === null && typeof window !== 'undefined') { - initialTeamIdRef.current = getLastTeamId(); - console.log('[ChatArea] Pre-loaded team ID from localStorage:', initialTeamIdRef.current); - } + if (initialTeamIdRef.current === null && typeof window !== 'undefined') { + initialTeamIdRef.current = getLastTeamId(); + }
694-735: Consider extracting duplicated FileUpload sections into a reusable component.The FileUpload preview (lines 696-708, 877-890) and FileUpload button (lines 724-735, 906-917) logic is duplicated between the "no messages" and "has messages" views. Extracting these into a shared component or render function would reduce duplication and improve maintainability.
Example extraction:
const renderFileUploadPreview = () => ( attachmentState.attachment || attachmentState.isUploading || attachmentState.error ? ( <div className="px-3 pt-2"> <FileUpload attachment={attachmentState.attachment} isUploading={attachmentState.isUploading} uploadProgress={attachmentState.uploadProgress} error={attachmentState.error} disabled={isLoading || isStreaming} onFileSelect={handleFileSelect} onRemove={handleAttachmentRemove} /> </div> ) : null )Also applies to: 877-917
backend/app/api/endpoints/adapter/chat.py (8)
76-76: Remove extraneousfprefix from strings without placeholders.Lines 76, 91, and 99 use f-strings with no interpolation.
- logger.info(f"[chat.py] _get_shell_type: user shell not found, checking public shells") + logger.info("[chat.py] _get_shell_type: user shell not found, checking public shells")- logger.info(f"[chat.py] _get_shell_type: public shell not found either") + logger.info("[chat.py] _get_shell_type: public shell not found either")- logger.info(f"[chat.py] _get_shell_type: shell has no json") + logger.info("[chat.py] _get_shell_type: shell has no json")Also applies to: 91-91, 99-99
77-77: Consider moving import to module level.The
PublicShellimport inside the function body works but is unconventional. Moving it to the top with other imports improves readability and follows Python conventions.
103-103: Unuseduser_idparameter creates confusion.The function accepts
user_idbut usesteam.user_idinstead (line 132). Either remove the unused parameter or use it consistently:-def _should_use_direct_chat(db: Session, team: Kind, user_id: int) -> bool: +def _should_use_direct_chat(db: Session, team: Kind) -> bool:Or if the intent is to check from the current user's perspective:
- shell_type = _get_shell_type(db, bot, team.user_id) + shell_type = _get_shell_type(db, bot, user_id)Also applies to: 132-132
144-151: Consider breaking down this long function.At ~200 lines, this function exceeds the 50-line guideline. Consider extracting logical sections into helper functions:
_get_bot_ids_from_team()_create_new_task()_create_subtasks()This would improve testability and readability.
308-308: Remove extraneousfprefix from strings without placeholders.- title=f"User message", + title="User message",- title=f"Assistant response", + title="Assistant response",Also applies to: 331-331
481-482: Chain exception for better debugging.Use
raise ... from eto preserve the exception chain:except ValueError as e: - raise HTTPException(status_code=400, detail=str(e)) + raise HTTPException(status_code=400, detail=str(e)) from e
491-494: Remove unused variablebot_crd.The variable is assigned but never used. Either remove it or use it for the intended purpose.
- # Build data_sources for placeholder replacement in DEFAULT_HEADERS - # This mirrors the executor's member_builder.py logic - bot_crd = Bot.model_validate(bot.json) bot_json = bot.json or {}
533-534: Move imports to module level.The
jsonandStreamingResponseimports should be at the top of the file with other imports.backend/app/api/endpoints/adapter/attachments.py (3)
82-86: Improve exception handling for file read errors.Use
logging.exceptionand chain the exception for better debugging:try: binary_data = await file.read() except Exception as e: - logger.error(f"Error reading uploaded file: {e}") - raise HTTPException(status_code=400, detail="Failed to read uploaded file") + logger.exception("Error reading uploaded file") + raise HTTPException(status_code=400, detail="Failed to read uploaded file") from e
114-120: Chain exceptions for better stack traces.except ValueError as e: - raise HTTPException(status_code=400, detail=str(e)) + raise HTTPException(status_code=400, detail=str(e)) from e except DocumentParseError as e: - raise HTTPException(status_code=400, detail=str(e)) + raise HTTPException(status_code=400, detail=str(e)) from e except Exception as e: logger.error(f"Error uploading attachment: {e}", exc_info=True) - raise HTTPException(status_code=500, detail="Failed to upload attachment") + raise HTTPException(status_code=500, detail="Failed to upload attachment") from e
179-185: Non-ASCII filenames may cause issues in Content-Disposition header.The current implementation may break with filenames containing special characters or non-ASCII text. Consider using RFC 5987 encoding:
+from urllib.parse import quote + return Response( content=attachment.binary_data, media_type=attachment.mime_type, headers={ - "Content-Disposition": f'attachment; filename="{attachment.original_filename}"' + "Content-Disposition": f"attachment; filename*=UTF-8''{quote(attachment.original_filename)}" } )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
backend/alembic/versions/add_subtask_attachments_table.py(1 hunks)backend/app/api/api.py(2 hunks)backend/app/api/endpoints/adapter/attachments.py(1 hunks)backend/app/api/endpoints/adapter/chat.py(1 hunks)backend/app/models/subtask_attachment.py(1 hunks)backend/app/services/attachment/__init__.py(1 hunks)backend/app/services/attachment/attachment_service.py(1 hunks)backend/app/services/attachment/parser.py(1 hunks)backend/requirements.txt(1 hunks)frontend/src/apis/attachments.ts(1 hunks)frontend/src/apis/chat.ts(1 hunks)frontend/src/features/tasks/components/AttachmentPreview.tsx(1 hunks)frontend/src/features/tasks/components/ChatArea.tsx(16 hunks)frontend/src/features/tasks/components/FileUpload.tsx(1 hunks)frontend/src/hooks/useAttachment.ts(1 hunks)frontend/src/types/api.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{py,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
All code comments, inline comments, block comments, docstrings, TODO/FIXME annotations, and type hints descriptions MUST be written in English
Files:
frontend/src/types/api.tsbackend/app/services/attachment/attachment_service.pyfrontend/src/features/tasks/components/FileUpload.tsxfrontend/src/features/tasks/components/AttachmentPreview.tsxfrontend/src/apis/attachments.tsbackend/app/models/subtask_attachment.pybackend/app/services/attachment/__init__.pyfrontend/src/hooks/useAttachment.tsbackend/app/api/endpoints/adapter/attachments.pyfrontend/src/apis/chat.tsfrontend/src/features/tasks/components/ChatArea.tsxbackend/alembic/versions/add_subtask_attachments_table.pybackend/app/api/endpoints/adapter/chat.pybackend/app/services/attachment/parser.pybackend/app/api/api.py
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: TypeScript code MUST use strict mode with type checking enabled
TypeScript/React code MUST use Prettier formatter with single quotes, no semicolons
TypeScript/React code MUST pass ESLint with Next.js configuration
React component names MUST use PascalCase convention
Files:
frontend/src/types/api.tsfrontend/src/features/tasks/components/FileUpload.tsxfrontend/src/features/tasks/components/AttachmentPreview.tsxfrontend/src/apis/attachments.tsfrontend/src/hooks/useAttachment.tsfrontend/src/apis/chat.tsfrontend/src/features/tasks/components/ChatArea.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: React components MUST use functional components with hooks instead of class-based components
Useconstoverlet, never usevarin TypeScript/JavaScript code
Files:
frontend/src/types/api.tsfrontend/src/features/tasks/components/FileUpload.tsxfrontend/src/features/tasks/components/AttachmentPreview.tsxfrontend/src/apis/attachments.tsfrontend/src/hooks/useAttachment.tsfrontend/src/apis/chat.tsfrontend/src/features/tasks/components/ChatArea.tsx
**/types/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript type definitions MUST be organized in
src/types/directory
Files:
frontend/src/types/api.ts
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code MUST be PEP 8 compliant with Black formatter (line length: 88) and isort for import organization
Python code MUST include type hints for all functions and variables
Python functions SHOULD NOT exceed 50 lines (preferred maximum)
Python functions and classes MUST have descriptive names and public functions/classes MUST include docstrings
Python code MUST extract magic numbers to named constants
Files:
backend/app/services/attachment/attachment_service.pybackend/app/models/subtask_attachment.pybackend/app/services/attachment/__init__.pybackend/app/api/endpoints/adapter/attachments.pybackend/alembic/versions/add_subtask_attachments_table.pybackend/app/api/endpoints/adapter/chat.pybackend/app/services/attachment/parser.pybackend/app/api/api.py
**/backend/app/services/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend business logic MUST be placed in
app/services/directory
Files:
backend/app/services/attachment/attachment_service.pybackend/app/services/attachment/__init__.pybackend/app/services/attachment/parser.py
**/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
React component files MUST use kebab-case naming convention
Files:
frontend/src/features/tasks/components/FileUpload.tsxfrontend/src/features/tasks/components/AttachmentPreview.tsxfrontend/src/features/tasks/components/ChatArea.tsx
**/src/**/*.{tsx,jsx,css}
📄 CodeRabbit inference engine (AGENTS.md)
Frontend Tailwind CSS MUST use provided CSS variables for color system (e.g., --color-bg-base, --color-text-primary, --color-primary)
Files:
frontend/src/features/tasks/components/FileUpload.tsxfrontend/src/features/tasks/components/AttachmentPreview.tsxfrontend/src/features/tasks/components/ChatArea.tsx
**/src/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/src/**/*.{tsx,jsx}: Frontend responsive design MUST follow mobile-first approach with Tailwind breakpoints
Frontend React forms MUST use react-hook-form with zod validation schema
Frontend components MUST use shadcn/ui component library fromfrontend/src/components/ui/
Files:
frontend/src/features/tasks/components/FileUpload.tsxfrontend/src/features/tasks/components/AttachmentPreview.tsxfrontend/src/features/tasks/components/ChatArea.tsx
**/backend/app/models/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend SQLAlchemy models MUST be created in
app/models/directory
Files:
backend/app/models/subtask_attachment.py
**/backend/app/api/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend API route handlers MUST be created in
app/api/directory
Files:
backend/app/api/endpoints/adapter/attachments.pybackend/app/api/endpoints/adapter/chat.pybackend/app/api/api.py
**/backend/alembic/versions/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend Alembic database migrations MUST be reviewed before applying, especially auto-generated migrations
Files:
backend/alembic/versions/add_subtask_attachments_table.py
🧠 Learnings (1)
📚 Learning: 2025-12-01T06:44:24.499Z
Learnt from: CR
Repo: wecode-ai/Wegent PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T06:44:24.499Z
Learning: Applies to **/backend/alembic/versions/*.py : Backend Alembic database migrations MUST be reviewed before applying, especially auto-generated migrations
Applied to files:
backend/alembic/versions/add_subtask_attachments_table.py
🧬 Code graph analysis (10)
backend/app/services/attachment/attachment_service.py (2)
backend/app/models/subtask_attachment.py (2)
AttachmentStatus(20-25)SubtaskAttachment(28-82)backend/app/services/attachment/parser.py (6)
DocumentParser(27-306)DocumentParseError(22-24)is_supported_extension(61-63)validate_file_size(71-73)get_mime_type(66-68)parse(80-127)
frontend/src/features/tasks/components/FileUpload.tsx (5)
frontend/src/types/api.ts (1)
Attachment(399-410)frontend/src/apis/attachments.ts (6)
isSupportedExtension(81-84)isValidFileSize(89-91)SUPPORTED_EXTENSIONS(44-55)MAX_FILE_SIZE(76-76)getFileIcon(116-137)formatFileSize(103-111)frontend/src/components/ui/tooltip.tsx (4)
TooltipProvider(34-34)Tooltip(34-34)TooltipTrigger(34-34)TooltipContent(34-34)frontend/src/components/ui/button.tsx (1)
Button(55-55)frontend/src/components/ui/progress.tsx (1)
Progress(23-23)
frontend/src/features/tasks/components/AttachmentPreview.tsx (2)
frontend/src/types/api.ts (1)
Attachment(399-410)frontend/src/apis/attachments.ts (3)
downloadAttachment(254-283)getFileIcon(116-137)formatFileSize(103-111)
frontend/src/apis/attachments.ts (2)
frontend/src/types/api.ts (1)
AttachmentStatus(397-397)frontend/src/apis/user.ts (1)
getToken(54-59)
backend/app/models/subtask_attachment.py (2)
frontend/src/apis/attachments.ts (1)
AttachmentStatus(17-17)frontend/src/types/api.ts (1)
AttachmentStatus(397-397)
backend/app/services/attachment/__init__.py (1)
backend/app/services/attachment/parser.py (1)
DocumentParser(27-306)
backend/app/api/endpoints/adapter/attachments.py (3)
backend/app/models/subtask_attachment.py (1)
AttachmentStatus(20-25)backend/app/services/attachment/parser.py (3)
DocumentParseError(22-24)DocumentParser(27-306)validate_file_size(71-73)backend/app/services/attachment/attachment_service.py (4)
upload_attachment(31-115)get_attachment(117-141)delete_attachment(194-230)get_attachment_by_subtask(143-160)
frontend/src/apis/chat.ts (2)
frontend/src/apis/client.ts (1)
request(22-80)frontend/src/apis/user.ts (1)
getToken(54-59)
backend/app/api/endpoints/adapter/chat.py (7)
backend/app/models/kind.py (1)
Kind(25-42)backend/app/services/chat/base.py (2)
ChatServiceBase(78-99)is_direct_chat_shell(89-99)backend/app/services/chat/model_resolver.py (3)
build_default_headers_with_placeholders(173-202)get_bot_system_prompt(408-457)get_model_config_for_bot(205-278)backend/app/models/public_shell.py (1)
PublicShell(19-38)backend/app/models/subtask_attachment.py (1)
AttachmentStatus(20-25)backend/app/services/attachment/attachment_service.py (3)
get_attachment(117-141)build_message_with_attachment(232-258)link_attachment_to_subtask(162-192)backend/app/services/chat/chat_service.py (1)
chat_stream(50-144)
backend/app/services/attachment/parser.py (1)
frontend/src/apis/attachments.ts (2)
SUPPORTED_EXTENSIONS(44-55)MAX_FILE_SIZE(76-76)
🪛 Ruff (0.14.7)
backend/app/services/attachment/attachment_service.py
60-63: Avoid specifying long messages outside the exception class
(TRY003)
69-69: Avoid specifying long messages outside the exception class
(TRY003)
101-101: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
backend/app/api/endpoints/adapter/attachments.py
54-54: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
55-55: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
56-56: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
84-84: Do not catch blind exception: Exception
(BLE001)
85-85: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
86-86: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
115-115: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
117-117: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
120-120: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
126-126: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
127-127: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
161-161: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
162-162: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
191-191: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
192-192: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
232-232: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
233-233: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
backend/app/api/endpoints/adapter/chat.py
69-69: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
76-76: f-string without any placeholders
Remove extraneous f prefix
(F541)
83-83: Avoid equality comparisons to True; use PublicShell.is_active: for truth checks
Replace with PublicShell.is_active
(E712)
91-91: f-string without any placeholders
Remove extraneous f prefix
(F541)
99-99: f-string without any placeholders
Remove extraneous f prefix
(F541)
103-103: Unused function argument: user_id
(ARG001)
123-123: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
129-129: f-string without any placeholders
Remove extraneous f prefix
(F541)
140-140: f-string without any placeholders
Remove extraneous f prefix
(F541)
170-170: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
190-190: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
308-308: f-string without any placeholders
Remove extraneous f prefix
(F541)
331-331: f-string without any placeholders
Remove extraneous f prefix
(F541)
357-357: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
358-358: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
384-384: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
464-464: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
482-482: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
491-491: Local variable bot_crd is assigned to but never used
Remove assignment to unused variable bot_crd
(F841)
573-573: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
574-574: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
587-587: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
609-609: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
backend/app/services/attachment/parser.py
41-52: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
97-97: Avoid specifying long messages outside the exception class
(TRY003)
113-113: Abstract raise to an inner function
(TRY301)
113-113: Avoid specifying long messages outside the exception class
(TRY003)
117-119: Abstract raise to an inner function
(TRY301)
117-119: Avoid specifying long messages outside the exception class
(TRY003)
127-127: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
127-127: Avoid specifying long messages outside the exception class
(TRY003)
127-127: Use explicit conversion flag
Replace with conversion flag
(RUF010)
139-139: Abstract raise to an inner function
(TRY301)
139-139: Avoid specifying long messages outside the exception class
(TRY003)
153-153: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
153-153: Avoid specifying long messages outside the exception class
(TRY003)
153-153: Use explicit conversion flag
Replace with conversion flag
(RUF010)
161-163: Abstract raise to an inner function
(TRY301)
161-163: Avoid specifying long messages outside the exception class
(TRY003)
191-191: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
191-191: Avoid specifying long messages outside the exception class
(TRY003)
191-191: Use explicit conversion flag
Replace with conversion flag
(RUF010)
197-199: Abstract raise to an inner function
(TRY301)
197-199: Avoid specifying long messages outside the exception class
(TRY003)
233-233: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
233-233: Avoid specifying long messages outside the exception class
(TRY003)
233-233: Use explicit conversion flag
Replace with conversion flag
(RUF010)
239-241: Abstract raise to an inner function
(TRY301)
239-241: Avoid specifying long messages outside the exception class
(TRY003)
271-271: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
271-271: Avoid specifying long messages outside the exception class
(TRY003)
271-271: Use explicit conversion flag
Replace with conversion flag
(RUF010)
293-293: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
293-293: Avoid specifying long messages outside the exception class
(TRY003)
293-293: Use explicit conversion flag
Replace with conversion flag
(RUF010)
306-306: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
306-306: Avoid specifying long messages outside the exception class
(TRY003)
306-306: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-tests
🔇 Additional comments (27)
backend/requirements.txt (1)
87-94: LGTM!The document parsing dependencies are appropriate for the attachment feature. The version constraints align with the backend parser implementation that supports PDF, Word, PowerPoint, Excel, and text file extraction.
frontend/src/apis/attachments.ts (2)
146-209: LGTM!The upload function correctly uses
XMLHttpRequestfor progress tracking, validates files before upload, and handles various error scenarios (network, parsing, HTTP errors) appropriately.
254-283: LGTM!The download function properly creates a blob URL, triggers the download via a temporary anchor element, and cleans up by revoking the URL to prevent memory leaks.
frontend/src/types/api.ts (1)
395-418: LGTM!The attachment type definitions are well-structured and provide good coverage for the attachment lifecycle.
AttachmentUploadStateis a useful pattern for managing the upload UI state in components.backend/app/services/attachment/__init__.py (1)
1-12: LGTM!Clean package initialization that provides a well-defined public API. The
__all__export list correctly exposes the main components needed by consumers.frontend/src/features/tasks/components/AttachmentPreview.tsx (1)
41-84: LGTM!The component correctly implements two display modes with appropriate styling. Good use of semantic HTML with accessibility attributes (
titlefor tooltips). The conditional rendering logic is clear and handles both compact and full modes properly.frontend/src/features/tasks/components/FileUpload.tsx (2)
111-148: LGTM!Good implementation of the upload button with tooltip providing helpful information about supported file types and size limits. The
TooltipProviderwrapper and proper ARIA attributes improve accessibility.
161-199: LGTM!The attachment preview section handles all status states (
ready,failed,parsing) with appropriate visual feedback. The dark mode styling is properly considered withdark:prefixes for the error state.backend/app/api/api.py (2)
6-17: LGTM!The new
attachmentsandchatmodules are properly imported alongside existing adapter modules.
31-32: LGTM!The new routers are registered following the established pattern with appropriate prefixes and tags for API documentation grouping.
backend/app/services/attachment/attachment_service.py (3)
117-161: LGTM!The retrieval methods are well-implemented with proper optional ownership filtering. Query construction is clean and follows SQLAlchemy best practices.
194-230: LGTM!Good defensive check preventing deletion of attachments already linked to subtasks. The logging provides adequate audit trail for deletion operations.
232-262: LGTM!The message construction properly handles the case where no extracted text is available. The structured format with clear section headers makes the combined message readable for both users and the LLM.
backend/app/models/subtask_attachment.py (1)
28-82: LGTM overall.The model is well-structured with proper docstrings, type annotations via SQLAlchemy columns, consistent enum usage matching frontend types, and appropriate table configuration for multiple database dialects.
backend/app/services/attachment/parser.py (1)
27-78: LGTM on overall design.The parser architecture is well-structured with clear separation of concerns per format, proper validation, lazy imports for optional dependencies, and comprehensive error handling. The supported extensions and size limits match the frontend definitions.
frontend/src/features/tasks/components/ChatArea.tsx (2)
115-147: LGTM on streaming integration.The
useChatStreamhook integration is well-implemented with proper lifecycle handling: clearing optimistic UI on completion/error, resetting attachments, refreshing tasks, and updating the URL for new tasks.
325-407: LGTM on Chat Shell streaming flow.The streaming path correctly implements optimistic UI updates with
pendingUserMessage, handles model ID resolution, passes attachment IDs, and gracefully handles errors by clearing the optimistic state.backend/alembic/versions/add_subtask_attachments_table.py (1)
20-20: Thedown_revision = 'a1b2c3d4e5f6'is valid and references an existing migration filea1b2c3d4e5f6_add_auth_source_to_users.py.Likely an incorrect or invalid review comment.
frontend/src/apis/chat.ts (3)
1-16: LGTM!License header, imports, and constant definition are properly structured.
17-79: LGTM!Interface definitions are well-typed with appropriate optional fields and clear JSDoc documentation.
254-260: LGTM!Clean export pattern aggregating the API functions.
backend/app/api/endpoints/adapter/chat.py (3)
1-36: LGTM!Imports are well-organized and the module structure follows the codebase patterns.
38-53: LGTM!Request model is well-defined with proper type hints and defaults.
570-619: LGTM!The
check_direct_chatendpoint is well-implemented with proper validation and error handling.backend/app/api/endpoints/adapter/attachments.py (3)
1-50: LGTM!Response models are well-defined with proper Pydantic v2 configuration.
188-227: LGTM!Delete endpoint properly validates ownership and prevents deletion of linked attachments.
229-264: LGTM!The endpoint correctly handles the optional attachment case and properly verifies ownership before returning details.
| # Foreign key to users table | ||
| user_id = Column(Integer, nullable=False, index=True) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if users table and model exist
rg -n "class User\(" backend/app/models/
rg -n "__tablename__.*=.*['\"]users['\"]" backend/app/models/Repository: wecode-ai/Wegent
Length of output: 165
🏁 Script executed:
# Read the full subtask_attachment.py file to see all column definitions
wc -l backend/app/models/subtask_attachment.pyRepository: wecode-ai/Wegent
Length of output: 103
🏁 Script executed:
# Get the full content to see subtask_id and user_id definitions
cat -n backend/app/models/subtask_attachment.pyRepository: wecode-ai/Wegent
Length of output: 3038
Add ForeignKey constraint for user_id to the users table.
The comment on line 48 indicates user_id is a foreign key, but the column definition lacks the actual ForeignKey constraint. The subtask_id column (line 41-46) demonstrates the proper pattern with ForeignKey("subtasks.id", ondelete="CASCADE"). Since the User model exists, add the constraint:
user_id = Column(
Integer,
ForeignKey("users.id", ondelete="CASCADE"),
nullable=False,
index=True
)This ensures referential integrity and defines cascade behavior when users are deleted.
🤖 Prompt for AI Agents
In backend/app/models/subtask_attachment.py around lines 48 to 49, the user_id
column is documented as a foreign key but lacks the ForeignKey constraint;
update the Column definition to include ForeignKey("users.id",
ondelete="CASCADE") while keeping Integer, nullable=False and index=True so it
matches the pattern used for subtask_id and enforces referential integrity with
cascade on user deletion.
| export function isSupportedExtension(filename: string): boolean { | ||
| const ext = filename.toLowerCase().substring(filename.lastIndexOf('.')) | ||
| return SUPPORTED_EXTENSIONS.includes(ext) | ||
| } | ||
|
|
||
| /** | ||
| * Check if file size is within limits | ||
| */ | ||
| export function isValidFileSize(size: number): boolean { | ||
| return size <= MAX_FILE_SIZE | ||
| } | ||
|
|
||
| /** | ||
| * Get file extension from filename | ||
| */ | ||
| export function getFileExtension(filename: string): string { | ||
| return filename.toLowerCase().substring(filename.lastIndexOf('.')) | ||
| } |
There was a problem hiding this comment.
Edge case: filenames without extensions return incorrect values.
If the filename has no dot (e.g., "README"), lastIndexOf('.') returns -1, and substring(-1) returns the last character of the string, which is incorrect.
export function isSupportedExtension(filename: string): boolean {
- const ext = filename.toLowerCase().substring(filename.lastIndexOf('.'))
+ const lastDot = filename.lastIndexOf('.')
+ if (lastDot === -1) return false
+ const ext = filename.toLowerCase().substring(lastDot)
return SUPPORTED_EXTENSIONS.includes(ext)
}
...
export function getFileExtension(filename: string): string {
- return filename.toLowerCase().substring(filename.lastIndexOf('.'))
+ const lastDot = filename.lastIndexOf('.')
+ if (lastDot === -1) return ''
+ return filename.toLowerCase().substring(lastDot)
}🤖 Prompt for AI Agents
In frontend/src/apis/attachments.ts around lines 81 to 98, the current
getFileExtension and isSupportedExtension use
filename.substring(filename.lastIndexOf('.')) which returns incorrect results
for names without a dot (lastIndexOf returns -1); change the logic to compute
idx = filename.lastIndexOf('.') after trimming and lowercasing the name, then if
idx <= 0 (no dot or dot as first char) return an empty string, otherwise return
filename.substring(idx), and update isSupportedExtension to treat empty
extension as unsupported; leave isValidFileSize unchanged.
| let taskId = request.task_id || 0; | ||
| let subtaskId = 0; |
There was a problem hiding this comment.
Race condition: taskId returned before stream updates it.
The function returns taskId immediately after initiating the stream, but taskId is updated inside the detached IIFE when the first message arrives. Callers receive the initial value (0 or request.task_id) rather than the actual task ID from the server.
Consider restructuring to wait for the first message before returning, or document that callers should rely on onComplete for the actual IDs:
- let taskId = request.task_id || 0;
- let subtaskId = 0;
+ // Note: taskId/subtaskId are updated in the stream processor.
+ // The returned taskId is a best-effort initial value; use onComplete for accurate IDs.
+ let taskId = request.task_id || 0;
+ let subtaskId = 0;Alternatively, refactor to return a Promise that resolves after the first message with IDs is received.
Also applies to: 207-210
🤖 Prompt for AI Agents
In frontend/src/apis/chat.ts around lines 137-138 (and similarly at 207-210),
the code assigns taskId = request.task_id || 0 and immediately returns that
value while the real taskId is set later inside a detached IIFE when the first
stream message arrives, causing a race where callers get the stale ID; change
the function to wait for the first streamed message before returning (or convert
it to return a Promise) by moving the resolution of the function into the
stream's first-message handler: create a Promise that resolves with the actual
taskId/subtaskId when the first message arrives (or await that Promise if the
function is async), set taskId/subtaskId from the first message there, then
resolve/return so callers receive the correct IDs (also apply same change to
lines 207-210).
| if (!response.ok) { | ||
| const errorText = await response.text(); | ||
| let errorMsg = errorText; | ||
| try { | ||
| const json = JSON.parse(errorText); | ||
| if (json && typeof json.detail === 'string') { | ||
| errorMsg = json.detail; | ||
| } | ||
| } catch { | ||
| // Not JSON | ||
| } | ||
| throw new Error(errorMsg); |
There was a problem hiding this comment.
Same 401 handling gap applies here.
This function also lacks 401 authentication handling consistent with the existing client.ts pattern.
🤖 Prompt for AI Agents
In frontend/src/apis/chat.ts around lines 237-248, the error branch parses
response text and throws an Error but does not handle HTTP 401 like client.ts;
update this block to first check if response.status === 401 and invoke the
shared unauthorized handler used in client.ts (reuse the existing function or
import it from client.ts) before constructing/throwing the error; preserve the
existing JSON parsing for error.detail and then throw the error after the
unauthorized handler runs so 401s trigger the centralized auth flow
(sign-out/redirect/refresh) consistently.
| const handleDrop = useCallback( | ||
| (e: React.DragEvent) => { | ||
| e.preventDefault() | ||
| e.stopPropagation() | ||
|
|
||
| if (disabled || isUploading || attachment) return | ||
|
|
||
| const file = e.dataTransfer.files?.[0] | ||
| if (file) { | ||
| onFileSelect(file) | ||
| } | ||
| }, | ||
| [disabled, isUploading, attachment, onFileSelect] | ||
| ) |
There was a problem hiding this comment.
Drag-and-drop bypasses file validation, unlike click-to-upload.
The handleDrop function passes the file directly to onFileSelect without validating extension or size, while handleFileChange (click-to-upload) performs these validations. This inconsistency could lead to different error handling paths for the same invalid file depending on how it was selected.
const handleDrop = useCallback(
(e: React.DragEvent) => {
e.preventDefault()
e.stopPropagation()
if (disabled || isUploading || attachment) return
const file = e.dataTransfer.files?.[0]
if (file) {
+ // Validate before passing to parent (consistent with handleFileChange)
+ if (!isSupportedExtension(file.name) || !isValidFileSize(file.size)) {
+ // Let parent handle the error
+ onFileSelect(file)
+ return
+ }
onFileSelect(file)
}
},
[disabled, isUploading, attachment, onFileSelect]
)Alternatively, if validation is meant to be done by the parent, remove the validation from handleFileChange for consistency.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/features/tasks/components/FileUpload.tsx around lines 90 to 103,
handleDrop currently forwards the dropped file straight to onFileSelect,
bypassing the same extension/size validation that handleFileChange applies; make
these paths consistent by performing the same validation before calling
onFileSelect (extract the validation logic into a shared helper or call the
existing validator used by handleFileChange), and ensure validation errors are
surfaced the same way (set error state or call the same error handler).
Alternatively, if validation should live in the parent, remove local validation
from handleFileChange and document that onFileSelect must validate, keeping both
drop and click paths symmetrical.
| const handleRemove = useCallback(async () => { | ||
| const attachmentId = state.attachment?.id | ||
|
|
||
| // Reset state immediately for better UX | ||
| setState({ | ||
| file: null, | ||
| attachment: null, | ||
| isUploading: false, | ||
| uploadProgress: 0, | ||
| error: null, | ||
| }) | ||
|
|
||
| // Try to delete from server if it exists and is not linked to a subtask | ||
| if (attachmentId && !state.attachment?.subtask_id) { | ||
| try { | ||
| await deleteAttachment(attachmentId) | ||
| } catch { | ||
| // Ignore delete errors - attachment might already be linked | ||
| } | ||
| } | ||
| }, [state.attachment]) |
There was a problem hiding this comment.
Bug: Stale state check after reset causes condition to always fail.
At line 149, state.attachment?.subtask_id is checked after the state has been reset at lines 140-146. Since the state is already null, this condition will always evaluate to false, meaning the subtask_id check is ineffective.
Capture the subtask_id before resetting state:
const handleRemove = useCallback(async () => {
const attachmentId = state.attachment?.id
+ const subtaskId = state.attachment?.subtask_id
// Reset state immediately for better UX
setState({
file: null,
attachment: null,
isUploading: false,
uploadProgress: 0,
error: null,
})
// Try to delete from server if it exists and is not linked to a subtask
- if (attachmentId && !state.attachment?.subtask_id) {
+ if (attachmentId && !subtaskId) {
try {
await deleteAttachment(attachmentId)
} catch {
// Ignore delete errors - attachment might already be linked
}
}
-}, [state.attachment])
+}, [state.attachment?.id, state.attachment?.subtask_id])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleRemove = useCallback(async () => { | |
| const attachmentId = state.attachment?.id | |
| // Reset state immediately for better UX | |
| setState({ | |
| file: null, | |
| attachment: null, | |
| isUploading: false, | |
| uploadProgress: 0, | |
| error: null, | |
| }) | |
| // Try to delete from server if it exists and is not linked to a subtask | |
| if (attachmentId && !state.attachment?.subtask_id) { | |
| try { | |
| await deleteAttachment(attachmentId) | |
| } catch { | |
| // Ignore delete errors - attachment might already be linked | |
| } | |
| } | |
| }, [state.attachment]) | |
| const handleRemove = useCallback(async () => { | |
| const attachmentId = state.attachment?.id | |
| const subtaskId = state.attachment?.subtask_id | |
| // Reset state immediately for better UX | |
| setState({ | |
| file: null, | |
| attachment: null, | |
| isUploading: false, | |
| uploadProgress: 0, | |
| error: null, | |
| }) | |
| // Try to delete from server if it exists and is not linked to a subtask | |
| if (attachmentId && !subtaskId) { | |
| try { | |
| await deleteAttachment(attachmentId) | |
| } catch { | |
| // Ignore delete errors - attachment might already be linked | |
| } | |
| } | |
| }, [state.attachment?.id, state.attachment?.subtask_id]) |
🤖 Prompt for AI Agents
In frontend/src/hooks/useAttachment.ts around lines 136 to 156, the code resets
state before checking state.attachment?.subtask_id so the condition always
fails; capture attachment id and subtask_id into local constants (e.g., const
attachmentId = state.attachment?.id and const subtaskId =
state.attachment?.subtask_id) before calling setState, then use those local
values in the subsequent deletion conditional and await
deleteAttachment(attachmentId) only if attachmentId exists and subtaskId is
falsy; keep the setState call as-is and leave the useCallback dependency on
state.attachment or replace with a stable dependency if refactoring.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/app/models/subtask_attachment.py (1)
48-49: Add the missing ForeignKey constraint onuser_idto enforce referential integrity.The comment says
user_idis a foreign key, but the column currently lacks aForeignKeyconstraint, unlikesubtask_id. This can lead to orphaned attachments referencing non‑existent users and breaks the documented contract.You can mirror the
subtask_idpattern:- # Foreign key to users table - user_id = Column(Integer, nullable=False, index=True) + # Foreign key to users table + user_id = Column( + Integer, + ForeignKey("users.id", ondelete="CASCADE"), + nullable=False, + index=True, + )
🧹 Nitpick comments (6)
backend/app/models/subtask_attachment.py (3)
28-73: Consider using SQLAlchemy 2.0 typed ORM (Mapped/mapped_column) to satisfy type‑hint guidelines.Project guidelines require type hints for variables; with SQLAlchemy 2.0 you can get both ORM mapping and proper typing via
Mapped[...]. Not blocking, but migrating this model would improve IDE/editor support and align with the guidelines.Pattern example (illustrative, not exhaustive):
-from sqlalchemy import Column, DateTime, ForeignKey, Integer, LargeBinary, String, Text -from sqlalchemy import Enum as SQLEnum +from sqlalchemy import DateTime, ForeignKey, Integer, LargeBinary, String, Text +from sqlalchemy import Enum as SQLEnum +from sqlalchemy.orm import Mapped, mapped_column @@ - id = Column(Integer, primary_key=True, index=True) + id: Mapped[int] = mapped_column(primary_key=True, index=True) @@ - created_at = Column(DateTime, default=func.now()) + created_at: Mapped[datetime] = mapped_column(DateTime, default=func.now())You can then follow the same pattern for the remaining fields.
52-55: Extract literal length limits (255, 20, 100, 500) into named constants.To comply with the “no magic numbers” guideline and make future changes easier, consider defining explicit constants for these column lengths.
For example, near the top of the module:
MAX_FILENAME_LENGTH = 255 MAX_EXTENSION_LENGTH = 20 MAX_MIME_TYPE_LENGTH = 100 MAX_ERROR_MESSAGE_LENGTH = 500Then update the column definitions:
- original_filename = Column(String(255), nullable=False) - file_extension = Column(String(20), nullable=False) - file_size = Column(Integer, nullable=False) # File size in bytes - mime_type = Column(String(100), nullable=False) + original_filename = Column(String(MAX_FILENAME_LENGTH), nullable=False) + file_extension = Column(String(MAX_EXTENSION_LENGTH), nullable=False) + file_size = Column(Integer, nullable=False) # File size in bytes + mime_type = Column(String(MAX_MIME_TYPE_LENGTH), nullable=False) @@ - error_message = Column(String(500), nullable=True) + error_message = Column(String(MAX_ERROR_MESSAGE_LENGTH), nullable=True)Also applies to: 70-70
57-59: Validate whether storing full binary attachment data in this table is acceptable at expected scale.Using
LargeBinaryforbinary_datacentralizes everything in one table, but for large or numerous uploads it can bloat the primary DB and impact backup/restore and query performance. If you expect big files or high volume, consider an object store (S3, etc.) with this table holding metadata and object keys instead.backend/app/services/attachment/parser.py (3)
123-127: Preserve original exceptions when wrapping intoDocumentParseErrorIn
parse()and the private_parse_*helpers you generally:except Exception as e: logger.error(f"Error parsing ...: {e}", exc_info=True) raise DocumentParseError(f"Failed to parse ...: {str(e)}")This drops the original traceback and is what Ruff’s
B904is warning about.Consider chaining the original exception and making the conversion explicit, e.g. for
parse():- except DocumentParseError: - raise - except Exception as e: - logger.error(f"Error parsing document: {e}", exc_info=True) - raise DocumentParseError(f"Failed to parse document: {str(e)}") + except DocumentParseError: + raise + except Exception as exc: + logger.error("Error parsing document", exc_info=True) + raise DocumentParseError(f"Failed to parse document: {exc!s}") from excYou can apply the same pattern to the PDF/Word/PPT/Excel/CSV/text helpers to improve debuggability and satisfy the linter.
Also applies to: 149-153, 187-191, 229-233, 267-271, 291-293, 323-324
300-308: Avoid silent blanketExceptionhandling aroundchardet.detect
_parse_textdoes:try: detected = chardet.detect(binary_data) detected_encoding = detected.get("encoding") ... except Exception: pass # Ignore chardet errorsCatching all
Exceptionand silently ignoring it can hide real bugs (and triggers BLE001/S110). Since this is non-critical, you could at least log at debug level or narrow the except clause to the specific errors you expect fromchardet:except Exception as exc: # narrow if possible logger.debug("chardet.detect() failed, falling back to default encodings: %s", exc)This keeps the fallback behavior while making unexpected failures observable.
54-56: File size validation is already enforced upstream; parse() receives pre-validated input
validate_file_size()is called at two levels beforeparse()executes:
- API endpoint (attachments.py:89) validates immediately after reading the file
- Service layer (attachment_service.py:67) validates again before calling
parse()The current design separates concerns: validation happens at entry points, while
parse()focuses on parsing. This is reasonable since the only observed caller ofparse()always validates first.However, if
parse()may be called directly in the future from other contexts, adding a defensive check would strengthen robustness:def parse(self, binary_data: bytes, extension: str) -> Tuple[str, int]: - extension = extension.lower() + if not self.validate_file_size(len(binary_data)): + raise DocumentParseError( + f"File size exceeds maximum allowed size ({self.MAX_FILE_SIZE // (1024 * 1024)} MB)" + ) + + extension = extension.lower()Consider whether
parse()should be defensive-in-depth or remain a trusted utility expecting validated input.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/models/subtask_attachment.py(1 hunks)backend/app/services/attachment/parser.py(1 hunks)frontend/src/apis/attachments.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/apis/attachments.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{py,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
All code comments, inline comments, block comments, docstrings, TODO/FIXME annotations, and type hints descriptions MUST be written in English
Files:
backend/app/services/attachment/parser.pybackend/app/models/subtask_attachment.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code MUST be PEP 8 compliant with Black formatter (line length: 88) and isort for import organization
Python code MUST include type hints for all functions and variables
Python functions SHOULD NOT exceed 50 lines (preferred maximum)
Python functions and classes MUST have descriptive names and public functions/classes MUST include docstrings
Python code MUST extract magic numbers to named constants
Files:
backend/app/services/attachment/parser.pybackend/app/models/subtask_attachment.py
**/backend/app/services/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend business logic MUST be placed in
app/services/directory
Files:
backend/app/services/attachment/parser.py
**/backend/app/models/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend SQLAlchemy models MUST be created in
app/models/directory
Files:
backend/app/models/subtask_attachment.py
🧬 Code graph analysis (2)
backend/app/services/attachment/parser.py (1)
frontend/src/apis/attachments.ts (2)
SUPPORTED_EXTENSIONS(44-55)MAX_FILE_SIZE(76-76)
backend/app/models/subtask_attachment.py (2)
frontend/src/apis/attachments.ts (1)
AttachmentStatus(17-17)frontend/src/types/api.ts (1)
AttachmentStatus(397-397)
🪛 Ruff (0.14.7)
backend/app/services/attachment/parser.py
41-52: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
97-97: Avoid specifying long messages outside the exception class
(TRY003)
113-113: Abstract raise to an inner function
(TRY301)
113-113: Avoid specifying long messages outside the exception class
(TRY003)
117-119: Abstract raise to an inner function
(TRY301)
117-119: Avoid specifying long messages outside the exception class
(TRY003)
127-127: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
127-127: Avoid specifying long messages outside the exception class
(TRY003)
127-127: Use explicit conversion flag
Replace with conversion flag
(RUF010)
139-139: Abstract raise to an inner function
(TRY301)
139-139: Avoid specifying long messages outside the exception class
(TRY003)
153-153: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
153-153: Avoid specifying long messages outside the exception class
(TRY003)
153-153: Use explicit conversion flag
Replace with conversion flag
(RUF010)
161-163: Abstract raise to an inner function
(TRY301)
161-163: Avoid specifying long messages outside the exception class
(TRY003)
191-191: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
191-191: Avoid specifying long messages outside the exception class
(TRY003)
191-191: Use explicit conversion flag
Replace with conversion flag
(RUF010)
197-199: Abstract raise to an inner function
(TRY301)
197-199: Avoid specifying long messages outside the exception class
(TRY003)
233-233: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
233-233: Avoid specifying long messages outside the exception class
(TRY003)
233-233: Use explicit conversion flag
Replace with conversion flag
(RUF010)
239-241: Abstract raise to an inner function
(TRY301)
239-241: Avoid specifying long messages outside the exception class
(TRY003)
271-271: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
271-271: Avoid specifying long messages outside the exception class
(TRY003)
271-271: Use explicit conversion flag
Replace with conversion flag
(RUF010)
293-293: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
293-293: Avoid specifying long messages outside the exception class
(TRY003)
293-293: Use explicit conversion flag
Replace with conversion flag
(RUF010)
307-308: try-except-pass detected, consider logging the exception
(S110)
307-307: Do not catch blind exception: Exception
(BLE001)
324-324: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
324-324: Avoid specifying long messages outside the exception class
(TRY003)
324-324: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-tests
🔇 Additional comments (1)
backend/app/models/subtask_attachment.py (1)
20-25: AttachmentStatus enum + status column wiring looks correct and matches frontend types.Enum values (
uploading/parsing/ready/failed) align with the frontendAttachmentStatusunion, and usingstr+SQLEnum(..., values_callable=...)ensures the DB stores the string values while Python gets the enum. The default ofAttachmentStatus.UPLOADINGonstatusis sensible for newly created attachments.Also applies to: 65-69
| logger = logging.getLogger(__name__) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type hints for class and module-level variables
logger, SUPPORTED_EXTENSIONS, MAX_FILE_SIZE, MAX_TEXT_LENGTH, and document_parser are currently untyped, which violates the project rule that all variables must have type hints and also triggers Ruff’s RUF012 for mutable class attributes.
You can address this with something along these lines:
-import logging
-from typing import Tuple
+import logging
+from typing import ClassVar, Tuple
-logger = logging.getLogger(__name__)
+logger: logging.Logger = logging.getLogger(__name__)
@@
- # Supported file extensions and their MIME types
- SUPPORTED_EXTENSIONS = {
+ # Supported file extensions and their MIME types
+ SUPPORTED_EXTENSIONS: ClassVar[dict[str, str]] = {
@@
- # Maximum file size (5 MB)
- MAX_FILE_SIZE = 5 * 1024 * 1024
+ # Maximum file size (5 MB)
+ MAX_FILE_SIZE: ClassVar[int] = 5 * 1024 * 1024
@@
- # Maximum extracted text length (50000 characters)
- MAX_TEXT_LENGTH = 50000
+ # Maximum extracted text length (50000 characters)
+ MAX_TEXT_LENGTH: ClassVar[int] = 50000
@@
-# Global parser instance
-document_parser = DocumentParser()
+# Global parser instance
+document_parser: DocumentParser = DocumentParser()As per coding guidelines, Python code must include type hints for all variables.
Also applies to: 41-52, 54-58, 327-328
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/alembic/versions/fix_binary_data_column_size.py (1)
5-13: Document the MySQL database requirement.The migration uses MySQL-specific types (
LONGBLOB,LONGTEXT) but doesn't explicitly state this requirement in the docstring. Consider adding a note that this migration is MySQL-specific and will fail on other databases.Apply this diff to clarify the database requirement:
"""Fix binary_data column size to LONGBLOB for large file support Revision ID: fix_binary_data_size Revises: add_subtask_attachments Create Date: 2025-12-03 +Note: This migration is MySQL-specific and uses LONGBLOB/LONGTEXT types. + This migration changes the binary_data column from BLOB (64KB max) to LONGBLOB (4GB max) to support larger file uploads like PDFs and documents. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/alembic/versions/fix_binary_data_column_size.py(1 hunks)backend/app/models/subtask_attachment.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/models/subtask_attachment.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{py,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
All code comments, inline comments, block comments, docstrings, TODO/FIXME annotations, and type hints descriptions MUST be written in English
Files:
backend/alembic/versions/fix_binary_data_column_size.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code MUST be PEP 8 compliant with Black formatter (line length: 88) and isort for import organization
Python code MUST include type hints for all functions and variables
Python functions SHOULD NOT exceed 50 lines (preferred maximum)
Python functions and classes MUST have descriptive names and public functions/classes MUST include docstrings
Python code MUST extract magic numbers to named constants
Files:
backend/alembic/versions/fix_binary_data_column_size.py
**/backend/alembic/versions/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend Alembic database migrations MUST be reviewed before applying, especially auto-generated migrations
Files:
backend/alembic/versions/fix_binary_data_column_size.py
🧠 Learnings (1)
📚 Learning: 2025-12-01T06:44:24.499Z
Learnt from: CR
Repo: wecode-ai/Wegent PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T06:44:24.499Z
Learning: Applies to **/backend/alembic/versions/*.py : Backend Alembic database migrations MUST be reviewed before applying, especially auto-generated migrations
Applied to files:
backend/alembic/versions/fix_binary_data_column_size.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test Backend (3.10)
- GitHub Check: Test Backend (3.11)
- GitHub Check: e2e-tests
🔇 Additional comments (1)
backend/alembic/versions/fix_binary_data_column_size.py (1)
28-46: Migration is correct for MySQL-only project; no changes needed.This migration appropriately uses MySQL-specific types (
mysql.LONGBLOB(),mysql.LONGTEXT()) because the project is MySQL-only. The database configuration throughout the backend (env.py, config.py, session.py) hardcodes MySQL connection arguments (charset, timezone) and the DATABASE_URL defaults to MySQL. Adding database dialect checks would be unnecessary overhead for a MySQL-only application.
| def downgrade() -> None: | ||
| """Revert to original column types.""" | ||
| op.alter_column( | ||
| 'subtask_attachments', | ||
| 'binary_data', | ||
| existing_type=mysql.LONGBLOB(), | ||
| type_=sa.LargeBinary(), | ||
| existing_nullable=False | ||
| ) | ||
|
|
||
| op.alter_column( | ||
| 'subtask_attachments', | ||
| 'extracted_text', | ||
| existing_type=mysql.LONGTEXT(), | ||
| type_=sa.Text(), | ||
| existing_nullable=True | ||
| ) No newline at end of file |
There was a problem hiding this comment.
Prevent data loss during downgrade.
The downgrade path converts LONGBLOB (4GB max) back to LargeBinary/BLOB (64KB max) and LONGTEXT back to Text. If any records contain data exceeding the smaller type's limits, data will be truncated and permanently lost.
Add validation to prevent data loss:
def downgrade() -> None:
"""Revert to original column types."""
bind = op.get_bind()
# Check if any binary_data exceeds BLOB size limit (64KB)
result = bind.execute(sa.text(
"SELECT COUNT(*) FROM subtask_attachments "
"WHERE LENGTH(binary_data) > 65535"
))
large_binary_count = result.scalar()
if large_binary_count > 0:
raise RuntimeError(
f"Cannot downgrade: {large_binary_count} records have binary_data "
f"exceeding BLOB size limit (64KB). Data would be lost."
)
# Check if any extracted_text exceeds TEXT size limit (64KB)
result = bind.execute(sa.text(
"SELECT COUNT(*) FROM subtask_attachments "
"WHERE LENGTH(extracted_text) > 65535"
))
large_text_count = result.scalar()
if large_text_count > 0:
raise RuntimeError(
f"Cannot downgrade: {large_text_count} records have extracted_text "
f"exceeding TEXT size limit (64KB). Data would be lost."
)
op.alter_column(
'subtask_attachments',
'binary_data',
existing_type=mysql.LONGBLOB(),
type_=sa.LargeBinary(),
existing_nullable=False
)
op.alter_column(
'subtask_attachments',
'extracted_text',
existing_type=mysql.LONGTEXT(),
type_=sa.Text(),
existing_nullable=True
)🤖 Prompt for AI Agents
In backend/alembic/versions/fix_binary_data_column_size.py around lines 49 to
65, the downgrade currently alters LONGBLOB/LONGTEXT back to smaller types which
can truncate data; add a pre-check using bind = op.get_bind() and execute COUNT
queries (e.g. SELECT COUNT(*) FROM subtask_attachments WHERE LENGTH(binary_data)
> 65535 and WHERE LENGTH(extracted_text) > 65535) and if either count > 0 raise
a RuntimeError with the offending count(s) to abort the downgrade and avoid data
loss before calling op.alter_column for binary_data and extracted_text.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
backend/alembic/versions/add_subtask_attachments_add_subtask_attachments_table.py (1)
62-63: PostgreSQL-specific DROP TYPE statement in MySQL migration.This issue was already flagged in a previous review. The
DROP TYPE IF EXISTSstatement is PostgreSQL-specific and incompatible with MySQL. MySQL enums are embedded in column definitions and don't require explicit type drops.As per the previous review, remove this line or wrap it in a dialect check:
- # Drop the enum type - op.execute("DROP TYPE IF EXISTS attachmentstatus") + # Only drop enum type for PostgreSQL (MySQL enums are column-embedded) + from alembic import context + if context.get_context().dialect.name == 'postgresql': + op.execute("DROP TYPE IF EXISTS attachmentstatus")
🧹 Nitpick comments (1)
backend/alembic/versions/add_subtask_attachments_add_subtask_attachments_table.py (1)
50-50: Consider removing redundant index on primary key.The index
ix_subtask_attachments_idis redundant because theidcolumn is already a primary key, which creates an implicit index. In MySQL, this may result in duplicate index overhead.If you want to remove the redundant index:
- op.create_index('ix_subtask_attachments_id', 'subtask_attachments', ['id']) op.create_index('ix_subtask_attachments_subtask_id', 'subtask_attachments', ['subtask_id']) op.create_index('ix_subtask_attachments_user_id', 'subtask_attachments', ['user_id'])Don't forget to remove it from the downgrade function as well:
op.drop_index('ix_subtask_attachments_user_id', table_name='subtask_attachments') op.drop_index('ix_subtask_attachments_subtask_id', table_name='subtask_attachments') - op.drop_index('ix_subtask_attachments_id', table_name='subtask_attachments')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/alembic/versions/add_subtask_attachments_add_subtask_attachments_table.py(1 hunks)backend/alembic/versions/fix_binary_data_size_fix_binary_data_column_size.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{py,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
All code comments, inline comments, block comments, docstrings, TODO/FIXME annotations, and type hints descriptions MUST be written in English
Files:
backend/alembic/versions/fix_binary_data_size_fix_binary_data_column_size.pybackend/alembic/versions/add_subtask_attachments_add_subtask_attachments_table.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code MUST be PEP 8 compliant with Black formatter (line length: 88) and isort for import organization
Python code MUST include type hints for all functions and variables
Python functions SHOULD NOT exceed 50 lines (preferred maximum)
Python functions and classes MUST have descriptive names and public functions/classes MUST include docstrings
Python code MUST extract magic numbers to named constants
Files:
backend/alembic/versions/fix_binary_data_size_fix_binary_data_column_size.pybackend/alembic/versions/add_subtask_attachments_add_subtask_attachments_table.py
**/backend/alembic/versions/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend Alembic database migrations MUST be reviewed before applying, especially auto-generated migrations
Files:
backend/alembic/versions/fix_binary_data_size_fix_binary_data_column_size.pybackend/alembic/versions/add_subtask_attachments_add_subtask_attachments_table.py
🧠 Learnings (1)
📚 Learning: 2025-12-01T06:44:24.499Z
Learnt from: CR
Repo: wecode-ai/Wegent PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T06:44:24.499Z
Learning: Applies to **/backend/alembic/versions/*.py : Backend Alembic database migrations MUST be reviewed before applying, especially auto-generated migrations
Applied to files:
backend/alembic/versions/fix_binary_data_size_fix_binary_data_column_size.pybackend/alembic/versions/add_subtask_attachments_add_subtask_attachments_table.py
🧬 Code graph analysis (2)
backend/alembic/versions/fix_binary_data_size_fix_binary_data_column_size.py (1)
backend/alembic/versions/add_subtask_attachments_add_subtask_attachments_table.py (2)
upgrade(25-52)downgrade(55-63)
backend/alembic/versions/add_subtask_attachments_add_subtask_attachments_table.py (1)
backend/alembic/versions/fix_binary_data_size_fix_binary_data_column_size.py (2)
upgrade(28-46)downgrade(49-65)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test Backend (3.11)
- GitHub Check: Test Backend (3.10)
- GitHub Check: e2e-tests
🔇 Additional comments (2)
backend/alembic/versions/fix_binary_data_size_fix_binary_data_column_size.py (1)
1-25: LGTM: Migration metadata properly configured.The revision identifiers are correctly set, and the migration chain properly follows
add_subtask_attachments. The docstring clearly explains the purpose of extending column sizes for large file support.backend/alembic/versions/add_subtask_attachments_add_subtask_attachments_table.py (1)
18-22: Thedown_revisionvalue is not a placeholder and correctly references the existing migrationa1b2c3d4e5f6_add_auth_source_to_users.py. The migration chain is properly linked:a1b2c3d4e5f6→add_subtask_attachments→fix_binary_data_size, with the subsequent migration correctly pointing back to this one.Likely an incorrect or invalid review comment.
| sa.Column('user_id', sa.Integer(), nullable=False), | ||
| sa.Column('original_filename', sa.String(255), nullable=False), | ||
| sa.Column('file_extension', sa.String(20), nullable=False), | ||
| sa.Column('file_size', sa.Integer(), nullable=False), |
There was a problem hiding this comment.
file_size as Integer may be insufficient for large files.
The file_size column is defined as sa.Integer(), which is typically a signed 32-bit integer (max ~2.1GB). Given that the follow-up migration (fix_binary_data_size) extends binary_data to LONGBLOB (4GB max), file sizes could exceed Integer range.
Apply this diff to use BigInteger:
- sa.Column('file_size', sa.Integer(), nullable=False),
+ sa.Column('file_size', sa.BigInteger(), nullable=False),Note: If you make this change, the fix_binary_data_size migration should also alter this column type in its upgrade function for consistency.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sa.Column('file_size', sa.Integer(), nullable=False), | |
| sa.Column('file_size', sa.BigInteger(), nullable=False), |
🤖 Prompt for AI Agents
In
backend/alembic/versions/add_subtask_attachments_add_subtask_attachments_table.py
around line 34, the file_size column is declared as sa.Integer which can
overflow for files >~2.1GB; change its type to sa.BigInteger() in this migration
to support larger sizes and update the follow-up migration
(fix_binary_data_size) to ALTER this column to BigInteger in its upgrade as well
so both migrations remain consistent.
| def upgrade() -> None: | ||
| """Change binary_data column to LONGBLOB for MySQL.""" | ||
| # Use MySQL-specific LONGBLOB type for large file support | ||
| op.alter_column( | ||
| 'subtask_attachments', | ||
| 'binary_data', | ||
| existing_type=sa.LargeBinary(), | ||
| type_=mysql.LONGBLOB(), | ||
| existing_nullable=False | ||
| ) | ||
|
|
||
| # Also change extracted_text to LONGTEXT for large documents | ||
| op.alter_column( | ||
| 'subtask_attachments', | ||
| 'extracted_text', | ||
| existing_type=sa.Text(), | ||
| type_=mysql.LONGTEXT(), | ||
| existing_nullable=True | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for dialect checks and MySQL-specific patterns in migrations
cd /tmp && find . -name "backend" -type d 2>/dev/null | head -1 | xargs -I {} bash -c 'cd {} && pwd'Repository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Explore the migration structure first
echo "=== Finding Alembic versions directory ==="
find . -path "*/backend/alembic/versions" -type d 2>/dev/null
echo -e "\n=== Listing migrations ==="
ls -la backend/alembic/versions/ 2>/dev/null | head -20Repository: wecode-ai/Wegent
Length of output: 739
🏁 Script executed:
#!/bin/bash
# Check dialect checks pattern across migrations
echo "=== Checking for dialect/bind checks in migrations ==="
rg -n "dialect\.name|get_bind\(\)|if.*dialect" backend/alembic/versions/ || echo "No dialect checks found"
echo -e "\n=== Checking MySQL-specific type usage ==="
rg -n "mysql\.(LONGBLOB|LONGTEXT|MEDIUMBLOB|MEDIUMTEXT)" backend/alembic/versions/ || echo "No other MySQL-specific types found"
echo -e "\n=== Checking for the subtask_attachments table creation ==="
rg -n "subtask_attachments" backend/alembic/versions/ -B 2 -A 2Repository: wecode-ai/Wegent
Length of output: 8285
🏁 Script executed:
#!/bin/bash
# Check database configuration and strategy
echo "=== Checking database configuration files ==="
find backend -name "*.py" -type f | xargs rg -l "DATABASE_URL|SQLALCHEMY_DATABASE_URI|sqlalchemy\.create_engine" | head -10
echo -e "\n=== Checking alembic.ini or configuration ==="
if [ -f "backend/alembic.ini" ]; then
grep -i "sqlalchemy" backend/alembic.ini || echo "No sqlalchemy config found in alembic.ini"
fi
echo -e "\n=== Checking env.py for database configuration ==="
if [ -f "backend/alembic/env.py" ]; then
head -50 backend/alembic/env.py
fiRepository: wecode-ai/Wegent
Length of output: 2045
🏁 Script executed:
#!/bin/bash
# Search the migration file itself to understand the full context
echo "=== Reading the migration file ==="
cat -n backend/alembic/versions/fix_binary_data_size_fix_binary_data_column_size.py 2>/dev/null | head -60
echo -e "\n=== Looking for table creation migration ==="
rg -n "CreateTable.*subtask_attachments|subtask_attachments.*CreateTable" backend/alembic/versions/ -A 5 -B 1 | head -40Repository: wecode-ai/Wegent
Length of output: 2213
🏁 Script executed:
#!/bin/bash
# Read the table creation migration to verify original column types
cat -n backend/alembic/versions/add_subtask_attachments_add_subtask_attachments_table.py | head -50Repository: wecode-ai/Wegent
Length of output: 2323
🏁 Script executed:
#!/bin/bash
# Check if there's any database requirement documentation
echo "=== Checking for database strategy docs ==="
find . -name "README*" -o -name "ARCHITECTURE*" -o -name "DATABASE*" | xargs grep -l "database\|MySQL\|PostgreSQL" 2>/dev/null | head -5
echo -e "\n=== Checking backend config for database requirements ==="
head -50 backend/app/core/config.py | grep -A 10 -B 2 "DATABASE\|database"Repository: wecode-ai/Wegent
Length of output: 712
Add dialect checks or document MySQL-only database requirement.
The migration switches from database-agnostic types (sa.LargeBinary, sa.Text) to MySQL-specific types (mysql.LONGBLOB, mysql.LONGTEXT). While the table creation uses MySQL options, the original columns were intentionally defined with generic SQLAlchemy types. This migration creates vendor lock-in without explicit documentation.
Either add dialect checks to handle multiple databases:
def upgrade() -> None:
"""Change binary_data column to LONGBLOB for MySQL."""
bind = op.get_bind()
if bind.dialect.name == 'mysql':
op.alter_column(
'subtask_attachments',
'binary_data',
existing_type=sa.LargeBinary(),
type_=mysql.LONGBLOB(),
existing_nullable=False
)
op.alter_column(
'subtask_attachments',
'extracted_text',
existing_type=sa.Text(),
type_=mysql.LONGTEXT(),
existing_nullable=True
)Or add a module-level docstring clarifying that MySQL is the only supported database:
"""Fix binary_data column size to LONGBLOB for large file support
This migration uses MySQL-specific types and is only compatible with MySQL databases.
"""🤖 Prompt for AI Agents
In backend/alembic/versions/fix_binary_data_size_fix_binary_data_column_size.py
around lines 28 to 46, the migration replaces generic SQLAlchemy types with
MySQL-specific types which creates unintended vendor lock-in; either add a
runtime dialect check using op.get_bind() and only apply mysql.LONGBLOB and
mysql.LONGTEXT when bind.dialect.name == 'mysql' (skip or no-op for other DBs),
or add a module-level docstring stating this migration is MySQL-only and will
fail on other backends so reviewers/operators are aware.
| def downgrade() -> None: | ||
| """Revert to original column types.""" | ||
| op.alter_column( | ||
| 'subtask_attachments', | ||
| 'binary_data', | ||
| existing_type=mysql.LONGBLOB(), | ||
| type_=sa.LargeBinary(), | ||
| existing_nullable=False | ||
| ) | ||
|
|
||
| op.alter_column( | ||
| 'subtask_attachments', | ||
| 'extracted_text', | ||
| existing_type=mysql.LONGTEXT(), | ||
| type_=sa.Text(), | ||
| existing_nullable=True | ||
| ) No newline at end of file |
There was a problem hiding this comment.
Document potential data loss risk during downgrade.
The downgrade reverts LONGBLOB → LargeBinary (BLOB) and LONGTEXT → Text, which could truncate data if files larger than BLOB/Text limits exist. While downgrades are typically performed in controlled environments, the risk should be documented.
Consider adding a warning comment:
def downgrade() -> None:
- """Revert to original column types."""
+ """Revert to original column types.
+
+ WARNING: This downgrade will fail or truncate data if any attachments
+ exceed BLOB (64KB) or TEXT (64KB) limits. Ensure no large files exist
+ before downgrading, or manually delete large attachments first.
+ """📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def downgrade() -> None: | |
| """Revert to original column types.""" | |
| op.alter_column( | |
| 'subtask_attachments', | |
| 'binary_data', | |
| existing_type=mysql.LONGBLOB(), | |
| type_=sa.LargeBinary(), | |
| existing_nullable=False | |
| ) | |
| op.alter_column( | |
| 'subtask_attachments', | |
| 'extracted_text', | |
| existing_type=mysql.LONGTEXT(), | |
| type_=sa.Text(), | |
| existing_nullable=True | |
| ) | |
| def downgrade() -> None: | |
| """Revert to original column types. | |
| WARNING: This downgrade will fail or truncate data if any attachments | |
| exceed BLOB (64KB) or TEXT (64KB) limits. Ensure no large files exist | |
| before downgrading, or manually delete large attachments first. | |
| """ | |
| op.alter_column( | |
| 'subtask_attachments', | |
| 'binary_data', | |
| existing_type=mysql.LONGBLOB(), | |
| type_=sa.LargeBinary(), | |
| existing_nullable=False | |
| ) | |
| op.alter_column( | |
| 'subtask_attachments', | |
| 'extracted_text', | |
| existing_type=mysql.LONGTEXT(), | |
| type_=sa.Text(), | |
| existing_nullable=True | |
| ) |
🤖 Prompt for AI Agents
In backend/alembic/versions/fix_binary_data_size_fix_binary_data_column_size.py
around lines 49 to 65, the downgrade changes LONGBLOB→LargeBinary and
LONGTEXT→Text which can truncate data for values exceeding BLOB/Text limits; add
a clear warning comment immediately above the downgrade function documenting
this risk, instructing operators to backup the database, verify no column values
exceed target sizes (or refuse to run downgrade until verified), and recommend
an alternative migration strategy or manual cleanup to prevent data loss during
downgrade.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
frontend/src/apis/chat.ts (3)
117-129: Missing 401 authentication handling.This error handling block doesn't redirect to login on 401 responses, unlike the existing
client.tspattern. Users with expired tokens will see a generic error instead of being redirected to login.
137-138: Race condition:taskIdreturned before stream updates it.The function returns
taskIdimmediately after initiating the stream, buttaskIdis updated inside the detached IIFE when the first message arrives. Callers receive the initial value rather than the actual task ID from the server.Also applies to: 207-210
237-248: Same 401 handling gap applies here.This function also lacks 401 authentication handling consistent with the existing
client.tspattern.
🧹 Nitpick comments (14)
frontend/src/hooks/useChatStream.ts (1)
144-172: Consider using a ref to track streaming content for cleaner state access.The current approach of using
setStreamingContentcallback to read the current state (Lines 151-154) works but is unconventional. While it successfully captures the partial content, using a ref would be more idiomatic and clearer in intent.Consider this alternative approach:
const [streamingContent, setStreamingContent] = useState(''); + const streamingContentRef = useRef(''); // ... in startStream, when updating content: if (data.content) { setStreamingContent((prev) => { const newContent = prev + data.content; + streamingContentRef.current = newContent; - return prev + data.content; + return newContent; }); optionsRef.current.onChunk?.(data.content); } // ... in stopStream: const stopStream = useCallback(async () => { abortRef.current?.(); - let partialContent = ''; - setStreamingContent((prev) => { - partialContent = prev; - return prev; - }); + const partialContent = streamingContentRef.current; setIsStreaming(false);frontend/src/features/tasks/components/MessagesArea.tsx (1)
346-346: Consider the performance impact of displayContent in layout effect dependencies.Line 346 includes
displayContentin theuseLayoutEffectdependencies. SincedisplayContentupdates character-by-character during streaming (from the typewriter hook), this could trigger the layout effect very frequently, potentially causing performance issues with excessive scroll calculations.Consider whether scroll adjustments need to happen on every character update, or only when major layout changes occur:
- }, [displayMessages, displayContent, pendingUserMessage, isStreaming, AUTO_SCROLL_THRESHOLD]); + }, [displayMessages, pendingUserMessage, isStreaming, AUTO_SCROLL_THRESHOLD]);Alternatively, if scroll tracking during streaming is important, consider throttling or debouncing the scroll updates, or using a separate effect that only responds to the completion of streaming rather than each incremental update.
frontend/src/features/tasks/components/ChatArea.tsx (4)
373-378: Chinese text in toast should be consistent with the rest of the app.The toast message uses Chinese ("请等待文件上传完成") while other error messages in this file use English. Verify consistency with the application's localization strategy.
736-752: Significant UI duplication between "no messages" and "has messages" views.The drag overlay and file upload preview blocks are duplicated almost identically in both sections (lines 736-767 and 935-966). Consider extracting these into a reusable component or using a render function to reduce duplication.
Example extraction:
const renderDragOverlay = () => isDragging && ( <div className="absolute inset-0 z-50 rounded-2xl bg-base/95 ..."> {/* ... overlay content */} </div> ); const renderFileUploadPreview = () => ( attachmentState.attachment || attachmentState.isUploading || attachmentState.error ) && ( <div className="px-3 pt-2"> <FileUpload {...attachmentProps} /> </div> );Also applies to: 935-951
749-750: Chinese text in drag overlay should follow localization strategy.The drag overlay uses Chinese text ("释放以上传文件", "支持 PDF, Word, TXT, Markdown 等格式"). If the app supports i18n, these should use translation keys.
Also applies to: 948-949
762-762: Inconsistentdisabledprop between the two FileUpload instances.In the "no messages" view (line 762),
disabled={hasMessages || isLoading || isStreaming}, but in the "has messages" view (line 961),disabled={isLoading || isStreaming}. SincehasMessagesis always true in the second case, this is functionally correct, but the inconsistency suggests the first instance may have an unnecessary check.Also applies to: 961-961
frontend/src/apis/chat.ts (2)
176-178: Silent parse error swallowing may hide malformed server responses.Parse errors for incomplete chunks are silently ignored. While this is often correct for SSE (incomplete lines), consider logging at debug level to aid troubleshooting malformed responses.
} catch { - // Ignore parse errors for incomplete chunks + // Ignore parse errors for incomplete chunks + // console.debug('[chat.ts] Skipping non-JSON data line:', data); }
290-302: cancelChat also missing 401 handling - consider centralizing error handling.All three functions (
streamChat,checkDirectChat,cancelChat) have duplicated error handling code that lacks 401 handling. Consider extracting a shared error handler.async function handleApiResponse(response: Response): Promise<void> { if (!response.ok) { if (response.status === 401) { // Handle auth failure - redirect to login removeToken(); if (typeof window !== 'undefined') { window.location.href = '/login'; } throw new Error('Authentication failed'); } const errorText = await response.text(); let errorMsg = errorText; try { const json = JSON.parse(errorText); if (json && typeof json.detail === 'string') { errorMsg = json.detail; } } catch { // Not JSON, use original text } throw new Error(errorMsg); } }backend/app/api/endpoints/adapter/chat.py (6)
55-101: Consider simplifying with early returns and fixing f-string issues.The function has several f-strings without placeholders (lines 76, 91, 99) flagged by Ruff. Also, using
== Truefor boolean comparisons (line 69, 83) is not idiomatic.- logger.info(f"[chat.py] _get_shell_type: user shell not found, checking public shells") + logger.info("[chat.py] _get_shell_type: user shell not found, checking public shells") - Kind.is_active == True, + Kind.is_active.is_(True),
103-103: Unused parameteruser_idin_should_use_direct_chat.The
user_idparameter is declared but never used in the function body. The function usesteam.user_idinstead. Either remove the parameter or use it.-def _should_use_direct_chat(db: Session, team: Kind, user_id: int) -> bool: +def _should_use_direct_chat(db: Session, team: Kind) -> bool:Then update callers at lines 393 and 595:
- if not _should_use_direct_chat(db, team, current_user.id): + if not _should_use_direct_chat(db, team):
144-351: Function_create_task_and_subtasksexceeds 50-line guideline significantly (208 lines).Per coding guidelines, Python functions should not exceed 50 lines. This function handles multiple responsibilities:
- Bot ID collection
- Task retrieval/creation
- Workspace creation
- Subtask creation
Consider splitting into smaller functions for better maintainability.
Suggested decomposition:
def _get_bot_ids_from_team(db: Session, team: Kind, team_crd: Team) -> list[int]: """Extract bot IDs from team members.""" ... def _get_or_create_task(db: Session, user: User, team: Kind, ...) -> Kind: """Get existing task or create new one with workspace.""" ... def _create_chat_subtasks(db: Session, user: User, task_id: int, ...) -> Subtask: """Create USER and ASSISTANT subtasks for chat.""" ...
481-482: Re-raise exception with context usingfrom.When raising a new exception from a caught exception, use
raise ... from eto preserve the exception chain for better debugging.except ValueError as e: - raise HTTPException(status_code=400, detail=str(e)) + raise HTTPException(status_code=400, detail=str(e)) from e
491-491: Unused variablebot_crd.
bot_crdis assigned but never used. Remove this dead code.- bot_crd = Bot.model_validate(bot.json) bot_json = bot.json or {}
533-534: Late imports inside function.Importing
jsonandStreamingResponseinside the function is unusual. These should be at the top of the file with other imports for consistency and slight performance improvement.Move to top-level imports:
+import json from fastapi import APIRouter, Depends, HTTPException +from fastapi.responses import StreamingResponse
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/app/api/endpoints/adapter/chat.py(1 hunks)backend/app/models/subtask.py(1 hunks)backend/app/schemas/subtask.py(3 hunks)backend/app/services/adapters/task_kinds.py(2 hunks)backend/app/services/subtask.py(2 hunks)frontend/src/apis/chat.ts(1 hunks)frontend/src/app/api/chat/cancel/route.ts(1 hunks)frontend/src/features/tasks/components/ChatArea.tsx(17 hunks)frontend/src/features/tasks/components/MessagesArea.tsx(11 hunks)frontend/src/hooks/useChatStream.ts(1 hunks)frontend/src/types/api.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{py,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
All code comments, inline comments, block comments, docstrings, TODO/FIXME annotations, and type hints descriptions MUST be written in English
Files:
frontend/src/app/api/chat/cancel/route.tsfrontend/src/hooks/useChatStream.tsfrontend/src/types/api.tsbackend/app/services/subtask.pybackend/app/services/adapters/task_kinds.pybackend/app/schemas/subtask.pyfrontend/src/features/tasks/components/ChatArea.tsxbackend/app/models/subtask.pyfrontend/src/features/tasks/components/MessagesArea.tsxfrontend/src/apis/chat.tsbackend/app/api/endpoints/adapter/chat.py
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: TypeScript code MUST use strict mode with type checking enabled
TypeScript/React code MUST use Prettier formatter with single quotes, no semicolons
TypeScript/React code MUST pass ESLint with Next.js configuration
React component names MUST use PascalCase convention
Files:
frontend/src/app/api/chat/cancel/route.tsfrontend/src/hooks/useChatStream.tsfrontend/src/types/api.tsfrontend/src/features/tasks/components/ChatArea.tsxfrontend/src/features/tasks/components/MessagesArea.tsxfrontend/src/apis/chat.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: React components MUST use functional components with hooks instead of class-based components
Useconstoverlet, never usevarin TypeScript/JavaScript code
Files:
frontend/src/app/api/chat/cancel/route.tsfrontend/src/hooks/useChatStream.tsfrontend/src/types/api.tsfrontend/src/features/tasks/components/ChatArea.tsxfrontend/src/features/tasks/components/MessagesArea.tsxfrontend/src/apis/chat.ts
**/types/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript type definitions MUST be organized in
src/types/directory
Files:
frontend/src/types/api.ts
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code MUST be PEP 8 compliant with Black formatter (line length: 88) and isort for import organization
Python code MUST include type hints for all functions and variables
Python functions SHOULD NOT exceed 50 lines (preferred maximum)
Python functions and classes MUST have descriptive names and public functions/classes MUST include docstrings
Python code MUST extract magic numbers to named constants
Files:
backend/app/services/subtask.pybackend/app/services/adapters/task_kinds.pybackend/app/schemas/subtask.pybackend/app/models/subtask.pybackend/app/api/endpoints/adapter/chat.py
**/backend/app/services/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend business logic MUST be placed in
app/services/directory
Files:
backend/app/services/subtask.pybackend/app/services/adapters/task_kinds.py
**/backend/app/schemas/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend Pydantic schemas MUST be created in
app/schemas/directory
Files:
backend/app/schemas/subtask.py
**/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
React component files MUST use kebab-case naming convention
Files:
frontend/src/features/tasks/components/ChatArea.tsxfrontend/src/features/tasks/components/MessagesArea.tsx
**/src/**/*.{tsx,jsx,css}
📄 CodeRabbit inference engine (AGENTS.md)
Frontend Tailwind CSS MUST use provided CSS variables for color system (e.g., --color-bg-base, --color-text-primary, --color-primary)
Files:
frontend/src/features/tasks/components/ChatArea.tsxfrontend/src/features/tasks/components/MessagesArea.tsx
**/src/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/src/**/*.{tsx,jsx}: Frontend responsive design MUST follow mobile-first approach with Tailwind breakpoints
Frontend React forms MUST use react-hook-form with zod validation schema
Frontend components MUST use shadcn/ui component library fromfrontend/src/components/ui/
Files:
frontend/src/features/tasks/components/ChatArea.tsxfrontend/src/features/tasks/components/MessagesArea.tsx
**/backend/app/models/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend SQLAlchemy models MUST be created in
app/models/directory
Files:
backend/app/models/subtask.py
**/backend/app/api/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend API route handlers MUST be created in
app/api/directory
Files:
backend/app/api/endpoints/adapter/chat.py
🧬 Code graph analysis (6)
frontend/src/hooks/useChatStream.ts (1)
frontend/src/apis/chat.ts (4)
StreamChatRequest(40-59)streamChat(90-218)ChatStreamData(20-35)cancelChat(278-305)
frontend/src/types/api.ts (2)
backend/app/models/subtask_attachment.py (1)
AttachmentStatus(21-26)frontend/src/apis/attachments.ts (1)
AttachmentStatus(17-17)
backend/app/services/subtask.py (1)
backend/app/models/subtask.py (1)
Subtask(31-72)
backend/app/schemas/subtask.py (8)
backend/app/core/config.py (1)
Config(103-106)backend/app/schemas/task.py (3)
Config(105-106)Config(132-133)Config(156-157)backend/app/schemas/user.py (1)
Config(69-70)backend/app/schemas/bot.py (2)
Config(55-56)Config(75-76)backend/app/schemas/agent.py (2)
Config(38-39)Config(51-52)backend/app/schemas/model.py (2)
Config(40-41)Config(54-55)backend/app/schemas/shared_team.py (1)
Config(30-31)backend/app/schemas/team.py (2)
Config(73-74)Config(90-91)
frontend/src/features/tasks/components/ChatArea.tsx (6)
frontend/src/hooks/useAttachment.ts (1)
useAttachment(33-179)frontend/src/features/tasks/contexts/taskContext.tsx (1)
useTaskContext(284-290)frontend/src/hooks/useChatStream.ts (1)
useChatStream(75-194)frontend/src/features/tasks/service/messageService.ts (1)
isChatShell(15-26)frontend/src/features/tasks/components/ModelSelector.tsx (1)
DEFAULT_MODEL_NAME(40-40)frontend/src/features/tasks/components/FileUpload.tsx (1)
FileUpload(44-209)
frontend/src/apis/chat.ts (2)
backend/app/api/endpoints/adapter/chat.py (2)
StreamChatRequest(38-52)CancelChatRequest(622-626)frontend/src/apis/user.ts (1)
getToken(54-59)
🪛 Ruff (0.14.7)
backend/app/api/endpoints/adapter/chat.py
69-69: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
76-76: f-string without any placeholders
Remove extraneous f prefix
(F541)
83-83: Avoid equality comparisons to True; use PublicShell.is_active: for truth checks
Replace with PublicShell.is_active
(E712)
91-91: f-string without any placeholders
Remove extraneous f prefix
(F541)
99-99: f-string without any placeholders
Remove extraneous f prefix
(F541)
103-103: Unused function argument: user_id
(ARG001)
123-123: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
129-129: f-string without any placeholders
Remove extraneous f prefix
(F541)
140-140: f-string without any placeholders
Remove extraneous f prefix
(F541)
170-170: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
190-190: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
308-308: f-string without any placeholders
Remove extraneous f prefix
(F541)
331-331: f-string without any placeholders
Remove extraneous f prefix
(F541)
357-357: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
358-358: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
384-384: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
464-464: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
482-482: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
491-491: Local variable bot_crd is assigned to but never used
Remove assignment to unused variable bot_crd
(F841)
573-573: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
574-574: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
587-587: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
609-609: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
632-632: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
633-633: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
683-683: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: e2e-tests
- GitHub Check: Test Backend (3.10)
- GitHub Check: Test Backend (3.11)
🔇 Additional comments (26)
backend/app/services/adapters/task_kinds.py (2)
614-627: LGTM!The attachment collection logic is well-implemented with defensive programming. The use of
hasattrto check for theattachmentsrelationship and the graceful handling of both enum and raw status values (Line 623) demonstrate good defensive coding practices.
654-655: LGTM!The integration of attachments into the subtask dictionary is clean and consistent with the broader data structure.
backend/app/services/subtask.py (1)
9-9: LGTM!The addition of eager loading for the
attachmentsrelationship usingjoinedloadis an excellent optimization that prevents N+1 query problems when accessing subtask attachments. The import is correct and the usage follows SQLAlchemy best practices.Also applies to: 75-75
backend/app/schemas/subtask.py (2)
64-78: LGTM!The
SubtaskAttachmentschema is well-structured with proper field definitions and ORM configuration. The use ofvalidation_aliasfor thefilenamefield (Line 68) combined withpopulate_by_name = Truein the Config (Line 77) correctly enables both the alias (original_filename) and the field name (filename) to be accepted during validation, which provides flexibility for API consumers.
104-104: LGTM!The integration of the
attachmentsfield intoSubtaskInDBis clean and provides a default empty list, which prevents null-related issues when attachments are not present.backend/app/models/subtask.py (1)
57-63: LGTM!The
attachmentsrelationship is properly configured with appropriate cascade behavior. The use ofcascade="all, delete-orphan"ensures that attachments are automatically deleted when their parent subtask is removed, andpassive_deletes=Trueoptimizes deletion when database-level cascades are in place. Thebackrefprovides convenient bidirectional navigation.frontend/src/types/api.ts (3)
397-411: LGTM!The attachment type definitions are comprehensive and well-structured. The
AttachmentStatustype correctly mirrors the backend enum values, and theAttachmentinterface includes all necessary fields with appropriate optional markers for nullable fields.
413-419: LGTM!The
AttachmentUploadStateinterface provides a clean structure for managing upload UI state, including file reference, attachment data, progress tracking, and error handling.
263-263: LGTM!The integration of the optional
attachmentsfield intoTaskDetailSubtaskis clean and consistent with the broader type system.frontend/src/hooks/useChatStream.ts (2)
75-138: LGTM!The
startStreamimplementation is well-structured with proper state initialization, callback handling, and ID tracking. The use of refs (optionsRef,subtaskIdRef) to maintain stable references across async operations is a good pattern that prevents stale closure issues.
177-182: LGTM!The
resetStreamfunction properly clears all streaming-related state, providing a clean way to initialize for a new conversation.frontend/src/features/tasks/components/MessagesArea.tsx (5)
162-167: LGTM!The integration of the typewriter effect for streaming content (Line 162) and the Chat Shell detection (Lines 165-167) are well-implemented. The case-insensitive comparison for
agent_typeis a good defensive practice.
172-174: LGTM!The conditional polling logic correctly skips auto-refresh for Chat Shell mode, which uses streaming instead. The inclusion of
isChatShellin the dependency array (Line 191) ensures the effect responds to team changes.Also applies to: 191-191
569-584: LGTM!The
renderAttachmentshelper function is clean and reusable, properly handling empty attachment arrays and rendering attachment previews with download capability.
832-844: LGTM!The optimistic UI rendering for pending user messages provides immediate feedback while the request is in flight, improving the perceived responsiveness of the interface.
846-881: LGTM!The streaming AI response rendering is well-implemented with proper loading states, markdown rendering with link handling, and a visual cursor indicator. The conditional rendering based on
displayContentprovides a smooth progressive display experience.frontend/src/features/tasks/components/ChatArea.tsx (4)
80-90: LGTM - Attachment state management setup.The attachment hook integration looks correct, destructuring all necessary state and handlers for file management.
117-148: LGTM - Chat streaming hook integration.The
useChatStreamhook is properly configured withonCompleteandonErrorcallbacks. The logic correctly:
- Clears pending message and resets attachment after completion
- Refreshes tasks and updates URL for new tasks
- Shows toast on error
326-365: Drag-drop handlers look correct with proper guards.The implementation properly:
- Prevents default and stops propagation
- Guards against non-Chat Shell teams
- Checks loading/streaming/existing attachment states
- Handles the child element edge case in
handleDragLeave
392-449: Chat Shell streaming flow is well-structured.The streaming implementation correctly:
- Sets optimistic UI state before starting the stream
- Derives
modelIdconditionally based on default model selection- Creates a minimal task object for the selected task state
- Handles errors by clearing the pending message
frontend/src/apis/chat.ts (2)
20-35: LGTM - Well-documented stream data interface.The
ChatStreamDatainterface properly documents all possible fields in the stream events with clear JSDoc comments.
140-205: Detached async IIFE for stream processing is a valid pattern but has caveats.The detached IIFE
(async () => { ... })()processes the stream asynchronously. This is intentional to allow the function to return immediately with the abort controller. However:
- If the stream fails immediately,
onErroris called but the returnedtaskIdis stale- The caller must rely on
onCompletecallback for the actual IDsThis pattern is acceptable given the streaming nature, but ensure callers (like
useChatStream) properly handle the callback-based ID updates.backend/app/api/endpoints/adapter/chat.py (4)
38-53: LGTM - Request model aligns with frontend contract.The
StreamChatRequestmodel matches the frontendStreamChatRequestinterface and backend requirements. Good use of Optional types with defaults.
622-626: LGTM - CancelChatRequest model is properly defined.The model correctly defines the required
subtask_idand optionalpartial_contentfields.
666-676: LGTM - Cancel logic properly handles state transitions.The cancellation correctly:
- Validates the subtask belongs to the user
- Checks for cancellable states (PENDING, RUNNING)
- Updates status, timestamps, and error message
- Optionally saves partial content
688-701: Verify if task should always be cancelled when one subtask is cancelled, or only after checking other subtasks.The code immediately cancels the entire task when a single subtask is cancelled (line 693:
task_crd.status.status = "CANCELLED"), regardless of whether other subtasks in the same task are stillRUNNINGorPENDING. Additionally,_update_task_status_sync()in chat_service.py determines task status only from the last subtask and doesn't handle theCANCELLEDstatus state, creating inconsistent task lifecycle logic. Clarify whether this immediate cancellation is intentional (user cancels one subtask = cancel entire task) or if the endpoint should check all subtasks before cancelling the task.The
if task_crd.status:check on line 692 properly handles the None case, so no issue there.
| parent_id=next_message_id, | ||
| error_message="", | ||
| result=None, | ||
| completed_at=datetime.now(), # Placeholder, will be updated when stream completes |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "completed_at" --type py -C 2Repository: wecode-ai/Wegent
Length of output: 9645
🏁 Script executed:
rg -n "completed_at" --type py -C 3 | grep -E "(where|filter|query|\.select|\.where|if.*completed_at|completed_at.*is|completed_at.*!=|completed_at.*>|completed_at.*<)"Repository: wecode-ai/Wegent
Length of output: 201
🏁 Script executed:
rg -n "completed_at" backend/alembic/ -C 2Repository: wecode-ai/Wegent
Length of output: 596
Database schema inconsistency: completed_at column should be nullable.
The completed_at column in the subtasks table is defined as NOT NULL in the database but treated as Optional[datetime] in the schema. Setting completed_at=datetime.now() for PENDING subtasks is a workaround for this mismatch. While this doesn't directly cause logic issues (status field is used for completion checks, not completed_at), it creates semantic confusion and should be resolved by making the column nullable in the database migration to align with the schema definition.
🤖 Prompt for AI Agents
In backend/app/api/endpoints/adapter/chat.py around line 343, the code sets
completed_at=datetime.now() for PENDING subtasks to work around a schema
mismatch; fix this by making the database column nullable and align code
accordingly: update the Alembic/migration that creates/alter the subtasks table
to set completed_at NULLABLE (or add an ALTER TABLE migration to alter the
column to nullable), update the ORM model/schema to mark completed_at
Optional/nullable if not already, and change code that creates pending subtasks
to pass None (or omit completed_at) for incomplete subtasks; run and verify the
migration and adjust any tests that assumed NOT NULL.
| if (!backendResponse.ok) { | ||
| const errorText = await backendResponse.text(); | ||
| return new NextResponse(errorText, { | ||
| status: backendResponse.status, | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Verify Content-Type matches error response format.
Line 36 returns errorText (which comes from backendResponse.text()) with Content-Type: application/json, but there's no guarantee that errorText is valid JSON. If the backend returns plain text or HTML error pages, this could mislead clients.
Consider parsing and validating the error text:
if (!backendResponse.ok) {
const errorText = await backendResponse.text();
+ let contentType = 'text/plain';
+ try {
+ JSON.parse(errorText);
+ contentType = 'application/json';
+ } catch {
+ // Not JSON, use text/plain
+ }
return new NextResponse(errorText, {
status: backendResponse.status,
headers: {
- 'Content-Type': 'application/json',
+ 'Content-Type': contentType,
},
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!backendResponse.ok) { | |
| const errorText = await backendResponse.text(); | |
| return new NextResponse(errorText, { | |
| status: backendResponse.status, | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| }); | |
| } | |
| if (!backendResponse.ok) { | |
| const errorText = await backendResponse.text(); | |
| const contentType = (() => { | |
| try { | |
| JSON.parse(errorText); | |
| return 'application/json'; | |
| } catch { | |
| return 'text/plain'; | |
| } | |
| })(); | |
| return new NextResponse(errorText, { | |
| status: backendResponse.status, | |
| headers: { | |
| 'Content-Type': contentType, | |
| }, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In frontend/src/app/api/chat/cancel/route.ts around lines 34 to 42, the handler
returns backendResponse.text() but always sets Content-Type: application/json
which can be incorrect if the backend sent plain text or HTML; change the code
to read the response body, attempt to parse it as JSON inside a try/catch (or
check Content-Type header from backendResponse), and if valid JSON return it
with Content-Type: application/json, otherwise return the raw text with
Content-Type: text/plain (or the original backend Content-Type) while preserving
the backendResponse.status and headers as appropriate.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/requirements.txt (1)
17-18: Resolve conflictingcryptographyconstraints and keep a single entry.You have both
cryptography==41.0.5(Line 17) andcryptography>=41.0.5(Line 86). This can cause resolver conflicts or non-deterministic versions depending on installer behavior. Keep one line with the intended constraint and remove the other.Also applies to: 85-95
♻️ Duplicate comments (9)
frontend/src/apis/chat.ts (3)
136-210: Clarify/adjust returnedtaskIdsemantics to avoid race with stream updates.
taskIdis initialized fromrequest.task_id || 0and returned immediately, but later updated inside the streaming loop when the firstdata:message arrives. Callers currently get a best-effort ID, not necessarily the final server-assigned one. Either:
- Change the API to resolve only after the first message (so returned
taskIdis authoritative), or- Clearly document that callers must use
onComplete’s IDs instead of the returnedtaskId.
104-129: Add consistent 401 handling likeclient.tsbefore generic error parsing.This branch still treats all non‑OK statuses the same. For 401 you should clear auth and redirect (or call the shared unauthorized handler) just like
frontend/src/apis/client.tsdoes, then throw an auth-specific error before attempting to parse the body.
226-305: Apply same 401 handling tocheckDirectChatandcancelChat.Both error branches currently just parse the error body and throw. They should treat HTTP 401 like the central client helper: clear tokens and redirect (or call the shared unauthorized handler) before throwing, to avoid “stuck” sessions when tokens expire.
backend/app/services/chat/chat_service.py (2)
341-358: Remove unusedmasked_keyand fix mislabelled error log.
masked_keyis computed but never used, and the error log still says “Claude API error response” inside the OpenAI streaming helper.You can simplify and correct this block:
- # Log request details (mask API key for security) - masked_key = f"{api_key[:8]}...{api_key[-4:]}" if api_key and len(api_key) > 12 else ("EMPTY" if not api_key else "***") logger.info(f"Calling OpenAI API: {url}, model: {model_id}") @@ - if response.status_code >= 400: - # Read error response body for debugging - error_body = await response.aread() - logger.error(f"[DEBUG] Claude API error response: status={response.status_code}, body={error_body.decode('utf-8', errors='replace')}") + if response.status_code >= 400: + # Read error response body for debugging + error_body = await response.aread() + logger.error( + "[DEBUG] OpenAI API error response: status=%s, body=%s", + response.status_code, + error_body.decode("utf-8", errors="replace"), + )
559-592: Migrate deprecatedQuery.get()toSession.get()in_update_subtask_status.
db.query(Subtask).get(subtask_id)uses the legacyQuery.get()API, which is deprecated in SQLAlchemy 2.x. PreferSession.get()instead:- subtask = db.query(Subtask).get(subtask_id) + subtask = db.get(Subtask, subtask_id)This keeps you aligned with SQLAlchemy 2.0 semantics and avoids deprecation noise.
frontend/src/features/tasks/components/FileUpload.tsx (1)
56-78: Make drag-and-drop follow the same validation path as click-to-upload.
handleDroppasses the dropped file straight toonFileSelect, whilehandleFileChangevalidates extension and size first. This leads to inconsistent behavior depending on how the user selects a file.Either centralize validation and reuse it from both handlers, or remove local validation entirely and document that the parent validates. For example:
- const handleFileChange = useCallback( - (e: React.ChangeEvent<HTMLInputElement>) => { - const file = e.target.files?.[0]; - if (file) { - // Validate file before passing to parent - if (!isSupportedExtension(file.name)) { - // Let parent handle the error - onFileSelect(file); - return; - } - if (!isValidFileSize(file.size)) { - onFileSelect(file); - return; - } - onFileSelect(file); - } + const validateAndSelect = (file: File) => { + if (!isSupportedExtension(file.name) || !isValidFileSize(file.size)) { + onFileSelect(file); // let parent handle error display + return; + } + onFileSelect(file); + }; + + const handleFileChange = useCallback( + (e: React.ChangeEvent<HTMLInputElement>) => { + const file = e.target.files?.[0]; + if (file) { + validateAndSelect(file); + } @@ - const file = e.dataTransfer.files?.[0]; - if (file) { - onFileSelect(file); - } + const file = e.dataTransfer.files?.[0]; + if (file) { + validateAndSelect(file); + } }, - [disabled, isUploading, attachment, onFileSelect] + [disabled, isUploading, attachment, onFileSelect] );Also applies to: 85-98
backend/app/models/subtask_attachment.py (1)
49-50: Add explicitForeignKeyconstraint foruser_id.The comment says
user_idis a foreign key, but the column is justIntegerwith noForeignKey. To enforce referential integrity and match thesubtask_idpattern:- # Foreign key to users table - user_id = Column(Integer, nullable=False, index=True) + # Foreign key to users table + user_id = Column( + Integer, + ForeignKey("users.id", ondelete="CASCADE"), + nullable=False, + index=True, + )backend/app/services/attachment/parser.py (1)
20-21: Add type hints for all module and class-level variables.As per coding guidelines, Python code must include type hints for all variables. The logger and class-level constants lack proper typing.
This issue was previously flagged. Apply these type hints:
-import logging -from typing import Tuple, Optional +import logging +from typing import ClassVar, Tuple, Optional -logger = logging.getLogger(__name__) +logger: logging.Logger = logging.getLogger(__name__) class DocumentParser: - SUPPORTED_EXTENSIONS = { + SUPPORTED_EXTENSIONS: ClassVar[dict[str, str]] = { ".pdf": "application/pdf", ... } - MAX_FILE_SIZE = 20 * 1024 * 1024 + MAX_FILE_SIZE: ClassVar[int] = 20 * 1024 * 1024 - MAX_TEXT_LENGTH = 50000 + MAX_TEXT_LENGTH: ClassVar[int] = 50000 -document_parser = DocumentParser() +document_parser: DocumentParser = DocumentParser()Based on learnings, Python code MUST include type hints for all functions and variables.
Also applies to: 43-60, 383-383
frontend/src/apis/attachments.ts (1)
92-95: Fix edge case: filenames without extensions return incorrect values.When a filename has no extension (e.g.,
"README"),lastIndexOf('.')returns-1, andsubstring(-1)incorrectly returns the last character instead of an empty string or indicating unsupported.export function isSupportedExtension(filename: string): boolean { - const ext = filename.toLowerCase().substring(filename.lastIndexOf('.')) + const lastDot = filename.lastIndexOf('.') + if (lastDot === -1) return false + const ext = filename.toLowerCase().substring(lastDot) return SUPPORTED_EXTENSIONS.includes(ext) } export function getFileExtension(filename: string): string { - return filename.toLowerCase().substring(filename.lastIndexOf('.')) + const lastDot = filename.lastIndexOf('.') + if (lastDot === -1) return '' + return filename.toLowerCase().substring(lastDot) }Also applies to: 107-109
🧹 Nitpick comments (5)
backend/app/services/attachment/attachment_service.py (1)
54-70: Improve error logging and centralize image-extension logic.Two refinements worth making:
- Use
logger.exceptionfor parse failures
Inupload_attachment, theexcept DocumentParseErrorblock currently useslogger.error(...). Switching tologger.exception(...)will capture stack traces for harder‑to‑reproduce parser bugs while still updating status toFAILED.- except DocumentParseError as e: - logger.error(f"Document parsing failed for attachment {attachment.id}: {e}") + except DocumentParseError as e: + logger.exception("Document parsing failed for attachment %s", attachment.id) attachment.status = AttachmentStatus.FAILED attachment.error_message = str(e) db.commit() raise
- Avoid duplicating image extension lists
build_message_with_attachmentkeeps its own list of image extensions. IfDocumentParser(or other validation) defines image types, consider exposing and reusing that list here to avoid future drift when supported image types change.Also applies to: 91-107, 252-267
backend/app/services/chat/chat_service.py (1)
241-249: Adjustdefault_headerstype hints to allowNone.Both
_call_openai_streamingand_call_claude_streamingdeclaredefault_headers: Dict[str, Any] = Nonebut treat it as optional (if default_headers:). For PEP 484 compliance and clearer intent under strict typing, declare it as optional:-from typing import Any, AsyncGenerator, Dict, List, Optional, Union +from typing import Any, AsyncGenerator, Dict, List, Optional, Union @@ - default_headers: Dict[str, Any] = None + default_headers: Dict[str, Any] | None = None @@ - default_headers: Dict[str, Any] = None + default_headers: Dict[str, Any] | None = NoneThis also addresses the implicit-Optional warning from static analysis.
Also applies to: 378-386
backend/app/services/attachment/parser.py (2)
358-362: Remove unnecessary f-string prefix.Line 358 uses an f-string without any placeholders.
-text = f"[图片文件]\n" +text = "[图片文件]\n"
86-86: Uselogging.exceptioninstead oflogging.errorin except blocks.When logging from an exception handler,
logger.exception()automatically includes the traceback without needingexc_info=True.-logger.error(f"Error uploading attachment: {e}", exc_info=True) +logger.exception(f"Error uploading attachment: {e}")This applies to multiple locations throughout the file (lines 86, 120, 139, 165, 203, 245, 283, 305, 336, 378).
Also applies to: 120-120
backend/app/api/endpoints/adapter/attachments.py (1)
85-87: Uselogging.exceptionin exception handlers.When logging from an exception handler, use
logger.exception()to automatically include the traceback.except Exception as e: - logger.error(f"Error reading uploaded file: {e}") - raise HTTPException(status_code=400, detail="Failed to read uploaded file") + logger.exception("Error reading uploaded file") + raise HTTPException(status_code=400, detail="Failed to read uploaded file") from eAlso applies to line 120.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/alembic/versions/add_image_base64_to_attachments.py(1 hunks)backend/app/api/endpoints/adapter/attachments.py(1 hunks)backend/app/models/subtask_attachment.py(1 hunks)backend/app/services/attachment/attachment_service.py(1 hunks)backend/app/services/attachment/parser.py(1 hunks)backend/app/services/chat/chat_service.py(1 hunks)backend/requirements.txt(1 hunks)docker/backend/Dockerfile(1 hunks)frontend/src/apis/attachments.ts(1 hunks)frontend/src/apis/chat.ts(1 hunks)frontend/src/app/api/chat/cancel/route.ts(1 hunks)frontend/src/features/tasks/components/ChatArea.tsx(17 hunks)frontend/src/features/tasks/components/FileUpload.tsx(1 hunks)frontend/src/features/tasks/components/MessagesArea.tsx(11 hunks)frontend/src/hooks/useChatStream.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/app/api/chat/cancel/route.ts
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{py,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
All code comments, inline comments, block comments, docstrings, TODO/FIXME annotations, and type hints descriptions MUST be written in English
Files:
frontend/src/hooks/useChatStream.tsbackend/alembic/versions/add_image_base64_to_attachments.pyfrontend/src/features/tasks/components/MessagesArea.tsxfrontend/src/features/tasks/components/FileUpload.tsxfrontend/src/features/tasks/components/ChatArea.tsxbackend/app/services/chat/chat_service.pyfrontend/src/apis/chat.tsbackend/app/models/subtask_attachment.pybackend/app/api/endpoints/adapter/attachments.pybackend/app/services/attachment/attachment_service.pyfrontend/src/apis/attachments.tsbackend/app/services/attachment/parser.py
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: TypeScript code MUST use strict mode with type checking enabled
TypeScript/React code MUST use Prettier formatter with single quotes, no semicolons
TypeScript/React code MUST pass ESLint with Next.js configuration
React component names MUST use PascalCase convention
Files:
frontend/src/hooks/useChatStream.tsfrontend/src/features/tasks/components/MessagesArea.tsxfrontend/src/features/tasks/components/FileUpload.tsxfrontend/src/features/tasks/components/ChatArea.tsxfrontend/src/apis/chat.tsfrontend/src/apis/attachments.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: React components MUST use functional components with hooks instead of class-based components
Useconstoverlet, never usevarin TypeScript/JavaScript code
Files:
frontend/src/hooks/useChatStream.tsfrontend/src/features/tasks/components/MessagesArea.tsxfrontend/src/features/tasks/components/FileUpload.tsxfrontend/src/features/tasks/components/ChatArea.tsxfrontend/src/apis/chat.tsfrontend/src/apis/attachments.ts
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code MUST be PEP 8 compliant with Black formatter (line length: 88) and isort for import organization
Python code MUST include type hints for all functions and variables
Python functions SHOULD NOT exceed 50 lines (preferred maximum)
Python functions and classes MUST have descriptive names and public functions/classes MUST include docstrings
Python code MUST extract magic numbers to named constants
Files:
backend/alembic/versions/add_image_base64_to_attachments.pybackend/app/services/chat/chat_service.pybackend/app/models/subtask_attachment.pybackend/app/api/endpoints/adapter/attachments.pybackend/app/services/attachment/attachment_service.pybackend/app/services/attachment/parser.py
**/backend/alembic/versions/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend Alembic database migrations MUST be reviewed before applying, especially auto-generated migrations
Files:
backend/alembic/versions/add_image_base64_to_attachments.py
**/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
React component files MUST use kebab-case naming convention
Files:
frontend/src/features/tasks/components/MessagesArea.tsxfrontend/src/features/tasks/components/FileUpload.tsxfrontend/src/features/tasks/components/ChatArea.tsx
**/src/**/*.{tsx,jsx,css}
📄 CodeRabbit inference engine (AGENTS.md)
Frontend Tailwind CSS MUST use provided CSS variables for color system (e.g., --color-bg-base, --color-text-primary, --color-primary)
Files:
frontend/src/features/tasks/components/MessagesArea.tsxfrontend/src/features/tasks/components/FileUpload.tsxfrontend/src/features/tasks/components/ChatArea.tsx
**/src/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/src/**/*.{tsx,jsx}: Frontend responsive design MUST follow mobile-first approach with Tailwind breakpoints
Frontend React forms MUST use react-hook-form with zod validation schema
Frontend components MUST use shadcn/ui component library fromfrontend/src/components/ui/
Files:
frontend/src/features/tasks/components/MessagesArea.tsxfrontend/src/features/tasks/components/FileUpload.tsxfrontend/src/features/tasks/components/ChatArea.tsx
**/backend/app/services/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend business logic MUST be placed in
app/services/directory
Files:
backend/app/services/chat/chat_service.pybackend/app/services/attachment/attachment_service.pybackend/app/services/attachment/parser.py
**/backend/app/models/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend SQLAlchemy models MUST be created in
app/models/directory
Files:
backend/app/models/subtask_attachment.py
**/backend/app/api/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend API route handlers MUST be created in
app/api/directory
Files:
backend/app/api/endpoints/adapter/attachments.py
🧠 Learnings (2)
📚 Learning: 2025-12-01T06:44:24.499Z
Learnt from: CR
Repo: wecode-ai/Wegent PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T06:44:24.499Z
Learning: Applies to **/backend/alembic/versions/*.py : Backend Alembic database migrations MUST be reviewed before applying, especially auto-generated migrations
Applied to files:
backend/alembic/versions/add_image_base64_to_attachments.py
📚 Learning: 2025-12-01T06:44:24.499Z
Learnt from: CR
Repo: wecode-ai/Wegent PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T06:44:24.499Z
Learning: Applies to **/*.py : Python code MUST include type hints for all functions and variables
Applied to files:
backend/app/services/attachment/parser.py
🧬 Code graph analysis (6)
frontend/src/hooks/useChatStream.ts (3)
frontend/src/apis/client.ts (1)
request(22-80)frontend/src/apis/chat.ts (4)
StreamChatRequest(40-59)streamChat(90-218)ChatStreamData(20-35)cancelChat(278-305)backend/app/api/endpoints/adapter/chat.py (1)
StreamChatRequest(38-52)
backend/app/services/chat/chat_service.py (4)
backend/app/services/chat/base.py (1)
get_http_client(29-58)backend/app/services/chat/session_manager.py (2)
get_chat_history(35-61)append_user_and_assistant_messages(122-149)backend/app/models/subtask.py (1)
Subtask(31-72)backend/app/models/kind.py (1)
Kind(25-42)
frontend/src/apis/chat.ts (2)
frontend/src/apis/client.ts (1)
request(22-80)frontend/src/apis/user.ts (1)
getToken(54-59)
backend/app/api/endpoints/adapter/attachments.py (3)
backend/app/models/subtask_attachment.py (1)
AttachmentStatus(21-26)backend/app/services/attachment/parser.py (3)
DocumentParseError(23-25)DocumentParser(28-379)validate_file_size(79-81)backend/app/services/attachment/attachment_service.py (1)
upload_attachment(31-116)
frontend/src/apis/attachments.ts (4)
backend/app/models/subtask_attachment.py (1)
AttachmentStatus(21-26)frontend/src/types/api.ts (1)
AttachmentStatus(398-398)backend/app/api/endpoints/adapter/attachments.py (2)
AttachmentResponse(29-41)AttachmentDetailResponse(44-49)frontend/src/apis/user.ts (1)
getToken(54-59)
backend/app/services/attachment/parser.py (1)
frontend/src/apis/attachments.ts (2)
SUPPORTED_EXTENSIONS(44-61)MAX_FILE_SIZE(87-87)
🪛 Ruff (0.14.7)
backend/app/services/chat/chat_service.py
115-115: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
120-120: Use explicit conversion flag
Replace with conversion flag
(RUF010)
121-121: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
227-227: Avoid specifying long messages outside the exception class
(TRY003)
248-248: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
302-302: f-string without any placeholders
Remove extraneous f prefix
(F541)
339-339: f-string without any placeholders
Remove extraneous f prefix
(F541)
342-342: Local variable masked_key is assigned to but never used
Remove assignment to unused variable masked_key
(F841)
385-385: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
586-586: Do not catch blind exception: Exception
(BLE001)
587-587: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
611-611: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
656-656: Do not catch blind exception: Exception
(BLE001)
657-657: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
backend/app/api/endpoints/adapter/attachments.py
54-54: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
55-55: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
56-56: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
85-85: Do not catch blind exception: Exception
(BLE001)
86-86: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
87-87: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
116-116: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
118-118: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
121-121: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
127-127: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
128-128: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
162-162: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
163-163: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
192-192: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
193-193: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
233-233: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
234-234: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
backend/app/services/attachment/attachment_service.py
60-63: Avoid specifying long messages outside the exception class
(TRY003)
69-69: Avoid specifying long messages outside the exception class
(TRY003)
102-102: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
backend/app/services/attachment/parser.py
43-60: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
106-106: Avoid specifying long messages outside the exception class
(TRY003)
126-126: Abstract raise to an inner function
(TRY301)
126-126: Avoid specifying long messages outside the exception class
(TRY003)
130-132: Abstract raise to an inner function
(TRY301)
130-132: Avoid specifying long messages outside the exception class
(TRY003)
140-140: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
140-140: Avoid specifying long messages outside the exception class
(TRY003)
140-140: Use explicit conversion flag
Replace with conversion flag
(RUF010)
152-152: Abstract raise to an inner function
(TRY301)
152-152: Avoid specifying long messages outside the exception class
(TRY003)
166-166: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
166-166: Avoid specifying long messages outside the exception class
(TRY003)
166-166: Use explicit conversion flag
Replace with conversion flag
(RUF010)
174-176: Abstract raise to an inner function
(TRY301)
174-176: Avoid specifying long messages outside the exception class
(TRY003)
204-204: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
204-204: Avoid specifying long messages outside the exception class
(TRY003)
204-204: Use explicit conversion flag
Replace with conversion flag
(RUF010)
210-212: Abstract raise to an inner function
(TRY301)
210-212: Avoid specifying long messages outside the exception class
(TRY003)
246-246: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
246-246: Avoid specifying long messages outside the exception class
(TRY003)
246-246: Use explicit conversion flag
Replace with conversion flag
(RUF010)
252-254: Abstract raise to an inner function
(TRY301)
252-254: Avoid specifying long messages outside the exception class
(TRY003)
284-284: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
284-284: Avoid specifying long messages outside the exception class
(TRY003)
284-284: Use explicit conversion flag
Replace with conversion flag
(RUF010)
306-306: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
306-306: Avoid specifying long messages outside the exception class
(TRY003)
306-306: Use explicit conversion flag
Replace with conversion flag
(RUF010)
320-321: try-except-pass detected, consider logging the exception
(S110)
320-320: Do not catch blind exception: Exception
(BLE001)
337-337: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
337-337: Avoid specifying long messages outside the exception class
(TRY003)
337-337: Use explicit conversion flag
Replace with conversion flag
(RUF010)
358-358: f-string without any placeholders
Remove extraneous f prefix
(F541)
375-375: Consider moving this statement to an else block
(TRY300)
379-379: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
379-379: Avoid specifying long messages outside the exception class
(TRY003)
379-379: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test wegent CLI Integration
- GitHub Check: Test Backend (3.11)
- GitHub Check: Test Backend (3.10)
- GitHub Check: e2e-tests
🔇 Additional comments (13)
docker/backend/Dockerfile (1)
14-14: Formatting improvement approved.The added blank line improves readability by separating the dependency installation section from the source code copy block. No functional changes.
backend/alembic/versions/add_image_base64_to_attachments.py (1)
25-35: Migration shape looks correct and reversible.Upgrade adds a nullable
image_base64Textcolumn and downgrade drops it on the same table, matching the model change and keeping the revision reversible. No issues from an Alembic standpoint.frontend/src/features/tasks/components/MessagesArea.tsx (4)
169-169: LGTM! Typewriter effect properly handles empty content.The typewriter hook correctly handles the case when
streamingContentis undefined by using|| ''to provide an empty string fallback.
171-181: LGTM! Proper chat shell detection and polling prevention.The case-insensitive comparison for
agent_typeis correct, and disabling polling for chat shell mode prevents unnecessary server requests during streaming.
576-591: LGTM! Clean attachment rendering implementation.The
renderAttachmentshelper correctly handles empty/null arrays and delegates to theAttachmentPreviewcomponent with appropriate props.
839-888: LGTM! Well-structured streaming UI with proper fallbacks.The streaming UI correctly:
- Shows pending user messages optimistically
- Displays typewriter effect with markdown rendering
- Provides loading state when no content is available yet
- Uses proper accessibility with blinking cursor indicator
frontend/src/features/tasks/components/ChatArea.tsx (3)
320-365: LGTM! Robust drag-and-drop implementation.The drag-and-drop handlers correctly:
- Prevent default behavior and stop propagation
- Check for chat shell mode before allowing drops
- Validate state (loading, streaming, existing attachment)
- Handle drag-leave edge cases by checking if leaving to child elements
736-754: LGTM! Polished drag overlay with clear visual feedback.The drag overlay provides excellent UX with:
- Semi-transparent backdrop with blur effect
- Animated bounce effect for the upload icon
- Clear file format instructions in Chinese
- Proper z-index layering
825-887: LGTM! Comprehensive button state management.The streaming controls properly handle all states:
- Shows stop button during streaming
- Disables send button when streaming, loading, or attachment not ready
- Maintains existing task status buttons (PENDING, RUNNING, CANCELLING)
- Correctly manages button visual feedback
frontend/src/apis/attachments.ts (2)
164-225: LGTM! Well-implemented upload with progress tracking.The upload function correctly:
- Validates file type and size before upload
- Uses XMLHttpRequest for progress tracking
- Provides clear error messages in Chinese
- Handles all response states (success, error, abort)
- Parses error details from response body
268-291: LGTM! Proper file download with cleanup.The download function correctly:
- Creates a blob URL for the file
- Creates and clicks a temporary anchor element
- Properly cleans up the blob URL after download
- Removes the temporary DOM element
backend/app/api/endpoints/adapter/attachments.py (2)
52-122: LGTM! Comprehensive upload endpoint with proper validation.The upload endpoint correctly:
- Validates filename presence
- Reads and validates file size before processing
- Delegates parsing to the service layer
- Handles all error types appropriately (ValueError, DocumentParseError, generic Exception)
- Returns detailed attachment information including status
189-227: LGTM! Safe deletion with proper constraints.The delete endpoint correctly:
- Verifies attachment ownership
- Prevents deletion of attachments linked to subtasks
- Provides clear error messages
- Handles all failure cases
| except ValueError as e: | ||
| raise HTTPException(status_code=400, detail=str(e)) | ||
| except DocumentParseError as e: | ||
| raise HTTPException(status_code=400, detail=str(e)) | ||
| except Exception as e: | ||
| logger.error(f"Error uploading attachment: {e}", exc_info=True) | ||
| raise HTTPException(status_code=500, detail="Failed to upload attachment") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add exception chaining with from clause.
When converting exceptions to HTTPException, preserve the original exception chain for better error tracking.
except ValueError as e:
- raise HTTPException(status_code=400, detail=str(e))
+ raise HTTPException(status_code=400, detail=str(e)) from e
except DocumentParseError as e:
- raise HTTPException(status_code=400, detail=str(e))
+ raise HTTPException(status_code=400, detail=str(e)) from e
except Exception as e:
logger.error(f"Error uploading attachment: {e}", exc_info=True)
- raise HTTPException(status_code=500, detail="Failed to upload attachment")
+ raise HTTPException(status_code=500, detail="Failed to upload attachment") from e🧰 Tools
🪛 Ruff (0.14.7)
116-116: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
118-118: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
121-121: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In backend/app/api/endpoints/adapter/attachments.py around lines 115 to 121, the
code raises new HTTPException instances without exception chaining; update the
three raise statements to include the original exception using "from e" (e.g.,
raise HTTPException(status_code=400, detail=str(e)) from e) so the original
traceback is preserved for ValueError, DocumentParseError, and the generic
Exception branch (keep the logger.error call, then re-raise the HTTPException
from the caught exception).
| raise | ||
| except Exception as e: | ||
| logger.error(f"Error parsing document: {e}", exc_info=True) | ||
| raise DocumentParseError(f"Failed to parse document: {str(e)}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add exception chaining with from clause.
When re-raising exceptions within an except block, use raise ... from err to preserve the original exception chain for better debugging.
Apply this pattern throughout the file:
except DocumentParseError:
raise
except Exception as e:
logger.error(f"Error parsing document: {e}", exc_info=True)
- raise DocumentParseError(f"Failed to parse document: {str(e)}")
+ raise DocumentParseError(f"Failed to parse document: {str(e)}") from eThis applies to error handling in _parse_pdf, _parse_word, _parse_powerpoint, _parse_excel, _parse_csv, _parse_text, and _parse_image.
Also applies to: 166-166, 204-204, 246-246, 284-284, 306-306, 337-337, 379-379
🧰 Tools
🪛 Ruff (0.14.7)
140-140: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
140-140: Avoid specifying long messages outside the exception class
(TRY003)
140-140: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In backend/app/services/attachment/parser.py around lines 140, 166, 204, 246,
284, 306, 337, and 379, several except blocks re-raise DocumentParseError (or
other exceptions) without preserving the original exception; update each raise
to use exception chaining (e.g., raise DocumentParseError(f"...") from e) so the
original exception is attached, ensuring you capture the caught exception
variable name consistently (e or err) and add the from clause in each affected
handler (_parse_pdf, _parse_word, _parse_powerpoint, _parse_excel, _parse_csv,
_parse_text, _parse_image).
| except Exception: | ||
| pass # Ignore chardet errors |
There was a problem hiding this comment.
Log exceptions instead of silently ignoring them.
The bare except: pass silently suppresses chardet errors, making debugging difficult. At minimum, log the exception.
try:
detected = chardet.detect(binary_data)
detected_encoding = detected.get("encoding")
if detected_encoding and detected_encoding.lower() not in ["ascii", "charmap"]:
encodings_to_try.insert(0, detected_encoding)
-except Exception:
- pass # Ignore chardet errors
+except Exception as e:
+ logger.debug(f"Chardet encoding detection failed: {e}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception: | |
| pass # Ignore chardet errors | |
| try: | |
| detected = chardet.detect(binary_data) | |
| detected_encoding = detected.get("encoding") | |
| if detected_encoding and detected_encoding.lower() not in ["ascii", "charmap"]: | |
| encodings_to_try.insert(0, detected_encoding) | |
| except Exception as e: | |
| logger.debug(f"Chardet encoding detection failed: {e}") |
🧰 Tools
🪛 Ruff (0.14.7)
320-321: try-except-pass detected, consider logging the exception
(S110)
320-320: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In backend/app/services/attachment/parser.py around lines 320 to 321, the bare
"except: pass" silently swallows chardet errors; change it to catch Exception as
e and log the exception (e.g., use the module/logger's logger.exception or
logger.error with traceback) so errors are recorded, then continue; ensure you
do not re-raise, only replace the pass with a logging call that includes the
exception details.
| interface UseChatStreamReturn { | ||
| /** Whether streaming is in progress */ | ||
| isStreaming: boolean; | ||
| /** Accumulated streaming content */ | ||
| streamingContent: string; | ||
| /** Error if any */ | ||
| error: Error | null; | ||
| /** Task ID from the stream */ | ||
| taskId: number | null; | ||
| /** Subtask ID from the stream */ | ||
| subtaskId: number | null; | ||
| /** Start a new stream */ | ||
| startStream: (request: StreamChatRequest) => Promise<number>; | ||
| /** Stop the current stream */ | ||
| stopStream: () => void; | ||
| /** Reset streaming state */ | ||
| resetStream: () => void; | ||
| } |
There was a problem hiding this comment.
Fix stopStream type mismatch and stale subtaskIdRef between runs.
Two issues here:
-
Type mismatch
UseChatStreamReturndeclaresstopStream: () => void, butstopStreamisasync, so its actual type is() => Promise<void>. With strict TS this will not type‑check. -
Stale subtask ID on new streams
subtaskIdRefis only updated when a streamed message arrives. InstartStreamyou reset React state withsetSubtaskId(null)but never clearsubtaskIdRef. If a new stream is started and stopped before any chunk arrives,stopStreamwill callcancelChatusing the previous subtask ID.
A minimal fix could be:
-interface UseChatStreamReturn {
+interface UseChatStreamReturn {
// ...
- /** Stop the current stream */
- stopStream: () => void;
+ /** Stop the current stream */
+ stopStream: () => Promise<void>;
// ...
}
@@
const startStream = useCallback(async (request: StreamChatRequest): Promise<number> => {
@@
- setSubtaskId(null);
+ setSubtaskId(null);
+ subtaskIdRef.current = null;
@@
- const stopStream = useCallback(async () => {
+ const stopStream = useCallback(async () => {
// First abort the frontend fetch request
@@
}, []);Also applies to: 144-177
🤖 Prompt for AI Agents
In frontend/src/hooks/useChatStream.ts around lines 29 to 46 (and also apply
same change in the other occurrence at 144-177), the interface declares
stopStream as () => void but the implementation is async, and subtaskIdRef is
not cleared when a new stream starts causing stopStream to potentially cancel
using a stale subtask ID; change the stopStream signature to () => Promise<void>
in the UseChatStreamReturn type, and in startStream (and resetStream) explicitly
set subtaskIdRef.current = null when initializing a new stream so the ref
matches the React state; ensure any call sites/types are updated to await
stopStream as needed.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
frontend/src/hooks/useChatStream.ts (2)
44-45: Type mismatch:stopStreamisasyncbut declared as() => void.The interface declares
stopStream: () => voidbut the implementation at line 160 isasync, making its actual return type() => Promise<void>. This will cause TypeScript strict mode errors.- /** Stop the current stream */ - stopStream: () => void; + /** Stop the current stream */ + stopStream: () => Promise<void>;
100-108: ResetsubtaskIdRefwhen starting a new stream to prevent stale ID usage.
subtaskIdRef.currentis not cleared instartStream. If a new stream starts andstopStreamis called before any chunk arrives, it will use the previous stream's subtask ID.setSubtaskId(null); + subtaskIdRef.current = null; let resolvedTaskId = request.task_id || 0;frontend/src/features/tasks/components/ChatArea.tsx (2)
138-145: AddresetAttachment()to the error handler for complete cleanup.The
onErrorcallback clears pending UI state (setPendingUserMessage,setPendingAttachment) but doesn't callresetAttachment()to clean up the attachment hook's internal state. This leaves stale attachment state after streaming failures.onError: error => { setPendingUserMessage(null); setPendingAttachment(null); + resetAttachment(); toast({ variant: 'destructive', title: error.message, }); },
447-453: Clean up attachment state in the catch block as well.The catch block for
startStreamdoesn't reset pending attachment or attachment state, unlike theonErrorcallback. This inconsistency could leave stale state on early failures.} catch (err) { setPendingUserMessage(null); + setPendingAttachment(null); + resetAttachment(); toast({ variant: 'destructive', title: (err as Error)?.message || 'Failed to start chat stream', }); }backend/app/api/endpoints/adapter/chat.py (1)
303-345:completed_atplaceholder for PENDING assistant subtask should be removed once DB column is nullableThe assistant subtask is initialized with
status=PENDINGbutcompleted_at=datetime.now()and a comment explains this is a workaround because the DB column does not allow NULL:assistant_subtask = Subtask( ... status=SubtaskStatus.PENDING, ... completed_at=datetime.now(), # Placeholder, will be updated when stream completes )This keeps the ORM happy but semantically conflates “pending” with “completed now” and repeats the schema mismatch highlighted in the previous review on this topic. Longer-term, the better fix remains:
- Make
completed_atnullable at the DB level (via migration), consistent with the Pydantic/ORM schema.- For pending/running subtasks, either omit
completed_ator set it toNone.Once that migration is in place, this placeholder assignment should be removed to make status/timestamps consistent.
🧹 Nitpick comments (10)
frontend/src/hooks/useChatStream.ts (1)
214-220: Also resetsubtaskIdRefinresetStreamfor consistency.
resetStreamclearsstreamingContentRefbut notsubtaskIdRef, which could leave stale state.const resetStream = useCallback(() => { setStreamingContent(''); streamingContentRef.current = ''; // Reset ref too setError(null); setTaskId(null); setSubtaskId(null); + subtaskIdRef.current = null; }, []);frontend/src/features/tasks/components/MessagesArea.tsx (2)
283-295:useLayoutEffectwithonContentChangein dependencies may cause excessive calls.Including
onContentChangein the dependency array could cause the effect to run even when only the callback reference changes. IfonContentChangeis not memoized by the parent, this will trigger on every parent render.Consider using a ref for the callback to avoid dependency issues:
+ const onContentChangeRef = useRef(onContentChange); + onContentChangeRef.current = onContentChange; useLayoutEffect(() => { - if (onContentChange) { - onContentChange(); + if (onContentChangeRef.current) { + onContentChangeRef.current(); } - }, [ - displayMessages, - displayContent, - pendingUserMessage, - pendingAttachment, - isStreaming, - onContentChange, - ]); + }, [displayMessages, displayContent, pendingUserMessage, pendingAttachment, isStreaming]);
32-113: Extract sharedCopyButtonandBubbleToolscomponents to reduce duplication.
CopyButtonandBubbleToolsare duplicated in bothMessagesArea.tsxandMessageBubble.tsx. Consider extracting these to a shared location (e.g.,@/components/ui/or a local shared file).frontend/src/features/tasks/components/ChatArea.tsx (1)
758-918: Extract the input area UI to a shared component to reduce duplication.The input area UI (drag overlay, FileUpload, ChatInput, selectors, send/stop buttons) is duplicated between the main view (lines 758-918) and floating view (lines 970-1126). This ~160-line duplication increases maintenance burden and risk of divergence.
Consider extracting to a
ChatInputCardcomponent that accepts layout-specific props.frontend/src/features/tasks/components/MessageBubble.tsx (2)
586-596: Memo comparison may missattachmentschanges.The custom comparison function checks
content,subtaskStatus,subtaskId,timestamp, andtheme, but doesn't includeattachments. If attachments change independently, the component won't re-render.(prevProps, nextProps) => { return ( prevProps.msg.content === nextProps.msg.content && prevProps.msg.subtaskStatus === nextProps.msg.subtaskStatus && prevProps.msg.subtaskId === nextProps.msg.subtaskId && prevProps.msg.timestamp === nextProps.msg.timestamp && - prevProps.theme === nextProps.theme + prevProps.theme === nextProps.theme && + prevProps.msg.attachments === nextProps.msg.attachments ); }
380-461: Consider extracting parsing helpers to a utility file.The
parseMarkdownClarificationandparseMarkdownFinalPromptfunctions contain substantial parsing logic (~80 lines each). Extracting them to a utility file would improve testability and maintainability.backend/app/api/endpoints/adapter/chat.py (4)
103-141: Clarify_should_use_direct_chatuser_idsemantics and remove unused parameter
_should_use_direct_chatacceptsuser_id, but internally only usesteam.user_id(and passesteam.user_iddown to_get_shell_type). Theuser_idparameter is effectively unused and flagged by static analysis.Given the call sites pass
current_user.id, this is confusing and potentially misleading. I suggest either:
- If the current user’s custom shells should be considered, switch the internal queries (and
_get_shell_typecalls) to actually use theuser_idargument; or- If “user” here is always the team owner, drop the
user_idparameter from_should_use_direct_chatand its call sites, and rely solely onteam.user_id(and adjust the docstring accordingly).While adjusting, you can also simplify
Kind.is_active == Trueto the SQLAlchemy-preferred style (Kind.is_activeor.is_(True)), but that’s cosmetic.
144-288: Split_create_task_and_subtasksinto smaller helpers and extract magic numbers
_create_task_and_subtasksis quite large and mixes several responsibilities (bot resolution, task-id creation/validation, workspace creation, task JSON construction, and subtask sequencing). This makes it harder to test and reason about.Consider:
- Extracting helpers like
resolve_or_create_task,create_workspace_kind, andcompute_next_message_idsto keep this function closer to the ~50-line guideline.- Pulling out magic values into named constants (e.g., the
50-char title truncation limit,"workspace-"/"task-"prefixes, label literals like"chat","online","chat_shell"), so they can be reused and adjusted safely.These changes would improve readability and make it easier to modify task/subtask behavior later without touching one large function.
472-531: Clean up unusedbot_crdand double-check header placeholder inputsIn the model-config/system-prompt and header-building block:
bot_crd = Bot.model_validate(bot.json) bot_json = bot.json or {} bot_spec = bot_json.get("spec", {}) agent_config = bot_spec.get("agent_config", {}) ... raw_default_headers = model_config.get("default_headers", {}) if raw_default_headers: processed_headers = build_default_headers_with_placeholders( raw_default_headers, data_sources ) model_config["default_headers"] = processed_headers
bot_crdis never used after being created, so it can be removed unless you plan to deriveagent_configor other fields from it.- Currently,
agent_configis taken frombot.json["spec"]["agent_config"]rather than frombot_crd.spec. That’s fine, but if your canonical source is the CRD model, you might want to usebot_crdinstead for consistency and validation.The placeholder replacement logic itself looks solid and mirrors the executor’s behavior; this is mostly about trimming the unused variable and keeping the data-source origin consistent.
622-687: Consider marking truncated responses explicitly when cancelling chat
cancel_chatmarks the subtask/task asCOMPLETED, setsprogress = 100, and storespartial_contentintosubtask.result["value"]:subtask.status = SubtaskStatus.COMPLETED subtask.progress = 100 ... if request.partial_content: subtask.result = {"value": request.partial_content} else: subtask.result = {"value": ""}This is a nice UX choice (lets users continue the conversation), but there’s no explicit flag in the result or task JSON indicating that this completion was user-truncated rather than naturally finished. Downstream consumers and history UIs might not be able to distinguish a cancelled-but-partial answer from a full one.
If that distinction matters, consider:
- Adding a small flag (e.g.,
{"value": ..., "truncated": true}) tosubtask.result, and/or- Recording a lightweight status marker in
task_crd.statusso analytics/UX can tell real completions from cancellations.The current behavior is functionally fine; this would just improve observability and client behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/app/api/endpoints/adapter/chat.py(1 hunks)frontend/src/features/tasks/components/ChatArea.tsx(20 hunks)frontend/src/features/tasks/components/MessageBubble.tsx(1 hunks)frontend/src/features/tasks/components/MessagesArea.tsx(5 hunks)frontend/src/hooks/useChatStream.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{py,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
All code comments, inline comments, block comments, docstrings, TODO/FIXME annotations, and type hints descriptions MUST be written in English
Files:
frontend/src/hooks/useChatStream.tsfrontend/src/features/tasks/components/MessageBubble.tsxfrontend/src/features/tasks/components/MessagesArea.tsxbackend/app/api/endpoints/adapter/chat.pyfrontend/src/features/tasks/components/ChatArea.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: TypeScript code MUST use strict mode with type checking enabled
TypeScript/React code MUST use Prettier formatter with single quotes, no semicolons
TypeScript/React code MUST pass ESLint with Next.js configuration
React component names MUST use PascalCase convention
Files:
frontend/src/hooks/useChatStream.tsfrontend/src/features/tasks/components/MessageBubble.tsxfrontend/src/features/tasks/components/MessagesArea.tsxfrontend/src/features/tasks/components/ChatArea.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: React components MUST use functional components with hooks instead of class-based components
Useconstoverlet, never usevarin TypeScript/JavaScript code
Files:
frontend/src/hooks/useChatStream.tsfrontend/src/features/tasks/components/MessageBubble.tsxfrontend/src/features/tasks/components/MessagesArea.tsxfrontend/src/features/tasks/components/ChatArea.tsx
**/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
React component files MUST use kebab-case naming convention
Files:
frontend/src/features/tasks/components/MessageBubble.tsxfrontend/src/features/tasks/components/MessagesArea.tsxfrontend/src/features/tasks/components/ChatArea.tsx
**/src/**/*.{tsx,jsx,css}
📄 CodeRabbit inference engine (AGENTS.md)
Frontend Tailwind CSS MUST use provided CSS variables for color system (e.g., --color-bg-base, --color-text-primary, --color-primary)
Files:
frontend/src/features/tasks/components/MessageBubble.tsxfrontend/src/features/tasks/components/MessagesArea.tsxfrontend/src/features/tasks/components/ChatArea.tsx
**/src/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/src/**/*.{tsx,jsx}: Frontend responsive design MUST follow mobile-first approach with Tailwind breakpoints
Frontend React forms MUST use react-hook-form with zod validation schema
Frontend components MUST use shadcn/ui component library fromfrontend/src/components/ui/
Files:
frontend/src/features/tasks/components/MessageBubble.tsxfrontend/src/features/tasks/components/MessagesArea.tsxfrontend/src/features/tasks/components/ChatArea.tsx
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code MUST be PEP 8 compliant with Black formatter (line length: 88) and isort for import organization
Python code MUST include type hints for all functions and variables
Python functions SHOULD NOT exceed 50 lines (preferred maximum)
Python functions and classes MUST have descriptive names and public functions/classes MUST include docstrings
Python code MUST extract magic numbers to named constants
Files:
backend/app/api/endpoints/adapter/chat.py
**/backend/app/api/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend API route handlers MUST be created in
app/api/directory
Files:
backend/app/api/endpoints/adapter/chat.py
🧬 Code graph analysis (3)
frontend/src/hooks/useChatStream.ts (1)
frontend/src/apis/chat.ts (4)
StreamChatRequest(40-59)streamChat(90-218)ChatStreamData(20-35)cancelChat(278-305)
frontend/src/features/tasks/components/MessagesArea.tsx (6)
frontend/src/types/api.ts (1)
Attachment(400-411)frontend/src/features/tasks/contexts/taskContext.tsx (1)
useTaskContext(284-290)frontend/src/features/theme/ThemeProvider.tsx (1)
useTheme(106-112)frontend/src/hooks/useTypewriter.ts (1)
useTypewriter(17-67)frontend/src/features/tasks/service/messageService.ts (1)
isChatShell(15-26)frontend/src/features/tasks/components/AttachmentPreview.tsx (1)
AttachmentPreview(26-85)
frontend/src/features/tasks/components/ChatArea.tsx (6)
frontend/src/hooks/useAttachment.ts (1)
useAttachment(33-179)frontend/src/features/tasks/contexts/taskContext.tsx (1)
useTaskContext(284-290)frontend/src/hooks/useChatStream.ts (1)
useChatStream(77-233)frontend/src/features/tasks/service/messageService.ts (1)
isChatShell(15-26)frontend/src/features/tasks/components/ModelSelector.tsx (1)
DEFAULT_MODEL_NAME(40-40)frontend/src/features/tasks/components/FileUpload.tsx (1)
FileUpload(39-198)
🪛 Ruff (0.14.7)
backend/app/api/endpoints/adapter/chat.py
69-69: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
76-76: f-string without any placeholders
Remove extraneous f prefix
(F541)
83-83: Avoid equality comparisons to True; use PublicShell.is_active: for truth checks
Replace with PublicShell.is_active
(E712)
91-91: f-string without any placeholders
Remove extraneous f prefix
(F541)
99-99: f-string without any placeholders
Remove extraneous f prefix
(F541)
103-103: Unused function argument: user_id
(ARG001)
123-123: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
129-129: f-string without any placeholders
Remove extraneous f prefix
(F541)
140-140: f-string without any placeholders
Remove extraneous f prefix
(F541)
170-170: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
190-190: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
308-308: f-string without any placeholders
Remove extraneous f prefix
(F541)
331-331: f-string without any placeholders
Remove extraneous f prefix
(F541)
357-357: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
358-358: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
384-384: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
464-464: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
482-482: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
491-491: Local variable bot_crd is assigned to but never used
Remove assignment to unused variable bot_crd
(F841)
573-573: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
574-574: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
587-587: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
609-609: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
632-632: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
633-633: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
687-687: f-string without any placeholders
Remove extraneous f prefix
(F541)
695-695: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test wegent CLI Integration
- GitHub Check: Test Backend (3.11)
- GitHub Check: Test Backend (3.10)
- GitHub Check: e2e-tests
🔇 Additional comments (4)
frontend/src/features/tasks/components/MessagesArea.tsx (1)
337-398: LGTM on streaming content rendering.The conditional rendering logic correctly handles both active streaming (showing thinking indicator) and completed streaming (showing final content with copy/download tools). The typewriter effect provides a smooth UX.
frontend/src/features/tasks/components/ChatArea.tsx (1)
396-457: LGTM on Chat Shell streaming flow.The streaming mode path correctly handles optimistic UI updates, attachment handling, and task creation. The logic properly distinguishes between new tasks and existing conversations.
frontend/src/features/tasks/components/MessageBubble.tsx (1)
138-148: LGTM on component structure and memoization.The component properly uses
React.memowith a custom comparison function to optimize re-renders. The separation of user vs AI message rendering is clean and maintainable.backend/app/api/endpoints/adapter/chat.py (1)
536-567: Streaming wrapper overchat_service.chat_streamlooks reasonableThe
generate_with_idswrapper that emits an initial SSE withtask_id/subtask_idand then forwards chunks fromchat_service.chat_streamis straightforward and should integrate well with clients expecting SSE.Assuming
chat_service.chat_streamreturns aStreamingResponsewith SSE-formattedbody_iteratorchunks, this structure is fine.
| import logging | ||
| from datetime import datetime | ||
| from typing import Optional | ||
|
|
||
| from fastapi import APIRouter, Depends, HTTPException | ||
| from pydantic import BaseModel | ||
| from sqlalchemy.orm import Session | ||
|
|
||
| from app.api.dependencies import get_db | ||
| from app.core import security | ||
| from app.models.kind import Kind | ||
| from app.models.subtask import Subtask, SubtaskRole, SubtaskStatus | ||
| from app.models.user import User | ||
| from app.schemas.kind import Bot, Shell, Task, Team | ||
| from app.services.chat.base import ChatServiceBase | ||
| from app.services.chat.chat_service import chat_service | ||
| from app.services.chat.model_resolver import ( | ||
| build_default_headers_with_placeholders, | ||
| get_bot_system_prompt, | ||
| get_model_config_for_bot, | ||
| ) | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| router = APIRouter() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add missing type hints and run Black/isort to meet project Python standards
Per the repo guidelines, Python code here should be Black-formatted (88 cols), isort-organized, and fully type-annotated for functions and variables. This module currently has unannotated returns/locals and several lines that would be reflowed by Black. Please:
- Add explicit return types to all helpers and route handlers in this file.
- Add type hints where appropriate for module-level variables (e.g.,
logger,router) and key locals. - Run Black + isort over this module to align formatting/import ordering with the rest of the codebase.
| # Validate team exists | ||
| team = ( | ||
| db.query(Kind) | ||
| .filter( | ||
| Kind.id == request.team_id, | ||
| Kind.kind == "Team", | ||
| Kind.is_active == True, | ||
| ) | ||
| .first() | ||
| ) | ||
|
|
||
| if not team: | ||
| raise HTTPException(status_code=404, detail="Team not found") | ||
|
|
||
| # Check if team supports direct chat | ||
| if not _should_use_direct_chat(db, team, current_user.id): | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail="This team does not support direct chat. Please use the task API instead." | ||
| ) | ||
|
|
||
| # Handle attachment if provided |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -nP "Kind\.kind == \"Team\"" backend/app/api -C3Repository: wecode-ai/Wegent
Length of output: 1050
🏁 Script executed:
#!/bin/bash
# Get the full stream_chat function
sed -n '370,420p' backend/app/api/endpoints/adapter/chat.py
echo "---"
# Get the full check_direct_chat function
sed -n '575,610p' backend/app/api/endpoints/adapter/chat.py
echo "---"
# Look for Kind model definition
fd -e py . backend/app | xargs rg "class Kind" -A 10Repository: wecode-ai/Wegent
Length of output: 5910
🏁 Script executed:
#!/bin/bash
# Find _should_use_direct_chat function
rg -n "_should_use_direct_chat" backend/app/api -A 15
echo "---"
# Look for other team endpoints to see authorization patterns
rg -n "def.*team" backend/app/api/endpoints -i -A 3 | head -100
echo "---"
# Search for how other endpoints authorize team access
rg -n "Kind.user_id == current_user.id" backend/app/api -B 3 -A 3 | head -80Repository: wecode-ai/Wegent
Length of output: 13402
🏁 Script executed:
#!/bin/bash
# Check the get_team endpoint implementation
sed -n '63,74p' backend/app/api/endpoints/adapter/teams.py
echo "---"
# Check the list_teams endpoint implementation
sed -n '32,50p' backend/app/api/endpoints/adapter/teams.py
echo "---"
# Look for any team authorization helper functions
rg -n "def.*team" backend/app/services -i -A 5 | head -80Repository: wecode-ai/Wegent
Length of output: 7276
🏁 Script executed:
#!/bin/bash
# View the get_team_detail implementation
sed -n '310,350p' backend/app/services/adapters/team_kinds.pyRepository: wecode-ai/Wegent
Length of output: 1509
🏁 Script executed:
#!/bin/bash
# Check if stream_chat and check_direct_chat are actually route handlers
rg -n "@router\.|@app\." backend/app/api/endpoints/adapter/chat.py | head -20Repository: wecode-ai/Wegent
Length of output: 163
Add user authorization check to team lookups in stream_chat and check_direct_chat endpoints
Both stream_chat (line 378-386) and check_direct_chat (line 583-589) query teams without validating that the current user owns or has shared access to the team. This allows any authenticated user to trigger direct-chat operations against another user's team.
The proper authorization pattern is already implemented in team_kinds_service.get_team_detail(), which checks:
team.user_id == user_id(ownership)SharedTeamtable for shared access (if not owner)
Update both functions to validate user authorization. Either:
- Add filters:
Kind.user_id == current_user.idand checkSharedTeamfor shared access, or - Refactor to use
team_kinds_service.get_team_detail(team_id, current_user.id)which already handles this validation
🧰 Tools
🪛 Ruff (0.14.7)
384-384: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
| @router.get("/check-direct-chat/{team_id}") | ||
| async def check_direct_chat( | ||
| team_id: int, | ||
| db: Session = Depends(get_db), | ||
| current_user: User = Depends(security.get_current_user), | ||
| ): | ||
| """ | ||
| Check if a team supports direct chat mode. | ||
| Returns: | ||
| {"supports_direct_chat": bool, "shell_type": str} | ||
| """ | ||
| team = ( | ||
| db.query(Kind) | ||
| .filter( | ||
| Kind.id == team_id, | ||
| Kind.kind == "Team", | ||
| Kind.is_active == True, | ||
| ) | ||
| .first() | ||
| ) | ||
|
|
||
| if not team: | ||
| raise HTTPException(status_code=404, detail="Team not found") | ||
|
|
||
| supports_direct_chat = _should_use_direct_chat(db, team, current_user.id) | ||
|
|
||
| # Get shell type of first bot | ||
| shell_type = "" | ||
| team_crd = Team.model_validate(team.json) | ||
| if team_crd.spec.members: | ||
| first_member = team_crd.spec.members[0] | ||
| bot = ( | ||
| db.query(Kind) | ||
| .filter( | ||
| Kind.user_id == team.user_id, | ||
| Kind.kind == "Bot", | ||
| Kind.name == first_member.botRef.name, | ||
| Kind.namespace == first_member.botRef.namespace, | ||
| Kind.is_active == True, | ||
| ) | ||
| .first() | ||
| ) | ||
| if bot: | ||
| shell_type = _get_shell_type(db, bot, team.user_id) | ||
|
|
||
| return { | ||
| "supports_direct_chat": supports_direct_chat, | ||
| "shell_type": shell_type, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd backend && find . -name "chat.py" -path "*/api/endpoints/adapter/*" -type fRepository: wecode-ai/Wegent
Length of output: 95
🏁 Script executed:
cat -n backend/app/api/endpoints/adapter/chat.py | head -100Repository: wecode-ai/Wegent
Length of output: 4142
🏁 Script executed:
rg -n "def stream_chat|@router.get.*stream" backend/app/api/endpoints/adapter/chat.py -A 30Repository: wecode-ai/Wegent
Length of output: 1460
🏁 Script executed:
rg -n "def stream_chat" backend/app/api/endpoints/adapter/chat.py -A 80 | head -120Repository: wecode-ai/Wegent
Length of output: 3542
🏁 Script executed:
rg -n "_should_use_direct_chat" backend/app/api/endpoints/adapter/chat.py -B 5 -A 20Repository: wecode-ai/Wegent
Length of output: 4369
🏁 Script executed:
rg -n "team.user_id\|Kind.user_id" backend/app/api/endpoints/adapter/chat.py | head -20Repository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
rg -n "Kind.user_id == current_user.id" backend/app/api/endpoints/adapter/chat.py -B 5 -A 5Repository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
rg -n "@router\." backend/app/api/endpoints/adapter/chat.py | head -10Repository: wecode-ai/Wegent
Length of output: 163
🏁 Script executed:
fd "team" backend/app/models -type f -name "*.py" | head -5Repository: wecode-ai/Wegent
Length of output: 230
🏁 Script executed:
rg -n "Kind.user_id ==" backend/app/api/endpoints/ -l | head -5Repository: wecode-ai/Wegent
Length of output: 189
🏁 Script executed:
rg -n "Kind.user_id == current_user.id" backend/app/api/endpoints/adapter/ -B 3 -A 3Repository: wecode-ai/Wegent
Length of output: 3413
🏁 Script executed:
rg -n "team" backend/app/api/endpoints/adapter/shells.py -B 2 -A 2 | head -40Repository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
rg -n "def.*get.*team\|def.*Team" backend/app/api/endpoints/ -i -B 2 -A 10 | head -60Repository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
rg -n "team_id\|Team" backend/app/api/endpoints/adapter/chat.py | grep -E "query|filter" | head -20Repository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
rg -n "Kind.kind == \"Team\"" backend/app/api/endpoints/adapter/ -B 3 -A 3Repository: wecode-ai/Wegent
Length of output: 1050
Add Kind.user_id == current_user.id filter to both /stream and check_direct_chat endpoints
Both stream_chat (line 379-386) and check_direct_chat (line 583-589) query teams by id, kind, and is_active only, without scoping to the current user. This allows any authenticated user to discover whether other users' teams exist, support direct chat, and learn their shell types. Align with the established pattern in shells.py and models.py by adding Kind.user_id == current_user.id to the team query filters in both endpoints.
🧰 Tools
🪛 Ruff (0.14.7)
573-573: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
574-574: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
587-587: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
609-609: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
🤖 Prompt for AI Agents
In backend/app/api/endpoints/adapter/chat.py around lines 570 to 619 (and also
update the similar team lookup at lines ~379-386 for the /stream endpoint), the
team queries only filter by id, kind and is_active which allows any user to
probe other users' teams; add Kind.user_id == current_user.id to the filter list
for both the check_direct_chat and stream_chat team queries so the lookup is
scoped to the authenticated user, leaving the rest of the logic unchanged.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.