Skip to content

Conversation

@ricardogarim
Copy link
Contributor

@ricardogarim ricardogarim commented Oct 2, 2025

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

Summary by CodeRabbit

  • Bug Fixes
    • Improved reliability of reaction redactions in federated Matrix events by correctly reading nested event data. This ensures reactions are consistently identified and removed when redacted, reducing missed or incorrectly processed reactions. No changes to user interface or workflows.

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Oct 2, 2025

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label
  • This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Oct 2, 2025

⚠️ No Changeset found

Latest commit: 8cc48b6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 2, 2025

Walkthrough

Updates reaction redaction handling in ee/packages/federation-matrix/src/events/reaction.ts to read nested event fields (reactionEvent.event.type and reactionEvent.event.content) instead of top-level properties, maintaining existing control flow and error handling.

Changes

Cohort / File(s) Summary
Matrix reaction event access update
ee/packages/federation-matrix/src/events/reaction.ts
Switches from top-level reactionEvent.type/content to reactionEvent.event.type/content when validating and processing m.relates_to in redaction handler. No exported API changes.

Sequence Diagram(s)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

stat: ready to merge, stat: QA assured

Suggested reviewers

  • ggazzo
  • rodrigok

Poem

A bunny hops through nested fields so neat,
From type to content, tucked within the beat.
Reactions fade when redactions chime,
No phantom likes to haunt through time.
Thump-thump—clean trails in matrix land,
Little paws, precise and planned. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title accurately and concisely summarizes the fix by indicating that remote reaction unsetting in the federation module is being addressed. It follows conventional prefix usage and clearly reflects the main change in the pull request. It is clear and specific enough for teammates scanning history to understand the primary change without additional noise.
Linked Issues Check ✅ Passed The PR corrects the nested event property access for reaction redaction events so that removals are properly detected and handled across federated rooms. This ensures that reaction unsets on the origin room propagate to remote rooms, fulfilling the key requirement of FDR-187. The changes align with the issue’s objectives by enabling zero reaction counts when all users un-react. No unrelated logic has been altered, and the update directly targets the propagation bug described in the issue.
Out of Scope Changes Check ✅ Passed All modifications are confined to the reaction event handling code in the federation-matrix package and directly relate to processing remote reaction unsets. There are no additions or alterations to unrelated modules or features. The changes specifically target the bug described in FDR-187, and no out-of-scope functionality has been modified. This focused update ensures that only relevant code is touched.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/federation-unset-reactions

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 40a45d8 and 8cc48b6.

📒 Files selected for processing (1)
  • ee/packages/federation-matrix/src/events/reaction.ts (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: CodeQL
  • GitHub Check: 🔎 Code Check / Code Lint
  • GitHub Check: 🔎 Code Check / TypeScript
  • GitHub Check: 🔨 Test Storybook / Test Storybook
  • GitHub Check: 🔨 Test Unit / Unit Tests
  • GitHub Check: 📦 Meteor Build - coverage
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build
🔇 Additional comments (1)
ee/packages/federation-matrix/src/events/reaction.ts (1)

59-64: Proceeding to retrieve the snippet for confirmation.


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 and usage tips.

@codecov
Copy link

codecov bot commented Oct 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.39%. Comparing base (dca0b3a) to head (8cc48b6).
⚠️ Report is 8 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #37121      +/-   ##
===========================================
+ Coverage    67.38%   67.39%   +0.01%     
===========================================
  Files         3328     3330       +2     
  Lines       113342   113490     +148     
  Branches     20562    20600      +38     
===========================================
+ Hits         76375    76492     +117     
- Misses       34363    34393      +30     
- Partials      2604     2605       +1     
Flag Coverage Δ
e2e 57.35% <ø> (+0.06%) ⬆️
e2e-api 40.56% <ø> (+0.13%) ⬆️
unit 71.16% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@rodrigok rodrigok merged commit 1657571 into develop Oct 2, 2025
51 checks passed
@rodrigok rodrigok deleted the fix/federation-unset-reactions branch October 2, 2025 20:24
@rodrigok rodrigok restored the fix/federation-unset-reactions branch October 2, 2025 20:28
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.

3 participants