Skip to content

fix(reactions): dedup unicode reaction so it shows once#129

Merged
krazyjakee merged 3 commits into
masterfrom
claude/crazy-galileo-5ac34b
Jun 14, 2026
Merged

fix(reactions): dedup unicode reaction so it shows once#129
krazyjakee merged 3 commits into
masterfrom
claude/crazy-galileo-5ac34b

Conversation

@krazyjakee

@krazyjakee krazyjakee commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

What

Adding a unicode emoji reaction from the emoji picker showed up twice (two pills). This fixes it to show a single pill at the correct count.

Why

The emoji picker passes toggleReaction a unicode emoji's shortcode name (e.g. hamburger), so the optimistic pill was stored/keyed by that shortcode. The gateway's reaction.add echo arrives keyed by the glyph (🍔) — what the server actually stored. The dedup key (_emojiName) returned the shortcode for the optimistic pill and the glyph for the echo, so they never matched: the echo created a second pill instead of merging, and the includesMe double-count guard never fired.

Tapping an existing pill didn't double, because that path already passes the glyph — only the picker path passed a shortcode.

How

Introduce a canonical _emojiKey(name, id) helper that resolves unicode emoji to their glyph (custom emoji still key on name, with the id carried separately). All three reaction touchpoints now route through it:

  • toggleReaction — the "do I already have this reaction?" lookup
  • applyReaction — the match-or-create index, and it now stores the canonical key as the reaction's name
  • _emojiName — delegates to _emojiKey

So hamburger (optimistic) and 🍔 (gateway echo) both normalize to 🍔 and collapse to one pill.

Notes for reviewers

  • The pill renderer already resolves names to glyphs via resolveEmojiGlyph, so storing the glyph as the reaction name renders unchanged.
  • I couldn't run flutter analyze locally (the toolchain requires macOS 14+; this machine is on 13.0) — relying on CI. The change is self-contained; resolveEmojiGlyph/parseEmojiToken were already imported and used in the file.
  • Worth a manual check: react with a standard unicode emoji from the picker and confirm a single pill at count 1.

🤖 Generated with Claude Code

krazyjakee and others added 2 commits June 14, 2026 00:12
@krazyjakee @claude
fix(reactions): dedup unicode reaction by glyph so it shows once
23f042b 
The emoji picker hands toggleReaction a unicode emoji's shortcode name
(e.g. `hamburger`), so the optimistic pill was keyed by the shortcode.
The gateway's reaction.add echo arrives keyed by the glyph (`🍔`) — what
the server stored — so the two never matched and the echo spawned a
second pill instead of merging into the optimistic one.

Canonicalize unicode reactions to their glyph in the dedup key via a new
_emojiKey helper, used by toggleReaction's lookup, applyReaction's
match-or-create (and stored on the reaction), and _emojiName. Now the
optimistic pill and its gateway echo collapse to one, and the existing
includesMe guard correctly blocks the double count.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude
review(reactions): fix clearReactionEmoji shortcode mismatch + add tests
ec3087b 
After the PR introduced _emojiKey (which resolves unicode emoji to their
glyph), _emojiName(r) now always returns a canonical glyph for unicode
reactions. clearReactionEmoji was left comparing that canonical glyph
against the raw emojiName from the gateway, which can arrive as a
shortcode (e.g. `hamburger`). The filter then matched nothing and
reactions were never cleared.

Fix: canonicalise the incoming name through _emojiKey before the
comparison, consistent with how applyReaction and toggleReaction already
handle their inputs.

Tests added in accord_messages_reactions_test.dart covering:
- shortcode optimistic pill + glyph gateway echo collapse to one
- existing pill tap, REST-loaded shortcode removed via glyph
- custom emoji still keyed by name
- clearReactionEmoji with shortcode / glyph / cross-emoji isolation
- count decrement and includesMe guarding

https://claude.ai/code/session_01AoJpr7zMvJzXkTrgoxxRuJ
@krazyjakee Claude

krazyjakee commented Jun 13, 2026

Copy link
Copy Markdown
Contributor Author

Review findings — ranked by severity

🔴 HIGH — Bug: clearReactionEmoji comparison broken after _emojiKey introduction

File: lib/features/messaging/controllers/accord_messages.dart:452

_emojiName(r) now delegates to _emojiKey, which resolves unicode emoji to their canonical glyph (e.g. hamburger → 🍔). But clearReactionEmoji was left comparing that canonical glyph against the raw emojiName argument, which arrives from the gateway reaction.clear_emoji event and can be a shortcode. The filter never matched, so the clear was silently a no-op.

The same is true for reactions loaded from REST before the PR that still have their shortcode stored as the name: _emojiName(r) resolves to the glyph, but the comparison was against the shortcode.

Fix applied (ec3087b):

// before
.where((r) => _emojiName(r) != emojiName)
// after
.where((r) => _emojiName(r) != _emojiKey(emojiName, null))

🟡 MEDIUM — No tests for reaction dedup logic

The core invariant introduced by this PR (shortcode + glyph echo → one pill) had no test coverage. Given the bug found above, tests are particularly important here.

Added in test/features/messaging/accord_messages_reactions_test.dart (ec3087b):

  • Shortcode optimistic pill + glyph gateway echo collapse to one (the primary PR scenario)
  • Existing pill tap via glyph works correctly
  • REST-loaded shortcode-named reaction removed via glyph
  • Custom emoji still keyed by name, not resolved to a glyph
  • clearReactionEmoji with shortcode, glyph, and cross-emoji isolation
  • Count decrement and includesMe guarding

🟢 LOW — No other issues found

  • Code quality: The _emojiKey helper is clearly named, well-commented, and the three callsites all route through it consistently. _emojiName as a thin wrapper is readable.
  • File size: 482 lines — well under threshold.
  • Correctness of the primary fix: toggleReaction and applyReaction are correct; the optimistic add canonicalises via _emojiKey and stores the glyph, so the gateway echo (🍔) finds and merges into the pill rather than creating a second one.
  • No Discord endpoints or regressions introduced.

Note: Flutter/Dart toolchain is not available in this container, so tests weren't run locally — relying on CI for the green signal.

Generated by Claude Code

@krazyjakee @claude
test(reactions): seed own reaction so remove-via-glyph test exercises…
f008a41 
… the match

The REST-loaded-shortcode remove test seeded the pill with the default
includesMe:false, so applyReaction's own-remove guard (isOwn && !includesMe)
bailed before the canonical name match it meant to verify. Seed me:true so the
toggle-off proceeds and the glyph→shortcode match drives the count to zero.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@krazyjakee krazyjakee merged commit 31dda8b into master Jun 14, 2026
2 checks passed
@krazyjakee krazyjakee deleted the claude/crazy-galileo-5ac34b branch June 14, 2026 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@krazyjakee @claude