feat(web-ui): waiver confirmation + audit trail for Proof page - #521
Conversation
…#479) - WaiveDialog converted to 2-step flow: form → amber warning confirmation before submitting in both proof/page.tsx and proof/[req_id]/page.tsx - Confirmation step shows compliance warning and summary of entered data - Back button returns to form with values preserved - Add waived_at?: string | null to ProofWaiver type; detail page shows waiver timestamp when present (reason, approved_by, waived_at, expires) - Waived rows in requirements list styled with opacity-60 for distinct visual treatment from open/satisfied requirements - Add InformationCircleIcon to @hugeicons/react mock - 17 new tests covering all acceptance criteria
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a shared two-step WaiveDialog component, records waiver timestamps across models/API/types, replaces page-local dialogs with the shared component, updates UI to visually mark waived requirements and show waiver audit info, introduces tests for dialog flows/audit rendering, and adds a missing icon mock for tests. Changes
Sequence DiagramsequenceDiagram
autonumber
actor User
participant Page as "Proof Page / Detail"
participant Dialog as "WaiveDialog (UI)"
participant API as "proofApi"
participant SWR as "SWR (mutate)"
User->>Page: Click "Waive"
Page->>Dialog: open(reqId, workspacePath)
Dialog->>User: show form (reason, expires, approvedBy)
User->>Dialog: Enter reason → Continue
Dialog->>Dialog: validate reason
alt invalid
Dialog->>User: show validation error
else valid
Dialog->>User: show confirmation summary
User->>Dialog: Confirm Waive
Dialog->>API: POST waive(workspacePath, reqId, body)
API-->>Dialog: Success (waiver created with waived_at)
Dialog->>SWR: mutate()
Dialog->>Page: onSuccess → close dialog / update UI
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
Review: feat(web-ui): waiver confirmation + audit trail for Proof pageGood implementation overall — the 2-step confirmation flow is exactly the right UX pattern for a compliance-adjacent action like waiving a proof gate, and the test coverage is thorough. A few things worth addressing before merge: 1. WaiveDialog is duplicated across two files (most important)The Extract it into a shared component, e.g. 2. Dialog step state not reset when dismissedIf a user opens the dialog, clicks Continue → to reach the confirmation step, then closes the dialog (X button or clicking outside) without confirming, the The dialog's <Dialog open={open} onOpenChange={(isOpen) => {
if (!isOpen) {
setStep('form');
setError(null);
}
setOpen(isOpen);
}}>3. User input lost on API error during confirmationWhen 4. Minor: use
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-ui/src/types/index.ts (1)
285-291:⚠️ Potential issue | 🟠 MajorBackend does not send
waived_at— field will always be undefined.The
waived_atfield is added to the frontendProofWaivertype, but the backendWaiverOutmodel (codeframe/ui/routers/proof_v2.py:96-102) does not include this field. The underlyingWaiverdataclass (codeframe/core/proof/models.py:94-100) also lacks it.The detail page at
web-ui/src/app/proof/[req_id]/page.tsxconditionally renders this field:{req.waiver.waived_at && ( <p className="mt-1 text-sm text-muted-foreground">Waived: {new Date(req.waiver.waived_at).toLocaleString()}</p> )}But the API response will never contain
waived_at, so this conditional will always be false.Either:
- Update the backend
WaiverOutmodel andWaiverdataclass to includewaived_at, or- Remove the frontend type field and the detail page conditional that depends on it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/types/index.ts` around lines 285 - 291, The frontend ProofWaiver type includes waived_at but the backend models Waiver (codeframe/core/proof/models.py) and WaiverOut (codeframe/ui/routers/proof_v2.py) do not provide it, so the page conditional in web-ui/src/app/proof/[req_id]/page.tsx will never render; either add waived_at to the backend dataclass and serializer (add an optional/nullable waived_at field to Waiver and include it in WaiverOut) so API responses include it, or remove waived_at from the ProofWaiver interface and delete the conditional rendering that checks req.waiver.waived_at in page.tsx to keep frontend and API types consistent.
🧹 Nitpick comments (3)
web-ui/__tests__/app/proof/req_id/page.test.tsx (1)
4-14: Consider extracting shared test utilities.The
localStorageMockimplementation is duplicated between this file andweb-ui/__tests__/app/proof/page.test.tsx. Consider extracting to a shared test utility file (e.g.,web-ui/__tests__/utils/test-helpers.ts).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/__tests__/app/proof/req_id/page.test.tsx` around lines 4 - 14, Extract the duplicated localStorageMock from the test file into a shared test helper module and import it where needed: create a module that exports the existing localStorageMock (the IIFE that returns getItem/setItem/removeItem/clear) and the Object.defineProperty(window, 'localStorage', ...) setup, then replace the inline implementations in page.test.tsx and req_id/page.test.tsx with an import of that helper (referencing the symbol localStorageMock and the window localStorage definition) so both tests reuse the same utility.web-ui/__tests__/app/proof/page.test.tsx (1)
218-223: Consider asserting the fullWaiveRequestpayload.The assertion uses
expect.objectContaining({ reason: ... })which only verifies the reason field. For more thorough validation, consider asserting all fields to ensure the complete payload is sent correctly:🔧 Suggested improvement
await waitFor(() => { - expect(mockWaive).toHaveBeenCalledWith('/test/workspace', 'REQ-001', expect.objectContaining({ - reason: 'Risk accepted', - })); + expect(mockWaive).toHaveBeenCalledWith('/test/workspace', 'REQ-001', { + reason: 'Risk accepted', + expires: null, + manual_checklist: [], + approved_by: '', + }); expect(mutate).toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/__tests__/app/proof/page.test.tsx` around lines 218 - 223, The test currently only checks the reason field via expect.objectContaining in the waitFor block; update the assertion for mockWaive to assert the full WaiveRequest payload instead of a partial match: replace expect.objectContaining({ reason: 'Risk accepted' }) with a complete object matching all expected WaiveRequest fields (e.g., reason, requestedBy, timestamp/expiry or any other properties your WaiveRequest includes) so mockWaive('/test/workspace', 'REQ-001', <fullWaiveRequest>) is fully validated; ensure the test uses the exact payload shape produced by the code under test (matching property names and types) and keep the mutate call assertion as-is.web-ui/src/app/proof/page.tsx (1)
29-154: Consider extractingWaiveDialogto a shared component.This
WaiveDialogimplementation is nearly identical to the one inweb-ui/src/app/proof/[req_id]/page.tsx. The only difference is the prop name (requirementvsreqId). Extracting this to a shared component (e.g.,@/components/proof/WaiveDialog.tsx) would reduce duplication and ensure consistent behavior.🤖 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 29 - 154, The WaiveDialog component is duplicated; extract it into a shared component (e.g., create a new component file exporting WaiveDialog) and replace both occurrences with imports—preserve the existing API: props (requirement or reqId can be normalized to a single prop name like requirement or accept either), callbacks onClose and onSuccess, and internal logic (state, handleContinue, handleConfirm using proofApi.waive, submitting/error handling). Update callers in web-ui/src/app/proof/page.tsx and web-ui/src/app/proof/[req_id]/page.tsx to import the new component and pass the correct requirement/reqId (map reqId to a requirement object or update prop name), and ensure typings (ProofRequirement, WaiveRequest) and CSS/class usage remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@web-ui/src/types/index.ts`:
- Around line 285-291: The frontend ProofWaiver type includes waived_at but the
backend models Waiver (codeframe/core/proof/models.py) and WaiverOut
(codeframe/ui/routers/proof_v2.py) do not provide it, so the page conditional in
web-ui/src/app/proof/[req_id]/page.tsx will never render; either add waived_at
to the backend dataclass and serializer (add an optional/nullable waived_at
field to Waiver and include it in WaiverOut) so API responses include it, or
remove waived_at from the ProofWaiver interface and delete the conditional
rendering that checks req.waiver.waived_at in page.tsx to keep frontend and API
types consistent.
---
Nitpick comments:
In `@web-ui/__tests__/app/proof/page.test.tsx`:
- Around line 218-223: The test currently only checks the reason field via
expect.objectContaining in the waitFor block; update the assertion for mockWaive
to assert the full WaiveRequest payload instead of a partial match: replace
expect.objectContaining({ reason: 'Risk accepted' }) with a complete object
matching all expected WaiveRequest fields (e.g., reason, requestedBy,
timestamp/expiry or any other properties your WaiveRequest includes) so
mockWaive('/test/workspace', 'REQ-001', <fullWaiveRequest>) is fully validated;
ensure the test uses the exact payload shape produced by the code under test
(matching property names and types) and keep the mutate call assertion as-is.
In `@web-ui/__tests__/app/proof/req_id/page.test.tsx`:
- Around line 4-14: Extract the duplicated localStorageMock from the test file
into a shared test helper module and import it where needed: create a module
that exports the existing localStorageMock (the IIFE that returns
getItem/setItem/removeItem/clear) and the Object.defineProperty(window,
'localStorage', ...) setup, then replace the inline implementations in
page.test.tsx and req_id/page.test.tsx with an import of that helper
(referencing the symbol localStorageMock and the window localStorage definition)
so both tests reuse the same utility.
In `@web-ui/src/app/proof/page.tsx`:
- Around line 29-154: The WaiveDialog component is duplicated; extract it into a
shared component (e.g., create a new component file exporting WaiveDialog) and
replace both occurrences with imports—preserve the existing API: props
(requirement or reqId can be normalized to a single prop name like requirement
or accept either), callbacks onClose and onSuccess, and internal logic (state,
handleContinue, handleConfirm using proofApi.waive, submitting/error handling).
Update callers in web-ui/src/app/proof/page.tsx and
web-ui/src/app/proof/[req_id]/page.tsx to import the new component and pass the
correct requirement/reqId (map reqId to a requirement object or update prop
name), and ensure typings (ProofRequirement, WaiveRequest) and CSS/class usage
remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5689c7e5-9642-4a91-ac1c-ce37a14f3449
📒 Files selected for processing (6)
web-ui/__mocks__/@hugeicons/react.jsweb-ui/__tests__/app/proof/page.test.tsxweb-ui/__tests__/app/proof/req_id/page.test.tsxweb-ui/src/app/proof/[req_id]/page.tsxweb-ui/src/app/proof/page.tsxweb-ui/src/types/index.ts
- Add waived_at to backend Waiver dataclass, ledger serialization, and WaiverOut model so the API returns the timestamp when requirements are waived; waive endpoint sets waived_at = datetime.now(UTC) - Extract shared WaiveDialog to src/components/proof/WaiveDialog.tsx; both proof/page.tsx and proof/[req_id]/page.tsx now import the single implementation instead of duplicating it - Extract localStorageMock to __tests__/utils/test-helpers.ts and import from both proof test files to eliminate duplication - Strengthen WaiveRequest assertion in tests to validate all payload fields
Follow-up reviewGood progress — the three biggest items from my previous review and CodeRabbit's initial pass have all been addressed:
A few things still need attention: 1. Dialog step not reset on close (my item #2 — still open) From my previous review: if a user advances to the confirm step and then dismisses the dialog without confirming, step stays at confirm and they see the confirmation screen on next open. The onOpenChange handler should reset state on close: check if !isOpen, then setStep to form and setError to null before calling setOpen(isOpen). 2. Silent catch {} swallows the API error WaiveDialog.tsx has a bare catch with no variable binding, so the actual server error is discarded. If the backend returns a 422 with a specific message, the user always sees the generic string. At minimum, bind the error and log it: catch (err) { console.error('Waive failed:', err); ... }. Note: resetting to form on error still discards the user's entered values (my item #3 from before). If that is an acceptable trade-off for simplicity, a brief inline comment documenting the intent would be helpful. 3. localStorageMock runs as a module-level side-effect in test-helpers.ts The shared helper applies Object.defineProperty(window, 'localStorage', ...) at module import time. Any test that imports from test-helpers.ts will have localStorage globally replaced — including tests that do not need the mock. Since Object.defineProperty without configurable: true cannot be re-defined by later tests, this may cause ordering-dependent failures across the test suite. Safer pattern: export the setup as a function and call it inside beforeAll in each suite that needs it. 4. Minor: cn() still not used for conditional row class The string concatenation pattern is still in proof/page.tsx. Fine to leave for a follow-up if you want to keep this PR focused. Priority before merge: items 1 and 3 are the ones most likely to surface in testing. Item 2 is a meaningful improvement for a compliance-adjacent action. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
codeframe/ui/routers/proof_v2.py (1)
370-376:⚠️ Potential issue | 🟠 MajorMove waiver timestamping out of the HTTP route and into core waiver logic.
Line 375 adds business-state mutation in the router. This also creates inconsistent audit behavior across entry points (e.g., CLI waiver creation can still persist
waived_at=None), while the API path always stamps it.Proposed fix (keep router thin, centralize timestamping in core)
diff --git a/codeframe/ui/routers/proof_v2.py b/codeframe/ui/routers/proof_v2.py @@ - waiver = Waiver( + waiver = Waiver( reason=body.reason, expires=body.expires, manual_checklist=body.manual_checklist, approved_by=body.approved_by, - waived_at=datetime.now(timezone.utc), ) diff --git a/codeframe/core/proof/ledger.py b/codeframe/core/proof/ledger.py @@ def waive_requirement( workspace: Workspace, req_id: str, waiver: Waiver ) -> Optional[Requirement]: """Waive a requirement with reason and optional expiry.""" + if waiver.waived_at is None: + waiver = Waiver( + reason=waiver.reason, + expires=waiver.expires, + manual_checklist=waiver.manual_checklist, + approved_by=waiver.approved_by, + waived_at=_utc_now(), + ) _ensure_tables(workspace)As per coding guidelines, "Server layer must follow thin adapter pattern: routes delegate to core modules without duplicating business logic".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/ui/routers/proof_v2.py` around lines 370 - 376, The router currently sets waived_at when constructing Waiver in the HTTP handler; remove this business-state mutation from the route and instead set waived_at inside the core Waiver creation logic (e.g., in the Waiver constructor/factory or a core function like Waiver.create()/create_waiver()) so all entry points (API, CLI, etc.) consistently stamp waived_at = datetime.now(timezone.utc) if not provided; update the router to pass only reason, expires, manual_checklist, approved_by and let the core enforce timestamping and auditing.
🤖 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/__tests__/utils/test-helpers.ts`:
- Line 6: The getItem implementation in test-helpers.ts uses "getItem: (key:
string) => store[key] || null" which returns null for falsy stored values like
empty string; change it to return the actual stored value or null only when the
key is absent (e.g., use the nullish coalescing behavior or explicit
hasOwnProperty check) so getItem(key) returns "" for stored empty strings and
null only when the key does not exist; update the getItem function and any tests
that assume real localStorage semantics accordingly.
---
Outside diff comments:
In `@codeframe/ui/routers/proof_v2.py`:
- Around line 370-376: The router currently sets waived_at when constructing
Waiver in the HTTP handler; remove this business-state mutation from the route
and instead set waived_at inside the core Waiver creation logic (e.g., in the
Waiver constructor/factory or a core function like
Waiver.create()/create_waiver()) so all entry points (API, CLI, etc.)
consistently stamp waived_at = datetime.now(timezone.utc) if not provided;
update the router to pass only reason, expires, manual_checklist, approved_by
and let the core enforce timestamping and auditing.
🪄 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: 9da24dc8-fb60-4345-9e8d-f6aa846ceed5
📒 Files selected for processing (10)
codeframe/core/proof/ledger.pycodeframe/core/proof/models.pycodeframe/ui/routers/proof_v2.pyweb-ui/__tests__/app/proof/page.test.tsxweb-ui/__tests__/app/proof/req_id/page.test.tsxweb-ui/__tests__/utils/test-helpers.tsweb-ui/src/app/proof/[req_id]/page.tsxweb-ui/src/app/proof/page.tsxweb-ui/src/components/proof/WaiveDialog.tsxweb-ui/src/components/proof/index.ts
✅ Files skipped from review due to trivial changes (1)
- web-ui/src/components/proof/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- web-ui/tests/app/proof/page.test.tsx
- web-ui/tests/app/proof/req_id/page.test.tsx
- web-ui/src/app/proof/page.tsx
| export const localStorageMock = (() => { | ||
| let store: Record<string, string> = {}; | ||
| return { | ||
| getItem: (key: string) => store[key] || null, |
There was a problem hiding this comment.
Falsy value bug: getItem returns null for empty strings.
Using || null means stored empty strings ("") will incorrectly return null since "" is falsy. Real localStorage.getItem returns the exact stored value.
🐛 Proposed fix using nullish coalescing
- getItem: (key: string) => store[key] || null,
+ getItem: (key: string) => store[key] ?? null,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getItem: (key: string) => store[key] || null, | |
| getItem: (key: string) => store[key] ?? null, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web-ui/__tests__/utils/test-helpers.ts` at line 6, The getItem implementation
in test-helpers.ts uses "getItem: (key: string) => store[key] || null" which
returns null for falsy stored values like empty string; change it to return the
actual stored value or null only when the key is absent (e.g., use the nullish
coalescing behavior or explicit hasOwnProperty check) so getItem(key) returns ""
for stored empty strings and null only when the key does not exist; update the
getItem function and any tests that assume real localStorage semantics
accordingly.
Per thin-adapter pattern, business logic (setting waived_at) belongs in core not in the HTTP router. waive_requirement() in ledger.py now stamps waived_at = datetime.now(UTC) when not already set, ensuring all entry points (API, CLI) consistently record the audit timestamp.
Code Review — PR #521: waiver confirmation + audit trailOverall this is a clean, well-structured change. The extraction of Python backend
# current (fragile if Waiver grows more fields)
if waiver.waived_at is None:
waiver = Waiver(
reason=waiver.reason,
expires=waiver.expires,
manual_checklist=waiver.manual_checklist,
approved_by=waiver.approved_by,
waived_at=datetime.now(timezone.utc),
)
# cleaner
from dataclasses import replace
if waiver.waived_at is None:
waiver = replace(waiver, waived_at=datetime.now(timezone.utc))Not a bug, but the current form will silently drop any new fields added to Frontend
TestsCoverage is solid — 17 tests spanning the 2-step flow, validation, back-navigation with state preserved, API calls, and visual treatment for waived rows. Two gaps:
Missing: API error path test: There's no test covering Summary
The |
1 participant
Closes #479
Summary
proof/page.tsxandproof/[req_id]/page.tsx. The confirmation explains compliance implications and shows a summary of the entered data. Back button returns to the form with values preserved.waived_at?: string | nulltoProofWaivertype. Detail page now renders the waiver timestamp alongside reason, actor, and expiry when the API returns it.opacity-60styling, clearly separating them from open/satisfied requirements.Acceptance Criteria
Test plan
__tests__/app/proof/page.test.tsxand__tests__/app/proof/req_id/page.test.tsx— all passingNewSessionModal.tsxunrelated to this PRSummary by CodeRabbit
New Features
Tests