feat: add Android support for captive signing with URL - #3
Conversation
IronTony
left a comment
There was a problem hiding this comment.
I verified the premise against the shipped androidsdk:2.1.4 AAR rather than the docs. DSSigningDelegate.launchCaptiveSigning(Context, String signingURL, String, String, DSCaptiveSigningListener) is there. Its body calls CaptiveSigningActivity.getIntent(context, envelopeId, null, recipientId, signingURL) and sets setAuthNeeded(false) before startActivity. So the argument order here is right, params 3 and 4 really are nullable (only context, signingURL and listener get checkNotNullParameter), and the "no loginWithAccessToken required" claim holds. The README line this PR deletes was wrong.
Two items I want fixed before merge: the URL validation and the duplicate error event. The rest can land as follow-ups.
For the description: onCancel(envelopeId, recipientId) maps to handleSigningCancelled(envelopeId, null), so reason is always null on Android cancels, while iOS forwards DSMSigningExitReasonKey. That matches the existing session flow, so it is not a blocker, but the parity claim is a little stronger than what ships. Version bumps in package.json and android/build.gradle (both still on 1.0.5) I'll handle at release time.
| if (!isInitialized) { | ||
| completion(Result.failure(NotInitializedException())) | ||
| return | ||
| } |
There was a problem hiding this comment.
No validation on signingUrl, and the SDK does none on this path.
From the 2.1.4 bytecode: the 4-arg session overload checks for an empty envelope ID and an empty client user ID, and reports failures through the listener. The 5-arg URL overload has no checks at all and calls startActivity unconditionally. CaptiveSigningUrlRecord.signingUrl defaults to "" when JS omits it, so a blank or malformed URL launches an empty signing activity, and the promise settles only if that activity happens to call the listener back.
Add a guard next to this isInitialized check: reject a blank signingUrl, and ideally a non-https scheme.
(iOS has the same gap, but that is pre-existing and not yours to fix here. Tracking it separately.)
| export function presentCaptiveSigningWithUrl( | ||
| params: CaptiveSigningUrlParams, | ||
| ): Promise<SigningResult> { | ||
| if (Platform.OS !== 'ios') { | ||
| return Promise.reject( | ||
| new Error( | ||
| 'presentCaptiveSigningWithUrl is iOS-only. Use presentCaptiveSigning + loginWithAccessToken for Android parity.', | ||
| ), | ||
| ); | ||
| } | ||
| return DocuSignModule.presentCaptiveSigningWithUrl(params); | ||
| } |
There was a problem hiding this comment.
A small src/api.test.ts asserting presentCaptiveSigningWithUrl delegates to the native module would cover the branch this PR removes. jest.setup.js already stubs the native module.
The file's wider 0% coverage is pre-existing debt and not yours to fix here.
IronTony
left a comment
There was a problem hiding this comment.
Switching this to changes requested so the state is explicit. The approach is right and I verified it against the shipped SDK, so this is about two specific items before merge, both already threaded inline:
DocuSignManager.kt:338-341: reject a blank (and ideally non-https)signingUrl. The SDK's URL overload validates nothing and will launch an empty signing activity, leaving the promise unsettled.DocuSignModule.kt:171: drop the secondonSigningErroremit. It fires a duplicate event and flattensrecipient_signing_failedintosigning_failed.
The rest of the review can land as follow-ups. Ping me once those two are in and I'll re-review.
Re-triaged. Most of what I flagged was maintainer work on pre-existing code and has moved to #4. Replacing this with a smaller review.
IronTony
left a comment
There was a problem hiding this comment.
Trimmed this down. Five of my seven comments were really maintainer work on code that predates your PR, so I have deleted those threads and moved the items to #4. Sorry for the noise.
For the record, what moved: the listener and launch-helper extraction, the currentEnvelopeId catch in presentCaptiveSigning, the duplicate onSigningError emit, the CodedException code forwarding, and the README Throws bullet. They are tracked as a checklist on #4 and will land there after this merges.
Two things left, both in code this PR adds:
DocuSignManager.kt:341: reject a blank and ideally non-httpssigningUrl. The 5-arg overload validates nothing and callsstartActivityunconditionally, so a blank URL launches an empty signing activity and the promise never settles.src/api.ts: a small delegation test forpresentCaptiveSigningWithUrl.
The approach is right and I verified it against the shipped 2.1.4 AAR. Ping me once those two are in.
Heads up on #4: it adds an opt-in launchStrategy to the session flow that mints a recipient view on device and launches it through the same overload you exposed here. Different entry point, same SDK path, so it will consume your entrypoint rather than duplicate it. It is a draft and waits for this one to merge first, so there is no rush on your side and no rebase coming your way.
There was a problem hiding this comment.
🟡 Changes recommended
The Android implementation introduces androidx.core.net.toUri() usage without declaring core-ktx, which can break compilation depending on transitive dependencies.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Android support for the URL-based captive signing flow (presentCaptiveSigningWithUrl) so consumers can launch captive signing from a pre-minted recipient-view URL on both iOS and Android, aligning the JS API surface and documentation across platforms.
Changes:
- Remove the JS-side iOS-only guard for
presentCaptiveSigningWithUrland delegate directly to the native module on both platforms. - Implement Android
presentCaptiveSigningWithUrlviaDocuSign.getInstance().getSigningDelegate().launchCaptiveSigning(...), including basic HTTPS URL validation and in-flight session guarding. - Update README/CHANGELOG and add a Jest unit test plus native-module mocking for
presentCaptiveSigningWithUrl.
File summaries
| File | Description |
|---|---|
| src/api.ts | Removes iOS-only restriction and exposes URL flow on Android via native module. |
| src/api.test.ts | Adds a unit test asserting JS delegation to the native module. |
| README.md | Updates flow matrix and URL-flow docs to reflect iOS+Android support and initialize requirement. |
| jest.setup.js | Extends the native-module Jest stub to include presentCaptiveSigningWithUrl. |
| CHANGELOG.md | Documents upcoming Android URL-flow support under “Next”. |
| android/src/main/java/expo/modules/docusign/DocuSignModule.kt | Adds Android async function bridging for presentCaptiveSigningWithUrl. |
| android/src/main/java/expo/modules/docusign/DocuSignManager.kt | Implements URL-based captive signing on Android using the DocuSign SDK delegate. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
The Android URL validation can crash on malformed URLs, and not_initialized failures are currently surfaced as signing_failed instead of a distinct error code.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
android/src/main/java/expo/modules/docusign/DocuSignManager.kt:350
- The HTTPS validation can crash for malformed/relative URLs because
Uri.parse(signingUrl).schememay be null at runtime; calling.equals(...)on a null scheme will throw instead of returning a cleanSigningFailedException.
val signingUri = android.net.Uri.parse(signingUrl)
if (
!signingUri.scheme.equals("https", ignoreCase = true) ||
signingUri.host.isNullOrBlank()
) {
android/src/main/java/expo/modules/docusign/DocuSignModule.kt:173
presentCaptiveSigningWithUrlcan fail withNotInitializedException, but the module currently rejects it with codesigning_failed. This makes it hard for callers to distinguish a missinginitialize()call (and it contradicts the documentednot_initializedcode in the README error table).
onFailure = { error ->
emitSigningError(params.envelopeId, "signing_failed", error.message ?: "Unknown error")
promise.reject("signing_failed", error.message ?: "Unknown error", error as? Exception)
}
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
Add Android support for
presentCaptiveSigningWithUrlusing the URL basedlaunchCaptiveSigning, which is supported by the SDK (doc). README updated 👍 .