Add component audit report

docs/component-audit.md

@@ -0,0 +1,183 @@
+# Component audit — gpui-native-kit
+
+Full audit of every component ahead of migrating `signed` onto this kit. Scope: all 7 crates (~21k LOC) — `crates/core` (30 components), `crates/data`, `crates/editor`, `crates/layout`, plus the foundation crates `crates/theme`, `crates/icons`, `crates/utils`, the facade, and every `Cargo.toml` feature graph.
+
+Audit criteria, in priority order: no over-engineering, justified function arguments only, no memory leaks, best performance.
+
+## Method
+
+Seven parallel read-only audits (one per component group) plus a manual audit of the foundation crates. Every gpui-behavior claim was verified against the pinned zed checkout (`504a7f2`, per `Cargo.lock`); every "unused param" claim was backed by workspace-wide greps (`crates/`, `examples/`, `docs/`, `tests/`). All 15 major findings were re-verified by reading the cited lines. No code was modified; static audit only, no tests run.
+
+- Date: 2026-11-09
+- gpui rev: `504a7f2` (zed checkout, per `Cargo.lock`)
+- Revision audited: pre-`signed` migration state
+
+---
+
+## Verdict per criterion
+
+| Criterion | Verdict |
+|---|---|
+| **Memory leaks** | Near-clean. Zero `.detach()` on component tasks/subscriptions anywhere in `crates/` (grep-verified); tasks stored in fields, self-references via `WeakEntity`, undo history bounded (100 entries), icon cache bounded, keyed element state pruned by gpui per frame. **One real leak:** `AnchorBoundsRegistry` in utils (finding 11), plus one stuck-state bug in `resize_handle` (finding 10). |
+| **Over-engineering** | A handful of cases: dead defensive machinery (dialog/command_palette bounds plumbing), drifted duplicate painter (scroll_area), pass-through wrappers (markdown), single-use indirection, one speculative persistence API (resizable). |
+| **Function arguments** | ~20 removal candidates found, all with zero-call-site evidence (list below). Feature graphs and module gating are clean (verified per-file against actual imports). |
+| **Performance** | Three systemic hot-path issues (registry clone churn, uncapped caret blink, per-frame O(N) scan in diff_view) plus a recurring `debug_selector` pattern that allocates every frame in release builds across 7+ sites. |
+
+---
+
+## Major findings (15, all verified)
+
+### A. Shared keyed state across sibling instances — root cause: un-namespaced keys
+
+gpui scopes `use_keyed_state` by the element-id path; a `RenderOnce` renders under its **type name**, so two instances of the same component under the same ancestor chain resolve to the *same* keyed state. Existing tests never catch this class because every test window mounts exactly one instance.
+
+| # | Location | Bug | Fix |
+|---|---|---|---|
+| 1 | `crates/core/src/menu.rs:385,395-396` | `"menu-was-open"` / `"menu-scroll"` / `"menu-selection"` shared by all `Menu`s in a window → a closed sibling resets the open menu's selection every frame; arrow-key navigation and the selection highlight break with 2+ menus (the gallery ships 5 sibling dropdowns at `examples/gallery/src/main.rs:2761-2860`). | Wrap the three calls in `window.with_element_namespace(id.clone(), …)` — the instance `id` is already a parameter. `context_menu.rs:67-77` is the correct in-repo pattern. |
+| 2 | `crates/core/src/dropdown_menu.rs:210` | Same bug: `"dropdown-open-origin"` bare key. | Same namespacing. |
+| 3 | `crates/core/src/slider.rs:143` | `"slider-dragging"` bare key → dragging one slider drives both. | Key by `(self.id.clone(), "slider-dragging")` — the pattern `clipboard.rs:105` already uses. |
+| 4 | `crates/core/src/scrollbar.rs:144,157` | Default key `ElementId::Name("scrollbar")` → two default-keyed scrollbars in one view share drag + autohide state (gallery renders two side by side at `main.rs:3231,3257`). | Make the id a required constructor parameter (`Scrollbar::vertical(id, handle)`) and delete `.key()`; 6 call sites, most already have ids to pass. |
+
+### B. Correctness bugs
+
+| # | Location | Bug | Fix |
+|---|---|---|---|
+| 5 | `crates/core/src/input.rs:2518` (+ `:1802`) | `utf8_range_to_utf16` has a byte-for-byte copy of `utf16_range_to_utf8`'s body — it never converts UTF-8→UTF-16. Every IME selection/marked-range report is wrong for non-ASCII text (CJK/emoji candidate anchoring). `character_index_for_point` likewise returns UTF-8 where the trait contract (gpui `platform.rs`) demands UTF-16. ASCII-only tests cannot catch this. | Implement the real conversion (`encode_utf16().count()`); add a non-ASCII regression test. |
+| 6 | `crates/core/src/combobox.rs:509-518` | Chevron-open with a selected value refilters using the field text — which *is* the selected label — so only the selected option is listed. Inconsistent with `Select` (reopens listing all) and with query-focused open. | `refilter("")` when `state.selected.is_some()` in the chevron branch. |
+| 7 | `crates/core/src/command_palette.rs:481-510` | The modal overlay is not `deferred` — z-order depends on mount position; in the gallery, alerts/toasts paint and receive clicks above the open modal's scrim. Also: the whole `panel_bounds` `Rc<RefCell<Option<Bounds>>>` + capture canvas is dead (the scrim is a sibling of the panel, so its mouse handler cannot fire for panel clicks). | Wrap in `deferred(…)` like `alert.rs:303` / `toast.rs:221`; delete the bounds machinery, keep a bare canvas for the focus-steal paint. |
+| 8 | `crates/editor/src/code_view.rs:351` | Mouse-down maps position with `Pixels::ZERO` for scroll while mouse-move correctly uses `ScrollbarHandle::offset(...).y` → clicking after scrolling selects the wrong row. | Pass the same offset in the down handler. |
+| 9 | `crates/editor/src/code_view.rs:102-108,137` | tree-sitter `old_tree` reused across changed text without `Tree::edit()` — violates the documented parse contract → stale/garbled highlighting after edits (the gallery's Rust→Large sample toggle triggers it). | Delete the `tree` field and the `old_tree` param of `highlight_with_tree`; full reparse per change. Reintroduce incremental parsing only when a real consumer needs it. |
+| 10 | `crates/layout/src/resize_handle.rs:241-268` | The move handler checks only the stored `dragging` flag, never `event.pressed_button`. macOS synthetic drag does not synthesize a `MouseUpEvent` on outside-release → the seam keeps resizing on hover until the next click (stuck interaction state). | In the move handler, clear `dragging` when `event.pressed_button != Some(MouseButton::Left)`. |
+
+### C. Memory / performance
+
+| # | Location | Bug | Fix |
+|---|---|---|---|
+| 11 | `crates/utils/src/lib.rs:40-73` | `AnchorBoundsRegistry`: app-global `HashMap<ElementId, Bounds>` that only inserts (never removes), keyed **without `WindowId`** (a two-window app can read a same-named element's bounds from the other window), and every record **clones the whole map** (`cx.try_global().cloned()` then `set_global`). Every `Button` records per frame (`button.rs:244-248`) → unbounded growth + O(N²)-ish per-frame allocation churn. Consumed by tooltip anchoring. | Key by `(WindowId, ElementId)`; mutate in place instead of clone-and-set (verify the in-place global-update API in the pinned gpui before rewriting). |
+| 12 | `crates/core/src/input.rs:2049-2054` | Caret blink is an uncapped `Animation::new(CARET_BLINK_INTERVAL * 2).repeat()` → full-window repaint + full text re-shaping at display refresh rate whenever a field is focused (~119 wasted repaints/s at 120 Hz). Violates the repo's own `with_max_fps` rule. | `.with_max_fps(4.0)` (blink period is 1 s; `reduce_motion` is already handled by gpui). |
+| 13 | `crates/editor/src/diff_view.rs:332-349` | `widest_row` recomputed with an O(document) scan **every render** (walks every span of every row to feed `with_width_from_item`) — defeats virtualization on large diffs. | Compute once in `new`/`set_versions`, store in a field. |
+| 14 | `crates/data/src/tabs.rs:304,367,404` | Index-keyed element ids on a drag-reorderable strip (dock reorders tabs via `move_panel`) — violates the stable-domain-id rule; ids permute on every drag, re-keying interaction state, the spinner, and the close button. | Add a stable per-tab key (`Tab::new(key, label)` or `.key(SharedString)`); stable keys already exist at every call site (`tab.panel`, labels). The index-based `on_change`/`on_close` API stays unchanged. |
+| 15 | `crates/core/src/menu.rs:461` | Per-frame `Vec<MenuItem>` deep clone into the `on_key_down` closure — every label + handler cloned each frame the panel renders. | One `Rc<[MenuItem]>` per render; clone the `Rc` into the closure. |
+
+---
+
+## Param / dead-API removals
+
+All verified by workspace-wide greps with **zero call sites** outside their own definitions/tests.
+
+| Item | Location | Note |
+|---|---|---|
+| `Slider::value()` | `slider.rs:83-86` | `new` already takes `value` |
+| `Separator::orientation()` | `separator.rs:46-49` | third way to set one enum; only its own test uses it |
+| `Menu checked: Option<bool>` | `menu.rs:38,598` | only ever compared to `Some(true)`; `checked(false)` ≡ unset → plain `bool` (gallery/select call sites compile unchanged) |
+| `Tab::lead` | `tabs.rs:37,67-74,342-357` | zero callers; also silently overrides `icon` when both set |
+| `PanelDescriptor::icon` | `dock.rs:61` | zero of 45 constructor sites use it; delete the 5-hop plumbing (`PanelTab.icon`, `TabView.icon`, snapshot copy, open_panel copy, context-menu branch) |
+| `DropdownMenu::icon()`, `.tab_stop()` | `dropdown_menu.rs:76,88` | untested and unexercised |
+| `Alert::icon()` / `.no_icon()` | `alert.rs:82,88` | severity already derives an icon |
+| `ComboboxChanged` event + `EventEmitter` impl | `combobox.rs:181,212,352` | zero subscribers workspace-wide; `on_change` covers it |
+| `CodeView::language()` getter, `clear_selection()` | `code_view.rs:150,227` | zero callers |
+| `markdown plain_code()` / `highlight_code()` | `markdown.rs:199-203` | cascade: delete the `highlight_code` flag + `parse` param; construction-time double parse goes with it |
+| `MarkdownTableCell.column` | `markdown.rs:104` | derivable, unused by the renderer (only a test asserts it) |
+| `Input::submit_on()` getter | `input.rs:698` | the setter `set_submit_on` is used; getter is not |
+| `Input::accessibility_label` | `input.rs:1879` | placeholder/id fallback covers it |
+| `ScrollArea::accessibility_label` | `scroll_area.rs:190` | |
+| `scrollbar_state_key` → `pub(crate)` | `scroll_area.rs:200` | only consumer is `scrollbar.rs` |
+| `ResizeHandle::accessibility_label`, `.disabled()` | `resize_handle.rs:70,64` | `disabled` used only by its own test |
+| `Resizable::sizes()` / `set_sizes()` | `resizable.rs:38-49` | built for a persistence feature that doesn't exist (docs/CHECKLIST lists persistence as a follow-up) |
+| `Side::Right` arms in `toggle_icon`/`toggle_label` | `shell.rs:112-124` | the right strip renders `WindowControls`; arms unreachable |
+| `WindowControls::maximize_on_double_click` | `window_controls.rs:153` | single internal caller → private |
+| `input into_bindings` | `input.rs:273` | only called by `bind_input_keys` → private |
+| `render_tab(selected, selected_index)` | `tabs.rs:265-277` | same fact passed twice → keep `selected_index`, derive `selected` |
+| `register_panel(cx)` notify | `dock.rs:195-199` | notifies on first insert only, and nothing renders from `panels` → drop notify + the `cx` parameter (45 call sites, mechanical) |
+| `data` item closures taking `&mut Window` | `list.rs:102`, `sidebar.rs:148`, `tabs.rs:164`, `tree.rs:129` | every in-repo caller passes `|_, _|`; `&mut App` is real capability, `&mut Window` has no demonstrated use — judgment call, 9 call sites |
+| `input copy_selection(_window)`, `caret_visible(&mut self)` | `input.rs:919,820` | unused param / needless mut |
+| `accessibility_label` batch | `checkbox.rs:68`, `toggle.rs:65`, `switch.rs:68`, `link.rs:68`, `radio.rs:57,112`, `segmented.rs:114`, `popover.rs:83`, `dialog.rs:102`, `dropdown_menu.rs:112` | zero external callers on all of them. A uniform library convention — decide once: keep the convention or delete the dead ones. `Segmented`'s id-fallback announcing internal ids ("filter", "theme-demo-mode") should go regardless. |
+
+---
+
+## Over-engineering
+
+- `dialog.rs:186,236-252` — the panel's `stop_propagation` fires before the scrim handler can, making the scrim's `panel_bounds.contains` check unreachable → the whole `BoundsCell` + capture canvas is dead machinery. One dismissal mechanism suffices.
+- `scroll_area.rs:279-426` — a drifted ~150-line copy of `scrollbar.rs`'s painter (no horizontal axis, no track-click jump, different autohide policy); `ScrollAxes::Horizontal` is promised but unimplemented, with zero call sites. Delete the copy; compose the real `Scrollbar` element. This is deletion of a copy, not a new abstraction.
+- `input.rs:525,563,1088` — `Rc<RefCell<Option<InputLayout>>>` with a single owner → plain `Option<InputLayout>` field.
+- `input.rs:494-496,2180,2191` — two always-`Some` `Option<Rc<handler>>` fields papered over with `expect()`s → non-optional `Rc` fields, delete the expects.
+- `input.rs:522,772,809` — `caret_epoch_started: Option<Instant>` is never `None` → plain `Instant`.
+- `markdown.rs:302-311` — `CodeBuilder` is a pass-through wrapper around `String` → use `String::push_str` directly.
+- `markdown.rs:296-299` — `TableBuilder::None / Body { rows }` is exactly `Option<Vec<MarkdownTableRow>>` → replace with the Option.
+- `markdown.rs:662` — `Event::FootnoteReference` arm is unreachable (`ENABLE_FOOTNOTES` not enabled) and contradicts the module doc → delete.
+- `avatar.rs:110-189` — user style refined twice (base + `PixelAvatar`), children re-clipped/re-rounded by the base anyway → delete `PixelAvatar.style` + its `Styled` impl + duplicate clipping.
+- `combobox.rs:329-332` — `refilter` returns a `Vec<SharedString>` that all five call sites ignore (per-keystroke collect for nothing) → return `()`.
+- `combobox.rs:320-333,430-433` — `refilter` doesn't clamp `highlight` (the command palette does); the identity round-trip `position(visible[row]) == row` is a bounds check in disguise → clamp in `refilter`, then `highlight.filter(|&row| row < visible.len())`.
+- `code_view.rs:251-253` — `.max(0)` after `.clamp(0, len - 1)` is a no-op.
+- `code_view.rs:282` / `diff_view.rs:390` — `char_width = text_size * 0.62` guessed monospace advance, duplicated in two files, drives hit-testing and row width → hoist to a theme metric.
+- `segmented.rs:167-171` — `selectable: Vec<usize> = (0..len).collect()` is always `0..len` → index math.
+- `Cargo.toml` (layout) — the `dock` feature enables `resizable` for nothing.
+
+---
+
+## Performance (beyond the majors)
+
+### Eager `format!`/`to_string` for `debug_selector`
+
+gpui's cfg-gate only disables the method *body*; arguments allocate every frame **including release builds**. The correct pattern (already used in `clipboard.rs:126`, `status_dot.rs:97`) captures cheap clones and formats inside the closure: `.debug_selector(move || format!("{id}-row-{n}"))`.
+
+Sites: `switch.rs:136`, `field.rs:97-117` (×3), `combobox.rs:601,679`, `command_palette.rs:337,413`, `tabs.rs:305,409`, `sidebar.rs:198`, `code_view.rs:559`, `diff_view.rs:404,542`, `input.rs:2044`, `menu.rs:286` (+ the popover/dialog a11y-label fallbacks).
+
+### Per-frame clones/collects in render
+
+- `segmented.rs:167-171` — deep clone of every `SegmentedOption<T>` + the `0..len` Vec, per frame → store `Rc<[SegmentedOption<T>]>` at construction.
+- `select.rs:209-225` — whole options `Vec` cloned per frame and the trigger re-derives the label inside a closure → compute `text` once in `render`, move only the `SharedString` in.
+- `dock.rs:999-1111` — per pane per frame for the context menu: `pane.clone()`, `tabs.clone()`, three separate `panels` collects → capture one slim tuple Vec, collect once.
+- `alert.rs:307` — `entries().to_vec()` clones the entry Vec every render of `AlertsView` (borrow-scoping workaround; low priority).
+- `code_view.rs:478` — the whole `Line` cloned per visible row per frame; `render_row` only reads → borrow.
+- `menu.rs:286` + `popover.rs:115`, `dropdown_menu.rs:205`, `dialog.rs:158` — per-frame `SharedString::from(self.id.to_string())` fallback labels.
+
+### No-op notifies
+
+- `input.rs:897-916,1253-1261` — `move_cursor`/`select_range`/`drag_select_to`/`scroll_by` always `cx.notify()` + `window.notify_text_input_changed(...)`: pressing Left at offset 0 or wheeling a non-scrollable field triggers a full re-render + IME re-query → early-return when cursor/anchor/scroll are unchanged.
+
+### Theme-token violations
+
+Rule: `crates/theme` owns every color and metric — no raw `rgb`/`rgba`/`hsla` or layout `px(...)` elsewhere.
+
+- `button.rs:202-205` — `hsla(0.0, 0.0, 0.0, 0.0)` ×4 as a "no border" color → apply the border conditionally instead.
+- `dropdown_menu.rs:196` — `hsla(0.0, 0.0, 0.0, 0.0)` → `gpui::transparent_black()`.
+- `avatar.rs:187` — `Hsla { a: 0.16, ..main }` + luminance multipliers `1.6/0.95/0.45/0.18` → theme token.
+- `separator.rs:72` — `px(1.)` → `theme::BORDER_WIDTH` (already exists, used by button).
+- `menu.rs:472`, `combobox.rs:666`, `command_palette.rs:404` — `px(320.)` popup max-height, three copies → one theme metric.
+- `tooltip.rs:79,92` — `px(360.)` max width, `px(1.)` nudge.
+- `input.rs:1195,1278` + `:32` — `px(1.5)` caret width, `px(1.)` underline, `SCROLL_LINES_HEIGHT = 20.0` → theme metrics (next to `theme.scrollbar.*`).
+- `resize_handle.rs:17` — `KEYBOARD_STEP: f32 = 32.0`.
+- `window_controls.rs:26,215-220,237` — `TRAFFIC_LIGHT_PAD`, `rgba(...)` Windows close-hover, caption width.
+- `window_frame.rs:13-15` — `RESIZE_INSET`, `BORDER_WIDTH`.
+
+### Input misc
+
+- `input.rs:1726` — every IME composition step pushes undo history; steps >300 ms apart fragment undo → merge unconditionally while `marked_range` is active.
+- `input.rs:1168` — `entry.line.paint(...).ok()` swallows a `Result` (failure = invisible text) → log like the shape path at `input.rs:984` does.
+- `input.rs:820` — `caret_visible(&mut self)` doesn't mutate → `&self`.
+
+### Semantic inconsistencies
+
+- `alert.rs:264-269` — `Alerts::clear()` drops entries without running `on_dismiss` handlers (`Toasts::clear` runs them via `start_exit` → `finish_exit`) → run the handlers or document the difference.
+- `scroll_area.rs:403-407` vs `scrollbar.rs:348-353` — autohide touch-on-drag vs touch-on-hover: two painters, two behaviors → pick one policy (subsumed by the scroll_area consolidation).
+- `dock.rs:316-362` — `close_panel` reconciles the displayed right view when its tab closes, but `close_others`/`close_pane` don't → the sidebar can keep showing a closed panel's inspector.
+- `combobox.rs:651-658` — "No matches" row is near-dead (empty results close the popup); the palette stays open showing "No matches" → pick one semantic.
+- `command_palette.rs:336` — row ids are list indexes `("command", row)` for content that reorders every keystroke; `Command.id` is a stable `SharedString` → `("command", command.id.clone())`.
+- `shell.rs:283-324` — hardcoded element ids not namespaced by the shell id (`"sidebar-strip-left"` etc.), unlike `Dock` which composes every id from `dock_id` → compose `(id, "…")`.
+- `data` a11y sets — `sidebar.rs:270-298` counts headers in row sets; `tree.rs:250-261` uses flat-list position/size instead of per-sibling → compute among real siblings.
+
+---
+
+## Clean bills
+
+`badge`, `progress` (fps-capped), `spinner`, `status_dot`, `clipboard`, `empty_state`, `context_menu` (the namespacing exemplar), `list`, `editor/lib` + `editor/utils` + `highlight`, `layout/lib`, `theme` (engine sound; watchers are app-lifetime; `set_preset` has one small redundant name lookup), `icons` (cache bounded, negatives cached), all `Cargo.toml` feature graphs, and all lifecycle plumbing: no detached tasks/subscriptions anywhere in `crates/`, `WeakEntity` used correctly, `cx.notify()` never from `render`, overlay state reclaimed by gpui's element-state eviction on dismiss.
+
+---
+
+## Recommended fix order (before migrating `signed`)
+
+1. **P0 — shared keyed state sweep** (findings 1-4): ~15 lines total; add a two-sibling regression test per component, since single-instance tests provably mask this class.
+2. **P0 — user-facing bugs**: IME UTF-16 conversion (5), combobox chevron (6), palette z-order (7), code_view click (8) + incremental misuse (9), resize capture loss (10), tab element ids (14).
+3. **P1 — performance**: registry (11), caret blink (12), widest_row (13), the `debug_selector` batch, the per-frame clone batch.
+4. **P2 — deletions**: the param/dead-API table and the over-engineering list — this is where "no over-engineering" actually gets enforced, and it shrinks the API surface `signed` will migrate onto.