[failproofai-434] dashboard aesthetic redesign — audit-forward, calmer others#434
Merged
Conversation
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 20 minutes and 16 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughImplements fire-and-forget server audit runs with explicit finishRun(error) signaling, extends status responses with run errors, rewrites client rerun polling to poll indefinitely with network-failure tolerance, updates UI copy/params to use since: "all", adds/updates tests, and pins dev tooling versions plus .gitignore entry. ChangesDevelopment Configuration Updates
Audit run/rerun flow and UI
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…#434) The audit first run failed on the first click (a retry worked). triggerRun POSTed the audit run route through fetchWithTimeout's 15s default, but the route ran runAudit synchronously, so a cold scan (~17s) was aborted client-side while the server kept going and warmed the caches — making the second click succeed. Per the user's direction (no timeout at all; scan all history): - Server: the run route (app/api/audit/run/route.ts) is now fire-and-forget. It starts runAudit() as a detached task in the long-lived server process and returns 202 immediately; the route maxDuration cap is dropped. _state.ts gains an error field and finishRun(error); the 5-min lock auto-expiry is removed so a long-but-healthy run is never mistaken for a wedged one. The status route surfaces the error. - Client: triggerRun polls the status route with no duration cap; only ~10 consecutive lost-server polls abort it (network). A finished run carrying an error rejects post_failed; otherwise resolves (cache written before finishRun(null)). - Default window drops from 30 days to all history; empty-state and run-progress copy updated to "may take a while". Tests: rewrite the audit-state and rerun-button suites (finishRun/error, no-auto-expiry, and triggerRun cases: 202+resolve, long-run-no-cap, error to post_failed, consecutive-failures to network, non-OK POST, 409 to poll) and add a route test asserting the POST returns 202 without awaiting the run, 409s concurrents, and records a detached-run error. Full suite (1858) + lint + tsc green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… install passes (#434) c75edad pinned the @types and typescript dev dependencies but left bun.lock inconsistent with package.json, so CI's frozen-lockfile install failed at the Install dependencies step (test, test-e2e, hook-log-file jobs). Regenerate bun.lock to match; a local frozen install now exits 0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![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
🧹 Nitpick comments (3)
__tests__/audit/rerun-button.test.ts (1)
68-86: 💤 Low valueConsider removing the implementation-detail check.
Lines 84-85 verify that no explicit timeout override is passed to the kickoff POST. This tests an implementation detail (absence of a parameter) rather than observable behavior. If the internal implementation changes (e.g., a future refactor adds a default timeout), this assertion would fail even though the behavior is correct.
Recommended alternative
Focus on behavior: either remove the check entirely (the test already validates the 202 kickoff succeeds), or verify that the kickoff completes quickly (e.g., assert it resolves within a small timeout like 100ms).
- // The kickoff POST uses the default fast timeout (no explicit override) — - // it only times the 202 kickoff, not the run. - const runCall = fetchMock.mock.calls.find((c) => c[0] === "/api/audit/run"); - expect(runCall?.[2]).toBeUndefined();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/audit/rerun-button.test.ts` around lines 68 - 86, Remove the implementation-detail assertion that inspects the kickoff POST options: delete the lines that find runCall from fetchMock.mock.calls and assert runCall?.[2] is undefined; instead rely on the existing behavioral assertions (the test already checks the 202 kickoff and that triggerRun resolves) or, if you prefer a timing check, add a short behavioral assertion that the kickoff completes quickly (e.g., assert triggerRun settles within a small timeout) referencing the test's triggerRun call, fetchMock mock, and POLL_INTERVAL_MS.app/audit/_components/rerun-button.tsx (2)
59-119: ⚡ Quick winConsider adding cancellation support to prevent resource leaks.
The
triggerRunasync function has no mechanism to cancel the polling loop once started. If the calling component unmounts (e.g., user navigates away during a scan), the loop continues to fetch/api/audit/statusevery second indefinitely until completion. While the practical impact is limited because the fetch has built-in timeouts and the/auditpage rarely unmounts during active scans, supporting cancellation would be cleaner.♻️ Recommended approach using AbortSignal
Accept an optional
AbortSignaland check it in the loop:-export async function triggerRun(scanParams: ScanParams): Promise<void> { +export async function triggerRun(scanParams: ScanParams, signal?: AbortSignal): Promise<void> { + if (signal?.aborted) throw new RerunError("network", "cancelled"); + // Start the run... try { const res = await fetchWithTimeout("/api/audit/run", { method: "POST", headers: { "content-type": "application/json" }, body: JSON.stringify(paramsToBody(scanParams)), + signal, }); // ... } let consecutiveFailures = 0; for (;;) { + if (signal?.aborted) throw new RerunError("network", "cancelled"); await new Promise((r) => setTimeout(r, POLL_INTERVAL_MS)); // ... } }Callers can then wire it up with an AbortController in a
useEffectcleanup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/audit/_components/rerun-button.tsx` around lines 59 - 119, The triggerRun function lacks cancellation support causing the polling loop to continue after caller unmounts; modify triggerRun to accept an optional AbortSignal parameter (e.g., add signal?: AbortSignal to its signature) and thread it into fetchWithTimeout calls and the polling loop: check signal.aborted before each fetch and after the timeout sleep (POLL_INTERVAL_MS), abort ongoing fetches by passing the signal to fetchWithTimeout, and throw a RerunError("aborted", ...) or rethrow an AbortError when signal.aborted; ensure existing logic around consecutiveFailures, MAX_CONSECUTIVE_POLL_FAILURES, and handling of fetchWithTimeout remains unchanged.
89-94: ⚡ Quick winAdd runtime validation for the status response shape.
The type assertion
as { running: boolean; error?: string | null }on line 93 is unsafe. If the server ever returns a malformed response (e.g.,{ running: "true" }as a string instead of boolean), the assertion will lie to TypeScript and cause subtle runtime bugs when checking!status.runninglater.Since
/api/audit/statusis an internal endpoint, the risk is low, but adding a minimal runtime guard would make this more robust:♻️ Simple validation guard
let status: { running: boolean; error?: string | null } | null = null; try { const sres = await fetchWithTimeout("/api/audit/status", { cache: "no-store" }); if (sres.ok) { - status = (await sres.json()) as { running: boolean; error?: string | null }; + const parsed = await sres.json(); + if (parsed && typeof parsed.running === "boolean") { + status = parsed as { running: boolean; error?: string | null }; + } } } catch { // Transient fetch/JSON failure... }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/audit/_components/rerun-button.tsx` around lines 89 - 94, The current type assertion for status after fetchWithTimeout("/api/audit/status") is unsafe; instead parse the JSON into a temp value, perform a runtime guard that checks typeof temp.running === "boolean" and (temp.error === undefined || temp.error === null || typeof temp.error === "string"), and only then assign status = temp as { running: boolean; error?: string | null }; if the guard fails set status = null (or log/throw) so later checks like !status.running are safe; update the code around the status assignment in rerun-button.tsx (the fetchWithTimeout response handling) to implement this validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@__tests__/api/audit-run-route.test.ts`:
- Around line 29-79: Add a new test that simulates a successful detached run:
mock runAuditMock to return a Promise that you resolve later, call
POST(req("{}")) and assert immediate 202 and running lock set via
getRunState().running, then resolve the background promise, await a tick or two
for its .then to settle, and assert writeDashboardCache (writeCacheMock) was
called and the lock was cleared by finishRun(null) (i.e., getRunState().running
is false and error is null); reference POST(req("{}")), runAuditMock,
writeCacheMock, finishRun, and getRunState to locate code.
In `@app/api/audit/run/route.ts`:
- Around line 91-93: writeDashboardCache currently swallows exceptions so
finishRun(null) is called even when the cache write failed; change the call site
in the route where runAudit, writeDashboardCache, finishRun are used to detect
actual cache write success by either (A) updating writeDashboardCache to return
a boolean/Promise<boolean> or rethrow errors, and then await its result here, or
(B) wrap writeDashboardCache(opts, result) in a try/catch that passes the caught
error into finishRun(error) on failure and finishRun(null) on success; ensure
finishRun receives a real error when the cache write did not persist.
---
Nitpick comments:
In `@__tests__/audit/rerun-button.test.ts`:
- Around line 68-86: Remove the implementation-detail assertion that inspects
the kickoff POST options: delete the lines that find runCall from
fetchMock.mock.calls and assert runCall?.[2] is undefined; instead rely on the
existing behavioral assertions (the test already checks the 202 kickoff and that
triggerRun resolves) or, if you prefer a timing check, add a short behavioral
assertion that the kickoff completes quickly (e.g., assert triggerRun settles
within a small timeout) referencing the test's triggerRun call, fetchMock mock,
and POLL_INTERVAL_MS.
In `@app/audit/_components/rerun-button.tsx`:
- Around line 59-119: The triggerRun function lacks cancellation support causing
the polling loop to continue after caller unmounts; modify triggerRun to accept
an optional AbortSignal parameter (e.g., add signal?: AbortSignal to its
signature) and thread it into fetchWithTimeout calls and the polling loop: check
signal.aborted before each fetch and after the timeout sleep (POLL_INTERVAL_MS),
abort ongoing fetches by passing the signal to fetchWithTimeout, and throw a
RerunError("aborted", ...) or rethrow an AbortError when signal.aborted; ensure
existing logic around consecutiveFailures, MAX_CONSECUTIVE_POLL_FAILURES, and
handling of fetchWithTimeout remains unchanged.
- Around line 89-94: The current type assertion for status after
fetchWithTimeout("/api/audit/status") is unsafe; instead parse the JSON into a
temp value, perform a runtime guard that checks typeof temp.running ===
"boolean" and (temp.error === undefined || temp.error === null || typeof
temp.error === "string"), and only then assign status = temp as { running:
boolean; error?: string | null }; if the guard fails set status = null (or
log/throw) so later checks like !status.running are safe; update the code around
the status assignment in rerun-button.tsx (the fetchWithTimeout response
handling) to implement this validation.
🪄 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: 086e1e24-4bc9-48f1-b9d9-e23c63b50174
📒 Files selected for processing (12)
CHANGELOG.md__tests__/api/audit-run-route.test.ts__tests__/api/audit-state.test.ts__tests__/audit/rerun-button.test.tsapp/api/audit/_state.tsapp/api/audit/run/route.tsapp/api/audit/status/route.tsapp/audit/_components/audit-dashboard.tsxapp/audit/_components/audit-progress-strip.tsxapp/audit/_components/empty-state.tsxapp/audit/_components/rerun-button.tsxapp/audit/_components/run-progress.tsx
✅ Files skipped from review due to trivial changes (2)
- app/audit/_components/audit-progress-strip.tsx
- CHANGELOG.md
Addresses CodeRabbit review on the fire-and-forget run. writeDashboardCache swallowed its own IO errors and returned void, so the run route called finishRun(null) even when the result was never persisted; since the cache is the only channel by which a detached run's result reaches the client, the poller would report success while the user got stale data. writeDashboardCache now returns a boolean (true on persist, false on a swallowed write error) and the route reports a run error via finishRun when the write fails. Adds the success-path route test (cache written, lock cleared, no error) and a write-failure test (error surfaced), per the review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Brainstorming + design for redesigning the failproofai dashboard aesthetic, referencing demo videos under
C:\Users\mathr\OneDrive\Desktop\demos. The audit pane will be pushed further into the brutalist pixel-craft direction to encourage quirky sharing; policies / projects / project-detail get calmer so they read as instruments rather than billboards.The aesthetic-redesign spec + implementation will land as follow-up commits on this branch.
Landed so far
/audit's first run no longer fails on the first click, and the scan is no longer time-capped.POST /api/audit/runis now fire-and-forget (startsrunAudit()detached, returns202); the client polls/api/audit/statuswith no duration cap (only a ~10-poll lost-connection backstop stops it); the_staterun-lock's 5-minute auto-expiry is removed; server-side run errors surface via the status endpoint; and the default scan window becomes the user's entire session history instead of the last 30 days. New tests cover the fire-and-forget route, the unbounded poll, and the run-state machine. (commitsecc243e,2203835)@types/*+ typescript, ignore.tmp/, and reconcilebun.lockso CI's--frozen-lockfileinstall passes.Test plan
bun run test:run— 1858 tests),bun run lintclean,tsc --noEmitclean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
User-facing Changes
Chores
Tests