jacobh2/F-r-ldraledigplannerare
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge branch 'worktree-check-security-audit'

Browse Source 
Security and frontend-correctness hardening: analytics IDOR fix, scraper
log scrubbing, lowercase email normalisation (migration 009), stale-response
guards in the plan hooks, busy flags cleared unconditionally across all
useLatestRequest consumers, Calendar no longer blanks on mutation, plus
dependabot/CI config and removal of the stray frontend/out.js.

Conflicts came from main's later "day assertions" restructure, which made
usePlannedDays read-only and replaced the calendar's per-day add/update with
useDayAssertions.assertDay. Both intents kept:

- usePlannedDays: branch's fetch machinery (isRefreshing, in-flight dedup,
  unguarded busy clear); mutators dropped as on main.
- usePlanSync: branch's useLatestRequest guard also applied to main's new
  syncWithWish.
- useDayAssertions / useLatestBalance (new on main): busy flag now cleared
  unconditionally, as the branch did for every other consumer.
- useDayAssertions.assertDay gains the branch's `skipRefresh` option and a
  `refresh`, so Calendar's multi-day save keeps the batch shape (N writes,
  one trailing bumpPlanVersion + one assertions refresh) on main's
  assertDay path. Branch's M7 tests ported from the removed add/update API
  to assertDay.
- PlanSyncProvider: branch's isAuthenticated gate kept; eviction-preview
  state stays removed as on main.
- usePlannedDayExclusions.ts stays deleted (superseded by useDayAssertions).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T4wjgbVL4HAC5Rdaw75VbV
This commit is contained in:
Jacob Hagstedt
2026-09-21 07:40:09 +02:00
parent 07815e7fb7 3928d8eae3
commit 4fa79bad77
47 changed files with 1271 additions and 190 deletions
Download Patch File Download Diff File Expand all files Collapse all files
frontend/src/context/PlanSyncProvider.test.tsx
+49
View File
@@ -3,10 +3,12 @@ import { renderHook, waitFor, act } from '@testing-library/react'
import type { ReactNode } from 'react'
import { PlanSyncProvider, usePlanSyncContext } from './PlanSyncProvider'
import { useWorkspace } from './WorkspaceContext'
import { useAuth } from './AuthContext'
import * as api from '@/services/api'
import { ApiRequestError } from '@/services/errors'
import {
makeWorkspaceContextValue,
makeAuthContextValue,
makePlanSyncResponse,
makePlanProposal,
makePlanWarning,
@@ -16,6 +18,9 @@ vi.mock('@/services/api')
vi.mock('./WorkspaceContext', () => ({
useWorkspace: vi.fn(),
}))
vi.mock('./AuthContext', () => ({
useAuth: vi.fn(),
}))
function wrapper({ children }: { children: ReactNode }) {
return <PlanSyncProvider>{children}</PlanSyncProvider>
@@ -25,6 +30,10 @@ function mockWorkspace(overrides: Partial<ReturnType<typeof useWorkspace>> = {})
vi.mocked(useWorkspace).mockReturnValue(makeWorkspaceContextValue(overrides))
}
function mockAuth(overrides: Partial<ReturnType<typeof useAuth>> = {}) {
vi.mocked(useAuth).mockReturnValue(makeAuthContextValue(overrides))
}
describe('PlanSyncProvider', () => {
let bumpPlanVersion: ReturnType<typeof vi.fn>
@@ -32,6 +41,11 @@ describe('PlanSyncProvider', () => {
vi.clearAllMocks()
bumpPlanVersion = vi.fn()
mockWorkspace({ workspaceId: 'ws-1', planVersion: 0, bumpPlanVersion })
// Every existing test in this file exercises the already-authenticated
// steady state (real app: PlanSyncProvider is mounted inside
// AuthProvider — see App.tsx). The unauthenticated-cold-load case (M9)
// has its own describe block below with an explicit override.
mockAuth({ isAuthenticated: true })
vi.mocked(api.syncPlan).mockResolvedValue(makePlanSyncResponse())
})
@@ -42,6 +56,41 @@ describe('PlanSyncProvider', () => {
expect(api.syncPlan).not.toHaveBeenCalled()
})
describe('M9 — gated on auth, not just workspaceId', () => {
it('does not sync on a cold load before auth has resolved, even with a workspaceId already seeded from localStorage', async () => {
// Mirrors WorkspaceContext.tsx's real cold-load shape: workspaceId is
// seeded synchronously from localStorage while AuthContext's
// mount-time refreshToken() is still in flight (isAuthenticated still
// false). Without the auth gate this fires an unauthenticated
// POST /api/plan/sync.
mockAuth({ isAuthenticated: false })
mockWorkspace({ workspaceId: 'ws-1', planVersion: 0, bumpPlanVersion })
renderHook(() => usePlanSyncContext(), { wrapper })
// Give any wrongly-fired effect a tick to resolve before asserting.
await act(async () => {
await Promise.resolve()
})
expect(api.syncPlan).not.toHaveBeenCalled()
})
it('syncs once auth resolves to authenticated', async () => {
mockAuth({ isAuthenticated: false })
mockWorkspace({ workspaceId: 'ws-1', planVersion: 0, bumpPlanVersion })
const { rerender } = renderHook(() => usePlanSyncContext(), { wrapper })
expect(api.syncPlan).not.toHaveBeenCalled()
// The mount-time refreshToken() call in AuthContext resolves.
mockAuth({ isAuthenticated: true })
rerender()
await waitFor(() => expect(api.syncPlan).toHaveBeenCalledTimes(1))
expect(api.syncPlan).toHaveBeenCalledWith('ws-1')
})
})
it('syncs silently once the workspace is known', async () => {
renderHook(() => usePlanSyncContext(), { wrapper })
frontend/src/context/PlanSyncProvider.tsx
+23 -1
View File
@@ -1,6 +1,7 @@
import { createContext, useCallback, useContext, useEffect, useRef, useState } from 'react'
import * as api from '@/services/api'
import { useWorkspace } from './WorkspaceContext'
import { useAuth } from './AuthContext'
import { isApiError } from '@/services/errors'
import type { PlanProposal } from '@/types'
@@ -39,6 +40,17 @@ const PlanSyncContext = createContext<PlanSyncContextValue | null>(null)
* instance every plan-input hook already bumps, and runs for the lifetime
* of the app, not just while Calendar happens to be mounted.
*
* Also gated on `AuthContext`'s `isAuthenticated` — `workspaceId` is seeded
* synchronously from `localStorage` (see `WorkspaceContext.tsx`), so on a
* cold load it's already truthy before `AuthContext`'s mount-time
* `refreshToken()` call resolves. Without this gate the effect below fires
* an unauthenticated `POST /api/plan/sync`, which 401s and starts a second,
* redundant `/auth/refresh` racing `AuthContext`'s own. Reading `useAuth`
* here does not reintroduce the cycle the note below warns about:
* `AuthContext` never reads `WorkspaceContext`/`PlanSyncProvider` state, so
* this is a one-way dependency, not a loop — and `AuthProvider` sits above
* `WorkspaceProvider` in `App.tsx`, so the context is always available.
*
* **Cycle safety.** This effect must NEVER call `bumpPlanVersion`.
* `planVersion` is the shared invalidation signal every plan-input hook's
* own fetch effect watches (see `usePlannedDays.ts:84-93`); a sync path
@@ -48,6 +60,7 @@ const PlanSyncContext = createContext<PlanSyncContextValue | null>(null)
*/
export function PlanSyncProvider({ children }: { children: React.ReactNode }) {
const { workspaceId, planVersion } = useWorkspace()
const { isAuthenticated } = useAuth()
const [latestProposal, setLatestProposal] = useState<PlanProposal | null>(null)
const [latestProposalVersion, setLatestProposalVersion] = useState(0)
@@ -106,6 +119,15 @@ export function PlanSyncProvider({ children }: { children: React.ReactNode }) {
}, [])
useEffect(() => {
// `workspaceId` is seeded synchronously from `localStorage` (see
// `WorkspaceContext.tsx`), so on a cold load it's already set before
// `AuthContext`'s mount-time `refreshToken()` resolves. Without this
// gate, that window fires `POST /api/plan/sync` unauthenticated — a
// guaranteed 401 that then races `AuthContext`'s own in-flight
// `/auth/refresh` against the second one `api.ts`'s 401-retry starts.
// `isAuthenticated` flipping true (login, or the mount refresh landing)
// re-runs this effect and syncs then, same as a `planVersion` bump would.
if (!isAuthenticated) return
if (!workspaceId) return
if (inFlightRef.current) {
// A sync is already running — record that another bump landed and
@@ -116,7 +138,7 @@ export function PlanSyncProvider({ children }: { children: React.ReactNode }) {
return
}
void runSyncOnce(workspaceId)
}, [workspaceId, planVersion, runSyncOnce])
}, [isAuthenticated, workspaceId, planVersion, runSyncOnce])
return (
<PlanSyncContext.Provider
frontend/src/context/WorkspaceContext.test.tsx
+11 -37
View File
@@ -70,6 +70,7 @@ const userContext: api.UserContext = {
{ id: 'c1', name: 'Child One', birth_date: '2024-01-01' },
{ id: 'c2', name: 'Child Two', birth_date: '2025-01-01' },
],
take_pl_on_red_days: false,
}
describe('WorkspaceContext', () => {
@@ -92,23 +93,26 @@ describe('WorkspaceContext', () => {
expect(result.current.isLoading).toBe(false)
})
it('loads user context and workspace on mount when authenticated', async () => {
it('loads user context on mount when authenticated, with no follow-up workspace fetch', async () => {
// H12 step 1: cold load used to be getUserContext() -> getWorkspace(id).
// The workspace state this provider needs (id + takePlOnRedDays) now
// comes straight from the context response, so getWorkspace must not
// be called at all on this path.
vi.mocked(useAuth).mockReturnValue(authenticated)
vi.mocked(api.getUserContext).mockResolvedValue(userContext)
vi.mocked(api.getWorkspace).mockResolvedValue(workspace)
const { result } = renderHook(() => useWorkspace(), { wrapper })
await waitFor(() => expect(result.current.workspace).not.toBeNull())
expect(api.getUserContext).toHaveBeenCalled()
expect(api.getWorkspace).toHaveBeenCalledWith('ws1')
expect(api.getWorkspace).not.toHaveBeenCalled()
expect(result.current.workspaceId).toBe('ws1')
expect(result.current.personalDataUuid).toBe('pd1')
expect(result.current.parentName).toBe('Anna')
expect(result.current.children).toHaveLength(2)
expect(result.current.selectedChildId).toBe('c1')
expect(result.current.workspace).toEqual(workspace)
expect(result.current.workspace).toEqual({ id: 'ws1', takePlOnRedDays: false })
expect(localStorage.getItem('workspaceId')).toBe('ws1')
expect(result.current.isLoading).toBe(false)
})
@@ -136,31 +140,15 @@ describe('WorkspaceContext', () => {
expect(result.current.error).toBe('Failed to load user context')
})
it('sets an error when the workspace fetch inside refreshContext fails', async () => {
vi.mocked(useAuth).mockReturnValue(authenticated)
vi.mocked(api.getUserContext).mockResolvedValue(userContext)
vi.mocked(api.getWorkspace).mockRejectedValue(new Error('workspace boom'))
const { result } = renderHook(() => useWorkspace(), { wrapper })
await waitFor(() => expect(result.current.isLoading).toBe(false))
expect(result.current.error).toBe('workspace boom')
expect(result.current.workspace).toBeNull()
// But the rest of the context from getUserContext was still applied.
expect(result.current.workspaceId).toBe('ws1')
expect(result.current.selectedChildId).toBe('c1')
})
it('defaults to an empty children list when the context response omits children', async () => {
vi.mocked(useAuth).mockReturnValue(authenticated)
const contextWithoutChildren = {
personal_data_uuid: userContext.personal_data_uuid,
workspace_id: userContext.workspace_id,
parent_name: userContext.parent_name,
take_pl_on_red_days: false,
} as unknown as api.UserContext
vi.mocked(api.getUserContext).mockResolvedValue(contextWithoutChildren)
vi.mocked(api.getWorkspace).mockResolvedValue(workspace)
const { result } = renderHook(() => useWorkspace(), { wrapper })
@@ -170,19 +158,6 @@ describe('WorkspaceContext', () => {
expect(result.current.selectedChildId).toBeNull()
})
it('sets a non-Error rejection to a generic message when the workspace fetch inside refreshContext fails', async () => {
vi.mocked(useAuth).mockReturnValue(authenticated)
vi.mocked(api.getUserContext).mockResolvedValue(userContext)
vi.mocked(api.getWorkspace).mockRejectedValue('nope')
const { result } = renderHook(() => useWorkspace(), { wrapper })
await waitFor(() => expect(result.current.isLoading).toBe(false))
expect(result.current.error).toBe('Failed to load workspace')
expect(result.current.workspace).toBeNull()
})
it('refetch sets a non-Error rejection to a generic message', async () => {
vi.mocked(useAuth).mockReturnValue(authenticated)
vi.mocked(api.getUserContext).mockResolvedValue(userContext)
@@ -311,15 +286,14 @@ describe('WorkspaceContext', () => {
rerender()
await waitFor(() => expect(result.current.workspace).toBeNull())
const otherWorkspace: Workspace = { ...workspace, id: 'ws2', name: 'Other workspace' }
const otherUserContext: api.UserContext = {
personal_data_uuid: 'pd2',
workspace_id: 'ws2',
parent_name: 'Alex',
children: [{ id: 'c3', name: 'Child Three', birth_date: '2026-01-01' }],
take_pl_on_red_days: true,
}
vi.mocked(api.getUserContext).mockResolvedValue(otherUserContext)
vi.mocked(api.getWorkspace).mockResolvedValue(otherWorkspace)
vi.mocked(api.getUserContext).mockClear()
vi.mocked(useAuth).mockReturnValue(authenticated)
@@ -330,7 +304,7 @@ describe('WorkspaceContext', () => {
expect(result.current.workspaceId).toBe('ws2')
expect(result.current.personalDataUuid).toBe('pd2')
expect(result.current.selectedChildId).toBe('c3')
expect(result.current.workspace).toEqual(otherWorkspace)
expect(result.current.workspace).toEqual({ id: 'ws2', takePlOnRedDays: true })
})
it('discards a refreshContext response that resolves after logout (S4)', async () => {
frontend/src/context/WorkspaceContext.tsx
+6 -9
View File
@@ -98,15 +98,12 @@ export function WorkspaceProvider({ children: childrenProp }: { children: React.
if (childrenList.length > 0 && childrenList[0]) {
setSelectedChildId(childrenList[0].id)
}
try {
const ws = await api.getWorkspace(context.workspace_id)
if (generation !== generationRef.current) return
setWorkspace(ws)
} catch (err: unknown) {
if (generation !== generationRef.current) return
setError(err instanceof Error ? err.message : 'Failed to load workspace')
setWorkspace(null)
}
// H12 step 1: `GET /auth/context` now carries `take_pl_on_red_days`,
// the only field of `Workspace` any consumer actually reads off the
// cold-load `workspace` (see `Workspace`'s doc comment in
// src/types/entities.ts) — so building it here replaces a follow-up
// `GET /workspace/:id` that was otherwise redundant on every cold load.
setWorkspace({ id: context.workspace_id, takePlOnRedDays: context.take_pl_on_red_days })
} catch (err: unknown) {
if (generation !== generationRef.current) return
setError(err instanceof Error ? err.message : 'Failed to load user context')
frontend/src/hooks/useAnalytics.ts
+6 -3
View File
@@ -39,9 +39,12 @@ export function useAnalytics(workspaceId: string | null, childId?: string): UseA
setError(err instanceof Error ? err.message : 'Failed to fetch analytics')
setData(null)
} finally {
if (isCurrent(requestId)) {
setIsLoading(false)
}
// Unguarded: `isCurrent` decides which response's DATA wins, not
// whether a superseded call's own busy flag gets cleared — a stale
// call must still release the flag IT raised, or it can strand that
// flag true forever (see usePlannedDays.ts for the hang this exact
// guard caused there).
setIsLoading(false)
}
}, [workspaceId, childId, start, isCurrent])
frontend/src/hooks/useCalendarDays.ts
+6 -1
View File
@@ -101,7 +101,12 @@ export function useCalendarDays(
if (!isCurrent(requestId)) return
setError(err instanceof Error ? err.message : 'Failed to load calendar days')
} finally {
if (isCurrent(requestId)) setBusy(false)
// Unguarded: `isCurrent` decides which response's DATA wins, not
// whether a superseded call's own busy flag gets cleared — a stale
// call must still release the flag IT raised, or it can strand
// that flag true forever (see usePlannedDays.ts for the hang this
// exact guard caused there).
setBusy(false)
}
},
[start, isCurrent],
frontend/src/hooks/useChildSettings.ts
+6 -1
View File
@@ -55,7 +55,12 @@ export function useChildSettings(childId: string | null): UseChildSettingsResult
if (!isCurrent(requestId)) return
setError(err instanceof Error ? err.message : 'Failed to load child')
} finally {
if (isCurrent(requestId)) setBusy(false)
// Unguarded: `isCurrent` decides which response's DATA wins, not
// whether a superseded call's own busy flag gets cleared — a stale
// call must still release the flag IT raised, or it can strand
// that flag true forever (see usePlannedDays.ts for the hang this
// exact guard caused there).
setBusy(false)
}
},
[childId, start, isCurrent],
frontend/src/hooks/useDayAssertions.test.ts
+38
View File
@@ -87,6 +87,44 @@ describe('useDayAssertions', () => {
expect(bumpPlanVersion).toHaveBeenCalledTimes(1)
})
// Ported from `usePlannedDays.test.ts`'s "M7 part 2" block when its own
// mutators moved here — the per-write `skipRefresh` contract behind
// `Calendar.tsx`'s multi-day save (N writes, one trailing bump).
describe('M7 part 2 — suppressing the per-call refetch/bump (skipRefresh)', () => {
it('assertDay: skipRefresh skips this call\'s own refetch and bumpPlanVersion', async () => {
const { result } = renderHook(() => useDayAssertions('ws-1'))
await waitFor(() => expect(result.current.isLoading).toBe(false))
const callCountBefore = vi.mocked(api.listDayAssertionsByWorkspace).mock.calls.length
const created = await result.current.assertDay(
{
workspaceId: 'ws-1',
childId: 'child-1',
parentPersonalDataUuid: 'parent-1',
date: '2027-02-03',
dayType: 'vacation',
},
{ skipRefresh: true },
)
expect(api.assertDay).toHaveBeenCalledTimes(1)
expect(created.id).toBe('da-1')
expect(vi.mocked(api.listDayAssertionsByWorkspace).mock.calls.length).toBe(callCountBefore)
expect(bumpPlanVersion).not.toHaveBeenCalled()
})
it('refresh: refetches the list once, for a caller catching up after a batch', async () => {
const { result } = renderHook(() => useDayAssertions('ws-1'))
await waitFor(() => expect(result.current.isLoading).toBe(false))
const callCountBefore = vi.mocked(api.listDayAssertionsByWorkspace).mock.calls.length
await result.current.refresh()
expect(vi.mocked(api.listDayAssertionsByWorkspace).mock.calls.length).toBe(callCountBefore + 1)
expect(bumpPlanVersion).not.toHaveBeenCalled()
})
})
it('sets an error when the fetch fails', async () => {
vi.mocked(api.listDayAssertionsByWorkspace).mockRejectedValueOnce(new Error('Network error'))
frontend/src/hooks/useDayAssertions.ts
+29 -7
View File
@@ -8,10 +8,25 @@ interface UseDayAssertionsResult {
data: DayAssertion[]
isLoading: boolean
error: string | null
/** Refetches the workspace's assertions — for a caller that batched
* several `skipRefresh` writes and now wants the list caught up once. */
refresh: () => Promise<void>
/** Pin a concrete day type, register an ad-hoc day, or record an
* exclusion (`dayType: 'none'`) — mirrors immediately into
* `planned_leave_day`, see `api.assertDay`. */
assertDay: (req: DayAssertionCreateRequest) => Promise<DayAssertion>
* `planned_leave_day`, see `api.assertDay`.
*
* `options.skipRefresh` skips this call's own list refetch AND its
* `bumpPlanVersion()` — for a caller issuing several writes back-to-back
* (e.g. a multi-day save) that wants exactly one resulting refetch instead
* of one per write. The caller is then responsible for calling
* `bumpPlanVersion` (and `refresh` above) itself once after the batch,
* including on a failure partway through it — see `Calendar.tsx`'s
* `handleSavePinnedDay`. Without it, every write's own bump re-runs
* `usePlannedDays`' fetch effect — N refetches for an N-day save. */
assertDay: (
req: DayAssertionCreateRequest,
options?: { skipRefresh?: boolean },
) => Promise<DayAssertion>
/** Retract the assertion at a (workspace, child, parent, date) key, if
* any — idempotent. */
clear: (key: DayAssertionKey) => Promise<void>
@@ -47,7 +62,12 @@ export function useDayAssertions(workspaceId: string | null): UseDayAssertionsRe
if (!isCurrent(requestId)) return
setError(err instanceof Error ? err.message : 'Failed to load day assertions')
} finally {
if (isCurrent(requestId)) setIsLoading(false)
// Unguarded: `isCurrent` decides which response's DATA wins, not
// whether a superseded call's own busy flag gets cleared — a stale
// call must still release the flag IT raised, or it can strand that
// flag true forever (see usePlannedDays.ts for the hang this exact
// guard caused there).
setIsLoading(false)
}
}, [workspaceId, start, isCurrent])
@@ -56,10 +76,12 @@ export function useDayAssertions(workspaceId: string | null): UseDayAssertionsRe
}, [fetch])
const assertDay = useCallback(
async (req: DayAssertionCreateRequest) => {
async (req: DayAssertionCreateRequest, options?: { skipRefresh?: boolean }) => {
const created = await api.assertDay(req)
await fetch()
bumpPlanVersion()
if (!options?.skipRefresh) {
await fetch()
bumpPlanVersion()
}
return created
},
[fetch, bumpPlanVersion],
@@ -74,5 +96,5 @@ export function useDayAssertions(workspaceId: string | null): UseDayAssertionsRe
[fetch, bumpPlanVersion],
)
return { data, isLoading, error, assertDay, clear }
return { data, isLoading, error, refresh: fetch, assertDay, clear }
}
frontend/src/hooks/useDayOverrides.ts
+6 -1
View File
@@ -42,7 +42,12 @@ export function useDayOverrides(workspaceId: string): UseDayOverridesResult {
if (!isCurrent(requestId)) return
setError(err instanceof Error ? err.message : 'Failed to load day overrides')
} finally {
if (isCurrent(requestId)) setBusy(false)
// Unguarded: `isCurrent` decides which response's DATA wins, not
// whether a superseded call's own busy flag gets cleared — a stale
// call must still release the flag IT raised, or it can strand
// that flag true forever (see usePlannedDays.ts for the hang this
// exact guard caused there).
setBusy(false)
}
},
[workspaceId, start, isCurrent],
frontend/src/hooks/useLatestBalance.ts
+6 -1
View File
@@ -77,7 +77,12 @@ export function useLatestBalance(
setInitial(undefined)
setRaw(null)
} finally {
if (isCurrent(requestId)) setIsLoading(false)
// Unguarded: `isCurrent` decides which response's DATA wins, not
// whether a superseded call's own busy flag gets cleared — a stale
// call must still release the flag IT raised, or it can strand that
// flag true forever (see usePlannedDays.ts for the hang this exact
// guard caused there).
setIsLoading(false)
}
}, [personalDataUuid, childId, start, isCurrent])
frontend/src/hooks/useLeaveWishes.ts
+6 -1
View File
@@ -61,7 +61,12 @@ export function useLeaveWishes(workspaceId: string | null): UseLeaveWishesResult
if (!isCurrent(requestId)) return
setError(err instanceof Error ? err.message : 'Failed to load leave wishes')
} finally {
if (isCurrent(requestId)) setBusy(false)
// Unguarded: `isCurrent` decides which response's DATA wins, not
// whether a superseded call's own busy flag gets cleared — a stale
// call must still release the flag IT raised, or it can strand
// that flag true forever (see usePlannedDays.ts for the hang this
// exact guard caused there).
setBusy(false)
}
},
[workspaceId, start, isCurrent],
frontend/src/hooks/useOptimizationTips.ts
+6 -1
View File
@@ -32,7 +32,12 @@ export function useOptimizationTips(workspaceId: string | null): UseOptimization
setError(err instanceof Error ? err.message : 'Failed to fetch tips')
setData(null)
} finally {
if (isCurrent(requestId)) setIsLoading(false)
// Unguarded: `isCurrent` decides which response's DATA wins, not
// whether a superseded call's own busy flag gets cleared — a stale
// call must still release the flag IT raised, or it can strand that
// flag true forever (see usePlannedDays.ts for the hang this exact
// guard caused there).
setIsLoading(false)
}
}, [workspaceId, start, isCurrent])
frontend/src/hooks/useParentalLeave.ts
+6 -1
View File
@@ -40,7 +40,12 @@ export function useParentalLeave(
setError(err instanceof Error ? err.message : 'Failed to fetch parental leave data')
setData(null)
} finally {
if (isCurrent(requestId)) setIsLoading(false)
// Unguarded: `isCurrent` decides which response's DATA wins, not
// whether a superseded call's own busy flag gets cleared — a stale
// call must still release the flag IT raised, or it can strand that
// flag true forever (see usePlannedDays.ts for the hang this exact
// guard caused there).
setIsLoading(false)
}
}, [personalDataUuid, childId, start, isCurrent])
frontend/src/hooks/useParentalLeaves.ts
+6 -1
View File
@@ -61,7 +61,12 @@ export function useParentalLeaves(memberUuids: string[], childId: string | null)
setError(err instanceof Error ? err.message : 'Failed to fetch parental leave data')
setData({})
} finally {
if (isCurrent(requestId)) setIsLoading(false)
// Unguarded: `isCurrent` decides which response's DATA wins, not
// whether a superseded call's own busy flag gets cleared — a stale
// call must still release the flag IT raised, or it can strand that
// flag true forever (see usePlannedDays.ts for the hang this exact
// guard caused there).
setIsLoading(false)
}
}, [memberKey, childId, start, isCurrent])
frontend/src/hooks/usePlanProposal.test.ts
+39
View File
@@ -65,6 +65,45 @@ describe('usePlanProposal', () => {
expect(result.current.data).toBeNull()
})
it('discards a stale response from an earlier, slower propose() call (P1 stale-response guard)', async () => {
let resolveFirst: (value: ReturnType<typeof makePlanProposal>) => void = () => {}
let resolveSecond: (value: ReturnType<typeof makePlanProposal>) => void = () => {}
const firstPromise = new Promise<ReturnType<typeof makePlanProposal>>((resolve) => {
resolveFirst = resolve
})
const secondPromise = new Promise<ReturnType<typeof makePlanProposal>>((resolve) => {
resolveSecond = resolve
})
vi.mocked(api.proposePlan).mockReturnValueOnce(firstPromise).mockReturnValueOnce(secondPromise)
const { result } = renderHook(() => usePlanProposal('ws1'))
let firstCall: Promise<void> = Promise.resolve()
let secondCall: Promise<void> = Promise.resolve()
act(() => {
firstCall = result.current.propose()
})
act(() => {
secondCall = result.current.propose()
})
// The newer (second) call resolves first...
const fastProposal = makePlanProposal({ warnings: [] })
await act(async () => {
resolveSecond(fastProposal)
await secondCall
})
expect(result.current.data).toEqual(fastProposal)
// ...then the stale first response arrives late and must not overwrite it.
const slowProposal = makePlanProposal({ surplusAdvice: [makeSurplusAdvice()] })
await act(async () => {
resolveFirst(slowProposal)
await firstCall
})
expect(result.current.data).toEqual(fastProposal)
})
it('falls back to a generic message when a non-Error value is thrown', async () => {
vi.mocked(api.proposePlan).mockRejectedValue('boom')
frontend/src/hooks/usePlanProposal.ts
+16 -1
View File
@@ -1,5 +1,6 @@
import { useCallback, useState } from 'react'
import * as api from '@/services/api'
import { useLatestRequest } from './useLatestRequest'
import { usePlanSyncContextOptional } from '@/context'
import type { PlanProposal } from '@/types'
@@ -35,27 +36,41 @@ interface UsePlanProposalResult {
* this hook keeps working when rendered outside a `<PlanSyncProvider>` —
* e.g. this file's own unit tests, or a page test that auto-mocks
* `@/context` wholesale — falling back to `propose()`-only behaviour.
*
* A second `propose()` call before an earlier one resolves (e.g. a quick
* re-trigger) must not let the earlier, slower response land last and
* overwrite the newer one — `useLatestRequest` guards every `setState` in
* `propose()` for exactly that, same pattern as `useLeaveWishes.ts`.
*/
export function usePlanProposal(workspaceId: string | null): UsePlanProposalResult {
const [data, setData] = useState<PlanProposal | null>(null)
const [isLoading, setIsLoading] = useState(false)
const [error, setError] = useState<string | null>(null)
const syncContext = usePlanSyncContextOptional()
const { start, isCurrent } = useLatestRequest()
const propose = useCallback(async () => {
if (!workspaceId) return
const requestId = start()
setIsLoading(true)
setError(null)
try {
const result = await api.proposePlan(workspaceId)
if (!isCurrent(requestId)) return
setData(result)
} catch (err: unknown) {
if (!isCurrent(requestId)) return
setError(err instanceof Error ? err.message : 'Failed to compute plan proposal')
setData(null)
} finally {
// Unguarded: `isCurrent` decides which response's DATA wins, not
// whether a superseded call's own busy flag gets cleared — a stale
// call must still release the flag IT raised, or it can strand that
// flag true forever (see usePlannedDays.ts for the hang this exact
// guard caused there).
setIsLoading(false)
}
}, [workspaceId])
}, [workspaceId, start, isCurrent])
const hasSynced = (syncContext?.latestProposalVersion ?? 0) > 0
const freshest = hasSynced ? (syncContext?.latestProposal ?? null) : data
frontend/src/hooks/usePlanSync.test.ts
+39
View File
@@ -90,6 +90,45 @@ describe('usePlanSync', () => {
resolveSecond(makePlanSyncResponse())
})
it('discards a stale response from an earlier, slower sync() call (P1 stale-response guard)', async () => {
let resolveFirst: (value: ReturnType<typeof makePlanSyncResponse>) => void = () => {}
let resolveSecond: (value: ReturnType<typeof makePlanSyncResponse>) => void = () => {}
const firstPromise = new Promise<ReturnType<typeof makePlanSyncResponse>>((resolve) => {
resolveFirst = resolve
})
const secondPromise = new Promise<ReturnType<typeof makePlanSyncResponse>>((resolve) => {
resolveSecond = resolve
})
vi.mocked(api.syncPlan).mockReturnValueOnce(firstPromise).mockReturnValueOnce(secondPromise)
const { result } = renderHook(() => usePlanSync())
let firstCall: Promise<unknown> = Promise.resolve()
let secondCall: Promise<unknown> = Promise.resolve()
act(() => {
firstCall = result.current.sync('ws-1')
})
act(() => {
secondCall = result.current.sync('ws-1')
})
// The newer (second) call resolves first...
const fastResponse = makePlanSyncResponse({ changes: [makePlanChange({ kind: 'added' })] })
await act(async () => {
resolveSecond(fastResponse)
await secondCall
})
expect(result.current.lastChanges).toEqual(fastResponse.changes)
// ...then the stale first response arrives late and must not overwrite it.
const slowResponse = makePlanSyncResponse({ changes: [makePlanChange({ kind: 'retyped' })] })
await act(async () => {
resolveFirst(slowResponse)
await firstCall
})
expect(result.current.lastChanges).toEqual(fastResponse.changes)
})
it('sets a readable error and rethrows when the request rejects with an Error', async () => {
vi.mocked(api.syncPlan).mockRejectedValue(new Error('workspace not found'))
const { result } = renderHook(() => usePlanSync())
frontend/src/hooks/usePlanSync.ts
+28 -6
View File
@@ -1,5 +1,6 @@
import { useCallback, useState } from 'react'
import * as api from '@/services/api'
import { useLatestRequest } from './useLatestRequest'
import type { PlanSyncResponse, PlanSyncWithWishResponse, SyncPlanWithWishRequest } from '@/types'
interface UsePlanSyncResult {
@@ -23,45 +24,66 @@ interface UsePlanSyncResult {
* never retyping/overwriting a pinned day, and reporting exactly what
* changed. Callers (Calendar.tsx) drive their own toast/undo UI off the
* returned `changes`; this hook only owns the request's loading/error state.
*
* Calendar.tsx can call `sync()` again (e.g. a second edit) before an
* earlier call's response lands — `useLatestRequest` guards every setState
* below so a slow earlier response can never overwrite a faster later one's
* `lastChanges`/`error`. The caller-facing `throw` on rejection is
* unguarded (and always re-thrown) so each call's own awaiter still sees its
* own outcome; only the shared state updates are guarded.
*/
export function usePlanSync(): UsePlanSyncResult {
const [isSyncing, setIsSyncing] = useState(false)
const [error, setError] = useState<string | null>(null)
const [lastChanges, setLastChanges] = useState<PlanSyncResponse['changes'] | null>(null)
const { start, isCurrent } = useLatestRequest()
const sync = useCallback(async (workspaceId: string): Promise<PlanSyncResponse> => {
const requestId = start()
setIsSyncing(true)
setError(null)
setLastChanges(null)
try {
const result = await api.syncPlan(workspaceId)
setLastChanges(result.changes)
if (isCurrent(requestId)) setLastChanges(result.changes)
return result
} catch (err) {
setError(err instanceof Error ? err.message : 'Failed to sync the plan')
if (isCurrent(requestId)) {
setError(err instanceof Error ? err.message : 'Failed to sync the plan')
}
throw err
} finally {
// Unguarded: `isCurrent` decides which response's DATA wins, not
// whether a superseded call's own busy flag gets cleared — a stale
// call must still release the flag IT raised, or it can strand that
// flag true forever (see usePlannedDays.ts for the hang this exact
// guard caused there).
setIsSyncing(false)
}
}, [])
}, [start, isCurrent])
const syncWithWish = useCallback(
async (request: SyncPlanWithWishRequest): Promise<PlanSyncWithWishResponse> => {
const requestId = start()
setIsSyncing(true)
setError(null)
setLastChanges(null)
try {
const result = await api.syncPlanWithWish(request)
setLastChanges(result.sync.changes)
if (isCurrent(requestId)) setLastChanges(result.sync.changes)
return result
} catch (err) {
setError(err instanceof Error ? err.message : 'Failed to sync the plan')
if (isCurrent(requestId)) {
setError(err instanceof Error ? err.message : 'Failed to sync the plan')
}
throw err
} finally {
// Unguarded — same reasoning as `sync` above: a superseded call must
// still release the busy flag it raised.
setIsSyncing(false)
}
},
[],
[start, isCurrent],
)
return { isSyncing, error, lastChanges, sync, syncWithWish }
frontend/src/hooks/usePlannedDays.test.ts
+191 -1
View File
@@ -1,5 +1,6 @@
import { describe, it, expect, beforeEach, vi } from 'vitest'
import { renderHook, act } from '@testing-library/react'
import { createElement, StrictMode, type ReactNode } from 'react'
import { usePlannedDays } from './usePlannedDays'
import * as api from '@/services/api'
import { useWorkspace } from '@/context'
@@ -116,7 +117,193 @@ describe('usePlannedDays', () => {
expect(vi.mocked(api.getPlannedDays).mock.calls.length).toBeGreaterThan(callCount)
})
it('sets loading state during fetch', async () => {
it('M7 part 1 — a mutation results in exactly one background refetch, never isLoading (only isRefreshing flips)', async () => {
// This is the exact regression a browser verification pass caught: an
// earlier version had this hook's own mutators call `fetch({ background:
// true })` themselves AND rely on their own resulting `planVersion` bump
// to re-run this hook's effect a second time — and that second,
// effect-triggered fetch used the *blocking* default, so a save still
// blanked the page via a second GET. The writers (`useDayAssertions`,
// `usePlanSync`) now only ever call `bumpPlanVersion()`; the effect
// below is the ONLY place that fetches, so there is exactly one fetch,
// and it decides for itself whether that's the (blocking) first load or
// a (background) mutation refetch — see usePlannedDays.ts's doc comment.
vi.mocked(api.getPlannedDays).mockResolvedValue([])
// A bare `vi.fn()` `bumpPlanVersion` (this file's default) never causes
// a re-render on its own — wire up a real counter so calling it (as a
// writer hook would) and then `rerender()`ing genuinely reproduces what
// the real `WorkspaceProvider`'s state update would do.
let planVersion = 0
const bump = () => {
planVersion += 1
}
vi.mocked(useWorkspace).mockImplementation(() =>
makeWorkspaceContextValue({ planVersion, bumpPlanVersion: bump }),
)
const { result, rerender } = renderHook(() => usePlannedDays('child1'))
await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 0))
})
expect(result.current.isLoading).toBe(false)
const callCountAfterMount = vi.mocked(api.getPlannedDays).mock.calls.length
// The refetch that follows the mutation's bump should hang so the
// in-flight state is observable before it resolves.
let resolveRefetch: (value: ReturnType<typeof makePlannedLeaveDay>[]) => void = () => {}
vi.mocked(api.getPlannedDays).mockReturnValueOnce(
new Promise((resolve) => {
resolveRefetch = resolve
}),
)
await act(async () => {
// Stands in for a writer hook's post-mutation `bumpPlanVersion()`.
bump()
})
// The bump only takes effect on this instance's next render — nothing
// has refetched yet.
expect(vi.mocked(api.getPlannedDays).mock.calls.length).toBe(callCountAfterMount)
rerender()
await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 0))
})
// Exactly one refetch so far, and it's non-blocking.
expect(vi.mocked(api.getPlannedDays).mock.calls.length).toBe(callCountAfterMount + 1)
expect(result.current.isRefreshing).toBe(true)
expect(result.current.isLoading).toBe(false)
await act(async () => {
resolveRefetch([])
})
expect(result.current.isRefreshing).toBe(false)
expect(result.current.isLoading).toBe(false)
// Settling further (no further bump) raises no additional GET.
rerender()
await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 0))
})
expect(vi.mocked(api.getPlannedDays).mock.calls.length).toBe(callCountAfterMount + 1)
})
describe('M7 follow-up — the StrictMode double-mount hang', () => {
const strictModeWrapper = ({ children }: { children: ReactNode }) =>
createElement(StrictMode, null, children)
it('reproduces the reported hang: navigating to a page that mounts with childId already known must not strand isLoading true', async () => {
// React 18 StrictMode double-invokes a fresh mount's effect,
// back-to-back, before either invocation's fetch has resolved. On a
// real SPA navigation `childId` is already known at mount (unlike a
// hard reload, where it's still `undefined` on the very first
// render) — this is exactly what the bug report's "works on hard
// reload, hangs on in-app navigation" symptom traced back to.
vi.mocked(api.getPlannedDays).mockResolvedValue([])
const { result } = renderHook(() => usePlannedDays('child1'), {
wrapper: strictModeWrapper,
})
await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 0))
})
expect(result.current.isLoading).toBe(false)
expect(result.current.isRefreshing).toBe(false)
expect(result.current.data).toEqual([])
// The dedup fix means the double-invoked mount effect still issues
// exactly one real HTTP request, not two.
expect(api.getPlannedDays).toHaveBeenCalledTimes(1)
})
it('two overlapping calls for the same childId — one foreground, one background, started before either resolves — both end up cleared: isLoading false, isRefreshing false, exactly one request', async () => {
// The same hazard, exercised directly (per the coordinator's
// alternative to a StrictMode wrapper): two calls for the identical
// childId, racing, with DIFFERENT background flags — the exact shape
// that stranded `isLoading` before this fix (the stale call raised
// one flag, the current call raised the other, and only the current
// call's own — guarded — clear ever ran).
let resolveFetch: (value: ReturnType<typeof makePlannedLeaveDay>[]) => void = () => {}
vi.mocked(api.getPlannedDays).mockReturnValue(
new Promise((resolve) => {
resolveFetch = resolve
}),
)
const { result } = renderHook(() => usePlannedDays('child1'))
// The mount effect already started one (foreground) call — fire a
// second, differently-classified call for the SAME childId before it
// resolves.
let secondCall: Promise<unknown> = Promise.resolve()
act(() => {
secondCall = result.current.refresh({ background: true })
})
expect(result.current.isLoading).toBe(true)
await act(async () => {
resolveFetch([])
await secondCall
})
expect(result.current.isLoading).toBe(false)
expect(result.current.isRefreshing).toBe(false)
// Deduped into a single request.
expect(api.getPlannedDays).toHaveBeenCalledTimes(1)
})
})
describe('M7 part 2 — one trailing bump for a whole batch of writes', () => {
// The per-write `skipRefresh` option itself lives on the writer hook
// now (`useDayAssertions.assertDay` — see its tests); this covers the
// reader's side of that contract.
it('a batch of skipRefresh writes followed by one bumpPlanVersion() call (Calendar.tsx\'s multi-day save pattern) produces exactly one trailing background GET', async () => {
vi.mocked(api.getPlannedDays).mockResolvedValue([])
// A real counter (not this file's default inert `vi.fn()`) so calling
// `bump()` — standing in for `Calendar.tsx handleSavePinnedDay`'s
// trailing `bumpPlanVersion()` — then `rerender()`ing genuinely
// reproduces the real `WorkspaceProvider` state update that follows.
let planVersion = 0
const bump = () => {
planVersion += 1
}
vi.mocked(useWorkspace).mockImplementation(() =>
makeWorkspaceContextValue({ planVersion, bumpPlanVersion: bump }),
)
const { result, rerender } = renderHook(() => usePlannedDays('child1'))
await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 0))
})
const callCountAfterMount = vi.mocked(api.getPlannedDays).mock.calls.length
await act(async () => {
// Simulates Calendar.tsx's multi-day save loop: N writes through
// `useDayAssertions.assertDay(..., { skipRefresh: true })`, none of
// which touch this hook, then exactly ONE trailing bump for the
// whole batch (mirrors `handleSavePinnedDay`'s `finally`).
bump()
})
// The bump only takes effect on this instance's next render — nothing
// has refetched yet.
expect(vi.mocked(api.getPlannedDays).mock.calls.length).toBe(callCountAfterMount)
rerender()
await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 0))
})
// Exactly one trailing GET for the whole batch, and it never blocked.
expect(vi.mocked(api.getPlannedDays).mock.calls.length).toBe(callCountAfterMount + 1)
expect(result.current.isLoading).toBe(false)
expect(result.current.isRefreshing).toBe(false)
})
})
it('sets loading state during fetch (M7 part 1 — the initial mount load DOES block, unlike a post-mutation refetch)', async () => {
let resolveGetPlannedDays: (value: Awaited<ReturnType<typeof api.getPlannedDays>>) => void
const getPlannedDaysPromise: ReturnType<typeof api.getPlannedDays> = new Promise(resolve => {
resolveGetPlannedDays = resolve
@@ -126,7 +313,10 @@ describe('usePlannedDays', () => {
const { result } = renderHook(() => usePlannedDays('child1'))
// The very first fetch for a childId has nothing to render yet — it
// must block, unlike every subsequent mutation-triggered refetch.
expect(result.current.isLoading).toBe(true)
expect(result.current.isRefreshing).toBe(false)
await act(async () => {
resolveGetPlannedDays!([])
frontend/src/hooks/usePlannedDays.ts
+151 -34
View File
@@ -1,4 +1,4 @@
import { useState, useEffect, useCallback } from 'react'
import { useState, useEffect, useCallback, useRef } from 'react'
import * as api from '@/services/api'
import { useLatestRequest } from './useLatestRequest'
import { useWorkspace } from '@/context'
@@ -6,61 +6,178 @@ import type { PlannedLeaveDay } from '@/types'
interface UsePlannedDaysResult {
data: PlannedLeaveDay[] | null
/** True only while the initial (or childId-switch) load is in flight —
* safe to gate a full-page blocking spinner on. */
isLoading: boolean
/** True while a post-mutation background refresh is in flight. Never true
* at the same time as `isLoading`; a consumer that renders a blocking
* spinner should NOT key off this — it exists so a mutation's list refetch
* doesn't blank whatever is currently showing `data` (see `Calendar.tsx`'s
* `isLoading`, which only reads `isLoading`). Same shape as
* `useLeaveWishes.ts`'s `isRefreshing` — see its doc comment. */
isRefreshing: boolean
error: string | null
/** Refetches and returns the fresh list — for a caller that needs the
* just-fetched data synchronously rather than `data`, which is whatever
* this hook's `data` was at the closure's own render, frozen for that
* closure's lifetime, not a live view. */
refresh: () => Promise<PlannedLeaveDay[] | null>
* closure's lifetime, not a live view. `options.background` routes the
* call through `isRefreshing` instead of `isLoading` — pass it whenever
* the caller already has content on screen (e.g. a post-sync refresh)
* that a blocking spinner would otherwise unmount. */
refresh: (options?: { background?: boolean }) => Promise<PlannedLeaveDay[] | null>
}
/** `planned_leave_day` is purely derived output (restructure step 3b) — this
* hook is READ ONLY. Every write goes through `usePlanSync` (`POST
* /api/plan/sync(-with-wish)`) or `useDayAssertions` (`POST`/`DELETE
* /api/plan/sync(-with-wish)`) or `useDayAssertions` (`POST`/`DELETE`
* /api/day-assertions`), both of which bump the shared `planVersion` this
* hook watches to refetch — see the `useEffect` below.
*/
export function usePlannedDays(childId?: string): UsePlannedDaysResult {
const [data, setData] = useState<PlannedLeaveDay[] | null>(null)
const [isLoading, setIsLoading] = useState(false)
const [isRefreshing, setIsRefreshing] = useState(false)
const [error, setError] = useState<string | null>(null)
const { start, isCurrent } = useLatestRequest()
const { planVersion } = useWorkspace()
// Which childId `data`/`error` currently reflects — set only once a
// fetch's response actually lands (see `fetch`'s success/error branches),
// NEVER synchronously inside the effect below. That distinction is what
// keeps this hook's foreground/background classification consistent
// under React 18 StrictMode's double-invoked mount effect — see the
// effect's own comment.
const loadedForChildIdRef = useRef<string | undefined>(undefined)
// Dedupes concurrent calls for the same childId into a single HTTP
// request — see `fetch`'s own comment for why. Keyed by `requestId`
// (not the promise itself — a `const promise = (async () => { ...
// promise ... })()` self-reference is unassigned from TypeScript's
// point of view at the point the function body closes over it, even
// though it's only ever read long after the assignment completes).
const inFlightRef = useRef<{
childId: string
requestId: number
promise: Promise<PlannedLeaveDay[] | null>
} | null>(null)
const fetch = useCallback(async () => {
const requestId = start()
if (!childId) {
// The child this data belongs to is gone (e.g. a logout cleared the
// selection) — clear rather than leave the previous account's
// planned days rendered until a new childId arrives.
setData(null)
setError(null)
return null
}
setIsLoading(true)
setError(null)
try {
const response = await api.getPlannedDays(childId)
if (isCurrent(requestId)) setData(response)
return response
} catch (err) {
if (isCurrent(requestId)) {
setError(err instanceof Error ? err.message : 'Failed to fetch planned days')
const fetch = useCallback(
async (options?: { background?: boolean }): Promise<PlannedLeaveDay[] | null> => {
if (!childId) {
// The child this data belongs to is gone (e.g. a logout cleared the
// selection) — clear rather than leave the previous account's
// planned days rendered until a new childId arrives.
setData(null)
setError(null)
return null
}
return null
} finally {
if (isCurrent(requestId)) setIsLoading(false)
}
}, [childId, start, isCurrent])
// `planVersion` changing (from `usePlanSync`, `useDayAssertions`, or any
// other instance of this same hook elsewhere) is this hook's only signal
// to refetch — it never mutates on its own any more.
// Piggyback on an already-in-flight request for this exact childId
// instead of firing a second one. Without this, React 18 StrictMode's
// double-invoked mount effect (see the `planVersion` effect below)
// calls this twice, back-to-back, for the same childId before either
// has resolved — two real HTTP requests, and worse, the two calls
// could each independently (and inconsistently) decide whether to
// raise the blocking `isLoading` or the background `isRefreshing`
// flag. Deduping here removes that disagreement entirely: only the
// FIRST of an overlapping pair actually starts a request and decides
// which flag it raises; every other call for the same childId just
// awaits that same promise and raises nothing of its own.
const inFlight = inFlightRef.current
if (inFlight && inFlight.childId === childId) {
return inFlight.promise
}
const requestId = start()
const background = options?.background ?? false
const setBusy = background ? setIsRefreshing : setIsLoading
setBusy(true)
setError(null)
const promise = (async (): Promise<PlannedLeaveDay[] | null> => {
try {
const response = await api.getPlannedDays(childId)
if (isCurrent(requestId)) {
setData(response)
loadedForChildIdRef.current = childId
}
return response
} catch (err) {
if (isCurrent(requestId)) {
setError(err instanceof Error ? err.message : 'Failed to fetch planned days')
setData(null)
loadedForChildIdRef.current = childId
}
return null
} finally {
// Deliberately UNGUARDED — unlike `setData`/`setError` above, the
// busy flag this call raised must be cleared by this call
// regardless of whether `isCurrent(requestId)` is still true.
// `isCurrent` exists to decide which response's DATA wins; it
// must never also gate whether a superseded call's own spinner
// ever turns off. Guarding this the same way `setData` is guarded
// is exactly what stranded `isLoading` true forever the last time
// this hook was touched — a stale/losing call still owns its OWN
// busy flag and must always release it. (Dedup above already
// stops the specific StrictMode double-mount race that surfaced
// this; this asymmetry is the fix for the underlying hazard
// itself, which any other overlapping-call path could still hit.)
setBusy(false)
if (inFlightRef.current?.requestId === requestId) inFlightRef.current = null
}
})()
inFlightRef.current = { childId, requestId, promise }
return promise
},
[childId, start, isCurrent],
)
// The ONLY place this hook ever calls `fetch()` on a mutation's behalf —
// the writers (`usePlanSync`, `useDayAssertions`) never fetch this hook's
// data directly, only bump the shared `planVersion` counter. That is what
// M7 part 1 actually needs:
// this same effect already runs on mount/childId-change (a genuine first
// load, nothing to render yet — must block via `isLoading`) AND on every
// `planVersion` bump, from ANY instance (this one's own mutation, or a
// different `usePlannedDays` instance elsewhere, e.g. Dashboard's — must
// NOT block, `data` is already on screen). A prior version had the
// mutators call `fetch({ background: true })` themselves AND rely on this
// effect to re-fetch again on their own resulting bump — two fetches per
// mutation, and the second one (from this effect, with no options) used
// the *blocking* default, silently reintroducing the exact "blanks on
// every mutation" bug this hook exists to fix. Routing every refetch
// through this one effect makes "exactly one fetch per mutation" and
// "which one blocks" the same decision instead of two that can disagree.
//
// `loadedForChildIdRef` is how a single run tells those two cases apart:
// it's `undefined` until a fetch for this childId actually COMPLETES (set
// inside `fetch` itself, never here), and thereafter holds whichever
// `childId` that completed fetch was for. If the `childId` this render
// closed over doesn't match it, this run is a genuine new load (mount, or
// a real childId switch) — block via `isLoading`. If it matches,
// `childId` hasn't changed and data for it has already loaded at least
// once, so this run can only have been triggered by `planVersion`
// changing — background via `isRefreshing`, regardless of whose mutation
// (this instance's own, or another instance's) caused the bump.
//
// This effect's own body must NEVER be what decides "have we loaded this
// child" by writing to a ref synchronously here (an earlier version did,
// via a ref set at the top of this effect) — React 18 StrictMode
// double-invokes a fresh mount's effect, back-to-back, before either
// invocation's `fetch` has resolved. A ref written synchronously in the
// effect body would already hold the new `childId` by the time the SECOND
// invocation reads it, so invocation 1 would correctly classify itself as
// the first (foreground) load while invocation 2 — reading a ref
// invocation 1 had *already* overwritten a moment earlier — would
// misclassify itself as a background refresh of a page that had loaded
// nothing yet, permanently stranding `isLoading` true once invocation 1's
// now-superseded response landed (see `fetch`'s dedup comment for the
// rest of that story). Deriving `isNewChild` from a ref that only a
// completed *response* can move sidesteps this: neither StrictMode
// invocation has a response yet, so both compute the identical answer.
useEffect(() => {
fetch()
}, [fetch, planVersion])
const isNewChild = loadedForChildIdRef.current !== childId
void fetch({ background: !isNewChild })
}, [fetch, planVersion, childId])
return { data, isLoading, error, refresh: fetch }
return { data, isLoading, isRefreshing, error, refresh: fetch }
}
frontend/src/hooks/usePreschoolPlanningDays.ts
+6 -1
View File
@@ -68,7 +68,12 @@ export function usePreschoolPlanningDays(
if (!isCurrent(requestId)) return
setError(err instanceof Error ? err.message : 'Failed to load preschool planning days')
} finally {
if (isCurrent(requestId)) setBusy(false)
// Unguarded: `isCurrent` decides which response's DATA wins, not
// whether a superseded call's own busy flag gets cleared — a stale
// call must still release the flag IT raised, or it can strand
// that flag true forever (see usePlannedDays.ts for the hang this
// exact guard caused there).
setBusy(false)
}
},
[childId, year, start, isCurrent],
frontend/src/hooks/usePreschoolPlanningDaysList.ts
+6 -1
View File
@@ -64,7 +64,12 @@ export function usePreschoolPlanningDaysList(
if (!isCurrent(requestId)) return
setError(err instanceof Error ? err.message : 'Failed to load preschool planning days')
} finally {
if (isCurrent(requestId)) setBusy(false)
// Unguarded: `isCurrent` decides which response's DATA wins, not
// whether a superseded call's own busy flag gets cleared — a stale
// call must still release the flag IT raised, or it can strand
// that flag true forever (see usePlannedDays.ts for the hang this
// exact guard caused there).
setBusy(false)
}
},
[childId, start, isCurrent],
frontend/src/hooks/useVacationDays.ts
+6 -1
View File
@@ -54,7 +54,12 @@ export function useVacationDays(workspaceId: string): UseVacationDaysResult {
if (!isCurrent(requestId)) return
setError(err instanceof Error ? err.message : 'Failed to load vacation days')
} finally {
if (isCurrent(requestId)) setBusy(false)
// Unguarded: `isCurrent` decides which response's DATA wins, not
// whether a superseded call's own busy flag gets cleared — a stale
// call must still release the flag IT raised, or it can strand
// that flag true forever (see usePlannedDays.ts for the hang this
// exact guard caused there).
setBusy(false)
}
},
[workspaceId, start, isCurrent],
frontend/src/hooks/useWorkspaceMembers.ts
+6 -1
View File
@@ -30,7 +30,12 @@ export function useWorkspaceMembers(workspaceId: string | null): UseWorkspaceMem
if (!isCurrent(requestId)) return
setError(err instanceof Error ? err.message : 'Failed to load workspace members')
} finally {
if (isCurrent(requestId)) setIsLoading(false)
// Unguarded: `isCurrent` decides which response's DATA wins, not
// whether a superseded call's own busy flag gets cleared — a stale
// call must still release the flag IT raised, or it can strand that
// flag true forever (see usePlannedDays.ts for the hang this exact
// guard caused there).
setIsLoading(false)
}
}, [workspaceId, start, isCurrent])
frontend/src/pages/Calendar.test.tsx
+49 -3
View File
@@ -689,7 +689,9 @@ describe('Calendar', () => {
fireEvent.click(screen.getByTestId('modal-save-pinned'))
await waitFor(() =>
expect(assertDay).toHaveBeenCalledWith(expect.objectContaining({ dayType: 'vacation' })),
expect(assertDay).toHaveBeenCalledWith(expect.objectContaining({ dayType: 'vacation' }), {
skipRefresh: true,
}),
)
})
@@ -712,6 +714,7 @@ describe('Calendar', () => {
await waitFor(() =>
expect(assertDay).toHaveBeenCalledWith(
expect.objectContaining({ dayType: 'vacation', parentPersonalDataUuid: 'person-1', date: today }),
{ skipRefresh: true },
),
)
})
@@ -2984,9 +2987,49 @@ describe('Calendar', () => {
// All 3 dragged dates are asserted — never a LeaveWish, never a sync.
await waitFor(() => expect(assertDay).toHaveBeenCalledTimes(3))
expect(assertDay).toHaveBeenCalledWith(expect.objectContaining({ dayType: 'vacation' }))
expect(assertDay).toHaveBeenCalledWith(expect.objectContaining({ dayType: 'vacation' }), {
skipRefresh: true,
})
await waitFor(() => expect(screen.queryByTestId('modal')).not.toBeInTheDocument())
})
it('§M7 part 2 — issues N writes, every one skipping its own refetch/bump, plus exactly one trailing plan-version bump and one assertions refresh', async () => {
const assertDay = vi.fn().mockResolvedValue(makeDayAssertion())
const refresh = vi.fn().mockResolvedValue(undefined)
vi.mocked(useDayAssertions).mockReturnValue(makeDayAssertionsResult({ assertDay, refresh }))
render(<Calendar />)
const currentMonthDays = screen.getAllByTestId('calendar-day').filter((el) => !el.classList.contains('calendar-day--disabled'))
const startDay = currentMonthDays[0] as HTMLElement
// 4 dragged dates -> 4 assertions (assertDay is an upsert, so a
// pre-existing row needs no separate code path).
const endDay = currentMonthDays[3] as HTMLElement
fireEvent.pointerDown(startDay)
fireEvent.pointerEnter(endDay)
fireEvent.pointerUp(window)
const modal = await screen.findByTestId('modal')
expect(modal).toHaveAttribute('data-day-count', '4')
fireEvent.click(screen.getByTestId('modal-save-pinned'))
await waitFor(() => expect(screen.queryByTestId('modal')).not.toBeInTheDocument())
// N writes, every single one asking the hook to skip its own refetch/bump...
expect(assertDay).toHaveBeenCalledTimes(4)
expect(assertDay).toHaveBeenCalledWith(expect.objectContaining({ dayType: 'vacation' }), {
skipRefresh: true,
})
// ...but exactly one trailing plan-version bump for the whole batch —
// never a direct `plannedDaysHook.refresh()` call here (that would be
// a second, redundant GET on top of the one `usePlannedDays`' own
// `planVersion` effect already issues in response to this bump; see
// that hook's doc comment) — and one assertions-list refresh, whose
// data is not driven by `planVersion`.
expect(mockWorkspace.bumpPlanVersion).toHaveBeenCalledTimes(1)
expect(refresh).toHaveBeenCalledTimes(1)
expect(mockPlannedDays.refresh).not.toHaveBeenCalled()
})
})
describe('hand-pin path never lets a free day become vacation (§ free days never consume vacation)', () => {
@@ -3220,7 +3263,10 @@ describe('Calendar', () => {
// A's row must never be touched — assertDay is only ever called once,
// for B.
await waitFor(() =>
expect(assertDay).toHaveBeenCalledWith(expect.objectContaining({ parentPersonalDataUuid: 'pd-b', dayType: 'vacation' })),
expect(assertDay).toHaveBeenCalledWith(
expect.objectContaining({ parentPersonalDataUuid: 'pd-b', dayType: 'vacation' }),
{ skipRefresh: true },
),
)
expect(assertDay).toHaveBeenCalledTimes(1)
})
frontend/src/pages/Calendar.tsx
+44 -12
View File
@@ -570,7 +570,16 @@ export function Calendar(): ReactNode {
): Promise<PlanSyncResponse | undefined> => {
try {
const result = await syncHook.sync(workspaceId)
await Promise.all([plannedDaysHook.refresh(), calendarHook.refetch()])
// §M7 part 1 — a background refresh here so a sync triggered by a
// wish add/edit/delete elsewhere in this file doesn't blank the
// page either; `calendarHook.refetch()` has no such flag and keeps
// its pre-existing blocking behaviour (see its own doc comment).
// Unlike `handleSavePinnedDay`'s batch, this explicit call is NOT
// redundant with `usePlannedDays`' own `planVersion` effect:
// `syncPlan` deliberately never bumps `planVersion` (cycle safety —
// see `PlanSyncProvider.tsx`), so this is the only refetch a sync's
// day changes will ever get.
await Promise.all([plannedDaysHook.refresh({ background: true }), calendarHook.refetch()])
if (!options?.silent) {
const message = buildSyncToastMessage(
result,
@@ -840,17 +849,40 @@ export function Calendar(): ReactNode {
setLastLeaveType(payload.dayType)
for (const date of eligibleDates) {
// §Task7 — a hand-edit through the day popover asserts the day so a
// later `POST /api/plan/sync` never silently retypes it.
await assertionsHook.assertDay({
workspaceId: workspace.id,
childId: selectedChildId,
parentPersonalDataUuid: payload.parentId,
date,
dayType: payload.dayType,
isDubbeldag: payload.isDubbeldag,
})
// §M7 part 2 — a multi-day save otherwise issued 3 requests per date
// (write + the hook's own assertions refetch + a planned-days refetch
// from its bump) sequentially, and each of those planned-days refetches
// blocked the page. Each write below skips its own refetch/bump; the
// `finally` bumps exactly once for the whole batch, on both the
// success AND error path — a failure partway through must still
// invalidate so `data` doesn't go stale for the dates that DID save.
// That one bump is also the ONLY planned-days refetch: it reaches
// `usePlannedDays`' own `planVersion` effect (never a direct
// `plannedDaysHook.refresh()` call here) — see that hook's doc comment
// for why calling both was the M7-part-1 regression a browser
// verification pass caught (two GETs per save, the second blocking).
// `assertionsHook.refresh()` is not redundant with it: that hook's
// list isn't driven by `planVersion`. Writes stay sequential (never
// `Promise.all`) — the backend runs a single SQLite connection.
try {
for (const date of eligibleDates) {
// §Task7 — a hand-edit through the day popover asserts the day so a
// later `POST /api/plan/sync` never silently retypes it.
await assertionsHook.assertDay(
{
workspaceId: workspace.id,
childId: selectedChildId,
parentPersonalDataUuid: payload.parentId,
date,
dayType: payload.dayType,
isDubbeldag: payload.isDubbeldag,
},
{ skipRefresh: true },
)
}
} finally {
bumpPlanVersion()
await assertionsHook.refresh()
}
setIsModalOpen(false)
frontend/src/pages/DevFetch.test.tsx
+2
View File
@@ -23,6 +23,7 @@ describe('DevFetch', () => {
birth_date: '2022-01-15',
},
],
take_pl_on_red_days: false,
}
const emptyUserContext: api.UserContext = {
@@ -30,6 +31,7 @@ describe('DevFetch', () => {
personal_data_uuid: 'pd-test-456',
parent_name: 'Test Parent',
children: [],
take_pl_on_red_days: false,
}
const mockScraperFetchResult = {
frontend/src/services/api.ts
+4
View File
@@ -808,6 +808,10 @@ export interface UserContext {
workspace_id: string
parent_name: string
children: Array<{ id: string; name: string; birth_date: string }>
// Mirrors `RawWorkspace.take_pl_on_red_days` — added so `WorkspaceContext`
// can build its `workspace` state from this response alone on cold load,
// instead of following up with `GET /workspace/:id` (H12 step 1).
take_pl_on_red_days: boolean
}
export async function getUserContext(): Promise<UserContext> {
frontend/src/types/entities.ts
+12 -2
View File
@@ -3,8 +3,18 @@
export interface Workspace {
id: string
name: string
createdAt: string // ISO-8601
// `name`/`createdAt` are optional: since H12 step 1, the cold-load path
// (`WorkspaceContext.refreshContext`) builds `workspace` straight from
// `GET /auth/context`, which carries only `workspace_id` +
// `take_pl_on_red_days` — not the workspace's name or creation date. Both
// are still populated whenever a full `Workspace` is fetched directly
// (`getWorkspace`, `createWorkspace`, `updateWorkspace` — see `toWorkspace`
// in src/services/api.ts); nothing in `src/` reads either field off the
// cold-load `workspace` today, so leaving them unset there is safe. Kept
// on the type (rather than removed) so those direct-fetch call sites don't
// need touching.
name?: string
createdAt?: string // ISO-8601
// Optional (rather than required) so pre-existing test literals across the
// codebase that construct a `Workspace` by hand don't all need updating —
// a real API response always includes it (see `toWorkspace` in