diff --git a/.changeset/mosaic-accepted-invitation-switch.md b/.changeset/mosaic-accepted-invitation-switch.md new file mode 100644 index 00000000000..5a63154cf76 --- /dev/null +++ b/.changeset/mosaic-accepted-invitation-switch.md @@ -0,0 +1,5 @@ +--- +'@clerk/mosaic': patch +--- + +An organization joined by accepting an invitation in `UserButton` or `OrganizationSwitcher` now stays listed right away, even before it appears in the loaded memberships, and selecting it follows `afterSelectOrganizationUrl`. diff --git a/packages/mosaic/AGENTS.md b/packages/mosaic/AGENTS.md index 160344075b8..9579cf5d32d 100644 --- a/packages/mosaic/AGENTS.md +++ b/packages/mosaic/AGENTS.md @@ -31,27 +31,28 @@ A flow lives in `src/features//`. The **model** (`*.model.ts(x)`) is th Most problems a feature hits already have a shared answer. Find the row for your problem and use it. Do not write a local version, even a small one. -| Problem | Use | -| ----------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------- | -| An async action from a button or row: pending, an error, one lock | `usePendingAction` (`src/hooks/`) | -| A form: values, validation, submit, field errors | `useForm` (`src/components/form/`), with `validateAsync` for a server-side check | -| A model save whose Clerk errors should land on form fields | `save` in the model (`src/utils/errors.ts`), or reject with `FormSubmitError` | -| A confirm step before an action | `useConfirmController` (`src/blocks/confirm/`) with `Confirmation` (`src/blocks/confirmation/`) | -| A delete that needs the user to type a phrase | `useConfirmController` (`src/blocks/confirm/`) with `Destructive` (`src/blocks/destructive/`) | -| An action that may need step-up verification | `useReverificationFlow` (`src/features/reverification/`) in the model, passed to `useConfirmController` | -| Showing any error to the user | One of the owners above. They call `toLocalizableError` and resolve copy with `useErrorText` | -| Moving focus after a list row is removed | `useListRemovalFocus` (`src/hooks/`) | -| A debounced, abortable async check as the user types | `useDebouncedAsync` (`src/hooks/`) | -| A spinner that should not flash or flicker | `useSpinDelay` (`src/hooks/`) | -| A placeholder for content that is still loading | `Skeleton` (`src/components/skeleton/`), see [Skeletons](docs/skeletons.md) | -| A date shown to the user | `formatDate` (`src/utils/format-date.ts`) | -| Current time for display labels | `useNow` + `MosaicProvider` ([#10098](https://github.com/clerk/javascript/pull/10098), once available) | -| Whether enterprise SSO permits adding another identification | `allowsIdentificationCreation` (`src/utils/allows-identification-creation.ts`) | -| An inline message row that animates open and closed | `useHeldMessage`, `useMessageHeight`, `FeedbackBody` (`src/utils/feedback.tsx`) + `styles/feedback.styles.ts` | -| A table whose sorting, paging and search are done by the server | `useServerDataTable` (`src/hooks/`) | -| Siblings that slide when an item is added or removed | `useLayoutAnimation` (`src/primitives/hooks/`) | -| An interaction with an async lifecycle or two values that move together | A machine in the controller file, via `setup()` (`src/machine/`) | -| Styles several components share | `src/styles/*.styles.ts` | +| Problem | Use | +| ------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------- | +| An async action from a button or row: pending, an error, one lock | `usePendingAction` (`src/hooks/`) | +| A form: values, validation, submit, field errors | `useForm` (`src/components/form/`), with `validateAsync` for a server-side check | +| A model save whose Clerk errors should land on form fields | `save` in the model (`src/utils/errors.ts`), or reject with `FormSubmitError` | +| A confirm step before an action | `useConfirmController` (`src/blocks/confirm/`) with `Confirmation` (`src/blocks/confirmation/`) | +| A delete that needs the user to type a phrase | `useConfirmController` (`src/blocks/confirm/`) with `Destructive` (`src/blocks/destructive/`) | +| An action that may need step-up verification | `useReverificationFlow` (`src/features/reverification/`) in the model, passed to `useConfirmController` | +| Showing any error to the user | One of the owners above. They call `toLocalizableError` and resolve copy with `useErrorText` | +| Moving focus after a list row is removed | `useListRemovalFocus` (`src/hooks/`) | +| A debounced, abortable async check as the user types | `useDebouncedAsync` (`src/hooks/`) | +| A spinner that should not flash or flicker | `useSpinDelay` (`src/hooks/`) | +| A placeholder for content that is still loading | `Skeleton` (`src/components/skeleton/`), see [Skeletons](docs/skeletons.md) | +| A date shown to the user | `formatDate` (`src/utils/format-date.ts`) | +| Current time for display labels | `useNow` + `MosaicProvider` ([#10098](https://github.com/clerk/javascript/pull/10098), once available) | +| Whether enterprise SSO permits adding another identification | `allowsIdentificationCreation` (`src/utils/allows-identification-creation.ts`) | +| An inline message row that animates open and closed | `useHeldMessage`, `useMessageHeight`, `FeedbackBody` (`src/utils/feedback.tsx`) + `styles/feedback.styles.ts` | +| A table whose sorting, paging and search are done by the server | `useServerDataTable` (`src/hooks/`) | +| An item changed in place in an infinite list, kept until the next reload | `replaceInPages` (`src/utils/infinite-pages.ts`), passed to the list's `setData` | +| Siblings that slide when an item is added or removed | `useLayoutAnimation` (`src/primitives/hooks/`) | +| An interaction with an async lifecycle or two values that move together | A machine in the controller file, via `setup()` (`src/machine/`) | +| Styles several components share | `src/styles/*.styles.ts` | ## Signs you are about to invent a pattern diff --git a/packages/mosaic/docs/models.md b/packages/mosaic/docs/models.md index 892c82b143a..6e8d94551da 100644 --- a/packages/mosaic/docs/models.md +++ b/packages/mosaic/docs/models.md @@ -57,13 +57,13 @@ drops the fallback instead of holding the space open. Keep the two apart. from inside the callback that made it. Deciding _when_ is model logic: ```tsx - onAcceptInvitation: async invitationId => { + onAcceptSuggestion: async suggestionId => { try { - await invitationData.find(i => i.id === invitationId)?.accept(); + await suggestionData.find(s => s.id === suggestionId)?.accept(); } finally { // Always revalidate — a failed accept might be stale state. allSettled never throws, // so a failed revalidate doesn't look like a failed accept. - await Promise.allSettled([userInvitations.revalidate?.(), userMemberships.revalidate?.()]); + await Promise.allSettled([userSuggestions.revalidate?.()]); } }, ``` diff --git a/packages/mosaic/src/__tests__/feature/fake-fapi.ts b/packages/mosaic/src/__tests__/feature/fake-fapi.ts index 1ee4a085711..71923320984 100644 --- a/packages/mosaic/src/__tests__/feature/fake-fapi.ts +++ b/packages/mosaic/src/__tests__/feature/fake-fapi.ts @@ -743,6 +743,10 @@ export function serveFapi(seed: FakeFapiSeed = {}): FakeFapiState { leaveOrganization(state, membership.organization.id, user.id); return envelope({ object: 'organization_membership', id: membership.id, deleted: true }, state.client); }), + http.get(fapiUrl('/v1/organizations/:organizationId'), ({ params }) => { + const membership = state.memberships.find(m => m.organization.id === params.organizationId); + return membership ? envelope(membership.organization, null) : missing(); + }), http.post(fapiUrl('/v1/organizations/:organizationId'), async ({ params, request }) => { const method = new URL(request.url).searchParams.get('_method'); if (method === 'PATCH') { @@ -897,7 +901,7 @@ export function demandReverificationOnce(path: string): void { ); } -export function holdRequests(method: 'get' | 'post', path: string): HeldRequests { +export function holdRequests(method: 'get' | 'post', path: string, query: Record = {}): HeldRequests { const requests: Request[] = []; let resolveGate: (response: Response | undefined) => void = () => {}; const gate = new Promise(resolve => { @@ -912,6 +916,10 @@ export function holdRequests(method: 'get' | 'post', path: string): HeldRequests worker.use( http[method](fapiUrl(path), ({ request }) => { + const { searchParams } = new URL(request.url); + if (Object.entries(query).some(([key, value]) => searchParams.get(key) !== value)) { + return undefined; + } requests.push(request.clone()); return gate; }), diff --git a/packages/mosaic/src/features/organization-switcher/__tests__/organization-switcher.feature.test.tsx b/packages/mosaic/src/features/organization-switcher/__tests__/organization-switcher.feature.test.tsx index ac7e4dd4bc4..44a4fed63c5 100644 --- a/packages/mosaic/src/features/organization-switcher/__tests__/organization-switcher.feature.test.tsx +++ b/packages/mosaic/src/features/organization-switcher/__tests__/organization-switcher.feature.test.tsx @@ -197,6 +197,46 @@ describe('OrganizationSwitcher', () => { expect(within(trigger()).getByText('Other')).toBeInTheDocument(); }); + describe('an accepted invitation', () => { + const fullFirstPage = Array.from({ length: 10 }, (_, i) => + fapiMembership(fapiOrganization({ id: `org_page_${i}`, name: `Page ${i}` })), + ); + const enabledRow = async (name: string) => { + const row = await screen.findByRole('button', { name }); + await waitFor(() => expect(row).not.toHaveAttribute('aria-disabled', 'true')); + return row; + }; + + it('stays reachable, and follows afterSelectOrganizationUrl, before the memberships list its organization', async () => { + serveFapi( + noOffers({ + client: fapiClient([personalSession]), + memberships: fullFirstPage, + invitations: [fapiInvitation('inv_1', gamma)], + }), + ); + const nextPage = holdRequests('get', '/v1/me/organization_memberships', { offset: '10' }); + const { navigate } = await renderWithClerk(tree({ afterSelectOrganizationUrl: '/orgs/:id' })); + const user = await openWithList(); + + await user.click(screen.getByRole('button', { name: 'Accept' })); + await user.click(await enabledRow('Gamma')); + + await waitFor(() => expect(navigate).toHaveBeenCalledWith('/orgs/org_3')); + nextPage.release(); + }); + + it('lists once, as a membership, when the memberships list its organization', async () => { + await renderSwitcher({}, noOffers({ invitations: [fapiInvitation('inv_1', gamma)] })); + const user = await openWithList(); + + await user.click(screen.getByRole('button', { name: 'Accept' })); + await enabledRow('Gamma'); + + expect(reading('Gamma')).toEqual(['Gamma']); + }); + }); + it('carries custom rows beside Settings, in the order given', async () => { const terms = { id: 'terms', label: 'Terms of service', href: 'https://example.com/terms' }; const support = { id: 'support', label: 'Support', onClick: vi.fn() }; diff --git a/packages/mosaic/src/features/switcher/switcher.model.tsx b/packages/mosaic/src/features/switcher/switcher.model.tsx index 5a07d407b14..9af6ee70dce 100644 --- a/packages/mosaic/src/features/switcher/switcher.model.tsx +++ b/packages/mosaic/src/features/switcher/switcher.model.tsx @@ -13,6 +13,7 @@ import { populateParamFromObject } from '@clerk/shared/url'; import { useMosaicEnvironment } from '../../hooks/use-mosaic-environment'; import { useMosaicRouter } from '../../hooks/use-mosaic-router'; +import { replaceInPages, shrinkTotal } from '../../utils/infinite-pages'; import type { SwitcherBrandingProps, SwitcherCallbacks, @@ -282,12 +283,17 @@ export function useSwitcherModel(options?: SwitcherModelOptions, modals?: Switch return [toSession(s.id, sessionUser)]; }); - const afterSelectUrl = (organizationId: string | null): string | undefined => { + const afterSelectUrl = async (organizationId: string | null): Promise => { if (!organizationId) { return resolveAfterUrl(options?.afterSelectPersonalUrl, user); } - const selected = membershipData.find(m => m.organization.id === organizationId)?.organization; - return selected ? resolveAfterUrl(options?.afterSelectOrganizationUrl, selected) : undefined; + if (!options?.afterSelectOrganizationUrl) { + return undefined; + } + const selected = + membershipData.find(m => m.organization.id === organizationId)?.organization ?? + (await clerk.getOrganization(organizationId)); + return resolveAfterUrl(options.afterSelectOrganizationUrl, selected); }; return { @@ -309,8 +315,8 @@ export function useSwitcherModel(options?: SwitcherModelOptions, modals?: Switch ref, hasMore: Boolean(userMemberships.hasNextPage || userInvitations.hasNextPage || userSuggestions.hasNextPage), }, - onSelectOrganization: organizationId => - clerk.setActive({ organization: organizationId, redirectUrl: afterSelectUrl(organizationId) }), + onSelectOrganization: async organizationId => + clerk.setActive({ organization: organizationId, redirectUrl: await afterSelectUrl(organizationId) }), // The session switched to can carry a task of its own, and a plain `redirectUrl` routes past it. // App-level `taskUrls` outrank this callback, so it only answers for an app that set none. onSwitchSession: sessionId => @@ -359,11 +365,17 @@ export function useSwitcherModel(options?: SwitcherModelOptions, modals?: Switch onAcceptInvitation: async invitationId => { const invitation = invitationData.find(i => i.id === invitationId); try { - await invitation?.accept(); + const accepted = await invitation?.accept(); + if (accepted) { + await userInvitations.setData?.(pages => shrinkTotal(replaceInPages(pages ?? [], accepted))); + } + } catch (error) { + // A failed accept might be because of old state + await Promise.allSettled([userInvitations.revalidate?.()]); + throw error; } finally { - // We always revalidate, a failed accept might be because of old state // Using allSettled since it never throws and we don't want failed revalidates to look like failed accepts - await Promise.allSettled([userInvitations.revalidate?.(), userMemberships.revalidate?.()]); + await Promise.allSettled([userMemberships.revalidate?.()]); } }, }; diff --git a/packages/mosaic/src/features/user-button/__tests__/user-button.feature.test.tsx b/packages/mosaic/src/features/user-button/__tests__/user-button.feature.test.tsx index 1defa58251b..17bbfad742b 100644 --- a/packages/mosaic/src/features/user-button/__tests__/user-button.feature.test.tsx +++ b/packages/mosaic/src/features/user-button/__tests__/user-button.feature.test.tsx @@ -335,11 +335,11 @@ describe('UserButton', () => { expect(popup()).toBeInTheDocument(); }); - it('stays busy until the invitations have reloaded', async () => { + it('stays busy until the memberships have reloaded', async () => { await renderUserButton(); const user = await open(); await screen.findByRole('button', { name: 'Accept' }); - const reload = holdRequests('get', '/v1/me/organization_invitations'); + const reload = holdRequests('get', '/v1/me/organization_memberships'); await user.click(screen.getByRole('button', { name: 'Accept' })); await waitFor(() => expect(reload.requests).toHaveLength(1)); @@ -481,6 +481,18 @@ describe('UserButton', () => { await waitFor(() => expect(popup()).toBeNull()); }); + it('hands the CreateOrganization modal the portal root the app provides', async () => { + const container = document.createElement('div'); + serveFapi(signedIn()); + const { clerk } = await renderWithClerk( container}>{tree()}); + const openCreateOrganization = vi.spyOn(clerk, 'openCreateOrganization').mockImplementation(() => {}); + const user = await open(); + + await user.click(await screen.findByRole('button', { name: 'Add organization' })); + + expect(openCreateOrganization.mock.calls[0]?.[0]?.getContainer?.()).toBe(container); + }); + it('navigates to create an organization when a URL routes it', async () => { const { clerk, navigate } = await renderUserButton({ createOrganizationUrl: '/new-org' }); const openCreateOrganization = vi.spyOn(clerk, 'openCreateOrganization').mockImplementation(() => {}); diff --git a/packages/mosaic/src/utils/__tests__/infinite-pages.test.ts b/packages/mosaic/src/utils/__tests__/infinite-pages.test.ts new file mode 100644 index 00000000000..652a3c36e04 --- /dev/null +++ b/packages/mosaic/src/utils/__tests__/infinite-pages.test.ts @@ -0,0 +1,41 @@ +import { describe, expect, it } from 'vitest'; + +import { replaceInPages, shrinkTotal } from '../infinite-pages'; + +describe('replaceInPages', () => { + const page = (...ids: string[]) => ({ data: ids.map(id => ({ id, status: 'pending' })), total_count: 3 }); + + it('replaces the item sharing its id, wherever its page is', () => { + const accepted = { id: 'b', status: 'accepted' }; + + expect(replaceInPages([page('a'), page('b', 'c')], accepted)).toEqual([ + page('a'), + { + data: [ + { id: 'b', status: 'accepted' }, + { id: 'c', status: 'pending' }, + ], + total_count: 3, + }, + ]); + }); + + it('keeps a page that has yet to load where it is', () => { + expect(replaceInPages([undefined, page('a')], { id: 'a', status: 'accepted' })).toEqual([ + undefined, + { data: [{ id: 'a', status: 'accepted' }], total_count: 3 }, + ]); + }); + + it('leaves the pages as they are when no item shares its id', () => { + expect(replaceInPages([page('a')], { id: 'z', status: 'accepted' })).toEqual([page('a')]); + }); +}); + +describe('shrinkTotal', () => { + const page = (total_count: number) => ({ data: [{ id: 'a' }], total_count }); + + it('counts one fewer on every loaded page', () => { + expect(shrinkTotal([page(12), undefined, page(12)])).toEqual([page(11), undefined, page(11)]); + }); +}); diff --git a/packages/mosaic/src/utils/infinite-pages.ts b/packages/mosaic/src/utils/infinite-pages.ts new file mode 100644 index 00000000000..eaa6a7e9bef --- /dev/null +++ b/packages/mosaic/src/utils/infinite-pages.ts @@ -0,0 +1,16 @@ +import type { ClerkPaginatedResponse } from '@clerk/shared/types'; + +export function replaceInPages( + pages: (ClerkPaginatedResponse | undefined)[], + item: T, +): (ClerkPaginatedResponse | undefined)[] { + return pages.map( + page => page && { ...page, data: page.data.map(existing => (existing.id === item.id ? item : existing)) }, + ); +} + +export function shrinkTotal( + pages: (ClerkPaginatedResponse | undefined)[], +): (ClerkPaginatedResponse | undefined)[] { + return pages.map(page => page && { ...page, total_count: page.total_count - 1 }); +}