Skip to content

feat(TokenizedInput): Added new component TokenizedInput#375

Merged
feelsbadmans merged 20 commits intomainfrom
feat/tokenized-input
Apr 8, 2026
Merged

feat(TokenizedInput): Added new component TokenizedInput#375
feelsbadmans merged 20 commits intomainfrom
feat/tokenized-input

Conversation

@feelsbadmans
Copy link
Copy Markdown
Contributor

No description provided.

@gravity-ui-bot
Copy link
Copy Markdown
Contributor

Preview is ready.

Copy link
Copy Markdown
Contributor

@korvin89 korvin89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: TokenizedInput

Architecturally well-designed (composition pattern, undo/redo, platform-specific shortcuts), but the implementation needs work on reliability and testability. See inline comments below.

High: 9 issues (potential runtime crashes, memory leaks, SSR problems, stale closures)
Medium: 16 issues (missing tests for hooks, single context perf, 501-line hook, unnecessary dependency)
Low: 10 issues (accessibility, magic numbers, minor optimizations)

}
return navigator.userAgent.toUpperCase().indexOf('MAC') >= 0;
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isMac() is called at module level — in SSR environments window is undefined, so the result is always false (Win shortcuts). If the module is cached on the server and reused on the client, shortcuts will be wrong.

Also, navigator.userAgent parsing is a legacy approach. Consider navigator.userAgentData?.platform or at least lazy initialization inside the hook:

const useShortcuts = () => {
    return React.useMemo(() => {
        if (typeof window === 'undefined') return winShortcuts;
        return navigator.userAgent.toUpperCase().includes('MAC')
            ? macShortcuts
            : winShortcuts;
    }, []);
};

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the shortcut logic into a dedicated useShortcuts hook with lazy initialization inside useMemo

undoRedo,
],
);
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

501 lines in a single hook is a lot. Each sub-handler (navigationHandler, deleteHandler, blurHandler, undoRedo) could be extracted into its own hook for better readability and testability.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely decomposed useKeyDownHandler into smaller, focused hooks (useNavigationHandler, useDeleteHandler, useBlurHandler, useUndoRedoHandler) for better readability and testability


if (
(focus.key === key && focus.idx === idx) ||
tokens[idx].options?.readOnlyFields?.includes(key)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential crash: tokens[idx] can be undefined if prevField returned an invalid idx.

tokens[idx].options?.readOnlyFields?.includes(key)

Add a guard:

const token = tokens[idx];
if (!token) return false;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

value: {...t.value, ...newValue},
errors: undefined,
}
: {...t},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onRemoveToken has no bounds check on idx. If idx < 0 or idx >= tokens.length, filter silently returns the array unchanged, but undoRedoManager records an invalid operation.

if (idx < 0 || idx >= tokens.length) return;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

shouldAllowBlur: () => true,
},
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A single context holds inputInfo, focusInfo, and options. Any change in any of them triggers a re-render of all consumers. For a component with frequent updates (every keystroke), this can be a performance problem.

Consider splitting into 2-3 contexts (input, focus, options) so consumers only re-render when their specific slice changes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


const token = tokens[idx];

const hasChanges = React.useRef(false);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasChanges ref is not reset when idx changes. If React reuses the component for a different token (on delete/insert), the ref may remain true from the previous one:

React.useEffect(() => {
    hasChanges.current = false;
}, [idx]);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added effect

onClick={onClear}
tabIndex={-1}
aria-label="tokenized-input-clear-button"
>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aria-label="tokenized-input-clear-button" is a technical identifier, not a user-facing label. For accessibility:

aria-label={i18n('clear-input')}

Also, <Xmark /> is missing a title prop — gravity-ui guidelines recommend adding title to Icon components.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced the hardcoded aria-label with a localized i18n('clear_input') string. Note: Since the icon component doesn't accept a title prop directly, I added the title attribute to the wrapping instead to comply with accessibility guidelines

} from './components';

export type TokenValueBase = Record<string, string>;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isNew is runtime state, not token data. Mixing runtime flags with data in one type blurs separation of concerns. Consider a discriminated union:

type Token<T> = 
    | { kind: 'regular'; id: string; value: T; errors?: ...; options?: ... }
    | { kind: 'new'; id: string; value: T };

- `Escape` — close the suggestions menu; press again to remove focus
- `Cmd + I` (Mac) / `Ctrl + I` (Win/Linux) — open the suggestions menu
- `Cmd + Enter` (Mac) / `Ctrl + Enter` (Win/Linux) — finish the current token and go to the next (when the suggestions menu is closed)
- `Enter` — select a suggestion / finish the current token and go to the next (when the suggestions menu is closed)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a 5500-line component, the README is quite brief. Missing:

  • API reference (props table)
  • Usage examples (code snippets)
  • Composition pattern description — how to override sub-components
  • Mac vs Win shortcuts listed separately

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expanded the documentation significantly. Added usage examples with code snippets, a detailed description of the composition pattern (how to override sub-components), and separate shortcut lists for Mac and Windows/Linux

@feelsbadmans feelsbadmans force-pushed the feat/tokenized-input branch from 6afcc91 to 6590c92 Compare March 27, 2026 21:45
@feelsbadmans
Copy link
Copy Markdown
Contributor Author

Hi @korvin89. Thanks for detailed review and comments. I've taken note of all comments and added additional tests for hooks and utils

@feelsbadmans feelsbadmans requested a review from korvin89 April 8, 2026 08:18
@feelsbadmans feelsbadmans merged commit e861428 into main Apr 8, 2026
4 checks passed
@feelsbadmans feelsbadmans deleted the feat/tokenized-input branch April 8, 2026 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants