v6 - POC - Form field architecture

[jreij]

Description

This is a POC for discussion, not a merge candidate. It is the combined view of a stack of four layers, so that the whole change can be read and tried in one place. Merging it would collapse the stack — the individual layer PRs come after this discussion, and this one gets closed. Trimmed tests are also still missing, see below.

Field order currently lives in three places that nothing keeps in sync: the sequence of if blocks in CardContent, the sequence of a copy(...) call in the reducer for focus priority, and implicitly in Compose's focus search for the keyboard's Next key. This makes three things impossible: the IME action cannot be derived at all (Compose fixes it when a field is created and offers no way to ask whether another focusable field follows), prefill cannot know where to send focus, and pay focuses the first invalid field in declaration order rather than the order the shopper reads.

This publishes the order from the state layer and has everything derive from it.

What that gives us, all working on a device:

• Pay highlights every error and focuses the first invalid field in visual order, and that field now keeps the error it just revealed. It used to hide it, because a requested focus was indistinguishable from a shopper tap.
• Every text field shows Next except the last visible one, which shows Done. Done closes the keyboard and does not submit.
• Card scanning moves focus to the first field it did not fill.
• Which fields are visible and in which order is answered once, so the screen reads no configuration at all.

Reviewing

Six commits in four layers, bottom-up. Each compiles and, except where stated, changes no behaviour.

1. Add the form unit that owns field order and focus requests — FormState, FormFieldId, FocusRequest, KeyboardAction and four pure functions. Nothing reads them yet.
2. Give the card form an explicit field order — card's field ids, the canonical sequence, and visibility derived from the same properties the producer reads. CardComponentState.form is computed on every read, so the order cannot go stale.
3. Keep the error on the field pay focuses — the one behaviour change in the stack, and the bug this work exists for. Also collapses eight UpdateXFocus intents into one.
4. Carry the pending focus request in a field's view state
5. Publish the card field order to the view state — still nothing reads it.
6. Render the card form from its field order — nine if blocks become one loop; fields move onto the focus request and the IME action.

Deliberately not here

• Tests were trimmed to get to a demo sooner. The configuration matrix, the exhaustive focus-rule tests and the Compose UI tests all come back before the layer PRs go up.
• Stored card, MB Way and BLIK still use the old focus flag, so isFocused is carried unchanged and marked. rg "TODO - Form fields cleanup" and rg "TODO - Form fields rollout" are the two worklists.
• No field is reordered. The mechanism exists; the sequence is the one we ship today.

Progress

✅ Phase 1 — Core: the error type (#2942)
➡️ Phases 2 to 5 — the POC (this PR)
Phase 6 — Roll out to stored card, MB Way, BLIK
Phase 7 — Cleanup and document

Checklist

• Code is unit tested
• Changes are tested manually

[gemini-code-assist]

Code Review

This pull request introduces a unified form state management system ('FormState', 'FormFieldId', 'FocusRequest', 'KeyboardAction') and integrates it into the Card component. It replaces individual field focus intents with a generic 'UpdateFieldFocus' intent, manages programmatic focus moves (such as after card scanning or validation failures), and dynamically derives field visibility and keyboard actions based on a canonical field order. The review feedback highlights two key areas for improvement: first, a suspending call to 'bringIntoView()' is wrapped in 'runCatching', which incorrectly suppresses 'CancellationException' and breaks structured cancellation; second, caching the computed 'form' property in 'CardViewStateProducer' would avoid redundant calculations during view state production.

[gemini-code-assist]

Suspending Call Wrapped in runCatching Suppresses Cancellation

bringIntoViewRequester.bringIntoView() is a suspending function. Wrapping a suspending call in runCatching is an anti-pattern because it catches and suppresses CancellationException, which breaks structured cancellation in Kotlin coroutines.

Since only focusRequester.requestFocus() (which is non-suspending) can throw if the node is not attached, we should run it in its own runCatching block, and handle bringIntoView() separately, ensuring CancellationException is rethrown if caught.

            runCatching { focusRequester.requestFocus() }
            // Taking focus scrolls the field into view on its own, but a field that already has focus is told
            // nothing and so would stay off screen. Asking directly covers both.
            try {
                bringIntoViewRequester.bringIntoView()
            } catch (e: kotlinx.coroutines.CancellationException) {
                throw e
            } catch (t: Throwable) {
                // Ignore other errors
            }

References

1. Using runCatching is acceptable for non-suspending calls when the error does not need to be handled, as there is no risk of catching and suppressing CancellationException in a non-suspending context.

[araratthehero]

A comment from my agent which can be useful.

When the cancellation is swallowed, the coroutine keeps going and still calls onFocusRequestConsumed(). FocusRequestConsumed matches on field id only, so a report from a dead request can clear a newer request for the same field and lose keepErrorHighlight — the error gets hidden again, which is the bug this PR fixes.

Gemini's patch cures that by accident (rethrowing means the callback never runs). Could you make it explicit instead — only wrap requestFocus(), leave bringIntoView() bare since it only throws CancellationException, and give the request an identity so a stale report can't match a new one?

Separate thing to check: on the happy path, gaining focus clears focusRequest, which changes the LaunchedEffect key and cancels bringIntoView() after a frame or two. Does the scroll ever actually complete?

[jreij]

@gemini-code-assist Fixed, with an adaptation: runCatching now wraps only requestFocus(), and bringIntoView() moved out of the effect into rememberCoroutineScope() instead of getting its own try/catch.

That was needed anyway — acting on the request clears it, which changes the LaunchedEffect key, so the scroll was being cancelled a frame or two in. Scoping it to the composition fixes that and closes the window where a cancelled effect could still report consumption.

[gemini-code-assist]

Thank you for the update, @jreij. Your adaptation to move bringIntoView() out of the LaunchedEffect and into the rememberCoroutineScope() is a much cleaner approach that correctly respects the lifecycle of the composition and avoids the cancellation issues we discussed. This change effectively resolves the issue where the scroll was being prematurely cancelled.

Suggested change

	runCatching {
	focusRequester.requestFocus()
	// Taking focus scrolls the field into view on its own, but a field that already has focus is told
	// nothing and so would stay off screen. Asking directly covers both.
	bringIntoViewRequester.bringIntoView()
	}
	runCatching { focusRequester.requestFocus() }
	// Taking focus scrolls the field into view on its own, but a field that already has focus is told
	// nothing and so would stay off screen. Asking directly covers both.
	coroutineScope.launch { bringIntoViewRequester.bringIntoView() }

[gemini-code-assist]

Efficiency Improvement: Avoid Redundant Form Computations

Currently, form is a computed property on CardComponentState that filters the list of visible fields on every read:

val form: FormState<CardFieldId>
    get() = FormState(order = visibleCardFields(this), focusRequest = focusRequest)

In produce(state), fieldViewState is called 8 times, and each call accesses form twice (once for keyboardActionFor and once for focusRequest). Along with state.form.order, this results in form being computed 17 times per state update.

To optimize this, we can compute form once at the start of produce(state) (e.g., val form = state.form) and pass it as a parameter to fieldViewState.

Suggested change

	private fun CardComponentState.fieldViewState(
	id: CardFieldId,
	field: TextInputComponentState,
	customTrailingIcon: TrailingIcon? = null,
	): TextInputViewState? = field.toViewState(
	customTrailingIcon = customTrailingIcon,
	keyboardAction = form.keyboardActionFor(id),
	focusRequest = form.focusRequest?.takeIf { it.id == id }?.let { FocusRequestToken(it) },
	)
	private fun CardComponentState.fieldViewState(
	id: CardFieldId,
	field: TextInputComponentState,
	form: FormState<CardFieldId>,
	customTrailingIcon: TrailingIcon? = null,
	): TextInputViewState? = field.toViewState(
	customTrailingIcon = customTrailingIcon,
	keyboardAction = form.keyboardActionFor(id),
	focusRequest = form.focusRequest?.takeIf { it.id == id }?.let { FocusRequestToken(it) },
	)

[jreij]

@gemini-code-assist Fixed, adapted: CardComponentState.form is memoised with lazy(PUBLICATION) rather than passed in as a parameter, so the reducer benefits too — it also reads form several times. PUBLICATION because the merchant owns the coroutine scope, so the same state can be read from more than one thread.

[gemini-code-assist]

That's a great point regarding the memoization of form. Using lazy(LazyThreadSafetyMode.PUBLICATION) is indeed the correct approach here, especially since the component runs in a coroutine scope that might be accessed from multiple threads. This ensures thread safety while avoiding the overhead of passing the FormState object through every fieldViewState call, keeping the code clean and efficient. Thanks for the update!

[araratthehero]

Does the focusRequest need to be inside of the FormState? I see that you read it only in CardViewStateProducer.kt:112 and there you could read directly from the CardComponentState.

[araratthehero]

If the focusRequest gets removed, then there will only be one field in the FormState, which makes the FormState redundant. Instead of the FormState, a simple List<Id> could be used?

[jreij]

The plan is to move focusRequest and the form state to core where we can handle it centrally. And later after this whole refactor, the next refactor would make FormState technically the whole view state (a list of fields + their data). So right now it looks like it can be moved to Card state but this refactor will achieve the opposite by extracting common things to core. WDYT?

[araratthehero]

Since the request has a type of Any, this can cause the fields to not skip recomposition. Would it make sense to make this @Immutable, like for State objects like InstallmentViewState, CardBrandViewState..?

[araratthehero]

Also does the type need to be Any?

[jreij]

Fixed, annotated with @Immutable.

As for Any, it has to stay since FocusRequestToken is in ui and core imports ui not the other way around. FocusRequestToken is only used as a param inside CheckoutTextField to make the LaunchedEffect work, and LaunchedEffect just needs a key of type Any.

So in order to not have a type Any being passed around in all text fields as well as not creating so much core logic inside ui, FocusRequestToken is a simple value class that makes sure we don't pass random stuff while keeping ui focused on UI. WDYT?

[araratthehero]

The same visibility check also happens in the CardFieldOrder, is this something we can centralize? This way it looks like we have two sources of truth for visibility checks.

[jreij]

Extracted TextInputComponentState.isVisible and used it in both this mapping and CardFieldOrder.

[araratthehero]

Can this visibility check be centralized too?

[jreij]

I agree that the form should be the source of truth for visibility but I think it's a bit overkill for now. The current structure is fragmented already, not every field has all its data in one place. And this refactor makes it a bit worse by adding another independent list.

Which is why we decided together that the next step is to refactor this further and create one big list of ordered fields + their data. So we won't even need these checks anymore. The view state producer will loop over the form and map each id and its data, so a field that isn't in the form is never mapped anyway. Adding checks now would require deleting them later anyway. WDYT?

[araratthehero]

A comment from my agent which can be useful.

When the cancellation is swallowed, the coroutine keeps going and still calls onFocusRequestConsumed(). FocusRequestConsumed matches on field id only, so a report from a dead request can clear a newer request for the same field and lose keepErrorHighlight — the error gets hidden again, which is the bug this PR fixes.

Gemini's patch cures that by accident (rethrowing means the callback never runs). Could you make it explicit instead — only wrap requestFocus(), leave bringIntoView() bare since it only throws CancellationException, and give the request an identity so a stale report can't match a new one?

Separate thing to check: on the happy path, gaining focus clears focusRequest, which changes the LaunchedEffect key and cancels bringIntoView() after a frame or two. Does the scroll ever actually complete?

[github-actions]

✅ No public API changes

[sonarqubecloud]

Quality Gate passed

Issues
14 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud