Fix/debate room theme sync#328
Conversation
📝 WalkthroughWalkthroughIntroduces email autocomplete suggestions and client-side validation in authentication forms. Refactors DebateRoom component with a theme system, explicit type definitions, typed speech recognition, responsive card-based UI layout, and consolidated phase progression logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SpeechRec as Speech Recognition
participant DebateRoom
participant BotService
participant ThemeSystem
participant UI
User->>DebateRoom: Start debate / advance turn
DebateRoom->>ThemeSystem: Fetch active palette
ThemeSystem-->>UI: Apply theme colors
alt User's Turn
User->>SpeechRec: Start recording
SpeechRec-->>DebateRoom: Transcript received
DebateRoom->>DebateRoom: Validate & store message
DebateRoom->>UI: Render message with theme
end
DebateRoom->>DebateRoom: Check phase completion
alt Phase Complete
DebateRoom->>BotService: Request bot response
BotService-->>DebateRoom: Bot message + judgment data
DebateRoom->>DebateRoom: advanceTurn() - progress phase
DebateRoom->>UI: Update with bot message & next phase
end
alt Debate End
DebateRoom->>UI: Show end-of-phase popup
UI-->>User: Display final judgment
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/Pages/Authentication/forms.tsx (1)
445-465:⚠️ Potential issue | 🟡 Minor
ForgotPasswordFormmakes an API call without client-side email validation.
LoginFormandSignUpFormboth validate email format before submitting, butForgotPasswordForm.handleSubmitsends the request even for an empty or malformed email string. This is an inconsistency introduced in the same PR.🛡️ Proposed fix — add email validation before the fetch
const handleSubmit = async (e: React.FormEvent) => { e.preventDefault(); setError(''); + + if (!email.trim()) { + setError('Please enter your email address'); + return; + } + if (!validateEmail(email)) { + setError('Please enter a valid email address'); + return; + } try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Pages/Authentication/forms.tsx` around lines 445 - 465, ForgotPasswordForm.handleSubmit currently sends the POST to ${baseURL}/forgotPassword without validating the email; add the same client-side validation used in LoginForm/SignUpForm (check non-empty and RFC-like format) before calling fetch, and if validation fails call setError with a descriptive message and return early; ensure successful path still calls startResetPassword(email) and keep the try/catch around the network call so only valid emails trigger the fetch.frontend/src/Pages/DebateRoom.tsx (1)
128-132:⚠️ Potential issue | 🟠 MajorMissing null guard for
location.state— direct navigation will crash.
location.stateisnullwhen users navigate directly to this URL (bookmark, refresh, deep link). The unchecked cast on line 131 followed by immediate property access on line 132 will throw aTypeError.🛡️ Proposed fix: guard and redirect
const DebateRoom: React.FC = () => { const location = useLocation(); const navigate = useNavigate(); - const debateData = location.state as DebateProps; + const debateData = location.state as DebateProps | null; + + useEffect(() => { + if (!debateData) navigate("/game", { replace: true }); + }, [debateData, navigate]); + + if (!debateData) return null; + const phases = debateData.phaseTimings;🤖 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 128 - 132, DebateRoom currently casts location.state to DebateProps and immediately accesses phaseTimings (debateData and phases) which will crash when location.state is null; update the component (DebateRoom) to check if location.state is present (e.g., if (!location.state) or const debateData = location.state as DebateProps | null and if (!debateData)), and if missing use navigate(...) to redirect to a safe route (or render a fallback) before accessing debateData.phaseTimings so phases is only derived when debateData exists.
🧹 Nitpick comments (8)
frontend/src/Pages/Authentication/forms.tsx (3)
130-135:validateEmailandMIN_PASSWORD_LENGTHare duplicated verbatim in bothLoginFormandSignUpForm.Both are defined inside the component body (triggering recreation on every render) and are identical copies. Extract them once at module level.
♻️ Proposed refactor — module-level constants
Add above all component declarations:
+const MIN_PASSWORD_LENGTH = 8; + +const validateEmail = (email: string): boolean => { + const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; + return emailRegex.test(email); +}; +Then remove the in-component copies at lines 130–135 (LoginForm) and lines 266–271 (SignUpForm).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Pages/Authentication/forms.tsx` around lines 130 - 135, MIN_PASSWORD_LENGTH and validateEmail are duplicated inside both LoginForm and SignUpForm and recreated on each render; extract them to module-level constants instead. Define MIN_PASSWORD_LENGTH and validateEmail once at the top of the file (above the component declarations) and remove the in-component declarations from LoginForm and SignUpForm so both components import/use the single module-level symbols.
30-35:emailDomainsandgetEmailSuggestionsare re-created on every render.Both are defined inside the component body. As static data and a pure derivation, they should live at module scope (or
getEmailSuggestionsshould beuseCallback-memoized if kept inside).♻️ Proposed refactor — hoist to module level
+const EMAIL_DOMAINS = ['@gmail.com', '@yahoo.com', '@outlook.com', '@hotmail.com']; + +const getEmailSuggestions = (email: string): string[] => { + if (!email || email.includes('@')) return []; + return EMAIL_DOMAINS.map(domain => email + domain); +}; + const EmailInputWithSuggestions: React.FC<EmailInputWithSuggestionsProps> = ({ ... }) => { - const emailDomains = ['@gmail.com', '@yahoo.com', '@outlook.com', '@hotmail.com']; - - const getEmailSuggestions = (email: string) => { - if (!email || email.includes('@')) return []; - return emailDomains.map(domain => email + domain); - }; - const suggestions = getEmailSuggestions(value);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Pages/Authentication/forms.tsx` around lines 30 - 35, The emailDomains array and getEmailSuggestions function are recreated on every render because they live inside the component; hoist emailDomains (and preferably getEmailSuggestions) to module scope so they are defined once, or if you must keep getEmailSuggestions inside the component, wrap it with useCallback to memoize it; update references to use the moved getEmailSuggestions/emailDomains and ensure types remain correct for getEmailSuggestions.
86-94:type="text"on the email input sacrifices mobile keyboard and browser autofill.Using
type="email"would give mobile users the email-optimized keyboard layout (with@and.comshortcuts) and let browsers activate their email-address autofill heuristics. The custom suggestion dropdown remains fully functional alongsidetype="email".♻️ Proposed change
- type="text" + type="email"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Pages/Authentication/forms.tsx` around lines 86 - 94, The email input currently uses type="text", which prevents mobile email keyboards and browser autofill; update the <Input ... /> component to use type="email" instead of "text" while leaving props and handlers intact (value, onChange/handleInputChange, onKeyDown/handleKeyDown, onFocus that toggles setShowSuggestions, placeholder, className) so the suggestion dropdown and behavior remain unchanged but mobile keyboard and autofill are enabled.frontend/src/Pages/DebateRoom.tsx (5)
200-201: Theeslint-disablefor exhaustive-deps is tolerable here but worth a note.The suppressed dependency is
judgeDebateResult, which isn't wrapped inuseCallback. Its closure is effectively stable (only usesdebateData.userIdfromlocation.stateand stable setters), so the stale closure is safe in practice. If this component grows further, consider wrappingjudgeDebateResultinuseCallbackto remove the suppression.🤖 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 200 - 201, The eslint-disable bypass hides a missing dependency: wrap the judgeDebateResult function in useCallback (referencing any external values it uses, e.g., debateData.userId and stable setters) so its identity is stable, then remove the "// eslint-disable-next-line react-hooks/exhaustive-deps" comment and include judgeDebateResult in the useEffect dependency array (the effect currently depends on phases). Ensure the useCallback dependencies are minimal and correct to avoid stale closures.
494-496: AbusingsetStateupdater to read current state — confusing and fragile.
setState(pr => { advanceTurn(pr); return pr; })uses the updater solely to access current state, returns the same reference (so thissetStateitself is a no-op), and relies onadvanceTurninternally callingsetStatefor actual updates. This also triggers side effects inside an updater (same StrictMode concern as above).A cleaner approach: store state in a ref that's always current, or use
useReducerto co-locate side-effect triggers with state transitions.🤖 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 494 - 496, The onClick currently abuses setState by calling setState(pr => { advanceTurn(pr); return pr; }) which merely returns the same state reference and performs side effects inside the updater; replace this pattern by making the current state available via a stable ref or switching to useReducer so you can call advanceTurn with the real current state (or dispatch an action) instead of using the updater for side effects — update the button handler to either call advanceTurn(currentStateRef.current) or dispatch an action to the reducer; stop returning the unchanged state from setState and ensure advanceTurn itself performs pure state transitions or is invoked outside of state updaters.
398-408: Fixed card height of 540px may clip on smaller viewports.
height: "540px"(line 403) combined withoverflowY: "auto"on the content area works for desktop, but on mobile or smaller screens this may consume most of the viewport. Consider usingmax-heightwith a viewport-relative unit (e.g.,max-height: "70vh") or amin-height/max-heightrange for better responsiveness.🤖 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 398 - 408, The fixed 540px height in cardStyle causes clipping on smaller viewports; update the cardStyle function (used to render the debate cards) to remove height: "540px" and instead use responsive constraints such as maxHeight: "70vh" with an optional minHeight like "320px" (or a range using minHeight/maxHeight) so the card respects viewport height on mobile while keeping desktop sizing, and ensure the inner scrollable content (overflowY) still works with these max/min height rules.
139-153: 300ms polling interval for theme detection is aggressive for a rarely-changed setting.The
setIntervalat 300ms continuously runs for the lifetime of the component to detect same-tab theme changes. Since theme changes are infrequent user actions, consider either:
- Increasing the interval to ~1000ms, or
- Dispatching a
CustomEventfrom the theme setter and listening for it here, which eliminates polling entirely.
291-319:handleBotTurncloses over stalestatevalues for the API call, then uses a setState updater for the update — mixed approach risks inconsistency.Lines 293–301 read
state.currentPhase,state.phaseStep,state.messages, andstate.botStancedirectly from the closure (stale by the time theawaitresolves), while line 304 uses an updater (pr =>) to get fresh state for the message append. If a timer or other effect mutates state between theawaitcall and its resolution, the API request is sent with stale context but the response is appended to current state. This is unlikely to cause visible bugs in practice since bot turns block user interaction, but be aware of the inconsistency.🤖 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 291 - 319, handleBotTurn currently reads state fields from the closure before awaiting sendDebateMessage (state.currentPhase, state.phaseStep, state.messages, state.botStance) which can be stale; fix by reading a fresh snapshot of state immediately before the API call (e.g. add a stateRef that you update on each state change via useEffect and then inside handleBotTurn do const snap = stateRef.current and use snap.currentPhase/snap.phaseStep/snap.messages/snap.botStance when calling sendDebateMessage), keep the existing setState(pr => ...) updater to append the bot message so updates remain based on the latest state; reference handleBotTurn, sendDebateMessage, and setState(pr => ...) when making the change.
🤖 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/Pages/DebateRoom.tsx`:
- Around line 269-284: The setState updater in DebateRoom (the anonymous updater
that reads pr.timer) contains side effects (stopRec, setPopup, advanceTurn,
setNextTurnPending and setTimeout) which must be moved out of the updater;
change the updater to only compute and return the new state (e.g., reduce timer
or set timer to 0 and mark a flag like nextTurnRequested) and then, immediately
after calling setState, trigger the side effects based on the new state or use a
useEffect that watches the relevant state fields (timer,
nextTurnRequested/nextTurnPending) to call stopRec(), setPopup(), advanceTurn(),
setNextTurnPending(), clearInterval(timerRef.current) and schedule
setTimeouts—ensure the updater never invokes external functions and that
useEffect guards avoid double-running in StrictMode.
- Around line 354-372: The judgeDebateResult handler currently does
JSON.parse(extractJSON(result)) as JudgmentData which can silently produce
incomplete objects; add runtime structural validation before calling
setJudgmentData: parse the string from extractJSON(result), verify required
fields (e.g., verdict.winner, verdict.reason and other expected sections like
opening_statement, total) exist and have the correct types, and only then call
setJudgmentData with the parsed object; if validation fails, populate a safe
fallback JudgmentData (similar to the existing error branch) and log the
validation error; implement the validation as a small helper (e.g.,
validateJudgmentData(parsed): parsed is JudgmentData | null) and use it inside
judgeDebateResult to decide whether to set the parsed data or the fallback.
- Around line 117-118: phaseSequences and turnTypes are hardcoded to 3 phases
but debateData.phaseTimings from the backend may have a different length, which
can cause TypeError when indexing phaseSequences[currentPhase] or
turnTypes[state.currentPhase]; validate or derive instead. Fix by checking
debateData.phaseTimings.length against the hardcoded arrays early (e.g., in the
component init where debateData is consumed) and either (a) throw/log an
explicit error and fall back to a safe default if lengths differ, or (b)
refactor to compute phaseSequences and turnTypes dynamically from
debateData.phaseTimings (use the phase configuration to build arrays rather than
hardcoding) so access to phaseSequences[currentPhase] and
turnTypes[state.currentPhase] is always safe; reference the symbols
phaseSequences, turnTypes, debateData.phaseTimings,
currentPhase/state.currentPhase and the backend response producer in vsbot.ts
when implementing the validation or refactor.
---
Outside diff comments:
In `@frontend/src/Pages/Authentication/forms.tsx`:
- Around line 445-465: ForgotPasswordForm.handleSubmit currently sends the POST
to ${baseURL}/forgotPassword without validating the email; add the same
client-side validation used in LoginForm/SignUpForm (check non-empty and
RFC-like format) before calling fetch, and if validation fails call setError
with a descriptive message and return early; ensure successful path still calls
startResetPassword(email) and keep the try/catch around the network call so only
valid emails trigger the fetch.
In `@frontend/src/Pages/DebateRoom.tsx`:
- Around line 128-132: DebateRoom currently casts location.state to DebateProps
and immediately accesses phaseTimings (debateData and phases) which will crash
when location.state is null; update the component (DebateRoom) to check if
location.state is present (e.g., if (!location.state) or const debateData =
location.state as DebateProps | null and if (!debateData)), and if missing use
navigate(...) to redirect to a safe route (or render a fallback) before
accessing debateData.phaseTimings so phases is only derived when debateData
exists.
---
Nitpick comments:
In `@frontend/src/Pages/Authentication/forms.tsx`:
- Around line 130-135: MIN_PASSWORD_LENGTH and validateEmail are duplicated
inside both LoginForm and SignUpForm and recreated on each render; extract them
to module-level constants instead. Define MIN_PASSWORD_LENGTH and validateEmail
once at the top of the file (above the component declarations) and remove the
in-component declarations from LoginForm and SignUpForm so both components
import/use the single module-level symbols.
- Around line 30-35: The emailDomains array and getEmailSuggestions function are
recreated on every render because they live inside the component; hoist
emailDomains (and preferably getEmailSuggestions) to module scope so they are
defined once, or if you must keep getEmailSuggestions inside the component, wrap
it with useCallback to memoize it; update references to use the moved
getEmailSuggestions/emailDomains and ensure types remain correct for
getEmailSuggestions.
- Around line 86-94: The email input currently uses type="text", which prevents
mobile email keyboards and browser autofill; update the <Input ... /> component
to use type="email" instead of "text" while leaving props and handlers intact
(value, onChange/handleInputChange, onKeyDown/handleKeyDown, onFocus that
toggles setShowSuggestions, placeholder, className) so the suggestion dropdown
and behavior remain unchanged but mobile keyboard and autofill are enabled.
In `@frontend/src/Pages/DebateRoom.tsx`:
- Around line 200-201: The eslint-disable bypass hides a missing dependency:
wrap the judgeDebateResult function in useCallback (referencing any external
values it uses, e.g., debateData.userId and stable setters) so its identity is
stable, then remove the "// eslint-disable-next-line
react-hooks/exhaustive-deps" comment and include judgeDebateResult in the
useEffect dependency array (the effect currently depends on phases). Ensure the
useCallback dependencies are minimal and correct to avoid stale closures.
- Around line 494-496: The onClick currently abuses setState by calling
setState(pr => { advanceTurn(pr); return pr; }) which merely returns the same
state reference and performs side effects inside the updater; replace this
pattern by making the current state available via a stable ref or switching to
useReducer so you can call advanceTurn with the real current state (or dispatch
an action) instead of using the updater for side effects — update the button
handler to either call advanceTurn(currentStateRef.current) or dispatch an
action to the reducer; stop returning the unchanged state from setState and
ensure advanceTurn itself performs pure state transitions or is invoked outside
of state updaters.
- Around line 398-408: The fixed 540px height in cardStyle causes clipping on
smaller viewports; update the cardStyle function (used to render the debate
cards) to remove height: "540px" and instead use responsive constraints such as
maxHeight: "70vh" with an optional minHeight like "320px" (or a range using
minHeight/maxHeight) so the card respects viewport height on mobile while
keeping desktop sizing, and ensure the inner scrollable content (overflowY)
still works with these max/min height rules.
- Around line 291-319: handleBotTurn currently reads state fields from the
closure before awaiting sendDebateMessage (state.currentPhase, state.phaseStep,
state.messages, state.botStance) which can be stale; fix by reading a fresh
snapshot of state immediately before the API call (e.g. add a stateRef that you
update on each state change via useEffect and then inside handleBotTurn do const
snap = stateRef.current and use
snap.currentPhase/snap.phaseStep/snap.messages/snap.botStance when calling
sendDebateMessage), keep the existing setState(pr => ...) updater to append the
bot message so updates remain based on the latest state; reference
handleBotTurn, sendDebateMessage, and setState(pr => ...) when making the
change.
| const phaseSequences = [["For","Against"],["For","Against","Against","For"],["For","Against"]]; | ||
| const turnTypes = [["statement","statement"],["question","answer","question","answer"],["statement","statement"]]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "phaseTimings" --type=ts --type=tsx -C3Repository: AOSSIE-Org/DebateAI
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Examine the DebateRoom.tsx file around the flagged lines
find . -name "DebateRoom.tsx" -type f | head -1 | xargs wc -lRepository: AOSSIE-Org/DebateAI
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Get context around lines 117-118 and check the implementation
find . -name "DebateRoom.tsx" -type f | head -1 | xargs sed -n '100,200p'Repository: AOSSIE-Org/DebateAI
Length of output: 4737
🏁 Script executed:
#!/bin/bash
# Search for how currentPhase is assigned/used
find . -name "DebateRoom.tsx" -type f | head -1 | xargs rg "currentPhase" -nRepository: AOSSIE-Org/DebateAI
Length of output: 2233
🏁 Script executed:
rg -n "phaseTimings" --type=ts -A 2 -B 2Repository: AOSSIE-Org/DebateAI
Length of output: 5041
🏁 Script executed:
rg -n "DebateRoom" --type=ts --type=jsx -B 3 -A 3Repository: AOSSIE-Org/DebateAI
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Look for navigate calls to DebateRoom or state assignments with phaseTimings
rg -n "navigate.*DebateRoom|phaseTimings.*:" --type=ts --type=jsx -B 2 -A 2Repository: AOSSIE-Org/DebateAI
Length of output: 90
🏁 Script executed:
rg -n "defaultPhaseTimings" --type=ts -B 5 -A 5Repository: AOSSIE-Org/DebateAI
Length of output: 1916
🏁 Script executed:
#!/bin/bash
# Check what defaultPhaseTimings is set to
find . -name "*.ts" -type f | xargs grep -l "defaultPhaseTimings" | head -5Repository: AOSSIE-Org/DebateAI
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Look for where BotSelection navigates to DebateRoom
find . -name "BotSelection.tsx" -type f | head -1 | xargs sed -n '280,310p'Repository: AOSSIE-Org/DebateAI
Length of output: 834
🏁 Script executed:
#!/bin/bash
# Check if phaseTimings can be created/modified with variable length elsewhere
find . -name "*.ts" -type f | xargs grep -l "phaseTimings" | xargs grep -n "\.push\|\.slice\|\.concat\|\.filter" | grep -i phaseRepository: AOSSIE-Org/DebateAI
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Verify the bound check logic at line 186
find . -name "DebateRoom.tsx" -type f | head -1 | xargs sed -n '180,195p'Repository: AOSSIE-Org/DebateAI
Length of output: 993
🏁 Script executed:
#!/bin/bash
# Check if the API response can return variable-length phaseTimings
find . -name "vsbot.ts" -type f | head -1 | xargs sed -n '45,85p'Repository: AOSSIE-Org/DebateAI
Length of output: 1361
Hardcoded 3-phase arrays lack validation against backend response.
phaseSequences and turnTypes are hardcoded for exactly 3 phases, but debateData.phaseTimings comes from a backend response that is never validated. While the frontend UI cannot modify the array length and initialization defaults to 3 phases, the API response (line 45-80 in vsbot.ts) could return a different phase count without triggering an error. This would cause TypeError when accessing phaseSequences[currentPhase] or turnTypes[state.currentPhase] if currentPhase >= 3.
Add validation that backend phaseTimings length matches the hardcoded sequences, or refactor to derive phaseSequences and turnTypes from the phase configuration instead of hardcoding.
🤖 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 117 - 118, phaseSequences and
turnTypes are hardcoded to 3 phases but debateData.phaseTimings from the backend
may have a different length, which can cause TypeError when indexing
phaseSequences[currentPhase] or turnTypes[state.currentPhase]; validate or
derive instead. Fix by checking debateData.phaseTimings.length against the
hardcoded arrays early (e.g., in the component init where debateData is
consumed) and either (a) throw/log an explicit error and fall back to a safe
default if lengths differ, or (b) refactor to compute phaseSequences and
turnTypes dynamically from debateData.phaseTimings (use the phase configuration
to build arrays rather than hardcoding) so access to
phaseSequences[currentPhase] and turnTypes[state.currentPhase] is always safe;
reference the symbols phaseSequences, turnTypes, debateData.phaseTimings,
currentPhase/state.currentPhase and the backend response producer in vsbot.ts
when implementing the validation or refactor.
| setState(pr => { | ||
| if (pr.timer <= 1) { | ||
| if (timerRef.current) clearInterval(timerRef.current); | ||
| if (!pr.isBotTurn) { | ||
| stopRec(); | ||
| setPopup({ show: true, message: "Time's up! Moving to next turn." }); | ||
| setTimeout(() => setPopup({ show: false, message: "" }), 2000); | ||
| const updatedState = { ...prev, timer: 0 }; | ||
| advanceTurn(updatedState); | ||
| return updatedState; | ||
| } else { | ||
| setNextTurnPending(true); | ||
| return { ...prev, timer: 0 }; | ||
| const u = { ...pr, timer: 0 }; | ||
| advanceTurn(u); | ||
| return u; | ||
| } | ||
| setNextTurnPending(true); | ||
| return { ...pr, timer: 0 }; | ||
| } | ||
| return { ...prev, timer: prev.timer - 1 }; | ||
| return { ...pr, timer: pr.timer - 1 }; | ||
| }); |
There was a problem hiding this comment.
Side effects inside setState updater functions — anti-pattern that breaks in StrictMode.
The updater passed to setState (lines 269–284) calls stopRec(), setPopup(), advanceTurn(), and setNextTurnPending() — all side effects. React 18 StrictMode double-invokes updater functions in development to surface impure updaters, which would fire these side effects twice.
This same pattern recurs at lines 304–308 (setTimeout inside updater), 311–314, 343–347, and 495. Consider moving side effects out of updaters — use useEffect reacting to state changes, or compute the new state in the updater and trigger side effects after setState returns.
🔧 Sketch: refactor timer to separate state update from side effects
useEffect(() => {
if (state.timer > 0 && !state.isDebateEnded) {
timerRef.current = setInterval(() => {
- setState(pr => {
- if (pr.timer <= 1) {
- if (timerRef.current) clearInterval(timerRef.current);
- if (!pr.isBotTurn) {
- stopRec();
- setPopup({ show: true, message: "Time's up! Moving to next turn." });
- setTimeout(() => setPopup({ show: false, message: "" }), 2000);
- const u = { ...pr, timer: 0 };
- advanceTurn(u);
- return u;
- }
- setNextTurnPending(true);
- return { ...pr, timer: 0 };
- }
- return { ...pr, timer: pr.timer - 1 };
- });
+ setState(pr => ({ ...pr, timer: Math.max(pr.timer - 1, 0) }));
}, 1000);
}
return () => { if (timerRef.current) clearInterval(timerRef.current); };
- }, [state.timer, state.isDebateEnded, state.isBotTurn, advanceTurn, stopRec]);
+ }, [state.timer, state.isDebateEnded]);
+ // React to timer expiry with a separate effect
+ useEffect(() => {
+ if (state.timer === 0 && !state.isDebateEnded) {
+ if (timerRef.current) clearInterval(timerRef.current);
+ if (!state.isBotTurn) {
+ stopRec();
+ setPopup({ show: true, message: "Time's up! Moving to next turn." });
+ setTimeout(() => setPopup({ show: false, message: "" }), 2000);
+ advanceTurn(state);
+ } else {
+ setNextTurnPending(true);
+ }
+ }
+ }, [state.timer]);🤖 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 269 - 284, The setState
updater in DebateRoom (the anonymous updater that reads pr.timer) contains side
effects (stopRec, setPopup, advanceTurn, setNextTurnPending and setTimeout)
which must be moved out of the updater; change the updater to only compute and
return the new state (e.g., reduce timer or set timer to 0 and mark a flag like
nextTurnRequested) and then, immediately after calling setState, trigger the
side effects based on the new state or use a useEffect that watches the relevant
state fields (timer, nextTurnRequested/nextTurnPending) to call stopRec(),
setPopup(), advanceTurn(), setNextTurnPending(), clearInterval(timerRef.current)
and schedule setTimeouts—ensure the updater never invokes external functions and
that useEffect guards avoid double-running in StrictMode.
| const judgeDebateResult = async (messages: Message[]) => { | ||
| try { | ||
| console.log("Starting judgment with messages:", messages); | ||
| const { result } = await judgeDebate({ | ||
| history: messages, | ||
| userId: debateData.userId, | ||
| }); | ||
| console.log("Raw judge result:", result); | ||
|
|
||
| const jsonString = extractJSON(result); | ||
| console.log("Extracted JSON string:", jsonString); | ||
|
|
||
| let judgment: JudgmentData; | ||
| try { | ||
| judgment = JSON.parse(jsonString); | ||
| } catch (parseError) { | ||
| console.error("JSON parse error:", parseError, "Trying to fix JSON..."); | ||
| // Try to fix common JSON issues | ||
| const fixedJson = jsonString | ||
| .replace(/'/g, '"') // Replace single quotes with double quotes | ||
| .replace(/(\w+):/g, '"$1":') // Add quotes to keys | ||
| .replace(/,\s*}/g, '}') // Remove trailing commas | ||
| .replace(/,\s*]/g, ']'); // Remove trailing commas in arrays | ||
| try { | ||
| judgment = JSON.parse(fixedJson); | ||
| } catch (e) { | ||
| throw new Error(`Failed to parse JSON: ${e}`); | ||
| } | ||
| } | ||
|
|
||
| console.log("Parsed judgment:", judgment); | ||
| setJudgmentData(judgment); | ||
| const { result } = await judgeDebate({ history: messages, userId: debateData.userId }); | ||
| setJudgmentData(JSON.parse(extractJSON(result)) as JudgmentData); | ||
| setPopup({ show: false, message: "" }); | ||
| setShowJudgment(true); | ||
| } catch (error) { | ||
| console.error("Judging error:", error); | ||
| // Show error to user | ||
| setPopup({ | ||
| show: true, | ||
| message: `Judgment error: ${error instanceof Error ? error.message : "Unknown error"}. Showing default results.`, | ||
| isJudging: false, | ||
| }); | ||
|
|
||
| // Set default judgment data | ||
| } catch (e) { | ||
| console.error(e); | ||
| setJudgmentData({ | ||
| opening_statement: { | ||
| user: { score: 0, reason: "Error occurred during judgment" }, | ||
| bot: { score: 0, reason: "Error occurred during judgment" }, | ||
| }, | ||
| cross_examination: { | ||
| user: { score: 0, reason: "Error occurred during judgment" }, | ||
| bot: { score: 0, reason: "Error occurred during judgment" }, | ||
| }, | ||
| answers: { | ||
| user: { score: 0, reason: "Error occurred during judgment" }, | ||
| bot: { score: 0, reason: "Error occurred during judgment" }, | ||
| }, | ||
| closing: { | ||
| user: { score: 0, reason: "Error occurred during judgment" }, | ||
| bot: { score: 0, reason: "Error occurred during judgment" }, | ||
| }, | ||
| total: { user: 0, bot: 0 }, | ||
| verdict: { | ||
| winner: "None", | ||
| reason: error instanceof Error ? error.message : "Judgment failed", | ||
| congratulations: "", | ||
| opponent_analysis: "", | ||
| }, | ||
| opening_statement: { user: { score:0, reason:"Error" }, bot: { score:0, reason:"Error" } }, | ||
| cross_examination: { user: { score:0, reason:"Error" }, bot: { score:0, reason:"Error" } }, | ||
| answers: { user: { score:0, reason:"Error" }, bot: { score:0, reason:"Error" } }, | ||
| closing: { user: { score:0, reason:"Error" }, bot: { score:0, reason:"Error" } }, | ||
| total: { user:0, bot:0 }, | ||
| verdict: { winner:"None", reason: String(e), congratulations:"", opponent_analysis:"" }, | ||
| }); | ||
| setTimeout(() => { | ||
| setPopup({ show: false, message: "" }); | ||
| setShowJudgment(true); | ||
| }, 3000); | ||
| setTimeout(() => { setPopup({ show:false, message:"" }); setShowJudgment(true); }, 3000); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "JudgmentPopup|JudgmentPopup" --type fRepository: AOSSIE-Org/DebateAI
Length of output: 45
🏁 Script executed:
rg "interface JudgmentData|type JudgmentData" -A 15Repository: AOSSIE-Org/DebateAI
Length of output: 7084
🏁 Script executed:
cd frontend && find . -name "*.tsx" -o -name "*.ts" | head -20Repository: AOSSIE-Org/DebateAI
Length of output: 626
🏁 Script executed:
cat -n frontend/src/components/JudgementPopup.tsx | head -100Repository: AOSSIE-Org/DebateAI
Length of output: 3351
🏁 Script executed:
rg "judgment\." frontend/src/components/JudgementPopup.tsx | head -30Repository: AOSSIE-Org/DebateAI
Length of output: 294
🏁 Script executed:
cat -n frontend/src/Pages/DebateRoom.tsx | sed -n '354,372p'Repository: AOSSIE-Org/DebateAI
Length of output: 1201
🏁 Script executed:
rg "function extractJSON|const extractJSON|export.*extractJSON" -A 5Repository: AOSSIE-Org/DebateAI
Length of output: 1408
🏁 Script executed:
rg "judgment\.verdict" frontend/src/components/JudgementPopup.tsx -B 2 -A 2Repository: AOSSIE-Org/DebateAI
Length of output: 440
Unsafe cast of parsed JSON to JudgmentData — missing structural validation.
Line 357 does JSON.parse(extractJSON(result)) as JudgmentData. The extractJSON() function can return "{}" on empty/falsy input, and JSON.parse will succeed silently, producing an empty object. The cast succeeds at compile time but provides no runtime validation. If the API returns incomplete JSON, properties like judgment.verdict.winner will be undefined, causing the JudgementPopup to render undefined text.
The error fallback (lines 362–369) catches API failures and parse exceptions, but not the scenario where JSON.parse succeeds on incomplete data. Add a validation check before setting judgment data to ensure required fields exist (e.g., verdict?.winner and verdict?.reason).
🤖 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 354 - 372, The
judgeDebateResult handler currently does JSON.parse(extractJSON(result)) as
JudgmentData which can silently produce incomplete objects; add runtime
structural validation before calling setJudgmentData: parse the string from
extractJSON(result), verify required fields (e.g., verdict.winner,
verdict.reason and other expected sections like opening_statement, total) exist
and have the correct types, and only then call setJudgmentData with the parsed
object; if validation fails, populate a safe fallback JudgmentData (similar to
the existing error branch) and log the validation error; implement the
validation as a small helper (e.g., validateJudgmentData(parsed): parsed is
JudgmentData | null) and use it inside judgeDebateResult to decide whether to
set the parsed data or the fallback.
1 participant
Before:
After:
Additional Notes:
The Online Debate Room was not properly reacting to the global theme state.
This update ensures that the selected mode (Light/Dark) from the Login page is consistently applied in the Online Debate Room.
Changes made:
Checklist
AI Usage Disclosure
I have used the following AI models and tools:
Claude code for guidance and implementation assistance.
Issue #326
Summary by CodeRabbit
New Features
Improvements