Skip to content

feat(android,pwa): native Android build + offline caching + region download#377

Merged
giswqs merged 10 commits into
mainfrom
spike/tauri-android
Jun 15, 2026
Merged

feat(android,pwa): native Android build + offline caching + region download#377
giswqs merged 10 commits into
mainfrom
spike/tauri-android

Conversation

@giswqs

@giswqs giswqs commented Jun 15, 2026

Copy link
Copy Markdown
Member

Summary

Adds native Android support (Tauri v2 mobile), a responsive/touch layout pass, mobile feature-gating, slim signed APKs, and two PWA offline improvements — plus CI to build the Android APK.

Android (Tauri v2 mobile)

  • The crate compiles for Android. The only blocker was the desktop-only OAuth-popup window code — WebviewWindowBuilder::{on_new_window, window_features} don't exist on Tauri's mobile runtime — now behind #[cfg(desktop)]. Desktop behavior is unchanged (check:rust passes).
  • Slim APK: a release build (existing size-optimized + stripped Cargo profile) is ~41 MB vs ~208 MB for debug (the unstripped debug .so was 210 MB). tauri.android.conf.json also drops the bundled Python backend (unusable on Android).
  • Verified end-to-end: the signed 41 MB APK installs and runs on an Android 14 emulator, rendering the OpenFreeMap basemap with the responsive mobile layout.
  • New .github/workflows/android.yml builds the release APK in CI (JDK 21, Android SDK + NDK r27 LTS, Rust android targets) and signs it: with release-keystore secrets (ANDROID_KEYSTORE_BASE64 / ANDROID_KEYSTORE_PASSWORD / ANDROID_KEY_ALIAS / ANDROID_KEY_PASSWORD) when set, otherwise a throwaway debug keystore so the artifact is still installable for testing.

Mobile feature-gating

  • isTauri() is true on Android, so sidecar-backed tools previously appeared then failed. New UA-based isMobile() helper hides what can't run on a mobile OS:
    • Processing menu: Whitebox, Conversion, Raster, AI Segmentation (need the Python sidecar). Vector/Turf, SQL/PGlite, Python/Pyodide, geocode, statistics, network, assistant stay.
    • Add Data menu: PostgreSQL (Martin binary, no Android build).

Responsive / touch layout

  • On phone-width viewports (max-md), expanded Layers/Style panels overlay the map as slide-over sheets instead of squeezing it; collapsed bars stay in-flow. Resize handles are touch-capable (Pointer Events). Shared reactive useIsMobileViewport hook.

PWA / offline

  • Cache CDN engines: geolibre-cdn-engines CacheFirst rule caches Pyodide + PGlite/PostGIS for offline use after first load.
  • Download Offline Area (Project menu): pre-fetch the current view's basemap tiles/style/sprite/glyphs into the SW cache. New lib/offline-tiles.ts + OfflineRegionDialog, with unit tests.

Testing

  • Release APK installs + runs on an Android 14 emulator (real screenshot captured).
  • npm run build, npm run check:rust — pass.
  • Unit tests: tests/offline-tiles.test.ts (15) + tests/is-mobile.test.ts (4).
  • Responsive + mobile gating driven in a real browser with Playwright (Android UA vs desktop).
  • pre-commit run --files (eslint + npm build) — passes on every change.

Notes / follow-ups

  • For Play Store distribution, set the ANDROID_KEYSTORE_* repo secrets so CI signs with a real upload key (the debug-keystore fallback is test-only).
  • Pin the third-party android-actions/setup-android action to a commit SHA before relying on it on protected branches.
  • Local-file sources under Android scoped storage (content URIs vs absolute paths for MBTiles/raster/project files) remain a follow-up.

Summary by CodeRabbit

Release Notes

  • New Features
    • Added “Offline Area” tile pre-download (zoom range selection, progress, cancel) with warnings when no service worker is controlling the page and highlights for uncacheable hosts (desktop).
  • Bug Fixes
    • Improved offline download URL collection by expanding tile templates, quadkeys, subdomains, and required sprite/glyph assets for caching.
  • Tests
    • Added unit tests for offline tile/quadkey utilities and mobile user-agent detection.
  • Chores
    • Updated mobile/resize interactions to pointer events, improved mobile safe-area layout, hid select processing options on mobile, and refined PWA caching plus Android CI APK signing workflow.

@netlify

netlify Bot commented Jun 15, 2026

Copy link
Copy Markdown

Deploy Preview for geolibre-app ready!

Name Link
🔨 Latest commit b68fd74
🔍 Latest deploy log https://app.netlify.com/projects/geolibre-app/deploys/6a2f7f87d166ee0008f4da8f
😎 Deploy Preview https://deploy-preview-377--geolibre-app.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai

coderabbitai Bot commented Jun 15, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR introduces an offline map tile pre-download feature via a new offline-tiles utility library, OfflineRegionDialog component, and toolbar wiring. It adds Android/mobile support through a shared useIsMobileViewport hook, migrates panel resize handlers from mouse to pointer events, introduces isMobile detection to hide platform-incompatible features, gates Rust OAuth code for desktop-only builds, adds mobile safe-area CSS support, and creates an Android release APK CI workflow with conditional signing.

Changes

Offline Map Region Download

Layer / File(s) Summary
Tile math library, URL expansion, and tests
apps/geolibre-desktop/src/lib/offline-tiles.ts, tests/offline-tiles.test.ts
Defines Bbox/TileCoord types, Web Mercator lngLatToTile with latitude clamping, bbox-to-tile-range logic, tile enumeration/counting, quadkey generation, and URL template expansion ({z}/{x}/{y}, TMS {-y}, {quadkey}, {s}). Tests assert coordinate mappings, bbox-range ordering, count/enumeration consistency, and template substitution including Bing canonical quadkey examples.
URL collection, warming, and service worker detection
apps/geolibre-desktop/src/lib/offline-tiles.ts
collectOfflineUrls() walks MapLibre style sources for tile/sprite/glyph URLs with optional TileJSON fetching. warmUrls() runs bounded concurrent fetches with AbortSignal support tracking done/failed counts. hasActiveServiceWorker() checks page controller state.
OfflineRegionDialog component and i18n strings
apps/geolibre-desktop/src/components/layout/OfflineRegionDialog.tsx, apps/geolibre-desktop/src/i18n/locales/en.json
Adds OfflineRegionDialog with tile-count estimation, uncacheable-host scanning, zoom slider, async download/cancel via AbortController, progress tracking, and conditional phase-based UI. Adds common.offline.* and toolbar.item.offlineRegionEllipsis locale strings.
ProjectMenu and TopToolbar wiring
apps/geolibre-desktop/src/components/layout/toolbar/ProjectMenu.tsx, apps/geolibre-desktop/src/components/layout/TopToolbar.tsx
Extends ProjectMenuProps with onDownloadOffline callback, adds HardDriveDownload dropdown item in ProjectMenu, and wires TopToolbar to manage offlineRegionOpen state and render OfflineRegionDialog.
PWA Workbox caching rule updates
apps/geolibre-desktop/vite.config.ts
Adds CacheFirst rule for cdn.jsdelivr.net (Pyodide/PGlite) under geolibre-cdn-engines, increases geolibre-basemaps entry/age limits, and updates service-worker comments to clarify web vs. Tauri desktop behavior.

Android Port and Mobile Polish

Layer / File(s) Summary
isMobile user-agent detection utility and tests
apps/geolibre-desktop/src/lib/is-mobile.ts, tests/is-mobile.test.ts
Adds isMobile() function implementing Android/iOS user-agent regex detection with optional override. Tests validate Android, iPhone, iPad detection and false cases for desktop browsers and empty agents.
Shared useIsMobileViewport hook
apps/geolibre-desktop/src/hooks/useIsMobileViewport.ts
Adds getIsMobileViewport() for one-time matchMedia checks and useIsMobileViewport() hook with reactive viewport change listener and cleanup, replacing duplicated in-file mobile detection helpers.
Pointer event migration for panel resize handlers
apps/geolibre-desktop/src/components/layout/DesktopShell.tsx, apps/geolibre-desktop/src/components/panels/LayerPanel.tsx, apps/geolibre-desktop/src/components/panels/StylePanel.tsx
Migrates startLayerPanelResize and startStylePanelResize from mousemove/mouseup to pointermove/pointerup. Updates onResizeStart prop types to ReactPointerEvent, replaces local isMobileViewport() with getIsMobileViewport, adds max-md bottom-sheet CSS to StylePanel, and changes outer container from h-full to min-h-0.
Platform-specific menu item visibility
apps/geolibre-desktop/src/components/layout/toolbar/AddDataMenu.tsx, apps/geolibre-desktop/src/components/layout/toolbar/ProcessingMenu.tsx
Adds isMobile detection to conditionally hide PostgreSQL data source, Whitebox/processing, Conversion, Raster, and Segmentation entries on mobile due to Python sidecar incompatibility.
Rust #[cfg(desktop)] gating for OAuth popup windows
apps/geolibre-desktop/src-tauri/src/lib.rs
Adds desktop-only cfg guard comment, conditionally attaches on_new_window OAuth handler only on desktop builds, and gates create_oauth_popup_window behind #[cfg(desktop)].
Mobile safe-area CSS and viewport configuration
apps/geolibre-desktop/index.html, apps/geolibre-desktop/src/index.css
Sets viewport viewport-fit=cover meta tag for safe-area inset support on notched devices. Applies env(safe-area-inset-*) padding to #root element to prevent app chrome from rendering under mobile system bars and cutouts.
Android release APK CI workflow and config
.github/workflows/android.yml, apps/geolibre-desktop/src-tauri/tauri.android.conf.json
New path-filtered GitHub Actions workflow with Node 22 / JDK 21 / Android SDK 34 / NDK 27.3 setup, Rust stable with Android targets, Rust cache, npm ci, tauri android init, release APK build split per ABI, conditional signing (release keystore or debug keystore), zipalign verification, and artifact upload with 14-day retention. Includes Tauri v2 Android config with empty resources bundle.

Sequence Diagram(s)

sequenceDiagram
  rect rgba(100, 149, 237, 0.5)
    note over User,ServiceWorker: Offline Region Download Flow
  end
  participant User
  participant OfflineRegionDialog
  participant offline_tiles as offline-tiles
  participant MapLibreStyle as MapLibre Style
  participant TileJSON
  participant ServiceWorker

  User->>OfflineRegionDialog: click "Download Offline Region"
  OfflineRegionDialog->>offline_tiles: collectOfflineUrls(map, bbox, minZoom, maxZoom)
  offline_tiles->>MapLibreStyle: read sources (vector/raster), sprites, glyphs
  offline_tiles->>TileJSON: fetch TileJSON (if tiles[] missing)
  TileJSON-->>offline_tiles: tile URL templates
  offline_tiles-->>OfflineRegionDialog: deduplicated absolute URL[]
  OfflineRegionDialog->>offline_tiles: warmUrls(urls, { signal, onProgress })
  loop bounded concurrency workers
    offline_tiles->>ServiceWorker: fetch(url) → cache response
    offline_tiles-->>OfflineRegionDialog: onProgress({ done, total, failed })
    OfflineRegionDialog-->>User: update progress bar
  end
  offline_tiles-->>OfflineRegionDialog: WarmProgress final
  OfflineRegionDialog-->>User: show completion status
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • opengeos/GeoLibre#287: Both PRs modify apps/geolibre-desktop/vite.config.ts to configure PWA/service-worker behavior and Workbox runtime caching for CDN-loaded engines (Pyodide/PGlite).
  • opengeos/GeoLibre#280: Both PRs modify the app's PWA/service-worker behavior—this PR adjusts Workbox runtimeCaching rules for basemap and CDN "heavy engines" caching strategies.

Poem

🐇 Hippity-hop, I cached the map today,
Tiles and quadkeys tucked neatly away!
Pointer events for fingers that swipe,
Android builds now perfectly ripe. 🗺️
No more spinning when the signal goes dark—
This bunny downloaded the whole offline park! 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 58.62% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: native Android build support via Tauri, PWA offline caching capabilities, and offline region download functionality.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch spike/tauri-android

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment thread .github/workflows/android.yml Outdated
Comment thread apps/geolibre-desktop/src/lib/offline-tiles.ts
Comment thread apps/geolibre-desktop/src/lib/offline-tiles.ts Outdated
Comment thread apps/geolibre-desktop/src/lib/offline-tiles.ts Outdated
Comment thread apps/geolibre-desktop/src/components/layout/OfflineRegionDialog.tsx

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/android.yml:
- Around line 54-56: The `android-actions/setup-android@v3` action on line 56
uses a mutable version tag which weakens supply-chain integrity. Replace the
`@v3` tag reference with a pinned commit SHA (for example, `@abc123def...`) to
ensure the exact version of this third-party action is locked and cannot change
unexpectedly. Check the action's repository or releases page to identify the
appropriate commit SHA for the desired version.

In `@apps/geolibre-desktop/src/components/layout/OfflineRegionDialog.tsx`:
- Around line 86-104: The uncacheableHosts useMemo hook currently checks
spec.tiles?.[0] ?? spec.url for host detection, but this approach misses actual
tile hosts when sources use TileJSON indirection (spec.url pointing to a
TileJSON endpoint). For sources lacking a tiles array but having a url field,
fetch and parse the TileJSON file to extract the real tile templates from the
resolved TileJSON's tiles array, then validate those hosts against
CACHED_TILE_HOST instead of just checking the TileJSON endpoint's host. This
ensures all potential tile hosts are properly detected and validated for the
offline warning.

In `@apps/geolibre-desktop/src/lib/offline-tiles.ts`:
- Around line 177-204: The fetch call in the collectOfflineUrls function
performs a network request without an AbortSignal, making it impossible to
cancel the operation if the TileJSON endpoint is slow or unreachable. Add an
optional options parameter to the collectOfflineUrls function signature to
accept an AbortSignal, then pass this signal to the fetch() call within the try
block so that cancellations can properly abort the TileJSON discovery. Update
the call site to pass the controller.signal through the new options parameter.
- Around line 281-307: The concurrency parameter is not validated before being
used to determine the number of workers in the Array.from call. If concurrency
is 0 or negative, Math.min(concurrency, urls.length) will result in zero workers
being created, causing the warming function to silently return without
processing any URLs. Add a guard to clamp concurrency to a minimum of 1 before
calculating the array length. This ensures at least one worker is always spawned
to process the URLs, even if an invalid concurrency value is passed.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 6a98731b-b0af-413c-957e-2215622a7d03

📥 Commits

Reviewing files that changed from the base of the PR and between c08f427 and 7f2b9e4.

📒 Files selected for processing (10)
  • .github/workflows/android.yml
  • apps/geolibre-desktop/src-tauri/src/lib.rs
  • apps/geolibre-desktop/src/components/layout/OfflineRegionDialog.tsx
  • apps/geolibre-desktop/src/components/layout/TopToolbar.tsx
  • apps/geolibre-desktop/src/components/layout/toolbar/ProjectMenu.tsx
  • apps/geolibre-desktop/src/i18n/locales/en.json
  • apps/geolibre-desktop/src/lib/offline-tiles.ts
  • apps/geolibre-desktop/vite.config.ts
  • docs/android-port-spike.md
  • tests/offline-tiles.test.ts

Comment thread .github/workflows/android.yml Outdated
Comment thread apps/geolibre-desktop/src/components/layout/OfflineRegionDialog.tsx
Comment thread apps/geolibre-desktop/src/lib/offline-tiles.ts
Comment thread apps/geolibre-desktop/src/lib/offline-tiles.ts
@github-actions

Copy link
Copy Markdown
Contributor

Code review

Reviewed: .github/workflows/android.yml, apps/geolibre-desktop/src-tauri/src/lib.rs, apps/geolibre-desktop/src/lib/offline-tiles.ts, apps/geolibre-desktop/src/components/layout/OfflineRegionDialog.tsx, apps/geolibre-desktop/vite.config.ts, tests/offline-tiles.test.ts, docs/android-port-spike.md, apps/geolibre-desktop/src/i18n/locales/en.json.


Security

Finding Confidence
Unpinned android-actions/setup-android@v3 — floating mutable tag on a third-party action is a supply-chain risk; the PR comment even flags it as "should be pinned before protected branches" but leaves it unfixed. Inline comment on android.yml line 56. High

Bugs

Finding Confidence
expandTileUrl called without subdomains in collectOfflineUrls — tile templates containing {s} get replaced with "", producing invalid URLs like https://.tiles.example.com/. Inline comment on offline-tiles.ts:213. Medium
Sprite array format silently skipped — MapLibre allows style.sprite as {id,url}[]. The typeof sprite === "string" guard drops those styles, leaving icon sheets uncached offline. Inline comment on offline-tiles.ts:218. Medium
finally runs after abort-triggered return from catch — JS always executes finally even when return is issued from catch, so aborted fetches still increment progress.done. Practical impact is low (phase resets to idle via handleCancel) but the counter is semantically wrong. Inline comment on offline-tiles.ts:299-305. Medium
No error state in handleDownload — if collectOfflineUrls throws (e.g., TileJSON network error), the bare finally calls setPhase("done") with progress.total === 0, showing a misleading "Saved 0 of 0 tiles" message with no failure indication. Inline comment on OfflineRegionDialog.tsx:129-141. Medium
Closing the dialog does not abort the download — the cleanup useEffect(() => () => abortRef.current?.abort(), []) only fires on component unmount; Dialog stays mounted when open flips to false. If the user closes and reopens during a download, the reset effect fires but warmUrls resolves and clobbers the reset state shortly after. Inline comment on OfflineRegionDialog.tsx:129-141. Medium

Performance

Finding Confidence
geolibre-basemaps SW cache raised from 600 to 8,000 entries / 7 to 30 days — at 30 KB/tile that is ~240 MB of SW cache. The motivation is sound (offline region download needs room), but users on constrained storage may notice. Worth tracking as a follow-up (per-region cap instead of global raise). Low — deliberate trade-off

Quality / Nits

  • {s} subdomain replacement in expandTileUrl falls back to "" (line 144) — for templates like https://{s}.example.com/ with no subdomains arg this produces an invalid URL. Covered by the subdomains bug above but expandTileUrl could also drop the placeholder rather than insert an empty string.
  • collectOfflineUrls silently swallows TileJSON fetch failures (lines 205-207). A console.warn would aid debugging of incomplete offline regions.

CLAUDE.md

No violations. i18n keys added to en.json match every t() call in OfflineRegionDialog; common.close was pre-existing. The Rust change is cleanly #[cfg(desktop)]-gated with no desktop-behavior change. No direct MapLibre mutations bypass the store.


Overall: The Android cfg-gate is a clean, minimal fix. The PWA offline feature is well-designed (indirect-fetch strategy, bounded concurrency, size estimate preview, good tile-math test coverage). Primary items to address before merging: pin the third-party CI action, fix the two OfflineRegionDialog lifecycle issues (error state and close-without-cancel), and handle the subdomains/sprite-array gaps in collectOfflineUrls.

Comment thread .github/workflows/android.yml
Comment thread .github/workflows/android.yml
Comment thread .github/workflows/android.yml
Comment thread apps/geolibre-desktop/src/components/layout/DesktopShell.tsx
Comment thread apps/geolibre-desktop/src/components/layout/DesktopShell.tsx

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@apps/geolibre-desktop/src/components/layout/DesktopShell.tsx`:
- Around line 922-925: The onPointerUp handler in startLayerPanelResize and
startStylePanelResize only detaches event listeners when a pointer up event
occurs. If a pointercancel event fires instead or the component unmounts during
a resize, the global event listeners remain attached and document.body styles
stay locked with cursor and userSelect values. Add a pointercancel event
listener that performs the same cleanup as onPointerUp, and ensure proper
cleanup when the component unmounts. Apply this same pattern to both
startLayerPanelResize and startStylePanelResize functions to handle all teardown
scenarios safely.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 216c0216-ee28-43fa-8362-cc919ea52057

📥 Commits

Reviewing files that changed from the base of the PR and between 7f2b9e4 and f02b23b.

📒 Files selected for processing (4)
  • apps/geolibre-desktop/src/components/layout/DesktopShell.tsx
  • apps/geolibre-desktop/src/components/panels/LayerPanel.tsx
  • apps/geolibre-desktop/src/components/panels/StylePanel.tsx
  • apps/geolibre-desktop/src/hooks/useIsMobileViewport.ts

Comment thread apps/geolibre-desktop/src/components/layout/DesktopShell.tsx
Comment thread apps/geolibre-desktop/src/lib/offline-tiles.ts
Comment thread apps/geolibre-desktop/src/components/layout/OfflineRegionDialog.tsx
Comment thread apps/geolibre-desktop/src/components/layout/OfflineRegionDialog.tsx Outdated
@github-actions

Copy link
Copy Markdown
Contributor

Code review

Reviewed: Android CI workflow, Rust #[cfg(desktop)] gates, pointer-events resize migration, useIsMobileViewport hook, offline tile math library + tests, OfflineRegionDialog, PWA Workbox config changes, and i18n strings.


Bugs

# File Finding Confidence
1 .github/workflows/android.yml L42, L47 actions/checkout@v6 and actions/setup-node@v6 do not exist — the latest major version for both is @v4. GitHub Actions will fail with "Unable to resolve action" before a single build step runs. (Inline comments posted.) High
2 DesktopShell.tsx startLayerPanelResize and startStylePanelResize Missing pointercancel listener. On mobile, the browser fires pointercancel instead of pointerup when a native scroll gesture wins, the OS interrupts the touch, or the pointer leaves the viewport. Without a pointercancel handler the cleanup in onPointerUp never runs: cursor: col-resize and userSelect: none get permanently stuck on document.body, and the pointermove/pointerup listeners accumulate without being removed. (Inline comments posted.) High
3 lib/offline-tiles.ts L234-244 encodeURIComponent(stack) encodes commas, breaking glyph SW cache-key matching. MapLibre substitutes {fontstack} into the glyph URL template with a raw string replace (no comma encoding). The pre-warmed URL will contain %2C between font names; MapLibre's actual glyph request uses a literal comma. The service-worker CacheFirst route won't match, so offline label rendering silently falls back to the network (or fails offline entirely), defeating the purpose of warming glyphs. Fix: encode each font name individually and join with literal commas (suggestion posted inline). High

Security

# File Finding Confidence
4 .github/workflows/android.yml L62 android-actions/setup-android@v3 is unpinned. The PR description acknowledges this but defers it; it should be resolved before this workflow runs on protected branches. A compromised tag push could substitute arbitrary code into CI. (Inline comment posted.) Medium

Performance / Storage

# File Finding Confidence
5 vite.config.ts basemaps cache maxEntries: 8000 at a 30-day TTL with no user-visible storage indicator. At a conservative 50 KB/tile average, 8 000 entries can silently occupy ~400 MB of Cache Storage. The previous limit was 600 entries / 7 days. Workbox evicts the oldest entries once the cap is hit, but until then storage pressure is the only backstop (behaviour varies widely across browsers and devices). Consider a storage-usage hint in the dialog or a code comment noting the upper-bound implication. Medium

Quality

# File Finding Confidence
6 OfflineRegionDialog.tsx L121-140 Unexpected error in collectOfflineUrls produces a misleading "Saved 0 of 0 tiles" done state. If collectOfflineUrls throws, the setProgress({ total: urls.length }) call is skipped and finally still calls setPhase("done"). The UI shows a success message with all-zero counters. (Suggestion with restructured error handling posted inline; low probability since collectOfflineUrls swallows TileJSON fetch errors internally.) Low-medium
7 OfflineRegionDialog.tsx L235 Download button enabled when no service worker is active. The amber warning explains tiles won't persist, but the button is still clickable. A user who misses the notice can trigger a large batch fetch that accomplishes nothing. Suggest `disabled={...
8 hooks/useIsMobileViewport.ts useIsMobileViewport (the reactive hook) is exported but never imported anywhere in this PR. getIsMobileViewport (one-shot) is the only import. Not a bug, but the unused reactive export may confuse readers. Nit

What looks good

  • Rust #[cfg(desktop)] gates are correctly scoped: POPUP_COUNTER, create_oauth_popup_window, and the on_new_window handler are all gated together, so mobile builds compile without dead-code warnings.
  • Pointer-events migration in DesktopShell, LayerPanel, and StylePanel is mechanically correct (touch-none CSS, onPointerDown forwarding to window-level pointermove/pointerup) — the only gap is the missing pointercancel (finding 2 above).
  • Tile math (lngLatToTile, tileRangeForBbox, countTiles, expandTileUrl, tileToQuadkey) is correct and well-tested; the 15 unit tests cover boundary clamping, lat-row inversion, and the canonical Bing quadkey example.
  • warmUrls bounded concurrency + AbortSignal design is solid; the abort-check pattern correctly avoids calling onProgress after cancellation.
  • hasActiveServiceWorker correctly checks navigator.serviceWorker.controller (not just 'serviceWorker' in navigator), distinguishing an installed-and-active SW from one that has registered but not yet taken control.
  • i18n — all new strings are routed through t() and keys are added to en.json (the source-of-truth catalog per CLAUDE.md). No hard-coded user-facing strings in the new components.

Comment thread apps/geolibre-desktop/src/lib/offline-tiles.ts Outdated
Comment thread apps/geolibre-desktop/src/lib/offline-tiles.ts Outdated
Comment thread apps/geolibre-desktop/src/components/layout/DesktopShell.tsx
Comment thread apps/geolibre-desktop/src/components/layout/OfflineRegionDialog.tsx Outdated
Comment thread .github/workflows/android.yml Outdated
@github-actions

Copy link
Copy Markdown
Contributor

Code review

Reviewed: Android/PWA support, mobile feature-gating, pointer-event resize migration, offline tile warming, and CI workflow.

Bugs

# File Finding Confidence
1 lib/offline-tiles.ts:294–298 warmUrls: finally runs after catch { return } — when a fetch is aborted the worker's return inside catch does not skip finally, so progress.done++ fires for each aborted request. Aborted tiles are counted as successful completions, inflating the progress counter. Low user-visible impact (dialog resets to idle after cancel) but logically wrong. High
2 lib/offline-tiles.ts:217–223 Sprite array format silently skippedstyle.sprite can be a SpriteSpecification[] (MapLibre GL v3+). The code only handles the string case; array-sprite basemaps (e.g. styles using multiple sprite sheets) won't have their icon assets pre-fetched, leaving icon layers blank offline. Medium
3 OfflineRegionDialog.tsx:122–139 handleDownload has no catch — if collectOfflineUrls throws unexpectedly (e.g. unexpected JSON), the async function's rejection is unhandled (React doesn't surface promise rejections from onClick). The finally block would set phase("done") showing "Saved 0 of 0 tiles" with no explanation. Medium
4 OfflineRegionDialog.tsx:80–85 Tile count can silently exceed SW cache cap — at z=15 + 5 extra levels over a neighbourhood, countTiles returns ~21 k entries, but the SW geolibre-basemaps cache is capped at maxEntries: 8000. Workbox's LRU evicts the oldest as new tiles arrive, so by the time warming finishes most early tiles are gone. The user sees a "Saved N tiles" success message but the offline area has large gaps. A warning when tileCount > 8000 would help. Medium

Security

# File Finding Confidence
5 .github/workflows/android.yml:56 android-actions/setup-android@v3 unpinned — unlike dtolnay/rust-toolchain and Swatinem/rust-cache (both pinned to commit SHAs), this third-party action uses a mutable tag. A compromised tag update could exfiltrate VITE_GEE_OAUTH_CLIENT_ID. The PR description itself flags this as a follow-up; it should be resolved before the workflow runs on protected branches. High

Quality

# File Finding Confidence
6 DesktopShell.tsx:882 (and ~948) Missing setPointerCapture for touch drag — the PR migrates resize handles to Pointer Events specifically for touch support, but doesn't call event.currentTarget.setPointerCapture(event.pointerId) on pointerDown. Without capture, a fast finger-lift outside the browser viewport may not deliver pointerup to window, leaving the panel in a permanently-dragging state. setPointerCapture is the standard fix for this class of pointer-drag bugs. Medium
7 toolbar/AddDataMenu.tsx:571, ProcessingMenu.tsx:610 isMobile() called bare in rendernavigator.userAgent is stable but calling isMobile() on every render is wasteful. A module-level constant or a single useMemo(() => isMobile(), []) per component would be cleaner and clearer about the value being static. Low (nit)

What looks good

  • Rust #[cfg(desktop)] guard is correctly scoped: POPUP_COUNTER static, the on_new_window closure, and create_oauth_popup_window are all gated, and WebviewWindowBuilder::from_config is shared — clean split.
  • useIsMobileViewport hook (shared query constant, SSR-safe matchMedia guard, correct event listener lifecycle with sync on mount) is a solid extraction.
  • warmUrls concurrency design (shared cursor across coroutines, bounded parallelism via Array.from) is correct; JavaScript's cooperative scheduling makes the cursor increment safe without locks.
  • Tile math (lngLatToTile, tileRangeForBbox, expandTileUrl, tileToQuadkey) is correct and well-tested; the quadkey test against the canonical Bing example is a nice touch.
  • CI workflow permissions (contents: read), persist-credentials: false, and cancel-in-progress concurrency are all correctly set.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@apps/geolibre-desktop/src/lib/is-mobile.ts`:
- Around line 18-26: The MOBILE_UA_PATTERN regex in is-mobile.ts misses iPadOS
devices that report desktop-style user agents (containing "Macintosh" with
"Mobile/" string). Update the MOBILE_UA_PATTERN constant to also match these
desktop-class iPad user agent strings in addition to the existing Android,
iPhone, iPad, and iPod patterns. Then add a regression test case in
tests/is-mobile.test.ts that verifies isMobile() correctly returns true for a
desktop-class iPad user agent string (one containing "Macintosh" and "Mobile/").
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 0b20cd3e-baea-4faa-ab65-c6462c0c139d

📥 Commits

Reviewing files that changed from the base of the PR and between f02b23b and 0936dc6.

📒 Files selected for processing (4)
  • apps/geolibre-desktop/src/components/layout/toolbar/AddDataMenu.tsx
  • apps/geolibre-desktop/src/components/layout/toolbar/ProcessingMenu.tsx
  • apps/geolibre-desktop/src/lib/is-mobile.ts
  • tests/is-mobile.test.ts

Comment thread apps/geolibre-desktop/src/lib/is-mobile.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/android.yml:
- Around line 118-124: Add validation checks in the branch where KEYSTORE_BASE64
is checked to ensure that KEYSTORE_PASSWORD and KEY_ALIAS environment variables
are also provided. Add explicit precheck conditions after verifying
KEYSTORE_BASE64 is set, and emit a clear CI error message (using echo with exit
status) if either KEYSTORE_PASSWORD or KEY_ALIAS is missing. This allows the
workflow to fail fast with a clear error message before attempting to use
incomplete keystore configuration in apksigner.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 3bc9d6db-8cd6-40c8-9806-b7110aca1c1f

📥 Commits

Reviewing files that changed from the base of the PR and between 0936dc6 and 223ba0e.

📒 Files selected for processing (2)
  • .github/workflows/android.yml
  • apps/geolibre-desktop/src-tauri/tauri.android.conf.json

Comment thread .github/workflows/android.yml
Comment thread apps/geolibre-desktop/src/components/layout/OfflineRegionDialog.tsx
Comment thread .github/workflows/android.yml Outdated
Comment thread .github/workflows/android.yml Outdated
giswqs added 2 commits June 14, 2026 23:53
Override productName to "GeoLibre" in tauri.android.conf.json so the Android
launcher label and activity title read "GeoLibre". The desktop build keeps
"GeoLibre Desktop" (base tauri.conf.json). Verified a clean `tauri android init`
generates app_name=GeoLibre in strings.xml.
The foreground filled the full 108dp canvas, so the launcher's adaptive mask
cropped the globe's edges. Scale the foreground to ~64% centered (all densities)
so the whole icon stays within the ~66dp safe zone and is never clipped.
@giswqs
giswqs force-pushed the spike/tauri-android branch from 628b314 to e0b7347 Compare June 15, 2026 03:53
Comment thread .github/workflows/android.yml
Comment thread .github/workflows/android.yml
Comment thread apps/geolibre-desktop/src/lib/is-mobile.ts Outdated
Comment thread apps/geolibre-desktop/src/lib/offline-tiles.ts
Comment thread apps/geolibre-desktop/src/lib/offline-tiles.ts
Comment thread apps/geolibre-desktop/src/components/layout/toolbar/AddDataMenu.tsx Outdated
@github-actions

Copy link
Copy Markdown
Contributor

Code review

Reviewed: CI workflow, Rust mobile gating, offline tile download, responsive layout, is-mobile detection, and PWA cache rules. No approval/merge action taken.


Security

# Finding Confidence
S1 Unpinned third-party CI actionandroid-actions/setup-android@v3 is a mutable tag. A malicious tag update would execute with access to all job secrets (ANDROID_KEYSTORE_*, VITE_GEE_OAUTH_CLIENT_ID, etc.). The PR comment on that line acknowledges the gap but ships it unfixed. Pin to a commit SHA before this workflow runs on protected branches. High

Bugs

# Finding Confidence
B1 iPad UA false-negative (is-mobile.ts) — iPadOS 13+ Safari spoofs a macOS user-agent by default. isMobile() returns false, so sidecar-backed menu items (Whitebox, Conversion, Raster, AI Segmentation) appear on modern iPads running the PWA and then fail. Native Tauri/WKWebView iOS is unaffected (still reports "iPad"). Adding a maxTouchPoints > 1 heuristic for macOS UAs covers this. Medium
B2 warmUrls inflates done on abort (offline-tiles.ts) — A return inside a catch block does not skip the finally; progress.done++ fires for every in-flight fetch that was aborted, overcounting relative to progress.total. Not user-visible (the dialog resets phase to "idle" on cancel), but misleads future callers of onProgress. Low–Medium

Performance

# Finding Confidence
P1 vite.config.ts cache cap raised from 600 → 8 000 tiles — At ~30 KB/tile the "geolibre-basemaps" cache can now consume ~240 MB of browser storage. This is intentional for offline area downloads, but worth monitoring on low-storage devices. Informational

Quality

# Finding Confidence
Q1 Misleading CI job name (android.yml:39) — Build Android APK (debug) actually produces release APKs (no --debug flag). Inline comment posted. High
Q2 {s} subdomain placeholder expands to empty string (offline-tiles.ts) — Only templates[0] is consumed and subdomains is never passed to expandTileUrl, so a tile template like https://{s}.tile.example.com/{z}/{x}/{y}.png generates malformed URLs. Low practical impact because non-OpenFreeMap/CARTO tiles are not SW-cached anyway, but failure counts would be misleading. Medium
Q3 isMobile() called on every render (AddDataMenu.tsx, ProcessingMenu.tsx) — The UA is stable for the page lifetime; wrapping in useMemo(() => isMobile(), []) or a module-level constant avoids redundant regex evaluation on each render. Low

CLAUDE.md

No violations found. All new user-facing strings go through t() with matching en.json entries; Rust mobile gating via #[cfg(desktop)] follows the established Tauri pattern; CSS overrides are in index.css not node_modules; the new useIsMobileViewport hook correctly consolidates the duplicate isMobileViewport functions that existed independently in LayerPanel and StylePanel.

giswqs added 2 commits June 15, 2026 00:06
- README: native Android app + Download Offline Area + engine offline caching in
  the v1.3 feature list, an Android build/run section, and a Reference link.
- New docs/android.md: toolchain setup, build, signing, sideload, and emulator
  guide (added to mkdocs nav).
- roadmap: a "Mobile, offline, and Android" subsection under v1.3.
- index: mention Android/mobile/offline in the v1.3 status summary.

Also: build the Android APK only on published releases (and on demand), not on
every PR — the Rust cross-compile + Gradle build is slow.
On Android the dropdown max-height used 100vh / Radix's available-height, both
based on the layout viewport, which extends under the system status/navigation
bars. Long menus (e.g. Add Data) then ran their last items under the nav bar
where they were clipped and unreachable. Cap the max-height with dvh minus the
top/bottom safe-area insets so the menu (and its scroll area) stays within the
visible region and scrolls to the end. Desktop/web are unchanged (dvh == vh and
insets are 0).
Comment thread apps/geolibre-desktop/src/components/layout/DesktopShell.tsx
Comment thread apps/geolibre-desktop/src/components/layout/OfflineRegionDialog.tsx Outdated
Comment thread apps/geolibre-desktop/src/lib/offline-tiles.ts Outdated
Comment thread .github/workflows/android.yml Outdated
// would otherwise evict its own freshly-cached tiles past 600.
cacheName: "geolibre-basemaps",
expiration: { maxEntries: 600, maxAgeSeconds: 60 * 60 * 24 * 7 },
expiration: { maxEntries: 8000, maxAgeSeconds: 60 * 60 * 24 * 30 },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Design concern: 8 000-entry / 30-day cap applies to every PWA user, not just offline-download users

The previous limits (600 entries, 7 days) were reasonable for the "tiles you happen to scroll past" ambient caching use case. The new limits are sized for the "Download Offline Area" download — but they apply to all geolibre-basemaps traffic, meaning every PWA user now gets up to 8 000 tiles and 30-day retention whether or not they ever use the offline download feature.

Two risks:

  1. Self-eviction: A moderate offline area at zoom 15–17 easily exceeds 8 000 tiles. Workbox's LRU eviction will start purging the oldest entries — potentially the tiles that were just downloaded — long before the area is fully cached.
  2. Storage pressure: 8 000 × ~30 KB average ≈ 240 MB of IndexedDB for every PWA install, on top of the CDN-engines cache.

Consider giving the offline-download tiles their own Workbox cache bucket (e.g. geolibre-offline-region) with a higher cap, while keeping the ambient basemap cache smaller. The warmUrls fetch could send a custom header or use a dedicated fetch() path that the SW matches to the offline bucket.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point — leaving this open for @giswqs to decide. Note the cap is a ceiling, not a preallocation: CacheFirst only stores tiles actually fetched, so ambient PWA users who scroll a little still cache only a few tiles (not 240 MB). The self-eviction risk for large downloads is now surfaced by the new offline.tooManyTiles warning. A dedicated geolibre-offline-region Workbox bucket (separate cap) is a reasonable follow-up if you'd like ambient vs offline-download caches sized independently.

@github-actions

github-actions Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Code review

Reviewed the full diff (~1 400 additions): Android CI workflow, Rust #[cfg(desktop)] guards, pointer-events migration, mobile feature-gating, isMobile() / useIsMobileViewport hooks, OfflineRegionDialog, offline-tiles.ts, PWA cache config, and tests.


Bugs

Finding File Confidence
Missing pointercancel handler in both resize callbacks (startLayerPanelResize / startStylePanelResize). An interrupted touch drag (system gesture, incoming call, palm rejection) fires pointercancel instead of pointerup, leaving pointermove on window, body.userSelect = "none", and the cursor override stuck until page reload. DesktopShell.tsx:941,1007 High
handleDownload finally → setPhase("done") even on unexpected throw. collectOfflineUrls is designed not to throw, but if it does (null style, OOM, etc.), the dialog transitions to "done" with 0/0 tiles and no error feedback. OfflineRegionDialog.tsx:137 Medium
Array-sprite format silently ignored. MapLibre GL JS v3+ supports style.sprite as Array<{id,url}>. The guard typeof style.sprite === "string" skips those sprites entirely, so icon assets for multi-sprite styles won't be pre-fetched for offline use. offline-tiles.ts:217 Medium

Performance / Storage

Finding File Confidence
Cache cap 600 → 8 000 entries, 7 → 30 days applies to every PWA user, not only those who use "Download Offline Area". A single modest offline-area download at zoom 17+ can exceed 8 000 tiles and trigger LRU self-eviction of freshly-cached tiles. A separate Workbox bucket for offline-region tiles (distinct from ambient basemap caching) would let you size each independently. vite.config.ts:587 Medium

Quality / CLAUDE.md

Finding File Confidence
Job name says "(debug)" but the build is release. The step comment and tauri android build call both confirm release APKs (~40 MB); the job-level name is misleading for anyone reading Actions runs. .github/workflows/android.yml:28 High
isMobile() called in component render, not memoised. Called on every render in AddDataMenu and ProcessingMenu. Since navigator.userAgent is immutable this is functionally correct but could be a module-level constant or useMemo. Minor. AddDataMenu.tsx, ProcessingMenu.tsx Low
CACHED_TILE_HOST regex is duplicated between OfflineRegionDialog.tsx and vite.config.ts. Divergence after a basemap host change would silently show the wrong warning. A shared constant (or a comment pointing to the other location) would help. OfflineRegionDialog.tsx:469 Low

What I checked and found clean

  • Rust #[cfg(desktop)] guards around POPUP_COUNTER and create_oauth_popup_window are correct for Tauri v2 mobile.
  • tauri.android.conf.json dropping bundle.resources (Python backend) is appropriate.
  • warmUrls worker-pool concurrency model is correct under JS's single-threaded event loop; cursor++ is safe.
  • fetch(url, { cache: "no-cache" }) does not prevent the service worker's CacheFirst handler from caching responses — the cache option targets the browser HTTP cache layer, below the SW.
  • lngLatToTile / tileRangeForBbox / tileToQuadkey math and the test suite are correct (Bing quadkey example verified).
  • useIsMobileViewport hook properly syncs on resize/orientation change and avoids SSR issues.
  • The APK signing script uses set -euo pipefail and validates with apksigner verify; secrets are not echoed.

offline-tiles.ts:
- collectOfflineUrls: accept an AbortSignal (Cancel now interrupts TileJSON
  discovery), thread source `subdomains` into expandTileUrl, and handle the
  MapLibre array `sprite` format so multi-sprite styles cache their icons.
- expandTileUrl: drop `{s}` together with its trailing dot when no subdomains,
  avoiding invalid `https://.host/...` URLs.
- Glyphs: stop encodeURIComponent-ing the fontstack so the warmed URL matches
  MapLibre's actual (browser-normalized) glyph request and the SW cache key hits.
- countTiles/enumerateTiles: split antimeridian-crossing bboxes (west > east).
- warmUrls: clamp concurrency to >=1, and don't increment `done` on aborted
  fetches (finally ran after the catch `return`).

OfflineRegionDialog.tsx: catch collectOfflineUrls errors (no false "done"),
abort the in-flight download when the dialog closes, only clear abortRef if it
still points to this run, disable Download when no service worker is active, and
warn when the area exceeds the SW cache cap.

DesktopShell.tsx: panel resize handles now setPointerCapture, listen for
pointercancel, and tear down on unmount, so a canceled/interrupted drag can't
leave listeners attached or document.body styles stuck.

is-mobile.ts: detect iPadOS 13+ desktop-UA via maxTouchPoints (+ tests).

AddDataMenu/ProcessingMenu: memoize the isMobile() check (stable per session).

android.yml: pin android-actions/setup-android to a commit SHA, rename the job
to "(release)", fail fast when keystore secrets are partially set, and pass
apksigner passwords via files instead of CLI args.
try {
// CacheFirst means already-cached URLs return instantly; new ones hit
// the network and are stored by the SW. We discard the body.
const res = await fetch(url, { signal, cache: "no-cache" });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance – cache: "no-cache" works against the HTTP cache without benefit

The cache: "no-cache" option affects only the browser's HTTP cache, not the Service Worker's Cache Storage. The SW's CacheFirst handler intercepts the request first; when the SW cache hits, the response is returned immediately regardless of this flag. When the SW cache misses, the request is forwarded to the network carrying Cache-Control: no-cache, which tells the tile server to revalidate even for version-pinned, effectively-immutable tile URLs — wasting one RTT per cache miss.

For a large offline pre-fetch where thousands of tiles need to be fetched fresh, this is a meaningful overhead. Dropping the cache: option (or using "default") lets the browser's HTTP cache serve tiles that were recently loaded by the map viewer, while still letting the SW store the response for offline use.

Suggested change
const res = await fetch(url, { signal, cache: "no-cache" });
const res = await fetch(url, { signal });

Confidence: medium — the scenario only matters when the user is downloading a region whose tiles are partly already in the HTTP cache (i.e., recently panned through). The correctness of SW caching is unaffected.

}
if (!templates || templates.length === 0) continue;

const template = templates[0];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quality – only the first tile URL template is used for warming

templates[0] concentrates every warming request to a single endpoint. For sources that list multiple tile URLs (load-balanced CDN shards, e.g. ["https://a.tile.openstreetmap.org/{z}/{x}/{y}.png", "https://b.tile…", "https://c.tile…"]), this can hit one server far harder than normal map use would, potentially triggering per-IP rate limits on large offline regions.

A simple round-robin over all templates would spread the load the same way the map viewer does:

Suggested change
const template = templates[0];
for (const [i, tile] of [...enumerateTiles(bbox, minZoom, maxZoom)].entries()) {
const template = templates[i % templates.length];
urls.add(absolute(expandTileUrl(template, tile, spec.subdomains)));
}

Confidence: low-medium — the default OpenFreeMap/CARTO basemaps likely don't hit per-shard rate limits in practice, but the helper is a general utility and the pattern is fragile for other sources.

* vite.config.ts). Beyond this, Workbox evicts the oldest tiles as new ones
* arrive, so a region larger than this can't be fully retained — warn the user.
*/
const MAX_CACHE_ENTRIES = 8000;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quality – magic constant duplicated from vite.config.ts

MAX_CACHE_ENTRIES = 8000 must equal the Workbox maxEntries: 8000 in vite.config.ts:587. Currently the comment says "kept in sync", but if someone bumps the Workbox limit (or the dialog constant) in isolation, the eviction warning will silently lie to users about when their tiles will be evicted.

Consider re-exporting the value from a shared constants file (or at minimum add a test that reads vite.config.ts and asserts the two values match). Confidence: high.

const verticalResizeGuideRef = useRef<HTMLDivElement>(null);
// Teardown for an in-progress panel resize, so a pointercancel or an unmount
// mid-drag still detaches the global listeners and restores document.body.
const activeResizeCleanupRef = useRef<(() => void) | null>(null);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug (edge case) – single shared cleanup ref for two independent resize handlers

activeResizeCleanupRef is overwritten by whichever resize handler runs last. On a touch device a user can theoretically place two fingers simultaneously on the Layer-panel handle and the Style-panel handle. In that case:

  1. startLayerPanelResize fires → stores its onPointerUp in the ref, adds window listeners.
  2. startStylePanelResize fires → overwrites the ref with its own onPointerUp, adds its own window listeners.
  3. Component unmounts mid-drag → only the Style handler's listeners are removed; the Layer handler's pointermove/pointerup listeners on window are permanently retained (memory leak + stale handler).

A Map<number, () => void> keyed by pointerId would handle N concurrent drags cleanly:

Suggested change
const activeResizeCleanupRef = useRef<(() => void) | null>(null);
const activeResizeCleanupMap = useRef<Map<number, () => void>>(new Map());
useEffect(
() => () => activeResizeCleanupMap.current.forEach((fn) => fn()),
[],
);

Then replace activeResizeCleanupRef.current = onPointerUp with activeResizeCleanupMap.current.set(event.pointerId, onPointerUp) and activeResizeCleanupRef.current = null with activeResizeCleanupMap.current.delete(pointerId) (capture the id in a const at the top of each handler).

Confidence: medium — simultaneous two-panel resize is rare, but the listener leak is real and the fix is low-risk.

echo "signed $signed ($(du -h "$signed" | cut -f1))"
done < <(find apps/geolibre-desktop/src-tauri/gen/android \
-name '*release-unsigned.apk')
rm -f "$store_pass_file" "$key_pass_file"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security – decoded keystore file left on disk after signing

The password temp-files (ks.pass, key.pass) are deleted here, but $RUNNER_TEMP/release.jks written earlier is not. GitHub-hosted runners are ephemeral, so this is low-risk in practice, but defense-in-depth suggests wiping the keystore bytes as well. Consider:

Suggested change
rm -f "$store_pass_file" "$key_pass_file"
rm -f "$store_pass_file" "$key_pass_file" "$RUNNER_TEMP/release.jks"

Confidence: low (ephemeral runners make exploitation unlikely, but trivial to fix).

maxZoom: number,
options: { glyphRanges?: string[]; signal?: AbortSignal } = {},
): Promise<string[]> {
const { glyphRanges = ["0-255", "256-511"], signal } = options;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quality – default glyph ranges cover only basic Latin

The default ["0-255", "256-511"] warms Unicode blocks 0x0000–0x01FF (Basic Latin + Latin Extended). Styles that label in Arabic, Hebrew, CJK, Devanagari, Thai, etc. require higher Unicode blocks (e.g. "1024-1279" for Arabic, "4096-4351" for CJK). Users running "Download Offline Area" on a non-Latin basemap will find place labels absent offline even after a complete tile download.

This is the right default for the common OpenFreeMap/CARTO Latin-label case, but it's worth a note in the JSDoc (or in the dialog UI) that non-Latin basemaps need a broader glyphRanges list passed in. Confidence: high (the limitation is real; whether to address it in this PR is a scope question).

@github-actions

Copy link
Copy Markdown
Contributor

Code review

Reviewed: Android CI workflow, Rust cfg(desktop) gating, offline tile download logic (offline-tiles.ts + OfflineRegionDialog), PWA Workbox cache rules, pointer-event resize migration, mobile feature-gating (isMobile / useIsMobileViewport), and the responsive layout changes. See inline comments for details and suggestions.

Bugs

  • Shared activeResizeCleanupRef across two resize handlers (DesktopShell.tsx:310): if both Layer and Style panel handles receive simultaneous pointer-down events (two-finger touch), the second handler overwrites the first's cleanup; the first's window listeners are never removed on unmount. Confidence: medium.

Security

  • Decoded keystore (release.jks) not deleted after signing (android.yml:159): password temp-files are wiped but the JKS itself is not. Ephemeral runners make this low-risk; the fix is a one-line rm. Confidence: low.

Performance

  • cache: "no-cache" in warmUrls (offline-tiles.ts:330): this flag only affects the browser HTTP cache, not SW Cache Storage. The SW CacheFirst handler intercepts first; the flag then forces tile-server revalidation on cache misses and prevents the browser HTTP cache from serving recently-panned tiles during warming. Confidence: medium.
  • Only templates[0] used for tile URL expansion (offline-tiles.ts:239): all warming requests go to one shard; load-balanced sources could hit per-IP rate limits on large regions. Confidence: low-medium.

Quality

  • MAX_CACHE_ENTRIES = 8000 duplicated from vite.config.ts (OfflineRegionDialog.tsx:44): eviction warning threshold and Workbox cache cap must be kept in sync manually; a shared constant or test assertion would prevent silent drift. Confidence: high.
  • Default glyphRanges covers only Latin U+0000-U+01FF (offline-tiles.ts:211): non-Latin basemaps (Arabic, CJK, Devanagari) will have missing labels offline even after a complete tile pre-fetch. Confidence: high (known limitation).

CLAUDE.md: nothing to flag — Rust changes are correctly desktop-gated, new i18n strings use t() with en.json entries, no direct commits to main.


Overall: The core logic (tile enumeration, antimeridian splitting, quadkey, SW warming loop, abort plumbing, pointer-event migration, Rust mobile gating) is solid and well-tested. All six findings are non-blocking; the most actionable are the activeResizeCleanupRef multi-touch edge case and the MAX_CACHE_ENTRIES duplication.

@giswqs
giswqs merged commit 04f581c into main Jun 15, 2026
118 checks passed
@giswqs
giswqs deleted the spike/tauri-android branch June 15, 2026 05:06
mlanini added a commit to mlanini/milgeo-app that referenced this pull request Jun 16, 2026
- Upstream: TopToolbar god-component refactored into menus/hooks/toolbar/
- Upstream: AddDataDialog split into per-source components (add-data/)
- Upstream: geocoding multi-provider + batch/reverse (opengeos#346, opengeos#318, opengeos#363)
- Upstream: real-time collaboration MVP (opengeos#307)
- Upstream: Python console + automation API (opengeos#327)
- Upstream: storymap builder (opengeos#299), print layout (opengeos#303)
- Upstream: network analysis (isochrones, OD matrices) (opengeos#308)
- Upstream: spatial statistics toolbox (opengeos#342)
- Upstream: AI segmentation (SamGeo/SAM3) (opengeos#301)
- Upstream: GIS assistant (Strands agent) (opengeos#331)
- Upstream: client-side vector tiling (opengeos#356)
- Upstream: layer groups/folders (opengeos#311)
- Upstream: shapefile/GeoPackage export (opengeos#310)
- Upstream: raster pseudocolor + band combination (opengeos#319)
- Upstream: attribute table virtualization + statistics (opengeos#338, opengeos#315)
- Upstream: i18n framework (opengeos#273), a11y pass (opengeos#279)
- Upstream: undo/redo for layers (opengeos#269), command palette (opengeos#278)
- Upstream: Android PWA + offline caching (opengeos#377)
- Upstream: CDN CereusDB wasm, PGlite CDN loader

MilGeo customizations preserved:
- MilSymb/Analysis/Sillages toolbar buttons + aside panels
- milsymbolPlugin in vite.config.ts (APP-6D dev server middleware)
- MilSymbolRenderer in DesktopShell
- PrivacyNoticeDialog (startup gate)
- ui.analysisOpen / ui.sillagesOpen in store
- MGRS status bar display
- Switzerland default map center (zoom 3)
- mil-symbol / mil-graphic LayerTypes
- MilGeo rebrand (title, favicon, URLs)
- Traccar dev-proxy in vite.config.ts
- openTopographyApiKey / demSource / localDtmPath settings
- milsymbol@^3.0.4 dependency
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant