-
Notifications
You must be signed in to change notification settings - Fork 517
fix(ui): keep loading below pending chat message #5041
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| import assert from 'node:assert/strict'; | ||
| import test from 'node:test'; | ||
| import { createElement, type ComponentProps } from 'react'; | ||
| import { renderToStaticMarkup } from 'react-dom/server'; | ||
| import { parseHTML } from 'linkedom'; | ||
| import type { SessionSummary, StoredMessage } from '@maka/core/session'; | ||
| import { ChatSurfaceLayout } from '../chat-surface-layout.js'; | ||
| import { ChatView, type TransientUserMessageProjection } from '../chat-view.js'; | ||
| import type { LiveTurnProjection } from '../live-turn-projection.js'; | ||
| import { LocaleProvider } from '../locale-context.js'; | ||
|
|
||
| const activeSession = { | ||
| id: 'session-1', name: 'Session', status: 'running', labels: [], | ||
| } as unknown as SessionSummary; | ||
|
|
||
| const settledRound: StoredMessage[] = [ | ||
| { type: 'user', id: 'u1', turnId: 't1', ts: 1, text: 'first' }, | ||
| { type: 'assistant', id: 'a1', turnId: 't1', ts: 2, text: 'answer', modelId: 'test' }, | ||
| ]; | ||
|
|
||
| const pendingSend: TransientUserMessageProjection = { | ||
| id: 'u2', text: 'second', ts: 3, transientPlacement: 'current_turn', | ||
| }; | ||
|
|
||
| function renderChat(props: Partial<ComponentProps<typeof ChatView>> = {}): string { | ||
| const view = createElement(ChatView, { | ||
| messages: settledRound, | ||
| activeSession, | ||
| onNew: () => undefined, | ||
| scrollBehavior: 'auto', | ||
| runningStatus: true, | ||
| ...props, | ||
| }); | ||
| return renderToStaticMarkup(createElement(LocaleProvider, { | ||
| locale: 'en', | ||
| children: createElement(ChatSurfaceLayout, { composer: null, children: view }), | ||
| })); | ||
| } | ||
|
|
||
| test('places loading after a pending send whose running turn is not loaded', () => { | ||
| const markup = renderChat({ | ||
| activeSession: { ...activeSession, runningTurnIds: ['t2'] }, | ||
| transientMessages: [pendingSend], | ||
| }); | ||
| const pendingMessage = markup.indexOf('data-transient-message-id="u2"'); | ||
| const loading = markup.indexOf('Waiting for model output'); | ||
|
|
||
| assert.doesNotMatch(markup, /data-turn-id="t1"[^>]*data-live-streaming="true"/); | ||
| assert.ok(pendingMessage >= 0 && pendingMessage < loading); | ||
| }); | ||
|
|
||
| test('keeps loading at the boundary without a unique runtime identity', () => { | ||
| const messages: StoredMessage[] = [ | ||
| ...settledRound, | ||
| { type: 'user', id: 'u2', turnId: 't2', ts: 3, text: 'second' }, | ||
| ]; | ||
| const { document } = parseHTML(renderChat({ | ||
| messages, | ||
| activeSession: { ...activeSession, runningTurnIds: ['t2', 'unloaded-turn'] }, | ||
| })); | ||
| assert.equal(document.querySelector('section[data-turn-id][data-live-streaming]'), null); | ||
| const status = document.querySelector('.maka-turn-processing'); | ||
| assert.ok(status); | ||
| assert.equal(status.closest('section')?.hasAttribute('data-turn-id'), false); | ||
| }); | ||
|
|
||
| test('a live turn outranks a conflicting directory identity', () => { | ||
| const messages: StoredMessage[] = [ | ||
| ...settledRound, | ||
| { type: 'user', id: 'u2', turnId: 't2', ts: 3, text: 'second' }, | ||
| ]; | ||
| const liveTurn: LiveTurnProjection = { | ||
| turnId: 't2', | ||
| phase: 'waiting', | ||
| steps: [], | ||
| }; | ||
| const markup = renderChat({ | ||
| messages, | ||
| liveTurn, | ||
| activeSession: { ...activeSession, runningTurnIds: ['t1'] }, | ||
| }); | ||
| const { document } = parseHTML(markup); | ||
|
|
||
| assert.equal(document.querySelectorAll('.maka-turn-processing').length, 1); | ||
| assert.equal(document.querySelector('.maka-turn-processing')?.closest('section')?.getAttribute('data-turn-id'), 't2'); | ||
| assert.equal(document.querySelector('section[data-turn-id="t1"]')?.getAttribute('data-live-streaming'), null); | ||
| }); | ||
|
|
||
| test('keeps the newer gap after old history when the running turn is not loaded', () => { | ||
| const markup = renderChat({ | ||
| hasNewerHistory: true, | ||
| activeSession: { ...activeSession, runningTurnIds: ['t2'] }, | ||
| }); | ||
|
|
||
| assert.doesNotMatch(markup, /data-turn-id="t1"[^>]*data-live-streaming="true"/); | ||
| const gap = markup.indexOf('data-transcript-gap="newer"'); | ||
| assert.ok(markup.indexOf('data-turn-id="t1"') < gap); | ||
| assert.ok(gap < markup.indexOf('Waiting for model output')); | ||
| }); | ||
|
|
||
| test('recorded terminal evidence outranks a stale runtime ID', () => { | ||
| const markup = renderChat({ | ||
| activeSession: { ...activeSession, runningTurnIds: ['t1'] }, | ||
| messages: [ | ||
| ...settledRound, | ||
| { type: 'turn_state', id: 'done', turnId: 't1', ts: 3, status: 'completed' }, | ||
| ], | ||
| hasNewerHistory: true, | ||
| }); | ||
| const { document } = parseHTML(markup); | ||
| const settledTurn = document.querySelector('section[data-turn-id="t1"]')!; | ||
|
|
||
| assert.equal(settledTurn.getAttribute('data-live-streaming'), null); | ||
| assert.doesNotMatch(markup, /Waiting for model output/); | ||
| assert.ok(markup.indexOf('data-turn-id="t1"') < markup.indexOf('data-transcript-gap="newer"')); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -423,13 +423,12 @@ export function ChatView(props: { | |
| // transcript a second, independent authority whose outputs then had to be | ||
| // interned by value to line up again. | ||
| const turnPresentation = props.deriveTurnPresentation?.(turns); | ||
| // #642 single render path: the in-flight answer is injected into the tail | ||
| // #642 single render path: the in-flight answer is injected into its own | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3 Two comment leftovers. The edit here breaks mid-sentence — 中文两处注释残留。这里的改动断在半句上—— |
||
| // turn's TurnView (the SAME node as the eventual committed turn) instead of a | ||
| // separate streaming <section>, so live→settled is a data-source swap, not an | ||
| // unmount/mount. The streaming turn is always the last turn: the user message | ||
| // is committed optimistically (showOptimisticUserMessage) before streaming | ||
| // starts, so `materializeTurns` already emits it — with an empty assistant | ||
| // timeline — as `turns[last]`. Only the tail TurnView gets a fresh | ||
| // unmount/mount. The Host may identify a running turn before its prompt | ||
| // reaches the transcript, so identity and materialization are separate. | ||
| // Only the identified TurnView gets a fresh | ||
| // `liveStreaming` object per delta (→ it alone re-renders); every sibling | ||
| // gets a stable `undefined` and its memo skips. That the sibling's `turn` | ||
| // prop is also stable is the projection's tested contract, not a property | ||
|
|
@@ -441,10 +440,8 @@ export function ChatView(props: { | |
| // settled branch, whose derived status is `completed`, rendering an actionable | ||
| // footer on a still-running answer (review P2-B). A tool-only tail renders the | ||
| // running tool from its timeline with no empty live bubble. | ||
| // The model-wait indicator keeps the tail turn "live" too, so its footer stays | ||
| // the non-actionable placeholder and the indicator injects into the tail turn | ||
| // (not the fallback section) — it is, by derivation, only ever true when text / | ||
| // thinking / tools are all absent. | ||
| // The running indicator rides the whole turn, keeping its footer | ||
| // non-actionable even between content events. | ||
| // | ||
| // Terminal liveTurn is evidence overlay only (e.g. empty shell_run still needs | ||
| // pre-handoff chunks). It must NOT block footer actions — keeping evidence and | ||
|
|
@@ -463,18 +460,29 @@ export function ChatView(props: { | |
| // content or the row is hidden behind the empty hero. | ||
| const hasLiveCompactionRow = isCompactionLive && (props.liveTurn?.steps.length ?? 0) === 0; | ||
| const liveInFlight = !!(props.liveTurn && !props.liveTurn.terminal) && !isCompactionLive; | ||
| const streamingActive = | ||
| liveInFlight || (!props.liveTurn?.terminal && !!props.runningStatus && !isCompactionLive); | ||
| const tailTurnId = liveInFlight | ||
| // Ordinary sends may have a Host running ID before any live content arrives. | ||
| // Unknown or concurrent IDs cannot identify a single owner from row order. | ||
| const runningTurnId = props.activeSession?.runningTurnIds?.length === 1 | ||
| ? props.activeSession.runningTurnIds[0] | ||
| : undefined; | ||
| const identifiedTurnId = liveInFlight | ||
| ? props.liveTurn!.turnId | ||
| : (streamingActive ? turns[turns.length - 1]?.turnId : undefined); | ||
| const hasRenderedLiveTurn = tailTurnId !== undefined && turns.some((turn) => turn.turnId === tailTurnId); | ||
| : runningTurnId; | ||
| const identifiedTurn = turns.find((turn) => turn.turnId === identifiedTurnId); | ||
| // A directory refresh may lag the transcript's terminal record. Inferred | ||
| // legacy status does not supply that evidence; a live projection still wins. | ||
| const recordedRuntimeTurnEnded = !liveInFlight | ||
| && identifiedTurn?.statusSource === 'recorded' && identifiedTurn.status !== 'running'; | ||
| const streamingActive = !recordedRuntimeTurnEnded && ( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 (reachability ②) A stale recorded terminal turns the indicator off entirely, including under a freshly sent prompt that hasn't materialized yet. The path: t1 completes → the transcript records Probed ( The old code put the indicator on the wrong (completed) turn here; the new code removes it altogether — the same class of problem this PR set out to kill, in a different shape. const staleIdentity = !liveInFlight
&& identifiedTurn?.statusSource === 'recorded' && identifiedTurn.status !== 'running';
const hasPendingCurrentTurnSend = transientMessages.some(
(message) => message.transientPlacement === 'current_turn',
);
const streamingActive = (!staleIdentity || hasPendingCurrentTurnSend) && (
liveInFlight || (!props.liveTurn?.terminal && !!props.runningStatus && !isCompactionLive)
);
const activeTurnId = streamingActive && !staleIdentity ? identifiedTurnId : undefined;
const activeTurn = streamingActive && !staleIdentity ? identifiedTurn : undefined;With no transient the behaviour is unchanged — I checked that 中文(可达②)陈旧的 recorded 终态会把指示器整个关掉,连刚发出、还没物化的新 prompt 下面也没有。 路径:t1 完成 → transcript 落了 实测( 旧代码在这里是把指示器错贴到已完成的 t1 上;新代码是让它彻底消失——正是本 PR 想消灭的那类问题换了个形态。 无 transient 时行为不变——我核对过 |
||
| liveInFlight || (!props.liveTurn?.terminal && !!props.runningStatus && !isCompactionLive) | ||
| ); | ||
| const activeTurnId = streamingActive ? identifiedTurnId : undefined; | ||
| const activeTurn = streamingActive ? identifiedTurn : undefined; | ||
| const pendingRunningStartedAt = transientMessages.findLast((message) => | ||
| message.transientPlacement === 'current_turn' | ||
| && (tailTurnId === undefined || message.hostTurnId === undefined || message.hostTurnId === tailTurnId), | ||
| && (activeTurnId === undefined || message.hostTurnId === undefined || message.hostTurnId === activeTurnId), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3 When 中文
|
||
| )?.ts ?? props.liveTurn?.startedAt; | ||
| const boundaryOverlayTurnId = props.liveTurn?.turnId | ||
| ?? (streamingActive ? tailTurnId : undefined); | ||
| const boundaryOverlayTurnId = props.liveTurn?.turnId ?? activeTurnId; | ||
| const transcriptRows = useMemo(() => projectTranscriptRows({ | ||
| turns, | ||
| hasOlder: props.hasOlderHistory === true, | ||
|
|
@@ -604,21 +612,19 @@ export function ChatView(props: { | |
| const railAlignment = resolveRailAlignedTarget(railClaimRef.current, props.scrollTargetTurn); | ||
| railClaimRef.current = railAlignment.claim; | ||
| const scrollTargetTurn = railAlignment.target; | ||
| const inlineTransientMessages = tailTurnId | ||
| const inlineTransientMessages = activeTurn | ||
| ? transientMessages.filter((message) => { | ||
| const turn = turns.find((candidate) => candidate.turnId === tailTurnId); | ||
| if ( | ||
| turn === undefined | ||
| || turn.user !== undefined | ||
| || turn.timeline.some((item) => item.kind === 'user' && item.messageId === message.id) | ||
| activeTurn.user !== undefined | ||
| || activeTurn.timeline.some((item) => item.kind === 'user' && item.messageId === message.id) | ||
| ) { | ||
| return false; | ||
| } | ||
| // An unbound row belongs to the Turn the user is looking at; a bound | ||
| // one only renders inline in the Turn the Host named. | ||
| return ( | ||
| message.transientPlacement === 'current_turn' | ||
| && (message.hostTurnId === undefined || message.hostTurnId === tailTurnId) | ||
| && (message.hostTurnId === undefined || message.hostTurnId === activeTurnId) | ||
| ); | ||
| }) | ||
| : []; | ||
|
|
@@ -869,7 +875,7 @@ export function ChatView(props: { | |
| > | ||
| <TurnView | ||
| turn={turn} | ||
| transientMessages={turn.turnId === tailTurnId ? inlineTransientMessages : undefined} | ||
| transientMessages={turn.turnId === activeTurnId ? inlineTransientMessages : undefined} | ||
| userLabel={props.userLabel} | ||
| footerActions={turnPresentation?.footerActionsByTurn[turn.turnId]} | ||
| onFooterAction={stableTurnFooterAction} | ||
|
|
@@ -898,7 +904,7 @@ export function ChatView(props: { | |
| } | ||
| searchHighlighted={highlightedTurnId === turn.turnId} | ||
| liveStreaming={ | ||
| turn.turnId === tailTurnId | ||
| turn.turnId === activeTurnId | ||
| ? { | ||
| onStreamingSettled: props.onStreamingSettled, | ||
| runningStatus: props.runningStatus, | ||
|
|
@@ -925,10 +931,11 @@ export function ChatView(props: { | |
| message={message} | ||
| /> | ||
| ))} | ||
| {/* A send arm already names its Turn, but the transcript may not | ||
| contain it yet. Keep feedback below the pending prompt until | ||
| that same TurnView can take over. */} | ||
| {streamingActive && !hasRenderedLiveTurn && ( | ||
| {/* #642 fallback: the live turn has no materialized transcript row | ||
| to own the stream yet, so render the answer at the transcript | ||
| boundary instead of dropping it or claiming an older turn. | ||
| Mutually exclusive with the tail injection above. */} | ||
| {streamingActive && !activeTurn && ( | ||
| <section className="maka-turn" data-live-streaming="true"> | ||
| <LocalizedChatMessage | ||
| accessibleLabel={conversationCopy.messages.assistantAriaLabel} | ||
|
|
||
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.
P3 This assertion depends on JSX attribute order.
assert.doesNotMatch(markup, /data-turn-id="t1"[^>]*data-live-streaming="true"/)only means anything whilechat-turn.tsx:486-487keepsdata-turn-idbeforedata-live-streaming; swap those two lines and the assertion silently becomes vacuously true. Same at:108-109and:130-131, and themarkup.indexOf(...)ordering comparisons have the same problem.The third case in this very file already does it right — copy that:
中文
这条断言依赖 JSX 属性的书写顺序。
assert.doesNotMatch(markup, /data-turn-id="t1"[^>]*data-live-streaming="true"/)只在chat-turn.tsx:486-487保持data-turn-id先于data-live-streaming时才有意义;一旦有人调换这两行,断言会静默变成永真。:108-109和:130-131同理,markup.indexOf(...)的顺序比较也有同样问题。同一个文件的第三个用例已经写对了,照抄即可(见上)。