Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR introduces a webhook tunnel system: a Cloudflare Workers app (apps/wh) with D1/SQLite storage and Drizzle ORM, DB migrations for tunnels and deliveries, provider interface extensions for tunnel lifecycle and stale-signature handling, a Stripe tunnel implementation, a new ChangesWebhook Tunnel Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as paykit listen
participant CloudAPI
participant DB as D1 SQLite
participant Provider
participant Local as Local App
User->>CLI: start listen --url
CLI->>CloudAPI: POST /api/tunnels/ensure
CloudAPI->>DB: create/update tunnel
CLI->>Provider: ensureTunnelWebhook(url)
loop Poll
CLI->>CloudAPI: GET /api/tunnels/:id/pull
CloudAPI->>DB: select pullable deliveries
CloudAPI-->>CLI: deliveries
CLI->>Local: POST replay (sanitized headers)
Local-->>CLI: 200 / error
CLI->>CloudAPI: POST /api/deliveries/:id/ack or /fail
CloudAPI->>DB: update delivery timestamps
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (2)
packages/stripe/src/stripe-provider.ts (1)
959-969: 💤 Low valueMinor DRY:
displayNameextraction duplicated betweengetTunnelAccountandcheck()The three-part fallback chain
account.settings?.dashboard?.display_name || account.business_profile?.name || account.idis repeated identically at line 1016. Consider extracting agetDisplayName(account)helper.♻️ Suggested extraction
+function getStripeDisplayName(account: StripeSdk.Account): string { + return account.settings?.dashboard?.display_name || account.business_profile?.name || account.id; +} + // ...in getTunnelAccount: - const displayName = - account.settings?.dashboard?.display_name || account.business_profile?.name || account.id; + const displayName = getStripeDisplayName(account); // ...in check(): - const displayName = - account.settings?.dashboard?.display_name || account.business_profile?.name || account.id; + const displayName = getStripeDisplayName(account);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stripe/src/stripe-provider.ts` around lines 959 - 969, The display name fallback chain is duplicated in getTunnelAccount and check; extract a small helper (e.g., getDisplayName(account)) that returns account.settings?.dashboard?.display_name || account.business_profile?.name || account.id, then replace the inline expression in both getTunnelAccount and check with calls to getDisplayName(account) so the logic is centralized and DRY.packages/paykit/src/cli/commands/listen.ts (1)
147-204: ⚡ Quick win
printReadyBlockandprintEnableSummaryare near-duplicates.The bullet/label/account formatting (lines 156-167 and 185-196) is the same. Extract a shared
formatTunnelSummaryso future label/format changes only happen in one place.♻️ Suggested refactor sketch
function formatTunnelSummary(account: TunnelAccountSummary, webhookUrl: string, webhookSecret?: string): string { const bullet = picocolors.cyan("•"); const labelWidth = 16; const formatLabel = (label: string) => label + " ".repeat(labelWidth - label.length); const accountName = account.displayName ?? account.providerAccountId; const accountSummary = `${accountName} ${picocolors.dim(`(${formatEnvironment(account.environment)})`)}`; const reminder = webhookSecret ? `\n${" ".repeat(2 + labelWidth + 1)}${picocolors.dim("^ don't forget add to .env")}` : ""; return ( `${bullet} ${formatLabel("Stripe")} ${accountSummary}\n` + `${bullet} ${formatLabel("Webhook endpoint")} ${webhookUrl}\n` + `${bullet} ${formatLabel("Webhook secret")} ${webhookSecret ?? picocolors.dim("(existing secret hidden)")}${reminder}` ); }Then
printReadyBlockandprintEnableSummaryonly differ in their lead/trailer lines.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/paykit/src/cli/commands/listen.ts` around lines 147 - 204, Both printReadyBlock and printEnableSummary duplicate the same bullet/label/account formatting; extract that into a shared helper (e.g., formatTunnelSummary(account: TunnelAccountSummary, webhookUrl: string, webhookSecret?: string): string) that returns the three-line formatted summary including the optional reminder, then update printReadyBlock and printEnableSummary to call formatTunnelSummary and only prepend/append their differing lead/trailer strings (keep existing symbols picocolors, formatEnvironment, and labelWidth logic inside the helper so future label/format changes happen in one place).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/demo/package.json`:
- Line 19: The bun CLI invocation for the paykitjs script removed the custom
export condition and risks breaking module resolution for exports using the
paykit-source condition; either restore the flag by changing the paykitjs script
invocation to include --conditions=paykit-source when calling bun
../../packages/paykit/src/cli/index.ts, or if you intend to remove it, perform a
project-wide audit (check export maps in packages/paykit, packages/stripe,
packages/polar and the TypeScript custom condition registration) and run the
full build/test cycle to confirm no imports rely on paykit-source before
committing the removal.
In `@apps/wh/drizzle.config.ts`:
- Around line 8-31: The config eagerly calls resolveLocalSqliteFile() when
building dbCredentials causing failures for schema-only commands; change
dbCredentials in the export from a static object value to a lazily-evaluated
value (e.g., a function or getter) so resolveLocalSqliteFile() is only executed
when a real DB connection is required, and leave defineConfig,
resolveLocalSqliteFile, and the existing error behavior unchanged otherwise;
additionally, optionally short-circuit the call by checking a lightweight flag
(process.env or a Drizzle command indicator) before invoking
resolveLocalSqliteFile() so drizzle-kit generate can run without a local SQLite
file.
In `@apps/wh/package.json`:
- Line 8: The deploy script in package.json uses an unquoted env expansion
(--var PAYKIT_WEBHOOK_API_BASE_URL:$PAYKIT_WEBHOOK_API_BASE_URL) which can break
if the URL contains whitespace or IFS chars; update the "deploy" script so the
PAYKIT_WEBHOOK_API_BASE_URL expansion is quoted when passed to wrangler (i.e.,
quote the variable part in the --var argument of the deploy npm script) to
ensure correct tokenization and safe handling of values.
In `@apps/wh/src/index.ts`:
- Around line 136-164: pruneDeliveries currently issues a global deletion
(delete where delivery.receivedAt < cutoff) which scans all tunnels; restrict
that delete to the current tunnel by adding the tunnel filter (use
delivery.tunnelId / params.tunnelId) so the initial await
params.db.delete(delivery).where(lt(delivery.receivedAt, cutoff)) becomes a
tunnel-scoped delete, or alternatively move the global retention logic out of
pruneDeliveries into a scheduled/cron handler; update the function
pruneDeliveries and the initial delete call accordingly to reference
params.tunnelId (and keep the subsequent overflow-trimming logic unchanged).
- Around line 321-325: The clamp and Math.max calls are receiving NaN when query
params like "limit", "offset", and "retryWindowMs" are non-numeric; update the
parsing so you coerce invalid values to their defaults before applying
clamp/Math.max: parse the query value for "limit" and "offset" into a Number,
check isNaN(parsed) and if true use the default (30 for limit, 0 for offset)
then call clamp; for "retryWindowMs" parse and if isNaN(parsed) use 0 before
Math.max; similarly for includeFailedBeforeRaw parse to Number and set
includeFailedBefore to undefined when parsing yields NaN. Apply these changes
around the variables limit, offset, retryWindowMs, and includeFailedBefore to
prevent NaN from reaching DB/query code.
- Around line 87-99: getOwnedTunnel currently returns tunnels regardless of
status, so management endpoints (/welcome, /pull, /provider-webhook, delivery
ack/fail) continue processing disabled tunnels; update the logic so disabled
tunnels are rejected with HTTP 410. Fix by either adding a status filter to the
DB query in getOwnedTunnel (e.g., include eq(tunnel.status, 'active') or the
equivalent active status constant) or by checking the returned object's
tunnel.status in each caller and returning 410 when status === 'disabled';
reference getOwnedTunnel and tunnel.status when making the change so callers of
getOwnedTunnel (the /welcome, /pull, /provider-webhook handlers and delivery
ack/fail handlers) will not process disabled tunnels.
In `@packages/paykit/src/cli/commands/listen.ts`:
- Line 225: The error message thrown when a provider does not support listening
incorrectly says "paykitjs listen"; update the throw in the block that
constructs the error (the throw new Error(...) referencing provider.name) to use
the correct CLI command name "paykit listen" instead of "paykitjs listen" so the
message reads: Provider "<provider.name>" does not support paykit listen yet.
- Around line 523-547: The infinite listen loop currently lets exceptions from
pullDeliveries/processPendingDeliveries (and underlying requestCloud →
failDelivery/ackDelivery) bubble up and kill the process; wrap the body of the
for(;;) iteration (the call to pullDeliveries and subsequent
processPendingDeliveries and sleep) in a try/catch, log a warning/error via
devLogger including the caught error, and implement a bounded backoff before
continuing (e.g., increase a delay up to a max then reset after a successful
iteration); ensure the catch does not rethrow and that mode handling (mode ===
"replay" → "live") still happens only on successful runs so replay semantics
remain unchanged.
- Around line 576-578: The diff leaves a dead compute and no-op on
localWebhookUrl: remove the unused call to buildLocalWebhookUrl(localOrigin,
config.options.basePath ?? "/paykit") and the subsequent void localWebhookUrl;
(and keep devLogger.stop() as-is) so enable no longer computes and discards a
value; if you intended to validate the origin/basePath, instead call a named
validator or comment “// validate” and invoke buildLocalWebhookUrl in that
explicit validation path; references: localWebhookUrl,
buildLocalWebhookUrl(...), printEnableSummary, devLogger.stop(), enable.
In `@packages/paykit/src/cli/utils/device-token.ts`:
- Line 30: The device token file is created without restrictive permissions;
update the fs.writeFileSync call that writes DEVICE_CONFIG_PATH (where
deviceToken is serialized) to pass an options object that sets mode to 0o600 so
the file is created readable/writable only by the owner; locate the write in
device-token.ts (the fs.writeFileSync(...) that writes JSON.stringify({
deviceToken })) and add the { mode: 0o600 } option to the call.
- Around line 21-23: Wrap the JSON.parse call that reads DEVICE_CONFIG_PATH in a
try/catch so a malformed ~/.config/paykit.json won't crash the CLI: when
fs.existsSync(DEVICE_CONFIG_PATH) is true, try to parse into parsed
(Partial<DeviceConfig>), and on a SyntaxError (or any parse failure) catch it,
log or warn about invalid config, set parsed = {} (or otherwise treat as missing
config) so the subsequent typeof parsed.deviceToken checks continue safely;
ensure this uses the same symbols (DEVICE_CONFIG_PATH, parsed, DeviceConfig,
deviceToken) so downstream logic can create and persist a fresh token instead of
hard-failing.
In `@packages/paykit/src/webhook/webhook.api.ts`:
- Around line 12-14: The current shouldAllowStaleSignatures(headers) uses a
negated NODE_ENV check which treats an unset NODE_ENV as non-production and can
silently bypass signature verification; update the function to use an explicit
allow-list or opt-in flag instead—replace the body of shouldAllowStaleSignatures
to return true only when a dedicated opt-in env flag (e.g.
process.env.PAYKIT_ALLOW_STALE_SIGNATURES === "1") is set or when NODE_ENV is
explicitly one of a short allow-list like "development" or "test", and still
require headers.get("x-paykit-cloud-replay") === "1" in addition; ensure the new
check is used wherever shouldAllowStaleSignatures is called.
In `@packages/stripe/src/stripe-provider.ts`:
- Around line 983-985: The silent catch currently swallows all errors when
attempting to retrieve the stored webhook endpoint and causes creation of a new
endpoint (with a new webhookSecret) on any failure; change the catch in the
endpoint retrieval logic (the block that references options.webhookSecret,
webhookSecret and the stored endpoint ID lookup—likely in a function like
ensureWebhookEndpoint/getStoredWebhookEndpoint) to only swallow the "resource
not found" case and re-throw everything else: inspect the thrown error (e.g.,
err.statusCode === 404 or err.type === 'StripeInvalidRequestError' && err.code
=== 'resource_missing') and only continue to create a fresh endpoint for that
case, otherwise throw the error so network/auth/rate-limit/server errors are
propagated.
---
Nitpick comments:
In `@packages/paykit/src/cli/commands/listen.ts`:
- Around line 147-204: Both printReadyBlock and printEnableSummary duplicate the
same bullet/label/account formatting; extract that into a shared helper (e.g.,
formatTunnelSummary(account: TunnelAccountSummary, webhookUrl: string,
webhookSecret?: string): string) that returns the three-line formatted summary
including the optional reminder, then update printReadyBlock and
printEnableSummary to call formatTunnelSummary and only prepend/append their
differing lead/trailer strings (keep existing symbols picocolors,
formatEnvironment, and labelWidth logic inside the helper so future label/format
changes happen in one place).
In `@packages/stripe/src/stripe-provider.ts`:
- Around line 959-969: The display name fallback chain is duplicated in
getTunnelAccount and check; extract a small helper (e.g.,
getDisplayName(account)) that returns account.settings?.dashboard?.display_name
|| account.business_profile?.name || account.id, then replace the inline
expression in both getTunnelAccount and check with calls to
getDisplayName(account) so the logic is centralized and DRY.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2e0c369f-8391-4e8c-9071-f019802ecf77
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
.gitignoreapps/demo/package.jsonapps/wh/drizzle.config.tsapps/wh/migrations/0000_init.sqlapps/wh/migrations/0001_delivery-failure-tracking.sqlapps/wh/migrations/meta/0000_snapshot.jsonapps/wh/migrations/meta/0001_snapshot.jsonapps/wh/migrations/meta/_journal.jsonapps/wh/package.jsonapps/wh/src/db/schema.tsapps/wh/src/index.tsapps/wh/tsconfig.jsonapps/wh/wrangler.jsoncpackage.jsonpackages/paykit/package.jsonpackages/paykit/src/cli/commands/listen.tspackages/paykit/src/cli/index.tspackages/paykit/src/cli/utils/dev-logger.tspackages/paykit/src/cli/utils/device-token.tspackages/paykit/src/index.tspackages/paykit/src/providers/provider.tspackages/paykit/src/webhook/webhook.api.tspackages/paykit/src/webhook/webhook.service.tspackages/stripe/src/stripe-provider.tsscripts/worktree-setup.sh
💤 Files with no reviewable changes (1)
- package.json
| "start": "next start", | ||
| "tunnel": "set -a; [ -f .env ] && . ./.env; set +a; if [ -n \"$CF_TUNNEL_ID\" ]; then cloudflared tunnel --url http://localhost:3000 run \"$CF_TUNNEL_ID\"; fi", | ||
| "paykitjs": "tsx --conditions=paykit-source ../../packages/paykit/src/cli/index.ts", | ||
| "paykitjs": "bun ../../packages/paykit/src/cli/index.ts", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the paykit-source export condition in any package.json
rg '"paykit-source"' --type json -nRepository: getpaykit/paykit
Length of output: 649
Revisit removal of --conditions=paykit-source from CLI invocation
The paykit-source export condition is still actively used throughout the monorepo: it's defined in export maps across packages/paykit, packages/stripe, and packages/polar, and registered as a custom condition in the TypeScript configuration. Removing --conditions=paykit-source from the bun command may cause module resolution failures if the CLI or its dependencies import modules that specify paykit-source export conditions. Either restore the --conditions flag or confirm that none of the CLI's import chain relies on these exports.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/demo/package.json` at line 19, The bun CLI invocation for the paykitjs
script removed the custom export condition and risks breaking module resolution
for exports using the paykit-source condition; either restore the flag by
changing the paykitjs script invocation to include --conditions=paykit-source
when calling bun ../../packages/paykit/src/cli/index.ts, or if you intend to
remove it, perform a project-wide audit (check export maps in packages/paykit,
packages/stripe, packages/polar and the TypeScript custom condition
registration) and run the full build/test cycle to confirm no imports rely on
paykit-source before committing the removal.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Summary by CodeRabbit