feat/add_support_for_fabric_native_components_in_nativeadview #424

alhiwatan · 2025-02-28T07:55:42Z

Add support for Fabric Native Components in NativeAdView.

Description by Korbit AI

What change is being made?

Add support for Fabric Native Components in NativeAdView across Android and iOS platforms to optimize and enhance the integration of native ads within the React Native framework.

Why are these changes being made?

The adoption of Fabric Native Components leverages the new React Native architecture, which aims to improve performance by directly connecting native views and JavaScript. This change facilitates better event handling and component updates, aligning with modern programming approaches and ensuring better ad rendering performance within the AppLovin ecosystem. Additionally, the update transitions from using existing structures to the new architecture with minimal redundancy while maintaining backward compatibility where necessary.

Is this description stale? Ask me to generate a new description by commenting /korbit-generate-pr-description

korbit-ai

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Category	Issue	Fix Detected
	Duplicated Complex Parameter Parsing Logic ▹ view	✅
	Unsafe Non-null Assertion in renderNativeAd Command ▹ view	✅
	Unnecessary Style Flattening ▹ view	✅
	Incorrect Image Source Logic ▹ view	✅
	Unnecessary empty constructor ▹ view	✅
	Repetitive Ref Declarations ▹ view	✅
	Suboptimal parameter type for view tag ▹ view

Files scanned

File Path	Reviewed
src/nativeAd/NativeAdViewProvider.tsx	✅
android/src/main/java/com/applovin/reactnative/AppLovinMAXNativeAdViewManagerImpl.java	✅
android/src/oldarch/com/applovin/reactnative/AppLovinMAXNativeAdViewManager.java	✅
android/src/newarch/com/applovin/reactnative/AppLovinMAXNativeAdViewManager.java	✅
src/nativeAd/NativeAdViewComponents.tsx	✅
src/nativeAd/NativeAdView.tsx	✅
android/src/main/java/com/applovin/reactnative/AppLovinMAXNativeAdView.java	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.

You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.

Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.

Chat with Korbit on issues we post by tagging @korbit-ai in your reply.

Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

Check out our docs on how you can make Korbit work best for you and your team.

Customize Korbit for your organization through the Korbit Console.

Feedback and Support

Tell us what you think of Korbit

Schedule a call with our team

Email us @ support@korbit.ai

alhiwatan · 2025-03-06T08:42:59Z

src/nativeAd/NativeAdViewComponents.tsx

+    const imageSource =
+        (nativeAd?.url && { uri: nativeAd.url }) || (nativeAd.image ? defaultIcon : nativeAd.imageSource ? { uri: `data:image/jpeg;base64,${nativeAd.imageSource}` } : defaultIcon);


Updated using a simple if-else statement.

thomasmso · 2025-03-04T23:17:34Z

android/src/main/java/com/applovin/reactnative/AppLovinMAXNativeAdView.java

+        for ( int i = 0; i < readableArray.size(); i++ )
        {
-            extraParameters = readableMap.toHashMap();
+            ReadableMap map = readableArray.getMap( i );
+
+            String key = map.getString( "key" );
+
+            if ( !AppLovinSdkUtils.isValidString( key ) ) continue;
+
+            if ( map.hasKey( "value" ) )


I'd take the recommendation from Korbit actually. The code can probably be abstracted and consolidated.

Great to see you're considering the suggestion, @thomasmso! Implementing the helper method will indeed improve the readability and maintainability of the code. The suggested solution looks solid. Let us know if you encounter any issues while integrating this!

Created a helper function to support both extraParameters and localExtraParameters.

alhiwatan · 2025-03-06T08:44:44Z

src/nativeAd/NativeAdView.tsx

-        },
-        [onAdLoadFailed]
-    );
+            Commands.renderNativeAd(nativeAdViewRef.current!);


As suggested, check null first.

Thank you for taking the time to incorporate the changes. Before merging, I see in this diff that you still have a non-null assertion operator with nativeAdViewRef.current in the onAdLoadedEvent callback. To prevent potential crashes, please consider adding a null check before implicitly asserting that nativeAdViewRef.current is not null. Apart from this, everything appears to be okay.

korbit-ai · 2025-02-28T08:01:48Z

android/src/newarch/com/applovin/reactnative/AppLovinMAXNativeAdViewManager.java

+    }
+
+    @Override
+    public void updateAssetView(final AppLovinMAXNativeAdView view, final double assetViewTag, final String assetViewName)


Suboptimal parameter type for view tag

Tell me more

What is the issue?

Using double type for what appears to be an integer view tag, requiring an explicit Math.round conversion

Why this matters

The unnecessary use of double for an ID/tag creates potential for precision loss and requires extra conversion code, making the API less intuitive.

Suggested change ∙ Feature Preview

Change the parameter type to int:

updateAssetView(final AppLovinMAXNativeAdView view, final int assetViewTag, final String assetViewName)

_{💬 Chat with Korbit by mentioning @korbit-ai.}

Has to be double since the signature of this function is defined by codegen.

Thanks for the clarification @alhiwatan. Agreed, the types need to be aligned with the codegen signature to avoid potential issues. In this case, using double makes sense. If there's room for optimisation in the future, we could consider investigating if codegen could be more adaptable for cases like this.

alhiwatan · 2025-03-06T08:48:55Z

src/nativeAd/NativeAdViewProvider.tsx

+    const titleRef = useRef<React.ElementRef<typeof Text> | null>(null);
+    const advertiserRef = useRef<React.ElementRef<typeof Text> | null>(null);
+    const bodyRef = useRef<React.ElementRef<typeof Text> | null>(null);
+    const callToActionRef = useRef<React.ElementRef<typeof Text> | null>(null);
+    const imageRef = useRef<React.ElementRef<typeof Image> | null>(null);
+    const optionViewRef = useRef<React.ElementRef<typeof View> | null>(null);
+    const mediaViewRef = useRef<React.ElementRef<typeof View> | null>(null);


Update as suggested.

Great, I see you've updated the ref declarations as suggested. This indeed improves code readability. Also, I encourage you to consider maintaining this approach for future updates. Please proceed with the merging process.

alhiwatan · 2025-03-06T08:49:03Z

android/src/oldarch/com/applovin/reactnative/AppLovinMAXNativeAdViewManager.java

+public class AppLovinMAXNativeAdViewManager
+    extends ViewGroupManager<AppLovinMAXNativeAdView>
+{
+    public AppLovinMAXNativeAdViewManager() { }


Thanks for the update. The code looks cleaner without the unnecessary empty constructor. Good job!

alhiwatan · 2025-03-06T08:49:13Z

src/nativeAd/NativeAdViewComponents.tsx

-    const starColor = StyleSheet.flatten((style as StyleProp<TextStyle>) || {}).color ?? '#ffe234';
-    const starSize = StyleSheet.flatten((style as StyleProp<TextStyle>) || {}).fontSize ?? 10;
-
+    const starTextStyle = StyleSheet.flatten(style) as TextStyle;


Updated as suggested.

Thanks for making the update. The Memoization has been properly implemented to help optimize the style computations. This is good programming practice especially for functions with expensive operations. Everything looks good here, so this issue can be marked as resolved. If there are any other concerns or changes please let me know.

thomasmso · 2025-03-04T23:14:11Z

android/src/main/java/com/applovin/reactnative/AppLovinMAXNativeAdView.java

+
+        Map<String, Object> extraParametersMap = new HashMap<>();
+
+        for ( int i = 0; i < readableArray.size(); i++ )


This logic is used in the other PR for native adviews - so is this something we can consolidate the logic for? Like in Utils class?

Created a helper function to support both AdView and NativeAdView.

thomasmso · 2025-03-04T23:15:26Z

android/src/main/java/com/applovin/reactnative/AppLovinMAXNativeAdView.java

@@ -108,19 +116,84 @@ public void setCustomData(@Nullable final String value)
        customData = value;
    }

-    public void setExtraParameters(@Nullable final ReadableMap readableMap)
+    public void setExtraParameters(@Nullable final ReadableArray readableArray)


We should name parameters to describe what it is, not its type. So I'd name it extraParameters.

thomasmso · 2025-03-04T23:17:01Z

android/src/main/java/com/applovin/reactnative/AppLovinMAXNativeAdView.java

+                }
+                else if ( type == ReadableType.Null )
+                {
+                    extraParametersMap.put( key, null );


If null - should remove? When is this going to be the case?

Can null be used to reset the value of the key?

thomasmso · 2025-03-04T23:17:34Z

android/src/main/java/com/applovin/reactnative/AppLovinMAXNativeAdView.java

+        for ( int i = 0; i < readableArray.size(); i++ )
        {
-            extraParameters = readableMap.toHashMap();
+            ReadableMap map = readableArray.getMap( i );
+
+            String key = map.getString( "key" );
+
+            if ( !AppLovinSdkUtils.isValidString( key ) ) continue;
+
+            if ( map.hasKey( "value" ) )


I'd take the recommendation from Korbit actually. The code can probably be abstracted and consolidated.

thomasmso · 2025-03-04T23:19:05Z

android/src/main/java/com/applovin/reactnative/AppLovinMAXNativeAdView.java

@@ -170,6 +243,45 @@ public void loadAd()
        }
    }

+    public void updateAssetView(final int assetViewTag, final String assetViewName)
+    {
+        if ( assetViewName.equals( "TitleView" ) )


nit: in Java, to avoid NPEs, we operate on the string first. So "TitleView".equals( assetViewName ).

alhiwatan · 2025-03-06T08:56:22Z

Please review again!

Add support for Fabric Native Components in NativeAdView

954ce03

korbit-ai bot reviewed Feb 28, 2025

View reviewed changes

thomasmso reviewed Mar 4, 2025

View reviewed changes

Update for the review comments

aaab5c8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat/add_support_for_fabric_native_components_in_nativeadview #424

feat/add_support_for_fabric_native_components_in_nativeadview #424

alhiwatan commented Feb 28, 2025 •

edited by korbit-ai bot

Loading

korbit-ai bot left a comment •

edited

Loading

This comment was marked as resolved.

alhiwatan Mar 6, 2025

This comment was marked as resolved.

thomasmso Mar 4, 2025

korbit-ai bot Mar 4, 2025

alhiwatan Mar 6, 2025

This comment was marked as resolved.

alhiwatan Mar 6, 2025

korbit-ai bot Mar 6, 2025

korbit-ai bot Feb 28, 2025

alhiwatan Mar 6, 2025

korbit-ai bot Mar 6, 2025

This comment was marked as resolved.

alhiwatan Mar 6, 2025

korbit-ai bot Mar 6, 2025

This comment was marked as resolved.

alhiwatan Mar 6, 2025

korbit-ai bot Mar 6, 2025

This comment was marked as resolved.

alhiwatan Mar 6, 2025

korbit-ai bot Mar 6, 2025

thomasmso Mar 4, 2025

alhiwatan Mar 6, 2025

thomasmso Mar 4, 2025

thomasmso Mar 4, 2025

alhiwatan Mar 6, 2025

thomasmso Mar 4, 2025

thomasmso Mar 4, 2025

alhiwatan commented Mar 6, 2025

		const imageSource =
		(nativeAd?.url && { uri: nativeAd.url }) \|\| (nativeAd.image ? defaultIcon : nativeAd.imageSource ? { uri: `data:image/jpeg;base64,${nativeAd.imageSource}` } : defaultIcon);


		Map<String, Object> extraParametersMap = new HashMap<>();

		for ( int i = 0; i < readableArray.size(); i++ )

feat/add_support_for_fabric_native_components_in_nativeadview #424

Are you sure you want to change the base?

feat/add_support_for_fabric_native_components_in_nativeadview #424

Conversation

alhiwatan commented Feb 28, 2025 • edited by korbit-ai bot Loading

Description by Korbit AI

What change is being made?

Why are these changes being made?

korbit-ai bot left a comment • edited Loading

Choose a reason for hiding this comment

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Interacting with Korbit

Customizing Korbit

Feedback and Support

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

korbit-ai bot Mar 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

korbit-ai bot Mar 6, 2025

Choose a reason for hiding this comment

korbit-ai bot Feb 28, 2025

Choose a reason for hiding this comment

Suboptimal parameter type for view tag

What is the issue?

Why this matters

Suggested change ∙ Feature Preview

Choose a reason for hiding this comment

korbit-ai bot Mar 6, 2025

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

korbit-ai bot Mar 6, 2025

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

korbit-ai bot Mar 6, 2025

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

korbit-ai bot Mar 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alhiwatan commented Mar 6, 2025

alhiwatan commented Feb 28, 2025 •

edited by korbit-ai bot

Loading

korbit-ai bot left a comment •

edited

Loading