app deleted shared handler#2343
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
5 Skipped Deployments
|
🦋 Changeset detectedLatest commit: 0418fee The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Differences Found✅ No packages or licenses were added. SummaryExpand
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2343 +/- ##
==========================================
- Coverage 37.84% 37.76% -0.09%
==========================================
Files 1037 1040 +3
Lines 66937 67099 +162
Branches 3533 3536 +3
==========================================
+ Hits 25335 25339 +4
- Misses 41221 41377 +156
- Partials 381 383 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds a shared APP_DELETED webhook handler to @saleor/webhook-utils and wires it into the Klaviyo app so that APL auth data is cleaned up automatically when an app installation is deleted in Saleor.
Changes:
- Introduce
createAppDeletedHandler()in@saleor/webhook-utilsbacked by a newAppDeletedGraphQL subscription. - Add a new Klaviyo webhook API route (
/api/webhooks/app-deleted) and include it in the app manifest. - Update workspace deps/lockfile and add a changeset for the new webhook-utils functionality.
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Locks new workspace linkage for @saleor/webhook-utils and related dependency graph changes. |
| packages/webhook-utils/src/app-deleted-handler.ts | New shared handler that receives APP_DELETED and deletes APL auth data. |
| packages/webhook-utils/package.json | Exposes a new subpath export for ./app-deleted-handler and declares logger peer dependency. |
| packages/webhook-utils/graphql/subscriptions/app-deleted.graphql | Adds the AppDeleted subscription definition used by the webhook. |
| packages/webhook-utils/generated/graphql.ts | Updates generated GraphQL types/documents to include AppDeleted. |
| apps/klaviyo/src/pages/api/webhooks/app-deleted.ts | New webhook route using the shared handler. |
| apps/klaviyo/src/pages/api/manifest.ts | Adds the APP_DELETED webhook manifest to the Klaviyo app manifest response. |
| apps/klaviyo/package.json | Adds @saleor/webhook-utils dependency. |
| .changeset/petite-days-watch.md | Declares a minor bump for @saleor/webhook-utils for the new shared webhook. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| const { handler, getWebhookManifest } = createAppDeletedHandler({ | ||
| apl: saleorApp.apl, | ||
| logger: createLogger("APP_DELETED handler"), | ||
| webhookPath: "api/webhooks/app-deleted", | ||
|
|
There was a problem hiding this comment.
Unlike the other webhook routes in this app, this handler is not wrapped with wrapWithLoggerContext and withSpanAttributes, so logs/traces for APP_DELETED may miss the usual context and OpenTelemetry attributes. Consider wrapping the exported handler consistently (or have createAppDeletedHandler expose a wrapped handler option).
| // todo something failing here | ||
| console.log("asdf"); |
There was a problem hiding this comment.
Remove the leftover debug console.log("asdf") (and the adjacent TODO). This will pollute logs in production and bypasses the app logger/observability pipeline used elsewhere in the repo.
| // todo something failing here | |
| console.log("asdf"); |
| import { type Logger } from "@saleor/apps-logger"; | ||
|
|
||
| import { AppDeletedDocument } from "../generated/graphql"; | ||
|
|
||
| type Params = { | ||
| apl: APL; | ||
| webhookPath: string; | ||
| logger: Logger; |
There was a problem hiding this comment.
This new API requires Logger from @saleor/apps-logger, but the package already defines its own Logger type in src/types.ts (used by other exports). Having two incompatible logger types in the same package is confusing for consumers; consider accepting a minimal logger interface (info/warn/error/debug) or reusing the existing src/types.ts Logger shape and letting apps pass their logger adapter.
| import { type Logger } from "@saleor/apps-logger"; | |
| import { AppDeletedDocument } from "../generated/graphql"; | |
| type Params = { | |
| apl: APL; | |
| webhookPath: string; | |
| logger: Logger; | |
| import { AppDeletedDocument } from "../generated/graphql"; | |
| type MinimalLogger = { | |
| info: (message: string, ...meta: unknown[]) => void; | |
| warn?: (message: string, ...meta: unknown[]) => void; | |
| error: (message: string, ...meta: unknown[]) => void; | |
| debug?: (message: string, ...meta: unknown[]) => void; | |
| }; | |
| type Params = { | |
| apl: APL; | |
| webhookPath: string; | |
| logger: MinimalLogger; |
| export const createAppDeletedHandler = ({ apl, webhookPath, hooks = {}, logger }: Params) => { | ||
| const webhook = new SaleorAsyncWebhook({ | ||
| apl, | ||
| name: "APP_DELETED", | ||
| query: AppDeletedDocument, |
There was a problem hiding this comment.
This introduces new webhook-handling behavior (creating a SaleorAsyncWebhook and deleting APL entries) but there are no unit tests covering the success/error paths. Since this package already uses Vitest tests, add tests that verify apl.delete is called with the Saleor API URL and that the handler returns 200 on success and 500 on failure.
| const { handler, getWebhookManifest } = createAppDeletedHandler({ | ||
| apl: saleorApp.apl, | ||
| logger: createLogger("APP_DELETED handler"), | ||
| webhookPath: "api/webhooks/app-deleted", | ||
|
|
||
| hooks: { | ||
| onEvent: console.log, | ||
| onAuthDataDeleted: console.log, | ||
| onAuthDataDeleteError: console.log, |
There was a problem hiding this comment.
hooks.onEvent: console.log will likely print the entire WebhookContext, which includes authData (e.g. token) in other webhook handlers. This is a sensitive-data leakage risk; avoid logging the full context and log only non-sensitive fields via the app logger (or remove these hooks from the production route).
| const { handler, getWebhookManifest } = createAppDeletedHandler({ | |
| apl: saleorApp.apl, | |
| logger: createLogger("APP_DELETED handler"), | |
| webhookPath: "api/webhooks/app-deleted", | |
| hooks: { | |
| onEvent: console.log, | |
| onAuthDataDeleted: console.log, | |
| onAuthDataDeleteError: console.log, | |
| const logger = createLogger("APP_DELETED handler"); | |
| const { handler, getWebhookManifest } = createAppDeletedHandler({ | |
| apl: saleorApp.apl, | |
| logger, | |
| webhookPath: "api/webhooks/app-deleted", | |
| hooks: { | |
| onEvent: () => logger.info("App deleted webhook event received"), | |
| onAuthDataDeleted: () => logger.info("Auth data deleted after app removal"), | |
| onAuthDataDeleteError: () => logger.error("Failed to delete auth data after app removal"), |
| }); | ||
|
|
||
| export { getWebhookManifest }; | ||
|
|
There was a problem hiding this comment.
This API route is missing export const config = { api: { bodyParser: false } }, which all other Saleor webhook routes in this app export. Without disabling Next.js body parsing, webhook signature verification can break because the raw request body is no longer available.
| export const config = { | |
| api: { | |
| bodyParser: false, | |
| }, | |
| }; |
| const webhook = new SaleorAsyncWebhook({ | ||
| apl, | ||
| name: "APP_DELETED", | ||
| query: AppDeletedDocument, | ||
| event: "APP_DELETED", | ||
| isActive: true, | ||
| webhookPath, |
There was a problem hiding this comment.
The webhook name is set to the all-caps event identifier ("APP_DELETED"). In other apps, webhook name is a human-readable label (e.g. "OrderCancelled", "Customer Created") and shows up in Saleor’s webhook list. Consider changing this to a descriptive name like "App deleted" / "AppDeleted" while keeping event: "APP_DELETED".
| return new Response('"Failed to clean up auth data."', { status: 500 }); | ||
| } | ||
| } catch (e) { | ||
| logger.error("Failed to execute APP_DELETED event", { error: e }); | ||
|
|
||
| return new Response('"Failed to clean up auth data."', { status: 500 }); |
There was a problem hiding this comment.
The error responses return a body that includes extra quote characters ('"Failed to clean up auth data."'). This will send the quotes to the client and looks accidental; return a plain string (or JSON) without embedded quotes.
| return new Response('"Failed to clean up auth data."', { status: 500 }); | |
| } | |
| } catch (e) { | |
| logger.error("Failed to execute APP_DELETED event", { error: e }); | |
| return new Response('"Failed to clean up auth data."', { status: 500 }); | |
| return new Response("Failed to clean up auth data.", { status: 500 }); | |
| } | |
| } catch (e) { | |
| logger.error("Failed to execute APP_DELETED event", { error: e }); | |
| return new Response("Failed to clean up auth data.", { status: 500 }); |
| const [saleorApiUrl] = LogsTable.decomposePrimaryKey(log.id); | ||
|
|
||
| return args.saleorApiUrl !== saleorApiUrl; |
There was a problem hiding this comment.
LogsRepositoryMemory.pruneAllLogs() calls LogsTable.decomposePrimaryKey(log.id), but ClientLog.id is an ULID (see LogsTransformer mapping id: entity.ulid). Splitting the ULID by # won’t yield saleorApiUrl, so this prune will never remove any logs.
| const [saleorApiUrl] = LogsTable.decomposePrimaryKey(log.id); | |
| return args.saleorApiUrl !== saleorApiUrl; | |
| return args.saleorApiUrl !== log.saleorApiUrl; |
|
|
||
| async pruneAllLogs(args: { | ||
| saleorApiUrl: string; | ||
| appId: string; |
There was a problem hiding this comment.
LogsRepositoryMemory.pruneAllLogs() requires { saleorApiUrl, appId }, but ILogsRepository.pruneAllLogs() is declared as { saleorApiUrl }. Even if this currently compiles, it’s inconsistent and can break callers that use the interface type; align the method signature with the interface (and drop appId if it’s not needed).
| appId: string; |
| do { | ||
| const scanResult = await ResultAsync.fromPromise( | ||
| this.logsTable | ||
| .build(ScanCommand) | ||
| .entities(this.logByDateEntity, this.logsByCheckoutOrOrderId) | ||
| .options({ | ||
| exclusiveStartKey: lastEvaluatedKey, | ||
| showEntityAttr: true, | ||
| filters: { | ||
| LOG_BY_DATE: { attr: "PK", beginsWith: pkPrefix }, | ||
| LOG_BY_CHECKOUT_OR_ORDER_ID: { attr: "PK", beginsWith: pkPrefix }, | ||
| }, | ||
| }) | ||
| .send(), |
There was a problem hiding this comment.
pruneAllLogs() performs a full table Scan and filters by PK prefix. For a logs table that can grow large, this can be slow/expensive and may exceed webhook execution time limits during app deletion. Consider an access pattern that avoids scanning (e.g., a GSI partitioned by saleorApiUrl, or storing logs under a dedicated PK for saleorApiUrl so it can be queried and deleted in batches).
| const repo = new LogsRepositoryDynamodb({ | ||
| logsTable, | ||
| logByDateEntity, | ||
| logByCheckoutOrOrderId, | ||
| }); | ||
|
|
||
| await repo.pruneAllLogs({ saleorApiUrl: authData.saleorApiUrl }); | ||
| }, |
There was a problem hiding this comment.
The pruneAllLogs() call returns a Result, but it’s awaited and ignored. If pruning fails, it will be silently skipped (and because it doesn’t throw, the shared handler will continue). Handle the Result explicitly (log on error or convert to a thrown error if you want the webhook to fail).
| "saleor-app-klaviyo": minor | ||
| --- | ||
|
|
||
| App now will listen on APP_DELETED webhook and clean up it's Auth Data once removed |
There was a problem hiding this comment.
Correct grammar in the changeset text: “clean up it's Auth Data” should use the possessive “its”.
| App now will listen on APP_DELETED webhook and clean up it's Auth Data once removed | |
| App now will listen on APP_DELETED webhook and clean up its Auth Data once removed |
| const requests = chunk.map((item) => | ||
| item.entity === "LOG_BY_DATE" | ||
| ? this.logByDateEntity | ||
| .build(BatchDeleteRequest) | ||
| .key({ PK: item.PK, ulid: item.ulid, date: item.date }) | ||
| : this.logsByCheckoutOrOrderId.build(BatchDeleteRequest).key({ | ||
| PK: item.PK, | ||
| ulid: item.ulid, | ||
| date: item.date, | ||
| checkoutOrOrderId: item.checkoutOrOrderId, | ||
| }), |
There was a problem hiding this comment.
pruneAllLogs() relies on item.entity to detect which entity a scanned item belongs to. In this table the entity attribute is _et (see LogsTable generic and existing tests asserting _et: "LOG_BY_DATE"/"LOG_BY_CHECKOUT_OR_ORDER_ID"), so this check will likely always fail and the wrong delete key shape will be built.
| import { AppDeletedDocument } from "../generated/graphql"; | ||
|
|
||
| type Params = { | ||
| apl: APL; | ||
| webhookPath: string; | ||
| logger: Logger; | ||
| hooks?: { | ||
| onEvent?: (ctx: WebhookContext<unknown>) => Promise<void>; |
There was a problem hiding this comment.
The hooks.onEvent callback is typed as WebhookContext<unknown>, which loses payload typing for consumers. Since this handler is tied to AppDeletedDocument, it can expose a concrete context type (e.g. WebhookContext<AppDeletedSubscription>) to keep downstream hooks type-safe.
| import { AppDeletedDocument } from "../generated/graphql"; | |
| type Params = { | |
| apl: APL; | |
| webhookPath: string; | |
| logger: Logger; | |
| hooks?: { | |
| onEvent?: (ctx: WebhookContext<unknown>) => Promise<void>; | |
| import { AppDeletedDocument, type AppDeletedSubscription } from "../generated/graphql"; | |
| type Params = { | |
| apl: APL; | |
| webhookPath: string; | |
| logger: Logger; | |
| hooks?: { | |
| onEvent?: (ctx: WebhookContext<AppDeletedSubscription>) => Promise<void>; |
| export const createAppDeletedHandler = ({ apl, webhookPath, hooks = {}, logger }: Params) => { | ||
| const webhook = new SaleorAsyncWebhook({ | ||
| apl, | ||
| name: "APP_DELETED", | ||
| query: AppDeletedDocument, | ||
| event: "APP_DELETED", | ||
| isActive: true, | ||
| webhookPath, | ||
| }); | ||
|
|
||
| const handler = webhook.createHandler(async (_req, ctx) => { | ||
| try { | ||
| logger.info("APP_DELETED event received. Auth Data will be removed"); | ||
|
|
||
| await hooks.onEvent?.(ctx); | ||
|
|
||
| try { | ||
| await apl.delete(ctx.authData.saleorApiUrl); | ||
|
|
||
| await hooks.onAuthDataDeleted?.(); | ||
|
|
||
| return new Response("ok", { status: 200 }); | ||
| } catch (e) { | ||
| logger.error("Error deleting auth data on APP_DELETED", { error: e }); | ||
|
|
||
| await hooks.onAuthDataDeleteError?.(e as Error); | ||
|
|
||
| return new Response('"Failed to clean up auth data."', { status: 500 }); | ||
| } | ||
| } catch (e) { | ||
| logger.error("Failed to execute APP_DELETED event", { error: e }); | ||
|
|
||
| return new Response('"Failed to clean up auth data."', { status: 500 }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This new shared handler introduces non-trivial behavior (invoking hooks, deleting APL auth data, and mapping failures to HTTP responses) but there are no unit tests covering success/failure paths. Add tests for: (1) calling apl.delete with ctx.authData.saleorApiUrl, (2) hook invocation order, and (3) 500 responses + onAuthDataDeleteError on deletion failure.
| async pruneAllLogs(args: { | ||
| saleorApiUrl: string; | ||
| appId: string; | ||
| }): Promise<Result<undefined, unknown>> { | ||
| this.logs = this.logs.filter((l) => { | ||
| const log = l.getValue(); | ||
| const [saleorApiUrl] = LogsTable.decomposePrimaryKey(log.id); | ||
|
|
||
| return args.saleorApiUrl !== saleorApiUrl; | ||
| }); |
There was a problem hiding this comment.
LogsRepositoryMemory.pruneAllLogs has a signature that doesn't match ILogsRepository (it requires appId, while the interface does not), which will break TypeScript compilation. Also, the pruning logic splits log.id using LogsTable.decomposePrimaryKey, but ClientLog.id is just a ULID (see LogsTransformer mapping), so this filter will never match and logs won't be pruned. Align the method signature with the interface and store enough context in the memory repo (e.g. persist PK / saleorApiUrl alongside the log) to make pruning work.
| const repo = new LogsRepositoryDynamodb({ | ||
| logsTable, | ||
| logByDateEntity, | ||
| logByCheckoutOrOrderId, | ||
| }); | ||
|
|
||
| await repo.pruneAllLogs({ saleorApiUrl: authData.saleorApiUrl }); | ||
| }, |
There was a problem hiding this comment.
repo.pruneAllLogs returns a Result, but the returned value is ignored here. That means pruning failures will be silently swallowed and the webhook will continue (and still delete APL auth data), making cleanup unreliable and hard to diagnose. Handle the Result explicitly (log and/or throw on Err) so failures are observable and can fail the webhook when appropriate.
| async pruneAllLogs({ | ||
| saleorApiUrl, | ||
| }: { | ||
| saleorApiUrl: string; | ||
| }): Promise< | ||
| Result< | ||
| undefined, | ||
| | InstanceType<typeof LogsRepositoryDynamodb.LogsFetchError> | ||
| | InstanceType<typeof LogsRepositoryDynamodb.WriteLogError> | ||
| | InstanceType<typeof LogsRepositoryDynamodb.UnprocessedItemsError> | ||
| > | ||
| > { | ||
| this.logger.debug("Starting pruning logs for saleorApiUrl", { saleorApiUrl }); | ||
|
|
||
| const pkPrefix = `${saleorApiUrl}#`; | ||
|
|
||
| let lastEvaluatedKey: LastEvaluatedKey; | ||
| let deletedCount = 0; | ||
|
|
||
| do { |
There was a problem hiding this comment.
pruneAllLogs is a new public method on ILogsRepository / LogsRepositoryDynamodb, and this module already has unit tests (logs-repository.test.ts), but there are no tests validating pruning behavior (e.g. scan pagination, chunking into 25-item batch deletes, and handling of UnprocessedItems). Add unit tests (with DynamoDB toolbox client mocked) to cover at least the happy path and the UnprocessedItems error path.
Scope of the PR
Related issues
Checklist