Skip to content

fix: ensure livechat callbacks fire only on actual updates + code quality improvements#38476

Closed
rushikesh-bobade wants to merge 1 commit intoRocketChat:developfrom
rushikesh-bobade:fix/livechat-callback-and-tests
Closed

fix: ensure livechat callbacks fire only on actual updates + code quality improvements#38476
rushikesh-bobade wants to merge 1 commit intoRocketChat:developfrom
rushikesh-bobade:fix/livechat-callback-and-tests

Conversation

@rushikesh-bobade
Copy link
Copy Markdown

@rushikesh-bobade rushikesh-bobade commented Feb 3, 2026

Summary

This PR addresses several code quality improvements across the Rocket.Chat codebase, focusing on bug fixes, documentation accuracy, and test coverage enhancement.

Motivation

While exploring the codebase, I identified the following opportunities for improvement:

  1. Livechat Callback Optimization: The livechat.setUserStatusLivechat callback was firing regardless of whether a database update actually occurred, potentially causing unnecessary processing and spurious notifications.

  2. Documentation Typo: A spelling error in the Apps-Engine type definitions could cause confusion for developers integrating with the API.

  3. Code Clarity: A TODO comment in the OAuth URI parsing function was unclear about the function's intentional behavior.

  4. Test Coverage Gap: Empty message edge case was not covered in the markdown parsing tests.

Changes Made

Bug Fix: Livechat Status Callback

File: apps/meteor/app/livechat/server/lib/utils.ts

Moved callbacks.runAsync('livechat.setUserStatusLivechat', ...) inside the if (modifiedCount > 0) conditional blocks in both setUserStatusLivechat() and setUserStatusLivechatIf() functions. This ensures callbacks only execute when the database state actually changes, preventing unnecessary event processing.

Before: Callback fired on every function call
After: Callback fires only when modifiedCount > 0

Documentation Fix: Typo Correction

File: packages/apps-engine/src/definition/messages/IMessageReactionContext.ts

Fixed typo in JSDoc comment: recievedreceived

Documentation Improvement: OAuth URI Parsing

File: apps/meteor/app/oauth2-server-config/server/admin/functions/parseUriList.ts

Replaced ambiguous TODO comment with clear documentation explaining the function's intentional behavior of normalizing URIs to a comma-delimited string for consistent storage.

Test Coverage: Empty Message Handling

File: apps/meteor/client/lib/parseMessageTextToAstMarkdown.spec.ts

Added unit test to verify that parseMessageTextToAstMarkdown() correctly returns an empty md array when the message content is empty. This ensures the edge case is covered and prevents potential regressions.

Testing

  • New unit test added for empty message parsing
  • Existing test suite should be run to verify no regressions

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • Documentation update
  • Test coverage improvement

Checklist

  • I have read the Contributing Guidelines
  • I have signed the CLA
  • My code follows the project's coding standards
  • I have added tests that prove my fix is effective
  • My changes generate no new warnings

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Optimized livechat status update callbacks to execute only when data modifications are confirmed, improving operational efficiency.
  • Tests

    • Added test coverage for empty message input handling.

@rushikesh-bobade rushikesh-bobade requested review from a team as code owners February 3, 2026 18:45
@dionisio-bot
Copy link
Copy Markdown
Contributor

dionisio-bot Bot commented Feb 3, 2026

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

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Feb 3, 2026

Walkthrough

Four files receive targeted improvements: callback invocation is now conditional in livechat utilities (executed only when records are modified), a clarifying comment update in OAuth2 URI parsing, a new test case for empty message parsing, and a documentation typo correction in message reaction context.

Changes

Cohort / File(s) Summary
Livechat Utility Callbacks
apps/meteor/app/livechat/server/lib/utils.ts
Modified setUserStatusLivechat and setUserStatusLivechatIf to invoke callbacks conditionally only when modifiedCount > 0, preventing unnecessary callback execution.
OAuth2 URI Parsing
apps/meteor/app/oauth2-server-config/server/admin/functions/parseUriList.ts
Updated inline comment to clarify normalization rationale; no functional behavior change.
Message Parsing Tests
apps/meteor/client/lib/parseMessageTextToAstMarkdown.spec.ts
Added test case verifying parseMessageTextToAstMarkdown returns an empty array when input message is empty.
Documentation Correction
packages/apps-engine/src/definition/messages/IMessageReactionContext.ts
Fixed typo in documentation comment: "recieved" → "received".

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

stat: ready to merge, stat: QA assured

Suggested reviewers

  • KevLehman
  • tassoevan
  • sampaiodiego

Poem

🐰 Four little tweaks across the code,
Callbacks now walk the conditional road,
Empty arrays tested with care,
Typos corrected with flair—
Our warren hops forward with ease! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ 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%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main bug fix (livechat callbacks firing only on updates) and mentions code quality improvements, aligning well with the actual changes made.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Important

Action Needed: IP Allowlist Update

If your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:

  • 136.113.208.247/32 (new)
  • 34.170.211.100/32
  • 35.222.179.152/32

Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist.


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.

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Feb 3, 2026

⚠️ No Changeset found

Latest commit: d8b2924

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

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

No issues found across 4 files

@rushikesh-bobade
Copy link
Copy Markdown
Author

Hi maintainers!

I realize this PR contains multiple unrelated changes (callback fix, typo correction, comment update, and test addition).

As a first-time contributor, I'm still learning the best practices. I'm happy to close this and split into separate PRs if that would be preferred.

Please let me know how you'd like me to proceed. I'll ensure future contributions follow the one-change-per-PR practice.
Thanks!

@rushikesh-bobade
Copy link
Copy Markdown
Author

Hi maintainers, I’m closing this PR because it bundled multiple unrelated changes and is now superseded by smaller, single-concern PRs to make review easier. I’ve split the original changes into focused follow-ups (livechat callback fix, apps-engine typo fix, OAuth comment clarification, and empty-message parsing test). Thanks for the guidance and patience while I learned the preferred contribution workflow.

@rushikesh-bobade rushikesh-bobade deleted the fix/livechat-callback-and-tests branch April 23, 2026 15:29
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.

1 participant