fix(framework-editor): prevent silent loss of new requirements

apps/framework-editor/app/(pages)/controls/ControlsClientPage.tsx

@@ -41,21 +41,37 @@ async function fetchAllPolicyTemplates(): Promise<RelationalItem[]> {
   return apiClient<RelationalItem[]>('/policy-template');
 }
 
+function toRequirementItem(r: RequirementApiItem): RelationalItem {
+  let displayName = r.identifier;
+  if (r.identifier && r.name) {
+    displayName = `${r.identifier} - ${r.name}`;
+  } else if (r.name) {
+    displayName = r.name;
+  }
+  return {
+    id: r.id,
+    name: displayName || 'Unnamed Requirement',
+    sublabel: r.framework?.name,
+  };
+}
+
 async function fetchAllRequirements(): Promise<RelationalItem[]> {
   const reqs = await apiClient<RequirementApiItem[]>('/requirement');
-  return reqs.map((r) => {
-    let displayName = r.identifier;
-    if (r.identifier && r.name) {
-      displayName = `${r.identifier} - ${r.name}`;
-    } else if (r.name) {
-      displayName = r.name;
-    }
-    return {
-      id: r.id,
-      name: displayName || 'Unnamed Requirement',
-      sublabel: r.framework?.name,
-    };
-  });
+  return reqs.map(toRequirementItem);
+}
+
+// On a framework's Controls tab only this framework's requirements are
+// linkable — links to them are what makes a control show up on the tab.
+async function fetchRequirementsForFramework(
+  frameworkId: string,
+): Promise<RelationalItem[]> {
+  const framework = await apiClient<{
+    name: string;
+    requirements: Array<{ id: string; name: string; identifier: string }>;
+  }>(`/framework/${frameworkId}`);
+  return framework.requirements.map((r) =>
+    toRequirementItem({ ...r, framework: { name: framework.name } }),
+  );
 }
 
 async function fetchAllTaskTemplates(): Promise<RelationalItem[]> {
@@ -115,6 +131,17 @@ export function ControlsClientPage({ initialControls, emptyMessage, frameworkId
         apiClient(`/control-template/${id}`, {
           method: 'DELETE',
         }),
+      linkRequirement: (controlId: string, requirementId: string) =>
+        linkControlRelation(controlId, 'requirements', requirementId),
+      // Policy/task links are framework-scoped; only offered on a framework tab.
+      ...(frameworkId
+        ? {
+            linkPolicyTemplate: (controlId: string, policyTemplateId: string) =>
+              linkControlRelation(controlId, 'policy-templates', policyTemplateId, frameworkId),
+            linkTaskTemplate: (controlId: string, taskTemplateId: string) =>
+              linkControlRelation(controlId, 'task-templates', taskTemplateId, frameworkId),
+          }
+        : {}),
     }),
     [frameworkId],
   );
@@ -157,7 +184,9 @@ export function ControlsClientPage({ initialControls, emptyMessage, frameworkId
     isDirty,
     createdIds,
     changesSummary,
-  } = useChangeTracking(initialGridData, mutations);
+  } = useChangeTracking(initialGridData, mutations, {
+    requireRequirementLink: !!frameworkId,
+  });
 
   const {
     families,
@@ -175,6 +204,12 @@ export function ControlsClientPage({ initialControls, emptyMessage, frameworkId
     [updateCell],
   );
 
+  const getRequirementItems = useCallback(
+    () =>
+      frameworkId ? fetchRequirementsForFramework(frameworkId) : fetchAllRequirements(),
+    [frameworkId],
+  );
+
   const columns = useMemo(
     () => [
       columnHelper.accessor('name', {
@@ -226,6 +261,7 @@ export function ControlsClientPage({ initialControls, emptyMessage, frameworkId
               items={getValue()}
               rowId={row.original.id}
               isNewRow={createdIds.has(row.original.id)}
+              allowSelectOnNewRows={!!frameworkId}
               getAllItems={fetchAllPolicyTemplates}
               onLink={(controlId: string, ptId: string) =>
                 linkControlRelation(controlId, 'policy-templates', ptId, frameworkId)
@@ -252,7 +288,8 @@ export function ControlsClientPage({ initialControls, emptyMessage, frameworkId
               items={getValue()}
               rowId={row.original.id}
               isNewRow={createdIds.has(row.original.id)}
-              getAllItems={fetchAllRequirements}
+              allowSelectOnNewRows
+              getAllItems={getRequirementItems}
               onLink={(controlId: string, reqId: string) =>
                 linkControlRelation(controlId, 'requirements', reqId)
               }
@@ -278,6 +315,7 @@ export function ControlsClientPage({ initialControls, emptyMessage, frameworkId
               items={getValue()}
               rowId={row.original.id}
               isNewRow={createdIds.has(row.original.id)}
+              allowSelectOnNewRows={!!frameworkId}
               getAllItems={fetchAllTaskTemplates}
               onLink={(controlId: string, ttId: string) =>
                 linkControlRelation(controlId, 'task-templates', ttId, frameworkId)
@@ -346,7 +384,7 @@ export function ControlsClientPage({ initialControls, emptyMessage, frameworkId
         ),
       }),
     ],
-    [uniqueFamilies, updateCell, updateRelational, deleteRow, createdIds, handleDocumentTypesUpdate, frameworkId],
+    [uniqueFamilies, updateCell, updateRelational, deleteRow, createdIds, handleDocumentTypesUpdate, frameworkId, getRequirementItems],
   );
 
   const [sorting, setSorting] = useState<SortingState>([]);

apps/framework-editor/app/(pages)/controls/hooks/useChangeTracking.test.tsx

@@ -0,0 +1,187 @@
+import { act, renderHook } from '@testing-library/react';
+import { beforeEach, describe, expect, it, vi } from 'vitest';
+import type { ControlsPageGridData } from '../types';
+import { useChangeTracking, type ControlMutations } from './useChangeTracking';
+
+const { refreshMock, toastMock } = vi.hoisted(() => ({
+  refreshMock: vi.fn(),
+  toastMock: { success: vi.fn(), error: vi.fn() },
+}));
+
+vi.mock('next/navigation', () => ({
+  useRouter: () => ({ refresh: refreshMock }),
+}));
+vi.mock('sonner', () => ({ toast: toastMock }));
+
+// Stable reference: the hook resets its state whenever the initialData
+// identity changes, so a fresh [] per render would loop forever.
+const NO_ROWS: ControlsPageGridData[] = [];
+
+function makeRow(overrides: Partial<ControlsPageGridData> = {}): ControlsPageGridData {
+  return {
+    id: 'temp-1',
+    name: 'New Control',
+    description: '',
+    controlFamily: 'Access Control',
+    policyTemplates: [],
+    requirements: [],
+    taskTemplates: [],
+    documentTypes: [],
+    policyTemplatesLength: 0,
+    requirementsLength: 0,
+    taskTemplatesLength: 0,
+    documentTypesLength: 0,
+    createdAt: new Date(),
+    updatedAt: new Date(),
+    ...overrides,
+  };
+}
+
+function makeMutations(): ControlMutations {
+  return {
+    createControl: vi.fn(async () => ({ id: 'frk_ct_new' })),
+    updateControl: vi.fn(async () => ({})),
+    deleteControl: vi.fn(async () => ({})),
+    linkRequirement: vi.fn(async () => ({})),
+    linkPolicyTemplate: vi.fn(async () => ({})),
+    linkTaskTemplate: vi.fn(async () => ({})),
+  };
+}
+
+describe('useChangeTracking handleCommit', () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+  });
+
+  it('blocks creating an unlinked control when requireRequirementLink is set', async () => {
+    const mutations = makeMutations();
+    const { result } = renderHook(() =>
+      useChangeTracking(NO_ROWS, mutations, { requireRequirementLink: true }),
+    );
+
+    act(() => {
+      result.current.addRow(makeRow());
+    });
+    await act(async () => {
+      await result.current.handleCommit();
+    });
+
+    expect(mutations.createControl).not.toHaveBeenCalled();
+    expect(toastMock.error).toHaveBeenCalledWith(
+      'Some operations failed',
+      expect.objectContaining({
+        description: expect.stringContaining('link at least one requirement'),
+      }),
+    );
+    // The row stays in the grid so the user can fix it instead of losing it.
+    expect(result.current.isDirty).toBe(true);
+    expect(result.current.data).toHaveLength(1);
+    expect(refreshMock).not.toHaveBeenCalled();
+  });
+
+  it('creates and links requirements, policies and tasks picked on the new row', async () => {
+    const mutations = makeMutations();
+    const { result } = renderHook(() =>
+      useChangeTracking(NO_ROWS, mutations, { requireRequirementLink: true }),
+    );
+
+    act(() => {
+      result.current.addRow(
+        makeRow({
+          requirements: [{ id: 'req_1', name: '10.3 - Access Review' }],
+          requirementsLength: 1,
+          policyTemplates: [{ id: 'pol_1', name: 'Access Policy' }],
+          policyTemplatesLength: 1,
+          taskTemplates: [{ id: 'task_1', name: 'Review accounts' }],
+          taskTemplatesLength: 1,
+        }),
+      );
+    });
+    await act(async () => {
+      await result.current.handleCommit();
+    });
+
+    expect(mutations.createControl).toHaveBeenCalledWith({
+      name: 'New Control',
+      description: '',
+      controlFamily: 'Access Control',
+      documentTypes: [],
+    });
+    expect(mutations.linkRequirement).toHaveBeenCalledWith('frk_ct_new', 'req_1');
+    expect(mutations.linkPolicyTemplate).toHaveBeenCalledWith('frk_ct_new', 'pol_1');
+    expect(mutations.linkTaskTemplate).toHaveBeenCalledWith('frk_ct_new', 'task_1');
+    // Links picked before commit stay visible after the id swap.
+    expect(result.current.data[0].id).toBe('frk_ct_new');
+    expect(result.current.data[0].requirements).toHaveLength(1);
+    expect(result.current.isDirty).toBe(false);
+    expect(refreshMock).toHaveBeenCalled();
+  });
+
+  it('allows unlinked controls when requireRequirementLink is not set (global page)', async () => {
+    const mutations = makeMutations();
+    const { result } = renderHook(() => useChangeTracking(NO_ROWS, mutations));
+
+    act(() => {
+      result.current.addRow(makeRow());
+    });
+    await act(async () => {
+      await result.current.handleCommit();
+    });
+
+    expect(mutations.createControl).toHaveBeenCalled();
+    expect(result.current.isDirty).toBe(false);
+    expect(refreshMock).toHaveBeenCalled();
+  });
+
+  it('reports an error instead of silently skipping rows without a name', async () => {
+    const mutations = makeMutations();
+    const { result } = renderHook(() => useChangeTracking(NO_ROWS, mutations));
+
+    act(() => {
+      result.current.addRow(makeRow({ name: '' }));
+    });
+    await act(async () => {
+      await result.current.handleCommit();
+    });
+
+    expect(mutations.createControl).not.toHaveBeenCalled();
+    expect(toastMock.error).toHaveBeenCalledWith(
+      'Some operations failed',
+      expect.objectContaining({
+        description: expect.stringContaining('name is required'),
+      }),
+    );
+    expect(result.current.isDirty).toBe(true);
+  });
+
+  it('surfaces link failures after a successful create', async () => {
+    const mutations = makeMutations();
+    mutations.linkRequirement = vi.fn(async () => {
+      throw new Error('link failed');
+    });
+    const { result } = renderHook(() =>
+      useChangeTracking(NO_ROWS, mutations, { requireRequirementLink: true }),
+    );
+
+    act(() => {
+      result.current.addRow(
+        makeRow({
+          requirements: [{ id: 'req_1', name: '10.3 - Access Review' }],
+          requirementsLength: 1,
+        }),
+      );
+    });
+    await act(async () => {
+      await result.current.handleCommit();
+    });
+
+    expect(result.current.data[0].id).toBe('frk_ct_new');
+    expect(toastMock.error).toHaveBeenCalledWith(
+      'Some operations failed',
+      expect.objectContaining({
+        description: expect.stringContaining('failed to link'),
+      }),
+    );
+    expect(refreshMock).not.toHaveBeenCalled();
+  });
+});