feat(Form): automatically focus the first field with an error upon submission failure#6547
Open
rdjanuar wants to merge 5 commits into
Open
feat(Form): automatically focus the first field with an error upon submission failure#6547rdjanuar wants to merge 5 commits into
rdjanuar wants to merge 5 commits into
Conversation
added 2 commits
June 3, 2026 21:15
…eld with an error upon submission failure
…nstead of auto-focusing the element
rdjanuar
changed the title
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a focusOnError prop (default true) to Form.vue and imports a new scrollToErrorEl(errors) utility that finds the first error by id and focuses its DOM element. On submit validation failure the form calls scrollToErrorEl after nextTick when focusOnError is enabled. Documentation and the example were updated to reflect the behavior and show a toast on error; a test verifies only the first error element is focused. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/runtime/utils/form.ts (1)
118-118: 💤 Low valueName doesn't match behavior.
scrollToErrorElonly callsfocus(); considerfocusFirstErrorEl(or similar) to avoid implying explicit scrolling. Browsers may scroll on focus, but the name overstates the intent.🤖 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/runtime/utils/form.ts` at line 118, The function name scrollToErrorEl (exported function scrollToErrorEl) is misleading because it only calls focus() and does not explicitly scroll; rename it to focusFirstErrorEl (or another name indicating focus behavior) and update the function declaration, its export, and all call sites to use the new name, or alternatively implement explicit scrolling behavior (e.g., element.scrollIntoView) inside scrollToErrorEl if you want to keep the current name—choose one approach and make the function, its export, and all references consistent.
🤖 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/runtime/components/Form.vue`:
- Around line 63-66: Update the JSDoc for the focusOnError prop in Form.vue to
fix the grammar and add an `@defaultValue` tag; change the description to
something like "When true, the form will automatically focus the first form
element with an error" and add "`@defaultValue` false" to document the default
(ensure the runtime default remains controlled via withDefaults() where the prop
is declared). Reference the focusOnError prop to locate the comment and adjust
only the JSDoc text.
In `@src/runtime/utils/form.ts`:
- Around line 118-130: The function scrollToErrorEl currently declares a local
timeOut that never persists (making the clearTimeout branch dead) and
force-asserts error.id; change it to either use a module-scoped timeout variable
(e.g., move timeOut outside the scrollToErrorEl function) if you want
debounce/cancel behavior, or remove the clearTimeout and call setTimeout
directly; additionally, guard the id on the first error (check error &&
error.id) before calling document.getElementById(error.id) to avoid asserting
away the optional id. Ensure references to the local variable name timeOut and
the function scrollToErrorEl are updated accordingly.
---
Nitpick comments:
In `@src/runtime/utils/form.ts`:
- Line 118: The function name scrollToErrorEl (exported function
scrollToErrorEl) is misleading because it only calls focus() and does not
explicitly scroll; rename it to focusFirstErrorEl (or another name indicating
focus behavior) and update the function declaration, its export, and all call
sites to use the new name, or alternatively implement explicit scrolling
behavior (e.g., element.scrollIntoView) inside scrollToErrorEl if you want to
keep the current name—choose one approach and make the function, its export, and
all references consistent.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c398736b-2c79-4244-846f-433703c2c179
📒 Files selected for processing (5)
docs/app/components/content/examples/form/FormExampleOnError.vuedocs/content/docs/2.components/form.mdsrc/runtime/components/Form.vuesrc/runtime/utils/form.tstest/components/Form.spec.ts
commit: |
OrbisK
reviewed
Jun 3, 2026
Comment on lines
+125
to
+127
| timeOut = setTimeout(() => { | ||
| el.focus() | ||
| }, 0) |
Member
There was a problem hiding this comment.
Why cant we use nextTick() instead?
Contributor
Author
There was a problem hiding this comment.
it's because when we submit the Form the field become disabled. i think i can use nextTick outside the try & catch block on function onSubmitWrapper and remove the setTimeout.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/runtime/utils/form.ts (1)
120-122:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard optional
idinstead of force-asserting it.
FormErrorWithId.idis optional, soerror.id!still bypasses the declared contract (Line 121). Please short-circuit when the first error has noid.Suggested fix
export function scrollToErrorEl(errors: FormErrorWithId[]) { const error = errors[0] - if (error) { - const el = document.getElementById(error.id!) - if (el) { - el.focus() - } - } + if (!error?.id) return + const el = document.getElementById(error.id) + if (el) { + el.focus() + } }🤖 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/runtime/utils/form.ts` around lines 120 - 122, The code force-asserts error.id (error.id!) even though FormErrorWithId.id is optional; update the logic in the function handling form errors so it short-circuits if the first error has no id (e.g., check if error.id is falsy before calling document.getElementById), and only call document.getElementById(error.id) and proceed to use el when error.id is present; locate the block using the variable error and the FormErrorWithId type to apply this guard.
🤖 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.
Duplicate comments:
In `@src/runtime/utils/form.ts`:
- Around line 120-122: The code force-asserts error.id (error.id!) even though
FormErrorWithId.id is optional; update the logic in the function handling form
errors so it short-circuits if the first error has no id (e.g., check if
error.id is falsy before calling document.getElementById), and only call
document.getElementById(error.id) and proceed to use el when error.id is
present; locate the block using the variable error and the FormErrorWithId type
to apply this guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82ad0129-95fb-463d-a303-3dbc8074095b
📒 Files selected for processing (2)
src/runtime/components/Form.vuesrc/runtime/utils/form.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/runtime/components/Form.vue
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
🔗 Linked issue
Resolves #4612
❓ Type of change
📚 Description
This PR just a proposal that match with form behavior base on
https://www.w3.org/WAI/WCAG22/Understanding/error-identification.html
in client-validation form when
fieldiserrorthe browser automaticly focus to the firstfield. currentlyNuxtUIdoesn't do that we need to manage by it self using@errorprop. this PR make theFormwill automatically focus to the error field📝 Checklist