Skip to content

Comments

feat(Device): B2PDE-1770 implement new method setBiometricsAuthenticationStatus#226

Merged
atabel merged 4 commits intomasterfrom
jprieto/B2PDE-1770
Jul 18, 2025
Merged

feat(Device): B2PDE-1770 implement new method setBiometricsAuthenticationStatus#226
atabel merged 4 commits intomasterfrom
jprieto/B2PDE-1770

Conversation

@jprietoTuenti
Copy link
Contributor

@jprietoTuenti jprietoTuenti commented Jul 14, 2025

B2PDE-1770

closes #225

@jprietoTuenti jprietoTuenti requested a review from Copilot July 14, 2025 07:52
@jprietoTuenti jprietoTuenti changed the title feat: B2PDE-1770 implement new method setBiometricsAuthenticationStatus feat(Device): B2PDE-1770 implement new method setBiometricsAuthenticationStatus Jul 14, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces a new API for toggling biometric authentication status between the web and native layers.

  • Adds a new SET_BIOMETRICS_AUTHENTICATION_STATUS message type in the post-message interface
  • Implements setBiometricsAuthenticationStatus(enable: boolean) in device.ts with accompanying unit tests
  • Updates package exports and user documentation (README and index.ts)

Reviewed Changes

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

Show a summary per file
File Description
src/post-message.ts Register new SET_BIOMETRICS_AUTHENTICATION_STATUS response type
src/device.ts Implement setBiometricsAuthenticationStatus function and JSDoc
src/tests/device-test.ts Add unit tests for the new method
index.ts Export setBiometricsAuthenticationStatus from the public API
README.md Document setBiometricsAuthenticationStatus API and error cases
Comments suppressed due to low confidence (2)

src/tests/device-test.ts:563

  • Add an assertion to verify that msg.payload.enable matches the expected value in the error test, ensuring the correct data is sent.
            expect(msg.type).toBe('SET_BIOMETRICS_AUTHENTICATION_STATUS');

README.md:1802

  • [nitpick] Consider adding a 404 error case for unsupported bridge implementations, mirroring the documentation of similar methods.
-   `400`: enable parameter is missing

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@jprietoTuenti jprietoTuenti added the AI Generated Code and PR generated from AI (Copilot) label Jul 14, 2025
Copy link
Contributor

@jeslat jeslat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

src/device.ts Outdated
postMessageToNativeApp({
type: 'SET_BIOMETRICS_AUTHENTICATION_STATUS',
payload: {
enable: enable,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the spec this should be:

Suggested change
enable: enable,
newStatus: enable,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think "newStatus" has the wrong semantic once the parameter has been changed to a boolean. What do you all think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, then change the spec

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

@pladaria pladaria Jul 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of a single parameter, consider accepting an object: {enable: boolean}

This makes the code easier to understand (the object acts as named parameters):
setBiometricsAuthenticationStatus({enable: true})
instead of:
setBiometricsAuthenticationStatus(true)

and it is easier to extend, adding new properties

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@jprietoTuenti jprietoTuenti requested a review from atabel July 15, 2025 15:45
@atabel atabel merged commit 552ea94 into master Jul 18, 2025
2 checks passed
@atabel atabel deleted the jprieto/B2PDE-1770 branch July 18, 2025 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI Generated Code and PR generated from AI (Copilot)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

New method to set Biometrics Authentication Status

5 participants