Add viewport persistence and default offline map region support by torlando-tech · Pull Request #427 · torlando-tech/columba

torlando-tech · 2026-02-09T05:11:46Z

Summary

This PR adds two related features to improve the map experience:

Viewport Persistence: Save and restore the map camera position (center, zoom) when switching between tabs, preventing the map from resetting to the default location
Default Offline Map Region: Allow users to designate an offline map region as the default map center, used as a fallback when GPS location is unavailable

Key Changes

Map Viewport Persistence

Added SavedCameraPosition data class to store latitude, longitude, and zoom level
Modified MapScreen to save camera position on every camera move via saveCameraPosition()
Initialize map with saved viewport if available, preventing unwanted re-centering after tab switches
Updated hasInitiallyCentered logic to skip re-centering when viewport is restored from saved state

Default Offline Map Region

Added isDefault boolean column to OfflineMapRegionEntity (database migration 35→36)
Extended OfflineMapRegionRepository with methods to get, set, and clear default regions
Added UI controls in OfflineMapsScreen to toggle default status via star icon (only for completed regions)
Updated map initialization priority: saved viewport → GPS location → default region center → world view (0,0)
Display "Default" badge on offline map region cards that are marked as default

ViewModel Updates

MapViewModel now loads the default region center on initialization and uses it as fallback
OfflineMapsViewModel provides setDefaultRegion() and clearDefaultRegion() methods
Added defaultRegionCenter and lastCameraPosition to MapState

UI Improvements

Added star icon button to toggle default region status in OfflineMapRegionCard
Reorganized action buttons into a column layout for better spacing
Added "Default" label badge next to status chip for marked regions

Implementation Details

Camera position is saved on every camera move to ensure accuracy across tab switches
Only one region can be marked as default at a time (enforced by clearing previous default)
Default region feature only applies to completed offline map regions
Maintains backward compatibility with existing offline map data

https://claude.ai/code/session_01Ldx336WY2L1T4L34zpbQzM

greptile-apps · 2026-02-09T05:14:43Z

Greptile Summary

This PR adds two complementary features: viewport persistence (saving/restoring the map camera position across tab switches) and default offline map region support (using an offline region as the fallback map center when GPS is unavailable). The prior review feedback on debouncing camera saves and transaction-safe default region toggling has been addressed — camera saves now use onCameraIdle and the DAO's setDefaultRegion() is @Transaction-wrapped.

Viewport persistence: SavedCameraPosition stored in MapState, saved via onCameraIdle listener, restored with highest priority during map initialization
Default region: New isDefault column on OfflineMapRegionEntity with migration 35→36, star toggle in OfflineMapRegionCard, and fallback priority in MapScreen
Issue: MapViewModel loads defaultRegionCenter once in init and never refreshes it — if the user changes the default region mid-session, the map won't reflect the change until restart
Minor: OfflineMapRegionRepository.getDefaultRegion() KDoc claims a fallback behavior that the implementation doesn't provide

Confidence Score: 4/5

This PR is safe to merge with one logic gap around stale default region state that has low impact in practice.
The core features (viewport persistence and default region) are well-implemented with clean data layer changes, a correct Room migration, and transaction-safe DAO operations. The prior review feedback has been addressed. The main concern is that MapViewModel.defaultRegionCenter is loaded once and never refreshed, which means changing the default region mid-session won't take effect until restart. This is a low-priority fallback path (priority deps(deps): bump the testing group with 3 updates #3 of 4), so it's unlikely to cause user-visible issues in most scenarios.
Pay attention to MapViewModel.kt — the one-time default region loading creates a staleness gap. OfflineMapRegionRepository.kt has a misleading KDoc that should be corrected.

Important Files Changed

Filename	Overview
app/src/main/java/com/lxmf/messenger/ui/screens/MapScreen.kt	Adds camera position priority logic (saved > GPS > default region > world view) and `onCameraIdle` listener for viewport persistence. Clear logic, uses idle listener per prior review feedback.
app/src/main/java/com/lxmf/messenger/ui/screens/offlinemaps/OfflineMapsScreen.kt	Adds star icon toggle for default region and "Default" badge. Clean UI changes with appropriate conditional rendering for completed regions only.
app/src/main/java/com/lxmf/messenger/viewmodel/MapViewModel.kt	Adds `SavedCameraPosition` data class, `saveCameraPosition()`, and loads default region in init. Default region is loaded once and not refreshed, which creates a staleness issue.
app/src/main/java/com/lxmf/messenger/viewmodel/OfflineMapsViewModel.kt	Adds `setDefaultRegion()` and `clearDefaultRegion()` methods with proper error handling. Straightforward delegation to repository.
data/src/main/java/com/lxmf/messenger/data/db/ColumbaDatabase.kt	Version bump from 35 to 36. Consistent with the new migration.
data/src/main/java/com/lxmf/messenger/data/db/dao/OfflineMapRegionDao.kt	Adds `@Transaction`-wrapped `setDefaultRegion()` to atomically clear and set default. Addresses prior review feedback on race conditions. Clean DAO additions.
data/src/main/java/com/lxmf/messenger/data/db/entity/OfflineMapRegionEntity.kt	Adds `isDefault` boolean column with sensible default of `false`. Consistent with migration.
data/src/main/java/com/lxmf/messenger/data/di/DatabaseModule.kt	Adds MIGRATION_35_36 with correct `ALTER TABLE` for `isDefault` column. Migration is properly registered in `ALL_MIGRATIONS`.
data/src/main/java/com/lxmf/messenger/data/repository/OfflineMapRegionRepository.kt	Adds default region management methods. `getDefaultRegion()` has a misleading KDoc that contradicts its implementation (claims fallback that doesn't exist).

Flowchart

flowchart TD
    A[Map Initializes] --> B{Saved Camera Position?}
    B -- Yes --> C[Use saved lat/lng/zoom]
    B -- No --> D{GPS Location Available?}
    D -- Yes --> E[Use GPS coords, zoom 15]
    D -- No --> F{Default Offline Region?}
    F -- Yes --> G[Use region center, zoom 12]
    F -- No --> H["Fallback: 0,0 zoom 2 (world view)"]
    C --> I[Set Camera Position]
    E --> I
    G --> I
    H --> I
    I --> J[Register onCameraIdle Listener]
    J --> K[User pans/zooms map]
    K --> L[Camera becomes idle]
    L --> M[Save camera position to MapState]
    M --> K

_{Last reviewed commit: e56843a}

greptile-apps

_{5 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-02-09T05:14:46Z

data/src/main/java/com/lxmf/messenger/data/repository/OfflineMapRegionRepository.kt

+        suspend fun setDefaultRegion(id: Long) {
+            offlineMapRegionDao.clearDefaultRegion()
+            offlineMapRegionDao.setDefaultRegion(id)
+        }


Race condition: clearDefaultRegion() and setDefaultRegion(id) are separate database operations without transaction wrapping. If another thread calls setDefaultRegion() between these two calls, both regions could end up marked as default.

Wrap both operations in a @Transaction annotation or use a single SQL UPDATE query.

Prompt To Fix With AI

This is a comment left during a code review. Path: data/src/main/java/com/lxmf/messenger/data/repository/OfflineMapRegionRepository.kt Line: 299:302 Comment: Race condition: `clearDefaultRegion()` and `setDefaultRegion(id)` are separate database operations without transaction wrapping. If another thread calls `setDefaultRegion()` between these two calls, both regions could end up marked as default. Wrap both operations in a `@Transaction` annotation or use a single SQL UPDATE query. How can I resolve this? If you propose a fix, please make it concise.

app/src/main/java/com/lxmf/messenger/ui/screens/MapScreen.kt

torlando-tech · 2026-02-15T07:00:04Z

@greptileai review

greptile-apps

_{9 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-02-15T07:03:24Z

data/src/main/java/com/lxmf/messenger/data/repository/OfflineMapRegionRepository.kt

+         * Get the region marked as default map center.
+         * If no region is explicitly marked, returns the first completed region (auto-default).
+         */
+        suspend fun getDefaultRegion(): OfflineMapRegion? {
+            // Try explicitly marked default first
+            val explicit = offlineMapRegionDao.getDefaultRegion()
+            if (explicit != null) return explicit.toOfflineMapRegion()
+
+            // Fall back to first completed region (implicit default when only one exists)
+            return null
+        }


Misleading KDoc contradicts implementation

The docstring says "If no region is explicitly marked, returns the first completed region (auto-default)" but the fallback path just returns null. The comment on line 291 ("Fall back to first completed region") also contradicts the actual return null. This looks like a leftover from a previous iteration where auto-defaulting was planned but removed.

Either update the KDoc to reflect the actual behavior, or implement the described fallback.

Suggested change

* Get the region marked as default map center.

* If no region is explicitly marked, returns the first completed region (auto-default).

*/

suspend fun getDefaultRegion(): OfflineMapRegion? {

// Try explicitly marked default first

val explicit = offlineMapRegionDao.getDefaultRegion()

if (explicit != null) return explicit.toOfflineMapRegion()

// Fall back to first completed region (implicit default when only one exists)

return null

}

/**

* Get the region marked as default map center.

* Returns null if no region is explicitly marked as default.

*/

suspend fun getDefaultRegion(): OfflineMapRegion? {

return offlineMapRegionDao.getDefaultRegion()?.toOfflineMapRegion()

}

Prompt To Fix With AI

This is a comment left during a code review. Path: data/src/main/java/com/lxmf/messenger/data/repository/OfflineMapRegionRepository.kt Line: 283:293 Comment: **Misleading KDoc contradicts implementation** The docstring says "If no region is explicitly marked, returns the first completed region (auto-default)" but the fallback path just returns `null`. The comment on line 291 ("Fall back to first completed region") also contradicts the actual `return null`. This looks like a leftover from a previous iteration where auto-defaulting was planned but removed. Either update the KDoc to reflect the actual behavior, or implement the described fallback. ```suggestion /** * Get the region marked as default map center. * Returns null if no region is explicitly marked as default. */ suspend fun getDefaultRegion(): OfflineMapRegion? { return offlineMapRegionDao.getDefaultRegion()?.toOfflineMapRegion() } ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-02-15T07:03:25Z

app/src/main/java/com/lxmf/messenger/viewmodel/MapViewModel.kt

+            // Load default offline map region center as fallback for initial map position
+            viewModelScope.launch {
+                val defaultRegion = offlineMapRegionRepository.getDefaultRegion()
+                if (defaultRegion != null) {
+                    _state.update {
+                        it.copy(
+                            defaultRegionCenter = SavedCameraPosition(
+                                latitude = defaultRegion.centerLatitude,
+                                longitude = defaultRegion.centerLongitude,
+                                zoom = 12.0,
+                            ),
+                        )
+                    }
+                    Log.d(TAG, "Default region loaded: ${defaultRegion.name} at ${defaultRegion.centerLatitude}, ${defaultRegion.centerLongitude}")
+                }
+            }


defaultRegionCenter is loaded once and never refreshed

The default region center is loaded once in init and stored in MapState. If the user navigates to the Offline Maps screen and changes the default region, MapViewModel.state.defaultRegionCenter will still hold the old value (or remain null if no default was set at startup). Since MapViewModel is @HiltViewModel and persists across tab switches, this value won't update until the next app launch.

This matters when the map initializes without a saved camera position and without GPS — it would center on the old default (or 0,0) instead of the newly selected one. Consider observing the default region reactively, or re-loading it when the map screen is resumed.

Prompt To Fix With AI

This is a comment left during a code review. Path: app/src/main/java/com/lxmf/messenger/viewmodel/MapViewModel.kt Line: 183:198 Comment: **`defaultRegionCenter` is loaded once and never refreshed** The default region center is loaded once in `init` and stored in `MapState`. If the user navigates to the Offline Maps screen and changes the default region, `MapViewModel.state.defaultRegionCenter` will still hold the old value (or remain `null` if no default was set at startup). Since `MapViewModel` is `@HiltViewModel` and persists across tab switches, this value won't update until the next app launch. This matters when the map initializes without a saved camera position and without GPS — it would center on the old default (or 0,0) instead of the newly selected one. Consider observing the default region reactively, or re-loading it when the map screen is resumed. How can I resolve this? If you propose a fix, please make it concise.

…333) When no GPS location is available, the map now uses the center coordinates of the user's default offline map region instead of hardcoding San Francisco. Changes: - Add isDefault column to offline_map_regions table (migration 35->36) - Add DAO queries to get/set/clear default region - Add star toggle UI on completed offline map cards to set default region - Show "Default" badge next to status on default region cards - Save/restore camera viewport across tab switches to prevent reset - Fall back to world view (0,0 zoom 2) when no location or default region https://claude.ai/code/session_01Ldx336WY2L1T4L34zpbQzM

@transaction

…ra save - Wrap clearDefaultRegion + setDefaultRegionById in @transaction in DAO to prevent race condition where multiple regions could be marked default - Move saveCameraPosition from onCameraMoveListener to onCameraIdleListener to avoid excessive state updates during pan/zoom gestures https://claude.ai/code/session_01Ldx336WY2L1T4L34zpbQzM

- Capture delegated state.userLocation in a local val to enable smart cast - Use safe null-check on CameraPosition.target (nullable LatLng) - Suppress TooManyFunctions detekt warning on Room DAO interface Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

sentry · 2026-02-16T21:02:58Z

Codecov Report

❌ Patch coverage is 23.95833% with 73 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...in/java/com/lxmf/messenger/ui/screens/MapScreen.kt	0.00%	31 Missing ⚠️
.../java/com/lxmf/messenger/viewmodel/MapViewModel.kt	25.00%	14 Missing and 1 partial ⚠️
...senger/ui/screens/offlinemaps/OfflineMapsScreen.kt	56.25%	9 Missing and 5 partials ⚠️
...m/lxmf/messenger/viewmodel/OfflineMapsViewModel.kt	0.00%	13 Missing ⚠️

📢 Thoughts on this report? Let us know!

Constructor gained the new parameter after rebase conflict resolution. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

greptile-apps bot reviewed Feb 9, 2026

View reviewed changes

torlando-tech linked an issue Feb 9, 2026 that may be closed by this pull request

Center map at application start to the the center of offline map instead of San Francisco #333

Closed

torlando-tech added this to the v0.9.0 milestone Feb 9, 2026

greptile-apps bot reviewed Feb 15, 2026

View reviewed changes

claude and others added 3 commits February 16, 2026 15:59

torlando-tech force-pushed the claude/center-map-startup-SCmnI branch from e56843a to 2f129be Compare February 16, 2026 20:59

fix: add offlineMapRegionRepository mock to MapViewModelTest

b61f7c3

Constructor gained the new parameter after rebase conflict resolution. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

torlando-tech merged commit 6ad66a3 into main Feb 16, 2026
9 of 10 checks passed

torlando-tech deleted the claude/center-map-startup-SCmnI branch February 16, 2026 21:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add viewport persistence and default offline map region support#427

Add viewport persistence and default offline map region support#427
torlando-tech merged 4 commits intomainfrom
claude/center-map-startup-SCmnI

torlando-tech commented Feb 9, 2026

Uh oh!

greptile-apps bot commented Feb 9, 2026 •

edited

Loading

Uh oh!

greptile-apps bot left a comment

Uh oh!

greptile-apps bot Feb 9, 2026

Uh oh!

Uh oh!

torlando-tech commented Feb 15, 2026

Uh oh!

greptile-apps bot left a comment

Uh oh!

greptile-apps bot Feb 15, 2026

Uh oh!

greptile-apps bot Feb 15, 2026

Uh oh!

sentry bot commented Feb 16, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

torlando-tech commented Feb 9, 2026

Summary

Key Changes

Map Viewport Persistence

Default Offline Map Region

ViewModel Updates

UI Improvements

Implementation Details

Uh oh!

greptile-apps bot commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Flowchart

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

torlando-tech commented Feb 15, 2026

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Feb 15, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Feb 15, 2026

Choose a reason for hiding this comment

Uh oh!

sentry bot commented Feb 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

greptile-apps bot commented Feb 9, 2026 •

edited

Loading

sentry bot commented Feb 16, 2026 •

edited

Loading