Skip to content

Commit 1ddd530

Browse files
committed
fix(sim-search): keep Slack searchable when it is the only source, and tell a member to reconnect
1 parent aa3a1cd commit 1ddd530

8 files changed

Lines changed: 77 additions & 34 deletions

File tree

apps/sim/app/workspace/[workspaceId]/home/components/knowledge-search-results/knowledge-search-results.tsx

Lines changed: 25 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -226,28 +226,28 @@ export function KnowledgeSearchResults({
226226
}, [documents, filtersActive, filters.source, filters.updated])
227227

228228
const failure = basesError ?? error
229-
if (failure) {
230-
return <p className='px-2 py-2 text-[var(--text-error)] text-caption'>{failure.message}</p>
231-
}
232-
if (!basesPending && knowledgeBaseIds.length === 0) {
233-
return (
234-
<p className='px-2 py-2 text-[var(--text-muted)] text-caption'>
235-
Nothing to search yet. Clear the query and connect a source to index what you can open.
236-
</p>
237-
)
238-
}
239-
/** Kept results belong to the previous query; a new query shows its own state. */
240-
if (isPending || isPlaceholderData || (isFetching && !results)) {
241-
return <p className='px-2 py-2 text-[var(--text-muted)] text-caption'>Searching…</p>
242-
}
243-
244229
const indexingNote =
245230
indexing.length > 0
246231
? `Still indexing ${indexing.join(', ')}; results grow as documents land.`
247232
: null
248233

249-
return (
250-
<div className='flex flex-col'>
234+
/**
235+
* The indexed half of the page, in whatever state it is in. It is a branch
236+
* rather than an early return because a federated source is searched even
237+
* where there is nothing indexed at all — a workspace whose only source is
238+
* Slack has no knowledge base, and its failures are not Slack's.
239+
*/
240+
const knowledgeSection = failure ? (
241+
<p className='px-2 py-2 text-[var(--text-error)] text-caption'>{failure.message}</p>
242+
) : !basesPending && knowledgeBaseIds.length === 0 ? (
243+
<p className='px-2 py-2 text-[var(--text-muted)] text-caption'>
244+
No indexed sources yet. Connect one to index what you can open.
245+
</p>
246+
) : /** Kept results belong to the previous query; a new query shows its own state. */
247+
isPending || isPlaceholderData || (isFetching && !results) ? (
248+
<p className='px-2 py-2 text-[var(--text-muted)] text-caption'>Searching…</p>
249+
) : (
250+
<>
251251
<div className='flex items-center gap-2 px-2 py-2'>
252252
<span className='min-w-0 flex-1 text-[var(--text-muted)] text-caption'>
253253
<span className='tabular-nums'>
@@ -299,7 +299,7 @@ export function KnowledgeSearchResults({
299299
: 'No documents match these filters.'}
300300
</p>
301301
) : (
302-
<div className='flex flex-col' onKeyDown={handleResultsKeyDown}>
302+
<div className='flex flex-col'>
303303
{visible.map((result) => {
304304
const source = toSource(result, query)
305305
return source ? (
@@ -317,6 +317,13 @@ export function KnowledgeSearchResults({
317317
})}
318318
</div>
319319
)}
320+
</>
321+
)
322+
323+
/** One keyboard container over both groups, so the arrows walk every result. */
324+
return (
325+
<div className='flex flex-col' onKeyDown={handleResultsKeyDown}>
326+
{knowledgeSection}
320327
<SlackSearchResults workspaceId={workspaceId} query={query} onSummarize={onSummarize} />
321328
</div>
322329
)

apps/sim/hooks/queries/kb/knowledge.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ import {
5454
type WorkspaceKnowledgeSearchBody,
5555
type WorkspaceKnowledgeSearchResult,
5656
} from '@/lib/api/contracts/knowledge'
57+
import { useSession } from '@/lib/auth/auth-client'
5758
import type { ChunkingStrategy, StrategyOptions } from '@/lib/chunkers/types'
5859
import type { DocumentSortField, SortOrder } from '@/lib/knowledge/documents/types'
5960
import { folderKeys } from '@/hooks/queries/utils/folder-keys'
@@ -1212,11 +1213,14 @@ async function searchSlack(body: SearchSimSearchSlackBody, signal?: AbortSignal)
12121213
*/
12131214
export function useSimSearchSlack(workspaceId: string | undefined, query: string) {
12141215
const trimmed = query.trim()
1216+
const { data: session } = useSession()
1217+
const viewerId = session?.user?.id
12151218
return useQuery({
1216-
queryKey: knowledgeKeys.slackSearch(workspaceId, trimmed),
1219+
queryKey: knowledgeKeys.slackSearch(workspaceId, viewerId, trimmed),
12171220
queryFn: ({ signal }) =>
12181221
searchSlack({ workspaceId: workspaceId as string, query: trimmed }, signal),
1219-
enabled: Boolean(workspaceId) && trimmed.length > 0,
1222+
/** Held until the viewer is known, so no answer is ever cached under an empty identity. */
1223+
enabled: Boolean(workspaceId) && Boolean(viewerId) && trimmed.length > 0,
12201224
staleTime: SIM_SEARCH_SLACK_STALE_TIME,
12211225
placeholderData: keepPreviousData,
12221226
})

apps/sim/hooks/queries/utils/knowledge-keys.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,13 @@ export const knowledgeKeys = {
3232
[...knowledgeKeys.details(), knowledgeBaseId ?? ''] as const,
3333
searches: () => [...knowledgeKeys.all, 'search'] as const,
3434
slackSearches: () => [...knowledgeKeys.all, 'slackSearch'] as const,
35-
slackSearch: (workspaceId: string | undefined, query: string) =>
36-
[...knowledgeKeys.slackSearches(), workspaceId ?? '', query] as const,
35+
/**
36+
* Keyed by viewer as well as workspace: the answer is one person's own Slack,
37+
* including their direct messages, so a session change in an open tab must
38+
* never be served another person's cached results.
39+
*/
40+
slackSearch: (workspaceId: string | undefined, userId: string | undefined, query: string) =>
41+
[...knowledgeKeys.slackSearches(), workspaceId ?? '', userId ?? '', query] as const,
3742
search: (workspaceId: string | undefined, knowledgeBaseIds: readonly string[], query: string) =>
3843
[
3944
...knowledgeKeys.searches(),

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -133,8 +133,12 @@ describe('searchSlack', () => {
133133
)
134134
})
135135

136-
it('raises for a transport failure', async () => {
137-
fetchMock.mockResolvedValue(new Response('nope', { status: 429 }))
136+
it('raises for a transport failure, releasing the unread body', async () => {
137+
const response = new Response('nope', { status: 429 })
138+
const cancel = vi.spyOn(response.body as ReadableStream, 'cancel')
139+
fetchMock.mockResolvedValue(response)
140+
138141
await expect(searchSlack({ accessToken: 'token', query: 'deploy' })).rejects.toThrow('http_429')
142+
expect(cancel).toHaveBeenCalled()
139143
})
140144
})

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,8 @@ export async function searchSlack({
153153
})
154154

155155
if (!response.ok) {
156+
/** Nothing here reads the body, and an uncancelled one holds the connection open. */
157+
await response.body?.cancel().catch(() => {})
156158
throw new SlackSearchError(`http_${response.status}`)
157159
}
158160

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

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,17 @@ import { getCredentialGroupProviderId } from '@/lib/credential-groups/providers'
88
* The Slack account a federated search runs as: the asking person's own,
99
* enrolled through a Credential Group in this workspace.
1010
*
11-
* The joins mirror the knowledge access scope exactly — verified email, live
12-
* enrollment, active group, active managed credential, active option — because
13-
* both answer the same question about the same rows, and a search that used a
14-
* looser rule than the one governing document access would be the odd one out.
15-
* Nothing is cached: revoking a credential takes effect on the next search.
11+
* Deliberately does not filter on `managedOauthStatus`. A credential that needs
12+
* authorizing again is still a connection the person made, and the difference
13+
* between "you never connected Slack" and "reconnect Slack" is the whole of
14+
* what the surface can tell them to do. Excluding it here would collapse the
15+
* second into the first — which is exactly what a scope-policy change does to
16+
* every enrolled credential at once. `resolveManagedOAuthToken` classifies it,
17+
* and an active credential is preferred when a person somehow holds several.
18+
*
19+
* The group must belong to this workspace as well as the credential: the two
20+
* are set together today, and requiring both keeps a workspace's search inside
21+
* its own groups even if they ever diverge.
1622
*/
1723
export async function findViewerSlackCredentialId(params: {
1824
workspaceId: string
@@ -35,6 +41,7 @@ export async function findViewerSlackCredentialId(params: {
3541
credentialGroup,
3642
and(
3743
eq(credentialGroup.id, credentialGroupEnrollment.credentialGroupId),
44+
eq(credentialGroup.workspaceId, params.workspaceId),
3845
eq(credentialGroup.status, 'active')
3946
)
4047
)
@@ -44,7 +51,6 @@ export async function findViewerSlackCredentialId(params: {
4451
eq(credential.credentialGroupEnrollmentId, credentialGroupEnrollment.id),
4552
eq(credential.workspaceId, params.workspaceId),
4653
eq(credential.type, 'managed_oauth'),
47-
eq(credential.managedOauthStatus, 'active'),
4854
eq(credential.providerId, getCredentialGroupProviderId('slack')),
4955
sql`EXISTS (
5056
SELECT 1 FROM jsonb_array_elements(${credentialGroup.options}) AS option
@@ -54,6 +60,7 @@ export async function findViewerSlackCredentialId(params: {
5460
)
5561
)
5662
.where(and(eq(user.id, params.userId), eq(user.emailVerified, true)))
63+
.orderBy(sql`CASE WHEN ${credential.managedOauthStatus} = 'active' THEN 0 ELSE 1 END`)
5764
.limit(1)
5865

5966
return row?.credentialId ?? null

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,4 +95,10 @@ describe('searchSlackForViewer', () => {
9595
mocks.resolveManagedOAuthToken.mockRejectedValue(new Error('pool exhausted'))
9696
await expect(searchSlackForViewer(params)).resolves.toEqual({ status: 'unavailable' })
9797
})
98+
99+
it('absorbs a failure looking the credential up at all', async () => {
100+
mocks.findViewerSlackCredentialId.mockRejectedValue(new Error('connection terminated'))
101+
await expect(searchSlackForViewer(params)).resolves.toEqual({ status: 'unavailable' })
102+
expect(mocks.searchSlack).not.toHaveBeenCalled()
103+
})
98104
})

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

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,18 @@ export interface SearchSlackForViewerParams {
6969
export async function searchSlackForViewer(
7070
params: SearchSlackForViewerParams
7171
): Promise<SlackSearchOutcome> {
72-
const credentialId = await findViewerSlackCredentialId({
73-
workspaceId: params.workspaceId,
74-
userId: params.userId,
75-
})
72+
let credentialId: string | null
73+
try {
74+
credentialId = await findViewerSlackCredentialId({
75+
workspaceId: params.workspaceId,
76+
userId: params.userId,
77+
})
78+
} catch (error) {
79+
logger.error('Failed to look up a Slack search credential', {
80+
error: getErrorMessage(error),
81+
})
82+
return { status: 'unavailable' }
83+
}
7684
if (!credentialId) return { status: 'not_connected' }
7785

7886
let accessToken: string

0 commit comments

Comments
 (0)