fix(web): apply PR #32009 review feedback #32010
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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`. | ||
| */ | ||
| export function buildTranscriptItems( | ||
| input: BuildTranscriptItemsInput, | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -98,6 +98,7 @@ function makeRefs( | |
| }; | ||
| return { | ||
| messagesRef: { current: [] } as MutableRefObject<DisplayMessage[]>, | ||
| sanitizedMessagesRef: { current: [] } as MutableRefObject<DisplayMessage[]>, | ||
| transcriptRef: { current: null as TranscriptHandle | null }, | ||
| streamContextRef: { current: null } as MutableRefObject<{ | ||
| assistantId: string; | ||
|
|
@@ -125,18 +126,18 @@ function makeRefs( | |
| // --------------------------------------------------------------------------- | ||
|
|
||
| describe("createChatDebugApi.tail", () => { | ||
| test("empty sanitizedMessagesRef → empty tail", () => { | ||
| const api = createChatDebugApi(makeRefs()); | ||
| const result = api.tail(); | ||
| expect(result).toEqual([]); | ||
| }); | ||
|
|
||
| test("returns the underlying DisplayMessage objects untouched", () => { | ||
| const message = fakeDisplayMessage({ content: "hello world" }); | ||
| const sanitizedMessagesRef = { | ||
| current: [message], | ||
| } as MutableRefObject<DisplayMessage[]>; | ||
| 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 sanitizedMessagesRef = { | ||
| current: items, | ||
| } as MutableRefObject<DisplayMessage[]>; | ||
| 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 sanitizedMessagesRef = { | ||
| current: items, | ||
| } as MutableRefObject<DisplayMessage[]>; | ||
| 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 sanitizedMessagesRef = { | ||
| current: items, | ||
| } as MutableRefObject<DisplayMessage[]>; | ||
| 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 sanitizedMessagesRef = { | ||
| current: items, | ||
| } as MutableRefObject<DisplayMessage[]>; | ||
| 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<DisplayMessage[]>, | ||
| sanitizedMessagesRef: { | ||
| current: [sanitizedOnly], | ||
| } as MutableRefObject<DisplayMessage[]>, | ||
| }), | ||
| ); | ||
| const result = api.tail(); | ||
| expect(result).toHaveLength(1); | ||
| expect(result[0]!.stableId).toBe("sanitized-only"); | ||
| }); | ||
| }); | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 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<DisplayMessage[]>; | ||
| /** | ||
| * 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<DisplayMessage[]>; | ||
| /** | ||
| * Ref to the mounted `<Transcript />` 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; | ||
| // 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[] 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, | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| // ----------------------------------------------------------------------------- | ||
| // sanitizeDisplayMessages — single home for "this shouldn't be necessary, | ||
| // but is" frontend cleanup applied to DisplayMessage[] before the transcript | ||
| // 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"; | ||
|
|
||
| export function sanitizeDisplayMessages( | ||
| messages: DisplayMessage[], | ||
| ): DisplayMessage[] { | ||
| 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. | ||
| // ----------------------------------------------------------------------------- | ||
| // (Implementation: `sortedByTimestamp`, imported at the top.) | ||
|
Comment on lines
-56
to
+46
Contributor
There was a problem hiding this comment. Ah ok we are using this in reconcileMessages, which we plan to cleanup, simplify, and remove in another convo. for now, let's add back the wrapper method we had here until we are ready to delete reconcileMessages |
||
|
|
||
| // ----------------------------------------------------------------------------- | ||
| // 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 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. | ||
| // | ||
| // SHORT TERM until: the assistant stops emitting orphan `tool_result` rows and | ||
| // phantom unknown-tool placeholders at history boundaries. | ||
| // ----------------------------------------------------------------------------- | ||
| function removeInvalidMessages(messages: DisplayMessage[]): DisplayMessage[] { | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Do we need this ref anymore?