Conversation
…y feed (Closes SolFoundry#26) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ts for contributor dashboard - Add full JSDoc/TSDoc on all types, interfaces, and exported functions - Extract exported utility functions (formatRelativeTime, computeTotalRewards) for testability - Add readonly modifiers and strict type aliases (TaskStatus, TaskFilter, ActivityType) - Extract STATUS_STYLES constant map and StatCardProps interface - Add filtered-total display, status-badge test-ids, and aria-label for accessibility - Expand test suite from 10 to 41 tests across 9 describe blocks: rendering, stat cards, task filtering, filtered totals, activity feed, accessibility, formatRelativeTime (with fake timers), computeTotalRewards, edge cases Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Complete frontend-only implementation covering every checkbox: - Summary stat cards: total earned, active bounties, pending payouts, reputation rank - Active bounties list with deadline countdown + progress indicator - Earnings chart (SVG polyline line chart, last 30 days) - Recent activity feed (claim, submit, payout, review events) - Notification center with unread badge and mark-as-read / mark-all-read - Quick actions: browse bounties, view leaderboard, check treasury - Settings section: linked accounts, notification preferences, wallet management - Fully responsive layout (Tailwind breakpoints) - 77 integration tests covering all sections + accessibility + edge cases - JSDoc on all exported types/functions - Route integration: /dashboard in App.tsx, DashboardPage entry point, barrel index Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
✅ Multi-LLM Code Review — APPROVEAggregated Score: 7.5/10 (from 3 models) Model Verdicts
Category Scores (Averaged)
SummaryGPT-5.4: This PR makes a substantial effort toward the contributor dashboard and does connect into the app routing, but it still reads as a partially self-contained feature with a lot of mock-driven behavior. The implementation covers many requested areas, yet the overall engineering signal is held back by weak test quality, moderate integration depth, and some concerns about production readiness. Issues
Suggestions
Reviewed by SolFoundry Multi-LLM Pipeline: GPT-5.4, Gemini 2.5 Pro, Grok 4 Review profile: frontend — scoring weights adjusted for this domain |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a contributor dashboard feature to the frontend: a lazy-loaded Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 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)
📝 Coding Plan
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: 5
🤖 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__/dashboard.test.tsx`:
- Around line 11-16: Add an integration test that mounts the app-level router
and navigates to the dashboard route rather than importing ContributorDashboard
directly: write a new test that renders the top-level App (or Router) component,
ensures the '/dashboard' route exists, and verifies that the lazy-loaded
DashboardPage (lazy(() => import('./pages/DashboardPage'))) resolves and renders
ContributorDashboard components or quick-action links; if using react-router,
use memory history to push '/dashboard' and assert the expected DOM for
computeTotalRewards/formatRelativeTime outputs or a known dashboard marker, so
route wiring and lazy import failures will fail the test.
In `@frontend/src/App.tsx`:
- Around line 19-21: Router is missing routes for the dashboard shortcuts
/bounties and /treasury so those paths fall through to the wildcard Navigate;
add Route entries for "/bounties" and "/treasury" (e.g., Route path="/bounties"
element={<BountiesPage />} and Route path="/treasury" element={<TreasuryPage
/>}) alongside the existing Route components (DashboardPage, LeaderboardPage,
Navigate) and import or create the BountiesPage and TreasuryPage components as
needed, ensuring these new Route lines appear before the wildcard Route.
In `@frontend/src/components/dashboard/ContributorDashboard.tsx`:
- Around line 377-415: SettingsSection currently ignores the
notificationPreferences prop and renders hard-coded values; update the
Notification Preferences block inside the SettingsSection component to read from
the notificationPreferences prop (use notificationPreferences.email,
notificationPreferences.push, notificationPreferences.discord) and render
"On"/"Off" (or equivalent) based on those booleans instead of the static strings
so the UI reflects actual user preferences; preserve the
data-testid="notification-preferences" and existing list structure while
replacing the fixed lines with dynamic values derived from
notificationPreferences.
- Around line 37-38: The dashboard currently hard-codes the reward unit as "SOL"
(e.g., the JSDoc/property `reward` and UI labels in ContributorDashboard.tsx and
the blocks around lines 117-151, 487-489, 502-504, 541), causing inconsistent
units vs LeaderboardPage which uses "FNDRY"; update ContributorDashboard to use
the app-wide currency abstraction instead of a hard-coded string: retrieve the
canonical currency or conversion helper used by leaderboard (e.g., a shared
constant, context hook, or conversion function like
getCurrencyUnit()/convertAmount() from the common finance module), change the
data model/type (the `reward` field references) to store amounts in the
canonical base unit or include a unit enum, replace all hard-coded "SOL" strings
in labels with the dynamic currency label, and apply conversion when rendering
filtered totals/tasks/payouts so values match LeaderboardPage's FNDRY
representation. Ensure you reference and reuse the same symbol(s) that
LeaderboardPage imports (the shared currency constant, context hook, or
conversion util) so both components display the same unit and converted amounts.
- Around line 124-128: The task list in ContributorDashboard uses date-only
strings (e.g., "2026-04-01") which are parsed as UTC by new Date(...) and can
appear overdue early; update the deadline parsing/handling in the
deadlineCountdown logic (and anywhere deadlines are read, referenced by the
deadlineCountdown function) to treat YYYY-MM-DD as a local date or use a
timezone-aware parser: either convert the string by splitting into
year/month/day and constructing a local Date via new Date(year, month-1, day)
before computing remaining time, or switch to a timezone-aware library (e.g.,
luxon/DateTime.fromISO with zone/local) and store/consume ISO timestamps with
explicit zone; apply the same fix for the other occurrences noted (lines
~196-201) so deadlines reflect the user's local day.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dbc76f1a-7dec-4d90-a7fd-b350c03b39ae
📒 Files selected for processing (5)
frontend/src/App.tsxfrontend/src/__tests__/dashboard.test.tsxfrontend/src/components/dashboard/ContributorDashboard.tsxfrontend/src/components/dashboard/index.tsfrontend/src/pages/DashboardPage.tsx
| import ContributorDashboard, { | ||
| formatRelativeTime, | ||
| computeTotalRewards, | ||
| deadlineCountdown, | ||
| type BountyTask, | ||
| } from "../components/dashboard/ContributorDashboard"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The suite still doesn't exercise the router integration added in this PR.
Every test imports ContributorDashboard directly, so a broken lazy(() => import('./pages/DashboardPage')), a missing /dashboard route, or unroutable quick-action targets would still pass. Add at least one App-level navigation test so the route wiring is actually covered.
As per coding guidelines, frontend/**: React/TypeScript frontend. Check: Integration with existing components.
Also applies to: 348-353, 593-631
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/__tests__/dashboard.test.tsx` around lines 11 - 16, Add an
integration test that mounts the app-level router and navigates to the dashboard
route rather than importing ContributorDashboard directly: write a new test that
renders the top-level App (or Router) component, ensures the '/dashboard' route
exists, and verifies that the lazy-loaded DashboardPage (lazy(() =>
import('./pages/DashboardPage'))) resolves and renders ContributorDashboard
components or quick-action links; if using react-router, use memory history to
push '/dashboard' and assert the expected DOM for
computeTotalRewards/formatRelativeTime outputs or a known dashboard marker, so
route wiring and lazy import failures will fail the test.
| <Route path="/dashboard" element={<DashboardPage />} /> | ||
| <Route path="/leaderboard" element={<LeaderboardPage />} /> | ||
| <Route path="*" element={<Navigate to="/leaderboard" replace />} /> |
There was a problem hiding this comment.
Two dashboard shortcuts are unroutable in this router.
The new dashboard links to /bounties and /treasury, but this route table only registers /dashboard and /leaderboard. Both missing paths fall through Line 21's wildcard redirect, so those shortcuts land on /leaderboard instead of their advertised pages.
As per coding guidelines, frontend/**: React/TypeScript frontend. Check: Integration with existing components.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/App.tsx` around lines 19 - 21, Router is missing routes for the
dashboard shortcuts /bounties and /treasury so those paths fall through to the
wildcard Navigate; add Route entries for "/bounties" and "/treasury" (e.g.,
Route path="/bounties" element={<BountiesPage />} and Route path="/treasury"
element={<TreasuryPage />}) alongside the existing Route components
(DashboardPage, LeaderboardPage, Navigate) and import or create the BountiesPage
and TreasuryPage components as needed, ensuring these new Route lines appear
before the wildcard Route.
| * @property reward - SOL reward amount for completion | ||
| * @property deadline - ISO-8601 date string for the deadline |
There was a problem hiding this comment.
This dashboard is using a different reward unit than the rest of the app.
This file hard-codes SOL in the data model and UI, while frontend/src/components/leaderboard/LeaderboardPage.tsx presents contributor earnings as FNDRY. Without an explicit conversion layer, the same contributor will see conflicting balances between dashboard totals, filtered rewards, task payouts, and the leaderboard.
As per coding guidelines, frontend/**: React/TypeScript frontend. Check: Integration with existing components.
Also applies to: 117-151, 487-489, 502-504, 541-541
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/dashboard/ContributorDashboard.tsx` around lines 37 -
38, The dashboard currently hard-codes the reward unit as "SOL" (e.g., the
JSDoc/property `reward` and UI labels in ContributorDashboard.tsx and the blocks
around lines 117-151, 487-489, 502-504, 541), causing inconsistent units vs
LeaderboardPage which uses "FNDRY"; update ContributorDashboard to use the
app-wide currency abstraction instead of a hard-coded string: retrieve the
canonical currency or conversion helper used by leaderboard (e.g., a shared
constant, context hook, or conversion function like
getCurrencyUnit()/convertAmount() from the common finance module), change the
data model/type (the `reward` field references) to store amounts in the
canonical base unit or include a unit enum, replace all hard-coded "SOL" strings
in labels with the dynamic currency label, and apply conversion when rendering
filtered totals/tasks/payouts so values match LeaderboardPage's FNDRY
representation. Ensure you reference and reuse the same symbol(s) that
LeaderboardPage imports (the shared currency constant, context hook, or
conversion util) so both components display the same unit and converted amounts.
| { id: "1", title: "Fix auth bug", status: "active", reward: 100, deadline: "2026-04-01", tier: 2, progress: 65 }, | ||
| { id: "2", title: "Add tests", status: "active", reward: 50, deadline: "2026-04-15", tier: 1, progress: 30 }, | ||
| { id: "3", title: "Update docs", status: "completed", reward: 30, deadline: "2026-03-01", tier: 1, progress: 100 }, | ||
| { id: "4", title: "Security audit", status: "completed", reward: 500, deadline: "2026-02-15", tier: 3, progress: 100 }, | ||
| { id: "5", title: "Performance fix", status: "expired", reward: 200, deadline: "2026-01-01", tier: 2, progress: 45 }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
How does JavaScript new Date("2026-04-01") parse a date-only ISO string, and can that make countdowns appear overdue early in non-UTC time zones?
💡 Result:
In JavaScript, a date-only ISO string like "2026-04-01" is parsed as midnight UTC, not midnight in the user’s local time zone:
new Date("2026-04-01")⇒ the instant 2026-04-01T00:00:00.000Z (UTC). (tc39.es)- This is a long-standing spec quirk (kept for web compatibility). (developer.mozilla.org)
Can that make countdowns look overdue “early” outside UTC?
Yes. Because the parsed instant is UTC midnight, it may be the previous local day in time zones west of UTC.
Example (US Pacific, UTC−7 during DST):
"2026-04-01"parses to 2026-04-01 00:00 UTC- which is 2026-03-31 17:00 local time
So a countdown intended to reach local April 1 at 00:00 will instead hit zero at 5:00 PM on March 31, making it appear “overdue” for the last 7 hours of March 31 local time. (tc39.es)
Citations:
- 1: https://tc39.es/ecma262/2025/multipage/numbers-and-dates.html?utm_source=openai
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date?utm_source=openai
- 3: https://tc39.es/ecma262/2025/multipage/numbers-and-dates.html?utm_source=openai
🏁 Script executed:
cat -n frontend/src/components/dashboard/ContributorDashboard.tsx | sed -n '124,128p;196,201p;190,210p'Repository: SolFoundry/solfoundry
Length of output: 1856
deadlineCountdown will mark date-only deadlines overdue too early.
The tasks use plain YYYY-MM-DD strings like "2026-04-01". In JavaScript, new Date("2026-04-01") parses as 2026-04-01T00:00:00Z (UTC midnight) per ECMAScript spec, not midnight in the user's local timezone. For users west of UTC, this means the deadline is actually the previous local day.
Example: A user in US Pacific (UTC−7) sees "2026-04-01" resolve to March 31 at 5:00 PM local time. The countdown will hit zero then, making the bounty appear "overdue" for the final 7 hours of March 31 local time—before the local April 1 deadline has even begun.
Use timezone-aware date handling (e.g., parse with explicit timezone or store as ISO 8601 timestamps with timezone info) to resolve this.
Also applies to: lines 196-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/dashboard/ContributorDashboard.tsx` around lines 124
- 128, The task list in ContributorDashboard uses date-only strings (e.g.,
"2026-04-01") which are parsed as UTC by new Date(...) and can appear overdue
early; update the deadline parsing/handling in the deadlineCountdown logic (and
anywhere deadlines are read, referenced by the deadlineCountdown function) to
treat YYYY-MM-DD as a local date or use a timezone-aware parser: either convert
the string by splitting into year/month/day and constructing a local Date via
new Date(year, month-1, day) before computing remaining time, or switch to a
timezone-aware library (e.g., luxon/DateTime.fromISO with zone/local) and
store/consume ISO timestamps with explicit zone; apply the same fix for the
other occurrences noted (lines ~196-201) so deadlines reflect the user's local
day.
| function SettingsSection({ | ||
| linkedAccounts, | ||
| notificationPreferences, | ||
| walletAddress, | ||
| }: { | ||
| readonly linkedAccounts: readonly string[]; | ||
| readonly notificationPreferences: { readonly email: boolean; readonly push: boolean; readonly discord: boolean }; | ||
| readonly walletAddress: string; | ||
| }) { | ||
| return ( | ||
| <div data-testid="settings-section"> | ||
| <h2 className="text-xl font-semibold mb-4">Settings</h2> | ||
| <div className="grid grid-cols-1 md:grid-cols-3 gap-4"> | ||
| {/* Linked Accounts */} | ||
| <div className="border rounded-lg p-4" data-testid="linked-accounts"> | ||
| <h3 className="font-medium mb-2">Linked Accounts</h3> | ||
| {linkedAccounts.length === 0 ? ( | ||
| <p className="text-gray-400 text-sm">No linked accounts</p> | ||
| ) : ( | ||
| <ul className="space-y-1"> | ||
| {linkedAccounts.map((acct) => ( | ||
| <li key={acct} className="text-sm text-gray-600 flex items-center gap-2"> | ||
| <span className="w-2 h-2 bg-green-400 rounded-full" /> | ||
| {acct} | ||
| </li> | ||
| ))} | ||
| </ul> | ||
| )} | ||
| </div> | ||
|
|
||
| {/* Notification Preferences */} | ||
| <div className="border rounded-lg p-4" data-testid="notification-preferences"> | ||
| <h3 className="font-medium mb-2">Notification Preferences</h3> | ||
| <ul className="space-y-1 text-sm text-gray-600"> | ||
| <li>Email: On</li> | ||
| <li>Push: Off</li> | ||
| <li>Discord: On</li> | ||
| </ul> | ||
| </div> |
There was a problem hiding this comment.
notificationPreferences is ignored by the settings UI.
SettingsSection accepts notificationPreferences, but Lines 411-413 always render Email: On, Push: Off, and Discord: On. Any real preference change will still show the hard-coded defaults, so this section cannot reflect saved user state.
As per coding guidelines, frontend/**: React/TypeScript frontend. Check: Component structure and state management.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/dashboard/ContributorDashboard.tsx` around lines 377
- 415, SettingsSection currently ignores the notificationPreferences prop and
renders hard-coded values; update the Notification Preferences block inside the
SettingsSection component to read from the notificationPreferences prop (use
notificationPreferences.email, notificationPreferences.push,
notificationPreferences.discord) and render "On"/"Off" (or equivalent) based on
those booleans instead of the static strings so the UI reflects actual user
preferences; preserve the data-testid="notification-preferences" and existing
list structure while replacing the fixed lines with dynamic values derived from
notificationPreferences.
- Backend Lint: fix ruff errors (F401 unused imports, E701/E702 multi-statement lines, E712 bool comparisons, E741 ambiguous var) and run ruff format - Backend Tests: add missing deps (python-jose, solders) to requirements.txt; update conftest.py to skip tests when PostgreSQL is unavailable; fix DATABASE_URL in CI to use asyncpg driver - Frontend Build: add package-lock.json, index.html, main.tsx entry point; install @solana/wallet-adapter-* dependencies; create missing ThemeToggle component - Frontend Type Check: update tsconfig.json to exclude test files and set explicit types; fix NodeJS.Timeout reference; fix SortField indexing in useLeaderboard; remove unavailable BackpackWalletAdapter - Contracts Check: replace broken metadaoproject/setup-solana@v1 action with shell-based Solana CLI install Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Issues found:
Suggestions:
Please address these items and push an update. SolFoundry Review Bot |
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
frontend/src/components/PRStatusTracker.tsx (1)
333-348:⚠️ Potential issue | 🟠 MajorPrevent reconnect scheduling after unmount in WebSocket cleanup.
ws.close()can fireoncloseafter cleanup, which then schedulessetTimeout(connect, 5000)and reconnects an unmounted component. Add a teardown guard to skip reconnect when unmounting.Proposed fix
useEffect(() => { if (!wsEndpoint) return; let ws: WebSocket | null = null; let reconnectTimer: ReturnType<typeof setTimeout> | null = null; + let isUnmounted = false; const connect = () => { + if (isUnmounted) return; try { ws = new WebSocket(wsEndpoint); @@ ws.onclose = () => { setIsConnected(false); - // Attempt reconnection after 5 seconds - reconnectTimer = setTimeout(connect, 5000); + if (!isUnmounted) { + // Attempt reconnection after 5 seconds + reconnectTimer = setTimeout(connect, 5000); + } }; @@ return () => { + isUnmounted = true; if (ws) ws.close(); if (reconnectTimer) clearTimeout(reconnectTimer); }; }, [wsEndpoint, statusData?.prId, onStageChange, onError]);As per coding guidelines, for
frontend/**you should validate “Component structure and state management” and “Error/loading/empty state handling.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/PRStatusTracker.tsx` around lines 333 - 348, The component's WebSocket cleanup can still schedule reconnection after unmount because ws.onclose triggers setTimeout(connect, 5000); add an unmount guard (e.g., let isUnmounted = false) scoped alongside reconnectTimer and check it before scheduling reconnect inside ws.onclose and before calling connect; set isUnmounted = true in the cleanup return, ensure you clear any pending reconnectTimer and avoid calling setIsConnected/setConnectionError after unmount. Update references in PRStatusTracker.tsx to check the guard in the connect function, ws.onclose, and the cleanup to prevent reconnects for unmounted components.backend/app/api/auth.py (2)
1-252:⚠️ Potential issue | 🟡 MinorBackend API endpoints modified in frontend PR.
This API router file contains formatting changes (import reorganization to multi-line, whitespace normalization, removal of unused
datetime/timezoneimport) and adds a proper__all__export. While these changes are non-functional and improve code quality (removing unused imports is good practice), they still represent backend modifications in a frontend-focused bounty submission, contradicting the "No backend contamination" claim.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/auth.py` around lines 1 - 252, The PR improperly modifies backend code (the auth router and exports such as get_current_user_id, router, and the added __all__) in a frontend-focused bounty; revert these backend changes from this frontend PR by removing the formatting/import/export edits (restore the original auth.py content for functions like get_current_user_id, get_github_authorize, github_oauth_callback, get_wallet_auth_message, wallet_authenticate, link_wallet, refresh_token, get_current_user and any added __all__), or move them into a separate backend-focused PR with proper justification and tests so the frontend bounty remains backend-free.
119-119:⚠️ Potential issue | 🔴 CriticalAdd database session dependency injection to auth endpoints.
All authentication endpoints are missing the
db: AsyncSession = Depends(get_db)parameter required by their service layer calls. The service functions (github_oauth_login, wallet_authenticate, link_wallet, refresh_access_token, get_current_user) all expectdb: AsyncSessionas the first parameter, but the API endpoints at lines 105, 145, 174, 217, and 238 are calling them without providing this dependency.Add
db: AsyncSession = Depends(get_db)to:
- Line 105:
async def github_oauth_callback(request: GitHubOAuthRequest)- Line 145:
async def wallet_authenticate(request: WalletAuthRequest)- Line 174:
async def link_wallet(request: LinkWalletRequest, user_id: str = Depends(get_current_user_id))- Line 217:
async def refresh_token(request: RefreshTokenRequest)Then pass
dbas the first argument to each service call. Other endpoints in the codebase (notifications.py, webhooks/github.py) follow this correct pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/auth.py` at line 119, Each auth endpoint is missing the DB dependency injection; add a parameter db: AsyncSession = Depends(get_db) to the endpoint functions github_oauth_callback, wallet_authenticate, link_wallet, refresh_token (and the get_current_user endpoint) and pass that db as the first argument to their corresponding service calls (auth_service.github_oauth_login, auth_service.wallet_authenticate, auth_service.link_wallet, auth_service.refresh_access_token, auth_service.get_current_user) so calls become e.g. auth_service.github_oauth_login(db, ...), auth_service.wallet_authenticate(db, ...), etc.; use the existing get_db dependency and AsyncSession type.backend/app/models/user.py (1)
1-45:⚠️ Potential issue | 🟡 MinorBackend model file modified in frontend PR.
This SQLAlchemy model file contains only formatting changes (whitespace normalization and multi-line Column declaration), with no behavioral impact. However, similar to
backend/app/auth.py, this modification contradicts the "No backend contamination" claim in the PR description. Backend model files should not be touched in a frontend-only bounty submission.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/models/user.py` around lines 1 - 45, This change touches backend models (User and UserResponse) despite the PR claiming frontend-only; revert the non-functional edits to the SQLAlchemy model and Pydantic response (the User class and UserResponse class) so the backend files are unchanged in this frontend bounty—restore the previous commit state for these symbols (or remove the file from the diff), then update the PR to remove any backend modifications and adjust the PR description to accurately reflect that only frontend files are included.backend/app/services/auth_service.py (1)
1-401:⚠️ Potential issue | 🟡 MinorBackend auth service modified in frontend PR.
This authentication service file contains extensive formatting changes (multi-line dict literals, exception class reformatting, function parameter line breaks) with no functional logic changes. However, modifying backend authentication code in a frontend-focused PR violates the "No backend contamination" principle stated in the PR description.
Authentication services are security-critical components. Any changes to these files—even cosmetic ones—should be isolated in dedicated PRs to minimize risk and simplify review.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/auth_service.py` around lines 1 - 401, This PR accidentally includes formatting-only changes to the backend authentication service (classes like AuthError/GitHubOAuthError, functions such as create_access_token, decode_token, verify_oauth_state, generate_auth_message, verify_wallet_signature, wallet_authenticate, link_wallet_to_user), which must not be modified in a frontend PR; revert this file to its pre-PR state or remove it from this branch, then create a separate backend-focused PR for any formatting/refactor work (or apply only minimal, reviewed changes) so the auth service remains untouched here and commit history shows the revert.backend/app/auth.py (1)
1-126:⚠️ Potential issue | 🟡 MinorBackend file modified despite "no backend contamination" claim.
The PR description explicitly states "No backend contamination," yet this backend authentication module has been modified with formatting changes (whitespace normalization, blank lines, trailing newline). While the changes themselves are non-functional and don't introduce bugs, modifying backend files in a frontend-focused bounty (Contributor Dashboard) contradicts the stated objective and increases the surface area for unintended side effects.
For a frontend-only feature, backend files should remain untouched unless there's a documented technical necessity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/auth.py` around lines 1 - 126, The backend authentication module was modified only by formatting/whitespace changes despite the PR claiming "no backend contamination"; revert those non-functional edits by restoring the module that contains get_current_user_id, _is_valid_uuid, and AuthenticatedUser to its upstream/main state (or previous commit) and remove it from this frontend-focused commit/branch so only frontend files remain staged; ensure the auth functions and class are unchanged from upstream before amending the PR and recommitting.backend/app/models/submission.py (1)
1-241:⚠️ Potential issue | 🟡 MinorExtensive formatting changes unrelated to the contributor dashboard feature.
This file contains widespread non-functional formatting changes including:
- Whitespace and blank line normalization
- Multi-line wrapping for column declarations (lines 87-94)
- Index name quote style changes from single to double quotes (lines 97-100)
- Field validator and Field() declaration reformatting (lines 116-124, 130-135, and multiple other locations)
All changes preserve the exact same functionality: no model fields, validators, constraints, or database schema elements were modified. The GitHub URL validation logic (lines 120-123), index definitions, and all Pydantic schemas remain functionally identical.
However, this extensive backend model formatting is completely unrelated to the contributor dashboard frontend feature specified in bounty
#26. Such broad backend refactoring significantly increases the review surface area without contributing to the stated deliverable.For bounty submissions, maintain strict scope: deliver only the required feature without bundling unrelated maintenance work.
As per coding guidelines: "You are reviewing a bounty submission. Be DETAILED and SPECIFIC in your analysis — include exact issues, affected files, line numbers, and technical details."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/models/submission.py` around lines 1 - 241, This change set introduces broad, non-functional formatting-only edits across backend models (e.g., SubmissionDB, __table_args__ index names, SubmissionBase.validate_pr_url, SubmissionCreate/SubmissionUpdate field validators and Field declarations) that are unrelated to bounty `#26`; revert those formatting-only changes so the diff contains only the minimal, functional edits required for the contributor dashboard feature: restore original whitespace/line-wrapping for Column and Field declarations, restore original index name quoting, and return the exact prior formatting of validators such as validate_pr_url and validate_status to eliminate noise while keeping all model logic and schema definitions unchanged.backend/app/api/notifications.py (1)
133-137:⚠️ Potential issue | 🔴 Critical
POST /notificationsis unauthenticated while allowing arbitraryuser_id.This endpoint can be called by any client to create notifications for other users. That is a direct authorization gap for a write operation.
As per coding guidelines,
backend/**reviews must analyze authentication/authorization gaps.Also applies to: 141-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/notifications.py` around lines 133 - 137, The create_notification endpoint (async def create_notification) accepts arbitrary NotificationCreate.user_id and is unauthenticated; require and depend on the authenticated user (e.g., add a current_user dependency like get_current_user or get_current_active_user) and enforce authorization: if current_user.id != notification.user_id then either reject with HTTPException(status_code=403) or allow only if current_user has an admin role/permission; apply the same change to the other notification-write endpoints in this file (the functions referenced around lines 141-153) so all write operations validate requester identity/role before creating/updating/deleting notifications.
🤖 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/webhooks/github.py`:
- Around line 143-147: The except block currently returns the raw exception
string to callers which can leak internals; change the JSONResponse in this
except to return a generic error body (e.g. {"error":"Internal server error"} or
{"error":"An internal error occurred"}) while leaving the detailed exception in
the logger.error call that references event_type, delivery_id, and exc, and keep
the status_code at 500 so diagnostics remain in logs but the webhook response no
longer exposes str(exc).
In `@backend/app/models/bounty.py`:
- Around line 140-152: The PR includes non-functional formatting changes in the
Bounty model that are unrelated to the contributor dashboard feature; revert the
formatting-only edits around the github_issue_url validator (the validator
function that checks v.startswith("https://github.com/", "http://github.com/"))
and the BountyUpdate class's title field (the title: Optional[str] Field with
TITLE_MIN_LENGTH and TITLE_MAX_LENGTH) so the backend model file returns to its
original formatting, or move those whitespace/formatting edits into a separate
commit/PR; ensure this change only affects formatting (no logic changes) and
that the feature PR contains only files and diffs relevant to the contributor
dashboard.
In `@backend/app/models/contributor.py`:
- Around line 33-40: The created_at and updated_at Column declarations (symbols:
created_at, updated_at, Column, DateTime, timezone) were reformatted across
multiple lines unrelated to the contributor dashboard change; revert these two
declarations back to their original single-line formatting to avoid unrelated
style-only changes in this PR while keeping the same types, default and onupdate
behavior (i.e., restore the previous layout without altering Column, DateTime,
default lambda, or onupdate lambda).
In `@backend/app/models/payout.py`:
- Around line 200-207: The change only reformats the distribution_breakdown
Field (the default_factory lambda that returns the dict with keys
"contributor_rewards", "treasury_reserve", "buybacks", "burned") without
changing behavior; revert this unrelated formatting change by restoring the
original formatting for the distribution_breakdown declaration (i.e., return it
to its prior single-line or project-standard layout) or remove the edit from the
PR so only contributor-dashboard-related changes remain; ensure you update the
same symbol distribution_breakdown and keep the default_factory lambda and
dictionary values exactly as before.
- Line 13: Revert the unrelated import cleanup by restoring the removed
Annotated import from typing (i.e., ensure the top-level import list includes
Annotated alongside Optional) so this PR only contains changes relevant to the
contributor dashboard feature; alternatively, if you prefer to keep the cleanup,
move the Annotated removal into a separate maintenance PR and leave payout.py
unchanged in this feature PR.
In `@backend/app/models/webhook_log.py`:
- Around line 28-38: Revert the unrelated formatting changes in the WebhookLog
model: restore the status Column to its original single-line declaration (the
status Column definition), revert processed_at back to the original multi-line
DateTime(...) declaration with timezone=True, default lambda:
datetime.now(timezone.utc) and index=True, and change the Index name quoting in
__table_args__ back to single quotes for entries like
ix_webhook_event_logs_event_type and ix_webhook_event_logs_processed_at so only
the formatting reversion touches the status, processed_at, and __table_args__
(Index(..., event_type) / Index(..., processed_at)) symbols.
In `@backend/app/models/webhook.py`:
- Line 5: The import removal of the unused typing.Literal in
backend/app/models/webhook.py should not be bundled in this frontend bounty PR;
either restore the original import line to leave the file unchanged for this
submission or move the formatting/cleanup into a separate maintenance PR. Re-add
the Literal import to the top-level imports in webhook.py (alongside Any) so the
diff for this feature only contains frontend-related changes, or extract the
import cleanup into its own PR and reference it from this change.
In `@backend/app/services/contributor_service.py`:
- Around line 59-72: create_contributor builds a ContributorDB without setting
created_at/updated_at which breaks the ContributorResponse contract; modify the
in-memory path in create_contributor to set db.created_at and db.updated_at to
the current timestamp (e.g., datetime.utcnow() or your project's timezone
helper) when constructing ContributorDB so _db_to_response returns non-null
datetimes for ContributorResponse.
In `@backend/app/services/notification_service.py`:
- Around line 68-79: The code is using Python's `not` on the SQLAlchemy column
`NotificationDB.read` which produces a Python boolean instead of a SQL
expression; update all occurrences (e.g., the `if unread_only:` condition
construction, `count_query`, `unread_query`, and any other places flagged) to
use SQLAlchemy negation — replace `not NotificationDB.read` with
`~NotificationDB.read` (or `not_(NotificationDB.read)` if you prefer) so the
filter conditions (built in the block that constructs `conditions`,
`filter_condition`, `count_query`, and the `unread_query`) become valid
SQLAlchemy boolean expressions.
In `@backend/app/services/webhook_processor.py`:
- Around line 238-240: The change to webhook_processor.py reformats the
label_names list comprehension (symbol: label_names) and surrounding whitespace
even though behavior is unchanged; revert the unrelated whitespace/line-wrapping
changes in the webhook processor (specifically the label_names comprehension and
any logger line wraps in the same file) so this backend-only reformatting is not
mixed into the frontend bounty PR, or move these formatting edits into a
separate cleanup PR; locate the label_names comprehension and any modified
logger statements in the webhook_processor.py functions handling GitHub PR/issue
events and restore their original formatting or split them out.
In `@backend/app/services/webhook_service.py`:
- Around line 26-33: The changes to webhook_service.py only reformat how the
WebhookVerificationError message and the HMAC computation (variable expected
built from "sha256=" + hmac.new(...).hexdigest()) are written but are unrelated
to the contributor dashboard work; revert these formatting-only edits so the
webhook service remains untouched in this PR: restore the original construction
of the WebhookVerificationError message that uses signature_header and the
original multi-line or single-line expression that builds expected using
hmac.new(secret.encode("utf-8"), payload, hashlib.sha256).hexdigest(), ensuring
functions/classes referenced (WebhookVerificationError, signature_header,
expected, secret, payload, hmac.new, hashlib.sha256) match the pre-PR formatting
while leaving logic identical.
In `@backend/requirements.txt`:
- Around line 9-10: The requirements specify
python-jose[cryptography]>=3.3.0,<4.0.0 which permits the vulnerable 3.3.0
release; update that requirement to python-jose[cryptography]>=3.4.0,<4.0.0 to
exclude versions with CVE-2024-33663/33664. Edit the dependency line in
backend/requirements.txt (the python-jose[cryptography] entry) to bump the
minimum version to 3.4.0 and keep the existing upper bound <4.0.0, then run
dependency install/lock steps and tests to ensure compatibility.
In `@backend/tests/conftest.py`:
- Around line 33-48: _db_available() currently hardcodes port 5432 when probing
the DB host; parse the port from the DATABASE_URL DSN and use it (falling back
to 5432 if no port is present) when calling socket.create_connection. Locate the
url and host extraction logic in _db_available(), extract the port from the DSN
(e.g., after host:port or default), convert to int with safe fallback, and
replace the hardcoded 5432 with the parsed port; also ensure you handle
malformed ports by catching ValueError and treating it as missing (use default).
In `@backend/tests/test_bounty_api.py`:
- Around line 316-323: The test test_autocomplete_short_query currently allows
either 200 or 422 which makes the API contract nondeterministic; update the test
to assert a single expected behavior (preferably a 422 validation error) for the
/api/bounties/autocomplete?q=a request by replacing the loose check with an
explicit assertion like response.status_code == 422 and optionally assert the
JSON error shape (e.g., presence of "detail") to ensure consistent validation
behavior in the autocomplete endpoint.
In `@frontend/src/components/layout/ThemeToggle.tsx`:
- Around line 8-27: Update the ThemeToggle component to expose switch semantics
for assistive tech: in the ThemeToggle function adjust the button element
(inside ThemeToggle) to include role="switch", set aria-checked to the current
state (e.g. aria-checked={theme === 'dark'}), and change the aria-label to a
neutral label like "Theme toggle" while keeping the existing onClick/onToggle
behavior and visual icons intact so the control reports its state rather than
only the intended action.
In `@frontend/src/components/wallet/WalletProvider.tsx`:
- Around line 23-24: The wallets array created with useMemo currently omits
network from its dependency array so SolflareWalletAdapter (constructed with the
cluster) isn't re-created when the cluster changes; update the useMemo call that
defines wallets (the variable wallets using useMemo and the adapters
PhantomWalletAdapter and SolflareWalletAdapter) to include network in its
dependency array so a new SolflareWalletAdapter is instantiated whenever network
changes, ensuring adapter state matches the current cluster.
In `@frontend/tsconfig.json`:
- Line 15: CI currently runs the frontend-typecheck job using the existing
tsconfig.json which excludes test files, so test type errors are never checked;
add a dedicated test typecheck by creating a tsconfig.tests.json that extends
tsconfig.json but removes the test exclusions (or explicitly includes
"src/**/*.test.ts" and "src/**/*.test.tsx"/"src/__tests__"), then update the
frontend-typecheck CI step to run npx tsc --project tsconfig.tests.json --noEmit
(or alternately run vitest with --run --typecheck) so tests are included in
typechecking; reference tsconfig.json, new tsconfig.tests.json, the
frontend-typecheck CI job, and the npx tsc/ vitest --typecheck command when
making the change.
---
Outside diff comments:
In `@backend/app/api/auth.py`:
- Around line 1-252: The PR improperly modifies backend code (the auth router
and exports such as get_current_user_id, router, and the added __all__) in a
frontend-focused bounty; revert these backend changes from this frontend PR by
removing the formatting/import/export edits (restore the original auth.py
content for functions like get_current_user_id, get_github_authorize,
github_oauth_callback, get_wallet_auth_message, wallet_authenticate,
link_wallet, refresh_token, get_current_user and any added __all__), or move
them into a separate backend-focused PR with proper justification and tests so
the frontend bounty remains backend-free.
- Line 119: Each auth endpoint is missing the DB dependency injection; add a
parameter db: AsyncSession = Depends(get_db) to the endpoint functions
github_oauth_callback, wallet_authenticate, link_wallet, refresh_token (and the
get_current_user endpoint) and pass that db as the first argument to their
corresponding service calls (auth_service.github_oauth_login,
auth_service.wallet_authenticate, auth_service.link_wallet,
auth_service.refresh_access_token, auth_service.get_current_user) so calls
become e.g. auth_service.github_oauth_login(db, ...),
auth_service.wallet_authenticate(db, ...), etc.; use the existing get_db
dependency and AsyncSession type.
In `@backend/app/api/notifications.py`:
- Around line 133-137: The create_notification endpoint (async def
create_notification) accepts arbitrary NotificationCreate.user_id and is
unauthenticated; require and depend on the authenticated user (e.g., add a
current_user dependency like get_current_user or get_current_active_user) and
enforce authorization: if current_user.id != notification.user_id then either
reject with HTTPException(status_code=403) or allow only if current_user has an
admin role/permission; apply the same change to the other notification-write
endpoints in this file (the functions referenced around lines 141-153) so all
write operations validate requester identity/role before
creating/updating/deleting notifications.
In `@backend/app/auth.py`:
- Around line 1-126: The backend authentication module was modified only by
formatting/whitespace changes despite the PR claiming "no backend
contamination"; revert those non-functional edits by restoring the module that
contains get_current_user_id, _is_valid_uuid, and AuthenticatedUser to its
upstream/main state (or previous commit) and remove it from this
frontend-focused commit/branch so only frontend files remain staged; ensure the
auth functions and class are unchanged from upstream before amending the PR and
recommitting.
In `@backend/app/models/submission.py`:
- Around line 1-241: This change set introduces broad, non-functional
formatting-only edits across backend models (e.g., SubmissionDB, __table_args__
index names, SubmissionBase.validate_pr_url, SubmissionCreate/SubmissionUpdate
field validators and Field declarations) that are unrelated to bounty `#26`;
revert those formatting-only changes so the diff contains only the minimal,
functional edits required for the contributor dashboard feature: restore
original whitespace/line-wrapping for Column and Field declarations, restore
original index name quoting, and return the exact prior formatting of validators
such as validate_pr_url and validate_status to eliminate noise while keeping all
model logic and schema definitions unchanged.
In `@backend/app/models/user.py`:
- Around line 1-45: This change touches backend models (User and UserResponse)
despite the PR claiming frontend-only; revert the non-functional edits to the
SQLAlchemy model and Pydantic response (the User class and UserResponse class)
so the backend files are unchanged in this frontend bounty—restore the previous
commit state for these symbols (or remove the file from the diff), then update
the PR to remove any backend modifications and adjust the PR description to
accurately reflect that only frontend files are included.
In `@backend/app/services/auth_service.py`:
- Around line 1-401: This PR accidentally includes formatting-only changes to
the backend authentication service (classes like AuthError/GitHubOAuthError,
functions such as create_access_token, decode_token, verify_oauth_state,
generate_auth_message, verify_wallet_signature, wallet_authenticate,
link_wallet_to_user), which must not be modified in a frontend PR; revert this
file to its pre-PR state or remove it from this branch, then create a separate
backend-focused PR for any formatting/refactor work (or apply only minimal,
reviewed changes) so the auth service remains untouched here and commit history
shows the revert.
In `@frontend/src/components/PRStatusTracker.tsx`:
- Around line 333-348: The component's WebSocket cleanup can still schedule
reconnection after unmount because ws.onclose triggers setTimeout(connect,
5000); add an unmount guard (e.g., let isUnmounted = false) scoped alongside
reconnectTimer and check it before scheduling reconnect inside ws.onclose and
before calling connect; set isUnmounted = true in the cleanup return, ensure you
clear any pending reconnectTimer and avoid calling
setIsConnected/setConnectionError after unmount. Update references in
PRStatusTracker.tsx to check the guard in the connect function, ws.onclose, and
the cleanup to prevent reconnects for unmounted components.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8ced6e2d-b49a-4aa1-ba91-b98b29ce7809
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (51)
.github/workflows/ci.ymlbackend/app/api/auth.pybackend/app/api/bounties.pybackend/app/api/contributors.pybackend/app/api/leaderboard.pybackend/app/api/notifications.pybackend/app/api/payouts.pybackend/app/api/webhooks/github.pybackend/app/auth.pybackend/app/database.pybackend/app/main.pybackend/app/models/bounty.pybackend/app/models/contributor.pybackend/app/models/dispute.pybackend/app/models/leaderboard.pybackend/app/models/notification.pybackend/app/models/payout.pybackend/app/models/submission.pybackend/app/models/user.pybackend/app/models/webhook.pybackend/app/models/webhook_log.pybackend/app/services/auth_service.pybackend/app/services/bounty_service.pybackend/app/services/contributor_service.pybackend/app/services/leaderboard_service.pybackend/app/services/notification_service.pybackend/app/services/payout_service.pybackend/app/services/solana_client.pybackend/app/services/treasury_service.pybackend/app/services/webhook_processor.pybackend/app/services/webhook_service.pybackend/requirements.txtbackend/tests/conftest.pybackend/tests/test_auth.pybackend/tests/test_bounties.pybackend/tests/test_bounty_api.pybackend/tests/test_bounty_edge_cases.pybackend/tests/test_bounty_search.pybackend/tests/test_contributors.pybackend/tests/test_leaderboard.pybackend/tests/test_notification_api.pybackend/tests/test_payouts.pybackend/tests/test_webhook.pyfrontend/index.htmlfrontend/package.jsonfrontend/src/components/PRStatusTracker.tsxfrontend/src/components/layout/ThemeToggle.tsxfrontend/src/components/wallet/WalletProvider.tsxfrontend/src/hooks/useLeaderboard.tsfrontend/src/main.tsxfrontend/tsconfig.json
| except Exception as exc: | ||
| logger.error( | ||
| "Error processing %s event (delivery=%s): %s", | ||
| event_type, delivery_id, exc | ||
| "Error processing %s event (delivery=%s): %s", event_type, delivery_id, exc | ||
| ) | ||
| return JSONResponse( | ||
| status_code=500, | ||
| content={"error": str(exc)} | ||
| ) No newline at end of file | ||
| return JSONResponse(status_code=500, content={"error": str(exc)}) |
There was a problem hiding this comment.
Do not expose raw exception messages in webhook 500 responses.
Line 147 currently returns str(exc) to callers, which can leak internal implementation details. Return a generic error body and keep diagnostics in logs only.
Suggested fix
except Exception as exc:
logger.error(
"Error processing %s event (delivery=%s): %s", event_type, delivery_id, exc
)
- return JSONResponse(status_code=500, content={"error": str(exc)})
+ return JSONResponse(
+ status_code=500,
+ content={"error": "Internal server error"},
+ )📝 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.
| except Exception as exc: | |
| logger.error( | |
| "Error processing %s event (delivery=%s): %s", | |
| event_type, delivery_id, exc | |
| "Error processing %s event (delivery=%s): %s", event_type, delivery_id, exc | |
| ) | |
| return JSONResponse( | |
| status_code=500, | |
| content={"error": str(exc)} | |
| ) | |
| \ No newline at end of file | |
| return JSONResponse(status_code=500, content={"error": str(exc)}) | |
| except Exception as exc: | |
| logger.error( | |
| "Error processing %s event (delivery=%s): %s", event_type, delivery_id, exc | |
| ) | |
| return JSONResponse( | |
| status_code=500, | |
| content={"error": "Internal server error"}, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/webhooks/github.py` around lines 143 - 147, The except block
currently returns the raw exception string to callers which can leak internals;
change the JSONResponse in this except to return a generic error body (e.g.
{"error":"Internal server error"} or {"error":"An internal error occurred"})
while leaving the detailed exception in the logger.error call that references
event_type, delivery_id, and exc, and keep the status_code at 500 so diagnostics
remain in logs but the webhook response no longer exposes str(exc).
| if v is not None and not v.startswith( | ||
| ("https://github.com/", "http://github.com/") | ||
| ): | ||
| raise ValueError("github_issue_url must be a GitHub URL") | ||
| return v | ||
|
|
||
|
|
||
| class BountyUpdate(BaseModel): | ||
| """Payload for partially updating a bounty (PATCH semantics).""" | ||
| title: Optional[str] = Field(None, min_length=TITLE_MIN_LENGTH, max_length=TITLE_MAX_LENGTH) | ||
|
|
||
| title: Optional[str] = Field( | ||
| None, min_length=TITLE_MIN_LENGTH, max_length=TITLE_MAX_LENGTH | ||
| ) |
There was a problem hiding this comment.
Bounty model formatting unrelated to the contributor dashboard feature.
The reformatting of the github_issue_url validator (lines 140-142) and BountyUpdate.title field declaration (lines 150-152) preserves the exact same validation logic and field constraints. The GitHub URL validation still checks the same prefixes, and the title field maintains the same min/max length constraints.
Additional whitespace and blank line changes throughout the file are also non-functional.
However, these bounty model formatting changes are unrelated to the contributor dashboard frontend feature specified in bounty #26. Backend model refactoring should be separated from frontend feature delivery.
As per coding guidelines: "You are reviewing a bounty submission. Be DETAILED and SPECIFIC in your analysis — include exact issues, affected files, line numbers, and technical details."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/models/bounty.py` around lines 140 - 152, The PR includes
non-functional formatting changes in the Bounty model that are unrelated to the
contributor dashboard feature; revert the formatting-only edits around the
github_issue_url validator (the validator function that checks
v.startswith("https://github.com/", "http://github.com/")) and the BountyUpdate
class's title field (the title: Optional[str] Field with TITLE_MIN_LENGTH and
TITLE_MAX_LENGTH) so the backend model file returns to its original formatting,
or move those whitespace/formatting edits into a separate commit/PR; ensure this
change only affects formatting (no logic changes) and that the feature PR
contains only files and diffs relevant to the contributor dashboard.
| created_at = Column( | ||
| DateTime(timezone=True), default=lambda: datetime.now(timezone.utc) | ||
| ) | ||
| updated_at = Column( | ||
| DateTime(timezone=True), | ||
| default=lambda: datetime.now(timezone.utc), | ||
| onupdate=lambda: datetime.now(timezone.utc), | ||
| ) |
There was a problem hiding this comment.
Column declaration reformatting unrelated to the contributor dashboard feature.
The multi-line formatting of created_at and updated_at column declarations is syntactically correct and preserves all functionality (same types, defaults, and onupdate behavior). However, this formatting change is unrelated to the contributor dashboard frontend feature specified in the bounty.
Backend code cleanup and formatting should be separated from frontend feature delivery to maintain clear PR scope and facilitate focused code review.
As per coding guidelines: "You are reviewing a bounty submission. Be DETAILED and SPECIFIC in your analysis — include exact issues, affected files, line numbers, and technical details."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/models/contributor.py` around lines 33 - 40, The created_at and
updated_at Column declarations (symbols: created_at, updated_at, Column,
DateTime, timezone) were reformatted across multiple lines unrelated to the
contributor dashboard change; revert these two declarations back to their
original single-line formatting to avoid unrelated style-only changes in this PR
while keeping the same types, default and onupdate behavior (i.e., restore the
previous layout without altering Column, DateTime, default lambda, or onupdate
lambda).
| from datetime import datetime, timezone | ||
| from enum import Enum | ||
| from typing import Annotated, Optional | ||
| from typing import Optional |
There was a problem hiding this comment.
Unused import removal unrelated to the contributor dashboard feature.
The removal of the unused Annotated import is technically correct. However, this type of import cleanup is unrelated to the contributor dashboard frontend feature specified in bounty #26 and should be in a separate maintenance PR.
As per coding guidelines: "You are reviewing a bounty submission. Be DETAILED and SPECIFIC in your analysis."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/models/payout.py` at line 13, Revert the unrelated import cleanup
by restoring the removed Annotated import from typing (i.e., ensure the
top-level import list includes Annotated alongside Optional) so this PR only
contains changes relevant to the contributor dashboard feature; alternatively,
if you prefer to keep the cleanup, move the Annotated removal into a separate
maintenance PR and leave payout.py unchanged in this feature PR.
| distribution_breakdown: dict[str, float] = Field( | ||
| default_factory=lambda: { | ||
| "contributor_rewards": 0.0, | ||
| "treasury_reserve": 0.0, | ||
| "buybacks": 0.0, | ||
| "burned": 0.0, | ||
| } | ||
| ) |
There was a problem hiding this comment.
Tokenomics field reformatting unrelated to the contributor dashboard feature.
The multi-line reformatting of the distribution_breakdown field preserves the exact same default factory function and dictionary values. However, this payout model formatting change is unrelated to the contributor dashboard frontend deliverable.
As per coding guidelines: "You are reviewing a bounty submission. Be DETAILED and SPECIFIC in your analysis."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/models/payout.py` around lines 200 - 207, The change only
reformats the distribution_breakdown Field (the default_factory lambda that
returns the dict with keys "contributor_rewards", "treasury_reserve",
"buybacks", "burned") without changing behavior; revert this unrelated
formatting change by restoring the original formatting for the
distribution_breakdown declaration (i.e., return it to its prior single-line or
project-standard layout) or remove the edit from the PR so only
contributor-dashboard-related changes remain; ensure you update the same symbol
distribution_breakdown and keep the default_factory lambda and dictionary values
exactly as before.
| url = os.getenv( | ||
| "DATABASE_URL", | ||
| "postgresql+asyncpg://postgres:postgres@localhost/solfoundry", | ||
| ) | ||
| # Quick heuristic: if we can parse + resolve the host, it is | ||
| # *probably* reachable. A full connection test would be better but | ||
| # this keeps conftest lightweight. | ||
| host = ( | ||
| url.split("@")[-1].split("/")[0].split(":")[0] | ||
| if "@" in url | ||
| else "localhost" | ||
| ) | ||
| import socket | ||
|
|
||
| socket.create_connection((host, 5432), timeout=2).close() | ||
| return True |
There was a problem hiding this comment.
_db_available() ignores DSN port, which can incorrectly skip DB tests.
Line 47 always probes port 5432 instead of the port from DATABASE_URL. If CI/dev uses a non-default port, _db_available() returns false and Line 53-54 skips DB-backed modules even when DB is reachable.
Suggested fix
import asyncio
import os
+from urllib.parse import urlparse
import pytest
@@
def _db_available() -> bool:
@@
- host = (
- url.split("@")[-1].split("/")[0].split(":")[0]
- if "@" in url
- else "localhost"
- )
+ parsed = urlparse(url)
+ host = parsed.hostname or "localhost"
+ port = parsed.port or 5432
import socket
- socket.create_connection((host, 5432), timeout=2).close()
+ socket.create_connection((host, port), timeout=2).close()
return TrueAs per coding guidelines, backend/** reviews must analyze error handling and edge case coverage.
Also applies to: 53-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/conftest.py` around lines 33 - 48, _db_available() currently
hardcodes port 5432 when probing the DB host; parse the port from the
DATABASE_URL DSN and use it (falling back to 5432 if no port is present) when
calling socket.create_connection. Locate the url and host extraction logic in
_db_available(), extract the port from the DSN (e.g., after host:port or
default), convert to int with safe fallback, and replace the hardcoded 5432 with
the parsed port; also ensure you handle malformed ports by catching ValueError
and treating it as missing (use default).
| async def test_autocomplete_short_query(self, client, sample_bounties): | ||
| """Test autocomplete with short query.""" | ||
| response = await client.get("/api/bounties/autocomplete?q=a") | ||
|
|
||
| # Should work with minimum 2 characters | ||
| # Query "a" is too short, might return empty or validation error | ||
| assert response.status_code in [200, 422] | ||
|
|
There was a problem hiding this comment.
Make short-query autocomplete expectation deterministic.
Line 322 currently accepts both 200 and 422, which weakens contract validation and can hide regressions in request validation behavior. Pick one expected behavior and assert it consistently.
As per coding guidelines, backend/**: Python FastAPI backend. Analyze thoroughly: “Error handling and edge case coverage” and “API contract consistency with spec.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/test_bounty_api.py` around lines 316 - 323, The test
test_autocomplete_short_query currently allows either 200 or 422 which makes the
API contract nondeterministic; update the test to assert a single expected
behavior (preferably a 422 validation error) for the
/api/bounties/autocomplete?q=a request by replacing the loose check with an
explicit assertion like response.status_code == 422 and optionally assert the
JSON error shape (e.g., presence of "detail") to ensure consistent validation
behavior in the autocomplete endpoint.
| export function ThemeToggle({ theme, onToggle }: ThemeToggleProps) { | ||
| return ( | ||
| <button | ||
| type="button" | ||
| onClick={onToggle} | ||
| aria-label={`Switch to ${theme === 'light' ? 'dark' : 'light'} mode`} | ||
| className="rounded-lg p-2 text-gray-500 hover:text-gray-700 dark:text-gray-400 dark:hover:text-gray-200" | ||
| > | ||
| {theme === 'light' ? ( | ||
| <svg className="h-5 w-5" fill="none" viewBox="0 0 24 24" stroke="currentColor"> | ||
| <path strokeLinecap="round" strokeLinejoin="round" strokeWidth={2} d="M20.354 15.354A9 9 0 018.646 3.646 9.003 9.003 0 0012 21a9.003 9.003 0 008.354-5.646z" /> | ||
| </svg> | ||
| ) : ( | ||
| <svg className="h-5 w-5" fill="none" viewBox="0 0 24 24" stroke="currentColor"> | ||
| <path strokeLinecap="round" strokeLinejoin="round" strokeWidth={2} d="M12 3v1m0 16v1m9-9h-1M4 12H3m15.364 6.364l-.707-.707M6.343 6.343l-.707-.707m12.728 0l-.707.707M6.343 17.657l-.707.707M16 12a4 4 0 11-8 0 4 4 0 018 0z" /> | ||
| </svg> | ||
| )} | ||
| </button> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider enhancing accessibility with switch role semantics.
The button currently uses aria-label to indicate the action ("Switch to X mode"), which is functional but not optimal for a stateful toggle. For better semantic accessibility, consider using role="switch" with aria-checked to convey the current state rather than the pending action:
<button
type="button"
onClick={onToggle}
role="switch"
aria-checked={theme === 'dark'}
aria-label="Theme toggle"
className="rounded-lg p-2 text-gray-500 hover:text-gray-700 dark:text-gray-400 dark:hover:text-gray-200"
>This pattern aligns with WCAG guidelines for toggle controls by exposing the current state to assistive technologies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/layout/ThemeToggle.tsx` around lines 8 - 27, Update
the ThemeToggle component to expose switch semantics for assistive tech: in the
ThemeToggle function adjust the button element (inside ThemeToggle) to include
role="switch", set aria-checked to the current state (e.g. aria-checked={theme
=== 'dark'}), and change the aria-label to a neutral label like "Theme toggle"
while keeping the existing onClick/onToggle behavior and visual icons intact so
the control reports its state rather than only the intended action.
| const wallets = useMemo(() => [new PhantomWalletAdapter(), new SolflareWalletAdapter()], []); | ||
| const setNetwork = useCallback((n: SolanaNetwork) => setNetworkState(n), []); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect local usage paths for network switching and wallet adapter construction.
rg -n --type=tsx '\bsetNetwork\(' frontend/src
rg -n --type=tsx 'new\s+PhantomWalletAdapter|new\s+SolflareWalletAdapter' frontend/srcRepository: SolFoundry/solfoundry
Length of output: 120
🌐 Web query:
Does SolflareWalletAdapter(from@solana/wallet-adapter-wallets) bind network at construction time, and should wallet adapter instances be recreated when the selected cluster changes?
💡 Result:
Yes—SolflareWalletAdapter effectively “binds” the network at construction time (via its stored config), and you should recreate the adapter instance when the selected cluster changes.
- The adapter’s constructor stores the passed config object on the instance (
this._config = config). It does not expose any method to update the network later. [1] - When you call
connect(), it instantiates the Solflare SDK withnetwork: this._config.network. So the network value used for the wallet session comes from whatever was in the adapter config at the time the adapter instance was created. [2] - The package explicitly documents that the Solflare adapter supports “network configuration” via its constructor config. [3]
Implication / recommended pattern: if your app lets users switch clusters (e.g., devnet ↔ mainnet-beta), treat that as a configuration change that requires new adapter instances—typically by rebuilding your wallets array with useMemo(..., [network]) and (optionally) forcing a disconnect on change to avoid a network mismatch. [2]
Sources: [1] [2] [3]
🏁 Script executed:
#!/bin/bash
# Fix file type issue - try without type filter and with correct extensions
rg -n 'setNetwork' frontend/src --glob '*.tsx'
rg -n 'new\s+(Phantom|Solflare)WalletAdapter' frontend/src --glob '*.tsx'Repository: SolFoundry/solfoundry
Length of output: 1359
🏁 Script executed:
#!/bin/bash
# Read the WalletProvider.tsx file to understand context
cat frontend/src/components/wallet/WalletProvider.tsxRepository: SolFoundry/solfoundry
Length of output: 2260
🌐 Web query:
Does PhantomWalletAdapter from @solana/wallet-adapter-wallets require re-instantiation when the network/cluster changes?
💡 Result:
No. PhantomWalletAdapter does not need to be re-instantiated when your app switches Solana network/cluster.
- The adapter’s config is empty (
interface PhantomWalletAdapterConfig {}), so it doesn’t store a cluster/network internally. [1] - The cluster is effectively determined by the
Connectionyou pass at call time:sendTransaction(transaction, connection, ...)takes aConnectionparameter, so switching clusters just means using a differentConnection/RPC endpoint. [1][2]
In React, you typically keep the same PhantomWalletAdapter instance stable (e.g., useMemo) and only swap the endpoint/ConnectionProvider when the cluster changes. [2]
Sources: [1] [2]
SolflareWalletAdapter doesn't switch networks when user changes clusters—fix wallet dependencies.
Line 23 memoizes wallet instances once ([]), but SolflareWalletAdapter binds the network at construction time and does not expose a method to update it. When setNetwork changes the endpoint (line 22), the Solflare adapter instance is never recreated, creating a mismatch between the active network and the adapter's internal state. PhantomWalletAdapter is unaffected (its network is determined at call time via the Connection parameter).
Fix: Add network to the dependency array: const wallets = useMemo(() => [new PhantomWalletAdapter(), new SolflareWalletAdapter()], [network]); to ensure the adapter is re-instantiated when the cluster changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/wallet/WalletProvider.tsx` around lines 23 - 24, The
wallets array created with useMemo currently omits network from its dependency
array so SolflareWalletAdapter (constructed with the cluster) isn't re-created
when the cluster changes; update the useMemo call that defines wallets (the
variable wallets using useMemo and the adapters PhantomWalletAdapter and
SolflareWalletAdapter) to include network in its dependency array so a new
SolflareWalletAdapter is instantiated whenever network changes, ensuring adapter
state matches the current cluster.
| "types": ["react", "react-dom", "node"] | ||
| }, | ||
| "include": ["src"], | ||
| "exclude": ["src/**/*.test.tsx", "src/**/*.test.ts", "src/__tests__"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether tests are typechecked through a separate tsconfig/script
echo "== frontend package scripts related to typecheck/test =="
rg -n '"(typecheck|test|test:|lint|build)"\s*:' frontend/package.json
echo
echo "== all frontend tsconfig variants =="
fd "tsconfig.*\\.json" frontend
echo
echo "== references to test-specific tsconfig usage =="
rg -n 'tsc\s+(-p|--project)|tsconfig.*test|vitest.*typecheck|vue-tsc|ts-jest' frontend/package.json frontendRepository: SolFoundry/solfoundry
Length of output: 295
🏁 Script executed:
#!/bin/bash
# Read the full tsconfig.json to verify the exclude clause
echo "=== frontend/tsconfig.json content ==="
cat -n frontend/tsconfig.json
echo
echo "=== vitest configuration file(s) ==="
fd "vitest.*\\.config\\.(ts|js|mjs|json)" frontend
echo
echo "=== CI workflow files ==="
fd -e yaml -e yml . .github/workflows 2>/dev/null || echo "No GitHub workflows found"Repository: SolFoundry/solfoundry
Length of output: 988
🏁 Script executed:
#!/bin/bash
# Check for vitest config content if it exists
if [ -f "frontend/vitest.config.ts" ]; then
echo "=== vitest.config.ts content ==="
cat -n frontend/vitest.config.ts
elif [ -f "frontend/vitest.config.js" ]; then
echo "=== vitest.config.js content ==="
cat -n frontend/vitest.config.js
else
echo "No vitest config file found in frontend"
fi
echo
echo "=== Check root vitest config ==="
fd "vitest.*\\.config\\.(ts|js|mjs|json)" . --max-depth 1Repository: SolFoundry/solfoundry
Length of output: 138
🏁 Script executed:
#!/bin/bash
# Check for any test-related scripts in root package.json
echo "=== Root package.json test-related scripts ==="
cat package.json 2>/dev/null | jq '.scripts | with_entries(select(.key | contains("test") or contains("type")))' 2>/dev/null || echo "Could not parse package.json"
echo
echo "=== Check GitHub workflows for typecheck of tests ==="
if [ -d ".github/workflows" ]; then
grep -r "typecheck\|tsc\|vitest" .github/workflows --include="*.yml" --include="*.yaml" -n 2>/dev/null || echo "No typecheck references found"
fiRepository: SolFoundry/solfoundry
Length of output: 641
🏁 Script executed:
#!/bin/bash
# Get the full frontend-typecheck step from CI workflow
echo "=== Full frontend-typecheck step in ci.yml ==="
sed -n '118,150p' .github/workflows/ci.ymlRepository: SolFoundry/solfoundry
Length of output: 1003
Test files are excluded from TypeScript compilation — add test typecheck to CI.
The tsconfig.json excludes test files (src/**/*.test.ts/tsx, src/__tests__), and while CI has a frontend-typecheck step, it runs npx tsc --noEmit against this same config, skipping all test files. There is no separate test typecheck (no test-specific tsconfig or vitest --run --typecheck). This means test type errors will not be caught pre-merge, leaving type safety gaps in test code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/tsconfig.json` at line 15, CI currently runs the frontend-typecheck
job using the existing tsconfig.json which excludes test files, so test type
errors are never checked; add a dedicated test typecheck by creating a
tsconfig.tests.json that extends tsconfig.json but removes the test exclusions
(or explicitly includes "src/**/*.test.ts" and
"src/**/*.test.tsx"/"src/__tests__"), then update the frontend-typecheck CI step
to run npx tsc --project tsconfig.tests.json --noEmit (or alternately run vitest
with --run --typecheck) so tests are included in typechecking; reference
tsconfig.json, new tsconfig.tests.json, the frontend-typecheck CI job, and the
npx tsc/ vitest --typecheck command when making the change.
npm ci fails when lockfileVersion generated by npm 11 (Node 24) is consumed by npm 10 (Node 20 in CI). Fall back to npm install to ensure dependencies install regardless of npm version differences. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Issues found:
Suggestions:
⏰ 71 hours remaining out of 72h since your first submission. Please address these items and push an update. SolFoundry Review Bot |
1 participant
Description
Personal dashboard for logged-in contributors with full activity overview, earnings tracking, and settings management.
Closes #26
Features
Solana Wallet for Payout
Wallet:
97VihHW2Br7BKUU16c7RxjiEMHsD4dWisGDT2Y3LyJxFChecklist
Summary by CodeRabbit
New Features
UX Changes