fix(hot): skip revoked transactions on subs upgrade#19
Conversation
WalkthroughAdds conditional checks in the transaction processing loop to skip revoked transactions and already-processed transactions, with debug logging. When skipping already-processed transactions, the code also removes them from the processed set. Unchanged behavior for unprocessed transactions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant OpenIapModule
participant StoreKit as StoreKit Transaction
App->>OpenIapModule: processTransactions()
loop For each transaction
OpenIapModule->>StoreKit: read transaction
alt Revoked (revocationDate != nil)
OpenIapModule-->>App: skip with debug log
else Already processed
OpenIapModule->>OpenIapModule: remove from processed set
OpenIapModule-->>App: skip with debug log
else Unprocessed
OpenIapModule->>OpenIapModule: process transaction (existing flow)
OpenIapModule-->>App: continue
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Sources/OpenIapModule.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Sources/OpenIapModule.swift (1)
Sources/Helpers/IapState.swift (1)
isProcessed(26-26)
🔇 Additional comments (1)
Sources/OpenIapModule.swift (1)
841-845: LGTM! Correct handling of revoked transactions.The check for
revocationDateappropriately skips revoked transactions that occur during subscription upgrades. This prevents processing obsolete transactions and the debug logging aids troubleshooting.
| if await self.state.isProcessed(transactionId) { | ||
| OpenIapLog.debug("⏭️ Skipping already processed transaction: \(transactionId)") | ||
| // Remove from processed set for future updates (e.g., subscription renewals) | ||
| await self.state.unmarkProcessed(transactionId) | ||
| continue | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the unmarking logic for already-processed transactions.
Unmarking a transaction immediately when skipping it (line 850) could lead to duplicate processing if the same transaction appears again shortly after. The flow becomes:
- First appearance: Transaction processed and marked
- Second appearance: Found processed → unmarked and skipped
- Third appearance: Not marked anymore → processed again (duplicate)
The comment mentions "subscription renewals," but renewals generate new transaction IDs rather than reusing existing ones. Additionally, lines 859-862 already handle unmarking after a 5-second delay for legitimate reprocessing scenarios.
Expected behavior: If a transaction is already processed, it should be skipped without unmarking (the 5-second timer handles unmarking). Alternatively, if the intent is to reprocess on subsequent appearances, the transaction should be unmarked and then processed (not skipped).
Consider one of these approaches:
Option A: Remove the unmarkProcessed call to preserve the original skip behavior:
if await self.state.isProcessed(transactionId) {
OpenIapLog.debug("⏭️ Skipping already processed transaction: \(transactionId)")
- // Remove from processed set for future updates (e.g., subscription renewals)
- await self.state.unmarkProcessed(transactionId)
continue
}Option B: If reprocessing is intentional, unmark and process (don't skip):
if await self.state.isProcessed(transactionId) {
OpenIapLog.debug("⏭️ Reprocessing transaction: \(transactionId)")
await self.state.unmarkProcessed(transactionId)
- continue
+ // Fall through to process the transaction again
}Please verify which behavior is correct for your use case. Run the following script to check if there are any existing tests that document the expected behavior:
🏁 Script executed:
#!/bin/bash
# Search for tests related to transaction processing and duplicate handling
rg -n --type swift -C 3 'isProcessed|markProcessed|unmarkProcessed|processUnfinished'Length of output: 3986
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if await self.state.isProcessed(transactionId) { | |
| OpenIapLog.debug("⏭️ Skipping already processed transaction: \(transactionId)") | |
| // Remove from processed set for future updates (e.g., subscription renewals) | |
| await self.state.unmarkProcessed(transactionId) | |
| continue | |
| } | |
| if await self.state.isProcessed(transactionId) { | |
| OpenIapLog.debug("⏭️ Skipping already processed transaction: \(transactionId)") | |
| continue | |
| } |
🤖 Prompt for AI Agents
In Sources/OpenIapModule.swift around lines 847 to 852, remove the immediate
call to await self.state.unmarkProcessed(transactionId) inside the if-block that
skips already processed transactions; simply keep the skip logging and continue,
relying on the existing 5‑second timer to unmark processed IDs instead of
unmarking before continue to avoid re-processing duplicates.
1 participant
Related hyochan/react-native-iap#3054