refactor(dashboard): validate the schema form before page-level checks - #3116
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the validation logic across four backup-related creation pages to improve the user experience. By prioritizing RJSF schema validation over manual page-level checks, the application now ensures that inline form errors are rendered correctly before any blocking alerts are triggered. This change aligns the console's behavior with expected form validation patterns and includes updated regression tests to maintain stability. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughThe backup create pages now run SchemaForm validation before page-level required-field alerts. Matching tests control emitted specs and verify that failed validation blocks submission and alerts. ChangesBackup submit validation order
Estimated code review effort: 2 (Simple) | ~15 minutes Merge Risk: ⚪ Minimal · up to This localized change makes schema validation run before page-specific checks so inline errors appear consistently; no actionable merge-blocking risk remains after normal checks. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
There was a problem hiding this comment.
Code Review
This pull request modifies the backup-related creation pages (BackupCreatePage, BackupJobCreatePage, BackupPlanCreatePage, and BackupRestoreJobCreatePage) to execute RJSF validation before page-level required-field checks, ensuring inline schema errors are not masked by alerts. It also adds corresponding unit tests. The review feedback suggests using optional chaining (schemaFormRef.current?.validate()) to ensure a safer fail-closed behavior, and wrapping test assertions in try...finally blocks to guarantee mock restoration and prevent test leakage.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| // Run RJSF validation before the page-level checks so schema-required | ||
| // fields render inline errors instead of being masked by the alerts below. | ||
| if (schemaFormRef.current && !schemaFormRef.current.validate()) return |
There was a problem hiding this comment.
Using optional chaining schemaFormRef.current?.validate() is more idiomatic in TypeScript and safer because it fails closed. If schemaFormRef.current is unexpectedly null or undefined, !schemaFormRef.current?.validate() will evaluate to true and return early, preventing an invalid form submission. The current implementation schemaFormRef.current && !schemaFormRef.current.validate() would evaluate to false and proceed to submission, failing open.
| if (schemaFormRef.current && !schemaFormRef.current.validate()) return | |
| if (!schemaFormRef.current?.validate()) return |
|
|
||
| // Run RJSF validation before the page-level checks so schema-required | ||
| // fields render inline errors instead of being masked by the alerts below. | ||
| if (schemaFormRef.current && !schemaFormRef.current.validate()) return |
There was a problem hiding this comment.
Using optional chaining schemaFormRef.current?.validate() is more idiomatic in TypeScript and safer because it fails closed. If schemaFormRef.current is unexpectedly null or undefined, !schemaFormRef.current?.validate() will evaluate to true and return early, preventing an invalid form submission. The current implementation schemaFormRef.current && !schemaFormRef.current.validate() would evaluate to false and proceed to submission, failing open.
| if (schemaFormRef.current && !schemaFormRef.current.validate()) return | |
| if (!schemaFormRef.current?.validate()) return |
|
|
||
| // Run RJSF validation before the page-level checks so schema-required | ||
| // fields render inline errors instead of being masked by the alerts below. | ||
| if (schemaFormRef.current && !schemaFormRef.current.validate()) return |
There was a problem hiding this comment.
Using optional chaining schemaFormRef.current?.validate() is more idiomatic in TypeScript and safer because it fails closed. If schemaFormRef.current is unexpectedly null or undefined, !schemaFormRef.current?.validate() will evaluate to true and return early, preventing an invalid form submission. The current implementation schemaFormRef.current && !schemaFormRef.current.validate() would evaluate to false and proceed to submission, failing open.
| if (schemaFormRef.current && !schemaFormRef.current.validate()) return | |
| if (!schemaFormRef.current?.validate()) return |
|
|
||
| // Run RJSF validation before the page-level checks so schema-required | ||
| // fields render inline errors instead of being masked by the alerts below. | ||
| if (schemaFormRef.current && !schemaFormRef.current.validate()) return |
There was a problem hiding this comment.
Using optional chaining schemaFormRef.current?.validate() is more idiomatic in TypeScript and safer because it fails closed. If schemaFormRef.current is unexpectedly null or undefined, !schemaFormRef.current?.validate() will evaluate to true and return early, preventing an invalid form submission. The current implementation schemaFormRef.current && !schemaFormRef.current.validate() would evaluate to false and proceed to submission, failing open.
| if (schemaFormRef.current && !schemaFormRef.current.validate()) return | |
| if (!schemaFormRef.current?.validate()) return |
| it("runs RJSF validation before the page-level required-field alerts", async () => { | ||
| // Form is RJSF-invalid AND a page-required field is missing. The gate must | ||
| // fire first, so no alert() is shown — proving validate() runs before the | ||
| // manual checks (under the old ordering the applicationRef alert fired). | ||
| h.validateReturn = false | ||
| h.emitSpec = {} | ||
| const alertSpy = vi.spyOn(window, "alert").mockImplementation(() => {}) | ||
| const user = userEvent.setup() | ||
| renderPage() | ||
|
|
||
| await user.type(screen.getByRole("textbox"), "my-backup") | ||
| await user.click(screen.getByRole("button", { name: /create/i })) | ||
|
|
||
| expect(h.createMutateAsync).not.toHaveBeenCalled() | ||
| expect(alertSpy).not.toHaveBeenCalled() | ||
| alertSpy.mockRestore() | ||
| }) |
There was a problem hiding this comment.
Wrapping the test assertions in a try...finally block ensures that alertSpy.mockRestore() is always called, even if an assertion fails. This prevents mock leakage to other tests in the suite.
it("runs RJSF validation before the page-level required-field alerts", async () => {
// Form is RJSF-invalid AND a page-required field is missing. The gate must
// fire first, so no alert() is shown — proving validate() runs before the
// manual checks (under the old ordering the applicationRef alert fired).
h.validateReturn = false
h.emitSpec = {}
const alertSpy = vi.spyOn(window, "alert").mockImplementation(() => {})
try {
const user = userEvent.setup()
renderPage()
await user.type(screen.getByRole("textbox"), "my-backup")
await user.click(screen.getByRole("button", { name: /create/i }))
expect(h.createMutateAsync).not.toHaveBeenCalled()
expect(alertSpy).not.toHaveBeenCalled()
} finally {
alertSpy.mockRestore()
}
})
| it("runs RJSF validation before the page-level required-field alerts", async () => { | ||
| h.validateReturn = false | ||
| h.emitSpec = {} | ||
| const alertSpy = vi.spyOn(window, "alert").mockImplementation(() => {}) | ||
| const user = userEvent.setup() | ||
| renderPage() | ||
|
|
||
| await user.type(screen.getByRole("textbox"), "my-job") | ||
| await user.click(screen.getByRole("button", { name: /create/i })) | ||
|
|
||
| expect(h.createMutateAsync).not.toHaveBeenCalled() | ||
| expect(alertSpy).not.toHaveBeenCalled() | ||
| alertSpy.mockRestore() | ||
| }) |
There was a problem hiding this comment.
Wrapping the test assertions in a try...finally block ensures that alertSpy.mockRestore() is always called, even if an assertion fails. This prevents mock leakage to other tests in the suite.
it("runs RJSF validation before the page-level required-field alerts", async () => {
h.validateReturn = false
h.emitSpec = {}
const alertSpy = vi.spyOn(window, "alert").mockImplementation(() => {})
try {
const user = userEvent.setup()
renderPage()
await user.type(screen.getByRole("textbox"), "my-job")
await user.click(screen.getByRole("button", { name: /create/i }))
expect(h.createMutateAsync).not.toHaveBeenCalled()
expect(alertSpy).not.toHaveBeenCalled()
} finally {
alertSpy.mockRestore()
}
})
| it("runs RJSF validation before the page-level required-field alerts", async () => { | ||
| h.validateReturn = false | ||
| h.emitSpec = {} | ||
| const alertSpy = vi.spyOn(window, "alert").mockImplementation(() => {}) | ||
| const user = userEvent.setup() | ||
| renderPage() | ||
|
|
||
| await user.type(screen.getByRole("textbox"), "my-plan") | ||
| await user.click(screen.getByRole("button", { name: /create/i })) | ||
|
|
||
| expect(h.createMutateAsync).not.toHaveBeenCalled() | ||
| expect(alertSpy).not.toHaveBeenCalled() | ||
| alertSpy.mockRestore() | ||
| }) |
There was a problem hiding this comment.
Wrapping the test assertions in a try...finally block ensures that alertSpy.mockRestore() is always called, even if an assertion fails. This prevents mock leakage to other tests in the suite.
it("runs RJSF validation before the page-level required-field alerts", async () => {
h.validateReturn = false
h.emitSpec = {}
const alertSpy = vi.spyOn(window, "alert").mockImplementation(() => {})
try {
const user = userEvent.setup()
renderPage()
await user.type(screen.getByRole("textbox"), "my-plan")
await user.click(screen.getByRole("button", { name: /create/i }))
expect(h.createMutateAsync).not.toHaveBeenCalled()
expect(alertSpy).not.toHaveBeenCalled()
} finally {
alertSpy.mockRestore()
}
})
| it("runs RJSF validation before the page-level required-field alerts", async () => { | ||
| h.validateReturn = false | ||
| h.emitSpec = {} | ||
| const alertSpy = vi.spyOn(window, "alert").mockImplementation(() => {}) | ||
| const user = userEvent.setup() | ||
| renderPage() | ||
|
|
||
| await user.type(screen.getByRole("textbox"), "my-restore") | ||
| await user.click(screen.getByRole("button", { name: /create/i })) | ||
|
|
||
| expect(h.createMutateAsync).not.toHaveBeenCalled() | ||
| expect(alertSpy).not.toHaveBeenCalled() | ||
| alertSpy.mockRestore() | ||
| }) |
There was a problem hiding this comment.
Wrapping the test assertions in a try...finally block ensures that alertSpy.mockRestore() is always called, even if an assertion fails. This prevents mock leakage to other tests in the suite.
it("runs RJSF validation before the page-level required-field alerts", async () => {
h.validateReturn = false
h.emitSpec = {}
const alertSpy = vi.spyOn(window, "alert").mockImplementation(() => {})
try {
const user = userEvent.setup()
renderPage()
await user.type(screen.getByRole("textbox"), "my-restore")
await user.click(screen.getByRole("button", { name: /create/i }))
expect(h.createMutateAsync).not.toHaveBeenCalled()
expect(alertSpy).not.toHaveBeenCalled()
} finally {
alertSpy.mockRestore()
}
})
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — behavior-preserving reorder; validation is moved, not dropped, and submission stays gated.
Verified across all four pages (Backup, BackupJob, BackupPlan, BackupRestoreJob): the single schemaFormRef.current.validate() call is relocated above the page-level required-field checks rather than removed. Every guard still early-returns on failure, so mutateAsync is reached only when all checks pass — the gating outcome (submit blocked iff any validation fails) is unchanged. The page-level alerts are not short-circuited: when validate() passes it falls through to them. The only behavior delta is the intended one — when a field is both schema-required and page-required, the user now sees the inline form errors instead of a masking alert.
The new per-page test ("runs RJSF validation before the page-level required-field alerts") sets the form invalid + an empty emitted spec and asserts no alert and no POST; under the previous ordering the page-level alert would have fired, so it genuinely pins the new order. The existing invalid→no-POST / valid→POST tests still hold.
Non-blocking: consider a test for the fall-through case (schema valid, a page-required field missing → page alert still fires) to lock that path too; and the schemaFormRef.current && !current.validate() guard is fine as-is (the form is always mounted when submit is reachable), so the optional-chaining suggestion is cosmetic. UI typecheck/test job is green.
368ccc9 to
8700e3f
Compare
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
Validation-before-page-checks reorder is correct; zero CRITICAL/MAJOR. The new tests are non-vacuous (confirmed by mutation). Only minor cleanups below.
Note on the post-approval commit: the branch was force-pushed. lexfrei approved on 2026-06-26, but the current single commit 8700e3f7 is dated 2026-06-29 — the commit lexfrei saw is no longer in the tree. This is a fresh full review of the current state: same substance (validation reorder), no new severe issues introduced by the force-push.
Findings (all non-blocking)
[MINOR] packages/system/dashboard/images/console/apps/console/src/routes/BackupCreatePage.tsx:87-100 (and the same pattern in BackupJobCreatePage.tsx:104-112, BackupPlanCreatePage.tsx:109-117, BackupRestoreJobCreatePage.tsx:125-128) — the manual per-field alerts are now dead code. All these fields are schema-required (confirmed against the CRD definitions in packages/system/backup-controller/definitions/*.yaml), so validate(), which now runs first, always fails earlier and control never reaches the alerts. This contradicts the PR-body claim that "page-level alerts cover only page-only requirements". Not a regression (the fix works) — either remove the dead branches or correct the wording.
[MINOR] packages/system/dashboard/images/console/apps/console/src/routes/BackupCreatePage.test.tsx:96-105 (same in all 4 test files) — alertSpy.mockRestore() is the last line after expect(...); if the assertion throws, restore never runs and the window.alert spy leaks into the rest of the block (test/setup.ts has no vi.restoreAllMocks()). Use try/finally or restore in afterEach.
[PARTIAL claim] The PR body states "full console suite green (317 passed)". The full-suite run here gave 316 passed / 1 failed (DynamicOptionsWidget.test.tsx) — a pre-existing, load-dependent flake in an untouched file (8/8 green in isolation). The 4 relevant PR test files (12 tests) were green in every run. Update the claim.
Verification performed
- Non-vacuity confirmed by mutation: reverting the 4
.tsxfiles to their pre-fix state (aec16212) while keeping the new tests fails exactly the 4 new "runs RJSF validation before…" tests (4/4), leaving the other 8 green. Working tree restored;tsc --noEmitexit 0. - Phase 5b / 5c: N/A — only TS/TSX source touched, no chart template / values / migration changes, so no upgrade or fresh-install surface.
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. The submit button lives outside RJSF and bypasses its validation, so each page triggers validate() explicitly; it previously ran only after the manual alert 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. Re-applied from cozystack/cozystack-ui#26 onto the in-tree console after the UI was vendored (#2963); the source repo is being archived. Scope is the validation-ordering change only — the original PR's VMDisk/StorageClass parts were obsoleted by the DynamicOptionsWidget refactor (#30) already in the vendored snapshot. Includes the original per-page tests. Co-authored-by: Aleksei Sviridkin <1366514+lexfrei@users.noreply.github.com> Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
8700e3f to
6181493
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
What this PR does
Re-applies cozystack/cozystack-ui#26 (by Aleksei Sviridkin (@lexfrei)) onto the in-tree console, after the UI was vendored into the monorepo (#2963) and the standalone repo is being archived. Part of the cozystack-ui retirement tracked in #3097.
In the four backup create pages (Backup, BackupJob, BackupPlan, BackupRestoreJob), the RJSF
SchemaForm.validate()call now runs right after the name check and before the page-level required-field alerts. The submit button lives outside RJSF and bypasses its validation, so each page triggersvalidate()explicitly; it previously ran only after the manual alert 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 is the validation-ordering change only — the original PR's VMDisk/StorageClass parts were obsoleted by the
DynamicOptionsWidgetrefactor (#30) already present in the vendored snapshot.Verification
pnpm typecheckclean; full console suite green (317 passed).Original PR: cozystack/cozystack-ui#26 — credited via
Co-authored-by.Summary by CodeRabbit
Bug Fixes
Tests