Add favorite.toggle RPC for per-user room favorites#247
Conversation
|
Warning Review limit reached
More reviews will be available in 6 minutes and 3 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThis PR implements a subscription favorite-toggle RPC feature across room-service and inbox-worker, allowing users to synchronously flip a per-room favorite boolean with automatic cross-site federation support. ChangesFavorite Toggle RPC Implementation
Sequence DiagramsequenceDiagram
participant Client
participant RoomService
participant RoomMongo as Room Mongo Store
participant CoreStream
participant OutboxStream
participant InboxWorker
participant HomeMongo as Home Mongo Store
Client->>RoomService: favorite.toggle request<br/>(account, roomID)
RoomService->>RoomMongo: ToggleSubscriptionFavorite<br/>(roomID, account)
RoomMongo-->>RoomService: *Subscription{Favorite}
RoomService->>CoreStream: publish SubscriptionUpdate<br/>(action: favorite_toggled)
RoomService->>OutboxStream: publish OutboxSubscriptionFavoriteToggled<br/>(if cross-site)
RoomService-->>Client: FavoriteToggleResponse{Favorite}
OutboxStream->>InboxWorker: SubscriptionFavoriteToggled event
InboxWorker->>HomeMongo: UpdateSubscriptionFavorite<br/>(roomID, account, favorite)
HomeMongo-->>InboxWorker: nil or silent no-op
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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: 3
🧹 Nitpick comments (1)
pkg/subject/subject_test.go (1)
366-388: ⚡ Quick winAdd invalid-account coverage for
FavoriteToggle.Please add a negative test (wildcard account
*/>) to lock in the account-token guard behavior, similar to other subject builder guard tests in this file.🤖 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_test.go` around lines 366 - 388, Add a negative test that ensures wildcard account tokens are rejected: call subject.FavoriteToggle with account "*" (and another case with account ">") and a valid room ("r1") and site ("site-a"), then pass the resulting subject into subject.ParseUserRoomSubject and assert that ok is false (and account/roomID are not treated as valid "alice"/"r1"). This mirrors other subject guard tests and locks in the account-token guard behavior for FavoriteToggle and ParseUserRoomSubject.
🤖 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 `@pkg/subject/subject.go`:
- Around line 379-381: FavoriteToggle currently interpolates account directly
and can leak wildcard tokens; add the same shared account-token guard used
elsewhere by calling isValidAccountToken(account) at the top of FavoriteToggle
and if it returns false bail out (e.g., return ""), otherwise proceed to
fmt.Sprintf as before; reference the FavoriteToggle function and
isValidAccountToken validator when making the change.
In `@room-service/handler.go`:
- Around line 1506-1559: The toggle currently commits via
h.store.ToggleSubscriptionFavorite then performs federation fanout
(GetUserSiteID, publishToStream) and returns an error if fanout fails, which can
re-trigger the toggle on retry; change to an acknowledged-after-commit pattern:
after ToggleSubscriptionFavorite succeeds, ensure outbox events are durably
recorded (e.g., insert an OutboxEvent row via a new store method like
CreateOutboxEvent or use an existing outbox writer) before attempting
publishToStream, and never return a retriable error to the client if outbox
publish fails; instead log the publish error and rely on a background worker to
read pending OutboxEvent entries and call publishToStream (or retry publish)
until success. Reference ToggleSubscriptionFavorite, GetUserSiteID,
publishToStream, model.OutboxEvent and the favorite-toggled outbox publish paths
when implementing durable outbox insertion and background retry.
- Around line 1480-1490: The error log in natsFavoriteToggle currently prints
m.Msg.Subject (leaking account/room info); change the slog.Error call to avoid
the raw subject and instead log the request ID from the ctx returned by
wrappedCtx and any safe structured fields (e.g., room_id or caller_id if already
parsed by handleFavoriteToggle) — do not include the full NATS subject. Update
natsFavoriteToggle to extract requestID from ctx (the same context used for
handleFavoriteToggle), pass that as "request_id" in slog.Error along with
sanitized fields (or omit sensitive identifiers), and keep calling
natsutil.ReplyError(sanitizeError(err)). Ensure the other respond-error logging
remains unchanged.
---
Nitpick comments:
In `@pkg/subject/subject_test.go`:
- Around line 366-388: Add a negative test that ensures wildcard account tokens
are rejected: call subject.FavoriteToggle with account "*" (and another case
with account ">") and a valid room ("r1") and site ("site-a"), then pass the
resulting subject into subject.ParseUserRoomSubject and assert that ok is false
(and account/roomID are not treated as valid "alice"/"r1"). This mirrors other
subject guard tests and locks in the account-token guard behavior for
FavoriteToggle and ParseUserRoomSubject.
🪄 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: d834a03e-9519-4e29-ac57-d890eb6511f6
📒 Files selected for processing (18)
docs/client-api.mddocs/superpowers/specs/2026-06-01-favorite-toggle-rpc-design.mdinbox-worker/handler.goinbox-worker/handler_test.goinbox-worker/main.gopkg/model/event.gopkg/model/model_test.gopkg/model/subscription.gopkg/subject/subject.gopkg/subject/subject_test.goroom-service/handler.goroom-service/handler_test.goroom-service/helper.goroom-service/helper_test.goroom-service/integration_test.goroom-service/mock_store_test.goroom-service/store.goroom-service/store_mongo.go
| func FavoriteToggle(account, roomID, siteID string) string { | ||
| return fmt.Sprintf("chat.user.%s.request.room.%s.%s.favorite.toggle", account, roomID, siteID) | ||
| } |
There was a problem hiding this comment.
Add the shared account-token guard in FavoriteToggle.
FavoriteToggle accepts account but skips isValidAccountToken, so wildcard tokens (*, >) can leak into constructed subjects. Match the same guard pattern used by other account-based builders.
Suggested patch
func FavoriteToggle(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.favorite.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… reject * and >.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func FavoriteToggle(account, roomID, siteID string) string { | |
| return fmt.Sprintf("chat.user.%s.request.room.%s.%s.favorite.toggle", account, roomID, siteID) | |
| } | |
| func FavoriteToggle(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.favorite.toggle", account, roomID, siteID) | |
| } |
🤖 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 379 - 381, FavoriteToggle currently
interpolates account directly and can leak wildcard tokens; add the same shared
account-token guard used elsewhere by calling isValidAccountToken(account) at
the top of FavoriteToggle and if it returns false bail out (e.g., return ""),
otherwise proceed to fmt.Sprintf as before; reference the FavoriteToggle
function and isValidAccountToken validator when making the change.
There was a problem hiding this comment.
Verified — the guard is missing across the entire room-request family, not just FavoriteToggle. Two adjacent conventions live side by side in pkg/subject/subject.go:
Guarded (User* family): UserStatus*, UserProfile*, UserSubscription* (all 8), MsgHistory, MsgThread
Unguarded (room-request family): MsgSend, MsgGet, MemberRoleUpdate, MemberRemove, MemberList, RoomCreate, MessageRead, MuteToggle, FavoriteToggle, SubscriptionUpdate
The parser side has the same split: ParseRoomCreateSubject rejects wildcard tokens (line 478); ParseUserRoomSubject and ParseUserRoomSiteSubject (used by every room-request handler) don't. Historical drift since PR #179, not deliberate.
Adding the guard to FavoriteToggle alone makes it the only outlier in its family without touching the real defense-in-depth surface — the unguarded parsers are where malicious inbound subjects would land, not the outbound builders that this PR ships. The right scope is a focused PR that retrofits ParseUserRoomSubject/ParseUserRoomSiteSubject and the room-family builders together. Leaving as-is here for sibling parity; tracking the family-wide cleanup separately.
Generated by Claude Code
There was a problem hiding this comment.
@general-lex, understood — the split is a family-level pattern, not an individual function gap, and patching only FavoriteToggle would leave it as the lone guarded member of an otherwise unguarded cohort. The real leverage is at ParseUserRoomSubject/ParseUserRoomSiteSubject where inbound subjects land. I'll withdraw the comment and note the design for future reviews.
Would you like me to open a GitHub issue to track the family-wide retrofit of ParseUserRoomSubject, ParseUserRoomSiteSubject, and the room-request builders?
✏️ Learnings added
Learnt from: general-lex
Repo: hmchangw/chat PR: 247
File: pkg/subject/subject.go:379-381
Timestamp: 2026-06-01T08:54:00.378Z
Learning: In hmchangw/chat `pkg/subject/subject.go`, the account-token guard (`isValidAccountToken` / panic on `*` or `>`) is intentionally present only in the **User\* family** of builders (`UserStatus*`, `UserProfile*`, `UserSubscription*` (all 8), `MsgHistory`, `MsgThread`) and absent from the entire **room-request family** (`MsgSend`, `MsgGet`, `MemberRoleUpdate`, `MemberRemove`, `MemberList`, `RoomCreate`, `MessageRead`, `MuteToggle`, `FavoriteToggle`, `SubscriptionUpdate`). This is historical drift since PR `#179`, not deliberate design. The room-request builders are left unguarded for sibling parity pending a family-wide cleanup PR that will retrofit both the builders and the corresponding parsers (`ParseUserRoomSubject`, `ParseUserRoomSiteSubject`). Do NOT flag individual room-request builders for missing the guard; flag only new builders that deviate from their immediate sibling convention.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: mliu33
Repo: hmchangw/chat PR: 55
File: pkg/subject/subject.go:16-20
Timestamp: 2026-04-08T01:06:42.136Z
Learning: 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. Specifically, reject account values containing NATS wildcard characters `*` and `>` (use one common validation function so all call sites behave consistently). Rely on the assumption that `.` is already operationally absent only if that guarantee is enforced elsewhere (e.g., by upstream validation/CI); otherwise include `.` in the guard as well.
Learnt from: Joey0538
Repo: hmchangw/chat PR: 59
File: pkg/roomkeysender/roomkeysender.go:28-30
Timestamp: 2026-04-08T08:25:32.108Z
Learning: In this repository’s Go code, follow the CLAUDE.md rule: don’t add defensive error handling, fallbacks, or nil/validation guards for conditions that are guaranteed not to occur. For internal functions/methods (e.g., when all callers are known to pass non-nil pointer parameters, such as pkg/roomkeysender/roomkeysender.go’s Send), the absence of nil-pointer guards is acceptable—do not flag it as a review issue. Reserve pointer validation and other safety checks for system boundaries (e.g., external inputs, API/HTTP requests, deserialization, or other untrusted entry points).
Learnt from: Joey0538
Repo: hmchangw/chat PR: 59
File: pkg/model/model_test.go:306-317
Timestamp: 2026-04-08T08:25:48.132Z
Learning: In Go, `time.Time` is `comparable` and therefore may be used as a type argument to generic constraints requiring `comparable` (e.g., `func roundTrip[T comparable](...)`). This is because `time.Time`’s underlying fields are all comparable (`uint64`, `int64`, and a pointer `*Location`, where pointers are comparable by address). As a result, any struct that contains or embeds a `time.Time` field (e.g., `model.Message`, or structs embedding it like `model.NotificationEvent`) should also satisfy `comparable` and should not be flagged as a compilation-error when used with `T comparable`.
Learnt from: general-lex
Repo: hmchangw/chat PR: 141
File: message-worker/main.go:95-99
Timestamp: 2026-05-04T03:30:38.678Z
Learning: In hmchangw/chat, `pkg/natsutil/connect.go`’s `natsutil.Connect` returns an OpenTelemetry-wrapped NATS connection (`*otelnats.Conn` from `github.com/Marz32onE/instrumentation-go/otel-nats/otelnats`), not a plain `*nats.Conn`. Because `otelnats.Conn.Publish` is defined as `Publish(ctx context.Context, subj string, data []byte) error` (it uses `ctx` to propagate traces via NATS headers), code review should NOT flag `nc.Publish(ctx, subj, data)` as an incorrect method call shape. This `Publish(ctx, ...)` pattern is the standard/correct usage across the codebase (e.g., message-worker, broadcast-worker, notification-worker, history-service).
Learnt from: Joey0538
Repo: hmchangw/chat PR: 182
File: docs/superpowers/specs/2026-05-14-message-edit-delete-canonical-events-design.md:172-172
Timestamp: 2026-05-18T00:31:05.210Z
Learning: In hmchangw/chat, when defining `subject.UserRoomEvent`-style chat event subject strings, use the segment order `chat.user.%s.event.room` (and wildcards `chat.user.*.event.room`) rather than `chat.user.<account>.room.event`. Suffixes under this pattern must extend in the same order, e.g. `.event.room.update`, `.event.room.key`, and `.event.room.metadata.update`. Review new/changed subject definitions to ensure they follow the established `.event.room` ordering convention.
| func (h *Handler) natsFavoriteToggle(m otelnats.Msg) { | ||
| ctx := wrappedCtx(m) | ||
| resp, err := h.handleFavoriteToggle(ctx, m.Msg.Subject, m.Msg.Data) | ||
| if err != nil { | ||
| slog.Error("favorite toggle failed", "error", err, "subject", m.Msg.Subject) | ||
| natsutil.ReplyError(m.Msg, sanitizeError(err)) | ||
| return | ||
| } | ||
| if err := m.Msg.Respond(resp); err != nil { | ||
| slog.Error("failed to respond to favorite toggle", "error", err) | ||
| } |
There was a problem hiding this comment.
Avoid logging the raw NATS subject here.
m.Msg.Subject encodes the caller account and room ID, so this new error log leaks user identifiers while still omitting the request ID that should correlate the failure. Log the request ID plus safe structured fields instead of the full subject.
🤖 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 1480 - 1490, The error log in
natsFavoriteToggle currently prints m.Msg.Subject (leaking account/room info);
change the slog.Error call to avoid the raw subject and instead log the request
ID from the ctx returned by wrappedCtx and any safe structured fields (e.g.,
room_id or caller_id if already parsed by handleFavoriteToggle) — do not include
the full NATS subject. Update natsFavoriteToggle to extract requestID from ctx
(the same context used for handleFavoriteToggle), pass that as "request_id" in
slog.Error along with sanitized fields (or omit sensitive identifiers), and keep
calling natsutil.ReplyError(sanitizeError(err)). Ensure the other respond-error
logging remains unchanged.
There was a problem hiding this comment.
Verified against the existing code — this is the established pattern across room-service/handler.go:
:119slog.Error("create-room failed", "error", err, "subject", m.Msg.Subject):1399slog.Error("mute toggle failed", "error", err, "subject", m.Msg.Subject):1484slog.Error("favorite toggle failed", "error", err, "subject", m.Msg.Subject)← this PR
The PII concern is valid in principle but applies uniformly to the whole room-service handler family, not just the favorite-toggle path. Fixing it here alone would split log shape across siblings (operators grepping for failures would get two formats from the same service) without addressing the underlying concern.
The right scope is a service-wide sweep — replace subject with request_id + safe structured fields across every handler in one focused PR. Leaving as-is here to preserve parity with the mute-toggle pattern this PR explicitly mirrors; tracking the broader cleanup as a separate follow-up.
Generated by Claude Code
There was a problem hiding this comment.
@general-lex, that's a sound call — splitting log shape mid-file would hurt operability more than it helps in isolation. The consistency argument is correct, and a single focused sweep is the right vehicle for this.
Would you like me to open a follow-up GitHub issue to track the service-wide cleanup (replacing "subject", m.Msg.Subject with request_id + safe structured fields across all nats* handlers in room-service/handler.go)? Happy to do that now if it helps.
✏️ Learnings added
Learnt from: general-lex
Repo: hmchangw/chat PR: 247
File: room-service/handler.go:1480-1490
Timestamp: 2026-06-01T08:44:38.014Z
Learning: In hmchangw/chat, the established logging pattern across all `nats*` handler wrappers in `room-service/handler.go` is to include `"subject", m.Msg.Subject` in `slog.Error` calls (e.g., lines 119, 1399, 1484). Although this logs the raw NATS subject (which encodes account/room identifiers), it is the intentional, uniform pattern throughout the file. Do NOT flag individual handlers for this — any PII/log-shape fix must be done as a service-wide sweep across all handlers in a dedicated PR to preserve log consistency for operators.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: Joey0538
Repo: hmchangw/chat PR: 59
File: pkg/roomkeysender/roomkeysender.go:28-30
Timestamp: 2026-04-08T08:25:32.108Z
Learning: In this repository’s Go code, follow the CLAUDE.md rule: don’t add defensive error handling, fallbacks, or nil/validation guards for conditions that are guaranteed not to occur. For internal functions/methods (e.g., when all callers are known to pass non-nil pointer parameters, such as pkg/roomkeysender/roomkeysender.go’s Send), the absence of nil-pointer guards is acceptable—do not flag it as a review issue. Reserve pointer validation and other safety checks for system boundaries (e.g., external inputs, API/HTTP requests, deserialization, or other untrusted entry points).
Learnt from: Joey0538
Repo: hmchangw/chat PR: 59
File: pkg/model/model_test.go:306-317
Timestamp: 2026-04-08T08:25:48.132Z
Learning: In Go, `time.Time` is `comparable` and therefore may be used as a type argument to generic constraints requiring `comparable` (e.g., `func roundTrip[T comparable](...)`). This is because `time.Time`’s underlying fields are all comparable (`uint64`, `int64`, and a pointer `*Location`, where pointers are comparable by address). As a result, any struct that contains or embeds a `time.Time` field (e.g., `model.Message`, or structs embedding it like `model.NotificationEvent`) should also satisfy `comparable` and should not be flagged as a compilation-error when used with `T comparable`.
Learnt from: general-lex
Repo: hmchangw/chat PR: 141
File: message-worker/main.go:95-99
Timestamp: 2026-05-04T03:30:38.678Z
Learning: In hmchangw/chat, `pkg/natsutil/connect.go`’s `natsutil.Connect` returns an OpenTelemetry-wrapped NATS connection (`*otelnats.Conn` from `github.com/Marz32onE/instrumentation-go/otel-nats/otelnats`), not a plain `*nats.Conn`. Because `otelnats.Conn.Publish` is defined as `Publish(ctx context.Context, subj string, data []byte) error` (it uses `ctx` to propagate traces via NATS headers), code review should NOT flag `nc.Publish(ctx, subj, data)` as an incorrect method call shape. This `Publish(ctx, ...)` pattern is the standard/correct usage across the codebase (e.g., message-worker, broadcast-worker, notification-worker, history-service).
Learnt from: general-lex
Repo: hmchangw/chat PR: 205
File: history-service/internal/service/pin.go:66-83
Timestamp: 2026-05-21T05:30:30.938Z
Learning: When reviewing cap enforcement in hmchangw/chat within history-service and room-service (e.g., functions like enforcePinLimit in history-service/internal/service/pin.go using MAX_* limits), the code may intentionally use a non-atomic read-then-write “soft cap” (allowing a small TOCTOU overage). Do not flag it as a TOCTOU/race-condition bug if the cap is explicitly designed to be operationally bounded rather than a hard invariant—i.e., it self-corrects because subsequent requests observe the inflated count and are rejected, and the codebase has not chosen atomic enforcement (e.g., LWT/counter table or findAndModify) as a requirement. Only flag as a TOCTOU issue when the surrounding codebase/design explicitly adopts atomic cap enforcement for these limits.
| sub, err := h.store.ToggleSubscriptionFavorite(ctx, roomID, account) | ||
| if err != nil { | ||
| if errors.Is(err, model.ErrSubscriptionNotFound) { | ||
| return nil, errNotRoomMember | ||
| } | ||
| return nil, fmt.Errorf("toggle subscription favorite: %w", err) | ||
| } | ||
|
|
||
| now := time.Now().UTC() | ||
|
|
||
| subEvt := model.SubscriptionUpdateEvent{ | ||
| UserID: sub.User.ID, | ||
| Subscription: *sub, | ||
| Action: "favorite_toggled", | ||
| Timestamp: now.UnixMilli(), | ||
| } | ||
| subEvtData, err := json.Marshal(subEvt) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("marshal subscription update event: %w", err) | ||
| } | ||
| if err := h.publishCore(ctx, subject.SubscriptionUpdate(account), subEvtData); err != nil { | ||
| slog.Error("subscription update publish failed", "error", err, "account", account) | ||
| // Non-fatal — the DB write is the source of truth; clients will reconcile on next refetch. | ||
| } | ||
|
|
||
| userSiteID, err := h.store.GetUserSiteID(ctx, account) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("get user siteId: %w", err) | ||
| } | ||
| if userSiteID != "" && userSiteID != h.siteID { | ||
| payload := model.SubscriptionFavoriteToggledEvent{ | ||
| Account: account, | ||
| RoomID: roomID, | ||
| Favorite: sub.Favorite, | ||
| Timestamp: now.UnixMilli(), | ||
| } | ||
| payloadData, err := json.Marshal(payload) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("marshal favorite-toggled payload: %w", err) | ||
| } | ||
| outbox := model.OutboxEvent{ | ||
| Type: model.OutboxSubscriptionFavoriteToggled, | ||
| SiteID: h.siteID, | ||
| DestSiteID: userSiteID, | ||
| Payload: payloadData, | ||
| Timestamp: now.UnixMilli(), | ||
| } | ||
| outboxData, err := json.Marshal(outbox) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("marshal outbox event: %w", err) | ||
| } | ||
| if err := h.publishToStream(ctx, subject.Outbox(h.siteID, userSiteID, model.OutboxSubscriptionFavoriteToggled), outboxData); err != nil { | ||
| return nil, fmt.Errorf("publish favorite-toggled outbox: %w", err) | ||
| } |
There was a problem hiding this comment.
Don't return a retriable error after the toggle has already committed.
ToggleSubscriptionFavorite flips the bit before the outbox publish happens. If GetUserSiteID or publishToStream fails, the client gets an error even though the mutation already succeeded; a retry will flip favorite again and can leave the local state and federation stream out of sync. This needs an acknowledged-after-commit design, or a durable outbox/retry mechanism around the post-write fanout.
🤖 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 1506 - 1559, The toggle currently
commits via h.store.ToggleSubscriptionFavorite then performs federation fanout
(GetUserSiteID, publishToStream) and returns an error if fanout fails, which can
re-trigger the toggle on retry; change to an acknowledged-after-commit pattern:
after ToggleSubscriptionFavorite succeeds, ensure outbox events are durably
recorded (e.g., insert an OutboxEvent row via a new store method like
CreateOutboxEvent or use an existing outbox writer) before attempting
publishToStream, and never return a retriable error to the client if outbox
publish fails; instead log the publish error and rely on a background worker to
read pending OutboxEvent entries and call publishToStream (or retry publish)
until success. Reference ToggleSubscriptionFavorite, GetUserSiteID,
publishToStream, model.OutboxEvent and the favorite-toggled outbox publish paths
when implementing durable outbox insertion and background retry.
There was a problem hiding this comment.
Verified — this trade-off is uniform across every cross-site publish in the system, not specific to favorite-toggle. mute.toggle, message.read, member.role.update, member.remove, add.member, and every other handler that writes to local Mongo and then publishes to OUTBOX use the same publish-then-error sequence.
A real transactional outbox is the right architectural answer eventually — Mongo outbox collection, new store method, idempotency keys, background drainer, dead-letter handling, and matching changes at every existing cross-site publish site. That's a multi-PR service-wide initiative, not a one-handler retrofit. Fixing only favorite.toggle would give the same Subscription document two different retry contracts depending on which boolean was mutated (favorite idempotent-on-retry, mute not), which is worse client-facing UX than the current uniform "client debounces" rule.
Leaving as-is to preserve sibling parity; tracking the durable-outbox initiative as a separate design discussion.
Generated by Claude Code
There was a problem hiding this comment.
@general-lex, understood — the reasoning is sound. Applying a transactional-outbox retrofit to a single handler while all sibling handlers remain on the same "client debounces" contract would produce an inconsistent retry surface on the same Subscription document. Leaving this as-is for parity makes sense.
Would you like me to open a GitHub issue to track the service-wide durable-outbox initiative (Mongo outbox collection, background drainer, idempotency keys, dead-letter handling) so it doesn't get lost?
✏️ Learnings added
Learnt from: general-lex
Repo: hmchangw/chat PR: 247
File: room-service/handler.go:1506-1559
Timestamp: 2026-06-01T09:01:36.432Z
Learning: In hmchangw/chat, the pattern of committing a local Mongo write then publishing a cross-site OUTBOX event and returning an error to the client if the publish fails is uniform across ALL cross-site handlers (mute.toggle, message.read, member.role.update, member.remove, add.member, favorite.toggle, thread.read, etc.). This is an intentional architectural trade-off: the DB write is the source of truth and clients are expected to debounce retries. A proper durable-outbox solution (Mongo outbox collection, new store method, idempotency keys, background drainer, dead-letter handling) is tracked as a separate multi-PR service-wide initiative. Do NOT flag individual handlers for this pattern as it is consistent by design.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: Joey0538
Repo: hmchangw/chat PR: 59
File: pkg/roomkeysender/roomkeysender.go:28-30
Timestamp: 2026-04-08T08:25:32.108Z
Learning: In this repository’s Go code, follow the CLAUDE.md rule: don’t add defensive error handling, fallbacks, or nil/validation guards for conditions that are guaranteed not to occur. For internal functions/methods (e.g., when all callers are known to pass non-nil pointer parameters, such as pkg/roomkeysender/roomkeysender.go’s Send), the absence of nil-pointer guards is acceptable—do not flag it as a review issue. Reserve pointer validation and other safety checks for system boundaries (e.g., external inputs, API/HTTP requests, deserialization, or other untrusted entry points).
Learnt from: Joey0538
Repo: hmchangw/chat PR: 59
File: pkg/model/model_test.go:306-317
Timestamp: 2026-04-08T08:25:48.132Z
Learning: In Go, `time.Time` is `comparable` and therefore may be used as a type argument to generic constraints requiring `comparable` (e.g., `func roundTrip[T comparable](...)`). This is because `time.Time`’s underlying fields are all comparable (`uint64`, `int64`, and a pointer `*Location`, where pointers are comparable by address). As a result, any struct that contains or embeds a `time.Time` field (e.g., `model.Message`, or structs embedding it like `model.NotificationEvent`) should also satisfy `comparable` and should not be flagged as a compilation-error when used with `T comparable`.
Learnt from: general-lex
Repo: hmchangw/chat PR: 141
File: message-worker/main.go:95-99
Timestamp: 2026-05-04T03:30:38.678Z
Learning: In hmchangw/chat, `pkg/natsutil/connect.go`’s `natsutil.Connect` returns an OpenTelemetry-wrapped NATS connection (`*otelnats.Conn` from `github.com/Marz32onE/instrumentation-go/otel-nats/otelnats`), not a plain `*nats.Conn`. Because `otelnats.Conn.Publish` is defined as `Publish(ctx context.Context, subj string, data []byte) error` (it uses `ctx` to propagate traces via NATS headers), code review should NOT flag `nc.Publish(ctx, subj, data)` as an incorrect method call shape. This `Publish(ctx, ...)` pattern is the standard/correct usage across the codebase (e.g., message-worker, broadcast-worker, notification-worker, history-service).
Learnt from: general-lex
Repo: hmchangw/chat PR: 205
File: history-service/internal/service/pin.go:66-83
Timestamp: 2026-05-21T05:30:30.938Z
Learning: When reviewing cap enforcement in hmchangw/chat within history-service and room-service (e.g., functions like enforcePinLimit in history-service/internal/service/pin.go using MAX_* limits), the code may intentionally use a non-atomic read-then-write “soft cap” (allowing a small TOCTOU overage). Do not flag it as a TOCTOU/race-condition bug if the cap is explicitly designed to be operationally bounded rather than a hard invariant—i.e., it self-corrects because subsequent requests observe the inflated count and are rejected, and the codebase has not chosen atomic enforcement (e.g., LWT/counter table or findAndModify) as a requirement. Only flag as a TOCTOU issue when the surrounding codebase/design explicitly adopts atomic cap enforcement for these limits.
Mirrors the mute-toggle architecture for the new Favorite flag — a per-user, room-scoped boolean rendered in the sidebar. room-service flips Subscription.favorite atomically via a FindOneAndUpdate aggregation pipeline (treats missing field as false), fans out a subscription.update event with action="favorite_toggled" to the requester's other sessions, and when the requester's home site differs from the room's site, publishes a subscription_favorite_toggled OutboxEvent that inbox-worker mirrors onto the home-site copy. Backend treats favorite as a pure render hint — notification-worker and broadcast-worker do not consult it. https://claude.ai/code/session_01CNQTuBnbtUqQcoNKcF3i8f
general-lex
force-pushed
the
claude/focused-hypatia-gijQ6
branch
from
0e6efba to
9c54abf
Compare
| Alert bool `json:"alert" bson:"alert"` | ||
| Muted bool `json:"muted" bson:"muted"` | ||
| Favorite bool `json:"favorite,omitempty" bson:"favorite,omitempty"` | ||
| Favorite bool `json:"favorite" bson:"favorite"` |
There was a problem hiding this comment.
Why do we need to remove omitempty flag here?
There was a problem hiding this comment.
it is to match the convention of every other boolean state flag on Subscription (Alert, Muted, HasMention — all serialize-when-false), and I have published docs/client-api.md contract already like others for this field, is always present
| if err != nil { | ||
| return nil, fmt.Errorf("marshal outbox event: %w", err) | ||
| } | ||
| if err := h.publishToStream(ctx, subject.Outbox(h.siteID, userSiteID, model.OutboxSubscriptionFavoriteToggled), outboxData); err != nil { |
There was a problem hiding this comment.
There will be no more outbox and we are going to have one "INBOX_{siteID}" per site and publishing to it directly
There was a problem hiding this comment.
I'm aware there will be changes. But specifics are tentative. I think it's fine to keep things consistent in main, refactor can be done at once.
3 participants
Summary
Implements a new client-facing NATS RPC
favorite.togglethat flips a booleanSubscription.favoriteflag for the requester on a single room. The flag is a per-user, per-room render hint for a "favorites" sidebar section; the backend treats it as opaque and does not gate notifications, routing, or validation.Architecturally mirrors the existing
mute.toggleRPC (PR #217): same subject shape, same store pattern, same cross-site replication via Outbox/Inbox streams. The mute and favorite flags are independent toggles onSubscriptionwith parallel, non-coupled handlers.Key Changes
Model & Events
Favorite boolfield toSubscription(always serialized, noomitempty)FavoriteToggleResponsewithStatusandFavoritefieldsSubscriptionFavoriteToggledEventfor cross-site federation payloadsOutboxSubscriptionFavoriteToggledevent-type constantSubscriptionUpdateEvent.Actionenum to include"favorite_toggled"Subject Builders (
pkg/subject/subject.go)FavoriteToggle(account, roomID, siteID)→ concrete subjectFavoriteToggleWildcard(siteID)→ wildcard for queue subscriptionRoom Service Handler (
room-service/handler.go)FavoriteToggleWildcardnatsFavoriteTogglewraps context and error handlinghandleFavoriteToggleorchestrates the toggle:ParseUserRoomSubjectstore.ToggleSubscriptionFavorite(atomic MongoFindOneAndUpdate)SubscriptionUpdateEventwithAction: "favorite_toggled"(non-fatal on failure)SubscriptionFavoriteToggledEventto Outbox stream{status: "ok", favorite: <post-flip value>}Store Interface & Implementation (
room-service/store.go,store_mongo.go)ToggleSubscriptionFavorite(ctx, roomID, account)atomically flips the bit via$notand$ifNull(handles legacy docs without the field)(roomId, "u.account")compound index)Inbox Worker (
inbox-worker/handler.go,main.go)"subscription_favorite_toggled"eventsUpdateSubscriptionFavoritestore method sets the flag by(roomID, account)Client API Documentation (
docs/client-api.md)"favorite_toggled"action andfavoritefieldImplementation Details
mute.toggle).$ifNull: ["$favorite", false]so documents written before this change (nofavoritekey) deterministically toggle totrueon first call. No migration script required.SubscriptionUpdateEventpublish fails on same-site toggles, the handler still replies success — the DB write is the source of truth and clients reconcile on next refetch.https://claude.ai/code/session_01CNQTuBnbtUqQcoNKcF3i8f
Summary by CodeRabbit
New Features
Documentation