name	gerrit-frontend-engineering
description	Provides guidance and best practices on Polygerrit UI development, frontend architecture, and TypeScript/JS coding standards in Gerrit.

Frontend Engineering & UI Development Engineering Guide

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

Rules

T3-01: Strict DOM Query Assertions in Testing

Rule: Always use the queryAndAssert utility to locate and verify DOM elements simultaneously in unit tests.

What: Use the queryAndAssert utility to locate DOM elements in unit tests instead of using query coupled with manual truthiness assertions or optional chaining.

Applies To: Frontend Web Component testing; specifically LitElement test fixtures interacting with the DOM.

Why: Using a standard query that returns null when an element is missing causes cryptic TypeErrors later in the test execution. Asserting immediately provides clear, fail-fast test reporting. Failing to adhere to this typically results in Test TypeErrors / Cryptic Failures.

Trap 1: Querying for an element and manually asserting its existence using an assertion library.

Don't:

const warning = query(element, '.expensiveDiff');
assert.isOk(warning);
assert.include(warning.textContent, 'Diff too expensive');

Do:

const warning = queryAndAssert(element, '.expensiveDiff');
assert.include(warning.textContent, 'Diff too expensive');

Trap 2: Using optional chaining to silently swallow null elements when checking class lists.

Don't:

const diffContainer = query(element, '.diffContainer');
assert.isTrue(diffContainer?.classList.contains('hidden'));

Do:

const diffContainer = queryAndAssert(element, '.diffContainer');
assert.isTrue(diffContainer.classList.contains('hidden'));

T3-02: Shadow DOM Event Property Fallbacks for Tests

Rule: Must implement explicit attribute fallbacks when reading custom event properties that fail to synchronize in hermetic test environments.

What: When handling custom event properties (like value from custom md-outlined-select components) that fail to reliably synchronize to the target's property in the testing environment, safely fallback to reading the value attribute.

Applies To: Lit element event handlers dealing with custom web components and executed in hermetic testing environments.

Why: Automated tests simulating user selections in md-outlined-select were failing because the testing framework failed to read the value from the event target property, requiring an explicit fallback to the attribute. Failing to adhere to this typically results in False Negative Test Failures.

Trap 1: Relying exclusively on the element property which may remain empty in test fixtures.

Don't:

@change=${(e: Event) => {
  this.selectedLabelForVote = (e.target as HTMLSelectElement).value;
}}

Do:

@change=${(e: Event) => {
  // TODO: Remove reading from attribute once test env issue is fixed
  this.selectedLabelForVote =
    ((e.target as HTMLSelectElement).value ||
    (e.target as HTMLSelectElement).getAttribute('value')) ?? '';
}}

T3-03: Comprehensive Shadow DOM Snapshot Assertions

Rule: Never use shallow DOM assertions; always validate UI structures using assert.shadowDom.equal.

What: UI component tests must use assert.shadowDom.equal to validate the entire rendered structure against an expected HTML snapshot, rather than asserting shallow DOM properties.

Applies To: Frontend UI unit testing of Lit web components.

Why: During refactoring, comprehensive DOM checks were accidentally replaced with shallow element counts, creating a blind spot where regressions in structural layout, attributes, and text content could slip past CI. Failing to adhere to this typically results in Missed UI Regressions.

Trap 1: Checking the length of elements matching a query selector to verify UI rendering.

Don't:

// BAD: Shallow DOM assertion
const flowElements = element.shadowRoot!.querySelectorAll('.flow');
assert.equal(flowElements.length, 2);

Do:

// GOOD: Full Shadow DOM snapshot assertion
assert.shadowDom.equal(
  element,
  /* HTML */ `
    <div class="container">
      <div class="flow">...</div>
      <div class="flow">...</div>
    </div>
  `
);

T3-04: Centralized Test Data Generation via Factory Helpers

Rule: Always construct mock data structures using centralized factory helpers configured with Partial<T> overrides.

What: Avoid manually instantiating large mock data payloads. Utilize centralized factory functions that accept Partial<T> and provide sensible defaults, eliminating manual type assertions.

Applies To: Frontend unit tests and mock data setup blocks.

Why: Tests repeatedly hardcoded large data objects, making the test suite brittle to schema changes and requiring explicit type casting (as FlowInfo) to bypass compiler complaints about missing fields. Failing to adhere to this typically results in Brittle Tests.

Trap 1: Manually creating partial data structures in tests and using type casting to satisfy the compiler.

Don't:

// BAD: Manual object creation with casting
const flow = {
  uuid: 'flow1',
  owner: {name: 'owner1'},
  created: '2025-01-01T10:00:00.000Z' as Timestamp,
} as FlowInfo;

Do:

// GOOD: Using a test data generator with Partial overrides
const flow = createFlow({
  uuid: 'flow1',
  owner: {name: 'owner1', _account_id: 1 as AccountId},
});

T3-05: Isolation of Visual Regression Tests

Rule: Must isolate DOM snapshot and screenshot operations into parallel _screenshot_test.ts files.

What: Visual regression (screenshot) tests utilizing DOM snapshots must reside in a separate _screenshot_test.ts file rather than being appended to standard unit test files.

Applies To: Test directory structure and files utilizing visualDiff.

Why: Combining fast unit tests with slow visual diffing tests bloated unit execution times and complicated testing step separation in the CI pipeline. Failing to adhere to this typically results in CI Bottleneck.

Trap 1: Adding a visualDiff snapshot assertion inside standard logical unit tests.

Don't:

Putting visual visualDiff test blocks into the standard gr-[component]_test.ts file alongside business logic tests.

Do:

Creating a parallel gr-[component]_screenshot_test.ts dedicated exclusively to visual assertions.

T3-06: Awaiting Asynchronous Render Cycles Before Assertions

Rule: Always await element.updateComplete prior to evaluating DOM nodes following a UI state change.

What: Tests asserting UI state changes must strictly await element.updateComplete before querying DOM elements or their properties, avoiding false positive evaluations.

Applies To: Lit web component unit tests.

Why: Tests were erroneously passing because asynchronous state updates lacked await, causing assertions to execute and pass before the updated component render cycle actually fired. Failing to adhere to this typically results in False Positive Tests.

Trap 1: Triggering a UI update and immediately asserting on the resulting DOM without waiting for the Lit rendering engine lifecycle.

Don't:

// BAD: Synchronous assertion after state change
element.renderInOrder([{path: 'p2'}]);
assert.equal(reviewStub.callCount, 1);

Do:

// GOOD: Awaiting update completion
await element.renderInOrder([{path: 'p2'}]);
await element.updateComplete;
assert.equal(reviewStub.callCount, 1);

T3-07: DRY Centralization of Shared Test Setup Logic

Rule: Must extract repeated setup mechanisms and multi-step UI sequences into centralized, shared asynchronous helpers.

What: Repeated interaction sequences required to establish a test's initial state (e.g., clicking menus, awaiting cycles, querying elements) must be extracted into asynchronous helper functions.

Applies To: Component test suites with repetitive, multi-step UI interaction setups.

Why: Tests were repeatedly copying and pasting the multi-step DOM interaction needed to open a modal and fetch an input reference, inflating the codebase and making structural changes painful. Failing to adhere to this typically results in High Maintenance Burden.

Trap 1: Duplicating 5+ lines of UI interaction setup across multiple test cases.

Don't:

Copying and pasting the same set of element lookups, simulated clicks, and await element.updateComplete; lines into every test block.

Do:

Extracting the setup steps into a shared, typed async helper (e.g., async function openLinkDialogAndGetInput(): Promise<HTMLInputElement>) and calling it in each test.

T3-08: Avoid Monkey-Patching Test-Only Properties on Component Instances

Rule: Never mutate component instances with custom, untyped tracking properties; track test states using locally scoped variables.

What: Avoid mutating component instances with test-only custom properties to track state during tests. Utilize locally scoped variables inside the test setup or perform proper stub verification instead.

Applies To: Unit testing, specifically when stubbing instance methods or external calls.

Why: Attaching custom test-only properties (like tracking URLs directly on the element) polluted the component object model, bypassed encapsulation, and risked state leakage between unit tests. Failing to adhere to this typically results in Test State Leakage.

Trap 1: Assigning a random, dynamic property to the component being tested to track mock responses or internal states.

Don't:

.callsFake(function (this: MyComponent, arg: any) {
  // BAD: Modifying the instance for testing purposes
  (this as any).__test_url = arg;
  return 'data:image...';
});
// ... later in the test ...
assert.equal((element as any).__test_url, expectedUrl);

Do:

let generatedUrl: string;
.callsFake(function (this: MyComponent, arg: unknown) {
  // GOOD: Using a scoped variable for tracking
  generatedUrl = arg;
  return 'data:image...';
});
// ... later in the test ...
assert.equal(generatedUrl, expectedUrl);

T3-09: Explicit Mocking of External Network Assets in Tests

Rule: Must completely stub remote library calls or image-fetching mechanisms to execute entirely in isolation.

What: External libraries that trigger network requests (e.g., fetching scripts or image diffing workers) must be fully mocked in local test setups to prevent 404 network errors and ensure hermetic execution.

Applies To: Unit testing for components integrating with third-party, dynamically loaded libraries.

Why: Failure to mock external dependencies caused the test runner to attempt fetching remote or non-existent local assets, resulting in 404 console errors, flaky tests, and significantly slower execution times. Failing to adhere to this typically results in 404 Network Errors.

Trap 1: Allowing the component to natively load its third-party script dependencies during a basic unit test fixture setup.

Don't:

setup(async () => {
  // BAD: Component attempts to download external libraries over the network
  element = await fixture(`<my-viewer></my-viewer>`);
});

Do:

setup(async () => {
  // GOOD: Mock external library loaders to resolve instantly
  // @ts-expect-error
  const libLoader = MyViewer.libLoader;
  sinon.stub(libLoader, 'getLibrary').resolves();

  // @ts-expect-error
  window.externalLib = sinon.stub().returns({ ... });

  element = await fixture(`<my-viewer></my-viewer>`);
});

T3-10: Strict Limits on Test Timeouts for Visual Regressions

Rule: Never augment native test timeouts (this.timeout()) as a workaround for slow rendering components or unreliable baselines.

What: Do not artificially inflate test timeouts (e.g., Mocha this.timeout()) to mask slow component rendering or flakiness in visual regression tests. Maintain default timeouts.

Applies To: Screenshot testing and visual regression test suites.

Why: Artificially extending timeouts allowed significant performance regressions in UI rendering to go unnoticed, as tests would wait excessively long for slow components to stabilize rather than failing fast. Failing to adhere to this typically results in Masked Performance Regressions.

Trap 1: Increasing the Mocha test suite timeout inside a failing screenshot test to force it to pass.

Don't:

suite('component screenshot tests', function () {
  this.timeout(4000);
  // ... test body ...
});

Do:

suite('component screenshot tests', () => {
  // Default timeout ensures fast failure if rendering degrades
  // Update screenshots with CLI commands if baseline legitimately changed
  // ... test body ...
});

T3-11: Realistic Component Initialization in Tests

Rule: Must initialize required component properties to functional values prior to asserting against the shadow DOM.

What: Unit tests that assert against a component's shadow DOM must initialize the component with realistic property states, ensuring the DOM is not trivially empty or unrendered.

Applies To: Lit component tests doing Shadow DOM assertions.

Why: Asserting against an empty or completely uninitialized DOM resulted in false-positive test passes that failed to verify any actual template or structural logic. Failing to adhere to this typically results in False Positive Tests.

Trap 1: Rendering a generic template without required inputs and asserting the shadow DOM is empty.

Don't:

test('render', async () => {
  await element.updateComplete;
  assert.shadowDom.equal(element, '');
});

Do:

test('render', async () => {
  element.name = 'Test';
  await element.updateComplete;
  assert.shadowDom.equal(
    element,
    `<div><h3 class="title">Test</h3></div>`
  );
});

T3-12: Descriptive and Contextual Test Case Naming

Rule: Always name test cases with strict, functional "renders [state] if [condition]" descriptions.

What: Test cases must use descriptive 'renders X if Y' naming structures rather than relying on abstract, ambiguous, or overly technical jargon that lacks functional context.

Applies To: Unit test descriptions (the string argument in test() or it()).

Why: Ambiguous test names using mathematical or overly specific internal jargon made it difficult for developers to deduce the functional intent of the test upon failure without reading the implementation. Failing to adhere to this typically results in Unclear Test Intent.

Trap 1: Naming a test based on an abstract concept rather than the behavioral condition.

Don't:

test('render attempt ordinal', async () => { ... });

Do:

test('renders attempt number if not single attempt', async () => { ... });

T3-13: Production-Aligned Visual Regression Mock Data

Rule: Must guarantee that mock datasets used in visual regressions bypass data points filtered out by production pipeline logic.

What: Mock data structures used to drive screenshot/visual regression tests must precisely mirror the data shapes and filtering logic deployed in the production frontend.

Applies To: Frontend visual regression tests (*_screenshot_test.ts) and mock data fixtures.

Why: Visual tests previously hardcoded reference types that were intentionally stripped out by frontend plugins in production. This resulted in visual tests asserting on UI components that users would never actually see. Failing to adhere to this typically results in False Positive Tests.

Trap 1: Defining static mock data for visual tests that includes properties normally filtered out by the production environment.

Don't:

Defining test data that bypasses standard application filtering logic to artificially inflate component coverage.

Do:

Removing filtered mock references from visual test configurations so the

ملف SKILL.md هذا كبير جدا، لذلك يعرض SkillsMP القسم الاول فقط هنا. عرض على GitHub

name	gerrit-frontend-engineering
description	Provides guidance and best practices on Polygerrit UI development, frontend architecture, and TypeScript/JS coding standards in Gerrit.

Frontend Engineering & UI Development Engineering Guide

Executive Summary

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

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

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

Summary

Rules

T3-01: Strict DOM Query Assertions in Testing

Rule: Always use the queryAndAssert utility to locate and verify DOM elements simultaneously in unit tests.

What: Use the queryAndAssert utility to locate DOM elements in unit tests instead of using query coupled with manual truthiness assertions or optional chaining.

Applies To: Frontend Web Component testing; specifically LitElement test fixtures interacting with the DOM.

Why: Using a standard query that returns null when an element is missing causes cryptic TypeErrors later in the test execution. Asserting immediately provides clear, fail-fast test reporting. Failing to adhere to this typically results in Test TypeErrors / Cryptic Failures.

Trap 1: Querying for an element and manually asserting its existence using an assertion library.

Don't:

const warning = query(element, '.expensiveDiff');
assert.isOk(warning);
assert.include(warning.textContent, 'Diff too expensive');

Do:

const warning = queryAndAssert(element, '.expensiveDiff');
assert.include(warning.textContent, 'Diff too expensive');

Trap 2: Using optional chaining to silently swallow null elements when checking class lists.

Don't:

const diffContainer = query(element, '.diffContainer');
assert.isTrue(diffContainer?.classList.contains('hidden'));

Do:

const diffContainer = queryAndAssert(element, '.diffContainer');
assert.isTrue(diffContainer.classList.contains('hidden'));

T3-02: Shadow DOM Event Property Fallbacks for Tests

Rule: Must implement explicit attribute fallbacks when reading custom event properties that fail to synchronize in hermetic test environments.

What: When handling custom event properties (like value from custom md-outlined-select components) that fail to reliably synchronize to the target's property in the testing environment, safely fallback to reading the value attribute.

Applies To: Lit element event handlers dealing with custom web components and executed in hermetic testing environments.

Why: Automated tests simulating user selections in md-outlined-select were failing because the testing framework failed to read the value from the event target property, requiring an explicit fallback to the attribute. Failing to adhere to this typically results in False Negative Test Failures.

Trap 1: Relying exclusively on the element property which may remain empty in test fixtures.

Don't:

@change=${(e: Event) => {
  this.selectedLabelForVote = (e.target as HTMLSelectElement).value;
}}

Do:

@change=${(e: Event) => {
  // TODO: Remove reading from attribute once test env issue is fixed
  this.selectedLabelForVote =
    ((e.target as HTMLSelectElement).value ||
    (e.target as HTMLSelectElement).getAttribute('value')) ?? '';
}}

T3-03: Comprehensive Shadow DOM Snapshot Assertions

Rule: Never use shallow DOM assertions; always validate UI structures using assert.shadowDom.equal.

What: UI component tests must use assert.shadowDom.equal to validate the entire rendered structure against an expected HTML snapshot, rather than asserting shallow DOM properties.

Applies To: Frontend UI unit testing of Lit web components.

Why: During refactoring, comprehensive DOM checks were accidentally replaced with shallow element counts, creating a blind spot where regressions in structural layout, attributes, and text content could slip past CI. Failing to adhere to this typically results in Missed UI Regressions.

Trap 1: Checking the length of elements matching a query selector to verify UI rendering.

Don't:

// BAD: Shallow DOM assertion
const flowElements = element.shadowRoot!.querySelectorAll('.flow');
assert.equal(flowElements.length, 2);

Do:

// GOOD: Full Shadow DOM snapshot assertion
assert.shadowDom.equal(
  element,
  /* HTML */ `
    <div class="container">
      <div class="flow">...</div>
      <div class="flow">...</div>
    </div>
  `
);

T3-04: Centralized Test Data Generation via Factory Helpers

Rule: Always construct mock data structures using centralized factory helpers configured with Partial<T> overrides.

What: Avoid manually instantiating large mock data payloads. Utilize centralized factory functions that accept Partial<T> and provide sensible defaults, eliminating manual type assertions.

Applies To: Frontend unit tests and mock data setup blocks.

Why: Tests repeatedly hardcoded large data objects, making the test suite brittle to schema changes and requiring explicit type casting (as FlowInfo) to bypass compiler complaints about missing fields. Failing to adhere to this typically results in Brittle Tests.

Trap 1: Manually creating partial data structures in tests and using type casting to satisfy the compiler.

Don't:

// BAD: Manual object creation with casting
const flow = {
  uuid: 'flow1',
  owner: {name: 'owner1'},
  created: '2025-01-01T10:00:00.000Z' as Timestamp,
} as FlowInfo;

Do:

// GOOD: Using a test data generator with Partial overrides
const flow = createFlow({
  uuid: 'flow1',
  owner: {name: 'owner1', _account_id: 1 as AccountId},
});

T3-05: Isolation of Visual Regression Tests

Rule: Must isolate DOM snapshot and screenshot operations into parallel _screenshot_test.ts files.

What: Visual regression (screenshot) tests utilizing DOM snapshots must reside in a separate _screenshot_test.ts file rather than being appended to standard unit test files.

Applies To: Test directory structure and files utilizing visualDiff.

Why: Combining fast unit tests with slow visual diffing tests bloated unit execution times and complicated testing step separation in the CI pipeline. Failing to adhere to this typically results in CI Bottleneck.

Trap 1: Adding a visualDiff snapshot assertion inside standard logical unit tests.

Don't:

Putting visual visualDiff test blocks into the standard gr-[component]_test.ts file alongside business logic tests.

Do:

Creating a parallel gr-[component]_screenshot_test.ts dedicated exclusively to visual assertions.

T3-06: Awaiting Asynchronous Render Cycles Before Assertions

Rule: Always await element.updateComplete prior to evaluating DOM nodes following a UI state change.

What: Tests asserting UI state changes must strictly await element.updateComplete before querying DOM elements or their properties, avoiding false positive evaluations.

Applies To: Lit web component unit tests.

Why: Tests were erroneously passing because asynchronous state updates lacked await, causing assertions to execute and pass before the updated component render cycle actually fired. Failing to adhere to this typically results in False Positive Tests.

Trap 1: Triggering a UI update and immediately asserting on the resulting DOM without waiting for the Lit rendering engine lifecycle.

Don't:

// BAD: Synchronous assertion after state change
element.renderInOrder([{path: 'p2'}]);
assert.equal(reviewStub.callCount, 1);

Do:

// GOOD: Awaiting update completion
await element.renderInOrder([{path: 'p2'}]);
await element.updateComplete;
assert.equal(reviewStub.callCount, 1);

T3-07: DRY Centralization of Shared Test Setup Logic

Rule: Must extract repeated setup mechanisms and multi-step UI sequences into centralized, shared asynchronous helpers.

What: Repeated interaction sequences required to establish a test's initial state (e.g., clicking menus, awaiting cycles, querying elements) must be extracted into asynchronous helper functions.

Applies To: Component test suites with repetitive, multi-step UI interaction setups.

Why: Tests were repeatedly copying and pasting the multi-step DOM interaction needed to open a modal and fetch an input reference, inflating the codebase and making structural changes painful. Failing to adhere to this typically results in High Maintenance Burden.

Trap 1: Duplicating 5+ lines of UI interaction setup across multiple test cases.

Don't:

Copying and pasting the same set of element lookups, simulated clicks, and await element.updateComplete; lines into every test block.

Do:

Extracting the setup steps into a shared, typed async helper (e.g., async function openLinkDialogAndGetInput(): Promise<HTMLInputElement>) and calling it in each test.

T3-08: Avoid Monkey-Patching Test-Only Properties on Component Instances

Rule: Never mutate component instances with custom, untyped tracking properties; track test states using locally scoped variables.

What: Avoid mutating component instances with test-only custom properties to track state during tests. Utilize locally scoped variables inside the test setup or perform proper stub verification instead.

Applies To: Unit testing, specifically when stubbing instance methods or external calls.

Why: Attaching custom test-only properties (like tracking URLs directly on the element) polluted the component object model, bypassed encapsulation, and risked state leakage between unit tests. Failing to adhere to this typically results in Test State Leakage.

Trap 1: Assigning a random, dynamic property to the component being tested to track mock responses or internal states.

Don't:

.callsFake(function (this: MyComponent, arg: any) {
  // BAD: Modifying the instance for testing purposes
  (this as any).__test_url = arg;
  return 'data:image...';
});
// ... later in the test ...
assert.equal((element as any).__test_url, expectedUrl);

Do:

let generatedUrl: string;
.callsFake(function (this: MyComponent, arg: unknown) {
  // GOOD: Using a scoped variable for tracking
  generatedUrl = arg;
  return 'data:image...';
});
// ... later in the test ...
assert.equal(generatedUrl, expectedUrl);

T3-09: Explicit Mocking of External Network Assets in Tests

Rule: Must completely stub remote library calls or image-fetching mechanisms to execute entirely in isolation.

What: External libraries that trigger network requests (e.g., fetching scripts or image diffing workers) must be fully mocked in local test setups to prevent 404 network errors and ensure hermetic execution.

Applies To: Unit testing for components integrating with third-party, dynamically loaded libraries.

Why: Failure to mock external dependencies caused the test runner to attempt fetching remote or non-existent local assets, resulting in 404 console errors, flaky tests, and significantly slower execution times. Failing to adhere to this typically results in 404 Network Errors.

Trap 1: Allowing the component to natively load its third-party script dependencies during a basic unit test fixture setup.

Don't:

setup(async () => {
  // BAD: Component attempts to download external libraries over the network
  element = await fixture(`<my-viewer></my-viewer>`);
});

Do:

setup(async () => {
  // GOOD: Mock external library loaders to resolve instantly
  // @ts-expect-error
  const libLoader = MyViewer.libLoader;
  sinon.stub(libLoader, 'getLibrary').resolves();

  // @ts-expect-error
  window.externalLib = sinon.stub().returns({ ... });

  element = await fixture(`<my-viewer></my-viewer>`);
});

T3-10: Strict Limits on Test Timeouts for Visual Regressions

Rule: Never augment native test timeouts (this.timeout()) as a workaround for slow rendering components or unreliable baselines.

What: Do not artificially inflate test timeouts (e.g., Mocha this.timeout()) to mask slow component rendering or flakiness in visual regression tests. Maintain default timeouts.

Applies To: Screenshot testing and visual regression test suites.

Why: Artificially extending timeouts allowed significant performance regressions in UI rendering to go unnoticed, as tests would wait excessively long for slow components to stabilize rather than failing fast. Failing to adhere to this typically results in Masked Performance Regressions.

Trap 1: Increasing the Mocha test suite timeout inside a failing screenshot test to force it to pass.

Don't:

suite('component screenshot tests', function () {
  this.timeout(4000);
  // ... test body ...
});

Do:

suite('component screenshot tests', () => {
  // Default timeout ensures fast failure if rendering degrades
  // Update screenshots with CLI commands if baseline legitimately changed
  // ... test body ...
});

T3-11: Realistic Component Initialization in Tests

Rule: Must initialize required component properties to functional values prior to asserting against the shadow DOM.

What: Unit tests that assert against a component's shadow DOM must initialize the component with realistic property states, ensuring the DOM is not trivially empty or unrendered.

Applies To: Lit component tests doing Shadow DOM assertions.

Why: Asserting against an empty or completely uninitialized DOM resulted in false-positive test passes that failed to verify any actual template or structural logic. Failing to adhere to this typically results in False Positive Tests.

Trap 1: Rendering a generic template without required inputs and asserting the shadow DOM is empty.

Don't:

test('render', async () => {
  await element.updateComplete;
  assert.shadowDom.equal(element, '');
});

Do:

test('render', async () => {
  element.name = 'Test';
  await element.updateComplete;
  assert.shadowDom.equal(
    element,
    `<div><h3 class="title">Test</h3></div>`
  );
});

T3-12: Descriptive and Contextual Test Case Naming

Rule: Always name test cases with strict, functional "renders [state] if [condition]" descriptions.

What: Test cases must use descriptive 'renders X if Y' naming structures rather than relying on abstract, ambiguous, or overly technical jargon that lacks functional context.

Applies To: Unit test descriptions (the string argument in test() or it()).

Why: Ambiguous test names using mathematical or overly specific internal jargon made it difficult for developers to deduce the functional intent of the test upon failure without reading the implementation. Failing to adhere to this typically results in Unclear Test Intent.

Trap 1: Naming a test based on an abstract concept rather than the behavioral condition.

Don't:

test('render attempt ordinal', async () => { ... });

Do:

test('renders attempt number if not single attempt', async () => { ... });

T3-13: Production-Aligned Visual Regression Mock Data

Rule: Must guarantee that mock datasets used in visual regressions bypass data points filtered out by production pipeline logic.

What: Mock data structures used to drive screenshot/visual regression tests must precisely mirror the data shapes and filtering logic deployed in the production frontend.

Applies To: Frontend visual regression tests (*_screenshot_test.ts) and mock data fixtures.

Why: Visual tests previously hardcoded reference types that were intentionally stripped out by frontend plugins in production. This resulted in visual tests asserting on UI components that users would never actually see. Failing to adhere to this typically results in False Positive Tests.

Trap 1: Defining static mock data for visual tests that includes properties normally filtered out by the production environment.

Don't:

Defining test data that bypasses standard application filtering logic to artificially inflate component coverage.

Do:

Removing filtered mock references from visual test configurations so the

gerrit-frontend-engineering

المزيد من هذا المستودع

المزيد من هذا المستودع

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

Rules

T3-01: Strict DOM Query Assertions in Testing

T3-02: Shadow DOM Event Property Fallbacks for Tests

T3-03: Comprehensive Shadow DOM Snapshot Assertions

T3-04: Centralized Test Data Generation via Factory Helpers

T3-05: Isolation of Visual Regression Tests

T3-06: Awaiting Asynchronous Render Cycles Before Assertions

T3-07: DRY Centralization of Shared Test Setup Logic

T3-08: Avoid Monkey-Patching Test-Only Properties on Component Instances

T3-09: Explicit Mocking of External Network Assets in Tests

T3-10: Strict Limits on Test Timeouts for Visual Regressions

T3-11: Realistic Component Initialization in Tests

T3-12: Descriptive and Contextual Test Case Naming

T3-13: Production-Aligned Visual Regression Mock Data

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

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

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

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

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