fix: display expanded modules if they're collapsed and have errors#943
fix: display expanded modules if they're collapsed and have errors#943tomrndom wants to merge 1 commit into
Conversation
… submit Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
📝 WalkthroughWalkthroughThe form component now manages accordion expansion internally using controlled state instead of MUI's ChangesAccordion state and error-driven scroll
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🤖 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/pages/sponsors-global/page-templates/page-template-popup/page-template-modules-form.js`:
- Around line 50-70: The useEffect that scrolls to the first module error
(useEffect referencing submitCount, moduleErrors, modules, setCollapsedModules,
moduleRefMap, DEBOUNCE_WAIT_150) is missing dependencies and cleanup: include
moduleErrors and modules in the dependency array so the effect re-runs after
async validation completes, and store the setTimeout id so you can clearTimeout
on cleanup (return a cleanup function) to avoid queued scrolls across
submissions; ensure you still guard early with submitCount === 0 and
Array.isArray(moduleErrors) before computing errorIds.
🪄 Autofix (Beta)
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: eba33ab7-4185-4139-85f6-2820fdf52d9d
📒 Files selected for processing (2)
src/pages/sponsors-global/page-templates/page-template-popup/page-template-modules-form.jssrc/utils/constants.js
| useEffect(() => { | ||
| if (submitCount === 0 || !Array.isArray(moduleErrors)) return; | ||
|
|
||
| const errorIds = moduleErrors | ||
| .map((err, i) => err && getModuleId(modules[i])) | ||
| .filter(Boolean); | ||
|
|
||
| if (errorIds.length === 0) return; | ||
|
|
||
| setCollapsedModules((prev) => { | ||
| const next = new Set(prev); | ||
| errorIds.forEach((id) => next.delete(id)); | ||
| return next; | ||
| }); | ||
|
|
||
| setTimeout(() => { | ||
| moduleRefMap.current | ||
| .get(errorIds[0]) | ||
| ?.scrollIntoView({ behavior: "smooth" }); | ||
| }, DEBOUNCE_WAIT_150); | ||
| }, [submitCount]); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Formik 2.4.x submission lifecycle: does submitCountincrement before async validation updateserrors, and what dependency pattern is recommended for useEffectlogic that depends on bothsubmitCountanderrors?
💡 Result:
In Formik 2.4.x, the submitCount property increments immediately when handleSubmit or submitForm is called, before any asynchronous validation has completed [1]. Because validation is asynchronous, this creates a race condition where submitCount reflects a new submission attempt while the errors object still contains results from the previous state [2][3]. To handle logic that depends on both submitCount and errors (such as triggering side effects only after a failed submission attempt), the recommended pattern is to avoid relying solely on submitCount as a trigger, as it may be out of sync with the current errors [2][3]. Instead, use a combination of submitCount and isValidating or isSubmitting to ensure the form has finished processing [1]. A common and robust pattern is to use a useEffect that monitors submitCount but includes a check to ensure the form is no longer validating [1]: useEffect( => { if (submitCount > 0 &&!isValidating) { // Logic here will run only after validation has finished if (Object.keys(errors).length > 0) { // Handle submission errors } } }, [submitCount, isValidating, errors]); This approach ensures that your effect only executes once the validation cycle triggered by the submission attempt has fully resolved, preventing the use of stale error data [2][1][3].
Citations:
- 1: https://formik.org/docs/guides/form-submission
- 2: Increment
submitCountonly after validation has finished running jaredpalmer/formik#1726 - 3: Add Formik validated/submitted with errors handler jaredpalmer/formik#3048
🏁 Script executed:
cat -n src/pages/sponsors-global/page-templates/page-template-popup/page-template-modules-form.js | head -100Repository: fntechgit/summit-admin
Length of output: 4219
Add missing dependencies and cleanup to handle async validation timing in scroll-to-error effect.
The effect relies on moduleErrors and modules but only declares submitCount as a dependency. Because Formik's submitCount increments immediately before async validation completes, moduleErrors may be stale on the first run. Additionally, the setTimeout callback lacks cleanup, which can cause multiple scroll attempts to queue up across submissions.
Add moduleErrors and modules to the dependency array so the effect re-runs after validation completes with fresh error data. Also clean up the timeout on unmount or when dependencies change:
Suggested fix
useEffect(() => {
- if (submitCount === 0 || !Array.isArray(moduleErrors)) return;
+ if (submitCount === 0 || !Array.isArray(moduleErrors)) return;
const errorIds = moduleErrors
.map((err, i) => err && getModuleId(modules[i]))
.filter(Boolean);
if (errorIds.length === 0) return;
setCollapsedModules((prev) => {
const next = new Set(prev);
errorIds.forEach((id) => next.delete(id));
return next;
});
- setTimeout(() => {
+ const timerId = setTimeout(() => {
moduleRefMap.current
.get(errorIds[0])
?.scrollIntoView({ behavior: "smooth" });
}, DEBOUNCE_WAIT_150);
- }, [submitCount]);
+
+ return () => clearTimeout(timerId);
+ }, [submitCount, moduleErrors, modules]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (submitCount === 0 || !Array.isArray(moduleErrors)) return; | |
| const errorIds = moduleErrors | |
| .map((err, i) => err && getModuleId(modules[i])) | |
| .filter(Boolean); | |
| if (errorIds.length === 0) return; | |
| setCollapsedModules((prev) => { | |
| const next = new Set(prev); | |
| errorIds.forEach((id) => next.delete(id)); | |
| return next; | |
| }); | |
| setTimeout(() => { | |
| moduleRefMap.current | |
| .get(errorIds[0]) | |
| ?.scrollIntoView({ behavior: "smooth" }); | |
| }, DEBOUNCE_WAIT_150); | |
| }, [submitCount]); | |
| useEffect(() => { | |
| if (submitCount === 0 || !Array.isArray(moduleErrors)) return; | |
| const errorIds = moduleErrors | |
| .map((err, i) => err && getModuleId(modules[i])) | |
| .filter(Boolean); | |
| if (errorIds.length === 0) return; | |
| setCollapsedModules((prev) => { | |
| const next = new Set(prev); | |
| errorIds.forEach((id) => next.delete(id)); | |
| return next; | |
| }); | |
| const timerId = setTimeout(() => { | |
| moduleRefMap.current | |
| .get(errorIds[0]) | |
| ?.scrollIntoView({ behavior: "smooth" }); | |
| }, DEBOUNCE_WAIT_150); | |
| return () => clearTimeout(timerId); | |
| }, [submitCount, moduleErrors, modules]); |
🤖 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/pages/sponsors-global/page-templates/page-template-popup/page-template-modules-form.js`
around lines 50 - 70, The useEffect that scrolls to the first module error
(useEffect referencing submitCount, moduleErrors, modules, setCollapsedModules,
moduleRefMap, DEBOUNCE_WAIT_150) is missing dependencies and cleanup: include
moduleErrors and modules in the dependency array so the effect re-runs after
async validation completes, and store the setTimeout id so you can clearTimeout
on cleanup (return a cleanup function) to avoid queued scrolls across
submissions; ensure you still guard early with submitCount === 0 and
Array.isArray(moduleErrors) before computing errorIds.
1 participant
ref: https://app.clickup.com/t/86b9c31ne
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit