feat(android): update to 5 #46
Conversation
WalkthroughAndroid Gradle dependencies for ExpoFP libraries were bumped (fplan, crowdconnected to 5.1.0); the background module remains commented in the file. ExpofpModule’s preload now logs and immediately rejects its Promise with PRELOAD_UNAVAILABLE. ExpofpViewManager was refactored to use ExpoFpView and a presenter-based initialization with optional CrowdConnected provider, updating method signatures accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant RN as React Native
participant VM as ExpofpViewManager
participant V as ExpoFpView
participant Plan as ExpoFpPlan
participant Presenter as Plan Presenter
participant CC as CrowdConnected Provider
RN->>VM: createViewInstance()
VM-->>RN: ExpoFpView (V)
RN->>VM: setSettings(V, settingsMap)
VM->>Plan: initialize(context)
VM->>Plan: extract ExpoFpKey from URL
VM->>Presenter: create Presenter with ExpoFpKey
alt CrowdConnected enabled in settings
VM->>CC: createCrowdConnectedProvider(settings)
VM->>Presenter: attachLocationProvider(CC)
end
VM->>V: attachPresenter(Presenter)
sequenceDiagram
autonumber
participant JS as JS bridge
participant Mod as ExpofpModule
JS->>Mod: preload(url)
Note right of Mod: New behavior
Mod->>Mod: Log "preload: not implemented"
Mod-->>JS: Reject Promise with PRELOAD_UNAVAILABLE + message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
android/build.gradle(1 hunks)android/src/main/java/com/expofp/ExpofpModule.kt(1 hunks)android/src/main/java/com/expofp/ExpofpViewManager.kt(1 hunks)
| isAllowedInBackground = enableBackground, | ||
| isHeadingEnabled = true, | ||
| aliases = aliases, | ||
| notificationText = "Indoor navigation is active", | ||
| serviceIcon = R.drawable.placeholder_icon | ||
| ) | ||
|
|
||
| return if (enableBackground) { | ||
| ExpoFpCrowdConnectedBackgroundLocationProvider(application, settings) | ||
| } else { | ||
| ExpoFpCrowdConnectedLocationProvider(application, settings) | ||
| } |
There was a problem hiding this comment.
enableBackground is undeclared
Both branches reference enableBackground, but the property (and its React prop setter) were removed. This file will fail to compile. Please reintroduce the backing field and setter so the value can be provided from JS.
Apply this diff to restore the missing pieces:
class ExpofpViewManager : SimpleViewManager<ExpoFpView>() {
private var reactContext: ThemedReactContext? = null
+ private var enableBackground: Boolean = false
@@
@ReactProp(name = "settings")
fun setSettings(view: ExpoFpView, settingsMap: ReadableMap?) {
@@
view.attachPresenter(p)
}
+
+ @ReactProp(name = "enableBackground")
+ fun setEnableBackground(view: ExpoFpView, value: Boolean) {
+ enableBackground = value
+ }
}🤖 Prompt for AI Agents
In android/src/main/java/com/expofp/ExpofpViewManager.kt around lines 54–65, the
property enableBackground was removed but still referenced when constructing the
location provider; reintroduce a backing Boolean field (default false) and a
React prop setter so JS can pass the value. Add a private var
enableBackground:Boolean = false in the manager and implement a @ReactProp(name
= "enableBackground") setter method that updates that field from incoming JS
props (or calls setNeedsUpdate on the view) so the code paths using
enableBackground compile and reflect the JS-provided value.
There was a problem hiding this comment.
@arys like bot says I don't see the enableBackground declaration, does it come with new library version?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| isAllowedInBackground = enableBackground, | ||
| isHeadingEnabled = true, | ||
| aliases = aliases, | ||
| notificationText = "Indoor navigation is active", |
| isAllowedInBackground = enableBackground, | ||
| isHeadingEnabled = true, | ||
| aliases = aliases, | ||
| notificationText = "Indoor navigation is active", | ||
| serviceIcon = R.drawable.placeholder_icon | ||
| ) | ||
|
|
||
| return if (enableBackground) { | ||
| ExpoFpCrowdConnectedBackgroundLocationProvider(application, settings) | ||
| } else { | ||
| ExpoFpCrowdConnectedLocationProvider(application, settings) | ||
| } |
There was a problem hiding this comment.
@arys like bot says I don't see the enableBackground declaration, does it come with new library version?
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
android/src/main/java/com/expofp/ExpofpViewManager.kt (1)
57-57: Localize the notification text.The hardcoded English string was previously flagged and remains unaddressed. Users with non-English locales will see untranslated text in the system notification.
Move the text to a string resource:
- notificationText = "Indoor navigation is active", + notificationText = context.getString(R.string.indoor_navigation_active),
🧹 Nitpick comments (1)
android/src/main/java/com/expofp/ExpofpViewManager.kt (1)
72-72: Consider removing redundant ExpoFpPlan initialization.
ExpoFpPlan.initialize(context)is already called increateViewInstance(line 24). Re-initializing insetSettingsmay be unnecessary unless settings can change at runtime or the view can be reused. Verify whether this is intentional or can be removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
android/build.gradle(1 hunks)android/src/main/java/com/expofp/ExpofpModule.kt(1 hunks)android/src/main/java/com/expofp/ExpofpViewManager.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- android/build.gradle
🔇 Additional comments (3)
android/src/main/java/com/expofp/ExpofpModule.kt (1)
13-16: Previous concern resolved.The promise rejection has been implemented as suggested in previous reviews, preventing JS callers from hanging indefinitely while the preload feature remains unimplemented in Android v5.
android/src/main/java/com/expofp/ExpofpViewManager.kt (2)
17-17: LGTM!The refactor to use
ExpoFpViewinstead of genericViewimproves type safety. The initialization ofExpoFpPlanduring view creation is appropriate for the presenter-based architecture.Also applies to: 22-26, 28-30
48-48: Clarify the intent of the empty string default for oneSignalUserId.If
oneSignalUserIdis not provided or is null, the code defaults to an empty string. Verify whether an empty alias is semantically valid for the CrowdConnected SDK or if the alias entry should be omitted entirely when the value is absent.
| token = token, | ||
| secret = secret, | ||
| navigationType = ExpoFpCrowdConnectedNavigationType.ALL, | ||
| isAllowedInBackground = false, |
There was a problem hiding this comment.
Make background mode configurable from JavaScript.
The isAllowedInBackground property is hardcoded to false, preventing JavaScript code from controlling background location tracking. This limits flexibility and may break use cases that require background positioning.
Consider adding a React prop and backing field to expose this setting:
class ExpofpViewManager : SimpleViewManager<ExpoFpView>() {
private var reactContext: ThemedReactContext? = null
+ private var enableBackground: Boolean = falseThen update the provider creation:
val settings = ExpoFpCrowdConnectedLocationProviderSettings(
appKey = appKey,
token = token,
secret = secret,
navigationType = ExpoFpCrowdConnectedNavigationType.ALL,
- isAllowedInBackground = false,
+ isAllowedInBackground = enableBackground,
isHeadingEnabled = true,
aliases = aliases,
notificationText = "Indoor navigation is active",
serviceIcon = R.drawable.placeholder_icon
)And add a setter:
@ReactProp(name = "enableBackground")
fun setEnableBackground(view: ExpoFpView, value: Boolean) {
enableBackground = value
}🤖 Prompt for AI Agents
In android/src/main/java/com/expofp/ExpofpViewManager.kt around line 54,
isAllowedInBackground is hardcoded to false which prevents JS from toggling
background location; add a private backing field (e.g. enableBackground: Boolean
= false) to the ViewManager, expose a React prop name (e.g.
"enableBackground"/"enableBackgroundLocation") and implement a @ReactProp setter
that updates the backing field, then use that backing field when
constructing/creating the provider (replace the hardcoded false with
enableBackground) so the JS prop controls background mode.
| isHeadingEnabled = true, | ||
| aliases = aliases, | ||
| notificationText = "Indoor navigation is active", | ||
| serviceIcon = R.drawable.placeholder_icon |
There was a problem hiding this comment.
Replace the placeholder icon with a production asset.
Using R.drawable.placeholder_icon in production code can result in a poor user experience if the placeholder is visually unappealing or confusing.
🤖 Prompt for AI Agents
In android/src/main/java/com/expofp/ExpofpViewManager.kt around line 58, the
code sets serviceIcon = R.drawable.placeholder_icon which is a temporary asset;
replace this with the correct production drawable resource (e.g.,
R.drawable.service_icon_prod) by adding the final asset files into the
appropriate res/drawable(-density) folders, update the reference here to the
production resource name, ensure naming follows Android resource conventions,
and verify the image renders correctly across screen densities and themes.
3 participants
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores