Conversation
WalkthroughThis pull request introduces comprehensive CI/build infrastructure, code quality tooling, and a new birthday notification validation helper method. Changes include workflow automation, pre-commit hooks, linting configuration, a Makefile, updated Docker setup, and minor formatting/whitespace adjustments across the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant Cron as Scheduled Job
participant Bot as processBirthdays()
participant Helper as shouldSendBirthdayNotification()
participant DB as Models/Database
participant Notifier as Send Notification
Cron->>Bot: Trigger birthday check
Bot->>Bot: Check current hour in window?
alt Outside notification window
Bot->>Bot: Early return (skip processing)
else Within window
Bot->>DB: Fetch birthdays
loop For each birthday
Bot->>Helper: shouldSendBirthdayNotification(birthday, type, daysDiff)
alt BIRTHDAY_TODAY type
Helper->>Helper: Check LastNotification same day?
alt Same day
Helper-->>Bot: false (no repeat)
else Different day
Helper-->>Bot: true (send)
end
else Other types
Helper-->>Bot: true (send)
end
alt Should send?
Bot->>Notifier: Send notification
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ 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: 3
🧹 Nitpick comments (4)
.github/workflows/docker.yml (1)
4-4: Consider fixing the yamllint warning instead of disabling it.The
yamllint disable-line rule:truthycomment suggests that the YAML parser flagson:as a truthy value. While the disable comment is functional, consider using the alternative syntax'on':or"on":to avoid needing the disable comment altogether.Apply this diff to use quoted syntax instead:
-on: # yamllint disable-line rule:truthy +'on': push: branches: [main].pre-commit-config.yaml (1)
26-31: Update golangci-lint to v2.6.1 (or latest stable version).The pinned version
v1.55.2is significantly outdated. The latest golangci-lint release is v2.6.1 (released November 4, 2025), representing a full major version jump with likely improvements, new linters, and bug fixes. Update to a current version to benefit from these enhancements.internal/bot/birthday_notification_test.go (1)
35-41: Clarify the test scenario with a more realistic setup.The test sets
LastNotificationto a future date (tomorrow) and then checks if a notification should be sent today. While this technically validates the date comparison logic, it represents an unrealistic scenario—LastNotificationshould never be in the future during normal operation.Consider revising the test to use a more realistic past date:
- // Simulate birthday notification on a different day - futureTime := time.Now().AddDate(0, 0, 1) - testBirthday.LastNotification = futureTime + // Simulate birthday notification on a different day (yesterday) + pastTime := time.Now().AddDate(0, 0, -1) + testBirthday.LastNotification = pastTime shouldSend = bot.shouldSendBirthdayNotification(testBirthday, "BIRTHDAY_TODAY", 0) if !shouldSend { t.Error("Birthday notification should be sent on a different day") }internal/bot/bot.go (1)
530-546: Consider removing or utilizing the unuseddaysDiffparameter.The
daysDiffparameter is declared but never used within the method body. If it's not needed for future logic, consider removing it to simplify the signature.If the parameter is not needed:
-func (b *Bot) shouldSendBirthdayNotification(birthday models.Birthday, notificationType string, daysDiff int) bool { +func (b *Bot) shouldSendBirthdayNotification(birthday models.Birthday, notificationType string) bool {Then update the call site at line 674:
- if b.shouldSendBirthdayNotification(birthday, notificationType, daysDiff) { + if b.shouldSendBirthdayNotification(birthday, notificationType) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.github/workflows/docker.yml(2 hunks).gitignore(1 hunks).pre-commit-config.yaml(1 hunks).yamllint(1 hunks)Makefile(1 hunks)cmd/app/main.go(0 hunks)cmd/app/main_test.go(1 hunks)docker-compose.yml(1 hunks)internal/bot/birthday_notification_test.go(1 hunks)internal/bot/bot.go(6 hunks)internal/bot/left_member_test.go(1 hunks)internal/handlers/bot_info.go(2 hunks)internal/handlers/handlers.go(2 hunks)internal/logger/logger.go(1 hunks)internal/logger/logger_test.go(1 hunks)internal/templates/templates.go(1 hunks)internal/templates/tmpl/bot-info.gohtml(1 hunks)internal/templates/tmpl/scripts.gohtml(1 hunks)internal/templates/tmpl/styles.gohtml(1 hunks)
💤 Files with no reviewable changes (1)
- cmd/app/main.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/bot/birthday_notification_test.go (2)
internal/bot/bot.go (1)
Bot(19-31)internal/models/birthday.go (1)
Birthday(5-10)
internal/bot/bot.go (1)
internal/models/birthday.go (1)
Birthday(5-10)
🪛 checkmake (0.2.2)
Makefile
[warning] 84-84: Target "ci" should be declared PHONY.
(phonydeclared)
⏰ 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 (10)
docker-compose.yml (1)
12-21: Clarify Compose version requirement fordevelop.watch.Docker Compose only accepts the
developsection starting with CLI v2.22. Older installations (including the legacydocker-composebinary) will error out with “additional property develop is not allowed,” so teammates on older tooling won’t be able to rundocker compose up. Please confirm our baseline version or gate this behind anx-developextension / document the minimum version so local setups don’t break.(docs.docker.com).yamllint (1)
1-11: LGTM!The YAML lint configuration is sensible, extending the default rules while providing appropriate customizations for comments and line length flexibility.
cmd/app/main_test.go (1)
49-55: LGTM!The formatting alignment improves readability without changing the test logic or values.
.gitignore (1)
40-41: LGTM!The build artifacts section and
build/directory ignore pattern align well with the Makefile targets that generate build outputs.internal/templates/templates.go (1)
20-27: LGTM!The formatting alignment of the function map improves code readability without altering functionality.
internal/handlers/bot_info.go (1)
17-27: LGTM!The formatting alignment improves readability of the struct initialization.
internal/handlers/handlers.go (2)
21-31: LGTM!The formatting alignment of the struct fields improves code readability.
192-202: LGTM!The formatting alignment of the struct initialization is consistent with the struct declaration changes.
.pre-commit-config.yaml (1)
1-56: LGTM!The pre-commit configuration is well-structured with appropriate hooks for code quality, security checks, and Go-specific tooling. The
always_run: truesetting ensures consistent enforcement across all commits.internal/bot/bot.go (1)
673-674: Good refactoring to centralize notification decision logic.The introduction of
shouldSendBirthdayNotificationimproves code clarity by consolidating the notification decision logic, making it easier to test and extend in the future.
| - name: Set up Go | ||
| uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: 'stable' | ||
|
|
||
| - name: Install dependencies | ||
| run: make deps | ||
|
|
||
| - name: Run tests | ||
| run: make ci |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that the Makefile targets are ordered correctly.
The workflow runs make deps followed by make ci. Ensure that make ci doesn't already run make deps internally, as this would result in unnecessary duplication. If make ci is comprehensive (including dependency installation), consider removing the separate make deps step.
Run the following script to check the Makefile targets:
🏁 Script executed:
#!/bin/bash
# Description: Examine Makefile to verify deps and ci targets and their dependencies
# Check if Makefile exists and display the deps and ci target definitions
if [ -f "Makefile" ]; then
echo "=== Makefile deps target ==="
rg -A 10 '^deps:' Makefile || echo "deps target not found"
echo ""
echo "=== Makefile ci target ==="
rg -A 10 '^ci:' Makefile || echo "ci target not found"
else
echo "Makefile not found in repository"
fiLength of output: 287
Remove the duplicate make deps step from the workflow.
The ci target already includes deps as a prerequisite (ci: deps fmt vet test build), so running make deps separately before make ci causes unnecessary duplication. Remove the "Install dependencies" step (line 31-32) and keep only the "Run tests" step, which will automatically install dependencies first.
🤖 Prompt for AI Agents
In .github/workflows/docker.yml around lines 26 to 35, remove the redundant
"Install dependencies" step that runs `make deps` (lines 31-32) because the `ci`
Make target already depends on `deps`; update the workflow to delete those two
lines so only the "Run tests" step remains and `make ci` will handle installing
dependencies first.
| .env | ||
|
|
||
| /data/ | ||
| -e |
There was a problem hiding this comment.
Remove the erroneous -e entry.
The literal -e entry appears to be a mistake, possibly from an echo -e command that was accidentally committed. This is not a valid ignore pattern and should be removed.
Apply this diff to remove the erroneous entry:
/data/
--e
# Build artifacts
build/📝 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.
| -e | |
| /data/ | |
| # Build artifacts | |
| build/ |
🤖 Prompt for AI Agents
In .gitignore around line 39, there's an erroneous literal entry "-e" that is
not a valid ignore pattern; remove that line so the file does not contain the
stray "-e" entry (simply delete the line and save the file), then run a quick
git status to confirm no unintended changes remain and commit the fix.
|
Warning Docstrings generation - IN PROGRESS Generating docstrings for this pull request |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Summary by CodeRabbit
New Features
Chores