Fix duplicate Coach routes and improve routing structure#263
Fix duplicate Coach routes and improve routing structure#263Abhishek2005-ard wants to merge 9 commits intoAOSSIE-Org:mainfrom
Conversation
📝 WalkthroughWalkthroughReorganized app routing into explicit public vs protected nesting with a top-level catch-all redirect; added a re-entrancy guard and navigation flow in DebateRoom; passed botDesc into JudgmentPopup; reset Matchmaking UI state on mount; updated header/select styling and added high-contrast CSS variables. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DebateRoom
participant Server
participant JudgmentPopup
participant Router
User->>DebateRoom: trigger judge action
DebateRoom->>DebateRoom: check judgingRef (prevent re-entrancy)
DebateRoom->>Server: POST judgeDebateResult
Server-->>DebateRoom: judgment result / error
DebateRoom->>JudgmentPopup: open with judgment data (or default on error)
User->>JudgmentPopup: close popup
JudgmentPopup->>Router: navigate to "/"
Router-->>User: redirected to root
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/JudgementPopup.tsx (1)
94-107: Hardcoded localhost URLs will break in production.The
coachSkillsarray contains hardcodedhttp://localhost:5173URLs. These won't work in deployed environments.🐛 Suggested fix: Use relative paths
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', }, ];
🤖 Fix all issues with AI agents
In `@frontend/src/Pages/DebateRoom.tsx`:
- Around line 10-14: The hooks useRef and useNavigate are being called at module
scope (judgingRef and navigate) which violates React's Rules of Hooks; move the
calls inside the DebateRoom component so they are invoked during rendering:
remove the module-level declarations of judgingRef and navigate and create const
judgingRef = useRef(false) and const navigate = useNavigate() inside the
DebateRoom function (use appropriate typing if needed) and update any references
to those symbols to use the local variables.
🧹 Nitpick comments (2)
frontend/src/index.css (1)
88-91: Consider moving.high-contrastinside@layer basefor consistency.The
.darkand.contrasttheme classes are defined inside@layer base(lines 34-86), but.high-contrastis defined outside. This inconsistency may cause specificity issues since Tailwind's layer ordering affects cascade priority.♻️ Suggested fix
.contrast { /* ... existing contrast vars ... */ } + + .high-contrast { + --hc-hover-bg: 48 100% 50%; + --hc-hover-text: 0 0% 0%; + } } -.high-contrast { - --hc-hover-bg: 48 100% 50%; - --hc-hover-text: 0 0% 0%; -} `@layer` base {frontend/src/components/Matchmaking.tsx (1)
38-44: Redundant effect: state is already initialized to these values.This
useEffectresetsisInPooltofalseandwaitTimeto0, butuseStatealready initializes them to these exact values on lines 32-33. The effect is unnecessary unless there's a specific scenario (like HMR state persistence) that requires explicit reset.Additionally, the indentation appears incorrect—the effect body is indented with extra spaces.
♻️ Consider removing this redundant effect
- // Reset matchmaking UI state on page refresh - useEffect(() => { - setIsInPool(false); - setWaitTime(0); - }, []); - -If there's a specific reason for this reset (e.g., state rehydration from a persistence layer), please add a comment explaining the use case.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/Pages/DebateRoom.tsx`:
- Line 10: Remove the duplicate import of useNavigate (the second import
statement that re-imports useNavigate) to resolve the noRedeclare error; keep
the single existing import where useNavigate is already imported (ensure the
remaining import that references useNavigate is the only one in DebateRoom.tsx
and remove the redundant import line).
♻️ Duplicate comments (1)
frontend/src/Pages/DebateRoom.tsx (1)
13-14: Move hooks intoDebateRoomto satisfy Rules of Hooks.
useRef/useNavigateat module scope will throw at runtime;navigateis already declared inside the component.🐛 Proposed fix
-const judgingRef = useRef(false); -const navigate = useNavigate(); const DebateRoom: React.FC = () => { const location = useLocation(); const navigate = useNavigate(); @@ const recognitionRef = useRef<SpeechRecognition | null>(null); + const judgingRef = useRef(false);#!/bin/bash # Verify hooks are not declared at module scope rg -n "useRef\\(|useNavigate\\(" frontend/src/Pages/DebateRoom.tsx | head -n 20
🧹 Nitpick comments (2)
frontend/src/components/Header.tsx (2)
130-193: Inconsistent theming: popovers still use hardcoded gray colors.The header, drawer, and NavItem were updated to use semantic tokens (
text-foreground,bg-muted, etc.), but the notification popover still uses hardcoded colors liketext-gray-600,border-gray-100,text-gray-900, andbg-gray-50. This will cause visual inconsistency if dark mode or custom themes are used.Consider updating these to semantic tokens as well (e.g.,
text-muted-foreground,border-border,text-foreground,bg-muted).
195-241: Same theming inconsistency in user popover.The user profile popover also uses hardcoded
gray-*colors throughout. Apply the same semantic token updates here for consistency with the header and drawer styling changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/Pages/DebateRoom.tsx`:
- Around line 220-223: There is a duplicate declaration of the navigate hook:
remove the redundant `const navigate = useNavigate();` so only one `navigate`
variable is defined in DebateRoom (keep the existing `useNavigate()` usage and
delete the second occurrence), ensuring the single `navigate` binding is used
throughout and React hooks rules are preserved.
1 participant
What does this PR do?
This PR resolves duplicate and inconsistent route definitions related to the Coach feature in
App.jsx.Problem
/coachandcoach/strengthen-argumentroutes were defined multiple timespathpropSolution
Result
Screenshots / Testing
/coach/coach/strengthen-argument/coach/pros-consRelated Issue
Fixes: Duplicate Routes in App.jsx Causing Redundant Routing Definitions
close #261
Summary by CodeRabbit
Refactor
Bug Fixes
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.