Skip to content

Commit 626b045

Browse files
fix(core-components): close the field-mount race and repair the doc edit (#1109)
From an independent review of the promotion. expectFieldIdsUniquePerNode read the field ids through evaluateAll, which resolves the selector once with no auto-wait. The only prior gate was .react-flow__node reaching 2, which proves the NODES mounted, not their parameter fields, so a field arriving one commit later read as `got 0`. That is a hard, well-named failure rather than a vacuous pass -- but on the daily a hard failure strips @stable automatically and commits to main, so the race is worth closing rather than triaging later. Force-failed at toHaveCount(3) on both cases. Doc repairs, all introduced by the promotion edit itself: - the Tags section opened on a truncated sentence ("and it now has"); - rewriting Preconditions removed the only bullet identifying which build the "build carrying langflow#14312" measurement row referred to, leaving that row unanchored and undatable; it is now named inline; - the row added for 1.12.0.dev18 recorded literal DOM ids whose suffix is a per-node nonce regenerated on every mount -- four runs produced four different pairs -- so it read as if a literal suffix were a contract; - Tags and Preconditions both asserted the no-credential claim and pointed at each other; Preconditions now carries the measurement (green with no provider configured locally, green after Collect models on the PR lane) and Tags the rationale; - the @agentS note understated the tooling: provider-dependent-specs.mjs reads the tag file-scoped as a provider declaration, which both forces the Collect models sweep on a PR that changes this spec and excludes it from a PR that only changes a helper it imports. Also records that the canvas sweep only widens coverage on Case B: a two-node API Request canvas carries exactly 2 form controls with an id, the same two the field assertion already covers, while the two-Agent canvas carries 4.
1 parent 2aa69ff commit 626b045

2 files changed

Lines changed: 21 additions & 7 deletions

File tree

docs/core-components/duplicate-dom-ids-regression.md

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ If this test fails, either two nodes are colliding on a DOM id again (a renderer
2525

2626
Case A: `@stable` `@regression` `@components` · Case B: `@stable` `@regression` `@components` `@agents`
2727

28-
**`@stable` was deliberately withheld until the fix reached the 1.12 line, and it now has (#1109).** When this spec was merged (#1102 / PR #1106) the upstream fix existed only on the **`release-1.11.2`** branch, while `langflowai/langflow-nightly:latest` — the image `daily-stable.yml` runs — is built from the highest `release-*` branch. Both cases therefore hard-failed on the nightly *by design*, and tagging them `@stable` then would have opened a `daily-failure` issue every weekday and tripped `auto-remove-stable` into stripping the tag and committing to `main`. The helper now resolves on `main` **and** on `release-1.12.0` at the same sha (`a8a903e`), and both cases run green on `1.12.0.dev18`:
28+
**`@stable` was deliberately withheld until the upstream fix reached the 1.12 line; it has, and the tag was added by #1109.** When this spec was merged (#1102 / PR #1106) the upstream fix existed only on the **`release-1.11.2`** branch, while `langflowai/langflow-nightly:latest` — the image `daily-stable.yml` runs — is built from the highest `release-*` branch. Both cases therefore hard-failed on the nightly *by design*, and tagging them `@stable` then would have opened a `daily-failure` issue every weekday and tripped `auto-remove-stable` into stripping the tag and committing to `main`. The helper now resolves on `main` **and** on `release-1.12.0` at the same sha (`a8a903e`), and both cases run green on `1.12.0.dev18`:
2929

3030
```bash
3131
gh api repos/langflow-ai/langflow/contents/src/frontend/src/components/core/parameterRenderComponent/helpers/get-node-scoped-dom-id.ts?ref=release-1.12.0 --jq .sha
@@ -35,7 +35,7 @@ Note the ref that matters is the **release line the nightly is cut from**, not `
3535

3636
`@release` remains deliberately absent: this is a DOM-contract / accessibility regression guard, not a happy-path flow required before a deploy.
3737

38-
`@agents` on Case B is a **subject** tag, not a dependency one: the Agent node is placed and inspected, never executed, so the case needs no provider credential (see *Preconditions*).
38+
`@agents` on Case B names the **subject**, not a dependency — but the tooling reads it as one, and the mismatch has consequences worth knowing. `scripts/provider-dependent-specs.mjs` treats `@agents` as a provider declaration **file-scoped**, so it marks both cases. Two effects on `pr-validation.yml`: a PR that changes this spec **forces** the `Collect models` sweep (a hard gate — so a tag-only PR here is coupled to provider-key health, #772/#1029/#1169), while a PR that only changes a helper this spec imports gets it **excluded and announced** instead of run. Neither is wrong for the mechanism; both rest on a premise this file does not meet, since it places and inspects the Agent without ever executing it.
3939

4040
---
4141

@@ -47,7 +47,7 @@ Note the ref that matters is the **release line the nightly is cut from**, not `
4747
2. `addComponentFromSidebar(page, "API Request", "add-component-button-api-request")`.
4848
3. Gate on `title-API Request` being visible before adding the second node — the sidebar click is fire-and-forget, and asserting the count immediately can observe 1 while the second node is still mounting.
4949
4. Add the second API Request the same way; assert `.react-flow__node` count is `2`.
50-
5. Read the `id` of every element matching `popover-anchor-input-url_input` and assert: exactly 2 elements, both with a non-empty id, and the two ids distinct.
50+
5. Wait for `popover-anchor-input-url_input` to reach 2 elements, then read the `id` of every one and assert: exactly 2 elements, both with a non-empty id, and the two ids distinct. The wait is load-bearing: `evaluateAll` resolves the selector once with no auto-wait, and the preceding `.react-flow__node` count proves the **nodes** mounted, not their parameter fields — a field arriving one commit later would read as `got 0`/`got 1`. Hard failure, never a vacuous pass, but on the daily a hard failure strips `@stable` automatically (#1109).
5151
6. Sweep `input[id], textarea[id], select[id]` **inside the canvas** and assert no id appears more than once.
5252

5353
**Case B — two Agent nodes** (`TextAreaComponent` id path)
@@ -82,16 +82,20 @@ Measured behaviour on both sides of the upstream fix, two nodes on canvas:
8282
| build | Case A duplicates | Case B duplicates |
8383
|---|---|---|
8484
| nightly `1.12.0.dev9` (pre-fix) | `popover-anchor-input-url_input x2` | `popover-anchor-input-input_value x2`, `textarea_str_system_prompt x2` |
85-
| build carrying langflow#14312 | none — ids read `…-APIRequest-<suffix>` | none — ids read `…-Agent-<suffix>` |
86-
| nightly `1.12.0.dev18` (post-merge-back, the promotion baseline) | none — `popover-anchor-input-url_input-APIRequest-P55sP` / `-APIRequest-c5ewx` | none — `textarea_str_system_prompt-Agent-k1lgI` / `-Agent-mBubM`, and `popover-anchor-input-input_value` scoped likewise |
85+
| local build off `release-1.11.2` carrying langflow#14312 — the original validation, #1106 | none — ids read `popover-anchor-input-url_input-APIRequest-<suffix>` | none — ids read `textarea_str_system_prompt-Agent-<suffix>` |
86+
| nightly `1.12.0.dev18` (post-merge-back, the promotion baseline, #1109) | none — same shape | none — same shape, and `popover-anchor-input-input_value-Agent-<suffix>` scoped likewise |
87+
88+
`<suffix>` is a **per-node nonce**, regenerated on every mount — four consecutive runs produced four different pairs. Only the *shape* is a contract; a literal suffix never is.
8789

8890
In every build the field **testid** resolved to 2 elements — the contract half staying green across the fix.
8991

9092
### Scope of the sweep, and what that costs
9193

9294
**Canvas-scoped, not document-scoped.** The parameters side panel renders the same field with the **same DOM id** as the node body: `popover/index.tsx` applies `getNodeScopedDomId(id, nodeId)` unconditionally, and only `data-testid` gets the `-edit` suffix in edit mode. A document-wide sweep would therefore report a duplicate whenever that panel is open for a selected node — a false failure with no LE-2037 regression behind it. Scoping to `#react-flow-id` also excludes app-chrome and portal noise. The sweep throws if the canvas root is missing, so a selector change surfaces as an error instead of an empty list that would pass vacuously.
9395

94-
**Form controls only** (`input` / `textarea` / `select`) — what the DevTools warning covers, what breaks autofill, and what upstream's own regression test sweeps. Icon SVGs legitimately repeat their internal ids (gradients, masks, filters) whenever the same icon renders twice and must not fail this test for the wrong reason. **The cost is real:** of the 15 `id=` sites langflow#14312 edited, those whose id lands on a `span`, a `div`, a contenteditable `div` or a Radix `button[role="switch"]``promptComponent`, `mustachePromptComponent`, `accordionPromptComponent`, `emptyParameterComponent`, `toggleShadComponent` — are **not** reached by this sweep. The per-field assertions cover the two paths this spec claims regardless of element type; the broader sweep is a bonus net over form controls only. (`select[id]` is defensive: Langflow renders Radix Select, not a native `<select>`.)
96+
**Form controls only** (`input` / `textarea` / `select`) — what the DevTools warning covers, what breaks autofill, and what upstream's own regression test sweeps. Icon SVGs legitimately repeat their internal ids (gradients, masks, filters) whenever the same icon renders twice and must not fail this test for the wrong reason. **The cost is real:** of the 15 `id=` sites langflow#14312 edited, those whose id lands on a `span`, a `div`, a contenteditable `div` or a Radix `button[role="switch"]``promptComponent`, `mustachePromptComponent`, `accordionPromptComponent`, `emptyParameterComponent`, `toggleShadComponent` — are **not** reached by this sweep. The per-field assertions cover the two paths this spec claims regardless of element type; the broader sweep is a bonus net over form controls only.
97+
98+
**And that bonus is real for Case B only.** Measured on `1.12.0.dev18`: a two-node API Request canvas carries exactly **2** form controls with an id — the same two the field assertion already covers, so Case A's sweep restates its own second step. The two-Agent canvas carries **4**, and the extra pair (`popover-anchor-input-input_value`) is a genuine pre-fix duplicate the field assertion would miss. Keeping the sweep in Case A is still worth its ~0 s: it is the step that would catch a *newly added* form control regressing, which is exactly the change this spec is meant to be a tripwire for. (`select[id]` is defensive: Langflow renders Radix Select, not a native `<select>`.)
9599

96100
---
97101

@@ -127,7 +131,7 @@ In every build the field **testid** resolved to 2 elements — the contract half
127131
## Preconditions *(optional)*
128132

129133
- Langflow running at `PLAYWRIGHT_BASE_URL` on a build that includes langflow#14312 — the `release-1.11.2` line and, since the merge-back, `main` / `release-1.12.0` and therefore `langflowai/langflow-nightly:latest`. On an older build without the helper both cases fail **by design**; that is the negative control, not a defect in the test. The **published** `langflowai/langflow:1.11.1` image does not contain the helper, so it is not a reproduction target.
130-
- No model provider credentials requiredthe nodes are placed and inspected, never executed, so the Agent case makes no LLM call. This holds even though Case B carries `@agents`.
134+
- No model provider credentials required. Measured in both environment states: green locally with the preflight explicitly warning that no provider key was configured as a Langflow global variable, and green on the PR lane *after* `Collect models` had configured them. See *Tags* for why Case B still carries `@agents`.
131135

132136
---
133137

tests/tests-automations/regression/core-components/duplicate-dom-ids-regression.spec.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,16 @@ async function expectFieldIdsUniquePerNode(
140140
page: Page,
141141
fieldTestId: string,
142142
): Promise<void> {
143+
// `evaluateAll` resolves the selector once — no auto-wait, no retry. The only
144+
// prior gate is `.react-flow__node` reaching 2, which says the NODES mounted,
145+
// not that their parameter fields did; a field arriving one commit later reads
146+
// as `got 0`/`got 1`. A hard, well-named failure rather than a vacuous pass —
147+
// but on the daily a hard failure strips `@stable` automatically and commits to
148+
// `main`, so the race is worth closing rather than triaging later.
149+
await expect(page.getByTestId(fieldTestId)).toHaveCount(2, {
150+
timeout: 15000,
151+
});
152+
143153
const ids = await page
144154
.getByTestId(fieldTestId)
145155
.evaluateAll((elements) => elements.map((element) => element.id));

0 commit comments

Comments
 (0)