fix(button-toggle)!: prevent NG0100 from an unresolved initial group value (#DS-2953)

[KamilEmeleev]

Summary

KbqButtonToggleGroup derived value from the selection alone, so a value assigned before its
toggles existed came back out as an empty selection. A [(value)] binding wrote that undefined
into the consumer's field, and a binding declared above the group in the same change detection pass
produced NG0100.

The assigned value is now kept until a toggle takes it — the contract KbqRadioGroup already
documents. A toggle rendered later behind @if picks it up; a user interaction replaces it.

Breaking changes

Documented in migration.{en,ru}.md § 11.

• A value matching no toggle is kept instead of dropped, so value can name a toggle selected
  does not hold.
• valueChange no longer echoes an assignment — it fires only when the value actually changes.
• ngAfterContentInit is gone from KbqButtonToggleGroup; its body could not change anything.

List of notable changes

• fixed a value assigned before the toggles render being reported as an empty selection —
  including toggles behind @if, and multiple with an unset [(value)]
• fixed phantom valueChange(undefined) emissions, and multiple mode handing the consumer
  back their own array
• fixed a remounted toggle re-checking a value the user had already moved off
• added 12 regression tests; 11 fail against main's component
• updated accordion-states and accordion-sections examples back to [(value)]

What should reviewers focus on?

• The persistence contract — a value naming no toggle now survives for the group's lifetime.
  That is what makes the @if case work, since the group cannot tell "not rendered yet" from
  "never will be", but it is the breaking bit.
• ngAfterContentInit removal — the only change visible in the public API guard.

[github-actions]

Visit the preview URL for this PR (updated for commit 2f34152):

https://koobiq-next--prs-1913-fg6lanb4.web.app

_{(expires Sat, 05 Sep 2026 17:42:55 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: c9e37e518febda70d0317d07e8ceb35ac43c534c}

[Copilot]

Pull request overview

This PR addresses an Angular NG0100 (“expression changed after it was checked”) scenario caused by KbqButtonToggleGroup temporarily reporting an empty selection when a value is assigned before its toggles have initialized. It updates the group to report the assigned value until the toggles have been matched against it, adds regression tests for the problematic template ordering, and updates accordion docs examples to avoid reading state back from the group during the same change detection pass.

Changes:

• Update KbqButtonToggleGroup to expose the assigned (raw) value until initial toggle matching has occurred.
• Add regression tests covering “value read before group is bound” for single, multiple, and unmatched-value cases.
• Update accordion examples to hold state in component fields (signals) and update via (change) rather than reading from a template ref.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
packages/docs-examples/components/accordion/accordion-states/accordion-states-example.ts	Adds a variant signal to store accordion variant state locally.
packages/docs-examples/components/accordion/accordion-states/accordion-states-example.html	Uses the variant() signal for [variant] and updates it via (change).
packages/docs-examples/components/accordion/accordion-sections/accordion-sections-example.ts	Adds a type signal to store accordion type state locally.
packages/docs-examples/components/accordion/accordion-sections/accordion-sections-example.html	Uses the type() signal for [type] and updates it via (change).
packages/components/button-toggle/button-toggle.component.ts	Introduces selectionResolved + rawValue signal and updates currentValue behavior pre-content-init.
packages/components/button-toggle/button-toggle.component.spec.ts	Adds regression tests for templates that read group.value before the group’s [value]/[(value)] binding runs.

---

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

[lskramarov]

Review — fix(button-toggle): prevent NG0100 from an unresolved initial group value

I compiled origin/main's KbqButtonToggleGroup side by side with this branch's and ran the same fixtures against both, so the regression / pre-existing split below is measured rather than reasoned. Three results shaped the ranking:

• One behavioural regression. A toggle that selects itself through [checked] used to reach a [(value)] binding; it no longer does. origin/main emits 2 (multiple: [1]), this branch emits undefined (multiple: []). This is the library's own documented pattern — packages/components/button-toggle/e2e.ts:66, button-toggle-overview-example.html:3,20,29, button-toggle-alignment-overview-example.html:12,21. (Corroborates the existing Copilot thread on line 178.)
• The latch has a blind spot. A value assigned after content init while the toggles have not rendered still reproduces NG0100: … Previous value: 'green'. Current value: 'undefined' and still wipes the [(value)] field. multiple [(value)] with an unset initial value throws too (Previous value: 'undefined'. Current value: '[]') — on main as well, but it is the case the PR title names.
• Two of the four new tests pass against the unfixed component. should preserve a two-way bound value and should stabilize an array value in multiple selection mode both go green on origin/main. Only the valueChange-recording test and the unmatched-value test actually fail without the production change.

15 comments below, most severe first: 7 correctness, 2 test-coverage, 1 test-quality, and one each of altitude, simplification, efficiency, reuse and conventions. Two of them (currentSelection, clearSelection) sit just outside the diff hunks, so they are anchored to the nearest changed line and say so.

yarn run unit:components on button-toggle is green (164 tests), and prettier/eslint are clean on all six changed files — nothing here is a build failure.

🤖 Generated with Claude Code

[lskramarov]

Regression — a [checked] initial selection is now emitted as undefined.

syncButtonToggle ends with this.valueChange.emit(this.value) (line 332), and a toggle's checked setter reaches it during the first update pass — before ngAfterContentInit. With this branch this.value falls through to rawValue(), which nothing has assigned, so the group announces an empty selection it does not have.

Verified by compiling origin/main's group alongside this one and running the same fixture against both:

<kbq-button-toggle-group [(value)]="v">
    <kbq-button-toggle [value]="1" [checked]="false">1</kbq-button-toggle>
    <kbq-button-toggle [value]="2" [checked]="true">2</kbq-button-toggle>
</kbq-button-toggle-group>

	valueChange emissions	group.value after init
origin/main	[2]	2
this branch	[undefined]	2

In multiple mode the same fixture goes from [[1]] to [[]].

v therefore stays undefined forever while the group reports 2, and the [value] binding slot recorded undefined, so nothing re-syncs it. [checked] inside a group is the library's own documented pattern — packages/components/button-toggle/e2e.ts:66, button-toggle-overview-example.html:3,20,29, button-toggle-alignment-overview-example.html:12,21.

This is the same hole Copilot flagged on line 178; the numbers above are the confirmation. Its suggestion — take the fallback only when rawValue() !== undefined — fixes this case, though it leaves the ones in the comments below.

[lskramarov]

The latch never re-arms, so the NG0100 comes back for toggles that render late.

selectionResolved is set once in ngAfterContentInit and nothing resets it when buttonToggles() changes. A group whose toggles live behind @if/@for has zero of them at content init, so the latch flips having matched nothing, and every later assignment goes back through the unguarded path.

Reproduced on this branch, with the toggles behind @if (show()) and show starting false:

fixture.detectChanges();
fixture.componentInstance.value = 'green';   // the saved value arrives before the option list
fixture.detectChanges();
// NG0100: ExpressionChangedAfterItHasBeenCheckedError.
// Previous value: 'green'. Current value: 'undefined'.

setSelectionByValue finds no matching toggle, valueChange.emit(this.value) emits undefined, and the two-way listener wipes the field — the exact defect this PR removes, one tick later. Async options and restored drafts are the common shape of this, and none of the three new fixtures covers it.

Deriving resolution from the query (or re-arming the latch when buttonToggles() changes) would close it; keeping the assigned value authoritative, as KbqRadioGroup does, would make the question moot.

[lskramarov]

Resolution changes what value reports but emits nothing, so an unmatched value diverges silently and forever.

This line is the moment value stops meaning what you assigned and starts meaning what is selected. No valueChange, no controlValueAccessorChangeFn, and the [value] binding sees no change — so setSelectionByValue is never re-entered and the two stores can never reconcile.

<kbq-button-toggle-group [(value)]="selected"> with selected = 'missing' (a stale id from a URL param or a restored form): nothing renders as selected, group.value is undefined, and selected still reads 'missing' for the component's lifetime — and gets submitted. Same for multiple with a partially matched array: the model keeps ['one','nope'] while the group reports ['one'].

Before this PR the model self-corrected to undefined — wrong-looking, but consistent, and loud. The new test enshrines the divergence as expected:

expect(fixture.componentInstance.value).toBe('missing');
expect(fixture.componentInstance.group().value).toBeUndefined();

If that trade-off is deliberate it needs saying out loud — a note on the value accessor's JSDoc and an entry in docs/guides/migration.{en,ru}.md § 11 (the still-open section for the button-toggle rework). Right now a consumer has no way to learn that their bound value was rejected. A one-shot emit at the latch transition, when the resolved value differs from the raw one, would keep both the NG0100 fix and the convergence.

[lskramarov]

rawValue is never refreshed by user interaction, so a toggle created later re-checks a stale assignment.

rawValue is written only by setSelectionByValue; clicks go through syncButtonToggle, which never touches it. With [(value)] the write-back happens to re-enter the setter and refresh it, but with [formControl]/[ngModel] nothing does.

Reproduced — a group on [formControl] holding 'red', with the 'red' toggle behind an @if:

user clicks green   -> control.value = 'green', group.value = 'green', rawValue still 'red'
show() false / true -> the red toggle's ngOnInit -> isPrechecked -> toggle.value === rawValue -> true
                    -> checked = true -> syncButtonToggle deselects green

after remount: control.value = "green"
               group.value   = "red"
               checked       = [["red", true], ["green", false]]

The DOM and the form model disagree permanently, and because isUserInput is false the form is never notified.

Pre-existing — but the diff rewrites all three isPrechecked lines and the rawValue JSDoc right above them, and that new JSDoc claims this is the case it covers: "Also covers the toggles being swapped out under an assigned value." It does not.

[lskramarov]

In multiple mode this hands the consumer their own array back, and reports entries that match no toggle.

The two neighbouring producers both copy — selected.map(...) on line 183, [...selected] on line 192. This one does not.

Verified with a multiple group bound [value]="values" where values = ['one','two'] but only a 'one' toggle exists:

valueChange emissions   = [["one","two"], ["one","two"]]
final group.value       = ["one"]
seen[0] === this.values -> true

Two consequences:

1. Phantom values. For the whole unresolved window the group announces a selection it never had. A (valueChange) handler that persists this stores a value for a toggle that does not exist.
2. Aliasing. A consumer who treats group.value as their own (group.value.push(x), .sort()) mutates the array feeding their own [value] binding — and since the reference never changes, the computed does not invalidate, bindingUpdated short-circuits, and no OnPush child input update fires.

The comment on line 170 ("a repeated read hands back the same array reference") is about memoization; it does not cover handing out somebody else's array. [...assigned] here would restore the ownership rule, and filtering it against buttonToggles() would remove the phantoms.

[lskramarov]

The statement the latch is appended to is a no-op, so selectionResolved does not mean what its JSDoc says.

KbqButtonToggle.checked → checkedState() → buttonToggleGroup.isSelected(this) → selectedToggles().includes(toggle) — i.e. exactly the model's current selection. The filter can only ever yield toggles that are already selected, so the select(...) and the publishSelection() after it cannot change anything. The real matching happened earlier, in each toggle's ngOnInit → isPrechecked → checked = true → syncButtonToggle (a statically checked toggle routes through the same setter).

So the flag reads "the content hook ran", not "Whether the toggles have been matched against the assigned value at least once" (line 167) — which is precisely why it flips with zero toggles present, and why the hole in the comment on line 168 exists.

Either way the invariant a reader is asked to trust is documented on a line that does nothing.

[lskramarov]

The comment contradicts the assertion under it, and new Set(...) erases what is being counted.

The comment reads as if undefined is expected in values; the line two below forbids it. A maintainer "fixing" the test to match the comment would delete the only assertion in this block that fails on the unfixed component.

expect(values.length).toBeGreaterThan(0) is fully subsumed — expect(new Set([])).toEqual(new Set(['green'])) already fails.

And the Set collapses duplicates: the group currently emits valueChange twice during init (once from the value setter, once from the pre-check inside the toggle's ngOnInit). Every one of those is a write-back through [(value)] and a re-run of consumer code, so the count is the interesting part — and a change that makes it five keeps this test green.

Replacing all three lines with the sequence assertion fails pre-fix (values is [undefined, 'green'] there) and pins the emission count:

expect(values).toEqual(['green', 'green']);

[lskramarov]

These examples now teach the workaround instead of the binding this PR just repaired.

[(value)]="type" is the one-binding equivalent — valueChange exists precisely "to facilitate two-way data binding" (component, lines 148-152), Angular writes two-way bindings straight into a WritableSignal, and this PR's own three fixtures all use [(value)]="value" to prove it is safe now. docs-examples already uses the short form on this very component (button-toggle-alignment-overview-example.html:1) and on kbq-select/kbq-timezone-select across ~10 examples.

Two things the replacement costs:

1. The rewrite is not required by the fix. I ran the pre-PR shape against this branch — <span [title]="toggle.value"> above <kbq-button-toggle-group #toggle [value]="'single'"> — and it no longer throws; the title settles to 'single'. So the examples that used to reproduce DS-2953 have been rewritten away, and with them the only place in the repo that exercised the fixed path.
2. (change) is strictly weaker than valueChange. It fires only from emitChangeEvent, i.e. only on user input. A selected toggle being destroyed (ngOnDestroy → syncButtonToggle(this, false)) or any programmatic write moves the group's value without firing it, leaving the signal stale — and then [value]="type()" re-asserts the stale value over the group's real state.

There is also churn per click: echoing the value back re-enters setSelectionByValue, which runs clearSelection() (unchecking the toggle the user just checked, emitting a spurious valueChange(undefined)) before selectValue() puts it back. Three extra emissions where the old [value]="'single'" seed was inert.

Same at accordion-states-example.html:42.

[lskramarov]

Efficiency — one assignment fans out 1 + N publishes and a phantom valueChange. (About clearSelection/selectValue, lines 381-398, just below this hunk.)

setSelectionByValue publishes the selection once from clearSelection (line 384), once more for every toggle whose checked = false re-enters syncButtonToggle, and once per matched value in selectValue (line 397). Each is an array copy plus a signal write that invalidates currentValue/currentSelection and marks the host view for traversal — and each syncButtonToggle also emits valueChange carrying the intermediate empty selection.

Post-init writeValue('green') on a group currently showing 'red' therefore emits valueChange(undefined) and then valueChange('green'): a (valueChange) subscriber sees a phantom clear, and a [(value)] model momentarily holds undefined. In multiple mode with three matched values the same single assignment writes selectedToggles five times.

A single publishSelection() at the end of setSelectionByValue collapses all of it — and would remove the phantom undefined emission along the way, which is the same defect class this PR is fixing.

[lskramarov]

AGENTS.md (TypeScript Best Practices): "Use protected for template bindings".

type() is referenced only from accordion-sections-example.html:2 and :42; there is no TypeScript reader. Same for readonly variant in accordion-states-example.ts:21.

Suggested change

	readonly type = signal<KbqAccordionType>('single');
	protected readonly type = signal<KbqAccordionType>('single');

Precedent in docs-examples is genuinely split (189 of 573 example files use protected, and the pre-existing accordionVariant field in the sibling file is public), so this is the written rule rather than local custom — but it is new code, and the newer examples follow it.