Implement full stack admin dashboard#634
Closed
codebestia wants to merge 1 commit intoSolFoundry:mainfrom
Closed
Conversation
📝 WalkthroughWalkthroughThis PR introduces a complete full-stack admin dashboard for SolFoundry. The backend adds a new FastAPI admin router with 12+ endpoints supporting bounty management, contributor moderation, review pipeline metrics, financial summaries, system health monitoring, and audit logging, all secured via Bearer token authentication against an ADMIN_API_KEY. The frontend provides a responsive React admin interface with components for bounties, contributors, reviews, financials, and system health, integrated with WebSocket for real-time updates, along with React Query hooks for data fetching and mutations. Comprehensive test coverage is added for both backend endpoints and frontend component behavior. The implementation aggregates data from in-memory stores, database connections (SQLAlchemy, Redis), and existing service components to provide a unified control center for platform operations. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 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: 23
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)
379-391:⚠️ Potential issue | 🟠 MajorSecurity gap:
/api/syncendpoint lacks authentication.This endpoint is tagged as
["admin"]but has no authentication dependency. Any unauthenticated client can trigger a full GitHub sync, which:
- Makes arbitrary GitHub API calls (sync_bounties, sync_contributors) leading to rate limiting
- Can be used as a denial-of-service vector
- Exposes internal sync behavior to attackers
Unlike the properly protected admin router which uses
require_admindependency for Bearer token validation, this endpoint has no guards. The docstring even acknowledges it "should be protected by admin authentication in production."Add
Depends(require_admin)to the function signature or move this endpoint into the admin router to match the authentication pattern used by other admin endpoints.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/main.py` around lines 379 - 391, The /api/sync endpoint (trigger_sync) is missing admin authentication: update the endpoint to require the same admin dependency used elsewhere (require_admin) by adding a Depends(require_admin) parameter to the trigger_sync function signature or move trigger_sync into the existing admin router so it inherits admin protection; ensure the handler still awaits sync_all() and returns its result after adding the dependency to prevent unauthenticated callers from invoking sync_bounties/sync_contributors.
🤖 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/api/admin.py`:
- Around line 168-173: BountyAdminUpdate.status must be validated against the
BountyStatus enum to avoid persisting arbitrary strings; update the model and/or
handler so status is an Optional[BountyStatus> (or validate incoming str by
calling BountyStatus(value) and catching ValueError) before assigning to
bounty.status in the PATCH handler (the place currently doing the direct
assignment at the lines referenced). If validation fails return a 400/validation
error, and when storing use the enum value consistently (or store the enum
instance) so the rest of the file that expects BountyStatus enums (e.g., the
overview/financial/payout computations) sees only valid states.
- Around line 32-74: The current admin auth uses a single process-wide bearer
(ADMIN_API_KEY) and collapses every caller to the same actor ID via
require_admin, which prevents GitHub OAuth + role-based access and prevents
secret rotation; replace this by implementing an auth dependency that validates
per-request credentials against the proper source (GitHub OAuth token
verification or a rotatable secret store) and maps them to distinct actor IDs
and roles (admin, reviewer, viewer) instead of returning the constant "admin";
update the dependency currently named require_admin and the HTTPBearer _security
usage to return a structure containing actor_id and role, ensure any places
writing to _audit_log (the audit buffer used around lines 82-92) record the
actor_id and role, and stop reading ADMIN_API_KEY only at import time (either
read env/secret per request or use a secret-management client) so rotation takes
effect without process restart.
- Around line 352-405: The admin endpoints update_bounty_admin and
close_bounty_admin currently allow editing or cancelling bounties regardless of
lifecycle state; change both to reject modifications when the bounty is in
terminal financial states (e.g., BountyStatus.COMPLETED, BountyStatus.PAID, and
other terminal statuses you treat as immutable) by checking bounty.status at the
start of update_bounty_admin and close_bounty_admin and raising
HTTPException(400) (or appropriate 409) if the status is terminal; for
update_bounty_admin do not apply status/reward/title changes when the check
fails, and for close_bounty_admin only allow forcing CANCELLED for non-terminal
bounties, ensuring _log is only called on successful state changes so historical
financial records remain immutable.
- Around line 279-281: The code treats BountyStatus.COMPLETED inconsistently
(both as paid/distributed and as pending); fix by making COMPLETED consistently
considered a paid/distributed status across all aggregates: update the filters
used by total_fndry (variable total_fndry), total_fndry_distributed,
total_paid_bounties to include BountyStatus.COMPLETED, and remove
BountyStatus.COMPLETED from any filters used for pending_payout_count,
pending_payout_amount and the payout history generation so those only include
truly pending statuses (e.g., only PENDING/READY_FOR_PAYOUT). Ensure the same
status set is used for the overview “paid” total and the detailed
distributed/paid aggregates so all endpoints agree on what “paid/completed”
means.
- Around line 650-680: The health endpoint currently only uses db_status to set
the top-level status and reports webhook_events_processed as len(_audit_log),
which is misleading; update the logic around the redis probe
(redis_from_url/client.ping) and the overall SystemHealthResponse construction
so that any core dependency failure (database, redis, webhook/bot) sets status
to "degraded" (not just db_status), add explicit probes for webhook/bot (e.g.,
call the webhook health check or bot ping) and include their results in the
services dict alongside "database" and "redis", and replace the capped
ring-buffer metric _audit_log with a true cumulative counter or metric (e.g.,
use or introduce a _webhook_events_count or similar persistent counter) so
webhook_events_processed reports total processed events rather than
len(_audit_log); ensure you update the SystemHealthResponse usage
(SystemHealthResponse(..., services={...}, webhook_events_processed=...)) and
reference redis_from_url, client.ping, _audit_log, SystemHealthResponse, and
_bounty_store when making these changes.
- Around line 94-106: _bounty_to_dict (and related payout-serialization code) is
fabricating payout winner and completion timestamps by falling back to
bounty.created_by and bounty.created_at; instead, use the actual payout/winner
fields (e.g., b.payout_recipient, b.winner, or b.payouts[x].recipient) and the
payout completion timestamp (e.g., b.payout_completed_at or
b.payouts[x].completed_at) and return null/None when those fields are absent —
do not default to created_by/created_at. Update _bounty_to_dict and any
payout-history builders referenced in the review to read from the proper
winner/payout fields and set winner/completion_time to None when missing,
ensuring API contract consistency.
- Around line 352-355: The patch/close/ban/unban bounty endpoints return
inconsistent types (some return boolean True, others the string "true") and lack
a documented response_model; create or reuse a Pydantic response model (e.g.,
schemas.OkResponse with ok: bool), add response_model=schemas.OkResponse to the
route decorators for the handlers that include update_bounty/patch (the function
handling PATCH /bounties/{bounty_id}), close_bounty, ban_bounty, and
unban_bounty, and change their return values to consistently return {"ok": True}
(a boolean) instead of string values so FastAPI enforces and documents a stable
boolean contract.
- Around line 35-36: The current audit trail is only an in-memory deque
(_audit_log) and misses many admin GET handlers and persistence; replace this
volatile implementation by writing every audit event (calls to _log() and new
calls from GET handlers) to a durable, shared store (e.g., the existing DB) and
use the deque only as an optional in-process cache. Modify
backend/app/core/audit.py to persist events (insert a normalized audit record
with timestamp, actor, action, target, metadata, and request id) and continue to
emit logger.info as a fallback; update backend/app/api/admin.py to call the
unified audit API (instead of only pushing to _audit_log) from all admin
endpoints (including the GET handlers noted) and remove or repurpose the deque
to read recent entries from the persistent store. Ensure writes are
idempotent/transactional, surface write errors to processLogger with context,
add pagination/filtering to /api/admin/audit-log to match the API contract, and
add tests for cross-worker visibility, restart persistence, and error handling.
In `@frontend/src/__tests__/AdminDashboard.test.tsx`:
- Around line 248-306: Add a test that verifies the ban confirmation flow: mock
adminData.useAdminContributors to return a non-banned contributor and mock
adminData.useBanContributor to return a noop/mocked mutation, render
<ContributorManagement />, click the ban button (getByTestId 'ban-c1'), fill the
ban reason input (test id 'ban-reason' or the component's reason input), click
the confirm button in the modal (test id 'ban-confirm'), and assert the mocked
useBanContributor.mutate or mutateAsync was called with the contributor id and
the entered reason; reference ContributorManagement,
adminData.useBanContributor, 'ban-c1', 'ban-modal', 'ban-reason', and
'ban-confirm' to locate elements and hooks.
In `@frontend/src/App.tsx`:
- Around line 170-175: Update the route patterns to be v6-compatible: change the
parent Route path from "/admin*" to "/admin/*" in the Routes/Route JSX, and
inside the AdminRoutes component convert any nested absolute paths like "/admin"
to relative paths (remove the leading slash or use index/relative segments) so
the nested routes match under the parent "/admin/*" route; ensure AdminRoutes
uses relative Route paths rather than absolute ones.
In `@frontend/src/components/admin/AdminLayout.tsx`:
- Around line 40-45: The handleSubmit currently only checks key.trim() and
blindly calls setAdminToken(key) and onSuccess; change it to perform an async
verification call to the server before saving the token: make handleSubmit
async, POST the trimmed key to a verification endpoint (e.g. /api/admin/verify)
using fetch/axios, check for a 2xx response (or specific success flag) and only
then call setAdminToken(key.trim()) and onSuccess(); on non-2xx or errors call
setError with a clear message (e.g. "Invalid API key" or include server error
text) and avoid storing the token. Keep try/catch to handle network errors and
reference handleSubmit, setAdminToken, onSuccess, key, and setError to locate
the changes.
In `@frontend/src/components/admin/AuditLogPanel.tsx`:
- Around line 16-25: relativeTime currently only computes once per render so
timestamps don't tick; update AuditLogPanel to trigger periodic re-renders
(e.g., every 5–15s) so relativeTime(iso) is recalculated: add a small piece of
state (e.g., tick) and a useEffect in the AuditLogPanel component that sets up
setInterval to update that state on the chosen interval and clears the interval
on cleanup, then reference that tick state in the render so relativeTime is
re-run for each item; ensure interval is cleared in the effect cleanup to avoid
leaks.
- Around line 87-91: The list uses the array index as a React key in
data.entries.map (key={i}) inside AuditLogPanel.tsx which breaks rendering on
live updates; change the key to a stable unique identifier from each entry
(e.g., entry.id or entry.timestamp) in the mapped element and update the
data-testid from `audit-entry-${i}` to a stable variant (e.g.,
`audit-entry-${entry.id}`) so React can correctly reconcile items when new
entries are prepended or updated.
In `@frontend/src/components/admin/BountyManagement.tsx`:
- Around line 30-39: The handleSave function currently calls update.mutateAsync
without guarding for exceptions, so wrap the mutateAsync call in a try/catch:
call await update.mutateAsync({ id: bounty.id, update: patch }) inside try, then
call onClose() only on success; in catch just swallow or rethrow (UI error state
is already surfaced via update.isError) so the modal stays open on failure and
closes only on success. Ensure you reference handleSave, update.mutateAsync,
onClose, and update.isError when making the change.
- Around line 84-92: The reward input allows values below 1 because handleSave
only checks isNaN; update the handleSave logic to parse Number(reward) into
rewardNum and only include patch.reward_amount when rewardNum is a valid number
and rewardNum >= 1 and rewardNum !== bounty.reward_amount (or otherwise
set/raise a validation error); locate this change in the handleSave function and
also ensure the input state updater setReward still accepts strings but
validation enforces the minimum before submitting.
- Around line 41-44: The handleClose function currently calls
close.mutateAsync(bounty.id) immediately with no user confirmation and no error
handling; change handleClose to first prompt the user (e.g., window.confirm or
the existing confirmation Modal component) to confirm the destructive close
action, and wrap the mutation call in a try/catch so failures are caught; on
error, surface the error to the UI in the same way update.isError is rendered
(or set a local error state / show a toast) and only call onClose() after a
successful mutation, referencing handleClose, close.mutateAsync, close.isError
and onClose in your changes.
In `@frontend/src/components/admin/ContributorManagement.tsx`:
- Around line 198-206: The unban mutation uses a shared pending flag
(unban.isPending) which disables every Unban button; introduce local state to
track the specific contributor being unbanned (e.g., const [unbanning,
setUnbanning] = useState<string | null>(null)), set setUnbanning(c.id) before
calling unban.mutate or mutateAsync and clear it in the finally callback, and
change the button disabled prop from unban.isPending to (unbanning === c.id) so
only the clicked contributor's Unban button is disabled; update any places
referencing unban.isPending to use this per-id state and keep the existing
unban.mutate(c.id) call signature.
In `@frontend/src/components/admin/FinancialPanel.tsx`:
- Around line 78-80: The current JSX in payouts.items.map uses a composite key
`${p.bounty_id}-${i}` which still relies on the array index; update the row key
in the render produced by payouts.items.map to use a truly unique identifier
from each payout (e.g., p.id or p.payout_id) if available instead of the index,
or if bounty_id is guaranteed unique just use p.bounty_id; change the key on the
<tr> generated in the payouts.items.map callback accordingly (look for the map
callback and the key prop on the <tr>).
- Around line 83-87: The anchor tag in FinancialPanel.tsx uses a native <a href>
with {p.bounty_id} which causes a full page reload; replace it with
react-router-dom's Link (e.g., <Link to={`/bounties/${p.bounty_id}`}>), keep the
existing className and text content (p.bounty_title), and add an import for Link
from 'react-router-dom' near the top of the file so navigation is client-side
and SPA state is preserved.
In `@frontend/src/components/admin/SystemHealth.tsx`:
- Around line 4-17: ServiceBadge currently treats only 'connected' and 'healthy'
as OK (constant ok in ServiceBadge), leading intermediate states like
'degraded', 'warning', or 'initializing' to appear as unhealthy; update
ServiceBadge to map status to a display variant instead of a binary check —
either make ok computed from a whitelist (e.g., const ok =
['connected','healthy','initializing','degraded'].includes(status)) or create a
status->variant mapping (e.g., 'healthy'/'connected' => green,
'warning'/'degraded' => yellow, 'error'/'disconnected' => red) and apply
corresponding badge colors and animation classes based on that variant so
intermediate/warning states are rendered appropriately.
In `@frontend/src/hooks/useAdminWebSocket.ts`:
- Around line 40-42: The admin token is currently embedded in the WebSocket URL
(created where setStatus('connecting') and wsRef.current is set) which exposes
it; change the client to open the WebSocket without the token query (use
`${WS_BASE}/ws`) and then authenticate by sending the token in an initial
message or via the optional subprotocols parameter: create the WebSocket, assign
wsRef.current, add ws.onopen handler that sends a JSON auth message (e.g. {type:
'auth', token}) or use protocols=[token] if backend supports it, and handle auth
success/failure before proceeding; update any code using the existing URL-based
token accordingly (references: setStatus, WS_BASE, wsRef, and the WebSocket
construction).
In `@frontend/src/pages/AdminPage.tsx`:
- Around line 18-24: The code unsafely type-asserts URL param to AdminSection in
AdminPage by doing const section = (searchParams.get('section') ??
DEFAULT_SECTION) as AdminSection; instead validate the raw value before casting:
read rawSection from searchParams, check it against a whitelist (e.g.,
VALID_SECTIONS Set of AdminSection values), and only assign section = rawSection
as AdminSection when it passes the guard, otherwise use DEFAULT_SECTION; update
navigate (which calls setSearchParams) unchanged but reference the validated
section variable throughout the component.
In `@frontend/src/types/admin.ts`:
- Around line 129-137: The frontend type SystemHealthResponse is stricter than
the backend; update the type so it matches the backend's flexible string status
by changing the status field from the union 'healthy' | 'degraded' to a plain
string, i.e., replace the status declaration in SystemHealthResponse with
status: string; alternatively, if you want to keep stricter values, introduce a
shared enum (e.g., SystemHealthStatus) that mirrors the backend and use that
enum for the status field in SystemHealthResponse so both sides stay in sync.
---
Outside diff comments:
In `@backend/app/main.py`:
- Around line 379-391: The /api/sync endpoint (trigger_sync) is missing admin
authentication: update the endpoint to require the same admin dependency used
elsewhere (require_admin) by adding a Depends(require_admin) parameter to the
trigger_sync function signature or move trigger_sync into the existing admin
router so it inherits admin protection; ensure the handler still awaits
sync_all() and returns its result after adding the dependency to prevent
unauthenticated callers from invoking sync_bounties/sync_contributors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dbbb92ae-ea77-4469-9465-e7204b6bc972
📒 Files selected for processing (17)
backend/app/api/admin.pybackend/app/main.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
Comment on lines
+32
to
+74
| ADMIN_API_KEY = os.getenv("ADMIN_API_KEY", "") | ||
| _security = HTTPBearer(auto_error=False) | ||
|
|
||
| # In-process audit ring buffer (last 1 000 admin actions) | ||
| _audit_log: deque[Dict[str, Any]] = deque(maxlen=1_000) | ||
|
|
||
| router = APIRouter(prefix="/api/admin", tags=["admin"]) | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Auth dependency | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| async def require_admin( | ||
| credentials: Optional[HTTPAuthorizationCredentials] = Depends(_security), | ||
| ) -> str: | ||
| """Verify the caller holds a valid admin API key. | ||
|
|
||
| Returns the string ``"admin"`` on success so callers can use it as an | ||
| actor ID in audit entries. | ||
|
|
||
| Raises: | ||
| HTTPException 401: No credentials supplied. | ||
| HTTPException 403: Credentials present but invalid. | ||
| """ | ||
| if not credentials: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_401_UNAUTHORIZED, | ||
| detail="Admin authentication required", | ||
| headers={"WWW-Authenticate": "Bearer"}, | ||
| ) | ||
| if not ADMIN_API_KEY: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_503_SERVICE_UNAVAILABLE, | ||
| detail="Admin authentication is not configured on this server", | ||
| ) | ||
| if credentials.credentials != ADMIN_API_KEY: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_403_FORBIDDEN, | ||
| detail="Invalid admin credentials", | ||
| ) | ||
| return "admin" |
There was a problem hiding this comment.
The auth model does not match the required admin-access design.
Lines 32-74 protect the entire admin surface with one process-wide bearer secret and collapse every authenticated caller into the same actor ID, "admin". That does not satisfy the linked objective’s GitHub OAuth + role-based access model (admin, reviewer, viewer), and it makes all audit entries generated through Lines 82-92 indistinguishable at the user level. Because the key is read once at Line 32, secret rotation also has no effect until the process is reloaded.
As per coding guidelines, backend changes must analyze thoroughly: Authentication/authorization gaps; API contract consistency with spec.
Also applies to: 82-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/admin.py` around lines 32 - 74, The current admin auth uses a
single process-wide bearer (ADMIN_API_KEY) and collapses every caller to the
same actor ID via require_admin, which prevents GitHub OAuth + role-based access
and prevents secret rotation; replace this by implementing an auth dependency
that validates per-request credentials against the proper source (GitHub OAuth
token verification or a rotatable secret store) and maps them to distinct actor
IDs and roles (admin, reviewer, viewer) instead of returning the constant
"admin"; update the dependency currently named require_admin and the HTTPBearer
_security usage to return a structure containing actor_id and role, ensure any
places writing to _audit_log (the audit buffer used around lines 82-92) record
the actor_id and role, and stop reading ADMIN_API_KEY only at import time
(either read env/secret per request or use a secret-management client) so
rotation takes effect without process restart.
Comment on lines
+35
to
+36
| # In-process audit ring buffer (last 1 000 admin actions) | ||
| _audit_log: deque[Dict[str, Any]] = deque(maxlen=1_000) |
There was a problem hiding this comment.
The audit trail is volatile, truncated, and incomplete.
The only backing store for /api/admin/audit-log is the in-process deque(maxlen=1_000) at Lines 35-36, so history is silently truncated after 1,000 actions, lost on restart, and split across workers/instances. Cross-file evidence: backend/app/core/audit.py lines ~73-80 only forwards audit events to logger.info(...), so there is no persistent source of truth behind this endpoint. In addition, only mutating handlers call _log(); the GET admin actions in Lines 269-349, 412-445, 500-548, 556-621, 629-682, and 690-720 are never recorded even though the objective calls for an admin audit log.
As per coding guidelines, backend changes must analyze thoroughly: Error handling and edge case coverage; API contract consistency with spec.
Also applies to: 82-92, 269-349, 412-445, 500-548, 556-621, 629-682, 690-720
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/admin.py` around lines 35 - 36, The current audit trail is
only an in-memory deque (_audit_log) and misses many admin GET handlers and
persistence; replace this volatile implementation by writing every audit event
(calls to _log() and new calls from GET handlers) to a durable, shared store
(e.g., the existing DB) and use the deque only as an optional in-process cache.
Modify backend/app/core/audit.py to persist events (insert a normalized audit
record with timestamp, actor, action, target, metadata, and request id) and
continue to emit logger.info as a fallback; update backend/app/api/admin.py to
call the unified audit API (instead of only pushing to _audit_log) from all
admin endpoints (including the GET handlers noted) and remove or repurpose the
deque to read recent entries from the persistent store. Ensure writes are
idempotent/transactional, surface write errors to processLogger with context,
add pagination/filtering to /api/admin/audit-log to match the API contract, and
add tests for cross-worker visibility, restart persistence, and error handling.
Comment on lines
+94
to
+106
| def _bounty_to_dict(b: Any) -> Dict[str, Any]: | ||
| """Serialise a BountyDB to a JSON-safe dict for admin responses.""" | ||
| return { | ||
| "id": b.id, | ||
| "title": b.title, | ||
| "status": b.status, | ||
| "tier": b.tier, | ||
| "reward_amount": b.reward_amount, | ||
| "created_by": b.created_by, | ||
| "deadline": b.deadline.isoformat() if hasattr(b.deadline, "isoformat") else str(b.deadline), | ||
| "submission_count": len(b.submissions) if b.submissions else 0, | ||
| "created_at": b.created_at.isoformat() if hasattr(b.created_at, "isoformat") else str(b.created_at), | ||
| } |
There was a problem hiding this comment.
Payout history fabricates winner and completion time from unrelated bounty fields.
At Line 610, winner falls back to created_by, but this module exposes created_by elsewhere as the bounty creator field at Lines 102 and 155, not the payout recipient. Lines 598-600 and 613-616 also order and populate payout completion using created_at, which is the bounty creation timestamp rather than payout/completion time. When the real winner or completion metadata is absent, this endpoint returns incorrect financial history instead of surfacing missing data.
As per coding guidelines, backend changes must analyze thoroughly: API contract consistency with spec.
Also applies to: 149-156, 598-616
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/admin.py` around lines 94 - 106, _bounty_to_dict (and related
payout-serialization code) is fabricating payout winner and completion
timestamps by falling back to bounty.created_by and bounty.created_at; instead,
use the actual payout/winner fields (e.g., b.payout_recipient, b.winner, or
b.payouts[x].recipient) and the payout completion timestamp (e.g.,
b.payout_completed_at or b.payouts[x].completed_at) and return null/None when
those fields are absent — do not default to created_by/created_at. Update
_bounty_to_dict and any payout-history builders referenced in the review to read
from the proper winner/payout fields and set winner/completion_time to None when
missing, ensuring API contract consistency.
Comment on lines
+168
to
+173
| class BountyAdminUpdate(BaseModel): | ||
| """Fields an admin can update on a bounty.""" | ||
|
|
||
| status: Optional[str] = Field(None, description="New lifecycle status") | ||
| reward_amount: Optional[float] = Field(None, gt=0, description="Adjusted reward") | ||
| title: Optional[str] = Field(None, min_length=3, max_length=200) |
There was a problem hiding this comment.
PATCH /bounties/{bounty_id} can write invalid lifecycle states into the store.
BountyAdminUpdate.status is declared as Optional[str] at Lines 171-173, and Lines 368-370 assign it directly to bounty.status without validating it against BountyStatus. A payload like "archived" or "paidd" will persist, and the rest of this file only recognizes enum-backed statuses when computing overview, financial, and payout results (for example, Lines 294-296, 564-566, and 594-597). That turns malformed admin input into silent state corruption instead of a rejected request.
As per coding guidelines, backend changes must analyze thoroughly: Input validation and SQL injection vectors; API contract consistency with spec.
Also applies to: 368-370, 294-296, 564-566, 594-597
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/admin.py` around lines 168 - 173, BountyAdminUpdate.status
must be validated against the BountyStatus enum to avoid persisting arbitrary
strings; update the model and/or handler so status is an Optional[BountyStatus>
(or validate incoming str by calling BountyStatus(value) and catching
ValueError) before assigning to bounty.status in the PATCH handler (the place
currently doing the direct assignment at the lines referenced). If validation
fails return a 400/validation error, and when storing use the enum value
consistently (or store the enum instance) so the rest of the file that expects
BountyStatus enums (e.g., the overview/financial/payout computations) sees only
valid states.
Comment on lines
+279
to
+281
| total_fndry = sum( | ||
| b.reward_amount for b in bounties if b.status in (BountyStatus.PAID, BountyStatus.COMPLETED) | ||
| ) |
There was a problem hiding this comment.
The financial endpoints contradict themselves on BountyStatus.COMPLETED.
Lines 564-575 count COMPLETED inside total_fndry_distributed and total_paid_bounties, while Lines 565-577 also count the same status inside pending_payout_count and pending_payout_amount, and Lines 594-621 include it in payout history. A COMPLETED bounty is therefore simultaneously treated as already distributed and still awaiting payout, so the admin dashboard cannot trust these aggregates. The same ambiguity also leaks into the overview’s “paid” total at Lines 279-281.
As per coding guidelines, backend changes must analyze thoroughly: API contract consistency with spec.
Also applies to: 564-577, 594-621
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/admin.py` around lines 279 - 281, The code treats
BountyStatus.COMPLETED inconsistently (both as paid/distributed and as pending);
fix by making COMPLETED consistently considered a paid/distributed status across
all aggregates: update the filters used by total_fndry (variable total_fndry),
total_fndry_distributed, total_paid_bounties to include BountyStatus.COMPLETED,
and remove BountyStatus.COMPLETED from any filters used for
pending_payout_count, pending_payout_amount and the payout history generation so
those only include truly pending statuses (e.g., only PENDING/READY_FOR_PAYOUT).
Ensure the same status set is used for the overview “paid” total and the
detailed distributed/paid aggregates so all endpoints agree on what
“paid/completed” means.
Comment on lines
+83
to
+87
| <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.
Using <a href> causes full page reload instead of client-side navigation.
Line 84 uses a native anchor tag with href which will trigger a full page reload when clicked. Since this is a React SPA with react-router-dom, use the Link component for client-side navigation:
+import { Link } from 'react-router-dom';
...
-<a href={`/bounties/${p.bounty_id}`} className="text-[`#9945FF`] hover:underline">
+<Link to={`/bounties/${p.bounty_id}`} className="text-[`#9945FF`] hover:underline">
{p.bounty_title}
-</a>
+</Link>This preserves SPA state and provides faster navigation.
🤖 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, The
anchor tag in FinancialPanel.tsx uses a native <a href> with {p.bounty_id} which
causes a full page reload; replace it with react-router-dom's Link (e.g., <Link
to={`/bounties/${p.bounty_id}`}>), keep the existing className and text content
(p.bounty_title), and add an import for Link from 'react-router-dom' near the
top of the file so navigation is client-side and SPA state is preserved.
Comment on lines
+4
to
+17
| function ServiceBadge({ name, status }: { name: string; status: string }) { | ||
| const ok = status === 'connected' || status === 'healthy'; | ||
| return ( | ||
| <div className="flex items-center justify-between rounded-xl border border-white/5 bg-white/[0.03] px-4 py-3"> | ||
| <span className="text-sm text-gray-300 capitalize">{name}</span> | ||
| <div className="flex items-center gap-2"> | ||
| <span className={`w-2 h-2 rounded-full ${ok ? 'bg-[#14F195]' : 'bg-red-500'} ${ok ? 'animate-pulse' : ''}`} /> | ||
| <span className={`text-xs font-medium ${ok ? 'text-[#14F195]' : 'text-red-400'}`}> | ||
| {status} | ||
| </span> | ||
| </div> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
ServiceBadge status check may not handle all service states.
Line 5 only treats 'connected' and 'healthy' as OK states. If the backend introduces intermediate states like 'degraded', 'warning', or 'initializing', they would incorrectly show as unhealthy (red).
Consider either:
- Checking against a list of "not OK" states instead:
const ok = status !== 'error' && status !== 'disconnected' - Or supporting a third "warning" state with yellow styling
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/admin/SystemHealth.tsx` around lines 4 - 17,
ServiceBadge currently treats only 'connected' and 'healthy' as OK (constant ok
in ServiceBadge), leading intermediate states like 'degraded', 'warning', or
'initializing' to appear as unhealthy; update ServiceBadge to map status to a
display variant instead of a binary check — either make ok computed from a
whitelist (e.g., const ok =
['connected','healthy','initializing','degraded'].includes(status)) or create a
status->variant mapping (e.g., 'healthy'/'connected' => green,
'warning'/'degraded' => yellow, 'error'/'disconnected' => red) and apply
corresponding badge colors and animation classes based on that variant so
intermediate/warning states are rendered appropriately.
Comment on lines
+40
to
+42
| setStatus('connecting'); | ||
| const ws = new WebSocket(`${WS_BASE}/ws?token=${encodeURIComponent(token)}`); | ||
| wsRef.current = ws; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Admin token exposed in WebSocket URL query parameter.
The token is passed as ?token=${encodeURIComponent(token)} which means it appears in:
- Browser history
- Server access logs
- Network inspection tools
For an internal admin dashboard with Bearer token auth, this is a common pattern and acceptable. The backend WebSocket manager (websocket_manager.py) authenticates via this token parameter.
For enhanced security, consider using a subprotocol or initial message for auth instead of query params, but this would require backend changes.
🤖 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 40 - 42, The admin
token is currently embedded in the WebSocket URL (created where
setStatus('connecting') and wsRef.current is set) which exposes it; change the
client to open the WebSocket without the token query (use `${WS_BASE}/ws`) and
then authenticate by sending the token in an initial message or via the optional
subprotocols parameter: create the WebSocket, assign wsRef.current, add
ws.onopen handler that sends a JSON auth message (e.g. {type: 'auth', token}) or
use protocols=[token] if backend supports it, and handle auth success/failure
before proceeding; update any code using the existing URL-based token
accordingly (references: setStatus, WS_BASE, wsRef, and the WebSocket
construction).
Comment on lines
+18
to
+24
| export default function AdminPage() { | ||
| const [searchParams, setSearchParams] = useSearchParams(); | ||
| const section = (searchParams.get('section') ?? DEFAULT_SECTION) as AdminSection; | ||
|
|
||
| const navigate = (s: AdminSection) => { | ||
| setSearchParams({ section: s }, { replace: true }); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Type assertion on URL parameter is unsafe but functionally harmless.
Line 20 casts searchParams.get('section') directly to AdminSection without validation. While the default case in the switch (line 34) safely handles invalid values by rendering OverviewPanel, the type assertion is misleading—TypeScript assumes section is a valid AdminSection when it could be any string.
Consider using a type guard or validation:
const VALID_SECTIONS: Set<AdminSection> = new Set(['overview', 'bounties', 'contributors', 'reviews', 'financial', 'health', 'audit-log']);
const rawSection = searchParams.get('section') ?? DEFAULT_SECTION;
const section: AdminSection = VALID_SECTIONS.has(rawSection as AdminSection) ? (rawSection as AdminSection) : DEFAULT_SECTION;This is a minor type-safety improvement; the current code works correctly due to the fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/pages/AdminPage.tsx` around lines 18 - 24, The code unsafely
type-asserts URL param to AdminSection in AdminPage by doing const section =
(searchParams.get('section') ?? DEFAULT_SECTION) as AdminSection; instead
validate the raw value before casting: read rawSection from searchParams, check
it against a whitelist (e.g., VALID_SECTIONS Set of AdminSection values), and
only assign section = rawSection as AdminSection when it passes the guard,
otherwise use DEFAULT_SECTION; update navigate (which calls setSearchParams)
unchanged but reference the validated section variable throughout the component.
Comment on lines
+129
to
+137
| export interface SystemHealthResponse { | ||
| status: 'healthy' | 'degraded'; | ||
| uptime_seconds: number; | ||
| timestamp: string; | ||
| services: Record<string, string>; | ||
| queue_depth: number; | ||
| webhook_events_processed: number; | ||
| active_websocket_connections: number; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
SystemHealthResponse type is stricter than backend.
The TypeScript type constrains status to 'healthy' | 'degraded', but the backend SystemHealthResponse.status is typed as str. If the backend ever returns a different status value (e.g., 'critical'), TypeScript won't catch it at compile time but the value will still be received.
This is acceptable - the stricter frontend type serves as documentation of expected values. Consider syncing with backend to use an enum.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/types/admin.ts` around lines 129 - 137, The frontend type
SystemHealthResponse is stricter than the backend; update the type so it matches
the backend's flexible string status by changing the status field from the union
'healthy' | 'degraded' to a plain string, i.e., replace the status declaration
in SystemHealthResponse with status: string; alternatively, if you want to keep
stricter values, introduce a shared enum (e.g., SystemHealthStatus) that mirrors
the backend and use that enum for the status field in SystemHealthResponse so
both sides stay in sync.
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
1 participant
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.
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 disconnectedTest results