fix(progress-spinner)!: errors following a full review of the component (#DS-5482) by artembelik · Pull Request #1979 · koobiq/angular-components

artembelik · 2026-09-02T14:41:36Z

What

A full review of progress-spinner, in the same shape as the 20.3.0 component reviews.

`size` was the last accessor input — and it did two things

set size(value: ProgressSpinnerSize | string) {
    this._size = value;
    this.svgCircleRadius = value === 'big' ? '47%' : '42.5%';
}

That side effect is exactly why the automated signal migration skipped it. The radius is a computed now, size is a plain input(), and it is typed ProgressSpinnerSize instead of an arbitrary string — resolving the //@TODO use Exclude<KbqDefaultSizes, 'normal'> that predates this review.

`value` gained `numberAttribute`

value="40" used to pass the string "40"; Math.min(100, '40') coerced it by accident. It is a number now.

Closed internals

percentage, dashOffsetPercent and svgCircleRadius are protected. The first two are derived from the value the consumer already binds; the last is SVG geometry, not a contract.

`_IdGenerator`

The generated id comes from the CDK _IdGenerator instead of a module-level let id = 0, so it no longer collides across lazily loaded bundles. The kbq-progress-spinner-<n> shape is unchanged.

One line outside the component

KbqLoaderOverlay.spinnerSize reports ProgressSpinnerSize rather than string, to keep feeding the narrowed input. It is a narrowing of a getter's return type, so nothing that reads it breaks.

Migration

progress-spinner-signals runs from ng update @koobiq/components@20. It rewrites size reads to calls — on receivers typed KbqProgressSpinner and through template reference variables on <kbq-progress-spinner>, in external and inline templates — and reports the rest.

Documented in docs/guides/migration.{en,ru}.md, section 18.

Testing

progress-spinner.component.spec.ts rewritten: 8 → 10 tests. The old ones read percentage off the instance; they assert the rendered stroke-dashoffset, circle radius and host classes now, which is what the value actually drives.
progress-spinner-signals/index.spec.ts: 13 tests — auto-fix, idempotence, receiver scoping, template refs, warnings, and the --fix=false path.
Full packages/components (4994 tests) and packages/schematics (447 tests) suites pass.
check-api is in sync.

No e2e screenshots were regenerated: the e2e cases pass size="compact" / size="big" as before, and the rendered geometry is unchanged.

BREAKING CHANGE

🤖 Generated with Claude Code

`size` was the last accessor input on the spinner, and the reason the automated signal migration skipped it: its setter stored the size and computed the SVG circle radius in one go. The radius is a `computed` now and `size` is a plain `input()`, typed `ProgressSpinnerSize` instead of an arbitrary string — which resolves a TODO that predates this review. `value` gained `numberAttribute`: `value="40"` used to pass the string `"40"`, which the percentage arithmetic coerced by accident. `percentage`, `dashOffsetPercent` and `svgCircleRadius` are `protected`. They are derived from the inputs the consumer already binds, and the last one is SVG geometry rather than a contract. The generated `id` comes from the CDK `_IdGenerator` instead of a module-level counter, so it no longer collides across lazily loaded bundles. `KbqLoaderOverlay.spinnerSize` reports `ProgressSpinnerSize` rather than `string` to keep feeding the narrowed input. BREAKING CHANGE: `KbqProgressSpinner.size` is a signal and no longer accepts an arbitrary string; `percentage`, `dashOffsetPercent` and `svgCircleRadius` are protected; `value` is a `numberAttribute` input, so a `null` binding yields `NaN` where it used to clamp to `0`. Reported and partly rewritten by the `progress-spinner-signals` schematic. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

github-actions · 2026-09-02T14:45:57Z

Visit the preview URL for this PR (updated for commit 3ffe091):

https://koobiq-next--prs-1979-cqb35iq6.web.app

_{(expires Sat, 05 Sep 2026 14:45:46 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: c9e37e518febda70d0317d07e8ceb35ac43c534c}

lskramarov

Code review — `max` effort

15 findings, ranked most severe first. Thirteen are correctness, one is test coverage, one is the
migration guide.

The two that block: the schematic never writes anything under ng update
(index.ts:394), and numberAttribute without a fallback makes a nullish [value] paint a full ring
instead of an empty one (progress-spinner.component.ts:64).

Most of the rest are guards that exist in the newer sibling migrations (navbar-signals-and-aria,
button-toggle-signals-and-aria) but not in this clone of the older alert-signals: the assignment
lookahead in the ref regex, variableScope for block-scoped locals, initializerTypeOf for
inject()/viewChild() receivers, and the template-side manual-member report.

Posted by Claude Code.

lskramarov · 2026-09-03T06:59:47Z

+
+export default function progressSpinnerSignals(options: Schema): Rule {
+    return async (tree: Tree, context: SchematicContext) => {
+        const { project, fix } = options;


fix is undefined under ng update, so the migration never writes anything.

migrations.json declares no schema for this entry (correctly — that matches every sibling), so the
"default": true in schema.json never reaches the rule when the CLI runs the migration. options.fix is
undefined, commit() takes the else branch for every file, and the consumer gets
[progress-spinner-signals] would update <file> (run with --fix to apply) with zero writes — followed by a
build failure on the very size reads that were supposed to be rewritten.

Eleven siblings guard this. list-tree-multiple-input/index.ts:219-220 even carries the explanation:

ng update invokes migrations with no options at all, and migrations.json declares no schema,
so the schema default never reaches us — applying the fix is the intended behaviour there.

index.spec.ts:37 always passes fix explicitly (run(fix: boolean = true)), so no test can catch it, and
Schema.fix being typed non-optional hides it from the compiler.

Suggested change

const { project, fix } = options;

const { project } = options;

// `ng update` invokes migrations with no options at all, and migrations.json declares no schema,

// so the schema default never reaches us — applying the fix is the intended behaviour there.

const fix = options.fix ?? true;

lskramarov · 2026-09-03T06:59:47Z

-    set size(value: ProgressSpinnerSize | string) {
-        this._size = value;
+    /** Progress of the operation, in percent. Clamped to `[0, 100]` and only rendered in `determinate` mode. */
+    readonly value = input(0, { transform: numberAttribute });


numberAttribute with no fallback turns a nullish binding into a fully-drawn ring.

numberAttribute(v, fallbackValue = NaN) returns NaN for null, undefined, '', 'abc', '40px',
true, [] and {}. NaN survives the clamp — Math.max(0, Math.min(100, NaN)) is NaN — so
dashOffsetPercent() produces the literal string "NaN%".

CSSOM rejects an unparseable declaration value, so style.setProperty('stroke-dashoffset', 'NaN%') is a
no-op:

first render — nothing is set. .kbq-progress-spinner__circle declares stroke-dasharray: 295% and no
stroke-dashoffset outside the indeterminate branch, so the initial 0 applies and the ring paints
100% complete.

later transition from a valid value — the previous inline offset stays, so the ring freezes at the
old percentage.

So <kbq-progress-spinner [value]="progress$ | async" /> renders "done" while the data is still loading. The
base version clamped null to 0 and painted an empty ring.

The type-level guard moved the other way at the same time: the guard file now records
InputSignalWithTransform<number, unknown>, so under strictTemplates [value]="progress$ | async",
[value]="'40%'" and [value]="{}" all type-check where they used to be errors. The PR narrows size for
type safety and drops the only static check on value in the same commit.

Minor, same subject: the migration note says "A binding that passes null or undefined used to clamp to
0". Only null did — Math.min(100, undefined) was already NaN on the base version.

Suggested change

readonly value = input(0, { transform: numberAttribute });

readonly value = input(0, { transform: (value: unknown) => numberAttribute(value, 0) });

lskramarov · 2026-09-03T06:59:47Z

+
+    for (const ref of refs) {
+        // `\bref\.(member)\b(?!\s*\()` — skip anything already invoked, so the rewrite is idempotent.
+        const pattern = new RegExp(`\\b(${escapeRegExp(ref)})\\.(${members})\\b(?!\\s*\\()`, 'g');


The template-ref rewrite has no assignment guard, so it emits a template that will not parse.

The lookahead only skips an immediate (, so an assignment passes straight through:

<kbq-progress-spinner #spinner /> <button (click)="spinner.size = 'big'">grow</button>

becomes (click)="spinner.size() = 'big'" — an Angular expression parse error. The migration turns a
compiling app into a build failure.

This also contradicts the TS pass in this same file: the comment at lines 149-150 says a write is
deliberately left untouched because there is no writable member. The invariant holds in TypeScript and is
broken in every external and inline template.

navbar-signals-and-aria/index.ts:405-420 already has exactly this guard, with the reasoning spelled out
("rewriting ref.expanded = value blindly into ref.expanded() = value would replace a valid assignment
with invalid syntax"), and reports such writes separately via collectRefWriteWarnings.

Suggested change

const pattern = new RegExp(`\\b(${escapeRegExp(ref)})\\.(${members})\\b(?!\\s*\\()`, 'g');

const pattern = new RegExp(`\\b(${escapeRegExp(ref)})\\.(${members})\\b(?!\\s*\\()(?!\\s*=(?!=))`, 'g');

lskramarov · 2026-09-03T06:59:47Z

+    for (const ref of refs) {
+        // `\bref\.(member)\b(?!\s*\()` — skip anything already invoked, so the rewrite is idempotent.
+        const pattern = new RegExp(`\\b(${escapeRegExp(ref)})\\.(${members})\\b(?!\\s*\\()`, 'g');
+        const next = content.replace(pattern, '$1.$2()');


The rewrite is a raw regex over the whole template text, so it corrupts unrelated identifiers.

The AST is used only to discover ref names; the replacement then ignores it entirely and matches every
textual <ref>.size in the file. Verified outputs for a template that declares
<kbq-progress-spinner #spinner />:

input output

@for (spinner of rows; track spinner.id) { {{ spinner.size }} } {{ spinner.size() }} on a plain row object

[cfg]="state.spinner.size" state.spinner.size() — \b matches after a .

 comment rewritten

<p>Read spinner.size to get the size.</p> visible body text rewritten

<img alt="spinner.size" /> attribute string rewritten

The @for case is the dangerous one: spinner is a very common loop-variable name, and the result is
TypeError: spinner.size is not a function on the first change-detection pass, in a file the migration
reported as successfully fixed.

Two smaller issues in the same expression:

members (SIGNAL_MEMBERS.join('|')) is interpolated unescaped while ref goes through
escapeRegExp — safe only because the array is ['size'] today.

SpinnerRefCollector ignores the ref's exportAs value, so #spinner="cdkOverlayOrigin" on a spinner
element is collected as a spinner ref even though spinner is then the directive instance.

lskramarov · 2026-09-03T06:59:48Z

+    if (
+        ts.isBinaryExpression(parent) &&
+        parent.left === node &&
+        parent.operatorToken.kind === ts.SyntaxKind.EqualsToken


Only = is recognised as a write, so every other assignment form is rewritten into invalid syntax.

The early return lives inside the EqualsToken branch, so any other parent falls through to the "read"
branch at line 167 and gets () appended. Verified outputs on a receiver typed KbqProgressSpinner:

in out

spinner.size += 'x'; spinner.size() += 'x';

spinner.size++; spinner.size()++;

`spinner.size

[spinner.size] = ['big']; [spinner.size()] = ['big'];

({ x: spinner.size } = o); ({ x: spinner.size() } = o);

All five are TS2364: The left-hand side of an assignment expression must be a variable or a property access — the consumer gets a syntax error on already-overwritten source, rather than the clean compile
error the comment above promises.

Conversely const { size } = spinner, spinner['size'] and delete spinner.size are silently skipped with
no edit and no warning. Guarding the read branch on "not an assignment target of any kind" covers both
directions better than enumerating one token.

lskramarov · 2026-09-03T06:59:48Z

+
+            if (!original) continue;
+
+            const { content, changed } = await migrateTemplate(original);


Protected-member reads in templates are never reported, though three docs promise they are.

PROTECTED_MEMBERS is reachable only through collectProtectedAccess → warnReceiverMembers, which runs
exclusively in the .ts loop and keys off TypeScript type annotations. This HTML loop calls only
migrateTemplate, which rewrites size and returns nothing else; migrateInlineTemplates likewise
produces no findings.

So:

<kbq-progress-spinner #s [value]="v" /> <span>{{ s.percentage }}</span>

migrates with a clean report, then fails the next AOT build with NG9 "Property 'percentage' is protected
and only accessible within class 'KbqProgressSpinner'" — and no file pointer. collection.json:148,
README.md:37 and the migration guide all state these members are reported.

Both newer siblings have a template-side reporting path for exactly this (collectRefManualMembers in
navbar-signals-and-aria, Findings.manual in button-toggle-signals-and-aria).

Two smaller gaps in the same loop worth folding in: an unparseable template is swallowed at index.ts:307
with no diagnostic at all (the siblings thread an unparseable flag and log
UNPARSEABLE_TEMPLATE_MESSAGE), and consumers is incremented on "mentions the spinner" for .ts but on
"was actually rewritten" for .html, so a project whose only usage is a plain [value] binding in an
external template can end with consumers === 0 and lose the whole SUMMARY.

lskramarov · 2026-09-03T06:59:48Z

+export const warnPatterns: WarnPattern[] = [
+    {
+        anchor: '\\bKbqProgressSpinner\\b',
+        pattern: '(?:viewChild|ViewChild|contentChild|ContentChild)[^\\n;]*\\bKbqProgressSpinner\\b',


The warn pattern matches the decorator queries too, and then tells the user to write code that throws.

The alternation includes ViewChild/ContentChild, but a decorator query returns the instance, not a
signal. The migration's own spec case (index.spec.ts:73-89) matches:

@ViewChild(KbqProgressSpinner) spinner: KbqProgressSpinner; read() { return this.spinner.size; }

The schematic correctly rewrites it to this.spinner.size() and, in the same run, logs "reading it is a
double call, e.g. this.spinner().size()". A consumer who follows the warning edits the correct output into
this.spinner().size() → TypeError: this.spinner is not a function.

The spec asserts the rewrite but never asserts the warning is absent, so the contradiction is invisible.

Suggested change

pattern: '(?:viewChild|ViewChild|contentChild|ContentChild)[^\\n;]*\\bKbqProgressSpinner\\b',

pattern: '(?:viewChild|contentChild)[^\\n;]*\\bKbqProgressSpinner\\b',

lskramarov · 2026-09-03T06:59:48Z


 const MIN_PERCENT = 0;
 const MAX_PERCENT = 100;
+const MAX_DASH_ARRAY = 295;


The dash array is calibrated for the big radius only, so the default compact size over-draws.

The review made the radius vary with size (47% / 42.5%) but left MAX_DASH_ARRAY size-independent.
With viewBox="0 0 100 100", SVG percentages resolve against a normalized diagonal of 100, so:

size r path length dash array

big 47% 2π·47 = 295.3 295 ✔

compact (default) 42.5% 2π·42.5 = 267.0 295 ✘

Drawn fraction on compact is 295·v / 267 = 1.105·v:

[value]="44" — what e2e.ts renders — paints 48.6% on compact vs 44.0% on big. Same input, two
different pictures.

[value]="50" paints 55%.

everything from 90.5 upward paints a complete ring, so a compact determinate spinner has no last 10%
of travel.

Pre-existing, but this PR rewrote all three functions involved (svgCircleRadius, percentage,
dashOffsetPercent) and replaced the spec's size-independent percentage assertions with raw offset
strings (dashOffsetPairs), which turns the miscalibration into an asserted expectation rather than a
detectable defect.

pathLength="100" on the <circle> makes the arithmetic size-independent by construction. Note
progress-spinner.scss:22 hardcodes the same 295 a second time, and the indeterminate arc
(stroke-dashoffset: 80%, under an untouched // TODO: rework this place) is off by the same ratio: 80.5%
of the ring on compact vs 72.8% on big.

lskramarov · 2026-09-03T06:59:48Z

-        expect(progressSpinnerDebugElement.nativeElement.getAttribute('id')).toBeDefined();
+    it('should auto generate a unique id', () => {
+        expect(defaultHost.getAttribute('id')).toMatch(/^kbq-progress-spinner-/);
+        expect(defaultHost.getAttribute('id')).not.toBe(host.getAttribute('id'));


"should auto generate a unique id" cannot fail — it compares against a hardcoded id.

host is the .first spinner, whose id is bound to readonly id = signal('test-spinner') (line 132). So
this line compares a generated id against the literal 'test-spinner', which the toMatch(/^kbq-progress- spinner-/) on the line above already implies.

Replace inject(_IdGenerator).getId('kbq-progress-spinner-') with a constant string and both assertions
still pass — the collision the PR description says was fixed ships untested. Uniqueness needs a second
unbound <kbq-progress-spinner /> in TestApp so two generated ids can be compared.

Related gaps in this file:

The numberAttribute transform — the PR's headline breaking change — has zero coverage in either
direction. TestApp.value is signal(0) typed number and no test binds value="40", null or ''.
Dropping the transform entirely would leave all 10 tests green.

If you do add such a test, assert the number, not the style: cssstyle 2.3.0 keeps stroke-dashoffset in
allExtraProperties with no value validation, so under jsdom style.strokeDashoffset reads back
'NaN%' where a real browser rejects it and keeps the previous declaration — i.e. the assertion would
encode the opposite of production behaviour.

The two negative tests in progress-spinner-signals/index.spec.ts (lines 91 and 161) both bail at an
early guard before reaching the check they are named for: deleting
element.name === SPINNER_ELEMENT from SpinnerRefCollector.visitElement keeps that suite green.

lskramarov · 2026-09-03T06:59:48Z

 ### 18. Component review (20.3.0)

-Ten components went through a full review in 20.3.0: notification-center, popover, search-expandable, select, split-button, title, toast, tooltip, tree and tree-select. Each review closed the members that were never part of the component's contract, moved inputs to signals where that was the point of it, and fixed the behavior it uncovered along the way. Only the changes that reach a consumer are listed here.
+Components went through a full review in 20.3.0, in two waves. The first covered notification-center, popover, search-expandable, select, split-button, title, toast, tooltip, tree and tree-select; the second is the one each subsection below belongs to. Each review closed the members that were never part of the component's contract, moved inputs to signals where that was the point of it, and fixed the behavior it uncovered along the way. Only the changes that reach a consumer are listed here.


The new "two waves" sentence contradicts the subsections it introduces.

It assigns search-expandable, split-button and title to the first wave, then says "the second is the
one each subsection below belongs to" — but the #### headings that follow are Progress spinner,
Search expandable, Split button and Title. Three of the four are in the list they were just
excluded from.

The pre-PR sentence ("Ten components went through a full review in 20.3.0: …") was coherent with the same
subsections. As written, a reader cannot tell which sections are new in this release.

docs/guides/migration.ru.md:1042 carries the identical defect and needs the same edit.

Suggested change

Components went through a full review in 20.3.0, in two waves. The first covered notification-center, popover, search-expandable, select, split-button, title, toast, tooltip, tree and tree-select; the second is the one each subsection below belongs to. Each review closed the members that were never part of the component's contract, moved inputs to signals where that was the point of it, and fixed the behavior it uncovered along the way. Only the changes that reach a consumer are listed here.

Components went through a full review in 20.3.0. The first wave covered notification-center, popover, select, toast, tooltip, tree and tree-select; search-expandable, split-button, title and progress-spinner followed, and each has a subsection below. Each review closed the members that were never part of the component's contract, moved inputs to signals where that was the point of it, and fixed the behavior it uncovered along the way. Only the changes that reach a consumer are listed here.

`numberAttribute` with no fallback turned a null binding into `NaN`, and the value feeds a `stroke-dashoffset` percentage — `NaN%` is not a CSS length, so the browser dropped the declaration and the circle rendered full. It falls back to 0, which is what the pre-migration input did. The schematic counted a `.html` file as a consumer only when it had a `size` read to rewrite, so a template-only consumer never heard that `size` no longer accepts an arbitrary string. Tests: the uniqueness assertion compared a generated id against a bound one, so it passed without testing uniqueness; and nothing covered `value="40"` as a static attribute, which is the case the transform exists for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

github-actions Bot added bug Something isn't working breaking changes labels Sep 2, 2026

lskramarov reviewed Sep 3, 2026

View reviewed changes

Navigation Menu

Uh oh!

fix(progress-spinner)!: errors following a full review of the component (#DS-5482) - #1979

fix(progress-spinner)!: errors following a full review of the component (#DS-5482)#1979
artembelik wants to merge 2 commits into
mainfrom
fix/progress-spinner-signals

artembelik commented Sep 2, 2026

Uh oh!

github-actions Bot commented Sep 2, 2026

Uh oh!

lskramarov left a comment

Uh oh!

lskramarov Sep 3, 2026

Uh oh!

lskramarov Sep 3, 2026

Uh oh!

lskramarov Sep 3, 2026

Uh oh!

lskramarov Sep 3, 2026

Uh oh!

lskramarov Sep 3, 2026

Uh oh!

lskramarov Sep 3, 2026

Uh oh!

lskramarov Sep 3, 2026

Uh oh!

lskramarov Sep 3, 2026

Uh oh!

lskramarov Sep 3, 2026

Uh oh!

lskramarov Sep 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-        const { project, fix } = options;
+        const { project } = options;
+        // `ng update` invokes migrations with no options at all, and migrations.json declares no schema,
+        // so the schema default never reaches us — applying the fix is the intended behaviour there.
+        const fix = options.fix ?? true;

	readonly value = input(0, { transform: numberAttribute });
	readonly value = input(0, { transform: (value: unknown) => numberAttribute(value, 0) });

	const pattern = new RegExp(`\\b(${escapeRegExp(ref)})\\.(${members})\\b(?!\\s*\\()`, 'g');
	const pattern = new RegExp(`\\b(${escapeRegExp(ref)})\\.(${members})\\b(?!\\s\\()(?!\\s=(?!=))`, 'g');

input	output
`@for (spinner of rows; track spinner.id) { {{ spinner.size }} }`	`{{ spinner.size() }}` on a plain row object
`[cfg]="state.spinner.size"`	`state.spinner.size()` — `\b` matches after a `.`
`<!-- spinner.size is derived -->`	comment rewritten
`<p>Read spinner.size to get the size.</p>`	visible body text rewritten
`<img alt="spinner.size" />`	attribute string rewritten

in	out
`spinner.size += 'x';`	`spinner.size() += 'x';`
`spinner.size++;`	`spinner.size()++;`
`spinner.size
`[spinner.size] = ['big'];`	`[spinner.size()] = ['big'];`
`({ x: spinner.size } = o);`	`({ x: spinner.size() } = o);`


		if (!original) continue;

		const { content, changed } = await migrateTemplate(original);

	pattern: '(?:viewChild\|ViewChild\|contentChild\|ContentChild)[^\\n;]*\\bKbqProgressSpinner\\b',
	pattern: '(?:viewChild\|contentChild)[^\\n;]*\\bKbqProgressSpinner\\b',

size	`r`	path length	dash array
big	`47%`	2π·47 = 295.3	295 ✔
compact (default)	`42.5%`	2π·42.5 = 267.0	295 ✘

Uh oh!

Conversation

artembelik commented Sep 2, 2026

What

size was the last accessor input — and it did two things

value gained numberAttribute

Closed internals

_IdGenerator

One line outside the component

Migration

Testing

Uh oh!

github-actions Bot commented Sep 2, 2026

Uh oh!

lskramarov left a comment

Choose a reason for hiding this comment

Code review — max effort

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

`size` was the last accessor input — and it did two things

`value` gained `numberAttribute`

`_IdGenerator`

Code review — `max` effort