From 190d1bc818d61207b069d5f6933642ad20c48c37 Mon Sep 17 00:00:00 2001
From: Junaid <62522218+junaidzm13@users.noreply.github.com>
Date: Mon, 22 Jan 2024 20:04:02 +0800
Subject: [PATCH] #1074 fix state updates issue in useFilterBar hook (#1135)
- fixes it by removing the use of useEffect to listen for changes
in filters and active indices and then call onApplyFilter. Now
I've made the calls to onApplyFilter explicit making it easier
to maintain and read.
- with this change I've also fixed a bug in filter deletion:
Cause: we used to delete an active filter's index from active
filter indices array without adjusting the rest of the indices.
Required adjustment: if a filter gets deleted from filters array
then the index of all the filters on the right are decremented by
1. But we weren't making this adjustment with active indices array.
- and have added component tests to prevent this filter deletion
bug from being re-introduced.
---
.gitignore | 6 +-
.../__component__/filter-bar/Filterbar.cy.tsx | 81 ++++++++++++
.../src/filter-bar/useFilterBar.ts | 72 +++-------
.../src/filter-bar/useFilterState.ts | 67 ++++++++++
.../vuu-filters/src/filter-bar/useFilters.ts | 125 ++++++++++--------
.../packages/vuu-filters/src/filter-utils.ts | 1 -
6 files changed, 236 insertions(+), 116 deletions(-)
create mode 100644 vuu-ui/packages/vuu-filters/src/filter-bar/useFilterState.ts
diff --git a/.gitignore b/.gitignore
index 786c22e94..64b4741e6 100644
--- a/.gitignore
+++ b/.gitignore
@@ -5,11 +5,7 @@
/.idea/
target/
/gc.log
-/toolbox/toolbox.iml
-/example/**/*.iml
-/vuu-ui/vuu-ui.iml
-/vuu/vuu.iml
-/vuu-parent.iml
+/**/*.iml
/vuu-book/src/.DS_Store
.DS_Store
diff --git a/vuu-ui/packages/vuu-filters/src/__tests__/__component__/filter-bar/Filterbar.cy.tsx b/vuu-ui/packages/vuu-filters/src/__tests__/__component__/filter-bar/Filterbar.cy.tsx
index 3cd43241f..328b02117 100644
--- a/vuu-ui/packages/vuu-filters/src/__tests__/__component__/filter-bar/Filterbar.cy.tsx
+++ b/vuu-ui/packages/vuu-filters/src/__tests__/__component__/filter-bar/Filterbar.cy.tsx
@@ -227,6 +227,87 @@ describe("The mouse user", () => {
});
});
});
+
+ describe("WHEN adds two filters", () => {
+ const filter1 = {
+ column: "currency",
+ op: "!=",
+ value: "USD",
+ name: 'currency != "USD"',
+ };
+ const filter2 = {
+ column: "currency",
+ op: "!=",
+ value: "CAD",
+ name: 'currency != "CAD"',
+ };
+
+ beforeEach(() => {
+ const onFiltersChanged = cy.stub().as("filtersChangedHandler");
+ const onApplyFilter = cy.stub().as("applyFilterHandler");
+ cy.mount(
+
+ );
+ cy.get(ADD_BUTTON).realClick();
+ clickListItems(filter1.column, filter1.op, filter1.value);
+ clickButton("APPLY AND SAVE");
+ waitUntilEditableLabelIsFocused(".vuuFilterPill");
+ cy.realPress("Enter");
+
+ cy.get(ADD_BUTTON).realClick();
+ clickListItems(filter2.column, filter2.op, filter2.value);
+ clickButton("APPLY AND SAVE");
+ waitUntilEditableLabelIsFocused(".vuuFilterPill");
+ cy.realPress("Enter");
+ });
+
+ it("THEN filtersChangedHandler & applyFilterHandler callbacks are invoked with correct values", () => {
+ cy.get("@filtersChangedHandler").should("be.calledWith", [
+ filter1,
+ filter2,
+ ]);
+
+ cy.get("@applyFilterHandler").should("be.calledWith", {
+ filter: 'currency != "USD" and currency != "CAD"',
+ filterStruct: { op: "and", filters: [filter1, filter2] },
+ });
+ });
+
+ it("AND WHEN second filter is deleted THEN changes are correctly applied", () => {
+ findOverflowItem('[data-index="1"]')
+ .find(".vuuFilterPillMenu")
+ .realClick();
+ clickButton("Delete");
+ clickButton("Remove");
+
+ cy.get("@filtersChangedHandler").should("be.calledWithExactly", [
+ filter1,
+ ]);
+ cy.get("@applyFilterHandler").should("be.calledWithExactly", {
+ filter: 'currency != "USD"',
+ filterStruct: filter1,
+ });
+ });
+
+ it("AND WHEN first filter is deleted THEN changes are correctly applied", () => {
+ findOverflowItem('[data-index="0"]')
+ .find(".vuuFilterPillMenu")
+ .realClick();
+ clickButton("Delete");
+ clickButton("Remove");
+
+ cy.get("@filtersChangedHandler").should("be.calledWithExactly", [
+ filter2,
+ ]);
+ cy.get("@applyFilterHandler").should("be.calledWithExactly", {
+ filter: 'currency != "CAD"',
+ filterStruct: filter2,
+ });
+ });
+ });
});
describe("The keyboard user", () => {
diff --git a/vuu-ui/packages/vuu-filters/src/filter-bar/useFilterBar.ts b/vuu-ui/packages/vuu-filters/src/filter-bar/useFilterBar.ts
index 1cbbcd81c..b9d63dd3a 100644
--- a/vuu-ui/packages/vuu-filters/src/filter-bar/useFilterBar.ts
+++ b/vuu-ui/packages/vuu-filters/src/filter-bar/useFilterBar.ts
@@ -23,7 +23,6 @@ import {
KeyboardEventHandler,
RefObject,
useCallback,
- useEffect,
useMemo,
useRef,
useState,
@@ -65,8 +64,6 @@ export const useFilterBar = ({
}: FilterBarHookProps) => {
const addButtonRef = useRef(null);
const editingFilter = useRef();
- const [activeFilterIndex, setActiveFilterIndex] =
- useState(activeFilterIdexProp);
const [showMenu, setShowMenu] = useState(showMenuProp);
const [editFilter, setEditFilter] = useState<
Partial | FilterWithPartialClause | undefined
@@ -78,13 +75,25 @@ export const useFilterBar = ({
[columnDescriptors]
);
+ const applyFilter = useCallback(
+ (filter?: Filter) => {
+ const query = filter ? filterAsQuery(filter, { columnsByName }) : "";
+ onApplyFilter({ filter: query, filterStruct: filter });
+ },
+ [columnsByName, onApplyFilter]
+ );
+
const {
+ activeFilterIndex,
filters,
onAddFilter,
onChangeFilter,
onDeleteFilter,
onRenameFilter,
+ onChangeActiveFilterIndex,
} = useFilters({
+ activeFilterIndex: activeFilterIdexProp,
+ applyFilter,
filters: filtersProp,
onFiltersChanged,
tableSchema,
@@ -142,29 +151,9 @@ export const useFilterBar = ({
[containerRef]
);
- const applyFilter = useCallback(
- (filter?: Filter) => {
- const query = filter ? filterAsQuery(filter, { columnsByName }) : "";
- onApplyFilter({ filter: query, filterStruct: filter });
- },
- [columnsByName, onApplyFilter]
- );
-
const deleteConfirmed = useCallback(
(filter: Filter) => {
- const indexOfFilter = filters.indexOf(filter);
- if (activeFilterIndex.includes(indexOfFilter)) {
- // deselect filter
- setActiveFilterIndex(
- activeFilterIndex.filter((i) => i !== indexOfFilter)
- );
- }
- const indexOfDeletedFilter = onDeleteFilter?.(filter);
- if (activeFilterIndex.includes(indexOfDeletedFilter)) {
- setActiveFilterIndex((indices) =>
- indices.filter((i) => i !== indexOfDeletedFilter)
- );
- }
+ onDeleteFilter(filter);
// TODO move focus to next/previous filter
requestAnimationFrame(() => {
@@ -173,7 +162,7 @@ export const useFilterBar = ({
}
});
},
- [activeFilterIndex, filters, focusFilterPill, onDeleteFilter]
+ [focusFilterPill, onDeleteFilter]
);
const getDeletePrompt = useMemo(
@@ -263,9 +252,8 @@ export const useFilterBar = ({
if (existing === undefined) {
const idx = onAddFilter(edited);
editPillLabel(idx);
- return idx;
} else {
- return onChangeFilter(existing, edited);
+ onChangeFilter(existing, edited);
}
},
[editPillLabel, onAddFilter, onChangeFilter]
@@ -276,8 +264,7 @@ export const useFilterBar = ({
switch (menuId) {
case "apply-save": {
const editedFilter = editFilter as Filter;
- const idx = addIfNewElseUpdate(editedFilter, editingFilter.current);
- setActiveFilterIndex(appendIfNotPresent(idx));
+ addIfNewElseUpdate(editedFilter, editingFilter.current);
setEditFilter(undefined);
editingFilter.current = undefined;
setShowMenu(false);
@@ -307,31 +294,12 @@ export const useFilterBar = ({
[editFilter, addIfNewElseUpdate]
);
- useEffect(() => {
- if (activeFilterIndex.length > 0) {
- const activeFilters = activeFilterIndex.map(
- (index) => filters[index]
- );
- if (activeFilters.length === 1) {
- const [filter] = activeFilters;
- applyFilter(filter);
- } else {
- applyFilter({
- op: "and",
- filters: activeFilters,
- });
- }
- } else {
- applyFilter();
- }
- }, [activeFilterIndex, applyFilter, filters]);
-
const handleChangeActiveFilterIndex = useCallback(
(itemIndex) => {
- setActiveFilterIndex(itemIndex);
+ onChangeActiveFilterIndex(itemIndex);
onChangeActiveFilterIndexProp?.(itemIndex);
},
- [onChangeActiveFilterIndexProp]
+ [onChangeActiveFilterIndexProp, onChangeActiveFilterIndex]
);
const handleClickAddFilter = useCallback(() => {
@@ -350,7 +318,6 @@ export const useFilterBar = ({
const handleChangeFilterClause = useCallback(
(idx: number) => (filterClause: Partial) => {
- console.log(`handleCHangeFilterClause ${JSON.stringify(filterClause)}`);
if (filterClause !== undefined) {
setEditFilter((ef) => replaceClause(ef, filterClause, idx));
setShowMenu(true);
@@ -477,9 +444,6 @@ export const useFilterBar = ({
};
};
-const appendIfNotPresent = (n: number) => (ns: number[]) =>
- ns.includes(n) ? ns : ns.concat(n);
-
function columnDescriptorsByName(
columns: ColumnDescriptor[]
): ColumnDescriptorsByName {
diff --git a/vuu-ui/packages/vuu-filters/src/filter-bar/useFilterState.ts b/vuu-ui/packages/vuu-filters/src/filter-bar/useFilterState.ts
new file mode 100644
index 000000000..6fd8c572e
--- /dev/null
+++ b/vuu-ui/packages/vuu-filters/src/filter-bar/useFilterState.ts
@@ -0,0 +1,67 @@
+import { useCallback, useState } from "react";
+import { Filter } from "@finos/vuu-filter-types";
+import { useControlled } from "@finos/vuu-ui-controls";
+
+export interface FilterStateHookProps {
+ activeFilterIndex: number[];
+ applyFilter: (f?: Filter) => void;
+ defaultFilters?: Filter[];
+ filters?: Filter[];
+}
+
+export function useFilterState({
+ activeFilterIndex: activeFilterIdexProp,
+ applyFilter,
+ defaultFilters,
+ filters: filtersProp,
+}: FilterStateHookProps) {
+ const [filters, setFilters] = useControlled({
+ controlled: filtersProp,
+ default: defaultFilters ?? [],
+ name: "useFilters",
+ state: "Filters",
+ });
+
+ const [activeIndices, setActiveIndices] =
+ useState(activeFilterIdexProp);
+
+ const onApplyFilter = useCallback(
+ ({ activeIndices, filters }: FilterState) => {
+ if (activeIndices.length > 0) {
+ const activeFilters = activeIndices.map((i) => filters[i]);
+ if (activeFilters.length === 1) {
+ const [filter] = activeFilters;
+ applyFilter(filter);
+ } else {
+ applyFilter({ op: "and", filters: activeFilters });
+ }
+ } else {
+ applyFilter();
+ }
+ },
+ [applyFilter]
+ );
+
+ const onFilterStateChange = useCallback(
+ ({ filters, activeIndices }: FilterState) => {
+ setFilters(filters);
+ setActiveIndices(activeIndices);
+ onApplyFilter({ filters, activeIndices });
+ },
+ [onApplyFilter]
+ );
+
+ const handleActiveIndicesChange = useCallback(
+ (indices: number[]) =>
+ onFilterStateChange({ filters, activeIndices: indices }),
+ [filters, onFilterStateChange]
+ );
+
+ return {
+ filterState: { activeIndices, filters },
+ onActiveIndicesChange: handleActiveIndicesChange,
+ onFilterStateChange,
+ };
+}
+
+type FilterState = { filters: Filter[]; activeIndices: number[] };
diff --git a/vuu-ui/packages/vuu-filters/src/filter-bar/useFilters.ts b/vuu-ui/packages/vuu-filters/src/filter-bar/useFilters.ts
index 877b40d60..2aa637fca 100644
--- a/vuu-ui/packages/vuu-filters/src/filter-bar/useFilters.ts
+++ b/vuu-ui/packages/vuu-filters/src/filter-bar/useFilters.ts
@@ -1,38 +1,23 @@
+import { useCallback } from "react";
import { TableSchema } from "@finos/vuu-data-types";
import { Filter, NamedFilter } from "@finos/vuu-filter-types";
import { useLayoutManager } from "@finos/vuu-shell";
-import { useControlled } from "@finos/vuu-ui-controls";
-import { useCallback } from "react";
+import { FilterStateHookProps, useFilterState } from "./useFilterState";
-export interface FiltersHookProps {
- defaultFilters?: Filter[];
- filters?: Filter[];
+export interface FiltersHookProps extends FilterStateHookProps {
onFiltersChanged?: (filters: Filter[]) => void;
tableSchema?: TableSchema;
}
export const useFilters = ({
- defaultFilters,
- filters: filtersProp,
onFiltersChanged,
tableSchema,
+ ...filterStateHookProps
}: FiltersHookProps) => {
- const [filters, setFilters] = useControlled({
- controlled: filtersProp,
- default: defaultFilters ?? [],
- name: "useFilters",
- state: "Filters",
- });
-
const { getApplicationSettings, saveApplicationSettings } =
useLayoutManager();
-
- type SavedFilterMap = {
- [key: string]: NamedFilter[];
- };
-
- const hasFilter = (filters: NamedFilter[], name: string) =>
- filters.findIndex((f) => f.name === name) !== -1;
+ const { filterState, onFilterStateChange, onActiveIndicesChange } =
+ useFilterState(filterStateHookProps);
const saveFilterToSettings = useCallback(
(filter: Filter, name?: string) => {
@@ -45,7 +30,7 @@ export const useFilters = ({
const key = `${module}:${table}`;
if (savedFilters) {
if (savedFilters[key]) {
- if (hasFilter(savedFilters[key], name)) {
+ if (hasFilterWithName(savedFilters[key], name)) {
newFilters = {
...savedFilters,
[key]: savedFilters[key].map((f) =>
@@ -55,7 +40,7 @@ export const useFilters = ({
} else if (
filter?.name &&
filter?.name !== name &&
- hasFilter(savedFilters[key], filter.name)
+ hasFilterWithName(savedFilters[key], filter.name)
) {
newFilters = {
...savedFilters,
@@ -90,23 +75,20 @@ export const useFilters = ({
const removeFilterFromSettings = useCallback(
(filter: Filter | NamedFilter) => {
- if (tableSchema && filter.name) {
- const savedFilters = getApplicationSettings(
- "filters"
- ) as SavedFilterMap;
+ if (!tableSchema || !filter.name) return;
- const { module, table } = tableSchema.table;
- const key = `${module}:${table}`;
+ const savedFilters = getApplicationSettings("filters") as SavedFilterMap;
+ if (!savedFilters) return;
- if (
- savedFilters[key]?.findIndex((f) => f.name === filter.name) !== -1
- ) {
- const newSavedFilters = {
- ...savedFilters,
- [key]: savedFilters[key].filter((f) => f.name !== filter.name),
- };
- saveApplicationSettings(newSavedFilters, "filters");
- }
+ const { module, table } = tableSchema.table;
+ const key = `${module}:${table}`;
+
+ if (hasFilterWithName(savedFilters[key], filter.name)) {
+ const newSavedFilters = {
+ ...savedFilters,
+ [key]: savedFilters[key].filter((f) => f.name !== filter.name),
+ };
+ saveApplicationSettings(newSavedFilters, "filters");
}
},
[getApplicationSettings, saveApplicationSettings, tableSchema]
@@ -114,23 +96,20 @@ export const useFilters = ({
const handleAddFilter = useCallback(
(filter: Filter) => {
- const index = filters.length;
- const newFilters = filters.concat(filter);
- setFilters(newFilters);
+ const index = filterState.filters.length;
+ const newFilters = filterState.filters.concat(filter);
+ const newIndices = appendIfNotPresent(filterState.activeIndices, index);
+ onFilterStateChange({ filters: newFilters, activeIndices: newIndices });
onFiltersChanged?.(newFilters);
return index;
},
- [filters, onFiltersChanged, setFilters]
+ [filterState, onFiltersChanged, onFilterStateChange]
);
const handleDeleteFilter = useCallback(
(filter: Filter) => {
- console.log(`handleDeleteFilter`, {
- filter,
- });
-
let index = -1;
- const newFilters = filters.filter((f, i) => {
+ const newFilters = filterState.filters.filter((f, i) => {
if (f !== filter) {
return true;
} else {
@@ -138,18 +117,29 @@ export const useFilters = ({
return false;
}
});
- setFilters(newFilters);
+
+ const newIndices = removeIndexAndDecrementLarger(
+ filterState.activeIndices,
+ index
+ );
+
+ onFilterStateChange({ filters: newFilters, activeIndices: newIndices });
onFiltersChanged?.(newFilters);
removeFilterFromSettings(filter);
return index;
},
- [filters, onFiltersChanged, removeFilterFromSettings, setFilters]
+ [
+ filterState,
+ onFiltersChanged,
+ onFilterStateChange,
+ removeFilterFromSettings,
+ ]
);
const handleRenameFilter = useCallback(
(filter: Filter, name: string) => {
let index = -1;
- const newFilters = filters.map((f, i) => {
+ const newFilters = filterState.filters.map((f, i) => {
if (f === filter) {
index = i;
return { ...filter, name };
@@ -157,19 +147,19 @@ export const useFilters = ({
return f;
}
});
- setFilters(newFilters);
+ onFilterStateChange({ ...filterState, filters: newFilters });
onFiltersChanged?.(newFilters);
saveFilterToSettings(filter, name);
return index;
},
- [filters, onFiltersChanged, saveFilterToSettings, setFilters]
+ [filterState, onFiltersChanged, onFilterStateChange, saveFilterToSettings]
);
const handleChangeFilter = useCallback(
(oldFilter: Filter, newFilter: Filter) => {
let index = -1;
- const newFilters = filters.map((f, i) => {
+ const newFilters = filterState.filters.map((f, i) => {
if (f === oldFilter) {
index = i;
return newFilter;
@@ -177,18 +167,41 @@ export const useFilters = ({
return f;
}
});
- setFilters(newFilters);
+ onFilterStateChange({ ...filterState, filters: newFilters });
onFiltersChanged?.(newFilters);
+
return index;
},
- [filters, onFiltersChanged, setFilters]
+ [filterState, onFiltersChanged, onFilterStateChange]
);
return {
- filters,
+ ...filterState,
+ activeFilterIndex: filterState.activeIndices,
+ onChangeActiveFilterIndex: onActiveIndicesChange,
onAddFilter: handleAddFilter,
onChangeFilter: handleChangeFilter,
onDeleteFilter: handleDeleteFilter,
onRenameFilter: handleRenameFilter,
};
};
+
+type SavedFilterMap = {
+ [key: string]: NamedFilter[];
+};
+
+const hasFilterWithName = (filters: NamedFilter[], name: string) =>
+ filters.findIndex((f) => f.name === name) !== -1;
+
+const appendIfNotPresent = (ns: number[], n: number) =>
+ ns.includes(n) ? ns : ns.concat(n);
+
+const removeIndexAndDecrementLarger = (
+ indices: number[],
+ idxToRemove: number
+) => {
+ return indices.reduce((res, i) => {
+ if (i === idxToRemove) return res;
+ return res.concat(i > idxToRemove ? i - 1 : i);
+ }, []);
+};
diff --git a/vuu-ui/packages/vuu-filters/src/filter-utils.ts b/vuu-ui/packages/vuu-filters/src/filter-utils.ts
index a2930dfcc..9d09fb881 100644
--- a/vuu-ui/packages/vuu-filters/src/filter-utils.ts
+++ b/vuu-ui/packages/vuu-filters/src/filter-utils.ts
@@ -86,7 +86,6 @@ export const addClause = (
clause: Partial,
{ combineWith = AND }: AddFilterOptions = DEFAULT_ADD_FILTER_OPTS
): FilterWithPartialClause => {
- console.log({ existingFilter, clause });
if (
isMultiClauseFilter(existingFilter) &&
existingFilter.op === combineWith