review-ui-widgets

---
name: review-ui-widgets
description: >-
  Reviews Chrome DevTools frontend changes in path group ui-and-widgets (^front_end/(ui|panels)/.*\.(ts|css)$),
  strictly reflecting review standards, architectural invariants, and security boundaries
  derived from historical code reviews.
---

# Chrome DevTools frontend reviewer skill: ui-and-widgets

This skill provides review guidance for Chrome DevTools frontend changes matching the `ui-and-widgets` path group (^front_end/(ui|panels)/.*\.(ts|css)$). It focuses specifically on code review verification for UI and widget changes, complementing authoring guidance in `ui-widgets` and [`docs/ui_engineering.md`](../../../docs/ui_engineering.md), and integrating with the general review checklist in `code-review`. Every rule below recurred in at least 5 CLs from 2 distinct reviewers in historical reviews; cite the rule id in findings.

## Path group `ui-and-widgets` (17 rules)

### `UI_MODERN_COMPONENT_AND_ENCAPSULATION_STANDARDS`: Use modern DevTools UI primitives (`devtools-button`, `devtools-data-grid`, `devtools-icon`), #private fields, and module entrypoint imports
- **Severity**: WARNING
- **Why**: Legacy `UI.Fragment`, `DataGridImpl`, `LitElement`, `new Icon()`, and TypeScript `private` fields are superseded by standard `<devtools-*>` custom elements (imported via module entrypoints such as `import * as Buttons from '../../ui/components/buttons/buttons.js'` so custom tags register at runtime) and ECMAScript `#private` fields.
- **Look for**: `UI.Fragment.build`, legacy `DataGridImpl`, `extends LitElement`, `private ` keyword instead of `#` in new UI classes, or `<devtools-*>` tags in Lit templates without importing the corresponding component module entrypoint (e.g. `import * as Buttons from '../../ui/components/buttons/buttons.js'`).
- **Do not flag**: Overriding inherited `protected` lifecycle methods on framework base classes (e.g. extending `UI.Widget.Widget`).

### `UI_TYPE_SAFETY_AND_EXHAUSTIVENESS`: Model multi-state UI workflows with discriminated unions, Platform.assertNever exhaustiveness checks, and strict domain types
- **Severity**: WARNING
- **Why**: Representing mutually exclusive UI states with loose optional boolean flags or `any` types permits impossible states and silently misses new enum variants. Discriminated unions paired with `Platform.assertNever` in `default:` branches enforce compile-time exhaustiveness.
- **Look for**: `switch` statements over union/enum types missing `Platform.assertNever(x)` in `default:`, `any` or `declare` field modifiers in UI classes, or multiple boolean flags (`isLoading`, `isError`, `isEmpty`) representing mutually exclusive states.
- **Do not flag**: Switch statements that intentionally handle only a small subset of a large external protocol enum.

### `UI_PURE_VIEW_AND_PRESENTER_SEPARATION`: Keep UI.Widget view functions pure and stateless by computing presentation data in presenters and passing decoupled ViewInput data structures
- **Severity**: WARNING
- **Why**: In DevTools' MVP architecture, `DEFAULT_VIEW` functions must be pure projections of `ViewInput` to Lit templates. Querying SDK models, reading global settings, mutating presenter state, or calling host/clipboard APIs inside the view couples rendering to global state and breaks isolated view testing.
- **Look for**: `DEFAULT_VIEW` or view helper functions calling `Common.Settings.Settings.instance()`, `SDK.*`, `Host.*`, mutating `input.*` arrays in place, or accepting raw `SDK`/`Protocol` domain objects instead of decoupled presentation interfaces on `ViewInput`.
- **Do not flag**: Legacy `UI.Widget` subclasses that have not yet migrated to a `DEFAULT_VIEW` function.

### `UI_LIT_DECLARATIVE_TEMPLATES_AND_DIRECTIVES`: Use Lit.nothing, repeat with keys, classMap/styleMap, and ifDefined in single-pass declarative templates instead of imperative DOM mutations
- **Severity**: WARNING
- **Why**: Mutating DOM elements via `appendChild`, `classList.toggle`, or `innerHTML` alongside Lit templates, or returning `''`/`null` instead of `Lit.nothing` in conditional branches, corrupts Lit's part tracking and creates unnecessary text nodes or wrapper `<div>`s.
- **Look for**: Ternary branches returning `''` or `undefined` in `html` templates instead of `Lit.nothing`, unkeyed `.map()` on dynamic stateful lists instead of `repeat()`, `ifDefined` omissions on optional attributes, or multiple `Lit.render()` calls mutating the same container.
- **Do not flag**: Static arrays of primitive labels where item identity never reorders.

### `UI_WIDGET_DOM_AND_LISTENER_LIFECYCLE`: Attach listeners to external models and managers in wasShown and detach them symmetrically in willHide
- **Severity**: ERROR
- **Why**: Subscribing to long-lived SDK `TargetManager`, `Settings`, or model events in a widget's constructor without unsubscribing in `willHide()` (or `onDetach()` / `onunbind()`) causes hidden panels to process background updates and leaks detached widget instances.
- **Look for**: `addEventListener`, `addModelListener`, or `addChangeListener` on singletons, models, or settings inside `UI.Widget` constructors or `wasShown()` without a matching `remove*` call in `willHide()` using the same function reference.
- **Do not flag**: Listeners attached to the widget's own internal DOM elements (`this.contentElement`), which are garbage-collected together with the widget.

### `UI_ACCESSIBILITY_AND_SEMANTIC_HTML`: Use native semantic HTML controls with accurate ARIA attributes, keyboard handlers, and !event.isComposing guards
- **Severity**: WARNING
- **Why**: Clickable `<div>`/`<span>` elements and nested interactive controls (`<button>` inside `<a>` or `<button>`) break keyboard navigation and screen readers. Interactive elements require semantic tags (`<button>`, `<details>`/`<summary>`, `<thead>`/`<tbody>`), synchronized ARIA state (`aria-expanded`, `aria-details` for rich tooltips vs `aria-describedby` for plain text), and `!event.isComposing` checks on `Enter` keydown handlers.
- **Look for**: `@click` on `<div>`/`<span>` without button semantics, `<button>` nested inside `<button>`/`<x-link>`, `Enter` keydown handlers missing `!event.isComposing`, or `aria-describedby` pointing to interactive `<devtools-tooltip>` elements.
- **Do not flag**: Delegating click listeners on container elements that handle bubbled events from child buttons.

### `UI_PERFORMANCE_AND_FLAMECHART_RENDERING`: Precompute expensive metrics outside render loops, use Set/Map for O(1) lookups, and debounce high-frequency UI updates
- **Severity**: WARNING
- **Why**: Performing linear `Array.includes`/`find` scans, sorting, or regex parsing inside FlameChart canvas draw loops, `ElementsTreeOutline` mutation callbacks, or Lit render passes causes frame drops and UI jank on large traces and DOM trees.
- **Look for**: `.includes()` / `.find()` / `.sort()` inside canvas draw or event iteration loops, repeated `ResizeObserver` registrations on individual child items instead of the parent container, or un-debounced filter/mutation handlers.
- **Do not flag**: One-time initialization passes over bounded small arrays (less than 20 items).

### `UI_LOCALIZATION_AND_UISTRINGS_HYGIENE`: Define user-facing strings in sentence-case UIStrings using complete sentences and i18nTemplate for embedded elements, and prune unused entries
- **Severity**: WARNING
- **Why**: Concatenating translated fragments breaks word order in non-English locales, title-Cased strings violate Chromium UX standards, translating English-only experimental strings wastes localization quota, and unused `UIStrings` entries bloat locale bundles.
- **Look for**: String concatenation around `i18nString(...)`, HTML/DOM elements interpolated without `i18nTemplate`, Title Case labels in `UIStrings`, unlocalized English literals in templates, or unused keys in `UIStrings`.
- **Do not flag**: Technical protocol identifiers, MIME types, or strings in `UIStringsNotTranslate` / `i18n.i18n.lockedString(...)`.

### `UI_REACTIVE_WIDGET_UPDATE_LIFECYCLE`: Schedule widget re-renders asynchronously via this.requestUpdate() and guard property setters with equality checks
- **Severity**: WARNING
- **Why**: Calling `this.performUpdate()` or invoking `this.#view(...)` synchronously inside property setters causes redundant synchronous DOM renders when multiple properties change in the same tick. Setters should short-circuit if the value is unchanged and call `this.requestUpdate()`.
- **Look for**: Widget property setters calling `this.performUpdate()` or `this.#view(...)` directly, or setters calling `this.requestUpdate()` without checking `if (this.#prop === value) return;`.
- **Do not flag**: Internal test helpers or synchronous initialization paths explicitly documented as requiring immediate flush.

### `UI_VISUAL_LOGGING_INSTRUMENTATION`: Instrument interactive UI elements and containers with kebab-case VisualLogging contexts registered in KnownContextValues
- **Severity**: WARNING
- **Why**: DevTools relies on Visual Elements (`jslog`) telemetry to measure feature usage. Omitting `jslog` on new buttons/toggles/menus, using localized or camelCase context strings, adding redundant panel prefixes inside already-logged containers, or dropping `jslog` during Lit refactors breaks analytics.
- **Look for**: Interactive elements (`<button>`, `<devtools-button>`, `<input>`, menus, drawers) missing `jslog` or `jslogContext`, non-kebab-case context names, or new context values missing from `KnownContextValues.ts`.
- **Do not flag**: Purely decorative icons or internal wrapper `<div>`s that represent neither a distinct visual section nor an interactive control.

### `UI_CSS_SCOPING_AND_FLEX_LAYOUT`: Scope light-DOM stylesheets and align elements using flexbox or grid rather than competing margin and position offsets
- **Severity**: INFO
- **Why**: Unscoped class or tag selectors in light-DOM widget stylesheets leak across the DevTools document, while combining `vertical-align`, negative margins, and `position: relative` offsets creates brittle layouts that break under font scaling or zoom.
- **Look for**: Unscoped generic selectors in light-DOM `.css` files (or `:host` inside `@scope` blocks instead of `:scope`), or CSS rules combining `vertical-align`, negative `margin-top`, and pixel offsets to center icons and buttons.
- **Do not flag**: Shadow DOM component stylesheets where `:host` scoping is native.

### `UI_VIEW_OUTPUT_AND_CALLBACK_CONTRACTS`: Encapsulate imperative DOM actions in ViewOutput callbacks and pass user actions via semantic ViewInput callbacks instead of raw DOM references
- **Severity**: WARNING
- **Why**: Exposing raw `HTMLElement` references on `ViewOutput` or passing raw `MouseEvent`/`KeyboardEvent` objects to presenter callbacks leaks DOM details into presenters. Views should populate narrow functional closures on `ViewOutput` (e.g. `output.focusInput = () => ...`) and unpack event values before calling `input.onSelect(value)`.
- **Look for**: `ViewOutput` interfaces containing `HTMLElement` or `Element` properties, or `ViewInput` callbacks typed as `(event: Event) => void` instead of semantic domain values.
- **Do not flag**: Third-party editor or canvas host containers that require a raw mount `HTMLElement`.

### `UI_PANEL_AND_META_REGISTRATION`: Follow DevTools panel directory conventions and register settings and views in the appropriate *-meta.ts scope
- **Severity**: WARNING
- **Why**: Panel-level `UI.Widget` views belong directly under `front_end/panels/<panel>/` (reserving `components/` for reusable leaf components), internal non-user-facing settings must use `SettingCategory.NONE` or model descriptors rather than appearing in Settings UI, and shared SDK settings belong in `sdk-meta.ts`.
- **Look for**: Panel-level `UI.Widget` views placed inside `panels/<panel>/components/`, internal state flags registered with user-facing titles in `*-meta.ts`, or `loadView` callbacks calling `.instance()` instead of using the passed `Universe` context.
- **Do not flag**: Standalone reusable leaf components inside `components/` or genuine user-configurable preferences.

### `UI_WEAK_DOM_REFERENCES_AND_OBSERVER_CLEANUP`: Store DOM node metadata in WeakMap or WeakRef collections and clean up observers when elements detach
- **Severity**: WARNING
- **Why**: Retaining DOM `Element` or `SDK.Target` references in strong `Map`/`Set` collections (or in `WeakMap` values that hold strong references back to their keys) prevents detached DOM trees from being garbage-collected, while leaving `ResizeObserver`, `MutationObserver`, or `Lit.Directives.ref` callbacks active after unmount leaks memory.
- **Look for**: Strong `Map<Element, ...>` / `Set<Element>` collections holding DOM/model associations, `WeakMap` values retaining strong references back to their DOM keys, `ResizeObserver`/`IntersectionObserver` without `.unobserve()`/`.disconnect()` on teardown, or `ref(el => ...)` callbacks that do not handle `el === undefined`.
- **Do not flag**: Module-level `WeakMap<Element, ...>` globals whose values do not strongly reference the key, or short-lived local maps created and discarded within a single synchronous function call.

### `UI_SECURITY_AND_OUTPUT_SANITIZATION`: Sanitize untrusted strings, URLs, CSS selectors, and Markdown exports before rendering or exporting
- **Severity**: ERROR
- **Why**: Interpolating untrusted network URLs, DOM attributes, or user strings into raw HTML snippets, `CSS.escape`-free attribute selectors, Markdown table exports, or `href` attributes without protocol validation risks injection, XSS, broken layout formatting, or text spoofing.
- **Look for**: Raw string interpolation in `querySelector('[attr="' + val + '"]')` without `CSS.escape`, `href` attribute bindings with unvalidated `javascript:` URLs, Markdown table cell serialization without escaping `|*_<>\`, or raw HTML snippet generation without HTML escaping.
- **Do not flag**: Standard Lit `html` text bindings (`${text}`), which escape HTML entities automatically.

### `UI_DECLARATIVE_CHILD_WIDGET_COMPOSITION`: Compose child UI.Widget instances declaratively in Lit templates using the widget() directive instead of imperative show() calls
- **Severity**: WARNING
- **Why**: Imperatively instantiating child widgets and calling `child.show(container)` inside presenters or views bypasses Lit reconciliation, complicates lifecycle management, and recreates widgets unnecessarily across renders.
- **Look for**: `new ChildWidget()` followed by `.show(...)` or `.appendChild(child.element)` inside Lit view functions or migrated `performUpdate()` methods instead of `<devtools-widget ${widget(ChildWidget, {...})}></devtools-widget>` (or `${widget(ChildWidget, {...})}`).
- **Do not flag**: Top-level legacy split widgets or tabbed panes that have not migrated to Lit views.

### `UI_AWAIT_UPDATE_COMPLETE_POST_RENDER`: Await this.updateComplete before executing post-render DOM measurements, focus changes, or scroll adjustments
- **Severity**: ERROR
- **Why**: Because `this.requestUpdate()` schedules rendering asynchronously in a microtask, querying DOM geometry, focusing newly rendered controls, or anchoring popovers immediately after `requestUpdate()` operates on stale pre-render DOM state.
- **Look for**: `this.requestUpdate()` followed synchronously by `.focus()`, `.getBoundingClientRect()`, `.scrollIntoView()`, or child widget measurement without `await this.updateComplete`.
- **Do not flag**: State updates that do not interact with rendered DOM nodes after scheduling the update.

More agent context in ChromeDevTools/devtools-frontend

21 other files this repository gives its agents.