Repository navigation
Conversation
📊 Diff Coverage Report✓ Coverage check passed (no source files changed) |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Multi-model review (Claude Opus 5.5, GPT 5.6 Sol, Grok 4.7) — demo-only JWT/IV parity with iOS. 10 files, +357/−84.
Act on (3/3): JwtAskStore.onInvalidated posts to the main queue, while answered/clear write immediately. A 401 that was already queued can run after logout or PROVIDE JWT and put the banner/snackbar back for a user whose tokens were just cleared. Stamp a generation on the posted runnable (or send every mutation through the handler) so clear/answered win.
Consider (Grok; related Opus): A different-user loginUser only calls resetUserFetch() and waits for onUserStateChange. That callback is onesignal_id hydrate, which does not fire while LoginUserOperation is parked for a JWT (Android still does not ask on park — SDK-5476). Launch already fetches by external_id without a onesignal_id; a switch does not, so the REST row stays — and a stored bearer is never sent. Calling fetchUserDataFromApi() after _externalUserId is updated would match launch.
Consider (3/3): readJwtTokens deletes JWT_TOKEN_PREF without copying it under the cached external id. Upgrading testers lose the old single token and get an unsigned 401 until they paste it again.
Noted: snackbar “once per ask” is per Activity (rememberSaveable), not process-wide. answered/clear assume the main thread.
Dismissed: listener not literally “right after init” — it is still after initWithContext and before any async 401. resetUserFetch racing a hydrate is unlikely: createAndSwitchToNewUser replaces the identity model (onModelReplaced is empty) and login() returns before enqueue.
Sent by Cursor Automation: PR Reviews
| fun onInvalidated(externalId: String) { | ||
| mainHandler.post { | ||
| _externalId.value = externalId |
There was a problem hiding this comment.
Act on (3/3): this post is not invalidated by answered/clear. invalidateJwt already hops to launchOnDefault before this runs, so a logout or PROVIDE JWT can setValue(null) and then this runnable restores the old ask (banner + snackbar). Give the runnable a generation, or post answered/clear through the same handler.
| if (sameUser) { | ||
| // No user-state event fires for the same user, so refetch here instead of | ||
| // clearing lists that nothing would repopulate. | ||
| fetchUserDataFromApi() | ||
| return@withContext | ||
| } | ||
| resetUserFetch() |
There was a problem hiding this comment.
Consider (Grok; related Opus): same-user refetches here because no user-state event fires. The other branch only resetUserFetch() and waits for onUserStateChange, which is onesignal_id hydrate — not replace, and not a parked LoginUserOperation (SDK-5476). Launch already fetches by external_id with no onesignal_id; a switch leaves the row at — and never sends a stored bearer. fetchUserDataFromApi() after _externalUserId is set would match launch.
| if (prefs.contains(LEGACY_JWT_TOKEN_PREF)) { | ||
| prefs.edit().remove(LEGACY_JWT_TOKEN_PREF).apply() |
There was a problem hiding this comment.
Consider (3/3): this deletes the old single-token key and does not copy it under the cached external id. An upgraded demo with the toggle on will unsigned-401 until the tester pastes the JWT again. If that is acceptable, say so in examples/build.md; otherwise migrate then delete.
abdulraqeeb33
left a comment
abdulraqeeb33
left a comment
There was a problem hiding this comment.
Approve. Demo-only, and the behavior in the description matches the code.
The JwtAskStore ordering note already on the thread is the one nit: post answered and clear through the same handler, and let a later invalidation still show.
A different-user login that succeeds still refetches when the server id arrives. A parked login leaving the REST row at the dash placeholder is what resetUserFetch describes. The legacy JWT_TOKEN_PREF delete is intentional in SharedPreferenceUtil.
fadi-george
left a comment
fadi-george
left a comment
There was a problem hiding this comment.
I need to test it out.
The demo's Identity Verification controls now follow one contract shared with the iOS demo. - onUserJwtInvalidated only logged. MainApplication now registers the listener right after init, since the SDK doesn't replay the event and MainViewModel dies with MainActivity. The listener shows a banner with the asked id, a PROVIDE JWT button, and a snackbar once per ask. It leaves the stored token alone, since an SDK that asks when requests park without a token fires the same event. - The demo kept one token, so an invalidation for another user or an A to B to A switch lost the right one. It now keeps a token per external id. A login with a token or updateUserJwt stores it, and logout clears them all. - A failed /users fetch was invisible. A REST fetch row now reads OK or the reason, for example "external_id fetch failed: HTTP 401". A user switch, logout, or skipped fetch resets it, and a switch or logout drops any fetch still in flight for the previous user. - The toggle read "Identity Verification" but only picks the alias for the demo's own REST fetch. It now reads "Fetch by external_id (JWT)" and refetches when flipped. The launch fetch no longer needs a onesignal_id. - Update User JWT opens a dialog with the fields stacked full width, prefilled from the pending ask, confirming with "Update". UPDATE USER JWT and PROVIDE JWT are always enabled.
Description
One Line Summary
Make the demo's Identity Verification controls behave the same as the iOS demo (OneSignal/OneSignal-iOS-SDK#1774).
Details
Motivation
A review of both demos found that their JWT controls behaved differently, and found bugs. The REST fetch row could show
OKfor a fetch that never ran. A late fetch could bring back the previous user after logout. The demo stored a single token, so some user switches lost it.Scope
❗️Demo app only. No SDK changes.
MainApplication, so events aren't missed whileMainActivityis gone. The User section shows a banner with a PROVIDE JWT button, and a snackbar.OKor the error, and resets on a user switch or logout.The remaining differences from the iOS demo come from the SDKs (SDK-5474, SDK-5476) and are listed in
examples/build.md.Testing
Unit testing
None. Demo UI only.
Manual testing
Ran 13 scenarios on an Android 34 emulator against a test app with Identity Verification on. They covered login with and without a token, user switches, expired tokens, PROVIDE JWT, logout, and rotation. All behaved as expected.
Screenshots
iOS on the left, Android on the right, both demos on an app with Identity Verification on.
alicewith an expired token. The REST fetch row shows the 401 and the banner names the user the SDK is waiting on. Android also shows the snackbar.Affected code checklist
Checklist
Overview
Testing
Final pass
🤖 Generated with Claude Code