message-worker/handler_test.go (1)

1837-1882: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a test for GetHistorySharedSince returning an error.

markThreadMentions now wraps and returns an error when GetHistorySharedSince fails (handler.go ~511-513), but no test exercises that branch here.

As per path instructions, **/*_test.go requires "Tests must cover: happy path, error paths, edge cases... and invalid input", and **/*handler_test.go requires "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 win

Consider centralizing mentionVisible instead of duplicating it per service.

This function is byte-identical to message-worker/helper.go's mentionVisible (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 shared pkg/ (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 win

Add store-error coverage for the new GetHistorySharedSince/SetThreadSubscriptionMentions gating.

Both new tests only cover the happy path for allowedThreadMentions. There's no test where store.GetHistorySharedSince (channel or DM path) or store.SetThreadSubscriptionMentions returns an error, to confirm handleThreadCreated propagates 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 win

Correct 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 identical mentionVisible is being added to broadcast-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 shared pkg/ package (e.g., pkg/mentionvisibility) importable by both main packages.

🤖 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 win

Duplicate implementation of GetHistorySharedSince across services.

This is byte-for-byte identical (filter, projection, minimal decode shape, and even the same explanatory comment) to GetHistorySharedSince in message-worker's threadStoreMongo. Both query the same subscriptions-style schema for the same historySharedSince semantics. Worth extracting into a shared helper (e.g., pkg/subscriptionstore or similar) taking a *mongo.Collection parameter, 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.

broadcast-worker/handler.go (1)

211-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider extracting the repeated parse → gate → fan-out sequence.

The mention.Parse + allowedThreadMentions + channelThreadFanOut orchestration is duplicated verbatim across handleThreadCreated, handleThreadUpdated, and handleThreadDeleted. 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 win

Tighten GetHistorySharedSince mock to assert the exact mentioned accounts.

gomock.Any() is used for the accounts argument, so an implementation bug that queries the wrong account set (e.g., missing dave or 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.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes