Feature/bookmarks#558
Open
AnishCoder2006 wants to merge 3 commits into
Open
Conversation
… error conditions and Improved Activity page spacing tracking
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
📝 WalkthroughWalkthroughThis PR implements a complete bookmarks feature for saving and managing frequently tracked GitHub profiles. The backend adds a ChangesGitHub Profile Bookmarks Feature
Sequence Diagram(s)sequenceDiagram
participant Client as Client (React)
participant Server as Express Server
participant Auth as ensureAuth
participant DB as MongoDB User
Client->>Server: GET /api/bookmarks
Server->>Auth: Check Session
Auth-->>Server: Authenticated
Server->>DB: User.findById().select('bookmarks')
DB-->>Server: User bookmarks array
Server-->>Client: { bookmarks: [...] }
Client->>Server: POST /api/bookmarks<br/>{ githubUsername }
Server->>Auth: Check Session
Auth-->>Server: Authenticated
Server->>DB: Load user, validate username
DB-->>Server: User + bookmarks
Server->>DB: Prepend bookmark, save
DB-->>Server: Saved user
Server-->>Client: 201 { bookmark }
Client->>Server: DELETE /api/bookmarks/:username
Server->>Auth: Check Session
Auth-->>Server: Authenticated
Server->>DB: Filter bookmarks, save
DB-->>Server: Updated user
Server-->>Client: 200 Success
sequenceDiagram
participant Component as ActivityFeed
participant Backend as /api/bookmarks
participant GitHub as GitHub API
Component->>Backend: GET /api/bookmarks (check if username saved)
Backend-->>Component: { bookmarks: [...] }
Component->>Component: Update isBookmarked state
Component->>GitHub: GET /repos/:username/events
GitHub-->>Component: Events array
Component->>Component: Validate array, slice to 10
Component->>Component: Render cards with star button
Component->>Backend: POST /api/bookmarks { githubUsername }
Backend-->>Component: 201 { bookmark }
Component->>Component: Set isBookmarked true
Component->>Backend: DELETE /api/bookmarks/:username
Backend-->>Component: 200 Success
Component->>Component: Set isBookmarked false
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ 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 |
Author
|
5 successful checks have been completed,please approve my PR for issue #546 @mehul-m-prajapati |
![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: 5
🧹 Nitpick comments (7)
backend/routes/bookmarks.js (6)
6-8: 💤 Low valueSimplify isAuthenticated check to standard Passport pattern.
The double-check
req.isAuthenticated && req.isAuthenticated()is overly defensive. With Passport properly initialized (as seen inserver.js),req.isAuthenticatedwill always be a function.♻️ Standard Passport pattern
function ensureAuth(req, res, next) { - if (req.isAuthenticated && req.isAuthenticated()) return next(); + if (req.isAuthenticated()) return next(); return res.status(401).json({ message: 'Authentication 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/bookmarks.js` around lines 6 - 8, The ensureAuth middleware currently uses an overly defensive check (req.isAuthenticated && req.isAuthenticated()); simplify it to the standard Passport pattern by calling req.isAuthenticated() directly in ensureAuth and returning next() when true, otherwise return the 401 JSON response; update the function named ensureAuth to remove the redundant truthy check and use only req.isAuthenticated() to determine authentication.
14-15: ⚡ Quick winUse consistent user ID accessor.
Line 14 logs
req.user.idbut line 15 queries withreq.user._id. While both work in Mongoose (.idis a virtual getter for._id.toString()), mixing them is confusing. Usereq.user._idconsistently throughout the routes.♻️ Consistent ID usage
- console.log('GET /api/bookmarks req.isAuthenticated=', req.isAuthenticated && req.isAuthenticated(), 'user=', req.user && req.user.id); + console.log('GET /api/bookmarks req.isAuthenticated=', req.isAuthenticated && req.isAuthenticated(), 'user=', req.user && req.user._id);Apply the same fix to lines 26 and 54 in POST and DELETE routes.
🤖 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/bookmarks.js` around lines 14 - 15, Change all uses of the user's ID in this route file to the same accessor: replace any occurrences of req.user.id with req.user._id so the console.log and database queries are consistent (e.g., the GET handler where you call User.findById(req.user._id)). Apply the same consistent change in the POST and DELETE bookmark handlers (the handlers that reference req.user.id around the POST and DELETE routes) so all lookups and logs use req.user._id.
70-72: ⚡ Quick winAdd error logging for consistency and avoid exposing internal error details.
The DELETE route is missing error logging that the GET and POST routes have. Additionally, exposing
err.messagein production responses (across all three routes) could leak internal implementation details.🔒 Add logging and sanitize error response
} catch (err) { + console.error('Error removing bookmark', err); - return res.status(500).json({ message: 'Failed to remove bookmark', error: err.message }); + return res.status(500).json({ message: 'Failed to remove bookmark' }); }Also apply the error sanitization to GET (line 19) and POST (line 47) routes:
// GET route (line 19) - return res.status(500).json({ message: 'Failed to fetch bookmarks', error: err.message }); + return res.status(500).json({ message: 'Failed to fetch bookmarks' }); // POST route (line 47) - return res.status(500).json({ message: 'Failed to save bookmark', error: err.message }); + return res.status(500).json({ message: 'Failed to save bookmark' });🤖 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/bookmarks.js` around lines 70 - 72, Add error logging to the DELETE '/:id' handler and stop returning raw err.message from all three route handlers (GET '/', POST '/', DELETE '/:id'); instead, log the full error using the same logger used by the other routes (e.g., logger.error or console.error) and return a sanitized JSON response like { message: 'Failed to ...' } without internal error details. Update the GET and POST handlers to mirror this pattern: log the caught error and respond with a generic message rather than sending err.message to the client.
14-14: ⚡ Quick winReplace console.log with proper logger for production readiness.
The routes use
console.logfor debugging. The project already has a logger (imported inserver.jsline 12). Using a proper logger enables log levels, formatting, and centralized log management.📊 Use project logger
Add logger import at the top of the file:
const express = require('express'); const router = express.Router(); const User = require('../models/User'); +const logger = require('../logger');Then replace console.log/console.error calls:
- console.log('GET /api/bookmarks req.isAuthenticated=', req.isAuthenticated && req.isAuthenticated(), 'user=', req.user && req.user._id); + logger.info('GET /api/bookmarks', { isAuthenticated: req.isAuthenticated(), userId: req.user?._id }); - console.error('Error fetching bookmarks', err); + logger.error('Error fetching bookmarks', err);Apply similar changes to lines 26, 46, 54, and add logger.error at line 70.
Also applies to: 26-26, 54-54
🤖 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/bookmarks.js` at line 14, Replace the console.log debug calls in bookmarks.js with the project's centralized logger: import the existing logger (same export used in server.js) at the top of the file, then change the console.log on the GET /api/bookmarks route (the line referencing req.isAuthenticated and req.user.id) to logger.info or logger.debug and format the message to include the same context; likewise replace other console.log occurrences (the ones at the locations that currently reference request/auth details) with logger.info/debug, and add a logger.error call where errors are handled (the spot mentioned around line 70) instead of console.error so errors are logged at the error level. Make sure to reference the exact names used in the file (req.isAuthenticated, req.user) and keep messages descriptive and consistent with the project logger usage.
27-30: ⚡ Quick winAdd validation for githubUsername format and avatarUrl.
The route validates presence of
githubUsernamebut not its format. Consider adding:
- Regex validation that
githubUsernamematches GitHub's rules (alphanumeric, hyphens, max 39 chars, cannot start with hyphen)- URL format validation for
avatarUrlif providedThis prevents storing invalid data and provides better error messages to users.
🛡️ Enhanced input validation
const { githubUsername, avatarUrl } = req.body; if (!githubUsername || !githubUsername.trim()) { return res.status(400).json({ message: 'githubUsername is required' }); } + + // Validate GitHub username format + const githubUsernameRegex = /^[a-zA-Z0-9](?:[a-zA-Z0-9]|-(?=[a-zA-Z0-9])){0,38}$/; + if (!githubUsernameRegex.test(githubUsername.trim())) { + return res.status(400).json({ message: 'Invalid GitHub username format' }); + } + + // Validate avatarUrl if provided + if (avatarUrl && !/^https?:\/\/.+/.test(avatarUrl)) { + return res.status(400).json({ message: 'avatarUrl must be a valid HTTP(S) URL' }); + }🤖 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/bookmarks.js` around lines 27 - 30, The route currently only checks presence of githubUsername; add format validation for githubUsername and optional avatarUrl: validate githubUsername from req.body against GitHub rules (only alphanumeric and hyphens, cannot start with a hyphen, max 39 characters) and return res.status(400).json({ message: 'invalid githubUsername' }) on failure; if avatarUrl is provided validate it is a well-formed URL (use URL constructor or a robust URL validator) and return res.status(400).json({ message: 'invalid avatarUrl' }) on failure; keep using githubUsername and avatarUrl from req.body and update the request handler in bookmarks.js to perform these checks before proceeding with storage.
40-40: 💤 Low valueRemove unnecessary bookmarks array initialization.
Line 40 defensively initializes
user.bookmarkswithuser.bookmarks || [], but the schema (User.js lines 19-25) definesbookmarksas an array field with a default of[]. A document fetched from the database will always have this field initialized, making this line redundant.♻️ Remove redundant initialization
- user.bookmarks = user.bookmarks || []; user.bookmarks.unshift({ githubUsername, avatarUrl });🤖 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/bookmarks.js` at line 40, Remove the redundant defensive initialization of user.bookmarks (the line using "user.bookmarks = user.bookmarks || []"); the User schema already provides a default empty array, so delete that assignment wherever it appears in the bookmarks route handler (referenced as user.bookmarks) to avoid unnecessary code and rely on the schema default.backend/models/User.js (1)
21-22: ⚡ Quick winAdd validation for githubUsername and avatarUrl formats.
The schema accepts any string for
githubUsernameandavatarUrlwithout format validation. Consider adding:
- A regex pattern for
githubUsernameto match GitHub's username rules (alphanumeric, hyphens, max 39 chars, cannot start with hyphen)- A URL validation or regex pattern for
avatarUrlto prevent malformed or potentially unsafe URLsWhile the API layer can validate these inputs at request time, schema-level validation provides defense in depth and ensures data integrity regardless of how documents are created or updated.
🛡️ Suggested schema validation patterns
bookmarks: [ { - githubUsername: { type: String, required: true }, + githubUsername: { + type: String, + required: true, + match: /^[a-zA-Z0-9](?:[a-zA-Z0-9]|-(?=[a-zA-Z0-9])){0,38}$/, + }, - avatarUrl: { type: String }, + avatarUrl: { + type: String, + validate: { + validator: function(v) { + return !v || /^https?:\/\/.+/.test(v); + }, + message: 'avatarUrl must be a valid HTTP(S) URL' + } + }, savedAt: { type: Date, default: Date.now }, }, ],🤖 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 21 - 22, Update the Mongoose schema for the User model to validate githubUsername and avatarUrl: add a regex validator to the githubUsername field (match GitHub rules: alphanumeric and hyphens, cannot start/end with hyphen, max 39 chars) and add a URL validator or URL-regex to the avatarUrl field (ensure well-formed http/https URLs), using the existing githubUsername and avatarUrl schema keys (e.g., in UserSchema or the exported User model) so invalid values are rejected at the schema level and provide clear validation error messages.
🤖 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/server.js`:
- Line 38: The session secret currently falls back to a hardcoded 'dev-secret'
(secret: process.env.SESSION_SECRET || 'dev-secret') which is unsafe; change the
logic to require SESSION_SECRET in production by throwing an error if
process.env.NODE_ENV === 'production' and process.env.SESSION_SECRET is falsy,
or remove the default entirely and always use process.env.SESSION_SECRET so the
app fails fast when missing; update the code paths that reference secret to use
only process.env.SESSION_SECRET (or the validated value) and add a startup check
that validates SESSION_SECRET when the server initializes.
In `@src/components/ActivityFeed.tsx`:
- Around line 63-65: The GitHub events URL is built by interpolating the raw
username into the fetch call, which can produce malformed requests for special
characters; update the fetch invocation in the ActivityFeed component to encode
the username (use encodeURIComponent(username)) when constructing
`https://api.github.com/users/.../events` so the request is safe for all
inputs—locate the fetch call that references the `username` variable in
ActivityFeed (or any helper like fetchEvents) and replace the direct
interpolation with an encoded username.
- Around line 39-45: In ActivityFeed.tsx, update the bookmark check to use a
proper typed payload instead of (b: any) and remove the unused catch parameter:
define or import an interface like Bookmark { githubUsername: string } and
change the some callback to (b: Bookmark) => b.githubUsername.toLowerCase() ===
username.toLowerCase(); also replace the unused catch (err) { /* ignore */ }
with catch { } to avoid an unused variable; keep the existing
setIsBookmarked(!!found) logic intact.
In `@src/pages/Bookmarks.tsx`:
- Around line 18-20: Remove the unused ThemeContext destructuring and unused
type: delete the lines that call useContext(ThemeContext) as ThemeContextType
and the subsequent const { mode } = themeContext (and remove any now-unused
imports ThemeContext and ThemeContextType if present). Also replace any
try/catch blocks that use unused catch bindings (e.g., catch (err: unknown) and
catch (err)) with bare catch blocks (catch) so ESLint no longer flags unused
variables; keep existing error handling logic inside the catch body unchanged.
In `@src/Routes/Router.tsx`:
- Line 26: The /bookmarks route in Router.tsx is unprotected; wrap the Bookmarks
element with your auth guard so unauthenticated users are redirected to /login
(e.g., replace <Route path="/bookmarks" element={<Bookmarks />} /> with
something like element={<ProtectedRoute><Bookmarks/></ProtectedRoute>} or use a
component that returns <Navigate to="/login" /> when auth fails). Ensure the
guard you use (ProtectedRoute/PrivateRoute/AuthGuard) checks the same auth state
your app uses and performs the redirect to "/login" so Bookmarks.tsx no longer
has to handle access control via API errors.
---
Nitpick comments:
In `@backend/models/User.js`:
- Around line 21-22: Update the Mongoose schema for the User model to validate
githubUsername and avatarUrl: add a regex validator to the githubUsername field
(match GitHub rules: alphanumeric and hyphens, cannot start/end with hyphen, max
39 chars) and add a URL validator or URL-regex to the avatarUrl field (ensure
well-formed http/https URLs), using the existing githubUsername and avatarUrl
schema keys (e.g., in UserSchema or the exported User model) so invalid values
are rejected at the schema level and provide clear validation error messages.
In `@backend/routes/bookmarks.js`:
- Around line 6-8: The ensureAuth middleware currently uses an overly defensive
check (req.isAuthenticated && req.isAuthenticated()); simplify it to the
standard Passport pattern by calling req.isAuthenticated() directly in
ensureAuth and returning next() when true, otherwise return the 401 JSON
response; update the function named ensureAuth to remove the redundant truthy
check and use only req.isAuthenticated() to determine authentication.
- Around line 14-15: Change all uses of the user's ID in this route file to the
same accessor: replace any occurrences of req.user.id with req.user._id so the
console.log and database queries are consistent (e.g., the GET handler where you
call User.findById(req.user._id)). Apply the same consistent change in the POST
and DELETE bookmark handlers (the handlers that reference req.user.id around the
POST and DELETE routes) so all lookups and logs use req.user._id.
- Around line 70-72: Add error logging to the DELETE '/:id' handler and stop
returning raw err.message from all three route handlers (GET '/', POST '/',
DELETE '/:id'); instead, log the full error using the same logger used by the
other routes (e.g., logger.error or console.error) and return a sanitized JSON
response like { message: 'Failed to ...' } without internal error details.
Update the GET and POST handlers to mirror this pattern: log the caught error
and respond with a generic message rather than sending err.message to the
client.
- Line 14: Replace the console.log debug calls in bookmarks.js with the
project's centralized logger: import the existing logger (same export used in
server.js) at the top of the file, then change the console.log on the GET
/api/bookmarks route (the line referencing req.isAuthenticated and req.user.id)
to logger.info or logger.debug and format the message to include the same
context; likewise replace other console.log occurrences (the ones at the
locations that currently reference request/auth details) with logger.info/debug,
and add a logger.error call where errors are handled (the spot mentioned around
line 70) instead of console.error so errors are logged at the error level. Make
sure to reference the exact names used in the file (req.isAuthenticated,
req.user) and keep messages descriptive and consistent with the project logger
usage.
- Around line 27-30: The route currently only checks presence of githubUsername;
add format validation for githubUsername and optional avatarUrl: validate
githubUsername from req.body against GitHub rules (only alphanumeric and
hyphens, cannot start with a hyphen, max 39 characters) and return
res.status(400).json({ message: 'invalid githubUsername' }) on failure; if
avatarUrl is provided validate it is a well-formed URL (use URL constructor or a
robust URL validator) and return res.status(400).json({ message: 'invalid
avatarUrl' }) on failure; keep using githubUsername and avatarUrl from req.body
and update the request handler in bookmarks.js to perform these checks before
proceeding with storage.
- Line 40: Remove the redundant defensive initialization of user.bookmarks (the
line using "user.bookmarks = user.bookmarks || []"); the User schema already
provides a default empty array, so delete that assignment wherever it appears in
the bookmarks route handler (referenced as user.bookmarks) to avoid unnecessary
code and rely on the schema default.
🪄 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: ef40e08b-8f82-4c2b-92ef-45a200aec8f3
📒 Files selected for processing (9)
backend/models/User.jsbackend/routes/bookmarks.jsbackend/server.jssrc/Routes/Router.tsxsrc/components/ActivityFeed.tsxsrc/pages/Activity.tsxsrc/pages/Bookmarks.tsxsrc/pages/Login/Login.tsxsrc/pages/Signup/Signup.tsx
| app.use(bodyParser.json()); | ||
| app.use(session({ | ||
| secret: process.env.SESSION_SECRET, | ||
| secret: process.env.SESSION_SECRET || 'dev-secret', |
Contributor
There was a problem hiding this comment.
Prevent hardcoded session secret in production.
The fallback 'dev-secret' is a security risk if the SESSION_SECRET environment variable is not set in production. A weak or known secret enables session forgery and hijacking attacks.
🔒 Enforce SESSION_SECRET in production
Option 1 (recommended): Fail fast in production if secret is missing:
app.use(session({
- secret: process.env.SESSION_SECRET || 'dev-secret',
+ secret: process.env.SESSION_SECRET || (() => {
+ if (process.env.NODE_ENV === 'production') {
+ throw new Error('SESSION_SECRET must be set in production');
+ }
+ logger.warn('Using dev-secret for session - DO NOT use in production');
+ return 'dev-secret';
+ })(),
resave: false,
saveUninitialized: false,Option 2 (simpler): Require SESSION_SECRET always:
- secret: process.env.SESSION_SECRET || 'dev-secret',
+ secret: process.env.SESSION_SECRET,And document in README/env.example that developers must set SESSION_SECRET=dev-secret locally.
📝 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
| secret: process.env.SESSION_SECRET || 'dev-secret', | |
| secret: process.env.SESSION_SECRET, |
🤖 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/server.js` at line 38, The session secret currently falls back to a
hardcoded 'dev-secret' (secret: process.env.SESSION_SECRET || 'dev-secret')
which is unsafe; change the logic to require SESSION_SECRET in production by
throwing an error if process.env.NODE_ENV === 'production' and
process.env.SESSION_SECRET is falsy, or remove the default entirely and always
use process.env.SESSION_SECRET so the app fails fast when missing; update the
code paths that reference secret to use only process.env.SESSION_SECRET (or the
validated value) and add a startup check that validates SESSION_SECRET when the
server initializes.
Comment on lines
+39
to
+45
| const data = await res.json(); | ||
| const found = (data.bookmarks || []).some( | ||
| (b: any) => b.githubUsername.toLowerCase() === username.toLowerCase() | ||
| ); | ||
| setIsBookmarked(!!found); | ||
| } catch (err) { | ||
| // ignore |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and show the relevant code in ActivityFeed.tsx
FILE="src/components/ActivityFeed.tsx"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "---- $FILE (lines 25-80) ----"
nl -ba "$FILE" | sed -n '25,80p'
echo "---- grep for catch bindings in ActivityFeed.tsx ----"
rg -n "catch\s*\([^)]*\)" "$FILE" || true
echo "---- grep for 'any' in ActivityFeed.tsx ----"
rg -n "\bany\b" "$FILE" || trueRepository: GitMetricsLab/github_tracker
Length of output: 190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/components/ActivityFeed.tsx"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "---- $FILE (lines 25-80) ----"
cat -n "$FILE" | sed -n '25,80p'
echo "---- grep for catch bindings in ActivityFeed.tsx ----"
rg -n "catch\s*\([^)]*\)" "$FILE" || true
echo "---- grep for 'any' in ActivityFeed.tsx ----"
rg -n "\bany\b" "$FILE" || trueRepository: GitMetricsLab/github_tracker
Length of output: 2479
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/components/ActivityFeed.tsx"
echo "---- $FILE (lines 80-160) ----"
cat -n "$FILE" | sed -n '80,160p'
echo "---- grep for 'any' in $FILE ----"
rg -n "\bany\b" "$FILE" || trueRepository: GitMetricsLab/github_tracker
Length of output: 3425
Type bookmark payload and remove unused catch parameter in the bookmark check
- Replace
(b: any)with a typed bookmark payload sogithubUsernameis properly typed. - In the
checkBookmarkpath,catch (err)declareserrbut it’s unused (the block just ignores), so switch tocatch {}.
Suggested fix
+type BookmarkPayload = { bookmarks?: Array<{ githubUsername: string }> };
+
...
- const data = await res.json();
+ const data: BookmarkPayload = await res.json();
const found = (data.bookmarks || []).some(
- (b: any) => b.githubUsername.toLowerCase() === username.toLowerCase()
+ (b) => b.githubUsername.toLowerCase() === username.toLowerCase()
);
setIsBookmarked(!!found);
- } catch (err) {
+ } catch {
// ignore
}📝 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 data = await res.json(); | |
| const found = (data.bookmarks || []).some( | |
| (b: any) => b.githubUsername.toLowerCase() === username.toLowerCase() | |
| ); | |
| setIsBookmarked(!!found); | |
| } catch (err) { | |
| // ignore | |
| type BookmarkPayload = { bookmarks?: Array<{ githubUsername: string }> }; | |
| const data: BookmarkPayload = await res.json(); | |
| const found = (data.bookmarks || []).some( | |
| (b) => b.githubUsername.toLowerCase() === username.toLowerCase() | |
| ); | |
| setIsBookmarked(!!found); | |
| } catch { | |
| // ignore | |
| } |
🧰 Tools
🪛 ESLint
[error] 41-41: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 44-44: 'err' 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/ActivityFeed.tsx` around lines 39 - 45, In ActivityFeed.tsx,
update the bookmark check to use a proper typed payload instead of (b: any) and
remove the unused catch parameter: define or import an interface like Bookmark {
githubUsername: string } and change the some callback to (b: Bookmark) =>
b.githubUsername.toLowerCase() === username.toLowerCase(); also replace the
unused catch (err) { /* ignore */ } with catch { } to avoid an unused variable;
keep the existing setIsBookmarked(!!found) logic intact.
Comment on lines
63
to
65
| const res = await fetch( | ||
| `https://api.github.com/users/${username}/events` | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/components/ActivityFeed.tsx"
echo "== Lines 50-90 =="
nl -ba "$FILE" | sed -n '50,90p'
echo
echo "== Search for encodeURIComponent(username) in repo =="
rg -n "encodeURIComponent\s*\(\s*username\s*\)" .
echo
echo "== Search for username usage/sanitization in this file =="
rg -n "\busername\b" "$FILE"Repository: GitMetricsLab/github_tracker
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/components/ActivityFeed.tsx"
echo "== Lines 50-90 =="
cat -n "$FILE" | sed -n '50,90p'
echo
echo "== Search for encodeURIComponent(username) in repo =="
rg -n "encodeURIComponent\s*\(\s*username\s*\)" .
echo
echo "== Search for username usage/sanitization in this file =="
rg -n "\busername\b" "$FILE"Repository: GitMetricsLab/github_tracker
Length of output: 2740
Encode username when building the GitHub Events URL
username is interpolated raw into https://api.github.com/users/${username}/events; encode it to handle special characters and avoid malformed requests.
Suggested fix
- const res = await fetch(
- `https://api.github.com/users/${username}/events`
- );
+ const res = await fetch(
+ `https://api.github.com/users/${encodeURIComponent(username)}/events`
+ );📝 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 res = await fetch( | |
| `https://api.github.com/users/${username}/events` | |
| ); | |
| const res = await fetch( | |
| `https://api.github.com/users/${encodeURIComponent(username)}/events` | |
| ); |
🤖 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/ActivityFeed.tsx` around lines 63 - 65, The GitHub events URL
is built by interpolating the raw username into the fetch call, which can
produce malformed requests for special characters; update the fetch invocation
in the ActivityFeed component to encode the username (use
encodeURIComponent(username)) when constructing
`https://api.github.com/users/.../events` so the request is safe for all
inputs—locate the fetch call that references the `username` variable in
ActivityFeed (or any helper like fetchEvents) and replace the direct
interpolation with an encoded username.
Comment on lines
+18
to
+20
| const themeContext = useContext(ThemeContext) as ThemeContextType; | ||
| const { mode } = themeContext; | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant parts of the file with line numbers
sed -n '1,120p' src/pages/Bookmarks.tsx | nl -ba | sed -n '1,120p'
# Focus on exact referenced lines/nearby context
sed -n '10,40p' src/pages/Bookmarks.tsx | nl -ba
sed -n '40,70p' src/pages/Bookmarks.tsx | nl -ba
# Check whether `mode` is referenced anywhere
rg -n "\bmode\b" src/pages/Bookmarks.tsx || true
# Check whether catch parameters are referenced inside those catch blocks
rg -n "catch\s*\(([^)]*)\)" src/pages/Bookmarks.tsx || trueRepository: GitMetricsLab/github_tracker
Length of output: 115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/pages/Bookmarks.tsx"
# Show the relevant file sections with line numbers via awk
awk 'NR>=1 && NR<=120 {printf "%4d:%s\n", NR, $0}' "$file" | sed -n '1,120p'
awk 'NR>=10 && NR<=45 {printf "%4d:%s\n", NR, $0}' "$file"
awk 'NR>=40 && NR<=70 {printf "%4d:%s\n", NR, $0}' "$file"
# Is `mode` referenced anywhere?
rg -n "\bmode\b" "$file" || true
# Catch blocks and their parameter lists
rg -n "catch\s*\(([^)]*)\)" "$file" || true
# If any `catch (...) {` uses a named param later, search for the param identifiers
# (We’ll collect identifiers found in catch params and then grep for each.)
params=$(rg -o --no-filename "catch\s*\(([^)]*)\)" "$file" | sed -E 's/^catch\s*\(([^)]*)\).*/\1/' | tr -d ' ' || true)
echo "Catch params raw:"
echo "$params" | head -n 20
# Heuristic: extract simple identifiers from catch params (e.g., err) and search for them.
# Handles patterns like `err: unknown` or `err`.
echo "$params" | rg -o "([A-Za-z_][A-Za-z0-9_]*)" | sort -u | while read -r ident; do
[ -n "$ident" ] || continue
echo "Searching identifier '$ident' usage:"
rg -n "\\b$ident\\b" "$file" || true
doneRepository: GitMetricsLab/github_tracker
Length of output: 7919
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/pages/Bookmarks.tsx"
echo "mode usages:"
rg -n "\bmode\b" "$file" || true
echo "err/unknown usages:"
rg -n "\berr\b" "$file" || true
rg -n "\bunknown\b" "$file" || trueRepository: GitMetricsLab/github_tracker
Length of output: 242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/pages/Bookmarks.tsx"
echo "useContext usages:"
rg -n "\buseContext\b" "$file" || true
echo "ThemeContext usages:"
rg -n "\bThemeContext\b" "$file" || true
echo "ThemeContextType usages:"
rg -n "\bThemeContextType\b" "$file" || trueRepository: GitMetricsLab/github_tracker
Length of output: 617
Remove unused mode and unused catch bindings in src/pages/Bookmarks.tsx
mode (line 19) and the catch parameters (err: unknown on line 29, err on line 48) are unused. Remove the unused ThemeContext destructuring (and related imports) and switch catch (err...) to catch to pass ESLint consistently.
Suggested fix
-import React, { useEffect, useState, useContext } from 'react';
-import { ThemeContext } from '../context/ThemeContext';
-import type { ThemeContextType } from '../context/ThemeContext';
+import React, { useEffect, useState } from 'react';
...
- const themeContext = useContext(ThemeContext) as ThemeContextType;
- const { mode } = themeContext;
...
- } catch (err: unknown) {
+ } catch {
setError('Failed to load bookmarks');
} finally {
...
- } catch (err) {
+ } catch {
setError('Unable to remove bookmark');
}🧰 Tools
🪛 ESLint
[error] 19-19: 'mode' is assigned a value 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/pages/Bookmarks.tsx` around lines 18 - 20, Remove the unused ThemeContext
destructuring and unused type: delete the lines that call
useContext(ThemeContext) as ThemeContextType and the subsequent const { mode } =
themeContext (and remove any now-unused imports ThemeContext and
ThemeContextType if present). Also replace any try/catch blocks that use unused
catch bindings (e.g., catch (err: unknown) and catch (err)) with bare catch
blocks (catch) so ESLint no longer flags unused variables; keep existing error
handling logic inside the catch body unchanged.
| <Route path="/contributors" element={<Contributors />} /> | ||
| <Route path="/contributor/:username" element={<ContributorProfile />} /> | ||
| <Route path="/activity" element={<Activity />} /> | ||
| <Route path="/bookmarks" element={<Bookmarks />} /> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether an existing route guard/auth wrapper already exists to reuse.
rg -n --type=tsx -C3 '\b(ProtectedRoute|PrivateRoute|RequireAuth|AuthGuard)\b|isAuthenticated|currentUser|/login' srcRepository: GitMetricsLab/github_tracker
Length of output: 100
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect Router.tsx around the referenced line
echo "---- Router.tsx (context around /bookmarks) ----"
if [ -f "src/Routes/Router.tsx" ]; then
nl -ba src/Routes/Router.tsx | sed -n '1,120p' | sed -n '15,45p'
else
echo "Missing: src/Routes/Router.tsx" >&2
exit 1
fi
echo "---- Search for existing auth/route guards in TSX files ----"
rg -n -g'*.{ts,tsx}' -C3 '\b(ProtectedRoute|PrivateRoute|RequireAuth|AuthGuard)\b|isAuthenticated|currentUser|/login' src || true
echo "---- Search specifically for route protection patterns (auth + redirect) ----"
rg -n -g'*.{ts,tsx}' -C3 '(Navigate\s+to=|redirect|/login|useAuth|useUser|auth|session)' src/Routes src || trueRepository: GitMetricsLab/github_tracker
Length of output: 182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Check file exists ----"
ls -la src/Routes/Router.tsx || true
echo "---- Router.tsx (context around /bookmarks) ----"
if [ -f "src/Routes/Router.tsx" ]; then
cat -n src/Routes/Router.tsx | sed -n '15,45p'
fi
echo "---- Search for existing auth/route guards in TS/TSX files ----"
rg -n -g'*.ts' -g'*.tsx' -C3 '\b(ProtectedRoute|PrivateRoute|RequireAuth|AuthGuard)\b|isAuthenticated|currentUser|/login' src || true
echo "---- Search specifically for route protection patterns (auth + redirect) ----"
rg -n -g'*.ts' -g'*.tsx' -C3 '(Navigate\s+to=|redirect|/login|useAuth|useUser|auth|session)' src/Routes src || trueRepository: GitMetricsLab/github_tracker
Length of output: 14988
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Inspect src/App.tsx (routing/auth gating) ----"
cat -n src/App.tsx | sed -n '1,120p'
echo "---- Inspect src/pages/Bookmarks/* (if exists) ----"
ls -la src/pages/Bookmarks || true
fd -t f "Bookmarks.*" src/pages || true
fd -t f "bookmarks" src/pages -i || true
# Show Bookmarks route component if present
if [ -d "src/pages/Bookmarks" ]; then
fd -t f . src/pages/Bookmarks --max-depth 2 -i || true
fi
# fallback: search for "function Bookmarks" or "const Bookmarks"
rg -n -g'*.{ts,tsx}' 'function\s+Bookmarks|const\s+Bookmarks|export\s+default\s+Bookmarks' src/pages src/components src -S || trueRepository: GitMetricsLab/github_tracker
Length of output: 2104
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Router.tsx (full) ----"
cat -n src/Routes/Router.tsx
echo "---- Bookmarks.tsx (key sections) ----"
cat -n src/pages/Bookmarks.tsx | sed -n '1,200p'
echo "---- Search for routing-level auth wrappers/components ----"
rg -n -g'*.{ts,tsx}' '\b(ProtectedRoute|PrivateRoute|RequireAuth|AuthGuard)\b' src || true
echo "---- Search for auth checks / redirects used in pages/components ----"
rg -n -g'*.{ts,tsx}' 'Navigate\s+to=|/login|redirect|withCredentials|session|token|useGitHubAuth|useAuth|isAuthenticated|currentUser' src || true
echo "---- Auth usage within Bookmarks.tsx ----"
rg -n -g'*.tsx' 'useGitHubAuth|token|authError|Navigate\s+to=|/login|withCredentials|axios|fetch' src/pages/Bookmarks.tsx || trueRepository: GitMetricsLab/github_tracker
Length of output: 9650
Protect the /bookmarks route with an auth guard (redirect to /login).
src/Routes/Router.tsx renders /bookmarks directly to <Bookmarks /> with no ProtectedRoute/PrivateRoute/AuthGuard-style wrapper in src. src/pages/Bookmarks.tsx calls /api/bookmarks and only shows “Failed to load bookmarks” on failure, so unauthenticated users get an API-error experience instead of access control.
🤖 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/Routes/Router.tsx` at line 26, The /bookmarks route in Router.tsx is
unprotected; wrap the Bookmarks element with your auth guard so unauthenticated
users are redirected to /login (e.g., replace <Route path="/bookmarks"
element={<Bookmarks />} /> with something like
element={<ProtectedRoute><Bookmarks/></ProtectedRoute>} or use a component that
returns <Navigate to="/login" /> when auth fails). Ensure the guard you use
(ProtectedRoute/PrivateRoute/AuthGuard) checks the same auth state your app uses
and performs the redirect to "/login" so Bookmarks.tsx no longer has to handle
access control via API errors.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/index.css (1)
5-7: ⚡ Quick winRespect
prefers-reduced-motionfor global smooth scrolling.Global smooth scrolling can conflict with reduced-motion accessibility preferences; add an override for users who opt out of motion.
♿ Suggested refinement
html { scroll-behavior: smooth; } + +@media (prefers-reduced-motion: reduce) { + html { + scroll-behavior: auto; + } +}🤖 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/index.css` around lines 5 - 7, The global smooth-scrolling rule on the html selector should respect users' prefers-reduced-motion setting; add a media query for `@media` (prefers-reduced-motion: reduce) that overrides html { scroll-behavior: smooth; } and sets scroll-behavior to auto (or unset) so users who opt out of motion won't get smooth scrolling—update the html rule and include the new media query near the existing html { scroll-behavior: smooth; } declaration.
🤖 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 `@src/components/Navbar.tsx`:
- Around line 22-23: The early return "if (!themeContext) return null;" in the
Navbar component executes before the useEffect, breaking hook order; remove or
move that guard so hooks (the useEffect defined in Navbar) always run
unconditionally, and instead gate the rendered output by returning null in the
JSX (e.g., render theme-dependent bits conditionally or use themeContext ? <...>
: null) or check themeContext inside the effect via optional chaining; ensure
the symbol Navbar keeps its useEffect call unconditional while only the
render/content is conditional on themeContext.
---
Nitpick comments:
In `@src/index.css`:
- Around line 5-7: The global smooth-scrolling rule on the html selector should
respect users' prefers-reduced-motion setting; add a media query for `@media`
(prefers-reduced-motion: reduce) that overrides html { scroll-behavior: smooth;
} and sets scroll-behavior to auto (or unset) so users who opt out of motion
won't get smooth scrolling—update the html rule and include the new media query
near the existing html { scroll-behavior: smooth; } declaration.
🪄 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: c73b400b-5ea2-4708-a55a-f412a61c7db9
📒 Files selected for processing (4)
src/components/Hero.tsxsrc/components/Navbar.tsxsrc/index.csssrc/pages/Home/Home.tsx
Comment on lines
22
to
23
| if (!themeContext) return null; | ||
|
|
Contributor
There was a problem hiding this comment.
Fix React hook order in src/components/Navbar.tsx (null guard runs before useEffect).
Navbar returns on !themeContext at lines 22–23, so the useEffect registered at lines 34–63 is only called conditionally, violating react-hooks/rules-of-hooks.
🔧 Proposed fix
const Navbar: React.FC = () => {
const [isOpen, setIsOpen] = useState(false);
const [activeSection, setActiveSection] = useState<string>("");
const location = useLocation();
const themeContext = useContext(ThemeContext);
-
- if (!themeContext) return null;
-
- const { toggleTheme, mode } = themeContext;
const navLinkStyles = (isActive: boolean) =>
`px-4 py-2 rounded-xl text-sm lg:text-base font-semibold transition-all duration-300 ${isActive
? "text-blue-600 bg-blue-100 dark:bg-blue-900/40 shadow-sm"
: "text-slate-700 dark:text-gray-300 hover:text-blue-500"
}`;
const closeMenu = () => setIsOpen(false);
useEffect(() => {
@@
}, [location.pathname]);
+
+ if (!themeContext) return null;
+ const { toggleTheme, mode } = themeContext;🤖 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` around lines 22 - 23, The early return "if
(!themeContext) return null;" in the Navbar component executes before the
useEffect, breaking hook order; remove or move that guard so hooks (the
useEffect defined in Navbar) always run unconditionally, and instead gate the
rendered output by returning null in the JSX (e.g., render theme-dependent bits
conditionally or use themeContext ? <...> : null) or check themeContext inside
the effect via optional chaining; ensure the symbol Navbar keeps its useEffect
call unconditional while only the render/content is conditional on themeContext.
Author
|
Please accept my pr for this.5 checks have been successfully passed. Screenshot is also attached with pr description. Please accept this change @mehul-m-prajapati |
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.
PR Title
Add user-tracking, bookmarks API, and client UI for saving GitHub profiles
Summary
Closes: #546
Adds interactive username tracking on the Activity page and a Bookmarks feature so authenticated users can save/remove GitHub profiles. Includes backend API endpoints, MongoDB persistence, and frontend UI with loading/error states.
Changes (key files)
Backend:
backend/models/User.js: add bookmarks array to user schema.
backend/routes/bookmarks.js: new GET / POST / DELETE bookmarks routes (session-auth protected) + debug logging.
backend/server.js: mount bookmarks route; expanded allowedOrigins and session cookie options to improve local-dev CORS/session behavior.
Frontend:
src/pages/Activity.tsx: add username input + Track Activity button, responsive layout.
src/components/ActivityFeed.tsx: loading / empty / error states; Save/Saved star button calling bookmarks API; fallback backend URL.
src/pages/Bookmarks.tsx: dedicated page to list and remove saved profiles.
src/Routes/Router.tsx: add /bookmarks route.
src/pages/Login/Login.tsx and src/pages/Signup/Signup.tsx: include credentials in auth requests and align signup validation with backend rules.
Why
Makes the Activity page interactive for arbitrary GitHub users.
Lets authenticated users persist frequently-tracked profiles for quick access.
Summary by CodeRabbit
New Features
Bug Fixes