Harden saved-list place links and canonical coordinates#51
Conversation
📝 WalkthroughWalkthroughThe PR adds direct CID-based Maps URLs for addressless saved-list records and changes place-coordinate extraction to prioritize structured coordinates from canonical resolved URLs over preview or viewport data. ChangesExtraction strategy updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DebugScraper
participant SourceMerger
participant ResolvedPlaceURL
participant PlaceDetails
participant Diagnostics
DebugScraper->>SourceMerger: merge DOM and preview sources
SourceMerger->>ResolvedPlaceURL: provide resolved_url
ResolvedPlaceURL-->>SourceMerger: return canonical !3d/!4d coordinates
SourceMerger->>PlaceDetails: build details from adjusted snapshot
PlaceDetails->>Diagnostics: record coordinate field sources
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/gmaps_scraper/place_scraper.py (1)
4287-4296: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winFallback coordinate extraction bypasses the range validation applied to the canonical path.
_extract_canonical_place_coordinatesrejects out-of-range!3d/!4dpairs via_valid_coordinates(Line 4315-4316), causing_build_place_detailsto fall into theelsebranch (Lines 2052-2058). That branch calls_extract_coordinate_from_url, which now also matches the same!3d(...)!4d(...)pattern (Line 4288) but performs no range check before returning the value. An out-of-range/malformed canonical coordinate is thus silently re-admitted through the fallback path instead of being rejected end-to-end.The
!3d/!4dregex is also now duplicated verbatim between the two functions, increasing the chance the checks drift apart further.As per coding guidelines,
**/*.pyshould "preserve diagnostics and quality flags for partial or uncertain extraction," but this bypass admits unvalidated coordinates without any quality flag.🛡️ Proposed fix to share the regex and validate the fallback range
+_CANONICAL_COORDINATE_PATTERN = re.compile(r"!3d(-?\d+(?:\.\d+)?)!4d(-?\d+(?:\.\d+)?)") + + def _extract_coordinate_from_url(url: str, *, index: int) -> float | None: - match = re.search(r"!3d(-?\d+(?:\.\d+)?)!4d(-?\d+(?:\.\d+)?)", url) + match = _CANONICAL_COORDINATE_PATTERN.search(url) if match is None: match = re.search(r"@(-?\d+(?:\.\d+)?),(-?\d+(?:\.\d+)?)", url) if match is None: return None try: - return float(match.group(index + 1)) + value = float(match.group(index + 1)) except ValueError: return None + bound = 90 if index == 0 else 180 + if not (-bound <= value <= bound): + return None + return valueAlso applies to: 4299-4319
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gmaps_scraper/place_scraper.py` around lines 4287 - 4296, Update _extract_coordinate_from_url and _extract_canonical_place_coordinates to share one module-level regex for !3d/!4d extraction, and validate extracted latitude/longitude pairs with _valid_coordinates before returning them. Ensure fallback extraction cannot re-admit out-of-range or malformed canonical coordinates, while preserving the existing `@lat`,long fallback behavior and partial-extraction handling.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/gmaps_scraper/place_scraper.py`:
- Around line 4287-4296: Update _extract_coordinate_from_url and
_extract_canonical_place_coordinates to share one module-level regex for !3d/!4d
extraction, and validate extracted latitude/longitude pairs with
_valid_coordinates before returning them. Ensure fallback extraction cannot
re-admit out-of-range or malformed canonical coordinates, while preserving the
existing `@lat`,long fallback behavior and partial-extraction handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b72a279-2e33-40d1-a62d-1fc27058ae40
📒 Files selected for processing (6)
docs/ARCHITECTURE.mdsrc/gmaps_scraper/cli.pysrc/gmaps_scraper/parser.pysrc/gmaps_scraper/place_scraper.pytests/test_parser.pytests/test_place_scraper.py
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_86f2ffff-235c-4d0e-818f-b7342901f543) |
|
Addressed the outside-diff coordinate validation finding from CodeRabbit review 4680412102 in c4bd9af. Canonical |
1 participant
Summary
!3d/!4dpin coordinates from a canonical resolved place URL over conflicting preview payload coordinates@lat,lngviewport coordinates only as a fallback when structured coordinates are unavailableWhy
The issue surfaced while investigating demflyers-favorites PR #101: the saved-list feed itself supplied two incoherent rows, and a separate place-page path could let coordinates from an unrelated preview entity outrank the selected place marker.
Verification
./scripts/lint.sh./scripts/typecheck.shuv run python3 -m unittest discover -s tests— 280 tests passedgit diff --checkNote
Medium Risk
Changes core coordinate and URL emission logic for place and list outputs; wrong regex or precedence could shift lat/lng or links for many records, though behavior is covered by new unit tests.
Overview
Hardens saved-list place links and place-page coordinates so downstream feeds get less ambiguous location data.
For saved-list rows,
maps_urlnow prefers a directhttps://www.google.com/maps?cid=…link when there is no address but a CID is known; name-only search URLs remain when both address and CID are missing.For place scraping, lat/lng from the resolved canonical
/maps/place/URL’s!3d/!4dpin now override conflicting preview (or merged snapshot) coordinates, with@lat,lngviewport values used only when structured pin coords are absent.field_sourcesrecordsresolved_urlprovenance for those coordinates; the same preference runs after DOM/preview merge inplace_scraperand in the CLI debug scrape path.docs/ARCHITECTURE.mddocuments both behaviors.Reviewed by Cursor Bugbot for commit 28b36b8. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Documentation