feat(ui-rewrite): add chip-style TagInput with /tags autocomplete

[Altamimi-Dev]

Pull Request

🔗 Related Issue

Closes #5640

📝 Summary

Replaces the comma-separated <Input> used for tag entry in the tool, resource, prompt, and virtual-server forms with a chip-style TagInput, and adds autocomplete against the existing /tags endpoint. Built from repo primitives — no new dependency.

• New client/src/components/ui/tag-input.tsx: chips (via CardTag) + a text field. Commit on Enter / , / Tab / blur; Backspace on empty removes the last tag; each chip has a Remove <tag> button; paste splits on , \n \t; case-insensitive trimmed dedupe; maxTags, disabled, aria-invalid, aria-describedby props. Suggestions render in a role="listbox" with arrow-key navigation and a Create "<value>" entry for unmatched input; the input is a role="combobox" with aria-activedescendant.
• New client/src/hooks/useTagSuggestions.ts: fetches /tags once per session (module-cached) and returns the names for autocomplete. Best-effort — a failed fetch just yields no suggestions and never blocks tag entry.
• Migrates the four form hooks (useToolForm, useResourceForm, usePromptForm, useCreateServerForm) from tags: string to tags: string[], dropping the inline .split(",") / .join(", ") and the parseTags helper. Per-tag sanitize lengths and max-count limits are preserved; the zod schemas now validate z.array(z.string()).
• Wires TagInput into ToolAdvancedSettings, ResourceForm, PromptForm, and CreateServerForm.

📏 Reviewability

• This PR has one clear purpose
• The linked issue is not labeled triage
• Unrelated bugs or improvements are tracked in separate issues/PRs
• Tests are included with the code they validate
• If AI-assisted, I understand and can explain the generated changes

🏷️ Type of Change

• Bug fix
• Feature / Enhancement
• Documentation
• Refactor
• Chore (deps, CI, tooling)

🧪 Verification

Run from client/:

Check	Command	Status
Lint (changed files)	npx eslint <changed .ts/.tsx>	✅ clean
Format (changed files)	npx prettier --check <changed files>	✅ clean
Full unit tests	npx vitest run	✅ 2452 passed

New tag-input.test.tsx (11 tests) covers chip rendering, commit keys, backspace/×-button removal, paste-splitting, case-insensitive dedupe, suggestion filtering + selection, the Create option, maxTags, and disabled. The four hook/form test suites were updated to the string[] shape and to drive the chip input.

📓 Notes

• Read-only tag rendering in cards/detail panels (CardTag) and tag-management CRUD are out of scope, per the issue.
• One behavior nuance: on a prompt update with no tags, the payload now carries tags: [] (previously null) — both mean "no tags", and edit mode pre-fills existing tags, so a save that doesn't touch tags preserves them.
• The /tags endpoint had no prior client consumer; useTagSuggestions is the first, typed against the TagInfo name field.

[Altamimi-Dev]

Folded in the suggestions from the review note on #5640:

1. Max tag validation — added a shared MAX_TAGS constant in client/src/utils/tags.ts, wired as maxTags into the TagInput in all four forms (tools, resources, prompts, virtual servers). Set it to 20 to match the limit useCreateServerForm already enforced, so no existing form regresses and createServer's zod .max() now reads from the same constant. Tool/resource/prompt gain a limit where they had none. TagInput disables entry and shows "Maximum N tags reached." (role="status") once the limit is hit.
2. Tag label helper — added getTagLabels(tags: Array<string | { label: string }>) to client/src/utils/tags.ts and used it in ToolForm; it's exported so the inline-add work (feat(ui-rewrite): add a new tags inline component to each details panel #5662) can share it.
3. Rate limiting — not needed here: useTagSuggestions fetches /tags once per session (module-cached) and filtering is client-side, so there are no per-keystroke API calls to debounce.

Pushed as a follow-up commit.

[marekdano]

@Altamimi-Dev - thanks for the contribution!

Nice work on this! The string → string[] migration is clean and consistent across all four form hooks, building it from repo primitives with no new dependency was the right call, and the combobox/listbox ARIA implementation is genuinely well done (activedescendant, arrow-key nav, backspace-to-remove, focus restoration after removal, role="status" for the max message). Tests ship with the code too. Just one thing worth addressing before merge, plus a few optional notes.

Worth fixing

Resource form dropped per-tag sanitization. The PR notes say per-tag sanitize/length limits are preserved, but that holds for tool/prompt/server (their zod schemas keep sanitizeString(...) in the transform) and not for resources:

// useResourceForm.ts
tags: z.array(z.string()).optional(),         // no transform
// getFormData:
tags: tags.length > 0 ? tags : undefined,     // raw, unsanitized

TagInput only trims - it doesn't strip control chars/CRLF or enforce the 200-char cap that sanitizeString gives. The backend validators are still a backstop, so this is a defense-in-depth gap rather than an open hole, but it's inconsistent with the other three forms. Could we restore the transform on the resource schema (or in getFormData) to match? e.g. z.array(z.string().transform((t) => sanitizeString(t, 200))).

Suggestions

• i18n: TagInput hardcodes a few user-visible / screen-reader strings — Create "<value>", Maximum N tags reached., and the Remove <tag> aria-label. Every consuming form uses intl.formatMessage(...), so it'd be nice to route these through react-intl for consistency.
• Test coverage: the suite is solid, but a couple of core keyboard paths aren't covered - arrow-key + Enter to select a suggestion (only mouse-click selection is tested, and this is the main keyboard-a11y path), Tab/blur commit, and paste that exceeds maxTags.

Minor

• The // ponytail: comment in useTagSuggestions.ts looks like a stray artifact — worth rewording.
• A failed /tags fetch leaves the module cache null, so it refetches on every subsequent form mount (e.g. a 403 per form open for a user without tags.read). A "fetched" sentinel would avoid that if you think it's worth it.
• Placeholders like "tag1, tag2, tag3" read slightly oddly now that it's a chip input (comma still works, so low impact).

All tests should be green.

[Altamimi-Dev]

Thanks for the review — the sanitization regression was a genuine miss, and the i18n and test gaps were fair calls. All taken.

Pushed in fdfa729c8, rebased onto current epic/ui-rewrite.

CI (the two tsc errors): both came from the epic rebase, not from new code — the generated ToolReadTagsItem is now { [key: string]: string }, so ToolReadTagsItem[] no longer satisfied getTagLabels's { label: string } parameter. Rather than cast at the ToolForm call site, I widened the helper to Array<string | { label?: string | null }> and made it drop blank labels, so it adapts to the generated shape instead of hiding the mismatch. The second error was an msw resolver reading DefaultBodyType; it now carries an explicit body type, matching how TestConnectionPanel.test.tsx types its captured bodies. npm run generate && tsc -b is clean locally — that's exactly what the failing "Generate and verify types" job runs. I'd expect the Playwright smoke to follow, since it builds the client; if it stays red I'll pull the failing spec from the run log and fix it here.

Review items:

• Resource tag sanitization restored. useResourceForm runs tags through sanitizeString(t, 200) again, the same shape as useToolForm. It was dropped when the field moved to TagInput, which meant resources took an unsanitized path that tools didn't.
• i18n. The three hardcoded TagInput strings (create option, max-reached message, chip remove label) plus the resource tags placeholder are now common.tagInput.*, added to en-US, es-ES and pt-BR. The component uses useIntl() like copy-value.tsx does, and the tests render through the shared renderWithProviders intl wrapper.
• Placeholder. ResourceForm's "tag1, tag2, tag3" is gone, replaced by common.tagInput.placeholder. I left the prompts placeholder alone — it's already localized and comma entry still works, so it isn't stale.
• Failed-fetch sentinel. useTagSuggestions now guards on a module-level fetched flag set before the request goes out, instead of cache truthiness. A 403 or network failure previously left cache null and refetched on every form mount; cache still holds the data itself.
• Comment reworded — dropped the tooling prefix, kept the substance.
• Four new tests: ArrowDown+Enter selects the highlighted suggestion, Tab commits pending text as a chip, blur commits pending text, and a paste of three tags into a maxTags=2 input adds exactly two and surfaces the max message.

Full client suite green (136 files / 2557 tests), tsc -b and eslint/prettier clean on the touched files.

[Altamimi-Dev]

Correction to my note above — the Playwright failure was not downstream of the type break. I ran the suite locally and it was a real regression in this PR.

virtual-servers.spec.ts:138 fills the tags field with "security, qa" rather than typing it. TagInput only split on delimiters inside the paste handler, so a programmatically set value (Playwright fill, and browser autofill by the same mechanism) reached the blur commit with no comma keydown and became a single chip. The payload went out as tags: ["security, qa"].

Fixed in 03c0c1033 by moving the split into addTags, so every commit path — Enter, comma, Tab, blur, paste, suggestion select — shares one delimiter rule instead of paste having its own. Added a unit test that fills the input programmatically and blurs, which fails without the fix.

Local runs on the pushed head: Playwright 171/171 passed, vitest 2558 passed, tsc -b and eslint/prettier clean. Note the CI runs are sitting in action_required pending maintainer approval, so nothing has executed on GitHub yet.

[Altamimi-Dev]

One more from your placeholder note — I'd only half-done it. Fixed in 6dfb0e0dd.

ToolAdvancedSettings.tsx still hardcoded "Add optional tags separated with commas". That one hit both your suggestions at once: unlocalized, and describing comma-separated entry for what is now a chip input. It goes through common.tagInput.placeholder now, same as the resource form.

Deliberately left alone, for the record:

• prompts.add.placeholder.tags — already localized, and its "e.g., greeting,template,conversation" phrasing still describes something that works.
• InlineTagAdd's "Add tags separated with commas" (the details-panel inline add) — a different component that isn't a chip input, so the wording is accurate there.

Verification on the pushed head: Playwright 171/171, vitest 2558 passed, tsc -b and eslint/prettier clean.

[marekdano]

@Altamimi-Dev — thanks for the thorough follow-ups on this round. The i18n, keyboard-test, and fetch-sentinel items are all cleanly addressed, and the programmatic-fill regression catch in 03c0c1033 was a good one. The combobox/listbox a11y work remains the standout here.

One thing surfaced on a closer read that I'd like to sort before merge, plus a couple of optional notes.

Worth fixing

Tool and Resource forms send tags unsanitized — the schema transform never runs on the payload.

The "restore resource tag sanitization" commit added the transform to the schema, which reads correctly, but for these two hooks getFormData builds the payload by hand and never calls schema.parse — the transform only runs in validateForm/isValid, whose transformed output is discarded. So it's inert for what actually gets submitted.

Before this PR, both hooks sanitized each tag inline in getFormData:

// old getFormData (tool + resource)
tags: tags ? tags.split(",").map((t) => sanitizeString(t.trim(), 200)).filter(Boolean) : undefined,

Now:

// new getFormData (tool + resource)
tags: tags.length > 0 ? tags : undefined,   // raw — no sanitizeString

Net effect: tool and resource submissions now carry tags with no control-char/CRLF stripping and no 200-char cap (TagInput only trims). usePromptForm and useCreateServerForm are unaffected — their getFormData calls schema.parse(...) and returns the parsed tags, so the transform genuinely applies there.

This is defense-in-depth rather than an open hole — the backend validators are still a backstop — but the PR notes say "Per-tag sanitize lengths and max-count limits are preserved," and that doesn't hold for tool/resource. Simplest fix is to sanitize where the payload is built, matching the other fields already handled there:

tags: tags.length > 0 ? tags.map((t) => sanitizeString(t, 200)) : undefined,

in both useToolForm.getFormData and useResourceForm.getFormData. A payload-level assertion in the two hook test suites would have caught this and would guard against it regressing again.

Minor notes (optional)

• Focus after removing a chip at maxTags: removeAt calls inputRef.current?.focus() while the input is still disabled for that render (atMax is still true until the state update commits), so focus can be briefly dropped before it re-enables. Small edge case.

Everything else from the previous round looks good to me. Once the tag sanitization is restored on the tool/resource payloads, I'm happy to approve.

[Altamimi-Dev]

Thanks @marekdano, both spot-on — you're right that the schema transform was inert for these two hooks. Fixed in e49731066.

Tag sanitization on tool/resource payloads

useToolForm.getFormData and useResourceForm.getFormData now sanitize where the payload is built, alongside the other fields already handled there:

const cleanTags = tags.map((t) => sanitizeString(t, 200)).filter(Boolean);
// ...
tags: cleanTags.length > 0 ? cleanTags : undefined,

One deviation from your suggested snippet: I kept the .filter(Boolean) from the pre-PR code. A tag consisting only of control characters sanitizes to "", and sending an empty string seemed worse than dropping it — that matches the old behaviour you quoted. Happy to drop it if you'd rather stay closer to the literal suggestion.

usePromptForm and useCreateServerForm are untouched, since as you noted their getFormData returns schema.parse(...) output and the transform genuinely applies.

Payload-level tests

Added the assertions you asked for, in both useToolForm.test.ts and useResourceForm.test.ts:

result.current.setTags(["  clean  ", "evil\r\ninject", "x".repeat(250), "\x00\x07"]);
// ...
expect(payload.tool.tags).toEqual(["clean", "evilinject", "x".repeat(200)]);

That covers CRLF stripping, the 200-char cap, and the all-control-chars drop. I verified both fail against the previous code. Also renamed "keeps tags array as-is in getFormData" to "passes clean tags through unchanged" — the old name described exactly the contract we no longer want.

Focus after removing a chip at maxTags

Took this one too, since it was a small change. removeAt now flags a pending focus and a useLayoutEffect restores it after the commit, so it lands on an input that has actually re-enabled:

const removeAt = (index: number) => {
  onChange(value.filter((_, i) => i !== index));
  pendingFocusRef.current = true;
};

useLayoutEffect(() => {
  if (pendingFocusRef.current) {
    pendingFocusRef.current = false;
    inputRef.current?.focus();
  }
});

This covers both remove paths (× button and Backspace) and keeps the non-max behaviour identical — the layout effect runs before paint, so there's no visible flicker. New test renders with maxTags={1}, removes the only chip, and asserts the combobox is both enabled and focused; it fails against the old synchronous focus().

Verification

• npx vitest run — 2561 passed, 1 skipped (136 files)
• npx playwright test — 171 passed
• tsc -b, eslint, and prettier --check clean on the touched files

[marekdano]

@Altamimi-Dev — all three items from the last round are cleanly resolved, and CI is green.

LGTM 🚀