Land a succeeded draft only on a plan read from the server
A redraft keeps the plan's id, so a plan the service worker answered from its cache could pass for the finished rebuild. The API client's apiRead now says whether a GET was the cached copy; the plan carries it, and the Draft board shows the retryable draft-unread error instead of a cached plan with the drafted id. A later read from the server still lands the draft. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -225,4 +225,14 @@ describe('network status', () => {
|
|||||||
await expect(api.apiFetch('/api/v1/recipes/01J9ZR')).resolves.toEqual({ id: '01J9ZR' })
|
await expect(api.apiFetch('/api/v1/recipes/01J9ZR')).resolves.toEqual({ id: '01J9ZR' })
|
||||||
expect(api.network.offline()).toBe(false)
|
expect(api.network.offline()).toBe(false)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it('says whether a read is the service worker\'s cached copy', async () => {
|
||||||
|
const cached = jsonResponse(200, { id: '01J9ZR' })
|
||||||
|
cached.headers.set(cacheFallbackHeader, '1')
|
||||||
|
stubFetch(cached, jsonResponse(200, { id: '01J9ZR' }))
|
||||||
|
const api = createApiClient(fakeLocation(), createNetworkStatus(window), createVerifiedSession(localStorage))
|
||||||
|
|
||||||
|
await expect(api.apiRead('/api/v1/recipes/01J9ZR')).resolves.toEqual({ body: { id: '01J9ZR' }, cached: true })
|
||||||
|
await expect(api.apiRead('/api/v1/recipes/01J9ZR')).resolves.toEqual({ body: { id: '01J9ZR' }, cached: false })
|
||||||
|
})
|
||||||
})
|
})
|
||||||
|
|||||||
+19
-1
@@ -43,11 +43,18 @@ export type ApiRequest = {
|
|||||||
body?: unknown
|
body?: unknown
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// ApiRead is a GET's decoded response body, and whether the service worker
|
||||||
|
// answered it from its cache because the server could not be reached in time.
|
||||||
|
export type ApiRead<T> = { body: T; cached: boolean }
|
||||||
|
|
||||||
export type ApiClient = {
|
export type ApiClient = {
|
||||||
// apiFetch sends a same-origin JSON request to path, which must be under
|
// apiFetch sends a same-origin JSON request to path, which must be under
|
||||||
// /api/v1/, and resolves with the decoded response body, or rejects with an
|
// /api/v1/, and resolves with the decoded response body, or rejects with an
|
||||||
// ApiError.
|
// ApiError.
|
||||||
apiFetch<T>(path: string, request?: ApiRequest): Promise<T>
|
apiFetch<T>(path: string, request?: ApiRequest): Promise<T>
|
||||||
|
// apiRead is apiFetch's GET that also says whether the body is the service
|
||||||
|
// worker's cached copy rather than the server's answer.
|
||||||
|
apiRead<T>(path: string): Promise<ApiRead<T>>
|
||||||
// loadMe fetches GET /api/v1/me and caches its CSRF token.
|
// loadMe fetches GET /api/v1/me and caches its CSRF token.
|
||||||
loadMe(): Promise<Me>
|
loadMe(): Promise<Me>
|
||||||
// forbidden reports whether a request was refused because the signed-in
|
// forbidden reports whether a request was refused because the signed-in
|
||||||
@@ -114,6 +121,16 @@ export function createApiClient(location: ApiLocation, network: NetworkStatus, s
|
|||||||
}
|
}
|
||||||
|
|
||||||
async function apiFetch<T>(path: string, request: ApiRequest = {}): Promise<T> {
|
async function apiFetch<T>(path: string, request: ApiRequest = {}): Promise<T> {
|
||||||
|
return (await exchange<T>(path, request)).body
|
||||||
|
}
|
||||||
|
|
||||||
|
function apiRead<T>(path: string): Promise<ApiRead<T>> {
|
||||||
|
return exchange<T>(path, {})
|
||||||
|
}
|
||||||
|
|
||||||
|
// exchange sends request to path and decodes the response, noting whether
|
||||||
|
// it is the service worker's cached copy.
|
||||||
|
async function exchange<T>(path: string, request: ApiRequest): Promise<ApiRead<T>> {
|
||||||
const method = request.method ?? 'GET'
|
const method = request.method ?? 'GET'
|
||||||
const url = apiURL(location.origin, method, path)
|
const url = apiURL(location.origin, method, path)
|
||||||
// fetch refuses a GET with a body by rejecting with a TypeError, the same
|
// fetch refuses a GET with a body by rejecting with a TypeError, the same
|
||||||
@@ -131,7 +148,7 @@ export function createApiClient(location: ApiLocation, network: NetworkStatus, s
|
|||||||
response = await send(method, url, request.body)
|
response = await send(method, url, request.body)
|
||||||
if (response.status === 403) throw await parseForbidden(method, path, response)
|
if (response.status === 403) throw await parseForbidden(method, path, response)
|
||||||
}
|
}
|
||||||
return decode<T>(method, path, response)
|
return { body: await decode<T>(method, path, response), cached: response.headers.has(cacheFallbackHeader) }
|
||||||
}
|
}
|
||||||
|
|
||||||
// parseForbidden reads a 403. Any API error other than csrf means the
|
// parseForbidden reads a 403. Any API error other than csrf means the
|
||||||
@@ -179,6 +196,7 @@ export function createApiClient(location: ApiLocation, network: NetworkStatus, s
|
|||||||
|
|
||||||
return {
|
return {
|
||||||
apiFetch,
|
apiFetch,
|
||||||
|
apiRead,
|
||||||
loadMe,
|
loadMe,
|
||||||
forbidden: () => forbidden,
|
forbidden: () => forbidden,
|
||||||
subscribe(listener) {
|
subscribe(listener) {
|
||||||
|
|||||||
@@ -1,5 +1,5 @@
|
|||||||
export { ApiProvider, useApi } from './ApiContext.tsx'
|
export { ApiProvider, useApi } from './ApiContext.tsx'
|
||||||
export { createApiClient, CSRF_HEADER, ME_PATH, type ApiClient, type ApiLocation, type ApiRequest, type Me } from './client.ts'
|
export { createApiClient, CSRF_HEADER, ME_PATH, type ApiClient, type ApiRead, type ApiLocation, type ApiRequest, type Me } from './client.ts'
|
||||||
export { ApiError } from './errors.ts'
|
export { ApiError } from './errors.ts'
|
||||||
export { createQueryClient, shouldRetry } from './queryClient.ts'
|
export { createQueryClient, shouldRetry } from './queryClient.ts'
|
||||||
export { createVerifiedSession, VERIFIED_SESSION_KEY, type VerifiedSession } from './verifiedSession.ts'
|
export { createVerifiedSession, VERIFIED_SESSION_KEY, type VerifiedSession } from './verifiedSession.ts'
|
||||||
|
|||||||
@@ -1,8 +1,9 @@
|
|||||||
import { QueryClientProvider } from '@tanstack/react-query'
|
import { QueryClientProvider } from '@tanstack/react-query'
|
||||||
import { fireEvent, render, screen, waitFor, within } from '@testing-library/react'
|
import { act, fireEvent, render, screen, waitFor, within } from '@testing-library/react'
|
||||||
import { afterEach, beforeEach, describe, expect, it, vi, type Mock } from 'vitest'
|
import { afterEach, beforeEach, describe, expect, it, vi, type Mock } from 'vitest'
|
||||||
import { ApiProvider, createApiClient, createQueryClient, createVerifiedSession } from '../api/index.ts'
|
import { ApiProvider, createApiClient, createQueryClient, createVerifiedSession } from '../api/index.ts'
|
||||||
import { createNetworkStatus } from '../pwa/networkStatus.ts'
|
import { createNetworkStatus } from '../pwa/networkStatus.ts'
|
||||||
|
import { cacheFallbackHeader } from '../sw/routes.ts'
|
||||||
import { HOUSEHOLD_SETTINGS } from '../test/ApiProviders.tsx'
|
import { HOUSEHOLD_SETTINGS } from '../test/ApiProviders.tsx'
|
||||||
import { declaredStyle } from '../test/cssRules.ts'
|
import { declaredStyle } from '../test/cssRules.ts'
|
||||||
import { errorResponse, fakeLocation, jsonResponse, ME } from '../test/fetchStub.ts'
|
import { errorResponse, fakeLocation, jsonResponse, ME } from '../test/fetchStub.ts'
|
||||||
@@ -323,6 +324,45 @@ describe('PlanPage', () => {
|
|||||||
expect(screen.queryByRole('alert')).not.toBeInTheDocument()
|
expect(screen.queryByRole('alert')).not.toBeInTheDocument()
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it("doesn't land a succeeded draft on the plan cached before it, and lands it once the server is read", async () => {
|
||||||
|
let plan = planBody('draft')
|
||||||
|
let cached = false
|
||||||
|
let job = draftJob('running')
|
||||||
|
stubServer({
|
||||||
|
[`GET ${THIS_WEEK}`]: () => {
|
||||||
|
const response = jsonResponse(200, plan)
|
||||||
|
if (cached) response.headers.set(cacheFallbackHeader, '1')
|
||||||
|
return response
|
||||||
|
},
|
||||||
|
[`POST ${DRAFT}`]: () => jsonResponse(202, { jobId: 'j1' }),
|
||||||
|
[`GET ${JOB}`]: () => jsonResponse(200, job),
|
||||||
|
})
|
||||||
|
await renderPage()
|
||||||
|
|
||||||
|
fireEvent.click(await screen.findByRole('button', { name: 'Rebuild week' }))
|
||||||
|
expect(await screen.findByRole('region', { name: 'Drafting this week' })).toBeInTheDocument()
|
||||||
|
// The draft succeeds, but the server can't be reached: the service
|
||||||
|
// worker answers with the plan cached before the rebuild, same id.
|
||||||
|
cached = true
|
||||||
|
job = draftJob('succeeded')
|
||||||
|
const alert = await screen.findByRole('alert', {}, { timeout: 3000 })
|
||||||
|
expect(alert).toHaveTextContent("Couldn't read the draft. The plan came from this device's cache, not the server")
|
||||||
|
expect(within(alert).getByRole('button', { name: 'Retry' })).toBeInTheDocument()
|
||||||
|
expect(screen.queryByText('Beef tacos')).not.toBeInTheDocument()
|
||||||
|
expect(screen.queryByTestId('skeleton-tile')).not.toBeInTheDocument()
|
||||||
|
|
||||||
|
// The server is back: the next read of the plan is the draft.
|
||||||
|
plan = planBody('draft', ['Lasagna', ...NAMES.slice(1)])
|
||||||
|
cached = false
|
||||||
|
vi.setSystemTime(new Date('2026-10-07T16:01:00Z'))
|
||||||
|
act(() => {
|
||||||
|
window.dispatchEvent(new Event('visibilitychange'))
|
||||||
|
})
|
||||||
|
expect(await screen.findByText('Lasagna')).toBeInTheDocument()
|
||||||
|
expect(screen.queryByRole('alert')).not.toBeInTheDocument()
|
||||||
|
expect(screen.queryByTestId('skeleton-tile')).not.toBeInTheDocument()
|
||||||
|
})
|
||||||
|
|
||||||
it('moves between weeks with the previous and next week buttons', async () => {
|
it('moves between weeks with the previous and next week buttons', async () => {
|
||||||
stubServer({
|
stubServer({
|
||||||
[`GET ${THIS_WEEK}`]: () => jsonResponse(200, planBody('draft')),
|
[`GET ${THIS_WEEK}`]: () => jsonResponse(200, planBody('draft')),
|
||||||
|
|||||||
@@ -115,15 +115,23 @@ function WeekBoard({ weekStart, timezone, onWeek }: { weekStart: string; timezon
|
|||||||
}
|
}
|
||||||
}, [queryClient, ended, jobId, weekStart])
|
}, [queryClient, ended, jobId, weekStart])
|
||||||
|
|
||||||
// The plan read shows once it is the one drafted; no plan, or another
|
// The plan read shows once it is the one drafted, read from the server; no
|
||||||
// one, is an error rather than an empty or stale board.
|
// plan, another one, or a copy cached before the draft (a redraft keeps the
|
||||||
|
// plan's id) is an error rather than an empty or stale board. A later read
|
||||||
|
// from the server that is the draft still lands it.
|
||||||
const { draft } = result
|
const { draft } = result
|
||||||
const read = draft !== null && readJobId === jobId && plan.isSuccess
|
const read = draft !== null && readJobId === jobId && plan.isSuccess
|
||||||
const landed = read && plan.data?.id === draft.plan_id
|
const landed = read && plan.data?.id === draft.plan_id && !plan.data.cached
|
||||||
const resultError =
|
const resultError =
|
||||||
result.error ??
|
result.error ??
|
||||||
(read && !landed
|
(read && !landed
|
||||||
? new Error(plan.data === null ? `The week has no plan, not draft ${draft.plan_id}.` : `The week's plan is not draft ${draft.plan_id}.`)
|
? new Error(
|
||||||
|
plan.data === null
|
||||||
|
? `The week has no plan, not draft ${draft.plan_id}.`
|
||||||
|
: plan.data.id !== draft.plan_id
|
||||||
|
? `The week's plan is not draft ${draft.plan_id}.`
|
||||||
|
: `The plan came from this device's cache, not the server, so it may not be draft ${draft.plan_id} yet.`,
|
||||||
|
)
|
||||||
: null)
|
: null)
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
if (landed) setJobId(null)
|
if (landed) setJobId(null)
|
||||||
@@ -168,6 +176,9 @@ function WeekBoard({ weekStart, timezone, onWeek }: { weekStart: string; timezon
|
|||||||
body = <ErrorBanner onRetry={() => void plan.refetch()}>Couldn't load this week's plan. {plan.error.message}</ErrorBanner>
|
body = <ErrorBanner onRetry={() => void plan.refetch()}>Couldn't load this week's plan. {plan.error.message}</ErrorBanner>
|
||||||
} else if (drafting) {
|
} else if (drafting) {
|
||||||
body = <SkeletonTiles label="Drafting this week">Drafting your week…</SkeletonTiles>
|
body = <SkeletonTiles label="Drafting this week">Drafting your week…</SkeletonTiles>
|
||||||
|
} else if (resultError !== null) {
|
||||||
|
// An unread draft shows no board: the plan read is not the draft.
|
||||||
|
body = null
|
||||||
} else if (plan.isPending) {
|
} else if (plan.isPending) {
|
||||||
body = <SkeletonTiles label="Plan loading" />
|
body = <SkeletonTiles label="Plan loading" />
|
||||||
} else if (plan.data === null) {
|
} else if (plan.data === null) {
|
||||||
|
|||||||
+10
-7
@@ -1,5 +1,5 @@
|
|||||||
import { useMutation, useQuery } from '@tanstack/react-query'
|
import { useMutation, useQuery } from '@tanstack/react-query'
|
||||||
import { ApiError, useApi } from '../api/index.ts'
|
import { ApiError, useApi, type ApiRead } from '../api/index.ts'
|
||||||
import { isRecord } from '../api/errors.ts'
|
import { isRecord } from '../api/errors.ts'
|
||||||
import { oneOf } from '../dinners/api.ts'
|
import { oneOf } from '../dinners/api.ts'
|
||||||
|
|
||||||
@@ -32,13 +32,15 @@ export type PlanDay = {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Plan is GET /api/v1/plans/{weekStart}: the week's seven days, Monday to
|
// Plan is GET /api/v1/plans/{weekStart}: the week's seven days, Monday to
|
||||||
// Sunday, and the AI's note on the draft, if any.
|
// Sunday, and the AI's note on the draft, if any. cached says the service
|
||||||
|
// worker answered from its cache: the server may have changed the plan since.
|
||||||
export type Plan = {
|
export type Plan = {
|
||||||
id: string
|
id: string
|
||||||
week_start: string
|
week_start: string
|
||||||
status: PlanStatus
|
status: PlanStatus
|
||||||
ai_note: string | null
|
ai_note: string | null
|
||||||
days: PlanDay[]
|
days: PlanDay[]
|
||||||
|
cached: boolean
|
||||||
}
|
}
|
||||||
|
|
||||||
// DRAFT_JOB_KIND is the kind of the job drafting a week's plan.
|
// DRAFT_JOB_KIND is the kind of the job drafting a week's plan.
|
||||||
@@ -62,7 +64,7 @@ function parseDay(where: string, body: unknown): PlanDay {
|
|||||||
return { date: body.date, dinner: { id: body.dinner.id, name: body.dinner.name }, is_new: body.is_new, start_by: body.start_by }
|
return { date: body.date, dinner: { id: body.dinner.id, name: body.dinner.name }, is_new: body.is_new, start_by: body.start_by }
|
||||||
}
|
}
|
||||||
|
|
||||||
function parsePlan(where: string, weekStart: string, body: unknown): Plan {
|
function parsePlan(where: string, weekStart: string, body: unknown, cached: boolean): Plan {
|
||||||
if (
|
if (
|
||||||
!isRecord(body) ||
|
!isRecord(body) ||
|
||||||
typeof body.id !== 'string' ||
|
typeof body.id !== 'string' ||
|
||||||
@@ -79,6 +81,7 @@ function parsePlan(where: string, weekStart: string, body: unknown): Plan {
|
|||||||
status: oneOf(where, 'status', PLAN_STATUSES, body.status),
|
status: oneOf(where, 'status', PLAN_STATUSES, body.status),
|
||||||
ai_note: body.ai_note,
|
ai_note: body.ai_note,
|
||||||
days: body.days.map((day) => parseDay(where, day)),
|
days: body.days.map((day) => parseDay(where, day)),
|
||||||
|
cached,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -94,19 +97,19 @@ export function parseDraftResult(where: string, weekStart: string, body: unknown
|
|||||||
// usePlan is the plan of the week starting on Monday weekStart (GET
|
// usePlan is the plan of the week starting on Monday weekStart (GET
|
||||||
// /api/v1/plans/{weekStart}), or null when the week has none (404).
|
// /api/v1/plans/{weekStart}), or null when the week has none (404).
|
||||||
export function usePlan(weekStart: string) {
|
export function usePlan(weekStart: string) {
|
||||||
const { apiFetch } = useApi()
|
const { apiRead } = useApi()
|
||||||
return useQuery({
|
return useQuery({
|
||||||
queryKey: planKey(weekStart),
|
queryKey: planKey(weekStart),
|
||||||
queryFn: async (): Promise<Plan | null> => {
|
queryFn: async (): Promise<Plan | null> => {
|
||||||
const path = planPath(weekStart)
|
const path = planPath(weekStart)
|
||||||
let body: unknown
|
let read: ApiRead<unknown>
|
||||||
try {
|
try {
|
||||||
body = await apiFetch<unknown>(path)
|
read = await apiRead<unknown>(path)
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
if (error instanceof ApiError && error.status === 404) return null
|
if (error instanceof ApiError && error.status === 404) return null
|
||||||
throw error
|
throw error
|
||||||
}
|
}
|
||||||
return parsePlan(`GET ${path}`, weekStart, body)
|
return parsePlan(`GET ${path}`, weekStart, read.body, read.cached)
|
||||||
},
|
},
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user