Keep what the payer typed when the form leaves the composition

[Alex Arguello (alex-arguello)]

Ticket: PLA-2491

The payment form kept everything the payer had typed inside the composition that drew it. Anything that ends
that composition emptied the boxes, and a rotation, a fold, a switch to another tab and a return from a screen
pushed on top all end it. The values now live on the payment flow, which a host already holds for the life of
the screen, so a payer who rotates the phone mid-form does not enter the card again.

Structure

The state moves onto PayabliPayInPaymentFlow. PayabliPayInForm already takes that object, and its
documentation already tells a host to keep it in whatever survives a configuration change: a ViewModel, a
Decompose component, a presenter. So the thing with the right lifetime was in the signature and the values
were simply not on it. Nothing is added to the public API and no integrator changes a call site.

Nothing typed reaches saved instance state, and rememberSaveable is now absent from the module. A
Bundle is serialized by the system and can be written to disk, so a form reopened after process death is an
empty form; what recovers a payment interrupted there is the idempotency key the submission already mints.

The draft decides for itself when to refill. It records the configuration it was last filled from and a
digest of the seeded values, and compares against those, so a composition starting again with the same pair
keeps what is in the boxes while a caller handing over new values still replaces them. Reading that comparison
as unchanged is what a wrong answer costs: the form would submit the card the caller replaced, so the digest
reads the characters rather than String.hashCode, which puts a miss on a 64-bit coincidence. It is a digest
rather than the object because a caller's PayInFormValues can carry a card number and the draft outlives the
composition that drew it, and salted per draft so the value kept cannot be computed elsewhere and matched
against a table of card numbers.

That comparison cannot live in a remember key, which goes with the composition it belongs to. It also cannot
run unconditionally: refilling on every composition writes state that the same composition then reads, and the
form recomposes every frame without ever settling.

Notable changes

• PayInFormDraft holds the chosen instrument, the typed values, the fields the service rejected and whether
  a submission in flight is this form's. All four were separate remember calls before, three of which were
  keyed on the configuration and one of which was saveable.
• PayInFormContent takes the draft as a required parameter with no default. Where it is held is what decides
  whether the payer keeps their input, so a composable that supplied its own would settle that by omission.
• The instrument is still emptied on any outcome, approved or refused, by the same field list as before.
  Everything is emptied when the host's scope is cancelled, which is the screen going for good.
• The submit button reads the instrument and the rejected fields from the draft at the tap rather than from
  what the last composition drew, which is what it already did when they were local state.
• RecordingLogSink in :core's shared test fixtures collects into a CopyOnWriteArrayList. It backed onto
  an ArrayList and is written by every request a transport has in flight, which lost 11,421 of 16,000 lines
  across eight writers and threw out of ArrayList.add often enough to redden a run. That is what failed CI on
  this branch, in a module it does not touch.
• One form per flow, stated on PayabliPayInForm. Two forms given the same flow draw the same boxes, and two
  given the same flow with different configurations refill each other on every frame.

Verification

Unit tests, ktlintCheck and lint are clean. All 37 :payin instrumented tests pass on four targets with
nothing skipped: a Pixel 7a and an SM-S908U1 on API 36, an SM-A136U1 on API 33, and the API 34 emulator the
nightly runs. The sample app's 7 pass on that emulator, since it draws this form.

16 unit tests cover the draft and 5 instrumented tests cover retention. Four separate breaks were introduced
deliberately and reverted, and each was caught by exactly the tests naming the guarantee it broke: removing
the refill comparison, stopping the seed reading the caller's values, adding a saved-state copy of a typed
value, and holding the caller's seed object rather than its hash. The inventory is on the ticket.

Not included

Whether the form sets FLAG_SECURE, and whether it opts its fields out of platform autofill. Neither is set
today, both are decisions rather than omissions, and both are recorded on the ticket with what they turn on.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Moves PayIn form state (typed values, selected method, rejected fields, submission pending) out of the Compose composition into a reusable draft object so user input survives composition teardown (e.g., rotation / navigation), while explicitly not persisting sensitive input to saved instance state.

Changes:

• Introduce PayInFormDraft and thread it through PayInFormContent and PayabliPayInPaymentFlow to retain form state outside Compose remember.
• Update UI and instrumentation tests to use the shared draft and add new retention/draft-behavior tests.
• Update public KDoc to clarify retention guarantees and constraints (one form per flow, no saved-instance retention of typed values).

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
payin/src/main/java/com/payabli/sdk/payin/form/PayInFormDraft.kt	Adds draft object that owns method/typed/errors/pending and handles seeding/switching/clearing.
payin/src/main/java/com/payabli/sdk/payin/ui/PayInFormContent.kt	Replaces composition-owned state (remember/rememberSaveable) with the injected draft.
payin/src/main/java/com/payabli/sdk/payin/payment/PayabliPayInPaymentFlow.kt	Stores a long-lived draft in the flow and attempts to clear it on scope completion.
payin/src/main/java/com/payabli/sdk/payin/PayabliPayInForm.kt	Passes flow.draft down and updates KDoc on retention and lifecycle expectations.
payin/src/test/java/com/payabli/sdk/payin/form/PayInFormDraftTest.kt	Adds JVM unit tests for draft seeding/resetting/switching/clearing semantics.
payin/src/androidTest/java/com/payabli/sdk/payin/ui/PayInFormSubmissionInstrumentedTest.kt	Uses a composition-external draft to match host behavior.
payin/src/androidTest/java/com/payabli/sdk/payin/ui/PayInFormSeedInstrumentedTest.kt	Uses a composition-external draft to match host behavior.
payin/src/androidTest/java/com/payabli/sdk/payin/ui/PayInFormRetentionInstrumentedTest.kt	Adds coverage for retention across composition teardown vs. saved-state restore.
payin/src/androidTest/java/com/payabli/sdk/payin/ui/PayInFormOutcomeAcrossRecreationInstrumentedTest.kt	Updates test to supply draft; aligns commentary with new retention approach.
payin/src/androidTest/java/com/payabli/sdk/payin/ui/PayInBrandBadgeInstrumentedTest.kt	Updates test to supply draft.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

payin/src/main/java/com/payabli/sdk/payin/form/PayInFormDraft.kt:81

• seed() is called on every composition, and initialValues?.hashCode() for a data class containing a Map is typically O(n) over entries. This adds avoidable overhead during frequent recompositions (e.g., while typing). Consider having callers supply a stable lightweight seed key (e.g., a version/nonce) or caching a precomputed seed hash on PayInFormValues creation so seed() can compare in O(1).

    fun seed(
        configuration: PayInFormConfiguration,
        initialValues: PayInFormValues?,
    ) {
        val key = configuration to initialValues?.hashCode()
        if (seededFrom == key) return
        seededFrom = key

payin/src/test/java/com/payabli/sdk/payin/form/PayInFormDraftTest.kt:167

• This test is inherently flaky: System.gc() is not deterministic and CI/JVM GC behavior can vary widely, so this can intermittently fail even when the implementation is correct. A more reliable approach is to assert the draft’s internal seed tracking does not retain the values object (e.g., by design/structure), or to use a ReferenceQueue and memory-pressure allocation techniques to make collection more likely and the test less timing-dependent.

        // System.gc is a request rather than a command, so this asks until it is answered.
        repeat(GC_ATTEMPTS) {
            if (watched.get() == null) return
            System.gc()
            Thread.sleep(GC_PAUSE_MILLIS)
        }

        fail("the draft still holds the values it was seeded from")

payin/src/test/java/com/payabli/sdk/payin/form/PayInFormDraftTest.kt:189

• To improve the reliability of the GC-based assertion, explicitly drop the strong reference before returning (e.g., create the WeakReference first, then set a var values: PayInFormValues? = ... to null). As written, the JVM may keep values strongly reachable longer than expected (e.g., due to stack/escape analysis), increasing flakiness.

    private fun seedAndRelease(): WeakReference<PayInFormValues> {
        val values =
            PayInFormValues(
                PayInMethodType.Card,
                // Built rather than written as a literal, so the map holds a string the constant pool does not.
                mapOf(PayInField.CardNumber to StringBuilder("4111111111111111").toString()),
            )
        draft.seed(configuration, values)
        return WeakReference(values)
    }

[Alex Arguello (alex-arguello)]

Suppressed comment · payin/src/main/java/com/payabli/sdk/payin/form/PayInFormDraft.kt:81 · seed hash cost per composition

seed() is called on every composition, and initialValues?.hashCode() for a data class containing a Map is typically O(n) over entries. This adds avoidable overhead during frequent recompositions (e.g., while typing). Consider having callers supply a stable lightweight seed key (e.g., a version/nonce) or caching a precomputed seed hash on PayInFormValues creation so seed() can compare in O(1).

Declined

seed on an already-seeded draft costs 54 ns, measured over two million calls after warm-up on the JVM, with a five-field card seed. A single keystroke recomposes the form, which is orders of magnitude more work than that.

The cost is O(n) and it is new. Before this branch remember(configuration, initialValues) compared the two seeds with equals, and PayInFormValues.equals opens with this === other, so a host passing a stable instance paid O(1) there.

Caching the hash on PayInFormValues means a mutable memo field on a public @Immutable value type, and a caller-supplied nonce means a new public parameter every integrator has to maintain. Neither is worth 54 ns, and both are one-line changes if a profile on a device ever disagrees.

Commit none. Nothing in this PR changes.
Test none. The measurement above was a throwaway harness, not a check worth keeping: a timing assertion on a figure this small measures the machine.

The other two suppressed items are fixed in eb3fa71.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

payin/src/main/java/com/payabli/sdk/payin/payment/PayabliPayInPaymentFlow.kt:63

• The Job lookup is nullable, so in the (mis)configuration where the provided CoroutineScope has no Job, the draft will never be cleared and may retain sensitive typed data longer than intended. Consider failing fast (require a Job), or ensure the flow always constructs/owns a scope with a Job and uses that for lifecycle cleanup.

        scope.coroutineContext[Job]?.invokeOnCompletion { draft.clear() }

payin/src/main/java/com/payabli/sdk/payin/ui/PayInFormContent.kt:82

• draft.seed(...) can mutate Compose snapshot state during composition (it clears/fills the state map and updates mutableStateOf fields). This pattern is fragile and often triggers Compose runtime/lint warnings about state writes in composition, and can introduce hard-to-debug recomposition behavior when future refactors cause reads before the write. A safer approach is to move seeding into an effect keyed by (configuration, initialValues) (and render only once seeded), or otherwise structure seed so it doesn’t perform snapshot state writes directly from the composition phase.

    // Before anything below reads the draft, and on every composition: a caller replacing the configuration or
    // the values starts the form again from what they handed over, and re-entering a composition with the same
    // pair keeps what the payer typed.
    draft.seed(configuration, initialValues)

payin/src/test/java/com/payabli/sdk/payin/form/PayInFormDraftTest.kt:184

• This test relies on GC timing/behavior (System.gc(), allocation pressure, sleeps) and can be flaky across JVMs/CI environments. To make it deterministic, consider asserting the implementation behavior directly (e.g., that the draft stores only a derived key and does not retain the PayInFormValues reference), or reworking the test to avoid depending on GC guarantees.

        repeat(GC_ATTEMPTS) {
            if (watched.get() == null) return
            System.gc()
            // Real garbage each round, so a collection the runtime is free to skip has a reason to happen. The
            // previous array becomes unreachable as this one replaces it.
            ballast = ByteArray(BALLAST_BYTES)
            Thread.sleep(GC_PAUSE_MILLIS)
        }

        fail("the draft still holds the values it was seeded from")

[Alex Arguello (alex-arguello)]

Suppressed comment · payin/src/test/java/com/payabli/sdk/payin/form/PayInFormDraftTest.kt:184 · GC-dependent retention check

This test relies on GC timing/behavior (System.gc(), allocation pressure, sleeps) and can be flaky across JVMs/CI environments. To make it deterministic, consider asserting the implementation behavior directly (e.g., that the draft stores only a derived key and does not retain the PayInFormValues reference), or reworking the test to avoid depending on GC guarantees.

Fixed · 402fa53

The check now reads the draft's own fields and fails naming any that holds a PayInFormValues, so it answers in the same millisecond every run. It also covers every field rather than one by name, which the collector-based version could not do.

Change System.gc, the allocation ballast and the sleeps are gone.
Test Putting the object back in the key fails it with the draft holds the caller's values in [seededFrom], and fails nothing else out of 13.

Suppressed comment · payin/src/main/java/com/payabli/sdk/payin/payment/PayabliPayInPaymentFlow.kt:63 · nullable Job lookup

The Job lookup is nullable, so in the (mis)configuration where the provided CoroutineScope has no Job, the draft will never be cleared and may retain sensitive typed data longer than intended. Consider failing fast (require a Job), or ensure the flow always constructs/owns a scope with a Job and uses that for lifecycle cleanup.

Declined

Answered on an earlier round with the same evidence. CoroutineScope(context) adds a Job() when the context has none, so the configuration described is reachable only through a hand-written CoroutineScope or GlobalScope. Neither can be cancelled at all: CoroutineScope.cancel raises Scope cannot be cancelled because it does not have a job. There is no completion to observe, so nothing is being missed.

Owning a scope inside the flow is the design this SDK settled against, because cancelling a scope does not un-charge a card and the host decides whether a payment dies with its screen.

Commit none. Nothing in this PR changes.
Test none. Only a real scope cancellation reaches the handler.

Suppressed comment · payin/src/main/java/com/payabli/sdk/payin/ui/PayInFormContent.kt:82 · snapshot writes during composition

draft.seed(...) can mutate Compose snapshot state during composition (it clears/fills the state map and updates mutableStateOf fields). This pattern is fragile and often triggers Compose runtime/lint warnings about state writes in composition, and can introduce hard-to-debug recomposition behavior when future refactors cause reads before the write.

Declined

Also answered earlier. The write precedes every read of the draft in that composition, so it cannot invalidate the composition making it, and lint reports nothing on this file.

Moving it into an effect was run rather than reasoned about: with LaunchedEffect(configuration, initialValues), all six of PayInFormSeedInstrumentedTest fail with a form draft was read before it was seeded, because the first composition reads the draft before the effect fires. Rendering only once seeded means a prefilled form shows empty for a frame, which is the feature initialValues exists to provide.

The concern about a later refactor reading before the write is the reason PayInFormDraft.method raises instead of answering: an unseeded read fails loudly rather than picking an instrument of its own, and readingTheInstrumentBeforeSeedingFails pins it.

Commit none. Nothing in this PR changes.
Test PayInFormSeedInstrumentedTest, six tests on four targets, covers the behaviour either shape has to keep.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

core/src/sharedTest/java/com/payabli/sdk/core/logging/RecordingLogSink.kt:33

• CopyOnWriteArrayList is optimized for many reads / few writes; each add copies the backing array, which can significantly slow tests that log heavily (and can add GC pressure). For a write-heavy sink, prefer a concurrent queue (e.g., ConcurrentLinkedQueue) or a synchronized list (Collections.synchronizedList(...)) and expose a snapshot for assertions when needed.

    /**
     * Thread-safe, because a transport under test writes from every request in flight at once.
     *
     * An `ArrayList` here loses records rather than reporting anything: eight writers appending 2000 lines each
     * left 4579 of 16000, and the same race throws out of `ArrayList.add` often enough to redden a run that has
     * nothing wrong with it. `LoopbackServer.requests` is a `CopyOnWriteArrayList` for the same reason.
     */
    val records: MutableList<Record> = CopyOnWriteArrayList()

payin/src/main/java/com/payabli/sdk/payin/ui/PayInFormContent.kt:82

• draft.seed(...) can mutate Compose state during composition (entered.clear(), chosen = ..., etc.). Even with the idempotency guard, this pattern can trigger Compose warnings and is easy to regress into recomposition loops. A safer approach is to (a) make PayInFormDraft constructible in a pre-seeded state (so the first composition can read it without mutation), or (b) allow method to be nullable/derivable and run seed from a side-effect (LaunchedEffect / SideEffect) while rendering a consistent initial UI until seeding completes.

    // Before anything below reads the draft, and on every composition: a caller replacing the configuration or
    // the values starts the form again from what they handed over, and re-entering a composition with the same
    // pair keeps what the payer typed.
    draft.seed(configuration, initialValues)

payin/src/main/java/com/payabli/sdk/payin/form/PayInFormDraft.kt:81

• Using initialValues?.hashCode() as the seed identity intentionally trades correctness for not retaining PayInFormValues, but it makes seed-change detection vulnerable to hash collisions (causing a genuinely new seed to be treated as the old one). To reduce collision risk without retaining the original values, consider storing a more collision-resistant fingerprint (e.g., a 64-bit/128-bit content hash derived from method + field/value hashes) or storing a redacted copy of PayInFormValues where sensitive fields are removed before caching for equality.

        val key = configuration to initialValues?.hashCode()
        if (seededFrom == key) return
        seededFrom = key

core/src/test/java/com/payabli/sdk/core/logging/RecordingLogSinkConcurrencyTest.kt:36

• If the await assertion fails (or any earlier assertion throws), the executor may not be shut down, potentially leaking threads to later tests. Wrap the executor lifecycle in try/finally and call shutdownNow() (or shutdown() + bounded awaitTermination) in the finally. Also prefer assertTrue(\"a writer never finished\", finished.await(...)) for clearer intent.

        val pool = Executors.newFixedThreadPool(WRITERS)
        val start = CountDownLatch(1)
        val finished = CountDownLatch(WRITERS)

        repeat(WRITERS) {
            pool.submit {
                start.await()
                repeat(WRITES_EACH) { sink.write(LogLevel.DEBUG, "tag", "message") }
                finished.countDown()
            }
        }
        start.countDown()

        assertEquals("a writer never finished", true, finished.await(TIMEOUT_SECONDS, TimeUnit.SECONDS))
        pool.shutdown()
        assertEquals(WRITERS * WRITES_EACH, sink.records.size)
    }

[Alex Arguello (alex-arguello)]

Suppressed comment · core/src/test/java/com/payabli/sdk/core/logging/RecordingLogSinkConcurrencyTest.kt:36 · executor left running on a failed assertion

If the await assertion fails (or any earlier assertion throws), the executor may not be shut down, potentially leaking threads to later tests. Wrap the executor lifecycle in try/finally and call shutdownNow() (or shutdown() + bounded awaitTermination) in the finally. Also prefer assertTrue(...).

Fixed · f329737

Change The pool's lifecycle is in a try/finally with shutdownNow, and the latch check is assertTrue rather than an equality against a boolean.
Test none beyond the test itself. The leak only appears when an assertion in it fails, which is the path the finally now covers.

Suppressed comment · core/src/sharedTest/java/com/payabli/sdk/core/logging/RecordingLogSink.kt:33 · CopyOnWriteArrayList for a write-heavy sink

CopyOnWriteArrayList is optimized for many reads / few writes; each add copies the backing array, which can significantly slow tests that log heavily (and can add GC pressure). For a write-heavy sink, prefer a concurrent queue (e.g., ConcurrentLinkedQueue) or a synchronized list (Collections.synchronizedList(...)).

Declined

Measured on the suite this affects. :core runs 449 tests in 5.59s with CopyOnWriteArrayList and 5.61s with Collections.synchronizedList, so the copying is not observable at the volumes these tests log. No test in the module writes enough records for the quadratic term to appear.

A synchronized list also costs something the copying does not. Six call sites iterate records, and Collections.synchronizedList documents that iteration must be inside synchronized (list) { }. A test reading records while a request is still in flight would meet a ConcurrentModificationException, which is the class of intermittent failure this change exists to remove. CopyOnWriteArrayList hands out a snapshot iterator and never throws.

LoopbackServer.requests is a CopyOnWriteArrayList for the same pair of reasons.

Commit none. Nothing in this PR changes.
Test RecordingLogSinkConcurrencyTest covers the guarantee either type would have to provide.

Suppressed comment · payin/src/main/java/com/payabli/sdk/payin/form/PayInFormDraft.kt:81 · a wider fingerprint for the seed key

Using initialValues?.hashCode() as the seed identity intentionally trades correctness for not retaining PayInFormValues, but it makes seed-change detection vulnerable to hash collisions. Consider storing a more collision-resistant fingerprint (e.g., a 64-bit/128-bit content hash derived from method + field/value hashes) or storing a redacted copy of PayInFormValues where sensitive fields are removed before caching for equality.

Declined, and the trade runs the other way from the one described.

A 64-bit value derived from field and value hashes does not reduce the collision risk. Each value contributes String.hashCode, which is 32 bits, so two seeds differing in one field collide at the same rate however wide the accumulator is. Widening the output without widening the input buys nothing.

A fingerprint over the characters would genuinely be stronger, and that is the reason not to hold one. A 16-digit card number lives in a space of about 2^50. A 32-bit digest of one has roughly 2^18 preimages in that space, so it names no particular card. A 64-bit digest has fewer than one, so it identifies the card and a memory dump becomes an offline search rather than a dead end. The stronger the fingerprint, the better an oracle it is for the value it was introduced to avoid holding.

The redacted-copy suggestion was declined on an earlier round: dropping PayInSensitiveFields.CLEARED_ON_OUTCOME from the comparison means a caller swapping one stored card for another changes only CardNumber, and the form ignores the new one.

What a collision costs is bounded: a caller hands over genuinely different values, the form keeps the ones it has, and the payer edits the field. That is a visible, recoverable outcome at roughly 2^-32 per seed change, against a card number held for the life of a screen.

Commit none. Nothing in this PR changes.
Test PayInFormDraftTest, 13 tests, covers seeding, re-seeding and non-retention. A collision cannot be provoked without constructing one, which would assert the arithmetic rather than the behaviour.

Suppressed comment · payin/src/main/java/com/payabli/sdk/payin/ui/PayInFormContent.kt:82 · snapshot writes during composition

draft.seed(...) can mutate Compose state during composition (entered.clear(), chosen = ..., etc.). Even with the idempotency guard, this pattern can trigger Compose warnings and is easy to regress into recomposition loops.

Declined

Answered on two earlier rounds with the same evidence. The write precedes every read of the draft in that composition, so it cannot invalidate the composition making it, and lint reports nothing on the file.

Both alternatives were run rather than argued. With LaunchedEffect(configuration, initialValues), all six of PayInFormSeedInstrumentedTest fail with a form draft was read before it was seeded, because the first composition reads before the effect fires. Rendering a placeholder until seeding completes shows a prefilled form empty for a frame, which is the feature initialValues exists to provide.

Constructing the draft pre-seeded is the third suggestion and it does not reach: the draft is built by PayabliPayInPaymentFlow, which never sees a PayInFormConfiguration or the caller's values.

The regression the finding names is the reason PayInFormDraft.method raises rather than answering. A read before the write fails loudly instead of picking an instrument of its own, and readingTheInstrumentBeforeSeedingFails pins it.

Commit none. Nothing in this PR changes.
Test PayInFormSeedInstrumentedTest, six tests on four targets, plus readingTheInstrumentBeforeSeedingFails.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

payin/src/main/java/com/payabli/sdk/payin/form/PayInFormDraft.kt:81

• initialValues?.hashCode() is derived from the caller-provided values and may incorporate sensitive strings (e.g., PAN) into a long-lived fingerprint. Even though it’s not reversible, retaining a derived value of sensitive input can be problematic for some compliance/security interpretations. Consider using a non-sensitive seed revision token (passed separately), System.identityHashCode(initialValues) (trades off “equal content” detection), or a fingerprint that deliberately excludes sensitive fields (PayInSensitiveFields) so no PAN-derived data is kept.

        val key = configuration to initialValues?.hashCode()
        if (seededFrom == key) return
        seededFrom = key

payin/src/main/java/com/payabli/sdk/payin/ui/PayInFormContent.kt:82

• draft.seed(...) mutates Compose snapshot state and is invoked directly during composition. Even with the internal early-return, mutating state from the composable body is a side-effect pattern that can trigger Compose runtime warnings and makes recomposition behavior harder to reason about. If possible, move seeding to a Compose side-effect (SideEffect/LaunchedEffect driven by a stable key) or restructure so the draft is seeded before the composable reads it (e.g., by the flow/host), keeping composition free of state mutations.

    // Before anything below reads the draft, and on every composition: a caller replacing the configuration or
    // the values starts the form again from what they handed over, and re-entering a composition with the same
    // pair keeps what the payer typed.
    draft.seed(configuration, initialValues)

    val method = draft.method
    val typed = draft.typed

[Alex Arguello (alex-arguello)]

Suppressed comment · payin/src/main/java/com/payabli/sdk/payin/form/PayInFormDraft.kt:81 · a PAN-derived value kept as the seed key

initialValues?.hashCode() is derived from the caller-provided values and may incorporate sensitive strings (e.g., PAN) into a long-lived fingerprint. Even though it's not reversible, retaining a derived value of sensitive input can be problematic for some compliance/security interpretations. Consider using a non-sensitive seed revision token (passed separately), System.identityHashCode(initialValues) (trades off "equal content" detection), or a fingerprint that deliberately excludes sensitive fields (PayInSensitiveFields) so no PAN-derived data is kept.

Declined, on each of the three alternatives, and one part of the premise is worth stating exactly.

System.identityHashCode breaks a host pattern that is already tested. A caller building its PayInFormValues inline hands over a different instance on every composition, so an identity key differs every time, the draft refills every composition, and the form empties as the payer types. PayInFormDraftTest.anEqualConfigurationRebuiltByTheCallerIsNotANewOne fails against it.

Excluding PayInSensitiveFields.CLEARED_ON_OUTCOME was declined on the first round and for the same reason: a caller swapping one stored card for another changes only CardNumber, so the form would ignore the new card.

A separately-passed revision token is a public parameter every integrator has to maintain correctly, and getting it wrong is silent in both directions: stale means the payer's typing is wiped, fresh-when-unchanged means a prefill never applies.

On what is actually retained: 32 bits, in memory, never persisted and never logged. A 16-digit card number lives in a space of about 2^50, so a 32-bit digest of one has on the order of 2^18 preimages there and identifies no particular card. The reason not to widen it is the same arithmetic, and it was answered on the previous round.

The one real point underneath this is that the key outlives the card number itself, since clearInstrument empties entered on an outcome and the key stays until the screen ends. That is the trade the KDoc on seededFrom states.

Commit none. Nothing in this PR changes.
Test PayInFormDraftTest, 13 tests, including the rebuilt-configuration case that the identity-hash alternative fails.

Suppressed comment · payin/src/main/java/com/payabli/sdk/payin/ui/PayInFormContent.kt:82 · snapshot writes during composition

draft.seed(...) mutates Compose snapshot state and is invoked directly during composition. If possible, move seeding to a Compose side-effect (SideEffect/LaunchedEffect driven by a stable key) or restructure so the draft is seeded before the composable reads it (e.g., by the flow/host), keeping composition free of state mutations.

Declined

Third report, and the two alternatives have both been run.

LaunchedEffect(configuration, initialValues) fails all six of PayInFormSeedInstrumentedTest with a form draft was read before it was seeded, because the first composition reads the draft before the effect fires. SideEffect lands in the same place, and its own KDoc scopes it to objects not backed by snapshots.

Seeding from the flow or the host is the new half of the suggestion. PayabliPayInPaymentFlow builds the draft and never sees a PayInFormConfiguration or the caller's values; both arrive at PayabliPayInForm as parameters, one composition below. Moving the seed there means passing the configuration into the flow's constructor, which makes a host hand the same object to two places and keep them in step.

The write lands before every read of the draft in that composition, so it cannot invalidate the composition making it, and lint reports nothing on the file.

Commit none. Nothing in this PR changes.
Test PayInFormSeedInstrumentedTest, six tests on four targets, plus PayInFormDraftTest.readingTheInstrumentBeforeSeedingFails, which is what makes a read-before-write fail loudly rather than pick an instrument of its own.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

core/src/sharedTest/java/com/payabli/sdk/core/logging/RecordingLogSink.kt:33

• CopyOnWriteArrayList is optimized for many readers / few writers, but RecordingLogSink is write-heavy (every log line is an add, which copies the backing array). Even in tests this can become a noticeable slowdown as parallel request counts grow. Prefer a write-optimized thread-safe structure (e.g., Collections.synchronizedList(mutableListOf()) or ConcurrentLinkedQueue) and adapt read/iteration accordingly.

    val records: MutableList<Record> = CopyOnWriteArrayList()

[Alex Arguello (alex-arguello)]

Fixed, recommendation corrected · 8c7a877

The transient method read failure is real, and the earlier replies on this thread were wrong to treat the ordering inside clear as covering it. Reproduced by seeding and reading in a loop on one thread while clearing on another: IllegalStateException: a form draft was read before it was seeded, within three seconds on the first run.

The interleaving the ordering misses: a reader passes seed's early return while seededFrom still matches, and clear then nulls the instrument before that reader reaches method. Nulling seededFrom first only helps a reader that has not yet made its check.

The threading half of the finding is still not what is happening. SnapshotStateMap mutates under a lock with a compare-and-retry loop, so nothing is corrupted and no snapshot exception is raised. entered.clear() from another thread is safe. The failure is a nullable field read after being nulled, and it would happen identically with no Compose state involved.

So neither remedy is the fix. Dispatching to the main thread cannot use the host's scope, which has completed by the time invokeOnCompletion runs, and Snapshot.withMutableSnapshot throws SnapshotApplyConflictException on a concurrent write, which is a crash at teardown rather than a fix for one. Taking the draft off Compose state removes what makes the form recompose as the payer types.

Change clear leaves the chosen instrument alone. Which tab is on screen is not the payer's data and does not need clearing, and the next seed sets it before anything reads it, so no interleaving leaves a reader without one. seededFrom becomes @Volatile, being a plain field written by the completing thread and read by the composition.

Test PayInFormDraftTest.aClearedDraftStillAnswersWhichInstrumentIsOnScreen. Restoring the null fails it and nothing else, over 14. The loop that reproduced the race was used to prove the defect and then removed, because a probabilistic test is the shape this review has twice asked to be replaced.

[Alex Arguello (alex-arguello)]

Suppressed comment · core/src/sharedTest/java/com/payabli/sdk/core/logging/RecordingLogSink.kt:33 · CopyOnWriteArrayList for a write-heavy sink

CopyOnWriteArrayList is optimized for many readers / few writers, but RecordingLogSink is write-heavy (every log line is an add, which copies the backing array). Even in tests this can become a noticeable slowdown as parallel request counts grow. Prefer a write-optimized thread-safe structure (e.g., Collections.synchronizedList(mutableListOf()) or ConcurrentLinkedQueue) and adapt read/iteration accordingly.

Declined, same as the previous round, with the measurement repeated here since it is the answer.

:core runs 449 tests in 5.59s with CopyOnWriteArrayList and 5.61s with Collections.synchronizedList. No test in the module logs enough for the copying to appear.

The swap also costs something. Six call sites iterate records, and Collections.synchronizedList requires iteration inside synchronized (list) { }; a test reading while a request is still in flight would meet a ConcurrentModificationException, which is the class of intermittent failure this change removed. A ConcurrentLinkedQueue is not a MutableList and would rewrite all nineteen call sites for the same outcome.

LoopbackServer.requests is a CopyOnWriteArrayList for both reasons.

Commit none. Nothing in this PR changes.
Test RecordingLogSinkConcurrencyTest covers the guarantee any of the three would have to provide.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

payin/src/main/java/com/payabli/sdk/payin/form/PayInFormDraft.kt:88

• Using initialValues?.hashCode() as the only discriminator for value changes can mis-detect changes due to hash collisions, causing the draft to incorrectly retain prior typed state when the caller actually provided new values. Mandatory fix: use a collision-resistant signature (e.g., a stable digest/long hash over method + ordered field/value pairs) or store a non-sensitive structural key provided by the caller (e.g., initialValuesKey) so “new values” cannot be skipped due to an Int collision.

    fun seed(
        configuration: PayInFormConfiguration,
        initialValues: PayInFormValues?,
    ) {
        val key = configuration to initialValues?.hashCode()
        if (seededFrom == key) return
        seededFrom = key

        chosen = initialValues?.method?.takeIf { it in configuration.methodsOffered } ?: configuration.startingMethod
        entered.clear()
        initialValues?.values?.forEach { (field, value) -> if (value.isNotEmpty()) entered[field] = value }
        rejectedFields = emptyMap()
    }

core/src/sharedTest/java/com/payabli/sdk/core/logging/RecordingLogSink.kt:33

• CopyOnWriteArrayList is thread-safe but is optimized for read-mostly workloads; each add copies the entire backing array, which is disproportionately expensive for a log sink where writes dominate (especially under concurrency). Suggested fix: switch to a structure designed for frequent concurrent appends (e.g., ConcurrentLinkedQueue) or a Collections.synchronizedList(mutableListOf()) if you need list semantics; then adapt records exposure accordingly.

    val records: MutableList<Record> = CopyOnWriteArrayList()

core/src/test/java/com/payabli/sdk/core/logging/RecordingLogSinkConcurrencyTest.kt:39

• shutdownNow() does not guarantee thread termination before the test ends; if a worker ignores interrupts (or is between interrupt checks), the thread can leak into subsequent tests. Suggested fix: follow shutdownNow() with awaitTermination(...) (or use shutdown() + awaitTermination(...) with a timeout) to reliably ensure the pool is fully stopped before returning.

        } finally {
            // In a finally, so a failed assertion leaves no writers behind for whatever test runs next.
            pool.shutdownNow()
        }

[Alex Arguello (alex-arguello)]

Declined

The race this named is fixed, in 8c7a877 and on the reply above. What remains here is the threading claim, answered on four rounds and unchanged.

Writing snapshot state off the main thread does not throw. SnapshotStateMap mutates through a compare-and-retry loop under an explicit lock, and that lock's own comment in SnapshotStateMap.kt, androidx.compose.runtime 1.10.4, describes concurrent writers as ordinary rather than as a violation:

As there is already contention on the global snapshot lock to write so the additional contention introduced by this lock is nominal.

The failure that was real had nothing to do with snapshots: a nullable field was read after being nulled, and it would have happened identically with plain vars. That is why the fix is to stop nulling it rather than to move where the nulling runs.

Both remedies still cost more than they buy. Dispatchers.Main.immediate needs a live scope, and invokeOnCompletion runs after the host's has completed, so the clear would never execute. Snapshot.withMutableSnapshot throws SnapshotApplyConflictException when a concurrent write lands inside its apply, which is a crash at teardown in exactly the case that motivates it.

Commit 8c7a877 for the race. Nothing further changes.
Test PayInFormDraftTest.aClearedDraftStillAnswersWhichInstrumentIsOnScreen, which fails against the null and nothing else.

[Alex Arguello (alex-arguello)]

Suppressed comment · payin/src/main/java/com/payabli/sdk/payin/form/PayInFormDraft.kt:88 · collision-resistant seed signature

Using initialValues?.hashCode() as the only discriminator for value changes can mis-detect changes due to hash collisions, causing the draft to incorrectly retain prior typed state when the caller actually provided new values. Mandatory fix: use a collision-resistant signature (e.g., a stable digest/long hash over method + ordered field/value pairs) or store a non-sensitive structural key provided by the caller (e.g., initialValuesKey).

Fixed · d6d0787

Two corrections to the earlier decline on this, both of which changed the answer.

A wider accumulator folded from String.hashCode values would not have helped, because 32 bits of input is what bounds it. A digest over the characters is a different thing and does help, and that is the reading this now implements.

The consequence was also understated before. A missed refill does not leave a stale prefill for the payer to edit; it submits the card the caller replaced.

Change The key is a 64-bit FNV-style digest over the method and each field's characters, summed per field so the order a caller assembled its map in is not a change. It is salted per draft, so the retained value cannot be computed elsewhere and matched against a table of card numbers, which is the reason a stronger digest was resisted at all.

Test Three. swappingOneStoredCardForAnotherFillsInTheNewOne and theOrderACallerBuiltItsValuesInIsNotAChange in PayInFormDraftTest, and capturingTheCardASecondSeedReplacedTheFirstWith in the live tier, which seeds one card, seeds another, and captures against a real environment, because a wrong answer here is a payment on the wrong card. Measured over the draft rather than the arithmetic: 0 missed swaps in 2,000,000 pairs of unrelated cards and 0 in 576,000 pairs one digit apart.

Suppressed comment · core/src/test/java/com/payabli/sdk/core/logging/RecordingLogSinkConcurrencyTest.kt:39 · shutdownNow does not await termination

shutdownNow() does not guarantee thread termination before the test ends; if a worker ignores interrupts (or is between interrupt checks), the thread can leak into subsequent tests. Suggested fix: follow shutdownNow() with awaitTermination(...).

Fixed · 377b0b4

Change awaitTermination is asserted after shutdownNow, on the bound the latch already uses.
Test none beyond the test itself, which is the thing being bounded.

Suppressed comment · core/src/sharedTest/java/com/payabli/sdk/core/logging/RecordingLogSink.kt:33 · CopyOnWriteArrayList for a write-heavy sink

CopyOnWriteArrayList is thread-safe but is optimized for read-mostly workloads; each add copies the entire backing array, which is disproportionately expensive for a log sink where writes dominate.

Declined, third report, and the measurement is the answer: :core runs 449 tests in 5.59s with CopyOnWriteArrayList and 5.61s with Collections.synchronizedList. No test in the module logs enough for the copying to appear.

The alternatives also cost something. Six call sites iterate records, and Collections.synchronizedList requires iteration inside synchronized (list) { }, so a test reading while a request is in flight would meet a ConcurrentModificationException. A ConcurrentLinkedQueue is not a MutableList and rewrites all nineteen call sites for the same outcome.

Commit none. Nothing in this PR changes.
Test RecordingLogSinkConcurrencyTest covers the guarantee any of the three would have to provide.

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

payin/src/main/java/com/payabli/sdk/payin/form/PayInFormDraft.kt:150

• The digest is used as an equality key to decide whether seed() should re-fill. Summing per-field digests (total + ...) materially increases collision likelihood compared to a normal fold, which can make different seeds look identical and cause the draft to ignore changed input (the exact failure this key is meant to prevent). Prefer a canonical order (e.g., sort by field.ordinal) and then fold with mix(...) so the hash remains order-independent without relying on commutative addition.

    private fun PayInFormValues.digest(): Long =
        values.entries.fold(mix(salt, method.ordinal.toLong())) { total, (field, value) ->
            total + value.fold(mix(salt, field.ordinal.toLong())) { running, char -> mix(running, char.code.toLong()) }
        }

payin/src/main/java/com/payabli/sdk/payin/ui/PayInFormContent.kt:82

• draft.seed(...) mutates Compose state (chosen, entered, rejectedFields) during composition. Even though it currently runs before this composable reads the draft, state writes during composition are fragile and can break with refactors (and may trigger snapshot/runtime warnings). Consider moving this call into a Compose side-effect (SideEffect or LaunchedEffect keyed by configuration + a stable representation of initialValues) while keeping the seed-key comparison inside PayInFormDraft.

    // Before anything below reads the draft, and on every composition: a caller replacing the configuration or
    // the values starts the form again from what they handed over, and re-entering a composition with the same
    // pair keeps what the payer typed.
    draft.seed(configuration, initialValues)

core/src/sharedTest/java/com/payabli/sdk/core/logging/RecordingLogSink.kt:33

• CopyOnWriteArrayList makes every add copy the backing array; for log sinks that can receive many writes (especially in concurrent transport tests), this can become a noticeable overhead. A better fit is a write-optimized thread-safe structure such as Collections.synchronizedList(mutableListOf()) or ConcurrentLinkedQueue, converting to a list only when needed for assertions.

    val records: MutableList<Record> = CopyOnWriteArrayList()

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
96.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud