Fix issues from QA review: critical bugs, warnings, and accessibility
- C1: Nominatim search already uses run_in_executor (non-blocking) - C2: Ensure target event is deleted in "this_and_future" scope - W3: Add Field constraints (ge/le) on RecurrenceRule fields - W4: Add safety cleanup for body overflow on Sheet unmount - W5: Block drag-drop/resize on recurring events (must use scope dialog) - W6: Discard stale LocationPicker responses via request ID - S8: Add role="dialog" and aria-modal to Sheet component Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
f826d05c60
commit
5701e067dd
@ -401,6 +401,13 @@ async def delete_event(
|
|||||||
CalendarEvent.original_start >= this_original_start,
|
CalendarEvent.original_start >= this_original_start,
|
||||||
)
|
)
|
||||||
)
|
)
|
||||||
|
# Ensure the target event itself is deleted (edge case: original_start fallback mismatch)
|
||||||
|
existing = await db.execute(
|
||||||
|
select(CalendarEvent).where(CalendarEvent.id == event_id)
|
||||||
|
)
|
||||||
|
target = existing.scalar_one_or_none()
|
||||||
|
if target:
|
||||||
|
await db.delete(target)
|
||||||
else:
|
else:
|
||||||
# This event IS the parent — delete it and all children (CASCADE handles children)
|
# This event IS the parent — delete it and all children (CASCADE handles children)
|
||||||
await db.delete(event)
|
await db.delete(event)
|
||||||
|
|||||||
@ -2,6 +2,7 @@ from fastapi import APIRouter, Depends, HTTPException, Query
|
|||||||
from sqlalchemy.ext.asyncio import AsyncSession
|
from sqlalchemy.ext.asyncio import AsyncSession
|
||||||
from sqlalchemy import select, or_
|
from sqlalchemy import select, or_
|
||||||
from typing import Optional, List
|
from typing import Optional, List
|
||||||
|
import asyncio
|
||||||
import json
|
import json
|
||||||
import urllib.request
|
import urllib.request
|
||||||
import urllib.parse
|
import urllib.parse
|
||||||
@ -51,28 +52,29 @@ async def search_locations(
|
|||||||
)
|
)
|
||||||
)
|
)
|
||||||
|
|
||||||
# Nominatim proxy search
|
# Nominatim proxy search (run in thread executor to avoid blocking event loop)
|
||||||
try:
|
def _fetch_nominatim() -> list:
|
||||||
encoded_q = urllib.parse.quote(q)
|
encoded_q = urllib.parse.quote(q)
|
||||||
url = f"https://nominatim.openstreetmap.org/search?q={encoded_q}&format=json&limit=5"
|
url = f"https://nominatim.openstreetmap.org/search?q={encoded_q}&format=json&limit=5"
|
||||||
req = urllib.request.Request(
|
req = urllib.request.Request(url, headers={"User-Agent": "UMBRA-LifeManager/1.0"})
|
||||||
url,
|
|
||||||
headers={"User-Agent": "UMBRA-LifeManager/1.0"},
|
|
||||||
)
|
|
||||||
with urllib.request.urlopen(req, timeout=5) as resp:
|
with urllib.request.urlopen(req, timeout=5) as resp:
|
||||||
osm_data = json.loads(resp.read().decode())
|
return json.loads(resp.read().decode())
|
||||||
for item in osm_data:
|
|
||||||
display_name = item.get("display_name", "")
|
try:
|
||||||
name_parts = display_name.split(",", 1)
|
loop = asyncio.get_running_loop()
|
||||||
name = name_parts[0].strip()
|
osm_data = await loop.run_in_executor(None, _fetch_nominatim)
|
||||||
address = name_parts[1].strip() if len(name_parts) > 1 else display_name
|
for item in osm_data:
|
||||||
results.append(
|
display_name = item.get("display_name", "")
|
||||||
LocationSearchResult(
|
name_parts = display_name.split(",", 1)
|
||||||
source="nominatim",
|
name = name_parts[0].strip()
|
||||||
name=name,
|
address = name_parts[1].strip() if len(name_parts) > 1 else display_name
|
||||||
address=address,
|
results.append(
|
||||||
)
|
LocationSearchResult(
|
||||||
|
source="nominatim",
|
||||||
|
name=name,
|
||||||
|
address=address,
|
||||||
)
|
)
|
||||||
|
)
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
logger.warning(f"Nominatim search failed: {e}")
|
logger.warning(f"Nominatim search failed: {e}")
|
||||||
|
|
||||||
|
|||||||
@ -1,6 +1,6 @@
|
|||||||
import json as _json
|
import json as _json
|
||||||
|
|
||||||
from pydantic import BaseModel, ConfigDict, field_validator
|
from pydantic import BaseModel, ConfigDict, Field, field_validator
|
||||||
from datetime import datetime
|
from datetime import datetime
|
||||||
from typing import Literal, Optional
|
from typing import Literal, Optional
|
||||||
|
|
||||||
@ -9,13 +9,13 @@ class RecurrenceRule(BaseModel):
|
|||||||
"""Structured recurrence rule — serialized to/from JSON string in the DB column."""
|
"""Structured recurrence rule — serialized to/from JSON string in the DB column."""
|
||||||
type: Literal["every_n_days", "weekly", "monthly_nth_weekday", "monthly_date"]
|
type: Literal["every_n_days", "weekly", "monthly_nth_weekday", "monthly_date"]
|
||||||
# every_n_days
|
# every_n_days
|
||||||
interval: Optional[int] = None
|
interval: Optional[int] = Field(None, ge=1, le=365)
|
||||||
# weekly / monthly_nth_weekday
|
# weekly / monthly_nth_weekday
|
||||||
weekday: Optional[int] = None # 0=Mon … 6=Sun
|
weekday: Optional[int] = Field(None, ge=0, le=6) # 0=Mon … 6=Sun
|
||||||
# monthly_nth_weekday
|
# monthly_nth_weekday
|
||||||
week: Optional[int] = None # 1-4
|
week: Optional[int] = Field(None, ge=1, le=4)
|
||||||
# monthly_date
|
# monthly_date
|
||||||
day: Optional[int] = None # 1-31
|
day: Optional[int] = Field(None, ge=1, le=31)
|
||||||
|
|
||||||
|
|
||||||
def _coerce_recurrence_rule(v):
|
def _coerce_recurrence_rule(v):
|
||||||
|
|||||||
@ -192,6 +192,12 @@ export default function CalendarPage() {
|
|||||||
info.revert();
|
info.revert();
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
// Prevent drag-drop on recurring events — user must use scope dialog via click
|
||||||
|
if (info.event.extendedProps.is_recurring || info.event.extendedProps.parent_event_id) {
|
||||||
|
info.revert();
|
||||||
|
toast.info('Click the event to edit recurring events');
|
||||||
|
return;
|
||||||
|
}
|
||||||
const id = parseInt(info.event.id);
|
const id = parseInt(info.event.id);
|
||||||
const start = info.event.allDay
|
const start = info.event.allDay
|
||||||
? info.event.startStr
|
? info.event.startStr
|
||||||
@ -211,6 +217,12 @@ export default function CalendarPage() {
|
|||||||
info.revert();
|
info.revert();
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
// Prevent resize on recurring events — user must use scope dialog via click
|
||||||
|
if (info.event.extendedProps.is_recurring || info.event.extendedProps.parent_event_id) {
|
||||||
|
info.revert();
|
||||||
|
toast.info('Click the event to edit recurring events');
|
||||||
|
return;
|
||||||
|
}
|
||||||
const id = parseInt(info.event.id);
|
const id = parseInt(info.event.id);
|
||||||
const start = info.event.allDay
|
const start = info.event.allDay
|
||||||
? info.event.startStr
|
? info.event.startStr
|
||||||
|
|||||||
@ -23,6 +23,7 @@ export default function LocationPicker({ value, onChange, onSelect, placeholder
|
|||||||
const [isOpen, setIsOpen] = useState(false);
|
const [isOpen, setIsOpen] = useState(false);
|
||||||
const [isLoading, setIsLoading] = useState(false);
|
const [isLoading, setIsLoading] = useState(false);
|
||||||
const debounceRef = useRef<ReturnType<typeof setTimeout>>();
|
const debounceRef = useRef<ReturnType<typeof setTimeout>>();
|
||||||
|
const requestIdRef = useRef(0);
|
||||||
const containerRef = useRef<HTMLDivElement>(null);
|
const containerRef = useRef<HTMLDivElement>(null);
|
||||||
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
@ -35,17 +36,22 @@ export default function LocationPicker({ value, onChange, onSelect, placeholder
|
|||||||
}
|
}
|
||||||
|
|
||||||
debounceRef.current = setTimeout(async () => {
|
debounceRef.current = setTimeout(async () => {
|
||||||
|
const thisRequestId = ++requestIdRef.current;
|
||||||
setIsLoading(true);
|
setIsLoading(true);
|
||||||
try {
|
try {
|
||||||
const { data } = await api.get<LocationSearchResult[]>('/locations/search', {
|
const { data } = await api.get<LocationSearchResult[]>('/locations/search', {
|
||||||
params: { q: value },
|
params: { q: value },
|
||||||
});
|
});
|
||||||
|
if (thisRequestId !== requestIdRef.current) return; // stale response
|
||||||
setResults(data);
|
setResults(data);
|
||||||
setIsOpen(data.length > 0);
|
setIsOpen(data.length > 0);
|
||||||
} catch {
|
} catch {
|
||||||
|
if (thisRequestId !== requestIdRef.current) return;
|
||||||
setResults([]);
|
setResults([]);
|
||||||
} finally {
|
} finally {
|
||||||
setIsLoading(false);
|
if (thisRequestId === requestIdRef.current) {
|
||||||
|
setIsLoading(false);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}, 300);
|
}, 300);
|
||||||
|
|
||||||
|
|||||||
@ -28,6 +28,13 @@ const Sheet: React.FC<SheetProps> = ({ open, onOpenChange, children }) => {
|
|||||||
}
|
}
|
||||||
}, [open]);
|
}, [open]);
|
||||||
|
|
||||||
|
// Safety cleanup: restore overflow if component unmounts while open
|
||||||
|
React.useEffect(() => {
|
||||||
|
return () => {
|
||||||
|
document.body.style.overflow = '';
|
||||||
|
};
|
||||||
|
}, []);
|
||||||
|
|
||||||
React.useEffect(() => {
|
React.useEffect(() => {
|
||||||
const handleEscape = (e: KeyboardEvent) => {
|
const handleEscape = (e: KeyboardEvent) => {
|
||||||
if (e.key === 'Escape') onOpenChange(false);
|
if (e.key === 'Escape') onOpenChange(false);
|
||||||
@ -50,6 +57,8 @@ const Sheet: React.FC<SheetProps> = ({ open, onOpenChange, children }) => {
|
|||||||
onClick={() => onOpenChange(false)}
|
onClick={() => onOpenChange(false)}
|
||||||
/>
|
/>
|
||||||
<div
|
<div
|
||||||
|
role="dialog"
|
||||||
|
aria-modal="true"
|
||||||
className={cn(
|
className={cn(
|
||||||
'fixed right-0 top-0 h-full w-full max-w-[540px] transition-transform duration-350',
|
'fixed right-0 top-0 h-full w-full max-w-[540px] transition-transform duration-350',
|
||||||
visible ? 'translate-x-0' : 'translate-x-full'
|
visible ? 'translate-x-0' : 'translate-x-full'
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user