Fix admin login token storage before validating API response#350
Open
Abhishek2005-ard wants to merge 14 commits into
Open
Fix admin login token storage before validating API response#350Abhishek2005-ard wants to merge 14 commits into
Abhishek2005-ard wants to merge 14 commits into
Conversation
📝 WalkthroughWalkthroughThis PR extends the transcript system to track Elo rating changes across backend and frontend. It updates the SaveDebateTranscript signature to include metadata and score parameters, adds EloChange field to models, reorganizes frontend routing with protected layouts, enhances JSON parsing in debate judgment flows, and updates UI styling with design tokens. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/services/transcriptservice.go (2)
972-981:⚠️ Potential issue | 🟡 MinorInconsistent
eloChangehandling compared toGetProfile.
GetDebateStatsuses a hardcoded0foreloChange(line 979), whileGetProfileinprofile_controller.go(line 257) correctly readstranscript.EloChange. This inconsistency means the two endpoints return different Elo change values for the same data.♻️ Suggested fix
// Add to recent debates recentDebates = append(recentDebates, map[string]interface{}{ "id": transcript.ID.Hex(), "topic": transcript.Topic, "result": transcript.Result, "opponent": transcript.Opponent, "debateType": transcript.DebateType, "date": transcript.CreatedAt.Format("2006-01-02T15:04:05Z07:00"), - "eloChange": 0, // TODO: Add actual Elo change tracking + "eloChange": transcript.EloChange, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/transcriptservice.go` around lines 972 - 981, GetDebateStats is populating recentDebates with a hardcoded "eloChange": 0 which is inconsistent with GetProfile; update the map population in the loop inside GetDebateStats to read the actual value from transcript.EloChange (same field used by GetProfile) instead of 0 so both endpoints return the same Elo change; locate the loop that appends to recentDebates and replace the constant with transcript.EloChange, preserving types/formatting used elsewhere.
190-210:⚠️ Potential issue | 🟠 MajorActual Elo change values are calculated but not persisted.
The
UpdateRatingscall at line 221 computesdebateRecord.RatingChangeandopponentRecord.RatingChange, but these values aren't passed toSaveDebateTranscript(lines 181-191 and 197-207), which receives hardcoded0.0. This means the persistedEloChangefield will always be zero for user-vs-user debates, even though the actual rating changes are calculated.Consider restructuring to call
UpdateRatingsfirst, then pass the computedratingChangevalues toSaveDebateTranscript.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/transcriptservice.go` around lines 190 - 210, The computed Elo changes from UpdateRatings are not being persisted because SaveDebateTranscript is still passed hardcoded 0.0; call UpdateRatings (the function that sets debateRecord.RatingChange and opponentRecord.RatingChange) before each SaveDebateTranscript invocation and pass the actual values (e.g., debateRecord.RatingChange and opponentRecord.RatingChange or the local variables returned by UpdateRatings) into SaveDebateTranscript instead of 0.0 so the EloChange/EloPersisted fields are saved for both the forUser and againstUser transcripts.
🧹 Nitpick comments (7)
frontend/src/Pages/Admin/AdminSignup.tsx (2)
29-36: Indentation inconsistency.Line 29 uses 5 spaces and lines 30-36 use 6 spaces, which is inconsistent with typical 2 or 4 space indentation conventions. The email normalization (
trim().toLowerCase()) is a good addition for handling case-insensitive email matching.🔧 Proposed fix for consistent indentation
try { - const response = await adminLogin(email.trim().toLowerCase(), password); - // Validate response before saving - if (!response || !response.accessToken || !response.admin) { - throw new Error("Invalid login response"); - } - localStorage.setItem("adminToken", response.accessToken); - localStorage.setItem("admin", JSON.stringify(response.admin)); - navigate("/admin/dashboard"); + const response = await adminLogin(email.trim().toLowerCase(), password); + // Validate response before saving + if (!response || !response.accessToken || !response.admin) { + throw new Error("Invalid login response"); + } + localStorage.setItem("adminToken", response.accessToken); + localStorage.setItem("admin", JSON.stringify(response.admin)); + navigate("/admin/dashboard"); } catch (err: any) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Pages/Admin/AdminSignup.tsx` around lines 29 - 36, Align the indentation in the admin login block to the project's standard (use consistent 2- or 4-space indentation) so the lines around the adminLogin call, the response validation (response, response.accessToken, response.admin), the localStorage.setItem calls, and the navigate("/admin/dashboard") call all use the same indentation level; keep the email normalization (email.trim().toLowerCase()) and preserve the response validation and storage logic (variables: response, adminLogin, localStorage.setItem, navigate).
16-16: File name doesn't match exported function name.The file is named
AdminSignup.tsxbut exportsAdminLogin. This creates confusion for developers navigating the codebase. Consider renaming the file toAdminLogin.tsxto match the component, or keep the function name asAdminSignupif the file name is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Pages/Admin/AdminSignup.tsx` at line 16, The exported React component name AdminLogin does not match the file name AdminSignup.tsx; either rename the file to AdminLogin.tsx or rename the exported function to AdminSignup to make them consistent. Update any imports referencing this module (search for AdminSignup / AdminLogin) so they match the new name, and ensure the default export signature (function AdminLogin / function AdminSignup) is the same as the filename to avoid confusion and import errors.frontend/src/components/Header.tsx (1)
330-331: Differentiate hovered and active drawer items.The inactive hover state now renders the same as the active state (
bg-muted text-foreground), so hovering any item makes it look selected. Keeping a distinct active treatment will preserve location context in the drawer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/Header.tsx` around lines 330 - 331, The drawer item conditional currently returns the same visual classes for active and hovered states ("bg-muted text-foreground" vs "text-muted-foreground hover:bg-muted hover:text-foreground"), so hovering an inactive item looks identical to the active item; update the inactive branch in Header.tsx (the ternary that currently yields "text-muted-foreground hover:bg-muted hover:text-foreground") to use a distinct hover style (for example replace the hover utilities with a subtler utility like "hover:bg-muted/10 hover:text-foreground" or "hover:bg-transparent hover:text-foreground") so active items keep "bg-muted text-foreground" and hovered-but-inactive items remain visually distinct.backend/controllers/transcript_controller.go (1)
109-119: Consider acceptingeloChangefrom the request payload.The handler saves with hardcoded
eloChange: 0.0. If clients should be able to specify an Elo change value, consider adding it toSaveTranscriptRequestand passing it through.♻️ Optional enhancement
type SaveTranscriptRequest struct { DebateType string `json:"debateType" binding:"required"` Topic string `json:"topic" binding:"required"` Opponent string `json:"opponent" binding:"required"` Result string `json:"result"` Messages []models.Message `json:"messages"` Transcripts map[string]string `json:"transcripts,omitempty"` + EloChange float64 `json:"eloChange"` }Then pass
req.EloChangetoSaveDebateTranscript.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/transcript_controller.go` around lines 109 - 119, The handler currently calls SaveDebateTranscript with a hardcoded eloChange (0.0); update the request and call chain to accept and pass an Elo value: add an EloChange (float64) field to the SaveTranscriptRequest struct, validate/parse it in the controller where req is built, and replace the hardcoded 0.0 with req.EloChange when calling SaveDebateTranscript so the service receives the client-provided Elo change.backend/controllers/profile_controller.go (1)
257-257: Outdated TODO comment - the field is now being tracked.The code correctly reads
transcript.EloChange, but the TODO comment is misleading. The field is now persisted, though upstream call sites currently pass0.0. Either remove the TODO or update it to clarify that the upstream calculation needs to pass the actual Elo change value.✏️ Suggested fix
- "eloChange": transcript.EloChange,// TODO: Add actual Elo change tracking + "eloChange": transcript.EloChange,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/profile_controller.go` at line 257, The inline TODO next to "eloChange": transcript.EloChange is outdated; either remove the TODO or replace it with a short clarifying comment that the Elo value is now persisted and that upstream callers must compute and pass the real Elo delta (not 0.0). Update the comment near transcript.EloChange (and any nearby comment block) to state that EloChange is persisted and that the responsibility to calculate and supply the actual change lies with the upstream caller (or remove the TODO entirely).frontend/src/components/Matchmaking.tsx (1)
38-42: Incorrect indentation on the useEffect hook.The useEffect block has inconsistent indentation compared to the rest of the component. While the logic is correct for resetting matchmaking UI state on mount/remount (e.g., after a page refresh or tab switch), the indentation should align with other hooks in the component.
🔧 Proposed formatting fix
- // Reset matchmaking UI state on page refresh - useEffect(() => { - setIsInPool(false); - setWaitTime(0); - }, []); + // Reset matchmaking UI state on page refresh + useEffect(() => { + setIsInPool(false); + setWaitTime(0); + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/Matchmaking.tsx` around lines 38 - 42, Align the indentation of the useEffect hook with the other hooks in the component: reformat the block that calls useEffect (the one that calls setIsInPool(false) and setWaitTime(0)) so its indentation matches surrounding hooks and component style, ensuring the opening useEffect line, its callback body, and the closing bracket/array are all consistently indented with other hook calls like useState/useEffect in Matchmaking.tsx.frontend/src/App.tsx (1)
83-83: Consider adding an index route for/coach.When a user navigates to
/coachdirectly, onlyCoachPagerenders. If the intent is forCoachPageto serve as both a landing page and a layout wrapper, this is fine. However, if child routes should have a default, consider adding anindexroute:<Route path="coach" element={<CoachPage />}> <Route index element={<StrengthenArgument />} /> {/* or a CoachLanding component */} <Route path="strengthen-argument" element={<StrengthenArgument />} /> <Route path="pros-cons" element={<ProsConsChallenge />} /> </Route>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/App.tsx` at line 83, Add an index child route under the existing Route that uses CoachPage so navigating to /coach renders a default child; specifically, modify the Route with element={<CoachPage />} to include an index route (using Route index) that renders the intended default component (e.g., StrengthenArgument or a CoachLanding component) alongside the existing child routes such as path="strengthen-argument" -> StrengthenArgument and path="pros-cons" -> ProsConsChallenge so /coach loads the index child rather than only the CoachPage wrapper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/App.tsx`:
- Around line 83-89: The nested routes under CoachPage (routes for
StrengthenArgument and ProsConsChallenge) won't render because CoachPage lacks
react-router-dom's Outlet; open CoachPage component and import { Outlet } from
'react-router-dom', then insert <Outlet /> at the spot where child route content
should render (e.g., after the main landing/Featured Learning Paths area) so
navigating to /coach/strengthen-argument and /coach/pros-cons mounts
StrengthenArgument and ProsConsChallenge respectively; alternatively, if you
intended those routes to replace CoachPage, flatten the routes in App.tsx
instead of nesting.
In `@frontend/src/components/ui/select.tsx`:
- Around line 121-122: Register the custom Tailwind variant and/or unify the CSS
class names: add a Tailwind variant named "high-contrast" via addVariant(...) in
frontend/tailwind.config.js so the "high-contrast:..." utilities in select.tsx
produce CSS, or change the variant usage to the existing "contrast" variant (or
change the theme provider to emit "high-contrast") so the class applied by
theme-provider.tsx ("contrast") matches the CSS selector in src/index.css;
update either the variant registration or the class name usage consistently
across frontend/src/components/ui/select.tsx, frontend/src/index.css, and
frontend/src/context/theme-provider.tsx to ensure the highlighted state works.
In `@frontend/src/index.css`:
- Around line 88-91: The CSS class name mismatch prevents the contrast vars from
being applied; update either the CSS or the theme application so the body
receives the same class name used in the stylesheet. Specifically, change the
class applied in theme-provider.tsx (where it currently applies "contrast" to
<body>) to "high-contrast", or rename the CSS selector .high-contrast to
.contrast so both sides match; ensure the hover tokens (--hc-hover-bg and
--hc-hover-text) are defined on the exact class string used when toggling
contrast in the ThemeProvider.
In `@frontend/src/Pages/DebateRoom.tsx`:
- Line 728: The modal's container in DebateRoom.tsx uses an invalid Tailwind
class "max-w-m" causing it to lack a max-width; update the class list on the
modal wrapper div (the element with className starting "bg-white
dark:bg-zinc-900 rounded-xl p-6 ...") to use a valid Tailwind max-width such as
"max-w-md" (or another appropriate "max-w-..." utility) so the popup is capped
at the intended width on larger screens.
- Around line 759-762: The onClose handler currently both hides the judgment
popup and navigates home, which prevents users from simply dismissing it; update
the onClose callback used in DebateRoom (the function referencing
setShowJudgment and navigate) to only dismiss the popup by calling
setShowJudgment(false) and remove the navigate("/") invocation so Close acts as
a pure dismiss action (the existing Back to Home button in JudgementPopup
remains the navigation control).
- Around line 596-609: Parsed JSON may be "{}" and pass JSON5.parse but lack
required fields for JudgmentPopup; after parsing the string from extractJSON and
before calling setJudgmentData or setShowJudgment, validate the resulting
judgment object (type JudgmentData) contains the required keys/sections (e.g.,
whatever JudgmentData mandates: summary, verdict, evidence, etc.), and if
validation fails either throw a descriptive error or trigger the existing
fallback (setPopup with an error message and do not call
setJudgmentData/setShowJudgment); update the logic around extractJSON,
JSON5.parse, setJudgmentData, and setShowJudgment to include this shape check.
- Around line 699-704: The dark-mode UI has container classes but descendant
text/background tokens remain light, causing low contrast; update the relevant
JSX in DebateRoom.tsx (the outer <div className="min-h-screen...">, the inner
<div className="w-full max-w-5xl..."> and the rounded panel <div
className="rounded-xl..."> and the child elements referenced around lines ~728
and ~768) to add matching dark-mode tokens: replace specific light-only classes
like text-gray-900/text-gray-800/text-gray-700/text-gray-500 and bg-gray-50 with
paired classes (e.g., text-gray-900 dark:text-gray-100 or text-gray-700
dark:text-zinc-300, bg-gray-50 dark:bg-zinc-900) or add
dark:text-*/dark:bg-*/dark:via-* variants so headers, modal copy, and bot panel
metadata use high-contrast colors in dark mode while preserving the current
light-mode styles.
In `@frontend/src/Pages/TeamBuilder.tsx`:
- Line 338: Replace the non-standard Tailwind class on the H1 in TeamBuilder
(the element with className "text-5xl font-600 ...") by using the standard
weight class "font-semibold" and swap the hardcoded "text-blue-400" to your
design token color class (e.g., "text-primary" or the project's equivalent) to
keep styles consistent with the design system; update the className string on
that H1 accordingly.
---
Outside diff comments:
In `@backend/services/transcriptservice.go`:
- Around line 972-981: GetDebateStats is populating recentDebates with a
hardcoded "eloChange": 0 which is inconsistent with GetProfile; update the map
population in the loop inside GetDebateStats to read the actual value from
transcript.EloChange (same field used by GetProfile) instead of 0 so both
endpoints return the same Elo change; locate the loop that appends to
recentDebates and replace the constant with transcript.EloChange, preserving
types/formatting used elsewhere.
- Around line 190-210: The computed Elo changes from UpdateRatings are not being
persisted because SaveDebateTranscript is still passed hardcoded 0.0; call
UpdateRatings (the function that sets debateRecord.RatingChange and
opponentRecord.RatingChange) before each SaveDebateTranscript invocation and
pass the actual values (e.g., debateRecord.RatingChange and
opponentRecord.RatingChange or the local variables returned by UpdateRatings)
into SaveDebateTranscript instead of 0.0 so the EloChange/EloPersisted fields
are saved for both the forUser and againstUser transcripts.
---
Nitpick comments:
In `@backend/controllers/profile_controller.go`:
- Line 257: The inline TODO next to "eloChange": transcript.EloChange is
outdated; either remove the TODO or replace it with a short clarifying comment
that the Elo value is now persisted and that upstream callers must compute and
pass the real Elo delta (not 0.0). Update the comment near transcript.EloChange
(and any nearby comment block) to state that EloChange is persisted and that the
responsibility to calculate and supply the actual change lies with the upstream
caller (or remove the TODO entirely).
In `@backend/controllers/transcript_controller.go`:
- Around line 109-119: The handler currently calls SaveDebateTranscript with a
hardcoded eloChange (0.0); update the request and call chain to accept and pass
an Elo value: add an EloChange (float64) field to the SaveTranscriptRequest
struct, validate/parse it in the controller where req is built, and replace the
hardcoded 0.0 with req.EloChange when calling SaveDebateTranscript so the
service receives the client-provided Elo change.
In `@frontend/src/App.tsx`:
- Line 83: Add an index child route under the existing Route that uses CoachPage
so navigating to /coach renders a default child; specifically, modify the Route
with element={<CoachPage />} to include an index route (using Route index) that
renders the intended default component (e.g., StrengthenArgument or a
CoachLanding component) alongside the existing child routes such as
path="strengthen-argument" -> StrengthenArgument and path="pros-cons" ->
ProsConsChallenge so /coach loads the index child rather than only the CoachPage
wrapper.
In `@frontend/src/components/Header.tsx`:
- Around line 330-331: The drawer item conditional currently returns the same
visual classes for active and hovered states ("bg-muted text-foreground" vs
"text-muted-foreground hover:bg-muted hover:text-foreground"), so hovering an
inactive item looks identical to the active item; update the inactive branch in
Header.tsx (the ternary that currently yields "text-muted-foreground
hover:bg-muted hover:text-foreground") to use a distinct hover style (for
example replace the hover utilities with a subtler utility like
"hover:bg-muted/10 hover:text-foreground" or "hover:bg-transparent
hover:text-foreground") so active items keep "bg-muted text-foreground" and
hovered-but-inactive items remain visually distinct.
In `@frontend/src/components/Matchmaking.tsx`:
- Around line 38-42: Align the indentation of the useEffect hook with the other
hooks in the component: reformat the block that calls useEffect (the one that
calls setIsInPool(false) and setWaitTime(0)) so its indentation matches
surrounding hooks and component style, ensuring the opening useEffect line, its
callback body, and the closing bracket/array are all consistently indented with
other hook calls like useState/useEffect in Matchmaking.tsx.
In `@frontend/src/Pages/Admin/AdminSignup.tsx`:
- Around line 29-36: Align the indentation in the admin login block to the
project's standard (use consistent 2- or 4-space indentation) so the lines
around the adminLogin call, the response validation (response,
response.accessToken, response.admin), the localStorage.setItem calls, and the
navigate("/admin/dashboard") call all use the same indentation level; keep the
email normalization (email.trim().toLowerCase()) and preserve the response
validation and storage logic (variables: response, adminLogin,
localStorage.setItem, navigate).
- Line 16: The exported React component name AdminLogin does not match the file
name AdminSignup.tsx; either rename the file to AdminLogin.tsx or rename the
exported function to AdminSignup to make them consistent. Update any imports
referencing this module (search for AdminSignup / AdminLogin) so they match the
new name, and ensure the default export signature (function AdminLogin /
function AdminSignup) is the same as the filename to avoid confusion and import
errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc9b6034-33e9-4079-acfc-afd9d4473818
📒 Files selected for processing (15)
backend/config/config.prod.sample.ymlbackend/controllers/debatevsbot_controller.gobackend/controllers/profile_controller.gobackend/controllers/transcript_controller.gobackend/models/transcript.gobackend/services/transcriptservice.gofrontend/src/App.tsxfrontend/src/Pages/Admin/AdminSignup.tsxfrontend/src/Pages/DebateRoom.tsxfrontend/src/Pages/TeamBuilder.tsxfrontend/src/components/Header.tsxfrontend/src/components/JudgementPopup.tsxfrontend/src/components/Matchmaking.tsxfrontend/src/components/ui/select.tsxfrontend/src/index.css
💤 Files with no reviewable changes (1)
- backend/config/config.prod.sample.yml
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.
Overview
This PR fixes an issue where the admin authentication token was stored in localStorage before validating the server response.
Problem
Previously, the application stored
adminTokenandadmindata immediately after the API call. If the server returned an invalid or incomplete response (for example, missingaccessTokenoradmindata), the application could still store invalid values in localStorage and redirect the user to the admin dashboard.This could lead to broken authentication states or unintended access behavior.
Solution
Added validation to ensure that the login response contains both
accessTokenandadmindata before storing them in localStorage and navigating to the admin dashboard.If the response is invalid, an error is thrown and the login flow is stopped.
Changes
Added response validation before storing authentication data.
Improved error handling in the login process.
Prevented invalid tokens from being stored in localStorage.
Testing
Tested the following scenarios:
Valid credentials
Admin successfully logs in
Token and admin data stored in localStorage
Redirects to
/admin/dashboardInvalid credentials
Error message displayed
No token stored in localStorage
User remains on login page
Impact
Improves authentication reliability and prevents invalid login sessions caused by incomplete API responses.
Summary by CodeRabbit
New Features
Bug Fixes
Style
Chores