fix(inbox): correctness and cross-platform parity fixes - #799
fix(inbox): correctness and cross-platform parity fixes#799mahmoud-elmorabea wants to merge 7 commits into
Conversation
- treat an absent/unparseable X-CIO-Inbox-Enabled header as no signal so a 304 or failed poll no longer hides the inbox until the next clean 200 - reopen the once-per-session revalidation gate on a new store session id, so a logout cannot serve the previous user's templates/branding - drop expired messages inclusively (expiry <= now) to match iOS - remove the unused fromCache flag from InboxVisibility/InboxFetchOutcome; it was miscomputed when one asset was served from cache and the other refetched - remove the unnecessary @OptIn(InternalCustomerIOApi) from both samples - delete the stale MessagingInboxModuleTest placeholder Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sample app builds 📱Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/overlay-inbox #799 +/- ##
========================================================
- Coverage 64.88% 64.63% -0.25%
- Complexity 1033 1041 +8
========================================================
Files 175 175
Lines 6017 6034 +17
Branches 940 949 +9
========================================================
- Hits 3904 3900 -4
- Misses 1791 1819 +28
+ Partials 322 315 -7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
📏 SDK Binary Size Comparison Report
|
|
Build available to test |
- share uiStateFlow via shareIn and make the controller a process singleton, so bell, panel and any embedded view observe one upstream and share the per-session dedupe guards instead of each running their own pipeline - pass the collected state into the list instead of collecting a second time - memoize the parsed branding against its raw payload; it is read several times per store change while the payload changes at most once a session - build the snapshot from the message list the visibility decision already produced, dropping a duplicate selection plus Jist deep-copy - correct the action docs: openUrl opens externally as the platform resolves it, matching how push and in-app already handle links; it was never http(s)-only Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
9ebdeed to
affb0a9
Compare
|
|
|
|
…s lazy A top-level lazy outlived CustomerIO teardown, so the next initialization got a stale module reference and the instrumentation leak test failed. Graph-held singletons are released on reset. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
…-fetch A logout landing while a templates/branding fetch is in flight reopens the session gate, but the completing fetch then closed it again, so the next session served render assets it never revalidated. Close the gate only when the session that started the cycle is still current. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 70568c4. Configure here.
|
|
The store is a StateFlow collected from a launched coroutine, so the first delivery replays the current session id rather than signalling a change, and it can land after a load has already closed the revalidation gate — costing a redundant conditional GET. Reset only on a change away from a session id already seen, matching the guards the other subscribeToAttribute call sites use. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
| private fun isExpired(message: InboxMessage, now: Date): Boolean { | ||
| val expiry = message.expiry ?: return false | ||
| return expiry.before(now) | ||
| return expiry <= now |
There was a problem hiding this comment.
The <= check looks right. Could we keep a follow-up for time-driven expiry? Selection only reruns after another update, so an expired message can remain visible during a quiet SSE session.
There was a problem hiding this comment.
Agreed, and I have deliberately left this one out of these PRs — it is a real gap but a behavioral change rather than a fix to what this PR touched, and it needs a design decision.
Confirming the diagnosis: expiry is applied on READ, in InboxSelection/VisualInboxSelector, and a read only happens when something re-triggers it — a queue poll, an SSE store update, or a templates/branding fetch completing. Nothing is time-driven, so during a quiet stretch an expired message stays on screen until the next unrelated update. iOS is identical (VisualInboxSelector.select(messages:now:) off the same read path), so any fix wants to land on both.
The open question is what drives re-evaluation. A wall-clock timer in the SDK is the obvious answer but means a periodic wake purely to hide a row, and it interacts with the polling interval the server already dictates. Scheduling a single one-shot at the earliest upcoming expiry — recomputed whenever the selection changes — gets the same result with one pending timer and no idle work, which is what I would propose.
Happy to open a follow-up with that shape unless you would rather it ride along here. One caveat either way: the expiry check is now inclusive (expiry <= now), so a timer firing exactly at the boundary does drop the message.
| // Reuse the list the visibility decision was made from rather than re-selecting: a second | ||
| // read would repeat the selection and the Jist deep-copy, and could observe a store change | ||
| // in between and disagree with the state published alongside it. | ||
| val messages = (visibility as? InboxVisibility.Visible) |
There was a problem hiding this comment.
Could we remove getSelectedMessages() and its stale test setup? snapshot() now uses visibility.messages, so this method no longer has a production caller.
There was a problem hiding this comment.
Done in 1b0df36 — confirmed, and it was worse than dead code.
getSelectedMessages() is gone from VisualInbox along with all eight references in VisualInboxControllerTest: five every { ... } returns stubs, one returnsMany, and two verify(exactly = 0) assertions that were only meaningful while the method existed. snapshot_givenHiddenButSelectableMessagesExist_... is renamed to snapshot_givenHidden_... since the real invariant it guards is that only InboxVisibility.Visible carries messages.
Checked before removing: @InternalCustomerIOApi, absent from messaginginapp.api, and unreferenced by the React Native and Flutter wrappers — so no baseline change and no wrapper break. apiDump validation is clean.
Worth passing on, because it nearly cost me: removing the method broke :messaginginbox test COMPILATION, and this repo's test task has ignoreFailures, so ./gradlew test printed BUILD SUCCESSFUL while that module silently produced no JUnit XML at all — 75 tests just vanished from the run rather than failing. I only caught it because I count tests out of the XML. Both modules now report: 592 tests, 0 failures.
… dead API An empty selection no longer hides the inbox: a renderable inbox with nothing in it is "You're all caught up", which the list already had an empty state for but could never reach. Overlay chrome stays gated on having renderable messages, so no bell appears over an empty panel. The revalidation gate now closes only when there is something to serve. A failure with an empty cache had no fallback, so closing it stranded the inbox hidden for the rest of the process while later store updates re-read the same empty cache. Removes getSelectedMessages(), whose last production caller went away when the snapshot began reusing the visibility's message list, along with its test setup. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
| private fun isExpired(message: InboxMessage, now: Date): Boolean { | ||
| val expiry = message.expiry ?: return false | ||
| return expiry.before(now) | ||
| return expiry <= now |
There was a problem hiding this comment.
The one-shot timer at the earliest expiry sounds like the right follow-up. Could you open and link that issue here so it is not lost? This does not need to block this PR.
| emptyList() | ||
| } | ||
| // | ||
| // Reuse the list the visibility decision was made from rather than re-selecting: a second |
There was a problem hiding this comment.
Could we update the remaining comments to match the new empty-inbox behavior? This file still says visibility requires a message and that dismissing the last row makes the inbox hidden. The old rule also remains in InboxRepository.computeVisibility, InboxFetchOutcome, and the repository test header.
2 participants
Addresses the correctness findings from the cross-platform review of the inbox overlay work. Paired with the equivalent iOS PR so both SDKs behave identically.
X-CIO-Inbox-Enabledheader no longer hides the inbox. 304s and error responses routinely omit it, so the previous coercion tofalsemade the bell disappear until the next clean 200.expiry <= now), matching iOS.fromCacheis removed fromInboxVisibility/InboxFetchOutcome; nothing consumed it and it was miscomputed when one asset came from cache and the other was refetched.messageOpenedcallback reports the post-action opened state.@OptIn(InternalCustomerIOApi)from both samples (verified they compile without it).MessagingInboxModuleTestplaceholder.Verified locally:
messaginginapp+messaginginboxunit tests, ktlint,apiCheck, and both sample apps compile.🤖 Generated with Claude Code
Note
Medium Risk
Touches server-driven enablement, per-session asset caching across logout, and visibility rules that control when the inbox UI appears—well-tested but user-facing inbox behavior.
Overview
Aligns Android visual inbox behavior with iOS and hardens edge cases around enablement, caching, and what “visible” means.
Enablement: Missing or bad
X-CIO-Inbox-Enabledno longer forces the inbox off; the last known flag is kept so 304/error polls don’t hide the bell.Visibility & empty state: Templates + branding + enabled gate visibility; zero messages is still Visible (empty “caught up” UI). Overlay chrome stays gated on renderable messages. Drops unused
fromCacheand theno selected messageshidden reason.Templates/branding session gate: Revalidation reopens on store
sessionIdchange (logout/reset). The gate only closes when the fetch’s session is still current and there are servable assets—so mid-fetch reset, total fetch failure with no cache, and cross-user asset bleed are avoided. Branding JSON parse is memoized.Overlay: One SDK-scoped
VisualInboxControllerwith shareduiStateFlow(no duplicate load/snapshot per composable). UI messages come from the same list asInboxVisibility, not a secondgetSelectedMessages()pass.messageOpenedreportsopened = true. Expiry filter usesexpiry <= now.Reviewed by Cursor Bugbot for commit 1b0df36. Bugbot is set up for automated code reviews on this repo. Configure here.