fix: prevent push notifications from being sent after logout#37845
fix: prevent push notifications from being sent after logout#37845kodiakhq[bot] merged 4 commits intodevelopfrom
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: a4e9c9e The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 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 |
WalkthroughUpdates push token cleanup: watcher now treats both Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 04:43", "12/19 05:39 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 25 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Fri, 19 Dec 2025 05:39:19 GMT |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/server/services/push/service.ts (1)
16-22: Fix correctly addresses the logout push notification bug.The guard now properly handles both
undefinedand empty array[]cases, ensuring push tokens are cleaned up on logout.Minor observation: The
=== undefinedcheck on line 19 is logically redundant since line 17 normalizes undefined to[], which satisfiesloginTokens.length === 0. Consider simplifying:- if (data.diff['services.resume.loginTokens'] === undefined || loginTokens.length === 0) { + if (loginTokens.length === 0) {That said, the explicit check does document the original bug scenario, so keeping it for clarity is also reasonable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.changeset/poor-apricots-heal.md(1 hunks)apps/meteor/server/services/push/service.ts(1 hunks)
🧰 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/server/services/push/service.ts
⏰ 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). (10)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.2 [legacy watchers] coverage (3/5)
🔇 Additional comments (2)
.changeset/poor-apricots-heal.md (1)
1-5: LGTM!Changeset is correctly formatted with an appropriate patch-level bump and a clear description of the fix.
apps/meteor/server/services/push/service.ts (1)
23-26: Logic is correct.The token extraction and cleanup work as expected.
Note: The
tokens.length > 0check is redundant since we return early at line 21 whenloginTokens.length === 0, andtokenshas the same length viamap. The check doesn't cause harm, but could be removed for clarity if desired.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37845 +/- ##
===========================================
- Coverage 67.69% 67.69% -0.01%
===========================================
Files 3474 3474
Lines 113862 113861 -1
Branches 20942 20949 +7
===========================================
- Hits 77084 77079 -5
- Misses 34598 34600 +2
- Partials 2180 2182 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ricardogarim
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
ricardogarim
removed
stat: ready to merge
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/meteor/tests/end-to-end/api/users.ts (2)
4475-4475: Consider replacing the hardcoded delay with a polling mechanism.The 1-second wait assumes push token cleanup completes within that timeframe, which may not hold under load or in slower CI environments, potentially causing flaky test failures.
🔎 Apply this suggestion to improve test reliability:
Replace the fixed delay with a polling approach that retries until the expected state is reached or a timeout occurs:
- await new Promise((resolve) => setTimeout(resolve, 1000)); + // Poll for token removal with timeout + let tokenRemoved = false; + for (let i = 0; i < 10 && !tokenRemoved; i++) { + await new Promise((resolve) => setTimeout(resolve, 200)); + const res = await request + .delete(api('push.token')) + .set(credentials2) + .send({ token: 'device-1-token' }); + if (res.statusCode === 404) { + tokenRemoved = true; + break; + } + } + expect(tokenRemoved).to.be.true;Alternatively, extract this pattern into a reusable helper function if the codebase adopts this approach for other async tests.
5056-5056: Consider replacing the hardcoded delay with a polling mechanism.Same issue as in the
logoutOtherClientstest above: the fixed 1-second wait may cause flaky failures in slower environments.Apply a similar polling strategy as suggested for line 4475, or extract a shared helper function to avoid duplication:
async function waitForTokenRemoval(credentials: Credentials, token: string, maxAttempts = 10): Promise<boolean> { for (let i = 0; i < maxAttempts; i++) { await new Promise((resolve) => setTimeout(resolve, 200)); const res = await request .delete(api('push.token')) .set(credentials) .send({ token }); if (res.statusCode === 404) { return true; } } return false; }Then use it in both tests to eliminate duplication and improve reliability.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/tests/end-to-end/api/users.ts(2 hunks)
🧰 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/users.ts
🧠 Learnings (8)
📚 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/tests/end-to-end/api/users.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/tests/end-to-end/api/users.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/tests/end-to-end/api/users.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/tests/end-to-end/api/users.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/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/users.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/tests/end-to-end/api/users.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/tests/end-to-end/api/users.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/tests/end-to-end/api/users.ts
🧬 Code graph analysis (1)
apps/meteor/tests/end-to-end/api/users.ts (4)
apps/meteor/tests/data/users.helper.ts (1)
login(80-95)apps/meteor/client/startup/iframeCommands.ts (1)
user(80-89)apps/meteor/tests/e2e/page-objects/fragments/e2ee.ts (1)
password(51-53)apps/meteor/tests/data/api-data.ts (1)
request(10-10)
⏰ 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 comments (1)
apps/meteor/tests/end-to-end/api/users.ts (1)
4437-4498: Good test coverage for push token cleanup behavior.These tests effectively verify the PR's core functionality: ensuring push tokens are removed during logout operations. The test logic correctly validates that:
logoutOtherClientsremoves tokens only for logged-out sessions- Full logout removes all push tokens
Also applies to: 5029-5070
ricardogarim
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
3 participants
As per CORE-1567, this PR resolves a bug that caused logged-out users to continue receiving push notifications.
When a user logged out, their
loginTokensfield was updated to an empty array ([]). However, thePushServicewatcher only handled theundefinedcase, so the cleanup logic responsible for removing push tokens was never triggered on logout. This resulted in stale push tokens remaining in the database and notifications being sent even after the user had logged out.Proposed changes (including videos or screenshots)
Adds a check for empty
loginTokensarrays to ensure push tokens are removed when users log out.Issue(s)
Steps to test or reproduce
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.