feat: add browser chrome customization - #21
Conversation
| let result = try await DodoCheckout.start( | ||
| checkoutUrl: checkoutUrl, | ||
| returnUrl: returnUrl, | ||
| customization: customization, |
There was a problem hiding this comment.
The iOS half of this feature does not exist yet, so this will not compile.
BrowserCustomization and the customization: parameter on DodoCheckout.start are not defined anywhere in this repo, nor in the Swift core that swift/ currently pins (v1.0.2, which is also the newest tag on the satellite repo). I grepped every .swift file in the tree: there is no struct/class/enum BrowserCustomization definition, and the pinned start signature is (checkoutUrl:returnUrl:onEvent:) only.
Because scripts/sync-ios-core.sh vendors the core out of the pinned submodule at build/release time, both this bridge and flutter/ios/Classes/DodoCheckoutFlutterPlugin.swift:66 will fail to compile against undefined symbols for every consumer who builds for iOS.
Worth flagging that nothing in this repo will catch it: all three CI jobs run on Linux (RN typecheck+jest, Android gradle test, Flutter test) and the release workflows only vendor and pack — they never compile iOS. So this would ship green and break at the integrator's build.
Per RELEASING.md, the ordering is: land + tag the change on dodopayments-mobile-sdk-ios, then bump the swift submodule pointer here, then release RN/Flutter. This PR does not move the submodule pointer.
| // Relative-luminance heuristic (unweighted by gamma — good enough for a | ||
| // binary black/white choice), not Chrome's own internal formula. | ||
| private fun iconColorFor(toolbarColor: Int?): Int { | ||
| if (toolbarColor == null) return Color.BLACK |
There was a problem hiding this comment.
The toolbarColor == null fallback to Color.BLACK produces an invisible icon in what is probably the most common way this option gets used.
The class doc is right that a custom bitmap gets no automatic contrast tinting, but colorScheme is never consulted — only toolbarColor is passed in from applyBrowserCustomization. So:
closeButtonStyle = BACK+colorScheme = DARK, notoolbarColor→ the SDK itself forces a dark toolbar, then draws a black arrow on it. The information needed to get this right is already incustomization, it is just not plumbed through.closeButtonStyle = BACK, nothing else set, device in dark mode →COLOR_SCHEME_SYSTEMgives a dark toolbar, same black-on-dark result.
Setting closeButtonStyle = BACK alone (no colors) seems like the single most likely usage, and it is broken for every dark-mode user.
Suggest passing colorScheme into backArrow and resolving in this order: explicit toolbarColor → luminance as today; else colorScheme LIGHT/DARK → black/white; else fall back to the host's own night-mode config (context.resources.configuration.uiMode and Configuration.UI_MODE_NIGHT_MASK) as the best available proxy for what the Custom Tab host will render.
Unrelated nit on the same function: the luminance threshold picks WHITE at exactly 0.5, which is fine, but the > 0.5 comparison on unweighted sRGB will read mid-tone brand colors as darker than they look. Not worth changing unless you see it in practice.
| if (has(key) && !isNull(key)) getString(key) else null | ||
|
|
||
| private fun JSONObject.optBooleanOrNull(key: String): Boolean? = | ||
| if (has(key) && !isNull(key)) getBoolean(key) else null |
There was a problem hiding this comment.
getBoolean throws JSONException for any value that is not a boolean (or the strings "true"/"false") — a number, object, or array all throw rather than returning null.
That exception escapes start() entirely. The runCatching on line 45 wraps only the JSONObject(...) construction, and toBrowserCustomization() is invoked while building checkoutParams on lines 41-47, i.e. before the scope.launch { try { ... } } on line 49. So a throw here propagates synchronously out of the TurboModule method and the promise is never settled — the JS start() hangs instead of rejecting.
This looks like an incomplete version of the intent already expressed one line up: a malformed customizationJson string degrades gracefully to defaults, so malformed contents presumably should too. It is also asymmetric with the iOS bridge, which uses as? Bool and safely yields nil for the same input.
TypeScript will stop most callers, but the types are not enforced at runtime and plenty of RN consumers are plain JS — { shareButtonEnabled: 1 } is enough to hang the promise.
Cheapest fix is to make the accessor total, e.g. runCatching { getBoolean(key) }.getOrNull(). Moving the whole CheckoutParams construction inside the guarded block would also work and would cover any future accessor added here.
Note optStringOrNull is fine as-is — getString coerces non-string values rather than throwing, and isNull already guards JSON null.
There was a problem hiding this comment.
Summary
Nicely structured feature. The null means "never call the platform setter" contract is the right call, and it is applied consistently through all four layers (Dart → Pigeon → Kotlin core; TS → JSON → both native bridges) rather than being asserted in one place and quietly defaulted in another. The rationale comments — especially why toolbarColor is not a shared top-level field and why customizationJson crosses the RN bridge as a string — answer the questions a reader would actually have. Test coverage of the mapping and the "explicit-but-empty bag still maps to null" case is thorough.
One blocking problem, plus two correctness issues worth fixing before release.
Blocking
The iOS implementation is missing, so both iOS targets will not compile. BrowserCustomization and the customization: argument on DodoCheckout.start are referenced by react-native/ios/DodoCheckoutReactNative.swift and flutter/ios/Classes/DodoCheckoutFlutterPlugin.swift, but are defined neither in this repo nor in the Swift core commit that swift/ pins. That pin is unchanged by this PR, and the pinned tag is also the newest one on the satellite repo. Since the core is vendored from the submodule at build/release time, iOS consumers of both packages get undefined symbols. Details and the RELEASING.md ordering are in an inline comment.
This will not be caught here: all three CI jobs are Linux-only and the release workflows vendor and pack without ever compiling iOS. Adding even a swift build of the submodule, or a pod lib lint, would turn this class of break into a red build instead of an integrator bug report.
Should fix
- Android back-arrow is invisible in dark mode.
CloseButtonIconsfalls back to black whenevertoolbarColoris null and never looks atcolorScheme, socloseButtonStyle = BACKon its own — plausibly the most common use — renders black-on-dark for dark-mode users. Inline comment has a suggested resolution order. - RN Android can hang the promise on malformed input. A
JSONExceptionout ofgetBooleanescapesstart()before the guarded block, so the promise is never settled. Inline comment has details; iOS handles the same input safely, so the two bridges currently disagree.
Minor
kotlin/README.mdstill showscheckout-android:1.0.2in its install snippet whilebuild.gradle.ktsmoves to1.1.0— easy to miss since the Customization section was added to that same file.- Adding a defaulted
customizationparameter to theCheckoutParamsdata class removes the generated 2-arg constructor and changescopy(). Source-compatible for Kotlin after a recompile, but binary-breaking for pre-compiled and Java callers of a published Maven artifact on a minor bump.@JvmOverloadson the constructor would preserve the old signature if Java consumers matter. - The close-button style value is spelled three ways across the SDKs:
CloseButtonStyle.standard(Flutter),'default'(RN),DEFAULT(Kotlin). The Dart deviation is explained by the reserved keyword, but RN and Kotlin could still have matched each other and Flutter's docs. - All three READMEs defer the option list to a
#appearance-customizationdocs anchor and document nothing inline, so those docs need to land with the release or the feature is undiscoverable.
Verified
- Pigeon output is genuinely regenerated and self-consistent: codec IDs 129–142 line up across the Dart, Kotlin, and Swift codecs, and field order matches. The wire IDs shifted for pre-existing types, which is safe only because all three are regenerated together — worth keeping in mind that the Dart and native halves must always ship as one version.
- ARGB handling is correct end to end. The generated Dart codec forces every
intto Int64, so the Kotlinas Long?cast is safe, and.toInt()truncation oftoARGB32()yields the right packed ARGB int including for alpha ≥ 0x80. - The Kotlin
Bundle↔Mapencoding round-trips through process death, and unrecognized enum names decode to null rather than a stale default — good forward-compatibility choice, and tested. - The
.mmchange is safe: assigning a nilcustomizationJsonthroughNSMutableDictionarysubscripting removes the key rather than throwing, and the Swift side treats the absent key as unset. Color.toARGB32()and the Dart switch expressions are available on the pinned Flutter 3.44 / Dart 3.12 toolchain;androidx.browser1.8.0 covers the Custom Tabs setters used.- React Native suite is green: 31 tests pass and
tsc --noEmitis clean.
I could not execute the Kotlin or Flutter suites in this environment (no JDK or Flutter toolchain available), so the Android and Dart assertions above are from reading the code and generated output rather than a run.
797daab to
0557df9
Compare
|
@dodo-squirrels Addressed both "should fix" items and two of the "minor" ones. Fixed
Investigated, not a real bug
Deferred, with reasoning
Still blocking, not yet resolved
Re-review appreciated on everything above — please approve if it all looks correct to you. |
|
Re-reviewed everything. Three fixes confirmed correct, and you were right about the dark-mode item — I was wrong to flag it. Details below. One thing still blocks approval, and it is the one you already called out. Confirmed fixedRN Android promise hang. Correct fix, and the mechanism holds up: Android's I also checked whether
Dark mode — withdrawing this oneYour on-device result is correct, and it reproduces straight from Chromium source. I traced it because the claim contradicted the original code comment, and the comment turned out to be simply stale:
So on any Chrome from ~2022 onward the colour Non-blocking, take it or leave it: the tint guarantee is Chromium-specific, and a non-Chromium Custom Tabs provider is not bound by it. Deferred — agreed on bothClose-button naming: agreed. Renaming shipped public API surface across two SDKs for cosmetic symmetry, when one of the three is pinned by a Dart reserved word anyway, is not worth breaking integrators over. Docs anchor: agreed, that resolves on the dodo-docs side. Still blockingThe iOS pin, exactly as you describe. Re-confirmed against the current state rather than taking it on trust:
So both iOS targets still fail to compile against the pinned core. Sequencing understood and not disputed — but it does mean I cannot approve for merge yet, since approving would put this one green button away from shipping a broken iOS build for both packages. Happy to approve the moment the submodule pointer moves. Restating one point from the first pass since it is what makes this invisible: all seven workflow jobs are Verification runAll 7 CI checks are green, including the Android |
6242096 to
cbf0a42
Compare
|
@dodo-squirrels bumped the swift submodule. good to merge now? approve if so |
cfb3c3f to
2126188
Compare
There was a problem hiding this comment.
Approving — the submodule pin was the only thing outstanding, and it resolves correctly. I re-verified it against the satellite repo rather than taking the bump on trust.
Blocker resolved
- The pin moves
9842302→a30ad72, which is exactly thev1.1.0tag and an ancestor of the satellite'smain— not a dangling branch commit that could be rewritten out from under us. iOS #4 is merged; the pinned commit is #5 (the 1.1.0 changelog) sitting on top of it. BrowserCustomizationexists at that commit withinit(dismissButtonStyle:barCollapsingEnabled:presentationStyle:colorScheme:), all optional and defaulted, andDodoCheckout.startis now(checkoutUrl:returnUrl:customization:onEvent:). Argument labels and order match both bridge call sites exactly.- Every enum case each mapper switches on exists in the core:
.done/.close/.cancel,.pageSheet/.fullScreen,.system/.light/.dark. Pigeon's generated Swift enums carry exactly those case sets, andNativeIosBrowserOptions' fields are allOptional, so Flutter'scase nilswitches are genuinely exhaustive rather than accidentally compiling. - The bump is purely additive — diffing the two pins shows no public declaration removed, only
BrowserCustomization.swiftadded plus the two call-through changes. So bumping the submodule to pick up this feature didn't silently regress any other API the bridges use, which was the other way this could have gone wrong.
Vendoring verified end to end
Ran both sync-ios-core.sh scripts against the real submodule: each copies 12 files including BrowserCustomization.swift. The core is flat under Sources/DodoCheckout/, so the non-recursive cp *.swift picks up all of it. Both release workflows check out with submodules: true and invoke the script with no path override, so a release vendors exactly the pinned commit — the pin really is authoritative here.
The satellite repo's own macOS CI is green on the pinned commit, so the core compiles as shipped.
Core semantics spot-check
Since this code now ships inside both published packages, I read the parts that changed: barCollapsingEnabled is correctly the one field applied at Configuration construction rather than in apply() (it can't change post-init), apply() skips every nil field, and presentationStyle: nil deliberately resolves to .pageSheet to preserve the SDK's pre-existing presentation. Consistent with the documented contract on all three layers.
Release ordering — worth a moment before you tag
com.dodopayments.api:checkout-android:1.1.0 is not on Maven Central yet (1.0.2 is still newest), and both react-native/android/build.gradle and flutter/android/build.gradle already pin 1.1.0. That's fine and is the documented flow, but the order is load-bearing: merge → release Kotlin → then release RN and Flutter. Releasing either dependent first publishes a package whose Android dependency doesn't resolve.
Flagging it because nothing enforces it. CI is Linux-only and never resolves that coordinate, and neither release workflow builds the Android side — so this would surface as an integrator bug report, same failure mode as the iOS pin. Version numbers themselves are internally consistent (kotlin build.gradle.kts + README + both dependents all 1.1.0; flutter pubspec and podspec both 1.1.0; RN 1.2.0).
Verification
All 7 checks green, including the Android and Flutter suites. Locally: 31 React Native tests pass, tsc --noEmit clean, and both vendoring scripts produce a complete core.
One honest caveat on scope: I confirmed API-surface compatibility by reading the pinned core against both bridges, plus the satellite's macOS CI proving the core itself builds — I don't have macOS here, so I did not independently reproduce your demo-app builds. That combination is strong enough for me on this change, since a label/arity/case mismatch is what would break and all three are verified directly.
Restating the standing gap, non-blocking and not for this PR: nothing anywhere compiles the bridges against the vendored core. The satellite CI covers the core alone, but DodoCheckoutReactNative.swift and DodoCheckoutFlutterPlugin.swift — the two files that were actually broken — are never compiled by any job. A pod lib lint or a demo-app build would turn that class of break into a red check.
No description provided.