Implement full stack admin dashboard#662
Conversation
📝 WalkthroughWalkthroughAdds a full-stack admin dashboard: a new Alembic migration and SQLAlchemy model for Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/__tests__/AdminDashboard.test.tsx`:
- Around line 364-378: Update the mock return in the test for
AdminDashboard.test.tsx so the audit entry includes a non-admin role to exercise
the role-badge branch in AuditLogPanel: change the entry passed to
adminData.useAuditLog in the 'renders audit entries' test to include role:
'reviewer' (and actor: 'mod_user' if desired) instead of omitting it, then
assert the role badge is rendered (e.g., by checking for the role text or the
existing audit-entry test id) so the AuditLogPanel conditional at lines 107-111
is covered.
In `@frontend/src/components/admin/AdminLayout.tsx`:
- Around line 49-54: Replace the static OAuth state with a cryptographically
random value in handleGitHubLogin: generate a secure random string (e.g., from
crypto.getRandomValues or a secure helper), store it in sessionStorage under a
new key (e.g., 'sf_admin_oauth_state') alongside the existing
'sf_admin_oauth_pending' flag, and include that random value as the state query
param in the redirect URL instead of 'admin_login'; then update the AdminPage
callback handler to read the returned state param and verify it matches the
stored 'sf_admin_oauth_state' before accepting the token and clearing the
sessionStorage keys.
- Around line 157-180: The WebSocket hook is currently invoked before the auth
check causing unnecessary connection attempts; update AdminLayout so
useAdminWebSocket(handleWsEvent) is only called when the user is authenticated
(i.e., after/authed is true) — either move the call below the if (!authed)
return AdminLoginForm branch or gate it with authed (call useAdminWebSocket only
when authed) and keep handleWsEvent as the callback; ensure any dependent
variables (previously const { status: wsStatus }) are derived from the
conditional hook result or replaced with a safe default when not authed.
In `@frontend/src/components/admin/AuditLogPanel.tsx`:
- Around line 16-25: The relativeTime function can return negative values for
future timestamps (e.g., "-5s ago"); update relativeTime(iso: string) to detect
when Date.now() - new Date(iso).getTime() is negative and handle that case
(e.g., treat negative diffs as zero or return a "just now" / "in the future"
string) so the UI never displays negative durations; modify the logic inside
relativeTime to clamp the diff to >=0 (or branch to a friendly future message)
and ensure callers like where entry.timestamp is passed continue to work.
In `@frontend/src/components/admin/BountyManagement.tsx`:
- Around line 139-149: The handleSubmit function currently does Number(reward)
which converts an empty string to 0 and allows invalid payloads; update
handleSubmit (the BountyAdminCreate creation path before calling
create.mutateAsync) to validate reward explicitly: parse the reward (e.g.,
parseFloat or Number) and check that the input is non-empty and the numeric
value is > 0 (or produce a validation error/return early), and only call
create.mutateAsync(payload) when reward_amount > 0; reference handleSubmit, the
reward variable, BountyAdminCreate and create.mutateAsync when making the
change.
- Around line 41-44: handleClose currently calls close.mutateAsync(bounty.id)
but doesn't surface failures to the user; update the handleClose function to
catch errors from close.mutateAsync and present them (e.g., set a local error
state or call the existing UI toast/error handler) instead of silently failing,
and only call onClose() after a successful mutation; reference the handleClose
function and the close.mutateAsync(...) call to locate where to add a try/catch
and error-display logic.
In `@frontend/src/components/admin/ContributorManagement.tsx`:
- Around line 204-212: The unban button currently uses a shared unban.isPending
flag which disables every row during any unban; change to track pending state
per contributor ID by replacing the global check with a per-ID check (e.g.,
maintain a Set or Map pendingUnbans in the ContributorManagement component keyed
by c.id or extend the mutation to expose isPending for a specific id), update
that set in the unban.mutate lifecycle (onMutate add id, onError/onSettled
remove id) and use pendingUnbans.has(c.id) (or a helper like
unban.isPendingFor(id)) to disable only the specific row's button; keep using
unban.mutate(c.id) for the call but wire up per-id pending state in the mutation
callbacks to locate code around unban.mutate and the button rendering for c.id.
In `@frontend/src/components/admin/FinancialPanel.tsx`:
- Around line 83-87: In FinancialPanel, replace the plain anchor used for
internal navigation (the <a href={`/bounties/${p.bounty_id}`}> wrapping
{p.bounty_title}) with react-router-dom's Link to avoid full page reloads;
import { Link } from 'react-router-dom', keep the same className and text
content, and use Link's to={`/bounties/${p.bounty_id}`} so client-side routing
preserves React state and avoids re-authentication.
- Around line 11-16: The FinancialPanel component is missing error handling for
the two data hooks; update FinancialPanel to check the error returns from
useFinancialOverview and usePayoutHistory (e.g., overviewError / payoutsError or
isError fields from those hooks) alongside ovLoading and payLoading, and render
a clear error state (a message or fallback UI) when either fetch fails instead
of showing an empty UI; reference the existing variables (overview, payouts,
ovLoading, payLoading) and add conditional rendering paths that show the error
details and optionally a retry action.
In `@frontend/src/components/admin/ReviewPipeline.tsx`:
- Around line 81-85: Replace the plain anchor in ReviewPipeline.tsx that links
to `/bounties/${r.bounty_id}` with React Router's Link to avoid full page
reloads: import { Link } from 'react-router-dom' if not present, and change the
<a href={`/bounties/${r.bounty_id}`}> usage to a <Link
to={`/bounties/${r.bounty_id}`}> while preserving the existing className
("text-[`#9945FF`] hover:underline") and children (`{r.bounty_title}`) so
client-side routing is used instead of a full navigation.
In `@frontend/src/hooks/useAdminWebSocket.ts`:
- Around line 83-96: The effect currently calls connect() while disabling
exhaustive-deps, which risks a stale onEvent capture; update the hook to ensure
the latest onEvent is used by storing onEvent in a ref (e.g., create
onEventRef.current = onEvent in a small useEffect that runs when onEvent
changes) and have connect and any websocket event handlers call
onEventRef.current(...) instead of the closed-over onEvent; keep the outer
useEffect (that toggles shouldReconnect, starts connect, clears reconnectTimer,
and closes wsRef) with an empty deps array while removing the eslint override,
or alternatively document in the hook API that callers must memoize onEvent (but
prefer the ref approach for safety).
- Around line 51-64: ws.onmessage currently assumes evt.data is a string which
can throw or ignore valid JSON in Blob/ArrayBuffer frames; update the handler
(ws.onmessage) to first guard on typeof evt.data and convert ArrayBuffer/Blob to
a UTF-8 string (e.g., use TextDecoder for ArrayBuffer and blob.text() or
FileReader for Blob) before JSON.parse, then build the AdminWsEvent as before
and call setLastEvent and onEvent; keep the existing try/catch to silently
ignore non-JSON frames but ensure the conversion is awaited/handled so parsing
succeeds for binary frames.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 533686a0-048f-4391-9a2a-80899081518e
📒 Files selected for processing (19)
backend/alembic/versions/003_add_admin_audit_log.pybackend/app/api/admin.pybackend/app/main.pybackend/app/models/tables.pybackend/tests/test_admin.pyfrontend/src/App.tsxfrontend/src/__tests__/AdminDashboard.test.tsxfrontend/src/components/admin/AdminLayout.tsxfrontend/src/components/admin/AuditLogPanel.tsxfrontend/src/components/admin/BountyManagement.tsxfrontend/src/components/admin/ContributorManagement.tsxfrontend/src/components/admin/FinancialPanel.tsxfrontend/src/components/admin/OverviewPanel.tsxfrontend/src/components/admin/ReviewPipeline.tsxfrontend/src/components/admin/SystemHealth.tsxfrontend/src/hooks/useAdminData.tsfrontend/src/hooks/useAdminWebSocket.tsfrontend/src/pages/AdminPage.tsxfrontend/src/types/admin.ts
| it('renders audit entries', () => { | ||
| vi.mocked(adminData.useAuditLog).mockReturnValue({ | ||
| ...noopQuery(), | ||
| data: { | ||
| entries: [ | ||
| { event: 'admin_bounty_closed', actor: 'admin', timestamp: new Date().toISOString(), details: { bounty_id: 'b1' } }, | ||
| ], | ||
| total: 1, | ||
| }, | ||
| } as ReturnType<typeof adminData.useAuditLog>); | ||
| const qc = makeQC(); | ||
| render(<Wrapper qc={qc}><AuditLogPanel /></Wrapper>); | ||
| expect(screen.getByText('admin_bounty_closed')).toBeDefined(); | ||
| expect(screen.getByTestId('audit-entry-0')).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Audit entry mock missing optional role field.
The AuditLogPanel component conditionally renders a role badge when entry.role exists and isn't "admin" (lines 107-111 in AuditLogPanel.tsx). Consider adding a test case with a non-admin role to ensure that branch is covered:
{ event: 'admin_bounty_closed', actor: 'mod_user', role: 'reviewer', timestamp: new Date().toISOString(), details: { bounty_id: 'b1' } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/__tests__/AdminDashboard.test.tsx` around lines 364 - 378,
Update the mock return in the test for AdminDashboard.test.tsx so the audit
entry includes a non-admin role to exercise the role-badge branch in
AuditLogPanel: change the entry passed to adminData.useAuditLog in the 'renders
audit entries' test to include role: 'reviewer' (and actor: 'mod_user' if
desired) instead of omitting it, then assert the role badge is rendered (e.g.,
by checking for the role text or the existing audit-entry test id) so the
AuditLogPanel conditional at lines 107-111 is covered.
| /** Redirect to GitHub OAuth — on return the token lands via URL param. */ | ||
| const handleGitHubLogin = () => { | ||
| // Store a flag so AdminPage can capture the token on redirect-back | ||
| sessionStorage.setItem('sf_admin_oauth_pending', '1'); | ||
| window.location.href = `${API_BASE}/api/auth/github/authorize?state=admin_login`; | ||
| }; |
There was a problem hiding this comment.
OAuth state parameter should be random to prevent CSRF attacks.
The state=admin_login parameter is static, making the OAuth flow vulnerable to CSRF attacks. An attacker could initiate an OAuth flow and trick a user into completing it, potentially hijacking the session.
Generate a random state value, store it in sessionStorage, and verify it on callback:
🔒 Proposed fix
const handleGitHubLogin = () => {
+ const state = crypto.randomUUID();
+ sessionStorage.setItem('sf_admin_oauth_state', state);
sessionStorage.setItem('sf_admin_oauth_pending', '1');
- window.location.href = `${API_BASE}/api/auth/github/authorize?state=admin_login`;
+ window.location.href = `${API_BASE}/api/auth/github/authorize?state=${state}`;
};Then verify on callback that the returned state matches the stored value.
📝 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.
| /** Redirect to GitHub OAuth — on return the token lands via URL param. */ | |
| const handleGitHubLogin = () => { | |
| // Store a flag so AdminPage can capture the token on redirect-back | |
| sessionStorage.setItem('sf_admin_oauth_pending', '1'); | |
| window.location.href = `${API_BASE}/api/auth/github/authorize?state=admin_login`; | |
| }; | |
| /** Redirect to GitHub OAuth — on return the token lands via URL param. */ | |
| const handleGitHubLogin = () => { | |
| // Store a flag so AdminPage can capture the token on redirect-back | |
| const state = crypto.randomUUID(); | |
| sessionStorage.setItem('sf_admin_oauth_state', state); | |
| sessionStorage.setItem('sf_admin_oauth_pending', '1'); | |
| window.location.href = `${API_BASE}/api/auth/github/authorize?state=${state}`; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/admin/AdminLayout.tsx` around lines 49 - 54, Replace
the static OAuth state with a cryptographically random value in
handleGitHubLogin: generate a secure random string (e.g., from
crypto.getRandomValues or a secure helper), store it in sessionStorage under a
new key (e.g., 'sf_admin_oauth_state') alongside the existing
'sf_admin_oauth_pending' flag, and include that random value as the state query
param in the redirect URL instead of 'admin_login'; then update the AdminPage
callback handler to read the returned state param and verify it matches the
stored 'sf_admin_oauth_state' before accepting the token and clearing the
sessionStorage keys.
| export function AdminLayout({ active, onNavigate, children }: Props) { | ||
| const [authed, setAuthed] = useState(() => Boolean(getAdminToken())); | ||
| const queryClient = useQueryClient(); | ||
|
|
||
| // Invalidate relevant queries on real-time WS events | ||
| const handleWsEvent = useCallback( | ||
| (event: AdminWsEvent) => { | ||
| const keys = WS_EVENT_INVALIDATIONS[event.type]; | ||
| if (keys) { | ||
| keys.forEach(key => queryClient.invalidateQueries({ queryKey: key })); | ||
| } else { | ||
| // Unknown event: refresh overview + audit log conservatively | ||
| queryClient.invalidateQueries({ queryKey: ['admin', 'overview'] }); | ||
| queryClient.invalidateQueries({ queryKey: ['admin', 'audit-log'] }); | ||
| } | ||
| }, | ||
| [queryClient], | ||
| ); | ||
|
|
||
| const { status: wsStatus } = useAdminWebSocket(handleWsEvent); | ||
|
|
||
| if (!authed) { | ||
| return <AdminLoginForm onSuccess={() => setAuthed(true)} />; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
WebSocket hook is invoked before auth check.
useAdminWebSocket(handleWsEvent) is called at line 176, but the auth check (if (!authed)) is at line 178. This means the WebSocket connection attempt happens even for unauthenticated users, where getAdminToken() returns empty string, causing the hook to set status: 'error'.
This is harmless but wasteful. Consider moving the hook call inside the authenticated branch or making it conditional:
♻️ Optional refactor
- const { status: wsStatus } = useAdminWebSocket(handleWsEvent);
+ const { status: wsStatus } = useAdminWebSocket(authed ? handleWsEvent : undefined);Or conditionally enable the hook inside useAdminWebSocket based on token presence (which it already does, but still creates the hook state).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/admin/AdminLayout.tsx` around lines 157 - 180, The
WebSocket hook is currently invoked before the auth check causing unnecessary
connection attempts; update AdminLayout so useAdminWebSocket(handleWsEvent) is
only called when the user is authenticated (i.e., after/authed is true) — either
move the call below the if (!authed) return AdminLoginForm branch or gate it
with authed (call useAdminWebSocket only when authed) and keep handleWsEvent as
the callback; ensure any dependent variables (previously const { status:
wsStatus }) are derived from the conditional hook result or replaced with a safe
default when not authed.
| function relativeTime(iso: string) { | ||
| const diff = Date.now() - new Date(iso).getTime(); | ||
| const s = Math.floor(diff / 1000); | ||
| if (s < 60) return `${s}s ago`; | ||
| const m = Math.floor(s / 60); | ||
| if (m < 60) return `${m}m ago`; | ||
| const h = Math.floor(m / 60); | ||
| if (h < 24) return `${h}h ago`; | ||
| return new Date(iso).toLocaleDateString(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Edge case: relativeTime may produce confusing output for future timestamps.
If entry.timestamp is in the future (e.g., due to clock skew or server/client timezone mismatch), diff will be negative, producing outputs like -5s ago. Consider guarding against this:
🛡️ Suggested defensive fix
function relativeTime(iso: string) {
const diff = Date.now() - new Date(iso).getTime();
+ if (diff < 0) return 'just now';
const s = Math.floor(diff / 1000);📝 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.
| function relativeTime(iso: string) { | |
| const diff = Date.now() - new Date(iso).getTime(); | |
| const s = Math.floor(diff / 1000); | |
| if (s < 60) return `${s}s ago`; | |
| const m = Math.floor(s / 60); | |
| if (m < 60) return `${m}m ago`; | |
| const h = Math.floor(m / 60); | |
| if (h < 24) return `${h}h ago`; | |
| return new Date(iso).toLocaleDateString(); | |
| } | |
| function relativeTime(iso: string) { | |
| const diff = Date.now() - new Date(iso).getTime(); | |
| if (diff < 0) return 'just now'; | |
| const s = Math.floor(diff / 1000); | |
| if (s < 60) return `${s}s ago`; | |
| const m = Math.floor(s / 60); | |
| if (m < 60) return `${m}m ago`; | |
| const h = Math.floor(m / 60); | |
| if (h < 24) return `${h}h ago`; | |
| return new Date(iso).toLocaleDateString(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/admin/AuditLogPanel.tsx` around lines 16 - 25, The
relativeTime function can return negative values for future timestamps (e.g.,
"-5s ago"); update relativeTime(iso: string) to detect when Date.now() - new
Date(iso).getTime() is negative and handle that case (e.g., treat negative diffs
as zero or return a "just now" / "in the future" string) so the UI never
displays negative durations; modify the logic inside relativeTime to clamp the
diff to >=0 (or branch to a friendly future message) and ensure callers like
where entry.timestamp is passed continue to work.
| const handleClose = async () => { | ||
| await close.mutateAsync(bounty.id); | ||
| onClose(); | ||
| }; |
There was a problem hiding this comment.
handleClose (Force Close) doesn't handle mutation errors.
If close.mutateAsync(bounty.id) fails, the error is thrown but onClose() is still called due to the await completing before the error propagates. Actually, reviewing again: if mutateAsync throws, the subsequent onClose() won't execute. However, there's no user feedback if the close fails - the modal stays open but no error is shown.
Consider showing the close mutation error:
🐛 Proposed fix to show close errors
- {update.isError && (
- <p className="text-xs text-red-400">{(update.error as Error).message}</p>
+ {(update.isError || close.isError) && (
+ <p className="text-xs text-red-400">
+ {((update.error ?? close.error) as Error).message}
+ </p>
)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/admin/BountyManagement.tsx` around lines 41 - 44,
handleClose currently calls close.mutateAsync(bounty.id) but doesn't surface
failures to the user; update the handleClose function to catch errors from
close.mutateAsync and present them (e.g., set a local error state or call the
existing UI toast/error handler) instead of silently failing, and only call
onClose() after a successful mutation; reference the handleClose function and
the close.mutateAsync(...) call to locate where to add a try/catch and
error-display logic.
| export function FinancialPanel() { | ||
| const [page, setPage] = useState(1); | ||
| const { data: overview, isLoading: ovLoading } = useFinancialOverview(); | ||
| const { data: payouts, isLoading: payLoading } = usePayoutHistory(page, 20); | ||
|
|
||
| const totalPages = payouts ? Math.ceil(payouts.total / 20) : 1; |
There was a problem hiding this comment.
Missing error state handling for data fetches.
Both useFinancialOverview and usePayoutHistory can return errors, but the component doesn't render error states. If either fetch fails, users see no feedback.
🐛 Proposed fix to add error handling
export function FinancialPanel() {
const [page, setPage] = useState(1);
- const { data: overview, isLoading: ovLoading } = useFinancialOverview();
- const { data: payouts, isLoading: payLoading } = usePayoutHistory(page, 20);
+ const { data: overview, isLoading: ovLoading, error: ovError } = useFinancialOverview();
+ const { data: payouts, isLoading: payLoading, error: payError } = usePayoutHistory(page, 20);Then add error rendering after the loading states:
+ {ovError && <p className="text-sm text-red-400">{(ovError as Error).message}</p>}
{overview && (+ {payError && <p className="text-sm text-red-400">{(payError as Error).message}</p>}
{payouts && payouts.items.length === 0 && (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/admin/FinancialPanel.tsx` around lines 11 - 16, The
FinancialPanel component is missing error handling for the two data hooks;
update FinancialPanel to check the error returns from useFinancialOverview and
usePayoutHistory (e.g., overviewError / payoutsError or isError fields from
those hooks) alongside ovLoading and payLoading, and render a clear error state
(a message or fallback UI) when either fetch fails instead of showing an empty
UI; reference the existing variables (overview, payouts, ovLoading, payLoading)
and add conditional rendering paths that show the error details and optionally a
retry action.
| <td className="px-4 py-3 font-medium truncate max-w-[200px]"> | ||
| <a href={`/bounties/${p.bounty_id}`} className="text-[#9945FF] hover:underline"> | ||
| {p.bounty_title} | ||
| </a> | ||
| </td> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using React Router Link for internal navigation.
Using a plain <a href> for internal routes (/bounties/${p.bounty_id}) causes a full page reload, losing React state and requiring re-authentication. Since react-router-dom is available, use Link for client-side navigation.
♻️ Proposed fix
+import { Link } from 'react-router-dom';
// ...
- <td className="px-4 py-3 font-medium truncate max-w-[200px]">
- <a href={`/bounties/${p.bounty_id}`} className="text-[`#9945FF`] hover:underline">
- {p.bounty_title}
- </a>
- </td>
+ <td className="px-4 py-3 font-medium truncate max-w-[200px]">
+ <Link to={`/bounties/${p.bounty_id}`} className="text-[`#9945FF`] hover:underline">
+ {p.bounty_title}
+ </Link>
+ </td>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/admin/FinancialPanel.tsx` around lines 83 - 87, In
FinancialPanel, replace the plain anchor used for internal navigation (the <a
href={`/bounties/${p.bounty_id}`}> wrapping {p.bounty_title}) with
react-router-dom's Link to avoid full page reloads; import { Link } from
'react-router-dom', keep the same className and text content, and use Link's
to={`/bounties/${p.bounty_id}`} so client-side routing preserves React state and
avoids re-authentication.
| <td className="px-4 py-3 font-medium truncate max-w-[180px]"> | ||
| <a href={`/bounties/${r.bounty_id}`} className="text-[#9945FF] hover:underline"> | ||
| {r.bounty_title} | ||
| </a> | ||
| </td> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use React Router Link for internal bounty navigation.
Same issue as FinancialPanel.tsx: using plain <a href> for internal routes causes full page reloads.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/admin/ReviewPipeline.tsx` around lines 81 - 85,
Replace the plain anchor in ReviewPipeline.tsx that links to
`/bounties/${r.bounty_id}` with React Router's Link to avoid full page reloads:
import { Link } from 'react-router-dom' if not present, and change the <a
href={`/bounties/${r.bounty_id}`}> usage to a <Link
to={`/bounties/${r.bounty_id}`}> while preserving the existing className
("text-[`#9945FF`] hover:underline") and children (`{r.bounty_title}`) so
client-side routing is used instead of a full navigation.
| ws.onmessage = (evt) => { | ||
| try { | ||
| const raw = JSON.parse(evt.data as string); | ||
| const event: AdminWsEvent = { | ||
| type: raw.type ?? raw.action ?? 'unknown', | ||
| payload: raw.payload ?? raw, | ||
| timestamp: raw.timestamp ?? new Date().toISOString(), | ||
| }; | ||
| setLastEvent(event); | ||
| onEvent?.(event); | ||
| } catch { | ||
| // Non-JSON frames (e.g. heartbeat pings) are silently ignored | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
WebSocket message data type assumption.
evt.data is typed as string | ArrayBuffer | Blob. The current code assumes it's always a string. While server-sent JSON messages are typically strings, consider adding a guard:
🛡️ Defensive type check
ws.onmessage = (evt) => {
+ if (typeof evt.data !== 'string') return;
try {
- const raw = JSON.parse(evt.data as string);
+ const raw = JSON.parse(evt.data);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/hooks/useAdminWebSocket.ts` around lines 51 - 64, ws.onmessage
currently assumes evt.data is a string which can throw or ignore valid JSON in
Blob/ArrayBuffer frames; update the handler (ws.onmessage) to first guard on
typeof evt.data and convert ArrayBuffer/Blob to a UTF-8 string (e.g., use
TextDecoder for ArrayBuffer and blob.text() or FileReader for Blob) before
JSON.parse, then build the AdminWsEvent as before and call setLastEvent and
onEvent; keep the existing try/catch to silently ignore non-JSON frames but
ensure the conversion is awaited/handled so parsing succeeds for binary frames.
| }, [onEvent]); | ||
|
|
||
| useEffect(() => { | ||
| shouldReconnect.current = true; | ||
| connect(); | ||
|
|
||
| return () => { | ||
| shouldReconnect.current = false; | ||
| if (reconnectTimer.current) clearTimeout(reconnectTimer.current); | ||
| wsRef.current?.close(); | ||
| }; | ||
| // connect is stable (useCallback with no deps that change), so this is safe | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Potential stale closure for onEvent callback.
The connect function depends on onEvent (line 83), but the useEffect disables the exhaustive-deps rule and uses an empty dependency array. If onEvent changes (e.g., due to parent re-render without useCallback), the WebSocket will continue using the original callback.
Since AdminLayout wraps handleWsEvent in useCallback with stable dependencies, this works in practice. However, consider either:
- Documenting that callers must memoize
onEvent, or - Using a ref to always call the latest callback:
♻️ Alternative pattern using ref
+const onEventRef = useRef(onEvent);
+useEffect(() => { onEventRef.current = onEvent; }, [onEvent]);
// In ws.onmessage:
- onEvent?.(event);
+ onEventRef.current?.(event);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/hooks/useAdminWebSocket.ts` around lines 83 - 96, The effect
currently calls connect() while disabling exhaustive-deps, which risks a stale
onEvent capture; update the hook to ensure the latest onEvent is used by storing
onEvent in a ref (e.g., create onEventRef.current = onEvent in a small useEffect
that runs when onEvent changes) and have connect and any websocket event
handlers call onEventRef.current(...) instead of the closed-over onEvent; keep
the outer useEffect (that toggles shouldReconnect, starts connect, clears
reconnectTimer, and closes wsRef) with an empty deps array while removing the
eslint override, or alternatively document in the hook API that callers must
memoize onEvent (but prefer the ref approach for safety).
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/main.py (1)
401-412:⚠️ Potential issue | 🟠 MajorUnprotected admin endpoint:
/api/synclacks authentication.This endpoint is tagged as
["admin"]and the docstring explicitly acknowledges it "should be protected by admin authentication in production" (line 405), yet no authentication dependency is applied. The codebase establishes a clear pattern for admin endpoints: useactor: str = Depends(require_admin)as a parameter (e.g.,create_bounty_adminat line 468 inbackend/app/api/admin.py).Risks:
- Unauthenticated users can trigger expensive GitHub sync operations
- Repeated calls could exhaust GitHub API rate limits
- Potential denial-of-service vector
Apply
require_admindependency to protect this endpoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/main.py` around lines 401 - 412, The trigger_sync endpoint is unprotected; update its signature to require the admin dependency by adding a parameter like actor: str = Depends(require_admin) to the async def trigger_sync(...) so FastAPI enforces admin auth before calling sync_all; also import require_admin (from wherever require_admin is defined) at the top of the file if it's not already imported and preserve calling await sync_all() inside the function body.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/main.py`:
- Line 53: Remove the unused import "engine" from the imports in main.py; keep
the existing imports for init_db and close_db unchanged since close_db handles
engine internally, so update the import line to only import init_db and close_db
(i.e., remove "engine") to eliminate the unused symbol.
In `@backend/app/models/tables.py`:
- Around line 137-153: Update the AdminAuditLogTable model to match the
migration: add index=True to the actor Column; change details to use
postgresql.JSONB (not sa.JSON) and set a database-side default (server_default)
to an empty JSONB object; change id to use a database-side default
(server_default=sa.text("gen_random_uuid()")) instead of a Python uuid.uuid4
default; keep created_at as-is but consider adding a composite index on (event,
created_at) or (created_at, event) to speed common time-range/event queries.
Ensure you import postgresql and sa.text where needed and mirror the exact
server_default expressions used in the migration so the ORM matches the deployed
schema.
In `@frontend/src/App.tsx`:
- Around line 149-160: The Admin routing uses an invalid splat and an absolute
child path: change the parent route that currently uses path="/admin*" to use
path="/admin/*" and update the nested Route inside the AdminRoutes component
(the Route that currently has element={<AdminPage />}) to use a relative child
path (e.g., path="" or path="/") instead of the absolute "/admin"; ensure the
adjustments are applied to the AdminRoutes function and the parent route
declaration so nested routing works correctly.
---
Outside diff comments:
In `@backend/app/main.py`:
- Around line 401-412: The trigger_sync endpoint is unprotected; update its
signature to require the admin dependency by adding a parameter like actor: str
= Depends(require_admin) to the async def trigger_sync(...) so FastAPI enforces
admin auth before calling sync_all; also import require_admin (from wherever
require_admin is defined) at the top of the file if it's not already imported
and preserve calling await sync_all() inside the function body.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 730d1d6f-36dc-4d88-a392-51c524c590e0
📒 Files selected for processing (3)
backend/app/main.pybackend/app/models/tables.pyfrontend/src/App.tsx
| from app.api.stats import router as stats_router | ||
| from app.api.escrow import router as escrow_router | ||
| from app.api.admin import router as admin_router | ||
| from app.database import init_db, close_db, engine |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused import: engine.
The engine object is imported but not used anywhere in this file. The close_db() function in database.py accesses engine internally; it doesn't need to be passed or imported here.
♻️ Remove unused import
-from app.database import init_db, close_db, engine
+from app.database import init_db, close_db📝 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.
| from app.database import init_db, close_db, engine | |
| from app.database import init_db, close_db |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/main.py` at line 53, Remove the unused import "engine" from the
imports in main.py; keep the existing imports for init_db and close_db unchanged
since close_db handles engine internally, so update the import line to only
import init_db and close_db (i.e., remove "engine") to eliminate the unused
symbol.
| class AdminAuditLogTable(Base): | ||
| """Persistent audit log for all admin actions. | ||
|
|
||
| Replaces the in-memory deque so entries survive restarts and are | ||
| queryable with time-range and event-type filters. | ||
| """ | ||
|
|
||
| __tablename__ = "admin_audit_log" | ||
|
|
||
| id = Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) | ||
| event = Column(String(100), nullable=False, index=True) | ||
| actor = Column(String(200), nullable=False) | ||
| role = Column(String(20), nullable=False, server_default="admin") | ||
| details = Column(sa.JSON, nullable=False, default=dict) | ||
| created_at = Column( | ||
| DateTime(timezone=True), nullable=False, default=_now, index=True | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify migration 003_add_admin_audit_log.py defines the same columns/indexes
# Expected: Migration should match model definition; check if actor index exists
fd -t f '003_add_admin_audit_log.py' --exec cat {}Repository: SolFoundry/solfoundry
Length of output: 1491
Missing actor index and schema drift between model definition and migration requires alignment.
The actor column lacks an index in both the model and migration. This will cause full table scans for queries like "show all actions by admin X" as the audit log grows—a material performance issue.
More critically, the model definition at lines 137-153 diverges from the actual migration schema on three points:
- Column type: Model uses
sa.JSON; migration usespostgresql.JSONB. JSONB is PostgreSQL-native and more efficient. - Details default: Model uses
default=dict(Python-side); migration usesserver_default="{}"(database-side). Server-side defaults are preferred for data consistency. - ID generation: Model uses
default=uuid.uuid4(Python-side); migration usesserver_default=sa.text("gen_random_uuid()")(database-side).
The migration's schema is more correct (server-side defaults prevent application-layer inconsistencies), but the ORM model does not reflect it. Update the model to match the deployed migration schema, and add index=True to actor.
A composite index on (created_at, event) or (event, created_at) would also benefit common time-range filtered queries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/models/tables.py` around lines 137 - 153, Update the
AdminAuditLogTable model to match the migration: add index=True to the actor
Column; change details to use postgresql.JSONB (not sa.JSON) and set a
database-side default (server_default) to an empty JSONB object; change id to
use a database-side default (server_default=sa.text("gen_random_uuid()"))
instead of a Python uuid.uuid4 default; keep created_at as-is but consider
adding a composite index on (event, created_at) or (created_at, event) to speed
common time-range/event queries. Ensure you import postgresql and sa.text where
needed and mirror the exact server_default expressions used in the migration so
the ORM matches the deployed schema.
| // ── Admin layout (bypasses SiteLayout — has its own shell) ─────────────────── | ||
| function AdminRoutes() { | ||
| return ( | ||
| <ErrorBoundary> | ||
| <Suspense fallback={<LoadingSpinner />}> | ||
| <Routes> | ||
| <Route path="/admin" element={<AdminPage />} /> | ||
| </Routes> | ||
| </Suspense> | ||
| </ErrorBoundary> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
react-router-dom v6 wildcard route syntax
💡 Result:
In react-router-dom v6, “wildcard” routes use the * splat matcher:
-
Catch-all (404) route:
<Route path="*" element={<NoMatch />} />
-
Match a prefix + anything after it (splat at the end):
<Route path="/files/*" element={<Files />} />
This matches
/files,/files/a,/files/a/b, etc. (reactrouter.com) -
Wildcard rules in v6:
*is allowed only at the end of the path (not in the middle).- v6 path placeholders are basically just
:paramand*. (reactrouter.com)
-
Read the splat value:
params["*"] // e.g. "one/two"
(You can destructure it as
{"*": splat}.) (reactrouter.com)
Citations:
- 1: https://reactrouter.com/6.30.3/start/faq?utm_source=openai
- 2: https://reactrouter.com/docs/en/v6/route/route?utm_source=openai
- 3: https://reactrouter.com/6.30.3/upgrading/v5?utm_source=openai
- 4: https://reactrouter.com/docs/en/v6/route/route?utm_source=openai
🏁 Script executed:
cd frontend && head -180 src/App.tsx | tail -50Repository: SolFoundry/solfoundry
Length of output: 1718
Fix wildcard route syntax and nested route path in admin routing configuration.
The admin routing has two critical issues:
-
Line 172:
path="/admin*"uses invalid syntax. In react-router-dom v6, splat routes require a slash before the asterisk:path="/admin/*". -
Lines 155 + 172 together: AdminRoutes contains a nested
<Routes>component with an absolute path/admin. When routes are nested under a parent splat route, child routes must use relative paths. Change the child route topath=""orpath="/".
The current configuration will cause routing failures for the admin section.
Proposed fix
- <Route path="/admin*" element={<AdminRoutes />} />
+ <Route path="/admin/*" element={<AdminRoutes />} /> <Route path="/admin" element={<AdminPage />} />
+ <Route path="" element={<AdminPage />} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/App.tsx` around lines 149 - 160, The Admin routing uses an
invalid splat and an absolute child path: change the parent route that currently
uses path="/admin*" to use path="/admin/*" and update the nested Route inside
the AdminRoutes component (the Route that currently has element={<AdminPage />})
to use a relative child path (e.g., path="" or path="/") instead of the absolute
"/admin"; ensure the adjustments are applied to the AdminRoutes function and the
parent route declaration so nested routing works correctly.
1 participant
Description
Implements the SolFoundry admin dashboard — a full-stack management interface
for bounty operations, contributor management, review pipeline monitoring,
financial reporting, and system health observability.
The backend adds a dedicated
/api/admin/*router protected by anADMIN_API_KEYBearer token. The frontend adds a standalone/adminroutewith its own layout shell (sidebar + auth gate) that bypasses SiteLayout.
All data is fetched via React Query with auto-refresh intervals; real-time
updates use the existing
/wsWebSocket endpoint.Closes #599
Solana Wallet for Payout
Wallet: 4QhseKvBuaCQhdkP248iXoUxohPzVC5m8pE9hAv4nMYw
Type of Change
Checklist
console.logor debugging code left behindTesting
Screenshots (if applicable)
Additional Notes
New files
backend/app/api/admin.py/api/admin/*backend/tests/test_admin.pyfrontend/src/types/admin.tsfrontend/src/hooks/useAdminData.tsadminFetchhelper + token managementfrontend/src/hooks/useAdminWebSocket.tsfrontend/src/components/admin/AdminLayout.tsxfrontend/src/components/admin/OverviewPanel.tsxfrontend/src/components/admin/BountyManagement.tsxfrontend/src/components/admin/ContributorManagement.tsxfrontend/src/components/admin/ReviewPipeline.tsxfrontend/src/components/admin/FinancialPanel.tsxfrontend/src/components/admin/SystemHealth.tsxfrontend/src/components/admin/AuditLogPanel.tsxfrontend/src/pages/AdminPage.tsx?section=URL paramfrontend/src/__tests__/AdminDashboard.test.tsxModified files
backend/app/main.pyadmin_routerfrontend/src/App.tsx/admin*route (bypasses SiteLayout)Architecture decisions
ADMIN_API_KEYenv var checked against Bearer token — no DB schema change needed; stateless and easy to rotatedeque(maxlen=1000)ring buffer for instant API response without a DB round-trip; structlog stream ensures persistence via log aggregators?section=overviewparam makes each panel deep-linkable and shareable/wsWebSocket; React QueryrefetchIntervalprovides a polling fallback when WS is disconnected