fix(kit): ASC 401 in production + stale-toast on Products tab#128
Conversation
The completion toast was re-firing every time the operator opened the Products tab when a stale terminal job sat in `productSyncJobs`. Earlier passes guarded the in-memory `lastShownJobIdRef` against `progress.phase === "dismissed"`, but the underlying issue was using a "have I shown this jobId yet" set — a fresh mount started with an empty set, so the very first observation of an already-terminal job re-toasted it. Replace the set with a transition rule: track the previous status per platform, and only toast when the prior render saw the same jobId in `queued` / `running` and the new render sees it in `succeeded` / `failed`. Result: - Land on the page with a pre-existing terminal job → silent. The result banner still surfaces it; nothing pops up. - Trigger a sync from this mount → toast fires on completion as before. - Job ids change between mounts (a new sync ran on another tab) → no toast on the first observation; if it's still running we'll toast when it terminates here. - Dismissed jobs continue to be skipped explicitly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe sync-completion toast behavior in the products page is refactored to emit toasts only upon true status transitions. The previous per-jobId-per-mount guard is replaced with a per-platform status snapshot that tracks prior job state and only fires success/failure toasts when a job transitions from queued/running to a terminal state. ChangesToast Transition Logic
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request refactors the product sync job notification logic to use a state transition model, ensuring that completion toasts only fire when a job moves from a non-terminal to a terminal state. This change prevents redundant notifications when revisiting the page. Feedback was provided to leverage the existing "SyncJob" type for the job status snapshot to ensure consistency with the project's type definitions and adhere to the Single Source of Truth principle.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/kit/src/pages/auth/organization/project/products.tsx (1)
67-72: ⚡ Quick winUse an interface and reuse
SyncJobfield types for the snapshot.This new object shape is a better fit for an
interface, andjobId/statuscan stay aligned with the backend type instead of being re-declared here.♻️ Suggested refactor
- type JobStatusSnapshot = { - jobId: string; - status: "queued" | "running" | "succeeded" | "failed"; - }; + interface JobStatusSnapshot { + jobId: SyncJob["_id"]; + status: SyncJob["status"]; + }As per coding guidelines,
**/*.{ts,tsx}: Prefer interface for defining object shapes in TypeScript.🤖 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 67 - 72, Replace the type alias JobStatusSnapshot with an interface and reuse the backend SyncJob field types instead of re-declaring them: define interface JobStatusSnapshot { jobId: SyncJob['id']; status: SyncJob['status']; } and keep prevJobStatusRef as useRef<Record<'IOS' | 'Android', JobStatusSnapshot | null>> so the snapshot stays aligned with the SyncJob type and follows the project's interface preference.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/kit/src/pages/auth/organization/project/products.tsx`:
- Around line 67-72: Replace the type alias JobStatusSnapshot with an interface
and reuse the backend SyncJob field types instead of re-declaring them: define
interface JobStatusSnapshot { jobId: SyncJob['id']; status: SyncJob['status']; }
and keep prevJobStatusRef as useRef<Record<'IOS' | 'Android', JobStatusSnapshot
| null>> so the snapshot stays aligned with the SyncJob type and follows the
project's interface preference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fae1d3ba-7941-4d59-869b-f61527b99c57
📒 Files selected for processing (1)
packages/kit/src/pages/auth/organization/project/products.tsx
…ASC private key `resolveAscCredentials` required both `iosAscIssuerId` AND `iosAscKeyId` to be populated before using the ASC pair. The Settings UI deliberately exposes only ONE Issuer ID input (documented as "shared with Server API above") because Apple uses a single Issuer per team across both API gateways — so production projects always have `iosAscIssuerId = undefined` and `iosAscKeyId = "<asc team key id>"`. Old gate (`iosAscIssuerId && iosAscKeyId`) returned false → fallback path used `iosAppStoreKeyId` (the In-App Purchase / Server API key id) but signed the JWT with the ASC private key content (loaded via `getAppleAscApiKey`). Apple rejected every request with 401 because `kid` claim didn't match the signing key. Affects every production tenant; reported by LukasB-DEV in the PR #127 thread, reproduced by hyochan on kit.openiap.dev ("localhost works, prod doesn't" — dev DBs happened to have matching values from earlier UI iterations). Fix: gate on `iosAscKeyId` alone, fall back issuer to `iosAppStoreIssuerId` when the dedicated ASC issuer slot is empty. Behavior: - ASC slot has key id → ASC pair (`issuerId`: ASC slot ?? legacy shared, `keyId`: ASC slot, `.p8`: ASC slot ?? legacy) - ASC slot empty → legacy Server API pair end-to-end (back-compat for projects that uploaded the ASC key into the legacy slot before the second slot existed) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…her products" header
The Products table renders a "Subscription Group · {name}" header
above clustered subscriptions, but Consumable / NonConsumable rows
fell straight after the cluster with no closing delimiter — visually
they looked like part of the subscription group even though
`groupRowsByHierarchy` had correctly partitioned them as ungrouped.
Add an explicit "Other products" header row before the ungrouped
section, but only when at least one subscription group already
rendered above (otherwise the table is just a flat list and the
extra header would be noise). Same row structure as the existing
group header, just a different label, so the visual treatment is
consistent and the section break is unambiguous.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
28c0f71 to
d75d401
Compare
The kit dev launch config booted Vite (5173), Hono (3000), and Convex in parallel. Hono serves the SPA out of `./dist`, but `dist/` is whatever the previous build left behind — and `bun run build` is `vite build` which defaults to mode=production. In production mode Vite loads `.env.production` AFTER `.env.local`, so the prod Convex URL wins and gets baked into every `dist/assets/*.js` bundle. Visiting localhost:3000 in that state ran the prod GitHub OAuth App and bounced the operator out to kit.openiap.dev — even in incognito, because the redirect target lived in the bundle, not in cookies. Symptom triaged on Hyo Dev / martie. Wipe `dist/` + `node_modules/.vite`, then explicitly rebuild with `vite build --mode development` so Hono's static fallback at port 3000 ships a bundle pinned to `.env.local` (dev Convex URL). Vite at 5173 was already correct (its dev server runs in development mode by default). Single launch entry, always-fresh dev env, no manual cache management. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
d75d401 to
bdf7c9d
Compare
…ly result banner
Three small Products-page tweaks driven by yesterday's triage:
- Move "🧰 Kit: Dev (Vite + Hono + Convex)" to the top of
`.vscode/launch.json` since kit is the daily-driver task.
- Group headers in the Products table now use colored backgrounds
(blue tint for "Subscription Group · {name}", amber tint for
"Other products") with a 4px left accent bar so the section
break is unambiguous. Earlier `bg-muted/60` blended into the
data rows on dark mode.
- Result banner + completion toast now gate on
`sessionTriggeredJobIdsRef` — a `Set<jobId>` populated when the
operator clicks Sync / Dry-run / Reset from THIS mount. Stale
terminal jobs from a previous session (HMR reload, page
revisit, sync triggered in another tab) no longer surface as if
they had just run. The ref resets on every remount so the gate
is automatic and never sticky.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Gemini review on PR #128 — pull the field types straight off `SyncJob` (= `Doc<"productSyncJobs">`) so adding a new status literal in `convex/schema.ts` automatically widens the local snapshot type instead of silently drifting from the database shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…learing Three production-breaking issues surfaced after PR #128 deploy (LukasB-DEV report on PR #128): 1. **ASC "No price tier matches USD 39.99 / 24.99 / 299.99"**. `findIapUsaPricePointId` / `findSubUsaPricePointId` only fetched the first `limit=200` page of Apple's USA price points. Apple returns price points in opaque-id order, not by USD amount, so standard tiers like $24.99 / $39.99 / $299.99 could land beyond the first page and miss the tolerance match — surfaced as a hard failure on otherwise-valid pushes. Switched both to `collectAllPages` so every tier is considered. 2. **Pulled-from-store rows re-pushed on every sync**. ASC states like `PREPARE_FOR_SUBMISSION` / `MISSING_METADATA` map to kit `Draft` via `mapAscState`'s default branch. The push filter (`state === "Draft"`) then picks them up, patches them back into ASC, `markPushed` flips them to `Ready`, and the next pull re-applies ASC's Draft-equivalent state — looping forever and inflating the `pushed` counter for products the operator never edited. Fix: new `origin: "kit" | "store"` schema field set on first insert. `listDraft*Products` now filters `state === "Draft" AND (origin === "kit" OR storeRef === undefined)` so pulled drafts are excluded while partial-sync resumption (kit row whose CREATE succeeded but localization/price failed, has storeRef AND origin="kit") still works. `upsertProduct` UPDATE always claims `origin: "kit"` — an operator editing a pulled row through the dashboard form is explicitly saying "I want this version on the store", so the next push picks it up. `upsertFromStore` UPDATE only back-fills `origin: "store"` when undefined, so a kit-edited row stays kit-managed. 3. **`subscriptionGroupName` clung to non-Subscription rows**. Convex treats `db.patch({field: undefined})` as a no-op, so the IAP pull path's empty group never cleared a stale value left over from a previous Subscription state or operator-form input. Rows kept showing under the dashboard's "Subscription Group" cluster. Fix: schema widened to `v.union(v.string(), v.null())` for `subscriptionGroupId` / `subscriptionGroupName`, and `upsertFromStore` passes `null` (not `undefined`) when `args.type !== "Subscription"`. Public `listProducts` query coerces `null → undefined` at the boundary so the SDK / SPA contract doesn't change. No data migration needed — both new fields are optional unions on existing optional columns. Self-heals on the first sync after deploy: `upsertFromStore` back-fills `origin: "store"` on legacy rows touched by pull, and clears stale group fields on non-Sub rows. 346/346 vitest, typecheck/lint/format clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Bundle of post-merge fixes for the kit Products page on top of #127. All driven by real reports — LukasB-DEV on #127, hyochan triage on Hyo Dev / martie.
Fixes
1. ASC API 401 across every production tenant (4037afe)
resolveAscCredentialsrequired bothiosAscIssuerIdANDiosAscKeyIdto flip into the ASC slot. The Settings UI deliberately exposes a single shared Issuer ID input ("Issuer ID is shared with the Server API above — Apple uses one Issuer per team"), so production projects always haveiosAscIssuerId = undefinedandiosAscKeyId = "<team-key-id>".Old gate returned
false→ fallback resolved(iosAppStoreIssuerId, iosAppStoreKeyId)(Server API / In-App Purchase pair) but the .p8 came fromgetAppleAscApiKey(ASC private key). The JWT was signed with the ASC private key while itskidclaim was the Server API key id — Apple rejected every such request with 401:Fix: gate on
iosAscKeyIdalone;issuerIdfalls back toiosAppStoreIssuerIdwhen the dedicated ASC issuer slot is empty. Legacy single-slot uploads keep working.2. Stale completion toast on Products tab mount (f290ab1)
The completion toast re-fired every time the operator opened the Products tab when a stale terminal job sat in
productSyncJobs. Replaced the in-memory "have I shown this jobId" set with a transition rule: track previous status per platform, only toast onqueued|running → succeeded|failedfor the same jobId. Fresh observations of an already-terminal job stay silent.3. Subscription Group section had no closing delimiter (b6dc03c)
groupRowsByHierarchycorrectly partitioned Consumables / NonConsumables into ungrouped, but the table rendered them straight after the subscription cluster with no visual delimiter — Consumables looked like part of the subscription group. Added an explicit "Other products" header before the ungrouped section when at least one subscription group renders above.4. Dev launch.json baked PROD URLs into Hono's static fallback (bdf7c9d)
The kit dev launch booted Vite (5173), Hono (3000), and Convex in parallel. Hono serves the SPA out of
./dist, butbun run buildisvite buildwhich defaults to mode=production and loads.env.productionAFTER.env.local— so the prod Convex URL won and got baked into everydist/assets/*.jsbundle. Visiting localhost:3000 in that state ran the prod GitHub OAuth App and bounced the operator out to kit.openiap.dev — even in incognito, because the redirect target lived in the bundle, not in cookies.Fix: wipe
dist/+node_modules/.vite, then explicitly rebuild withvite build --mode developmentso Hono's static fallback ships a bundle pinned to.env.local(dev Convex URL).5. Kit dev entry on top + colored group headers + session-scoped result banner (c45d1a1)
.vscode/launch.json.sessionTriggeredJobIdsRef: Set<jobId>populated when the operator clicks Sync / Dry-run / Reset from THIS mount. Stale terminal jobs from a previous session (HMR reload, page revisit, sync triggered in another tab) no longer surface as if they had just run.6. SSOT for JobStatusSnapshot (13d3154)
Pull
jobId/statusfield types straight offSyncJob(=Doc<"productSyncJobs">) — schema additions inconvex/schema.tsautomatically widen the local snapshot type instead of silently drifting (Gemini SSOT review).Test plan
npx convex deploy --prod— schema only adds optionalprojects.activeSyncJobIds, no migration needed🤖 Generated with Claude Code