From ae1ed30b250ae1faa1c9a3a2be2258e8c70fc9b9 Mon Sep 17 00:00:00 2001 From: Test User Date: Thu, 9 Apr 2026 22:07:38 -0700 Subject: [PATCH 1/2] fix(web-ui): address CodeRabbit review feedback on Glitch Capture (#568) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three bugs identified post-merge, all confirmed and fixed: 1. Gates silently discarded (Medium) — selectedGates was validated but never sent to the backend. CaptureRequirementRequest has no gates field; obligations are derived by LLM from description. Fix: append selected gates as "Required gates: ..." section to the description so the backend LLM uses them when deriving obligations. 2. Expiry field was dead code (Low) — expires state was collected and rendered but CaptureRequirementRequest has no expires field (that belongs to WaiveRequirementRequest). Fix: removed expiry field from form and state entirely. 3. Axios error detail never surfaced (Low) — err.detail is always undefined on axios errors; detail lives at err.response.data.detail. Fix: correct extraction path with string-type guard and fallback. Also refactored from centered Dialog (max-w-lg) to right-anchored slide-over using Radix DialogPrimitive directly, as the issue spec required "full-screen modal or slide-over". Tests: 718/718 pass (added tests for correct error surfacing and absence of expiry field; updated submission assertion to verify gates are appended to description). --- .../proof/CaptureGlitchModal.test.tsx | 46 ++- .../components/proof/CaptureGlitchModal.tsx | 274 +++++++++--------- 2 files changed, 178 insertions(+), 142 deletions(-) diff --git a/web-ui/src/__tests__/components/proof/CaptureGlitchModal.test.tsx b/web-ui/src/__tests__/components/proof/CaptureGlitchModal.test.tsx index 86f37e33..cfca9f45 100644 --- a/web-ui/src/__tests__/components/proof/CaptureGlitchModal.test.tsx +++ b/web-ui/src/__tests__/components/proof/CaptureGlitchModal.test.tsx @@ -55,7 +55,7 @@ describe('CaptureGlitchModal', () => { }); describe('rendering', () => { - it('renders dialog with title and description when open', () => { + it('renders slide-over panel with title and description when open', () => { setup(); expect(screen.getByRole('heading', { name: 'Capture Glitch' })).toBeInTheDocument(); expect(screen.getByText(/Convert a production failure/i)).toBeInTheDocument(); @@ -67,7 +67,11 @@ describe('CaptureGlitchModal', () => { expect(screen.getByLabelText(/Where was it found/i)).toBeInTheDocument(); expect(screen.getByLabelText(/Scope/i)).toBeInTheDocument(); expect(screen.getByLabelText(/Severity/i)).toBeInTheDocument(); - expect(screen.getByLabelText(/Expiry/i)).toBeInTheDocument(); + }); + + it('does not render an expiry date field (expiry is set at waiver time)', () => { + setup(); + expect(screen.queryByLabelText(/Expiry/i)).not.toBeInTheDocument(); }); it('renders all 9 gate checkboxes', () => { @@ -88,7 +92,6 @@ describe('CaptureGlitchModal', () => { describe('validation', () => { it('shows error when description is empty on submit', async () => { setup(); - // Check at least one gate fireEvent.click(screen.getByRole('checkbox', { name: 'unit' })); fireEvent.click(screen.getByRole('button', { name: /Capture Glitch/i })); await waitFor(() => { @@ -111,7 +114,7 @@ describe('CaptureGlitchModal', () => { }); describe('submission', () => { - it('calls proofApi.capture with correct fields and calls onSuccess', async () => { + it('appends selected gates to description and calls onSuccess', async () => { mockCapture.mockResolvedValue(MOCK_REQ); setup(); @@ -119,6 +122,7 @@ describe('CaptureGlitchModal', () => { target: { value: 'Something broke in production' }, }); fireEvent.click(screen.getByRole('checkbox', { name: 'unit' })); + fireEvent.click(screen.getByRole('checkbox', { name: 'sec' })); fireEvent.click(screen.getByRole('button', { name: /Capture Glitch/i })); await waitFor(() => { @@ -126,7 +130,7 @@ describe('CaptureGlitchModal', () => { WORKSPACE, expect.objectContaining({ title: 'Something broke in production', - description: 'Something broke in production', + description: 'Something broke in production\n\nRequired gates: unit, sec', severity: 'high', source: 'production', created_by: 'human', @@ -138,7 +142,7 @@ describe('CaptureGlitchModal', () => { }); }); - it('truncates description to 80 chars for title', async () => { + it('truncates long description to 80 chars for title', async () => { mockCapture.mockResolvedValue(MOCK_REQ); setup(); @@ -157,7 +161,24 @@ describe('CaptureGlitchModal', () => { }); }); - it('shows inline error and keeps modal open on API failure', async () => { + it('surfaces backend error detail from axios response on failure', async () => { + const axiosError = { response: { data: { detail: 'Workspace not found' } } }; + mockCapture.mockRejectedValue(axiosError); + setup(); + + fireEvent.change(screen.getByLabelText(/Description/i), { + target: { value: 'Something broke' }, + }); + fireEvent.click(screen.getByRole('checkbox', { name: 'demo' })); + fireEvent.click(screen.getByRole('button', { name: /Capture Glitch/i })); + + await waitFor(() => { + expect(screen.getByText('Workspace not found')).toBeInTheDocument(); + }); + expect(DEFAULT_PROPS.onSuccess).not.toHaveBeenCalled(); + }); + + it('shows fallback error message when axios error has no detail', async () => { mockCapture.mockRejectedValue(new Error('Network error')); setup(); @@ -170,12 +191,9 @@ describe('CaptureGlitchModal', () => { await waitFor(() => { expect(screen.getByText(/Failed to capture glitch/i)).toBeInTheDocument(); }); - expect(DEFAULT_PROPS.onSuccess).not.toHaveBeenCalled(); - // Modal still open - expect(screen.getByRole('heading', { name: 'Capture Glitch' })).toBeInTheDocument(); }); - it('disables submit button while submitting', async () => { + it('shows submitting state while in-flight', async () => { let resolve!: (v: ProofRequirement) => void; mockCapture.mockReturnValue(new Promise((r) => { resolve = r; })); setup(); @@ -199,6 +217,12 @@ describe('CaptureGlitchModal', () => { fireEvent.click(screen.getByRole('button', { name: /Cancel/i })); expect(DEFAULT_PROPS.onClose).toHaveBeenCalled(); }); + + it('calls onClose when the × button is clicked', () => { + setup(); + fireEvent.click(screen.getByRole('button', { name: /Close/i })); + expect(DEFAULT_PROPS.onClose).toHaveBeenCalled(); + }); }); describe('state reset on reopen', () => { diff --git a/web-ui/src/components/proof/CaptureGlitchModal.tsx b/web-ui/src/components/proof/CaptureGlitchModal.tsx index 19dc06b8..48c979ab 100644 --- a/web-ui/src/components/proof/CaptureGlitchModal.tsx +++ b/web-ui/src/components/proof/CaptureGlitchModal.tsx @@ -1,14 +1,7 @@ 'use client'; import { useState, useEffect } from 'react'; -import { - Dialog, - DialogContent, - DialogHeader, - DialogTitle, - DialogDescription, - DialogFooter, -} from '@/components/ui/dialog'; +import * as DialogPrimitive from '@radix-ui/react-dialog'; import { Button } from '@/components/ui/button'; import { Input } from '@/components/ui/input'; import { Textarea } from '@/components/ui/textarea'; @@ -54,7 +47,6 @@ export function CaptureGlitchModal({ open, workspacePath, onClose, onSuccess }: const [scopeText, setScopeText] = useState(''); const [selectedGates, setSelectedGates] = useState>(new Set()); const [severity, setSeverity] = useState('high'); - const [expires, setExpires] = useState(''); const [submitting, setSubmitting] = useState(false); const [error, setError] = useState(null); @@ -66,7 +58,6 @@ export function CaptureGlitchModal({ open, workspacePath, onClose, onSuccess }: setScopeText(''); setSelectedGates(new Set()); setSeverity('high'); - setExpires(''); setSubmitting(false); setError(null); } @@ -101,13 +92,19 @@ export function CaptureGlitchModal({ open, workspacePath, onClose, onSuccess }: // Derive title from first line of description (max 80 chars) const title = description.trim().split('\n')[0].slice(0, 80); + // Append selected gates to description so the backend LLM uses them when + // deriving obligations (CaptureRequirementRequest has no gates field; + // obligations are auto-derived from the description). + const gateHint = `\n\nRequired gates: ${Array.from(selectedGates).join(', ')}`; + const fullDescription = description.trim() + gateHint; + // Derive `where` from scope lines, falling back to source const scopeLines = scopeText.split('\n').map((l) => l.trim()).filter(Boolean); const where = scopeLines.length > 0 ? scopeLines.join(', ') : source; const body: CaptureGlitchRequest = { title, - description: description.trim(), + description: fullDescription, where, severity, source, @@ -118,140 +115,155 @@ export function CaptureGlitchModal({ open, workspacePath, onClose, onSuccess }: const result = await proofApi.capture(workspacePath, body); onSuccess(result); } catch (err: unknown) { - const detail = (err as { detail?: string })?.detail; - setError(detail ?? 'Failed to capture glitch. Please try again.'); + // Axios errors carry detail at err.response.data.detail + const axiosErr = err as { response?: { data?: { detail?: string } } }; + const detail = axiosErr?.response?.data?.detail; + setError(typeof detail === 'string' ? detail : 'Failed to capture glitch. Please try again.'); } finally { setSubmitting(false); } } + // Slide-over panel using Radix Dialog primitives directly so we can position + // it as a right-anchored sheet rather than a centered modal. return ( - { if (!o) onClose(); }}> - - - Capture Glitch - - Convert a production failure into a permanent PROOF9 requirement. - - - -
- {/* Description */} -
- -