feat(comms): trigger notif deletes on soft delete#2789
Merged
Conversation
Contributor
seanaye
commented
Apr 23, 2026
seanaye
commented
- trigger notification deletes via comms message deletes
- remove old migrations
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 40 minutes and 45 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis change removes 14 database migration files from the comms database client and introduces a new migration in the macro database client that implements cascading soft-delete logic for communications messages to their associated notifications. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@rust/cloud-storage/macro_db_client/migrations/20260423120000_cascade_comms_message_delete_to_notifications.sql`:
- Around line 8-31: Add an index to speed lookups on
notification.metadata->>'messageId' used by the trigger
cascade_comms_message_delete_to_notifications() and the backfill DELETE;
specifically, add a B-tree expression index like
notification_channel_message_id_idx ON notification ((metadata->>'messageId'))
WITH a WHERE clause event_item_type = 'channel' (or alternatively add a GIN
index notification_metadata_gin_idx USING GIN (metadata jsonb_path_ops) if you
need wider JSONB querying), and include the CREATE INDEX IF NOT EXISTS statement
in this same migration so both the trigger deletes and the backfill use the
index.
- Around line 8-31: The backfill DELETE currently removes any notification with
a metadata->>'messageId' pointing to a deleted comms_messages row but omits the
same event_item_id check used by the trigger
cascade_comms_message_delete_to_notifications; update the backfill to include a
join to comms_messages (alias m) and add a predicate matching n.event_item_id =
m.channel_id::text (and keep event_item_type = 'channel' and the m.deleted_at IS
NOT NULL check) so the backfill mirrors the trigger's three conditions
(event_item_type, event_item_id, metadata->>'messageId').
- Around line 18-21: Make the trigger creation idempotent and align the backfill
WHERE clause with the trigger: change the trigger DDL for
trg_cascade_comms_message_delete_to_notifications to use the PostgreSQL 16
idempotent form (CREATE OR REPLACE TRIGGER) instead of plain CREATE TRIGGER, and
update the backfill DELETE/UPDATE statement to include the same event_item_id
filter used by the trigger (the event_item_id = channel_id check — mirroring the
trigger's event_item_id = NEW.channel_id::text logic) so the backfill only
touches rows the trigger would handle; ensure you still call the same
cascade_comms_message_delete_to_notifications() function from the trigger.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 86042550-ccfe-4845-a178-bcaa22276112
📒 Files selected for processing (15)
rust/cloud-storage/comms_db_client/migrations/20250110210334_schema.sqlrust/cloud-storage/comms_db_client/migrations/20250207203331_nullable_channel.name_and_constraints.sqlrust/cloud-storage/comms_db_client/migrations/20250303194724_non_user_specific_mentions.sqlrust/cloud-storage/comms_db_client/migrations/20250305165720_add_mention_index_on_entity.sqlrust/cloud-storage/comms_db_client/migrations/20250320212539_activity_index.sqlrust/cloud-storage/comms_db_client/migrations/20250613182043_attachment_references_index.sqlrust/cloud-storage/comms_db_client/migrations/20250625000000_optimize_message_mentions_index.sqlrust/cloud-storage/comms_db_client/migrations/20250625174641_entity_mentions.sqlrust/cloud-storage/comms_db_client/migrations/20250627000000_add_uuid_to_entity_mentions.sqlrust/cloud-storage/comms_db_client/migrations/20250627000001_allow_duplicate_entity_mentions.sqlrust/cloud-storage/comms_db_client/migrations/20250627000002_add_user_id_to_entity_mentions.sqlrust/cloud-storage/comms_db_client/migrations/20250628000000_add_user_id_string_to_entity_mentions.sqlrust/cloud-storage/comms_db_client/migrations/20250729201036_optimize_channel_query_indexes.sqlrust/cloud-storage/comms_db_client/migrations/20250820120000_optimize_latest_messages_index.sqlrust/cloud-storage/macro_db_client/migrations/20260423120000_cascade_comms_message_delete_to_notifications.sql
💤 Files with no reviewable changes (14)
- rust/cloud-storage/comms_db_client/migrations/20250628000000_add_user_id_string_to_entity_mentions.sql
- rust/cloud-storage/comms_db_client/migrations/20250305165720_add_mention_index_on_entity.sql
- rust/cloud-storage/comms_db_client/migrations/20250729201036_optimize_channel_query_indexes.sql
- rust/cloud-storage/comms_db_client/migrations/20250820120000_optimize_latest_messages_index.sql
- rust/cloud-storage/comms_db_client/migrations/20250627000002_add_user_id_to_entity_mentions.sql
- rust/cloud-storage/comms_db_client/migrations/20250207203331_nullable_channel.name_and_constraints.sql
- rust/cloud-storage/comms_db_client/migrations/20250613182043_attachment_references_index.sql
- rust/cloud-storage/comms_db_client/migrations/20250303194724_non_user_specific_mentions.sql
- rust/cloud-storage/comms_db_client/migrations/20250627000001_allow_duplicate_entity_mentions.sql
- rust/cloud-storage/comms_db_client/migrations/20250625000000_optimize_message_mentions_index.sql
- rust/cloud-storage/comms_db_client/migrations/20250110210334_schema.sql
- rust/cloud-storage/comms_db_client/migrations/20250625174641_entity_mentions.sql
- rust/cloud-storage/comms_db_client/migrations/20250627000000_add_uuid_to_entity_mentions.sql
- rust/cloud-storage/comms_db_client/migrations/20250320212539_activity_index.sql
Comment on lines
+8
to
+31
| IF OLD.deleted_at IS NULL AND NEW.deleted_at IS NOT NULL THEN | ||
| DELETE FROM notification | ||
| WHERE event_item_type = 'channel' | ||
| AND event_item_id = NEW.channel_id::text | ||
| AND metadata->>'messageId' = NEW.id::text; | ||
| END IF; | ||
| RETURN NEW; | ||
| END; | ||
| $$ LANGUAGE plpgsql; | ||
|
|
||
| CREATE TRIGGER trg_cascade_comms_message_delete_to_notifications | ||
| AFTER UPDATE OF deleted_at ON comms_messages | ||
| FOR EACH ROW | ||
| EXECUTE FUNCTION cascade_comms_message_delete_to_notifications(); | ||
|
|
||
| -- Backfill: sweep notifications for already-soft-deleted messages. | ||
| DELETE FROM notification n | ||
| WHERE n.event_item_type = 'channel' | ||
| AND n.metadata->>'messageId' IS NOT NULL | ||
| AND EXISTS ( | ||
| SELECT 1 FROM comms_messages m | ||
| WHERE m.id::text = n.metadata->>'messageId' | ||
| AND m.deleted_at IS NOT NULL | ||
| ); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether any existing migration already indexes notification.metadata.
rg -nP -C2 'CREATE\s+(UNIQUE\s+)?INDEX.*\bnotification\b' --type=sql
rg -nP -C2 "metadata" rust/cloud-storage/macro_db_client/migrationsRepository: macro-inc/macro
Length of output: 9105
Add an index on notification(metadata->>'messageId') to support the new trigger and backfill efficiently.
The trigger function executes a DELETE FROM notification WHERE metadata->>'messageId' = $X on every soft-delete of comms_messages. The backfill query also filters by this expression. Without an index, both operations perform sequential scans. This is acceptable for the one-time backfill but will be paid repeatedly on production deletes.
Currently, only event_item_id, event_item_type, and a composite (event_item_type, event_item_id) index exist; no index covers the metadata JSONB expression.
Consider adding one of these in the same migration:
-- Narrow B-tree expression index (best if messageId is the primary metadata access pattern):
CREATE INDEX IF NOT EXISTS notification_channel_message_id_idx
ON notification ((metadata->>'messageId'))
WHERE event_item_type = 'channel';
-- Or GIN if metadata is frequently queried by multiple keys:
CREATE INDEX IF NOT EXISTS notification_metadata_gin_idx
ON notification USING GIN (metadata jsonb_path_ops);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@rust/cloud-storage/macro_db_client/migrations/20260423120000_cascade_comms_message_delete_to_notifications.sql`
around lines 8 - 31, Add an index to speed lookups on
notification.metadata->>'messageId' used by the trigger
cascade_comms_message_delete_to_notifications() and the backfill DELETE;
specifically, add a B-tree expression index like
notification_channel_message_id_idx ON notification ((metadata->>'messageId'))
WITH a WHERE clause event_item_type = 'channel' (or alternatively add a GIN
index notification_metadata_gin_idx USING GIN (metadata jsonb_path_ops) if you
need wider JSONB querying), and include the CREATE INDEX IF NOT EXISTS statement
in this same migration so both the trigger deletes and the backfill use the
index.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all insertions into the notification table and inspect event_item_id vs metadata.messageId conventions.
rg -nP --type=rust -C4 'INTO\s+notification\b|insert_into\(\s*notification\b|from\s+notification\b'
rg -nP --type=rust -C6 "event_item_type.*['\"]channel['\"]|['\"]channel['\"].*event_item_type"
rg -nP --type=rust -C4 "metadata.*messageId|\"messageId\""Repository: macro-inc/macro
Length of output: 28141
🏁 Script executed:
# Find calls to the notification repository insertion
rg -nP --type=rust -C5 "dispatch_notification|notification.*insert|send_notification" rust/cloud-storage/notification/src | head -150Repository: macro-inc/macro
Length of output: 14225
🏁 Script executed:
# Look for where event_item_id is set in channel notifications
rg -nP --type=rust -C8 "event_item_id.*channel_id|channel_id.*event_item_id"Repository: macro-inc/macro
Length of output: 2079
🏁 Script executed:
# Find the dispatcher that creates notifications
fd -type f -name "*.rs" rust/cloud-storage/notification*/src | xargs rg -l "dispatch_notifications_for"Repository: macro-inc/macro
Length of output: 435
🏁 Script executed:
cat -n rust/cloud-storage/comms_service/src/notification.rs | head -100Repository: macro-inc/macro
Length of output: 5041
🏁 Script executed:
# Search for where the notification builder sets event_item_id for channel type
rg -nP --type=rust -C10 "event_item_id|SendNotificationRequestBuilder" rust/cloud-storage/comms_service/src/notification.rsRepository: macro-inc/macro
Length of output: 7504
Align the backfill predicate with the trigger to prevent inconsistent cleanup.
The trigger (lines 10-12) deletes channel-type notifications matching three conditions:
event_item_type = 'channel'event_item_id = NEW.channel_id::text(the specific channel)metadata->>'messageId' = NEW.id::text
The backfill (lines 25-31) omits the event_item_id filter and will delete notifications for any channel that has a messageId pointing to an already-soft-deleted message. Going forward, the trigger will only clean up notifications for the specific channel of the deleted message, creating a divergence.
Confirm that the backfill should include an event_item_id check (e.g., AND n.event_item_id = m.channel_id::text after joining to comms_messages m), or document why the backfill intentionally cleans up notifications across all channels.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@rust/cloud-storage/macro_db_client/migrations/20260423120000_cascade_comms_message_delete_to_notifications.sql`
around lines 8 - 31, The backfill DELETE currently removes any notification with
a metadata->>'messageId' pointing to a deleted comms_messages row but omits the
same event_item_id check used by the trigger
cascade_comms_message_delete_to_notifications; update the backfill to include a
join to comms_messages (alias m) and add a predicate matching n.event_item_id =
m.channel_id::text (and keep event_item_type = 'channel' and the m.deleted_at IS
NOT NULL check) so the backfill mirrors the trigger's three conditions
(event_item_type, event_item_id, metadata->>'messageId').
Comment on lines
+18
to
+21
| CREATE TRIGGER trg_cascade_comms_message_delete_to_notifications | ||
| AFTER UPDATE OF deleted_at ON comms_messages | ||
| FOR EACH ROW | ||
| EXECUTE FUNCTION cascade_comms_message_delete_to_notifications(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does PostgreSQL support CREATE OR REPLACE TRIGGER and in which version was it introduced?
💡 Result:
Yes, PostgreSQL supports CREATE OR REPLACE TRIGGER. It was introduced in PostgreSQL 14.
Citations:
- 1: https://www.postgresql.org/docs/current/sql-createtrigger.html
- 2: https://stackoverflow.com/questions/35927365/create-or-replace-trigger-postgres
- 3: https://git.postgresql.org/pg/commitdiff/92bf7e2d027466d750b4ac5b026f6f4ac29be881
- 4: https://www.postgresql.org/docs/release/14/
🏁 Script executed:
# Check PostgreSQL version target in project config/docker/rust setup files
fd -e toml -e yml -e yaml -e Dockerfile | head -20 | xargs grep -l -i postgres 2>/dev/null | head -5Repository: macro-inc/macro
Length of output: 106
🏁 Script executed:
# Read the migration file to confirm the trigger and backfill WHERE clauses
cat -n "rust/cloud-storage/macro_db_client/migrations/20260423120000_cascade_comms_message_delete_to_notifications.sql"Repository: macro-inc/macro
Length of output: 1383
🏁 Script executed:
# Search for notification table schema and indexes
find rust/cloud-storage/macro_db_client -name "*.sql" -type f | xargs rg -A 20 "CREATE TABLE.*notification" 2>/dev/null | head -80Repository: macro-inc/macro
Length of output: 11228
🏁 Script executed:
# Check PostgreSQL version in docker-compose files
grep -i "postgres" docker-compose*.yml | grep -i imageRepository: macro-inc/macro
Length of output: 110
🏁 Script executed:
# Search for all indexes on notification table
rg "CREATE INDEX.*notification" rust/cloud-storage/macro_db_client/migrations/ -A 3Repository: macro-inc/macro
Length of output: 4588
Fix non-idempotent trigger creation and correct WHERE clause inconsistency between trigger and backfill.
Two issues:
- Non-idempotent trigger (lines 18–21):
CREATE TRIGGERwithoutIF NOT EXISTSwill fail on re-run. Since the project targets PostgreSQL 16, useCREATE OR REPLACE TRIGGERinstead:
Idempotency fix
-CREATE TRIGGER trg_cascade_comms_message_delete_to_notifications
+CREATE OR REPLACE TRIGGER trg_cascade_comms_message_delete_to_notifications
AFTER UPDATE OF deleted_at ON comms_messages
FOR EACH ROW
EXECUTE FUNCTION cascade_comms_message_delete_to_notifications();- Semantic mismatch between trigger and backfill (lines 8–31): The trigger filters by
event_item_id = NEW.channel_id::text(line 11), but the backfill omits this check entirely (lines 24–31). If any channel notification exists withevent_item_id ≠ channel_id, the backfill will delete it while the trigger will not. This creates data inconsistency going forward. The backfill should include the same event_item_id filter as the trigger, or document why they intentionally differ.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@rust/cloud-storage/macro_db_client/migrations/20260423120000_cascade_comms_message_delete_to_notifications.sql`
around lines 18 - 21, Make the trigger creation idempotent and align the
backfill WHERE clause with the trigger: change the trigger DDL for
trg_cascade_comms_message_delete_to_notifications to use the PostgreSQL 16
idempotent form (CREATE OR REPLACE TRIGGER) instead of plain CREATE TRIGGER, and
update the backfill DELETE/UPDATE statement to include the same event_item_id
filter used by the trigger (the event_item_id = channel_id check — mirroring the
trigger's event_item_id = NEW.channel_id::text logic) so the backfill only
touches rows the trigger would handle; ensure you still call the same
cascade_comms_message_delete_to_notifications() function from the trigger.
seanaye
force-pushed
the
seanaye/feat/trigger-notif-deletes-from-comms
branch
from
9328684 to
70ccc4f
Compare
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
1 participant
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.