Skip to content

Commit bc04b2a

Browse files
committed
Address review feedback
1 parent 31bc04f commit bc04b2a

6 files changed

Lines changed: 189 additions & 36 deletions

‎packages/shared/src/react/hooks/__tests__/useOrganizationDirectorySync.spec.tsx‎

Lines changed: 106 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,20 @@ import { __internal_useOrganizationDirectorySync } from '../useOrganizationDirec
99
import { createMockClerk, createMockQueryClient } from './mocks/clerk';
1010
import { wrapper } from './wrapper';
1111

12-
const directory = { id: 'dir_1', enterpriseConnectionId: 'ent_1' };
12+
const updateSpy = vi.fn(() => Promise.resolve({ ...directory, name: 'Renamed' }));
13+
const rotateTokenSpy = vi.fn(() => Promise.resolve({ ...directory, token: 'tok_new' }));
14+
const deleteSpy = vi.fn(() => Promise.resolve({ object: 'directory', id: 'dir_1', deleted: true }));
15+
const directory = {
16+
id: 'dir_1',
17+
enterpriseConnectionId: 'ent_1',
18+
update: updateSpy,
19+
rotateToken: rotateTokenSpy,
20+
delete: deleteSpy,
21+
};
1322
const getDirectorySyncSpy = vi.fn((_enterpriseConnectionId: string) => Promise.resolve(directory));
23+
const createDirectorySyncSpy = vi.fn((_enterpriseConnectionId: string, _params?: unknown) =>
24+
Promise.resolve({ ...directory, token: 'tok_1' }),
25+
);
1426

1527
const defaultQueryClient = createMockQueryClient();
1628

@@ -19,7 +31,7 @@ const mockClerk = createMockClerk({
1931
__internal_lastEmittedResources: {
2032
user: null,
2133
session: null,
22-
organization: { id: 'org_1', getDirectorySync: getDirectorySyncSpy },
34+
organization: { id: 'org_1', getDirectorySync: getDirectorySyncSpy, createDirectorySync: createDirectorySyncSpy },
2335
client: null,
2436
},
2537
});
@@ -89,4 +101,96 @@ describe('useOrganizationDirectorySync', () => {
89101
expect(getDirectorySyncSpy).toHaveBeenCalledTimes(2);
90102
expect(defaultQueryClient.client.getQueryState(otherKey)?.isInvalidated).toBe(false);
91103
});
104+
105+
describe('mutations', () => {
106+
it('createDirectorySync creates for the connection, resolves the token-bearing resource, and refetches', async () => {
107+
const { result } = renderDirectorySync();
108+
await waitFor(() => expect(result.current.isLoading).toBe(false));
109+
expect(getDirectorySyncSpy).toHaveBeenCalledTimes(1);
110+
111+
let created: Awaited<ReturnType<typeof result.current.createDirectorySync>>;
112+
await act(async () => {
113+
created = await result.current.createDirectorySync({ name: 'Okta' });
114+
});
115+
116+
expect(createDirectorySyncSpy).toHaveBeenCalledWith('ent_1', { name: 'Okta' });
117+
expect(created).toMatchObject({ id: 'dir_1', token: 'tok_1' });
118+
expect(getDirectorySyncSpy).toHaveBeenCalledTimes(2);
119+
});
120+
121+
it('createDirectorySync is a no-op without an enterprise connection id', async () => {
122+
const { result } = renderDirectorySync(null);
123+
124+
await expect(result.current.createDirectorySync()).resolves.toBeUndefined();
125+
expect(createDirectorySyncSpy).not.toHaveBeenCalled();
126+
});
127+
128+
it.each([
129+
[
130+
'updateDirectorySync',
131+
updateSpy,
132+
(r: ReturnType<typeof renderDirectorySync>['result']) => r.current.updateDirectorySync({ name: 'Renamed' }),
133+
],
134+
[
135+
'rotateDirectorySyncToken',
136+
rotateTokenSpy,
137+
(r: ReturnType<typeof renderDirectorySync>['result']) => r.current.rotateDirectorySyncToken(),
138+
],
139+
[
140+
'deleteDirectorySync',
141+
deleteSpy,
142+
(r: ReturnType<typeof renderDirectorySync>['result']) => r.current.deleteDirectorySync(),
143+
],
144+
] as const)('%s acts on the loaded directory and refetches it', async (_name, resourceSpy, run) => {
145+
const { result } = renderDirectorySync();
146+
await waitFor(() => expect(result.current.isLoading).toBe(false));
147+
expect(getDirectorySyncSpy).toHaveBeenCalledTimes(1);
148+
149+
let resolved: unknown;
150+
await act(async () => {
151+
resolved = await run(result);
152+
});
153+
154+
expect(resourceSpy).toHaveBeenCalledTimes(1);
155+
expect(resolved).toBe(await resourceSpy.mock.results[0].value);
156+
expect(getDirectorySyncSpy).toHaveBeenCalledTimes(2);
157+
});
158+
159+
it('updateDirectorySync forwards its params to the resource', async () => {
160+
const { result } = renderDirectorySync();
161+
await waitFor(() => expect(result.current.isLoading).toBe(false));
162+
163+
await act(async () => {
164+
await result.current.updateDirectorySync({ name: 'Renamed' });
165+
});
166+
167+
expect(updateSpy).toHaveBeenCalledWith({ name: 'Renamed' });
168+
});
169+
170+
it('directory-scoped mutations resolve undefined before the directory has loaded', async () => {
171+
getDirectorySyncSpy.mockRejectedValueOnce(new ClerkAPIResponseError('Not found', { status: 404, data: [] }));
172+
const { result } = renderDirectorySync();
173+
await waitFor(() => expect(result.current.isLoading).toBe(false));
174+
expect(result.current.data).toBeNull();
175+
176+
await expect(result.current.updateDirectorySync({ name: 'Renamed' })).resolves.toBeUndefined();
177+
await expect(result.current.rotateDirectorySyncToken()).resolves.toBeUndefined();
178+
await expect(result.current.deleteDirectorySync()).resolves.toBeUndefined();
179+
expect(updateSpy).not.toHaveBeenCalled();
180+
expect(rotateTokenSpy).not.toHaveBeenCalled();
181+
expect(deleteSpy).not.toHaveBeenCalled();
182+
});
183+
184+
it('propagates a failed mutation and skips the refetch', async () => {
185+
const { result } = renderDirectorySync();
186+
await waitFor(() => expect(result.current.isLoading).toBe(false));
187+
expect(getDirectorySyncSpy).toHaveBeenCalledTimes(1);
188+
189+
const failure = new Error('rotate failed');
190+
rotateTokenSpy.mockRejectedValueOnce(failure);
191+
192+
await expect(result.current.rotateDirectorySyncToken()).rejects.toBe(failure);
193+
expect(getDirectorySyncSpy).toHaveBeenCalledTimes(1);
194+
});
195+
});
92196
});

‎packages/shared/src/react/hooks/__tests__/useOrganizationDirectorySyncUsers.spec.tsx‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,10 @@ describe('useOrganizationDirectorySyncUsers', () => {
6767

6868
act(() => result.current.stopPolling());
6969
expect(result.current.isPolling).toBe(false);
70+
71+
const callsAfterStop = getUsersSpy.mock.calls.length;
72+
await new Promise(resolve => setTimeout(resolve, POLL_INTERVAL_MS * 3));
73+
expect(getUsersSpy.mock.calls.length).toBe(callsAfterStop);
7074
});
7175

7276
it('disarms polling when the directory identity changes', async () => {
@@ -80,6 +84,23 @@ describe('useOrganizationDirectorySyncUsers', () => {
8084
expect(result.current.isPolling).toBe(false);
8185
});
8286

87+
it('does not resume polling when the original directory returns', async () => {
88+
const { result, rerender } = renderUsers(createDirectory('dir_1'));
89+
await waitFor(() => expect(result.current.isLoading).toBe(false));
90+
91+
act(() => result.current.startPolling());
92+
expect(result.current.isPolling).toBe(true);
93+
94+
rerender({ directory: createDirectory('dir_2') });
95+
rerender({ directory: createDirectory('dir_1') });
96+
await waitFor(() => expect(result.current.isLoading).toBe(false));
97+
expect(result.current.isPolling).toBe(false);
98+
99+
const callsAfterReturn = getUsersSpy.mock.calls.length;
100+
await new Promise(resolve => setTimeout(resolve, POLL_INTERVAL_MS * 3));
101+
expect(getUsersSpy.mock.calls.length).toBe(callsAfterReturn);
102+
});
103+
83104
it('keeps polling armed by a child effect in the same commit the directory arrives', async () => {
84105
let latest: UseOrganizationDirectorySyncUsersReturn | undefined;
85106

‎packages/shared/src/react/hooks/__tests__/useOrganizationEnterpriseConnectionTestRuns.spec.tsx‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,4 +166,28 @@ describe('useOrganizationEnterpriseConnectionTestRuns — polling arm scope', ()
166166
rerender({ enterpriseConnectionId: 'ent_2' });
167167
expect(result.current.isPolling).toBe(false);
168168
});
169+
170+
it('does not resume polling when the original connection returns without a new revalidate', async () => {
171+
getTestRunsSpy.mockImplementation(() => Promise.resolve({ data: [], total_count: 0 }));
172+
const { result, rerender } = renderHook(
173+
({ enterpriseConnectionId }: { enterpriseConnectionId: string }) =>
174+
__internal_useOrganizationEnterpriseConnectionTestRuns({ enterpriseConnectionId, pollIntervalMs: 20 }),
175+
{ wrapper, initialProps: { enterpriseConnectionId: 'ent_1' } },
176+
);
177+
await waitFor(() => expect(result.current.isLoading).toBe(false));
178+
179+
await act(async () => {
180+
await result.current.revalidate();
181+
});
182+
expect(result.current.isPolling).toBe(true);
183+
184+
rerender({ enterpriseConnectionId: 'ent_2' });
185+
rerender({ enterpriseConnectionId: 'ent_1' });
186+
await waitFor(() => expect(result.current.isLoading).toBe(false));
187+
expect(result.current.isPolling).toBe(false);
188+
189+
const callsAfterReturn = getTestRunsSpy.mock.calls.length;
190+
await new Promise(resolve => setTimeout(resolve, 60));
191+
expect(getTestRunsSpy.mock.calls.length).toBe(callsAfterReturn);
192+
});
169193
});

‎packages/shared/src/react/hooks/useOrganizationDirectorySync.tsx‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,7 @@ export type UseOrganizationDirectorySyncParams = {
2020
};
2121

2222
export type UseOrganizationDirectorySyncReturn = {
23-
/**
24-
* The connection's directory, `null` when none has been created yet, `undefined` while loading.
25-
* Never carries the bearer token — that only exists on the resources resolved by
26-
* `createDirectorySync` and `rotateDirectorySyncToken`.
27-
*/
23+
/** The connection's directory, `null` when none has been created yet, `undefined` while loading. */
2824
data: DirectorySyncResource | null | undefined;
2925
error: Error | null;
3026
isLoading: boolean;

‎packages/shared/src/react/hooks/useOrganizationDirectorySyncUsers.tsx‎

Lines changed: 25 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { useCallback, useState } from 'react';
1+
import { useCallback, useEffect, useState } from 'react';
22

33
import type {
44
DirectorySyncResource,
@@ -15,21 +15,21 @@ import { useOrganizationDirectorySyncUsersCacheKeys } from './useOrganizationDir
1515
const DEFAULT_POLL_INTERVAL_MS = 2_000;
1616

1717
export type UseOrganizationDirectorySyncUsersParams = {
18-
/** The directory to list users for, e.g. `data` from `useOrganizationDirectorySync`. Dormant while nullish. */
18+
/** The directory to list users for, e.g. `data` from `useOrganizationDirectorySync`. Nothing is fetched while `null` or `undefined`. */
1919
directory: DirectorySyncResource | null | undefined;
2020
/**
2121
* Pass-through fetch parameters (pagination).
2222
* Defaults to `{ initialPage: 1, pageSize: 10 }`.
2323
*/
2424
params?: GetDirectorySyncUsersParams;
2525
/**
26-
* Polling interval (ms) applied while polling is armed via `startPolling`.
26+
* Polling interval (ms) used while polling is active.
2727
*
2828
* @default 2000
2929
*/
3030
pollIntervalMs?: number;
3131
/**
32-
* If `false`, the hook is dormant — no fetch, no polling.
32+
* If `false`, nothing is fetched and polling is paused.
3333
*
3434
* @default true
3535
*/
@@ -38,25 +38,21 @@ export type UseOrganizationDirectorySyncUsersParams = {
3838
};
3939

4040
export type UseOrganizationDirectorySyncUsersReturn = {
41-
/** `undefined` while loading and while the hook is dormant. */
41+
/** `undefined` while loading and while the hook is disabled. */
4242
data: DirectorySyncUserResource[] | undefined;
4343
totalCount: number | undefined;
4444
error: Error | null;
4545
isLoading: boolean;
4646
isFetching: boolean;
47-
/**
48-
* `true` while the hook is actively polling
49-
*/
47+
/** `true` while the hook is polling. */
5048
isPolling: boolean;
5149
/**
52-
* Start polling. Polling runs continuously (new provisions, updates, and
53-
* deprovisions keep appearing) until `stopPolling` is called — callers
54-
* should stop on unmount of the view that armed it.
50+
* Start polling for changes to the list. Polling continues until `stopPolling`
51+
* is called or the hook unmounts, so colocate the hook with the view that
52+
* needs the feed and call this from an effect on mount.
5553
*/
5654
startPolling: () => void;
57-
/**
58-
* Stop polling.
59-
*/
55+
/** Stop polling. */
6056
stopPolling: () => void;
6157
/**
6258
* Force a refetch.
@@ -66,8 +62,8 @@ export type UseOrganizationDirectorySyncUsersReturn = {
6662

6763
/**
6864
* The users provisioned into an enterprise connection's Directory Sync
69-
* directory, most recently touched first. Polls continuously while armed via
70-
* `startPolling`, so the setup flow doubles as a recent-activity feed.
65+
* directory, most recently touched first. Polling is opt-in via `startPolling`,
66+
* which lets the setup flow use the list as a live activity feed.
7167
*
7268
* @internal
7369
*/
@@ -103,10 +99,16 @@ function useOrganizationDirectorySyncUsers(
10399

104100
const queryEnabled = enabled && clerk.loaded && Boolean(organization) && Boolean(directory);
105101

106-
// Polling is armed for a specific directory and derived, not reset in an effect: a child
107-
// effect arming it in the same commit the directory arrives would otherwise be cancelled.
108-
const [armedForDirectoryId, setArmedForDirectoryId] = useState<string | null>(null);
109-
const shouldPoll = armedForDirectoryId !== null && armedForDirectoryId === directoryId;
102+
// Polling is requested for a specific directory, so a directory change stops it. This is
103+
// derived rather than reset in an effect because a child component may call `startPolling`
104+
// in the same commit the directory arrives, and an unconditional reset would cancel that.
105+
const [pollingDirectoryId, setPollingDirectoryId] = useState<string | null>(null);
106+
const shouldPoll = pollingDirectoryId !== null && pollingDirectoryId === directoryId;
107+
108+
useEffect(() => {
109+
// Drop a request left over from a previous directory so it cannot resume if that directory returns.
110+
setPollingDirectoryId(current => (current !== null && current !== directoryId ? null : current));
111+
}, [directoryId]);
110112

111113
const currentTracked = queryKey[2];
112114
const query = useClerkQuery({
@@ -137,11 +139,11 @@ function useOrganizationDirectorySyncUsers(
137139
});
138140

139141
const startPolling = useCallback(() => {
140-
setArmedForDirectoryId(directoryId);
142+
setPollingDirectoryId(directoryId);
141143
}, [directoryId]);
142144

143145
const stopPolling = useCallback(() => {
144-
setArmedForDirectoryId(null);
146+
setPollingDirectoryId(null);
145147
}, []);
146148

147149
const revalidate = useCallback(async () => {
@@ -151,7 +153,7 @@ function useOrganizationDirectorySyncUsers(
151153
const isPolling = queryEnabled && shouldPoll;
152154

153155
return {
154-
// Dormant means dormant: never surface cached rows while the query cannot run.
156+
// A disabled query still exposes rows cached under its key; report none until it can run.
155157
data: queryEnabled ? query.data?.data : undefined,
156158
totalCount: queryEnabled ? query.data?.total_count : undefined,
157159
error: query.error ?? null,

‎packages/shared/src/react/hooks/useOrganizationEnterpriseConnectionTestRuns.tsx‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -136,10 +136,16 @@ function useOrganizationEnterpriseConnectionTestRuns(
136136

137137
const queryEnabled = enabled && clerk.loaded && Boolean(organization) && Boolean(enterpriseConnectionId);
138138

139-
// Polling is armed for a specific connection and derived, not reset in an effect: a child
140-
// effect arming it in the same commit the connection arrives would otherwise be cancelled.
141-
const [armedForConnectionId, setArmedForConnectionId] = useState<string | null>(null);
142-
const shouldPoll = armedForConnectionId !== null && armedForConnectionId === enterpriseConnectionId;
139+
// Polling is requested for a specific connection, so a connection change stops it. This is
140+
// derived rather than reset in an effect because a child component may call `revalidate` in
141+
// the same commit the connection arrives, and an unconditional reset would cancel that.
142+
const [pollingConnectionId, setPollingConnectionId] = useState<string | null>(null);
143+
const shouldPoll = pollingConnectionId !== null && pollingConnectionId === enterpriseConnectionId;
144+
145+
useEffect(() => {
146+
// Drop a request left over from a previous connection so it cannot resume if that connection returns.
147+
setPollingConnectionId(current => (current !== null && current !== enterpriseConnectionId ? null : current));
148+
}, [enterpriseConnectionId]);
143149

144150
const query = useClerkQuery({
145151
queryKey,
@@ -166,7 +172,7 @@ function useOrganizationEnterpriseConnectionTestRuns(
166172

167173
useEffect(() => {
168174
if (shouldPoll && hasRows) {
169-
setArmedForConnectionId(null);
175+
setPollingConnectionId(null);
170176
}
171177
}, [shouldPoll, hasRows]);
172178

@@ -181,7 +187,7 @@ function useOrganizationEnterpriseConnectionTestRuns(
181187
// off. Once any record has been seen, this is a one-shot refetch.
182188
const armPolling = options?.armPolling ?? true;
183189
if (armPolling && !hasRows) {
184-
setArmedForConnectionId(enterpriseConnectionId);
190+
setPollingConnectionId(enterpriseConnectionId);
185191
}
186192
// `invalidateQueries` awaits the refetch it triggers, so by the time it
187193
// resolves the cache already holds the fresh page. Read it back from the

0 commit comments

Comments
 (0)