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) <noreply@anthropic.com>
This commit is contained in:
Kyle 2026-03-17 05:28:34 +08:00
parent dd637bdc84
commit 0a449f166c
11 changed files with 76 additions and 53 deletions

View File

@ -167,6 +167,12 @@ async def poll_calendar(
except ValueError: except ValueError:
raise HTTPException(status_code=400, detail="Invalid ISO timestamp") 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 # Check calendar-level update
cal_result = await db.execute( cal_result = await db.execute(
select(Calendar.updated_at).where(Calendar.id == calendar_id) select(Calendar.updated_at).where(Calendar.id == calendar_id)

View File

@ -971,6 +971,11 @@ async def poll_project(
except ValueError: except ValueError:
raise HTTPException(status_code=400, detail="Invalid ISO timestamp") 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 # Check project-level update
proj_result = await db.execute( proj_result = await db.execute(
select(Project.updated_at).where(Project.id == project_id) select(Project.updated_at).where(Project.id == project_id)

View File

@ -1,8 +1,11 @@
import logging
from pydantic import BaseModel, ConfigDict, Field, model_validator from pydantic import BaseModel, ConfigDict, Field, model_validator
from datetime import datetime, date from datetime import datetime, date
from typing import Optional, List, Literal from typing import Optional, List, Literal
from app.schemas.project_task import ProjectTaskResponse from app.schemas.project_task import ProjectTaskResponse
logger = logging.getLogger(__name__)
ProjectStatus = Literal["not_started", "in_progress", "completed", "blocked", "review", "on_hold"] ProjectStatus = Literal["not_started", "in_progress", "completed", "blocked", "review", "on_hold"]
@ -64,8 +67,8 @@ class ProjectResponse(BaseModel):
updated_at=data.updated_at, updated_at=data.updated_at,
tasks=data.tasks, tasks=data.tasks,
) )
except Exception: except Exception as exc:
pass # If members aren't loaded, default to 0 logger.debug("member_count compute skipped: %s", exc)
return data return data

View File

@ -1,6 +1,6 @@
from typing import Annotated, Optional, Literal
from pydantic import BaseModel, ConfigDict, Field from pydantic import BaseModel, ConfigDict, Field
from datetime import datetime from datetime import datetime
from typing import Optional, Literal
MemberPermission = Literal["read_only", "create_modify"] MemberPermission = Literal["read_only", "create_modify"]
MemberStatus = Literal["pending", "accepted", "rejected"] MemberStatus = Literal["pending", "accepted", "rejected"]
@ -10,7 +10,7 @@ InviteResponse = Literal["accepted", "rejected"]
class ProjectMemberInvite(BaseModel): class ProjectMemberInvite(BaseModel):
model_config = ConfigDict(extra="forbid") 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" permission: MemberPermission = "create_modify"

View File

@ -1,3 +1,4 @@
from typing import Annotated
from pydantic import BaseModel, ConfigDict, Field, model_validator from pydantic import BaseModel, ConfigDict, Field, model_validator
from datetime import datetime from datetime import datetime
@ -5,7 +6,7 @@ from datetime import datetime
class TaskAssignmentCreate(BaseModel): class TaskAssignmentCreate(BaseModel):
model_config = ConfigDict(extra="forbid") 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): class TaskAssignmentResponse(BaseModel):
@ -22,14 +23,9 @@ class TaskAssignmentResponse(BaseModel):
@classmethod @classmethod
def resolve_user_name(cls, data): # type: ignore[override] def resolve_user_name(cls, data): # type: ignore[override]
"""Populate user_name from eagerly loaded user relationship.""" """Populate user_name from eagerly loaded user relationship."""
if hasattr(data, "user") and data.user is not None: if hasattr(data, "user") and data.user is not None and not getattr(data, "user_name", None):
if not getattr(data, "user_name", None): # Build dict from ORM columns so new fields are auto-included
data = dict( cols = {c.key: getattr(data, c.key) for c in data.__table__.columns}
id=data.id, cols["user_name"] = data.user.username
task_id=data.task_id, return cols
user_id=data.user_id,
assigned_by=data.assigned_by,
user_name=data.user.username,
created_at=data.created_at,
)
return data return data

View File

@ -22,15 +22,8 @@ class TaskCommentResponse(BaseModel):
@classmethod @classmethod
def resolve_author_name(cls, data): # type: ignore[override] def resolve_author_name(cls, data): # type: ignore[override]
"""Populate author_name from eagerly loaded user relationship.""" """Populate author_name from eagerly loaded user relationship."""
if hasattr(data, "user") and data.user is not None: if hasattr(data, "user") and data.user is not None and not getattr(data, "author_name", None):
if not getattr(data, "author_name", None): cols = {c.key: getattr(data, c.key) for c in data.__table__.columns}
# Use username as fallback — preferred_name is on Settings, not User cols["author_name"] = data.user.username
data = dict( return cols
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,
)
return data return data

View File

@ -220,20 +220,22 @@ async def cascade_projects_on_disconnect(
2. Find all ProjectTaskAssignment rows for those memberships 2. Find all ProjectTaskAssignment rows for those memberships
3. Remove assignments, then remove memberships 3. Remove assignments, then remove memberships
""" """
# Find projects owned by each user # Single query: find projects owned by each user
a_proj_result = await db.execute( result = await db.execute(
select(Project.id).where(Project.user_id == user_a_id) 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()] a_proj_ids: list[int] = []
b_proj_ids: list[int] = []
b_proj_result = await db.execute( for proj_id, owner_id in result.all():
select(Project.id).where(Project.user_id == user_b_id) if owner_id == user_a_id:
) a_proj_ids.append(proj_id)
b_proj_ids = [r[0] for r in b_proj_result.all()] else:
b_proj_ids.append(proj_id)
# Remove user_b's assignments + memberships on user_a's projects # Remove user_b's assignments + memberships on user_a's projects
if a_proj_ids: if a_proj_ids:
# Delete task assignments first
await db.execute( await db.execute(
delete(ProjectTaskAssignment).where( delete(ProjectTaskAssignment).where(
ProjectTaskAssignment.user_id == user_b_id, ProjectTaskAssignment.user_id == user_b_id,

View File

@ -1,4 +1,5 @@
import { useEffect, useRef, useCallback } from 'react'; import { useEffect, useRef, useCallback } from 'react';
import { useNavigate } from 'react-router-dom';
import { toast } from 'sonner'; import { toast } from 'sonner';
import { Check, X, Bell, UserPlus, Calendar, Clock, FolderKanban, ClipboardList } from 'lucide-react'; import { Check, X, Bell, UserPlus, Calendar, Clock, FolderKanban, ClipboardList } from 'lucide-react';
import { useQueryClient } from '@tanstack/react-query'; import { useQueryClient } from '@tanstack/react-query';
@ -26,6 +27,9 @@ export default function NotificationToaster() {
respondRef.current = respond; respondRef.current = respond;
const markReadRef = useRef(markRead); const markReadRef = useRef(markRead);
markReadRef.current = markRead; markReadRef.current = markRead;
const navigate = useNavigate();
const navigateRef = useRef(navigate);
navigateRef.current = navigate;
const handleConnectionRespond = useCallback( const handleConnectionRespond = useCallback(
async (requestId: number, action: 'accept' | 'reject', toastId: string | number, notificationId: number) => { async (requestId: number, action: 'accept' | 'reject', toastId: string | number, notificationId: number) => {
@ -426,7 +430,7 @@ export default function NotificationToaster() {
onClick={() => { onClick={() => {
toast.dismiss(id); toast.dismiss(id);
markReadRef.current([notification.id]).catch(() => {}); 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" 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"
> >

View File

@ -25,6 +25,7 @@ import {
Calendar, CheckCircle2, PlayCircle, AlertTriangle, Calendar, CheckCircle2, PlayCircle, AlertTriangle,
List, Columns3, ArrowUpDown, Users, Eye, List, Columns3, ArrowUpDown, Users, Eye,
} from 'lucide-react'; } from 'lucide-react';
import axios from 'axios';
import api from '@/lib/api'; import api from '@/lib/api';
import type { Project, ProjectTask, ProjectMember } from '@/types'; import type { Project, ProjectTask, ProjectMember } from '@/types';
import { Button } from '@/components/ui/button'; 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 canEdit = isOwner; // Members with create_modify can also edit tasks (handled per-task)
const canManageProject = isOwner; 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]); const pollKey = useMemo(() => ['projects', id], [id]);
useDeltaPoll( useDeltaPoll(
id ? `/projects/${id}/poll` : null, id && project && project.member_count > 0 ? `/projects/${id}/poll` : null,
pollKey, pollKey,
5000, 5000,
); );
@ -173,16 +174,21 @@ export default function ProjectDetail() {
const canEditTasks = isOwner || myPermission === 'create_modify'; const canEditTasks = isOwner || myPermission === 'create_modify';
const toggleTaskMutation = useMutation({ 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 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; return data;
}, },
onSuccess: () => { onSuccess: () => {
queryClient.invalidateQueries({ queryKey: ['projects', id] }); queryClient.invalidateQueries({ queryKey: ['projects', id] });
}, },
onError: () => { onError: (error) => {
toast.error('Failed to update task'); 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({ const updateTaskStatusMutation = useMutation({
mutationFn: async ({ taskId, status }: { taskId: number; status: string }) => { mutationFn: async ({ taskId, status, version }: { taskId: number; status: string; version: number }) => {
const { data } = await api.put(`/projects/${id}/tasks/${taskId}`, { status }); const { data } = await api.put(`/projects/${id}/tasks/${taskId}`, { status, version });
return data; return data;
}, },
onSuccess: () => { onSuccess: () => {
queryClient.invalidateQueries({ queryKey: ['projects', id] }); queryClient.invalidateQueries({ queryKey: ['projects', id] });
}, },
onError: () => { onError: (error) => {
toast.error('Failed to update task status'); 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} selectedTaskId={selectedTaskId}
kanbanParentTask={kanbanParentTask} kanbanParentTask={kanbanParentTask}
onSelectTask={handleKanbanSelectTask} onSelectTask={handleKanbanSelectTask}
onStatusChange={(taskId, status) => onStatusChange={(taskId, status) => {
updateTaskStatusMutation.mutate({ 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} onBackToAllTasks={handleBackToAllTasks}
/> />
) : ( ) : (
@ -669,6 +681,7 @@ export default function ProjectDetail() {
toggleTaskMutation.mutate({ toggleTaskMutation.mutate({
taskId: task.id, taskId: task.id,
status: task.status, status: task.status,
version: task.version,
}) })
} }
togglePending={toggleTaskMutation.isPending} togglePending={toggleTaskMutation.isPending}

View File

@ -236,7 +236,7 @@ export function ProjectShareSheet({ open, onOpenChange, projectId, isOwner, owne
<Input <Input
value={search} value={search}
onChange={(e) => setSearch(e.target.value)} onChange={(e) => setSearch(e.target.value)}
onBlur={() => setTimeout(() => setSearch(''), 200)} onBlur={() => setSearch('')}
placeholder="Search connections..." placeholder="Search connections..."
className="h-8 pl-8 text-xs" className="h-8 pl-8 text-xs"
/> />

View File

@ -118,9 +118,9 @@ export default function TaskDetailPanel({
// --- Mutations --- // --- Mutations ---
const toggleSubtaskMutation = useMutation({ 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 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; return data;
}, },
onSuccess: () => { onSuccess: () => {
@ -514,6 +514,7 @@ export default function TaskDetailPanel({
toggleSubtaskMutation.mutate({ toggleSubtaskMutation.mutate({
taskId: subtask.id, taskId: subtask.id,
status: subtask.status, status: subtask.status,
version: subtask.version,
}) })
} }
disabled={toggleSubtaskMutation.isPending} disabled={toggleSubtaskMutation.isPending}