feat(security-agent): add audit report UI#4082
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryIncremental diff (13 files) over the previously reviewed commit — clean refactoring of state management (useState → useReducer), component extraction (badge/button variants), React 19 pattern migration (forwardRef → ref prop), and semantic token adoption. No bugs, regressions, or memory leaks. Files Reviewed (13 files in incremental diff)
Previous Review Summary (commit 4b02241)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 4b02241)Status: No Issues Found | Recommendation: Merge Executive SummaryLarge UI feature adding audit report pages, refreshed dashboard, and finding detail flows — all well-structured with clean React patterns, proper URL validation, and no memory leaks. Files Reviewed (42 files)
Reviewed by deepseek-v4-pro-20260423 · 461,582 tokens Review guidance: REVIEW.md from base branch |
|
|
||
| export function SecurityAuditReportPage() { | ||
| const trpc = useTRPC(); | ||
| const searchParams = useSearchParams(); |
There was a problem hiding this comment.
Low, Functional: Filter and date-range state is read from the URL on mount but never written back. useSearchParams() is read once inside the useReducer initializer (here and the parseAuditReportFilters call below), and submit-report/filter changes only update local reducer state. There is no router.replace/pushState anywhere in the file.
The PR notes describe this state as "URL-backed," but it is one-way: a pasted deep link is honored on first load, yet once the user changes the period or a filter the address bar goes stale. They cannot bookmark or share the view they are looking at, and back/forward will not move between filter states. For an audit report this sharing case is plausible.
Suggestion: on submit-report (and optionally filter changes), push the current range and filters to the URL with router.replace. If read-only deep-linking is the intended scope, consider softening the "URL-backed" wording so it does not imply round-tripping.
| return finding.status === 'ignored' && finding.ignored_reason?.startsWith('superseded:'); | ||
| } | ||
|
|
||
| export function getAnalysisPresentation(finding: SecurityFinding): FindingStatusPresentation { |
There was a problem hiding this comment.
Low, Design: This getAnalysisPresentation and getAnalysisStatus in FindingDetailDialog.tsx both map analysis_status + sandboxAnalysis.isExploitable + triage.suggestedAction to a label and tone, and they have already drifted. This helper omits the sandbox.extractionStatus === 'failed' and isExploitable === 'unknown' branches the dialog handles, and labels the not-exploitable case "Not exploitable" where the dialog says "No reachable path."
The result is that the same finding can read differently depending on whether it is shown in the list or the detail dialog. Only this helper is unit-tested.
Suggestion: extract one shared, tested helper for the analysis-status-to-presentation mapping (surfaces can still pick their own icon/tooltip from a common state), or at minimum add a test around the dialog's getAnalysisStatus so the two stay aligned.
| } | ||
|
|
||
| function formatReportDateTime(value: string): string { | ||
| return formatDateTime24Hour(value).replace(/, (\d{2}:\d{2} UTC)$/, ' at $1'); |
There was a problem hiding this comment.
Nit, Design: formatReportDateTime reshapes the formatted string with .replace(/, (\d{2}:\d{2} UTC)$/, ' at $1'), which depends on the exact Intl.DateTimeFormat output shape. If an ICU/runtime version formats the parts differently the replace silently no-ops and you get the unmodified string. Degradation is graceful, just brittle. Assembling the date and time from formatToParts (or composing two formatters) would avoid relying on the output layout.
2 participants
Summary
Security Agent now has the web UI for owner-scoped Audit Reports and a refreshed management experience, stacked on the backend/API foundation in #4081.
Why this change is needed
Security owners need an interactive way to review recorded Security Finding evidence by period, repository, severity, and state. Splitting the UI keeps the deployable backend migration and API review separate from the larger visual and interaction changes.
How this is addressed
react-day-pickerfor report date-range selection.Verification
Visual Changes
Reviewer Notes
Human Reviewer Flags
Code Reviewer Agent
Code Reviewer Notes