feat(room-worker): actor-named, count-based membership system messages#419
feat(room-worker): actor-named, count-based membership system messages#419vjauhari-work wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughSys-message rendering now uses chatroom wording, requester-aware removal text, and count-based add messages. Handler paths, persistence coverage, and client docs were updated for typed ChangesSys-message formatting and payload updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/client-api.md`:
- Around line 2335-2356: The docs for sysMsgData are incomplete and should be
updated to match the runtime payloads exposed by model.MembersAdded and the
member_* shapes. Expand the members_added table to include the missing channels
([ChannelRef][]) and addedUsersCount fields, and add a shared named SysMsgUser
schema for the user object so member_removed and member_left reference it
instead of an inline compound type. Also add JSON examples for members_added,
member_removed, and member_left so clients can parse each payload shape
correctly.
In `@history-service/internal/cassrepo/messages_by_room_integration_test.go`:
- Around line 351-373: The round-trip test for `GetMessagesBefore` only covers
populated `MembersAdded` slices, so it does not verify the empty-array
serialization behavior. Extend `messages_by_room_integration_test.go` by adding
a case around `model.MembersAdded` that uses an empty `Individuals` or `Orgs`
slice, then load it through `repo.GetMessagesBefore` and assert the decoded
slice is non-nil and has length 0. Keep the existing
`model.MessageTypeMembersAdded` and `json.Unmarshal` checks so the test locks
down the schema change for empty collections too.
In `@room-worker/handler_test.go`:
- Around line 4173-4193: The test for publishChannelSysMessages currently only
checks that MessageTypeMembersAdded is not emitted, so it can still pass if
nothing is published at all. Update
TestHandler_PublishChannelSysMessages_EmptySkipsMembersAdded to also assert that
a room_created event is emitted by publishChannelSysMessages for the create-room
path, using the existing published slice and model.MessageEvent checks alongside
the current members_added negative assertion.
- Around line 4116-4171: The tests for publishChannelSysMessages only verify the
populated side of model.MembersAdded and miss the empty-slice contract, so add
assertions in the existing create-room cases to check that users-only publishes
a non-nil empty Orgs slice and orgs-only publishes a non-nil empty Individuals
slice. Use the existing TestHandler_PublishChannelSysMessages_UsersOnly and
TestHandler_PublishChannelSysMessages_OrgsOnly cases, and validate the
unmarshaled payload fields in the same spot where payload.Individuals is already
checked, so regressions to null or omitted empty collections are caught.
🪄 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: a5ff251b-328f-4856-b453-7bc9f158e7c7
📒 Files selected for processing (8)
docs/client-api.mdhistory-service/internal/cassrepo/messages_by_room_integration_test.gopkg/model/member.goroom-worker/handler.goroom-worker/handler_test.goroom-worker/integration_test.goroom-worker/sysmsg.goroom-worker/sysmsg_test.go
78a63c8 to
8033878
Compare
|
Addressed CodeRabbit's review (force-pushed as
Not applied: the Docstring Coverage pre-merge warning (47.62% < 80%). This repo's conventions (CLAUDE.md §3) mandate minimal comments (≤2 lines, why-not-what); adding boilerplate docstrings to satisfy the threshold would conflict with that, and it's an advisory check, not a blocking gate. Verified: Generated by Claude Code |
c665713 to
9d5f44a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
room-worker/handler_test.go (1)
4032-4306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a shared test-setup helper to cut boilerplate.
Nearly every new test in this range (Lines 4032-4306) repeats the same
gomock.NewController+NewMockSubscriptionStore+published []publishedMsg+&Handler{...}construction. A helper mirroringnewCreateRoomTestHandler(used at Line 2333) for the add/remove-member tests would cut duplication and make future additions less error-prone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@room-worker/handler_test.go` around lines 4032 - 4306, The add-member and channel sys-message tests repeat the same controller/store/published-handler setup, so extract that boilerplate into a shared helper like the existing create-room test helper. Add a helper near these tests that builds the gomock controller, NewMockSubscriptionStore, published slice, and Handler with the common siteID/keyStore/keySender wiring, then update the affected tests to use it so future add/remove-member cases stay consistent and easier to maintain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@room-worker/handler_test.go`:
- Around line 4107-4156: The add-members sys-msg test in
TestHandler_ProcessAddMembers_RequesterExcludedFromSysMsg should also verify the
empty Orgs contract. Extend the payload assertions after unmarshalling
model.MembersAdded to check that payload.Orgs is non-nil and empty, matching the
same nonNil serialization behavior already covered in the
PublishChannelSysMessages tests. This keeps the processAddMembers path aligned
with the existing members_added payload contract alongside Individuals and
Channels.
- Around line 4252-4275: The test in
TestHandler_PublishChannelSysMessages_OrgsOnly hard-codes the added users count,
which leaves the emitted sysmsg payload field unverified. Replace the fixed
value passed into publishChannelSysMessages with a scenario-derived count, or
add an assertion on payload.AddedUsersCount after unmarshalling the MembersAdded
payload, so the test checks the actual value written by
Handler.publishChannelSysMessages.
---
Nitpick comments:
In `@room-worker/handler_test.go`:
- Around line 4032-4306: The add-member and channel sys-message tests repeat the
same controller/store/published-handler setup, so extract that boilerplate into
a shared helper like the existing create-room test helper. Add a helper near
these tests that builds the gomock controller, NewMockSubscriptionStore,
published slice, and Handler with the common siteID/keyStore/keySender wiring,
then update the affected tests to use it so future add/remove-member cases stay
consistent and easier to maintain.
🪄 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: ae734fa2-207a-47d2-91fc-3929229499a3
📒 Files selected for processing (9)
docs/client-api.mdhistory-service/internal/cassrepo/messages_by_room_integration_test.gopkg/model/member.goroom-service/handler.goroom-worker/handler.goroom-worker/handler_test.goroom-worker/integration_test.goroom-worker/sysmsg.goroom-worker/sysmsg_test.go
✅ Files skipped from review due to trivial changes (3)
- room-service/handler.go
- pkg/model/member.go
- room-worker/integration_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- history-service/internal/cassrepo/messages_by_room_integration_test.go
- docs/client-api.md
- room-worker/sysmsg.go
- room-worker/handler.go
- room-worker/sysmsg_test.go
Make create-channel / add-members / remove-member system messages
actor-named and count-based so the frontend can render clickable lists of
who was added/removed:
- members_added (add + create): "Alice added 2 people and 1 organization to
the chatroom", with a single shared addedContent helper used by both flows;
a lone individual (no orgs) still renders the named form. Payload lists are
the resolved member set — req.Users for add, req.ResolvedUsers for create
(channel-expanded individuals included, org members represented as orgs).
An empty channel emits no members_added (room_created still fires).
- member_removed / member_left: actor-named removals
("Alice removed Bob from the chatroom"); wording "channel" -> "chatroom"
across all membership messages.
- Empty individuals/orgs lists marshal as [] (not null) for the frontend.
- docs/client-api.md: document the members_added / member_removed /
member_left sysMsgData payload schema.
- history-service: integration test asserting sysMsgData survives the
load-history round-trip.
No pkg/model struct changes, no room-service/materialization changes, and no
new database reads.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WuJMTQH2mnUqKd6jhSg8HK
9d5f44a to
089f467
Compare
Summary
Membership system messages (create-channel / add-members / remove-member) are now actor-named and count-based so the frontend can render clickable lists of who was added/removed.
members_added(add + create):"Alice" added 2 people and 1 organization to the chatroom, built by a single sharedaddedContenthelper used by both add-members and create-channel. A lone individual (no orgs) still renders the named form ("Alice" added "Bob" to the chatroom).req.Usersfor add,req.ResolvedUsersfor create — so channel-expanded individuals are included and org members are represented as orgs, never as people.members_added(onlyroom_created).individuals/orgsalways marshal as[](nevernull).member_removed/member_left: removals are actor-named ("Alice" removed "Bob" from the chatroom); wording changed from "channel" to "chatroom" across all membership messages.docs/client-api.md: documents themembers_added/member_removed/member_leftsysMsgDatapayload schema (the clickable-list wire contract).sysMsgDatasurvives the load-history round-trip (verified against real Cassandra).No
pkg/modelstruct changes, no room-service / materialization changes, and no new database reads.Test Plan
make lint— 0 issuesmake test— unit tests green (whole repo)make test-integration SERVICE=room-worker— pass (real Mongo)make test-integration SERVICE=history-service— pass (real Cassandra;sysMsgDataround-trip)sysMsgDatafor system messages (read scans it;LoadHistorypasses it through untouched)🤖 Generated with Claude Code
https://claude.ai/code/session_01WuJMTQH2mnUqKd6jhSg8HK
Generated by Claude Code
Summary by CodeRabbit
Documentation
sysMsgData, adding type-dependent JSON payload shapes for member-added/removed/left and how to decodeClientMessage.sysMsgData.Bug Fixes
members_addedpayload/content to reflect requester-excluded resolved member/org sets and ensure empty slices serialize as[]and that irrelevant “added” messages are skipped.Tests
sys_msg_dataround-tripping and the new rendered/payload expectations.