fix(forms): stop VM disk selection being dropped on deploy#25
Conversation
|
Warning Review limit reached
More reviews will be available in 53 minutes and 40 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (20)
📝 WalkthroughWalkthroughThis PR introduces an imperative validation API to ChangesCore SchemaForm Validation Infrastructure
Widget Placeholder & Selection Normalization
Page Submission Validation Gates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 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 |
github-actions
Bot
added
size/XXL
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces explicit validation gating across several form pages (such as ApplicationOrderPage, BackupCreatePage, BackupJobCreatePage, BackupPlanCreatePage, BackupResourceEditPage, and BackupRestoreJobCreatePage) by exposing a validate method on SchemaForm via forwardRef and useImperativeHandle. It also refactors several widgets (BackupClassWidget, StorageClassWidget, and VMDiskWidget) to always render explicit placeholders, prevent accidental auto-selection of the first item, and keep committed values visible during loading. Comprehensive unit tests have been added for these widgets and form pages. Regarding the feedback, a critical bug was identified in StorageClassWidget.tsx where resetting hasAutoDefaulted.current to false when clearing the selection triggers the auto-select effect again, making it impossible to clear the field. It is recommended to set hasAutoDefaulted.current = true on any user interaction to permanently disable automatic defaulting.
| onChange={(e) => { | ||
| if (!e.target.value) hasAutoDefaulted.current = false | ||
| onChange(e.target.value || undefined) | ||
| }} |
There was a problem hiding this comment.
There is a bug in the onChange handler where hasAutoDefaulted.current is reset to false when the user clears the selection (i.e., selects -- None --).
Because hasAutoDefaulted.current is reset to false and the value becomes undefined, the useEffect hook's auto-select condition is met again on the next render:
!hasAutoDefaulted.current && !value && defaultSC && !isLoading
This causes the widget to immediately re-select the default storage class, making it impossible for the user to ever clear the field or select -- None --.
Additionally, if the form is in edit mode with an existing storage class, hasAutoDefaulted.current starts as false. If the user clears the field, the same auto-select effect will trigger and force-select the default storage class.
To fix both issues, we should set hasAutoDefaulted.current = true on any user interaction (including clearing the field) to permanently disable any automatic defaulting.
| onChange={(e) => { | |
| if (!e.target.value) hasAutoDefaulted.current = false | |
| onChange(e.target.value || undefined) | |
| }} | |
| onChange={(e) => { | |
| hasAutoDefaulted.current = true | |
| onChange(e.target.value || undefined) | |
| }} |
A required disk select rendered no empty <option>, so a value-less control visually showed the first disk while the form value stayed empty; the value was then committed from a post-load effect, and a fast submit on a cold cache raced that effect and produced spec.disks: []. Render an explicit placeholder option even when required, keep the committed value visible as a stable option across async re-renders, and drop the effect-based auto-select so the value is only ever set by an explicit pick. The drop is no longer timing-dependent. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
…option The required storage-class select rendered no empty <option>, so a value-less control visually showed the first class while the form value stayed empty, and a committed value could be dropped on an async re-render of the class list. Render an explicit placeholder even when required and keep the committed value visible as a stable option. The cluster-default auto-select is kept — a default class is a real default, unlike an arbitrary first item. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
…required The empty <option> was only rendered when the field was optional, so a required backup-class select with no value would visually show the first class while the form value stayed empty. Render the placeholder regardless of required, matching the disk and storage-class selects. The stable committed-value option is unchanged. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
The application deploy page and the five backup create/edit pages render SchemaForm with the submit button outside RJSF, so the Deploy/Create/Save handlers sent the spec to the API without ever running RJSF validation. A required field left empty — e.g. a disk row whose name was never picked — was submitted and silently produced an invalid or empty result. Expose a validate() handle on SchemaForm (delegating to RJSF's validateForm) and call it from every one of those submit handlers; an invalid form renders its errors inline and the submit is aborted. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Aleksei Sviridkin (lexfrei)
force-pushed
the
fix/vminstance-disk-select-race
branch
from
7284b55 to
3803066
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/console/src/components/StorageClassWidget.tsx (1)
38-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't re-arm the auto-default after a manual clear.
Line 57 sets
hasAutoDefaulted.currentback tofalse, so if a default storage class exists, clearing the field causes the effect on Lines 38-43 to immediately write that default back on the next render. That makes optionalstorageClassimpossible to keep empty and can silently submit a value the user explicitly removed.Suggested fix
onChange={(e) => { - if (!e.target.value) hasAutoDefaulted.current = false onChange(e.target.value || undefined) }}Also applies to: 56-58
🤖 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 `@apps/console/src/components/StorageClassWidget.tsx` around lines 38 - 43, The bug is that hasAutoDefaulted.current is being reset (lines 56-58) so clearing the field re-triggers the effect on next render and re-applies the default storage class; stop re-arming the auto-default. Remove or guard the code that sets hasAutoDefaulted.current = false (the clear handler/reset logic) so it is not flipped when the user manually clears the input; instead only initialize/clear that ref when the defaultSC changes or on mount. Update references in the component where hasAutoDefaulted is modified (the useEffect that auto-applies defaultSC, any clear/handler that sets it to false) so manual clears do not reset the flag while preserving the original auto-apply behavior when defaultSC first appears.apps/console/src/routes/BackupCreatePage.tsx (1)
77-100:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun
SchemaForm.validate()before the page-level spec alerts.The new gate is currently downstream of the manual
applicationRef/strategyRef/takenAtchecks, so those invalid form states still exit viaalert(...)without ever rendering RJSF inline errors. That means form-mode submission is not consistently gated by the new validation path this PR is introducing.Suggested change
const handleSubmit = async () => { if (!name.trim()) { alert("Name is required") return } + if (!schemaFormRef.current?.validate()) return + if (!formData.applicationRef?.kind || !formData.applicationRef?.name) { alert("Application reference is required") return } if (!formData.strategyRef?.kind || !formData.strategyRef?.name) { alert("Strategy reference is required") return } if (!formData.takenAt) { alert("Taken at timestamp is required") return } - - // The submit button lives outside RJSF and bypasses its validation, so - // trigger it explicitly; an invalid spec renders errors inline and aborts. - if (schemaFormRef.current && !schemaFormRef.current.validate()) return🤖 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 `@apps/console/src/routes/BackupCreatePage.tsx` around lines 77 - 100, In handleSubmit, run the RJSF validation via schemaFormRef.current.validate() before performing the page-level alerts so RJSF inline errors are rendered for invalid spec state; specifically, in the handleSubmit function call schemaFormRef.current?.validate() (and bail if it returns false) at the top of the function (or at least before the applicationRef/strategyRef/takenAt alert checks) so the schema form's validation gate runs first and prevents the manual alert early-exits.
🤖 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 `@apps/console/src/components/SchemaForm.tsx`:
- Around line 289-292: The useImperativeHandle implementation for validate
currently returns true when formRef.current is null; update the handler in
useImperativeHandle so that validate calls formRef.current?.validateForm() and,
if the inner ref is missing, returns false (fail closed) instead of true; locate
the useImperativeHandle invocation that references formRef and the validate
method and change the fallback from true to false so callers treat a missing
RJSF ref as invalid.
---
Outside diff comments:
In `@apps/console/src/components/StorageClassWidget.tsx`:
- Around line 38-43: The bug is that hasAutoDefaulted.current is being reset
(lines 56-58) so clearing the field re-triggers the effect on next render and
re-applies the default storage class; stop re-arming the auto-default. Remove or
guard the code that sets hasAutoDefaulted.current = false (the clear
handler/reset logic) so it is not flipped when the user manually clears the
input; instead only initialize/clear that ref when the defaultSC changes or on
mount. Update references in the component where hasAutoDefaulted is modified
(the useEffect that auto-applies defaultSC, any clear/handler that sets it to
false) so manual clears do not reset the flag while preserving the original
auto-apply behavior when defaultSC first appears.
In `@apps/console/src/routes/BackupCreatePage.tsx`:
- Around line 77-100: In handleSubmit, run the RJSF validation via
schemaFormRef.current.validate() before performing the page-level alerts so RJSF
inline errors are rendered for invalid spec state; specifically, in the
handleSubmit function call schemaFormRef.current?.validate() (and bail if it
returns false) at the top of the function (or at least before the
applicationRef/strategyRef/takenAt alert checks) so the schema form's validation
gate runs first and prevents the manual alert early-exits.
🪄 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: 1d56ac85-6981-4daf-a584-8e3916421481
📒 Files selected for processing (20)
apps/console/src/components/BackupClassWidget.test.tsxapps/console/src/components/BackupClassWidget.tsxapps/console/src/components/SchemaForm.test.tsxapps/console/src/components/SchemaForm.tsxapps/console/src/components/StorageClassWidget.test.tsxapps/console/src/components/StorageClassWidget.tsxapps/console/src/components/VMDiskWidget.test.tsxapps/console/src/components/VMDiskWidget.tsxapps/console/src/routes/ApplicationOrderPage.test.tsxapps/console/src/routes/ApplicationOrderPage.tsxapps/console/src/routes/BackupCreatePage.test.tsxapps/console/src/routes/BackupCreatePage.tsxapps/console/src/routes/BackupJobCreatePage.test.tsxapps/console/src/routes/BackupJobCreatePage.tsxapps/console/src/routes/BackupPlanCreatePage.test.tsxapps/console/src/routes/BackupPlanCreatePage.tsxapps/console/src/routes/BackupResourceEditPage.test.tsxapps/console/src/routes/BackupResourceEditPage.tsxapps/console/src/routes/BackupRestoreJobCreatePage.test.tsxapps/console/src/routes/BackupRestoreJobCreatePage.tsx
| useImperativeHandle( | ||
| ref, | ||
| () => ({ validate: () => formRef.current?.validateForm() ?? true }), | ||
| [], |
There was a problem hiding this comment.
Make validate() fail closed when the RJSF ref is missing.
This currently returns true on formRef.current === null, which means every caller will treat the form as valid and still submit if the inner ref ever fails to bind. Since this handle is now the submit gate, the safer fallback is false.
Suggested fix
useImperativeHandle(
ref,
- () => ({ validate: () => formRef.current?.validateForm() ?? true }),
+ () => ({ validate: () => formRef.current?.validateForm() ?? false }),
[],
)🤖 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 `@apps/console/src/components/SchemaForm.tsx` around lines 289 - 292, The
useImperativeHandle implementation for validate currently returns true when
formRef.current is null; update the handler in useImperativeHandle so that
validate calls formRef.current?.validateForm() and, if the inner ref is missing,
returns false (fail closed) instead of true; locate the useImperativeHandle
invocation that references formRef and the validate method and change the
fallback from true to false so callers treat a missing RJSF ref as invalid.
VerdictLGTM with non-blocking notes. The two interacting defects are addressed at the right layer (widget-level placeholder + stable-option, page-level submit gate via Findings[MINOR]
[MINOR] The PR justifies keeping it ("a default storage class is a real, meaningful default worth pre-filling") and the new submit gate plus disabled placeholder neutralise it for required fields. The residual exposure is a non-required Claim mismatches[PARTIAL] "pnpm typecheck and pnpm test (235 tests) pass." — Actual on PR head: 248 tests pass across 33 files. The fix direction is correct; the number in the PR body is stale. Cosmetic, no action needed beyond optionally refreshing the description. Red-team concerns
Recommended follow-ups
|
2 participants
What
On the "Deploy new instance" form, a disk shown as selected in the
disksdropdown was intermittently dropped on create — the resulting VMInstance ended up withspec.disks: []. The loss was entirely client-side, before submit.Two interacting defects in
VMDiskWidgetcaused it:requireddisk<select>rendered no empty<option>, so a value-less control visually displayed the first disk while the form value stayed empty — the UI showed a disk "selected" that the form data did not hold.useEffect; on a cold cache a fast Deploy click raced that effect and submitted a disk row with no name.Changes
VMDiskWidget: drop the effect-based auto-select so the value is only ever set by an explicit pick (no timing-dependent drop); always render an explicit placeholder, disabled when the field is required; keep the committed value visible as a stable option across async re-renders of the disk list.StorageClassWidgetandBackupClassWidget, the sibling list-fed selects with the same latent issue.StorageClassWidgetkeeps its cluster-default auto-select — a real default, unlike an arbitrary first item.SchemaFormnow exposes avalidate()handle delegating to RJSF'svalidateForm, and all six form pages gate their submit on it so an invalid spec can no longer reach the API. YAML mode is left for the API to reject.Testing
SchemaForm.validate()handle, and the submit gate on each of the six consumer pages (an invalid form must not POST/PUT; a valid one must).pnpm typecheckandpnpm test(235 tests) pass.Summary by CodeRabbit
New Features
Bug Fixes
Tests