refactor(forms): lift VM disk lookup into SchemaForm + form-validation fixes#26
refactor(forms): lift VM disk lookup into SchemaForm + form-validation fixes#26Aleksei Sviridkin (lexfrei) wants to merge 3 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 42 minutes and 30 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 (16)
✨ 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
2 times, most recently
from
4e8da2a to
0ab84bc
Compare
VMDiskWidget called useK8sList and useTenantContext itself, so the disk lookup ran per field and the widget could not render without K8s/tenant providers. Move the lookup into a gated SchemaForm subcomponent that fetches vmdisks once (one-shot, no watch) only when the schema declares a disks[] array, and hand the list to the widget through RJSF formContext. The widget is now a pure controlled <select> that needs no providers, and schemas without disks never touch the K8s hook path. The validate() handle also fails closed now when the inner form ref is unbound. Add useOptionalTenantContext (non-throwing) for the gated lookup. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
…s it The widget reset the auto-default guard whenever the field was cleared, so on the next render the effect re-applied the cluster-default storage class — an optional storageClass could not be left empty and a value the user removed could be silently resubmitted. Disable the auto-default on any user interaction instead, so the default is only ever applied once, before the user touches the field. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
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
0ab84bc to
3b38d70
Compare
1 participant
What
Follow-up to the VM-disk-selection fix. Adopts the cleaner data-fetching architecture for the disk dropdown and addresses the form-validation review feedback that surfaced on that change.
Changes
SchemaForm.VMDiskWidgetno longer callsuseK8sList/useTenantContextitself. A gatedSchemaFormsubcomponent fetchesvmdisksonce (one-shot, no watch) only when the schema declares adisks[]array, and hands the list to the widget through RJSFformContext. The widget is now a pure controlled<select>that needs no providers, and schemas without disks never touch the K8s hook path. Adds a non-throwinguseOptionalTenantContext.SchemaForm.validate()fails closed when the inner RJSF form ref is unbound — it gates submit, so a missing ref should block rather than pass.StorageClassWidget: keep an optional storage class empty after the user clears it. The widget reset its auto-default guard on clear, so the cluster default was immediately reapplied and an optionalstorageClasscould not be left empty. The auto-default now fires only once, before the user touches the field.validate()before the page-level alerts on the backup create pages, so a schema-required field renders RJSF inline errors instead of being masked by a manualalert.Testing
VMDiskWidgettests now drive options viaformContext(no K8s provider needed); a newSchemaFormtest exercises the gated disk fetch end-to-end.StorageClassWidgettest pins that a cleared optional field stays empty.pnpm typecheck,pnpm test(250 tests), andpnpm buildpass.