fix(mcp): re-open the flow under test by id in mcp-server (#1340) - #1341
Conversation
Both re-opens in "mcp server tools should be refreshed when editing a server" clicked the first `list-card` whose name contained "New Flow". Langflow names every blank flow "New Flow"/"New Flow (N)", so under `fullyParallel` the shared project holds one per worker and `.first()` resolves whichever card the list puts first. Measured on nightly 1.12.0.dev18, and the measurement is the point: in isolation the test's own flow ranks first and the click is correct, which is why this never produced a recorded daily failure. Seeding ONE competing "New Flow ..." in the same project before the list fetch flips it — the rendered order became ["New Flow probeB-...", "New Flow (1)", "Basic Prompting"], the click opened the competitor, and the test then died on the `text="MCP Tools"` wait at 30 s, blaming the node for a flow it was never in. The same locator cost `auto-save-off.spec.ts` two dailies before it was diagnosed (#1336). Use `openFlowById` (#1214) — the repo's by-id entry, whose own header states that a name-filtered `list-card` click is exactly what makes an entry parallel-unsafe. It adds two guarantees the card click never had: the assistant-onboarding flag is seeded before the load, and the flow is gated on being writable, which is the #1005 window where a mutation is silently swallowed. This test mutates the flow it re-opens. The id is read AFTER the blank-flow navigation, never before it: the bootstrap reaches the templates modal through "New Flow", which parks the page on a placeholder flow Langflow deletes as soon as the modal navigates elsewhere (#490/#681). No assertion changes: what the test proves is unchanged, it now proves it about the flow it actually created.
Victor-w-Madeira
added
the
follow-up
There was a problem hiding this comment.
Pull request overview
This PR fixes a parallel-safety hazard in the MCP server regression spec by changing flow re-open steps to use the repo’s canonical by-id navigation helper (openFlowById) instead of selecting a flow card by a non-unique “New Flow” name.
Changes:
- Update
mcp-server.spec.tsto re-open the flow under test viaopenFlowById(page, flowId)for both re-open points. - Add logic to capture the flow id after creating the blank flow, and document the rationale in-line.
- Update the spec doc (
docs/mcp/server/mcp-server.md) to reflect the by-id re-open requirement and record the #1340 measurement/notes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/tests-automations/regression/mcp/server/mcp-server.spec.ts | Replaces name-filtered flow-card reopen with openFlowById and introduces flow id tracking for the flow under test. |
| docs/mcp/server/mcp-server.md | Aligns documentation with by-id reopen behavior and adds a note explaining the parallel hazard (#1340). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // The flow under test has to be addressed by id from here on (#1340). The | ||
| // id is read AFTER this navigation, never before it: `awaitBootstrapTest` | ||
| // reaches the templates modal through "New Flow", which parks the page on a | ||
| // placeholder flow Langflow deletes as soon as the modal navigates | ||
| // elsewhere (#490/#681). | ||
| const placeholderUrl = page.url(); | ||
| await page.getByTestId("blank-flow").click(); | ||
| await page.waitForURL( | ||
| (url) => | ||
| /\/flow\/[0-9a-f-]{36}/.test(url.pathname) && | ||
| url.toString() !== placeholderUrl, | ||
| { timeout: 30000 }, | ||
| ); | ||
| const flowUnderTest = new URL(page.url()).pathname.match( | ||
| /\/flow\/([0-9a-f-]{36})/, | ||
| )![1]; |
There was a problem hiding this comment.
Substantially right on both halves — addressed in cf27946, though not by replacing the URL read.
On the convention. You are correct that this file's Pattern-A note says only the POST /api/v1/flows 201 responses are trustworthy, and my placeholderUrl guard answered the staleness without answering that. But taking the id from the response instead is weaker here, and the reason is specific to this test: the page creates two flows — the bootstrap placeholder and the blank flow — so createdFlowIds holds both, and picking one means trusting the arrival order of two async resp.json() reads. The wrong pick is precisely the id Langflow deletes when the templates modal navigates away, which would reproduce #1340's symptom from the other direction.
Neither source alone is sufficient, so the fix now requires both: the URL says which flow the editor ended up on, and membership in the tracked 201 ids says this page created it and that it is persisted. A transient or client-only id cannot satisfy the membership test, which closes the residual gap in the URL read you point at.
On the 30 s hang. Fair, and fixed. The waitForURL became an expect.poll over the conjunction with an explicit message, so a blank-flow click that stops navigating fails at that step naming the cause instead of as a bare waitForURL timeout. Verified by mutating the predicate to an impossible id — the message does reach the report:
Error: the blank-flow click never landed the editor on a newly created flow: the URL
still holds the bootstrap placeholder, or its id is not among this page's
POST /api/v1/flows 201 responses
One correction to the premise, stated because the PR should not imply a defect it did not measure: the placeholder is not reused today. Measured 5/5 on nightly 1.12.0.dev18 — the blank-flow click issues its own POST /flows 201 and moves the URL every time. So this change is about convention-compliance and attribution, not about a hang anyone has hit.
Review feedback on PR #1341: deriving the flow under test from `page.url()` alone sits against this file's own Pattern-A note, which says the canvas URL id around the templates-modal hop is the stale bootstrap one and only the `POST /api/v1/flows` 201 responses are trustworthy (#681/#505). The "is it different from the placeholder URL" guard addressed the staleness but not the convention, and it would spend 30 s in `waitForURL` before saying anything if the click ever stopped navigating. Neither source alone is sufficient here, so require both. The URL says which flow the editor ended up on; membership in the tracked 201 ids says the page actually created it and that it is persisted — which the raw URL cannot, since this page creates the placeholder AND the blank flow, and picking one from the id list means trusting the arrival order of two async body reads, where the wrong pick is exactly the id Langflow deletes. Polling the conjunction also fixes the attribution: a transient or client-only id can never satisfy membership, and a blank-flow click that fails to navigate now fails at that step naming the cause instead of as a bare waitForURL timeout. Verified by mutating the predicate to an impossible id — the message reaches the report. No behaviour change on the happy path: measured 5/5 on nightly 1.12.0.dev18 that the blank-flow click issues its own POST /flows 201 and moves the URL, so the placeholder is never reused.
Fixes #1340. Closes #1340.
Problem
Both re-opens in test 5 (
mcp server tools should be refreshed when editing a server,@stable) clicked the firstlist-cardwhose name contained"New Flow". Langflow names every blank flowNew Flow/New Flow (N), so underfullyParallelthe shared project holds one per worker and.first()resolves whichever card the list puts first.The issue asked to measure before assuming, and the measurement is the interesting part:
updated_at), both re-opens land on the right id, and the daily history holds no recorded failure for this test. Latent, exactly as filed.New Flow …in the same project before the list fetch flips it. Rendered order became["New Flow probeB-p0d7n", "New Flow (1)", "Basic Prompting"]; the click opened the competitor (match=false), and the test then died onpage.waitForSelector('text="MCP Tools"')at 30 s — an error blaming the MCP Tools node for a flow the test was never in. That is the same locator, and the same misdirected symptom, that costauto-save-off.spec.tstwo dailies before it was diagnosed ([Daily #1330] auto-save-off — reopening the just-created flow's card intermittently times out at 45 s (recurrent 2×) #1336).Two dead ends worth recording, because both produced a false negative on the first try: a competitor created without
folder_idlands in another project and never reaches this list, and a competitor created after the page's list fetch never triggers a refetch. Neither is a property of the spec — they were bugs in the probe.Fix
Use
openFlowById(#1214) for both re-opens. It is the repo's canonical by-id entry, and its own header already states the rule this spec was breaking: "Addressing the flow by id — neverlist-card.first()or a name-filteredlist-card-open-buttonclick — is what makes the entry parallel-safe." It also brings two guarantees the card click never had:POST /api/v1/authz/me/permissionsis in flight is silently swallowed. This test mutates the flow it re-opens, so the gate is wanted.The flow id is read after the blank-flow navigation, never before it: the bootstrap reaches the templates modal through "New Flow", which parks the page on a placeholder flow Langflow deletes as soon as the modal navigates elsewhere (#490/#681).
The two
awaitBootstrapTest(page, { skipModal: true })hops are kept as-is. They are now redundant withopenFlowById's own document load, but removing them would change how the test leaves the canvas — out of scope for a locator fix.Scope note
No assertion changed. What the test proves is identical; it now proves it about the flow it actually created.
helpers/mcp/open-add-mcp-server-modal.tsis deliberately not touched — see below.Validation (nightly
1.12.0.dev18,--retries=0)npm run typecheck✅ ·npx eslint <spec>✅ (0 errors; 103 pre-existing style warnings)text="MCP Tools"(30 s)match=true) by instrumentationNew Floworphans and notest_server_*left on the instance after the runs (GET /api/v1/flows/,GET /api/v2/mcp/servers)Honest caveat on the burst the issue asked for. A
--workers=4burst of this spec cannot validate the re-open, and did not: 3 of 4 runs failed athelpers/mcp/open-add-mcp-server-modal.ts:10(mcp-server-dropdown,locator.clicktimeout) — always before the changed code. This is pre-existing and separately filed as #1335. Confirmed by a control run of the unmodified spec onmain: same 2/3 pass rate, same step. Raising that budget from 3 s to 30 s locally did not help under--workers=2+— the dropdown simply never becomes clickable when copies of this MCP spec run concurrently, which is a self-inflicted condition (the daily runs it once). That budget belongs to #1335 and to a shared helper with other callers, so it stays untouched here; the measurement is posted on #1335.