refactor(forms): validate the schema form before page-level checks#26
refactor(forms): validate the schema form before page-level checks#26Aleksei Sviridkin (lexfrei) wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 20 minutes and 49 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 (8)
✨ 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/L
![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 refactors SchemaForm to conditionally fetch VMDisks only when a disks array is present in the schema, passing the options down to VMDiskWidget via formContext. It also updates StorageClassWidget to prevent auto-defaulting from re-triggering after a user clears the field, introduces an opportunistic useOptionalTenantContext hook, and runs form validation earlier in backup creation pages. The review feedback suggests enhancing schemaHasDiskField to recursively traverse array items for nested disks fields, and initializing hasAutoDefaulted in StorageClassWidget based on the initial value on mount to better support pre-existing values.
| function schemaHasDiskField(schema: unknown): boolean { | ||
| if (!schema || typeof schema !== "object") return false | ||
| const properties = (schema as { properties?: Record<string, unknown> }).properties | ||
| if (!properties || typeof properties !== "object") return false | ||
| for (const [key, value] of Object.entries(properties)) { | ||
| if (key === "disks" && isDiskArraySchema(value)) return true | ||
| if (value && typeof value === "object" && (value as { properties?: unknown }).properties) { | ||
| if (schemaHasDiskField(value)) return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
The current implementation of schemaHasDiskField only traverses nested objects if they have a properties field. It does not traverse into array items (which contain object schemas with their own properties). If a schema defines a disks field nested inside an array of objects, this function will fail to detect it.
We can make the traversal more robust by checking both properties and items recursively.
function schemaHasDiskField(schema: unknown): boolean {
if (!schema || typeof schema !== "object") return false
const items = (schema as { items?: unknown }).items
if (items && typeof items === "object") {
if (schemaHasDiskField(items)) return true
}
const properties = (schema as { properties?: Record<string, unknown> }).properties
if (!properties || typeof properties !== "object") return false
for (const [key, value] of Object.entries(properties)) {
if (key === "disks" && isDiskArraySchema(value)) return true
if (value && typeof value === "object") {
if (schemaHasDiskField(value)) return true
}
}
return false
}
There was a problem hiding this comment.
Thanks — this traversal is intentionally identical to the widget binder addVMDiskWidgets, which also recurses only through properties. schemaHasDiskField is the fetch gate and has to match the binder exactly; recursing the gate into array items without doing the same in the binder would reintroduce the gate/binder divergence this PR closes (disks detected for fetch but never bound to VMDiskWidget, or vice-versa). There's a test pinning that parity ("gate matches the widget binder"). No shipped VM openAPISchema nests disks inside an array of objects — it lives at a fixed top-level disks[] path — so the deeper recursion isn't needed today. If a chart ever requires it, I'd add items recursion to the gate and the binder together so they stay in lockstep.
| onChange={(e) => { | ||
| if (!e.target.value) hasAutoDefaulted.current = false | ||
| // Any user interaction (including clearing) permanently disables the | ||
| // auto-default, so an optional field the user clears is not immediately | ||
| // refilled with the cluster default on the next render. | ||
| hasAutoDefaulted.current = true | ||
| onChange(e.target.value || undefined) | ||
| }} |
There was a problem hiding this comment.
While setting hasAutoDefaulted.current = true on user interaction prevents the auto-default from re-triggering when the user clears the field, it does not handle cases where the field is cleared programmatically or if the component is remounted with a pre-existing value.
To make this more robust, consider initializing hasAutoDefaulted based on whether a value is already present on mount (e.g., const hasAutoDefaulted = useRef(value !== undefined && value !== "") on line 37). This ensures that pre-existing values (including explicitly saved empty values in edit flows) are respected and not overwritten by the cluster default.
There was a problem hiding this comment.
Both cases are already handled. A remount with a pre-existing value doesn't auto-default: the effect is guarded by !value, so it never fires when a value is present (covered by the test "does not snap back to the default after clearing a value loaded from formData"). In the controlled RJSF form the value only ever changes through this widget's own onChange, which sets hasAutoDefaulted.current = true, so there's no programmatic-clear path that bypasses the guard. Also, useRef(value !== undefined && value !== "") evaluates to false for an explicitly-empty "", so it wouldn't preserve a saved-empty value the way the suggestion intends. Keeping the current logic.
Aleksei Sviridkin (lexfrei)
force-pushed
the
refactor/schemaform-vmdisk-lookup
branch
from
69c48cf to
622e603
Compare
github-actions
Bot
added
size/XL
Aleksei Sviridkin (lexfrei)
force-pushed
the
refactor/schemaform-vmdisk-lookup
branch
3 times, most recently
from
0ab84bc to
3b38d70
Compare
The backup create pages ran SchemaForm.validate() only after their manual required-field alerts, so a schema-required field left empty exited via an alert without RJSF ever rendering its inline errors. Run validate() right after the name check, before the page-level alerts, so inline errors show consistently and the alerts only cover page-only requirements. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Aleksei Sviridkin (lexfrei)
force-pushed
the
refactor/schemaform-vmdisk-lookup
branch
from
3b38d70 to
2ac2b94
Compare
github-actions
Bot
added
size/L
Aleksei Sviridkin (lexfrei)
changed the title
|
Reworked: rebased onto main and reduced scope to the validation-ordering change. The other two parts — lifting VM disk lookup into |
1 participant
What
In the four backup create pages (Backup, BackupJob, BackupPlan, BackupRestoreJob), run the RJSF
SchemaForm.validate()call right after the name check and before the page-level required-field alerts.Why
The submit button lives outside RJSF and bypasses its validation, so each page triggers
validate()explicitly. It previously ran only after the manual page-levelalert(...)checks, so a schema-required field left empty exited via an alert without RJSF ever rendering its inline errors. Validating first makes inline errors show consistently; the page-level alerts then cover only page-only requirements (application/backup reference, strategy, timestamp).Scope
This PR originally also lifted the VM disk lookup into
SchemaFormand fixed an optional StorageClass not staying cleared. Both are now obsolete: theDynamicOptionsWidgetrefactor (#30) removedVMDiskWidgetandStorageClassWidget, moved option-fetching into a genericx-cozystack-optionsmechanism, and already latches the auto-default so a cleared optional field sticks. The PR is reduced to the validation-ordering change, which is still missing onmain.