fix(backup): several fixes around backup-pages#11
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR refactors backup creation pages to add API group awareness and introduces a dedicated BackupJobCreatePage to replace a generic resource creator. The changes improve SchemaForm enum handling, add conditional resource filtering by API group across backup plan and restore job creation flows, and establish new routing for the specialized backup job creator. ChangesBackup Creation Pages with API Group Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
![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 a dedicated BackupJobCreatePage and refactors existing backup-related creation pages to handle API groups more robustly, specifically filtering application kinds and instances when the API group is apps.cozystack.io. It also improves SchemaForm to preserve user input during async schema updates and fixes UI issues where defaults interfered with selection widgets. Feedback includes suggestions to wrap JSON.parse calls in try-catch blocks for safety, replace alert() calls with better UX patterns like toast notifications, and improve accessibility by adding id and htmlFor attributes to form labels and inputs.
| const schema = useMemo(() => { | ||
| if (!baseSchema) return null | ||
|
|
||
| const base = JSON.parse(baseSchema) |
There was a problem hiding this comment.
The JSON.parse(baseSchema) call is unsafe and will crash the component if baseSchema is invalid or empty. Consider wrapping it in a try-catch block to handle parsing errors gracefully, similar to the implementation in SchemaForm.tsx.
| const base = JSON.parse(baseSchema) | |
| let base; | |
| try { | |
| base = JSON.parse(baseSchema); | |
| } catch (e) { | |
| console.error("Failed to parse BackupJob schema:", e); | |
| return null; | |
| } |
| @@ -53,7 +54,12 @@ export function BackupPlanCreatePage() { | |||
| if (!baseSchema) return null | |||
|
|
|||
| const base = JSON.parse(baseSchema) | |||
There was a problem hiding this comment.
The JSON.parse(baseSchema) call is unsafe and will crash the component if baseSchema is invalid. Consider wrapping it in a try-catch block to handle potential parsing errors.
| const base = JSON.parse(baseSchema) | |
| let base; | |
| try { | |
| base = JSON.parse(baseSchema); | |
| } catch (e) { | |
| console.error("Failed to parse Plan schema:", e); | |
| return null; | |
| } |
| @@ -55,7 +56,13 @@ export function BackupRestoreJobCreatePage() { | |||
|
|
|||
| const base = JSON.parse(baseSchema) | |||
There was a problem hiding this comment.
The JSON.parse(baseSchema) call is unsafe and will crash the component if baseSchema is invalid. Consider wrapping it in a try-catch block.
let base;
try {
base = JSON.parse(baseSchema);
} catch (e) {
console.error("Failed to parse RestoreJob schema:", e);
return null;
}
| alert("Tenant namespace is not available. Please refresh.") | ||
| return | ||
| } | ||
|
|
||
| if (!name.trim()) { | ||
| alert("Name is required") | ||
| return | ||
| } | ||
|
|
||
| if (!formData.applicationRef?.kind || !formData.applicationRef?.name) { | ||
| alert("Application reference is required") | ||
| return | ||
| } | ||
|
|
||
| if (!formData.backupClassName) { | ||
| alert("Backup class name is required") | ||
| return | ||
| } | ||
|
|
||
| const resource = { | ||
| apiVersion: "backups.cozystack.io/v1alpha1", | ||
| kind: "BackupJob", | ||
| metadata: { | ||
| name: name.trim(), | ||
| namespace: tenantNamespace ?? undefined, | ||
| }, | ||
| spec: formData, | ||
| } | ||
|
|
||
| try { | ||
| await createMutation.mutateAsync(resource) | ||
| navigate("/console/backups/backupjobs") | ||
| } catch (err) { | ||
| alert(`Failed to create BackupJob: ${(err as Error).message}`) |
There was a problem hiding this comment.
Using alert() for validation and error reporting is generally discouraged as it provides a poor user experience and blocks the main thread. Consider using a toast notification system or inline validation messages. Additionally, these strings are hardcoded and should be moved to a localization system if the project supports i18n.
| <label className="block text-sm font-medium text-slate-700 mb-1"> | ||
| Backup Job Name <span className="text-red-500">*</span> | ||
| </label> | ||
| <input | ||
| type="text" | ||
| value={name} | ||
| onChange={(e) => setName(e.target.value)} | ||
| placeholder="my-backup-job" | ||
| className="w-full rounded-lg border border-slate-300 bg-white px-3 py-2 text-sm text-slate-900 outline-none focus:border-blue-400 focus:ring-1 focus:ring-blue-400" | ||
| required | ||
| /> |
There was a problem hiding this comment.
The 'Backup Job Name' input field is missing an id, and its label is missing a htmlFor attribute. Adding these improves accessibility for screen readers and allows users to focus the input by clicking the label.
| <label className="block text-sm font-medium text-slate-700 mb-1"> | |
| Backup Job Name <span className="text-red-500">*</span> | |
| </label> | |
| <input | |
| type="text" | |
| value={name} | |
| onChange={(e) => setName(e.target.value)} | |
| placeholder="my-backup-job" | |
| className="w-full rounded-lg border border-slate-300 bg-white px-3 py-2 text-sm text-slate-900 outline-none focus:border-blue-400 focus:ring-1 focus:ring-blue-400" | |
| required | |
| /> | |
| <div> | |
| <label htmlFor="backup-job-name" className="block text-sm font-medium text-slate-700 mb-1"> | |
| Backup Job Name <span className="text-red-500">*</span> | |
| </label> | |
| <input | |
| id="backup-job-name" | |
| type="text" | |
| value={name} | |
| onChange={(e) => setName(e.target.value)} | |
| placeholder="my-backup-job" | |
| className="w-full rounded-lg border border-slate-300 bg-white px-3 py-2 text-sm text-slate-900 outline-none focus:border-blue-400 focus:ring-1 focus:ring-blue-400" | |
| required | |
| /> | |
| </div> |
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
androndo
force-pushed
the
fix/backup-pages
branch
from
361fd3c to
5bbda8f
Compare
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor