Skip to content
Prev Previous commit
Next Next commit
fix remaining broken tests
  • Loading branch information
snowystinger committed Feb 7, 2024
commit ca1545a66450f256aa217ad11870d64b32dccd86
4 changes: 1 addition & 3 deletions packages/@react-spectrum/listbox/test/ListBox.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -889,9 +889,8 @@ describe('ListBox', function () {
});

describe('When focused item is removed', function () {
it.only('should move focus to the next item that is not disabled', () => {
it('should move focus to the next item that is not disabled', () => {
let tree = render(<Provider theme={theme}><FocusExample /></Provider>);
console.log('done render')
act(() => jest.runAllTimers());
let listbox = tree.getByRole('listbox');
let options = within(listbox).getAllByRole('option');
Expand All @@ -911,7 +910,6 @@ describe('ListBox', function () {
expect(document.activeElement).toBe(confirmationDialog);
let confirmationDialogButton = within(confirmationDialog).getByRole('button');
expect(confirmationDialogButton).toBeInTheDocument();
console.log('deleting')
triggerPress(confirmationDialogButton);
act(() => jest.runAllTimers());
options = within(listbox).getAllByRole('option');
Expand Down
26 changes: 13 additions & 13 deletions packages/@react-stately/data/src/useTreeData.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ export interface TreeData<T extends object> {
*/
update(key: Key, newValue: T): void
}
let creation = 0;

/**
* Manages state for an immutable tree data structure, and provides convenience methods to
* update the data over time.
Expand All @@ -141,7 +141,6 @@ export function useTreeData<T extends object>(options: TreeOptions<T>): TreeData
return {
items: initialItems.map(item => {
let node: TreeNode<T> = {
creation: creation++,
key: getKey(item),
parentKey: parentKey,
value: item,
Expand All @@ -166,16 +165,15 @@ export function useTreeData<T extends object>(options: TreeOptions<T>): TreeData
// Create a new node. If null, then delete the node, otherwise replace.
let newNode = update(node);
if (newNode == null) {
deleteNode(node);
deleteNode(node, map);
} else {
addNode(newNode);
addNode(newNode, map);
}

// Walk up the tree and update each parent to refer to the new children.
while (node.parentKey) {
let nextParent = map.get(node.parentKey);
let copy: TreeNode<T> = {
creation: creation++,
key: nextParent.key,
parentKey: nextParent.parentKey,
value: nextParent.value,
Expand Down Expand Up @@ -218,17 +216,17 @@ export function useTreeData<T extends object>(options: TreeOptions<T>): TreeData
};
}

function addNode(node: TreeNode<T>) {
function addNode(node: TreeNode<T>, map) {
map.set(node.key, node);
for (let child of node.children) {
addNode(child);
addNode(child, map);
}
}

function deleteNode(node: TreeNode<T>) {
function deleteNode(node: TreeNode<T>, map) {
map.delete(node.key);
for (let child of node.children) {
deleteNode(child);
deleteNode(child, map);
}
}

Expand All @@ -241,7 +239,7 @@ export function useTreeData<T extends object>(options: TreeOptions<T>): TreeData
},
insert(parentKey: Key | null, index: number, ...values: T[]) {
setItems(({items, mappy: originalMap}) => {
let {items: nodes, mappy: newMap} = buildTree(values, parentKey);
let {items: nodes, mappy: newMap} = buildTree(values, parentKey, originalMap);
Copy link
Member

Choose a reason for hiding this comment

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

won't this still result in originalMap being mutated? buildTree only creates a new map if acc is not provided, otherwise it reuses it.

Copy link
Member

Choose a reason for hiding this comment

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

not quite sure I follow here. We are passing originalMap here to buildTree to build the newMap for the parentKey === null case. If we didn't create pass it, buildTree would create a new map that would then only have the new nodes that are being inserted here instead of the full modified tree map since values is just the new values being inserted

modifying the originalMap here shouldn't matter right since we are now storing the map in state and accessing the previous map inside the setState call?

Copy link
Member

Choose a reason for hiding this comment

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

yeah here https://github.com/adobe/react-spectrum/pull/5835/files#diff-c1a25c31946ec151047ebc265b0f5cf0971d990a3407c0dd66b7beecb1be4b24R151 we mutate the map that gets passed into buildTree, thereby mutating the map stored in state.


// If parentKey is null, insert into the root.
if (parentKey == null) {
Expand Down Expand Up @@ -308,16 +306,18 @@ export function useTreeData<T extends object>(options: TreeOptions<T>): TreeData
remove(...keys: Key[]) {
let newItems = items;
let prevMap = map;
let tree;
for (let key of keys) {
let tree = updateTree(newItems, key, () => null, prevMap);
tree = updateTree(newItems, key, () => null, prevMap);
prevMap = tree.mappy;
newItems = tree.items;
}

setItems(tree);

let selection = new Set(selectedKeys);
for (let key of selectedKeys) {
if (!map.has(key)) {
if (!tree.mappy.has(key)) {
selection.delete(key);
}
}
Expand Down Expand Up @@ -373,7 +373,7 @@ export function useTreeData<T extends object>(options: TreeOptions<T>): TreeData
children: null
};

let tree = buildTree(getChildren(newValue), node.key);
let tree = buildTree(getChildren(newValue), node.key, originalMap);
node.children = tree.items;
return node;
}, originalMap));
Expand Down