Fix AuthContext Safety Handling in StartDebate Component#336
Open
Abhishek2005-ard wants to merge 22 commits intoAOSSIE-Org:mainfrom
Open
Fix AuthContext Safety Handling in StartDebate Component#336Abhishek2005-ard wants to merge 22 commits intoAOSSIE-Org:mainfrom
Abhishek2005-ard wants to merge 22 commits intoAOSSIE-Org:mainfrom
Conversation
… initialized via useState)
…anted restart loop
📝 WalkthroughWalkthroughThis PR extends the debate transcript saving pipeline to track Elo changes in the backend, reorganizes frontend routing with admin protection, enhances styling with dark mode and accessibility tokens, and refactors speech recognition with auto-restart capability and improved error handling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/components/JudgementPopup.tsx (1)
94-107:⚠️ Potential issue | 🟠 MajorHardcoded
localhostURLs will break in deployed environments.The
coachSkillsURLs are hardcoded tohttp://localhost:5173/.... These should use relative paths or be driven by an environment variable.🐛 Proposed fix
const coachSkills: CoachSkill[] = [ { title: 'Strengthen Argument', description: 'Master the art of crafting compelling, persuasive arguments that win debates.', - url: 'http://localhost:5173/coach/strengthen-argument', + url: '/coach/strengthen-argument', }, { title: 'Pros and Cons Challenge', description: 'Test your critical thinking by crafting up to 5 pros and cons for engaging debate topics.', - url: 'http://localhost:5173/coach/pros-cons', + url: '/coach/pros-cons', }, ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/JudgementPopup.tsx` around lines 94 - 107, The coachSkills array currently hardcodes full localhost URLs which will fail in production; update the coachSkills definition (the coachSkills constant in JudgementPopup.tsx) to use relative paths (e.g. "/coach/strengthen-argument") or build the URLs from a runtime environment variable (e.g. VITE_BASE_URL / process.env.BASE_URL) so links resolve correctly in deployed environments; replace the url fields accordingly and ensure any link rendering code still treats these values as safe hrefs.backend/controllers/debatevsbot_controller.go (1)
270-280:⚠️ Potential issue | 🟠 MajorHandle the
SaveDebateTranscripterror instead of discarding it.This call is currently unchecked, so a transcript persistence failure is silent and the API still returns success.
🛠️ Proposed fix
-services.SaveDebateTranscript( +if err := services.SaveDebateTranscript( userID, email, "user_vs_bot", latestDebate.Topic, latestDebate.BotName, resultStatus, req.History, nil, 0.0, -) +); err != nil { + log.Printf("failed to save bot debate transcript for user %s: %v", userID.Hex(), err) + c.JSON(500, gin.H{"error": "Failed to save debate transcript"}) + return +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/debatevsbot_controller.go` around lines 270 - 280, The call to services.SaveDebateTranscript is ignoring its returned error so transcript persistence failures are silent; update the code to capture the returned error (err := services.SaveDebateTranscript(...)) and handle it: if err != nil then log the error (include context: userID, latestDebate.Topic, latestDebate.BotName) and return an error HTTP response instead of continuing to return success (e.g., respond with a 500 and a helpful message). Keep the same arguments (userID, email, "user_vs_bot", latestDebate.Topic, latestDebate.BotName, resultStatus, req.History, nil, 0.0) but ensure error handling occurs right after the call so persistence failures are surfaced to the caller.
🧹 Nitpick comments (3)
frontend/src/Pages/DebateRoom.tsx (3)
598-603: JSON5 parse with rethrow is reasonable but consider logging the raw string on failure.When the parse fails, the error message includes
ebut not thejsonStringthat failed to parse. This can make debugging difficult since the raw LLM output is the critical context.♻️ Suggested improvement
try { judgment = JSON5.parse(jsonString); }catch (e) { - throw new Error(`Failed to parse judgment JSON: ${e}`); + console.error("Failed to parse judgment JSON string:", jsonString); + throw new Error(`Failed to parse judgment JSON: ${e}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Pages/DebateRoom.tsx` around lines 598 - 603, The catch block currently rethrows a new Error with only the parse exception (e) and omits the raw LLM output; update the catch in the code around JSON5.parse (where judgment is parsed from jsonString) to log or include jsonString when the parse fails: capture the thrown error in the catch, record the offending jsonString (e.g., via console.error or the existing logger) along with a descriptive message, then rethrow the Error including both the original error message and a truncated or full jsonString for easier debugging while avoiding leaking sensitive data.
579-579: Indentation inconsistency injudgeDebateResult.The function declaration at Line 579 is indented with 4 spaces, while the rest of the file's top-level declarations within the component use 2 spaces. This makes the function visually inconsistent with siblings like
formatTime(Line 658),renderPhaseMessages(Line 673), etc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Pages/DebateRoom.tsx` at line 579, The function declaration for judgeDebateResult is indented with 4 spaces while other top-level declarations inside the component (e.g., formatTime and renderPhaseMessages) use 2-space indentation; fix by adjusting the indentation of the judgeDebateResult declaration and its entire body to match the 2-space style used for the component's sibling functions (search for the function name judgeDebateResult to locate it).
610-650: Default judgment data on error is good UX but consider clearer user messaging.Showing zeroed-out scores after an error could confuse users into thinking they performed poorly. The error message in the popup (Lines 615-617) is displayed for only 3 seconds, which may not be enough time for users to read and understand it before the zero-score judgment appears.
Consider either extending the timeout or adding a visible error banner within the
JudgmentPopupitself so users understand the scores are not real.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Pages/DebateRoom.tsx` around lines 610 - 650, The popup and default judgment flow currently auto-hides after 3s and immediately shows zeroed scores, which can confuse users; update the error handling around setPopup, setJudgmentData, setTimeout and setShowJudgment (and the JudgmentPopup component) so that either (A) the timeout is extended and the popup includes an explicit error flag/message (e.g., popup.isError) that the JudgmentPopup renders as a persistent banner until user dismisses, or (B) replace the fixed timeout with a dismiss callback that only calls setShowJudgment(true) after the user acknowledges the error; also mark the default judgment state (e.g., judgmentData.isDefaultDueToError) so the UI can clearly label scores as placeholders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/services/transcriptservice.go`:
- Line 668: The SaveDebateTranscript signature was changed to include an
eloChange float64 parameter, but callers like the one in
debatevsbot_controller.go (around the handler invoking SaveDebateTranscript)
were not updated; update all call sites to pass a float64 eloChange (e.g.,
computed elo delta or 0.0 if not applicable) when calling
SaveDebateTranscript(userID, email, debateType, topic, opponent, result,
messages, transcripts, eloChange), ensuring the value you pass matches the
intended logic and type.
- Around line 182-210: The current code logs errors from SaveDebateTranscript
but continues and later deletes the original transcripts
(forSubmission.Transcripts and againstSubmission.Transcripts), risking data
loss; modify the logic around SaveDebateTranscript calls so that if
SaveDebateTranscript returns an error (for either the forUser or againstUser
call) you abort further processing and do not delete the source
transcripts—return or propagate the error instead of only logging (update the
error handling around the SaveDebateTranscript calls and the subsequent deletion
code that references forSubmission.Transcripts and againstSubmission.Transcripts
to be conditional on successful saves).
- Around line 190-207: The two calls to SaveDebateTranscript are passing a
hardcoded 0.0 for eloChange, so SavedDebateTranscript.EloChange is always zero;
replace those literals with the actual computed elo delta values (e.g.,
eloDeltaForUser / eloDeltaAgainstUser or whatever variables compute rating
movement in this flow) so the first SaveDebateTranscript call for forUser uses
the for-user delta and the second call for againstUser uses the against-user
delta; update references around SaveDebateTranscript, SaveDebateTranscript(...)
and any place that computes elo changes to pass the correct variables instead of
0.0.
In `@frontend/src/components/ProtectedRoute.tsx`:
- Around line 8-12: The ProtectedRoute check uses
localStorage.getItem("adminData") but the admin signup/login flow stores the
payload under "admin" (mismatch between adminData and admin); update the
ProtectedRoute component to read the same key used by the login/signup flow
(replace uses of adminData with localStorage.getItem("admin")) so the condition:
const adminToken = localStorage.getItem("adminToken"); const admin =
localStorage.getItem("admin"); if (!adminToken || !admin) return <Navigate
to="/admin/login" replace />; (ensure the symbol names adminToken and admin in
ProtectedRoute match the storage key used in AdminSignup).
In `@frontend/src/index.css`:
- Around line 90-93: The .high-contrast CSS rule defines variables
(--hc-hover-bg, --hc-hover-text) that are never used at runtime; either remove
the .high-contrast block from index.css or ensure the class is applied when
high-contrast mode is enabled (e.g., add logic in your high-contrast
toggle/feature to add/remove the "high-contrast" class on the root or target
elements so the --hc-hover-bg and --hc-hover-text variables become effective).
Locate the .high-contrast rule in index.css and choose the fix: delete the rule
or wire up the runtime code that toggles the "high-contrast" class where
accessibility/theme toggles are implemented.
In `@frontend/src/Pages/DebateRoom.tsx`:
- Around line 697-703: The user section in the DebateRoom component lacks
dark-mode Tailwind variants (it uses "bg-white border border-gray-200" and
header/message elements use "bg-gray-50") while the bot section includes
"dark:bg-zinc-900" and "dark:border-zinc-700"; update the user section and
shared inner elements to mirror the bot's dark variants by adding corresponding
dark: classes (e.g., replace/add dark:bg-zinc-900 or dark:bg-zinc-800 for
container/header backgrounds and dark:border-zinc-700 for borders) and add
dark:bg-... for message bubbles so the styles between the bot and user blocks
(and headers) match in dark mode within the DebateRoom component.
- Around line 758-761: In JudgementPopup.tsx locate the navigate call that uses
the lowercase path '/startdebate' (the onClose handler or any direct
navigate('/startdebate') invocation) and change it to the camelCase
'/startDebate' to match the route defined for StartDebate in App.tsx; update any
related occurrences in the JudgementPopup component (e.g., the onClose handler
that calls setShowJudgment and navigate) so the path casing is consistent with
other components like DebateRoom, Header, and Sidebar.
In `@frontend/src/utils/speechTest.ts`:
- Around line 60-73: The auto-restart race happens because restart() calls
this.recognition.start() without setting this.isListening, so a concurrent
stop() can see isListening false and skip calling recognition.stop(); to fix, in
the restart logic (where this.restartTimeout is set and where
recognition.start() is invoked) set this.isListening = true immediately before
calling this.recognition.start() (or immediately when scheduling the restart)
and ensure restartTimeout is cleared on stop(); also review the other restart
path (the similar block around lines 132–145) and apply the same change so
start/stop always have a consistent isListening state and stop() reliably calls
recognition.stop().
- Line 155: The lint error comes from using an expression-bodied arrow callback
in the call to stream.getTracks().forEach; change the callback passed to forEach
in speechTest.ts so it uses a block-bodied function that invokes track.stop()
(e.g., replace the expression-bodied arrow with a block-bodied arrow or
function) to satisfy lint/suspicious/useIterableCallbackReturn while maintaining
the same behavior.
---
Outside diff comments:
In `@backend/controllers/debatevsbot_controller.go`:
- Around line 270-280: The call to services.SaveDebateTranscript is ignoring its
returned error so transcript persistence failures are silent; update the code to
capture the returned error (err := services.SaveDebateTranscript(...)) and
handle it: if err != nil then log the error (include context: userID,
latestDebate.Topic, latestDebate.BotName) and return an error HTTP response
instead of continuing to return success (e.g., respond with a 500 and a helpful
message). Keep the same arguments (userID, email, "user_vs_bot",
latestDebate.Topic, latestDebate.BotName, resultStatus, req.History, nil, 0.0)
but ensure error handling occurs right after the call so persistence failures
are surfaced to the caller.
In `@frontend/src/components/JudgementPopup.tsx`:
- Around line 94-107: The coachSkills array currently hardcodes full localhost
URLs which will fail in production; update the coachSkills definition (the
coachSkills constant in JudgementPopup.tsx) to use relative paths (e.g.
"/coach/strengthen-argument") or build the URLs from a runtime environment
variable (e.g. VITE_BASE_URL / process.env.BASE_URL) so links resolve correctly
in deployed environments; replace the url fields accordingly and ensure any link
rendering code still treats these values as safe hrefs.
---
Nitpick comments:
In `@frontend/src/Pages/DebateRoom.tsx`:
- Around line 598-603: The catch block currently rethrows a new Error with only
the parse exception (e) and omits the raw LLM output; update the catch in the
code around JSON5.parse (where judgment is parsed from jsonString) to log or
include jsonString when the parse fails: capture the thrown error in the catch,
record the offending jsonString (e.g., via console.error or the existing logger)
along with a descriptive message, then rethrow the Error including both the
original error message and a truncated or full jsonString for easier debugging
while avoiding leaking sensitive data.
- Line 579: The function declaration for judgeDebateResult is indented with 4
spaces while other top-level declarations inside the component (e.g., formatTime
and renderPhaseMessages) use 2-space indentation; fix by adjusting the
indentation of the judgeDebateResult declaration and its entire body to match
the 2-space style used for the component's sibling functions (search for the
function name judgeDebateResult to locate it).
- Around line 610-650: The popup and default judgment flow currently auto-hides
after 3s and immediately shows zeroed scores, which can confuse users; update
the error handling around setPopup, setJudgmentData, setTimeout and
setShowJudgment (and the JudgmentPopup component) so that either (A) the timeout
is extended and the popup includes an explicit error flag/message (e.g.,
popup.isError) that the JudgmentPopup renders as a persistent banner until user
dismisses, or (B) replace the fixed timeout with a dismiss callback that only
calls setShowJudgment(true) after the user acknowledges the error; also mark the
default judgment state (e.g., judgmentData.isDefaultDueToError) so the UI can
clearly label scores as placeholders.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
backend/controllers/debatevsbot_controller.gobackend/controllers/profile_controller.gobackend/models/transcript.gobackend/services/transcriptservice.gofrontend/package.jsonfrontend/src/App.tsxfrontend/src/Pages/About.tsxfrontend/src/Pages/DebateRoom.tsxfrontend/src/Pages/TeamDebateRoom.tsxfrontend/src/components/Header.tsxfrontend/src/components/JudgementPopup.tsxfrontend/src/components/Matchmaking.tsxfrontend/src/components/ProtectedRoute.tsxfrontend/src/components/ui/select.tsxfrontend/src/index.cssfrontend/src/utils/speechTest.tsfrontend/tailwind.config.js
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.
This PR improves the way AuthContext is handled in the StartDebate component.
Previously, the component accessed context using:
const authContext = useContext(AuthContext);
and checked authentication using optional chaining:
if (authContext?.isAuthenticated)
Although this prevents runtime errors, it can silently fail if the AuthContext provider is not properly wrapped around the component. In such cases, authContext becomes undefined, and the authentication check always evaluates to false, leading to unintended redirection to the login page.
This update refactors the context usage to safely destructure with a fallback:
const { isAuthenticated } = useContext(AuthContext) || {};
This improves readability, removes repeated optional chaining, and prevents hidden authentication logic issues, making the component more robust and maintainable.
close #335
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Chores