Added a user profile and github token saving after logging in#490
Merged
Conversation
…pplication in UserProvider to provide user context
…s username and token are already present, added a profile page to enable logging out
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Warning Review limit reached
More reviews will be available in 49 minutes and 5 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR implements persistent GitHub token storage and user profile management. The backend adds a token field to users and exposes a POST /token endpoint; the frontend introduces UserContext for session state, new token entry and profile pages, conditional navigation based on login status, and auto-fetch behavior in the tracker that eliminates manual token re-entry. ChangesUser Profile and Token Management Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thank you @anshggss for your contribution. Please make sure your PR follows https://github.com/GitMetricsLab/github_tracker/blob/main/CONTRIBUTING.md#-pull-request-guidelines
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds per-user GitHub Personal Access Token (PAT) management to the authentication flow, introducing a Profile/SetToken UX and wiring the stored token into the tracker experience.
Changes:
- Adds a backend
/api/auth/tokenendpoint and extends the user/session payload to include a stored GitHub token. - Introduces a frontend
UserContext, updates login to populate it, and adds Profile + SetToken pages/routes. - Updates navbar and tracker to reflect logged-in user info and prefill username/token for GitHub data fetching.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/Routes/Router.tsx |
Adds routes for /enterToken and /profile. |
src/pages/Tracker/Tracker.tsx |
Prefills tracker auth fields from UserContext and auto-fetches on username changes. |
src/pages/Profile/Profile.tsx |
New profile page to display username, save token, and logout. |
src/pages/Login/Login.tsx |
Sets UserContext on login and redirects to SetToken if token is missing. |
src/main.tsx |
Wraps app in UserProvider. |
src/context/UserContext.tsx |
Adds new context/provider for logged-in user data + token updates. |
src/components/SetToken.tsx |
New fullscreen flow to save a token immediately after login. |
src/components/Navbar.tsx |
Shows username (link to profile) when logged in, otherwise Login link. |
src/App.tsx |
Treats /enterToken as a fullscreen route. |
backend/routes/auth.js |
Adds token-save endpoint to persist PAT for authenticated users. |
backend/models/User.js |
Adds token field to the User schema. |
backend/config/passportConfig.js |
Includes token in the login response user payload. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+19
to
+23
| token: { | ||
| type: String, | ||
| unique: true, | ||
| sparse: true, | ||
| }, |
Comment on lines
+39
to
+53
| router.post("/token", async (req, res) => { | ||
| if (!req.isAuthenticated()) { | ||
| return res.status(401).json({ message: 'Not authenticated' }); | ||
| } | ||
| const { token } = req.body; | ||
| if (!token) { | ||
| return res.status(400).json({ message: 'Token is required' }); | ||
| } | ||
| try { | ||
| await User.findByIdAndUpdate(req.user._id, { token }); | ||
| req.user.token = token; | ||
| res.status(200).json({ success: true, message: 'Token saved successfully' }); | ||
| } catch (err) { | ||
| res.status(500).json({ message: 'Error saving token', error: err.message }); | ||
| } |
Comment on lines
+83
to
+88
| // Prefill from user context on mount | ||
| useEffect(() => { | ||
| const user = userContext?.user; | ||
| if (user?.username) setUsername(user.username); | ||
| if (user?.token) setToken(user.token); | ||
| }, []); |
Comment on lines
+3
to
+8
| export interface UserData { | ||
| _id: string; | ||
| username: string; | ||
| email: string; | ||
| token?: string; | ||
| } |
Comment on lines
+38
to
+54
| // Save GitHub token route | ||
| router.post("/token", async (req, res) => { | ||
| if (!req.isAuthenticated()) { | ||
| return res.status(401).json({ message: 'Not authenticated' }); | ||
| } | ||
| const { token } = req.body; | ||
| if (!token) { | ||
| return res.status(400).json({ message: 'Token is required' }); | ||
| } | ||
| try { | ||
| await User.findByIdAndUpdate(req.user._id, { token }); | ||
| req.user.token = token; | ||
| res.status(200).json({ success: true, message: 'Token saved successfully' }); | ||
| } catch (err) { | ||
| res.status(500).json({ message: 'Error saving token', error: err.message }); | ||
| } | ||
| }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
src/pages/Tracker/Tracker.tsx (1)
93-98: ⚡ Quick winAvoid double fetches between username-change and page/tab effects.
fetchDatais invoked in both effects. When username changes while on a later page, this can issue duplicate page-1 requests and increase API load.Refactor direction
- // Auto-fetch when debounced username or token changes - useEffect(() => { - if (debouncedUsername && token.trim()) { - setPage(0); - fetchData(debouncedUsername, 1, ROWS_PER_PAGE); - } - }, [debouncedUsername, token]); + // Reset pagination when auth/query identity changes + useEffect(() => { + setPage(0); + }, [debouncedUsername, token]); - // Fetch when tab or page changes (username already set) + // Single fetch path useEffect(() => { - if (username) { - fetchData(username, page + 1, ROWS_PER_PAGE); + if (debouncedUsername && token.trim()) { + fetchData(debouncedUsername, page + 1, ROWS_PER_PAGE); } - }, [tab, page]); + }, [debouncedUsername, token, tab, page]);Also applies to: 101-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/Tracker/Tracker.tsx` around lines 93 - 98, The current useEffect calling setPage(0) and fetchData(debouncedUsername, 1, ROWS_PER_PAGE) causes duplicate requests because another effect also fetches when page/tab changes; change the debouncedUsername effect to only setPage(0) (remove the direct fetchData call) and ensure the existing page/tab effect (the effect that listens to page, tab, token) also includes debouncedUsername in its dependency list so it will perform a single fetchData(debouncedUsername, 1, ROWS_PER_PAGE) when the username changes to page 1.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/models/User.js`:
- Around line 19-23: The User schema currently stores the GitHub PAT in the
token field as plaintext; change storage to a secure pattern: stop exposing it
by default (make the token field non-selectable) and switch to encrypted-at-rest
or a secrets-vault reference. Concretely, update the token definition in User
schema (token) to set it as excluded from default queries (select:false) and
replace direct assignment with a setter/getter that encrypts/decrypts using the
chosen key management solution (or store only a vault/secretId reference instead
of the raw PAT); ensure any code paths that read/write token use the new
encrypt/decrypt or vault API through well-named helpers (e.g.,
UserSchema.methods.setGithubToken / getGithubToken) so plaintext tokens are
never persisted or returned by default.
In `@backend/routes/auth.js`:
- Around line 43-46: The current check accepts whitespace-only tokens; update
the handler that reads const { token } = req.body to trim the incoming string
and validate the trimmed value instead: create a trimmedToken (e.g.,
tokenTrimmed = token && token.trim()), return res.status(400).json({ message:
'Token is required' }) when tokenTrimmed is falsy/empty, and thereafter use the
trimmedToken wherever the token is stored or used (replace usages of token with
the trimmed value).
- Around line 48-53: The current token-save logic always returns success and
leaks internal error messages; update the handler around User.findByIdAndUpdate
to check the returned value (result) and only set req.user.token and send a 200
when result is truthy, return a 404/appropriate client error if no document was
updated, and on exceptions avoid exposing err.message to clients — log the full
error server-side and return a generic 500 message; also detect duplicate-key
errors (e.g., err.code === 11000) and return a 409-ish response with a safe
message.
In `@src/components/Navbar.tsx`:
- Line 5: Remove the unused Github import from the lucide-react import list in
Navbar.tsx: locate the import statement that currently imports Moon, Sun, Menu,
X, Github and delete Github so only the used icons (Moon, Sun, Menu, X) remain,
which will resolve the ESLint unused-import error.
In `@src/components/SetToken.tsx`:
- Around line 30-33: The handler in the SetToken component currently only acts
when response.data.success is true; add an else branch after the if
(response.data.success) block to explicitly set an error message in component
state (use the existing message setter, e.g., setMessage) and avoid silent
failures; prefer using response.data.message if present, otherwise set a generic
failure string—keep calls to updateToken(token) and navigate("/") only inside
the success branch.
In `@src/context/UserContext.tsx`:
- Around line 3-8: UserData currently declares _id while backend auth payload
uses id; update the frontend contract to accept the backend shape by replacing
or supporting _id with id: change the UserData interface (symbol: UserData in
src/context/UserContext.tsx) so it uses id: string instead of _id, or allow both
by adding id?: string and mapping/normalizing when consuming auth payloads
(e.g., normalizeAuthPayload functions or where setUser is called) so the app
consistently uses user.id internally; ensure all usages of UserData (state,
setUser, selectors) read the normalized id field.
In `@src/pages/Login/Login.tsx`:
- Around line 44-51: The login code currently checks response.data.message ===
"Login successful" and assumes response.data.user exists; change the branch to
rely on a stable success indicator (e.g., HTTP status or a boolean like
response.data.success) instead of a specific message string, guard against a
missing user payload before calling setUser/navigating (check response.data.user
and handle null/undefined by showing an error or fallback), and when user object
exists use user.token to choose navigate("/enterToken") vs navigate("/"); update
references in this flow (response, response.data, setUser, userData, token,
navigate) accordingly.
In `@src/pages/Profile/Profile.tsx`:
- Around line 38-45: The token submission allows blank/whitespace values; before
calling axios.post and before calling updateToken/setSaved validate the token by
trimming it and rejecting empty strings (e.g. const cleaned = token.trim(); if
(!cleaned) return/show validation error). Apply the same trim-and-non-empty
check in both places where tokens are saved (the block that calls
axios.post(...) and the other save path around updateToken/setSaved referenced
at lines 112-118) so only non-empty trimmed tokens are posted and persisted.
- Around line 23-29: handleLogout currently calls axios.get to /api/auth/logout
which uses a GET for a state-changing operation; change the request to use
axios.post (e.g., await axios.post(`${backendUrl}/api/auth/logout`, {}, {
withCredentials: true })) and ensure the backend route that handles logout
accepts POST; keep the finally behavior (setUser(null); navigate("/login")) and
retain withCredentials so cookies/session are sent.
In `@src/pages/Tracker/Tracker.tsx`:
- Around line 93-98: The effect currently triggers fetchData when
debouncedUsername changes even if token is empty; update the useEffect
conditional to require a truthy token before auto-fetching (e.g., change the
inner if to check both debouncedUsername and token), so call setPage(0) and
fetchData(debouncedUsername, 1, ROWS_PER_PAGE) only when token is present; keep
token in the dependency array and use the existing symbols useEffect,
debouncedUsername, token, setPage, fetchData, and ROWS_PER_PAGE to locate the
change.
---
Nitpick comments:
In `@src/pages/Tracker/Tracker.tsx`:
- Around line 93-98: The current useEffect calling setPage(0) and
fetchData(debouncedUsername, 1, ROWS_PER_PAGE) causes duplicate requests because
another effect also fetches when page/tab changes; change the debouncedUsername
effect to only setPage(0) (remove the direct fetchData call) and ensure the
existing page/tab effect (the effect that listens to page, tab, token) also
includes debouncedUsername in its dependency list so it will perform a single
fetchData(debouncedUsername, 1, ROWS_PER_PAGE) when the username changes to page
1.
🪄 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: 1b75422d-76d9-4d9d-bd0f-5cc24439cf81
📒 Files selected for processing (12)
backend/config/passportConfig.jsbackend/models/User.jsbackend/routes/auth.jssrc/App.tsxsrc/Routes/Router.tsxsrc/components/Navbar.tsxsrc/components/SetToken.tsxsrc/context/UserContext.tsxsrc/main.tsxsrc/pages/Login/Login.tsxsrc/pages/Profile/Profile.tsxsrc/pages/Tracker/Tracker.tsx
Comment on lines
+19
to
+23
| token: { | ||
| type: String, | ||
| unique: true, | ||
| sparse: true, | ||
| }, |
Contributor
There was a problem hiding this comment.
Avoid storing GitHub PATs as plaintext in user records.
This persists a long-lived credential directly in clear text. Encrypt it at rest (or use a secrets vault pattern) and avoid exposing it by default in queries.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/models/User.js` around lines 19 - 23, The User schema currently
stores the GitHub PAT in the token field as plaintext; change storage to a
secure pattern: stop exposing it by default (make the token field
non-selectable) and switch to encrypted-at-rest or a secrets-vault reference.
Concretely, update the token definition in User schema (token) to set it as
excluded from default queries (select:false) and replace direct assignment with
a setter/getter that encrypts/decrypts using the chosen key management solution
(or store only a vault/secretId reference instead of the raw PAT); ensure any
code paths that read/write token use the new encrypt/decrypt or vault API
through well-named helpers (e.g., UserSchema.methods.setGithubToken /
getGithubToken) so plaintext tokens are never persisted or returned by default.
Comment on lines
+43
to
+46
| const { token } = req.body; | ||
| if (!token) { | ||
| return res.status(400).json({ message: 'Token is required' }); | ||
| } |
Contributor
There was a problem hiding this comment.
Reject whitespace-only token input.
At Line 44, a string like " " passes validation and gets stored as a token. Trim first and validate the trimmed value.
Suggested patch
- const { token } = req.body;
- if (!token) {
+ const token = req.body.token?.trim();
+ if (!token) {
return res.status(400).json({ message: 'Token is required' });
}📝 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.
Suggested change
| const { token } = req.body; | |
| if (!token) { | |
| return res.status(400).json({ message: 'Token is required' }); | |
| } | |
| const token = req.body.token?.trim(); | |
| if (!token) { | |
| return res.status(400).json({ message: 'Token is required' }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/routes/auth.js` around lines 43 - 46, The current check accepts
whitespace-only tokens; update the handler that reads const { token } = req.body
to trim the incoming string and validate the trimmed value instead: create a
trimmedToken (e.g., tokenTrimmed = token && token.trim()), return
res.status(400).json({ message: 'Token is required' }) when tokenTrimmed is
falsy/empty, and thereafter use the trimmedToken wherever the token is stored or
used (replace usages of token with the trimmed value).
Comment on lines
+48
to
+53
| await User.findByIdAndUpdate(req.user._id, { token }); | ||
| req.user.token = token; | ||
| res.status(200).json({ success: true, message: 'Token saved successfully' }); | ||
| } catch (err) { | ||
| res.status(500).json({ message: 'Error saving token', error: err.message }); | ||
| } |
Contributor
There was a problem hiding this comment.
Verify update success and avoid leaking raw backend errors.
At Line 48-53, this always returns success when no document is updated, and exposes err.message to clients. Return a safe generic 500 and handle null-update/duplicate-key cases explicitly.
Suggested patch
- await User.findByIdAndUpdate(req.user._id, { token });
+ const updated = await User.findByIdAndUpdate(
+ req.user._id,
+ { token },
+ { new: true, runValidators: true }
+ );
+ if (!updated) {
+ return res.status(404).json({ message: "User not found" });
+ }
req.user.token = token;
res.status(200).json({ success: true, message: 'Token saved successfully' });
} catch (err) {
- res.status(500).json({ message: 'Error saving token', error: err.message });
+ if (err?.code === 11000) {
+ return res.status(409).json({ message: "Token already in use" });
+ }
+ res.status(500).json({ message: 'Error saving token' });
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/routes/auth.js` around lines 48 - 53, The current token-save logic
always returns success and leaks internal error messages; update the handler
around User.findByIdAndUpdate to check the returned value (result) and only set
req.user.token and send a 200 when result is truthy, return a 404/appropriate
client error if no document was updated, and on exceptions avoid exposing
err.message to clients — log the full error server-side and return a generic 500
message; also detect duplicate-key errors (e.g., err.code === 11000) and return
a 409-ish response with a safe message.
| import { useState, useContext } from "react"; | ||
| import { ThemeContext } from "../context/ThemeContext"; | ||
| import { UserContext } from "../context/UserContext"; | ||
| import { Moon, Sun, Menu, X, Github } from "lucide-react"; |
Contributor
There was a problem hiding this comment.
Remove unused Github import to clear lint failure.
Github is imported but never used, matching the ESLint error.
Suggested fix
-import { Moon, Sun, Menu, X, Github } from "lucide-react";
+import { Moon, Sun, Menu, X } from "lucide-react";📝 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.
Suggested change
| import { Moon, Sun, Menu, X, Github } from "lucide-react"; | |
| import { Moon, Sun, Menu, X } from "lucide-react"; |
🧰 Tools
🪛 ESLint
[error] 5-5: 'Github' is defined but never used.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Navbar.tsx` at line 5, Remove the unused Github import from
the lucide-react import list in Navbar.tsx: locate the import statement that
currently imports Moon, Sun, Menu, X, Github and delete Github so only the used
icons (Moon, Sun, Menu, X) remain, which will resolve the ESLint unused-import
error.
Comment on lines
+30
to
+33
| if (response.data.success) { | ||
| updateToken(token); | ||
| navigate("/"); | ||
| } |
Contributor
There was a problem hiding this comment.
Handle non-success API responses explicitly.
If the request resolves but success is false, the UI shows no error and does nothing. Add an else branch to set a message.
Suggested patch
if (response.data.success) {
updateToken(token);
navigate("/");
+ } else {
+ setMessage(response.data.message ?? "Failed to save token.");
}📝 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.
Suggested change
| if (response.data.success) { | |
| updateToken(token); | |
| navigate("/"); | |
| } | |
| if (response.data.success) { | |
| updateToken(token); | |
| navigate("/"); | |
| } else { | |
| setMessage(response.data.message ?? "Failed to save token."); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/SetToken.tsx` around lines 30 - 33, The handler in the
SetToken component currently only acts when response.data.success is true; add
an else branch after the if (response.data.success) block to explicitly set an
error message in component state (use the existing message setter, e.g.,
setMessage) and avoid silent failures; prefer using response.data.message if
present, otherwise set a generic failure string—keep calls to updateToken(token)
and navigate("/") only inside the success branch.
Comment on lines
+3
to
+8
| export interface UserData { | ||
| _id: string; | ||
| username: string; | ||
| email: string; | ||
| token?: string; | ||
| } |
Contributor
There was a problem hiding this comment.
Unify user identifier contract (id vs _id) across backend/frontend.
UserData requires _id, but current auth payload uses id. This mismatch is brittle and can break state handling unless every caller remaps fields.
Suggested patch (frontend contract-side)
export interface UserData {
- _id: string;
+ id: string;
+ _id?: string;
username: string;
email: string;
token?: string;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/context/UserContext.tsx` around lines 3 - 8, UserData currently declares
_id while backend auth payload uses id; update the frontend contract to accept
the backend shape by replacing or supporting _id with id: change the UserData
interface (symbol: UserData in src/context/UserContext.tsx) so it uses id:
string instead of _id, or allow both by adding id?: string and
mapping/normalizing when consuming auth payloads (e.g., normalizeAuthPayload
functions or where setUser is called) so the app consistently uses user.id
internally; ensure all usages of UserData (state, setUser, selectors) read the
normalized id field.
Comment on lines
+44
to
+51
| if (response.data.message === "Login successful") { | ||
| const userData = response.data.user; | ||
| setUser(userData); | ||
| if (!userData.token) { | ||
| navigate("/enterToken"); | ||
| } else { | ||
| navigate("/"); | ||
| } |
Contributor
There was a problem hiding this comment.
Decouple login success flow from message text and guard missing user payload.
The post-login branch is tied to an exact message string and assumes response.data.user exists. A backend copy change or malformed success payload can skip navigation or crash on userData.token.
Suggested fix
- if (response.data.message === "Login successful") {
- const userData = response.data.user;
- setUser(userData);
- if (!userData.token) {
- navigate("/enterToken");
- } else {
- navigate("/");
- }
- }
+ const userData = response.data?.user;
+ if (response.status === 200 && userData) {
+ setUser(userData);
+ navigate(userData.token ? "/" : "/enterToken");
+ } else {
+ setMessage(response.data?.message || "Login failed");
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/Login/Login.tsx` around lines 44 - 51, The login code currently
checks response.data.message === "Login successful" and assumes
response.data.user exists; change the branch to rely on a stable success
indicator (e.g., HTTP status or a boolean like response.data.success) instead of
a specific message string, guard against a missing user payload before calling
setUser/navigating (check response.data.user and handle null/undefined by
showing an error or fallback), and when user object exists use user.token to
choose navigate("/enterToken") vs navigate("/"); update references in this flow
(response, response.data, setUser, userData, token, navigate) accordingly.
Comment on lines
+23
to
+29
| const handleLogout = async () => { | ||
| try { | ||
| await axios.get(`${backendUrl}/api/auth/logout`, { withCredentials: true }); | ||
| } finally { | ||
| setUser(null); | ||
| navigate("/login"); | ||
| } |
Contributor
There was a problem hiding this comment.
Use a non-GET request for logout.
Logout mutates session state and should not be invoked with GET. Using POST (and matching backend route) reduces CSRF/safe-method exposure.
Suggested fix
- await axios.get(`${backendUrl}/api/auth/logout`, { withCredentials: true });
+ await axios.post(
+ `${backendUrl}/api/auth/logout`,
+ {},
+ { withCredentials: true },
+ );📝 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.
Suggested change
| const handleLogout = async () => { | |
| try { | |
| await axios.get(`${backendUrl}/api/auth/logout`, { withCredentials: true }); | |
| } finally { | |
| setUser(null); | |
| navigate("/login"); | |
| } | |
| const handleLogout = async () => { | |
| try { | |
| await axios.post( | |
| `${backendUrl}/api/auth/logout`, | |
| {}, | |
| { withCredentials: true }, | |
| ); | |
| } finally { | |
| setUser(null); | |
| navigate("/login"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/Profile/Profile.tsx` around lines 23 - 29, handleLogout currently
calls axios.get to /api/auth/logout which uses a GET for a state-changing
operation; change the request to use axios.post (e.g., await
axios.post(`${backendUrl}/api/auth/logout`, {}, { withCredentials: true })) and
ensure the backend route that handles logout accepts POST; keep the finally
behavior (setUser(null); navigate("/login")) and retain withCredentials so
cookies/session are sent.
Comment on lines
+38
to
+45
| const response = await axios.post( | ||
| `${backendUrl}/api/auth/token`, | ||
| { token }, | ||
| { withCredentials: true } | ||
| ); | ||
| if (response.data.success) { | ||
| updateToken(token); | ||
| setSaved(true); |
Contributor
There was a problem hiding this comment.
Block empty or whitespace-only token saves.
Token submission currently accepts blank/whitespace input, which can save unusable credentials and keep users stuck in token setup flows.
Suggested fix
const handleSaveToken = async (e: React.FormEvent) => {
e.preventDefault();
+ const trimmedToken = token.trim();
+ if (!trimmedToken) {
+ setError("Token is required.");
+ return;
+ }
setIsLoading(true);
setError("");
setSaved(false);
try {
const response = await axios.post(
`${backendUrl}/api/auth/token`,
- { token },
+ { token: trimmedToken },
{ withCredentials: true }
);
if (response.data.success) {
- updateToken(token);
+ updateToken(trimmedToken);
setSaved(true);
setTimeout(() => setSaved(false), 3000);
}Also applies to: 112-118
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/Profile/Profile.tsx` around lines 38 - 45, The token submission
allows blank/whitespace values; before calling axios.post and before calling
updateToken/setSaved validate the token by trimming it and rejecting empty
strings (e.g. const cleaned = token.trim(); if (!cleaned) return/show validation
error). Apply the same trim-and-non-empty check in both places where tokens are
saved (the block that calls axios.post(...) and the other save path around
updateToken/setSaved referenced at lines 112-118) so only non-empty trimmed
tokens are posted and persisted.
Comment on lines
+93
to
+98
| useEffect(() => { | ||
| if (debouncedUsername) { | ||
| setPage(0); | ||
| fetchData(debouncedUsername, 1, ROWS_PER_PAGE); | ||
| } | ||
| }, [debouncedUsername, token]); |
Contributor
There was a problem hiding this comment.
Gate auto-fetch by token presence.
This effect fetches on username debounce even when token is empty, which can create repeated unauthenticated calls and noisy error states.
Suggested fix
- useEffect(() => {
- if (debouncedUsername) {
+ useEffect(() => {
+ if (debouncedUsername && token.trim()) {
setPage(0);
fetchData(debouncedUsername, 1, ROWS_PER_PAGE);
}
}, [debouncedUsername, token]);📝 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.
Suggested change
| useEffect(() => { | |
| if (debouncedUsername) { | |
| setPage(0); | |
| fetchData(debouncedUsername, 1, ROWS_PER_PAGE); | |
| } | |
| }, [debouncedUsername, token]); | |
| useEffect(() => { | |
| if (debouncedUsername && token.trim()) { | |
| setPage(0); | |
| fetchData(debouncedUsername, 1, ROWS_PER_PAGE); | |
| } | |
| }, [debouncedUsername, token]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/Tracker/Tracker.tsx` around lines 93 - 98, The effect currently
triggers fetchData when debouncedUsername changes even if token is empty; update
the useEffect conditional to require a truthy token before auto-fetching (e.g.,
change the inner if to check both debouncedUsername and token), so call
setPage(0) and fetchData(debouncedUsername, 1, ROWS_PER_PAGE) only when token is
present; keep token in the dependency array and use the existing symbols
useEffect, debouncedUsername, token, setPage, fetchData, and ROWS_PER_PAGE to
locate the change.
Contributor
Author
|
Hey @mehul-m-prajapati can you check my pr and and merge? |
|
🎉🎉 Thank you for your contribution! Your PR #490 has been merged! 🎉🎉 |
5 tasks
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
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.
Related Issue
Description
This PR adds GitHub token management to the authentication flow, enabling users to save and use their GitHub personal access token within the app.
Key changes include:
POSTroute to save the GitHub token to MongoDBsetTokenroute so users can enter their token on loginSetTokenpage when a user has no token savedHow Has This Been Tested?
SetTokenpage when token is absentScreenshots (if applicable)
Type of Change
Summary by CodeRabbit