From 4da94ef26c49fb4ac5127b003cb464d97d1a1cb5 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Thu, 2 May 2024 13:52:46 +0000 Subject: [PATCH 01/19] fix: remove some of the jank around our core App component --- site/src/App.tsx | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/site/src/App.tsx b/site/src/App.tsx index 9b0a554b107af..f2dd6988ec273 100644 --- a/site/src/App.tsx +++ b/site/src/App.tsx @@ -38,9 +38,23 @@ export const AppProviders: FC = ({ }) => { // https://tanstack.com/query/v4/docs/react/devtools const [showDevtools, setShowDevtools] = useState(false); + useEffect(() => { - window.toggleDevtools = () => setShowDevtools((old) => !old); - // eslint-disable-next-line react-hooks/exhaustive-deps -- no dependencies needed here + // Storing key in variable to avoid accidental typos; we're working with the + // window object, so there's basically zero type-checking available + const toggleKey = "toggleDevtools"; + + // Don't want to throw away the previous devtools value if some other + // extension added something already + const devtoolsBeforeSync = window[toggleKey]; + window[toggleKey] = () => { + devtoolsBeforeSync?.(); + setShowDevtools((current) => !current); + }; + + return () => { + window[toggleKey] = devtoolsBeforeSync; + }; }, []); return ( @@ -60,10 +74,10 @@ export const AppProviders: FC = ({ export const App: FC = () => { return ( - - + + - - + + ); }; From 998602456da17b8d8816bc36e4fbf58473573516 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Thu, 2 May 2024 17:53:36 +0000 Subject: [PATCH 02/19] refactor: scope navigation logic more aggressively --- scripts/testidp/main.go | 6 +++++- site/src/contexts/auth/RequireAuth.tsx | 9 +++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/scripts/testidp/main.go b/scripts/testidp/main.go index 9feae443abf2d..fc965adefb96c 100644 --- a/scripts/testidp/main.go +++ b/scripts/testidp/main.go @@ -84,7 +84,11 @@ func RunIDP() func(t *testing.T) { return func(t *testing.T) { idp := oidctest.NewFakeIDP(t, oidctest.WithServing(), - oidctest.WithStaticUserInfo(jwt.MapClaims{}), + oidctest.WithStaticUserInfo(jwt.MapClaims{ + "email": "blah@coder.com", + "preferred_username": "blah", + "email_verified": true, + }), oidctest.WithDefaultIDClaims(jwt.MapClaims{}), oidctest.WithDefaultExpire(*expiry), oidctest.WithStaticCredentials(*clientID, *clientSecret), diff --git a/site/src/contexts/auth/RequireAuth.tsx b/site/src/contexts/auth/RequireAuth.tsx index d8db9071cc940..3a04b49556344 100644 --- a/site/src/contexts/auth/RequireAuth.tsx +++ b/site/src/contexts/auth/RequireAuth.tsx @@ -12,10 +12,6 @@ export const RequireAuth: FC = () => { const { signOut, isSigningOut, isSignedOut, isSignedIn, isLoading } = useAuthContext(); const location = useLocation(); - const isHomePage = location.pathname === "/"; - const navigateTo = isHomePage - ? "/login" - : embedRedirect(`${location.pathname}${location.search}`); useEffect(() => { if (isLoading || isSigningOut || !isSignedIn) { @@ -48,6 +44,11 @@ export const RequireAuth: FC = () => { } if (isSignedOut) { + const isHomePage = location.pathname === "/"; + const navigateTo = isHomePage + ? "/login" + : embedRedirect(`${location.pathname}${location.search}`); + return ( ); From 7004c9c2bc2444011e2749e2332e0e2ae39254cb Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Thu, 2 May 2024 19:02:00 +0000 Subject: [PATCH 03/19] refactor: add explicit return type to useAuthenticated --- site/src/contexts/auth/RequireAuth.tsx | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/site/src/contexts/auth/RequireAuth.tsx b/site/src/contexts/auth/RequireAuth.tsx index 3a04b49556344..7f23da60c3dbb 100644 --- a/site/src/contexts/auth/RequireAuth.tsx +++ b/site/src/contexts/auth/RequireAuth.tsx @@ -9,9 +9,9 @@ import { embedRedirect } from "utils/redirect"; import { type AuthContextValue, useAuthContext } from "./AuthProvider"; export const RequireAuth: FC = () => { + const location = useLocation(); const { signOut, isSigningOut, isSignedOut, isSignedIn, isLoading } = useAuthContext(); - const location = useLocation(); useEffect(() => { if (isLoading || isSigningOut || !isSignedIn) { @@ -65,7 +65,15 @@ export const RequireAuth: FC = () => { ); }; -export const useAuthenticated = () => { +// We can do some TS magic here but I would rather to be explicit on what +// values are not undefined when authenticated +type NonNullableAuth = AuthContextValue & { + user: Exclude; + permissions: Exclude; + organizationId: Exclude; +}; + +export const useAuthenticated = (): NonNullableAuth => { const auth = useAuthContext(); if (!auth.user) { @@ -76,11 +84,5 @@ export const useAuthenticated = () => { throw new Error("Permissions are not available."); } - // We can do some TS magic here but I would rather to be explicit on what - // values are not undefined when authenticated - return auth as AuthContextValue & { - user: Exclude; - permissions: Exclude; - organizationId: Exclude; - }; + return auth as NonNullableAuth; }; From ebfaec538c746b5e30b84a9833ca65ffdd094db6 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Thu, 2 May 2024 19:02:38 +0000 Subject: [PATCH 04/19] refactor: clean up ProxyContext code --- site/src/contexts/ProxyContext.tsx | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/site/src/contexts/ProxyContext.tsx b/site/src/contexts/ProxyContext.tsx index 07e1486dd12e6..6848294699dfb 100644 --- a/site/src/contexts/ProxyContext.tsx +++ b/site/src/contexts/ProxyContext.tsx @@ -118,7 +118,7 @@ export const ProxyProvider: FC = ({ children }) => { }); const { permissions } = useAuthenticated(); - const query = async (): Promise => { + const queryFn = async (): Promise => { const endpoint = permissions.editWorkspaceProxies ? getWorkspaceProxies : getWorkspaceProxyRegions; @@ -131,13 +131,7 @@ export const ProxyProvider: FC = ({ children }) => { error: proxiesError, isLoading: proxiesLoading, isFetched: proxiesFetched, - } = useQuery( - cachedQuery({ - initialData, - queryKey, - queryFn: query, - }), - ); + } = useQuery(cachedQuery({ initialData, queryKey, queryFn })); // Every time we get a new proxiesResponse, update the latency check // to each workspace proxy. From 1192eb357be28351162cb5cab498e75f0f737dbd Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Thu, 2 May 2024 21:58:02 +0000 Subject: [PATCH 05/19] wip: add code for consolidating the HTML metadata --- site/src/hooks/useHtmlMetadata.ts | 155 ++++++++++++++++++++++++++++++ 1 file changed, 155 insertions(+) create mode 100644 site/src/hooks/useHtmlMetadata.ts diff --git a/site/src/hooks/useHtmlMetadata.ts b/site/src/hooks/useHtmlMetadata.ts new file mode 100644 index 0000000000000..056ecb7c1c20a --- /dev/null +++ b/site/src/hooks/useHtmlMetadata.ts @@ -0,0 +1,155 @@ +import { useMemo, useSyncExternalStore } from "react"; +import type { + AppearanceConfig, + BuildInfoResponse, + Entitlements, + Experiments, + User, +} from "api/typesGenerated"; + +/** + * This is the set of values that are currently being exposed to the React + * application during production. These values are embedded via the Go server, + * so they will never exist when using a JavaScript runtime for the backend + */ +type SourceHtmlMetadata = Readonly<{ + user: User; + experiments: Experiments; + appearanceConfig: AppearanceConfig; + buildInfo: BuildInfoResponse; + entitlements: Entitlements; +}>; + +type MetadataKey = keyof SourceHtmlMetadata; +export type RuntimeHtmlMetadata = Readonly<{ + [Key in MetadataKey]: SourceHtmlMetadata[Key] | undefined; +}>; + +type SubscriptionCallback = (metadata: RuntimeHtmlMetadata) => void; +type QuerySelector = typeof document.querySelector; + +interface MetadataManagerApi { + subscribe: (callback: SubscriptionCallback) => () => void; + getMetadata: () => RuntimeHtmlMetadata; + clearMetadataByKey: (key: MetadataKey) => void; +} + +export class MetadataManager implements MetadataManagerApi { + private readonly querySelector: QuerySelector; + private readonly subscriptions: Set; + private readonly trackedMetadataNodes: Map; + private metadata: RuntimeHtmlMetadata; + + constructor(querySelector?: QuerySelector) { + this.querySelector = querySelector ?? document.querySelector; + this.subscriptions = new Set(); + this.trackedMetadataNodes = new Map(); + + this.metadata = { + user: this.registerAsJson("user"), + appearanceConfig: this.registerAsJson("appearance"), + buildInfo: this.registerAsJson("build-info"), + entitlements: this.registerAsJson("entitlements"), + experiments: this.registerAsJson("experiments"), + }; + } + + private notifySubscriptionsOfStateChange(): void { + const metadataBinding = this.metadata; + this.subscriptions.forEach((cb) => cb(metadataBinding)); + } + + private registerAsJson>( + key: string, + ): T | undefined { + const metadataNode = this.querySelector(`meta[property=${key}]`); + if (!metadataNode) { + return undefined; + } + + const rawContent = metadataNode.getAttribute("content"); + if (rawContent) { + try { + const result = JSON.parse(rawContent) as T; + this.trackedMetadataNodes.set(key, metadataNode); + return result; + } catch (err) { + // In development, the metadata is always going to be empty; error is + // only a concern for production + if (process.env.NODE_ENV === "production") { + console.warn(`Failed to parse ${key} metadata. Error message:`); + console.warn(err); + } + } + } + + return undefined; + } + + ////////////////////////////////////////////////////////////////////////////// + // All public functions should be defined as arrow functions to ensure that + // they cannot lose their `this` context when passed around the React UI + ////////////////////////////////////////////////////////////////////////////// + + subscribe = (callback: SubscriptionCallback): (() => void) => { + this.subscriptions.add(callback); + return () => this.subscriptions.delete(callback); + }; + + getMetadata = (): RuntimeHtmlMetadata => { + return this.metadata; + }; + + clearMetadataByKey = (key: MetadataKey): void => { + const metadataValue = this.metadata[key]; + if (metadataValue === undefined) { + return; + } + + this.metadata = { + ...this.metadata, + [key]: undefined, + }; + + const metadataNode = this.trackedMetadataNodes.get(key); + if (metadataNode !== undefined) { + metadataNode.remove(); + this.trackedMetadataNodes.delete(key); + } + + this.notifySubscriptionsOfStateChange(); + }; +} + +type UseHtmlMetadataResult = Readonly<{ + metadata: RuntimeHtmlMetadata; + clearMetadataByKey: MetadataManager["clearMetadataByKey"]; +}>; + +export function makeUseHtmlMetadata( + manager: MetadataManager, +): () => UseHtmlMetadataResult { + return function useHtmlMetadata(): UseHtmlMetadataResult { + // Hook binds re-renders to the memory reference of the entire exposed + // metadata object, meaning that even if you only care about one value, + // using the hook will cause a component to re-render if the object changes + // at all If this becomes a performance issue down the line, we can look + // into selector functions to minimize re-renders + const metadata = useSyncExternalStore( + manager.subscribe, + manager.getMetadata, + ); + + const result = useMemo(() => { + return { + metadata, + clearMetadataByKey: manager.clearMetadataByKey, + }; + }, [metadata]); + + return result; + }; +} + +const defaultManager = new MetadataManager(); +export const useHtmlMetadata = makeUseHtmlMetadata(defaultManager); From bbe2ae0a6f686f20a63bdeb6c1857ef83160cb84 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Thu, 2 May 2024 22:02:29 +0000 Subject: [PATCH 06/19] refactor: clean up hook logic --- site/src/hooks/useHtmlMetadata.ts | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/site/src/hooks/useHtmlMetadata.ts b/site/src/hooks/useHtmlMetadata.ts index 056ecb7c1c20a..5ed17f2583859 100644 --- a/site/src/hooks/useHtmlMetadata.ts +++ b/site/src/hooks/useHtmlMetadata.ts @@ -11,6 +11,10 @@ import type { * This is the set of values that are currently being exposed to the React * application during production. These values are embedded via the Go server, * so they will never exist when using a JavaScript runtime for the backend + * + * If you need to add a new type of metadata value, add a new property to the + * type alias here, and then rest of the file should light up with errors for + * what else needs to be adjusted */ type SourceHtmlMetadata = Readonly<{ user: User; @@ -106,17 +110,15 @@ export class MetadataManager implements MetadataManagerApi { return; } + const metadataNode = this.trackedMetadataNodes.get(key); + metadataNode?.remove(); + this.trackedMetadataNodes.delete(key); + this.metadata = { ...this.metadata, [key]: undefined, }; - const metadataNode = this.trackedMetadataNodes.get(key); - if (metadataNode !== undefined) { - metadataNode.remove(); - this.trackedMetadataNodes.delete(key); - } - this.notifySubscriptionsOfStateChange(); }; } @@ -140,14 +142,14 @@ export function makeUseHtmlMetadata( manager.getMetadata, ); - const result = useMemo(() => { + const stableMetadataResult = useMemo(() => { return { metadata, clearMetadataByKey: manager.clearMetadataByKey, }; }, [metadata]); - return result; + return stableMetadataResult; }; } From 1c41937cb286c2053db606700228f156fd17477e Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Thu, 2 May 2024 23:04:28 +0000 Subject: [PATCH 07/19] refactor: rename useHtmlMetadata to useEmbeddedMetadata --- site/src/hooks/useEmbeddedMetadata.ts | 202 ++++++++++++++++++++++++++ 1 file changed, 202 insertions(+) create mode 100644 site/src/hooks/useEmbeddedMetadata.ts diff --git a/site/src/hooks/useEmbeddedMetadata.ts b/site/src/hooks/useEmbeddedMetadata.ts new file mode 100644 index 0000000000000..34dfdc9bf88f6 --- /dev/null +++ b/site/src/hooks/useEmbeddedMetadata.ts @@ -0,0 +1,202 @@ +import { useMemo, useSyncExternalStore } from "react"; +import type { + AppearanceConfig, + BuildInfoResponse, + Entitlements, + Experiments, + User, +} from "api/typesGenerated"; + +/** + * This is the set of values that are currently being exposed to the React + * application during production. These values are embedded via the Go server, + * so they will never exist when using a JavaScript runtime for the backend + * + * If you need to add a new type of metadata value, add a new property to the + * type alias here, and then rest of the file should light up with errors for + * what else needs to be adjusted + */ +type AvailableMetadata = Readonly<{ + user: User; + experiments: Experiments; + appearanceConfig: AppearanceConfig; + buildInfo: BuildInfoResponse; + entitlements: Entitlements; +}>; + +type MetadataKey = keyof AvailableMetadata; +type MetadataValue = AvailableMetadata[MetadataKey]; + +export type MetadataState = Readonly<{ + // undefined chosen to signify missing value because unlike null, it isn't a + // valid JSON-serializable value. It's impossible to be returned by the API + value: T | undefined; + status: "missing" | "loadedOnMount" | "deleted"; +}>; + +export type RuntimeHtmlMetadata = Readonly<{ + [Key in MetadataKey]: MetadataState; +}>; + +type SubscriptionCallback = (metadata: RuntimeHtmlMetadata) => void; +type QuerySelector = typeof document.querySelector; + +type ParseJsonResult = Readonly< + | { + value: T; + node: Element; + } + | { + value: undefined; + node: null; + } +>; + +interface MetadataManagerApi { + subscribe: (callback: SubscriptionCallback) => () => void; + getMetadata: () => RuntimeHtmlMetadata; + clearMetadataByKey: (key: MetadataKey) => void; +} + +export class MetadataManager implements MetadataManagerApi { + private readonly querySelector: QuerySelector; + private readonly subscriptions: Set; + private readonly trackedMetadataNodes: Map; + private metadata: RuntimeHtmlMetadata; + + constructor(querySelector?: QuerySelector) { + this.querySelector = querySelector ?? document.querySelector; + this.subscriptions = new Set(); + this.trackedMetadataNodes = new Map(); + + this.metadata = { + user: this.registerValue("user"), + appearanceConfig: this.registerValue("appearance"), + buildInfo: this.registerValue("build-info"), + entitlements: this.registerValue("entitlements"), + experiments: this.registerValue("experiments"), + }; + } + + private notifySubscriptionsOfStateChange(): void { + const metadataBinding = this.metadata; + this.subscriptions.forEach((cb) => cb(metadataBinding)); + } + + private registerValue( + propertyName: string, + ): MetadataState { + const { value, node } = this.parseJson(propertyName); + + let newEntry: MetadataState; + if (!node || value === undefined) { + newEntry = { + value: undefined, + status: "missing", + }; + } else { + newEntry = { + value, + status: "loadedOnMount", + }; + } + + this.trackedMetadataNodes.set(propertyName, node); + return newEntry; + } + + private parseJson(key: string): ParseJsonResult { + const node = this.querySelector(`meta[property=${key}]`); + if (!node) { + return { value: undefined, node: null }; + } + + const rawContent = node.getAttribute("content"); + if (rawContent) { + try { + const value = JSON.parse(rawContent) as T; + return { value, node }; + } catch (err) { + // In development, the metadata is always going to be empty; error is + // only a concern for production + if (process.env.NODE_ENV === "production") { + console.warn(`Failed to parse ${key} metadata. Error message:`); + console.warn(err); + } + } + } + + return { value: undefined, node: null }; + } + + ////////////////////////////////////////////////////////////////////////////// + // All public functions should be defined as arrow functions to ensure that + // they cannot lose their `this` context when passed around the React UI + ////////////////////////////////////////////////////////////////////////////// + + subscribe = (callback: SubscriptionCallback): (() => void) => { + this.subscriptions.add(callback); + return () => this.subscriptions.delete(callback); + }; + + getMetadata = (): RuntimeHtmlMetadata => { + return this.metadata; + }; + + clearMetadataByKey = (key: MetadataKey): void => { + const metadataValue = this.metadata[key]; + if (metadataValue.status === "missing") { + return; + } + + const metadataNode = this.trackedMetadataNodes.get(key); + this.trackedMetadataNodes.delete(key); + + // Delete the node entirely so that no other code can accidentally access + // the value after it's supposed to have been made unavailable + metadataNode?.remove(); + + type NewState = MetadataState>; + const newState: NewState = { + ...metadataValue, + value: undefined, + status: "deleted", + }; + + this.metadata = { ...this.metadata, [key]: newState }; + this.notifySubscriptionsOfStateChange(); + }; +} + +type UseHtmlMetadataResult = Readonly<{ + metadata: RuntimeHtmlMetadata; + clearMetadataByKey: MetadataManager["clearMetadataByKey"]; +}>; + +export function makeUseHtmlMetadata( + manager: MetadataManager, +): () => UseHtmlMetadataResult { + return function useHtmlMetadata(): UseHtmlMetadataResult { + // Hook binds re-renders to the memory reference of the entire exposed + // metadata object, meaning that even if you only care about one value, + // using the hook will cause a component to re-render if the object changes + // at all If this becomes a performance issue down the line, we can look + // into selector functions to minimize re-renders + const metadata = useSyncExternalStore( + manager.subscribe, + manager.getMetadata, + ); + + const stableMetadataResult = useMemo(() => { + return { + metadata, + clearMetadataByKey: manager.clearMetadataByKey, + }; + }, [metadata]); + + return stableMetadataResult; + }; +} + +const defaultManager = new MetadataManager(); +export const useHtmlMetadata = makeUseHtmlMetadata(defaultManager); From 79e9c450a153deaee52959632db4587e30be6a4d Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Thu, 2 May 2024 23:06:30 +0000 Subject: [PATCH 08/19] fix: correct names that weren't updated --- site/src/hooks/useEmbeddedMetadata.ts | 12 +- site/src/hooks/useHtmlMetadata.ts | 157 -------------------------- 2 files changed, 6 insertions(+), 163 deletions(-) delete mode 100644 site/src/hooks/useHtmlMetadata.ts diff --git a/site/src/hooks/useEmbeddedMetadata.ts b/site/src/hooks/useEmbeddedMetadata.ts index 34dfdc9bf88f6..56954a88da1f6 100644 --- a/site/src/hooks/useEmbeddedMetadata.ts +++ b/site/src/hooks/useEmbeddedMetadata.ts @@ -168,15 +168,15 @@ export class MetadataManager implements MetadataManagerApi { }; } -type UseHtmlMetadataResult = Readonly<{ +type UseEmbeddedMetadataResult = Readonly<{ metadata: RuntimeHtmlMetadata; clearMetadataByKey: MetadataManager["clearMetadataByKey"]; }>; -export function makeUseHtmlMetadata( +export function makeUseEmbeddedMetadata( manager: MetadataManager, -): () => UseHtmlMetadataResult { - return function useHtmlMetadata(): UseHtmlMetadataResult { +): () => UseEmbeddedMetadataResult { + return function useEmbeddedMetadata(): UseEmbeddedMetadataResult { // Hook binds re-renders to the memory reference of the entire exposed // metadata object, meaning that even if you only care about one value, // using the hook will cause a component to re-render if the object changes @@ -187,7 +187,7 @@ export function makeUseHtmlMetadata( manager.getMetadata, ); - const stableMetadataResult = useMemo(() => { + const stableMetadataResult = useMemo(() => { return { metadata, clearMetadataByKey: manager.clearMetadataByKey, @@ -199,4 +199,4 @@ export function makeUseHtmlMetadata( } const defaultManager = new MetadataManager(); -export const useHtmlMetadata = makeUseHtmlMetadata(defaultManager); +export const useEmbeddedMetadata = makeUseEmbeddedMetadata(defaultManager); diff --git a/site/src/hooks/useHtmlMetadata.ts b/site/src/hooks/useHtmlMetadata.ts deleted file mode 100644 index 5ed17f2583859..0000000000000 --- a/site/src/hooks/useHtmlMetadata.ts +++ /dev/null @@ -1,157 +0,0 @@ -import { useMemo, useSyncExternalStore } from "react"; -import type { - AppearanceConfig, - BuildInfoResponse, - Entitlements, - Experiments, - User, -} from "api/typesGenerated"; - -/** - * This is the set of values that are currently being exposed to the React - * application during production. These values are embedded via the Go server, - * so they will never exist when using a JavaScript runtime for the backend - * - * If you need to add a new type of metadata value, add a new property to the - * type alias here, and then rest of the file should light up with errors for - * what else needs to be adjusted - */ -type SourceHtmlMetadata = Readonly<{ - user: User; - experiments: Experiments; - appearanceConfig: AppearanceConfig; - buildInfo: BuildInfoResponse; - entitlements: Entitlements; -}>; - -type MetadataKey = keyof SourceHtmlMetadata; -export type RuntimeHtmlMetadata = Readonly<{ - [Key in MetadataKey]: SourceHtmlMetadata[Key] | undefined; -}>; - -type SubscriptionCallback = (metadata: RuntimeHtmlMetadata) => void; -type QuerySelector = typeof document.querySelector; - -interface MetadataManagerApi { - subscribe: (callback: SubscriptionCallback) => () => void; - getMetadata: () => RuntimeHtmlMetadata; - clearMetadataByKey: (key: MetadataKey) => void; -} - -export class MetadataManager implements MetadataManagerApi { - private readonly querySelector: QuerySelector; - private readonly subscriptions: Set; - private readonly trackedMetadataNodes: Map; - private metadata: RuntimeHtmlMetadata; - - constructor(querySelector?: QuerySelector) { - this.querySelector = querySelector ?? document.querySelector; - this.subscriptions = new Set(); - this.trackedMetadataNodes = new Map(); - - this.metadata = { - user: this.registerAsJson("user"), - appearanceConfig: this.registerAsJson("appearance"), - buildInfo: this.registerAsJson("build-info"), - entitlements: this.registerAsJson("entitlements"), - experiments: this.registerAsJson("experiments"), - }; - } - - private notifySubscriptionsOfStateChange(): void { - const metadataBinding = this.metadata; - this.subscriptions.forEach((cb) => cb(metadataBinding)); - } - - private registerAsJson>( - key: string, - ): T | undefined { - const metadataNode = this.querySelector(`meta[property=${key}]`); - if (!metadataNode) { - return undefined; - } - - const rawContent = metadataNode.getAttribute("content"); - if (rawContent) { - try { - const result = JSON.parse(rawContent) as T; - this.trackedMetadataNodes.set(key, metadataNode); - return result; - } catch (err) { - // In development, the metadata is always going to be empty; error is - // only a concern for production - if (process.env.NODE_ENV === "production") { - console.warn(`Failed to parse ${key} metadata. Error message:`); - console.warn(err); - } - } - } - - return undefined; - } - - ////////////////////////////////////////////////////////////////////////////// - // All public functions should be defined as arrow functions to ensure that - // they cannot lose their `this` context when passed around the React UI - ////////////////////////////////////////////////////////////////////////////// - - subscribe = (callback: SubscriptionCallback): (() => void) => { - this.subscriptions.add(callback); - return () => this.subscriptions.delete(callback); - }; - - getMetadata = (): RuntimeHtmlMetadata => { - return this.metadata; - }; - - clearMetadataByKey = (key: MetadataKey): void => { - const metadataValue = this.metadata[key]; - if (metadataValue === undefined) { - return; - } - - const metadataNode = this.trackedMetadataNodes.get(key); - metadataNode?.remove(); - this.trackedMetadataNodes.delete(key); - - this.metadata = { - ...this.metadata, - [key]: undefined, - }; - - this.notifySubscriptionsOfStateChange(); - }; -} - -type UseHtmlMetadataResult = Readonly<{ - metadata: RuntimeHtmlMetadata; - clearMetadataByKey: MetadataManager["clearMetadataByKey"]; -}>; - -export function makeUseHtmlMetadata( - manager: MetadataManager, -): () => UseHtmlMetadataResult { - return function useHtmlMetadata(): UseHtmlMetadataResult { - // Hook binds re-renders to the memory reference of the entire exposed - // metadata object, meaning that even if you only care about one value, - // using the hook will cause a component to re-render if the object changes - // at all If this becomes a performance issue down the line, we can look - // into selector functions to minimize re-renders - const metadata = useSyncExternalStore( - manager.subscribe, - manager.getMetadata, - ); - - const stableMetadataResult = useMemo(() => { - return { - metadata, - clearMetadataByKey: manager.clearMetadataByKey, - }; - }, [metadata]); - - return stableMetadataResult; - }; -} - -const defaultManager = new MetadataManager(); -export const useHtmlMetadata = makeUseHtmlMetadata(defaultManager); From 81f2cd96e36588158779a16ed7746609f5d10db7 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Thu, 2 May 2024 23:13:04 +0000 Subject: [PATCH 09/19] fix: update type-safety of useEmbeddedMetadata further --- site/src/hooks/useEmbeddedMetadata.ts | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/site/src/hooks/useEmbeddedMetadata.ts b/site/src/hooks/useEmbeddedMetadata.ts index 56954a88da1f6..498dd31f4b124 100644 --- a/site/src/hooks/useEmbeddedMetadata.ts +++ b/site/src/hooks/useEmbeddedMetadata.ts @@ -12,16 +12,17 @@ import type { * application during production. These values are embedded via the Go server, * so they will never exist when using a JavaScript runtime for the backend * - * If you need to add a new type of metadata value, add a new property to the - * type alias here, and then rest of the file should light up with errors for - * what else needs to be adjusted + * If you want to add new metadata in a type-safe way, add it to this type. + * Each key should the name of the "property" attribute that will be used on the + * HTML elements themselves, and the values should be the data you get back from + * parsing those element's text content */ type AvailableMetadata = Readonly<{ user: User; experiments: Experiments; - appearanceConfig: AppearanceConfig; - buildInfo: BuildInfoResponse; + appearance: AppearanceConfig; entitlements: Entitlements; + "build-info": BuildInfoResponse; }>; type MetadataKey = keyof AvailableMetadata; @@ -71,10 +72,10 @@ export class MetadataManager implements MetadataManagerApi { this.metadata = { user: this.registerValue("user"), - appearanceConfig: this.registerValue("appearance"), - buildInfo: this.registerValue("build-info"), + appearance: this.registerValue("appearance"), entitlements: this.registerValue("entitlements"), experiments: this.registerValue("experiments"), + "build-info": this.registerValue("build-info"), }; } @@ -84,9 +85,9 @@ export class MetadataManager implements MetadataManagerApi { } private registerValue( - propertyName: string, + key: MetadataKey, ): MetadataState { - const { value, node } = this.parseJson(propertyName); + const { value, node } = this.parseJson(key); let newEntry: MetadataState; if (!node || value === undefined) { @@ -101,7 +102,7 @@ export class MetadataManager implements MetadataManagerApi { }; } - this.trackedMetadataNodes.set(propertyName, node); + this.trackedMetadataNodes.set(key, node); return newEntry; } From 390418f7542f799e36fde977292f3aa8c7daebbd Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 3 May 2024 01:35:18 +0000 Subject: [PATCH 10/19] wip: switch codebase to use metadata hook --- site/src/api/queries/appearance.ts | 10 +-- site/src/api/queries/buildInfo.ts | 8 +- site/src/api/queries/entitlements.ts | 9 +- site/src/api/queries/experiments.ts | 8 +- site/src/api/queries/users.ts | 16 ++-- site/src/api/queries/util.ts | 84 ++++++++++++++----- site/src/contexts/auth/AuthProvider.tsx | 18 +++- site/src/hooks/useEmbeddedMetadata.ts | 14 +--- .../modules/dashboard/DashboardProvider.tsx | 8 +- site/src/utils/metadata.ts | 24 ------ 10 files changed, 107 insertions(+), 92 deletions(-) delete mode 100644 site/src/utils/metadata.ts diff --git a/site/src/api/queries/appearance.ts b/site/src/api/queries/appearance.ts index 8178ed602abb7..d9337bc39e79d 100644 --- a/site/src/api/queries/appearance.ts +++ b/site/src/api/queries/appearance.ts @@ -1,16 +1,14 @@ -import type { QueryClient, UseQueryOptions } from "react-query"; +import type { QueryClient } from "react-query"; import * as API from "api/api"; import type { AppearanceConfig } from "api/typesGenerated"; -import { getMetadataAsJSON } from "utils/metadata"; +import type { MetadataState } from "hooks/useEmbeddedMetadata"; import { cachedQuery } from "./util"; -const initialAppearanceData = getMetadataAsJSON("appearance"); const appearanceConfigKey = ["appearance"] as const; -export const appearance = (): UseQueryOptions => { - // We either have our initial data or should immediately fetch and never again! +export const appearance = (metadata: MetadataState) => { return cachedQuery({ - initialData: initialAppearanceData, + metadata, queryKey: ["appearance"], queryFn: () => API.getAppearance(), }); diff --git a/site/src/api/queries/buildInfo.ts b/site/src/api/queries/buildInfo.ts index 504b59bd1d341..0f0eecafa9f49 100644 --- a/site/src/api/queries/buildInfo.ts +++ b/site/src/api/queries/buildInfo.ts @@ -1,16 +1,14 @@ -import type { UseQueryOptions } from "react-query"; import * as API from "api/api"; import type { BuildInfoResponse } from "api/typesGenerated"; -import { getMetadataAsJSON } from "utils/metadata"; +import type { MetadataState } from "hooks/useEmbeddedMetadata"; import { cachedQuery } from "./util"; -const initialBuildInfoData = getMetadataAsJSON("build-info"); const buildInfoKey = ["buildInfo"] as const; -export const buildInfo = (): UseQueryOptions => { +export const buildInfo = (metadata: MetadataState) => { // The version of the app can't change without reloading the page. return cachedQuery({ - initialData: initialBuildInfoData, + metadata, queryKey: buildInfoKey, queryFn: () => API.getBuildInfo(), }); diff --git a/site/src/api/queries/entitlements.ts b/site/src/api/queries/entitlements.ts index 46b41133bd476..48f43630ea29a 100644 --- a/site/src/api/queries/entitlements.ts +++ b/site/src/api/queries/entitlements.ts @@ -1,15 +1,14 @@ -import type { QueryClient, UseQueryOptions } from "react-query"; +import type { QueryClient } from "react-query"; import * as API from "api/api"; import type { Entitlements } from "api/typesGenerated"; -import { getMetadataAsJSON } from "utils/metadata"; +import type { MetadataState } from "hooks/useEmbeddedMetadata"; import { cachedQuery } from "./util"; -const initialEntitlementsData = getMetadataAsJSON("entitlements"); const entitlementsQueryKey = ["entitlements"] as const; -export const entitlements = (): UseQueryOptions => { +export const entitlements = (metadata: MetadataState) => { return cachedQuery({ - initialData: initialEntitlementsData, + metadata, queryKey: entitlementsQueryKey, queryFn: () => API.getEntitlements(), }); diff --git a/site/src/api/queries/experiments.ts b/site/src/api/queries/experiments.ts index 934e44b863437..e0a2749d75829 100644 --- a/site/src/api/queries/experiments.ts +++ b/site/src/api/queries/experiments.ts @@ -1,15 +1,13 @@ -import type { UseQueryOptions } from "react-query"; import * as API from "api/api"; import type { Experiments } from "api/typesGenerated"; -import { getMetadataAsJSON } from "utils/metadata"; +import type { MetadataState } from "hooks/useEmbeddedMetadata"; import { cachedQuery } from "./util"; -const initialExperimentsData = getMetadataAsJSON("experiments"); const experimentsKey = ["experiments"] as const; -export const experiments = (): UseQueryOptions => { +export const experiments = (metadata: MetadataState) => { return cachedQuery({ - initialData: initialExperimentsData, + metadata, queryKey: experimentsKey, queryFn: () => API.getExperiments(), }); diff --git a/site/src/api/queries/users.ts b/site/src/api/queries/users.ts index 3a1806276146b..1efbc22909704 100644 --- a/site/src/api/queries/users.ts +++ b/site/src/api/queries/users.ts @@ -1,6 +1,5 @@ import type { QueryClient, - QueryKey, UseMutationOptions, UseQueryOptions, } from "react-query"; @@ -15,9 +14,9 @@ import type { User, GenerateAPIKeyResponse, } from "api/typesGenerated"; +import type { MetadataState } from "hooks/useEmbeddedMetadata"; import type { UsePaginatedQueryOptions } from "hooks/usePaginatedQuery"; import { prepareQuery } from "utils/filters"; -import { getMetadataAsJSON } from "utils/metadata"; import { getAuthorizationKey } from "./authCheck"; import { cachedQuery } from "./util"; @@ -113,8 +112,6 @@ export const updateRoles = (queryClient: QueryClient) => { }; }; -const initialUserData = getMetadataAsJSON("user"); - export const authMethods = () => { return { // Even the endpoint being /users/authmethods we don't want to revalidate it @@ -126,11 +123,9 @@ export const authMethods = () => { const meKey = ["me"]; -export const me = (): UseQueryOptions & { - queryKey: QueryKey; -} => { +export const me = (metadata: MetadataState) => { return cachedQuery({ - initialData: initialUserData, + metadata, queryKey: meKey, queryFn: API.getAuthenticatedUser, }); @@ -143,10 +138,9 @@ export function apiKey(): UseQueryOptions { }; } -export const hasFirstUser = (): UseQueryOptions => { +export const hasFirstUser = (userMetadata: MetadataState) => { return cachedQuery({ - // This cannot be false otherwise it will not fetch! - initialData: Boolean(initialUserData) || undefined, + metadata: userMetadata, queryKey: ["hasFirstUser"], queryFn: API.hasFirstUser, }); diff --git a/site/src/api/queries/util.ts b/site/src/api/queries/util.ts index 6dc02ac897482..8d95e54813f99 100644 --- a/site/src/api/queries/util.ts +++ b/site/src/api/queries/util.ts @@ -1,4 +1,38 @@ -import type { UseQueryOptions } from "react-query"; +import type { UseQueryOptions, QueryKey } from "react-query"; +import type { MetadataState, MetadataValue } from "hooks/useEmbeddedMetadata"; + +const disabledFetchOptions = { + cacheTime: Infinity, + staleTime: Infinity, + refetchOnMount: false, + refetchOnReconnect: false, + refetchOnWindowFocus: false, +} as const satisfies UseQueryOptions; + +type UseQueryOptionsWithMetadata< + TMetadata extends MetadataValue = MetadataValue, + TQueryFnData = unknown, + TError = unknown, + TData = TQueryFnData, + TQueryKey extends QueryKey = QueryKey, +> = Omit< + UseQueryOptions, + "initialData" +> & { + metadata: MetadataState; +}; + +type FormattedQueryOptionsResult< + TQueryFnData = unknown, + TError = unknown, + TData = TQueryFnData, + TQueryKey extends QueryKey = QueryKey, +> = Omit< + UseQueryOptions, + "initialData" +> & { + queryKey: NonNullable; +}; /** * cachedQuery allows the caller to only make a request a single time, and use @@ -6,22 +40,34 @@ import type { UseQueryOptions } from "react-query"; * values injected via metadata. We do this for the initial user fetch, * buildinfo, and a few others to reduce page load time. */ -export const cachedQuery = < - TQueryOptions extends UseQueryOptions, - TData, +export function cachedQuery< + TMetadata extends MetadataValue = MetadataValue, + TQueryFnData = unknown, + TError = unknown, + TData = TQueryFnData, + TQueryKey extends QueryKey = QueryKey, >( - options: TQueryOptions, -): TQueryOptions => - // Only do this if there is initial data, otherwise it can conflict with tests. - ({ - ...(options.initialData - ? { - cacheTime: Infinity, - staleTime: Infinity, - refetchOnMount: false, - refetchOnReconnect: false, - refetchOnWindowFocus: false, - } - : {}), - ...options, - }); + options: UseQueryOptionsWithMetadata< + TMetadata, + TQueryFnData, + TError, + TData, + TQueryKey + >, +): FormattedQueryOptionsResult { + const { metadata, ...delegatedOptions } = options; + const metadataIsAvailable = metadata.status === "loaded"; + + const newOptions = { + ...delegatedOptions, + ...(metadataIsAvailable ? disabledFetchOptions : {}), + initialData: metadataIsAvailable ? metadata.value : undefined, + }; + + return newOptions as FormattedQueryOptionsResult< + TQueryFnData, + TError, + TData, + TQueryKey + >; +} diff --git a/site/src/contexts/auth/AuthProvider.tsx b/site/src/contexts/auth/AuthProvider.tsx index f581971b1175a..b085bb6e6b2cf 100644 --- a/site/src/contexts/auth/AuthProvider.tsx +++ b/site/src/contexts/auth/AuthProvider.tsx @@ -17,6 +17,7 @@ import { } from "api/queries/users"; import type { UpdateUserProfileRequest, User } from "api/typesGenerated"; import { displaySuccess } from "components/GlobalSnackbar/utils"; +import { useEmbeddedMetadata } from "hooks/useEmbeddedMetadata"; import { permissionsToCheck, type Permissions } from "./permissions"; export type AuthContextValue = { @@ -42,19 +43,28 @@ export const AuthContext = createContext( ); export const AuthProvider: FC = ({ children }) => { - const queryClient = useQueryClient(); - const meOptions = me(); + const { metadata, clearMetadataByKey } = useEmbeddedMetadata(); + const userMetadataState = metadata.user; + + const meOptions = me(userMetadataState); const userQuery = useQuery(meOptions); - const hasFirstUserQuery = useQuery(hasFirstUser()); + const hasFirstUserQuery = useQuery(hasFirstUser(userMetadataState)); + const permissionsQuery = useQuery({ ...checkAuthorization({ checks: permissionsToCheck }), enabled: userQuery.data !== undefined, }); + const queryClient = useQueryClient(); const loginMutation = useMutation( login({ checks: permissionsToCheck }, queryClient), ); - const logoutMutation = useMutation(logout(queryClient)); + + const logoutMutation = useMutation({ + ...logout(queryClient), + onSuccess: () => clearMetadataByKey("user"), + }); + const updateProfileMutation = useMutation({ ...updateProfileOptions("me"), diff --git a/site/src/hooks/useEmbeddedMetadata.ts b/site/src/hooks/useEmbeddedMetadata.ts index 498dd31f4b124..73c64571f13dd 100644 --- a/site/src/hooks/useEmbeddedMetadata.ts +++ b/site/src/hooks/useEmbeddedMetadata.ts @@ -26,13 +26,13 @@ type AvailableMetadata = Readonly<{ }>; type MetadataKey = keyof AvailableMetadata; -type MetadataValue = AvailableMetadata[MetadataKey]; +export type MetadataValue = AvailableMetadata[MetadataKey]; export type MetadataState = Readonly<{ // undefined chosen to signify missing value because unlike null, it isn't a // valid JSON-serializable value. It's impossible to be returned by the API value: T | undefined; - status: "missing" | "loadedOnMount" | "deleted"; + status: "missing" | "loaded" | "deleted"; }>; export type RuntimeHtmlMetadata = Readonly<{ @@ -91,15 +91,9 @@ export class MetadataManager implements MetadataManagerApi { let newEntry: MetadataState; if (!node || value === undefined) { - newEntry = { - value: undefined, - status: "missing", - }; + newEntry = { value: undefined, status: "missing" }; } else { - newEntry = { - value, - status: "loadedOnMount", - }; + newEntry = { value, status: "loaded" }; } this.trackedMetadataNodes.set(key, node); diff --git a/site/src/modules/dashboard/DashboardProvider.tsx b/site/src/modules/dashboard/DashboardProvider.tsx index 7c1871946cf4c..3cb85c2461c8d 100644 --- a/site/src/modules/dashboard/DashboardProvider.tsx +++ b/site/src/modules/dashboard/DashboardProvider.tsx @@ -16,6 +16,7 @@ import type { } from "api/typesGenerated"; import { displayError } from "components/GlobalSnackbar/utils"; import { Loader } from "components/Loader/Loader"; +import { useEmbeddedMetadata } from "hooks/useEmbeddedMetadata"; import { hslToHex, isHexColor, isHslColor } from "utils/colors"; interface Appearance { @@ -35,9 +36,10 @@ export const DashboardContext = createContext( ); export const DashboardProvider: FC = ({ children }) => { - const entitlementsQuery = useQuery(entitlements()); - const experimentsQuery = useQuery(experiments()); - const appearanceQuery = useQuery(appearance()); + const { metadata } = useEmbeddedMetadata(); + const entitlementsQuery = useQuery(entitlements(metadata.entitlements)); + const experimentsQuery = useQuery(experiments(metadata.experiments)); + const appearanceQuery = useQuery(appearance(metadata.appearance)); const isLoading = !entitlementsQuery.data || !appearanceQuery.data || !experimentsQuery.data; diff --git a/site/src/utils/metadata.ts b/site/src/utils/metadata.ts deleted file mode 100644 index 723b2e508395e..0000000000000 --- a/site/src/utils/metadata.ts +++ /dev/null @@ -1,24 +0,0 @@ -export const getMetadataAsJSON = >( - property: string, -): T | undefined => { - const metadata = document.querySelector(`meta[property=${property}]`); - - if (metadata) { - const rawContent = metadata.getAttribute("content"); - - if (rawContent) { - try { - return JSON.parse(rawContent); - } catch (err) { - // In development, the metadata is always going to be empty; error is - // only a concern for production - if (process.env.NODE_ENV === "production") { - console.warn(`Failed to parse ${property} metadata. Error message:`); - console.warn(err); - } - } - } - } - - return undefined; -}; From e072f7ad40a3814e6496df75b24b5096659046c6 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 3 May 2024 02:02:15 +0000 Subject: [PATCH 11/19] refactor: simplify design of metadata hook --- site/src/api/queries/util.ts | 9 ++++---- site/src/hooks/useEmbeddedMetadata.ts | 33 +++++++++++++-------------- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/site/src/api/queries/util.ts b/site/src/api/queries/util.ts index 8d95e54813f99..6043b984fab93 100644 --- a/site/src/api/queries/util.ts +++ b/site/src/api/queries/util.ts @@ -56,12 +56,13 @@ export function cachedQuery< >, ): FormattedQueryOptionsResult { const { metadata, ...delegatedOptions } = options; - const metadataIsAvailable = metadata.status === "loaded"; - const newOptions = { ...delegatedOptions, - ...(metadataIsAvailable ? disabledFetchOptions : {}), - initialData: metadataIsAvailable ? metadata.value : undefined, + initialData: metadata.available ? metadata.value : undefined, + + // Make sure the disabled options are always serialized last, so that no + // one using this function can accidentally override the values + ...(metadata.available ? disabledFetchOptions : {}), }; return newOptions as FormattedQueryOptionsResult< diff --git a/site/src/hooks/useEmbeddedMetadata.ts b/site/src/hooks/useEmbeddedMetadata.ts index fd5e8b13454df..73745ae7f47d0 100644 --- a/site/src/hooks/useEmbeddedMetadata.ts +++ b/site/src/hooks/useEmbeddedMetadata.ts @@ -34,9 +34,14 @@ export type MetadataState = Readonly<{ // undefined chosen to signify missing value because unlike null, it isn't a // valid JSON-serializable value. It's impossible to be returned by the API value: T | undefined; - status: "missing" | "loaded" | "deleted"; + available: boolean; }>; +const unavailableState = { + value: undefined, + available: false, +} as const satisfies MetadataState; + export type RuntimeHtmlMetadata = Readonly<{ [Key in MetadataKey]: MetadataState; }>; @@ -78,7 +83,7 @@ export class MetadataManager implements MetadataManagerApi { entitlements: this.registerValue("entitlements"), experiments: this.registerValue("experiments"), "build-info": this.registerValue("build-info"), - regions: this.registerRegionValue("region"), + regions: this.registerRegionValue(), }; } @@ -99,7 +104,7 @@ export class MetadataManager implements MetadataManagerApi { * can be tightened up even further (e.g., adding a type constraint to * parseJson) */ - private registerRegionValue(key: "region"): MetadataState { + private registerRegionValue(): MetadataState { type RegionResponse = | readonly Region[] | Readonly<{ @@ -110,13 +115,14 @@ export class MetadataManager implements MetadataManagerApi { let newEntry: MetadataState; if (!node || value === undefined) { - newEntry = { value: undefined, status: "missing" }; + newEntry = unavailableState; } else if ("regions" in value) { - newEntry = { value: value.regions, status: "loaded" }; + newEntry = { value: value.regions, available: true }; } else { - newEntry = { value, status: "loaded" }; + newEntry = { value, available: true }; } + const key = "regions" satisfies MetadataKey; this.trackedMetadataNodes.set(key, node); return newEntry; } @@ -128,9 +134,9 @@ export class MetadataManager implements MetadataManagerApi { let newEntry: MetadataState; if (!node || value === undefined) { - newEntry = { value: undefined, status: "missing" }; + newEntry = unavailableState; } else { - newEntry = { value, status: "loaded" }; + newEntry = { value, available: true }; } this.trackedMetadataNodes.set(key, node); @@ -177,7 +183,7 @@ export class MetadataManager implements MetadataManagerApi { clearMetadataByKey = (key: MetadataKey): void => { const metadataValue = this.metadata[key]; - if (metadataValue.status === "missing") { + if (!metadataValue.available) { return; } @@ -188,14 +194,7 @@ export class MetadataManager implements MetadataManagerApi { // the value after it's supposed to have been made unavailable metadataNode?.remove(); - type NewState = MetadataState>; - const newState: NewState = { - ...metadataValue, - value: undefined, - status: "deleted", - }; - - this.metadata = { ...this.metadata, [key]: newState }; + this.metadata = { ...this.metadata, [key]: unavailableState }; this.notifySubscriptionsOfStateChange(); }; } From 2a5832267a25c46b135cf199d41822f21bd9d10c Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 3 May 2024 02:20:00 +0000 Subject: [PATCH 12/19] fix: update stray type mismatches --- site/src/hooks/useEmbeddedMetadata.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/site/src/hooks/useEmbeddedMetadata.ts b/site/src/hooks/useEmbeddedMetadata.ts index 73745ae7f47d0..89ca1906990e8 100644 --- a/site/src/hooks/useEmbeddedMetadata.ts +++ b/site/src/hooks/useEmbeddedMetadata.ts @@ -211,8 +211,8 @@ export function makeUseEmbeddedMetadata( // Hook binds re-renders to the memory reference of the entire exposed // metadata object, meaning that even if you only care about one value, // using the hook will cause a component to re-render if the object changes - // at all If this becomes a performance issue down the line, we can look - // into selector functions to minimize re-renders + // at all. If this becomes a performance issue down the line, we can look + // into selector functions to minimize re-renders, but let's wait for now const metadata = useSyncExternalStore( manager.subscribe, manager.getMetadata, From b55abb734385867b2a5244fc00923143b6bc0997 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 3 May 2024 02:20:18 +0000 Subject: [PATCH 13/19] fix: more type fixing --- site/src/modules/dashboard/Navbar/Navbar.tsx | 6 +++++- .../GeneralSettingsPage/GeneralSettingsPage.tsx | 7 +++++-- .../LicensesSettingsPage/LicensesSettingsPage.tsx | 6 +++++- site/src/pages/LoginPage/LoginPage.tsx | 5 ++++- .../TemplateInsightsPage/TemplateInsightsPage.tsx | 7 ++++++- site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx | 5 ++++- 6 files changed, 29 insertions(+), 7 deletions(-) diff --git a/site/src/modules/dashboard/Navbar/Navbar.tsx b/site/src/modules/dashboard/Navbar/Navbar.tsx index d39cfc8417b23..388622fdf7636 100644 --- a/site/src/modules/dashboard/Navbar/Navbar.tsx +++ b/site/src/modules/dashboard/Navbar/Navbar.tsx @@ -3,13 +3,16 @@ import { useQuery } from "react-query"; import { buildInfo } from "api/queries/buildInfo"; import { useAuthenticated } from "contexts/auth/RequireAuth"; import { useProxy } from "contexts/ProxyContext"; +import { useEmbeddedMetadata } from "hooks/useEmbeddedMetadata"; import { useDashboard } from "modules/dashboard/useDashboard"; import { useFeatureVisibility } from "../useFeatureVisibility"; import { NavbarView } from "./NavbarView"; export const Navbar: FC = () => { + const { metadata } = useEmbeddedMetadata(); + const buildInfoQuery = useQuery(buildInfo(metadata["build-info"])); + const { appearance } = useDashboard(); - const buildInfoQuery = useQuery(buildInfo()); const { user: me, permissions, signOut } = useAuthenticated(); const featureVisibility = useFeatureVisibility(); const canViewAuditLog = @@ -18,6 +21,7 @@ export const Navbar: FC = () => { const canViewAllUsers = Boolean(permissions.readAllUsers); const proxyContextValue = useProxy(); const canViewHealth = canViewDeployment; + return ( { const { deploymentValues } = useDeploySettings(); const deploymentDAUsQuery = useQuery(deploymentDAUs()); - const entitlementsQuery = useQuery(entitlements()); - const enabledExperimentsQuery = useQuery(experiments()); const safeExperimentsQuery = useQuery(availableExperiments()); + const { metadata } = useEmbeddedMetadata(); + const entitlementsQuery = useQuery(entitlements(metadata.entitlements)); + const enabledExperimentsQuery = useQuery(experiments(metadata.experiments)); + const safeExperiments = safeExperimentsQuery.data?.safe ?? []; const invalidExperiments = enabledExperimentsQuery.data?.filter((exp) => { diff --git a/site/src/pages/DeploySettingsPage/LicensesSettingsPage/LicensesSettingsPage.tsx b/site/src/pages/DeploySettingsPage/LicensesSettingsPage/LicensesSettingsPage.tsx index 8f172cf1155a1..dcd219c99e8c9 100644 --- a/site/src/pages/DeploySettingsPage/LicensesSettingsPage/LicensesSettingsPage.tsx +++ b/site/src/pages/DeploySettingsPage/LicensesSettingsPage/LicensesSettingsPage.tsx @@ -7,6 +7,7 @@ import { getLicenses, removeLicense } from "api/api"; import { getErrorMessage } from "api/errors"; import { entitlements, refreshEntitlements } from "api/queries/entitlements"; import { displayError, displaySuccess } from "components/GlobalSnackbar/utils"; +import { useEmbeddedMetadata } from "hooks/useEmbeddedMetadata"; import { pageTitle } from "utils/page"; import LicensesSettingsPageView from "./LicensesSettingsPageView"; @@ -15,7 +16,10 @@ const LicensesSettingsPage: FC = () => { const [searchParams, setSearchParams] = useSearchParams(); const success = searchParams.get("success"); const [confettiOn, toggleConfettiOn] = useToggle(false); - const entitlementsQuery = useQuery(entitlements()); + + const { metadata } = useEmbeddedMetadata(); + const entitlementsQuery = useQuery(entitlements(metadata.entitlements)); + const refreshEntitlementsMutation = useMutation( refreshEntitlements(queryClient), ); diff --git a/site/src/pages/LoginPage/LoginPage.tsx b/site/src/pages/LoginPage/LoginPage.tsx index 418a734efbe5d..f05c0b40d981f 100644 --- a/site/src/pages/LoginPage/LoginPage.tsx +++ b/site/src/pages/LoginPage/LoginPage.tsx @@ -5,6 +5,7 @@ import { Navigate, useLocation, useNavigate } from "react-router-dom"; import { buildInfo } from "api/queries/buildInfo"; import { authMethods } from "api/queries/users"; import { useAuthContext } from "contexts/auth/AuthProvider"; +import { useEmbeddedMetadata } from "hooks/useEmbeddedMetadata"; import { getApplicationName } from "utils/appearance"; import { retrieveRedirect } from "utils/redirect"; import { LoginPageView } from "./LoginPageView"; @@ -23,7 +24,9 @@ export const LoginPage: FC = () => { const redirectTo = retrieveRedirect(location.search); const applicationName = getApplicationName(); const navigate = useNavigate(); - const buildInfoQuery = useQuery(buildInfo()); + + const { metadata } = useEmbeddedMetadata(); + const buildInfoQuery = useQuery(buildInfo(metadata["build-info"])); if (isSignedIn) { // If the redirect is going to a workspace application, and we diff --git a/site/src/pages/TemplatePage/TemplateInsightsPage/TemplateInsightsPage.tsx b/site/src/pages/TemplatePage/TemplateInsightsPage/TemplateInsightsPage.tsx index e067425a639c7..5839fb044941e 100644 --- a/site/src/pages/TemplatePage/TemplateInsightsPage/TemplateInsightsPage.tsx +++ b/site/src/pages/TemplatePage/TemplateInsightsPage/TemplateInsightsPage.tsx @@ -53,6 +53,7 @@ import { } from "components/HelpTooltip/HelpTooltip"; import { Loader } from "components/Loader/Loader"; import { UserAvatar } from "components/UserAvatar/UserAvatar"; +import { useEmbeddedMetadata } from "hooks/useEmbeddedMetadata"; import { useTemplateLayoutContext } from "pages/TemplatePage/TemplateLayout"; import { getLatencyColor } from "utils/latency"; import { getTemplatePageTitle } from "../utils"; @@ -91,7 +92,11 @@ export default function TemplateInsightsPage() { const { data: templateInsights } = useQuery(insightsTemplate(insightsFilter)); const { data: userLatency } = useQuery(insightsUserLatency(commonFilters)); const { data: userActivity } = useQuery(insightsUserActivity(commonFilters)); - const { data: entitlementsQuery } = useQuery(entitlements()); + + const { metadata } = useEmbeddedMetadata(); + const { data: entitlementsQuery } = useQuery( + entitlements(metadata.entitlements), + ); return ( <> diff --git a/site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx b/site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx index 5df3e18bc6fe6..e460c7163c1e6 100644 --- a/site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx +++ b/site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx @@ -27,6 +27,7 @@ import { displayError } from "components/GlobalSnackbar/utils"; import { MemoizedInlineMarkdown } from "components/Markdown/Markdown"; import { Stack } from "components/Stack/Stack"; import { useAuthenticated } from "contexts/auth/RequireAuth"; +import { useEmbeddedMetadata } from "hooks/useEmbeddedMetadata"; import { useWorkspaceBuildLogs } from "hooks/useWorkspaceBuildLogs"; import { useFeatureVisibility } from "modules/dashboard/useFeatureVisibility"; import { pageTitle } from "utils/page"; @@ -48,9 +49,11 @@ export const WorkspaceReadyPage: FC = ({ template, permissions, }) => { + const { metadata } = useEmbeddedMetadata(); + const buildInfoQuery = useQuery(buildInfo(metadata["build-info"])); const navigate = useNavigate(); const queryClient = useQueryClient(); - const buildInfoQuery = useQuery(buildInfo()); + const featureVisibility = useFeatureVisibility(); if (workspace === undefined) { throw Error("Workspace is undefined"); From c45e1b7ead2b7b464d40bdbfa36622e05f817c73 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 3 May 2024 02:29:39 +0000 Subject: [PATCH 14/19] fix: resolve illegal invocation error --- site/src/hooks/useEmbeddedMetadata.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/site/src/hooks/useEmbeddedMetadata.ts b/site/src/hooks/useEmbeddedMetadata.ts index 89ca1906990e8..9ee2540203409 100644 --- a/site/src/hooks/useEmbeddedMetadata.ts +++ b/site/src/hooks/useEmbeddedMetadata.ts @@ -47,7 +47,6 @@ export type RuntimeHtmlMetadata = Readonly<{ }>; type SubscriptionCallback = (metadata: RuntimeHtmlMetadata) => void; -type QuerySelector = typeof document.querySelector; type ParseJsonResult = Readonly< | { @@ -67,13 +66,11 @@ interface MetadataManagerApi { } export class MetadataManager implements MetadataManagerApi { - private readonly querySelector: QuerySelector; private readonly subscriptions: Set; private readonly trackedMetadataNodes: Map; private metadata: RuntimeHtmlMetadata; - constructor(querySelector?: QuerySelector) { - this.querySelector = querySelector ?? document.querySelector; + constructor() { this.subscriptions = new Set(); this.trackedMetadataNodes = new Map(); @@ -144,7 +141,7 @@ export class MetadataManager implements MetadataManagerApi { } private parseJson(key: string): ParseJsonResult { - const node = this.querySelector(`meta[property=${key}]`); + const node = document.querySelector(`meta[property=${key}]`); if (!node) { return { value: undefined, node: null }; } From 2a63c1dbc1a452b564c5587916ec74545962e4da Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 3 May 2024 03:52:50 +0000 Subject: [PATCH 15/19] fix: get metadata issue resolved --- site/src/api/queries/users.ts | 21 ++++++++++++++++++++- site/src/contexts/auth/AuthProvider.tsx | 11 +++-------- site/src/hooks/useEmbeddedMetadata.ts | 6 ++++-- 3 files changed, 27 insertions(+), 11 deletions(-) diff --git a/site/src/api/queries/users.ts b/site/src/api/queries/users.ts index 1efbc22909704..b188c8b4f340f 100644 --- a/site/src/api/queries/users.ts +++ b/site/src/api/queries/users.ts @@ -14,7 +14,10 @@ import type { User, GenerateAPIKeyResponse, } from "api/typesGenerated"; -import type { MetadataState } from "hooks/useEmbeddedMetadata"; +import { + defaultMetadataManager, + type MetadataState, +} from "hooks/useEmbeddedMetadata"; import type { UsePaginatedQueryOptions } from "hooks/usePaginatedQuery"; import { prepareQuery } from "utils/filters"; import { getAuthorizationKey } from "./authCheck"; @@ -187,6 +190,22 @@ export const logout = (queryClient: QueryClient) => { return { mutationFn: API.logout, onSuccess: () => { + /** + * 2024-05-02 - If we persist any form of user data after the user logs + * out, that will continue to seed the React Query cache, creating + * "impossible" states where we'll have data we're not supposed to have. + * + * This has caused issues where logging out will instantly throw a + * completely uncaught runtime rendering error. Worse yet, the error only + * exists when serving the site from the Go backend (the JS environment + * has zero issues because it doesn't have access to the metadata). These + * errors can only be caught with E2E tests. + * + * Deleting the user data will mean that all future requests have to take + * a full roundtrip, but better that than having the logout button blow + * the entire app up. + */ + defaultMetadataManager.clearMetadataByKey("user"); queryClient.removeQueries(); }, }; diff --git a/site/src/contexts/auth/AuthProvider.tsx b/site/src/contexts/auth/AuthProvider.tsx index b085bb6e6b2cf..767606e8d605f 100644 --- a/site/src/contexts/auth/AuthProvider.tsx +++ b/site/src/contexts/auth/AuthProvider.tsx @@ -1,7 +1,7 @@ import { - createContext, type FC, type PropsWithChildren, + createContext, useCallback, useContext, } from "react"; @@ -43,7 +43,7 @@ export const AuthContext = createContext( ); export const AuthProvider: FC = ({ children }) => { - const { metadata, clearMetadataByKey } = useEmbeddedMetadata(); + const { metadata } = useEmbeddedMetadata(); const userMetadataState = metadata.user; const meOptions = me(userMetadataState); @@ -60,14 +60,9 @@ export const AuthProvider: FC = ({ children }) => { login({ checks: permissionsToCheck }, queryClient), ); - const logoutMutation = useMutation({ - ...logout(queryClient), - onSuccess: () => clearMetadataByKey("user"), - }); - + const logoutMutation = useMutation(logout(queryClient)); const updateProfileMutation = useMutation({ ...updateProfileOptions("me"), - onSuccess: (user) => { queryClient.setQueryData(meOptions.queryKey, user); displaySuccess("Updated settings."); diff --git a/site/src/hooks/useEmbeddedMetadata.ts b/site/src/hooks/useEmbeddedMetadata.ts index 9ee2540203409..3d1f55d87df81 100644 --- a/site/src/hooks/useEmbeddedMetadata.ts +++ b/site/src/hooks/useEmbeddedMetadata.ts @@ -226,5 +226,7 @@ export function makeUseEmbeddedMetadata( }; } -const defaultManager = new MetadataManager(); -export const useEmbeddedMetadata = makeUseEmbeddedMetadata(defaultManager); +export const defaultMetadataManager = new MetadataManager(); +export const useEmbeddedMetadata = makeUseEmbeddedMetadata( + defaultMetadataManager, +); From 4d3b1558f93154378ce153ccfa3fda69e6137d5e Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 3 May 2024 12:08:02 +0000 Subject: [PATCH 16/19] fix: update comments --- site/src/api/queries/users.ts | 4 ++-- site/src/hooks/useEmbeddedMetadata.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/site/src/api/queries/users.ts b/site/src/api/queries/users.ts index b188c8b4f340f..ded7c7a5f29c8 100644 --- a/site/src/api/queries/users.ts +++ b/site/src/api/queries/users.ts @@ -202,8 +202,8 @@ export const logout = (queryClient: QueryClient) => { * errors can only be caught with E2E tests. * * Deleting the user data will mean that all future requests have to take - * a full roundtrip, but better that than having the logout button blow - * the entire app up. + * a full roundtrip, but this still felt like the best way to ensure that + * manually logging out doesn't blow the entire app up. */ defaultMetadataManager.clearMetadataByKey("user"); queryClient.removeQueries(); diff --git a/site/src/hooks/useEmbeddedMetadata.ts b/site/src/hooks/useEmbeddedMetadata.ts index 3d1f55d87df81..bcfc00a74ee20 100644 --- a/site/src/hooks/useEmbeddedMetadata.ts +++ b/site/src/hooks/useEmbeddedMetadata.ts @@ -15,8 +15,8 @@ import type { * * If you want to add new metadata in a type-safe way, add it to this type. * Each key should the name of the "property" attribute that will be used on the - * HTML elements themselves, and the values should be the data you get back from - * parsing those element's text content + * HTML elements themselves (current selector is meta[property=${key}]), and the + * values should be the data you get back from parsing those element's content */ type AvailableMetadata = Readonly<{ user: User; From 8067e77175f09b551c282cfb47e685a67694c527 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 3 May 2024 13:23:20 +0000 Subject: [PATCH 17/19] chore: add unit tests for MetadataManager --- site/src/hooks/useEmbeddedMetadata.test.ts | 234 +++++++++++++++++++++ site/src/hooks/useEmbeddedMetadata.ts | 19 +- 2 files changed, 246 insertions(+), 7 deletions(-) create mode 100644 site/src/hooks/useEmbeddedMetadata.test.ts diff --git a/site/src/hooks/useEmbeddedMetadata.test.ts b/site/src/hooks/useEmbeddedMetadata.test.ts new file mode 100644 index 0000000000000..54091c16fb557 --- /dev/null +++ b/site/src/hooks/useEmbeddedMetadata.test.ts @@ -0,0 +1,234 @@ +/** + * Qualities I want to test: + * 1. Logic is able to detect when specific types of metadata are available + * 2. Logic detects when metadata just doesn't exist + * 2. Lets external systems subscribe to React changes + * 3. Hook lets external systems know when metadata is deleted + * 4. Metadata is treated as an immutable value when anything is deleted + * + * Setup helpers I'll need + * 1. A way to populate the DOM with fake metadata nodes + */ +import { act, renderHook } from "@testing-library/react"; +import type { Region } from "api/typesGenerated"; +import { + MockAppearanceConfig, + MockBuildInfo, + MockEntitlements, + MockExperiments, + MockUser, +} from "testHelpers/entities"; +import { + type MetadataKey, + type MetadataValue, + type RuntimeHtmlMetadata, + DEFAULT_METADATA_KEY, + makeUseEmbeddedMetadata, + MetadataManager, + useEmbeddedMetadata, +} from "./useEmbeddedMetadata"; + +// Make sure that no matter what happens in the tests, all metadata is +// eventually deleted +const allAppendedNodes = new Set>(); +afterAll(() => { + allAppendedNodes.forEach((tracker) => { + tracker.forEach((node) => node.remove()); + }); +}); + +// Using empty array for now, because we don't have a separate mock regions +// value, but it's still good enough for the tests because it's truthy +const MockRegions: readonly Region[] = []; + +const mockDataForTags = { + appearance: MockAppearanceConfig, + "build-info": MockBuildInfo, + entitlements: MockEntitlements, + experiments: MockExperiments, + user: MockUser, + regions: MockRegions, +} as const satisfies Record; + +const emptyMetadata: RuntimeHtmlMetadata = { + appearance: { + available: false, + value: undefined, + }, + "build-info": { + available: false, + value: undefined, + }, + entitlements: { + available: false, + value: undefined, + }, + experiments: { + available: false, + value: undefined, + }, + regions: { + available: false, + value: undefined, + }, + user: { + available: false, + value: undefined, + }, +}; + +const populatedMetadata: RuntimeHtmlMetadata = { + appearance: { + available: true, + value: MockAppearanceConfig, + }, + "build-info": { + available: true, + value: MockBuildInfo, + }, + entitlements: { + available: true, + value: MockEntitlements, + }, + experiments: { + available: true, + value: MockExperiments, + }, + regions: { + available: true, + value: MockRegions, + }, + user: { + available: true, + value: MockUser, + }, +}; + +function seedInitialMetadata(metadataKey: string): () => void { + // Enforcing this to make sure that even if we start to adopt more concurrent + // tests through Vitest (or similar), there's no risk of global state causing + // weird, hard-to-test false positives/negatives with other tests + if (metadataKey === DEFAULT_METADATA_KEY) { + throw new Error( + "Please ensure that the key you provide does not match the key used throughout the majority of the application", + ); + } + + const trackedNodes = new Set(); + allAppendedNodes.add(trackedNodes); + + for (const metadataName in mockDataForTags) { + // Serializing first because that's the part that can fail; want to fail + // early if possible + const value = mockDataForTags[metadataName as keyof typeof mockDataForTags]; + const serialized = JSON.stringify(value); + + const newNode = document.createElement("meta"); + newNode.setAttribute(metadataKey, metadataName); + newNode.setAttribute("content", serialized); + document.head.append(newNode); + + trackedNodes.add(newNode); + } + + return () => { + trackedNodes.forEach((node) => node.remove()); + }; +} + +function renderMetadataHook(metadataKey: string) { + const manager = new MetadataManager(metadataKey); + const hook = makeUseEmbeddedMetadata(manager); + + return { + ...renderHook(hook), + manager, + }; +} + +// Just to be on the safe side, probably want to make sure that each test case +// is set up with a unique key +describe(useEmbeddedMetadata.name, () => { + it("Correctly detects when metadata is missing in the HTML page", () => { + const key = "cat"; + + // Deliberately avoid seeding any metadata + const { result } = renderMetadataHook(key); + expect(result.current.metadata).toEqual(emptyMetadata); + }); + + it("Can detect when metadata exists in the HTML", () => { + const key = "dog"; + + const cleanupTags = seedInitialMetadata(key); + const { result } = renderMetadataHook(key); + expect(result.current.metadata).toEqual( + populatedMetadata, + ); + + cleanupTags(); + }); + + it("Lets external systems (including React) subscribe to when metadata values are deleted", () => { + const key = "bird"; + const tag1 = "user"; + const tag2 = "appearance"; + + const cleanupTags = seedInitialMetadata(key); + const { result: reactResult, manager } = renderMetadataHook(key); + + const nonReactSubscriber = jest.fn(); + manager.subscribe(nonReactSubscriber); + + const expectedUpdate1: RuntimeHtmlMetadata = { + ...populatedMetadata, + [tag1]: { + available: false, + value: undefined, + }, + }; + + // Test that updates work when run directly through the metadata manager + // itself + act(() => manager.clearMetadataByKey(tag1)); + expect(reactResult.current.metadata).toEqual(expectedUpdate1); + expect(nonReactSubscriber).toBeCalledWith( + expect.objectContaining(expectedUpdate1), + ); + + nonReactSubscriber.mockClear(); + const expectedUpdate2: RuntimeHtmlMetadata = { + ...expectedUpdate1, + [tag2]: { + available: false, + value: undefined, + }, + }; + + // Test that updates work when calling the convenience function exposed + // through the React hooks + act(() => reactResult.current.clearMetadataByKey("appearance")); + expect(reactResult.current.metadata).toEqual(expectedUpdate2); + expect(nonReactSubscriber).toBeCalledWith( + expect.objectContaining(expectedUpdate2), + ); + + cleanupTags(); + }); + + it("Always treats metadata as immutable values during all deletions", () => { + const key = "hamster"; + const tagToDelete = "user"; + + const cleanupTags = seedInitialMetadata(key); + const { result } = renderMetadataHook(key); + + const initialResult = result.current.metadata; + act(() => result.current.clearMetadataByKey(tagToDelete)); + const newResult = result.current.metadata; + + // Need to use toBe, not toEqual here + expect(initialResult).not.toBe(newResult); + cleanupTags(); + }); +}); diff --git a/site/src/hooks/useEmbeddedMetadata.ts b/site/src/hooks/useEmbeddedMetadata.ts index bcfc00a74ee20..2701523374ccd 100644 --- a/site/src/hooks/useEmbeddedMetadata.ts +++ b/site/src/hooks/useEmbeddedMetadata.ts @@ -8,6 +8,8 @@ import type { User, } from "api/typesGenerated"; +export const DEFAULT_METADATA_KEY = "property"; + /** * This is the set of values that are currently being exposed to the React * application during production. These values are embedded via the Go server, @@ -27,7 +29,7 @@ type AvailableMetadata = Readonly<{ "build-info": BuildInfoResponse; }>; -type MetadataKey = keyof AvailableMetadata; +export type MetadataKey = keyof AvailableMetadata; export type MetadataValue = AvailableMetadata[MetadataKey]; export type MetadataState = Readonly<{ @@ -66,11 +68,14 @@ interface MetadataManagerApi { } export class MetadataManager implements MetadataManagerApi { + private readonly metadataKey: string; private readonly subscriptions: Set; private readonly trackedMetadataNodes: Map; + private metadata: RuntimeHtmlMetadata; - constructor() { + constructor(metadataKey?: string) { + this.metadataKey = metadataKey ?? DEFAULT_METADATA_KEY; this.subscriptions = new Set(); this.trackedMetadataNodes = new Map(); @@ -141,7 +146,7 @@ export class MetadataManager implements MetadataManagerApi { } private parseJson(key: string): ParseJsonResult { - const node = document.querySelector(`meta[property=${key}]`); + const node = document.querySelector(`meta[${this.metadataKey}=${key}]`); if (!node) { return { value: undefined, node: null }; } @@ -206,10 +211,10 @@ export function makeUseEmbeddedMetadata( ): () => UseEmbeddedMetadataResult { return function useEmbeddedMetadata(): UseEmbeddedMetadataResult { // Hook binds re-renders to the memory reference of the entire exposed - // metadata object, meaning that even if you only care about one value, - // using the hook will cause a component to re-render if the object changes - // at all. If this becomes a performance issue down the line, we can look - // into selector functions to minimize re-renders, but let's wait for now + // metadata object, meaning that even if you only care about one metadata + // property, the hook will cause a component to re-render if the object + // changes at all. If this becomes a performance issue down the line, we can + // look into selector functions to minimize re-renders, but let's wait const metadata = useSyncExternalStore( manager.subscribe, manager.getMetadata, From 5e6e974abf64a1022396041fc28aa641a0320e05 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 3 May 2024 13:43:38 +0000 Subject: [PATCH 18/19] fix: beef up tests --- site/src/hooks/useEmbeddedMetadata.test.ts | 38 ++++++++++------------ site/src/hooks/useEmbeddedMetadata.ts | 5 +-- 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/site/src/hooks/useEmbeddedMetadata.test.ts b/site/src/hooks/useEmbeddedMetadata.test.ts index 54091c16fb557..4cf6259455176 100644 --- a/site/src/hooks/useEmbeddedMetadata.test.ts +++ b/site/src/hooks/useEmbeddedMetadata.test.ts @@ -1,16 +1,5 @@ -/** - * Qualities I want to test: - * 1. Logic is able to detect when specific types of metadata are available - * 2. Logic detects when metadata just doesn't exist - * 2. Lets external systems subscribe to React changes - * 3. Hook lets external systems know when metadata is deleted - * 4. Metadata is treated as an immutable value when anything is deleted - * - * Setup helpers I'll need - * 1. A way to populate the DOM with fake metadata nodes - */ import { act, renderHook } from "@testing-library/react"; -import type { Region } from "api/typesGenerated"; +import type { Region, User } from "api/typesGenerated"; import { MockAppearanceConfig, MockBuildInfo, @@ -192,9 +181,7 @@ describe(useEmbeddedMetadata.name, () => { // itself act(() => manager.clearMetadataByKey(tag1)); expect(reactResult.current.metadata).toEqual(expectedUpdate1); - expect(nonReactSubscriber).toBeCalledWith( - expect.objectContaining(expectedUpdate1), - ); + expect(nonReactSubscriber).toBeCalledWith(expectedUpdate1); nonReactSubscriber.mockClear(); const expectedUpdate2: RuntimeHtmlMetadata = { @@ -209,13 +196,13 @@ describe(useEmbeddedMetadata.name, () => { // through the React hooks act(() => reactResult.current.clearMetadataByKey("appearance")); expect(reactResult.current.metadata).toEqual(expectedUpdate2); - expect(nonReactSubscriber).toBeCalledWith( - expect.objectContaining(expectedUpdate2), - ); + expect(nonReactSubscriber).toBeCalledWith(expectedUpdate2); cleanupTags(); }); + // Need to guarantee this, or else we could have a good number of bugs in the + // React UI it("Always treats metadata as immutable values during all deletions", () => { const key = "hamster"; const tagToDelete = "user"; @@ -226,9 +213,20 @@ describe(useEmbeddedMetadata.name, () => { const initialResult = result.current.metadata; act(() => result.current.clearMetadataByKey(tagToDelete)); const newResult = result.current.metadata; - - // Need to use toBe, not toEqual here expect(initialResult).not.toBe(newResult); + + // Mutate the initial result, and make sure the change doesn't propagate to + // the updated result + const mutableUser = initialResult.user as { + available: boolean; + value: User | undefined; + }; + + mutableUser.available = false; + mutableUser.value = undefined; + expect(mutableUser).toEqual(newResult.user); + expect(mutableUser).not.toBe(newResult.user); + cleanupTags(); }); }); diff --git a/site/src/hooks/useEmbeddedMetadata.ts b/site/src/hooks/useEmbeddedMetadata.ts index 2701523374ccd..61529fe70fa52 100644 --- a/site/src/hooks/useEmbeddedMetadata.ts +++ b/site/src/hooks/useEmbeddedMetadata.ts @@ -16,9 +16,10 @@ export const DEFAULT_METADATA_KEY = "property"; * so they will never exist when using a JavaScript runtime for the backend * * If you want to add new metadata in a type-safe way, add it to this type. - * Each key should the name of the "property" attribute that will be used on the - * HTML elements themselves (current selector is meta[property=${key}]), and the + * Each key should be the name of the "property" attribute that will be used on + * the HTML meta elements themselves (e.g., meta[property=${key}]), and the * values should be the data you get back from parsing those element's content + * attributes */ type AvailableMetadata = Readonly<{ user: User; From 772b96f9a7e167ca5295211b1074f598a5a291a9 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 3 May 2024 14:00:44 +0000 Subject: [PATCH 19/19] fix: update typo in tests --- site/src/hooks/useEmbeddedMetadata.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/site/src/hooks/useEmbeddedMetadata.test.ts b/site/src/hooks/useEmbeddedMetadata.test.ts index 4cf6259455176..697e924122ed9 100644 --- a/site/src/hooks/useEmbeddedMetadata.test.ts +++ b/site/src/hooks/useEmbeddedMetadata.test.ts @@ -160,8 +160,8 @@ describe(useEmbeddedMetadata.name, () => { it("Lets external systems (including React) subscribe to when metadata values are deleted", () => { const key = "bird"; - const tag1 = "user"; - const tag2 = "appearance"; + const tag1: MetadataKey = "user"; + const tag2: MetadataKey = "appearance"; const cleanupTags = seedInitialMetadata(key); const { result: reactResult, manager } = renderMetadataHook(key); @@ -194,7 +194,7 @@ describe(useEmbeddedMetadata.name, () => { // Test that updates work when calling the convenience function exposed // through the React hooks - act(() => reactResult.current.clearMetadataByKey("appearance")); + act(() => reactResult.current.clearMetadataByKey(tag2)); expect(reactResult.current.metadata).toEqual(expectedUpdate2); expect(nonReactSubscriber).toBeCalledWith(expectedUpdate2); @@ -205,7 +205,7 @@ describe(useEmbeddedMetadata.name, () => { // React UI it("Always treats metadata as immutable values during all deletions", () => { const key = "hamster"; - const tagToDelete = "user"; + const tagToDelete: MetadataKey = "user"; const cleanupTags = seedInitialMetadata(key); const { result } = renderMetadataHook(key);