fix: harden IAPKit sync, verification, and MCP sessions - #292
Merged
Merged
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>
A Google purchase verified moments after it completed could be rejected, and because the app then (correctly) refuses to acknowledge what kit would not verify, Google auto-voided it ~301s later as unacknowledged. The reported hypothesis — that kit treats acknowledgementState 0 as invalid — is not what happens: PENDING_ACKNOWLEDGMENT and READY_TO_CONSUME are both valid states, and an existing test covers the fresh-purchase shape. Three other mechanisms are real: - productsv2 / subscriptionsv2 are eventually consistent, so a token seconds old can 404 in both. 4xx is excluded from retryOnTransient, so that became a hard 400 on the very first attempt — exactly the t≈1s verify the reporter measured. Retry the product→subscription pair when neither catalog knows the token yet (~2s of backoff at worst). - The replay guard armed its 300s negative cooldown on ANY isValid:false, and that window almost exactly spans Google's ~301s void window: one blip made the purchase permanently unverifiable before it could be acknowledged. Restrict the cooldown to settled verdicts (INAUTHENTIC, CANCELED, EXPIRED); PENDING and UNKNOWN can legitimately change on retry. Replay-attack protection is unaffected — a revoked receipt still reports a terminal state. - productId was read from productLineItem[0] regardless of how many items the token carried, so a multi-item purchase could be compared against the wrong product and fall through applyExpectedProductId to INAUTHENTIC. Prefer the line item the caller asked about. Refs #289 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ions The hosted kit.openiap.dev/mcp endpoint keeps StreamableHTTP sessions in a per-process Map, so with more than one Fly machine behind the proxy a valid mcp-session-id was rejected with 400 "initialize first" whenever the request landed on a sibling machine (~65% of calls in the issue repro, consistent with 3-machine round-robin). - Prefix session ids with FLY_MACHINE_ID and answer requests for a foreign machine's session with a fly-replay header so Fly's proxy re-routes them to the owner. No shared store needed; the transport object holds live SSE state and cannot be serialized anyway. - Never replay twice (fly-replay-src guard) and never replay to self, so stale machine ids after a deploy cannot loop. - Answer 404 (-32001 Session not found) instead of 400 for a session this process genuinely cannot serve — the MCP spec makes clients transparently re-initialize on 404, so restarts now self-heal. - Add packages/mcp-server/** to deploy-kit.yml triggers: kit's Fly binary imports the MCP handler from source, so MCP fixes previously merged without ever deploying. Also run the MCP server's own vitest suite in the verify job — no CI ran it before. Fixes #287 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
hyochan
added
🐛 bug
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change adds localized product listings and sales regions across Kit, APIs, UI, store synchronization, and MCP tooling. Android verification selects expected products and retries fresh-token failures. Replay cooldowns classify stable rejections. Webhook processing becomes replayable. MCP sessions support machine-affine routing. ChangesProduct localization and store synchronization
Android purchase verification
Verification replay guard
Machine-affine MCP sessions
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 |
Two things land together because they touch the same push paths.
## Localized listings (Play + App Store Connect)
Every listing write hardcoded `en-US`, so a Korean buyer saw an English
product name even though pricing was already localized. Products gain an
optional `localizations: [{locale, title, description?}]`; `title` /
`description` remain the base en-US listing, so a row without any
publishes exactly what it published before.
- Play: one-time (modern + legacy), subscription create, and
subscription patch all expand to the full listing set. `updateMask`
REPLACES the listings array, so writes merge over what Play already
has — a locale added in Play Console is never deleted by a kit push.
- ASC: `upsertAscReviewLocalization` already accepted a locale and
always got the default; it now runs per locale, upserting rather than
replacing so an ASC-authored locale survives.
- Pull captures every locale instead of listing[0], round-tripping
through the same representation.
- Locale format, duplicates, blank titles, and store length caps are
validated in the mutation, so dashboard, REST, and MCP callers share
one rule set. Dashboard gains an add/remove language list; the MCP
`iapkit_create_product` tool and `POST /v1/products` accept the field.
## Self-review findings
Eight survived adversarial verification of the previous three commits;
fifteen were refuted.
- A dry run of a non-USD subscription failed with "Play could not
convert …" for a conversion never attempted — conversion was skipped
in dry-run, routing the empty config set into the USD fallback guard.
Conversion now happens only on the real write path.
- Conversion failure on an UPDATE preserved every existing region
verbatim, silently discarding the operator's price change. The new
amount now lands on regions already in the base currency, and the
manual action reports how many took it versus kept their old price.
- `buildRegionalPricingConfigs` documented an add-only-on-create rule
its code never implemented. The code is right — adding regions repairs
an already-broken US-only product, and withdrawn regions keep their
availability — so the comment now matches.
- Rebuilding the `buy` option dropped offerTags,
taxAndComplianceSettings, and an operator-set newRegionsConfig; they
are preserved, minus the output-only `state` Play rejects on write.
- Pull ranked prices US-first, so reading back a pushed product
overwrote an authored KRW/JPY row with its converted dollar amount and
the next push re-converted from that. Pull now prefers the currency
the row already carries.
- The fresh-token retry cost two Play calls per attempt, so four
attempts made a bogus-token probe eight calls and a ~2s hold. Three
attempts inside ~750ms still covers propagation.
- `fly-replay: instance=` has no fallback, so an unreachable owner
failed at the proxy and never produced the 404 the fix relies on.
`prefer_instance` degrades to "route anywhere", where the
already-replayed guard answers 404 and the client re-initializes.
Also: pull reports `product_type_assumed` when the modern Play API
forces it to guess NonConsumable (issue #289's silent-state mismatch),
and the pre-commit gate mirrors CI's new mcp-server step.
Tests: 913 → 950 (kit), 43 → 44 (mcp-server).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Member
Author
|
Pushed two follow-up commits since the description above. Localized store listings (new)Regional pricing was fixed by the first commit, but every listing write still hardcoded
Not covered: per-region price overrides (kit still stores one authored price and lets Play convert), and removing a localization in kit does not delete it upstream — same trade the regional configs make. Self-review findings applied23 candidate findings across four independent lenses; 8 confirmed, 15 refuted after adversarial verification.
Two more found by reading my own diff, outside the lenses:
Also widened Tests: 913 → 950 (kit), 43 → 44 (mcp-server). Full The device/console verification checklist in the description is unchanged and still applies — plus: confirm a pushed product shows the localized name in a non-English Play locale. |
hyochan
changed the title
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/kit/src/pages/auth/organization/project/products.tsx (1)
260-291: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReport upsert failures to the operator.
upsertProductnow rejects a malformed locale, a duplicate locale, a blank localization title, and over-long store text.onAdddoes not catch the rejection, and the caller invokes it asvoid onAdd(). The operator gets no message and the form keeps the values, so a typedko_KRlooks like a silent no-op.🐛 Proposed fix
- await upsert({ - projectId: project._id, - ... - state: "Draft", - }); - setDraft({ + try { + await upsert({ + projectId: project._id, + ... + state: "Draft", + }); + } catch (error) { + toast.error( + error instanceof Error ? error.message : String(error), + { duration: 8_000 }, + ); + return; + } + setDraft({🤖 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/src/pages/auth/organization/project/products.tsx` around lines 260 - 291, Update onAdd in the product form to catch failures from upsert, display the rejection message to the operator through the existing UI error mechanism, and return before resetting draft/localization state. Preserve the current reset behavior only after a successful upsert, including when the caller invokes onAdd with void.
🧹 Nitpick comments (1)
packages/kit/convex/products/play.ts (1)
440-456: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAggregate the assumed-type notice into one manual action.
This pushes one manual action per newly imported product. A first pull of a large catalog emits dozens of near-identical entries and can trip
manualActionsTruncated, which then hides the genuinely per-productregional_pricing_incompleteentries. Collect the product ids during the pull and emit one action that lists them.🤖 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 440 - 456, Aggregate newly imported product IDs instead of pushing one product_type_assumed action per item in the existingType === undefined branch. Emit a single manual action after the pull listing all affected IDs, while preserving the existing warning message and keeping per-product regional_pricing_incomplete actions unchanged.
🤖 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 @.github/workflows/deploy-kit.yml:
- Around line 19-22: Update the pull_request.paths configuration in
deploy-kit.yml to include .github/workflows/deploy-kit.yml, bun.lock, and
package.json alongside the existing package paths, so changes to these
dependency and workflow files trigger the verification job.
In `@packages/kit/convex/products/asc.ts`:
- Around line 2033-2048: The localization loop around listingRowsForProduct and
upsertAscReviewLocalization must isolate failures per locale. Add a per-call
error handler that records the failing locale in the failure productId and
allows subsequent locale submissions to continue, while retaining the outer
recordFailure only for the base listing.
In `@packages/kit/convex/products/localizations.ts`:
- Around line 36-40: Update LOCALE_PATTERN to accept supported four-letter
script subtags such as zh-Hans and zh-Hant, while retaining validation for
existing language-only and language-REGION forms and rejecting unsupported
exotic subtags.
In `@packages/kit/convex/products/mutation.ts`:
- Around line 635-640: Update the localizations schema field to allow null, then
change the explicit-localizations branch in the product mutation to persist null
when normalization returns undefined, while preserving existing localizations
when the argument is omitted. Update read paths feeding listingRowsForProduct to
coerce row.localizations with ?? undefined, following the existing
subscriptionGroupName pattern.
In `@packages/kit/convex/products/play.ts`:
- Line 1558: Use a single converted-region count derived from
convertedRegionPrices in packages/kit/convex/products/play.ts at lines
1558-1558, and use it both for the early conversion-success check and the
degraded arm in buildRegionalPricingConfigs so an empty map follows
base-currency repricing. At lines 876-887, apply the same count check before
adding regional_pricing_incomplete. Add play.test.ts coverage for
convertedRegionPrices: {} in both one-time and subscription paths.
In `@packages/kit/src/pages/auth/organization/project/products.tsx`:
- Around line 272-278: Update the localizations payload in the form submission
mapping so it is undefined when no authored localization rows remain after
filtering, while preserving the mapped array when at least one valid row exists.
Use the existing localizations value near the shown mapping and ensure the
mutation is not given an empty array for an untouched localization editor.
---
Outside diff comments:
In `@packages/kit/src/pages/auth/organization/project/products.tsx`:
- Around line 260-291: Update onAdd in the product form to catch failures from
upsert, display the rejection message to the operator through the existing UI
error mechanism, and return before resetting draft/localization state. Preserve
the current reset behavior only after a successful upsert, including when the
caller invokes onAdd with void.
---
Nitpick comments:
In `@packages/kit/convex/products/play.ts`:
- Around line 440-456: Aggregate newly imported product IDs instead of pushing
one product_type_assumed action per item in the existingType === undefined
branch. Emit a single manual action after the pull listing all affected IDs,
while preserving the existing warning message and keeping per-product
regional_pricing_incomplete actions unchanged.
🪄 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: 7b5d7dcb-d218-49ba-b585-b6f555b83aa2
📒 Files selected for processing (24)
.github/workflows/deploy-kit.yml.husky/pre-commitpackages/kit/convex/products/asc.tspackages/kit/convex/products/localizations.test.tspackages/kit/convex/products/localizations.tspackages/kit/convex/products/mutation.tspackages/kit/convex/products/play.test.tspackages/kit/convex/products/play.tspackages/kit/convex/products/sync.tspackages/kit/convex/purchases/android.test.tspackages/kit/convex/purchases/android.tspackages/kit/convex/schema.tspackages/kit/server/api/v1/products.tspackages/kit/server/api/v1/replay-guard.test.tspackages/kit/server/api/v1/replay-guard.tspackages/kit/src/pages/auth/organization/project/products.tsxpackages/mcp-server/src/http.tspackages/mcp-server/src/kit-client.tspackages/mcp-server/src/mcp.tspackages/mcp-server/src/session-routing.tspackages/mcp-server/src/web.tspackages/mcp-server/test/http.test.tspackages/mcp-server/test/session-routing.test.tspackages/mcp-server/test/web.test.ts
Six review comments, all valid.
- An empty `convertedRegionPrices` map read as success because `{}` is
truthy, so a product Play returned no conversions for shipped US-only
while the sync reported a clean push. Every "did conversion work"
decision now goes through `convertedRegionCount`, which also enables
the base-currency repricing arm for that case.
- Clearing the last localization was a no-op: the normalizer returns
undefined for an empty list, and Convex treats undefined in a patch as
"leave unchanged", so the stale locales kept getting republished. The
column is nullable now and an explicit empty array patches null. Draft
queries coerce it back to optional at the worker boundary, the way
subscriptionGroupName already does.
- The dashboard sent `[]` on every save, which will delete locales once
clearing works. It now omits the field unless the operator authored a
language, matching how a blank description preserves the stored one.
- `LOCALE_PATTERN` rejected `zh-Hans` and `es-419`. Play and ASC do not
share a locale vocabulary (zh-CN vs zh-Hans, es-419 vs es-MX), but a
product row targets one platform, so the operator authors that store's
codes and the pattern simply has to admit both families.
- One failing ASC locale aborted the locales after it and reported only
the product id. Non-base locales now fail individually and name the
locale that failed.
CodeRabbit also asked for root paths on `deploy-kit.yml`'s pull_request
trigger; those were already present.
Tests: 950 → 952.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…review Round 2 of self-review over the localization work. Seven issues, several of which made the feature lossy rather than merely incomplete. - `splitStoreListings` dropped a locale when a store had no en-US listing: the first listing was promoted into the base slot and its locale discarded, so the next push republished that text AS en-US. The promoted listing is now retained as a localization too, making the pull → push round trip lossless. - `mergedSubscriptionListings` swallowed read errors and fell through to kit's own listing set. Since the patch replaces the array, that turned a transient 403 into permanent deletion of every Play-Console-authored locale — the exact outcome the read exists to prevent. Read errors now propagate to the per-row failure handler. - `localizations` was returned by no read surface, so the dashboard form could not show what was stored: an operator editing a localized product saw an empty language list and had no way to see or keep it. Exposed on the products query and prefilled when the typed productId matches an existing row. - The dashboard swallowed mutation rejections. The new validation throws on a malformed locale or over-long text, and `void onAdd()` discarded it, so a rejected save looked like nothing happening. Now toasted. - Listing length was validated against Play's caps for both platforms. ASC allows 30/45 against Play's 55/200, so an iOS operator was told their text was fine right up until App Store Connect refused it. Limits are per-platform now. - Locales were trimmed but not case-canonicalized, so `ko-kr` and `ko-KR` both validated as distinct locales and `EN-us` slipped past the base-locale guard. Canonicalized to `ko-KR` / `zh-Hans` form. - ASC's already-submitted comparison checked only the base pair, so a Draft whose only change was a translation looked identical to the locked version and was marked pushed without shipping it. Also documented why the ASC pull deliberately does not read localizations, and why that cannot lose data. Tests: 952 → 955. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/asc.ts`:
- Around line 2024-2047: Update the dry-run Subscription and IAP paths to
iterate over every entry from listingRowsForProduct(row), matching the real push
validation instead of checking only en-US. For each locale, validate the listing
and report its locale when mismatched; also emit a planned write for every
locale rather than only the en-US write.
In `@packages/kit/src/pages/auth/organization/project/products.tsx`:
- Around line 289-295: Update the localization preparation and save flow around
filledLocalizations so any row containing non-whitespace locale, title, or
description is validated rather than silently filtered out. Reject incomplete
rows unless both locale and title are present, and only call upsert—and
subsequently clear the draft—when all entered rows pass validation.
- Around line 140-147: Update the localization-loading useEffect around
loadedLocalizationsKey and setLocalizations so that when editingExisting is
absent or its platform/productId key changes, the existing localizations state
is cleared before returning. Preserve loading behavior for a matching existing
product, ensuring onAdd cannot reuse translations from the previous product.
🪄 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: 641b1630-2f12-48c3-93c4-4ff6c4dc9a7a
📒 Files selected for processing (11)
packages/kit/convex/products/asc.tspackages/kit/convex/products/localizations.test.tspackages/kit/convex/products/localizations.tspackages/kit/convex/products/mutation.tspackages/kit/convex/products/play.test.tspackages/kit/convex/products/play.tspackages/kit/convex/products/query.tspackages/kit/convex/products/sync.tspackages/kit/convex/schema.tspackages/kit/server/api/v1/replay-guard.tspackages/kit/src/pages/auth/organization/project/products.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/kit/convex/schema.ts
- packages/kit/convex/products/localizations.test.ts
- packages/kit/convex/products/mutation.ts
- packages/kit/server/api/v1/replay-guard.ts
- packages/kit/convex/products/sync.ts
- packages/kit/convex/products/play.ts
…ipping a push Round-2 verification confirmed five findings; seventeen were refuted. - The authored-currency pull preference was placed ahead of the US-first rule and matched on currency alone. Play prices several non-US regions in USD (EC, SV, TL, ZW…), so a plain USD row — the default after any first import — resolved to whichever of those Play listed first. US-first now applies within the authored currency, so the KRW/JPY case stays fixed without regressing the common one. - That pull fix reached one-time products only; `pickSubBasePlanPrice` still hard-preferred USD, leaving every subscription row exposed to the same overwrite. It now takes the authored currency too. - ASC's already-submitted comparison inspected only the base pair, so a Draft whose sole change was a translation compared equal to the locked version and was marked pushed without the translation shipping. `ascReviewLocalizationMatches` becomes `ascReviewLocalizationMismatch`, taking the whole listing set and returning the first differing locale from one list fetch. All three call sites — push and both dry-run previews — use it, and the operator-facing message now names the locale instead of always saying en-US. Also fixes the prettier failure on `server/api/v1/replay-guard.ts` from the previous push: it was committed with --no-verify, which skipped the pre-commit gate that mirrors CI's format check. Tests: 955 → 956. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…writes Three review comments on the previous commit. - The form's new prefill loaded an existing row's translations but never cleared them when the typed productId stopped matching that row, so saving a different product published the previous product's locales onto it. Switching away from a loaded row now clears; rows typed for a brand-new product are untouched. - A half-filled language row was silently filtered out and the form then reset, so the operator lost the text with no error. Any row carrying a value now has to have both a locale and a title. - The ASC dry run planned a single en-US localization while the real push writes every locale, so the preview hid the translations the operator was checking. Both dry-run paths now emit one planned write per locale and name the locales they would keep. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The CI step this PR added runs `lint` + `test`, but mcp-server's `lint` was only `tsc --noEmit`. Four files this PR added to that package were unformatted and nothing caught it — the package has no prettier gate at all, so the step could never have failed on style. `lint` now runs the format check too, and the four files are formatted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ions version Live Play E2E against the Petgu app found two things review could not. **Regions version.** `convertRegionPrices` always converts using Play's CURRENT region definitions, but the write pinned `2022/01`. Bulgaria has moved from BGN to EUR since, so pushing a freshly-converted price failed outright: "Invalid currency for region code BG at the specified regions version 2022/01. Expected BGN but got EUR." Every push of a converted price was broken, not just an edge case. The write now uses the version the conversion reports, falling back to the historical pin when there is no conversion to align with. The subscription listings-only patch keeps the pin — it sends no prices. **Sales regions.** Defaulting to every region fixes #288, but it also decided something the operator never said, and `newRegionsConfig` silently opted the product into markets Play launches later. Products now take an optional `regions: ["US","KR","JP"]`; unset keeps the sell-everywhere default. Play refuses to drop a region once a purchase option has it ("Cannot remove region once it has been added"), so an excluded region is withdrawn — `NO_LONGER_AVAILABLE`, which the API documents as legal only from `AVAILABLE`, so anything already withdrawn is left alone — rather than omitted. An explicit footprint also withdraws `newRegionsConfig`; merely omitting it would let the existing purchase option spread a previously-enabled config forward. Wired through schema, mutation, draft queries, both push paths, the products query, `POST /v1/products`, `iapkit_create_product`, and the dashboard. Also fixes the round-3 finding that the dashboard could not clear the last localization: the prefill made delete-all expressible, but `onAdd` collapsed an empty list to `undefined`, which the mutation reads as "leave unchanged". Editing an existing row now sends the array. Verified on Play against dev.hyo.petgu.app with a temporary SKU: 173 regions priced in local currency on create; a price change kept all 173; `regions: [US,KR,JP]` left exactly 3 AVAILABLE with 170 withdrawn and newRegionsConfig NO_LONGER_AVAILABLE; en-US/ko-KR/ja-JP listings all landed. Temporary SKU deleted from kit and Play afterwards. Tests: 956 → 963. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.claude/launch.json:
- Around line 9-22: Update the kit-dashboard launch profile to use port 5173
consistently: change both the Vite --port runtime argument and the profile’s
port field, while preserving strictPort behavior.
In `@packages/kit/convex/products/ascReview.ts`:
- Around line 863-885: Update ascReviewLocalizationMismatch to retrieve and
combine every ASC localization page by following the response pagination links,
including links.next, before matching locales against args.listings. Preserve
the existing title and description comparison logic, and add coverage for a
matching locale returned on a subsequent page.
In `@packages/kit/convex/products/regions.ts`:
- Around line 15-18: Update REGION_PATTERN/productRegionsValidator and the
upsertProduct validation path to verify region codes against an authoritative
Google Play-compatible assigned-region set, rather than accepting any two
uppercase letters. Ensure invalid codes such as ZZ are rejected before
persistence and Android sync, and add a test covering ZZ rejection.
🪄 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: 1618192a-cb46-4611-a5ed-b87941ea5e9c
⛔ Files ignored due to path filters (1)
packages/kit/convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (22)
.claude/launch.jsonpackages/kit/convex/products/asc.tspackages/kit/convex/products/ascReview.test.tspackages/kit/convex/products/ascReview.tspackages/kit/convex/products/mutation.tspackages/kit/convex/products/play.test.tspackages/kit/convex/products/play.tspackages/kit/convex/products/query.tspackages/kit/convex/products/regions.test.tspackages/kit/convex/products/regions.tspackages/kit/convex/products/sync.tspackages/kit/convex/schema.tspackages/kit/server/api/v1/products.tspackages/kit/server/api/v1/replay-guard.tspackages/kit/src/pages/auth/organization/project/products.tsxpackages/mcp-server/package.jsonpackages/mcp-server/src/kit-client.tspackages/mcp-server/src/mcp.tspackages/mcp-server/src/session-routing.tspackages/mcp-server/test/http.test.tspackages/mcp-server/test/session-routing.test.tspackages/mcp-server/test/web.test.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/kit/convex/products/query.ts
- packages/mcp-server/src/kit-client.ts
- packages/mcp-server/test/session-routing.test.ts
- packages/mcp-server/test/http.test.ts
- packages/mcp-server/src/mcp.ts
- packages/kit/convex/schema.ts
- packages/kit/src/pages/auth/organization/project/products.tsx
- packages/kit/convex/products/mutation.ts
- packages/kit/server/api/v1/replay-guard.ts
- packages/mcp-server/test/web.test.ts
- packages/kit/convex/products/sync.ts
- packages/mcp-server/src/session-routing.ts
- packages/kit/convex/products/asc.ts
- packages/kit/convex/products/play.ts
- packages/kit/server/api/v1/products.ts
…coverage Self-review round 3 confirmed twelve findings. The two HIGH ones were coverage gaps proven by mutating the code and watching the suite stay green — so the #289 and pull-ranking fixes were shipping with nothing guarding them. - The replay-guard fix had only a unit test on its predicate. Added middleware tests over the wiring that actually implements it, and verified by mutation that removing `isStableRejection` from the `finally` block now fails the suite. - Both pull-ranking fixes were unreachable from tests because the ranking was inline in the action. Extracted `pickPlayRegionalPrice` and exported `pickSubBasePlanPrice`, then covered the KRW case the fix was for and the USD cases the fix originally regressed. - Subscription offer rows still hard-preferred USD while the row price preferred the authored currency, so one subscription could show its base plan in KRW and its offers in USD. - The new per-locale ASC handler swallowed cancellation and deadline aborts, letting the loop grind through remaining locales after the operator cancelled. Aborts now rethrow. - Play dry-run previews claimed "en-US" while the real push writes every locale, and said nothing about regions. They now describe both. - Listing length was validated with Play's one-time caps for every Android row, but Play documents no title cap for a subscription, so a legal subscription name was refused. Limits are per platform AND type. - The dashboard prefill wiped operator-authored languages and regions when a productId only transiently matched an existing row while being typed. A dirty flag now protects hand-authored rows. Not changed, deliberately: a locale removed in Play Console stays in kit's row. Clearing it would mean pushes delete upstream locales, which is the destructive behaviour this PR exists to remove — same trade the regional configs make. Tests: 963 → 975. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.test.ts`:
- Around line 1101-1123: Update the sub fixture used by the “keeps the
basePlanId paired with the price it picked” test to include a second base plan
with a distinct basePlanId and the selected KRW price, then assert that
pickSubBasePlanPrice(sub, "KRW").basePlanId matches that second plan rather than
the existing monthly plan.
🪄 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: c06f944c-4844-448a-bbb4-09d8b39375e8
📒 Files selected for processing (8)
packages/kit/convex/products/asc.tspackages/kit/convex/products/localizations.test.tspackages/kit/convex/products/localizations.tspackages/kit/convex/products/mutation.tspackages/kit/convex/products/play.test.tspackages/kit/convex/products/play.tspackages/kit/server/api/v1/replay-guard.test.tspackages/kit/src/pages/auth/organization/project/products.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/kit/src/pages/auth/organization/project/products.tsx
- packages/kit/convex/products/asc.ts
- packages/kit/convex/products/mutation.ts
- packages/kit/convex/products/play.ts
The round-3 dirty flag was on the wrong side. It stopped the editor being cleared when the operator left a loaded row, which meant hand-editing a loaded product and then typing a brand-new id kept that product's locales — the very leak the clear existed to prevent. And it did nothing about the actual destructive direction: a half-typed productId that transiently matches an existing row would prefill over work the operator was in the middle of writing. Replaced with the distinction that matters: whether the editor contents came from a prefill or from the operator. Leaving a prefilled row clears; hand-authored content survives a transient match and is never overwritten; switching between two stored rows still loads the second. Both refs reset after a save so the next typed id loads normally. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Eighteen confirmed. The sales-regions feature added last commit carried most of them — it was accepted on surfaces that never apply it. - `regions` is now rejected for iOS and for subscriptions instead of being stored and silently ignored. ASC prices per territory through a resource this workflow doesn't touch, and Play fixes a base plan's regional configs at create (the update masks `listings` only), so on those paths the field could never do anything. The legacy `inappproducts` fallback likewise fails loudly rather than pricing every region for an operator who asked for three. - The regions-version fix only covered the success branch. On the degraded path the write still echoes configs Play generated at a newer version, so the historical pin reproduced the exact BG/BGN rejection the fix was for. The write now follows the version Play reports on the product it read back. - The no-conversion US fallback published US even when the footprint excluded it; it now fails with a message naming the footprint. - A region kit withdrew could never be re-enabled by adding it back — the footprint was a one-way door. - The manual action counted deliberately withdrawn regions as prices left stale, so a three-region product reported 170 stale prices. - Dry-run previews advertised a regions footprint on the subscription patch path, which writes listings only. The dashboard prefill guard is rewritten again. The round-3 flag needed resetting on every path that empties the editors; missing one latched prefill off permanently, and another made a cleared editor overwrite a stored product's locales on the next save. It now derives "is this hand-authored" from what is actually on screen, which cannot go stale. Also pinned prettier in mcp-server rather than fetching it per CI run. Not changed: Play's live discovery documents a 200-character cap for a subscription listing description, so the current limit is right — the finding that called it wrong was reading a stale bundled type. Tests: 975 → 979. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/kit/convex/products/mutation.ts (1)
595-599: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate preserved fields when
typechanges.When an existing Android one-time product has non-empty
existing.regions, a caller can changeargs.typeto"Subscription"and omitregions. The guard checks only the new argument, so it does not reject the update. Line [665] then preserves the regions on the subscription. The row violates the invariant stated in the new validation block, and the Play sync path cannot apply those regional settings to the subscription.The same type change skips
normalizeProductLocalizationswhenlocalizationsis omitted. Existing listings can therefore bypass the subscription-specific limits enforced by the normalizer.Load the existing row before final validation and validate the effective fields, or reject incompatible type changes. Add tests for omitted fields and explicit empty-array clears.
Also applies to: 607-613, 660-665
🤖 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/mutation.ts` around lines 595 - 599, Update the product mutation around normalizeProductLocalizations and the final validation block to load the existing row first and validate effective fields formed from existing values plus supplied arguments. Ensure changing an Android one-time product to Subscription cannot preserve non-empty regions, and always apply subscription localization limits even when localizations is omitted. Add coverage for omitted fields and explicit empty-array clears.
🧹 Nitpick comments (1)
packages/kit/convex/products/play.test.ts (1)
1248-1258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that
manualActionis present before checking the message.The assertion uses optional chaining. If
manualActionisundefined,outcome.manualAction?.messageisundefinedandnot.toContainstill passes. The test then passes for the wrong reason. Add a positive assertion on the reported region count.♻️ Proposed test tightening
// DE was withdrawn on purpose; calling it a stale price would be // alarming and wrong. + expect(outcome.manualAction).toBeDefined(); + expect(outcome.manualAction?.message).toContain("applied to 1 region(s)"); expect(outcome.manualAction?.message).not.toContain( "kept their previous price", );🤖 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 1248 - 1258, Add a positive assertion that outcome.manualAction is present and reports the expected region count before checking its message in the upsertModernAndroidOneTimeProduct test. Then access the asserted manualAction message directly so the test cannot pass when no manual action is returned.
🤖 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.
Outside diff comments:
In `@packages/kit/convex/products/mutation.ts`:
- Around line 595-599: Update the product mutation around
normalizeProductLocalizations and the final validation block to load the
existing row first and validate effective fields formed from existing values
plus supplied arguments. Ensure changing an Android one-time product to
Subscription cannot preserve non-empty regions, and always apply subscription
localization limits even when localizations is omitted. Add coverage for omitted
fields and explicit empty-array clears.
---
Nitpick comments:
In `@packages/kit/convex/products/play.test.ts`:
- Around line 1248-1258: Add a positive assertion that outcome.manualAction is
present and reports the expected region count before checking its message in the
upsertModernAndroidOneTimeProduct test. Then access the asserted manualAction
message directly so the test cannot pass when no manual action is returned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 67ccd892-c549-4122-b036-f1dee27a4340
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
packages/kit/convex/products/mutation.tspackages/kit/convex/products/play.test.tspackages/kit/convex/products/play.tspackages/kit/src/pages/auth/organization/project/products.tsxpackages/mcp-server/package.jsonpackages/mcp-server/src/session-routing.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/mcp-server/package.json
- packages/mcp-server/src/session-routing.ts
- packages/kit/src/pages/auth/organization/project/products.tsx
- packages/kit/convex/products/play.ts
… save The previous commit stopped the mutation storing a region footprint for iOS and for subscriptions, but every surface still invited one. The dashboard rendered the input for an iOS product or a subscription and only reported the problem after the operator filled it in and pressed save, and the MCP tool described the field as if it worked everywhere. The form now derives support from the draft's platform and type, hides the field when it does not apply, and never sends it in that case; the MCP description states the restriction up front. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e-count arithmetic Round 5 confirmed thirteen. Two were the dashboard prefill again, so that guard is gone rather than repaired. Inferring "load this product's stored metadata" from "the typed id happens to match a row" was rewritten three times and lost data three different ways: a half-typed id overwrote work in progress, a stale flag latched loading off permanently, and — found this round — a single blank language row made an apparently-empty editor delete a product's stored listings and regions on save. The heuristic has no state that is both safe and complete, because "the editor is empty" and "the operator wants it empty" are indistinguishable without an explicit action. There is now a "Load stored languages" button. An empty editor sends nothing and preserves what is stored; only a row the operator explicitly loaded may send an empty array, which is how delete-all still works. The form says which mode it is in. Also from round 5: - The stale-region count mixed a filtered numerator with an unfiltered counter, so a withdrawn region that happened to be repriced made `stale` negative and silently dropped the "kept their previous price" warning — and reported withdrawn regions as successfully priced. Both numbers now come from the same set, and a config with no availability counts as live. - The legacy-path region guard ran inside the fallback, replacing whatever the modern API actually failed with. It now runs before the modern attempt. - The regions guard inspected only the incoming argument, so a product with stored regions retyped as a Subscription kept a footprint nothing applies. It now checks the row as it will be. - Localization and region validation threw plain Errors, which REST and MCP mapped to 500. They are ConvexErrors now, so operator input mistakes surface as 400 with the message. Tests: 979 → 982, including a mutation-verified case for the stale arithmetic. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d back on 5173 Three review comments. - The region validator accepted any two letters, so `ZZ` was stored and then silently dropped by the Android sync. It now requires an assigned territory: `Intl.DisplayNames` catches ordinary typos, and the ISO 3166-1 reserved ranges are excluded explicitly rather than by shipping a country list that would go stale. `XK` stays valid — CLDR names it, and whether Play sells there is answered by the sync, not a guess here. - Backstopping that: a requested region Play returns no price for now produces a manual action naming it, instead of quietly not appearing in the write. That covers codes the validator cannot know about, including a region Play stops selling in later. - The `kit-dashboard` launch profile forced port 5174, which is exactly what packages/kit/README.md warns against: Convex `SITE_URL` is set to http://localhost:5173 for local OAuth, so 5174 makes the OAuth return URL unreachable. Fixed to 5173, keeping `--strictPort` so a clash fails visibly. Not changed: `ascReviewLocalizationMismatch` reading one page of `localizations?limit=200`. The App Store has 39 locales and a version holds at most one localization per locale, so the list cannot paginate. Tests: 982 → 984. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round 6 confirmed fifteen. The first one matters more than the rest. Round 5 moved `assertLegacyPathUsableFor` out of the legacy fallback to the top of `upsertAndroidOneTimeProduct`, answering "the regions message hides the real modern failure". That made it unconditional: every Android one-time product with a region footprint threw before Play was ever contacted, telling the operator their app is on the legacy API without having checked. The feature was dead on its only supported surface — and the whole suite stayed green, because every regions test called `upsertModernAndroidOneTimeProduct` directly and never went through the wrapper the guard now sat in. The check is back on the legacy branch, and round 5's concern is met by chaining the modern failure into the message instead of replacing it. `upsertAndroidOneTimeProduct` is exported and tested, so reintroducing that mistake now fails two tests (verified by mutation). The rest: - The unpriced-region action returned early, so a conversion failure was never reported when a footprint also had an unpriced region. Both are now reported, and the unpriced check uses the same `isLive` predicate as the stale count — testing `availability === "AVAILABLE"` directly was wrong because Play omits the field when it is available, which produced false "Play does not sell there" claims. - The region validator returned early for QA–QL, waving unassigned codes past the CLDR check. Only the reserved spans (QM–QZ, XA–XZ) short circuit now; QA is Qatar, QB is nothing. - Duplicate-locale and blank-title rejections still threw plain Errors, so REST and MCP answered 500 for them. - The dashboard could silently replace a product's stored languages: adding one row without loading first sends a one-element array, and an array replaces. It now refuses and says to load first. - The save toast printed Convex's wrapper instead of the guidance the mutation wrote. Tests: 984 → 988. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Not a retreat from the idea — a judgement about where it belongs. `regions` was my addition mid-review, and no part of #287, #288 or #289 needs it. Across four review rounds it produced 13 of round 7's 16 confirmed findings, and the per-round totals have not converged: 8, 5, 12, 18, 13, 15, 16. It also shipped once completely broken, with every test green, because the guard that killed it sat in a wrapper no test went through. The rest of this PR is in a different state: the three issues and the localized listings are verified against real Play and real App Store Connect data and have been stable across rounds. Continuing to carry an optional feature that keeps generating findings is what is stopping this converge, so it comes out and goes into its own PR with its own E2E. What stays, because it fixes something this PR is actually about: - `regionsVersionFor` and the version read back from the product. Live E2E proved every push of a converted price failed at the pinned 2022/01 once Bulgaria moved BGN to EUR; that is issue #288, not the regions feature. - The conversion-failure manual action and its applied/stale counts, simplified back to "no footprint" and with `isLive` now treating a region priced ahead of release as live rather than as one Play refuses to sell. - Everything about localized listings. Tests: 988 → 973, the difference being the regions cases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…t had none Round 8 confirmed 23, but the shape changed: 16 are missing coverage and only 7 are behaviour. Those 7 first. - The cooldown exemption included `UNKNOWN`, reasoning that an answer we could not interpret deserves another try. But Google's 410 "the purchase token is no longer valid" maps to exactly `UNKNOWN`, so the captured-then-revoked receipt this guard exists to wall off could replay freely. Only `PENDING` is exempt now — a deferred payment does resolve, a revoked token does not. #289 is unaffected: a fresh purchase verifies valid and never reaches the cooldown. - A push flipped an operator's withdrawn `newRegionsConfig` back to AVAILABLE. Per-region availability was already preserved, so this was an asymmetry, and it silently undid "do not follow Play into new markets" on every reprice. - The generated Convex API still imported the deleted `products/regions` module; regenerated. - Comments the removal orphaned, including one that told a future maintainer to skip adding a region the product lacks — the opposite of the #288 repair, and stated where a test cannot contradict it. Coverage, all mutation-verified (the mutation was applied, the suite run, the file restored): - `mergedSubscriptionListings`, the only thing stopping a subscription patch deleting Play-Console locales, had no tests at all. - The #288 repair path — a live US-only product GAINING regions — had none either. Every existing test created fresh or preserved, so the behaviour the issue is about was unproven. - `regionsVersionFor`'s precedence survived inversion silently, and that precedence is what the live E2E failure came down to. Kept at 200: Play's live discovery document reports a 200-character cap for a subscription listing description. The bundled googleapis 157 types still say 80; validating at 80 would refuse text Play accepts. Noted in the code so it stops being re-raised. Tests: 973 → 981. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round-8 coverage pass. Each gap below was confirmed by cutting the production code and watching the suite stay green, then re-confirmed by the new test failing on the same cut. - upsertProduct: omitted vs [] localizations (undefined is Convex's "leave unchanged", so only [] can clear a stored set) - upsertFromStore: a pull never deletes a kit-authored locale; drop the dead v.null() from its validator, which read as a clear but coalesced - retry predicate: a permission error must fail fast, not retry — with shouldRetry widened to every error all 31 tests still passed - legacy inappproducts fallback: autoConvertMissingPrices on both insert and patch, every locale in the listings map, and no fallback on a 403 (this whole path, half of #288, had no test) - ASC localization compare: a description-only change now fails; the existing case changed both fields, so a title-only compare passed - REST /products: localization shape rejection and forwarding - MCP http.ts: GET/DELETE unknown-session routing (web.ts had it) - localizations sort: asserted directly instead of incidentally Extract the dashboard form's localization decision into resolveProductLocalizations so the delete-all, replace-guard, and half-typed-row rules are testable — that logic lost operator data three times while it was inline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Restores the sales-regions field (#293) into this PR, since every kit-related issue is being closed together. This is a revert of the revert, but not a straight one — auto-merge silently reintroduced two regressions that had been fixed after the feature was cut, and both are undone here: - `isLive` went back to `=== "AVAILABLE"`. Play returns AVAILABLE_IF_RELEASED for a region priced ahead of release, so that reading made the stale count disagree with the configs it counted. Kept as the NOT-withdrawn test, plus the footprint exclusion. - `newRegionsConfig` went back to `undefined` in the no-footprint branch, relying on the request spread to carry the old value. Made explicit again — the same reasoning that makes the withdrawal necessary applies to preserving it. The precondition that mattered most from #293 was coverage through the public door: the feature once shipped completely dead, with a green suite, because the guard that killed it sat in a wrapper no test went through. `upsertAndroidOneTimeProduct` now has its own regions cases, and re-creating that exact bug — hoisting assertLegacyPathUsableFor above the modern attempt — fails 4 of them. Ignoring the footprint fails 3, and re-enabling the new-regions opt-in fails 3. The dashboard's language and region rules share one load-before-edit guard, so the extracted resolver now covers both rather than the form carrying the region half inline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Phase 1 already made the source-aware webhookEvents index the dedup read. The key row was then a second copy of a guarantee the event row already makes, on the same (projectId, source, sourceNotificationId) triple and in the same transaction — one extra insert per webhook, for every webhook. Nothing writes new rows now. The reads stay, and are drain-only: rows written before this still dedup, a half-written legacy row still gets linked to the event this call creates, and the prune cron still ages them out. Cutting the link leaves the orphan sweep free to delete a row a replay depends on, so it is covered — removing it fails 2 tests. Phases 3 and 4 (drop the table, its cron, and the deletion cascades) cannot ride this deploy: the legacy fallback has to stay live until the table drains past WEBHOOK_RETENTION_MS, which is 30 days of real time, not a code change. #241 stays open for that step. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
hyochan
changed the title
hyochan
changed the title
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/kit/convex/products/play.test.ts (1)
58-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDisable gaxios retries in
stubAndroidPublisher.
stubSubscriptionsat Line 1728 setsretryConfig: { retry: 0, noResponseRetries: 0 }, butstubAndroidPublisherdoes not. Several tests throw a 500 from theconverthandler, for example at Lines 1246, 1494, 1524, 1561, and 1614. googleapis retries 5xx responses by default, so those tests can issue repeated conversion requests and record extra entries inrequests. The assertions usefindandsome, so they still pass, but the tests run slower and any future request-count assertion becomes unstable.♻️ Proposed fix
const androidpublisher = google.androidpublisher({ version: "v3", + retryConfig: { retry: 0, noResponseRetries: 0 }, adapter: async <T>(🤖 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 58 - 91, Update stubAndroidPublisher to disable googleapis/gaxios retries by configuring the client with retryConfig values retry: 0 and noResponseRetries: 0, matching stubSubscriptions. Preserve the existing request adapter and handler behavior.packages/kit/convex/products/asc.test.ts (1)
696-795: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the benign 409 branch.
pushAscReviewLocalizationsskips a non-base locale whenisBenignAscRetryConflict(error)is true, and it records no failure. No test covers that branch. A regression there would convert a replayed sync into a reported failure for every already-written locale.Add one case that throws an
AscApiErrorwith status 409 and an "already exists" message forko-KR, then assertja-JPis still attempted andfailuresstays empty.🤖 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/asc.test.ts` around lines 696 - 795, Add a test in the pushAscReviewLocalizations suite covering a ko-KR upsert that throws an AscApiError with status 409 and an “already exists” message; verify ja-JP is still attempted and the collected failures array remains empty.packages/kit/convex/products/ascReview.test.ts (1)
1138-1184: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover the rejected pagination host.
This test covers only a
links.nextURL on the ASC origin.ascNextPathalso throws when the origin differs, which is the guard that stops the paginator from following an attacker-supplied or misconfigured host. Add a case wherelinks.nextpoints to another origin and assert the rejection message.🤖 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/ascReview.test.ts` around lines 1138 - 1184, Extend the pagination tests around ascReviewLocalizationMismatch to include a links.next URL with a different origin, exercising ascNextPath’s host validation. Assert that the call rejects with the expected rejection message and does not follow the external URL, while preserving the existing same-origin pagination case.
🤖 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/webhooks/internal.ts`:
- Around line 304-317: Update the Google webhook handler around
lookupExistingEvent so retries for already-recorded events still reach
applySubscriptionEvent before returning, while preserving the Play API quota
optimization. Ensure genuinely duplicate events remain deduplicated, and add a
regression test covering a first delivery that records the event but fails
before subscription application, followed by a retry that repairs the
subscription.
In `@packages/kit/src/pages/auth/organization/project/product-localizations.ts`:
- Around line 102-116: Update the editingExisting and not isLoadedRow guard to
consider region changes only when args.supportsSalesRegions is true, while
continuing to check filled languages. Keep replacesRegions and the existing
error messages aligned with this gated condition so unsupported products are not
blocked by regionMode.
---
Nitpick comments:
In `@packages/kit/convex/products/asc.test.ts`:
- Around line 696-795: Add a test in the pushAscReviewLocalizations suite
covering a ko-KR upsert that throws an AscApiError with status 409 and an
“already exists” message; verify ja-JP is still attempted and the collected
failures array remains empty.
In `@packages/kit/convex/products/ascReview.test.ts`:
- Around line 1138-1184: Extend the pagination tests around
ascReviewLocalizationMismatch to include a links.next URL with a different
origin, exercising ascNextPath’s host validation. Assert that the call rejects
with the expected rejection message and does not follow the external URL, while
preserving the existing same-origin pagination case.
In `@packages/kit/convex/products/play.test.ts`:
- Around line 58-91: Update stubAndroidPublisher to disable googleapis/gaxios
retries by configuring the client with retryConfig values retry: 0 and
noResponseRetries: 0, matching stubSubscriptions. Preserve the existing request
adapter and handler behavior.
🪄 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: 4e61d1c1-3c4d-46c8-9592-dfd5425e437e
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
.claude/launch.jsonpackages/kit/convex/migrations.tspackages/kit/convex/products/asc.test.tspackages/kit/convex/products/asc.tspackages/kit/convex/products/ascReview.test.tspackages/kit/convex/products/ascReview.tspackages/kit/convex/products/localizations.test.tspackages/kit/convex/products/localizations.tspackages/kit/convex/products/mutation.test.tspackages/kit/convex/products/mutation.tspackages/kit/convex/products/play.test.tspackages/kit/convex/products/play.tspackages/kit/convex/products/query.tspackages/kit/convex/products/regions.test.tspackages/kit/convex/products/regions.tspackages/kit/convex/products/sync.test.tspackages/kit/convex/products/sync.tspackages/kit/convex/purchases/android.test.tspackages/kit/convex/purchases/android.tspackages/kit/convex/purchases/extract-order-id.test.tspackages/kit/convex/purchases/extract-product-id.test.tspackages/kit/convex/purchases/internal.tspackages/kit/convex/purchases/query.tspackages/kit/convex/purchases/save-purchase-idempotency.test.tspackages/kit/convex/purchases/shared.tspackages/kit/convex/schema.tspackages/kit/convex/webhooks/internal.test.tspackages/kit/convex/webhooks/internal.tspackages/kit/server/api/v1/products.test.tspackages/kit/server/api/v1/products.tspackages/kit/server/api/v1/replay-guard.test.tspackages/kit/server/api/v1/replay-guard.tspackages/kit/server/api/v1/routes.test.tspackages/kit/server/api/v1/routes.tspackages/kit/src/pages/auth/organization/project/product-localizations.test.tspackages/kit/src/pages/auth/organization/project/product-localizations.tspackages/kit/src/pages/auth/organization/project/products.tsxpackages/mcp-server/package.jsonpackages/mcp-server/src/kit-client.tspackages/mcp-server/src/mcp.tspackages/mcp-server/src/session-routing.tspackages/mcp-server/test/http.test.tspackages/mcp-server/test/kit-client.test.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/mcp-server/src/kit-client.ts
- .claude/launch.json
- packages/mcp-server/package.json
- packages/kit/convex/products/query.ts
- packages/mcp-server/src/mcp.ts
- packages/kit/convex/products/mutation.ts
- packages/kit/server/api/v1/products.ts
- packages/mcp-server/src/session-routing.ts
- packages/kit/src/pages/auth/organization/project/products.tsx
- packages/kit/server/api/v1/replay-guard.test.ts
- packages/kit/convex/products/play.ts
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/kit/convex/subscriptions/internal.test.ts (1)
191-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the
appliedAtassertions.Line 191 reads
appliedAtwith optional chaining. If the handler never setsappliedAt, both reads produceundefinedand Line 196 still passes. The test then no longer proves that the first call marks the event applied. Assert that the value is a number before comparing.♻️ Proposed change
const appliedAt = db.rows("webhookEvents")[0]?.appliedAt; + expect(appliedAt).toBe(Date.now());🤖 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/subscriptions/internal.test.ts` around lines 191 - 196, Strengthen the appliedAt assertion in the test around applySubscriptionEventHandler by first asserting that the captured appliedAt value is a number, then retain the comparison against the post-call value to verify it remains unchanged.
🤖 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/subscriptions/internal.ts`:
- Around line 164-179: Change the obsolete-event comparison in the lastEventId
handling branch to require lastEvent.occurredAt strictly greater than
storedEvent.occurredAt, allowing distinct same-timestamp lifecycle events to
apply. Update the nearby rollout-compatibility comment to clarify that this
branch still preserves ordering for newer recorded events.
---
Nitpick comments:
In `@packages/kit/convex/subscriptions/internal.test.ts`:
- Around line 191-196: Strengthen the appliedAt assertion in the test around
applySubscriptionEventHandler by first asserting that the captured appliedAt
value is a number, then retain the comparison against the post-call value to
verify it remains unchanged.
🪄 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: 3a940dfd-37d6-432c-bc62-fb8360e3fef0
📒 Files selected for processing (12)
packages/kit/convex/products/asc.test.tspackages/kit/convex/products/asc.tspackages/kit/convex/products/ascReview.test.tspackages/kit/convex/products/ascReview.tspackages/kit/convex/products/localizations.test.tspackages/kit/convex/products/localizations.tspackages/kit/convex/schema.tspackages/kit/convex/subscriptions/internal.test.tspackages/kit/convex/subscriptions/internal.tspackages/kit/convex/webhooks/apple.tspackages/kit/convex/webhooks/google.test.tspackages/kit/convex/webhooks/google.ts
💤 Files with no reviewable changes (1)
- packages/kit/convex/webhooks/google.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/kit/convex/products/ascReview.test.ts
- packages/kit/convex/webhooks/google.ts
- packages/kit/convex/products/localizations.test.ts
- packages/kit/convex/products/ascReview.ts
- packages/kit/convex/products/asc.test.ts
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
hyochan
added a commit
that referenced
this pull request
Aug 13, 2026
The page states it is the canonical changelog and that every shipped PR lands an entry, but the last one was 2026-07-28 while five production changes had deployed since. Entries reconstructed from each PR, dated by its merge to main, which is when deploy-kit.yml ships it: order lookup (#285), sync/verification/MCP session correctness (#292), the production Convex target guard (#314), store verification integrity (#313), and the entitlement defects the conformance suite surfaced (#316). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
7 tasks done
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.
Fixes #287
Fixes #288
Fixes #289
Fixes #293
Implements phase 2 of #241 without closing it. Phases 3–4 still require the legacy webhook-idempotency rows to age past the retention window.
Summary
This PR closes the remaining IAPKit correctness gaps across Play and App Store Connect product sync, purchase verification, localized listings, sales regions, MCP session routing, and webhook lifecycle processing.
Google Play product sync
Sales-region contract
Android one-time products now have four stored states:
"all": deliberately expand to every priced region and follow Play into future markets["US", "KR"]: restrict to exactly the named markets and withdraw the rest[]: clear a stored choice back to inherited behaviorThe dashboard exposes these choices directly. REST and MCP accept the same contract. iOS products and subscriptions reject explicit footprints because their current push paths cannot apply them.
Region validation uses the current ISO 3166-1 alpha-2 assignment set plus
XK, rejecting macroregions, deleted aliases, reserved codes, and CLDR pseudo-regions before persistence.Localized listings and App Store Connect prices
en-US.jaandkoshortcodes and validates the complete ASC locale inventory before persistence.startDatefor an immediately effective ASC in-app-purchase price; explicit future schedules retain their supplied date.Fresh and multi-item purchase verification
expectedProductIdbefore expiry ranking for multi-line subscriptions.UNKNOWNas retryable by default.UNKNOWNonly when Google explicitly returns the stable 410 revoked-token verdict.MCP session affinity
Webhook deduplication and atomic application
webhookIdempotencyKeysrows. The source-awarewebhookEventsindex remains authoritative, while legacy reads stay in place until retention makes table removal safe.webhookEvents.appliedAtin the same Convex mutation as the subscription and incremental-stat transition. A stale duplicate event therefore remains a no-op even after a newer event replacessubscriptions.lastEventId.Verification
git diff --checkpassedFinal-head live regression checks also confirmed:
en-US,ja-JP, andko-KR; a price-only update preserved the exact region set; an explicitUS/KR/JPfootprint produced exactly three live regions; pull-back preserved the catalog; deletion returned PlayNOT_FOUND.en-US,ja, andko; the current price had null start/end dates; deletion returned ASCNOT_FOUND.isValid: true, the app finished the consumable transaction, the matching receipt count increased exactly from 14 to 15, and no duplicate Google remote ID was stored.Started(A) → Expired(B) → duplicate(A)without state or stats rollback.All temporary store SKUs, local rows, USB reverse mappings, servers, and test configuration were removed or restored afterward. Existing Petgu iOS rows were restored to their original
Draftstate.Follow-up after deployment
Re-run the #287 cross-machine reproduction: initialize one MCP session and issue 20 consecutive
tools/listrequests. Expected result: 20/20 succeed through Fly machine replay.Summary by CodeRabbit
New Features
Bug Fixes