Add comments and doc strings throughout the code #12
Conversation
WalkthroughDocumentation enhancements and structural improvements across bot, handlers, middleware, and database layers. Key updates include expanded docstrings, new message helper methods, refactored authorization logic checking both allowed chats and superadmin status, improved error handling, and database initialization with a new public DB variable and sequential migration logging. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Mixed complexity across documentation improvements (repetitive, lower effort) and behavioral logic changes (authorization refactor, new helpers, transaction-based logging) distributed across six files. Changes are localized and clear in scope, but require understanding middleware authorization flow, repository transaction semantics, and initialization sequences. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 (3)
internal/bot/middleware.go (1)
84-93: Fix invalid access of CallbackQuery message (compile error/NPE).
update.CallbackQuery.Message.Messageis not a valid field; useupdate.CallbackQuery.Message.-} else if update.CallbackQuery != nil { - user = update.CallbackQuery.From.Username +} else if update.CallbackQuery != nil { + user = update.CallbackQuery.From.Username if user == "" { user = update.CallbackQuery.From.FirstName } userID = update.CallbackQuery.From.ID - if update.CallbackQuery.Message.Message != nil { - chatID = update.CallbackQuery.Message.Message.Chat.ID - messageThreadID = update.CallbackQuery.Message.Message.MessageThreadID - } + if update.CallbackQuery.Message != nil { + chatID = update.CallbackQuery.Message.Chat.ID + messageThreadID = update.CallbackQuery.Message.MessageThreadID + }internal/bot/handlers.go (1)
110-140: Fix loop: cannot range over int.
for i := range maxTorrentswon’t compile. Use an indexed loop or slice range.- for i := range maxTorrents { - t := torrents[i] + for i := 0; i < maxTorrents; i++ { + t := torrents[i]Alternatively:
- for i := range maxTorrents { + for i := range torrents[:maxTorrents] {internal/bot/bot.go (1)
161-166: Fix invalid CallbackQuery message access (compile error/NPE).Use
update.CallbackQuery.Message, not.Message.Message.- if update.CallbackQuery.Message.Message != nil { - chatID = update.CallbackQuery.Message.Message.Chat.ID - if update.CallbackQuery.Message.Message.MessageThreadID != 0 { - messageThreadID = update.CallbackQuery.Message.Message.MessageThreadID - } - } + if update.CallbackQuery.Message != nil { + chatID = update.CallbackQuery.Message.Chat.ID + if update.CallbackQuery.Message.MessageThreadID != 0 { + messageThreadID = update.CallbackQuery.Message.MessageThreadID + } + }
🧹 Nitpick comments (11)
internal/bot/middleware.go (1)
57-62: Make rate limiter cancellable (accept ctx).Using context.Background() prevents shutdown/timeout propagation. Thread can block unnecessarily.
Apply in this file:
-func (m *Middleware) WaitForRateLimit() error { - ctx := context.Background() - if err := m.limiter.Wait(ctx); err != nil { +func (m *Middleware) WaitForRateLimit(ctx context.Context) error { + if err := m.limiter.Wait(ctx); err != nil { return fmt.Errorf("rate limit error: %w", err) } return nil }Update callers (examples):
- if err := b.middleware.WaitForRateLimit(); err != nil { + if err := b.middleware.WaitForRateLimit(ctx); err != nil {(in sendMessage, sendHTMLMessage, withAuth).
internal/bot/handlers.go (2)
165-166: Usage messages: enable HTML parsing and escape placeholders.These strings include HTML tags but are sent with plain text. Switch to
sendHTMLMessageand escape angle-bracket placeholders.- b.sendMessage(ctx, chatID, messageThreadID, "<b>Usage:</b> /add <magnet_link>") + b.sendHTMLMessage(ctx, chatID, messageThreadID, "<b>Usage:</b> <code>/add <magnet_link></code>") - b.sendMessage(ctx, chatID, messageThreadID, "<b>Usage:</b> /info <torrent_id>") + b.sendHTMLMessage(ctx, chatID, messageThreadID, "<b>Usage:</b> <code>/info <torrent_id></code>") - b.sendMessage(ctx, chatID, messageThreadID, "<b>Usage:</b> /delete <torrent_id>") + b.sendHTMLMessage(ctx, chatID, messageThreadID, "<b>Usage:</b> <code>/delete <torrent_id></code>") - b.sendMessage(ctx, chatID, messageThreadID, "<b>Usage:</b> /unrestrict <link>") + b.sendHTMLMessage(ctx, chatID, messageThreadID, "<b>Usage:</b> <code>/unrestrict <link></code>") - b.sendMessage(ctx, chatID, messageThreadID, "<b>Usage:</b> /removelink <download_id>") + b.sendHTMLMessage(ctx, chatID, messageThreadID, "<b>Usage:</b> <code>/removelink <download_id></code>")Also applies to: 221-223, 306-308, 343-345, 460-462
622-624: Propagate ctx to rate limiter (after API change).Pass ctx to
WaitForRateLimitso sends are cancelable on shutdown/timeouts.- if err := b.middleware.WaitForRateLimit(); err != nil { + if err := b.middleware.WaitForRateLimit(ctx); err != nil {Also applies to: 640-642
internal/db/models.go (1)
80-81: Doc mismatch: progress scale.Handlers format progress as a percentage (0–100). Comment says 0.0–1.0.
- Progress float64 // Progress of the torrent operation (0.0 to 1.0) + Progress float64 // Progress of the torrent operation (0–100 percent as returned by API)internal/bot/bot.go (1)
296-303: Handle read/JSON errors in IP tests.Avoid ignoring io.ReadAll/Unmarshal errors; return wrapped errors for diagnosability.
- body, _ := io.ReadAll(resp.Body) - if err := json.Unmarshal(body, &ipResponse); err != nil { + body, err := io.ReadAll(resp.Body) + if err != nil { + log.Printf("Warning: Failed reading primary IP response: %v", err) + } else if err := json.Unmarshal(body, &ipResponse); err != nil { log.Printf("Warning: Failed to parse primary IP test response: %v", err) } else { primaryIP = ipResponse.IP log.Printf("Primary IP detected: %s", primaryIP) }- verifyBody, _ := io.ReadAll(verifyResp.Body) - if err := json.Unmarshal(verifyBody, &verifyIpResponse); err != nil { + verifyBody, err := io.ReadAll(verifyResp.Body) + if err != nil { + return fmt.Errorf("failed to read IP verification test response: %w", err) + } + if err := json.Unmarshal(verifyBody, &verifyIpResponse); err != nil { return fmt.Errorf("failed to parse IP verification test response: %w", err) }Also applies to: 318-321
internal/db/repository.go (1)
80-101: Accept context for DB calls.Repository methods don’t accept
context.Context, which limits cancellation/timeouts for slow DBs.Example signature and usage:
- func (r *ActivityRepository) LogActivity(... ) error { + func (r *ActivityRepository) LogActivity(ctx context.Context, ... ) error { - return r.db.Create(&activity).Error + return r.db.WithContext(ctx).Create(&activity).Error }Apply similarly to
LogTorrentActivity,LogDownloadActivity, andLogCommand.Also applies to: 118-151, 188-215, 233-261
internal/db/db.go (5)
18-22: Guard the global DB lifecycle: make Init concurrency-safe and Close idempotent; nil the handle after close.Avoid races/double inits and ensure repeated Close calls don’t error or leave a dangling handle.
Apply:
var DB *gorm.DB +// initMu serializes Init/Close to avoid races. +var initMu sync.Mutexfunc Init(dsn string) (*gorm.DB, error) { + initMu.Lock() + defer initMu.Unlock() + if DB != nil { + return DB, nil + } // Configure custom logger for GORMfunc Close() error { - if DB != nil { - sqlDB, err := DB.DB() - if err != nil { - return err - } - return sqlDB.Close() - } - return nil + initMu.Lock() + defer initMu.Unlock() + if DB == nil { + return nil + } + sqlDB, err := DB.DB() + if err != nil { + return fmt.Errorf("get underlying db: %w", err) + } + if err := sqlDB.Close(); err != nil { + return fmt.Errorf("close database: %w", err) + } + DB = nil + return nil }Add imports:
// add to imports "sync"Run-time check: ensure only one Init is used across the app; see verification script below.
Also applies to: 61-65, 111-119, 14-16
45-49: Fail fast: ping the database after opening the pool.Prevents lazy connection surprises and surfaces bad DSNs/network early.
Apply:
sqlDB, err := db.DB() if err != nil { return nil, fmt.Errorf("failed to get database instance: %w", err) } +// Verify connectivity with a short timeout +ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) +defer cancel() +if err := sqlDB.PingContext(ctx); err != nil { + return nil, fmt.Errorf("database ping failed: %w", err) +}Add imports:
// add to imports "context"
52-55: Pool tuning: add IdleTime and consider driving values from config.Database.Adds better control over idle connections and avoids stale conns; align with
internal/config/config.go’sDatabaseConfig.Apply:
sqlDB.SetMaxIdleConns(10) // Maximum number of connections in the idle connection pool sqlDB.SetMaxOpenConns(100) // Maximum number of open connections to the database sqlDB.SetConnMaxLifetime(time.Hour) // Maximum amount of time a connection may be reused +sqlDB.SetConnMaxIdleTime(30 * time.Minute) // Optional: close idle conns proactivelyRecommendation: read these from config (min/max open/idle, lifetime/idle time) for environment-specific tuning.
67-104: Optional: wrap migrations in a transaction and apply a timeout.Reduces partial-state risk and hangs during deploys. Postgres supports transactional DDL (avoid CONCURRENTLY).
Example:
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() return db.WithContext(ctx).Transaction(func(tx *gorm.DB) error { if err := tx.AutoMigrate(&User{}); err != nil { return fmt.Errorf("migrate users: %w", err) } if err := tx.AutoMigrate(&ActivityLog{}); err != nil { return fmt.Errorf("migrate activity_logs: %w", err) } if err := tx.AutoMigrate(&TorrentActivity{}); err != nil { return fmt.Errorf("migrate torrent_activities: %w", err) } if err := tx.AutoMigrate(&CommandLog{}); err != nil { return fmt.Errorf("migrate command_logs: %w", err) } if err := tx.AutoMigrate(&DownloadActivity{}); err != nil { return fmt.Errorf("migrate download_activities: %w", err) } return nil })Confirm you’re not using
CONCURRENTLYindexes in AutoMigrate; those aren’t allowed inside a transaction in Postgres.
23-33: Minor: logger writer prefix.Using “\r\n” as the log prefix can add blank lines/CRLF in some environments. Consider a clearer prefix (e.g., "gorm: ") with no newlines.
Apply:
- newLogger := logger.New( - log.New(os.Stdout, "\r\n", log.LstdFlags), + newLogger := logger.New( + log.New(os.Stdout, "gorm: ", log.LstdFlags),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/bot/bot.go(8 hunks)internal/bot/handlers.go(17 hunks)internal/bot/middleware.go(5 hunks)internal/db/db.go(2 hunks)internal/db/models.go(1 hunks)internal/db/repository.go(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
internal/db/repository.go (1)
internal/db/models.go (2)
User(11-25)User(136-138)
internal/bot/handlers.go (1)
internal/db/models.go (2)
ActivityTypeCommandStart(39-39)ActivityTypeCommandHelp(40-40)
internal/db/db.go (1)
internal/config/config.go (1)
Config(13-18)
internal/bot/bot.go (5)
internal/realdebrid/client.go (2)
Client(15-19)NewClient(36-57)internal/bot/middleware.go (2)
Middleware(16-19)NewMiddleware(23-31)internal/config/config.go (1)
Config(13-18)internal/db/db.go (2)
DB(16-16)Init(22-65)internal/db/repository.go (5)
UserRepository(14-16)ActivityRepository(66-68)TorrentRepository(104-106)DownloadRepository(174-176)CommandRepository(219-221)
internal/bot/middleware.go (1)
internal/config/config.go (1)
Config(13-18)
🔇 Additional comments (4)
internal/db/repository.go (1)
233-261: Good transactional logging.Atomic insert + counter increment via
TransactionandUpdateColumnlooks solid.If you expect high write volume, consider adding an index on
command_logs(user_id, created_at)for stats queries. Do you want a quick migration snippet?internal/db/db.go (3)
37-39: Good: force UTC timestamps.Ensures consistent CreatedAt/UpdatedAt semantics across services.
18-22: Docs/readability LGTM.Expanded comments clarify behavior and lifecycle; helpful for operators and contributors.
18-22: All verification checks passed — single initialization path confirmed with proper shutdown cleanup.The codebase correctly implements the expected patterns:
- Single initialization path: Only one
db.Init()call atinternal/bot/bot.go:79- Package-level DB variable is set within
Init()atinternal/db/db.go:62and68Close()is properly called on shutdown viaBot.Stop()atinternal/bot/bot.go:134The implementation uses both the returned value (stored in
Bot.db) and the package-level variable, which is a sound pattern for accessibility throughout the application.
| SlowThreshold: 200 * time.Millisecond, // Log queries slower than this threshold | ||
| LogLevel: logger.Info, // Log level (Info, Warn, Error, Silent) | ||
| IgnoreRecordNotFoundError: true, // Ignore "record not found" errors | ||
| Colorful: true, // Enable colored output | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Reduce SQL data leakage; enable parameterized logging and avoid noisy colorized output in prod.
At Info level, GORM will print query parameters (potential PII/secrets). Enable parameterized logging and consider disabling ANSI colors for non-TTY logs; optionally lower the level in prod.
Apply:
- logger.Config{
+ logger.Config{
SlowThreshold: 200 * time.Millisecond, // Log queries slower than this threshold
- LogLevel: logger.Info, // Log level (Info, Warn, Error, Silent)
+ LogLevel: logger.Info, // Consider lowering in production
IgnoreRecordNotFoundError: true, // Ignore "record not found" errors
- Colorful: true, // Enable colored output
+ Colorful: false, // Disable color in non‑TTY logs
+ Parameterized: true, // Do not log SQL variables (privacy)
- },
+ },Please confirm your gorm.io/gorm version supports logger.Config.Parameterized (v1.25+).
I need to verify several technical claims in the review comment. Let me gather information about GORM's logger configuration and the codebase's current setup.
🏁 Script executed:
#!/bin/bash
# Check GORM version in go.mod
if [ -f go.mod ]; then
echo "=== GORM dependency version ==="
grep -i "gorm" go.mod | head -20
fiLength of output: 85
🌐 Web query:
GORM logger.Config Parameterized field v1.25 documentation
💡 Result:
The logger.Config field is ParameterizedQueries (type bool). It’s used when creating a logger (example: ParameterizedQueries: true). See GORM logger docs and Config docs for v2 usage. [1][2]
Sources:
[1] https://gorm.io/docs/logger.html
[2] https://gorm.io/docs/gorm_config.html
🌐 Web query:
GORM Info log level query parameters logging behavior
💡 Result:
Short answer
- At logger.Info level GORM logs each SQL statement (with timing/rows). Whether the bound parameters are included in that logged SQL is controlled by the logger configuration (logger.Config). See GORM docs. [1]
Key details
- Log levels: Silent, Error, Warn, Info — Info is the most verbose and will emit SQL traces. [1]
- Parameter output: logger.Config has ParameterizedQueries (and ParamsFilter) to control printing/masking of parameters. If you enable printing of parameterized queries the logger will substitute parameters into the SQL (or print them alongside) in the Trace output; you can also implement ParamsFilter to mask/remove sensitive values. Many adapter loggers (zap/zerolog/slog adapters) provide options to turn parameter logging on/off. [1][2][3]
- Defaults/notes: parameter logging is treated as optional for security/verbosity; third‑party adapters and recent GORM fixes have added explicit support for the ParameterizedQueries config. Check your logger.Config and adapter docs to know whether parameters are printed in your setup. [1][4]
Useful links
- GORM logger docs (explains LogMode, Trace, ParameterizedQueries and ParamsFilter). [1]
- Examples / adapters showing parameter logging options (zerolog/gorm, zapgorm2, etc.). [2][3]
- Recent GORM release note referencing ParameterizedQueries support in loggers. [4]
References
[1] https://gorm.io/docs/logger.html
[2] https://pkg.go.dev/github.com/skynet2/zerolog-gorm
[3] https://github.com/moul/zapgorm2
[4] https://newreleases.io/project/github/go-gorm/gorm/release/v1.30.4
I found a critical discrepancy. The web search reveals the correct field name is ParameterizedQueries, not Parameterized as suggested in the review diff. Let me verify the current code state and confirm the exact behavior.
🏁 Script executed:
#!/bin/bash
# Inspect the actual logger.Config in internal/db/db.go
echo "=== Current logger.Config section ==="
sed -n '20,35p' internal/db/db.goLength of output: 878
🏁 Script executed:
#!/bin/bash
# Check if ParameterizedQueries is used anywhere in codebase
echo "=== Search for ParameterizedQueries or Parameterized in codebase ==="
rg -n "ParameterizedQueries|Parameterized" --type goLength of output: 68
🌐 Web query:
GORM v1.31.0 ParameterizedQueries logger config what does it do
💡 Result:
Short answer: ParameterizedQueries controls whether GORM's default logger logs the SQL with actual parameter values substituted or logs the parameterized SQL (placeholders) only. When ParameterizedQueries = true the logger avoids printing raw parameter values (it logs the parameterized query form); when false it will render/format the SQL with the parameters inserted. [1][2]
Sources:
- GORM logger docs (lists ParameterizedQueries option). [1]
- Logging summary / docs reference describing that ParameterizedQueries hides query parameters. [2]
Correct the field name in the logger config diff: use ParameterizedQueries not Parameterized.
Your codebase uses GORM v1.31.0, which supports the ParameterizedQueries field. However, the proposed diff uses the incorrect field name Parameterized, which does not exist in GORM and will not compile. The correct approach is to set ParameterizedQueries: true to prevent raw SQL parameter values from being logged at Info level (avoiding PII/secrets leakage). Additionally, consider disabling Colorful for production logs that may not have TTY output.
Apply:
logger.Config{
SlowThreshold: 200 * time.Millisecond, // Log queries slower than this threshold
- LogLevel: logger.Info, // Log level (Info, Warn, Error, Silent)
+ LogLevel: logger.Info, // Consider lowering in production
IgnoreRecordNotFoundError: true, // Ignore "record not found" errors
- Colorful: true, // Enable colored output
+ Colorful: false, // Disable color in non-TTY logs
+ ParameterizedQueries: true, // Hide SQL parameter values (privacy)
},📝 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.
| SlowThreshold: 200 * time.Millisecond, // Log queries slower than this threshold | |
| LogLevel: logger.Info, // Log level (Info, Warn, Error, Silent) | |
| IgnoreRecordNotFoundError: true, // Ignore "record not found" errors | |
| Colorful: true, // Enable colored output | |
| }, | |
| SlowThreshold: 200 * time.Millisecond, // Log queries slower than this threshold | |
| LogLevel: logger.Info, // Consider lowering in production | |
| IgnoreRecordNotFoundError: true, // Ignore "record not found" errors | |
| Colorful: false, // Disable color in non-TTY logs | |
| ParameterizedQueries: true, // Hide SQL parameter values (privacy) | |
| }, |
🤖 Prompt for AI Agents
In internal/db/db.go around lines 27 to 31, replace the incorrect logger config
field name `Parameterized` with the correct GORM v1.31.0 field
`ParameterizedQueries` and set it to true (i.e., ParameterizedQueries: true) so
SQL parameter values are not logged; also consider setting Colorful to false for
production/TTY-less environments (Colorful: false).
Summary by CodeRabbit
Release Notes
Bug Fixes & Improvements
Chores