chore: pre-release improvements from multi-lens review#69
Merged
Conversation
Verified, low-risk improvements surfaced by a 6-persona review pass
(every finding independently confirmed before inclusion).
Correctness / robustness:
- validatePhone: coerce input with String() so non-string args return
false instead of throwing (every other validator already does this).
- isRepeated: replace the per-call `new RegExp(ref[0])` (which throws on
regex metacharacters and treats '.' as a wildcard) with a char loop —
fixes the latent crash/false-positive and is ~16x faster on the hot path.
- validateEmail: raise the TLD cap from {2,10} to {2,24} so valid long
TLDs (.photography, .international) are accepted.
- Remove dead code: validateCPF's unreachable `!clearValue`, validatePIS's
redundant onlyDigitsRegex test.
- validateCNH: add the explicit `: boolean` return type for API consistency.
Perf (behaviour-preserving):
- validateCNPJ / validatePIS checksums: replace split('').reduce with a
plain index loop (no per-call array allocation).
- validatePhone: indexOf(...) !== -1 -> includes(...).
Packaging (fixes broken types for TS consumers):
- Add an `exports` map and point `types` at the emitted lib/index.d.cts
(the old `types: lib/index.d.ts` referenced a file tsdown never emits;
verified green across node10/node16/bundler with @arethetypeswrong/cli).
- package.json: structured `repository`, add `bugs`, `engines` (node >=18),
`publishConfig.provenance`, and a clearer/searchable `description`.
- tsdown: `clean: true`.
Tests:
- Add coverage thresholds (lock the current floor).
- Fix the duplicate `describe('UseValidationBr Pis')` that actually tested
UF; add CNH dispatch, non-string, and long-TLD cases.
Docs:
- README/README.en: fix validateCEP -> validateCep, add `cnh` to the
useValidationsBR type union, document the alphanumeric CNPJ format, and
translate the leftover Portuguese headings in the English README.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
coverage: 97.765% (+6.3%) from 91.492% — chore/review-improvements into main |
From the review triage you approved: - #7 Branch-coverage tests: add verified-valid IEs (brute-forced and confirmed by the validators themselves) that exercise the previously untested ">= 10 -> clamp to 0" check-digit branches across RJ/RS/MT/PR/ MG/PE/AL/GO/RO, plus 10-digit RN, 9-digit TO, and invalid prefix/length cases for DF/BA/TO; add CNH numbers covering the exceptional DV branches. Branch coverage 85% -> 92%; thresholds raised to 95/88/100/95. - #9 tsconfig: module ESNext + moduleResolution bundler (matches the tsdown pipeline instead of the misleading commonjs); add isolatedDeclarations (faster dts via oxc, enforces explicit return types — added the missing one on isRepeated). - #8 SP: replace `(sum % 11).toString().substring(...)` with `(sum%11)%10`. Skipped/deferred (see PR discussion): full tsdown publint/attw build flags (would hard-couple the release build to those tools), the ie calcDigit dedup and naming nits (cosmetic), and the genuinely-unreachable branches (AM sum<11 needs an all-zero body; utils mod11 reset is dead defensive code). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adversarial review of the PR flagged two real items (verified): - The CNH "exceptional branch" comment over-claimed: only 64644399109 exercises the dv>=10 / dv<0 paths. Comment corrected; kept the extra valid samples. - The "unreachable" framing for AM/AP/GO/RN branches was wrong — they are reachable, just untested. Added verified-valid IEs that hit them: AM sum<11, both AP body ranges, GO special range, RN 10-digit DV clamp, plus the AP wrong-length guard. Branch coverage 91.8% -> 94.2%; thresholds raised to 97/90/100/97. Only utils.ts mod11 reset remains genuinely unreachable with current callers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pre-release hardening from a 6-persona review (domain correctness, tests/QA, packaging/DX, code quality, CI/security, performance) + a focused tsdown pass. Every finding was independently verified before inclusion — including two confident-but-wrong claims that were rejected (see bottom). Do not merge yet — for review.
Included (verified, low-risk)
Correctness / robustness
validatePhone(null)no longer throws — addedString()coercion to match every other validator. (3 agents)isRepeatedrewritten to a char loop — the oldnew RegExp(ref[0])threw on regex metachars (*,+,(,[,\) and treated.as a wildcard (false positives); also ~16× faster on the hot path. Behaviour identical for all real (sanitized) inputs.{2,10}→{2,24}— accepts valid long TLDs (.photography,.international). Only loosens; no previously-valid email is now rejected.validateCPF's unreachable!clearValue;validatePIS's redundantonlyDigitsRegextest.validateCNH: explicit: booleanreturn type.Performance (behaviour-preserving)
split('').reduce→ index loop (no per-call array alloc).validatePhone:indexOf(...) !== -1→includes(...).Packaging — fixes broken types for all TS consumers
typespointed atlib/index.d.ts, which tsdown never emits. Added anexportsmap + repointedtypestolib/index.d.cts. Verified green across node10 / node16 (CJS+ESM) / bundler with@arethetypeswrong/cli("No problems found 🌟").repository, addedbugs,engines(node >=18),publishConfig.provenance, clearerdescription.tsdown: clean: true.Tests
describe('UseValidationBr Pis')block that actually tested UF; added CNH-dispatch, non-string, and long-TLD cases. 119 tests pass; coverage 94.98% / 85.09% / 100% / 94.72%.Docs
validateCEP→validateCep(broken example), addedcnhto theuseValidationsBRtype union, documented the alphanumeric CNPJ format, translated leftover PT headings inREADME.en.md.CI
concurrency+timeout-minuteson ci/e2e.Deferred (your call — not in this PR)
Listed in the review summary I'm sending you. Highlights: full tsdown
exports:trueauto-gen (would make all 27 IE state files public subpaths),publint/attw/isolatedDeclarationsin the build, action SHA-pinning + Dependabot,GH_PAT→GITHUB_TOKEN, the possible double-GitHub-release config,useValidationsBRdynamic-import tree-shaking, and ~15 per-state branch-coverage tests.Rejected (verified false)
1234567-48validatestrue; the current code is correct. Would have shipped a regression.biome.jsoncnoSubstr comment is wrong" —biome lint --only=style/noSubstrflags 49.substring()calls; the rule does target substring, the comment is accurate.🤖 Generated with Claude Code