fix: Administration settings not being listed after logout and login #37345
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 9082481 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 |
WalkthroughDefers cached-store Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Admin UI
participant SettingsProv as SettingsProvider
participant AuthProv as AuthorizationProvider
participant PrivateStore as PrivateCachedStore
participant PermissionsStore as PermissionsCachedStore
User->>UI: Open Settings
UI->>SettingsProv: request settings
SettingsProv->>PrivateStore: initialize/load cached collection
PrivateStore-->>SettingsProv: cached collection ready
SettingsProv->>PrivateStore: call listen() %%[note `#ccffcc`]%%
PrivateStore-->>SettingsProv: start observing
User->>UI: Logout → Login
UI->>AuthProv: init after login
AuthProv->>PermissionsStore: initialize/load cached permissions
PermissionsStore-->>AuthProv: cached collection ready
AuthProv->>PermissionsStore: call listen() %%[note `#ccffcc`]%%
PermissionsStore-->>AuthProv: start observing
Note over SettingsProv,AuthProv: listen() is deferred until cached collections are ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ 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 |
Fixes administration settings page not listing the settings after logging out and back into the workspace
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37345 +/- ##
===========================================
- Coverage 68.13% 68.12% -0.02%
===========================================
Files 3364 3364
Lines 115811 115808 -3
Branches 20903 20857 -46
===========================================
- Hits 78911 78895 -16
+ Misses 34216 34210 -6
- Partials 2684 2703 +19
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
I wonder if the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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/client/providers/AuthorizationProvider.tsx(2 hunks)apps/meteor/client/providers/SettingsProvider.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/meteor/client/providers/SettingsProvider.tsx (1)
apps/meteor/client/lib/cachedStores/CachedStore.ts (1)
PrivateCachedStore(382-404)
apps/meteor/client/providers/AuthorizationProvider.tsx (1)
apps/meteor/client/cachedStores/PermissionsCachedStore.ts (1)
PermissionsCachedStore(6-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). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
aleksandernsilva
force-pushed
the
fix/settings-not-loading-after-logout
branch
from
b0f0613 to
4205d46
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/meteor/tests/e2e/administration-settings.spec.ts (2)
26-29: Add positive assertion and extract repeated locator.The test currently only verifies that "No results found" is not visible. Consider:
- Adding a positive assertion to verify actual settings content is displayed (e.g., checking for a specific setting or settings list element)
- Extracting the repeated locator to a constant for reuse
This will make the test more robust and maintainable.
Example refactor:
+const noResultsMessage = page.getByText('No results found'); + await test.step('should list settings on load', async () => { - await expect(page.getByText('No results found')).not.toBeVisible(); + await expect(noResultsMessage).not.toBeVisible(); + // Verify actual settings are present + await expect(page.getByRole('region', { name: 'Settings' })).toBeVisible(); });As per coding guidelines
Also applies to: 41-42
32-32: Remove the unnecessary sidebar.close() call to keep the test focused on verifying settings persistence across logout/login.The sidebar.close() operation is pure UI cleanup that doesn't affect logout functionality. Examination of other E2E tests shows this pattern is unique to this test—all other logout calls proceed directly without closing the sidebar first. Since logout is a server-side operation independent of UI state, and the test's core verification is whether settings display persists after logout/login, this line should be removed.
📜 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/administration-settings.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/meteor/tests/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
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)
Use descriptive test names that clearly communicate expected behavior
Use test.beforeAll() and test.afterAll() for setup and teardown
Use test.step() to organize complex test scenarios
Group related tests in the same file
Utilize Playwright fixtures (test, page, expect) consistently
Prefer web-first assertions (e.g., toBeVisible, toHaveText)
Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Maintain test isolation between test cases
Ensure a clean state for each test execution
Ensure tests run reliably in parallel without shared state conflicts
Files:
apps/meteor/tests/e2e/administration-settings.spec.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/administration-settings.spec.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/administration-settings.spec.ts
🧠 Learnings (11)
📚 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/administration-settings.spec.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 : Use descriptive test names that clearly communicate expected behavior
Applied to files:
apps/meteor/tests/e2e/administration-settings.spec.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/administration-settings.spec.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/administration-settings.spec.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 : Group related tests in the same file
Applied to files:
apps/meteor/tests/e2e/administration-settings.spec.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/administration-settings.spec.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/administration-settings.spec.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 : Ensure a clean state for each test execution
Applied to files:
apps/meteor/tests/e2e/administration-settings.spec.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 : Maintain test isolation between test cases
Applied to files:
apps/meteor/tests/e2e/administration-settings.spec.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 : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/e2e/administration-settings.spec.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/administration-settings.spec.ts
🧬 Code graph analysis (1)
apps/meteor/tests/e2e/administration-settings.spec.ts (3)
apps/meteor/tests/e2e/page-objects/admin-settings.ts (1)
AdminSettings(5-45)apps/meteor/tests/e2e/page-objects/fragments/home-sidenav.ts (1)
HomeSidenav(7-254)apps/meteor/tests/e2e/page-objects/login.ts (1)
LoginPage(9-55)
⏰ 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). (7)
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (5)
apps/meteor/tests/e2e/administration-settings.spec.ts (5)
2-4: LGTM! Imports align with Page Object Model.The new imports are correctly structured and necessary for the logout/login test flow.
12-13: LGTM! Page object declarations follow established pattern.
17-19: LGTM! Page objects correctly initialized in beforeEach.
21-24: LGTM! Test structure follows best practices.Good use of nested describe block and beforeEach for navigation.
31-39: Test flow correctly reproduces the reported bug scenario.The logout/login cycle followed by navigation to Settings accurately matches the steps to reproduce from the PR description.
tassoevan
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
This PR fixes an issue in the admin settings page where settings were not being displayed after logging out and back into the workspace. The issue occurred because the necessary cleanup logic wasn’t executed during logout, leaving stale data from the previous session and preventing the settings cache from initializing correctly.
Before:
settings-not-working.mov
After
settings-working.mov
Issue(s)
CORE-1257
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
Tests
Refactor
Chore