Improvement: Github webhook, CORS, global exception exception handling in FastAPI returns JSON response. #216
Conversation
…to ensure all unhandled errors return a JSON response are improved
📝 WalkthroughWalkthroughAdds GitHub webhook signature verification and event mapping, makes CORS origins configurable via settings, registers a global exception handler, refactors frontend auth into a new Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Webhook
participant Route as /github/webhook
participant Verify as verify_signature
participant Config as Settings
participant Handler as Event Handler
GH->>Route: POST body + headers (X-GitHub-Event, X-Hub-Signature-256)
Route->>Verify: send body + X-Hub-Signature-256
Verify->>Config: read github_webhook_secret
alt secret empty
Verify-->>Route: verification skipped
else secret present
Verify->>Verify: HMAC-SHA256(body, secret)
Verify->>Route: compare to header
alt match
Verify-->>Route: verified
else mismatch or missing
Verify-->>Route: raise 401 Unauthorized
end
end
Route->>Route: parse JSON payload (handle parse errors)
Route->>Handler: map event/action -> EventType
alt PR closed & merged
Handler->>Handler: emit PR_MERGED
else mapped event
Handler->>Handler: emit mapped event
else unmapped
Handler-->>Route: log and ignore
end
Handler-->>GH: respond 200/appropriate status
sequenceDiagram
participant App as App Component
participant Hook as useAuth
participant Supa as Supabase Auth
participant Toast as Toast Notifier
App->>Hook: initialize/useAuth()
Hook->>Supa: supabase.auth.getSession()
Supa-->>Hook: session|null
Hook->>Hook: set isAuthenticated / loading
Hook->>Supa: supabase.auth.onAuthStateChange(subscribe)
alt SIGNED_IN
Supa-->>Hook: SIGNED_IN event
Hook->>Hook: set isAuthenticated=true
Hook->>Toast: show signed-in toast
else SIGNED_OUT
Supa-->>Hook: SIGNED_OUT event
Hook->>Hook: set isAuthenticated=false
Hook->>Toast: show signed-out toast
end
App->>Hook: call logout()
Hook->>Supa: supabase.auth.signOut()
Supa-->>Hook: result
Hook->>Hook: set isAuthenticated=false
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI Agents
In @backend/routes.py:
- Around line 1-9: The module uses the logging module and uuid.uuid4() but
neither is imported; add imports for logging and uuid at the top of the file
(e.g., insert "import logging" and "import uuid" alongside other imports) so
references to logging and uuid.uuid4() (used where events/IDs are created)
resolve at runtime.
- Around line 38-55: verify_signature currently consumes the request body with
await request.body(), which prevents github_webhook from later calling await
request.json(); fix by reading the body once in verify_signature (raw_body =
await request.body()), computing the HMAC against that raw_body, and then make
the same raw_body available to the downstream handler (e.g., attach it to
request.state.raw_body or set the cached body attribute so request.json() can
reuse it) and update github_webhook to parse the payload from that shared
raw_body (use json.loads on the bytes/decoded string) and add import json to
imports.
In @frontend/index.html:
- Line 6: The favicon reference in frontend/index.html (<link rel="icon"
type="image/svg+xml" href="/DevrAI.svg" />) points to a public asset but the
file is at the repo root; create the frontend/public directory (if missing) and
move DevrAI.svg into frontend/public/DevrAI.svg so Vite can serve it as
/DevrAI.svg; no HTML change required other than ensuring the file path matches
the href.
In @frontend/src/components/contributors/ContributorsPage.tsx:
- Line 12: The contributors local state is typed as any[] in ContributorsPage;
update it to use the proper RepositoryData contributors type (e.g.,
useState<RepositoryData["contributors"]>(repoData?.contributors || [])) or
remove the local state entirely and read contributors directly from repoData;
adjust the state setter usages (setContributors) and any derived variables to
match the new typed shape and import RepositoryData from your types file so
TypeScript enforces the correct contributor type.
In @frontend/src/components/dashboard/Dashboard.tsx:
- Around line 53-58: The trend prop currently mirrors the absolute count
(repoData.pull_requests?.open) — change it to a percentage change: compute
current = repoData.pull_requests?.open || 0 and previous =
repoData.pull_requests?.previous_open (or a historical field you have); if
previous > 0 set trend = Math.round(((current - previous) / previous) * 100)
else fallback to 0 (or a placeholder) and pass that number to the StatCard trend
prop; ensure you guard against missing previous data and divide-by-zero so the
StatCard receives a meaningful percentage instead of the raw count.
In @frontend/src/components/pages/AnalyticsPage.tsx:
- Around line 117-121: The useState hooks for selectedRange/setSelectedRange and
activeIndex/setActiveIndex are declared after an early return, violating the
Rules of Hooks; move the declarations for React.useState('Last Week')
(selectedRange/setSelectedRange) and React.useState(0)
(activeIndex/setActiveIndex) to the top of the AnalyticsPage component (so they
run unconditionally on every render, before the conditional that checks repoData
and returns the "No data available" message), then keep the existing conditional
return that checks repoData.pull_requests.
In @frontend/src/types/index.ts:
- Line 30: The contributors property in the RepositoryData type is declared as
contributors?: any[] which removes type safety; define a proper Contributor
interface (e.g., with fields like id, login, avatarUrl, contributions) above
RepositoryData and replace any[] with Contributor[] (update the RepositoryData
type and any usages such as ContributorsPage to use the new Contributor type) so
consumers get strong typing and autocompletion.
🧹 Nitpick comments (7)
backend/app/core/config/settings.py (1)
17-17: Consider documenting the security implications of an empty webhook secret.When
github_webhook_secretis empty, webhook signature verification is skipped inroutes.py. This is acceptable for local development but poses a security risk in production. Consider adding a comment or emitting a warning log at startup when this field is empty.backend/main.py (1)
117-125: Consider usingJSONResponsefor cleaner code.The global exception handler correctly logs unhandled exceptions and returns a JSON error. The
requestparameter is required by FastAPI's exception handler signature, so the static analysis hint can be ignored.Consider using
JSONResponseinstead of manually constructing the response:🔎 Proposed refactor
+from fastapi.responses import JSONResponse + @api.exception_handler(Exception) async def global_exception_handler(request, exc): """Log all unhandled exceptions and return a JSON error response.""" logger.error(f"Unhandled exception: {exc!s}", exc_info=True) - return Response( - content='{"detail": "Internal Server Error"}', - status_code=500, - media_type="application/json" - ) + return JSONResponse( + status_code=500, + content={"detail": "Internal Server Error"} + )backend/routes.py (1)
71-91: Good refactor to event mapping; consider adding missing GitHub event actions.The event-mapping dictionary is a cleaner pattern than if/elif chains. However, some common GitHub webhook actions are not mapped:
issues:reopened(could map toISSUE_UPDATED)pull_request:synchronize(pushed new commits),reopened,ready_for_reviewpull_request_review:submitted(forPR_REVIEWEDwhich exists inEventType)These may be intentionally omitted, but worth confirming.
frontend/src/components/contributors/ContributorsPage.tsx (1)
12-18: Simplify by removing redundant local state.The
contributorsstate is initialized fromrepoData?.contributorsand the useEffect simply copies the same data when repoData changes. Since this component doesn't mutate contributors, you can userepoData.contributorsdirectly in the render, eliminating the state and useEffect.🔎 Proposed simplification
const ContributorsPage: React.FC<Props> = ({ repoData }) => { - const [contributors, setContributors] = useState<any[]>(repoData?.contributors || []); - - useEffect(() => { - if (repoData?.contributors) { - setContributors(repoData.contributors); - } - }, [repoData]); const handleExportReport = () => { toast.success('Generating contributor report...');Then update the render to use
repoData.contributorsdirectly:<div className="grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 gap-6"> - {contributors.map((contributor) => ( + {(repoData.contributors || []).map((contributor) => ( <ContributorCardfrontend/src/hooks/useAuth.ts (1)
10-17: Reconsider showing error toast for initial session check.Showing a toast error when the session check fails (line 12) may be too noisy. On initial page load, a missing session is expected for unauthenticated users and isn't necessarily an error condition. Consider only showing the toast for actual errors (network issues, etc.) rather than missing sessions.
🔎 Proposed adjustment
supabase.auth.getSession().then(({ data, error }) => { if (error) { - toast.error('Session check failed'); console.error('Error checking session:', error); + // Only show toast for actual errors, not missing sessions } setIsAuthenticated(!!data?.session); setLoading(false); });frontend/src/components/dashboard/Dashboard.tsx (1)
59-70: Consider replacing hardcoded stat values with actual data or placeholders.The "Community Posts" (line 62) and "Response Rate" (line 68) stat cards use hardcoded string values. While acceptable as temporary placeholders, consider either:
- Deriving these from actual repository data when available
- Using a more obvious placeholder like "N/A" or "–"
- Adding a TODO comment to track the implementation
frontend/src/App.tsx (1)
28-34: Remove the unusedhandleLogincallback and updateLoginPageto not require theonLoginprop.The
handleLoginfunction is a no-op since auth state is already managed by theuseAuthhook, which automatically listens to Supabase authentication state changes viaonAuthStateChange. When a user successfully logs in, Supabase triggers aSIGNED_INevent that the hook processes, making theonLogin()call inLoginPageredundant.🔎 Proposed fix
Remove the callback from App.tsx:
- const handleLogin = () => { - // Auth state is handled by the useAuth hook listener - };Update the route:
- <LoginPage onLogin={handleLogin} /> + <LoginPage />Update LoginPage.tsx to remove the optional prop and its call:
interface LoginPageProps { - onLogin: () => void; } export default function LoginPage({ onLogin }: LoginPageProps) {And remove the
onLogin()call:- onLogin(); navigate('/');
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/Devr logo.pngis excluded by!**/*.png
📒 Files selected for processing (12)
backend/app/core/config/settings.pybackend/main.pybackend/routes.pyfrontend/index.htmlfrontend/src/App.tsxfrontend/src/components/contributors/ContributorsPage.tsxfrontend/src/components/dashboard/Dashboard.tsxfrontend/src/components/dashboard/StatCard.tsxfrontend/src/components/pages/AnalyticsPage.tsxfrontend/src/components/pages/PullRequestsPage.tsxfrontend/src/hooks/useAuth.tsfrontend/src/types/index.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-28T23:14:21.089Z
Learnt from: smokeyScraper
Repo: AOSSIE-Org/Devr.AI PR: 87
File: backend/app/api/v1/health.py:1-12
Timestamp: 2025-06-28T23:14:21.089Z
Learning: In the Devr.AI project, the application is designed to run from the backend directory, making import paths like `from main import DevRAIApplication` correct for the current setup. Import path adjustments for deployment will be configured later during the deployment process.
Applied to files:
frontend/index.html
🧬 Code graph analysis (6)
frontend/src/hooks/useAuth.ts (1)
frontend/src/lib/supabaseClient.ts (1)
supabase(12-12)
frontend/src/components/pages/AnalyticsPage.tsx (1)
frontend/src/types/index.ts (1)
RepositoryData(15-36)
backend/routes.py (4)
backend/app/core/events/event_bus.py (1)
EventBus(10-54)backend/app/core/events/enums.py (2)
EventType(10-32)PlatformType(3-8)backend/app/core/events/base.py (1)
BaseEvent(5-25)backend/app/core/handler/handler_registry.py (1)
HandlerRegistry(6-38)
frontend/src/components/pages/PullRequestsPage.tsx (1)
frontend/src/types/index.ts (1)
RepositoryData(15-36)
frontend/src/components/dashboard/Dashboard.tsx (2)
frontend/src/types/index.ts (1)
RepositoryData(15-36)frontend/src/components/dashboard/StatCard.tsx (1)
StatCard(11-43)
frontend/src/components/contributors/ContributorsPage.tsx (1)
frontend/src/types/index.ts (1)
RepositoryData(15-36)
🪛 Ruff (0.14.10)
backend/routes.py
67-67: Undefined name logging
(F821)
115-115: Undefined name logging
(F821)
backend/main.py
118-118: Unused function argument: request
(ARG001)
120-120: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (18)
frontend/index.html (1)
1-16: LGTM! Clean HTML5 boilerplate.The HTML structure follows best practices for a React/Vite application with proper meta tags, charset declaration, viewport configuration, and module script loading.
backend/app/core/config/settings.py (1)
43-49: LGTM! Sensible defaults for local development.The CORS origins configuration with localhost defaults is appropriate for development, and loading from environment variables allows production customization.
backend/main.py (1)
110-110: LGTM! Dynamic CORS configuration.Using
settings.allow_originsinstead of hardcoded values enables environment-specific configuration.backend/routes.py (1)
93-103: LGTM! PR merge detection logic is correct.The special handling for
pull_request+closedaction correctly checks themergedflag to distinguish between merged and simply closed PRs.frontend/src/components/dashboard/StatCard.tsx (1)
7-7: LGTM! Type widening appropriately supports numeric stat values.The update to accept
string | numberfor the value prop is appropriate and aligns with the Dashboard usage where numeric values like contributor counts and PR counts are passed. The change is backward compatible since string values are still supported.frontend/src/components/dashboard/Dashboard.tsx (1)
15-20: LGTM! Improved null state UX.The enhanced null state with the Activity icon provides better visual feedback compared to a plain message.
frontend/src/hooks/useAuth.ts (1)
37-37: Pattern is correct per Supabase API documentation.The
subscription.subscription.unsubscribe()call is the correct way to unsubscribe. Because the code destructures with an alias (const { data: subscription } = ...), the variablesubscriptionholds what the API returns asdata, which contains a nestedsubscriptionproperty. This matches the Supabase v2.53.0 API exactly and is equivalent to the documented patterndata.subscription.unsubscribe().Likely an incorrect or invalid review comment.
frontend/src/App.tsx (3)
20-26: Good refactoring to centralize auth logic.The introduction of
useAuthhook cleanly encapsulates authentication state management. The destructuredisAuthenticated,loading, andlogoutprovide clear intent.
36-38: LGTM!The logout handler correctly delegates to the hook's
logout()function with proper async/await handling.
48-49: Minor formatting change.The className template literal formatting is adjusted but behavior is preserved.
frontend/src/components/pages/AnalyticsPage.tsx (4)
34-34: Good type improvement.Replacing
anywithRepositoryData | nullimproves type safety and aligns with the shared type definitions infrontend/src/types/index.ts.Also applies to: 112-114
165-168: Minor formatting adjustment.The template literal formatting change preserves the same behavior.
332-337: Good practice using_for unused parameter.Using underscore prefix signals the parameter is intentionally unused while still accessing the index for color cycling.
348-370: SimplifiedrenderActiveShaperenders only labels.The function now only renders the center text labels (name, percentage, value) without the Sector shape. This relies on Recharts' Pie component to render the actual slice via its default
activeShapebehavior combined with this custom rendering.Verify this renders correctly in the browser - the active pie slice should still highlight properly with the labels appearing in the center.
frontend/src/components/pages/PullRequestsPage.tsx (4)
3-3: Good type alignment with shared type definitions.Using
RepositoryDatafrom centralized types ensures consistency across components like Dashboard, AnalyticsPage, and ContributorsPage.Also applies to: 5-7
11-18: Good defensive coding with styled fallback UI.The null check and empty array fallback ensure safe rendering. The placeholder message provides clear user guidance.
45-89: Well-structured table with accessibility and UX improvements.The table includes:
- Semantic table markup with proper headers
- External links with
rel="noopener noreferrer"for security- Hover transitions for interactivity
- Color-coded status badges
62-85: No issue found. ThePullRequesttype definesauthoras a required property with requiredlogin,avatar_url, andprofile_urlstring fields. TypeScript's type system ensures these properties always exist at runtime, making null checks unnecessary.
| <StatCard | ||
| icon={<GitPullRequest size={24} />} | ||
| title="Open PRs" | ||
| value={repoData.pull_requests?.open || 0} | ||
| trend={repoData.pull_requests?.open || 0} | ||
| /> |
There was a problem hiding this comment.
Fix trend calculation to represent growth percentage, not absolute value.
The trend prop is set to repoData.pull_requests?.open || 0 (line 57), which is the same as the value prop (line 56). The trend should represent a percentage change (e.g., +12% increase) rather than the absolute count. This likely produces incorrect or misleading trend indicators in the UI.
🔎 Suggested approach
Either:
- Calculate the actual trend based on historical data (preferred), or
- Use a placeholder percentage like the other StatCard instances until historical data is available:
<StatCard
icon={<GitPullRequest size={24} />}
title="Open PRs"
value={repoData.pull_requests?.open || 0}
- trend={repoData.pull_requests?.open || 0}
+ trend={8}
/>📝 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.
| <StatCard | |
| icon={<GitPullRequest size={24} />} | |
| title="Open PRs" | |
| value={repoData.pull_requests?.open || 0} | |
| trend={repoData.pull_requests?.open || 0} | |
| /> | |
| <StatCard | |
| icon={<GitPullRequest size={24} />} | |
| title="Open PRs" | |
| value={repoData.pull_requests?.open || 0} | |
| trend={8} | |
| /> |
🤖 Prompt for AI Agents
In @frontend/src/components/dashboard/Dashboard.tsx around lines 53-58, The
trend prop currently mirrors the absolute count (repoData.pull_requests?.open) —
change it to a percentage change: compute current = repoData.pull_requests?.open
|| 0 and previous = repoData.pull_requests?.previous_open (or a historical field
you have); if previous > 0 set trend = Math.round(((current - previous) /
previous) * 100) else fallback to 0 (or a placeholder) and pass that number to
the StatCard trend prop; ensure you guard against missing previous data and
divide-by-zero so the StatCard receives a meaningful percentage instead of the
raw count.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/lib/supabaseClient.ts (1)
7-12: Critical: Revert to throwing an error for missing Supabase configuration.Replacing the error throw with
console.warnallows the app to proceed with an invalid Supabase client (empty URL and key). This will cause runtime failures when any auth method is invoked, making debugging harder. The original behavior of throwing an error was correct—it fails fast and clearly indicates a configuration problem at startup.🔎 Proposed fix to restore fail-fast behavior
if (!supabaseUrl || !supabaseAnonKey) { - console.warn('Missing Supabase configuration. Please set VITE_SUPABASE_URL and VITE_SUPABASE_KEY environment variables.'); + throw new Error('Missing Supabase configuration. Please set VITE_SUPABASE_URL and VITE_SUPABASE_KEY environment variables.'); }
🤖 Fix all issues with AI Agents
In @backend/routes.py:
- Around line 41-57: The helper function verify_signature incorrectly uses
Header(None) for x_hub_signature_256 (dependency injection only works in route
handlers); change verify_signature to accept x_hub_signature_256 as a normal
parameter (e.g., x_hub_signature_256: str = None) and remove the Header
import/annotation, then update the github_webhook route to pass the extracted
header value into verify_signature when calling it so the header is validated
correctly.
- Around line 71-74: The JSON decode except block should preserve the original
exception chain: change the except to "except json.JSONDecodeError as err:" and
re-raise the HTTPException with "from err" (e.g., raise
HTTPException(status_code=400, detail="Invalid JSON payload") from err) so the
original json.loads/body.decode error is chained for traceability.
- Around line 96-98: Remove the invalid "pull_request_comment" mapping from the
event-to-EventType dispatch dictionary (the entry that maps
"pull_request_comment" -> EventType.PR_COMMENTED); delete that key/value so the
code no longer tries to match a non-existent GitHub webhook event. Also search
for any code paths or tests that reference EventType.PR_COMMENTED and either
remove/replace them with the correct event types (e.g., "issue_comment",
"pull_request_review_comment", or "pull_request_review") as appropriate.
In @frontend/src/App.tsx:
- Around line 23-41: ProtectedLayout currently types props as any; define a
proper props interface (e.g., ProtectedLayoutProps) that declares isSidebarOpen:
boolean, setIsSidebarOpen: React.Dispatch<React.SetStateAction<boolean>> (or
(val:boolean)=>void if you prefer), and handleLogout: () => void, then update
the component signature to use that interface (e.g., const ProtectedLayout:
React.FC<ProtectedLayoutProps> = ({ ... }) => ...) so type checking propagates
to the Sidebar props (isOpen, setIsOpen, onLogout) and removes the any.
In @frontend/src/components/pages/LoginPage.tsx:
- Line 70: In the LoginPage component update the toast error string in the
toast.error call (the expression using error?.message || "An Unknown error
occured!") to correct the typo and normalize casing: change "occured" to
"occurred" and "Unknown" to "unknown" so the fallback reads "An unknown error
occurred!".
- Line 50: The component signature for LoginPage currently uses an unnecessary
empty object destructuring in "function LoginPage({ }: LoginPageProps)"; remove
the empty destructuring and change the declaration to accept no parameters
(e.g., "function LoginPage(): JSX.Element" or "function LoginPage(props:
LoginPageProps)" if you plan to keep props) so the static analysis warning is
resolved; update the export default LoginPage declaration accordingly and remove
the unused LoginPageProps reference if you opt for no parameters.
🧹 Nitpick comments (4)
frontend/src/types/index.ts (2)
1-13: Good type definitions; consider strengthening PullRequest.state.The PullRequest interface is well-structured. To improve type safety, consider making the
statefield a union type (e.g.,"open" | "closed" | "merged") rather than a generic string.🔎 Optional: Use a union type for state
export interface PullRequest { number: number; title: string; author: { login: string; avatar_url: string; profile_url: string; }; - state: string; + state: "open" | "closed" | "merged"; url: string; comments?: number; created_at?: string; }
4-7: Consider aligning Contributor with PullRequest.author structure.The
PullRequest.authorobject includes aprofile_urlfield, but theContributorinterface does not. If contributors and PR authors represent similar entities, adding an optionalprofile_url?: stringto Contributor would improve consistency.🔎 Optional: Add profile_url to Contributor
export interface Contributor { login: string; avatar_url: string; contributions: number; + profile_url?: string; }Also applies to: 15-19
frontend/src/hooks/useAuth.ts (1)
10-16: Consider showing session check errors to users.Session check errors are logged to the console but not displayed to users via toast. While this may be intentional to avoid alarming users on initial load, consider whether certain error types (e.g., network failures) warrant user notification.
🔎 Optional: Add user-facing error notification
supabase.auth.getSession().then(({ data, error }) => { if (error) { console.error('Error checking session:', error); + toast.error('Failed to check authentication status'); } setIsAuthenticated(!!data?.session); setLoading(false); });frontend/src/components/pages/AnalyticsPage.tsx (1)
349-371: Consider adding visual feedback to the active pie segment.The
renderActiveShapefunction currently only displays text labels (name, percentage, value) without highlighting the active segment. Adding a visual element like aSectorcomponent alongside the text would provide better user feedback on hover.If text-only feedback is intentional, this can be disregarded. Otherwise, consider restoring visual highlighting by importing
Sectorfrom 'recharts' and rendering it along with the text elements.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/public/DevrAI.svgis excluded by!**/*.svg
📒 Files selected for processing (9)
backend/routes.pyfrontend/src/App.tsxfrontend/src/components/contributors/ContributorsPage.tsxfrontend/src/components/dashboard/Dashboard.tsxfrontend/src/components/pages/AnalyticsPage.tsxfrontend/src/components/pages/LoginPage.tsxfrontend/src/hooks/useAuth.tsfrontend/src/lib/supabaseClient.tsfrontend/src/types/index.ts
🧰 Additional context used
🧬 Code graph analysis (6)
frontend/src/App.tsx (2)
frontend/src/types/index.ts (1)
RepositoryData(21-42)frontend/src/hooks/useAuth.ts (1)
useAuth(5-50)
frontend/src/hooks/useAuth.ts (2)
frontend/src/lib/supabaseClient.ts (1)
supabase(12-12)backend/app/services/auth/supabase.py (1)
logout(40-49)
backend/routes.py (4)
backend/app/core/events/event_bus.py (1)
EventBus(10-54)backend/app/core/events/enums.py (2)
EventType(10-32)PlatformType(3-8)backend/app/core/events/base.py (1)
BaseEvent(5-25)backend/app/core/handler/handler_registry.py (1)
HandlerRegistry(6-38)
frontend/src/components/pages/LoginPage.tsx (1)
frontend/src/lib/supabaseClient.ts (1)
supabase(12-12)
frontend/src/components/pages/AnalyticsPage.tsx (1)
frontend/src/types/index.ts (1)
RepositoryData(21-42)
frontend/src/components/contributors/ContributorsPage.tsx (1)
frontend/src/types/index.ts (1)
RepositoryData(21-42)
🪛 Biome (2.1.2)
frontend/src/components/pages/LoginPage.tsx
[error] 50-50: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
🪛 Ruff (0.14.10)
backend/routes.py
74-74: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (18)
backend/routes.py (3)
1-12: Imports look good - past issues resolved.All necessary imports (
logging,uuid,json,hmac,hashlib) have been added to support the new webhook signature verification and event mapping functionality.
65-70: Body handling correctly addresses past review concerns.Reading the body once (line 66) and reusing it for both signature verification and JSON parsing correctly resolves the stream exhaustion issue flagged in previous reviews.
101-124: Event mapping and dispatching logic is well-structured.The event_mapping dictionary approach (lines 80-99) provides a cleaner, more maintainable alternative to if/elif chains. The special case for PR merges (lines 106-110) correctly handles GitHub's webhook payload structure where merged PRs send
action="closed"withmerged=true.frontend/src/types/index.ts (1)
15-19: LGTM! Contributor type successfully addresses the past review comment.The Contributor interface properly types contributors and eliminates the
any[]usage flagged in the previous review.frontend/src/hooks/useAuth.ts (2)
40-47: LGTM! Logout function handles errors appropriately.The logout function properly handles errors with user feedback via toast and updates the authentication state on success.
18-37: The subscription cleanup pattern is correct.The code uses
subscription.subscription.unsubscribe()becausesubscriptionis the destructureddataobject fromonAuthStateChange, and.subscriptionis the property containing the actual subscription object. This matches the Supabase v2.53.0 documentation pattern for unsubscribing from auth state changes.frontend/src/components/pages/LoginPage.tsx (1)
54-72: LGTM! Login flow aligns with the new useAuth pattern.The removal of the
onLogin()callback invocation is intentional and correct—authentication state is now managed centrally by theuseAuthhook, and navigation to the home page is handled directly here.frontend/src/components/contributors/ContributorsPage.tsx (2)
1-12: Excellent refactoring! Types and data flow improved.This change successfully addresses the past review comments by:
- Typing
repoDataasRepositoryData | nullinstead ofany- Deriving
contributorsdirectly fromrepoDatawith proper typing (automatically inferred asContributor[])- Removing unnecessary local state management
The component is now simpler and type-safe.
22-24: LGTM! Good use of early return guard.The early return when
repoDatais null provides a clear guard clause and improves readability.frontend/src/components/pages/AnalyticsPage.tsx (2)
34-34: LGTM! Type safety improved.The import of
RepositoryDataand the prop type change fromanytoRepositoryData | nullimprove type safety and align with the PR's objective to remove widespreadanyusage.Also applies to: 113-113
117-122: Excellent fix! Rules of Hooks violation resolved.Moving the
useStatehooks before the conditional return resolves the critical issue flagged in the previous review. Hooks are now called unconditionally in the same order on every render, as required by React.frontend/src/components/dashboard/Dashboard.tsx (4)
7-7: LGTM! Type safety improved.The import of
RepositoryDataand the prop type change fromanytoRepositoryData | nullenhance type safety and align with the PR's goal to eliminate widespreadanyusage.Also applies to: 10-10
14-21: Nice UX improvement for empty state.The enhanced null state UI with a centered layout and Activity icon provides better visual feedback when no repository data is available.
53-58: Good progress on trend calculation.The trend value is now a placeholder percentage (8) instead of mirroring the absolute count, which addresses the issue raised in the previous review. While this is still a placeholder rather than a calculated growth percentage, it's an acceptable interim solution that provides meaningful UI feedback.
76-93: No action required. TheBotIntegrationcomponent no longer has aconnectedprop and correctly manages connection status internally via theisConnectedstate. The component fetches integration status on mount (line 31-32) using API calls and updates its UI accordingly (lines 107-112). The three instances in Dashboard.tsx are using the component correctly without passing aconnectedprop.frontend/src/App.tsx (3)
20-20: Excellent refactoring with useAuth hook.The integration of the
useAuthhook successfully centralizes authentication logic, simplifiesApp.tsx, and properly handles the loading state with appropriate UI feedback. This aligns well with the PR's objective to create a customuseAuthhook for Supabase authentication.Also applies to: 46-46, 48-54
21-21: LGTM! RepositoryData type properly applied.The import and usage of
RepositoryDatatype for therepoDatastate improves type safety and maintains consistency with the typed props passed to child components.Also applies to: 45-45
68-68: LoginPage successfully handles authentication without theonLoginprop.The
LoginPagecomponent handles authentication internally via direct Supabase calls (supabase.auth.signInWithPassword). TheonLoginprop is already optional in the component signature and is not used anywhere in the implementation. The component manages login state and post-login navigation independently, so removal of the prop has no impact.
backend/routes.py
Outdated
| async def verify_signature(body: bytes, x_hub_signature_256: str = Header(None)): | ||
| """Verify that the payload was sent from GitHub by validating the SHA256 signature.""" | ||
| if not settings.github_webhook_secret: | ||
| # If secret is not configured, we skip verification (not recommended for production) | ||
| return | ||
|
|
||
| if not x_hub_signature_256: | ||
| raise HTTPException(status_code=401, detail="X-Hub-Signature-256 header is missing") | ||
|
|
||
| signature = hmac.new( | ||
| settings.github_webhook_secret.encode(), | ||
| body, | ||
| hashlib.sha256 | ||
| ).hexdigest() | ||
|
|
||
| if not hmac.compare_digest(f"sha256={signature}", x_hub_signature_256): | ||
| raise HTTPException(status_code=401, detail="Invalid webhook signature") |
There was a problem hiding this comment.
Incorrect use of Header() in helper function.
The Header(None) dependency annotation on line 41 only works in FastAPI route handlers where dependency injection is active. In a helper function called manually (line 69), it will not extract the header value as intended and may cause runtime errors.
The header is already correctly extracted in the github_webhook route handler (line 63). Simply pass it as a regular parameter.
🔎 Proposed fix
-async def verify_signature(body: bytes, x_hub_signature_256: str = Header(None)):
+async def verify_signature(body: bytes, x_hub_signature_256: str | None):
"""Verify that the payload was sent from GitHub by validating the SHA256 signature."""
if not settings.github_webhook_secret:
# If secret is not configured, we skip verification (not recommended for production)
return📝 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.
| async def verify_signature(body: bytes, x_hub_signature_256: str = Header(None)): | |
| """Verify that the payload was sent from GitHub by validating the SHA256 signature.""" | |
| if not settings.github_webhook_secret: | |
| # If secret is not configured, we skip verification (not recommended for production) | |
| return | |
| if not x_hub_signature_256: | |
| raise HTTPException(status_code=401, detail="X-Hub-Signature-256 header is missing") | |
| signature = hmac.new( | |
| settings.github_webhook_secret.encode(), | |
| body, | |
| hashlib.sha256 | |
| ).hexdigest() | |
| if not hmac.compare_digest(f"sha256={signature}", x_hub_signature_256): | |
| raise HTTPException(status_code=401, detail="Invalid webhook signature") | |
| async def verify_signature(body: bytes, x_hub_signature_256: str | None): | |
| """Verify that the payload was sent from GitHub by validating the SHA256 signature.""" | |
| if not settings.github_webhook_secret: | |
| # If secret is not configured, we skip verification (not recommended for production) | |
| return | |
| if not x_hub_signature_256: | |
| raise HTTPException(status_code=401, detail="X-Hub-Signature-256 header is missing") | |
| signature = hmac.new( | |
| settings.github_webhook_secret.encode(), | |
| body, | |
| hashlib.sha256 | |
| ).hexdigest() | |
| if not hmac.compare_digest(f"sha256={signature}", x_hub_signature_256): | |
| raise HTTPException(status_code=401, detail="Invalid webhook signature") |
🤖 Prompt for AI Agents
In @backend/routes.py around lines 41 - 57, The helper function verify_signature
incorrectly uses Header(None) for x_hub_signature_256 (dependency injection only
works in route handlers); change verify_signature to accept x_hub_signature_256
as a normal parameter (e.g., x_hub_signature_256: str = None) and remove the
Header import/annotation, then update the github_webhook route to pass the
extracted header value into verify_signature when calling it so the header is
validated correctly.
backend/routes.py
Outdated
| try: | ||
| payload = json.loads(body.decode()) | ||
| except json.JSONDecodeError: | ||
| raise HTTPException(status_code=400, detail="Invalid JSON payload") |
There was a problem hiding this comment.
Add exception chaining for better error traceability.
When re-raising or raising a new exception within an except clause, use raise ... from err to preserve the exception chain, or raise ... from None to explicitly suppress it.
🔎 Proposed fix
try:
payload = json.loads(body.decode())
except json.JSONDecodeError:
- raise HTTPException(status_code=400, detail="Invalid JSON payload")
+ raise HTTPException(status_code=400, detail="Invalid JSON payload") from NoneBased on static analysis hints.
📝 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.
| try: | |
| payload = json.loads(body.decode()) | |
| except json.JSONDecodeError: | |
| raise HTTPException(status_code=400, detail="Invalid JSON payload") | |
| try: | |
| payload = json.loads(body.decode()) | |
| except json.JSONDecodeError: | |
| raise HTTPException(status_code=400, detail="Invalid JSON payload") from None |
🧰 Tools
🪛 Ruff (0.14.10)
74-74: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In @backend/routes.py around lines 71 - 74, The JSON decode except block should
preserve the original exception chain: change the except to "except
json.JSONDecodeError as err:" and re-raise the HTTPException with "from err"
(e.g., raise HTTPException(status_code=400, detail="Invalid JSON payload") from
err) so the original json.loads/body.decode error is chained for traceability.
| { | ||
| toast.error(error?.message ||"An Unknown error occured!"); | ||
| else { | ||
| toast.error(error?.message || "An Unknown error occured!"); |
There was a problem hiding this comment.
Fix typo in error message.
"occured" should be spelled "occurred".
🔎 Proposed fix
- toast.error(error?.message || "An Unknown error occured!");
+ toast.error(error?.message || "An unknown error occurred!");Note: Also changed "Unknown" to lowercase for consistency.
📝 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.
| toast.error(error?.message || "An Unknown error occured!"); | |
| toast.error(error?.message || "An unknown error occurred!"); |
🤖 Prompt for AI Agents
In @frontend/src/components/pages/LoginPage.tsx at line 70, In the LoginPage
component update the toast error string in the toast.error call (the expression
using error?.message || "An Unknown error occured!") to correct the typo and
normalize casing: change "occured" to "occurred" and "Unknown" to "unknown" so
the fallback reads "An unknown error occurred!".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @backend/routes.py:
- Line 62: The x_github_event header parameter is currently optional
(Header(None)); make it required by replacing Header(None) with Header(...) in
the route signature where x_github_event: str is declared (so FastAPI will
validate and return a 422 when the header is missing), ensuring the webhook
handler parameter name x_github_event is updated in that function signature and
any callers/tests adjusted if needed.
- Around line 43-45: When the code hits the condition "if not
settings.github_webhook_secret" it currently returns silently; add a
warning-level log immediately before the return to surface that signature
verification is being skipped (e.g., logger.warning or app.logger.warning with a
clear message like "GitHub webhook secret not configured; skipping signature
verification — insecure for production."). Update the webhook handler code where
the settings.github_webhook_secret check lives to emit this warning and include
identifying context (environment or endpoint) using the existing logger
instance, then return as before.
🧹 Nitpick comments (2)
frontend/src/App.tsx (1)
52-60: Clean integration with the useAuth hook.The loading state prevents rendering protected routes before authentication status is resolved. The
handleLogoutwrapper currently just delegates tologout—you could passlogoutdirectly if no additional logic is planned, but keeping the wrapper is fine if you anticipate adding side effects (e.g., toast notifications) later.backend/routes.py (1)
89-92: Consider adding a comment about the special PR closed handling.The
pull_requestmapping intentionally excludes the"closed"action because it receives special handling below (lines 103-108 distinguish between merged and non-merged closes). A brief comment would help future maintainers understand this design choice.🔎 Proposed addition
"pull_request": { "opened": EventType.PR_CREATED, "edited": EventType.PR_UPDATED, + # Note: "closed" action is handled specially below (lines 103-108) + # to distinguish merged vs. non-merged closes },
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/routes.pyfrontend/src/App.tsxfrontend/src/components/pages/LoginPage.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/pages/LoginPage.tsx (1)
frontend/src/lib/supabaseClient.ts (1)
supabase(12-12)
🔇 Additional comments (7)
frontend/src/App.tsx (4)
20-21: LGTM!Clean imports for the centralized
useAuthhook andRepositoryDatatype, aligning well with the PR's objective to remove inline type definitions and simplify authentication handling.
23-27: Good fix addressing the previous review feedback.The
ProtectedLayoutPropsinterface properly types the component props, replacing the previousanyusage. Type definitions are accurate—handleLogoutcorrectly returnsPromise<void>matching the async implementation.
29-47: LGTM!Well-structured
ProtectedLayoutcomponent with proper TypeScript typing. The layout correctly handles sidebar state transitions and usesAnimatePresencefor smooth route animations.
95-107: LGTM!The protected route pattern is correctly implemented using
Navigatewithreplacefor unauthenticated users. Props are properly passed toProtectedLayoutfor sidebar and logout functionality.frontend/src/components/pages/LoginPage.tsx (1)
47-47: LGTM! Empty destructuring issue resolved.The function signature has been correctly updated to remove the unnecessary empty object destructuring, addressing the previous review feedback.
backend/routes.py (2)
1-12: LGTM! All required imports are present.The previously flagged missing imports for
logginganduuidhave been correctly added. All imports are properly organized and necessary for the webhook handling functionality.
98-122: LGTM! Event resolution and dispatch logic is sound.The event mapping lookup, special PR merged handling, and fallback logging are all implemented correctly. The code safely handles missing keys and unmatched events, and the distinction between merged and non-merged PR closes is intentional and well-implemented.
| if not settings.github_webhook_secret: | ||
| # If secret is not configured, we skip verification (not recommended for production) | ||
| return |
There was a problem hiding this comment.
Add warning log when signature verification is skipped.
Silently skipping signature verification creates a security risk. If accidentally deployed to production without a configured secret, the endpoint will accept unauthenticated webhook requests from any source.
🔎 Proposed fix
if not settings.github_webhook_secret:
# If secret is not configured, we skip verification (not recommended for production)
+ logging.warning("GitHub webhook secret not configured - signature verification skipped. This is insecure for production!")
return🤖 Prompt for AI Agents
In @backend/routes.py around lines 43 - 45, When the code hits the condition "if
not settings.github_webhook_secret" it currently returns silently; add a
warning-level log immediately before the return to surface that signature
verification is being skipped (e.g., logger.warning or app.logger.warning with a
clear message like "GitHub webhook secret not configured; skipping signature
verification — insecure for production."). Update the webhook handler code where
the settings.github_webhook_secret check lives to emit this warning and include
identifying context (environment or endpoint) using the existing logger
instance, then return as before.
| logging.info(f"Received GitHub event: {event_header}") | ||
| async def github_webhook( | ||
| request: Request, | ||
| x_github_event: str = Header(None), |
There was a problem hiding this comment.
Validate required webhook headers explicitly.
The x_github_event header can be None, but there's no explicit validation. While the code handles None gracefully downstream (it won't match any event mapping), validating required headers upfront provides clearer error messages and better API hygiene.
🔎 Proposed fix
async def github_webhook(
request: Request,
- x_github_event: str = Header(None),
+ x_github_event: str = Header(...),
x_hub_signature_256: str = Header(None)
):Using Header(...) instead of Header(None) makes x_github_event required, and FastAPI will automatically return a clear 422 error if the header is missing.
🤖 Prompt for AI Agents
In @backend/routes.py at line 62, The x_github_event header parameter is
currently optional (Header(None)); make it required by replacing Header(None)
with Header(...) in the route signature where x_github_event: str is declared
(so FastAPI will validate and return a 422 when the header is missing), ensuring
the webhook handler parameter name x_github_event is updated in that function
signature and any callers/tests adjusted if needed.
1 participant
…to ensure all unhandled errors return a JSON response are improved
Closes #
📝 Description
This Pull Request focuses on improving code quality, security, and maintainability across both the backend and frontend. It also stabilizes the frontend development environment to ensure a smoother and more reliable developer experience.
Key areas addressed include backend security hardening, standardized error handling, frontend type safety, and overall cleanup/refactoring for long-term scalability.
🔧 Changes Made
Backend Improvements
if/elifwebhook routing logic with a clean event-mapping strategy for improved readability and extensibility.Frontend Improvements
module not founderrors.src/types/index.ts:RepositoryDataPullRequestanyusage:useAuthhook to encapsulate Supabase authentication logic, simplifyingApp.tsx.StatCardto support both numeric and string values.BotIntegrationby removing redundant prop passing.📷 Screenshots or Visual Changes (if applicable)
N/A — no visual UI changes introduced.
🤝 Collaboration
Collaborated with: (none)
✅ Checklist
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.