feat: Implement basic backup/restore UI #37
Conversation
- Create BackupsTab.vue component for backup management - Add backup/restore with async job polling (2s interval) - Add scheduled backup creation UI with cron expression - Add BackupJob types and API methods to api.ts - Update DeploymentDetailView to use BackupsTab component - Include restore, download, delete functionality
Code Review SummaryThe pull request introduces a new 🚀 Key Improvements
💡 Minor Suggestions
🚨 Critical Issues
|
![sourceant[bot]](https://avatars.githubusercontent.com/in/1161781?s=60&v=4){kind=small}
| const mockGetDeploymentBackups = backupsApi.getDeploymentBackups as ReturnType<typeof vi.fn>; | ||
| const mockCreateDeploymentBackup = backupsApi.createDeploymentBackup as ReturnType<typeof vi.fn>; | ||
| const mockDeleteBackup = backupsApi.delete as ReturnType<typeof vi.fn>; | ||
| const mockRestoreBackup = backupsApi.restore as ReturnType<typeof vi.fn>; | ||
| const mockListTasks = schedulerApi.listTasks as ReturnType<typeof vi.fn>; | ||
| const mockCreateTask = schedulerApi.createTask as ReturnType<typeof vi.fn>; |
There was a problem hiding this comment.
For better type safety and IDE auto-completion with vitest mocks, it's recommended to import MockedFunction and use it instead of ReturnType<typeof vi.fn>. This provides more specific typings for mock properties and methods, making the mocks easier to work with.
| const mockGetDeploymentBackups = backupsApi.getDeploymentBackups as ReturnType<typeof vi.fn>; | |
| const mockCreateDeploymentBackup = backupsApi.createDeploymentBackup as ReturnType<typeof vi.fn>; | |
| const mockDeleteBackup = backupsApi.delete as ReturnType<typeof vi.fn>; | |
| const mockRestoreBackup = backupsApi.restore as ReturnType<typeof vi.fn>; | |
| const mockListTasks = schedulerApi.listTasks as ReturnType<typeof vi.fn>; | |
| const mockCreateTask = schedulerApi.createTask as ReturnType<typeof vi.fn>; | |
| import { MockedFunction } from "vitest"; | |
| const mockGetDeploymentBackups = backupsApi.getDeploymentBackups as MockedFunction<typeof backupsApi.getDeploymentBackups>; | |
| const mockCreateDeploymentBackup = backupsApi.createDeploymentBackup as MockedFunction<typeof backupsApi.createDeploymentBackup>; | |
| const mockDeleteBackup = backupsApi.delete as MockedFunction<typeof backupsApi.delete>; | |
| const mockRestoreBackup = backupsApi.restore as MockedFunction<typeof backupsApi.restore>; | |
| const mockListTasks = schedulerApi.listTasks as MockedFunction<typeof schedulerApi.listTasks>; | |
| const mockCreateTask = schedulerApi.createTask as MockedFunction<typeof schedulerApi.createTask>; |
| const props = defineProps<{ | ||
| deploymentName: string; | ||
| }>(); |
There was a problem hiding this comment.
Adding the readonly modifier to props explicitly declares that they should not be modified within the component. This enhances type safety and clearly communicates the intended immutability of the prop, aligning with Vue's best practices.
| const props = defineProps<{ | |
| deploymentName: string; | |
| }>(); | |
| const props = defineProps<{ | |
| readonly deploymentName: string; | |
| }>(); |
src/components/BackupsTab.vue
Outdated
| const scheduleForm = ref({ | ||
| name: "", | ||
| cronExpr: "0 2 * * *", | ||
| retentionCount: 7, | ||
| enabled: true, |
There was a problem hiding this comment.
Extracting the initial state of the scheduleForm into a factory function (initialScheduleFormState) improves reusability and makes resetting the form to its default values more concise and robust. This approach centralizes the default form structure.
| const scheduleForm = ref({ | |
| name: "", | |
| cronExpr: "0 2 * * *", | |
| retentionCount: 7, | |
| enabled: true, | |
| const initialScheduleFormState = () => ({ | |
| name: "", | |
| cronExpr: "0 2 * * *", | |
| retentionCount: 7, | |
| enabled: true, | |
| }); | |
| const scheduleForm = ref(initialScheduleFormState()); |
| for (const job of activeJobs.value) { | ||
| try { | ||
| const response = await backupsApi.getJob(job.id); | ||
| const updatedJob = response.data.job; | ||
| if (updatedJob.status === "completed") { | ||
| if (updatedJob.type === "backup") { | ||
| notifications.success("Backup Complete", "Backup has been created successfully"); | ||
| creatingBackup.value = false; | ||
| } else { | ||
| notifications.success("Restore Complete", "Backup has been restored successfully"); | ||
| restoringBackup.value = null; | ||
| } | ||
| await fetchBackups(); | ||
| } else if (updatedJob.status === "failed") { | ||
| if (updatedJob.type === "backup") { | ||
| notifications.error("Backup Failed", updatedJob.error || "Backup creation failed"); | ||
| creatingBackup.value = false; | ||
| } else { | ||
| notifications.error("Restore Failed", updatedJob.error || "Backup restore failed"); | ||
| restoringBackup.value = null; | ||
| } | ||
| } else { | ||
| updatedJobs.push(updatedJob); | ||
| } | ||
| } catch { | ||
| updatedJobs.push(job); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current pollActiveJobs uses a for...of loop with await inside, meaning if one job's API call fails, it could delay or block the processing of subsequent jobs in the same polling cycle. Using Promise.allSettled to fetch all job statuses concurrently makes the polling mechanism more robust, as it allows all promises to settle regardless of individual success or failure, ensuring all jobs are checked efficiently and gracefully handling partial failures.
| for (const job of activeJobs.value) { | |
| try { | |
| const response = await backupsApi.getJob(job.id); | |
| const updatedJob = response.data.job; | |
| if (updatedJob.status === "completed") { | |
| if (updatedJob.type === "backup") { | |
| notifications.success("Backup Complete", "Backup has been created successfully"); | |
| creatingBackup.value = false; | |
| } else { | |
| notifications.success("Restore Complete", "Backup has been restored successfully"); | |
| restoringBackup.value = null; | |
| } | |
| await fetchBackups(); | |
| } else if (updatedJob.status === "failed") { | |
| if (updatedJob.type === "backup") { | |
| notifications.error("Backup Failed", updatedJob.error || "Backup creation failed"); | |
| creatingBackup.value = false; | |
| } else { | |
| notifications.error("Restore Failed", updatedJob.error || "Backup restore failed"); | |
| restoringBackup.value = null; | |
| } | |
| } else { | |
| updatedJobs.push(updatedJob); | |
| } | |
| } catch { | |
| updatedJobs.push(job); | |
| } | |
| } | |
| const jobPromises = activeJobs.value.map(async (job) => { | |
| try { | |
| const response = await backupsApi.getJob(job.id); | |
| const updatedJob = response.data.job; | |
| if (updatedJob.status === "completed") { | |
| if (updatedJob.type === "backup") { | |
| notifications.success("Backup Complete", "Backup has been created successfully"); | |
| creatingBackup.value = false; | |
| } else { | |
| notifications.success("Restore Complete", "Backup has been restored successfully"); | |
| restoringBackup.value = null; | |
| } | |
| await fetchBackups(); | |
| return null; // Job is completed/failed, no longer active | |
| } else if (updatedJob.status === "failed") { | |
| if (updatedJob.type === "backup") { | |
| notifications.error("Backup Failed", updatedJob.error || "Backup creation failed"); | |
| creatingBackup.value = false; | |
| } else { | |
| notifications.error("Restore Failed", updatedJob.error || "Backup restore failed"); | |
| restoringBackup.value = null; | |
| } | |
| return null; // Job is completed/failed, no longer active | |
| } else { | |
| return updatedJob; // Job is still active | |
| } | |
| } catch (error) { | |
| console.error("Error polling job:", job.id, error); | |
| return job; // Keep job in active list if API call itself fails | |
| } | |
| }); | |
| const results = await Promise.allSettled(jobPromises); | |
| activeJobs.value = results | |
| .filter((result) => result.status === 'fulfilled' && result.value !== null) | |
| .map((result) => (result as PromiseFulfilledResult<BackupJob>).value as BackupJob); | |
| if (activeJobs.value.length === 0) { | |
| stopJobPolling(); | |
| } |
| const date = new Date(dateStr); | ||
| return date.toLocaleString(); | ||
| }; |
There was a problem hiding this comment.
The toLocaleString() method's output varies depending on the user's browser and locale settings, which can lead to an inconsistent user experience. Using Intl.DateTimeFormat with explicit options allows for specifying a consistent format (e.g., 'en-US') or dynamically adapting it if a global locale is managed. Added a check for an empty date string.
| const date = new Date(dateStr); | |
| return date.toLocaleString(); | |
| }; | |
| const formatDate = (dateStr: string): string => { | |
| if (!dateStr) return "N/A"; | |
| const date = new Date(dateStr); | |
| return new Intl.DateTimeFormat('en-US', { | |
| year: 'numeric', | |
| month: 'short', | |
| day: 'numeric', | |
| hour: '2-digit', | |
| minute: '2-digit', | |
| second: '2-digit', | |
| hour12: false | |
| }).format(date); | |
| }; |
| export const backupsApi = { | ||
| list: (deployment?: string, limit?: number) => |
There was a problem hiding this comment.
Explicitly defining the full return type of API methods, including Promise<AxiosResponse<T>>, provides better clarity on the expected output and improves type checking for consumers of these API methods. It helps developers understand what to expect without needing to infer types. Ensure AxiosResponse is imported from axios.
| export const backupsApi = { | |
| list: (deployment?: string, limit?: number) => | |
| create: (deploymentName: string): Promise<AxiosResponse<{ job_id: string; message: string }>> => | |
| apiClient.post<{ job_id: string; message: string }>("/backups", { deployment_name: deploymentName }), |
nfebe
force-pushed
the
feat/backup-and-scheduler
branch
from
2aff62c to
cb0577f
Compare
Deploying flatrun-ui with
|
| Latest commit: |
7041067
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0969b105.flatrun-ui.pages.dev |
| Branch Preview URL: | https://feat-backup-and-scheduler.flatrun-ui.pages.dev |
| mockListTasks.mockResolvedValue({ data: { tasks: [] } }); | ||
| }); | ||
|
|
||
| const mountBackupsTab = (options: { backups?: typeof mockBackups; tasks?: typeof mockScheduledTasks } = {}) => { |
There was a problem hiding this comment.
It's good practice to use the directly imported types (Backup, ScheduledTask) instead of typeof mockBackups for clarity and consistency, as these types are explicitly defined in @/services/api.
| const mountBackupsTab = (options: { backups?: typeof mockBackups; tasks?: typeof mockScheduledTasks } = {}) => { | |
| const mountBackupsTab = (options: { backups?: Backup[]; tasks?: ScheduledTask[] } = {}) => { |
| const k = 1024; | ||
| const sizes = ["B", "KB", "MB", "GB"]; | ||
| const i = Math.floor(Math.log(bytes) / Math.log(k)); | ||
| return parseFloat((bytes / Math.pow(k, i)).toFixed(1)) + " " + sizes[i]; |
There was a problem hiding this comment.
The toFixed(1) method will always return one decimal place, even for whole numbers (e.g., "1.0 KB"). To match the test expectation of "1 KB" for exact kilobyte values, explicitly check if the number has a fractional part before applying toFixed(1) or round to the nearest integer if it is effectively a whole number.
| return parseFloat((bytes / Math.pow(k, i)).toFixed(1)) + " " + sizes[i]; | |
| return (Math.round(bytes / Math.pow(k, i) * 10) / 10).toString() + " " + sizes[i]; |
| } catch { | ||
| scheduledTasks.value = []; | ||
| } |
There was a problem hiding this comment.
The schedulerApi.listTasks already accepts a deployment parameter, making the client-side filtering by t.deployment_name === props.deploymentName redundant. You only need to filter by type if the API does not support that parameter directly.
| } catch { | |
| scheduledTasks.value = []; | |
| } | |
| scheduledTasks.value = (response.data.tasks || []).filter( | |
| (t) => t.type === "backup" | |
| ); |
nfebe
force-pushed
the
feat/backup-and-scheduler
branch
from
cb0577f to
0524b77
Compare
| await new Promise((r) => setTimeout(r, 10)); | ||
| await wrapper.vm.$nextTick(); |
There was a problem hiding this comment.
The pattern await new Promise((r) => setTimeout(r, 10)); await wrapper.vm.$nextTick(); is a common workaround but flushPromises() from @vue/test-utils is generally a more robust and idiomatic way to ensure all pending promises (including microtasks and Vue's reactivity updates) are resolved before assertions. Consider replacing this pattern throughout the test file for better clarity and reliability.
| await new Promise((r) => setTimeout(r, 10)); | |
| await wrapper.vm.$nextTick(); | |
| await flushPromises(); |
| export type BackupJobType = "backup" | "restore"; | ||
| export type BackupJobStatus = "pending" | "running" | "completed" | "failed"; |
There was a problem hiding this comment.
Type aliases BackupJobType and BackupJobStatus are already defined on lines 809 and 810. Using these aliases here improves type consistency and avoids redundant inline string literal types. It makes the BackupJob interface more readable and maintainable by referring to established types.
| export type BackupJobType = "backup" | "restore"; | |
| export type BackupJobStatus = "pending" | "running" | "completed" | "failed"; | |
| type: BackupJobType; | |
| status: BackupJobStatus; |
Add comprehensive tests for: - Component structure and rendering - Backup list display and actions - Create, delete, and restore functionality - Schedule modal and scheduled tasks - Job polling behavior Signed-off-by: nfebe <fenn25.fn@gmail.com>
nfebe
force-pushed
the
feat/backup-and-scheduler
branch
from
0524b77 to
7041067
Compare
🔍 Code Review💡 1. **src/components/BackupsTab.test.ts** (Lines 128-130) - BUGUsing Suggested Code: Current Code: await wrapper.vm.$nextTick();
await new Promise((r) => setTimeout(r, 10));
await wrapper.vm.$nextTick();💡 2. **src/components/BackupsTab.test.ts** (Lines 138-140) - BUGSimilar to the previous comment, replace Suggested Code: Current Code: await wrapper.vm.$nextTick();
await new Promise((r) => setTimeout(r, 10));
await wrapper.vm.$nextTick();💡 3. **src/components/BackupsTab.test.ts** (Lines 178-180) - BUGAgain, avoid Suggested Code: Current Code: await wrapper.vm.$nextTick();
await new Promise((r) => setTimeout(r, 10));
await wrapper.vm.$nextTick();💡 4. **src/components/BackupsTab.test.ts** (Lines 188-190) - BUGRemove the Suggested Code: Current Code: await wrapper.vm.$nextTick();
await new Promise((r) => setTimeout(r, 10));
await wrapper.vm.$nextTick();💡 5. **src/components/BackupsTab.test.ts** (Lines 197-199) - BUGThis Suggested Code: Current Code: await wrapper.vm.$nextTick();
await new Promise((r) => setTimeout(r, 10));
await wrapper.vm.$nextTick();💡 6. **src/components/BackupsTab.test.ts** (Lines 208-210) - BUGReplace Suggested Code: Current Code: await wrapper.vm.$nextTick();
await new Promise((r) => setTimeout(r, 10));
await wrapper.vm.$nextTick();💡 7. **src/components/BackupsTab.test.ts** (Lines 217-219) - BUGThe use of Suggested Code: Current Code: await wrapper.vm.$nextTick();
await new Promise((r) => setTimeout(r, 10));
await wrapper.vm.$nextTick();💡 8. **src/components/BackupsTab.test.ts** (Lines 228-230) - BUGRemove Suggested Code: Current Code: await wrapper.vm.$nextTick();
await new Promise((r) => setTimeout(r, 10));
await wrapper.vm.$nextTick();💡 9. **src/components/BackupsTab.test.ts** (Lines 286-288) - BUGEliminate Suggested Code: Current Code: await wrapper.vm.$nextTick();
await new Promise((r) => setTimeout(r, 10));
await wrapper.vm.$nextTick();💡 10. **src/components/BackupsTab.test.ts** (Lines 321-323) - BUGRemove the Suggested Code: Current Code: await wrapper.vm.$nextTick();
await new Promise((r) => setTimeout(r, 10));
await wrapper.vm.$nextTick();💡 11. **src/components/BackupsTab.test.ts** (Lines 390-392) - BUGRemove Suggested Code: Current Code: await wrapper.vm.$nextTick();
await new Promise((r) => setTimeout(r, 10));
await wrapper.vm.$nextTick();💡 12. **src/components/BackupsTab.test.ts** (Lines 409-411) - BUGAvoid using Suggested Code: Current Code: await wrapper.vm.$nextTick();
await new Promise((r) => setTimeout(r, 10));
await wrapper.vm.$nextTick();💡 13. **src/components/BackupsTab.test.ts** (Lines 418-420) - BUGReplace Suggested Code: Current Code: await wrapper.vm.$nextTick();
await new Promise((r) => setTimeout(r, 10));
await wrapper.vm.$nextTick();💡 14. **src/components/BackupsTab.test.ts** (Lines 427-429) - BUGRemove Suggested Code: Current Code: await wrapper.vm.$nextTick();
await new Promise((r) => setTimeout(r, 10));
await wrapper.vm.$nextTick();💡 15. **src/components/BackupsTab.vue** (Lines 40-44) - STYLEThe Suggested Code: Current Code: <div v-if="backup.components?.length" class="backup-components">
<span v-for="comp in backup.components" :key="comp" class="component-badge">
{{ comp }}
</span>
</div>💡 16. **src/components/BackupsTab.vue** (Lines 65-89) - CLARITYConsider adding an empty state for scheduled tasks. Currently, if Suggested Code: Current Code: <div v-if="scheduledTasks.length > 0" class="scheduled-backups-section">
<h4>Scheduled Backups</h4>
<div class="scheduled-tasks-list">
<div v-for="task in scheduledTasks" :key="task.id" class="scheduled-task-item">
<div class="task-info">
<span class="task-name">{{ task.name }}</span>
<span class="task-schedule">{{ task.cron_expr }}</span>
<span v-if="task.next_run" class="task-next"> Next: {{ formatDate(task.next_run) }} </span>
</div>
<div class="task-actions">
<label class="toggle-switch small">
<input type="checkbox" :checked="task.enabled" @change="toggleTask(task)" />
<span class="toggle-slider" />
</label>
<button class="btn btn-sm btn-secondary" @click="runTaskNow(task.id)">
<i class="pi pi-play" />
</button>
<button class="btn btn-sm btn-danger" @click="confirmDeleteTask(task.id)">
<i class="pi pi-trash" />
</button>
</div>
</div>
</div>
</div>💡 17. **src/components/BackupsTab.vue** (Line 93) - IMPROVEMENTFor improved accessibility, modals should be closable using the Suggested Code: Current Code: <div v-if="showScheduleModal" class="modal-overlay" @click.self="showScheduleModal = false">💡 18. **src/components/BackupsTab.vue** (Line 113) - STYLETo maintain consistency and avoid hardcoding, consider referencing the default Suggested Code: Current Code: <input v-model="scheduleForm.cronExpr" type="text" placeholder="0 2 * * *" class="form-control" />💡 19. **src/components/BackupsTab.vue** (Lines 234-238) - REFACTORError handling is a bit repetitive. You could create a helper function, possibly within Suggested Code: Current Code: } catch (err: any) {
notifications.error("Error", err.response?.data?.error || "Failed to load backups");
} finally {💡 20. **src/components/BackupsTab.vue** (Lines 248-249) - REFACTORA dedicated error helper function could simplify this error handling as well. For example, Suggested Code: Current Code: } catch {
scheduledTasks.value = [];💡 21. **src/components/BackupsTab.vue** (Lines 333-335) - REFACTORCentralize the error handling logic to reduce repetition. A helper function can abstract away the Suggested Code: Current Code: } catch (err: any) {
notifications.error("Backup Failed", err.response?.data?.error || "Failed to start backup");
creatingBackup.value = false;
}💡 22. **src/components/BackupsTab.vue** (Lines 350-352) - REFACTORThis error handling can also be consolidated using a helper function to keep the component script cleaner and more focused on its core logic. Suggested Code: Current Code: } catch (err: any) {
notifications.error("Delete Failed", err.response?.data?.error || "Failed to delete backup");
} finally {💡 23. **src/components/BackupsTab.vue** (Lines 386-388) - REFACTORConsolidate this repetitive error handling by using a shared utility function. This improves maintainability and ensures a consistent error message format. Suggested Code: Current Code: } catch (err: any) {
notifications.error("Restore Failed", err.response?.data?.error || "Failed to start restore");
restoringBackup.value = null;
} finally {💡 24. **src/components/BackupsTab.vue** (Lines 417-419) - REFACTOREncapsulate this error handling pattern into a utility function to avoid code duplication and promote consistency across the component's API interactions. Suggested Code: Current Code: } catch (err: any) {
notifications.error("Error", err.response?.data?.error || "Failed to create schedule");
} finally {💡 25. **src/components/BackupsTab.vue** (Lines 428-430) - REFACTORAbstract this error handling into a reusable function for consistency and brevity. Suggested Code: Current Code: } catch (err: any) {
notifications.error("Error", err.response?.data?.error || "Failed to update task");
}💡 26. **src/components/BackupsTab.vue** (Lines 437-439) - REFACTORConsolidate this error message logic into a single helper to prevent repetition and ensure uniform error reporting. Suggested Code: Current Code: } catch (err: any) {
notifications.error("Error", err.response?.data?.error || "Failed to run task");
}💡 27. **src/components/BackupsTab.vue** (Lines 451-453) - REFACTORThis error handling block can also be streamlined with the common helper function, reducing boilerplate and improving clarity. Suggested Code: Current Code: } catch (err: any) {
notifications.error("Delete Failed", err.response?.data?.error || "Failed to delete task");
} finally {💡 28. **src/components/BackupsTab.vue** (Line 190) - REFACTORTo streamline error reporting and reduce code duplication, extract the repetitive error notification logic into a helper function. This function can take the error object and a default message, then use the const handleApiError = (err: any, defaultMsg: string) => {
notifications.error("Error", err.response?.data?.error || `Failed to ${defaultMsg}`);
};Then, replace repeated Suggested Code: Current Code: import ConfirmModal from "@/components/ConfirmModal.vue";💡 29. **src/views/DeploymentDetailView.vue** (Lines 1435-1439) - IMPROVEMENTThe Suggested Code: Current Code: <Teleport to="body">
<div v-if="showActionModal" class="modal-overlay">
<div class="action-modal modal-container">
<div class="modal-header">
<h3>
<i class="pi pi-bolt" />
{{ editingActionId ? "Edit Quick Action" : "Add Quick Action" }}
</h3>
<button class="close-btn" @click="showActionModal = false">
<i class="pi pi-times" />
</button>
</div>Verdict: APPROVE Posted as a comment because posting a review failed. |
No description provided.