Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 75 additions & 3 deletions .claude/commands/review-pr.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,50 @@ For each comment:
4. **Reply directly to the inline review comment** (NOT a general PR comment)
5. **Resolve the conversation** via GraphQL API

After the fix batch is pushed (once per round, not per comment), trigger a fresh round from every automated reviewer wired into this repo:

```bash
# Re-request Copilot review (note: capital C; the bot login is literally "Copilot").
# GOTCHA: if Copilot has already submitted a review on an earlier commit, the
# REST POST below returns HTTP 201 but silently leaves `requested_reviewers`
# empty — it's idempotent against reviewers whose prior review is still on
# record, so a plain re-POST does nothing. The reliable workaround is DELETE
# + POST so GitHub treats it as a fresh request.
gh api -X DELETE "repos/hyodotdev/openiap/pulls/$PR_NUMBER/requested_reviewers" \
-f 'reviewers[]=Copilot' >/dev/null 2>&1 || true
sleep 2
gh api -X POST "repos/hyodotdev/openiap/pulls/$PR_NUMBER/requested_reviewers" \
-f 'reviewers[]=Copilot' >/dev/null

# Verify it actually took — GitHub occasionally still drops the re-request
# even after DELETE. If the list is empty, warn so the user can hit "Re-request
# review" in the GitHub UI manually as a last resort (the UI uses a
# privileged endpoint that works even when the API silently no-ops).
if [ -z "$(gh api repos/hyodotdev/openiap/pulls/$PR_NUMBER --jq '.requested_reviewers | map(select(.login == "Copilot")) | .[0].login // empty')" ]; then
echo "WARN: Copilot re-request didn't stick via API; click Re-request review in the GitHub UI if you need it."
fi

# Kick off a new Gemini review pass
gh pr comment "$PR_NUMBER" --body "/gemini review"

# Kick off a new CodeRabbit review pass
gh pr comment "$PR_NUMBER" --body "@coderabbitai review"
```

`/gemini review` and `@coderabbitai review` comments always start new passes. Copilot's bot is flakier — the DELETE+POST dance is the best programmatic option, and the verification step flags the cases where manual intervention is needed so we don't pretend the re-request succeeded when it silently didn't.

## Polling Loop (after fix batch)

The automated reviewers (Copilot + Gemini) need a few minutes to produce feedback. After pushing a round of fixes and posting `/gemini review`, schedule a wake-up in **~480 seconds (8 minutes)** and re-enter `/review-pr $PR_NUMBER` to:

1. Re-fetch unresolved review threads (`gh api repos/{owner}/{repo}/pulls/$PR_NUMBER/comments`).
2. If new unresolved threads exist → fix them, push, post `/gemini review` again, and schedule another 8-minute wake-up.
3. If no new unresolved threads exist → the PR is clean. End the loop and report completion to the user.

Use the `ScheduleWakeup` tool for the wake-up, passing `/review-pr $PR_NUMBER` back as the prompt so the next firing re-enters this skill with full context. Omit the call to stop the loop once all threads are resolved.

Guard against infinite loops: if a reviewer keeps flagging the same finding after two fix attempts, stop scheduling wake-ups and hand back to the user with a summary of what remains disputed.

### Replying to Inline Review Comments

**CRITICAL:** Always reply to inline review comments using the comment-specific reply API, NOT `gh pr comment`.
Expand Down Expand Up @@ -90,9 +134,37 @@ mutation {
```

**Thread Resolution Rules:**
- Only resolve threads where code changes have been made and pushed
- Do not resolve threads that are just suggestions for future improvement
- Do not resolve threads awaiting user clarification
- Resolve threads where code changes have been made and pushed (after posting a reply with the commit hash).
- **Auto-resolve outdated threads only**: GitHub marks a thread as `isOutdated: true` when the underlying code has already shifted out from under the comment, so those findings no longer apply to the current diff. Sweep those without a reply at the start of every round. **Do not** auto-resolve threads just because the last comment is from the author — the reviewer may still need to confirm the fix.
- Do not resolve threads that are just suggestions for future improvement unless explicitly acknowledged.
- Do not resolve threads awaiting user clarification.

Outdated sweep (run once per round before fetching open findings):

```bash
PR_NUMBER=...
gh api graphql -f query='
query($owner:String!,$name:String!,$pr:Int!) {
repository(owner:$owner, name:$name) {
pullRequest(number:$pr) {
reviewThreads(first:100) {
nodes { id isResolved isOutdated }
}
}
}
}' -F owner=hyodotdev -F name=openiap -F pr=$PR_NUMBER --jq '
.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false)
| select(.isOutdated == true)
| .id' | while read tid; do
[ -n "$tid" ] && gh api graphql -f query='
mutation($id:ID!) {
resolveReviewThread(input:{threadId:$id}) { thread { id isResolved } }
}' -F id="$tid" >/dev/null && echo "auto-resolved outdated $tid"
done
```

Threads that the author has already replied to still show up in the "unresolved" list on the next round — that is intentional so the reviewer can confirm the fix landed and either agree (resolve manually / mark fixed) or push back. Resolving them as soon as the author replies would silence legitimate follow-up feedback.

## Reply Format Rules (CRITICAL)

Expand Down
1 change: 1 addition & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ openiap/
**CRITICAL**: Before writing or editing anything in a package or library:

1. **Read the relevant knowledge files** from `knowledge/internal/`
- When the GraphQL schema adds or changes an API, follow the **SDK Parity Checklist** in [`knowledge/internal/04-platform-packages.md`](knowledge/internal/04-platform-packages.md#sdk-parity-checklist-critical--prevents-declared-but-not-implemented) to avoid phantom interfaces (declared in types but never wired end-to-end — the class of bug behind GitHub issue #104).
2. **Check the package-specific CONVENTION.md**:
- [`packages/gql/CONVENTION.md`](packages/gql/CONVENTION.md)
- [`packages/google/CONVENTION.md`](packages/google/CONVENTION.md)
Expand Down
77 changes: 77 additions & 0 deletions knowledge/internal/04-platform-packages.md
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,83 @@ swift build # Verifies ObjC bridge compiles

---

## SDK Parity Checklist (CRITICAL — prevents "declared but not implemented")

When the GraphQL schema in [`packages/gql`](../../packages/gql) adds or changes an API, the regenerated `types.*` files **declare** the handler but do not **implement** it. Every wrapper library must wire the new API end-to-end or users will see silent nulls, phantom interfaces (GitHub issue #104), or `UnsupportedOperationException` at runtime.

### The bug pattern

A symptom like "interface exists in `types.dart` / `types.ts` / `Types.kt` but calling it does nothing / throws" means one or more of these layers is missing:

```text
GraphQL schema ─► generated types ─► public API ─► native bridge ─► core module impl
(SSOT) (auto-generated) (hand-written) (hand-written) (shared Swift/Kotlin)
▲ ▲ ▲
│ │ │
must match must be exported must dispatch
```

### Per-library completion checklist

For every new/changed handler in the generated types, verify **all five** of these per target library before considering the change shippable:

| Library | 1. Type declared | 2. Public API exposed | 3. Platform bridge | 4. Wired into handlers bundle | 5. Test coverage |
|---------|------------------|-----------------------|--------------------|-------------------------------|------------------|
| **react-native-iap** | `src/types.ts` (generated) | `src/index.ts` export (Nitro or composed TS) | `ios/HybridRnIap.swift` (iOS), `android/.../HybridRnIap.kt` (Android) | Not required (flat exports) | Mock stub in all 4 `mockIap` objects in `__tests__/` (per memory) |
| **expo-iap** | `src/types.ts` (generated) | `src/modules/ios.ts` / `android.ts` export, re-exported from `src/index.ts` | `ios/ExpoIapModule.swift` `AsyncFunction`, `android/.../ExpoIapModule.kt` | Not required (flat exports) | `src/modules/__tests__/*.test.ts` |
| **flutter_inapp_purchase** | `lib/types.dart` (generated) | getter on `FlutterInappPurchase` in `lib/flutter_inapp_purchase.dart` | `case "<name>":` in `ios/Classes/FlutterInappPurchasePlugin.swift`, Android plugin `onMethodCall` | `queryHandlers` / `mutationHandlers` / `subscriptionHandlers` bundles near the bottom of `flutter_inapp_purchase.dart` | Mock + test in `test/ios_methods_test.dart` (and the `errors_unit_test.dart` error-mapping test) |
| **kmp-iap** | `library/src/commonMain/.../openiap/Types.kt` (generated interface) | exposed via `KmpInAppPurchase` / `kmpIapInstance` | `library/src/iosMain/.../InAppPurchaseIOS.kt` — must call `openIapModule.<name>WithCompletion { ... }`, **never** `throw UnsupportedOperationException` | Not required (interface dispatch) | `library/src/commonTest/` if testable cross-platform |
| **godot-iap** | `addons/godot-iap/types.gd` (generated) | public `snake_case` function in `addons/godot-iap/godot_iap.gd` | `ios-gdextension/Sources/GodotIap/GodotIap.swift` (iOS), `android/src/main/java/.../GodotIap.java` (Android) | Not required | Manual testing — no automated test suite yet |

### Platform suffix rule (who needs what)

The suffix on the handler name tells you which native bridges are required:

- **`…IOS` suffix** → iOS bridge only. Non-iOS platforms should return the type's zero value (`false`, `null`, empty list) or throw a documented `PlatformException` for void ops. **Do not** wire into Android bridges.
- **`…Android` suffix** → Android bridge only. Same rule in reverse.
- **No suffix** → both iOS and Android bridges required.

Wiring an iOS-suffixed method into an Android bridge is a bug — the earlier audit agents produced false positives like this.

### Common failure modes observed in the codebase

1. **Phantom interface** (GitHub issue #104, Flutter `beginRefundRequestIOS` pre-2026-04): generated type exists, nothing else does. Users see an uncallable interface.
2. **`UnsupportedOperationException` stub** (KMP pattern): method declared, iOS impl deliberately throws with "not implemented in OpenIAP". Usually a stale stub — the ObjC bridge method may already exist. Always `grep OpenIapModule+ObjC.swift` for `<name>With*` before assuming the bridge is missing.
3. **Channel-name drift** (Flutter `getAppTransactionIOS` pre-2026-04): Dart calls `_channel.invokeMethod('getAppTransaction')` but the Swift plugin only handles `"getAppTransactionIOS"` (or vice versa). Mocked tests passed because the test intercepted the wrong name too.
4. **Handler bundle omission** (Flutter): Dart getter exists, Swift bridge exists, but the new handler is not listed in `queryHandlers` / `mutationHandlers`. Consumers using the generated handler bundle (e.g., for cross-platform dispatch) silently miss the API.

### Audit command for a new handler

After regenerating types, run for each library:

```bash
# Replace <name> with the new handler name (camelCase, e.g., beginRefundRequestIOS)
NAME=<name>

echo "=== Type declared? ==="
rg -n "$NAME" \
libraries/*/lib/types.dart \
libraries/*/src/types.ts \
libraries/kmp-iap/library/src/commonMain/kotlin \
libraries/*/addons/godot-iap/types.gd

Comment thread
coderabbitai[bot] marked this conversation as resolved.
echo "=== Public API exposed? ==="
rg -n "^export (const|async function|function) $NAME\b|get $NAME\b|func $NAME\b|snake_case equivalent" libraries/

echo "=== Native bridge? ==="
rg -n "\"$NAME\"|\.$NAME\b" libraries/*/ios libraries/*/android libraries/*/ios-gdextension

echo "=== Wired into handlers bundle? (Flutter only) ==="
rg -n "$NAME:" libraries/flutter_inapp_purchase/lib/flutter_inapp_purchase.dart

echo "=== Throws stub? ==="
rg -n "UnsupportedOperationException.*$NAME" libraries/
```

Any empty result for a layer that *should* have the handler (per the suffix rule) is a gap that must be filled before merging.

---

## Google Package (packages/google)

### Required Pre-Work (Google)
Expand Down
31 changes: 31 additions & 0 deletions libraries/expo-iap/src/modules/android.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,37 @@ export const validateReceiptAndroid = async ({
return response.json();
};

/**
* Consume a purchase token so the user can purchase the same product again
* (Android consumable products). Prefer using `finishTransaction` with
* `isConsumable: true`, which dispatches to this under the hood.
*/
export const consumePurchaseAndroid: MutationField<
'consumePurchaseAndroid'
> = async (purchaseToken) => {
const result = await ExpoIapModule.consumePurchaseAndroid(purchaseToken);

if (typeof result === 'boolean') {
return result;
}

if (result && typeof result === 'object') {
const record = result as Record<string, unknown>;
if (typeof record.success === 'boolean') {
return record.success;
}
if (typeof record.responseCode === 'number') {
return record.responseCode === 0;
}
}

throw new Error(
`consumePurchaseAndroid returned an unexpected response payload: ${JSON.stringify(
result,
)}`,
);
};
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/**
* Acknowledge a product (on Android.) No-op on iOS.
* @param {Object} params - The parameters object
Expand Down
16 changes: 16 additions & 0 deletions libraries/expo-iap/src/modules/ios.ts
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,22 @@ export const getReceiptDataIOS: QueryField<'getReceiptDataIOS'> = async () => {

export const getReceiptIOS = getReceiptDataIOS;

/**
* Get the current App Store storefront country code on iOS.
*
* @deprecated Use cross-platform `getStorefront` from the main index instead.
* The native module exposes a single `getStorefront` AsyncFunction that already
* resolves to the iOS storefront on iOS. This helper is kept as an iOS-only
* alias so consumers who previously imported `getStorefrontIOS` do not break.
*
* @returns {Promise<string>} ISO 3166-1 alpha-2 country code (e.g. "US")
*
* @platform iOS
*/
export const getStorefrontIOS: QueryField<'getStorefrontIOS'> = async () => {
return ExpoIapModule.getStorefront();
};

/**
* Refresh the receipt data from Apple's servers and return the updated receipt.
* This calls AppStore.sync() before reading the receipt, ensuring the latest
Expand Down
Loading
Loading