feat: implement Bible reader view for Android#39
feat: implement Bible reader view for Android#39sidorchukandrew wants to merge 1 commit intomainfrom
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Greptile SummaryThis PR replaces the Android Bible reader view placeholder with a real implementation that delegates to the Kotlin SDK's
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant RN as React Native
participant Module as RNBibleReaderViewModule
participant View as YVPBibleReaderView
participant Compose as Jetpack Compose
participant SDK as BibleReader (Kotlin SDK)
RN->>Module: Register "BibleReaderView"
RN->>View: Create view with props
Note over View: Props initialized to null
RN->>View: Set appName, signInMessage, versionId, bookUSFM, chapter, etc.
Compose->>View: Content(modifier)
View->>View: bibleReference()
alt verseStart & verseEnd set
View->>SDK: BibleReference(versionId, bookUSFM, chapter, verseStart, verseEnd)
else hasReference == true
View->>SDK: BibleReference(versionId, bookUSFM, chapter, verse?)
else no reference
View->>SDK: null
end
View->>SDK: BibleReader(appName, signInMessage, bibleReference)
Last reviewed commit: 22e36fd |
| BibleReader( | ||
| appName = props.appName.value!!, | ||
| appSignInMessage = props.signInMessage.value!!, |
There was a problem hiding this comment.
Force-unwrapping nullable props will crash on initial render
props.appName.value and props.signInMessage.value are MutableState<String?> initialized to null. The !! operator will throw a NullPointerException when Content() is composed before React Native has sent the prop values. Unlike the iOS counterpart where @Field var appName: String provides a non-optional default, Android's MutableState<String?> starts as null.
Consider adding a guard to return early if the required props are not yet set:
| BibleReader( | |
| appName = props.appName.value!!, | |
| appSignInMessage = props.signInMessage.value!!, | |
| val appName = props.appName.value ?: return | |
| val signInMessage = props.signInMessage.value ?: return | |
| BibleReader( | |
| appName = appName, | |
| appSignInMessage = signInMessage, |
Context Used: Rule from dashboard - Avoid unnecessary negations and else blocks to reduce nesting and improve code readability. Use earl... (source)
| package com.youversion.reactnativesdk | ||
|
|
||
| import com.youversion.reactnativesdk.views.YVPBibleReaderView | ||
| import com.youversion.reactnativesdk.views.YVPBibleWidgetView |
There was a problem hiding this comment.
Unused import of YVPBibleWidgetView
YVPBibleWidgetView is imported but never referenced in this module — only YVPBibleReaderView is registered. YVPBibleWidgetView is already imported in its own module (RNBibleWidgetViewModule.kt). This appears to be accidentally added.
| import com.youversion.reactnativesdk.views.YVPBibleWidgetView |
| import expo.modules.kotlin.views.AutoSizingComposable | ||
| import expo.modules.kotlin.views.ComposeProps | ||
| import expo.modules.kotlin.views.Direction | ||
| import expo.modules.kotlin.views.ExpoComposeView | ||
| import java.util.EnumSet |
There was a problem hiding this comment.
Unused imports
AutoSizingComposable, Direction, and EnumSet are imported but never used in this file. They appear to have been carried over from YVPSignInWithYouVersionButton.kt where they are used for auto-sizing.
| import expo.modules.kotlin.views.AutoSizingComposable | |
| import expo.modules.kotlin.views.ComposeProps | |
| import expo.modules.kotlin.views.Direction | |
| import expo.modules.kotlin.views.ExpoComposeView | |
| import java.util.EnumSet |
| if (start != null && end != null) { | ||
| return BibleReference( | ||
| versionId = props.versionId.value!!, | ||
| bookUSFM = props.bookUSFM.value!!, | ||
| chapter = props.chapter.value!!, | ||
| verseStart = start, | ||
| verseEnd = end | ||
| ) | ||
| } |
There was a problem hiding this comment.
Force-unwraps on versionId, bookUSFM, and chapter risk NullPointerException
Similar to the Content() method, the !! operators on lines 49-51 will crash if these props haven't been set yet. Since bibleReference() is called from Content() during composition, the same timing issue applies — props may still be null on the first composition pass.
Consider using null-safe access and returning null if required fields are missing:
| if (start != null && end != null) { | |
| return BibleReference( | |
| versionId = props.versionId.value!!, | |
| bookUSFM = props.bookUSFM.value!!, | |
| chapter = props.chapter.value!!, | |
| verseStart = start, | |
| verseEnd = end | |
| ) | |
| } | |
| if (start != null && end != null) { | |
| val versionId = props.versionId.value ?: return null | |
| val bookUSFM = props.bookUSFM.value ?: return null | |
| val chapter = props.chapter.value ?: return null | |
| return BibleReference( | |
| versionId = versionId, | |
| bookUSFM = bookUSFM, | |
| chapter = chapter, | |
| verseStart = start, | |
| verseEnd = end | |
| ) | |
| } |
Context Used: Rule from dashboard - Avoid unnecessary negations and else blocks to reduce nesting and improve code readability. Use earl... (source)
Description
This implements the Bible reader view for Android. The Kotlin SDK does not have all of the reader implemented natively yet, but this is enough for us to wrap it and expose it. Any changes made to the Kotlin side should not really affect the RN SDK.
Type of Change
feat:New feature (non-breaking change which adds functionality)fix:Bug fix (non-breaking change which fixes an issue)docs:Documentation updaterefactor:Code refactoring (no functional changes)perf:Performance improvementtest:Test additions or updatesbuild:Build system or dependency changesci:CI configuration changeschore:Other changes (maintenance, etc.)Checklist