feat: [SDK-4334] add Appium-based E2E test workflow for React Native#1942
feat: [SDK-4334] add Appium-based E2E test workflow for React Native#1942fadi-george merged 20 commits intomainfrom
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a large PR (46 files) adding meaningful CI infrastructure — new E2E workflow, iOS codesigning, demo app refactor, and build config changes — that warrants a human look before merging.
Extended reasoning...
Overview
The PR adds an Appium-driven E2E workflow wired into a shared OneSignal/sdk-shared reusable workflow. It touches: new GHA workflow and composite action, iOS ExportOptions.plist and project settings (ccache, manual codesigning), Android arch restriction to arm64-v8a only, and a broad demo app refactor removing LogManager/LogView in favor of Toast snackbars with testID attributes throughout.
Security Risks
The workflow uses secrets: inherit when calling the external OneSignal/sdk-shared reusable workflow. This is standard practice within an org's shared-repo model but means any compromise of that repo could receive all secrets. The API key and app ID are correctly injected from GitHub secrets, not hardcoded. The ExportOptions.plist contains a public Team ID which is benign.
Level of Scrutiny
This PR warrants human review despite being demo/CI-only because: (1) it restricts Android builds to arm64-v8a only, which affects developer experience on non-arm machines; (2) the ccache configuration in the Xcode project settings is subtle and could interact with CI unexpectedly; (3) the PR author explicitly notes provisioning profiles and secrets must be set up as follow-up before the workflow runs green, so a human should confirm the infrastructure plan is sound.
Other Factors
No bugs were found by the automated system. The demo app changes are largely mechanical (testID additions, log replacement) and the intent is clear. However, 46 files across CI infra, native build config, and significant app-layer changes is a meaningful scope that a human reviewer should sign off on.
fadi-george
force-pushed
the
fadi/sdk-4334-use-appium-action-for-react-native
branch
from
d27f13a to
40339b1
Compare
fadi-george
force-pushed
the
fadi/sdk-4334-use-appium-action-for-react-native
branch
from
8b2fd2d to
a2f5843
Compare
fadi-george
force-pushed
the
fadi/sdk-4334-use-appium-action-for-react-native
branch
from
08b8d20 to
9717a54
Compare
fadi-george
force-pushed
the
fadi/sdk-4334-use-appium-action-for-react-native
branch
from
3c38886 to
797af4d
Compare
| # Use this property to specify which architecture you want to build. | ||
| # You can also override it from the CLI using | ||
| # ./gradlew <task> -PreactNativeArchitectures=x86_64 | ||
| reactNativeArchitectures=armeabi-v7a,arm64-v8a,x86,x86_64 |
There was a problem hiding this comment.
For what reason(s) are we dropping support for these? Is this tied to the new architecture work in #1920?
There was a problem hiding this comment.
For demo we only need arm64-v8a to build fast
| // Prefer the Activity captured by ActivityLifecycleTracker (registered via androidx.startup | ||
| // before MainActivity.onResume), then fall back to ReactApplicationContext's accessor and | ||
| // finally the ApplicationContext. Passing the real Activity lets the OneSignal SDK populate | ||
| // ApplicationService.current immediately, so requestPermission() can launch the OS dialog | ||
| // on the first cold-start instead of waiting for the next foreground event. | ||
| Context context = ActivityLifecycleTracker.getInstance().getCurrentActivity(); | ||
| if (context == null) { | ||
| context = reactContext.getCurrentActivity(); | ||
| } |
There was a problem hiding this comment.
Is there any context for this change? Like an issue or is it to get certain tests to pass with Appium?
There was a problem hiding this comment.
There was an existing issue where if you do a fresh-start, then nothing is initialized unless you minimize/close the app and reopen. This fixes it so the app re-uses existing activity when app finally launches
| * real Activity. That in turn leaves {@code ApplicationService.current == null} and queues | ||
| * {@code requestPermission()} until the next foreground. | ||
| */ | ||
| public class ActivityLifecycleTracker implements Application.ActivityLifecycleCallbacks { |
There was a problem hiding this comment.
Is this tied to LiveActivities or something like RN rendering lifecycles?
There was a problem hiding this comment.
See other comment
fadi-george
force-pushed
the
fadi/sdk-4334-use-appium-action-for-react-native
branch
from
a0c2410 to
cc00bf6
Compare
| OneSignal.User.removeEventListener('change', userChangeHandler); | ||
| }; | ||
| }, [fetchUserDataFromApi]); | ||
|
|
||
| const loginUser = async (nextExternalUserId: string) => { | ||
| setAliasesList([]); | ||
| setEmailsList([]); | ||
| setSmsNumbersList([]); | ||
| setTagsList([]); | ||
| setTriggersList([]); | ||
| setExternalUserId(nextExternalUserId); | ||
| setIsLoading(true); | ||
|
|
||
| try { | ||
| OneSignal.login(nextExternalUserId); | ||
| await preferences.setExternalUserId(nextExternalUserId); | ||
| console.log(`Logged in as: ${nextExternalUserId}`); | ||
| // The user 'change' listener runs fetchUserDataFromApi once the new | ||
| // onesignalId is assigned; that call clears isLoading in its finally. | ||
| } catch (err) { | ||
| console.error(`Login error: ${String(err)}`); | ||
| setIsLoading(false); | ||
| } | ||
| }; | ||
|
|
||
| const logoutUser = async () => { |
There was a problem hiding this comment.
🔴 loginUser() calls setIsLoading(true) without incrementing requestSequenceRef.current, so any in-flight fetchUserDataFromApi() call will see its sequence guard pass (requestSequenceRef.current === requestId) and incorrectly call setIsLoading(false), prematurely clearing the loading indicator mid-login. This also allows the in-flight fetch's stale pre-login user data to be written to state. Fix: add requestSequenceRef.current += 1 at the top of loginUser() before setIsLoading(true).
Extended reasoning...
What the bug is and how it manifests
fetchUserDataFromApi() (lines 127–153 of useOneSignal.ts) uses a requestSequenceRef counter to ensure stale responses are discarded. At entry it captures requestId = requestSequenceRef.current + 1 and increments the ref. In the finally block it only calls setIsLoading(false) when requestSequenceRef.current === requestId — this guard is meant to prevent a superseded fetch from clearing the loading state set by a newer request.
loginUser() (lines 300–323) calls setIsLoading(true) but never increments requestSequenceRef.current. This breaks the contract that the ref is always advanced whenever a new "owner" of isLoading is established.
The specific code path that triggers it
- The initial load calls fetchUserDataFromApi(); requestId=1, requestSequenceRef.current=1, isLoading=true.
- While that network request is in flight, the user taps LOGIN USER.
- loginUser() fires: clears state arrays, calls setIsLoading(true), but requestSequenceRef.current stays at 1.
- The in-flight fetch completes; its finally block evaluates requestSequenceRef.current (1) === requestId (1) → true → setIsLoading(false) is called, prematurely clearing the loading indicator.
- Additionally, the try-block stale guard if (requestSequenceRef.current !== requestId) return; also evaluates to false (1 === 1), so stale pre-login aliases, tags, emails, and SMS data is written to state.
- Moments later the SDK fires a user 'change' event; userChangeHandler calls fetchUserDataFromApi() → requestId=2, isLoading=true again.
- Net visible result: isLoading flickers true→false→true and pre-login user data briefly appears in the UI.
Why existing code doesn't prevent it
The requestSequenceRef pattern is already used correctly inside fetchUserDataFromApi — the bug is that loginUser() completely bypasses it. Any caller that sets isLoading=true without advancing the ref implicitly authorizes the last in-flight fetch to cancel the new loading state.
What the impact would be
Loading spinners in the Aliases, Tags, Emails, and SMS sections flicker off and back on during login while a prior fetch is in flight. Worse, pre-login user data is briefly displayed in those sections after the login action is taken, before the post-login fetch completes. For the Appium E2E test suite this PR introduces, any test that asserts loading state or data absence immediately after login could observe this inconsistent transient state.
How to fix it
Add requestSequenceRef.current += 1 as the first line of loginUser(), before setIsLoading(true):
Step-by-step proof
- App cold-starts; load() calls fetchUserDataFromApi(); requestId=1, ref=1, network request begins.
- While network is in flight, user taps LOGIN. loginUser('alice') executes: ref stays at 1, isLoading=true.
- Network response arrives for the anonymous user fetch. finally block: ref(1) === requestId(1) → setIsLoading(false). Loading indicator disappears.
- The try-block guard also passes, so anonymous user's aliases/tags/emails/sms are written to state — briefly displayed under alice's login session.
- SDK fires user 'change'. userChangeHandler → fetchUserDataFromApi(); requestId=2, isLoading=true. Loading reappears.
- With the fix, step 2 increments ref to 2. Step 3's finally sees ref(2) !== requestId(1) → does NOT call setIsLoading(false). The try-block guard also fires correctly. No flicker, no stale data.
| }; | ||
|
|
||
| return ( | ||
| <SectionCard title="Location" onInfoTap={onInfoTap}> | ||
| <SectionCard title="Location" onInfoTap={onInfoTap} sectionKey="location"> |
There was a problem hiding this comment.
🟡 handleCheckLocation in LocationSection.tsx (lines 28–31) is an async function with no try/catch, passed directly to ActionButton.onPress which is typed as () => void. If OneSignal.Location.isShared() throws (SDK not initialized, native error, permissions issue), the returned Promise is silently dropped at the call site, showSnackbar is never called, and the user gets no error feedback. Fix by wrapping the body of handleCheckLocation in try/catch and calling showSnackbar with an error message on failure.
Extended reasoning...
What the bug is and how it manifests
LocationSection.tsx introduces a new handleCheckLocation function (lines 28–31) that is async and has no try/catch block:
const handleCheckLocation = async () => {
const shared = await onCheckLocationShared();
showSnackbar(`Location shared: ${shared}`);
};This function is then passed directly to ActionButton.onPress, which is typed as () => void. TypeScript intentionally allows () => Promise to satisfy () => void structurally (a deliberate TypeScript design decision), so no compile-time error surfaces. The returned Promise is silently dropped when ActionButton calls onPress — and with it, any rejection that might occur.
The specific code path that triggers it
The call chain is: handleCheckLocation → onCheckLocationShared (passed in from HomeScreen as os.checkLocationShared) → checkLocationShared() in useOneSignal.ts (lines 516–520) → OneSignal.Location.isShared(). The last step is a native module call that can throw for several reasons: the SDK was not yet fully initialized, a native bridge exception occurs, or the location subsystem encounters a permissions error. None of handleCheckLocation, onCheckLocationShared, or checkLocationShared have a try/catch.
Why existing code does not prevent it
ActionButton internally calls its onPress prop via TouchableOpacity, which fires the callback synchronously and discards the return value. The returned Promise (and any subsequent rejection) has no handler at the call site. React Native's global unhandled-rejection handler may emit a warning or crash in strict mode, but no user-visible feedback is provided: showSnackbar is only called on the happy path after the await resolves, never in the error path.
What the impact would be
If the native call throws, the user sees no toast, no error message, no loading spinner dismissal — nothing. In E2E testing (which this PR introduces), any Appium test that relies on the snackbar feedback from this button to verify behavior will time out. The failure is entirely silent.
How to fix it
Wrap the body of handleCheckLocation in try/catch, mirroring the pattern used elsewhere in the demo (e.g., handleLogin in UserSection):
const handleCheckLocation = async () => {
try {
const shared = await onCheckLocationShared();
showSnackbar(`Location shared: ${shared}`);
} catch (err) {
showSnackbar('Failed to check location shared status');
console.error('checkLocationShared error:', err);
}
};Step-by-step proof
- User taps CHECK LOCATION SHARED.
- handleCheckLocation() is called fire-and-forget by TouchableOpacity (onPress typed as () => void).
- handleCheckLocation calls await onCheckLocationShared(), which calls OneSignal.Location.isShared().
- OneSignal.Location.isShared() throws (e.g., native bridge not initialized yet).
- The rejection propagates through onCheckLocationShared → handleCheckLocation and out of the async function.
- The Promise returned by handleCheckLocation is discarded by ActionButton's onPress handler — no handler exists.
- showSnackbar is never reached. The user sees nothing. React Native may emit an unhandled-rejection warning in development, but the app produces no user-facing error feedback.
This is the same async-without-try/catch onPress pattern identified in UserSection.handleLogout and LoginModal.onConfirm elsewhere in this PR.
fadi-george
force-pushed
the
fadi/sdk-4334-use-appium-action-for-react-native
branch
2 times, most recently
from
09a9804 to
056581e
Compare
fadi-george
force-pushed
the
fadi/sdk-4334-use-appium-action-for-react-native
branch
from
056581e to
ba9e1d4
Compare
fadi-george
force-pushed
the
fadi/sdk-4334-use-appium-action-for-react-native
branch
from
abd273a to
3060b69
Compare
|
|
||
| const checkLocationShared = async () => { | ||
| const shared = await OneSignal.Location.isShared(); | ||
| console.log(`Location shared: ${shared}`); | ||
| return shared; | ||
| }; |
There was a problem hiding this comment.
🔴 checkLocationShared() in useOneSignal.ts reads the actual SDK value via OneSignal.Location.isShared() and returns it, but never calls setLocationSharedState(shared), leaving the LocationSection ToggleRow bound to stale React state. If the OS or SDK changes location-sharing status externally (e.g., user revokes permission in system settings), pressing CHECK LOCATION SHARED will show the correct SDK value in the snackbar while the toggle still displays the old optimistic state — contradictory UI. Fix: add setLocationSharedState(shared) inside checkLocationShared() after the await.
Extended reasoning...
What the bug is and how it manifests
In useOneSignal.ts (lines 515–520), checkLocationShared() is defined as:
const checkLocationShared = async () => {
const shared = await OneSignal.Location.isShared();
console.log(`Location shared: ${shared}`);
return shared;
};It reads the actual SDK value but never calls setLocationSharedState(shared). By contrast, setLocationShared() at lines ~509–513 correctly calls setLocationSharedState(shared) before the SDK call. This means pressing CHECK LOCATION SHARED returns the real SDK value (which drives the snackbar in LocationSection.tsx), while the locationShared React state driving the ToggleRow is never updated to reflect it.
The specific code path that triggers it
In LocationSection.tsx, handleCheckLocation calls onCheckLocationShared() and passes the result directly to showSnackbar(Location shared: ${shared}). The ToggleRow at line ~36 uses the locationShared prop which originates from React state. If the actual SDK isShared() value diverges from React state — for example because system settings revoke location permission, or the SDK internally resets the value during initialization — the CHECK LOCATION SHARED button will expose the discrepancy rather than fix it: the snackbar says "false", but the toggle continues to show "on".
Why existing code doesn't prevent it
setLocationShared() is correctly implemented: it calls setLocationSharedState(shared) optimistically before the SDK call, so the toggle and SDK stay in sync for all user-initiated changes. The omission in checkLocationShared() means the "check" operation is purely diagnostic — it reports truth from the SDK without reconciling the React state. There is no guard elsewhere that detects the divergence; the ToggleRow will persist showing stale state for the remainder of the session unless the user manually toggles the switch.
Impact
The user sees contradictory information: the snackbar accurately reports Location shared: false while the toggle continues to display the "on" position. For a demo app whose purpose is to verify SDK behavior, this is a meaningful regression. Any Appium E2E test that asserts toggle state after invoking CHECK LOCATION SHARED would observe the stale React state and could fail.
How to fix it
Add setLocationSharedState(shared) after the await in checkLocationShared():
const checkLocationShared = async () => {
const shared = await OneSignal.Location.isShared();
setLocationSharedState(shared); // ← add this line
console.log(`Location shared: ${shared}`);
return shared;
};This ensures the toggle always reflects the actual SDK value after the user presses CHECK LOCATION SHARED, eliminating the snackbar/toggle mismatch.
Step-by-step proof
- User enables location sharing via the toggle →
setLocationShared(true)→setLocationSharedState(true)+ SDKsetShared(true). Toggle shows "on". - System settings (or another mechanism) disables location sharing at the SDK level without going through the React hook.
- User presses CHECK LOCATION SHARED.
checkLocationShared()awaitsOneSignal.Location.isShared()→ returnsfalse.console.log('Location shared: false')runs;return falseis passed back.handleCheckLocationinLocationSection.tsxreceivesfalseand callsshowSnackbar('Location shared: false'). Snackbar shows "false".locationSharedReact state is stilltrue. The ToggleRow continues to show "on".- The UI shows contradictory information for the rest of the session; no further event will correct it.
| OneSignal.User.pushSubscription.optIn(); | ||
| } else { | ||
| OneSignal.User.pushSubscription.optOut(); | ||
| } | ||
| setIsPushEnabled(enabled); | ||
| console.log(enabled ? 'Push enabled' : 'Push disabled'); | ||
| }; | ||
|
|
||
| const sendNotification = async (type: NotificationType) => { | ||
| const success = await postNotification(type); | ||
| console.log(success ? `Notification sent: ${type}` : 'Failed to send notification'); | ||
| }; | ||
|
|
||
| const sendCustomNotification = async (title: string, body: string) => { |
There was a problem hiding this comment.
🟡 In logoutUser(), OneSignal.logout() is called synchronously before await preferences.setExternalUserId(null), creating a split-brain state if AsyncStorage throws: the SDK enters anonymous mode but the stored external user ID is never cleared. On the next cold start, load() reads the stale ID from preferences and calls OneSignal.login(storedExternalUserId), silently re-logging the user in after an explicit logout. Fix by swapping the order — persist the cleared preference before calling OneSignal.logout() — so any storage failure leaves both the SDK and preferences in a consistent logged-in state.
Extended reasoning...
What the bug is and how it manifests
In useOneSignal.ts, logoutUser() executes operations in the wrong order for error safety. OneSignal.logout() is called synchronously as the very first statement, before the async preference write await preferences.setExternalUserId(null). If AsyncStorage throws at that point, the SDK has already entered anonymous mode but the stored external user ID remains intact in persistent storage.
The specific code path that triggers it
logoutUser()is invoked.OneSignal.logout()fires immediately — the SDK is now in anonymous mode.await preferences.setExternalUserId(null)callsAsyncStorage.removeItem('onesignal_external_user_id').- If AsyncStorage throws (storage corruption, quota exceeded, OS-level I/O error), execution halts. The subsequent
setExternalUserId(undefined),setAliasesList([]), etc. never run. - Result: SDK is anonymous, but AsyncStorage still holds the old external user ID.
Why existing code doesn't prevent it
logoutUser() has no try/catch block. Even if it did, the SDK state has already been mutated at step 1. There is no rollback mechanism — if the preference write fails, there is no code path that calls OneSignal.login(previousId) to restore consistency.
What the impact would be
On the next cold start, load() reads storedExternalUserId = await preferences.getExternalUserId(). Finding the stale ID, it calls OneSignal.login(storedExternalUserId), re-associating the device with the user who just explicitly logged out. No error is surfaced — the user has no indication this happened. All subsequent tags, aliases, emails, and SMS operations silently target the old user identity.
How to fix it
Swap the order — call await preferences.setExternalUserId(null) first, then OneSignal.logout(). This ensures that if the storage write fails, both the SDK and preferences remain in the logged-in state (consistent). If AsyncStorage succeeds but the SDK logout somehow fails, the user can retry. Alternatively, add a try/catch that calls OneSignal.login(previousExternalUserId) in the catch block to roll back the SDK state if the preference write fails.
Step-by-step proof
- User logs in as 'alice'. On cold start,
load()reads 'alice' from preferences, callsOneSignal.login('alice'). SDK and preferences are both associated with 'alice'. - User taps LOGOUT USER.
handleLogout()→logoutUser()fires. OneSignal.logout()executes — SDK enters anonymous mode immediately.await preferences.setExternalUserId(null)callsAsyncStorage.removeItem. AsyncStorage encounters an I/O error and throws.- Execution exits
logoutUser()without clearing any state. AsyncStorage still contains 'alice'. - User restarts the app.
load()readsstoredExternalUserId = 'alice'from preferences, callsOneSignal.login('alice'). - The user is now silently re-logged in as 'alice', defeating the explicit logout action with no feedback to the user.
fadi-george
deleted the
fadi/sdk-4334-use-appium-action-for-react-native
branch
| OneSignal.User.pushSubscription.getIdAsync(), | ||
| OneSignal.User.pushSubscription.getOptedInAsync(), | ||
| ]); | ||
| setPushSubscriptionId(id ?? undefined); | ||
| setIsPushEnabled(optedIn); | ||
| }; | ||
|
|
||
| const permissionHandler = async () => { | ||
| setHasNotificationPermission(await OneSignal.Notifications.getPermissionAsync()); | ||
| }; | ||
|
|
||
| const userChangeHandler = (event: UserChangedState) => { |
There was a problem hiding this comment.
🔴 Both pushSubHandler (lines 187–194) and permissionHandler (lines 196–198) are declared as async functions and registered as SDK event listeners, but the SDK discards the returned Promise without attaching .catch() — so if getIdAsync(), getOptedInAsync(), or getPermissionAsync() rejects, the state setters (setPushSubscriptionId, setIsPushEnabled, setHasNotificationPermission) are never called, leaving push and permission UI stuck at stale values. Add try/catch inside each handler body, or prefix each call with void handler().catch(console.error).
Extended reasoning...
What the bug is and how it manifests
In useOneSignal.ts (lines 187–198), pushSubHandler and permissionHandler are both declared as async arrow functions and registered as SDK event listeners:
const pushSubHandler = async () => {
const [id, optedIn] = await Promise.all([
OneSignal.User.pushSubscription.getIdAsync(),
OneSignal.User.pushSubscription.getOptedInAsync(),
]);
setPushSubscriptionId(id ?? undefined);
setIsPushEnabled(optedIn);
};
const permissionHandler = async () => {
setHasNotificationPermission(await OneSignal.Notifications.getPermissionAsync());
};Both are then wired up via addEventListener. When the SDK fires these events, it calls the handlers synchronously as callbacks and discards the returned Promise — there is no .catch() attached and no void operator used.
The specific code path that triggers it
EventManager.ts (line 131) dispatches events via dispatchHandlers(), which calls handler(payload) with no await and no Promise handling. So any rejection from Promise.all([getIdAsync(), getOptedInAsync()]) inside pushSubHandler, or from getPermissionAsync() inside permissionHandler, propagates into an unhandled Promise rejection. The state setters (setPushSubscriptionId, setIsPushEnabled, setHasNotificationPermission) are on the other side of the awaits, so they are never called in the rejection path. One verifier confirmed that getPermissionAsync() can explicitly return Promise.reject(new Error('OneSignal native module not loaded')) — making this a realistic failure mode, not just a theoretical one.
Why existing code does not prevent it
The file already demonstrates the correct pattern: void load().catch((err) => { ... }) in the useEffect body. That convention is followed for the outer async initialization but was not applied to these two inner handlers. There is no try/catch inside either handler body and no .catch() at the call site.
What the impact would be
When a rejection occurs, the Push section's displayed subscription ID, enabled state, and permission indicator are stuck at their last-known values with no error indication to the user or developer. In React Native, unhandled promise rejections produce LogBox warnings in development and can cause crashes in strict-mode configurations. For the Appium E2E test suite this PR introduces, any test that waits on push subscription or permission state after an OS permission dialog is dismissed could time out if getPermissionAsync() rejects on the first call.
How to fix it
Wrap each handler body in try/catch:
const pushSubHandler = async () => {
try {
const [id, optedIn] = await Promise.all([...]);
setPushSubscriptionId(id ?? undefined);
setIsPushEnabled(optedIn);
} catch (err) {
console.error('pushSubHandler error:', err);
}
};
const permissionHandler = async () => {
try {
setHasNotificationPermission(await OneSignal.Notifications.getPermissionAsync());
} catch (err) {
console.error('permissionHandler error:', err);
}
};Alternatively, keep the async body and prefix the registration call with void: void pushSubHandler().catch(console.error) — but wrapping inside is cleaner for locally-defined handlers.
Step-by-step proof
- OneSignalProvider mounts; load() completes and registers pushSubHandler and permissionHandler as SDK listeners.
- The user dismisses the OS push-permission dialog. The SDK fires a 'permissionChange' event.
- permissionHandler() is called by the SDK. The returned Promise is immediately discarded (no await, no .catch).
- getPermissionAsync() rejects (e.g., native module not loaded).
- The rejection propagates out of permissionHandler and becomes an unhandled promise rejection.
- setHasNotificationPermission is never called. The permission toggle in PushSection continues to show the pre-dialog value.
- React Native emits an unhandled rejection warning; in strict mode this may terminate the JS thread.
| const logoutUser = async () => { | ||
| OneSignal.logout(); | ||
| await preferences.setExternalUserId(null); | ||
| setExternalUserId(undefined); | ||
| setAliasesList([]); | ||
| setEmailsList([]); | ||
| setSmsNumbersList([]); | ||
| setTagsList([]); | ||
| setTriggersList([]); | ||
| console.log('Logged out'); | ||
| }; |
There was a problem hiding this comment.
🔴 In logoutUser() (lines 325–335), the request-sequence counter (requestSequenceRef) is never incremented before clearing UI state, so any fetchUserDataFromApi() call that was in-flight at logout time will pass the guard at line 141 and overwrite the just-cleared anonymous state with the previous user's stale data. Fix: add requestSequenceRef.current += 1 as the first line of logoutUser().
Extended reasoning...
Bug description
fetchUserDataFromApi() (lines 127–153) uses a sequence-counter pattern to cancel superseded requests: it increments requestSequenceRef.current, captures the new value as requestId, and then, before committing any state writes, checks if (requestSequenceRef.current \!== requestId) return; (line 141). This guard only works if something increments the ref to invalidate the outstanding requestId. logoutUser() clears every UI-state array (lines 328–333) but never touches requestSequenceRef.current, leaving the guard permanently passable for any in-flight fetch.
Concrete race scenario
- The SDK fires a
userChangeevent (e.g. an alias or tag was added), causinguserChangeHandlerto callfetchUserDataFromApi(). It capturesrequestId = Nand begins three sequential awaits:getOnesignalId(),apiService.fetchUser(),getExternalId(). - While those network calls are in progress, the user taps LOGOUT.
logoutUser()callsOneSignal.logout()and immediately zeros out all state arrays (setAliasesList([]),setTagsList([]), etc.). - The SDK fires another
userChangeevent for the logout transition;userChangeHandlerseesnextOnesignalId === nullat line 204 and returns early — no newfetchUserDataFromApi()call is made, sorequestSequenceRef.currentremains atN. - The original in-flight fetch completes. It reaches the guard at line 141:
requestSequenceRef.current (N) === requestId (N)— the check passes. Lines 143–147 then writesetAliasesList,setTagsList,setEmailsList,setSmsNumbersList, andsetExternalUserIdwith the prior user's data, replacing the clean anonymous statelogoutUser()just established.
Why existing code does not prevent this
The sequence guard at line 141 is the sole cancellation mechanism. Because logoutUser() skips the increment, it cannot invalidate any outstanding fetch. The userChangeHandler early-return for null onesignalId (line 204) means the logout event path never triggers a counter-bumping call either; there is simply no other code path that increments the ref during logout.
Impact
After logout the UI shows the previous user's aliases, tags, email addresses, and SMS numbers instead of an empty anonymous profile. In an Appium E2E test (the context this PR specifically adds), a logout assertion will intermittently fail depending on network latency, making the test flaky. In a real app, this is a user-data leak between accounts.
Fix
Add one line at the top of logoutUser():
const logoutUser = async () => {
requestSequenceRef.current += 1; // invalidate any in-flight fetchUserDataFromApi
OneSignal.logout();
...
};This mirrors how fetchUserDataFromApi() itself invalidates earlier calls and is the minimal, sufficient correction.
| const MASK_CHAR = '•'; | ||
|
|
||
| function maskValue(value: string): string { | ||
| if (E2E_MODE === 'true') { | ||
| return MASK_CHAR.repeat(value.length); | ||
| } | ||
| return value; | ||
| } |
There was a problem hiding this comment.
🟡 MASK_CHAR and maskValue are defined identically in both AppSection.tsx and PushSection.tsx, creating a DRY violation in test-critical masking logic. If the two copies drift (e.g., one gets a value.slice(-4) tweak for debugging), E2E assertions against app_id_value and push_id_value could silently diverge. Extract the shared logic to src/utils/maskValue.ts, following the showSnackbar.ts pattern already established in this PR.
Extended reasoning...
The PR introduces Appium-based E2E testing and, to prevent sensitive IDs from leaking into test logs/screenshots, adds a maskValue() helper that replaces characters with • when E2E_MODE === 'true'. This helper — along with its MASK_CHAR constant — is copy-pasted verbatim into two sibling components: AppSection.tsx (lines 9-16) and PushSection.tsx (lines 10-17). The implementations are byte-for-byte identical today, but they are maintained independently.
The specific code path is: AppSection renders app_id_value testID with maskValue(appId), and PushSection renders push_id_value testID with maskValue(pushId). The Appium test suite asserts against these testIDs. Both rely on the same masking contract, yet neither copy is authoritative — a change to one does not automatically propagate to the other.
The existing code does not prevent future drift. There is no shared import, no type alias, and no lint rule enforcing the two to stay in sync. A developer adding a showLast4 option, changing the bullet character, or toggling masking only in one file would silently break the other section's E2E assertions while leaving that section's unit tests green (assuming they mock E2E_MODE).
The impact is E2E flakiness that is hard to diagnose: one masked value passes the Appium assertion and another fails, the difference tracing back to a seemingly unrelated one-line change in a sibling file. Because this code exists solely to support E2E infrastructure — not product logic — reliability is especially important.
The fix is straightforward: create examples/demo/src/utils/maskValue.ts exporting the maskValue function (and optionally MASK_CHAR), then replace both local definitions with a single import. This is exactly the pattern the PR already uses for showSnackbar.ts, so it is consistent with the PR's own architecture and requires minimal effort.
Step-by-step proof of divergence risk: (1) A developer later wants to show the last 4 characters of the App ID for debugging — they edit AppSection.tsx so maskValue returns '•'.repeat(value.length - 4) + value.slice(-4). (2) They don't touch PushSection.tsx. (3) The Appium test for app_id_value now expects a partially-unmasked value, but the test for push_id_value still expects fully-masked output. (4) Both tests were written against the original spec, so one of them fails — or worse, the test is updated for App ID but the discrepancy in Push ID goes unnoticed until a later CI run on a real device.
2 participants
Description
One Line Summary
Adds an Appium-driven E2E test workflow for the React Native demo app, with signed Android and iOS builds wired into the shared
OneSignal/sdk-sharedAppium runner.rn_android.mov
rn_ios_720.mov
Details
Motivation
We need automated end-to-end coverage for the React Native SDK to catch regressions across native + JS layers before release. This brings the React Native repo in line with the Flutter SDK, which already runs on the shared Appium workflow, and gives us a single place to keep the demo app and its E2E setup honest on every
rel/**push.Scope
e2e.ymlthat builds the demo for Android and iOS and hands artifacts off toOneSignal/sdk-shared/.github/workflows/appium-e2e.yml@main.setup-demothat installs Bun + Vite+, sets up the demo, optionally updates pods, and writes the demo.env.OneSignal/sdk-shared/.github/actions/setup-ios-demo-codesigning@mainand exports a signed IPA using a newexamples/demo/ios/ExportOptions.plist. Adds anaps-environmententitlement check so we fail fast if push subscription would be broken.LogView/LogManager),E2E_MODEflag plumbed through.env, and minor build tweaks (arm64-v8a only on Android, ccache enabled, iOS DerivedData + Pods caching).AppHeadercomponent for a consistent JS-rendered header with a real drop shadow on both platforms (the native-stack header was ignoring shadow styling), clearer action-button labels, an icon on the Add Row button, and standardized testIDs for multi-pair rows.react-native-onesignal@5.4.3.Not changed: the SDK source, native module APIs, or any consumer-facing behavior.
Testing
Manual testing
workflow_dispatchon a feature branch to validate APK upload, signed IPA export, and theaps-environmentverification step.Unit testing
No unit tests added; this PR is CI plumbing and demo-only changes. Existing tests pass.
Affected code checklist
Checklist
Overview
Testing
Final pass
Follow-up needed before this can run green
The new iOS codesigning step expects two provisioning profiles to exist in the OneSignal Apple Developer team that match the RN demo's extension bundle IDs:
com.onesignal.exampleAppium OneSignal Main(already exists, shared with Flutter)com.onesignal.example.OneSignalNotificationServiceExtensionRNAppium OneSignal NSE RNcom.onesignal.example.OneSignalWidgetExtensionAppium OneSignal Widget RNRepo also needs the same Appium / App Store Connect secrets the Flutter workflow uses:
APPIUM_IOS_DEV_CERT_P12_BASE64,APPIUM_IOS_DEV_CERT_PASSWORD,APPIUM_APP_STORE_CONNECT_KEY_ID,APPIUM_APP_STORE_CONNECT_ISSUER_ID,APPIUM_APP_STORE_CONNECT_PRIVATE_KEY, plusAPPIUM_ONESIGNAL_APP_ID(var) andAPPIUM_ONESIGNAL_API_KEY(secret).Made with Cursor