Feature/auth enhancement#574
Open
arpit2006 wants to merge 7 commits into
Open
Conversation
✅ 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 5 minutes and 32 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 (11)
📝 WalkthroughWalkthroughThis PR adds a complete Community Discussions feature with backend REST API and frontend page, introducing file-backed JSON authentication as a fallback when MongoDB is unavailable. It includes discussion CRUD, likes, comments, filtering, sorting, and refactored authentication pages using a new shared layout component. ChangesCommunity Discussions Feature
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Browser)
participant AuthAPI as Auth Routes
participant UserStore as User JSON Store
participant SessionMW as Session Middleware
participant DiscAPI as Discussions Routes
participant DiscStore as Discussions JSON Store
Client->>AuthAPI: POST /signup (email, password)
AuthAPI->>UserStore: Read users.json (under lock)
alt User exists
AuthAPI->>Client: 409 Conflict
else New user
AuthAPI->>UserStore: Write new user + bcrypt hash (lock+rename)
AuthAPI->>SessionMW: Set req.session.authUser
AuthAPI->>Client: 201 Created + Set-Cookie
end
Client->>AuthAPI: POST /login (email, password)
AuthAPI->>UserStore: Read users.json (under lock)
alt User found + password matches
AuthAPI->>SessionMW: Set req.session.authUser
AuthAPI->>Client: 200 OK + Set-Cookie
else Invalid credentials
AuthAPI->>Client: 401 Unauthorized
end
Client->>SessionMW: All requests with session cookie
SessionMW->>SessionMW: Map req.session.authUser → req.user
Client->>DiscAPI: GET /api/discussions?search=query&category=bug&sort=trending
DiscAPI->>DiscStore: Read discussions.json (under mutex)
DiscAPI->>DiscStore: Apply filter + compute trending score
DiscAPI->>Client: 200 + filtered+sorted items + categories
Client->>DiscAPI: POST /api/discussions (title, body, category, tags)
Note over DiscAPI: Requires req.user (401 if missing)
DiscAPI->>DiscStore: Prepend new discussion (UUID id, timestamps, under mutex)
DiscAPI->>Client: 201 + new public discussion
Client->>DiscAPI: POST /api/discussions/:id/likes
DiscAPI->>DiscStore: Toggle user's like in discussion.likes array + update updatedAt
DiscAPI->>Client: 200 + updated public discussion
Client->>DiscAPI: POST /api/discussions/:id/comments (text)
DiscAPI->>DiscStore: Append comment (UUID id, author, timestamp, under mutex)
DiscAPI->>Client: 201 + updated public discussion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 |
…06/github_tracker into feature/auth-enhancement
![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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/server.js (1)
17-31:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix broken CORS block (syntax) and align auth fallback with Mongo connection failures.
backend/server.jslines 17-31: theapp.use(cors({ ...block is not closed beforeconst allowedOrigins = ..., producing aSyntaxErrorand preventing the server from booting.backend/server.js: whenmongoose.connect(...)fails butMONGO_URIis still set,startServer()still runs; howeverbackend/routes/auth.jsselects auth mode viaBoolean(process.env.MONGO_URI)and the server’s file-backed auth middleware only registers when!process.env.MONGO_URI, so auth won’t fall back during Mongo outages.
🧹 Nitpick comments (5)
src/pages/Community/Community.tsx (4)
213-226: ⚡ Quick winConsider adding a confirmation dialog before deletion.
The delete handler immediately removes the discussion without user confirmation. Adding a confirmation step would prevent accidental deletions and improve the user experience, especially for valuable content.
💡 Suggested improvement
const handleDelete = async (discussionId: string) => { + if (!window.confirm("Are you sure you want to delete this discussion? This action cannot be undone.")) { + return; + } + try { await axios.delete(`${apiBase}/api/discussions/${discussionId}`, { withCredentials: true });🤖 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/Community/Community.tsx` around lines 213 - 226, Add a user confirmation step inside the handleDelete function before calling axios.delete: when handleDelete(discussionId) is invoked, show a confirmation (e.g., window.confirm or trigger your existing Modal/Confirm component) and only proceed with the axios.delete, loadDiscussions(), and setMessage("Discussion deleted.") if the user confirms; otherwise abort without making the HTTP call. Keep the existing auth/error handling intact (the try/catch and axios.isAxiosError check) and place the confirmation check at the very start of handleDelete so accidental deletions are prevented.
107-107: ⚡ Quick winRemove unused
filteredDiscussionsor implement actual filtering.The
filteredDiscussionsmemo simply returnsdiscussionsunchanged, which provides no value and is misleading. Either implement the filtering logic or remove this memo and usediscussionsdirectly throughout the component.♻️ Proposed fix
- const filteredDiscussions = useMemo(() => discussions, [discussions]); - const trendingDiscussions = useMemo( - () => [...filteredDiscussions] + () => [...discussions] .sort((left, right) => { const leftScore = left.likesCount * 2 + left.commentsCount; const rightScore = right.likesCount * 2 + right.commentsCount; return rightScore - leftScore; }) .slice(0, 3), - [filteredDiscussions] + [discussions] );🤖 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/Community/Community.tsx` at line 107, The useMemo call creating filteredDiscussions simply returns discussions unchanged (const filteredDiscussions = useMemo(() => discussions, [discussions])) and should be removed or replaced with real filtering; update the Community component to either remove filteredDiscussions and use discussions directly wherever filteredDiscussions is referenced, or implement the intended filter logic inside the useMemo (e.g., derive a filtered array from discussions based on component state/props) and ensure the dependency array includes the filter inputs so the memo is correct.
254-261: ⚖️ Poor tradeoffConsider optimistic UI updates for comment interactions.
Similar to the like handler, posting a comment triggers a full reload of all discussions. Consider appending the new comment optimistically to local state and only fetching the updated discussion from the server, or using a WebSocket/polling strategy for real-time updates without full page refreshes.
🤖 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/Community/Community.tsx` around lines 254 - 261, Currently the comment POST handler uses axios.post then calls loadDiscussions() which reloads all discussions; replace this with an optimistic update by appending a temporary comment object to the local discussions state for the specific discussion, then clear the draft with setCommentDrafts and call the API (axios.post) in background; on success replace the temp comment with the server response (or fetch only that discussion), and on failure remove the temp comment and show an error. Locate the POST logic around axios.post, the state updater setCommentDrafts and the loadDiscussions call and modify it to: create a temp comment (using discussionId and commentText), update the discussions state to include it, clear the draft, send the axios.post, then reconcile the response by updating that discussion’s comments or reverting on error.
228-240: ⚖️ Poor tradeoffConsider optimistic UI updates for like interactions.
Currently, liking a discussion triggers a full reload of all discussions via
loadDiscussions(). This creates unnecessary network traffic and may cause UI flicker. Consider implementing optimistic updates by immediately toggling the like state in local state, then reconciling with the server response. This would provide instant feedback and reduce API load.🤖 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/Community/Community.tsx` around lines 228 - 240, Refactor handleLike to perform an optimistic UI update instead of calling loadDiscussions(): immediately toggle the liked state and update like count in the local discussions state (the state modified by loadDiscussions) for the discussionId, then fire the axios POST to `${apiBase}/api/discussions/${discussionId}/likes` with withCredentials; on success keep the optimistic state, on error (including axios.isAxiosError 401) revert the local change and call setIsAuthenticated(false)/setMessage("Sign in to like discussions.") for 401 or setMessage("Unable to update like state.") for other failures. Keep references to handleLike, loadDiscussions, setIsAuthenticated, and setMessage so reviewers can find the updated logic and ensure reconciliation paths revert state consistently.backend/models/Discussion.js (1)
3-21: ⚡ Quick winKeep the Mongoose schema aligned with the request validator.
backend/validators/discussionValidator.jscurrently enforcescomment.textmin length,categorymin length, and a max of 6 tags, but those bounds are missing here. Any Mongo write path that bypassesvalidateRequest(...)can persist documents the API later rejects. Mirror the same limits in the schema so both persistence modes share one contract.Also applies to: 40-55
🤖 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/Discussion.js` around lines 3 - 21, The Mongoose schema (CommentSchema) is missing the request-validator bounds: add minlength for text to match discussionValidator (e.g., text.minlength), enforce category's minlength on the Discussion schema, and limit tags length to max 6 (use tags: [{ type: String, trim: true }], validate: v => v.length <= 6). Update CommentSchema and the related Discussion schema (the block referenced at lines ~40-55) to mirror the same minlength/max constraints from backend/validators/discussionValidator.js so DB writes cannot bypass the request-level rules.
🤖 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/data/users.json`:
- Around line 4-7: The seeded auth store in backend/data/users.json contains
real PII and a reusable password hash (fields id, username, email, password);
remove these real values and replace them with clearly fake/demo data (e.g., a
dummy UUID, username "demo-user", a non-real email like "user@example.com", and
a non-functional placeholder password/hash) or remove the hardcoded user
entirely and instead generate the user at test/demo setup time; ensure no real
personal emails or production password hashes remain in users.json and update
any tests/fixtures that depend on this seeded record to use the new fake/demo
user or dynamic creation.
In `@backend/routes/auth.js`:
- Around line 163-169: Change the two different 401 responses into a single
generic failure message so email presence can't be inferred: in the login route
handler in backend/routes/auth.js, replace both res.status(401).json({ message:
'Email is invalid ' }) and res.status(401).json({ message: 'Invalid password' })
with the same payload (e.g., res.status(401).json({ message: 'Invalid email or
password' })); keep the existing user lookup (user) and password check (isMatch
via bcrypt.compare) logic but ensure both failure paths (user missing or
!isMatch) return the identical response.
- Around line 60-69: The catch in readUsersUnlocked currently swallows
JSON.parse errors and returns an empty array, which can cause corruption to be
overwritten on next signup; change the error handling in readUsersUnlocked so
that on JSON.parse failure it throws a descriptive error (including the
usersFile path and the original exception) instead of returning [], allowing
callers (and ensureUsersFileUnlocked or route handlers) to surface a 5xx error
rather than treating the store as empty.
- Around line 172-177: In the non-Mongo /login flow, avoid session fixation by
calling req.session.regenerate (or the app's session rotation helper) before
assigning req.session.authUser and req.user in createSessionUser; update the
login handler to invoke req.session.regenerate((err) => { if err -> handle/log
and return 500 }, then call createSessionUser(req, user), set req.user, and send
the 200 JSON response—ensure errors from regenerate are handled and the response
is only sent after successful regeneration and session assignment.
In `@backend/routes/discussions.js`:
- Around line 36-39: The writeStore function currently writes directly to
dataFile which can leave discussions.json partially written; change writeStore
(and keep using ensureDataFile) to write JSON to a temporary file (e.g.,
dataFile + '.tmp' or a unique tmp name in the same directory) then fs.rename
that temp file to dataFile so the rename is atomic and readers never see a
partial file; mirror the temp-file-plus-rename approach used in
backend/routes/auth.js and ensure the temp file is written with utf8 and closed
before renaming.
- Around line 133-138: The handler currently calls getCommunityIdentity(req)
unconditionally which may write req.session and create sessions for anonymous
GETs; change the router.get('/') logic to only call getCommunityIdentity when
req.user is present, otherwise read an existing guest id from req.session
without invoking getCommunityIdentity (or set currentUserId to null) so you
don't modify req.session; specifically adjust how currentUserId is computed in
the router.get callback to avoid calling getCommunityIdentity for anonymous
requests.
In `@backend/server.js`:
- Around line 68-79: The auth fallback is still gated by
Boolean(process.env.MONGO_URI) in backend/routes/auth.js even when
mongoose.connect fails; change the initialization to derive auth mode from
actual connection success instead: add a shared flag (e.g., mongooseConnected)
or pass an explicit fallback parameter into the routes initializer when calling
startServer so that startServer (or the code that mounts backend/routes/auth.js)
can choose the JSON fallback when mongoose.connect rejects; update the
mongoose.connect .then() to set the flag to true and .catch() to set it false
before invoking startServer, and modify backend/routes/auth.js to read that flag
(or accept the passed parameter) instead of Boolean(process.env.MONGO_URI).
In `@src/App.tsx`:
- Line 20: The App.tsx change removed parent centering from the main container
which exposes that Home's top-level wrapper (the Home component's root <div> in
Home.tsx) is empty and no longer vertically centers content; update the Home
component's root wrapper to restore vertical spacing by adding appropriate
layout utility classes (for example min-h-screen and flex with items-center and
justify-center or equivalent padding/margins) so the home page matches the
intended centering/alignment when rendered inside the modified main in App.tsx.
In `@src/components/AuthShell.tsx`:
- Around line 63-67: Labels use fixed pale tokens (text-cyan-300/90 and
text-fuchsia-300/90) that are too light on the light card variant; update the
JSX in the highlights mapping to choose darker accent tokens when mode ===
"light" (e.g., use text-cyan-600/90 or text-fuchsia-600/90 for light mode)
instead of the current static classes, apply the same change to the second
occurrence around lines 72-73, and keep the selection tied to the existing mode
variable so highlight.title rendering uses a conditional class rather than a
hardcoded token.
In `@src/pages/Community/Community.tsx`:
- Around line 371-383: The category dropdown (<select> using
value={selectedCategory} and onChange={event =>
setSelectedCategory(event.target.value)}) is missing an accessible label; add a
descriptive label so screen readers understand its purpose—either add an
aria-label like "Filter by category" to that <select> or add a <label> element
(visually hidden via your "sr-only" or equivalent class) associated with the
select to describe it (e.g., "Category" or "Filter by category").
- Around line 361-369: The search input currently uses only placeholder text and
is not accessible; update the input element (the one with value={search} and
onChange={(event) => setSearch(event.target.value)}) to include an accessible
label—either add an aria-label like aria-label="Search discussions, tags, or
categories" or add a visually hidden <label> (use the site's sr-only class)
associated via htmlFor/id—so screen readers announce the field when focused.
- Around line 120-146: The loadDiscussions callback incorrectly sets
setIsAuthenticated(true) for any 200 response and redundantly calls setError("")
after already clearing error; change it to derive authentication from the
response payload (e.g., check response.data.currentUserId,
response.data.identity?.id, or an explicit response.data.isAuthenticated flag
returned by the backend/routes/discussions.js/toPublicDiscussion logic) and only
call setIsAuthenticated(true) when an identity/flag exists; also remove the
duplicate setError("") after setting discussions/categories so error is only
cleared at the function start.
In `@src/pages/Login/Login.tsx`:
- Around line 140-146: The login feedback banner in the Login component (the JSX
that renders when the message variable is truthy) uses pastel text classes
(text-rose-300 / text-emerald-300) that are hard to read in light mode; update
the className on that div to be theme-aware using Tailwind dark: variants so
light-mode uses stronger/darker text and borders (e.g. replace text-rose-300
with text-rose-700 and add dark:text-rose-300, and similarly replace
text-emerald-300 with text-emerald-700 and add dark:text-emerald-300), and
adjust the corresponding border-*/bg-* classes to include dark: variants so the
banner reads well in both light and dark themes.
In `@src/pages/Signup/Signup.tsx`:
- Line 168: Validation and result messages use light 300-tone colors that are
too faint on light card variant; update the classNames for the inline error <p>
elements (e.g., the element rendering errors.username) and the signup
result/banner to use a darker tone (e.g., text-rose-600 for errors and
text-emerald-600 for success) or switch to theme-aware classes (conditional
className or tailwind's dark: variants) so the text is readable in light mode
while preserving current dark-mode styles.
- Line 9: The backendUrl constant in Signup.tsx currently falls back to
"http://localhost:5000" which causes production builds without VITE_BACKEND_URL
to point browsers at localhost; change the fallback to the same-origin pattern
used in Login.tsx (i.e., use import.meta.env.VITE_BACKEND_URL ||
window.location.origin for non-development) so backendUrl and any signup POSTs
use the site origin when the env var is missing; update the backendUrl
declaration and any usages of backendUrl in the Signup component accordingly.
---
Nitpick comments:
In `@backend/models/Discussion.js`:
- Around line 3-21: The Mongoose schema (CommentSchema) is missing the
request-validator bounds: add minlength for text to match discussionValidator
(e.g., text.minlength), enforce category's minlength on the Discussion schema,
and limit tags length to max 6 (use tags: [{ type: String, trim: true }],
validate: v => v.length <= 6). Update CommentSchema and the related Discussion
schema (the block referenced at lines ~40-55) to mirror the same minlength/max
constraints from backend/validators/discussionValidator.js so DB writes cannot
bypass the request-level rules.
In `@src/pages/Community/Community.tsx`:
- Around line 213-226: Add a user confirmation step inside the handleDelete
function before calling axios.delete: when handleDelete(discussionId) is
invoked, show a confirmation (e.g., window.confirm or trigger your existing
Modal/Confirm component) and only proceed with the axios.delete,
loadDiscussions(), and setMessage("Discussion deleted.") if the user confirms;
otherwise abort without making the HTTP call. Keep the existing auth/error
handling intact (the try/catch and axios.isAxiosError check) and place the
confirmation check at the very start of handleDelete so accidental deletions are
prevented.
- Line 107: The useMemo call creating filteredDiscussions simply returns
discussions unchanged (const filteredDiscussions = useMemo(() => discussions,
[discussions])) and should be removed or replaced with real filtering; update
the Community component to either remove filteredDiscussions and use discussions
directly wherever filteredDiscussions is referenced, or implement the intended
filter logic inside the useMemo (e.g., derive a filtered array from discussions
based on component state/props) and ensure the dependency array includes the
filter inputs so the memo is correct.
- Around line 254-261: Currently the comment POST handler uses axios.post then
calls loadDiscussions() which reloads all discussions; replace this with an
optimistic update by appending a temporary comment object to the local
discussions state for the specific discussion, then clear the draft with
setCommentDrafts and call the API (axios.post) in background; on success replace
the temp comment with the server response (or fetch only that discussion), and
on failure remove the temp comment and show an error. Locate the POST logic
around axios.post, the state updater setCommentDrafts and the loadDiscussions
call and modify it to: create a temp comment (using discussionId and
commentText), update the discussions state to include it, clear the draft, send
the axios.post, then reconcile the response by updating that discussion’s
comments or reverting on error.
- Around line 228-240: Refactor handleLike to perform an optimistic UI update
instead of calling loadDiscussions(): immediately toggle the liked state and
update like count in the local discussions state (the state modified by
loadDiscussions) for the discussionId, then fire the axios POST to
`${apiBase}/api/discussions/${discussionId}/likes` with withCredentials; on
success keep the optimistic state, on error (including axios.isAxiosError 401)
revert the local change and call setIsAuthenticated(false)/setMessage("Sign in
to like discussions.") for 401 or setMessage("Unable to update like state.") for
other failures. Keep references to handleLike, loadDiscussions,
setIsAuthenticated, and setMessage so reviewers can find the updated logic and
ensure reconciliation paths revert state consistently.
🪄 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: a19d2cd6-95dd-4cab-b4d6-b0a8bfdb137b
📒 Files selected for processing (14)
backend/data/discussions.jsonbackend/data/users.jsonbackend/models/Discussion.jsbackend/routes/auth.jsbackend/routes/discussions.jsbackend/server.jsbackend/validators/discussionValidator.jssrc/App.tsxsrc/Routes/Router.tsxsrc/components/AuthShell.tsxsrc/components/Navbar.tsxsrc/pages/Community/Community.tsxsrc/pages/Login/Login.tsxsrc/pages/Signup/Signup.tsx
Contributor
Author
|
@mehul-m-prajapati , Requesting Review! |
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.
Related Issue
Description
Enhanced the Login Page UI/UX to create a more modern, responsive, and visually appealing authentication experience.
Improvements Made:
How Has This Been Tested?
Screenshots
Type of Change
Summary by CodeRabbit
New Features
Style