feat(TreeSelect): add renderTag prop (DS-5277) - #472
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughTreeSelect adds ChangesTreeSelect custom tag rendering
Priority: ⬇️ Low — Defer this TreeSelect enhancement because it narrowly adds custom selected-tag rendering and its public API types without supplied evidence of elevated product urgency. Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The TreeSelect custom tag API and associated behavior coverage are ready to merge with no identified current-head risk. Sequence Diagram(s)sequenceDiagram
participant Consumer
participant TreeSelect
participant SelectedTags
participant TreeSelectTag
Consumer->>TreeSelect: Provide renderTag
TreeSelect->>SelectedTags: Forward renderTag
SelectedTags->>TreeSelectTag: Render selected item with tag props
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 4 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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 |
|
Visit the preview URL for this PR (updated for commit 59a4730): https://react-koobiq-next--prs-472-z093159k.web.app (expires Sun, 13 Sep 2026 14:27:31 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: fc29847d4a9e5cb1adf458c76a9b681c76e2eeff |
There was a problem hiding this comment.
🟢 Approval recommended
The change is a small, fully-tested addition that mirrors the established SelectNext renderTag pattern with correct end-to-end wiring and updated public API guard.
Pull request overview
This PR adds a renderTag prop to TreeSelect, letting consumers customize how selected tags are rendered in multiple selection mode. It also exposes the TreeSelect.Tag compound slot and a TreeSelectTagProps type. The implementation reuses the existing shared SelectedTags component (which already supports renderTag for both responsive and multiline overflow), so the change is mostly plumbing plus public API, tests, story, and docs. It mirrors the identical renderTag/Select.Tag feature already present in SelectNext, keeping the two multi-select components consistent.
Changes:
- Add
renderTag?: (item: Node<T>, tagProps: TagProps) => ReactNodeprop and forward it toSelectedTags; ignored in single selection mode. - Expose
TreeSelect.Tagslot andTreeSelectTagProps, updating the API Extractor report accordingly. - Add tests, a
CustomTagRenderstory, and MDX documentation; bump the story badge tostatus:updated/date:2026-09-07.
File summaries
| File | Description |
|---|---|
packages/components/src/components/TreeSelect/types.ts |
Adds Node/TagProps imports, TreeSelectTagProps, and the renderTag prop type with JSDoc. |
packages/components/src/components/TreeSelect/TreeSelect.tsx |
Destructures renderTag, forwards it to SelectedTags, and registers the TreeSelect.Tag slot. |
packages/components/src/components/TreeSelect/TreeSelect.test.tsx |
Adds coverage for single-mode ignore, responsive/multiline customization, tag removal, and disabled/read-only states. |
packages/components/src/components/TreeSelect/TreeSelect.stories.tsx |
Adds CustomTagRender story, subcomponent entry, and updates status/date tags. |
packages/components/src/components/TreeSelect/TreeSelect.mdx |
Documents the renderTag usage with a story reference. |
tools/public_api_guard/components/TreeSelect.api.md |
Regenerated API report reflecting renderTag and TreeSelectTagProps. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| defaultValue={[2, 3]} | ||
| renderTag={(item, tagProps) => ( | ||
| <TreeSelect.Tag | ||
| {...tagProps} |
There was a problem hiding this comment.
A consumer slotProps silently makes the tag unremovable and plants a focusable control inside an aria-hidden subtree.
tagProps.slotProps carries the entire removal wiring — { removeIcon: { as: 'div', tabIndex: undefined, onPress: onRemove, isDisabled: isReadOnly || isDisabled } } (SelectedTagsResponsive.tsx:60-67). Because JSX later-prop-wins and slotProps is a nested object, a completely natural override shallow-replaces it:
<TreeSelect.Tag {...tagProps} slotProps={{ icon: { className: 'mine' } }}>I ran this. Result:
- the remove control falls back to
IconButton's defaultas='button'→tagName: BUTTON,tabindex="0"— a keyboard-focusable element inside thearia-hidden="true"container (SelectedTagsResponsive.tsx:39), which is an axearia-hidden-focusviolation; onPressis gone, so clicking the × does nothing —onChangewas never called;- the
isReadOnlyguard is gone too.
All of it type-clean and silent. The same hazard applies to className (dropping s.tag disables the [aria-hidden='true'] { position: absolute; visibility: hidden } rule in SelectedTags.module.css:40-46).
Worth either documenting "spread tagProps and don't replace slotProps/className", or — better — having SelectedTags provide the wiring via context so TreeSelect.Tag merges instead of being clobbered.
There was a problem hiding this comment.
This PR exposes the existing renderTag API in TreeSelect, following Select. It does not change how SelectedTags builds tagProps or how Tag handles overrides. The example already forwards these props. Moving the wiring into context would be a separate change to the shared implementation and should be considered for both components together.
| isRequired, | ||
| }} | ||
| selectedTagsOverflow={selectedTagsOverflow} | ||
| renderTag={renderTag} |
There was a problem hiding this comment.
renderTag type-checks in single selection mode but is silently dropped here.
TreeSelectProps<T, M> accepts renderTag for every M, yet only the 'multiple' branch forwards it — the single branch renders state.selectedItems[0]?.textValue. There is no dev warning (grep NODE_ENV in this directory returns nothing), and the MDX section does not mention the precondition even though the section directly above it states it for the sibling selectedTagsOverflow. The new test at line 372 codifies the silent drop as intended.
The file already has the machinery to make this a compile error: useTreeSelectState.ts:27-38 defines TreeSelectValueType<M> / TreeSelectChangeValueType<M>, which TreeSelectProps inherits — that is why defaultValue={[2, 3]} in single mode already errors today.
I tried the one-line constraint and type-checked the whole package:
renderTag?: M extends 'multiple'
? (item: Node<T>, tagProps: TagProps) => ReactNode
: never;- single mode +
renderTag→ errors, as intended - multiple mode +
renderTag→ no false positive - dynamic
selectionMode={someUnion}→ no error (the naked type parameter distributes, so it degrades permissively) - across every story and test in
packages/components, exactly one error surfaces:TreeSelect.test.tsx:375, i.e. this PR's own single-mode test
Two caveats if you take it: the message reads not assignable to type 'undefined', which is correct but unhelpful; and SelectNext/types.ts:79 carries the byte-identical loose signature, so it is worth doing both at once — or leaving the types as they are and adding a process.env.NODE_ENV !== 'production' guarded logger.warn instead.
There was a problem hiding this comment.
Ignoring renderTag in single-selection mode is intentional and matches Select. The callback customizes selected tags, which are only rendered in multiple-selection mode. This is now stated in the documentation. Introducing a conditional type or a runtime warning would change the shared API convention and should be discussed separately for both components.
| renderTag={(item, tagProps) => ( | ||
| <TreeSelect.Tag | ||
| {...tagProps} | ||
| variant="warning-fade" |
There was a problem hiding this comment.
The documented example teaches an override that permanently suppresses the invalid state.
SelectedTags derives the tag colour from form state — variant: isInvalid ? 'error-fade' : 'contrast-fade' (SelectedTagsResponsive.tsx:59, SelectedTagsMultiline.tsx:41). Placing a literal variant after {...tagProps} pins it, so a field that fails validation keeps warning-coloured tags while its border and error message go red. It carries into the remove button too, via matchTagVariantToIconButton[variant] (Tag.tsx:64).
The test at lines 406-409 asserts the override wins, and isInvalid appears exactly once in the whole test file (line 283) — outside the renderTag block — so the interaction is untested.
This is copied from SelectNext/Select.stories.tsx:459-463, so it is propagated rather than introduced; that makes it a two-component docs problem worth fixing once. Either note the trade-off in the MDX, or have SelectedTags apply state-derived visuals after the consumer's props.
There was a problem hiding this comment.
The explicit variant override is intentional: this story demonstrates customizing the tag’s appearance. tagProps provides the default state-dependent variant, and the consumer can choose to preserve or override it. Applying the error variant after consumer props would restrict that customization. The field’s validation state itself is unaffected.
| expect(renderTag).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| describe.each(['responsive', 'multiline'] as const)( |
There was a problem hiding this comment.
No test pins the ReactNode half of the contract.
Every callback in this block returns <TreeSelect.Tag {...tagProps}>, and Tag absorbs ref / className / aria-hidden into its root div — so the assertions would pass identically if SelectedTags stopped supplying them.
The reference implementation covers exactly this: SelectNext/__tests__/SelectMultiple.test.tsx:173-192 returns a plain <div> built from a partial destructure of { className, ref, 'aria-hidden': ariaHidden }.
I injected the regression (making SelectedTags* render only nodes whose type is Tag):
TreeSelect.test.tsx -> 66 passed (66) # green
SelectMultiple.test.tsx -> 1 failed | 9 passed
CI as a whole still catches it, because both components share SelectedTags — so this is a suite-symmetry gap rather than a repo-level hole. Mirroring SelectNext's test here is cheap, and it would also pin the ref / className contract that the responsive mode depends on.
(Separately: the describe.each over both overflow modes is earning its keep. I mutated SelectedTagsMultiline alone and only the multiline arm failed, so the two arms are not redundant despite rendering identical markup in jsdom.)
There was a problem hiding this comment.
Agreed. Added a plain div renderer test for both overflow modes, including checks for className forwarding and responsive measurement ref attachment.
| TreeSelect.Item = Tree.Item; | ||
| TreeSelect.ItemContent = Tree.ItemContent; | ||
| TreeSelect.LoadMoreItem = Tree.LoadMoreItem; | ||
| TreeSelect.Tag = Tag; |
There was a problem hiding this comment.
TreeSelect.Tag is the first slot here whose component type no api report covers.
The other three slots are safe: Tree is in tools/api-extractor/config.json, so Tree.api.md backs Item / ItemContent / LoadMoreItem. Tag is guarded nowhere — it is absent from that components array, and the compound itself is emitted opaquely (export const TreeSelect: CompoundedComponent_2;, with CompoundedComponent_2 flagged as a forgotten export), so no slot appears in TreeSelect's report either.
I mutated the built Tag types to check what that costs — renamed variant, deleted icon and allowsRemoving, and cut tagPropVariant from four values to one. Re-running API Extractor for TreeSelect produced a report identical to the committed one, so pnpm check-api stays green while every renderTag consumer — and this PR's own story, which passes variant and icon — breaks at compile time.
Adding "Tag" to the components array closes it, and covers SelectNext.Tag at the same time.
Worth knowing while you are here: SelectNext is missing from that array too, so tools/public_api_guard/components/SelectNext.api.md is an orphaned report that is never regenerated or checked — it still has neither renderTag nor SelectNextTagProps.
There was a problem hiding this comment.
The API coverage gap is valid, but it predates this PR: TreeSelect.Tag exposes the existing Tag component, already exposed through SelectNext.Tag, without changing its props. Adding shared Tag coverage and restoring SelectNext report generation should be handled in a separate tooling change. This PR updates TreeSelect’s own API report.
| children: FileNode[]; | ||
| }; | ||
|
|
||
| const items: FileNode[] = [ |
There was a problem hiding this comment.
This fixture shadows the module-level items and reuses its ids for different nodes.
TreeSelect.stories.tsx:35 already defines items, which the other dynamic stories in this file share. The local one shadows it, renames the child key test to children, and reassigns ids: 3 is index.html (nested under Http) in the module fixture but config (a root) here, and 4 is Providers vs public. Since the MDX renders story source verbatim, a reader comparing "Selected value tags" with "Custom tag render" has to re-derive which tree is which.
The module fixture already contains app(1) → Http(2), config(6) and public(9), so items={items} with defaultValue={[2, 6]} reproduces this story — nested selection included — and drops ~15 lines along with the FileNode type and the children: [] boilerplate it forces.
There was a problem hiding this comment.
The local fixture is intentional. Our Storybook convention keeps example data inside render so the displayed source is self-contained. Item IDs are scoped to each collection and do not need to match IDs in other stories. This small fixture demonstrates nested selection without pulling in the larger shared tree.
| export type TreeSelectItemProps = TreeItemProps; | ||
| export type TreeSelectItemContentProps = TreeItemContentProps; | ||
| export type TreeSelectLoadMoreItemProps = TreeLoadMoreItemProps; | ||
| export type TreeSelectTagProps = TagProps; |
There was a problem hiding this comment.
Third verbatim copy of this signature.
The identical declaration — and the identical JSDoc line — now lives in three places:
SelectedTags/types.ts:30(the producer; the only place it is actually called)SelectNext/types.ts:79- here
TreeSelect only forwards the value, so nothing type-checks the copies against the producer; structural compatibility hides drift instead of failing the build. That drift has already started on the sibling prop: SelectNext documents selectedTagsOverflow with its value list and @default 'responsive', while TreeSelect's copy is a bare one-liner — and per AGENTS.md the Storybook Props table is generated from those comments, so TreeSelect's docs page omits the default it actually applies.
This file already establishes the fix eleven lines below: treeSelectPropSelectedTagsOverflow = selectedTagsPropOverflow re-exposes a SelectedTags contract under a TreeSelect name. Exporting the prop type from SelectedTags (SelectedTagsProps<T>['renderTag'], or a named SelectedTagsRenderTag<T>) and referencing it here would finish the pattern — and make the new Node import unnecessary.
There was a problem hiding this comment.
The callback is type-checked against SelectedTags when it is forwarded, so incompatible signatures are already checked by TypeScript. Keeping an explicit public signature also lets it reference the consumer-facing TreeSelectTagProps name. Extracting a shared renderer type is a possible refactoring, but is not required for this addition. The existing selectedTagsOverflow documentation is separate from this change.
| expect(screen.queryByTestId('tag-1')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('should remove a custom tag without opening the dropdown', async () => { |
There was a problem hiding this comment.
Removing a tag does not work on the first click while the dropdown is open, and this is the only removal test.
The title scopes it to the closed dropdown, which is the path that works. With the popover open the click on a tag's ✕ is consumed by the overlay's dismiss instead:
selectionMode="multiple", defaultValue={[2, 7]}, defaultOpen, renderTag
click ✕ inside tag-2 -> onChange: 0 calls
onOpenChange: [false]
tag-2 still in the DOM
Only after the popover finishes unmounting does a second click produce onChange([7]). It behaves identically with default tags, so this is pre-existing rather than something renderTag introduced — but it is exactly the flow a reader of the new CustomTagRender story will hit first (open the list, change your mind, click ✕, nothing happens), and eight green tests around the closed path give the opposite impression.
Worth either a companion test asserting the open-dropdown behaviour you actually want, or a fix so the tag's remove control does not lose its press to the dismiss handler.
There was a problem hiding this comment.
This also reproduces with default tags and without renderTag: the existing modal overlay consumes the first outside interaction to dismiss the dropdown. This PR does not change overlay dismissal or tag removal handling. Changing that interaction should be addressed separately; the new test is explicitly scoped to removal while the dropdown is closed.
Summary by CodeRabbit
New Features
TreeSelectwith therenderTagoption.TreeSelect.Tagfor creating styled custom tags, including icons and warning variants.Documentation