Imported from GerritCodeReview/gerrit (configs/skills/gerrit_frontend_engineering/SKILL.md). Install upstream with npx skills add GerritCodeReview/gerrit --skill gerrit_frontend_engineering. Copyright stays with the author.

Frontend Engineering & UI Development Engineering Guide

Name: gerrit-frontend-engineering
Author: GerritCodeReview

Executive Summary

Welcome to the Frontend Engineering & UI Development guide. This repository serves as the authoritative source of tribal knowledge for our UI architecture, born from historical refactoring efforts, critical performance optimizations, and the ongoing necessity to prevent recurrent regression of known failure modes. By codifying these engineering standards, we ensure that incoming engineers can confidently navigate the complexities of our frontend ecosystem without falling into legacy traps, introducing unverified UI states, or triggering silent runtime failures.

This guide enforces strict architectural boundaries across the entire UI development lifecycle. It mandates rigorous state encapsulation within the Lit framework, uncompromising TypeScript type safety, and highly resilient client-server integrations. It further standardizes hermetic UI testing methodologies, unifies our CSS design systems, and outlines strict strategies for client-side performance profiling.

Adherence to these principles guarantees structural consistency and system reliability. Whether you are building dynamic web components, integrating complex REST API endpoints, or optimizing data payloads for AI contexts and telemetry, this documentation establishes the foundational constraints required to ship a performant, predictable, and scalable user interface.

Summary

Chapter Theme / Title	Scope & Objective
**Lit Framework Idioms & State	Enforce strict Lit framework idioms
: Encapsulation** : by leveraging declarative rendering, :
: : reactive property encapsulation, and :
: : native lifecycle hooks. Avoid :
: : imperative DOM manipulation and :
: : properly isolate transient UI status :
: : from the core data models. :
**TypeScript Strictness & Type	This domain governs the strict
: Safety** : enforcement of TypeScript type safety :
: : by explicitly forbidding unsafe :
: : casting and blanket compiler :
: : suppressions. It mandates precise :
: : component modeling, strict interface :
: : adherence, and proper access :
: : modifiers to eliminate silent runtime :
: : failures and unverified states. :
**Hermetic Testing & Visual	This chapter mandates the strict
: Regression** : isolation of unit tests, :
: : comprehensive visual validation using :
: : full shadow DOM snapshots, and the :
: : centralization of test data :
: : generation to guarantee hermetic :
: : execution and prevent false-positive :
: : assertions. :
**Client-Side Performance &	This theme governs the optimization
: Telemetry** : of client-side performance by :
: : enforcing strict telemetry on :
: : synchronous CPU-bound operations and :
: : minimizing network latency. It :
: : mandates the caching of asynchronous :
: : API promises, the derivation of state :
: : from existing payloads, and the rigid :
: : bounding of background data requests. :
**CSS Architecture & Design System	This domain governs the consistent
: Consistency** : application of styles across the UI, :
: : mandating the use of content-driven :
: : layout techniques, externalized :
: : custom property configurations for :
: : visual assets, and declarative Lit :
: : directives over imperative inline :
: : styling. :
API Integration & Error Handling	This chapter governs the resilient
: : integration of frontend logic with :
: : REST APIs. It establishes strict :
: : constraints for maintaining backend :
: : payload parity, explicitly modeling :
: : structural variances, and enforcing :
: : centralized error handling over :
: : localized `try...catch` blocks. :
**AI Context & Telemetry Payload	This domain governs the client-side
: Optimization** : lifecycle and optimization of data :
: : structures sent to AI agents and :
: : telemetry pipelines. It strictly :
: : dictates payload deduplication :
: : strategies, transparent AI token :
: : constraint surfacing, and rigorous :
: : parsing validation to prevent silent :
: : context loss. :

Chapter: Lit Framework Idioms & State Encapsulation

Context: Enforce strict Lit framework idioms by leveraging declarative rendering, reactive property encapsulation, and native lifecycle hooks. Avoid imperative DOM manipulation and properly isolate transient UI status from the core data models.

Summary

Rules

T1-01: Separation of Data and UI State Variables

Rule: Never overload core data variables with transient UI status strings. You must use dedicated status properties to manage UI state separately from the underlying data model.

What: Do not overload core data variables with transient UI status strings (e.g., 'Loading...', 'Error'). Use dedicated status properties to manage UI state separately from the underlying data model.

Applies To: Lit components managing asynchronous operations and displaying resulting data alongside loading or error states.

Why: A generatedPassword property was overloaded to temporarily hold 'Generating...' or 'Failed to generate'. This caused the associated 'Copy to clipboard' component to copy the error or status text instead of a valid password. Failing to adhere to this typically results in Invalid Data Copied.

Trap 1: Reusing a data property to hold UI loading or error text, which corrupts the data being passed to child components or the clipboard.

Don't:

this._generatedPassword = 'Generating...';
this.restApiService.generatePassword().then(newPassword => {
  this._generatedPassword = newPassword ?? 'Failed to generate';
});

Do:

this.status = 'Generating...';
this.restApiService.generatePassword().then(newPassword => {
  if (newPassword) {
    this.generatedPassword = newPassword;
    this.status = undefined;
  } else {
    this.status = 'Failed to generate';
  }
});

T1-02: Explicit Boolean Properties over Dynamic Slot Detection

Rule: Always control conditional layout wrappers using explicit boolean properties mapped to the component API. Never dynamically query the DOM for the presence of assigned slots.

What: Control the rendering of conditional layout wrappers using explicit boolean properties mapped to the component's API, rather than dynamically querying the DOM for the presence of assigned slots.

Applies To: Lit components featuring conditional layout containers that wrap <slot> elements (e.g., search bars with optional leading icons).

Why: Dynamically checking hasNamedSlot by iterating over this.children was computationally brittle and caused rendering bugs where structural elements (like search icons) were accidentally removed or created unintended 'ghost spacing'. Failing to adhere to this typically results in Layout Regression / Missing Elements.

Trap 1: Inspecting the DOM via this.children in Lit element methods to determine if a named slot element was provided by the parent.

Don't:

private hasNamedSlot(name: string): boolean {
  return Array.from(this.children).some(
    el => el.getAttribute('slot') === name
  );
}

render() {
  return this.hasNamedSlot('leading-icon') ? html`<div><slot name="leading-icon"></slot></div>` : nothing;
}

Do:

@property({type: Boolean})
showLeadingIcon = false;

render() {
  return this.showLeadingIcon ? html`<div><slot name="leading-icon"></slot></div>` : nothing;
}

T1-03: Declarative Event Listeners in Lit Templates

Rule: Always bind event listeners declaratively directly within the component's render() template. Never imperatively attach listeners using this.addEventListener in the constructor.

What: Bind event listeners declaratively directly within the component's render() template using @event syntax, rather than imperatively attaching them using this.addEventListener in the constructor.

Applies To: LitElement initialization and user interaction event handling.

Why: Imperatively adding listeners via the constructor disconnects the logic from the declarative template, risks memory leaks if not cleaned up, leaves inline documentation orphaned, and triggers automated code health warnings. Failing to adhere to this typically results in Structural Anti-Pattern / Orphaned Context.

Trap 1: Attaching DOM event listeners imperatively within the component constructor.

Don't:

constructor() {
  super();
  // BAD: Imperative binding
  this.addEventListener('mousedown', e => this.handleMouseDown(e));
}

Do:

override render() {
  // GOOD: Declarative binding directly in the Lit template
  return html`
    <div class="menu" @mousedown=${this.handleMenuMouseDown}>
      <div class="menu-item">${this.hoverCardText}</div>
    </div>
  `;
}

T1-04: Declarative Conditional CSS via classMap

Rule: Must use the Lit classMap directive for applying conditional CSS classes in templates. Avoid manual string interpolation with ternary operators.

What: Use the Lit classMap directive for applying conditional CSS classes in templates instead of manual string interpolation with ternary operators.

Applies To: Lit Framework templates (render() methods).

Why: Manual string interpolation for classes is difficult to read and prone to whitespace concatenation errors, which leads to incorrectly applied or missed CSS selectors during dynamic state changes. Failing to adhere to this typically results in Malformed Class Strings / UI Bugs.

Trap 1: Chaining ternary operators inside a template string to build a class list.

Don't:

<div class="diffContainer ${this.shownSidebar ? 'sidebarOpen' : ''} ${this.file?.diffs_too_expensive_to_compute ? 'hidden' : ''}">

Do:

<div class=${classMap({
  diffContainer: true,
  sidebarOpen: this.shownSidebar,
  hidden: !!this.file?.diffs_too_expensive_to_compute
})}>

T1-05: Centralized Periodic LitElement Updates via Generic Utility

Rule: Always register components requiring timer-based periodic re-rendering with a centralized update manager. Never implement individual setInterval loops inside isolated components.

What: Components requiring timer-based periodic re-rendering must register with a centralized PeriodicUpdateManager rather than implementing individual setInterval and lifecycle cleanup logic inside the component.

Applies To: LitElements displaying time-sensitive data (e.g., relative dates, "time ago" formatters).

Why: Individual date formatter components initially implemented their own object literal manager or interval timers. This violated the separation of concerns and led to memory leaks if components failed to clean up their specific timers upon disconnection. Failing to adhere to this typically results in Memory Leaks / Duplicated Timer Logic.

Trap 1: Defining a custom manager object or singleton strictly tied to one specific UI component type for periodic updates.

Don't:

export const dateFormatterManager = {
  formatters: new Set<GrDateFormatter>(),
  register(formatter) { /* set interval to call requestUpdate */ }
}

Do:

// In periodic-update-util.ts
export class PeriodicUpdateManager<T extends LitElement> {
  constructor(private readonly refreshIntervalMs: number) {}
  register(component: T) { /* generic interval logic */ }
}

// In component
override connectedCallback() {
  super.connectedCallback();
  dateFormatterManager.register(this);
}

T1-06: Declarative Attribute Reflection in Lit Components

Rule: Must use Lit's @property({reflect: true}) decorator to synchronize a component's property state with DOM attributes. Never manually manipulate attributes via setAttribute within lifecycle methods.

What: Use Lit's @property({reflect: true}) decorator to automatically synchronize a component's property state with its corresponding DOM attributes, replacing manual DOM manipulation calls.

Applies To: Lit web components, styling hooks, and state management.

Why: A custom icon wrapper was manually setting and removing an attribute inside the willUpdate lifecycle method to apply CSS selectors, violating declarative state management principles. Failing to adhere to this typically results in Imperative DOM Manipulation.

Trap 1: Manually invoking setAttribute or removeAttribute within a Lit lifecycle method to sync DOM properties.

Don't:

override willUpdate() {
  if (this.icon) {
    this.setAttribute('custom', '');
  } else {
    this.removeAttribute('custom');
  }
}

Do:

@property({type: Boolean, reflect: true})
custom = false;

override willUpdate() {
  this.custom = this.icon ? true : false;
}

T1-07: Idempotent Telemetry Reporting in Lit Component Lifecycle Hooks

Rule: Must guard telemetry interactions fired during the updated() lifecycle hook with a dedicated state flag. Never allow subsequent, unrelated property changes to trigger duplicate reporting.

What: Telemetry interactions fired during the updated() lifecycle hook must be guarded by a dedicated state flag to prevent duplicate reporting triggered by subsequent, unrelated property changes.

Applies To: Lit components executing side effects (like analytics or impression tracking) within the updated() loop.

Why: When streaming AI responses, the component's state rapidly updated. Without an idempotent guard flag, the system generated multiple telemetry events for the exact same interaction every time the state re-rendered. Failing to adhere to this typically results in Duplicate Impression Reporting.

Trap 1: Firing a tracking event inside updated purely based on conditional presence, without a state flag acknowledging it fired.

Don't:

override updated(changedProperties: PropertyValues) {
  if (this.message()?.responseComplete) {
    this.reportSuggestionsShown();
  }
}

Do:

private reportedSuggestionsShown = false;

override updated(changedProperties: PropertyValues) {
  if (!this.reportedSuggestionsShown && this.message()?.responseComplete) {
    this.reportSuggestionsShown();
    this.reportedSuggestionsShown = true;
  }
}

T1-08: Strict Truthiness Checks for Asynchronous Array Data

Rule: Always explicitly check for null or undefined before evaluating the .length property of asynchronous array data. Never rely solely on optional chaining length checks for truthiness.

What: When determining the fallback UI state based on asynchronous array data (like API responses), explicitly check for null/undefined before checking the .length property.

Applies To: Lit components conditionally rendering UI elements based on the loaded state of arrays.

Why: Checking array?.length === 0 fails when the array is still undefined, because undefined === 0 resolves to false, causing the application to skip rendering the fallback UI during the loading or uninitialized state. Failing to adhere to this typically results in Missing Fallback UI.

Trap 1: Relying on optional chaining length checks to represent empty or missing data.

Don't:

if (this.repoLabels?.length === 0) {
  return this.renderDefaultParameterInputField();
}

Do:

if (!this.repoLabels || this.repoLabels.length === 0) {
  return this.renderDefaultParameterInputField();
}

T1-09: Pre-computing Derived State in Lit Lifecycle Methods

Rule: Always pre-compute derived state within the willUpdate lifecycle method or a dedicated observer. Never process strings or execute heavy array computations directly inside the render() loop.

What: Avoid processing strings or doing heavy array computations directly inside the render() method or helper template methods. Instead, reactively compute derived state (e.g., parsing a string into an array) within the willUpdate lifecycle method or a dedicated observer.

Applies To: Lit components dealing with data transformation before rendering.

Why: Dynamic computation inside render cycles degrades performance and muddles template readability. Helper methods that executed .split() and regex operations on strings were running repeatedly on every re-render. Failing to adhere to this typically results in Redundant Computations.

Trap 1: A helper method called in render() that parses raw strings into arrays on every invocation.

Don't:

private getParameters(): string[] {
  if (this.parameters) return this.parameters;
  if (this.parameterStr?.trim()) {
    return this.parameterStr.trim().split(/\s+/);
  }
  return [];
}

render() {
  const params = this.getParameters();
  // ...
}

Do:

// Handle the calculation inside `willUpdate` reacting to changes in `this.parameterStr`
willUpdate(changedProperties: PropertyValues) {
  if (changedProperties.has('parameterStr')) {
    this.parameters = this.parameterStr?.trim() ? this.parameterStr.trim().split(/\s+/) : [];
  }
}

T1-10: Returning `nothing` for Empty Templates in Lit

Rule: Must explicitly return the nothing sentinel value instead of an empty template literal when rendering conditionally empty elements.

What: In Lit templates, explicitly return the nothing sentinel value instead of an empty template literal when rendering conditionally empty elements.

Applies To: Lit components, specifically conditional rendering blocks.

Why: Historically, empty template instances (html``) were returned for false conditions, which created unnecessary markers in the DOM, slightly degrading rendering efficiency and DOM cleanliness. Failing to adhere to this typically results in DOM Clutter.

Trap 1: Using an empty template literal or omitting a return when a condition fails in the template.

Don't:

// BAD: Returning empty template
render() {
  if (!this.show) return html``;
  return html`<div>Content</div>`;
}

Do:

// GOOD: Returning nothing
import {nothing} from 'lit';

render() {
  if (!this.show) return nothing;
  return html`<div>Content</div>`;
}

T1-11: Single-Pass Initialization of Reactive Properties

Rule: Always initialize base reactive properties during construction or via bound property updates. Never use the firstUpdated lifecycle hook for initial assignment.

What: Base reactive properties must be initialized during construction or via bound property updates rather than using the firstUpdated lifecycle hook, which triggers a redundant second render cycle.

Applies To: Lit components, specifically lifecycle hooks (constructor, willUpdate, firstUpdated).

Why: Assigning derived URL states inside firstUpdated triggered immediate, unnecessary re-renders, hurting initial paint performance. Failing to adhere to this typically results in Redundant Re-rendering.

Trap 1: Assigning values to @state() or @property() fields inside the firstUpdated hook.

Don't:

// BAD: Triggers a second render cycle immediately after the first
override firstUpdated() {
  this.hostUrl = window.location.origin;
}

Do:

// GOOD: Reactively bound to updates before render
override willUpdate(changedProperties: PropertyValues) {
  if (!this.hostUrl) {
    this.hostUrl = window.location.origin;
  }
}

Exceptions: Properties that strictly depend on measuring DOM dimensions or child element readiness after layout.

T1-12: Guarding Asynchronous State Updates Post-Disconnection

Rule: Always explicitly verify this.isConnected before executing asynchronous tasks or debounced updates. Never execute callbacks that mutate state during DOM teardown.

What: Components that schedule asynchronous tasks or debounced updates must explicitly verify this.isConnected before executing work to prevent mutations during DOM teardown.

Applies To: Event handlers, async callbacks, and debounced routines in Lit web components.

Why: Property updates triggered willUpdate hooks even after disconnectedCallback had run, scheduling new background tasks for components that were no longer attached to the document. Failing to adhere to this typically results in Memory Leaks.

Trap 1: Executing timeout handlers or asynchronous DOM updates without confirming the component remains in the DOM structure.

Don't:

// BAD: Running debounced task without verifying connection
updateSuggestions() {
  this.scheduleDebounceTask();
}

Do:

// GOOD: Short-circuiting if disconnected
updateSuggestions() {
  if (!this.isConnected) return;
  this.scheduleDebounceTask();
}

T1-13: Use Lit classMap Directive for Conditional CSS Classes

Rule: Must use Lit's classMap directive for managing multiple conditional CSS classes. Never use manual string concatenation or ternary chaining within class attributes.

What: Use Lit's classMap directive for managing multiple conditional CSS classes rather than manual string interpolation.

Applies To: Lit component templates rendering conditional classes based on component state.

Why: Historically, manual string concatenation for multiple conditional CSS classes led to messy, error-prone template structures that were difficult to read and maintain. Failing to adhere to this typically results in Unreadable/Error-Prone Templates.

Trap 1: Concatenating ternary operators inside the class attribute string of a Lit HTML template.

Don't:

class="context-chip ${this.isSuggestion ? 'suggested-chip' : ''} ${this.isCustomAction ? 'custom-action-chip' : ''}"

Do:

class=${classMap({'context-chip': true, 'suggested-chip': this.isSuggestion, 'custom-action-chip': this.isCustomAction})}

T1-14: Strict Lit State Encapsulation

Rule: Always isolate internal component variables that drive UI re-renders using the @state decorator. Never use @property for internal state that is not intended to be configured via HTML attributes.

What: Internal component variables that drive UI re-renders but are not intended to be configured via HTML attributes must use the @state decorator instead of @property.

Applies To: All Lit-based Web Components.

Why: Component logic was exposing internal data retrieval statuses (like loading state or fetched lists) as public @property attributes. This polluted the component's public API surface and allowed external DOM manipulation to improperly overwrite internal component state. Failing to adhere to this typically results in State Leakage.

Trap 1: Using the @property decorator for variables that represent internal component state (like data lists or loading spinners).

Don't:

// BAD: Internal state exposed as an attribute
@property({type: Boolean})
_loading = true;

@property({type: Array})
submitRequirements?: SubmitRequirementInfo[];

Do:

// GOOD: Internal state isolated using @state
@state()
loading = true;

@state()
submitRequirements?: SubmitRequirementInfo[];

T1-15: Declarative Component Visibility Toggling

Rule: Always handle conditional rendering of DOM elements using Lit's declarative when or nothing directives. Never dynamically apply CSS classes that set display: none to toggle visibility.

What: Conditional rendering of DOM elements must be handled using Lit's declarative when or nothing directives within the HTML template, rather than dynamically applying CSS classes that set display: none.

Applies To: All Lit templates, particularly for loading states and conditional sections.

Why: Loading states were previously managed by dynamically assigning a .loading CSS class to an element, which relied on external stylesheet rules to hide/show the node. This made the UI state harder to reason about and bypassed Lit's native DOM reconciliation. Failing to adhere to this typically results in Layout Shifts / DOM Bloat.

Trap 1: Controlling element visibility using CSS class conditionals.

Don't:

// BAD: CSS-driven visibility
render() {
  return html`
    <table class="${this.loading ? 'loading' : ''}">
      <!-- rows -->
    </table>
  `;
}
// Relying on: .loading #target { display: none; }

Do:

// GOOD: Declarative Lit rendering directives
render() {
  return html`
    <tbody>
      ${when(
        this.loading,
        () => html`<tr><td>Loading...</td></tr>`,
        () => html`<!-- Render data rows -->`
      )}
    </tbody>
  `;
}

T1-16: Immutable Lit Form State Resets

Rule: Always assign a newly constructed object reference to the state property when resetting form state or clearing dialog inputs. Never mutate the existing object's properties in-place.

What: When resetting form state or clearing dialog inputs in Lit, assign a newly constructed object reference to the state property rather than mutating the existing object's properties in-place.

Applies To: Lit form components and dialogs with complex object state (@state).

Why: Form dialogs failed to clear properly after creating a new item because the state object was mutated in place or partially reset, resulting in stale UI data where fields appeared populated but submitted empty values. Failing to adhere to this typically results in Stale UI Data.

Trap 1: Mutating form state fields directly without triggering a reference change for Lit to detect.

Don't:

// BAD: In-place mutation does not consistently trigger a full re-render
handleCreateCancel() {
  this.newRequirement.name = '';
  this.newRequirement.description = '';
  this.dialog.close();
}

Do:

// GOOD: Provide a new object reference to trigger full reactive updates
private getEmptyRequirement() {
  return { name: '', description: '' };
}

handleCreateCancel() {
  this.newRequirement = this.getEmptyRequirement();
  this.dialog.close();
}

Cross-Domain Dependencies

Upstream: T6 | API Integration & Error Handling - Fetching asynchronous data structures via REST clients dictates Lit component reactive loading states and rendering fallbacks.
Downstream: T5 | CSS Architecture & Design System Consistency - Lit template directives like classMap dynamically consume centralized structural CSS classes and shared UI styles.
Downstream: T7 | AI Context & Telemetry Payload Optimization - Lit lifecycle methods like updated() natively trigger telemetry interaction payloads that require idempotent guards to prevent data bloat.

Chapter: TypeScript Strictness & Type Safety

Context: This domain governs the strict enforcement of TypeScript type safety by explicitly forbidding unsafe casting and blanket compiler suppressions. It mandates precise component modeling, strict interface adherence, and proper access modifiers to eliminate silent runtime failures and unverified states.

Summary

Rules

T2-01: Targeted Type Casting over Broad Error Suppression

Rule: Always use targeted type casts (e.g., as any) to bypass specific visibility constraints in unit tests rather than silencing the entire line with @ts-expect-error.

What: Replace blanket @ts-expect-error directives with targeted type casts (e.g., (element as any)) when attempting to access private component methods or properties in unit tests.

Applies To: TypeScript unit tests interacting with encapsulated component logic.

Why: Using // @ts-expect-error suppresses all TypeScript errors on the subsequent line. Historically, this masked actual bugs like typos in test assertions (e.g., a typo in assert.isFalse), rendering the tests unreliable. Failing to adhere to this typically results in Masked Bugs / Silent Failures.

Trap 1: Suppressing all TypeScript compiler errors on a line just to bypass a private visibility check in a unit test.

Don't:

// BAD: Masks all TS errors, including typos in the assert statement.
// @ts-expect-error
assert.isFalse(element.hasAiComments());

Do:

// GOOD: Bypasses visibility selectively while maintaining strict type checking on the assertion.
assert.isFalse((element as any).hasAiComments());

T2-02: Removal of Underscore Prefixes for Reactive Properties

Rule: Never use leading underscores for Lit element properties; strictly enforce private state using TypeScript access modifiers.

What: Do not use leading underscores for Lit element properties. Rely on TypeScript private access modifiers to encapsulate internal state instead of naming conventions.

Applies To: All LitElement @property and @state declarations.

Why: Leading underscores were heavily used in the legacy Polymer implementation to denote privacy, but they violate current TypeScript strictness standards and style guidelines for the modernized PolyGerrit UI. Failing to adhere to this typically results in Style Guide Violation.

Trap 1: Prefixing reactive Lit properties with an underscore to denote private state.

Don't:

@property({type: String})
_passwordUrl: string | null = null;

Do:

@property({type: String})
passwordUrl: string | null = null;

T2-03: Elimination of TypeScript Compiler Suppressions in Unit Tests

Rule: Must never bypass compiler checks to modify test internals; explicitly elevate visibility of the required property and document the exception.

What: Unit tests must not bypass compiler checks using @ts-expect-error to test internal logic. Instead, widen the visibility of the target property or method from private to public/internal, and document it with a comment.

Applies To: TypeScript unit test suites and component class files (e.g., Lit components).

Why: Developers were using @ts-expect-error directives to suppress compiler errors when assigning or calling private component members in tests. This masked actual type regressions from the TS compiler. Failing to adhere to this typically results in Type Safety Bypass.

Trap 1: Suppressing the compiler error to mutate a private property in the test setup.

Don't:

// In test file:
// @ts-expect-error
element.docsBaseUrl = 'https://docs.com/';
// @ts-expect-error
assert.equal(element.computeHelpUrl(), '...');

Do:

// In component file:
// private but used in test
public docsBaseUrl = '';

// In test file (no suppressions):
element.docsBaseUrl = 'https://docs.com/';
assert.equal(element.computeHelpUrl(), '...');

T2-04: Utilizing TypeScript Utility Types over Unsafe Casting

Rule: Always construct precise subsets of interfaces using utility types like Pick<T, K> rather than forcibly blinding the compiler via as Type.

What: When creating objects that fulfill only a specific subset of an interface's requirements, construct the correct type signature using TypeScript utility types (e.g., Pick<T, K>) rather than using 'as Type' type assertions to blind the compiler.

Applies To: TypeScript data transformations, API response mapping, and component state variables.

Why: A subset of a LabelDefinitionInfo object was generated and aggressively typed using as LabelDefinitionInfo. This misled consumers of the data about which properties were actually populated, creating potential runtime hazards. Failing to adhere to this typically results in Unsafe Type Assertion.

Trap 1: Constructing an object with missing properties and masking the error by casting it as the full interface type.

Don't:

const partial = {
  name: 'LabelName',
  values: { '+1': '' }
} as LabelDefinitionInfo;

Do:

const partial: Pick<LabelDefinitionInfo, 'name'> & Pick<LabelDefinitionInfo, 'values'> = {
  name: 'LabelName',
  values: { '+1': '' }
};

T2-05: Prototype Methods over Arrow Function Properties

Rule: Always define class behaviors as standard prototype methods to avoid the excess instantiation overhead caused by arrow function properties.

What: Define class behaviors using standard class methods rather than assigning arrow functions as class properties to optimize memory usage.

Applies To: TypeScript class definitions across the application (Providers, Services, Models).

Why: Assigning arrow functions directly as class properties caused a new instance of the function to be created in memory for every instantiation of the class, unnecessarily increasing memory overhead. Failing to adhere to this typically results in Excessive Memory Overhead.

Trap 1: Using an arrow function assigned to a variable inside the class body.

Don't:

export class LabelSuggestionsProvider {
  getSuggestions = (
    predicate: string,
    expression: string
  ): Promise<AutocompleteSuggestion[]> => {
    // logic
  };
}

Do:

export class LabelSuggestionsProvider {
  getSuggestions(
    predicate: string,
    expression: string
  ): Promise<AutocompleteSuggestion[]> {
    // logic
  }
}

Exceptions: Arrow functions are acceptable if the method is passed around as a callback and strictly requires preserving the this lexical binding without manual .bind(this).

T2-06: Elimination of Unsafe `any` Type Casts

Rule: Never use the any type; enforce strict bounds using concrete interfaces or rely on unknown for safe downcasting.

What: The any type must be strictly avoided. Use explicit interfaces, strict types, or unknown for downcasting, eliminating @ts-expect-error and @typescript-eslint/no-explicit-any.

Applies To: Global TypeScript codebase, particularly component state assignments, plugin configurations, and test setups.

Why: Developer convenience led to pervasive use of any type casting, bypassing the compiler and masking structural mismatches that caused uncaught runtime exceptions when interfaces evolved. Failing to adhere to this typically results in Type Erasure.

Trap 1: Suppressing TypeScript errors or forcibly casting objects to any when dynamic types don't strictly align.

Don't:

// BAD: Bypassing type safety
// eslint-disable-next-line @typescript-eslint/no-explicit-any
.then((element: any) => {
  assert.strictEqual(element, module);
})

Do:

// GOOD: Using unknown or explicit interfaces
.then((element: unknown) => {
  assert.strictEqual(element, module);
})

T2-07: Testing Private State Encapsulation Rules

Rule: Must never pierce class boundaries using bracket notation (['property']) in tests; natively expose and document properties required for testing.

What: Do not bypass private property access via bracket notation (element['privateProp']) in tests. If a property must be exposed for testing, remove the private modifier and document it with a standard comment.

Applies To: Component state definitions and their respective test suites.

Why: Engineers were circumventing class lexical scope constraints by using bracket notation to set private component states in tests. This violated the TypeScript style guide and evaded static analysis tools. Failing to adhere to this typically results in Encapsulation Violation.

Trap 1: Forcibly accessing class internals using string-keyed bracket notation in test files.

Don't:

// BAD: Bypassing private scope
element['stages'] = [{ condition: 'status:open' }];

Do:

// GOOD: Remove private, add explicit documentation, and use dot notation
// In component:
@state() // private but used in tests
stages: Stage[] = [];

// In test:
element.stages = [{ condition: 'status:open' }];

T2-08: Eliminate Unsafe 'any' Type Casting in Test Stubs

Rule: Never substitute actual types with any in mock definitions; enforce accurate mock function signatures or use unknown.

What: Avoid using the any type when defining function arguments in stub callbacks (e.g., Sinon callsFake). Use the actual mocked type or unknown.

Applies To: Sinon stubs and mock definitions within unit tests.

Why: Using the 'any' type bypassed the TypeScript compiler's type checking, allowing signature mismatches between the mock and the actual implementation to silently pass. Failing to adhere to this typically results in Silent Type Mismatches.

Trap 1: Casting function arguments to any within a mock's custom callback function.

Don't:

sinon.stub(Obj, 'method').callsFake(function (this: Obj, account: any) { ... })

Do:

sinon.stub(Obj, 'method').callsFake(function (this: Obj, account: unknown) { ... })

T2-09: Suppression of Private Member Access in Tests via @ts-expect-error

Rule: Always apply @ts-expect-error to suppress targeted visibility issues in mocks rather than annihilating all type validation by casting to any.

What: When accessing private or static members in unit tests for mocking purposes, use the @ts-expect-error directive instead of casting the entire class or object to any.

Applies To: Unit test setup scripts needing to stub private, protected, or unexported properties on classes or global objects.

Why: Casting objects to 'any' completely stripped all type checking for subsequent operations. Utilizing @ts-expect-error maintains the type context, ensuring the test fails at compile-time if the underlying property's visibility or type changes in the future. Failing to adhere to this typically results in Complete Type Loss.

Trap 1: Casting the class reference or global object to any to bypass TypeScript's visibility and presence checks.

Don't:

// BAD: Overrides all type checking
const libLoader = (GrImageViewer as any).libLoader;
(window as any).resemble = sinon.stub();

Do:

// GOOD: Suppresses visibility error but retains underlying type
// @ts-expect-error
const libLoader = GrImageViewer.libLoader;
// @ts-expect-error
window.resemble = sinon.stub();

T2-10: Enforce Type Contracts Over Redundant Runtime Checks

Rule: Must never litter code with redundant falsy checks for arrays/objects explicitly typed as defined; fix non-compliant test data upstream.

What: Do not add redundant runtime filtering or null-checks for conditions that the TypeScript definitions explicitly forbid. Fix the upstream source or test data instead.

Applies To: Business logic and data processing on strictly typed models.

Why: Adding defensive runtime checks for strictly-typed data degraded readability and masked underlying flaws in test setups that were passing invalid, poorly-mocked data into functions. Failing to adhere to this typically results in Masked Upstream Bugs / Cluttered Logic.

Trap 1: Adding .filter(Boolean) or explicit undefined checks on an array that is strictly typed as containing only defined objects.

Don't:

// Method strictly accepts: changes: ChangeInfo[]
async sync(changes: ChangeInfo[]) {
  // BAD: Redundant runtime check
  const validChanges = changes.filter(c => c && isChangeInfo(c));
  const basicChanges = new Map(validChanges.map(c => [c.id, c]));
}

Do:

// GOOD: Trust the contract and fix failing tests upstream
async sync(changes: ChangeInfo[]) {
  const basicChanges = new Map(changes.map(c => [c.id, c]));
}

Exceptions: External payloads lacking robust typing across integration boundaries.

T2-11: Explicit TypeScript Access Modifiers

Rule: Always codify class visibility through strict private, protected, or public modifiers instead of falling back on leading underscore conventions.

What: Class members and methods intended for internal use must be enforced using the TypeScript private access modifier rather than relying on the legacy _ (underscore) naming convention.

Applies To: TypeScript classes and Web Component definitions.

Why: The codebase contained legacy properties like _loading and methods without explicit access modifiers, which failed to utilize the TypeScript compiler to prevent external dependencies from coupling to internal implementation details. Failing to adhere to this typically results in Broken Encapsulation.

Trap 1: Prefixing internal class methods or properties with an underscore while leaving them functionally public.

Don't:

// BAD: Relying on naming conventions for privacy
@state()
_loading = true;

renderBoolean() { ... }

Do:

// GOOD: Enforcing privacy with TypeScript modifiers
@state()
private loading = true;

private renderCheckmark() { ... }

T2-12: Idiomatic Boolean Casting

Rule: Always cast optional or truthy/falsy values using the idiomatic double-negation operator (!!) rather than the nullish coalescing operator (?? false).

What: To cast an optionally undefined or truthy/falsy value to a strict boolean, use the double-negation operator (!!) instead of the nullish coalescing operator (?? false).

Applies To: TypeScript logic handling optional object properties or API responses.

Why: During permission evaluation, access?.is_owner ?? false was flagged during review as unidiomatic, and the codebase was aligned to use standard double-negation for boolean casts. Failing to adhere to this typically results in Unidiomatic Code.

Trap 1: Casting a potentially undefined boolean to a strict boolean using the nullish coalescing operator.

Don't:

// BAD: Verbose and unidiomatic casting
this.isProjectOwner = access?.is_owner ?? false;

Do:

// GOOD: Concise, standard boolean cast
this.isProjectOwner = !!access?.is_owner;

Cross-Domain Dependencies

Upstream: T1 | Lit Framework Idioms & State Encapsulation - TypeScript strictness principles govern the visibility and structural integrity of Lit component state variables and properties.
Downstream: T3 | Hermetic Testing & Visual Regression - Strict typing directly dictates the syntax and permitted mocking strategies for test stubs and fixtures.

Chapter: Hermetic Testing & Visual Regression

Context: This chapter mandates the strict isolation of unit tests, comprehensive visual validation using full shadow DOM snapshots, and the centralization of test data generation to guarantee hermetic execution and prevent false-positive assertions.

Summary

Truncated - read the full file at https://github.com/GerritCodeReview/gerrit/blob/48f79e2c974b35dfea9abed3583bb134a96af707/configs/skills/gerrit_frontend_engineering/SKILL.md.

About

Frontend Engineering & UI Development Engineering Guide

Executive Summary

Summary

Chapter: Lit Framework Idioms & State Encapsulation

Summary

Rules

T1-01: Separation of Data and UI State Variables

T1-02: Explicit Boolean Properties over Dynamic Slot Detection

T1-03: Declarative Event Listeners in Lit Templates

T1-04: Declarative Conditional CSS via classMap

T1-05: Centralized Periodic LitElement Updates via Generic Utility

T1-06: Declarative Attribute Reflection in Lit Components

T1-07: Idempotent Telemetry Reporting in Lit Component Lifecycle Hooks

T1-08: Strict Truthiness Checks for Asynchronous Array Data

T1-09: Pre-computing Derived State in Lit Lifecycle Methods

T1-10: Returning nothing for Empty Templates in Lit

T1-11: Single-Pass Initialization of Reactive Properties

T1-12: Guarding Asynchronous State Updates Post-Disconnection

T1-13: Use Lit classMap Directive for Conditional CSS Classes

T1-14: Strict Lit State Encapsulation

T1-15: Declarative Component Visibility Toggling

T1-16: Immutable Lit Form State Resets

Cross-Domain Dependencies

Chapter: TypeScript Strictness & Type Safety

Summary

Rules

T2-01: Targeted Type Casting over Broad Error Suppression

T2-02: Removal of Underscore Prefixes for Reactive Properties

T2-03: Elimination of TypeScript Compiler Suppressions in Unit Tests

T2-04: Utilizing TypeScript Utility Types over Unsafe Casting

T2-05: Prototype Methods over Arrow Function Properties

T2-06: Elimination of Unsafe any Type Casts

T2-07: Testing Private State Encapsulation Rules

T2-08: Eliminate Unsafe 'any' Type Casting in Test Stubs

T2-09: Suppression of Private Member Access in Tests via @ts-expect-error

T2-10: Enforce Type Contracts Over Redundant Runtime Checks

T2-11: Explicit TypeScript Access Modifiers

T2-12: Idiomatic Boolean Casting

Cross-Domain Dependencies

Chapter: Hermetic Testing & Visual Regression

Summary

Use it

Manifest

Reviews

T1-10: Returning `nothing` for Empty Templates in Lit

T2-06: Elimination of Unsafe `any` Type Casts