From e231fcc31d16d1e561574f41309784aa8086f3b3 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Wed, 10 Jun 2026 10:41:57 -0400 Subject: [PATCH 1/2] fix(framework-editor): prevent silent loss of new requirements and controls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A control created from a framework's Controls tab was created as a global template with no link to the framework, while the tab only lists controls linked to the framework's requirements — so committed controls silently vanished on the next visit. Uncommitted grid rows were also discarded without warning on tab navigation, and rows that failed (or were skipped for a missing name) stayed in the grid looking committed. - RelationalCell: allow picking links on uncommitted rows (opt-in allowSelectOnNewRows); both grids persist those links on commit - controls grid: on a framework tab, block committing a control with zero requirement links with an actionable error instead of letting it vanish - framework tab: requirement picker on new control rows is scoped to the framework's own requirements - both grids: empty-name rows now produce a commit error instead of being silently skipped; fully-successful commits router.refresh() to re-sync - FrameworkTabs + beforeunload: confirm before discarding uncommitted rows - AddExistingItemDialog: surface the API's real error message (e.g. "Framework has no requirements to link the control to") - add vitest setup for framework-editor + 15 unit tests Co-Authored-By: Claude Fable 5 --- .../(pages)/controls/ControlsClientPage.tsx | 70 +++++-- .../controls/hooks/useChangeTracking.test.tsx | 187 ++++++++++++++++++ .../controls/hooks/useChangeTracking.ts | 90 +++++++-- .../FrameworkRequirementsClientPage.tsx | 1 + .../[frameworkId]/FrameworkTabs.tsx | 10 +- .../useRequirementChangeTracking.test.tsx | 166 ++++++++++++++++ .../hooks/useRequirementChangeTracking.ts | 39 +++- .../app/components/AddExistingItemDialog.tsx | 24 ++- .../components/table/RelationalCell.test.tsx | 88 +++++++++ .../app/components/table/RelationalCell.tsx | 7 +- .../app/lib/unsaved-changes.test.tsx | 45 +++++ .../app/lib/unsaved-changes.ts | 42 ++++ apps/framework-editor/package.json | 7 +- apps/framework-editor/vitest.config.ts | 18 ++ apps/framework-editor/vitest.setup.ts | 7 + bun.lock | 4 + 16 files changed, 766 insertions(+), 39 deletions(-) create mode 100644 apps/framework-editor/app/(pages)/controls/hooks/useChangeTracking.test.tsx create mode 100644 apps/framework-editor/app/(pages)/frameworks/[frameworkId]/hooks/useRequirementChangeTracking.test.tsx create mode 100644 apps/framework-editor/app/components/table/RelationalCell.test.tsx create mode 100644 apps/framework-editor/app/lib/unsaved-changes.test.tsx create mode 100644 apps/framework-editor/app/lib/unsaved-changes.ts create mode 100644 apps/framework-editor/vitest.config.ts create mode 100644 apps/framework-editor/vitest.setup.ts diff --git a/apps/framework-editor/app/(pages)/controls/ControlsClientPage.tsx b/apps/framework-editor/app/(pages)/controls/ControlsClientPage.tsx index f4f5d17172..326df586d5 100644 --- a/apps/framework-editor/app/(pages)/controls/ControlsClientPage.tsx +++ b/apps/framework-editor/app/(pages)/controls/ControlsClientPage.tsx @@ -41,21 +41,37 @@ async function fetchAllPolicyTemplates(): Promise { return apiClient('/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 { const reqs = await apiClient('/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 { + 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 { @@ -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([]); diff --git a/apps/framework-editor/app/(pages)/controls/hooks/useChangeTracking.test.tsx b/apps/framework-editor/app/(pages)/controls/hooks/useChangeTracking.test.tsx new file mode 100644 index 0000000000..c981130b3a --- /dev/null +++ b/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 { + 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(); + }); +}); diff --git a/apps/framework-editor/app/(pages)/controls/hooks/useChangeTracking.ts b/apps/framework-editor/app/(pages)/controls/hooks/useChangeTracking.ts index 3b987eb1a7..0cf0d3dacd 100644 --- a/apps/framework-editor/app/(pages)/controls/hooks/useChangeTracking.ts +++ b/apps/framework-editor/app/(pages)/controls/hooks/useChangeTracking.ts @@ -1,3 +1,5 @@ +import { useUnsavedChangesGuard } from '@/app/lib/unsaved-changes'; +import { useRouter } from 'next/navigation'; import { useCallback, useEffect, useMemo, useState } from 'react'; import { toast } from 'sonner'; import type { ControlsPageGridData } from '../types'; @@ -16,12 +18,29 @@ export interface ControlMutations { data: { name: string; description: string; controlFamily: string | null; documentTypes: string[] }, ) => Promise; deleteControl: (id: string) => Promise; + /** Persist links picked on uncommitted rows. Optional per page context. */ + linkRequirement?: (controlId: string, requirementId: string) => Promise; + linkPolicyTemplate?: (controlId: string, policyTemplateId: string) => Promise; + linkTaskTemplate?: (controlId: string, taskTemplateId: string) => Promise; +} + +export interface ChangeTrackingOptions { + /** + * On a framework's Controls tab the listing only shows controls linked to + * one of the framework's requirements — committing an unlinked control + * would make it silently vanish from the tab. When set, new rows must + * have at least one requirement link before they can be committed. + */ + requireRequirementLink?: boolean; } export const useChangeTracking = ( initialData: ControlsPageGridData[], mutations: ControlMutations, + options: ChangeTrackingOptions = {}, ) => { + const router = useRouter(); + const { requireRequirementLink = false } = options; const [data, setData] = useState(() => initialData); const [prevData, setPrevData] = useState(() => initialData); @@ -179,7 +198,17 @@ export const useChangeTracking = ( for (const tempId of createdIds) { const row = currentData.find((r) => r.id === tempId); - if (!row?.name) continue; + if (!row) continue; + if (!row.name?.trim()) { + results.errors.push('New control: name is required'); + continue; + } + if (requireRequirementLink && row.requirements.length === 0) { + results.errors.push( + `"${row.name}": link at least one requirement (Linked Requirements column) — controls only appear on this framework's page through their requirement links`, + ); + continue; + } try { const newControl = await mutations.createControl({ @@ -188,21 +217,39 @@ export const useChangeTracking = ( controlFamily: row.controlFamily, documentTypes: row.documentTypes, }); - results.successes.push(`Created: ${row.name}`); + + // Persist links the user picked on the uncommitted row. + const failedLinks: string[] = []; + const linkSelections = [ + { items: row.requirements, link: mutations.linkRequirement, noun: 'requirement' }, + { items: row.policyTemplates, link: mutations.linkPolicyTemplate, noun: 'policy' }, + { items: row.taskTemplates, link: mutations.linkTaskTemplate, noun: 'task' }, + ]; + for (const { items, link, noun } of linkSelections) { + for (const item of items) { + if (!link) { + failedLinks.push(`${noun} "${item.name}"`); + continue; + } + try { + await link(newControl.id, item.id); + } catch { + failedLinks.push(`${noun} "${item.name}"`); + } + } + } + + if (failedLinks.length > 0) { + results.errors.push( + `Created "${row.name}" but failed to link: ${failedLinks.join(', ')}`, + ); + } else { + results.successes.push(`Created: ${row.name}`); + } successfullyCreatedIds.add(tempId); setData((prev) => - prev.map((r) => - r.id === tempId - ? { - ...r, - id: newControl.id, - policyTemplates: [], - requirements: [], - taskTemplates: [], - } - : r, - ), + prev.map((r) => (r.id === tempId ? { ...r, id: newControl.id } : r)), ); } catch (error) { results.errors.push( @@ -275,11 +322,22 @@ export const useChangeTracking = ( toast.success('Changes saved', { description: `${results.successes.length} operation(s) completed`, }); + // Re-sync the grid with server truth (ids, timestamps, links). + router.refresh(); } } finally { setIsCommitting(false); } - }, [data, createdIds, updatedIds, deletedIds, mutations, isCommitting]); + }, [ + data, + createdIds, + updatedIds, + deletedIds, + mutations, + isCommitting, + requireRequirementLink, + router, + ]); const handleCancel = useCallback(() => { if (isCommitting) return; @@ -293,6 +351,10 @@ export const useChangeTracking = ( return createdIds.size > 0 || updatedIds.size > 0 || deletedIds.size > 0; }, [createdIds, updatedIds, deletedIds]); + // Uncommitted rows only live in this grid's state — warn before they are + // discarded by a reload or a tab switch. + useUnsavedChangesGuard('controls-grid', isDirty); + const changesSummary = useMemo(() => { const total = createdIds.size + updatedIds.size + deletedIds.size; if (total === 0) return ''; diff --git a/apps/framework-editor/app/(pages)/frameworks/[frameworkId]/FrameworkRequirementsClientPage.tsx b/apps/framework-editor/app/(pages)/frameworks/[frameworkId]/FrameworkRequirementsClientPage.tsx index a20650d1e1..d842b1b353 100644 --- a/apps/framework-editor/app/(pages)/frameworks/[frameworkId]/FrameworkRequirementsClientPage.tsx +++ b/apps/framework-editor/app/(pages)/frameworks/[frameworkId]/FrameworkRequirementsClientPage.tsx @@ -174,6 +174,7 @@ export function FrameworkRequirementsClientPage({ items={getValue()} rowId={row.original.id} isNewRow={createdIds.has(row.original.id)} + allowSelectOnNewRows getAllItems={fetchAllControlTemplates} onLink={linkControlToRequirement} onUnlink={unlinkControlFromRequirement} diff --git a/apps/framework-editor/app/(pages)/frameworks/[frameworkId]/FrameworkTabs.tsx b/apps/framework-editor/app/(pages)/frameworks/[frameworkId]/FrameworkTabs.tsx index 253b83a37c..5a6d27c0da 100644 --- a/apps/framework-editor/app/(pages)/frameworks/[frameworkId]/FrameworkTabs.tsx +++ b/apps/framework-editor/app/(pages)/frameworks/[frameworkId]/FrameworkTabs.tsx @@ -1,5 +1,6 @@ 'use client'; +import { confirmDiscardUnsavedChanges } from '@/app/lib/unsaved-changes'; import { Tabs, TabsList, TabsTrigger } from '@trycompai/ui'; import Link from 'next/link'; import { useParams, useSelectedLayoutSegment } from 'next/navigation'; @@ -29,7 +30,14 @@ export function FrameworkTabs() { className="flex-1" asChild > - {tab.name} + { + if (!confirmDiscardUnsavedChanges()) event.preventDefault(); + }} + > + {tab.name} + ))} diff --git a/apps/framework-editor/app/(pages)/frameworks/[frameworkId]/hooks/useRequirementChangeTracking.test.tsx b/apps/framework-editor/app/(pages)/frameworks/[frameworkId]/hooks/useRequirementChangeTracking.test.tsx new file mode 100644 index 0000000000..9f91bc66ff --- /dev/null +++ b/apps/framework-editor/app/(pages)/frameworks/[frameworkId]/hooks/useRequirementChangeTracking.test.tsx @@ -0,0 +1,166 @@ +import { act, renderHook } from '@testing-library/react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { + useRequirementChangeTracking, + type RequirementGridRow, +} from './useRequirementChangeTracking'; + +const { apiClientMock, refreshMock, toastMock } = vi.hoisted(() => ({ + apiClientMock: vi.fn(), + refreshMock: vi.fn(), + toastMock: { success: vi.fn(), error: vi.fn() }, +})); + +vi.mock('@/app/lib/api-client', () => ({ apiClient: apiClientMock })); +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: RequirementGridRow[] = []; + +function makeRow(overrides: Partial = {}): RequirementGridRow { + return { + id: 'temp-1', + name: 'New Requirement', + identifier: '10.3', + description: '', + requirementFamily: 'Access Control', + controlTemplates: [], + controlTemplatesLength: 0, + createdAt: new Date(), + updatedAt: new Date(), + ...overrides, + }; +} + +describe('useRequirementChangeTracking handleCommit', () => { + beforeEach(() => { + vi.clearAllMocks(); + apiClientMock.mockImplementation(async (path: string) => { + if (path === '/requirement') return { id: 'frk_rq_new' }; + return {}; + }); + }); + + it('creates a new requirement and clears dirty state', async () => { + const { result } = renderHook(() => useRequirementChangeTracking(NO_ROWS, 'frk_1')); + + act(() => { + result.current.addRow(makeRow()); + }); + expect(result.current.isDirty).toBe(true); + + await act(async () => { + await result.current.handleCommit(); + }); + + expect(apiClientMock).toHaveBeenCalledWith( + '/requirement', + expect.objectContaining({ method: 'POST' }), + ); + const createCall = apiClientMock.mock.calls.find((call) => call[0] === '/requirement'); + expect(JSON.parse(createCall![1].body)).toMatchObject({ + frameworkId: 'frk_1', + name: 'New Requirement', + identifier: '10.3', + requirementFamily: 'Access Control', + }); + expect(result.current.data[0].id).toBe('frk_rq_new'); + expect(result.current.isDirty).toBe(false); + expect(toastMock.success).toHaveBeenCalled(); + expect(refreshMock).toHaveBeenCalled(); + }); + + it('persists control links picked on the uncommitted row', async () => { + const { result } = renderHook(() => useRequirementChangeTracking(NO_ROWS, 'frk_1')); + + act(() => { + result.current.addRow( + makeRow({ + controlTemplates: [{ id: 'ct_1', name: 'Control 1' }], + controlTemplatesLength: 1, + }), + ); + }); + await act(async () => { + await result.current.handleCommit(); + }); + + expect(apiClientMock).toHaveBeenCalledWith( + '/control-template/ct_1/requirements/frk_rq_new', + { method: 'POST' }, + ); + expect(toastMock.error).not.toHaveBeenCalled(); + expect(refreshMock).toHaveBeenCalled(); + }); + + it('reports an error instead of silently skipping rows without a name', async () => { + const { result } = renderHook(() => useRequirementChangeTracking(NO_ROWS, 'frk_1')); + + act(() => { + result.current.addRow(makeRow({ name: '' })); + }); + await act(async () => { + await result.current.handleCommit(); + }); + + expect(apiClientMock).not.toHaveBeenCalled(); + expect(toastMock.error).toHaveBeenCalledWith( + 'Some operations failed', + expect.objectContaining({ + description: expect.stringContaining('name is required'), + }), + ); + expect(result.current.isDirty).toBe(true); + expect(refreshMock).not.toHaveBeenCalled(); + }); + + it('keeps failed rows dirty and does not refresh', async () => { + apiClientMock.mockRejectedValue(new Error('boom')); + const { result } = renderHook(() => useRequirementChangeTracking(NO_ROWS, 'frk_1')); + + act(() => { + result.current.addRow(makeRow()); + }); + await act(async () => { + await result.current.handleCommit(); + }); + + expect(toastMock.error).toHaveBeenCalled(); + expect(result.current.isDirty).toBe(true); + expect(refreshMock).not.toHaveBeenCalled(); + }); + + it('surfaces link failures after a successful create', async () => { + apiClientMock.mockImplementation(async (path: string) => { + if (path === '/requirement') return { id: 'frk_rq_new' }; + throw new Error('link failed'); + }); + const { result } = renderHook(() => useRequirementChangeTracking(NO_ROWS, 'frk_1')); + + act(() => { + result.current.addRow( + makeRow({ + controlTemplates: [{ id: 'ct_1', name: 'Control 1' }], + controlTemplatesLength: 1, + }), + ); + }); + await act(async () => { + await result.current.handleCommit(); + }); + + // The requirement exists (id swapped in) but the user is told links failed. + expect(result.current.data[0].id).toBe('frk_rq_new'); + expect(toastMock.error).toHaveBeenCalledWith( + 'Some operations failed', + expect.objectContaining({ + description: expect.stringContaining('failed to link'), + }), + ); + expect(refreshMock).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/framework-editor/app/(pages)/frameworks/[frameworkId]/hooks/useRequirementChangeTracking.ts b/apps/framework-editor/app/(pages)/frameworks/[frameworkId]/hooks/useRequirementChangeTracking.ts index 6bc3aa5cf9..3d699e390d 100644 --- a/apps/framework-editor/app/(pages)/frameworks/[frameworkId]/hooks/useRequirementChangeTracking.ts +++ b/apps/framework-editor/app/(pages)/frameworks/[frameworkId]/hooks/useRequirementChangeTracking.ts @@ -1,4 +1,6 @@ import { apiClient } from '@/app/lib/api-client'; +import { useUnsavedChangesGuard } from '@/app/lib/unsaved-changes'; +import { useRouter } from 'next/navigation'; import { useCallback, useEffect, useMemo, useState } from 'react'; import { toast } from 'sonner'; @@ -20,6 +22,7 @@ export function useRequirementChangeTracking( initialData: RequirementGridRow[], frameworkId: string, ) { + const router = useRouter(); const [data, setData] = useState(() => initialData); const [prevData, setPrevData] = useState(() => initialData); @@ -123,7 +126,13 @@ export function useRequirementChangeTracking( for (const tempId of createdIds) { const row = currentData.find((r) => r.id === tempId); - if (!row?.name) continue; + if (!row) continue; + if (!row.name?.trim()) { + results.errors.push( + `New requirement${row.identifier ? ` "${row.identifier}"` : ''}: name is required`, + ); + continue; + } try { const created = await apiClient<{ id: string }>('/requirement', { method: 'POST', @@ -135,7 +144,25 @@ export function useRequirementChangeTracking( requirementFamily: row.requirementFamily ?? undefined, }), }); - results.successes.push(`Created: ${row.name}`); + // Persist control links the user picked on the uncommitted row. + const failedLinks: string[] = []; + for (const controlTemplate of row.controlTemplates) { + try { + await apiClient( + `/control-template/${controlTemplate.id}/requirements/${created.id}`, + { method: 'POST' }, + ); + } catch { + failedLinks.push(controlTemplate.name); + } + } + if (failedLinks.length > 0) { + results.errors.push( + `Created "${row.name}" but failed to link control(s): ${failedLinks.join(', ')}`, + ); + } else { + results.successes.push(`Created: ${row.name}`); + } okCreated.add(tempId); setData((prev) => prev.map((r) => (r.id === tempId ? { ...r, id: created.id } : r)), @@ -226,8 +253,10 @@ export function useRequirementChangeTracking( toast.success('Changes saved', { description: `${results.successes.length} operation(s) completed`, }); + // Re-sync the grid with server truth (ids, timestamps, links). + router.refresh(); } - }, [data, createdIds, updatedIds, deletedIds, frameworkId]); + }, [data, createdIds, updatedIds, deletedIds, frameworkId, router]); const handleCancel = useCallback(() => { setData(prevData); @@ -241,6 +270,10 @@ export function useRequirementChangeTracking( [createdIds, updatedIds, deletedIds], ); + // Uncommitted rows only live in this grid's state — warn before they are + // discarded by a reload or a tab switch. + useUnsavedChangesGuard('framework-requirements-grid', isDirty); + const changesSummary = useMemo(() => { const total = createdIds.size + updatedIds.size + deletedIds.size; if (total === 0) return ''; diff --git a/apps/framework-editor/app/components/AddExistingItemDialog.tsx b/apps/framework-editor/app/components/AddExistingItemDialog.tsx index 66a9b0bd7c..786f56da4e 100644 --- a/apps/framework-editor/app/components/AddExistingItemDialog.tsx +++ b/apps/framework-editor/app/components/AddExistingItemDialog.tsx @@ -58,6 +58,23 @@ interface AddExistingItemDialogProps { fetchAllItems: () => Promise; } +// apiClient throws Error(); pull the NestJS `message` +// field out so the user sees e.g. "Framework has no requirements to link +// the control to" instead of a generic failure. +function extractApiErrorMessage(error: unknown): string | null { + if (!(error instanceof Error) || !error.message) return null; + try { + const parsed: unknown = JSON.parse(error.message); + if (parsed && typeof parsed === 'object' && 'message' in parsed) { + const message = (parsed as { message?: unknown }).message; + if (typeof message === 'string') return message; + } + } catch { + // Not JSON — fall through to the raw message. + } + return error.message; +} + function extractFrameworkNames(item: ExistingItemRaw): string[] { const names = new Set(); @@ -142,8 +159,11 @@ export function AddExistingItemDialog({ setLinkedIds((prev) => new Set(prev).add(item.id)); toast.success(`${config.label} "${item.name}" linked successfully`); router.refresh(); - } catch { - toast.error(`Failed to link ${config.label.toLowerCase()}`); + } catch (error) { + toast.error( + extractApiErrorMessage(error) ?? + `Failed to link ${config.label.toLowerCase()}`, + ); } finally { setLinkingId(null); } diff --git a/apps/framework-editor/app/components/table/RelationalCell.test.tsx b/apps/framework-editor/app/components/table/RelationalCell.test.tsx new file mode 100644 index 0000000000..7efbf39b59 --- /dev/null +++ b/apps/framework-editor/app/components/table/RelationalCell.test.tsx @@ -0,0 +1,88 @@ +import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { RelationalCell } from './RelationalCell'; + +const { toastMock } = vi.hoisted(() => ({ + toastMock: { success: vi.fn(), error: vi.fn() }, +})); +vi.mock('sonner', () => ({ toast: toastMock })); + +// The ui package ships untranspiled JSX in dist; the cell only needs Button. +vi.mock('@trycompai/ui', () => ({ + Button: ({ + children, + variant: _variant, + size: _size, + ...props + }: { variant?: string; size?: string } & React.ComponentProps<'button'>) => ( + + ), +})); + +function renderCell(props: Partial[0]> = {}) { + const onLink = vi.fn(async () => {}); + const onUnlink = vi.fn(async () => {}); + const onLocalUpdate = vi.fn(); + const getAllItems = vi.fn(async () => [{ id: 'item_1', name: 'Item One' }]); + render( + , + ); + return { onLink, onUnlink, onLocalUpdate, getAllItems }; +} + +describe('RelationalCell on uncommitted rows', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('blocks link selection by default', () => { + renderCell(); + fireEvent.click(screen.getByText('None')); + expect(screen.getByText('Save row first to link items')).toBeDefined(); + expect(screen.queryByRole('button', { name: /add control/i })).toBeNull(); + }); + + it('allows local link selection with allowSelectOnNewRows', async () => { + const { onLink, onLocalUpdate, getAllItems } = renderCell({ + allowSelectOnNewRows: true, + }); + + fireEvent.click(screen.getByText('None')); + expect(screen.queryByText('Save row first to link items')).toBeNull(); + + fireEvent.click(screen.getByRole('button', { name: /add control/i })); + expect(getAllItems).toHaveBeenCalled(); + + fireEvent.click(await screen.findByText('Item One')); + + // New rows collect links locally; nothing is persisted until commit. + expect(onLink).not.toHaveBeenCalled(); + expect(onLocalUpdate).toHaveBeenCalledWith([{ id: 'item_1', name: 'Item One' }]); + expect(toastMock.success).toHaveBeenCalledWith('Control will be linked when you commit'); + }); + + it('keeps immediate linking for already-saved rows', async () => { + const { onLink, onLocalUpdate } = renderCell({ isNewRow: false }); + + fireEvent.click(screen.getByText('None')); + fireEvent.click(screen.getByRole('button', { name: /add control/i })); + fireEvent.click(await screen.findByText('Item One')); + + expect(onLink).toHaveBeenCalledWith('row-1', 'item_1'); + // onLocalUpdate runs after the awaited onLink resolves. + await waitFor(() => + expect(onLocalUpdate).toHaveBeenCalledWith([{ id: 'item_1', name: 'Item One' }]), + ); + }); +}); diff --git a/apps/framework-editor/app/components/table/RelationalCell.tsx b/apps/framework-editor/app/components/table/RelationalCell.tsx index c89e3d4098..b29d5fc33d 100644 --- a/apps/framework-editor/app/components/table/RelationalCell.tsx +++ b/apps/framework-editor/app/components/table/RelationalCell.tsx @@ -15,6 +15,8 @@ interface RelationalCellProps { items: RelationalItem[]; rowId: string; isNewRow: boolean; + /** Let uncommitted rows pick links locally; they are persisted on commit. */ + allowSelectOnNewRows?: boolean; getAllItems: () => Promise; onLink: (parentId: string, itemId: string) => Promise; onUnlink: (parentId: string, itemId: string) => Promise; @@ -27,6 +29,7 @@ export function RelationalCell({ items, rowId, isNewRow, + allowSelectOnNewRows = false, getAllItems, onLink, onUnlink, @@ -183,7 +186,7 @@ export function RelationalCell({ {/* Add section */} - {!isNewRow && ( + {(!isNewRow || allowSelectOnNewRows) && (
{isSearching ? ( <> @@ -235,7 +238,7 @@ export function RelationalCell({
)} - {isNewRow && ( + {isNewRow && !allowSelectOnNewRows && (
Save row first to link items
diff --git a/apps/framework-editor/app/lib/unsaved-changes.test.tsx b/apps/framework-editor/app/lib/unsaved-changes.test.tsx new file mode 100644 index 0000000000..2270832412 --- /dev/null +++ b/apps/framework-editor/app/lib/unsaved-changes.test.tsx @@ -0,0 +1,45 @@ +import { renderHook } from '@testing-library/react'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { + confirmDiscardUnsavedChanges, + hasUnsavedChanges, + useUnsavedChangesGuard, +} from './unsaved-changes'; + +describe('unsaved-changes guard', () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('tracks dirty state while mounted and releases on cleanup', () => { + const { rerender, unmount } = renderHook( + ({ dirty }: { dirty: boolean }) => useUnsavedChangesGuard('grid-a', dirty), + { initialProps: { dirty: false } }, + ); + expect(hasUnsavedChanges()).toBe(false); + + rerender({ dirty: true }); + expect(hasUnsavedChanges()).toBe(true); + + rerender({ dirty: false }); + expect(hasUnsavedChanges()).toBe(false); + + rerender({ dirty: true }); + unmount(); + expect(hasUnsavedChanges()).toBe(false); + }); + + it('confirmDiscardUnsavedChanges passes through when clean and prompts when dirty', () => { + const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(false); + expect(confirmDiscardUnsavedChanges()).toBe(true); + expect(confirmSpy).not.toHaveBeenCalled(); + + const { unmount } = renderHook(() => useUnsavedChangesGuard('grid-b', true)); + expect(confirmDiscardUnsavedChanges()).toBe(false); + expect(confirmSpy).toHaveBeenCalledOnce(); + + confirmSpy.mockReturnValue(true); + expect(confirmDiscardUnsavedChanges()).toBe(true); + unmount(); + }); +}); diff --git a/apps/framework-editor/app/lib/unsaved-changes.ts b/apps/framework-editor/app/lib/unsaved-changes.ts new file mode 100644 index 0000000000..28caf85df2 --- /dev/null +++ b/apps/framework-editor/app/lib/unsaved-changes.ts @@ -0,0 +1,42 @@ +'use client'; + +import { useEffect } from 'react'; + +// Module-level registry of grids that currently hold uncommitted changes. +// Navigation chrome (e.g. FrameworkTabs) consults it before letting the user +// leave, since leaving a page discards all uncommitted grid rows. +const dirtyKeys = new Set(); + +export const UNSAVED_CHANGES_MESSAGE = + 'You have uncommitted changes that will be lost if you leave. Click "Commit Changes" first, or leave anyway?'; + +export function hasUnsavedChanges(): boolean { + return dirtyKeys.size > 0; +} + +export function confirmDiscardUnsavedChanges(): boolean { + if (!hasUnsavedChanges()) return true; + return window.confirm(UNSAVED_CHANGES_MESSAGE); +} + +/** + * Registers a grid's dirty state while mounted and warns on hard + * reload/close via beforeunload. In-app navigation is guarded by callers of + * confirmDiscardUnsavedChanges() (e.g. FrameworkTabs). + */ +export function useUnsavedChangesGuard(key: string, isDirty: boolean) { + useEffect(() => { + if (!isDirty) return; + dirtyKeys.add(key); + const handleBeforeUnload = (event: BeforeUnloadEvent) => { + event.preventDefault(); + // Chrome still requires returnValue to be set to show the prompt. + event.returnValue = ''; + }; + window.addEventListener('beforeunload', handleBeforeUnload); + return () => { + dirtyKeys.delete(key); + window.removeEventListener('beforeunload', handleBeforeUnload); + }; + }, [key, isDirty]); +} diff --git a/apps/framework-editor/package.json b/apps/framework-editor/package.json index 72457f1fd6..bac9c73723 100644 --- a/apps/framework-editor/package.json +++ b/apps/framework-editor/package.json @@ -30,14 +30,18 @@ "zod": "^3.24.2" }, "devDependencies": { + "@testing-library/react": "^16.3.0", "@types/node": "^22.15.0", "@types/react": "^19.1.6", "@types/react-dom": "^19.1.1", + "@vitejs/plugin-react": "^4.6.0", "autoprefixer": "^10.4.21", + "jsdom": "^26.1.0", "postcss": "^8.5.4", "prisma": "7.6.0", "tailwindcss": "^4.1.8", - "typescript": "^5.8.3" + "typescript": "^5.8.3", + "vitest": "^3.2.4" }, "private": true, "scripts": { @@ -47,6 +51,7 @@ "lint": "echo 'no lint configured'", "prebuild": "bun run db:generate", "start": "next start", + "test": "vitest run", "typecheck": "tsc --noEmit" }, "type": "module" diff --git a/apps/framework-editor/vitest.config.ts b/apps/framework-editor/vitest.config.ts new file mode 100644 index 0000000000..6452d6ae9e --- /dev/null +++ b/apps/framework-editor/vitest.config.ts @@ -0,0 +1,18 @@ +import react from '@vitejs/plugin-react'; +import { resolve } from 'path'; +import { defineConfig } from 'vitest/config'; + +export default defineConfig({ + plugins: [react()], + test: { + environment: 'jsdom', + setupFiles: ['./vitest.setup.ts'], + include: ['app/**/*.{test,spec}.{ts,tsx}'], + exclude: ['node_modules', '.next'], + }, + resolve: { + alias: { + '@': resolve(__dirname, './'), + }, + }, +}); diff --git a/apps/framework-editor/vitest.setup.ts b/apps/framework-editor/vitest.setup.ts new file mode 100644 index 0000000000..878063b678 --- /dev/null +++ b/apps/framework-editor/vitest.setup.ts @@ -0,0 +1,7 @@ +import { cleanup } from '@testing-library/react'; +import { afterEach } from 'vitest'; + +// Without test globals RTL cannot register its own afterEach cleanup. +afterEach(() => { + cleanup(); +}); diff --git a/bun.lock b/bun.lock index f2f0be905f..85096ca3ea 100644 --- a/bun.lock +++ b/bun.lock @@ -430,14 +430,18 @@ "zod": "^3.24.2", }, "devDependencies": { + "@testing-library/react": "^16.3.0", "@types/node": "^22.15.0", "@types/react": "^19.1.6", "@types/react-dom": "^19.1.1", + "@vitejs/plugin-react": "^4.6.0", "autoprefixer": "^10.4.21", + "jsdom": "^26.1.0", "postcss": "^8.5.4", "prisma": "7.6.0", "tailwindcss": "^4.1.8", "typescript": "^5.8.3", + "vitest": "^3.2.4", }, }, "apps/portal": { From 716205c3e63f7d6595ee0b82d5976ff1059e6726 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Wed, 10 Jun 2026 11:00:07 -0400 Subject: [PATCH 2/2] fix(framework-editor): drop @vitejs/plugin-react from vitest config Next's build-time typecheck on Vercel resolves two vite copies (root vs vitest's bundled one), making the plugin's Plugin type incompatible with vitest's defineConfig. esbuild already handles TSX for tests, so the plugin was only adding the type conflict. Co-Authored-By: Claude Fable 5 --- apps/framework-editor/package.json | 1 - apps/framework-editor/vitest.config.ts | 5 +++-- bun.lock | 1 - 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/apps/framework-editor/package.json b/apps/framework-editor/package.json index bac9c73723..9d1736d060 100644 --- a/apps/framework-editor/package.json +++ b/apps/framework-editor/package.json @@ -34,7 +34,6 @@ "@types/node": "^22.15.0", "@types/react": "^19.1.6", "@types/react-dom": "^19.1.1", - "@vitejs/plugin-react": "^4.6.0", "autoprefixer": "^10.4.21", "jsdom": "^26.1.0", "postcss": "^8.5.4", diff --git a/apps/framework-editor/vitest.config.ts b/apps/framework-editor/vitest.config.ts index 6452d6ae9e..dca56bdfde 100644 --- a/apps/framework-editor/vitest.config.ts +++ b/apps/framework-editor/vitest.config.ts @@ -1,9 +1,10 @@ -import react from '@vitejs/plugin-react'; import { resolve } from 'path'; import { defineConfig } from 'vitest/config'; +// No @vitejs/plugin-react on purpose: esbuild already transforms TSX with the +// automatic JSX runtime, and the plugin's vite Plugin type clashes with +// vitest's bundled vite during Next's build-time typecheck on Vercel. export default defineConfig({ - plugins: [react()], test: { environment: 'jsdom', setupFiles: ['./vitest.setup.ts'], diff --git a/bun.lock b/bun.lock index 85096ca3ea..3d06bfdff4 100644 --- a/bun.lock +++ b/bun.lock @@ -434,7 +434,6 @@ "@types/node": "^22.15.0", "@types/react": "^19.1.6", "@types/react-dom": "^19.1.1", - "@vitejs/plugin-react": "^4.6.0", "autoprefixer": "^10.4.21", "jsdom": "^26.1.0", "postcss": "^8.5.4",