Skip to content

Release/2.5.4: fix common-phone-number storing the wrong number when +1 is typed - #20

Merged
istevens507 merged 6 commits into
mainfrom
release/2.5.4
Oct 14, 2026
Merged

istevens507 merged 6 commits into
mainfrom
release/2.5.4

Conversation

@istevens507

Copy link
Copy Markdown
Contributor

Problem

common-phone-number already shows a +1 prefix, but its mask accepted any digit first. Typing +1 (234) 567-8978 stored 1234567897: the typed 1 became the area code and the last digit was dropped. The validator only counted digits, so the control stayed valid. In MSP DEAM the submission then failed with should match pattern "^[2-9]([0-9]{9})$". The mask has behaved this way since 1.0.0.

Fix

  • Default mask is now (N00) 000-0000, where N accepts 2-9 only, so a typed leading 1 or 0 is ignored. The stock ngx-mask tokens are kept, so a custom [mask] without N behaves as before.
  • The validator also requires PhoneNumberRegEx (10 digits, first digit 2-9), so programmatic values are checked too.

Breaking (released as a patch, following 2.5.2)

  • A value starting with 0 or 1 is now invalid, and with the default mask it loses its first digit when written to the control. No North American area code starts with 0 or 1.
  • N is now a mask token. No enrolment repo passes a custom [mask].

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The default masked configuration still accepts a programmatic numeric zero as empty, and the release note misstates programmatic model behavior.

3 open findings
What changed in this PR

Fixes common-phone-number handling of typed country codes and invalid area-code prefixes.

Changes:

  • Adds an N mask token requiring an initial digit from 2–9.
  • Strengthens validation and adds regression tests.
  • Bumps the package to 2.5.4 with release notes.
File Description
src/​lib/​components/​phone-number/​phone-number.component.ts Adds mask pattern and validation.
src/​lib/​components/​phone-number/​phone-number.component.spec.ts Adds phone-input regression tests.
src/​lib/​components/​phone-number/​phone-number.component.html Supplies custom patterns to ngx-mask.
projects/​common-lib/​package.json Bumps published package version.
package.json Bumps workspace version.
package-lock.json Synchronizes lockfile version.
CHANGELOG.md Documents the 2.5.4 release.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/lib/components/phone-number/phone-number.component.spec.ts
Comment thread src/lib/components/phone-number/phone-number.component.ts
Comment thread CHANGELOG.md Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The breaking public behavior should not be distributed as an automatically adopted patch release.

1 open finding
3 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Publish breaking mask changes under a major version

projects/​common-lib/​package.json:3

This release is explicitly documented as breaking: existing custom masks containing literal N change meaning, and loading some existing values can rewrite the model. Publishing it as 2.5.4 allows consumers using a normal ^2.5.x range to receive those changes automatically, which is incompatible with patch-version expectations. Use a major version for this public API behavior change, or preserve the prior behavior for 2.x consumers.

🧠 Review effort: Balanced

Comment thread src/lib/components/phone-number/phone-number.component.stories.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The documented breaking validation change should follow the repository’s established major-version policy.

0 open findings

1 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Release validation-breaking changes as the next major version

projects/​common-lib/​package.json:3

Publishing this as 2.5.4 conflicts with the repository's established compatibility policy: CHANGELOG.md:880-891 used a major release specifically because validation fixes can make previously accepted forms invalid without a compile-time warning. This PR documents the same break and can also rewrite loaded values, so consumers using ^2.5.3 could receive it automatically as a patch. Please release it as the next major version and synchronize the root manifest, lockfile, and changelog.

🧠 Review effort: Balanced

@istevens507

Copy link
Copy Markdown
Contributor Author

🔵 Needs a closer look

The documented breaking validation change should follow the repository’s established major-version policy.

0 open findings

1 resolved since last review

Previously missed (1)
In code that hasn't changed since last review

Medium severity Release validation-breaking changes as the next major version
projects/​common-lib/​package.json:3

Publishing this as 2.5.4 conflicts with the repository's established compatibility policy: CHANGELOG.md:880-891 used a major release specifically because validation fixes can make previously accepted forms invalid without a compile-time warning. This PR documents the same break and can also rewrite loaded values, so consumers using ^2.5.3 could receive it automatically as a patch. Please release it as the next major version and synchronize the root manifest, lockfile, and changelog.

🧠 Review effort: Balanced

Keeping 2.5.4 as a patch, following the 2.5.2 precedent (common-phone-number began rejecting partial numbers in a patch). The consumers build with npm ci from lockfiles, so none pick this up without an explicit update. BCP, the other consumer of this component, was verified against the packed tarball (unit tests, build, Cypress happy paths) and loads no saved phone data. The Breaking section documents the change for anyone who upgrades.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The file-uploader documentation incorrectly claims that CannotOpenPDF is displayed rather than emitted through errorDocument.

1 open finding

🧠 Review effort: Balanced

Comment thread src/lib/components/file-uploader/file-uploader.component.stories.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The implementation addresses typed and programmatic values with focused regression coverage and consistent release documentation.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

@istevens507
istevens507 enabled auto-merge (squash) October 14, 2026 14:38
@istevens507
istevens507 merged commit ccce203 into main Oct 14, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants