feat: add BotDM re-subscribe upsert with DisableNotification field#202
Merged
Conversation
… field Adds the design doc for fixing the botDM mute/re-activation no-op: when a muted botDM is "re-opened", the subscription must refresh DisableNotification, IsSubscribed, and JoinedAt instead of silently swallowing the duplicate-key insert. Spec scopes the change to a new BulkUpsertSubscriptions store method used only on botDM paths; channel, regular-DM, and add-member paths retain insert-only semantics. The notification-worker filter and user-facing mute toggle are explicitly deferred to follow-up PRs.
8-task TDD plan: model field, store interface + mock, MongoStore impl with integration test, two handler wiring tasks (async + sync botDM paths), end-to-end re-subscribe refresh integration test, regular-DM regression integration test, and a final verification gate.
Adds a botDM-only upsert path: on (roomId, u.account) collision, refreshes DisableNotification → false, IsSubscribed, and JoinedAt while preserving the existing document's _id and runtime state (LastSeenAt, HasMention, ThreadUnread, Alert). Existing BulkCreateSubscriptions is unchanged — channel/DM/add-member paths keep their safe insert-only contract. Integration test added but not executed locally (sandbox has no Docker); must be exercised in CI before merge.
processCreateRoom's botDM branch now upserts subscriptions so muted/ inactive botDM rooms are reactivated when the user re-opens them, and re-reads the canonical sub pair via FindDMSubscription so downstream events carry persisted _id/JoinedAt. Regular-DM branch is unchanged.
processSyncCreateDM's botDM branch now upserts subscriptions so muted/ inactive cross-site botDM rooms are reactivated on re-create. The existing post-write FindDMSubscription re-read handles the canonical sub fetch unchanged. Regular-DM branch is unchanged.
End-to-end test that processCreateRoom on a muted/inactive botDM refreshes DisableNotification/IsSubscribed/JoinedAt while preserving the existing _id and runtime fields (HasMention, Alert, LastSeenAt). Not executed locally (sandbox has no Docker); must be exercised in CI.
Locks in that processCreateRoom's regular-DM branch must NOT refresh a pre-existing subscription's DisableNotification or JoinedAt — only the botDM branch upserts. Guards against accidental upsert wiring spreading to the regular-DM path in future edits.
…psertModel - BulkUpsertSubscriptions now uses pkg/mongoutil.UpsertModel instead of hand-rolling mongo.NewUpdateOneModel().SetFilter().SetUpdate().SetUpsert. - findDMSubscriptionPair consolidates the post-write canonical-sub re-read shared by processCreateRoom's botDM upsert branch and processSyncCreateDM.
📝 WalkthroughWalkthroughAdds ChangesBotDM Re-Subscribe / Canonical Read
Sequence DiagramsequenceDiagram
participant Client
participant processCreateRoom
participant SubscriptionStore
participant MongoStore
participant findDMSubscriptionPair
participant DownstreamEvents
Client->>processCreateRoom: Create DM room (botDM)
processCreateRoom->>SubscriptionStore: BulkCreateSubscriptions(subs)
SubscriptionStore->>MongoStore: BulkWrite upsert models (`$setOnInsert`)
MongoStore-->>SubscriptionStore: BulkWrite result
processCreateRoom->>findDMSubscriptionPair: FindDMSubscriptionPair(roomID, requester)
findDMSubscriptionPair->>MongoStore: Query subscriptions by roomID
MongoStore-->>findDMSubscriptionPair: Two persisted subscriptions (_id, JoinedAt)
findDMSubscriptionPair-->>processCreateRoom: Canonical subscription pair
processCreateRoom->>DownstreamEvents: Publish using persisted subscription state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
🧹 Nitpick comments (1)
room-worker/integration_test.go (1)
1451-1494: ⚡ Quick winStrengthen bot-side re-subscribe assertions to prove collision refresh
On Line 1451, the seeded bot subscription does not set
DisableNotification: true, so Line 1493 can pass without actually verifying mute-clear on collision. Also, bot-sideJoinedAtrefresh is not asserted. Seed a muted bot row and assert both clear +JoinedAtrefresh.Proposed test tightening
mustInsertSub(t, db, &model.Subscription{ ID: "existing-bot-sub", User: model.SubscriptionUser{ID: "u_helper_bot", Account: "helper.bot"}, RoomID: roomID, SiteID: "site-A", RoomType: model.RoomTypeBotDM, Name: "alice", IsSubscribed: false, + DisableNotification: true, JoinedAt: oldJoinedAt, }) @@ assert.Equal(t, "existing-bot-sub", botSub.ID, "_id preserved") assert.False(t, botSub.IsSubscribed, "bot side stays IsSubscribed=false") assert.False(t, botSub.DisableNotification, "bot side DisableNotification cleared (idempotent no-op)") + assert.False(t, botSub.JoinedAt.Equal(oldJoinedAt), "bot side JoinedAt refreshed")As per coding guidelines: "Tests must cover happy path, error paths, edge cases (empty collections, boundary conditions), and invalid input."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@room-worker/integration_test.go` around lines 1451 - 1494, The seeded bot subscription used for the collision test (ID "existing-bot-sub", user "helper.bot") should be created as muted and with an old JoinedAt so we can assert the collision refresh: when seeding via mustInsertSub set DisableNotification: true and a known oldJoinedAt, then after calling h.processCreateRoom(ctx, body) fetch the bot subscription with store.GetSubscription("helper.bot", roomID) and assert ID stays "existing-bot-sub", IsSubscribed remains false, DisableNotification is now false (mute cleared), and JoinedAt is not equal to the oldJoinedAt (i.e., was refreshed). Ensure these checks sit alongside the existing human assertions so processCreateRoom collision-refresh behavior is fully verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@room-worker/handler.go`:
- Around line 1591-1604: The current code reuses the room's CreatedAt
(existing.CreatedAt set to acceptedAt) when doing botDM subscription upserts,
causing JoinedAt to be overwritten with the stale room creation time; to fix,
keep room creation time and subscription join time separate: when handling the
duplicate-room path do not assign existing.CreatedAt into the variable used for
subscription JoinedAt for botDM upserts—use a fresh timestamp (e.g.,
time.Now()/acceptedAtOriginal) when calling buildDMSubs and the
BulkUpsertSubscriptions/BulkCreateSubscriptions for botDM, then after re-reading
the canonical subs with findDMSubscriptionPair pass requesterSub.JoinedAt (not
the room creation time) into publishSyncDMOutbox so the outbox and persisted
JoinedAt reflect the fresh re-subscribe time.
---
Nitpick comments:
In `@room-worker/integration_test.go`:
- Around line 1451-1494: The seeded bot subscription used for the collision test
(ID "existing-bot-sub", user "helper.bot") should be created as muted and with
an old JoinedAt so we can assert the collision refresh: when seeding via
mustInsertSub set DisableNotification: true and a known oldJoinedAt, then after
calling h.processCreateRoom(ctx, body) fetch the bot subscription with
store.GetSubscription("helper.bot", roomID) and assert ID stays
"existing-bot-sub", IsSubscribed remains false, DisableNotification is now false
(mute cleared), and JoinedAt is not equal to the oldJoinedAt (i.e., was
refreshed). Ensure these checks sit alongside the existing human assertions so
processCreateRoom collision-refresh behavior is fully verified.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9933e10a-e069-427a-8b39-aaab29886d3c
📒 Files selected for processing (10)
docs/superpowers/plans/2026-05-19-botdm-resubscribe-upsert.mddocs/superpowers/specs/2026-05-19-botdm-resubscribe-upsert-design.mdpkg/model/model_test.gopkg/model/subscription.goroom-worker/handler.goroom-worker/handler_test.goroom-worker/integration_test.goroom-worker/mock_store_test.goroom-worker/store.goroom-worker/store_mongo.go
The sync DM path reset acceptedAt = existing.CreatedAt on a CreateRoom dup-key to preserve regular-DM idempotency. That same reset defeated the botDM upsert's refresh-JoinedAt intent: the upsert's \$set joinedAt was writing back the original room creation time instead of a fresh value. Split acceptedAt into roomCreatedAt (for the room doc + outbox dedup seed, which must stay stable across retries) and joinedAt (for the sub document). Only reset joinedAt = existing.CreatedAt on the regular-DM branch; the botDM branch keeps joinedAt fresh so the upsert genuinely refreshes the persisted JoinedAt. The outbox event's JoinedAt now comes from the canonical post-write re-read (requesterSub.JoinedAt) so cross-site receivers see the refreshed value. The outbox dedup seed migrates to room.CreatedAt — also stable across retries — so dedup behaviour is preserved for the regular-DM path while allowing the botDM joinedAt to vary. Resolves CodeRabbit comment on PR #202.
mliu33
reviewed
May 19, 2026
| } | ||
| if err := h.store.BulkCreateSubscriptions(ctx, subs); err != nil { | ||
| return fmt.Errorf("bulk create subs: %w", err) | ||
| if err := h.store.BulkCreateSubscriptions(ctx, subs); err != nil { |
Collaborator
There was a problem hiding this comment.
I think we should apply bulkUpsert for both bot dm and user dm because room-worker might also run into duplicate key issue during reprocess due to Jetstream msg redelivery
mliu33
reviewed
May 20, 2026
| } | ||
| models = append(models, mongoutil.UpsertModel(filter, update)) | ||
| } | ||
| opts := options.BulkWrite().SetOrdered(false) |
Collaborator
There was a problem hiding this comment.
check inbox-worker blkwrite method. Do we need to set "SetUpsert(true)" for bulkwrite ?
Collaborator
Author
There was a problem hiding this comment.
yes John, mongoutil.UspertModel already use SetUpsert(true) -
Line 21 in d4bb50c
…ulkCreate Addresses review feedback on the botDM re-subscribe PR: - FindDMSubscriptionPair: new store method that fetches both subs of a DM/botDM room in a single Find call, replacing the two sequential FindDMSubscription round-trips behind findDMSubscriptionPair. - BulkCreateSubscriptions (Mongo impl): switch from InsertMany + IsDuplicateKeyError-swallow to BulkWrite + UpdateOneModel + $setOnInsert + Upsert(true). Behaviour is preserved (no field refresh on collision, insert-only contract intact) but the dup-key error path is eliminated entirely, making JetStream redelivery safety explicit in the idiom rather than via silent error swallowing. Mocks regenerated; unit tests updated to expect FindDMSubscriptionPair. https://claude.ai/code/session_01QYsemwJYF25SrLjYxExWxw
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
room-worker/handler.go (1)
1184-1189:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-read canonical DM subscriptions after the idempotent bulk create.
BulkCreateSubscriptionsnow preserves the existing Mongo rows on(roomId, u.account)collisions, but this branch still sends the freshly-built in-memory subs intofinishCreateRoom. On a JetStream redelivery / duplicate-room replay,subscription.updatecan therefore fan out_idvalues that were never persisted. Mirror the botDM branch and replacesubswithfindDMSubscriptionPair(...)before publishing.Suggested fix
} else { subs = buildDMSubs(requester, counterpart, room, acceptedAt) if err := h.store.BulkCreateSubscriptions(ctx, subs); err != nil { return fmt.Errorf("bulk create subs: %w", err) } + requesterSub, counterpartSub, err := h.findDMSubscriptionPair(ctx, room.ID, requester.Account) + if err != nil { + return fmt.Errorf("re-read DM subs after bulk create: %w", err) + } + subs = []*model.Subscription{requesterSub, counterpartSub} }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@room-worker/handler.go` around lines 1184 - 1189, The branch that calls buildDMSubs then BulkCreateSubscriptions still passes the freshly-built in-memory subs into finishCreateRoom, which can publish subscription._id values that weren't persisted on redelivery; after BulkCreateSubscriptions succeeds replace the in-memory subs with the canonical rows returned by findDMSubscriptionPair (mirror the botDM branch) and pass those retrieved subscriptions into finishCreateRoom so published subscription.update events use persisted _id values; update the else branch around buildDMSubs, BulkCreateSubscriptions, and finishCreateRoom to call findDMSubscriptionPair(ctx, requester, counterpart, room) and use its result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@room-worker/handler.go`:
- Around line 1184-1189: The branch that calls buildDMSubs then
BulkCreateSubscriptions still passes the freshly-built in-memory subs into
finishCreateRoom, which can publish subscription._id values that weren't
persisted on redelivery; after BulkCreateSubscriptions succeeds replace the
in-memory subs with the canonical rows returned by findDMSubscriptionPair
(mirror the botDM branch) and pass those retrieved subscriptions into
finishCreateRoom so published subscription.update events use persisted _id
values; update the else branch around buildDMSubs, BulkCreateSubscriptions, and
finishCreateRoom to call findDMSubscriptionPair(ctx, requester, counterpart,
room) and use its result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f86a1318-4ea7-4cd7-b827-b97910537c18
📒 Files selected for processing (5)
room-worker/handler.goroom-worker/handler_test.goroom-worker/mock_store_test.goroom-worker/store.goroom-worker/store_mongo.go
✅ Files skipped from review due to trivial changes (1)
- room-worker/mock_store_test.go
Repository owner
deleted a comment from
mliu33
May 20, 2026
Reflects the d4bb50c refactor in the design spec and plan: - Spec §3a: BulkCreateSubscriptions is now a $setOnInsert upsert (insert-only contract preserved, dup-key error path eliminated). - Spec §3b: FindDMSubscriptionPair returns both DM/botDM subs in a single Mongo Find, replacing two sequential FindDMSubscription calls. - Spec §5: re-read path now uses the single-query helper. - Plan: addendum with the two follow-up tasks and verification log. https://claude.ai/code/session_01QYsemwJYF25SrLjYxExWxw
mliu33
reviewed
May 20, 2026
| // stable across NATS retries). joinedAt drives the subscription's JoinedAt | ||
| // field — for regular DM it tracks the room's original creation time on a | ||
| // dup-key retry (idempotency), but for botDM it stays fresh so the upsert | ||
| // actually refreshes the sub's JoinedAt on re-subscribe. |
Collaborator
There was a problem hiding this comment.
Because user-service will not call createRoomSync if subscriptions already exist, the upsert logic is just for handling Jetstream event redelivery. In this case, we don't need to update the sub's JoinedAt
mliu33
reviewed
May 20, 2026
| room = existing | ||
| acceptedAt = existing.CreatedAt | ||
| if req.RoomType == model.RoomTypeDM { | ||
| joinedAt = existing.CreatedAt |
Collaborator
There was a problem hiding this comment.
we should apply this logic to both DM and botDM.
| } | ||
|
|
||
| // FindDMSubscription returns the requester's dm/botDM sub by Name; ErrSubscriptionNotFound on miss. | ||
| func (s *MongoStore) FindDMSubscription(ctx context.Context, account, targetName string) (*model.Subscription, error) { |
Collaborator
There was a problem hiding this comment.
Since we are using FindDMSubscriptionPair, we can delete this method as it's no longer being used in room-worker.
| "roomType": bson.M{"$in": []model.RoomType{model.RoomTypeDM, model.RoomTypeBotDM}}, | ||
| }) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("find DM subscription pair for room %q: %w", roomID, err) |
Collaborator
There was a problem hiding this comment.
There is another wrapper for error in handler.go as well. We can wrap the error only one time
| @@ -47,6 +56,12 @@ type SubscriptionStore interface { | |||
| GetUser(ctx context.Context, account string) (*model.User, error) | |||
| // FindDMSubscription returns the requester's dm/botDM sub by Name; ErrSubscriptionNotFound on miss. | |||
| FindDMSubscription(ctx context.Context, account, targetName string) (*model.Subscription, error) | |||
Collaborator
There was a problem hiding this comment.
Remove if not being used
mliu33
reviewed
May 20, 2026
| // identity (_id, u, roomId, siteId, roomType, name, roles) plus | ||
| // hasMention/alert zero values. Used exclusively by botDM creation | ||
| // paths — see store.go for the interface comment. | ||
| func (s *MongoStore) BulkUpsertSubscriptions(ctx context.Context, subs []*model.Subscription) error { |
Collaborator
There was a problem hiding this comment.
user-service will handle the resubscribe scenario by "SetUserAppSubscribe". As a result, we don't need this anymore. Both DM and botDM should just apply BulkCreateSubscriptions to handle Jetstream redlivery
mliu33
reviewed
May 20, 2026
| if err := cursor.All(ctx, &subs); err != nil { | ||
| return nil, nil, fmt.Errorf("decode DM subscription pair for room %q: %w", roomID, err) | ||
| } | ||
| if len(subs) < 2 { |
Collaborator
There was a problem hiding this comment.
make the check != 2 just to be robust in case
mliu33
reviewed
May 20, 2026
| } | ||
| // Upsert may have hit an existing row; re-read so finishCreateRoom's | ||
| // fan-out carries persisted _id/JoinedAt. | ||
| requesterSub, counterpartSub, err := h.findDMSubscriptionPair(ctx, room.ID, requester.Account) |
Collaborator
There was a problem hiding this comment.
Both DM and botDM can reuse the same "BulkCreateSubscriptions" (explained in comments below)
…wns re-subscribe Addresses second review round on PR #202. user-service will own the re-subscribe semantic (clears DisableNotification, sets IsSubscribed via SetUserAppSubscribe) and only calls createRoomSync when no subscription exists. room-worker's job for DM and botDM is now strictly JetStream redelivery idempotency. - Remove BulkUpsertSubscriptions interface + Mongo impl entirely. - Remove FindDMSubscription — only FindDMSubscriptionPair is used now. - Async processCreateRoom DM branch: drop botDM vs. DM dispatch; both call BulkCreateSubscriptions then re-read via FindDMSubscriptionPair. - Sync handleSyncCreateDM: same simplification; joinedAt now reuses existing.CreatedAt for both DM and botDM on the dup-room path (drops the botDM carve-out). - FindDMSubscriptionPair: tighten count check to != 2 and stop double-wrapping the driver error (handler already adds context). - Inline the findDMSubscriptionPair helper (pure pass-through after the redundant wrap was removed). - Invert the botDM re-subscribe integration test into a regression guard asserting pre-seeded state is preserved on redelivery. - Delete the now-obsolete BulkUpsertSubscriptions store integration test. Spec + plan updated with a second post-review addendum. https://claude.ai/code/session_01QYsemwJYF25SrLjYxExWxw
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
room-worker/handler_test.go (1)
2018-2021: ⚡ Quick winAvoid panic-prone indexing in mocked canonical-pair return paths.
This callback indexes
capturedSubs[0/1]directly. If a regression ever produces fewer subs, the test panics instead of failing with a clear assertion.Proposed test-hardening diff
mockStore.EXPECT().FindDMSubscriptionPair(gomock.Any(), "room-dm-1", "alice"). DoAndReturn(func(_ context.Context, _, _ string) (*model.Subscription, *model.Subscription, error) { + require.Len(t, capturedSubs, 2, "expected exactly 2 subscriptions before canonical pair read") return capturedSubs[0], capturedSubs[1], nil })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@room-worker/handler_test.go` around lines 2018 - 2021, The mock callback for mockStore.EXPECT().FindDMSubscriptionPair directly indexes capturedSubs[0] and [1], which can panic if capturedSubs has fewer entries; change the DoAndReturn closure to first check len(capturedSubs) >= 2 and if not return a clear error (e.g., nil, nil, fmt.Errorf("expected >=2 capturedSubs but got %d", len(capturedSubs))) or invoke the test failure helper, otherwise return capturedSubs[0], capturedSubs[1], nil so the test fails cleanly instead of panicking.room-worker/integration_test.go (1)
1377-1388: ⚡ Quick winStrengthen insert-only regression by asserting counterpart subscription invariants too.
Both tests currently verify only the requester-side subscription. A one-sided refresh bug on the counterpart row would slip through.
Suggested assertion extension
got, err := store.GetSubscription(ctx, "alice", roomID) require.NoError(t, err) ... assert.True(t, got.JoinedAt.Equal(oldJoinedAt), ...) +gotOther, err := store.GetSubscription(ctx, "helper.bot", roomID) // use "bob" in regular-DM test +require.NoError(t, err) +assert.True(t, gotOther.JoinedAt.Equal(oldJoinedAt), "counterpart sub must also remain insert-only on redelivery") +assert.False(t, gotOther.IsSubscribed, "counterpart sub must not be refreshed")Also applies to: 1455-1460
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@room-worker/integration_test.go` around lines 1377 - 1388, Add assertions that verify the counterpart (peer) subscription row is unchanged on redelivery: after the existing check that calls store.GetSubscription(ctx, "alice", roomID) and validates DisableNotification, IsSubscribed, and JoinedAt against oldJoinedAt, also call store.GetSubscription for the counterpart user (the other participant in roomID) and assert the same three invariants (DisableNotification true, IsSubscribed false, JoinedAt equal oldJoinedAt). Keep the existing subCount check via db.Collection("subscriptions").CountDocuments(...)=2. Repeat the same counterpart assertions at the other similar block referenced (around the 1455-1460 region).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@room-worker/store_mongo.go`:
- Around line 448-452: The function FindDMSubscriptionPair returns bare errors
from the Mongo operations; update both error returns (the one after the DB query
failure and the one after cursor.All) to wrap the original err with context
using fmt.Errorf, e.g. fmt.Errorf("FindDMSubscriptionPair: query failed: %w",
err) and fmt.Errorf("FindDMSubscriptionPair: decode cursor failed: %w", err), so
callers receive operation-specific context while preserving the underlying
error.
---
Nitpick comments:
In `@room-worker/handler_test.go`:
- Around line 2018-2021: The mock callback for
mockStore.EXPECT().FindDMSubscriptionPair directly indexes capturedSubs[0] and
[1], which can panic if capturedSubs has fewer entries; change the DoAndReturn
closure to first check len(capturedSubs) >= 2 and if not return a clear error
(e.g., nil, nil, fmt.Errorf("expected >=2 capturedSubs but got %d",
len(capturedSubs))) or invoke the test failure helper, otherwise return
capturedSubs[0], capturedSubs[1], nil so the test fails cleanly instead of
panicking.
In `@room-worker/integration_test.go`:
- Around line 1377-1388: Add assertions that verify the counterpart (peer)
subscription row is unchanged on redelivery: after the existing check that calls
store.GetSubscription(ctx, "alice", roomID) and validates DisableNotification,
IsSubscribed, and JoinedAt against oldJoinedAt, also call store.GetSubscription
for the counterpart user (the other participant in roomID) and assert the same
three invariants (DisableNotification true, IsSubscribed false, JoinedAt equal
oldJoinedAt). Keep the existing subCount check via
db.Collection("subscriptions").CountDocuments(...)=2. Repeat the same
counterpart assertions at the other similar block referenced (around the
1455-1460 region).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2896d3e6-9599-4695-9ee5-4d87b949aab0
📒 Files selected for processing (8)
docs/superpowers/plans/2026-05-19-botdm-resubscribe-upsert.mddocs/superpowers/specs/2026-05-19-botdm-resubscribe-upsert-design.mdroom-worker/handler.goroom-worker/handler_test.goroom-worker/integration_test.goroom-worker/mock_store_test.goroom-worker/store.goroom-worker/store_mongo.go
💤 Files with no reviewable changes (1)
- room-worker/mock_store_test.go
Comment on lines
+448
to
+452
| return nil, nil, err | ||
| } | ||
| return &sub, nil | ||
| var subs []model.Subscription | ||
| if err := cursor.All(ctx, &subs); err != nil { | ||
| return nil, nil, err |
There was a problem hiding this comment.
Wrap FindDMSubscriptionPair errors with operation context.
At Line 448 and Line 452, returning bare err drops critical call-site context.
Proposed fix
if err != nil {
- return nil, nil, err
+ return nil, nil, fmt.Errorf("find DM subscription pair for room %q: %w", roomID, err)
}
var subs []model.Subscription
if err := cursor.All(ctx, &subs); err != nil {
- return nil, nil, err
+ return nil, nil, fmt.Errorf("decode DM subscription pair for room %q: %w", roomID, err)
}As per coding guidelines: "Always wrap errors with context using fmt.Errorf("short description: %w", err) ... Never return bare err."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@room-worker/store_mongo.go` around lines 448 - 452, The function
FindDMSubscriptionPair returns bare errors from the Mongo operations; update
both error returns (the one after the DB query failure and the one after
cursor.All) to wrap the original err with context using fmt.Errorf, e.g.
fmt.Errorf("FindDMSubscriptionPair: query failed: %w", err) and
fmt.Errorf("FindDMSubscriptionPair: decode cursor failed: %w", err), so callers
receive operation-specific context while preserving the underlying error.
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
4 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
Implements botDM re-subscription refresh semantics: when a user re-creates a botDM they previously muted or left, the subscription is now reactivated (mute cleared,
IsSubscribedrefreshed,JoinedAtupdated) while preserving runtime state (unread mentions, alerts, last-seen timestamp). AddsDisableNotificationfield tomodel.Subscriptionand introducesBulkUpsertSubscriptionsstore method for botDM-only paths; all other membership flows (channels, regular DMs, add-member) remain on safe insert-only semantics.Key Changes
Model (
pkg/model/subscription.go): AddDisableNotification boolfield alongsideAlert, matching the convention of always-present boolean flags. Noomitempty— existing Mongo records without the field decode to Go zero value (false, the desired default).Store interface & implementation (
room-worker/store.go,room-worker/store_mongo.go):BulkUpsertSubscriptionsinterface method for botDM-only upsert semanticsBulkWritewithUpdateOneModelupserts keyed on(roomId, u.account)$setrefreshes re-activation fields (disableNotification → false,isSubscribed,joinedAt); runtime fields (lastSeenAt,hasMention,threadUnread,alert) are preserved$setOnInsertinitializes identity (_id,u,roomId,siteId,roomType,name,roles) and zero-value runtime defaults (hasMention: false,alert: false)Handler wiring (
room-worker/handler.go):processCreateRoombotDM branch: switch toBulkUpsertSubscriptions, then re-read canonical sub pair viaFindDMSubscriptionso downstreamsubscription.update/MemberAddEventfan-out carries persisted_idandJoinedAtprocessSyncCreateDMbotDM branch: switch toBulkUpsertSubscriptions(existing post-writeFindDMSubscriptionre-read already handles canonical fetch)BulkCreateSubscriptions(insert-only, safe for JetStream redelivery)Tests (
room-worker/handler_test.go,room-worker/integration_test.go):BulkUpsertSubscriptions+ re-read calls on botDM pathsprocessCreateRoomBulkCreateSubscriptions(no upsert)Implementation Details
BulkUpsertSubscriptionsuses unorderedBulkWriteso partial collisions don't halt the batchprocessCreateRoombotDM path mirrors syncprocessSyncCreateDMby re-reading subs after upsert, ensuring in-memory subs handed tofinishCreateRoomcarry persisted statemake generate(no manual edits tomock_store_test.go)https://claude.ai/code/session_01QYsemwJYF25SrLjYxExWxw
Summary by CodeRabbit
New Features
Behavior Changes
Tests
Documentation