From b9f659e929ea8a8514990c75d219618382293caf Mon Sep 17 00:00:00 2001 From: Brad Groux <3053586+BradGroux@users.noreply.github.com> Date: Fri, 4 Sep 2026 12:43:17 -0500 Subject: [PATCH] fix: retain template dialog during application --- docs/design/TASK-OVERLAY-ACCEPTANCE.md | 6 + e2e/task-template-pending.spec.ts | 195 ++++++++++++++ ...il-agent-template-metrics-mantine.test.tsx | 98 ++++++- .../components/task/ApplyTemplateDialog.tsx | 242 +++++++++++------- 4 files changed, 445 insertions(+), 96 deletions(-) create mode 100644 e2e/task-template-pending.spec.ts diff --git a/docs/design/TASK-OVERLAY-ACCEPTANCE.md b/docs/design/TASK-OVERLAY-ACCEPTANCE.md index 445c271f..5feec3bf 100644 --- a/docs/design/TASK-OVERLAY-ACCEPTANCE.md +++ b/docs/design/TASK-OVERLAY-ACCEPTANCE.md @@ -36,6 +36,12 @@ Reduced-motion preferences make CSS transitions immediate, with no transition de - `task-detail.spec.ts` expanded-workspace case passed. `task-popout-stack.spec.ts` adds template and nested-utility browser checks. Fixtures do not launch agents, start preview servers, resolve conflicts, or create managed worktrees; managed ownership exists only in intercepted browser reads. - Independent standards and specification source reviews identified scrolling utility controls and non-quiet Cancel actions. Both were corrected and cleared on recheck. Remaining evidence gaps are explicit above. +### Apply Template submission + +Apply Template owns submission and dismissal until both the task update and non-fatal activity logging settle. Template selection, variables, overwrite strategy and close controls stay disabled during that operation. Update failures preserve the draft and focus a visible inline error; logging failure after a successful update still completes the application. Variable input values remain stable across deferred rendering. + +The verification contract includes immediate duplicate prevention, retained inputs, deliberate retry and delayed logging failure. `task-template-pending.spec.ts` checks pending and failed states in both themes and motion settings at 1700×900/16px, 1180×760/20px and 900×480/20px, with fixed/reachable footers, exact intercepted update payloads, disabled dismissal/inputs and opener restoration. Fixture-backed browser checks do not establish native utility-operation or final documentation-media acceptance. Delivery evidence is tracked in #1503. + ### Supporting task dialogs `task-support-popouts.spec.ts` covers task, comment, attachment, observation, and deliverable deletion plus manual time entry. Each family is exercised in light/dark, normal/reduced motion, and 1700×900 at 16px, 1180×760 at 20px, and 900×480 at 20px. Checks cover bounded bodies, fixed/reachable footers, no horizontal overflow, keyboard opening, trapped focus, exact opener restoration, and retained task title. Delayed synthetic failed requests exercise Escape, header-close, and backdrop dismissal guards, one submission, inline error recovery, and preserved manual-time drafts. This proves retry availability, not a successful retry. No supporting-record mutation reaches the backend. diff --git a/e2e/task-template-pending.spec.ts b/e2e/task-template-pending.spec.ts new file mode 100644 index 00000000..3e2f648d --- /dev/null +++ b/e2e/task-template-pending.spec.ts @@ -0,0 +1,195 @@ +import { expect, test } from '@playwright/test'; +import { bypassAuth, cleanupRoutes, deleteTask, seedTestTask } from './helpers/auth'; + +for (const theme of ['light', 'dark']) { + for (const reducedMotion of ['no-preference', 'reduce'] as const) { + test(`template application retains pending state in ${theme}, motion ${reducedMotion}`, async ({ + page, + }) => { + await bypassAuth(page); + await page.emulateMedia({ reducedMotion }); + await page.addInitScript( + (theme) => localStorage.setItem('veritas-kanban-theme', theme), + theme + ); + const title = `Template pending fixture ${theme} ${reducedMotion}`; + const task = await seedTestTask(page, { title, type: 'code', description: '' }); + const writes: Array<{ method: string; path: string; body: unknown }> = []; + let release = () => {}; + const gate = new Promise((resolve) => { + release = resolve; + }); + await page.route('**/api/templates', (route) => + route.fulfill({ + json: [ + { + id: 'template-pending-fixture', + name: 'Pending fixture', + category: 'bug', + taskDefaults: { descriptionTemplate: 'Investigate {{custom:ticket}}' }, + subtaskTemplates: [], + }, + ], + }) + ); + await page.route( + new RegExp(`/api/tasks/${task.id}(?:/apply-template)?(?:\\?|$)`), + async (route) => { + const request = route.request(); + if (request.method() === 'GET') return route.fallback(); + writes.push({ + method: request.method(), + path: new URL(request.url()).pathname, + body: request.postDataJSON(), + }); + await gate; + return route.fulfill({ + status: 503, + json: { error: 'Template fixture update failed. Your draft is retained for retry.' }, + }); + } + ); + try { + await page.setViewportSize({ width: 1700, height: 900 }); + await page.goto('/'); + await page.getByRole('article', { name: `Task: ${title}` }).press('Enter'); + const workspace = page.getByTestId('task-detail-panel'); + await workspace.getByRole('button', { name: 'Plan', exact: true }).click(); + const opener = workspace.getByRole('button', { name: 'Template', exact: true }); + await opener.press('Enter'); + const dialog = page.getByRole('dialog', { name: 'Apply Template to Task' }); + await expect(workspace).toHaveAttribute('inert', ''); + await dialog.getByRole('combobox', { name: 'Template', exact: true }).click(); + await page.getByRole('option', { name: /Pending fixture/ }).click(); + const variable = dialog.getByLabel('ticket', { exact: true }); + await variable.fill('BUG-42'); + await dialog.getByRole('switch', { name: 'Force overwrite' }).press('Space'); + await expect(dialog.getByRole('switch', { name: 'Force overwrite' })).toBeChecked(); + await dialog.getByRole('button', { name: 'Help', exact: true }).click(); + const submit = dialog.getByRole('button', { name: 'Apply Template', exact: true }); + await submit.click(); + await expect.poll(() => writes.length).toBe(1); + await submit.evaluate((el) => (el as HTMLButtonElement).click()); + for (const size of [ + { width: 1700, height: 900, fontSize: '16px' }, + { width: 1180, height: 760, fontSize: '20px' }, + { width: 900, height: 480, fontSize: '20px' }, + ]) { + await page.setViewportSize(size); + await page.evaluate((fontSize) => { + document.documentElement.style.fontSize = fontSize; + }, size.fontSize); + await expect(dialog.getByRole('button', { name: 'Close dialog' })).toHaveCSS( + 'width', + size.fontSize === '20px' ? '42.5px' : '34px' + ); + await dialog.evaluate(async (el) => { + await Promise.all( + el + .getAnimations({ subtree: true }) + .filter((a) => a.effect?.getTiming().iterations !== Infinity) + .map((a) => a.finished.catch(() => {})) + ); + }); + const footer = dialog.locator('.vk-overlay-footer'); + const before = await footer.boundingBox(); + await dialog.locator('.vk-overlay-scroll').evaluate((el) => { + el.scrollTop = el.scrollHeight; + }); + expect(await footer.boundingBox()).toEqual(before); + for (const name of ['Apply Template', 'Cancel', 'Close dialog']) { + await expect(dialog.getByRole('button', { name, exact: true })).toBeDisabled(); + await expect(dialog.getByRole('button', { name, exact: true })).toBeInViewport({ + ratio: 1, + }); + } + await expect(variable).toBeDisabled(); + await expect(dialog.getByRole('combobox', { name: 'Template' })).toBeDisabled(); + await expect(dialog.getByRole('switch', { name: 'Force overwrite' })).toBeDisabled(); + await page.keyboard.press('Escape'); + await dialog + .getByRole('button', { name: 'Close dialog' }) + .evaluate((el) => (el as HTMLButtonElement).click()); + await page.mouse.click(3, 3); + await expect(dialog).toBeVisible(); + expect(await dialog.evaluate((el) => el.scrollWidth - el.clientWidth)).toBe(0); + } + await page.screenshot({ path: test.info().outputPath('template-pending.png') }); + release(); + const error = dialog.getByRole('alert', { name: 'Template not applied' }); + await expect(error).toContainText('Template fixture update failed'); + await expect(error).toBeFocused(); + for (const size of [ + { width: 1700, height: 900, fontSize: '16px' }, + { width: 1180, height: 760, fontSize: '20px' }, + { width: 900, height: 480, fontSize: '20px' }, + ]) { + await page.setViewportSize(size); + await page.evaluate((fontSize) => { + document.documentElement.style.fontSize = fontSize; + }, size.fontSize); + await expect(dialog.getByRole('button', { name: 'Close dialog' })).toHaveCSS( + 'width', + size.fontSize === '20px' ? '42.5px' : '34px' + ); + await dialog.evaluate(async (el) => { + await Promise.all( + el + .getAnimations({ subtree: true }) + .filter((a) => a.effect?.getTiming().iterations !== Infinity) + .map((a) => a.finished.catch(() => {})) + ); + }); + await error.scrollIntoViewIfNeeded(); + await expect(error).toBeInViewport({ ratio: 1 }); + await expect( + error.getByText('Template fixture update failed. Your draft is retained for retry.', { + exact: true, + }) + ).toBeInViewport({ ratio: 1 }); + const footer = dialog.locator('.vk-overlay-footer'); + const before = await footer.boundingBox(); + await dialog.locator('.vk-overlay-scroll').evaluate((el) => { + el.scrollTop = el.scrollHeight; + }); + expect(await footer.boundingBox()).toEqual(before); + for (const name of ['Apply Template', 'Cancel', 'Close dialog']) { + await expect(dialog.getByRole('button', { name, exact: true })).toBeEnabled(); + await expect(dialog.getByRole('button', { name, exact: true })).toBeInViewport({ + ratio: 1, + }); + await dialog.getByRole('button', { name, exact: true }).click({ trial: true }); + } + } + await expect(error).toBeInViewport({ ratio: 1 }); + await expect( + error.getByText('Template fixture update failed. Your draft is retained for retry.', { + exact: true, + }) + ).toBeInViewport({ ratio: 1 }); + await expect(variable).toHaveValue('BUG-42'); + await expect(dialog.getByRole('switch', { name: 'Force overwrite' })).toBeChecked(); + await expect(submit).toBeEnabled(); + await submit.click({ trial: true }); + expect(writes).toEqual([ + { + method: 'PATCH', + path: `/api/tasks/${task.id}`, + body: { description: 'Investigate BUG-42' }, + }, + ]); + // The query's global failure notification is separate from the retained inline error. + const notificationClose = page.getByRole('button', { name: 'Close notification' }); + if (await notificationClose.isVisible()) await notificationClose.click(); + await page.screenshot({ path: test.info().outputPath('template-failure.png') }); + await page.keyboard.press('Escape'); + await expect(dialog).toHaveCount(0); + await expect(opener).toBeFocused(); + } finally { + release(); + await cleanupRoutes(page).catch(() => {}); + await deleteTask(page, String(task.id)).catch(() => {}); + } + }); + } +} diff --git a/web/src/__tests__/task-detail-agent-template-metrics-mantine.test.tsx b/web/src/__tests__/task-detail-agent-template-metrics-mantine.test.tsx index de69ee44..1622d709 100644 --- a/web/src/__tests__/task-detail-agent-template-metrics-mantine.test.tsx +++ b/web/src/__tests__/task-detail-agent-template-metrics-mantine.test.tsx @@ -160,8 +160,8 @@ describe('task detail agent, template, and metrics Mantine migration', () => { data: { agent: 'codex', model: 'sonnet', reason: 'Best configured agent' }, }); mocks.useTemplates.mockReturnValue({ data: [template] }); - mocks.updateTaskMutateAsync.mockResolvedValue({}); - mocks.applyTemplateActivity.mockResolvedValue({}); + mocks.updateTaskMutateAsync.mockReset().mockResolvedValue({}); + mocks.applyTemplateActivity.mockReset().mockResolvedValue({}); mocks.useTaskMetrics.mockReturnValue({ data: { totalRuns: 2, @@ -705,6 +705,100 @@ describe('task detail agent, template, and metrics Mantine migration', () => { expect(onApplied).toHaveBeenCalled(); }); + it('retains template inputs and dismissal ownership through update failure and retry', async () => { + let rejectUpdate!: (error: Error) => void; + mocks.updateTaskMutateAsync.mockImplementationOnce( + () => + new Promise((_resolve, reject) => { + rejectUpdate = reject; + }) + ); + const user = userEvent.setup(); + const onOpenChange = vi.fn(); + const onApplied = vi.fn(); + renderWithProviders( + + ); + await user.click(screen.getByRole('combobox', { name: 'Template' })); + await user.click(await screen.findByRole('option', { name: /Bug Fix - Resolve defect/ })); + const variable = screen.getByLabelText('bugId') as HTMLInputElement; + fireEvent.change(variable, { target: { value: 'BUG-42' } }); + const submit = screen.getByRole('button', { name: 'Apply Template' }); + fireEvent.click(submit); + fireEvent.click(submit); + fireEvent.keyDown(document.body, { key: 'Escape' }); + fireEvent.click(screen.getByRole('button', { name: 'Close dialog' })); + fireEvent.click(screen.getByRole('button', { name: 'Cancel' })); + expect(onOpenChange).not.toHaveBeenCalled(); + expect(mocks.updateTaskMutateAsync).toHaveBeenCalledOnce(); + expect(variable.disabled).toBe(true); + expect((screen.getByRole('combobox', { name: 'Template' }) as HTMLInputElement).disabled).toBe( + true + ); + expect( + (screen.getByRole('switch', { name: 'Force overwrite' }) as HTMLInputElement).disabled + ).toBe(true); + await act(async () => rejectUpdate(new Error('Template fixture update failed'))); + expect(screen.getByRole('alert').textContent).toContain('Template fixture update failed'); + expect(document.activeElement).toBe(screen.getByRole('alert')); + expect(variable.value).toBe('BUG-42'); + expect(variable.disabled).toBe(false); + expect(onApplied).not.toHaveBeenCalled(); + expect(mocks.applyTemplateActivity).not.toHaveBeenCalled(); + await act(async () => fireEvent.click(submit)); + expect(mocks.updateTaskMutateAsync).toHaveBeenCalledTimes(2); + expect(mocks.updateTaskMutateAsync.mock.calls[1][0].input.description).toBe( + 'Fix BUG-42 for veritas' + ); + expect(onOpenChange).toHaveBeenCalledExactlyOnceWith(false); + expect(onApplied).toHaveBeenCalledOnce(); + }); + + it('keeps template application locked through non-fatal activity logging', async () => { + let rejectActivity!: (error: Error) => void; + mocks.applyTemplateActivity.mockImplementationOnce( + () => + new Promise((_resolve, reject) => { + rejectActivity = reject; + }) + ); + const log = vi.spyOn(console, 'error').mockImplementation(() => {}); + const user = userEvent.setup(); + const onOpenChange = vi.fn(); + const onApplied = vi.fn(); + try { + renderWithProviders( + + ); + await user.click(screen.getByRole('combobox', { name: 'Template' })); + await user.click(await screen.findByRole('option', { name: /Bug Fix - Resolve defect/ })); + const submit = screen.getByRole('button', { name: 'Apply Template' }); + await act(async () => fireEvent.click(submit)); + expect(mocks.applyTemplateActivity).toHaveBeenCalledOnce(); + fireEvent.click(submit); + fireEvent.keyDown(document.body, { key: 'Escape' }); + fireEvent.click(screen.getByRole('button', { name: 'Cancel' })); + expect(mocks.updateTaskMutateAsync).toHaveBeenCalledOnce(); + expect(onOpenChange).not.toHaveBeenCalled(); + await act(async () => rejectActivity(new Error('Activity fixture failed'))); + expect(onOpenChange).toHaveBeenCalledExactlyOnceWith(false); + expect(onApplied).toHaveBeenCalledOnce(); + expect(screen.queryByRole('alert')).toBeNull(); + } finally { + log.mockRestore(); + } + }); + it('renders task metrics and export controls through direct Mantine primitives', async () => { const user = userEvent.setup(); const task = createMockTask({ diff --git a/web/src/components/task/ApplyTemplateDialog.tsx b/web/src/components/task/ApplyTemplateDialog.tsx index bdbc5a78..8745eeea 100644 --- a/web/src/components/task/ApplyTemplateDialog.tsx +++ b/web/src/components/task/ApplyTemplateDialog.tsx @@ -1,4 +1,4 @@ -import { useState, useMemo } from 'react'; +import { useState, useMemo, useRef, useEffect } from 'react'; import { UiModal as Modal, OverlayFooter } from '@/components/ui/UiOverlay'; import { UiAction } from '@/components/ui/UiVocabulary'; import { @@ -69,6 +69,24 @@ export function ApplyTemplateDialog({ const [requiredCustomVars, setRequiredCustomVars] = useState([]); const [forceOverwrite, setForceOverwrite] = useState(false); const [showHelp, setShowHelp] = useState(false); + const [isApplying, setIsApplying] = useState(false); + const [applyError, setApplyError] = useState(null); + const applyInFlight = useRef(false); + const errorRef = useRef(null); + + useEffect(() => { + if (applyError) { + errorRef.current?.focus({ preventScroll: true }); + errorRef.current?.scrollIntoView({ block: 'center', behavior: 'instant' }); + } + }, [applyError]); + + const handleClose = () => { + if (!applyInFlight.current) { + setApplyError(null); + onOpenChange(false); + } + }; const { data: templates } = useTemplates(); const updateTask = useUpdateTask(); @@ -217,97 +235,116 @@ export function ApplyTemplateDialog({ // Apply the template const handleApply = async () => { - if (!template) return; + if (!template || applyInFlight.current) return; + applyInFlight.current = true; + setIsApplying(true); + setApplyError(null); - // Build variable context - const context: VariableContext = { - project: task.project, - author: 'User', - customVars, - }; - - // Build update input based on merge strategy - const updates: Record = {}; - - // Description - if (template.taskDefaults.descriptionTemplate) { - const interpolated = interpolateVariables(template.taskDefaults.descriptionTemplate, context); - if (forceOverwrite || !task.description) { - updates.description = interpolated; - } - } - - // Type - if (template.taskDefaults.type && (forceOverwrite || !task.type)) { - updates.type = template.taskDefaults.type; - } - - // Priority - if (template.taskDefaults.priority && (forceOverwrite || !task.priority)) { - updates.priority = template.taskDefaults.priority; - } - - // Project - if (template.taskDefaults.project && (forceOverwrite || !task.project)) { - updates.project = template.taskDefaults.project; - } - - // Subtasks - APPEND to existing - if (template.subtaskTemplates && template.subtaskTemplates.length > 0) { - const now = new Date().toISOString(); - const newSubtasks: Subtask[] = template.subtaskTemplates - .sort((a, b) => a.order - b.order) - .map((st) => ({ - id: nanoid(), - title: interpolateVariables(st.title, context), - completed: false, - created: now, - ...(st.acceptanceCriteria?.length && { - acceptanceCriteria: st.acceptanceCriteria.map((criterion) => - interpolateVariables(criterion, context) - ), - criteriaChecked: new Array(st.acceptanceCriteria.length).fill(false), - }), - })); - - // Append to existing subtasks - const existingSubtasks = task.subtasks || []; - updates.subtasks = [...existingSubtasks, ...newSubtasks]; - } - - // Apply the updates - await updateTask.mutateAsync({ - id: task.id, - input: updates, - }); - - // Track which fields were changed for activity logging - const changedFields = Object.keys(updates); - - // Log activity try { - await api.tasks.applyTemplate(task.id, template.id, template.name, changedFields); - } catch (error) { - // Intentionally non-fatal: don't fail the whole operation if activity logging fails - console.error('Failed to log template application:', error); + // Build variable context + const context: VariableContext = { + project: task.project, + author: 'User', + customVars, + }; + + // Build update input based on merge strategy + const updates: Record = {}; + + // Description + if (template.taskDefaults.descriptionTemplate) { + const interpolated = interpolateVariables( + template.taskDefaults.descriptionTemplate, + context + ); + if (forceOverwrite || !task.description) { + updates.description = interpolated; + } + } + + // Type + if (template.taskDefaults.type && (forceOverwrite || !task.type)) { + updates.type = template.taskDefaults.type; + } + + // Priority + if (template.taskDefaults.priority && (forceOverwrite || !task.priority)) { + updates.priority = template.taskDefaults.priority; + } + + // Project + if (template.taskDefaults.project && (forceOverwrite || !task.project)) { + updates.project = template.taskDefaults.project; + } + + // Subtasks - APPEND to existing + if (template.subtaskTemplates && template.subtaskTemplates.length > 0) { + const now = new Date().toISOString(); + const newSubtasks: Subtask[] = [...template.subtaskTemplates] + .sort((a, b) => a.order - b.order) + .map((st) => ({ + id: nanoid(), + title: interpolateVariables(st.title, context), + completed: false, + created: now, + ...(st.acceptanceCriteria?.length && { + acceptanceCriteria: st.acceptanceCriteria.map((criterion) => + interpolateVariables(criterion, context) + ), + criteriaChecked: new Array(st.acceptanceCriteria.length).fill(false), + }), + })); + + // Append to existing subtasks + const existingSubtasks = task.subtasks || []; + updates.subtasks = [...existingSubtasks, ...newSubtasks]; + } + + // Apply the updates + try { + await updateTask.mutateAsync({ + id: task.id, + input: updates, + }); + } catch (error) { + setApplyError(error instanceof Error ? error.message : 'Unable to apply template.'); + return; + } + + // Track which fields were changed for activity logging + const changedFields = Object.keys(updates); + + // Log activity + try { + await api.tasks.applyTemplate(task.id, template.id, template.name, changedFields); + } catch (error) { + // Intentionally non-fatal: don't fail the whole operation if activity logging fails + console.error('Failed to log template application:', error); + } + + // Close dialog and notify parent + onOpenChange(false); + onApplied?.(); + + // Reset state + setSelectedTemplate(null); + setCustomVars({}); + setRequiredCustomVars([]); + setForceOverwrite(false); + } finally { + applyInFlight.current = false; + setIsApplying(false); } - - // Close dialog and notify parent - onOpenChange(false); - onApplied?.(); - - // Reset state - setSelectedTemplate(null); - setCustomVars({}); - setRequiredCustomVars([]); - setForceOverwrite(false); }; return ( onOpenChange(false)} + onClose={handleClose} + closeOnEscape={!isApplying} + closeOnClickOutside={!isApplying} + closeButtonProps={{ disabled: isApplying }} title={ @@ -315,12 +352,13 @@ export function ApplyTemplateDialog({ } > - +