feat(room-service): add member.statuses + subscription.mentionable RPCs#260
Open
vjauhari-work wants to merge 3 commits into
Open
feat(room-service): add member.statuses + subscription.mentionable RPCs#260vjauhari-work wants to merge 3 commits into
vjauhari-work wants to merge 3 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds two room-scoped read RPCs ( ChangesMember Statuses & Mentionable Subscriptions Implementation
Sequence DiagramsequenceDiagram
participant Client
participant RoomService as RoomService.Handler
participant MongoStore as MongoStore
Client->>RoomService: Publish request on subject (member.statuses / subscription.mentionable)
RoomService->>RoomService: Parse subject, decode JSON, set trace attrs
RoomService->>MongoStore: GetSubscription (membership probe)
RoomService->>MongoStore: GetRoom (room probe)
RoomService->>MongoStore: ListMemberStatuses / ListMentionableSubscriptions (escapedFilter, limit)
MongoStore-->>RoomService: Aggregation results
RoomService-->>Client: Sanitized JSON reply or sentinel error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 |
Collaborator
Author
|
@coderabbitai full-review |
|
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
room-service/handler_test.go (1)
4046-4266: ⚡ Quick winAdd a regression case for
GetRoomfailing beforeGetSubscriptioncompletes.These suites cover the happy/error branches well, but they don't pin the cancellation race inside
requireMembershipAndGetRoom. A test that forcesGetRoomto fail first andGetSubscriptionto observe cancellation would catch the current error-masking bug and keep the precedence contract from regressing.Also applies to: 4308-4554
🤖 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_test.go` around lines 4046 - 4266, Add a regression test to TestHandler_ListMemberStatuses that forces GetRoom to fail before GetSubscription finishes to exercise requireMembershipAndGetRoom’s cancellation race: mock MockRoomStore.GetRoom to immediately return an error (e.g. fmt.Errorf("mongo exploded")) and mock MockRoomStore.GetSubscription to block/return after context cancellation (simulate by returning a channel-blocking stub or an error wrapped with context.Canceled), then call Handler.handleListMemberStatuses and assert the returned error originates from the GetRoom path (contains "get room" or matches the expected store error). Reference the functions/objects: Handler.handleListMemberStatuses, requireMembershipAndGetRoom, MockRoomStore.GetRoom, MockRoomStore.GetSubscription, and ensure the test asserts the GetRoom error is not masked by a subscription cancellation.room-service/handler.go (1)
459-467: ⚡ Quick winAdd the request ID to these new error logs.
Both handlers already build
ctxviawrappedCtx, but the newslog.Errorcalls drop the correlation ID, which makes cross-service tracing harder when these RPCs fail.Suggested fix
func (h *Handler) natsListMemberStatuses(m otelnats.Msg) { ctx := wrappedCtx(m) + requestID := natsutil.RequestIDFromContext(ctx) resp, err := h.handleListMemberStatuses(ctx, m.Msg.Subject, m.Msg.Data) if err != nil { - slog.Error("list member statuses failed", "error", err, "subject", m.Msg.Subject) + slog.Error("list member statuses failed", "error", err, "subject", m.Msg.Subject, "request_id", requestID) natsutil.ReplyError(m.Msg, sanitizeError(err)) return } natsutil.ReplyJSON(m.Msg, resp) } func (h *Handler) natsListMentionableSubscriptions(m otelnats.Msg) { ctx := wrappedCtx(m) + requestID := natsutil.RequestIDFromContext(ctx) resp, err := h.handleListMentionableSubscriptions(ctx, m.Msg.Subject, m.Msg.Data) if err != nil { - slog.Error("list mentionable subscriptions failed", "error", err, "subject", m.Msg.Subject) + slog.Error("list mentionable subscriptions failed", "error", err, "subject", m.Msg.Subject, "request_id", requestID) natsutil.ReplyError(m.Msg, sanitizeError(err)) return } natsutil.ReplyJSON(m.Msg, resp) }As per coding guidelines, "Generate or extract a unique request/correlation ID at the entry point ... include in all log lines".
Also applies to: 559-567
🤖 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 459 - 467, The error logs in natsListMemberStatuses are missing the request/correlation ID; update the slog.Error call to include the request ID pulled from ctx (use the same context key or helper used elsewhere in the package, e.g., requestIDFromCtx(ctx) or ctx.Value(requestIDKey)) as an extra field (e.g., "request_id", id) so the log contains the correlation id; apply the same change to the other NATS handler in this file that has a similar slog.Error call later in the file.
🤖 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
`@docs/superpowers/plans/2026-05-29-room-member-statuses-and-mentionable-subscriptions.md`:
- Around line 243-249: Update the documented default and error text to match
shipped behavior: change MentionableSubscriptionsRequest's default Limit from 1
to 3 (and note the constraint stays >0 and <= room.UserCount + room.AppCount)
and replace the old membership error string "only room members can list members"
with the generalized "not a room member" wherever the
subscription.mentionable/member-status contract is documented (including the
other occurrences referenced). Locate mentions by symbol names like
MentionableSubscriptionsRequest and any documented RPC error messages for
subscription.mentionable/member-status and update the text accordingly so
generated tests/docs reflect the final contract.
- Around line 7-9: Update the plan to reflect the landed account classification
by replacing the old combined bot/platform-admin rule (`\.bot$|^p_`) with the
new split semantics: treat accounts matching `\.bot$` as assistant/bot accounts
and treat accounts prefixed with `p_` as platform-admin/webhook accounts; update
any prose, examples, and the referenced test steps that rely on `botPattern` or
the regex in the plan sections (including the occurrences around the ranges you
noted) so they assert and document the two distinct rules separately and ensure
references to `botPattern` and any Mongo/Go regex use the two-pattern behavior.
In
`@docs/superpowers/specs/2026-05-29-room-member-statuses-and-mentionable-subscriptions-design.md`:
- Around line 110-114: Update the spec to reflect the implemented default limit
of 3 for mentionable subscriptions: change the comment on the
MentionableSubscriptionsRequest struct from "nil → default 1" to "nil → default
3", replace any inline example assigning `limit := 1` with `limit := 3`, and
update the descriptive text that says "default of 1 — also intentionally tight"
to state the default is 3; ensure these edits align with the implementation in
pkg/model/member.go and the PR summary which raised the default to 3.
- Around line 47-64: The spec and implementation disagree on the bot regex:
either update the spec to document the two-constant design (botAccountRegex and
platformAdminRegex) and show how the Feature 2 Mongo pipeline uses both (or a
combined expression derived from them), or change the implementation to match
the spec by consolidating into a single constant (botAccountRegex =
`\.bot$|^p_`) and update botPattern and isBot accordingly; reference the symbols
botAccountRegex, platformAdminRegex, botPattern, and isBot so the spec, unit
tests, and the Feature 2 pipeline all use the same source of truth.
In `@room-service/handler.go`:
- Around line 487-509: The concurrent fetch uses errgroup.WithContext which
cancels the sibling call on the first error, causing GetSubscription to surface
context.Canceled instead of its original error; replace
errgroup.WithContext(ctx) with a plain errgroup.Group (e.g., declare var g
errgroup.Group), call h.store.GetSubscription and h.store.GetRoom with the
original ctx (not gctx), keep assigning subErr and roomErr from those
goroutines, and preserve the existing post-Wait checks (errors.Is(subErr,
model.ErrSubscriptionNotFound) -> errNotRoomMember, then other error wraps) so
both real errors are visible and priority is unchanged.
---
Nitpick comments:
In `@room-service/handler_test.go`:
- Around line 4046-4266: Add a regression test to TestHandler_ListMemberStatuses
that forces GetRoom to fail before GetSubscription finishes to exercise
requireMembershipAndGetRoom’s cancellation race: mock MockRoomStore.GetRoom to
immediately return an error (e.g. fmt.Errorf("mongo exploded")) and mock
MockRoomStore.GetSubscription to block/return after context cancellation
(simulate by returning a channel-blocking stub or an error wrapped with
context.Canceled), then call Handler.handleListMemberStatuses and assert the
returned error originates from the GetRoom path (contains "get room" or matches
the expected store error). Reference the functions/objects:
Handler.handleListMemberStatuses, requireMembershipAndGetRoom,
MockRoomStore.GetRoom, MockRoomStore.GetSubscription, and ensure the test
asserts the GetRoom error is not masked by a subscription cancellation.
In `@room-service/handler.go`:
- Around line 459-467: The error logs in natsListMemberStatuses are missing the
request/correlation ID; update the slog.Error call to include the request ID
pulled from ctx (use the same context key or helper used elsewhere in the
package, e.g., requestIDFromCtx(ctx) or ctx.Value(requestIDKey)) as an extra
field (e.g., "request_id", id) so the log contains the correlation id; apply the
same change to the other NATS handler in this file that has a similar slog.Error
call later in the file.
🪄 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: 96e49861-3c7c-496c-a947-e16c4fa7abef
📒 Files selected for processing (22)
docs/client-api.mddocs/superpowers/plans/2026-05-29-room-member-statuses-and-mentionable-subscriptions.mddocs/superpowers/specs/2026-05-29-room-member-statuses-and-mentionable-subscriptions-design.mdpkg/model/account.gopkg/model/account_test.gopkg/model/member.gopkg/model/model_test.gopkg/model/user.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.goroom-worker/handler.goroom-worker/integration_test.goroom-worker/store_mongo.goroom-worker/subscription_isbot_test.go
💤 Files with no reviewable changes (2)
- pkg/model/account.go
- pkg/model/account_test.go
vjauhari-work
pushed a commit
that referenced
this pull request
Jun 2, 2026
Concurrency fix:
- requireMembershipAndGetRoom: switch from errgroup.WithContext to plain
errgroup.Group. WithContext cancelled the sibling on first error, so a
fast GetRoom failure could turn GetSubscription into context.Canceled
and mask errNotRoomMember. Both probes now run to completion on the
original ctx; precedence is enforced post-Wait. (CodeRabbit Major)
Test coverage:
- Add a "not-member takes precedence over GetRoom error" subtest to both
TestHandler_ListMemberStatuses and TestHandler_ListMentionableSubscriptions
— locks in the precedence contract that the errgroup fix preserves.
(CodeRabbit nitpick)
Observability:
- Add requestID via natsutil.RequestIDFromContext to the slog.Error in
natsListMemberStatuses and natsListMentionableSubscriptions, per
CLAUDE.md "include request ID in all log lines". (CodeRabbit nitpick)
Spec/plan drift:
- Spec §3.3 rewritten to describe the two-constant split
(botAccountRegex + platformAdminRegex) the code actually uses, with
the User.IsBot / User.IsPlatformAdmin Go-side equivalents.
- Spec MentionableSubscriptionsRequest comment + example + "Future
optimizations" note updated for the default limit of 3 (was 1).
- Plan: added an explicit "Drift from shipped implementation" preamble
listing the four design-vs-code divergences (split classification,
default limit 3, "not a room member" error text, parallelized
membership probe). Plus targeted edits to the affected code snippets
inside the plan so cut-and-paste from those examples doesn't produce
stale behavior.
Skipped:
- CodeRabbit's "Docstring Coverage 38% < 80%" pre-merge warning runs
counter to CLAUDE.md's "default to writing no comments" guidance.
Not a project-side requirement.
vjauhari-work
force-pushed
the
claude/zen-volta-mZ7G4
branch
from
d490cbd to
7e8d9ed
Compare
vjauhari-work
pushed a commit
that referenced
this pull request
Jun 2, 2026
Concurrency fix:
- requireMembershipAndGetRoom: switch from errgroup.WithContext to plain
errgroup.Group. WithContext cancelled the sibling on first error, so a
fast GetRoom failure could turn GetSubscription into context.Canceled
and mask errNotRoomMember. Both probes now run to completion on the
original ctx; precedence is enforced post-Wait. (CodeRabbit Major)
Test coverage:
- Add a "not-member takes precedence over GetRoom error" subtest to both
TestHandler_ListMemberStatuses and TestHandler_ListMentionableSubscriptions
— locks in the precedence contract that the errgroup fix preserves.
(CodeRabbit nitpick)
Observability:
- Add requestID via natsutil.RequestIDFromContext to the slog.Error in
natsListMemberStatuses and natsListMentionableSubscriptions, per
CLAUDE.md "include request ID in all log lines". (CodeRabbit nitpick)
Spec/plan drift:
- Spec §3.3 rewritten to describe the two-constant split
(botAccountRegex + platformAdminRegex) the code actually uses, with
the User.IsBot / User.IsPlatformAdmin Go-side equivalents.
- Spec MentionableSubscriptionsRequest comment + example + "Future
optimizations" note updated for the default limit of 3 (was 1).
- Plan: added an explicit "Drift from shipped implementation" preamble
listing the four design-vs-code divergences (split classification,
default limit 3, "not a room member" error text, parallelized
membership probe). Plus targeted edits to the affected code snippets
inside the plan so cut-and-paste from those examples doesn't produce
stale behavior.
Skipped:
- CodeRabbit's "Docstring Coverage 38% < 80%" pre-merge warning runs
counter to CLAUDE.md's "default to writing no comments" guidance.
Not a project-side requirement.
vjauhari-work
force-pushed
the
claude/zen-volta-mZ7G4
branch
2 times, most recently
from
96481c0 to
757e98d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
docs/superpowers/plans/2026-05-29-room-member-statuses-and-mentionable-subscriptions.md (2)
700-739:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Implementation guidance has incorrect
$limitplacement.The pipeline (line 708) applies
$limitBEFORE$lookup, identical to the design spec issue. This implementation will be copy-pasted by developers and will ship the bug.Problem: If the first N subscriptions include orphaned users, the response will have fewer than
limitmembers even when valid members exist.See the design spec review comment for detailed explanation and fix.
🤖 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 `@docs/superpowers/plans/2026-05-29-room-member-statuses-and-mentionable-subscriptions.md` around lines 700 - 739, The pipeline in ListMemberStatuses applies the $limit before the $lookup/$unwind, so orphaned subscriptions can reduce returned members; move the $limit so it is applied after the $lookup/$unwind (i.e., limit the resulting joined user documents), ensuring the stage ordering is: $match, $lookup, $unwind (preserveNullAndEmptyArrays:false), $replaceWith (or $project), then $limit (using int64(limit)) before aggregation execution; update the mongo.Pipeline accordingly so ListMemberStatuses returns up to `limit` valid member documents.
1218-1276:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMajor: Implementation is missing platform-admin filtering.
The pipeline (lines 1230-1234) filters
excludeAccountbut does not filter outp_platform-admin accounts. Per the drift note (lines 15-16), "the mentionable pipeline...hidesp_accounts entirely."This implementation guidance will ship without the required filtering.
🔧 Add the missing filter
pipeline := mongo.Pipeline{ {{Key: "$match", Value: bson.M{ "roomId": roomID, "u.account": bson.M{"$ne": excludeAccount}, }}}, + {{Key: "$match", Value: bson.M{ + "u.account": bson.M{"$not": bson.M{"$regex": platformAdminRegex}}, + }}}, {{Key: "$lookup", Value: bson.M{🤖 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 `@docs/superpowers/plans/2026-05-29-room-member-statuses-and-mentionable-subscriptions.md` around lines 1218 - 1276, The ListMentionableSubscriptions pipeline is missing the platform-admin exclusion — update the initial $match in the pipeline (the bson.M that currently has "roomId": roomID and "u.account": bson.M{"$ne": excludeAccount}) to also exclude accounts that start with "p_" (platform admins); for example add a condition on "u.account" such as bson.M{"$ne": excludeAccount, "$not": bson.RegEx{Pattern: "^p_", Options: ""}} (or an equivalent "$not": bson.M{"$regex": "^p_"}) so platform-admin accounts are filtered out alongside excludeAccount.
🧹 Nitpick comments (1)
docs/superpowers/plans/2026-05-29-room-member-statuses-and-mentionable-subscriptions.md (1)
1663-1704: ⚡ Quick winMinor: Test references undefined
isBot()helper.Line 1687 calls
isBot(acct)but this function is never defined in the integration test file or imported.Likely intent: A local test helper that implements
strings.HasSuffix(acct, ".bot")to match the Go-side classification, separate from the(*model.User).IsBot()method.Suggestion: Add the helper definition to the test preamble for clarity:
// isBot returns true if account ends in ".bot" (assistant bot). // Matches the classification in (*model.User).IsBot(). func isBot(account string) bool { return strings.HasSuffix(account, ".bot") }This makes the test self-contained and documents the Go-side classification rule being verified.
🤖 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 `@docs/superpowers/plans/2026-05-29-room-member-statuses-and-mentionable-subscriptions.md` around lines 1663 - 1704, The test calls an undefined helper isBot(acct); add a local helper function isBot(account string) bool that returns strings.HasSuffix(account, ".bot") (and import "strings" if needed), placing it in the test file preamble so TestBotPattern_GoAndMongoAgree_Integration can call isBot() to mirror the Go-side classification used when seeding Subscription.User.IsBot.
🤖 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
`@docs/superpowers/specs/2026-05-29-room-member-statuses-and-mentionable-subscriptions-design.md`:
- Around line 358-414: The pipeline in ListMentionableSubscriptions is missing a
filter to exclude platform admin accounts (those matching platformAdminRegex),
so add an early $match stage (before the $lookup stages) that applies
{"u.account": {"$not": platformAdminRegex}} (together with the existing roomId
and excludeAccount conditions) to prevent any "p_" accounts from proceeding
through the joins and appearing in results; update the mongo.Pipeline
construction in ListMentionableSubscriptions to include this $match as the
first/second stage so platform admins are fully excluded from mention
autocomplete.
- Around line 253-290: The pipeline in ListMemberStatuses applies the $limit
before $lookup/$unwind, which can return fewer than the requested valid members
when early subscriptions are orphaned; move the $limit stage to after the
$unwind (or after the $replaceWith) so that orphaned subscriptions are dropped
first and the limit is applied to the post-join, filtered member documents;
update the mongo.Pipeline in ListMemberStatuses accordingly and keep
preserveNullAndEmptyArrays: false so the later $limit bounds the number of valid
members.
---
Duplicate comments:
In
`@docs/superpowers/plans/2026-05-29-room-member-statuses-and-mentionable-subscriptions.md`:
- Around line 700-739: The pipeline in ListMemberStatuses applies the $limit
before the $lookup/$unwind, so orphaned subscriptions can reduce returned
members; move the $limit so it is applied after the $lookup/$unwind (i.e., limit
the resulting joined user documents), ensuring the stage ordering is: $match,
$lookup, $unwind (preserveNullAndEmptyArrays:false), $replaceWith (or $project),
then $limit (using int64(limit)) before aggregation execution; update the
mongo.Pipeline accordingly so ListMemberStatuses returns up to `limit` valid
member documents.
- Around line 1218-1276: The ListMentionableSubscriptions pipeline is missing
the platform-admin exclusion — update the initial $match in the pipeline (the
bson.M that currently has "roomId": roomID and "u.account": bson.M{"$ne":
excludeAccount}) to also exclude accounts that start with "p_" (platform
admins); for example add a condition on "u.account" such as bson.M{"$ne":
excludeAccount, "$not": bson.RegEx{Pattern: "^p_", Options: ""}} (or an
equivalent "$not": bson.M{"$regex": "^p_"}) so platform-admin accounts are
filtered out alongside excludeAccount.
---
Nitpick comments:
In
`@docs/superpowers/plans/2026-05-29-room-member-statuses-and-mentionable-subscriptions.md`:
- Around line 1663-1704: The test calls an undefined helper isBot(acct); add a
local helper function isBot(account string) bool that returns
strings.HasSuffix(account, ".bot") (and import "strings" if needed), placing it
in the test file preamble so TestBotPattern_GoAndMongoAgree_Integration can call
isBot() to mirror the Go-side classification used when seeding
Subscription.User.IsBot.
🪄 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: 0d012e8b-72c0-445e-a58f-26feee7aea8e
📒 Files selected for processing (16)
docs/client-api.mddocs/superpowers/plans/2026-05-29-room-member-statuses-and-mentionable-subscriptions.mddocs/superpowers/specs/2026-05-29-room-member-statuses-and-mentionable-subscriptions-design.mdpkg/model/member.gopkg/model/model_test.gopkg/model/user.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
💤 Files with no reviewable changes (13)
- pkg/subject/subject_test.go
- pkg/model/user.go
- pkg/subject/subject.go
- room-service/helper.go
- room-service/store.go
- room-service/handler.go
- room-service/helper_test.go
- room-service/integration_test.go
- pkg/model/member.go
- room-service/store_mongo.go
- pkg/model/model_test.go
- room-service/mock_store_test.go
- room-service/handler_test.go
✅ Files skipped from review due to trivial changes (1)
- docs/client-api.md
vjauhari-work
force-pushed
the
claude/zen-volta-mZ7G4
branch
2 times, most recently
from
6d950c9 to
c24fd4a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/superpowers/plans/2026-05-29-room-member-statuses-and-mentionable-subscriptions.md (1)
1376-1386:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnify mentionable default-limit guidance to
3across the plan.Several executable snippets/checklists still encode default
1, which conflicts with the drift note and shipped behavior and can cause future replays/tests/docs to regress.Suggested doc diff
- name: "default limit 1, empty filter, happy path", + name: "default limit 3, empty filter, happy path", - ListMentionableSubscriptions(gomock.Any(), roomID, requester, "", 1). + ListMentionableSubscriptions(gomock.Any(), roomID, requester, "", 3). - | `limit` | number | no | Defaults to `1`. Must be `> 0` and `<= room.userCount + room.appCount`. | + | `limit` | number | no | Defaults to `3` (effectively `min(3, room.userCount + room.appCount)`). Must be `> 0` and `<= room.userCount + room.appCount`. | - - Default-limit numbers (3 / 1) match between handler code, tests, docs, and the spec. + - Default-limit numbers (3 / 3) match between handler code, tests, docs, and the spec.Also applies to: 1502-1503, 1833-1834, 1941-1941
🤖 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 `@docs/superpowers/plans/2026-05-29-room-member-statuses-and-mentionable-subscriptions.md` around lines 1376 - 1386, The tests and doc snippets that call ListMentionableSubscriptions (e.g., the case named "default limit 1, empty filter, happy path") still use a default limit of 1; update those calls to use the unified default limit of 3 (change the literal 1 passed to ListMentionableSubscriptions to 3) and update the other similar occurrences referenced in the plan (the spots around the other examples that call ListMentionableSubscriptions) so the examples, tests, and docs consistently reflect the shipped default of 3.
🤖 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
`@docs/superpowers/plans/2026-05-29-room-member-statuses-and-mentionable-subscriptions.md`:
- Around line 20-22: The doc contains a typo in the membership-error wording
that renders the contrast meaningless; update the sentence to show the intended
contrasting error strings and correct the drift-note so it accurately states
which text is used (e.g., clarify that the shared sentinel errNotRoomMember
yields "not a room member" across the four membership-gated RPCs rather than
repeating the same phrase twice). Locate the sentence referencing
errNotRoomMember and replace the incorrect quoted pair with the correct
contrasting phrases so the note clearly communicates the intended difference.
---
Duplicate comments:
In
`@docs/superpowers/plans/2026-05-29-room-member-statuses-and-mentionable-subscriptions.md`:
- Around line 1376-1386: The tests and doc snippets that call
ListMentionableSubscriptions (e.g., the case named "default limit 1, empty
filter, happy path") still use a default limit of 1; update those calls to use
the unified default limit of 3 (change the literal 1 passed to
ListMentionableSubscriptions to 3) and update the other similar occurrences
referenced in the plan (the spots around the other examples that call
ListMentionableSubscriptions) so the examples, tests, and docs consistently
reflect the shipped default of 3.
🪄 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: d8363f61-2b3a-4ad2-9613-e4b289787c43
📒 Files selected for processing (16)
docs/client-api.mddocs/superpowers/plans/2026-05-29-room-member-statuses-and-mentionable-subscriptions.mddocs/superpowers/specs/2026-05-29-room-member-statuses-and-mentionable-subscriptions-design.mdpkg/model/member.gopkg/model/model_test.gopkg/model/user.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
💤 Files with no reviewable changes (10)
- room-service/helper.go
- pkg/subject/subject_test.go
- room-service/store.go
- pkg/subject/subject.go
- room-service/mock_store_test.go
- room-service/store_mongo.go
- room-service/helper_test.go
- room-service/handler_test.go
- room-service/handler.go
- room-service/integration_test.go
✅ Files skipped from review due to trivial changes (1)
- docs/client-api.md
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/model/member.go
- pkg/model/user.go
- pkg/model/model_test.go
vjauhari-work
force-pushed
the
claude/zen-volta-mZ7G4
branch
2 times, most recently
from
2b5658d to
951c7b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.go`:
- Around line 668-670: The fast-path returns use the zero-value
model.ListMemberStatusesResponse{} which JSON-encodes slices as null; change
these return sites (the branch checking room.UserCount == 0 and the other
empty-room branch around the subscriptions return) to return an explicit
response with empty slices (e.g. Members: []model.MemberStatus{} and
Subscriptions: []model.Subscription{} as appropriate) so JSON encodes [] instead
of null; locate the returns that currently return
model.ListMemberStatusesResponse{} and replace them with a constructed
model.ListMemberStatusesResponse containing the empty slice fields.
🪄 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: 3e8baa11-6b50-4c1f-8e1a-12d4e8bf2aa1
📒 Files selected for processing (16)
docs/client-api.mddocs/superpowers/plans/2026-05-29-room-member-statuses-and-mentionable-subscriptions.mddocs/superpowers/specs/2026-05-29-room-member-statuses-and-mentionable-subscriptions-design.mdpkg/model/member.gopkg/model/model_test.gopkg/model/user.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
✅ Files skipped from review due to trivial changes (1)
- room-service/mock_store_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- pkg/subject/subject.go
- room-service/store.go
- room-service/store_mongo.go
- pkg/subject/subject_test.go
- pkg/model/user.go
- room-service/helper_test.go
- room-service/integration_test.go
- pkg/model/model_test.go
- docs/client-api.md
- room-service/helper.go
- room-service/handler_test.go
vjauhari-work
force-pushed
the
claude/zen-volta-mZ7G4
branch
2 times, most recently
from
469f645 to
da32f18
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
docs/superpowers/plans/2026-05-29-room-member-statuses-and-mentionable-subscriptions.md (1)
20-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the drift note membership error text to the shipped sentinel string.
This still says the shared text is
"only room members can perform this action", but the shipped contract is"not a room member"across membership-gated RPCs.🤖 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 `@docs/superpowers/plans/2026-05-29-room-member-statuses-and-mentionable-subscriptions.md` around lines 20 - 23, Update the drift note to reflect the actual shipped sentinel string by replacing the quoted membership error text `"only room members can perform this action"` with the shipped `"not a room member"`; reference the shared sentinel errNotRoomMember which is reused across the four membership-gated RPCs so the wording is RPC-agnostic and must match that sentinel.docs/superpowers/specs/2026-05-29-room-member-statuses-and-mentionable-subscriptions-design.md (1)
20-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the stale
botPatterncontract and align all sections to split bot/admin semantics.The spec still states combined
\.bot$|^p_behavior and even claimsp_becomesoptionType="app", which contradicts the shipped split (.bot=> app,p_=> hidden).Also applies to: 525-526, 564-565
🤖 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 `@docs/superpowers/specs/2026-05-29-room-member-statuses-and-mentionable-subscriptions-design.md` around lines 20 - 21, The spec incorrectly describes a single `botPattern` regex (\.bot$|^p_) and claims `p_` becomes optionType="app"; update the document to remove this stale combined contract and instead consistently describe the shipped split: `.bot` identifies app accounts (optionType="app") and `^p_` identifies hidden accounts (optionType="hidden"). Replace any references to a single `botPattern` and any assertions that `p_` maps to "app" with the explicit split semantics, and update the sections that mention the Go-side and Mongo pipeline usage as well as the new `docs/client-api.md` RPC docs to reflect `.bot => app` and `p_ => hidden` consistently throughout (including all other occurrences of `botPattern`, `optionType="app"`, and the RPC descriptions).
🤖 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
`@docs/superpowers/plans/2026-05-29-room-member-statuses-and-mentionable-subscriptions.md`:
- Around line 956-993: The handler handleListMemberStatuses currently calls
GetSubscription then GetRoom sequentially; change it to invoke
h.store.GetSubscription(ctx, requesterAccount, roomID) and h.store.GetRoom(ctx,
roomID) in parallel (e.g., goroutines + err channels) and then reconcile results
so subscription membership errors take precedence (if GetSubscription returns
model.ErrSubscriptionNotFound return errNotRoomMember immediately), while other
errors from either call are wrapped normally. Also adjust limit handling to
mirror shipped behavior: treat nil or non-positive req.Limit as the default (3),
clamp the final limit to room.UserCount (and allow empty-room behavior by
permitting limit==0 only if that matches shipped semantics), and keep using
ListMemberStatuses(ctx, roomID, limit) afterward; refer to
handleListMemberStatuses, h.store.GetSubscription, h.store.GetRoom,
h.store.ListMemberStatuses, errNotRoomMember, and errMemberStatusesLimitInvalid
when making changes.
In
`@docs/superpowers/specs/2026-05-29-room-member-statuses-and-mentionable-subscriptions-design.md`:
- Around line 202-203: The membership-check comment and error mapping are
outdated: when h.store.GetSubscription(ctx, requesterAccount, roomID) fails, the
handler currently references the old sentinel text "only room members can list
members"; update the sanitizeError/handler mapping so that the sentinel returned
for subscription/membership failures uses the shared text "not a room member"
instead, ensuring any code paths that call sanitizeError (or the error message
constructed around h.store.GetSubscription) produce the exact sentinel "not a
room member" used by shipped behavior.
---
Duplicate comments:
In
`@docs/superpowers/plans/2026-05-29-room-member-statuses-and-mentionable-subscriptions.md`:
- Around line 20-23: Update the drift note to reflect the actual shipped
sentinel string by replacing the quoted membership error text `"only room
members can perform this action"` with the shipped `"not a room member"`;
reference the shared sentinel errNotRoomMember which is reused across the four
membership-gated RPCs so the wording is RPC-agnostic and must match that
sentinel.
In
`@docs/superpowers/specs/2026-05-29-room-member-statuses-and-mentionable-subscriptions-design.md`:
- Around line 20-21: The spec incorrectly describes a single `botPattern` regex
(\.bot$|^p_) and claims `p_` becomes optionType="app"; update the document to
remove this stale combined contract and instead consistently describe the
shipped split: `.bot` identifies app accounts (optionType="app") and `^p_`
identifies hidden accounts (optionType="hidden"). Replace any references to a
single `botPattern` and any assertions that `p_` maps to "app" with the explicit
split semantics, and update the sections that mention the Go-side and Mongo
pipeline usage as well as the new `docs/client-api.md` RPC docs to reflect `.bot
=> app` and `p_ => hidden` consistently throughout (including all other
occurrences of `botPattern`, `optionType="app"`, and the RPC descriptions).
🪄 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: dabf03fc-48df-4903-bdaf-f0f9ffd8421c
📒 Files selected for processing (16)
docs/client-api.mddocs/superpowers/plans/2026-05-29-room-member-statuses-and-mentionable-subscriptions.mddocs/superpowers/specs/2026-05-29-room-member-statuses-and-mentionable-subscriptions-design.mdpkg/model/member.gopkg/model/model_test.gopkg/model/user.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
✅ Files skipped from review due to trivial changes (2)
- room-service/mock_store_test.go
- docs/client-api.md
🚧 Files skipped from review as they are similar to previous changes (11)
- room-service/store.go
- pkg/model/user.go
- pkg/model/member.go
- pkg/subject/subject_test.go
- room-service/helper.go
- pkg/model/model_test.go
- room-service/handler.go
- room-service/store_mongo.go
- room-service/helper_test.go
- room-service/handler_test.go
- room-service/integration_test.go
Two new per-room read-only NATS RPCs:
- chat.user.{account}.request.room.{roomID}.{siteID}.member.statuses
Returns up to `limit` MemberStatus rows (engName, chineseName,
statusIsShow, statusText) for the room. Default limit is min(3, cap)
where cap is room.UserCount. Caller must be a room member.
- chat.user.{account}.request.room.{roomID}.{siteID}.subscription.mentionable
Returns up to `limit` MentionableSubscription rows for @-mention
autocomplete. Discriminated union: human accounts → HRInfo, ".bot"
accounts → App + Assistant. Platform-admin ("p_") accounts are
filtered out at the Mongo $match stage and never appear as mention
targets. The caller is excluded server-side. Filter is a case-
insensitive regex match against the dash-joined keyword (account,
engName, chineseName, app.name, app.assistant.name); regex
metacharacters in the filter are escaped via regexp.QuoteMeta.
Default limit is min(3, cap) where cap is room.UserCount +
room.AppCount.
Wire contracts (pkg/model + pkg/subject):
- pkg/model: MemberStatus, ListMemberStatusesRequest/Response,
MentionableSubscription discriminated union (user|app),
ListMentionableSubscriptionsRequest/Response, MentionableHRInfo,
MentionableApp, MentionableAppAssistant.
- pkg/model/User: new StatusIsShow + StatusText fields surfaced
through the member.statuses projection.
- pkg/subject: MemberStatuses / MemberStatusesWildcard and
MentionableSubscriptions / MentionableSubscriptionsWildcard
builders.
Both handlers use:
- requireMembershipAndGetRoom: sync.WaitGroup-based parallel
GetSubscription + GetRoom with post-Wait precedence — membership-
not-found wins over racing GetRoom errors, preventing context.
Canceled from masking the not-member signal.
- Post-join $limit so orphan subscriptions (user doc hard-deleted)
don't silently shrink the result below the requested limit.
- errcode-typed errors + errnats.Reply at the boundary, matching
the pattern established in PR #250.
No outbox / cross-site federation: both RPCs are scoped to the
room's owning siteID and read-only.
Includes unit tests (table-driven, full membership/limit/filter
coverage) and integration tests against testcontainers Mongo.
- docs/client-api.md: wire reference for the two new room-scoped RPCs, including subject patterns, request/response shape, the discriminated union for mentionable subscriptions (user vs. app), error cases, and the platform-admin (p_) filtering rule. - docs/superpowers/specs: design doc for the feature, covering the membership-gate precedence, post-join $limit rationale, filter regex escaping, and the choice not to publish to outbox. - docs/superpowers/plans: implementation plan with TDD checkpoints.
- Plan drift-note "shared membership-error text is X, not Y": the X side
was a stray duplicate of Y. Set X to the actual shipped string
("only room members can perform this action") that errNotRoomMember
uses across the four membership-gated RPCs.
- Plan/spec mentionable default-limit snippets still showed `1` in:
- test case name + ListMentionableSubscriptions(... , 1) mock call
- store-error test mock call
- request-body table entry
- drift-grep checklist
- spec test catalogue
Updated all to `3` so plan/spec match the shipped default.
vjauhari-work
force-pushed
the
claude/zen-volta-mZ7G4
branch
from
b7d80fc to
76a71b5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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
room-servicefor the message-composer experience:member.statuses(lists room members with theirengName/chineseName/statusIsShow/statusText) andsubscription.mentionable(@-mention autocomplete returning a discriminateduser | appunion with a regex-escaped substring filter over a 5-field keyword).GetSubscription+GetRoomviaerrgroup(membership error takes precedence on conflict), emitroom.id/site.idOTel span attributes, and clamp a missinglimitto the room cap (or short-circuit empty when the room is empty). Thementionablepipeline hidesp_platform-admin accounts entirely; themember.statusespipeline applies$limitafter the user-join so orphan subscriptions in the prefix don't cause silent under-delivery.model.User.IsBot()/model.User.IsPlatformAdmin()— narrows the predicate so only.bot-suffixed accounts are bots andp_accounts are platform admins (no longer conflated). Drops the package-levelpkg/model.IsBotAccountand consolidates allroom-serviceandroom-workercall sites onto the methods.Supporting changes
pkg/model/member.go: newMemberStatus,MentionableSubscription(+ innerHRInfo/App/AppAssistant), and request/response wrapper types.pkg/model/user.go: newStatusIsShow,StatusTextfields onUser;IsBot()/IsPlatformAdmin()methods.pkg/subject/subject.go: newMemberStatuses/MemberStatusesWildcard+MentionableSubscriptions/MentionableSubscriptionsWildcardbuilders.room-service/store.go+store_mongo.go: newListMemberStatusesandListMentionableSubscriptionsstore methods; sharedbotAccountRegex+platformAdminRegexconstants for the Mongo$regexMatchstages.room-service/helper.go: two new sentinel errors (errMemberStatusesLimitInvalid,errMentionableLimitInvalid) wired intosanitizeError; the sharederrNotRoomMembertext is generalized to"not a room member"now that four handlers reuse it.docs/client-api.md: full request/response/error documentation for both RPCs.Test plan
make lint— 0 issuesmake test— unit tests green with-race(12 + 13 handler subtests, plus pkg/model + pkg/subject + room-worker)TestBotAndAdminPredicate_GoAndMongoAgree_Integration) locks Go predicates against the Mongo regex on 9 probes (.bot,p_, look-alikes, case sensitivity, boundary cases)make test-integration SERVICE=room-service— runs in CI (Docker not available locally); coversListMemberStatuses(5 subtests incl. post-join$limitunder-delivery regression) andListMentionableSubscriptions(8 subtests incl.p_exclusion + orphan-row null-leaf safety)make sast— runs in CI per CLAUDE.md §5https://claude.ai/code/session_01P23mygfBErAmFFMd1VVtWr
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests