diff --git a/frontend/src/metabase/querying/notebook/components/LimitStep/LimitStep.tsx b/frontend/src/metabase/querying/notebook/components/LimitStep/LimitStep.tsx index b0267f630654f97a551b48246768c97d8d3fa346..6d1db67c6b6e76b9d8fd266bc590c96848727aa5 100644 --- a/frontend/src/metabase/querying/notebook/components/LimitStep/LimitStep.tsx +++ b/frontend/src/metabase/querying/notebook/components/LimitStep/LimitStep.tsx @@ -9,6 +9,8 @@ import * as Lib from "metabase-lib"; import type { NotebookStepProps } from "../../types"; import { NotebookCell } from "../NotebookCell"; +import { isLimitValid, parseLimit } from "./util"; + export function LimitStep({ query, step, @@ -20,15 +22,23 @@ export function LimitStep({ const limit = Lib.currentLimit(query, stageIndex); const [value, setValue] = useState(typeof limit === "number" ? limit : ""); - const handleBlur = (event: FocusEvent<HTMLInputElement>) => { - const nextLimit = parseInt(event.target.value, 0); - if (nextLimit >= 1) { + const updateLimit = (nextLimit: number) => { + if (isLimitValid(nextLimit)) { updateQuery(Lib.limit(query, stageIndex, nextLimit)); } }; + const handleBlur = (event: FocusEvent<HTMLInputElement>) => { + updateLimit(parseLimit(event.target.value)); + }; + const handleChange = (event: ChangeEvent<HTMLInputElement>) => { setValue(event.target.value); + + const isFocused = event.target === document.activeElement; + if (!isFocused) { + updateLimit(parseLimit(event.target.value)); + } }; return ( @@ -36,6 +46,7 @@ export function LimitStep({ <LimitInput className={CS.mb1} type="number" + min={1} value={value} placeholder={t`Enter a limit`} small diff --git a/frontend/src/metabase/querying/notebook/components/LimitStep/LimitStep.unit.spec.tsx b/frontend/src/metabase/querying/notebook/components/LimitStep/LimitStep.unit.spec.tsx index 0016b27ae5797de3cdc3e922b26525b6e75a079b..10643cdb80d4f3cfdb5e7be4834fb787566df0c6 100644 --- a/frontend/src/metabase/querying/notebook/components/LimitStep/LimitStep.unit.spec.tsx +++ b/frontend/src/metabase/querying/notebook/components/LimitStep/LimitStep.unit.spec.tsx @@ -54,14 +54,36 @@ describe("LimitStep", () => { expect(Lib.currentLimit(getNextQuery(), 0)).toBe(52); }); - it("should update the limit", () => { + it("should update the limit only when blurred", () => { const step = createMockNotebookStep({ query: QUERY_WITH_LIMIT }); - const { getNextQuery } = setup(step); + const { getNextQuery, updateQuery } = setup(step); const limitInput = screen.getByPlaceholderText("Enter a limit"); + + fireEvent.focus(limitInput); + + // HACK: manually set document.activeElement to the input + // so the focus check works. + Object.defineProperty(document, "activeElement", { + value: limitInput, + writable: false, + }); + + fireEvent.change(limitInput, { target: { value: "100" } }); fireEvent.change(limitInput, { target: { value: "1000" } }); fireEvent.blur(limitInput); + expect(updateQuery).toHaveBeenCalledTimes(1); + expect(Lib.currentLimit(getNextQuery(), 0)).toBe(1000); + }); + + it("should update the limit when not focused (eg. when clicking the arrows) (metabase#49587)", () => { + const step = createMockNotebookStep({ query: QUERY_WITH_LIMIT }); + const { getNextQuery } = setup(step); + + const limitInput = screen.getByPlaceholderText("Enter a limit"); + fireEvent.change(limitInput, { target: { value: "1000" } }); + expect(Lib.currentLimit(getNextQuery(), 0)).toBe(1000); }); @@ -84,4 +106,14 @@ describe("LimitStep", () => { expect(updateQuery).not.toHaveBeenCalled(); }); + + it("shouldn't update the limit if its not a valid number", () => { + const step = createMockNotebookStep({ query: QUERY_WITH_LIMIT }); + const { updateQuery } = setup(step); + + const limitInput = screen.getByPlaceholderText("Enter a limit"); + fireEvent.change(limitInput, { target: { value: "not a number" } }); + + expect(updateQuery).not.toHaveBeenCalled(); + }); }); diff --git a/frontend/src/metabase/querying/notebook/components/LimitStep/util.ts b/frontend/src/metabase/querying/notebook/components/LimitStep/util.ts new file mode 100644 index 0000000000000000000000000000000000000000..d9e2f561abaef32d287515cacb0898fa9295d92a --- /dev/null +++ b/frontend/src/metabase/querying/notebook/components/LimitStep/util.ts @@ -0,0 +1,7 @@ +export function isLimitValid(number: number) { + return !Number.isNaN(number) && Number.isInteger(number) && number > 0; +} + +export function parseLimit(value: string) { + return parseInt(value, 10); +}