From a710e21281026287a869fca293407902aba4499f Mon Sep 17 00:00:00 2001 From: Codex Review Integration Date: Tue, 18 Aug 2026 13:09:33 -0300 Subject: [PATCH 1/5] fix(vendors): additive technician add for existing companies (SH-250, SH-246) --- .../_components/use-vendor-roster-form.ts | 37 ++- .../use-vendor-roster-selection.ts | 26 +- .../vendors/api/vendor-company-roster-api.ts | 22 +- .../vendors/mappers/vendor-roster-mapper.ts | 22 ++ .../use-save-vendor-company-roster.ts | 64 +++- .../vendors/use-vendor-roster-form.test.tsx | 302 +++++++++++++++++- .../api/vendor-company-roster-api.test.ts | 71 ++++ ...ve-vendor-company-roster-additive.test.tsx | 234 ++++++++++++++ 8 files changed, 756 insertions(+), 22 deletions(-) create mode 100644 src/test/domain/vendors/use-cases/use-save-vendor-company-roster-additive.test.tsx diff --git a/src/app/(protected)/vendors/_components/use-vendor-roster-form.ts b/src/app/(protected)/vendors/_components/use-vendor-roster-form.ts index 6bf89b1f..61f6f678 100644 --- a/src/app/(protected)/vendors/_components/use-vendor-roster-form.ts +++ b/src/app/(protected)/vendors/_components/use-vendor-roster-form.ts @@ -1,4 +1,5 @@ import { useCallback, useMemo, useState } from "react"; +import { toast } from "react-toastify"; import { useForm, useWatch, type FieldErrors } from "react-hook-form"; import { emptyVendorCompanyRosterForm, @@ -8,6 +9,7 @@ import { } from "@/domain/vendors/schemas/vendor-roster-schema"; import { useVendorCompanyRoster } from "@/domain/vendors/use-cases/use-vendor-company-roster"; import { + buildAdditiveRosterPatch, getSingleStatusOnlyChange, useSaveVendorCompanyRoster, } from "@/domain/vendors/use-cases/use-save-vendor-company-roster"; @@ -129,17 +131,38 @@ export function useVendorRosterForm({ clearConflict(); }, [clearConflict, createDefaults, reset, resetSelection]); - const committedRoster = mode === "update" ? routeRoster : selection.selectedRoster; - const isUpdate = mode === "update" || selection.selectedRoster != null; - const submit = (formValues: VendorCompanyRosterFormValues) => { setConflict(null); + const values = withoutBlankNewTechnicians(formValues); + if (mode === "create" && selection.selectedRoster != null) { + const selected = selection.selectedRoster; + if (!buildAdditiveRosterPatch(selected, values, selected.rowVersion)) { + toast.error("Enter at least one technician to add to this vendor."); + return; + } + save.mutate( + { + mode: "add", + values, + companyId: selected.companyId, + rowVersion: selected.rowVersion, + originalRoster: selected, + }, + { + onSuccess: (data) => onSuccess?.(data), + onError: (error) => { + if (isVendorRosterConflictError(error)) setConflict(error.conflict); + }, + }, + ); + return; + } save.mutate( { - mode: isUpdate ? "update" : "create", - values: withoutBlankNewTechnicians(formValues), - companyId: isUpdate ? (committedRoster?.companyId ?? companyId) : undefined, - rowVersion: isUpdate ? committedRoster?.rowVersion : undefined, + mode: mode === "update" ? "update" : "create", + values, + companyId: mode === "update" ? (routeRoster?.companyId ?? companyId) : undefined, + rowVersion: mode === "update" ? routeRoster?.rowVersion : undefined, originalRoster: mode === "update" ? routeRoster : undefined, }, { diff --git a/src/app/(protected)/vendors/_components/use-vendor-roster-selection.ts b/src/app/(protected)/vendors/_components/use-vendor-roster-selection.ts index cce00e18..4eb141b1 100644 --- a/src/app/(protected)/vendors/_components/use-vendor-roster-selection.ts +++ b/src/app/(protected)/vendors/_components/use-vendor-roster-selection.ts @@ -1,6 +1,7 @@ import { useCallback, useRef, useState } from "react"; import { useQueryClient, type UseQueryResult } from "@tanstack/react-query"; import { + emptyRosterTechnician, emptyVendorCompanyRosterForm, type VendorCompanyRosterFormValues, } from "@/domain/vendors/schemas/vendor-roster-schema"; @@ -53,6 +54,20 @@ export function toFormValues(roster: VendorCompanyRoster): VendorCompanyRosterFo }; } +export function toCompanyFormValues(roster: VendorCompanyRoster): VendorCompanyRosterFormValues { + return { ...toFormValues(roster), technicians: [] }; +} + +function withCompanyFieldsOnly( + roster: VendorCompanyRoster, +): (current: VendorCompanyRosterFormValues) => VendorCompanyRosterFormValues { + return (current) => ({ + ...toCompanyFormValues(roster), + technicians: + current.technicians.length > 0 ? current.technicians : [{ ...emptyRosterTechnician }], + }); +} + export function useVendorRosterSelection({ mode, reset, @@ -91,7 +106,7 @@ export function useVendorRosterSelection({ try { const roster = await fetchRosterByCompany(company.companyId); if (requestIdRef.current !== requestId) return; - reset(toFormValues(roster)); + reset(withCompanyFieldsOnly(roster)); setSelectedRoster(roster); clearConflict(); retryTargetRef.current = null; @@ -107,7 +122,11 @@ export function useVendorRosterSelection({ (nextName = "") => { requestIdRef.current += 1; if (selectedRoster) { - reset({ ...emptyVendorCompanyRosterForm, name: nextName }); + reset((current) => ({ + ...emptyVendorCompanyRosterForm, + name: nextName, + technicians: current.technicians, + })); } setSelectedRoster(null); clearConflict(); @@ -134,7 +153,6 @@ export function useVendorRosterSelection({ void fetchRosterByCompany(selectedRoster.companyId) .then((roster) => { if (requestIdRef.current !== requestId) return; - reset(toFormValues(roster)); setSelectedRoster(roster); retryTargetRef.current = null; }) @@ -143,7 +161,7 @@ export function useVendorRosterSelection({ setLoadErrorMessage(RELOAD_ERROR_MESSAGE); }); } - }, [clearConflict, fetchRosterByCompany, mode, query, reset, selectedRoster]); + }, [clearConflict, fetchRosterByCompany, mode, query, selectedRoster]); const retryLoad = useCallback(() => { const target = retryTargetRef.current; diff --git a/src/domain/vendors/api/vendor-company-roster-api.ts b/src/domain/vendors/api/vendor-company-roster-api.ts index dd9c978a..fb051bbb 100644 --- a/src/domain/vendors/api/vendor-company-roster-api.ts +++ b/src/domain/vendors/api/vendor-company-roster-api.ts @@ -1,14 +1,16 @@ import { isHTTPError } from "ky"; import { API_PATHS } from "@/api/api-paths"; -import { apiGet, apiPost, apiPut } from "@/api/api"; +import { apiGet, apiPatch, apiPost, apiPut } from "@/api/api"; import { handleApiResponse } from "@/api/handle-api-response"; import { mapRosterConflict, mapVendorCompanyRoster, + mapVendorRosterAdditivePatchToBackend, mapVendorRosterToBackend, } from "@/domain/vendors/mappers/vendor-roster-mapper"; import { VendorRosterConflictError } from "@/domain/vendors/lib/vendor-roster-conflict"; import type { VendorCompanyRoster } from "@/domain/vendors/types/vendor"; +import type { VendorRosterAdditivePatch } from "@/domain/vendors/mappers/vendor-roster-mapper"; const STALE_MESSAGE = "This company was changed by another session. Reload the latest version and try again."; @@ -97,4 +99,22 @@ export const vendorCompanyRosterApi = { throw error; } }, + + addTechnicians: async ( + companyId: string | number, + patch: VendorRosterAdditivePatch, + ): Promise => { + try { + const data = await apiPatch( + API_PATHS.vendorCompanyRoster.byCompany(companyId), + mapVendorRosterAdditivePatchToBackend(patch), + ); + return mapVendorCompanyRoster(handleApiResponse(data)); + } catch (error) { + if (isHTTPError(error) && error.response.status === 409) { + await throwRosterConflict(error); + } + throw error; + } + }, }; diff --git a/src/domain/vendors/mappers/vendor-roster-mapper.ts b/src/domain/vendors/mappers/vendor-roster-mapper.ts index 8dd450c4..e4de67dd 100644 --- a/src/domain/vendors/mappers/vendor-roster-mapper.ts +++ b/src/domain/vendors/mappers/vendor-roster-mapper.ts @@ -134,6 +134,28 @@ export function mapVendorRosterToBackend(values: unknown): Record; +} + +export function mapVendorRosterAdditivePatchToBackend( + patch: VendorRosterAdditivePatch, +): Record { + const item = asRecord(patch); + const payload: Record = { + rowVersion: readString(item, "rowVersion", "RowVersion"), + addTechnicians: patch.addTechnicians.map((technician) => { + const mapped = mapRosterTechnicianToBackend(technician); + delete mapped.id; + return mapped; + }), + }; + if (patch.companyFields) payload.companyFields = { ...patch.companyFields }; + return payload; +} + function mapBlockedWorkOrder(raw: unknown): VendorRosterBlockedWorkOrder { const item = asRecord(raw); return { diff --git a/src/domain/vendors/use-cases/use-save-vendor-company-roster.ts b/src/domain/vendors/use-cases/use-save-vendor-company-roster.ts index 37532220..1d1c4865 100644 --- a/src/domain/vendors/use-cases/use-save-vendor-company-roster.ts +++ b/src/domain/vendors/use-cases/use-save-vendor-company-roster.ts @@ -11,7 +11,7 @@ import type { import { queryKeys } from "@/infra/query-key/query-key"; export interface SaveVendorCompanyRosterInput { - mode: "create" | "update"; + mode: "create" | "update" | "add"; values: VendorCompanyRosterFormValues; companyId?: string | number | null; rowVersion?: string; @@ -65,6 +65,55 @@ export function getSingleStatusOnlyChange( return changed.length === 1 ? changed[0] : null; } +export interface VendorRosterAdditivePatchInput { + rowVersion: string; + addTechnicians: Array<{ + contactName: string; + phone: string; + email: string; + preferredContact?: string; + tradeSpecialties: string; + isActive: boolean; + }>; + companyFields?: Record; +} + +function hasTechnicianContent(technician: { + contactName: string; + phone: string; + email: string; + tradeSpecialties: string; +}): boolean { + return Boolean( + technician.contactName.trim() || + technician.phone.trim() || + technician.email.trim() || + technician.tradeSpecialties.trim(), + ); +} + +export function buildAdditiveRosterPatch( + roster: VendorCompanyRoster | null | undefined, + values: VendorCompanyRosterFormValues, + rowVersion: string, +): VendorRosterAdditivePatchInput | null { + const addTechnicians = values.technicians.filter( + (technician) => technician.id == null && hasTechnicianContent(technician), + ); + const companyFields: Record = {}; + if (roster) { + for (const field of COMPANY_FIELDS) { + if (values[field] !== roster[field]) companyFields[field] = values[field]; + } + } + if (addTechnicians.length === 0 && Object.keys(companyFields).length === 0) return null; + return { + rowVersion, + addTechnicians, + ...(Object.keys(companyFields).length > 0 ? { companyFields } : {}), + }; +} + export function useSaveVendorCompanyRoster(): UseMutationResult< VendorCompanyRoster, unknown, @@ -108,17 +157,26 @@ export function useSaveVendorCompanyRoster(): UseMutationResult< if (!rowVersion) throw new Error("Row version is required to update"); return vendorCompanyRosterApi.update(companyId, values, rowVersion); } + if (mode === "add") { + if (companyId === undefined || companyId === null || companyId === "") { + throw new Error("Company id is required to add technicians"); + } + if (!rowVersion) throw new Error("Row version is required to add technicians"); + const patch = buildAdditiveRosterPatch(originalRoster, values, rowVersion); + if (!patch) throw new Error("Nothing to add to this vendor company"); + return vendorCompanyRosterApi.addTechnicians(companyId, patch); + } return vendorCompanyRosterApi.create(values); }, onSuccess: (data, variables) => { void queryClient.invalidateQueries({ queryKey: queryKeys.vendors.all }); - if (variables.mode === "update" && variables.companyId) { + if ((variables.mode === "update" || variables.mode === "add") && variables.companyId) { void queryClient.invalidateQueries({ queryKey: queryKeys.vendors.roster("companyId", variables.companyId), }); } toast.success( - variables.mode === "update" ? "Vendor company updated" : "Vendor company created", + variables.mode === "create" ? "Vendor company created" : "Vendor company updated", ); }, onError: (error) => { diff --git a/src/test/app/(protected)/vendors/use-vendor-roster-form.test.tsx b/src/test/app/(protected)/vendors/use-vendor-roster-form.test.tsx index 2e9f8f1f..80091636 100644 --- a/src/test/app/(protected)/vendors/use-vendor-roster-form.test.tsx +++ b/src/test/app/(protected)/vendors/use-vendor-roster-form.test.tsx @@ -12,15 +12,19 @@ vi.mock("@/domain/vendors/api/vendor-company-roster-api", () => ({ })); vi.mock("@/domain/vendors/use-cases/use-vendor-company-roster", () => ({ - useVendorCompanyRoster: () => ({ - data: undefined, - isLoading: false, - isError: false, - error: null, - refetch: vi.fn(), - }), + useVendorCompanyRoster: () => rosterQueryResult, })); +vi.mock("react-toastify", () => ({ toast: { success: vi.fn(), error: vi.fn() } })); + +let rosterQueryResult: { + data: VendorCompanyRoster | undefined; + isLoading: boolean; + isError: boolean; + error: Error | null; + refetch: ReturnType; +} = { data: undefined, isLoading: false, isError: false, error: null, refetch: vi.fn() }; + vi.mock("@/domain/vendors/use-cases/use-vendor-facets", () => ({ useVendorFacets: () => ({ data: { companies: [], trades: [] } }), })); @@ -36,6 +40,7 @@ vi.mock("@/domain/vendors/use-cases/use-save-vendor-company-roster", async () => }); import { useVendorRosterForm } from "@/app/(protected)/vendors/_components/use-vendor-roster-form"; +import type { VendorCompanyRosterFormValues } from "@/domain/vendors/schemas/vendor-roster-schema"; import type { VendorCompanyRoster, VendorFacetCompany } from "@/domain/vendors/types/vendor"; const company: VendorFacetCompany = { @@ -81,6 +86,72 @@ const secondRoster: VendorCompanyRoster = { email: "dispatch@metro.test", }; +const rosterWithTechnicians: VendorCompanyRoster = { + ...roster, + technicians: [ + { + id: 11, + contactName: "Ray Holt", + phone: "(314) 555-0122", + email: "", + preferredContact: "Phone", + tradeSpecialties: "Plumbing", + isActive: true, + totalJobs: 0, + }, + { + id: 12, + contactName: "Amy Santiago", + phone: "(314) 555-0133", + email: "", + preferredContact: "Text", + tradeSpecialties: "HVAC", + isActive: true, + totalJobs: 0, + }, + ], +}; + +const newTechnician = { + contactName: "Dana Kim", + phone: "(314) 555-0144", + email: "dana@test.test", + preferredContact: "Text" as const, + tradeSpecialties: "HVAC", + isActive: true, +}; + +function submitValues( + technicians: VendorCompanyRosterFormValues["technicians"], +): VendorCompanyRosterFormValues { + return { + name: "Gateway Plumbing", + companyPhone: "(314) 555-0100", + email: "dispatch@gateway.test", + address: "1 Main St", + city: "St. Louis", + state: "MO", + zip: "63101", + googleMapsUrl: "", + notes: "", + technicians, + }; +} + +function resetRosterQuery(): void { + rosterQueryResult = { + data: undefined, + isLoading: false, + isError: false, + error: null, + refetch: vi.fn(), + }; +} + +beforeEach(() => { + resetRosterQuery(); +}); + function createClient(): QueryClient { return new QueryClient({ defaultOptions: { queries: { retry: false } } }); } @@ -316,3 +387,220 @@ describe("useVendorRosterForm stale-selection handling", () => { expect(result.current.name.field.value).toBe("Draft Vendor"); }); }); + +describe("useVendorRosterForm additive add flow (SH-250)", () => { + beforeEach(() => { + rosterGet.mockReset(); + saveMutate.mockReset(); + }); + + it("issues an additive add — never update — when an existing company is selected", async () => { + rosterGet.mockResolvedValueOnce(roster); + const { result } = renderHook( + () => { + const form = useVendorRosterForm({ mode: "create", startWithTechnician: true }); + const technician = useController({ + control: form.control, + name: "technicians.0.contactName", + }); + return { form, technician }; + }, + { wrapper: makeWrapper(createClient()) }, + ); + + await act(async () => { + await result.current.form.selectCompany(company); + }); + await act(async () => { + result.current.technician.field.onChange("Dana Kim"); + }); + act(() => { + result.current.form.submit(submitValues([newTechnician])); + }); + + expect(saveMutate).toHaveBeenCalledTimes(1); + const input = saveMutate.mock.calls[0]?.[0] as { mode: string }; + expect(input.mode).toBe("add"); + expect(saveMutate).toHaveBeenCalledWith( + expect.objectContaining({ + companyId: 5, + rowVersion: "rv-1", + originalRoster: roster, + values: expect.objectContaining({ technicians: [newTechnician] }), + }), + expect.any(Object), + ); + }); + + it("still creates via POST when no existing company is matched", () => { + const { result } = renderHook( + () => useVendorRosterForm({ mode: "create", startWithTechnician: true }), + { wrapper: makeWrapper(createClient()) }, + ); + + act(() => { + result.current.submit(submitValues([newTechnician])); + }); + + expect(saveMutate).toHaveBeenCalledTimes(1); + expect(saveMutate.mock.calls[0]?.[0]).toEqual( + expect.objectContaining({ mode: "create", companyId: undefined, rowVersion: undefined }), + ); + }); + + it("blocks submit when the selected company has nothing to add", async () => { + rosterGet.mockResolvedValueOnce(roster); + const { result } = renderHook(() => useVendorRosterForm({ mode: "create" }), { + wrapper: makeWrapper(createClient()), + }); + + await act(async () => { + await result.current.selectCompany(company); + }); + act(() => { + result.current.submit(submitValues([])); + }); + + expect(saveMutate).not.toHaveBeenCalled(); + }); + + it("keeps the entered technician and retries with the fresh rowVersion after reload", async () => { + rosterGet.mockResolvedValueOnce(roster); + const { result } = renderHook( + () => { + const form = useVendorRosterForm({ mode: "create", startWithTechnician: true }); + const technician = useController({ + control: form.control, + name: "technicians.0.contactName", + }); + return { form, technician }; + }, + { wrapper: makeWrapper(createClient()) }, + ); + + await act(async () => { + await result.current.form.selectCompany(company); + }); + await act(async () => { + result.current.technician.field.onChange("Dana Kim"); + }); + act(() => { + result.current.form.submit(submitValues([newTechnician])); + }); + expect(saveMutate.mock.calls[0]?.[0]).toEqual(expect.objectContaining({ rowVersion: "rv-1" })); + + rosterGet.mockResolvedValueOnce({ ...roster, rowVersion: "rv-2" }); + await act(async () => { + result.current.form.reload(); + }); + await waitFor(() => expect(result.current.form.selectedCompanyId).toBe(5)); + + act(() => { + result.current.form.submit(submitValues([newTechnician])); + }); + expect(saveMutate.mock.calls[1]?.[0]).toEqual( + expect.objectContaining({ mode: "add", rowVersion: "rv-2" }), + ); + }); + + it("does not regress the Edit Vendor reconcile path (mode update)", () => { + rosterQueryResult = { + data: rosterWithTechnicians, + isLoading: false, + isError: false, + error: null, + refetch: vi.fn(), + }; + const { result } = renderHook(() => useVendorRosterForm({ mode: "update", vendorId: 7 }), { + wrapper: makeWrapper(createClient()), + }); + + act(() => { + result.current.submit(submitValues([...rosterWithTechnicians.technicians, newTechnician])); + }); + + expect(saveMutate).toHaveBeenCalledTimes(1); + expect(saveMutate.mock.calls[0]?.[0]).toEqual( + expect.objectContaining({ + mode: "update", + companyId: 5, + rowVersion: "rv-1", + originalRoster: rosterWithTechnicians, + values: expect.objectContaining({ + technicians: [...rosterWithTechnicians.technicians, newTechnician], + }), + }), + ); + }); +}); + +describe("useVendorRosterForm technician autopopulation (SH-246)", () => { + beforeEach(() => { + rosterGet.mockReset(); + saveMutate.mockReset(); + }); + + it("does not autopopulate technician fields when a company is selected", async () => { + rosterGet.mockResolvedValueOnce(rosterWithTechnicians); + const { result } = renderHook( + () => { + const form = useVendorRosterForm({ mode: "create" }); + const technicians = useController({ control: form.control, name: "technicians" }); + const name = useController({ control: form.control, name: "name" }); + return { form, technicians, name }; + }, + { wrapper: makeWrapper(createClient()) }, + ); + + await act(async () => { + await result.current.form.selectCompany(company); + }); + + const technicians = result.current.technicians.field.value; + expect(technicians).toEqual([ + { + contactName: "", + phone: "", + email: "", + preferredContact: "Phone", + tradeSpecialties: "", + isActive: true, + }, + ]); + expect(technicians).not.toContainEqual(expect.objectContaining({ contactName: "Ray Holt" })); + expect(technicians).not.toContainEqual( + expect.objectContaining({ contactName: "Amy Santiago" }), + ); + expect(result.current.name.field.value).toBe("Gateway Plumbing"); + expect(result.current.form.selectedCompanyId).toBe(5); + }); + + it("keeps a typed technician when the company selection is cleared to free text", async () => { + rosterGet.mockResolvedValueOnce(roster); + const { result } = renderHook( + () => { + const form = useVendorRosterForm({ mode: "create", startWithTechnician: true }); + const technician = useController({ + control: form.control, + name: "technicians.0.contactName", + }); + return { form, technician }; + }, + { wrapper: makeWrapper(createClient()) }, + ); + + await act(async () => { + await result.current.form.selectCompany(company); + }); + await act(async () => { + result.current.technician.field.onChange("Dana Kim"); + }); + + act(() => { + result.current.form.clearSelectedCompany("Draft Vendor"); + }); + + expect(result.current.form.selectedCompanyId).toBeNull(); + expect(result.current.technician.field.value).toBe("Dana Kim"); + }); +}); diff --git a/src/test/domain/vendors/api/vendor-company-roster-api.test.ts b/src/test/domain/vendors/api/vendor-company-roster-api.test.ts index 1bad51e4..32fb5aeb 100644 --- a/src/test/domain/vendors/api/vendor-company-roster-api.test.ts +++ b/src/test/domain/vendors/api/vendor-company-roster-api.test.ts @@ -4,11 +4,13 @@ import { API_PATHS } from "@/api/api-paths"; const apiGet = vi.fn(); const apiPost = vi.fn(); const apiPut = vi.fn(); +const apiPatch = vi.fn(); vi.mock("@/api/api", () => ({ apiGet: (...args: unknown[]) => apiGet(...args), apiPost: (...args: unknown[]) => apiPost(...args), apiPut: (...args: unknown[]) => apiPut(...args), + apiPatch: (...args: unknown[]) => apiPatch(...args), })); vi.mock("ky", () => ({ @@ -38,6 +40,7 @@ describe("vendorCompanyRosterApi", () => { apiGet.mockReset(); apiPost.mockReset(); apiPut.mockReset(); + apiPatch.mockReset(); }); it("fetches by vendorId or companyId with the correct query params", async () => { @@ -121,4 +124,72 @@ describe("vendorCompanyRosterApi", () => { await expect(vendorCompanyRosterApi.create({ name: "Solo Co" })).rejects.toBe(generic); }); + + it("patches the additive payload without a technician id and without PUT", async () => { + apiPatch.mockResolvedValueOnce({ data: { companyId: 5, name: "Solo Co" } }); + + await vendorCompanyRosterApi.addTechnicians(5, { + rowVersion: "rv-1", + addTechnicians: [ + { + id: 99, + contactName: "Adam", + phone: "3145550198", + email: "", + preferredContact: "Phone", + tradeSpecialties: "Plumbing", + isActive: true, + }, + ], + }); + + expect(apiPatch).toHaveBeenCalledWith( + API_PATHS.vendorCompanyRoster.byCompany(5), + expect.objectContaining({ + rowVersion: "rv-1", + addTechnicians: [expect.objectContaining({ contactName: "Adam", phone: "(314) 555-0198" })], + }), + ); + const body = apiPatch.mock.calls[0]?.[1] as Record; + expect(body).not.toHaveProperty("companyFields"); + expect((body.addTechnicians as Array>)[0]).not.toHaveProperty("id"); + expect(apiPut).not.toHaveBeenCalled(); + expect(apiPost).not.toHaveBeenCalled(); + }); + + it("includes companyFields on the additive patch when provided", async () => { + apiPatch.mockResolvedValueOnce({ data: { companyId: 5, name: "Solo Co" } }); + + await vendorCompanyRosterApi.addTechnicians(5, { + rowVersion: "rv-2", + addTechnicians: [ + { contactName: "Adam", phone: "", email: "", tradeSpecialties: "", isActive: true }, + ], + companyFields: { notes: "Updated notes" }, + }); + + expect(apiPatch).toHaveBeenCalledWith( + API_PATHS.vendorCompanyRoster.byCompany(5), + expect.objectContaining({ + rowVersion: "rv-2", + companyFields: { notes: "Updated notes" }, + }), + ); + }); + + it("throws a stale conflict on a 409 from the additive patch", async () => { + apiPatch.mockRejectedValueOnce(httpError(409, { message: "rowversion mismatch" })); + + await expect( + vendorCompanyRosterApi.addTechnicians(5, { + rowVersion: "rv-1", + addTechnicians: [ + { contactName: "Adam", phone: "", email: "", tradeSpecialties: "", isActive: true }, + ], + }), + ).rejects.toSatisfy((error: unknown) => { + if (!isVendorRosterConflictError(error)) return false; + return error.conflict.kind === "stale"; + }); + }); }); diff --git a/src/test/domain/vendors/use-cases/use-save-vendor-company-roster-additive.test.tsx b/src/test/domain/vendors/use-cases/use-save-vendor-company-roster-additive.test.tsx new file mode 100644 index 00000000..61c431e6 --- /dev/null +++ b/src/test/domain/vendors/use-cases/use-save-vendor-company-roster-additive.test.tsx @@ -0,0 +1,234 @@ +import { act, renderHook, waitFor } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import type { ReactNode } from "react"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const rosterAdd = vi.fn(); +const rosterUpdate = vi.fn(); +const rosterCreate = vi.fn(); + +vi.mock("@/domain/vendors/api/vendor-company-roster-api", () => ({ + vendorCompanyRosterApi: { + addTechnicians: (...args: unknown[]) => rosterAdd(...args), + update: (...args: unknown[]) => rosterUpdate(...args), + create: (...args: unknown[]) => rosterCreate(...args), + }, +})); + +vi.mock("@/domain/vendors/api/vendors-api", () => ({ + vendorsApi: { update: vi.fn() }, +})); + +vi.mock("react-toastify", () => ({ toast: { success: vi.fn(), error: vi.fn() } })); + +import { + buildAdditiveRosterPatch, + useSaveVendorCompanyRoster, +} from "@/domain/vendors/use-cases/use-save-vendor-company-roster"; +import type { VendorCompanyRoster } from "@/domain/vendors/types/vendor"; +import type { VendorCompanyRosterFormValues } from "@/domain/vendors/schemas/vendor-roster-schema"; + +const roster: VendorCompanyRoster = { + companyId: 5, + rowVersion: "rv-1", + name: "Gateway Plumbing", + companyPhone: "(314) 555-0100", + email: "dispatch@gateway.test", + address: "1 Main St", + city: "St. Louis", + state: "MO", + zip: "63101", + googleMapsUrl: "", + notes: "Preferred vendor", + technicians: [ + { + id: 11, + contactName: "Ray Holt", + phone: "(314) 555-0122", + email: "", + preferredContact: "Phone", + tradeSpecialties: "Plumbing", + isActive: true, + totalJobs: 0, + }, + ], +}; + +const newTechnician = { + contactName: "Dana Kim", + phone: "(314) 555-0144", + email: "dana@test.test", + preferredContact: "Text" as const, + tradeSpecialties: "HVAC", + isActive: true, +}; + +function formValues( + overrides: Partial = {}, +): VendorCompanyRosterFormValues { + return { + name: roster.name, + companyPhone: roster.companyPhone, + email: roster.email, + address: roster.address, + city: roster.city, + state: roster.state, + zip: roster.zip, + googleMapsUrl: roster.googleMapsUrl, + notes: roster.notes, + technicians: [newTechnician], + ...overrides, + }; +} + +describe("buildAdditiveRosterPatch", () => { + it("carries only the newly entered technician, never a full roster snapshot", () => { + const patch = buildAdditiveRosterPatch(roster, formValues(), "rv-1"); + + expect(patch).not.toBeNull(); + expect(patch?.addTechnicians).toEqual([newTechnician]); + expect(patch?.addTechnicians[0]).not.toHaveProperty("id"); + }); + + it("ignores blank technician rows", () => { + const patch = buildAdditiveRosterPatch( + roster, + formValues({ + technicians: [ + { + contactName: "", + phone: "", + email: "", + preferredContact: "Phone", + tradeSpecialties: "", + isActive: true, + }, + newTechnician, + ], + }), + "rv-1", + ); + + expect(patch?.addTechnicians).toEqual([newTechnician]); + }); + + it("sends only changed company fields", () => { + const patch = buildAdditiveRosterPatch( + roster, + formValues({ notes: "New notes", address: "2 Oak Ave" }), + "rv-1", + ); + + expect(patch?.companyFields).toEqual({ notes: "New notes", address: "2 Oak Ave" }); + }); + + it("omits companyFields entirely when nothing changed", () => { + const patch = buildAdditiveRosterPatch(roster, formValues(), "rv-1"); + + expect(patch).not.toHaveProperty("companyFields"); + }); + + it("returns null when there is nothing to add or change", () => { + const patch = buildAdditiveRosterPatch( + roster, + formValues({ + technicians: [ + { + contactName: "", + phone: "", + email: "", + preferredContact: "Phone", + tradeSpecialties: "", + isActive: true, + }, + ], + }), + "rv-1", + ); + + expect(patch).toBeNull(); + }); +}); + +function createWrapper() { + const client = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + return function Wrapper({ children }: { children: ReactNode }) { + return {children}; + }; +} + +describe("useSaveVendorCompanyRoster routing", () => { + beforeEach(() => { + rosterAdd.mockReset(); + rosterUpdate.mockReset(); + rosterCreate.mockReset(); + }); + + it("add mode issues the additive patch and never the reconcile PUT", async () => { + rosterAdd.mockResolvedValueOnce(roster); + const { result } = renderHook(() => useSaveVendorCompanyRoster(), { + wrapper: createWrapper(), + }); + + act(() => { + result.current.mutate({ + mode: "add", + values: formValues(), + companyId: 5, + rowVersion: "rv-1", + originalRoster: roster, + }); + }); + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + + expect(rosterAdd).toHaveBeenCalledTimes(1); + expect(rosterAdd).toHaveBeenCalledWith( + 5, + expect.objectContaining({ + rowVersion: "rv-1", + addTechnicians: [newTechnician], + }), + ); + expect(rosterUpdate).not.toHaveBeenCalled(); + expect(rosterCreate).not.toHaveBeenCalled(); + }); + + it("update mode still issues the full reconcile PUT", async () => { + rosterUpdate.mockResolvedValueOnce(roster); + const { result } = renderHook(() => useSaveVendorCompanyRoster(), { + wrapper: createWrapper(), + }); + + act(() => { + result.current.mutate({ + mode: "update", + values: formValues({ + technicians: [{ id: 11, ...newTechnician }, newTechnician], + }), + companyId: 5, + rowVersion: "rv-1", + originalRoster: roster, + }); + }); + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + + expect(rosterUpdate).toHaveBeenCalledWith(5, expect.anything(), "rv-1"); + expect(rosterAdd).not.toHaveBeenCalled(); + }); + + it("create mode still issues the POST", async () => { + rosterCreate.mockResolvedValueOnce(roster); + const { result } = renderHook(() => useSaveVendorCompanyRoster(), { + wrapper: createWrapper(), + }); + + act(() => { + result.current.mutate({ mode: "create", values: formValues() }); + }); + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + + expect(rosterCreate).toHaveBeenCalledTimes(1); + expect(rosterAdd).not.toHaveBeenCalled(); + expect(rosterUpdate).not.toHaveBeenCalled(); + }); +}); From 4d1f02b87dd3e4e071bc0e299b8bfbce91eb30d7 Mon Sep 17 00:00:00 2001 From: Codex Review Integration Date: Tue, 18 Aug 2026 13:13:04 -0300 Subject: [PATCH 2/5] test(vendors): e2e asserts additive PATCH and no destructive PUT from add flow (SH-250, SH-246) --- e2e/vendors/vendors.spec.ts | 86 +++++++++++++++++++++++++++++-------- 1 file changed, 69 insertions(+), 17 deletions(-) diff --git a/e2e/vendors/vendors.spec.ts b/e2e/vendors/vendors.spec.ts index ad1a89e8..f5520d1d 100644 --- a/e2e/vendors/vendors.spec.ts +++ b/e2e/vendors/vendors.spec.ts @@ -65,6 +65,8 @@ interface MockState { listUrls: string[]; createdBody?: Record; updatedBody?: Record; + patchedBody?: Record; + patchedCompanyId?: string; deletedId?: string; } @@ -175,6 +177,52 @@ async function mockVendorApi( return; } + if (request.method() === "PATCH" && pathCompanyId) { + state.patchedBody = request.postDataJSON(); + state.patchedCompanyId = pathCompanyId; + const anchor = vendorRecords.find((vendor) => String(vendor.CompanyId) === pathCompanyId); + if (!anchor) { + await fulfillJson(route, { message: "Vendor roster not found" }, 404); + return; + } + const added = Array.isArray(state.patchedBody.addTechnicians) + ? (state.patchedBody.addTechnicians as Array>) + : []; + await fulfillJson(route, { + companyId: anchor.CompanyId, + rowVersion: "rv-patched", + name: anchor.CompanyName, + companyPhone: anchor.CompanyPhone, + email: anchor.Email, + address: anchor.Address, + city: anchor.City, + state: anchor.State, + zip: anchor.Zip, + googleMapsUrl: anchor.GoogleMapsUrl, + notes: anchor.Notes, + technicians: [ + ...vendorRecords + .filter((vendor) => vendor.CompanyId === anchor.CompanyId) + .map((vendor) => ({ + id: vendor.Id, + contactName: vendor.ContactName, + phone: vendor.Phone, + email: vendor.Email, + preferredContact: vendor.PreferredContact ?? "Phone", + tradeSpecialties: vendor.TradeSpecialties, + isActive: vendor.IsActive, + totalJobs: vendor.TotalJobs, + })), + ...added.map((technician, index) => ({ + id: 900 + index, + totalJobs: 0, + ...technician, + })), + ], + }); + return; + } + if (request.method() === "PUT" && pathCompanyId) { state.updatedBody = request.postDataJSON(); const technicians = Array.isArray(state.updatedBody.technicians) @@ -409,6 +457,11 @@ test.describe("Vendor directory prototype parity", () => { "https://maps.google.com/gateway", ); await expect(page.getByLabel("Preferred Contact")).toHaveCount(0); + await expect(page.getByLabel("Technician name (optional)")).toHaveCount(1); + await expect(page.getByLabel("Technician name (optional)")).toHaveValue(""); + await expect( + page.getByRole("dialog", { name: /Add Vendor/ }).getByText("Adam Whyte"), + ).toHaveCount(0); await page.getByRole("button", { name: "Add technician" }).click(); await page.getByLabel("Technician name (optional)").last().fill("New Technician"); const tradeInput = page.getByRole("combobox", { name: "Add Trade" }).last(); @@ -420,27 +473,26 @@ test.describe("Vendor directory prototype parity", () => { await page.getByLabel("Notes (optional)").fill("Created in browser E2E"); await page.getByRole("button", { name: "Add Vendor", exact: true }).last().click(); await expect(page.getByRole("dialog", { name: /Add Vendor/ })).toHaveCount(0); - expect(state.updatedBody).toMatchObject({ - name: "Gateway Plumbing", - companyPhone: "(314) 555-0100", - notes: "Created in browser E2E", - rowVersion: "rv-1", - }); - expect(state.updatedBody?.technicians).toEqual( - expect.arrayContaining([ - expect.objectContaining({ - contactName: "New Technician", - tradeSpecialties: "Plumbing, HVAC", - }), - ]), - ); - const submittedTechnicians = Array.isArray(state.updatedBody?.technicians) - ? (state.updatedBody.technicians as Array>) + expect(state.patchedCompanyId).toBe("101"); + expect(state.patchedBody).toMatchObject({ rowVersion: "rv-1" }); + expect(state.patchedBody?.companyFields).toEqual({ notes: "Created in browser E2E" }); + expect(state.patchedBody?.addTechnicians).toEqual([ + expect.objectContaining({ + contactName: "New Technician", + tradeSpecialties: "Plumbing, HVAC", + }), + ]); + const patchedTechnicians = Array.isArray(state.patchedBody?.addTechnicians) + ? (state.patchedBody.addTechnicians as Array>) : []; - const newTechnician = submittedTechnicians.find( + const newTechnician = patchedTechnicians.find( (technician) => technician.contactName === "New Technician", ); expect(newTechnician?.preferredContact).toBeUndefined(); + expect(patchedTechnicians.some((technician) => technician.contactName === "Adam Whyte")).toBe( + false, + ); + expect(state.updatedBody).toBeUndefined(); await page.getByRole("button", { name: "View vendor Gateway Plumbing" }).click(); const detailDrawer = page.locator(".MuiDrawer-paper").last(); From 9624c264de0346fcc55a6ca322f516259ff7de9c Mon Sep 17 00:00:00 2001 From: Codex Review Integration Date: Tue, 18 Aug 2026 14:42:01 -0300 Subject: [PATCH 3/5] fix(vendors): send only user-edited company fields on additive add (SH-250) Adversarial review found a last-writer-wins window: after a stale 409 the roster is refetched while the form keeps its pre-conflict values, so the form-vs-roster diff resent our stale value for any company field another user changed in that window, silently reverting their edit. Transmit only fields the user actually edited, taken from react-hook-form dirty state. Selecting a company calls reset(), so loaded values are never dirty and only genuine edits are sent. dirtyFields is resolved during render: formState is a Proxy that only tracks properties read at render time, so reading it inside the submit callback returned empty. The vendors E2E caught that; unit tests could not, since they pass the flags in directly. --- e2e/vendors/vendors.spec.ts | 7 ++++ .../_components/use-vendor-roster-form.ts | 34 ++++++++++++++++++- .../use-save-vendor-company-roster.ts | 23 ++++++++++++- ...ve-vendor-company-roster-additive.test.tsx | 31 ++++++++++++++++- 4 files changed, 92 insertions(+), 3 deletions(-) diff --git a/e2e/vendors/vendors.spec.ts b/e2e/vendors/vendors.spec.ts index f5520d1d..b348fd21 100644 --- a/e2e/vendors/vendors.spec.ts +++ b/e2e/vendors/vendors.spec.ts @@ -475,7 +475,14 @@ test.describe("Vendor directory prototype parity", () => { await expect(page.getByRole("dialog", { name: /Add Vendor/ })).toHaveCount(0); expect(state.patchedCompanyId).toBe("101"); expect(state.patchedBody).toMatchObject({ rowVersion: "rv-1" }); + // Only the field the user actually typed into is transmitted. Address/phone/email were + // populated by selecting the company and never edited, so they must not be sent — otherwise a + // concurrent edit by another user to any of them would be silently reverted on resubmit. expect(state.patchedBody?.companyFields).toEqual({ notes: "Created in browser E2E" }); + expect(state.patchedBody?.companyFields).not.toHaveProperty("companyPhone"); + expect(state.patchedBody?.companyFields).not.toHaveProperty("address"); + expect(state.patchedBody?.companyFields).not.toHaveProperty("email"); + expect(state.patchedBody?.companyFields).not.toHaveProperty("name"); expect(state.patchedBody?.addTechnicians).toEqual([ expect.objectContaining({ contactName: "New Technician", diff --git a/src/app/(protected)/vendors/_components/use-vendor-roster-form.ts b/src/app/(protected)/vendors/_components/use-vendor-roster-form.ts index 61f6f678..96f5e3b7 100644 --- a/src/app/(protected)/vendors/_components/use-vendor-roster-form.ts +++ b/src/app/(protected)/vendors/_components/use-vendor-roster-form.ts @@ -12,8 +12,35 @@ import { buildAdditiveRosterPatch, getSingleStatusOnlyChange, useSaveVendorCompanyRoster, + type EditedCompanyFields, } from "@/domain/vendors/use-cases/use-save-vendor-company-roster"; import { useVendorRosterResolver } from "./use-vendor-roster-resolver"; + +const EDITABLE_COMPANY_FIELDS = [ + "name", + "companyPhone", + "email", + "address", + "city", + "state", + "zip", + "googleMapsUrl", + "notes", +] as const; + +/** + * Company fields the user actually typed into, taken from react-hook-form dirty state. + * Selecting a company calls reset(), so loaded values are not dirty; only genuine edits are. + * The additive PATCH must send these rather than a form-vs-roster diff, which would resend + * stale values for fields another user changed while a conflict was being resolved. + */ +function pickEditedCompanyFields(dirtyFields: Record): EditedCompanyFields { + const edited: EditedCompanyFields = {}; + for (const field of EDITABLE_COMPANY_FIELDS) { + if (dirtyFields[field] === true) edited[field] = true; + } + return edited; +} import { toFormValues, useVendorRosterSelection } from "./use-vendor-roster-selection"; import { useVendorFacets } from "@/domain/vendors/use-cases/use-vendor-facets"; import { isVendorRosterConflictError } from "@/domain/vendors/lib/vendor-roster-conflict"; @@ -106,6 +133,10 @@ export function useVendorRosterForm({ mode: "onChange", }); const { control, handleSubmit, reset, formState } = form; + // react-hook-form exposes formState through a Proxy that only tracks properties read during + // render. Reading dirtyFields inside the submit callback would not subscribe and would come + // back empty, so it is resolved here on every render. + const editedCompanyFields = pickEditedCompanyFields(formState.dirtyFields); const watched = useWatch({ control }); const clearConflict = useCallback(() => setConflict(null), []); @@ -136,7 +167,7 @@ export function useVendorRosterForm({ const values = withoutBlankNewTechnicians(formValues); if (mode === "create" && selection.selectedRoster != null) { const selected = selection.selectedRoster; - if (!buildAdditiveRosterPatch(selected, values, selected.rowVersion)) { + if (!buildAdditiveRosterPatch(selected, values, selected.rowVersion, editedCompanyFields)) { toast.error("Enter at least one technician to add to this vendor."); return; } @@ -147,6 +178,7 @@ export function useVendorRosterForm({ companyId: selected.companyId, rowVersion: selected.rowVersion, originalRoster: selected, + editedCompanyFields, }, { onSuccess: (data) => onSuccess?.(data), diff --git a/src/domain/vendors/use-cases/use-save-vendor-company-roster.ts b/src/domain/vendors/use-cases/use-save-vendor-company-roster.ts index 1d1c4865..ca7fa247 100644 --- a/src/domain/vendors/use-cases/use-save-vendor-company-roster.ts +++ b/src/domain/vendors/use-cases/use-save-vendor-company-roster.ts @@ -16,8 +16,12 @@ export interface SaveVendorCompanyRosterInput { companyId?: string | number | null; rowVersion?: string; originalRoster?: VendorCompanyRoster; + /** Company fields the user actually edited. See buildAdditiveRosterPatch. */ + editedCompanyFields?: EditedCompanyFields; } +export type EditedCompanyFields = Partial>; + export interface SaveVendorCompanyRosterContext { conflict?: unknown; } @@ -92,10 +96,20 @@ function hasTechnicianContent(technician: { ); } +/** + * Builds the additive PATCH body for the Add flow. + * + * `editedCompanyFields` must carry the fields the user actually touched. A plain + * form-vs-roster diff is unsafe here: after a stale-409 the roster is refetched while the form + * keeps the values loaded before the conflict, so a field another user changed in that window + * would look "different" and be sent back with our stale value, silently reverting their edit. + * Only fields the user edited are ever transmitted. + */ export function buildAdditiveRosterPatch( roster: VendorCompanyRoster | null | undefined, values: VendorCompanyRosterFormValues, rowVersion: string, + editedCompanyFields: EditedCompanyFields = {}, ): VendorRosterAdditivePatchInput | null { const addTechnicians = values.technicians.filter( (technician) => technician.id == null && hasTechnicianContent(technician), @@ -103,6 +117,7 @@ export function buildAdditiveRosterPatch( const companyFields: Record = {}; if (roster) { for (const field of COMPANY_FIELDS) { + if (editedCompanyFields[field] !== true) continue; if (values[field] !== roster[field]) companyFields[field] = values[field]; } } @@ -134,6 +149,7 @@ export function useSaveVendorCompanyRoster(): UseMutationResult< companyId, rowVersion, originalRoster, + editedCompanyFields, }: SaveVendorCompanyRosterInput) => { if (mode === "update") { const statusChange = originalRoster @@ -162,7 +178,12 @@ export function useSaveVendorCompanyRoster(): UseMutationResult< throw new Error("Company id is required to add technicians"); } if (!rowVersion) throw new Error("Row version is required to add technicians"); - const patch = buildAdditiveRosterPatch(originalRoster, values, rowVersion); + const patch = buildAdditiveRosterPatch( + originalRoster, + values, + rowVersion, + editedCompanyFields, + ); if (!patch) throw new Error("Nothing to add to this vendor company"); return vendorCompanyRosterApi.addTechnicians(companyId, patch); } diff --git a/src/test/domain/vendors/use-cases/use-save-vendor-company-roster-additive.test.tsx b/src/test/domain/vendors/use-cases/use-save-vendor-company-roster-additive.test.tsx index 61c431e6..f03c2a27 100644 --- a/src/test/domain/vendors/use-cases/use-save-vendor-company-roster-additive.test.tsx +++ b/src/test/domain/vendors/use-cases/use-save-vendor-company-roster-additive.test.tsx @@ -112,16 +112,45 @@ describe("buildAdditiveRosterPatch", () => { expect(patch?.addTechnicians).toEqual([newTechnician]); }); - it("sends only changed company fields", () => { + it("sends only company fields the user edited", () => { const patch = buildAdditiveRosterPatch( roster, formValues({ notes: "New notes", address: "2 Oak Ave" }), "rv-1", + { notes: true, address: true }, ); expect(patch?.companyFields).toEqual({ notes: "New notes", address: "2 Oak Ave" }); }); + it("never sends a company field the user did not edit, even when it differs from the roster", () => { + // Regression: after a stale-409 the roster is refetched while the form keeps pre-conflict + // values. A form-vs-roster diff would resend our stale value for a field another user changed + // in that window, silently reverting their edit. Only edited fields may be transmitted. + const rosterAfterOtherUserEditedPhone = { ...roster, companyPhone: "314-555-9999" }; + + const patch = buildAdditiveRosterPatch( + rosterAfterOtherUserEditedPhone, + formValues({ companyPhone: roster.companyPhone, notes: "New notes" }), + "rv-2", + { notes: true }, + ); + + expect(patch?.companyFields).toEqual({ notes: "New notes" }); + expect(patch?.companyFields).not.toHaveProperty("companyPhone"); + }); + + it("omits companyFields when values differ but nothing was edited", () => { + const patch = buildAdditiveRosterPatch( + roster, + formValues({ notes: "Drifted under us" }), + "rv-1", + {}, + ); + + expect(patch).not.toHaveProperty("companyFields"); + }); + it("omits companyFields entirely when nothing changed", () => { const patch = buildAdditiveRosterPatch(roster, formValues(), "rv-1"); From 9a066092b3c619124e21c4f84d75b16f817c468f Mon Sep 17 00:00:00 2001 From: Codex Review Integration Date: Tue, 18 Aug 2026 17:28:48 -0300 Subject: [PATCH 4/5] fix(vendors): correct additive add reload, phone and contact (SH-250, SH-246) Three review findings on the additive add path: - Create-mode reload refreshed selectedRoster (including rowVersion) but left the form company fields on their pre-conflict values, so a retry diffed stale fields against the reloaded roster and could overwrite the concurrent company update that caused the 409. Reload now resets the form from the fresh roster, matching the select path. - mapVendorRosterAdditivePatchToBackend copied companyFields verbatim while the reconcile write mapper canonicalizes companyPhone, so PATCH and PUT could send different phone shapes for the same input. - emptyRosterTechnician seeded preferredContact "Phone" even though that control was removed from the form, so filling the first blank row submitted a fabricated value. append already omitted it; both paths now agree. --- .../use-vendor-roster-selection.ts | 7 +++- .../vendors/mappers/vendor-roster-mapper.ts | 10 +++++- .../vendors/schemas/vendor-roster-schema.ts | 5 ++- .../vendors/use-vendor-roster-form.test.tsx | 35 ++++++++++++++++++- 4 files changed, 53 insertions(+), 4 deletions(-) diff --git a/src/app/(protected)/vendors/_components/use-vendor-roster-selection.ts b/src/app/(protected)/vendors/_components/use-vendor-roster-selection.ts index 4eb141b1..4bd467f1 100644 --- a/src/app/(protected)/vendors/_components/use-vendor-roster-selection.ts +++ b/src/app/(protected)/vendors/_components/use-vendor-roster-selection.ts @@ -153,6 +153,11 @@ export function useVendorRosterSelection({ void fetchRosterByCompany(selectedRoster.companyId) .then((roster) => { if (requestIdRef.current !== requestId) return; + // SH-250: refresh the visible company fields alongside rowVersion. + // Refreshing only selectedRoster left the form on pre-conflict values, + // so a retry diffed stale fields against the reloaded roster and could + // overwrite the concurrent company update that caused the 409. + reset(withCompanyFieldsOnly(roster)); setSelectedRoster(roster); retryTargetRef.current = null; }) @@ -161,7 +166,7 @@ export function useVendorRosterSelection({ setLoadErrorMessage(RELOAD_ERROR_MESSAGE); }); } - }, [clearConflict, fetchRosterByCompany, mode, query, selectedRoster]); + }, [clearConflict, fetchRosterByCompany, mode, query, reset, selectedRoster]); const retryLoad = useCallback(() => { const target = retryTargetRef.current; diff --git a/src/domain/vendors/mappers/vendor-roster-mapper.ts b/src/domain/vendors/mappers/vendor-roster-mapper.ts index e4de67dd..641042ef 100644 --- a/src/domain/vendors/mappers/vendor-roster-mapper.ts +++ b/src/domain/vendors/mappers/vendor-roster-mapper.ts @@ -152,7 +152,15 @@ export function mapVendorRosterAdditivePatchToBackend( return mapped; }), }; - if (patch.companyFields) payload.companyFields = { ...patch.companyFields }; + if (patch.companyFields) { + // SH-250: match the reconcile write path, which canonicalizes companyPhone. + // Copying companyFields verbatim let additive patches send a different phone + // shape than PUT for the same user input. + const companyFields = { ...patch.companyFields }; + if (companyFields.companyPhone !== undefined) + companyFields.companyPhone = toCanonicalPhone(companyFields.companyPhone); + payload.companyFields = companyFields; + } return payload; } diff --git a/src/domain/vendors/schemas/vendor-roster-schema.ts b/src/domain/vendors/schemas/vendor-roster-schema.ts index a429d69e..9358b12b 100644 --- a/src/domain/vendors/schemas/vendor-roster-schema.ts +++ b/src/domain/vendors/schemas/vendor-roster-schema.ts @@ -83,11 +83,14 @@ export const vendorCompanyRosterUpdateSchema = vendorCompanyRosterSchema.and( export type VendorCompanyRosterUpdateValues = z.infer; +// SH-250: the preferred-contact control was removed from the form, so seeding a +// value here fabricated one on submit. `append` already omits it; the schema +// keeps it optional, so both paths now agree and the backend receives it only +// when a real value exists. export const emptyRosterTechnician: RosterTechnicianValues = { contactName: "", phone: "", email: "", - preferredContact: "Phone", tradeSpecialties: "", isActive: true, }; diff --git a/src/test/app/(protected)/vendors/use-vendor-roster-form.test.tsx b/src/test/app/(protected)/vendors/use-vendor-roster-form.test.tsx index 80091636..777363fd 100644 --- a/src/test/app/(protected)/vendors/use-vendor-roster-form.test.tsx +++ b/src/test/app/(protected)/vendors/use-vendor-roster-form.test.tsx @@ -503,6 +503,37 @@ describe("useVendorRosterForm additive add flow (SH-250)", () => { ); }); + it("refreshes the visible company fields on reload, not just rowVersion", async () => { + rosterGet.mockResolvedValueOnce(roster); + const { result } = renderHook( + () => { + const form = useVendorRosterForm({ mode: "create", startWithTechnician: true }); + const name = useController({ control: form.control, name: "name" }); + return { form, name }; + }, + { wrapper: makeWrapper(createClient()) }, + ); + + await act(async () => { + await result.current.form.selectCompany(company); + }); + expect(result.current.name.field.value).toBe("Gateway Plumbing"); + + // Someone else renamed the company, which is what caused the 409. + rosterGet.mockResolvedValueOnce({ + ...roster, + rowVersion: "rv-2", + name: "Gateway Plumbing & Drain", + }); + await act(async () => { + result.current.form.reload(); + }); + + // Reload must not leave the form on the pre-conflict name, or the retry + // would diff stale fields and overwrite the concurrent update. + await waitFor(() => expect(result.current.name.field.value).toBe("Gateway Plumbing & Drain")); + }); + it("does not regress the Edit Vendor reconcile path (mode update)", () => { rosterQueryResult = { data: rosterWithTechnicians, @@ -562,11 +593,13 @@ describe("useVendorRosterForm technician autopopulation (SH-246)", () => { contactName: "", phone: "", email: "", - preferredContact: "Phone", tradeSpecialties: "", isActive: true, }, ]); + // SH-250: the blank row must not carry a fabricated preferredContact, since + // the control was removed from the form and `append` omits it too. + expect(technicians[0]).not.toHaveProperty("preferredContact"); expect(technicians).not.toContainEqual(expect.objectContaining({ contactName: "Ray Holt" })); expect(technicians).not.toContainEqual( expect.objectContaining({ contactName: "Amy Santiago" }), From 1305bf35dd5a3e9aee02c4e5c7f117078e3993fb Mon Sep 17 00:00:00 2001 From: Codex Review Integration Date: Wed, 19 Aug 2026 10:26:28 -0300 Subject: [PATCH 5/5] fix(vendors): bypass the query cache when reloading after a conflict (SH-250) The reload fix re-seeded the form from queryClient.fetchQuery, which inherits the global 5-minute staleTime. A 409 never invalidates that key - useSaveVendorCompanyRoster invalidates only in onSuccess and its onError returns early for conflicts - and the Add flow has no mounted observer on it, so reload returned the cached roster with the same stale rowVersion and every retry 409'd again. Update mode was unaffected because it recovers via query.refetch(), which bypasses staleTime. Pass staleTime: 0 on that fetch, since its sole purpose is the freshest rowVersion. The regression test uses a client with the app's real staleTime; the default test client uses 0, which masked this entirely. --- .../use-vendor-roster-selection.ts | 7 +++ .../vendors/use-vendor-roster-form.test.tsx | 46 +++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/src/app/(protected)/vendors/_components/use-vendor-roster-selection.ts b/src/app/(protected)/vendors/_components/use-vendor-roster-selection.ts index 4bd467f1..24d51ce8 100644 --- a/src/app/(protected)/vendors/_components/use-vendor-roster-selection.ts +++ b/src/app/(protected)/vendors/_components/use-vendor-roster-selection.ts @@ -87,6 +87,13 @@ export function useVendorRosterSelection({ queryClient.fetchQuery({ queryKey: queryKeys.vendors.roster("companyId", id), queryFn: () => vendorCompanyRosterApi.get({ companyId: id }), + // SH-250: this fetch exists to obtain the freshest rowVersion, so it must not + // serve the global 5-minute staleTime cache. A conflict never invalidates this + // key (useSaveVendorCompanyRoster invalidates only on success, and its onError + // returns early for conflicts), and the Add flow has no mounted observer on it. + // Without staleTime: 0 the Reload button re-seeds the same stale rowVersion and + // the retry 409s again indefinitely. + staleTime: 0, }), [queryClient], ); diff --git a/src/test/app/(protected)/vendors/use-vendor-roster-form.test.tsx b/src/test/app/(protected)/vendors/use-vendor-roster-form.test.tsx index 777363fd..b56aa714 100644 --- a/src/test/app/(protected)/vendors/use-vendor-roster-form.test.tsx +++ b/src/test/app/(protected)/vendors/use-vendor-roster-form.test.tsx @@ -156,6 +156,17 @@ function createClient(): QueryClient { return new QueryClient({ defaultOptions: { queries: { retry: false } } }); } +/** + * SH-250: mirrors the app's real staleTime (src/lib/query/query-client.ts). The default + * test client uses staleTime 0, which silently masks cache-staleness defects on the + * reload path — the conflict recovery must not depend on an empty cache. + */ +function createCachingClient(): QueryClient { + return new QueryClient({ + defaultOptions: { queries: { retry: false, staleTime: 5 * 60 * 1000 } }, + }); +} + function makeWrapper(client: QueryClient) { return function Wrapper({ children }: { children: ReactNode }) { return {children}; @@ -534,6 +545,41 @@ describe("useVendorRosterForm additive add flow (SH-250)", () => { await waitFor(() => expect(result.current.name.field.value).toBe("Gateway Plumbing & Drain")); }); + it("reload bypasses the cache so the retry carries the fresh rowVersion", async () => { + rosterGet.mockResolvedValueOnce(roster); + const { result } = renderHook( + () => useVendorRosterForm({ mode: "create", startWithTechnician: true }), + { wrapper: makeWrapper(createCachingClient()) }, + ); + + await act(async () => { + await result.current.selectCompany(company); + }); + expect(rosterGet).toHaveBeenCalledTimes(1); + + act(() => { + result.current.submit(submitValues([newTechnician])); + }); + expect(saveMutate.mock.calls[0]?.[0]).toEqual(expect.objectContaining({ rowVersion: "rv-1" })); + + // The 409 never invalidates this key, and in the Add flow nothing else observes it. + // Without staleTime: 0 on the reload fetch, the cached rv-1 is served straight back + // and every retry 409s again. + rosterGet.mockResolvedValueOnce({ ...roster, rowVersion: "rv-2" }); + await act(async () => { + result.current.reload(); + }); + + await waitFor(() => expect(rosterGet).toHaveBeenCalledTimes(2)); + + act(() => { + result.current.submit(submitValues([newTechnician])); + }); + expect(saveMutate.mock.calls[1]?.[0]).toEqual( + expect.objectContaining({ mode: "add", rowVersion: "rv-2" }), + ); + }); + it("does not regress the Edit Vendor reconcile path (mode update)", () => { rosterQueryResult = { data: rosterWithTechnicians,