Fix: validation errors in hidden tabs #1037
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,131 @@ | ||
| // @testing-library/react 12 (React 16) does not export renderHook; use a | ||
| // lightweight component wrapper instead. | ||
| import "@testing-library/jest-dom"; | ||
| import React, { useState } from "react"; | ||
| import { act, render, screen } from "@testing-library/react"; | ||
| import { useFormik } from "formik"; | ||
| import useScrollToError from "../useScrollToError"; | ||
|
|
||
| window.HTMLElement.prototype.scrollIntoView = jest.fn(); | ||
|
|
||
| // jsdom implements no layout, so offsetParent is always null regardless of | ||
| // CSS. Reflect the one hiding mechanism this hook cares about (the `hidden` | ||
| // attribute) so tests can simulate real display:none semantics. | ||
| beforeAll(() => { | ||
| Object.defineProperty(window.HTMLElement.prototype, "offsetParent", { | ||
| configurable: true, | ||
| get() { | ||
| let node = this; | ||
| while (node) { | ||
| if (node.hidden) return null; | ||
| node = node.parentElement; | ||
| } | ||
| return document.body; | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| beforeEach(() => { | ||
| window.HTMLElement.prototype.scrollIntoView.mockClear(); | ||
| }); | ||
|
|
||
| const flushDoubleRaf = () => | ||
| act( | ||
| () => | ||
| new Promise((resolve) => { | ||
| requestAnimationFrame(() => requestAnimationFrame(resolve)); | ||
| }) | ||
| ); | ||
|
|
||
| const TabbedHarness = ({ onActiveTabChange }) => { | ||
| const [activeTab, setActiveTab] = useState("b"); | ||
| const formik = useFormik({ | ||
| initialValues: { name: "" }, | ||
| validate: (values) => (values.name ? {} : { name: "required" }), | ||
| onSubmit: () => {} | ||
| }); | ||
|
|
||
| useScrollToError(formik, true, (value) => { | ||
| setActiveTab(value); | ||
| onActiveTabChange?.(value); | ||
| }); | ||
|
|
||
| return ( | ||
| <form onSubmit={formik.handleSubmit}> | ||
| <div role="tabpanel" id="tabpanel-a" hidden={activeTab !== "a"}> | ||
| <input name="name" onChange={formik.handleChange} /> | ||
| </div> | ||
| <div role="tabpanel" id="tabpanel-b" hidden={activeTab !== "b"} /> | ||
| <button type="submit">save</button> | ||
| </form> | ||
| ); | ||
| }; | ||
|
|
||
| const VisibleHarness = () => { | ||
| const formik = useFormik({ | ||
| initialValues: { name: "" }, | ||
| validate: (values) => (values.name ? {} : { name: "required" }), | ||
| onSubmit: () => {} | ||
| }); | ||
|
|
||
| useScrollToError(formik, true, jest.fn()); | ||
|
|
||
| return ( | ||
| <form onSubmit={formik.handleSubmit}> | ||
| <input name="name" onChange={formik.handleChange} /> | ||
| <button type="submit">save</button> | ||
| </form> | ||
| ); | ||
| }; | ||
|
|
||
| const UntaggedHarness = () => { | ||
| const formik = useFormik({ | ||
| initialValues: { name: "" }, | ||
| validate: (values) => (values.name ? {} : { name: "required" }), | ||
| onSubmit: () => {} | ||
| }); | ||
|
|
||
| useScrollToError(formik, true); | ||
|
|
||
| return ( | ||
| <form onSubmit={formik.handleSubmit}> | ||
| <input name="name" onChange={formik.handleChange} /> | ||
| <button type="submit">save</button> | ||
| </form> | ||
| ); | ||
| }; | ||
|
|
||
| describe("useScrollToError (tab-aware)", () => { | ||
| it("switches to the owning tab and scrolls when the errored field is hidden", async () => { | ||
| const onActiveTabChange = jest.fn(); | ||
| render(<TabbedHarness onActiveTabChange={onActiveTabChange} />); | ||
|
|
||
| await act(async () => { | ||
| screen.getByText("save").click(); | ||
| }); | ||
| await flushDoubleRaf(); | ||
|
|
||
| expect(onActiveTabChange).toHaveBeenCalledWith("a"); | ||
| expect(window.HTMLElement.prototype.scrollIntoView).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("does not switch tabs when the errored field is already visible", async () => { | ||
| render(<VisibleHarness />); | ||
|
|
||
| await act(async () => { | ||
| screen.getByText("save").click(); | ||
| }); | ||
|
|
||
| expect(window.HTMLElement.prototype.scrollIntoView).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("behaves as before when setActiveTab is not passed", async () => { | ||
| render(<UntaggedHarness />); | ||
|
|
||
| await act(async () => { | ||
| screen.getByText("save").click(); | ||
| }); | ||
|
|
||
| expect(window.HTMLElement.prototype.scrollIntoView).toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,40 +27,88 @@ function smoothScrollTo(targetScrollTop, duration) { | |
| requestAnimationFrame(animationStep); | ||
| } | ||
|
|
||
| // Waits two animation frames so a DOM mutation applied just before this call | ||
| // (e.g. React removing a tabpanel's `hidden` attribute after setActiveTab) | ||
| // has been through layout before we measure/scroll against it. | ||
| function afterNextLayout(callback) { | ||
| requestAnimationFrame(() => requestAnimationFrame(callback)); | ||
| } | ||
|
|
||
| const useScrollToError = (formik, relative = false, setActiveTab) => { | ||
| const { errors, isValid, isSubmitting } = formik; | ||
| const errorArray = Object.keys(errors); | ||
| const errorCount = errorArray.length; | ||
|
|
||
| useEffect(() => { | ||
| if (isValid || errorCount === 0) return; | ||
|
|
||
| const scrollToFirstVisible = () => { | ||
| const elementsSorted = errorArray | ||
| .reduce((result, error) => { | ||
| const element = document.querySelector(`[name='${error}']`); | ||
| if (!element) return result; | ||
|
|
||
| const rect = element.getBoundingClientRect(); | ||
| const absoluteTop = rect.top + window.pageYOffset; | ||
|
|
||
| result.push({ element, top: absoluteTop }); | ||
| return result; | ||
| }, []) | ||
| .sort((a, b) => a.top - b.top); | ||
|
Comment on lines
+48
to
+57
There was a problem hiding this comment. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Exclude hidden fields before selecting the scroll target. When errors exist in both visible and hidden panels, Proposed fix const element = document.querySelector(`[name='${error}']`);
- if (!element) return result;
+ if (!element || element.offsetParent === null) return result;Add a mixed-tab regression test with one hidden error and one visible error. The hook must scroll to the visible field. Also applies to: 94-96 🤖 Prompt for AI Agents |
||
|
|
||
| if (elementsSorted.length === 0) return; | ||
|
|
||
| const target = elementsSorted[0]; | ||
|
|
||
| const offset = 100; // adjust as needed | ||
| const duration = 500; // 500ms scroll duration | ||
| const scrollToY = target.top - offset; | ||
|
|
||
| if (relative) { | ||
| target?.element.scrollIntoView({ | ||
| behavior: "smooth", | ||
| block: "center" | ||
| }); | ||
| } else { | ||
| smoothScrollTo(scrollToY, duration); | ||
| } | ||
| }; | ||
|
|
||
| if (typeof setActiveTab !== "function") { | ||
| scrollToFirstVisible(); // unchanged path for every other call site | ||
| return; | ||
| } | ||
|
|
||
| // Tab-aware path: panels in a tabbed form are typically mounted-but- | ||
| // hidden, so `[name=...]` selectors still match fields on an inactive | ||
| // tab, but measuring/scrolling a `display:none` element is meaningless. | ||
| // `offsetParent` is `null` for any element hidden via `display:none`, | ||
| // including via an ancestor's `hidden` attribute. | ||
| const matches = errorArray | ||
| .map((error) => document.querySelector(`[name='${error}']`)) | ||
| .filter(Boolean); | ||
|
|
||
| const allMatchesHidden = | ||
| matches.length > 0 && matches.every((el) => el.offsetParent === null); | ||
|
|
||
| if (!allMatchesHidden) { | ||
| scrollToFirstVisible(); | ||
| return; | ||
| } | ||
|
|
||
| // Every matched field is hidden: jump to the tab that owns the first | ||
| // errored field, then defer the scroll until it's actually visible. | ||
| // Tab panels are identified by the `id="tabpanel-<value>"` convention. | ||
| const panelId = matches[0].closest("[role=\"tabpanel\"]")?.id; | ||
| const tabValue = panelId?.match(/^tabpanel-(.+)$/)?.[1]; | ||
|
|
||
| if (!tabValue) { | ||
| scrollToFirstVisible(); // not inside a tagged tab panel, fall back | ||
| return; | ||
| } | ||
|
|
||
| setActiveTab(tabValue); | ||
| afterNextLayout(scrollToFirstVisible); | ||
| }, [isSubmitting]); | ||
|
There was a problem hiding this comment. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline src/hooks/useScrollToError.js --view expanded
rg -n -C 4 'formik\.setErrors|useScrollToError\(' \
src/hooks/useScrollToError.js \
src/components/forms/selection-plan-form.js \
src/pages/events/components/event-type-dialog.js \
src/hooks/__tests__/useScrollToError.test.jsRepository: fntechgit/summit-admin Length of output: 4491 🏁 Script executed: #!/bin/bash
set -euo pipefail
cat -n src/hooks/useScrollToError.js
printf '\n--- selection-plan-form context ---\n'
sed -n '90,130p' src/components/forms/selection-plan-form.js
printf '\n--- event-type-dialog context ---\n'
sed -n '145,180p' src/pages/events/components/event-type-dialog.js
printf '\n--- hook tests ---\n'
cat -n src/hooks/__tests__/useScrollToError.test.jsRepository: fntechgit/summit-admin Length of output: 12045 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Formik declarations and dependency versions ---'
rg -n -C 3 '"formik"|setErrors|SET_ERRORS|isSubmitting' \
package.json package-lock.json yarn.lock pnpm-lock.yaml \
src --glob '!src/hooks/useScrollToError.js' \
--glob '!src/hooks/__tests__/useScrollToError.test.js' \
2>/dev/null || true
printf '%s\n' '--- selection-plan-form error inputs ---'
rg -n -C 6 'propsErrors|onSave|errors' src/components/forms/selection-plan-form.js
printf '%s\n' '--- event-type-dialog error inputs ---'
rg -n -C 6 'const .*errors|errors[ =:]|onSubmit|dispatch|save' \
src/pages/events/components/event-type-dialog.js
printf '%s\n' '--- test configuration and existing external-error patterns ---'
rg -n -C 4 'setErrors|external.*error|server.*error|render\(|flushDoubleRaf|isSubmitting' \
src --glob '*test*' --glob '*spec*' \
package.json jest.config.* babel.config.* 2>/dev/null || trueRepository: fntechgit/summit-admin Length of output: 50379 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- package metadata ---'
python3 - <<'PY'
import json
from pathlib import Path
p = Path("package.json")
if p.exists():
data = json.loads(p.read_text())
print("formik:", data.get("dependencies", {}).get("formik") or data.get("devDependencies", {}).get("formik"))
PY
for f in package-lock.json yarn.lock pnpm-lock.yaml; do
if [ -f "$f" ]; then
printf '%s\n' "--- $f ---"
rg -n -m 3 'formik(@|:|")' "$f" || true
fi
done
printf '%s\n' '--- focused error-flow context ---'
sed -n '1,90p' src/components/forms/selection-plan-form.js
sed -n '1,90p' src/pages/events/components/event-type-dialog.js
rg -n -C 8 'propsErrors|errors[),]|errors[ =:]|setErrors|isSubmitting' \
src/components/forms/selection-plan-form.js \
src/pages/events/components/event-type-dialog.js
printf '%s\n' '--- static dependency invariant ---'
python3 - <<'PY'
import re
from pathlib import Path
hook = Path("src/hooks/useScrollToError.js").read_text()
effect = re.search(r'useEffect\(\(\) => \{(?P<body>.*?)\n\s*\},\s*\[(?P<deps>[^\]]*)\]\);', hook, re.S)
if not effect:
raise SystemExit("hook effect not found")
body = effect.group("body")
deps = [x.strip() for x in effect.group("deps").split(",") if x.strip()]
print("effect dependencies:", deps)
for name in ("errors", "isValid", "isSubmitting", "errorArray", "errorCount"):
print(f"{name}: body={bool(re.search(r'\\b' + name + r'\\b', body))}")
print("external setErrors call sites:")
for path in (
"src/components/forms/selection-plan-form.js",
"src/pages/events/components/event-type-dialog.js",
):
text = Path(path).read_text()
print(path, len(re.findall(r'formik\\.setErrors\\s*\\(', text)))
PYRepository: fntechgit/summit-admin Length of output: 16651 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- locked Formik version ---'
sed -n '6450,6475p' yarn.lock
printf '%s\n' '--- dependency invariant ---'
python3 - <<'PY'
import re
from pathlib import Path
hook = Path("src/hooks/useScrollToError.js").read_text()
effect = re.search(
r'useEffect\(\(\) => \{(?P<body>.*?)\n\s*\},\s*\[(?P<deps>[^\]]*)\]\);',
hook,
re.S,
)
if not effect:
raise SystemExit("hook effect not found")
body = effect.group("body")
deps = [item.strip() for item in effect.group("deps").split(",") if item.strip()]
print("effect dependencies:", deps)
for name in ("errors", "isValid", "isSubmitting", "errorArray", "errorCount"):
present = bool(re.search(r"\b" + re.escape(name) + r"\b", body))
print(f"{name}: body={present}")
for path in (
"src/components/forms/selection-plan-form.js",
"src/pages/events/components/event-type-dialog.js",
):
text = Path(path).read_text()
effect_deps = re.findall(
r"(?:propsErrors|errors)\s*\]\s*\);", text
)
calls = len(re.findall(r"formik\.setErrors\s*\(", text))
print(f"{path}: setErrors_calls={calls}, matching_effect_dependency_tokens={effect_deps}")
PYRepository: fntechgit/summit-admin Length of output: 1521 🌐 Web query:
💡 Result: In Formik 2.4.6, the setErrors method and the SET_ERRORS reducer action are implemented as follows: setErrors Implementation: The setErrors function is a memoized callback provided by the Formik component (via useEventCallback or React.useCallback) that dispatches an action to the internal state reducer [1][2][3]. const setErrors = React.useCallback((errors: FormikErrors) => { dispatch({ type: 'SET_ERRORS', payload: errors }); }, []); SET_ERRORS Reducer Action: The internal formikReducer handles the SET_ERRORS action by updating the errors object in the state, but only if the new errors object is not deeply equal to the current errors object [1][4][3]. This optimization prevents unnecessary re-renders [5]. case 'SET_ERRORS': if (isEqual(state.errors, msg.payload)) { return state; } return {...state, errors: msg.payload }; isSubmitting State: The isSubmitting state is independent of setErrors. It is managed via its own reducer action, SET_ISSUBMITTING [1][4][3]. Formik automatically sets isSubmitting to true when a submission is attempted and ensures it is set back to false once the submission handler (if it returns a promise) resolves or rejects [6][7]. If the submission handler is synchronous, you must manually call setSubmitting(false) to reset this state [6][7]. Top results: [1], [4], [7], [3] Citations:
Re-run scrolling when Formik errors change. Add 🤖 Prompt for AI Agents |
||
| }; | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that visible errors do not activate a tab.
VisibleHarnesspasses an unreachablejest.fn()to the hook. The test only asserts scrolling. It still passes if the hook incorrectly callssetActiveTabfor a visible field.Pass a mock through
VisibleHarnessand assert that it was not called.Also applies to: 112-120
🤖 Prompt for AI Agents