fix: use country-level resolution for low-precision geohashes#1044
fix: use country-level resolution for low-precision geohashes#1044malkovitc wants to merge 5 commits intopermissionlesstech:mainfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dedcd5ed68
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let country = pm.country, !country.isEmpty, !seenAdmins.contains(country) { | ||
| seenAdmins.insert(country) | ||
| uniqueAdmins.append(country) | ||
| } else if let admin = pm.administrativeArea, !admin.isEmpty, !seenAdmins.contains(admin) { |
There was a problem hiding this comment.
In resolveCompositeAdminName, the new else if still falls back to administrativeArea when pm.country exists but was already seen, so multi-point sampling inside the same country can produce mixed labels like "United Kingdom and Scotland" instead of staying country-level. Because finalName uses only the first two collected values, this can also crowd out a true second country in border-spanning cells. For low-precision geohashes, admin fallback should only run when country is missing, not when it is duplicate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Fixed in 6d64a6d — admin fallback now only triggers when pm.country is nil/empty, not when it's a duplicate. This prevents mixed labels like "United Kingdom and Scotland".
Prevents mixed labels like "United Kingdom and Scotland" for single-country geohashes. Admin fallback now only triggers when no country is available from the placemark.
|
I think there is still one gap here for existing users: the naming logic changes look right, but previously cached bookmark names will not be recomputed. |
…c applies Users who bookmarked a <=2-char geohash before the country-first resolver kept the old administrativeArea cache (e.g. "England" for `gc`) because resolveBookmarkNameIfNeeded bails when bookmarkNames[gh] is non-nil. Add a one-shot, versioned migration that drops cached names for <=2-char geohashes on load; the next LocationChannelsSheet .onAppear re-resolves them via the fixed logic. Higher-precision entries are untouched.
|
Good point — fixed in 4865471. Added a one-shot versioned migration in loadPersistedState that drops cached names for ≤2-char geohashes; the next LocationChannelsSheet .onAppear re-resolves them via the new country-first path. Higher-precision entries are left alone. Covered by a test. |
2 participants
Summary
Fixes #887 — When bookmarking a 2-character geohash channel (e.g., "gc" for United Kingdom), the resolved location name showed "England" instead of "United Kingdom".
Root cause:
resolveCompositeAdminName()andnameForGeohashLength()inLocationStateManager.swiftprioritizedadministrativeArea(province/state) overcountryfor low-precision geohashes (≤2 characters). A 2-char geohash covers a region/country, so the country name should take precedence.Changes
resolveCompositeAdminName()— swappedif/elsepriority: checkpm.countryfirst, fall back topm.administrativeAreanameForGeohashLength()— changedcase 0...2frompm.administrativeArea ?? pm.countrytopm.country ?? pm.administrativeAreaHow it works now
gc(UK)u3(France)Higher-precision geohashes (3+ chars) are unaffected — they continue to resolve to province/city/neighborhood as appropriate.
Test plan