feat(phone): locale-aware region derivation + fix two-column fixture phone - #100
Conversation
rohithgollapalli
left a comment
rohithgollapalli
left a comment
There was a problem hiding this comment.
A few non-blocking notes from review. Overall this is clean and well-tested - fixture persona verified synthetic, two_column trigger and markdown coverage both intact. Approving direction; just some follow-up nits inline.
| // by US_LOCATION_RE (which matches any 2-uppercase-letter token after a comma). | ||
| const intlMatch = INTL_LOCATION_RE.exec(location); | ||
| if (intlMatch) { | ||
| const countryTail = intlMatch[2].trim().toLowerCase(); |
There was a problem hiding this comment.
regionFromLocation only inspects the segment immediately after the first comma. "Bengaluru, India" maps to IN, but "Bengaluru, Karnataka, India" (City, State, Country) yields no map hit, so the caller falls back to US and an Indian national-format number won't parse. No regression (it degrades to current US-default behavior) and City, Country is the common form, but worth a one-line comment noting the country must be the segment right after the first comma - it sets expectations for the explicit table.
There was a problem hiding this comment.
Good catch. Added a JSDoc note to regionFromLocation clarifying that three-part strings ("City, State, Country") don't map — INTL_LOCATION_RE captures only the first comma-segment as the country tail, so the country must sit immediately after the first comma. Fixed in 9e59a01.
| * 2. `/\+\d/` — catches E.164 international numbers (`+44 …`, `+1 …`) | ||
| * whose space-separated groups fall outside PHONE_RE's US-biased pattern. | ||
| * | ||
| * For non-US regions the pre-filter is relaxed to any 6+ digit sequence, |
There was a problem hiding this comment.
Doc/impl mismatch: this says the non-US filter is relaxed to "any 6+ digit sequence," but the code (line 195) is text.replace(/\D/g, "").length >= 7 - that's 7+ total digits across the string, not a contiguous sequence (the inline comment there even argues against a sequence check). Suggest tightening this JSDoc to match: "7+ total digits across the string."
There was a problem hiding this comment.
You're right — doc/impl mismatch. Updated the JSDoc to "7+ total digits across the string" to match the >= 7 check and the inline comment's own wording. Fixed in 9e59a01.
| // India "098765 43210") are space-separated so no single run of 6+ digits | ||
| // exists; counting all digits is the reliable pre-filter. The heavier | ||
| // libphonenumber parser is the authoritative validity gate. | ||
| if (region !== "US") return (text.replace(/\D/g, "").length >= 7); |
There was a problem hiding this comment.
Tier-1.5 asymmetry: this relaxed pre-filter only loosens for region !== "US", but regex-fallback.ts:77 still calls findFirstPhone(rawText) with the default US region. So for an intl resume where tier-1 misses the phone, the tier-1.5 recovery path benefits from neither region derivation nor this relaxation - an intl national-format number recoverable only at tier-1.5 would still drop. The common case (tier-1 finds it) is covered, so this is a deferral not a bug, but worth an explicit note/follow-up issue since the PR title implies full-pipeline locale-awareness.
There was a problem hiding this comment.
Fixed rather than deferred. regex-fallback.ts already imports from phone.ts, so I imported regionFromLocation there too and derive region from parsed.location (set by tier-1) before the findFirstPhone call. No cascade.ts changes needed. Fixed in 9e59a01.
…phone Phone-handling epic (#71) — two children: - #69: derive phone region from the extracted location instead of a hardcoded US default. Adds regionFromLocation() (US + small explicit country→ISO table) in phone.ts, threads the derived region into findFirstPhone() at the extract-fields call site (location now resolved before the phone scan), and relaxes the mightHavePhone pre-filter for non-US regions so national-format intl numbers reach libphonenumber. Falls back to US when location is absent/unmapped. Unit + extract-fields tests for US default and GB/IN national-format paths. - #62: fix the chromium-two-column-sidebar fixture, whose 7-digit 555-0142 was unparseable (libphonenumber rejects it; not a parser gap). Replaced with a valid 10-digit synthetic (512) 555-0142 and re-baked the snapshot — phone now detected, completeness 24→27, two_column trigger preserved. In-place patch wasn't viable (CID-encoded fonts), so the PDF was regenerated from reconstructed HTML; skillsCount/ experienceCount/sectionSource deltas are layout-detection artifacts of regeneration, not parser regressions. No production-code change; README documents the 10-digit synthetic-phone convention. Resolves #69 Resolves #62 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
12f23f8 to
c6b0104
Compare
- JSDoc: "6+ digit sequence" → "7+ total digits across the string" (matches the >= 7 check at line 195, per comment #3422797695) - JSDoc: note that City, State, Country 3-part strings don't map in regionFromLocation — INTL_LOCATION_RE captures only the first comma-segment (per comment #3422797691) - Tier-1.5 region fix: import regionFromLocation into regex-fallback.ts and derive region from parsed.location before the findFirstPhone call, so intl national-format numbers benefit from locale-aware parsing when tier-1 missed the phone (closes the gap noted in comment #3422797702)
…phone (#100) Phone-handling epic (#71), two children on one branch: - #69 locale-aware region derivation: regionFromLocation() derives region from extracted location, threaded into findFirstPhone(); non-US pre-filter relaxed; falls back to US. - #62 fix two-column fixture phone: replaced unparseable 7-digit 555-0142 with valid (512) 555-0142, re-baked snapshot; two_column trigger preserved. Review nits (Rohith) addressed in cbffbae: 3-part-location JSDoc, "7+ total digits" doc fix, tier-1.5 region derivation in regex-fallback.ts. CI green (verify + fallow). Merged via admin bypass. Resolves #69 Resolves #62
…phone (#100) Phone-handling epic (#71), two children on one branch: - #69 locale-aware region derivation: regionFromLocation() derives region from extracted location, threaded into findFirstPhone(); non-US pre-filter relaxed; falls back to US. - #62 fix two-column fixture phone: replaced unparseable 7-digit 555-0142 with valid (512) 555-0142, re-baked snapshot; two_column trigger preserved. Review nits (Rohith) addressed in cbffbae: 3-part-location JSDoc, "7+ total digits" doc fix, tier-1.5 region derivation in regex-fallback.ts. CI green (verify + fallow). Merged via admin bypass. Resolves #69 Resolves #62
2 participants
Summary
Phone-handling epic (#71), two children landed together on one branch:
regionFromLocation()(US + a small explicit country→ISO table) inphone.tsderives the region from the résumé's already-extractedlocation, threaded intofindFirstPhone()at theextract-fieldscall site (location is now resolved before the phone scan). ThemightHavePhonepre-filter is relaxed for non-US regions so national-format intl numbers reach libphonenumber. Falls back toUSwhen location is absent or unmapped.chromium-two-column-sidebarfixture carried a 7-digit555-0142, unparseable by libphonenumber (a bad-fixture artifact, not a parser gap — we deliberately do not broaden to 7-digit). Replaced with a valid 10-digit synthetic(512) 555-0142and re-baked the snapshot: phone now detected, completeness 24→27,two_columntrigger preserved.Note on #62 snapshot churn
In-place phone patching wasn't viable (CID-encoded fonts — no ASCII bytes to swap), so the PDF was regenerated from reconstructed HTML. The
skillsCount(5→10),experienceCount(7→5), andsectionSource(markdown→regex) deltas are layout-detection artifacts of regeneration, not parser regressions. Two-column detection and the fixture's #60 purpose are intact. No production-code change in #62.Resolves #69
Resolves #62
Test plan
npm run typecheckcleannpm run testgreen (407/407)Jamie Rivera/@example.com/(512) 555-0142, no real PII (Step 3.5)npm run dev/npm run preview