From fa57d4d14c3fa21cc230d504d7c564472b429163 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Wed, 28 Oct 2026 06:08:05 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A7=AA=20fix:=20Wait=20Out=20the=20Langfu?= =?UTF-8?q?se=20Load-Time=20Verification=20in=20Its=20Tests=20and=20Bring?= =?UTF-8?q?=20the=20Live=20Model=20Specs=20Journey=20Up=20to=20Date?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit LangfuseRenderer verifies a saved connection once its status renders, and while that request is in flight every control is disabled and the primary button reads "Checking" instead of "Save & enable". "preserves the stored secret when only the public key is edited" and "enables a disabled connection when credential edits are saved with Save & enable" clicked Edit as soon as the stored connection showed, before that verification had run or settled, so under load the click landed inside the in-flight window and the Save & enable lookup failed. renderVerified() now renders a saved connection and waits for the Verified status, which renders in the same pass that releases the controls; every test that acts on a saved connection uses it, with its assertions unchanged. The default verification mock answers on a later task, as a request does, so the window is always there and a test that acts inside it fails on every run instead of rarely. A new test holds the verification open and checks that Edit and Disable stay disabled until it settles. e2e/specs.spec.ts expected a new spec to open on Endpoint. That held only while a new spec showed its Model group first; the editor then went back to one group order for new and saved specs (Basics, then Model), so a new card focuses its Label, and the journey was never updated. The same journey also expected the card summary to read "openAI · gpt-4o-mini", but spec rows name the endpoint ("OpenAI") since the change that made them do so. addSpec now expects Label focused and fills Label, Endpoint and Model in that order, and the summary assertion expects "OpenAI · gpt-4o-mini". A unit test pins the new spec's group order and its Label focus, so CI catches the next change. --- e2e/specs.spec.ts | 14 ++- .../__tests__/LangfuseRenderer.test.tsx | 95 ++++++++++++++----- .../__tests__/ModelSpecsRenderer.test.tsx | 69 +++++++++----- 3 files changed, 126 insertions(+), 52 deletions(-) diff --git a/e2e/specs.spec.ts b/e2e/specs.spec.ts index b852e6bc..901253dc 100644 --- a/e2e/specs.spec.ts +++ b/e2e/specs.spec.ts @@ -46,16 +46,20 @@ async function revertList(page: Page) { await saveAndWait(page); } -/** Adds a spec with the four required fields (it opens on Endpoint, then Model), and returns its card. */ +/** + * Adds a spec with the four required fields, and returns its card. A new spec + * opens on Label, with Basics before the open Model group (Endpoint, Model), + * the order a saved spec shows. + */ async function addSpec(page: Page, label: string, endpoint: string, model: string) { await page.getByRole('button', { name: 'Add spec' }).click(); const card = cards(page).last(); - await expect(card.getByLabel(/^Endpoint\*?$/)).toBeFocused(); + await expect(card.getByLabel(/^Label\*?$/)).toBeFocused(); + await card.getByLabel(/^Label\*?$/).fill(label); + await card.getByLabel(/^Label\*?$/).press('Tab'); await card.getByLabel(/^Endpoint\*?$/).selectOption(endpoint); await card.getByRole('combobox', { name: /^Model\*?$/ }).fill(model); await card.getByRole('combobox', { name: /^Model\*?$/ }).press('Tab'); - await card.getByLabel(/^Label\*?$/).fill(label); - await card.getByLabel(/^Label\*?$/).press('Tab'); return card; } @@ -85,7 +89,7 @@ test.describe('Model specs', () => { await page.waitForLoadState('networkidle'); await expect(cards(page)).toHaveCount(before + 1); await expect(cards(page).last()).toContainText('E2E Spec'); - await expect(cards(page).last()).toContainText('openAI · gpt-4o-mini'); + await expect(cards(page).last()).toContainText('OpenAI · gpt-4o-mini'); await revertList(page); }); diff --git a/src/components/configuration/sections/__tests__/LangfuseRenderer.test.tsx b/src/components/configuration/sections/__tests__/LangfuseRenderer.test.tsx index 0fad035e..4c7ac8de 100644 --- a/src/components/configuration/sections/__tests__/LangfuseRenderer.test.tsx +++ b/src/components/configuration/sections/__tests__/LangfuseRenderer.test.tsx @@ -1,6 +1,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'; +import type { TLangfuseConnectionStatus } from 'librechat-data-provider'; import type * as t from '@/types'; import { getLangfuseConnectionFn, @@ -116,10 +117,34 @@ function renderLangfuse(overrides: Partial = {}) { return { ...result, queryClient }; } +/** + * Verifies on a later task, as a request does, so every verification is in + * flight (controls disabled, Save reading "Checking") before it settles. With + * an already-resolved promise that window was usually too short to see, so a + * test that acted inside it failed only under load. + */ +function verifyOnNextTask(): Promise<{ success: boolean }> { + return new Promise((resolve) => { + setTimeout(() => resolve({ success: true }), 0); + }); +} + +/** + * Renders a saved connection and waits until the verification the renderer + * starts on load has settled and released the controls. + */ +async function renderVerified(status: TLangfuseConnectionStatus) { + mockGet.mockResolvedValue(status); + const view = renderLangfuse(); + expect(await screen.findByText('com_config_langfuse_verified')).toBeVisible(); + expect(mockTest).toHaveBeenCalledTimes(1); + return view; +} + beforeEach(() => { vi.clearAllMocks(); mockGet.mockResolvedValue({ configured: false, enabled: false, destinations }); - mockTest.mockResolvedValue({ success: true }); + mockTest.mockImplementation(verifyOnNextTask); }); describe('LangfuseRenderer', () => { @@ -166,6 +191,44 @@ describe('LangfuseRenderer', () => { expect(await screen.findByText('com_config_langfuse_verified')).toBeVisible(); }); + it('holds the connection controls until the stored connection is verified', async () => { + let resolveVerification: ((result: { success: boolean }) => void) | undefined; + mockTest.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveVerification = resolve; + }), + ); + mockGet.mockResolvedValue({ + configured: true, + enabled: true, + destinations, + destination: 'eu', + publicKey: 'pk-lf-existing', + secretKeyPreview: 'sk-lf-...515f', + }); + renderLangfuse(); + + expect(await screen.findByText('com_config_langfuse_checking')).toBeVisible(); + const editPublicKey = screen.getByRole('button', { + name: 'com_ui_edit com_config_langfuse_public_key', + }); + const editSecretKey = screen.getByRole('button', { + name: 'com_ui_edit com_config_langfuse_secret_key', + }); + const disable = screen.getByRole('button', { name: 'com_config_langfuse_disable' }); + expect(editPublicKey).toBeDisabled(); + expect(editSecretKey).toBeDisabled(); + expect(disable).toBeDisabled(); + + await act(async () => resolveVerification?.({ success: true })); + + expect(await screen.findByText('com_config_langfuse_verified')).toBeVisible(); + expect(editPublicKey).toBeEnabled(); + expect(editSecretKey).toBeEnabled(); + expect(disable).toBeEnabled(); + }); + it('shows a configured connection as unverified without verifying it for read-only (disabled) viewers', async () => { mockGet.mockResolvedValue({ configured: true, @@ -254,12 +317,11 @@ describe('LangfuseRenderer', () => { publicKey: 'pk-lf-old', secretKeyPreview: 'sk-lf-...515f', }; - mockGet.mockResolvedValue(configuredStatus); mockUpdate.mockResolvedValue({ ...configuredStatus, publicKey: 'pk-lf-new' }); - const { queryClient } = renderLangfuse(); + const { queryClient } = await renderVerified(configuredStatus); fireEvent.click( - await screen.findByRole('button', { name: 'com_ui_edit com_config_langfuse_public_key' }), + screen.getByRole('button', { name: 'com_ui_edit com_config_langfuse_public_key' }), ); expect(screen.getByRole('button', { name: 'com_ui_cancel' })).toBeVisible(); expect( @@ -296,13 +358,10 @@ describe('LangfuseRenderer', () => { publicKey: 'pk-lf-stored', secretKeyPreview: 'sk-lf-...515f', }; - mockGet.mockResolvedValue(configuredStatus); - const { queryClient } = renderLangfuse(); - expect(await screen.findByText('com_config_langfuse_verified')).toBeVisible(); - expect(mockTest).toHaveBeenCalledTimes(1); + const { queryClient } = await renderVerified(configuredStatus); fireEvent.click( - await screen.findByRole('button', { name: 'com_ui_edit com_config_langfuse_public_key' }), + screen.getByRole('button', { name: 'com_ui_edit com_config_langfuse_public_key' }), ); fireEvent.change(screen.getByPlaceholderText('pk-lf-...'), { target: { value: 'pk-lf-draft' }, @@ -388,11 +447,8 @@ describe('LangfuseRenderer', () => { publicKey: 'pk-lf-existing', secretKeyPreview: 'sk-lf-...515f', }; - mockGet.mockResolvedValue(configuredStatus); mockUpdate.mockResolvedValue({ ...configuredStatus, enabled: false }); - renderLangfuse(); - await screen.findByText('sk-lf-...515f'); - await waitFor(() => expect(mockTest).toHaveBeenCalledTimes(1)); + await renderVerified(configuredStatus); mockTest.mockClear(); fireEvent.click(screen.getByRole('button', { name: 'com_config_langfuse_disable' })); @@ -415,11 +471,8 @@ describe('LangfuseRenderer', () => { publicKey: 'pk-lf-existing', secretKeyPreview: 'sk-lf-...515f', }; - mockGet.mockResolvedValue(configuredStatus); mockUpdate.mockResolvedValue({ ...configuredStatus, enabled: true }); - renderLangfuse(); - await screen.findByText('sk-lf-...515f'); - await waitFor(() => expect(mockTest).toHaveBeenCalledTimes(1)); + await renderVerified(configuredStatus); mockTest.mockClear(); fireEvent.click(screen.getByRole('button', { name: 'com_config_langfuse_enable' })); @@ -444,10 +497,8 @@ describe('LangfuseRenderer', () => { publicKey: 'pk-lf-existing', secretKeyPreview: 'sk-lf-...515f', }; - mockGet.mockResolvedValue(configuredStatus); mockUpdate.mockResolvedValue({ ...configuredStatus, enabled: true, publicKey: 'pk-lf-new' }); - renderLangfuse(); - await screen.findByText('sk-lf-...515f'); + await renderVerified(configuredStatus); fireEvent.click( screen.getByRole('button', { name: 'com_ui_edit com_config_langfuse_public_key' }), @@ -465,7 +516,7 @@ describe('LangfuseRenderer', () => { }); it('re-verifies the stored connection when an invalid key edit is cancelled', async () => { - mockGet.mockResolvedValue({ + await renderVerified({ configured: true, enabled: true, destinations, @@ -473,8 +524,6 @@ describe('LangfuseRenderer', () => { publicKey: 'pk-lf-existing', secretKeyPreview: 'sk-lf-...515f', }); - renderLangfuse(); - await screen.findByText('com_config_langfuse_verified'); mockTest.mockResolvedValueOnce({ success: false, message: 'invalid keys' }); fireEvent.click( diff --git a/src/components/configuration/sections/specs/__tests__/ModelSpecsRenderer.test.tsx b/src/components/configuration/sections/specs/__tests__/ModelSpecsRenderer.test.tsx index 1d0a4897..12518b76 100644 --- a/src/components/configuration/sections/specs/__tests__/ModelSpecsRenderer.test.tsx +++ b/src/components/configuration/sections/specs/__tests__/ModelSpecsRenderer.test.tsx @@ -105,6 +105,34 @@ function renderSpecs( return { onChange, ...render(wrap(ui)) }; } +/** The renderer over a spec list it keeps in state, as the configuration page does. */ +function StatefulSpecs() { + const [list, setList] = useState(SPECS); + return ( + + (path === 'modelSpecs.list' ? list : fallback)} + onChange={(path, value) => { + if (path === 'modelSpecs.list') setList(value); + }} + onResetField={vi.fn()} + onDiscardField={vi.fn()} + permissions={{ canView: true, canEdit: true, canAssign: true }} + /> + + ); +} + +/** The Basics and Model group headers of a spec card, in the order it shows them. */ +function basicsAndModelOrder(card: HTMLElement): string[] { + return [...card.querySelectorAll('button[aria-expanded]')] + .map((button) => button.textContent ?? '') + .filter((text) => /com_config_specs_group_(basics|model)$/.test(text)); +} + describe('ModelSpecsRenderer', () => { it('lists each spec by label with its badges and endpoint · model', () => { renderSpecs({ list: SPECS }); @@ -126,6 +154,18 @@ describe('ModelSpecsRenderer', () => { expect(onChange).toHaveBeenCalledWith('modelSpecs.list', [...SPECS, {}]); }); + it('opens a new spec on its Label, with Basics before Model as on a saved spec', async () => { + render(); + fireEvent.click(screen.getByRole('button', { name: 'com_config_specs_add' })); + const card = document.querySelectorAll('[data-entry-card]')[SPECS.length]; + expect(basicsAndModelOrder(card)).toEqual([ + 'com_config_specs_group_basics', + 'com_config_specs_group_model', + ]); + await act(() => new Promise((resolve) => requestAnimationFrame(resolve))); + expect(document.getElementById(`modelSpecs-list-${SPECS.length}-label`)).toHaveFocus(); + }); + it('shows the endpoint as a choice of configured endpoints and the model as text', () => { renderSpecs({ list: SPECS }); const toggle = document.querySelectorAll( @@ -162,10 +202,10 @@ describe('ModelSpecsRenderer', () => { ); const card = document.querySelectorAll('[data-entry-card]')[1]; fireEvent.click(card.querySelector('button[aria-expanded]')!); - const groups = [...card.querySelectorAll('button[aria-expanded]')] - .map((button) => button.textContent ?? '') - .filter((text) => /com_config_specs_group_(basics|model)$/.test(text)); - expect(groups).toEqual(['com_config_specs_group_basics', 'com_config_specs_group_model']); + expect(basicsAndModelOrder(card)).toEqual([ + 'com_config_specs_group_basics', + 'com_config_specs_group_model', + ]); const label = document.getElementById('modelSpecs-list-1-label') as HTMLInputElement; fireEvent.change(label, { target: { value: 'Claude V2' } }); fireEvent.blur(label); @@ -476,26 +516,7 @@ describe('ModelSpecsRenderer', () => { }); it('keeps an expanded spec open on the spec itself when it is moved', () => { - function Stateful() { - const [list, setList] = useState(SPECS); - return ( - - (path === 'modelSpecs.list' ? list : fallback)} - onChange={(path, value) => { - if (path === 'modelSpecs.list') setList(value); - }} - onResetField={vi.fn()} - onDiscardField={vi.fn()} - permissions={{ canView: true, canEdit: true, canAssign: true }} - /> - - ); - } - render(); + render(); const toggle = (index: number) => document .querySelectorAll('[data-entry-card]')