🧩 refactor: Serve Chat Context Through a Facade-Owned Provider - #16610
Conversation
berry-13
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc916776e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await page.goto(`/agents/all?q=${encodeURIComponent(name)}`, { timeout: 10_000 }); | ||
| const heading = page.getByRole('heading', { name, exact: true }); | ||
| await expect(heading).toBeVisible({ timeout: 30_000 }); | ||
| await heading.click(); |
There was a problem hiding this comment.
In the marketplace UI, AgentCard places this heading inside a pointer-events-none layer while selection is handled by an absolute sibling button whose accessible name comes from the heading (AgentCard.tsx:104-117,119-160). Playwright's non-forced heading.click() therefore sees the underlying button receive the hit and retries until timeout, so this scenario never reaches the dialog. Locate and click the button with accessible name name instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fd711e0: the scenario now clicks the card button that takes its accessible name from the heading. reviewctl verify passed on that head (desktop light, desktop dark, mobile).
codegraph-librechat
Bot
added
the
🗺️ Chat State Mgmt
fd711e0 to
36bf001
Compare
36bf001 to
80bcab8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80bcab81ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await page.getByRole('button', { name: 'Start Chat' }).click(); | ||
| /** The chat route applies `agent_id` and then drops it from the URL, so the turn's | ||
| * request body is what proves which agent the chat started with. */ | ||
| await expect(page).toHaveURL(/\/c\/new/, { timeout: 15_000 }); |
There was a problem hiding this comment.
This URL assertion succeeds immediately after navigation even while ?endpoint=agents&agent_id=... is still present, but useQueryParams does not apply those settings until its 100 ms polling callback runs (client/src/hooks/Input/useQueryParams.ts:263-352). On a fast browser run, sendMessage can therefore submit using the previous/default conversation, making the new scenario intermittently fail its agent_id assertion. 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.
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).
80bcab8 to
4015926
Compare
4015926 to
cf14cea
Compare
cf14cea to
c100ec5
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new tests contain repeated broad casts that violate the repository’s explicit-type convention.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Introduces a facade-owned ChatProvider and migrates the marketplace to use it without changing behavior.
Changes:
- Adds and tests the reusable chat provider.
- Migrates marketplace context to the provider.
- Adds end-to-end marketplace chat coverage.
| File | Description |
|---|---|
client/src/hooks/Chat/provider.tsx |
Adds ChatProvider. |
client/src/hooks/Chat/__tests__/provider.spec.tsx |
Tests provider defaults and facade integration. |
client/src/components/Agents/MarketplaceContext.tsx |
Adopts ChatProvider. |
client/src/components/Agents/tests/MarketplaceContext.spec.tsx |
Updates marketplace provider tests. |
client/src/routes/__tests__/Marketplace.spec.tsx |
Updates the helper mock path. |
e2e/specs/mock/scenarios/marketplace-start-chat.spec.ts |
Verifies agent selection when starting a marketplace chat. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| jest.mock('~/hooks/Chat/useChatHelpers', () => ({ | ||
| __esModule: true, | ||
| default: (...args: unknown[]) => mockUseChatHelpers(...args), |
There was a problem hiding this comment.
Fixed in dd0122b: the mock passes index and paramId with the hook's parameter types.
|
|
||
| jest.mock('../useChatHelpers', () => ({ | ||
| __esModule: true, | ||
| default: (...args: unknown[]) => mockUseChatHelpers(...args), |
There was a problem hiding this comment.
Fixed in dd0122b: typed mock parameters, and the spec builds a complete typed ChatContract instead of casting a partial one.
4673caf to
dd0122b
Compare
ChatProvider builds a pane's chat contract and serves it to the facade, so a host no longer imports useChatHelpers to fill ChatContext itself. The marketplace is its first host; its specs stub the helper module the provider imports.
Opens an agent's marketplace card, starts a chat from it, and expects the new chat to send its turn to that agent and show the reply.
The card heading sits under the card's click layer, so the scenario clicks the button that takes its accessible name from the heading.
The chat route applies agent_id on a short query-param poll and then drops it from the URL, so the scenario waits for the parameter to go before it sends the turn.
The provider and marketplace specs pass index and paramId through explicitly instead of an unknown[] spread, and the provider spec builds a complete typed chat contract.
dd0122b to
0848b18
Compare
Pull Request
Summary
Three components still imported
useChatHelpersto buildChatContextthemselves: the chat view, the unified sidebar and the marketplace. That kept the legacy hook inclient/src/componentseven though everything below them can read the chat through the facade. This covers chat audit criterion C7.A new
ChatProviderinclient/src/hooks/Chat/provider.tsxbuilds a pane's chat contract and serves it touseChat,useChatActionsanduseChatContext. The marketplace now uses it, with the same pane and conversation as before (index 0,new). The chat view (ChatView.tsx, owned by #15911) and the unified sidebar (UnifiedSidebar.tsx, owned by #16248) are left for those PRs to adopt the provider; they are the only legacy-hook imports left in components.Depends on #16609 (the base link of this stack).
Type of change
Testing
Tested environments/configuration:
lcdev servers on this branch: the live flows listed on the base link ran against this head.Automated tests:
provider.spec.tsx: the provider serves the contract it builds to the facade and defaults to the root pane.MarketplaceContext.spec.tsxnow asserts the root pane's new-chat contract; it androutes/__tests__/Marketplace.spec.tsxstub the helper module the provider imports.marketplace-start-chat.spec.ts(@scenario:a-chat-started-from-the-marketplace-answers-with-that-agent).npx jest --findRelatedTestson the provider and the marketplace context: 9 suites, 80 tests passed.tscand static checks: clean.Screenshots / recordings
No user-facing change.
Risk / compatibility
None expected: the marketplace builds the same contract through the provider instead of inline.
Checklist