ADFA-2436: Add Maps (offline OSM) plugin - #22
Conversation
…ile writes (+ unit tests)
…t tests) Hand-rolls what go-pmtiles' `pmtiles extract` does (bbox → minimal v3 archive), over HTTP range requests against the IIAB-hosted global archive, because we can't ship/shell-out to a per-arch Go binary on-device. The code is dense (Hilbert-curve tile-id ranges, directory walking, run-length entries) but isolated behind a small surface and heavily unit-tested — trust the tests. If it proves hard to maintain, it's swappable for the `pmtiles` Go CLI. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…s), installer, stores (+ unit tests)
…pp (Kotlin + Java) (+ unit tests)
From the license-audit + code-review DoD pass (both 0-blocker): - THIRD_PARTY_LICENSES.txt: add com.github.davidmoten:hilbert-curve:0.2.3 and its transitives (guava-mini, listenablefuture) — Apache-2.0 runtime deps bundled in the .cgp that were the one un-attributed entry. - SourcePickerFragment.persistLanHost: use applicationContext (matching the off-main read) so the write shares the one process-wide SharedPreferencesImpl — a future reorder can't reintroduce a first-access disk read on Main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jatezzz
left a comment
jatezzz
left a comment
There was a problem hiding this comment.
Some of these files are quite long. Please consider splitting them into smaller, more focused files to improve readability and adhere to the Single Responsibility Principle (SRP).
| @@ -0,0 +1,93 @@ | |||
| package org.appdevforall.maps.data | |||
There was a problem hiding this comment.
Please ensure file operations runs on Dispatchers.IO.
There was a problem hiding this comment.
Thanks...no need to identify more of these examples. I'll go through and review all file operations.
There was a problem hiding this comment.
Done — and I swept the whole data layer rather than just the three files you flagged. Concretely:
data/ went from 4 suspend functions to 23 — 19 file-I/O entry points that previously ran on the caller's thread now run on Dispatchers.IO. By class:
| Class | I/O entry points moved to Dispatchers.IO |
|---|---|
RegionCache |
5 |
ActiveRegionStore (you flagged) |
3 — read, write, clear |
FirstRegionAutoActivator (you flagged) |
1 |
MapFontExtractor |
1 |
RegionInstaller |
1 (a second one was already correct) |
ProjectRegionCoordinator, ReachabilityProbe |
1 each (new — extracted out of the Fragments) |
And the fragment refactor (see other reply) also moved all I/O out to the data layer.
| @@ -0,0 +1,95 @@ | |||
| package org.appdevforall.maps.data | |||
There was a problem hiding this comment.
Please ensure file operations runs on Dispatchers.IO.
| @@ -0,0 +1,280 @@ | |||
| package org.appdevforall.maps.data | |||
There was a problem hiding this comment.
Please ensure file operations runs on Dispatchers.IO.
@jatezzz thanks for reviewing. I already worked with Claude to do a couple of cohesion and coupling passes. Broke up many things and made them more testable. Looks like I missed one file...BboxFragmentPicker needs help. And I'll try to break up the RegionManagerFragment too...that popped up as a borderline judgement call last time. There's probably a better way to organize the wizard steps. Anything else you spotted? Also is there a guideline for how this team applies SRP? It's one of the more vague rules and I'm curious how you and the team apply it. The more specific we can be, the more likely it is that agents can plan for and build the right thing on the first try. |
|
@fryanpan Thanks for the open response and for doing those passes with Claude! Regarding the files: You make a fantastic point about SRP being vague, and I completely agree that being specific helps both humans and AI agents. Here is how we generally look at SRP pragmatically on this team:
I love your point about agents planning better with specific rules. We should probably document these exact heuristics in a guide file so the AI tools can pick them up automatically as context. Let me know how the refactoring goes with those two fragments! |
…s-plugin # Conflicts: # README.md
…(ADFA-2436) Responds to the review on PR #22. Dispatchers.IO (3 inline comments). Every entry point in data/ is now `suspend` + `withContext(Dispatchers.IO)` — ActiveRegionStore, RegionCache and FirstRegionAutoActivator (the three called out), plus RegionDownloader, RegionInstaller, ProjectRegionCoordinator, MapFontExtractor, RegionSizeEstimator and ReachabilityProbe. The dispatch is pushed down into the data layer rather than wrapped at the Fragment call sites, so a caller can't do file I/O on Main. SRP / long files. Business logic pulled out of the Fragments: - domain/ (pure Kotlin, JVM-unit-tested): AutoShrinkBbox, ZoomFit, ZoomLabel, EstimateDisplay, IiabStyleBuilder, RegionWizardStateMachine - data/: RegionSizeEstimator, ProjectRegionCoordinator, ReachabilityProbe - ui/: BboxSelectionModel, MapLocationController, BboxPickerArgs - slicer/: TileMath extracted from PmtilesRegionSlicer The download wizard is now per-step Fragments (SourcePicker, DownloadProgress, Step3Save) orchestrated by RegionWizardStateMachine, per the reviewer's suggestion. Line counts, code only (comments + blanks stripped): BboxPickerFragment 1320 -> 762 raw / 446 code RegionManagerFragment 647 -> 542 raw / 401 code PmtilesRegionSlicer 643 -> 566 raw / 349 code Also in this pass: dark-mode correctness, and dead branding tokens stripped from PluginTheme (plugins take the host's Material colors — they can't re-theme host-loaded Material widgets through plugin theme attrs).
Re: SRPTried to address SRP comments by splitting up UI vs. logic and break out responsibilities more sensibly:
Re: DocumentationWill try to get more about SRP guidelines into a draft PR on the plugin-examples side, and also CoGo side. |
How this was testedAutomated
On device — Samsung A56 (Android 16), plugin installed from a clean state, both template languages:
Recordings
|
There was a problem hiding this comment.
Code Review
Overview
A new headless-plus-UI plugin that adds a Maps bottom-sheet tab and an Offline OSM Map project template. The interesting engineering is range-slicing a bbox out of IIAB's ~87 GB global PMTiles archive rather than downloading it, then bundling the slice into a generated MapLibre app that serves its own tiles over a loopback HTTP server. Region cache is shared across projects at /sdcard/CodeOnTheGo/maps/<id>/.
This is high-quality work. The comments explain why consistently (the native.code permission, the pmtiles://http:// scheme constraint, the ConnectivityReceiver.setConnected(true) hack, Pebble's newline trimming) — that's the kind of documentation that saves the next person a device round-trip. Day/night theming follows the reference pattern exactly (values-night/, themedPluginInflater + onGetLayoutInflater on all five fragments, zero hardcoded hex, zero hardcoded layout strings). Path-traversal guards are layered (regex validation plus canonical containment) in every path that resolves under the cache root.
Blocking
1. Tooltip category is wrong → tab tooltip will render n/a
maps/src/main/kotlin/org/appdevforall/maps/MapsPlugin.kt:189
override fun getTooltipCategory(): String = "plugin_maps"The category must be "plugin_" + the full plugin.id, i.e. "plugin_org.appdevforall.maps". The host registers entries under the returned string but derives plugin_<pluginId> when resolving a lookup, so a short slug silently mismatches and long-press shows the literal string n/a. The build stays green and the DB row is present — only device long-press catches it.
override fun getTooltipCategory(): String = "plugin_$PLUGIN_ID"MapsPluginCoverageTest.kt:188 asserts the wrong value too, so it needs updating with the fix. Worth noting: several existing plugins in this repo (bookshelf, random-xkcd, keystore-generator) have the same short-slug shape, while compose-preview, sketch-to-ui-plugin, and apk-viewer derive from the id. If device testing showed the Maps tooltip working, then the host is more lenient than documented — say so and I'll drop this — but the documented contract and the majority-correct plugins both point at deriving from PLUGIN_ID, which is free to do either way.
2. Template tooltip tag has no registered entry
MapTemplateBuilder.kt:63,146 attach tooltipTag = "maps.template.region_map" to the template, but getTooltipEntries() (MapsPlugin.kt:191) returns exactly one entry, tagged maps.bottom_sheet.maps_tab. The New Project card's tooltip has nothing to resolve → n/a. Add a second PluginTooltipEntry for the template tag.
3. Hard-coded upstream archive date breaks every download on rotation
RegionDownloader.kt:54,327
private const val FALLBACK_VECTOR_DATE = "2026-04-01"
...
"openstreetmap-openmaptiles.$FALLBACK_VECTOR_DATE.z00-z14.pmtiles"The constant is named FALLBACK_ but it's the only path — there is no discovery. HttpRangeByteCache.clear()'s own comment says "IIAB rotates the same URL weekly"; when that rotation drops the 2026-04-01 file, every download 404s with no recovery and the plugin looks broken in the field.
The fix is already half-built: ReachabilityProbe.buildLanProbeUrl (ReachabilityProbe.kt:47) HEAD-probes /maps/extracts.json, "the canonical IIAB inventory". Read that inventory to resolve the current filename and keep the constant as a genuine fallback.
Should fix
4. Play Services dependency contradicts the template and can crash on GMS-free devices
build.gradle.kts:92 pulls play-services-location:21.3.0, and MapLocationController.kt:165 calls LocationServices.getFusedLocationProviderClient(ctx) — the one unguarded call in a file that runCatchings everything else. On a de-Googled or AOSP device (a meaningful slice of ADFA's low-end target audience), client construction can throw before either listener is attached, and it propagates out of fetchLastKnownLocation().
Meanwhile the generated app deliberately avoids GMS — app/build.gradle.kts.peb says "The map's current-location dot uses MapLibre's built-in default location engine, so no Play Services Location dependency is needed", and MapRegionActivity proves it works. The plugin's own enableLocationComponent already uses useDefaultLocationEngine(true). Drop the dependency and use the existing fallbackToLocationManager() walk as the primary path — smaller .cgp, consistent with the template, and no GMS assumption. At minimum, wrap the client creation in runCatching { … } ?: fallbackToLocationManager().
5. PmtilesHttpServer: partial skip serves wrong bytes under a correct-looking header
templates/region-map/app/src/main/java/PACKAGE_NAME/PmtilesHttpServer.kt.peb:131-137
while (skipped < rangeStart) {
val sk = fis.skip(rangeStart - skipped)
if (sk <= 0) break // <-- then serves from the wrong offset
skipped += sk
}InputStream.skip returning 0 does not imply EOF. On a short skip the loop breaks and the body is streamed from wherever the cursor landed, while the already-written Content-Range header promises rangeStart-rangeEnd. MapLibre gets structurally valid but wrong PMTiles bytes — silent tile corruption that is very hard to trace. Fail the request instead (500, or 416) rather than serving mismatched data.
Two smaller items in the same file:
- No
setSoTimeouton the accepted socket, feeding an unboundednewCachedThreadPool. A hung client pins a thread indefinitely. Loopback keeps the risk low, but this is template code students will copy. - Suffix ranges are silently ignored:
if (dash > 0)(line 95) skipsbytes=-500, which then serves the whole file under a200. MapLibre doesn't emit suffix ranges, so this is latent, not live.
Worth an explicit note in osm-tutorial.html: this server is unauthenticated and reachable by any app on the device. It's harmless here (public map tiles already world-readable inside the APK), but the pattern is teaching material and a student who points rootPrefix at something sensitive has built a local data leak. One sentence in the tutorial and a comment on the class would cover it.
6. The 206 branch lacks the size cap the 200 branch has
RangeFetcher.kt:146-154. The 200 fallback carefully refuses unknown-length or oversized bodies before buffering (lines 162-168) — good. The 206 branch calls body.bytes() unconditionally and only checks the size in a require after the whole body is in memory. A misbehaving or hostile LAN IIAB host answering 206 with a multi-GB body OOMs the process. Check body.contentLength() against length before reading, mirroring the 200 path.
Also line 149: bytes.size == length || bytes.size == (end - offset + 1).toInt() — end is defined as offset + length - 1, so the second disjunct is always identical to the first. Dead condition.
7. Rebase before merge
13 commits behind main. Both README.md (the Examples table) and .github/workflows/update-libs.yml (the MAP array) insert into regions that main has since changed, so expect conflicts in exactly those two spots. Trivial to resolve, but better done by the author than at merge time.
Polish
- Bundled Gradle wrapper.
maps/gradlew,maps/gradlew.bat,maps/gradle/wrapper/duplicate the repo-root wrapper. Repo convention is to use the root one (cd maps && ../gradlew assemblePlugin), asflutter-templatedoes. The../libs/*.jarreferences are correct — just the wrapper to delete. .cgpsize. ~24 MB of assets before the plugin's own MapLibre.so. Two concrete trims: the vendored repo ships threekotlin-stdlibversions (1.7.10, 1.8.22, 2.2.10) where resolution needs one — ~3 MB; and the template pinsabiFilters.add("arm64-v8a")with a clear rationale, but the plugin's ownbuild.gradle.ktsdoesn't, so the.cgpcarries every ABI MapLibre's AAR publishes. Applying the same filter to the plugin is consistent and free.- arm64-only generated app. Worth stating in the tutorial and template README — a student who reaches for an x86_64 emulator instead of a device gets a missing-
.sofailure, and the reason lives only in a Gradle comment. - Hardcoded English strings.
MapLocationController.kt:126-144—setTitle("Location is off for Code on the Go"),"Open Settings","Not now", and the long message. The siblingmaybeRequestLocation()(lines 107-117) correctly usesR.string.maps_location_perm_*. These are user-facing UI, so they belong instrings.xmlalongside the rest; it's the only such offender in the plugin. Bboxhas noequals/hashCode/toString. It's a plainclass(Bbox.kt:17).RegionDownloader.kt:123logsbbox=$bbox, which printsBbox@1a2b3c4— the download log line loses its most useful field. Note the contrast withRegionInfo, where the hand-rolledequals/hashCodefor theDoubleArrayfield is done carefully and correctly. MakingBboxadata classfixes the log and forecloses any future reference-equality surprise.- Delete keys off the meta-declared id.
RegionManagerFragment.kt:505callsRegionCache.delete(info.regionId), whereregionIdcomes frommeta.optString("regionId", dir.name)(RegionCache.kt:203)./sdcardis world-writable, so a plantedmeta.jsoncan name a different valid region and delete the wrong directory. Not traversal — the containment check holds — butinfo.directoryis already canonicalized and unambiguous, so passing that instead removes the class of bug. HttpRangeByteCache.gethands back the internal array.RangeFetcher.kt:73returns the cachedByteArrayby reference from a process-global cache, though the KDoc calls them "instant in-memory copies". No current caller mutates it, but a.copyOf()(or a comment) would make that safe by construction rather than by convention. Separately,put'scontainsKeyearly-return (line 80) doesn't promote in an access-order map, so re-requesting a cached range doesn't refresh its LRU position.compileSdk/targetSdk = 34. #54 is standardizing the toolchain across plugins — coordinate so this doesn't land needing an immediate follow-up.mkdirs()precedes the containment check.ProjectMapEmitter.kt:90-95creates the directory then verifies it doesn't escape.MAPS_ASSETS_SUBPATHis a constant so it can't, but ordering the check first makes the guard real rather than decorative.- Doc/code mismatch.
Bbox.heightKm()(line 35) is documented as "latitude span × 111.32"; the body callshaversineKm. - Cleartext to LAN hosts.
RegionDownloader.base()defaults tohttp://for LAN (correct — IIAB boxes rarely run TLS), but the plugin declares nonetworkSecurityConfigand rides the host's. Worth confirming CoGo permits cleartext to arbitrary LAN hosts, or LAN downloads will fail on a host that doesn't. The template correctly scopes its own config to127.0.0.1.
Testing
359 tests passing, 98.8% line / 89.3% branch on non-UI logic. The injectable-seam design (downloadInto's copyBasemap/sliceTiles/nowMillis, ProjectMapEmitter's Android-free purity, RegionCache's *FromRoot overloads) is the right way to get this testable without Robolectric, and it's applied consistently. UI at 0% with device E2E is a reasonable call and honestly labeled.
Gaps worth a follow-up: the PMTiles slicer is 4k lines of binary-format parsing whose failure mode is silently wrong tiles rather than a crash, and the tests exercise it against synthetic archives. A golden-file test — slice a known bbox from a checked-in small PMTiles and assert the output byte-for-byte against a pmtiles CLI extract — would be the cheapest guard against a regression here, and would also de-risk the "rip this out for the CLI later" option the PR description raises. I'd also add a regression test for #5 (a short-skip Range request) since the symptom is corruption rather than an exception.
Verdict
Approve after #1–#3. Those are all small, mechanical fixes, and #1/#2 are the class of defect that only device long-press catches — worth doing before this ships. #4–#6 are the ones I'd want addressed in this PR rather than deferred; the rest can be follow-ups.
I have not built or device-tested this — those findings come from reading the diff, so #1 in particular is worth a quick long-press check on the Maps tab to confirm.
🤖 Generated with Claude Code
3 participants
ADFA-2436: Add Maps (offline OSM) plugin
Ticket
ADFA-2436
Description
Adds a Maps plugin that helps developers make a working offline mapping app in a few minutes.
Lets the developer download an OpenStreetMap (OSM) map region from an Internet in A Box (IIAB) server and build a read-only mapping app (annotation left for a future ticket)
Workflow
You need internet for steps 1 and 3, but the developer can start working offline after that. And the generated app also works offline.
Details by Commit
I asked Claude to reorganize the work into commits, to try and make a big PR easier to review. Each file is in exactly one commit. Some commits could benefit from more review than others.
In the "Files Changed" tab above, you can review by commit to get a more organized view:
5049a687981fa8min_ide_versionpin with no max),MapsPlugin. Mostly boilerplate.c044a6a44ce879c30e5cab3b53f29c5acb487d274636c9985THIRD_PARTY_LICENSES. Check attribution + licensing is reasonable, not code.Testing
Automated checks below in the table, plus I've manually tested the flow + reviewed the code.
assemblePlugin+test)P.S. The automated checks are a bit messy and defined in the wrapper repo. I'll make a future ticket to look into cleaning them up and adding them into the plugin repo. Let me know if there are parts of this you think are especially important (or not important!)
Unit Tests
Didn't spend too much effort trying to get to 100% coverage. Did try to get over 90% on most non-UI code....but it was a bit hard to test some of the areas that required Android integration or mocking out CodeOnTheGo integration. So I left it to the E2E tests to cover these areas.
./gradlew jacocoTestReportCoverage by folder (full breakdown + the 8-check quality assessment in
coverage-quality-assessment-2026-06-04.md):domain/slicer/util/data/templates/maps/(MapsPlugin)ui/On-Device Automated E2E QA Walk
Was using this method regularly during development to have Claude do end-to-end testing using mobile-mcp.
Link to the test cases here which cover the whole flow from installing the plugin, using the project template, downloading and managing map regions, building the templated app offline, running the templated app, and testing the offline map in the templated map. The tests also went through both Kotlin and Java project templates.
Device recordings from running the tests:
StrictMode
Was getting a few warnings about this a few days ago (e.g. warnings/errors during project creation)...did a cleanup pass. The plugin should be StrictMode clean now -- no main-thread I/O. Sockets are traffic-stats-tagged.
But creating a project still triggers StrictMode warnings that are coming from the CodeOnTheGo side. Added a comment to ADFA-4121.
Notes & Questions for Reviewers
Ease of Testing + CoverageCouple of followup items would help in the future. I'll add some followup tickets:
Decisions to Note
Future Improvements — Two nice-to-haves tracked but not blocking v1