Skip to content

feat: implement Bible reader view for Android#39

Open
sidorchukandrew wants to merge 1 commit intomainfrom
as/kotlin-bible-reader
Open

feat: implement Bible reader view for Android#39
sidorchukandrew wants to merge 1 commit intomainfrom
as/kotlin-bible-reader

Conversation

@sidorchukandrew
Copy link
Collaborator

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 update
  • refactor: Code refactoring (no functional changes)
  • perf: Performance improvement
  • test: Test additions or updates
  • build: Build system or dependency changes
  • ci: CI configuration changes
  • chore: Other changes (maintenance, etc.)

Checklist

  • My code follows the project's code style guidelines
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings or errors
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • All commit messages follow conventional commits format
  • I have updated the appropriate section in documentation (if needed)

@sidorchukandrew sidorchukandrew self-assigned this Feb 18, 2026
@chatgpt-codex-connector
Copy link

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 18, 2026

Greptile Summary

This PR replaces the Android Bible reader view placeholder with a real implementation that delegates to the Kotlin SDK's BibleReader composable. The bibleReference() method mirrors the iOS Swift counterpart's logic for constructing BibleReference objects from React Native props.

  • Crash risk from force-unwraps: Multiple !! operators on nullable MutableState props (initialized to null) in both Content() and bibleReference() will throw NullPointerException if the composable renders before React Native delivers prop values. The iOS version avoids this because Swift's @Field provides non-optional defaults.
  • Unused import in module: YVPBibleWidgetView is imported in RNBibleReaderViewModule.kt but not used — it's already registered in its own module.
  • Unused imports in view: AutoSizingComposable, Direction, and EnumSet are imported but not referenced, likely carried over from YVPSignInWithYouVersionButton.kt.

Confidence Score: 2/5

  • This PR has a high risk of runtime crashes due to force-unwrapping nullable props that start as null.
  • The core logic is sound and mirrors the iOS implementation, but the force-unwrap operators (!!) on MutableState<String?> and MutableState<Int?> props initialized to null will cause NullPointerException crashes. Compose may invoke Content() before React Native has delivered all prop values, making this a likely crash scenario rather than a theoretical one.
  • android/src/main/java/com/youversion/reactnativesdk/views/YVPBibleReaderView.kt — force-unwraps on lines 37, 38, 49, 50, 51, 59, 60, 61 need null-safety guards.

Important Files Changed

Filename Overview
android/src/main/java/com/youversion/reactnativesdk/RNBibleReaderViewModule.kt Adds unused import of YVPBibleWidgetView; the module definition itself is correct and follows existing patterns.
android/src/main/java/com/youversion/reactnativesdk/views/YVPBibleReaderView.kt Replaces placeholder with real BibleReader composable. Contains force-unwrap operators (!!) on nullable props that will cause NullPointerException crashes if props are not set before first composition. Also has 3 unused imports.

Sequence Diagram

sequenceDiagram
    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)
Loading

Last reviewed commit: 22e36fd

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

2 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

Comment on lines +36 to +38
BibleReader(
appName = props.appName.value!!,
appSignInMessage = props.signInMessage.value!!,
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

Suggested change
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Suggested change
import com.youversion.reactnativesdk.views.YVPBibleWidgetView

Comment on lines +11 to +15
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Suggested change
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

Comment on lines +47 to 55
if (start != null && end != null) {
return BibleReference(
versionId = props.versionId.value!!,
bookUSFM = props.bookUSFM.value!!,
chapter = props.chapter.value!!,
verseStart = start,
verseEnd = end
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

Suggested change
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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Comments