[SDK-717] Fix stale customAction replay when handler returns false - #1089
joaodordio wants to merge 7 commits into
Conversation
The IterableCustomActionHandler interface documents its boolean return value as 'Reserved for future use'. Clients routinely return false, as does the React Native SDK wrapper. Since the SDK-307 fix (3.6.5), returning false from the handler left pendingAction alive, causing the handler to replay on every app foreground and every initialize() call. Fix: treat invocation of the handler as consumed regardless of return value. Only keep pendingAction alive when the handler is null (SDK not yet initialized) — the original SDK-307 intent. Also remove the duplicate processPendingAction() call early in initialize(), before loadLastSavedConfiguration() runs. A single call after full initialization is sufficient. Affected versions: 3.6.5–3.10.1.
The previous approach always returned true from callCustomActionIfSpecified when a handler was present. That broke the openApp fallback in handlePushAction: 'if (openApp && !handled)' never fired, so the launcher activity was not started for push taps where the handler returned false. Move the fix to processPendingAction instead. Before dispatching, capture whether customActionHandler was present. After dispatching, clear pendingAction if handled=true OR if the handler was present — invocation itself is treated as consumed. The handler's return value is preserved as-is so the openApp fallback in handlePushAction continues to work.
|
This solution makes the openApp fallback work correctly, but customHandlerPresent does not necessarily mean the custom handler was invoked. From what i checked, an unhandled openUrl action is also cleared when an unrelated custom-action handler is configured, preventing its foreground retry. I though about adding improving readability with something internal like this enum ActionDispatchResult { This would make it easier to distinguish when to clear the pending action or when to run it. I think having this boolean when it actually can represent more things can be a bit confusing to debug and to understand what is the intended behavior (like the bug itself) |
Replace the customHandlerPresent boolean with a typed enum that cleanly
separates the two concerns a single boolean cannot express:
- whether pendingAction should be cleared
- whether the openApp launcher fallback should fire
NOT_DISPATCHED — handler was null, keep pendingAction for retry (SDK-307)
DISPATCHED_WITH_FALLBACK — handler ran but returned false, or URL failed;
clear pendingAction, allow openApp fallback
HANDLED — handler returned true or URL opened; clear pendingAction,
suppress openApp fallback
Also fixes Franco's callout: the previous customHandlerPresent check would
incorrectly clear a failed openUrl action whenever an unrelated custom
action handler was configured. dispatchPendingAction now checks handler
presence only for non-URL actions, so URL and custom action paths are
independently reasoned about.
|
I was checking this PR with some tests and realized that we introduced a new problem, currently if the app is closed and a push url is passed, we silently fail it. Currently on the initializeForPush we just pass the android context, not the iterable config. We could rename NOT_DISPATCHED to RETRY_LATER and return it instead in this case, so a failed URL before full initialization should produce RETRY_LATER, not DISPATCHED_WITH_FALLBACK. |
When only initializeForPush ran (_apiKey null), the SDK lacks the full config (urlHandler, deep link handlers). A URL push action would fall through dispatchPendingAction to DISPATCHED_WITH_FALLBACK, clearing pendingAction and silently losing the action before initialize() ran. Fix: URL actions now check _apiKey == null and return RETRY_LATER, so they are deferred alongside custom actions when the SDK is not yet fully initialized. Also renames NOT_DISPATCHED -> RETRY_LATER throughout — clearer intent. Adds testBackgroundUrlActionDeferredUntilSDKInit and a matching fixture to cover the openApp=false + URL action + pre-init path.
The customActionHandler != null check was too narrow — when the SDK was fully initialized but had no customActionHandler configured, dispatch returned RETRY_LATER and never called IterableActionRunner.executeAction. This broke testTrackPushOpenWithCustomAction and testPushActionWithTextInput (zero mock interactions) and in production would strand pendingAction alive indefinitely for any app without a customActionHandler. The correct signal for 'not ready to dispatch' is _apiKey == null (only initializeForPush ran, not the full initialize()). When the SDK is fully initialized, always dispatch — IterableActionRunner returns false naturally when no handler is configured, which correctly produces DISPATCHED_WITH_FALLBACK and allows the openApp fallback to fire. Also moves tracking after the RETRY_LATER guard to prevent double trackPushOpen calls on subsequent retries.
| // customActionHandler). When the SDK is fully initialized, always dispatch and let | ||
| // IterableActionRunner return false naturally if no handler is configured — that | ||
| // correctly allows the openApp fallback to fire without keeping pendingAction alive. | ||
| if (IterableApi.sharedInstance._apiKey == null) { |
There was a problem hiding this comment.
I'm a bit reluctant of changing this to here, this will change the behavior on many calls, even if they are changing to something "more correct" i think we should stick to fixing the reported bug and add visibility and readability with this PR. Let me know what you think, i pushed a sibling branch with a similar solution but less "breaking". Let me know what you think
Move ActionDispatchResult into IterableActionRunner where the dispatch logic lives. Add dispatchAction() alongside executeAction() (kept as a boolean adapter for existing callers). Three states: NOT_HANDLED — no handler or URL failed; preserve retry behavior DISPATCHED_UNHANDLED — handler invoked, returned false; consumed, allow openApp fallback HANDLED — handler returned true or URL opened; consumed, suppress fallback processPendingAction clears pendingAction on anything != NOT_HANDLED, so a failed URL action is no longer prematurely cleared just because an unrelated customActionHandler happens to be configured. Tests: update mock stubs and verifies to dispatchAction; replace verbose test comments with Franco's cleaner versions; add testCustomActionHandlerDoesNotConsumeFailedUrlAction to cover the URL/customActionHandler independence case; remove the now-unnecessary background URL fixture and test.
2 participants
📝 Summary
Fix stale
IterableCustomActionHandlerreplay when handler returnsfalse, a regression present in 3.6.5–3.10.1.🎟️ Jira Ticket: SDK-717
📖 Description
The
IterableCustomActionHandlerinterface documents its boolean return value as "Reserved for future use". Clients commonly returnfalse(Kotlin default, follows the javadoc), and the React Native SDK wrapper always does too. Since the SDK-307 fix (3.6.5, PR #975), returningfalsewas treated as "action not consumed", leavingpendingActionalive and causing the handler to fire again on every foreground and everyinitialize()call.Three replay paths:
onForeground()callsprocessPendingActionon every app foregroundinitialize()called it twice — before and afterloadLastSavedConfiguration()super.onResume()triggersonActivityResumed → onForeground → processPendingActionon the stale action beforehandlePushActionsets the new oneIterableActionRunner.java—callCustomActionIfSpecifiednow invokes the handler and always returnstrue(consumed). Returnsfalseonly when the handler isnull(SDK not yet initialized), keeping the SDK-307 retry intent intact.IterableApi.java— removed the duplicateprocessPendingAction()call beforeloadLastSavedConfiguration()ininitialize(). One call after full initialization is enough.🧪 How to test?
Added
testCustomActionHandlerReturnFalseDoesNotReplayOnForegroundtoIterablePushActionReceiverTest: sends a push with a handler that returnsfalse, simulates a foreground event viaprocessPendingAction, and asserts the handler fired exactly once.Existing
testBackgroundCustomActionProcessedAfterSDKInitcontinues to cover the SDK-307 scenario (handler null at push time, retried after initialize).🧾 Changelog
Added a
Fixedentry toCHANGELOG.mdunder[Unreleased]calling out affected versions 3.6.5–3.10.1.📹 Loom recording if applicable
N/A
🐞 Github Issues solved
N/A
📚 Docs PR if applicable
N/A