Skip to content

feat: add mute.toggle RPC and rename DisableNotification field#217

Merged
GITMateuszCharczuk merged 33 commits into
mainfrom
claude/explore-room-service-7tNlq
May 22, 2026
Merged

feat: add mute.toggle RPC and rename DisableNotification field#217
GITMateuszCharczuk merged 33 commits into
mainfrom
claude/explore-room-service-7tNlq

Conversation

@GITMateuszCharczuk
Copy link
Copy Markdown
Collaborator

@GITMateuszCharczuk GITMateuszCharczuk commented May 22, 2026

Summary

Adds a new per-user mute.toggle RPC to room-service that atomically flips Subscription.DisableNotifications for the requester in a single room. The toggle is mirrored across sites via inbox-worker using the outbox/inbox pattern, and the field is renamed from DisableNotification (singular) to DisableNotifications (plural) throughout the codebase.

Key Changes

  • Field rename: Subscription.DisableNotificationDisableNotifications across all model references, tests, and integration tests
  • New RPC handler: room-service implements handleMuteToggle with atomic Mongo FindOneAndUpdate via aggregation pipeline, publishes SubscriptionUpdateEvent to chat.user.{account}.event.subscription.update for client UI fan-out
  • Cross-site federation: When user's home site differs from room's site, publishes subscription_mute_toggled outbox event so inbox-worker mirrors the change on the user's home site
  • Subject builders: Added MuteToggle and MuteToggleWildcard to pkg/subject for RPC routing
  • Event types: New MuteToggleResponse, SubscriptionMuteToggledEvent, and OutboxSubscriptionMuteToggled const in pkg/model
  • Store interface: Added ToggleSubscriptionMute method to RoomStore interface with Mongo implementation using atomic aggregation pipeline
  • Handler wiring: Injected publishCore closure alongside existing publishToStream to support core-NATS publish for subscription update events
  • Inbox-worker: Added handleSubscriptionMuteToggled dispatch case and UpdateSubscriptionMute store method to mirror toggles on user's home site
  • API documentation: Updated docs/client-api.md with full RPC specification, request/response format, error cases, and cross-site behavior

Implementation Details

  • Follows the message.read pattern (Pattern B): inline atomic write in room-service, cross-site outbox publish, inbox-worker mirror
  • Uses MongoDB aggregation pipeline $not + $ifNull to atomically flip the boolean, treating missing field as false for legacy documents
  • No canonical event, no room-worker involvement, no key rotation
  • Field rename does not require data migration (local dev databases recreate on docker-compose up; future persistent environments can run one-shot updateMany)
  • Includes comprehensive unit tests (table-driven), integration test for store method, and handler tests for success and error paths

https://claude.ai/code/session_01JMBBydPH1indHnCAQP86Te

Summary by CodeRabbit

Release Notes

  • New Features

    • Added a room mute toggle feature that allows users to toggle notifications per room. Successful requests return the updated mute status and trigger subscription update events for UI synchronization across sites.
  • Documentation

    • Documented the new mute toggle RPC including request/response contracts and notification synchronization behavior in client API documentation.

Review Change Stack

claude added 27 commits May 21, 2026 10:06
Plan for new per-user mute.toggle RPC in room-service following the
message.read inline-write pattern, plus cross-site outbox/inbox sync
mirroring subscription_read. Also renames Subscription.DisableNotification
to DisableNotifications across production code.

https://claude.ai/code/session_01JMBBydPH1indHnCAQP86Te
No prod or staging exists yet, so the disableNotification →
disableNotifications field rename does not need a Mongo migration step
inside the plan. Local dev DBs are recreated on docker-compose up.

https://claude.ai/code/session_01JMBBydPH1indHnCAQP86Te
Uses aggregation-pipeline FindOneAndUpdate with \$ifNull guard so legacy
documents (missing disableNotifications field) toggle to true on first call.
Also regenerates mock and fixes goimports alignment in subscription struct
and dependent test files after the DisableNotifications rename.

https://claude.ai/code/session_01JMBBydPH1indHnCAQP86Te
- Refresh SubscriptionUpdateEvent.Action JSON contract comment to list all
  current values (added | removed | role_updated | mute_toggled).
- Add room.id / site.id span attributes to handleMuteToggle so traces match
  the instrumentation peer handlers already emit.

https://claude.ai/code/session_01JMBBydPH1indHnCAQP86Te
The mute.toggle handler returns "invalid mute-toggle subject: <subj>" on
parse failure, and docs/client-api.md documents that exact string. Without
this entry the reply path collapsed it to "internal error", silently
breaking the API contract just-shipped in this PR.

https://claude.ai/code/session_01JMBBydPH1indHnCAQP86Te
handleMuteToggle previously swallowed GetUserSiteID errors and returned
"ok" while silently dropping the cross-site outbox publish, leaving the
user's home site permanently out of sync with the room's site on any
transient DB hiccup. Mirror handleMessageRead's precedent and propagate
the error as a hard failure so the client can retry.

https://claude.ai/code/session_01JMBBydPH1indHnCAQP86Te
…ip sub

ToggleSubscriptionMute now returns the full post-flip *Subscription instead
of just the bool, so the handler no longer needs a separate GetSubscription
call to populate the SubscriptionUpdateEvent payload. Halves the per-request
Mongo round-trips and closes the TOCTOU window between the two reads; the
previously-dead ErrSubscriptionNotFound check after the toggle becomes the
sole membership gate, which is also more honest about what the atomic write
already guarantees. Also fixes pre-existing NewHandler arity errors in
integration_test.go caused by the publishCore parameter added in a prior commit.

https://claude.ai/code/session_01JMBBydPH1indHnCAQP86Te
…oggle

The hard-error path at the end of handleMuteToggle (publish mute-toggled
outbox: %w) was the last untested branch in the handler. With this test the
six TestHandler_MuteToggle_* tests cover every error path (parse, store,
GetUserSiteID, outbox publish) plus the same-site and cross-site happy paths.

https://claude.ai/code/session_01JMBBydPH1indHnCAQP86Te
Delete four WHAT-style step labels in the integration test and shorten
seven godoc/internal comments to one or two lines without losing the
WHY content (atomicity guarantee, no-op-for-federation-race rationale).
Final 1-line ratio across this branch's comment surface is 9/11 = 82%.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 22, 2026

📝 Walkthrough

Walkthrough

This PR implements a per-user room mute toggle feature with cross-site federation. It introduces a new mute.toggle RPC, renames Subscription.DisableNotification to Muted, adds atomic store operations, wires a handler to process toggle requests with local and cross-site event publishing, and mirrors remote mute changes via inbox-worker.

Changes

Mute Toggle Feature Implementation

Layer / File(s) Summary
Data model and event contracts
pkg/model/subscription.go, pkg/model/event.go, pkg/model/model_test.go
Subscription field DisableNotification renamed to Muted (JSON/BSON: muted). New types: MuteToggleResponse (status, muted), SubscriptionMuteToggledEvent (account, roomId, muted, timestamp), and OutboxSubscriptionMuteToggled constant. Tests validate JSON round-trips and wire formats.
Subject builders and client API contract
pkg/subject/subject.go, pkg/subject/subject_test.go, docs/client-api.md
New functions MuteToggle() and MuteToggleWildcard() generate NATS subjects for chat.user.{account}.request.room.{roomID}.{siteID}.mute.toggle. Client API documentation describes RPC behavior (atomic toggle, MuteToggleResponse reply, subscription.update event), cross-site outbox/inbox mirroring, and notes that notification-worker does not yet enforce muted state.
Room store: atomic toggle and mocks
room-service/store.go, room-service/store_mongo.go, room-service/mock_store_test.go, room-service/integration_test.go
RoomStore interface adds ToggleSubscriptionMute(ctx, roomID, account) returning post-flip subscription. MongoStore implements via atomic FindOneAndUpdate with negation pipeline; returns ErrSubscriptionNotFound when no subscription matches. GoMock regenerated. Integration test verifies toggle persistence and error handling.
Room handler: mute toggle request flow and publishing
room-service/handler.go, room-service/handler_test.go, room-service/helper.go, room-service/helper_test.go, room-service/main.go, room-service/integration_test.go
Handler gains publishCore field for local event publishing; NewHandler accepts both JetStream and core-NATS publish functions. RegisterCRUD subscribes to MuteToggleWildcard; handleMuteToggle parses subject, toggles subscription mute, publishes local subscription update via core NATS (non-fatal), conditionally publishes cross-site outbox event via JetStream. Response includes updated Muted value. sanitizeError allowlists "invalid mute-toggle subject" errors. Comprehensive tests cover success, cross-site publishing, membership gating, invalid subject, store error, and non-fatal core publish failures. Integration and unit tests updated to new constructor signature across multiple test cases.
Inbox worker: cross-site mute state mirroring
inbox-worker/handler.go, inbox-worker/handler_test.go, inbox-worker/main.go
InboxStore interface adds UpdateSubscriptionMute(ctx, roomID, account, muted). Handler.HandleEvent routes subscription_mute_toggled outbox events to handleSubscriptionMuteToggled, which unmarshals payload and calls store to update muted state. MongoStore uses UpdateOne to set muted field; silent no-op when subscription missing. Tests verify success, missing subscription no-op, and malformed payload error.
Implementation plan, documentation, and cross-service tests
docs/superpowers/plans/2026-05-21-room-service-mute-toggle.md, room-worker/integration_test.go
Comprehensive implementation plan document detailing all tasks (field rename, subject builders, models, store, handler, inbox-worker, docs, verification). Room-worker integration tests updated to use Muted field in subscription seeding and assertions for BotDM and regular-DM redelivery scenarios, confirming mute state is not refreshed on message redelivery.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

  • hmchangw/chat#202: Prior PR introduces the notification-disable field pattern on Subscription that this PR refactors and extends for mute-toggle support.
  • hmchangw/chat#72: Both PRs extend shared outbox event handling infrastructure in pkg/model/event.go and inbox-worker/handler.go for subscription state synchronization.

Suggested reviewers

  • mliu33

Poem

🐰 A toggle hops 'cross sites so wide,
Muting bells where chats collide,
NATS and outbox dance in time,
Subscriptions sync—a feature sublime! 📬✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.31% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately summarizes the main change: adding a mute.toggle RPC and renaming the DisableNotification field, which are the core objectives.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/explore-room-service-7tNlq

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (2)
pkg/subject/subject.go (1)

365-367: ⚡ Quick win

Apply the shared account-token guard in MuteToggle.

MuteToggle accepts account but currently skips isValidAccountToken, so wildcard-containing values can slip through unlike the guarded subject builders.

♻️ Suggested patch
 func MuteToggle(account, roomID, siteID string) string {
+	if !isValidAccountToken(account) {
+		panic("invalid account token: contains NATS wildcard characters")
+	}
 	return fmt.Sprintf("chat.user.%s.request.room.%s.%s.mute.toggle", account, roomID, siteID)
 }

Based on learnings: “For Go subject builder/parser code under pkg/subject, add a single shared guard/validator for any function that accepts an account token to prevent NATS subject special-character issues.”

🤖 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 `@pkg/subject/subject.go` around lines 365 - 367, MuteToggle currently builds a
subject using the raw account token without validation; call the shared
validator isValidAccountToken(account) at the start of the MuteToggle function
and, if it fails, return an empty string (matching the pattern used by other
pkg/subject builders) instead of constructing the subject, so
wildcard/special-character account values cannot slip through.
pkg/model/subscription.go (1)

42-42: Plan a rollout backfill for the renamed persisted field.

Because this changes the stored BSON key to disableNotifications, make sure deployment includes an updateMany backfill (or equivalent) so previously muted users in existing datasets keep their mute state.

🤖 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 `@pkg/model/subscription.go` at line 42, The Subscription struct's field
DisableNotifications was renamed to use the BSON key "disableNotifications", so
add a DB migration that preserves existing users' mute state: write an
updateMany migration that finds documents missing "disableNotifications" but
having the old key (detect the previous persisted key used before this change),
runs a $set to set disableNotifications to the old key's value and a follow-up
$unset to remove the legacy key, and run this migration as part of the rollout
(with a dry-run on a subset and logs) so the Subscription.DisableNotifications
property continues to reflect prior mute state.
🤖 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-service/handler_test.go`:
- Around line 3204-3410: Add a new test verifying that a failure in publishCore
inside handleMuteToggle returns an error: create
TestHandler_MuteToggle_CorePublishFailure that mocks ToggleSubscriptionMute to
return a subscription and mocks GetUserSiteID (AnyTimes) to return the same
site, construct a Handler with publishCore returning fmt.Errorf("core nats
down") and publishToStream that calls t.Fatal if invoked, call
h.handleMuteToggle with subject.MuteToggle("alice","r1","site-a"), and assert an
error is returned and its message contains "publish subscription update";
reference ToggleSubscriptionMute, GetUserSiteID, publishCore, publishToStream,
handleMuteToggle and name the test TestHandler_MuteToggle_CorePublishFailure.

In `@room-service/handler.go`:
- Around line 1252-1305: The handler currently returns errors after successfully
calling h.store.ToggleSubscriptionMute which makes mute.toggle unsafe to retry;
change the handler so that once h.store.ToggleSubscriptionMute (and the DB
commit) succeeds you do NOT return an error to the client for subsequent
notification failures — instead log them and continue. Concretely: keep the DB
write via h.store.ToggleSubscriptionMute as the source-of-truth, make calls to
h.publishCore, h.store.GetUserSiteID and h.publishToStream non-fatal (log errors
and proceed) or move those notification steps to a background goroutine so the
handler always returns the updated subscription after ToggleSubscriptionMute
succeeds; ensure you still record/serialize the same
model.SubscriptionUpdateEvent and model.OutboxEvent but never escalate
publish/notification errors into the handler response.

In `@room-service/store_mongo.go`:
- Around line 687-689: The MongoDB `$not` expression is using an object operand
instead of an array; update the mute-toggle update that sets
"disableNotifications" to use array syntax for `$not`, i.e. replace
bson.M{"$not": bson.M{ "$ifNull": bson.A{"$disableNotifications", false}}} with
bson.M{"$not": bson.A{bson.M{"$ifNull": bson.A{"$disableNotifications",
false}}}} so the "$not" operand is a single-element array; locate the mute
toggle code that constructs the "disableNotifications" update and make this
change.

---

Nitpick comments:
In `@pkg/model/subscription.go`:
- Line 42: The Subscription struct's field DisableNotifications was renamed to
use the BSON key "disableNotifications", so add a DB migration that preserves
existing users' mute state: write an updateMany migration that finds documents
missing "disableNotifications" but having the old key (detect the previous
persisted key used before this change), runs a $set to set disableNotifications
to the old key's value and a follow-up $unset to remove the legacy key, and run
this migration as part of the rollout (with a dry-run on a subset and logs) so
the Subscription.DisableNotifications property continues to reflect prior mute
state.

In `@pkg/subject/subject.go`:
- Around line 365-367: MuteToggle currently builds a subject using the raw
account token without validation; call the shared validator
isValidAccountToken(account) at the start of the MuteToggle function and, if it
fails, return an empty string (matching the pattern used by other pkg/subject
builders) instead of constructing the subject, so wildcard/special-character
account values cannot slip through.
🪄 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: 0bbad3e0-3ef6-49a5-bfad-268d2bd17c78

📥 Commits

Reviewing files that changed from the base of the PR and between c0d023f and 7361df0.

📒 Files selected for processing (21)
  • docs/client-api.md
  • docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md
  • docs/superpowers/plans/2026-05-21-room-service-mute-toggle.md
  • inbox-worker/handler.go
  • inbox-worker/handler_test.go
  • inbox-worker/main.go
  • pkg/model/event.go
  • pkg/model/model_test.go
  • pkg/model/subscription.go
  • pkg/subject/subject.go
  • pkg/subject/subject_test.go
  • room-service/handler.go
  • room-service/handler_test.go
  • room-service/helper.go
  • room-service/helper_test.go
  • room-service/integration_test.go
  • room-service/main.go
  • room-service/mock_store_test.go
  • room-service/store.go
  • room-service/store_mongo.go
  • room-worker/integration_test.go

Comment thread room-service/handler_test.go
Comment thread room-service/handler.go
Comment thread room-service/store_mongo.go Outdated
…ervice-7tNlq

# Conflicts:
#	room-service/integration_test.go
…n test

The merge from main removed RoomStore.CreateSubscription; the new
TestMongoStore_ToggleSubscriptionMute was still calling it, breaking
the integration-test build. Switch to the mustInsertSub helper used
by the rest of the file.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
inbox-worker/handler_test.go (1)

1205-1263: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add a store-error test for subscription_mute_toggled handler coverage.

These tests cover valid input, malformed payload, and missing-subscription no-op, but they don’t cover the UpdateSubscriptionMute store-failure path. Please add a case where the store returns an error and assert HandleEvent surfaces it.

As per coding guidelines: “Every handler method must have tests for: valid input, invalid/malformed input, store errors, and edge cases.”

🤖 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 `@inbox-worker/handler_test.go` around lines 1205 - 1263, Add a new test that
exercises the store-error path for the subscription_mute_toggled handler by
having the stub inbox store return an error from its UpdateSubscriptionMute
implementation and asserting that Handler.HandleEvent returns that error;
specifically, create a test similar to TestHandler_SubscriptionMuteToggled but
initialize the stubInboxStore to include an existing subscription and configure
a field (e.g., updateSubscriptionMuteErr or equivalent) to return a non-nil
error from UpdateSubscriptionMute, marshal a model.OutboxEvent with Type
model.OutboxSubscriptionMuteToggled and the appropriate
model.SubscriptionMuteToggledEvent payload, call h.HandleEvent(ctx, evt) and
require that it returns an error, ensuring the test references NewHandler,
HandleEvent and UpdateSubscriptionMute so it fails if the error path is not
propagated.
♻️ Duplicate comments (2)
room-service/store_mongo.go (1)

812-815: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Fix $not operand shape in the mute-toggle pipeline.

$not here is built with an object operand; in aggregation expressions it must receive an array-wrapped expression. This can break FindOneAndUpdate at runtime.

🐛 Proposed fix
 	update := mongo.Pipeline{
 		bson.D{{Key: "$set", Value: bson.M{
-			"disableNotifications": bson.M{"$not": bson.M{
-				"$ifNull": bson.A{"$disableNotifications", false},
-			}},
+			"disableNotifications": bson.M{"$not": bson.A{
+				bson.M{"$ifNull": bson.A{"$disableNotifications", false}},
+			}},
 		}}},
 	}
In MongoDB aggregation expressions used in update pipelines (e.g., findOneAndUpdate), what is the required operand syntax for `$not`? Is it `{ $not: [ <expression> ] }`?
🤖 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-service/store_mongo.go` around lines 812 - 815, The aggregation update
builds a $not with an object operand which is invalid; change the $not operand
to be array-wrapped (e.g., { "$not": [ <expression> ] }) in the mute-toggle
update so the inner $ifNull expression is passed as an array element;
specifically update the bson.M for "disableNotifications" where you currently
have bson.M{"$not": bson.M{"$ifNull": bson.A{"$disableNotifications", false}}}
to use bson.M{"$not": bson.A{bson.M{"$ifNull": bson.A{"$disableNotifications",
false}}}} so the FindOneAndUpdate aggregation expression is valid.
room-service/handler.go (1)

1316-1369: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don’t fail the RPC after the mute state is already committed.

After Line 1316 commits the toggle, returning errors from Line 1341 or Line 1367 makes retries unsafe (a retry can flip the bit back).

💡 Suggested direction
 	userSiteID, err := h.store.GetUserSiteID(ctx, account)
 	if err != nil {
-		return nil, fmt.Errorf("get user siteId: %w", err)
+		slog.Error("get user siteId failed after mute toggle", "error", err, "account", account, "roomId", roomID)
+		userSiteID = ""
 	}
@@
 		if err := h.publishToStream(ctx, subject.Outbox(h.siteID, userSiteID, model.OutboxSubscriptionMuteToggled), outboxData); err != nil {
-			return nil, fmt.Errorf("publish mute-toggled outbox: %w", err)
+			slog.Error("publish mute-toggled outbox failed", "error", err, "account", account, "roomId", roomID, "destSiteId", userSiteID)
 		}
 	}
🤖 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-service/handler.go` around lines 1316 - 1369, The RPC currently returns
errors after h.store.ToggleSubscriptionMute successfully commits, which can
cause unsafe retries that flip the mute bit; change the post-commit flow so
failures are best-effort only: after ToggleSubscriptionMute succeeds, do not
propagate errors from publishCore, h.store.GetUserSiteID, json.Marshal for the
outbox payloads, or h.publishToStream — instead log those errors (e.g., using
slog.Error) and continue to return the successful subscription object to the
caller; keep attempting publishes but never return fmt.Errorf from the handler
once ToggleSubscriptionMute has succeeded (refer to ToggleSubscriptionMute,
publishCore, GetUserSiteID, and publishToStream to locate the affected code).
🧹 Nitpick comments (1)
room-service/integration_test.go (1)

1776-1814: ⚡ Quick win

Add a legacy-doc edge case for missing disableNotifications.

Please add one case inserting a raw subscription doc without the disableNotifications field and assert first toggle returns DisableNotifications=true. That directly guards the $ifNull compatibility path.

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-service/integration_test.go` around lines 1776 - 1814, Extend
TestMongoStore_ToggleSubscriptionMute to include an edge-case where a raw Mongo
document for the subscription is inserted without the disableNotifications field
(use the test DB handle used by mustInsertSub to insert a BSON map for
user/account "alice" and room "r-edge" or similar), then call
store.ToggleSubscriptionMute(ctx, "r-edge", "alice") and assert the returned
subscription has DisableNotifications == true and that persisted value (via
store.GetSubscription) is true; this ensures ToggleSubscriptionMute's $ifNull
compatibility path is covered.
🤖 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 `@inbox-worker/handler_test.go`:
- Around line 1205-1263: Add a new test that exercises the store-error path for
the subscription_mute_toggled handler by having the stub inbox store return an
error from its UpdateSubscriptionMute implementation and asserting that
Handler.HandleEvent returns that error; specifically, create a test similar to
TestHandler_SubscriptionMuteToggled but initialize the stubInboxStore to include
an existing subscription and configure a field (e.g., updateSubscriptionMuteErr
or equivalent) to return a non-nil error from UpdateSubscriptionMute, marshal a
model.OutboxEvent with Type model.OutboxSubscriptionMuteToggled and the
appropriate model.SubscriptionMuteToggledEvent payload, call h.HandleEvent(ctx,
evt) and require that it returns an error, ensuring the test references
NewHandler, HandleEvent and UpdateSubscriptionMute so it fails if the error path
is not propagated.

---

Duplicate comments:
In `@room-service/handler.go`:
- Around line 1316-1369: The RPC currently returns errors after
h.store.ToggleSubscriptionMute successfully commits, which can cause unsafe
retries that flip the mute bit; change the post-commit flow so failures are
best-effort only: after ToggleSubscriptionMute succeeds, do not propagate errors
from publishCore, h.store.GetUserSiteID, json.Marshal for the outbox payloads,
or h.publishToStream — instead log those errors (e.g., using slog.Error) and
continue to return the successful subscription object to the caller; keep
attempting publishes but never return fmt.Errorf from the handler once
ToggleSubscriptionMute has succeeded (refer to ToggleSubscriptionMute,
publishCore, GetUserSiteID, and publishToStream to locate the affected code).

In `@room-service/store_mongo.go`:
- Around line 812-815: The aggregation update builds a $not with an object
operand which is invalid; change the $not operand to be array-wrapped (e.g., {
"$not": [ <expression> ] }) in the mute-toggle update so the inner $ifNull
expression is passed as an array element; specifically update the bson.M for
"disableNotifications" where you currently have bson.M{"$not": bson.M{"$ifNull":
bson.A{"$disableNotifications", false}}} to use bson.M{"$not":
bson.A{bson.M{"$ifNull": bson.A{"$disableNotifications", false}}}} so the
FindOneAndUpdate aggregation expression is valid.

---

Nitpick comments:
In `@room-service/integration_test.go`:
- Around line 1776-1814: Extend TestMongoStore_ToggleSubscriptionMute to include
an edge-case where a raw Mongo document for the subscription is inserted without
the disableNotifications field (use the test DB handle used by mustInsertSub to
insert a BSON map for user/account "alice" and room "r-edge" or similar), then
call store.ToggleSubscriptionMute(ctx, "r-edge", "alice") and assert the
returned subscription has DisableNotifications == true and that persisted value
(via store.GetSubscription) is true; this ensures ToggleSubscriptionMute's
$ifNull compatibility path is covered.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4bbc06f2-ecac-4cbe-be28-9d9dcd77d6ef

📥 Commits

Reviewing files that changed from the base of the PR and between 7361df0 and fc9d443.

📒 Files selected for processing (11)
  • docs/client-api.md
  • inbox-worker/handler_test.go
  • pkg/model/event.go
  • pkg/model/model_test.go
  • room-service/handler.go
  • room-service/handler_test.go
  • room-service/integration_test.go
  • room-service/mock_store_test.go
  • room-service/store.go
  • room-service/store_mongo.go
  • room-worker/integration_test.go
✅ Files skipped from review due to trivial changes (1)
  • room-service/mock_store_test.go

claude added 2 commits May 22, 2026 05:49
MongoDB aggregation expression $not requires array syntax — see
https://www.mongodb.com/docs/manual/reference/operator/aggregation/not/.
Wrapping the $ifNull expression in bson.A makes the pipeline conform.
Reported by CodeRabbit on PR #217.
CodeRabbit (PR #217) flagged that publishCore failure was untested. The
intended behaviour is non-fatal — DB write is the source of truth and
clients reconcile on next refetch (documented inline at handler.go:1277).
Add a test that asserts the handler still replies ok when publishCore
returns an error, so a future refactor can't silently flip the contract.
Comment thread room-service/handler.go
Comment thread room-service/handler.go
Comment thread room-service/integration_test.go
Comment thread room-service/main.go
Comment thread docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md Outdated
claude added 2 commits May 22, 2026 07:15
Per PM, the field is now called Muted. JSON/BSON tags become "muted"
and the outbox payload + response field align. Mongo data on existing
sites would need a one-shot updateMany rename — operational task, no
prod yet so no migration in this PR.
The maintainer flagged that the multi-lens review file shouldn't ship
to main. It was a working artifact for the fix-up cycle; the conclusions
are captured in the implementation + PR review threads.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
room-service/integration_test.go (1)

1776-1814: ⚡ Quick win

Add a legacy-document case for missing muted field.

This test validates explicit Muted: false, but it doesn’t exercise the contract that missing muted is treated as false then toggled to true. Add a subtest that inserts a subscription document without muted and asserts first toggle returns/persists Muted=true.

💡 Suggested test addition
 func TestMongoStore_ToggleSubscriptionMute(t *testing.T) {
@@
 	gotNil, err := store.ToggleSubscriptionMute(ctx, "missing", "alice")
 	assert.Nil(t, gotNil)
 	assert.ErrorIs(t, err, model.ErrSubscriptionNotFound)
+
+	t.Run("missing muted field defaults to false then toggles to true", func(t *testing.T) {
+		_, err := db.Collection("subscriptions").InsertOne(ctx, bson.M{
+			"_id":      idgen.GenerateUUIDv7(),
+			"roomId":   "r2",
+			"roomType": model.RoomTypeChannel,
+			"siteId":   "site-a",
+			"u": bson.M{
+				"_id":     "u2",
+				"account": "bob",
+			},
+			"roles":    []model.Role{model.RoleMember},
+			"joinedAt": time.Now().UTC(),
+			// intentionally omit "muted"
+		})
+		require.NoError(t, err)
+
+		got, err := store.ToggleSubscriptionMute(ctx, "r2", "bob")
+		require.NoError(t, err)
+		require.NotNil(t, got)
+		assert.True(t, got.Muted)
+	})
 }

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-service/integration_test.go` around lines 1776 - 1814, Add a subtest in
TestMongoStore_ToggleSubscriptionMute that covers the legacy case where the
Subscription document has no muted field: insert a subscription document for
user "alice" / room "r1" without setting the Muted field (use the test
DB/collection directly or adapt mustInsertSub to allow omitting Muted), then
call store.ToggleSubscriptionMute(ctx, "r1", "alice") and assert the returned
Subscription has Muted == true and that a subsequent GetSubscription persists
Muted == true; this verifies ToggleSubscriptionMute treats a missing muted field
as false and toggles it to true.
🤖 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.

Nitpick comments:
In `@room-service/integration_test.go`:
- Around line 1776-1814: Add a subtest in TestMongoStore_ToggleSubscriptionMute
that covers the legacy case where the Subscription document has no muted field:
insert a subscription document for user "alice" / room "r1" without setting the
Muted field (use the test DB/collection directly or adapt mustInsertSub to allow
omitting Muted), then call store.ToggleSubscriptionMute(ctx, "r1", "alice") and
assert the returned Subscription has Muted == true and that a subsequent
GetSubscription persists Muted == true; this verifies ToggleSubscriptionMute
treats a missing muted field as false and toggles it to true.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0e78b243-e7aa-43cd-87a3-1e5a8dc485d3

📥 Commits

Reviewing files that changed from the base of the PR and between f38e7d5 and d44c8c9.

📒 Files selected for processing (13)
  • docs/client-api.md
  • inbox-worker/handler.go
  • inbox-worker/handler_test.go
  • inbox-worker/main.go
  • pkg/model/event.go
  • pkg/model/model_test.go
  • pkg/model/subscription.go
  • room-service/handler.go
  • room-service/handler_test.go
  • room-service/integration_test.go
  • room-service/store.go
  • room-service/store_mongo.go
  • room-worker/integration_test.go

Copy link
Copy Markdown
Collaborator

@vjauhari-work vjauhari-work left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good!

@GITMateuszCharczuk GITMateuszCharczuk merged commit daf5145 into main May 22, 2026
8 checks passed
Joey0538 pushed a commit that referenced this pull request May 22, 2026
Rebased onto origin/main, which merged #217 (mute.toggle RPC) and #219
(SubscriptionUser.IsBot).

- Field rename: Subscription.DisableNotification is now Subscription.Muted
  on main. The Member projection field, the Stage-1 mute exclusion, and the
  C / bug-fix references are updated to Muted (json tag "muted").
- E2 changed from lazy to eager invalidation for v1: the worker subscribes
  (fan-out, core NATS) to SubscriptionUpdate — room-service and room-worker
  publish a SubscriptionUpdateEvent there on every membership change (add /
  remove / role / mute toggle), and cross-site changes resurface locally via
  inbox-worker -> room-worker. The handler calls roomsubcache.Invalidate;
  the 5-minute TTL stays as the backstop. The eager-invalidation Future-work
  entry is removed (now v1 scope).

https://claude.ai/code/session_015TKwuwNPZFjGezj4zuzQPS
@mliu33
Copy link
Copy Markdown
Collaborator

mliu33 commented May 22, 2026

Excellent, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants