227 lines
27 KiB
Markdown
227 lines
27 KiB
Markdown
# 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.
|
||
|
||
> **Re-checked 2026-11-09** (post-P1): several entries below are superseded by the policy "visual/a11y surface stays even when dead" — see the current P2 plan at the bottom of this document for the authoritative delete/keep split. Notably kept: `PanelDescriptor::icon`, `DropdownMenu::icon()`/`.tab_stop()`, `Alert::icon()`/`.no_icon()`, `markdown plain_code()`, the `accessibility_label` convention, `ResizeHandle::disabled()`, `Resizable::sizes()`/`set_sizes()` (test fixture), `Separator::orientation()`. Still deleted: everything non-UI in the list. Already resolved by review passes: `Tab::lead` (deleted), `ComboboxChanged` re-verified still present.
|
||
|
||
| 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.
|
||
|
||
---
|
||
|
||
## Progress
|
||
|
||
### P0 — done (commit `73f7dc3`, reviewed)
|
||
|
||
- Shared keyed-state sweep (findings 1-4): all `use_keyed_state` keys carry the instance id; `Scrollbar::vertical/horizontal(id, handle)` now require an id, `.key()` deleted.
|
||
- Correctness bugs (5, 6, 7, 8, 9, 10, 14): IME UTF-16 conversion fixed, combobox chevron lists all options, palette modal deferred + dead bounds machinery removed, code_view click honors scroll, tree-sitter incremental misuse deleted (`highlight_with_tree` folded into `highlight`), resize drag ends on capture loss, `Tab::new(key, label)` with stable element ids.
|
||
- Regression tests added: two-sibling independence for slider / dropdown / scrollbar, resize capture-loss, multibyte UTF-16 conversions.
|
||
|
||
### P1 — done (in working tree, uncommitted)
|
||
|
||
- Registry (11): keyed by `(WindowId, ElementId)`, mutated in place via `cx.default_global` — no whole-map clone, no cross-window bleed. `anchor_bounds` takes the window id now.
|
||
- Caret blink (12): `with_max_fps(4.0)`; `field_bench` updated to probe the capped contract (executor-timer redraw, no next-frame demand).
|
||
- widest_row (13): cached in `set_rows` (called by `new`/`set_versions`).
|
||
- `debug_selector` batch: all eager formats moved into closures (switch, field ×3, combobox ×2, command_palette ×2, tabs ×2, sidebar, code_view, markdown, dock ×2 reusing the `.id()` format).
|
||
- Per-frame clone batch: segmented options are `Rc<[SegmentedOption<T>]>` (closures capture bumps; `selectable` Vec → index math), select trigger text computed in render, menu/dropdown items passed as `Rc<[MenuItem]>`, alert borrows entries, code_view borrows the row `Line`.
|
||
- Skipped deliberately: the caret animation-id `format!` (removing it needs id plumbing through `InputState` for a ~30-byte alloc — over-engineering).
|
||
- Decision: registry stays a gpui `Global`, not a `OnceLock`. `WindowId`s come from a per-`App` slotmap, so a process-static map would leak state across tests (every `TestAppContext` mints the same early window ids); a Global dies with its `App` and needs no locking.
|
||
|
||
Validation after P1: `cargo check --workspace --all-targets` clean, `cargo test --workspace` 64 binaries 0 failures, `cargo fmt --check` clean.
|
||
|
||
### P2 — done (in working tree, uncommitted)
|
||
|
||
Re-verified the whole P2 list against the tree first. Policy recorded in the plan below: visual/a11y surface stays even when dead; only non-UI dead code was deleted.
|
||
|
||
**1a — deleted (non-UI dead API):** `Slider::value()`, `ComboboxChanged` + its `EventEmitter` impl + emit, `CodeView::language()`, `CodeView::clear_selection()`, `Input::submit_on()` getter, `MarkdownTableCell.column` (+ bookkeeping + test assertion), `Side::toggle_icon`/`toggle_label` (left values inlined at the call site), `render_tab`'s duplicate `selected` param (derived from `selected_index`), `register_panel`'s first-insert-only notify and its `cx` parameter (all call sites updated), `Menu checked` field → plain `bool`, `refilter` → `()`. Visibility narrowed: `scrollbar_state_key` → `pub(crate)`, `maximize_on_double_click` → private, `into_bindings` → `pub(crate)`.
|
||
|
||
**1b — kept (UI/a11y surface, dead is fine):** `PanelDescriptor::icon`, `DropdownMenu::icon()`/`.tab_stop()`, `Alert::icon()`/`.no_icon()`, `markdown plain_code()`, the whole `accessibility_label` convention, `ResizeHandle::disabled()`, `Resizable::sizes()`/`set_sizes()`, `Separator::orientation()`, `data` closure `&mut Window` params. One remaining fix moved to follow-ups: `Segmented`'s unlabeled aria fallback announcing internal ids.
|
||
|
||
**2 — over-engineering deletions:** dialog's `BoundsCell` + capture canvas + bounds check deleted (panel `stop_propagation` is the one mechanism); scroll_area's ~150-line drifted painter deleted — it now composes the real `Scrollbar` (`ScrollbarVisibility`, `Autohide`, and the geometry helpers moved into `scrollbar.rs`), and `ScrollAxes::Both` now renders a horizontal thumb too, resolving the promised-but-unimplemented `Horizontal`; input's `Rc<RefCell<Option<InputLayout>>>` → plain `Option<InputLayout>` field, both always-`Some` handler `Option`s → non-optional (two `expect`s deleted), `caret_epoch_started: Option<Instant>` → `Instant`; markdown's `CodeBuilder` and `TableBuilder` deleted (`String` / `Option<Vec<_>>`), the unreachable footnote arm folded into the render-nothing group; avatar's `PixelAvatar.style` + `Styled` impl + duplicate clipping deleted; combobox `refilter` clamps the highlight and the identity round-trip bounds check became `highlight.filter(...)`; code_view's two no-op `.max(0)`s deleted; layout `Cargo.toml`: `dock` now depends on `resize_handle` directly instead of `resizable`.
|
||
|
||
**3 — theme-token hoists:** new theme consts: `POPUP_MAX_HEIGHT` (the three `px(320.)` popup sites), `TOOLTIP_MAX_WIDTH`/`TOOLTIP_OFFSET`, `CARET_WIDTH`, `UNDERLINE_WIDTH`, `WHEEL_LINE_HEIGHT`, `RESIZE_KEYBOARD_STEP`, `WINDOW_RESIZE_INSET`, `TRAFFIC_LIGHT_PAD` (moved out of window_controls; dock/shell/tests import it from the theme), `WINDOWS_CAPTION_WIDTH`, `WINDOWS_CLOSE_HOVER_HEX`, `MONO_ADVANCE_RATIO` (the duplicated `0.62` estimate), `PIXEL_AVATAR_*` tint/shade factors. Button's Ghost border now paints `transparent_black()` instead of raw `hsla(0,0,0,0)` (kept painted for uniform control metrics), dropdown fallback uses `transparent_black()`, separator hairline uses `BORDER_WIDTH`.
|
||
|
||
**4 — smaller cleanups:** input early-returns when a cursor/selection/wheel change is a no-op (no notify, no IME re-query, no blink-epoch reset); IME pre-edit steps merge into the previous undo entry unconditionally while a composition is active; `line.paint` errors logged instead of `.ok()`; `caret_visible` takes `&self`; `Alerts::clear` runs every entry's `on_dismiss` handler (matches `Toasts::clear`); dock `close_others`/`close_pane` now reconcile the displayed right view (`close_panel` already did); combobox arms row 0 unconditionally on open, so empty results show "No matches" instead of closing (palette parity); palette rows keyed `("command", command.id)` — stable domain ids, not per-keystroke indexes; shell element ids composed from the shell id (`(id, "strip")`, `(id, "strip-drag")`, `(id, "sidebar-toggle")`, `(id, "window-controls")`); sidebar a11y sets count sibling rows only (headers excluded); tree a11y position/size computed among same-depth siblings in one pass.
|
||
|
||
Validation after P2: `cargo fmt --check` clean, `cargo check --workspace --all-targets` 0 errors/0 warnings, `cargo test --workspace` 64 binaries 0 failures.
|
||
|
||
---
|
||
|
||
## Next step — `signed` migration
|
||
|
||
The audit backlog (P0-P2) is complete. Remaining small follow-ups, none blocking: `Segmented`'s unlabeled aria fallback; `markdown plain_code()`'s double parse if a consumer ever uses it; `Separator::orientation()` fold if strictness is ever wanted.
|
||
|
||
Kickoff:
|
||
|
||
1. Clone `signed` (git.reya.info/reya/signed) beside this workspace.
|
||
2. Inventory its views against the component list; mark each usage as: direct fit / needs a small adapter / needs a new component.
|
||
3. Bring missing components in-tree here (per repo rules: components are written here, not ported), then migrate view-by-view.
|