Skip to content

Review 4329

Jump to bottom
Cindy Zhang edited this page Aug 27, 2026 · 1 revision

Review 4329

PR

#4329 feat(Tokenizer): split delimited text into separate tokens on type and paste (#4175) by AKnassa (bucket: contributor)

HEAD REVIEWED

54cd11e3b1e13ef890baa5242f45cfe347e404c6

VERSIONS

LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13

LANE

LANE: full WHY: contributor PR adds public API and core runtime behavior; the exact-head visual job is pending/cancelled, so local browser evidence is required.

PROBLEM

WHY 1: A free-text Tokenizer currently treats a, b, c as one candidate value instead of a pasted list. WHY 2: People adding email/tag/name lists must manually split common clipboard formats or ship a single messy token. WHY 3: Tokenizer exists to build a set of values quickly; list paste failing makes the main data-entry job slower and error-prone. USER-FACING PROBLEM: A user pasting several tags into a creatable Tokenizer cannot turn the list into separate tokens without manual cleanup. PROBLEM SEVERITY: harmful friction — #4175 states the common email/tag paste case, and warm main creates one token from the whole comma list after Enter.

VERDICT: clear

SOLUTION

Tokenizer adds a delimiter config for free-text creation and rewrites edited input text before the query is stored. The same path handles typing, paste, and composition end, so a list can become a batch of created tokens while ordinary search and Enter-to-create stay intact. PowerSearch opts out because its saved string-list values may legitimately contain commas.

SOLUTION (4 decisions · ~255 runtime/helper lines of 1423 changed lines)

  1. delimiters controls create-on-delimiter for Tokenizer.
  2. transformQuery lets BaseTypeahead run the rewrite before search/query state.
  3. TokenizerChange.items? carries multi-token create metadata while preserving item.
  4. PowerSearch sets delimiters={[]} to preserve comma-containing saved filters.

BURDEN: medium — one public prop, one public callback prop on BaseTypeahead, one optional callback payload field, a pure parser helper, no effects/listeners/observers. BURDEN MATCH: mostly proportionate — the stated problem is a common list-entry flow, but BaseTypeahead exposing transformQuery is API surface created for internal Tokenizer plumbing.

VERDICT: note — public API shape still needs maintainer judgement after the correctness block is fixed.

ARCHITECTURE

OWNER: Tokenizer owns free-text token creation; BaseTypeahead owns the private query input and paste/composition event boundary. TIER 1: reuses BaseTypeahead, SearchSource, useAnnounce, useTranslator for existing single-token announcements. TIER 2: no theme/size/status architecture change. SEAMS: search source, hasCreate, hasEntriesOnFocus, disabledMessage/readOnly, PowerSearch string_list, docs/template stories. BEHAVIOR UNIT: pure utility (delimiters.ts) plus inline Tokenizer callback — testable for splitting; BaseTypeahead transformQuery should be judged as public plumbing.

seam driven result
hasCreate + default delimiter story Chromium paste produced three tokens and cleared the input.
delimiters={[]} story Chromium typing Smith, John kept the comma in the input and created no split token before Enter.
custom delimiter story Chromium semicolon paste produced two tokens and cleared the input.
focusable-disabled story Chromium paste did not mutate the disabled tokenizer.
PowerSearch string_list Focused test proves Acme, Inc. remains one prospective value.

VERDICT: note — implementation owner is plausible, but the public BaseTypeahead prop should be reviewed as API surface.

IMPACT

Every Tokenizer hasCreate gets comma/newline create behavior by default. Builders with comma-containing values must opt out with delimiters={[]}; PowerSearch does so internally for saved string-list filters. Assistive-technology users hear the same live-region path as single create, and that path currently hardcodes the new batch announcement.

VERDICT: BLOCKS — non-English screen-reader users hear a new English batch announcement.

API

Smallest real call site:

<Tokenizer
  label="Tags"
  searchSource={emptySource}
  value={tags}
  onChange={setTags}
  hasCreate
  delimiters={[]}
/>
change public? class doc'd? verdict
+ `Tokenizer.delimiters?: ReadonlyArray RegExp = [',', '\n', '\r']` public via core barrel creatable token entry; no landed sibling with this prop en+zh+dense
~ TokenizerChange<T>.create + items?: T[] public via core barrel batch change metadata en+zh+dense ok shape, but API judgement rides with delimiter feature
+ BaseTypeahead.transformQuery?: (nextQuery: string) => string public via Typeahead barrel/docs no external caller; added for Tokenizer en+zh/dense needs maintainer API judgement

OSSIFICATION: delimiters is the exact surface Cindy's issue proposed for this class, with [] as the comma-safe escape hatch. items? is backward-compatible and avoids N stale onChange calls. transformQuery is the costly line: because BaseTypeaheadProps is exported, Tokenizer's private editing hook becomes a general public mutation point unless renamed internal or otherwise accepted.

VERDICT: [Needs human judgement] — contributor-added public API surface, especially BaseTypeahead.transformQuery.

THEMING

No theme targets, tokens, variables, or style contracts are added or removed. The only story style conflict from the refresh kept --color-text-secondary for the explanatory text.

VERDICT: clear

BREAKING

BEHAVIOR: yes, intentionally — hasCreate now splits comma/newline-delimited text by default; PowerSearch pins the old comma behavior with delimiters={[]}. API: additive — new optional prop and optional change metadata, no removed or required field. VISUAL: yes, intentional state result — one pasted list now renders several tokens instead of one candidate string; captured in exact-head Storybook. THEME: no target/token change.

VERDICT: note — behavior change is the stated feature and has a patch changeset.

PERFORMANCE & RESOURCES

EFFECTS: zero added/changed Effects.

Effect + deps external system why render/handler cannot do it measured render cost lifetime + cleanup focused test
none none n/a n/a n/a n/a

RENDER: no added render-time loops beyond compiling delimiter pattern in useMemo; split work runs only on user edit/paste/composition. LISTENERS/OBSERVERS: no new persistent listener/observer; paste and composition are React input handlers. LAYOUT: no layout reads/writes. BUNDLE: no dependency change; pure helper file only.

VERDICT: clear

VISUAL EVIDENCE

VISUAL CHECK: manual frames required WHY: the acceptance behavior is visible and the exact-head visual-acceptance status was pending/cancelled; local Storybook captured the changed state.

A pasted comma list becomes separate tokens

Before After
Before After

SENSOR RECEIPT: build 54cd11e3b1e13ef890baa5242f45cfe347e404c6, story core-tokenizer--creatable, theme neutral, color mode light, direction ltr, viewport 900x420@1, hover pointer, reduced motion false, target .astryx-tokenizer count 1, fonts loaded, zero page/Storybook errors. After state: tokens a@x.com, b@y.com, c@z.com; input empty; live region Added 3 items.

VERDICT: clear

REMEDY SEARCH

REMEDY SEARCH: not triggered — no proven visual defect

A11Y & I18N

Auto-covered: CI lint failed on the exact head for @astryx/no-hardcoded-i18n-string at Tokenizer.tsx:774 and :775. Checked by me: Chromium shows the new multi-token path writes Added 3 items to the polite live region. Source shows single add/remove announcements use t('@astryx.tokenizer.tokenAdded'), but the batch branch hardcodes English. Browser-needed: keyboard/paste visible behavior was driven in Chromium for create, custom delimiter, comma-safe opt-out, and focusable-disabled paste.

VERDICT: BLOCKS — non-English screen-reader users hear the new batch-created announcement in English.

JUDGEMENT

slot verdict
PROBLEM clear
SOLUTION note — public API shape still needs maintainer judgement after the correctness block is fixed
ARCHITECTURE note — BaseTypeahead public plumbing needs API review
IMPACT BLOCKS — non-English AT users hear English
API [Needs human judgement] — contributor-added public API surface
THEMING clear
BREAKING note — intended behavior change
PERFORMANCE clear
VISUAL clear
A11Y & I18N BLOCKS — hardcoded live-region announcement

GOAL: met — warm main turns the comma list into one candidate/token; exact-head Storybook turns it into three tokens and focused Vitest passes 190 tests. DISPOSITION: hardcoded batch announcement → blocks now; API surface → human judgement after the correctness block; stale minQueryLength JSDoc → evidence not spent, can be fixed with the API/doc pass. ADVICE: proven existing pattern — the same file already routes single add/remove announcements through useTranslator() keys. AUTHOR CAN PROCEED: yes — route the batch announcement through a catalog key with ICU count and update the localization test. WORST OUTCOME: “non-English screen-reader users hear the new batch-created announcement in English” → request changes

JUDGEMENT NEEDED: API — BaseTypeahead.transformQuery and Tokenizer.delimiters are new public surface, but the blocking i18n defect comes first.

request changes

  1. The batch-created announcement is hardcoded English → non-English screen-reader users hear Added 3 items instead of a localized live-region update · packages/core/src/Tokenizer/Tokenizer.tsx:774

REVIEW

Thanks, this does make the list-paste path work: local Storybook turns a@x.com, b@y.com, c@z.com into three tokens, and the focused Tokenizer/Typeahead/PowerSearch tests are green.

One thing blocks merge: the new batch announcement is hardcoded English, so screen-reader users in non-English locales still hear Added 3 items while the single add/remove path uses catalog strings. Could you route the batch message through useTranslator() with an ICU count key and update the localization test? If you'd rather talk it through with someone, we're in Discord.

[Reviewed by Robohands]

INLINE (0–3, only when genuinely line-anchored)

  • packages/core/src/Tokenizer/Tokenizer.tsx:774 — This live-region string needs the catalog, like the single add/remove announcements.

EVIDENCE I DID NOT SPEND

  • TokenizerProps.minQueryLength JSDoc still says Create waits below threshold, but current docs/behavior say Create remains available.
  • BaseTypeahead.transformQuery is public because BaseTypeaheadProps is exported from Typeahead/index.ts.
  • Exact-head CI lint is red for the same hardcoded announce string.

TIME

TIME total 10m setup 1m fast-install 40s + one build 2s + Storybook ready 0s; warm main reused: yes reading 4m rules, PR intent, source/types/tests/call-sites/history/API radius (prep phase) measuring 0m focused Vitest 8s + Chromium Storybook 4s + warm-main comparison 2s writing 4m presentation, gate, wiki record waste 0m first screenshot sensor expected default theme/mode incorrectly, then corrected to neutral/light

WHAT I COULD NOT VERIFY

  • CI was still in progress except lint, which had already failed on the reported issue.

What changed before posting

Not posted to the PR. Drafted read-only; gate 1 passed with no rewrite.

Clone this wiki locally