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

[artembelik]

What

A full review of splitter, in the same shape as the 20.3.0 component reviews. All thirteen inputs on the component and its gutter were accessors with coercion in the setter, which is why the automated signal migration skipped every one of them.

KbqGutterGhostDirective declared five inputs it never accepted

@if (useGhost()) {
    <kbq-gutter-ghost />
}

No bindings — the splitter sets ghost.x, ghost.y, ghost.size, ghost.direction and ghost.visible imperatively during a drag, from a mousemove handler running outside the Angular zone. Making them signal inputs would have broken exactly that. They are plain properties now, and the directive is @docs-private: it is a rendering detail of the splitter, not something a consumer places.

Three behavior fixes

• gutterSize fallback. this._gutterSize = size > 0 ? size : this.gutterSize read its own getter, so an invalid value silently preserved whatever was there before. A non-positive value falls back to the default 6 now, via the input's transform.
• The gutter lays itself out reactively. It did so once in ngOnInit, so [order] and [direction] changes after init never reached the DOM — and the gutters live in an @for with track area, so they keep their instances across reorders. Moving it to an effect surfaced a latent bug the old code could not hit: switching direction left a stale width or height behind. The effect clears the dimension the other direction owns.
• An area unsubscribes from gutterPositionChange on destroy. ngAfterViewInit subscribed and nothing ever unsubscribed, so an area removed from a long-lived splitter kept emitting sizeChange for every later drag.

resizing was dead

get resizing(): boolean { return this._resizing; }
private _resizing: boolean = false;

_resizing is never assigned anywhere, so splitter.resizing always reported false. Removed; isDragging() is the real thing.

Closed internals

elementRef, changeDetectorRef, areas, areaRefs, gutters and ghost are private or protected. addArea / removeArea / setSize / getSize / getMinSize / getPosition / setOrder / disableFlex stay public but @docs-private: they are how the area and the splitter talk to each other.

Migration

splitter-signals runs from ng update @koobiq/components@20. It rewrites reads on receivers typed KbqSplitterComponent or KbqGutterDirective, rewrites gutter.dragged = … to .set(…), and reports the rest.

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

Testing

• splitter.spec.ts: 8 → 11 tests. New coverage for the gutterSize fallback, the valueless disabled/hideGutters/useGhost attributes, and a direction change after init — that last one is the test that caught the stale-dimension bug.
• The dynamic-data test reached into the private elementRef through as any; it reads the rendered style.order off the DOM now.
• splitter-signals/index.spec.ts: 16 tests, covering both receiver types and the dragged write.
• Full packages/components (4996 tests) and packages/schematics (450 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 f32d0b2):

https://koobiq-next--prs-1986-ix0zngmj.web.app

_{(expires Sat, 05 Sep 2026 16:10:25 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: c9e37e518febda70d0317d07e8ceb35ac43c534c}

[lskramarov]

Code review — max effort

Ten finder angles plus a gap sweep over the splitter conversion, the splitter-signals schematic and the migration docs. 15 findings, most severe first, as inline comments.

The two that matter most:

1. The schematic is a no-op under ng update — fix is destructured with no ?? true, and migrations.json carries no schema, so the CLI's empty options leave it undefined. Ten sibling migrations guard this with an explicit comment.
2. A runtime direction change is now half applied — the host and gutters became reactive, the areas did not, so they keep the previous axis' inline sizes (including a pixel width left by an earlier drag).

Also worth a look before merge: the booleanAttribute note in the migration guide describes a change that never happened (coerceBooleanProperty('') was already true), and both new regression tests pass unchanged against the base commit.

🤖 Generated with Claude Code

[lskramarov]

fix has no default, so ng update rewrites nothing.

migrations.json declares no schema for splitter-signals (no entry in that file does), and the CLI invokes update migrations with no options at all — executeSchematic(workflow, migration.collection.name, migration.name) in @angular/cli/src/commands/update/cli.js, whose options parameter defaults to {}. So the "default": true in schema.json only reaches the ng generate path; under ng update options.fix is undefined, commit() takes the else branch for every file, and the run prints would update N file(s) (run with --fix to apply) while writing nothing.

That contradicts migration.en.md:1041 ("Every schematic named below runs automatically") and :1118 ("the reads and the dragged write are rewritten").

Ten sibling migrations carry the guard and the comment explaining it — see navbar-signals-and-aria/index.ts:594, button-state-and-styles/index.ts:212, app-switcher-signals/index.ts:411. Those also declare fix?: boolean in schema.ts; here it is fix: boolean (non-optional), which is why the compiler does not flag the missing 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]

A runtime direction change is now only half applied — the areas never re-run.

The PR made the host (effect at :288) and the gutters (effect at :114) follow direction, but KbqSplitterAreaDirective still writes its axis-dependent dimensions exactly once here, in ngAfterViewInit, outside any reactive context.

Trace: splitter starts Horizontal → each area gets height: 100% and width removed. A drag calls setSize(), which routes through getSizeProperty() → Width, so an area now also carries e.g. width: 412px, and disableFlex() has removed flex. The consumer then flips [direction] to Vertical: the host becomes flex-direction: column and the gutters flip, but no area ever runs removeStyle(Width) again. Every area stays pinned to 412px wide inside a column splitter (an explicit width beats align-items: stretch), and keeps the height: 100% that now sits on the main axis.

Before this PR nothing followed direction, so the flip was a consistent no-op. Now half the layout follows it.

The new reactive layout spec cannot catch this: checkDirection asserts only splitter.nativeElement.style.flexDirection and the gutters' width/height, never an area. An effect() in the area mirroring the gutter's would close it.

[lskramarov]

Receivers created with inject() are invisible to the migration — and the un-migrated read is silently truthy.

collectReceivers matches only a bare TypeReferenceNode annotation, so the repo's own idiomatic shape is missed:

private readonly splitter = inject(KbqSplitterComponent);   // inferred type — no annotation
…
if (this.splitter.disabled) return;

Nothing is rewritten and nothing is warned (warnReceiverMembers returns early on receivers.length === 0). After the update this.splitter.disabled is an InputSignalWithTransform — a function object, so if (…) is always true and TypeScript accepts it. The guard silently inverts with a green build.

Also missed, silently: splitter: KbqSplitterComponent | undefined (UnionTypeNode), KbqSplitterComponent[] / QueryList<KbqSplitterComponent>, and aliased/namespaced imports (import { KbqSplitterComponent as Sp }, kbq.KbqSplitterComponent). The viewChild() shape at least gets a warnPatterns entry; these get nothing.

Worth extending isTypeReference to unwrap unions/arrays, and adding a warn pattern for inject(KbqSplitterComponent) / inject(KbqGutterDirective).

[lskramarov]

Compound assignments fall through to the read branch and produce code that does not parse.

The three guards above match a call, a .set access, and a BinaryExpression whose operator is exactly EqualsToken. Everything else reaches this line and gets () appended to an assignment target: +=, -=, ||=, &&=, ??=, ++/-- (PostfixUnaryExpression / PrefixUnaryExpression), and delete.

// before — valid today, gutterSize was a public accessor pair
splitter.gutterSize += 2;
splitter.gutterSize++;
gutter.dragged ||= isDown;

// after --fix
splitter.gutterSize() += 2;   // TS1005 / TS2364
splitter.gutterSize()++;      // TS1005
gutter.dragged() ||= isDown;  // TS1005

A localised type error becomes a file that no longer parses. gutter.dragged = !gutter.dragged is handled correctly, so this is only the non-= operators. The safest fix is to skip any access whose parent is an assignment-like expression and report it instead — the members here (gutterSize, size, order, dragged) are exactly the numeric/boolean ones people write += and ||= on.

[lskramarov]

The ref rewrite is a flat regex over the whole raw template, and \b matches right after a dot.

The ref is discovered through the parsed AST, but applied with String.replace over the entire template text — no binding/interpolation scoping, no embedded-view scoping, no left-context guard. So <anything>.<ref>.<member> matches too:

<!-- before -->
<kbq-splitter #layout [direction]="dir">…</kbq-splitter>
<span>{{ settings.layout.direction }}</span>
<p>Set layout.size in the config</p>
<!-- layout.direction defaults to horizontal -->

<!-- after --fix -->
<span>{{ settings.layout.direction() }}</span>   <!-- runtime "not a function" -->
<p>Set layout.size() in the config</p>            <!-- rendered text corrupted -->
<!-- layout.direction() defaults to horizontal -->

This shape is inherited from alert-signals, but there the member list is compact/alertStyle. Here it is ten names, six of which are ordinary view-model words: direction, size, order, disabled, dragged, isVertical. Anchoring the rewrite to the parsed BoundAttribute/BoundText source spans instead of the raw string would remove the whole class of false positives.

[lskramarov]

order's fallback for an invalid value silently changed from 0 to NaN.

coerceNumberProperty(value)  // fallbackValue = 0
numberAttribute(value)       // fallbackValue = NaN

Everywhere else in this file the transform carries an explicit fallback — gutterSizeAttribute uses DEFAULT_GUTTER_SIZE, setSize uses numberAttribute(size, 0) — so order is the one that lost it. KbqGutterDirective is still in KbqSplitterModule.exports, so <kbq-gutter [order]="fromConfig"> with a non-numeric value now yields order() === NaN, writes the invalid declaration order: NaN (dropped by the parser, so the flex order falls back to the stylesheet default instead of the explicit 0 the old setter wrote), and makes this.gutters().find((gutter) => gutter.order() === gutterOrder) return undefined — a useGhost drag then silently falls back to live resizing.

Suggested change

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

Related and also undocumented: the gutter's own size (and the ghost's setter at :207) moved from coerceNumberProperty (fallback 0, no clamp) to gutterSizeAttribute (fallback 6, clamps <= 0 to 6), so <kbq-gutter [size]="0"> used to collapse the divider to zero thickness and now renders 6px. The migration note at migration.en.md:1112 covers KbqSplitterComponent.gutterSize only.

[lskramarov]

This doc says the opposite of what the template does.

The template renders [style.display]="hideGutters() ? 'none' : 'flex'" on <kbq-gutter>, and that element's (mousedown) is the only drag entry point. With display: none the gutter is not hit-tested, so it receives no mousedown and the areas are not resizable while gutters are hidden.

A consumer reading "The areas stay resizable" sets hideGutters expecting an invisible-but-draggable divider and ships a splitter nobody can resize.

Suggested change

	/** Whether the gutters are hidden. The areas stay resizable. */
	/** Whether the gutters are hidden. A hidden gutter is not draggable either. */

(If an invisible-but-draggable divider is the intent, visibility: hidden / opacity: 0 keeps hit-testing where display: none does not — but then the note should say so.)

[lskramarov]

Both new regression tests pass unchanged against the pre-PR component, so neither guards the behaviour it names.

gutterSize — the sequence is 0 → 12. The old setter was this._gutterSize = size > 0 ? size : this.gutterSize; with _gutterSize = 6, so 0 also yielded 6 (the initializer) and 12 yielded 12. The differentiating sequence is 12 → 0: old keeps 12, new falls back to 6. That is the one the test does not exercise.

valueless attributes — asserts true, which coerceBooleanProperty('') already returned (see the note on migration.en.md:1110).

Revert gutterSizeAttribute to the old semantics and booleanAttribute to coerceBooleanProperty, and both new tests still go green.

Two smaller things in this file:

• The it name says "should fall back to the default for a non-positive size", but the second half asserts that a valid 12 is applied — a failure there reports a fallback bug that isn't one.
• Line 14 still reads // Mirrors the directive defaults in splitter.component.ts (KbqGutterDirective._size, KbqSplitterComponent._gutterSize). — both fields were deleted by this PR in favour of DEFAULT_GUTTER_SIZE. AGENTS.md: "Update or remove comments that become outdated because of your changes."

[lskramarov]

Leftovers from the blind KbqAlert → KbqSplitterComponent rename of alert-signals.

/\bKbqSplitterComponent\w*\b/ matches neither KbqSplitterModule, nor KbqSplitterAreaDirective, nor KbqGutterDirective/KbqGutterGhostDirective — the \w* suffix was doing real work in the original (KbqAlert\w* catches KbqAlertModule, KbqAlertColor), and here it catches nothing extra. The JSDoc above claims the gate covers "any of the exported symbols", and names KbqSplitterComponentModule, which does not exist (the module is KbqSplitterModule). In practice the SPLITTER_PACKAGE check masks most of it, but a file whose only reference is a KbqGutterDirective-typed receiver imported through an app-local barrel is skipped with no rewrite and no warning — even though RECEIVER_TYPES claims to cover that type.

Suggested change

	/\bKbqSplitterComponent\w*\b/.test(content) ||
	/\bKbq(?:Splitter|Gutter)\w*\b/.test(content) ||

Same rename residue elsewhere in the file, worth a sweep:

• :167 — // Read (incl. optional chain \x?.compact`). compactis an alert input; it is not inSIGNAL_MEMBERS` and not a member of any splitter type.
• :150 — "Every KbqSplitterComponent signal member is input() (read-only), so there is no writable member", directly above the WRITABLE_MEMBERS branch that rewrites dragged.
• :382 — KbqSplitterComponentModule.
• data.ts:38 — SPLITTER_TYPE is exported and imported nowhere (RECEIVER_TYPES[0] is the same literal).
• data.ts:76 — anchor and pattern are the identical string, so the two-stage filter data.ts:66 documents ("only evaluated for files that also name it") can never reject anything; entry 2's anchor is likewise a substring of its own pattern.

[lskramarov]

collectInlineTemplateRanges is already exported from the module this file imports two symbols from.

packages/schematics/src/utils/typescript.ts:74 exports an identical implementation — the local copy differs only by the missing export keyword, and its JSDoc drops the two caveats the shared one documents (a template in a comment or another object; a template literal with a ${…} substitution) while downgrading the correct half-open [start, end) to [start, end].

Seven migrations import 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 copies in alert-signals/form-field-signals predate the extraction, so this PR adds a new one after the util existed. Any future fix to inline-template discovery lands in the util and silently skips this migration.

Line 7 already reads import { forEachClass, parseTemplate } from '../../utils/typescript'; — adding collectInlineTemplateRanges there and deleting lines 318-353 also drops the then-unused forEachClass.

[lskramarov]

Addendum — gap sweep

A final gap pass finished after the review above. Five more findings, all verified against the head commit; none overlaps the fifteen already posted.

The one worth acting on first: the PR's single behavioural fix — the gutterPositionChange teardown — has no test. Deleting the ?.unsubscribe() leaves the whole suite green.

🤖 Generated with Claude Code

[lskramarov]

The one behavioural bug fix in this PR has no test.

The guide and the schematic SUMMARY both headline it: "A splitter area unsubscribes from gutterPositionChange when it is destroyed. An area removed from a long-lived splitter used to keep emitting sizeChange for every later drag."

Three tests were added (gutterSize, valueless attributes, reactive layout) and none covers this one. splitter.spec.ts contains no reference to unsubscribe, destroy or ngOnDestroy at all, and the pre-existing dynamic data test removes areaA but only asserts orderOf('areaB'). Delete the ?.unsubscribe() at :555 and the whole suite still goes green.

The regression it guards against is silent — a destroyed area keeps emitting into consumer (sizeChange) handlers. Worth a test that removes an area, dispatches a second drag, and asserts the removed area's handler was not called again.

[lskramarov]

This rationale is the opposite of what the code does.

The template is @for (area of areas; track area) — identity tracking on the IArea object. ngAfterContentInit reassigns this.areas = data.map(this.mapAndOrderArea), and mapAndOrderArea (:394) returns a fresh object literal every call. So on every areaRefs.changes emission every track key changes, and the gutters are destroyed and recreated — they do not keep their instance across reorders.

The effect is still the right call, but for the reason the new reactive layout test actually exercises: a runtime direction / gutterSize change, which ngOnInit could never pick up. A reader who trusts this comment will conclude the effect becomes unnecessary once tracking is fixed, and revert it.

Suggested change

	// The gutters are rendered inside an `@for` and keep their instance across reorders, so the layout
	// has to follow the inputs rather than run once on init.
	// `ngOnInit` ran once, so a `direction` or `size` change after init never reached the layout.

[lskramarov]

AGENTS.md asks for a narrow token here, and this line was touched by the PR (private → private readonly).

When a projected child depends on its host component, inject a narrow InjectionToken scoped to the members it uses, not the host's concrete class.
— AGENTS.md:168

KbqSplitterAreaDirective is projected into <kbq-splitter> and uses exactly five members: isVertical, isDragging, addArea, removeArea, gutterPositionChange. Injecting the concrete class is also what forces @ContentChildren(forwardRef(() => KbqSplitterAreaDirective)) at :250, and what keeps addArea/removeArea/isVertical public when the rest of the internals were closed.

Not something to bolt on late, but a review that reworked the whole public surface was the moment for it — flagging so it is a deliberate deferral rather than an oversight.

[lskramarov]

addArea and removeArea stayed public while every sibling internal was closed.

The review closed elementRef, changeDetectorRef, areas, areaRefs, gutters, ghost, onMouseDown and isResizing, but the two mutators that write this.areas are still // @public in tools/public_api_guard/components/splitter.api.md:85,100 — marked only @docs-private, which the api guard does not strip.

They cannot simply become protected: KbqSplitterAreaDirective calls both across the injection boundary, which is the same coupling as the note on :521. But as it stands an outside caller can do splitter.addArea(someArea), pushing a duplicate IArea that shifts every gutter's order and desynchronises the leftAreaIndex * 2 + 1 lookup in onMouseDown. Neither the guide nor the schematic mentions them either way, so consumers get no signal about whether they are contract.

[lskramarov]

The ref collector never reads the ref's exportAs value, so a ref that exports a different directive on the same element is treated as the splitter.

attr.value is not inspected anywhere in visitElement — any #name on <kbq-splitter> is added to refs, and rewriteRefReads then appends () to its SIGNAL_MEMBERS reads.

<kbq-splitter #d="kbqDropdownTrigger" [kbqDropdownTriggerFor]="menu">…</kbq-splitter>
{{ d.disabled }}      <!-- the trigger's disabled, not the splitter's -->

becomes d.disabled() — a template that compiled before now fails, or calls something that is not a function. disabled, direction and size are common enough member names elsewhere for this to be reachable.

Distinct from the note on :289: that one is the regex misapplying a correct ref; this is the collector producing a wrong one. Guarding on attr.value === '' || attr.value === 'kbqSplitter' (the component's exportAs) closes it.