feat(Tabs): add renderTabs for library-agnostic drag-and-drop#2723
Draft
korvin89 wants to merge 3 commits into
Draft
feat(Tabs): add renderTabs for library-agnostic drag-and-drop#2723korvin89 wants to merge 3 commits into
korvin89 wants to merge 3 commits into
Conversation
Contributor
Reviewer's GuideAdds a new renderTabs escape hatch to TabList to enable library-agnostic drag-and-drop wrapping of visible tabs (including with contentOverflow="collapse"), plus Storybook examples and runtime validation of the contract. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
korvin89
marked this pull request as draft
July 1, 2026 19:01
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
useEffectthat validatesrenderTabshas no dependency array, so it runs on every render; consider adding a dependency list (e.g.[renderTabs, shownChildren.length, bTab()]) or otherwise tightening it to avoid unnecessary DOM reads and warnings. - The
renderTabsprop is typed as(tabs: React.ReactElement[]), butshownChildrencan theoretically contain non-Tabelements; if that’s not supported, it might be safer to narrow/validate the element type or explicitly document that onlyTabchildren are allowed and relied upon here.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `useEffect` that validates `renderTabs` has no dependency array, so it runs on every render; consider adding a dependency list (e.g. `[renderTabs, shownChildren.length, bTab()]`) or otherwise tightening it to avoid unnecessary DOM reads and warnings.
- The `renderTabs` prop is typed as `(tabs: React.ReactElement[])`, but `shownChildren` can theoretically contain non-`Tab` elements; if that’s not supported, it might be safer to narrow/validate the element type or explicitly document that only `Tab` children are allowed and relied upon here.
## Individual Comments
### Comment 1
<location path="src/components/tabs/TabList.tsx" line_range="60-78" />
<code_context>
+ // Dev-only validation of the `renderTabs` result: `warnOnce` already gates on
+ // NODE_ENV and dedupes. We check the actual DOM (the element tree can't be validated
+ // reliably — dnd libraries hide the `<Tab>` behind a render function).
+ React.useEffect(() => {
+ if (!renderTabs || !listRef.current) {
+ return;
+ }
+ const tabClassName = bTab();
+ const shownCount = Array.from(listRef.current.children).filter((child) =>
+ child.classList.contains(tabClassName),
+ ).length;
+ if (shownCount !== shownChildren.length) {
+ warnOnce(
+ 'TabList: `renderTabs` must render exactly one visible tab per received element ' +
+ '(preserve keys, no extra wrapper DOM around a tab). A wrapper element breaks ' +
+ 'the `contentOverflow="collapse"` measurement.',
+ );
+ }
+ });
+
return (
</code_context>
<issue_to_address>
**suggestion (performance):** The validation effect is missing a dependency array and will run after every render.
Since this dev-only effect only depends on `renderTabs`, `shownChildren.length`, and `listRef.current`, it’s doing extra work by running after every render. Consider adding a dependency array such as `[renderTabs, shownChildren.length]` and checking `listRef.current` inside the effect, so it only runs when the relevant inputs change.
```suggestion
// Dev-only validation of the `renderTabs` result: `warnOnce` already gates on
// NODE_ENV and dedupes. We check the actual DOM (the element tree can't be validated
// reliably — dnd libraries hide the `<Tab>` behind a render function).
React.useEffect(() => {
if (!renderTabs || !listRef.current) {
return;
}
const tabClassName = bTab();
const shownCount = Array.from(listRef.current.children).filter((child) =>
child.classList.contains(tabClassName),
).length;
if (shownCount !== shownChildren.length) {
warnOnce(
'TabList: `renderTabs` must render exactly one visible tab per received element ' +
'(preserve keys, no extra wrapper DOM around a tab). A wrapper element breaks ' +
'the `contentOverflow="collapse"` measurement.',
);
}
}, [renderTabs, shownChildren.length]);
```
</issue_to_address>
### Comment 2
<location path="src/components/tabs/__stories__/TabList.stories.tsx" line_range="215-219" />
<code_context>
+ );
+ return (
+ <React.Fragment>
+ {shown.map((tab, index) => (
+ <Draggable
+ key={tab.key ?? index}
+ draggableId={String((tab.props as TabProps).value)}
+ index={index}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `index` as a fallback key can cause unnecessary remounting when tabs are reordered.
In a sortable list, index-based keys will change as items move, causing components to remount and lose internal state (including focus). Since you already have a stable `value` for `draggableId`, prefer a value-based key such as `key={tab.key ?? (tab.props as TabProps).value}` so keys stay consistent across reorders.
```suggestion
{shown.map((tab, index) => (
<Draggable
key={tab.key ?? (tab.props as TabProps).value}
draggableId={String((tab.props as TabProps).value)}
index={index}
```
</issue_to_address>
### Comment 3
<location path="src/components/tabs/__stories__/TabList.stories.tsx" line_range="164" />
<code_context>
+ * `contentOverflow`) and hands them here to be wrapped in the DnD library of your choice.
+ * The collapsed tabs are still rendered as plain `<Tab>` in the overflow menu.
+ */
+const DragAndDropRender = (args: TabListProps) => {
+ const [tabs, setTabs] = React.useState<TabProps[]>(() => getTabsMock({}));
+ const [value, setValue] = React.useState(tabs[0].value);
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the drag-and-drop logic, tab-cloning helpers, and placeholder styling into small helper functions to make the `DragAndDropRender` story component easier to read and maintain.
You can keep the new functionality but reduce “god component” feel by extracting a few focused helpers. That will make the story much easier to scan.
**1. Extract DnD reorder logic**
Move the `onDragEnd` logic into a small pure helper so `DragAndDropRender` only wires it:
```ts
function reorderTabsByDragResult(
tabs: TabProps[],
shownValues: string[],
{destination, draggableId}: DropResult,
): TabProps[] {
if (!destination) return tabs;
const toValue = shownValues[destination.index];
const from = tabs.findIndex((tab) => tab.value === draggableId);
const to = tabs.findIndex((tab) => tab.value === toValue);
if (from === -1 || to === -1) return tabs;
return reorder(tabs, from, to);
}
```
Then in `DragAndDropRender`:
```ts
const onDragEnd = (result: DropResult) => {
setTabs((prev) => reorderTabsByDragResult(prev, shownValuesRef.current, result));
};
```
**2. Extract tab value / cloning helpers**
Avoid repeated casts and inline `cloneElement` noise:
```ts
function getTabValue(tab: React.ReactElement<TabProps>): string {
return String((tab.props as TabProps).value);
}
function cloneTabWithDndProps(
tab: React.ReactElement<TabProps>,
provided: DraggableProvided,
): React.ReactElement<TabProps> {
return React.cloneElement(tab, {
ref: provided.innerRef,
...provided.draggableProps,
...provided.dragHandleProps,
} as Partial<TabProps> & React.Attributes);
}
```
Usage in `renderTabs`:
```tsx
renderTabs={(shown) => {
shownValuesRef.current = shown.map((tab) => (tab.props as TabProps).value);
return (
<>
{shown.map((tab, index) => (
<Draggable
key={tab.key ?? index}
draggableId={getTabValue(tab as React.ReactElement<TabProps>)}
index={index}
disableInteractiveElementBlocking
>
{(draggableProvided) => cloneTabWithDndProps(
tab as React.ReactElement<TabProps>,
draggableProvided,
)}
</Draggable>
))}
{droppableProvided.placeholder}
</>
);
}}
```
**3. Extract placeholder styling**
Move the inline `<style>` into a small helper or constant so the JSX reads as layout only:
```ts
const dndPlaceholderReset = `
.tabs-dnd-story [data-rfd-placeholder-context-id] {
-webkit-appearance: none;
appearance: none;
border: 0;
background: transparent;
padding: 0;
}
`;
function DndPlaceholderStyle() {
return <style>{dndPlaceholderReset}</style>;
}
```
Then:
```tsx
<div className="tabs-dnd-story">
<DndPlaceholderStyle />
<DragDropContext onDragEnd={onDragEnd}>
{/* ... */}
</DragDropContext>
</div>
```
These small extractions keep all behavior the same but make `DragAndDropRender` mostly about “what is rendered” instead of “how it works,” reducing perceived complexity.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+60
to
+78
| // Dev-only validation of the `renderTabs` result: `warnOnce` already gates on | ||
| // NODE_ENV and dedupes. We check the actual DOM (the element tree can't be validated | ||
| // reliably — dnd libraries hide the `<Tab>` behind a render function). | ||
| React.useEffect(() => { | ||
| if (!renderTabs || !listRef.current) { | ||
| return; | ||
| } | ||
| const tabClassName = bTab(); | ||
| const shownCount = Array.from(listRef.current.children).filter((child) => | ||
| child.classList.contains(tabClassName), | ||
| ).length; | ||
| if (shownCount !== shownChildren.length) { | ||
| warnOnce( | ||
| 'TabList: `renderTabs` must render exactly one visible tab per received element ' + | ||
| '(preserve keys, no extra wrapper DOM around a tab). A wrapper element breaks ' + | ||
| 'the `contentOverflow="collapse"` measurement.', | ||
| ); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
suggestion (performance): The validation effect is missing a dependency array and will run after every render.
Since this dev-only effect only depends on renderTabs, shownChildren.length, and listRef.current, it’s doing extra work by running after every render. Consider adding a dependency array such as [renderTabs, shownChildren.length] and checking listRef.current inside the effect, so it only runs when the relevant inputs change.
Suggested change
| // Dev-only validation of the `renderTabs` result: `warnOnce` already gates on | |
| // NODE_ENV and dedupes. We check the actual DOM (the element tree can't be validated | |
| // reliably — dnd libraries hide the `<Tab>` behind a render function). | |
| React.useEffect(() => { | |
| if (!renderTabs || !listRef.current) { | |
| return; | |
| } | |
| const tabClassName = bTab(); | |
| const shownCount = Array.from(listRef.current.children).filter((child) => | |
| child.classList.contains(tabClassName), | |
| ).length; | |
| if (shownCount !== shownChildren.length) { | |
| warnOnce( | |
| 'TabList: `renderTabs` must render exactly one visible tab per received element ' + | |
| '(preserve keys, no extra wrapper DOM around a tab). A wrapper element breaks ' + | |
| 'the `contentOverflow="collapse"` measurement.', | |
| ); | |
| } | |
| }); | |
| // Dev-only validation of the `renderTabs` result: `warnOnce` already gates on | |
| // NODE_ENV and dedupes. We check the actual DOM (the element tree can't be validated | |
| // reliably — dnd libraries hide the `<Tab>` behind a render function). | |
| React.useEffect(() => { | |
| if (!renderTabs || !listRef.current) { | |
| return; | |
| } | |
| const tabClassName = bTab(); | |
| const shownCount = Array.from(listRef.current.children).filter((child) => | |
| child.classList.contains(tabClassName), | |
| ).length; | |
| if (shownCount !== shownChildren.length) { | |
| warnOnce( | |
| 'TabList: `renderTabs` must render exactly one visible tab per received element ' + | |
| '(preserve keys, no extra wrapper DOM around a tab). A wrapper element breaks ' + | |
| 'the `contentOverflow="collapse"` measurement.', | |
| ); | |
| } | |
| }, [renderTabs, shownChildren.length]); |
Comment on lines
+215
to
+219
| {shown.map((tab, index) => ( | ||
| <Draggable | ||
| key={tab.key ?? index} | ||
| draggableId={String((tab.props as TabProps).value)} | ||
| index={index} |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Using index as a fallback key can cause unnecessary remounting when tabs are reordered.
In a sortable list, index-based keys will change as items move, causing components to remount and lose internal state (including focus). Since you already have a stable value for draggableId, prefer a value-based key such as key={tab.key ?? (tab.props as TabProps).value} so keys stay consistent across reorders.
Suggested change
| {shown.map((tab, index) => ( | |
| <Draggable | |
| key={tab.key ?? index} | |
| draggableId={String((tab.props as TabProps).value)} | |
| index={index} | |
| {shown.map((tab, index) => ( | |
| <Draggable | |
| key={tab.key ?? (tab.props as TabProps).value} | |
| draggableId={String((tab.props as TabProps).value)} | |
| index={index} |
| * `contentOverflow`) and hands them here to be wrapped in the DnD library of your choice. | ||
| * The collapsed tabs are still rendered as plain `<Tab>` in the overflow menu. | ||
| */ | ||
| const DragAndDropRender = (args: TabListProps) => { |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider extracting the drag-and-drop logic, tab-cloning helpers, and placeholder styling into small helper functions to make the DragAndDropRender story component easier to read and maintain.
You can keep the new functionality but reduce “god component” feel by extracting a few focused helpers. That will make the story much easier to scan.
1. Extract DnD reorder logic
Move the onDragEnd logic into a small pure helper so DragAndDropRender only wires it:
function reorderTabsByDragResult(
tabs: TabProps[],
shownValues: string[],
{destination, draggableId}: DropResult,
): TabProps[] {
if (!destination) return tabs;
const toValue = shownValues[destination.index];
const from = tabs.findIndex((tab) => tab.value === draggableId);
const to = tabs.findIndex((tab) => tab.value === toValue);
if (from === -1 || to === -1) return tabs;
return reorder(tabs, from, to);
}Then in DragAndDropRender:
const onDragEnd = (result: DropResult) => {
setTabs((prev) => reorderTabsByDragResult(prev, shownValuesRef.current, result));
};2. Extract tab value / cloning helpers
Avoid repeated casts and inline cloneElement noise:
function getTabValue(tab: React.ReactElement<TabProps>): string {
return String((tab.props as TabProps).value);
}
function cloneTabWithDndProps(
tab: React.ReactElement<TabProps>,
provided: DraggableProvided,
): React.ReactElement<TabProps> {
return React.cloneElement(tab, {
ref: provided.innerRef,
...provided.draggableProps,
...provided.dragHandleProps,
} as Partial<TabProps> & React.Attributes);
}Usage in renderTabs:
renderTabs={(shown) => {
shownValuesRef.current = shown.map((tab) => (tab.props as TabProps).value);
return (
<>
{shown.map((tab, index) => (
<Draggable
key={tab.key ?? index}
draggableId={getTabValue(tab as React.ReactElement<TabProps>)}
index={index}
disableInteractiveElementBlocking
>
{(draggableProvided) => cloneTabWithDndProps(
tab as React.ReactElement<TabProps>,
draggableProvided,
)}
</Draggable>
))}
{droppableProvided.placeholder}
</>
);
}}3. Extract placeholder styling
Move the inline <style> into a small helper or constant so the JSX reads as layout only:
const dndPlaceholderReset = `
.tabs-dnd-story [data-rfd-placeholder-context-id] {
-webkit-appearance: none;
appearance: none;
border: 0;
background: transparent;
padding: 0;
}
`;
function DndPlaceholderStyle() {
return <style>{dndPlaceholderReset}</style>;
}Then:
<div className="tabs-dnd-story">
<DndPlaceholderStyle />
<DragDropContext onDragEnd={onDragEnd}>
{/* ... */}
</DragDropContext>
</div>These small extractions keep all behavior the same but make DragAndDropRender mostly about “what is rendered” instead of “how it works,” reducing perceived complexity.
Contributor
|
Preview is ready. |
Contributor
|
🎭 Component Tests Report is ready. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a
renderTabsprop toTabList— a small, drag-and-drop-library-agnostic seam that lets consumers wrap the visible tabs in any DnD library, including withcontentOverflow="collapse".Why
Wrapping tabs in a DnD library externally breaks with
contentOverflow="collapse": the collapse split runs on the raw<Tab>children, so<Draggable>wrappers (which hide the<Tab>behind a render function) plus the extra dnd placeholder node get miscounted — phantom "More", wrong collapsed count, and an empty overflow menu.Approach
TabListkeeps computing the shown/collapsed split from the raw<Tab>children and hands only the already-computed visible tabs torenderTabs. Collapsed tabs stay plain<Tab>in the overflow menu. No DnD-library types leak into the component.contentOverflowmodes; the collapse More menu keeps working.renderTabs.Prior art: this mirrors Ant Design's
renderTabBarescape hatch; no major library bakes DnD into tabs.Stories
DragAndDrop/DragAndDropCollapse(with@hello-pangea/dnd) demonstrate it.Tests and README docs will follow once the approach is agreed.
Summary by Sourcery
Add a renderTabs escape hatch to TabList to support library-agnostic wrapping of visible tabs, including under collapsed overflow, with a dev-time contract check and Storybook demos using drag-and-drop.
New Features:
Enhancements:
Tests: