Fix: validation errors in hidden tabs - #1037
Conversation
📝 WalkthroughWalkthroughThe PR adds tab-aware error scrolling, explicit form field names, and tabpanel identifiers. It updates selection plan and event type forms to use the enhanced ChangesForm error navigation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Formik
participant useScrollToError
participant setActiveTab
participant TabPanel
participant Browser
Formik->>useScrollToError: Report validation errors
useScrollToError->>TabPanel: Identify owning panel
useScrollToError->>setActiveTab: Select hidden field tab
setActiveTab->>TabPanel: Show selected panel
useScrollToError->>Browser: Scroll to first visible error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/hooks/__tests__/useScrollToError.test.js`:
- Line 71: Update VisibleHarness and its test cases for useScrollToError to
accept and pass a setActiveTab mock, then assert it was not called when the
field error is visible while retaining the existing scroll assertion.
In `@src/hooks/useScrollToError.js`:
- Around line 48-57: Update the error-element collection in useScrollToError so
hidden fields are excluded before sorting and selecting the scroll target, while
retaining visible fields and their existing document-position ordering. Add a
mixed-tab regression test covering one hidden and one visible error, asserting
the hook scrolls to the visible field without activating the hidden field’s tab.
- Line 112: Update the effect dependency array in useScrollToError to include
Formik errors or another deliberate trigger that changes whenever errors are
externally applied, while preserving the existing isSubmitting behavior. Add a
regression test covering errors injected after submission settles and verify
scrolling still reaches errors in inactive panels.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ac464da-40ea-4814-9bba-3dfd2cb67bec
📒 Files selected for processing (9)
src/components/forms/__tests__/selection-plan-form.test.jssrc/components/forms/selection-plan-form.jssrc/components/forms/selection-plan-form/email-templates-tab.jssrc/components/forms/selection-plan-form/main-tab.jssrc/components/forms/selection-plan-form/track-chair-settings-tab.jssrc/hooks/__tests__/useScrollToError.test.jssrc/hooks/useScrollToError.jssrc/pages/events/components/__tests__/event-type-dialog.test.jssrc/pages/events/components/event-type-dialog.js
| onSubmit: () => {} | ||
| }); | ||
|
|
||
| useScrollToError(formik, true, jest.fn()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that visible errors do not activate a tab.
VisibleHarness passes an unreachable jest.fn() to the hook. The test only asserts scrolling. It still passes if the hook incorrectly calls setActiveTab for a visible field.
Pass a mock through VisibleHarness and assert that it was not called.
Also applies to: 112-120
🤖 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/hooks/__tests__/useScrollToError.test.js` at line 71, Update
VisibleHarness and its test cases for useScrollToError to accept and pass a
setActiveTab mock, then assert it was not called when the field error is visible
while retaining the existing scroll assertion.
| 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); |
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, allMatchesHidden is false. scrollToFirstVisible then includes hidden fields. A hidden field has a zero layout rect and can sort before the visible field. The hook then calls scrollIntoView on the hidden field without activating its tab.
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
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/hooks/useScrollToError.js` around lines 48 - 57, Update the error-element
collection in useScrollToError so hidden fields are excluded before sorting and
selecting the scroll target, while retaining visible fields and their existing
document-position ordering. Add a mixed-tab regression test covering one hidden
and one visible error, asserting the hook scrolls to the visible field without
activating the hidden field’s tab.
|
|
||
| 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:
Formik 2.4.6 source setErrors implementation isSubmitting reducer SET_ERRORS
💡 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:
- 1: https://github.com/jaredpalmer/formik/blob/26c4f8627a5ecfd81ec2196c7a9687b3f39f2836/packages/formik/src/Formik.tsx
- 2: https://github.com/jaredpalmer/formik/blob/d4049f2da7e2e8811b498ea8013b77af063c6c8a/packages/formik/src/Formik.tsx
- 3: https://github.com/jaredpalmer/formik/blob/main/packages/formik/src/Formik.tsx
- 4: https://unpkg.com/browse/formik@2.4.6/dist/formik.cjs.production.min.js.map
- 5:
helpers.setErrorfromuseFieldcalled inside field asyncvalidatemethod brings inconsistent form state jaredpalmer/formik#3008 - 6: https://github.com/jaredpalmer/formik/blob/main/docs/api/formik.md
- 7: https://formik.org/docs/guides/form-submission
Re-run scrolling when Formik errors change.
Add errors (or another deliberate error trigger) to the effect dependencies. formik.setErrors updates errors without changing isSubmitting, so externally applied errors can remain hidden in inactive panels. Add a regression test for errors injected after submission settles.
🤖 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/hooks/useScrollToError.js` at line 112, Update the effect dependency
array in useScrollToError to include Formik errors or another deliberate trigger
that changes whenever errors are externally applied, while preserving the
existing isSubmitting behavior. Add a regression test covering errors injected
after submission settles and verify scrolling still reaches errors in inactive
panels.
1 participant
https://app.clickup.com/t/9014802374/86baqq2n5
Summary by CodeRabbit
Bug Fixes
Accessibility