Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: number input reset #3276

Merged
merged 4 commits into from
Dec 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions frontend/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
"@radix-ui/react-tooltip": "~1.0.7",
"@radix-ui/react-use-callback-ref": "~1.0.1",
"@radix-ui/react-use-controllable-state": "~1.0.1",
"@react-aria/focus": "^3.18.2",
"@react-aria/focus": "^3.19.0",
"@replit/codemirror-vim": "^6.2.1",
"@tailwindcss/typography": "^0.5.14",
"@tanstack/react-table": "^8.20.1",
Expand Down Expand Up @@ -115,7 +115,7 @@
"plotly.js": "^2.35.2",
"pyodide": "^0.26.2",
"react-arborist": "^3.4.0",
"react-aria-components": "^1.3.2",
"react-aria-components": "^1.5.0",
"react-codemirror-merge": "^4.23.5",
"react-dropzone": "^14.2.3",
"react-error-boundary": "^4.0.13",
Expand Down
2,037 changes: 1,038 additions & 999 deletions frontend/pnpm-lock.yaml

Large diffs are not rendered by default.

64 changes: 63 additions & 1 deletion frontend/src/hooks/__tests__/useDebounce.test.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
/* Copyright 2024 Marimo. All rights reserved. */
import { expect, describe, it, vi, beforeEach, afterEach } from "vitest";
import { renderHook, act } from "@testing-library/react";
import { useDebounce, useDebounceControlledState } from "../useDebounce";
import {
useDebounce,
useDebounceControlledState,
useDebouncedCallback,
} from "../useDebounce";

describe("useDebounce", () => {
beforeEach(() => {
Expand Down Expand Up @@ -117,3 +121,61 @@ describe("useDebounceControlledState", () => {
expect(onChange).not.toHaveBeenCalled();
});
});

describe("useDebouncedCallback", () => {
beforeEach(() => {
vi.useFakeTimers();
});

afterEach(() => {
vi.restoreAllMocks();
vi.useRealTimers();
});

it("should debounce the callback", () => {
const callback = vi.fn();
const { result } = renderHook(() => useDebouncedCallback(callback, 1000));

act(() => {
result.current("test");
result.current("test2");
result.current("test3");
});

expect(callback).not.toHaveBeenCalled();

act(() => {
vi.runAllTimers();
});

expect(callback).toHaveBeenCalledTimes(1);
expect(callback).toHaveBeenCalledWith("test3");
});

it("should maintain the same reference when deps don't change", () => {
const callback = vi.fn();
const { result, rerender } = renderHook(() =>
useDebouncedCallback(callback, 1000),
);

const firstRef = result.current;
rerender();
const secondRef = result.current;

expect(firstRef).toBe(secondRef);
});

it("should create new debounced function when delay changes", () => {
const callback = vi.fn();
const { result, rerender } = renderHook(
({ delay }) => useDebouncedCallback(callback, delay),
{ initialProps: { delay: 1000 } },
);

const firstRef = result.current;
rerender({ delay: 2000 });
const secondRef = result.current;

expect(firstRef).not.toBe(secondRef);
});
});
17 changes: 12 additions & 5 deletions frontend/src/hooks/useDebounce.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,18 +46,25 @@ export function useDebounceControlledState<T>(opts: {

const onUpdate = useEvent(onChange);

// If the initialValue changes, update the internal value
// If the initialValue changes, update the internal value and trigger onChange immediately
useEffect(() => {
setInternalValue(initialValue);
}, [initialValue]);
// When initialValue changes (like during reset), update immediately
if (!disabled) {
onUpdate(initialValue);
}
}, [initialValue, disabled, onUpdate]);

// Handle debounced updates for user input
useEffect(() => {
if (disabled) {
return;
}

onUpdate(debouncedValue);
}, [debouncedValue, disabled, onUpdate]);
// Only trigger debounced update if the value is different from initialValue
if (debouncedValue !== initialValue) {
onUpdate(debouncedValue);
}
}, [debouncedValue, initialValue, disabled, onUpdate]);

// If disabled, just pass through the initialValue and onChange
if (disabled) {
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/plugins/impl/DatePickerPlugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ interface DatePickerProps extends Data {
}

const DatePickerComponent = (props: DatePickerProps): JSX.Element => {
const handleInput = (valueAsDate: CalendarDate) => {
const handleInput = (valueAsDate: CalendarDate | null) => {
if (!valueAsDate) {
return;
}
Expand Down
10 changes: 6 additions & 4 deletions frontend/src/plugins/impl/DateRangePlugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,12 @@ interface DateRangePickerProps extends Data {
}

const DateRangePickerComponent = (props: DateRangePickerProps): JSX.Element => {
const handleInput = (valueAsDateRange: {
start: CalendarDate;
end: CalendarDate;
}) => {
const handleInput = (
valueAsDateRange: {
start: CalendarDate;
end: CalendarDate;
} | null,
) => {
if (!valueAsDateRange) {
return;
}
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/plugins/impl/DateTimePickerPlugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ interface DateTimePickerProps extends Data {
}

const DateTimePickerComponent = (props: DateTimePickerProps): JSX.Element => {
const handleInput = (valueAsDateTime: CalendarDateTime) => {
const handleInput = (valueAsDateTime: CalendarDateTime | null) => {
if (!valueAsDateTime) {
return;
}
Expand Down
10 changes: 8 additions & 2 deletions frontend/src/plugins/impl/NumberPlugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,19 @@ interface NumberComponentProps extends Data {
}

const NumberComponent = (props: NumberComponentProps): JSX.Element => {
const id = useId();
let id = useId();
if (import.meta.env.VITEST) {
id = "test-id";
}

// Create a debounced value of 200
const { value, onChange } = useDebounceControlledState({
initialValue: props.value,
delay: 200,
disabled: !props.debounce,
onChange: props.setValue,
onChange: (v) => {
props.setValue(v);
},
});

return (
Expand All @@ -70,6 +75,7 @@ const NumberComponent = (props: NumberComponentProps): JSX.Element => {
step={props.step}
onChange={onChange}
id={id}
aria-label={props.label || "Number input"}
/>
</Labeled>
);
Expand Down
144 changes: 144 additions & 0 deletions frontend/src/plugins/impl/__tests__/NumberPlugin.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
/* Copyright 2024 Marimo. All rights reserved. */
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
import { render, act, fireEvent } from "@testing-library/react";
import { NumberPlugin } from "../NumberPlugin";
import type { IPluginProps } from "../../types";

describe("NumberPlugin", () => {
beforeEach(() => {
vi.useFakeTimers();
});

afterEach(() => {
vi.useRealTimers();
});

it("renders with initial value and updates correctly", () => {
const plugin = new NumberPlugin();
const host = document.createElement("div");
const setValue = vi.fn();

// Initial render with value 5
const props: IPluginProps<
number | null,
(typeof plugin)["validator"]["_type"]
> = {
host,
value: 5,
setValue,
data: {
start: 0,
stop: 10,
step: 1,
label: null,
debounce: false,
fullWidth: false,
},
functions: {},
};

const { getByRole, rerender } = render(plugin.render(props));

// Wait for React-Aria NumberField to initialize
act(() => {
vi.advanceTimersByTime(0);
});

const input = getByRole("textbox", {
name: "Number input",
}) as HTMLInputElement;
expect(input).toBeTruthy();
expect(input.value).toBe("5");

// Update to value 7
const updatedProps = { ...props, value: 7 };
rerender(plugin.render(updatedProps));
expect(input.value).toBe("7");

// Reset to value 0
const resetProps = { ...props, value: 0 };
rerender(plugin.render(resetProps));
expect(input.value).toBe("0");
});

it("handles both immediate prop changes and debounced user input", () => {
const plugin = new NumberPlugin();
const host = document.createElement("div");
const setValue = vi.fn();

const props: IPluginProps<
number | null,
(typeof plugin)["validator"]["_type"]
> = {
host,
value: 5,
setValue,
data: {
start: 0,
stop: 10,
step: 1,
label: null,
debounce: true,
fullWidth: false,
},
functions: {},
};

// Initial render - setValue should be called immediately with initial value
const { getAllByRole, rerender } = render(plugin.render(props));

// Wait for React-Aria NumberField to initialize
act(() => {
vi.advanceTimersByTime(0);
});

expect(setValue).toHaveBeenCalledWith(5);

// Clear the mock to test debounced user input
setValue.mockClear();

const allInputs = getAllByRole("textbox", {
name: "Number input",
});
const input = allInputs[1];
expect(input).toBeTruthy();

// Simulate user typing and committing the value
act(() => {
// Focus and type the value
fireEvent.focus(input);
// Simulate React-Aria NumberField value change
fireEvent.change(input, { target: { value: "7" } });
// Commit the value with Enter key
fireEvent.keyDown(input, { key: "Enter" });
});

// Let React process the input
act(() => {
vi.advanceTimersByTime(0);
});

// Commit the value
act(() => {
fireEvent.blur(input);
});

// Process debounced updates
act(() => {
vi.advanceTimersByTime(200);
});

// Should call setValue after debounce for user input
expect(setValue).toHaveBeenCalledWith(7);

// Clear the mock again to test immediate prop changes
setValue.mockClear();

// Update props - should trigger immediate setValue
const updatedProps = { ...props, value: 3 };
rerender(plugin.render(updatedProps));
expect(setValue).toHaveBeenCalledWith(3);

vi.useRealTimers();
});
});
Loading
Loading