fix: business-hour availability not recomputed when agents are removed from a department - #41158
Merged
Merged
Conversation
Contributor
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: fbd687d The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
WalkthroughAdds a Changesets patch note for a business-hours agent availability recomputation fix, reformats the aggregation pipeline in ChangesBusiness Hour Agent Availability Fix
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #41158 +/- ##
=========================================
Coverage 69.29% 69.30%
=========================================
Files 3539 3545 +6
Lines 138832 139290 +458
Branches 24767 24908 +141
=========================================
+ Hits 96210 96537 +327
- Misses 38607 38725 +118
- Partials 4015 4028 +13
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/tests/end-to-end/api/livechat/19-business-hours.ts (1)
738-738: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImplementation comments added.
As per coding guidelines,
**/*.{ts,tsx,js}should "Avoid code comments in the implementation." These explanatory comments about the fire-and-forget timing could be dropped or moved into more descriptive test/variable names.Also applies to: 753-753, 791-791, 815-815
🤖 Prompt for 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. In `@apps/meteor/tests/end-to-end/api/livechat/19-business-hours.ts` at line 738, The livechat business-hours test contains implementation comments that should be removed from the code; the note about the fire-and-forget recomputation timing should be expressed through clearer test/variable names instead of inline comments. Update the affected sections in the 19-business-hours test to drop these explanatory comments while keeping the polling/assertion flow in the same test helpers and steps.Source: Coding guidelines
🤖 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 `@apps/meteor/tests/end-to-end/api/livechat/19-business-hours.ts`:
- Around line 790-824: The test in the business-hours scenario still relies on a
fixed sleep, which can hide failures in the async recomputation path. Replace
the sleep in the test around addOrRemoveAgentFromDepartment/getMe with an
explicit wait helper or polling assertion that watches
ILivechatAgent.openBusinessHours until the expected settled state is observed.
Use the existing test helpers and the latestAgent assertion block to wait for
the transition instead of assuming a 2-second delay is enough.
---
Nitpick comments:
In `@apps/meteor/tests/end-to-end/api/livechat/19-business-hours.ts`:
- Line 738: The livechat business-hours test contains implementation comments
that should be removed from the code; the note about the fire-and-forget
recomputation timing should be expressed through clearer test/variable names
instead of inline comments. Update the affected sections in the
19-business-hours test to drop these explanatory comments while keeping the
polling/assertion flow in the same test helpers and steps.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e005dcd5-1d9b-4f6d-b586-af607665d188
📒 Files selected for processing (2)
.changeset/fix-business-hour-agents-availability.mdapps/meteor/tests/end-to-end/api/livechat/19-business-hours.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/fix-business-hour-agents-availability.md
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: Hacktron Security Check
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/tests/end-to-end/api/livechat/19-business-hours.ts
🧠 Learnings (3)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/tests/end-to-end/api/livechat/19-business-hours.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/tests/end-to-end/api/livechat/19-business-hours.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/tests/end-to-end/api/livechat/19-business-hours.ts
🔇 Additional comments (3)
apps/meteor/tests/end-to-end/api/livechat/19-business-hours.ts (3)
732-769: LGTM!
771-788: LGTM!
826-834: LGTM!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…partment
LivechatDepartment.findAgentsByBusinessHourId passed its pipeline double-wrapped
(aggregate([[...stages]])); MongoDB rejects non-object pipeline elements with
TypeMismatch, so the method threw on every invocation since its introduction.
The only caller runs via callbacks.runAsync('livechat.removeAgentDepartment')
(fire-and-forget), so the failure was silent: removing agents from a department
linked to a business hour (multiple business hours mode) succeeded, but the
removed agents' openBusinessHours were never recomputed, leaving their livechat
availability stale until an unrelated event refreshed it.
…a department Two EE e2e tests for the multiple-business-hours flow: - removing an agent from its only department falls back to the default business hour (fails against the unfixed code: the recomputation callback rejects and, in dev/test mode, crashes the server) - removing an agent from one of two departments sharing the same business hour keeps that business hour open for the agent (exercises the agentIds set returned by the fixed aggregation) Both poll for the result since the recomputation runs on a fire-and-forget callback (callbacks.runAsync).
KevLehman
force-pushed
the
fix/business-hour-agents-availability
branch
from
July 3, 2026 14:06
3cfbb96 to
673657a
Compare
The kept-business-hour assertion equals the pre-removal state, so on its own it cannot distinguish 'correctly kept' from 'callback never ran'. Add a control agent that belongs only to the first department and is removed in the same request: both agents are processed by the same callback invocation, so observing the control agent fall back to the default business hour proves the recomputation ran before asserting the overlapping agent's unchanged state. Also replaces the fixed 2s sleep with the existing polling helper.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/views/admin/settings/SettingsGroupPage/SettingsGroupPage.tsx (1)
92-117: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset
invalidwhen reverting edits.cancel()restoresvalueandchanged, but leavesinvalidbehind, so a code setting can keep showing an error state after reverting to a valid value.🤖 Prompt for 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. In `@apps/meteor/client/views/admin/settings/SettingsGroupPage/SettingsGroupPage.tsx` around lines 92 - 117, The cancel handler in SettingsGroupPage is restoring value and changed, but it is not clearing invalid, so reverted settings can remain stuck in an error state. Update the cancel useStableCallback block to also reset invalid when building the settingsToDispatch entries, and make sure both the isColorSetting branch and the non-color branch in the SettingsGroupPage component preserve or clear all relevant fields consistently before calling dispatchToEditing.
🤖 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 `@apps/meteor/client/hooks/useDeviceLogout.tsx`:
- Around line 27-43: The logout flow in useDeviceLogout should not run
success-only side effects inside onSettled, since it fires for both success and
failure. Move the current success behavior from useMutation’s onSettled into
onSuccess in logoutDevice, and add an onError handler to avoid showing
Device_Logged_Out, invalidating deviceManagementQueryKeys, closing the
contextual bar, or calling logout() when the request fails. Keep
isCurrentSession and the current
queryClient/handleCloseContextualBar/disptachToastMessage behavior scoped to the
success path only.
---
Outside diff comments:
In
`@apps/meteor/client/views/admin/settings/SettingsGroupPage/SettingsGroupPage.tsx`:
- Around line 92-117: The cancel handler in SettingsGroupPage is restoring value
and changed, but it is not clearing invalid, so reverted settings can remain
stuck in an error state. Update the cancel useStableCallback block to also reset
invalid when building the settingsToDispatch entries, and make sure both the
isColorSetting branch and the non-color branch in the SettingsGroupPage
component preserve or clear all relevant fields consistently before calling
dispatchToEditing.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3af9e0a3-b04d-4bba-a454-842ef4b468ca
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (43)
.changeset/all-baths-cry.md.changeset/fix-business-hour-agents-availability.md.changeset/forty-stars-bathe.md.changeset/json-setting-validation-feedback.md.changeset/spotlight-perf.mdapps/meteor/client/hooks/useDeviceLogout.tsxapps/meteor/client/lib/utils/getCodeSettingError.spec.tsapps/meteor/client/lib/utils/getCodeSettingError.tsapps/meteor/client/providers/SettingsProvider.tsxapps/meteor/client/views/account/deviceManagement/DeviceManagementAccountTable/DeviceManagementAccountRow.tsxapps/meteor/client/views/account/deviceManagement/DeviceManagementAccountTable/DeviceManagementAccountTable.tsxapps/meteor/client/views/admin/EditableSettingsContext.tsapps/meteor/client/views/admin/settings/Setting/Setting.tsxapps/meteor/client/views/admin/settings/Setting/inputs/CodeMirror/CodeMirrorBox.tsxapps/meteor/client/views/admin/settings/Setting/inputs/CodeSettingInput.spec.tsxapps/meteor/client/views/admin/settings/Setting/inputs/CodeSettingInput.tsxapps/meteor/client/views/admin/settings/SettingsGroupPage/SettingsGroupPage.tsxapps/meteor/ee/server/api/sessions.tsapps/meteor/ee/server/models/raw/LivechatDepartment.tsapps/meteor/ee/server/settings/ldap.tsapps/meteor/lib/utils/isJSON.spec.tsapps/meteor/lib/utils/isJSON.tsapps/meteor/package.jsonapps/meteor/server/lib/spotlight.jsapps/meteor/server/publications/spotlight.tsapps/meteor/server/settings/ldap.tsapps/meteor/tests/end-to-end/api/livechat/19-business-hours.tsapps/uikit-playground/package.jsonpackages/core-typings/src/ISession.tspackages/fuselage-ui-kit/package.jsonpackages/gazzodown/package.jsonpackages/i18n/src/locales/en.i18n.jsonpackages/model-typings/src/models/ISessionsModel.tspackages/models/src/models/Sessions.tspackages/models/src/models/Subscriptions.tspackages/storybook-config/package.jsonpackages/ui-avatar/package.jsonpackages/ui-client/package.jsonpackages/ui-composer/package.jsonpackages/ui-contexts/package.jsonpackages/ui-video-conf/package.jsonpackages/ui-voip/package.jsonpackages/web-ui-registration/package.json
✅ Files skipped from review due to trivial changes (12)
- apps/uikit-playground/package.json
- packages/ui-composer/package.json
- packages/ui-avatar/package.json
- .changeset/forty-stars-bathe.md
- packages/ui-voip/package.json
- apps/meteor/lib/utils/isJSON.spec.ts
- apps/meteor/client/lib/utils/getCodeSettingError.ts
- packages/ui-contexts/package.json
- .changeset/json-setting-validation-feedback.md
- .changeset/spotlight-perf.md
- .changeset/all-baths-cry.md
- apps/meteor/ee/server/models/raw/LivechatDepartment.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/tests/end-to-end/api/livechat/19-business-hours.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Hacktron Security Check
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/views/account/deviceManagement/DeviceManagementAccountTable/DeviceManagementAccountTable.tsxpackages/model-typings/src/models/ISessionsModel.tsapps/meteor/client/lib/utils/getCodeSettingError.spec.tsapps/meteor/server/settings/ldap.tsapps/meteor/client/views/admin/settings/Setting/inputs/CodeSettingInput.spec.tsxapps/meteor/lib/utils/isJSON.tsapps/meteor/client/views/admin/EditableSettingsContext.tsapps/meteor/client/views/admin/settings/Setting/inputs/CodeSettingInput.tsxapps/meteor/server/publications/spotlight.tspackages/models/src/models/Subscriptions.tsapps/meteor/client/views/admin/settings/SettingsGroupPage/SettingsGroupPage.tsxapps/meteor/ee/server/api/sessions.tspackages/models/src/models/Sessions.tsapps/meteor/client/providers/SettingsProvider.tsxpackages/core-typings/src/ISession.tsapps/meteor/ee/server/settings/ldap.tsapps/meteor/server/lib/spotlight.jsapps/meteor/client/views/account/deviceManagement/DeviceManagementAccountTable/DeviceManagementAccountRow.tsxapps/meteor/client/views/admin/settings/Setting/inputs/CodeMirror/CodeMirrorBox.tsxapps/meteor/client/views/admin/settings/Setting/Setting.tsxapps/meteor/client/hooks/useDeviceLogout.tsx
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/client/lib/utils/getCodeSettingError.spec.ts
🧠 Learnings (12)
📚 Learning: 2026-06-16T14:13:34.463Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 40974
File: packages/web-ui-registration/package.json:31-31
Timestamp: 2026-06-16T14:13:34.463Z
Learning: In Rocket.Chat’s monorepo, when reviewing a dependency entry and flagging that a specific version “does not exist” (e.g., in package.json), first verify the exact package/version directly against the npm registry (use URLs like https://registry.npmjs.org/<package>/<version> or https://www.npmjs.com/package/<package>/v/<version>). Do not rely on web search results for this check, since they may be stale or cached and may not reflect the latest published versions.
Applied to files:
packages/web-ui-registration/package.jsonpackages/fuselage-ui-kit/package.jsonpackages/ui-client/package.jsonpackages/gazzodown/package.jsonpackages/ui-video-conf/package.jsonpackages/storybook-config/package.json
📚 Learning: 2026-06-16T14:13:49.795Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 40974
File: packages/web-ui-registration/package.json:26-26
Timestamp: 2026-06-16T14:13:49.795Z
Learning: During code reviews that check whether a dependency version exists in package.json (especially for Rocket.Chat’s rocket.chat/fuselage and related rocket.chat/fuselage-* packages), don’t rely on web search results. Instead, verify the version directly against the npm registry (e.g., via the npm registry API or the canonical package URL https://www.npmjs.com/package/<package>/v/<version>) before deciding that a version bump is invalid. If the version is present in the npm registry, do not flag it as invalid.
Applied to files:
packages/web-ui-registration/package.jsonpackages/fuselage-ui-kit/package.jsonpackages/ui-client/package.jsonpackages/gazzodown/package.jsonpackages/ui-video-conf/package.jsonpackages/storybook-config/package.json
📚 Learning: 2026-06-16T14:13:59.986Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 40974
File: packages/ui-video-conf/package.json:25-25
Timestamp: 2026-06-16T14:13:59.986Z
Learning: In the Rocket.Chat monorepo, when reviewing a dependency version bump for rocket.chat/fuselage in a package.json, do not flag the new version constraint as “non-existent” or invalid unless you verify the published versions directly from the npm registry (https://www.npmjs.com/package/rocket.chat/fuselage). Don’t rely on search/web results for available versions since they can be stale.
Applied to files:
packages/web-ui-registration/package.jsonpackages/fuselage-ui-kit/package.jsonpackages/ui-client/package.jsonpackages/gazzodown/package.jsonpackages/ui-video-conf/package.jsonpackages/storybook-config/package.json
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/views/account/deviceManagement/DeviceManagementAccountTable/DeviceManagementAccountTable.tsxapps/meteor/client/views/admin/settings/Setting/inputs/CodeSettingInput.spec.tsxapps/meteor/client/views/admin/settings/Setting/inputs/CodeSettingInput.tsxapps/meteor/client/views/admin/settings/SettingsGroupPage/SettingsGroupPage.tsxapps/meteor/client/providers/SettingsProvider.tsxapps/meteor/client/views/account/deviceManagement/DeviceManagementAccountTable/DeviceManagementAccountRow.tsxapps/meteor/client/views/admin/settings/Setting/inputs/CodeMirror/CodeMirrorBox.tsxapps/meteor/client/views/admin/settings/Setting/Setting.tsxapps/meteor/client/hooks/useDeviceLogout.tsx
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/client/views/account/deviceManagement/DeviceManagementAccountTable/DeviceManagementAccountTable.tsxpackages/model-typings/src/models/ISessionsModel.tsapps/meteor/client/lib/utils/getCodeSettingError.spec.tsapps/meteor/server/settings/ldap.tsapps/meteor/client/views/admin/settings/Setting/inputs/CodeSettingInput.spec.tsxapps/meteor/lib/utils/isJSON.tsapps/meteor/client/views/admin/EditableSettingsContext.tsapps/meteor/client/views/admin/settings/Setting/inputs/CodeSettingInput.tsxapps/meteor/server/publications/spotlight.tspackages/models/src/models/Subscriptions.tsapps/meteor/client/views/admin/settings/SettingsGroupPage/SettingsGroupPage.tsxapps/meteor/ee/server/api/sessions.tspackages/models/src/models/Sessions.tsapps/meteor/client/providers/SettingsProvider.tsxpackages/core-typings/src/ISession.tsapps/meteor/ee/server/settings/ldap.tsapps/meteor/client/views/account/deviceManagement/DeviceManagementAccountTable/DeviceManagementAccountRow.tsxapps/meteor/client/views/admin/settings/Setting/inputs/CodeMirror/CodeMirrorBox.tsxapps/meteor/client/views/admin/settings/Setting/Setting.tsxapps/meteor/client/hooks/useDeviceLogout.tsx
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/model-typings/src/models/ISessionsModel.tsapps/meteor/client/lib/utils/getCodeSettingError.spec.tsapps/meteor/server/settings/ldap.tsapps/meteor/lib/utils/isJSON.tsapps/meteor/client/views/admin/EditableSettingsContext.tsapps/meteor/server/publications/spotlight.tspackages/models/src/models/Subscriptions.tsapps/meteor/ee/server/api/sessions.tspackages/models/src/models/Sessions.tspackages/core-typings/src/ISession.tsapps/meteor/ee/server/settings/ldap.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/model-typings/src/models/ISessionsModel.tsapps/meteor/client/lib/utils/getCodeSettingError.spec.tsapps/meteor/server/settings/ldap.tsapps/meteor/lib/utils/isJSON.tsapps/meteor/client/views/admin/EditableSettingsContext.tsapps/meteor/server/publications/spotlight.tspackages/models/src/models/Subscriptions.tsapps/meteor/ee/server/api/sessions.tspackages/models/src/models/Sessions.tspackages/core-typings/src/ISession.tsapps/meteor/ee/server/settings/ldap.ts
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/lib/utils/getCodeSettingError.spec.tsapps/meteor/client/views/admin/EditableSettingsContext.ts
📚 Learning: 2026-05-11T20:30:35.265Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 40480
File: apps/meteor/client/meteor/startup/accounts.ts:59-61
Timestamp: 2026-05-11T20:30:35.265Z
Learning: In Rocket.Chat’s Meteor client code, when calling `dispatchToastMessage` with `{ type: 'error' }`, pass the raw caught error object as `message` without manual normalization. `dispatchToastMessage` is designed to accept `message: unknown` for error toasts, so avoid converting errors to strings (e.g., `String(error)`) or extracting `error.message` before passing them.
Applied to files:
apps/meteor/client/lib/utils/getCodeSettingError.spec.tsapps/meteor/client/views/admin/EditableSettingsContext.ts
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
apps/meteor/client/lib/utils/getCodeSettingError.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/client/lib/utils/getCodeSettingError.spec.tsapps/meteor/client/views/admin/settings/Setting/inputs/CodeSettingInput.spec.tsx
📚 Learning: 2026-03-16T21:50:37.589Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:37.589Z
Learning: For changes related to OpenAPI migrations in Rocket.Chat/OpenAPI, when removing endpoint types and validators from rocket.chat/rest-typings (e.g., UserRegisterParamsPOST, /v1/users.register) document this as a minor changeset (not breaking) per RocketChat/Rocket.Chat-Open-API#150 Rule 7. Note that the endpoint type is re-exposed via a module augmentation .d.ts in the consuming package (e.g., packages/web-ui-registration/src/users-register.d.ts). In reviews, ensure the changeset clearly states: this is a non-breaking change, the major version should not be bumped, and the changeset reflects a minor version bump. Do not treat this as a breaking change during OpenAPI migrations.
Applied to files:
.changeset/fix-business-hour-agents-availability.md
🔇 Additional comments (32)
.changeset/fix-business-hour-agents-availability.md (1)
5-5: LGTM!apps/meteor/server/lib/spotlight.js (1)
42-42: LGTM!Also applies to: 59-70
apps/meteor/server/publications/spotlight.ts (1)
64-70: LGTM!packages/models/src/models/Subscriptions.ts (1)
460-465: LGTM!Projected fields align exactly with what's consumed downstream in the corresponding
$groupstages.Also applies to: 502-503
packages/ui-video-conf/package.json (1)
25-25: LGTM!packages/web-ui-registration/package.json (1)
26-26: LGTM!apps/meteor/package.json (1)
110-110: LGTM!packages/fuselage-ui-kit/package.json (1)
46-49: LGTM!packages/gazzodown/package.json (1)
34-34: LGTM!packages/storybook-config/package.json (1)
36-36: LGTM!packages/ui-client/package.json (1)
29-29: LGTM!apps/meteor/ee/server/api/sessions.ts (1)
98-105: 🎯 Functional CorrectnessSee related concern in
packages/models/src/models/Sessions.ts.This is the source of the
currentLoginTokenvalue used in the$eqcomparison flagged inSessions.ts(aggregateSessionsByUserId). Please confirmthis.tokenhere matches the format ofSessions.loginTokenbefore relying on this for the "current" session flag.packages/core-typings/src/ISession.ts (1)
78-80: LGTM!packages/model-typings/src/models/ISessionsModel.ts (1)
40-54: LGTM!apps/meteor/client/hooks/useDeviceLogout.tsx (2)
9-21: LGTM!Also applies to: 45-49
22-22: 🎯 Functional CorrectnessNo issue: both logout endpoints share the same
SessionsPropspayload, so the union path doesn’t affectlogoutDevice({ sessionId }).apps/meteor/client/views/account/deviceManagement/DeviceManagementAccountTable/DeviceManagementAccountRow.tsx (1)
16-36: LGTM!apps/meteor/client/views/account/deviceManagement/DeviceManagementAccountTable/DeviceManagementAccountTable.tsx (1)
69-69: LGTM!packages/i18n/src/locales/en.i18n.json (1)
7285-7286: LGTM!packages/models/src/models/Sessions.ts (1)
745-758: 🎯 Functional CorrectnessNo raw-vs-hashed mismatch here
this.tokenis the hashed auth token, and the session/logout flow uses the same token format, so thecurrentflag won’t be stuck false for that reason.> Likely an incorrect or invalid review comment.apps/meteor/lib/utils/isJSON.ts (1)
1-8: LGTM!apps/meteor/client/views/admin/settings/Setting/inputs/CodeSettingInput.tsx (1)
2-4: LGTM!Also applies to: 32-34, 48-48
apps/meteor/client/views/admin/settings/Setting/inputs/CodeMirror/CodeMirrorBox.tsx (1)
1-13: LGTM! Manualid/aria-describedby/role="alert"wiring is appropriate here sinceFieldErroris imported from plain@rocket.chat/fuselage, notfuselage-formswrappers, which don't auto-wire these attributes.Based on learnings, "This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs."
Also applies to: 32-48, 63-79
Source: Learnings
apps/meteor/client/views/admin/settings/Setting/inputs/CodeSettingInput.spec.tsx (2)
1-41: LGTM! Good coverage for the invalid/valid JSON and accessibility association cases.
12-22: 🎯 Functional CorrectnessNo change needed for
basePropsautocomplete,required, andonResetButtonClickare optional inSettingInputProps, so omitting them here does not affect type-checking.> Likely an incorrect or invalid review comment.apps/meteor/ee/server/settings/ldap.ts (1)
138-146: LGTM!code: 'application/json'additions are consistent with the existing pattern used byLDAP_Sync_User_Data_RolesMapandLDAP_Sync_User_Data_ChannelsMap.Also applies to: 259-266, 303-312
apps/meteor/server/settings/ldap.ts (1)
229-235: LGTM!apps/meteor/client/lib/utils/getCodeSettingError.spec.ts (1)
1-30: LGTM!apps/meteor/client/providers/SettingsProvider.tsx (1)
2-2: LGTM!Also applies to: 15-15, 119-130
apps/meteor/client/views/admin/EditableSettingsContext.ts (1)
7-12: LGTM!apps/meteor/client/views/admin/settings/Setting/Setting.tsx (1)
2-2: LGTM!Also applies to: 12-12, 40-41, 56-61, 81-86
apps/meteor/client/views/admin/settings/SettingsGroupPage/SettingsGroupPage.tsx (1)
49-51: LGTM!Also applies to: 79-81, 168-168
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/views/admin/settings/SettingsGroupPage/SettingsGroupPage.tsx (1)
92-117: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset
invalidwhen reverting edits.cancel()restoresvalueandchanged, but leavesinvalidbehind, so a code setting can keep showing an error state after reverting to a valid value.🤖 Prompt for 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. In `@apps/meteor/client/views/admin/settings/SettingsGroupPage/SettingsGroupPage.tsx` around lines 92 - 117, The cancel handler in SettingsGroupPage is restoring value and changed, but it is not clearing invalid, so reverted settings can remain stuck in an error state. Update the cancel useStableCallback block to also reset invalid when building the settingsToDispatch entries, and make sure both the isColorSetting branch and the non-color branch in the SettingsGroupPage component preserve or clear all relevant fields consistently before calling dispatchToEditing.
🤖 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 `@apps/meteor/client/hooks/useDeviceLogout.tsx`:
- Around line 27-43: The logout flow in useDeviceLogout should not run
success-only side effects inside onSettled, since it fires for both success and
failure. Move the current success behavior from useMutation’s onSettled into
onSuccess in logoutDevice, and add an onError handler to avoid showing
Device_Logged_Out, invalidating deviceManagementQueryKeys, closing the
contextual bar, or calling logout() when the request fails. Keep
isCurrentSession and the current
queryClient/handleCloseContextualBar/disptachToastMessage behavior scoped to the
success path only.
---
Outside diff comments:
In
`@apps/meteor/client/views/admin/settings/SettingsGroupPage/SettingsGroupPage.tsx`:
- Around line 92-117: The cancel handler in SettingsGroupPage is restoring value
and changed, but it is not clearing invalid, so reverted settings can remain
stuck in an error state. Update the cancel useStableCallback block to also reset
invalid when building the settingsToDispatch entries, and make sure both the
isColorSetting branch and the non-color branch in the SettingsGroupPage
component preserve or clear all relevant fields consistently before calling
dispatchToEditing.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3af9e0a3-b04d-4bba-a454-842ef4b468ca
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (43)
.changeset/all-baths-cry.md.changeset/fix-business-hour-agents-availability.md.changeset/forty-stars-bathe.md.changeset/json-setting-validation-feedback.md.changeset/spotlight-perf.mdapps/meteor/client/hooks/useDeviceLogout.tsxapps/meteor/client/lib/utils/getCodeSettingError.spec.tsapps/meteor/client/lib/utils/getCodeSettingError.tsapps/meteor/client/providers/SettingsProvider.tsxapps/meteor/client/views/account/deviceManagement/DeviceManagementAccountTable/DeviceManagementAccountRow.tsxapps/meteor/client/views/account/deviceManagement/DeviceManagementAccountTable/DeviceManagementAccountTable.tsxapps/meteor/client/views/admin/EditableSettingsContext.tsapps/meteor/client/views/admin/settings/Setting/Setting.tsxapps/meteor/client/views/admin/settings/Setting/inputs/CodeMirror/CodeMirrorBox.tsxapps/meteor/client/views/admin/settings/Setting/inputs/CodeSettingInput.spec.tsxapps/meteor/client/views/admin/settings/Setting/inputs/CodeSettingInput.tsxapps/meteor/client/views/admin/settings/SettingsGroupPage/SettingsGroupPage.tsxapps/meteor/ee/server/api/sessions.tsapps/meteor/ee/server/models/raw/LivechatDepartment.tsapps/meteor/ee/server/settings/ldap.tsapps/meteor/lib/utils/isJSON.spec.tsapps/meteor/lib/utils/isJSON.tsapps/meteor/package.jsonapps/meteor/server/lib/spotlight.jsapps/meteor/server/publications/spotlight.tsapps/meteor/server/settings/ldap.tsapps/meteor/tests/end-to-end/api/livechat/19-business-hours.tsapps/uikit-playground/package.jsonpackages/core-typings/src/ISession.tspackages/fuselage-ui-kit/package.jsonpackages/gazzodown/package.jsonpackages/i18n/src/locales/en.i18n.jsonpackages/model-typings/src/models/ISessionsModel.tspackages/models/src/models/Sessions.tspackages/models/src/models/Subscriptions.tspackages/storybook-config/package.jsonpackages/ui-avatar/package.jsonpackages/ui-client/package.jsonpackages/ui-composer/package.jsonpackages/ui-contexts/package.jsonpackages/ui-video-conf/package.jsonpackages/ui-voip/package.jsonpackages/web-ui-registration/package.json
✅ Files skipped from review due to trivial changes (12)
- apps/uikit-playground/package.json
- packages/ui-composer/package.json
- packages/ui-avatar/package.json
- .changeset/forty-stars-bathe.md
- packages/ui-voip/package.json
- apps/meteor/lib/utils/isJSON.spec.ts
- apps/meteor/client/lib/utils/getCodeSettingError.ts
- packages/ui-contexts/package.json
- .changeset/json-setting-validation-feedback.md
- .changeset/spotlight-perf.md
- .changeset/all-baths-cry.md
- apps/meteor/ee/server/models/raw/LivechatDepartment.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/tests/end-to-end/api/livechat/19-business-hours.ts
📜 Review details
🔇 Additional comments (32)
.changeset/fix-business-hour-agents-availability.md (1)
5-5: LGTM!apps/meteor/server/lib/spotlight.js (1)
42-42: LGTM!Also applies to: 59-70
apps/meteor/server/publications/spotlight.ts (1)
64-70: LGTM!packages/models/src/models/Subscriptions.ts (1)
460-465: LGTM!Projected fields align exactly with what's consumed downstream in the corresponding
$groupstages.Also applies to: 502-503
packages/ui-video-conf/package.json (1)
25-25: LGTM!packages/web-ui-registration/package.json (1)
26-26: LGTM!apps/meteor/package.json (1)
110-110: LGTM!packages/fuselage-ui-kit/package.json (1)
46-49: LGTM!packages/gazzodown/package.json (1)
34-34: LGTM!packages/storybook-config/package.json (1)
36-36: LGTM!packages/ui-client/package.json (1)
29-29: LGTM!apps/meteor/ee/server/api/sessions.ts (1)
98-105: 🎯 Functional CorrectnessSee related concern in
packages/models/src/models/Sessions.ts.This is the source of the
currentLoginTokenvalue used in the$eqcomparison flagged inSessions.ts(aggregateSessionsByUserId). Please confirmthis.tokenhere matches the format ofSessions.loginTokenbefore relying on this for the "current" session flag.packages/core-typings/src/ISession.ts (1)
78-80: LGTM!packages/model-typings/src/models/ISessionsModel.ts (1)
40-54: LGTM!apps/meteor/client/hooks/useDeviceLogout.tsx (2)
9-21: LGTM!Also applies to: 45-49
22-22: 🎯 Functional CorrectnessNo issue: both logout endpoints share the same
SessionsPropspayload, so the union path doesn’t affectlogoutDevice({ sessionId }).apps/meteor/client/views/account/deviceManagement/DeviceManagementAccountTable/DeviceManagementAccountRow.tsx (1)
16-36: LGTM!apps/meteor/client/views/account/deviceManagement/DeviceManagementAccountTable/DeviceManagementAccountTable.tsx (1)
69-69: LGTM!packages/i18n/src/locales/en.i18n.json (1)
7285-7286: LGTM!packages/models/src/models/Sessions.ts (1)
745-758: 🎯 Functional CorrectnessNo raw-vs-hashed mismatch here
this.tokenis the hashed auth token, and the session/logout flow uses the same token format, so thecurrentflag won’t be stuck false for that reason.> Likely an incorrect or invalid review comment.apps/meteor/lib/utils/isJSON.ts (1)
1-8: LGTM!apps/meteor/client/views/admin/settings/Setting/inputs/CodeSettingInput.tsx (1)
2-4: LGTM!Also applies to: 32-34, 48-48
apps/meteor/client/views/admin/settings/Setting/inputs/CodeMirror/CodeMirrorBox.tsx (1)
1-13: LGTM! Manualid/aria-describedby/role="alert"wiring is appropriate here sinceFieldErroris imported from plain@rocket.chat/fuselage, notfuselage-formswrappers, which don't auto-wire these attributes.Based on learnings, "This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs."
Also applies to: 32-48, 63-79
Source: Learnings
apps/meteor/client/views/admin/settings/Setting/inputs/CodeSettingInput.spec.tsx (2)
1-41: LGTM! Good coverage for the invalid/valid JSON and accessibility association cases.
12-22: 🎯 Functional CorrectnessNo change needed for
basePropsautocomplete,required, andonResetButtonClickare optional inSettingInputProps, so omitting them here does not affect type-checking.> Likely an incorrect or invalid review comment.apps/meteor/ee/server/settings/ldap.ts (1)
138-146: LGTM!code: 'application/json'additions are consistent with the existing pattern used byLDAP_Sync_User_Data_RolesMapandLDAP_Sync_User_Data_ChannelsMap.Also applies to: 259-266, 303-312
apps/meteor/server/settings/ldap.ts (1)
229-235: LGTM!apps/meteor/client/lib/utils/getCodeSettingError.spec.ts (1)
1-30: LGTM!apps/meteor/client/providers/SettingsProvider.tsx (1)
2-2: LGTM!Also applies to: 15-15, 119-130
apps/meteor/client/views/admin/EditableSettingsContext.ts (1)
7-12: LGTM!apps/meteor/client/views/admin/settings/Setting/Setting.tsx (1)
2-2: LGTM!Also applies to: 12-12, 40-41, 56-61, 81-86
apps/meteor/client/views/admin/settings/SettingsGroupPage/SettingsGroupPage.tsx (1)
49-51: LGTM!Also applies to: 79-81, 168-168
🛑 Comments failed to post (1)
apps/meteor/client/hooks/useDeviceLogout.tsx (1)
27-43: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Success-path side effects run even when the logout request fails.
onSettledexecutes on both success and failure, but this block contains only "success" behavior (toast, cache invalidation, or forcing a client-sidelogout()) with noonErrorhandler andthrowOnError: false. A failed API call will still show "Device Logged Out", invalidate queries, and even log the current user out client-side, while giving no error feedback at all.🛠️ Proposed fix: split success/error handling
const { mutate: logoutDevice } = useMutation({ mutationFn: logoutEndpoint, - onSettled: () => { - if (isCurrentSession) { - setModal(null); - logout(); - } else { - queryClient.invalidateQueries({ queryKey: deviceManagementQueryKeys.all }); - if (isContextualBarOpen) { - handleCloseContextualBar(); - } - dispatchToastMessage({ type: 'success', message: t('Device_Logged_Out') }); - setModal(null); - } - }, + onSuccess: () => { + if (isCurrentSession) { + logout(); + return; + } + queryClient.invalidateQueries({ queryKey: deviceManagementQueryKeys.all }); + if (isContextualBarOpen) { + handleCloseContextualBar(); + } + dispatchToastMessage({ type: 'success', message: t('Device_Logged_Out') }); + }, + onError: (error) => { + dispatchToastMessage({ type: 'error', message: error }); + }, + onSettled: () => { + setModal(null); + }, throwOnError: false, });🤖 Prompt for 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. In `@apps/meteor/client/hooks/useDeviceLogout.tsx` around lines 27 - 43, The logout flow in useDeviceLogout should not run success-only side effects inside onSettled, since it fires for both success and failure. Move the current success behavior from useMutation’s onSettled into onSuccess in logoutDevice, and add an onError handler to avoid showing Device_Logged_Out, invalidating deviceManagementQueryKeys, closing the contextual bar, or calling logout() when the request fails. Keep isCurrentSession and the current queryClient/handleCloseContextualBar/disptachToastMessage behavior scoped to the success path only.
Member
Author
|
/jira CORE |
KevLehman
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
ricardogarim
requested changes
Jul 3, 2026
KevLehman
commented
Jul 3, 2026
ricardogarim
approved these changes
Jul 3, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 3, 2026
dionisio-bot
Bot
removed
the
stat: ready to merge
dionisio-bot
Bot
added
the
stat: ready to merge
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 4, 2026
This was referenced Jul 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
Related to the aggregation audit in #41157 (independent — can merge in any order; rebased onto develop).
LivechatDepartment.findAgentsByBusinessHourId(EE) passes its pipeline double-wrapped —aggregate([[...stages]])— and MongoDB rejects non-object pipeline elements (TypeMismatch: Each element of the 'pipeline' array must be an object, reproduced against a live server). The method has thrown on every invocation since it was introduced in #31468 (2024-01-19).Its only call path is fire-and-forget:
updateDepartmentAgents→callbacks.runAsync('livechat.removeAgentDepartment')→MultipleBusinessHoursBehavior.onRemoveAgentFromDepartment→handleRemoveAgentsFromDepartments.runAsyncschedules callbacks viasetTimeoutwith a discarded promise, so the rejection surfaces as an unhandled promise rejection. PerRocketChat.ErrorHandler.ts, the impact depends on the environment:EXIT_UNHANDLEDPROMISEREJECTIONset:process.exit(1)— removing an agent from a business-hour-linked department crashes the server (reproduced locally).openBusinessHoursis never recomputed, leaving their livechat availability stale until an unrelated event refreshes it.This PR unwraps the pipeline so the recomputation actually executes, and adds two EE e2e tests:
agentIdsset the fixed aggregation returns. A control agent (only in the first department, removed in the same request) must observably fall back to the default business hour first, proving the recomputation ran — the kept-state assertion equals the pre-removal state, so it isn't meaningful on its own.Both tests poll for the result since the recomputation is asynchronous.
Issue(s)
Steps to test or reproduce
TypeMismatchrejection (dev/test: server exits; prod: silently skipped bookkeeping, agent keeps staleopenBusinessHours). After: the agent's business hour closes or falls back to the default business hour.Or:
IS_EE=true yarn testapi:livechat --grep 'On agent removed from department'Further comments
Requires EE + multiple business hours + a department linked to a business hour — narrow enough that the failure survived 2.5 years.
Summary by CodeRabbit
Bug Fixes
Tests
Task: CORE-2378