-
Notifications
You must be signed in to change notification settings - Fork 0
Implement hardResetDatabase method #2
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
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.
Pull request overview
This PR implements the hardResetDatabase method to provide a comprehensive reset of the Mapbox offline database. The implementation handles platform differences: iOS deletes both .mapbox/map_data and .mapbox_custom directories and reinitializes the OfflineRegionManager with a fresh custom directory, while Android simply deletes the .mapbox/map_data directory and relies on the C++ manager to auto-recreate the database.
- Adds
hardResetDatabase()method across TypeScript, iOS, and Android implementations - Updates iOS MapView and OfflineRegionManager initialization to support custom data path resolution
- Implements platform-specific cleanup strategies (iOS: full reset with reinitialization, Android: directory deletion only)
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/modules/offline/offlineManagerLegacy.ts |
Adds hardResetDatabase() method with detailed documentation explaining platform differences |
ios/RNMBX/RNMBXMapView.swift |
Adds data path resolution logic to prefer .mapbox_custom directories over default .mapbox/map_data |
ios/RNMBX/Offline/RNMBXOfflineModuleLegacy.swift |
Implements hardResetDatabase() and reinitOfflineRegionManager() with custom data path resolution; updates offlineRegionManager lazy property to support custom paths |
ios/RNMBX/Offline/RNMBXOfflineModuleLegacy.m |
Exposes hardResetDatabase method to React Native bridge |
android/src/main/java/com/rnmapbox/rnmbx/modules/RNMBXOfflineModuleLegacy.kt |
Implements hardResetDatabase() to delete .mapbox/map_data directory on a background thread |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let fileManager = FileManager.default | ||
| let appSupport = fileManager.urls( | ||
| for: .applicationSupportDirectory, | ||
| in: .userDomainMask | ||
| ).first! |
Copilot
AI
Dec 17, 2025
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.
The force unwrap on fileManager.urls(for:in:).first! could crash if the application support directory is unexpectedly unavailable. While this is extremely unlikely in practice, consider using guard-let or if-let with proper error handling for better safety, similar to standard Swift practices.
| lazy var offlineRegionManager: OfflineRegionManager = { | ||
| #if RNMBX_11 | ||
| return OfflineRegionManager() | ||
| #else | ||
| return OfflineRegionManager(resourceOptions: .init(accessToken: RNMBXModule.accessToken!)) | ||
| #endif | ||
| // This logic should be aligned with MapView initialization @see ios/RNMBX/RNMBXMapView.swift | ||
| let fileManager = FileManager.default | ||
| let appSupport = fileManager.urls( | ||
| for: .applicationSupportDirectory, | ||
| in: .userDomainMask | ||
| ).first! | ||
|
|
||
| // Default: Library/Application Support/.mapbox/map_data | ||
| let defaultDataPath = appSupport.appendingPathComponent(".mapbox/map_data") | ||
|
|
||
| // Preferred: Library/Application Support/.mapbox_custom/XXXX/map_data/map_data.db | ||
| var customDataPath: URL? = nil | ||
| let customRoot = appSupport.appendingPathComponent(".mapbox_custom", isDirectory: true) | ||
|
|
||
| if fileManager.fileExists(atPath: customRoot.path), | ||
| let entries = try? fileManager.contentsOfDirectory( | ||
| at: customRoot, | ||
| includingPropertiesForKeys: [.isDirectoryKey], | ||
| options: [] | ||
| ) { | ||
| for entry in entries { | ||
| // Only consider subdirectories under .mapbox_custom | ||
| if let isDir = try? entry.resourceValues(forKeys: [.isDirectoryKey]).isDirectory, | ||
| isDir == true { | ||
| let candidateMapData = entry.appendingPathComponent("map_data", isDirectory: true) | ||
| let candidateDb = candidateMapData.appendingPathComponent("map_data.db") | ||
|
|
||
| if fileManager.fileExists(atPath: candidateDb.path) { | ||
| customDataPath = candidateMapData | ||
| break | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let dataPathURL = customDataPath ?? defaultDataPath | ||
|
|
||
| #if RNMBX_11 | ||
| return OfflineRegionManager( | ||
| resourceOptions: .init( | ||
| accessToken: RNMBXModule.accessToken!, | ||
| dataPathURL: dataPathURL | ||
| ) | ||
| ) | ||
| #else | ||
| return OfflineRegionManager( | ||
| resourceOptions: .init( | ||
| accessToken: RNMBXModule.accessToken!, | ||
| dataPathURL: dataPathURL | ||
| ) | ||
| ) | ||
| #endif |
Copilot
AI
Dec 17, 2025
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.
The data path resolution logic (lines 8-60) is duplicated in three locations: here in the offlineRegionManager lazy property, in reinitOfflineRegionManager (lines 488-495), and in RNMBXMapView.swift (lines 259-293). This creates a maintainability issue as any changes to this logic need to be applied in three places. Consider extracting this logic into a shared function that all three locations can call.
| #if RNMBX_11 | ||
| return OfflineRegionManager( | ||
| resourceOptions: .init( | ||
| accessToken: RNMBXModule.accessToken!, | ||
| dataPathURL: dataPathURL | ||
| ) | ||
| ) | ||
| #else | ||
| return OfflineRegionManager( | ||
| resourceOptions: .init( | ||
| accessToken: RNMBXModule.accessToken!, | ||
| dataPathURL: dataPathURL | ||
| ) | ||
| ) | ||
| #endif |
Copilot
AI
Dec 17, 2025
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.
The conditional compilation blocks for RNMBX_11 vs. the else branch are identical (lines 46-60). Both branches create an OfflineRegionManager with the same parameters. You can remove the conditional compilation and use a single implementation for both versions.
| #if RNMBX_11 | ||
| self.offlineRegionManager = OfflineRegionManager( | ||
| resourceOptions: .init( | ||
| accessToken: RNMBXModule.accessToken!, | ||
| dataPathURL: targetDir | ||
| ) | ||
| ) | ||
| #else | ||
| self.offlineRegionManager = OfflineRegionManager( | ||
| resourceOptions: .init( | ||
| accessToken: RNMBXModule.accessToken!, | ||
| dataPathURL: targetDir | ||
| ) | ||
| ) | ||
| #endif |
Copilot
AI
Dec 17, 2025
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.
The conditional compilation blocks for RNMBX_11 vs. the else branch are identical (lines 511-525). Both branches create an OfflineRegionManager with the same parameters. You can remove the conditional compilation and use a single implementation for both versions.
| // Generate random suffix, e.g. "A1B2" | ||
| let randomSuffix = UUID().uuidString.prefix(4) | ||
| let folderName = ".mapbox_custom/\(randomSuffix)/map_data" |
Copilot
AI
Dec 17, 2025
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.
The random suffix generation uses only the first 4 characters of a UUID, which could potentially lead to collisions. While UUID().uuidString.prefix(4) gives 65,536 possible values (16^4), for a more robust solution, consider using a larger portion of the UUID or a timestamp-based approach to ensure uniqueness, especially if this function might be called multiple times.
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.
agree about ts based approach. but some occasions in future, check uuid v7.
| fun hardResetDatabase(promise: Promise) { | ||
| Thread { | ||
| try { | ||
| val mapDataDir = File(mReactContext.filesDir.absolutePath + "/.mapbox/map_data") | ||
| Log.d(LOG_TAG, "hardResetDatabase: deleting ${mapDataDir.absolutePath}") | ||
| if (mapDataDir.exists()) { | ||
| mapDataDir.deleteRecursively() | ||
| } | ||
| Log.d(LOG_TAG, "hardResetDatabase: done") | ||
| promise.resolve(null) | ||
| } catch (t: Throwable) { | ||
| Log.e(LOG_TAG, "hardResetDatabase: failed", t) | ||
| promise.reject("hardResetDatabase error:", t) | ||
| } | ||
| }.start() | ||
| } |
Copilot
AI
Dec 17, 2025
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.
The promise resolution in hardResetDatabase is called directly from a background thread (line 465), which differs from the pattern used in other methods in this class (e.g., resetDatabase at line 388 uses UiThreadUtil.runOnUiThread). While React Native promises can typically be resolved from any thread, for consistency with the rest of the codebase and to avoid potential issues, consider using UiThreadUtil.runOnUiThread to resolve/reject the promise.
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.
Another agent suggested not to use UI thread. However, as deletion operation is fast, will switch to runOnUiThread
| let fileManager = FileManager.default | ||
| let appSupport = fileManager.urls(for: .applicationSupportDirectory, | ||
| in: .userDomainMask).first! |
Copilot
AI
Dec 17, 2025
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.
The force unwrap on fileManager.urls(for:in:).first! could crash if the application support directory is unexpectedly unavailable. While this is extremely unlikely in practice, consider using guard-let or if-let with proper error handling for better safety, similar to standard Swift practices.
| let fileManager = FileManager.default | ||
| let appSupport = fileManager.urls( | ||
| for: .applicationSupportDirectory, | ||
| in: .userDomainMask | ||
| ).first! |
Copilot
AI
Dec 17, 2025
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.
The force unwrap on fileManager.urls(for:in:).first! could crash if the application support directory is unexpectedly unavailable. While this is extremely unlikely in practice, consider using guard-let or if-let with proper error handling for better safety, similar to standard Swift practices.
|
cleanest setup:
i think you might have mentioned that above flow doesn't work. looking at the code, don't see why this is not possible. i'd use the same approach on both platforms. |
Almost the same approach was tested on iOS, but it doesn’t work. “Almost” because there is no way to unmount the offline manager on the Swift side (unless you’ve found a workaround?). We also thought this approach would work well on Android. Partially, yes: I ran several test iterations in debug with different offline data sizes (0.1–2.5 GB), and initially tested small sizes in production. No issues were noticed. However, after testing the deletion of a 2.5 GB dataset in production, the simpler approach failed. It turned out that this approach is not reliable for larger databases. To summarize, iOS and Android now follow the same hard reset flow, which is robust and validated for databases up to 2.5 GB. We will run one more iteration with a 4.5 GB database, as one user attempted to update/delete approximately that amount of data. |
|
Test result 1. Deleting ~4.6GB on iOS passed |
|
alternative unmount map view -> reinint offline manager -> destroy old db -> mount map view |
good work. i'm happy with this. i just felt it could be cleaner. |
|
No need anymore. Hard reset could be done fully from JS side by usign this flow |
Description
Added hardResetDatabase that includes 2 steps:
.mapbox_custom. To force Mapbox recreate a new database in a new folder. Otherwise, recreation doesn't work