Skip to content

Typeahead extensions, grouped options, and MentionTextarea (kata ry18, t662, zdn0)#22

Merged
mariusvniekerk merged 11 commits into
mainfrom
t3code/outstanding-kata-tasks
Jul 7, 2026
Merged

Typeahead extensions, grouped options, and MentionTextarea (kata ry18, t662, zdn0)#22
mariusvniekerk merged 11 commits into
mainfrom
t3code/outstanding-kata-tasks

Conversation

@mariusvniekerk

Copy link
Copy Markdown
Contributor

Closes out the three open kata issues, all aimed at the same maintenance burden: middleman keeps three hand-rolled components (TypeaheadTrigger, RepoTypeahead, TaskReferenceTextarea) — plus checker suppressions covering them — because kit's primitives couldn't express what those consumers rely on. Every copy is drift the checker exists to prevent but currently has to look away from.

Typeahead extensibility (ry18). TypeaheadTrigger's features land as opt-in props on the one kit component rather than a second variant, so deleting the middleman copy doesn't just move the fork: allowClear/clearLabel, allowCustom, placement, searched meta text, triggerPrefix, a header snippet (the repo-browser ref picker's Branches/Tags switcher), and loading/error rows. Veto is expressed as a false/throw return from the existing onselect instead of a new callback. One deliberate divergence from middleman: Enter while the clear row is highlighted with a live query prefers the first match or custom value over silently clearing — with allowCustom, the highlight parks on the clear row precisely when the user typed something unmatched.

Grouped options (t662). TypeaheadOption.children turns an option into an expand/collapse group (RepoTypeahead's provider/host tree): ArrowRight/ArrowLeft expand-collapse with leaf→parent jump, Enter toggling groups, filter-forced expansion of matching subtrees, and role=tree/treeitem/aria-level wiring replacing the flat listbox when groups are present. placement support extends the shared floatingPopoverStyle contract (above/below forcing) rather than forking positioning.

MentionTextarea (zdn0). New primitive with the same app/kit split as TaskReferenceTextarea: kit owns word-boundary trigger detection, caret query extraction, version-guarded async search plumbing, the keyboard protocol (Cmd/Ctrl+Enter passes through so submit shortcuts survive), and insert-on-select with caret placement; the app supplies search and optional row rendering, with the trigger char a prop so @-mentions work as well as #-refs. The menu uses the shared fixed-position popover contract, so unlike the middleman original it survives overflow-hidden ancestors. The hand-rolled-dropdown rule now steers listbox-next-to-textarea markup to it, removing the reason for the standing suppression.

Reviewer notes. Typeahead's popover DOM changed from a bare ul to a panel wrapper (needed for the header snippet) — internal classes only, but downstream style overrides targeting .kit-typeahead__list as the card would need updating. onselect is now allowed to return a value; existing (v) => (x = v) arrows that implicitly return a string need a block body (the demo was the only in-repo case).

Validation. svelte-check 0/0, fmt + lint, bun test (146), check:usage 0 findings, Playwright suite 48/48 including new typeahead.spec.ts and mention-textarea.spec.ts driving all the above end-to-end in Chromium.

🤖 Generated with Claude Code

mariusvniekerk and others added 2 commits July 2, 2026 22:26
…tree (kata ry18, t662)

middleman keeps two hand-rolled typeaheads because kit's lacked their
features. TypeaheadTrigger needs an allowClear 'None' row
(KataSearchPanel, KataIssueProperties), allowCustom free-text values
(FleetSettings, KataIssueProperties), placement=top for triggers near
the viewport bottom, an onChange that can veto and keep the list open,
searched per-option meta text, and a triggerPrefix; the repo-browser
ref picker additionally needs a menu-header snippet (Branches/Tags tab
switcher) with async loading rows, and the docs folder menu needs
error/empty rows. RepoTypeahead needs provider/host group nodes with
ArrowRight/ArrowLeft expand-collapse, which kit's flat {name,label}
list couldn't express.

All land as opt-in props on the one component rather than a
TypeaheadTrigger variant, so the middleman copies can be deleted
without a second component drifting: veto is a false/throw return from
onselect (error surfaces through the new error row without losing the
query), groups are TypeaheadOption.children (unique names across the
tree; role=tree/treeitem/aria-level wiring replaces the flat listbox
when present), and filtering forces matching subtrees open. placement
extends the shared floatingPopoverStyle contract ('above'/'below'
forcing) instead of forking the positioning. One deliberate divergence
from TypeaheadTrigger: Enter on a highlighted clear row with a live
query prefers the first match or the custom value over silently
clearing, since with allowCustom the highlight parks on the clear row
whenever nothing matches.

Validation: svelte-check 0/0, fmt + lint, bun test (144), check:usage
0 findings, Playwright suite (48) including a new typeahead.spec.ts
driving clear/meta/custom/veto/grouped/header/placement end-to-end.

Generated with Claude Code (claude-fable-5)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
middleman's TaskReferenceTextarea overlays a listbox on a textarea for
#ref completion, and kit had no editor-autocomplete primitive — so the
checker's hand-rolled-dropdown finding stayed suppressed there with a
reason, exactly the drift the checker exists to prevent.

The primitive keeps the app/kit split of the original: kit owns trigger
detection at word boundaries, query extraction at the caret, the
version-guarded async search plumbing, the keyboard protocol
(ArrowUp/Down cycle, Tab/Enter insert, Escape dismiss, Cmd/Ctrl+Enter
passes through so submit shortcuts keep working), and insert-on-select
with caret placement; the app supplies search and (optionally) row
rendering via a snippet, with the trigger char a prop so @-mentions
work as well as #refs. The menu uses the shared fixed-position popover
contract rather than middleman's absolute positioning, so it survives
overflow-hidden ancestors like every other kit popover.

The hand-rolled-dropdown rule now steers listbox markup in files with a
textarea toward MentionTextarea, so consumers lose the reason to keep
the suppression.

Validation: svelte-check 0/0, fmt + lint, bun test (146 incl. new rule
cases), check:usage 0 findings, Playwright suite (48) including a new
mention-textarea.spec.ts driving trigger/keyboard/insert end-to-end.

Generated with Claude Code (claude-fable-5)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@roborev-ci

roborev-ci Bot commented Jul 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (90508c5)

Medium issues remain in Typeahead and MentionTextarea; no Critical or High findings were reported.

Medium

  • src/lib/components/Typeahead.svelte:213
    When loading or error is true, the UI renders only a status row, but keyboard handling can still navigate/select from hidden rows. Pressing Enter during loading or after an error can select a stale, non-visible option.
    Fix: Short-circuit Arrow/Enter selection while loading || error, or derive an empty interactive row set for those states and only allow Escape/typing.

  • src/lib/components/MentionTextarea.svelte:59
    Closing the mention menu does not invalidate an in-flight search. If that request resolves while closed, it can repopulate results, so the next open can briefly show and allow selecting stale options before the new search resolves.
    Fix: Increment searchVersion and clear searching when closing, or guard async result assignment on both version and open.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 4m32s

…tatus-row keys, header focus-out)

roborev flagged four issues across the two new components; the concrete
correctness ones are fixed here.

MentionTextarea dropped the previous query's results only lazily, so a
slow search left the prior query's options selectable — Enter/Tab during
the pending window could insert a stale reference — and an in-flight
request that resolved after the menu closed could repopulate results and
flash on the next open. The search effect now clears results at the
start of every lookup and bumps the version guard on close, so nothing
stale is ever navigable, insertable, or shown.

Typeahead let Enter and the arrow keys act on the hidden option rows
while a loading/error status row was the only thing rendered, so keyboard
users could select an option the UI said was unavailable; navigation and
selection are now inert whenever a status row shows (Escape still
closes). Its header snippet can hold focusable controls, but dismissal
hung off the input's blur alone, so tabbing input→header→out left the
panel stuck open. Dismissal now listens for focusout on the container
(which bubbles, unlike blur) with an opening guard so the trigger→input
focus handoff doesn't self-close.

The remaining findings were design-completeness notes rather than bugs;
resolved by documenting the contracts they asked about (error is
caller-cleared, single-character trigger + boundary predicate, native
form attributes are a deliberate non-goal).

Validation: svelte-check 0/0, fmt + lint, bun test (144), check:usage 0
findings, Playwright suite (50) with new cases for the stale-results
guard, status-row key inertness, and header focus-out dismissal.

Generated with Claude Code (claude-opus-4-8)

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
@roborev-ci

roborev-ci Bot commented Jul 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (a99a50f)

Medium-risk issues remain in Typeahead; no Critical or High findings reported.

Medium

  • src/lib/components/Typeahead.svelte:151
    allowCustom can select a free-text value, but the closed trigger still falls back to fallbackLabel because displayValue only renders matched options. After selecting a custom value, the control visually looks unselected even though value changed.
    Fix: When allowCustom is true and value is non-empty but unmatched, use value as the display label.

  • src/lib/components/Typeahead.svelte:225
    While loading or error is shown, handleKeydown returns before handling Enter, so Enter is not inert; in a form, it can submit the form even though the status row is supposed to stand in for options.
    Fix: In the loading || error branch, prevent default for Enter and navigation keys that should be inert, while still allowing Escape to close.


Reviewers: 2 done | Synthesis: codex, 8s | Total: 4m44s

… during loading/error

Two roborev findings on a99a50f: an allowCustom selection has no matching
option, so the closed trigger fell back to fallbackLabel and looked
unselected even though value had changed; and while the loading/error
status row replaces the options, handleKeydown returned before Enter was
handled, so Enter could submit an enclosing form despite there being
nothing to select.

Validation: svelte-autofixer clean, bun run check at 0 errors/0 warnings.

Generated with Claude Code (claude-fable-5)
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@roborev-ci

roborev-ci Bot commented Jul 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (08ea4e9)

Summary verdict: Two medium-severity issues need attention; no high or critical findings were reported.

Medium

  • Location: src/lib/components/Typeahead.svelte:201
    Problem: With allowClear and a non-empty query, the clear row can be highlighted and exposed through aria-activedescendant, but Enter ignores it and commits the first match or custom value instead. Keyboard users can see/hear “None” as active while a different value is selected.
    Fix: Make Enter honor the highlighted row, or render/highlight a real custom row so the active descendant always matches what Enter will commit.

  • Location: src/lib/components/Typeahead.svelte:180
    Problem: Concurrent async onselect calls are not ordered. A slow earlier selection can resolve after a later veto/error and still call closeDropdown(), hiding the caller’s error state.
    Fix: Track a selection attempt token and only close for the latest attempt, or block additional selections while one is pending.


Reviewers: 2 done | Synthesis: codex, 7s | Total: 4m52s

… onselect

Two roborev findings on 08ea4e9, both accessibility/correctness gaps in
Enter handling:

aria-activedescendant could name the clear row while Enter silently
committed the first match or a hidden custom value — a screen-reader user
heard "None" as active while Enter selected something else. Instead of
special-casing Enter, the invisible behavior is removed: the custom value
is now a real "Use \"query\"" row (new customLabel prop for i18n) that
gets highlighted and announced, and Enter commits exactly the row the
active descendant names, including clearing when the clear row is
deliberately highlighted while filtering.

Concurrent async onselect calls are now ordered by an attempt token so a
slow earlier selection resolving after a later veto/error cannot close
the list and hide the caller's error state.

Validation: bun run check / fmt:check / lint clean; all 9 typeahead
Playwright specs pass, including new coverage asserting the active
descendant matches what Enter commits.

Generated with Claude Code (claude-fable-5)
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@roborev-ci

roborev-ci Bot commented Jul 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (5f7b19b)

Medium-risk issues remain in Typeahead.svelte; no Critical or High findings were reported.

Medium

  • Location: src/lib/components/Typeahead.svelte:197
    Problem: A pending async onselect can still close the dropdown after the user has dismissed and reopened it, because selectSeq is only advanced by new selections. A slow successful selection from an earlier open instance can unexpectedly close a later menu.
    Fix: Invalidate pending selections when the dropdown is closed or reopened, for example by incrementing a generation counter in closeDropdown and checking it before calling closeDropdown() from select.

  • Location: src/lib/components/Typeahead.svelte:339
    Problem: highlightIndex is not clamped when rowCount changes due to async option/header updates or other prop changes. If the active index was valid for the old rows but out of range for the new rows, aria-activedescendant points at a missing element and Enter selects nothing.
    Fix: Add a reactive clamp/reset whenever rowCount changes while open, and clear the active descendant when no valid active row exists.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 7m53s

…ve row

Two roborev findings on 5f7b19b, both about state outliving what the
user currently sees:

A pending async onselect could resolve after the user dismissed and
reopened the menu and still close the newer instance, since the attempt
token only advanced on new selections. closeDropdown now bumps the token
so no selection outlives the open instance it started in.

highlightIndex was never re-validated when async option/header swaps
shrank the list, letting aria-activedescendant name a missing row and
Enter go dead. Rendering, aria, and selection now read a clamped
$derived activeIndex (a pure derivation rather than an effect writing
state, per the repo rune conventions), so the announced row is always a
rendered one; with zero rows the descendant is dropped as before.

Validation: svelte-autofixer clean, bun run check / fmt:check / lint
clean, all 9 typeahead Playwright specs pass.

Generated with Claude Code (claude-fable-5)
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@roborev-ci

roborev-ci Bot commented Jul 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (154736d)

Summary verdict: One medium issue remains; no critical or high findings were reported.

Medium

  • Location: src/lib/components/Typeahead.svelte:372
  • Problem: When error is set after a vetoed onselect, the component replaces all options with the error row and blocks Enter/arrow selection while exposing no query-change callback for callers to clear the error. The demo’s locked branch path can leave the Typeahead stuck on the error across typing, close, and reopen.
  • Fix: Clear or dismiss the displayed error on user edits/open/close via an API callback, or render the error alongside selectable options instead of replacing them.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 7m54s

roborev finding on 154736d: the error row replaced the options with
Enter and arrows inert, but error is caller-owned and cleared on the
next onselect — which could never fire again, leaving the menu stuck on
the error across typing, close, and reopen (the demo's locked-branch
path deadlocked this way).

Rather than adding a query-change callback for callers to wire up, the
error now renders as a role="alert" row above the options, which stay
visible and selectable: the retry itself is the dismissal path, and the
veto is still announced to screen readers. Only loading keeps the
stand-in semantics (inert keys, dropped aria-activedescendant).

Validation: svelte-autofixer, bun run check / fmt:check / lint /
check:usage clean; all 9 typeahead Playwright specs pass, with the veto
spec now asserting a rejected selection can be retried and the retry
clears the error row.

Generated with Claude Code (claude-fable-5)
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@roborev-ci

roborev-ci Bot commented Jul 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (4117548)

Medium severity issue found; no critical or high severity findings.

Medium

  • src/lib/components/MentionTextarea.svelte:146: handleKeyup refreshes the mention query for Left/Right/Home/End, but not ArrowUp/ArrowDown. In multiline textareas those keys can move the caret when the menu is closed, empty, or searching, leaving the popup tied to a stale caret position or failing to open when moving into a mention.
    • Fix: Include ArrowUp and ArrowDown in the caret-movement refresh path, or centralize refresh behavior around selection/caret changes.

Reviewers: 2 done | Synthesis: codex, 12s | Total: 6m4s

The keyup refresh only covered ArrowLeft/Right/Home/End, but in a
multiline textarea ArrowUp/ArrowDown move the caret whenever the menu
is not consuming them (closed, searching, or zero results). The popup
could then track a stale caret position or fail to open when the caret
moved into a mention token on another line.

All caret-movement keys (including PageUp/PageDown) now share one
refresh path. No consumed-key bookkeeping is needed: when the menu does
consume ArrowUp/Down for highlight cycling it prevents default, so the
caret has not moved and the refresh is a no-op that leaves the highlight
and pending search untouched.

Validation: bun run check / fmt:check / lint clean; mention-textarea
Playwright specs pass, with new coverage for ArrowUp reopening the menu
on a mention line and closing it after leaving a no-match query.

Generated with Claude Code (claude-fable-5)
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@roborev-ci

roborev-ci Bot commented Jul 7, 2026

Copy link
Copy Markdown

roborev: Combined Review (65f5cd8)

Summary: One medium-severity issue needs attention; no critical or high findings were reported.

Medium

  • src/lib/components/Typeahead.svelte:241: Escape closes the Typeahead but does not mark the event handled, so parent overlays using Escape can close at the same time.
    • Fix: Call e.preventDefault() before closeDropdown(); consider e.stopPropagation() as well for parent keydown handlers.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 7m49s

roborev finding on 65f5cd8: Escape closed the dropdown without marking
the event handled, so a parent overlay (modal, sidebar) listening for
Escape would close in the same keystroke. The handler now calls
preventDefault + stopPropagation before closing, matching how
CommandPalette/FindBar/SearchInput consume Escape. Also drops an
unreachable second Escape branch later in the same keydown chain.

While verifying, the whole typeahead suite failed because Playwright
reused a Vite on 4198 started by a different worktree, silently testing
that checkout's code. The config port is now overridable via
KIT_UI_TEST_PORT so parallel checkouts can test in isolation;
docs/testing.md documents the trap.

Validation: full browser suite (52 specs) passes on an isolated port;
bun run check / fmt:check / lint clean.

Generated with Claude Code (claude-fable-5)
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@roborev-ci

roborev-ci Bot commented Jul 7, 2026

Copy link
Copy Markdown

roborev: Combined Review (c7bee8c)

Verdict: One medium-severity issue should be addressed before merge.

Medium

  • src/lib/components/Typeahead.svelte:346: Escape handling is only wired to the input, so when focus is inside a header snippet control, pressing Escape does not close the typeahead and can bubble to a parent overlay.
    • Fix: Handle Escape at the component/panel level while open, respect defaultPrevented, and close/consume it for any focused descendant.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 6m57s

mariusvniekerk and others added 2 commits July 7, 2026 12:23
Follow-up to c7bee8c: a fixed default port still let a dev server from
another checkout/worktree squat on it (reuseExistingServer would then
silently test that checkout's code), and hand-picking a random number
doesn't check availability. The config now binds to port 0, takes the
OS assignment, and releases it for Vite (--strictPort keeps any race
loud). The port is stashed in env because Playwright re-evaluates the
config in worker processes, which inherit env and must agree on it.
KIT_UI_TEST_PORT still pins the port to keep one server alive across
runs while iterating.

Validation: full browser suite (52 specs) passes with the other
worktree's server still holding 4198.

Generated with Claude Code (claude-fable-5)
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
roborev finding on adbc60d: Escape was only wired to the input, so with
focus inside a focusable header-snippet control it left the typeahead
open and bubbled to parent overlays, closing them instead.

Escape is now handled by a shared handler attached to both the input and
the panel (the header lives in the panel, a DOM sibling of the input).
It respects defaultPrevented so a header control that already consumed
Escape — e.g. a SearchInput clearing its text — wins, and consumes the
event otherwise so parents stay open.

Keyboard-driven closes (Escape, selection) now also return focus to the
re-mounted trigger: unmounting the focused input dropped focus on <body>,
stranding keyboard users. Focusout-driven closes deliberately do not
refocus, since the user is leaving the component.

Validation: svelte-autofixer clean, bun run check / fmt:check / lint
clean, full browser suite (53 specs) passes including new coverage for
Escape from a header control refocusing the trigger.

Generated with Claude Code (claude-fable-5)
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@roborev-ci

roborev-ci Bot commented Jul 7, 2026

Copy link
Copy Markdown

roborev: Combined Review (82efacf)

Verdict: No medium, high, or critical findings to report.

The only reported issue was low severity and is omitted per the requested threshold.


Reviewers: 2 done | Synthesis: codex, 8s | Total: 8m58s

@mariusvniekerk mariusvniekerk merged commit d6f8727 into main Jul 7, 2026
6 checks passed
@mariusvniekerk mariusvniekerk deleted the t3code/outstanding-kata-tasks branch July 7, 2026 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant