feat(frontend): update oauth flow and add load testing script#127
feat(frontend): update oauth flow and add load testing script#127omsherikar merged 3 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
bot
added
type:refactor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 22 minutes and 48 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds GitHub disconnect UI and a GitHub "connect" OAuth path; persists sidebar collapsed state to localStorage and refactors header/avatar helpers; replaces several mailto contact addresses with om@refactron.dev or external Cal.com scheduling links; updates OAuth exchange to support authenticated connect flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant GitHub
participant Server
rect rgba(100,150,255,0.5)
Note over User,Server: Authenticated "Connect" Flow
User->>Browser: Click "Connect GitHub"
Browser->>GitHub: Redirect to GitHub auth
GitHub->>User: Approve
GitHub->>Browser: Redirect to /auth/callback with code
Browser->>Server: POST /api/auth/connect/github (Bearer token + code + redirectUri)
Server->>Server: Validate token, exchange code, attach provider to account
Server->>Browser: Return updated user data
Browser->>Browser: updateUser(data.user) — keep existing session
end
rect rgba(100,200,100,0.5)
Note over User,Server: Login/Signup Flow (New Session)
User->>Browser: Click "Login with GitHub"
Browser->>GitHub: Redirect to GitHub auth
GitHub->>User: Approve
GitHub->>Browser: Redirect to /auth/callback with code+state
Browser->>Server: POST /api/auth/callback/github (code, state, type, redirectUri)
Server->>Server: Exchange code for tokens, create/return session
Server->>Browser: Return accessToken + user
Browser->>Browser: login(accessToken, user) — replace session
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/OAuthCallback.tsx (1)
115-115:⚠️ Potential issue | 🔴 CriticalAdd missing
updateUserto the useEffect dependency array.The pipeline is failing because
updateUseris used inside the effect but not listed in the dependency array. This violates thereact-hooks/exhaustive-depsrule and causes the CI lint check to fail.🐛 Fix for the ESLint error
- }, [searchParams, navigate, login]); + }, [searchParams, navigate, login, updateUser]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/OAuthCallback.tsx` at line 115, The useEffect in OAuthCallback references updateUser but the dependency array only lists searchParams, navigate, and login, causing an exhaustive-deps lint error; fix by adding updateUser to the dependency array of the useEffect that runs in OAuthCallback (the same effect referencing searchParams, navigate, and login), and if updateUser is not stable ensure it is memoized (e.g., defined with useCallback or moved to a stable context) so adding it to the array does not introduce unwanted rerenders.
🧹 Nitpick comments (2)
src/components/DashboardLayout.tsx (1)
78-84: Consider guarding against invalid date strings.If
isois malformed or empty,new Date(iso).getTime()returnsNaN, resulting in"NaNm ago"being displayed. This is a minor edge case if the API always returns valid timestamps, but a defensive check would prevent confusing output.🛡️ Optional defensive fix
function formatNotifTime(iso: string): string { + const time = new Date(iso).getTime(); + if (Number.isNaN(time)) return ''; - const mins = Math.floor((Date.now() - new Date(iso).getTime()) / 60000); + const mins = Math.floor((Date.now() - time) / 60000); if (mins < 60) return `${mins}m ago`; const hrs = Math.floor(mins / 60); if (hrs < 24) return `${hrs}h ago`; return `${Math.floor(hrs / 24)}d ago`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DashboardLayout.tsx` around lines 78 - 84, The function formatNotifTime should guard against invalid or empty ISO strings: inside formatNotifTime (the function handling the timestamp formatting) validate the parsed date before using it (e.g., const t = new Date(iso).getTime(); if (isNaN(t)) return a sensible fallback like 'unknown' or '' or 'just now') and then compute mins/hrs/days from that validated timestamp; this prevents outputs like "NaNm ago" when iso is malformed and keeps the rest of the logic (mins/hrs/days) unchanged.src/components/FAQSection.tsx (1)
135-137: Extract the booking URL to a shared constant.This URL is now duplicated across multiple components; centralizing it will avoid future drift and partial updates.
♻️ Example refactor (local start)
+const CONTACT_BOOKING_URL = 'https://cal.com/omsherikar/queries-refactron'; ... - href="https://cal.com/omsherikar/queries-refactron" + href={CONTACT_BOOKING_URL}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/FAQSection.tsx` around lines 135 - 137, The booking URL string used in JSX (e.g., href="https://cal.com/omsherikar/queries-refactron" inside FAQSection's anchor) should be extracted into a shared exported constant (suggest name BOOKING_URL) in a central module (e.g., a new or existing constants file) and then imported where needed; update FAQSection (and any other components duplicating the URL) to reference BOOKING_URL instead of the hardcoded string so all components use the single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/EarlyAccessModal.tsx`:
- Around line 58-60: Update the environment template so the documented defaults
for REACT_APP_FROM_EMAIL and REACT_APP_NOTIFICATION_EMAIL match the new
fallbacks used in EarlyAccessModal (om@refactron.dev); open the env example that
documents those two variables and replace the old hello@refactron.dev values
with om@refactron.dev (or note the new defaults) so users who copy the template
won't accidentally route notifications to the old inbox.
---
Outside diff comments:
In `@src/components/OAuthCallback.tsx`:
- Line 115: The useEffect in OAuthCallback references updateUser but the
dependency array only lists searchParams, navigate, and login, causing an
exhaustive-deps lint error; fix by adding updateUser to the dependency array of
the useEffect that runs in OAuthCallback (the same effect referencing
searchParams, navigate, and login), and if updateUser is not stable ensure it is
memoized (e.g., defined with useCallback or moved to a stable context) so adding
it to the array does not introduce unwanted rerenders.
---
Nitpick comments:
In `@src/components/DashboardLayout.tsx`:
- Around line 78-84: The function formatNotifTime should guard against invalid
or empty ISO strings: inside formatNotifTime (the function handling the
timestamp formatting) validate the parsed date before using it (e.g., const t =
new Date(iso).getTime(); if (isNaN(t)) return a sensible fallback like 'unknown'
or '' or 'just now') and then compute mins/hrs/days from that validated
timestamp; this prevents outputs like "NaNm ago" when iso is malformed and keeps
the rest of the logic (mins/hrs/days) unchanged.
In `@src/components/FAQSection.tsx`:
- Around line 135-137: The booking URL string used in JSX (e.g.,
href="https://cal.com/omsherikar/queries-refactron" inside FAQSection's anchor)
should be extracted into a shared exported constant (suggest name BOOKING_URL)
in a central module (e.g., a new or existing constants file) and then imported
where needed; update FAQSection (and any other components duplicating the URL)
to reference BOOKING_URL instead of the hardcoded string so all components use
the single source of truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3e7a540-cdf9-4d62-8e47-151b6003baab
📒 Files selected for processing (12)
src/components/AboutPage.tsxsrc/components/AccountSettings.tsxsrc/components/CaseStudiesPage.tsxsrc/components/DashboardLayout.tsxsrc/components/EarlyAccessForm.tsxsrc/components/EarlyAccessModal.tsxsrc/components/ErrorBoundary.tsxsrc/components/FAQSection.tsxsrc/components/Footer.tsxsrc/components/OAuthCallback.tsxsrc/components/PricingSection.tsxsrc/utils/oauth.ts
src/components/EarlyAccessModal.tsx
Outdated
| process.env.REACT_APP_FROM_EMAIL || 'om@refactron.dev'; | ||
| const notificationEmail = | ||
| process.env.REACT_APP_NOTIFICATION_EMAIL || 'hello@refactron.dev'; | ||
| process.env.REACT_APP_NOTIFICATION_EMAIL || 'om@refactron.dev'; |
There was a problem hiding this comment.
Update .env.example to avoid sending notifications to the old inbox.
Lines 58 and 60 changed the fallback, but .env.example Line 8-9 still documents hello@refactron.dev. If those vars are set from the template, they override these new defaults and route emails incorrectly.
🛠️ Suggested follow-up patch (`.env.example`)
-REACT_APP_FROM_EMAIL=hello@refactron.dev
-REACT_APP_NOTIFICATION_EMAIL=hello@refactron.dev
+REACT_APP_FROM_EMAIL=om@refactron.dev
+REACT_APP_NOTIFICATION_EMAIL=om@refactron.dev🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/EarlyAccessModal.tsx` around lines 58 - 60, Update the
environment template so the documented defaults for REACT_APP_FROM_EMAIL and
REACT_APP_NOTIFICATION_EMAIL match the new fallbacks used in EarlyAccessModal
(om@refactron.dev); open the env example that documents those two variables and
replace the old hello@refactron.dev values with om@refactron.dev (or note the
new defaults) so users who copy the template won't accidentally route
notifications to the old inbox.
There was a problem hiding this comment.
Pull request overview
Updates the frontend OAuth callback handling to support a dedicated “connect GitHub” flow without replacing the current session, alongside several UI updates (dashboard layout refactor, GitHub disconnect UI, and updated contact links).
Changes:
- Add a dedicated OAuth “connect” callback path (
/api/auth/connect/:provider) and propagate anisConnectflag through the callback handling. - Update auth callback UI logic to refresh the user state on connect (instead of calling
login). - Refactor
DashboardLayoutsidebar/nav UI and update various contact links/emails across the site; add GitHub disconnect controls in Account Settings.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/oauth.ts | Adds connect-flow endpoint handling and isConnect return flag; refactors redirectUri usage. |
| src/components/OAuthCallback.tsx | Uses updateUser for connect flow; keeps login for login/signup flow. |
| src/components/DashboardLayout.tsx | Sidebar refactor (collapse persistence, helper components, nav rendering) and GitHub connect entrypoint. |
| src/components/AccountSettings.tsx | Adds GitHub disconnect UI + request handler. |
| src/components/PricingSection.tsx | Updates contact mailto address. |
| src/components/Footer.tsx | Switches “Contact” to external scheduling link (new tab). |
| src/components/FAQSection.tsx | Switches “Contact Us” to external scheduling link (new tab). |
| src/components/ErrorBoundary.tsx | Updates support mailto address. |
| src/components/EarlyAccessModal.tsx | Updates default from/notification email fallbacks. |
| src/components/EarlyAccessForm.tsx | Updates default from/notification email fallbacks and displayed support email. |
| src/components/CaseStudiesPage.tsx | Switches CTA link to external scheduling link (new tab). |
| src/components/AboutPage.tsx | Switches CTA link to external scheduling link (new tab). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Connect flow — use dedicated authenticated endpoint, preserve existing session | ||
| if (type === 'connect') { | ||
| const token = localStorage.getItem('accessToken'); | ||
| if (!token) { | ||
| return { | ||
| success: false, | ||
| error: 'You must be logged in to connect a GitHub account.', | ||
| }; | ||
| } | ||
|
|
||
| const response = await fetch( | ||
| `${apiBaseUrl}/api/auth/connect/${provider}`, |
There was a problem hiding this comment.
PR title mentions “add load testing script”, but the changed files in this PR appear limited to frontend OAuth/UI updates and contact link changes; no load-testing script (e.g. under scripts/ or a new tool config) is included. Please either add the load testing script as described or update the PR title to match the actual scope.
| if (!token) { | ||
| return { | ||
| success: false, | ||
| error: 'You must be logged in to connect a GitHub account.', |
There was a problem hiding this comment.
In the connect flow, the error message is hard-coded to “connect a GitHub account”, but this function is provider-agnostic (provider comes from state and the endpoint uses /connect/${provider}). Please make the message generic (e.g., “connect your account”) or interpolate the provider so it stays accurate if additional providers support type === 'connect'.
| error: 'You must be logged in to connect a GitHub account.', | |
| error: `You must be logged in to connect your ${provider} account.`, |
| // Connect flow — use dedicated authenticated endpoint, preserve existing session | ||
| if (type === 'connect') { | ||
| const token = localStorage.getItem('accessToken'); | ||
| if (!token) { | ||
| return { | ||
| success: false, | ||
| error: 'You must be logged in to connect a GitHub account.', | ||
| }; | ||
| } | ||
|
|
||
| const response = await fetch( | ||
| `${apiBaseUrl}/api/auth/connect/${provider}`, | ||
| { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| Authorization: `Bearer ${token}`, | ||
| }, | ||
| credentials: 'include', | ||
| body: JSON.stringify({ code, redirectUri }), | ||
| } | ||
| ); | ||
|
|
||
| const data = await response.json(); | ||
|
|
||
| if (!response.ok) { | ||
| return { | ||
| success: false, | ||
| error: data.message || `Failed to connect ${provider} account.`, | ||
| }; | ||
| } | ||
|
|
||
| // Return isConnect flag so OAuthCallback knows not to replace the session | ||
| return { success: true, data, isConnect: true }; | ||
| } |
There was a problem hiding this comment.
handleOAuthCallback now has a new type === 'connect' branch and new return shape (isConnect). There are existing unit tests for handleOAuthCallback in src/utils/oauth.test.ts, but they don’t cover the connect branch. Please add tests for: (1) missing access token path, (2) successful connect returning isConnect: true, and (3) non-OK response mapping data.message to error.
| if (result.isConnect) { | ||
| // Connect flow — keep existing session, just refresh user state | ||
| updateUser(result.data.user); | ||
| } else { | ||
| // Login / signup flow — replace session with new tokens | ||
| await login(result.data.accessToken, result.data.user); | ||
| } |
There was a problem hiding this comment.
The new result.isConnect branch changes the component’s auth behavior (updating user state without calling login). There are existing component tests (OAuthCallback.test.tsx) but they don’t exercise the connect path; please add a test that asserts updateUser is called (and login is not) when handleOAuthCallback resolves with { success: true, isConnect: true, data: { user: ... } }.
|
|
||
| const handleGitHubConnect = async () => { | ||
| try { | ||
| const { initiateOAuth } = await import('../utils/oauth'); | ||
| await initiateOAuth('github', 'connect', { |
There was a problem hiding this comment.
initiateOAuth here uses window.location.origin for redirectUri, but other OAuth entrypoints (e.g. login/signup) use getBaseUrl() and OAuthCallback passes getBaseUrl() as the callback redirect. On non-app.* hosts routed to the same deployment (e.g. docs.*), window.location.origin can differ from getBaseUrl(), which can cause redirect URI mismatches during code exchange. Please align this to use getBaseUrl() (or otherwise ensure the same redirect URI is used for both authorization and token exchange).
|
|
||
| return ( | ||
| <div className="flex h-screen bg-black text-white font-space overflow-hidden"> | ||
| {/* Sidebar */} |
There was a problem hiding this comment.
The sidebar <motion.aside> now has overflow-hidden. This will clip dropdown popovers and/or their shadows (notifications/profile/org menus) when they extend beyond the sidebar bounds, which is especially noticeable with shadow-2xl. Please remove overflow-hidden or move overflow clipping to a more specific inner container that won’t contain the popovers.
| {/* Sidebar */} | |
| className="flex flex-col border-r border-white/[0.08] bg-black z-20" |
Summary by CodeRabbit
New Features
Improvements
Behavior