Conversation
kit→Play push wrote a single hardcoded regionCode "US" into each
product's regionalPricingAndAvailabilityConfigs, so products created
through iapkit_sync_products were unbuyable — and unqueryable — outside
the United States, while the sync reported {pushed: n, failures: []}.
The modern one-time-product API has no autoConvertMissingPrices
equivalent, so the fix calls monetization.convertRegionPrices and writes
every region it returns, plus newRegionsConfig / otherRegionsConfig so
markets Play launches later stay covered.
- Read the product's existing purchase option before patching. The
updateMask "purchaseOptions" makes Play REPLACE the repeated field,
so pushing to a product with 173 regions previously deleted 172 of
them. Regions Play didn't reprice are now preserved verbatim, and a
region the operator withdrew in Play Console keeps its availability
through a price edit. This destructive round-trip was reachable on
live products, since pull prefers the US price and stores it as USD —
exactly the value the old currency guard let through.
- Subscriptions get the same treatment on create (base plans were
US-only too).
- Legacy inappproducts fallback passes autoConvertMissingPrices=true.
- Drop the blanket non-USD rejection: conversion returns each region's
own local currency, so any base currency now works. The USD-only
constraint survives just on the degraded path, where a non-USD amount
can't legally go to the US fallback region.
- When conversion is unavailable the push still lands, but reports a
regional_pricing_incomplete manual action instead of a clean success.
AndroidSyncResult gains manualActions; the schema, job worker, and
dashboard banner were already platform-agnostic.
Fixes #288
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAndroid product sync now publishes converted regional prices for one-time products and subscriptions. Updates preserve existing regions and availability. Conversion failures use validated USD fallbacks and add manual actions to sync results. ChangesAndroid regional pricing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SyncProducts
participant AndroidProductSync
participant GooglePlayAndroidPublisher
SyncProducts->>AndroidProductSync: push Android products
AndroidProductSync->>GooglePlayAndroidPublisher: read existing configuration
AndroidProductSync->>GooglePlayAndroidPublisher: write regional pricing
GooglePlayAndroidPublisher-->>AndroidProductSync: update result
AndroidProductSync-->>SyncProducts: pushed results and manualActions
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/kit/convex/products/play.test.ts (1)
362-418: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for an empty converted map.
The suite covers a successful conversion and an unavailable conversion. It does not cover
convertedRegionPrices: {}or entries without aprice. That input currently produces a fallback-only config with no manual action. Add one case per builder once the gating fix on play.ts lands.🤖 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/kit/convex/products/play.test.ts` around lines 362 - 418, Add test coverage for empty convertedRegionPrices and converted entries missing price in the buildSubscriptionRegionalConfigs suite, with one case per builder after the play.ts gating fix. Verify each input produces the expected fallback-only configuration and requires the appropriate manual action.
🤖 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 `@packages/kit/convex/products/play.ts`:
- Around line 1330-1343: The manual-action gates incorrectly treat any present
convertedRegionPrices object as successful conversion. In
packages/kit/convex/products/play.ts lines 1330-1343 and 808-816, define or
reuse one shared predicate that checks whether convertedRegionPrices contains at
least one entry with a price, then use it for both the converted and
subscriptionConverted success checks.
- Around line 1069-1129: The docstring for buildRegionalPricingConfigs promises
create/update asymmetry that the implementation does not enforce. Either pass
options.allowCreate into buildRegionalPricingConfigs and only add newly
converted regions during creation, or revise the docstring to describe the
current behavior; keep preservation of existing NO_LONGER_AVAILABLE regions
unchanged.
- Around line 794-816: Update the subscription pricing flow around
buildSubscriptionRegionalConfigs so dry-run mode does not call it with an
undefined conversion result. Compute subscriptionRegionalConfigs only on the
non-dry-run write path, while preserving the existing regional pricing and
manual-action behavior for real pushes.
---
Nitpick comments:
In `@packages/kit/convex/products/play.test.ts`:
- Around line 362-418: Add test coverage for empty convertedRegionPrices and
converted entries missing price in the buildSubscriptionRegionalConfigs suite,
with one case per builder after the play.ts gating fix. Verify each input
produces the expected fallback-only configuration and requires the appropriate
manual action.
🪄 Autofix
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 Plus
Run ID: 2ec5a5d8-0fa6-438a-8a25-6f2930d46f4f
📒 Files selected for processing (2)
packages/kit/convex/products/play.test.tspackages/kit/convex/products/play.ts
Comment on lines
+794
to
+816
| const subscriptionConverted = dryRun | ||
| ? undefined | ||
| : await convertAndroidRegionPrices( | ||
| androidpublisher, | ||
| packageName, | ||
| subscriptionBasePrice, | ||
| ); | ||
| const subscriptionRegionalConfigs = buildSubscriptionRegionalConfigs( | ||
| subscriptionConverted, | ||
| subscriptionBasePrice, | ||
| row.productId, | ||
| ); | ||
| const subscriptionOtherRegions = | ||
| subscriptionConverted?.convertedOtherRegionsPrice; | ||
| if (!dryRun && !subscriptionConverted?.convertedRegionPrices) { | ||
| manualActions.push({ | ||
| productId: row.productId, | ||
| code: "regional_pricing_incomplete", | ||
| message: | ||
| `Play could not convert ${row.currency} ${(row.priceAmountMicros / 1_000_000).toFixed(2)} into regional prices, so base plan "${basePlanId}" of "${row.productId}" ` + | ||
| `is available in ${subscriptionRegionalConfigs.length} region(s) only. Set the remaining regions in Play Console → the subscription's base plan → Set prices.`, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Dry-run fails for non-USD subscriptions.
In dry-run, subscriptionConverted is always undefined. buildSubscriptionRegionalConfigs then takes the fallback branch and assertUsdFallbackRegion throws for any non-USD row.currency. The per-row try/catch converts this into a sync failure, so the preview reports a failure for a row that the real push would publish successfully through conversion.
subscriptionRegionalConfigs is also unused in the dry-run branch. Compute it only on the write path.
🐛 Proposed fix
- const subscriptionRegionalConfigs = buildSubscriptionRegionalConfigs(
- subscriptionConverted,
- subscriptionBasePrice,
- row.productId,
- );
+ const subscriptionRegionalConfigs = dryRun
+ ? []
+ : buildSubscriptionRegionalConfigs(
+ subscriptionConverted,
+ subscriptionBasePrice,
+ row.productId,
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const subscriptionConverted = dryRun | |
| ? undefined | |
| : await convertAndroidRegionPrices( | |
| androidpublisher, | |
| packageName, | |
| subscriptionBasePrice, | |
| ); | |
| const subscriptionRegionalConfigs = buildSubscriptionRegionalConfigs( | |
| subscriptionConverted, | |
| subscriptionBasePrice, | |
| row.productId, | |
| ); | |
| const subscriptionOtherRegions = | |
| subscriptionConverted?.convertedOtherRegionsPrice; | |
| if (!dryRun && !subscriptionConverted?.convertedRegionPrices) { | |
| manualActions.push({ | |
| productId: row.productId, | |
| code: "regional_pricing_incomplete", | |
| message: | |
| `Play could not convert ${row.currency} ${(row.priceAmountMicros / 1_000_000).toFixed(2)} into regional prices, so base plan "${basePlanId}" of "${row.productId}" ` + | |
| `is available in ${subscriptionRegionalConfigs.length} region(s) only. Set the remaining regions in Play Console → the subscription's base plan → Set prices.`, | |
| }); | |
| } | |
| const subscriptionConverted = dryRun | |
| ? undefined | |
| : await convertAndroidRegionPrices( | |
| androidpublisher, | |
| packageName, | |
| subscriptionBasePrice, | |
| ); | |
| const subscriptionRegionalConfigs = dryRun | |
| ? [] | |
| : buildSubscriptionRegionalConfigs( | |
| subscriptionConverted, | |
| subscriptionBasePrice, | |
| row.productId, | |
| ); | |
| const subscriptionOtherRegions = | |
| subscriptionConverted?.convertedOtherRegionsPrice; | |
| if (!dryRun && !subscriptionConverted?.convertedRegionPrices) { | |
| manualActions.push({ | |
| productId: row.productId, | |
| code: "regional_pricing_incomplete", | |
| message: | |
| `Play could not convert ${row.currency} ${(row.priceAmountMicros / 1_000_000).toFixed(2)} into regional prices, so base plan "${basePlanId}" of "${row.productId}" ` + | |
| `is available in ${subscriptionRegionalConfigs.length} region(s) only. Set the remaining regions in Play Console → the subscription's base plan → Set prices.`, | |
| }); | |
| } |
🤖 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/kit/convex/products/play.ts` around lines 794 - 816, Update the
subscription pricing flow around buildSubscriptionRegionalConfigs so dry-run
mode does not call it with an undefined conversion result. Compute
subscriptionRegionalConfigs only on the non-dry-run write path, while preserving
the existing regional pricing and manual-action behavior for real pushes.
Comment on lines
+1069
to
+1129
| /** | ||
| * Builds the regional pricing rows for a purchase option. | ||
| * | ||
| * `existingByRegion` carries the product's current configs on an update: | ||
| * a region Play already knows about keeps its own availability so a | ||
| * price refresh can never revoke a market, and regions Play returned | ||
| * prices for but the product doesn't have yet are only added on create. | ||
| * That asymmetry is deliberate — an operator who deliberately withdrew a | ||
| * region in Play Console must not have it silently reinstated by a | ||
| * routine price edit. | ||
| */ | ||
| function buildRegionalPricingConfigs( | ||
| converted: androidpublisher_v3.Schema$ConvertRegionPricesResponse | undefined, | ||
| basePrice: androidpublisher_v3.Schema$Money, | ||
| productId: string, | ||
| existingByRegion: Map< | ||
| string, | ||
| androidpublisher_v3.Schema$OneTimeProductPurchaseOptionRegionalPricingAndAvailabilityConfig | ||
| >, | ||
| ): androidpublisher_v3.Schema$OneTimeProductPurchaseOptionRegionalPricingAndAvailabilityConfig[] { | ||
| const configs = new Map< | ||
| string, | ||
| androidpublisher_v3.Schema$OneTimeProductPurchaseOptionRegionalPricingAndAvailabilityConfig | ||
| >(); | ||
|
|
||
| for (const [regionCode, regionPrice] of Object.entries( | ||
| converted?.convertedRegionPrices ?? {}, | ||
| )) { | ||
| if (!regionPrice.price) continue; | ||
| const existing = existingByRegion.get(regionCode); | ||
| configs.set(regionCode, { | ||
| regionCode, | ||
| // Play rejects a config that pairs a region with a currency that | ||
| // isn't its own, so the converted Money is the only safe price | ||
| // here — never the operator's base-currency amount. | ||
| price: regionPrice.price, | ||
| availability: existing?.availability ?? "AVAILABLE", | ||
| }); | ||
| } | ||
|
|
||
| // Regions Play didn't return a conversion for (or the whole set when | ||
| // conversion failed) keep whatever they already had, so an update | ||
| // never drops a market from the product. | ||
| for (const [regionCode, existing] of existingByRegion) { | ||
| if (configs.has(regionCode)) continue; | ||
| configs.set(regionCode, existing); | ||
| } | ||
|
|
||
| if (configs.size === 0) { | ||
| // Nothing to preserve and no conversion — fall back to the base | ||
| // region so the product is at least purchasable somewhere. The | ||
| // caller reports this as a manual action rather than a silent | ||
| // success. | ||
| configs.set(assertUsdFallbackRegion(basePrice, productId), { | ||
| regionCode: "US", | ||
| availability: "AVAILABLE", | ||
| price: basePrice, | ||
| }); | ||
| } | ||
|
|
||
| return Array.from(configs.values()); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The docstring promises a create/update asymmetry that the code does not implement.
The docstring states that regions Play converted but the product does not have yet "are only added on create". buildRegionalPricingConfigs receives no create/update flag, so every converted region is written on updates too. A region the operator deleted from the purchase option in Play Console is therefore re-added as AVAILABLE by a routine price edit. Regions marked NO_LONGER_AVAILABLE are preserved correctly, so only the delete case diverges.
Choose one behaviour and align both: pass options.allowCreate into the builder, or correct the docstring.
🤖 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/kit/convex/products/play.ts` around lines 1069 - 1129, The docstring
for buildRegionalPricingConfigs promises create/update asymmetry that the
implementation does not enforce. Either pass options.allowCreate into
buildRegionalPricingConfigs and only add newly converted regions during
creation, or revise the docstring to describe the current behavior; keep
preservation of existing NO_LONGER_AVAILABLE regions unchanged.
Comment on lines
+1330
to
+1343
| if (converted?.convertedRegionPrices) return {}; | ||
|
|
||
| // Conversion failed. The product still went out — but only for the | ||
| // regions we could account for, so say so instead of reporting a | ||
| // clean success the operator would read as "available everywhere". | ||
| return { | ||
| manualAction: { | ||
| productId: args.productId, | ||
| code: "regional_pricing_incomplete", | ||
| message: | ||
| `Play could not convert ${args.currency} ${(args.priceAmountMicros / 1_000_000).toFixed(2)} into regional prices, so "${args.productId}" ` + | ||
| `is available in ${regionalConfigs.length} region(s) only. Set the remaining regions in Play Console → the product's purchase option → Set prices.`, | ||
| }, | ||
| }; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Manual-action gating tests object presence instead of converted region count. Both sites treat a truthy convertedRegionPrices as proof that conversion succeeded. An empty map, or entries without a price, passes that test while the product ships with only the fallback or preserved regions. The sync then reports clean success, which issue #288 requires it not to do. Derive one shared predicate from the converted entries that carry a price, and gate both manual actions on it.
packages/kit/convex/products/play.ts#L1330-L1343: replaceif (converted?.convertedRegionPrices) return {}with a check that at least one converted region supplied aprice.packages/kit/convex/products/play.ts#L808-L816: replace!subscriptionConverted?.convertedRegionPriceswith the same predicate applied tosubscriptionConverted.
📍 Affects 1 file
packages/kit/convex/products/play.ts#L1330-L1343(this comment)packages/kit/convex/products/play.ts#L808-L816
🤖 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/kit/convex/products/play.ts` around lines 1330 - 1343, The
manual-action gates incorrectly treat any present convertedRegionPrices object
as successful conversion. In packages/kit/convex/products/play.ts lines
1330-1343 and 808-816, define or reuse one shared predicate that checks whether
convertedRegionPrices contains at least one entry with a price, then use it for
both the converted and subscriptionConverted success checks.
Member
Author
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
Fixes #288 —
iapkit_sync_productswith{platform: "Android", direction: "push"}created Google Play products available in the United States only, while reporting{"pushed": 2, "failures": []}.Root cause is literal:
buildAndroidOneTimeProductwrote one hardcoded{regionCode: "US", availability: "AVAILABLE"}entry intoregionalPricingAndAvailabilityConfigs. Subscription base plans had the same hardcoding.The bigger problem the issue didn't mention
monetization.onetimeproducts.patchis called withupdateMask: "listings,purchaseOptions". Under protobuf FieldMask semantics a masked repeated field is replaced, so pushing to a product that already had 173 regions deleted 172 of them.That path was reachable on live products, not just new ones: the pull direction explicitly prefers the
regionCode === "US"price candidate, so a worldwide product lands in kit ascurrency: "USD"— precisely the value that satisfied the oldcurrency !== "USD"guard. The guard blocked the harmless case and admitted the destructive one.Fix
The modern one-time-product API has no
autoConvertMissingPricesequivalent (confirmed against the live discovery document), so:monetization.convertRegionPriceswith the base price and publish each returned region with its own local currency, plusnewRegionsConfig(one-time) /otherRegionsConfig(subscriptions) so markets Play launches later stay covered.NO_LONGER_AVAILABLEthrough a price edit. The read runs on the create path too, sinceallowMissingupserts and a retried "create" can land on an existing product.inappproductsinsert/patch now passautoConvertMissingPrices: true(one query param, the legacy-path half of the same bug).regional_pricing_incompletemanual action.AndroidSyncResultgainsmanualActions; theproductSyncJobsschema,markJobSucceeded, and the dashboard banner were already platform-agnostic and simply were never fed from the Android side.Tests
packages/kit: 921 pass (was 913). New coverage inconvex/products/play.test.ts:newRegionsConfigVerification after merge
This writes live Play billing config, so CI can't be the last word. On a test app: push a product, confirm Play Console shows the full region count (not
United States); then edit its price and re-push, confirming the region count is unchanged.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes