Repository navigation
feat(frontend): add the knowledge base and file copies to the migration page - #15622
ogabrielluiz merged 3 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
WalkthroughThe migration page adds test runs and report details for knowledge-base and file copies. The copy API contract carries dry-run state, status counts, failed-item details, and file byte progress. Localized messages and tests cover the updated workflow. ChangesMigration copy workflow
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
actor Admin
participant CopyStep
participant useStartCopyMutation
participant MigrationRunsEndpoint
Admin->>CopyStep: Select test run or regular copy
CopyStep->>useStartCopyMutation: Start with true or false
useStartCopyMutation->>MigrationRunsEndpoint: Send dry_run value
Merge Risk: ⚪ Minimal · up to The knowledge-base and file copy steps appear mergeable after normal checks; no unresolved issue was identified that should hold this change. 🚥 Pre-merge checks | ✅ 8 | ❌ 1
✨ Finishing Touches 💡 2
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 — The file-copy progress line can show a literal {{bytes}} to the admin
copyProgress adds bytes to the interpolation values only when the event carries one, but settings.migration.files.progress always contains the {{bytes}} token. i18next's skipOnVariables defaults to true (I checked src/frontend/node_modules/i18next/dist/cjs/i18next.js:1695 and src/frontend/src/i18n.ts:62-64, which overrides only escapeValue), and on a missing value that branch keeps match[0] — the raw {{bytes}} — in the output. So a copy_files progress event in any phase other than checking or preparing_target that has no bytes renders "Files: 1 of 3. Copied so far: {{bytes}}." on the migration screen. The conditional spread is itself the evidence that the author expects bytes to be absent sometimes.
Either default it (bytes: formatFileSize(event.bytes ?? 0)) or pick a bytes-free key when the event has none. If the server in fact always sends bytes on a copying-phase file event, then make bytes required in CopyEvent and drop the condition, so the type carries the guarantee.
// src/frontend/src/pages/SettingsPage/pages/MigrationPage/catalog.ts:268-275
return [
`${COPIES[step].slug}.progress`,
{
done: count(event.done),
total: count(event.total),
...(event.bytes !== undefined && { bytes: formatFileSize(event.bytes) }),
},
];I2 — A step blocked on a per-item code reads as "the step stopped with an error" when that item is not in the retained hundred
CopyStep.tsx:141-148 shows copy.attention ("Some were not copied. Each one below says why.") only when the blocking code appears among the items the record kept, and otherwise falls through to line(code), which resolves COPY_CODES[code] ?? "error.crashed". The per-item codes live in ITEM_CODES (catalog.ts:226-242), not in COPY_CODES (catalog.ts:206-220) — only bucket_error is in both. The report keeps the first hundred failures, so if 100+ files fail with no_source_bytes and the one file_conflict that blocks the step is number 120, items.some(...) is false and the admin reads settings.migration.error.crashed — "The step stopped with an error. Run it again. If it happens again, download the migration record." (en.json:763) — for a step that did not crash and that re-running will not fix. The same happens for any blocked step whose run row has no report.
The fix is one line: resolve the blocked code through ITEM_CODES before falling back, or show copy.attention whenever counted.failed > 0 and the code is a known item code.
// src/frontend/src/pages/SettingsPage/pages/MigrationPage/CopyStep.tsx:145-147
{items.some((item) => item.code === code)
? t("settings.migration.copy.attention")
: line(code)}I3 — "Test run" stays clickable while the real copy's POST is in flight, and the spinner jumps to it
Both buttons share one mutation and derive their busy state from start.variables, so exactly one of them is ever busy. Button blocks activation only on the button that is itself busy (src/frontend/src/components/ui/button.tsx:132 — blockActivation = Boolean(disabled) || isBusy), so during the copy's POST the "Test run" button is fully live. Clicking it fires a second POST steps/<step>/runs with dry_run: true, flips start.variables to true, and moves the loading indicator off the copy button onto the test button while the copy request is still outstanding. Best case the server refuses it and the admin sees a refusal line under a copy that is actually starting; worst case two runs are started on a one-way migration surface.
Disable both while start.isPending (disabled={start.isPending} on each, keeping loading as the per-button indicator) so the UI cannot issue the second start at all.
I4 — The result of a test run is never announced
The only live region in the component is the progress paragraph inside the run.status === "running" branch (CopyStep.tsx:63), and it unmounts the moment the run ends. The result then mounts in a plain <p> (CopyStep.tsx:129-140) and the per-item list in a plain <ul>, neither of them live. For a real copy that is partly covered, because finishing the step moves focus to the next step's heading (index.tsx:410), and a blocked step gets the role="alert" block. But a test run changes no step state and never completes the step, so a screen-reader user who activates "Test run" is told the button is busy and then told nothing at all — the counts, which are the entire point of a test run, arrive silently.
Make the result region aria-live="polite" (it is the same region in both states, so a single wrapper with the live attribute covers progress and result), or move focus to it when a dry run ends.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/migration-page-copy-database #15622 +/- ##
=====================================================================
+ Coverage 68.77% 69.91% +1.14%
=====================================================================
Files 2772 2777 +5
Lines 299091 300584 +1493
Branches 40299 41813 +1514
=====================================================================
+ Hits 205700 210166 +4466
+ Misses 90897 87924 -2973
Partials 2494 2494
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
9ca0557 to
53167b8
Compare
github-actions
Bot
added
enhancement
53167b8 to
6d4d3f9
Compare
github-actions
Bot
added
enhancement
erichare
left a comment
erichare
left a comment
There was a problem hiding this comment.
@ogabrielluiz Approving with four small fixes pushed on top. Thank you for matching the frontend types so exactly to the commands' --json output: copyCounts sums exactly the status literals both reports emit, and the stale/test-run logic in CopyStep mirrors _copy_step case by case.
Fixes I pushed
- 753713e has three
CopyStepfixes, each with a jest test that fails without it:- No "Test run" on a done copy step (L219).
admit()puts the dry run in the copy's slot of the record, and_copy_stepreturnscurrentfor any dry run. So a test run on a finished step reopened it, removed its "Copied X of Y" summary and locked the steps after it again. A backend probe showed('done', None)before the test run and('current', None)after it. - One start at a time (L208, L223). Both buttons now set
disabled={start.isPending}, andloadingstill marks the busy one. Before, a click on the other button sent a second POST, which came back asrun_active("running elsewhere") while your own copy was still starting. This was Cristhianzl's I3. - The test run's result is read out (L65, L137). The progress line and the counts are now the same keyed
role="status"node. React keeps that node when the run ends, so the counts land in a live region the screen reader already tracks. It stays in the tree while empty, using the sameempty:-mttrick asCheckStep. This was Cristhianzl's I4.
- No "Test run" on a done copy step (L219).
- b8918c3 rewords
files.badNamein all 8 locales.bad_namecomes from Langflow's own name check (_failure_code), and the local backend applies that check too, so the old text was wrong to blame S3. Renaming the file in Langflow also can't fix it, because the rename changes onlyFile.nameand not the stored name the copy reads (v2/files.py). The new text says what is true: the stored name has a backslash or..in it, and Langflow's file storage refuses that name.
#15623 and #15624 need a restack onto the new head.
Non-blocking
- No decision in
DECISIONScoversbad_name(migration_copies.py). A file whose stored name was refused therefore blocks "Copy files", and nothing on the page gets the admin past it. This belongs to the API PR, so I left the copy describing the cause without offering a remedy.
Nits
- Cristhianzl's I1: the relocate-files command always sends
bytes, so a literal{{bytes}}can't appear today. Makingbytesrequired on the file event, or usingformatFileSize(event.bytes ?? 0)(catalog.ts#L273), would put that guarantee in the types. runCopyin the e2e walk readsrun_idwithout checkingstarted.ok()(migration-walk.ts#L112). A refused start shows up later as a vague UI timeout instead of failing at the refusal.
Verification
npx jest src/pages/SettingsPage/pages/MigrationPage -> 4 suites, 90 passed (87 + 3 new)
new tests on the original CopyStep.tsx -> 3 failed (as expected)
npx @biomejs/biome check <touched files> -> clean
npm run check:i18n -> all 7 locales in parity
npx tsc --noEmit (filtered to MigrationPage/locales) -> no errors
753713e to
0d9f11f
Compare
|
Hey @erichare, thanks for the fixes. Each was on the old head of its PR, so I put them in one line and pushed #15622 to #15633 again. Your commits are all there, still yours. Two of them changed to sit on each other: Your status line from #15621 and the one from this PR are one node now. It shows the progress, and after the run the counts (or the database result for a screen reader). Your tests for both pass as you wrote them. In #15624 the two gate tests asked for the only |
github-actions
Bot
added
enhancement
github-actions
Bot
added
enhancement
e1df335 to
2083249
Compare
github-actions
Bot
added
enhancement
2083249 to
1a56945
Compare
github-actions
Bot
added
enhancement
1a56945 to
67ff094
Compare
github-actions
Bot
added
enhancement
67ff094 to
4cfc983
Compare
github-actions
Bot
added
enhancement
4cfc983 to
d3e9609
Compare
…on page "Copy knowledge bases" and "Copy files" use the panel of the database copy. Both can be tried first: a test run says what a copy would do and completes nothing. After a run the step shows what was copied, what was there already and what was not. Each knowledge base or file that was not copied is listed with the page's line for its code and the command's own words under Details. The record keeps the first hundred of them, and the step says so when there are more. On an instance that already runs on PostgreSQL the knowledge base copy changes this instance's own rows, and the step says so before it starts. A copy that is out of date shows none of what it found. The steps after the copies are not built yet. Once the copies are done the server holds them back, and their rows read "Coming soon".
The bad_name line blamed S3 and told the admin to rename the file in Langflow. The code comes from Langflow's own storage rule, which the local backend applies too, and a rename changes only the display name, not the stored name the copy reads, so the advice could not help.
…out its result - Offer no test run on a done copy step: the server puts the test run in the copy's place, so the step waited again and the later steps locked. - Disable both start buttons while a start is on its way, so a second click no longer sends another start that is refused as running elsewhere. - Keep one live region from a run's progress line to what it found, so a screen reader reads out a test run's counts.
3 participants
Stacked on the PR below it, which adds the database copy and the panel this one reuses. Settings > Migration gets its seventh and eighth steps, "Copy knowledge bases" and "Copy files". It needs the two copies and their test runs from the API PRs further down the stack.
What it adds
Both steps can be tried first. "Test run" asks the server what a copy would do and copies nothing, and a test run never completes its step. A step whose copy is done offers no test run, because a test run would take the copy's place in the record. While a start is on its way both buttons are disabled. After a test run or a copy the step shows three counts: copied (or to copy), already copied, and not copied. The counts stand in the same live region as the progress line, so a screen reader hears what a copy or a test run found.
Each knowledge base or file that was not copied is listed by name and owner, with the page's line for its code and the command's own words under Details. The record keeps the first hundred of them, and the step says so when more failed. When one of them is what stops the step, the step's own line points at the list.
Progress follows the commands: the entries of the knowledge base being copied, and the files with how much of them was copied so far.
On an instance that already runs on PostgreSQL, the knowledge base copy changes this instance's own rows. The step says so before the copy starts.
A step whose copy is done shows how many were copied, out of how many, in its row.
A copy the server no longer counts shows none of what it found, and the step says the copy has to be made again. That is a copy made before changes were last turned back on, or a knowledge base copy made before the database was copied again.
The two steps after the copies are not built yet. Once the copies are done the server holds them back, and their rows read "Coming soon".
Try it
Finish the first six steps as in the PRs below, on an instance that has a knowledge base and an uploaded file. The destination database needs the
vectorextension (CREATE EXTENSION vector;). Click "Test run" on "Copy knowledge bases", read the counts, then copy. Do the same for the files.To see the list of what was not copied, delete an uploaded file from the instance's disk before the first step. The check reports it, and you accept that finding there. The file copy then lists the file with its reason.
How it was tested
Jest has 16 new tests. They cover the test run and what it sends, the counts of a test run and of a copy, each line of the list for knowledge bases and for files, a list longer than the record keeps, two chat messages that miss the same file, a copy that is out of date, a run that ended without a report, the note for PostgreSQL, the done rows, the row of a step the server holds back, one start at a time, the counts read out where the progress was, and no test run on a done step. Jest sends no request. I planted 41 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.tsgains a walk through both steps in a browser, against a real backend, a real PostgreSQL database with pgvector and a real S3 bucket: a test run and then the copy, for each step, and the focus moving to the files once the knowledge bases are done. On an instance with no knowledge base on its server the walk checks the "Not needed" line for that step. 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.I ran it on an instance with one knowledge base of 12 entries, made through the app's own services, and two uploaded files. Afterwards the destination's
knowledge_baserow saidpostgresand its vector table held 12 rows. This instance's own row still saidsqlite, and the bucket held both files.By hand against the real API, I copied the database and the knowledge bases, then copied the database again. The knowledge base step waited again, said its copy was out of date and showed no counts from it. With every copy made, the rows of "Start the new instance" and "Check the new instance" read "Coming soon".
npm run check:i18npasses with the 23 new strings in all 8 locales. Biome is clean on the touched files, and the type check shows no new error.Summary by CodeRabbit