[WIP] Card view #787
[WIP] Card view #787
Conversation
Summary of ChangesHello @descorp, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a major architectural overhaul to the Adyen React Native SDK, focusing on enhancing modularity and developer experience. Key changes include the implementation of an embedded CardView component, a new MessageBus pattern for decoupled event communication, and a more structured approach to native module wrappers. The accompanying example application has been updated to demonstrate these new capabilities through a tabbed navigation interface, providing clear examples of various checkout flows and component integrations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant architectural improvements, primarily by adding an embedded CardView component and a MessageBus for decoupled event communication. The changes lead to a more modular and cleaner code structure, which is a great step forward. The new Architecture.md file is also an excellent addition for documenting these changes. My review includes a few suggestions to enhance code quality and fix a potential issue with error handling.
| .catch((error) => { | ||
| onError( | ||
| { | ||
| message: String(error), | ||
| errorCode: 'sessionError', | ||
| }, | ||
| SessionHelper | ||
| ); | ||
| console.error(error); | ||
| }); |
There was a problem hiding this comment.
The error from SessionHelper.createSession is only logged to the console and not propagated to the onError prop. This is a regression from previous behavior and means that the application developer will not be notified of session creation failures. The onError prop should be called to allow for proper error handling in the app.
.catch((error) => {
console.error(error);
onError?.(
{
message: String(error),
errorCode: 'sessionError',
},
SessionHelper
);
});
| yarn test > /dev/null 2>&1 || yarn test | ||
| yarn lint > /dev/null 2>&1 || yarn lint | ||
| yarn typecheck > /dev/null 2>&1 || yarn typecheck | ||
| swiftformat --lint ios > /dev/null 2>&1 || swiftformat --lint ios | ||
| ktlint "!**node_modules**" > /dev/null 2>&1 || ktlint "!**node_modules**" |
There was a problem hiding this comment.
The current approach of running commands twice on failure (e.g., yarn test > /dev/null 2>&1 || yarn test) can be inefficient for long-running tasks like tests. A more efficient pattern would be to capture the command's output and only print it if the command fails, thus avoiding a second execution. For example:
if ! output=$(yarn test 2>&1); then
echo "$output"
exit 1
fiThis would improve the developer experience by speeding up the pre-commit hook when failures occur.
| companion object { | ||
| public val messageBus: MessageBus | ||
| get() { | ||
| return _messageBus ?: throw Exception("AdyenCheckout MessageBus is not initialized") |
There was a problem hiding this comment.
Throwing a generic java.lang.Exception is not ideal. It's better to use a more specific exception, like IllegalStateException, to indicate that a component is being used in an invalid state. This provides more clarity to developers using the SDK and allows for more specific catch blocks.
| return _messageBus ?: throw Exception("AdyenCheckout MessageBus is not initialized") | |
| return _messageBus ?: throw IllegalStateException("AdyenCheckout MessageBus is not initialized") |
| let addressModels: [LookupAddressModel] = results | ||
| .compactMap { $0 as? NSDictionary } | ||
| .compactMap { try? $0.decode() } |
There was a problem hiding this comment.
The use of try? for decoding the address models will cause any decoding errors to be silently ignored. An address that fails to parse will simply be missing from the addressModels array. It would be better to handle potential decoding errors explicitly, for example by logging them, to aid in debugging issues with address data.
let addressModels: [LookupAddressModel] = results.compactMap {
guard let dict = $0 as? NSDictionary else { return nil }
do {
return try dict.decode()
} catch {
adyenPrint("Failed to decode LookupAddressModel: \(error)")
return nil
}
}
|
1 participant
Changes