feat(user-service): cross-site thread-unread badge RPC#436
Open
yenta wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughAdds a user-service thread unread RPC, a room-service batch thread-room info RPC, and a Mongo-backed thread-subscription read path. The old ThreadUnreadSummary flow is removed, and related docs, tests, and wiring are updated. ChangesThread Unread RPC Implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant UserService
participant ThreadSubscriptionRepo
participant RoomClient
participant RoomService
Client->>UserService: thread.unread request
UserService->>ThreadSubscriptionRepo: ListByAccount(account)
ThreadSubscriptionRepo-->>UserService: []ThreadSubRef
UserService->>RoomClient: GetThreadRoomInfoBatch(siteID, chunked threadRoomIDs)
RoomClient->>RoomService: ThreadRoomInfoBatch RPC
RoomService-->>RoomClient: ThreadRoomInfoBatchResponse
RoomClient-->>UserService: []ThreadRoomInfo or error
UserService-->>Client: ThreadUnreadResponse
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
🧹 Nitpick comments (2)
user-service/roomclient/client_integration_test.go (1)
125-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd missing test parity with
GetRoomsInfocoverage.
TestGetRoomsInfo_Integrationalso covers "no responder" (RPC failure) and "cross-site siteID routing" scenarios, butTestGetThreadRoomInfoBatch_Integrationonly covers happy path and errcode relay. SinceGetThreadRoomInfoBatchhas the identical siteID-routing pattern, mirroring the missing subtests would close a coverage gap on a newly introduced fan-out path used byGetThreadUnread.✅ Suggested additional subtests
t.Run("errcode reply relayed", func(t *testing.T) { nc := dial(t) sub, err := nc.Subscribe(subject.ThreadRoomInfoBatch("site-a"), func(m otelnats.Msg) { data, _ := json.Marshal(errcode.BadRequest("bad")) _ = m.Msg.Respond(data) }) require.NoError(t, err) t.Cleanup(func() { _ = sub.Unsubscribe() }) _, err = New(nc, "site-a").GetThreadRoomInfoBatch(context.Background(), "site-a", []string{"tr1"}) var e *errcode.Error require.True(t, errors.As(err, &e)) assert.Equal(t, errcode.CodeBadRequest, e.Code) }) + + t.Run("no responder — returns error wrapping thread-room-info rpc", func(t *testing.T) { + nc := dial(t) + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + _, err := New(nc, "site-a").GetThreadRoomInfoBatch(ctx, "site-a", []string{"tr1"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "thread-room-info rpc") + }) + + t.Run("cross-site siteID routing — uses siteID param not c.siteID", func(t *testing.T) { + nc := dial(t) + sub, err := nc.Subscribe(subject.ThreadRoomInfoBatch("site-b"), func(m otelnats.Msg) { + out, _ := json.Marshal(model.ThreadRoomInfoBatchResponse{ + Threads: []model.ThreadRoomInfo{{ThreadRoomID: "tr2", Found: true, LastMsgAt: 10, RoomType: model.RoomTypeChannel}}, + }) + _ = m.Msg.Respond(out) + }) + require.NoError(t, err) + t.Cleanup(func() { _ = sub.Unsubscribe() }) + + got, err := New(nc, "site-a").GetThreadRoomInfoBatch(context.Background(), "site-b", []string{"tr2"}) + require.NoError(t, err) + require.Len(t, got, 1) + })🤖 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/roomclient/client_integration_test.go` around lines 125 - 159, `TestGetThreadRoomInfoBatch_Integration` is missing the same coverage that `TestGetRoomsInfo_Integration` already has: add subtests for the no-responder RPC failure case and for cross-site `siteID` routing. Use `New(...).GetThreadRoomInfoBatch`, `subject.ThreadRoomInfoBatch`, and the existing `dial`/`Subscribe` test pattern to verify the request fails when no subscriber exists and that a request sent with one site ID does not hit a subscriber for another site ID.user-service/service/threadunread_test.go (1)
50-66: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a multi-site (2+ sites, mixed success/failure) test case run with
-race.Current failure test only exercises a single site. A test with ≥2 sites (one succeeding, one failing) run under
-racewould directly validate the per-goroutineresults[i]indexing and rule out any loop-variable-capture regression, independent of Go version.🤖 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/threadunread_test.go` around lines 50 - 66, The current GetThreadUnread_SiteFailureDegrades test in threadunread_test.go only covers one site, so add a multi-site case that exercises both a successful and a failing site in the same call. Use threadUnreadService.GetThreadUnread with multiple ThreadSubRef entries and stub RoomClient.GetThreadRoomInfoBatch for at least two site IDs so one returns data and one returns an error, then assert the mixed results and unavailable sites. Run the new test under -race to validate the per-goroutine results[i] handling and guard against loop-variable-capture issues.
🤖 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/store_mongo.go`:
- Around line 1778-1787: The GetThreadRoomInfos lookup is using ListRoomsByIDs,
but that shared helper must keep returning full room documents for other
callers. Update GetThreadRoomInfos to query room ids directly with a projection
that only fetches _id and type, then populate typeByRoom from that result; keep
the bson field name as type to match model.Room.Type.
---
Nitpick comments:
In `@user-service/roomclient/client_integration_test.go`:
- Around line 125-159: `TestGetThreadRoomInfoBatch_Integration` is missing the
same coverage that `TestGetRoomsInfo_Integration` already has: add subtests for
the no-responder RPC failure case and for cross-site `siteID` routing. Use
`New(...).GetThreadRoomInfoBatch`, `subject.ThreadRoomInfoBatch`, and the
existing `dial`/`Subscribe` test pattern to verify the request fails when no
subscriber exists and that a request sent with one site ID does not hit a
subscriber for another site ID.
In `@user-service/service/threadunread_test.go`:
- Around line 50-66: The current GetThreadUnread_SiteFailureDegrades test in
threadunread_test.go only covers one site, so add a multi-site case that
exercises both a successful and a failing site in the same call. Use
threadUnreadService.GetThreadUnread with multiple ThreadSubRef entries and stub
RoomClient.GetThreadRoomInfoBatch for at least two site IDs so one returns data
and one returns an error, then assert the mixed results and unavailable sites.
Run the new test under -race to validate the per-goroutine results[i] handling
and guard against loop-variable-capture issues.
🪄 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: 2074dd2f-ca73-463c-b098-26ee954b480c
📒 Files selected for processing (25)
docs/client-api.mddocs/superpowers/plans/2026-07-01-user-thread-unread-rpc.mddocs/superpowers/specs/2026-07-01-user-thread-unread-rpc-design.mdpkg/model/model_test.gopkg/model/threadsubscription.gopkg/subject/subject.gopkg/subject/subject_test.goroom-service/handler.goroom-service/handler_test.goroom-service/integration_test.goroom-service/mock_store_test.goroom-service/store.goroom-service/store_mongo.gouser-service/main.gouser-service/mongorepo/threadsubscriptions.gouser-service/mongorepo/threadsubscriptions_test.gouser-service/roomclient/client.gouser-service/roomclient/client_integration_test.gouser-service/service/mocks/mock_repository.gouser-service/service/service.gouser-service/service/service_test.gouser-service/service/status_test.gouser-service/service/threads_test.gouser-service/service/threadunread.gouser-service/service/threadunread_test.go
yenta
force-pushed
the
claude/thread-unread-status-rpc-70zmfx
branch
from
5959db3 to
93912e2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
user-service/service/threadunread_test.go (1)
132-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
size <= 0case to coverchunkStrings' invalid-input branch. Theif size <= 0 { size = len(ids) }path inchunkStringsis untested; a case like{name: "non-positive size", ids: []string{"a","b"}, size: 0, expected: [][]string{{"a","b"}}}closes the gap. As per coding guidelines, "Tests must cover: happy path, error paths, edge cases ..., and invalid input".🤖 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/threadunread_test.go` around lines 132 - 163, Add a test case in TestChunkStrings to cover the invalid-input branch in chunkStrings where size <= 0, using a non-positive size and asserting it falls back to returning the full ids slice as a single chunk. Locate the fix in the chunkStrings test table and ensure the new case exercises the existing size normalization behavior alongside the current happy-path cases.Source: Coding guidelines
docs/superpowers/specs/2026-07-01-user-thread-unread-rpc-design.md (1)
136-150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTag the data-flow fence with a language.
The fenced diagram currently has no language tag, which is what markdownlint is flagging.
text(or a plain list) will keep the docs lint-clean.🤖 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-07-01-user-thread-unread-rpc-design.md` around lines 136 - 150, The fenced data-flow diagram under the thread.unread RPC flow is missing a language tag, causing markdownlint to fail. Update the fence around the diagram in the spec so it uses an explicit language like text, or convert it to a plain list, and keep the content unchanged while ensuring the markdown remains lint-clean.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@docs/superpowers/specs/2026-07-01-user-thread-unread-rpc-design.md`:
- Around line 136-150: The fenced data-flow diagram under the thread.unread RPC
flow is missing a language tag, causing markdownlint to fail. Update the fence
around the diagram in the spec so it uses an explicit language like text, or
convert it to a plain list, and keep the content unchanged while ensuring the
markdown remains lint-clean.
In `@user-service/service/threadunread_test.go`:
- Around line 132-163: Add a test case in TestChunkStrings to cover the
invalid-input branch in chunkStrings where size <= 0, using a non-positive size
and asserting it falls back to returning the full ids slice as a single chunk.
Locate the fix in the chunkStrings test table and ensure the new case exercises
the existing size normalization behavior alongside the current happy-path cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c2e645c-89e4-4db2-a194-bc7ee6a853c5
📒 Files selected for processing (25)
docs/client-api.mddocs/superpowers/plans/2026-07-01-user-thread-unread-rpc.mddocs/superpowers/specs/2026-07-01-user-thread-unread-rpc-design.mdpkg/model/model_test.gopkg/model/threadsubscription.gopkg/subject/subject.gopkg/subject/subject_test.goroom-service/handler.goroom-service/handler_test.goroom-service/integration_test.goroom-service/mock_store_test.goroom-service/store.goroom-service/store_mongo.gouser-service/main.gouser-service/mongorepo/threadsubscriptions.gouser-service/mongorepo/threadsubscriptions_test.gouser-service/roomclient/client.gouser-service/roomclient/client_integration_test.gouser-service/service/mocks/mock_repository.gouser-service/service/service.gouser-service/service/service_test.gouser-service/service/status_test.gouser-service/service/threads_test.gouser-service/service/threadunread.gouser-service/service/threadunread_test.go
✅ Files skipped from review due to trivial changes (3)
- docs/client-api.md
- user-service/service/mocks/mock_repository.go
- room-service/mock_store_test.go
🚧 Files skipped from review as they are similar to previous changes (18)
- user-service/main.go
- user-service/service/service_test.go
- user-service/service/status_test.go
- user-service/service/threads_test.go
- user-service/mongorepo/threadsubscriptions_test.go
- user-service/roomclient/client.go
- user-service/roomclient/client_integration_test.go
- user-service/mongorepo/threadsubscriptions.go
- room-service/handler_test.go
- pkg/subject/subject_test.go
- room-service/integration_test.go
- user-service/service/service.go
- user-service/service/threadunread.go
- room-service/store.go
- pkg/subject/subject.go
- pkg/model/model_test.go
- pkg/model/threadsubscription.go
- room-service/handler.go
yenta
force-pushed
the
claude/thread-unread-status-rpc-70zmfx
branch
2 times, most recently
from
41b96e3 to
e9ff324
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/superpowers/plans/2026-07-01-user-thread-unread-rpc.md (1)
27-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnify the subject builder shape.
This plan still introduces
ThreadRoomInfoBatchSubscribe, but the spec says to reuseThreadRoomInfoBatch(siteID)for both publishing and handler registration. Keeping both names will drift the public subject API.🤖 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-07-01-user-thread-unread-rpc.md` around lines 27 - 30, The subject API is inconsistent because the plan adds both ThreadRoomInfoBatch and ThreadRoomInfoBatchSubscribe, but the spec requires a single ThreadRoomInfoBatch(siteID) shape for both publishing and handler registration. Update pkg/subject/subject.go to expose only the unified ThreadRoomInfoBatch builder and have room-service/handler.go registration use that same symbol, while removing the extra ThreadRoomInfoBatchSubscribe reference from the plan and any related call sites.
🤖 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.
Nitpick comments:
In `@docs/superpowers/plans/2026-07-01-user-thread-unread-rpc.md`:
- Around line 27-30: The subject API is inconsistent because the plan adds both
ThreadRoomInfoBatch and ThreadRoomInfoBatchSubscribe, but the spec requires a
single ThreadRoomInfoBatch(siteID) shape for both publishing and handler
registration. Update pkg/subject/subject.go to expose only the unified
ThreadRoomInfoBatch builder and have room-service/handler.go registration use
that same symbol, while removing the extra ThreadRoomInfoBatchSubscribe
reference from the plan and any related call sites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e91b84f-0fa4-4d47-ae5d-fae0ed2fc79f
📒 Files selected for processing (25)
docs/client-api.mddocs/superpowers/plans/2026-07-01-user-thread-unread-rpc.mddocs/superpowers/specs/2026-07-01-user-thread-unread-rpc-design.mdpkg/model/model_test.gopkg/model/threadsubscription.gopkg/subject/subject.gopkg/subject/subject_test.goroom-service/handler.goroom-service/handler_test.goroom-service/integration_test.goroom-service/mock_store_test.goroom-service/store.goroom-service/store_mongo.gouser-service/main.gouser-service/mongorepo/threadsubscriptions.gouser-service/mongorepo/threadsubscriptions_test.gouser-service/roomclient/client.gouser-service/roomclient/client_integration_test.gouser-service/service/mocks/mock_repository.gouser-service/service/service.gouser-service/service/service_test.gouser-service/service/status_test.gouser-service/service/threads_test.gouser-service/service/threadunread.gouser-service/service/threadunread_test.go
✅ Files skipped from review due to trivial changes (2)
- room-service/mock_store_test.go
- user-service/service/mocks/mock_repository.go
🚧 Files skipped from review as they are similar to previous changes (20)
- user-service/mongorepo/threadsubscriptions_test.go
- user-service/mongorepo/threadsubscriptions.go
- user-service/main.go
- user-service/service/service_test.go
- user-service/service/status_test.go
- user-service/service/threads_test.go
- user-service/roomclient/client.go
- user-service/service/threadunread_test.go
- pkg/subject/subject_test.go
- user-service/service/threadunread.go
- room-service/store.go
- docs/client-api.md
- room-service/integration_test.go
- user-service/roomclient/client_integration_test.go
- user-service/service/service.go
- pkg/model/threadsubscription.go
- room-service/handler.go
- room-service/handler_test.go
- pkg/subject/subject.go
- pkg/model/model_test.go
yenta
force-pushed
the
claude/thread-unread-status-rpc-70zmfx
branch
from
e9ff324 to
b7a3391
Compare
Adds a client-facing RPC that returns a per-user thread-unread badge
aggregated across every federation site the user participates in, and
removes the superseded (callerless) ThreadUnreadSummary leaf.
New client RPC: chat.user.{account}.request.user.{siteID}.thread.unread
-> ThreadUnreadResponse{ unread, unreadDirectMessage, unreadMention,
lastMessageAt?, unavailableSites? }.
How it works (mirrors the existing subscription-unread pattern, one
unread() helper): user-service reads all of the user's thread-subs from
the home-site replica (ThreadSubscriptionRepo.ListByAccount), groups them
by the room's home siteId, and per site (uncapped fan-out, chunked by
maxBatchSize) calls the new room-service ThreadRoomInfoBatch RPC
(chat.server.request.room.{siteID}.thread.info.batch) for each thread
room's lastMsgAt + parent roomType. Results fold via unread(): OR the
booleans, max lastMsgAt over found rows, unreadDirectMessage = unread &&
roomType==dm, unreadMention = hasMention. Per-site RPC failures degrade
into unavailableSites. Grouping by siteId counts each thread exactly once.
- pkg/subject: UserThreadUnread/Pattern, ThreadRoomInfoBatch/Subscribe.
- pkg/model: ThreadUnreadRequest/Response, ThreadRoomInfoBatchRequest/
Response, ThreadRoomInfo, ThreadSubRef.
- room-service: GetThreadRoomInfos store method (two projected finds,
no $lookup) + threadRoomInfoBatch handler.
- user-service: roomclient.GetThreadRoomInfoBatch,
mongorepo.ThreadSubscriptionRepo.ListByAccount, RoomClient/
ThreadSubscriptionRepository interfaces, GetThreadUnread aggregator.
- docs/client-api.md: documents the new thread.unread RPC.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BocBvJn2N9YYMmciA5EWJX
yenta
force-pushed
the
claude/thread-unread-status-rpc-70zmfx
branch
from
b7a3391 to
dc0c03e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
user-service/service/threadunread.go (1)
14-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMagic batch-size constant duplicated across services without a shared source of truth.
threadInfoBatchChunk = 500is a local guess at room-service'sMAX_BATCH_SIZE(documented as "default 1000" in the comment). If room-service's configured batch limit is lowered below 500 in some deployment, this constant silently becomes wrong with no compile-time or config-time signal. Consider sourcing this limit from a sharedpkgconstant (or having the room-service batch RPC report/enforce it back to callers) rather than a comment-documented assumption.🤖 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/threadunread.go` around lines 14 - 16, The threadInfoBatchChunk constant in threadunread.go is a duplicated batch-size assumption with no shared source of truth. Update the ThreadRoomInfoBatch call path to use a shared pkg constant or a value provided/enforced by room-service instead of hardcoding 500, and make the batch sizing logic in the relevant thread unread helper(s) derive from that centralized limit so it stays consistent across deployments.
🤖 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-07-01-user-thread-unread-rpc-design.md`:
- Around line 136-147: The ThreadUnreadSummary flow currently limits
thread_subscriptions reads to the newest 500 rows, which can undercount unread
state for accounts with more subscriptions. Update the ListByAccount-based
aggregation in the spec to read all subscriptions for the account, and remove
the repeated 500-row cap wherever it appears in the later components section,
keeping the rest of the unread/group-by-site logic unchanged.
- Around line 63-89: The RPC contract naming in this spec is inconsistent with
the implementation plan: update the subject helpers and model/request-response
names from ThreadUnreadSummary* and thread.unread.summary to ThreadUnread* and
thread.unread throughout the referenced spec sections. Use the existing
identifiers like UserThreadUnreadSummary, UserThreadUnreadSummaryPattern,
ThreadUnreadSummaryRequest, and ThreadUnreadSummaryResponse as the places to
rename so the docs point to one contract only.
---
Nitpick comments:
In `@user-service/service/threadunread.go`:
- Around line 14-16: The threadInfoBatchChunk constant in threadunread.go is a
duplicated batch-size assumption with no shared source of truth. Update the
ThreadRoomInfoBatch call path to use a shared pkg constant or a value
provided/enforced by room-service instead of hardcoding 500, and make the batch
sizing logic in the relevant thread unread helper(s) derive from that
centralized limit so it stays consistent across deployments.
🪄 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: 3aae2764-4f4c-4d93-9b50-095f24cd503f
📒 Files selected for processing (25)
docs/client-api.mddocs/superpowers/plans/2026-07-01-user-thread-unread-rpc.mddocs/superpowers/specs/2026-07-01-user-thread-unread-rpc-design.mdpkg/model/model_test.gopkg/model/threadsubscription.gopkg/subject/subject.gopkg/subject/subject_test.goroom-service/handler.goroom-service/handler_test.goroom-service/integration_test.goroom-service/mock_store_test.goroom-service/store.goroom-service/store_mongo.gouser-service/main.gouser-service/mongorepo/threadsubscriptions.gouser-service/mongorepo/threadsubscriptions_test.gouser-service/roomclient/client.gouser-service/roomclient/client_integration_test.gouser-service/service/mocks/mock_repository.gouser-service/service/service.gouser-service/service/service_test.gouser-service/service/status_test.gouser-service/service/threads_test.gouser-service/service/threadunread.gouser-service/service/threadunread_test.go
✅ Files skipped from review due to trivial changes (1)
- user-service/service/mocks/mock_repository.go
🚧 Files skipped from review as they are similar to previous changes (14)
- user-service/service/service_test.go
- user-service/mongorepo/threadsubscriptions.go
- user-service/service/threads_test.go
- user-service/main.go
- user-service/roomclient/client.go
- room-service/mock_store_test.go
- user-service/service/status_test.go
- room-service/integration_test.go
- room-service/handler_test.go
- user-service/service/threadunread_test.go
- room-service/handler.go
- user-service/roomclient/client_integration_test.go
- room-service/store.go
- room-service/store_mongo.go
Comment on lines
+63
to
+89
| **Subject** (`pkg/subject/subject.go`, mirroring `UserThreadList`): | ||
|
|
||
| ```go | ||
| func UserThreadUnreadSummary(account, siteID string) string { | ||
| if !isValidAccountToken(account) { | ||
| panic("invalid account token: contains NATS wildcard characters") | ||
| } | ||
| return fmt.Sprintf("chat.user.%s.request.user.%s.thread.unread.summary", account, siteID) | ||
| } | ||
| func UserThreadUnreadSummaryPattern(siteID string) string { | ||
| return fmt.Sprintf("chat.user.{account}.request.user.%s.thread.unread.summary", siteID) | ||
| } | ||
| ``` | ||
|
|
||
| **Request / response** (`pkg/model`): | ||
|
|
||
| ```go | ||
| // ThreadUnreadSummaryRequest carries no fields: the account rides the subject. | ||
| type ThreadUnreadSummaryRequest struct{} | ||
|
|
||
| type ThreadUnreadSummaryResponse struct { | ||
| Unread bool `json:"unread"` | ||
| UnreadDirectMessage bool `json:"unreadDirectMessage"` | ||
| UnreadMention bool `json:"unreadMention"` | ||
| LastMessageAt *int64 `json:"lastMessageAt,omitempty"` // UnixMilli; nil when no responding site reported a thread | ||
| UnavailableSites []string `json:"unavailableSites,omitempty"` | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Rename the RPC contract consistently.
This spec still describes the client RPC and several model/handler names as ThreadUnreadSummary* / thread.unread.summary, but the implementation plan already switched the contract to ThreadUnread* / thread.unread. Leaving both in the docs will send implementation in two different directions. Based on the implementation plan and spec.
Also applies to: 175-229, 282-285
🤖 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-07-01-user-thread-unread-rpc-design.md` around
lines 63 - 89, The RPC contract naming in this spec is inconsistent with the
implementation plan: update the subject helpers and model/request-response names
from ThreadUnreadSummary* and thread.unread.summary to ThreadUnread* and
thread.unread throughout the referenced spec sections. Use the existing
identifiers like UserThreadUnreadSummary, UserThreadUnreadSummaryPattern,
ThreadUnreadSummaryRequest, and ThreadUnreadSummaryResponse as the places to
rename so the docs point to one contract only.
Comment on lines
+136
to
+147
| 1. threadSubs.ListByAccount(account) (local Mongo thread_subscriptions replica — ALL sites; newest 500 by _id desc) | ||
| → rows: {threadRoomId, siteId, lastSeenAt, hasMention} | ||
| 2. group rows by siteId; concurrently (no fan-out cap), per site, chunked by maxBatchSize: | ||
| rooms.GetThreadRoomInfoBatch(site, threadRoomIds) → map[threadRoomId]ThreadRoomInfo | ||
| compute per row (shared unread() helper): | ||
| unread = info.Found && (lastSeenAt == nil || info.LastMsgAt > lastSeenAt(UnixMilli)) | ||
| dmUnread = unread && info.RoomType == dm | ||
| mention = hasMention | ||
| track max(info.LastMsgAt) where Found | ||
| 3. aggregate: OR unread / dmUnread / mention across all rows; | ||
| LastMessageAt = max lastMsgAt; failed sites → UnavailableSites | ||
| → ThreadUnreadSummaryResponse |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the 500-row cap from ListByAccount.
The badge is supposed to aggregate every thread subscription the user has, but this spec limits the replica read to the newest 500 rows. That will silently undercount unread state for anyone with more than 500 thread subs, and the same cap is repeated later in the components section. Based on the implementation plan and spec.
Also applies to: 211-223
🤖 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-07-01-user-thread-unread-rpc-design.md` around
lines 136 - 147, The ThreadUnreadSummary flow currently limits
thread_subscriptions reads to the newest 500 rows, which can undercount unread
state for accounts with more subscriptions. Update the ListByAccount-based
aggregation in the spec to read all subscriptions for the account, and remove
the repeated 500-row cap wherever it appears in the later components section,
keeping the rest of the unread/group-by-site logic unchanged.
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
Adds a client-facing RPC that returns a per-user thread-unread badge aggregated across every federation site the user participates in, and removes the superseded (callerless)
ThreadUnreadSummaryleaf.New client RPC:
chat.user.{account}.request.user.{siteID}.thread.unread→ThreadUnreadResponse{ unread, unreadDirectMessage, unreadMention, lastMessageAt?, unavailableSites? }.How it works
It mirrors the existing subscription-unread pattern (one
unread()helper, two data sources):user-servicereads all of the user's thread-subscriptions from the home-site replica (ThreadSubscriptionRepo.ListByAccount) — these are federated to the home site, each tagged with the room's homesiteId.siteIdand, per site (uncapped fan-out, bounded in practice by the user's distinct thread-sub sites, chunked bymaxBatchSize), calls a new room-service RPCThreadRoomInfoBatch(chat.server.request.room.{siteID}.thread.info.batch) which returns each thread room'slastMsgAt+ parentroomType.unread()helper: OR the booleans across all rows,maxlastMsgAtover found rows,unreadDirectMessage = unread && roomType==dm,unreadMention = hasMention. Per-site RPC failures degrade intounavailableSitesinstead of failing the request.Grouping by
siteId(one thread-sub row per(threadRoomId, account)) routes each thread to exactly one owning site, so nothing is double-counted.Removed
The
ThreadUnreadSummaryroom-service leaf (chat.server.request.room.{siteID}.thread.unread.summary) had no runtime caller and is superseded by this design, so its handler, store method, subject builders, model types, and tests are deleted. Thethread_roomscollection handle and the{userAccount, siteId}index are retained (reused byGetThreadRoomInfos/ListByAccount).Notable design decisions (accepted trade-offs)
ThreadRoomInfoBatchreturnsroomType(via aroomslookup at the room's home site) sounreadDirectMessageuses authoritative data; the store method uses two projected finds joined in Go, no$lookup.InboxThreadReadevent replicates thelastSeenAtto the home replica (self-heals within the inbox lag). This matches how room-unread already behaves.Changes
pkg/subject:UserThreadUnread/Pattern,ThreadRoomInfoBatch/Subscribe.pkg/model:ThreadUnreadRequest/Response,ThreadRoomInfoBatchRequest/Response,ThreadRoomInfo,ThreadSubRef.room-service:GetThreadRoomInfosstore method +threadRoomInfoBatchhandler.user-service:roomclient.GetThreadRoomInfoBatch,mongorepo.ThreadSubscriptionRepo.ListByAccount,RoomClient/ThreadSubscriptionRepositoryinterfaces, and theGetThreadUnreadaggregator handler.docs/client-api.md: documents the newthread.unreadRPC.Testing
-race) for the subject builders, model round-trips, the room-service handler (happy path, empty, over-limit, store-error), theGetThreadUnreadaggregation (multi-site OR/max, null-lastSeen⇒unread, DM fold, mention, not-found skip, site-failure degrade, empty-subs, list error), andchunkStringsboundaries.GetThreadRoomInfos,GetThreadRoomInfoBatch(roomclient), andListByAccount.make test(race) andmake lintpass;gosecpasses. Integration tests andgovulncheck/semgrepwere network-blocked in the build environment and run in CI.🤖 Generated with Claude Code
https://claude.ai/code/session_01BocBvJn2N9YYMmciA5EWJX
Generated by Claude Code
Summary by CodeRabbit
@mentionstates.