Fix QA review findings: C-01, C-02, W-01, W-02, W-04, S-01, S-02, S-03

C-01: Remove nginx rate limit on event invitations endpoint — was
      blocking GET (invitee list) on rapid event switching. Backend
      already caps at 20 invitations per event with connection validation.

C-02: respondingRef uses string prefixes (conn-, cal-, event-) instead
      of fragile numeric offsets (+100000/+200000) to prevent collisions.

W-01: get_accessible_event_scope combined into single UNION ALL query
      (3 DB round-trips → 1) for calendar IDs + invitation IDs.

W-02: Dashboard and upcoming endpoints now include is_invited,
      invitation_status, and display_calendar_id on event items.

W-04: LeaveEventDialog closes on error (.finally) instead of staying
      open when mutation rejects.

S-01: Migration 055 FK constraint gets explicit name for consistency.

S-02: InviteSearch dropdown dismisses on blur (150ms delay for clicks).

S-03: Display calendar picker shows only owned calendars, not shared.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Kyle 2026-03-16 20:27:01 +08:00
parent 25830bb99e
commit f54ab5079e
7 changed files with 90 additions and 30 deletions

View File

@ -22,7 +22,7 @@ def upgrade() -> None:
sa.Column( sa.Column(
"display_calendar_id", "display_calendar_id",
sa.Integer(), sa.Integer(),
sa.ForeignKey("calendars.id", ondelete="SET NULL"), sa.ForeignKey("calendars.id", ondelete="SET NULL", name="fk_event_invitations_display_calendar_id"),
nullable=True, nullable=True,
), ),
) )

View File

@ -12,6 +12,7 @@ from app.models.reminder import Reminder
from app.models.project import Project from app.models.project import Project
from app.models.user import User from app.models.user import User
from app.routers.auth import get_current_user, get_current_settings from app.routers.auth import get_current_user, get_current_settings
from app.models.event_invitation import EventInvitation
from app.services.calendar_sharing import get_accessible_calendar_ids, get_accessible_event_scope from app.services.calendar_sharing import get_accessible_calendar_ids, get_accessible_event_scope
router = APIRouter() router = APIRouter()
@ -54,6 +55,22 @@ async def get_dashboard(
events_result = await db.execute(events_query) events_result = await db.execute(events_query)
todays_events = events_result.scalars().all() todays_events = events_result.scalars().all()
# Build invitation lookup for today's events
invited_event_id_set = set(invited_event_ids)
today_inv_map: dict[int, tuple[str, int | None]] = {}
today_event_ids = [e.id for e in todays_events]
parent_ids_in_today = [e.parent_event_id for e in todays_events if e.parent_event_id and e.parent_event_id in invited_event_id_set]
inv_lookup_ids = list(set(today_event_ids + parent_ids_in_today) & invited_event_id_set)
if inv_lookup_ids:
inv_result = await db.execute(
select(EventInvitation.event_id, EventInvitation.status, EventInvitation.display_calendar_id).where(
EventInvitation.user_id == current_user.id,
EventInvitation.event_id.in_(inv_lookup_ids),
)
)
for eid, status, disp_cal_id in inv_result.all():
today_inv_map[eid] = (status, disp_cal_id)
# Upcoming todos (not completed, with due date from today through upcoming_days) # Upcoming todos (not completed, with due date from today through upcoming_days)
todos_query = select(Todo).where( todos_query = select(Todo).where(
Todo.user_id == current_user.id, Todo.user_id == current_user.id,
@ -129,7 +146,10 @@ async def get_dashboard(
"end_datetime": event.end_datetime, "end_datetime": event.end_datetime,
"all_day": event.all_day, "all_day": event.all_day,
"color": event.color, "color": event.color,
"is_starred": event.is_starred "is_starred": event.is_starred,
"is_invited": (event.parent_event_id or event.id) in invited_event_id_set,
"invitation_status": today_inv_map.get(event.parent_event_id or event.id, (None,))[0],
"display_calendar_id": today_inv_map.get(event.parent_event_id or event.id, (None, None))[1],
} }
for event in todays_events for event in todays_events
], ],
@ -218,6 +238,20 @@ async def get_upcoming(
reminders_result = await db.execute(reminders_query) reminders_result = await db.execute(reminders_query)
reminders = reminders_result.scalars().all() reminders = reminders_result.scalars().all()
# Build invitation lookup for upcoming events
invited_event_id_set_up = set(invited_event_ids)
upcoming_inv_map: dict[int, tuple[str, int | None]] = {}
up_parent_ids = list({e.parent_event_id or e.id for e in events} & invited_event_id_set_up)
if up_parent_ids:
up_inv_result = await db.execute(
select(EventInvitation.event_id, EventInvitation.status, EventInvitation.display_calendar_id).where(
EventInvitation.user_id == current_user.id,
EventInvitation.event_id.in_(up_parent_ids),
)
)
for eid, status, disp_cal_id in up_inv_result.all():
upcoming_inv_map[eid] = (status, disp_cal_id)
# Combine into unified list # Combine into unified list
upcoming_items: List[Dict[str, Any]] = [] upcoming_items: List[Dict[str, Any]] = []
@ -235,6 +269,8 @@ async def get_upcoming(
for event in events: for event in events:
end_dt = event.end_datetime end_dt = event.end_datetime
parent_id = event.parent_event_id or event.id
is_inv = parent_id in invited_event_id_set_up
upcoming_items.append({ upcoming_items.append({
"type": "event", "type": "event",
"id": event.id, "id": event.id,
@ -245,6 +281,9 @@ async def get_upcoming(
"all_day": event.all_day, "all_day": event.all_day,
"color": event.color, "color": event.color,
"is_starred": event.is_starred, "is_starred": event.is_starred,
"is_invited": is_inv,
"invitation_status": upcoming_inv_map.get(parent_id, (None,))[0] if is_inv else None,
"display_calendar_id": upcoming_inv_map.get(parent_id, (None, None))[1] if is_inv else None,
}) })
for reminder in reminders: for reminder in reminders:

View File

@ -7,7 +7,7 @@ import logging
from datetime import datetime, timedelta from datetime import datetime, timedelta
from fastapi import HTTPException from fastapi import HTTPException
from sqlalchemy import delete, select, text, update from sqlalchemy import delete, literal_column, select, text, update
from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.ext.asyncio import AsyncSession
from app.models.calendar import Calendar from app.models.calendar import Calendar
@ -38,15 +38,38 @@ async def get_accessible_event_scope(
user_id: int, db: AsyncSession user_id: int, db: AsyncSession
) -> tuple[list[int], list[int]]: ) -> tuple[list[int], list[int]]:
""" """
Returns (calendar_ids, invited_parent_event_ids). Returns (calendar_ids, invited_parent_event_ids) in a single DB round-trip.
calendar_ids: all calendars the user can access (owned + accepted shared). calendar_ids: all calendars the user can access (owned + accepted shared).
invited_parent_event_ids: event IDs where the user has a non-declined invitation. invited_parent_event_ids: event IDs where the user has a non-declined invitation.
""" """
from app.services.event_invitation import get_invited_event_ids from app.models.event_invitation import EventInvitation
cal_ids = await get_accessible_calendar_ids(user_id, db) result = await db.execute(
invited_event_ids = await get_invited_event_ids(db, user_id) select(literal_column("'c'").label("kind"), Calendar.id.label("val"))
return cal_ids, invited_event_ids .where(Calendar.user_id == user_id)
.union_all(
select(literal_column("'c'"), CalendarMember.calendar_id)
.where(
CalendarMember.user_id == user_id,
CalendarMember.status == "accepted",
)
)
.union_all(
select(literal_column("'i'"), EventInvitation.event_id)
.where(
EventInvitation.user_id == user_id,
EventInvitation.status != "declined",
)
)
)
cal_ids: list[int] = []
inv_ids: list[int] = []
for kind, val in result.all():
if kind == "c":
cal_ids.append(val)
else:
inv_ids.append(val)
return cal_ids, inv_ids
async def get_user_permission(db: AsyncSession, calendar_id: int, user_id: int) -> str | None: async def get_user_permission(db: AsyncSession, calendar_id: int, user_id: int) -> str | None:

View File

@ -111,13 +111,6 @@ server {
include /etc/nginx/proxy-params.conf; include /etc/nginx/proxy-params.conf;
} }
# Event invite rate-limited to prevent invite spam (reuse cal_invite_limit zone)
location ~ /api/events/\d+/invitations$ {
limit_req zone=cal_invite_limit burst=3 nodelay;
limit_req_status 429;
include /etc/nginx/proxy-params.conf;
}
# Calendar sync rate-limited to prevent excessive polling # Calendar sync rate-limited to prevent excessive polling
location /api/shared-calendars/sync { location /api/shared-calendars/sync {
limit_req zone=cal_sync_limit burst=5 nodelay; limit_req zone=cal_sync_limit burst=5 nodelay;

View File

@ -237,6 +237,7 @@ export default function EventDetailPanel({
.filter((m) => m.permission === 'create_modify' || m.permission === 'full_access') .filter((m) => m.permission === 'create_modify' || m.permission === 'full_access')
.map((m) => ({ id: m.calendar_id, name: m.calendar_name, color: m.local_color || m.calendar_color, is_default: false })), .map((m) => ({ id: m.calendar_id, name: m.calendar_name, color: m.local_color || m.calendar_color, is_default: false })),
]; ];
const ownedCalendars = calendars.filter((c) => !c.is_system);
const defaultCalendar = calendars.find((c) => c.is_default); const defaultCalendar = calendars.find((c) => c.is_default);
const { data: locations = [] } = useQuery({ const { data: locations = [] } = useQuery({
@ -957,7 +958,7 @@ export default function EventDetailPanel({
{!event?.display_calendar_id && ( {!event?.display_calendar_id && (
<option value="" disabled>Assign to calendar...</option> <option value="" disabled>Assign to calendar...</option>
)} )}
{selectableCalendars.map((cal) => ( {ownedCalendars.map((cal) => (
<option key={cal.id} value={cal.id}>{cal.name}</option> <option key={cal.id} value={cal.id}>{cal.name}</option>
))} ))}
</Select> </Select>
@ -1106,10 +1107,10 @@ export default function EventDetailPanel({
open={showLeaveDialog} open={showLeaveDialog}
onClose={() => setShowLeaveDialog(false)} onClose={() => setShowLeaveDialog(false)}
onConfirm={() => { onConfirm={() => {
leaveInvitation(myInvitationId).then(() => { leaveInvitation(myInvitationId)
setShowLeaveDialog(false); .then(() => onClose())
onClose(); .catch(() => {})
}); .finally(() => setShowLeaveDialog(false));
}} }}
eventTitle={event.title} eventTitle={event.title}
isRecurring={!!(event.is_recurring || event.parent_event_id)} isRecurring={!!(event.is_recurring || event.parent_event_id)}

View File

@ -131,6 +131,7 @@ export function InviteSearch({ connections, existingInviteeIds, onInvite, isInvi
<Input <Input
value={search} value={search}
onChange={(e) => setSearch(e.target.value)} onChange={(e) => setSearch(e.target.value)}
onBlur={() => setTimeout(() => setSearch(''), 150)}
placeholder="Search connections..." placeholder="Search connections..."
className="h-8 pl-8 text-xs" className="h-8 pl-8 text-xs"
/> />

View File

@ -18,7 +18,7 @@ export default function NotificationToaster() {
const initializedRef = useRef(false); const initializedRef = useRef(false);
const prevUnreadRef = useRef(0); const prevUnreadRef = useRef(0);
// Track in-flight request IDs so repeated clicks are blocked // Track in-flight request IDs so repeated clicks are blocked
const respondingRef = useRef<Set<number>>(new Set()); const respondingRef = useRef<Set<string>>(new Set());
// Always call the latest respond — Sonner toasts capture closures at creation time // Always call the latest respond — Sonner toasts capture closures at creation time
const respondInviteRef = useRef(respondInvite); const respondInviteRef = useRef(respondInvite);
const respondRef = useRef(respond); const respondRef = useRef(respond);
@ -30,8 +30,9 @@ export default function NotificationToaster() {
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) => {
// Guard against double-clicks (Sonner toasts are static, no disabled prop) // Guard against double-clicks (Sonner toasts are static, no disabled prop)
if (respondingRef.current.has(requestId)) return; const key = `conn-${requestId}`;
respondingRef.current.add(requestId); if (respondingRef.current.has(key)) return;
respondingRef.current.add(key);
// Immediately dismiss the custom toast and show a loading indicator // Immediately dismiss the custom toast and show a loading indicator
toast.dismiss(toastId); toast.dismiss(toastId);
@ -54,7 +55,7 @@ export default function NotificationToaster() {
toast.error(getErrorMessage(err, 'Failed to respond to request')); toast.error(getErrorMessage(err, 'Failed to respond to request'));
} }
} finally { } finally {
respondingRef.current.delete(requestId); respondingRef.current.delete(key);
} }
}, },
[], [],
@ -63,8 +64,9 @@ export default function NotificationToaster() {
const handleCalendarInviteRespond = useCallback( const handleCalendarInviteRespond = useCallback(
async (inviteId: number, action: 'accept' | 'reject', toastId: string | number, notificationId: number) => { async (inviteId: number, action: 'accept' | 'reject', toastId: string | number, notificationId: number) => {
if (respondingRef.current.has(inviteId + 100000)) return; const key = `cal-${inviteId}`;
respondingRef.current.add(inviteId + 100000); if (respondingRef.current.has(key)) return;
respondingRef.current.add(key);
toast.dismiss(toastId); toast.dismiss(toastId);
const loadingId = toast.loading( const loadingId = toast.loading(
@ -83,15 +85,16 @@ export default function NotificationToaster() {
toast.error(getErrorMessage(err, 'Failed to respond to invite')); toast.error(getErrorMessage(err, 'Failed to respond to invite'));
} }
} finally { } finally {
respondingRef.current.delete(inviteId + 100000); respondingRef.current.delete(key);
} }
}, },
[], [],
); );
const handleEventInviteRespond = useCallback( const handleEventInviteRespond = useCallback(
async (invitationId: number, status: 'accepted' | 'tentative' | 'declined', toastId: string | number, notificationId: number) => { async (invitationId: number, status: 'accepted' | 'tentative' | 'declined', toastId: string | number, notificationId: number) => {
if (respondingRef.current.has(invitationId + 200000)) return; const key = `event-${invitationId}`;
respondingRef.current.add(invitationId + 200000); if (respondingRef.current.has(key)) return;
respondingRef.current.add(key);
toast.dismiss(toastId); toast.dismiss(toastId);
const statusLabel = { accepted: 'Accepting', tentative: 'Setting tentative', declined: 'Declining' }; const statusLabel = { accepted: 'Accepting', tentative: 'Setting tentative', declined: 'Declining' };
@ -114,7 +117,7 @@ export default function NotificationToaster() {
toast.error(getErrorMessage(err, 'Failed to respond')); toast.error(getErrorMessage(err, 'Failed to respond'));
} }
} finally { } finally {
respondingRef.current.delete(invitationId + 200000); respondingRef.current.delete(key);
} }
}, },
[], [],