Skip to content

fix(web): consolidate chat display-message hacks into sanitize pipeline #32009

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.

Sign up for GitHub

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

Merged
dvargasfuertes merged 1 commit into from
May 25, 2026
Merged

fix(web): consolidate chat display-message hacks into sanitize pipeline #32009

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 11 additions & 15 deletions apps/web/src/domains/chat/components/chat-route-content.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,9 @@ import { useDeployStore } from "@/domains/chat/deploy-store.js";
import { useInteractionStore } from "@/domains/interactions/interaction-store.js";
import type { SubagentEntry, SubagentState } from "@/domains/subagents/subagent-store.js";
import type { DisplayAttachment, DisplayMessage } from "@/domains/chat/utils/reconcile.js";
import { sortedByTimestamp } from "@/domains/chat/utils/message-sorting.js";

import { buildTranscriptItems } from "@/domains/chat/transcript/build-items.js";
import { sanitizeDisplayMessages } from "@/domains/chat/utils/sanitize-display-messages.js";
import type { TranscriptPaginationState } from "@/domains/chat/transcript/types.js";
import type { HistoryPaginationResult } from "@/domains/chat/transcript/use-history-pagination.js";
import {
Expand Down Expand Up @@ -774,20 +775,15 @@ export function ChatRouteContent({

const thinkingLabel = getThinkingStatusText(turnState);

// Defensive ascending-timestamp sort at the render boundary. The internal
// mutators (`setMessages` in stream handlers, `reconcile`, etc.) try to
// keep the array sorted, but a handful of paths can land rows out of
// order — multi-row server clusters with the same `daemonMessageId`, a
// late tool-result event for an earlier bubble, history pages stitched
// around an in-flight stream, etc. Sorting here guarantees the user
// always sees messages in chronological order, regardless of which write
// path landed last. The deeper fix (server-side cluster merging so the
// client never sees the fragmented rows) is tracked separately.
//
// `sortedByTimestamp` is stable: rows without a `timestamp` keep their
// original slot, and equal timestamps preserve insertion order, so
// streaming bubbles don't flicker.
const sortedMessages = useMemo(() => sortedByTimestamp(messages), [messages]);
// Single render-boundary cleanup pass. `sanitizeDisplayMessages` houses
// every "this shouldn't be necessary, but is" hack we apply before the
// transcript renders (timestamp sort, blank/phantom row filter, duplicate
// trailing assistant drop). See `sanitize-display-messages.ts` for the
// rationale and removal triggers for each sub-step.
const sortedMessages = useMemo(
() => sanitizeDisplayMessages(messages),
[messages],
);

const transcriptItems = useMemo(
Copy link
Copy Markdown
Contributor

@dvargasfuertes dvargasfuertes May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between transcriptItems and display messages? are they different data models? do we need both?

() =>
Expand Down
92 changes: 11 additions & 81 deletions apps/web/src/domains/chat/transcript/build-items.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,34 +267,14 @@ describe("buildTranscriptItems", () => {
});

// ---------------------------------------------------------------------------
// Phantom tool-only message filter (ATL-659)
// Phantom tool-only message filter (ATL-659) — pass-through behaviour only
//
// The daemon synthesises tool calls with `toolName === "unknown"` when a
// tool_result block has no matching tool_use (orphan). They arrive as
// empty user messages whose only payload is a list of unknown tool calls
// and would otherwise render as a confusing "Completed 1 step / Used
// unknown" chip. Drop them at the projection step.
// The drop-the-phantom logic now lives in `sanitizeDisplayMessages` and is
// exercised by `sanitize-display-messages.test.ts`. The tests below
// confirm that `buildTranscriptItems` does NOT mistakenly drop legitimate
// mixed-tool messages — the positive half of the original spec.
// ---------------------------------------------------------------------------

test("phantom tool-only messages (all toolName === 'unknown') are dropped", () => {
const phantom = makeMessage({
id: "m1",
role: "user",
content: "",
stableId: "s-phantom",
toolCalls: [
{ id: "tc-1", toolName: "unknown", input: {}, status: "completed", result: "orphan" },
],
});

const items = buildTranscriptItems({
...emptyInput(),
messages: [phantom],
});

expect(items).toHaveLength(0);
});

test("mixed messages with unknown tool calls alongside content are kept", () => {
const mixed = makeMessage({
id: "m1",
Expand Down Expand Up @@ -407,65 +387,15 @@ describe("buildTranscriptItems", () => {
});

// ---------------------------------------------------------------------------
// Blank user-row filter — pagination-boundary orphans
// Blank user-row filter — pass-through behaviour only
//
// At a history-pagination boundary, the runtime keeps tool_result-only
// user rows even when their parent tool_use lives on a previous page
// (to avoid permanent data loss). `renderHistoryContent` then drops the
// orphan tool_result block, leaving the row on the wire with no content,
// no segments, no surfaces, no attachments, and no tool calls — a blank
// user bubble. The projection layer drops these so they don't render.
// The drop-blank-rows logic now lives in `sanitizeDisplayMessages` and is
// exercised by `sanitize-display-messages.test.ts`. The tests below
// confirm that `buildTranscriptItems` does NOT mistakenly drop legitimate
// user rows whose `content` is empty but which carry segments, surfaces,
// attachments, or are in a queued state — the positive half of the spec.
// ---------------------------------------------------------------------------

test("truly blank user rows (no content, no segments, no surfaces, no attachments, no tool calls) are dropped", () => {
const blank = makeMessage({
id: "m1",
role: "user",
content: "",
stableId: "s-blank-server",
});

const items = buildTranscriptItems({
...emptyInput(),
messages: [blank],
});

expect(items).toHaveLength(0);
});

test("blank user rows with whitespace-only content are dropped", () => {
const whitespace = makeMessage({
id: "m1",
role: "user",
content: " \n\t ",
stableId: "s-blank-ws",
});

const items = buildTranscriptItems({
...emptyInput(),
messages: [whitespace],
});

expect(items).toHaveLength(0);
});

test("blank user rows with empty textSegments are dropped", () => {
const emptySegments = makeMessage({
id: "m1",
role: "user",
content: "",
stableId: "s-empty-segments",
textSegments: [{ type: "text", content: "" }],
});

const items = buildTranscriptItems({
...emptyInput(),
messages: [emptySegments],
});

expect(items).toHaveLength(0);
});

test("user rows with non-empty textSegments are kept (even if content is empty)", () => {
// Some history paths populate textSegments instead of (or in addition to)
// the flat content field — those rows are meaningful and must render.
Expand Down
30 changes: 3 additions & 27 deletions apps/web/src/domains/chat/transcript/build-items.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,30 +53,10 @@ export interface BuildTranscriptItemsInput {
* d. `ErrorItem` when `errorNotice` is a non-empty string.
*
* Every returned item carries a non-empty, distinct `key`.
*
* `messages` must already be sanitized — phantom/blank row filtering and
* trailing-duplicate drops happen upstream in `sanitizeDisplayMessages`.
Comment on lines +56 to +58
Copy link
Copy Markdown
Contributor

@dvargasfuertes dvargasfuertes May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete this comment

*/
function isInvalidMessage(message: DisplayMessage): boolean {
// Assistant rows always render; queued user rows collapse into a marker upstream.
if (message.role !== "user") return false;
if (message.queueStatus === "queued") return false;

// Any meaningful signal short-circuits as valid. Without one of these the
// row is a blank bubble (e.g. an orphan tool_result at a pagination boundary
// that the daemon's renderer already stripped).
if (message.content && message.content.trim().length > 0) return false;
if (
message.textSegments?.some(
(s) => typeof s.content === "string" && s.content.trim().length > 0,
)
)
return false;
if (message.surfaces && message.surfaces.length > 0) return false;
if (message.attachments && message.attachments.length > 0) return false;
if (message.slackMessage) return false;
if (message.toolCalls?.some((tc) => tc.toolName !== "unknown")) return false;

return true;
}

export function buildTranscriptItems(
input: BuildTranscriptItemsInput,
): TranscriptItem[] {
Expand Down Expand Up @@ -105,10 +85,6 @@ export function buildTranscriptItems(
continue;
}

if (isInvalidMessage(message)) {
continue;
}

const isQueuedUser =
message.role === "user" && message.queueStatus === "queued";

Expand Down
9 changes: 8 additions & 1 deletion apps/web/src/domains/chat/utils/debug-api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import type {
} from "@/domains/chat/types/chat-ui-types.js";
import { recordChatDiagnostic } from "@/domains/chat/utils/diagnostics.js";
import type { DisplayMessage } from "@/domains/chat/utils/reconcile.js";
import { sanitizeDisplayMessages } from "@/domains/chat/utils/sanitize-display-messages.js";
import type { ReconcileActiveConversationResult } from "@/domains/chat/hooks/use-message-reconciliation.js";
import {
classifyScrollPosition,
Expand Down Expand Up @@ -379,7 +380,13 @@ export function createChatDebugApi(refs: ChatDebugRefs): ChatDebugApi {
Number.isFinite(limit) && limit > 0
? Math.floor(limit)
: DEFAULT_TAIL_LIMIT;
const messages = refs.messagesRef.current ?? [];
// Sanitize so `tail()` returns the same array the UI ends up rendering
// (the chat-route render path also pipes `messages` through
// `sanitizeDisplayMessages`). Without this we'd surface trailing
// duplicates / blank rows that the UI is intentionally filtering out,
// which would be misleading when triaging "why does my chat look like X"
// reports.
const messages = sanitizeDisplayMessages(refs.messagesRef.current ?? []);
const startIndex = Math.max(0, messages.length - safeLimit);
return messages.slice(startIndex);
}
Expand Down
Loading