fix(web): apply PR #32009 review feedback by vellum-apollo-bot[bot] · Pull Request #32010 · vellum-ai/vellum-assistant

vellum-apollo-bot · 2026-05-25T15:42:39Z

Follow-up to #32009. Addresses each piece of review feedback and applies the suggested refactor pattern.

Closes review threads on #32009:

`sanitize-display-messages.ts`

Header (line 4) — dropped the parenthetical about window._vellumDebug.chat.tail(), since tail() no longer touches the pipeline directly (see debug-api.ts below).
sanitizeDisplayMessages body (line 23) — adopted the pipeline.reduce pattern Vargas suggested. The JSDoc that enumerated the three steps was indeed redundant with the implementation, so the JSDoc is gone and the array literal is now the spec.
sortByTimestamp wrapper (line 57) — deleted. The imported sortedByTimestamp from message-sorting.ts is the pipeline step directly. Added a one-line (Implementation: sortedByTimestamp, imported at the top.) under the Hack feat: initialize Next.js app in /web directory #1 header so the docblock still says "here's the named step."
removeInvalidMessages docblock (lines 72, 75) — switched "daemon" → "assistant" for the runtime that synthesises unknown tool calls, dropped the inline ticket reference.

`build-items.ts`

line 58 — removed the comment claiming messages must already be sanitized. buildTranscriptItems takes whatever it's given; sanitization isn't its contract.

`debug-api.ts` — the meatier one

Vargas: "This method is doing too much logic, and it should be just reading from the same ref that eventually is looped over the react components."

Done. The flow is now:

messagesRef (raw)
    │
    └─► chat-route-content useMemo(sanitizeDisplayMessages)
            │
            ├─► sanitizedMessagesRef.current = …    ← new
            ├─► buildTranscriptItems(sanitizedMessages)
            └─► <Transcript messages={sanitizedMessages} />

sanitizedMessagesRef is created in chat-page.tsx next to messagesRef, threaded down through ChatRouteRefs, populated by chat-route-content.tsx right after the render-boundary useMemo, and passed into useChatDebugApi.
tail() is now a one-liner that slices sanitizedMessagesRef.current. The sanitizeDisplayMessages import is gone from debug-api.ts entirely.
Renamed sortedMessages → sanitizedMessages in chat-route-content.tsx to match what the variable actually holds (it's not "just sorted" anymore).

Tests

debug-api.test.ts — every tail() test now populates sanitizedMessagesRef instead of messagesRef to reflect the new contract. Added one new contract test (reads from sanitizedMessagesRef, NOT raw messagesRef) so the two refs can't be silently swapped back later.
Existing sanitize-display-messages.test.ts (24 tests) is untouched — pipeline.reduce is wire-compatible with the old let result = …; result = … chain.

Local checks

bun test on touched files: 170 passing, 0 failing.
bun run lint on touched files: clean.
bun run audit:cross-domain:check: clean.
bunx tsc --noEmit: identical pre-existing errors as main (all in settings/ai + Assistant type, unrelated to this PR).

Codex P1 on `line 181`

Acknowledged Vargas's reply on the merged PR — the position-for-position toolName+result matcher is intentional. No change here.

Broader design question

Discussed separately in chat. tl;dr: DisplayMessage[] is the logical conversation model; TranscriptItem[] is the virtualized render list (a discriminated union of 10 kinds, only one of which is message). They're not interchangeable. The duplication we just eliminated wasn't between them — it was between two independent computations of "sanitized DisplayMessage[]" at the render boundary and inside tail(). With this PR there's now a single computation, and a ref bridges it to the debug API.

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.

dvargasfuertes · 2026-05-25T16:07:18Z

+  /**
+   * 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.
+   */


dvargasfuertes · 2026-05-25T16:07:30Z


+  // Mirror into a ref so `useChatDebugApi`'s `tail()` can read what the
+  // UI is actually rendering without re-running the pipeline.


Suggested change

// Mirror into a ref so `useChatDebugApi`'s `tail()` can read what the

// UI is actually rendering without re-running the pipeline.

dvargasfuertes · 2026-05-25T16:07:50Z

+  // UI is actually rendering without re-running the pipeline.
+  sanitizedMessagesRef.current = sanitizedMessages;
+
  const transcriptItems = useMemo(


introduce another ref to track transcript items

dvargasfuertes · 2026-05-25T16:09:48Z

+  // 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.


dvargasfuertes · 2026-05-25T16:09:57Z

  const messagesRef = useRef<DisplayMessage[]>(messages);
  messagesRef.current = messages;


Do we need this ref anymore?

dvargasfuertes · 2026-05-25T16:11:20Z

-function sortByTimestamp(messages: DisplayMessage[]): DisplayMessage[] {
-  return sortedByTimestamp(messages);
-}
+// (Implementation: `sortedByTimestamp`, imported at the top.)


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

vellum-apollo-bot · 2026-05-25T16:27:21Z

Follow-up shipped as #32013 — Option C from the chat thread.

This PR's feedback → resolution:

chat-route-content.tsx:226 Delete → ✅
chat-route-content.tsx:797 (Mirror-into-a-ref) → ✅
chat-route-content.tsx:800 introduce another ref → ✅ transcriptItemsRef added to ChatRouteRefs, populated right after buildTranscriptItems
chat-page.tsx:254 Delete → ✅
chat-page.tsx:250 Do we need messagesRef anymore? → kept. 5 stream/send/secondary-action hooks read it for raw (pre-sanitize) messages (use-send-message, use-stream-event-handler, use-conversation-secondary-actions, interaction-handlers, surface-handlers). Migrating those is its own follow-up; the contract pin "debug API reads sanitized, not raw" still holds.
sanitize-display-messages.ts:46 add back the wrapper → ✅ local sortByTimestamp reinstated until reconcile.ts cleanup

Plus from chat (Option C):

Rename tail() → getClientMessages() ✅
Add getTranscriptItems() ✅
Migrated the one in-tree consumer (share-feedback-modal.tsx) to the new method names — its triage tar now carries both clientMessages + transcriptItems.

CI on #32013.

…tTranscriptItems() (#32013) Follow-up to PR #32010 review feedback. Vargas chose Option C from the DisplayMessage vs TranscriptItem design conversation: keep both arrays visible side-by-side via the debug API before deciding which transcript items should be promoted to messages or whether wrapper kinds can be dropped. Changes ------- - Rename `chat.tail(n?)` → `chat.getClientMessages(n?)`. Same shape (`DisplayMessage[]`), same default limit, same source ref. The new name disambiguates "what the client thinks the message list is" from the server-side `serverMessages()` view. - Add `chat.getTranscriptItems(): TranscriptItem[]`. The full virtualized row list — messages plus the thinking indicator, pending prompts, queued marker, error notices, and the onboarding-choice row. Logic-free reader of a new `transcriptItemsRef` populated by `chat-route-content.tsx` right after `buildTranscriptItems`. - Migrate the one in-tree consumer: `share-feedback-modal.tsx` now attaches `clientMessages` + `transcriptItems` to the triage tar. Review-feedback resolutions on PR #32010 ---------------------------------------- - chat-route-content.tsx:226 — deleted `sanitizedMessagesRef` JSDoc. - chat-route-content.tsx:797 — deleted "Mirror into a ref so …" comment. - chat-route-content.tsx:800 — added `transcriptItemsRef` to `ChatRouteRefs`, populated in the same spot as `sanitizedMessagesRef`. - chat-page.tsx:254 — deleted the JSDoc above `sanitizedMessagesRef`. - chat-page.tsx:250 ("Do we need messagesRef anymore?") — kept. `messagesRef` is consumed by 5+ stream/send/secondary-action hooks (`use-send-message`, `use-stream-event-handler`, `use-conversation-secondary-actions`, `interaction-handlers`, `surface-handlers`) and a contract-pin test asserts the debug API reads sanitized, not raw. Migrating those consumers is its own follow-up. - sanitize-display-messages.ts:46 — restored the local `sortByTimestamp` wrapper. `reconcileMessages` still imports `sortedByTimestamp` directly; the wrapper keeps this file's pipeline self-contained until the reconcile cleanup lands. Tests ----- - 64 tests pass across `debug-api.test.ts` + `sanitize-display-messages.test.ts`. - New `createChatDebugApi.getTranscriptItems` describe block covers: empty case, reference-identity (`expect(result).toBe(items)`), full discriminated-union surface, and a contract pin asserting the impl reads from `transcriptItemsRef` (NOT derived from `sanitizedMessagesRef`). - Renamed contract pin: `"reads from sanitizedMessagesRef, NOT raw messagesRef"` still applies to `getClientMessages`. - Help-text test now asserts both `.getClientMessages(` and `.getTranscriptItems(` appear. Verification ------------ - `bun test`: 64/64 pass. - `bun run lint`: clean on all 6 touched files. - `bun run audit:cross-domain:check`: clean. - `bun run typecheck`: 10 errors, identical set to `origin/main` baseline (settings/ai, platform_actor_token, provisioned_storage_gib — all in untouched domains). Refs: #32010, #32009 Co-authored-by: vellum-apollo-bot[bot] <220448527+vellum-apollo-bot[bot]@users.noreply.github.com>

vellum-apollo-bot Bot mentioned this pull request May 25, 2026

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

Merged

dvargasfuertes approved these changes May 25, 2026

View reviewed changes

dvargasfuertes merged commit 0dbec30 into main May 25, 2026
7 checks passed

dvargasfuertes deleted the apollo/sanitize-display-followups branch May 25, 2026 16:11

vellum-apollo-bot Bot mentioned this pull request May 25, 2026

feat(web): debug API — rename tail() → getClientMessages() and add getTranscriptItems() #32013

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(web): apply PR #32009 review feedback#32010

fix(web): apply PR #32009 review feedback#32010
dvargasfuertes merged 1 commit into
mainfrom
apollo/sanitize-display-followups

vellum-apollo-bot Bot commented May 25, 2026

Uh oh!

dvargasfuertes May 25, 2026

Uh oh!

dvargasfuertes May 25, 2026

Uh oh!

dvargasfuertes May 25, 2026

Uh oh!

dvargasfuertes May 25, 2026

Uh oh!

dvargasfuertes May 25, 2026

Uh oh!

dvargasfuertes May 25, 2026

Uh oh!

Uh oh!

vellum-apollo-bot Bot commented May 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant


		// Mirror into a ref so `useChatDebugApi`'s `tail()` can read what the
		// UI is actually rendering without re-running the pipeline.

		const messagesRef = useRef<DisplayMessage[]>(messages);
		messagesRef.current = messages;

Conversation

vellum-apollo-bot Bot commented May 25, 2026

sanitize-display-messages.ts

build-items.ts

debug-api.ts — the meatier one

Tests

Local checks

Codex P1 on line 181

Broader design question

Uh oh!

dvargasfuertes May 25, 2026

Choose a reason for hiding this comment

Uh oh!

dvargasfuertes May 25, 2026

Choose a reason for hiding this comment

Uh oh!

dvargasfuertes May 25, 2026

Choose a reason for hiding this comment

Uh oh!

dvargasfuertes May 25, 2026

Choose a reason for hiding this comment

Uh oh!

dvargasfuertes May 25, 2026

Choose a reason for hiding this comment

Uh oh!

dvargasfuertes May 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vellum-apollo-bot Bot commented May 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

`sanitize-display-messages.ts`

`build-items.ts`

`debug-api.ts` — the meatier one

Codex P1 on `line 181`