Improve discount creation error handling across Stripe and Shopify - #4173
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughDiscount provider errors are standardized across Stripe and Shopify with explicit codes, recoverability, and retry classification. Discount creation, disable flows, partner actions, and cron routes now consume the typed errors and renamed integration assertion. ChangesDiscount provider error handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CronRoute
participant DiscountProvider
participant ExternalProvider
CronRoute->>DiscountProvider: create or disable discount code
DiscountProvider->>ExternalProvider: perform provider operation
ExternalProvider-->>DiscountProvider: success or provider failure
DiscountProvider-->>CronRoute: typed DiscountProviderError
CronRoute->>CronRoute: classify recoverability and log or rethrow
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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.
🧹 Nitpick comments (3)
apps/web/lib/discounts/discount-provider-shopify.ts (1)
41-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded messages passed here are discarded by
resolveDiscountProviderMessage.These three
DiscountProviderErrorcalls pass the exact same literal strings thatresolveDiscountProviderMessage(indiscount-error.ts) already hardcodes and returns unconditionally forINTEGRATION_NOT_AVAILABLE/PERMISSIONS_REQUIRED. The passed message has no effect; it's duplicated text that can drift from the canonical copy.See consolidated comment for the fix location and all affected sites.
Also applies to: 56-61, 67-72
🤖 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/web/lib/discounts/discount-provider-shopify.ts` around lines 41 - 45, The literal messages in the three DiscountProviderError calls for INTEGRATION_NOT_AVAILABLE and PERMISSIONS_REQUIRED are discarded by resolveDiscountProviderMessage. Remove the duplicated message arguments at the affected call sites, relying on resolveDiscountProviderMessage in discount-error.ts as the canonical source while preserving the existing provider and error codes.apps/web/lib/discounts/discount-provider-stripe.ts (1)
22-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame discarded-message duplication as the Shopify provider.
These
INTEGRATION_NOT_AVAILABLEthrows pass a hardcoded string thatresolveDiscountProviderMessageignores and re-derives anyway.See consolidated comment.
Also applies to: 37-43
🤖 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/web/lib/discounts/discount-provider-stripe.ts` around lines 22 - 28, Remove the redundant hardcoded message arguments from the INTEGRATION_NOT_AVAILABLE DiscountProviderError throws in the Stripe provider, including both workspace connection checks, so resolveDiscountProviderMessage remains the single source for these messages while preserving the existing provider and error-code arguments.apps/web/lib/discounts/discount-error.ts (1)
21-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOne root cause:
resolveDiscountProviderMessagediscards the passedmessageforINTEGRATION_NOT_AVAILABLE/PERMISSIONS_REQUIRED, making every call-site message at these sites dead code. The most consequential instance is Stripe'sPERMISSIONS_REQUIREDpath, where the actual Stripe-provided reason is thrown away in favor of a generic string.
apps/web/lib/discounts/discount-error.ts#L21-L39: stop unconditionally overridingmessage; either drop the parameter for these two codes or merge the dynamic message into the standardized text.apps/web/lib/discounts/discount-provider-shopify.ts#L41-L72: once the root cause is fixed, these three hardcoded strings become redundant with the canonical copy indiscount-error.tsand can be simplified/removed.apps/web/lib/discounts/discount-provider-stripe.ts#L22-L43: same simplification applies to these twoINTEGRATION_NOT_AVAILABLEcall sites.apps/web/lib/discounts/discount-provider-stripe.ts#L219-L225: after the fix, this site should surface Stripe'serrorMessagealongside the standardized upgrade-required text instead of losing it.🤖 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/web/lib/discounts/discount-error.ts` around lines 21 - 39, Update resolveDiscountProviderMessage in apps/web/lib/discounts/discount-error.ts#L21-L39 to preserve or merge the supplied message for INTEGRATION_NOT_AVAILABLE and PERMISSIONS_REQUIRED, including Stripe’s specific reason with the standardized text. Simplify/remove the now-redundant hardcoded messages in apps/web/lib/discounts/discount-provider-shopify.ts#L41-L72 and apps/web/lib/discounts/discount-provider-stripe.ts#L22-L43 so callers rely on the canonical resolver copy. Ensure apps/web/lib/discounts/discount-provider-stripe.ts#L219-L225 passes through errorMessage and surfaces it alongside the upgrade-required message.
🤖 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.
Nitpick comments:
In `@apps/web/lib/discounts/discount-error.ts`:
- Around line 21-39: Update resolveDiscountProviderMessage in
apps/web/lib/discounts/discount-error.ts#L21-L39 to preserve or merge the
supplied message for INTEGRATION_NOT_AVAILABLE and PERMISSIONS_REQUIRED,
including Stripe’s specific reason with the standardized text. Simplify/remove
the now-redundant hardcoded messages in
apps/web/lib/discounts/discount-provider-shopify.ts#L41-L72 and
apps/web/lib/discounts/discount-provider-stripe.ts#L22-L43 so callers rely on
the canonical resolver copy. Ensure
apps/web/lib/discounts/discount-provider-stripe.ts#L219-L225 passes through
errorMessage and surfaces it alongside the upgrade-required message.
In `@apps/web/lib/discounts/discount-provider-shopify.ts`:
- Around line 41-45: The literal messages in the three DiscountProviderError
calls for INTEGRATION_NOT_AVAILABLE and PERMISSIONS_REQUIRED are discarded by
resolveDiscountProviderMessage. Remove the duplicated message arguments at the
affected call sites, relying on resolveDiscountProviderMessage in
discount-error.ts as the canonical source while preserving the existing provider
and error codes.
In `@apps/web/lib/discounts/discount-provider-stripe.ts`:
- Around line 22-28: Remove the redundant hardcoded message arguments from the
INTEGRATION_NOT_AVAILABLE DiscountProviderError throws in the Stripe provider,
including both workspace connection checks, so resolveDiscountProviderMessage
remains the single source for these messages while preserving the existing
provider and error-code arguments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa09ba23-5626-4407-89c5-7c6d66a3fda7
📒 Files selected for processing (9)
apps/web/app/(ee)/api/cron/discount-codes/create/queue-batches/route.tsapps/web/app/(ee)/api/cron/discount-codes/create/route.tsapps/web/app/(ee)/api/cron/discount-codes/disable/route.tsapps/web/app/(ee)/api/cron/groups/remap-discount-codes/route.tsapps/web/lib/actions/partners/create-discount.tsapps/web/lib/discounts/create-discount-code.tsapps/web/lib/discounts/discount-error.tsapps/web/lib/discounts/discount-provider-shopify.tsapps/web/lib/discounts/discount-provider-stripe.ts
Map unauthorized provider responses to AUTH_EXPIRED with reconnect CTAs, treat retry exhaustion as CREATE_FAILED, and stop remapping on non-recoverable integration failures.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/lib/discounts/discount-provider-stripe.ts (1)
230-236: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winProvide a fallback string to prevent passing
undefinedtoDiscountProviderError.Because
erroris typed asanyin these catch blocks, TypeScript doesn't catch thaterrorMessage(orerror.message) might evaluate toundefined. Since theDiscountProviderErrorconstructor expects a strongly typedstringfor itsmessageargument, passingundefinedcould cause runtime errors when the message is formatted downstream.Please add a fallback message, similar to how it is handled for
CREATE_FAILEDon line 249.
apps/web/lib/discounts/discount-provider-stripe.ts#L230-L236: Add a fallback string such aserrorMessage || "Stripe account is missing required permissions."apps/web/lib/discounts/discount-provider-stripe.ts#L310-L316: Add a fallback string such aserror.raw?.message || error.message || "Stripe account is missing required permissions."🤖 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/web/lib/discounts/discount-provider-stripe.ts` around lines 230 - 236, Ensure both Stripe permissions-error handling sites always pass a string message to DiscountProviderError: at apps/web/lib/discounts/discount-provider-stripe.ts lines 230-236, fall back from errorMessage to a descriptive permissions message; at lines 310-316, fall back from error.raw?.message and error.message to the same descriptive message. Preserve the existing PERMISSIONS_REQUIRED behavior.
🤖 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/web/ui/partners/constants.ts`:
- Around line 20-34: Update the ctaUrl values in SHOPIFY_RECONNECT_REQUIRED and
STRIPE_RECONNECT_REQUIRED to use the workspace integration settings paths for
Shopify and Stripe instead of marketplace URLs. Leave
SHOPIFY_APP_UPGRADE_REQUIRED unchanged.
---
Outside diff comments:
In `@apps/web/lib/discounts/discount-provider-stripe.ts`:
- Around line 230-236: Ensure both Stripe permissions-error handling sites
always pass a string message to DiscountProviderError: at
apps/web/lib/discounts/discount-provider-stripe.ts lines 230-236, fall back from
errorMessage to a descriptive permissions message; at lines 310-316, fall back
from error.raw?.message and error.message to the same descriptive message.
Preserve the existing PERMISSIONS_REQUIRED behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39b62de8-6813-4b2b-bea7-71c6209a5f6f
📒 Files selected for processing (5)
apps/web/app/(ee)/api/cron/groups/remap-discount-codes/route.tsapps/web/lib/discounts/discount-error.tsapps/web/lib/discounts/discount-provider-shopify.tsapps/web/lib/discounts/discount-provider-stripe.tsapps/web/ui/partners/constants.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/lib/discounts/discount-error.ts
|
@coderabbitai review please. Any breaking changes? |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Changed files ==='
git diff --name-status origin/main...HEAD
echo '=== Full PR diff ==='
git diff --find-renames --find-copies --unified=80 origin/main...HEAD -- \
'apps/web/lib/discounts/discount-error.ts' \
'apps/web/lib/discounts/discount-provider-shopify.ts' \
'apps/web/lib/discounts/discount-provider-stripe.ts' \
'apps/web/lib/discounts/create-discount-code.ts' \
'apps/web/app/(ee)/api/cron/discount-codes/create/route.ts' \
'apps/web/app/(ee)/api/cron/discount-codes/create/queue-batches/route.ts' \
'apps/web/app/(ee)/api/cron/discount-codes/disable/route.ts' \
'apps/web/app/(ee)/api/cron/groups/remap-discount-codes/route.ts' \
'apps/web/lib/actions/partners/create-discount.ts' \
'apps/web/ui/partners/constants.ts'
echo '=== Provider/error API references ==='
rg -n -C 4 \
'assertDiscountIntegration(?:Available)?\s*\(|assertDiscountIntegration(?:Available)?\b|DiscountIntegrationNotAvailableError|isDiscountIntegrationNotAvailableError|DiscountProviderError|isNonRecoverableDiscountError|isDiscountProviderError' \
apps/web
echo '=== Discount tests ==='
fd -t f . apps/web | rg '(discount|shopify|stripe).*(test|spec)\.[cm]?[jt]sx?$|(?:test|spec).*(discount|shopify|stripe)\.[cm]?[jt]sx?$' || true
echo '=== Type/interface declarations near provider objects ==='
rg -n -C 12 'discountProvider|stripeDiscountProvider|shopifyDiscountProvider|assertDiscountIntegration' \
apps/web/lib/discounts apps/web/lib/actions/partners/create-discount.tsLength of output: 102845 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== createDiscountCode call sites ==='
rg -n -C 12 '\bcreateDiscountCode\s*\(' apps/web --glob '*.ts' --glob '*.tsx'
echo '=== Shopify GraphQL error definitions and throw sites ==='
ast-grep outline apps/web/lib/integrations/shopify/admin-graphql.ts --items all || true
rg -n -C 10 'class ShopifyAdminGraphqlError|new ShopifyAdminGraphqlError|ShopifyAdminGraphqlError' \
apps/web/lib/integrations/shopify apps/web --glob '*.ts' --glob '*.tsx'
echo '=== Discount tests ==='
cat -n apps/web/tests/discounts/index.test.ts
echo '=== API error and action error serialization ==='
rg -n -C 8 'instanceof DubApiError|DubApiError|ERROR_MAP|error\.message' \
apps/web/lib/actions apps/web/app apps/web/ui/partners \
--glob '*.ts' --glob '*.tsx' | head -n 500
echo '=== Explicit code parameters around discount creation ==='
rg -n -C 10 '\bcode\s*[:=]|createDiscountCode\(' \
apps/web/lib/discounts apps/web/app --glob '*.ts' --glob '*.tsx'Length of output: 175035 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Shopify GraphQL helper ==='
cat -n apps/web/lib/integrations/shopify/admin-graphql.ts
echo '=== Exact createDiscountCode invocation contexts ==='
rg -n -C 18 '\bcreateDiscountCode\s*\(' \
apps/web/lib/discounts \
'apps/web/app/(ee)/api/cron' \
--glob '*.ts' --glob '*.tsx'
echo '=== Explicit `code` argument passed to local createDiscountCode ==='
rg -n -U -C 14 \
'createDiscountCode\s*\(\s*\{(?:(?!\}\s*\)).)*\bcode\s*:' \
apps/web --glob '*.ts' --glob '*.tsx' || true
echo '=== Discount tests ==='
cat -n apps/web/tests/discounts/index.test.ts
echo '=== UI consumer for ERROR_MAP ==='
rg -n -C 12 '\bERROR_MAP\b' apps/web --glob '*.ts' --glob '*.tsx'
echo '=== Provider test files and focused references ==='
fd -t f -a . apps/web | rg '(^|/)(discount-provider-(stripe|shopify)|discount-error|create-discount-code).*\.(test|spec)\.[cm]?[jt]sx?$' || trueLength of output: 29283
One compatibility note: the renamed assertion method is a breaking API change for any consumer importing The main gap is test coverage: the existing discount tests only cover discount retrieval, not the newly changed provider error mappings, duplicate retries, or disable flows. I’d recommend adding focused unit tests before relying on those paths heavily, but I did not find a release-blocking regression. ✅ Action performedReview finished.
|
|
@coderabbitai to confirm, this is backwards compatible right? |
|
Tip For best results, initiate chat on the files or code changes.
The one exception is an external consumer directly accessing: stripeDiscountProvider.assertDiscountIntegrationAvailable
shopifyDiscountProvider.assertDiscountIntegrationAvailableThose property names were renamed to |
3 participants
Summary by CodeRabbit