-
Notifications
You must be signed in to change notification settings - Fork 0
Feat/map preloading #43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughIntroduces an offline-first loading flow on Android and iOS: derive an expoKey, prefer cached offline plan, then bundled ZIP asset, otherwise network URL; after load, trigger an asynchronous cache refresh. Also bumps com.expofp Android dependency versions to 4.11.0. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App
participant View as ExpofpView
participant Cache as Offline Cache
participant Assets as Bundled ZIP/Assets
participant Net as Network
User->>App: Open map view
App->>View: Initialize with URL
View->>View: Derive expoKey from URL
View->>Cache: Lookup latest plan for expoKey
alt Cached plan found
Cache-->>View: Offline plan path/info
View->>View: Open offline plan
else No cached plan
View->>Assets: Check for <expoKey>.zip
alt Asset ZIP exists
Assets-->>View: ZIP path/stream
View->>View: Open from ZIP
else No asset ZIP
View->>Net: Load URL
Net-->>View: Remote content
end
end
note over View: Post-load background refresh (async)
View->>Net: Trigger offline plan download
Net-->>Cache: Save/Update cached plan
Cache-->>View: Callback (success/error log)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🧹 Nitpick comments (2)
android/src/main/java/com/expofp/ExpofpViewManager.kt (2)
65-65
: Remove unused variable.
val info = ExpofpModule.downloadedOfflinePlanInfo
is never used.Apply this diff:
- val info = ExpofpModule.downloadedOfflinePlanInfo
80-86
: Don’t swallow exceptions when probing the asset; log them.Current catch hides the root cause. Also, reuse the asset name.
Apply this diff:
- val am = ctx.assets - val cachePlanExists = try { - am.open("${expoKey}.zip").close() - true - } catch (e: Exception) { - false - } + val assetName = "$expoKey.zip" + val am = ctx.assets + val cachePlanExists = try { + am.open(assetName).close() + true + } catch (e: Exception) { + Log.w("ExpofpModule", "Asset not found or unreadable: $assetName", e) + false + }
📜 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/ExpofpViewManager.kt
(2 hunks)ios/ExpofpViewManager.swift
(1 hunks)
🧰 Additional context used
🪛 detekt (1.23.8)
android/src/main/java/com/expofp/ExpofpViewManager.kt
[warning] 83-83: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 91-91: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (2)
android/build.gradle (1)
94-98
: Version bump looks consistent; double‑check transitive compatibility.All com.expofp artifacts are aligned at 4.11.0, which is good. Please confirm:
- No remaining 4.10.x references in the app using this module.
- 4.11.0 doesn’t raise minSdk/AGP/Kotlin requirements beyond what this library declares.
android/src/main/java/com/expofp/ExpofpViewManager.kt (1)
104-112
: Good: background cache refresh with callbacks.Callback wiring and logging look correct and align with the offline‑first flow.
Please confirm that
downloadOfflinePlanToCache
is idempotent for the same expoKey and won’t thrash downloads across rapid mounts/unmounts.
val url = it.getString("url") ?: "" | ||
val expoKey = url.substringAfter("https://").substringBefore(".expofp.com") | ||
|
||
val offlinePlanManager = FplanView.getOfflinePlanManager(reactContext) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make expoKey parsing robust; handle malformed/unsupported URLs early.
String slicing assumes https and a specific host shape. Prefer URI host parsing and first label extraction, and bail out to URL load if host is unexpected.
Apply this diff (also add the import shown):
+import android.net.Uri
@@
- val url = it.getString("url") ?: ""
- val expoKey = url.substringAfter("https://").substringBefore(".expofp.com")
+ val url = it.getString("url")?.takeIf { s -> s.isNotBlank() } ?: run {
+ Log.e("ExpofpModule", "Missing 'url' in settings; skipping.")
+ return
+ }
+ val host = Uri.parse(url).host.orEmpty()
+ val expoKey = host.substringBefore('.')
+ if (!host.endsWith(".expofp.com") || expoKey.isBlank()) {
+ Log.w("ExpofpModule", "Unexpected host '$host'; loading URL directly.")
+ view.load(url, com.expofp.fplan.models.Settings().withGlobalLocationProvider())
+ return
+ }
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
val url = it.getString("url") ?: "" | |
val expoKey = url.substringAfter("https://").substringBefore(".expofp.com") | |
val offlinePlanManager = FplanView.getOfflinePlanManager(reactContext) | |
import android.net.Uri | |
val url = it.getString("url")?.takeIf { s -> s.isNotBlank() } ?: run { | |
Log.e("ExpofpModule", "Missing 'url' in settings; skipping.") | |
return | |
} | |
val host = Uri.parse(url).host.orEmpty() | |
val expoKey = host.substringBefore('.') | |
if (!host.endsWith(".expofp.com") || expoKey.isBlank()) { | |
Log.w("ExpofpModule", "Unexpected host '$host'; loading URL directly.") | |
view.load(url, com.expofp.fplan.models.Settings().withGlobalLocationProvider()) | |
return | |
} | |
val offlinePlanManager = FplanView.getOfflinePlanManager(reactContext) |
val offlinePlanManager = FplanView.getOfflinePlanManager(reactContext) | ||
val latestOfflinePlan = offlinePlanManager.allOfflinePlansFromCache | ||
.filter { offlinePlanInfo -> offlinePlanInfo.expoKey == expoKey } | ||
.maxByOrNull { offlinePlanInfo -> offlinePlanInfo.version } | ||
if (latestOfflinePlan != null) { | ||
Log.d("ExpofpModule", latestOfflinePlan.expoKey) | ||
view.openOfflinePlan(latestOfflinePlan, "", com.expofp.fplan.models.Settings().withGlobalLocationProvider()) | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a non-null Context source for both offline manager and assets.
reactContext
can be null; use view.context
as fallback and reuse the same ctx
throughout.
Apply this diff:
- val offlinePlanManager = FplanView.getOfflinePlanManager(reactContext)
+ val ctx = this.reactContext ?: view.context
+ val offlinePlanManager = FplanView.getOfflinePlanManager(ctx)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
val offlinePlanManager = FplanView.getOfflinePlanManager(reactContext) | |
val latestOfflinePlan = offlinePlanManager.allOfflinePlansFromCache | |
.filter { offlinePlanInfo -> offlinePlanInfo.expoKey == expoKey } | |
.maxByOrNull { offlinePlanInfo -> offlinePlanInfo.version } | |
if (latestOfflinePlan != null) { | |
Log.d("ExpofpModule", latestOfflinePlan.expoKey) | |
view.openOfflinePlan(latestOfflinePlan, "", com.expofp.fplan.models.Settings().withGlobalLocationProvider()) | |
} else { | |
val ctx = this.reactContext ?: view.context | |
val offlinePlanManager = FplanView.getOfflinePlanManager(ctx) | |
val latestOfflinePlan = offlinePlanManager.allOfflinePlansFromCache | |
.filter { offlinePlanInfo -> offlinePlanInfo.expoKey == expoKey } | |
.maxByOrNull { offlinePlanInfo -> offlinePlanInfo.version } | |
if (latestOfflinePlan != null) { | |
Log.d("ExpofpModule", latestOfflinePlan.expoKey) | |
view.openOfflinePlan(latestOfflinePlan, "", com.expofp.fplan.models.Settings().withGlobalLocationProvider()) | |
} else { |
🤖 Prompt for AI Agents
In android/src/main/java/com/expofp/ExpofpViewManager.kt around lines 69 to 76,
reactContext may be null so do not call FplanView.getOfflinePlanManager or
access assets with reactContext directly; instead obtain a non-null ctx by using
the view's context as a fallback (e.g., val ctx = reactContext ?: view.context)
and then reuse ctx for both the offline manager and any asset access, replacing
reactContext usages in this block with ctx.
try { | ||
Log.d("ExpofpModule", "openZipFromAssets: ${'$'}candidate") | ||
view.openZipFromAssets("${expoKey}.zip", "", com.expofp.fplan.models.Settings().withGlobalLocationProvider(), ctx) | ||
} catch (e: Exception) { | ||
Log.d("ExpofpModule", "failed to open asset zip, loading url: ${'$'}url") | ||
view.load(url, com.expofp.fplan.models.Settings().withGlobalLocationProvider()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix undefined log variable and include exception details on failure.
candidate
is undefined. Also log the caught exception.
Apply this diff:
- try {
- Log.d("ExpofpModule", "openZipFromAssets: ${'$'}candidate")
- view.openZipFromAssets("${expoKey}.zip", "", com.expofp.fplan.models.Settings().withGlobalLocationProvider(), ctx)
- } catch (e: Exception) {
- Log.d("ExpofpModule", "failed to open asset zip, loading url: ${'$'}url")
- view.load(url, com.expofp.fplan.models.Settings().withGlobalLocationProvider())
- }
+ try {
+ Log.d("ExpofpModule", "openZipFromAssets: ${'$'}assetName")
+ view.openZipFromAssets(assetName, "", com.expofp.fplan.models.Settings().withGlobalLocationProvider(), ctx)
+ } catch (e: Exception) {
+ Log.w("ExpofpModule", "Failed to open asset zip '$assetName', loading url: ${'$'}url", e)
+ view.load(url, com.expofp.fplan.models.Settings().withGlobalLocationProvider())
+ }
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 detekt (1.23.8)
[warning] 91-91: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 Prompt for AI Agents
In android/src/main/java/com/expofp/ExpofpViewManager.kt around lines 88 to 94,
the Log.d call references an undefined variable `candidate` and the catch block
does not include the caught exception details; replace `candidate` with the
correct variable `expoKey` (or the appropriate existing identifier used to form
the zip name) in the debug log, and update the catch to log the failure and the
exception (use Log.e with a clear message and pass the exception to include
stacktrace), e.g., log that opening the asset zip failed including expoKey and
then call view.load as before.
ios/ExpofpViewManager.swift
Outdated
print("downlpading the map") | ||
fplanView.downloadZipToCache(dataStore.url as String) { htmlFilePath, error in | ||
if let error = error { | ||
print("error doenaloding the map") | ||
print(error) | ||
} else { | ||
print("success downloading the map") | ||
if let htmlFilePath = htmlFilePath { | ||
print("htmlFilePath: \(htmlFilePath)") | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard URL before background download; move assignment after successful load; fix typos.
- Don’t start a download with an empty/invalid URL.
- Set
loadedUrl
after you decide and start loading, not before. - Tidy log typos.
Apply this diff:
- loadedUrl = dataStore.url
- print("downlpading the map")
- fplanView.downloadZipToCache(dataStore.url as String) { htmlFilePath, error in
+ loadedUrl = dataStore.url
+ let urlString = dataStore.url as String
+ guard !urlString.isEmpty else {
+ print("skip downloading: empty url")
+ return
+ }
+ print("downloading the map")
+ fplanView.downloadZipToCache(urlString) { htmlFilePath, error in
if let error = error {
- print("error doenaloding the map")
+ print("error downloading the map")
print(error)
} else {
print("success downloading the map")
if let htmlFilePath = htmlFilePath {
print("htmlFilePath: \(htmlFilePath)")
}
}
}
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In ios/ExpofpViewManager.swift around lines 104 to 115, the code starts a
background download without validating the URL, assigns loadedUrl prematurely,
and contains several log typos; update the block to first guard the URL is
non-empty/valid (use guard let url = dataStore.url as String, url.isEmpty ==
false else { log and return }), only call fplanView.downloadZipToCache(...) when
url is valid, move any assignment to loadedUrl to after you have successfully
started/confirmed the download, and replace print typos with clear messages
(e.g., "starting map download", "error downloading map:", "success downloading
map", include the error details and htmlFilePath when present).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (4)
android/src/main/java/com/expofp/ExpofpViewManager.kt (4)
40-82
: Extract settings instance to reduce duplication.The settings object is created and used multiple times. Consider extracting it as suggested in the past review.
40-95
: Consider extracting map loading logic into smaller methods.The map loading logic could be broken down into smaller, more focused methods for better readability and testability, as suggested in past reviews.
36-38
: Make URL parsing more robust to handle edge cases.The
getExpoKeyFromUrl
method assumes a specific URL format and may fail on malformed or unexpected URLs. Consider using URI parsing for better reliability.Apply this diff to improve URL parsing:
+import android.net.Uri -private fun getExpoKeyFromUrl(url: String): String { - return url.substringAfter("https://").substringBefore(".expofp.com") -} +private fun getExpoKeyFromUrl(url: String): String? { + val uri = Uri.parse(url) + val host = uri.host ?: return null + if (!host.endsWith(".expofp.com")) return null + return host.substringBefore(".expofp.com").takeIf { it.isNotBlank() } +}Then update
openMapForUrl
to handle the nullable return:private fun openMapForUrl(view: FplanView, url: String) { - val expoKey = getExpoKeyFromUrl(url) + val expoKey = getExpoKeyFromUrl(url) ?: run { + Log.w("ExpofpModule", "Could not extract expoKey from URL: $url") + view.load(url, com.expofp.fplan.models.Settings().withGlobalLocationProvider()) + return + } val settings = com.expofp.fplan.models.Settings().withGlobalLocationProvider()
44-44
: Use non-null context for offline manager initialization.
reactContext
can be null, which would cause issues when initializing the offline manager.Apply this diff to ensure a non-null context:
- val offlinePlanManager = FplanView.getOfflinePlanManager(reactContext) + val ctx = this.reactContext ?: view.context + val offlinePlanManager = FplanView.getOfflinePlanManager(ctx)
🧹 Nitpick comments (5)
android/src/main/java/com/expofp/ExpofpViewManager.kt (5)
55-58
: Simplify context handling and reuse throughout the method.The context fallback pattern here should be established earlier and reused consistently.
Apply this diff to establish context once at the beginning:
private fun openMapForUrl(view: FplanView, url: String) { val expoKey = getExpoKeyFromUrl(url) val settings = com.expofp.fplan.models.Settings().withGlobalLocationProvider() + val ctx = this.reactContext ?: view.context - val offlinePlanManager = FplanView.getOfflinePlanManager(reactContext) + val offlinePlanManager = FplanView.getOfflinePlanManager(ctx) val latestOfflinePlan = offlinePlanManager.allOfflinePlansFromCache .filter { offlinePlanInfo -> offlinePlanInfo.expoKey == expoKey } .maxByOrNull { offlinePlanInfo -> offlinePlanInfo.version } if (latestOfflinePlan != null) { Log.d("ExpofpModule", latestOfflinePlan.expoKey) view.openOfflinePlan(latestOfflinePlan, "", settings) return } - val ctx = this.reactContext ?: run { - view.load(url, settings) - return - } - val am = ctx.assets
61-66
: Consider logging the exception for debugging asset access issues.While swallowing the exception is intentional here for control flow, logging it at debug level would help troubleshoot asset-related issues.
Apply this diff to add debug logging:
val cachePlanExists = try { am.open("${expoKey}.zip").close() true } catch (e: Exception) { + Log.d("ExpofpModule", "Asset ${expoKey}.zip not found: ${e.message}") false }
73-77
: Include exception details in error log.The caught exception should be logged with its details to aid debugging.
Apply this diff to improve error logging:
} catch (e: Exception) { - Log.d("ExpofpModule", "failed to open asset zip, loading url: $url") + Log.w("ExpofpModule", "Failed to open asset zip '${expoKey}.zip', loading url: $url", e) view.load(url, settings) return }
126-126
: Handle potential null/empty URL gracefully.If the URL is empty or null,
getExpoKeyFromUrl
might return unexpected results.Apply this diff to add validation:
- val url = it.getString("url") ?: "" - val expoKey = getExpoKeyFromUrl(url) + val url = it.getString("url") + if (url.isNullOrBlank()) { + Log.e("ExpofpModule", "URL is missing or empty in settings") + return + } + val expoKey = getExpoKeyFromUrl(url) ?: run { + Log.w("ExpofpModule", "Could not extract expoKey from URL: $url") + view.load(url, com.expofp.fplan.models.Settings().withGlobalLocationProvider()) + return + }
85-85
: Ensure consistent context usage for offline manager.The offline manager should use the same context source consistently.
Apply this diff to use a non-null context:
- val offlinePlanManager = FplanView.getOfflinePlanManager(reactContext) + val ctx = this.reactContext ?: FplanView(reactContext).context + val offlinePlanManager = FplanView.getOfflinePlanManager(ctx)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
android/src/main/java/com/expofp/ExpofpViewManager.kt
(3 hunks)
🧰 Additional context used
🪛 detekt (1.23.8)
android/src/main/java/com/expofp/ExpofpViewManager.kt
[warning] 64-64: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 73-73: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (4)
ios/ExpofpViewManager.swift (4)
85-111
: Good fix: fallback chain no longer blocked by cachePath binding.Early returns ensure preloaded/URL fallbacks are reached when cache exists but expoKey mismatches.
131-136
: Thanks for switching to value comparison.Using != for NSStrings fixes the prior identity-comparison bug. Optional: compare as Swift Strings to avoid bridging edge cases:
if (loadedUrl as String?) != (dataStore.url as String) { … }
113-125
: Guard invalid/empty URL before starting background download.Download should no-op on empty/invalid strings to avoid undefined SDK behavior.
- private func downloadOffline(for urlString: String) { - print("downloading the map") - fplanView.downloadZipToCache(urlString) { htmlFilePath, error in + private func downloadOffline(for urlString: String) { + guard !urlString.isEmpty else { + print("skip downloading: empty url") + return + } + print("downloading the map") + fplanView.downloadZipToCache(urlString) { htmlFilePath, error in
90-97
: Prevent crash: derive cachedExpoKey via URL APIs, not string-split/index.Accessing pathComponents[count - 2] will crash for short/atypical paths. Use Foundation URL helpers.
Apply:
- let pathComponents = cachePath.absoluteString.components(separatedBy: "/") - let cachedExpoKey = pathComponents[pathComponents.count - 2] + let cachedExpoKey = cachePath.deletingLastPathComponent().lastPathComponent
🧹 Nitpick comments (2)
ios/ExpofpViewManager.swift (2)
103-111
: Use a single settings path for consistency (or confirm API).You introduced defaultSettings but openZip/load still use the legacy useGlobalLocationProvider flag. Prefer one configuration path to avoid divergence.
If the SDK exposes “settings:” overloads, switch to them:
- fplanView.openZip(path, params: nil, useGlobalLocationProvider: true) + fplanView.openZip(path, params: nil, settings: defaultSettings) … - fplanView.load(urlString, useGlobalLocationProvider: true) + fplanView.load(urlString, settings: defaultSettings)If not, consider removing defaultSettings to avoid partial usage.
81-83
: expoKey(from:) may resolve to “www”. Strip common prefixes.For hosts like www.example.com this returns “www”, likely not your intended key. Strip “www.” (and lowercase) before taking the first label.
- private func expoKey(from urlString: String) -> String { - return (URL(string: urlString)?.host ?? "").components(separatedBy: ".").first ?? "" - } + private func expoKey(from urlString: String) -> String { + let host = (URL(string: urlString)?.host ?? "").lowercased() + let normalized = host.replacingOccurrences(of: #"^www\."#, with: "", options: .regularExpression) + return normalized.components(separatedBy: ".").first ?? "" + }Confirm this matches how your bundled ZIPs are named on iOS and Android.
Summary by CodeRabbit
New Features
Enhancements
Chores