diff --git a/application/single_app/config.py b/application/single_app/config.py index 9dc7ba6fc..bf5a6c6e2 100644 --- a/application/single_app/config.py +++ b/application/single_app/config.py @@ -101,7 +101,7 @@ EXECUTOR_TYPE = 'thread' EXECUTOR_MAX_WORKERS = 30 SESSION_TYPE = 'filesystem' -VERSION = "0.261.225" +VERSION = "0.261.226" IS_DEVELOPMENT = is_development_env_enabled() # Opt-out for deployments where App Service Easy Auth is active but the platform diff --git a/application/v2_ui/src/components/documents/DocumentExplorer.tsx b/application/v2_ui/src/components/documents/DocumentExplorer.tsx index 77464a62d..857ec68ba 100644 --- a/application/v2_ui/src/components/documents/DocumentExplorer.tsx +++ b/application/v2_ui/src/components/documents/DocumentExplorer.tsx @@ -71,6 +71,7 @@ import { upsertSavedView, } from '../../lib/documentSavedViews'; import { useBootstrapStore } from '../../stores/bootstrapStore'; +import { useChatStore } from '../../stores/chatStore'; import { useUserSettingsStore } from '../../stores/userSettingsStore'; import { toast } from '../../stores/toastStore'; import { EmptyState, GlassButton, Skeleton } from '../ui/primitives'; @@ -1098,6 +1099,12 @@ function ScopedDocumentExplorer({ contextDocuments: documents.map((document) => ({ document, scope })), contextTags: tags.map((name) => ({ name, scope })), }; + // Always a brand-new chat, as the classic interface's page load into chat is. + // The chat store outlives a route change, so navigating alone opened whichever + // conversation was last open -- busy or not -- and attached these documents to + // it. Leaving it detaches rather than cancels, so an answer still being written + // there finishes and is saved. + useChatStore.getState().startNewConversation(); navigate(`/chat?${handoff}`, { state }); } catch (cause) { if (!controller.signal.aborted && mounted.current) toast.error(errorMessage(cause, 'Could not confirm the selected documents for chat.')); diff --git a/application/v2_ui/src/lib/orchestrationController.ts b/application/v2_ui/src/lib/orchestrationController.ts index 86359eff2..f0cb12ad1 100644 --- a/application/v2_ui/src/lib/orchestrationController.ts +++ b/application/v2_ui/src/lib/orchestrationController.ts @@ -60,7 +60,7 @@ import { applyOrchestrationOutputEvent } from './orchestrationOutputController'; import { choosePlanSubmissionId, rememberPlanSubmission } from './planSubmissionIds'; import { canonicalPlanReferences, PlanReferenceError, type PlanReference } from './planReferences'; import { announceCompletedReply } from './replyEvents'; -import { useChatStore } from '../stores/chatStore'; +import { currentConversationEpoch, useChatStore } from '../stores/chatStore'; import { selectEdits, selectCanEditPlan, @@ -187,9 +187,10 @@ export interface StartPlanParams { /** * Create the conversation a first message needs, or pass an existing one through. * - * Mirrors the minimal half of `sendMessage`'s creation: claim `activeConversationId` only if - * nothing has been opened in the round trip, so a reader who opens another thread meanwhile keeps - * their place while this turn still attaches to the conversation it created. + * Mirrors the minimal half of `sendMessage`'s creation: claim `activeConversationId` only if the + * reader is still in the new chat this was sent from, so a reader who opens another thread -- or + * starts another new chat -- meanwhile keeps their place while this turn still attaches to the + * conversation it created. */ async function ensureConversation( conversationId: string | null, @@ -198,10 +199,13 @@ async function ensureConversation( if (conversationId) { return conversationId; } + // Taken before the round trip: New chat leaves the id null too, so only the epoch says the + // reader has moved on to a different new chat. + const epoch = currentConversationEpoch(); try { const created = await createConversation(message); const chat = useChatStore.getState(); - if (chat.activeConversationId === null) { + if (chat.activeConversationId === null && currentConversationEpoch() === epoch) { useChatStore.setState({ activeConversationId: created.conversation_id, activeConversationKind: 'personal', diff --git a/application/v2_ui/src/pages/HomePage.tsx b/application/v2_ui/src/pages/HomePage.tsx index 683e438e6..aa18ea681 100644 --- a/application/v2_ui/src/pages/HomePage.tsx +++ b/application/v2_ui/src/pages/HomePage.tsx @@ -12,6 +12,7 @@ import { Link } from 'react-router-dom'; import { MessageSquarePlus } from 'lucide-react'; import { useBootstrapStore } from '../stores/bootstrapStore'; +import { useChatStore } from '../stores/chatStore'; import { useUiStore } from '../stores/uiStore'; import { AdminMarkdown } from '../components/admin/AdminMarkdown'; @@ -72,8 +73,11 @@ export function HomePage() { /> ) : null} + {/* A fresh chat, as the label promises. The chat store outlives a route change, + so following the link alone reopened whichever conversation was last open. */} useChatStore.getState().startNewConversation()} className="inline-flex items-center gap-2 rounded-xl bg-accent px-6 py-3 text-base font-medium text-on-accent transition-colors hover:bg-accent-hover" > diff --git a/application/v2_ui/src/pages/workspace/TagsSection.tsx b/application/v2_ui/src/pages/workspace/TagsSection.tsx index 2a63456e4..97a546058 100644 --- a/application/v2_ui/src/pages/workspace/TagsSection.tsx +++ b/application/v2_ui/src/pages/workspace/TagsSection.tsx @@ -29,6 +29,7 @@ import { import type { GroupWorkspaceContext } from '../../lib/workspaceContext'; import { groupWorkspacePath } from '../../lib/groupWorkspaceNavigation'; import { useBootstrapStore } from '../../stores/bootstrapStore'; +import { useChatStore } from '../../stores/chatStore'; import { GlassButton, EmptyState, Skeleton } from '../../components/ui/primitives'; import { SectionError, SectionIntro, SectionSearch } from '../../components/workspace/primitives'; import { errorMessage } from '../../components/workspace/useSectionResource'; @@ -405,6 +406,9 @@ function ScopedTagsSection({ const state: ContextHandoffState = { contextTags: [{ name: tag.name, scope }], }; + // A brand-new chat, for the same reason the documents hand-off starts one: otherwise + // the tag lands in whichever conversation was last open. + useChatStore.getState().startNewConversation(); navigate(`/chat?${query}`, { state }); }; diff --git a/application/v2_ui/src/stores/chatStore.ts b/application/v2_ui/src/stores/chatStore.ts index 5a9e854d8..9cc14c587 100644 --- a/application/v2_ui/src/stores/chatStore.ts +++ b/application/v2_ui/src/stores/chatStore.ts @@ -864,6 +864,36 @@ let streamingConversationId: string | null = null; */ let streamingConversationKind: ConversationKind = 'personal'; +/** + * Conversations whose orchestration turn currently holds the streaming surface. + * + * A plan or run is driven by its own controller, not `activeStreamController`, so + * `detachActiveStream` knows nothing about it. The turn takes the shared `streaming` flag in + * `beginOrchestrationTurn` and gives it back in `settleOrchestrationTurn`, but both only write + * while that conversation is on screen. Relying on that alone, leaving a conversation mid-turn + * stranded the flag on whatever came next: a new chat showed the old turn thinking, offered a + * Stop with nothing to stop, and refused to send until the page was reloaded. + * + * Kept per conversation, whatever is on screen, so leaving can drop the flag and reopening a + * conversation whose turn is still running in this tab can put it back. + */ +const orchestrationSurfaces = new Set(); + +/** + * Bumped each time the reader starts a new chat or opens a conversation. + * + * A first message creates its conversation in a round trip, then claims the screen only if the + * reader is still in the new chat it was sent from. `activeConversationId === null` cannot say + * that on its own: clicking New chat during the round trip leaves it null as well, and the old + * question used to take over the fresh chat. Comparing the epoch from before the round trip can. + */ +let conversationEpoch = 0; + +/** The current epoch, for the creation path that lives outside this store (orchestration). */ +export function currentConversationEpoch(): number { + return conversationEpoch; +} + /** * Stop reading the in-flight stream without asking the server to stop producing it. * @@ -888,9 +918,10 @@ function detachActiveStream(): void { activeStreamController.abort(); activeStreamController = null; streamingConversationId = null; - // Included here rather than left to callers: neither `selectConversation` nor - // `startNewConversation` clears `streaming` itself, so dropping it would leave the - // composer stuck showing Stop for a stream that is no longer being read. + // Included here rather than left to callers: `stopStreaming` has no state reset of its + // own, so dropping it would leave the composer stuck showing Stop for a stream that is no + // longer being read. `selectConversation` and `startNewConversation` also set `streaming` + // themselves, because an orchestration turn holds it without a controller to detach. useChatStore.setState({ streaming: false, streamingContent: '', reconnectPhase: null }); } @@ -2084,6 +2115,7 @@ export const useChatStore = create((set, get) => ({ // keep delivering the previous conversation's messages into this one. stopCollaborationEvents(); useCollaborationStore.getState().reset(); + conversationEpoch += 1; // Known from the rail row when there is one, which is the common case and costs no // request. A conversation opened from a link is not in the list yet, so its kind is @@ -2104,6 +2136,12 @@ export const useChatStore = create((set, get) => ({ analysisTurnRevision: analysisConversationChanged ? null : get().analysisTurnRevision, messages: [], messagesError: null, + // Anything streaming belonged to the conversation being left. An orchestration turn + // still running in this tab for the one being opened gets its Thinking state and + // Stop back, which also keeps the composer from sending into a busy turn; its later + // thoughts and its settle land here as usual. A chat stream is picked back up by + // `resumeChatStream` below. + streaming: conversationId ? orchestrationSurfaces.has(conversationId) : false, streamingContent: '', thoughts: [], streamingReasoningAdjustments: [], @@ -2290,6 +2328,7 @@ export const useChatStore = create((set, get) => ({ detachActiveStream(); stopCollaborationEvents(); useCollaborationStore.getState().reset(); + conversationEpoch += 1; // The conversation row is created by the server on first send, so a new chat is // purely a local reset until then. This avoids leaving empty conversations behind @@ -2306,7 +2345,16 @@ export const useChatStore = create((set, get) => ({ // that already exists, so there is no way to start one shared. activeConversationKind: null, messages: [], + // A load still in flight for the conversation being left returns without touching + // this once it sees the reader has moved on, so the new chat would otherwise sit + // behind loading placeholders instead of its empty state. + messagesLoading: false, messagesError: null, + // Cleared here and not only by `detachActiveStream`, which knows about chat streams + // alone. An orchestration turn holds this flag without one, and its settle is skipped + // once its conversation is off screen, so the new chat kept the old turn's Thinking + // state and a Stop button with nothing to stop. The turn itself keeps running. + streaming: false, streamingContent: '', thoughts: [], streamingReasoningAdjustments: [], @@ -2723,6 +2771,9 @@ export const useChatStore = create((set, get) => ({ // one: a shared conversation always already exists, because sharing is done to a // conversation rather than at the moment of writing into one. if (!conversationId) { + // Taken before the round trip: New chat leaves `activeConversationId` null too, so + // null alone cannot say the reader is still in the chat this was sent from. + const epoch = conversationEpoch; try { const created = await createConversation(trimmed); conversationId = created.conversation_id; @@ -2732,8 +2783,9 @@ export const useChatStore = create((set, get) => ({ // the interface stays where they put it instead of snapping back to a chat // they have already left. Claiming `activeConversationId` unconditionally // also let the thread they had just opened render its messages under this - // new one, because the list is keyed on whatever is active. - if (get().activeConversationId === null) { + // new one, because the list is keyed on whatever is active. Starting another + // new chat is the same case, which the epoch is what tells apart. + if (get().activeConversationId === null && conversationEpoch === epoch) { set({ activeConversationId: conversationId, activeConversationKind: 'personal' }); // Mirrored like every other write to this field. Today the conversation just // created can only be personal — this branch is reached only when nothing was @@ -3058,10 +3110,13 @@ export const useChatStore = create((set, get) => ({ let conversationId = get().activeConversationId; const isNewConversation = !conversationId; if (!conversationId) { + // As in sendMessage: only the epoch tells "still in this new chat" apart from + // "started another one" while the conversation is being created. + const epoch = conversationEpoch; try { const created = await createConversation(trimmed); conversationId = created.conversation_id; - if (get().activeConversationId === null) { + if (get().activeConversationId === null && conversationEpoch === epoch) { set({ activeConversationId: conversationId, activeConversationKind: 'personal' }); useCollaborationStore.getState().setActiveConversation(conversationId); } @@ -3158,6 +3213,9 @@ export const useChatStore = create((set, get) => ({ beginOrchestrationTurn: (conversationId, text, addUserMessage = true, turnId, promptInfo) => { const trimmed = text.trim(); const pendingUserMessageId = addUserMessage ? `pending-user-${Date.now()}` : ''; + // Recorded whether or not the conversation is on screen, so opening it while this turn + // is still in flight shows it working. + orchestrationSurfaces.add(conversationId); // Guarded on the open conversation, exactly like sendMessage's optimistic write: a run // started here keeps going after the reader opens another thread, and its question must // not appear inside that other thread. @@ -3237,6 +3295,9 @@ export const useChatStore = create((set, get) => ({ }, settleOrchestrationTurn: (conversationId, outcome) => { + // Released whether or not the conversation is on screen, so reopening it later does not + // bring back a Thinking state for a turn that has already finished. + orchestrationSurfaces.delete(conversationId); // The reader is elsewhere: there is no streaming surface of this conversation's to // resolve, and the run's own record in the orchestration store is what remembers it ran. if (get().activeConversationId !== conversationId) { @@ -3394,6 +3455,10 @@ export const useChatStore = create((set, get) => ({ if (!conversationChanged && !turnChanged) { return; } + // The turn's claim on the streaming surface follows it to the id the server named. + if (conversationChanged && orchestrationSurfaces.delete(fromConversationId)) { + orchestrationSurfaces.add(toConversationId); + } set((state) => { const messages = state.messages.map((message) => { if (message.conversation_id !== fromConversationId) { diff --git a/docs/explanation/features/V2_GROUP_DOCUMENT_BROWSING.md b/docs/explanation/features/V2_GROUP_DOCUMENT_BROWSING.md index 501cbf769..25931dd6e 100644 --- a/docs/explanation/features/V2_GROUP_DOCUMENT_BROWSING.md +++ b/docs/explanation/features/V2_GROUP_DOCUMENT_BROWSING.md @@ -110,8 +110,11 @@ these reads. This is not a new group-only retrieval mode. The existing chat request mapping (`chat_type=user`, `doc_scope=all`, and the established active-group union) -remains unchanged. Existing conversation scope locks are not cleared or -retargeted; incompatible requests retain the conversation and show its error. +remains unchanged. From version 0.261.226, choosing **Chat** always starts a +brand-new conversation, so the selection is never added to whichever +conversation happened to be open. That conversation's scope lock is not cleared +or retargeted, and a reply still being written there finishes in the +background ([V2_NEW_CHAT_RESET_FIX.md](../fixes/V2_NEW_CHAT_RESET_FIX.md)). ## File structure diff --git a/docs/explanation/fixes/V2_NEW_CHAT_RESET_FIX.md b/docs/explanation/fixes/V2_NEW_CHAT_RESET_FIX.md new file mode 100644 index 000000000..b644aac6f --- /dev/null +++ b/docs/explanation/fixes/V2_NEW_CHAT_RESET_FIX.md @@ -0,0 +1,165 @@ +# V2 New Chat Keeps a Busy Conversation's Thinking State + +**Fixed in version:** 0.261.226 + +**Issue:** [#1617](https://github.com/microsoft/simplechat/issues/1617) + +## Issue + +In the V2 interface, starting a new conversation while the open one was busy did not give a +clean conversation: + +* Clicking **New chat** while chat orchestration was planning or running kept the old turn's + "Thinking" bubble and a **Stop** button that did nothing. The new chat's empty state never + appeared, and the composer refused to send until the page was reloaded. +* Clicking **New chat** while the first message of a new chat was still creating its + conversation let that message take over the new chat. Its question and answer appeared in + the chat the reader had just opened. +* Clicking **New chat** while a conversation's messages were loading left the loading + placeholders in the new chat instead of its empty state. +* Choosing **Chat** on selected workspace documents or a tag, or **Start chatting** on the + home page, opened whichever conversation was last open. The documents were attached to it, + even if a reply was still being written there. + +The classic interface always starts a fresh conversation in each of these cases. + +## Root cause + +### Orchestration turns stranded the `streaming` flag + +`chatStore.ts` has one `streaming` flag for the conversation on screen. It drives the +streaming bubble, the Stop button and the empty state, and `sendMessage` refuses to send while +it is set. + +A plain chat stream is owned by `activeStreamController`. Leaving its conversation runs +`detachActiveStream()`, which aborts the reader and clears `streaming`. `selectConversation` +and `startNewConversation` relied on that helper to clear the flag. + +An orchestration plan or run is driven by `orchestrationController.ts`, which uses its own +controllers. Its turn sets `streaming` in `beginOrchestrationTurn` and clears it in +`settleOrchestrationTurn`, but both only write while that conversation is on screen. So: + +1. `beginOrchestrationTurn` set `streaming: true`. +2. The reader clicked **New chat**. `detachActiveStream()` found no chat-stream controller and + returned early, so `streaming` stayed `true`. +3. When the turn finished, `settleOrchestrationTurn` saw that its conversation was no longer + on screen and skipped the write. + +The flag stayed set for the rest of the page's life. The Stop button routed to +`stopStreaming()`, which also found no controller and did nothing. + +### The first-message claim could not see a New chat + +A first message has to create its conversation (`POST /api/create_conversation`) before it +can stream. After that round trip, `sendMessage`, `generateImageFromReference` and the +orchestration `ensureConversation` claimed the screen if `activeConversationId` was still +`null`. That check was meant to catch the reader opening another conversation meanwhile. But +**New chat** also leaves the id `null`, so a new chat opened during the round trip was +claimed by the old question. + +### New chat did not reset `messagesLoading` + +`selectConversation` sets `messagesLoading: true` and, when its response arrives for a +conversation that is no longer open, returns without clearing it. `startNewConversation` did +not clear it either. + +### Hand-offs into chat reused the open conversation + +`DocumentExplorer.onChat`, `TagsSection.onChat` and the home page link navigated to `/chat` +without resetting the chat store. The store outlives a route change, so the last-open +conversation was still active when the chat page mounted. + +## Fix + +### Files modified + +| File | Change | +|---|---| +| `application/v2_ui/src/stores/chatStore.ts` | Adds a module-level `orchestrationSurfaces` set of conversations whose orchestration turn holds the streaming surface. `beginOrchestrationTurn` adds to it and `settleOrchestrationTurn` removes from it, both before their on-screen guards, and `reassignOrchestrationTurn` moves the entry when the server re-keys a conversation. `startNewConversation` now clears `streaming` and `messagesLoading`. `selectConversation` sets `streaming` from the set. Both advance a new `conversationEpoch`, exposed through `currentConversationEpoch()`. `sendMessage` and `generateImageFromReference` claim the screen only if the epoch is unchanged since the message was sent. | +| `application/v2_ui/src/lib/orchestrationController.ts` | `ensureConversation` uses the same epoch-guarded claim | +| `application/v2_ui/src/components/documents/DocumentExplorer.tsx` | **Chat** on selected documents calls `startNewConversation()` immediately before navigating | +| `application/v2_ui/src/pages/workspace/TagsSection.tsx` | **Chat** on a tag does the same | +| `application/v2_ui/src/pages/HomePage.tsx` | **Start chatting** starts a new chat when followed | +| `application/single_app/config.py` | Version `0.261.225` -> `0.261.226` | + +### Design notes + +* Leaving a busy conversation still **detaches rather than cancels**. The answer, or the + orchestration plan or run, keeps going and is saved. Only Stop ends work, as described in + [V2_STREAM_CANCELLED_ON_CONVERSATION_LEAVE_FIX.md](V2_STREAM_CANCELLED_ON_CONVERSATION_LEAVE_FIX.md). +* Reopening a conversation whose orchestration turn is still running in this tab restores its + Thinking state and Stop button, and the turn's next reasoning steps and answer land there as + usual. Reasoning steps that arrived while the reader was elsewhere aren't replayed. The plan + card and its step progress come from the orchestration store, which keeps updating while the + reader is away. +* A first message whose new chat was replaced during its round trip is still sent. Its answer + is generated and saved, and the conversation appears in the rail, as already happened when + the reader opened a different conversation instead. +* The composer keeps unsent text, chips and an attached prompt across **New chat**. The + documents or tag handed over by a workspace **Chat** action arrive as chips in the new chat. +* Clicking **Chats** in the navigation rail from another page still returns to a reply that is + streaming instead of resetting it. That exception is deliberate (see + [V2_NEW_CHAT_BUTTON_SCOPING_FIX.md](V2_NEW_CHAT_BUTTON_SCOPING_FIX.md)). With this + fix, `streaming` reflects a reply that is really in progress. + +## Behaviour after the fix + +* **New chat** always shows an empty conversation: the "New chat" title, the empty state, + **Send** and no Thinking bubble, whatever the previous conversation was doing. +* The previous conversation's plan, run or reply finishes in the background and is there when + it is reopened. +* A new chat opened while another first message is still being created stays empty. +* **New chat** while a conversation is loading shows the empty state. +* **Chat** on workspace documents or a tag, and **Start chatting** on the home page, open a + brand-new conversation. + +## Validation + +### Browser tests + +`ui_tests/test_v2_new_chat_reset.py` (7 tests) runs the production AppShell, Sidebar, HomePage, +ChatPage, Composer, stores, orchestration controller and SSE reader against held, server-shaped +streams. It covers: + +* **New chat** during planning. The test then reopens the conversation, checks that Thinking and + Stop are restored, leaves again, lets the plan finish out of sight, and checks that the plan + card is shown on return with no Thinking. +* **New chat** during a run. The run's answer must not appear in the new chat, the new chat must + be able to send at once, and Stop must end the new chat's own turn. +* **New chat** during a plain chat stream. This is a regression check: no cancel request may be + sent. +* **New chat** during the first message's `create_conversation` round trip, for both chat and + orchestration. +* **New chat** while a conversation's messages are loading. +* **Start chatting** on the home page while orchestration is planning. + +Six of the seven tests fail against the pre-fix code. The plain chat stream case already worked +and is kept as a regression check. + +Updated browser tests: + +* `ui_tests/test_v2_orchestration_planning_retry.py`: the late-retry test now also asserts that + `streaming` is released and that Send, not Stop, is shown. +* `ui_tests/test_v2_chat_context_selection.py`: the workspace **Chat** hand-off test now starts + from a busy conversation and expects a brand-new one, with the first message creating its own + conversation. + +All three updated tests fail against the pre-fix code. + +### Functional tests + +* `functional_tests/test_v2_new_chat_reset.py` (6 tests) pins the reset fields, the surface + bookkeeping and its ordering against the on-screen guards, the epoch-guarded claims in all + three creation paths, the hand-offs, and the version. +* `functional_tests/test_v2_stream_leave_without_cancel.py` now requires `startNewConversation` + and `selectConversation` to set `streaming` themselves, requires the epoch-guarded claim in + `sendMessage`, and follows the Stop button's `handleStop` routing. +* `functional_tests/test_v2_new_chat_scoping.py` selectors now match the Sidebar's current + `mobile` prop and inline click handler. + +## Related + +* [V2_STREAM_CANCELLED_ON_CONVERSATION_LEAVE_FIX.md](V2_STREAM_CANCELLED_ON_CONVERSATION_LEAVE_FIX.md) +* [V2_NEW_CHAT_BUTTON_SCOPING_FIX.md](V2_NEW_CHAT_BUTTON_SCOPING_FIX.md) +* `ui_tests/test_v2_new_chat_reset.py` +* `functional_tests/test_v2_new_chat_reset.py` diff --git a/docs/explanation/release_notes.md b/docs/explanation/release_notes.md index 2e554975c..9a2bae3be 100644 --- a/docs/explanation/release_notes.md +++ b/docs/explanation/release_notes.md @@ -2,6 +2,17 @@ For feature-focused and fix-focused drill-downs by version, see [Features by Version](https://github.com/microsoft/simplechat/tree/main/docs/explanation/features) and [Fixes by Version](https://github.com/microsoft/simplechat/tree/main/docs/explanation/fixes). +### **(v0.261.226)** + +#### Bug Fixes + +* **V2 New Chat Always Starts A Clean Conversation** + * Clicking **New chat** while chat orchestration was planning or running no longer carries the old turn into the new chat. Before, the new chat kept the previous turn's **Thinking** bubble and a **Stop** button that did nothing. Its empty state never appeared and it wouldn't send until the page was reloaded. + * The previous conversation's plan, run or reply keeps going in the background and is there when you reopen it. If you reopen it while its turn is still running, it shows as working, with **Stop**. + * Clicking **New chat** while the first message of a new chat is still creating its conversation no longer lets that message take over the new chat. The message is still sent, and its conversation appears in the list. Clicking **New chat** while a conversation is loading now shows the empty state instead of loading placeholders. + * **Chat** on selected workspace documents or on a tag, and **Start chatting** on the home page, now always open a brand-new conversation, as the classic interface does. The documents or tag arrive as context in the new chat instead of being added to whichever conversation was last open. Text you've typed but not sent stays in the message box. + * (Ref: #1617, `chatStore.ts`, `orchestrationController.ts`, `DocumentExplorer.tsx`, `TagsSection.tsx`, `HomePage.tsx`, [V2 New Chat Reset Fix](fixes/V2_NEW_CHAT_RESET_FIX.md)) + ### **(v0.261.224)** #### Bug Fixes diff --git a/docs/guides/manage-group-workspaces.md b/docs/guides/manage-group-workspaces.md index 9c6adec63..ae2ade419 100644 --- a/docs/guides/manage-group-workspaces.md +++ b/docs/guides/manage-group-workspaces.md @@ -78,10 +78,13 @@ whose scan was rejected or failed; chat, download, tagging, editing and sharing wait until a reviewer resolves the hold in Content review. On narrow screens, use **Filters** and the details toggle to open the same controls in dialogs. -Select eligible documents and choose **Chat** to carry their group/document -context into the existing chat experience. Current access is checked again -before those sources are adopted. This does not create a group-only retrieval -mode or override an existing conversation's workspace lock. +Select eligible documents and choose **Chat** to start a new conversation with +their group/document context attached. From version **0.261.226**, this always +opens a new conversation rather than adding the documents to whichever +conversation was open, and a reply still being written there keeps going. +Current access is checked again before those sources are adopted. This does not +create a group-only retrieval mode or change an existing conversation's +workspace lock. ### Manage group files and tags diff --git a/functional_tests/test_v2_new_chat_reset.py b/functional_tests/test_v2_new_chat_reset.py new file mode 100644 index 000000000..28c7ec58c --- /dev/null +++ b/functional_tests/test_v2_new_chat_reset.py @@ -0,0 +1,235 @@ +#!/usr/bin/env python3 +# test_v2_new_chat_reset.py +""" +Functional test for starting a new chat in V2 while the open conversation is busy. + +Version: 0.261.226 +Implemented in: 0.261.226 + +Clicking New chat while an orchestration turn was planning or running kept the old turn's +"Thinking" bubble and a Stop button with nothing to stop, hid the new chat's empty state, +and refused to send until the page was reloaded (issue #1617). An orchestration turn holds +the store's `streaming` flag without the chat-stream controller that `detachActiveStream` +clears, and its settle is skipped once its conversation is off screen, so nothing ever +cleared it. + +Three neighbouring defects are pinned here as well: a first message still creating its +conversation took over a new chat opened during that round trip, a conversation still +loading left its loading placeholders in the new chat, and the workspace Chat actions and +Home's Start chatting reopened whichever conversation was last open instead of starting a +new one. + +The browser behaviour is covered by ui_tests/test_v2_new_chat_reset.py; this test pins the +source decisions that behaviour depends on. +""" + +import re +import sys +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[1] +V2_SRC = REPO_ROOT / "application" / "v2_ui" / "src" + +sys.path.insert(0, str(REPO_ROOT / "functional_tests")) + +from test_support.versioning import assert_app_version_at_least # noqa: E402 + + +def _read(path): + return path.read_text(encoding="utf-8") + + +def _store(): + return _read(V2_SRC / "stores" / "chatStore.ts") + + +def _store_action_body(source, action_name): + """Return one store action's body, matched to its closing brace at store indentation. + + Sliced past the `ChatState` interface first, which declares every action with the same + `name: (` shape as the implementation. + """ + start = source.index("export const useChatStore = create") + implementation = source[start:] + match = re.search( + r"^ " + re.escape(action_name) + r": (?:async )?\((.|\n)*?\n \},", + implementation, + re.MULTILINE, + ) + assert match, f"Could not find the {action_name} store action" + return match.group(0) + + +def test_new_chat_releases_the_streaming_surface(): + """New chat clears `streaming` and `messagesLoading` itself, not only via the detach helper.""" + print("Testing the New chat reset...") + + start_new = _store_action_body(_store(), "startNewConversation") + + assert "streaming: false," in start_new, ( + "startNewConversation must clear `streaming`: an orchestration turn holds it without a " + "chat-stream controller, so detachActiveStream never clears it and the new chat keeps the " + "old turn's Thinking state and a Stop button with nothing to stop" + ) + assert "messagesLoading: false," in start_new, ( + "startNewConversation must clear `messagesLoading`: a load in flight for the conversation " + "being left returns early without clearing it, leaving placeholders in the new chat" + ) + assert "conversationEpoch += 1;" in start_new, ( + "startNewConversation must advance the conversation epoch so a first message still " + "creating its conversation cannot claim the new chat" + ) + assert start_new.index("detachActiveStream();") < start_new.index("streaming: false,"), ( + "The chat stream must still be detached (not cancelled) before the reset" + ) + + print("New chat reset test passed!") + return True + + +def test_opening_a_conversation_restores_only_a_running_orchestration_turn(): + """Opening a conversation drops the inherited flag and restores it only for its own turn.""" + print("Testing selectConversation's streaming flag...") + + select = _store_action_body(_store(), "selectConversation") + + assert "streaming: conversationId ? orchestrationSurfaces.has(conversationId) : false," in select, ( + "selectConversation must not inherit `streaming` from the conversation being left, and " + "should restore Thinking and Stop for an orchestration turn still running in the one " + "being opened" + ) + assert "conversationEpoch += 1;" in select, ( + "selectConversation must advance the conversation epoch" + ) + + print("selectConversation streaming test passed!") + return True + + +def test_orchestration_turns_record_their_streaming_surface_whatever_is_on_screen(): + """Begin claims and settle releases the surface before their on-screen guards.""" + print("Testing the orchestration surface bookkeeping...") + + store = _store() + assert "const orchestrationSurfaces = new Set();" in store, ( + "The store must track which conversations' orchestration turns hold the streaming surface" + ) + + begin = _store_action_body(store, "beginOrchestrationTurn") + claim = begin.index("orchestrationSurfaces.add(conversationId);") + assert claim < begin.index("if (get().activeConversationId === conversationId) {"), ( + "beginOrchestrationTurn must record the surface before its on-screen guard, so a turn " + "started out of sight still shows as working when its conversation is opened" + ) + + settle = _store_action_body(store, "settleOrchestrationTurn") + release = settle.index("orchestrationSurfaces.delete(conversationId);") + assert release < settle.index("if (get().activeConversationId !== conversationId) {"), ( + "settleOrchestrationTurn must release the surface before its on-screen guard, or a turn " + "that finished out of sight comes back as Thinking when its conversation is reopened" + ) + + reassign = _store_action_body(store, "reassignOrchestrationTurn") + assert re.search( + r"if \(conversationChanged && orchestrationSurfaces\.delete\(fromConversationId\)\) \{\s*" + r"orchestrationSurfaces\.add\(toConversationId\);", + reassign, + ), "A turn re-keyed to the server's conversation id must take its surface with it" + + print("Orchestration surface bookkeeping test passed!") + return True + + +def test_first_message_claims_need_an_unchanged_epoch(): + """Every creation path claims the screen only if no New chat or open happened meanwhile.""" + print("Testing the conversation-creation claims...") + + store = _store() + assert re.search(r"let conversationEpoch = 0;", store), "The epoch counter is missing" + assert "export function currentConversationEpoch(): number {" in store, ( + "The orchestration controller needs a reader for the epoch" + ) + + for action in ("sendMessage", "generateImageFromReference"): + body = _store_action_body(store, action) + assert "const epoch = conversationEpoch;" in body, ( + f"{action} must note the epoch before creating the conversation" + ) + assert "if (get().activeConversationId === null && conversationEpoch === epoch) {" in body, ( + f"{action} claims the screen whenever the id is still null, which New chat also " + f"leaves it; the question then takes over the fresh chat" + ) + + controller = _read(V2_SRC / "lib" / "orchestrationController.ts") + ensure = re.search(r"async function ensureConversation\((.|\n)*?\n\}", controller) + assert ensure, "Could not find ensureConversation" + body = ensure.group(0) + assert "const epoch = currentConversationEpoch();" in body, ( + "ensureConversation must note the epoch before creating the conversation" + ) + assert "if (chat.activeConversationId === null && currentConversationEpoch() === epoch) {" in body, ( + "ensureConversation claims the screen whenever the id is still null" + ) + + print("Conversation-creation claim test passed!") + return True + + +def test_chat_hand_offs_start_a_brand_new_conversation(): + """Workspace Chat actions and Home's Start chatting never reuse the open conversation.""" + print("Testing the hand-offs into chat...") + + explorer = _read(V2_SRC / "components" / "documents" / "DocumentExplorer.tsx") + assert re.search( + r"useChatStore\.getState\(\)\.startNewConversation\(\);\s*\n\s*navigate\(`/chat\?\$\{handoff\}`, \{ state \}\);", + explorer, + ), "Chat on selected documents must start a new chat immediately before navigating" + + tags = _read(V2_SRC / "pages" / "workspace" / "TagsSection.tsx") + assert re.search( + r"useChatStore\.getState\(\)\.startNewConversation\(\);\s*\n\s*navigate\(`/chat\?\$\{query\}`, \{ state \}\);", + tags, + ), "Chat on a tag must start a new chat immediately before navigating" + + home = _read(V2_SRC / "pages" / "HomePage.tsx") + assert re.search( + r" useChatStore\.getState\(\)\.startNewConversation\(\)\}", + home, + ), "Start chatting must open a new chat rather than the conversation last open" + + print("Hand-off test passed!") + return True + + +def test_version_is_at_least_implementation_version(): + """The application version is at or beyond the version that added the fix.""" + print("Testing application version...") + assert_app_version_at_least("0.261.226") + print("Application version test passed!") + return True + + +if __name__ == "__main__": + tests = [ + test_new_chat_releases_the_streaming_surface, + test_opening_a_conversation_restores_only_a_running_orchestration_turn, + test_orchestration_turns_record_their_streaming_surface_whatever_is_on_screen, + test_first_message_claims_need_an_unchanged_epoch, + test_chat_hand_offs_start_a_brand_new_conversation, + test_version_is_at_least_implementation_version, + ] + + results = [] + for test in tests: + print(f"\nRunning {test.__name__}...") + try: + results.append(bool(test())) + except Exception as exc: # noqa: BLE001 - surface any failure with a traceback + print(f"Test failed: {exc}") + import traceback + + traceback.print_exc() + results.append(False) + + print(f"\nResults: {sum(results)}/{len(results)} tests passed") + sys.exit(0 if all(results) else 1) diff --git a/functional_tests/test_v2_new_chat_scoping.py b/functional_tests/test_v2_new_chat_scoping.py index 77e8569e8..ec3e59dcf 100644 --- a/functional_tests/test_v2_new_chat_scoping.py +++ b/functional_tests/test_v2_new_chat_scoping.py @@ -2,8 +2,9 @@ """ Functional test for the V2 interface's New chat button. -Version: 0.261.044 +Version: 0.261.226 Implemented in: 0.261.044 +Updated in: 0.261.226 (selectors follow the Sidebar's `mobile` prop and inline click handler) Three defects met at one button. @@ -45,7 +46,8 @@ def _read(path): def _sidebar_component(): """The body of the exported Sidebar component, without its helper components.""" source = _read(V2_SRC / "components" / "layout" / "Sidebar.tsx") - component = re.search(r"export function Sidebar\(\)(.|\n)*", source) + # Matched on the name alone: the component takes props (`mobile`) now. + component = re.search(r"export function Sidebar\((.|\n)*", source) assert component, "Sidebar should export a Sidebar component" return component.group(0) @@ -67,7 +69,7 @@ def test_new_chat_is_only_offered_on_the_chat_page(): "The New chat button must be gated on the chat route. Rendered elsewhere it " "resets chat state that is not on screen and leaves the reader where they were" ) - assert "onClick={startNewConversation}" in button.group(0), ( + assert "startNewConversation();" in button.group(0), ( "The button must still start a new conversation" ) assert "New chat" in button.group(0), "The gated block must be the New chat button" diff --git a/functional_tests/test_v2_stream_leave_without_cancel.py b/functional_tests/test_v2_stream_leave_without_cancel.py index 7ffa785df..fe4709f80 100644 --- a/functional_tests/test_v2_stream_leave_without_cancel.py +++ b/functional_tests/test_v2_stream_leave_without_cancel.py @@ -2,8 +2,10 @@ """ Functional test for leaving a V2 conversation without cancelling its generation. -Version: 0.261.051 +Version: 0.261.226 Implemented in: 0.261.050 (cancellation), 0.261.051 (conversation-creation window) +Updated in: 0.261.226 (orchestration turns release `streaming` on leave; New chat during the +creation window; Stop routed through the Composer's handleStop) Sending a message and then opening a different conversation used to end the answer. Not because the reader was dropped -- that is harmless, generation runs in background execution @@ -200,12 +202,18 @@ def test_the_detach_helper_does_not_cancel(): def test_detaching_leaves_the_composer_usable(): - """The streaming flag has to be cleared by the helper, because no caller clears it. - - Neither `selectConversation` nor `startNewConversation` sets `streaming: false` in its - own state reset -- both relied on `stopStreaming` doing it. If the helper dropped it, - leaving a generating thread would strand the composer showing Stop for a stream nothing - is reading. + """The streaming flag is cleared by the helper and, for orchestration, by the callers. + + `stopStreaming` has no state reset of its own and relies on the helper to drop + `streaming`; if the helper stopped doing that, Stop would strand the composer showing + Stop for a stream nothing is reading. + + The helper only knows about chat streams, though. An orchestration turn holds the same + flag with no controller to detach, and its settle is skipped once its conversation is + off screen, so `startNewConversation` and `selectConversation` set `streaming` + themselves: New chat clears it, and opening a conversation restores it only for an + orchestration turn still running there. Without that, a new chat kept the old turn's + Thinking state and a dead Stop button until the page was reloaded. """ print("Testing composer state after detach...") @@ -213,19 +221,24 @@ def test_detaching_leaves_the_composer_usable(): detach = _function_body(store, r"function detachActiveStream\(\): void \{") assert "streaming: false" in detach, ( - "detachActiveStream must clear the streaming flag; its callers do not" + "detachActiveStream must clear the streaming flag; stopStreaming relies on it" ) assert "reconnectPhase: null" in detach, ( "A detach must clear any reconnect phase, or the next thread opens still claiming " "to be reconnecting" ) - for action in ("selectConversation", "startNewConversation"): - body = _store_action_body(store, action) - assert "streaming: false" not in body, ( - f"{action} now clears `streaming` itself. That is fine, but this test's reason " - f"for requiring it of detachActiveStream needs rechecking" - ) + start_new = _store_action_body(store, "startNewConversation") + assert "streaming: false," in start_new, ( + "startNewConversation must clear `streaming` itself: an orchestration turn holds it " + "with no chat-stream controller for detachActiveStream to find" + ) + + select = _store_action_body(store, "selectConversation") + assert "streaming: conversationId ? orchestrationSurfaces.has(conversationId) : false," in select, ( + "selectConversation must drop the streaming flag it inherited, restoring it only for " + "an orchestration turn still running in the conversation being opened" + ) print("Composer state test passed!") return True @@ -246,10 +259,16 @@ def test_stop_still_cancels(): "stopStreaming should share the teardown rather than repeat it" ) - # Stop is a user action, so it stays wired to the button rather than to navigation. + # Stop is a user action, so it stays wired to the button rather than to navigation. The + # button routes through handleStop, which picks the orchestration cancel when a plan or + # run is streaming and otherwise ends the chat stream with stopStreaming. composer = _read(V2_SRC / "components" / "chat" / "Composer.tsx") - assert "onClick={stopStreaming}" in composer, ( - "The Stop button is no longer wired to stopStreaming" + assert "onClick={handleStop}" in composer, ( + "The Stop button is no longer wired to handleStop" + ) + handle_stop = re.search(r"const handleStop = \(\) => \{(.|\n)*?\n \};", composer) + assert handle_stop and "stopStreaming();" in handle_stop.group(0), ( + "handleStop no longer reaches stopStreaming for an ordinary chat stream" ) print("Stop cancellation test passed!") @@ -333,19 +352,25 @@ def test_creating_a_conversation_does_not_take_the_screen_back(): The first message of a brand-new chat has to create the conversation before it can stream, and that round trip is a window in which the reader can click elsewhere. Claiming `activeConversationId` unconditionally afterwards snapped them back into the chat they - had just left. + had just left. Clicking New chat in that window leaves the id null too, so null alone + let the old question take over the fresh chat; the claim also needs the conversation + epoch to be unchanged since the message was sent. """ print("Testing conversation creation does not steal the screen...") store = _read(V2_SRC / "stores" / "chatStore.ts") send = _store_action_body(store, "sendMessage") + assert "const epoch = conversationEpoch;" in send, ( + "sendMessage must note the conversation epoch before creating the conversation" + ) assert re.search( - r"if \(get\(\)\.activeConversationId === null\) \{\s*\n\s*set\(\{ activeConversationId: conversationId", + r"if \(get\(\)\.activeConversationId === null && conversationEpoch === epoch\) \{\s*\n\s*set\(\{ activeConversationId: conversationId", send, ), ( "sendMessage claims activeConversationId without checking whether the reader opened " - "something else while the conversation was being created; their click gets undone" + "something else, or started another new chat, while the conversation was being " + "created; their click gets undone" ) print("Screen ownership on creation test passed!") diff --git a/ui_tests/fixtures/orchestration/harness_entry.tsx b/ui_tests/fixtures/orchestration/harness_entry.tsx index e0ae4e646..6ee969ad4 100644 --- a/ui_tests/fixtures/orchestration/harness_entry.tsx +++ b/ui_tests/fixtures/orchestration/harness_entry.tsx @@ -38,6 +38,7 @@ import { ParticipantsPanel } from '../../../application/v2_ui/src/components/cha import { Toaster } from '../../../application/v2_ui/src/components/ui/Toaster'; import { AppShell } from '../../../application/v2_ui/src/components/layout/AppShell'; import { ChatPage } from '../../../application/v2_ui/src/pages/ChatPage'; +import { HomePage } from '../../../application/v2_ui/src/pages/HomePage'; import { ModelCatalogManager } from '../../../application/v2_ui/src/components/admin/ModelCatalogManager'; import { OrchestrationPlannerModelPicker } from '../../../application/v2_ui/src/components/admin/OrchestrationPlannerModelPicker'; import { PLANNER_MODEL_KEYS } from '../../../application/v2_ui/src/lib/orchestrationPlannerModel'; @@ -47,6 +48,20 @@ function ChatExperience() { return
; } +/** The landing page and the chat page behind one shell, so its call to action can be followed. */ +function HomeExperience() { + return ( +
+ + + } /> + } /> + + +
+ ); +} + function PlanEditorExperience() { const conversationId = chatStore.useChatStore((state) => state.activeConversationId); const drawer = chatStore.useChatStore((state) => state.drawerMode); @@ -147,6 +162,7 @@ type ComponentName = | 'Composer' | 'PromptExperience' | 'ChatExperience' + | 'HomeExperience' | 'ApprovalPreferenceWorkflow' | 'PlannerModelWorkflow' | 'PlanEditorExperience' @@ -167,6 +183,7 @@ const components: Record ReactElement | null> = { Composer, PromptExperience, ChatExperience, + HomeExperience, ApprovalPreferenceWorkflow, PlannerModelWorkflow, PlanEditorExperience, diff --git a/ui_tests/test_v2_chat_context_selection.py b/ui_tests/test_v2_chat_context_selection.py index 5e4afc855..d3df6f5c1 100644 --- a/ui_tests/test_v2_chat_context_selection.py +++ b/ui_tests/test_v2_chat_context_selection.py @@ -1,12 +1,13 @@ # test_v2_chat_context_selection.py """ Browser regressions for V2 context selection and explicitly chosen inline mentions. -Version: 0.261.183 +Version: 0.261.226 Implemented in: 0.261.094 Single orchestration contract updated in: 0.261.181 (the React V2 branch's 0.261.139) Shared editor and prompt dispatch regression coverage added in: 0.261.096 The public chat list is answered as its route answers it (chat_list), with no generated artifact awaiting publication: 0.261.183 +The workspace Chat action starts a brand-new conversation instead of reusing the open one: 0.261.226 The real Composer, DocumentExplorer, stores, router, and request builders run in the existing Playwright harness. The shared connection fixture supports a configured Azure @@ -42,6 +43,8 @@ pytestmark = pytest.mark.ui CONVERSATION_ID = "context-selection-chat" +# The conversation the workspace hand-off's first message creates. +HANDOFF_CONVERSATION_ID = "context-handoff-chat" SCOPES = { "personal": {"kind": "personal", "id": None, "name": "My workspace"}, "group": {"kind": "group", "id": "group-1", "name": "Marketing"}, @@ -113,6 +116,14 @@ def handle(self, route: Route): path = url.path self.requests.append((request.method, path)) + if request.method == "POST" and path == "/api/create_conversation": + route.fulfill(json={"conversation_id": HANDOFF_CONVERSATION_ID, "title": "New chat"}) + return + if request.method == "GET" and path == "/api/conversations/feed": + # Re-read once a new conversation's first answer lands. + route.fulfill(json={"conversations": [], "has_more": False, "next_cursor": None}) + return + if request.method == "POST" and path in STREAM_PATHS.values(): if path == STREAM_PATHS["chat"]: frames = [{"response": "Mock answer."}, {"done": True}] @@ -376,7 +387,7 @@ def delete_range(page: Page, start, end): draft.press("Backspace") -def send_draft(page: Page, api: ContextApi, dispatch="chat"): +def send_draft(page: Page, api: ContextApi, dispatch="chat", conversation_id=CONVERSATION_ID): draft = page.get_by_role("textbox", name="Message", exact=True) message = draft.input_value() endpoint = STREAM_PATHS[dispatch] @@ -387,7 +398,7 @@ def send_draft(page: Page, api: ContextApi, dispatch="chat"): payload = sent.value.post_data_json assert payload["message"] == message - assert payload["conversation_id"] == CONVERSATION_ID + assert payload["conversation_id"] == conversation_id expect(draft).to_have_value("") expect_pills(page) page.wait_for_function( @@ -407,7 +418,9 @@ def send_draft(page: Page, api: ContextApi, dispatch="chat"): ) assert state["error"] is None assert [user["content"] for user in state["users"]] == [message] - assert [path for method, path in api.requests if method == "POST"] == [endpoint] + # A new chat's first message creates its conversation before it is sent. + created = [] if conversation_id == CONVERSATION_ID else ["/api/create_conversation"] + assert [path for method, path in api.requests if method == "POST"] == [*created, endpoint] if dispatch == "plan": assert state["plans"] == 1 return payload @@ -532,6 +545,17 @@ def test_tag_or_workspace_alone_enables_search_with_its_scope(context_page, scop def test_workspace_chat_action_hands_real_selection_to_pills_only(context_page): page, api = context_page mount_workflow(page, "/workspace", strict_mode=True) + # The conversation left open in chat is busy: a reply is still being written into it. + page.evaluate( + """(conversationId) => window.OrchHarness.stores.chat.useChatStore.setState({ + streaming: true, + messages: [{ + id: 'earlier-question', conversation_id: conversationId, + role: 'user', content: 'An earlier question in another chat.', + }], + })""", + CONVERSATION_ID, + ) page.get_by_role("checkbox", name="Select Quarterly brief", exact=True).check() page.get_by_role("checkbox", name="Select Budget notes", exact=True).check() page.get_by_role("button", name="Chat", exact=True).first.click() @@ -540,10 +564,19 @@ def test_workspace_chat_action_hands_real_selection_to_pills_only(context_page): draft = page.get_by_role("textbox", name="Message", exact=True) expect(draft).to_have_value("") expect_pills(page, "Quarterly brief", "Budget notes") + # Chatting with documents always starts a brand-new conversation, not the busy one. + state = page.evaluate( + """() => { + const state = window.OrchHarness.stores.chat.useChatStore.getState(); + return {active: state.activeConversationId, streaming: state.streaming, messages: state.messages}; + }""" + ) + assert state == {"active": None, "streaming": False, "messages": []}, state + expect(page.get_by_role("button", name="Send message", exact=True)).to_be_visible() assert ("GET", "/api/documents/personal-brief") not in api.requests assert ("GET", "/api/documents/personal-budget") not in api.requests draft.fill("Compare my selected documents.") - payload = send_draft(page, api) + payload = send_draft(page, api, conversation_id=HANDOFF_CONVERSATION_ID) expect_context_metadata(payload, documents=["personal-brief", "personal-budget"]) draft.fill("Start the next turn without those pills.") expect_pills(page) diff --git a/ui_tests/test_v2_new_chat_reset.py b/ui_tests/test_v2_new_chat_reset.py new file mode 100644 index 000000000..1b6259370 --- /dev/null +++ b/ui_tests/test_v2_new_chat_reset.py @@ -0,0 +1,619 @@ +# test_v2_new_chat_reset.py +""" +Browser regressions for starting a new chat in V2 while the open conversation is busy. +Version: 0.261.226 +Implemented in: 0.261.226 + +Clicking New chat while an orchestration turn was planning or running kept the old turn's +"Thinking" bubble and a Stop button with nothing to stop, hid the new chat's empty state, +and refused to send until the page was reloaded. A first message still creating its +conversation could take the new chat over, a conversation still loading left its loading +placeholders in the new chat, and Home's Start chatting reopened whichever conversation +was last open (issue #1617). + +The production AppShell, Sidebar, HomePage, ChatPage, Composer, MessageList, stores, +orchestration controller and SSE reader run in Chromium with production CSS. Only HTTP is +deterministic: plan, run and chat streams are held open and fed one server-shaped frame at +a time, and conversation creation and message loading can be held, so New chat lands +inside exactly the round trip each defect needed. No live model, deployment or workspace +content is used. + +Build CSS with the existing V2 build, keeping outputs in UI test artifacts: +npm --prefix .\\application\\v2_ui run build -- --outDir ..\\..\\ui_tests\\artifacts\\orchestration-plan-editor +Run: python -m pytest .\\ui_tests\\test_v2_new_chat_reset.py -q +""" + +from urllib.parse import parse_qs, unquote, urlsplit + +import pytest +from playwright.sync_api import expect + +import test_v2_orchestration_plan_editor as editor_tests +from test_v2_orchestration_plan_editor import ( # noqa: F401 + connect_options, + editor_assets, + editor_browser, +) + + +pytestmark = pytest.mark.ui +BUSY = "busy-review" +BUSY_TITLE = "Busy review" +OTHER = "other-review" +OTHER_TITLE = "Other review" +CREATED = "fresh-chat" +BACKGROUND = "background-chat" +BACKGROUND_TITLE = "Background question" +PLAN = "/api/v2/orchestration/plan" +RUN = editor_tests.RUN +CHAT = "/api/chat/stream" +QUESTION = "Compare the quarterly reports and summarise how revenue changed." +FOLLOW_UP = "Draft a one-line status update for the team." +EARLIER_QUESTION = "Which reports are in this review?" +EARLIER_ANSWER = "Last quarter's report is attached to this review." +OTHER_ANSWER = "The other review is about supplier contracts." +DECIDING = "Deciding what this question needs." +ANSWER = "Revenue rose 12% between the two quarters." +UPDATE = "Revenue is up 12% quarter on quarter." +EMPTY_STATE = "Start a conversation with SimpleChat" + +# Plan, run and chat responses stay open until the test ends them, keyed by path and the +# conversation the request names, so the turn that was left and the new chat's own turn can +# be fed separately. Like a real fetch, an aborted request fails its body, which is how a +# detached chat reader stops. Other requests go to the page router. +INSTALL_STREAMS = r""" +(paths) => { + const encoder = new TextEncoder(); + const passThrough = window.fetch.bind(window); + const key = (path, conversation) => path + '|' + conversation; + window.__streams = {}; + window.emitFrame = (path, conversation, event) => { + window.__streams[key(path, conversation)].enqueue( + encoder.encode('data: ' + JSON.stringify(event) + '\n\n'), + ); + }; + window.endStream = (path, conversation) => { + try { + window.__streams[key(path, conversation)].close(); + } catch (error) { + // The reader already stopped at the terminal frame or was aborted. + } + }; + window.fetch = (input, init = {}) => { + const url = new URL(input instanceof Request ? input.url : String(input), location.href); + if (!paths.includes(url.pathname)) { + return passThrough(input, init); + } + let body = {}; + try { + body = JSON.parse(init.body || '{}'); + } catch (error) { + body = {}; + } + const conversation = String(body.conversation_id || ''); + const stream = new ReadableStream({ + start(controller) { + window.__streams[key(url.pathname, conversation)] = controller; + init.signal?.addEventListener('abort', () => { + try { + controller.error(new DOMException('The operation was aborted.', 'AbortError')); + } catch (error) { + // Already closed. + } + }); + }, + }); + return Promise.resolve(new Response(stream, { + status: 200, + headers: { 'Content-Type': 'text/event-stream' }, + })); + }; +} +""" + + +class NewChatApi: + """Serves the harness and answers the chat page's reads; anything else fails the test. + + Conversation creation and message loading can be held, so New chat can be clicked while + either round trip is still open. + """ + + def __init__(self, assets): + self.assets = assets + self.errors = [] + self.requests = [] + self.conversations = [{"id": BUSY, "title": BUSY_TITLE}] + self.messages = {BUSY: earlier_messages()} + self.next_created = (CREATED, "New chat") + self.hold_create = False + self.hold_messages = False + self.pending = [] + + def handle(self, route): + request = route.request + url = urlsplit(request.url) + path = url.path + if request.method == "GET" and path in self.assets: + route.fulfill(path=str(self.assets[path])) + return + if path == "/favicon.ico": + route.fulfill(status=204) + return + self.requests.append((request.method, path)) + if path == "/api/create_conversation" and request.method == "POST": + if self.hold_create: + self.pending.append(("create", route)) + else: + self.create(route) + return + if path == "/api/get_messages": + conversation = parse_qs(url.query).get("conversation_id", [""])[0] + if self.hold_messages: + self.pending.append(("messages", route, conversation)) + else: + route.fulfill(json={"messages": self.messages.get(conversation, [])}) + return + if path in ("/api/conversations/feed", "/api/get_conversations"): + route.fulfill(json={"conversations": self.conversations, "has_more": False, "next_cursor": None}) + return + if path.startswith("/api/conversations/") and path.endswith("/metadata"): + conversation = unquote(path.split("/")[3]) + title = next( + (item["title"] for item in self.conversations if item["id"] == conversation), + "New chat", + ) + route.fulfill(json={"conversation_id": conversation, "title": title}) + return + if path.startswith("/api/chat/stream/status/"): + route.fulfill(json={"active": False, "pending": False, "reattachable": False}) + return + if path.startswith("/api/chat/stream/cancel/"): + # Answered so a regression shows up as an assertion, not a hung request. + route.fulfill(json={"success": True}) + return + if path == "/api/v2/orchestration/runs": + route.fulfill(json={"runs": []}) + return + if path == "/api/collaboration/file-approvals": + route.fulfill(json={"approvals": []}) + return + if path == "/api/user/settings": + route.fulfill(json={"settings": {}}) + return + if path == "/api/user/collaboration-suggestions": + route.fulfill(json={"results": []}) + return + self.errors.append(f"{request.method} {path}") + route.fulfill(status=404, json={"error": "Unexpected test request."}) + + def create(self, route): + conversation, title = self.next_created + self.conversations = [{"id": conversation, "title": title}, *self.conversations] + route.fulfill(json={"conversation_id": conversation, "title": title}) + + def release(self): + pending, self.pending = self.pending, [] + for held in pending: + if held[0] == "create": + self.create(held[1]) + else: + held[1].fulfill(json={"messages": self.messages.get(held[2], [])}) + + def created(self): + return [request for request in self.requests if request == ("POST", "/api/create_conversation")] + + +def earlier_messages(): + return [ + {"id": "busy-q0", "conversation_id": BUSY, "role": "user", "content": EARLIER_QUESTION}, + {"id": "busy-a0", "conversation_id": BUSY, "role": "assistant", "content": EARLIER_ANSWER}, + ] + + +def planning_thought(content, status="running"): + """A planner reasoning step, shaped as build_planning_thought serializes it.""" + return { + "type": "thought", + "message_id": None, + "step_index": 1, + "step_type": "orchestration_planning", + "content": content, + "activity": { + "lane_key": "orchestration", + "kind": "orchestration_planning", + "title": "Building a plan", + "status": status, + }, + } + + +def run_done(plan, turn, conversation=BUSY): + """A run's terminal frame, shaped as build_run_done_event serializes a completed run.""" + return { + "done": True, + "type": "orchestration_done", + "conversation_id": conversation, + "message_id": f"{conversation}-answer", + "run_id": plan["run_id"], + "full_content": ANSWER, + "hybrid_citations": [], + "web_search_citations": [], + "agent_citations": [], + "augmented": False, + "generated_artifacts": [], + "orchestration": {}, + "status": "completed", + "outcome": "completed", + "turn_id": turn, + "attempt_index": 1, + "retry_of_run_id": None, + "failure": None, + "failures": [], + "recovery": None, + "message_saved": True, + "reasoning_adjustments": [], + } + + +@pytest.fixture +def chat_ui(editor_browser, editor_assets): + context = editor_browser.new_context(viewport={"width": 1280, "height": 900}) + page = context.new_page() + api = NewChatApi(editor_assets) + page.route("**/*", api.handle) + page.on("pageerror", lambda error: api.errors.append(str(error))) + page.on("console", lambda message: api.errors.append(message.text) if message.type == "error" else None) + try: + yield page, api + finally: + api.release() + context.close() + assert not api.errors, api.errors + + +def mount(page, api, *, conversation=BUSY, orchestration=True, experience="ChatExperience", entry="/chat"): + page.goto(editor_tests.ORIGIN + editor_tests.HARNESS) + page.wait_for_function("() => Boolean(window.OrchHarness)") + for url in api.assets: + if url.endswith(".css"): + page.add_style_tag(url=editor_tests.ORIGIN + url) + page.evaluate(INSTALL_STREAMS, [PLAN, RUN, CHAT]) + page.evaluate( + """(spec) => { + const H = window.OrchHarness; + H.reset(); + H.stores.bootstrap.useBootstrapStore.setState({data: { + features: {enable_chat_orchestration: spec.orchestration}, + settings: {}, user: {id: 'tester', display_name: 'Tester'}, + catalogs: {models: [], agents: [], prompts: []}, + scope: {groups: [], public_workspaces: []}, + orchestration: { + enabled: spec.orchestration, capabilities: [], default_approval_mode: 'manual', + }, + }}); + H.stores.chat.useChatStore.setState({ + activeConversationId: spec.conversation, + activeConversationKind: spec.conversation ? 'personal' : null, + messages: spec.messages, messagesLoading: false, messagesError: null, + streaming: false, streamError: null, streamingContent: '', thoughts: [], + conversations: spec.conversations, + }); + H.mount('mount-a', spec.experience, {}, {initialEntries: [spec.entry]}); + }""", + { + "conversation": conversation, + "orchestration": orchestration, + "experience": experience, + "entry": entry, + "messages": api.messages.get(conversation, []) if conversation else [], + "conversations": api.conversations, + }, + ) + if experience == "ChatExperience": + expect(page.get_by_title("Start a new chat", exact=True)).to_be_visible() + + +def chat_state(page): + return page.evaluate( + """() => { + const S = window.OrchHarness.stores.chat.useChatStore.getState(); + return { + active: S.activeConversationId, streaming: S.streaming, loading: S.messagesLoading, + messages: S.messages.map((message) => message.content), + }; + }""" + ) + + +def wait_until(page, predicate, message): + """Let Playwright dispatch held routes until a Python-side condition holds.""" + for _ in range(200): + if predicate(): + return + page.wait_for_timeout(25) + raise AssertionError(message) + + +def emit(page, path, conversation, event): + page.evaluate( + "([path, conversation, event]) => window.emitFrame(path, conversation, event)", + [path, conversation, event], + ) + + +def end_stream(page, path, conversation): + page.evaluate("([path, conversation]) => window.endStream(path, conversation)", [path, conversation]) + + +def wait_for_stream(page, path, conversation): + page.wait_for_function( + "([path, conversation]) => Boolean(window.__streams[path + '|' + conversation])", + arg=[path, conversation], + ) + + +def wait_for_plan(page, conversation, turn): + """The plan is stored for its own conversation, which proves its frames were all read.""" + page.wait_for_function( + """([conversation, turn]) => { + const H = window.OrchHarness; + return Boolean(H.stores.orchestration.selectPlan( + H.stores.orchestration.useOrchestrationStore.getState(), conversation, turn)); + }""", + arg=[conversation, turn], + ) + + +def thinking(page): + return page.get_by_text("Thinking", exact=True) + + +def stop_button(page): + return page.get_by_role("button", name="Stop generating", exact=True) + + +def send_button(page): + return page.get_by_role("button", name="Send message", exact=True) + + +def draft(page): + return page.get_by_role("textbox", name="Message", exact=True) + + +def approve_button(page): + return page.get_by_role("button", name="Approve and run the plan") + + +def click_new_chat(page): + # Matched on the title, which the button keeps whether the rail is expanded or collapsed. + page.get_by_title("Start a new chat", exact=True).click() + + +def open_from_rail(page, title): + page.get_by_role("navigation", name="Primary").get_by_role("button", name=title, exact=True).click() + expect(page.get_by_role("heading", name=title, level=1)).to_be_visible() + + +def expect_clean_new_chat(page, *absent): + """Everything a new chat shows, and nothing the conversation that was left showed.""" + expect(page.get_by_role("heading", name="New chat", level=1)).to_be_visible() + expect(page.get_by_text(EMPTY_STATE, exact=True)).to_be_visible() + expect(thinking(page)).to_have_count(0) + expect(stop_button(page)).to_have_count(0) + expect(send_button(page)).to_be_visible() + for text in absent: + expect(page.get_by_text(text, exact=True)).to_have_count(0) + state = chat_state(page) + assert state == {"active": None, "streaming": False, "loading": False, "messages": []}, state + + +def start_plan(page, conversation=BUSY, message=QUESTION): + page.evaluate( + """(spec) => { void window.OrchHarness.controller.startOrchestrationPlan(spec); }""", + {"conversationId": conversation, "message": message, "approvalMode": "manual", "seeds": {}}, + ) + wait_for_stream(page, PLAN, conversation) + return page.evaluate( + "(conversation) => window.OrchHarness.stores.orchestration.useOrchestrationStore" + ".getState().activeTurns[conversation]", + conversation, + ) + + +def finish_plan(page, turn, conversation=BUSY): + plan = editor_tests.make_plan(conversation, turn) + emit(page, PLAN, conversation, planning_thought("Plan ready.", status="completed")) + emit(page, PLAN, conversation, {"type": "orchestration_plan", "plan": plan, "done": True}) + end_stream(page, PLAN, conversation) + wait_for_plan(page, conversation, turn) + return plan + + +def persist_question(api, turn): + """What the server holds for the conversation once its question has been asked.""" + api.messages[BUSY] = [ + *earlier_messages(), + { + "id": "busy-question", "conversation_id": BUSY, "role": "user", "content": QUESTION, + "metadata": {"orchestration_turn_id": turn}, + }, + ] + + +def test_new_chat_while_planning_starts_clean_and_returning_restores_the_turn(chat_ui): + page, api = chat_ui + mount(page, api) + turn = start_plan(page) + persist_question(api, turn) + expect(thinking(page)).to_be_visible() + expect(stop_button(page)).to_be_visible() + expect(page.get_by_text(QUESTION, exact=True)).to_be_visible() + + click_new_chat(page) + expect_clean_new_chat(page, QUESTION, EARLIER_ANSWER) + + # The plan is still being written for the conversation that was left. Going back shows it + # working, with Stop, and its next reasoning step lands there as usual. + open_from_rail(page, BUSY_TITLE) + expect(page.get_by_text(QUESTION, exact=True)).to_be_visible() + expect(thinking(page)).to_be_visible() + expect(stop_button(page)).to_be_visible() + emit(page, PLAN, BUSY, planning_thought(DECIDING)) + expect(page.get_by_role("button", name="1 reasoning step", exact=True)).to_be_visible() + + # Leave again and let the plan finish out of sight: it must not appear in the new chat. + click_new_chat(page) + expect_clean_new_chat(page, QUESTION, EARLIER_ANSWER) + finish_plan(page, turn) + expect(approve_button(page)).to_have_count(0) + expect_clean_new_chat(page, QUESTION, EARLIER_ANSWER) + + # Its turn settled while the reader was away, so coming back shows the plan, not Thinking. + open_from_rail(page, BUSY_TITLE) + expect(approve_button(page).first).to_be_visible() + expect(thinking(page)).to_have_count(0) + expect(stop_button(page)).to_have_count(0) + expect(send_button(page)).to_be_visible() + assert chat_state(page)["streaming"] is False + + +def test_new_chat_while_a_plan_runs_keeps_the_answer_in_its_own_chat_and_can_send(chat_ui): + page, api = chat_ui + mount(page, api) + turn = start_plan(page) + plan = finish_plan(page, turn) + approve_button(page).first.click() + wait_for_stream(page, RUN, BUSY) + expect(thinking(page)).to_be_visible() + expect(stop_button(page)).to_be_visible() + + click_new_chat(page) + expect_clean_new_chat(page, QUESTION, EARLIER_ANSWER) + + emit(page, RUN, BUSY, {"content": ANSWER}) + emit(page, RUN, BUSY, run_done(plan, turn)) + end_stream(page, RUN, BUSY) + page.wait_for_function( + """(conversation) => !Object.values(window.OrchHarness.stores.orchestration + .useOrchestrationStore.getState().inFlight) + .some((run) => run.conversationId === conversation)""", + arg=BUSY, + ) + expect_clean_new_chat(page, QUESTION, EARLIER_ANSWER, ANSWER) + + # The composer is usable straight away: the new chat's own first question goes out. + draft(page).fill(FOLLOW_UP) + send_button(page).click() + wait_for_stream(page, PLAN, CREATED) + expect(page.get_by_text(FOLLOW_UP, exact=True)).to_be_visible() + expect(thinking(page)).to_be_visible() + assert chat_state(page)["active"] == CREATED + assert len(api.created()) == 1 + expect(page.get_by_text(QUESTION, exact=True)).to_have_count(0) + + # Stop now belongs to the new chat's turn, and ends it. + stop_button(page).click() + expect(thinking(page)).to_have_count(0) + expect(send_button(page)).to_be_visible() + + +def test_new_chat_while_a_chat_reply_streams_detaches_without_cancelling(chat_ui): + page, api = chat_ui + mount(page, api, orchestration=False) + draft(page).fill(QUESTION) + send_button(page).click() + wait_for_stream(page, CHAT, BUSY) + expect(thinking(page)).to_be_visible() + expect(stop_button(page)).to_be_visible() + + click_new_chat(page) + expect_clean_new_chat(page, QUESTION, EARLIER_ANSWER) + # Leaving stops reading; only Stop may end the answer being written. + assert ("POST", f"/api/chat/stream/cancel/{BUSY}") not in api.requests + + draft(page).fill(FOLLOW_UP) + send_button(page).click() + wait_for_stream(page, CHAT, CREATED) + expect(page.get_by_text(FOLLOW_UP, exact=True)).to_be_visible() + emit(page, CHAT, CREATED, {"content": UPDATE}) + emit(page, CHAT, CREATED, { + "done": True, "conversation_id": CREATED, "message_id": "fresh-answer", "full_content": UPDATE, + }) + end_stream(page, CHAT, CREATED) + expect(page.get_by_text(UPDATE, exact=True)).to_be_visible() + expect(thinking(page)).to_have_count(0) + expect(page.get_by_text(QUESTION, exact=True)).to_have_count(0) + + +@pytest.mark.parametrize("mode", ["chat", "plan"]) +def test_new_chat_while_the_first_message_creates_its_conversation_stays_empty(chat_ui, mode): + page, api = chat_ui + api.next_created = (BACKGROUND, BACKGROUND_TITLE) + api.hold_create = True + mount(page, api, conversation=None, orchestration=mode == "plan") + draft(page).fill(QUESTION) + send_button(page).click() + wait_until(page, lambda: api.pending, "The new chat never asked to create its conversation") + + click_new_chat(page) + api.release() + + # The question is still sent, from the conversation it created... + path = CHAT if mode == "chat" else PLAN + wait_for_stream(page, path, BACKGROUND) + # ...but the chat the reader opened since stays theirs. + expect_clean_new_chat(page, QUESTION) + + if mode == "chat": + emit(page, CHAT, BACKGROUND, { + "done": True, "conversation_id": BACKGROUND, "message_id": "background-answer", + "full_content": ANSWER, + }) + end_stream(page, CHAT, BACKGROUND) + else: + turn = page.evaluate( + "(conversation) => window.OrchHarness.stores.orchestration.useOrchestrationStore" + ".getState().activeTurns[conversation]", + BACKGROUND, + ) + finish_plan(page, turn, BACKGROUND) + # The answer is kept with its own conversation, which the rail lists. + expect( + page.get_by_role("navigation", name="Primary").get_by_role("button", name=BACKGROUND_TITLE, exact=True) + ).to_be_visible() + expect(approve_button(page)).to_have_count(0) + expect_clean_new_chat(page, QUESTION, ANSWER) + + +def test_new_chat_while_a_conversation_loads_shows_the_empty_state(chat_ui): + page, api = chat_ui + api.conversations = [{"id": BUSY, "title": BUSY_TITLE}, {"id": OTHER, "title": OTHER_TITLE}] + api.messages[OTHER] = [ + {"id": "other-a0", "conversation_id": OTHER, "role": "assistant", "content": OTHER_ANSWER}, + ] + mount(page, api) + api.hold_messages = True + page.get_by_role("navigation", name="Primary").get_by_role("button", name=OTHER_TITLE, exact=True).click() + wait_until(page, lambda: api.pending, "Opening the conversation never asked for its messages") + assert chat_state(page)["loading"] is True + + click_new_chat(page) + with page.expect_response(lambda response: urlsplit(response.url).path == "/api/get_messages"): + api.release() + expect_clean_new_chat(page, OTHER_ANSWER, EARLIER_ANSWER) + + +def test_home_start_chatting_opens_a_new_chat_not_the_busy_one(chat_ui): + page, api = chat_ui + mount(page, api, experience="HomeExperience", entry="/") + turn = start_plan(page) + assert chat_state(page)["streaming"] is True + + page.get_by_role("link", name="Start chatting", exact=True).click() + expect_clean_new_chat(page, QUESTION, EARLIER_ANSWER) + + # The turn that was left carries on and finishes where it belongs. + finish_plan(page, turn) + expect(approve_button(page)).to_have_count(0) + expect_clean_new_chat(page, QUESTION, EARLIER_ANSWER) diff --git a/ui_tests/test_v2_orchestration_planning_retry.py b/ui_tests/test_v2_orchestration_planning_retry.py index 6602d9e4d..5a4a3f792 100644 --- a/ui_tests/test_v2_orchestration_planning_retry.py +++ b/ui_tests/test_v2_orchestration_planning_retry.py @@ -1,8 +1,9 @@ # test_v2_orchestration_planning_retry.py """ Browser regressions for failed, unpersisted orchestration planning turns. -Version: 0.261.115 +Version: 0.261.226 Implemented in: 0.261.115 +Leaving a planning turn for a new chat also releases its Thinking state and Stop: 0.261.226 The production controller, stores, SSE reader, message actions, and composer run with production CSS. Only HTTP responses are deterministic. No live model, @@ -302,9 +303,16 @@ def test_late_retry_does_not_write_into_a_new_or_deleted_conversation(planning_u const H = window.OrchHarness; const S = H.stores.chat.useChatStore.getState(); return {id: S.activeConversationId, messages: S.messages, error: S.streamError, + streaming: S.streaming, plans: H.stores.orchestration.useOrchestrationStore.getState().plans}; }""") assert state["id"] is None and state["messages"] == [] and state["error"] is None + # The retry's Thinking state belonged to the conversation that was left, so the composer is + # ready to send rather than offering a Stop with nothing to stop. + assert state["streaming"] is False + expect(page.get_by_text("Thinking", exact=True)).to_have_count(0) + expect(page.get_by_role("button", name="Stop generating", exact=True)).to_have_count(0) + expect(page.get_by_role("button", name="Send message", exact=True)).to_be_visible() if action == "delete": assert state["plans"] == {} assert not api.calls(editor_tests.RUN)