Conversation
…#474) - TaskDetailModal: add FAILED-state guidance panel with destructive styling - TaskDetailModal: add 'View PROOF9 Gates' + 'Reset to Ready' footer buttons for FAILED - proof/page.tsx: add ?gate= filter support using useSearchParams - Filter requirements by obligation.gate match (pytest/ruff/build/custom) - Show 'Filtered by gate: X' badge with clear link when filter active - Empty state for no-match case - Wrap ProofPageContent in Suspense for useSearchParams compatibility
WalkthroughThe PR adds gate-based filtering to the Proof page (reads Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as TaskDetailModal
participant Router
participant ProofPage
participant Data as RequirementsStore
User->>UI: Open failed task modal
UI->>UI: Render FAILED guidance + "View PROOF9 Gates"
User->>UI: Click "View PROOF9 Gates"
UI->>Router: navigate to /proof?gate=<derivedGate>
Router->>ProofPage: load page with gate query
ProofPage->>Data: request requirements list
Data-->>ProofPage: return requirements
ProofPage->>ProofPage: filter requirements by gate (visibleReqs)
ProofPage-->>User: render filtered requirements (or empty-state)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
PR review posted below as structured feedback on the two changed files. |
|
proof/page.tsx - IIFE in JSX: The IIFE pattern used to scope visibleReqs inside JSX is unusual and harder to read. Extract it above the return statement instead. The conditional rendering block becomes a clean JSX expression and the tricky double-closing pattern disappears. |
|
proof/page.tsx - Suspense missing fallback: The Suspense wrapper has no fallback prop so React renders nothing during useSearchParams hydration. This can cause a layout flash. Add a minimal loading placeholder as the fallback. |
|
proof/page.tsx - Stats bar shows unfiltered totals: When gateFilter is active the open/satisfied/waived/total counts still reflect all requirements not just the visible filtered set. A user who filters by pytest and sees 10 total cannot tell if that is 10 matching or 10 overall. Suggest adding a showing-N-of-M label next to the filter badge. |
|
proof/page.tsx - Clear filter button accessibility: The unicode X character link used to clear the gate filter has no accessible label. Add aria-label='Clear gate filter' for screen reader users. |
|
TaskDetailModal.tsx - View PROOF9 Gates does not pass gate context: The proof page now supports ?gate= filtering but the button in the FAILED task modal navigates to /proof without it. If the gate that caused failure is derivable from task.requirement_ids this would be a natural integration point. Worth a follow-up issue once the task object exposes that info. |
|
Overall: Good focused PR that fills a real gap. The IIFE refactor and Suspense fallback are the two items worth addressing before merge. The stats/filter consistency and gate-context navigation are solid follow-up candidates to keep this PR scoped tightly. Using Suspense around useSearchParams is exactly right for Next.js App Router. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/app/proof/page.tsx`:
- Around line 204-208: The clear-filter Link next to the gateFilter label is
icon-only and lacks an accessible name; update the Link (the JSX element
rendering the "✕") to include an accessible label by adding an aria-label (e.g.,
aria-label="Clear filter") or by including visible text for screen readers
(e.g., a visually-hidden "Clear filter" span) so assistive tech can announce its
purpose; ensure this change is applied to the Link element that renders the
close icon adjacent to gateFilter.
In `@web-ui/src/components/tasks/TaskDetailModal.tsx`:
- Around line 226-241: The FAILED-state panel in TaskDetailModal currently links
generically to /proof and loses root-cause context; update the component to
detect and carry the failing gate/requirement (use task.requirement_ids and any
resolved gate identifier present on the task object) and build a contextual
link: if a single requirement id exists navigate to /proof/<reqId>, if a gate
identifier is available navigate to /proof?gate=<gateId>, otherwise fall back to
/proof; apply the same change to the similar block referenced around the other
FAILED-state rendering (the other FAILED panel at the 281-305 region) so both
CTA buttons/routes preserve and pass the resolved gate/requirement context.
🪄 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: 98f9d415-2874-4579-a9fa-2549cbf70d04
📒 Files selected for processing (2)
web-ui/src/app/proof/page.tsxweb-ui/src/components/tasks/TaskDetailModal.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (2)
web-ui/src/app/proof/page.tsx (2)
399-405: Add afallbackto<Suspense>for smoother hydration.When
useSearchParamssuspends during SSR/hydration, React renders the fallback. Without one, users may see a brief flash of empty content. Consider adding a minimal loading state.♻️ Suggested fix with loading fallback
export default function ProofPage() { return ( - <Suspense> + <Suspense fallback={ + <main className="min-h-screen bg-background"> + <div className="mx-auto max-w-7xl px-4 py-8"> + <div className="space-y-2"> + {[...Array(4)].map((_, i) => ( + <div key={i} className="h-12 animate-pulse rounded-md bg-muted" /> + ))} + </div> + </div> + </main> + }> <ProofPageContent /> </Suspense> ); }This reuses the same skeleton pattern already present in
ProofPageContentfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/app/proof/page.tsx` around lines 399 - 405, The Suspense wrapper in ProofPage lacks a fallback, causing a blank flash when useSearchParams suspends; update the ProofPage component to pass a minimal loading fallback prop to Suspense (e.g., a small skeleton/loading UI that mirrors the skeleton used inside ProofPageContent) so hydration shows that state until ProofPageContent resolves; locate ProofPage and Suspense around ProofPageContent and reuse the skeleton pattern from ProofPageContent for the fallback.
190-196: Consider replacing the IIFE withuseMemofor clearer intent.The inline IIFE pattern works but is unconventional in React. Using
useMemomakes the derived computation explicit and avoids re-filtering on every render when dependencies haven't changed.♻️ Suggested refactor using useMemo
Add
useMemoto the import on line 3:-import { useState, useEffect, Suspense } from 'react'; +import { useState, useEffect, Suspense, useMemo } from 'react';Then replace the IIFE with a memoized value before the return statement (after line 136):
+ const visibleReqs = useMemo(() => { + if (!data || data.total === 0) return []; + return gateFilter + ? data.requirements.filter((r) => + r.obligations.some((o) => o.gate.toLowerCase() === gateFilter) + ) + : data.requirements; + }, [data, gateFilter]);Then simplify the render block (lines 190-328) to use
visibleReqsdirectly without the IIFE wrapper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/app/proof/page.tsx` around lines 190 - 196, Replace the inline IIFE that computes visibleReqs with a memoized value using React's useMemo: import useMemo, create a const visibleReqs = useMemo(() => gateFilter ? data.requirements.filter(r => r.obligations.some(o => o.gate.toLowerCase() === gateFilter)) : data.requirements, [data, gateFilter]) placed before the component return, and then use visibleReqs directly in the render instead of invoking the IIFE; reference the existing symbols gateFilter, data.requirements, visibleReqs and ensure the dependency array includes data and gateFilter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web-ui/src/app/proof/page.tsx`:
- Around line 399-405: The Suspense wrapper in ProofPage lacks a fallback,
causing a blank flash when useSearchParams suspends; update the ProofPage
component to pass a minimal loading fallback prop to Suspense (e.g., a small
skeleton/loading UI that mirrors the skeleton used inside ProofPageContent) so
hydration shows that state until ProofPageContent resolves; locate ProofPage and
Suspense around ProofPageContent and reuse the skeleton pattern from
ProofPageContent for the fallback.
- Around line 190-196: Replace the inline IIFE that computes visibleReqs with a
memoized value using React's useMemo: import useMemo, create a const visibleReqs
= useMemo(() => gateFilter ? data.requirements.filter(r => r.obligations.some(o
=> o.gate.toLowerCase() === gateFilter)) : data.requirements, [data,
gateFilter]) placed before the component return, and then use visibleReqs
directly in the render instead of invoking the IIFE; reference the existing
symbols gateFilter, data.requirements, visibleReqs and ensure the dependency
array includes data and gateFilter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef295985-00e8-42ca-aae2-0b12ebc4f4f5
📒 Files selected for processing (2)
web-ui/src/app/proof/page.tsxweb-ui/src/components/tasks/TaskDetailModal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- web-ui/src/components/tasks/TaskDetailModal.tsx
|
Code Review — PR #496: Link failed tasks to PROOF9 gate failure for root cause The overall direction is correct and the UX flow makes sense. The implementation is clean and the Suspense wrapper is the right fix for the Next.js useSearchParams constraint. A few issues worth addressing before merge: BUG 1: Gate link picks the wrong obligation in some cases File: web-ui/src/components/tasks/TaskDetailModal.tsx, gate-link derivation block The loop that derives proofLink takes the first obligation of the first requirement with any obligation. If the task has multiple requirements or a requirement has multiple obligations, the picked gate may not be the one that caused the failure. Consider checking obligation.status (or any similar failure signal) to pick the most relevant gate, or iterate all obligations across all linked requirements to find the best candidate rather than stopping at the first match. BUG 2: Summary counts are not adjusted when a gate filter is active File: web-ui/src/app/proof/page.tsx, summary row The summary row (N open / N satisfied / N waived / N total) is sourced from data.by_status and data.total, which reflect the full unfiltered dataset. When gateFilter is set, the table may show 3 rows while the summary still says 12 total. Either hide the counts row when filtering, or recompute the counts from visibleReqs so they match what is displayed in the table. ISSUE 3: Missing Suspense fallback File: web-ui/src/app/proof/page.tsx, Suspense wrapper Suspense without a fallback prop renders null during the hydration gap, giving a blank screen. Even fallback={null} (explicit) is better because it documents the intent. A loading skeleton matching the existing isLoading skeleton would be ideal. ISSUE 4: Code quality: IIFE pattern inside JSX Both files use immediately-invoked function expressions to introduce local variables inside JSX. This pattern is valid but unconventional in React and makes the render path harder to follow. The standard approach is to compute visibleReqs and proofLink above the return statement, before the JSX block. This also eliminates the nesting that currently hides the empty-state check inside the IIFE. MINOR 5: gateFilter value is rendered verbatim without sanitization The value from searchParams.get('gate') is displayed in two places. React's escaping prevents XSS, but a malformed or excessively long query string value will be shown to the user. A simple slice(0, 64) guard or an allowlist check against known gate names (pytest, ruff, build, custom) would make this more defensive. MINOR 6: Pre-existing but newly exercised: extra SWR fetch on modal open useRequirementsLookup in TaskDetailModal issues a fetch for all PROOF9 requirements whenever the modal opens on a page that has not already fetched them. This is pre-existing behavior, but the new FAILED-state code path makes it more likely to be hit. Worth noting for a future optimization (e.g., conditionally fetching only when task.status === 'FAILED'). Summary: Bugs 1 and 2 are the most important to fix before merge. Issue 3 (missing Suspense fallback) and Issue 4 (IIFE style) are meaningful improvements. Items 5 and 6 are lower priority. |
1 participant
Summary
?gate=filter:useSearchParamsreads?gate=param and filters the requirements table to show only requirements with matching obligation gate names (pytest, ruff, build, custom)<Suspense>for Next.jsuseSearchParamscompatibilityCloses #474
Test plan
/proof?gate=pytestfilters table to pytest-obligation requirements/proof?gate=rufffilters to ruff-obligation requirementsSummary by CodeRabbit