Fix saved list CID parsing#49
Conversation
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_ffd843cf-7753-4c2f-aff6-db0e0a8dac0d) |
📝 WalkthroughWalkthroughThis PR refactors CID extraction in the Google Maps scraper parser by splitting a single helper into structured-slot and fallback-value parsing paths with new normalization and rejection rules. Tests are updated with shared cell ID/CID constants and new regression cases covering slot-6 fingerprint and fallback edge cases. ChangesCID Extraction Refactor
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/gmaps_scraper/parser.py (2)
640-660: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDuplicate owner-rejection and numeric-extraction logic across both helpers.
The owner-rejection block (647-649) is identical to (668-670), and the numeric-token extraction comprehension (651-655) is identical to (671-675). Extracting these into shared helpers would remove the duplication and keep the two selection-rule differences (index-1 vs. single-token) as the only distinguishing logic.
♻️ Proposed extraction
+def _is_rejected_owner_list(value: list[JSONValue]) -> bool: + owner = _parse_list_owner(value) + return owner is not None and (owner.photo_url is not None or owner.profile_id is not None) + + +def _extract_long_integer_texts(value: list[JSONValue]) -> list[str]: + return [ + text + for text in (_clean_text(item) for item in value) + if text is not None and _LONG_INTEGER_PATTERN.fullmatch(text) is not None + ] + + def _find_cid_in_structured_slot(value: JSONValue | None) -> str | None: ... if not isinstance(value, list): return None - owner = _parse_list_owner(value) - if owner is not None and (owner.photo_url is not None or owner.profile_id is not None): + if _is_rejected_owner_list(value): return None - numeric_texts = [ - text - for text in (_clean_text(item) for item in value) - if text is not None and _LONG_INTEGER_PATTERN.fullmatch(text) is not None - ] + numeric_texts = _extract_long_integer_texts(value) ...Apply the same substitution in
_find_cid_in_fallback_value.Also applies to: 663-678
🤖 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/parser.py` around lines 640 - 660, The CID parsing logic in _find_cid_in_structured_slot and _find_cid_in_fallback_value duplicates the same owner-rejection check and numeric-token extraction, so factor those shared steps into helper functions and reuse them in both paths. Keep the existing selection differences isolated to each caller (index-1 vs single-token behavior), and make sure the shared helpers cover the _parse_list_owner, _clean_text, and _LONG_INTEGER_PATTERN-based filtering consistently.
640-660: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject short numeric CID candidates in the structured-slot path.
_find_cid_in_structured_slotstill normalizes bare ints/strings and single-element lists with_normalize_cid_token, so 10–14 digit timestamp-like values can slip through even though the fallback path now requires 15+ digits. Reuse the length-gated normalizer here as well, or share a single candidate check.🤖 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/parser.py` around lines 640 - 660, The structured-slot CID parser still accepts bare ints/strings and single numeric list values without the same length gate as the fallback path, allowing short timestamp-like numbers to be normalized. Update _find_cid_in_structured_slot to apply the same minimum-length CID validation used elsewhere, or centralize the candidate check so _normalize_cid_token is only called for valid 15+ digit CID candidates, including the numeric_texts selection logic.Source: Coding guidelines
🧹 Nitpick comments (2)
tests/test_parser.py (1)
343-343: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
_NORTHWIND_CELL_IDinstead of the duplicated literal.
"7451636382641713350"is the same value as_NORTHWIND_CELL_IDdefined at Line 18. Since this PR's whole point is consolidating these magic numbers into shared constants, this line should use the constant for consistency.♻️ Proposed fix
- second_metadata[6] = ["7451636382641713350", _NORTHWIND_CID] + second_metadata[6] = [_NORTHWIND_CELL_ID, _NORTHWIND_CID]🤖 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 `@tests/test_parser.py` at line 343, Replace the duplicated Northwind cell ID literal in the test data with the shared _NORTHWIND_CELL_ID constant so the parser tests consistently use the centralized magic-number definition. Update the assignment in the test case that builds second_metadata to reference _NORTHWIND_CELL_ID alongside _NORTHWIND_CID, keeping the existing test behavior unchanged while aligning with the constant-based pattern used elsewhere in tests/test_parser.py.src/gmaps_scraper/parser.py (1)
658-659: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHardcoded
numeric_texts[1]assumes a fixed two-element ordering.When 2+ numeric tokens exist, the code unconditionally takes index 1 as the fingerprint. This matches the observed
[cellId, fingerprint]fixtures, but if a third numeric token ever appears (schema drift), index 1 may not be the fingerprint. Given the guideline to keep extraction defensive against schema drift, consider a more resilient selection (e.g., picking the largest-magnitude token, or the one that differs in sign from a plausible S2 cell ID) rather than a fixed position.🤖 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/parser.py` around lines 658 - 659, The fingerprint extraction in the parser currently assumes a fixed ordering by returning numeric_texts[1] when multiple numeric tokens are present, which is brittle if the token layout changes. Update the logic in the numeric token handling path to select the fingerprint more defensively instead of relying on the second element, using a heuristic such as a magnitude- or cell-id-aware choice while keeping _normalize_cid_token as the final normalization step.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.
Inline comments:
In `@tests/test_parser.py`:
- Around line 270-280: The test in
test_ignores_timestamp_like_fallback_values_when_cid_is_missing only checks the
final parsed cid, so it can still pass if _find_cid stops scanning fallback
values. Update this test to assert the fallback scan itself by verifying that
the appended timestamp-like value is actually handled through
_find_cid_in_fallback_value when parse_saved_list_artifacts runs. Use the
existing helpers and symbols (_find_cid, _find_cid_in_fallback_value,
parse_saved_list_artifacts, and place_metadata) so the test fails if fallback
scanning regresses.
---
Outside diff comments:
In `@src/gmaps_scraper/parser.py`:
- Around line 640-660: The CID parsing logic in _find_cid_in_structured_slot and
_find_cid_in_fallback_value duplicates the same owner-rejection check and
numeric-token extraction, so factor those shared steps into helper functions and
reuse them in both paths. Keep the existing selection differences isolated to
each caller (index-1 vs single-token behavior), and make sure the shared helpers
cover the _parse_list_owner, _clean_text, and _LONG_INTEGER_PATTERN-based
filtering consistently.
- Around line 640-660: The structured-slot CID parser still accepts bare
ints/strings and single numeric list values without the same length gate as the
fallback path, allowing short timestamp-like numbers to be normalized. Update
_find_cid_in_structured_slot to apply the same minimum-length CID validation
used elsewhere, or centralize the candidate check so _normalize_cid_token is
only called for valid 15+ digit CID candidates, including the numeric_texts
selection logic.
---
Nitpick comments:
In `@src/gmaps_scraper/parser.py`:
- Around line 658-659: The fingerprint extraction in the parser currently
assumes a fixed ordering by returning numeric_texts[1] when multiple numeric
tokens are present, which is brittle if the token layout changes. Update the
logic in the numeric token handling path to select the fingerprint more
defensively instead of relying on the second element, using a heuristic such as
a magnitude- or cell-id-aware choice while keeping _normalize_cid_token as the
final normalization step.
In `@tests/test_parser.py`:
- Line 343: Replace the duplicated Northwind cell ID literal in the test data
with the shared _NORTHWIND_CELL_ID constant so the parser tests consistently use
the centralized magic-number definition. Update the assignment in the test case
that builds second_metadata to reference _NORTHWIND_CELL_ID alongside
_NORTHWIND_CID, keeping the existing test behavior unchanged while aligning with
the constant-based pattern used elsewhere in tests/test_parser.py.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb7a3efb-0f45-44b9-a8e3-7b6bfafb3049
📒 Files selected for processing (2)
src/gmaps_scraper/parser.pytests/test_parser.py
| def test_ignores_timestamp_like_fallback_values_when_cid_is_missing(self) -> None: | ||
| runtime_state = copy.deepcopy(["noise", _LIST_NODE]) | ||
| place_metadata = runtime_state[1][8][0][1] | ||
| assert isinstance(place_metadata, list) | ||
| place_metadata[6] = [None] | ||
| place_metadata.append(1776063335) | ||
|
|
||
| parsed = parse_saved_list_artifacts(_LIST_URL, runtime_state=runtime_state) | ||
|
|
||
| self.assertEqual(parsed.places[0].cid, None) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the _find_cid dispatch function to see what value is passed to
# _find_cid_in_fallback_value, and confirm place_metadata's appended item
# is part of that scanned value.
rg -n -B5 -A30 'def _find_cid\(' src/gmaps_scraper/parser.py
rg -n '_LONG_INTEGER_PATTERN\s*=' src/gmaps_scraper/parser.pyRepository: 508-dev/gmaps-scraper
Length of output: 1425
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the helper chain around fallback CID detection and the test context.
sed -n '520,700p' src/gmaps_scraper/parser.py
printf '\n--- TEST CONTEXT ---\n'
sed -n '230,310p' tests/test_parser.pyRepository: 508-dev/gmaps-scraper
Length of output: 8821
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact fallback token normalization logic in isolation.
rg -n -A20 -B10 'def _find_cid_in_fallback_value|def _normalize_fallback_cid_token|def _find_cid\(' src/gmaps_scraper/parser.pyRepository: 508-dev/gmaps-scraper
Length of output: 3115
Assert the fallback scan, not just the final None. This still passes if _find_cid stops iterating fallback values entirely; add an assertion that the appended timestamp-like value is actually consumed by _find_cid_in_fallback_value so the regression test breaks when fallback scanning regresses.
🤖 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 `@tests/test_parser.py` around lines 270 - 280, The test in
test_ignores_timestamp_like_fallback_values_when_cid_is_missing only checks the
final parsed cid, so it can still pass if _find_cid stops scanning fallback
values. Update this test to assert the fallback scan itself by verifying that
the appended timestamp-like value is actually handled through
_find_cid_in_fallback_value when parse_saved_list_artifacts runs. Use the
existing helpers and symbols (_find_cid, _find_cid_in_fallback_value,
parse_saved_list_artifacts, and place_metadata) so the test fails if fallback
scanning regresses.
1 participant
Summary
Fix saved-list place CID extraction so the parser prefers the fingerprint in the structured metadata slot instead of returning the positive S2 cell ID. The fallback path is now narrower so timestamp-like values and owner profile IDs are not treated as place CIDs.
Validation
uv run python -m unittest tests.test_parser./scripts/lint.sh./scripts/typecheck.shgit diff --checkNote
Medium Risk
CID values drive deduplication and identity for scraped places; wrong CIDs would mis-link or drop entries, though the change is localized to parser heuristics with expanded unit tests.
Overview
Fixes place CID extraction for Google Maps saved lists so
Place.cidreflects the business fingerprint, not the positive S2 cell ID in structured metadata slot 6.When slot 6 is a list with two or more long integer strings, the parser now always uses the second value (dropping the old “first non-negative” shortcut). Negative fingerprints are still normalized via unsigned 64-bit conversion. Fallback scanning is split from structured slot parsing: it only accepts a single long integer per value, requires at least 15 digits (excluding sign), and no longer treats ambiguous multi-number lists or bare timestamp-like integers as CIDs.
Tests were updated so fixtures model cell ID + fingerprint pairs, expectations use the fingerprint constants, and new cases cover slot-6 preference, negative normalization, and ignoring timestamp fallbacks when slot 6 is empty.
Reviewed by Cursor Bugbot for commit cad5629. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests