# Ngp Code Review > Review changed code in the ng-primitives repo against project conventions. Use when the user asks for a code review, asks "review my changes", or asks if a PR/branch is ready. Do not invoke automatically before commits. - Skill: `ng-primitives/ngp-code-review` (Agent Skill) - Install (CLI): `npx skillmds@latest add ng-primitives/ngp-code-review` - Raw SKILL.md: https://api.skillmd.com/api/skills/ng-primitives/ngp-code-review/raw - Safety review: pending (external: skill-scanner PASS, skillspector PASS) - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Coding & Dev Tools - Author: ng-primitives (https://skillmd.com/u/ng-primitives) - Updated: 2026-09-17 - Page: https://skillmd.com/skills/ng-primitives/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 `. 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 ` — 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 -- ` 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 `-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: ```ts 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. | Rule | What to flag | | ----------------------------- | ----------------------------------------------------------------------------------------------------------------- | | `prefer-state` | Direct `this.input()` access in stateful directives — use `this.state.*`. | | `avoid-model` | Any `model()` usage — use an explicit input/output pair. | | `avoid-state-emit` | Outputs that emit `this.state.X` — emit the local controlled value so parent bindings round-trip. | | `avoid-early-state` | State registration not at the end of the class property block. | | `require-state-generic` | `createStateInjector()` missing ``. | | `take-until-destroyed` | `takeUntilDestroyed` from `@angular/core/rxjs-interop` — use `safeTakeUntilDestroyed` from `ng-primitives/utils`. | | `rxjs-compat` | Operators imported from `rxjs` — import from `rxjs/operators`. | | `prefer-entrypoint-imports` | Cross-package relative imports — use package aliases (`ng-primitives/foo`). | | `prefer-document-from-common` | `DOCUMENT` imported from `@angular/core` — import from `@angular/common`. | ### 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 `waitFor`s 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: ```bash 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. ```ts 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//`. - Look for a docs page under `apps/documentation/src/app/pages/primitives//`. - Generators: `nx g @ng-primitives/tools:example --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 `.example.ts` without the matching change to `.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> ## ### 🔴 [HIGH · 95%] — `file.ts:42` **Fix:** \`\`\`ts \`\`\` ### 🟡 [MEDIUM · 80%] — `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.