Skip to content

Commit fcc8b9e

Browse files
authored
fix: prevent workspace search bar text from getting garbled (#9703)
* chore: Reorganize hook calls for useWorkspacesFilter * refactor: Clean up some filter logic * refactor: Create debounce utility hooks * docs: Clean up comments for clarity * fix: Update focus logic to apply for any inner focus * fix: Add onBlur behavior for state syncs * chore: Add progress for debounce test * chore: Finish tests for debounce hooks * docs: Add file description and warning
1 parent b104e0e commit fcc8b9e

File tree

5 files changed

+374
-42
lines changed

5 files changed

+374
-42
lines changed

site/src/components/Filter/filter.tsx

Lines changed: 43 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,11 @@ import {
2020
} from "api/errors";
2121
import { useFilterMenu } from "./menu";
2222
import { BaseOption } from "./options";
23-
import debounce from "just-debounce-it";
2423
import MenuList from "@mui/material/MenuList";
2524
import { Loader } from "components/Loader/Loader";
2625
import Divider from "@mui/material/Divider";
2726
import OpenInNewOutlined from "@mui/icons-material/OpenInNewOutlined";
27+
import { useDebouncedFunction } from "hooks/debounce";
2828

2929
export type PresetFilter = {
3030
name: string;
@@ -58,7 +58,7 @@ export const useFilter = ({
5858
}
5959
};
6060

61-
const debounceUpdate = debounce(
61+
const { debounced: debounceUpdate, cancelDebounce } = useDebouncedFunction(
6262
(values: string | FilterValues) => update(values),
6363
500,
6464
);
@@ -69,6 +69,7 @@ export const useFilter = ({
6969
query,
7070
update,
7171
debounceUpdate,
72+
cancelDebounce,
7273
values,
7374
used,
7475
};
@@ -130,17 +131,7 @@ export const MenuSkeleton = () => (
130131
<BaseSkeleton sx={{ minWidth: 200, flexShrink: 0 }} />
131132
);
132133

133-
export const Filter = ({
134-
filter,
135-
isLoading,
136-
error,
137-
skeleton,
138-
options,
139-
learnMoreLink,
140-
learnMoreLabel2,
141-
learnMoreLink2,
142-
presets,
143-
}: {
134+
type FilterProps = {
144135
filter: ReturnType<typeof useFilter>;
145136
skeleton: ReactNode;
146137
isLoading: boolean;
@@ -150,20 +141,42 @@ export const Filter = ({
150141
error?: unknown;
151142
options?: ReactNode;
152143
presets: PresetFilter[];
153-
}) => {
154-
const shouldDisplayError = hasError(error) && isApiValidationError(error);
155-
const hasFilterQuery = filter.query !== "";
156-
const [searchQuery, setSearchQuery] = useState(filter.query);
157-
const inputRef = useRef<HTMLInputElement>(null);
144+
};
158145

146+
export const Filter = ({
147+
filter,
148+
isLoading,
149+
error,
150+
skeleton,
151+
options,
152+
learnMoreLink,
153+
learnMoreLabel2,
154+
learnMoreLink2,
155+
presets,
156+
}: FilterProps) => {
157+
// Storing local copy of the filter query so that it can be updated more
158+
// aggressively without re-renders rippling out to the rest of the app every
159+
// single time. Exists for performance reasons - not really a good way to
160+
// remove this; render keys would cause the component to remount too often
161+
const [queryCopy, setQueryCopy] = useState(filter.query);
162+
const textboxInputRef = useRef<HTMLInputElement>(null);
163+
164+
// Conditionally re-syncs the parent and local filter queries
159165
useEffect(() => {
160-
// We don't want to update this while the user is typing something or has the focus in the input
161-
const isFocused = document.activeElement === inputRef.current;
162-
if (!isFocused) {
163-
setSearchQuery(filter.query);
166+
const hasSelfOrInnerFocus =
167+
textboxInputRef.current?.contains(document.activeElement) ?? false;
168+
169+
// This doesn't address all state sync issues - namely, what happens if the
170+
// user removes focus just after this synchronizing effect fires. Also need
171+
// to rely on onBlur behavior as an extra safety measure
172+
if (!hasSelfOrInnerFocus) {
173+
setQueryCopy(filter.query);
164174
}
165175
}, [filter.query]);
166176

177+
const shouldDisplayError = hasError(error) && isApiValidationError(error);
178+
const hasFilterQuery = filter.query !== "";
179+
167180
return (
168181
<Box
169182
sx={{
@@ -198,12 +211,17 @@ export const Filter = ({
198211
"aria-label": "Filter",
199212
name: "query",
200213
placeholder: "Search...",
201-
value: searchQuery,
202-
ref: inputRef,
214+
value: queryCopy,
215+
ref: textboxInputRef,
203216
onChange: (e) => {
204-
setSearchQuery(e.target.value);
217+
setQueryCopy(e.target.value);
205218
filter.debounceUpdate(e.target.value);
206219
},
220+
onBlur: () => {
221+
if (queryCopy !== filter.query) {
222+
setQueryCopy(filter.query);
223+
}
224+
},
207225
sx: {
208226
borderRadius: "6px",
209227
borderTopLeftRadius: 0,

site/src/hooks/debounce.test.ts

Lines changed: 193 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,193 @@
1+
import { renderHook } from "@testing-library/react";
2+
import { useDebouncedFunction, useDebouncedValue } from "./debounce";
3+
4+
beforeAll(() => {
5+
jest.useFakeTimers();
6+
jest.spyOn(global, "setTimeout");
7+
});
8+
9+
afterAll(() => {
10+
jest.useRealTimers();
11+
jest.clearAllMocks();
12+
});
13+
14+
// Most UI tests should be structure from the user's experience, but just
15+
// because these are more abstract, general-purpose hooks, it seemed harder to
16+
// do that. Had to bring in some mocks
17+
function renderDebouncedValue<T = unknown>(value: T, time: number) {
18+
return renderHook(
19+
({ value, time }: { value: T; time: number }) => {
20+
return useDebouncedValue(value, time);
21+
},
22+
{
23+
initialProps: { value, time },
24+
},
25+
);
26+
}
27+
28+
function renderDebouncedFunction<Args extends unknown[]>(
29+
callbackArg: (...args: Args) => void | Promise<void>,
30+
time: number,
31+
) {
32+
return renderHook(
33+
({ callback, time }: { callback: typeof callbackArg; time: number }) => {
34+
return useDebouncedFunction<Args>(callback, time);
35+
},
36+
{
37+
initialProps: { callback: callbackArg, time },
38+
},
39+
);
40+
}
41+
42+
describe(`${useDebouncedValue.name}`, () => {
43+
it("Should immediately return out the exact same value (by reference) on mount", () => {
44+
const value = {};
45+
const { result } = renderDebouncedValue(value, 2000);
46+
47+
expect(result.current).toBe(value);
48+
expect.hasAssertions();
49+
});
50+
51+
it("Should not immediately resync state as the hook re-renders with new value argument", async () => {
52+
let value = 0;
53+
const time = 5000;
54+
55+
const { result, rerender } = renderDebouncedValue(value, time);
56+
expect(result.current).toEqual(0);
57+
58+
for (let i = 1; i <= 5; i++) {
59+
setTimeout(() => {
60+
value++;
61+
rerender({ value, time });
62+
}, i * 100);
63+
}
64+
65+
await jest.advanceTimersByTimeAsync(time - 100);
66+
expect(result.current).toEqual(0);
67+
expect.hasAssertions();
68+
});
69+
70+
it("Should resync after specified milliseconds pass with no change to arguments", async () => {
71+
const initialValue = false;
72+
const time = 5000;
73+
74+
const { result, rerender } = renderDebouncedValue(initialValue, time);
75+
expect(result.current).toEqual(false);
76+
77+
rerender({ value: !initialValue, time });
78+
await jest.runAllTimersAsync();
79+
80+
expect(result.current).toEqual(true);
81+
expect.hasAssertions();
82+
});
83+
});
84+
85+
describe(`${useDebouncedFunction.name}`, () => {
86+
describe("hook", () => {
87+
it("Should provide stable function references across re-renders", () => {
88+
const time = 5000;
89+
const { result, rerender } = renderDebouncedFunction(jest.fn(), time);
90+
91+
const { debounced: oldDebounced, cancelDebounce: oldCancel } =
92+
result.current;
93+
94+
rerender({ callback: jest.fn(), time });
95+
const { debounced: newDebounced, cancelDebounce: newCancel } =
96+
result.current;
97+
98+
expect(oldDebounced).toBe(newDebounced);
99+
expect(oldCancel).toBe(newCancel);
100+
expect.hasAssertions();
101+
});
102+
103+
it("Resets any pending debounces if the timer argument changes", async () => {
104+
const time = 5000;
105+
let count = 0;
106+
const incrementCount = () => {
107+
count++;
108+
};
109+
110+
const { result, rerender } = renderDebouncedFunction(
111+
incrementCount,
112+
time,
113+
);
114+
115+
result.current.debounced();
116+
rerender({ callback: incrementCount, time: time + 1 });
117+
118+
await jest.runAllTimersAsync();
119+
expect(count).toEqual(0);
120+
expect.hasAssertions();
121+
});
122+
});
123+
124+
describe("debounced function", () => {
125+
it("Resolve the debounce after specified milliseconds pass with no other calls", async () => {
126+
let value = false;
127+
const { result } = renderDebouncedFunction(() => {
128+
value = !value;
129+
}, 100);
130+
131+
result.current.debounced();
132+
133+
await jest.runOnlyPendingTimersAsync();
134+
expect(value).toBe(true);
135+
expect.hasAssertions();
136+
});
137+
138+
it("Always uses the most recent callback argument passed in (even if it switches while a debounce is queued)", async () => {
139+
let count = 0;
140+
const time = 500;
141+
142+
const { result, rerender } = renderDebouncedFunction(() => {
143+
count = 1;
144+
}, time);
145+
146+
result.current.debounced();
147+
rerender({
148+
callback: () => {
149+
count = 9999;
150+
},
151+
time,
152+
});
153+
154+
await jest.runAllTimersAsync();
155+
expect(count).toEqual(9999);
156+
expect.hasAssertions();
157+
});
158+
159+
it("Should reset the debounce timer with repeated calls to the method", async () => {
160+
let count = 0;
161+
const { result } = renderDebouncedFunction(() => {
162+
count++;
163+
}, 2000);
164+
165+
for (let i = 0; i < 10; i++) {
166+
setTimeout(() => {
167+
result.current.debounced();
168+
}, i * 100);
169+
}
170+
171+
await jest.runAllTimersAsync();
172+
expect(count).toBe(1);
173+
expect.hasAssertions();
174+
});
175+
});
176+
177+
describe("cancelDebounce function", () => {
178+
it("Should be able to cancel a pending debounce", async () => {
179+
let count = 0;
180+
const { result } = renderDebouncedFunction(() => {
181+
count++;
182+
}, 2000);
183+
184+
const { debounced, cancelDebounce } = result.current;
185+
debounced();
186+
cancelDebounce();
187+
188+
await jest.runAllTimersAsync();
189+
expect(count).toEqual(0);
190+
expect.hasAssertions();
191+
});
192+
});
193+
});

0 commit comments

Comments
 (0)