Improve Visibility of Team Builder Section#349
Open
Abhishek2005-ard wants to merge 2 commits intoAOSSIE-Org:mainfrom
Open
Improve Visibility of Team Builder Section#349Abhishek2005-ard wants to merge 2 commits intoAOSSIE-Org:mainfrom
Abhishek2005-ard wants to merge 2 commits intoAOSSIE-Org:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis pull request updates DebateRoom to add a concurrency guard for judging, switches judgment parsing to JSON5, changes JudgmentPopup close to navigate to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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🧪 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 Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/models/transcript.go (1)
41-55:⚠️ Potential issue | 🔴 CriticalMarshalJSON produces duplicate/conflicting ID fields.
The current implementation embeds the
Aliasstruct (which contains zeroedID/UserIDfields) alongside explicit stringID/UserIDfields. This causes JSON output to contain both:
"id": "actual-hex-string"from the explicit field"id": "000000000000000000000000"from the embedded Alias (NilObjectID)The last field with the same name wins in JSON, but behavior varies across decoders and this is confusing.
Proposed fix
Use a type alias that excludes the ID fields entirely, or create a separate output struct:
func (s SavedDebateTranscript) MarshalJSON() ([]byte, error) { - type Alias SavedDebateTranscript - a := Alias(s) - a.ID = primitive.NilObjectID - a.UserID = primitive.NilObjectID - return json.Marshal(&struct { - ID string `json:"id"` - UserID string `json:"userId"` - Alias - }{ - ID: s.ID.Hex(), - UserID: s.UserID.Hex(), - Alias: a, - }) + type Alias SavedDebateTranscript + return json.Marshal(&struct { + ID string `json:"id"` + UserID string `json:"userId"` + *Alias + }{ + ID: s.ID.Hex(), + UserID: s.UserID.Hex(), + Alias: (*Alias)(&s), + }) }Note: The embedded
*Aliaswill still includeidanduserIdfields. A cleaner approach is to usejson:"-"tags on the original struct's ID fields if you always want string serialization, or create an explicit output struct without embedding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/models/transcript.go` around lines 41 - 55, The custom MarshalJSON for SavedDebateTranscript embeds Alias which still contains the ID and UserID fields, producing duplicate/conflicting "id"/"userId" keys; fix it by removing the embedded Alias with ID/UserID and instead build and marshal an explicit output struct that sets ID: s.ID.Hex(), UserID: s.UserID.Hex() and copies only the other needed fields from s (or use a type alias that omits the ID/UserID fields), updating the MarshalJSON function (SavedDebateTranscript, Alias, ID, UserID, MarshalJSON) to return JSON from that explicit output struct so no duplicate id/userId keys are emitted.backend/services/transcriptservice.go (1)
194-206:⚠️ Potential issue | 🔴 CriticalMissing
eloChangeargument - compilation error.The
SaveDebateTranscriptcall for the "against" user is missing the requiredeloChangeparameter that was added to the function signature at line 663.Proposed fix
// Save transcript for "against" user err = SaveDebateTranscript( againstUser.ID, againstUser.Email, "user_vs_user", topic, forUser.Email, resultAgainst, []models.Message{}, againstSubmission.Transcripts, + againstEloChange, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/transcriptservice.go` around lines 194 - 206, The call to SaveDebateTranscript for the "against" user is missing the newly required eloChange argument; locate the analogous SaveDebateTranscript call used for the "for" user and mirror its final parameter by passing the appropriate eloChange value (e.g., eloChange or eloChangeAgainst) into the againstUser.SaveDebateTranscript call so the call signature matches the updated function definition.
🧹 Nitpick comments (7)
frontend/src/components/Matchmaking.tsx (1)
38-44: Redundant useEffect —useStatealready initializes these values.This effect sets
isInPooltofalseandwaitTimeto0on mount, but both variables are already initialized to these exact values viauseStateon lines 32-33. The effect has no practical effect.Additionally:
- Misleading comment: States "page refresh" but
[]dependency runs on every mount, including when users switch tabs inDebatePopup, causing the component to remount.- Indentation inconsistency: Uses 4-space indentation while the rest of the component uses 2-space.
- State sync already handled: The
pool_updatehandler (lines 91-97) andmatchmaking_started/matchmaking_stoppedhandlers (lines 109-117) already synchronizeisInPoolwith server state.If you're addressing a specific edge case (e.g., stale state persisting across remounts), please clarify the scenario. Otherwise, consider removing this effect.
Suggested removal
- - - // 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 - 44, Remove the redundant useEffect in Matchmaking that calls setIsInPool(false) and setWaitTime(0) on mount because those states are already initialized via useState and are kept in sync by the pool_update and matchmaking_started/matchmaking_stopped handlers; delete the entire useEffect block (the one that references setIsInPool and setWaitTime) and ensure indentation matches the file's 2-space style after removal.frontend/src/index.css (1)
88-91: Consider moving.high-contrastinside@layer basefor consistency.The
.darkand.contrasttheme classes (lines 34-86) are defined inside@layer base, but.high-contrastis placed outside. This inconsistency may cause unexpected specificity behavior where.high-contrastrules override layered rules.Suggested placement
Move the
.high-contrastblock inside@layer basealongside the other theme classes:.contrast { /* ... existing contrast tokens ... */ } + + .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%; -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/index.css` around lines 88 - 91, The .high-contrast theme variables are defined outside `@layer` base while .dark and .contrast live inside, which can cause specificity/order issues; move the .high-contrast block into the existing `@layer` base section next to .dark and .contrast so all theme classes (.dark, .contrast, .high-contrast) are defined within `@layer` base to ensure consistent layering and predictable cascading.backend/services/transcriptservice.go (1)
975-976: Stale TODO comment.The TODO comment "Add actual Elo change tracking" is now outdated since
EloChangehas been added to the model and is being tracked. This should be updated to usetranscript.EloChange.Proposed fix
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 975 - 976, The TODO left a hardcoded zero for "eloChange" in the response; replace the stub with the tracked value by using transcript.EloChange (or the appropriate field on the Transcript/model) instead of 0 and remove the outdated TODO comment; locate the code block constructing the response (the map/object that includes "eloChange": 0) and substitute transcript.EloChange so the actual Elo change is returned.frontend/src/Pages/TeamBuilder.tsx (1)
341-343: Subtitle color may have poor visibility in dark mode.The hardcoded
text-gray-700won't adapt to dark mode themes, potentially causing readability issues. Consider using a theme-aware token for consistency with the rest of the application.Proposed fix
- <p className="text-lg text-gray-700"> + <p className="text-lg text-muted-foreground">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Pages/TeamBuilder.tsx` around lines 341 - 343, The subtitle uses a hardcoded color class ("text-gray-700") that won't adapt to dark mode; update the <p> element in TeamBuilder.tsx to use a theme-aware token or Tailwind dark variant (for example replace className="text-lg text-gray-700" with className="text-lg text-gray-700 dark:text-gray-300" or with your app's design token like "text-lg text-secondary") so the subtitle remains readable in both light and dark themes.backend/controllers/profile_controller.go (1)
257-257: Remove or update the stale TODO comment.The comment
// TODO: Add actual Elo change trackingis inconsistent with the code—you're now usingtranscript.EloChange, which indicates Elo tracking is implemented. Either remove this TODO or update it to reflect any remaining work.Also, add a space before
//for consistent formatting.✏️ 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 TODO comment is stale and formatting is inconsistent: remove or update the comment next to transcript.EloChange in the response/struct assembly so it accurately reflects current behavior (either delete "// TODO: Add actual Elo change tracking" or replace it with a short, accurate note) and ensure there is a space before "//" for consistent formatting where the field "eloChange": transcript.EloChange is set.frontend/src/components/Header.tsx (2)
130-193: Inconsistent use of design tokens in notification popover.The header, drawer, and NavItem now use design tokens (
text-foreground,bg-muted, etc.), but the notification popover still uses hardcoded colors liketext-gray-600,text-gray-900,border-gray-100,bg-gray-50, andtext-blue-600. This inconsistency may cause visual discrepancies in dark mode or when theming is applied.Consider migrating these to design tokens for consistency:
text-gray-600→text-muted-foregroundtext-gray-900→text-foregroundborder-gray-100→border-borderbg-gray-50→bg-muted🤖 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 130 - 193, The notification popover uses hardcoded Tailwind gray/blue classes causing theming inconsistencies; update the class names inside the Popover/PopoverContent block (the JSX that renders the Bell button, PopoverContent header, notification items, and the "Mark all read" Button) to use design tokens: replace text-gray-600 with text-muted-foreground, text-gray-900 with text-foreground, border-gray-100 with border-border, bg-gray-50 with bg-muted and text-blue-600 on the Button with primary-foreground or an appropriate token; ensure the unread item background (bg-blue-50/50 and bg-blue-500) maps to a token (e.g., bg-accent/50 and accent-foreground or bg-accent and accent-foreground) so colors align with existing token usage across Header, Drawer, and NavItem.
195-241: User popover also uses hardcoded colors.Same observation as the notification popover—this section uses
text-gray-900,text-gray-500,bg-gray-100,border-gray-200, etc. Consider updating to design tokens for consistent theming.🤖 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 195 - 241, The popover UI in Header.tsx (Popover / PopoverContent / PopoverTrigger) uses hardcoded Tailwind color classes (e.g., text-gray-900, text-gray-500, bg-gray-100, border-gray-200) which breaks theming; replace those specific utility classes on the avatar img, h4, p, the User ID/Rating spans and the container borders/backgrounds with your design token classes (for example text-default/text-muted, bg-surface, border-muted, text-accent) provided by the design system so the popover respects global theming and dark mode.
🤖 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`:
- Around line 189-190: The code references an undefined variable forRecord
(forRecord.RatingChange) causing a compilation error because
debateRecord/opponentRecord are only produced by UpdateRatings(); fix by either
moving the SaveDebateTranscript calls so they occur after UpdateRatings() and
then pass debateRecord.RatingChange and opponentRecord.RatingChange, or if you
must keep SaveDebateTranscript here, replace forRecord.RatingChange with a safe
placeholder (e.g., 0.0) until the real rating changes are available; update
calls to SaveDebateTranscript and any related variables accordingly (look for
SaveDebateTranscript, UpdateRatings, debateRecord, opponentRecord, forRecord,
RatingChange).
In `@frontend/src/components/ui/select.tsx`:
- Around line 121-122: The Tailwind classes
"high-contrast:data-[highlighted]:bg-[hsl(var(--hc-hover-bg))]" and
"high-contrast:data-[highlighted]:text-[hsl(var(--hc-hover-text))]" in
select.tsx use a non-configured variant; either add a custom variant in
tailwind.config.js by registering a plugin that calls
addVariant('high-contrast', '.high-contrast &') so those prefixed classes become
active, or remove the variant prefix and implement the equivalent styles via the
existing .high-contrast selector in index.css; update the config/plugin or
refactor the two affected class strings in Select component accordingly.
In `@frontend/src/Pages/DebateRoom.tsx`:
- Line 10: The project imports JSON5 (import JSON5 from "json5") and uses it in
judgeDebateResult, but json5 is missing from frontend/package.json; add "json5"
to the dependencies in package.json and run the package manager to install it
(e.g., npm install --save json5 or yarn add json5) so the import in
DebateRoom.tsx and usage in judgeDebateResult resolve correctly.
In `@frontend/src/Pages/TeamBuilder.tsx`:
- Around line 338-340: The Tailwind class "font-600" used on the <h1> element in
TeamBuilder.tsx is invalid; update the className on the Team Builder heading
(the <h1> in TeamBuilder) to use a valid semantic weight such as "font-semibold"
(or "font-bold"/"font-extrabold" if you prefer a heavier weight) instead of
"font-600".
---
Outside diff comments:
In `@backend/models/transcript.go`:
- Around line 41-55: The custom MarshalJSON for SavedDebateTranscript embeds
Alias which still contains the ID and UserID fields, producing
duplicate/conflicting "id"/"userId" keys; fix it by removing the embedded Alias
with ID/UserID and instead build and marshal an explicit output struct that sets
ID: s.ID.Hex(), UserID: s.UserID.Hex() and copies only the other needed fields
from s (or use a type alias that omits the ID/UserID fields), updating the
MarshalJSON function (SavedDebateTranscript, Alias, ID, UserID, MarshalJSON) to
return JSON from that explicit output struct so no duplicate id/userId keys are
emitted.
In `@backend/services/transcriptservice.go`:
- Around line 194-206: The call to SaveDebateTranscript for the "against" user
is missing the newly required eloChange argument; locate the analogous
SaveDebateTranscript call used for the "for" user and mirror its final parameter
by passing the appropriate eloChange value (e.g., eloChange or eloChangeAgainst)
into the againstUser.SaveDebateTranscript call so the call signature matches the
updated function definition.
---
Nitpick comments:
In `@backend/controllers/profile_controller.go`:
- Line 257: The TODO comment is stale and formatting is inconsistent: remove or
update the comment next to transcript.EloChange in the response/struct assembly
so it accurately reflects current behavior (either delete "// TODO: Add actual
Elo change tracking" or replace it with a short, accurate note) and ensure there
is a space before "//" for consistent formatting where the field "eloChange":
transcript.EloChange is set.
In `@backend/services/transcriptservice.go`:
- Around line 975-976: The TODO left a hardcoded zero for "eloChange" in the
response; replace the stub with the tracked value by using transcript.EloChange
(or the appropriate field on the Transcript/model) instead of 0 and remove the
outdated TODO comment; locate the code block constructing the response (the
map/object that includes "eloChange": 0) and substitute transcript.EloChange so
the actual Elo change is returned.
In `@frontend/src/components/Header.tsx`:
- Around line 130-193: The notification popover uses hardcoded Tailwind
gray/blue classes causing theming inconsistencies; update the class names inside
the Popover/PopoverContent block (the JSX that renders the Bell button,
PopoverContent header, notification items, and the "Mark all read" Button) to
use design tokens: replace text-gray-600 with text-muted-foreground,
text-gray-900 with text-foreground, border-gray-100 with border-border,
bg-gray-50 with bg-muted and text-blue-600 on the Button with primary-foreground
or an appropriate token; ensure the unread item background (bg-blue-50/50 and
bg-blue-500) maps to a token (e.g., bg-accent/50 and accent-foreground or
bg-accent and accent-foreground) so colors align with existing token usage
across Header, Drawer, and NavItem.
- Around line 195-241: The popover UI in Header.tsx (Popover / PopoverContent /
PopoverTrigger) uses hardcoded Tailwind color classes (e.g., text-gray-900,
text-gray-500, bg-gray-100, border-gray-200) which breaks theming; replace those
specific utility classes on the avatar img, h4, p, the User ID/Rating spans and
the container borders/backgrounds with your design token classes (for example
text-default/text-muted, bg-surface, border-muted, text-accent) provided by the
design system so the popover respects global theming and dark mode.
In `@frontend/src/components/Matchmaking.tsx`:
- Around line 38-44: Remove the redundant useEffect in Matchmaking that calls
setIsInPool(false) and setWaitTime(0) on mount because those states are already
initialized via useState and are kept in sync by the pool_update and
matchmaking_started/matchmaking_stopped handlers; delete the entire useEffect
block (the one that references setIsInPool and setWaitTime) and ensure
indentation matches the file's 2-space style after removal.
In `@frontend/src/index.css`:
- Around line 88-91: The .high-contrast theme variables are defined outside
`@layer` base while .dark and .contrast live inside, which can cause
specificity/order issues; move the .high-contrast block into the existing `@layer`
base section next to .dark and .contrast so all theme classes (.dark, .contrast,
.high-contrast) are defined within `@layer` base to ensure consistent layering and
predictable cascading.
In `@frontend/src/Pages/TeamBuilder.tsx`:
- Around line 341-343: The subtitle uses a hardcoded color class
("text-gray-700") that won't adapt to dark mode; update the <p> element in
TeamBuilder.tsx to use a theme-aware token or Tailwind dark variant (for example
replace className="text-lg text-gray-700" with className="text-lg text-gray-700
dark:text-gray-300" or with your app's design token like "text-lg
text-secondary") so the subtitle remains readable in both light and dark themes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0cffd401-284b-4969-9938-a9e9db2879d1
📒 Files selected for processing (12)
backend/config/config.prod.sample.ymlbackend/controllers/profile_controller.gobackend/models/transcript.gobackend/services/transcriptservice.gofrontend/src/App.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
Contributor
|
12 files changes, for a small UI fix? |
Abhishek2005-ard
force-pushed
the
fix/team-builder-ui
branch
from
23ad9ed to
113540e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/Pages/DebateRoom.tsx (1)
1-10: Consider splitting unrelated changes into a separate PR.This file contains judgment concurrency guards, JSON5 parsing changes, navigation behavior updates, and dark mode styling—none of which relate to the stated PR objective of improving TeamBuilder section visibility. Bundling unrelated changes increases review complexity and risk.
Consider moving these DebateRoom enhancements to a dedicated PR for cleaner history and easier rollback if needed.
🤖 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 1 - 10, This file mixes unrelated changes with the TeamBuilder visibility PR—undo or extract the DebateRoom-specific changes (judgment concurrency guards around judgeDebate/concedeDebate, JSON5 parsing changes, navigation behavior updates around useNavigate/useLocation, and dark-mode styling and the JudgmentPopup/Mic imports) into a separate branch/PR; specifically, revert or remove edits in DebateRoom.tsx that touch sendDebateMessage, judgeDebate, concedeDebate logic, JSON5 usage, and dark-mode class/style changes so this PR only contains TeamBuilder visibility work, then open a new PR containing the DebateRoom enhancements (retain function names like judgeDebate, concedeDebate, sendDebateMessage, JudgmentPopup, JSON5 usage) for independent review.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/src/Pages/DebateRoom.tsx`:
- Around line 1-10: This file mixes unrelated changes with the TeamBuilder
visibility PR—undo or extract the DebateRoom-specific changes (judgment
concurrency guards around judgeDebate/concedeDebate, JSON5 parsing changes,
navigation behavior updates around useNavigate/useLocation, and dark-mode
styling and the JudgmentPopup/Mic imports) into a separate branch/PR;
specifically, revert or remove edits in DebateRoom.tsx that touch
sendDebateMessage, judgeDebate, concedeDebate logic, JSON5 usage, and dark-mode
class/style changes so this PR only contains TeamBuilder visibility work, then
open a new PR containing the DebateRoom enhancements (retain function names like
judgeDebate, concedeDebate, sendDebateMessage, JudgmentPopup, JSON5 usage) for
independent review.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29acea39-d285-493f-ae53-9275a94638a4
📒 Files selected for processing (2)
frontend/src/Pages/DebateRoom.tsxfrontend/src/Pages/TeamBuilder.tsx
✅ Files skipped from review due to trivial changes (1)
- frontend/src/Pages/TeamBuilder.tsx
Contributor
Author
|
hello @bhavik-mangla |
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
2 participants
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 visibility and UI clarity of the Team Builder section in the interface. Previously, the heading and description of the Team Builder section were not prominent enough, making it harder for users to immediately notice the feature. This update enhances the styling to make the section more visually clear and user-friendly.
Changes Made
Increased the heading font size
Improved font weight and spacing
Updated text color for better contrast
Improved layout alignment for better readability
Before
After
PR Importance
Minor UI Enhancement
Issue Link
Closes #348
Summary by CodeRabbit
New Features
Bug Fixes
Style
Chores