-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
馃З refactor: Serve Chat Context Through a Facade-Owned Provider #16610
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
ff01002
refactor: Serve the Marketplace Chat Through a Facade-Owned Provider
berry-13 b65d291
test: Start a Chat From a Marketplace Agent Card
berry-13 00941aa
test: Open the Marketplace Card Through Its Button
berry-13 38bb4ee
test: Wait for the Marketplace Agent Before Sending
berry-13 0848b18
test: Type the useChatHelpers Mocks With the Hook's Parameters
berry-13 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| import React from 'react'; | ||
| import { renderHook } from '@testing-library/react'; | ||
| import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; | ||
| import type { TConversation } from 'librechat-data-provider'; | ||
| import type { ChatContract } from '../contract'; | ||
| import { useChatActions } from '../facade'; | ||
| import { ChatProvider } from '../provider'; | ||
|
|
||
| const mockUseChatHelpers = jest.fn(); | ||
|
|
||
| jest.mock('../useChatHelpers', () => ({ | ||
| __esModule: true, | ||
| default: (index?: number, paramId?: string) => mockUseChatHelpers(index, paramId), | ||
| })); | ||
|
|
||
| const noop = () => undefined; | ||
|
|
||
| const contract: ChatContract = { | ||
| index: 0, | ||
| conversation: { conversationId: 'convo-1' } as TConversation, | ||
| setConversation: noop, | ||
| newConversation: noop, | ||
| preset: null, | ||
| setPreset: noop, | ||
| optionSettings: {}, | ||
| setOptionSettings: noop, | ||
| getMessages: () => [], | ||
| messagesKey: 'convo-1', | ||
| setMessages: noop, | ||
| setSiblingIdx: noop, | ||
| latestMessageId: undefined, | ||
| latestMessageDepth: undefined, | ||
| ask: jest.fn(), | ||
| regenerate: jest.fn(), | ||
| isSubmitting: false, | ||
| setIsSubmitting: noop, | ||
| handleRegenerate: noop, | ||
| handleContinue: noop, | ||
| stopGenerating: jest.fn(() => Promise.resolve()), | ||
| handleStopGenerating: noop, | ||
| abortScroll: false, | ||
| setAbortScroll: noop, | ||
| files: new Map(), | ||
| setFiles: noop, | ||
| filesLoading: false, | ||
| setFilesLoading: noop, | ||
| showPopover: false, | ||
| setShowPopover: noop, | ||
| feedbackEnabled: false, | ||
| }; | ||
|
|
||
| const renderUnder = (props: { index?: number; conversationId?: string }) => { | ||
| const queryClient = new QueryClient(); | ||
| return renderHook(() => useChatActions(), { | ||
| wrapper: ({ children }) => ( | ||
| <QueryClientProvider client={queryClient}> | ||
| <ChatProvider {...props}>{children}</ChatProvider> | ||
| </QueryClientProvider> | ||
| ), | ||
| }); | ||
| }; | ||
|
|
||
| describe('ChatProvider', () => { | ||
| beforeEach(() => { | ||
| mockUseChatHelpers.mockReset(); | ||
| mockUseChatHelpers.mockReturnValue(contract); | ||
| }); | ||
|
|
||
| it('serves the pane contract it builds to the facade below it', async () => { | ||
| const { result } = renderUnder({ index: 1, conversationId: 'convo-1' }); | ||
|
|
||
| expect(mockUseChatHelpers).toHaveBeenCalledWith(1, 'convo-1'); | ||
| expect(result.current.id).toBe('convo-1'); | ||
| expect(result.current.status).toBe('ready'); | ||
| await result.current.stop(); | ||
| expect(contract.stopGenerating).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('builds the root pane by default', () => { | ||
| renderUnder({}); | ||
|
|
||
| expect(mockUseChatHelpers).toHaveBeenCalledWith(0, undefined); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| import type { ReactNode } from 'react'; | ||
| import { ChatContext } from '~/Providers/ChatContext'; | ||
| import useChatHelpers from './useChatHelpers'; | ||
|
|
||
| /** | ||
| * Builds a pane's chat contract and serves it to `useChat`, `useChatActions` and | ||
| * `useChatContext` below. A host renders this instead of calling `useChatHelpers` itself, so | ||
| * components reach the chat only through the facade and its context. | ||
| */ | ||
| export function ChatProvider({ | ||
| index = 0, | ||
| conversationId, | ||
| children, | ||
| }: { | ||
| /** The pane: `0` is the root pane, `1` the added (multi-convo) pane. */ | ||
| index?: number; | ||
| /** The route's conversation id, which can run ahead of the pane's conversation. */ | ||
| conversationId?: string; | ||
| children: ReactNode; | ||
| }) { | ||
| const chat = useChatHelpers(index, conversationId); | ||
| return <ChatContext.Provider value={chat}>{children}</ChatContext.Provider>; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| import { expect, test } from '@playwright/test'; | ||
| import type { Page } from '@playwright/test'; | ||
| import { withMongo } from '../db'; | ||
| import { uniqueAgentName } from '../agents.helpers'; | ||
| import { | ||
| getAccessToken, | ||
| messagesView, | ||
| replyPrompt, | ||
| replyText, | ||
| requestJson, | ||
| sendMessage, | ||
| } from '../helpers'; | ||
|
|
||
| type AgentResponse = { id: string }; | ||
|
|
||
| async function createAgent(page: Page, name: string): Promise<AgentResponse> { | ||
| const token = await getAccessToken(page); | ||
| return requestJson<AgentResponse>(page, { | ||
| path: '/api/agents', | ||
| token, | ||
| method: 'POST', | ||
| body: { | ||
| name, | ||
| description: 'Agent used by the marketplace start-chat scenario.', | ||
| instructions: 'Respond deterministically for the marketplace start-chat scenario.', | ||
| provider: 'Mock Provider A', | ||
| model: 'mock-model-a', | ||
| model_parameters: {}, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| async function cleanupAgent(agentId: string): Promise<void> { | ||
| await withMongo(async (db) => { | ||
| const agent = await db | ||
| .collection('agents') | ||
| .findOne({ id: agentId }, { projection: { _id: 1 } }); | ||
| if (agent) { | ||
| await db.collection('aclentries').deleteMany({ resourceId: agent._id }); | ||
| } | ||
| await db.collection('agents').deleteMany({ id: agentId }); | ||
| }); | ||
| } | ||
|
|
||
| test.describe('marketplace start chat', () => { | ||
| /* `getAccessToken` refreshes through a relative URL from the page, so the tab has to be | ||
| on the app before the test asks for a token. */ | ||
| test.beforeEach(async ({ page }) => { | ||
| await page.goto('/agents/all', { timeout: 30_000 }); | ||
| }); | ||
|
|
||
| test('@scenario:a-chat-started-from-the-marketplace-answers-with-that-agent starting a chat from an agent card opens a new chat that the agent answers', async ({ | ||
| page, | ||
| }) => { | ||
| test.setTimeout(120_000); | ||
| const name = uniqueAgentName('E2E Marketplace Start'); | ||
| const agent = await createAgent(page, name); | ||
|
|
||
| try { | ||
| await page.goto(`/agents/all?q=${encodeURIComponent(name)}`, { timeout: 10_000 }); | ||
| await expect(page.getByRole('heading', { name, exact: true })).toBeVisible({ | ||
| timeout: 30_000, | ||
| }); | ||
| /** The card's heading sits under its click layer; the button that opens the dialog | ||
| * takes its accessible name from that heading. */ | ||
| await page.getByRole('button', { name, exact: true }).click(); | ||
|
|
||
| await page.getByRole('button', { name: 'Start Chat' }).click(); | ||
| /** The chat route applies `agent_id` on its query-param poll and then drops it from the | ||
| * URL, so waiting for it to go is waiting for the agent to be selected. The turn's request | ||
| * body is what proves which agent the chat started with. */ | ||
| await expect(page).toHaveURL(/\/c\/new/, { timeout: 15_000 }); | ||
| await expect(page).not.toHaveURL(/agent_id=/, { timeout: 15_000 }); | ||
|
|
||
| const label = `marketplace-start-${Date.now()}`; | ||
| const response = await sendMessage(page, replyPrompt(label)); | ||
| expect(response.ok()).toBeTruthy(); | ||
| await expect(messagesView(page).getByText(replyText(label))).toBeVisible({ | ||
| timeout: 30_000, | ||
| }); | ||
| expect(response.request().postDataJSON()).toEqual( | ||
| expect.objectContaining({ agent_id: agent.id }), | ||
| ); | ||
| } finally { | ||
| await cleanupAgent(agent.id); | ||
| } | ||
| }); | ||
| }); | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This URL assertion succeeds immediately after navigation even while
?endpoint=agents&agent_id=...is still present, butuseQueryParamsdoes not apply those settings until its 100 ms polling callback runs (client/src/hooks/Input/useQueryParams.ts:263-352). On a fast browser run,sendMessagecan therefore submit using the previous/default conversation, making the new scenario intermittently fail itsagent_idassertion. Wait until the query parameters have been removed, or until the selected agent is otherwise observable, before sending.Useful? React with 馃憤聽/ 馃憥.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 4015926: the scenario waits for agent_id to leave the URL, which useQueryParams does once it has applied the agent, before sending. reviewctl verify passed on that head (desktop light, desktop dark, mobile).