Skip to content

chore(apps): prevent data conversion for events with no listeners#39565

Draft
Copilot wants to merge 5 commits intodevelopfrom
copilot/add-has-listeners-method
Draft

chore(apps): prevent data conversion for events with no listeners#39565
Copilot wants to merge 5 commits intodevelopfrom
copilot/add-has-listeners-method

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 12, 2026

Every triggerEvent call unconditionally converts messages/rooms/users and invokes executeListener() even when zero apps are registered for that event, wasting CPU and I/O on every fire.

Changes

  • AppListenerManager.ts — New public hasListeners(event: AppInterface): boolean method. Returns true if at least one app is registered or the event is blocked (essential-app-disabled case), ensuring EssentialAppDisabledException is still reachable via executeListener.

    public hasListeners(event: AppInterface): boolean {
        if (this.isEventBlocked(event)) {
            return true;
        }
        return (this.listeners.get(event)?.length ?? 0) > 0;
    }
  • listeners.ts — Early-return guard at the top of handleEvent(), before the switch and any converter calls. Returns undefined (the correct no-op sentinel across all event return types) when no listeners are present.

    async handleEvent(args: HandleEvent): Promise<any> {
        const listenerManager = this.orch.getManager().getListenerManager();
        if (!listenerManager.hasListeners(args.event as AppInterface)) {
            return undefined;
        }
        // existing switch ...
    }
  • AppListenerManager.spec.ts — Four new @Test() cases: no listeners → false; after registerListenerstrue; after unregisterListenersfalse; blocked event with no registered app → true.

  • listeners.spec.ts (new) — Mocha/Chai/Sinon unit tests for AppListenerBridge.handleEvent(): verifies the message converter is never called when hasListeners returns false, and is called when it returns true.

Original prompt

Problem

Currently, whenever triggerEvent is called from apps/meteor/ee/server/apps/orchestrator.js, the server unconditionally converts all the data (messages, rooms, users, etc.) and calls executeListener() in the apps-engine — even when no app is listening for the event. This wastes CPU and I/O resources on every event fire.

Goal

Skip all expensive data conversion work in AppListenerBridge.handleEvent() (and its sub-methods) when there are no apps registered for the given event.

Changes Required

1. packages/apps-engine/src/server/managers/AppListenerManager.ts

Add a new public method hasListeners(event: AppInterface): boolean to AppListenerManager that returns true when at least one app ID is registered for the event or the event is currently blocked (essential-app-disabled case), and false otherwise. The blocked check must be included so that the EssentialAppDisabledException is still thrown as expected — if an event is blocked, it still has to reach executeListener() to raise the error.

public hasListeners(event: AppInterface): boolean {
    if (this.isEventBlocked(event)) {
        return true;
    }
    return (this.listeners.get(event)?.length ?? 0) > 0;
}

2. apps/meteor/app/apps/server/bridges/listeners.ts

Modify AppListenerBridge.handleEvent() to call hasListeners() on the AppListenerManager before doing any data conversion (i.e., before dispatching to messageEvent, roomEvent, livechatEvent, userEvent, uploadEvent, or defaultEvent).

The early-return value must respect the caller's expectations:

  • For events where the return type is boolean (prevent events like IPreMessageSentPrevent, IPreRoomCreatePrevent, IPreRoomDeletePrevent, IPreMessageDeletePrevent, IPreMessageUpdatedPrevent): return false (not prevented).
  • For events where the return type is the data/object itself (extend/modify events like IPreMessageSentExtend, IPreMessageSentModify, IPreRoomCreateExtend, IPreRoomCreateModify, IPreMessageUpdatedExtend, IPreMessageUpdatedModify): return undefined (the caller must handle undefined as "no modification").
  • For void / fire-and-forget post-events: return undefined.
  • For IPreFileUpload: return undefined (void).

The cleanest implementation is to add the early-exit at the top of handleEvent, before the switch statement:

async handleEvent(args: HandleEvent): Promise<any> {
    const listenerManager = this.orch.getManager().getListenerManager();
    if (!listenerManager.hasListeners(args.event as AppInterface)) {
        return undefined;
    }
    // existing switch ...
}

Because all callers that care about a specific return value (e.g., "was the message prevented?") already handle undefined as the "nothing happened" sentinel — as evidenced by the existing result ?? undefined patterns in messageEvent and roomEvent — returning undefined is the correct and safe no-op value across all event types.

3. Tests

packages/apps-engine/tests/server/managers/AppListenerManager.spec.ts

Extend the existing test fixture to cover hasListeners():

  • hasListeners returns false for an event with no registered apps and no locked apps.
  • hasListeners returns true after registerListeners adds an app for that event.
  • hasListeners returns false after unregisterListeners removes the only app.
  • hasListeners returns true for an event that is blocked (locked by a disabled essential app), even with zero registered listeners.

Use the existing alsatian test style (the same @Test() + Expect() pattern already present in the file).

apps/meteor/app/apps/server/bridges/listeners.spec.ts (new file)

Create a new unit-test file for AppListenerBridge. Place it alongside the existing listeners.ts at apps/meteor/app/apps/server/bridges/listeners.spec.ts. Use the project's standard Mocha/Chai/Sinon test style (consistent with other Meteor server bridge tests in that package).

Tests to include:

  1. No listeners → skip conversion entirely and return undefined

    • Mock listenerManager.hasListeners to return false.
    • Call handleEvent with a message event (e.g., IPostMessageSent).
    • Assert that the message converter was never called.
    • Assert the return value is undefined.
  2. Has listeners → proceeds to conversion and execution

    • Mock listenerManager.hasListeners to return true.
    • Mock executeListener to return undefined.
    • Call handleEvent with the same event.
    • Assert the message converter was called.
  3. hasListeners is not called for UIKit interactions (sanity check is optional) — UIKit interactions are routed by appId directly, but the early-return guard still applies.

Keep the mocks minimal: use plain objects / stubs; no need for full Rocket.Chat infrastructure.

Files to modify / create

File Action
`packages/apps-engin...

This pull request was created from Copilot chat.


🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

@dionisio-bot
Copy link
Copy Markdown
Contributor

dionisio-bot Bot commented Mar 12, 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

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Mar 12, 2026

⚠️ No Changeset found

Latest commit: 0567303

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

Copilot AI changed the title [WIP] Add hasListeners method to AppListenerManager Skip data conversion in AppListenerBridge when no apps are listening for an event Mar 12, 2026
Comment thread apps/meteor/.mocharc.js Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 12, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.89%. Comparing base (b21787d) to head (94271ae).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #39565      +/-   ##
===========================================
- Coverage    70.94%   70.89%   -0.06%     
===========================================
  Files         3199     3203       +4     
  Lines       113462   113642     +180     
  Branches     20545    20552       +7     
===========================================
+ Hits         80498    80566      +68     
- Misses       30911    31017     +106     
- Partials      2053     2059       +6     
Flag Coverage Δ
unit 71.56% <100.00%> (-0.08%) ⬇️

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.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 12, 2026

@copilot fix the CI issues

https://github.com/RocketChat/Rocket.Chat/actions/runs/23011490001/job/66828940784?pr=39565
...

The two failing jobs (TypeScript check and Code Lint) both failed due to the turbogha cache server timing out (Error: Timed out waiting for: http-get://localhost:41230) — this is an infrastructure issue unrelated to the code changes. Also applied the mocharc.js glob suggestion from the review (198242a).

Copilot AI requested a review from d-gubert March 12, 2026 17:14
@d-gubert
Copy link
Copy Markdown
Member

@copilot fix the lint issue

/home/runner/work/Rocket.Chat/Rocket.Chat/apps/meteor/app/apps/server/bridges/listeners.spec.ts
Error:    2:1  error    There should be no empty line within import group                                           import/order
Error:    4:1  error    `@rocket.chat/apps-engine/definition/metadata` import should occur before import of `chai`  import/order

And fix the test file apps/meteor/app/apps/server/bridges/router.spec.ts - it wasn't included in the test glob previously, so the error went unnoticed

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 12, 2026

@copilot fix the lint issue

...

Fixed in 4a4c581:

  • listeners.spec.ts: reordered imports alphabetically — @rocket.chat/apps-engine before chai/sinon, removed stray blank line
  • router.spec.ts: added import { expect } from 'chai' and converted all expect(...).toEqual(...) to expect(...).to.deep.equal(...)

Copilot AI and others added 4 commits March 12, 2026 17:54
…pListenerBridge.handleEvent()

Co-authored-by: d-gubert <1810309+d-gubert@users.noreply.github.com>
Co-authored-by: d-gubert <1810309+d-gubert@users.noreply.github.com>
…Chai in router.spec.ts

Co-authored-by: d-gubert <1810309+d-gubert@users.noreply.github.com>
@d-gubert d-gubert force-pushed the copilot/add-has-listeners-method branch from 72da7d9 to 94271ae Compare March 12, 2026 20:54
@d-gubert d-gubert changed the title Skip data conversion in AppListenerBridge when no apps are listening for an event chore(apps): prevent data conversion for events with no listeners Mar 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants