fix: filter versionUpdate banners where version <= current installed#38932
Merged
fix: filter versionUpdate banners where version <= current installed#38932
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 7895954 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 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:
WalkthroughAdds semver-based filtering and bulk cleanup of outdated or invalid "versionUpdate-*" banners, applies filtering in getUserInfo, updates buildVersionUpdateMessage to accept structured version objects and sort them descending, adds bulk banner update model methods, tests, and a changeset for a patch bump. Changes
Sequence Diagram(s)sequenceDiagram
actor VersionService as "Version Checker"
participant Info as "Info (current version)"
participant Users as "Users Model"
participant Notifier as "sendMessagesToAdmins / Notifier"
rect rgba(200,200,255,0.5)
VersionService->>Info: read current installed version
VersionService->>Users: fetch admin users with banners
Users-->>VersionService: admin users + banners
VersionService->>Users: remove outdated/invalid versionUpdate-* banners (bulk)
end
rect rgba(200,255,200,0.5)
VersionService->>VersionService: sort available versions (semver desc)
VersionService->>Notifier: send messages for highest new version
Notifier-->>Users: deliver banner updates to admins
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
ricardogarim
force-pushed
the
fix/versionupdate-banner
branch
from
b359052 to
6c20de6
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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 (4)
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts (1)
87-104: Consider adding a test for theadmin.banners = undefinedguard pathThe implementation has an explicit early-exit (
if (!admin.banners) { continue; }) for admins returned from the query with no banners field, but there is no test covering this branch. Adding a case where the admin object has nobannersproperty would provide complete coverage of the guard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts` around lines 87 - 104, Add a new spec in the describe('cleanupOutdatedVersionUpdateBanners') block that returns an admin object without a banners property from mockFindUsersInRolesWithQuery and calls buildVersionUpdateMessage([]); assert that mockRemoveBannerById was not called, verifying the early-exit guard (the code path in buildVersionUpdateMessage that checks if (!admin.banners) { continue; }). Use the same test pattern as the existing cases (mockFindUsersInRolesWithQuery.mockReturnValue(createAsyncIterableFromArray([admin])) and await buildVersionUpdateMessage([])) and expect(mockRemoveBannerById).not.toHaveBeenCalled().apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts (2)
19-19:for awaitover a synchronousObject.keysarray is unnecessary
Object.keysreturns a plainstring[]— a synchronous iterable.for await...ofworks here because it wraps each value inPromise.resolve, but it misleads readers into thinking the keys are an async source. Theawaitneeded forremoveBannerByIdbelongs on the call inside the loop body, not on the iteration itself.♻️ Proposed fix
- for await (const bannerId of Object.keys(admin.banners)) { + for (const bannerId of Object.keys(admin.banners)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts` at line 19, In buildVersionUpdateMessage, the loop uses "for await (const bannerId of Object.keys(admin.banners))" which is unnecessary because Object.keys returns a synchronous string[]; change the loop to a regular "for (const bannerId of Object.keys(admin.banners))" and ensure the async call removeBannerById(bannerId) inside the loop is awaited (await removeBannerById(...)) so the removal still happens sequentially and any errors propagate correctly; reference admin.banners, the loop over bannerId, and removeBannerById when making this change.
49-49:semver.rcomparethrowsTypeErrorfor any non-semver string inversions, aborting the entire function
new SemVer(...)insidercomparethrowsTypeError: Invalid Versionfor malformed strings, and that exception propagates out ofArray.sort, preventing any banner creation or cleanup for that run. Theversionsarray is API-sourced so this is low-risk in practice, but pre-filtering withsemver.validbefore sorting is cheap insurance.🛡️ Proposed defensive filter before sort
- const sortedVersions = [...versions].sort((a, b) => semver.rcompare(a.version, b.version)); + const sortedVersions = [...versions] + .filter((v) => semver.valid(v.version)) + .sort((a, b) => semver.rcompare(a.version, b.version));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts` at line 49, The sort can throw when semver.rcompare encounters invalid version strings; before calling semver.rcompare in the creation of sortedVersions, pre-filter the versions array by checking semver.valid (e.g. keep entries where semver.valid(v.version) is truthy) so only valid semver strings are passed into the sort, and optionally log or drop invalid entries; update the code that builds sortedVersions (the variable named sortedVersions and the source array versions) to perform this defensive filter prior to sorting with semver.rcompare.apps/meteor/app/api/server/helpers/getUserInfo.spec.ts (1)
212-238: Add an edge-case test for a banner whose version exactly equals the current installed versionThe
semver.lteboundary includes equality — a banner for version7.5.0whenInfo.versionis7.5.0should be filtered out. This "equal" case isn't covered and is the most interesting boundary given the PR's stated semantics ("version ≤ current installed").➕ Suggested additional test
+ it('should filter out versionUpdate banners for the exact current installed version', async () => { + user.banners = { + 'versionUpdate-7_5_0': { id: 'versionUpdate-7_5_0', priority: 10, title: 'Update', text: 'New version', modifiers: [], link: '', read: false }, + }; + const userInfo = await getUserInfo(user); + expect(userInfo.banners).toEqual({}); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/helpers/getUserInfo.spec.ts` around lines 212 - 238, Add a test case to cover the equality boundary for semver.lte: in the describe block for "version update banner filtering" (where mockInfoVersion is set to '7.5.0') create a new it() that sets user.banners to include a 'versionUpdate-7_5_0' banner, call getUserInfo(user), and assert that the returned userInfo.banners does not contain that banner (expect toEqual({}) or to notHaveProperty('versionUpdate-7_5_0')). This ensures the equality case (banner version === Info.version) is filtered out as intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts`:
- Around line 11-30: cleanupOutdatedVersionUpdateBanners currently only removes
banners when semver.valid(version) is true, so malformed version banner IDs
accumulate; update cleanupOutdatedVersionUpdateBanners to also remove banners
where semver.valid(version) is false (i.e., when bannerId
startsWith('versionUpdate-') but parsed version is invalid) by calling
Users.removeBannerById(admin._id, bannerId) for those cases (and optionally log
the removal), keeping the same bannerId parsing logic used now and aligning
behavior with filterOutdatedVersionUpdateBanners in getUserInfo.ts.
---
Nitpick comments:
In `@apps/meteor/app/api/server/helpers/getUserInfo.spec.ts`:
- Around line 212-238: Add a test case to cover the equality boundary for
semver.lte: in the describe block for "version update banner filtering" (where
mockInfoVersion is set to '7.5.0') create a new it() that sets user.banners to
include a 'versionUpdate-7_5_0' banner, call getUserInfo(user), and assert that
the returned userInfo.banners does not contain that banner (expect toEqual({})
or to notHaveProperty('versionUpdate-7_5_0')). This ensures the equality case
(banner version === Info.version) is filtered out as intended.
In
`@apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts`:
- Around line 87-104: Add a new spec in the
describe('cleanupOutdatedVersionUpdateBanners') block that returns an admin
object without a banners property from mockFindUsersInRolesWithQuery and calls
buildVersionUpdateMessage([]); assert that mockRemoveBannerById was not called,
verifying the early-exit guard (the code path in buildVersionUpdateMessage that
checks if (!admin.banners) { continue; }). Use the same test pattern as the
existing cases
(mockFindUsersInRolesWithQuery.mockReturnValue(createAsyncIterableFromArray([admin]))
and await buildVersionUpdateMessage([])) and
expect(mockRemoveBannerById).not.toHaveBeenCalled().
In `@apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts`:
- Line 19: In buildVersionUpdateMessage, the loop uses "for await (const
bannerId of Object.keys(admin.banners))" which is unnecessary because
Object.keys returns a synchronous string[]; change the loop to a regular "for
(const bannerId of Object.keys(admin.banners))" and ensure the async call
removeBannerById(bannerId) inside the loop is awaited (await
removeBannerById(...)) so the removal still happens sequentially and any errors
propagate correctly; reference admin.banners, the loop over bannerId, and
removeBannerById when making this change.
- Line 49: The sort can throw when semver.rcompare encounters invalid version
strings; before calling semver.rcompare in the creation of sortedVersions,
pre-filter the versions array by checking semver.valid (e.g. keep entries where
semver.valid(v.version) is truthy) so only valid semver strings are passed into
the sort, and optionally log or drop invalid entries; update the code that
builds sortedVersions (the variable named sortedVersions and the source array
versions) to perform this defensive filter prior to sorting with
semver.rcompare.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/olive-hairs-report.mdapps/meteor/app/api/server/helpers/getUserInfo.spec.tsapps/meteor/app/api/server/helpers/getUserInfo.tsapps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.tsapps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.tsapps/meteor/jest.config.ts
📜 Review details
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 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/app/version-check/server/functions/buildVersionUpdateMessage.spec.tsapps/meteor/app/api/server/helpers/getUserInfo.spec.tsapps/meteor/jest.config.tsapps/meteor/app/api/server/helpers/getUserInfo.tsapps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts
**/*.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/app/version-check/server/functions/buildVersionUpdateMessage.spec.tsapps/meteor/app/api/server/helpers/getUserInfo.spec.ts
🧠 Learnings (16)
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.tsapps/meteor/app/api/server/helpers/getUserInfo.spec.tsapps/meteor/jest.config.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.tsapps/meteor/jest.config.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.tsapps/meteor/jest.config.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.tsapps/meteor/jest.config.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.tsapps/meteor/jest.config.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.tsapps/meteor/jest.config.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.tsapps/meteor/jest.config.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.tsapps/meteor/app/api/server/helpers/getUserInfo.tsapps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/jest.config.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.spec.ts : Use `.spec.ts` extension for test files (e.g., `login.spec.ts`)
Applied to files:
apps/meteor/jest.config.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/jest.config.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.{ts,tsx,js} : Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Applied to files:
apps/meteor/jest.config.ts
📚 Learning: 2025-11-05T20:53:57.761Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts
🧬 Code graph analysis (1)
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts (1)
packages/models/src/index.ts (1)
Users(192-192)
🔇 Additional comments (3)
apps/meteor/jest.config.ts (1)
51-51: LGTM — newtestMatchentry is consistent with existing server patterns and correctly scopes the new spec file..changeset/olive-hairs-report.md (1)
1-5: LGTM — correct package target (@rocket.chat/meteor) and changeset type (patch) for this bug fix.apps/meteor/app/api/server/helpers/getUserInfo.ts (1)
30-49:filterOutdatedVersionUpdateBannersimplementation looks correct.The short-circuit
!semver.valid(version)guard at line 42 ensuressemver.lteis never called with an invalid string, so no throw risk here.
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts
Show resolved
Hide resolved
ricardogarim
force-pushed
the
fix/versionupdate-banner
branch
from
d45a429 to
aac1e55
Compare
ricardogarim
force-pushed
the
fix/versionupdate-banner
branch
from
d541436 to
e6920f7
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38932 +/- ##
===========================================
+ Coverage 70.66% 70.68% +0.02%
===========================================
Files 3189 3191 +2
Lines 112717 112863 +146
Branches 20459 20443 -16
===========================================
+ Hits 79655 79781 +126
- Misses 31018 31037 +19
- Partials 2044 2045 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
abhinavkrin
requested changes
Feb 24, 2026
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts
Show resolved
Hide resolved
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts
Outdated
Show resolved
Hide resolved
KevLehman
requested changes
Feb 24, 2026
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts
Outdated
Show resolved
Hide resolved
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts
Show resolved
Hide resolved
Member
|
By incredible reasons, this would fix #18922 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts`:
- Around line 78-80: The afterAll teardown currently restores TEST_MODE by
assigning process.env.TEST_MODE = originalTestMode which will write the string
"undefined" if originalTestMode was undefined; change the restore logic in the
afterAll block to check originalTestMode: if it's undefined use delete
process.env.TEST_MODE, otherwise set process.env.TEST_MODE = originalTestMode,
ensuring the process.env.TEST_MODE key is removed rather than set to the string
"undefined".
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts
📜 Review details
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 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/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts
**/*.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/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.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/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts
🧬 Code graph analysis (1)
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts (2)
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts (1)
buildVersionUpdateMessage(32-98)apps/meteor/server/lib/sendMessagesToAdmins.ts (1)
sendMessagesToAdmins(29-84)
🔇 Additional comments (2)
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts (2)
88-115: Cleanup test coverage is solid for outdated/newer/invalid banner IDs.These cases directly validate the downgrade-filtering behavior targeted by CORE-1084.
117-137: Version-ordering assertion correctly verifies highest-version banner selection.The
toHaveBeenCalledTimes(1)+ expectedversionUpdate-8_0_0pairing is a good guard for the single-banner flow.
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts (1)
62-62:⚠️ Potential issue | 🟠 MajorPre-filter invalid semver strings before
semver.rcompareto preventTypeError.
semver.rcomparethrows aTypeErrorwhen either argument is not a valid semver string. Sinceversionsoriginates from an external cloud API response, malformed entries are plausible and would crash the entire version-check job before any banner logic executes.🔧 Proposed fix
- const sortedVersions = [...versions].sort((a, b) => semver.rcompare(a.version, b.version)); + const sortedVersions = versions + .filter(({ version }) => Boolean(semver.valid(version))) + .sort((a, b) => semver.rcompare(a.version, b.version));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts` at line 62, Pre-filter the incoming versions array for valid semver strings before calling semver.rcompare to avoid TypeError: use semver.valid to filter entries (e.g., keep only items where semver.valid(item.version) is truthy) and then sort that filtered list; update the code around sortedVersions and versions in buildVersionUpdateMessage to perform the filter first and optionally emit a warning/log for any discarded malformed entries so invalid cloud API responses don't crash the job.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts`:
- Line 62: Pre-filter the incoming versions array for valid semver strings
before calling semver.rcompare to avoid TypeError: use semver.valid to filter
entries (e.g., keep only items where semver.valid(item.version) is truthy) and
then sort that filtered list; update the code around sortedVersions and versions
in buildVersionUpdateMessage to perform the filter first and optionally emit a
warning/log for any discarded malformed entries so invalid cloud API responses
don't crash the job.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.tsapps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.tspackages/model-typings/src/models/IUsersModel.tspackages/models/src/models/Users.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/model-typings/src/models/IUsersModel.ts
- apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.spec.ts
- packages/models/src/models/Users.ts
📜 Review details
🧰 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/app/version-check/server/functions/buildVersionUpdateMessage.ts
🧠 Learnings (9)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts
📚 Learning: 2026-02-23T17:53:18.785Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts
📚 Learning: 2025-11-05T20:53:57.761Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.
Applied to files:
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts
🧬 Code graph analysis (1)
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts (2)
packages/models/src/index.ts (1)
Users(192-192)packages/core-typings/src/IUser.ts (1)
IUser(169-239)
🔇 Additional comments (3)
apps/meteor/app/version-check/server/functions/buildVersionUpdateMessage.ts (3)
1-2: LGTM!Imports are clean and both
IUser(used for typing inupdatesarray) andUsers(used for DB queries and bulk updates) are consumed in the new code.
12-43: LGTM!The cleanup logic correctly removes both invalid-semver and outdated (
<= Info.version) banners, consistent with the API-side filtering ingetUserInfo.ts. The short-circuit on!semver.valid(version)preventssemver.ltefrom being called with an invalid left operand. Batching updates viasetBannersInBulkand skipping users with no banner changes is efficient.
66-110: LGTM!The descending-sort +
breakpattern correctly ensures only the single highest applicable version produces a banner and notification. The skip guards for prereleases and versions ≤lastCheckedVersion/Info.versionare sound.
ricardogarim
requested review from
KevLehman,
abhinavkrin and
nazabucciarelli
abhinavkrin
previously approved these changes
Feb 25, 2026
KevLehman
reviewed
Feb 25, 2026
ricardogarim
force-pushed
the
fix/versionupdate-banner
branch
from
1aef834 to
7dc92f0
Compare
KevLehman
previously approved these changes
Feb 25, 2026
ricardogarim
force-pushed
the
fix/versionupdate-banner
branch
from
f8ec332 to
7895954
Compare
KevLehman
approved these changes
Feb 25, 2026
tassoevan
approved these changes
Feb 25, 2026
ricardogarim
added
the
stat: QA assured
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
ricardogarim
added
stat: QA assured
Closed
Open
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
6 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.
This PR fixes a bug where servers display outdated version update banners after upgrading. For example, a server on version 7.5 was showing a banner prompting to update to version 6.2.0.
Old
versionUpdatebanners persist in admin user documents (user.banners) after server upgrades. When the server was on 6.1.x, a banner for 6.2.0 was created. After upgrading to 7.5, the old banner remained in the database and continued to be displayed if not read.Proposed changes (including videos or screenshots)
1. Filter outdated banners in
getUserInfo.tsfilterOutdatedVersionUpdateBanners()function that filters out version banners where version ≤ current installed version/meendpoint (primary way client gets banners)2. Cleanup outdated banners in
buildVersionUpdateMessage.tscleanupOutdatedVersionUpdateBanners()function that removes outdated banners from admin users in the databasewatch.usersstream case where banners are sent directly from DB3. Sort versions descending before processing
breakstatement, only the highest version banner is createdIssue(s)
Steps to test or reproduce
To test the filter:
bannersfield/api/v1/meendpointTo test the cleanup:
Further comments
getUserInfoprovides immediate protection for the/meendpointbuildVersionUpdateMessageensures the database stays clean and handles thewatch.usersstream caseSummary by CodeRabbit
Bug Fixes
Tests
Chores