# Perfetto UI Review Antipatterns This is a canonical catalogue of UI antipatterns and the preferred alternatives, **mined from real code-review feedback** that maintainers repeatedly gave on `ui/src/` pull requests authored by other contributors. **Purpose:** it is intended to be read by an AI agent (or human) *at code-review or code-authoring time* so we can pre-empt the mistakes and invalid assumptions that recur across contributions, instead of relitigating them PR by PR. How to use it: - Each item is phrased as **❌ antipattern → ✅ fix**, with a one-line *why* and one or more provenance PR numbers (`#NNNN`) you can look up for full context. - 🔁 marks rules that recurred across **many** PRs — weight these most heavily; they are the maintainers' strongest, most-repeated preferences. - This complements (and goes deeper than) the "Code Review Pet Peeves" section in [`AGENTS-ui.md`](AGENTS-ui.md). When they overlap, both are authoritative. > Meta-principle that underlies almost everything below: **don't over-engineer, > reuse what exists, keep state in one place, and make failures visible.** When > in doubt, prefer the simpler, smaller, more boring change. --- ## 1. Mithril & Rendering - 🔁 **❌ Calling `m.redraw()` / `raf.scheduleFullRedraw()` manually → ✅ rely on autoredraw.** Mithril automatically schedules a redraw after (1) DOM event handlers it registered and (2) trace-processor queries completing. Manual redraws are only needed for genuinely out-of-band async (`setTimeout`, `fetch`, `yield`). The *vast majority* of manual redraws in submitted code are unnecessary — delete them. If you think you need one, first find out *why* the UI isn't updating; it usually points to a real bug elsewhere, not a missing redraw. When you do need it, call `m.redraw()` at the point the event happens (it doesn't depend on `app`), not `app.raf.scheduleFullRedraw()`. *(#4769, #4464, #4357, #4226, #3999, #3693, #3124, #2615, #1097, #5595, #5436, #5284, #5243)* - 🔁 **❌ Caching `attrs`, settings, or query results in private member fields → ✅ read them fresh.** Mithril passes `attrs` to `oncreate`/`onupdate`/`view`; use them from there. The settings store is already cache-backed — read the value every access so it can't go stale when the user changes it. A presentation component must tolerate its data source changing every frame. *(#3857, #6031, #4737)* - **❌ `requestAnimationFrame` / `setTimeout` timing hacks hoping the DOM is ready "next frame" → ✅ use a proper post-render callback.** rAF to dodge a `ResizeObserver` loop warning is a code smell. If you need to act after render, expose an `onReady` callback invoked from both `oncreate` and `onupdate`. *(#4761, #4146)* - **❌ Passing `m.Children` (vnodes) into a long-lived / one-shot API (e.g. a modal) → ✅ pass a function returning `m.Children`.** A stored vnode gets reused across render cycles, which is undefined behaviour in Mithril. Pass `() => m.Children` so it's re-invoked each render. Likewise, don't store vnodes in an immer-controlled data model (immer doesn't support vnodes; it breaks undo/redo). *(#4192, #3999)* - **❌ Doing init / async loads in lifecycle hooks (`oncreate`, `onPopupMount`, constructor) when `view()` would do → ✅ compute in `view()`, init once where it belongs.** `onPopupMount` is only for when you genuinely need the popup's DOM element. Constructors run on every remount, so an async load there can fire multiple times — load once in the plugin's `onTraceLoad()` and pass the data in via `attrs`. *(#3817, #1389)* - **❌ Ternary returning `undefined` for conditional children → ✅ short-circuit `cond && m(...)`.** Mithril ignores falsy children. *(#2021, #1389)* - **❌ Unnecessary wrapper `div`s / empty `{}` attrs or `{style:{}}` objects → ✅ apply the class to the existing element and omit empty objects.** *(#5073, #4993, #2039, #1406)* - **❌ `stopPropagation()` *and* `preventDefault()` together "to be safe" → ✅ pick the one you actually need** and justify how it interacts with other handlers. *(#3707)* --- ## 2. State, Data Model & Lifecycle - 🔁 **❌ Module-level globals / static fields / new singletons for "convenient access" → ✅ inject the dependency.** Global mutable state breaks the fact that **multiple UI instances can run on one page**, and creates ordering/maintenance headaches. Plumb state through the `trace`/`app` objects (model it like tabs are modeled), or pass a context/settings object. A plugin class is already effectively a singleton — use `this.member`, not a `static` field. *(#1097, #2036, #5436, #5284, #5286)* - 🔁 **❌ Caching the `Trace` object on a singleton plugin → ✅ pass `Trace` as a function argument.** A cached trace becomes the *wrong* trace after a second trace loads; an in-flight async function then resolves against a stale trace. Initialize per-trace data in `onTraceLoad()` and pass it as `Readonly` args, so you also avoid optional members and `!` assertions everywhere. To attach per-trace data without growing the `Trace` API, use a `WeakMap`. *(#5284, #2900)* - 🔁 **❌ Hand-rolled loading/in-flight booleans and manual change-detection for async data → ✅ use `QuerySlot` (and `AsyncLimiter`).** Selection-driven queries fired straight off events cause unbounded head-of-line blocking when the user clicks faster than queries run. Use a `QuerySlot` (declare a key, poll it each render, render stale data with `retainOn`), or load inside `TrackEventDetailsPanel.load()`, which auto-cancels obsolete loads. Guard concurrency with `AsyncLimiter.isRunning` rather than a bespoke flag. *(#4464, #4582, #4737, #5436, #4192)* - **❌ Keeping derived state that must be manually kept in sync → ✅ derive it from the single source of truth.** Don't reverse-engineer state by matching against options — persist the actual choice (e.g. the preset *name*) and re-apply it. Use one unified selection list for heterogeneous items rather than parallel selection state. *(#3693, #3780, #3999)* - 🔁 **❌ Storing UI timestamps/durations as JS `number` → ✅ use `bigint`.** JS numbers are doubles and lose precision over the large dynamic range of trace time. Type SQL time columns as `LONG`/`LONG_NULL` so they materialize as `bigint` (no manual `BigInt()` / no precision loss), and use `Time.fromRaw()` rather than casts. *(#2503, #4767, #4769)* - **❌ Querying rows by human-readable display name → ✅ query by stable typed columns** (e.g. `type = 'suspend_resume'`, not `name = "Suspend/Resume Latency"`). Display strings are fragile. *(#2149)* - **❌ Forgetting to await for async functions calls in `onActivate()`, `onTraceLoad()` or other lifecycle methods.** It's extremely rare to have a legitimate reason for calling an async method without await or without a .then(), as that creates fire-and-forget semantics that cause races. If really needed, it must be documented with a comment. *(#2353, #1758)* - **❌ Ad-hoc `localStorage` keys for dismissible hints / persisted layout → ✅ use the settings system** (and parse unknown persisted objects with Zod, making newly-added fields optional for backward compatibility). Make banners permanently dismissable; pair a one-time hint with a permanent discoverable button rather than a recurring intrusive banner. *(#5761, #2772, #3660, #3185)* --- ## 3. Architecture & Layering - 🔁 **❌ Vendor / product / plugin-specific code in `src/core` or the `public/` API surface → ✅ push specifics into a plugin; keep the core interface generic.** This is wrong layering. The same applies to chrome: don't add a plugin-specific section to the shared sidebar — keep navigation inside the plugin's own page via subtabs/submenu. Keep plugin-specific constants (track kinds), factories, and schemas inside the owning plugin. *(#5284, #5153, #1624, #1241)* - **❌ Pure, general-purpose library code (track impls, panels) wrapped as a plugin → ✅ put it in `ui/src/components`.** Conversely, code in its own folder should generally *be* a `PerfettoPlugin`; if it isn't, reconsider its placement. Genuinely reusable widgets go in `widgets/`; *(#4767, #5284, #1097)* - **❌ Hidden / incidental cross-plugin dependencies (working "by chance" via load order) → ✅ declare dependencies explicitly**, or move the command into the plugin it conceptually belongs to. Depend on the plugin that *creates* a track before consuming it. Don't take a heavy plugin dependency just to reuse one shared resource — extract it (e.g. `dev.perfetto.StandardGroups`). *(#4613, #2666, #2137)* - **❌ Inheritance / abstract base hierarchies, and one-method "manager" classes → ✅ prefer composition and standalone functions.** A class with one method and no state should just be a function. Remove thin wrappers that only re-export a base helper. *(#4844, #4184)* --- ## 4. Plugin & API Design - 🔁 **❌ Plugin with no / placeholder `description` → ✅ add a real `static readonly description`.** It surfaces on the plugins page and tells users what the plugin does and why they'd enable it. Not a TODO. *(#4716, #2394, #5436, #5284)* - **❌ Manually parsing `location.hash` in a plugin → ✅ use the route-args mechanism** — plugin-prefixed URL params are decoded and passed as the second arg to `onActivate()`. Design URL/query-param formats with standard conventions (repeated keys or comma-separated) and standard percent-encoding; once external users adopt a bespoke format it's an interface you can't change. *(#5019, #2471)* - **❌ Reaching into internal structures (e.g. adding tracks directly to the pinned-tracks node) → ✅ use the high-level API** (`TrackNode.pin()`). Add tracks to the normal workspace then pin, so unpinning leaves them in the tree. Filter tracks by tags (e.g. the `trackIds` tag), don't hand-construct/guess track URIs. *(#4814, #4820, #4524)* - **❌ Reusing an internal core schema for a wire/persisted format → ✅ define the external schema locally.** Reusing it means an unrelated change to the core type silently mutates a contract that's supposed to be stable. *(#4192)* - **❌ Unused abstraction interfaces / speculative callback props → ✅ remove them (YAGNI).** Don't introduce indirection nobody implements, or expose options for unsupported formats and then throw. *(#4767, #5065, #1565)* - **❌ Settings that only take effect after restart with no signal → ✅ set `requiresReload: true`** so the user is prompted to reload. *(#2394, #1298)* - **❌ `has()/get()`-then-mutate, iterate-to-find-first, separate accessors always used together → ✅ use the provided helpers.** `getOrCreate(map, k, () => [])`, `result.maybeFirstRow({...})` (undefined for 0 rows), and fold paired accessors into one function. *(#5284, #4039, #2036)* - **❌ Assuming a stdlib table is available → ✅ add the `INCLUDE PERFETTO MODULE`** the query depends on (e.g. `slices.with_context`). *(#5392)* --- ## 5. Widgets & Reuse > 🔁 **Search the widget library (`ui/src/widgets/`) before building anything.** > "The vast majority of this file could disappear if you use the existing widgets > and theme variables." This is the single most common reuse note. - **Use the existing widget instead of raw HTML or hand-rolled markup:** `Button`/`SegmentedButton` (not `