Conversation
WalkthroughThe change modifies the Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/notification/notification-event.service.ts`:
- Around line 98-105: The current parallel writes over mentions call
this.notificationService.createMentionNotification(post, mention.id) which uses
a raw insert and can create duplicate notifications on retries; update
createMentionNotification to use the same idempotency pattern used for
likes/follows by applying an .onConflict().ignore() (or equivalent conflict
resolution) to the insert into the notifications table (or add a DB unique
constraint on (user_id, account_id, post_id, event_type) and then use
onConflict().ignore()) so concurrent/retried inserts do not produce duplicates.
| await Promise.all( | ||
| mentions.map((mention) => | ||
| this.notificationService.createMentionNotification( | ||
| post, | ||
| mention.id, | ||
| ), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# 1) Find the createMentionNotification implementation
rg -n "createMentionNotification" --type=ts -A 10Repository: TryGhost/ActivityPub
Length of output: 9025
🏁 Script executed:
# 2) Check the NotificationService implementation for idempotency handling
rg -n "class NotificationService" --type=ts -A 20Repository: TryGhost/ActivityPub
Length of output: 1650
🏁 Script executed:
# 3) Search for database migrations and schema constraints on notifications
fd -e "migration" -o -e "sql" | head -20Repository: TryGhost/ActivityPub
Length of output: 180
🏁 Script executed:
# 4) Look for any uniqueness constraints or upsert logic in notification creation
rg -n "unique|constraint|upsert" --type=ts --type=sql | grep -i notifRepository: TryGhost/ActivityPub
Length of output: 46
🏁 Script executed:
# 5) Check if there are any comments indicating placeholder/dummy status
rg -n "placeholder|dummy|TODO|FIXME|draft" --type=ts src/notification/ | head -20Repository: TryGhost/ActivityPub
Length of output: 46
🏁 Script executed:
# Get the full implementation of createMentionNotification
sed -n '440,500p' src/notification/notification.service.tsRepository: TryGhost/ActivityPub
Length of output: 1915
🏁 Script executed:
# Search for database write operations (insert, create, save) in the notification service
rg -n "\.insert\(|\.create\(|\.save\(" src/notification/notification.service.ts -A 5Repository: TryGhost/ActivityPub
Length of output: 1245
🏁 Script executed:
# Check the notifications table schema in migrations
find . -type f -name "*migration*" | xargs grep -l "notifications" 2>/dev/null | head -5Repository: TryGhost/ActivityPub
Length of output: 46
🏁 Script executed:
# Look for any database schema definitions for notifications table
rg -n "createTable.*notifications|table.*notifications" --type=ts --type=js -A 15Repository: TryGhost/ActivityPub
Length of output: 46
🏁 Script executed:
# Check for any idempotency or upsert patterns in notification creation
rg -n "onConflict|onDuplicate|upsert|returning" src/notification/ --type=tsRepository: TryGhost/ActivityPub
Length of output: 46
🏁 Script executed:
# Check if there's a unique constraint or index on the notifications table
rg -n "notifications" -A 30 --type=ts | grep -E "(unique|constraint|index|primary)" | head -20Repository: TryGhost/ActivityPub
Length of output: 177
🏁 Script executed:
# Look for the schema/migration files specifically
find . -type f \( -name "*.ts" -o -name "*.js" -o -name "*.sql" \) | xargs grep -l "notifications" | grep -i migratRepository: TryGhost/ActivityPub
Length of output: 842
🏁 Script executed:
# Check knex schema builder usage for notifications table
rg -n "notifications" --type=ts -B 5 -A 20 | grep -E "(createTable|table|unique|primary|index)" | head -30Repository: TryGhost/ActivityPub
Length of output: 838
🏁 Script executed:
# Read the initial notifications table schema
cat migrate/migrations/000026_add-notifications-table.up.sqlRepository: TryGhost/ActivityPub
Length of output: 722
🏁 Script executed:
# Read the down migration for completeness
cat migrate/migrations/000026_add-notifications-table.down.sqlRepository: TryGhost/ActivityPub
Length of output: 99
🏁 Script executed:
# Check the index migration
cat migrate/migrations/000058_add-index-on-notifications-table.up.sqlRepository: TryGhost/ActivityPub
Length of output: 145
🏁 Script executed:
# Check if there are any other places where mention notifications are created
rg -n "createMentionNotification|notifications.*insert" src/notification/ --type=ts | head -20Repository: TryGhost/ActivityPub
Length of output: 1842
🏁 Script executed:
# Verify the current behavior by checking tests that might reveal idempotency expectations
rg -n "createMentionNotification" src/notification/notification.service.integration.test.ts -B 5 -A 15Repository: TryGhost/ActivityPub
Length of output: 3363
🏁 Script executed:
# Check if there's any test that calls createMentionNotification twice with the same params
rg -n "createMentionNotification.*createMentionNotification" src/notification/notification.service.integration.test.ts -B 5 -A 10Repository: TryGhost/ActivityPub
Length of output: 46
🏁 Script executed:
# Look for any idempotency patterns (UPSERT, INSERT...ON DUPLICATE, etc.) elsewhere in the codebase
rg -n "INSERT.*ON|onConflict|merge|upsert" --type=ts --type=sql src/ | head -15Repository: TryGhost/ActivityPub
Length of output: 1042
🏁 Script executed:
# Check if there's a test that calls createMentionNotification twice with same params to verify duplicate behavior
rg -n "createMentionNotification" src/notification/notification.service.integration.test.ts -B 10 -A 5 | grep -E "(createMentionNotification|expect.*length|it\()" | head -40Repository: TryGhost/ActivityPub
Length of output: 395
🏁 Script executed:
# Examine the notification-event-service to see if there's retry logic at a higher level
rg -n "retry|Retry|attempt|Attempt|catch|error" src/notification/notification-event.service.ts -B 3 -A 3Repository: TryGhost/ActivityPub
Length of output: 46
Parallel mention writes without idempotency protection create duplicate notification risk
The Promise.all on line 98 will reject on first failure, but concurrently started writes may succeed, causing partial writes. Without idempotency or a database uniqueness constraint on mention notifications, retrying will create duplicates.
The notifications table lacks a unique constraint on (user_id, account_id, post_id, event_type), and createMentionNotification uses a raw .insert() with no conflict handling. Apply the .onConflict().ignore() pattern (used elsewhere in the codebase for likes and follows) to prevent duplicates on retry.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notification/notification-event.service.ts` around lines 98 - 105, The
current parallel writes over mentions call
this.notificationService.createMentionNotification(post, mention.id) which uses
a raw insert and can create duplicate notifications on retries; update
createMentionNotification to use the same idempotency pattern used for
likes/follows by applying an .onConflict().ignore() (or equivalent conflict
resolution) to the insert into the notifications table (or add a DB unique
constraint on (user_id, account_id, post_id, event_type) and then use
onConflict().ignore()) so concurrent/retried inserts do not produce duplicates.
1 participant
ref https://linear.app/ghost/issue/BER-3241