refactor: extract GitHub DB ops to service layer (OPE-80)#261
Merged
DevanshuNEU merged 2 commits intoFeb 24, 2026
Merged
Conversation
…vices/github_connections.py Routes should orchestrate HTTP flow, not query databases directly. Extracted 4 functions from routes/github.py to services/github_connections.py: - get_connection(user_id) -- fetch GitHub OAuth connection - save_connection(...) -- upsert OAuth token + user info - delete_connection(user_id) -- remove connection - update_last_used(user_id) -- update last_used_at timestamp Functions changed from async to sync (Supabase client is sync). Callers in routes/github.py updated: dropped await, renamed to service function names. routes/github.py: 360 -> 297 lines (-63) services/github_connections.py: 75 lines (new) 289 tests pass. Closes OPE-80
|
@DevanshuNEU is attempting to deploy a commit to the Dev's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis refactoring extracts GitHub OAuth database operations from route handlers into a dedicated service layer. The new GitHub Connections Service provides four helper functions for managing GitHub authentication data, while the route module is simplified to delegate persistence logic to this service. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Collaborator
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/services/github_connections.py (1)
74-75:update_last_usedswallows all exceptions silently — add at least a debug-level log.
except Exception: passdiscards every failure with no trace in logs or metrics. A transient network error or schema mismatch during alast_used_atupdate will be completely invisible, making future debugging difficult.♻️ Proposed fix
except Exception: - pass + logger.error("Failed to update last_used_at", user_id=user_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/github_connections.py` around lines 74 - 75, The except Exception: pass in update_last_used silently swallows all errors; modify that except block in the update_last_used function to catch Exception as e and log the error (include e and stack trace) at debug or error level instead of passing — e.g., use the module's logger (or processLogger) to record a clear message like "failed to update last_used_at" with the exception details so failures are visible for debugging.
🤖 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/services/github_connections.py`:
- Around line 1-75: Wrap the module-level functions (get_connection,
save_connection, delete_connection, update_last_used) into a singleton service
class (e.g., class GitHubConnectionsService) that provides those methods, move
the existing logic into instance methods, and create a module-level singleton
instance (e.g., github_connections_service = GitHubConnectionsService()) for
callers to import; keep internal calls to get_supabase_service and logger
unchanged and preserve method signatures but as instance methods so code follows
the same singleton pattern used by dependency_analyzer.py.
- Around line 13-75: The four functions get_connection, save_connection,
delete_connection, and update_last_used perform blocking Supabase network I/O
and must be converted to async: change each signature to async and wrap the
blocking DB calls (the calls to get_supabase_service().client and the subsequent
table(...).select()/upsert()/delete()/update().execute()) in asyncio.to_thread
(or asyncio.get_event_loop().run_in_executor(None, ...)) so the DB round-trips
run off the event loop; return/raise the same values as before and then update
all call sites (e.g., routes/github.py) to await these async functions. Ensure
exception logging behavior is preserved when wrapping the blocking calls.
- Line 16: The file currently performs repeated inline imports of
get_supabase_service inside functions (seen in save_connection,
delete_connection, update_last_used and another spot); hoist a single import
statement "from services.supabase_service import get_supabase_service" to the
top-level module imports and remove the deferred inline imports from those
functions so they use the module-level symbol instead, keeping behavior
identical but improving readability and following standard practice.
---
Nitpick comments:
In `@backend/services/github_connections.py`:
- Around line 74-75: The except Exception: pass in update_last_used silently
swallows all errors; modify that except block in the update_last_used function
to catch Exception as e and log the error (include e and stack trace) at debug
or error level instead of passing — e.g., use the module's logger (or
processLogger) to record a clear message like "failed to update last_used_at"
with the exception details so failures are visible for debugging.
1. Hoisted 4 identical inline 'from services.supabase_service import get_supabase_service' to module-level. No circular import issue. 2. update_last_used: silent 'except: pass' now logs at debug level so timestamp update failures are visible. Skipped: singleton class (stateless module, zero instance state), async conversion (5th+ time, sync Supabase client). 289 tests pass.
Collaborator
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
backend/services/github_connections.py (2)
14-72: 🛠️ Refactor suggestion | 🟠 MajorUnresolved service pattern mismatch: wrap these functions in the project’s singleton service shape.
This new service still exposes bare module-level functions instead of the singleton class pattern used by other services (e.g.,
dependency_analyzer.py).♻️ Suggested structure
+class GitHubConnectionsService: + def get_connection(self, user_id: str) -> Optional[dict]: + ... + def save_connection(self, ...) -> bool: + ... + def delete_connection(self, user_id: str) -> bool: + ... + def update_last_used(self, user_id: str) -> None: + ... + +_github_connections_service: Optional[GitHubConnectionsService] = None + +def get_github_connections_service() -> GitHubConnectionsService: + global _github_connections_service + if _github_connections_service is None: + _github_connections_service = GitHubConnectionsService() + return _github_connections_serviceAs per coding guidelines: "All new services should follow the singleton pattern used in
dependency_analyzer.py" (backend/services/**/*.py).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/github_connections.py` around lines 14 - 72, Convert these module-level functions (get_connection, save_connection, delete_connection, update_last_used) into methods on a singleton service class (e.g., GitHubConnectionsService) following the pattern used in dependency_analyzer.py: move DB access (get_supabase_service().client) and the try/except logic into instance methods, provide a classmethod like get_instance() or module-level INSTANCE to enforce a single instance, and export that instance for callers to use instead of the bare functions; preserve signatures and return values but call the methods via the singleton (e.g., GitHubConnectionsService.get_instance().get_connection(...)) so the service conforms to the project's singleton service shape.
14-72:⚠️ Potential issue | 🟠 MajorBlocking DB I/O in sync helpers is executed on async request paths.
Line 18, Line 46, Line 57, and Line 70 perform synchronous Supabase
.execute()calls. These functions are then invoked inside async FastAPI handlers, so DB round-trips can block the event loop under load.#!/bin/bash set -euo pipefail echo "1) Service signatures (expect async defs for I/O operations):" rg -nP '^\s*def\s+(get_connection|save_connection|delete_connection|update_last_used)\s*\(' backend/services/github_connections.py rg -nP '^\s*async\s+def\s+(get_connection|save_connection|delete_connection|update_last_used)\s*\(' backend/services/github_connections.py || true echo echo "2) Async route handlers calling these helpers (awaited should be True if helpers are async):" python - <<'PY' import ast, pathlib targets = {"get_connection","save_connection","delete_connection","update_last_used"} p = pathlib.Path("backend/routes/github.py") tree = ast.parse(p.read_text()) parent = {} for n in ast.walk(tree): for c in ast.iter_child_nodes(n): parent[c] = n for fn in [n for n in tree.body if isinstance(n, ast.AsyncFunctionDef)]: for n in ast.walk(fn): if isinstance(n, ast.Call) and isinstance(n.func, ast.Name) and n.func.id in targets: awaited = isinstance(parent.get(n), ast.Await) print(f"{fn.name}: Line {n.lineno} -> {n.func.id} (awaited={awaited})") PYAs per coding guidelines: "Use async/await for I/O operations in Python backend" (
backend/**/*.py).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/github_connections.py` around lines 14 - 72, The four helpers (get_connection, save_connection, delete_connection, update_last_used) perform blocking Supabase .execute() calls on async request paths; update each to be non-blocking by making the functions async (async def ...) and moving DB round-trips off the event loop—either by switching to an async Supabase client and awaiting the .execute() calls or by wrapping the current sync calls with a thread executor (e.g., asyncio.to_thread or FastAPI's run_in_threadpool) so the .table(...).select/...upsert/...delete/...update(...).execute() calls do not block; keep the same return values and exception handling but await the threaded/async call in the updated async functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/services/github_connections.py`:
- Around line 14-72: Convert these module-level functions (get_connection,
save_connection, delete_connection, update_last_used) into methods on a
singleton service class (e.g., GitHubConnectionsService) following the pattern
used in dependency_analyzer.py: move DB access (get_supabase_service().client)
and the try/except logic into instance methods, provide a classmethod like
get_instance() or module-level INSTANCE to enforce a single instance, and export
that instance for callers to use instead of the bare functions; preserve
signatures and return values but call the methods via the singleton (e.g.,
GitHubConnectionsService.get_instance().get_connection(...)) so the service
conforms to the project's singleton service shape.
- Around line 14-72: The four helpers (get_connection, save_connection,
delete_connection, update_last_used) perform blocking Supabase .execute() calls
on async request paths; update each to be non-blocking by making the functions
async (async def ...) and moving DB round-trips off the event loop—either by
switching to an async Supabase client and awaiting the .execute() calls or by
wrapping the current sync calls with a thread executor (e.g., asyncio.to_thread
or FastAPI's run_in_threadpool) so the
.table(...).select/...upsert/...delete/...update(...).execute() calls do not
block; keep the same return values and exception handling but await the
threaded/async call in the updated async functions.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
routes/github.py had 4 database helper functions (
_get_github_connection,_save_github_connection,_delete_github_connection,_update_last_used) directly callingget_supabase_service().client.table(...). Routes should orchestrate, not query databases.Fix
New
services/github_connections.py(75 lines) with clean public API:get_connection(user_id)save_connection(...)delete_connection(user_id)update_last_used(user_id)Functions changed from async to sync -- Supabase client is sync, the async wrappers were misleading.
Changes
2 files changed. 289 tests pass.
Closes OPE-80
Summary by CodeRabbit