review-component-pr - SKILL.md Agent Skill

name: review-component-pr description: Comprehensive code review checklist for component pull requests ensuring quality, accessibility, and adherence to project conventions

Review Component PR

This skill provides a systematic checklist for reviewing pull requests that add or modify web components in the project.

Example Usage

"Review the new tooltip component PR"
"Check if the badge component changes follow our conventions"
"Perform a code review on PR #123"

Related Skills

create-new-component - Reference for new component structure
add-component-property - Property implementation patterns

When to Use

Reviewing a PR that adds a new component
Reviewing changes to an existing component
Pre-merge quality gate check
Onboarding new contributors

Review Checklist

1. Project Structure & Files

Component directory exists at src/components/[component-name]/
Component file named correctly: [component-name].ts
Test file exists: [component-name].spec.ts
Storybook story exists: stories/[component-name].stories.ts
Theme files present in themes/ directory
- [component-name].base.scss
- themes.ts aggregator
- All four theme files (bootstrap, material, fluent, indigo)
Export added to src/index.ts in alphabetical order

2. TypeScript Quality

No any types - Use unknown or proper types
Strict type checking passes
Import paths use .js extension (not .ts)
No native private fields - Use _prefix or TypeScript private, not #
Internal API prefixed with underscore: _method(), _property
readonly modifier used for immutable properties
Explicit return types on methods (unless obvious/cluttering)
Property decorators used correctly (@property, @query, etc.)
Type annotations present on all public APIs
Interfaces/types defined for complex structures

Good:

@property({ reflect: true })
public variant: StyleVariant = 'primary';

private _internalState: string = '';

Bad:

@property()
public variant: any; // ❌ No any types

#privateField = ''; // ❌ No native private fields

3. Component Class Structure

Extends LitElement or appropriate mixin
tagName static property defined: 'igc-[component-name]' with readonly modifier
styles static property includes base and shared styles
register() static method present
Region comments organize code: Internal state, Public properties, Lit lifecycle, Event handlers, Internal API, Public API
Internal API prefixed with underscore: _internalMethod(), _privateState
readonly modifier used for immutable properties
Explicit return types specified for methods (unless obvious)
Theming controller added in constructor: addThemingController(this, all)
Constructor calls super() first if overridden
Default export used: export default class
Global declaration included for TypeScript

Template:

export default class IgcComponentComponent extends LitElement {
  public static readonly tagName = 'igc-component';
  public static override styles = [styles, shared];

  public static register(): void {
    registerComponent(IgcComponentComponent);
  }

  //#region Internal state

  private _internalState = '';

  //#endregion

  //#region Public properties

  @property({ reflect: true })
  public variant = 'primary';

  //#endregion

  constructor() {
    super();
    addThemingController(this, all);
  }

  //#region Lit lifecycle

  protected override render() {
    return html`<div part="base"><slot></slot></div>`;
  }

  //#endregion

  //#region Internal API

  private _handleChange(): void {
    // Internal helper
  }

  //#endregion
}

declare global {
  interface HTMLElementTagNameMap {
    'igc-component': IgcComponentComponent;
  }
}

4. Properties & Attributes

Only primitives exposed as attributes (string, number, boolean)
Complex types use attribute: false
Boolean properties use { type: Boolean, reflect: true }
Number properties use { type: Number }
reflect: true only for primitives that should be attributes
Default values are appropriate
JSDoc comments include @attr for reflected properties
JSDoc comments include @default for default values

Good:

/**
 * The variant style of the component
 * @attr variant
 * @default 'primary'
 */
@property({ reflect: true })
public variant: StyleVariant = 'primary';

/**
 * Complex configuration object
 */
@property({ attribute: false })
public config: Config = {};

Bad:

@property({ reflect: true }) // ❌ Trying to reflect complex type
public config: Config = {};

5. Lifecycle & Reactivity

Lifecycle hooks used correctly
- update() for side effects (DOM available)
- willUpdate() only for pre-render computation
- super.update() called when overriding update()
changedProperties.has() checked to avoid unnecessary work
No @watch decorator used (prefer lifecycle hooks)
requestUpdate() not called unnecessarily

Good:

protected override update(changedProperties: PropertyValues<this>): void {
  if (changedProperties.has('disabled')) {
    // Update ARIA or other side effects
    this._internals.setARIA({ ariaDisabled: `${this.disabled}` });
  }
  super.update(changedProperties);
}

6. Shadow DOM & Styling

Shadow DOM used (not open mode issues)
CSS parts exposed with part attribute for external styling
CSS parts documented in JSDoc with @csspart
CSS custom properties documented with @cssproperty
SCSS files transpiled to .css.js files
No direct .css.js edits (only edit SCSS)
All themes implemented (bootstrap, material, fluent, indigo)
Light and dark modes considered
:host styles include appropriate display value

Good:

/**
 * @csspart base - The main container
 * @csspart content - The content wrapper
 * @cssproperty --padding - Internal padding
 */
protected override render() {
  return html`<div part="base"><slot></slot></div>`;
}

7. Accessibility (Critical)

Accessibility tests pass - await expect(el).to.be.accessible()
Semantic HTML used where appropriate
ARIA attributes set correctly
role attribute appropriate for component type
Keyboard navigation implemented
Focus management works correctly
delegatesFocus used if component should delegate focus
Screen reader announcements appropriate
Color contrast meets WCAG standards
Focus indicators visible
addInternalsController used for ARIA management if needed

Check:

// Should have accessibility test
it('passes the a11y audit', async () => {
  const el = await fixture<IgcComponentComponent>(
    html`<igc-component></igc-component>`
  );

  await expect(el).shadowDom.to.be.accessible();
  await expect(el).to.be.accessible();
});

8. Events

Events use EventEmitterMixin if custom events needed
Event map interface defined
Events documented with @event in JSDoc
Event names follow convention (lowercase, no on prefix)
Events composed if they should cross Shadow DOM boundary
Events bubbling configured appropriately
Event detail types are properly typed

Good:

export interface IgcComponentEventMap {
  igcChange: CustomEvent<string>;
}

/**
 * @event igcChange - Emitted when value changes
 */
export default class IgcComponentComponent extends EventEmitterMixin<
  IgcComponentEventMap,
  Constructor<LitElement>
>(LitElement) {
  // ...
  this.emitEvent('igcChange', { detail: newValue });
}

9. Tests

Accessibility test included (required)
Default values tested
Property changes tested
Attribute reflection tested (for reflected properties)
Events tested
Slot content tested
Edge cases covered
defineComponents() called in before() hook
elementUpdated() used after programmatic changes
Tests use @open-wc/testing framework
All tests pass locally

Minimum Required:

describe('Component', () => {
  before(() => {
    defineComponents(IgcComponentComponent);
  });

  it('passes the a11y audit', async () => {
    // Required accessibility test
  });

  it('should initialize with default values', async () => {
    // Test defaults
  });

  it('can change properties', async () => {
    // Test reactivity
  });
});

10. Storybook Stories

Story file exists at stories/[component-name].stories.ts
Metadata defined with title, component, description
argTypes match component properties
args provide default values
Controls appropriate for property types
TypeScript interface for args
At least one story demonstrates basic usage
Multiple stories for different variants (ideal)
Story renders correctly in Storybook

11. Documentation

JSDoc comments on component class
@element tag with component tag name
@slot tags for all slots (default and named)
@csspart tags for all exposed parts
@cssproperty tags for CSS custom properties
@event tags for emitted events
Property descriptions clear and complete
Examples in JSDoc if complex usage

Complete Example:

/**
 * A button component for user interactions.
 *
 * @element igc-button
 *
 * @slot - Default slot for button text
 * @slot prefix - Content before the button text
 *
 * @csspart base - The button element
 * @csspart content - The content wrapper
 *
 * @cssproperty --padding - Button padding
 *
 * @event igcClick - Emitted on click
 */

12. Form Integration (if applicable)

FormAssociatedRequiredMixin used for form controls
Form value state managed correctly
Validation implemented if needed
value property reactive and synced
Form reset handled
Required/disabled states work correctly
Labels associated correctly

13. Build & Compilation

TypeScript compiles without errors: npm run check-types
No linting errors: Check biome/eslint output
SCSS transpiled to .css.js files
All tests pass: npm run test
No console errors/warnings in Storybook
Bundle size reasonable (check for large dependencies)

14. Code Quality

No commented-out code (unless with explanation)
No debug statements (console.log, debugger)
Consistent formatting (should be auto-formatted)
Meaningful variable names
No magic numbers (use named constants)
Error handling appropriate
Code is DRY (Don't Repeat Yourself)

15. Project Conventions

Follows composition over inheritance
No heavy third-party dependencies unless necessary
Uses project utilities from common/ where applicable
Consistent with existing component patterns
Theming uses igniteui-theming package
Modern ECMAScript features used appropriately

Common Issues to Watch For

Issue: Missing Theming Controller

Problem: Component doesn't respond to theme changes Fix: Add addThemingController(this, all) in constructor

Issue: Wrong Import Extensions

Problem: Using .ts instead of .js in imports Fix: All TypeScript imports must use .js extension

Issue: Exposing Complex Types as Attributes

Problem: Trying to reflect objects/arrays to HTML attributes Fix: Use attribute: false for non-primitive types

Issue: Missing Accessibility Tests

Problem: No a11y test in spec file Fix: Always include accessibility audit test

Issue: Forgot to Export Component

Problem: Component not available when importing from package Fix: Add export to src/index.ts in alphabetical order

Issue: Using @watch Decorator

Problem: Using deprecated pattern for property changes Fix: Use update() or willUpdate() lifecycle hooks

Issue: Native Private Fields

Problem: Using #privateField syntax Fix: Use _privateField or TypeScript private keyword

Review Process

Start with structure - Verify all required files exist
Check TypeScript - Ensure strict typing and no any
Review accessibility - This is critical, don't skip
Test locally - Run build and tests
Check Storybook - Verify visual behavior
Review code quality - Look for patterns and conventions
Provide constructive feedback - Be specific and helpful

Quick Pass/Fail Criteria

Immediate Reject if:

No accessibility tests
Using any types extensively
Native private fields (#)
No tests at all
TypeScript compilation errors

Request Changes if:

Missing documentation
Incomplete test coverage
Accessibility issues
Not following project conventions
Missing theme files

Approve if:

All checkboxes checked
Tests pass
Accessibility verified
Code quality good
Follows project patterns

Resources

Coding Guidelines - Comprehensive coding standards and best practices
Lit Best Practices
WCAG Guidelines
Web Components Best Practices