Gate thread-reply visibility by history window; batch Cassandra writes#435
Open
Allan-Code-hub wants to merge 1 commit into
Open
Gate thread-reply visibility by history window; batch Cassandra writes#435Allan-Code-hub wants to merge 1 commit into
Allan-Code-hub wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR adds history-window gating for thread-reply ChangesThread-reply history visibility gate
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BW as broadcast-worker Handler
participant BWS as broadcast-worker Store
participant MW as message-worker Handler
participant MWS as message-worker ThreadStore
BW->>BWS: GetHistorySharedSince(roomID, mentionedAccounts)
BWS-->>BW: map[account]*historySharedSince
BW->>BW: filter via mentionVisible(parentCreatedAt)
BW->>BWS: SetThreadSubscriptionMentions(parentMessageID, allowedAccounts)
MW->>MWS: GetHistorySharedSince(roomID, candidates)
MWS-->>MW: map[account]*historySharedSince
MW->>MW: filter via mentionVisible(parentCreatedAt)
MW->>MW: mark mentions, inbox updates, replyAccounts
Possibly related PRs
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.
🧹 Nitpick comments (5)
message-worker/handler_test.go (1)
1837-1882: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for
GetHistorySharedSincereturning an error.
markThreadMentionsnow wraps and returns an error whenGetHistorySharedSincefails (handler.go ~511-513), but no test exercises that branch here.As per path instructions,
**/*_test.gorequires "Tests must cover: happy path, error paths, edge cases... and invalid input", and**/*handler_test.gorequires "Every handler method must have tests for: valid input, invalid/malformed input, store errors, and edge cases."✅ Suggested test addition
func TestHandler_MarkThreadMentions_HistoryLookupError(t *testing.T) { ctrl := gomock.NewController(t) ts := NewMockThreadStore(ctrl) ts.EXPECT().GetHistorySharedSince(gomock.Any(), "r1", gomock.Any()). Return(nil, errors.New("mongo: connection refused")) h := NewHandler(NewMockStore(ctrl), NewMockUserStore(ctrl), ts, "site-a", func(_ context.Context, _ string, _ []byte, _ string) error { return nil }) msg := &model.Message{ ID: "msg-reply", RoomID: "r1", UserID: "u-sender", UserAccount: "sender", ThreadParentMessageID: "msg-parent", Mentions: []model.Participant{{UserID: "u-al", Account: "alice", SiteID: "site-a"}}, } err := h.markThreadMentions(context.Background(), msg, "tr-1", "site-a", false) require.Error(t, err) }🤖 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 `@message-worker/handler_test.go` around lines 1837 - 1882, `markThreadMentions` needs a test for the `GetHistorySharedSince` failure path, since it now returns a wrapped error from that lookup. Add a new case in `message-worker/handler_test.go` that sets up `MockThreadStore.GetHistorySharedSince` to return an error, invokes `Handler.markThreadMentions`, and asserts the call fails; use the existing `markThreadMentions` and `GetHistorySharedSince` symbols to locate the behavior and keep the rest of the setup minimal.Source: Path instructions
broadcast-worker/helper.go (1)
3-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider centralizing
mentionVisibleinstead of duplicating it per service.This function is byte-identical to
message-worker/helper.go'smentionVisible(confirmed via cross-file context). Since this determines who is allowed to see a thread-reply mention — a security/privacy-relevant decision — keeping two independent copies risks silent behavioral drift if one gets patched without the other. Consider moving it to a sharedpkg/(e.g.pkg/mentionvisibility) and having both services import it.🤖 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 `@broadcast-worker/helper.go` around lines 3 - 21, The mentionVisible helper is duplicated here and in message-worker, creating a risk of divergent access-control behavior. Move the shared visibility check into a common package (such as a pkg-level helper) and update both broadcast-worker and message-worker to call that shared function instead of maintaining separate copies. Keep the shared symbol named clearly so both services can import it and use the same thread-reply mention visibility logic.broadcast-worker/handler_test.go (1)
2177-2226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd store-error coverage for the new
GetHistorySharedSince/SetThreadSubscriptionMentionsgating.Both new tests only cover the happy path for
allowedThreadMentions. There's no test wherestore.GetHistorySharedSince(channel or DM path) orstore.SetThreadSubscriptionMentionsreturns an error, to confirmhandleThreadCreatedpropagates it correctly.As per path instructions for
**/*handler_test.go: "Every handler method must have tests for: valid input, invalid/malformed input, store errors, and edge cases."✅ Example addition
func TestHandleThreadCreated_Channel_HistorySharedSinceErrorPropagates(t *testing.T) { ctrl := gomock.NewController(t) store := NewMockStore(ctrl) us := NewMockUserStore(ctrl) pub := &mockPublisher{} keyStore := NewMockRoomKeyProvider(ctrl) store.EXPECT().GetRoomMeta(gomock.Any(), "room-1").Return(metaOf(testChannelRoom), nil) store.EXPECT().GetHistorySharedSince(gomock.Any(), "room-1", gomock.Any()). Return(nil, errors.New("mongo down")) evt := model.MessageEvent{ /* ... thread reply with `@-mention` ... */ } data, _ := json.Marshal(evt) h := NewHandler(store, us, pub, keyStore, false) require.Error(t, h.HandleMessage(context.Background(), data)) }🤖 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 `@broadcast-worker/handler_test.go` around lines 2177 - 2226, The new thread-mention gating path in handleThreadCreated needs error-path coverage, not just the happy path. Add tests in handler_test.go for when Store.GetHistorySharedSince fails in both the channel and DM/other relevant branch used by allowedThreadMentions, and when Store.SetThreadSubscriptionMentions returns an error. Use the existing TestHandleThreadCreated_* pattern with NewHandler, HandleMessage, and mock Store expectations to assert the error is propagated instead of silently continuing.Source: Path instructions
message-worker/helper.go (1)
10-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect logic; consider extracting to a shared package.
The nil/boundary handling is correct (verified against all test cases in
helper_test.go). Per the stack outline, an identicalmentionVisibleis being added tobroadcast-worker/helper.go. Since both services must apply the exact same visibility rule, duplicating it risks drift if one copy is later tweaked without the other. Consider hoisting it into a sharedpkg/package (e.g.,pkg/mentionvisibility) importable by bothmainpackages.🤖 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 `@message-worker/helper.go` around lines 10 - 18, The mention visibility logic in mentionVisible is correct, but it is being duplicated across message-worker and broadcast-worker, which risks future drift. Move this shared rule into a common pkg package (for example, a mentionvisibility helper) and update both helper.go implementations to call that shared function instead of maintaining separate copies.broadcast-worker/store_mongo.go (1)
185-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate implementation of
GetHistorySharedSinceacross services.This is byte-for-byte identical (filter, projection, minimal decode shape, and even the same explanatory comment) to
GetHistorySharedSincein message-worker'sthreadStoreMongo. Both query the samesubscriptions-style schema for the samehistorySharedSincesemantics. Worth extracting into a shared helper (e.g.,pkg/subscriptionstoreor similar) taking a*mongo.Collectionparameter, so the tricky nil-vs-absent decode logic has one source of truth instead of two copies that can silently drift.🤖 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 `@broadcast-worker/store_mongo.go` around lines 185 - 213, `GetHistorySharedSince` in `mongoStore` duplicates the same query/decode logic already used by `threadStoreMongo`, so extract the shared subscriptions lookup into a common helper (for example in a shared subscription store package) that accepts the target `*mongo.Collection` and returns the account-to-time map. Keep the filter, projection, and minimal decode shape centralized there, then have both `mongoStore.GetHistorySharedSince` implementations delegate to it to avoid drift and preserve the nil-vs-absent handling in one place.
🤖 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 `@broadcast-worker/handler_test.go`:
- Around line 2177-2226: The new thread-mention gating path in
handleThreadCreated needs error-path coverage, not just the happy path. Add
tests in handler_test.go for when Store.GetHistorySharedSince fails in both the
channel and DM/other relevant branch used by allowedThreadMentions, and when
Store.SetThreadSubscriptionMentions returns an error. Use the existing
TestHandleThreadCreated_* pattern with NewHandler, HandleMessage, and mock Store
expectations to assert the error is propagated instead of silently continuing.
In `@broadcast-worker/helper.go`:
- Around line 3-21: The mentionVisible helper is duplicated here and in
message-worker, creating a risk of divergent access-control behavior. Move the
shared visibility check into a common package (such as a pkg-level helper) and
update both broadcast-worker and message-worker to call that shared function
instead of maintaining separate copies. Keep the shared symbol named clearly so
both services can import it and use the same thread-reply mention visibility
logic.
In `@broadcast-worker/store_mongo.go`:
- Around line 185-213: `GetHistorySharedSince` in `mongoStore` duplicates the
same query/decode logic already used by `threadStoreMongo`, so extract the
shared subscriptions lookup into a common helper (for example in a shared
subscription store package) that accepts the target `*mongo.Collection` and
returns the account-to-time map. Keep the filter, projection, and minimal decode
shape centralized there, then have both `mongoStore.GetHistorySharedSince`
implementations delegate to it to avoid drift and preserve the nil-vs-absent
handling in one place.
In `@message-worker/handler_test.go`:
- Around line 1837-1882: `markThreadMentions` needs a test for the
`GetHistorySharedSince` failure path, since it now returns a wrapped error from
that lookup. Add a new case in `message-worker/handler_test.go` that sets up
`MockThreadStore.GetHistorySharedSince` to return an error, invokes
`Handler.markThreadMentions`, and asserts the call fails; use the existing
`markThreadMentions` and `GetHistorySharedSince` symbols to locate the behavior
and keep the rest of the setup minimal.
In `@message-worker/helper.go`:
- Around line 10-18: The mention visibility logic in mentionVisible is correct,
but it is being duplicated across message-worker and broadcast-worker, which
risks future drift. Move this shared rule into a common pkg package (for
example, a mentionvisibility helper) and update both helper.go implementations
to call that shared function instead of maintaining separate copies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ce34681-c7e4-49fa-8ce3-fccd5dffec30
📒 Files selected for processing (22)
broadcast-worker/handler.gobroadcast-worker/handler_test.gobroadcast-worker/helper.gobroadcast-worker/helper_test.gobroadcast-worker/integration_test.gobroadcast-worker/main.gobroadcast-worker/metacache_integration_test.gobroadcast-worker/mock_store_test.gobroadcast-worker/store.gobroadcast-worker/store_mongo.godocs/client-api.mddocs/superpowers/plans/2026-06-30-thread-reply-history-visibility.mddocs/superpowers/specs/2026-06-30-thread-reply-history-visibility-design.mdmessage-worker/handler.gomessage-worker/handler_test.gomessage-worker/helper.gomessage-worker/helper_test.gomessage-worker/integration_test.gomessage-worker/mock_store_test.gomessage-worker/store.gomessage-worker/store_cassandra.gomessage-worker/store_mongo.go
…ow-ups Bring broadcast-worker and message-worker in line with notification-worker's history-visibility rule for thread replies, and clear the remaining PR #245 review follow-ups. Visibility gate (mirrors notification-worker.isRestricted): - An @-mentioned user receives/subscribes to a thread reply only if they are a room member (has a subscription) AND their HistorySharedSince admits the parent (nil window, or ParentCreatedAt >= HistorySharedSince). Non-members and members who joined after the parent are excluded. - broadcast-worker: new mentionVisible helper + Store.GetHistorySharedSince; handleThreadCreated/Updated/Deleted gate channel-mention fan-out. Followers (thread_rooms.replyAccounts) are never filtered. - message-worker: new mentionVisible helper + ThreadStore.GetHistorySharedSince; markThreadMentions skips non-member/restricted mentionees (no thread sub, no cross-site inbox, not added to replyAccounts). - history-service: canonical EventUpdated/EventDeleted now carry ThreadParentMessageCreatedAt so the edit/delete gate has the parent timestamp. Other PR #245 follow-ups: - broadcast-worker: DM thread-mention badge writes the THREAD subscription (SetThreadSubscriptionMentions, by parentMessageId) instead of the room subscription; adds a (parentMessageId, userAccount) index. Channel thread-sub hasMention remains message-worker's responsibility. - message-worker: SaveThreadMessage / saveThreadMessageEncrypted now write via a single gocql.UnloggedBatch (matching SaveMessage); idempotency unchanged. - docs/client-api.md: document eventTimestamp on new_message, message_edited, message_reacted. Includes design spec and implementation plan under docs/superpowers/. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SCRr8qfUgbezB7uarvYhdb
Allan-Code-hub
force-pushed
the
claude/broadcast-worker-thread-visibility-d4p1g2
branch
from
bec90d8 to
6271d18
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
broadcast-worker/handler.go (1)
211-230: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winBare error return loses call-site context.
h.allowedThreadMentionserror is returned bare (return err), unlike the very next call (channelThreadFanOut) in the same block which wraps withfmt.Errorf("channel thread fan-out for parent %s: %w", ...). The same pattern repeats at Lines 329-332 and 386-389. As per coding guidelines, "Never return bareerr... without descriptive context" and "Always wrap errors with context usingfmt.Errorf(\"short description: %w\", err)".🛠️ Proposed fix
allowedMentions, err := h.allowedThreadMentions(ctx, msg.RoomID, parsed.Accounts, msg.ThreadParentMessageCreatedAt) if err != nil { - return err + return fmt.Errorf("gate thread mentions for parent %s: %w", parentMsgID, err) }🤖 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 `@broadcast-worker/handler.go` around lines 211 - 230, The error from h.allowedThreadMentions is returned bare, which drops call-site context and is inconsistent with the surrounding h.channelThreadFanOut handling. Update the return in this thread-reply fan-out block to wrap the error with a descriptive fmt.Errorf message including the parent message ID (similar to the existing channelThreadFanOut wrap), and apply the same contextual wrapping pattern anywhere the same bare return appears in handler.go, especially around the other allowedThreadMentions call sites.Source: Coding guidelines
🧹 Nitpick comments (2)
broadcast-worker/handler.go (1)
211-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated parse → gate → fan-out sequence.
The
mention.Parse+allowedThreadMentions+channelThreadFanOutorchestration is duplicated verbatim acrosshandleThreadCreated,handleThreadUpdated, andhandleThreadDeleted. A small private helper would remove the triplication and centralize the error-wrapping fix above.func (h *Handler) gatedChannelFanOut(ctx context.Context, roomID, parentMsgID, sender, content string, parentCreatedAt *time.Time) ([]string, error) { parsed := mention.Parse(content) allowed, err := h.allowedThreadMentions(ctx, roomID, parsed.Accounts, parentCreatedAt) if err != nil { return nil, fmt.Errorf("gate thread mentions for parent %s: %w", parentMsgID, err) } return h.channelThreadFanOut(ctx, parentMsgID, sender, allowed) }Also applies to: 329-333, 386-390
🤖 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 `@broadcast-worker/handler.go` around lines 211 - 230, The parse → gate → fan-out flow is duplicated in handleThreadCreated, handleThreadUpdated, and handleThreadDeleted, so extract it into a small private helper like gatedChannelFanOut on Handler. Have the helper perform mention.Parse, call allowedThreadMentions, then channelThreadFanOut, and wrap errors with the parent message context so the same behavior is centralized and easier to maintain.broadcast-worker/handler_test.go (1)
2176-2224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten
GetHistorySharedSincemock to assert the exact mentioned accounts.
gomock.Any()is used for theaccountsargument, so an implementation bug that queries the wrong account set (e.g., missingdaveor passing stale mentions) would not be caught by this test, even though this gating logic is the core visibility guarantee under test.🧪 Proposed tightening
- store.EXPECT().GetHistorySharedSince(gomock.Any(), "room-1", gomock.Any()). + store.EXPECT().GetHistorySharedSince(gomock.Any(), "room-1", []string{"bob", "carol", "dave"}). Return(map[string]*time.Time{"bob": nil, "carol": &joinedAfter}, nil)🤖 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 `@broadcast-worker/handler_test.go` around lines 2176 - 2224, The test for HandleMessage is too loose because GetHistorySharedSince is mocked with gomock.Any() for the mentioned accounts, so it won’t catch bugs where the handler passes the wrong set of usernames. Tighten TestHandleThreadCreated_ChannelExcludesRestrictedAndNonMemberMentions by asserting the exact accounts sent from the mention parsing/visibility path in HandleMessage, using the GetHistorySharedSince expectation (and related FindUsersByAccounts call if needed) to verify the handler queries only the intended mentionees like bob, carol, and dave.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-06-30-thread-reply-history-visibility.md`:
- Around line 269-275: The history-sharing tests in the shared-since assertions
can pass even if a nil-window member is missing from the result map. Update the
checks around GetHistorySharedSince so the bob case also verifies key presence
before asserting nil, mirroring the existing present/absent distinction already
used for carol. Use the existing GetHistorySharedSince call and the got map
assertions to add a presence check for every member with historySharedSince ==
nil.
---
Outside diff comments:
In `@broadcast-worker/handler.go`:
- Around line 211-230: The error from h.allowedThreadMentions is returned bare,
which drops call-site context and is inconsistent with the surrounding
h.channelThreadFanOut handling. Update the return in this thread-reply fan-out
block to wrap the error with a descriptive fmt.Errorf message including the
parent message ID (similar to the existing channelThreadFanOut wrap), and apply
the same contextual wrapping pattern anywhere the same bare return appears in
handler.go, especially around the other allowedThreadMentions call sites.
---
Nitpick comments:
In `@broadcast-worker/handler_test.go`:
- Around line 2176-2224: The test for HandleMessage is too loose because
GetHistorySharedSince is mocked with gomock.Any() for the mentioned accounts, so
it won’t catch bugs where the handler passes the wrong set of usernames. Tighten
TestHandleThreadCreated_ChannelExcludesRestrictedAndNonMemberMentions by
asserting the exact accounts sent from the mention parsing/visibility path in
HandleMessage, using the GetHistorySharedSince expectation (and related
FindUsersByAccounts call if needed) to verify the handler queries only the
intended mentionees like bob, carol, and dave.
In `@broadcast-worker/handler.go`:
- Around line 211-230: The parse → gate → fan-out flow is duplicated in
handleThreadCreated, handleThreadUpdated, and handleThreadDeleted, so extract it
into a small private helper like gatedChannelFanOut on Handler. Have the helper
perform mention.Parse, call allowedThreadMentions, then channelThreadFanOut, and
wrap errors with the parent message context so the same behavior is centralized
and easier to maintain.
🪄 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: baf44548-89d9-42f8-9c7a-1658cd5d1956
📒 Files selected for processing (24)
broadcast-worker/handler.gobroadcast-worker/handler_test.gobroadcast-worker/helper.gobroadcast-worker/helper_test.gobroadcast-worker/integration_test.gobroadcast-worker/main.gobroadcast-worker/metacache_integration_test.gobroadcast-worker/mock_store_test.gobroadcast-worker/store.gobroadcast-worker/store_mongo.godocs/client-api.mddocs/superpowers/plans/2026-06-30-thread-reply-history-visibility.mddocs/superpowers/specs/2026-06-30-thread-reply-history-visibility-design.mdhistory-service/internal/service/messages.gohistory-service/internal/service/messages_test.gomessage-worker/handler.gomessage-worker/handler_test.gomessage-worker/helper.gomessage-worker/helper_test.gomessage-worker/integration_test.gomessage-worker/mock_store_test.gomessage-worker/store.gomessage-worker/store_cassandra.gomessage-worker/store_mongo.go
✅ Files skipped from review due to trivial changes (3)
- broadcast-worker/helper_test.go
- broadcast-worker/mock_store_test.go
- docs/superpowers/specs/2026-06-30-thread-reply-history-visibility-design.md
🚧 Files skipped from review as they are similar to previous changes (18)
- broadcast-worker/metacache_integration_test.go
- message-worker/helper.go
- message-worker/helper_test.go
- broadcast-worker/helper.go
- broadcast-worker/store.go
- message-worker/store.go
- history-service/internal/service/messages_test.go
- message-worker/handler.go
- history-service/internal/service/messages.go
- broadcast-worker/main.go
- broadcast-worker/store_mongo.go
- message-worker/mock_store_test.go
- docs/client-api.md
- message-worker/store_mongo.go
- message-worker/store_cassandra.go
- message-worker/handler_test.go
- broadcast-worker/integration_test.go
- message-worker/integration_test.go
Comment on lines
+269
to
+275
| got, err := store.GetHistorySharedSince(ctx, "r1", []string{"alice", "bob", "carol"}) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, got["alice"]) | ||
| assert.Equal(t, shared.UnixMilli(), got["alice"].UTC().UnixMilli()) | ||
| assert.Nil(t, got["bob"], "member without window decodes to nil") | ||
| _, present := got["carol"] | ||
| assert.False(t, present, "non-member is absent from the map") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert key presence for the nil-window member cases.
assert.Nil(t, got["bob"]) passes even if bob is absent from the map, so these tests would still pass when a member with historySharedSince == nil is incorrectly dropped. Add a presence check in both snippets.
♻️ Minimal fix
- assert.Nil(t, got["bob"], "member without window decodes to nil")
+ val, ok := got["bob"]
+ require.True(t, ok, "member with nil window should still be present")
+ assert.Nil(t, val, "member without window decodes to nil")Also applies to: 665-671
🤖 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-06-30-thread-reply-history-visibility.md` around
lines 269 - 275, The history-sharing tests in the shared-since assertions can
pass even if a nil-window member is missing from the result map. Update the
checks around GetHistorySharedSince so the bob case also verifies key presence
before asserting nil, mirroring the existing present/absent distinction already
used for carol. Use the existing GetHistorySharedSince call and the got map
assertions to add a presence check for every member with historySharedSince ==
nil.
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
Implements thread-reply history visibility gating for @-mentioned users and clears PR #245 follow-ups. Mirrors
notification-worker'sisRestrictedlogic to prevent restricted or non-member mentionees from receiving thread replies they cannot see. Also redirects DM thread-mention badges from room subscriptions to thread subscriptions, and batches Cassandra thread-message writes for efficiency.Key Changes
broadcast-worker & message-worker: Add
mentionVisible(historySharedSince, parentCreatedAt *time.Time) boolhelper to gate mention visibility by room subscription history window. A user is allowed only whenHistorySharedSince == nil(unrestricted) ORParentCreatedAt >= HistorySharedSince(joined at/before parent).broadcast-worker store: Add
GetHistorySharedSince(roomID, accounts)to fetch room-subscription history windows (returns map of account →*time.Time, with non-members absent from the map). AddSetThreadSubscriptionMentions(parentMessageID, accounts)to mark thread subscriptions'hasMention=true(not room subscriptions).broadcast-worker handler: Gate channel mention fan-out via
allowedThreadMentions()— filters mentions to members only and applies history-window visibility check. Redirect DM mention badge fromSetSubscriptionMentions(room) toSetThreadSubscriptionMentions(thread).message-worker store: Add
GetHistorySharedSince(roomID, accounts)to ThreadStore for gating thread-subscription creation on mention-only paths.message-worker handler: Gate thread-subscription creation for mention-only subscribers — skip subscription and
replyAccountswrite when mentionee is restricted or non-member.message-worker Cassandra: Convert
SaveThreadMessagefrom 2–3 sequential INSERTs to oneUnloggedBatch, matching theSaveMessagepattern for efficiency.Client API docs: Document
eventTimestampfield (milliseconds since Unix epoch, UTC) in the MessageEvent payload.Implementation Details
subscriptionscollection by(roomId, u.account)with precise projection — no new index needed (covered by existing membership indexes).(parentMessageId, userAccount)index to serve the update filter; co-exists with message-worker's unique(threadRoomId, userAccount)index.GetHistorySharedSinceresult map — key-presence encodes membership.UnloggedBatch(not Logged) because each INSERT is idempotent on its primary key and JetStream redelivery safely re-runs the whole batch.claude/broadcast-worker-thread-visibility-d4p1g2.https://claude.ai/code/session_01SCRr8qfUgbezB7uarvYhdb
Summary by CodeRabbit
@mentions, so only recipients who can view the thread parent message receive delivery and mention indicators.@-mention delivery for channel thread replies.TShowmirror writes.eventTimestamp(epoch ms UTC) to documented message-related client event payloads.