Add user-service (NATS request/reply) + consolidate user endpoints + frontend migration#279
Add user-service (NATS request/reply) + consolidate user endpoints + frontend migration#279amdla wants to merge 13 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (63)
💤 Files with no reviewable changes (4)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (45)
📝 WalkthroughWalkthroughConsolidates subscription RPCs into a single subscription.list, adds a new user-service implementation (config, service, mongorepo, roomclient, publisher), updates frontend callsites/tests and loadgen, removes mock-user-service, and updates docs and deployment manifests. ChangesUser-service rollout and contract migration
Sequence Diagram(s)sequenceDiagram
participant ChatFrontend
participant UserService
participant Mongorepo
participant RoomClient
participant Publisher
ChatFrontend->>UserService: subscription.list / subscription.count / subscription.setAppSubscription / status.set / apps.list
UserService->>Mongorepo: query or update user, subscription, app data
UserService->>RoomClient: GetRoomsInfo or CreateDMRoom RPC
UserService->>Publisher: publish UserStatusUpdated event to outbox subjects
UserService-->>ChatFrontend: reply payload or typed error
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (2)
user-service/mongorepo/store_test.go (1)
19-25: ⚡ Quick winAssert concrete index definitions instead of minimum counts.
This test can still pass if an expected index is removed but another unrelated index exists. Consider asserting index names (or key specs) explicitly for
subscriptionsandusers.🤖 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 `@user-service/mongorepo/store_test.go` around lines 19 - 25, The test currently only checks minimum index counts for the subscriptions and users index lists (variables idx and uidx), which can miss missing specific indexes; update the assertions to verify the exact index definitions instead: after loading idx and uidx (from s.subscriptions.Raw().Indexes().List and s.users.Raw().Indexes().List via ucur), assert that the expected index entries exist by checking either the index "name" values or the "key" specs (e.g., that an index with key {"account": 1} or the known index name is present) rather than using require.GreaterOrEqual; implement this by building a set of found index names/keys from idx/uidx and asserting the expected names/keys are contained.user-service/service/service.go (1)
13-13: ⚡ Quick winAlign mock generation with the repo’s test-mock convention.
Line 13 generates mocks into a non-test file under
service/mocks. Repository guidance for store mocks is to generate test-only mocks (mock_store_test.go) and regenerate viamake generate.As per coding guidelines, “Define the store interface in
store.go… include a//go:generate mockgendirective to auto-generate mocks intomock_store_test.go.”🤖 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 `@user-service/service/service.go` at line 13, Update the mockgen directive so mocks follow the repo convention: move the //go:generate line out of service.go and place it into store.go (where the store interfaces are defined) and change the directive to generate test-only mocks into mock_store_test.go (targeting the UserStore interface); remove or stop generating non-test mocks for RoomClient/EventPublisher from service/mocks so only store mocks are produced via mock_store_test.go as per the project guideline.Source: Coding guidelines
🤖 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 `@chat-frontend/src/context/RoomEventsContext/RoomEventsContext.tsx`:
- Around line 382-385: Update the cold-start fallback comment in
RoomEventsContext.tsx to remove the stale reference to listRooms and instead
state that summaries can be populated from subscription.list results;
specifically, replace the phrase "listRooms may have populated `summaries`" with
wording that `subscription.list` may have already populated `summaries` so the
comment correctly reflects the current bootstrap flow used by the sidebar (see
references to `subscription.list` and the `summaries` variable).
In `@claude_its_a_gift_for_u.txt`:
- Line 534: Remove the stray raw closing tag '</content>' that appears in the
markdown-style guide in claude_its_a_gift_for_u.txt; locate the literal string
"</content>" and delete it so the markdown renders correctly and no extraneous
XML/HTML tag remains in the document.
In `@docs/client-api.md`:
- Line 3118: Update the stale cross-reference in the "Initial key bootstrap on
(re)connect" paragraph to reference the consolidated user-service subscription
endpoints instead of the removed subscription.get* APIs: replace the mention of
`subscription.get*` with the current bootstrap path (e.g.,
`subscription.list`-based bootstrap) and point to §3.4's consolidated
user-service contract; ensure the text clarifies that the initial set of keys
for already-subscribed rooms is delivered via the subscription.list (or
equivalent user-service subscription endpoint) rather than live events only.
In `@docs/superpowers/specs/2026-06-04-user-service-design.md`:
- Around line 336-338: Remove the impossible "Each method first guards the site"
sentence and replace it with a clear statement that site isolation is
structural: explain that the *Pattern builders (e.g.,
UserStatusGetByNamePattern(siteID)) bake siteID as a literal token (only
{account} is captured), so c.Param("siteID") will be empty and per-request
guards are not used; state that service instances subscribe only to subjects
containing their cfg.SiteID so handlers should start directly with
validation/business logic, and note that s.siteID and s.allSiteIDs are used by
publishStatus for cross-site outbox routing (this mirrors the behavior seen in
service.go where no per-request site guard is present).
In `@pkg/model/model_test.go`:
- Around line 3051-3061: The test TestUserStatusUpdated_RoundTrip only covers
the non-nil StatusIsShow path; add a second subtest that constructs src :=
model.UserStatusUpdated with StatusIsShow set to nil (and the same other
fields), roundTrips it into dst using the existing roundTrip helper, and asserts
the deserialized dst.StatusIsShow is nil to lock the omitempty/wire-shape
behavior; update or add assertions consistent with the existing test style so
both the non-nil and nil cases are covered.
In `@tools/loadgen/daily_actions_test.go`:
- Around line 75-78: The test currently asserts the raw JSON string for the
subscription.list payload; change it to unmarshal c.reqs[0].Data into the typed
request struct from pkg/model (e.g. model.SubscriptionListRequest or the struct
used for subscription.list) and assert the struct equals the expected value
(Type: "rooms") instead of using require.JSONEq; update the assertion after
refreshRoomList(ctx, u) so it decodes c.reqs[0].Data with encoding/json into the
model struct and uses require.Equal/require.EqualValues against the expected
model struct, leaving subject.UserSubscriptionList("user-1", "site-test") check
intact.
In `@tools/loadgen/daily_actions.go`:
- Around line 94-99: Replace the ad-hoc map payload with a typed request struct
from pkg/model (e.g. add or use pkg/model.SubscriptionListRequest with a Type
string field), then marshal that struct instead of the map before calling
a.Request; update the code in daily_actions.go where payload is created
(currently json.Marshal(map[string]string{"type":"rooms"})) to
json.Marshal(model.SubscriptionListRequest{Type: "rooms"}), add the struct in
pkg/model if missing, and add the necessary import for pkg/model so the
compile-time contract is enforced.
In `@user-service/models/subscription_test.go`:
- Around line 81-106: TestCountRequest_RoundTrip lacks a case for a non-nil
false pointer; add a subtest under TestCountRequest_RoundTrip that constructs in
:= CountRequest{Unread: ptr(false)}, marshals and unmarshals it, asserts
equality with in, and specifically checks out.Unread is non-nil and *out.Unread
== false to ensure the JSON contains "unread": false; reference the
TestCountRequest_RoundTrip function and the CountRequest.Unread field when
adding this new subtest.
In `@user-service/mongorepo/apps_test.go`:
- Around line 21-26: Add a collision seed with the same subscription name but a
different roomType and assert it doesn't affect the botDM-only check: use the
existing seed helper to insert another document for user "u-alice" with "name":
"helper.bot" and "roomType": "channel" (or similar) into the "subscriptions"
collection, then call the same IsSubscribed check (the function/method under
test, e.g., IsSubscribed on the service `s`) and assert the result still
reflects only the botDM subscription; repeat the same change for the other test
block referenced (lines 55-63).
In `@user-service/mongorepo/apps.go`:
- Around line 26-30: In ListApps where the subscription lookup pipeline builds
the $match $expr (the bson.A $and array used in the pipeline variable), add a
condition to require the subscription's roomType equals "botDM" so isSubscribed
only flips for bot DM subscriptions; update the bson.M{"$and": bson.A{...}} to
include a bson.M{"$eq": bson.A{"$roomType", "botDM"}} (or equivalent constant)
alongside the existing account/name/isSubscribed checks so the subscription
match is restricted to botDM records.
In `@user-service/mongorepo/users_test.go`:
- Around line 53-79: The two subtests in TestSetUserStatus_Integration are
sharing mutable state; make them independent by seeding the user state
separately for each t.Run (or resetting the store) before calling
SetUserStatus/GetUserStatus. Specifically, inside each subtest body re-run
seed(...) to create a fresh document for account "bob" with the exact initial
fields you need (first subtest: initial StatusIsShow nil/false as appropriate;
second subtest: initial StatusIsShow true) so SetUserStatus and GetUserStatus
operate on an isolated state; ensure you use the same store instance s and
context ctx but avoid relying on the first subtest to set status for the second.
In `@user-service/publisher/publisher.go`:
- Around line 19-20: The Publisher.Publish method currently returns the raw
error from the transport call p.nc.Publish; change it to wrap that error with
contextual text (e.g., using fmt.Errorf("publish to %s failed: %w", subject,
err)) so callers see the operation boundary and subject; update the Publish
function to capture the error from p.nc.Publish and return a wrapped error when
non-nil.
In `@user-service/service/service.go`:
- Around line 69-77: The handler registration is using the passed-in siteID
instead of the service's configured site ID; update RegisterHandlers to use the
service configuration (e.g., s.cfg.SiteID or s.SiteID depending on your struct)
when calling natsrouter.Register and natsrouter.RegisterNoBody (function:
RegisterHandlers), replacing occurrences of the local siteID variable with the
configured site ID; optionally remove the siteID parameter from RegisterHandlers
to avoid future divergence and update all call sites accordingly.
In `@user-service/service/status.go`:
- Line 66: The publish-failure log call using slog.Error should include the
request/correlation ID from the current context: extract the ID from ctx (using
your existing helper like RequestIDFromContext(ctx) or ctx.Value("request_id")
if available), handle a missing value with a sensible default, and add it as an
explicit field (e.g. "request_id" or "correlation_id") to the slog.Error call
alongside the existing "error", "dest", and "account" fields so the log line is
traceable across services; update the logging call that currently passes err,
dest, account to include this ID and ensure the extraction is safe when ctx is
nil.
In `@user-service/service/subscriptions.go`:
- Around line 212-213: The unread count is undercounted because
GetActiveSubscriptions is called with s.maxSubs instead of the already-computed
total; change the call to s.store.GetActiveSubscriptions(ctx, account, total)
(or otherwise use the precomputed total variable) so that subs is fetched using
the correct cap used for unread calculation (ensure the existing variable named
total is in scope where subs is requested and remove/replace s.maxSubs there).
---
Nitpick comments:
In `@user-service/mongorepo/store_test.go`:
- Around line 19-25: The test currently only checks minimum index counts for the
subscriptions and users index lists (variables idx and uidx), which can miss
missing specific indexes; update the assertions to verify the exact index
definitions instead: after loading idx and uidx (from
s.subscriptions.Raw().Indexes().List and s.users.Raw().Indexes().List via ucur),
assert that the expected index entries exist by checking either the index "name"
values or the "key" specs (e.g., that an index with key {"account": 1} or the
known index name is present) rather than using require.GreaterOrEqual; implement
this by building a set of found index names/keys from idx/uidx and asserting the
expected names/keys are contained.
In `@user-service/service/service.go`:
- Line 13: Update the mockgen directive so mocks follow the repo convention:
move the //go:generate line out of service.go and place it into store.go (where
the store interfaces are defined) and change the directive to generate test-only
mocks into mock_store_test.go (targeting the UserStore interface); remove or
stop generating non-test mocks for RoomClient/EventPublisher from service/mocks
so only store mocks are produced via mock_store_test.go as per the project
guideline.
🪄 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: 5e9c5ed7-0650-4124-8b68-f0329807f601
📒 Files selected for processing (64)
CLAUDE.mdchat-frontend/src/api/_transport/subjects.test.jschat-frontend/src/api/_transport/subjects.tschat-frontend/src/api/fetchSidebarBuckets/index.tschat-frontend/src/api/types.tschat-frontend/src/context/RoomEventsContext/RoomEventsContext.test.jsxchat-frontend/src/context/RoomEventsContext/RoomEventsContext.tsxchat-frontend/src/context/RoomKeysContext/RoomKeysContext.tsxclaude_its_a_gift_for_u.txtdocs/client-api.mddocs/superpowers/plans/2026-06-04-user-service.mddocs/superpowers/specs/2026-06-04-user-service-design.mddocs/user-service-endpoint-consolidation.mdmock-user-service/deploy/Dockerfilemock-user-service/handler.gomock-user-service/handler_test.gomock-user-service/main.gopkg/errcode/codes_test.gopkg/errcode/codes_user.gopkg/errcode/codes_user_test.gopkg/model/event.gopkg/model/model_test.gopkg/model/subscription.gopkg/model/user.gopkg/subject/subject.gopkg/subject/subject_test.gotools/loadgen/daily_actions.gotools/loadgen/daily_actions_test.gouser-service/config/config.gouser-service/config/config_test.gouser-service/deploy/Dockerfileuser-service/deploy/azure-pipelines.ymluser-service/deploy/docker-compose.ymluser-service/main.gouser-service/models/app.gouser-service/models/app_test.gouser-service/models/status.gouser-service/models/status_test.gouser-service/models/subscription.gouser-service/models/subscription_test.gouser-service/mongorepo/apps.gouser-service/mongorepo/apps_test.gouser-service/mongorepo/main_test.gouser-service/mongorepo/setup_test.gouser-service/mongorepo/store.gouser-service/mongorepo/store_test.gouser-service/mongorepo/subscriptions.gouser-service/mongorepo/subscriptions_test.gouser-service/mongorepo/users.gouser-service/mongorepo/users_test.gouser-service/publisher/publisher.gouser-service/publisher/publisher_integration_test.gouser-service/roomclient/client.gouser-service/roomclient/client_integration_test.gouser-service/service/apps.gouser-service/service/apps_test.gouser-service/service/enrich_test.gouser-service/service/mocks/mock_repository.gouser-service/service/service.gouser-service/service/service_test.gouser-service/service/status.gouser-service/service/status_test.gouser-service/service/subscriptions.gouser-service/service/subscriptions_test.go
💤 Files with no reviewable changes (4)
- mock-user-service/deploy/Dockerfile
- mock-user-service/handler.go
- mock-user-service/handler_test.go
- mock-user-service/main.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/superpowers/specs/2026-06-04-user-service-design.md (1)
546-554:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the
siteID mismatcherror row; it conflicts with the structural routing model.Line 553 reintroduces a per-request site-check outcome (
NotFound("unknown site")), but this spec now states thatsiteIDis not captured and handlers do not perform site guards. Keep the table aligned with Line 338 to avoid re-implementing the old bug.🤖 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-user-service-design.md` around lines 546 - 554, The table in the reply conditions includes a "siteID mismatch" row that conflicts with the new routing model; remove the `siteID` mismatch → `NotFound("unknown site")` entry so handlers no longer perform per-request site guards and the table stays consistent with the earlier spec (see the same conditions table around Line 338); ensure only the remaining rows (e.g., `type` checks, `getDM` targets, app not found/disabled, DM not found, Mongo error) remain.
🧹 Nitpick comments (1)
docs/superpowers/specs/2026-06-04-user-service-design.md (1)
66-67: ⚡ Quick winDrop the stale
+ checkSitemention in theservice.golayout note.This text is now inconsistent with the updated handler behavior section and can confuse implementers about expected service structure.
🤖 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-user-service-design.md` around lines 66 - 67, Update the layout note that documents service.go to remove the outdated "+ checkSite" mention: edit the "service.go # UserService struct, New(), RegisterHandlers; interfaces + mockgen + checkSite" line so it no longer references "+ checkSite", leaving only the current items (e.g., "interfaces + mockgen") to match the updated handler behavior; ensure the note matches the actual responsibilities of UserService, New(), and RegisterHandlers and update any adjacent wording if necessary to avoid future confusion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/superpowers/specs/2026-06-04-user-service-design.md`:
- Around line 546-554: The table in the reply conditions includes a "siteID
mismatch" row that conflicts with the new routing model; remove the `siteID`
mismatch → `NotFound("unknown site")` entry so handlers no longer perform
per-request site guards and the table stays consistent with the earlier spec
(see the same conditions table around Line 338); ensure only the remaining rows
(e.g., `type` checks, `getDM` targets, app not found/disabled, DM not found,
Mongo error) remain.
---
Nitpick comments:
In `@docs/superpowers/specs/2026-06-04-user-service-design.md`:
- Around line 66-67: Update the layout note that documents service.go to remove
the outdated "+ checkSite" mention: edit the "service.go #
UserService struct, New(), RegisterHandlers; interfaces + mockgen + checkSite"
line so it no longer references "+ checkSite", leaving only the current items
(e.g., "interfaces + mockgen") to match the updated handler behavior; ensure the
note matches the actual responsibilities of UserService, New(), and
RegisterHandlers and update any adjacent wording if necessary to avoid future
confusion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7f44c44-9608-4bb9-bd4c-15286da628af
📒 Files selected for processing (25)
chat-frontend/src/context/RoomEventsContext/RoomEventsContext.tsxdocs/client-api.mddocs/superpowers/specs/2026-06-04-user-service-design.mdpkg/model/event.gopkg/model/model_test.gotools/loadgen/daily_actions.gotools/loadgen/daily_actions_test.gouser-service/config/config.gouser-service/config/config_test.gouser-service/deploy/docker-compose.ymluser-service/main.gouser-service/models/subscription_test.gouser-service/mongorepo/apps.gouser-service/mongorepo/apps_test.gouser-service/mongorepo/store.gouser-service/mongorepo/subscriptions.gouser-service/mongorepo/subscriptions_test.gouser-service/mongorepo/users_test.gouser-service/publisher/publisher.gouser-service/service/metrics.gouser-service/service/mocks/mock_repository.gouser-service/service/service.gouser-service/service/status.gouser-service/service/subscriptions.gouser-service/service/subscriptions_test.go
✅ Files skipped from review due to trivial changes (2)
- chat-frontend/src/context/RoomEventsContext/RoomEventsContext.tsx
- user-service/service/mocks/mock_repository.go
🚧 Files skipped from review as they are similar to previous changes (17)
- pkg/model/event.go
- user-service/mongorepo/apps.go
- pkg/model/model_test.go
- user-service/mongorepo/apps_test.go
- tools/loadgen/daily_actions.go
- tools/loadgen/daily_actions_test.go
- user-service/mongorepo/store.go
- user-service/deploy/docker-compose.yml
- user-service/mongorepo/users_test.go
- user-service/main.go
- docs/client-api.md
- user-service/service/status.go
- user-service/mongorepo/subscriptions_test.go
- user-service/models/subscription_test.go
- user-service/service/subscriptions_test.go
- user-service/mongorepo/subscriptions.go
- user-service/service/subscriptions.go
24ec624 to
57bf53c
Compare
Summary
Recreates
user-serviceas a standalone NATS request/reply microservice (no JetStream), consolidates the user-facing endpoint surface, removes the supersededmock-user-service, and migrateschat-frontendonto the new contract.8 endpoints (down from the legacy sprawl):
status.getByName,status.set,subscription.list,subscription.getChannels,subscription.getDM,subscription.count,subscription.setAppSubscription,apps.list.getCurrent/getRooms/getApps→ unifiedsubscription.list(typeparam)subscribeApp/unsubscribeApp→ idempotent PUT-likesubscription.setAppSubscriptionprofile.getByName+ the employee endpoint removedArchitecture
Root
main.go+ unpacked packages:config·models(DTOs) ·service(8 handlers + the sharedenrichWithRoomInfo) ·mongorepo(per-collection stores;$facet/Del--filter/room-join pipelines) ·roomclient(room-service/room-worker RPC) ·publisher(core-NATS outbox). Built on history-service's patterns (consumer-defined interfaces + mocks,pkg/natsrouter/mongoutil/errcode/shutdown). Cross-site status federation is fire-and-forget core-NATS to all configured sites.Subscription replies are room-info enriched
list/getChannels/getDMbodies getname/lastMsgAt/alert/hasMentioncomputed from room-service via per-site parallelGetRoomsInfo(per-site degradation; deletedDel-rooms filtered;userCount/lastMsgIdfor local rooms only).subscription.countuses errgroup all-or-nothing fallback-to-total — deliberately distinct from enrichment's per-site degradation.Testing
-race):service96.2%,config100%, whole repo green.bson:",inline"on embedded structs (Mongo decoded embedded fields empty/nil) and a_updatedAt→updatedAtfield-name error.gosecclean on new code.make lint0 issues.make generateidempotent.npm run typecheckclean, 601 tests pass.Schema decisions (confirmed with product)
HRInfo.Name← user'schineseName;engNameis the English name.updatedWithinDaysfilters the subscription doc's realupdatedAtfield.activefilter is{$ne: false}— the (scheduled)activefield being absent counts as active; only explicitactive:falsedrops the user.CI notes
govulncheckandsemgrepcould not run in the dev sandbox (vuln-DB / registry network-blocked) — they run in thesastCI gate.docs/client-api.md+CLAUDE.mdupdated for the 8 endpoints, new reason codes, enrichment behaviors, and theUserStatusUpdatedevent.https://claude.ai/code/session_016ptgodFMgtsDaEnzL15TMG
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Deprecations