Add shared activist tag-input component - #468
Conversation
Working Groups (#464) and Circles (#466) independently built near-duplicate chip/autocomplete multi-select components for picking activist names (person-multi-select.tsx and activist-tag-input.tsx). Extract a single shared, presentational TagInput covering the union of both: single- and multi-select via `max`, disabled state, optional built-in label, and type-ahead filtering restricted to the given `options`. Filtering uses case-insensitive "starts with", matching the legacy Vue b-taginput behavior in both WorkingGroupList.vue and CirclesList.vue (the Working Groups PR's port had drifted to substring matching). This is a base PR — the component is unused here; #464 and #466 will adopt it and delete their local copies in follow-up commits. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Replaces the page-local PersonMultiSelect with the shared TagInput component from frontend-v2/src/components/tag-input.tsx (introduced in #468), which the circles page also uses. Prop mapping is 1:1. Suggestion filtering changes from substring match to case-insensitive prefix match, which is a parity correction matching the legacy Vue getFilteredOrganizers behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Extracts a shared TagInput chip/autocomplete component into frontend-v2 to unify the activist-name multi-select UI used by the parallel Working Groups and Circles React ports.
Changes:
- Adds a new
TagInputcomponent that supports single-select (max=1), multi-select, disabled state, and optional internal labeling. - Implements case-insensitive “starts with” filtering for suggestions to match legacy Vue behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <input | ||
| id={inputId} | ||
| className="min-w-[8rem] flex-1 border-0 bg-transparent p-1 text-sm outline-none placeholder:text-muted-foreground" | ||
| value={text} | ||
| placeholder={value.length === 0 ? placeholder : undefined} | ||
| onChange={(e) => { | ||
| setText(e.target.value) | ||
| setOpen(true) | ||
| }} | ||
| onFocus={() => setOpen(true)} | ||
| onBlur={() => setOpen(false)} |
There was a problem hiding this comment.
[Claude] Fixed in daced44: the input now has role="combobox", aria-autocomplete, aria-expanded, and aria-controls, linking it to the suggestions list. This mirrors the existing combobox pattern in suggestion-input.tsx.
| <ul | ||
| role="listbox" | ||
| className="max-h-[240px] overflow-y-auto rounded-md border border-gray-200 bg-white shadow-lg" | ||
| > | ||
| {suggestions.map((s) => ( | ||
| <li | ||
| key={s} | ||
| role="option" | ||
| aria-selected={false} |
There was a problem hiding this comment.
[Claude] Fixed in daced44 (same fix as the combobox comment above): the listbox now has a stable id and each option has an id, completing the aria-controls/aria-activedescendant linkage.
|
|
||
| return ( | ||
| <div className="space-y-1.5"> | ||
| <Label htmlFor={inputId}>{label}</Label> |
There was a problem hiding this comment.
[Claude] Fixed in daced44: the <Label>'s htmlFor is now only set while the input is actually rendered, so there's no dangling reference when max is reached or the field is disabled.
| const addValue = (name: string) => { | ||
| if (!name.trim() || value.includes(name) || atMax) return | ||
| onChange([...value, name]) | ||
| setText('') | ||
| setOpen(false) |
There was a problem hiding this comment.
[Claude] Acknowledged — keyboard-interaction test coverage is a good candidate to add, especially alongside the arrow-key navigation work tracked in issue #474. Not added in this PR to keep it reviewable.
Address two bot-review findings on the shared TagInput:
- Add the combobox ARIA pattern already used by
events/suggestion-input.tsx: role="combobox", aria-autocomplete="list",
aria-expanded, and aria-controls on the input, plus a stable id on the
role="listbox" list and per-option ids.
- Only set the built-in Label's htmlFor while the input is actually
rendered — the input unmounts when `disabled` or once `max` selections
are reached (e.g. max={1} fields after picking a value), which left the
label pointing at a non-existent element. Also document this pitfall on
the `id` prop for callers rendering external labels.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Final-review follow-ups for the shared TagInput base PR: - Add lib/members.ts with findPointPerson and countMailingListMembers, which the Working Groups (#464) and Circles (#466) tables currently duplicate (getPointPersonName/countMailingListMembers and hostName/memberCount respectively). Typed against a minimal structural member shape so both pages' member types satisfy them; the adopting branches will switch to these themselves. - Replace the multi-line JSX comment around the conditional Label htmlFor in tag-input.tsx with a named labelFor variable and a one-line comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Apply the repo-wide comment-style directive: every TagInputProps property JSDoc is now a single line, the component-level JSDoc is two lines, and the members.ts file header (which restated the function docs) is removed. Single-line comments carrying non-obvious context are kept as-is. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| /** Placeholder shown in the text input while no chips are selected. */ | ||
| placeholder?: string | ||
| /** Max selections — the input hides once reached; pass 1 for single-select, omit for unbounded. */ | ||
| max?: number |
There was a problem hiding this comment.
should we just have an option for single-select instead of max options if the only used values are binary (1 or no limit)?
There was a problem hiding this comment.
[Claude] Good call — checked every call site on both adopting branches (jake/react-working-groups, jake/react-circles): all 5 usages are either max={1} (point person, host) or unlimited (members, non-members). Replaced max?: number with single?: boolean in 546908a. Adopters change max={1} to single.
| /** Called with the full next array whenever a chip is added or removed. */ | ||
| onChange: (next: string[]) => void | ||
| /** All selectable values — callers own data fetching; already-selected values are excluded from suggestions. */ | ||
| options: string[] |
There was a problem hiding this comment.
hmmm is this really just an array? is it at least pre-sorted? wonder if it worth be a worthwhile perf win to add context for these options that all tag inputs could share, so if that is an overoptimization. my frontend skills are rusty.
There was a problem hiding this comment.
[Claude] Assessed and left as-is — I think context would be overoptimization here. The option lists are name-only arrays (a few thousand entries at most — the full activist dataset with metadata is ~3MB per activist-storage.ts, and these name lists are a small fraction of that). Both pages already share the fetch across components via the react-query cache key, so context would only save prop-drilling one array one level deep. Filtering is a single O(n) prefix scan per keystroke, capped at 20 results — well under a millisecond at this n. Pre-sorting would actually be wrong: GetAutocompleteNames deliberately orders by most recent event attendance (ORDER BY MAX(e.date) DESC) so recently active people rank first in suggestions, same as the legacy Vue pages that use these endpoints.
| if (!query) return [] | ||
| return options | ||
| .filter( | ||
| (name) => !value.includes(name) && name.toLowerCase().startsWith(query), |
There was a problem hiding this comment.
this looks like terrible perf
There was a problem hiding this comment.
[Claude] Fixed in 546908a. Before: the filter ran value.includes(name) per option — O(options x selected) per keystroke, plus it re-ran on every re-render. After: suggestions are computed in a useMemo keyed on (options, text, selected set, cap), with a memoized Set for the selected-value check — O(options) per keystroke and nothing on unrelated re-renders. At the actual data size (a few thousand short strings) this is comfortably sub-millisecond.
| disabled = false, | ||
| }: TagInputProps) { | ||
| const [text, setText] = useState('') | ||
| const [open, setOpen] = useState(false) |
There was a problem hiding this comment.
would prefer isOpen over open, unless this pattern is already established repo-wide
There was a problem hiding this comment.
[Claude] Renamed to isOpen in 546908a. Grep confirms isOpen is the established convention for internal state: 3 existing uses (suggestion-input.tsx, column-selector.tsx, debug-menu.tsx) plus pervasive is* boolean state names, vs a single const [open, (filters/filter-chip.tsx). Radix component props (<Popover open=...>) are untouched — that name is the library API.
| /** Optional label rendered above the control, wired to the input via `htmlFor`. */ | ||
| label?: string | ||
| /** Input id for an external label (auto-generated if omitted); prefer `label`, since the input unmounts while hidden. */ | ||
| id?: string |
There was a problem hiding this comment.
pretty confused what these ids are actually used for. if safe to remove, remove.
There was a problem hiding this comment.
[Claude] Cleaned up in 546908a. Removed: the id prop (neither adopting branch passes it now that the built-in label prop handles wiring) and the per-option ids, which nothing referenced — they only matter for aria-activedescendant keyboard highlighting, which this component does not implement. What remains: one useId()-generated input id that the label points at via htmlFor, and a derived listbox id linking the input to the dropdown via aria-controls so screen readers associate the combobox with its popup.
- Replace `max?: number` with `single?: boolean` — every call site on the
adopting branches uses either max={1} or no limit, so a boolean states
the intent directly.
- Use a memoized Set for selected-value lookup in the suggestions filter,
dropping the per-keystroke cost from O(options x selected) to O(options).
- Rename internal popover state open -> isOpen to match the prevailing
repo convention (suggestion-input, column-selector, debug-menu).
- Remove the `id` prop (no caller passes it now that the built-in label
handles htmlFor) and the per-option list ids, which nothing referenced
since we don't use aria-activedescendant. The remaining internal ids
wire the label to the input and aria-controls to the listbox.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary
jake/react-working-groups(Port working groups page to React #464):person-multi-select.tsx(point person, members, non-members)jake/react-circles(Port circles page to React #466):activist-tag-input.tsx(host, members)TagInputcomponent atfrontend-v2/src/components/tag-input.tsxcovering the union of both usages (single-select viamax=1, unbounded multi-select, disabled state, optional built-in label vs. externally-supplied label).Notes for adopters
getFilteredActivists/getFilteredOrganizersinWorkingGroupList.vueandCirclesList.vue. The Working Groups PR'sperson-multi-select.tsxhad drifted to substring (includes) matching — adoptingTagInputrestores parity with the legacy app and circles' existing behavior. This is an intentional, documented behavior change for that one call site.maxinstead ofmax/maxItems, addedmaxSuggestions,labelis optional and only renders internally when passed).Test plan
pnpm installpnpm exec tsc --noEmit(only pre-existingbaseUrldeprecation warning)pnpm lint(clean)pnpm build(clean)TagInputand confirm no visual/behavioral regressions on Working Groups and Circles forms🤖 Generated with Claude Code