Skip to content

Commit 924cbab

Browse files
icecrasher321claude
andcommitted
fix(selectors): restore the credential-group provider label resolver, and probe getQueryKey for missing dependsOn
Two findings from a final adversarial pass over the migration, both the same class as the three the review bots caught: something a `fetchOptions` sub-block declared that its replacement selector quietly does not. `credential-group.providerFilter` had a `fetchOptionById`; `workspace.credentialGroupProviders` had no `fetchById`, so the canvas card summarising several stored provider ids lost every label. The field is multi-select, which is exactly when a label has to resolve without the full list. The `dependsOn` assertion in `selector-backed-subblocks.test.ts` only probed `enabled` against three hand-listed context fields, which is why it caught `clickup.triggerWorkspaceId` and would have missed the rest. It now probes `getQueryKey` as well — a selector's key names every context field its RESULT depends on — and derives the sub-block-sourced set from `SELECTOR_CONTEXT_FIELDS` rather than a literal. Verified by deleting a real `dependsOn`: it fails naming the field and the fields it depends on. Also checked and NOT changed: `display.ts` and the copilot dropdown validator both guard `options` before use, so stripping `options: []` does not reach them. The validator's behaviour does shift from "reject every value" (an empty `validIds` array matched nothing) to "skip validation", which is a relaxation rather than a regression. `function.sandboxId` kept its `dependsOn: ['language']`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent f2499f3 commit 924cbab

2 files changed

Lines changed: 75 additions & 36 deletions

File tree

apps/sim/blocks/selector-backed-subblocks.test.ts

Lines changed: 39 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { describe, expect, it, vi } from 'vitest'
55

66
vi.unmock('@/blocks/registry')
77

8+
import { SELECTOR_CONTEXT_FIELDS } from '@/lib/workflows/subblocks/context'
89
import { getAllBlocks } from '@/blocks/registry'
910
import { getSelectorDefinition } from '@/hooks/selectors/registry'
1011
import type { SelectorKey } from '@/hooks/selectors/types'
@@ -17,6 +18,17 @@ import type { SelectorKey } from '@/hooks/selectors/types'
1718
* has to tolerate the absence. The controls read `options` on first paint, before any fetch
1819
* resolves, and an unguarded `.map` there takes out the whole editor for that block.
1920
*/
21+
/**
22+
* Context fields supplied by a sibling SUB-BLOCK value. `workspaceId` / `workflowId` come from
23+
* the surface itself and `excludeWorkflowId` from a flag, so none of them can be declared as a
24+
* `dependsOn` and none belong here.
25+
*/
26+
const SUB_BLOCK_SOURCED = new Set(
27+
[...SELECTOR_CONTEXT_FIELDS].filter(
28+
(field) => field !== 'workspaceId' && field !== 'workflowId' && field !== 'excludeWorkflowId'
29+
)
30+
)
31+
2032
describe('selector-backed sub-blocks', () => {
2133
const selectorBacked = getAllBlocks().flatMap((block) =>
2234
((block.subBlocks ?? []) as Array<Record<string, any>>)
@@ -51,33 +63,39 @@ describe('selector-backed sub-blocks', () => {
5163
}
5264
})
5365

54-
it('declares dependsOn for every context field its selector requires', () => {
55-
// A selector gated on `enabled` returns nothing until its context is populated, and the
56-
// context is only rebuilt when a `dependsOn` sibling changes. Without the declaration the
57-
// list loads once, empty, and never refetches when the credential is picked.
58-
const CONTEXT_SOURCED = new Set(['oauthCredential', 'credentialGroupId', 'tableId'])
66+
it('declares dependsOn for every sub-block-sourced context field its selector reads', () => {
67+
// A selector's `getQueryKey` names every context field its RESULT depends on, and `enabled`
68+
// names what it is gated on. Both are probed rather than listing fields by hand, which is
69+
// what let `clickup.triggerWorkspaceId` ship without a `dependsOn`.
70+
//
71+
// The declaration is what makes the list refetch: `useFetchedOptions` resets its fetch scope
72+
// on `dependsOn` values changing. Without it the list loads once — before the credential is
73+
// picked, or against the old language — and never reloads.
5974
for (const { block, sub } of selectorBacked) {
6075
const definition = getSelectorDefinition(sub.selectorKey as SelectorKey)
61-
if (!definition.enabled) continue
6276
const probed = new Set<string>()
63-
definition.enabled({
64-
key: definition.key,
65-
context: new Proxy({} as Record<string, unknown>, {
66-
get: (_target, property) => {
67-
if (typeof property === 'string') probed.add(property)
68-
return undefined
69-
},
70-
}),
77+
const context = new Proxy({} as Record<string, unknown>, {
78+
get: (_target, property) => {
79+
if (typeof property === 'string') probed.add(property)
80+
return undefined
81+
},
7182
})
72-
const needsContext = [...probed].some((field) => CONTEXT_SOURCED.has(field))
73-
if (!needsContext) continue
83+
const args = { key: definition.key, context }
84+
definition.getQueryKey(args)
85+
definition.enabled?.(args)
86+
87+
const needed = [...probed].filter((field) => SUB_BLOCK_SOURCED.has(field))
88+
if (needed.length === 0) continue
89+
7490
const dependsOn = sub.dependsOn
75-
const declared = Array.isArray(dependsOn)
76-
? dependsOn.length > 0
77-
: Boolean(dependsOn?.all?.length || dependsOn?.any?.length)
91+
const declared = new Set<string>(
92+
Array.isArray(dependsOn)
93+
? dependsOn
94+
: [...(dependsOn?.all ?? []), ...(dependsOn?.any ?? [])]
95+
)
7896
expect(
79-
declared,
80-
`${block}.${sub.id} uses ${sub.selectorKey}, which is gated on context, but declares no dependsOn`
97+
declared.size > 0,
98+
`${block}.${sub.id} uses ${sub.selectorKey}, whose result depends on ${needed.join(', ')}, but declares no dependsOn — its list would never refetch`
8199
).toBe(true)
82100
}
83101
})

apps/sim/hooks/selectors/providers/workspace/selectors.ts

Lines changed: 36 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -109,24 +109,45 @@ export const workspaceSelectors = {
109109
},
110110
},
111111
/** Providers represented inside ONE credential group, for its per-provider filter. */
112-
'workspace.credentialGroupProviders': workspaceScoped(
113-
'workspace.credentialGroupProviders',
114-
async (workspaceId, context) => {
115-
if (!context.credentialGroupId) return []
116-
const group = (await credentialGroups(workspaceId)).find(
112+
'workspace.credentialGroupProviders': {
113+
...workspaceScoped(
114+
'workspace.credentialGroupProviders',
115+
async (workspaceId, context) => {
116+
if (!context.credentialGroupId) return []
117+
const group = (await credentialGroups(workspaceId)).find(
118+
(candidate) => candidate.id === context.credentialGroupId
119+
)
120+
if (!group) return []
121+
return group.options
122+
.filter((option) => option.status === 'active')
123+
.map((option) => {
124+
const service = getCredentialGroupProviderService(option.provider)
125+
return { id: service.providerId, label: service.name }
126+
})
127+
.sort((a, b) => a.label.localeCompare(b.label))
128+
},
129+
(context) => context.credentialGroupId ?? 'none'
130+
),
131+
/**
132+
* Resolves one stored provider id to its service name. The field is multi-select, so the
133+
* canvas card summarises several stored ids at once and needs each label before (or
134+
* without) the full list — which is what `useDynamicSubBlockOptionDisplayName` asks for.
135+
*/
136+
fetchById: async ({ context, detailId }: SelectorQueryArgs) => {
137+
if (!context.workspaceId || !context.credentialGroupId || !detailId) return null
138+
const group = (await credentialGroups(context.workspaceId)).find(
117139
(candidate) => candidate.id === context.credentialGroupId
118140
)
119-
if (!group) return []
120-
return group.options
121-
.filter((option) => option.status === 'active')
122-
.map((option) => {
123-
const service = getCredentialGroupProviderService(option.provider)
124-
return { id: service.providerId, label: service.name }
125-
})
126-
.sort((a, b) => a.label.localeCompare(b.label))
141+
const option = group?.options.find(
142+
(candidate) =>
143+
candidate.status === 'active' &&
144+
getCredentialGroupProviderService(candidate.provider).providerId === detailId
145+
)
146+
if (!option) return null
147+
const service = getCredentialGroupProviderService(option.provider)
148+
return { id: service.providerId, label: service.name }
127149
},
128-
(context) => context.credentialGroupId ?? 'none'
129-
),
150+
},
130151
/**
131152
* Secret NAMES the workspace can resolve. Names only — values stay server-side and are
132153
* injected at execution. Both halves come from the one workspace-environment response, the

0 commit comments

Comments
 (0)