feat: add a compose friendly constructor to `SEPADirectDebit` by noguier · Pull Request #1704 · braintree/braintree_android

noguier · 2026-09-11T20:03:36Z

Summary of changes

Create compose-friendly constructor to SEPADirectDebitLauncher
wire the new constructor in the demo app to ensure it works as expected
add a changelog

AI Usage

Which AI Agent Was Used?

Copilot
Claude
Other (Type Name Here)

How was AI used?
test creation

Estimated AI Code Contribution

less than 30%
30 - 60%
60 - 100%

Checklist

Added a changelog entry
Tested and confirmed payment flows affected by this change are functioning as expected

Authors

List GitHub usernames for everyone who contributed to this pull request.
@noguier

github-actions · 2026-09-11T20:05:48Z

Dependency Review

✅ No vulnerabilities found.

Scanned Files

None

Copilot

🟢 Approval recommended

The only finding is a non-blocking test assertion nit.

Pull request overview

Adds a Compose-friendly ActivityResultRegistry constructor to SEPADirectDebitLauncher, with a unit test and changelog entry.

Changes:

Added registry-based launcher construction.
Added constructor test coverage.
Documented the change in CHANGELOG.md.

File summaries

File	Summary
`SEPADirectDebit/src/test/java/com/braintreepayments/api/sepadirectdebit/SEPADirectDebitLauncherUnitTest.kt`	Tests registry-based construction; includes a minor assertion nit.
`SEPADirectDebit/src/main/java/com/braintreepayments/api/sepadirectdebit/SEPADirectDebitLauncher.kt`	Adds the Compose-compatible constructor.
`CHANGELOG.md`	Documents the new constructor.

Review details

Suppressed comments (1)

SEPADirectDebit/src/test/java/com/braintreepayments/api/sepadirectdebit/SEPADirectDebitLauncherUnitTest.kt:43

This assertion only proves that Kotlin returned a non-null object, so it would still pass if this overload ignored registry and constructed the default BrowserSwitchClient. Please verify the registry-backed browser-switch client is actually used (or remove the test if compile-only coverage is intended), so the new Compose wiring is protected from regressions.

        val launcher = SEPADirectDebitLauncher(registry)

        assertNotNull(launcher)

Files reviewed: 3/3 changed files
Comments generated: 0
Review effort level: Lite

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

buzzamus · 2026-09-15T15:24:12Z

+     * Used to launch the SEPA mandate in a web browser and deliver results. Use it in Compose flows
+     * or anywhere else an [ActivityResultCaller] is not available.
+     * @param registry ActivityResultRegistry parameter. It will be passed to BrowserSwitchClient.
+     */


This is a nice, helpful comment!

buzzamus · 2026-09-15T15:24:54Z

+
+        assertNotNull(launcher)
+    }
+


This is a question coming from my lack of compose knowledge, but is this type of test good enough to confirm the compose constructor is working as intended?

great question, i think I was mostly trying to test here if the constructor can be constructed with ActivityResultCaller without crashing. Most of the hard work is passed to BrowserSwitchClient so it is hard to test anything besides launcher creation. I can possibly mock the creation of BrowserSwitchClient with the newly created launcher, do you think it will be better?

I think that question is probably best answered by you and other Android platform specific folks since I don't know the common best practices for compose UI tests and how much is necessary

When I read the text of the test, "creates a valid launcher", I think here I would at least expect some more assertions on the launcher itself to verify it is "valid". If all we wanted was a test that the launcher gets created when being passed the expected params, that kind of feels like a low value test to me unless there is something specific you're looking for having been set up from calling the constructor.

Yeah that makes sense and sort of what I was curious about: Are there additional things that could be asserted to see if any attributes can be verified, or is this comprehensive enough

Hi all, i removed this test, and instead wired a new constructor in the demo app, I dont think we can add a useful unit test for this change since all the constructor is doing is passing the registry to browserSwitchClient, that then does all the job. I feel like wiring the new constructor is comprehensive enough to ensure the flow works as expected. Let me know what you think :)

Copilot

🟡 Changes recommended

The test should verify that the supplied registry is actually used.

Get a fresh assessment by requesting another Copilot review.

Review details

Files reviewed: 3/3 changed files
Comments generated: 1
Review effort level: Lite

saralvasquez

These changes seem simple enough, but could I get a bit of context on why we are adding this? The ticket for this referenced the acceptance criteria but didn't give much explanation otherwise

I think I also second the comments on testing. I wonder if there would be a way to do extra testing on this by possibly adding to the demo app or something

noguier · 2026-09-18T22:38:00Z

hey @saralvasquez , thank you for your question! this ticket came as a follow up for this ask: #1662. Here we are asked to do the compose for GooglePay, but upon investigation I found that SEPADirectDebit and some other launchers can also use compose friendly constructor :)

I think adding that compose for the demo might be a good middle ground in testing the constructor out, I will add that!

Copilot

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1

Open (2)

Avoid ambiguous overload for nullable launcher constructor calls · New
Add unit test for registry-based launcher constructor · New

Resolved since last review (1)

This assertion only verifies that Kotlin returned a non-null object; it would still pass if this…

noguier · 2026-09-21T21:16:28Z

+    constructor(registry: ActivityResultRegistry) : this(
+        browserSwitchClient = BrowserSwitchClient(registry)


These constructors match the same pattern that Venmo and PayPal Launchers have today.

We should still validate (and they may be broken on those other Launchers if this ends up being true)

Added unit tests to test the behavior outlined by copilot. The code didnt not crash, so I believe we can continue with the merge.

Copilot

Copilot review overview

🔵 Needs a closer look

One or more issues must be addressed before approval.

Review effort: Lite
Findings: 1 · 1

Open (2)

Avoid ambiguous overload for nullable launcher constructor calls
Add unit test for registry-based launcher constructor

# Conflicts: # CHANGELOG.md

…delete unnecessary test

Copilot

Copilot review overview

🟢 Approval recommended

No unresolved review issues were identified.

Review effort: Lite
Findings: 1

Open (1)

Avoid ambiguous overload for nullable launcher constructor calls

Resolved since last review (1)

Add unit test for registry-based launcher constructor

saralvasquez

Thanks for adding the change to the demo app! I think this is a good approach since testing these changes is challenging

agedd

looks good! 🚀

noguier requested review from a team and a lite review from Copilot September 11, 2026 20:03

noguier requested a review from a team as a code owner September 11, 2026 20:03

Copilot started reviewing on behalf of noguier September 11, 2026 20:04

View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

buzzamus reviewed Sep 15, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 17, 2026 22:33

Copilot started reviewing on behalf of noguier September 17, 2026 22:33

View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

Comment thread ...t/src/test/java/com/braintreepayments/api/sepadirectdebit/SEPADirectDebitLauncherUnitTest.kt Outdated

saralvasquez reviewed Sep 17, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 21, 2026 20:06

Copilot started reviewing on behalf of noguier September 21, 2026 20:06

View session

Copilot AI reviewed Sep 21, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 21, 2026 21:10

Copilot started reviewing on behalf of noguier September 21, 2026 21:11

View session

Copilot AI reviewed Sep 21, 2026

View reviewed changes

noguier added 3 commits September 22, 2026 13:47

feat: add a compose friendly constructor to SEPADirectDebit

225a89e

# Conflicts: # CHANGELOG.md

fix: add a new constructor to the demo app to ensure the flow works, …

1cdbed8

…delete unnecessary test

fix: add @JvmOverloads to an ActivityResultCaller constructor

42ef463

noguier force-pushed the compose-constructor-SEPA branch from c54b1c8 to 42ef463 Compare September 22, 2026 18:47

Copilot AI review requested due to automatic review settings September 22, 2026 18:47

Copilot started reviewing on behalf of noguier September 22, 2026 18:48

View session

Copilot AI reviewed Sep 22, 2026

View reviewed changes

saralvasquez approved these changes Sep 22, 2026

View reviewed changes

agedd approved these changes Sep 22, 2026

View reviewed changes

noguier merged commit e9e856c into main Sep 23, 2026
26 of 28 checks passed

noguier deleted the compose-constructor-SEPA branch September 23, 2026 16:02

		constructor(registry: ActivityResultRegistry) : this(
		browserSwitchClient = BrowserSwitchClient(registry)

Conversation

noguier commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

AI Usage

Checklist

Authors

Uh oh!

github-actions Bot commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dependency Review

Scanned Files

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Suppressed comments (1)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

saralvasquez left a comment

Choose a reason for hiding this comment

Uh oh!

noguier commented Sep 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

saralvasquez left a comment

Choose a reason for hiding this comment

Uh oh!

agedd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

noguier commented Sep 11, 2026 •

edited

Loading

github-actions Bot commented Sep 11, 2026 •

edited

Loading