-
Notifications
You must be signed in to change notification settings - Fork 13k
fix: Custom status empty value not updating immediately #37443
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
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: 71763ab 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 |
WalkthroughAlways include Changes
Sequence Diagram(s)sequenceDiagram
participant Test as E2E Test
participant Sidenav as HomeSidenav / EditStatusModal
participant API as Meteor API (users.setStatus / setStatusText)
participant Listener as Presence Listener
participant Clients as Other Clients
Note over Test,Sidenav: Test triggers set or clear custom status
Test->>Sidenav: changeUserCustomStatus(text)
Sidenav->>Sidenav: open EditStatusModal, changeStatusMessage(text), dismiss toast
Sidenav->>API: POST users.setStatus (status?, statusText?)
API->>API: validate, update DB (status + statusText)
API->>Listener: build presence diff (status, statusText always present)
Listener->>Clients: broadcast presence diff
Clients->>Clients: render presence (show or clear custom status)
Note over Test: E2E asserts UI reflects update or cleared status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37443 +/- ##
===========================================
+ Coverage 68.79% 68.82% +0.02%
===========================================
Files 3361 3362 +1
Lines 114201 114214 +13
Branches 20619 20660 +41
===========================================
+ Hits 78563 78604 +41
+ Misses 33543 33515 -28
Partials 2095 2095
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
81f03d2 to
fef4820
Compare
cardoso
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
It looks like this test is flaky : Rocket.Chat/apps/meteor/tests/e2e/presence.spec.ts Lines 42 to 46 in fef4820
I think the status text should be generated using faker. And it actually isn't testing whether the status is empty, just that it doesn't contain "new status". |
I know, I'm trying to figure out why
why ?
if the menu doesn't contain the status text that it used to have previously, I think we achieve the result we want. Don't we? |
If the test relies on a fixed string like "new status", another test or a retry of the same test can break that expectation, causing flakyness.
Maybe, but it's better to target exactly the status if possible: Ideally this would be: |
I think its difficult, but yet, possible
I'm afraid its not since Im looking for plain text, no semantic tag besides the menu. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts (1)
256-262: Good Page Object abstraction, but consider stable selectors for menu options.The method correctly encapsulates the custom status change flow and follows the Page Object Model pattern. The delegation to
EditStatusModalpromotes reusability.However, Line 260 uses the localized string
'Custom Status'to identify the menu option. If the application supports multiple languages or the menu text changes, this test will break. Consider using a stable selector like adata-qaattribute instead.Based on learnings
Example refactor if a data-qa attribute is available:
- await this.getUserProfileMenuOption('Custom Status').click(); + await this.userProfileMenu.getByRole('menuitemcheckbox', { name: '', exact: true }).locator('[data-qa="custom-status-option"]').click();Note: This pattern also applies to existing menu interactions in this file (e.g., lines 102, 106, 110), which could be addressed in a follow-up refactor.
📜 Review details
Configuration used: CodeRabbit 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 (3)
apps/meteor/tests/e2e/page-objects/fragments/edit-status-modal.ts(0 hunks)apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts(2 hunks)apps/meteor/tests/e2e/presence.spec.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/meteor/tests/e2e/page-objects/fragments/edit-status-modal.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/tests/e2e/presence.spec.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/meteor/tests/e2e/page-objects/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Utilize and place Page Object implementations under apps/meteor/tests/e2e/page-objects/
Files:
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}: Write concise, technical TypeScript/JavaScript with accurate typing
Follow DRY by extracting reusable logic into helper functions or page objects
Avoid code comments in the implementation
Files:
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts
apps/meteor/tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx}: Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Store commonly used locators in variables/constants for reuse
Use page.waitFor() with specific conditions and avoid hardcoded timeouts
Implement proper wait strategies for dynamic content
Follow the Page Object Model pattern consistently
Files:
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts
🧠 Learnings (12)
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize and place Page Object implementations under apps/meteor/tests/e2e/page-objects/
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx} : Follow the Page Object Model pattern consistently
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (e.g., toBeVisible, toHaveText)
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (test, page, expect) consistently
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx} : Implement proper wait strategies for dynamic content
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx} : Store commonly used locators in variables/constants for reuse
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx} : Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Follow DRY by extracting reusable logic into helper functions or page objects
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx} : Use page.waitFor() with specific conditions and avoid hardcoded timeouts
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All Playwright test files must be located under apps/meteor/tests/e2e/ and use the .spec.ts extension (e.g., login.spec.ts)
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts
📚 Learning: 2025-09-23T19:22:59.217Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 36987
File: apps/meteor/tests/e2e/page-objects/fragments/room-toolbar.ts:10-20
Timestamp: 2025-09-23T19:22:59.217Z
Learning: In Playwright e2e tests, prefer stable selectors like data-qa-id attributes over localized text in getByRole() or getByText() calls to prevent test failures when UI language changes. Test translations separately by validating actual text content after ensuring UI interactions work with stable selectors.
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts
🧬 Code graph analysis (1)
apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts (1)
apps/meteor/tests/e2e/page-objects/fragments/edit-status-modal.ts (1)
EditStatusModal(5-18)
⏰ 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/tests/e2e/page-objects/fragments/edit-status-modal.ts (1)
18-22: Consider waiting for toast visibility before dismissing.The method dismisses the toast immediately after saving without waiting for it to appear or verifying save success. This could introduce a race condition where
dismissToast()is called before the toast is visible, or mask failures if an error toast appears instead.Consider adding explicit wait for toast visibility:
async changeStatusMessage(statusMessage: string): Promise<void> { await this.statusMessageInput.fill(statusMessage); await this.save(); + await this.toastMessages.waitForDisplay({ type: 'success' }); await this.toastMessages.dismissToast(); }Alternatively, if error cases are expected, consider handling both success and error toasts based on the operation result.
📜 Review details
Configuration used: CodeRabbit 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/e2e/page-objects/fragments/edit-status-modal.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/tests/e2e/page-objects/fragments/edit-status-modal.ts (1)
apps/meteor/tests/e2e/page-objects/fragments/toast-messages.ts (1)
ToastMessages(5-29)
⏰ 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/e2e/page-objects/fragments/edit-status-modal.ts (1)
4-4: LGTM: Clean integration of ToastMessages.The import, field declaration, and initialization follow proper patterns and support the new toast-handling behavior.
Also applies to: 7-7, 11-11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/app/api/server/v1/users.ts (1)
1347-1352: Technical debt acknowledged: double user update.The TODO comment correctly identifies that updating the user twice (once in
setStatusTextand again for status/statusDefault) is suboptimal. This creates two separate database writes and potential race conditions.However, the empty string handling on line 1350 properly addresses the PR's core objective—allowing custom status text to be cleared immediately.
Consider consolidating both updates into a single database write in a future refactor to improve performance and eliminate race conditions. This could be done by:
- Building a complete update object that includes both statusText and status changes
- Performing a single
Users.updateOne()call- Broadcasting once with all changed fields
📜 Review details
Configuration used: CodeRabbit 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/app/api/server/v1/users.ts(2 hunks)
⏰ 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 (2)
apps/meteor/app/api/server/v1/users.ts (2)
1354-1386: Good optimization: guard prevents unnecessary status updates.The guard condition
status !== this.bodyParams.status(line 1354) properly prevents unnecessary database writes and broadcasts when the status hasn't actually changed. This improves performance and reduces noise in the presence system.The logic correctly handles:
- Status validation against allowed values
- Permission check for invisible status
- Database update with both
statusandstatusDefault- Cancellation of upcoming calendar status changes
1376-1376: Verification complete—statusText broadcast mechanism is working correctly.The removal of
statusTextfrom line 1376 is intentional and correct. WhensetStatusTextis called, it:
- Updates the user document in the database
- Explicitly broadcasts a
presence.statusevent that includesstatusText- The listeners module (apps/meteor/server/modules/listeners/listeners.module.ts) receives this event and broadcasts
statusTextthrough multiple notification paths (userData,user-status, andsendPresence())This represents a clean separation: line 1376 broadcasts status changes (online/away/busy/offline), while
statusTextchanges flow through the dedicated presence event listener architecture. The PR's objective—ensuring empty status text updates are reflected immediately—is properly satisfied through this mechanism.
532fb00 to
c9c9a79
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/functions/setStatusText.ts (1)
29-29: Replace deprecatedsubstrwithslice.The
substrmethod is deprecated. Usesliceorsubstringinstead.Apply this diff:
- statusText = statusText.trim().substr(0, 120); + statusText = statusText.trim().slice(0, 120);
📜 Review details
Configuration used: CodeRabbit 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 (4)
apps/meteor/app/api/server/v1/users.ts(2 hunks)apps/meteor/app/lib/server/functions/saveUser/saveUser.ts(1 hunks)apps/meteor/app/lib/server/functions/setStatusText.ts(2 hunks)apps/meteor/tests/e2e/presence.spec.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/app/api/server/v1/users.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
apps/meteor/tests/e2e/presence.spec.ts
🧬 Code graph analysis (3)
apps/meteor/tests/e2e/presence.spec.ts (1)
apps/meteor/tests/e2e/page-objects/home-channel.ts (1)
HomeChannel(6-120)
apps/meteor/app/lib/server/functions/setStatusText.ts (2)
packages/models/src/index.ts (1)
Updater(138-138)packages/core-typings/src/IUser.ts (1)
IUser(186-255)
apps/meteor/app/lib/server/functions/saveUser/saveUser.ts (1)
apps/meteor/app/lib/server/functions/setStatusText.ts (1)
setStatusText(63-69)
⏰ 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 (5)
apps/meteor/tests/e2e/presence.spec.ts (2)
1-1: Good addition - addresses test flakiness.Using faker to generate dynamic status text prevents test interference when tests run concurrently or are retried, directly addressing the flakiness concerns raised in the PR comments.
Also applies to: 40-40
66-76: LGTM - test validates the bug fix.This test correctly validates that custom status can be cleared by passing an empty string. The use of
toPass()on line 75 properly handles potential timing issues with reactivity updates.apps/meteor/app/lib/server/functions/saveUser/saveUser.ts (1)
128-128: LGTM - signature change aligns with updated API.The call to
setStatusTextcorrectly uses the new options object signature, maintaining the same parameters (updaterandsession) in the new format.apps/meteor/app/lib/server/functions/setStatusText.ts (2)
12-24: LGTM - cleaner signature with options object.The refactored signature using an options object improves readability and extensibility. The
emitflag with a default value oftruemaintains backward compatibility while allowing callers to control broadcast timing.
50-58: LGTM - conditional broadcasting enables immediate updates.The
emitflag allows callers to control when presence updates are broadcast, and includingstatusTextin the broadcast payload (line 54) enables immediate UI updates. The normalizedstatusTextvalue from line 29 is correctly used in the broadcast.
271422d to
877dbb7
Compare
877dbb7 to
c50714d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/functions/setStatusText.ts (1)
12-24: Options bag + emit flag look good; all callers validate statusText as stringThe new
{ updater, session, emit = true }options bag keeps call sites cleaner and the default preserves previous behavior. All three call sites (setUserStatus, saveUser, users API) validatestatusTextas a string before calling—viacheck(statusText, String),typeofguard, andMatch.Maybe(String)respectively—so thestatusText.trim().substr(0, 120)normalization at line 29 will not throw. Theemitflag correctly gates the broadcast (lines 50-58), preserving behavior whenemit: falseis used in the API endpoint. For cleaner reuse across call sites, consider hoisting the inline options type to a sharedSetStatusTextOptionstype alias.
📜 Review details
Configuration used: CodeRabbit 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)
apps/meteor/app/api/server/v1/users.ts(2 hunks)apps/meteor/app/lib/server/functions/setStatusText.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/app/api/server/v1/users.ts
🧰 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/lib/server/functions/setStatusText.ts
🔇 Additional comments (1)
apps/meteor/app/lib/server/functions/setStatusText.ts (1)
50-58: Emit gating on presence broadcast is correct; call sites properly handle the parameterAll three server-side call sites of
setStatusTextcorrectly handle theemitparameter. The defaultemit: truepreserves the broadcast behavior for the two sites that rely on it (setUserStatus and saveUser), and the API endpoint explicitly passesemit: falsewhen avoiding the broadcast. The RateLimiter wrapper is compatible with this pattern.
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
CORE-1429
Summary by CodeRabbit
Bug Fixes
User Experience
Tests
✏️ Tip: You can customize this high-level summary in your review settings.