fix(plugin-multi-tenant): selector could become hidden (#13134)

The selector could become hidden by:
- logging in with a user that only has 1 tenant
- logging out
- logging in with a user that has more than 1 tenant

Simplifies useEffect usage. Adds e2e test for this case.

packages/plugin-multi-tenant/src/providers/TenantSelectionProvider/index.client.tsx

@@ -86,6 +86,8 @@ export const TenantSelectionProviderClient = ({
   const { user } = useAuth()
   const { config } = useConfig()
   const userID = React.useMemo(() => user?.id, [user?.id])
+  const prevUserID = React.useRef(userID)
+  const userChanged = userID !== prevUserID.current
   const [tenantOptions, setTenantOptions] = React.useState<OptionObject[]>(
     () => tenantOptionsFromProps,
   )
@@ -113,15 +115,15 @@ export const TenantSelectionProviderClient = ({
           // users with multiple tenants can clear the tenant selection
           setSelectedTenantID(undefined)
           deleteCookie()
-        } else {
+        } else if (tenantOptions[0]) {
           // if there is only one tenant, force the selection of that tenant
-          setSelectedTenantID(tenantOptions[0]?.value)
-          setCookie(String(tenantOptions[0]?.value))
+          setSelectedTenantID(tenantOptions[0].value)
+          setCookie(String(tenantOptions[0].value))
         }
       } else if (!tenantOptions.find((option) => option.value === id)) {
         // if the tenant is not valid, set the first tenant as selected
-        if (tenantOptions?.[0]?.value) {
-          setTenant({ id: tenantOptions[0].value, refresh: true })
+        if (tenantOptions[0]?.value) {
+          setTenant({ id: tenantOptions[0]?.value, refresh: true })
         } else {
           setTenant({ id: undefined, refresh: true })
         }
@@ -149,18 +151,23 @@ export const TenantSelectionProviderClient = ({
 
       const result = await req.json()
 
-      if (result.docs) {
+      if (result.docs && userID) {
         setTenantOptions(
           result.docs.map((doc: Record<string, number | string>) => ({
             label: doc[useAsTitle],
             value: doc.id,
           })),
         )
+
+        if (result.totalDocs === 1) {
+          setSelectedTenantID(result.docs[0].id)
+          setCookie(String(result.docs[0].id))
+        }
       }
     } catch (e) {
       toast.error(`Error fetching tenants`)
     }
-  }, [config.serverURL, config.routes.api, tenantsCollectionSlug, useAsTitle])
+  }, [config.serverURL, config.routes.api, tenantsCollectionSlug, useAsTitle, setCookie, userID])
 
   const updateTenants = React.useCallback<ContextType['updateTenants']>(
     ({ id, label }) => {
@@ -182,44 +189,21 @@ export const TenantSelectionProviderClient = ({
   )
 
   React.useEffect(() => {
-    if (userID && !tenantCookie) {
-      if (tenantOptionsFromProps.length === 1) {
-        // Users with no cookie set and only 1 tenant should set that tenant automatically
-        setTenant({ id: tenantOptionsFromProps[0]?.value, refresh: true })
-        setTenantOptions(tenantOptionsFromProps)
-      } else if (
-        (!tenantOptions || tenantOptions.length === 0) &&
-        tenantOptionsFromProps.length > 0
-      ) {
-        // If there are no tenant options, set them from the props
-        setTenantOptions(tenantOptionsFromProps)
-      }
-    } else if (userID && tenantCookie) {
-      if ((!tenantOptions || tenantOptions.length === 0) && tenantOptionsFromProps.length > 0) {
-        // If there are no tenant options, set them from the props
-        setTenantOptions(tenantOptionsFromProps)
+    if (userChanged) {
+      if (userID) {
+        // user logging in
+        void syncTenants()
+      } else {
+        // user logging out
+        setSelectedTenantID(undefined)
+        deleteCookie()
+        if (tenantOptions.length > 0) {
+          setTenantOptions([])
+        }
       }
+      prevUserID.current = userID
     }
-  }, [
-    initialValue,
-    selectedTenantID,
-    tenantCookie,
-    userID,
-    setTenant,
-    tenantOptionsFromProps,
-    tenantOptions,
-  ])
-
-  React.useEffect(() => {
-    if (!userID && tenantCookie) {
-      // User is not logged in, but has a tenant cookie, delete it
-      deleteCookie()
-      setSelectedTenantID(undefined)
-    } else if (userID) {
-      // User changed, refresh
-      router.refresh()
-    }
-  }, [userID, tenantCookie, deleteCookie, router])
+  }, [userID, userChanged, syncTenants, deleteCookie, tenantOptions])
 
   return (
     <span

test/helpers.ts

@@ -15,17 +15,11 @@ import shelljs from 'shelljs'
 import { setTimeout } from 'timers/promises'
 
 import { devUser } from './credentials.js'
+import { openNav } from './helpers/e2e/toggleNav.js'
 import { POLL_TOPASS_TIMEOUT } from './playwright.config.js'
 
 type AdminRoutes = NonNullable<Config['admin']>['routes']
 
-type FirstRegisterArgs = {
-  customAdminRoutes?: AdminRoutes
-  customRoutes?: Config['routes']
-  page: Page
-  serverURL: string
-}
-
 type LoginArgs = {
   customAdminRoutes?: AdminRoutes
   customRoutes?: Config['routes']
@@ -167,18 +161,55 @@ export async function throttleTest({
   return client
 }
 
-export async function firstRegister(args: FirstRegisterArgs): Promise<void> {
-  const { customAdminRoutes, customRoutes, page, serverURL } = args
+/**
+ * Logs a user in by navigating via click-ops instead of using page.goto()
+ */
+export async function loginClientSide(args: LoginArgs): Promise<void> {
+  const { customAdminRoutes, customRoutes, data = devUser, page, serverURL } = args
+  const {
+    routes: { admin: incomingAdminRoute } = {},
+    admin: { routes: { login: incomingLoginRoute, createFirstUser } = {} },
+  } = getRoutes({ customAdminRoutes, customRoutes })
 
-  const { routes: { admin: adminRoute } = {} } = getRoutes({ customAdminRoutes, customRoutes })
+  const adminRoute = formatAdminURL({ serverURL, adminRoute: incomingAdminRoute, path: '' })
+  const loginRoute = formatAdminURL({
+    serverURL,
+    adminRoute: incomingAdminRoute,
+    path: incomingLoginRoute,
+  })
+  const createFirstUserRoute = formatAdminURL({
+    serverURL,
+    adminRoute: incomingAdminRoute,
+    path: createFirstUser,
+  })
 
-  await page.goto(`${serverURL}${adminRoute}`)
-  await page.fill('#field-email', devUser.email)
-  await page.fill('#field-password', devUser.password)
-  await page.fill('#field-confirm-password', devUser.password)
+  if ((await page.locator('#nav-toggler').count()) > 0) {
+    // a user is already logged in - log them out
+    await openNav(page)
+    await expect(page.locator('.nav__controls [aria-label="Log out"]')).toBeVisible()
+    await page.locator('.nav__controls [aria-label="Log out"]').click()
+    await page.waitForURL(loginRoute)
+  }
+
+  await wait(500)
+  await page.fill('#field-email', data.email)
+  await page.fill('#field-password', data.password)
   await wait(500)
   await page.click('[type=submit]')
-  await page.waitForURL(`${serverURL}${adminRoute}`)
+
+  await expect(page.locator('.step-nav__home')).toBeVisible()
+  if ((await page.locator('a.step-nav__home').count()) > 0) {
+    await page.locator('a.step-nav__home').click()
+  }
+
+  await page.waitForURL(adminRoute)
+
+  await expect(() => expect(page.url()).not.toContain(loginRoute)).toPass({
+    timeout: POLL_TOPASS_TIMEOUT,
+  })
+  await expect(() => expect(page.url()).not.toContain(createFirstUserRoute)).toPass({
+    timeout: POLL_TOPASS_TIMEOUT,
+  })
 }
 
 export async function login(args: LoginArgs): Promise<void> {

test/helpers/e2e/toggleNav.ts

@@ -1,12 +1,16 @@
 import type { Page } from '@playwright/test'
 
 import { expect } from '@playwright/test'
-import { wait } from 'payload/shared'
 
 export async function openNav(page: Page): Promise<void> {
   // wait for the preferences/media queries to either open or close the nav
-  // TODO: remove this in the future when nav state can be initialized properly
-  await wait(250)
+  await expect(page.locator('.template-default--nav-hydrated')).toBeVisible()
+
+  // close all open modals
+  const dialogs = await page.locator('dialog[open]').elementHandles()
+  for (let i = 0; i < dialogs.length; i++) {
+    await page.keyboard.press('Escape')
+  }
 
   // check to see if the nav is already open and if not, open it
   // use the `--nav-open` modifier class to check if the nav is open
@@ -17,5 +21,6 @@ export async function openNav(page: Page): Promise<void> {
 
   // playwright: get first element with .nav-toggler which is VISIBLE (not hidden), could be 2 elements with .nav-toggler on mobile and desktop but only one is visible
   await page.locator('.nav-toggler >> visible=true').click()
+  await expect(page.locator('.nav--nav-animate[inert], .nav--nav-hydrated[inert]')).toBeHidden()
   await expect(page.locator('.template-default.template-default--nav-open')).toBeVisible()
 }

test/next.config.mjs

@@ -14,6 +14,9 @@ const withBundleAnalyzer = bundleAnalyzer({
 export default withBundleAnalyzer(
   withPayload(
     {
+      devIndicators: {
+        position: 'bottom-right',
+      },
       eslint: {
         ignoreDuringBuilds: true,
       },

test/plugin-multi-tenant/e2e.spec.ts

@@ -10,7 +10,7 @@ import type { Config } from './payload-types.js'
 import {
   ensureCompilationIsDone,
   initPageConsoleErrorCatch,
-  login,
+  loginClientSide,
   saveDocAndAssert,
 } from '../helpers.js'
 import { AdminUrlUtil } from '../helpers/adminUrlUtil.js'
@@ -62,7 +62,27 @@ test.describe('Multi Tenant', () => {
 
   test.describe('tenant selector', () => {
     test('should populate tenant selector on login', async () => {
-      await login({
+      await loginClientSide({
+        page,
+        serverURL,
+        data: credentials.admin,
+      })
+
+      await expect
+        .poll(async () => {
+          return (await getTenantOptions({ page })).sort()
+        })
+        .toEqual(['Blue Dog', 'Steel Cat', 'Anchor Bar'].sort())
+    })
+
+    test('should populate the tenant selector after logout with 1 tenant user', async () => {
+      await loginClientSide({
+        page,
+        serverURL,
+        data: credentials.blueDog,
+      })
+
+      await loginClientSide({
         page,
         serverURL,
         data: credentials.admin,
@@ -76,7 +96,7 @@ test.describe('Multi Tenant', () => {
     })
 
     test('should show all tenants for userHasAccessToAllTenants users', async () => {
-      await login({
+      await loginClientSide({
         page,
         serverURL,
         data: credentials.admin,
@@ -90,7 +110,7 @@ test.describe('Multi Tenant', () => {
     })
 
     test('should only show users assigned tenants', async () => {
-      await login({
+      await loginClientSide({
         page,
         serverURL,
         data: credentials.owner,
@@ -106,7 +126,7 @@ test.describe('Multi Tenant', () => {
 
   test.describe('Base List Filter', () => {
     test('should show all tenant items when tenant selector is empty', async () => {
-      await login({
+      await loginClientSide({
         page,
         serverURL,
         data: credentials.admin,
@@ -127,7 +147,7 @@ test.describe('Multi Tenant', () => {
       ).toBeVisible()
     })
     test('should show filtered tenant items when tenant selector is set', async () => {
-      await login({
+      await loginClientSide({
         page,
         serverURL,
         data: credentials.admin,
@@ -154,7 +174,7 @@ test.describe('Multi Tenant', () => {
 
   test.describe('globals', () => {
     test('should redirect list view to edit view', async () => {
-      await login({
+      await loginClientSide({
         page,
         serverURL,
         data: credentials.admin,
@@ -164,7 +184,7 @@ test.describe('Multi Tenant', () => {
     })
 
     test('should redirect from create to edit view when tenant already has content', async () => {
-      await login({
+      await loginClientSide({
         page,
         serverURL,
         data: credentials.admin,
@@ -179,7 +199,7 @@ test.describe('Multi Tenant', () => {
     })
 
     test('should prompt leave without saving changes modal when switching tenants', async () => {
-      await login({
+      await loginClientSide({
         page,
         serverURL,
         data: credentials.admin,
@@ -206,12 +226,16 @@ test.describe('Multi Tenant', () => {
           'Your changes have not been saved. If you leave now, you will lose your changes.',
         ),
       ).toBeVisible()
+
+      await confirmationModal.locator('#confirm-action').click()
+      await expect(page.locator('#confirm-leave-without-saving')).toBeHidden()
+      await page.locator('#nav-food-items').click()
     })
   })
 
   test.describe('documents', () => {
     test('should set tenant upon entering', async () => {
-      await login({
+      await loginClientSide({
         page,
         serverURL,
         data: credentials.admin,
@@ -238,7 +262,7 @@ test.describe('Multi Tenant', () => {
     })
 
     test('should prompt for confirmation upon tenant switching', async () => {
-      await login({
+      await loginClientSide({
         page,
         serverURL,
         data: credentials.admin,
@@ -268,7 +292,7 @@ test.describe('Multi Tenant', () => {
 
   test.describe('tenants', () => {
     test('should update the tenant name in the selector when editing a tenant', async () => {
-      await login({
+      await loginClientSide({
         page,
         serverURL,
         data: credentials.admin,
@@ -303,7 +327,7 @@ test.describe('Multi Tenant', () => {
     })
 
     test('should add tenant to the selector when creating a new tenant', async () => {
-      await login({
+      await loginClientSide({
         page,
         serverURL,
         data: credentials.admin,