Conversation
WalkthroughAdds a Bot type with lifecycle (New, Start, Stop), centralizes date-only (UTC midnight) birthday calculations to fix off‑by‑one issues, expands HTTP handlers and templates to expose bot status, enhances storage to ensure directories/files, improves logging, and adds tests reproducing/fixing the birthday date bug. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as cmd/app (main)
participant Bot as internal/bot.Bot
participant Storage as internal/storage
participant Handler as HTTP Handler
participant Templates as internal/templates
App->>App: initBot() reads TELEGRAM_BOT_TOKEN
alt token present
App->>Bot: New(token) -- init API client, fetch bot info
App->>Bot: Start() (background run loop)
Bot->>Storage: LoadBirthdays()
Storage-->>Bot: []Birthdays
Bot->>Bot: normalize current time to UTC midnight (date-only)
Bot->>Bot: compute daysDiff / next-year adjustments
Bot->>Bot: decide notification windows (0/14/28/+next-year)
Bot->>Storage: SaveBirthdays() (update LastNotification)
Bot->>Handler: expose BotStatusProvider data
Handler->>Templates: LoadTemplates() and render PageData with BotInfo
else token missing
App-->>App: returns nil bot (no runtime bot)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/bot/bot.go (2)
658-676: Same off-by-one bug persists in next-year calculations.Line 661 uses
nowinstead ofnowDate, so next-year birthday reminders can still be off by one day at late evening hours.Apply this diff:
} else if daysDiff < 0 { // Birthday has passed this year - check next year nextYearBirthday := thisYearBirthday.AddDate(1, 0, 0) - nextYearDaysDiff := int(nextYearBirthday.Sub(now).Hours() / 24) + nextYearDaysDiff := int(nextYearBirthday.Sub(nowDate).Hours() / 24)
708-714: Same inconsistency in debug logging branch.Line 711 also uses
nowinstead ofnowDatefor the next-year calculation.Apply this diff:
if daysDiff < 0 { // Birthday has passed, show next year info nextYearBirthday := thisYearBirthday.AddDate(1, 0, 0) - nextYearDaysDiff := int(nextYearBirthday.Sub(now).Hours() / 24) + nextYearDaysDiff := int(nextYearBirthday.Sub(nowDate).Hours() / 24)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/bot/birthday_notification_test.go(1 hunks)internal/bot/bot.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
internal/bot/bot.go (1)
636-640: Good fix for the off-by-one bug.Normalizing to midnight UTC ensures accurate date-only comparisons regardless of the current time of day.
internal/bot/birthday_notification_test.go (1)
134-174: Well-documented regression test.This test clearly reproduces the exact bug scenario and documents both the buggy and fixed behavior. Good practice for preventing regressions.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Caution Docstrings generation - FAILED No docstrings were generated. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/bot/birthday_notification_test.go (2)
111-116: Remove redundant fallback logic.The fallback at lines 111-116 performs the same operation as line 109 (both use
tc.currentTimefor the year), just withfmt.Sprintfinstead of string concatenation. Since line 109 will always succeed for valid test inputs, this fallback is unreachable and adds unnecessary complexity.Apply this diff to remove the redundant fallback:
// Parse the birthday MM-DD using the test case's year thisYearBirthday, err := time.Parse("2006-01-02", tc.currentTime.Format("2006")+"-"+tc.birthdayMMDD) if err != nil { - // Fallback: try parsing with explicit year from test case - thisYearBirthday, err = time.Parse("2006-01-02", fmt.Sprintf("%d-%s", tc.currentTime.Year(), tc.birthdayMMDD)) - if err != nil { - t.Fatalf("Failed to parse birthday date: %v", err) - } + t.Fatalf("Failed to parse birthday date: %v", err) }This would also eliminate the need for the
fmtimport (line 4).
135-175: Good bug reproduction test, but consider using dynamic year for consistency.The test correctly reproduces the original bug scenario and validates the fix. However, line 146 hardcodes
"2025-"instead of deriving it fromcurrentTime, which is inconsistent with the approach inTestBirthdayDateCalculationAtDifferentTimes(line 109).Apply this diff for consistency:
// Parse the birthday for this year - thisYearBirthday, err := time.Parse("2006-01-02", "2025-"+birthdayMMDD) + thisYearBirthday, err := time.Parse("2006-01-02", currentTime.Format("2006")+"-"+birthdayMMDD) if err != nil { t.Fatalf("Failed to parse birthday date: %v", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/bot/birthday_notification_test.go(2 hunks)internal/bot/bot.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/bot/bot.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
internal/bot/birthday_notification_test.go (2)
45-104: LGTM! Comprehensive test coverage for date calculation edge cases.The test cases cover a good range of scenarios including late evening times, early morning times, and various day differences. This will effectively validate the midnight-normalization fix.
106-132: Past review issue has been resolved.The previous concern about using
time.Now()for the year has been fixed. Line 109 now correctly usestc.currentTime.Format("2006"), ensuring both the birthday andnowDatereference the test case's year.
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 (2)
cmd/app/main.go (1)
21-40: Bug: possible panic when TELEGRAM_BOT_TOKEN is unset due to typed‑nil interfaceWhen
TELEGRAM_BOT_TOKENis not set,initBotreturns(nil, nil)(Line 48–50). Inmainyou then passtelegramBotdirectly to handlers:telegramBot, err := initBot() // ... http.HandleFunc("/", handlers.IndexHandler(tpl, telegramBot)) http.HandleFunc("/bot-info", handlers.BotInfoHandler(tpl, telegramBot))
IndexHandler’s parameter type is theBotStatusProviderinterface, and it checksif botProvider != nil. IftelegramBotis a*bot.Bottyped nil, converting it toBotStatusProvideryields a non‑nil interface with a nil underlying pointer.botProvider != nilwill be true, and any method call on it (e.g.,GetNotificationHours) will likely dereference a nil*Botand panic when the token is not configured.Suggested fix: keep
initBotas‑is, but introduce aBotStatusProvidervariable and only assign it when the bot is non‑nil, then pass that to the handlers:func main() { @@ - // Initialize Telegram bot - telegramBot, err := initBot() - if err != nil { - logger.Error("MAIN", "Failed to initialize Telegram bot: %v", err) - } - - tpl := templates.LoadTemplates() - - http.HandleFunc("/", handlers.IndexHandler(tpl, telegramBot)) - http.HandleFunc("/bot-info", handlers.BotInfoHandler(tpl, telegramBot)) + // Initialize Telegram bot + telegramBot, err := initBot() + if err != nil { + logger.Error("MAIN", "Failed to initialize Telegram bot: %v", err) + } + + // Expose bot to handlers via the BotStatusProvider interface, but avoid + // wrapping a typed-nil *Bot into a non-nil interface. + var botProvider handlers.BotStatusProvider + if telegramBot != nil { + botProvider = telegramBot + } + + tpl := templates.LoadTemplates() + + http.HandleFunc("/", handlers.IndexHandler(tpl, botProvider)) + http.HandleFunc("/bot-info", handlers.BotInfoHandler(tpl, botProvider)) @@ func initBot() (*bot.Bot, error) {This keeps the “bot optional” behavior while ensuring
botProvideris trulynilwhen the bot is not configured, soIndexHandler’sif botProvider != nilguard works as intended.Also applies to: 42-60
internal/logger/logger.go (1)
34-57: Fix inconsistentDEBUG/LOG_LEVELinteraction that can drop HTTP/BOT logs entirelyWhen both
DEBUGandLOG_LEVELare set,debugEnabledcan staytruewhilelogLevelisINFO/WARN/ERROR. In that case:
LogRequest/LogBotMessagetake theif debugEnabledbranch and callDebug(...).Debugthen checksshouldLog(DEBUG)which isfalseiflogLevel > DEBUG, so nothing is logged and the non‑debugInfo(...)path is never reached.This can silently suppress all request/bot logs despite
LOG_LEVEL=INFO(or higher).Consider making
LOG_LEVELauthoritative and always aligningdebugEnabledwith it, e.g.:// Check LOG_LEVEL environment variable if level := os.Getenv("LOG_LEVEL"); level != "" { switch strings.ToUpper(level) { case "DEBUG": logLevel = DEBUG - debugEnabled = true + debugEnabled = true case "INFO": logLevel = INFO + debugEnabled = false case "WARN", "WARNING": logLevel = WARN + debugEnabled = false case "ERROR": logLevel = ERROR + debugEnabled = false } }This keeps
debugEnabledand the level threshold consistent and avoids the “no logs at all” case.Also applies to: 63-65, 130-148
♻️ Duplicate comments (1)
internal/handlers/handlers.go (1)
205-245: IndexHandler logic is sound; behavior depends on main.go fix for nil botThe handler correctly:
- Loads birthdays with centralized error handling.
- Builds
BotInfowhenbotProvideris non‑nil, including notification window, next check, and “in window” flag.- Falls back to
"not configured"when the provider is nil.- Renders
PageDatavia the"page"template and logs render errors.Once
main.gois updated to avoid wrapping a typed‑nil*Botinto a non‑nilBotStatusProvider(see my comment there), this logic will behave correctly both when the bot is configured and when it’s absent.
🧹 Nitpick comments (9)
internal/templates/funcs.go (1)
11-66: Template helpers are correct; consider minor robustness tweaksThe helpers look correct and match the unknown-year (
0000-MM-DD) convention end‑to‑end:
dict’s validation and map construction are sound for template use.formatTime/isZeroTimebehave as expected for RFC3339 + zero‑time handling.formatBirthDate/formatBirthDateForInput/isUnknownYearare consistent with how handlers normalize dates.Two optional improvements:
formatBirthDateForInput+ leap days: if you ever store0000-02-29, replacing0000with a non‑leap current year will produce an invalid HTML date (e.g.2025-02-29). You might want to pick a fixed leap year (e.g. 2000) instead oftime.Now().Year()when the month/day is Feb 29.Testability: since
formatBirthDateForInputdepends ontime.Now(), if you later need deterministic tests you may want to inject anowfunction (package‑level var) or pass the year in explicitly from the caller.internal/storage/storage.go (1)
1-46: Storage logic is correct; consider small robustness and doc tweaksFunctionally this looks good: you create an empty YAML file (
[]\n) if missing, propagate IO/parse errors, and write back with0644perms.Two minor suggestions:
Comment vs behavior on failure (Lines 21–23): the comment says “Returns an empty slice and error on failure”, but on read/unmarshal failure you currently return
nil, err. For callers this usually doesn’t matter (a nil slice is range‑able), but you may want to either:
- Update the comment to say “may return a nil slice and an error on failure”, or
- Normalize to
return []models.Birthday{}, errin the error path for strict adherence.Directory creation for custom YAML_PATH: if
YAML_PATHpoints into a non‑existent directory,os.WriteFilewill fail. Depending on your deployment assumptions, you might want toos.MkdirAll(filepath.Dir(filePath), 0o755)before the first write. This is optional and may be overkill if/datais always pre‑created by the container/host.internal/bot/birthday_notification_test.go (1)
44-162: Date-only difference tests correctly cover the off‑by‑one bugThese two tests are well structured:
- Use fixed
time.Date(..., time.UTC)values and normalizenowDateto midnight, which matches the intended production fix.- Parse birthdays with
tc.currentTime’s year, so they won’t start failing in later calendar years (addressing the earliertime.Now().Format("2006")issue).- Cover both positive, zero, and negative deltas, plus an explicit reproduction of the original Dec 15/Dec 16 bug.
As a tiny cleanup, you could consider dropping the extra
if daysDiffFixed == 0check inTestBirthdayDateCalculationBugReproductionsincedaysDiffFixed != 1already fails the test, but it does document intent so leaving it is also fine.internal/handlers/handlers.go (2)
246-285: SaveRowHandler is correct; consider stricter validation feedbackThe add/update flow looks right:
idx == -1⇒ append newBirthday.- Otherwise, validate
idxbounds and update the existing record.- Use
updateBirthdayFromFormto centralize normalization/parsing.- Persist with
storage.SaveBirthdaysand then render the"table"partial.One minor consistency nit: for out‑of‑range
idx, you already log and return400 Bad Request. InupdateBirthdayFromForm, invalidlast_notificationandchat_idvalues are silently ignored; if those fields are user-editable, you might want to either validate and 400 the request, or at least log parse failures for debugging. Not critical, but it can help trace bad client input.
287-317: DeleteRowHandler works; consider returning 400 on invalid idxDelete behavior is mostly fine: you parse
idx, load birthdays, remove the selected element if in range, save, and re-render the"table"view.For invalid indices you currently only log:
} else { logger.Error("HANDLERS", "DeleteRowHandler invalid idx: %d", idx) }but still return
200 OKwith the unchanged table. For consistency withSaveRowHandler(which returns400on invalididx), you may want to:- } else { - logger.Error("HANDLERS", "DeleteRowHandler invalid idx: %d", idx) - } + } else { + logger.Error("HANDLERS", "DeleteRowHandler invalid idx: %d", idx) + http.Error(w, "Invalid idx", http.StatusBadRequest) + return + }This makes error handling consistent across handlers and gives clients a clear signal when they send a bad index.
internal/logger/logger.go (1)
59-61: Avoid double timestamps in log output
log.SetFlags(log.LstdFlags | log.Lmicroseconds)already adds a timestamp, andformatMessagealso prepends its own timestamp. This will result in duplicated timestamps on every line.Consider either:
- Dropping the explicit timestamp from
formatMessage, or- Setting
log.SetFlags(0)and lettingformatMessagebe the single source of timestamp formatting.Either approach will make log lines cleaner and avoid confusion.
Also applies to: 67-75
internal/bot/bot.go (3)
314-446: Birthday update flow is robust; only minor potential refactorsThe
/update_birth_datehandler does solid validation (MM‑DD →0000‑MM‑DD, then strictYYYY-MM-DDparse) and handles chat naming for groups vs privates correctly. Storage and error handling paths look safe.Two optional refinements you may consider:
- Precompile the
mmddRegexanddateRegexat package scope to avoid recompiling them on every command.- Factor out the “resolve chat display name” logic into a small helper to keep this handler shorter.
No functional issues spotted here.
538-585: Date‑only birthday calculation fix looks correct and should remove the off‑by‑oneNormalizing
nowtonowDateat UTC midnight and then computing:daysDiff := int(thisYearBirthday.Sub(nowDate).Hours() / 24) nextYearDaysDiff := int(nextYearBirthday.Sub(nowDate).Hours() / 24)ensures that:
- Differences are purely date-based (no time‑of‑day skew),
- 0/14/28‑day checks behave consistently throughout the day,
- Cross‑year cases (e.g., December 27 vs January 10 next year) are handled via the
nextYearBirthdaybranch.Combined with the per‑day
LastNotificationcheck and the summary logging, this should eliminate the “day before” bug while keeping the 2‑ and 4‑week reminders intact.Only small nit:
daysDiffis no longer used insideshouldSendBirthdayNotification, so its parameter list could be simplified in the future.Also applies to: 613-770
568-585:shouldSendBirthdayNotificationcould be simplified or removedGiven that
processBirthdaysalready skips any entry whoseLastNotificationis today, the extra “was birthday notification already sent today” logic inshouldSendBirthdayNotificationis redundant, anddaysDiffis unused.You could either:
- Inline the birthday‑today duplicate check into
processBirthdays(reusing the existinglastNotificationDate == todayguard), or- Keep this helper but drop the
daysDiffparameter and update the comment to reflect that it’s just an additional guard for birthday‑today.This is purely a cleanup; behavior is already correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.claude/settings.local.json(1 hunks)cmd/app/main.go(2 hunks)internal/bot/birthday_notification_test.go(1 hunks)internal/bot/bot.go(11 hunks)internal/handlers/bot_info.go(1 hunks)internal/handlers/handlers.go(5 hunks)internal/logger/logger.go(7 hunks)internal/models/birthday.go(1 hunks)internal/storage/storage.go(3 hunks)internal/templates/funcs.go(5 hunks)internal/templates/templates.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/handlers/bot_info.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/handlers/handlers.go (1)
internal/models/birthday.go (1)
Birthday(7-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
.claude/settings.local.json (1)
1-9: Config-only change looks fineAllow-listing only
grep,go list, andgo tool coveris narrow and reasonable for local tooling; no issues from an app/runtime standpoint.internal/templates/templates.go (1)
17-34: Template loading and FuncMap wiring look solid
LoadTemplatescorrectly usessync.Onceandtemplate.Must(ParseFS)with the registered helpers (dict, time/date formatters, unknown‑year helpers). This is idiomatic and should be safe for concurrent HTTP handlers once initialized.internal/models/birthday.go (1)
1-16: Model documentation aligns with usageThe added package and field comments clearly describe
Birthdaysemantics (including0000-MM-DDfor unknown year andLastNotificationpurpose) and match how handlers/templates treat these fields. No code changes needed here.internal/handlers/handlers.go (1)
19-63: BotInfo and BotStatusProvider shape look good and match template needsThe
PageData,BotInfo, andBotStatusProviderdefinitions are coherent and give templates everything they need (status, identity, uptime, notification window, next check, and configured flag). The interface surface is minimal and focused, which is good for testing and for decoupling handlers from the concretebot.Botimplementation.The
*bot.Bottype correctly implements all six methods ofBotStatusProvider:GetStatus(),GetUsername(),GetFirstName(),GetUptime(),GetNotificationsSent(), andGetNotificationHours()with matching signatures. IndexHandler also properly handles the nil case and uses the interface methods correctly.internal/logger/logger.go (1)
77-189: Structured, component-aware logging API looks solidThe levelled helpers (
Debug/Info/Warn/Errorplus the*fvariants) and domain-specific functions (LogRequest,LogBotMessage,LogBotAction,LogStorage,LogNotification) look cohesive and give good structure around log messages and components.Assuming the env/level interaction is fixed as above, this is a nice upgrade in observability.
internal/bot/bot.go (1)
67-85: Notification hour parsing and window logic look correct and flexibleParsing
NOTIFICATION_START_HOUR/NOTIFICATION_END_HOURwith validation and sensible defaults, combined withisWithinNotificationHours(including cross‑midnight windows), is clean and covers the main edge cases.Nice touch on logging invalid env values and falling back to defaults; this should make misconfiguration easy to diagnose.
Also applies to: 175-185, 522-536
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/bot/bot.go (3)
110-123: Lifecycle guard and shutdown behavior correctly address prior leak
Startnow uses a mutex +runningflag to avoid double-starting, and therunloop’sctx.Done()branch callsStopReceivingUpdates, flipsrunningback tofalseunder lock, and updates status, which resolves the previously flagged resource-leak / stray goroutine issue aroundGetUpdatesChan. One minor optional hardening would be to handle a closedupdateschannel explicitly (e.g.,update, ok := <-updatesand exit if!ok) to avoid a potential tight loop if the channel were ever closed independently ofctx.Done().Also applies to: 126-131, 198-225
685-689: Date-onlydaysDiffcalculation correctly fixes off‑by‑one behaviorNormalizing
nowtonowDateat UTC midnight before computingdaysDiff(andnextYearDaysDiff) makes the comparison purely date-based and removes the previous dependency on the current time-of-day, which is exactly what you want for “today / 2 weeks / 4 weeks” checks. The duplicatednextYearDaysDiffcomputation in the two branches is tiny; if this logic grows further, consider extracting a small helper for clarity, but it’s not necessary for correctness.Also applies to: 710-711, 760-761
584-600: Clarify or simplifyshouldSendBirthdayNotificationAPI
shouldSendBirthdayNotificationonly usesnotificationTypeandLastNotificationfor the"BIRTHDAY_TODAY"case; thedaysDiffparameter is unused, and the comment about “previous checks in the function” for 2/4‑week reminders is now misleading since those checks live inprocessBirthdays. Consider either (a) dropping thedaysDiffparameter and updating the comment, or (b) moving all last-notification/date-skipping logic into this helper so its signature/comment match its behavior.Also applies to: 731-769
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/bot/bot.go(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/bot/bot.go (1)
internal/logger/logger.go (1)
Warn(92-96)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
internal/bot/bot.go (1)
22-47: Bot state and initialization design looks soundStruct fields cover the necessary runtime state, and
Newdoes proper token validation, API creation,GetMe, env-based hour overrides with range checks, and context setup, with useful logging. No changes requested here.Also applies to: 50-107
Improved error handling in DeleteRowHandler
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/handlers/handlers.go (1)
138-164: Remove the unusednormalizeDatefunction.The
normalizeDatefunction (lines 138-164) is never called anywhere in the codebase. ThenormalizeDateWithOriginalfunction (called on line 117 inupdateBirthdayFromForm) serves the actual normalization logic. Delete the unusednormalizeDatefunction.
🧹 Nitpick comments (3)
internal/templates/funcs_test.go (1)
15-40: Consider adding edge case tests.The current test cases cover the main scenarios well. To further strengthen the test suite, consider adding test cases for:
- Invalid date formats (e.g.,
"invalid","","2025-13-01","2025-02-30")- Boundary dates (e.g.,
"0000-01-01","0000-12-31")Example addition:
+ { + name: "Invalid date format", + input: "invalid", + expected: "invalid", // or however the function handles invalid input + }, + { + name: "Empty string", + input: "", + expected: "", // or however the function handles empty input + },internal/storage/storage.go (1)
48-63: Consider extracting directory creation logic into a helper function.The directory creation logic (lines 56-60) is duplicated from
LoadBirthdays(lines 29-33). A small helper function would reduce duplication:func ensureParentDir(filePath string) error { if dir := filepath.Dir(filePath); dir != "" && dir != "." { return os.MkdirAll(dir, 0755) } return nil }internal/handlers/handlers.go (1)
114-136: Silent error handling may mask form validation issues.The function logs parse errors for
last_notificationandchat_idbut doesn't propagate them. While this prevents crashes, invalid form data will be silently ignored. Consider returning an error or at least clearing the affected field to signal invalid input to the user.Current behavior is acceptable for a web UI where users may not always fill all fields, but consider whether the UI should provide feedback on invalid inputs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/bot/birthday_notification_test.go(2 hunks)internal/bot/bot.go(16 hunks)internal/handlers/handlers.go(7 hunks)internal/logger/logger.go(8 hunks)internal/storage/storage.go(3 hunks)internal/templates/funcs.go(4 hunks)internal/templates/funcs_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/bot/birthday_notification_test.go
- internal/templates/funcs.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/storage/storage.go (1)
internal/models/birthday.go (1)
Birthday(7-16)
internal/handlers/handlers.go (2)
internal/models/birthday.go (1)
Birthday(7-16)internal/logger/logger.go (1)
Error(99-103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (25)
internal/templates/funcs_test.go (3)
8-13: Good practice: deterministic test setup.The save/restore pattern for
nowYearensures tests are deterministic and don't interfere with each other. This is excellent for testing time-dependent logic.
52-83: LGTM!The test cases correctly verify that
formatBirthDatereturns month-day format for unknown years and full date format for known years. The table-driven approach makes it easy to understand the expected behavior.
85-116: LGTM!The test cases correctly verify the unknown year detection logic. The function behavior is clear: dates prefixed with
"0000-"are considered to have unknown years.internal/storage/storage.go (2)
15-20: LGTM!The
getPath()function correctly prioritizes theYAML_PATHenvironment variable with a sensible default fallback.
25-44: LGTM!The function correctly handles first-run scenarios by creating parent directories and initializing an empty YAML array. Error handling is appropriate, and the directory permission (0755) is suitable for data directories.
internal/handlers/handlers.go (8)
19-63: LGTM!The
PageData,BotInfostruct, andBotStatusProviderinterface are well-designed with comprehensive documentation. The interface properly decouples the handlers from the bot implementation.
65-80: LGTM!The uptime formatting logic correctly handles various duration ranges with appropriate human-readable output.
82-99: LGTM!Both functions correctly handle the midnight-crossing case for notification windows, and the boundary logic is consistent.
101-105: LGTM!Simple and correct implementation that returns the next minute-aligned check time in UTC.
202-210: LGTM!Good helper pattern that reduces error handling duplication across handlers.
212-251: LGTM!Well-structured handler that gracefully handles nil
botProviderand properly renders the page template with bot status information.
253-292: LGTM!The handler correctly differentiates between adding new records (
idx == -1) and updating existing ones with proper bounds checking and error handling.
294-326: LGTM!The handler correctly validates index bounds and returns an error early for invalid indices. The slice deletion pattern is correct.
internal/bot/bot.go (9)
22-54: LGTM!The regex patterns are correct for date validation, and the
Botstruct is well-organized with proper synchronization primitives (mutex, context) for thread-safe operations.
56-114: LGTM!The constructor properly validates the token, fetches bot info from Telegram, parses notification hours with sensible defaults and validation, and sets up the context for graceful shutdown.
116-136: LGTM!The
Start()method correctly guards against duplicate goroutine spawns using therunningflag with proper mutex protection.Stop()cancels the context to signal shutdown.
138-202: LGTM!All getter methods correctly handle nil receivers and use read locks (
RLock) for thread-safe access to bot state.
223-231: Previous issue addressed:StopReceivingUpdates()is now called on shutdown.The shutdown sequence correctly calls
b.api.StopReceivingUpdates()to stop the long-polling goroutine and clears therunningflag before setting status to "stopped".
336-365: LGTM!Good extraction of chat name resolution logic into a reusable helper function. The fallback chain handles various chat types and edge cases appropriately.
698-704: Core fix for the off-by-one birthday notification issue.The normalization to midnight UTC ensures accurate date-only comparisons. This correctly fixes the issue where time-based comparisons could cause premature notifications (e.g., notifying on the day before if the check runs late in the evening).
598-614: LGTM!The function correctly prevents duplicate same-day birthday notifications while allowing reminder notifications. The duplicate check for reminders is handled earlier in
processBirthdaysat line 682.
616-800: LGTM!The
processBirthdaysfunction is comprehensive with proper handling of:
- Notification window checks
- Date extraction and validation
- Multiple notification scenarios (today, 14 days, 28 days)
- Next year birthday handling when current year birthday has passed
- Duplicate notification prevention
- Detailed logging for debugging
internal/logger/logger.go (3)
34-61: LGTM!The initialization correctly handles both
DEBUGandLOG_LEVELenvironment variables. Note thatLOG_LEVELtakes precedence when both are set (since it's checked second), which is reasonable behavior.
63-103: LGTM!Core logging functions are correctly implemented with consistent formatting and proper level filtering.
105-189: LGTM!Good implementation of backward-compatible logging variants and specialized helpers. The debug-mode checks appropriately control verbosity and avoid logging sensitive information (message content, user agent) in production.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/handlers/handlers.go (1)
147-150: MM-DD format validation is weak.The check
len(s) == 5 && strings.Count(s, "-") == 1accepts strings like"99-99"or"AB-CD"without validating they represent valid month-day values. Consider validating the parsed result.// Handle "MM-DD" format - convert to "0000-MM-DD" if len(s) == 5 && strings.Count(s, "-") == 1 { + // Validate by attempting to parse as a date + if _, err := time.Parse("01-02", s); err != nil { + return "" + } return "0000-" + s }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/handlers/handlers.go(8 hunks)internal/handlers/handlers_unit_test.go(2 hunks)internal/storage/storage.go(3 hunks)internal/templates/funcs_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
internal/storage/storage.go (1)
internal/models/birthday.go (1)
Birthday(7-16)
internal/handlers/handlers_unit_test.go (1)
internal/models/birthday.go (1)
Birthday(7-16)
internal/handlers/handlers.go (2)
internal/models/birthday.go (1)
Birthday(7-16)internal/logger/logger.go (2)
Error(99-103)Errorf(126-128)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (18)
internal/templates/funcs_test.go (3)
7-80: LGTM! Comprehensive test coverage with proper test isolation.The test function demonstrates excellent practices:
- Proper cleanup using defer to restore
nowYear(line 10)- Deterministic testing with fixed year 2025 (line 13)
- Comprehensive edge case coverage including leap day handling, boundary dates, and invalid inputs
- The leap year logic at line 28 is clever and correct—using 2000 for Feb 29 when the current year (2025) isn't a leap year
82-143: Verify the intended behavior for invalid date inputs.The test cases at lines 124-131 expect invalid dates (e.g., "2025-13-01", "2025-02-30") to be returned unchanged. This means
formatBirthDateperforms string manipulation without date validation. Please confirm this is the desired behavior, as it could potentially propagate invalid data downstream.Consider documenting this behavior in the function's comments if it's intentional, or add validation if stricter input handling is preferred.
187-190: Verify that prefix-only checking is intentional.The test at line 189 expects
isUnknownYear("0000-13-01")to returntrue, meaning the function checks only the "0000-" prefix without validating the month/day components. This is consistent with the no-validation approach seen in other tests, but please confirm this is the desired behavior. If so, consider documenting that the function performs prefix matching only, not date validation.internal/storage/storage.go (4)
1-2: Excellent package documentation.The documentation clearly describes the package's purpose and key functionality.
22-28: Well-implemented directory creation helper.The function correctly handles edge cases: skips creation for current directory (dir = "."), uses appropriate directory permissions (0755 vs 0644 for files), and leverages
os.MkdirAll's idempotent behavior for safe concurrent access.
36-39: Correct placement and error handling.Calling
ensureParentDironly when the file doesn't exist is the right approach—if the file exists, its parent directory must already exist.
60-65: Robust directory handling before file write.Always calling
ensureParentDirbefore writing is the safe approach—it's idempotent and ensures directories exist even if they were removed between operations.internal/handlers/handlers_unit_test.go (2)
22-25: LGTM!The test correctly captures and validates the error return from
updateBirthdayFromForm, aligning with the updated function signature.
82-110: LGTM!Good test coverage for the edge case where optional fields are empty. The assertions correctly verify that
LastNotificationdefaults to zero time andChatIDdefaults to 0.internal/handlers/handlers.go (9)
19-47: LGTM!The
PageDataandBotInfostructs are well-documented with clear field descriptions. The types appropriately support the template rendering needs.
49-63: LGTM!Clean interface definition with proper documentation. The interface follows Go conventions for provider patterns.
65-80: LGTM!The uptime formatting logic correctly handles different time ranges with appropriate human-readable output.
91-99: Clarify notification window boundary semantics.The window check uses inclusive boundaries (
currentHour <= endHour). IfendHour=20, notifications are allowed at hour 20 (20:00-20:59). Ensure this matches the expected behavior, as some systems treat end times as exclusive.
107-112: LGTM!Simple and correct implementation of form index parsing with proper error propagation.
114-140: LGTM!The updated function properly propagates parsing errors with informative messages. Good use of
fmt.Errorfwith%wfor error wrapping, and appropriate logging for debugging.
188-227: LGTM!The handler correctly handles both configured and unconfigured bot states. Error handling for template execution is appropriate.
247-263: LGTM!The handler now properly propagates form validation errors as HTTP 400 responses with descriptive messages. Both add (idx==-1) and update paths have consistent error handling.
300-304: LGTM!Good fix adding the explicit
returnafter the invalid idx error response. This prevents further execution and potential issues with template rendering after an error.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/handlers/handlers_unit_test.go (1)
83-111: Good edge case coverage.This test properly validates that empty optional fields default to zero values without causing errors. The assertions correctly verify:
LastNotification.IsZero()for empty timestampChatID == 0for empty chat_id- Required fields still process correctly
Optional: Consider using table-driven subtests.
For future test additions, you might consider using table-driven tests with
t.Runfor better organization and reporting:func TestUpdateBirthdayFromForm_ErrorCases(t *testing.T) { tests := []struct { name string form url.Values wantErr bool errContains string }{ { name: "invalid timestamp", form: url.Values{ "name": {"Alice"}, "birth_date": {"12-31"}, "last_notification": {"invalid-timestamp"}, "chat_id": {"123"}, }, wantErr: true, errContains: "invalid last_notification format", }, // ... more cases } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // test logic }) } }However, the current approach with separate test functions is perfectly acceptable and clear.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/handlers/handlers_unit_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/handlers/handlers_unit_test.go (2)
internal/models/birthday.go (1)
Birthday(7-16)internal/logger/logger.go (2)
Error(99-103)Errorf(126-128)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
internal/handlers/handlers_unit_test.go (3)
43-61: LGTM! Past review issue resolved.This test properly addresses the past review comment about ineffective error assertions. The implementation now:
- Uses
t.Fatalto fail immediately when error is nil (line 56)- Uses
strings.Containswitht.Errorfto properly validate error message content (lines 58-60)This pattern is more robust than exact string matching and ensures the test fails appropriately when expectations aren't met.
63-81: LGTM! Past review issue resolved.This test also properly addresses the past review comment with the same improved assertion pattern:
t.Fatalfor nil error check (line 76)strings.Containswitht.Errorffor error message validation (lines 78-80)Good coverage of the invalid chat_id error case.
23-26: Correct adaptation to error-returning function.The test properly handles the new error return value from
updateBirthdayFromForm, usingt.Fatalfto stop execution if an unexpected error occurs. This prevents meaningless assertions when the function fails.
1 participant
Summary by CodeRabbit
Bug Fixes
Tests
New Features
Storage
Logging
Templates
Config
✏️ Tip: You can customize this high-level summary in your review settings.