Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#9227 Refactor useTheme to use useManagedStorageState #9228

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
0ee3c37
add hideSidebarLogo to manatgedStorageSchema
mnholtz Oct 2, 2024
2bcc02e
override showSidebarLogo in useTheme
mnholtz Oct 2, 2024
5d60f92
add unit tests to useTheme
mnholtz Oct 2, 2024
d3db8e9
Merge branch 'main' into feature/9222_hide_sidebar_logo_policy
mnholtz Oct 2, 2024
7b1e90e
fix header button alignment when missing sidebar logo
mnholtz Oct 2, 2024
319fdb6
Merge branch 'feature/9222_hide_sidebar_logo_policy' of https://githu…
mnholtz Oct 2, 2024
afa7f80
refactor rename hideSidebarLogo -> showSidebarLogo
mnholtz Oct 3, 2024
58f65a8
refactor combine test cases
mnholtz Oct 3, 2024
3924d4d
add add custom theme to test cases
mnholtz Oct 3, 2024
ebba63f
deck version number in code comment
mnholtz Oct 3, 2024
4cba229
Merge branch 'main' into feature/9222_hide_sidebar_logo_policy
mnholtz Oct 3, 2024
1370d6a
refactor use useManagedStorageState instead of readManagedStorageByKey
mnholtz Oct 3, 2024
cb2355f
make test cases complete
mnholtz Oct 3, 2024
bcc470c
refactor use useManagedStorageState instead of readManagedStorageByKey
mnholtz Oct 3, 2024
29b5180
rebase fixes
mnholtz Oct 3, 2024
2fabe25
resolve merge conflicts
mnholtz Oct 3, 2024
0b71176
resolve merge issues
mnholtz Oct 3, 2024
9c46ed1
replace useManagedStorageState implementation with useAsyncExternalStore
mnholtz Oct 4, 2024
ce8a6ac
make some test accomodations for hook changes
mnholtz Oct 4, 2024
4ea0d9b
Merge branch 'refs/heads/main' into feature/9227_refactor_use_theme
mnholtz Oct 7, 2024
b584c2a
fix one useManagedStorageState unit test
mnholtz Oct 7, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 68 additions & 1 deletion src/hooks/useTheme.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,19 +22,39 @@ import { initialTheme } from "@/themes/themeStore";
import { type AsyncState } from "@/types/sliceTypes";
import { themeStorage } from "@/themes/themeUtils";
import { activateTheme } from "@/background/messenger/api";
import useManagedStorageState from "@/store/enterprise/useManagedStorageState";

afterEach(() => {
jest.clearAllMocks();
});

jest.mock("@/hooks/useAsyncExternalStore");
jest.mock("@/background/messenger/api");
jest.mock("@/store/enterprise/useManagedStorageState");

const customTheme = {
themeName: "custom",
showSidebarLogo: true,
customSidebarLogo: "https://example.com/custom-logo.png",
toolbarIcon: "https://example.com/custom-icon.svg",
logo: {
regular: "https://example.com/custom-logo-regular.png",
small: "https://example.com/custom-logo-small.png",
},
lastFetched: Date.now(),
};

describe("useTheme", () => {
beforeEach(() => {
beforeEach(async () => {
jest
.mocked(useAsyncExternalStore)
.mockReturnValue({ data: initialTheme, isLoading: false } as AsyncState);
jest.mocked(useManagedStorageState).mockReturnValue({
data: {},
isLoading: false,
});
});

test("calls useAsyncExternalStore and gets current theme state", async () => {
const { result: themeResult } = renderHook(() => useTheme());

Expand Down Expand Up @@ -66,11 +86,58 @@ describe("useTheme", () => {
} as AsyncState);

renderHook(() => useTheme());

expect(activateTheme).not.toHaveBeenCalled();

jest.advanceTimersByTime(125_000);

renderHook(() => useTheme());

expect(activateTheme).toHaveBeenCalledOnce();
});

it.each([
{ policyValue: true, themeValue: true, expectedValue: true },
{ policyValue: true, themeValue: false, expectedValue: true },
{ policyValue: false, themeValue: true, expectedValue: false },
{ policyValue: false, themeValue: false, expectedValue: false },
{ policyValue: undefined, themeValue: true, expectedValue: true },
{ policyValue: undefined, themeValue: false, expectedValue: false },
])(
"handles showSidebarLogo policy (policy: $policyValue, theme: $themeValue, expected: $expectedValue)",
async ({ policyValue, themeValue, expectedValue }) => {
jest.mocked(useAsyncExternalStore).mockReturnValue({
data: { ...customTheme, showSidebarLogo: themeValue },
isLoading: false,
} as AsyncState);
jest.mocked(useManagedStorageState).mockReturnValue({
data: { showSidebarLogo: policyValue },
isLoading: false,
});

const { result } = renderHook(() => useTheme());

expect(result.current.activeTheme).toMatchObject({
...customTheme,
showSidebarLogo: expectedValue,
});
},
);

it("uses activeTheme when an error occurs in managed storage", async () => {
jest.mocked(useAsyncExternalStore).mockReturnValue({
data: customTheme,
isLoading: false,
} as AsyncState);

jest.mocked(useManagedStorageState).mockImplementation(() => {
throw new Error("Managed storage error");
});

const { result } = renderHook(() => useTheme());

expect(result.current.activeTheme.showSidebarLogo).toBe(
customTheme.showSidebarLogo,
);
});
});
27 changes: 22 additions & 5 deletions src/hooks/useTheme.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
import { initialTheme } from "@/themes/themeStore";
import useAsyncExternalStore from "@/hooks/useAsyncExternalStore";
import { activateTheme } from "@/background/messenger/api";
import useManagedStorageState from "@/store/enterprise/useManagedStorageState";

const themeStorageSubscribe = (callback: () => void) => {
const abortController = new AbortController();
Expand All @@ -42,10 +43,15 @@ const themeStorageSubscribe = (callback: () => void) => {
function useTheme(): { activeTheme: ThemeAssets; isLoading: boolean } {
// The active theme is fetched with `getActiveTheme` in the background script and cached in the themeStorage,
// This hook subscribes to changes in themeStorage to retrieve the latest current activeTheme
const { data: cachedTheme, isLoading } = useAsyncExternalStore(
themeStorageSubscribe,
themeStorage.get,
);
const { data: cachedTheme, isLoading: isCachedThemeLoading } =
useAsyncExternalStore(themeStorageSubscribe, themeStorage.get);

const { data: managedStorageState, isLoading: isManagedStorageLoading } =
useManagedStorageState();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use this as an opportunity to properly refactor useManagedStorageState to return AsyncState

Copy link
Contributor

@twschiller twschiller Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably use the following? I don't remember if there's a reason why we didn't use that other than that maybe the useAsyncExternalStore method didn't exist yet (I haven't checked) / or it just wasn't very valuable given that managed storage values should never change in practice:

  • useAsyncExternalStore
  • readManagedStorage

I think we then get error handling for free with those

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering the same thing, re why not use readManagedStorage + useAsyncExternalStore

Copy link
Contributor

@twschiller twschiller Oct 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering the same thing, re why not use readManagedStorage + useAsyncExternalStore

I don't see a reason not to. The main difference will be it will be technically possible for the hook to return an error state. Whereas the current one could potentially stay isLoading indefinitely. But I don't think the difference matters in practice


const showSidebarLogoOverride = managedStorageState?.showSidebarLogo;

const isLoading = isManagedStorageLoading || isCachedThemeLoading;

useEffect(() => {
if (
Expand All @@ -70,7 +76,18 @@ function useTheme(): { activeTheme: ThemeAssets; isLoading: boolean } {
setThemeFavicon(activeTheme.themeName);
}, [activeTheme, cachedTheme, isLoading]);

return { activeTheme, isLoading };
return {
activeTheme: {
...activeTheme,
// There is a managed storage policy that overrides the sidebar logo visibility specified by the team theme
// See managedStorageSchema.json
showSidebarLogo:
showSidebarLogoOverride == null
? activeTheme.showSidebarLogo
: showSidebarLogoOverride,
},
isLoading,
};
}

export default useTheme;
15 changes: 5 additions & 10 deletions src/sidebar/Header.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ function reloadSidebar() {
const Header: React.FunctionComponent = () => {
const {
activeTheme: { logo, showSidebarLogo, customSidebarLogo, themeName },
isLoading,
isLoading: isThemeLoading,
} = useTheme();

const { flagOn } = useFlags();
Expand All @@ -47,23 +47,18 @@ const Header: React.FunctionComponent = () => {
[styles.themeColor || ""]: themeName !== DEFAULT_THEME,
});

if (isLoading) {
return null;
}

return (
<div className="d-flex py-2 pl-2 pr-0 align-items-center">
{showSidebarLogo && (
// `mx-auto` centers the logo
<div className="mx-auto">
<div className="mx-auto">
{!isThemeLoading && showSidebarLogo && (
<img
src={customSidebarLogo ?? logo.regular}
alt={customSidebarLogo ? "Custom logo" : "PixieBrix logo"}
className={styles.logo}
data-testid="sidebarHeaderLogo"
/>
</div>
)}
)}
</div>
{showDeveloperUI && (
<Button
type="button"
Expand Down
5 changes: 5 additions & 0 deletions src/store/enterprise/managedStorageTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,4 +73,9 @@ export type ManagedStorageState = {
* @since 1.7.36
*/
disableBrowserWarning?: boolean;
/**
* Controls the visibility of the PixieBrix sidebar logo. Overrides the team theme, if one exists.
* @since 2.1.3
*/
showSidebarLogo?: boolean;
};
4 changes: 4 additions & 0 deletions static/managedStorageSchema.json
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@
"type": "boolean",
"description": "Disable the browser warning for non-Chrome browsers, e.g., Microsoft Edge",
"default": false
},
"showSidebarLogo": {
"type": "boolean",
"description": "Controls the visibility the PixieBrix sidebar logo. Overrides the team theme, if one exists."
}
}
}
Loading