Skip to content

fix(llc): keep reaction group while its count stays positive - #2858

Merged
xsahil03x merged 3 commits into
masterfrom
sahil/flu-663-fix-reaction-group-delete
Jul 31, 2026
Merged

fix(llc): keep reaction group while its count stays positive#2858
xsahil03x merged 3 commits into
masterfrom
sahil/flu-663-fix-reaction-group-delete

Conversation

@xsahil03x

@xsahil03x xsahil03x commented Jul 31, 2026

Copy link
Copy Markdown
Member

Summary

Message dropped an entire ReactionGroup in two places when a group's summed scores were non-positive, even though its count was still positive — so count-based reaction UIs lost other users' reactions:

  1. deleteMyReaction (optimistic delete) kept a group only when count > 0 && sumScores > 0. During an optimistic delete, a group whose scores net to zero/negative vanished until the next server event re-hydrated it.
  2. _reactionGroupsReadValue (legacy-payload parse fallback that synthesizes groups from the old reaction_counts/reaction_scores maps) discarded a group when sumScores == 0, dropping a still-populated group at parse time. (Flagged in review by @renefloor.)

Root cause / backend alignment

The backend derives reaction groups from the reactions table via count(type) / sum(score) grouped by (message_id, type). A group exists iff at least one reaction row remains (count >= 1); sum(score) is an independent aggregate that may legitimately be 0 or negative and never gates whether the group exists.

Both call sites now gate purely on the count.

Changes

  • deleteMyReaction: retain the group while updatedCount > 0.
  • _reactionGroupsReadValue: retain the group while count > 0 (guard count <= 0 so a malformed negative count can't build an invalid group).
  • Regression tests: realistic delete-path fixture (own score: 1, other score: -1 → net zero, then negative after delete) and a legacy-payload test covering zero and negative score totals.
  • stream_chat CHANGELOG entries.

Testing

flutter test test/src/core/models/message_test.dart test/src/core/models/message_reaction_helper_test.dart — all pass.

Fixes FLU-663

🤖 Generated with Claude Code

Message.deleteMyReaction dropped the entire ReactionGroup whenever the
updated sumScores reached 0, even while the count was still greater than
zero, so count-based reaction UIs lost other users' reactions during the
optimistic delete until the next server event.

Gate the group's survival on count alone, mirroring the backend which
derives groups from the reaction count (sum of scores is an independent
aggregate that may legitimately be zero).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Message now retains reaction groups when their count is positive, even when the aggregate score is zero or negative. This applies to legacy payload parsing and deleteMyReaction. Regression tests and a changelog entry document the behavior.

Changes

Reaction group retention

Layer / File(s) Summary
Retain legacy reaction groups
packages/stream_chat/lib/src/core/models/message.dart, packages/stream_chat/test/src/core/models/message_test.dart
Legacy reaction parsing now skips groups only when their count is nonpositive. The regression test covers positive counts with zero and negative summed scores.
Update reaction deletion retention
packages/stream_chat/lib/src/core/models/message.dart, packages/stream_chat/test/src/core/models/message_reaction_helper_test.dart, packages/stream_chat/CHANGELOG.md
deleteMyReaction retains groups with a positive remaining count regardless of aggregate score. The regression test and changelog document the fix.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: preserving a reaction group while its count remains positive.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sahil/flu-663-fix-reaction-group-delete

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.86%. Comparing base (401ab01) to head (e0d821c).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2858   +/-   ##
=======================================
  Coverage   72.86%   72.86%           
=======================================
  Files         429      429           
  Lines       27698    27698           
=======================================
  Hits        20183    20183           
  Misses       7515     7515           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread packages/stream_chat/lib/src/core/models/message.dart Outdated
The _reactionGroupsReadValue fallback that synthesizes reaction groups
from the old reaction_counts/reaction_scores maps dropped a group when
its sum_scores was 0, discarding a still-populated group at parse time.
Gate on count only, matching the delete-path fix. Also trims the two
reaction-group comments to one terse line each.

Addresses review feedback on #2858.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
xsahil03x added a commit that referenced this pull request Jul 31, 2026
The _maybeGetReactionGroups fallback that derives reaction groups from
the old reaction_counts/reaction_scores maps dropped a group when its
sum_scores was 0, discarding a still-populated group. Gate on count
only, matching the delete-path fix; the score sum may legitimately be
zero or negative. Also trims the reaction-group comments to one terse
line each.

Port of the review follow-up on #2858.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@packages/stream_chat/lib/src/core/models/message.dart`:
- Around line 175-176: Update the count guard in the reaction-group construction
logic to skip all non-positive legacy counts by changing the zero-only check to
a less-than-or-equal check. Retain groups only when count is positive, while
preserving the existing handling of score values.
🪄 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 Plus

Run ID: 3e01010c-fcbd-44f3-8a28-f6759592791c

📥 Commits

Reviewing files that changed from the base of the PR and between 29f7508 and 668ed31.

📒 Files selected for processing (3)
  • packages/stream_chat/CHANGELOG.md
  • packages/stream_chat/lib/src/core/models/message.dart
  • packages/stream_chat/test/src/core/models/message_test.dart

Comment thread packages/stream_chat/lib/src/core/models/message.dart Outdated
- Guard the legacy synthesizer on count <= 0 so a malformed negative
  count can't build an invalid ReactionGroup (CodeRabbit).
- Use a realistic delete-path fixture (own score 1, other score -1)
  instead of unreachable score 0 reactions (renefloor).
- Extend the legacy-payload test to cover a negative score total.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
xsahil03x added a commit that referenced this pull request Jul 31, 2026
- Guard the legacy synthesizer on count <= 0 so a malformed negative
  count can't build an invalid ReactionGroup.
- Use a realistic delete-path fixture (own score 1, other score -1)
  instead of unreachable score 0 reactions.
- Derive the legacy-payload test from the deprecated reactionCounts/
  reactionScores params (a real Message) covering a zero/negative total.

Port of the review follow-up on #2858.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@xsahil03x xsahil03x changed the title fix(llc): keep reaction group while count stays positive on delete fix(llc): keep reaction group while its count stays positive Jul 31, 2026
@xsahil03x
xsahil03x merged commit a7e26f0 into master Jul 31, 2026
31 of 34 checks passed
@xsahil03x
xsahil03x deleted the sahil/flu-663-fix-reaction-group-delete branch July 31, 2026 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants