From 0a449f166c878c73ed27e0413a02ee9688e832e2 Mon Sep 17 00:00:00 2001 From: Kyle Pope Date: Tue, 17 Mar 2026 05:28:34 +0800 Subject: [PATCH] Polish pass: action all remaining QA suggestions before merge P-01: Clamp delta poll since param to max 24h in the past (projects + calendars) to prevent expensive full-table scans from malicious timestamps. P-02: Validate individual user_id elements in ProjectMemberInvite and TaskAssignmentCreate with Annotated[int, Field(ge=1, le=2147483647)]. P-04: Only enable delta polling for shared projects (member_count > 0). Solo projects skip the 5s poll entirely. P-05: Remove fragile 200ms onBlur timeout in ProjectShareSheet search. The onMouseDown preventDefault on dropdown items already prevents blur from firing before click registers. P-06/S-04: Replace manual dict construction in model_validators with __table__.columns iteration so new fields are auto-included. S-01: Replace bare except in ProjectResponse.compute_member_count with logger.debug to surface errors in development. S-03: Consolidate cascade_projects_on_disconnect from 2 project ID queries into 1 using IN clause with both user IDs. S-05: Send version in toggleTaskMutation, updateTaskStatusMutation, and toggleSubtaskMutation for full optimistic locking coverage. Handle 409 with refresh toast. S-07: Replace window.location.href with React Router navigateRef in task_assigned toast for client-side navigation. S-08: Already fixed in previous commit (subtask comment selectinload). Co-Authored-By: Claude Opus 4.6 (1M context) --- backend/app/routers/calendars.py | 6 +++ backend/app/routers/projects.py | 5 +++ backend/app/schemas/project.py | 7 +++- backend/app/schemas/project_member.py | 4 +- .../app/schemas/project_task_assignment.py | 18 ++++----- backend/app/schemas/task_comment.py | 15 ++----- backend/app/services/project_sharing.py | 22 ++++++----- .../notifications/NotificationToaster.tsx | 6 ++- .../src/components/projects/ProjectDetail.tsx | 39 ++++++++++++------- .../components/projects/ProjectShareSheet.tsx | 2 +- .../components/projects/TaskDetailPanel.tsx | 5 ++- 11 files changed, 76 insertions(+), 53 deletions(-) diff --git a/backend/app/routers/calendars.py b/backend/app/routers/calendars.py index 0c15a47..624e0aa 100644 --- a/backend/app/routers/calendars.py +++ b/backend/app/routers/calendars.py @@ -167,6 +167,12 @@ async def poll_calendar( except ValueError: raise HTTPException(status_code=400, detail="Invalid ISO timestamp") + # Clamp to max 24h in the past to prevent expensive full-table scans + from datetime import timedelta + min_since = datetime.now() - timedelta(hours=24) + if since_dt < min_since: + since_dt = min_since + # Check calendar-level update cal_result = await db.execute( select(Calendar.updated_at).where(Calendar.id == calendar_id) diff --git a/backend/app/routers/projects.py b/backend/app/routers/projects.py index 32cd0e7..021c8db 100644 --- a/backend/app/routers/projects.py +++ b/backend/app/routers/projects.py @@ -971,6 +971,11 @@ async def poll_project( except ValueError: raise HTTPException(status_code=400, detail="Invalid ISO timestamp") + # Clamp to max 24h in the past to prevent expensive full-table scans + min_since = datetime.now() - timedelta(hours=24) + if since_dt < min_since: + since_dt = min_since + # Check project-level update proj_result = await db.execute( select(Project.updated_at).where(Project.id == project_id) diff --git a/backend/app/schemas/project.py b/backend/app/schemas/project.py index 8e03e36..53f697c 100644 --- a/backend/app/schemas/project.py +++ b/backend/app/schemas/project.py @@ -1,8 +1,11 @@ +import logging from pydantic import BaseModel, ConfigDict, Field, model_validator from datetime import datetime, date from typing import Optional, List, Literal from app.schemas.project_task import ProjectTaskResponse +logger = logging.getLogger(__name__) + ProjectStatus = Literal["not_started", "in_progress", "completed", "blocked", "review", "on_hold"] @@ -64,8 +67,8 @@ class ProjectResponse(BaseModel): updated_at=data.updated_at, tasks=data.tasks, ) - except Exception: - pass # If members aren't loaded, default to 0 + except Exception as exc: + logger.debug("member_count compute skipped: %s", exc) return data diff --git a/backend/app/schemas/project_member.py b/backend/app/schemas/project_member.py index 0057f91..350fcb3 100644 --- a/backend/app/schemas/project_member.py +++ b/backend/app/schemas/project_member.py @@ -1,6 +1,6 @@ +from typing import Annotated, Optional, Literal from pydantic import BaseModel, ConfigDict, Field from datetime import datetime -from typing import Optional, Literal MemberPermission = Literal["read_only", "create_modify"] MemberStatus = Literal["pending", "accepted", "rejected"] @@ -10,7 +10,7 @@ InviteResponse = Literal["accepted", "rejected"] class ProjectMemberInvite(BaseModel): model_config = ConfigDict(extra="forbid") - user_ids: list[int] = Field(min_length=1, max_length=10) + user_ids: list[Annotated[int, Field(ge=1, le=2147483647)]] = Field(min_length=1, max_length=10) permission: MemberPermission = "create_modify" diff --git a/backend/app/schemas/project_task_assignment.py b/backend/app/schemas/project_task_assignment.py index 27bbaff..a4b42d9 100644 --- a/backend/app/schemas/project_task_assignment.py +++ b/backend/app/schemas/project_task_assignment.py @@ -1,3 +1,4 @@ +from typing import Annotated from pydantic import BaseModel, ConfigDict, Field, model_validator from datetime import datetime @@ -5,7 +6,7 @@ from datetime import datetime class TaskAssignmentCreate(BaseModel): model_config = ConfigDict(extra="forbid") - user_ids: list[int] = Field(min_length=1, max_length=20) + user_ids: list[Annotated[int, Field(ge=1, le=2147483647)]] = Field(min_length=1, max_length=20) class TaskAssignmentResponse(BaseModel): @@ -22,14 +23,9 @@ class TaskAssignmentResponse(BaseModel): @classmethod def resolve_user_name(cls, data): # type: ignore[override] """Populate user_name from eagerly loaded user relationship.""" - if hasattr(data, "user") and data.user is not None: - if not getattr(data, "user_name", None): - data = dict( - id=data.id, - task_id=data.task_id, - user_id=data.user_id, - assigned_by=data.assigned_by, - user_name=data.user.username, - created_at=data.created_at, - ) + if hasattr(data, "user") and data.user is not None and not getattr(data, "user_name", None): + # Build dict from ORM columns so new fields are auto-included + cols = {c.key: getattr(data, c.key) for c in data.__table__.columns} + cols["user_name"] = data.user.username + return cols return data diff --git a/backend/app/schemas/task_comment.py b/backend/app/schemas/task_comment.py index 328ae55..ac27abc 100644 --- a/backend/app/schemas/task_comment.py +++ b/backend/app/schemas/task_comment.py @@ -22,15 +22,8 @@ class TaskCommentResponse(BaseModel): @classmethod def resolve_author_name(cls, data): # type: ignore[override] """Populate author_name from eagerly loaded user relationship.""" - if hasattr(data, "user") and data.user is not None: - if not getattr(data, "author_name", None): - # Use username as fallback — preferred_name is on Settings, not User - data = dict( - id=data.id, - task_id=data.task_id, - user_id=data.user_id, - author_name=data.user.username, - content=data.content, - created_at=data.created_at, - ) + if hasattr(data, "user") and data.user is not None and not getattr(data, "author_name", None): + cols = {c.key: getattr(data, c.key) for c in data.__table__.columns} + cols["author_name"] = data.user.username + return cols return data diff --git a/backend/app/services/project_sharing.py b/backend/app/services/project_sharing.py index 6f6360d..852e9ac 100644 --- a/backend/app/services/project_sharing.py +++ b/backend/app/services/project_sharing.py @@ -220,20 +220,22 @@ async def cascade_projects_on_disconnect( 2. Find all ProjectTaskAssignment rows for those memberships 3. Remove assignments, then remove memberships """ - # Find projects owned by each user - a_proj_result = await db.execute( - select(Project.id).where(Project.user_id == user_a_id) + # Single query: find projects owned by each user + result = await db.execute( + select(Project.id, Project.user_id).where( + Project.user_id.in_([user_a_id, user_b_id]) + ) ) - a_proj_ids = [r[0] for r in a_proj_result.all()] - - b_proj_result = await db.execute( - select(Project.id).where(Project.user_id == user_b_id) - ) - b_proj_ids = [r[0] for r in b_proj_result.all()] + a_proj_ids: list[int] = [] + b_proj_ids: list[int] = [] + for proj_id, owner_id in result.all(): + if owner_id == user_a_id: + a_proj_ids.append(proj_id) + else: + b_proj_ids.append(proj_id) # Remove user_b's assignments + memberships on user_a's projects if a_proj_ids: - # Delete task assignments first await db.execute( delete(ProjectTaskAssignment).where( ProjectTaskAssignment.user_id == user_b_id, diff --git a/frontend/src/components/notifications/NotificationToaster.tsx b/frontend/src/components/notifications/NotificationToaster.tsx index 2062ffd..f67a107 100644 --- a/frontend/src/components/notifications/NotificationToaster.tsx +++ b/frontend/src/components/notifications/NotificationToaster.tsx @@ -1,4 +1,5 @@ import { useEffect, useRef, useCallback } from 'react'; +import { useNavigate } from 'react-router-dom'; import { toast } from 'sonner'; import { Check, X, Bell, UserPlus, Calendar, Clock, FolderKanban, ClipboardList } from 'lucide-react'; import { useQueryClient } from '@tanstack/react-query'; @@ -26,6 +27,9 @@ export default function NotificationToaster() { respondRef.current = respond; const markReadRef = useRef(markRead); markReadRef.current = markRead; + const navigate = useNavigate(); + const navigateRef = useRef(navigate); + navigateRef.current = navigate; const handleConnectionRespond = useCallback( async (requestId: number, action: 'accept' | 'reject', toastId: string | number, notificationId: number) => { @@ -426,7 +430,7 @@ export default function NotificationToaster() { onClick={() => { toast.dismiss(id); markReadRef.current([notification.id]).catch(() => {}); - window.location.href = `/projects/${projectId}`; + navigateRef.current(`/projects/${projectId}`); }} className="flex items-center gap-1 px-3 py-1.5 text-xs font-medium rounded-md bg-accent text-accent-foreground hover:bg-accent/90 transition-colors" > diff --git a/frontend/src/components/projects/ProjectDetail.tsx b/frontend/src/components/projects/ProjectDetail.tsx index 235a182..6549aea 100644 --- a/frontend/src/components/projects/ProjectDetail.tsx +++ b/frontend/src/components/projects/ProjectDetail.tsx @@ -25,6 +25,7 @@ import { Calendar, CheckCircle2, PlayCircle, AlertTriangle, List, Columns3, ArrowUpDown, Users, Eye, } from 'lucide-react'; +import axios from 'axios'; import api from '@/lib/api'; import type { Project, ProjectTask, ProjectMember } from '@/types'; import { Button } from '@/components/ui/button'; @@ -148,10 +149,10 @@ export default function ProjectDetail() { const canEdit = isOwner; // Members with create_modify can also edit tasks (handled per-task) const canManageProject = isOwner; - // Delta polling for real-time sync on shared projects + // Delta polling for real-time sync — only for shared projects (P-04) const pollKey = useMemo(() => ['projects', id], [id]); useDeltaPoll( - id ? `/projects/${id}/poll` : null, + id && project && project.member_count > 0 ? `/projects/${id}/poll` : null, pollKey, 5000, ); @@ -173,16 +174,21 @@ export default function ProjectDetail() { const canEditTasks = isOwner || myPermission === 'create_modify'; const toggleTaskMutation = useMutation({ - mutationFn: async ({ taskId, status }: { taskId: number; status: string }) => { + mutationFn: async ({ taskId, status, version }: { taskId: number; status: string; version: number }) => { const newStatus = status === 'completed' ? 'pending' : 'completed'; - const { data } = await api.put(`/projects/${id}/tasks/${taskId}`, { status: newStatus }); + const { data } = await api.put(`/projects/${id}/tasks/${taskId}`, { status: newStatus, version }); return data; }, onSuccess: () => { queryClient.invalidateQueries({ queryKey: ['projects', id] }); }, - onError: () => { - toast.error('Failed to update task'); + onError: (error) => { + if (axios.isAxiosError(error) && error.response?.status === 409) { + toast.error('Task was modified by another user — please refresh'); + queryClient.invalidateQueries({ queryKey: ['projects', id] }); + } else { + toast.error('Failed to update task'); + } }, }); @@ -237,15 +243,20 @@ export default function ProjectDetail() { }); const updateTaskStatusMutation = useMutation({ - mutationFn: async ({ taskId, status }: { taskId: number; status: string }) => { - const { data } = await api.put(`/projects/${id}/tasks/${taskId}`, { status }); + mutationFn: async ({ taskId, status, version }: { taskId: number; status: string; version: number }) => { + const { data } = await api.put(`/projects/${id}/tasks/${taskId}`, { status, version }); return data; }, onSuccess: () => { queryClient.invalidateQueries({ queryKey: ['projects', id] }); }, - onError: () => { - toast.error('Failed to update task status'); + onError: (error) => { + if (axios.isAxiosError(error) && error.response?.status === 409) { + toast.error('Task was modified by another user — please refresh'); + queryClient.invalidateQueries({ queryKey: ['projects', id] }); + } else { + toast.error('Failed to update task status'); + } }, }); @@ -635,9 +646,10 @@ export default function ProjectDetail() { selectedTaskId={selectedTaskId} kanbanParentTask={kanbanParentTask} onSelectTask={handleKanbanSelectTask} - onStatusChange={(taskId, status) => - updateTaskStatusMutation.mutate({ taskId, status }) - } + onStatusChange={(taskId, status) => { + const t = allTasks.find(tt => tt.id === taskId) ?? allTasks.flatMap(tt => tt.subtasks || []).find(st => st.id === taskId); + updateTaskStatusMutation.mutate({ taskId, status, version: t?.version ?? 1 }); + }} onBackToAllTasks={handleBackToAllTasks} /> ) : ( @@ -669,6 +681,7 @@ export default function ProjectDetail() { toggleTaskMutation.mutate({ taskId: task.id, status: task.status, + version: task.version, }) } togglePending={toggleTaskMutation.isPending} diff --git a/frontend/src/components/projects/ProjectShareSheet.tsx b/frontend/src/components/projects/ProjectShareSheet.tsx index 28aafb7..a161099 100644 --- a/frontend/src/components/projects/ProjectShareSheet.tsx +++ b/frontend/src/components/projects/ProjectShareSheet.tsx @@ -236,7 +236,7 @@ export function ProjectShareSheet({ open, onOpenChange, projectId, isOwner, owne setSearch(e.target.value)} - onBlur={() => setTimeout(() => setSearch(''), 200)} + onBlur={() => setSearch('')} placeholder="Search connections..." className="h-8 pl-8 text-xs" /> diff --git a/frontend/src/components/projects/TaskDetailPanel.tsx b/frontend/src/components/projects/TaskDetailPanel.tsx index 535b40e..d8e26f5 100644 --- a/frontend/src/components/projects/TaskDetailPanel.tsx +++ b/frontend/src/components/projects/TaskDetailPanel.tsx @@ -118,9 +118,9 @@ export default function TaskDetailPanel({ // --- Mutations --- const toggleSubtaskMutation = useMutation({ - mutationFn: async ({ taskId, status }: { taskId: number; status: string }) => { + mutationFn: async ({ taskId, status, version }: { taskId: number; status: string; version: number }) => { const newStatus = status === 'completed' ? 'pending' : 'completed'; - const { data } = await api.put(`/projects/${projectId}/tasks/${taskId}`, { status: newStatus }); + const { data } = await api.put(`/projects/${projectId}/tasks/${taskId}`, { status: newStatus, version }); return data; }, onSuccess: () => { @@ -514,6 +514,7 @@ export default function TaskDetailPanel({ toggleSubtaskMutation.mutate({ taskId: subtask.id, status: subtask.status, + version: subtask.version, }) } disabled={toggleSubtaskMutation.isPending}