Skip to content

Commit 345bfe2

Browse files
fix(slack-search): repair execution, OAuth, and onboarding (#7666)
* fix(slack-search): run turns inline and preserve OAuth callbacks * fix(slack-search): simplify setup and subscribe to app home events * fix(slack-search): make source prompts ephemeral and guard OAuth deadlines * chore(slack-search): remove implementation README * fix(slack-search): acknowledge missing sources in the thread * fix(slack-search): preserve existing app configuration updates
1 parent 778d445 commit 345bfe2

12 files changed

Lines changed: 457 additions & 364 deletions

File tree

apps/sim/app/o/[organizationId]/settings/components/organization-search-slack.test.tsx

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ const mocks = vi.hoisted(() => ({
1212
remove: vi.fn(),
1313
install: vi.fn(),
1414
refetch: vi.fn(),
15+
copy: vi.fn(),
1516
removeError: null as Error | null,
1617
}))
1718
vi.mock('nuqs', () => ({ useQueryState: () => [null, vi.fn()] }))
@@ -53,6 +54,8 @@ let container: HTMLDivElement
5354
beforeEach(() => {
5455
vi.clearAllMocks()
5556
vi.stubGlobal('IS_REACT_ACT_ENVIRONMENT', true)
57+
vi.stubGlobal('navigator', { clipboard: { writeText: mocks.copy } })
58+
mocks.copy.mockReset().mockResolvedValue(undefined)
5659
mocks.context.mockReturnValue({ organization: { id: 'org-1' }, viewer: { isAdmin: true } })
5760
mocks.list.mockReturnValue({ data: { installations: [], bots: [] } })
5861
mocks.manifest.mockReturnValue({
@@ -104,11 +107,14 @@ async function action(label: string) {
104107
}
105108

106109
describe('Slack Search settings and shared wizard', () => {
107-
it('starts with one setup action and the prefilled manifest, with no name or token form', async () => {
110+
it('starts with one setup action and a Slack app link, with no manifest preview or form', async () => {
108111
await render()
109112
expect(container.querySelectorAll('button')).toHaveLength(1)
110113
await click('Set up')
111-
expect(document.querySelector('[role="dialog"]')).toHaveTextContent('App manifest')
114+
expect(document.querySelector('[role="dialog"]')).not.toHaveTextContent('App manifest')
115+
expect(document.querySelector('a[href="https://api.slack.com/apps"]')).toHaveTextContent(
116+
'Create app in Slack'
117+
)
112118
expect(document.querySelectorAll('input')).toHaveLength(0)
113119
expect(mocks.manifest).toHaveBeenCalledWith('org-1', 'Sim Search')
114120
expect(mocks.install).not.toHaveBeenCalled()
@@ -135,7 +141,11 @@ describe('Slack Search settings and shared wizard', () => {
135141
expect(container.textContent).toContain('Enabled')
136142
await action('Reconnect')
137143
expect(document.querySelector('[role="dialog"]')).toHaveTextContent('Reconnect Slack Search')
144+
await click('Copy app configuration')
145+
expect(mocks.copy).toHaveBeenCalledExactlyOnceWith('{}')
138146
expect(document.querySelector('a[href="https://api.slack.com/apps/A1"]')).not.toBeNull()
147+
expect(document.querySelector('[role="dialog"]')).toHaveTextContent('Configuration copied')
148+
expect(document.querySelector('pre')).toBeNull()
139149
await click('Continue')
140150
expect(document.querySelector('[role="dialog"]')).toHaveTextContent('Leave fields blank')
141151
await click('Continue')
@@ -151,6 +161,40 @@ describe('Slack Search settings and shared wizard', () => {
151161
expect(mocks.install.mock.calls[0][0]).not.toHaveProperty('clientSecret')
152162
})
153163

164+
it('keeps the update action available when clipboard access fails', async () => {
165+
mocks.copy.mockRejectedValueOnce(new Error('Clipboard access denied'))
166+
await render(true)
167+
await action('Reconnect')
168+
await click('Copy app configuration')
169+
expect(document.querySelector('[role="alert"]')).toHaveTextContent('Allow clipboard access')
170+
expect(document.querySelector('a[href="https://api.slack.com/apps/A1"]')).toBeNull()
171+
expect(button('Copy app configuration')).toBeDefined()
172+
await click('Copy app configuration')
173+
expect(document.querySelector('[role="alert"]')).toBeNull()
174+
expect(document.querySelector('a[href="https://api.slack.com/apps/A1"]')).not.toBeNull()
175+
})
176+
177+
it('offers the same configuration update for an app shared with Slack sources', async () => {
178+
mocks.manifest.mockReturnValue({
179+
data: {
180+
manifest: '{"display_information":{"name":"Shared Slack app"}}',
181+
existingApp: { appId: 'A2' },
182+
createAppUrl: 'https://api.slack.com/apps',
183+
},
184+
isPending: false,
185+
refetch: mocks.refetch,
186+
})
187+
await render()
188+
await click('Set up')
189+
expect(document.querySelector('[role="dialog"]')).toHaveTextContent('Update your Slack app')
190+
await click('Copy app configuration')
191+
expect(mocks.copy).toHaveBeenCalledExactlyOnceWith(
192+
'{"display_information":{"name":"Shared Slack app"}}'
193+
)
194+
expect(document.querySelector('a[href="https://api.slack.com/apps/A2"]')).not.toBeNull()
195+
expect(document.querySelector('pre')).toBeNull()
196+
})
197+
154198
it('disables the selected connection from the actions menu', async () => {
155199
await render(true)
156200
await action('Disable')

apps/sim/components/integrations/slack-search-setup-wizard.tsx

Lines changed: 33 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,9 @@ import {
1010
ChipModalField,
1111
ChipModalFooter,
1212
ChipModalHeader,
13+
writeTextToClipboard,
1314
} from '@sim/emcn'
1415
import { SlackIcon } from '@/components/icons'
15-
import { SlackAppManifest } from '@/components/integrations/slack-app-manifest'
1616
import {
1717
SLACK_SEARCH_DEFAULT_DESCRIPTION,
1818
SLACK_SEARCH_DEFAULT_NAME,
@@ -43,11 +43,26 @@ export function SlackSearchSetupWizard({
4343
const [clientId, setClientId] = useState('')
4444
const [clientSecret, setClientSecret] = useState('')
4545
const [signingSecret, setSigningSecret] = useState('')
46-
const error = prepare.error ?? oauth.error
46+
const [configurationCopied, setConfigurationCopied] = useState(false)
47+
const [copyError, setCopyError] = useState<Error | null>(null)
48+
const error = prepare.error ?? oauth.error ?? copyError
4749
const busy = oauth.isPending
4850
const stepNumber = step === 'manifest' ? 1 : step === 'credentials' ? 2 : 3
4951
const configuredAppId = appId ?? prepare.data?.existingApp?.appId
5052

53+
async function copyConfiguration() {
54+
if (!prepare.data) throw new Error('Slack app configuration is not ready')
55+
setCopyError(null)
56+
try {
57+
await writeTextToClipboard(prepare.data.manifest)
58+
setConfigurationCopied(true)
59+
} catch {
60+
setCopyError(
61+
new Error('Could not copy the app configuration. Allow clipboard access and try again.')
62+
)
63+
}
64+
}
65+
5166
function advance() {
5267
if (step === 'manifest') {
5368
setStep('credentials')
@@ -101,11 +116,20 @@ export function SlackSearchSetupWizard({
101116
</p>
102117
)}
103118
{step === 'manifest' && prepare.data && (
104-
<>
105-
<ChipModalField
106-
type='custom'
107-
title={installationId ? 'Update your Slack app' : 'Create your Slack app'}
108-
>
119+
<ChipModalField
120+
type='custom'
121+
title={configuredAppId ? 'Update your Slack app' : 'Create your Slack app'}
122+
hint={
123+
configuredAppId
124+
? configurationCopied
125+
? 'Configuration copied. In Slack, open App Manifest, select JSON, replace the configuration, and save your changes before continuing.'
126+
: 'Copy the updated configuration, then open your app in Slack to apply it.'
127+
: undefined
128+
}
129+
>
130+
{configuredAppId && !configurationCopied ? (
131+
<Chip onClick={() => void copyConfiguration()}>Copy app configuration</Chip>
132+
) : (
109133
<ChipLink
110134
href={
111135
configuredAppId
@@ -117,16 +141,8 @@ export function SlackSearchSetupWizard({
117141
>
118142
{configuredAppId ? 'Open Slack app settings' : 'Create app in Slack'}
119143
</ChipLink>
120-
<p className='text-[var(--text-secondary)] text-caption'>
121-
{configuredAppId
122-
? 'Open App Manifest in your existing app and apply the updated configuration.'
123-
: 'Choose your Slack workspace, review the prepared manifest, then create the app.'}
124-
</p>
125-
</ChipModalField>
126-
<ChipModalField type='custom' title='App manifest'>
127-
<SlackAppManifest manifest={prepare.data.manifest} />
128-
</ChipModalField>
129-
</>
144+
)}
145+
</ChipModalField>
130146
)}
131147
{step === 'credentials' && (
132148
<>

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

Lines changed: 155 additions & 3 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,145 @@ 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('keeps a new authorization intact if an old deadline callback runs', async () => {
246+
vi.useFakeTimers()
247+
const timeouts = vi.spyOn(window, 'setTimeout')
248+
await render()
249+
await submit()
250+
const oldDeadline = timeouts.mock.calls.find(([, delay]) => delay === 10 * 60 * 1_000)?.[0]
251+
if (typeof oldDeadline !== 'function') throw new Error('Authorization deadline was not set')
252+
await clickButton('Cancel')
253+
254+
const nextPopup = { location: { href: '' }, closed: false, close: vi.fn() }
255+
vi.mocked(window.open).mockReturnValueOnce(nextPopup as unknown as Window)
256+
mocks.start.mockResolvedValueOnce({
257+
state: 'new-state',
258+
authorizationUrl: 'https://slack.com/oauth/v2/authorize',
259+
})
260+
await submit()
261+
await act(async () => oldDeadline())
262+
263+
expect(toast.error).not.toHaveBeenCalled()
264+
expect(nextPopup.close).not.toHaveBeenCalled()
265+
expect(document.body.textContent).toContain('Waiting for Slack...')
266+
await completeAuthorization('new-state')
267+
expect(toast.success).toHaveBeenCalledExactlyOnceWith('Slack configured')
268+
await act(async () => vi.advanceTimersByTimeAsync(10 * 60 * 1_000))
269+
expect(toast.error).not.toHaveBeenCalled()
270+
})
271+
272+
it('does not navigate or start a timeout when authorization startup finishes after cancel', async () => {
273+
vi.useFakeTimers()
274+
let finishStartup!: (value: { state: string; authorizationUrl: string }) => void
275+
mocks.start.mockReturnValueOnce(
276+
new Promise((resolve) => {
277+
finishStartup = resolve
278+
})
279+
)
280+
await render()
281+
await submit()
282+
await clickButton('Cancel')
283+
await act(async () => {
284+
finishStartup({ state: 'state', authorizationUrl: 'https://slack.com/oauth/v2/authorize' })
285+
})
286+
expect(popup.location.href).toBe('')
287+
await act(async () => vi.advanceTimersByTimeAsync(10 * 60 * 1_000))
288+
expect(toast.error).not.toHaveBeenCalled()
289+
})
290+
291+
it.each(['resolve', 'reject'] as const)(
292+
'ignores authorization startup that completes with %s after unmount',
293+
async (outcome) => {
294+
vi.useFakeTimers()
295+
let finishStartup!: () => void
296+
mocks.start.mockReturnValueOnce(
297+
new Promise((resolve, reject) => {
298+
finishStartup = () =>
299+
outcome === 'resolve'
300+
? resolve({
301+
state: 'state',
302+
authorizationUrl: 'https://slack.com/oauth/v2/authorize',
303+
})
304+
: reject(new Error('Authorization startup failed'))
305+
})
306+
)
307+
await render()
308+
await submit()
309+
await act(async () => root.render(null))
310+
await act(async () => finishStartup())
311+
312+
expect(popup.close).toHaveBeenCalledOnce()
313+
expect(popup.location.href).toBe('')
314+
await act(async () => vi.advanceTimersByTimeAsync(10 * 60 * 1_000))
315+
expect(toast.error).not.toHaveBeenCalled()
316+
expect(toast.success).not.toHaveBeenCalled()
317+
}
318+
)
319+
169320
it('opens Slack app setup inline and returns to member setup when canceled', async () => {
170321
await render(undefined, [])
171322
expect(document.querySelector('a')).toBeNull()
@@ -199,7 +350,8 @@ describe('Slack member access selection', () => {
199350
const dialog = appSetupDialog(true)
200351
expect(dialog).toBeDefined()
201352
expect(dialog?.textContent).toContain('Step 1 of 3')
202-
expect(dialog?.textContent).toContain('App manifest')
353+
expect(dialog?.textContent).not.toContain('App manifest')
354+
expect(dialog?.textContent).toContain('Create app in Slack')
203355
expect(mocks.manifest).toHaveBeenCalledWith('org-1', 'Sim Search')
204356
expect(mocks.start).not.toHaveBeenCalled()
205357
expect(mocks.create).not.toHaveBeenCalled()

0 commit comments

Comments
 (0)