Skip to content

Conversation

vladvelici
Copy link
Contributor

@vladvelici vladvelici commented Sep 2, 2025

Description

https://ably.atlassian.net/browse/CHA-1098
https://ably.atlassian.net/browse/CHA-1099

Checklist

Copy link

coderabbitai bot commented Sep 2, 2025

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chat/clipped-summaries

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.

@vladvelici vladvelici force-pushed the chat/clipped-summaries branch from 13b9890 to 4590f1a Compare September 2, 2025 09:30
@vladvelici vladvelici requested a review from AndyTWF September 2, 2025 09:31

When a summary is clipped:
- The `total` property shows the total number of reactions as expected, but the `clientIds` property will contain only a partial list of client IDs.
- There will be a property `clipped` set to `true`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick - would probably worth this the "clipped" property will be set to true, as otherwise it makes the property sound optional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reworded


If multiple reactions are sent in a short period of time, multiple reactions may be rolled up and only a single summary event will be published that contains the aggregated results of all reactions. This reduces the number of outbound messages and thus your costs in busy rooms.

#### Large summaries <a id="large-summaries"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need a language selector conditional until we have it in Kotlin/Swift?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we hide it or let people be aware even if field not yet available?

Copy link
Contributor Author

@vladvelici vladvelici Sep 10, 2025

Choose a reason for hiding this comment

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

Wrapped in <If> in the end

@vladvelici vladvelici force-pushed the chat/clipped-summaries branch from 4590f1a to efbb17d Compare September 9, 2025 14:53
@vladvelici vladvelici force-pushed the chat/clipped-summaries branch from efbb17d to 52aa079 Compare September 10, 2025 13:19
@vladvelici vladvelici requested a review from AndyTWF September 10, 2025 13:19
@vladvelici vladvelici merged commit 504be28 into main Sep 11, 2025
6 checks passed
@vladvelici vladvelici deleted the chat/clipped-summaries branch September 11, 2025 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants