-
Notifications
You must be signed in to change notification settings - Fork 61
[#348] Update CRON logging + fix reminders hook #349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughAdds logging and restructures Sunday vs. non‑Sunday control flow in the reminder cron, aggregates and reports non‑404 errors while removing noisy logs in role‑sync, and removes the unused Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/cron/src/crons/reminder.ts`:
- Around line 140-148: The code duplicates work by recalculating the total in a
new variable total_events; instead remove the total_events declaration and
accumulation loop and reuse the already-computed totalEvents (from lines where
totalEvents is calculated) when logging the final count, while keeping the
groupedPrefixes iteration only for printing group/event details (use
groupedPrefixes and event.name/event.name references as before and replace
console.log(`Found a total of ${total_events} events`) with console.log(`Found a
total of ${totalEvents} events`)).
🧹 Nitpick comments (3)
apps/cron/src/crons/role-sync.ts (1)
37-37: Add explicit type annotation forerroredUsers.Without an explicit type, TypeScript infers
never[](strict mode) orany[], which can cause type errors or bypasses type checking. Since you're storing user names (strings), annotate it explicitly:✏️ Proposed fix
- let erroredUsers = []; + let erroredUsers: string[] = [];As per coding guidelines: "No TypeScript escape hatches: Flag usage of 'any' type... suggest... proper typing instead"
apps/cron/src/crons/reminder.ts (1)
166-168: Consider wrapping webhook calls in try-catch to prevent crashes.Per cron guidelines, "Discord webhook failures should not crash the service." Currently, if
webhook.send()fails (network issue, rate limit, invalid webhook), the entire cron will throw and terminate.🛡️ Example pattern for resilient webhook calls
try { await webhook.send({ content: `...` }); } catch (error) { console.error(`Failed to send webhook message:`, error); // Optionally continue or abort gracefully }As per coding guidelines: "Discord webhook failures should not crash the service"
Also applies to: 177-179, 254-254
apps/cron/src/index.ts (1)
21-23: Placeholder mention won't notify anyone — consider a TODO with issue tracking.
@WHOEVER_IS_DEV_LEAD_RNisn't a valid GitHub mention and won't alert anyone. If this needs manual re-enabling for hackathons, consider linking to an issue or adding a more actionable TODO:✏️ Suggested improvement
-// Silencing for now, needs to be manually re-enabled for hacks `@WHOEVER_IS_DEV_LEAD_RN` +// TODO(`#348`): Re-enable hackReminders before next hackathon - requires manual activation by Dev Lead // hackReminders.schedule();
DVidal1205
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Why
CRON logging was a little cluttered in some places and not enough in others
What
Issue(s): #348
Test Plan
Tested all crons and their logging locally.
Checklist
db:pushbefore mergingSummary by CodeRabbit
New Features
Bug Fixes
Chores