Skip to content

fix(geolocate): guard permission query against synchronous throws#861

Merged
giswqs merged 1 commit into
mainfrom
fix/issue-839-geolocate-sync-throw
Jun 25, 2026
Merged

fix(geolocate): guard permission query against synchronous throws#861
giswqs merged 1 commit into
mainfrom
fix/issue-839-geolocate-sync-throw

Conversation

@giswqs

@giswqs giswqs commented Jun 25, 2026

Copy link
Copy Markdown
Member

Summary

Follow-up to #858 (merged). The final round of automated review on that PR landed two items after it was already merged, so this small PR carries them forward.

Changes

  • Guard permissions.query() against a synchronous throw. In handleGeolocateError, .catch() only intercepts async rejections. Some Permissions API implementations can throw synchronously (partial support, CSP restrictions, Firefox private browsing), which would escape the handler as an unhandled error. Wrap the query in try/catch and fall back to a deferred reset via queueMicrotask — consistent with the existing no-Permissions-API path, and it avoids tearing down the control mid error-dispatch.
  • Tests. Add coverage for the rejected-promise (.catch) branch and the synchronous-throw branch of handleGeolocateError.

Verification

  • tests/map-controller.test.ts — 35 pass (9 in the geolocate recovery suite).
  • tsc -b clean.

Relates to #839.

- Guard permissions.query() against a synchronous throw (partial API,
  CSP, private browsing) by wrapping it in try/catch and deferring a
  reset via queueMicrotask, matching the no-API fallback. .catch() alone
  only handles async rejections.
- Add tests for the rejected-promise (.catch) and synchronous-throw
  branches of handleGeolocateError.
@netlify

netlify Bot commented Jun 25, 2026

Copy link
Copy Markdown

Deploy Preview for geolibre-app ready!

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

QR Code

Use your smartphone camera to open QR code link.

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

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@giswqs, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 22 minutes and 34 seconds. Learn how PR review limits work.

To continue reviewing without waiting, enable usage-based billing in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 092f419e-0aba-4511-abd1-2771dd02856f

📥 Commits

Reviewing files that changed from the base of the PR and between f4ffe04 and 38a892e.

📒 Files selected for processing (2)
  • packages/map/src/map-controller.ts
  • tests/map-controller.test.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/issue-839-geolocate-sync-throw

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@github-actions

Copy link
Copy Markdown
Contributor

⚡ Cloudflare Pages preview

Item Value
Preview https://fd585b5c.geolibre-preview.pages.dev
Demo app https://fd585b5c.geolibre-preview.pages.dev/demo/
Commit 0f46c60

Comment thread tests/map-controller.test.ts
Comment thread packages/map/src/map-controller.ts
@github-actions

Copy link
Copy Markdown
Contributor

Code review

Overall the implementation is correct, the edge case is real, and the two new tests pin both branches. No bugs, no security issues, no performance concerns. Three minor observations below.


Bugs

None found.

Security

None found.

Performance

None found.

Quality

Finding Confidence
queryOverride type (tests/map-controller.test.ts:674) — typed as () => Promise<unknown> but the sync-throw test passes a function that throws instead of returning. TypeScript accepts it (throwing satisfies any return type via never), so no compile error. () => PromiseLike<unknown> would be marginally more accurate and better documents what the production code actually needs (a thenable). Posted as an inline comment. Low (nit)
flush JSDoc is stale (tests/map-controller.test.ts around line 702) — says "the async permission query and its .then" but the two new tests exercise paths where queueMicrotask is used instead of the promise chain. setTimeout(resolve, 0) flushes both correctly; the description just no longer covers all the cases. Couldn't attach inline (line is outside the diff). Low (nit)
Implicit coverage of non-thenable returns (packages/map/src/map-controller.ts:1720–1729) — wrapping the whole .query(...).then(...).catch(...) chain in try/catch means a broken Permissions API implementation that returns undefined (or any non-thenable) will also be caught, since calling .then() on it throws synchronously. This is correct and a nice defensive side-effect, but it's untested. No action needed; just worth knowing. Posted as an inline observation. Informational

CLAUDE.md

No CLAUDE.md guidelines apply to these files specifically (no new UI strings, no MapLibre control styling changes, no new external hosts).

@giswqs giswqs merged commit 16fe432 into main Jun 25, 2026
19 checks passed
@giswqs giswqs deleted the fix/issue-839-geolocate-sync-throw branch June 25, 2026 03:00
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