Skip to content
Open
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
1 change: 1 addition & 0 deletions src/components/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ export {TotalRow as MuiTotalRow, NotesRow as MuiNotesRow, FeeRow as MuiFeeRow, P
export {default as MuiFormikAsyncSelect} from './mui/formik-inputs/mui-formik-async-select'
export {default as MuiFormikCheckboxGroup} from './mui/formik-inputs/mui-formik-checkbox-group'
export {default as MuiFormikCheckbox} from './mui/formik-inputs/mui-formik-checkbox'
export {default as MuiFormikColorInput} from './mui/formik-inputs/mui-formik-color-input'
export {default as MuiFormikDatepicker} from './mui/formik-inputs/mui-formik-datepicker'
export {default as MuiFormikDiscountField} from './mui/formik-inputs/mui-formik-discountfield'
export {default as MuiFormikDropdownCheckbox} from './mui/formik-inputs/mui-formik-dropdown-checkbox'
Expand Down
166 changes: 166 additions & 0 deletions src/components/mui/__tests__/mui-formik-color-input.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
/**
* Copyright 2026 OpenStack Foundation
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
* http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
* */

jest.mock("i18n-react/dist/i18n-react", () => ({
__esModule: true,
default: { translate: (key) => key }
}));

import React from "react";
import { render, screen, fireEvent, act, waitFor } from "@testing-library/react";
import { Formik, Form } from "formik";
import "@testing-library/jest-dom";
import MuiFormikColorInput from "../formik-inputs/mui-formik-color-input";

const DEBOUNCE_MS = 250;
const PLACEHOLDER_KEY = "color_picker.placeholder";

const renderWithFormik = (props, initialValues = { testField: "" }) =>
render(
<Formik initialValues={initialValues} onSubmit={jest.fn()}>
<Form>
<MuiFormikColorInput name="testField" {...props} />
</Form>
</Formik>
);

const renderWithSubmit = (initialValues = { testField: "" }, props = {}) => {
const onSubmit = jest.fn();
render(
<Formik initialValues={initialValues} onSubmit={onSubmit}>
<Form>
<MuiFormikColorInput name="testField" {...props} />
<button type="submit">Submit</button>
</Form>
</Formik>
);
return onSubmit;
};

// Restore real timers after each test to prevent leaking fake timers
afterEach(() => jest.useRealTimers());

const getColorInput = () => document.querySelector('input[type="color"]');

describe("MuiFormikColorInput", () => {
test("renders a color input", () => {
renderWithFormik({});
expect(getColorInput()).toBeInTheDocument();
});

test("loads initial value: reflects value, shows clear button, hides placeholder", () => {
renderWithFormik({}, { testField: "#ff0000" });
expect(getColorInput()).toHaveValue("#ff0000");
expect(screen.getByRole("button")).toBeInTheDocument();
expect(screen.queryByText(PLACEHOLDER_KEY)).not.toBeInTheDocument();
});

test("empty initial state: shows placeholder and no clear button", () => {
renderWithFormik({});
expect(screen.getByText(PLACEHOLDER_KEY)).toBeInTheDocument();
expect(screen.queryByRole("button")).not.toBeInTheDocument();
});

test("blur without selection: formik value stays empty and placeholder remains", async () => {
const onSubmit = renderWithSubmit({ testField: "" });
const input = getColorInput();

fireEvent.focus(input);
fireEvent.blur(input);

expect(screen.getByText(PLACEHOLDER_KEY)).toBeInTheDocument();
fireEvent.click(screen.getByText("Submit"));
await waitFor(() => {
expect(onSubmit).toHaveBeenCalledWith({ testField: "" }, expect.anything());
});
});

// The useEffect resets hasValue to false whenever localValue changes (before the debounce
// commits the new color to field.value). The clear button only appears after the debounce fires.
test("select a color: shows clear button and hides placeholder after debounce", () => {
jest.useFakeTimers();
renderWithFormik({});

fireEvent.change(getColorInput(), { target: { value: "#33aaff" } });
act(() => jest.advanceTimersByTime(DEBOUNCE_MS));

expect(screen.getByRole("button")).toBeInTheDocument();
expect(screen.queryByText(PLACEHOLDER_KEY)).not.toBeInTheDocument();
});

test("select a color: updates formik value after debounce", async () => {
jest.useFakeTimers();
const onSubmit = renderWithSubmit({ testField: "" });

fireEvent.change(getColorInput(), { target: { value: "#33aaff" } });
act(() => jest.advanceTimersByTime(DEBOUNCE_MS));
jest.useRealTimers();

fireEvent.click(screen.getByText("Submit"));
await waitFor(() => {
expect(onSubmit).toHaveBeenCalledWith({ testField: "#33aaff" }, expect.anything());
});
});

test("clear button: shows placeholder and hides clear button", () => {
renderWithFormik({}, { testField: "#ff0000" });

fireEvent.click(screen.getByRole("button"));

expect(screen.getByText(PLACEHOLDER_KEY)).toBeInTheDocument();
expect(screen.queryByRole("button")).not.toBeInTheDocument();
});

test("clear button: resets formik value to empty", async () => {
const onSubmit = renderWithSubmit({ testField: "#ff0000" });

fireEvent.click(screen.getAllByRole("button")[0]);

fireEvent.click(screen.getByText("Submit"));
await waitFor(() => {
expect(onSubmit).toHaveBeenCalledWith({ testField: "" }, expect.anything());
});
});

test("shows error message when field is touched and has an error", () => {
render(
<Formik
initialValues={{ testField: "" }}
initialErrors={{ testField: "Color is required" }}
initialTouched={{ testField: true }}
onSubmit={jest.fn()}
>
<Form>
<MuiFormikColorInput name="testField" />
</Form>
</Formik>
);
expect(screen.getByText("Color is required")).toBeInTheDocument();
});

test("does not show error when field is not yet touched", () => {
render(
<Formik
initialValues={{ testField: "" }}
initialErrors={{ testField: "Color is required" }}
initialTouched={{ testField: false }}
onSubmit={jest.fn()}
>
<Form>
<MuiFormikColorInput name="testField" />
</Form>
</Formik>
);
expect(screen.queryByText("Color is required")).not.toBeInTheDocument();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -137,4 +137,4 @@ const MuiFormikAsyncAutocomplete = ({
);
};

export default MuiFormikAsyncAutocomplete;
export default MuiFormikAsyncAutocomplete;
114 changes: 114 additions & 0 deletions src/components/mui/formik-inputs/mui-formik-color-input.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import React, { useEffect, useRef, useState } from "react";
import T from "i18n-react";
import PropTypes from "prop-types";
import { Box, IconButton, InputAdornment, TextField } from "@mui/material";
import ClearIcon from "@mui/icons-material/Clear";
import { useField } from "formik";
import { DEBOUNCE_WAIT_250 } from "../../../utils/constants";

const MuiFormikColorInput = ({ name, placeholder = T.translate("color_picker.placeholder"), InputProps: restInputProps, ...rest }) => {
const [field, meta, helpers] = useField(name);
const [hasValue, setHasValue] = useState(Boolean(field.value));
const [localValue, setLocalValue] = useState(field.value || "#000000");
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[HIGH] Default "#000000" masks falsy initialValues.

useState(field.value || "#000000") silently shows black while Formik state stays "" / null. If the user never touches the input, the form submits with the original empty value while the swatch implies #000000 was selected → data mismatch on save.

Suggested fix: also seed Formik on mount when field.value is empty:

useEffect(() => { if (!field.value) helpers.setValue("#000000"); }, []);

or render the swatch only when field.value is set.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The color picker would pick a default color if the user don't interact with the component. It looks like this contradicts #231 (comment).
Please let me know if I should add some initial state to display the input as not selected since the input type="color" doesn't support an empty value

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PR updated with changes to display a placeholder if not value is selected and allow to clean value.

image image

const debounceRef = useRef(null);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[MEDIUM] No re-sync with external field.value changes.

localValue is initialized once from field.value and only mutated by user input. After Formik resetForm(), parent setFieldValue, or async-loaded initial values, the swatch keeps showing the old color while Formik state has changed → broken edit/reset flows.

Suggested fix:

useEffect(() => {
  if (field.value !== localValue) setLocalValue(field.value || "#000000");
}, [field.value]);

(guard against overwriting in-flight typing if needed.)

[LOW] No debounce cleanup on unmount.

A pending setTimeout can fire helpers.setValue after unmount → stale writes / dev warnings, especially in tabbed forms. Add:

useEffect(() => () => { if (debounceRef.current) clearTimeout(debounceRef.current); }, []);


useEffect(() => {
setHasValue(Boolean(field.value));
if (field.value && field.value !== localValue) setLocalValue(field.value);
}, [field.value, localValue]);
Comment on lines +15 to +18
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

cat -n src/components/mui/formik-inputs/mui-formik-color-input.js

Repository: OpenStackweb/openstack-uicore-foundation

Length of output: 4198


Remove localValue from the effect dependency array to prevent snapping during in-flight edits.

Including localValue in the dependency array causes the effect to re-run on every local keystroke. Since field.value hasn't changed yet (still debouncing), the condition on line 16 becomes true and overwrites the user's input with the stale Formik value, breaking the debounce UX.

Proposed fix
  useEffect(() => {
    setHasValue(Boolean(field.value));
    if (field.value && field.value !== localValue) setLocalValue(field.value);
-  }, [field.value, localValue]);
+  }, [field.value]);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/mui/formik-inputs/mui-formik-color-input.js` around lines 14 -
17, The effect watching Formik's value is re-running on every local keystroke
because localValue is in the dependency array; remove localValue from the
dependencies so the useEffect only reacts to changes in field.value. Keep the
body as-is: call setHasValue(Boolean(field.value)) and only call
setLocalValue(field.value) when field.value exists and differs from the current
local state, but ensure the dependency array for the useEffect uses
[field.value] (not [field.value, localValue]) so in-flight edits aren't
overwritten; update the useEffect declaration accordingly.


useEffect(() => () => {
if (debounceRef.current) clearTimeout(debounceRef.current);
}, []);
Comment thread
coderabbitai[bot] marked this conversation as resolved.

const handleChange = (e) => {
const value = e.target.value;
setLocalValue(value);
setHasValue(true);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why do we need this state ? can't you just use localValue ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this state is to track when the user pick a color since html color input requires a default value

if (debounceRef.current) clearTimeout(debounceRef.current);
debounceRef.current = setTimeout(() => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why do we need to debounce the onchange ? we never do this . If this is strictly necessary, add comments why

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is to limit the render of formik props when the user is dragging the cursor to pick a color, without this the component turns laggy.

helpers.setValue(value);
debounceRef.current = null;
}, DEBOUNCE_WAIT_250);
};

const handleBlur = (e) => {
field.onBlur(e);
helpers.setTouched(true);
if (debounceRef.current) {
clearTimeout(debounceRef.current);
debounceRef.current = null;
helpers.setValue(hasValue ? localValue : "");
}
};

const handleClear = (e) => {
e.stopPropagation();
if (debounceRef.current) {
clearTimeout(debounceRef.current);
debounceRef.current = null;
}
setHasValue(false);
helpers.setValue("");
helpers.setTouched(true);
};
Comment thread
coderabbitai[bot] marked this conversation as resolved.

return (
<Box sx={{ position: "relative", width: "100%" }}>
<TextField
type="color"
name={field.name}
value={localValue}
onChange={handleChange}
onBlur={handleBlur}
error={meta.touched && Boolean(meta.error)}
helperText={meta.touched && meta.error}
fullWidth
InputProps={{
...(hasValue ? {
endAdornment: (
<InputAdornment position="end">
<IconButton size="small" onClick={handleClear} edge="end" disableRipple>
<ClearIcon fontSize="small" />
</IconButton>
</InputAdornment>
),
} : {}), ...restInputProps
}}
sx={{
"& input[type='color']::-webkit-color-swatch-wrapper": { padding: "2px" },
}}
{...rest}
Comment thread
tomrndom marked this conversation as resolved.
/>
{!hasValue && (
<Box
sx={{
position: "absolute",
top: 2,
left: 2,
right: 2,
bottom: 2,
borderRadius: "3px",
bgcolor: "background.paper",
pointerEvents: "none",
display: "flex",
alignItems: "center",
px: "14px",
gap: 1,
}}
>
<Box component="span" sx={{ color: "text.disabled" }}>
{placeholder}
</Box>
</Box>
)}
</Box>
);
};

MuiFormikColorInput.propTypes = {
name: PropTypes.string.isRequired,
placeholder: PropTypes.string
};

export default MuiFormikColorInput;
3 changes: 3 additions & 0 deletions src/i18n/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -138,5 +138,8 @@
"total": "Total",
"rate": "Rate",
"action": "Action"
},
"color_picker": {
"placeholder": "Select a color"
}
}
1 change: 1 addition & 0 deletions src/utils/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ export const BPS = 100;

export const CODE_200 = 200;

export const DEBOUNCE_WAIT_150 = 150;
export const DEBOUNCE_WAIT_250 = 250;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

any reason why you can't just use 250 ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

250 made the delay between selecting a color and seeing it applied slightly noticeable, 150 felt a bit more responsive, but both work fine

export const DEBOUNCE_WAIT = 500;

Expand Down
1 change: 1 addition & 0 deletions webpack.common.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ module.exports = {
'components/mui/formik-inputs/async-select': './src/components/mui/formik-inputs/mui-formik-async-select.js',
'components/mui/formik-inputs/checkbox-group': './src/components/mui/formik-inputs/mui-formik-checkbox-group.js',
'components/mui/formik-inputs/checkbox': './src/components/mui/formik-inputs/mui-formik-checkbox.js',
'components/mui/formik-inputs/color-input': './src/components/mui/formik-inputs/mui-formik-color-input.js',
'components/mui/formik-inputs/datepicker': './src/components/mui/formik-inputs/mui-formik-datepicker.js',
'components/mui/formik-inputs/discount-field': './src/components/mui/formik-inputs/mui-formik-discountfield.js',
'components/mui/formik-inputs/dropdown-checkbox': './src/components/mui/formik-inputs/mui-formik-dropdown-checkbox.js',
Expand Down
Loading