Fix/unique displayname v2#384
Conversation
📝 WalkthroughWalkthroughThis PR introduces display name uniqueness validation across the application. A MongoDB index on the Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Frontend)
participant Frontend as Profile.tsx
participant Service as profileService
participant API as Backend API
participant DB as MongoDB
rect rgba(100, 150, 200, 0.5)
note over User,DB: Display Name Availability Check Flow
User->>Frontend: Input display name
Frontend->>Frontend: Debounce (300ms)
Frontend->>Service: checkDisplayNameAvailability()
Service->>API: GET /user/check-displayname?displayName=...
API->>DB: Query users collection
DB-->>API: Return query result
API-->>Service: {available: true/false}
Service-->>Frontend: Response received
Frontend->>Frontend: Update usernameStatus state
Frontend->>User: Show availability feedback
end
rect rgba(150, 100, 200, 0.5)
note over User,DB: Profile Update with Display Name Validation
User->>Frontend: Click Save button
Frontend->>Frontend: Validate usernameStatus != "taken"
Frontend->>Service: updateProfile(displayName, ...)
Service->>API: PUT /profile with displayName
API->>DB: Query for existing displayName
DB-->>API: Check if taken by another user
alt Display name available
API->>DB: Update user document with $set
DB-->>API: Update successful
API-->>Service: {success: true}
Service-->>Frontend: Update response
Frontend->>API: GET /profile (refetch)
API->>DB: Fetch updated profile
DB-->>API: Return profile
API-->>Frontend: Profile data
Frontend->>Frontend: Refresh profile, dashboard, recentDebates
Frontend->>User: Show success
else Display name taken
API-->>Service: Error 409 Conflict
Service-->>Frontend: Throw error
Frontend->>User: Show error message
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/src/Pages/Profile.tsx (1)
258-300:⚠️ Potential issue | 🟡 Minor
handleSubmitallows saving an empty or whitespace-only displayName.The guard on line 262 only blocks
usernameStatus === "taken". If the user clears the input, theonChangehandler (line 709-712) setsusernameStatusback to"idle"and leavesdashboard.profile.displayNameempty — at which point Save is re-enabled and submits"". Given the new unique+sparse index ondisplayName(inbackend/db/db.go) doesn't actually exclude empty strings, this can cause a backend 409/500 for the second user who tries it, and is a poor UX either way.Also consider blocking submit while
usernameStatus === "checking"so a keyboard-Enter during the debounce window doesn't bypass the availability check.🔧 Suggested fix
if (field === "displayName" && usernameStatus === "taken") { setErrorMessage("Display name is already taken."); return; } +if ( + field === "displayName" && + (!dashboard.profile.displayName?.trim() || usernameStatus === "checking") +) { + setErrorMessage( + usernameStatus === "checking" + ? "Please wait while we verify the display name." + : "Display name cannot be empty." + ); + return; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Pages/Profile.tsx` around lines 258 - 300, In handleSubmit, add validation to reject empty or whitespace-only displayName and prevent submission while usernameStatus is "checking"; specifically, before calling updateProfile in the handleSubmit function, trim dashboard.profile.displayName and if it is empty setErrorMessage("Display name cannot be empty.") and return, and also if usernameStatus === "checking" setErrorMessage("Checking display name availability, please wait.") and return; keep the existing usernameStatus === "taken" check, then proceed to call updateProfile/getProfile and the existing success/error handlers as before.backend/controllers/auth.go (1)
73-109:⚠️ Potential issue | 🟠 MajorRace window:
GoogleLoginrelies on pre-check but doesn't translate duplicate-key errors fromInsertOne.The pre-check at lines 75-83 is not atomic with
InsertOneon line 102, so two concurrent Google logins picking the samenicknamecan both pass the check and race intoInsertOne. The unique index ondisplayName(added inbackend/db/db.go) will reject one of them, but this handler returns a generic 500 ("Failed to create user") instead of a 400 "Display name already taken" — inconsistent withSignUp(which handles this at lines 196-199).🔧 Suggested fix
result, err := db.MongoDatabase.Collection("users").InsertOne(dbCtx, newUser) if err != nil { + if mongo.IsDuplicateKeyError(err) { + ctx.JSON(400, gin.H{"error": "Display name already taken"}) + return + } ctx.JSON(500, gin.H{"error": "Failed to create user", "message": err.Error()}) return }Additionally, for Google OAuth you may want to auto-suffix
nicknameon collision (e.g.nickname-2,nickname-3) rather than failing the login outright, since the user has no way to edit the name during the OAuth flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/auth.go` around lines 73 - 109, The GoogleLogin handler currently does a non-atomic pre-check for displayName then calls InsertOne, but InsertOne can fail with a duplicate-key error under race and is treated as a generic 500; update the GoogleLogin flow (the code around InsertOne in the GoogleLogin handler) to detect Mongo duplicate-key errors (e.g. inspect the returned error for mongo.WriteException with Code 11000 or use a helper like mongo.IsDuplicateKeyError if available) and translate that into the same 400 "Display name already taken" response as SignUp does; optionally implement a small retry/suffix strategy (generate nickname-2, nickname-3, attempt InsertOne again) before returning the 400 so OAuth users can be auto-suffixed instead of failing.backend/controllers/profile_controller.go (1)
300-349:⚠️ Potential issue | 🟠 MajorPartial updates and empty
displayNameboth cause data loss or conflicts.Two issues in the profile update path:
Partial-update vulnerability. The
updateProfilefunction accepts optional parameters (twitter?,instagram?,linkedin?,avatarUrl?). When omitted,JSON.stringifyexcludes them from the payload. The backend's plainstringstruct fields deserialize missing JSON properties to"", andsetFieldsunconditionally$sets every field to these empty defaults. While the current frontend always passes all profile fields, this code is vulnerable to accidental data loss if frontend behavior changes or if another client does a partial update. Either require all fields in every request (and document that contract), or switch to*stringand only include keys that were present in the payload.Empty
displayNameviolates unique sparse index. ThedisplayNameindex is correctly configured asuniqueandsparse. However, a sparse index excludes missing fields but still indexes empty strings. WhennewDisplayName == "", the uniqueness pre-check is skipped (correct), butsetFields["displayName"] = ""is still persisted. The second user who clears theirdisplayNamewill havedisplayName: ""in the database, triggeringmongo.IsDuplicateKeyErrorwith a misleading "Display name already taken" 409 response. Either skip the field when empty (so the sparse index ignores it) or use$unset:Proposed fix (sketch)
var updateData struct { - DisplayName string `json:"displayName"` - Bio string `json:"bio"` - Twitter string `json:"twitter"` - Instagram string `json:"instagram"` - LinkedIn string `json:"linkedin"` - AvatarURL string `json:"avatarUrl"` + DisplayName *string `json:"displayName"` + Bio *string `json:"bio"` + Twitter *string `json:"twitter"` + Instagram *string `json:"instagram"` + LinkedIn *string `json:"linkedin"` + AvatarURL *string `json:"avatarUrl"` } ... setFields := bson.M{"updatedAt": time.Now()} unsetFields := bson.M{} if updateData.DisplayName != nil { trimmed := strings.TrimSpace(*updateData.DisplayName) if trimmed == "" { unsetFields["displayName"] = "" } else { // uniqueness pre-check against `trimmed` ... setFields["displayName"] = trimmed } } // repeat for other fields ... update := bson.M{"$set": setFields} if len(unsetFields) > 0 { update["$unset"] = unsetFields }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/profile_controller.go` around lines 300 - 349, Change the inline updateData struct fields to pointer types (e.g., DisplayName *string, Bio *string, Twitter *string, Instagram *string, LinkedIn *string, AvatarURL *string) so you can detect omitted keys; only add keys to the bson.M "$set" when the corresponding pointer is non-nil and non-empty after trimming. For displayName: run the uniqueness pre-check only when updateData.DisplayName != nil and trimmed != ""; if trimmed == "" treat it as a clear-request and add that field to a "$unset" map instead of setting it to "", or simply omit it to preserve sparseness. Finally call UpdateOne with a combined update document (include "$set" only if non-empty and "$unset" only if non-empty) and keep the mongo.IsDuplicateKeyError handling as-is.
🧹 Nitpick comments (1)
backend/controllers/profile_controller.go (1)
353-382: Consider case-insensitive displayName comparison.The displayName unique index (backend/db/db.go) lacks collation for case-insensitive matching. Users typically expect usernames to be case-insensitive;
"Alice"and"alice"should be treated as duplicates. Add a collation-based unique index with{locale: "en", strength: 2}and update queries inCheckDisplayNameand auth signup flow to match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/profile_controller.go` around lines 353 - 382, The displayName uniqueness must be enforced case-insensitively: update the users collection index creation in backend/db/db.go to create a unique index on "displayName" with a Collation set to {Locale: "en", Strength: 2} (ensure you drop/recreate or alter the index if needed), and update all reads/writes that match displayName to use the same collation; specifically modify CheckDisplayName (the MongoDatabase.Collection("users").FindOne call in CheckDisplayName) to pass a FindOneOptions with Collation {Locale: "en", Strength: 2} and change the signup flow (the signup handler that inserts/queries users) to use the same collation when checking for existing displayName and when creating the unique index so comparisons treat "Alice" and "alice" as the same.
🤖 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/cmd/server/main.go`:
- Around line 36-38: Indentation in main is mixed spaces/tabs around the
db.EnsureIndexes() call and a trailing tab after the handler, causing
gofmt/govet failures; open the main function and replace the space-indented
lines around db.EnsureIndexes() with tab indentation to match the rest of the
file (the block containing db.EnsureIndexes()), remove any trailing tab
characters after the HTTP handler registration, and run gofmt (or gofmt -w) to
ensure consistent tabs throughout.
In `@backend/controllers/auth.go`:
- Around line 154-164: The signup flow derives defaultDisplayName via
utils.ExtractNameFromEmail and currently rejects signups when that displayName
collides (existingDisplayName) which blocks users from resolving it; update the
logic in the SignUp handler around db.MongoDatabase.Collection("users").FindOne
so that instead of returning "Display name already taken" it tries to
auto-resolve collisions by attempting incremental suffixes (e.g.,
defaultDisplayName, defaultDisplayName-2, defaultDisplayName-3, ...) and
checking existence until an unused displayName is found, then continue creating
the user with that resolved displayName; alternatively implement a deterministic
fallback that assigns a unique temporary displayName (e.g., "user-<random>")
when all suffix attempts fail and persist that value so the account is created
rather than returning a 400.
In `@backend/controllers/profile_controller.go`:
- Around line 316-325: The pre-check using
db.MongoDatabase.Collection("users").FindOne when validating newDisplayName
currently swallows non-ErrNoDocuments errors; change the logic in the display
name uniqueness block (the newDisplayName/FindOne/decode/existing flow) to
explicitly handle mongo.ErrNoDocuments as the only acceptable “not found” case
and return an internal server error (HTTP 500) for any other err returned by
FindOne (and log the DB error), following the pattern used in auth.go
(SignUp/GoogleLogin); keep the existing behavior of returning HTTP 409 when err
== nil and existing.Email != email.
- Around line 145-156: The UpdateOne call that sets user.Rating and
user.LastRatingUpdate is currently discarding errors (using "_, _ ="), so
capture the result and error from db.MongoDatabase.Collection("users").UpdateOne
(using the existing dbCtx and user.ID), check if err != nil, and log the error
with context (include user.ID, user.Rating, and operation name) via the
package's logger rather than ignoring it; after logging decide whether to return
an error response from the GetProfile handler or continue with the in-memory
defaults, but do not silently swallow the UpdateOne error.
- Line 24: The exported sentinel ErrUsernameTaken is unused; either remove its
declaration or wire it into the relevant error paths: in UpdateProfile and
CheckDisplayName return ErrUsernameTaken when a username/display name conflict
is detected (replace current string/error returns), and update any callers to
compare errors against ErrUsernameTaken (errors.Is or direct equality) so the
sentinel is honored; if you choose removal, delete the ErrUsernameTaken variable
and any references in comments/tests to avoid dead export.
In `@backend/db/db.go`:
- Around line 58-75: EnsureIndexes currently creates a unique sparse index on
users.displayName which doesn't prevent duplicates of empty strings and can
crash startup if duplicates exist; update the index options in the EnsureIndexes
function (the indexModel used with MongoDatabase.Collection("users") /
usersCol.Indexes().CreateOne) to use SetPartialFilterExpression that enforces
uniqueness only for non-empty string displayName values (e.g. filter where
displayName exists, is a string, and $ne ""), and change the startup error
handling where EnsureIndexes is invoked (avoid log.Fatalf on error in main.go)
to log the failure and continue so operators can remediate duplicates without
bringing the server down.
In `@frontend/src/Pages/Profile.tsx`:
- Around line 706-725: The onChange debounce uses debounceTimer.current but
never clears it on unmount and doesn't guard against stale/Out‑of‑order
responses; add a useEffect cleanup that clears debounceTimer.current on unmount,
and change the async debounce handler to use an AbortController (passed into
checkDisplayNameAvailability) or maintain a latestQuery ref (e.g.,
latestDisplayNameRef) to verify the response matches the most recent val before
calling setUsernameStatus; update calls to getAuthToken,
checkDisplayNameAvailability, and setUsernameStatus accordingly and ensure any
thrown abort is handled so state is not set for cancelled/stale requests.
---
Outside diff comments:
In `@backend/controllers/auth.go`:
- Around line 73-109: The GoogleLogin handler currently does a non-atomic
pre-check for displayName then calls InsertOne, but InsertOne can fail with a
duplicate-key error under race and is treated as a generic 500; update the
GoogleLogin flow (the code around InsertOne in the GoogleLogin handler) to
detect Mongo duplicate-key errors (e.g. inspect the returned error for
mongo.WriteException with Code 11000 or use a helper like
mongo.IsDuplicateKeyError if available) and translate that into the same 400
"Display name already taken" response as SignUp does; optionally implement a
small retry/suffix strategy (generate nickname-2, nickname-3, attempt InsertOne
again) before returning the 400 so OAuth users can be auto-suffixed instead of
failing.
In `@backend/controllers/profile_controller.go`:
- Around line 300-349: Change the inline updateData struct fields to pointer
types (e.g., DisplayName *string, Bio *string, Twitter *string, Instagram
*string, LinkedIn *string, AvatarURL *string) so you can detect omitted keys;
only add keys to the bson.M "$set" when the corresponding pointer is non-nil and
non-empty after trimming. For displayName: run the uniqueness pre-check only
when updateData.DisplayName != nil and trimmed != ""; if trimmed == "" treat it
as a clear-request and add that field to a "$unset" map instead of setting it to
"", or simply omit it to preserve sparseness. Finally call UpdateOne with a
combined update document (include "$set" only if non-empty and "$unset" only if
non-empty) and keep the mongo.IsDuplicateKeyError handling as-is.
In `@frontend/src/Pages/Profile.tsx`:
- Around line 258-300: In handleSubmit, add validation to reject empty or
whitespace-only displayName and prevent submission while usernameStatus is
"checking"; specifically, before calling updateProfile in the handleSubmit
function, trim dashboard.profile.displayName and if it is empty
setErrorMessage("Display name cannot be empty.") and return, and also if
usernameStatus === "checking" setErrorMessage("Checking display name
availability, please wait.") and return; keep the existing usernameStatus ===
"taken" check, then proceed to call updateProfile/getProfile and the existing
success/error handlers as before.
---
Nitpick comments:
In `@backend/controllers/profile_controller.go`:
- Around line 353-382: The displayName uniqueness must be enforced
case-insensitively: update the users collection index creation in
backend/db/db.go to create a unique index on "displayName" with a Collation set
to {Locale: "en", Strength: 2} (ensure you drop/recreate or alter the index if
needed), and update all reads/writes that match displayName to use the same
collation; specifically modify CheckDisplayName (the
MongoDatabase.Collection("users").FindOne call in CheckDisplayName) to pass a
FindOneOptions with Collation {Locale: "en", Strength: 2} and change the signup
flow (the signup handler that inserts/queries users) to use the same collation
when checking for existing displayName and when creating the unique index so
comparisons treat "Alice" and "alice" as the same.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be0bc5ea-575f-42f6-b16c-37d5226b0bcd
📒 Files selected for processing (9)
backend/cmd/server/main.gobackend/controllers/auth.gobackend/controllers/profile_controller.gobackend/db/db.gobackend/models/user.gobackend/routes/profile.gofrontend/src/Pages/Profile.tsxfrontend/src/services/profileService.tsfrontend/src/types/user.ts
| if err := db.EnsureIndexes(); err != nil { | ||
| log.Fatalf("Failed to ensure indexes: %v", err) | ||
| } |
There was a problem hiding this comment.
gofmt: mixed tab/space indentation.
Lines 37-38 are indented with spaces while the rest of main uses tabs; same on line 108 (and a trailing tab after the handler). This will fail gofmt/go vet in CI.
🔧 Fix
- if err := db.EnsureIndexes(); err != nil {
- log.Fatalf("Failed to ensure indexes: %v", err)
- }
+ if err := db.EnsureIndexes(); err != nil {
+ log.Fatalf("Failed to ensure indexes: %v", err)
+ }- auth.GET("/user/check-displayname", routes.CheckDisplayNameRouteHandler)
+ auth.GET("/user/check-displayname", routes.CheckDisplayNameRouteHandler)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := db.EnsureIndexes(); err != nil { | |
| log.Fatalf("Failed to ensure indexes: %v", err) | |
| } | |
| if err := db.EnsureIndexes(); err != nil { | |
| log.Fatalf("Failed to ensure indexes: %v", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/cmd/server/main.go` around lines 36 - 38, Indentation in main is
mixed spaces/tabs around the db.EnsureIndexes() call and a trailing tab after
the handler, causing gofmt/govet failures; open the main function and replace
the space-indented lines around db.EnsureIndexes() with tab indentation to match
the rest of the file (the block containing db.EnsureIndexes()), remove any
trailing tab characters after the HTTP handler registration, and run gofmt (or
gofmt -w) to ensure consistent tabs throughout.
| // Check if displayName is already taken | ||
| defaultDisplayName := utils.ExtractNameFromEmail(request.Email) | ||
| var existingDisplayName models.User | ||
| err = db.MongoDatabase.Collection("users").FindOne(dbCtx, bson.M{"displayName": defaultDisplayName}).Decode(&existingDisplayName) | ||
| if err == nil { | ||
| ctx.JSON(400, gin.H{"error": "Display name already taken"}) | ||
| return | ||
| } else if !errors.Is(err, mongo.ErrNoDocuments) { | ||
| ctx.JSON(500, gin.H{"error": "Database error"}) | ||
| return | ||
| } |
There was a problem hiding this comment.
UX: SignUp rejects with "Display name already taken" but the user cannot choose/change the display name at signup.
defaultDisplayName is deterministically derived from the email local part. If a collision occurs (e.g., john@a.com already exists and john@b.com signs up), the signup fails with an error the user has no way to resolve — they can't supply an alternate display name. Consider one of:
- Auto-suffix on collision (e.g., try
john,john-2,john-3, …) before returning the generic email-already-exists error, or - Allow the signup to complete with a temporary/unique displayName (e.g.,
user-<random>) and force the user to choose a name on first login.
Either approach avoids a dead-end for users whose email local parts happen to collide.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/controllers/auth.go` around lines 154 - 164, The signup flow derives
defaultDisplayName via utils.ExtractNameFromEmail and currently rejects signups
when that displayName collides (existingDisplayName) which blocks users from
resolving it; update the logic in the SignUp handler around
db.MongoDatabase.Collection("users").FindOne so that instead of returning
"Display name already taken" it tries to auto-resolve collisions by attempting
incremental suffixes (e.g., defaultDisplayName, defaultDisplayName-2,
defaultDisplayName-3, ...) and checking existence until an unused displayName is
found, then continue creating the user with that resolved displayName;
alternatively implement a deterministic fallback that assigns a unique temporary
displayName (e.g., "user-<random>") when all suffix attempts fail and persist
that value so the account is created rather than returning a 400.
| ) | ||
|
|
||
| // Calculate new Elo ratings using float64 | ||
| var ErrUsernameTaken = errors.New("username already taken") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '\bErrUsernameTaken\b' --type=go -C2Repository: AOSSIE-Org/DebateAI
Length of output: 452
Remove unused ErrUsernameTaken sentinel or wire it into error handling paths.
The variable declared at line 24 is never referenced or returned anywhere in the codebase. Either integrate it into UpdateProfile and CheckDisplayName error responses, or remove it to avoid dead exports.
🤖 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 24, The exported sentinel
ErrUsernameTaken is unused; either remove its declaration or wire it into the
relevant error paths: in UpdateProfile and CheckDisplayName return
ErrUsernameTaken when a username/display name conflict is detected (replace
current string/error returns), and update any callers to compare errors against
ErrUsernameTaken (errors.Is or direct equality) so the sentinel is honored; if
you choose removal, delete the ErrUsernameTaken variable and any references in
comments/tests to avoid dead export.
| if user.Rating == 0 { | ||
| user.Rating = 1200.0 | ||
| user.LastRatingUpdate = time.Now() | ||
| _, err = db.MongoDatabase.Collection("users").UpdateOne( | ||
| _, _ = db.MongoDatabase.Collection("users").UpdateOne( | ||
| dbCtx, | ||
| bson.M{"_id": user.ID}, | ||
| bson.M{"$set": bson.M{ | ||
| "rating": user.Rating, | ||
| "lastRatingUpdate": user.LastRatingUpdate, | ||
| }}, | ||
| ) | ||
| if err != nil { | ||
| } else { | ||
| } | ||
| } |
There was a problem hiding this comment.
Silent failure on rating default-update.
The previous error check was dropped in favor of _, _ =. If this UpdateOne fails transiently, user.Rating == 0 persists in DB and the branch will fire again on every subsequent GetProfile call, compounding load. At minimum log the error so it's observable.
Proposed fix
- _, _ = db.MongoDatabase.Collection("users").UpdateOne(
+ if _, uerr := db.MongoDatabase.Collection("users").UpdateOne(
dbCtx,
bson.M{"_id": user.ID},
bson.M{"$set": bson.M{
"rating": user.Rating,
"lastRatingUpdate": user.LastRatingUpdate,
}},
- )
+ ); uerr != nil {
+ log.Printf("GetProfile: failed to persist default rating for %s: %v", user.Email, uerr)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if user.Rating == 0 { | |
| user.Rating = 1200.0 | |
| user.LastRatingUpdate = time.Now() | |
| _, err = db.MongoDatabase.Collection("users").UpdateOne( | |
| _, _ = db.MongoDatabase.Collection("users").UpdateOne( | |
| dbCtx, | |
| bson.M{"_id": user.ID}, | |
| bson.M{"$set": bson.M{ | |
| "rating": user.Rating, | |
| "lastRatingUpdate": user.LastRatingUpdate, | |
| }}, | |
| ) | |
| if err != nil { | |
| } else { | |
| } | |
| } | |
| if user.Rating == 0 { | |
| user.Rating = 1200.0 | |
| user.LastRatingUpdate = time.Now() | |
| if _, uerr := db.MongoDatabase.Collection("users").UpdateOne( | |
| dbCtx, | |
| bson.M{"_id": user.ID}, | |
| bson.M{"$set": bson.M{ | |
| "rating": user.Rating, | |
| "lastRatingUpdate": user.LastRatingUpdate, | |
| }}, | |
| ); uerr != nil { | |
| log.Printf("GetProfile: failed to persist default rating for %s: %v", user.Email, uerr) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/controllers/profile_controller.go` around lines 145 - 156, The
UpdateOne call that sets user.Rating and user.LastRatingUpdate is currently
discarding errors (using "_, _ ="), so capture the result and error from
db.MongoDatabase.Collection("users").UpdateOne (using the existing dbCtx and
user.ID), check if err != nil, and log the error with context (include user.ID,
user.Rating, and operation name) via the package's logger rather than ignoring
it; after logging decide whether to return an error response from the GetProfile
handler or continue with the in-memory defaults, but do not silently swallow the
UpdateOne error.
| // Check displayName uniqueness | ||
| newDisplayName := strings.TrimSpace(updateData.DisplayName) | ||
| if newDisplayName != "" { | ||
| var existing models.User | ||
| err := db.MongoDatabase.Collection("users").FindOne(dbCtx, bson.M{"displayName": newDisplayName}).Decode(&existing) | ||
| if err == nil && existing.Email != email { | ||
| ctx.JSON(http.StatusConflict, gin.H{"error": "Display name already taken"}) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
Non-ErrNoDocuments errors from the pre-check are silently swallowed.
If FindOne returns a transient DB error, execution falls through to the UpdateOne instead of returning 500. Mirror the pattern used in auth.go (SignUp / GoogleLogin) so the handler behaves consistently:
Proposed fix
if newDisplayName != "" {
var existing models.User
err := db.MongoDatabase.Collection("users").FindOne(dbCtx, bson.M{"displayName": newDisplayName}).Decode(&existing)
- if err == nil && existing.Email != email {
- ctx.JSON(http.StatusConflict, gin.H{"error": "Display name already taken"})
- return
+ if err == nil {
+ if existing.Email != email {
+ ctx.JSON(http.StatusConflict, gin.H{"error": "Display name already taken"})
+ return
+ }
+ } else if !errors.Is(err, mongo.ErrNoDocuments) {
+ ctx.JSON(http.StatusInternalServerError, gin.H{"error": "Database error"})
+ return
}
}The unique index will still protect correctness, but the current control flow makes a DB outage indistinguishable from a successful "not taken" check.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check displayName uniqueness | |
| newDisplayName := strings.TrimSpace(updateData.DisplayName) | |
| if newDisplayName != "" { | |
| var existing models.User | |
| err := db.MongoDatabase.Collection("users").FindOne(dbCtx, bson.M{"displayName": newDisplayName}).Decode(&existing) | |
| if err == nil && existing.Email != email { | |
| ctx.JSON(http.StatusConflict, gin.H{"error": "Display name already taken"}) | |
| return | |
| } | |
| } | |
| // Check displayName uniqueness | |
| newDisplayName := strings.TrimSpace(updateData.DisplayName) | |
| if newDisplayName != "" { | |
| var existing models.User | |
| err := db.MongoDatabase.Collection("users").FindOne(dbCtx, bson.M{"displayName": newDisplayName}).Decode(&existing) | |
| if err == nil { | |
| if existing.Email != email { | |
| ctx.JSON(http.StatusConflict, gin.H{"error": "Display name already taken"}) | |
| return | |
| } | |
| } else if !errors.Is(err, mongo.ErrNoDocuments) { | |
| ctx.JSON(http.StatusInternalServerError, gin.H{"error": "Database error"}) | |
| return | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/controllers/profile_controller.go` around lines 316 - 325, The
pre-check using db.MongoDatabase.Collection("users").FindOne when validating
newDisplayName currently swallows non-ErrNoDocuments errors; change the logic in
the display name uniqueness block (the newDisplayName/FindOne/decode/existing
flow) to explicitly handle mongo.ErrNoDocuments as the only acceptable “not
found” case and return an internal server error (HTTP 500) for any other err
returned by FindOne (and log the DB error), following the pattern used in
auth.go (SignUp/GoogleLogin); keep the existing behavior of returning HTTP 409
when err == nil and existing.Email != email.
| func EnsureIndexes() error { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
| defer cancel() | ||
|
|
||
| usersCol := MongoDatabase.Collection("users") | ||
|
|
||
| indexModel := mongo.IndexModel{ | ||
| Keys: bson.D{{Key: "displayName", Value: 1}}, | ||
| Options: options.Index().SetUnique(true).SetSparse(true).SetName("unique_displayName"), | ||
| } | ||
|
|
||
| _, err := usersCol.Indexes().CreateOne(ctx, indexModel) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create displayName index: %w", err) | ||
| } | ||
| log.Println("DisplayName unique index ensured") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether any users currently have empty or missing displayName that would collide under the sparse+unique index.
rg -nP --type=go 'DisplayName\s+string\s+`bson:"displayName[^`]*"'
rg -nP --type=go -C2 'ExtractNameFromEmail'Repository: AOSSIE-Org/DebateAI
Length of output: 2573
🏁 Script executed:
# 1. Get the full User struct definition
cat -n backend/models/user.go | head -50
# 2. Check how displayName is assigned during user creation
rg -A10 "DisplayName\s*=" backend/controllers/auth.go | head -40
# 3. Check main.go for how EnsureIndexes is called and error handling
rg -B5 -A5 "EnsureIndexes" backend/main.goRepository: AOSSIE-Org/DebateAI
Length of output: 2233
🏁 Script executed:
# Find main.go or entry point
find . -name "main.go" -type f
# Check how displayName is set during user creation
rg -B3 -A3 "DisplayName.*=" backend/controllers/auth.go | head -50
# Look for where EnsureIndexes is called
rg "EnsureIndexes" . --type goRepository: AOSSIE-Org/DebateAI
Length of output: 572
🏁 Script executed:
# Check error handling of EnsureIndexes in server main.go
rg -B5 -A5 "EnsureIndexes" ./backend/cmd/server/main.go
# Check complete user creation flow to see if displayName can stay empty
rg -B10 -A10 "DisplayName.*=" ./backend/controllers/auth.go | head -80Repository: AOSSIE-Org/DebateAI
Length of output: 1231
🏁 Script executed:
# Check the complete user creation/insertion code
rg -A30 "hashedPassword.*bcrypt" ./backend/controllers/auth.go
# Check if displayName is explicitly set or required before insertion
rg -B5 -A15 "InsertOne.*user" ./backend/controllers/auth.goRepository: AOSSIE-Org/DebateAI
Length of output: 1845
🏁 Script executed:
# Check for migrations or db initialization scripts
find . -name "*migrat*" -o -name "*seed*" | grep -v node_modules
# Check for other places where users are created or displayName is modified
rg "displayName.*:" . --type go | grep -v test | head -30
# Verify ExtractNameFromEmail never returns empty
cat -n ./backend/utils/auth.go | sed -n '43,50p'Repository: AOSSIE-Org/DebateAI
Length of output: 2960
🏁 Script executed:
# Check profile controller for displayName update validation
rg -B5 -A15 "displayName.*newDisplayName" ./backend/controllers/profile_controller.go
# Check if there's any validation on displayName input
rg "newDisplayName" ./backend/controllers/profile_controller.go -B10 -A5Repository: AOSSIE-Org/DebateAI
Length of output: 2171
Sparse index won't prevent duplicate empty displayNames; startup crash risk on existing duplicates.
Two confirmed issues with the index strategy:
-
Sparse index does not handle empty strings. The
SetSparse(true)option only excludes documents where thedisplayNamefield is missing, not where it's an empty string"". However, theUsermodel storesDisplayName stringwithbson:"displayName"(noomitempty), and the profile update endpoint allows users to setdisplayNameto an empty string after trimming:newDisplayName := strings.TrimSpace(updateData.DisplayName) // ... "displayName": newDisplayName // can be ""
Two users with empty
displayNamewill violate the unique constraint, either during this initialCreateOnecall (if legacy data exists) or during subsequent updates. -
Server crashes on startup if any duplicates exist. The
EnsureIndexes()call useslog.Fatalfon failure inmain.go, which terminates the server. Any pre-existing or corrupted duplicatedisplayNamevalues (including empty strings) will cause a hard startup failure, making deployments fragile.
Suggested mitigations:
- Use a
PartialFilterExpressionto only enforce uniqueness on non-empty values:Options: options.Index(). SetUnique(true). SetName("unique_displayName"). SetPartialFilterExpression(bson.M{ "displayName": bson.M{"$exists": true, "$type": "string", "$ne": ""}, }),
- Change index creation to log and continue rather than fatal, allowing manual remediation of conflicting documents before re-deployment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/db/db.go` around lines 58 - 75, EnsureIndexes currently creates a
unique sparse index on users.displayName which doesn't prevent duplicates of
empty strings and can crash startup if duplicates exist; update the index
options in the EnsureIndexes function (the indexModel used with
MongoDatabase.Collection("users") / usersCol.Indexes().CreateOne) to use
SetPartialFilterExpression that enforces uniqueness only for non-empty string
displayName values (e.g. filter where displayName exists, is a string, and $ne
""), and change the startup error handling where EnsureIndexes is invoked (avoid
log.Fatalf on error in main.go) to log the failure and continue so operators can
remediate duplicates without bringing the server down.
| onChange={(e) => { | ||
| const val = e.target.value; | ||
| setDashboard({ ...dashboard, profile: { ...profile, displayName: val } }); | ||
| if (!val.trim()) { | ||
| setUsernameStatus("idle"); | ||
| return; | ||
| } | ||
| if (debounceTimer.current) clearTimeout(debounceTimer.current); | ||
| setUsernameStatus("checking"); | ||
| debounceTimer.current = setTimeout(async () => { | ||
| try { | ||
| const token = getAuthToken(); | ||
| if (!token) return; | ||
| const res = await checkDisplayNameAvailability(token, val.trim()); | ||
| setUsernameStatus(res.available ? "available" : "taken"); | ||
| } catch { | ||
| setUsernameStatus("idle"); | ||
| } | ||
| }, 300); | ||
| }} |
There was a problem hiding this comment.
Debounce timer is never cleared on unmount; can cause state updates on unmounted component and stale results.
debounceTimer.current is set inside onChange but there is no useEffect cleanup that clears it when the component unmounts. If the user navigates away during the 300ms window, checkDisplayNameAvailability will still fire and setUsernameStatus will be called on an unmounted component. There's also no request-cancellation (e.g., AbortController), so overlapping edits can resolve out of order and overwrite each other's results.
🔧 Suggested fix
+useEffect(() => {
+ return () => {
+ if (debounceTimer.current) clearTimeout(debounceTimer.current);
+ };
+}, []);Consider also tracking the latest query (e.g., compare the typed value to a ref) before calling setUsernameStatus, or use AbortController in checkDisplayNameAvailability to drop stale responses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/Pages/Profile.tsx` around lines 706 - 725, The onChange debounce
uses debounceTimer.current but never clears it on unmount and doesn't guard
against stale/Out‑of‑order responses; add a useEffect cleanup that clears
debounceTimer.current on unmount, and change the async debounce handler to use
an AbortController (passed into checkDisplayNameAvailability) or maintain a
latestQuery ref (e.g., latestDisplayNameRef) to verify the response matches the
most recent val before calling setUsernameStatus; update calls to getAuthToken,
checkDisplayNameAvailability, and setUsernameStatus accordingly and ensure any
thrown abort is handled so state is not set for cancelled/stale requests.
1 participant
Addressed Issues:
Fixes : #382
Enforces uniqueness of displayName (which serves as the username in DebateAI)
across all three layers: database index, backend validation, and frontend UI.
Changes
Backend
db/db.go — Added unique sparse MongoDB index on displayName field
controllers/auth.go — Added duplicate displayName check during signup
(both email/password and Google OAuth flows)
controllers/profile_controller.go — Added duplicate displayName check
during profile update; added CheckDisplayName availability endpoint;
removed separate username field (it was redundant with displayName)
routes/profile.go — Added CheckDisplayNameRouteHandler
cmd/server/main.go — Registered /user/check-displayname route
models/user.go — Removed redundant Username field
Frontend
services/profileService.ts — Added checkDisplayNameAvailability function;
removed username param from updateProfile (now uses displayName only)
Pages/Profile.tsx — Display name edit field now shows real-time availability
check; profile data refetches after successful save so UI updates immediately
without reload
Testing
Verified two users cannot sign up with the same display name
Verified updating display name to an existing name returns 409
Verified UI shows "Display name already taken" feedback in real time
Verified profile updates immediately after save without page reload
Screenshots/Recordings:
##BEFORE:
##AFTER:
AI Usage Disclosure:
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
##Checklist