From d6404edefcb6e0b5a0c807acf663ec6414008e79 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Mon, 13 Jul 2026 21:01:46 -0400 Subject: [PATCH] feat(contracts+web): thread concurrency tokens and 409 conflict handling through the domain layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 6b of the SHOC-alignment plan — client side of the optimistic-concurrency wire contract shipped in 6a. shared/api-contracts (additive): - rowVersion: string on ProposalListItem, ProposalDetail, and LineItem - proposalVersion?: string on UpdateProposalRequest and BulkUpdateLineItemsRequest; new ProposalVersionRequest - ConcurrencyConflict { message, currentState } — the SHOC ADR 0004 409 envelope (guarded path; the unguarded fallback carries no state) - matching zod schemas, all kept under the satisfies z.ZodType coupling web: - lib/api/errors.ts: ConflictError carrying the server's reloaded currentState; client.ts interceptor throws it on 409 (WEB-M2 401 handling untouched) - guarded mutations read the token from the cached proposal detail at mutate time; the save flow chains rotated tokens (PUT response token into the bulk replace) and ends with a detail refetch so approve-after-save never sends a stale version - 409 recovery in the admin use-cases: write currentState into the detail cache, invalidateProposalViews() (stale-queue invariant holds on the failure path too), and toast the conflict instead of the generic failure message - e2e smoke mock payloads carry rowVersion; vitest coverage for the interceptor ConflictError paths, token threading/rotation, and 409 cache recovery Verified: shared typecheck, web tsc/vitest (69)/build/prettier/Playwright smoke, mobile tsc (create-only, no changes needed). --- shared/api-contracts/src/index.ts | 28 ++++ shared/api-contracts/src/schemas.ts | 19 +++ web/e2e/smoke.spec.ts | 2 + .../domain/__tests__/admin.use-cases.test.tsx | 120 ++++++++++++++++-- .../__tests__/lineItems.use-cases.test.tsx | 35 ++++- web/src/domain/admin/api.ts | 30 +++-- web/src/domain/admin/use-cases.ts | 82 ++++++++++-- web/src/domain/lineItems/api.ts | 14 +- web/src/lib/api/__tests__/client.test.ts | 59 +++++++++ web/src/lib/api/client.ts | 14 ++ web/src/lib/api/errors.ts | 19 +++ 11 files changed, 386 insertions(+), 36 deletions(-) create mode 100644 web/src/lib/api/errors.ts diff --git a/shared/api-contracts/src/index.ts b/shared/api-contracts/src/index.ts index f189eb2..2c07502 100644 --- a/shared/api-contracts/src/index.ts +++ b/shared/api-contracts/src/index.ts @@ -34,6 +34,8 @@ export interface ProposalListItem { submittedAt: string; submittedByName: string | null; assignedAdminName: string | null; + /** Opaque optimistic-concurrency token (base64); echo back untouched. */ + rowVersion: string; } export interface ProposalDetail { @@ -62,6 +64,8 @@ export interface ProposalDetail { parentProposalId: string | null; createdAt: string; updatedAt: string; + /** Opaque optimistic-concurrency token (base64); echo back untouched. */ + rowVersion: string; } export interface CreateProposalRequest { @@ -81,6 +85,16 @@ export interface UpdateProposalRequest { poNumber?: string; workOrderNumber?: string; assignedAdminId?: string; + /** Expected proposal rowVersion — REQUIRED by the server (422 ProposalVersionRequired when missing). */ + proposalVersion?: string; +} + +/** Body for the state-transition endpoints (approve, return-to-review, send, + * revise): the expected proposal rowVersion. The server rejects a missing + * token with 422 {code:"ProposalVersionRequired"} and a malformed one with + * 422 {code:"InvalidRowVersion"}. */ +export interface ProposalVersionRequest { + proposalVersion?: string; } export interface ProposalFilters { @@ -116,6 +130,8 @@ export interface LineItem { source: LineItemSource; createdAt: string; updatedAt: string; + /** Opaque optimistic-concurrency token (base64); echo back untouched. */ + rowVersion: string; } export interface CreateLineItemRequest { @@ -143,6 +159,8 @@ export interface UpdateLineItemEntry { export interface BulkUpdateLineItemsRequest { lineItems: UpdateLineItemEntry[]; + /** Expected proposal rowVersion — the bulk replace is guarded by the PROPOSAL token. */ + proposalVersion?: string; } // ── Customers (CustomerDtos.cs) ─────────────────────────────────────────── @@ -285,3 +303,13 @@ export interface ApiProblem { detail: string; code: ApiProblemCode; } + +// Optimistic-concurrency conflict envelope (SHOC ADR 0004 — NOT ProblemDetails): +// guarded writes that lose a race return 409 with the reloaded currentState so +// clients can refresh immediately. The unguarded-race safety net returns +// 409 { status: "Conflict", message, code: 409 } instead — same `message` +// field, no currentState. +export interface ConcurrencyConflict { + message: string; + currentState: T | null; +} diff --git a/shared/api-contracts/src/schemas.ts b/shared/api-contracts/src/schemas.ts index 6929c86..0de0d7f 100644 --- a/shared/api-contracts/src/schemas.ts +++ b/shared/api-contracts/src/schemas.ts @@ -14,6 +14,8 @@ import type { ProposalDetail, CreateProposalRequest, UpdateProposalRequest, + ProposalVersionRequest, + ConcurrencyConflict, LineItem, CreateLineItemRequest, UpdateLineItemEntry, @@ -56,6 +58,7 @@ export const proposalListItemSchema = z.object({ submittedAt: z.string(), submittedByName: z.string().nullable(), assignedAdminName: z.string().nullable(), + rowVersion: z.string(), }) satisfies z.ZodType; export const proposalDetailSchema = z.object({ @@ -84,6 +87,7 @@ export const proposalDetailSchema = z.object({ parentProposalId: z.string().nullable(), createdAt: z.string(), updatedAt: z.string(), + rowVersion: z.string(), }) satisfies z.ZodType; export const createProposalRequestSchema = z.object({ @@ -103,8 +107,13 @@ export const updateProposalRequestSchema = z.object({ poNumber: z.string().optional(), workOrderNumber: z.string().optional(), assignedAdminId: z.string().optional(), + proposalVersion: z.string().optional(), }) satisfies z.ZodType; +export const proposalVersionRequestSchema = z.object({ + proposalVersion: z.string().optional(), +}) satisfies z.ZodType; + export const proposalStatsSchema = z.object({ totalCount: z.number(), inReviewCount: z.number(), @@ -126,6 +135,7 @@ export const lineItemSchema = z.object({ source: lineItemSourceSchema, createdAt: z.string(), updatedAt: z.string(), + rowVersion: z.string(), }) satisfies z.ZodType; export const createLineItemRequestSchema = z.object({ @@ -153,6 +163,7 @@ export const updateLineItemEntrySchema = z.object({ export const bulkUpdateLineItemsRequestSchema = z.object({ lineItems: z.array(updateLineItemEntrySchema), + proposalVersion: z.string().optional(), }) satisfies z.ZodType; // ── Customers ───────────────────────────────────────────────────────────── @@ -274,3 +285,11 @@ export const apiProblemSchema = z.object({ detail: z.string(), code: z.string(), }) satisfies z.ZodType; + +// 409 concurrency-conflict envelope. The proposal aggregate is the only +// guarded resource today, so the concrete schema is coupled to ProposalDetail; +// build others via the same pattern when new aggregates gain guards. +export const proposalConcurrencyConflictSchema = z.object({ + message: z.string(), + currentState: proposalDetailSchema.nullable(), +}) satisfies z.ZodType>; diff --git a/web/e2e/smoke.spec.ts b/web/e2e/smoke.spec.ts index 7fa5f1d..4a81900 100644 --- a/web/e2e/smoke.spec.ts +++ b/web/e2e/smoke.spec.ts @@ -38,6 +38,7 @@ const proposals = [ submittedAt: '2026-07-01T12:00:00Z', submittedByName: 'Adam Moussa', assignedAdminName: null, + rowVersion: 'AAAAAAAAAAE=', }, { id: 'p-2', @@ -51,6 +52,7 @@ const proposals = [ submittedAt: '2026-07-05T09:30:00Z', submittedByName: 'Adam Moussa', assignedAdminName: 'Sarah Chen', + rowVersion: 'AAAAAAAAAAI=', }, ]; diff --git a/web/src/domain/__tests__/admin.use-cases.test.tsx b/web/src/domain/__tests__/admin.use-cases.test.tsx index 50a7ab3..c888143 100644 --- a/web/src/domain/__tests__/admin.use-cases.test.tsx +++ b/web/src/domain/__tests__/admin.use-cases.test.tsx @@ -1,14 +1,20 @@ // Admin domain use-case hooks: state transitions must cross-domain -// invalidate the proposals/lineItems keys (domain README rule 3). The domain -// api modules are mocked — no axios traffic. +// invalidate the proposals/lineItems keys (domain README rule 3), thread the +// optimistic-concurrency token from the cached detail into guarded mutations, +// and recover from 409 conflicts by refreshing from the server's currentState. +// The domain api modules are mocked — no axios traffic. import { describe, it, expect, vi, beforeEach } from 'vitest'; import { renderHook, waitFor } from '@testing-library/react'; import { toast } from 'react-toastify'; import { createQueryHarness } from './hookTestUtils'; -import { useApproveProposal } from '../admin/use-cases'; +import { useApproveProposal, useSaveProposalWorkspace } from '../admin/use-cases'; import { proposalsKeys } from '../proposals/use-cases'; import { lineItemsKeys } from '../lineItems/use-cases'; import { adminApi } from '../admin/api'; +import { lineItemsApi } from '../lineItems/api'; +import { proposalsApi } from '../proposals/api'; +import { ConflictError } from '../../lib/api/errors'; +import type { ProposalDetail } from '../proposals/types'; vi.mock('../admin/api', () => ({ adminApi: { @@ -26,8 +32,8 @@ vi.mock('../admin/api', () => ({ }, })); -// useSaveProposalWorkspace pulls lineItemsApi directly; mock it so no test -// path can reach axios. +// useSaveProposalWorkspace pulls lineItemsApi and proposalsApi directly; +// mock both so no test path can reach axios. vi.mock('../lineItems/api', () => ({ lineItemsApi: { getAll: vi.fn(), @@ -37,27 +43,51 @@ vi.mock('../lineItems/api', () => ({ }, })); +vi.mock('../proposals/api', () => ({ + proposalsApi: { + create: vi.fn(), + getAll: vi.fn(), + getById: vi.fn(), + uploadAttachment: vi.fn(), + confirmUpload: vi.fn(), + getStats: vi.fn(), + getVendors: vi.fn(), + }, +})); + vi.mock('react-toastify', () => ({ toast: { success: vi.fn(), error: vi.fn(), warning: vi.fn(), info: vi.fn() }, })); const PROPOSAL_ID = 'p-7'; +const detail = (rowVersion: string): ProposalDetail => + ({ id: PROPOSAL_ID, rowVersion }) as unknown as ProposalDetail; + beforeEach(() => { vi.clearAllMocks(); }); describe('useApproveProposal', () => { - it('invalidates the proposal detail and its line items keys and toasts on success', async () => { - vi.mocked(adminApi.approveProposal).mockResolvedValue(undefined); + it('sends the cached rowVersion token, invalidates the proposal detail and its line items keys, and toasts on success', async () => { + vi.mocked(adminApi.approveProposal).mockResolvedValue(detail('AAAAAAAAAAM=')); const { queryClient, wrapper } = createQueryHarness(); + // The workspace always fetches the detail before actions are possible — + // the guarded mutation reads its token from this cache at mutate time. + queryClient.setQueryData(proposalsKeys.detail(PROPOSAL_ID), detail('AAAAAAAAAAI=')); const invalidateSpy = vi.spyOn(queryClient, 'invalidateQueries'); const { result } = renderHook(() => useApproveProposal(PROPOSAL_ID), { wrapper }); result.current.mutate(); await waitFor(() => expect(result.current.isSuccess).toBe(true)); - expect(adminApi.approveProposal).toHaveBeenCalledWith(PROPOSAL_ID); + expect(adminApi.approveProposal).toHaveBeenCalledWith(PROPOSAL_ID, 'AAAAAAAAAAI='); + // The response carries the rotated token — it must land in the detail + // cache immediately so a follow-on guarded action doesn't race the + // invalidation refetch. + expect(queryClient.getQueryData(proposalsKeys.detail(PROPOSAL_ID))).toEqual( + detail('AAAAAAAAAAM='), + ); // Approval changes proposal status AND locks/reprices line items — both // domains' keys must be refetched (cross-domain invalidation, rule 3). expect(invalidateSpy).toHaveBeenCalledWith({ @@ -94,4 +124,78 @@ describe('useApproveProposal', () => { expect(invalidateSpy).not.toHaveBeenCalled(); expect(toast.success).not.toHaveBeenCalled(); }); + + it('on 409 writes the server currentState into the detail cache, refreshes views, and toasts the conflict', async () => { + const currentState = detail('AAAAAAAAAAo='); + vi.mocked(adminApi.approveProposal).mockRejectedValue( + new ConflictError('Proposal was modified by another user.', currentState), + ); + const { queryClient, wrapper } = createQueryHarness(); + queryClient.setQueryData(proposalsKeys.detail(PROPOSAL_ID), detail('AAAAAAAAAAI=')); + const invalidateSpy = vi.spyOn(queryClient, 'invalidateQueries'); + + const { result } = renderHook(() => useApproveProposal(PROPOSAL_ID), { wrapper }); + result.current.mutate(); + + await waitFor(() => expect(result.current.isError).toBe(true)); + // Refresh-and-retry semantics: the stale detail is replaced by the + // server's reloaded state (fresh token included)... + expect(queryClient.getQueryData(proposalsKeys.detail(PROPOSAL_ID))).toEqual(currentState); + // ...every proposal view is refetched (stale-queue invariant holds even + // on the failure path)... + expect(invalidateSpy).toHaveBeenCalledWith({ queryKey: proposalsKeys.detail(PROPOSAL_ID) }); + expect(invalidateSpy).toHaveBeenCalledWith({ + queryKey: lineItemsKeys.byProposal(PROPOSAL_ID), + }); + expect(invalidateSpy).toHaveBeenCalledWith({ queryKey: proposalsKeys.lists() }); + // ...and the user gets the conflict message, not the generic failure toast. + expect(toast.warning).toHaveBeenCalledWith('Proposal was modified by another user.'); + expect(toast.error).not.toHaveBeenCalled(); + }); +}); + +describe('useSaveProposalWorkspace', () => { + it('rotates the token across the guarded pair and refetches the detail for the fresh token', async () => { + vi.mocked(adminApi.updateProposal).mockResolvedValue(detail('AAAAAAAAAAM=')); + vi.mocked(lineItemsApi.bulkUpdate).mockResolvedValue([]); + vi.mocked(proposalsApi.getById).mockResolvedValue(detail('AAAAAAAAAAQ=')); + const { queryClient, wrapper } = createQueryHarness(); + queryClient.setQueryData(proposalsKeys.detail(PROPOSAL_ID), detail('AAAAAAAAAAI=')); + + const { result } = renderHook(() => useSaveProposalWorkspace(PROPOSAL_ID), { wrapper }); + result.current.mutate({ refinedScope: 'refined', lineItems: [] }); + + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + // PUT sends the token the workspace loaded with... + expect(adminApi.updateProposal).toHaveBeenCalledWith(PROPOSAL_ID, { + refinedScope: 'refined', + proposalVersion: 'AAAAAAAAAAI=', + }); + // ...the bulk replace sends the token the PUT rotated to... + expect(lineItemsApi.bulkUpdate).toHaveBeenCalledWith(PROPOSAL_ID, [], 'AAAAAAAAAAM='); + // ...and the detail cache ends on the post-bulk token so approve-after-save + // never sends a stale version. + expect(queryClient.getQueryData(proposalsKeys.detail(PROPOSAL_ID))).toEqual( + detail('AAAAAAAAAAQ='), + ); + expect(toast.success).toHaveBeenCalledWith('Changes saved'); + }); + + it('on 409 refreshes from currentState and toasts the conflict instead of the save-failed toast', async () => { + const currentState = detail('AAAAAAAAAAo='); + vi.mocked(adminApi.updateProposal).mockRejectedValue( + new ConflictError('Proposal was modified by another user.', currentState), + ); + const { queryClient, wrapper } = createQueryHarness(); + queryClient.setQueryData(proposalsKeys.detail(PROPOSAL_ID), detail('AAAAAAAAAAI=')); + + const { result } = renderHook(() => useSaveProposalWorkspace(PROPOSAL_ID), { wrapper }); + result.current.mutate({ refinedScope: 'refined', lineItems: [] }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + expect(lineItemsApi.bulkUpdate).not.toHaveBeenCalled(); + expect(queryClient.getQueryData(proposalsKeys.detail(PROPOSAL_ID))).toEqual(currentState); + expect(toast.warning).toHaveBeenCalledWith('Proposal was modified by another user.'); + expect(toast.error).not.toHaveBeenCalled(); + }); }); diff --git a/web/src/domain/__tests__/lineItems.use-cases.test.tsx b/web/src/domain/__tests__/lineItems.use-cases.test.tsx index 9cde471..5ffe757 100644 --- a/web/src/domain/__tests__/lineItems.use-cases.test.tsx +++ b/web/src/domain/__tests__/lineItems.use-cases.test.tsx @@ -11,7 +11,9 @@ import { useSaveProposalWorkspace } from '../admin/use-cases'; import { proposalsKeys } from '../proposals/use-cases'; import { adminApi } from '../admin/api'; import { lineItemsApi } from '../lineItems/api'; +import { proposalsApi } from '../proposals/api'; import type { UpdateLineItemEntry } from '../lineItems/types'; +import type { ProposalDetail } from '../proposals/types'; vi.mock('../admin/api', () => ({ adminApi: { @@ -38,6 +40,19 @@ vi.mock('../lineItems/api', () => ({ }, })); +// The save flow ends with a detail refetch (post-bulk token rotation). +vi.mock('../proposals/api', () => ({ + proposalsApi: { + create: vi.fn(), + getAll: vi.fn(), + getById: vi.fn(), + uploadAttachment: vi.fn(), + confirmUpload: vi.fn(), + getStats: vi.fn(), + getVendors: vi.fn(), + }, +})); + vi.mock('react-toastify', () => ({ toast: { success: vi.fn(), error: vi.fn(), warning: vi.fn(), info: vi.fn() }, })); @@ -47,6 +62,9 @@ const entries = [ { description: 'Labor', quantity: 2, unitPrice: 150 }, ] as unknown as UpdateLineItemEntry[]; +const detail = (rowVersion: string): ProposalDetail => + ({ id: PROPOSAL_ID, rowVersion }) as unknown as ProposalDetail; + beforeEach(() => { vi.clearAllMocks(); }); @@ -60,17 +78,24 @@ describe('lineItemsKeys', () => { describe('useSaveProposalWorkspace', () => { it('persists scope + entries and invalidates line items, detail, and list/stats views', async () => { - vi.mocked(adminApi.updateProposal).mockResolvedValue(undefined); + vi.mocked(adminApi.updateProposal).mockResolvedValue(detail('v2')); vi.mocked(lineItemsApi.bulkUpdate).mockResolvedValue([]); + vi.mocked(proposalsApi.getById).mockResolvedValue(detail('v3')); const { queryClient, wrapper } = createQueryHarness(); + queryClient.setQueryData(proposalsKeys.detail(PROPOSAL_ID), detail('v1')); const invalidateSpy = vi.spyOn(queryClient, 'invalidateQueries'); const { result } = renderHook(() => useSaveProposalWorkspace(PROPOSAL_ID), { wrapper }); result.current.mutate({ refinedScope: 'refined', lineItems: entries }); await waitFor(() => expect(result.current.isSuccess).toBe(true)); - expect(adminApi.updateProposal).toHaveBeenCalledWith(PROPOSAL_ID, { refinedScope: 'refined' }); - expect(lineItemsApi.bulkUpdate).toHaveBeenCalledWith(PROPOSAL_ID, entries); + // Token rotation: the PUT sends the cached token, the bulk replace sends + // the PUT response's rotated token. + expect(adminApi.updateProposal).toHaveBeenCalledWith(PROPOSAL_ID, { + refinedScope: 'refined', + proposalVersion: 'v1', + }); + expect(lineItemsApi.bulkUpdate).toHaveBeenCalledWith(PROPOSAL_ID, entries, 'v2'); expect(invalidateSpy).toHaveBeenCalledWith({ queryKey: lineItemsKeys.byProposal(PROPOSAL_ID), }); @@ -85,7 +110,7 @@ describe('useSaveProposalWorkspace', () => { }); it('toasts a save failure and skips invalidation', async () => { - vi.mocked(adminApi.updateProposal).mockRejectedValue(new Error('409 conflict')); + vi.mocked(adminApi.updateProposal).mockRejectedValue(new Error('validation failed')); const { queryClient, wrapper } = createQueryHarness(); const invalidateSpy = vi.spyOn(queryClient, 'invalidateQueries'); @@ -93,7 +118,7 @@ describe('useSaveProposalWorkspace', () => { result.current.mutate({ refinedScope: 'refined', lineItems: entries }); await waitFor(() => expect(result.current.isError).toBe(true)); - expect(toast.error).toHaveBeenCalledWith('Save failed: 409 conflict'); + expect(toast.error).toHaveBeenCalledWith('Save failed: validation failed'); expect(lineItemsApi.bulkUpdate).not.toHaveBeenCalled(); expect(invalidateSpy).not.toHaveBeenCalled(); }); diff --git a/web/src/domain/admin/api.ts b/web/src/domain/admin/api.ts index 472f339..90728ca 100644 --- a/web/src/domain/admin/api.ts +++ b/web/src/domain/admin/api.ts @@ -9,24 +9,34 @@ export const adminApi = { return res.data; }, - updateProposal: async (id: string, data: UpdateProposalRequest): Promise => { - await apiClient.put(`/proposals/${id}`, data); + // Guarded mutations (optimistic concurrency): every write below requires the + // proposal's rowVersion token and returns the fresh proposal state — the + // server rotates the token on each guarded write, so callers chain from the + // RESPONSE's rowVersion, never the one they started with. + updateProposal: async (id: string, data: UpdateProposalRequest): Promise => { + const res = await apiClient.put(`/proposals/${id}`, data); + return res.data; }, - approveProposal: async (id: string): Promise => { - await apiClient.post(`/proposals/${id}/approve`); + approveProposal: async (id: string, proposalVersion?: string): Promise => { + const res = await apiClient.post(`/proposals/${id}/approve`, { proposalVersion }); + return res.data; }, - returnToReview: async (id: string): Promise => { - await apiClient.post(`/proposals/${id}/return-to-review`); + returnToReview: async (id: string, proposalVersion?: string): Promise => { + const res = await apiClient.post(`/proposals/${id}/return-to-review`, { proposalVersion }); + return res.data; }, - sendProposal: async (id: string): Promise => { - await apiClient.post(`/proposals/${id}/send`); + sendProposal: async (id: string, proposalVersion?: string): Promise => { + const res = await apiClient.post(`/proposals/${id}/send`, { proposalVersion }); + return res.data; }, - reviseProposal: async (id: string): Promise => { - await apiClient.post(`/proposals/${id}/revise`); + /** Returns the NEW revision (different id), not the revised original. */ + reviseProposal: async (id: string, proposalVersion?: string): Promise => { + const res = await apiClient.post(`/proposals/${id}/revise`, { proposalVersion }); + return res.data; }, getHistory: async (id: string): Promise => { diff --git a/web/src/domain/admin/use-cases.ts b/web/src/domain/admin/use-cases.ts index 9c0f8d7..36c9d63 100644 --- a/web/src/domain/admin/use-cases.ts +++ b/web/src/domain/admin/use-cases.ts @@ -8,9 +8,12 @@ import { useMutation, useQuery, useQueryClient, type QueryClient } from '@tansta import { toast } from 'react-toastify'; import { adminApi } from './api'; import { lineItemsApi } from '../lineItems/api'; +import { proposalsApi } from '../proposals/api'; import { proposalsKeys } from '../proposals/use-cases'; import { lineItemsKeys } from '../lineItems/use-cases'; +import { ConflictError } from '../../lib/api/errors'; import type { UpdateLineItemEntry } from '../lineItems/types'; +import type { ProposalDetail } from '../proposals/types'; export const adminKeys = { all: ['admin'] as const, @@ -33,6 +36,38 @@ function invalidateProposalViews(queryClient: QueryClient, proposalId: string) { queryClient.invalidateQueries({ queryKey: adminKeys.dashboard() }); } +/** + * The optimistic-concurrency token for a guarded mutation, read from the + * cached proposal detail at mutate time (the workspace always fetches the + * detail before any action is possible). Read lazily inside mutationFn — + * never captured at render — so a save-then-approve chain sees the token the + * save wrote back, not the one the page rendered with. + */ +function cachedProposalVersion(queryClient: QueryClient, proposalId: string): string | undefined { + return queryClient.getQueryData(proposalsKeys.detail(proposalId))?.rowVersion; +} + +/** + * 409 recovery (refresh-and-retry semantics): write the server's reloaded + * currentState into the detail cache (fresh token immediately available), + * refetch every proposal view, and tell the user their view was stale. + * Returns true when the error was a concurrency conflict — callers skip + * their generic failure toast in that case. + */ +function handleConcurrencyConflict( + queryClient: QueryClient, + proposalId: string, + error: Error, +): boolean { + if (!(error instanceof ConflictError)) return false; + if (error.currentState) { + queryClient.setQueryData(proposalsKeys.detail(proposalId), error.currentState); + } + invalidateProposalViews(queryClient, proposalId); + toast.warning(error.message); + return true; +} + /** Admin dashboard KPIs (AdminDashboard). */ export function useAdminDashboard() { return useQuery({ @@ -63,14 +98,25 @@ export function useSaveProposalWorkspace(proposalId: string) { const queryClient = useQueryClient(); return useMutation({ mutationFn: async ({ refinedScope, lineItems }: SaveWorkspaceVariables) => { - await adminApi.updateProposal(proposalId, { refinedScope }); - await lineItemsApi.bulkUpdate(proposalId, lineItems); + // Guarded pair: the PUT rotates the proposal token, so the bulk replace + // must send the PUT response's token, not the one the save started with. + const updated = await adminApi.updateProposal(proposalId, { + refinedScope, + proposalVersion: cachedProposalVersion(queryClient, proposalId), + }); + await lineItemsApi.bulkUpdate(proposalId, lineItems, updated.rowVersion); + // The bulk replace rotates the token AGAIN but returns only line items — + // refetch the detail so a chained guarded action (approve-after-save) + // holds the current token instead of racing the invalidation refetch. + return proposalsApi.getById(proposalId); }, - onSuccess: () => { + onSuccess: (fresh) => { + queryClient.setQueryData(proposalsKeys.detail(proposalId), fresh); invalidateProposalViews(queryClient, proposalId); toast.success('Changes saved'); }, onError: (error: Error) => { + if (handleConcurrencyConflict(queryClient, proposalId, error)) return; toast.error(`Save failed: ${error.message}`); }, }); @@ -79,12 +125,15 @@ export function useSaveProposalWorkspace(proposalId: string) { export function useApproveProposal(proposalId: string) { const queryClient = useQueryClient(); return useMutation({ - mutationFn: () => adminApi.approveProposal(proposalId), - onSuccess: () => { + mutationFn: () => + adminApi.approveProposal(proposalId, cachedProposalVersion(queryClient, proposalId)), + onSuccess: (proposal) => { + queryClient.setQueryData(proposalsKeys.detail(proposal.id), proposal); invalidateProposalViews(queryClient, proposalId); toast.success('Proposal approved'); }, onError: (error: Error) => { + if (handleConcurrencyConflict(queryClient, proposalId, error)) return; toast.error(`Approval failed: ${error.message}`); }, }); @@ -93,14 +142,17 @@ export function useApproveProposal(proposalId: string) { export function useSendProposal(proposalId: string) { const queryClient = useQueryClient(); return useMutation({ - mutationFn: () => adminApi.sendProposal(proposalId), - onSuccess: () => { + mutationFn: () => + adminApi.sendProposal(proposalId, cachedProposalVersion(queryClient, proposalId)), + onSuccess: (proposal) => { + queryClient.setQueryData(proposalsKeys.detail(proposal.id), proposal); invalidateProposalViews(queryClient, proposalId); toast.success('Proposal marked as sent'); }, // Fix: WEB-H5 — mutation must surface failures to the user (relocated // from AdminWorkspace during the domain-layer refactor) onError: (error: Error) => { + if (handleConcurrencyConflict(queryClient, proposalId, error)) return; toast.error(`Send failed: ${error.message}`); }, }); @@ -109,14 +161,19 @@ export function useSendProposal(proposalId: string) { export function useReviseProposal(proposalId: string) { const queryClient = useQueryClient(); return useMutation({ - mutationFn: () => adminApi.reviseProposal(proposalId), - onSuccess: () => { + mutationFn: () => + adminApi.reviseProposal(proposalId, cachedProposalVersion(queryClient, proposalId)), + onSuccess: (revision) => { + // The response is the NEW revision (different id) — seed its detail + // cache; the invalidation below refreshes the original's views. + queryClient.setQueryData(proposalsKeys.detail(revision.id), revision); invalidateProposalViews(queryClient, proposalId); toast.success('Revision created'); }, // Fix: WEB-H6 — mutation must surface failures to the user (relocated // from AdminWorkspace during the domain-layer refactor) onError: (error: Error) => { + if (handleConcurrencyConflict(queryClient, proposalId, error)) return; toast.error(`Revision failed: ${error.message}`); }, }); @@ -125,12 +182,15 @@ export function useReviseProposal(proposalId: string) { export function useReturnToReview(proposalId: string) { const queryClient = useQueryClient(); return useMutation({ - mutationFn: () => adminApi.returnToReview(proposalId), - onSuccess: () => { + mutationFn: () => + adminApi.returnToReview(proposalId, cachedProposalVersion(queryClient, proposalId)), + onSuccess: (proposal) => { + queryClient.setQueryData(proposalsKeys.detail(proposal.id), proposal); invalidateProposalViews(queryClient, proposalId); toast.success('Proposal returned to review'); }, onError: (error: Error) => { + if (handleConcurrencyConflict(queryClient, proposalId, error)) return; toast.error(`Return to review failed: ${error.message}`); }, }); diff --git a/web/src/domain/lineItems/api.ts b/web/src/domain/lineItems/api.ts index 6d18f1d..a19721a 100644 --- a/web/src/domain/lineItems/api.ts +++ b/web/src/domain/lineItems/api.ts @@ -13,8 +13,18 @@ export const lineItemsApi = { return res.data; }, - bulkUpdate: async (proposalId: string, lineItems: UpdateLineItemEntry[]): Promise => { - const res = await apiClient.put(`/proposals/${proposalId}/line-items`, { lineItems }); + /** Guarded by the PROPOSAL token: the wholesale replace conflicts with any + * concurrent change to the aggregate, so the server requires the proposal's + * current rowVersion. Create/delete stay unguarded (single-item ops). */ + bulkUpdate: async ( + proposalId: string, + lineItems: UpdateLineItemEntry[], + proposalVersion?: string, + ): Promise => { + const res = await apiClient.put(`/proposals/${proposalId}/line-items`, { + lineItems, + proposalVersion, + }); return res.data; }, diff --git a/web/src/lib/api/__tests__/client.test.ts b/web/src/lib/api/__tests__/client.test.ts index c2aa9b7..7f02999 100644 --- a/web/src/lib/api/__tests__/client.test.ts +++ b/web/src/lib/api/__tests__/client.test.ts @@ -7,11 +7,13 @@ * - Response interceptor clears the stored session and redirects on 401 * - Response interceptor returns friendly error message on 403 * - Response interceptor returns friendly error message on 404 + * - Response interceptor throws ConflictError (with currentState) on 409 * - Response interceptor extracts server error detail from response body * - Response interceptor handles network errors (no response) */ import { describe, it, expect, vi, beforeAll, beforeEach } from 'vitest'; import type { InternalAxiosRequestConfig } from 'axios'; +import { ConflictError } from '../errors'; // vi.hoisted returns values accessible in both the hoisted mock scope and test scope. const { interceptors } = vi.hoisted(() => { @@ -209,6 +211,63 @@ describe('API client interceptors', () => { await expect(promise).rejects.toThrow('The requested resource was not found.'); }); + it('409 with the guarded conflict envelope rejects with ConflictError carrying currentState', async () => { + const currentState = { id: 'p-1', status: 'Approved', rowVersion: 'AAAAAAAAAAM=' }; + const error = { + response: { + status: 409, + data: { message: 'Proposal was modified by another user.', currentState }, + }, + request: {}, + }; + + const promise = interceptors.responseRejected(error); + + await expect(promise).rejects.toMatchObject({ + name: 'ConflictError', + message: 'Proposal was modified by another user.', + currentState, + }); + await expect(promise).rejects.toBeInstanceOf(ConflictError); + }); + + it('409 with the unguarded fallback envelope rejects with ConflictError and null currentState', async () => { + const error = { + response: { + status: 409, + data: { + status: 'Conflict', + message: 'The record was modified by another user. Refresh and retry.', + code: 409, + }, + }, + request: {}, + }; + + const promise = interceptors.responseRejected(error); + + await expect(promise).rejects.toBeInstanceOf(ConflictError); + await expect(promise).rejects.toMatchObject({ + message: 'The record was modified by another user. Refresh and retry.', + currentState: null, + }); + }); + + it('409 with an empty body falls back to a generic conflict message', async () => { + const error = { + response: { status: 409, data: {} }, + request: {}, + }; + + const promise = interceptors.responseRejected(error); + + await expect(promise).rejects.toBeInstanceOf(ConflictError); + await expect(promise).rejects.toMatchObject({ + message: 'This record was modified by another user. Refresh and retry.', + currentState: null, + }); + }); + it('extracts detail message from server error response', async () => { const error = { response: { diff --git a/web/src/lib/api/client.ts b/web/src/lib/api/client.ts index 664cc31..0c464b4 100644 --- a/web/src/lib/api/client.ts +++ b/web/src/lib/api/client.ts @@ -1,6 +1,7 @@ import axios from 'axios'; import { API_URL, STORAGE_KEY_TOKEN } from '../../constants'; import { AUTH_SESSION_CLEARED_EVENT, clearAuth } from '../auth/authStorage'; +import { ConflictError } from './errors'; const apiClient = axios.create({ baseURL: API_URL, @@ -51,6 +52,19 @@ apiClient.interceptors.response.use( return Promise.reject(new Error('The requested resource was not found.')); } + if (status === 409) { + // Optimistic-concurrency conflict (SHOC ADR 0004): the guarded path + // carries { message, currentState }; the unguarded fallback carries + // { status, message, code } with no state. Throw a typed error so the + // domain layer can refresh from currentState instead of just toasting. + return Promise.reject( + new ConflictError( + data?.message || 'This record was modified by another user. Refresh and retry.', + data?.currentState ?? null, + ), + ); + } + const message = data?.detail || data?.title || data?.message || 'An error occurred'; return Promise.reject(new Error(message)); } diff --git a/web/src/lib/api/errors.ts b/web/src/lib/api/errors.ts new file mode 100644 index 0000000..ab8789e --- /dev/null +++ b/web/src/lib/api/errors.ts @@ -0,0 +1,19 @@ +// Typed transport errors thrown by the response interceptor in ./client. +import type { ConcurrencyConflict, ProposalDetail } from '@proposal-system/api-contracts'; + +/** + * HTTP 409 — optimistic-concurrency conflict (SHOC ADR 0004 envelope). + * Carries the server's reloaded `currentState` so callers can refresh their + * caches without a round trip. The proposal aggregate is the only guarded + * resource, so the state is typed to ProposalDetail; the unguarded-race + * fallback envelope has no state (`currentState` is null). + */ +export class ConflictError extends Error implements ConcurrencyConflict { + readonly currentState: ProposalDetail | null; + + constructor(message: string, currentState: ProposalDetail | null = null) { + super(message); + this.name = 'ConflictError'; + this.currentState = currentState; + } +}