Migrate room-service & room-worker RPCs to pkg/natsrouter#274
Open
vjauhari-work wants to merge 3 commits into
Open
Migrate room-service & room-worker RPCs to pkg/natsrouter#274vjauhari-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:
📝 WalkthroughWalkthroughThis PR migrates room-service (18 RPCs) and a room-worker RPC to pkg/natsrouter: adds strict RequireRequestID middleware, typed request/response models, subject-pattern builders, converts handlers to typed natsrouter signatures, updates main shutdown wiring, and refactors unit/integration tests and client API docs. ChangesNATS Router Migration: room-service & room-worker
Sequence DiagramsequenceDiagram
participant Client
participant RoomServiceRouter
participant RequireRequestID
participant RoomHandler
participant StreamPublisher
Client->>RoomServiceRouter: publish request (subject + body + X-Request-ID)
RoomServiceRouter->>RequireRequestID: validate X-Request-ID
RequireRequestID->>RoomHandler: invoke typed handler with context
RoomHandler->>StreamPublisher: publish canonical stream request (when applicable)
RoomHandler-->>RoomServiceRouter: return typed response (Status/CreateRoomReply)
RoomServiceRouter-->>Client: marshal & reply
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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: 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-worker/integration_test.go`:
- Around line 1153-1156: The test reuses a single mutable natsrouter.Context
(ctx) across multiple handler.serverCreateDM calls which can leak state; create
a fresh natsrouter.Context for each simulated delivery/replay invocation and
pass that new context into serverCreateDM (e.g., newCtx1 :=
natsrouter.NewContext(...) / construct a fresh context per call and use
handler.serverCreateDM(newCtx1, req), then newCtx2 for the next call). Apply the
same change for the other duplicated invocations (the pairs around the regions
noted for lines 1188-1189 and 1216-1217) so each serverCreateDM call gets its
own independent context.
🪄 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: eefd33c9-8107-4125-954a-cbb9ed904ac6
📒 Files selected for processing (20)
docs/client-api.mddocs/superpowers/plans/2026-06-04-room-service-natsrouter-migration.mddocs/superpowers/specs/2026-06-04-room-service-natsrouter-migration-design.mdpkg/model/event.gopkg/model/model_test.gopkg/natsrouter/middleware.gopkg/natsrouter/middleware_test.gopkg/subject/subject.gopkg/subject/subject_test.goroom-service/handler.goroom-service/handler_test.goroom-service/helper.goroom-service/integration_test.goroom-service/main.goroom-service/memberlist_client.goroom-service/store.goroom-worker/handler.goroom-worker/handler_test.goroom-worker/integration_test.goroom-worker/main.go
vjauhari-work
force-pushed
the
claude/zen-brown-Atb7v
branch
2 times, most recently
from
40e25ec to
3faf8e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-06-04-room-service-natsrouter-migration.md`:
- Around line 121-137: The example calls errnats.Reply(c, c.Msg, err)
unconditionally which panics when c.Msg is nil (test contexts); update
RequireRequestID to check c.Msg != nil before invoking errnats.Reply and
otherwise handle the error without replying (e.g., log the error on c.ctx or
c.Logger and call c.Abort()), ensuring you only call errnats.Reply when c.Msg is
non-nil.
In
`@docs/superpowers/specs/2026-06-04-room-service-natsrouter-migration-design.md`:
- Around line 82-94: The example in RequireRequestID replies with c.Msg
unconditionally on error which can panic when c.Msg is nil; update the error
path to guard the reply with a nil check (e.g. if c.Msg != nil {
errnats.Reply(c, c.Msg, err) }) before calling c.Abort(), keeping the rest of
the logic and the call to natsutil.RequireRequestID unchanged.
🪄 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: 5c88f297-1246-41bf-8583-ab986f5b7664
📒 Files selected for processing (20)
docs/client-api.mddocs/superpowers/plans/2026-06-04-room-service-natsrouter-migration.mddocs/superpowers/specs/2026-06-04-room-service-natsrouter-migration-design.mdpkg/model/event.gopkg/model/model_test.gopkg/natsrouter/middleware.gopkg/natsrouter/middleware_test.gopkg/subject/subject.gopkg/subject/subject_test.goroom-service/handler.goroom-service/handler_test.goroom-service/helper.goroom-service/integration_test.goroom-service/main.goroom-service/memberlist_client.goroom-service/store.goroom-worker/handler.goroom-worker/handler_test.goroom-worker/integration_test.goroom-worker/main.go
✅ Files skipped from review due to trivial changes (2)
- room-service/store.go
- room-service/memberlist_client.go
🚧 Files skipped from review as they are similar to previous changes (13)
- pkg/subject/subject_test.go
- room-worker/main.go
- pkg/natsrouter/middleware_test.go
- room-service/main.go
- pkg/model/event.go
- pkg/model/model_test.go
- docs/client-api.md
- pkg/subject/subject.go
- room-worker/integration_test.go
- pkg/natsrouter/middleware.go
- room-service/integration_test.go
- room-worker/handler_test.go
- room-service/handler.go
vjauhari-work
force-pushed
the
claude/zen-brown-Atb7v
branch
from
3faf8e3 to
bf175f0
Compare
GITMateuszCharczuk
approved these changes
Jun 5, 2026
Collaborator
GITMateuszCharczuk
left a comment
GITMateuszCharczuk
left a comment
There was a problem hiding this comment.
Correct migration, everything checks out. No issues.
vjauhari-work
force-pushed
the
claude/zen-brown-Atb7v
branch
from
bf175f0 to
3d2cb62
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-06-04-room-service-natsrouter-migration.md`:
- Line 5: The plan's migration counts are out of sync: include the additional
typed routes/builders `member.statuses` and `subscription.mentionable` in the
scope and update the overall RPC/pattern totals (previously "18 RPCs / 15 room
patterns") wherever those totals are stated; search for references to
`member.statuses`, `subscription.mentionable`, `room-service`,
`natsServerCreateDM`, and `pkg/natsrouter` in the doc and increment the
checklist numbers and any numbered ranges to reflect the true count so the
migration checklist and all mentions match the actual wired surface.
In
`@docs/superpowers/specs/2026-06-04-room-service-natsrouter-migration-design.md`:
- Around line 121-123: The spec uses the placeholder `{org}` but the
code/handler contract uses `{orgID}`; update this section (and any other
occurrences in the file) to use `{orgID}` so the token names are consistent with
the implemented pattern and future c.Param(...) wiring, and ensure the example
subject names/subject_test.go cases for RoomsInfoBatchSubscribe, RoomKeyEnsure
and RoomRestricted are described using `{orgID}` rather than `{org}`.
🪄 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: d25d8a2a-cb82-4eb0-bdc0-db4d513d2356
📒 Files selected for processing (20)
docs/client-api.mddocs/superpowers/plans/2026-06-04-room-service-natsrouter-migration.mddocs/superpowers/specs/2026-06-04-room-service-natsrouter-migration-design.mdpkg/model/event.gopkg/model/model_test.gopkg/natsrouter/middleware.gopkg/natsrouter/middleware_test.gopkg/subject/subject.gopkg/subject/subject_test.goroom-service/handler.goroom-service/handler_test.goroom-service/helper.goroom-service/integration_test.goroom-service/main.goroom-service/memberlist_client.goroom-service/store.goroom-worker/handler.goroom-worker/handler_test.goroom-worker/integration_test.goroom-worker/main.go
✅ Files skipped from review due to trivial changes (3)
- room-service/store.go
- room-service/memberlist_client.go
- docs/client-api.md
🚧 Files skipped from review as they are similar to previous changes (12)
- pkg/subject/subject_test.go
- pkg/model/event.go
- pkg/natsrouter/middleware.go
- room-worker/main.go
- pkg/natsrouter/middleware_test.go
- room-service/main.go
- pkg/model/model_test.go
- room-worker/integration_test.go
- room-worker/handler.go
- room-service/helper.go
- room-service/integration_test.go
- room-worker/handler_test.go
vjauhari-work
force-pushed
the
claude/zen-brown-Atb7v
branch
from
3d2cb62 to
a374fca
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
`@docs/superpowers/specs/2026-06-04-room-service-natsrouter-migration-design.md`:
- Around line 5-6: The doc's totals are inconsistent: reconcile the RPC and
pattern counts between the design spec and implementation plan by auditing every
NATS RPC in room-service and room-worker (including natsServerCreateDM) that are
moving off raw nc.QueueSubscribe + handwritten pattern builders; produce a
single authoritative list mapping each RPC name to its pattern-builder status
(e.g., migrated vs pending) and update the summary counts (total RPCs and total
pattern builders) in the spec text and the per-RPC inventory so the numbers at
lines referenced (including the counts near the 18/15 vs 20/17 discrepancy)
match the actual listed RPC entries and implementation plan.
🪄 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: 27ead81f-bbe8-45fd-ba03-f39a16110c94
📒 Files selected for processing (21)
docs/client-api.mddocs/superpowers/plans/2026-06-04-room-service-natsrouter-migration.mddocs/superpowers/specs/2026-06-04-room-service-natsrouter-migration-design.mdhistory-service/internal/cassrepo/reactions_integration_test.gopkg/model/event.gopkg/model/model_test.gopkg/natsrouter/middleware.gopkg/natsrouter/middleware_test.gopkg/subject/subject.gopkg/subject/subject_test.goroom-service/handler.goroom-service/handler_test.goroom-service/helper.goroom-service/integration_test.goroom-service/main.goroom-service/memberlist_client.goroom-service/store.goroom-worker/handler.goroom-worker/handler_test.goroom-worker/integration_test.goroom-worker/main.go
✅ Files skipped from review due to trivial changes (2)
- room-service/memberlist_client.go
- room-service/store.go
🚧 Files skipped from review as they are similar to previous changes (14)
- pkg/model/event.go
- pkg/model/model_test.go
- pkg/subject/subject_test.go
- pkg/natsrouter/middleware.go
- pkg/natsrouter/middleware_test.go
- docs/client-api.md
- room-service/helper.go
- room-service/main.go
- pkg/subject/subject.go
- room-worker/integration_test.go
- room-worker/main.go
- room-worker/handler_test.go
- room-service/handler.go
- room-service/integration_test.go
vjauhari-work
force-pushed
the
claude/zen-brown-Atb7v
branch
from
79a5c9f to
d5be424
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/superpowers/specs/2026-06-04-room-service-natsrouter-migration-design.md (1)
246-246:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
orgIDconsistently in the org-members contract text.The spec still mixes
org/orgIdwith the{orgID}token used by the pattern-builder contract. Please align these references toorgIDto avoidc.Param(...)naming drift in implementation/tests.Also applies to: 263-266
🤖 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-06-04-room-service-natsrouter-migration-design.md` at line 246, The org-members contract mixes `org`/`orgId` with the `{orgID}` token causing naming drift; update the spec so all references use `orgID` consistently (including the table row for `list-org-members` and the related text around `OrgMembersPattern` and `ListOrgMembersResponse`), ensure the pattern token is `{orgID}` everywhere and any example param mentions `c.Param("orgID")` instead of `org`/`orgId`; also apply the same change to the referenced block around lines 263-266 so the entire org-members contract consistently uses `orgID`.
🤖 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.
Duplicate comments:
In
`@docs/superpowers/specs/2026-06-04-room-service-natsrouter-migration-design.md`:
- Line 246: The org-members contract mixes `org`/`orgId` with the `{orgID}`
token causing naming drift; update the spec so all references use `orgID`
consistently (including the table row for `list-org-members` and the related
text around `OrgMembersPattern` and `ListOrgMembersResponse`), ensure the
pattern token is `{orgID}` everywhere and any example param mentions
`c.Param("orgID")` instead of `org`/`orgId`; also apply the same change to the
referenced block around lines 263-266 so the entire org-members contract
consistently uses `orgID`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6afe9a7-7359-4c43-be66-aba8a8aefbdf
📒 Files selected for processing (21)
docs/client-api.mddocs/superpowers/plans/2026-06-04-room-service-natsrouter-migration.mddocs/superpowers/specs/2026-06-04-room-service-natsrouter-migration-design.mdhistory-service/internal/cassrepo/reactions_integration_test.gopkg/model/event.gopkg/model/model_test.gopkg/natsrouter/middleware.gopkg/natsrouter/middleware_test.gopkg/subject/subject.gopkg/subject/subject_test.goroom-service/handler.goroom-service/handler_test.goroom-service/helper.goroom-service/integration_test.goroom-service/main.goroom-service/memberlist_client.goroom-service/store.goroom-worker/handler.goroom-worker/handler_test.goroom-worker/integration_test.goroom-worker/main.go
✅ Files skipped from review due to trivial changes (1)
- room-service/store.go
🚧 Files skipped from review as they are similar to previous changes (14)
- room-service/memberlist_client.go
- pkg/subject/subject_test.go
- room-service/main.go
- room-worker/main.go
- pkg/model/model_test.go
- history-service/internal/cassrepo/reactions_integration_test.go
- pkg/natsrouter/middleware.go
- pkg/subject/subject.go
- pkg/natsrouter/middleware_test.go
- docs/client-api.md
- room-service/helper.go
- room-worker/handler.go
- room-service/handler.go
- room-service/integration_test.go
…t-api Design spec + implementation plan for migrating room-service (20 RPCs, incl. member.statuses and subscription.mentionable) and room-worker's serverCreateDM onto pkg/natsrouter. client-api.md: rename malformed-body error is now bad_request/"invalid request payload", with a note that the transport layer rejects malformed bodies uniformly, plus a consolidated request-id-required note. https://claude.ai/code/session_01MnoZcdKefMvNcAFi1SkbZZ
…ter migration
- pkg/natsrouter: strict RequireRequestID() middleware (rejects missing/invalid
X-Request-ID, never mints, nil-Msg-safe) + tests.
- pkg/subject: 17 {name} *Pattern builders, proven byte-identical to the
existing *Wildcard subscription subjects.
- pkg/model: StatusReply, StatusWithRequestReply, RoomRenameRequest + round-trips.
https://claude.ai/code/session_01MnoZcdKefMvNcAFi1SkbZZ
Move all 20 room-service request/reply RPCs and room-worker's serverCreateDM off raw nc.QueueSubscribe + hand-written wrappers onto pkg/natsrouter typed handlers; cut both main.go's over to a Router (Recovery -> RequireRequestID -> Logging, router.Shutdown before nc.Drain). Includes member.statuses and subscription.mentionable (added on main in #260). Subjects, request/response JSON, DB calls, and business logic preserved; rename/restricted malformed-body now returns bad_request instead of internal. Also fixes a pre-existing, unrelated CI failure on main: the history-service reactions integration tests referenced a non-existent pinned_at column on messages_by_room and mis-keyed pinned_messages_by_room; aligned both with the actual Cassandra schema. https://claude.ai/code/session_01MnoZcdKefMvNcAFi1SkbZZ
vjauhari-work
force-pushed
the
claude/zen-brown-Atb7v
branch
from
d5be424 to
bdd2bfd
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
3 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
Migrates all 18 room-service NATS request/reply RPC handlers and room-worker's single RPC (
serverCreateDM) off the legacync.QueueSubscribe+ hand-written wrapper +wrappedCtxpattern ontopkg/natsroutertyped handlers.This unblocks per-message concurrency (handlers previously dispatched serially per subscription) and centralizes JSON marshal/unmarshal, error replies, panic recovery, request-ID handling, and logging — bringing both services in line with
search-service/history-service. room-worker's JetStream consumer is intentionally untouched.It's a pure transport/plumbing swap: subjects, request/response JSON, DB calls, and business logic are preserved.
Commits (3, grouped by area)
client-api.md(rename malformed-body error →bad_request/"invalid request payload", plus a §6 note that the transport layer rejects malformed bodies uniformly).RequireRequestID()middleware (rejects missing/invalidX-Request-ID, never mints, nil-Msg-safe); 15{name}*Patternsubject builders, proven byte-identical to the existing*Wildcardsubscriptions;StatusReply/StatusWithRequestReply/RoomRenameRequestmodel types.serverCreateDMconverted to typedRegister/RegisterNoBodyhandlers; bothmain.gos cut over to anatsrouter.Router(Recovery → RequireRequestID → Logging;router.Shutdownbeforenc.Drain).Behavior
bad_request/"invalid request payload"uniformly (was per-handler;rename/restrictedpreviously collapsed tointernal— a latent bug fix). The app-handler response-size cap is preserved.Testing
make lint0 issues · fullmake testgreen · gosec + errcode-semgrep clean.room-serviceok 183s,room-workerok 11s.https://claude.ai/code/session_01MnoZcdKefMvNcAFi1SkbZZ
Generated by Claude Code
Summary by CodeRabbit
Documentation
bad_request/ "invalid request payload"; Create/Rename RoomX-Request-IDcases documented asbad_request/reason: request_id_required. Added migration plan and design for router-based room-service/worker migration.New Features
Bug Fixes
Tests