feat: show progress indicator on mirrors #499
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR adds "in_progress" status support for mirror synchronization operations. It expands status types across server and client layers, implements real-time server event handling in the UI to reflect ongoing mirror operations, and refactors form reset logic from useEffect dependencies to event handlers. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/client/modules/backups/components/schedule-mirrors-config.tsx (1)
68-72:⚠️ Potential issue | 🟡 MinorPotential race: React Query refetch may overwrite SSE-driven
in_progressstate.When the SSE sets
lastCopyStatusto"in_progress", a subsequent React Query refetch ofcurrentMirrors(e.g., on window focus) could overwrite the assignment map with stale server data that doesn't yet reflectin_progress, causing the progress indicator to briefly disappear.One approach would be to merge rather than replace — e.g., skip overwriting entries currently marked
in_progressunless the server data also showsin_progressor a terminal status. That said, this is a timing edge case and unlikely to be disruptive in practice.
🤖 Fix all issues with AI agents
In `@app/client/modules/backups/components/schedule-mirrors-config.tsx`:
- Around line 355-360: The StatusDot is currently left to its internal default
when not syncing which causes it to animate for terminal variants; change the
prop so animation only occurs while syncing by explicitly passing a boolean:
compute animated via isSyncing(assignment) (e.g.,
animated={isSyncing(assignment)}) or check both syncing and variant from
getStatusVariant(assignment.lastCopyStatus) so that success/error terminal
variants get animated=false and only the syncing state sets animated=true;
update the StatusDot usage in schedule-mirrors-config.tsx accordingly.
- Around line 101-135: The SSE subscription is reinitialized because
addEventListener (from useServerEvents) is not memoized and changes each render;
update the useServerEvents hook to wrap and return addEventListener with
useCallback so it has a stable identity, ensure any internal dependencies are
listed in that useCallback dependency array, and then keep the current useEffect
in schedule-mirrors-config.tsx using addEventListener and scheduleId as
dependencies so subscriptions only change when truly needed.
In `@app/client/modules/backups/routes/backups.tsx`:
- Around line 50-54: The drag-reorder can pass -1 to arrayMove when active.id or
over.id aren’t found in baseItems; inside the setLocalItems updater (symbols:
setLocalItems, baseItems, arrayMove, active.id, over.id) check for missing IDs
and avoid calling arrayMove with -1: compute oldIndex/newIndex, and if either is
-1 then either rebuild baseItems from schedules?.map(s=>s.id) and recompute
indices, or if still missing simply return currentItems to noop the update;
ensure you use the same updater return type so no other state changes occur.
🧹 Nitpick comments (5)
app/client/modules/volumes/components/create-volume-form.tsx (1)
127-138: Good refactor: moving form reset fromuseEffectto the event handler.This is a cleaner pattern that avoids unnecessary render cycles and makes the reset logic explicit and traceable.
One minor note: in create mode,
field.onChange(value)on Line 129 is immediately overwritten byform.reset(...)on Line 131. It's harmless but redundant for that branch. Consider moving it into anelse:Optional: avoid redundant field.onChange in create mode
onValueChange={(value) => { - field.onChange(value); if (mode === "create") { form.reset({ name: form.getValues().name, ...defaultValuesForType[value as keyof typeof defaultValuesForType], }); + } else { + field.onChange(value); } }}app/client/modules/repositories/components/create-repository-form.tsx (1)
120-131: Nit:field.onChange(value)on Line 122 is redundant.
form.reset(...)on Line 123 already sets thebackendfield via thedefaultValuesForTypespread, so the explicitfield.onChange(value)call is overwritten immediately. Removing it would reduce confusion about which call actually takes effect.Suggested diff
<Select onValueChange={(value) => { - field.onChange(value); form.reset({ name: form.getValues().name, isExistingRepository: form.getValues().isExistingRepository, customPassword: form.getValues().customPassword, ...defaultValuesForType[value as keyof typeof defaultValuesForType], }); }} value={field.value} >app/client/modules/backups/routes/backups.tsx (1)
30-31:localItemsis never reset, causing stale UI after server-side data changes.Once the user drags (setting
localItems), the deriveditemslist will always uselocalItemsand never reflect server-side changes (e.g., newly created or deleted schedules) until the component remounts. Consider resettinglocalItemstonullwhenschedulesdata changes, so the component falls back to the server-derived order.Suggested approach
One option is to use a key or reset mechanism. A simple approach:
+import { useState, useEffect } from "react"; ... const [localItems, setLocalItems] = useState<number[] | null>(null); const items = localItems ?? schedules?.map((s) => s.id) ?? []; +// Reset local drag state when server data changes +useEffect(() => { + setLocalItems(null); +}, [schedules]);Alternatively, you could key the component on a derived value from
schedules(e.g., length or a hash of IDs) to force a remount when the set of schedules changes.app/server/modules/backups/backups.execution.ts (1)
369-436: Consider recovery for mirrors stuck inin_progressafter a crash.If the process crashes between setting
in_progress(line 379) and the catch/success handlers, the mirror will remain stuck inin_progressindefinitely. The backup schedule has a recovery mechanism (per learnings from PR#463— thestopBackupfunction resets stuck states), but mirrors lack an equivalent.This could be addressed in a follow-up with a startup sweep that resets any mirrors still in
in_progressto"error", similar to the pattern used for backup schedules. Based on learnings, the stopBackup function intentionally updates the schedule status in a finally block even when no backup is running, as a recovery mechanism to reset schedules stuck in "in_progress" state when backups crash unexpectedly.app/client/modules/backups/components/schedule-mirrors-config.tsx (1)
207-212: Consider using theMirrorAssignment["lastCopyStatus"]type instead ofstring | null.This would provide compile-time safety ensuring only valid status values are passed and the function stays in sync with
MirrorAssignment.Suggested change
-const getStatusVariant = (status: string | null) => { +const getStatusVariant = (status: MirrorAssignment["lastCopyStatus"]) => {
nicotsx
force-pushed
the
feat/mirror-progress-indicator
branch
from
dd07b83 to
8ff3e5f
Compare
refactor: remove unnecessary useEffects
nicotsx
force-pushed
the
feat/mirror-progress-indicator
branch
from
8ff3e5f to
75860c4
Compare
nicotsx
force-pushed
the
feat/mirror-progress-indicator
branch
from
75860c4 to
04718b7
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04718b740b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await mirrorQueries.updateStatus(mirror.id, { | ||
| lastCopyStatus: "in_progress", | ||
| lastCopyError: null, | ||
| }); |
There was a problem hiding this comment.
Persisting lastCopyStatus: "in_progress" here can strand a mirror in syncing state when mirror settings are saved during a running copy: updateScheduleMirrors deletes/reinserts mirror rows (new IDs) while preserving the current status, but this worker continues updating by the old mirror.id, so the terminal success/error update can affect zero rows and the recreated row keeps in_progress indefinitely. Updating status by a stable key (scheduleId + repositoryId) or avoiding persisted in_progress during mutable config windows would prevent permanent false progress indicators.
Useful? React with 👍 / 👎.
1 participant
Closes #474 #128
Summary by CodeRabbit
New Features
Improvements