feat: update Gemini API client, introduce comprehensive configuration management, and refine backend logic #127
Conversation
… management, and refine backend logic
WalkthroughConsolidates configuration handling (dynamic CONFIG_PATH, example YAML), migrates Gemini client and related deps, expands Comment → ModeratedComment model, adjusts several controllers and WebSocket broadcast logic, adds Python requirements and SSL bypass, and updates Redis/SMTP config usage and logging. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)backend/cmd/server/main.go (4)
🔇 Additional comments (6)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
backend/controllers/team_controller.go (2)
347-375: Race condition: Non-atomic capacity check and member addition.The flow fetches the team (line 348), checks capacity (line 359), then updates with
$push(line 375). If two users join simultaneously, both could pass the capacity check before either update completes, allowing the team to exceedmaxSize.Apply this approach to enforce capacity atomically:
- // First, get current team to calculate new average - var team models.Team - err = collection.FindOne(context.Background(), bson.M{"_id": objectID}).Decode(&team) - if err != nil { - c.JSON(http.StatusNotFound, gin.H{"error": "Team not found"}) - return - } - - capacity := team.MaxSize - if capacity <= 0 { - capacity = 4 - } - - if len(team.Members) >= capacity { - c.JSON(http.StatusBadRequest, gin.H{"error": "Team is already full"}) - return - } - - // Calculate new average Elo - totalElo := 0.0 - for _, member := range team.Members { - totalElo += member.Elo - } - - totalElo += newMember.Elo - newAverageElo := totalElo / float64(len(team.Members)+1) - - update["$set"].(bson.M)["averageElo"] = newAverageElo - - _, err = collection.UpdateOne(context.Background(), bson.M{"_id": objectID}, update) + // Use FindOneAndUpdate with size constraint to prevent race condition + filter := bson.M{ + "_id": objectID, + "$expr": bson.M{ + "$lt": bson.A{ + bson.M{"$size": "$members"}, + bson.M{"$ifNull": bson.A{"$maxSize", 4}}, + }, + }, + } + + // Calculate new average using aggregation pipeline update + updatePipeline := mongo.Pipeline{ + {{Key: "$set", Value: bson.M{ + "members": bson.M{"$concatArrays": bson.A{"$members", bson.A{newMember}}}, + "updatedAt": time.Now(), + }}}, + {{Key: "$set", Value: bson.M{ + "averageElo": bson.M{"$avg": "$members.elo"}, + }}}, + } + + result, err := collection.UpdateOne(context.Background(), filter, updatePipeline) if err != nil { c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to join team"}) return } + if result.MatchedCount == 0 { + c.JSON(http.StatusBadRequest, gin.H{"error": "Team not found or already full"}) + return + }
147-177: Potential race condition when reducing team size.The function fetches the team (line 147), validates that
len(team.Members) <= maxSize(line 160), then updatesmaxSize(line 173). If a member joins between the validation and update, the team could end up with more members than the newmaxSizeallows.Consider adding a constraint to the update filter:
// Update team size update := bson.M{ "$set": bson.M{ "maxSize": updateData.MaxSize, "updatedAt": time.Now(), }, } - _, err = collection.UpdateOne(context.Background(), bson.M{"_id": objectID}, update) + filter := bson.M{ + "_id": objectID, + "$expr": bson.M{ + "$lte": bson.A{ + bson.M{"$size": "$members"}, + updateData.MaxSize, + }, + }, + } + result, err := collection.UpdateOne(context.Background(), filter, update) if err != nil { c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to update team size"}) return } + if result.MatchedCount == 0 { + c.JSON(http.StatusBadRequest, gin.H{"error": "Cannot update: team size increased since validation. Please try again."}) + return + }backend/config/config.go (1)
40-43: Missingyamlstruct tag on JWT configuration.The
JWTstruct is missing theyaml:"jwt"tag, which may cause YAML unmarshaling to fail if the config file uses lowercasejwtas the key.JWT struct { Secret string `yaml:"secret"` Expiry int `yaml:"expiry"` - } + } `yaml:"jwt"`backend/controllers/auth.go (1)
104-107: Silent error swallowing may hide issues.The error from
persistUserStatsis being silently ignored. While the operation may be non-critical, consider at least logging the error for debugging purposes.if normalizeUserStats(&existingUser) { if err := persistUserStats(dbCtx, &existingUser); err != nil { + log.Printf("Failed to persist normalized user stats for %s: %v", existingUser.ID.Hex(), err) } }backend/cmd/server/main.go (1)
116-119: AuthMiddleware still uses a hard‑coded prod config pathInside the protected routes group you still have:
auth := router.Group("/") auth.Use(middlewares.AuthMiddleware("./config/config.prod.yml"))This bypasses the new dynamic
configPathand forces the auth middleware to read./config/config.prod.yml, which can:
- Break local/dev setups that only configure
config.dev.yml.- Cause JWT secrets and other auth‑related settings to diverge from the rest of the process, leading to confusing behaviour.
You likely want the same config file the rest of the server is using:
- auth.Use(middlewares.AuthMiddleware("./config/config.prod.yml")) + auth.Use(middlewares.AuthMiddleware(configPath))That keeps all components aligned on the same configuration source.
🧹 Nitpick comments (11)
backend/db/db.go (1)
7-7: Use project‑standard logging abstraction if one existsThe success log is harmless and useful, but it directly uses the stdlib
logpackage. If the rest of the backend uses a structured/logger abstraction (e.g., zap, logrus, or a customloggerwrapper), it would be better to route this through that for consistency and centralized configuration (log level, JSON formatting, etc.). If no such abstraction exists yet, this change is fine as‑is.Also applies to: 116-116
backend/test_server.go (1)
39-41: Debug prints for pool inspection are fine for this test harnessPrinting the pool contents before and after matching is useful for manual verification and is appropriate in this
test_server.gohelper. If this file ever evolves into something used in automated tests or production-like flows, consider switching tolog.Printfor a structured logger to make the output easier to filter and correlate, but that’s optional for now.Also applies to: 49-51
backend/requirements.txt (2)
1-3: Pin dependency versions for reproducibility and security.Dependencies without explicit versions will install the latest available version at install time, creating reproducibility issues and security risks across different environments.
Apply this diff to pin dependencies to specific versions:
-fastapi -uvicorn -openai-whisper +fastapi==0.115.0 +uvicorn==0.30.1 +openai-whisper==20240930Adjust the version numbers to match your tested and validated versions. You can find the latest stable versions on PyPI.
1-3: Consider adding development dependencies.Standard Python projects include separate development dependencies (testing, linting, formatting). Consider organizing dependencies into runtime and development groups.
For example:
+# Runtime dependencies fastapi==0.115.0 uvicorn==0.30.1 openai-whisper==20240930 + +# Development dependencies (optional) +# pytest==7.4.0 +# pytest-asyncio==0.21.0 +# black==23.9.0 +# ruff==0.10.0Alternatively, you can create separate
requirements-dev.txtor usepip-toolsto manage dependencies.backend/controllers/transcript_controller.go (3)
52-52: Extraemail == ""check inSubmitTranscriptsdefensively tightens authAdding
email == ""to the invalid-token condition avoids processing a request when token validation yields a blank identity, which is a sensible defensive check for a handler that passesValidateTokenAndFetchEmailguarantees a non-empty email whenevervalidis true, this is slightly redundant but still harmless.
260-263: APP_ENV guard correctly disables debug/test handlers in productionThe new
APP_ENVcheck (case-insensitive and trimmed) in both test handlers is a good safeguard to keep these debug endpoints unavailable whenAPP_ENVis set toprodorproduction. Just ensure your production deployments reliably setAPP_ENVto one of those values; otherwise these routes remain accessible. If you expect to add more such checks, consider extracting this pattern into a small helper (e.g.,utils.IsProdEnv()) to avoid duplicating the logic.Also applies to: 313-316
409-412: Token validation cleanup inUpdatePendingTranscriptsHandlerSwitching to
valid, _, err := ...removes an unusedUpdatePendingTranscripts). If this endpoint is meant to be privileged/admin-only, you may want to extend this in the future to enforce roles/claims rather than just token validity, but that’s outside the scope of this change.backend/config/config.go (1)
5-5: Replace deprecatedio/ioutilwithospackage.
ioutil.ReadFileis deprecated since Go 1.16. Useos.ReadFileinstead.import ( "fmt" - "io/ioutil" + "os" "gopkg.in/yaml.v3" )And update the
LoadConfigfunction:func LoadConfig(path string) (*Config, error) { - data, err := ioutil.ReadFile(path) + data, err := os.ReadFile(path) if err != nil {backend/controllers/admin_controller.go (1)
352-373: ModeratedComment mapping looks correct; consider clarifying pagination semanticsThe switch to
[]models.ModeratedCommentand the field mapping fromTeamDebateMessage/TeamChatMessage→ModeratedCommentlooks consistent with the new model (ID, content, user identifiers, team/debate IDs, timestamps, deletion flag).Two things to be aware of:
totalis just the size of this in-memory slice
total := int64(len(comments))returns only the number of comments loaded for this page, not the overall count acrossteam_debate_messagesandteam_chat_messages. If the admin UI or API consumers expect a true total for pagination, you’ll likely want separateCountDocumentscalls (or an aggregation) instead oflen(comments).Pagination across two collections is approximate
Becauseskip/limitare applied independently to each collection and then results are merged, a givenpage/limitpair can yield up to2 * limitcomments and page boundaries may not be stable across collections. If you need strict global ordering and accurate pagination across both sources, this will eventually want a more centralised query strategy.If current behaviour is acceptable for your admin use case, this is fine; otherwise, consider tightening this in a follow‑up.
Also applies to: 386-396, 402-403
backend/services/gemini.go (1)
8-9: Gemini client migration is correct; review safety settings and add minor robustnessThe migration to the new
genaiclient looks good and matches the rest of the codebase:
initGeminicorrectly callsgenai.NewClient(context.Background(), option.WithAPIKey(apiKey)).generateModelTextusesgeminiClient.GenerativeModel(modelName)and assembles text from candidates’genai.Textparts before passing it throughcleanModelOutput.A couple of follow‑ups worth considering:
Safety settings fully disable harm blocking
Setting all categories toHarmBlockNoneremoves the model’s built‑in safety filters:model.SafetySettings = []*genai.SafetySetting{ {Category: genai.HarmCategoryHarassment, Threshold: genai.HarmBlockNone}, {Category: genai.HarmCategoryHateSpeech, Threshold: genai.HarmBlockNone}, {Category: genai.HarmCategorySexuallyExplicit, Threshold: genai.HarmBlockNone}, {Category: genai.HarmCategoryDangerousContent, Threshold: genai.HarmBlockNone}, }If you need any guardrails (for moderation, ToS, or brand reasons), consider using a stricter threshold (e.g.,
HarmBlockSome/HarmBlockMedium) or relying on the library defaults instead of explicitly disabling all blocking.Defensive checks around candidates
To harden against unexpected responses, you could defensively guardrespandcand:if resp == nil || len(resp.Candidates) == 0 { return "", errors.New("empty response from Gemini model") } for _, cand := range resp.Candidates { if cand == nil || cand.Content == nil { continue } // existing parts loop… }This keeps the service from panicking if the SDK ever returns a
nilcandidate slice element.Non‑text parts are dropped (optional)
Right now you only processgenai.Textparts and silently ignore others. If you later introduce tools or structured outputs, you might want to handle or log unexpected part types for debugging.Functionally, the migration is sound; the above are mainly about safety posture and resilience.
Also applies to: 14-16, 23-28, 35-45
backend/cmd/server/main.go (1)
49-52: Tighten Redis config handling and remove unreachable fallback branchThe Redis block currently reads:
if cfg.Redis.Addr != "" { redisURL := cfg.Redis.Addr if redisURL == "" { redisURL = "localhost:6379" } // ... } else { log.Println("Redis URL not configured; continuing without Redis-backed features") }Because of the outer
if cfg.Redis.Addr != "", the innerif redisURL == ""can never be true, so the"localhost:6379"fallback is effectively dead code.Depending on your intent:
If an empty/omitted
addrshould mean “no Redis” (as the log message suggests), you can simply drop the innerif:if cfg.Redis.Addr != "" {redisURL := cfg.Redis.Addrif redisURL == "" {redisURL = "localhost:6379"}
redisURL := cfg.Redis.Addr if err := debate.InitRedis(redisURL, cfg.Redis.Password, cfg.Redis.DB); err != nil { // ... }
log.Println("Redis URL not configured; continuing without Redis-backed features")
}
- If you actually want a default Redis on localhost when
addris empty, then you should remove or relax the outer guard and rely on the inner default.Either way, clarifying this will make the intended behaviour more obvious and avoid misleading dormant branches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
backend/go.sumis excluded by!**/*.sumfrontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
.gitignore(1 hunks)backend/cmd/server/main.go(4 hunks)backend/config/config.example.yml(1 hunks)backend/config/config.go(1 hunks)backend/controllers/admin_controller.go(3 hunks)backend/controllers/auth.go(5 hunks)backend/controllers/team_controller.go(1 hunks)backend/controllers/transcript_controller.go(2 hunks)backend/db/db.go(1 hunks)backend/go.mod(3 hunks)backend/models/admin.go(1 hunks)backend/requirements.txt(1 hunks)backend/services/debatevsbot.go(1 hunks)backend/services/gemini.go(2 hunks)backend/test_server.go(3 hunks)backend/transcribeService.py(1 hunks)backend/websocket/team_websocket.go(2 hunks)backend/websocket/websocket.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/controllers/admin_controller.go (1)
backend/models/admin.go (1)
ModeratedComment(22-35)
backend/cmd/server/main.go (3)
backend/config/config.go (2)
LoadConfig(60-72)Config(10-57)backend/middlewares/rbac.go (1)
InitCasbin(26-101)backend/routes/admin.go (1)
SetupAdminRoutes(11-39)
🔇 Additional comments (17)
.gitignore (1)
1-5: LGTM! Solid configuration & build artifact management.The patterns align well with the PR objectives and follow established best practices:
.DS_Storeand**/__pycache__/are standard macOS/Python conventionsbackend/config/config.dev.ymldirectly prevents accidental commits of local secrets*.pyccaptures Python bytecodeFile structure is clean and properly terminated.
Optional: Consider adding
*.pyoand*.pydalongside*.pycfor more comprehensive Python bytecode coverage across different platforms and optimizations (nice-to-have, not blocking).backend/test_server.go (1)
3-8: fmt import matches new debug outputImporting
fmthere is appropriate and required for the addedfmt.Printfcalls; no issues from a build or behavior standpoint.backend/transcribeService.py (1)
5-8: No issues found. The SSL certificate verification override code referenced in this review comment does not exist inbackend/transcribeService.py. The file contains only FastAPI and Whisper imports with a basic transcription endpoint, with no SSL context manipulation.backend/requirements.txt (1)
1-3: No action required—openai-whisper is correct for this component.The backend is written in Go, not Python. The Python
requirements.txtis for ancillary services;openai-whisperis the appropriate dependency for audio transcription in the debate platform. Gemini API configuration is managed in the Go backend configuration (seeconfig.prod.yml). The PR title correctly reflects changes to the backend Gemini integration, which is separate from this Python utility requirements.backend/controllers/team_controller.go (2)
354-362: Inconsistency with AI summary: Capacity check still present.The AI summary states that the capacity-check logic was removed, but the code clearly shows capacity checking at lines 354-362 with a fallback to 4 when
MaxSize <= 0. This logic is still enforced before adding members.
620-627: LGTM: Idiomatic use ofbson.Afor query construction.The update to use
bson.Ainstead of a raw Go slice for the$ltcomparison is idiomatic for the Go MongoDB driver and improves type safety.backend/controllers/transcript_controller.go (1)
14-15:osimport correctly supports new APP_ENV-based guardsThe added
osimport is required for the new environment checks later in this file and is appropriately scoped; no issues here.backend/config/config.go (1)
45-52: LGTM!The SMTP struct fields now have proper YAML tags, which aligns with the configuration template in
config.example.yml.backend/controllers/auth.go (1)
88-91: LGTM!The initialization of gamification fields (
Score,Badges,CurrentStreak) is properly set to default values for new users.Also applies to: 175-178
backend/websocket/websocket.go (1)
309-325: LGTM!The client initialization is complete with all required fields properly set, including
IsSpectatorwhich is correctly assigned based on the query parameter check at line 284.Note: The AI summary claims
IsSpectatorinitialization was removed, but it is still present at line 316.backend/websocket/team_websocket.go (1)
954-954: LGTM!The formatting change maintains consistency with the rest of the codebase.
backend/go.mod (2)
25-25: LGTM!The indirect dependencies for OpenTelemetry instrumentation (
otelgrpc) and Google Cloud AI are appropriate additions that support the Gemini client migration and provide observability for gRPC calls.Also applies to: 67-67, 78-79
8-13: The review comment is based on incorrect information and should not be applied.The provided code snippet does not match the actual
backend/go.modfile content. The actual file (lines 8-13) containsgithub.com/gin-contrib/cors,github.com/gin-gonic/gin,github.com/golang-jwt/jwt/v5,github.com/google/uuid,github.com/gorilla/websocket, andgo.mongodb.org/mongo-driver—not the casbin or google/generative-ai-go dependencies mentioned in the review.Additionally, the review incorrectly recommends
google/generative-ai-goas the current Gemini API client. This library is deprecated and legacy. Google's current official Go SDK for Gemini isgoogle.golang.org/genai(googleapis/go-genai), which should be used instead for new code.Likely an incorrect or invalid review comment.
backend/services/debatevsbot.go (1)
15-15: Gemini client import path update looks consistentThe new import path
github.com/google/generative-ai-go/genaimatches the client usage ingemini.goand the globalgeminiClienttype, so this migration here is mechanically correct.backend/config/config.example.yml (1)
1-37: Config template is clear and well‑scoped for developmentThe example config cleanly documents all major sections (server, database, Gemini/OpenAI, Cognito, JWT, SMTP, Google OAuth, Redis) with placeholder values and sensible local defaults. This should make onboarding and avoiding accidental secret commits much easier.
Just keep an eye on keeping key names in sync with
config.Config’s YAML tags whenever that struct evolves (e.g.,gemini.apiKey,openai.gptApiKey,googleOAuth.clientID,redis.addr/password/db) so that this file remains a drop‑in template.backend/cmd/server/main.go (1)
23-27: Dynamic CONFIG_PATH wiring is a good improvementUsing
CONFIG_PATHwith a sensible default (./config/config.dev.yml) and threadingconfigPaththrough to:
- initial
config.LoadConfig(configPath)middlewares.InitCasbin(configPath)setupRouter(cfg, configPath)→routes.SetupAdminRoutes(router, configPath)gives you a single source of truth for configuration and makes it straightforward to run against different YAML files in various environments.
One small operational enhancement you might consider later is logging the resolved
configPathat startup so it’s immediately visible which config the process is using.Also applies to: 43-47, 76-76, 84-85, 165-166
backend/models/admin.go (1)
20-35: No changes needed—code and documentation are aligned as-isThe struct in
backend/models/admin.gois namedComment(notModeratedComment), and its doc comment already correctly describes its purpose. No references need updating because there was no rename. The twoCommenttypes in different model files serve distinct moderation and transcript-comment purposes without conflict.
1 participant
This PR improves the repository structure and standardizes the configuration setup.
Changes
Configuration
backend/config/config.dev.ymltobackend/config/config.example.ymlto act as a template..gitignore Updates
config.dev.yml__pycache__/.DS_StoreDependencies
backend/requirements.txtto manage Python dependencies.Cleanup
backend/test_server.gowas reviewed.How to Test
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.