feat(SDK-520): replace magic 1000ms timeouts with named, configurable…#882
Conversation
… constants - Add IterableConfig.androidWakeDelayMs and authCallbackTimeoutMs to expose the two hardcoded timeouts as tunable, documented values. - Replace the fixed-window auth callback gate with an event-driven Promise latch; the timer survives only as a safety-net fallback.
|
Qlty doesn't post analysis or coverage comments for contributors without a seat. An authorized user can grant @jferrao-itrbl a seat from this pull request's page in Qlty. |
…default - Replace shared authLatchResolver/pendingAuthResult with a per-invocation FIFO queue so overlapping handleAuthCalled invocations no longer drop successCallback/failureCallback - Route native success/failure events to the oldest pending invocation and shift the queue synchronously on resolve - Raise IterableConfig.authCallbackTimeoutMs default 1000 -> 6000 with rationale in TSDoc - Remove dead AUTH_CALLBACK_TIMEOUT_DEFAULT_MS / ANDROID_WAKE_DELAY_DEFAULT_MS fallbacks; IterableConfig is the single source of truth - Update Iterable.test.ts default assertions (1000 -> 6000), rework safety-net test to use a custom short timeout - Add regression test: two back-to-back handleAuthCalled invocations assert both successCallbacks fire with no 'No callback received' warning - Update CHANGELOG default
|
Really nice iteration on the latch, the FIFO scoping and the 6s default both look great. One thing I noticed while tracing through the new design that I think is worth catching before this lands: The FIFO queue only gets drained in the Concrete single-flight trace for the
Also a slow memory leak on the The fix I'd suggest is dropping the invocation from the queue in each of those four paths, same shape as the defensive splice you already have in const removeInvocation = () => {
const index = pendingAuthInvocations.indexOf(invocation);
if (index !== -1) {
pendingAuthInvocations.splice(index, 1);
}
};then call removeInvocation() in the string, null/undefined, unexpected-type branches, and in the .catch. There's a tiny caveat that a late native event for a .catched invocation will then misroute to whatever is at the new head (either nothing, or a newer invocation), but that's a pre-existing bridge limitation with no correlation id and the current behavior is strictly worse since it deterministically blocks the next real callback. Locking this down with a rejection-then-retry test in the same shape as the great overlap regression test you just added would be perfect, something like: fire handleAuthCalled → reject authHandler → fire handleAuthCalled again → resolve with IterableAuthResponse → assert the second successCallback fires. |
…bie invocations - Add removeInvocation() in handleAuthCalled; call it in the string, null/undefined, unexpected-type, and .catch branches so rejected/non-IterableAuthResponse auth handlers no longer leave a zombie at queue head - Prevents the next invocation's native success/failure event from routing to a dead invocation and dropping its callback - Add regression test: handleAuthCalled -> reject -> handleAuthCalled -> resolve -> native success asserts second successCallback fires with no 'No callback received' warning
|
Thanks for catching this @joaodordio , really good spot. Added a Added a regression test: All tests green and updated rules to make sure similar scenarios are avoided or caught earlier in the future. |
joaodordio
left a comment
There was a problem hiding this comment.
Thanks for addressing the issues João!
LGTM!

🔹 JIRA Ticket(s) if any
✏️ Description
Replace hardcoded magic timeouts: