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

[artembelik]

What

A full review of checkbox, in the same shape as the 20.3.0 component reviews.

The two inputs the automated migration skipped

id and clickAction were skipped because application code writes to them. Both are input() now, and id comes from the CDK _IdGenerator instead of let nextUniqueId = 0.

Four inputs that deliberately stay accessors

checked, disabled, indeterminate and tabIndex are two-way state — the component writes them on click, and the ControlValueAccessor writes them through the KbqCheckable host directive. A model() cannot carry the booleanAttribute / numberAttribute transform they need, and forwarding KbqCheckable's models through hostDirectives would drop the coercion disabled has today.

That is the same conclusion the reviewed KbqButtonToggle reached — it kept checked/disabled as accessors over private signals for exactly this reason. Reads and writes of those four are unchanged, and the schematic does not touch them. Flagging it explicitly in case you want to take a different route here; changing it properly would mean touching KbqCheckable in core, which the toggle shares.

Behavior the review fixed

• <kbq-checkbox [id]="null"> now falls back to the generated id on the host. It used to leave the host without an id while the hidden input still pointed its for at the generated one — so the label pointed somewhere the host didn't answer to.
• checked, big and indeterminate gained booleanAttribute. <kbq-checkbox checked> passed the empty string, which is falsy, so the valueless attribute did nothing.
• required defaults to false instead of undefined behind a boolean type, and value reports string | undefined instead of string over an undefined! default.

Closed internals

inputId, inputElement, getAriaChecked, onInputClick, onInteractionEvent and onLabelTextChange are protected — the wiring between the label, the visually hidden input and the click algorithm. focus() and toggle() remain the public way to drive the control.

Migration

checkbox-signals runs from ng update @koobiq/components@20. It rewrites the one-way input reads to calls and reports the rest, including the id shape change (kbq-checkbox-1 → kbq-checkbox-a1) and the gotcha that [clickAction]="undefined" overrides KBQ_CHECKBOX_CLICK_ACTION rather than falling back to it.

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

Testing

• checkbox.component.spec.ts: 42 → 47 tests, all existing ones kept.
  • Three tests reached into now-protected members. The indeterminate-click test called onInputClick with a synthetic new Event('inputClick'); it dispatches a real click on the native input now, which is what the other click tests already do and what actually exercises the path.
  • The clickAction override test used to mutate the field on the instance. It has its own host binding the input now — and the shared SingleCheckbox host deliberately does not bind [clickAction], because binding undefined would override the injected token and quietly break the two 'noop' tests.
  • New coverage: valueless checked/big/required attributes, the [id]="null" fallback, and the unbound required/value defaults.
• checkbox-signals/index.spec.ts: 14 tests, including one that pins that the four checkable-backed accessors are left alone.
• Full packages/components (4998 tests) and packages/schematics (448 tests) suites pass.
• check-api is in sync.

BREAKING CHANGE

🤖 Generated with Claude Code

[github-actions]

Visit the preview URL for this PR (updated for commit 469d27c):

https://koobiq-next--prs-1984-5pdr65d9.web.app

_{(expires Sat, 05 Sep 2026 15:33:05 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: c9e37e518febda70d0317d07e8ceb35ac43c534c}

[lskramarov]

/code-review max — checkbox review (#1984)

15 findings, most severe first. Most sit in the new checkbox-signals schematic, which writes consumer files unattended, so its failure modes carry further than the component change. Every claim below was checked against the source; the schematic behaviours were reproduced by running its own functions.

The component change itself is sound — I traced nextUniqueId→_IdGenerator, get inputId()→computed, the field-initialisation order (uniqueId at :88 precedes id at :97), writeValue/setDisabledState routing, and the clickAction token default, and they are behaviour-preserving. The [id]="null" host fallback and the valueless-attribute fix are real fixes. The decision to keep checked/disabled/indeterminate/tabIndex as accessor inputs checks out: ModelOptions in the installed @angular/core has no transform, and KbqButtonToggle (button-toggle.component.ts:458) really does use the same shape.

Highest impact: the migration never applies its fixes when invoked the way it is meant to be (inline comment on index.ts:394).

One finding that has no diff line to attach to

packages/components/checkbox/checkbox.ts:349 — onTouched fires on focus gain, not on blur.

private onInputFocusChange(focusOrigin: FocusOrigin) {
    if (focusOrigin) {
        this.checkable.onTouched();
    }
}

FocusMonitor emits the origin on focus and null on blur, so the ControlValueAccessor marks the control touched on the wrong edge. With <kbq-checkbox required [(ngModel)]="agreed"> and a touched-gated message, the "required" error renders the moment the user tabs onto the control, before any interaction. radio.component.ts:495 uses if (!focusOrigin && this.radioGroup) and Angular Material's checkbox calls _onTouched() only when focusOrigin is null, so this is checkbox-local. The line is unchanged by the diff and predates it — raising it because this PR is billed as a full review of the component, and nothing in the spec covers the touched transition.

Also noted, below the cut

• data.ts:31 WRITABLE_MEMBERS is an empty Set by construction, so the .set(...) branch it guards at index.ts:157-162 can never execute (~8 dead lines, inherited from alert-signals).
• checkbox.ts:71-72 — [id] and [attr.id] now always write the same non-empty string. id is a reflected IDL attribute, and hostId() can no longer be null, so the pair's only reason to exist is gone; [attr.id] alone is behaviour-preserving.
• index.ts:216-230, 356 — three ts.createSourceFile calls and two identical collectReceivers walks over the same string per consumer file (269 ms vs 90 ms across this repo's 216 consumers). The two collectReceivers call sites also have to be kept in lockstep by hand.
• checkbox.component.spec.ts:883-912 — both tests in describe('generated id fallback') inline the same four setup lines instead of using a beforeEach like describe('valueless attributes') two blocks up, and should report false for an unbound required asserts required() and value() — neither about id fallback.
• checkbox.component.spec.ts:462 — the clickAction override test dropped its runtime-change leg, and CheckboxWithClickAction.clickAction is a signal() nothing ever writes. The behaviour this PR actually changed is that onInputClick reads this.clickAction() at click time; regressing that to a captured value would leave the suite green.
• checkbox-required-validator.ts:19 — '[attr.required]': 'required ? "" : null' reads the raw input, while Angular's own RequiredValidator uses the booleanAttribute-normalised _enabled. For <kbq-checkbox required [(ngModel)]> the raw value is '', so the subclass's host binding (which runs last) removes the required attribute from the host at the exact moment validation is enforced. Untouched by this PR and no CSS depends on it today; noting it as part of the package review.
• checkbox.html:1 — #label is never read (no viewChild('label'); the specs use querySelector). The PR edits that line.

🤖 Generated with Claude Code

[lskramarov]

The migration never writes anything under ng update.

fix is destructured straight out of options, but no entry in migrations.json declares a schema (0 of 22), and the CLI invokes package migrations with no options at all — executeSchematic(workflow, migration.collection.name, migration.name) → executeSchematic(..., options = {}) (@angular/cli/src/commands/update/cli.js:329 → :233). So schema.json's "fix": { "default": true } never reaches the rule, fix is undefined, and commit() always takes the dry-run branch.

An app running ng update @koobiq/components@20 gets [checkbox-signals] would update N file(s) (run with --fix to apply), no rewrites, and a build that then fails on id being an InputSignal — while README.md:3 says the schematic is "invoked automatically by ng update".

button-toggle-signals-and-aria/index.ts:598 guards exactly this, with a comment naming the cause; 12 of the 17 migrations that read fix use that form. index.spec.ts:254 covers fix: false but never the no-options default.

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]

The template rewrite is an unanchored regex over raw file text — it over-matches and under-matches, silently in both directions.

The AST is used only to discover ref names; the rewrite itself is String.replace over the whole template. I ran this exact function:

Over-matching — \b matches after a dot, so the ref does not have to be the root of the expression:

<kbq-checkbox #checkbox />
{{ form.checkbox.value }}   →   {{ form.checkbox.value() }}

A FormGroup control named checkbox is about the likeliest collision for a checkbox ref, and it throws value is not a function at runtime. Also rewritten: prose (<p>Set checkbox.name in the config.</p>), HTML comments (<!-- checkbox.id -->), unrelated attribute values (alt="checkbox.value"), @for / ng-template let- bindings that shadow the ref name, and (click)="cb.id = 'x'" → cb.id() = 'x' (an Angular template parse error — the TS path deliberately leaves writes alone, the template path corrupts them).

Under-matching — the pattern hardcodes \., so these come back unchanged with no warning:

{{ cb . id }}
<span [title]="cb
    .value"></span>
{{ cb?.value }}

The last one matters most: index.spec.ts:55 advertises that optional chaining is handled, but only on the TS side. A leftover {{ cb?.value }} does not throw — Angular interpolates the InputSignal function object, so the page silently renders function source.

button-toggle-signals-and-aria/index.ts:450 uses (\s*\.\s*) for exactly this reason.

[lskramarov]

A #ref="exportAs" on <kbq-checkbox> is mistaken for the component.

The collector records the ref name and never looks at attr.value, so any directive exported from the element is treated as a KbqCheckbox.

<kbq-checkbox #ctrl="ngModel" [(ngModel)]="agree" required>I agree</kbq-checkbox>
@if (ctrl.invalid && ctrl.touched) { <span>Required</span> }
<p>{{ ctrl.value }} / {{ ctrl.name }}</p>

ctrl is the NgModel, which really has name (@Input() name: string) and value (AbstractControlDirective.get value()) — both in SIGNAL_MEMBERS (data.ts:17-25). Running rewriteRefReads on that template gives {{ ctrl.value() }} / {{ ctrl.name() }}.

#ctrl="ngModel" is the standard template-driven-forms idiom, the schematic writes the file, and the consumer's build then fails on ctrl.value not being callable — with no warning, since neither PROTECTED_MEMBERS nor warnPatterns ever run over templates.

Only collect a ref whose attribute value is empty or kbqCheckbox.

[lskramarov]

Receiver scope is the whole enclosing function (or the whole file), with no innermost-declaration resolution.

findAncestor(node, isFunctionLike) widens a block-scoped declaration to its function, ?? sourceFile widens a module-level one to the entire file, and inReceiverScope (line 139) then matches purely on expression text. Verified by running migrateTsExpressions:

function f(flag: boolean) {
    if (flag) {
        const checkbox: KbqCheckbox = a;
        checkbox.value;              // → checkbox.value()   ✓
    } else {
        const checkbox: HTMLInputElement = b;
        checkbox.value;              // → checkbox.value()   ✗
        checkbox.name;               // → checkbox.name()    ✗
    }
}

The file-wide case is worse: one module-level const checkbox: KbqCheckbox makes an unrelated handle(checkbox: HTMLInputElement) { return checkbox.value + checkbox.id; } elsewhere in the file become checkbox.value() + checkbox.id(). value, name and id are exactly the HTMLInputElement members an Angular app touches most.

button-toggle-signals-and-aria/index.ts:242 (resolveDeclaration) collects every declaration and picks the latest-starting one specifically to avoid this.

[lskramarov]

Compound, logical and increment assignments fall through to the read branch and emit invalid TypeScript.

The write guard above only matches SyntaxKind.EqualsToken. Verified by running the pass:

checkbox.id += '-suffix';        // → checkbox.id() += '-suffix';
checkbox.clickAction ??= 'noop'; // → checkbox.clickAction() ??= 'noop';

Both are TS2364 (invalid assignment target) — a grammar-level failure in a file the schematic just rewrote and reported as fixed, rather than the clean "input is read-only" error the comment on lines 150-151 promises. ++ / -- behave the same way (they are not BinaryExpressions at all).

This is squarely the at-risk shape: the README's own premise is that id and clickAction are the two inputs the automated migration skipped because application code writes to them. No warning fires either — warnPatterns[0]'s =[^=] does not match +=.

Treating any assignment-target access (not just =) as a write and returning would restore the intended behaviour.

[lskramarov]

The reworded intro contradicts its own subsection list.

Section 18's subsections are Checkbox (1052), Search expandable (1075), Split button (1097) and Title (1119). The sentence lists search-expandable, split-button and title in the first wave, then says "the second is the one each subsection below belongs to" — asserting three of the four subsections are in both waves.

The pre-PR sentence was consistent; the minimal correct edit was adding checkbox to the list. Same defect in migration.ru.md:1042.

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.

Eleven components went through a full review in 20.3.0: checkbox, 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.

[lskramarov]

This pattern matches the decorator form too, and warns about code the migration itself just fixed correctly.

@ViewChild(KbqCheckbox) checkbox: KbqCheckbox; matches (?:viewChild|ViewChild|…)[^\n;]*\bKbqCheckbox\b. But a decorator query holds the instance, so this.checkbox.id → this.checkbox.id() is right — and collectReceivers already performs that rewrite; index.spec.ts:73 pins it.

The consumer then gets, about that same file: "reading one is a double call, e.g. this.checkbox().id(). Verify query reads manually." — an invitation to break working code.

Meanwhile the genuinely unhandled case, readonly checkbox = viewChild(KbqCheckbox) (whose reads are not rewritten, since the receiver is a signal), gets only this same generic line.

Restrict the pattern to the lowercase signal-query form, or resolve the query kind from the AST the pass already builds.

[lskramarov]

This test never checks that a warning was emitted, despite its name.

The only assertion is expect((await run()).readText(ts)).toContain("checkbox.id = 'custom';") — the messages array collected in beforeEach is used by the three neighbouring warning tests (lines 186, 206, 243) but not here. Deleting warnPatterns[0] from data.ts entirely leaves this green.

That is how the pattern's real defects shipped uncovered. '\\.\\s*(?:id|clickAction)\\s*=[^=]' (data.ts:69) is anchored only by \bKbqCheckbox\b appearing somewhere in the file, so:

• function f(el: HTMLElement) { el.id = 'x'; } in a file that names KbqCheckbox → warns (false positive; .id = is one of the commonest assignments in an Angular app);
• a genuine c.id = 'x' as the file's last characters → silent, because the trailing [^=] requires a following character.

Suggested change

	expect((await run()).readText(ts)).toContain("checkbox.id = 'custom';");
	expect((await run()).readText(ts)).toContain("checkbox.id = 'custom';");
	expect(messages.join('\n')).toContain('read-only signal inputs');

[lskramarov]

node_modules is walked in full before being filtered out.

The filter runs inside the visitor, so HostDirEntry.getSubfilesRecursively has already recursed through every directory and allocated an entry for every file — there is no pruning hook, so the return saves an array push and nothing else. Measured on this repo:

shape	entries visited	.ts collected	wall clock
as written	118,975	11,858	29,913 ms
prune node_modules/.git/dist at the directory level	24,133	11,858	2,568 ms

68% of the entries (80,423 files across 9,747 dirs) are node_modules, discarded one at a time after the walk paid for them.

The multiplier is the real point: checkbox-signals is registered at 20.3.0-0 alongside 17 other migrations, and 22 of the 24 migrations here use this same post-hoc-filter shape — one ng update repeats the walk ~22 times. The fix belongs in a shared scan helper in packages/schematics/src/utils/ rather than this file alone, but this is the moment to stop cloning it.

(Absolute times are Windows stat costs; the entry counts are platform-independent.)

[lskramarov]

Byte-for-byte copy of a helper this file already imports from.

Line 7 is import { forEachClass, parseTemplate } from '../../utils/typescript'; — and that module exports collectInlineTemplateRanges at utils/typescript.ts:74. The bodies are identical bar the export keyword (including the // +1 / -1 to exclude the opening/closing quote characters. comment); forEachClass is imported only to feed this local copy.

The shared helper was extracted in d91bbd9 (#1788), and every migration authored since imports it — app-switcher-signals, button-state-and-styles, button-supported-colors, button-toggle-signals-and-aria, dropdown-demote-overlay, list-tree-multiple-input, navbar-signals-and-aria. The remaining local copies all predate the extraction; this one came along by copying alert-signals.

Deleting lines 316-354 and adding collectInlineTemplateRanges to the line-7 import removes 35 lines. The shared version also documents a rule the copy carries silently ("A template literal with a ${…} substitution is skipped"), and a future fix there would miss this migration alone.