feat(room-service): add app.tabs and app.cmd-menu RPCs#238
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds two read-only room-service RPCs (GetRoomAppTabs, GetRoomAppCommandMenu), updates member enrichment fields (sectName→orgName, bot name), introduces models and Mongo read/index methods, tightens subject formats with siteID, implements URL rewrite and bounded JSON replies, updates NewHandler signature, and adds comprehensive tests and docs. ChangesRoom Service App Tabs and Command Menu RPCs
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: 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/superpowers/specs/2026-05-26-room-service-app-tabs-and-cmd-menu-design.md`:
- Around line 388-404: Add a language tag to the fenced code block that contains
the Mongo aggregation pipeline (the block beginning with the array of stages
like {"$match": {"roomId": roomID, "u.isBot": true}} and the subsequent
{"$lookup": ...} pipeline) by changing the opening triple backticks to include
"json" (i.e., ```json) so markdownlint MD040 is satisfied and the JSON-like
aggregation pipeline renders correctly.
In `@pkg/subject/subject.go`:
- Around line 387-401: RoomAppTabs and RoomAppCmdMenu accept an account token
but do not use the existing account-token guard, enabling
wildcard/special-character leakage; update both functions to call the shared
validator isValidAccountToken(account) at the start and bail out when validation
fails (e.g., return an empty string or otherwise follow the same error/return
pattern used by other account-based builders) so all account-accepting subject
builders consistently enforce token validation.
In `@room-service/handler.go`:
- Around line 1589-1591: The slog.Warn call is logging the raw channel tab URL
template (app.ChannelTab.URL.Default) which may contain sensitive data; change
the log to avoid the full template by removing app.ChannelTab.URL.Default and
instead log only stable identifiers like app.ID and roomID plus a non-sensitive
indicator (e.g., a boolean hasTemplate flag or a one-way hash/fingerprint of
app.ChannelTab.URL.Default) so that slog.Warn("skipping app with empty or
unparseable channelTab url", "appId", app.ID, "roomId", roomID, ...) no longer
emits the raw URL template.
In `@room-service/store_mongo.go`:
- Line 36: Change the singular collection name used when constructing the
botCmdMenus collection to a lowercase plural form: replace the
db.Collection("bot_cmd_menu") call that initializes botCmdMenus with
db.Collection("bot_cmd_menus"), and update any other references (indexes, tests,
docs, and any code that assumes "bot_cmd_menu") to the new "bot_cmd_menus" name
to keep naming consistent with the repository convention.
🪄 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: 49d2bf3d-608c-4f5d-8a17-f6a4dd78804d
📒 Files selected for processing (20)
docs/client-api.mddocs/superpowers/plans/2026-05-26-room-service-app-tabs-and-cmd-menu.mddocs/superpowers/specs/2026-05-26-room-service-app-tabs-and-cmd-menu-design.mdpkg/model/app.gopkg/model/botcmdmenu.gopkg/model/model_test.gopkg/model/subscription.gopkg/model/user.gopkg/subject/subject.gopkg/subject/subject_test.goroom-service/deploy/docker-compose.ymlroom-service/handler.goroom-service/handler_test.goroom-service/helper.goroom-service/helper_test.goroom-service/integration_test.goroom-service/main.goroom-service/mock_store_test.goroom-service/store.goroom-service/store_mongo.go
vjauhari-work
force-pushed
the
claude/determined-fermi-A07HN
branch
from
7d01d55 to
2aa0ddf
Compare
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 867-868: Update the docs for the org-members endpoint to remove
the misleading "room's origin `siteID`" phrasing and explicitly state that
`{siteID}` selects which site's user directory to query for the organization
(i.e., the site whose view of org membership is being returned), matching the
same `member.list` per-site semantics; replace the sentence that mentions
"room's origin site" with a clear one-line explanation that the endpoint is
org-scoped and `{siteID}` denotes the site whose `users` collection should be
used, and add a short example or note clarifying routing/ownership expectations
for clients.
In `@pkg/subject/subject.go`:
- Around line 224-247: OrgMembers and ParseOrgMembersSubject currently allow
wildcard or special characters in tokens (e.g. account/orgId) which reintroduces
wildcard leaks; add a single helper like validateSubjectToken(token string) bool
that rejects tokens containing NATS special characters (at minimum '*' and '>'
and the separator '.') and use it in OrgMembers (validate the account before
building the subject) and in ParseOrgMembersSubject (ensure parts[2], parts[5],
and parts[6] are valid) so malformed subjects like
"chat.user.*.request.orgs.*.site-a.members" cause ParseOrgMembersSubject to
return ok=false and OrgMembers cannot produce subjects from invalid accounts.
In `@room-service/store_mongo.go`:
- Around line 649-702: The bot name enrichment implemented in
attachUserDisplayNames is only used by getRoomSubscriptions, so ListRoomMembers
via getRoomMembers never populates bot Member.Name; update the getRoomMembers
path to invoke attachUserDisplayNames (or inline equivalent logic) on the
members slice before returning so bot accounts get looked up via
findAppsForDisplay and Member.Name is copied onto bot members; specifically
modify getRoomMembers (the function used by ListRoomMembers) to call
s.attachUserDisplayNames(ctx, roomID, members) and handle/propagate errors
similarly to how getRoomSubscriptions does.
- Around line 1110-1118: The projection in ListDefaultChannelTabApps (in
function ListDefaultChannelTabApps) omits the app label so returned model.App
entries have Name == ""; update the projection passed to
options.Find().SetProjection (the bson.M map) to include the "name" field (e.g.,
add "name": 1) so GetRoomAppTabs and any consumers receive the app label
populated.
🪄 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: 429c4055-5068-40df-8340-d52d8609d33d
📒 Files selected for processing (21)
docs/client-api.mddocs/superpowers/plans/2026-05-26-room-service-app-tabs-and-cmd-menu.mddocs/superpowers/specs/2026-05-26-room-service-app-tabs-and-cmd-menu-design.mdpkg/model/app.gopkg/model/botcmdmenu.gopkg/model/member.gopkg/model/model_test.gopkg/model/subscription.gopkg/model/user.gopkg/subject/subject.gopkg/subject/subject_test.goroom-service/deploy/docker-compose.ymlroom-service/handler.goroom-service/handler_test.goroom-service/helper.goroom-service/helper_test.goroom-service/integration_test.goroom-service/main.goroom-service/mock_store_test.goroom-service/store.goroom-service/store_mongo.go
✅ Files skipped from review due to trivial changes (4)
- room-service/deploy/docker-compose.yml
- docs/superpowers/plans/2026-05-26-room-service-app-tabs-and-cmd-menu.md
- room-service/mock_store_test.go
- docs/superpowers/specs/2026-05-26-room-service-app-tabs-and-cmd-menu-design.md
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/model/subscription.go
- room-service/store.go
- room-service/helper_test.go
- pkg/model/app.go
- room-service/helper.go
- pkg/model/user.go
- pkg/model/botcmdmenu.go
- room-service/main.go
- room-service/handler.go
- room-service/handler_test.go
vjauhari-work
force-pushed
the
claude/determined-fermi-A07HN
branch
from
ca4ce3f to
5c6d14c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
room-service/handler.go (1)
1583-1590: ⚡ Quick winAdd the request ID to these warning logs.
These are request-scoped warnings, so leaving out the correlation ID makes bad templates much harder to trace back to the triggering RPC. As per coding guidelines: "Generate or extract a unique request/correlation ID at the entry point (HTTP middleware or NATS message handler), propagate via
context.Context, include in all log lines."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@room-service/handler.go` around lines 1583 - 1590, The warning logs in the ChannelTab checks omit the request/correlation ID; update the two slog.Warn calls inside the loop handling app.ChannelTab (the block referencing app.ChannelTab, roomID, app.ID and h.buildTabURL) to include the request ID retrieved from the request context (propagated into this handler) — e.g., extract the request ID from ctx (or the contextual logger) and add it as an extra key/value like "requestId" to both warning logs so every warning includes the correlation ID for tracing.room-service/store_mongo.go (1)
141-145: ⚡ Quick winDrop the second
(roomId, u.isBot)index creation.This block repeats the same
CreateOnealready executed at Lines 108-112. Keeping both adds an unnecessary startup round-trip and makes future index changes easy to update in only one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@room-service/store_mongo.go` around lines 141 - 145, Remove the duplicate index creation call that attempts to create an index on {"roomId", "u.isBot"} via s.subscriptions.Indexes().CreateOne; this block is redundant with the earlier CreateOne call that already creates the same bson.D{{"roomId", 1}, {"u.isBot", 1}} index, so delete this second CreateOne + error-check block and rely on the original index creation to avoid an extra startup round-trip and duplicate maintenance points.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@room-service/handler.go`:
- Around line 1419-1435: The authorizeRoomAppRead function currently grants
admin bypass when GetSubscription misses; change it to verify the room exists
first by calling h.store.GetRoom(ctx, roomID) when GetSubscription returns
model.ErrSubscriptionNotFound. If GetRoom returns a not-found error, return
errAppAccessDenied; if GetRoom returns any other error, return fmt.Errorf("check
room existence: %w", err). Only after confirming the room exists should you call
h.store.GetUser and allow model.IsPlatformAdmin to bypass. Keep existing checks
using model.IsRoomMember, model.IsPlatformAdmin, and preserve existing error
wrapping for other error cases.
---
Nitpick comments:
In `@room-service/handler.go`:
- Around line 1583-1590: The warning logs in the ChannelTab checks omit the
request/correlation ID; update the two slog.Warn calls inside the loop handling
app.ChannelTab (the block referencing app.ChannelTab, roomID, app.ID and
h.buildTabURL) to include the request ID retrieved from the request context
(propagated into this handler) — e.g., extract the request ID from ctx (or the
contextual logger) and add it as an extra key/value like "requestId" to both
warning logs so every warning includes the correlation ID for tracing.
In `@room-service/store_mongo.go`:
- Around line 141-145: Remove the duplicate index creation call that attempts to
create an index on {"roomId", "u.isBot"} via
s.subscriptions.Indexes().CreateOne; this block is redundant with the earlier
CreateOne call that already creates the same bson.D{{"roomId", 1}, {"u.isBot",
1}} index, so delete this second CreateOne + error-check block and rely on the
original index creation to avoid an extra startup round-trip and duplicate
maintenance points.
🪄 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: 4fb260cf-bfc6-4dfa-aee7-4383680f2bbf
📒 Files selected for processing (21)
docs/client-api.mddocs/superpowers/plans/2026-05-26-room-service-app-tabs-and-cmd-menu.mddocs/superpowers/specs/2026-05-26-room-service-app-tabs-and-cmd-menu-design.mdpkg/model/app.gopkg/model/botcmdmenu.gopkg/model/member.gopkg/model/model_test.gopkg/model/subscription.gopkg/model/user.gopkg/subject/subject.gopkg/subject/subject_test.goroom-service/deploy/docker-compose.ymlroom-service/handler.goroom-service/handler_test.goroom-service/helper.goroom-service/helper_test.goroom-service/integration_test.goroom-service/main.goroom-service/mock_store_test.goroom-service/store.goroom-service/store_mongo.go
✅ Files skipped from review due to trivial changes (2)
- room-service/mock_store_test.go
- docs/superpowers/specs/2026-05-26-room-service-app-tabs-and-cmd-menu-design.md
🚧 Files skipped from review as they are similar to previous changes (13)
- pkg/subject/subject_test.go
- pkg/model/app.go
- pkg/model/subscription.go
- room-service/helper_test.go
- room-service/main.go
- room-service/deploy/docker-compose.yml
- pkg/model/user.go
- room-service/store.go
- docs/client-api.md
- room-service/helper.go
- pkg/subject/subject.go
- room-service/handler_test.go
- room-service/integration_test.go
vjauhari-work
force-pushed
the
claude/determined-fermi-A07HN
branch
4 times, most recently
from
1cc01b6 to
dbc1ee9
Compare
mliu33
approved these changes
Jun 3, 2026
| return model.GetRoomAppTabsResponse{Apps: out}, nil | ||
| } | ||
|
|
||
| func (h *Handler) handleGetRoomAppCommandMenu(ctx context.Context, subj string, _ []byte) (model.GetRoomAppCommandMenuResponse, error) { |
Collaborator
There was a problem hiding this comment.
Third argument is redundant
Collaborator
Author
There was a problem hiding this comment.
Good catch — these are no-body RPCs, so the []byte body param was dead. Dropped it from both handleGetRoomAppTabs and handleGetRoomAppCommandMenu (plus the two NATS wrappers and the test call sites); the signatures now match handleListOrgMembers(ctx, subj). Done in ff87d47.
Generated by Claude Code
…errcode) Two read-only NATS request/reply RPCs gated by room-membership OR platform-admin: GetRoomAppTabs and GetRoomAppCommandMenu. Squash-rebased onto latest main and migrated to the centralized error handling introduced by #250: sentinels are now *errcode.Error (errAppAccessDenied -> Forbidden+RoomNotMember, errResponseTooLarge -> Internal); the NATS wrappers use wrappedCtx's (ctx, err) form + errnats.Reply; marshalBounded returns ([]byte, error); sanitizeError removed. Invalid-subject parse failures return errcode.BadRequest. Includes pkg/model types, pkg/subject builders, three RoomStore methods + compound indexes, SITE_URL config, and client-api docs. The 38 prior PR commits are collapsed here because the cross-cutting errcode migration made a per-commit replay impractical. https://claude.ai/code/session_016e4UVPcE4kQQoHrYa2H2yB
vjauhari-work
force-pushed
the
claude/determined-fermi-A07HN
branch
from
dbc1ee9 to
4b1cd7f
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
Two new read-only NATS request/reply RPCs in
room-service, gated by room membership OR platform-admin role:GetRoomAppTabs—chat.user.{account}.request.room.{roomID}.{siteID}.app.tabs— returns apps whosechannelTab.enabledANDchannelTab.defaultare both true, each with a per-roomtabUrlproduced by substituting${roomId}/${siteId}into the app's URL template and merging it with the configuredSITE_URL(scheme + host + path-prefix preserved, template userinfo stripped).GetRoomAppCommandMenu—chat.user.{account}.request.room.{roomID}.{siteID}.app.cmd-menu— for every bot subscribed to the room (u.isBot=true), returns the bot's activebot_cmd_menublocks joined with the owning app's name.What's in the PR
New types in
pkg/model—User.Roles+IsPlatformAdmin+PlatformRoleAdmin;IsRoomMember;App.AvatarURL;App.ChannelTab(+AppChannelTab,AppChannelTabURL);BotCmdMenu+CmdBlock+CmdModal; wire wrappersRoomApp,GetRoomAppTabsResponse,RoomAppAssistant,GetRoomAppCommandMenuResponse;MaxCmdBlockDepth=10+ValidateCmdBlockDepthguard.New subject builders in
pkg/subject—RoomAppTabs+ wildcard,RoomAppCmdMenu+ wildcard.ParseUserRoomSubjecttightened to reject empty/wildcardaccountandroomIDtokens.room-service— three newRoomStoremethods (ListDefaultChannelTabApps,ListRoomBotAppsaggregation,ListActiveCmdMenus); three new compound indexes (apps,subscriptions,bot_cmd_menu); sharedauthorizeRoomAppReadhelper;marshalBounded/replyBoundedJSONsourced fromnc.MaxPayload()so oversize replies fail with a cleanerrResponseTooLargerather than a silent NATS drop; URL rewrite inbuildTabURLwith RFC3986-unreserved input validation;SITE_URLenv var (required, validated at startup).Docs —
docs/client-api.mdgets two new entries per CLAUDE.md §5;room-service/deploy/docker-compose.ymlgetsSITE_URL=http://localhost:3000for local dev. Design spec atdocs/superpowers/specs/2026-05-26-room-service-app-tabs-and-cmd-menu-design.md; implementation plan atdocs/superpowers/plans/2026-05-26-room-service-app-tabs-and-cmd-menu.md.Test Plan
make test— greenmake test SERVICE=pkg/model,make test SERVICE=pkg/subject,make test SERVICE=room-service— greenmake lint— 0 issuesmake test-integration SERVICE=room-service— green against real Mongo 8.2.9 (verified locally with Docker+VFS, 21s for the 7 new test functions)SITE_URLingress path-prefix is preservedCoverage of new symbols (unit)
handleGetRoomAppTabshandleGetRoomAppCommandMenuauthorizeRoomAppReadbuildTabURLmarshalBoundedIsPlatformAdmin/IsRoomMember/ValidateCmdBlockDepthListDefaultChannelTabApps/ListRoomBotApps/ListActiveCmdMenusMigrations / ops
SITE_URLenv var is required on room-service. Set in every deployment (http(s)://host[/path-prefix]).User.Rolesis absent on legacy user documents — decodes to nil,IsPlatformAdminreturns false. Real platform admins lose access until ops backfills viaupdateMany.Subscription.User.IsBot(PR Fix: Added 'isBot' field in SubscriptionUser #219) — pre-existing bot subs without this field miss the cmd-menu join. Re-subscribes or a one-shot backfill close the gap.bot_cmd_menucollection is new; recommend a partial unique index{name:1} where {activeStatus:true}on the writer side.Out of scope (deferred)
Cross-site federation; write paths; caching; Prometheus per-handler metrics; auth+data parallelization via
errgroup(documented in spec's "Future optimizations").Generated by Claude Code
Summary by CodeRabbit
New Features
API Changes
Configuration
Documentation
Tests