Implement message reaction write path and cross-site delivery#258
Open
general-lex wants to merge 1 commit into
Open
Implement message reaction write path and cross-site delivery#258general-lex wants to merge 1 commit into
general-lex wants to merge 1 commit into
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 message reactions end-to-end: event model, shortcode validator + cache, Mongo custom-emoji repo, Cassandra per-cell reaction writes with mirror fan-out, HistoryService ReactMessage handler, canonical reacted publication, broadcast/notification fan-out, search-sync skip, tests, and docs. ChangesMessage Reactions Feature
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
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)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/natsutil/canonical_dedup.go (1)
32-39: ⚡ Quick winConsider logging the nil-delta fallback for observability.
The nil-delta fallback is well-documented and intentional (collisions make programmer errors visible), but there's no runtime logging when this fallback occurs. Adding
slog.Errorhere would make it easier to detect and diagnose publisher-side bugs in production without relying solely on downstream validation.📊 Proposed addition
case model.EventReacted: if evt.ReactionDelta == nil { + slog.Error("EventReacted missing ReactionDelta, falling back to bare messageID dedup", + "messageID", evt.Message.ID) return evt.Message.ID }🤖 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 `@pkg/natsutil/canonical_dedup.go` around lines 32 - 39, Add an observability log when the nil-delta fallback is used in the EventReacted branch: inside the case for model.EventReacted, when checking evt.ReactionDelta == nil (the current early-return that yields evt.Message.ID), emit a structured slog.Error or slog.Warn that includes identifying context (evt.Message.ID, evt.Event, evt.Timestamp and any other useful fields) so publisher-side bugs are visible; keep the existing return value unchanged. Target the code around the EventReacted case in canonical_dedup.go and the ReactionDelta nil-check to insert the log.history-service/internal/mongorepo/custom_emoji.go (1)
39-53: Consider caching custom-emoji existence.Each non-built-in reaction triggers a Mongo
FindOne. Custom emoji sets change rarely and are small per site, so a short-TTL or bounded LRU (the repo already depends onhashicorp/golang-lru/v2) keyed by(siteId, shortcode)would remove a per-reaction round-trip on the hot path. Built-ins already short-circuit before the lookup, so this only affects the custom path.🤖 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 `@history-service/internal/mongorepo/custom_emoji.go` around lines 39 - 53, CustomEmojiExists currently does a Mongo FindOne on every call; add an in-memory cache (use the existing hashicorp/golang-lru/v2 dependency) on CustomEmojiRepo to avoid round-trips for hot custom emoji lookups. Modify CustomEmojiRepo to hold an LRU cache (or a small bounded cache with short TTL semantics stored with the cached value), key entries by a deterministic string like siteID+"|"+shortcode, and update CustomEmojiExists to first consult the cache and return cached results when present; on cache miss, perform the FindOne as today, then insert the boolean result into the cache (and handle errors unchanged). Ensure cache is initialized when the repo is created and consider eviction/TTL to reflect rare updates to custom emoji sets.
🤖 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 `@history-service/internal/mongorepo/custom_emoji.go`:
- Around line 39-53: CustomEmojiExists currently does a Mongo FindOne on every
call; add an in-memory cache (use the existing hashicorp/golang-lru/v2
dependency) on CustomEmojiRepo to avoid round-trips for hot custom emoji
lookups. Modify CustomEmojiRepo to hold an LRU cache (or a small bounded cache
with short TTL semantics stored with the cached value), key entries by a
deterministic string like siteID+"|"+shortcode, and update CustomEmojiExists to
first consult the cache and return cached results when present; on cache miss,
perform the FindOne as today, then insert the boolean result into the cache (and
handle errors unchanged). Ensure cache is initialized when the repo is created
and consider eviction/TTL to reflect rare updates to custom emoji sets.
In `@pkg/natsutil/canonical_dedup.go`:
- Around line 32-39: Add an observability log when the nil-delta fallback is
used in the EventReacted branch: inside the case for model.EventReacted, when
checking evt.ReactionDelta == nil (the current early-return that yields
evt.Message.ID), emit a structured slog.Error or slog.Warn that includes
identifying context (evt.Message.ID, evt.Event, evt.Timestamp and any other
useful fields) so publisher-side bugs are visible; keep the existing return
value unchanged. Target the code around the EventReacted case in
canonical_dedup.go and the ReactionDelta nil-check to insert the log.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d1e66dc-7e33-476e-a6c8-091dc3b28d0b
📒 Files selected for processing (33)
broadcast-worker/handler.gobroadcast-worker/handler_test.godocs/client-api.mddocs/nats-subject-naming.mddocs/specs/message-reactions.mddocs/superpowers/specs/2026-05-28-message-reactions-write-path-design.mdgo.modhistory-service/cmd/main.gohistory-service/internal/cassrepo/reactions.gohistory-service/internal/cassrepo/reactions_integration_test.gohistory-service/internal/models/message.gohistory-service/internal/mongorepo/custom_emoji.gohistory-service/internal/service/integration_test.gohistory-service/internal/service/messages_test.gohistory-service/internal/service/mocks/mock_repository.gohistory-service/internal/service/reactions.gohistory-service/internal/service/reactions_test.gohistory-service/internal/service/service.gonotification-worker/handler.gonotification-worker/handler_test.gopkg/emoji/allowlist.gopkg/emoji/emoji.gopkg/emoji/emoji_test.gopkg/model/cassandra/reactions.gopkg/model/custom_emoji.gopkg/model/event.gopkg/model/model_test.gopkg/natsutil/canonical_dedup.gopkg/natsutil/canonical_dedup_test.gopkg/subject/subject.gopkg/subject/subject_test.gosearch-sync-worker/messages.gosearch-sync-worker/messages_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/emoji/emoji_test.go (1)
172-173:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix the NFC equivalence test: both forms are currently identical.
The test claims to verify that precomposed and decomposed Unicode forms collapse under NFC normalization, but both
precomposedanddecomposedare set to the same string literal"é". This means the test verifies the same input twice instead of proving that NFC collapses equivalent forms.🧪 Proposed fix using explicit Unicode escape sequences
- // "é" in two forms: precomposed (U+00E9) vs decomposed (U+0065 U+0301). - precomposed := "é" - decomposed := "é" + // "é" in two forms: precomposed (U+00E9) vs decomposed (U+0065 U+0301). + precomposed := "\u00e9" // é as a single codepoint + decomposed := "\u0065\u0301" // e + combining acute accentAlternatively, construct the decomposed form programmatically:
+import "golang.org/x/text/unicode/norm" + func TestValidator_Validate_NFC_CollapsesEquivalentForms(t *testing.T) { v := emoji.NewValidator(&stubLookup{}) - // "é" in two forms: precomposed (U+00E9) vs decomposed (U+0065 U+0301). - precomposed := "é" - decomposed := "é" + // "é" in two forms: precomposed (U+00E9) vs decomposed (U+0065 U+0301). + precomposed := "é" + decomposed := norm.NFD.String("é") // decompose to e + combining acuteEither approach ensures the test actually verifies that two different byte sequences representing the same visual character are handled consistently.
🤖 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 `@pkg/emoji/emoji_test.go` around lines 172 - 173, The NFC equivalence test is wrong because precomposed and decomposed are identical; change the test so precomposed is the single-codepoint form and decomposed is the base letter plus combining accent (or construct decomposed programmatically using runes) so they are different byte sequences that normalize to the same NFC; update the variables precomposed and decomposed in emoji_test.go (used by the test function that asserts NFC collapse) to use explicit Unicode escape sequences (e.g., U+00E9 vs U+0065 U+0301) or build decomposed with runes before running the normalization/assertion.
🤖 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.
Outside diff comments:
In `@pkg/emoji/emoji_test.go`:
- Around line 172-173: The NFC equivalence test is wrong because precomposed and
decomposed are identical; change the test so precomposed is the single-codepoint
form and decomposed is the base letter plus combining accent (or construct
decomposed programmatically using runes) so they are different byte sequences
that normalize to the same NFC; update the variables precomposed and decomposed
in emoji_test.go (used by the test function that asserts NFC collapse) to use
explicit Unicode escape sequences (e.g., U+00E9 vs U+0065 U+0301) or build
decomposed with runes before running the normalization/assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 51427407-3e7a-4546-b1a9-695ae1d24f04
📒 Files selected for processing (5)
docs/client-api.mddocs/specs/message-reactions.mddocs/superpowers/specs/2026-05-28-message-reactions-write-path-design.mdpkg/emoji/emoji.gopkg/emoji/emoji_test.go
✅ Files skipped from review due to trivial changes (2)
- docs/specs/message-reactions.md
- docs/superpowers/specs/2026-05-28-message-reactions-write-path-design.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/client-api.md
- pkg/emoji/emoji.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
notification-worker/handler_test.go (1)
305-307: ⚡ Quick winUse
getRecords()for consistency.Accessing
pub.recordsdirectly is inconsistent with the pattern used elsewhere in this file (e.g., lines 88, 149, 219, 258, 281). While safe in this single-threaded test, usinggetRecords()maintains consistency and avoids establishing a pattern that could be problematic in concurrent contexts.♻️ Suggested refactor
- if len(pub.records) != 0 { - t.Errorf("expected no notification publish on malformed event, got %d records", len(pub.records)) + if got := len(pub.getRecords()); got != 0 { + t.Errorf("expected no notification publish on malformed event, got %d records", got) }🤖 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 `@notification-worker/handler_test.go` around lines 305 - 307, Replace the direct access to pub.records with the accessor method getRecords() for consistency with other tests; locate the assertion that checks the length (currently using pub.records) and call pub.getRecords() to get the slice, then assert its length (e.g., check len(pub.getRecords()) == 0) so the test uses the same getRecords() pattern as in other cases (lines referencing pub.getRecords()) and avoids direct field reads.
🤖 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 `@notification-worker/handler_test.go`:
- Around line 305-307: Replace the direct access to pub.records with the
accessor method getRecords() for consistency with other tests; locate the
assertion that checks the length (currently using pub.records) and call
pub.getRecords() to get the slice, then assert its length (e.g., check
len(pub.getRecords()) == 0) so the test uses the same getRecords() pattern as in
other cases (lines referencing pub.getRecords()) and avoids direct field reads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f8fdf582-2f99-4535-8287-d49ac601aee4
📒 Files selected for processing (4)
broadcast-worker/handler.gobroadcast-worker/handler_test.gonotification-worker/handler.gonotification-worker/handler_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- notification-worker/handler.go
Adds message reactions on top of the v3 reactions schema:
- Storage: per-cell map UPDATE/DELETE on messages_by_id (source of
truth) plus the room-or-thread mirror; pinned table deliberately
excluded since the FE does not render reactions in the pinned panel.
- Validation: pkg/emoji.Validator (NFC + 256B input cap + regex +
custom-emoji Mongo lookup) with pkg/emoji.CachedLookup as the
LRU+TTL+singleflight cache so the hot path doesn't hit Mongo per
request.
- Handler: history-service.ReactMessage — input checks, validate,
access check, findMessage, resolve actor, in-row toggle decision,
Cassandra write, best-effort canonical publish on
chat.msg.canonical.{siteID}.reacted.
- Fan-out: broadcast-worker.handleReacted (channel + DM routing, no
encryption branch), notification-worker.handleReaction (author-only,
added-only, no self-react), search-sync-worker skips reaction
events. Both reaction-consumer guards on malformed payloads
log-and-Ack rather than NAK to avoid poison-pill loops.
- Model: EventReacted + ReactionDelta + ReactRoomEvent +
ReactionAction defined type. ReactionAction propagated through
ReactMessageResponse and the live event so a typo fails to compile.
- Subjects: MsgReactPattern + MsgCanonicalReacted via pkg/subject.
- Dedup: CanonicalDedupID extended for EventReacted (actor +
shortcode + action + timestamp); nil-delta is a 'loud collision'
fallback to the bare messageID by design.
- Custom emoji Mongo adapter with EnsureIndexes for the
(siteId, shortcode) unique compound index — the only dedup mechanism.
Documented design caveats: live event ordering best-effort under
JetStream redelivery (clients reconcile via REST refetch), reactions-
specific retry caveat on mirror torn state (same root cause as
edit/delete), frozen-UDT extensibility caveat on the schema, custom
emoji collection populated out-of-band today, no handler-level
deadline (per-driver bound convention).
https://claude.ai/code/session_01Dq1dEXdNcRNCchcBu5bcWn
general-lex
force-pushed
the
claude/add-reaction-support-jd0tU
branch
from
79d72dd to
4b4fc8d
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
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
Completes the message reactions feature by implementing the write path (add/remove reactions) and cross-site delivery. Reactions are toggled via NATS request/reply, written atomically per-cell to Cassandra's four mirror tables, and broadcast to subscribers via the existing NATS gateway infrastructure. The read path (schema, UDTs, inline reads) was already shipped in a prior commit.
Key Changes
Core reaction write logic:
history-service/internal/cassrepo/reactions.go— Per-cellUPDATE(add) andDELETE(remove) operations on all four message tables (messages_by_id,messages_by_room,thread_messages_by_thread,pinned_messages_by_room). Writes tomessages_by_idfirst as source of truth; remaining mirrors run concurrently viaerrgroup.history-service/internal/service/reactions.go—ReactMessagehandler validates shortcode (built-in or custom emoji), checks subscription and message existence, decides add-vs-remove from in-row map, publishes canonical event for downstream workers.Emoji validation:
pkg/emoji/emoji.go+pkg/emoji/allowlist.go— Shortcode validator supporting built-in Unicode emojis (thumbsup, tada, +1, -1, etc.) and site-scoped custom emoji lookups. Enforces wire format:[a-z0-9][a-z0-9_+-]{0,31}, NFC-normalizes input.history-service/internal/mongorepo/custom_emoji.go— MongoDB adapter for custom emoji existence checks.Event model & downstream dispatch:
pkg/model/event.go— NewEventReactedevent type andReactionDeltastruct (shortcode, action, actor).broadcast-worker/handler.go—handleReactedroutes reactions to room-scoped or DM channels (no encryption branch, no message content). PublishesReactRoomEventwith delta.notification-worker/handler.go— PublishesNotificationType:"reaction"to message author only when action is "added" and actor is not the author.search-sync-worker/messages.go— Skips reaction events (reactions don't change indexed content).Subject & dedup:
pkg/subject/subject.go— NewMsgReactPatternandMsgCanonicalReactedsubject builders.pkg/natsutil/canonical_dedup.go— Dedup ID includes actor account, shortcode, action, and timestamp to ensure idempotency across retries.Integration & wiring:
history-service/cmd/main.go— InjectsUserStoreandCustomEmojiStoreinto service; registersReactMessagehandler.history-service/internal/service/service.go— AddsUserStoreandCustomEmojiStoreinterfaces; updates mockgen directive.history-service/internal/models/message.go— ExportsReactionKeyandReactorInfotype aliases from Cassandra models.Tests:
history-service/internal/cassrepo/reactions_integration_test.go— Integration tests forAddReactionandRemoveReactionacross all four mirror tables (top-level, thread reply, pinned).history-service/internal/service/reactions_test.go— Unit tests for validation, subscription checks, message existence, soft-delete blocking, toggle logic, and event publishing.pkg/emoji/emoji_test.go— Shortcode validation tests (built-in, custom, format, length boundaries).broadcast-worker/handler_test.go— Tests for channel-scoped and DM fan-out of reaction events.notification-worker/handler_test.go— Tests for author-only notification on add, no notification on remove.pkg/model/model_test.go— JSON round-trip tests forReactionDeltaandNotificationEventwith reaction payload.https://claude.ai/code/session_01Dq1dEXdNcRNCchcBu5bcWn
Summary by CodeRabbit
New Features
Behavior
Documentation
Tests