From 9a066092b3c619124e21c4f84d75b16f817c468f Mon Sep 17 00:00:00 2001 From: Codex Review Integration Date: Tue, 18 Aug 2026 17:28:48 -0300 Subject: [PATCH] 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" }),