Repository navigation
feat(frontend): add the database copy to the migration page - #15621
ogabrielluiz wants to merge 2 commits into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (20)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review. WalkthroughThe migration page now supports database-copy runs with start and stop controls, streamed progress, localized status and error messages, and copy-result counts. New API types and operations describe copy runs and events. Frontend and end-to-end tests cover the workflow. ChangesMigration copy workflow
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
actor Admin
participant CopyStep
participant useStartCopyMutation
participant followCopy
participant useStopCopyMutation
Admin->>CopyStep: Start copy
CopyStep->>useStartCopyMutation: Start the selected step
useStartCopyMutation-->>CopyStep: Request settles and migration state is invalidated
CopyStep->>followCopy: Follow run events after the current sequence
followCopy-->>CopyStep: Deliver numbered copy events
Admin->>CopyStep: Confirm stop
CopyStep->>useStopCopyMutation: Delete the selected run
Merge Risk: ⚪ Minimal · up to The database-copy page has no established new issue requiring a fix before merge. 🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 72.73% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 11 files. (9 skipped: 9 unsupported.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
Cristhianzl
left a comment
Cristhianzl
left a comment
There was a problem hiding this comment.
⚠️ Important (preferably this PR)
I1 — A JSON error response aborts the follower's shared controller, so neither the retry nor the record re-read happens
File: src/frontend/src/controllers/API/queries/migration/use-migration.ts:212-216 and src/frontend/src/pages/SettingsPage/pages/MigrationPage/CopyStep.tsx:144-170
Issue: followCopy signals "this was a refusal, stop reading" by returning false from onData. performStreamingRequest reacts to a falsy onData by calling buildController.abort() (src/frontend/src/controllers/API/api.tsx:337-341, 405-412). But useProgress creates one controller and reuses it for every iteration of the retry loop, so that abort is permanent. Trace the two paths:
- 404 (the case the code explicitly handles — "a run the server no longer has"):
overbecomestrue, the loop exits, and thenif (!controller.signal.aborted) client.invalidateQueries(...)is false, so the record is never re-read. The step keeps rendering the running panel off stale query data. - Any other HTTP error with a JSON body (401 once the access token expires during a long copy, 403, 409, 500):
overstaysfalse, so the code intends to retry — butcontroller.signal.abortedis nowtrue, thewhilecondition fails, and the loop dies on the first attempt with no retry and no re-read.
Why it matters: both paths end with the admin looking at "Starting…" or a frozen row count, with a Stop button, on a copy the page is no longer following — for a long destructive-ish migration operation that is exactly the state you do not want to leave someone in. Only a reload recovers. The 404 Jest test at src/frontend/src/pages/SettingsPage/pages/MigrationPage/__tests__/steps.test.tsx:1113-1127 passes only because it mocks { ok: false, status: 404, body: null }; performStreamingRequest then returns at its response.body === null guard before onData runs, so the abort never happens in the test. A real FastAPI 404 carries {"detail": …}, which does reach onData.
Suggested fix: give each attempt its own controller inside followCopy (linked to the caller's signal), or stop using the onData return value to end a refusal — read the refusal body and let the stream finish on its own. Then re-point the 404 test at a mock with a realistic {"detail": …} body so it covers the path the server actually produces, and add a case for a 500 that asserts the loop retries.
Code reference
// use-migration.ts
onData: async (data) => {
// A refusal answers with one JSON error body in place of the events.
if (!refused) onEvent(data as CopyEvent);
return !refused; // -> performStreamingRequest calls buildController.abort()
},
// CopyStep.tsx — one controller for every attempt
const controller = new AbortController();
while (!over && !controller.signal.aborted) { ... }
if (!controller.signal.aborted) client.invalidateQueries({ queryKey: migrationKeys.all });I2 — A follow that fails permanently is invisible to the admin
File: src/frontend/src/pages/SettingsPage/pages/MigrationPage/CopyStep.tsx:50-58, src/frontend/src/controllers/API/queries/migration/use-migration.ts:203-221
Issue: followCopy resolves with { status } and useProgress uses it only to decide whether to keep looping — the status is never surfaced. There is no error state for the event stream at all: the running panel renders the last progress line (or "Starting…") and nothing else, whatever happened. Compare runSourceChecks in the same file, which at least carries detail back to its caller.
Why it matters: combined with I1 this is why the page looks healthy while it is blind. Even with I1 fixed, a server that is genuinely unreachable means the admin watches a frozen counter with no way to tell whether the copy stalled or the page just lost contact. The record is also not polled while a copy runs — useMigrationQuery's refetchInterval fires only for check_source (use-migration.ts:28-31) — so the stream is the single source of truth and its failure has to be visible.
Suggested fix: keep the last refusal in useProgress state and render a line under the progress text once the follower has failed at least once, something like "Lost contact with the copy. Retrying…" plus a manual "Check again" that invalidates migrationKeys.all. One new key in all 8 locales.
I3 — No accessibility coverage for the new states, and a finished copy is never announced
File: src/frontend/src/pages/SettingsPage/pages/MigrationPage/CopyStep.tsx:56, src/frontend/src/pages/SettingsPage/pages/MigrationPage/index.tsx:286-298
Issue: three things, all in the same area:
- There is no a11y test anywhere for this page —
git ls-treeshows no*.a11y.test.tsxundersrc/frontend/src/pages/SettingsPage/pages/MigrationPage/and nosrc/frontend/tests/a11y/migration.a11y.spec.ts. The green "jest-axe" check on this PR therefore proves nothing about the panel. The repo's bar (.claude/CLAUDE.md,.claude/skills/building-frontend-ui/references/langflow-a11y.md) is both engines at zero across every meaningful state, and this PR is the one that gives the page its first live region, its first confirmation dialog and its first alert. - The outcome of a long-running operation is never announced. While running,
aria-live="polite"carries the progress line; when the run finishes well, that region unmounts and the result moves to the step's summary row (index.tsx:290-297), which is not a status region. Failure is announced, because therole="alert"block atCopyStep.tsx:106-111is newly inserted — success is silent. That is WCAG 2.2 4.1.3 (Status Messages, AA) for the one transition that matters most here. - The progress live region is unthrottled.
written()insqlite_to_postgres.pyemits acopyingevent per table batch, so on a real database that region changes many times a second and every change queues a polite announcement.
Suggested fix: add __tests__/CopyStep.a11y.test.tsx covering the running, blocked and done states plus the open stop dialog, and a tests/a11y/migration.a11y.spec.ts live-DOM scan of the running step. Keep one role="status" region mounted across the whole step so the done/failed transition lands in it, and throttle the progress text to roughly one update per second (or announce only phase changes and leave the row count as plain text outside the region).
I4 — index.tsx crossed the 500-line gate and the step-render callback's branch count is far past cyclomatic 10
File: src/frontend/src/pages/SettingsPage/pages/MigrationPage/index.tsx:226-345 (file: 486 lines at the base → 511 at the head)
Issue: the callback passed to part.steps.map now carries an eight-branch summary chain, a six-branch body chain, the live/unfinished/expandable predicates and a three-term running expression — I counted well over thirty decision points in one function. This PR adds two of those branches (copy_database && done for the summary, live && isCopy for the body) and rewrites the running expression, so the diff touches the lines in question. .claude/CLAUDE.md sets ≤ 500 LOC per file and cyclomatic ≤ 10 per function as hard style rules, and the file passed the first one on this commit.
Why it matters: this is the single function every future step in the stack has to grow — four more steps are coming in 15622 through 15627, each adding a summary branch and a body branch. It is much cheaper to split it now than at step ten.
Suggested fix: move the summary chain into a stepSummary(step, state, record, t, language) in catalog.ts (which already owns the per-step catalog) and the body chain into a lookup keyed by step id. That takes index.tsx back under 500 lines and gets the render callback down to a handful of branches without changing behavior.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## fix/migration-streams-close-source #15621 +/- ##
======================================================================
+ Coverage 68.77% 69.78% +1.00%
======================================================================
Files 2772 2777 +5
Lines 299096 300378 +1282
Branches 40301 41763 +1462
======================================================================
+ Hits 205705 209612 +3907
+ Misses 90897 88272 -2625
Partials 2494 2494
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
164f7b1 to
3e32978
Compare
github-actions
Bot
added
enhancement
3e32978 to
3d10cb3
Compare
github-actions
Bot
added
enhancement
erichare
left a comment
erichare
left a comment
There was a problem hiding this comment.
@ogabrielluiz Approving. I pushed one small fix on top. Thanks for following the run by seq with events?after=: the page picks up exactly where the backend's follow_run resumes, and the scripted-stream test proves the wait and the re-ask.
Fixes I pushed
- 88a2279 fixes two items from Cristhianzl's earlier approval (I1 and part of I3). The push after that approval was only a rebase, so both were still open:
- I1, a refusal froze the follower. A real error body such as
{"detail":{"code":"run_not_found"}}reachesonData. Returning!refusedmadeperformStreamingRequestabort the one controller that every retry shares. That meant no retry and no record re-read, and the step stayed on "Starting…" with a Stop button until a reload.followCopynow returnstrueand lets the body end on its own (use-migration.ts#L212-L217). The 404 test now sends a real JSON body instead ofbody: null, and a new 500 case checks that the page asks again afterRETRY_MS(steps.test.tsx#L1164-L1233). - I3, a finished copy was never announced. The only live region lived in the running branch, so it unmounted when the run ended. CopyStep now keeps one
role="status"mounted in both states. It shows the progress line while the copy runs, and after a run the page watched it holds thecopyDb.doneline as screen-reader-only text (CopyStep.tsx#L53-L83). A rerender test checks that the same element goes from "Starting…" to "Tables: 59. Rows: 1,234."
- I1, a refusal froze the follower. A real error body such as
The PRs stacked above (#15622, #15623, #15624) will need a restack onto the new head.
Nits
- A 401 once the session expires during a long copy now retries every 2 s with nothing visible on the panel. A muted "Lost contact with the copy. Retrying…" line would separate that from a slow copy (Cristhianzl's I2). It needs one new key in all 8 locales, so I left the wording to you.
- A copy still running after changes are turned back on renders no CopyStep, because
liveis false. The admin sees a spinner but has no Stop button.(live || copy?.status === "running") && isCopy(step.id)would keep the panel (index.tsx#L318). index.tsxis now 511 lines. Moving the summary chain into astepSummary(...)helper before the next copy steps land would keep that callback readable.
Verification
npx jest src/pages/SettingsPage/pages/MigrationPage -> 4 suites, 76 tests passed
# the 404 and 500 cases fail with `return !refused`; the 500 and announcement cases fail on the old CopyStep
npx @biomejs/biome check <3 touched files> -> no issues
npx tsc --noEmit -p . -> no errors in the touched files
github-actions
Bot
added
enhancement
88a2279 to
be886d5
Compare
4a2e0f3 to
06058d1
Compare
github-actions
Bot
added
enhancement
06058d1 to
f4b69a4
Compare
github-actions
Bot
added
enhancement
f4b69a4 to
2ac0f56
Compare
github-actions
Bot
added
enhancement
2ac0f56 to
13c64ce
Compare
github-actions
Bot
added
enhancement
13c64ce to
eb5c4cd
Compare
github-actions
Bot
added
enhancement
"Copy the database" starts the copy on the server and follows it while it runs. The run is the server's own process, so the page asks for its events from the last one it saw: after a reload that is the first one, and after a dropped connection the one it had got to. Stopping a copy asks first. When the last copy does not count, the step says why in the page's words, with the command's own words under Details. A database that is not empty, or that holds an earlier copy this instance no longer matches, can only be replaced: the step says so and names the step where a new, empty one is saved. A code the page has no words for reads as a failure. When the step waits again after a copy ended, it says the copy is out of date and has to be made again. A finished copy shows its tables and rows in the step's row and can be made again. The panel is written for the three copy steps. The other two use it next.
… end A JSON error body from the events request reached onData, whose false return aborted the controller every retry shares: no retry and no record re-read, so the step froze on its last progress line. Let the refusal body end on its own instead. Keep one role=status region mounted across the running and finished states so a screen reader hears the result of a copy it waited on.
eb5c4cd to
8b7ea51
Compare
github-actions
Bot
added
enhancement
This branch has not been deployed
3 participants
Stacked on the PR below it. Settings > Migration gets its sixth step, "Copy the database". This is the first page PR for the copy steps, and it needs the copy run routes that the API PRs further down the stack add:
POST /api/v1/migration/steps/{id}/runs, the run's events and its stop.What it adds
The step starts the copy on the server and follows it while it runs. It shows what the copy is doing: checking, preparing the new database, then the rows copied so far. The copy is the server's own process and belongs to no page. A page that is loaded again finds the run in the record and follows it from its first event. A connection that drops is picked up from the last event the page saw. When the server refuses a read of the progress, the page asks again after a moment. A run the server no longer has is the one refusal that ends the following.
"Stop" asks first, because nothing from a stopped database copy is kept.
When the last copy does not count, the step says why in the page's words, with the command's own words under Details. The page has words for a database it cannot reach, values or rows that cannot be copied, a copy that stopped or crashed, a destination that changed after the copy, passwords the server no longer holds after a restart, and another copy that is still running. Any other code reads as a failure with the command's words under it.
Two of these have one way out. A database that is not empty cannot take a copy. A database that holds an earlier copy cannot take another one once a row was deleted on this instance in between, because the copy never clears its destination. Each has its own line. Both say that only a new, empty database will do, and both name the step where it is saved, "Where your data goes".
A copy made before changes were last turned back on no longer counts, and the server puts the step back to waiting. The step then says that the copy is out of date and has to be made again.
A finished copy shows its tables and rows in the step's row. A screen reader that followed the copy hears that result when the copy ends. The step opens again from its title, to copy again.
The panel is written for the three copy steps. The next PR puts the other two on it.
Try it
Finish the first five steps as in the PRs below, on a SQLite instance with an empty PostgreSQL database as the destination. Click "Copy the database" and load the page again while it runs. The step is still running and follows the copy to its end. Click "Copy again" and stop it to see the stopped state. Restart the backend and click "Copy again": the step says the passwords are gone and names the step where they are entered.
To see a copy go out of date, turn changes back on after a copy, pause again, and go through the check and the backup again. The step waits again and says the copy has to be made again.
How it was tested
Jest has 21 new tests. They cover the start and its refusals, the running state, stopping, each reason the page words, a code it has no words for, a copy that is out of date, the done row and the progress lines. Jest sends no request. Where the page decides from the server's answer alone, a spy gives it that answer, the way
source-checks.test.tsxdoes. Two tests feed the page a scripted event stream. One checks that after a dropped connection the page waits, then asks for what came after the last event it saw. The other checks that it stops asking for a run the server no longer has. Two more cover a refusal that is asked again and the result that is read out at the end of a copy. I planted 42 one-line defects in this PR's code, one at a time, and a Jest test fails for each. The whole Jest suite of the frontend passes with this PR.tests/core/features/migration-copy.spec.tswalks the step in a browser against a real backend and a real PostgreSQL database: the copy, a reload while it runs, the done row, a second copy stopped on the way, and a third one to the end. It skips unlessMIGRATION_E2E_DATABASE_URLandMIGRATION_E2E_S3_BUCKETare set, so it does not run in CI. A copy needs changes paused, which refuses every change to the instance, so the walk cannot share the CI backend with other specs.By hand against the real API, I dropped the destination database before a copy. The step said it could not reach the database, with the driver's message under Details. I restarted the backend before a copy, and the step asked for the passwords again. I copied the database, deleted a file on the instance and copied again into the same database. It ended with one row too many there. The step said the database held an earlier copy that no longer matched, and asked for a new, empty one. I also turned changes back on after a copy and paused again. The step waited again and said the copy was out of date.
npm run check:i18npasses with the 20 new strings in all 8 locales. Biome is clean on the touched files, and the type check shows no new error.Summary by CodeRabbit