Repository navigation
Load the Maps API over https rather than protocol-relative - #8
Conversation
Snowboard's Url utility does not recognise protocol-relative URLs: both to() and asset() test the URL against a regex requiring an explicit scheme and, when it does not match, strip every leading slash before resolving the remainder against the site. So the Maps API ends up being requested from https://<site>/maps.googleapis.com/maps/api/js, which 404s. A normal page render is unaffected, since AssetMaker passes protocol- relative URLs through untouched. It only breaks once the asset is injected over AJAX and goes through AssetLoader.loadScript(), which is every AJAX request made on a backend form carrying an AddressFinder. The result is worse than a missing script: loadScript() rejects its promise when the script errors and handleUpdateResponse() awaits it, so the handler never completes and the interface waits indefinitely with nothing but a 404 in the console to explain it. Requesting the scheme explicitly sidesteps this, and costs nothing: the Maps API only serves https regardless. The underlying Url behaviour is reported separately as wintercms/winter#1538. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018icgbPJpBGE4wCuDGxsKjE
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe Google Maps API asset URL in Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to AddressFinder now loads the Google Maps API via HTTPS during AJAX rendering, preventing the same-origin asset rewrite that blocked autocomplete and form handling. The focused change is ready to merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 PHPStan (2.2.8)Composer install failed: dependency resolution error. Check composer.json and composer.lock for version constraints. 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. Comment |
Summary
AddressFinder::loadAssets()registers the Google Maps API with a protocol-relative URL:Snowboard's
Urlutility doesn't recognise that form. Bothto()andasset()test for an explicit scheme and, when the URL doesn't match, strip every leading slash before resolving what's left against the site:So the browser requests
https://<site>/maps.googleapis.com/maps/api/js?…and gets a 404.Why it isn't noticed on a page load
AssetMaker::getAssetPath()andgetAssetScheme()both pass protocol-relative URLs through untouched, so a full render emits a working<script src="//maps.googleapis.com/…">. The break only happens when the same asset arrives viaX_WINTER_ASSETSand is loaded byAssetLoader.loadScript()— i.e. on any AJAX request made from a backend form containing an AddressFinder.The symptom is worse than a missing script.
loadScript()rejects its promise when the script errors, andhandleUpdateResponse()awaits it, so the AJAX handler never completes. I ran into this through LukeTowers.EasyAudit, where opening an audit log entry left the popup spinning forever with nothing but the 404 in the console to go on.The change
Requests the scheme explicitly. It costs nothing — the Maps API only serves over https anyway — and it's the smaller of the two fixes.
The underlying
Urlbehaviour is reported separately as wintercms/winter#1538, since a protocol-relative URL is valid and shouldn't be silently rewritten into a same-origin path. That fix belongs in core; this one unblocks the plugin in the meantime and remains correct either way.Version bumped to 2.2.2 (patch — a bug fix, no API or behaviour change).
Testing
Verified against a Winter 1.2.12 backend form containing an AddressFinder: before the change the AJAX request for the widget's assets resolved to
https://<site>/maps.googleapis.com/…and 404'd, leaving the handler's response unapplied; after it, the asset loads frommaps.googleapis.comand the handler completes. Confirmed the widget is the only protocol-relative asset in the plugin.🤖 Generated with Claude Code
https://claude.ai/code/session_018icgbPJpBGE4wCuDGxsKjE
Summary by CodeRabbit
Bug Fixes
Documentation