Ngp Code Review

ng-primitives code review

Use this skill when reviewing changes on a branch or PR in this repo. The goal is a structured, file-by-file review against project conventions — not a generic code review.

How to run the review

1. Scope the diff: git diff next...HEAD --stat, then git diff next...HEAD for content. (Main is next in this repo.) For a remote PR, use gh pr diff <num>.
2. For each changed file, walk the checklist below and cite specific file:line locations for any finding.
3. Cross-check against CONTRIBUTING.md and the PR template at .github/PULL_REQUEST_TEMPLATE.md (tests added, docs updated, issue linked, breaking-change disclosure).
4. If substantive code changed, suggest running pnpm lint and nx test <project> — don't run them silently; let the user decide.

Review scope rules

Review only the changed lines, not the whole file. If a file is touched, do not raise issues with surrounding code that the diff did not modify. Pre-existing issues are out of scope — e.g. if an existing getter elsewhere in the file should be a computed() but the diff didn't touch it, leave it alone. Use git diff next...HEAD -- <file> to keep your focus tight.

Verify "unused" / "unreferenced" claims before raising them. Symbols can be referenced where a plain grep won't show: directive attribute selectors used in templates (ngpButton), CSS classes in styles, DI tokens, downstream barrel exports, generator templates under packages/tools/. Before flagging something as unused, grep the selector, check templates (*.html and inline), check index.ts. Time-box this to a couple of greps per claim. If you can't confirm in a quick check, soften the finding to "possibly unused — please verify".

Filter speculative findings. Drop any finding whose justification only kicks in under hypothetical future changes ("if someone later adds X…", "if this describe block grows…", "if this primitive ever supports Y…"). Only raise issues that affect the code as it stands today — i.e. there is a concrete current call site, test, or usage that demonstrates the problem. If a future-proofing concern is genuinely important, reframe it as a concrete present-day risk or drop it. This rule supersedes anything else in the checklist.

Scale is not speculation. The one exception: §7 asks what the changed code costs per instance and per event. That is a property of the code as written, not a hypothetical future change — a headless library ships into pages its authors never see, so you may reason about cost × N without pointing at a call site that already does it. The escape hatch is bounded by §7's own "what not to flag" list; it does not license micro-optimisation findings.

Checklist

0. Scope of the change

Before reviewing details, judge whether the PR is appropriately scoped. A large diff is fine when the work genuinely requires it — a new complex primitive with state, sub-directives, tests, and docs is expected to be big. A large diff is not fine when it's an accident of process. Flag:

• Drive-by edits unrelated to the stated PR title (formatting churn in untouched files, unrelated renames, opportunistic refactors).
• Mass changes across many primitives bundled into one PR when each could ship independently — suggest splitting.
• New documentation that pads existing pages with AI-generated waffle: long pre-ambles, marketing copy, redundant restatements of what the code already shows, generic "best practices" boilerplate. Docs should be terse, concrete, and tied to actual primitive behaviour. If a docs change adds paragraphs without adding information, flag it for trimming.
• Speculative additions: extra inputs, options, or abstractions that aren't required by the issue being fixed.
• Test files balloon out of proportion to the behaviour change (e.g. dozens of describes for a one-line fix).

The bar: every line in the diff should be defensible against "is this needed for the stated change?" If not, suggest pulling it into a separate PR or dropping it.

1. Angular & signal APIs

Read .claude/rules/angular-patterns.md and flag any violation. Common ones in PRs: decorator inputs/outputs/queries (should be signal forms), non-readonly signals, @internal JSDoc on private members.

2. Naming & inputs

Read .claude/rules/naming-conventions.md and flag any violation. Common ones: missing ngp selector prefix, Directive/Component/Service class suffix, .directive.ts file names, inputs without ngp-prefixed aliases, number/boolean inputs without NumberInput/BooleanInput + coercion.

3. State pattern

Read .claude/rules/state-management.md. Every primitive and every part (container, input, toggle, item, option, trigger, …) uses the createPrimitive pattern — a <name>-state.ts that returns the 4-tuple and holds all host bindings inside the factory, with a thin directive that provides the state and delegates to it:

export const [NgpXStateToken, ngpX, injectXState, provideXState] = createPrimitive(
  'NgpX',
  (props: NgpXProps): NgpXState => {
    const element = injectElementRef();
    // attrBinding / dataBinding / listener all live HERE
    return {
      /* ... */
    } satisfies NgpXState;
  },
);

Flag as HIGH:

• New use of the legacy state pattern. Any added file — or any changed line in the diff — that calls createStateToken, createStateProvider, createStateInjector, or createState (from ng-primitives/state). These are the pre-createPrimitive primitives (search-era) and are deprecated for new code. Grep the diff: git diff next...HEAD | grep -nE '^\+.*\bcreateState(Token|Provider|Injector)?\b'. A hit on an added (+) line is a finding — the part must be rewritten with createPrimitive. Editing an existing legacy primitive in place does not require migrating it (don't force-migrate untouched primitives), but a new primitive or part using the quadruple is a HIGH finding.
• A part directive that inlines host bindings / listeners in the directive constructor instead of a -state.ts factory, or that omits provideXState() from its providers.
• A child part writing a parent's returned signal directly — state().x.set(...) from a sub-part. The parent must expose a setX/removeX (or register/unregister) function and keep the backing signal private; the child calls it, registering with onChange and deregistering with onDestroy. A part that registers but never deregisters (no onDestroy/removeX) is a HIGH finding when it drives an ARIA relationship (aria-labelledby/aria-describedby) — the stale id points at removed DOM. See dialog, roving-focus, select.

Flag as MEDIUM (convention, not lint-enforced):

• controlled on a value nothing mutates. controlled(input) / controlledState(...) is only for values the factory itself .set()s. Grep the factory (and any composed factory) for .set( on the wrapped signal; if nothing writes it, the input should pass straight through as a read-only Signal and the controlled wrapper dropped.
• Internal-only values exposed on the state. A computed/signal used solely for the factory's own bindings (not read by any other part or the directive) should be a local const, not returned on the NgpXState interface. Verify no other part reads it (grep injectXState) before flagging.

Specific rule violations (early state, missing generic, emitting state, reading raw input) are owned by §4.

4. Custom workspace lint rules

The following live under tools/eslint-rules/rules/ and run automatically via pnpm lint. This table is the single source of truth for rule names — call them out by name in the review so the user knows why something is flagged.

5. Tests

Tests live next to source as *.test.ts and use vitest + @testing-library/angular (render, screen, userEvent, waitFor, fireEvent). Flag:

• New behaviour or bug fix without a *.test.ts change — PR template requires tests.
• Inside dialog/overlay portals rooted on ApplicationRef, prefer fireEvent.input over userEvent.type to avoid NG0101 races.
• Tests that open dialogs/overlays without an afterEach that closes them and waitFors their DOM removal before destroying the fixture. Portals attach to root, not the fixture, so fixture.destroy() alone leaks DOM.
• Animation tests that rely on time instead of mocking Element.prototype.getAnimations.
• Two-way binding tests asserting on close ordering that use {escape} — Escape fires immediate: true in capture phase, which races; use userEvent.click(document.body) instead.
• Missing afterEach cleanup for overlays opened directly (e.g. document.querySelectorAll('[ngpTooltip]').forEach(el => el.remove())).

6. Accessibility

Accessibility is first-class per CONTRIBUTING.md. Flag:

• Interactive primitives missing ARIA roles, aria-* attributes, or tabindex.
• Controls not reachable by keyboard, or focus order broken after a state change.
• Focus-trap interactions that don't account for NgpOverlayRegistry — if a new overlay/portal primitive lets focus move outside a host element, verify focus-trap will treat it as an allowed external target.
• Hardcoded tabindex values that conflict with FocusMonitor / focus-trap logic.

7. Performance at scale

A headless library ships into pages its authors never see. Review the diff through one question: what does this cost when the primitive is on the page a thousand times, or when its handler fires sixty times a second? Work that is invisible on one instance is a hung tab on a dense page.

Three axes. Ask each of the changed code:

• Per instance × N — what runs for every instance, whether or not the user ever interacts with it.
• Per event × frequency — what runs inside a pointermove / scroll / wheel / input / keydown handler.
• Per notification, superlinear — work whose cost grows with the number of siblings, so adding one child re-does work for all of them.

What to flag

Forced synchronous layout. Grep the diff first — this is mechanical, so do it before reading:

git diff next...HEAD | grep -nE '^\+.*\b(getBoundingClientRect|getClientRects|getComputedStyle|scrollIntoView|offset(Width|Height|Top|Left|Parent)|client(Width|Height|Top|Left)|scroll(Width|Height|Top|Left))\b'

Every hit except scrollIntoView is a layout read. A read costs nothing on its own; it costs a full reflow when layout is dirty. scrollIntoView differs: it always scrolls the element into view (forcing layout to compute the destination), so it is a side effect, not a read. Judge each hit by what runs around it — and for a read, by what writes DOM before it:

• In a state factory body, a directive constructor, or anything running while Angular creates elements. Angular is writing DOM in the same pass, so each read forces its own reflow — N instances cost N reflows, each growing with the document. Defer to queueMicrotask (still ahead of paint, so nothing is a frame staler, and the whole creation pass shares one flush) or to the earlyRead phase of afterRenderEffect. fromResizeEvent (internal/src/utilities/resize.ts) is the worked example.

• Interleaved with writes in one afterRenderEffect. Plain afterRenderEffect(fn) is the mixed-read-write phase: a callback that measures and then writes styles or attributes dirties layout for every instance that follows it. Use the phased form so all reads land before any write.

  afterRenderEffect({
    earlyRead: () => element.nativeElement.scrollHeight,
    write: height => element.nativeElement.style.setProperty(heightVar, `${height()}px`),
  });
  

• Inside a subscriber to a batched producer. fromResizeEvent and fromMutationObserver batch their own reads into one microtask checkpoint; a synchronous DOM write in any subscriber breaks that batch for every instance after it.

• Inside a high-frequency handler without caching. A pointermove handler that re-reads the track's rect on every move is a reflow per frame per active pointer — read once on pointerdown, or on resize.

DOM traversal per instance or per event. closest(), querySelector/querySelectorAll, matches() in a loop, and hand-rolled parentElement walks are all O(depth) or O(document). Flag them in a factory body or a hot handler; hoist to the parent state and share the result, or cache the resolved element. An ancestor walk that runs on every scroll event, once per overlay, is the shape to watch (portal/src/scroll-strategy.ts).

Superlinear parent/child registration. Container primitives collect children (roving-focus, listbox, select, menu, tree, toolbar) and these are exactly the primitives that appear in the thousands. Registration must be O(1) amortised. Flag a register/unregister that scans, sorts, re-indexes, or compares document position across the whole collection on each call — N children then cost O(N²) to mount, which is the difference between a 200-row listbox and a 5,000-row one. The same applies to a parent computed that rebuilds an array every time any child changes while every child reads it.

Per-instance observers, listeners and timers. For each one added:

• Lazy? A ResizeObserver / MutationObserver / IntersectionObserver created for a feature that is switched off wastes an observer per instance. Gate it on a disabled signal.
• Torn down? Disconnected on destroy and when the feature is disabled — and any effect that could rebuild it destroyed alongside the subscription, or it rebuilds observers nobody listens to.
• Hoisted or scoped? A document/window listener held for the lifetime of an instance is a HIGH finding. Either hoist it into a root-level singleton (NgpOverlayRegistry, hover-interaction) or attach it only for the duration of a gesture (pointerdown → pointerup), as the slider and colour thumbs do. Scroll/wheel/touch listeners need { passive: true } unless they call preventDefault.

Eager work in the factory. Everything in a state factory body runs once per instance at construction, before any interaction. Flag anything heavier than a field assignment that isn't needed until the user acts: building an Intl formatter, compiling a regex, cloning config, materialising a list, subscribing to a stream for an off-by-default feature, or an effect/explicitEffect created unconditionally for a feature the inputs disabled. computed() is lazy and cached — prefer it to an eager const, and prefer one computed to an effect that writes a signal.

What not to flag

This section is about cost that scales. Constant-factor micro-optimisation is noise and will bury the findings that matter:

• Don't flag map/filter/spread over a small fixed collection, an extra object allocation, or a string concatenation.
• Don't propose memoising something that runs once per instance and is already cheap.
• Don't flag a cost that is genuinely bounded — a single overlay's positioning work is bounded by "one overlay is open", however expensive it looks.
• Severity follows the axis, not the instinct (this mapping takes precedence over the general severity guidance below for performance findings): HIGH for forced layout in a creation/render path, a superlinear registration, or a permanent global listener per instance; MEDIUM for a constant per-instance overhead that is real but not layout-forcing; below that, drop it.

Testing a scale fix

When the change is about cost rather than behaviour, the test has to assert the cost. Reject wall-clock assertions (expect(elapsed).toBeLessThan(...)) — on a slow CI box they either flake or are set so loose they catch nothing. Instead render N instances and count operations: patch the layout-reading accessors, or spy on the method whose call count is the invariant, and assert the count matches the intended complexity for that code path (tooltip/src/tooltip-trigger/tests/tooltip-overflow-scale.test.ts). O(N) total work is expected and correct when N independent instances each require constant setup; reject O(N²) or unnecessary repeated/duplicated work per instance (e.g., superlinear registration that re-scans all children on each hookup). Check the counter actually covers the work: a counter restored before a deferred microtask flushes never sees the deferred reads at all.

8. Documentation & examples

PR template requires docs updates for features and bug fixes. For new public behaviour:

• Look for a matching example under apps/documentation/src/app/examples/<primitive>/.
• Look for a docs page under apps/documentation/src/app/pages/primitives/<primitive>/.
• Generators: nx g @ng-primitives/tools:example <name> --primitive <primitive> and nx g @ng-primitives/tools:documentation.

If the diff adds or moves a ## section in a page under apps/documentation/src/app/pages/(documentation)/, check it against .claude/rules/documentation-pages.md. Flag by file:line:

• Section out of order. The canonical order is Import → Usage → Reusable Component → Schematics → Examples → API Reference → Styling → Animations → Global Configuration → Accessibility. Sections may be omitted, never reordered, and Accessibility is always last.
• A second ## Examples (or any duplicated section) on one page - add an ### subsection to the existing block instead.
• A page-specific ## section wedged between API Reference and Accessibility, splitting the reference tail. Example-style content belongs before API Reference.

If the diff touches files under apps/documentation/src/app/examples/, check them against .claude/rules/docs-example-styling.md. Common violations to flag by file:line:

• Off-brand colour. Brand red (--ngp-primary / #f01e2b / #ff4651) on a focus ring (:focus, [data-focus], [data-focus-visible], :focus-within) - focus must be blue (--ngp-focus-ring / blue-500 / blue-400). Conversely, blue / --ngp-background-inverse / off-palette tokens (--ngp-text-blue, --ngp-background-blue, --ngp-background-success) used for a state (checked, selected, active, fill, primary button) - those must be brand red.
• Menu/option focus as an outline instead of a background highlight, or a missing outline: none on the dropdown panel.
• CSS ↔ Tailwind drift. A change to <name>.example.ts without the matching change to <name>.tailwind.example.ts (or vice-versa), so the two variants no longer match.
• Typography/radii off-scale (500/600 weights instead of 510/590, no negative tracking, dark panels using bg-black / gray-700 instead of zinc-950 / zinc-800).
• Dialog overlay missing z-index: 1000 / z-[1000] (backdrop renders under the navbar).

9. Commits & PR template

• Conventional Commits required: feat(scope): …, fix(scope): …, docs(scope): …, chore(scope): …. Scope is usually the primitive name (combobox, focus-trap, tooltip).
• Flag malformed commit messages before suggesting merge.
• PR template checklist: tests added, docs updated, issue linked (Closes #...), breaking change disclosed.

10. Formatting

Formatting is enforced by .prettierrc plus the Prettier plugins (@trivago/prettier-plugin-sort-imports, prettier-plugin-organize-attributes, prettier-plugin-tailwindcss). If something looks off, suggest pnpm format rather than nitpicking line-by-line.

Output format

Each finding is its own block. Group blocks under the file they belong to, ordered by severity (High → Medium → Low). Do not pack multiple findings into one bullet — one finding per block so they're easy to read and triage. Always cite file:line. Use real markdown — the structure below shows the literal shape (do not emit it as a code fence).

## Summary
<1-3 sentences: what the diff does and overall verdict>

## <path/to/file.ts>

### 🔴 [HIGH · 95%] <one-line title> — `file.ts:42`
<one or two sentences explaining the issue and why it matters today>

**Fix:**
\`\`\`ts
<short before/after or single-line replacement — only if it fits in ~10 lines>
\`\`\`

### 🟡 [MEDIUM · 80%] <title> — `file.ts:60`
<explanation>

(no fix block if the change isn't short enough to show inline — describe it in prose instead)

Severity

Always prefix the severity tag with its emoji so the reader can scan the review visually:

• 🔴 HIGH — correctness bug, lint-rule violation, public API regression, accessibility regression, a performance finding that §7 maps to HIGH (forced layout in a creation/render path, superlinear registration, or a permanent global listener per instance — see §7's severity mapping, which takes precedence for performance findings), broken test, missing required test for a behaviour change. The PR should not merge without addressing it.
• 🟡 MEDIUM — clarity, maintainability, type-safety, or a convention miss that isn't lint-enforced. Worth fixing in this PR but not a blocker.
• 🟢 LOW — style or naming nits, usually auto-fixable by pnpm format. Mention briefly or omit.

Confidence

Express as a percentage (e.g. 85%) reflecting how sure you are the finding is real and the fix is correct, given the code as it stands today:

• 90–100% — verified against the rule, the file, and any referenced symbols. Safe to act on.
• 70–89% — strong signal but one assumption unverified (e.g. didn't run lint, didn't grep all call sites).
• 50–69% — soft finding; phrase as "verify" rather than "fix".
• Below 50% — do not raise. If you can't get above 50% with the information at hand, either dig further or drop the finding.

If a finding's confidence is below 70% and severity is LOW, drop it entirely — it's noise.

Inline fixes

When the fix is short (≤10 lines, ideally ≤3), include a **Fix:** code block showing the suggested change. For longer or more invasive changes, describe the fix in prose instead — do not paste large diffs into the review.

What not to include

• Speculative future-proofing (see "Filter speculative findings" above).
• Findings outside the diff scope.
• Praise sections, restatements of the diff, or summaries longer than three sentences.
• Categories with no findings (don't print empty headings).

Keep the review tight — fewer, sharper findings beat a long list.

After the review

Once the findings have been printed, follow this flow. Do not start fixing anything proactively — wait for the user to choose.

1. Ask what to fix

Use AskUserQuestion with a single question (header: "Fixes") offering these options. Skip severities that produced no findings — if there are no LOW findings, don't offer "Fix LOW". If there are no findings at all, skip this step entirely and go straight to step 3.

• Fix HIGH only — apply fixes for 🔴 HIGH findings.
• Fix HIGH + MEDIUM — apply fixes for 🔴 HIGH and 🟡 MEDIUM findings.
• Fix all — apply fixes for every finding regardless of severity.
• Choose specific issues — user will list which findings to fix (by file:line or title).
• Skip fixes — don't change anything.

When the user picks a subset, apply only those fixes and report what changed. Do not bundle unrequested cleanups.

2. Re-run after fixing

After applying any fixes, summarise in one or two sentences what was changed (cite file:line). Do not re-run the full review unless the user asks for it.

3. Offer to create a pull request

After fixes (or immediately, if the user chose "Skip fixes" or there were no findings), prompt the user about a PR.

First check for uncommitted changes with git status --porcelain. If there are any tracked changes that aren't committed:

• Print the list of uncommitted files.
• Ask via AskUserQuestion (header: "Commit?"):
  • Commit now — stage the relevant files and create a conventional-commit message. Confirm the message with the user before committing.
  • Skip commit — leave the working tree as-is; do not create the PR (a PR built off an uncommitted tree won't include the fixes).
  • Cancel — don't commit and don't open a PR.

Do not auto-stage .env, credentials, or files outside the diff scope. Mention any such files in the prompt rather than including them.

Once the tree is clean (or already was), ask via AskUserQuestion (header: "Open PR?"):

• Create pull request — run the PR-creation flow against next (this repo's main branch). Use the .github/PULL_REQUEST_TEMPLATE.md sections: PR Checklist, PR Type, linked issue, description, breaking-change disclosure. Confirm the title and body with the user before invoking gh pr create.
• Not yet — stop here; the review is done.

Never push branches or open PRs without explicit user confirmation at this prompt.