From 2c46cf762dddfaf9ecc6e3d86a5965974953b83e Mon Sep 17 00:00:00 2001 From: Jacob Hagstedt Date: Mon, 21 Sep 2026 09:22:30 +0200 Subject: [PATCH] Stop two stale ids reaching the API, and steer the drag test off free days MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-merge verification of the six merged branches turned up two real frontend defects and one stale test expectation. Settings mounts before WorkspaceContext has populated, so PartnerInvitePanel asked for invitations with an empty workspace id. The empty path segment collapses in Vite's proxy, FastAPI matches the single-segment workspace route with workspace_id="invitations", and the UUID parse 422s. Guarded the same way useWorkspaceMembers already does. PlanSyncProvider could sync a workspace id seeded from localStorage before WorkspaceContext refreshed it — a workspace a reseed had since deleted. It logged the resulting 404 and kept retrying it on every planVersion bump. A 404 now refreshes the workspace context instead, correcting the id rather than narrating the failure forever. The drag-create-delete expectation was simply out of date: vacation-typed saves correctly exclude free days, and targetRange() only skipped months ending on a Sunday, so a Monday-ending month put a Sunday in the range. It now skips any free day, computed independently of the calendar data the backend itself reads. Co-Authored-By: Claude Opus 5 --- frontend/e2e/support/dates.ts | 60 +++++++++++++++++++ frontend/e2e/tests/drag-create-delete.spec.ts | 24 +++++--- .../settings/PartnerInvitePanel.test.tsx | 10 ++++ .../settings/PartnerInvitePanel.tsx | 9 +++ .../src/context/PlanSyncProvider.test.tsx | 21 +++++++ frontend/src/context/PlanSyncProvider.tsx | 39 ++++++++---- 6 files changed, 145 insertions(+), 18 deletions(-) diff --git a/frontend/e2e/support/dates.ts b/frontend/e2e/support/dates.ts index 84d8d6d..d0ca40a 100644 --- a/frontend/e2e/support/dates.ts +++ b/frontend/e2e/support/dates.ts @@ -60,6 +60,66 @@ export function whitSunday(easter: Date): Date { return addDays(easter, 49) } +/** Midsummer's Day (Midsommardagen) — the Saturday falling between June 20 + * and June 26 inclusive, per Swedish law. */ +export function midsummerDay(year: number): Date { + for (let day = 20; day <= 26; day++) { + const candidate = new Date(Date.UTC(year, 5, day)) // June = month 5 + if (candidate.getUTCDay() === 6) return candidate + } + /* istanbul ignore next -- a Saturday always falls in a 7-day window */ + throw new Error(`no Saturday found in June 20-26 ${year} — impossible`) +} + +/** + * True for a Saturday, a Sunday, or a Swedish public/"de facto" free day — + * i.e. exactly the set of dates the planner treats as free (see + * `is_free_day`/`_FREE_CATEGORIES` in the backend's `app/utils/holidays.py` + * and `app/services/allocation_service.py`; `PlanningService. + * _reject_illegal_free_day` rejects a `vacation`-typed pin on any of them). + * Computed purely from date math, like `easterSunday` above — deliberately + * independent of the backend's `swedish_calendar.json` cache, so a test + * that uses this to pick "a range with no free days in it" isn't + * validating itself against the very data the system under test reads. + * Not meant to be exhaustive for every calendar nuance (klämdagar are + * intentionally excluded — they're workable, not free) — only for the + * free days a test needs to steer clear of. + */ +export function isSwedishFreeDay(date: Date): boolean { + const day = date.getUTCDay() + if (day === 0 || day === 6) return true // Sunday or Saturday + + const year = date.getUTCFullYear() + const iso = toIsoDate(date) + + const fixed = new Set([ + `${year}-01-01`, // Nyårsdagen + `${year}-01-06`, // Trettondedag jul + `${year}-05-01`, // Första maj + `${year}-06-06`, // Sveriges nationaldag + `${year}-12-24`, // Julafton (de facto) + `${year}-12-25`, // Juldagen + `${year}-12-26`, // Annandag jul + `${year}-12-31`, // Nyårsafton (de facto) + ]) + if (fixed.has(iso)) return true + + const easter = easterSunday(year) + const midsummer = midsummerDay(year) + const moveable = [ + addDays(easter, -2), // Långfredagen + addDays(easter, -1), // Påskafton (de facto) + easter, // Påskdagen + addDays(easter, 1), // Annandag påsk + addDays(easter, 39), // Kristi himmelsfärdsdag + addDays(easter, 48), // Pingstafton (de facto) + addDays(easter, 49), // Pingstdagen + addDays(midsummer, -1), // Midsommarafton (de facto) — midsummer day itself is always a Saturday, already covered above + ].map(toIsoDate) + + return moveable.includes(iso) +} + /** Monday of the ISO week containing `date` (Mon=0 .. Sun=6 offset). */ export function mondayOfWeek(date: Date): Date { const isoWeekday = (date.getUTCDay() + 6) % 7 // 0 = Monday diff --git a/frontend/e2e/tests/drag-create-delete.spec.ts b/frontend/e2e/tests/drag-create-delete.spec.ts index eca9ff8..36d6af6 100644 --- a/frontend/e2e/tests/drag-create-delete.spec.ts +++ b/frontend/e2e/tests/drag-create-delete.spec.ts @@ -29,7 +29,7 @@ */ import { test, expect } from '@playwright/test' import { goToCalendarMonth, calendarDayCell, dragSelectDates } from '../support/calendarNav' -import { toIsoDate, addDays } from '../support/dates' +import { toIsoDate, addDays, isSwedishFreeDay } from '../support/dates' import { uniqueEmail, registerAndReachOnboarding, fillChildStep, continueSoloAndSkipBalances } from '../support/onboarding' test.use({ storageState: { cookies: [], origins: [] } }) @@ -41,20 +41,30 @@ test.use({ storageState: { cookies: [], origins: [] } }) * all after the "no phantom week" fix; see `Calendar.tsx`'s week-building * loop). Returns that month plus the last two of its days and the first day * of the next month — the exact range the drag below crosses. + * + * None of the three dates may be a free day (weekend or red day): the + * first test below saves them with an explicit `vacation` day type, and + * the second lets the planner fund them as a wish — both deliberately drop + * free days from what actually gets persisted (see `handleSavePinnedDay` + * in `src/pages/Calendar.tsx` and the backend's `PlanningService. + * _reject_illegal_free_day`/`is_free_day`). Checking only the month's last + * day for a Sunday missed two cases: (1) that day being a Monday instead, + * which makes `dates[0]` (the day before it) a Sunday, and (2) a red day + * landing on any of the three regardless of weekday (e.g. the month rolls + * into a New Year's Day) — either would silently persist/fund fewer than + * three days, which the assertions below don't expect. */ function targetRange(): { month: Date; dates: [string, string, string] } { const now = new Date() let month = new Date(Date.UTC(now.getUTCFullYear(), now.getUTCMonth() + 2, 1)) let lastDayOfMonth = new Date(Date.UTC(month.getUTCFullYear(), month.getUTCMonth() + 1, 0)) - while (lastDayOfMonth.getUTCDay() === 0 /* Sunday */) { + let candidateDates = [addDays(lastDayOfMonth, -1), lastDayOfMonth, addDays(lastDayOfMonth, 1)] + while (candidateDates.some(isSwedishFreeDay)) { month = new Date(Date.UTC(month.getUTCFullYear(), month.getUTCMonth() + 1, 1)) lastDayOfMonth = new Date(Date.UTC(month.getUTCFullYear(), month.getUTCMonth() + 1, 0)) + candidateDates = [addDays(lastDayOfMonth, -1), lastDayOfMonth, addDays(lastDayOfMonth, 1)] } - const dates = [addDays(lastDayOfMonth, -1), lastDayOfMonth, addDays(lastDayOfMonth, 1)].map(toIsoDate) as [ - string, - string, - string, - ] + const dates = candidateDates.map(toIsoDate) as [string, string, string] return { month, dates } } diff --git a/frontend/src/components/settings/PartnerInvitePanel.test.tsx b/frontend/src/components/settings/PartnerInvitePanel.test.tsx index f85ef05..682885e 100644 --- a/frontend/src/components/settings/PartnerInvitePanel.test.tsx +++ b/frontend/src/components/settings/PartnerInvitePanel.test.tsx @@ -92,4 +92,14 @@ describe('PartnerInvitePanel', () => { expect(await screen.findByRole('alert')).toHaveTextContent('list boom') }) + + it('does not request invitations while workspaceId is still empty (Settings mounts before WorkspaceContext populates it)', async () => { + const { rerender } = render() + await waitFor(() => expect(screen.getByText('Laddar')).toBeInTheDocument()) + expect(mockListInvitations).not.toHaveBeenCalled() + + // Once the real workspace id lands, the panel fetches for it. + rerender() + await waitFor(() => expect(mockListInvitations).toHaveBeenCalledWith('ws-1')) + }) }) diff --git a/frontend/src/components/settings/PartnerInvitePanel.tsx b/frontend/src/components/settings/PartnerInvitePanel.tsx index 5df5003..fefe68c 100644 --- a/frontend/src/components/settings/PartnerInvitePanel.tsx +++ b/frontend/src/components/settings/PartnerInvitePanel.tsx @@ -30,6 +30,15 @@ export function PartnerInvitePanel({ workspaceId }: PartnerInvitePanelProps) { const [revokeError, setRevokeError] = useState(null) const loadInvitations = useCallback(async () => { + // Settings mounts (via RequireAuth) before WorkspaceContext has + // populated `workspaceId`, so this can run once with `workspaceId` + // still `''`. An empty path segment collapses in Vite's dev proxy, so + // `GET /api/workspace//invitations` becomes `GET /api/workspace/invitations` + // — FastAPI then matches `workspace_id="invitations"` against the + // `{workspace_id}` route and 422s on UUID parsing. Same guard as + // `useWorkspaceMembers.ts`; the effect below re-runs once the real id + // lands (it's a dependency of this callback). + if (!workspaceId) return setIsListLoading(true) setListError(null) try { diff --git a/frontend/src/context/PlanSyncProvider.test.tsx b/frontend/src/context/PlanSyncProvider.test.tsx index 3c19e87..f9565a4 100644 --- a/frontend/src/context/PlanSyncProvider.test.tsx +++ b/frontend/src/context/PlanSyncProvider.test.tsx @@ -185,6 +185,27 @@ describe('PlanSyncProvider', () => { consoleErrorSpy.mockRestore() }) + it('refreshes the workspace context instead of just logging when the target workspace 404s (stale id)', async () => { + const refreshContext = vi.fn() + mockWorkspace({ workspaceId: 'ws-stale', planVersion: 0, bumpPlanVersion, refreshContext }) + vi.mocked(api.syncPlan).mockRejectedValueOnce( + new ApiRequestError(404, { code: 'not_found', message: 'Workspace ws-stale not found', details: {} }), + ) + const consoleWarnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + + renderHook(() => usePlanSyncContext(), { wrapper }) + + await waitFor(() => expect(refreshContext).toHaveBeenCalledTimes(1)) + // A 404 is the expected "stale reference" case, not an unhandled + // background failure — it must not also go through the generic + // console.error path. + expect(consoleErrorSpy).not.toHaveBeenCalled() + expect(consoleWarnSpy).toHaveBeenCalledWith(expect.stringContaining('ws-stale')) + consoleWarnSpy.mockRestore() + consoleErrorSpy.mockRestore() + }) + it('logs a non-Error rejection without swallowing it', async () => { vi.mocked(api.syncPlan).mockRejectedValueOnce('network exploded') const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) diff --git a/frontend/src/context/PlanSyncProvider.tsx b/frontend/src/context/PlanSyncProvider.tsx index 92d70c5..2bbf79f 100644 --- a/frontend/src/context/PlanSyncProvider.tsx +++ b/frontend/src/context/PlanSyncProvider.tsx @@ -59,7 +59,7 @@ const PlanSyncContext = createContext(null) * a "plan input" write itself. */ export function PlanSyncProvider({ children }: { children: React.ReactNode }) { - const { workspaceId, planVersion } = useWorkspace() + const { workspaceId, planVersion, refreshContext } = useWorkspace() const { isAuthenticated } = useAuth() const [latestProposal, setLatestProposal] = useState(null) const [latestProposalVersion, setLatestProposalVersion] = useState(0) @@ -98,15 +98,32 @@ export function PlanSyncProvider({ children }: { children: React.ReactNode }) { setLatestProposal(result.proposal) setLatestProposalVersion((v) => v + 1) } catch (err) { - // No toast exists for a background sync — this is the minimum bar - // from the module doc comment: never an empty catch, always logged - // through the shared API-error model instead of silently dropped. - const message = isApiError(err) - ? `${err.status} ${err.error.message}` - : err instanceof Error - ? err.message - : String(err) - console.error(`Background plan sync failed for workspace ${id}: ${message}`) + if (isApiError(err) && err.status === 404) { + // `id` doesn't exist server-side any more — not the active + // workspace, just a stale one this effect was handed (e.g. + // `workspaceId` seeded synchronously from localStorage on cold load + // — see `WorkspaceContext.tsx` — racing ahead of that context's own + // `refreshContext()` call, or a workspace a reseed since dropped). + // Retrying `id` forever would just 404 on every future planVersion + // bump too, so refetch the caller's *current* workspace from the + // server instead of only logging; that both corrects `workspaceId` + // (re-triggering this effect for the real id) and overwrites the + // stale value this provider never itself sets/clears. + console.warn( + `Background plan sync targeted stale workspace ${id} (not found) — refreshing workspace context`, + ) + void refreshContext() + } else { + // No toast exists for a background sync — this is the minimum bar + // from the module doc comment: never an empty catch, always logged + // through the shared API-error model instead of silently dropped. + const message = isApiError(err) + ? `${err.status} ${err.error.message}` + : err instanceof Error + ? err.message + : String(err) + console.error(`Background plan sync failed for workspace ${id}: ${message}`) + } } finally { inFlightRef.current = false } @@ -116,7 +133,7 @@ export function PlanSyncProvider({ children }: { children: React.ReactNode }) { pendingWorkspaceIdRef.current = null await runSyncOnce(nextId) } - }, []) + }, [refreshContext]) useEffect(() => { // `workspaceId` is seeded synchronously from `localStorage` (see