Add SetCustomerUserId/ClearCustomerUserId for the post-Configure() case - #8
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Unity SDK adds public methods to set and clear customer user IDs after configuration. Android and iOS bridges forward values and clear markers to native SDKs. Editor, Android, and iOS tests validate normalization, ordering, errors, and platform behavior. ChangesCustomer user ID lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Unity as AppstackSDK
participant Native as AppstackSDKNative
participant Bridge as Android/iOS bridge
participant SDK as Native attribution SDK
Unity->>Native: SetCustomerUserId(trimmed ID or empty marker)
Native->>Bridge: Forward customer user ID
Bridge->>SDK: Set value or clear with null/nil
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Bridges the native iOS/Android setCustomerUserId setter through the C# facade, the bridge seam, and both native bridges, so the id can be set or cleared after Configure() — a repeat Configure() is a no-op that ignores its customerUserId. Clear semantics: C# cannot marshal a null string cleanly, so the empty string is the clear marker on the setCustomerUserId entry points, where it is unambiguous (on Configure it means "not provided" instead, and never clears). Both native bridges map empty to null/nil, which the native setters treat as the clear. ClearCustomerUserId() is a Unity-only spelling of SetCustomerUserId(null) for readable logout call sites; it adds no behavior the other wrappers lack, and the contract note in DEVELOPMENT.md records the decision.
a81e22a to
9497906
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@Runtime/AppstackSDK.cs`:
- Around line 103-110: Update the class-level AppstackSDK summary to state that
ClearCustomerUserId is a Unity-specific convenience wrapper, while preserving
the existing Flutter and React Native API parity description where applicable.
- Around line 79-85: Align the Configure lifecycle documentation with the
supported pre-configuration behavior: update Configure’s documentation to
explicitly exempt SetCustomerUserId and ClearCustomerUserId, while preserving
their safe-at-any-time semantics and the existing README.md and USAGE.md
guidance.
In `@Runtime/Plugins/Android/com/appstack/unity/AppstackUnityBridge.java`:
- Around line 57-60: Keep the iOS bridge blocked until the production SDK
exposes the customer-user-ID setter: in
Runtime/Plugins/iOS/AppstackUnityBridge.swift:40-47, update the dependency to a
compatible SDK or defer AppstackUnitySetCustomerUserId; no direct change is
required in
Runtime/Plugins/Android/com/appstack/unity/AppstackUnityBridge.java:57-60
because the resolved Android AAR already provides
AppstackAttributionSdk.setCustomerUserId(String).
In `@Tests`~/Native/iOS/run-tests.sh:
- Line 54: Update the iOS test archive reference and production AppstackSDK
package version together to a revision that exposes
AppstackAttributionSdk.setCustomerUserId, ensuring the
AppstackUnitySetCustomerUserId bridge compiles before the symbol check runs.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 796a390f-b1ba-4e0b-b98a-c974b3c6ae1f
📒 Files selected for processing (19)
CHANGELOG.mdDEVELOPMENT.mdREADME.mdRuntime/AppstackAndroidBridge.csRuntime/AppstackIOSBridge.csRuntime/AppstackSDK.csRuntime/AppstackSDKNative.csRuntime/IAppstackNativeBridge.csRuntime/Plugins/Android/com/appstack/unity/AppstackUnityBridge.javaRuntime/Plugins/iOS/AppstackUnityBridge.swiftTests/Editor/AppstackNativeBridgeSeamTests.csTests/Editor/AppstackPublicApiSnapshotTests.csTests/Editor/AppstackUnsupportedPlatformTests.csTests~/Native/Android/contract-tests/src/main/kotlin/com/appstack/attribution/AppstackAttributionSdk.ktTests~/Native/Android/contract-tests/src/test/kotlin/com/appstack/unity/AppstackUnityBridgeContractTest.ktTests~/Native/iOS/Stubs/AppstackSDK/AppstackSDK.swiftTests~/Native/iOS/Tests/AppstackUnityBridgeTests/AppstackUnityBridgeTests.swiftTests~/Native/iOS/run-tests.shUSAGE.md
📜 Review details
🧰 Additional context used
🪛 LanguageTool
README.md
[locale-violation] ~117-~117: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ... startup. More often a login reveals it afterwards, so set it whenever it becomes known: ...
(AFTERWARDS_US)
🔇 Additional comments (16)
Tests/Editor/AppstackNativeBridgeSeamTests.cs (1)
119-179: LGTM!Also applies to: 354-355, 382-387
Tests/Editor/AppstackPublicApiSnapshotTests.cs (1)
15-16: LGTM!Tests/Editor/AppstackUnsupportedPlatformTests.cs (1)
20-21: LGTM!Tests~/Native/Android/contract-tests/src/main/kotlin/com/appstack/attribution/AppstackAttributionSdk.kt (1)
62-65: LGTM!Also applies to: 95-95, 129-133
Tests~/Native/Android/contract-tests/src/test/kotlin/com/appstack/unity/AppstackUnityBridgeContractTest.kt (1)
75-103: LGTM!Tests~/Native/iOS/Stubs/AppstackSDK/AppstackSDK.swift (1)
47-48: LGTM!Also applies to: 60-60, 89-92
Tests~/Native/iOS/Tests/AppstackUnityBridgeTests/AppstackUnityBridgeTests.swift (1)
80-116: LGTM!Also applies to: 258-263
Runtime/AppstackSDK.cs (2)
16-18: LGTM!
86-94: 🗄️ Data Integrity & IntegrationVerify the any-thread and last-write-wins contract before shipping.
The Unity layer forwards each call without synchronization. The public documentation promises arbitrary-thread calls and deterministic ordering.
Runtime/AppstackSDK.cs#L86-L94: serialize setter, clear, and event operations, or prove native ordering with concurrent tests on both platforms.USAGE.md#L56-L59: retain the any-thread and last-write-wins wording only after those tests pass.Runtime/AppstackSDKNative.cs (1)
21-24: LGTM!Also applies to: 115-119, 161-165, 203-207
Runtime/IAppstackNativeBridge.cs (1)
15-17: LGTM!DEVELOPMENT.md (1)
29-30: LGTM!Also applies to: 50-56
README.md (1)
129-132: 🔒 Security & PrivacyVerify the buffered-event identity claim.
The Unity setter changes native SDK state. It cannot directly rewrite events already buffered by the native SDK. Verify whether the native SDK snapshots
customerUserIdwhen it queues an event or resolves it at flush. If it snapshots the value, this statement is false and logout can misattribute queued events. Add Android and iOS contract tests for events queued before setting or clearing the ID.CHANGELOG.md (1)
19-26: LGTM!Runtime/AppstackAndroidBridge.cs (1)
51-57: LGTM!Runtime/AppstackIOSBridge.cs (1)
30-31: LGTM!Also applies to: 65-69
| /// <summary> | ||
| /// Set — or clear — the customer user ID after <see cref="Configure"/>, e.g. once a | ||
| /// login reveals it. A repeat <see cref="Configure"/> is a no-op, so it cannot be | ||
| /// used to change the ID. Safe to call at any time; last write wins. | ||
| /// </summary> | ||
| /// <param name="customerUserId">Your identifier for the signed-in user. `null`, an | ||
| /// empty string, or whitespace clears it — see <see cref="ClearCustomerUserId"/>.</param> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the Configure lifecycle contract.
Configure still says it must run before any other SDK method. This method says it is safe at any time. README.md and USAGE.md also allow calls before Configure. Choose one contract. If pre-configuration calls are supported, document SetCustomerUserId and ClearCustomerUserId as exceptions in the Configure documentation.
Proposed documentation fix
- /// Must be called before any other SDK methods.
+ /// Must be called before event, status, and attribution methods.
+ /// SetCustomerUserId and ClearCustomerUserId may be called before or after Configure.🤖 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 `@Runtime/AppstackSDK.cs` around lines 79 - 85, Align the Configure lifecycle
documentation with the supported pre-configuration behavior: update Configure’s
documentation to explicitly exempt SetCustomerUserId and ClearCustomerUserId,
while preserving their safe-at-any-time semantics and the existing README.md and
USAGE.md guidance.
| /// <summary> | ||
| /// Clear the stored customer user ID — call this on logout, otherwise the previous | ||
| /// user's ID stays attached to every later event. Equivalent to | ||
| /// <c>SetCustomerUserId(null)</c>. | ||
| /// </summary> | ||
| public static void ClearCustomerUserId() | ||
| { | ||
| SetCustomerUserId(null); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the API parity description.
The class-level documentation says that AppstackSDK has the same API surface as Flutter and React Native. ClearCustomerUserId is documented as Unity-only in DEVELOPMENT.md. Update the class summary to describe the Unity-specific convenience wrapper.
🤖 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 `@Runtime/AppstackSDK.cs` around lines 103 - 110, Update the class-level
AppstackSDK summary to state that ClearCustomerUserId is a Unity-specific
convenience wrapper, while preserving the existing Flutter and React Native API
parity description where applicable.
|
Android pin. #9 moved the pin past #7's iOS artifact. No re-cut is required. The Verification on the merge: 112 editor tests, 36 Android contract tests, and 12 iOS Swift contract tests pass.
|
1 participant
Bridges the native
setCustomerUserIdsetter (iOSAppstackAttributionSdk.shared.setCustomerUserId(_:), AndroidAppstackAttributionSdk.setCustomerUserId) so the customer user ID can be set or cleared afterConfigure(). That is the common case — a login usually reveals the ID — and a secondConfigure()is not an alternative: it is a no-op and ignores itscustomerUserId.The clear-path decision
C# cannot marshal a null string cleanly, and
""already means "not provided" on theConfigurepath (customerUserId?.Trim() ?? ""→emptyToNull), so a Unity clear needed an explicit decision.""is the clear marker, but only on the newsetCustomerUserIdnative entry points. It is unambiguous there because those are separate native functions fromconfigure, so no sentinel gymnastics and no null-string marshalling: C# normalizesnull/blank to"", and the existingemptyToNull(Java) andstring(from:)(Swift) helpers already map it tonull/nil, which the native setters treat as the clear.ClearCustomerUserId()is a Unity-only spelling ofSetCustomerUserId(null)— added because C# logout call sites read better with it. It adds no behavior the other wrappers lack, andDEVELOPMENT.mdrecords it as an explicit cross-wrapper contract decision (that file's rule asks for one). Easy to drop if you would rather keep the surface identical across wrappers.Changes
AppstackSDK.cs,IAppstackNativeBridge.cs,AppstackSDKNative.cs(all three nested bridges, including the unsupported-platform no-op),AppstackAndroidBridge.cs,AppstackIOSBridge.cs, and both native bridges. Tests: the API snapshot, the bridge seam, the unsupported-platform silence check, plus theTests~/Nativecontract fixtures on both platforms and the iOS runner's expected-C-symbol list. README/USAGE/CHANGELOG/DEVELOPMENT updated.Native requirements
Requires iOS SDK
4.5.0and Android SDK1.7.0. Both are pinned onmain(#9), and the iOS contract fixture resolves and checksum-verifies the pinned release artifact (#10).mainis merged into this branch.Verification
real-artifactcompiles the production Java bridge, including the newsetCustomerUserIdcall, againsttech.appstack.android-sdk:appstack-android-sdk:1.7.0resolved from Maven Central.AppstackUnityBridge.swiftcompiles against the checksum-verified4.5.0release artifact, and all eight expected C symbols are present, including the newAppstackUnitySetCustomerUserId.