This pull request addresses the dependency vulnerabilities described in #115 #116
Conversation
📝 WalkthroughWalkthroughFrontend: app-level ThemeProvider added and HomePage/components updated for theme-aware styling; minor Button class change. Backend: ai routes converted to per-request async Supabase/Gemini access with runtime env validation; startup registers optional AsyncClient. Tests and .gitignore entries added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as HTTP Client
participant Server as FastAPI (trending_niches)
participant Supabase as Supabase (AsyncClient)
participant Gemini as Gemini API
Client->>Server: GET /ai/trending_niches
Server->>Server: get_supabase_client(request)
alt Supabase config present
Server->>Supabase: (async) select cached niches
Supabase-->>Server: niches result
end
alt Need external data
Server->>Server: fetch_from_gemini(...) via asyncio.to_thread
Server->>Gemini: POST /generate (API key from get_gemini_api_key())
Gemini-->>Server: generated content
Server->>Supabase: (async) insert/upsert fetched data
Supabase-->>Server: insert result
end
Server-->>Client: 200 OK (niches payload or fallback)
Note right of Server: Missing envs -> raise HTTPException(500)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 0
🧹 Nitpick comments (4)
Frontend/src/App.tsx (1)
3-3: Remove uncertainty comment or verify the import pathThe comment "Make sure path is correct" suggests uncertainty. Please verify the import path is correct and remove this comment once confirmed.
-import { ThemeProvider } from "./components/theme-provider"; // Make sure path is correct +import { ThemeProvider } from "./components/theme-provider";Frontend/src/pages/HomePage.tsx (3)
135-136: Apply theme styling to loading and error statesThe loading and error states should also respect the current theme for consistency.
- if (loading) return <div>Loading trending niches...</div>; - if (error) return <div>Error: {error}</div>; + if (loading) return <div className={theme === 'dark' ? 'text-gray-300' : 'text-gray-700'}>Loading trending niches...</div>; + if (error) return <div className={theme === 'dark' ? 'text-red-400' : 'text-red-600'}>Error: {error}</div>;
254-926: Consider extracting theme-aware className utilitiesThe HomePage component has extensive repeated conditional className patterns for theme handling. Consider creating utility functions or custom hooks to reduce repetition and improve maintainability.
Example utility that could be added to a separate file:
// utils/theme-classes.ts export const getThemeClasses = (theme: string, darkClasses: string, lightClasses: string) => { return theme === 'dark' ? darkClasses : lightClasses; }; // Or as a custom hook export const useThemeClasses = () => { const { theme } = useTheme(); return (darkClasses: string, lightClasses: string) => theme === 'dark' ? darkClasses : lightClasses; };This would simplify the many conditional class applications throughout the component.
26-27: Clean up commented importRemove the commented supabase import if it's no longer needed.
-// import { supabase } from "../utils/supabase"; import { useTheme } from "../components/theme-provider";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
Frontend/src/App.tsx(2 hunks)Frontend/src/components/mode-toggle.tsx(2 hunks)Frontend/src/pages/HomePage.tsx(20 hunks)
🔇 Additional comments (2)
Frontend/src/components/mode-toggle.tsx (1)
17-19: Good fix for positioning context!Adding the
relativeclass to the Button is necessary since the Moon icon uses absolute positioning. This ensures the absolutely positioned Moon icon is positioned relative to the button rather than a distant ancestor.Frontend/src/App.tsx (1)
48-140: Verify alignment with PR objectivesThe PR objectives mention fixing dependency vulnerabilities via
npm audit fix, but these changes introduce theme management functionality instead. Please clarify if this is the intended change for this PR or if these changes belong to a different feature branch.Likely an incorrect or invalid review comment.
|
i complete one more issue #117 in which i fixed dark mode toggle functionality before its look like this and now |
|
✅ This PR now addresses two issues: Fixes env handling for AI routes (no import crash, lazy Supabase client, clear 500s) — refs #263. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.gitignore (1)
29-32: Optional: Consider more comprehensive env patterns.For better future-proofing, consider adding patterns for common env file variations:
# Env files .env Backend/.env Frontend/.env +.env.local +.env.*.local +.env.production.localThis helps protect against local overrides and environment-specific files that developers might create during development.
Backend/test_ai_fix.py (3)
40-44: CatchHTTPExceptionspecifically instead of bareException.While the test works, catching
HTTPExceptiondirectly would be more precise and aligns with static analysis best practices.🔎 Proposed refactor
+ from fastapi import HTTPException + # Test 1: Try to get Supabase client without env vars print("\nTest 1: get_supabase_client() without SUPABASE_URL/KEY...") try: client = ai.get_supabase_client() print(" ✗ FAIL: Should have raised HTTPException") - except Exception as e: + except HTTPException as e: if "Supabase configuration missing" in str(e): print(f" ✓ PASS: Raised HTTPException with message: {e}") else: print(f" ✗ FAIL: Wrong exception: {type(e).__name__}: {e}")
51-55: CatchHTTPExceptionspecifically instead of bareException.Same as Test 1, catching
HTTPExceptiondirectly would improve precision and follow static analysis recommendations.🔎 Proposed refactor
# Test 2: Try to get Gemini key without env var print("\nTest 2: get_gemini_api_key() without GEMINI_API_KEY...") try: key = ai.get_gemini_api_key() print(" ✗ FAIL: Should have raised HTTPException") - except Exception as e: + except HTTPException as e: if "Gemini API key not configured" in str(e): print(f" ✓ PASS: Raised HTTPException with message: {e}") else: print(f" ✗ FAIL: Wrong exception: {type(e).__name__}: {e}")
65-69: Consider catching specific exceptions for clarity.While the bare
Exceptioncatch works here (since various connection errors are expected), being more specific would improve test clarity.🔎 Proposed refactor
# Test 3: Set env vars and verify client creation would proceed print("\nTest 3: Setting env vars and retrying get_supabase_client()...") os.environ['SUPABASE_URL'] = 'https://test.supabase.co' os.environ['SUPABASE_KEY'] = 'test_key_123' try: # Note: This will fail when trying to actually connect, but the env check passes client = ai.get_supabase_client() print(" - Got to client creation (would connect to real Supabase now)") - except Exception as e: + except HTTPException as e: + # Should not happen - env vars are set + print(f" ✗ FAIL: Still failing on env check: {e}") + except Exception as e: + # Expected: connection or other errors after env validation passes if "Supabase configuration missing" not in str(e): print(f" ✓ PASS: Passed env check, failed on actual connection: {type(e).__name__}") else: print(f" ✗ FAIL: Still failing on env check: {e}")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignoreBackend/app/routes/ai.pyBackend/test_ai_fix.py
🧰 Additional context used
🪛 Ruff (0.14.10)
Backend/test_ai_fix.py
40-40: Do not catch blind exception: Exception
(BLE001)
51-51: Do not catch blind exception: Exception
(BLE001)
65-65: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (7)
.gitignore (1)
29-32: ✓ Environment files properly ignored.The addition of
.env,Backend/.env, andFrontend/.envis necessary and appropriate given the PR's introduction of environment-based configuration for secrets (SUPABASE_URL, SUPABASE_KEY, GEMINI_API_KEY). These patterns prevent accidental commits of sensitive data.Backend/app/routes/ai.py (5)
4-4: LGTM!The
lru_cacheimport is appropriate for caching the Supabase client initialization.
15-20: LGTM!The defensive validation pattern with clear error messages is well-implemented. Moving configuration validation to per-request ensures the API can start even with missing environment variables.
29-33: LGTM!The validation is consistent with the defensive pattern established for Supabase configuration. Not caching the API key retrieval is appropriate since it's a simple environment variable read.
39-40: LGTM!The per-request API key retrieval is consistent with the lazy validation pattern and ensures proper error handling when the key is missing.
75-75: LGTM!The cached client retrieval maintains performance while enabling per-request validation. The existing fallback logic (lines 90-93) is preserved correctly.
Backend/test_ai_fix.py (1)
1-29: LGTM!The test correctly verifies that the module can be imported without environment variables, addressing the core requirement of moving validation from import-time to per-request.
Backend/app/routes/ai.py
Outdated
| @lru_cache(maxsize=1) | ||
| def get_supabase_client() -> Client: | ||
| url, key = _get_supabase_config() | ||
| return create_client(url, key) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Supabase Python client thread-safe concurrent requests FastAPI
💡 Result:
Short answer: don’t create a new Supabase client per FastAPI request and don’t mutate a single global client’s session for each request. Instead, create one (async) Supabase client once at app startup and reuse it; for per-user auth send the user’s access token with each request (so the client does per-request auth via headers) or call server-side endpoints with a service_role key where appropriate. This avoids client-creation overhead and race conditions from swapping session state.
Why and how (concise):
- supabase-py supports an AsyncClient and docs show creating the client once (use the async client for async frameworks). Reuse that client for concurrent requests. [1]
- Creating a client per request is slow (client creation can be expensive) and switching a global client’s session concurrently causes races and inconsistent behavior. See Supabase discussion and issues where people report performance problems and race conditions when making per-request clients or mutating the global session. [2][3]
- For per-user requests with RLS:
- Preferred: keep one global client and include the user’s access token in the request to Supabase (Authorization: Bearer <access_token>) / use the PostgREST endpoint with that header so Postgres checks RLS for that user. That avoids calling client.auth.set_session on a shared client. [2][3]
- Alternative: perform server-side actions with the service_role key (only for trusted server-only operations) or implement a short-lived per-request client if you accept the creation overhead (not recommended). [2][3]
Quick FastAPI pattern (concept only):
- create client at startup, attach to app state, inject via dependency
- for each request, read user's access token and forward it in the Authorization header when calling Supabase APIs (or set request-specific headers on the HTTPX session used by supabase-py if supported)
References:
- Supabase Python docs (async client / usage). [1]
- Supabase community discussion about FastAPI usage, performance, and session-switching race conditions. [2]
- Fast-start/docs examples and notes on client initialization / not creating many clients. [3]
If you want, I can:
- show a minimal FastAPI startup + dependency code example that reuses an async Supabase client and forwards per-request Authorization headers, or
- inspect your current code and adapt it for safe concurrency.
🏁 Script executed:
# Check the context of the ai.py file and how get_supabase_client is used
find . -name "ai.py" -type f | head -5Repository: AOSSIE-Org/InPactAI
Length of output: 89
🏁 Script executed:
# Search for usages of get_supabase_client throughout the codebase
rg "get_supabase_client" --type pyRepository: AOSSIE-Org/InPactAI
Length of output: 612
🏁 Script executed:
# Check the imports and full implementation of ai.py
head -40 Backend/app/routes/ai.pyRepository: AOSSIE-Org/InPactAI
Length of output: 1623
🏁 Script executed:
# Search for FastAPI app initialization and startup patterns
rg "FastAPI|app.*=" --type py -B2 -A2 | head -60Repository: AOSSIE-Org/InPactAI
Length of output: 2332
🏁 Script executed:
# Check if get_supabase_client is used with await (async context)
rg "await.*get_supabase_client|get_supabase_client.*await" --type pyRepository: AOSSIE-Org/InPactAI
Length of output: 45
🏁 Script executed:
# Look for async def functions that call get_supabase_client
rg -B3 "get_supabase_client" --type py | grep -E "^[^-].*async def|get_supabase_client"Repository: AOSSIE-Org/InPactAI
Length of output: 612
🏁 Script executed:
# Find where supabase = get_supabase_client() is used to see if it's in async context
rg -B5 -A5 "supabase = get_supabase_client" Backend/app/routes/ai.pyRepository: AOSSIE-Org/InPactAI
Length of output: 552
🏁 Script executed:
# Check if there are any async functions in ai.py that use this client
rg "async def" Backend/app/routes/ai.py -A10Repository: AOSSIE-Org/InPactAI
Length of output: 45
🏁 Script executed:
# Get the full context of where get_supabase_client is called - look for the function definition
rg -B10 "supabase = get_supabase_client()" Backend/app/routes/ai.py | head -30Repository: AOSSIE-Org/InPactAI
Length of output: 466
🏁 Script executed:
# Check what the actual endpoint functions look like
rg "@router\." Backend/app/routes/ai.py -A8 | head -50Repository: AOSSIE-Org/InPactAI
Length of output: 428
🏁 Script executed:
# Check the Client type imported - is it from supabase-py (synchronous) or supabase (async)?
rg "from supabase" Backend/app/routes/ai.pyRepository: AOSSIE-Org/InPactAI
Length of output: 105
Initialize Supabase client at app startup instead of lazy-loading with @lru_cache.
The lru_cache approach does prevent per-request client creation, but it initializes the client on first use rather than at app startup. FastAPI best practice (as shown in your lifespan context in main.py) is to initialize the Supabase client once at startup and store it in app state. This avoids potential initialization delays on the first request and follows established patterns.
Additionally, consider using Supabase's AsyncClient instead of the synchronous Client, since FastAPI is designed for async operations and the endpoint can benefit from async/await patterns.
🤖 Prompt for AI Agents
In Backend/app/routes/ai.py around lines 23 to 26, the Supabase client is being
lazily created via @lru_cache which defers initialization until first use;
replace this pattern by initializing a Supabase AsyncClient at application
startup and storing it on FastAPI's app.state (remove the lru_cache factory),
update imports and any call sites to use the AsyncClient from request.app.state
(or inject the app) so endpoints can await async operations, and ensure the
client is properly closed on shutdown in main.py's lifespan handler; also remove
synchronous Client usage and switch to Supabase's AsyncClient throughout.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Backend/app/main.py (1)
48-53: Cleanup logic is solid but could be simplified.The defensive checks are appropriate for lifecycle management. However, since
app.state.supabaseis controlled by your startup code and is guaranteed to be eitherNoneor anAsyncClientinstance (which hasaclose), you could simplify:if app.state.supabase: await app.state.supabase.aclose()That said, the current defensive approach is perfectly acceptable and more resilient to future changes.
Backend/app/routes/ai.py (1)
59-87: Excellent async conversion with proper error handling.The endpoint has been correctly converted to use async/await patterns:
- All Supabase AsyncClient operations are properly awaited
- Synchronous
fetch_from_geminiis appropriately wrapped inasyncio.to_thread()- Fallback strategy gracefully handles Gemini API failures
The broad
Exceptioncatch at line 83 (flagged by static analysis) is acceptable here since the fallback pattern needs to handle any Gemini-related failure. However, if you prefer more specific error handling, you could catchrequests.RequestExceptionandjson.JSONDecodeErrorexplicitly.🔎 Optional: More specific exception handling
- except Exception as e: + except (requests.RequestException, json.JSONDecodeError, KeyError) as e: print("Gemini fetch failed:", e) # fallback: serve most recent data result = await supabase.table("trending_niches").select("*").order("fetched_at", desc=True).limit(6).execute()This catches the most likely failure modes while still providing fallback behavior.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Backend/app/main.pyBackend/app/routes/ai.py
🧰 Additional context used
🧬 Code graph analysis (1)
Backend/app/main.py (1)
Frontend/src/utils/supabase.tsx (1)
supabase(11-11)
🪛 Ruff (0.14.10)
Backend/app/routes/ai.py
83-83: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (3)
Backend/app/routes/ai.py (3)
15-19: Excellent implementation of per-request client retrieval!This follows FastAPI best practices by:
- Using dependency injection via the
Requestobject- Providing clear error messages when configuration is missing
- Returning appropriate HTTP 500 status for server configuration issues
This addresses the previous review feedback about replacing
@lru_cachewith the app.state pattern.
22-26: LGTM: Consistent error handling for missing API key.The implementation mirrors the pattern used in
get_supabase_client, providing clear server-side configuration error messages.
28-57: Good integration of API key validation.The function now properly validates the Gemini API key via
get_gemini_api_key()instead of assuming a global variable exists. The synchronous nature of this function is acceptable since it's wrapped inasyncio.to_thread()at the call site (line 74).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Backend/app/main.py
🧰 Additional context used
🧬 Code graph analysis (1)
Backend/app/main.py (2)
Frontend/src/utils/supabase.tsx (1)
supabase(11-11)Backend/app/db/seed.py (1)
seed_db(6-57)
🪛 Ruff (0.14.10)
Backend/app/main.py
44-44: Redundant exception object included in logging.exception call
(TRY401)
🔇 Additional comments (3)
Backend/app/main.py (3)
15-15: LGTM!The
AsyncClientimport is correctly added to enable async Supabase client functionality used in the startup lifespan.
40-44: Past review concern addressed.The try-except block around
create_client()properly addresses the previous review comment requesting exception handling. The broadExceptioncatch withapp.state.supabase = Nonefallback ensures startup doesn't crash on initialization failures, which is appropriate for the non-fatal design.
52-57: LGTM!The shutdown cleanup logic properly handles the async Supabase client lifecycle. Using
getattrwith defaults provides defensive access, and awaitingaclose()ensures proper resource cleanup.
| app.state.supabase = create_client(supabase_url, supabase_key, supabase_cls=AsyncClient) | ||
| except Exception as exc: # broad to avoid startup crash; logs include stack | ||
| app.state.supabase = None | ||
| logging.exception("Failed to initialize Supabase AsyncClient: %s", exc) |
There was a problem hiding this comment.
Remove redundant exception parameter from logging.exception.
The logging.exception() method automatically includes the exception traceback and message, so passing exc as a format parameter is redundant.
🔎 Proposed fix
- logging.exception("Failed to initialize Supabase AsyncClient: %s", exc)
+ logging.exception("Failed to initialize Supabase AsyncClient")📝 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.
| logging.exception("Failed to initialize Supabase AsyncClient: %s", exc) | |
| logging.exception("Failed to initialize Supabase AsyncClient") |
🧰 Tools
🪛 Ruff (0.14.10)
44-44: Redundant exception object included in logging.exception call
(TRY401)
🤖 Prompt for AI Agents
In Backend/app/main.py around line 44, the call to logging.exception currently
passes the caught exception object as a format parameter which is redundant
because logging.exception automatically includes the current exception info;
remove the extra `exc` parameter and call logging.exception with just the
message ("Failed to initialize Supabase AsyncClient") so the traceback and
exception message are logged once correctly.
1 participant
This pull request updates project dependencies to address multiple security vulnerabilities that were reported during installation.
After running npm audit fix, all reported vulnerabilities were resolved, and the project now shows 0 vulnerabilities.
🔧 Changes Made
Ran npm audit fix to automatically apply safe dependency updates
Added 4 packages, removed 2 packages, updated 16 packages
Verified that no vulnerabilities remain (npm audit reports found 0 vulnerabilities)
Confirmed the project builds and runs successfully after dependency updates
before
after this change
Summary by CodeRabbit
New Features
Style
Chores
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.