feat(ui/ux): add live preview for theme selection in appearance settings#39835
Open
cloudblimp wants to merge 2 commits into
Open
feat(ui/ux): add live preview for theme selection in appearance settings#39835cloudblimp wants to merge 2 commits into
cloudblimp wants to merge 2 commits into
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: bd09bc9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 71 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:
WalkthroughThis PR introduces live theme preview functionality to Accessibility & Appearance Settings, enabling users to preview Light, Dark, and High Contrast themes in real-time before saving their selection. A new context-based provider and hook manage preview state, while the AccessibilityPage integrates preview functionality with form state management. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant AccessibilityPage
participant ThemePreviewProvider
participant useThemePreview
participant useThemeMode
User->>AccessibilityPage: Select theme (Dark)
activate AccessibilityPage
AccessibilityPage->>useThemePreview: setPreviewTheme('dark')
activate useThemePreview
useThemePreview->>ThemePreviewProvider: Update context state
activate ThemePreviewProvider
ThemePreviewProvider-->>useThemePreview: previewTheme = 'dark'
deactivate ThemePreviewProvider
useThemePreview->>useThemeMode: Get current theme mode
activate useThemeMode
useThemeMode-->>useThemePreview: saved theme mode
deactivate useThemeMode
useThemePreview->>useThemePreview: Resolve preview theme<br/>(prioritize 'dark' over saved)
useThemePreview-->>AccessibilityPage: resolvedPreviewTheme = 'dark'
deactivate useThemePreview
AccessibilityPage->>AccessibilityPage: Update form & UI state
deactivate AccessibilityPage
AccessibilityPage-->>User: Theme preview displayed immediately
User->>AccessibilityPage: Click "Save Changes"
activate AccessibilityPage
AccessibilityPage->>AccessibilityPage: Persist theme preference
AccessibilityPage->>useThemePreview: clearPreviewTheme()
useThemePreview->>ThemePreviewProvider: Reset preview to undefined
AccessibilityPage-->>User: Changes saved, preview cleared
deactivate AccessibilityPage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
coderabbitai
Bot
added
the
type: feature
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx">
<violation number="1" location="apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx:63">
P2: Theme Preview tests only validate static rendering without testing live-preview behavior, creating a false safety net. Tests import userEvent but never use it to validate theme selection triggers preview, cancel reverts changes, or save persists changes.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
cloudblimp
force-pushed
the
feat/theme-live-preview
branch
from
d4b2455 to
7e58027
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a client-side “live theme preview” flow for Accessibility & Appearance settings, letting users see theme changes immediately without persisting until “Save”, and reverting on “Cancel”/refresh. This is implemented by introducing a UI-client theme preview context/hook and wiring it into Meteor’s root layout styling and the Accessibility settings form.
Changes:
- Introduce
ThemePreviewProvider+useThemePreview()in@rocket.chat/ui-client(with a preview vs saved theme resolution). - Apply the preview-resolved theme in
MainLayoutStyleTagsso the app updates immediately on selection. - Integrate preview state into
AccessibilityPagesave/cancel behavior and add initial unit tests + a changeset.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui-client/src/providers/index.ts | Re-exports the new ThemePreview provider. |
| packages/ui-client/src/providers/ThemePreviewProvider.tsx | Adds preview theme context + provider. |
| packages/ui-client/src/providers/ThemePreviewProvider.spec.tsx | Adds provider tests (currently introduces a dependency issue). |
| packages/ui-client/src/hooks/useThemePreview.ts | Adds preview hook resolving effective theme. |
| packages/ui-client/src/hooks/useThemePreview.spec.tsx | Adds hook tests (currently has a TS type import issue). |
| packages/ui-client/src/hooks/index.ts | Exports the new hook. |
| apps/meteor/client/views/root/MainLayout/MainLayoutStyleTags.tsx | Switches to preview-resolved theme so UI updates instantly. |
| apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx | Hooks theme radios into preview + save/cancel/footer dirty logic. |
| apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx | Adds AccessibilityPage tests (currently mostly render-only + some issues). |
| apps/meteor/client/providers/MeteorProvider.tsx | Wraps app subtree with ThemePreviewProvider. |
| .changeset/wise-turkeys-matter.md | Adds a changeset (currently bumps many unrelated packages). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+29
to
+33
| clockMode: '0', | ||
| hideUsernames: false, | ||
| hideRoles: false, | ||
| mentionsWithSymbolRequired: false, | ||
| }), |
There was a problem hiding this comment.
The mock for useAccessiblityPreferencesValues returns mentionsWithSymbolRequired, but AccessibilityPage reads mentionsWithSymbol. This means the form default for mentionsWithSymbol will be undefined in tests, which can hide/introduce issues. Please update the mock to use the same keys as the real hook/page.
Comment on lines
+44
to
+46
| const wrapper = ({ children }: { children: React.ReactNode }) => ( | ||
| <ThemePreviewProvider>{children}</ThemePreviewProvider> | ||
| ); |
There was a problem hiding this comment.
This test file references React.ReactNode but doesn’t import React (or ReactNode). With the repo’s TS config (strict + react-jsx, without allowUmdGlobalAccess), this will fail typechecking. Import type { ReactNode } from 'react' and use that, or import type React explicitly.
Comment on lines
+109
to
+113
| if (newTheme === preferencesValues.themeAppearence) { | ||
| clearPreviewTheme(); | ||
| } else { | ||
| setPreviewTheme(newTheme as any); | ||
| } |
There was a problem hiding this comment.
setPreviewTheme(newTheme as any) introduces an any cast for a value that should be restricted to the ThemePreference union. Prefer typing newTheme (and the themeItems IDs) as ThemePreference so setPreviewTheme can be called without casts and invalid theme IDs can’t slip through.
Comment on lines
+1
to
+3
| import { render, screen, waitFor } from '@testing-library/react'; | ||
| import userEvent from '@testing-library/user-event'; | ||
| import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; |
There was a problem hiding this comment.
waitFor and userEvent are imported but never used in this test file. With the repo’s strict TS/ESLint settings (noUnusedLocals), this will fail lint/typecheck. Please remove the unused imports or add interaction assertions that use them.
Comment on lines
+4
to
+62
| 'rocketchat-services': patch | ||
| '@rocket.chat/uikit-playground': patch | ||
| '@rocket.chat/account-service': patch | ||
| '@rocket.chat/authorization-service': patch | ||
| '@rocket.chat/ddp-streamer': patch | ||
| '@rocket.chat/omnichannel-transcript': patch | ||
| '@rocket.chat/presence-service': patch | ||
| '@rocket.chat/queue-worker': patch | ||
| '@rocket.chat/abac': patch | ||
| '@rocket.chat/license': patch | ||
| '@rocket.chat/media-calls': patch | ||
| '@rocket.chat/network-broker': patch | ||
| '@rocket.chat/omni-core-ee': patch | ||
| '@rocket.chat/omnichannel-services': patch | ||
| '@rocket.chat/pdf-worker': patch | ||
| '@rocket.chat/presence': patch | ||
| '@rocket.chat/account-utils': patch | ||
| '@rocket.chat/agenda': patch | ||
| '@rocket.chat/api-client': patch | ||
| '@rocket.chat/base64': patch | ||
| '@rocket.chat/cas-validate': patch | ||
| '@rocket.chat/cron': patch | ||
| '@rocket.chat/ddp-client': patch | ||
| '@rocket.chat/desktop-api': patch | ||
| '@rocket.chat/eslint-config': patch | ||
| '@rocket.chat/favicon': patch | ||
| '@rocket.chat/fuselage-ui-kit': patch | ||
| '@rocket.chat/gazzodown': patch | ||
| '@rocket.chat/http-router': patch | ||
| '@rocket.chat/instance-status': patch | ||
| '@rocket.chat/jest-presets': patch | ||
| '@rocket.chat/jwt': patch | ||
| '@rocket.chat/livechat': patch | ||
| '@rocket.chat/log-format': patch | ||
| '@rocket.chat/logger': patch | ||
| '@rocket.chat/media-signaling': patch | ||
| '@rocket.chat/message-parser': patch | ||
| '@rocket.chat/mock-providers': patch | ||
| '@rocket.chat/mongo-adapter': patch | ||
| '@rocket.chat/poplib': patch | ||
| '@rocket.chat/omni-core': patch | ||
| '@rocket.chat/password-policies': patch | ||
| '@rocket.chat/patch-injection': patch | ||
| '@rocket.chat/peggy-loader': patch | ||
| '@rocket.chat/random': patch | ||
| '@rocket.chat/release-action': patch | ||
| '@rocket.chat/release-changelog': patch | ||
| '@rocket.chat/server-cloud-communication': patch | ||
| '@rocket.chat/sha256': patch | ||
| '@rocket.chat/storybook-config': patch | ||
| '@rocket.chat/tools': patch | ||
| '@rocket.chat/tracing': patch | ||
| '@rocket.chat/tsconfig': patch | ||
| '@rocket.chat/ui-avatar': patch | ||
| '@rocket.chat/ui-composer': patch | ||
| '@rocket.chat/ui-kit': patch | ||
| '@rocket.chat/ui-video-conf': patch | ||
| '@rocket.chat/ui-voip': patch | ||
| '@rocket.chat/web-ui-registration': patch |
There was a problem hiding this comment.
This changeset bumps a very large number of packages that don’t appear to be touched by this PR. That will trigger unnecessary releases and add a lot of version noise. Please reduce the frontmatter to only the packages actually impacted by the code changes (likely @rocket.chat/ui-client and @rocket.chat/meteor).
Suggested change
| 'rocketchat-services': patch | |
| '@rocket.chat/uikit-playground': patch | |
| '@rocket.chat/account-service': patch | |
| '@rocket.chat/authorization-service': patch | |
| '@rocket.chat/ddp-streamer': patch | |
| '@rocket.chat/omnichannel-transcript': patch | |
| '@rocket.chat/presence-service': patch | |
| '@rocket.chat/queue-worker': patch | |
| '@rocket.chat/abac': patch | |
| '@rocket.chat/license': patch | |
| '@rocket.chat/media-calls': patch | |
| '@rocket.chat/network-broker': patch | |
| '@rocket.chat/omni-core-ee': patch | |
| '@rocket.chat/omnichannel-services': patch | |
| '@rocket.chat/pdf-worker': patch | |
| '@rocket.chat/presence': patch | |
| '@rocket.chat/account-utils': patch | |
| '@rocket.chat/agenda': patch | |
| '@rocket.chat/api-client': patch | |
| '@rocket.chat/base64': patch | |
| '@rocket.chat/cas-validate': patch | |
| '@rocket.chat/cron': patch | |
| '@rocket.chat/ddp-client': patch | |
| '@rocket.chat/desktop-api': patch | |
| '@rocket.chat/eslint-config': patch | |
| '@rocket.chat/favicon': patch | |
| '@rocket.chat/fuselage-ui-kit': patch | |
| '@rocket.chat/gazzodown': patch | |
| '@rocket.chat/http-router': patch | |
| '@rocket.chat/instance-status': patch | |
| '@rocket.chat/jest-presets': patch | |
| '@rocket.chat/jwt': patch | |
| '@rocket.chat/livechat': patch | |
| '@rocket.chat/log-format': patch | |
| '@rocket.chat/logger': patch | |
| '@rocket.chat/media-signaling': patch | |
| '@rocket.chat/message-parser': patch | |
| '@rocket.chat/mock-providers': patch | |
| '@rocket.chat/mongo-adapter': patch | |
| '@rocket.chat/poplib': patch | |
| '@rocket.chat/omni-core': patch | |
| '@rocket.chat/password-policies': patch | |
| '@rocket.chat/patch-injection': patch | |
| '@rocket.chat/peggy-loader': patch | |
| '@rocket.chat/random': patch | |
| '@rocket.chat/release-action': patch | |
| '@rocket.chat/release-changelog': patch | |
| '@rocket.chat/server-cloud-communication': patch | |
| '@rocket.chat/sha256': patch | |
| '@rocket.chat/storybook-config': patch | |
| '@rocket.chat/tools': patch | |
| '@rocket.chat/tracing': patch | |
| '@rocket.chat/tsconfig': patch | |
| '@rocket.chat/ui-avatar': patch | |
| '@rocket.chat/ui-composer': patch | |
| '@rocket.chat/ui-kit': patch | |
| '@rocket.chat/ui-video-conf': patch | |
| '@rocket.chat/ui-voip': patch | |
| '@rocket.chat/web-ui-registration': patch |
Comment on lines
+16
to
+18
| * Note: This hook must be used within a ThemePreviewProvider context. | ||
| * | ||
| * @returns {Object} Theme preview utilities |
There was a problem hiding this comment.
The JSDoc says this hook “must be used within a ThemePreviewProvider context”, but the implementation intentionally supports being used outside the provider (NOOP setters + falling back to saved theme). Please update the comment to reflect the actual behavior so consumers don’t add unnecessary providers or expect a runtime error.
Comment on lines
+75
to
+78
| useEffect(() => { | ||
| if (previewTheme) { | ||
| setValue('themeAppearence', previewTheme as any, { shouldDirty: true }); | ||
| } |
There was a problem hiding this comment.
setValue('themeAppearence', previewTheme as any, ...) uses an any cast that weakens type-safety for a user preference field that is already typed as ThemePreference. Prefer keeping the form typed (e.g., useForm<AccessibilityPreferencesData>()) and passing previewTheme directly (or narrowing it) so invalid values can’t slip through.
Comment on lines
+52
to
+56
| render( | ||
| <QueryClientProvider client={queryClient}> | ||
| <AccessibilityPage /> | ||
| </QueryClientProvider> | ||
| ); |
There was a problem hiding this comment.
The current tests only assert that the UI renders. Please add interaction coverage for the new live-preview behavior (click radio -> preview applied + footer visible; click back to saved theme -> preview cleared + footer hidden; Cancel clears preview), so this UX doesn’t regress silently.
Comment on lines
+1
to
+3
| import { render, screen, act } from '@testing-library/react'; | ||
| import userEvent from '@testing-library/user-event'; | ||
| import { ThemePreviewProvider } from './ThemePreviewProvider'; |
There was a problem hiding this comment.
This spec imports @testing-library/user-event, but packages/ui-client/package.json doesn’t list it in devDependencies. That can cause yarn workspace @rocket.chat/ui-client test to fail with “Cannot find module '@testing-library/user-event'”. Either add it as a devDependency for @rocket.chat/ui-client or rewrite the test to avoid user-event (e.g., use fireEvent).
![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: 4
🧹 Nitpick comments (15)
packages/ui-client/src/providers/ThemePreviewProvider.spec.tsx (4)
122-122: Remove code comment.Per coding guidelines, avoid code comments in the implementation.
Suggested fix
- // Both consumers should reflect the same state expect(screen.getByTestId('consumer-a-theme')).toHaveTextContent('dark');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-client/src/providers/ThemePreviewProvider.spec.tsx` at line 122, Remove the inline code comment "// Both consumers should reflect the same state" from the test in ThemePreviewProvider.spec.tsx; locate the comment inside the test that uses ThemePreviewProvider and its two consumers (the test function referencing ThemePreviewProvider and the consumer components) and delete that single-line comment so the test implementation contains no code comments per guidelines.
118-120: Inconsistent click handling.This test uses native
setDarkButton.click()while other tests useuserEvent.click(). For consistency and better simulation of user behavior, preferuserEvent.click().Suggested fix
- act(() => { - setDarkButton.click(); - }); + await userEvent.click(setDarkButton);Note: This would also require making the test function
async.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-client/src/providers/ThemePreviewProvider.spec.tsx` around lines 118 - 120, Replace the native DOM click inside the act block with a user-event click to keep tests consistent and better simulate real user interactions: change the act(() => { setDarkButton.click(); }) usage to await userEvent.click(setDarkButton) and mark the surrounding test function as async; ensure any surrounding act wrappers are removed or adjusted since userEvent already handles async events.
6-6: Remove code comment.Per coding guidelines, avoid code comments in the implementation. This comment can be removed as the component name
TestComponentis self-explanatory for test purposes.Suggested fix
-// Test component that uses the hook const TestComponent = () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-client/src/providers/ThemePreviewProvider.spec.tsx` at line 6, Remove the inline code comment "// Test component that uses the hook" from the test file ThemePreviewProvider.spec.tsx; the TestComponent identifier is already self-descriptive for the test, so simply delete the comment line near the TestComponent declaration to comply with coding guidelines and leave the component and hook usage unchanged.
40-42: Redundantact()wrapper arounduserEvent.click.
userEventfrom@testing-library/user-eventalready wraps interactions inact()internally, making the explicit wrapper unnecessary. This pattern is repeated throughout the test file.Suggested simplification
- await act(async () => { - await userEvent.click(setDarkButton); - }); + await userEvent.click(setDarkButton);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-client/src/providers/ThemePreviewProvider.spec.tsx` around lines 40 - 42, Remove the redundant act wrapper around userEvent interactions: replace patterns like await act(async () => { await userEvent.click(setDarkButton); }); with a direct await userEvent.click(setDarkButton); and do the same for other occurrences in ThemePreviewProvider.spec.tsx where act is only wrapping userEvent.* calls (refer to setDarkButton, act, and userEvent.click to locate them). Ensure no other logic remains inside those act blocks before removing them and run the test suite to confirm behaviour.packages/ui-client/src/hooks/useThemePreview.spec.tsx (3)
33-33: Remove code comment.Per coding guidelines, avoid code comments in the implementation.
Suggested fix
- // Should not throw when calling NOOP functions act(() => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-client/src/hooks/useThemePreview.spec.tsx` at line 33, Remove the inline code comment "// Should not throw when calling NOOP functions" from the test file useThemePreview.spec.tsx so the implementation contains no code comments; locate the test that calls the NOOP functions (the test using the NOOP handlers or the "should not throw" assertion around useThemePreview or its NOOP callbacks) and delete that comment line, leaving the test assertions and code unchanged.
84-84: Remove code comments.Per coding guidelines, avoid code comments in the implementation. Lines 84, 87, and 92 contain inline comments that can be removed.
Suggested fix
- // Initially should be undefined expect(result.current.previewTheme).toBeUndefined(); - // Set preview theme act(() => { result.current.setPreviewTheme('dark'); }); - // Should now have dark as preview expect(result.current.previewTheme).toBe('dark');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-client/src/hooks/useThemePreview.spec.tsx` at line 84, Remove the inline code comments in the test file useThemePreview.spec.tsx: delete the comment text "Initially should be undefined" and the other two inline comments near the assertions in the same test block (the comments at the three positions flagged in the review). Keep the tests and assertions unchanged, only remove those comment lines to comply with the no-comments guideline in tests.
7-7: Remove code comment.Per coding guidelines, avoid code comments in the implementation.
Suggested fix
-// Mock dependencies jest.mock('./useThemeMode');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-client/src/hooks/useThemePreview.spec.tsx` at line 7, Remove the inline code comment "// Mock dependencies" from the useThemePreview.spec.tsx test file (the comment near the mock/setup block at the top of the spec). Simply delete that comment line so the test file contains no implementation comments per coding guidelines.packages/ui-client/src/hooks/useThemePreview.ts (2)
44-48: Remove inline comment.🔧 Suggested fix
if (effectiveTheme === 'high-contrast') { return 'high-contrast'; // takes priority } - //for auto and dark modes, use the system detection result if (isDarkMode) { return 'dark'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-client/src/hooks/useThemePreview.ts` around lines 44 - 48, Remove the inline comment "//for auto and dark modes, use the system detection result" from the useThemePreview function in useThemePreview.ts (locate the conditional handling isDarkMode in that function) so the logic returns 'dark' when isDarkMode is true and 'light' otherwise without the inline comment; simply delete the comment and keep the existing return statements intact.
34-37: Remove inline comments per coding guidelines.As per coding guidelines, avoid code comments in the implementation. The code is self-explanatory given the function names.
🔧 Suggested fix
- // Call useDarkMode with the correct parameter based on effective theme - // When 'auto', pass undefined to detect system preference - // When 'dark', pass true; otherwise, pass false const isDarkMode = useDarkMode(effectiveTheme === 'auto' ? undefined : effectiveTheme === 'dark');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-client/src/hooks/useThemePreview.ts` around lines 34 - 37, Remove the three inline comment lines above the useDarkMode call and leave the implementation as-is: keep the expression const isDarkMode = useDarkMode(effectiveTheme === 'auto' ? undefined : effectiveTheme === 'dark'); but delete the explanatory comments so the file only contains the code (referencing useDarkMode, effectiveTheme, and isDarkMode) in order to comply with the coding guidelines.apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx (3)
106-114: Remove inline comments.Per coding guidelines, avoid code comments. The function name and logic are self-explanatory.
🔧 Suggested fix
const handleThemeChange = (newTheme: string) => { - //Apply preview theme immediately for live preview - // If the selected theme matches the saved theme, clear the preview if (newTheme === preferencesValues.themeAppearence) { clearPreviewTheme(); } else { setPreviewTheme(newTheme as any); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx` around lines 106 - 114, Remove the inline comments inside handleThemeChange; the function name and logic are self-explanatory, so delete the two comment lines and keep the conditional behavior intact: compare newTheme to preferencesValues.themeAppearence and call clearPreviewTheme() or setPreviewTheme(newTheme) accordingly, leaving function signature and existing calls unchanged.
77-77: Avoidas anytype assertion.
previewThemeis typed asThemeMode | undefined, and the form fieldthemeAppearenceshould accept the same type. Theas anycast bypasses type checking and could hide mismatches.Consider ensuring the form's default type matches
ThemeMode:setValue('themeAppearence', previewTheme!, { shouldDirty: true });Or verify the form field type aligns with
ThemeMode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx` at line 77, Replace the unsafe cast in the call to setValue('themeAppearence', previewTheme as any, ...) by ensuring the form field type aligns with previewTheme (ThemeMode | undefined) or by asserting non-null where safe: update the form field type for "themeAppearence" to accept ThemeMode | undefined, or if previewTheme is guaranteed set at this point, use a non-null assertion on previewTheme; adjust the usage in the setValue call (referencing setValue, previewTheme, and the "themeAppearence" field) so no "as any" cast is required.
112-112: Replaceas anywith proper typing ofthemeItemsid field.The
themeItemsarray contains all validThemePreferencevalues ('light', 'dark', 'high-contrast', 'auto'). Type theidfield asThemeModeinstead ofstringto eliminate theas anycast and ensure type safety throughout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx` at line 112, The code is using an unsafe cast "as any" when calling setPreviewTheme(newTheme); fix this by typing the themeItems' id field as ThemeMode (the union of 'light'|'dark'|'high-contrast'|'auto') instead of string so newTheme is inferred as ThemeMode; update the themeItems declaration (the id property) and any related local types so setPreviewTheme accepts newTheme without a cast and remove "as any" from the setPreviewTheme call.apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx (3)
40-43: Consider mockinguseThemePreviewto test preview behavior.The test suite is named "Theme Preview" but doesn't mock
useThemePreview, so preview functionality (setting/clearing preview, theme change triggering preview) cannot be verified. The NOOP fallbacks prevent crashes, but consider adding a mock to enable behavioral testing.const mockSetPreviewTheme = jest.fn(); const mockClearPreviewTheme = jest.fn(); jest.mock('@rocket.chat/ui-client', () => ({ ...jest.requireActual('@rocket.chat/ui-client'), ExternalLink: ({ children, to }: any) => <a href={to}>{children}</a>, useThemePreview: () => ({ previewTheme: undefined, resolvedPreviewTheme: 'light', setPreviewTheme: mockSetPreviewTheme, clearPreviewTheme: mockClearPreviewTheme, }), }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx` around lines 40 - 43, The tests in AccessibilityPage.spec.tsx should mock useThemePreview so preview behavior is testable: add a jest.mock for '@rocket.chat/ui-client' that returns useThemePreview providing previewTheme, resolvedPreviewTheme, and jest.fn() stubs for setPreviewTheme and clearPreviewTheme (e.g., mockSetPreviewTheme, mockClearPreviewTheme) alongside the existing ExternalLink mock; update tests to assert setPreviewTheme/clearPreviewTheme calls and resolvedPreviewTheme-driven rendering in the "Theme Preview" suite using these mock functions.
63-72: Consider more specific assertions.
expect(radios.length).toBeGreaterThan(0)is a weak assertion. Since you expect 4 theme options (light, dark, high-contrast, auto), consider asserting the exact count or finding radios by their labels.expect(radios.length).toBe(4); // or verify each specific radio exists by label🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx` around lines 63 - 72, The test in the it block using screen.getAllByRole('radio') with expect(radios.length).toBeGreaterThan(0) is too weak; update the assertion to assert the exact expected radio count (expect(radios.length).toBe(4)) or replace the generic count check with specific existence checks via labels (use screen.getByLabelText for the 'Light', 'Dark', 'High-Contrast', and 'Auto' radios) so the AccessibilityPage radios are validated precisely.
6-6: Consider removing inline comments.Per coding guidelines, avoid code comments. The code structure and test names are self-documenting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx` at line 6, Remove the inline comment "// Setup" at the top of AccessibilityPage.spec.tsx; the test file and its describe/it/test names should be self-explanatory, so delete that comment (no code changes elsewhere) and ensure test blocks like the AccessibilityPage spec remain clear through their names rather than inline comments.
🤖 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/client/views/account/accessibility/AccessibilityPage.spec.tsx`:
- Line 1: Remove the unused import symbol waitFor from the import statement in
AccessibilityPage.spec.tsx (the line importing from '@testing-library/react');
update the import list to only include render and screen so there are no unused
imports and then run the tests to ensure no lint/errors remain.
- Line 2: Remove the unused import 'userEvent' from the test file: locate the
import statement "import userEvent from '@testing-library/user-event';" in
AccessibilityPage.spec.tsx and delete it (or replace with a used import) so the
file no longer imports a symbol that isn't referenced by any test.
- Line 14: The test file defines an unused jest mock named mockEndpoint; remove
the unused declaration (mockEndpoint) from AccessibilityPage.spec.tsx to
eliminate the dead variable and update any imports if they were only for that
mock so there are no unused import warnings.
In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx`:
- Around line 75-79: Remove the inline implementation comments and either remove
or tighten the useEffect that sets the form field from previewTheme: delete the
offending comments and then update the useEffect (the hook that reads
previewTheme and calls setValue('themeAppearence', ...)) so it only runs when
previewTheme represents an external change (e.g., compare previewTheme against
current form value via getValues('themeAppearence') and bail out if identical)
or remove the hook entirely if onChange(id) in handleThemeChange already
guarantees the form is always in sync; references: useEffect, previewTheme,
setValue, getValues (or equivalent), onChange, handleThemeChange,
setPreviewTheme, and the 'themeAppearence' form field.
---
Nitpick comments:
In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx`:
- Around line 40-43: The tests in AccessibilityPage.spec.tsx should mock
useThemePreview so preview behavior is testable: add a jest.mock for
'@rocket.chat/ui-client' that returns useThemePreview providing previewTheme,
resolvedPreviewTheme, and jest.fn() stubs for setPreviewTheme and
clearPreviewTheme (e.g., mockSetPreviewTheme, mockClearPreviewTheme) alongside
the existing ExternalLink mock; update tests to assert
setPreviewTheme/clearPreviewTheme calls and resolvedPreviewTheme-driven
rendering in the "Theme Preview" suite using these mock functions.
- Around line 63-72: The test in the it block using screen.getAllByRole('radio')
with expect(radios.length).toBeGreaterThan(0) is too weak; update the assertion
to assert the exact expected radio count (expect(radios.length).toBe(4)) or
replace the generic count check with specific existence checks via labels (use
screen.getByLabelText for the 'Light', 'Dark', 'High-Contrast', and 'Auto'
radios) so the AccessibilityPage radios are validated precisely.
- Line 6: Remove the inline comment "// Setup" at the top of
AccessibilityPage.spec.tsx; the test file and its describe/it/test names should
be self-explanatory, so delete that comment (no code changes elsewhere) and
ensure test blocks like the AccessibilityPage spec remain clear through their
names rather than inline comments.
In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx`:
- Around line 106-114: Remove the inline comments inside handleThemeChange; the
function name and logic are self-explanatory, so delete the two comment lines
and keep the conditional behavior intact: compare newTheme to
preferencesValues.themeAppearence and call clearPreviewTheme() or
setPreviewTheme(newTheme) accordingly, leaving function signature and existing
calls unchanged.
- Line 77: Replace the unsafe cast in the call to setValue('themeAppearence',
previewTheme as any, ...) by ensuring the form field type aligns with
previewTheme (ThemeMode | undefined) or by asserting non-null where safe: update
the form field type for "themeAppearence" to accept ThemeMode | undefined, or if
previewTheme is guaranteed set at this point, use a non-null assertion on
previewTheme; adjust the usage in the setValue call (referencing setValue,
previewTheme, and the "themeAppearence" field) so no "as any" cast is required.
- Line 112: The code is using an unsafe cast "as any" when calling
setPreviewTheme(newTheme); fix this by typing the themeItems' id field as
ThemeMode (the union of 'light'|'dark'|'high-contrast'|'auto') instead of string
so newTheme is inferred as ThemeMode; update the themeItems declaration (the id
property) and any related local types so setPreviewTheme accepts newTheme
without a cast and remove "as any" from the setPreviewTheme call.
In `@packages/ui-client/src/hooks/useThemePreview.spec.tsx`:
- Line 33: Remove the inline code comment "// Should not throw when calling NOOP
functions" from the test file useThemePreview.spec.tsx so the implementation
contains no code comments; locate the test that calls the NOOP functions (the
test using the NOOP handlers or the "should not throw" assertion around
useThemePreview or its NOOP callbacks) and delete that comment line, leaving the
test assertions and code unchanged.
- Line 84: Remove the inline code comments in the test file
useThemePreview.spec.tsx: delete the comment text "Initially should be
undefined" and the other two inline comments near the assertions in the same
test block (the comments at the three positions flagged in the review). Keep the
tests and assertions unchanged, only remove those comment lines to comply with
the no-comments guideline in tests.
- Line 7: Remove the inline code comment "// Mock dependencies" from the
useThemePreview.spec.tsx test file (the comment near the mock/setup block at the
top of the spec). Simply delete that comment line so the test file contains no
implementation comments per coding guidelines.
In `@packages/ui-client/src/hooks/useThemePreview.ts`:
- Around line 44-48: Remove the inline comment "//for auto and dark modes, use
the system detection result" from the useThemePreview function in
useThemePreview.ts (locate the conditional handling isDarkMode in that function)
so the logic returns 'dark' when isDarkMode is true and 'light' otherwise
without the inline comment; simply delete the comment and keep the existing
return statements intact.
- Around line 34-37: Remove the three inline comment lines above the useDarkMode
call and leave the implementation as-is: keep the expression const isDarkMode =
useDarkMode(effectiveTheme === 'auto' ? undefined : effectiveTheme === 'dark');
but delete the explanatory comments so the file only contains the code
(referencing useDarkMode, effectiveTheme, and isDarkMode) in order to comply
with the coding guidelines.
In `@packages/ui-client/src/providers/ThemePreviewProvider.spec.tsx`:
- Line 122: Remove the inline code comment "// Both consumers should reflect the
same state" from the test in ThemePreviewProvider.spec.tsx; locate the comment
inside the test that uses ThemePreviewProvider and its two consumers (the test
function referencing ThemePreviewProvider and the consumer components) and
delete that single-line comment so the test implementation contains no code
comments per guidelines.
- Around line 118-120: Replace the native DOM click inside the act block with a
user-event click to keep tests consistent and better simulate real user
interactions: change the act(() => { setDarkButton.click(); }) usage to await
userEvent.click(setDarkButton) and mark the surrounding test function as async;
ensure any surrounding act wrappers are removed or adjusted since userEvent
already handles async events.
- Line 6: Remove the inline code comment "// Test component that uses the hook"
from the test file ThemePreviewProvider.spec.tsx; the TestComponent identifier
is already self-descriptive for the test, so simply delete the comment line near
the TestComponent declaration to comply with coding guidelines and leave the
component and hook usage unchanged.
- Around line 40-42: Remove the redundant act wrapper around userEvent
interactions: replace patterns like await act(async () => { await
userEvent.click(setDarkButton); }); with a direct await
userEvent.click(setDarkButton); and do the same for other occurrences in
ThemePreviewProvider.spec.tsx where act is only wrapping userEvent.* calls
(refer to setDarkButton, act, and userEvent.click to locate them). Ensure no
other logic remains inside those act blocks before removing them and run the
test suite to confirm behaviour.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 677b1c29-655e-4066-bb0c-d0b490ba7c0f
📒 Files selected for processing (11)
.changeset/wise-turkeys-matter.mdapps/meteor/client/providers/MeteorProvider.tsxapps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsxapps/meteor/client/views/account/accessibility/AccessibilityPage.tsxapps/meteor/client/views/root/MainLayout/MainLayoutStyleTags.tsxpackages/ui-client/src/hooks/index.tspackages/ui-client/src/hooks/useThemePreview.spec.tsxpackages/ui-client/src/hooks/useThemePreview.tspackages/ui-client/src/providers/ThemePreviewProvider.spec.tsxpackages/ui-client/src/providers/ThemePreviewProvider.tsxpackages/ui-client/src/providers/index.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). (2)
- GitHub Check: Agent
- GitHub Check: cubic · AI code reviewer
🧰 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:
packages/ui-client/src/providers/index.tsapps/meteor/client/providers/MeteorProvider.tsxpackages/ui-client/src/hooks/index.tsapps/meteor/client/views/root/MainLayout/MainLayoutStyleTags.tsxpackages/ui-client/src/providers/ThemePreviewProvider.tsxapps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsxpackages/ui-client/src/hooks/useThemePreview.spec.tsxpackages/ui-client/src/hooks/useThemePreview.tsapps/meteor/client/views/account/accessibility/AccessibilityPage.tsxpackages/ui-client/src/providers/ThemePreviewProvider.spec.tsx
🧠 Learnings (23)
📚 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:
.changeset/wise-turkeys-matter.md
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
.changeset/wise-turkeys-matter.mdapps/meteor/client/providers/MeteorProvider.tsxapps/meteor/client/views/root/MainLayout/MainLayoutStyleTags.tsxapps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsxapps/meteor/client/views/account/accessibility/AccessibilityPage.tsx
📚 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:
.changeset/wise-turkeys-matter.md
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
.changeset/wise-turkeys-matter.md
📚 Learning: 2026-03-16T21:50:37.589Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:37.589Z
Learning: For changes related to OpenAPI migrations in Rocket.Chat/OpenAPI, when removing endpoint types and validators from rocket.chat/rest-typings (e.g., UserRegisterParamsPOST, /v1/users.register) document this as a minor changeset (not breaking) per RocketChat/Rocket.Chat-Open-API#150 Rule 7. Note that the endpoint type is re-exposed via a module augmentation .d.ts in the consuming package (e.g., packages/web-ui-registration/src/users-register.d.ts). In reviews, ensure the changeset clearly states: this is a non-breaking change, the major version should not be bumped, and the changeset reflects a minor version bump. Do not treat this as a breaking change during OpenAPI migrations.
Applied to files:
.changeset/wise-turkeys-matter.md
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/ui-client/src/providers/index.tspackages/ui-client/src/hooks/index.tspackages/ui-client/src/hooks/useThemePreview.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/ui-client/src/providers/index.tspackages/ui-client/src/hooks/index.tspackages/ui-client/src/hooks/useThemePreview.ts
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/client/providers/MeteorProvider.tsx
📚 Learning: 2026-03-04T14:16:49.202Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39304
File: packages/ui-contexts/src/ActionManagerContext.ts:26-26
Timestamp: 2026-03-04T14:16:49.202Z
Learning: In `packages/ui-contexts/src/ActionManagerContext.ts` (TypeScript, RocketChat/Rocket.Chat), the `disposeView` method in `IActionManager` uses an intentionally explicit union `UiKit.ModalView['id'] | UiKit.BannerView['viewId'] | UiKit.ContextualBarView['id']` to document which view types are accepted, even though all constituents resolve to the same primitive. The inline `// eslint-disable-next-line typescript-eslint/no-duplicate-type-constituents` comment is intentional and should not be flagged or removed.
Applied to files:
apps/meteor/client/providers/MeteorProvider.tsx
📚 Learning: 2025-11-17T15:07:13.273Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37398
File: packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx:357-363
Timestamp: 2025-11-17T15:07:13.273Z
Learning: In packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx, IconElement is a presentational, non-actionable element that does not require wrapping in AppIdProvider, similar to plain_text and mrkdwn renderers. Only actionable elements (those with actions, actionId, or interactive behavior) should be wrapped in AppIdProvider.
Applied to files:
apps/meteor/client/providers/MeteorProvider.tsx
📚 Learning: 2026-01-19T18:08:05.314Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38169
File: packages/ui-client/src/hooks/useGoToDirectMessage.ts:20-27
Timestamp: 2026-01-19T18:08:05.314Z
Learning: Rule of Hooks: In React, hooks (including custom hooks like useGoToDirectMessage and useUserSubscriptionByName) must be called unconditionally and in the same order on every render. Do not conditionally call hooks based on values; instead, derive safe inputs if a value may be undefined (e.g., pass an empty string or undefined) and handle variations in logic outside the hook invocation. This preserves hook order and avoids violations. Apply this guideline to all files under packages/ui-client/src/hooks where hooks are used.
Applied to files:
packages/ui-client/src/hooks/index.tspackages/ui-client/src/hooks/useThemePreview.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/client/views/account/accessibility/AccessibilityPage.spec.tsxpackages/ui-client/src/hooks/useThemePreview.spec.tsxpackages/ui-client/src/providers/ThemePreviewProvider.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 Learning: 2026-02-24T19:22:52.984Z
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:52.984Z
Learning: In RocketChat/Rocket.Chat Playwright e2e tests, prefer using translated text and ARIA roles (getByRole, getByText, etc.) over data-qa locators. If translation values change, update the corresponding test locators accordingly. Never prefer data-qa locators.
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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 : Avoid using `page.locator()` in Playwright tests - always prefer semantic locators such as `page.getByRole()`, `page.getByLabel()`, `page.getByText()`, or `page.getByTitle()`
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 Learning: 2026-02-24T19:39:52.176Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/message.ts:7-7
Timestamp: 2026-02-24T19:39:52.176Z
Learning: In RocketChat/Rocket.Chat e2e tests, never prefer data-qa attributes to locate elements. Always prefer semantic locators (getByRole, getByLabel, getByText, getByTitle) and ARIA-based selectors over data-qa locators.
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsxpackages/ui-client/src/hooks/useThemePreview.spec.tsxpackages/ui-client/src/providers/ThemePreviewProvider.spec.tsx
🔇 Additional comments (10)
.changeset/wise-turkeys-matter.md (1)
1-65: LGTM!The changeset correctly documents the new live theme preview feature with appropriate version bumps:
minorfor the packages receiving new functionality (@rocket.chat/ui-clientand@rocket.chat/meteor) andpatchfor dependent packages in the monorepo.packages/ui-client/src/providers/index.ts (1)
2-2: LGTM!The barrel export correctly exposes the new
ThemePreviewProvideralongside the existingModalProvider, with no naming collisions.packages/ui-client/src/hooks/index.ts (1)
11-11: LGTM!The new
useThemePreviewhook is correctly added to the barrel export, maintaining the existing alphabetical ordering convention.apps/meteor/client/providers/MeteorProvider.tsx (2)
1-1: LGTM!The import correctly combines
ModalProviderandThemePreviewProviderfrom@rocket.chat/ui-client.
45-61: LGTM!The
ThemePreviewProvideris well-positioned in the provider hierarchy—nested insideModalProviderso modals can also access the preview context, and wrapping the rest of the subtree to ensure all consumers (includingMainLayoutStyleTagsandAccessibilityPage) have access to the theme preview state.apps/meteor/client/views/root/MainLayout/MainLayoutStyleTags.tsx (1)
6-13: LGTM!The component correctly integrates with the new theme preview system:
- Uses
resolvedPreviewThemefromuseThemePreview()for theme resolution- Dynamic
tagIdensures proper style tag updates during live preview- Conditional
codeBlockpalette rendering correctly depends on the resolved preview themepackages/ui-client/src/providers/ThemePreviewProvider.tsx (1)
17-29: LGTM!Clean implementation of the theme preview context provider:
- Proper memoization of the context value prevents unnecessary re-renders
- Correct dependency array
[previewTheme]ensures the value updates only when the preview theme changes- The pattern of using
undefinedas the default context value works well with the NOOP fallback in the consumer hookpackages/ui-client/src/hooks/useThemePreview.ts (1)
24-56: LGTM!The hook implementation is well-structured with proper memoization, safe NOOP fallbacks when used outside the provider, and follows the established pattern from
useThemeMode. The hook order is maintained correctly.apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx (2)
151-159: LGTM - Radio button integration.The radio button correctly:
- Calls form's
onChangeto update form state- Calls
handleThemeChangeto manage preview state- Uses
(previewTheme ?? value)for checked state, properly prioritizing preview over saved value
265-271: LGTM - Footer and button state management.The footer visibility (
isDirty || !!previewTheme) and save button enablement (!isDirty && !previewTheme) correctly account for both form dirty state and active preview state.
| }, | ||
| }); | ||
|
|
||
| const mockEndpoint = jest.fn(); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused variable mockEndpoint.
mockEndpoint is defined but never used in any test case.
🔧 Suggested fix
-const mockEndpoint = jest.fn();
-
// Mock all the heavy dependencies📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const mockEndpoint = jest.fn(); | |
| // Mock all the heavy dependencies |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx` at
line 14, The test file defines an unused jest mock named mockEndpoint; remove
the unused declaration (mockEndpoint) from AccessibilityPage.spec.tsx to
eliminate the dead variable and update any imports if they were only for that
mock so there are no unused import warnings.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx (1)
35-39:⚠️ Potential issue | 🟠 MajorPreview theme persists if user navigates away without explicit Cancel/Save.
The
useThemePreviewhook has no cleanup mechanism on unmount, and this component only clears the preview on explicit user actions (Cancel button or successful Save). If a user selects a different theme and then navigates away (e.g., clicks a sidebar link), the preview state persists app-wide, affectingMainLayoutStyleTagsand leaving the entire app in an unintended visual state.Consider adding a cleanup effect:
🛡️ Proposed fix
const AccessibilityPage = () => { const t = useTranslation(); const dispatchToastMessage = useToastMessageDispatch(); const preferencesValues = useAccessiblityPreferencesValues(); const { setPreviewTheme, clearPreviewTheme, previewTheme } = useThemePreview(); + + useEffect(() => { + return () => { + clearPreviewTheme(); + }; + }, [clearPreviewTheme]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx` around lines 35 - 39, AccessibilityPage currently uses useThemePreview (setPreviewTheme, clearPreviewTheme, previewTheme) but doesn't clear the preview on unmount, so a selected preview can persist if the user navigates away; add a cleanup effect in the AccessibilityPage component (useEffect with a return cleanup) that calls clearPreviewTheme() on unmount (and optionally only when previewTheme is set) to ensure any temporary preview is removed if the user leaves without Cancel/Save.
🧹 Nitpick comments (3)
apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx (1)
77-77: Avoidas anytype assertions.Lines 77 and 112 use
as anyto bypass type checking. If the theme IDs fromthemeItemsdon't align with theThemeModetype expected bysetPreviewTheme, this could mask type mismatches. Consider using the proper type or adding a type guard.♻️ Suggested approach
+import type { ThemePreference as ThemeMode } from '@rocket.chat/core-typings'; + // In handleThemeChange: - setPreviewTheme(newTheme as any); + setPreviewTheme(newTheme as ThemeMode); // In useEffect: - setValue('themeAppearence', previewTheme as any, { shouldDirty: true }); + setValue('themeAppearence', previewTheme, { shouldDirty: true });If the form's
themeAppearencefield type differs fromThemeMode, consider aligning the types inAccessibilityPreferencesData.Also applies to: 112-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx` at line 77, Replace the unsafe "as any" assertions when calling setValue('themeAppearence', previewTheme as any, ...) and where setPreviewTheme is used: ensure previewTheme is validated/cast to the expected ThemeMode instead of bypassing types. Locate themeItems, previewTheme, AccessibilityPreferencesData, ThemeMode, setPreviewTheme and the setValue calls and either (a) update AccessibilityPreferencesData so themeAppearence's type matches ThemeMode, or (b) add a narrow type guard that converts/validates the theme ID from themeItems into a ThemeMode before passing it to setPreviewTheme/setValue; remove the "as any" once the value is properly typed.apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx (2)
40-43: Tests don't exercise actual theme preview functionality.The mock for
@rocket.chat/ui-clientspreadsrequireActualwithout mockinguseThemePreview, and noThemePreviewProviderwraps the rendered component. This causesuseThemePreviewto return NOOP functions, meaning these tests don't validate the live preview behavior described in the PR (clicking a theme radio should trigger a preview, cancel should revert, etc.).Consider either:
- Wrapping the render with
ThemePreviewProvider, or- Explicitly mocking
useThemePreviewto track calls and verify interactions♻️ Example: Mock useThemePreview for interaction testing
const mockSetPreviewTheme = jest.fn(); const mockClearPreviewTheme = jest.fn(); jest.mock('@rocket.chat/ui-client', () => ({ ...jest.requireActual('@rocket.chat/ui-client'), ExternalLink: ({ children, to }: any) => <a href={to}>{children}</a>, useThemePreview: () => ({ previewTheme: undefined, resolvedPreviewTheme: 'light', setPreviewTheme: mockSetPreviewTheme, clearPreviewTheme: mockClearPreviewTheme, }), })); // Then test interactions: it('should call setPreviewTheme when selecting a different theme', async () => { const user = userEvent.setup(); render(/* ... */); await user.click(screen.getByLabelText('Theme_dark')); expect(mockSetPreviewTheme).toHaveBeenCalledWith('dark'); });Also applies to: 51-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx` around lines 40 - 43, Tests currently don't exercise theme preview because useThemePreview returns NOOPs; update the test setup in AccessibilityPage.spec.tsx to either wrap the rendered component with ThemePreviewProvider or explicitly mock useThemePreview in the jest.mock block so you can assert interactions. If you choose mocking, add mockSetPreviewTheme and mockClearPreviewTheme (jest.fn()) and return them from useThemePreview along with previewTheme/resolvedPreviewTheme, then assert calls to mockSetPreviewTheme and mockClearPreviewTheme when interacting with the theme radio buttons; if you wrap with ThemePreviewProvider, render the component inside that provider so the real preview hooks are used.
70-72: Consider more specific assertions for better regression detection.The current assertions (
toBeGreaterThan(0),toBeGreaterThanOrEqual(2)) are very permissive. More specific assertions would catch regressions if theme options are accidentally removed or buttons change.♻️ Suggested improvements
it('should have theme radio buttons for different options', () => { // ... const radios = screen.getAllByRole('radio'); - expect(radios.length).toBeGreaterThan(0); + expect(radios).toHaveLength(4); // light, dark, high-contrast, match-system }); it('should render cancel and save buttons', () => { // ... - const buttons = screen.getAllByRole('button'); - expect(buttons.length).toBeGreaterThanOrEqual(2); + expect(screen.getByRole('button', { name: 'Cancel' })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Save_changes' })).toBeInTheDocument(); });Also applies to: 81-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx` around lines 70 - 72, Replace the permissive length assertions (e.g., the radios variable created by screen.getAllByRole('radio') and the expect(...).toBeGreaterThan(0) call) with more specific assertions: assert the exact expected count using expect(radios).toHaveLength(expectedCount) or assert the presence of specific options by using screen.getByLabelText('Option Label') or screen.getByRole('radio', { name: /Option Label/i }). Apply the same change to the other block (the getAllByRole(...) and expect(...).toBeGreaterThanOrEqual(...) around lines 81-84) by asserting exact lengths or specific labeled elements so the tests will catch accidental removals or renames.
🤖 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/client/views/account/accessibility/AccessibilityPage.spec.tsx`:
- Line 6: Remove the inline comments in AccessibilityPage.spec.tsx (specifically
the comment lines containing "// Setup", "// Mock all the heavy dependencies",
"// Check if theme options are rendered with translation keys", and "// Should
have at least Cancel and Save buttons") and any other stray // comments in that
spec file; instead, fold the intent into clearer test/describe/it names or
helper function names (e.g., rename tests under the
describe('AccessibilityPage', ...) block or extract setup into a named setup
function) so no inline comments remain.
---
Outside diff comments:
In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx`:
- Around line 35-39: AccessibilityPage currently uses useThemePreview
(setPreviewTheme, clearPreviewTheme, previewTheme) but doesn't clear the preview
on unmount, so a selected preview can persist if the user navigates away; add a
cleanup effect in the AccessibilityPage component (useEffect with a return
cleanup) that calls clearPreviewTheme() on unmount (and optionally only when
previewTheme is set) to ensure any temporary preview is removed if the user
leaves without Cancel/Save.
---
Nitpick comments:
In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx`:
- Around line 40-43: Tests currently don't exercise theme preview because
useThemePreview returns NOOPs; update the test setup in
AccessibilityPage.spec.tsx to either wrap the rendered component with
ThemePreviewProvider or explicitly mock useThemePreview in the jest.mock block
so you can assert interactions. If you choose mocking, add mockSetPreviewTheme
and mockClearPreviewTheme (jest.fn()) and return them from useThemePreview along
with previewTheme/resolvedPreviewTheme, then assert calls to mockSetPreviewTheme
and mockClearPreviewTheme when interacting with the theme radio buttons; if you
wrap with ThemePreviewProvider, render the component inside that provider so the
real preview hooks are used.
- Around line 70-72: Replace the permissive length assertions (e.g., the radios
variable created by screen.getAllByRole('radio') and the
expect(...).toBeGreaterThan(0) call) with more specific assertions: assert the
exact expected count using expect(radios).toHaveLength(expectedCount) or assert
the presence of specific options by using screen.getByLabelText('Option Label')
or screen.getByRole('radio', { name: /Option Label/i }). Apply the same change
to the other block (the getAllByRole(...) and
expect(...).toBeGreaterThanOrEqual(...) around lines 81-84) by asserting exact
lengths or specific labeled elements so the tests will catch accidental removals
or renames.
In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx`:
- Line 77: Replace the unsafe "as any" assertions when calling
setValue('themeAppearence', previewTheme as any, ...) and where setPreviewTheme
is used: ensure previewTheme is validated/cast to the expected ThemeMode instead
of bypassing types. Locate themeItems, previewTheme,
AccessibilityPreferencesData, ThemeMode, setPreviewTheme and the setValue calls
and either (a) update AccessibilityPreferencesData so themeAppearence's type
matches ThemeMode, or (b) add a narrow type guard that converts/validates the
theme ID from themeItems into a ThemeMode before passing it to
setPreviewTheme/setValue; remove the "as any" once the value is properly typed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 578a73cf-242d-44d4-8d19-a255559597ed
📒 Files selected for processing (11)
.changeset/wise-turkeys-matter.mdapps/meteor/client/providers/MeteorProvider.tsxapps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsxapps/meteor/client/views/account/accessibility/AccessibilityPage.tsxapps/meteor/client/views/root/MainLayout/MainLayoutStyleTags.tsxpackages/ui-client/src/hooks/index.tspackages/ui-client/src/hooks/useThemePreview.spec.tsxpackages/ui-client/src/hooks/useThemePreview.tspackages/ui-client/src/providers/ThemePreviewProvider.spec.tsxpackages/ui-client/src/providers/ThemePreviewProvider.tsxpackages/ui-client/src/providers/index.ts
✅ Files skipped from review due to trivial changes (5)
- packages/ui-client/src/providers/index.ts
- packages/ui-client/src/hooks/index.ts
- packages/ui-client/src/providers/ThemePreviewProvider.tsx
- packages/ui-client/src/providers/ThemePreviewProvider.spec.tsx
- .changeset/wise-turkeys-matter.md
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/meteor/client/views/root/MainLayout/MainLayoutStyleTags.tsx
- packages/ui-client/src/hooks/useThemePreview.spec.tsx
- packages/ui-client/src/hooks/useThemePreview.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/client/providers/MeteorProvider.tsxapps/meteor/client/views/account/accessibility/AccessibilityPage.tsxapps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
🧠 Learnings (28)
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/client/providers/MeteorProvider.tsx
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
apps/meteor/client/providers/MeteorProvider.tsxapps/meteor/client/views/account/accessibility/AccessibilityPage.tsxapps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 Learning: 2026-03-04T14:16:49.202Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39304
File: packages/ui-contexts/src/ActionManagerContext.ts:26-26
Timestamp: 2026-03-04T14:16:49.202Z
Learning: In `packages/ui-contexts/src/ActionManagerContext.ts` (TypeScript, RocketChat/Rocket.Chat), the `disposeView` method in `IActionManager` uses an intentionally explicit union `UiKit.ModalView['id'] | UiKit.BannerView['viewId'] | UiKit.ContextualBarView['id']` to document which view types are accepted, even though all constituents resolve to the same primitive. The inline `// eslint-disable-next-line typescript-eslint/no-duplicate-type-constituents` comment is intentional and should not be flagged or removed.
Applied to files:
apps/meteor/client/providers/MeteorProvider.tsxapps/meteor/client/views/account/accessibility/AccessibilityPage.tsx
📚 Learning: 2025-11-17T15:07:13.273Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37398
File: packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx:357-363
Timestamp: 2025-11-17T15:07:13.273Z
Learning: In packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx, IconElement is a presentational, non-actionable element that does not require wrapping in AppIdProvider, similar to plain_text and mrkdwn renderers. Only actionable elements (those with actions, actionId, or interactive behavior) should be wrapped in AppIdProvider.
Applied to files:
apps/meteor/client/providers/MeteorProvider.tsx
📚 Learning: 2026-03-06T18:10:23.330Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:23.330Z
Learning: In the RocketChat/Rocket.Chat `packages/gazzodown` package and more broadly, the HTML `<code>` element has an implicit ARIA role of `code` per WAI-ARIA 1.3, and `testing-library/dom` / jsdom supports it. Therefore, `screen.getByRole('code')` / `screen.findByRole('code')` correctly locates `<code>` elements without needing an explicit `role="code"` attribute. Do NOT flag `findByRole('code')` as invalid in future reviews.
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx
📚 Learning: 2026-03-18T16:08:17.800Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39590
File: apps/meteor/client/views/omnichannel/contactInfo/EditContactInfo.tsx:97-99
Timestamp: 2026-03-18T16:08:17.800Z
Learning: In `apps/meteor/client/views/omnichannel/contactInfo/EditContactInfo.tsx`, `reValidateMode: 'onBlur'` is intentionally used (not 'onChange') because the `validateEmailFormat` and `validatePhone` functions are async and call the `checkExistenceEndpoint` API to check for duplicates. Using 'onChange' would trigger excessive network requests on every keystroke. The combination of `mode: 'onSubmit'` with `reValidateMode: 'onBlur'` is a deliberate design decision to minimize API calls while still providing revalidation feedback.
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx
📚 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} : Avoid code comments in the implementation
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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 : Avoid using `page.locator()` in Playwright tests - always prefer semantic locators such as `page.getByRole()`, `page.getByLabel()`, `page.getByText()`, or `page.getByTitle()`
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 Learning: 2026-02-24T19:39:52.176Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/message.ts:7-7
Timestamp: 2026-02-24T19:39:52.176Z
Learning: In RocketChat/Rocket.Chat e2e tests, never prefer data-qa attributes to locate elements. Always prefer semantic locators (getByRole, getByLabel, getByText, getByTitle) and ARIA-based selectors over data-qa locators.
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 Learning: 2026-02-24T19:22:52.984Z
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:52.984Z
Learning: In RocketChat/Rocket.Chat Playwright e2e tests, prefer using translated text and ARIA roles (getByRole, getByText, etc.) over data-qa locators. If translation values change, update the corresponding test locators accordingly. Never prefer data-qa locators.
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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 `page.waitFor()` with specific conditions instead of hardcoded timeouts in Playwright tests
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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 : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 Learning: 2025-11-24T17:08:26.531Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/test-cases.mdc:0-0
Timestamp: 2025-11-24T17:08:26.531Z
Learning: Focus on comprehensive feature coverage and edge case validation in test design
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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} : Store commonly used locators in variables/constants for reuse
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
🔇 Additional comments (5)
apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx (3)
107-108: Remove inline comments that violate coding guidelines.These comments should be removed per the coding guideline to avoid code comments in the implementation.
184-184: LGTM on accessibility improvements.The
aria-labelattributes on theSelectcomponents improve screen reader support and align with the correspondingFieldLabeltext.Also applies to: 217-217
151-159: Well-designed preview integration.The theme preview logic correctly:
- Updates both form value and preview state on radio change
- Uses
previewTheme ?? valuefor visual state precedence- Accounts for preview state in footer dirty/disabled logic
- Clears preview on both Save success and Cancel
Also applies to: 265-268
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx (1)
1-2: Remove unused imports and variables.
waitFor,userEvent, andmockEndpointare imported/defined but never used in the tests.Also applies to: 14-14
apps/meteor/client/providers/MeteorProvider.tsx (1)
1-1: LGTM!The
ThemePreviewProvideris correctly integrated into the provider tree. Its placement insideModalProvideris appropriate since it has no dependencies on downstream providers—it only manages transient preview state viauseStateanduseMemo.Also applies to: 45-61
cloudblimp
force-pushed
the
feat/theme-live-preview
branch
from
db1c83e to
f6a74e7
Compare
coderabbitai
Bot
added
the
type: feature
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx (1)
44-129: Test the preview lifecycle, not just the static render.The feature added here is the provider/hook-driven preview flow, but this suite only renders the page and checks text/counts. It still doesn't prove that selecting a theme updates the checked radio, shows the dirty footer, and clears preview on Cancel/Save. Please add interaction-based coverage around the real preview context (or a stateful
useThemePreviewmock) instead of relying on> 0/>= 2assertions.Based on learnings, "Focus on comprehensive feature coverage and edge case validation in test design".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx` around lines 44 - 129, The tests only assert static render; add interaction tests that exercise the preview lifecycle via the real preview context or a stateful useThemePreview mock: render AccessibilityPage wrapped with the actual ThemePreviewProvider (or a test provider that uses useThemePreview), simulate clicking a theme radio (e.g., the radio with label 'Theme_dark'), assert that radio becomes checked, the dirty footer (Cancel/Save) appears, then click Cancel and assert the preview state is cleared and the radio reverts, and finally add a test where clicking Save clears the preview and calls the persistence function (mock the save handler) to assert it was invoked; reference AccessibilityPage, useThemePreview (or ThemePreviewProvider), and the Cancel/Save buttons when implementing these interaction assertions.
🤖 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/client/views/account/accessibility/AccessibilityPage.tsx`:
- Around line 76-80: The effect that writes previewTheme into the form must also
clear the preview on unmount: update the useEffect (the one that reads
previewTheme and calls setValue('themeAppearence', ...)) to return a cleanup
function that clears the preview state using the same API the Save/Cancel
handlers use (e.g. call the provider's clear/ reset function or
setPreviewTheme(undefined) from ThemePreviewProvider). Apply the same change to
the other similar effects noted (the ones at the other ranges) so navigating
away always removes the global preview.
---
Nitpick comments:
In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx`:
- Around line 44-129: The tests only assert static render; add interaction tests
that exercise the preview lifecycle via the real preview context or a stateful
useThemePreview mock: render AccessibilityPage wrapped with the actual
ThemePreviewProvider (or a test provider that uses useThemePreview), simulate
clicking a theme radio (e.g., the radio with label 'Theme_dark'), assert that
radio becomes checked, the dirty footer (Cancel/Save) appears, then click Cancel
and assert the preview state is cleared and the radio reverts, and finally add a
test where clicking Save clears the preview and calls the persistence function
(mock the save handler) to assert it was invoked; reference AccessibilityPage,
useThemePreview (or ThemePreviewProvider), and the Cancel/Save buttons when
implementing these interaction assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1e6dcd9-9381-45cc-8b8a-3976e2ac80d7
📒 Files selected for processing (11)
.changeset/wise-turkeys-matter.mdapps/meteor/client/providers/MeteorProvider.tsxapps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsxapps/meteor/client/views/account/accessibility/AccessibilityPage.tsxapps/meteor/client/views/root/MainLayout/MainLayoutStyleTags.tsxpackages/ui-client/src/hooks/index.tspackages/ui-client/src/hooks/useThemePreview.spec.tsxpackages/ui-client/src/hooks/useThemePreview.tspackages/ui-client/src/providers/ThemePreviewProvider.spec.tsxpackages/ui-client/src/providers/ThemePreviewProvider.tsxpackages/ui-client/src/providers/index.ts
✅ Files skipped from review due to trivial changes (5)
- .changeset/wise-turkeys-matter.md
- packages/ui-client/src/hooks/index.ts
- packages/ui-client/src/hooks/useThemePreview.spec.tsx
- packages/ui-client/src/providers/ThemePreviewProvider.spec.tsx
- packages/ui-client/src/providers/ThemePreviewProvider.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/meteor/client/providers/MeteorProvider.tsx
- packages/ui-client/src/providers/index.ts
- apps/meteor/client/views/root/MainLayout/MainLayoutStyleTags.tsx
- packages/ui-client/src/hooks/useThemePreview.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/client/views/account/accessibility/AccessibilityPage.tsxapps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
🧠 Learnings (24)
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.tsxapps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 Learning: 2026-03-06T18:10:23.330Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:23.330Z
Learning: In the RocketChat/Rocket.Chat `packages/gazzodown` package and more broadly, the HTML `<code>` element has an implicit ARIA role of `code` per WAI-ARIA 1.3, and `testing-library/dom` / jsdom supports it. Therefore, `screen.getByRole('code')` / `screen.findByRole('code')` correctly locates `<code>` elements without needing an explicit `role="code"` attribute. Do NOT flag `findByRole('code')` as invalid in future reviews.
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx
📚 Learning: 2026-03-04T14:16:49.202Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39304
File: packages/ui-contexts/src/ActionManagerContext.ts:26-26
Timestamp: 2026-03-04T14:16:49.202Z
Learning: In `packages/ui-contexts/src/ActionManagerContext.ts` (TypeScript, RocketChat/Rocket.Chat), the `disposeView` method in `IActionManager` uses an intentionally explicit union `UiKit.ModalView['id'] | UiKit.BannerView['viewId'] | UiKit.ContextualBarView['id']` to document which view types are accepted, even though all constituents resolve to the same primitive. The inline `// eslint-disable-next-line typescript-eslint/no-duplicate-type-constituents` comment is intentional and should not be flagged or removed.
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.tsxapps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 Learning: 2026-03-18T16:08:17.800Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39590
File: apps/meteor/client/views/omnichannel/contactInfo/EditContactInfo.tsx:97-99
Timestamp: 2026-03-18T16:08:17.800Z
Learning: In `apps/meteor/client/views/omnichannel/contactInfo/EditContactInfo.tsx`, `reValidateMode: 'onBlur'` is intentionally used (not 'onChange') because the `validateEmailFormat` and `validatePhone` functions are async and call the `checkExistenceEndpoint` API to check for duplicates. Using 'onChange' would trigger excessive network requests on every keystroke. The combination of `mode: 'onSubmit'` with `reValidateMode: 'onBlur'` is a deliberate design decision to minimize API calls while still providing revalidation feedback.
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx
📚 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} : Avoid code comments in the implementation
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.tsxapps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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 : Avoid using `page.locator()` in Playwright tests - always prefer semantic locators such as `page.getByRole()`, `page.getByLabel()`, `page.getByText()`, or `page.getByTitle()`
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 Learning: 2026-02-24T19:22:52.984Z
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:52.984Z
Learning: In RocketChat/Rocket.Chat Playwright e2e tests, prefer using translated text and ARIA roles (getByRole, getByText, etc.) over data-qa locators. If translation values change, update the corresponding test locators accordingly. Never prefer data-qa locators.
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 Learning: 2026-02-24T19:39:52.176Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/message.ts:7-7
Timestamp: 2026-02-24T19:39:52.176Z
Learning: In RocketChat/Rocket.Chat e2e tests, never prefer data-qa attributes to locate elements. Always prefer semantic locators (getByRole, getByLabel, getByText, getByTitle) and ARIA-based selectors over data-qa locators.
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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 `page.waitFor()` with specific conditions instead of hardcoded timeouts in Playwright tests
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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 : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 Learning: 2025-11-24T17:08:26.531Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/test-cases.mdc:0-0
Timestamp: 2025-11-24T17:08:26.531Z
Learning: Focus on comprehensive feature coverage and edge case validation in test design
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📚 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} : Store commonly used locators in variables/constants for reuse
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
🔇 Additional comments (1)
apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx (1)
108-109: Remove the inline implementation comments.They restate the branch logic and still violate the repo rule.
As per coding guidelines, "Avoid code comments in the implementation".
Comment on lines
+76
to
+80
| useEffect(() => { | ||
| if (previewTheme) { | ||
| setValue('themeAppearence', previewTheme as any, { shouldDirty: true }); | ||
| } | ||
| }, [previewTheme, setValue]); |
Contributor
There was a problem hiding this comment.
Clear the preview on page teardown, not only on Save/Cancel.
This wires preview state into the form, but previewTheme is only cleared in the explicit save/cancel paths. Because packages/ui-client/src/providers/ThemePreviewProvider.tsx:17-24 keeps that state in a root-level provider mounted by apps/meteor/client/providers/MeteorProvider.tsx:44-62, and apps/meteor/client/views/root/MainLayout/MainLayoutStyleTags.tsx:6-12 applies it globally, navigating away from Accessibility without clicking either button leaves an unsaved theme active across the rest of the app until refresh.
Also applies to: 86-89, 102-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx` around
lines 76 - 80, The effect that writes previewTheme into the form must also clear
the preview on unmount: update the useEffect (the one that reads previewTheme
and calls setValue('themeAppearence', ...)) to return a cleanup function that
clears the preview state using the same API the Save/Cancel handlers use (e.g.
call the provider's clear/ reset function or setPreviewTheme(undefined) from
ThemePreviewProvider). Apply the same change to the other similar effects noted
(the ones at the other ranges) so navigating away always removes the global
preview.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 99 out of 100 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+227
to
+232
| // updates user name whenever we receive a join event, because Matrix sends a new join event with the updated display name whenever a user changes their display name | ||
| if ('displayname' in content && content.displayname !== joiningUser.name) { | ||
| // whan a user changes the it's display name we receive a new join event for every room the user is in | ||
| // so we need to debounce the name update to avoid updating the name multiple times in a row | ||
| void updateUserNameDebounced(joiningUser._id, content.displayname || ''); | ||
| } |
There was a problem hiding this comment.
content.displayname || '' can end up setting a user’s name to an empty string if Matrix sends a join event with displayname missing/empty. That’s likely not intended and would overwrite existing names. Consider guarding on a non-empty displayname value before calling Users.setName, or falling back to the current name instead of ''.
Comment on lines
+952
to
+973
| // when a user changes their username, we need to send a new event for every room the user is a member | ||
| async updateUserName(user: IUser): Promise<void> { | ||
| const matrixUserId = userIdSchema.parse(`@${user.username}:${this.serverName}`); | ||
|
|
||
| const subs = await Subscriptions.findJoinedByUserId<Pick<ISubscription, 'rid'>>(user._id, { projection: { rid: 1 } }).toArray(); | ||
|
|
||
| const rooms = await Rooms.findFederatedByIds<Pick<IRoomNativeFederated, '_id' | 'federation' | 'federated'>>( | ||
| subs.map(({ rid }) => rid), | ||
| { projection: { _id: 1, federation: 1, federated: 1 } }, | ||
| ).toArray(); | ||
|
|
||
| await Promise.all( | ||
| rooms.map(async ({ federation }) => { | ||
| try { | ||
| await federationSDK.updateRoomMembership({ | ||
| roomId: roomIdSchema.parse(federation.mrid), | ||
| userId: matrixUserId, | ||
| membership: 'join', | ||
| content: { | ||
| displayname: user.name || user.username, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
updateUserName (and its preceding comment) talks about “username” changes, but the implementation actually updates Matrix displayname based on user.name || user.username. This is confusing and makes it easy to wire the callback to the wrong field in the future. Please rename this method/comment to reflect display-name propagation (or adjust the logic to actually handle username changes).
Comment on lines
+1
to
+65
| --- | ||
| '@rocket.chat/ui-client': minor | ||
| '@rocket.chat/meteor': minor | ||
| 'rocketchat-services': patch | ||
| '@rocket.chat/uikit-playground': patch | ||
| '@rocket.chat/account-service': patch | ||
| '@rocket.chat/authorization-service': patch | ||
| '@rocket.chat/ddp-streamer': patch | ||
| '@rocket.chat/omnichannel-transcript': patch | ||
| '@rocket.chat/presence-service': patch | ||
| '@rocket.chat/queue-worker': patch | ||
| '@rocket.chat/abac': patch | ||
| '@rocket.chat/license': patch | ||
| '@rocket.chat/media-calls': patch | ||
| '@rocket.chat/network-broker': patch | ||
| '@rocket.chat/omni-core-ee': patch | ||
| '@rocket.chat/omnichannel-services': patch | ||
| '@rocket.chat/pdf-worker': patch | ||
| '@rocket.chat/presence': patch | ||
| '@rocket.chat/account-utils': patch | ||
| '@rocket.chat/agenda': patch | ||
| '@rocket.chat/api-client': patch | ||
| '@rocket.chat/base64': patch | ||
| '@rocket.chat/cas-validate': patch | ||
| '@rocket.chat/cron': patch | ||
| '@rocket.chat/ddp-client': patch | ||
| '@rocket.chat/desktop-api': patch | ||
| '@rocket.chat/eslint-config': patch | ||
| '@rocket.chat/favicon': patch | ||
| '@rocket.chat/fuselage-ui-kit': patch | ||
| '@rocket.chat/gazzodown': patch | ||
| '@rocket.chat/http-router': patch | ||
| '@rocket.chat/instance-status': patch | ||
| '@rocket.chat/jest-presets': patch | ||
| '@rocket.chat/jwt': patch | ||
| '@rocket.chat/livechat': patch | ||
| '@rocket.chat/log-format': patch | ||
| '@rocket.chat/logger': patch | ||
| '@rocket.chat/media-signaling': patch | ||
| '@rocket.chat/message-parser': patch | ||
| '@rocket.chat/mock-providers': patch | ||
| '@rocket.chat/mongo-adapter': patch | ||
| '@rocket.chat/poplib': patch | ||
| '@rocket.chat/omni-core': patch | ||
| '@rocket.chat/password-policies': patch | ||
| '@rocket.chat/patch-injection': patch | ||
| '@rocket.chat/peggy-loader': patch | ||
| '@rocket.chat/random': patch | ||
| '@rocket.chat/release-action': patch | ||
| '@rocket.chat/release-changelog': patch | ||
| '@rocket.chat/server-cloud-communication': patch | ||
| '@rocket.chat/sha256': patch | ||
| '@rocket.chat/storybook-config': patch | ||
| '@rocket.chat/tools': patch | ||
| '@rocket.chat/tracing': patch | ||
| '@rocket.chat/tsconfig': patch | ||
| '@rocket.chat/ui-avatar': patch | ||
| '@rocket.chat/ui-composer': patch | ||
| '@rocket.chat/ui-kit': patch | ||
| '@rocket.chat/ui-video-conf': patch | ||
| '@rocket.chat/ui-voip': patch | ||
| '@rocket.chat/web-ui-registration': patch | ||
| --- | ||
|
|
||
| Add live theme preview feature to Accessibility settings, allowing users to preview Light, Dark, and High Contrast themes before saving preferences. |
There was a problem hiding this comment.
The PR title/description focus on live theme preview, but this changeset set (and the PR diff overall) also includes large, unrelated functional additions (ban/unban REST endpoints, federation membership/name sync, domain allowlist behavior, etc.). This makes review/rollback harder and doesn’t match the stated scope; consider splitting into separate PRs/changesets per feature area.
| if (!isBannedSubscription(subscription)) { | ||
| return true; | ||
| } | ||
| const deleteCount = await Subscriptions.removeByUserId(userToBeAdded._id); |
There was a problem hiding this comment.
In the banned-user re-invite path, this calls Subscriptions.removeByUserId(userToBeAdded._id), which removes all subscriptions for the user (across every room), not just the banned subscription for rid. This would unintentionally wipe the user’s room memberships when re-inviting/unbanning them in a single room. Use a room-scoped delete (e.g., remove the specific subscription document for this rid/user) instead.
Suggested change
| const deleteCount = await Subscriptions.removeByUserId(userToBeAdded._id); | |
| const deleteCount = await Subscriptions.removeByRoomIdAndUserId(rid, userToBeAdded._id); |
Comment on lines
76
to
+115
| @@ -85,6 +99,21 @@ const AccessibilityPage = () => { | |||
| setPreferencesAction.mutateAsync({ data }); | |||
| }; | |||
|
|
|||
| const handleCancel = () => { | |||
| clearPreviewTheme(); | |||
| reset(preferencesValues); | |||
| }; | |||
|
|
|||
| const handleThemeChange = (newTheme: string) => { | |||
| //Apply preview theme immediately for live preview | |||
| // If the selected theme matches the saved theme, clear the preview | |||
| if (newTheme === preferencesValues.themeAppearence) { | |||
| clearPreviewTheme(); | |||
| } else { | |||
| setPreviewTheme(newTheme as ThemePreference); | |||
| } | |||
| }; | |||
There was a problem hiding this comment.
setValue('themeAppearence', previewTheme as any, ...) and handleThemeChange(newTheme: string) both rely on unsafe casts. This defeats the point of importing ThemePreference and can allow invalid values into the form state. Prefer typing handleThemeChange to accept ThemePreference and passing previewTheme directly (or narrowing) so setValue stays type-safe.
Comment on lines
+106
to
+112
| const setDarkButton = screen.getByText('Set Dark (A)'); | ||
|
|
||
| setDarkButton.click(); | ||
|
|
||
| expect(screen.getByTestId('consumer-a-theme')).toHaveTextContent('dark'); | ||
| expect(screen.getByTestId('consumer-b-theme')).toHaveTextContent('dark'); | ||
| }); |
There was a problem hiding this comment.
This test triggers a state update via setDarkButton.click() without going through Testing Library’s userEvent helpers (and without awaiting). This can produce act warnings and flakiness. Prefer await userEvent.click(...) for consistency with the other tests in this file.
Comment on lines
+227
to
+231
| // updates user name whenever we receive a join event, because Matrix sends a new join event with the updated display name whenever a user changes their display name | ||
| if ('displayname' in content && content.displayname !== joiningUser.name) { | ||
| // whan a user changes the it's display name we receive a new join event for every room the user is in | ||
| // so we need to debounce the name update to avoid updating the name multiple times in a row | ||
| void updateUserNameDebounced(joiningUser._id, content.displayname || ''); |
There was a problem hiding this comment.
The comment has typos/grammar issues (e.g., "whan" / "the it's"). Since this is developer-facing documentation for a subtle debounce behavior, please correct the wording for clarity.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+44
to
+129
| describe('AccessibilityPage - Theme Preview', () => { | ||
| beforeEach(() => { | ||
| queryClient.clear(); | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('should render theme section with radio buttons', () => { | ||
| render( | ||
| <QueryClientProvider client={queryClient}> | ||
| <AccessibilityPage /> | ||
| </QueryClientProvider> | ||
| ); | ||
|
|
||
|
|
||
| expect(screen.getByText('Theme_light')).toBeInTheDocument(); | ||
| expect(screen.getByText('Theme_dark')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('should have theme radio buttons for different options', () => { | ||
| render( | ||
| <QueryClientProvider client={queryClient}> | ||
| <AccessibilityPage /> | ||
| </QueryClientProvider> | ||
| ); | ||
|
|
||
| const radios = screen.getAllByRole('radio'); | ||
| expect(radios.length).toBeGreaterThan(0); | ||
| }); | ||
|
|
||
| it('should render cancel and save buttons', () => { | ||
| render( | ||
| <QueryClientProvider client={queryClient}> | ||
| <AccessibilityPage /> | ||
| </QueryClientProvider> | ||
| ); | ||
|
|
||
| const buttons = screen.getAllByRole('button'); | ||
|
|
||
| expect(buttons.length).toBeGreaterThanOrEqual(2); | ||
| }); | ||
|
|
||
| it('should display font size select field with aria-label', () => { | ||
| render( | ||
| <QueryClientProvider client={queryClient}> | ||
| <AccessibilityPage /> | ||
| </QueryClientProvider> | ||
| ); | ||
|
|
||
| const fontSizeSelect = screen.getByLabelText('Font_size'); | ||
| expect(fontSizeSelect).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('should display clock mode select field with aria-label', () => { | ||
| render( | ||
| <QueryClientProvider client={queryClient}> | ||
| <AccessibilityPage /> | ||
| </QueryClientProvider> | ||
| ); | ||
|
|
||
| const clockModeSelect = screen.getByLabelText('Message_TimeFormat'); | ||
| expect(clockModeSelect).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('should render all theme option labels', () => { | ||
| render( | ||
| <QueryClientProvider client={queryClient}> | ||
| <AccessibilityPage /> | ||
| </QueryClientProvider> | ||
| ); | ||
|
|
||
| expect(screen.getByText('Theme_light')).toBeInTheDocument(); | ||
| expect(screen.getByText('Theme_dark')).toBeInTheDocument(); | ||
| expect(screen.getByText('Theme_high_contrast')).toBeInTheDocument(); | ||
| expect(screen.getByText('Theme_match_system')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('should have learn more section with accessibility links', () => { | ||
| render( | ||
| <QueryClientProvider client={queryClient}> | ||
| <AccessibilityPage /> | ||
| </QueryClientProvider> | ||
| ); | ||
|
|
||
| expect(screen.getByText('Accessibility_statement')).toBeInTheDocument(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The added tests currently only verify that the page renders basic controls, but they don't exercise the new live theme preview behavior (e.g., selecting a theme sets preview state, footer visibility toggles when selecting the saved theme again, Cancel clears preview, Save clears preview after success). Adding interaction tests around those flows would prevent regressions in the core feature this PR introduces.
Comment on lines
+76
to
+80
| useEffect(() => { | ||
| if (previewTheme) { | ||
| setValue('themeAppearence', previewTheme as any, { shouldDirty: true }); | ||
| } | ||
| }, [previewTheme, setValue]); |
There was a problem hiding this comment.
Because the form's themeAppearence value is only synchronized from previewTheme in a useEffect, there is a brief window on initial mount (or when returning to this page with an existing preview active) where the footer is shown and Save is enabled (!!previewTheme), but the form still holds the saved theme value. Submitting during that window could send no theme change (or the wrong value). Consider initializing the form field from previewTheme earlier (e.g., include it in initial form values / reset on mount, or use useLayoutEffect) so the form state is in sync before Save can be clicked.
Comment on lines
+69
to
+80
| setValue, | ||
| } = useForm({ | ||
| defaultValues: preferencesValues, | ||
| }); | ||
|
|
||
| const currentData = watch(); | ||
|
|
||
| useEffect(() => { | ||
| if (previewTheme) { | ||
| setValue('themeAppearence', previewTheme as any, { shouldDirty: true }); | ||
| } | ||
| }, [previewTheme, setValue]); |
There was a problem hiding this comment.
setValue('themeAppearence', previewTheme as any, ...) introduces an unnecessary any cast and the current useForm call is untyped, which weakens type-safety for this form. Consider typing the form as useForm<AccessibilityPreferencesData>({ defaultValues: preferencesValues }) and then passing previewTheme directly (and/or typing handleThemeChange as (newTheme: ThemePreference)), so invalid theme values are caught at compile time.
cloudblimp
force-pushed
the
feat/theme-live-preview
branch
from
312ca6f to
14777c7
Compare
coderabbitai
Bot
removed
the
type: feature
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/ui-client/src/hooks/useThemePreview.ts (1)
8-9: Remove inline lint-disable comment by makingNOOPnon-empty.Line 8 can be avoided by returning
undefinedexplicitly from the function body.Suggested diff
-// eslint-disable-next-line `@typescript-eslint/no-empty-function` -const NOOP = (): void => {}; +const NOOP = (): void => undefined;As per coding guidelines: "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-client/src/hooks/useThemePreview.ts` around lines 8 - 9, Replace the eslint-disable and empty-body NOOP with a tiny non-empty implementation: update the NOOP function (const NOOP = (): void => {}) to explicitly return undefined from its body (e.g., const NOOP = (): void => { return undefined; }) so the inline lint-disable comment can be removed and the function is no longer empty; keep the symbol name NOOP unchanged so callers still refer to the same identifier.packages/ui-client/src/hooks/useThemePreview.spec.tsx (1)
42-73: Add tests forhigh-contrastandautoresolution paths.The hook has explicit logic for
high-contrastandauto(useDarkMode(undefined)), but these branches are currently untested.Suggested diff
+ it('should resolve high-contrast when preview theme is high-contrast', () => { + const wrapper = ({ children }: { children: ReactNode }) => <ThemePreviewProvider>{children}</ThemePreviewProvider>; + const { result } = renderHook(() => useThemePreview(), { wrapper }); + + act(() => { + result.current.setPreviewTheme('high-contrast'); + }); + + expect(result.current.resolvedPreviewTheme).toBe('high-contrast'); + }); + + it('should pass undefined to useDarkMode when theme mode is auto', () => { + mockUseThemeMode.mockReturnValue(['auto', jest.fn(), 'light']); + renderHook(() => useThemePreview()); + expect(mockUseDarkMode).toHaveBeenCalledWith(undefined); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-client/src/hooks/useThemePreview.spec.tsx` around lines 42 - 73, Add two unit tests to cover the untested branches in useThemePreview: one that sets mockUseDarkMode.mockReturnValue('high-contrast') and asserts that useThemePreview().resolvedPreviewTheme is 'high-contrast', and another that sets mockUseDarkMode.mockReturnValue(undefined) (the "auto" path) and asserts that useThemePreview().resolvedPreviewTheme reflects the auto branch (i.e., the hook's auto resolution value); wrap both tests with the same ThemePreviewProvider wrapper and use renderHook(() => useThemePreview()) to get result (referencing useThemePreview, ThemePreviewProvider, mockUseDarkMode, and resolvedPreviewTheme).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ui-client/src/hooks/useThemePreview.spec.tsx`:
- Around line 42-73: Add two unit tests to cover the untested branches in
useThemePreview: one that sets mockUseDarkMode.mockReturnValue('high-contrast')
and asserts that useThemePreview().resolvedPreviewTheme is 'high-contrast', and
another that sets mockUseDarkMode.mockReturnValue(undefined) (the "auto" path)
and asserts that useThemePreview().resolvedPreviewTheme reflects the auto branch
(i.e., the hook's auto resolution value); wrap both tests with the same
ThemePreviewProvider wrapper and use renderHook(() => useThemePreview()) to get
result (referencing useThemePreview, ThemePreviewProvider, mockUseDarkMode, and
resolvedPreviewTheme).
In `@packages/ui-client/src/hooks/useThemePreview.ts`:
- Around line 8-9: Replace the eslint-disable and empty-body NOOP with a tiny
non-empty implementation: update the NOOP function (const NOOP = (): void => {})
to explicitly return undefined from its body (e.g., const NOOP = (): void => {
return undefined; }) so the inline lint-disable comment can be removed and the
function is no longer empty; keep the symbol name NOOP unchanged so callers
still refer to the same identifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e98cf923-ed1e-4755-a9d9-624e0b736e3a
📒 Files selected for processing (11)
.changeset/wise-turkeys-matter.mdapps/meteor/client/providers/MeteorProvider.tsxapps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsxapps/meteor/client/views/account/accessibility/AccessibilityPage.tsxapps/meteor/client/views/root/MainLayout/MainLayoutStyleTags.tsxpackages/ui-client/src/hooks/index.tspackages/ui-client/src/hooks/useThemePreview.spec.tsxpackages/ui-client/src/hooks/useThemePreview.tspackages/ui-client/src/providers/ThemePreviewProvider.spec.tsxpackages/ui-client/src/providers/ThemePreviewProvider.tsxpackages/ui-client/src/providers/index.ts
✅ Files skipped from review due to trivial changes (5)
- packages/ui-client/src/hooks/index.ts
- packages/ui-client/src/providers/index.ts
- .changeset/wise-turkeys-matter.md
- packages/ui-client/src/providers/ThemePreviewProvider.spec.tsx
- packages/ui-client/src/providers/ThemePreviewProvider.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/meteor/client/providers/MeteorProvider.tsx
- apps/meteor/client/views/root/MainLayout/MainLayoutStyleTags.tsx
- apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx
- apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
📜 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:
packages/ui-client/src/hooks/useThemePreview.spec.tsxpackages/ui-client/src/hooks/useThemePreview.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 : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
packages/ui-client/src/hooks/useThemePreview.spec.tsx
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
packages/ui-client/src/hooks/useThemePreview.spec.tsx
📚 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:
packages/ui-client/src/hooks/useThemePreview.spec.tsx
📚 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:
packages/ui-client/src/hooks/useThemePreview.spec.tsx
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
packages/ui-client/src/hooks/useThemePreview.spec.tsx
📚 Learning: 2026-01-19T18:08:05.314Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38169
File: packages/ui-client/src/hooks/useGoToDirectMessage.ts:20-27
Timestamp: 2026-01-19T18:08:05.314Z
Learning: Rule of Hooks: In React, hooks (including custom hooks like useGoToDirectMessage and useUserSubscriptionByName) must be called unconditionally and in the same order on every render. Do not conditionally call hooks based on values; instead, derive safe inputs if a value may be undefined (e.g., pass an empty string or undefined) and handle variations in logic outside the hook invocation. This preserves hook order and avoids violations. Apply this guideline to all files under packages/ui-client/src/hooks where hooks are used.
Applied to files:
packages/ui-client/src/hooks/useThemePreview.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/ui-client/src/hooks/useThemePreview.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/ui-client/src/hooks/useThemePreview.ts
🔇 Additional comments (2)
packages/ui-client/src/hooks/useThemePreview.ts (1)
17-38: Hook usage and fallback flow look correct.
useContext,useThemeMode, anduseDarkModeare invoked unconditionally, and the provider-absent fallback is safely handled.Based on learnings: "Rule of Hooks: In React, hooks ... must be called unconditionally and in the same order on every render."
packages/ui-client/src/hooks/useThemePreview.spec.tsx (1)
24-40: Good baseline safety coverage for provider-absent usage.These cases validate the non-provider fallback contract and ensure NOOP handlers are safe to call.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 99 out of 100 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+95
to
+107
| const subscription = await Subscriptions.findOneByRoomIdAndUserId(rid, userToBeAdded._id); | ||
| if (subscription) { | ||
| if (!isBannedSubscription(subscription)) { | ||
| return true; | ||
| } | ||
| const deleteCount = await Subscriptions.removeByUserId(userToBeAdded._id); | ||
| if (!deleteCount) { | ||
| return true; | ||
| } | ||
|
|
||
| if (!skipSystemMessage && inviter) { | ||
| await Message.saveSystemMessage('user-unbanned', rid, userToBeAdded.username!, inviter, { ts: now }); | ||
| } |
There was a problem hiding this comment.
In the banned-subscription re-invite path, this removes all subscriptions for the user (Subscriptions.removeByUserId(userToBeAdded._id)), which can unintentionally drop the user from every room (and adjust room counts / __rooms) when they’re only being unbanned for a single room. This should delete only the subscription for the current room (e.g., remove by subscription._id or by { rid, 'u._id' }), and avoid global user subscription cleanup here.
cloudblimp
force-pushed
the
feat/theme-live-preview
branch
from
14777c7 to
8cf9c90
Compare
coderabbitai
Bot
added
the
type: feature
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx (1)
94-104:⚠️ Potential issue | 🟠 MajorDon't reset away the pending theme after an error.
After a failed save,
reset(currentData)clearsdirtyFieldsbutpreviewThemestill keeps the footer actionable. The next submit then builds{}fromgetDirtyFields(formData, dirtyFields), so the previewed theme can't be persisted unless the user reselects it.Also applies to: 107-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx` around lines 94 - 104, The onSettled handler in setPreferencesAction unconditionally calls reset(currentData) which clears dirtyFields even after a failed save and prevents previewTheme from being persisted; change the logic in setPreferencesAction (onSettled / onError handling) so reset(currentData) is only invoked when there is no error (i.e., onSuccess or onSettled when the error arg is falsy), leaving dirtyFields and previewTheme intact after failures so getDirtyFields(formData, dirtyFields) still returns the pending theme for the next submit; update the onSettled signature usage accordingly and ensure onError does not call reset.
🧹 Nitpick comments (2)
packages/ui-client/src/hooks/useThemePreview.ts (1)
8-9: Drop the inline lint suppression forNOOP.
const NOOP = (): void => undefined;keeps the fallback stable without adding an implementation comment.♻️ Proposed cleanup
-// eslint-disable-next-line `@typescript-eslint/no-empty-function` -const NOOP = (): void => {}; +const NOOP = (): void => undefined;As per coding guidelines, "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-client/src/hooks/useThemePreview.ts` around lines 8 - 9, Replace the NOOP declaration that currently disables ESLint with a silent-return implementation: change the constant NOOP to return undefined instead of using an inline eslint-disable comment so the fallback remains stable and the lint suppression comment is removed; update the NOOP constant (used by useThemePreview) to be declared as a function returning undefined (e.g., NOOP = (): void => undefined) and remove the "// eslint-disable-next-line `@typescript-eslint/no-empty-function`" line.apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx (1)
62-69: Remove the explanatory inline comments.These branches are already readable from the code. Clear names are enough here and keep the implementation aligned with repo style.
As per coding guidelines, "Avoid code comments in the implementation".
Also applies to: 118-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx` around lines 62 - 69, Remove the inline explanatory comments inside the useMemo that computes defaultFormValues (and the similar comment at lines 118-119); leave the code as-is but delete the comment lines describing why previewTheme is merged and the "Compute default values..." header so the logic using useMemo, preferencesValues, previewTheme, and the themeAppearence property remains unchanged but without the explanatory inline comments to conform to the repository style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx`:
- Around line 94-104: The onSettled handler in setPreferencesAction
unconditionally calls reset(currentData) which clears dirtyFields even after a
failed save and prevents previewTheme from being persisted; change the logic in
setPreferencesAction (onSettled / onError handling) so reset(currentData) is
only invoked when there is no error (i.e., onSuccess or onSettled when the error
arg is falsy), leaving dirtyFields and previewTheme intact after failures so
getDirtyFields(formData, dirtyFields) still returns the pending theme for the
next submit; update the onSettled signature usage accordingly and ensure onError
does not call reset.
---
Nitpick comments:
In `@apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx`:
- Around line 62-69: Remove the inline explanatory comments inside the useMemo
that computes defaultFormValues (and the similar comment at lines 118-119);
leave the code as-is but delete the comment lines describing why previewTheme is
merged and the "Compute default values..." header so the logic using useMemo,
preferencesValues, previewTheme, and the themeAppearence property remains
unchanged but without the explanatory inline comments to conform to the
repository style.
In `@packages/ui-client/src/hooks/useThemePreview.ts`:
- Around line 8-9: Replace the NOOP declaration that currently disables ESLint
with a silent-return implementation: change the constant NOOP to return
undefined instead of using an inline eslint-disable comment so the fallback
remains stable and the lint suppression comment is removed; update the NOOP
constant (used by useThemePreview) to be declared as a function returning
undefined (e.g., NOOP = (): void => undefined) and remove the "//
eslint-disable-next-line `@typescript-eslint/no-empty-function`" line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31254f0d-9d18-484a-93b7-b6edbd6805ab
📒 Files selected for processing (7)
.changeset/wise-turkeys-matter.mdapps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsxapps/meteor/client/views/account/accessibility/AccessibilityPage.tsxpackages/ui-client/src/hooks/useThemePreview.spec.tsxpackages/ui-client/src/hooks/useThemePreview.tspackages/ui-client/src/providers/ThemePreviewProvider.spec.tsxpackages/ui-client/src/providers/ThemePreviewProvider.tsx
✅ Files skipped from review due to trivial changes (4)
- .changeset/wise-turkeys-matter.md
- apps/meteor/client/views/account/accessibility/AccessibilityPage.spec.tsx
- packages/ui-client/src/providers/ThemePreviewProvider.spec.tsx
- packages/ui-client/src/providers/ThemePreviewProvider.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui-client/src/hooks/useThemePreview.spec.tsx
📜 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:
packages/ui-client/src/hooks/useThemePreview.tsapps/meteor/client/views/account/accessibility/AccessibilityPage.tsx
🧠 Learnings (7)
📚 Learning: 2026-01-19T18:08:05.314Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38169
File: packages/ui-client/src/hooks/useGoToDirectMessage.ts:20-27
Timestamp: 2026-01-19T18:08:05.314Z
Learning: Rule of Hooks: In React, hooks (including custom hooks like useGoToDirectMessage and useUserSubscriptionByName) must be called unconditionally and in the same order on every render. Do not conditionally call hooks based on values; instead, derive safe inputs if a value may be undefined (e.g., pass an empty string or undefined) and handle variations in logic outside the hook invocation. This preserves hook order and avoids violations. Apply this guideline to all files under packages/ui-client/src/hooks where hooks are used.
Applied to files:
packages/ui-client/src/hooks/useThemePreview.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/ui-client/src/hooks/useThemePreview.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/ui-client/src/hooks/useThemePreview.ts
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx
📚 Learning: 2026-03-06T18:10:23.330Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:23.330Z
Learning: In the RocketChat/Rocket.Chat `packages/gazzodown` package and more broadly, the HTML `<code>` element has an implicit ARIA role of `code` per WAI-ARIA 1.3, and `testing-library/dom` / jsdom supports it. Therefore, `screen.getByRole('code')` / `screen.findByRole('code')` correctly locates `<code>` elements without needing an explicit `role="code"` attribute. Do NOT flag `findByRole('code')` as invalid in future reviews.
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx
📚 Learning: 2026-03-04T14:16:49.202Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39304
File: packages/ui-contexts/src/ActionManagerContext.ts:26-26
Timestamp: 2026-03-04T14:16:49.202Z
Learning: In `packages/ui-contexts/src/ActionManagerContext.ts` (TypeScript, RocketChat/Rocket.Chat), the `disposeView` method in `IActionManager` uses an intentionally explicit union `UiKit.ModalView['id'] | UiKit.BannerView['viewId'] | UiKit.ContextualBarView['id']` to document which view types are accepted, even though all constituents resolve to the same primitive. The inline `// eslint-disable-next-line typescript-eslint/no-duplicate-type-constituents` comment is intentional and should not be flagged or removed.
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx
📚 Learning: 2026-03-18T16:08:17.800Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39590
File: apps/meteor/client/views/omnichannel/contactInfo/EditContactInfo.tsx:97-99
Timestamp: 2026-03-18T16:08:17.800Z
Learning: In `apps/meteor/client/views/omnichannel/contactInfo/EditContactInfo.tsx`, `reValidateMode: 'onBlur'` is intentionally used (not 'onChange') because the `validateEmailFormat` and `validatePhone` functions are async and call the `checkExistenceEndpoint` API to check for duplicates. Using 'onChange' would trigger excessive network requests on every keystroke. The combination of `mode: 'onSubmit'` with `reValidateMode: 'onBlur'` is a deliberate design decision to minimize API calls while still providing revalidation feedback.
Applied to files:
apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx
🔇 Additional comments (2)
packages/ui-client/src/hooks/useThemePreview.ts (1)
17-31: Theme precedence and hook ordering look good.
previewThemecleanly overrides the saved mode, and both hooks stay unconditional.apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx (1)
40-40:⚠️ Potential issue | 🟠 MajorVerify that
ThemePreviewProvideris mounted in the app render tree.The
useThemePreview()hook inAccessibilityPage.tsx:40depends onThemePreviewProvider, but it is not mounted anywhere in the production code. When the hook is called outside the provider context,setPreviewThemeandclearPreviewThemedegrade to NOOPs (seepackages/ui-client/src/hooks/useThemePreview.ts:33-38). This means the live theme preview feature will not work—the form updates but the preview never applies. AddThemePreviewProvidertoMeteorProvider.tsxor another appropriate parent component in the render tree.⛔ Skipped due to learnings
Learnt from: juliajforesti Repo: RocketChat/Rocket.Chat PR: 38493 File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82 Timestamp: 2026-02-24T19:36:55.089Z Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Contributor
|
Caution Review failedAn error occurred during the review process. Please try again later. 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:
WalkthroughThis PR introduces live theme preview functionality to Accessibility & Appearance Settings, enabling users to preview Light, Dark, and High Contrast themes in real-time before saving their selection. A new context-based provider and hook manage preview state, while the AccessibilityPage integrates preview functionality with form state management. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant AccessibilityPage
participant ThemePreviewProvider
participant useThemePreview
participant useThemeMode
User->>AccessibilityPage: Select theme (Dark)
activate AccessibilityPage
AccessibilityPage->>useThemePreview: setPreviewTheme('dark')
activate useThemePreview
useThemePreview->>ThemePreviewProvider: Update context state
activate ThemePreviewProvider
ThemePreviewProvider-->>useThemePreview: previewTheme = 'dark'
deactivate ThemePreviewProvider
useThemePreview->>useThemeMode: Get current theme mode
activate useThemeMode
useThemeMode-->>useThemePreview: saved theme mode
deactivate useThemeMode
useThemePreview->>useThemePreview: Resolve preview theme<br/>(prioritize 'dark' over saved)
useThemePreview-->>AccessibilityPage: resolvedPreviewTheme = 'dark'
deactivate useThemePreview
AccessibilityPage->>AccessibilityPage: Update form & UI state
deactivate AccessibilityPage
AccessibilityPage-->>User: Theme preview displayed immediately
User->>AccessibilityPage: Click "Save Changes"
activate AccessibilityPage
AccessibilityPage->>AccessibilityPage: Persist theme preference
AccessibilityPage->>useThemePreview: clearPreviewTheme()
useThemePreview->>ThemePreviewProvider: Reset preview to undefined
AccessibilityPage-->>User: Changes saved, preview cleared
deactivate AccessibilityPage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
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
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
Introduce a live preview mechanism that applies theme CSS variables immediately upon selection.
Benefits
Technical Approach:
Sequence Diagram:
From the Sequence Diagram, following are the optimization highlights:
Three layer architecture
Short explaination: Provider stores -> Hook exposes -> Component uses
Issue(s)
Closes #39826
Steps to test or reproduce
Testing Live Theme Preview
Navigate to Accessibility & Appearance settings
Test theme preview functionality
Test smart revert detection
Test save functionality
Test cancel functionality
Affected Screens
apps/meteor/client/views/account/accessibility/AccessibilityPage.tsx)Proposed Behavior (Live Preview)
theme-preview-implementation.mov
Further comments
We could also consider removing the 'Save/Cancel' requirement entirely and persisting the theme choice immediately upon selection. While this aligns with 'Instant-save' trends in modern OS settings, I have prioritized the Live preview + Manual save approach for this PR to:
Summary by CodeRabbit
New Features
Tests
Chores