chore: Upgrade Kotlin side to match YouVersion Kotlin SDK 1.0.1#41
chore: Upgrade Kotlin side to match YouVersion Kotlin SDK 1.0.1#41sidorchukandrew wants to merge 5 commits intoas/expo-55-upgradefrom
Conversation
sidorchukandrew
force-pushed
the
as/new-kotlin-version
branch
from
bbb2e6f to
47a6d7c
Compare
sidorchukandrew
force-pushed
the
as/new-kotlin-version
branch
from
310c869 to
60270f8
Compare
cameronapak
left a comment
cameronapak
left a comment
There was a problem hiding this comment.
Devin Review found 2 potential issues.
View 4 additional findings in Devin Review.
| const NativeView: React.ComponentType<NativeProps> = | ||
| requireNativeView("BibleReaderView"); | ||
|
|
||
| const PlatformHost = Platform.OS === "ios" ? IosHost : AndroidHost; |
There was a problem hiding this comment.
🔴 Incomplete platform-specific Host migration — 3 of 5 TSX components still use iOS-only Host
The PR introduces the PlatformHost pattern in BibleReaderView.tsx and SignInWithYouVersionButton.tsx to select the correct Host component per platform (AndroidHost for Android, IosHost for iOS). However, the same transformation was not applied to src/components/BibleTextView.tsx:1, src/components/BibleWidgetView.tsx:1, and src/components/VotdView.tsx:1, which still import and use Host from @expo/ui/swift-ui (iOS-only). Since this PR also sets up all five Android Kotlin view files (e.g. YVPBibleTextView.kt, YVPBibleWidgetView.kt, YVPVotdView.kt), those three components will fail to render on Android because they wrap their native views in an iOS-only Host.
Prompt for agents
Apply the same platform-specific Host pattern used in src/components/BibleReaderView.tsx and src/components/SignInWithYouVersionButton.tsx to the three remaining component files:
1. src/components/BibleTextView.tsx (line 1): Replace `import { Host } from "@expo/ui/swift-ui";` with the AndroidHost/IosHost pattern, add Platform import, define PlatformHost, and use PlatformHost instead of Host in the JSX.
2. src/components/BibleWidgetView.tsx (line 1): Same changes — import both hosts, define PlatformHost, use PlatformHost instead of Host.
3. src/components/VotdView.tsx (line 1): Same changes — import both hosts, define PlatformHost, use PlatformHost instead of Host.
The pattern to follow is:
import { Host as AndroidHost } from "@expo/ui/jetpack-compose";
import { Host as IosHost } from "@expo/ui/swift-ui";
import { Platform } from "react-native";
const PlatformHost = Platform.OS === "ios" ? IosHost : AndroidHost;
Then replace all <Host .../> usages with <PlatformHost .../>.
Was this helpful? React with 👍 or 👎 to provide feedback.
| val passageId = bibleReference.bookUSFM + "." + bibleReference.chapter.toString() | ||
|
|
||
| val response = YouVersionApi.bible.passage( | ||
| versionId = bibleReference.versionId, | ||
| passageId = passageId | ||
| ) | ||
|
|
||
| return response | ||
| return response.content |
There was a problem hiding this comment.
🟡 Unused context parameter in chapter() after switching to YouVersionApi.bible.passage()
The chapter() function was refactored to use YouVersionApi.bible.passage() which does not require an Android Context, but the context: Context parameter was left in the function signature. The caller at RNYouVersionPlatformModule.kt:59-66 still obtains and passes the ReactContext, which would throw IllegalStateException if ReactContext is not yet available — an unnecessary risk for a parameter that's no longer used. The now-unused BibleVersionRepository import at YVPBibleApi.kt:6 also remains.
Prompt for agents
In android/src/main/java/com/youversion/reactnativesdk/api/YVPBibleApi.kt:
1. Remove the unused import of BibleVersionRepository on line 6
2. Remove the unused import of android.content.Context on line 3
3. Change the chapter function signature on line 29 from `suspend fun chapter(bibleReference: BibleReferenceRecord, context: Context): String` to `suspend fun chapter(bibleReference: BibleReferenceRecord): String`
In android/src/main/java/com/youversion/reactnativesdk/RNYouVersionPlatformModule.kt:
1. On lines 59-66, remove the context-obtaining code and simplify to just: `return@Coroutine YVPBibleApi.chapter(bibleReference = bibleReference)`
Was this helpful? React with 👍 or 👎 to provide feedback.
|
@greptileai, can you please review this PR? |
Greptile SummaryThis PR upgrades the YouVersion Kotlin SDK from Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant RN as React Native
participant API as YVPBibleApi / YVPLanguagesApi
participant YV as YouVersionApi (SDK 1.0.1)
Note over API,YV: Paginated versions fetch (new)
RN->>API: versions(languageTag)
loop while nextPageToken != null
API->>YV: bible.versions(languageTag, pageToken)
YV-->>API: { data, nextPageToken }
API->>API: allResults.addAll(data)
end
API-->>RN: List<BibleVersionRecord>
Note over API,YV: Chapter fetch (refactored)
RN->>API: chapter(bibleReference, context)
API->>YV: bible.passage(versionId, passageId)
YV-->>API: { content, ... }
API-->>RN: String (HTML content)
Last reviewed commit: "chore: use appropria..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -19,14 +27,13 @@ object YVPBibleApi { | |||
| } | |||
|
|
|||
| suspend fun chapter(bibleReference: BibleReferenceRecord, context: Context): String { | |||
There was a problem hiding this comment.
context parameter and stale imports
The refactored chapter() function no longer uses the context: Context parameter or the two domain imports that were needed by the old BibleVersionRepository-based implementation. These should be cleaned up to avoid dead code and potential compiler warnings.
| suspend fun chapter(bibleReference: BibleReferenceRecord, context: Context): String { | |
| suspend fun chapter(bibleReference: BibleReferenceRecord): String { |
Also remove the two now-unused import lines at the top of the file:
import com.youversion.platform.core.bibles.domain.BibleReference
import com.youversion.platform.core.bibles.domain.BibleVersionRepository
| @@ -71,6 +74,6 @@ export function SignInWithYouVersionButton({ | |||
| onTap={onPress} | |||
| modifiers={[fixedSize()]} | |||
There was a problem hiding this comment.
fixedSize() modifier applied on Android
fixedSize() is imported from @expo/ui/swift-ui/modifiers and has no Android equivalent. On iOS, combined with the old AutoSizingComposable wrapper (which has since been removed from the Kotlin side), it controlled button sizing. Now that AutoSizingComposable is gone and Android uses AndroidHost, this modifier will be silently ignored on Android.
Please verify that the SignInWithYouVersionButton renders and sizes correctly on Android without this modifier taking effect. If auto-sizing on Android is handled entirely by the new ComposableScope API, a comment here explaining that would help future readers understand why a SwiftUI modifier is still present in a cross-platform component.
2 participants
Description
This PR does 2 things:
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