Conversation
Contributor
Reviewer's GuideIntroduce the headless List core with a new useListState hook, its structural index reconciliation utilities, and accompanying types, tests, and README documentation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Preview is ready. |
Contributor
|
🎭 Component Tests Report is ready. |
korvin89
force-pushed
the
dataui-3901-uselist-state
branch
from
July 3, 2026 18:51
4047c3b to
01ae96f
Compare
korvin89
marked this pull request as ready for review
July 3, 2026 19:42
![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:
- In
core/index.tsyou re-exportListStateandUseListStatePropsfrom./useListState, but those types are defined inuseListState/types.tsand not re-exported fromuseListState.ts, so this barrel export will fail; update the export to source them from./useListState/types(or re-export them fromuseListState.ts).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `core/index.ts` you re-export `ListState` and `UseListStateProps` from `./useListState`, but those types are defined in `useListState/types.ts` and not re-exported from `useListState.ts`, so this barrel export will fail; update the export to source them from `./useListState/types` (or re-export them from `useListState.ts`).
## Individual Comments
### Comment 1
<location path="src/components/lab/List/core/useListState/listStateIndex.ts" line_range="126" />
<code_context>
+ };
+
+ const reconcileNode = (item: T, level: number, parentId: string | undefined): string => {
+ const id = getItemId(item);
+
+ // Unchanged reference at the same position ⇒ the whole subtree is intact; copy it over.
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider adding dev-time validation that `getItemId` returns a string to surface misconfigurations earlier.
The code currently trusts `getItemId` to always return a string, but if a consumer returns a non-string (e.g. number or `null`), the maps will still accept it and cause subtle, hard-to-debug behavior. Adding a `NODE_ENV !== 'production'` check that asserts `typeof id === 'string'` and logs a clear error (like the duplicate-id check) would catch these misconfigurations early without impacting production performance.
Suggested implementation:
```typescript
const reconcileNode = (item: T, level: number, parentId: string | undefined): string => {
const id = getItemId(item);
if (process.env.NODE_ENV !== 'production' && typeof id !== 'string') {
// Dev-time validation: surface misconfigured getItemId implementations early.
// Using console.error to match duplicate-id style diagnostics without affecting production.
console.error(
[
'MUI: ListStateIndex#getItemId should return a string.',
`Received: ${String(id)} (type: ${typeof id}).`,
'This can cause subtle bugs in list state reconciliation. ',
'Ensure getItemId(item) always returns a stable string identifier.',
].join(' '),
);
}
// Unchanged reference at the same position ⇒ the whole subtree is intact; copy it over.
if (
previous &&
previous.itemById.get(id) === item &&
previous.levelById.get(id) === level &&
previous.parentById.get(id) === parentId
) {
copyReusedSubtree(id);
return id;
}
```
1. If this file already has a shared dev-only logging helper (e.g. a `warn()` / `error()` utility or a MUI-specific diagnostic function), consider replacing the `console.error` call with that helper for consistency.
2. If there are existing tests around duplicate-id diagnostics, you may want to add a similar test case asserting that a non-string id triggers this warning in development mode and is silent in production mode.
</issue_to_address>
### Comment 2
<location path="src/components/lab/List/core/useListState/listStateIndex.ts" line_range="146" />
<code_context>
+ disabledById.set(id, getItemDisabled(item));
+ childrenStateById.set(id, getItemChildrenState(item));
+
+ const childItems = getItemChildren(item);
+ let childIds: string[] | undefined;
+ if (childItems === undefined) {
</code_context>
<issue_to_address>
**suggestion:** Handle or guard against non-array values from `getItemChildren` to avoid hard-to-debug runtime errors.
This path assumes any truthy `getItemChildren` result is a dense array, since it uses `.length` and index access. If a consumer returns a NodeList, Set, sparse array, or other non-array truthy value, you can get subtle runtime failures. Consider a dev-only `Array.isArray(childItems)` assertion before using `.length` and indices to catch misconfigurations early without affecting production performance.
Suggested implementation:
```typescript
childrenStateById.set(id, getItemChildrenState(item));
const childItems = getItemChildren(item);
if (process.env.NODE_ENV !== 'production' && childItems !== undefined && !Array.isArray(childItems)) {
throw new Error(
'getItemChildren must return an array or undefined. ' +
`Received: ${Object.prototype.toString.call(childItems)} for item with id "${id}".`,
);
}
let childIds: string[] | undefined;
if (childItems === undefined) {
childIds = undefined;
} else {
childIds = new Array<string>(childItems.length);
for (let i = 0; i < childItems.length; i++) {
childIds[i] = reconcileNode(childItems[i], level + 1, id);
}
}
childrenIdsById.set(id, childIds);
```
If this codebase already uses a specific dev flag or assertion utility (e.g. `__DEV__`, `warning`, or `invariant`), you should replace the `process.env.NODE_ENV !== 'production'` check and `throw new Error(...)` with that existing convention to keep the implementation consistent.
</issue_to_address>
### Comment 3
<location path="src/components/lab/List/core/useListState/listStateIndex.ts" line_range="14" />
<code_context>
+ * read-only source to reuse unchanged subtrees, which keeps *getter* work proportional to the
+ * change; the map allocation itself is O(total nodes) of cheap `Map.set` calls.
+ */
+export interface ListStateIndex<T> {
+ /** The `items` reference this index was reconciled from — used to short-circuit rebuilds */
+ sourceItems: T[];
</code_context>
<issue_to_address>
**issue (complexity):** Consider collapsing the parallel maps into a single node map and extracting duplicate-id tracking into a helper to simplify and decouple the list index logic.
You can keep the subtree-reuse behavior but significantly reduce the cross-map coupling by introducing a single `nodeById` map and pushing duplicate tracking into a thin wrapper.
### 1. Replace parallel maps with a single `nodeById`
Instead of manually keeping six maps in sync, store a unified node record. This makes `copyReusedSubtree` and `reconcileNode` much easier to reason about.
```ts
interface NodeInfo<T> {
item: T;
level: number;
parentId?: string;
childrenIds?: string[];
childrenState?: ListChildrenState;
disabled: boolean;
}
interface ListStateIndex<T> {
sourceItems: T[];
rootIds: string[];
nodeById: Map<string, NodeInfo<T>>;
}
```
Then `copyReusedSubtree` and `reconcileNode` only deal with one map:
```ts
const nodeById = new Map<string, NodeInfo<T>>();
const copyReusedSubtree = (id: string) => {
const prevNode = (previous as ListStateIndex<T>).nodeById.get(id);
if (!prevNode) return;
markSeen(id);
nodeById.set(id, prevNode); // shallow copy is fine if NodeInfo is treated as immutable
const childIds = prevNode.childrenIds;
if (childIds) {
for (let i = 0; i < childIds.length; i++) {
copyReusedSubtree(childIds[i]);
}
}
};
const reconcileNode = (item: T, level: number, parentId: string | undefined): string => {
const id = getItemId(item);
if (
previous &&
previous.nodeById.get(id)?.item === item &&
previous.nodeById.get(id)?.level === level &&
previous.nodeById.get(id)?.parentId === parentId
) {
copyReusedSubtree(id);
return id;
}
markSeen(id);
const childItems = getItemChildren(item);
const childIds =
childItems === undefined
? undefined
: childItems.map((child) => reconcileNode(child, level + 1, id));
nodeById.set(id, {
item,
level,
parentId,
childrenIds: childIds,
childrenState: getItemChildrenState(item),
disabled: getItemDisabled(item),
});
return id;
};
```
`computeVisibleIds` then only reads from `nodeById`:
```ts
export function computeVisibleIds<T>(
index: ListStateIndex<T>,
expandedIds: ReadonlySet<string>,
): string[] {
const result: string[] = [];
const walk = (ids: string[]) => {
for (let i = 0; i < ids.length; i++) {
const id = ids[i];
result.push(id);
const node = index.nodeById.get(id);
const childIds = node?.childrenIds;
if (childIds && childIds.length > 0 && expandedIds.has(id)) {
walk(childIds);
}
}
};
walk(index.rootIds);
return result;
}
```
This preserves all existing behavior (including subtree reuse) but removes the need to keep multiple maps perfectly aligned.
### 2. Isolate duplicate-id tracking from core traversal
You can keep the same validation but keep the traversal logic cleaner by wrapping `markSeen` creation:
```ts
function createMarkSeen() {
if (process.env.NODE_ENV === 'production') {
return (_id: string) => {};
}
const seen = new Set<string>();
return (id: string) => {
if (seen.has(id)) {
console.error(
`[useListState] Duplicate item id "${id}". Item ids must be unique — pass getItemId to derive stable, unique ids.`,
);
}
seen.add(id);
};
}
// in reconcileListStateIndex:
const markSeen = createMarkSeen();
```
This keeps debugging concerns out of the core reconciliation code paths, while retaining the same validation behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Per Sourcery review on the PR: replace the six parallel per-field maps (itemById / levelById / parentById / childrenIdsById / childrenStateById / disabledById) with one `nodeById` map of node records. Reduces cross-map coupling and makes subtree reuse cheaper (an unchanged subtree reuses its immutable records by reference). Behavior unchanged; tests green.
Contributor
Author
|
🤖 AI generated Re: Sourcery's overall review note:
False positive: there is no |
korvin89
marked this pull request as draft
July 3, 2026 21:10
Add a `getItemType` getter (`'item' | 'section'`, default `'item'`) and a `getItemType(id)` accessor on `ListState`, plus a `ListItemType` export. The upcoming selection hook needs a role-independent way to tell a selectable row from a non-interactive group label. A "leaf is an option, node with children is a section header" rule holds only for `listbox` and misclassifies a `tree` folder — and the selection hook has no role and runs before the behavior layer, so it cannot consult one. Marking the role explicitly in data (as react-aria collections do) keeps selection role-agnostic and, unlike the inference rule, lets a node with children also be a selectable group in a flat list — disclosure and selectability stay independent axes. Type is purely structural: it governs selection and navigation, not disclosure. A `section` still holds children and expands like any node; keeping a section always-open is a controlled-expansion concern.
korvin89
marked this pull request as ready for review
July 6, 2026 13:44
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/components/lab/List/core/useListState/__tests__/useListState.test.tsx" line_range="22-31" />
<code_context>
+ describe('normalization', () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for an empty items array to assert baseline behavior.
There’s no test explicitly covering the `items: []` case. Please add something like `useListState({ items: [] })` that asserts `visibleIds` is `[]`, `getItemById` returns `undefined` for any id, and no errors/logs occur, to lock in the empty-list behavior and guard against regressions in index initialization.
</issue_to_address>
### Comment 2
<location path="src/components/lab/List/core/useListState/__tests__/useListState.test.tsx" line_range="134-143" />
<code_context>
+ describe('visibleIds & expansion', () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for expanding/collapsing an id that does not exist in the index.
Current expansion tests only cover valid ids. Since `setExpanded` operates on the expanded id set, calling `setExpanded('missing', true)` should neither throw nor change `visibleIds` when no node exists for that id. Please add a test to confirm unknown ids are handled safely (no crash, no `visibleIds` changes), to improve robustness against stale ids from callers.
Suggested implementation:
```typescript
describe('visibleIds & expansion', () => {
it('shows only roots when collapsed', () => {
const {result} = renderHook(() => useListState({items: tree}));
expect(result.current.visibleIds).toEqual(['root', 'sibling']);
});
it('expands and collapses (uncontrolled)', () => {
const {result} = renderHook(() => useListState({items: tree, defaultExpandedIds: []}));
act(() => result.current.setExpanded('root', true));
expect(result.current.visibleIds).toEqual(['root', 'c1', 'c2', 'sibling']);
});
it('ignores expansion/collapse requests for unknown ids', () => {
const {result} = renderHook(() => useListState({items: tree, defaultExpandedIds: []}));
const initialVisibleIds = result.current.visibleIds;
act(() => {
result.current.setExpanded('missing', true);
});
expect(result.current.visibleIds).toEqual(initialVisibleIds);
act(() => {
result.current.setExpanded('missing', false);
});
expect(result.current.visibleIds).toEqual(initialVisibleIds);
```
If the actual test structure around `visibleIds & expansion` differs from the shown snippet (e.g., more expectations inside the uncontrolled expansion test or different indentation), adjust the SEARCH block to match the exact code segment and reapply the same REPLACE content. Also ensure that `renderHook`, `act`, and `useListState` are already imported at the top of the file as in the existing tests; no new imports are required for this change.
</issue_to_address>
### Comment 3
<location path="src/components/lab/List/core/useListState/listStateIndex.ts" line_range="97" />
<code_context>
+
+ const nodeById = new Map<string, ListNode<T>>();
+
+ const seen = process.env.NODE_ENV === 'production' ? null : new Set<string>();
+ const markSeen = (id: string) => {
+ if (process.env.NODE_ENV !== 'production' && seen) {
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the duplicate-id checking into a factory that returns a no-op in production so `markSeen` is simple and free of environment checks in the traversal code.
You can simplify the duplicate-id tracking and remove the environment-dependent branching from the hot path by encapsulating it in a helper, while keeping the behavior (dev-only warnings) intact.
Right now every `markSeen` call pays the cost of checking `NODE_ENV` and whether `seen` is defined:
```ts
const seen = process.env.NODE_ENV === 'production' ? null : new Set<string>();
const markSeen = (id: string) => {
if (process.env.NODE_ENV !== 'production' && seen) {
if (seen.has(id)) {
console.error(/* ... */);
}
seen.add(id);
}
};
```
Instead, define `markSeen` once based on `NODE_ENV`, and make the production version a no-op. This removes branching and makes the reconciliation traversal easier to follow:
```ts
const createDuplicateChecker = () => {
if (process.env.NODE_ENV === 'production') {
// No-op in production
return (_id: string) => {};
}
const seen = new Set<string>();
return (id: string) => {
if (seen.has(id)) {
console.error(
`[useListState] Duplicate item id "${id}". Item ids must be unique — pass getItemId to derive stable, unique ids.`,
);
}
seen.add(id);
};
};
const markSeen = createDuplicateChecker();
```
Then the reconciliation logic can use `markSeen(id)` without any extra guards:
```ts
const copyReusedSubtree = (id: string) => {
const node = previous?.nodeById.get(id);
if (!node) {
return;
}
markSeen(id);
nodeById.set(id, node);
if (node.childrenIds) {
for (let i = 0; i < node.childrenIds.length; i++) {
copyReusedSubtree(node.childrenIds[i]);
}
}
};
const reconcileNode = (item: T, level: number, parentId: string | undefined): string => {
const id = getItemId(item);
const prevNode = previous?.nodeById.get(id);
if (prevNode && prevNode.item === item && prevNode.level === level && prevNode.parentId === parentId) {
copyReusedSubtree(id);
return id;
}
markSeen(id);
// ...
};
```
This keeps all functionality (dev-only duplicate warnings, subtree reuse, immutability) but reduces the cognitive load in the core traversal by separating environment concerns from reconciliation.
</issue_to_address>
### Comment 4
<location path="src/components/lab/List/core/useListState/useListState.ts" line_range="41" />
<code_context>
+
+ const expandedSet = React.useMemo(() => new Set(currentExpandedIds), [currentExpandedIds]);
+
+ // Refs keep `setExpanded` referentially stable while reading the latest expansion and setter.
+ const expandedIdsRef = React.useRef(currentExpandedIds);
+ expandedIdsRef.current = currentExpandedIds;
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the expansion handling by removing ref-backed state and collapsing memoized accessors into a single memoized return object.
The main complexity comes from the ref-backed `setExpanded` and the layered `useMemo` usage. You can keep all functionality while simplifying by:
1. **Drop the refs and accept changing `setExpanded` identity**
You can rely on React’s normal handler identity changes instead of ref indirection. This removes three refs and makes data flow clearer:
```ts
const [currentExpandedIds, setExpandedIds] = useControlledState(
expandedIds,
defaultExpandedIds ?? EMPTY_IDS,
onExpandedUpdate,
);
const expandedSet = React.useMemo(
() => new Set(currentExpandedIds),
[currentExpandedIds],
);
const setExpanded = React.useCallback(
(id: string, expanded: boolean) => {
if (expandedSet.has(id) === expanded) {
return;
}
const next = expanded
? [...currentExpandedIds, id]
: currentExpandedIds.filter((expandedId) => expandedId !== id);
setExpandedIds(next);
},
[expandedSet, currentExpandedIds, setExpandedIds],
);
```
This keeps behavior identical (controlled/uncontrolled handling + expansion logic), but removes `expandedIdsRef`, `expandedSetRef`, and `setExpandedIdsRef`.
2. **Inline accessors into a single `useMemo` for the returned state**
You can skip the intermediate `accessors` memo and assemble the full `ListState` in one place:
```ts
const visibleIds = React.useMemo(
() => computeVisibleIds(index, expandedSet),
[index, expandedSet],
);
return React.useMemo<ListState<T>>(
() => ({
getItemById: (id: string) => index.nodeById.get(id)?.item,
getLevel: (id: string) => index.nodeById.get(id)?.level ?? 0,
getParentId: (id: string) => index.nodeById.get(id)?.parentId,
getChildrenIds: (id: string) => index.nodeById.get(id)?.childrenIds,
getChildrenState: (id: string) => index.nodeById.get(id)?.childrenState,
isDisabled: (id: string) => index.nodeById.get(id)?.disabled ?? false,
getItemType: (id: string) => index.nodeById.get(id)?.type ?? 'item',
visibleIds,
isExpanded: (id) => expandedSet.has(id),
setExpanded,
}),
[index, visibleIds, expandedSet, setExpanded],
);
```
This keeps memoization and all existing getters but removes one level of indirection, making the return shape easier to follow.
These two focused changes reduce indirection and memo layering without altering the feature set or the controlled/uncontrolled behavior.
</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
+22
to
+31
| describe('normalization', () => { | ||
| it('handles primitive string items with zero getters', () => { | ||
| const {result} = renderHook(() => useListState({items: ['Apple', 'Pear', 'Plum']})); | ||
|
|
||
| expect(result.current.visibleIds).toEqual(['Apple', 'Pear', 'Plum']); | ||
| expect(result.current.getItemById('Apple')).toBe('Apple'); | ||
| expect(result.current.getLevel('Apple')).toBe(0); | ||
| expect(result.current.getParentId('Apple')).toBeUndefined(); | ||
| expect(result.current.getChildrenIds('Apple')).toBeUndefined(); | ||
| expect(result.current.isDisabled('Apple')).toBe(false); |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a test for an empty items array to assert baseline behavior.
There’s no test explicitly covering the items: [] case. Please add something like useListState({ items: [] }) that asserts visibleIds is [], getItemById returns undefined for any id, and no errors/logs occur, to lock in the empty-list behavior and guard against regressions in index initialization.
Comment on lines
+134
to
+143
| describe('visibleIds & expansion', () => { | ||
| it('shows only roots when collapsed', () => { | ||
| const {result} = renderHook(() => useListState({items: tree})); | ||
| expect(result.current.visibleIds).toEqual(['root', 'sibling']); | ||
| }); | ||
|
|
||
| it('expands and collapses (uncontrolled)', () => { | ||
| const {result} = renderHook(() => useListState({items: tree, defaultExpandedIds: []})); | ||
|
|
||
| act(() => result.current.setExpanded('root', true)); |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for expanding/collapsing an id that does not exist in the index.
Current expansion tests only cover valid ids. Since setExpanded operates on the expanded id set, calling setExpanded('missing', true) should neither throw nor change visibleIds when no node exists for that id. Please add a test to confirm unknown ids are handled safely (no crash, no visibleIds changes), to improve robustness against stale ids from callers.
Suggested implementation:
describe('visibleIds & expansion', () => {
it('shows only roots when collapsed', () => {
const {result} = renderHook(() => useListState({items: tree}));
expect(result.current.visibleIds).toEqual(['root', 'sibling']);
});
it('expands and collapses (uncontrolled)', () => {
const {result} = renderHook(() => useListState({items: tree, defaultExpandedIds: []}));
act(() => result.current.setExpanded('root', true));
expect(result.current.visibleIds).toEqual(['root', 'c1', 'c2', 'sibling']);
});
it('ignores expansion/collapse requests for unknown ids', () => {
const {result} = renderHook(() => useListState({items: tree, defaultExpandedIds: []}));
const initialVisibleIds = result.current.visibleIds;
act(() => {
result.current.setExpanded('missing', true);
});
expect(result.current.visibleIds).toEqual(initialVisibleIds);
act(() => {
result.current.setExpanded('missing', false);
});
expect(result.current.visibleIds).toEqual(initialVisibleIds);If the actual test structure around visibleIds & expansion differs from the shown snippet (e.g., more expectations inside the uncontrolled expansion test or different indentation), adjust the SEARCH block to match the exact code segment and reapply the same REPLACE content. Also ensure that renderHook, act, and useListState are already imported at the top of the file as in the existing tests; no new imports are required for this change.
|
|
||
| const nodeById = new Map<string, ListNode<T>>(); | ||
|
|
||
| const seen = process.env.NODE_ENV === 'production' ? null : new Set<string>(); |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider extracting the duplicate-id checking into a factory that returns a no-op in production so markSeen is simple and free of environment checks in the traversal code.
You can simplify the duplicate-id tracking and remove the environment-dependent branching from the hot path by encapsulating it in a helper, while keeping the behavior (dev-only warnings) intact.
Right now every markSeen call pays the cost of checking NODE_ENV and whether seen is defined:
const seen = process.env.NODE_ENV === 'production' ? null : new Set<string>();
const markSeen = (id: string) => {
if (process.env.NODE_ENV !== 'production' && seen) {
if (seen.has(id)) {
console.error(/* ... */);
}
seen.add(id);
}
};Instead, define markSeen once based on NODE_ENV, and make the production version a no-op. This removes branching and makes the reconciliation traversal easier to follow:
const createDuplicateChecker = () => {
if (process.env.NODE_ENV === 'production') {
// No-op in production
return (_id: string) => {};
}
const seen = new Set<string>();
return (id: string) => {
if (seen.has(id)) {
console.error(
`[useListState] Duplicate item id "${id}". Item ids must be unique — pass getItemId to derive stable, unique ids.`,
);
}
seen.add(id);
};
};
const markSeen = createDuplicateChecker();Then the reconciliation logic can use markSeen(id) without any extra guards:
const copyReusedSubtree = (id: string) => {
const node = previous?.nodeById.get(id);
if (!node) {
return;
}
markSeen(id);
nodeById.set(id, node);
if (node.childrenIds) {
for (let i = 0; i < node.childrenIds.length; i++) {
copyReusedSubtree(node.childrenIds[i]);
}
}
};
const reconcileNode = (item: T, level: number, parentId: string | undefined): string => {
const id = getItemId(item);
const prevNode = previous?.nodeById.get(id);
if (prevNode && prevNode.item === item && prevNode.level === level && prevNode.parentId === parentId) {
copyReusedSubtree(id);
return id;
}
markSeen(id);
// ...
};This keeps all functionality (dev-only duplicate warnings, subtree reuse, immutability) but reduces the cognitive load in the core traversal by separating environment concerns from reconciliation.
|
|
||
| const expandedSet = React.useMemo(() => new Set(currentExpandedIds), [currentExpandedIds]); | ||
|
|
||
| // Refs keep `setExpanded` referentially stable while reading the latest expansion and setter. |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider simplifying the expansion handling by removing ref-backed state and collapsing memoized accessors into a single memoized return object.
The main complexity comes from the ref-backed setExpanded and the layered useMemo usage. You can keep all functionality while simplifying by:
- Drop the refs and accept changing
setExpandedidentity
You can rely on React’s normal handler identity changes instead of ref indirection. This removes three refs and makes data flow clearer:
const [currentExpandedIds, setExpandedIds] = useControlledState(
expandedIds,
defaultExpandedIds ?? EMPTY_IDS,
onExpandedUpdate,
);
const expandedSet = React.useMemo(
() => new Set(currentExpandedIds),
[currentExpandedIds],
);
const setExpanded = React.useCallback(
(id: string, expanded: boolean) => {
if (expandedSet.has(id) === expanded) {
return;
}
const next = expanded
? [...currentExpandedIds, id]
: currentExpandedIds.filter((expandedId) => expandedId !== id);
setExpandedIds(next);
},
[expandedSet, currentExpandedIds, setExpandedIds],
);This keeps behavior identical (controlled/uncontrolled handling + expansion logic), but removes expandedIdsRef, expandedSetRef, and setExpandedIdsRef.
- Inline accessors into a single
useMemofor the returned state
You can skip the intermediate accessors memo and assemble the full ListState in one place:
const visibleIds = React.useMemo(
() => computeVisibleIds(index, expandedSet),
[index, expandedSet],
);
return React.useMemo<ListState<T>>(
() => ({
getItemById: (id: string) => index.nodeById.get(id)?.item,
getLevel: (id: string) => index.nodeById.get(id)?.level ?? 0,
getParentId: (id: string) => index.nodeById.get(id)?.parentId,
getChildrenIds: (id: string) => index.nodeById.get(id)?.childrenIds,
getChildrenState: (id: string) => index.nodeById.get(id)?.childrenState,
isDisabled: (id: string) => index.nodeById.get(id)?.disabled ?? false,
getItemType: (id: string) => index.nodeById.get(id)?.type ?? 'item',
visibleIds,
isExpanded: (id) => expandedSet.has(id),
setExpanded,
}),
[index, visibleIds, expandedSet, setExpanded],
);This keeps memoization and all existing getters but removes one level of indirection, making the return shape easier to follow.
These two focused changes reduce indirection and memo layering without altering the feature set or the controlled/uncontrolled behavior.
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.
🤖 AI generated
Adds
useListStateand the initiallab/List/coremodule — the headless state layer of a new List / TreeList / GridList family. Development happens inlab; nothing is wired into a public entry point yet.What's inside
useListState— normalizesitems(objects or plain strings) into a flat index (id / level / parent / children / childrenState / disabled / type) and derives the orderedvisibleIdsslice from expansion. Expansion is controlled viaexpandedIdsor uncontrolled viadefaultExpandedIds+onExpandedUpdate.itemsreference, reusing unchanged-by-reference subtrees so getters run only for changed nodes. It never mutates a previously produced index, so it is render-pure and safe under React concurrent mode.ListChildrenState— async-subtree state that disambiguatesgetItemChildrenreturningundefined: a tree leaf vs an unloaded (lazy) folder, plus a loaded empty folder ([]).getItemType(item/section) — structural role of a node, so a later layer can tell a selectable row from a non-interactive group label. See below.ListChildrenState,ListItemGetters,ListItemType,UseListStateProps,ListState.Structural item type (
item/section) — why & how it's usedAdds
getItemType?: (item) => 'item' | 'section'(default'item') and agetItemType(id)accessor onListState.Why it lives in the data, not inferred by role
The selection hook layered on top of this state (next PR) needs a role-independent way to tell a selectable row from a non-interactive group label — it has no
roleand is created before the behavior layer. Inferring "a node with children is a section header" only holds for alistbox; in atreea node with children is a selectable folder, so that rule can't live in a role-agnostic layer. Marking the role explicitly in the data (as react-aria collections do via node types) keeps selection role-agnostic.It also decouples two axes the inference rule conflated: disclosure (expand / collapse) and selectability. A
sectionstill holds children and expands like any other node; and a node with children can be marked'item'to act as a selectable group — even in a flat list. The type governs selection and keyboard navigation only, not disclosure; keeping a section always-open is a controlled-expansion concern (expandedIds).How the components will use it
Summary by Sourcery
Introduce a headless List core state layer with a new useListState hook that normalizes list/tree data into a structural index and derives visibleIds from expansion.
New Features:
Enhancements:
Tests: