Add RoomType field to Subscription model#137
Merged
Merged
Conversation
Resets the spec onto current main: PR #118 already removed RoomTypeGroup and renamed broadcast-worker's switch case, role-update guard, and errRoomTypeGuard sentinel — those scope items are dropped. PR #118 also deleted processInvite, so the invite-side population step is gone too. Surviving work: the new constants, the Subscription.RoomType field, and populating it on three sub-creation sites and two remove-event payloads.
Adds the new RoomTypeBotDM and RoomTypeDiscussion constants and a RoomType field on Subscription so that downstream consumers can route on room kind without an extra room lookup. The TestSubscriptionJSON round-trip now exercises the new field; TestRoomTypeValues asserts each of the four constants.
The owner subscription auto-created by handleCreateRoom now carries the room type from the create request so downstream consumers can route on the kind without re-fetching the room.
processAddMembers hardcodes RoomTypeChannel — DM and botDM rooms do not support add-member. processRemoveIndividual and processRemoveOrg each do one GetRoom per operation and stamp room.Type onto the partial Subscription carried by SubscriptionUpdateEvent. The room fetch is non-fatal: a lookup error logs at warn and the event still fires with an empty RoomType, matching the existing best-effort treatment of notification payloads.
Cross-site member_added events only originate from rooms that support add-member (channels and discussions). Hardcoding RoomTypeChannel mirrors room-worker's processAddMembers logic so every persisted subscription carries a non-empty RoomType.
Removes stale claims that this PR removes RoomTypeGroup or renames its call sites — PR #118 already did both. Updates the commit-strategy section to reflect the five commits actually made instead of the single-commit plan that predated the rebase.
Adds the plan header, file structure table, prerequisites, and the four-task outline. Subsequent commits will fill in each task body one at a time.
Adds the bite-sized TDD steps for introducing RoomTypeBotDM and RoomTypeDiscussion constants and the RoomType field on Subscription.
Adds the TDD steps for stamping req.Type onto the auto-created owner subscription returned by handleCreateRoom.
Adds the TDD steps for hardcoding RoomTypeChannel on processAddMembers and for fetching the room (non-fatal) inside processRemoveIndividual and processRemoveOrg so the partial Subscription on each removed event carries RoomType. Covers all five existing tests that need GetRoom mocks plus the two outbox-failure tests that hit the same publish path.
Adds the TDD steps for hardcoding RoomTypeChannel on the cross-site member_added handler and a final verification + push checklist covering full make test, make lint, integration tests, and the push command (with --force-with-lease guidance for branches that already have a prior tip on the remote). Also includes a Risks and Rollback section.
Pulls the duplicated GetRoom-then-warn pattern out of processRemoveIndividual and processRemoveOrg into a Handler.lookupRoomType helper that returns "" on lookup failure. Adds brief WHY notes at the two RoomTypeChannel hardcodes (room-worker processAddMembers and inbox-worker handleMemberAdded) so future readers see the upstream invariant that prevents non-channel rooms from reaching those paths. Drops a narrative test comment that the following assertion already conveys.
📝 WalkthroughWalkthroughAdds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 2
🧹 Nitpick comments (4)
room-service/handler_test.go (1)
50-52: Strengthen this test to verify passthrough, not just value presence.Because the request in this test uses
RoomTypeChannel, a hardcoded implementation would still pass. Consider using a non-channel room type here to provehandleCreateRoomcopiesreq.Type.♻️ Suggested test tweak
-req := model.CreateRoomRequest{Name: "general", Type: model.RoomTypeChannel, CreatedBy: "u1", CreatedByAccount: "alice", SiteID: "site-a"} +req := model.CreateRoomRequest{Name: "general", Type: model.RoomTypeDiscussion, CreatedBy: "u1", CreatedByAccount: "alice", SiteID: "site-a"} -if capturedSub != nil && capturedSub.RoomType != model.RoomTypeChannel { - t.Errorf("expected owner subscription RoomType=%q, got %q", model.RoomTypeChannel, capturedSub.RoomType) +if capturedSub != nil && capturedSub.RoomType != model.RoomTypeDiscussion { + t.Errorf("expected owner subscription RoomType=%q, got %q", model.RoomTypeDiscussion, capturedSub.RoomType) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@room-service/handler_test.go` around lines 50 - 52, The test currently only asserts capturedSub.RoomType is not Channel; instead set the request's type to a non-channel value (e.g., model.RoomTypeDirect or another enum) and assert that handleCreateRoom copies req.Type through to the created subscription by checking capturedSub.RoomType == req.Type; update the test setup where the request is constructed and the assertion that currently references model.RoomTypeChannel to compare against the request's Type value (symbols: handleCreateRoom, req.Type, capturedSub, model.RoomTypeChannel).room-worker/handler_test.go (1)
313-316: Add one failure-path test forGetRoombest-effort lookup.These paths now expect
GetRoomsuccess, but the implementation contract is “warn and continue with emptyRoomType” when lookup fails. A dedicated test would lock that behavior and prevent regressions.As per coding guidelines: Every handler method must have tests for: valid input, invalid/malformed input, store errors, and edge cases.🧪 Suggested test to add
+func TestHandler_ProcessRemoveMember_GetRoomError_StillPublishesWithEmptyRoomType(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockSubscriptionStore(ctrl) + + const roomID, account, siteID = "room-1", "alice", "site-a" + store.EXPECT().GetUserWithMembership(gomock.Any(), roomID, account).Return(&UserWithMembership{ + User: model.User{ID: "u1", Account: account, SiteID: siteID}, + }, nil) + store.EXPECT().DeleteSubscription(gomock.Any(), roomID, account).Return(int64(1), nil) + store.EXPECT().DeleteRoomMember(gomock.Any(), roomID, model.RoomMemberIndividual, "u1").Return(nil) + store.EXPECT().ReconcileUserCount(gomock.Any(), roomID).Return(nil) + store.EXPECT().GetRoom(gomock.Any(), roomID).Return(nil, fmt.Errorf("db down")) + + var published []publishedMsg + h := NewHandler(store, siteID, func(_ context.Context, subj string, data []byte, _ string) error { + published = append(published, publishedMsg{subj: subj, data: data}) + return nil + }) + + req := model.RemoveMemberRequest{RoomID: roomID, Requester: account, Account: account} + data, _ := json.Marshal(req) + require.NoError(t, h.processRemoveMember(context.Background(), data)) + + for _, p := range published { + if p.subj != subject.SubscriptionUpdate(account) { + continue + } + var evt model.SubscriptionUpdateEvent + require.NoError(t, json.Unmarshal(p.data, &evt)) + assert.Equal(t, model.RoomType(""), evt.Subscription.RoomType) + } +}Also applies to: 498-501, 880-883, 941-944, 1168-1171, 1209-1212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@room-worker/handler_test.go` around lines 313 - 316, Tests currently assume store.GetRoom always succeeds; add a failure-path test that verifies the handler performs a best-effort lookup (logs/warns and proceeds with an empty RoomType) when GetRoom returns an error. In handler_test.go add a mock expectation on store.EXPECT().GetRoom(...).Return(nil, someError) and assert the handler continues without failing and that the resulting model.Room.Type is empty (or default), and that appropriate warning/log behavior occurs; apply the same pattern for other handler tests referencing GetRoom (lines around the other noted blocks) to lock this contract.room-worker/handler.go (2)
432-439: Preferroom.Typeover hardcodedRoomTypeChannelin add-members.
roomis already loaded in this function, so persistingroom.Typeavoids silent drift if add-member eligibility expands beyond channel later.♻️ Suggested refactor
sub := &model.Subscription{ ID: idgen.GenerateID(), User: model.SubscriptionUser{ID: user.ID, Account: user.Account}, RoomID: req.RoomID, - RoomType: model.RoomTypeChannel, + RoomType: room.Type, SiteID: room.SiteID, Roles: []model.Role{model.RoleMember}, JoinedAt: acceptedAt, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@room-worker/handler.go` around lines 432 - 439, The Subscription being created in the add-members handler hardcodes RoomType to model.RoomTypeChannel; change it to use the already-loaded room.Type so the persisted subscription reflects the actual room kind. Locate the code that constructs sub := &model.Subscription{... RoomType: model.RoomTypeChannel, ...} and replace the hardcoded value with RoomType: room.Type (ensuring room is in scope and of the expected type) so future non-channel room types won’t drift silently.
681-683: Log the nil-room path inlookupRoomTypefor diagnostics.Returning empty
RoomTypeis fine, but a warning onroom == nilwould make this best-effort fallback observable in production.📝 Optional observability tweak
if room == nil { + slog.Warn("room not found", "roomID", roomID, "op", op) return "" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@room-worker/handler.go` around lines 681 - 683, In lookupRoomType, when the early-return branch sees room == nil, emit a warning-level log before returning the empty RoomType so this fallback is observable; use the file's existing logger (the same logger used elsewhere in handler.go) and include identifying context such as the room ID or request/context info (e.g., roomID or id variable) and a short message like "lookupRoomType: room is nil, returning empty RoomType" to aid diagnostics, then return "" as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md`:
- Around line 45-46: The comment notes that inbox-worker's handleMemberAdded
currently always stamps RoomTypeChannel for cross-site member_added events;
update handleMemberAdded to determine the actual room type from the event
payload (or the room metadata) and only use RoomTypeChannel when the payload
indicates a channel, otherwise stamp the correct type (e.g., RoomTypeDiscussion
or RoomTypeDM) so discussion rooms are not misclassified; change any hardcoded
uses of RoomTypeChannel in handleMemberAdded to a conditional/mapper that
derives the room type from event.room_type or similar field.
- Around line 104-109: Several fenced code blocks in the markdown are missing
language labels (MD040); update each unlabeled triple-backtick block by adding
the appropriate language tag (e.g., bash for make commands like the block
containing "make test SERVICE=pkg/model" or "make test SERVICE=room-worker",
text for raw test output such as the block starting with "--- FAIL:
TestHandleEvent_MemberAdded", and go for Go snippets) so markdownlint passes;
apply the same change to the other occurrences called out in the review (the
blocks around the listed ranges) ensuring each fenced block has the correct
language label.
---
Nitpick comments:
In `@room-service/handler_test.go`:
- Around line 50-52: The test currently only asserts capturedSub.RoomType is not
Channel; instead set the request's type to a non-channel value (e.g.,
model.RoomTypeDirect or another enum) and assert that handleCreateRoom copies
req.Type through to the created subscription by checking capturedSub.RoomType ==
req.Type; update the test setup where the request is constructed and the
assertion that currently references model.RoomTypeChannel to compare against the
request's Type value (symbols: handleCreateRoom, req.Type, capturedSub,
model.RoomTypeChannel).
In `@room-worker/handler_test.go`:
- Around line 313-316: Tests currently assume store.GetRoom always succeeds; add
a failure-path test that verifies the handler performs a best-effort lookup
(logs/warns and proceeds with an empty RoomType) when GetRoom returns an error.
In handler_test.go add a mock expectation on
store.EXPECT().GetRoom(...).Return(nil, someError) and assert the handler
continues without failing and that the resulting model.Room.Type is empty (or
default), and that appropriate warning/log behavior occurs; apply the same
pattern for other handler tests referencing GetRoom (lines around the other
noted blocks) to lock this contract.
In `@room-worker/handler.go`:
- Around line 432-439: The Subscription being created in the add-members handler
hardcodes RoomType to model.RoomTypeChannel; change it to use the already-loaded
room.Type so the persisted subscription reflects the actual room kind. Locate
the code that constructs sub := &model.Subscription{... RoomType:
model.RoomTypeChannel, ...} and replace the hardcoded value with RoomType:
room.Type (ensuring room is in scope and of the expected type) so future
non-channel room types won’t drift silently.
- Around line 681-683: In lookupRoomType, when the early-return branch sees room
== nil, emit a warning-level log before returning the empty RoomType so this
fallback is observable; use the file's existing logger (the same logger used
elsewhere in handler.go) and include identifying context such as the room ID or
request/context info (e.g., roomID or id variable) and a short message like
"lookupRoomType: room is nil, returning empty RoomType" to aid diagnostics, then
return "" as before.
🪄 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: 66b129b3-64e4-434c-a6b4-ce89fc34bdcd
📒 Files selected for processing (11)
docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.mddocs/superpowers/specs/2026-04-23-roomtype-on-subscription-design.mdinbox-worker/handler.goinbox-worker/handler_test.gopkg/model/model_test.gopkg/model/room.gopkg/model/subscription.goroom-service/handler.goroom-service/handler_test.goroom-worker/handler.goroom-worker/handler_test.go
Comment on lines
+45
to
+46
| 4. **`inbox-worker`** — cross-site `handleMemberAdded` hardcodes `RoomTypeChannel` (only channel/discussion-style rooms ever produce cross-site `member_added` events, never DM/botDM). | ||
|
|
There was a problem hiding this comment.
Clarify discussion-room behavior vs hardcoded channel stamping.
This plan says member_added can come from channels and discussions, but the implementation instruction still hardcodes RoomTypeChannel. Please tighten the rule so future changes don’t persist an incorrect room type for discussion rooms.
Also applies to: 484-485
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md` around
lines 45 - 46, The comment notes that inbox-worker's handleMemberAdded currently
always stamps RoomTypeChannel for cross-site member_added events; update
handleMemberAdded to determine the actual room type from the event payload (or
the room metadata) and only use RoomTypeChannel when the payload indicates a
channel, otherwise stamp the correct type (e.g., RoomTypeDiscussion or
RoomTypeDM) so discussion rooms are not misclassified; change any hardcoded uses
of RoomTypeChannel in handleMemberAdded to a conditional/mapper that derives the
room type from event.room_type or similar field.
Comment on lines
+104
to
+109
| ``` | ||
| make test SERVICE=pkg/model | ||
| ``` | ||
|
|
||
| Expected output (verbatim): | ||
| ``` |
There was a problem hiding this comment.
Add language labels to fenced code blocks (MD040).
Several fenced blocks are unlabeled; add bash, go, or text to keep markdownlint clean.
🧩 Example fix pattern
- ```
+ ```bash
make test SERVICE=room-worker-
--- FAIL: TestHandleEvent_MemberAdded (0.00s) handler_test.go:222: subscription RoomType = "", want "channel"
</details>
Also applies to: 150-155, 199-204, 229-237, 365-365, 453-461, 498-503, 527-535, 556-564, 572-572, 582-587
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 104-104: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
---
[warning] 109-109: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md around
lines 104 - 109, Several fenced code blocks in the markdown are missing language
labels (MD040); update each unlabeled triple-backtick block by adding the
appropriate language tag (e.g., bash for make commands like the block containing
"make test SERVICE=pkg/model" or "make test SERVICE=room-worker", text for raw
test output such as the block starting with "--- FAIL:
TestHandleEvent_MemberAdded", and go for Go snippets) so markdownlint passes;
apply the same change to the other occurrences called out in the review (the
blocks around the listed ranges) ensuring each fenced block has the correct
language label.
</details>
<!-- fingerprinting:phantom:poseidon:hawk:5842c2f8-2fb3-4604-aff1-a717f91d0908 -->
<!-- d98c2f50 -->
<!-- This is an auto-generated comment by CodeRabbit -->
mliu33
requested changes
Apr 28, 2026
| RoomID: req.RoomID, | ||
| User: model.SubscriptionUser{ID: user.ID, Account: req.Account}, | ||
| RoomID: req.RoomID, | ||
| RoomType: roomType, |
Collaborator
There was a problem hiding this comment.
We can fix this to channel type too
Collaborator
Author
There was a problem hiding this comment.
hardcoded to roomTypeChannel.
| RoomID: req.RoomID, | ||
| User: model.SubscriptionUser{Account: m.Account}, | ||
| RoomID: req.RoomID, | ||
| RoomType: roomType, |
Collaborator
There was a problem hiding this comment.
We can fix this to channel too
Collaborator
Author
There was a problem hiding this comment.
hardcoded to RoomTypeChannel.
| // lookupRoomType fetches the room and returns its Type. On failure it logs at | ||
| // warn and returns "" — the field is best-effort metadata on notification | ||
| // payloads, never load-bearing. | ||
| func (h *Handler) lookupRoomType(ctx context.Context, roomID, op string) model.RoomType { |
Collaborator
There was a problem hiding this comment.
This is not needed as we can only add/remove/assign role in a channel room
Per mliu33's review on PR #137: member-management ops (member.add, member.remove, member.role-update) only apply to channel rooms — room-service rejects them for any other kind before they reach the room-worker stream. So the runtime GetRoom round-trip in processRemoveIndividual and processRemoveOrg is unnecessary; both sites now hardcode RoomTypeChannel on the partial Subscription literal, matching processAddMembers. The lookupRoomType helper is gone. Tests drop the GetRoom mocks (six call sites) and keep the existing RoomType assertion on the published SubscriptionUpdateEvent payload, which now passes by virtue of the hardcode. Resolves: #137 review threads on lines 183, 305, 675.
Spec §4 and plan Task 3 now describe hardcoding RoomTypeChannel at the two remove sites instead of the previous fetch-room approach, matching the room-worker code change in the prior commit. Also addresses CodeRabbit's MD040 nit by adding language tags (bash for shell commands, text for sample test output) to every previously unlabeled fenced code block in the plan.
Collaborator
Author
|
Addressed the review feedback in @mliu33 — three threads on
Member-management ops ( @coderabbitai — two threads on the plan doc:
The threads should appear outdated since the line anchors moved; please click "Resolve conversation" on each if it looks good. Generated by Claude Code |
|
Tip For best results, initiate chat on the files or code changes.
Both my threads are addressed:
I've gone ahead and resolved my conversations. [resolve] ✏️ Learnings added
🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md (1)
46-47:⚠️ Potential issue | 🟡 MinorTighten the member-added rule to channel-only.
This still mentions channels and discussions, but the invariant in this repo is channel-only. Update the wording so future follow-ups don’t preserve an incorrect
RoomTypefor discussion rooms.Suggested tweak
- Cross-site `member_added` events only fire for rooms that support add-member (channels and discussions). Hardcoding RoomTypeChannel mirrors room-worker.processAddMembers so every persisted subscription carries a non-empty RoomType. + Cross-site `member_added` events only fire for channel rooms. Hardcoding RoomTypeChannel mirrors room-worker.processAddMembers so every persisted subscription carries a non-empty RoomType.Based on learnings: member-management operations (
member.add,member.remove,member.role-update) are gated by room-service to channel rooms only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md` around lines 46 - 47, Update the documentation text in the "inbox-worker" bullet to state the channel-only invariant explicitly: replace the current wording that mentions channels and discussions with a definitive statement that cross-site handleMemberAdded (and the originating member.add gateway) applies only to RoomTypeChannel; reference the function/event names "inbox-worker", "handleMemberAdded", "member.add"/"member_added" and the enum/value "RoomTypeChannel" so readers know the exact symbols enforcing this rule and remove any phrasing that suggests discussion rooms are applicable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/specs/2026-04-23-roomtype-on-subscription-design.md`:
- Around line 157-163: Update the spec text to reflect the current
implementation: state that removed-event payloads embed the subscription with
RoomType set to the hardcoded RoomTypeChannel value (not a runtime lookup with
an empty fallback). Replace the sentence describing a runtime room
lookup/fallback for SubscriptionUpdateEvent.Subscription.RoomType with an
explicit note that removed events carry RoomTypeChannel, and keep the existing
note about old subscriptions without roomType being returned as RoomType: "" for
backwards compatibility.
In `@room-worker/handler_test.go`:
- Around line 338-345: The test only asserts evt.Subscription.RoomType for the
individual-removal publish path; add the same check for the org-removal path by
locating where processRemoveOrg is exercised and extending the published-events
loop (or a new loop) to match subject.SubscriptionUpdate for the org case and
unmarshal into model.SubscriptionUpdateEvent, then assert assert.Equal(t,
model.RoomTypeChannel, evt.Subscription.RoomType) for that event as well so both
processRemoveIndividual and processRemoveOrg are covered.
---
Duplicate comments:
In `@docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.md`:
- Around line 46-47: Update the documentation text in the "inbox-worker" bullet
to state the channel-only invariant explicitly: replace the current wording that
mentions channels and discussions with a definitive statement that cross-site
handleMemberAdded (and the originating member.add gateway) applies only to
RoomTypeChannel; reference the function/event names "inbox-worker",
"handleMemberAdded", "member.add"/"member_added" and the enum/value
"RoomTypeChannel" so readers know the exact symbols enforcing this rule and
remove any phrasing that suggests discussion rooms are applicable.
🪄 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: 1aca51e5-b292-41d3-b834-9d7e2e9c9e8d
📒 Files selected for processing (4)
docs/superpowers/plans/2026-04-27-roomtype-on-subscription-plan.mddocs/superpowers/specs/2026-04-23-roomtype-on-subscription-design.mdroom-worker/handler.goroom-worker/handler_test.go
Comment on lines
+157
to
+163
| - **Old subscriptions without `roomType`.** Returned as `RoomType: ""`. No | ||
| code currently reads the field, so this is harmless until a consumer starts | ||
| relying on it. A future backfill job (out of scope) can populate old | ||
| records. | ||
| - **Client code consuming `SubscriptionUpdateEvent.Subscription.RoomType` on | ||
| removed events.** Covered by populating the field from the fetched room; if | ||
| the fetch fails the field is empty, same as before the change. |
There was a problem hiding this comment.
Update the remove-path contract to match the implementation.
The spec still describes a runtime room lookup / empty fallback for removed-event payloads. The code now hardcodes RoomTypeChannel on the embedded subscription, so the design text should say that explicitly.
Suggested tweak
- Client code consuming SubscriptionUpdateEvent.Subscription.RoomType on removed events. Covered by populating the field from the fetched room; if the fetch fails the field is empty, same as before the change.
+ Client code consuming SubscriptionUpdateEvent.Subscription.RoomType on removed events. Covered by hardcoding `RoomTypeChannel` on the partial Subscription payloads in room-worker, matching the channel-only member-management invariant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/specs/2026-04-23-roomtype-on-subscription-design.md` around
lines 157 - 163, Update the spec text to reflect the current implementation:
state that removed-event payloads embed the subscription with RoomType set to
the hardcoded RoomTypeChannel value (not a runtime lookup with an empty
fallback). Replace the sentence describing a runtime room lookup/fallback for
SubscriptionUpdateEvent.Subscription.RoomType with an explicit note that removed
events carry RoomTypeChannel, and keep the existing note about old subscriptions
without roomType being returned as RoomType: "" for backwards compatibility.
Comment on lines
+338
to
+345
| for _, p := range published { | ||
| if p.subj != subject.SubscriptionUpdate(account) { | ||
| continue | ||
| } | ||
| var evt model.SubscriptionUpdateEvent | ||
| require.NoError(t, json.Unmarshal(p.data, &evt)) | ||
| assert.Equal(t, model.RoomTypeChannel, evt.Subscription.RoomType, "subscription update should carry RoomType") | ||
| } |
There was a problem hiding this comment.
Extend the RoomType assertion to the org-removal path too.
This only exercises processRemoveIndividual; processRemoveOrg still has no assertion for the embedded Subscription.RoomType, so a regression there would slip through.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@room-worker/handler_test.go` around lines 338 - 345, The test only asserts
evt.Subscription.RoomType for the individual-removal publish path; add the same
check for the org-removal path by locating where processRemoveOrg is exercised
and extending the published-events loop (or a new loop) to match
subject.SubscriptionUpdate for the org case and unmarshal into
model.SubscriptionUpdateEvent, then assert assert.Equal(t,
model.RoomTypeChannel, evt.Subscription.RoomType) for that event as well so both
processRemoveIndividual and processRemoveOrg are covered.
8 tasks
This was referenced May 8, 2026
Merged
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
This PR adds a
RoomTypefield tomodel.Subscriptionand introduces two new room type constants (RoomTypeBotDMandRoomTypeDiscussion). The change denormalizes room type information onto subscription documents so downstream consumers can route on subscription type without requiring an additional room lookup.Key Changes
Model updates (
pkg/model/)RoomTypeBotDMandRoomTypeDiscussionconstants to theRoomTypeenumRoomTypefield toSubscriptionstruct (positioned betweenRoomIDandSiteID)room-service (
room-service/handler.go)RoomTypeon the auto-created owner subscription duringCreateRoomby reading fromreq.Typeroom-worker (
room-worker/handler.go)processAddMembers: HardcodeRoomTypeChannelon created subscriptions (DM/botDM rooms reject member.add upstream)processRemoveIndividualandprocessRemoveOrg: Fetch the room once and stampRoomTypeonto the partialSubscriptionliteral inSubscriptionUpdateEventpayloads (non-fatal; logs warning on fetch failure)lookupRoomTypehelper method for consistent room-fetch error handlinginbox-worker (
inbox-worker/handler.go)RoomTypeChannelon cross-sitemember_addedsubscriptions (only channel/discussion rooms produce cross-site events)Tests updated across all services to assert
RoomTypeis correctly populated at each creation siteImplementation Details
"") for backward compatibility; existing call sites compile without modificationGetRoomfails, the removal proceeds with an emptyRoomTypeand a warning is loggedroom-workeralready hasGetRoomavailablehttps://claude.ai/code/session_01HQF5tspEtJBg69TT89oCdx
Summary by CodeRabbit
New Features
Documentation
Tests