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

@@ -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",

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<string, unknown>): 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),

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<Record<(typeof COMPANY_FIELDS)[number], boolean>>;
+
 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<string, string> = {};
   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);
       }

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");