Skip to content

Commit d1add4f

Browse files
fix(slack-search): run turns inline and preserve OAuth callbacks
1 parent c03ce29 commit d1add4f

5 files changed

Lines changed: 155 additions & 28 deletions

File tree

apps/sim/ee/credential-groups/components/slack-managed-users-access.test.tsx

Lines changed: 126 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
/** @vitest-environment jsdom */
22
import { act } from 'react'
3+
import { toast } from '@sim/emcn'
34
import { QueryClient, QueryClientProvider } from '@tanstack/react-query'
45
import { createRoot, type Root } from 'react-dom/client'
56
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
@@ -44,6 +45,8 @@ describe('Slack member access selection', () => {
4445
let root: Root
4546
let container: HTMLDivElement
4647
let client: QueryClient
48+
let channels: Array<{ onmessage: ((event: MessageEvent<unknown>) => void) | null }>
49+
let popup: { location: { href: string }; closed: boolean; close: ReturnType<typeof vi.fn> }
4750
const bot: WorkspaceCredential = {
4851
id: '11111111-1111-4111-8111-111111111111',
4952
workspaceId: 'workspace-1',
@@ -62,6 +65,8 @@ describe('Slack member access selection', () => {
6265

6366
beforeEach(() => {
6467
vi.clearAllMocks()
68+
vi.spyOn(toast, 'error').mockReturnValue('toast')
69+
vi.spyOn(toast, 'success').mockReturnValue('toast')
6570
vi.stubGlobal('IS_REACT_ACT_ENVIRONMENT', true)
6671
mocks.create.mockResolvedValue(undefined)
6772
mocks.apps.mockReturnValue({
@@ -81,17 +86,23 @@ describe('Slack member access selection', () => {
8186
state: 'state',
8287
authorizationUrl: 'https://slack.com/oauth/v2/authorize',
8388
})
89+
channels = []
8490
vi.stubGlobal(
8591
'BroadcastChannel',
8692
class {
93+
onmessage: ((event: MessageEvent<unknown>) => void) | null = null
94+
constructor() {
95+
channels.push(this)
96+
}
8797
close() {}
8898
}
8999
)
90-
vi.spyOn(window, 'open').mockReturnValue({
100+
popup = {
91101
location: { href: '' },
92102
closed: false,
93103
close: vi.fn(),
94-
} as unknown as Window)
104+
}
105+
vi.spyOn(window, 'open').mockReturnValue(popup as unknown as Window)
95106
container = document.createElement('div')
96107
document.body.appendChild(container)
97108
root = createRoot(container)
@@ -103,6 +114,7 @@ describe('Slack member access selection', () => {
103114
client.clear()
104115
vi.restoreAllMocks()
105116
vi.unstubAllGlobals()
117+
vi.useRealTimers()
106118
})
107119

108120
async function render(
@@ -166,6 +178,118 @@ describe('Slack member access selection', () => {
166178
)
167179
}
168180

181+
async function completeAuthorization(state = 'state') {
182+
await act(async () => {
183+
for (const channel of channels) {
184+
channel.onmessage?.(
185+
new MessageEvent('message', {
186+
data: {
187+
type: 'slack-managed-users',
188+
ok: true,
189+
state,
190+
credentialGroupId: 'group-1',
191+
slackBotCredentialId: bot.id,
192+
},
193+
})
194+
)
195+
}
196+
})
197+
}
198+
199+
it('accepts authorization after browser isolation reports a live popup as closed', async () => {
200+
vi.useFakeTimers()
201+
await render()
202+
await submit()
203+
popup.closed = true
204+
await act(async () => vi.advanceTimersByTimeAsync(1_000))
205+
206+
expect(toast.error).not.toHaveBeenCalled()
207+
expect(document.body.textContent).toContain('Waiting for Slack...')
208+
await completeAuthorization('unrelated-state')
209+
expect(toast.success).not.toHaveBeenCalled()
210+
await completeAuthorization()
211+
expect(toast.success).toHaveBeenCalledWith('Slack configured')
212+
expect(mocks.onOpenChange).toHaveBeenCalledWith(false)
213+
await act(async () => vi.advanceTimersByTimeAsync(10 * 60 * 1_000))
214+
expect(toast.error).not.toHaveBeenCalled()
215+
})
216+
217+
it('expires only after the authorization deadline and ignores a late callback', async () => {
218+
vi.useFakeTimers()
219+
await render()
220+
await submit()
221+
await act(async () => vi.advanceTimersByTimeAsync(10 * 60 * 1_000 - 1))
222+
expect(toast.error).not.toHaveBeenCalled()
223+
await act(async () => vi.advanceTimersByTimeAsync(1))
224+
expect(toast.error).toHaveBeenCalledExactlyOnceWith(
225+
'Slack authorization expired. Please try again.'
226+
)
227+
expect(popup.close).toHaveBeenCalledOnce()
228+
await completeAuthorization()
229+
expect(toast.success).not.toHaveBeenCalled()
230+
})
231+
232+
it('lets the user cancel an abandoned popup without reporting expiry', async () => {
233+
vi.useFakeTimers()
234+
await render()
235+
await submit()
236+
await clickButton('Cancel')
237+
expect(popup.close).toHaveBeenCalledOnce()
238+
expect(mocks.onOpenChange).toHaveBeenCalledWith(false)
239+
await completeAuthorization()
240+
await act(async () => vi.advanceTimersByTimeAsync(10 * 60 * 1_000))
241+
expect(toast.error).not.toHaveBeenCalled()
242+
expect(toast.success).not.toHaveBeenCalled()
243+
})
244+
245+
it('does not navigate or start a timeout when authorization startup finishes after cancel', async () => {
246+
vi.useFakeTimers()
247+
let finishStartup!: (value: { state: string; authorizationUrl: string }) => void
248+
mocks.start.mockReturnValueOnce(
249+
new Promise((resolve) => {
250+
finishStartup = resolve
251+
})
252+
)
253+
await render()
254+
await submit()
255+
await clickButton('Cancel')
256+
await act(async () => {
257+
finishStartup({ state: 'state', authorizationUrl: 'https://slack.com/oauth/v2/authorize' })
258+
})
259+
expect(popup.location.href).toBe('')
260+
await act(async () => vi.advanceTimersByTimeAsync(10 * 60 * 1_000))
261+
expect(toast.error).not.toHaveBeenCalled()
262+
})
263+
264+
it.each(['resolve', 'reject'] as const)(
265+
'ignores authorization startup that completes with %s after unmount',
266+
async (outcome) => {
267+
vi.useFakeTimers()
268+
let finishStartup!: () => void
269+
mocks.start.mockReturnValueOnce(
270+
new Promise((resolve, reject) => {
271+
finishStartup = () =>
272+
outcome === 'resolve'
273+
? resolve({
274+
state: 'state',
275+
authorizationUrl: 'https://slack.com/oauth/v2/authorize',
276+
})
277+
: reject(new Error('Authorization startup failed'))
278+
})
279+
)
280+
await render()
281+
await submit()
282+
await act(async () => root.render(null))
283+
await act(async () => finishStartup())
284+
285+
expect(popup.close).toHaveBeenCalledOnce()
286+
expect(popup.location.href).toBe('')
287+
await act(async () => vi.advanceTimersByTimeAsync(10 * 60 * 1_000))
288+
expect(toast.error).not.toHaveBeenCalled()
289+
expect(toast.success).not.toHaveBeenCalled()
290+
}
291+
)
292+
169293
it('opens Slack app setup inline and returns to member setup when canceled', async () => {
170294
await render(undefined, [])
171295
expect(document.querySelector('a')).toBeNull()

apps/sim/ee/credential-groups/components/slack-managed-users-modal.tsx

Lines changed: 17 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ export function SlackManagedUsersModal({
108108
const expectedState = useRef<string | null>(null)
109109
const expectedCredentialId = useRef<string | null>(null)
110110
const popup = useRef<Window | null>(null)
111-
const popupWatcher = useRef<number | null>(null)
111+
const authorizationTimeout = useRef<number | null>(null)
112112

113113
const defaultCredentialId = initialCredentialId
114114
? bots.some((bot) => bot.id === initialCredentialId)
@@ -141,8 +141,8 @@ export function SlackManagedUsersModal({
141141
const reset = () => {
142142
popup.current?.close()
143143
popup.current = null
144-
if (popupWatcher.current !== null) window.clearInterval(popupWatcher.current)
145-
popupWatcher.current = null
144+
if (authorizationTimeout.current !== null) window.clearTimeout(authorizationTimeout.current)
145+
authorizationTimeout.current = null
146146
expectedState.current = null
147147
expectedCredentialId.current = null
148148
setAppSetupOpen(false)
@@ -160,8 +160,8 @@ export function SlackManagedUsersModal({
160160
const verifiedCredentialId = expectedCredentialId.current
161161
expectedState.current = null
162162
expectedCredentialId.current = null
163-
if (popupWatcher.current !== null) window.clearInterval(popupWatcher.current)
164-
popupWatcher.current = null
163+
if (authorizationTimeout.current !== null) window.clearTimeout(authorizationTimeout.current)
164+
authorizationTimeout.current = null
165165
popup.current?.close()
166166
popup.current = null
167167
setPending(false)
@@ -222,14 +222,17 @@ export function SlackManagedUsersModal({
222222

223223
useEffect(
224224
() => () => {
225-
if (popupWatcher.current !== null) window.clearInterval(popupWatcher.current)
225+
if (authorizationTimeout.current !== null) window.clearTimeout(authorizationTimeout.current)
226226
popup.current?.close()
227+
popup.current = null
228+
authorizationTimeout.current = null
229+
expectedState.current = null
230+
expectedCredentialId.current = null
227231
},
228232
[]
229233
)
230234

231235
const handleOpenChange = (nextOpen: boolean) => {
232-
if (pending && !nextOpen) return
233236
onOpenChange(nextOpen)
234237
if (!nextOpen) reset()
235238
}
@@ -273,22 +276,22 @@ export function SlackManagedUsersModal({
273276
requiredScopes,
274277
},
275278
})
279+
if (popup.current !== opened) return
276280
expectedState.current = result.state
277281
expectedCredentialId.current = selectedBot?.id ?? null
278282
opened.location.href = result.authorizationUrl
279-
const startedAt = Date.now()
280-
popupWatcher.current = window.setInterval(() => {
281-
if (!opened.closed && Date.now() - startedAt < AUTHORIZATION_TIMEOUT_MS) return
282-
window.clearInterval(popupWatcher.current ?? undefined)
283-
popupWatcher.current = null
283+
/** COOP can report a live OAuth popup as closed; only the deadline expires its state. */
284+
authorizationTimeout.current = window.setTimeout(() => {
285+
authorizationTimeout.current = null
284286
opened.close()
285287
popup.current = null
286288
expectedState.current = null
287289
expectedCredentialId.current = null
288290
setPending(false)
289291
toast.error('Slack authorization expired. Please try again.')
290-
}, 500)
292+
}, AUTHORIZATION_TIMEOUT_MS)
291293
} catch (authorizationError) {
294+
if (popup.current !== opened) return
292295
opened.close()
293296
popup.current = null
294297
setPending(false)
@@ -318,15 +321,10 @@ export function SlackManagedUsersModal({
318321
<ChipModal
319322
open={open && !appSetupOpen}
320323
onOpenChange={handleOpenChange}
321-
dismissDisabled={pending}
322324
srTitle={title}
323325
size='md'
324326
>
325-
<ChipModalHeader
326-
icon={SlackIcon}
327-
onClose={() => handleOpenChange(false)}
328-
closeDisabled={pending}
329-
>
327+
<ChipModalHeader icon={SlackIcon} onClose={() => handleOpenChange(false)}>
330328
{title}
331329
</ChipModalHeader>
332330
<ChipModalBody>
@@ -467,7 +465,6 @@ export function SlackManagedUsersModal({
467465
</ChipModalBody>
468466
<ChipModalFooter
469467
onCancel={() => handleOpenChange(false)}
470-
cancelDisabled={pending}
471468
{...(needsApp
472469
? {
473470
primaryAction: {

apps/sim/lib/slack-search/README.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,8 @@ existing conflicting active workspace bindings before applying the unique index.
144144
per thread. The three-minute execution deadline starts after a claim.
145145
Expired executions are marked failed and never replayed after an ambiguous
146146
external send. The shared outbox processor repairs missed wake-ups.
147+
Wakes use the database-backed inline queue and run in the app process (ECS
148+
on hosted deployments), even when other background jobs use Trigger.dev.
147149
4. `knowledge/application/slack-search/assistant.ts` resolves the sender’s email
148150
through that installation’s bot credential. It requires exactly one current,
149151
verified Sim member of the bound organization. Email never selects the

apps/sim/lib/slack-search/queue.test.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
11
/** @vitest-environment node */
22
import { beforeEach, describe, expect, it, vi } from 'vitest'
33

4-
const mocks = vi.hoisted(() => ({ enqueue: vi.fn(), run: vi.fn() }))
5-
vi.mock('@/lib/core/async-jobs', () => ({ getJobQueue: async () => ({ enqueue: mocks.enqueue }) }))
4+
const mocks = vi.hoisted(() => ({ enqueue: vi.fn(), run: vi.fn(), externalEnqueue: vi.fn() }))
5+
vi.mock('@/lib/core/async-jobs', () => ({
6+
getInlineJobQueue: async () => ({ enqueue: mocks.enqueue }),
7+
getJobQueue: async () => ({ enqueue: mocks.externalEnqueue }),
8+
}))
69
vi.mock('@/lib/slack-search/handlers/search-message', () => ({
710
handleSlackSearchMessage: mocks.run,
811
}))
@@ -32,11 +35,12 @@ describe('Slack Search queue', () => {
3235
])
3336
}
3437
})
35-
it('supplies the same handler to the database runner', async () => {
38+
it('runs in the app process even when the default queue has an external worker', async () => {
3639
const signal = new AbortController().signal
3740
await enqueueSlackSearch(job)
3841
await mocks.enqueue.mock.calls[0][2].runner(job, signal)
3942
expect(mocks.run).toHaveBeenCalledWith(job, signal)
43+
expect(mocks.externalEnqueue).not.toHaveBeenCalled()
4044
})
4145
it('propagates enqueue failures so ingress can request a retry', async () => {
4246
mocks.enqueue.mockRejectedValueOnce(new Error('unavailable'))

apps/sim/lib/slack-search/queue.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
import { generateId } from '@sim/utils/id'
2-
import { getJobQueue } from '@/lib/core/async-jobs'
2+
import { getInlineJobQueue } from '@/lib/core/async-jobs'
33
import {
44
SLACK_SEARCH_CONCURRENCY,
55
SLACK_SEARCH_MAX_DURATION_SECONDS,
66
} from '@/lib/slack-search/constants'
77

8-
/** Wakes a durable turn; a repeated wake cannot repeat a claimed execution. */
8+
/** Runs durable turns in the app process; repeated wakes cannot repeat a claimed execution. */
99
export async function enqueueSlackSearch(input: { turnId: string; installationId: string }) {
10-
return (await getJobQueue()).enqueue(
10+
return (await getInlineJobQueue()).enqueue(
1111
'slack-search',
1212
{ turnId: input.turnId },
1313
{

0 commit comments

Comments
 (0)