From fbe7292c2de32a49988d84be047a38a2a3bd0fc2 Mon Sep 17 00:00:00 2001 From: "vellum-apollo-bot[bot]" <242025090+vellum-apollo-bot[bot]@users.noreply.github.com> Date: Mon, 25 May 2026 15:42:07 +0000 Subject: [PATCH] fix(web): apply PR #32009 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #32009 feedback follow-up: * sanitize-display-messages.ts - Trim header comment: drop the parenthetical about `window._vellumDebug.chat.tail()` (tail() no longer touches the pipeline directly — see debug-api change below). - Replace the redundant JSDoc + sequential lets in `sanitizeDisplayMessages` with a `pipeline.reduce` over the three named steps — the implementation now is the spec. - Drop the local `sortByTimestamp` wrapper; the imported `sortedByTimestamp` is the named pipeline step directly. - Update Hack #2 docblock + SHORT TERM line to use the codebase's 'assistant' terminology and drop the inline ticket reference. * debug-api.ts - `tail()` is now logic-free: it reads from a new `sanitizedMessagesRef` populated by the render path. DevTools mirrors the UI without re-running the sanitize pipeline. Drops the `sanitizeDisplayMessages` import inside debug-api entirely. * chat-page.tsx + chat-route-content.tsx - Own `sanitizedMessagesRef` in the composition root, thread it through `ChatRouteRefs`, populate it right after the `useMemo(() => sanitizeDisplayMessages(messages))`. Rename `sortedMessages` → `sanitizedMessages` to match what it actually is. * build-items.ts - Remove the comment claiming `messages` must already be sanitized. The function takes whatever it's given; sanitization isn't its contract. * debug-api.test.ts - Migrate the existing tail() tests to populate `sanitizedMessagesRef` (the new contract). Add a contract test that pins 'tail() reads from sanitizedMessagesRef, NOT raw messagesRef' so the two refs can't be silently swapped back. --- apps/web/src/domains/chat/chat-page.tsx | 7 +++ .../chat/components/chat-route-content.tsx | 20 ++++++-- .../domains/chat/transcript/build-items.ts | 3 -- .../src/domains/chat/utils/debug-api.test.ts | 50 +++++++++++++++---- apps/web/src/domains/chat/utils/debug-api.ts | 21 ++++---- .../chat/utils/sanitize-display-messages.ts | 34 ++++--------- 6 files changed, 85 insertions(+), 50 deletions(-) diff --git a/apps/web/src/domains/chat/chat-page.tsx b/apps/web/src/domains/chat/chat-page.tsx index 5e108f17d9f..7ca8234cf88 100644 --- a/apps/web/src/domains/chat/chat-page.tsx +++ b/apps/web/src/domains/chat/chat-page.tsx @@ -248,6 +248,11 @@ export function ChatPage() { const inputRef = useRef(null); const messagesRef = useRef(messages); messagesRef.current = messages; + // Populated by `chat-route-content.tsx` right after the render-boundary + // `useMemo(() => sanitizeDisplayMessages(messages))`. Owned here so + // `useChatDebugApi` (mounted in this component) can read the same array + // the UI is rendering without re-running the pipeline. + const sanitizedMessagesRef = useRef([]); // Owned here so `useChatDebugApi` (also called from this component) can // read scroll geometry directly via `transcriptRef.current.getScrollElement()`. // Threaded down to ChatRouteContent through the `refs` prop and bound on @@ -891,6 +896,7 @@ export function ChatPage() { // by which point initialization is complete. useChatDebugApi({ messagesRef, + sanitizedMessagesRef, transcriptRef, streamContextRef, streamRef, @@ -1559,6 +1565,7 @@ export function ChatPage() { refs: { inputRef, messagesRef, + sanitizedMessagesRef, activeConversationIdRef, assistantIdRef, streamContextRef, diff --git a/apps/web/src/domains/chat/components/chat-route-content.tsx b/apps/web/src/domains/chat/components/chat-route-content.tsx index c161a72aa41..91df49c5269 100644 --- a/apps/web/src/domains/chat/components/chat-route-content.tsx +++ b/apps/web/src/domains/chat/components/chat-route-content.tsx @@ -218,6 +218,13 @@ export interface AvatarData { export interface ChatRouteRefs { inputRef: RefObject; messagesRef: MutableRefObject; + /** + * Mirror of the post-`sanitizeDisplayMessages` array, populated below right + * after the render-boundary `useMemo`. Read by `useChatDebugApi`'s `tail()` + * so DevTools sees exactly what the transcript renders without re-running + * the sanitize pipeline. + */ + sanitizedMessagesRef: MutableRefObject; activeConversationIdRef: MutableRefObject; assistantIdRef: MutableRefObject; streamContextRef: MutableRefObject; @@ -503,6 +510,7 @@ export function ChatRouteContent({ const { inputRef, messagesRef, + sanitizedMessagesRef, activeConversationIdRef: _activeConversationIdRef, assistantIdRef: _assistantIdRef, streamContextRef, @@ -780,15 +788,19 @@ export function ChatRouteContent({ // 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( + const sanitizedMessages = useMemo( () => sanitizeDisplayMessages(messages), [messages], ); + // Mirror into a ref so `useChatDebugApi`'s `tail()` can read what the + // UI is actually rendering without re-running the pipeline. + sanitizedMessagesRef.current = sanitizedMessages; + const transcriptItems = useMemo( () => buildTranscriptItems({ - messages: sortedMessages, + messages: sanitizedMessages, pendingSecret: pendingSecret ? { requestId: pendingSecret.requestId } : null, @@ -812,7 +824,7 @@ export function ChatRouteContent({ showOnboardingChoice, }), [ - sortedMessages, + sanitizedMessages, pendingSecret, pendingConfirmation, inlineConfirmationAttached, @@ -1325,7 +1337,7 @@ export function ChatRouteContent({ ); diff --git a/apps/web/src/domains/chat/transcript/build-items.ts b/apps/web/src/domains/chat/transcript/build-items.ts index cd82a28ddb4..ce494cb6eca 100644 --- a/apps/web/src/domains/chat/transcript/build-items.ts +++ b/apps/web/src/domains/chat/transcript/build-items.ts @@ -53,9 +53,6 @@ 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`. */ export function buildTranscriptItems( input: BuildTranscriptItemsInput, diff --git a/apps/web/src/domains/chat/utils/debug-api.test.ts b/apps/web/src/domains/chat/utils/debug-api.test.ts index 36ba07c96fb..4794e019608 100644 --- a/apps/web/src/domains/chat/utils/debug-api.test.ts +++ b/apps/web/src/domains/chat/utils/debug-api.test.ts @@ -98,6 +98,7 @@ function makeRefs( }; return { messagesRef: { current: [] } as MutableRefObject, + sanitizedMessagesRef: { current: [] } as MutableRefObject, transcriptRef: { current: null as TranscriptHandle | null }, streamContextRef: { current: null } as MutableRefObject<{ assistantId: string; @@ -125,7 +126,7 @@ function makeRefs( // --------------------------------------------------------------------------- describe("createChatDebugApi.tail", () => { - test("empty messagesRef → empty tail", () => { + test("empty sanitizedMessagesRef → empty tail", () => { const api = createChatDebugApi(makeRefs()); const result = api.tail(); expect(result).toEqual([]); @@ -133,10 +134,10 @@ describe("createChatDebugApi.tail", () => { test("returns the underlying DisplayMessage objects untouched", () => { const message = fakeDisplayMessage({ content: "hello world" }); - const messagesRef = { + const sanitizedMessagesRef = { current: [message], } as MutableRefObject; - const api = createChatDebugApi(makeRefs({ messagesRef })); + const api = createChatDebugApi(makeRefs({ sanitizedMessagesRef })); const result = api.tail(); expect(result).toHaveLength(1); // Identity check — debug API must NOT project to a bespoke shape. @@ -147,8 +148,10 @@ describe("createChatDebugApi.tail", () => { const items: DisplayMessage[] = Array.from({ length: 30 }, (_, i) => fakeDisplayMessage({ stableId: `msg-${i}`, id: `id-${i}` }), ); - const messagesRef = { current: items } as MutableRefObject; - const api = createChatDebugApi(makeRefs({ messagesRef })); + const sanitizedMessagesRef = { + current: items, + } as MutableRefObject; + const api = createChatDebugApi(makeRefs({ sanitizedMessagesRef })); const result = api.tail(5); expect(result).toHaveLength(5); expect(result[0]!.stableId).toBe("msg-25"); @@ -159,8 +162,10 @@ describe("createChatDebugApi.tail", () => { const items: DisplayMessage[] = Array.from({ length: 30 }, (_, i) => fakeDisplayMessage({ stableId: `msg-${i}`, id: `id-${i}` }), ); - const messagesRef = { current: items } as MutableRefObject; - const api = createChatDebugApi(makeRefs({ messagesRef })); + const sanitizedMessagesRef = { + current: items, + } as MutableRefObject; + const api = createChatDebugApi(makeRefs({ sanitizedMessagesRef })); const result = api.tail(); expect(result).toHaveLength(20); expect(result[0]!.stableId).toBe("msg-10"); @@ -170,8 +175,10 @@ describe("createChatDebugApi.tail", () => { const items: DisplayMessage[] = Array.from({ length: 5 }, (_, i) => fakeDisplayMessage({ stableId: `msg-${i}`, id: `id-${i}` }), ); - const messagesRef = { current: items } as MutableRefObject; - const api = createChatDebugApi(makeRefs({ messagesRef })); + const sanitizedMessagesRef = { + current: items, + } as MutableRefObject; + const api = createChatDebugApi(makeRefs({ sanitizedMessagesRef })); const result = api.tail(20); expect(result).toHaveLength(5); expect(result[0]!.stableId).toBe("msg-0"); @@ -181,12 +188,33 @@ describe("createChatDebugApi.tail", () => { const items: DisplayMessage[] = Array.from({ length: 30 }, (_, i) => fakeDisplayMessage({ stableId: `msg-${i}`, id: `id-${i}` }), ); - const messagesRef = { current: items } as MutableRefObject; - const api = createChatDebugApi(makeRefs({ messagesRef })); + const sanitizedMessagesRef = { + current: items, + } as MutableRefObject; + const api = createChatDebugApi(makeRefs({ sanitizedMessagesRef })); expect(api.tail(-1)).toHaveLength(20); expect(api.tail(NaN)).toHaveLength(20); expect(api.tail(Infinity)).toHaveLength(20); }); + + test("reads from sanitizedMessagesRef, NOT raw messagesRef", () => { + // tail() is logic-free — it surfaces whatever the render path + // already wrote to `sanitizedMessagesRef`. Raw `messagesRef` is + // intentionally ignored so DevTools always mirrors the UI. + const rawOnly = fakeDisplayMessage({ stableId: "raw-only" }); + const sanitizedOnly = fakeDisplayMessage({ stableId: "sanitized-only" }); + const api = createChatDebugApi( + makeRefs({ + messagesRef: { current: [rawOnly] } as MutableRefObject, + sanitizedMessagesRef: { + current: [sanitizedOnly], + } as MutableRefObject, + }), + ); + const result = api.tail(); + expect(result).toHaveLength(1); + expect(result[0]!.stableId).toBe("sanitized-only"); + }); }); // --------------------------------------------------------------------------- diff --git a/apps/web/src/domains/chat/utils/debug-api.ts b/apps/web/src/domains/chat/utils/debug-api.ts index 16fb585d89e..bb8b317b2c3 100644 --- a/apps/web/src/domains/chat/utils/debug-api.ts +++ b/apps/web/src/domains/chat/utils/debug-api.ts @@ -39,7 +39,6 @@ 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, @@ -301,6 +300,13 @@ const CHAT_NS = "chat"; */ export interface ChatDebugRefs { messagesRef: MutableRefObject; + /** + * Post-`sanitizeDisplayMessages` snapshot — exactly what the transcript + * renders. Populated by `chat-route-content.tsx` right after the render + * boundary `useMemo`. `tail()` reads from this so DevTools mirrors the + * UI without re-running the sanitize pipeline. + */ + sanitizedMessagesRef: MutableRefObject; /** * Ref to the mounted `` imperative handle. Used by * {@link ChatDebugApi.getScrollState} to read scroll geometry directly @@ -380,13 +386,9 @@ export function createChatDebugApi(refs: ChatDebugRefs): ChatDebugApi { Number.isFinite(limit) && limit > 0 ? Math.floor(limit) : DEFAULT_TAIL_LIMIT; - // 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 ?? []); + // Read straight from the post-sanitization snapshot the render path + // already wrote. `tail()` is now logic-free — same array the UI iterates. + const messages = refs.sanitizedMessagesRef.current ?? []; const startIndex = Math.max(0, messages.length - safeLimit); return messages.slice(startIndex); } @@ -618,7 +620,7 @@ export function createChatDebugApi(refs: ChatDebugRefs): ChatDebugApi { const lines = [ "window._vellumDebug.chat — surgical chat debug API", "", - " .tail(n?) last N DisplayMessage[] from messagesRef — raw UI shape", + " .tail(n?) last N DisplayMessage[] the UI is rendering", " .thinkingIndicator() live evaluation of the `...` predicate + done signal", " .visible / .failingConditions tell you why dots are or aren't showing", " .done.terminal / .done.lastTerminalReason tell you if the turn is finished", @@ -729,6 +731,7 @@ export function useChatDebugApi(refs: ChatDebugRefs): void { useEffect(() => { const stableRefs: ChatDebugRefs = { messagesRef: refs.messagesRef, + sanitizedMessagesRef: refs.sanitizedMessagesRef, transcriptRef: refs.transcriptRef, streamContextRef: refs.streamContextRef, streamRef: refs.streamRef, diff --git a/apps/web/src/domains/chat/utils/sanitize-display-messages.ts b/apps/web/src/domains/chat/utils/sanitize-display-messages.ts index 8733ba0dac7..5a7d3f66393 100644 --- a/apps/web/src/domains/chat/utils/sanitize-display-messages.ts +++ b/apps/web/src/domains/chat/utils/sanitize-display-messages.ts @@ -1,7 +1,7 @@ // ----------------------------------------------------------------------------- // sanitizeDisplayMessages — single home for "this shouldn't be necessary, // but is" frontend cleanup applied to DisplayMessage[] before the transcript -// renders (and before `window._vellumDebug.chat.tail()` returns). +// renders. // // Every sub-method below patches over an upstream issue. They are SHORT TERM // and should be removed as the assistant backend stabilises the corresponding @@ -13,25 +13,15 @@ import { sortedByTimestamp } from "@/domains/chat/utils/message-sorting.js"; import type { DisplayMessage } from "@/domains/chat/types/types.js"; -/** - * Apply every render-boundary hack to `messages` in a fixed order and return - * a new array (never mutates the input). - * - * Pipeline: - * 1. `sortByTimestamp` (Hack #1) - * 2. `removeInvalidMessages` (Hack #2) - * 3. `removeDuplicateTrailingAssistant` (Hack #3) - * - * Each step is independent — removing any one when the corresponding upstream - * bug is fixed is a one-line edit. - */ export function sanitizeDisplayMessages( messages: DisplayMessage[], ): DisplayMessage[] { - let result = sortByTimestamp(messages); - result = removeInvalidMessages(result); - result = removeDuplicateTrailingAssistant(result); - return result; + const pipeline = [ + sortedByTimestamp, + removeInvalidMessages, + removeDuplicateTrailingAssistant, + ]; + return pipeline.reduce((msgs, step) => step(msgs), messages); } // ----------------------------------------------------------------------------- @@ -53,9 +43,7 @@ export function sanitizeDisplayMessages( // SHORT TERM until: the assistant backend merges multi-row clusters // server-side so the client never sees the fragmented rows. // ----------------------------------------------------------------------------- -function sortByTimestamp(messages: DisplayMessage[]): DisplayMessage[] { - return sortedByTimestamp(messages); -} +// (Implementation: `sortedByTimestamp`, imported at the top.) // ----------------------------------------------------------------------------- // Hack #2 — drop blank / phantom user rows @@ -66,12 +54,12 @@ function sortByTimestamp(messages: DisplayMessage[]): DisplayMessage[] { // user rows even when their parent `tool_use` lives on a previous page // (to avoid permanent data loss). The daemon's renderer then drops the // orphan `tool_result` block, leaving a blank user bubble on the wire. -// - The daemon synthesises tool calls with `toolName === "unknown"` when a +// - The assistant synthesises tool calls with `toolName === "unknown"` when a // `tool_result` has no matching `tool_use`. Those arrive as empty user // messages whose only payload is a list of "unknown" tools and would -// otherwise render as a confusing "Used unknown" chip (ATL-659). +// otherwise render as a confusing "Used unknown" chip. // -// SHORT TERM until: the daemon stops emitting orphan `tool_result` rows and +// SHORT TERM until: the assistant stops emitting orphan `tool_result` rows and // phantom unknown-tool placeholders at history boundaries. // ----------------------------------------------------------------------------- function removeInvalidMessages(messages: DisplayMessage[]): DisplayMessage[] {