Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions QA-CHECKLIST.md
Original file line number Diff line number Diff line change
Expand Up @@ -258,11 +258,12 @@
- [-] Isolation flow: user A cannot see user B's flows

#### 4.3 Global Variables (API Keys)
- [-] Create global variable
- [x] Create global variable
- [ ] Use global variable in component (API key)
- [-] Edit existing global variable
- [-] Delete global variable
- [-] Create global variable of type "Generic"
- [x] Delete global variable
- [x] Create global variable of type "Generic"
- [x] Credential variable value is hidden from the variable list

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declining this one — the Copilot suggestion contradicts the convention in this repo.

CLAUDE.md §QA-CHECKLIST.md (lines 172-177) explicitly states that the Coverage Summary table and the Phase 0 — Validated block (counts + bulleted list) are auto-generated and must never be edited manually. The update-coverage-summary.yml workflow regenerates both on every push to main that touches QA-CHECKLIST.md, the regeneration scripts, or any .spec.ts. The regenerators are idempotent — running them locally either produces no diff (if main is already in sync) or a transient diff that is immediately overwritten on the next push to main, which adds churn without value.

So flipping the bullets to [x] is the only edit needed in this PR; the table and Phase 0 block will be in sync as soon as this merges.


---

Expand Down
104 changes: 104 additions & 0 deletions docs/ui-ux/global-variables-crud.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
# Global Variables — CRUD via Component

**Last validated:** Langflow 1.10.x

---

## What this test validates *(required)*

Validates the Global Variables modal that opens from an OpenAI component's Globe icon, covering create and delete operations and the secrecy guarantee of Credential-typed variables:

1. **Create Generic variable** — a Generic-type global variable can be created via "Add New Variable" and appears in the variable list.
2. **Delete variable removes it from list** — after creating a variable, deleting it with the Trash2 icon removes it from the list entirely.
3. **Credential variable value is hidden from the variable list** — after saving a Credential-typed variable, the entered value must not appear anywhere as visible text on the page (list, toast, preview); only the variable name is rendered.

If these break, users cannot manage global variables (API keys, shared values) that are reused across components, or worse, secrets entered as Credential variables become visible in the UI.

---

## Tags *(required)*

`@stable` `@release` `@workspace` `@regression`

---

## Step by step *(required)*

**Both tests share the same setup:**

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid, fixed in fc12387. Heading is now "All three tests share the same setup:" — the file has had 3 tests in scope since Test 3 was added; the "Both" carryover was stale.

1. Set viewport to 1920×1080 (modal can overflow on smaller screens)
2. Bootstrap app, create blank flow, add OpenAI component
3. Click the OpenAI component header, then click the Globe icon
4. Wait for Global Variables modal to open

**Test 1 — create Generic variable**

1. Run setup
2. Click "Add New Variable" via JS evaluate (button may be off-screen)
3. Fill variable name with `test-generic-{timestamp}`
4. Assert "Generic" type label is visible
5. Fill value with `generic-value-123`
6. Click "Save Variable"
7. Assert variable name appears in the list
8. Cleanup: delete the variable in `finally` block if visible

**Test 2 — delete variable removes it from list**

1. Run setup
2. Click "Add New Variable" via JS evaluate
3. Fill name `delete-me-{timestamp}`, fill value `to-be-deleted`
4. Click "Save Variable", assert variable is visible
5. Click Trash2 icon, confirm "Delete"
6. Assert variable name has `count() === 0` in the list
7. Cleanup: delete in `finally` if `varCreated` flag is still true

**Test 3 — Credential variable value is hidden from the variable list**

1. Run setup
2. Click "Add New Variable" via JS evaluate
3. Credential is the default type — no need to switch tabs

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid — fixed in fc12387 in lockstep with the spec change. The Step-by-step section for Test 3 now has an explicit step "Click credential-tab — the modal opens on the Generic tab by default; this switches it to Credential before save." The misleading "Credential is the default type — no need to switch tabs" line is gone.

4. Fill name `credential-{timestamp}`, fill value with a distinctive sentinel `SECRET-SENTINEL-{timestamp}`
5. Click "Save Variable"
6. Sanity: assert variable name is visible in the list
7. Critical: assert `getByText(sentinelValue, { exact: true })` has `count() === 0` — value must not surface as visible text anywhere on the page

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid, fixed in fc12387 in lockstep with the spec. The validation criterion now reads:

Credential value (sentinel) has substring-text count 0 anywhere on the page after save (Test 3) — getByText(sentinelValue) without exact: true, so embedded occurrences inside longer messages also fail the test

The Test 3 Step-by-step also calls out the substring behaviour explicitly ("getByText(sentinelValue) (substring match, no exact)") so the doc no longer overstates what the assertion catches.

8. Cleanup: delete in `finally` if `varCreated` flag is still true

---

## Validation criterion *(required)*

- Variable name appears in list after creation (Test 1)
- Variable name has count 0 after deletion (Test 2)
- Credential value (sentinel) has visible-text count 0 anywhere on the page after save (Test 3)

---

## External dependencies *(required)*

- `src/frontend/src/components/core/parameterRenderComponent/` — Globe icon triggering the global variables modal
- `src/backend/base/langflow/api/v1/variable.py` — CRUD endpoints
- `data-testid="icon-Globe"` — Globe icon on OpenAI component
- `data-testid="icon-Trash2"` — delete button in the variables list

---

## What this test does not cover *(optional)*

- Using a global variable inside a component (auto-fill behavior)
- Editing an existing variable's value (covered in `global-variable-edit.spec.ts`)
- Deletion via the Settings page (covered in `global-variable-remove.spec.ts`)
- The Credential test does not interact with the eye/show-value toggle or assert that the input field is rendered with `type="password"` — it only verifies that the saved value never surfaces as visible text. Browser-level masking and toggle UX are out of scope.

---

## Preconditions *(optional)*

- Langflow running at `PLAYWRIGHT_BASE_URL`
- OpenAI component must be available in the sidebar
- No API key required — component is added but never run

---

## Notes *(optional)*

- `page.evaluate()` is used to click "Add New Variable" because the modal can render outside the viewport on smaller screens. The JS click bypasses the viewport boundary.
- `try/finally` cleanup ensures variables are deleted even when assertions fail mid-test — preventing test pollution between runs.
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { awaitBootstrapTest } from "../../../helpers/other/await-bootstrap-test"

test(
"create a Generic type global variable",
{ tag: ["@release", "@workspace", "@regression"] },
{ tag: ["@stable", "@release", "@workspace", "@regression"] },
async ({ page }) => {
// Use large viewport so the global variables modal is fully visible
await page.setViewportSize({ width: 1920, height: 1080 });
Expand Down Expand Up @@ -78,7 +78,7 @@ test(

test(
"delete a global variable removes it from the list",
{ tag: ["@release", "@workspace", "@regression"] },
{ tag: ["@stable", "@release", "@workspace", "@regression"] },
async ({ page }) => {
await page.setViewportSize({ width: 1920, height: 1080 });
await awaitBootstrapTest(page);
Expand Down Expand Up @@ -156,3 +156,83 @@ test(
}
},
);

test(
"Credential variable value is hidden from the variable list",
{ tag: ["@stable", "@release", "@workspace", "@regression"] },
async ({ page }) => {
await page.setViewportSize({ width: 1920, height: 1080 });
await awaitBootstrapTest(page);
await page.waitForSelector('[data-testid="blank-flow"]', { timeout: 30000 });
await page.getByTestId("blank-flow").click();

await page.getByTestId("sidebar-search-input").click();
await page.getByTestId("sidebar-search-input").fill("openai");
await page.waitForSelector('[data-testid="openaiOpenAI"]', {
timeout: 30000,
});
await page
.getByTestId("openaiOpenAI")
.hover()
.then(async () => {
await page.getByTestId("add-component-button-openai").last().click();
});

await page.waitForTimeout(1000);
await page.getByText("OpenAI", { exact: true }).last().click();
await page.getByTestId("icon-Globe").nth(0).click();
await page.waitForTimeout(500);

const varName = `credential-${Date.now()}`;
// Distinctive sentinel — if this string surfaces anywhere as visible text
// after save, the Credential value leaked into the DOM.
const sentinelValue = `SECRET-SENTINEL-${Date.now()}`;
let varCreated = false;

try {
// Use JS click because the button may render outside the browser viewport
await page.evaluate(() => {
const el = Array.from(document.querySelectorAll("button, span")).find(
(e) => e.textContent?.trim() === "Add New Variable",
) as HTMLElement | undefined;
if (el) el.click();
else throw new Error("Add New Variable button not found in DOM");
});
await page.waitForTimeout(500);

// Credential is the default type in the modal — no need to switch tabs
await page
.getByPlaceholder("Enter a name for the variable...")
.fill(varName);
await page
.getByPlaceholder("Enter a value for the variable...")
.fill(sentinelValue);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caught a real bug — fixed in fc12387. Confirmed via tests/tests-automations/regression/ui-ux/globalVariables.spec.ts:43-66 that the modal opens on the Generic tab by default: line 51 asserts "Generic" type label is visible immediately after Add New, and line 63 explicitly clicks credential-tab before saving the Credential variable.

So the previous test was silently creating a Generic variable and asserting its value wasn't leaked — which passes trivially because Generic variables also do not echo their value back into the list.

Fix: inserted await page.getByTestId("credential-tab").click() between the name fill and value fill, matching the existing flow. After the fix:

  • --workers=1 --retries=0: 3/3 PASS (26.1 s)
  • Force-fail on the sentinel toHaveCount(0)toHaveCount(42): failed exactly on the assertion line, reverted, repassed.
  • Zero backend errors in the audit run.


await page.getByText("Save Variable", { exact: true }).click();
await page.waitForTimeout(500);

// Sanity: variable name is in the list
await expect(page.getByText(varName, { exact: true })).toBeVisible({
timeout: 5000,
});
varCreated = true;

// Critical: the Credential value must NOT appear as visible text anywhere
// on the page. getByText scans rendered text, not input value attributes,
// so a masked <input type="password" value="…"> is fine — what we forbid
// is the value being echoed back into the list, a toast, or any preview.
await expect(
page.getByText(sentinelValue, { exact: true }),
).toHaveCount(0, { timeout: 5000 });

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid — fixed in fc12387. Dropped exact: true so getByText(sentinelValue) now does substring matching. A leak embedded in a longer message ("Saved: SECRET-SENTINEL-..." or any labeled preview) now fails the assertion, which matches the guarantee stated by the test name ("hidden from the variable list").

The inline comment was also expanded to explain the substring behaviour and clarify the deliberate gap (input-value attributes still don't count as visible text, since <input type="password" value="…"> is the expected mechanism for masking the editable input). Force-fail probe at toHaveCount(0)toHaveCount(42) confirmed the assertion fails on the exact line; reverted, repassed.

} finally {
if (varCreated) {
const varRow = page.getByText(varName, { exact: true });
if (await varRow.isVisible({ timeout: 2000 }).catch(() => false)) {
await page.getByTestId("icon-Trash2").last().click();
await page.waitForTimeout(300);
await page.getByText("Delete", { exact: true }).last().click();
}
}
}
},
);
Loading