From 9d67acb8cc62e4bc44be0267757d0f17629f7217 Mon Sep 17 00:00:00 2001 From: Rahim Rahman Date: Fri, 18 Jul 2025 06:48:29 -0600 Subject: [PATCH] fix(MM-63596): custom theme cannot be reselected (#8960) * initialTheme should not be a dependent of useCallback since its a ref * a few cleanup on test to make it more clearer * add a bit more description * fix(MM-64710): changing themes repeatedly in rapid succession cause the theme to not register (#8980) * add more test, doubleTap prevention test * changes based on comments * remove the use of currentTheme * consolidate handleSelectTheme with setThemePreference * add comment to explain why we're storing customTheme in a state --- .../display_theme/display_theme.test.tsx | 290 ++++++++++++++++++ .../settings/display_theme/display_theme.tsx | 43 ++- 2 files changed, 308 insertions(+), 25 deletions(-) create mode 100644 app/screens/settings/display_theme/display_theme.test.tsx diff --git a/app/screens/settings/display_theme/display_theme.test.tsx b/app/screens/settings/display_theme/display_theme.test.tsx new file mode 100644 index 000000000..d02801c0e --- /dev/null +++ b/app/screens/settings/display_theme/display_theme.test.tsx @@ -0,0 +1,290 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {fireEvent, render, screen, waitFor} from '@testing-library/react-native'; +import React from 'react'; +import {BackHandler} from 'react-native'; + +import {savePreference} from '@actions/remote/preference'; +import {Preferences} from '@constants'; +import {useTheme} from '@context/theme'; +import {popTopScreen} from '@screens/navigation'; +import NavigationStore from '@store/navigation_store'; +import {renderWithIntl} from '@test/intl-test-helper'; + +import DisplayTheme from './display_theme'; + +import type {AvailableScreens} from '@typings/screens/navigation'; + +jest.mock('@screens/navigation'); +jest.mock('@context/theme', () => ({ + useTheme: jest.fn(), +})); +jest.mock('@actions/remote/preference'); +jest.mock('@store/navigation_store'); + +const displayThemeOtherProps = { + componentId: 'DisplayTheme' as AvailableScreens, + currentTeamId: '1', + currentUserId: '1', +}; + +describe('DisplayTheme', () => { + + beforeEach(() => { + jest.clearAllMocks(); + jest.mocked(useTheme).mockImplementation(() => ({...Preferences.THEMES.denim, type: 'Denim'})); + }); + + it('should render with a few themes, denim selected', () => { + render( + ); + + expect(screen.getByTestId('theme_display_settings.denim.option')).toBeTruthy(); + expect(screen.getByTestId('theme_display_settings.denim.option.selected')).toBeTruthy(); + + expect(screen.getByTestId('theme_display_settings.sapphire.option')).toBeTruthy(); + expect(screen.queryByTestId('theme_display_settings.sapphire.option.selected')).toBeFalsy(); + }); + + it('should render with custom theme, current theme is custom', () => { + jest.mocked(useTheme).mockImplementation(() => ({...Preferences.THEMES.denim, type: 'custom'})); + + renderWithIntl( + ); + + expect(screen.getByTestId('theme_display_settings.custom.option')).toBeTruthy(); + expect(screen.getByTestId('theme_display_settings.custom.option.selected')).toBeTruthy(); + }); + + it('should render with custom theme (default) and user change to denim (non-custom)', async () => { + jest.mocked(useTheme).mockImplementation(() => ({...Preferences.THEMES.denim, type: 'custom'})); + + renderWithIntl( + ); + + expect(screen.getByTestId('theme_display_settings.custom.option')).toBeTruthy(); + expect(screen.getByTestId('theme_display_settings.custom.option.selected')).toBeTruthy(); + + const denimTile = screen.getByTestId('theme_display_settings.denim.option'); + + fireEvent.press(denimTile); + + await waitFor(() => { + expect(savePreference).toHaveBeenCalledWith( + expect.any(String), + expect.arrayContaining([ + expect.objectContaining({ + category: 'theme', + value: expect.stringContaining('"type":"Denim"'), + }), + ]), + ); + expect(savePreference).toHaveBeenCalledTimes(1); + }); + + // since we're mocking useTheme and savePreference, savePreference will post changes to the backend API, and upon success, + // it will update the `theme` preference via useTheme hook. + jest.mocked(useTheme).mockImplementation(() => ({...Preferences.THEMES.denim, type: 'Denim'})); + + // clearing the savePreference mock to show that it will not be called again after re-rendering the component + jest.mocked(savePreference).mockClear(); + + screen.rerender( + , + ); + + expect(savePreference).toHaveBeenCalledTimes(0); + + expect(screen.getByTestId('theme_display_settings.denim.option.selected')).toBeTruthy(); + }); + + it('should render only with custom theme, it gets de-selected, and then user re-selects it', async () => { + jest.mocked(useTheme).mockImplementation(() => ({...Preferences.THEMES.denim, type: 'custom'})); + + renderWithIntl( + ); + + jest.mocked(useTheme).mockImplementation(() => ({...Preferences.THEMES.denim, type: 'Denim'})); + + screen.rerender( + ); + + const customTile = screen.getByTestId('theme_display_settings.custom.option'); + + fireEvent.press(customTile); + + await waitFor(() => { + expect(savePreference).toHaveBeenCalledWith( + expect.any(String), + expect.arrayContaining([ + expect.objectContaining({ + category: 'theme', + value: expect.stringContaining('"type":"custom"'), + }), + ]), + ); + }); + }); + + it('should render denim, then a different client set the theme to custom, and this client should render custom and automatically switch to it', () => { + renderWithIntl( + ); + + expect(screen.queryByTestId('theme_display_settings.custom.option.selected')).toBeFalsy(); + + jest.mocked(useTheme).mockImplementation(() => ({...Preferences.THEMES.denim, type: 'custom'})); + + screen.rerender( + ); + + expect(screen.getByTestId('theme_display_settings.custom.option.selected')).toBeTruthy(); + }); + + it('should not call popTopScreen (closes the screen) when changing theme', async () => { + renderWithIntl( + , + ); + + const sapphireTile = screen.getByTestId('theme_display_settings.sapphire.option'); + + fireEvent.press(sapphireTile); + + jest.mocked(useTheme).mockImplementation(() => ({...Preferences.THEMES.sapphire, type: 'Sapphire'})); + + screen.rerender( + , + ); + + await waitFor(() => { + expect(screen.getByTestId('theme_display_settings.sapphire.option.selected')).toBeTruthy(); + }); + + expect(popTopScreen).toHaveBeenCalledTimes(0); + }); + + it('should call popTopScreen when Android back button is pressed', () => { + (NavigationStore.getVisibleScreen as jest.Mock).mockReturnValue('DisplayTheme'); + const androidBackButtonHandler = jest.spyOn(BackHandler, 'addEventListener'); + + renderWithIntl( + , + ); + + // simulate Android back button press + androidBackButtonHandler.mock.calls[0][1](); + + expect(popTopScreen).toHaveBeenCalledTimes(1); + }); + + it('should allow user to select two different themes using normal interaction', async () => { + jest.useFakeTimers(); + + const numOfSavePreferenceCalls = 2; + renderWithIntl( + , + ); + + const sapphireTile = screen.getByTestId('theme_display_settings.sapphire.option'); + + fireEvent.press(sapphireTile); + + jest.advanceTimersByTime(750); + jest.useRealTimers(); + + await waitFor(() => { + expect(savePreference).toHaveBeenCalledWith( + expect.any(String), + expect.arrayContaining([ + expect.objectContaining({ + category: 'theme', + value: expect.stringContaining('"type":"Sapphire"'), + }), + ]), + ); + expect(savePreference).toHaveBeenCalledTimes(1); + }); + + jest.useFakeTimers(); + jest.advanceTimersByTime(750); + + const denimTile = screen.getByTestId('theme_display_settings.denim.option'); + + fireEvent.press(denimTile); + + // firing denimTile will not cause the savePreference to be called again since we have the prevent double tap + expect(savePreference).toHaveBeenCalledTimes(numOfSavePreferenceCalls); + + jest.useRealTimers(); + }); + + it('should not allow user to select a theme rapidly', async () => { + const numOfSavePreferenceCalls = 1; + renderWithIntl( + , + ); + + const sapphireTile = screen.getByTestId('theme_display_settings.sapphire.option'); + + fireEvent.press(sapphireTile); + + await waitFor(() => { + expect(savePreference).toHaveBeenCalledWith( + expect.any(String), + expect.arrayContaining([ + expect.objectContaining({ + category: 'theme', + value: expect.stringContaining('"type":"Sapphire"'), + }), + ]), + ); + expect(savePreference).toHaveBeenCalledTimes(numOfSavePreferenceCalls); + }); + + const denimTile = screen.getByTestId('theme_display_settings.denim.option'); + + fireEvent.press(denimTile); + + // firing denimTile will not cause the savePreference to be called again since we have the prevent double tap + expect(savePreference).toHaveBeenCalledTimes(numOfSavePreferenceCalls); + }); +}); diff --git a/app/screens/settings/display_theme/display_theme.tsx b/app/screens/settings/display_theme/display_theme.tsx index 1627ea8c9..b95dcbd67 100644 --- a/app/screens/settings/display_theme/display_theme.tsx +++ b/app/screens/settings/display_theme/display_theme.tsx @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import React, {useCallback, useMemo, useState} from 'react'; +import React, {useCallback, useState} from 'react'; import {savePreference} from '@actions/remote/preference'; import SettingContainer from '@components/settings/container'; @@ -10,6 +10,7 @@ import {useServerUrl} from '@context/server'; import {useTheme} from '@context/theme'; import useAndroidHardwareBackHandler from '@hooks/android_back_handler'; import useDidUpdate from '@hooks/did_update'; +import {usePreventDoubleTap} from '@hooks/utils'; import {popTopScreen} from '@screens/navigation'; import CustomTheme from './custom_theme'; @@ -26,14 +27,13 @@ type DisplayThemeProps = { const DisplayTheme = ({allowedThemeKeys, componentId, currentTeamId, currentUserId}: DisplayThemeProps) => { const serverUrl = useServerUrl(); const theme = useTheme(); - const initialTheme = useMemo(() => theme, [/* dependency array should remain empty */]); - const [newTheme, setNewTheme] = useState(undefined); + const [customTheme, setCustomTheme] = useState(theme.type?.toLowerCase() === 'custom' ? theme : undefined); const close = () => popTopScreen(componentId); - const setThemePreference = useCallback(() => { - const allowedTheme = allowedThemeKeys.find((tk) => tk === newTheme); - const themeJson = Preferences.THEMES[allowedTheme as ThemeKey] || initialTheme; + const handleThemeChange = usePreventDoubleTap(useCallback(async (themeSelected: string) => { + const allowedTheme = allowedThemeKeys.find((tk) => tk === themeSelected); + const themeJson = Preferences.THEMES[allowedTheme as ThemeKey] || customTheme; const pref: PreferenceType = { category: Preferences.CATEGORIES.THEME, @@ -41,37 +41,30 @@ const DisplayTheme = ({allowedThemeKeys, componentId, currentTeamId, currentUser user_id: currentUserId, value: JSON.stringify(themeJson), }; - savePreference(serverUrl, [pref]); - }, [allowedThemeKeys, initialTheme, currentTeamId, currentUserId, serverUrl, newTheme]); + await savePreference(serverUrl, [pref]); + }, [allowedThemeKeys, currentTeamId, currentUserId, customTheme, serverUrl])); useDidUpdate(() => { - const differentTheme = theme.type?.toLowerCase() !== newTheme?.toLowerCase(); - - if (!differentTheme) { - close(); - return; + // when the user selects any of the predefined theme when the current theme is custom, the custom theme will disappear. + // by storing the current theme in the state, the custom theme will remain, and the user can switch back to it + if (theme.type?.toLowerCase() === 'custom') { + setCustomTheme(theme); } - setThemePreference(); - }, [close, newTheme, setThemePreference, theme.type]); + }, [theme.type]); - const onAndroidBack = () => { - setThemePreference(); - close(); - }; - - useAndroidHardwareBackHandler(componentId, onAndroidBack); + useAndroidHardwareBackHandler(componentId, close); return ( - {initialTheme.type === 'custom' && ( + {customTheme && ( )}