fix(web-ui): address CodeRabbit review feedback on Glitch Capture (#568)

[frankbria]

Fixes three bugs identified by CodeRabbit on #576 after merge.

Bugs fixed

Medium — Gates silently discarded
selectedGates was validated (≥1 required) but never included in the API payload. CaptureRequirementRequest has no gates field — obligations are auto-derived by LLM from the description. Fix: append \n\nRequired gates: unit, sec, ... to the description before POSTing, so the LLM uses the selections when deriving obligations.

Low — Expiry field was dead code
expires state was rendered and collected but CaptureRequirementRequest has no expires field (that belongs to WaiveRequirementRequest). Fix: removed the field from the form and state entirely. Expiry is set at waiver time.

Low — Axios error detail never surfaced
(err as { detail?: string })?.detail always resolves to undefined on Axios errors — the detail is at err.response.data.detail. Fix: correct extraction with fallback to generic message.

Also

Refactored from centered Dialog (max-w-lg) to a right-anchored slide-over using Radix DialogPrimitive directly. The issue spec required "full-screen modal or slide-over" and the form height warrants it.

Test plan

• New test: surfaces backend error detail from axios response on failure — verifies Workspace not found message is shown
• New test: shows fallback error message when axios error has no detail
• New test: does not render an expiry date field
• Updated: submission test now asserts gates appear in description payload
• 718/718 tests pass, clean build

Summary by CodeRabbit

• Refactor

  • Redesigned capture modal as a right-anchored slide-over panel
  • Removed expiry date field from the capture flow
  • Enhanced gate selection integration in form submission

• New Features

  • Improved error handling with backend-provided error details and fallback messaging

• Tests

  • Updated test suite to reflect modal UI redesign and submission behavior changes

[coderabbitai]

Warning

Rate limit exceeded

@frankbria has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 1 minutes and 1 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 1 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d07e3729-d7e7-4748-866f-91ee56bfcdf7

📥 Commits

Reviewing files that changed from the base of the PR and between ae1ed30 and 10339c5.

📒 Files selected for processing (2)

• web-ui/src/__tests__/components/proof/CaptureGlitchModal.test.tsx
• web-ui/src/components/proof/CaptureGlitchModal.tsx

Walkthrough

The CaptureGlitchModal component has been refactored from a centered dialog modal to a right-anchored slide-over using Radix UI primitives. The expiry date field has been removed entirely, selected gate checkboxes are now appended to the form description for backend processing, and error handling now extracts specific error details from Axios responses. All corresponding test expectations have been updated accordingly.

Changes

Cohort / File(s)	Summary
Component Implementation web-ui/src/components/proof/CaptureGlitchModal.tsx	Replaced custom Dialog components with Radix UI dialog primitives for slide-over sheet layout; removed expires state and expiry date input field; updated form submission to append selected gates to description string; enhanced error handling to extract and display response.data.detail from backend, with fallback to generic message.
Test Suite web-ui/src/__tests__/components/proof/CaptureGlitchModal.test.tsx	Updated rendering assertions for slide-over presentation; added test confirming expiry date field is absent; revised submission test to verify gates are appended to description; split generic API failure test into two specific assertions for detail extraction and fallback scenarios; enhanced coverage with close button interaction test.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• feat(web-ui): Glitch Capture entry point and form for PROOF9 (#568) #576: Main PR that directly modifies the same CaptureGlitchModal component and test file—converting the dialog UI from centered modal to right-anchored slide-over, removing expiry functionality, integrating gate selection into form description, and improving Axios error handling.

Poem

🐰 A slide-over appears, sleek and true,
No more expiry dates to wrestle through,
Gates now whisper in the description's ear,
Errors speak plainly when details are clear! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(web-ui): address CodeRabbit review feedback on Glitch Capture (#568)' clearly identifies this as a fix addressing specific review feedback on the Glitch Capture feature, which aligns with the PR's objective of fixing three bugs identified after a previous merge.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/glitch-capture-review-568

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Review — PR #577 (Glitch Capture follow-up fixes)

The three bug fixes are correct and well-reasoned. The slide-over refactor using DialogPrimitive directly is appropriate — there's no Sheet component in the UI kit, so reaching into Radix primitives is the right call. Test coverage is solid.

Two small issues worth addressing before merge:

---

1. Unused Input import

Input was only used for the expiry date field (now removed), but the import was left behind at line 7 of CaptureGlitchModal.tsx:

import { Input } from '@/components/ui/input';

Should be deleted. If noUnusedLocals is enforced in a stricter build, this will surface as an error.

---

2. Gate order in the description payload is click-order, not canonical GATE_LIST order

const gateHint = `\n\nRequired gates: ${Array.from(selectedGates).join(', ')}`;

Set preserves insertion order, so two users selecting unit + sec in different click sequences produce different prompts (unit, sec vs sec, unit). For deterministic LLM prompting, gates should be emitted in GATE_LIST order:

const orderedGates = GATE_LIST.filter((g) => selectedGates.has(g));
const gateHint = `\n\nRequired gates: ${orderedGates.join(', ')}`;

The existing test only covers unit → sec click order (matching GATE_LIST order), so it wouldn't catch a regression here. The fix also makes the test assertion more robust.

---

Minor: fallback error test lost the onSuccess not-called assertion

The old 'shows inline error and keeps modal open' test checked expect(DEFAULT_PROPS.onSuccess).not.toHaveBeenCalled(). The new 'shows fallback error message when axios error has no detail' test dropped it. The surfaces backend error detail test still verifies the onSuccess guard for the structured-error path, but the fallback path is now uncovered. Low-risk given the implementation shares the same catch block, but easy to add.

---

Everything else looks good

• Axios error extraction (err.response.data.detail) is correct, and the typeof detail === 'string' guard is the right defensive check.
• The expires removal is correct — that field belongs to WaiveRequirementRequest, not capture.
• Slide-over layout (scrollable body, sticky header/footer, sm:max-w-lg) is well-structured.
• × close button and overlay click both delegate to onClose correctly via onOpenChange.
• The aria-describedby="capture-description-text" on DialogPrimitive.Content satisfies Radix's accessible description requirement properly.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

web-ui/src/__tests__/components/proof/CaptureGlitchModal.test.tsx (1)

129-134: Make gate payload assertion order-agnostic to reduce brittleness.

The exact unit, sec string can fail on benign ordering changes while behavior is still correct.

Proposed test tweak

-            description: 'Something broke in production\n\nRequired gates: unit, sec',
+            description: expect.stringMatching(
+              /^Something broke in production\n\nRequired gates: (unit, sec|sec, unit)$/
+            ),

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web-ui/src/__tests__/components/proof/CaptureGlitchModal.test.tsx` around
lines 129 - 134, The assertion for mockCapture using WORKSPACE and
expect.objectContaining is brittle because it asserts the gates string order;
update the test (around mockCapture, WORKSPACE, expect.objectContaining, and the
description/title/severity checks) to be order-agnostic by either extracting the
description from the actual call and asserting it contains both gate names
(e.g., contains 'unit' and 'sec') regardless of order, or by asserting against a
parsed/explicit requiredGates array instead of the exact "unit, sec" substring;
keep the same objectContaining checks for title and severity but replace the
exact description string check with a check that validates presence of both gate
tokens in any order.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@web-ui/src/components/proof/CaptureGlitchModal.tsx`:
- Around line 147-155: Replace the inline SVG close glyph in
CaptureGlitchModal.tsx with the repository-standard Hugeicons React icon: remove
the <svg> inside the DialogPrimitive.Close button and render the appropriate
Hugeicons component (e.g., XMark icon) instead; add the corresponding import
from '@hugeicons/react' at the top of the file and ensure the rendered icon
inherits the same className/aria-hidden attributes used on the original <svg> so
styling and accessibility on the DialogPrimitive.Close button remain unchanged.

---

Nitpick comments:
In `@web-ui/src/__tests__/components/proof/CaptureGlitchModal.test.tsx`:
- Around line 129-134: The assertion for mockCapture using WORKSPACE and
expect.objectContaining is brittle because it asserts the gates string order;
update the test (around mockCapture, WORKSPACE, expect.objectContaining, and the
description/title/severity checks) to be order-agnostic by either extracting the
description from the actual call and asserting it contains both gate names
(e.g., contains 'unit' and 'sec') regardless of order, or by asserting against a
parsed/explicit requiredGates array instead of the exact "unit, sec" substring;
keep the same objectContaining checks for title and severity but replace the
exact description string check with a check that validates presence of both gate
tokens in any order.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 55ae9b84-8917-490a-9adb-0f43009e04af

📥 Commits

Reviewing files that changed from the base of the PR and between 19b0cd1 and ae1ed30.

📒 Files selected for processing (2)

• web-ui/src/__tests__/components/proof/CaptureGlitchModal.test.tsx
• web-ui/src/components/proof/CaptureGlitchModal.tsx

[claude]

Follow-up — PR #577

Good to see the CodeRabbit items addressed in the second commit: the close button correctly uses Cancel01Icon from @hugeicons/react, and the gate submission test now uses the order-agnostic stringMatching regex.

From my previous review, three items remain open:

---

1. Unused Input import (still present)

import { Input } from '@/components/ui/input'; is still in CaptureGlitchModal.tsx (unchanged context line in the diff). The expiry field that used it was removed, but the import wasn't cleaned up. Dead import should be deleted.

---

2. Gate order is still click-order, not GATE_LIST order

The test was made order-agnostic (good), but the underlying implementation still emits gates in Set insertion order:

const gateHint = `\n\nRequired gates: ${Array.from(selectedGates).join(', ')}`;

Two users selecting the same gates in different sequences produce different prompts. The fix:

const orderedGates = GATE_LIST.filter((g) => selectedGates.has(g));
const gateHint = `\n\nRequired gates: ${orderedGates.join(', ')}`;

With this, the test could use an exact string rather than a regex alternation, since order would be deterministic.

---

3. Fallback error test still missing onSuccess guard

shows fallback error message when axios error has no detail doesn't assert expect(DEFAULT_PROPS.onSuccess).not.toHaveBeenCalled(). Low-risk since the implementation shares a catch block, but a one-liner worth adding for completeness.

---

Items 1 and 2 are the priority. Item 3 is optional.