Async Batch Pipeline - Refactor actions-core changes - #3916
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Refactors the destination action/core pipeline to share common definition and execution logic between Action and AsyncAction, while closing parity gaps for audienceMembership feature-flag behavior and personasContext forwarding.
Changes:
- Introduces shared
CloudActionDefinitionandCloudActionbase class to consolidate schema/hook/dynamic-field/request-client logic. - Forwards
personasContextinto async batch execution and updates feature-awareaudienceMembershipresolution for async batches. - Adds unit tests for the new async batching parity behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/core/src/destination-kit/index.ts | Forwards personasContext when building async batch execution bundles. |
| packages/core/src/destination-kit/action.ts | Refactors shared logic into CloudAction/CloudActionDefinition; forwards features/personas for async batches. |
| packages/core/src/tests/batching.test.ts | Adds tests covering features forwarding and personasContext propagation to performBatch. |
| // All events in a batch share the same personas context because batching is keyed on audience/computation. | ||
| personasContext: events[0]?.context?.personas as Personas | undefined, |
| if (match) { | ||
| const [, parent, indexOrChild, child] = match | ||
| if (child) { | ||
| // It is an array, so we need to extract the index from parent.[index].child and call paret.child handler |
| /** | ||
| * A subset of the `BaseAction` that's common to both {@link ActionDefinition} and {@link AsyncActionDefinition}. | ||
| */ |
5e20569 to
cd3194a
Compare
cd3194a to
f9a1dc2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (3)
packages/core/src/destination-kit/action.ts:174
CloudActionDefinitionis introduced as a common type in the PR description, but it’s not exported here (it’s file-private). If this type is intended to be reused by external consumers or other modules (likeActionDefinition/AsyncActionDefinitionare), it should be exported (and potentially re-exported from the package entrypoint) to match the PR’s stated goal.
interface CloudActionDefinition<
Settings,
Payload,
AudienceSettings,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
GeneratedActionHookInputs = any,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
GeneratedActionHookOutputs = any
> extends BaseActionDefinition {
packages/core/src/destination-kit/action.ts:514
executeDynamicFieldis alreadyasync, so returningPromise.resolve(...)is unnecessary and adds noise. Returning the object directly improves readability and keeps return style consistent across early-return branches.
return Promise.resolve({
choices: [],
nextPage: '',
error: {
message: `No dynamic field named ${field} found.`,
code: '404'
}
})
packages/core/src/destination-kit/action.ts:1138
CloudActioncentralizes request execution and response parsing viaperformRequest(...), butperformPollbypasses that helper and returns the raw result ofperformPoll. To avoid inconsistent response handling (especially if some implementations return aResponsethat needs parsing) and to keep instrumentation behavior uniform, route polling through the sharedperformRequestpath as well.
return this.definition.performPoll(requestClient, dataBundle)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (3)
packages/core/src/destination-kit/action.ts:226
- The
eslint-disable-next-line @typescript-eslint/no-explicit-anyon line 221 applies only to the next line (export interface ActionDefinition<) which does not contain anany, so it’s likely an unused disable directive (and can fail lint ifreportUnusedDisableDirectivesis enabled). Remove this directive, or move/replace it with a disable that actually targets theanydefaults.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export interface ActionDefinition<
Settings,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Payload = any,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
packages/core/src/destination-kit/action.ts:515
- This is inside an
asyncfunction, soreturn Promise.resolve({...})is redundant. Returning the object directly is simpler and avoids an unnecessary promise construction.
if (typeof fn !== 'function') {
return Promise.resolve({
choices: [],
nextPage: '',
error: {
message: `No dynamic field named ${field} found.`,
code: '404'
}
})
}
packages/core/src/destination-kit/action.ts:169
- The PR description calls out introducing a common type
CloudActionDefinition, but it’s currently file-private (not exported). If this is intended to be a reusable type outside this file/module, it should be exported (and re-exported from the package entrypoint if needed). Otherwise, consider updating the PR description to clarify it’s an internal refactor detail.
/**
* A subset of {@link BaseActionDefinition} that's common to both {@link ActionDefinition} and {@link AsyncActionDefinition}.
*/
interface CloudActionDefinition<
Settings,
Payload,
AudienceSettings,
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/core/src/destination-kit/action.ts:166
CloudActionDefinitionis introduced per the PR description, but it is not exported. If the intent is for downstream destinations/consumers to reference this common type directly (beyond it being an internal implementation detail), it should beexport interface CloudActionDefinition<...>(or the PR description should be updated to clarify it's internal-only).
/**
* A subset of {@link BaseActionDefinition} that's common to both {@link ActionDefinition} and {@link AsyncActionDefinition}.
*/
interface CloudActionDefinition<
packages/core/src/destination-kit/action.ts:394
- The PR description says it introduces an abstract class
CloudAction, but the class is currently not exported. If this is intended to be part of the public destination-kit API (so other modules can extend it), it should beexport abstract class CloudAction<...>. If it’s intentionally internal, consider adjusting the PR description to avoid implying a new exported surface area.
abstract class CloudAction<
Settings,
Payload extends JSONLikeObject,
AudienceSettings = any,
Definition extends CloudActionDefinition<
Settings,
Payload,
AudienceSettings,
unknown,
unknown
> = CloudActionDefinition<Settings, Payload, AudienceSettings, unknown, unknown>
> extends EventEmitter {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/core/src/destination-kit/action.ts:174
ActionDefinition/AsyncActionDefinitionare exported and extendCloudActionDefinition, butCloudActionDefinitionis not exported. In TypeScript this typically fails declaration emit with an error like: "Exported interface 'ActionDefinition' has or is using private name 'CloudActionDefinition'". ExportCloudActionDefinition(or change it to an exported type alias) so the public types don’t reference a private symbol.
interface CloudActionDefinition<
Settings,
Payload,
AudienceSettings,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
GeneratedActionHookInputs = any,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
GeneratedActionHookOutputs = any
> extends BaseActionDefinition {
packages/core/src/destination-kit/action.ts:222
- This
eslint-disable-next-line @typescript-eslint/no-explicit-anyappears unnecessary here (the interface line itself doesn’t contain an explicitany, and the generic defaults already have their own per-line disables). Consider removing it to reduce lint suppression noise.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export interface ActionDefinition<

This PR refactors #3768 as follows.
CloudActionDefinition. The sub typesActionDefinitionandAsyncActionDefinitionnow extends this type.CloudAction. The sub classesActionandAsyncActionnow extends this abstract class.audienceMembershipandpersonasContextbetweenActionandAsyncAction.Testing
Testing completed successfully in local and in staging.
Changes were deployed to stage for more than 24 hours.
Security Review
Please ensure sensitive data is properly protected in your integration.
type: 'password'New Destination Checklist
verioning-info.tsfile. example