[feat]: add github authentication using !verify_github command #80
Conversation
WalkthroughThe changes introduce explicit Supabase configuration fields, refactor Supabase client initialization to use a centralized settings class, and update Supabase OAuth authentication functions to be asynchronous. Discord bot functionality is expanded with a new GitHub verification command, which utilizes a new interactive OAuth button view for user authentication. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DiscordBot
participant SupabaseAuth
User->>DiscordBot: !verify_github command
DiscordBot->>SupabaseAuth: login_with_github()
SupabaseAuth-->>DiscordBot: OAuth URL
DiscordBot->>User: Sends embed with OAuth button (OAuthView)
User->>SupabaseAuth: Clicks button, authenticates with GitHub
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (2)
backend/bots/discord/discord_cogs.py (1)
48-74:login_with_github()is synchronous under the hood – it will block the event-loop
supabase_client.auth.sign_in_with_oauthis a blocking HTTP call. Wrapping it in anasyncdef without off-loading still freezes the bot while waiting.- oauth_result = await login_with_github() + oauth_result = await asyncio.to_thread(login_with_github)(Requires
import asyncio).Alternatively, move the HTTP call itself into
asyncio.to_threadinsidelogin_with_oauth.
Without this, the bot cannot process other events during the network round-trip.backend/app/db/supabase/auth.py (1)
21-27: Same blocking issue for logout
sign_outis also synchronous – wrap inasyncio.to_threadto avoid blocking, and chain the exception.
🧹 Nitpick comments (2)
backend/bots/discord/discord_views.py (1)
6-16: Minor: mark the OAuth message as user-only to avoid link leaking
ctx.send(..., view=view)in the cog sends a public message in guild channels, so anybody could click the generated OAuth URL.
Consider setting the message to ephemeral for slash-commands or DM the user to restrict visibility.No code change required here (handled at the command call-site) – just flagging the risk.
backend/bots/discord/discord_cogs.py (1)
58-66: Useephemeral=Trueto keep the verification link privateAnyone in the channel can press the link button. If you intend the link to be user-specific, send it as an ephemeral interaction response or DM.
- await ctx.send(embed=embed, view=view) + await ctx.author.send(embed=embed, view=view)(This is just one option; slash-commands with
ctx.respond(ephemeral=True, …)work too.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/app/core/config.py(1 hunks)backend/app/db/supabase/auth.py(1 hunks)backend/app/db/supabase/supabase_client.py(1 hunks)backend/bots/discord/discord_cogs.py(2 hunks)backend/bots/discord/discord_views.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/db/supabase/supabase_client.py (1)
backend/app/core/config.py (1)
Settings(6-36)
🪛 Pylint (3.3.7)
backend/bots/discord/discord_views.py
[refactor] 3-3: Too few public methods (0/2)
(R0903)
🪛 Ruff (0.11.9)
backend/app/db/supabase/auth.py
10-10: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
| from app.core.config import Settings | ||
| from supabase import create_client | ||
|
|
||
| load_dotenv() | ||
| settings = Settings() | ||
|
|
||
| SUPABASE_URL = os.getenv("SUPABASE_URL") | ||
| SUPABASE_KEY = os.getenv("SUPABASE_KEY") | ||
|
|
||
| if SUPABASE_URL is None or SUPABASE_KEY is None: | ||
| raise ValueError("SUPABASE_URL and SUPABASE_KEY must be set in environment variables.") | ||
| SUPABASE_URL = settings.supabase_url | ||
| SUPABASE_KEY = settings.supabase_key | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reuse the singleton settings object and fail fast when credentials are missing
A second Settings() instantiation does IO and may silently pick up different env values than the global instance in core.config. It also allows the app to continue with empty creds.
-from app.core.config import Settings
+from app.core.config import settings
@@
-settings = Settings()
-
-SUPABASE_URL = settings.supabase_url
-SUPABASE_KEY = settings.supabase_key
+SUPABASE_URL = settings.supabase_url
+SUPABASE_KEY = settings.supabase_key
+
+if not SUPABASE_URL or not SUPABASE_KEY:
+ raise RuntimeError("Supabase URL / Key not configured – check environment variables")This keeps configuration consistent and surfaces deployment errors immediately.
📝 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.
| from app.core.config import Settings | |
| from supabase import create_client | |
| load_dotenv() | |
| settings = Settings() | |
| SUPABASE_URL = os.getenv("SUPABASE_URL") | |
| SUPABASE_KEY = os.getenv("SUPABASE_KEY") | |
| if SUPABASE_URL is None or SUPABASE_KEY is None: | |
| raise ValueError("SUPABASE_URL and SUPABASE_KEY must be set in environment variables.") | |
| SUPABASE_URL = settings.supabase_url | |
| SUPABASE_KEY = settings.supabase_key | |
| from app.core.config import settings | |
| from supabase import create_client | |
| SUPABASE_URL = settings.supabase_url | |
| SUPABASE_KEY = settings.supabase_key | |
| if not SUPABASE_URL or not SUPABASE_KEY: | |
| raise RuntimeError("Supabase URL / Key not configured – check environment variables") |
🤖 Prompt for AI Agents
In backend/app/db/supabase/supabase_client.py lines 1 to 8, avoid creating a new
Settings() instance locally; instead, import and reuse the existing singleton
Settings object from app.core.config to ensure consistent configuration. Add a
check right after loading SUPABASE_URL and SUPABASE_KEY to verify they are not
empty or None, and if they are, raise an error immediately to fail fast and
prevent the app from running with missing credentials.
smokeyScraper
force-pushed
the
auth_discord
branch
from
80b22db to
7236056
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/bots/discord/discord_views.py (2)
6-17: Consider disabling the view after timeout to avoid stale interaction surfacesThe
Viewlives for 5 minutes (timeout=300) but nothing resets/edits the original message once the view expires.
Although the button is a pure link (no callback), Discord will grey-out UI elements automatically only if the bot edits the message and setsview=None. Without that, users keep seeing a seemingly-live “Connect …” button that now does nothing, which is a minor UX snag.Optional quick fix (inside the cog after
ctx.send(embed=..., view=view)):msg = await ctx.send(embed=embed, view=view) await view.wait() # waits until timeout await msg.edit(view=None) # disables the button after 5 min(Or override
on_timeoutin the view and call the same edit.)
11-15: Basic URL sanity-check could prevent accidental malformed links
oauth_urlis injected straight into a link button. If upstream auth code ever returns an empty string or a malformed URL, users will click a broken link.
A minimal guard inside__init__keeps debugging noise low:- self.oauth_url = oauth_url + if not oauth_url.startswith(("http://", "https://")): # very small check + raise ValueError("oauth_url must be an absolute http(s) URL") + self.oauth_url = oauth_urlbackend/app/core/config.py (1)
35-41: Validator leaks no secret but could tighten type/length guaranteesThe
field_validatorsuccessfully blocks empty strings.
If you want stricter compile-time guarantees (and auto-docs), consider Pydantic’sconstrto enforce non-empty values without a custom validator:- supabase_url: str - supabase_key: str + from pydantic import constr + + supabase_url: constr(min_length=1) + supabase_key: constr(min_length=1)This removes the need for
_not_empty, keeps the error message compact, and slightly speeds up validation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/app/core/config.py(3 hunks)backend/app/db/supabase/auth.py(1 hunks)backend/app/db/supabase/supabase_client.py(1 hunks)backend/bots/discord/discord_cogs.py(2 hunks)backend/bots/discord/discord_views.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/app/db/supabase/supabase_client.py
- backend/bots/discord/discord_cogs.py
- backend/app/db/supabase/auth.py
🧰 Additional context used
🪛 Pylint (3.3.7)
backend/bots/discord/discord_views.py
[refactor] 3-3: Too few public methods (0/2)
(R0903)
🔇 Additional comments (1)
backend/app/core/config.py (1)
18-21: 👍 Mandatory Supabase credentials – good fail-fast moveMaking both
supabase_urlandsupabase_keyrequired solves the silent-mis-configuration issue raised earlier.
No further remarks here.
|
@chandansgowda, could you please review and merge? |
2 participants
Attached Interactions
Summary by CodeRabbit