Repository navigation
fix(frontend): fold what the migration rehearsal found into the page - #15624
ogabrielluiz wants to merge 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⚙️ Run configuration
📒 Files selected for processing (19)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review. WalkthroughThe migration settings page adds PostgreSQL knowledge-base destination guidance, distinguishes pgvector probe errors, gates copy controls on earlier step status, and supports retrying a pause after active requests are reported. Locale text and migration tests are updated for these changes. ChangesMigration settings
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The migration-page changes are mergeable after normal checks. 🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 10 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
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/migration-page-copy-decisions #15624 +/- ##
======================================================================
- Coverage 68.80% 68.80% -0.01%
======================================================================
Files 2769 2769
Lines 298493 298484 -9
Branches 43156 43152 -4
======================================================================
- Hits 205374 205365 -9
Misses 90625 90625
Partials 2494 2494
🚀 New features to boost your workflow:
|
github-actions
Bot
added
bug
Cristhianzl
left a comment
Cristhianzl
left a comment
There was a problem hiding this comment.
⚠️ Important (preferably this PR)
I1 — The copy step's new line was written as if no note stood above it, and one does
File: src/frontend/src/pages/SettingsPage/pages/MigrationPage/CopyStep.tsx:160-163 and :177-187; string settings.migration.kb.postgresEnv
Issue: The PR body justifies the shape of the new line explicitly — "No note stands above it, so the line says why as well, and it ends with the copy" — and the new test repeats the premise in a comment: "No note stands above this one, so it says why as well." The premise does not hold. The step renders a note under exactly the condition that produces the refusal:
{step === "copy_knowledge_bases" &&
migration.instance.database.type === "postgresql" && (
<p className="text-sm">{t("settings.migration.kb.postgresNote")}</p>
)}pgvector_env_missing comes from _unreadable_here (src/backend/base/langflow/api/v1/migration.py:941-948), which is postgresql and not postgres_env_configured() — so the note is always above the new line, never sometimes. What the admin reads is:
This instance uses PostgreSQL, so it switches to the new store too. Restoring the backup undoes it.
This instance already uses PostgreSQL, so its knowledge bases are copied to the store this server reads them from. Set PGVECTOR_CONNECTION_STRING on this server to that database and restart Langflow, then copy again.
Why it matters: The clause that was deliberately dropped from probe.pgvectorEnv — on the correct grounds that the note above it carries the "where they go" — was deliberately kept in kb.postgresEnv on grounds that are false. So the one surface that duplicates is the one that was reasoned about as not duplicating, in all 8 locales. It also means a reader comparing the two strings will conclude the asymmetry is intentional and leave it.
Suggested fix: Shorten kb.postgresEnv to the action alone — the same decision already taken for probe.pgvectorEnv — since kb.postgresNote is the note above it. Then add the missing assertion to the new test, which currently renders the duplicated state and says nothing about it:
expect(screen.queryByText(/^This instance uses PostgreSQL/)).toBeInTheDocument();
expect(screen.getByRole("alert").firstElementChild).toHaveTextContent(/^Set PGVECTOR_CONNECTION_STRING/);If the duplication is wanted after all — because the alert may be read on its own by a screen reader while the note is plain text — say that in the comment above the COPY_CODES entry, so the next person does not "fix" it.
I2 — The new gate unmounts the button the admin may be standing on, and nothing announces what replaced it
File: src/frontend/src/pages/SettingsPage/pages/MigrationPage/CopyStep.tsx:275-318
Issue: reached ? <buttons> : <p> removes the start buttons from the DOM. The transition happens asynchronously, not on a user action: useMigrationQuery polls every 5 s while a check is running (src/frontend/src/controllers/API/queries/migration/use-migration.ts:29-32), and useStartCopyMutation invalidates on onSettled (:180-181). Both are the rehearsal scenario from the body — another admin re-runs the check, or this tab's start comes back 409 locked. A keyboard user focused on "Copy again" has focus dropped to <body>, and the line that replaced it is a plain paragraph in no live region:
) : (
<p className="text-sm">{t("settings.migration.notStarted")}</p>
)}Why it matters: This is the exact failure mode the same file refuses elsewhere, with the reason written down at CopyStep.tsx:353: "aria-disabled, since native disabled would drop the keyboard user's focus to <body>." The gate is new behaviour introduced here, so the regression is this diff's. The two new Jest tests assert the rendered text in a static state and never drive the reached: true → false transition — both pass migration.steps arrays that are fixed for the life of the test — so nothing would catch it.
Suggested fix: Keep the buttons mounted and apply the pattern the file already uses — aria-disabled, a guarded onClick, and aria-describedby pointing at the notStarted line — or, if they should really disappear, give the paragraph role="status" and move focus to it. Either way, add a test that re-renders the same component with an earlier step flipped from done to blocked and asserts where focus landed; that is the assertion that fails without the fix.
b9db7ac to
9e814fc
Compare
github-actions
Bot
added
bug
9e814fc to
94af7f1
Compare
github-actions
Bot
added
bug
a4be90a to
66a9f06
Compare
github-actions
Bot
added
bug
github-actions
Bot
added
bug
66a9f06 to
5923f40
Compare
github-actions
Bot
added
bug
5923f40 to
84c0226
Compare
github-actions
Bot
added
bug
84c0226 to
46923fc
Compare
github-actions
Bot
added
bug
46923fc to
e32b268
Compare
A rehearsal of the whole move on real instances found these. The document itself could scroll on the migration page and hide the top bar. Each locked step holds a line that only a screen reader hears. It is positioned out of the flow, and nothing between it and the document was positioned, so it was placed against the document and made it taller than the window. The page's root is positioned now. The knowledge bases test of "Where your data goes" has two more reasons with a line each: the server has no pgvector package, and an instance that already runs on PostgreSQL has no PGVECTOR_CONNECTION_STRING. On such an instance the note at the test says the knowledge bases go to the store that variable names, where it said the new instance's database, and the refusal under it is what to do. The missing variable also stops "Copy knowledge bases" and refuses its start. No note stands there, so that line says why as well, and it ends with the copy. A pause that is refused because changes under way had not finished says to try again in a moment, and trying again does not ask twice. The walks cover the document that stays put, a backup confirmed with no place named, which the browser's own message stops, and a destination step on an instance that has knowledge bases.
… again A copy that ran keeps its place when a step before it opens again, for example when the check fails again during the pause. The server refuses to start it then. The page still offered "Copy again" and answered the refusal with the line it has for a failure it cannot name. A copy step now offers no start, real or test, while a step before it is neither done nor skipped. It says "Finish the steps above first." where the buttons were, and what the copy did stays on show. A tab that has not read the new state yet can still send a start. The server answers that it is locked, and the step says the same line. The line under an option the admin ticked said "Copy again for this to apply." After a test run no copy exists yet, so it now says that the choice applies to the next copy, in the 8 locales.
When a read of the state reopens a step above a copy, the start buttons go away. Focus then fell to the document body and the line in their place was not announced. That line is now a status, and it takes the focus when the buttons it replaces held it. Also correct the comments on the knowledge bases line for a missing PGVECTOR_CONNECTION_STRING: the PostgreSQL note always stands above it, and the test now asserts that note.
This branch has not been deployed
3 participants
Stacked on the PR below it. A rehearsal of the whole move, in a real browser on real instances, found these on Settings > Migration. They touch steps whose PRs are already approved, so they come as one PR on top of the page stack. It needs the three new refusal codes from the API PRs further down the stack.
What it fixes
The document itself could scroll and hide the top bar and the paused banner. Each locked step holds a line that only a screen reader hears. It is positioned out of the flow, and nothing between it and the document was positioned, so each one was placed against the document at its place in the list. With steps below the fold the document grew taller than the window. A focus or a find in page could then scroll the document, and the wheel could not bring it back. The page's root is positioned now, so those lines stay inside the settings scroll area.
The knowledge bases test of "Where your data goes" told every admin to ask the database admin for an extension. It now has a line for each of three reasons. The database lacks the pgvector extension. This Langflow server lacks the pgvector package, which a plain install does not include: install it, restart Langflow, test again. Or the instance already runs on PostgreSQL and its server has no
PGVECTOR_CONNECTION_STRING: set it to that database, restart Langflow, test again. That last line is the action alone, because the note above it already says where the knowledge bases go. The server's own words stay under each line.On an instance that already runs on PostgreSQL, the note in that section said knowledge bases are copied into the new instance's database. There they go to the store this server reads them from, the one that variable names, and the note says so.
"Copy knowledge bases" reads the missing variable too, as the reason the step is blocked and as the refusal of its start. It has a line of its own there. The note above it says the store changes and does not say which one, so the line names the store before it asks for the variable, and it ends with the copy.
A pause that is refused because changes already under way had not finished says to try again in a moment. Trying again does not ask a second time, since the admin already agreed to this pause.
A copy that ran keeps its place when a step before it opens again, for example when the check fails again during the pause. The server refuses to start it then, and the page still offered "Copy again" and answered with its line for a failure it cannot name. A copy step now offers no start, real or test, while a step before it is neither done nor skipped, and says "Finish the steps above first." where the buttons were. That line is a status, and it takes the focus when the buttons it replaces held it. A tab that has not read the new state yet can still send a start, and it gets the same line. The line under a ticked option now reads "This applies to the next copy.", because "Copy again for this to apply." was wrong after a test run, when no copy exists yet.
Try it
On /settings/migration, in the console:
document.querySelector('[data-testid="migration-step-check_target"]').scrollIntoView(), thendocument.scrollingElement.scrollTop. It reads 0, and the top bar stays where it is.On a Langflow server without the pgvector package, with a knowledge base on its disk, click "Test and save" in "Where your data goes". The Knowledge bases section says the package is missing.
Pause, make the copies, then make the check fail again, for example by removing the bytes of a listed file and running the check again. Open "Copy the database". It shows what it copied and "Finish the steps above first.", with no button.
How it was tested
Jest has 10 new tests: the three reasons of the knowledge bases test, each in its section with the server's words under it and the database field left unmarked, the note on a PostgreSQL instance, the blocked copy step and its refused start in the step's own words, and the refused pause with its second try. The last three are for the copy steps: no start while a step before it is open again, the start that stays when every step before it is done or not needed, and the locked refusal in the step's words. Two are for the focus: it moves to the line when the gate closes under a focused start, and it stays where it is otherwise. I planted 23 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.
The walks check the rest in a browser against a real backend.
migration-checks.spec.ts, which runs in CI, scrolls the last step into view and checks the document did not move. Without the fix that read 508 at 1280x720.migration-pause.spec.tsconfirms the backup with no place named: the field takes the focus, the browser shows its own required-field message and nothing is confirmed. The form already worked that way, and the walk now says so.migration-prepare.spec.tspasses on an instance that has knowledge bases, where its "Ready." matched two lines before.By hand against real servers, I saw each of the new lines. On an instance that already runs on PostgreSQL, with a knowledge base on its disk and no
PGVECTOR_CONNECTION_STRING, "Where your data goes" showed the new note and the line that asks for the variable, with the server's words under it. With the variable set the test passed, and the instance went on to the copies. With the server started again without it, "Copy knowledge bases" read its own line for the missing variable, and its start was refused with it. For the missing package I hidpgvectorfrom one server process, and the Knowledge bases section said the package is missing while the database field stayed unmarked. With the three copies made and the check made to fail again, each copy row opened with its result and "Finish the steps above first.", and offered no start. A second tab that still showed "Copy again" had its start refused as locked and showed the same line.npm run check:i18npasses with the 5 new strings in all 8 locales. Each sits next to the strings of its own step. One string of the decisions PR is reworded in all 8. Biome is clean on the touched files, and the type check shows no new error.Summary by CodeRabbit