chore: [SDK-4406] prepare Unity demo app for Appium E2E tests#869
Conversation
Row TextField names were derived from `_rows.Count` at construction time, but RemoveRow does not renumber survivors. After Add+Remove(middle)+Add, the new row's name collided with an existing survivor's name, and AccessibilityBridge.WalkAndUpsert dedupes by name (first-wins) — so the new row was silently dropped from the a11y tree and Appium taps landed on the stale row. Switch to a monotonic `_nextRowIndex++` so names are permanently unique regardless of remove ordering. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@claude review |
| case NotificationType.WithSound: | ||
| title = "Sound Notification"; | ||
| body = "This notification plays a custom sound"; | ||
| extra = new JObject | ||
| { | ||
| ["ios_sound"] = "vine_boom.wav", | ||
| ["android_channel_id"] = "b3b015d9-c050-4042-8548-dcc34aa44aa4", | ||
| }; | ||
| break; |
There was a problem hiding this comment.
🟡 The new WithSound notification payload sets ios_sound to vine_boom.wav, but no such file exists in the repo (only notification.wav under Android raw resources) and no iOS build post-processor copies a sound asset into the Xcode project. iOS requires the named UNNotificationSound resource to exist in the app bundle; when absent it silently falls back to the default system sound, so the new WITH SOUND button cannot deliver its advertised custom sound on iOS. Either bundle the .wav under Assets/Plugins/iOS/ and wire it through BuildPostProcessor (AddFile + AddFileToBuildSection on the main target), or remove the WithSound option until the asset pipeline is in place.
Extended reasoning...
What the bug is
OneSignalApiService.SendNotification (OneSignalApiService.cs:56-64) now handles a new NotificationType.WithSound case that builds a payload setting:
case NotificationType.WithSound:
title = "Sound Notification";
body = "This notification plays a custom sound";
extra = new JObject
{
["ios_sound"] = "vine_boom.wav",
["android_channel_id"] = "b3b015d9-c050-4042-8548-dcc34aa44aa4",
};
break;OneSignal's ios_sound parameter names a sound file that must be packaged in the app bundle as a UNNotificationSound resource. If the named file is not in the bundle, iOS does not raise an error — it silently falls back to the default system sound.
Why it manifests
A repo-wide search for vine_boom finds the string literal only at OneSignalApiService.cs:61. The only audio assets present anywhere in the repository are notification.wav under com.onesignal.unity.android/Editor/OneSignalConfig.androidlib/src/main/res/raw/ (and its mirror under examples/demo/Assets/Plugins/Android/...). There is no vine_boom.wav (or any other .wav / .caf / .aiff) in examples/demo/Assets/Plugins/iOS/, StreamingAssets, or the iOS plugin folder.
The new SigningPostProcessor only touches entitlements / bundle IDs / signing, and BuildPostProcessor only handles the Live Activities widget extension and its Podfile target — neither adds a sound asset via PBXProject.AddFile + AddFileToBuildSection on the main target, and no CopyResourceBundle is wired up. So when xcodebuild produces the .app, no vine_boom.wav ends up under MyApp.app/.
Step-by-step proof
- User taps WITH SOUND (
send_sound_button, SendPushSectionController.cs:50-52). AppViewModel.SendNotification(NotificationType.WithSound)calls_apiService.SendNotification(...).OneSignalApiService.SendNotificationbuilds the payload with"ios_sound": "vine_boom.wav"and POSTs it to/api/v1/notifications.- OneSignal delivers the push to APNs; the APS payload contains
"sound": "vine_boom.wav". - iOS
UNNotificationSound(named: "vine_boom.wav")walks the app bundle, finds nothing, and silently falls back to the default notification sound. - The user hears the default iOS chime, not a custom "vine boom" — the labeled behavior of the button ("plays a custom sound") does not happen on iOS.
Why existing code doesn't prevent it
There is no other dispatch path: Assets/Plugins/iOS/ contains only OneSignalDemoKeyboard.mm. The iOS plugin folder has no audio assets and no .bundle. BuildPostProcessor.cs adds the widget extension and rewrites the Podfile, but never references a sound file. Nothing in the build pipeline catches the missing asset.
Impact
Demo-only, and the push still delivers (with the default sound). E2E specs that only assert the API call happens still pass. The user-visible deficit is real but minor: a developer evaluating the demo, or a test that asserts sound semantics, would observe the wrong sound. Severity = nit.
Fix
Either:
- Ship
vine_boom.wav(or any other custom sound) underexamples/demo/Assets/Plugins/iOS/with a.meta, then inBuildPostProcessoradd it to the main target:var soundGuid = project.AddFile("vine_boom.wav", "vine_boom.wav"); project.AddFileToBuild(project.GetUnityMainTargetGuid(), soundGuid);
- Or remove
NotificationType.WithSoundand the WITH SOUND button until the asset pipeline is in place.
|
|
||
| public async void EndLiveActivity(string activityId) | ||
| { | ||
| if (string.IsNullOrEmpty(activityId) || _isLiveActivityUpdating) | ||
| if (string.IsNullOrEmpty(activityId)) | ||
| return; | ||
|
|
||
| _isLiveActivityUpdating = true; | ||
| NotifyStateChanged(); | ||
|
|
||
| try | ||
| { | ||
| var eventUpdates = new JObject |
There was a problem hiding this comment.
🟡 Nit: EndLiveActivity dropped its _isLiveActivityUpdating early-return guard and set/clear bookends (AppViewModel.cs:440-474), and LiveActivitiesSectionController.RefreshButtonStates dropped && !_viewModel.IsLiveActivityUpdating from the End button (line 107), while UpdateLiveActivity still uses both guards. Rapid taps on End now fire N concurrent POSTs to /api/v1/notifications consuming rate-limit credits. The asymmetry with the still-guarded Update path reads as an oversight from the toast-removal refactor (the removed lines were adjacent to deleted ShowToast calls) — either restore the guard on End for consistency, or add a code comment documenting the deliberate asymmetry.
Extended reasoning...
What the asymmetry is
AppViewModel.UpdateLiveActivity at lines 400–439 still uses both arms of the in-flight guard:
if (string.IsNullOrEmpty(activityId) || _isLiveActivityUpdating)
return;
_isLiveActivityUpdating = true;
NotifyStateChanged();
// ... await _apiService.UpdateLiveActivity(activityId, "update", ...) ...
_isLiveActivityUpdating = false;
NotifyStateChanged();EndLiveActivity at lines 440–474, however, has only the activity-id null-check. The previous || _isLiveActivityUpdating clause and the matching _isLiveActivityUpdating = true/false; NotifyStateChanged(); bookends were removed in this PR. The companion UI change at LiveActivitiesSectionController.cs:107 reduced _endButton?.SetEnabled(hasActivityId && hasApiKey && !_viewModel.IsLiveActivityUpdating) to just _endButton?.SetEnabled(hasActivityId && hasApiKey), while the Update button at line 102 kept the full clause.
Step-by-step proof of the consequence
- User starts a Live Activity.
_isLiveActivityUpdating = false, End button enabled. - User rapid-taps End twice within ~50 ms (or a fast E2E spec does
element.click(); element.click();). - First tap:
OnEndTap→_viewModel.EndLiveActivity(activityId). The new entry guard checks onlystring.IsNullOrEmpty(activityId)→ false → proceeds. It awaits_apiService.UpdateLiveActivity(activityId, "end", …), which builds the JSON payload withdismissal_date = DateTimeOffset.UtcNow.ToUnixTimeSeconds()andPOSTs tohttps://api.onesignal.com/api/v1/notifications. - Before that await resumes, the second tap fires.
_isLiveActivityUpdatingis never set, so there is no flag to short-circuit on. The End button is still enabled (the UI guard was also stripped). Same code path runs again: builds a new JSON payload with a freshdismissal_dateandPOSTs a second concurrent request. - The OneSignal end-event API is logically idempotent, so the final state converges, but both requests consume rate-limit credits against
/api/v1/notificationsand emit slightly-differentdismissal_datetimestamps in their bodies.
Why existing code doesn't prevent it
OnEndTap (LiveActivitiesSectionController.cs:129) dispatches straight to _viewModel.EndLiveActivity(activityId) with no debounce. The only flow-control on the End path used to be _isLiveActivityUpdating, and this PR removed both the model-side check and the UI-side disable in the same commit. Update kept both.
Addressing the refutation
A reviewer argued the change is intentional: End is a terminal operation and the OneSignal end event is idempotent, so duplicate calls converge to the same state; further, removing the guard lets users escape a stuck Update by hitting End, which is the better UX.
That is a plausible reading and I agree the functional outcome is benign — no data loss, no SDK behavior change, the UI eventually shows status_delivered. But (a) the PR description doesn't mention live-activity changes and lists only the Appium-prep scope, (b) the deleted lines sit immediately adjacent to deleted ShowToast calls in the diff, which is the cluster you'd expect from a mechanical toast-removal sweep, and (c) if the asymmetry is deliberate, a one-line comment (// End is idempotent; intentionally re-entrant so a stuck Update can be canceled) costs nothing and prevents a future reader from restoring symmetry by accident.
Impact
Demo-only, no SDK source changes. Worst case is N redundant POSTs to /api/v1/notifications consuming rate-limit credits on rapid double-tap. Filing at nit severity — either restore the guard for consistency, or annotate the intentional asymmetry so a future maintainer doesn't read it as a bug.
- Add vine_boom.wav for both Android (res/raw) and iOS (iOS/Sounds/) matching the convention used by the Flutter/RN/Cordova demos so the WITH SOUND notification actually resolves a UNNotificationSound resource. - Wire the iOS sound through BuildPostProcessor by registering vine_boom.wav on the Unity main target's Resources phase (idempotent across appends). - Restore the _isLiveActivityUpdating in-flight guard and matching End-button UI disable so rapid taps on End no longer fire concurrent POSTs. Co-authored-by: Cursor <cursoragent@cursor.com>
Unused sample sound asset added in 2023 with no consumers in code or docs across the repo or demo. Safe to delete; OneSignal channels reference sound files by name from the app's own res/raw, not from this asset. Co-authored-by: Cursor <cursoragent@cursor.com>
Description
One Line Summary
Prepare the Unity demo app for Appium E2E test automation by adding accessibility bridges, stabilizing UI element naming, and improving iOS/Android test reliability.
Details
Motivation
SDK-4406: enable Appium-driven E2E tests against the Unity demo app. Unity's UI Toolkit does not expose native accessibility identifiers, so Appium cannot reliably locate elements on iOS or Android. This PR adds native accessibility bridges (iOS and Android), standardizes UI element names, and fixes a number of demo-side bugs uncovered while wiring up the test harness.
Scope
examples/demo/is touched. No SDK source changes.AccessibilityBridgeplus iOS (OneSignalDemoKeyboard.mm) and Android (OneSignalUnityE2EAccessibility.java) native helpers used to expose UI elements to Appium.LogView/LogManager(replaced withDebug.Log), renamesTrackEventSectionControllertoCustomEventsSectionController, consolidates upsert helpers, replaces the loading overlay with inline loading states, and tightens dialog/keyboard behavior.Testing
Manual testing
./run-local.sh --sdk=unity --platform=android --spec="{01_,02_,03_}".--skip-buildfor01_through12_; all numbered specs passed.Affected code checklist
Checklist
Overview
Testing
Final pass
Made with Cursor