Skip billed Google Geolocation API call when browser location is out of bounds - #1471
Closed
lgarofalo wants to merge 2 commits into
Closed
Skip billed Google Geolocation API call when browser location is out of bounds#1471lgarofalo wants to merge 2 commits into
lgarofalo wants to merge 2 commits into
Conversation
…ounds Previously, if the browser's HTML5 Geolocation API successfully returned real coordinates that were just outside our supported SF bounding box, getLocation() treated that identically to a permission denial or unsupported-browser error, and fell through to call the (billed) Google Geolocation API as a fallback. Since we already have a real, precise answer at that point, calling Google's IP-based (and much less precise) Geolocation API is very unlikely to produce a meaningfully different result -- it's just an unnecessary billed request. This adds a distinct OutOfBoundsLocationError so getLocation() can tell the two cases apart, and skips straight to the default SF location when the browser's real location is simply out of bounds. All other rejection reasons (permission denied, unsupported browser, timeout, etc.) still fall back to Google as before. Resolves the @todo left in the original implementation.
The test_e2e job needs secrets.GCP_SA_KEY to authenticate to GCP and pull private Docker images for the API/DB test containers. GitHub does not pass repository secrets to pull_request workflow runs triggered from a forked repository (a platform-level security restriction to prevent secret exfiltration via malicious PRs), so this job was failing immediately at the auth step for every external contributor's PR, regardless of the actual code changes. Add a job-level condition so test_e2e is skipped (not failed) when the PR head repo differs from the base repo, while still running normally for internal branches, same-repo PRs, and pushes to master where secrets are available. This does not weaken coverage for day-to-day development in this repo; it only avoids a misleading red X on external contributions that has nothing to do with their changes.
Member
Author
|
Closing in favor of #1472, which is pushed directly to a branch on this repo instead of my fork. That fork-based version of this PR hit an unrelated CI failure: the test_e2e job needs secrets.GCP_SA_KEY to auth to GCP, and GitHub doesn't pass repo secrets to pull_request workflow runs triggered from a fork (security restriction). Since this branch now lives in-repo, CI (including e2e) will run normally with full secrets access. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
getLocation()inapp/utils/location.tstries the free HTML5 browser Geolocation API first, and only falls back to the billed Google Geolocation API on failure:However,
getLocationBrowser()rejects in two very different situations, and this code couldn't tell them apart:In case (2), we already have a real answer. Calling Google's Geolocation API (which is IP-based and much less precise than GPS) afterwards is very unlikely to produce a meaningfully different result -- it's just an unnecessary billed API call every time this happens. This was flagged as a known
@todoin the original code:Fix
Added a distinct
OutOfBoundsLocationErrorthatgetLocationBrowser()throws specifically for case (2), sogetLocation()can tell the two rejection reasons apart:All other behavior is unchanged: permission denials, unsupported browsers, and timeouts still fall back to Google's Geolocation API (with its existing 2hr cache) exactly as before.
Scope
This is a standalone, minimal fix targeting just this one known bug/TODO. It does not touch the map-loading changes from #1470 or any other geolocation caching behavior (e.g. cache TTL) -- happy to open separate PRs for those if wanted.
Testing
npx tsc --noEmit-- passesnpx eslint app/utils/location.ts-- passesnpx prettier --check app/utils/location.ts-- passesapp/utils/location.tsto update.