Skip to content

fix(webui): split Models screen, harden overflow hook and format cache #1976

Closed

Description

@inureyes

Problem / Background

Residual LOW items recorded in TECHNICAL_REPORTS/1975-webui-residual-low-items-20260925.en.md ("Remaining items") after PR #1975 (closed #1974), a follow-up of #1929 and #1930 under epic #1910. They share the WebUI subsystem and one verification pass, so they are grouped. Two of the report's premises did not hold on origin/main at 1f5f7470 and are restated below: there is no 500-line module rule (item 1), and no current caller mounts its table after the ref attaches (item 2).

Implement all four items in ONE PR, run the verification block once at the end, and keep pushes to a minimum. A WebUI push costs a full CI cycle including the GB10 installed-artifact job. Land item 1 as its own commit inside the PR so the mechanical move is reviewable apart from the fixes.

1. Split ModelsLibrary along its existing seams (behavior-preserving)

webui/src/features/models/screen.tsx is 731 lines, and ModelsLibrary (screen.tsx:82-731) is one function holding page state, the row-focus effect, action handlers, column renderers, the filter toolbar, and the page JSX. This is not a guideline violation: docs/code-guidelines.md ("File Size and Module Structure", the authoritative source) calls anything under 800 lines fine as one file. The split is for cohesion and reviewability.

Extract, in the same directory:

  • row-focus.ts: a useRowFocus() hook owning RowTarget, RowFocus, AFTER_LOAD, AFTER_UNLOAD (screen.tsx:63-76), the rowControls/rowFocus/rowRefs refs (:100-103), the focus-follow effect (:128-172) and register (:330-345). It returns what the screen still reads (rowFocus, rowControls, register, the AFTER_* constants).
  • columns.tsx: rowComparator, sortable, modelActions, downloadBadge, downloadProgress, downloadActions, and the columns array (:352-525), as libraryColumns(context) taking one typed object of the values and callbacks they close over. downloadRepo (:78-80) and LibraryRow (:59) move with it and are imported back by screen.tsx.
  • toolbar.tsx: a LibraryToolbar component for the search Field and the three Selects (the .models-toolbar block), taking locale, catalog, filter, and onChange: (patch: Partial<InventoryFilter>) => void. LIFECYCLE_FILTERS moves with it.

Target: screen.tsx at or under about 450 lines and every new module under 500. No change to exports from screen.tsx, rendered DOM, class names, test ids, or handler order.

  • The three modules exist and screen.tsx imports them; ModelsLibrary remains the only export consumers use.
  • screen.test.tsx, load-profile-integration.test.tsx, and webui/tests/models.spec.ts pass with no edits to those files.

2. useOverflowRegion observes only the table present at attach

useOverflowRegion (webui/src/design-system/common-adapters.tsx:119-148) resolves region.querySelector('table') once when the ref attaches (:145-146). The hook is exported through primitives.tsx:7, and its doc says it "follows the box's size and its table's". Current callers are unaffected: ui-common alpha.19's DataTable always renders its <table> (loading and empty states are rows inside it), and the Activity box at features/activity/slots.tsx:69-71 mounts together with its table. A consumer whose region has no table at attach, or swaps it, is never observed for content growth.

Fix: add a MutationObserver on the region ({ childList: true, subtree: true }) that re-resolves the table, unobserves the previous one from the ResizeObserver, observes the new one, and calls update. disconnect tears down both observers. Re-resolving only on ResizeObserver ticks is rejected because a table can mount without the box resizing.

  • New test in common-adapters.test.tsx on the raw hook (a harness component rendering <div ref={useOverflowRegion({ label: 'X' })}> with a <p> child): with overflow mocked as the existing tests do (:309-312) and ResizeObserver stubbed, swap the child for a <table> and, after await act(async () => ...) flushes the mutation, expect the new table among the stub's observed elements and role="region" on the box. It fails on current main.

3. Shrinking a focused scroll box drops focus to <body>

When update (common-adapters.tsx:127-140) finds no overflow it removes tabindex; if the box has focus, the browser's focus fixup moves focus to <body>. Models names its box with label (no heading) and Activity's <h3> is not focusable, so moving focus to a heading does not work for both.

Fix: when the box would lose its tab stop and document.activeElement === region, keep tabindex, role, and the name, and add a one-shot blur listener that re-runs update. disconnect removes that listener.

  • New test: stub ResizeObserver capturing its callback, render the harness overflowing, focus the box, switch the overflow mock to fitting, invoke the callback, expect tabIndex === 0 and role="region"; dispatch blur, expect both removed. Assert on attributes, not activeElement (jsdom has no focus fixup). It fails on current main.

4. cached() has no size bound

cached() (webui/src/design-system/format.ts:8-15) is exported and unbounded. Callers today use small fixed key sets (format.ts:20, features/activity/format.ts:32,40,64,69), but a caller keyed by server data would grow without bound. features/chat/time.ts:26-32 keeps a private unbounded copy with a (tag) => T factory.

Fix: add const CACHE_LIMIT = 32; on inserting a new key into a map already at the limit, delete the oldest key (cache.keys().next().value) first. Restricting the key type is rejected because format.ts:20 builds ${locale}:${digits} from a number. Replace the private copy in chat/time.ts with the shared helper (() => new Intl.RelativeTimeFormat(localeTag(locale), ...)).

  • New test in format.test.ts: insert CACHE_LIMIT + 1 distinct keys into one map, expect size === CACHE_LIMIT, the first key evicted, and a repeat lookup of the newest key not calling its factory. It fails on current main.
  • chat/time.ts defines no cached; its existing tests pass unchanged.

Verification

cd webui
npm run typecheck && npm run lint && npm run unit
npm run browser -- tests/models.spec.ts tests/activity.spec.ts
cd .. && python3 scripts/webui/build_bundle.py && (cd webui && npm run verify-generated)

The checked-in bundle under src/webui/assets must be regenerated in the same PR (Node v26.5.1, per REQUIRED_NODE_VERSION in scripts/webui/build_bundle.py).

Activity

  1. added
    type:bugBug fixes, error corrections, or issue resolutions
    area:architectureArchitecture and code structure changes
    and removed on Sep 26, 2026
  2. added a commit that references this issue on Sep 29, 2026
    44f17d8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:architectureArchitecture and code structure changespriority:lowLow prioritystatus:doneCompletedtype:bugBug fixes, error corrections, or issue resolutions

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions