Support multiple workouts in submission window open emails#385
Open
zacjones93 wants to merge 2 commits intomainfrom
Open
Support multiple workouts in submission window open emails#385zacjones93 wants to merge 2 commits intomainfrom
zacjones93 wants to merge 2 commits intomainfrom
Conversation
When multiple workouts share the same submission window, send a single email listing all workouts instead of separate emails per workout. Groups events by (competitionId, submissionOpensAt) and consolidates them into one notification with all workout names and descriptions. https://claude.ai/code/session_01NPjNVihbNaLUbBNU4ZETri
Contributor
WalkthroughThe pull request extends the submission window notification system to support multiple workouts per notification email. The email component now accepts an array of workouts with fallback to legacy single-workout properties. The server notification handler groups "window opens" events by competition and submission time, then sends one email per group listing all associated workouts. Changes
Sequence Diagram(s)sequenceDiagram
participant Service as Notification Service
participant DB as Database
participant Grouper as Event Grouper
participant Email as Email Renderer
participant Mailer as Email Sender
Service->>Grouper: processSubmissionWindowNotifications(events)
Grouper->>Grouper: Group by (competitionId, submissionOpensAt)
loop For each event group
Grouper->>DB: Attempt reservation for each eventId
DB-->>Grouper: Reservation results
alt At least one succeeded
Grouper->>Email: render with workouts[]
Email-->>Grouper: HTML email
Grouper->>Mailer: send email
alt Email sent successfully
Grouper->>Grouper: Continue
else Email failed
Grouper->>DB: Delete all reservations for group
DB-->>Grouper: Deleted
end
else All failed
Grouper->>Grouper: Skip (already sent)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🚀 Preview DeployedURL: https://wodsmith-app-pr-385.zacjones93.workers.dev
This comment is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (1)
apps/wodsmith-start/test/server/notifications/submission-window.test.ts (1)
68-105:⚠️ Potential issue | 🟡 MinorAdd
//@lat:references to the updated specs.These updated tests are missing the required spec-trace comments. Please add exactly one
//@lat: [[section-id]]beside each relevantit(...)block.As per coding guidelines, "Add
//@lat: [[section-id]]comments in test code to tie source code to test specifications in lat.md" and "Place exactly one@lat:code reference comment next to the relevant test, not at the top of the file".Also applies to: 107-147, 149-180, 182-221, 223-249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/wodsmith-start/test/server/notifications/submission-window.test.ts` around lines 68 - 105, The test block for "sends notification when reservation succeeds (first time)" and the other listed it(...) blocks are missing the required spec-trace comment; add exactly one comment line like // `@lat`: [[section-id]] immediately beside each relevant it(...) declaration (e.g., directly after the it("sends notification when reservation succeeds (first time)", ...) line) for the tests that reference sendWindowOpensNotification and the other it blocks mentioned so each test has a single `@lat` reference tying it to the spec.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/wodsmith-start/src/server/notifications/submission-window.ts`:
- Around line 662-678: The grouping currently builds groupKey using
event.submissionOpensAt which collapses events with identical open times but
different close times; update the logic around windowOpenGroups and groupKey to
include event.submissionClosesAt (e.g., use
`${competition.id}::${event.submissionOpensAt}::${event.submissionClosesAt}`) or
alternatively assert that any event added to an existing group has the same
event.submissionClosesAt and reject/branch if it differs, and ensure the stored
group.submissionClosesAt comes from the grouping key (not the first-seen event)
so the correct deadline is used; apply the same change to the similar grouping
code near the second occurrence (the block referenced at lines ~804–805).
- Around line 251-270: The loop that calls reserveNotification over
competitionEventIds can result in race conditions; change it to perform a single
atomic reservation for the whole group using Drizzle ORM transactions so either
the entire set is reserved or none are. Wrap the reservations in
db.transaction() (or create a single group-lock row) and perform the per-event
reserve operations inside that transaction, using reserveNotification (or a new
groupReservation routine) and checking a single outcome (e.g., reservedEventIds
populated inside the transaction) before returning; reference
competitionEventIds, reservedEventIds, reserveNotification, and
SUBMISSION_WINDOW_NOTIFICATION_TYPES.WINDOW_OPENS when making the change. Ensure
the transaction returns a definitive boolean/owned list so only one process
proceeds to send the grouped email.
---
Outside diff comments:
In `@apps/wodsmith-start/test/server/notifications/submission-window.test.ts`:
- Around line 68-105: The test block for "sends notification when reservation
succeeds (first time)" and the other listed it(...) blocks are missing the
required spec-trace comment; add exactly one comment line like // `@lat`:
[[section-id]] immediately beside each relevant it(...) declaration (e.g.,
directly after the it("sends notification when reservation succeeds (first
time)", ...) line) for the tests that reference sendWindowOpensNotification and
the other it blocks mentioned so each test has a single `@lat` reference tying it
to the spec.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 28227d69-9db1-4640-8b59-e8d6f3f15c39
📒 Files selected for processing (3)
apps/wodsmith-start/src/react-email/submission-window-opens.tsxapps/wodsmith-start/src/server/notifications/submission-window.tsapps/wodsmith-start/test/server/notifications/submission-window.test.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/wodsmith-start/src/server/notifications/submission-window.ts">
<violation number="1" location="apps/wodsmith-start/src/server/notifications/submission-window.ts:268">
P1: Grouped window-open emails can be sent twice in concurrent runs because sending is allowed when only a subset of event reservations succeeds.</violation>
<violation number="2" location="apps/wodsmith-start/src/server/notifications/submission-window.ts:664">
P2: The grouping key omits `submissionClosesAt`, so events with different close times can be merged and emailed with the wrong close deadline.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…osesAt in group key Wrap grouped notification reservations in a db.transaction() so either all events are reserved or none are, preventing duplicate emails from concurrent cron runs. Also include submissionClosesAt in the grouping key so events with different close times aren't merged into one email with the wrong deadline. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/wodsmith-start/src/server/notifications/submission-window.ts">
<violation number="1" location="apps/wodsmith-start/src/server/notifications/submission-window.ts:279">
P1: Early-returning `[]` inside the transaction can commit partial reservations, causing some events to be marked as notified even though no grouped email was sent.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| if (existing) { | ||
| // This event was already reserved — another process owns this group | ||
| return [] |
Contributor
There was a problem hiding this comment.
P1: Early-returning [] inside the transaction can commit partial reservations, causing some events to be marked as notified even though no grouped email was sent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/wodsmith-start/src/server/notifications/submission-window.ts, line 279:
<comment>Early-returning `[]` inside the transaction can commit partial reservations, causing some events to be marked as notified even though no grouped email was sent.</comment>
<file context>
@@ -248,22 +248,49 @@ export async function sendWindowOpensNotification(params: {
+
+ if (existing) {
+ // This event was already reserved — another process owns this group
+ return []
+ }
+
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR enables sending a single "submission window opens" email when multiple workouts in the same competition share the same submission window, improving the user experience by reducing email volume for events with multiple concurrent workouts.
Key Changes
Updated
sendWindowOpensNotificationfunction to accept multiple competition event IDs and workout information instead of a single event/workoutcompetitionEventIdparameter tocompetitionEventIds: string[]workoutName/workoutDescriptionparameters toworkouts: WorkoutInfo[]Enhanced email template (
SubmissionWindowOpensEmail) to display multiple workoutsWorkoutInfointerface for structured workout dataworkoutName/workoutDescriptionpropsRefactored notification processing in
processSubmissionWindowNotificationsWindowOpenGroupinterface to group events by competition + submission open timeUpdated tests to reflect new multi-event API
Implementation Details
${competitionId}::${submissionOpensAt}, ensuring events in the same competition with identical submission times are grouped togetherhttps://claude.ai/code/session_01NPjNVihbNaLUbBNU4ZETri
Summary by cubic
Send a single “submission window opens” email when multiple workouts in the same competition share the same window, listing all workouts in one message. Adds atomic reservation to prevent duplicate sends and groups by competition + open time + close time.
New Features
SubmissionWindowOpensEmailto render multiple workouts with pluralized copy, dividers, and a pluralized CTA; subject lists all workout names.Migration
sendWindowOpensNotificationnow takescompetitionEventIds: string[]andworkouts: WorkoutInfo[](replacescompetitionEventId,workoutName, andworkoutDescription).SubmissionWindowOpensEmailsupportsworkouts; single-workout props are still accepted but deprecated.Written for commit 7c439ce. Summary will update on new commits.
Summary by CodeRabbit
New Features
Chores