🪜 feat: Trace Viewer Steps, Sequence Scale and Previews - #16009
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fcb424606
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (part.type === ContentTypes.TOOL_CALL) { | ||
| if (current == null) { | ||
| current = { text: '', toolCalls: [] }; | ||
| steps.push(current); | ||
| } |
There was a problem hiding this comment.
When consecutive model rounds emit tool calls without intervening text, every TOOL_CALL is appended to the same preview because only a later TEXT part starts another step. This occurs when a model calls one tool, receives its result, and immediately calls another; previewFor indexes previews by trace step, so the second step receives no arguments while the first step receives both. Use the persisted run-step identity (such as tool_call.stepId) or another round boundary to separate sequential tool-only rounds while retaining parallel calls in one step.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5e04128. buildStepPreviews now starts a new round when a tool call carries a different run-step id than the previous one (parallel calls of one round share runStep.id, so they stay together) and when reasoning follows a tool call. Covered by preview.test.ts: "separates consecutive tool-only rounds by run step and keeps parallel calls together" and "starts the next round at reasoning that follows a tool call"; both fail with the boundary removed.
| const ancestor = nearestAncestor(nodes, node, (candidate) => isSimpleRecord(candidate.record)); | ||
| if (ancestor != null) { | ||
| continue; |
There was a problem hiding this comment.
When a generation is nested beneath another shown record—for example, a subagent model call beneath the tool observation that invoked it—this ancestor check excludes the generation from stepRoots. Since groupSteps creates steps only from these roots, that model call inherits the outer record's step instead of starting its own, undercounting turn.steps and associating its previews with the wrong step. Nested generations need to remain step boundaries even when they have a model/tool ancestor.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Half fixed in 5e04128, half rejected. Fixed: previews now apply only to a step's own roots (step.rootIds), so a nested model call or nested tool never borrows the enclosing step's text or arguments; toolOrdinal counts roots only. Covered by "leaves what ran inside a tool, such as a subagent model call, without a preview", which fails with the guard removed. Rejected: a generation nested under a tool is that tool's work (a subagent's rounds are the subagent's steps, not the response's), so it stays under the tool and does not start a step of the response. turn.steps deliberately counts the response's own model calls.
| if ('name' in call && typeof call.name === 'string') { | ||
| return { name: call.name, args: argsPreview(call.args) }; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
For persisted Code Interpreter calls, TMessageContentParts supplies code_interpreter.input but neither function nor name, so this falls through to null and the corresponding tool row never shows the available input preview. The same dispatch also drops retrieval/file-search shapes outright. Add explicit handling for the non-function variants, including a stable tool name and the input when that shape carries one.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Rejected. CodeToolCall, RetrievalToolCall and FileSearchToolCall are the Assistants API shapes; only agent runs are exported to Langfuse, and their code execution is persisted as an agent ToolCall (name: 'execute_code', args), which this handles. An Assistants shape carries no tool name to match a trace record with, and the Assistants endpoint is being sunset (#15916), so there is nothing to preview for it.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e041288a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| useEffect(() => { | ||
| setSelectedId((id) => (id != null && !model.nodes.has(id) ? null : id)); | ||
| setView((current) => (current != null ? fitWindow(current, model) : current)); | ||
| setView((current) => (current != null ? fitWindow(current, bounds, minSpan) : current)); |
There was a problem hiding this comment.
When the default sequence scale has an active zoom/selection and the user loads an older page, the newly loaded turns are numbered before the existing turns, but fitWindow preserves the old numeric endpoints. The same window therefore begins showing different, older records even though the selection remains visible. Reset the sequence window when records are inserted before it, or anchor the window to record identities and rebase it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 37dcb21. A window now follows the records it covered: rebaseWindow maps the sequence window's first and last records by id into the new model and clears it when either is gone (time scale keeps the fit). Covered by model.test.ts "carries a sequence window across a page that renumbers the records" and Viewer.test.tsx "keeps a focused record range on the same records when an older page renumbers them"; the viewer test fails with the rebase replaced by a plain fit.
| if (current == null || (isGeneration && current.some(isGenerationId(nodes)))) { | ||
| groups.push([id]); | ||
| } else { | ||
| current.push(id); |
There was a problem hiding this comment.
When a turn has a tool root before its first generation—explicitly supported here and possible while a turn is split across trace pages—this branch places both records in step 1. For message content shaped as tool call followed by reasoning/text, buildStepPreviews starts a second preview after the tool, so the generation reads the empty text from preview 1 and the actual answer in preview 2 is unused; later previews can likewise shift. Either give the leading tool its own step or align preview indices with this grouping rule.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 37dcb21. Tools before the first model call now form their own step, which is the same boundary buildStepPreviews draws for a tool-only round, so a turn split across pages keeps its previews aligned. Covered by "gives tools that ran before any model call their own step, and a turn with no model call one step", which fails with the old grouping.
| const ordinal = toolOrdinal(node, model, step.rootIds); | ||
| const match = preview.toolCalls.filter((call) => call.name === record.name)[ordinal]; |
There was a problem hiding this comment.
For parallel calls to the same tool that start within the same parsed millisecond, step.rootIds is ordered by observation ID after Date.parse collapses their timestamps, while preview.toolCalls remains in message-content order. Using the resulting ordinal can therefore attach each call's arguments to the other tool row. Match on a persisted tool-call identity, or suppress the preview when same-name calls cannot be ordered reliably.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 37dcb21. Same-name root tools of one step that share a start millisecond get no preview rather than a possibly swapped one (the list read carries no tool-call id to match on). Covered by "gives no preview to same-name calls that started in the same millisecond", which fails with the check removed.
| const ancestor = nearestAncestor(nodes, node, (candidate) => isSimpleRecord(candidate.record)); | ||
| if (ancestor != null) { | ||
| continue; |
There was a problem hiding this comment.
When an otherwise hidden agent or chain wrapper has status: 'error', isSimpleRecord makes it a qualifying ancestor here, so every generation beneath it is excluded from stepRoots; the failed wrapper becomes one generation-less step and multiple model calls collapse into that single step with no previews. Fresh evidence beyond the earlier nested-generation case is that changing only the wrapper's status to error changes the step count for the same trace structure. Error rows should remain visible without becoming step-boundary ancestors.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 37dcb21. Step roots and the simple-mode parent are now anchored only by model calls and tools (isStepAnchor); a failed wrapper stays visible as a row in the first step but anchors nothing, so the model calls beneath it keep their own steps. Covered by "keeps a failed wrapper visible without folding the model calls beneath it into one step", which fails with the error kind restored as an anchor.
| if (steps.length === 0 && message?.text) { | ||
| steps.push({ text: compact(message.text), toolCalls: [] }); |
There was a problem hiding this comment.
For a restored message that stores its answer only in top-level message.text, this fallback always creates preview index 0. If its trace has multiple generation steps, previewFor therefore displays the final answer beside the first generation and leaves the actual final generation without a preview. Preserve compatibility with these stored messages by associating the flat-text fallback with the last response generation rather than the first.
AGENTS.md reference: AGENTS.md:L31-L33
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 37dcb21. A message stored with only its final text is marked fromText, and that text previews the last model call of the response, none of the earlier ones. Covered by "attaches a message stored as flat text to the last model call", which fails when attached to the first.
| /** A refresh or a settled run trims the cache to its newest page. An interval or a selection | ||
| * on records that left with the older pages would hide every row, or reopen when they reload. */ | ||
| useEffect(() => { | ||
| setSelectedId((id) => (id != null && !model.nodes.has(id) ? null : id)); |
There was a problem hiding this comment.
When a user selects a generic span in All spans mode and then switches back to simple mode, the record remains in model.nodes with shown: false, so this check preserves selectedId and leaves an inspector open for a row that no longer exists in the ledger. Clear the selection when the selected node is absent or not shown in the active mode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 37dcb21. The selection now clears when the selected record is absent or not listed in the active mode (shown), so switching back from All spans closes an inspector on a hidden span. Covered by "clears a selection the simple mode no longer lists", which fails with the old nodes.has check.
…pages, modes and failed wrappers
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37dcb2151d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const last = record.kind === 'generation' && turn != null && step.index === turn.steps; | ||
| return last ? message.steps[0]?.text || undefined : undefined; | ||
| } | ||
| const preview = message.steps[step.index - 1]; |
There was a problem hiding this comment.
When readFrom reaches maxRecords, its continuation can split a response's observations, so the newest page may contain only a later generation while the cached chat message still contains every model round. That loaded generation is renumbered as step 1 here and receives message.steps[0], displaying the first round's text or tool arguments beside a later model call until older records load. The leading-tool fix does not cover an omitted whole generation; suppress or offset previews for a partial boundary turn until its earlier steps are known.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 99a5af9. While another page remains, the oldest loaded turn is the one a page boundary can split, so its rows get no previews until that page loads; every other turn's rounds are complete and number correctly. Covered by Viewer.test.tsx "withholds previews for a response split across pages until its earlier steps load", which fails with the suppression removed.
| if (steps.length === 0 && message?.text) { | ||
| return { steps: [{ text: compact(message.text), toolCalls: [] }], fromText: true }; |
There was a problem hiding this comment.
For an agent run that fails after its Langfuse run is created, api/server/controllers/agents/request.js persists an assistant row with error: true, text: errorText, and langfuseRunId pointing at that run. This fallback therefore treats a UI error such as “Generation failed” as model output and attaches it to the trace's last generation, even though the model never wrote it; skip failed/error-only messages before applying the flat-text fallback.
AGENTS.md reference: AGENTS.md:L31-L33
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 99a5af9. buildMessagePreview skips the flat-text fallback when message.error is set, so a failed turn's error text never previews a model call; content parts of such a row are still read. Covered by preview.test.ts "never treats a failed turn's error text as model output", which fails with the fallback restored.
| groups.forEach((rootIds, index) => { | ||
| const key = stepKey(turn.messageId, origin, index + 1); |
There was a problem hiding this comment.
When loading an older trace page inserts an earlier generation into a response that was split across pages, every existing step's ordinal key shifts. Because Viewer retains its collapsed set, a collapsed step:...:1 then folds the newly inserted step while the step the user actually collapsed moves to key 2 and opens. Derive the persistence key from the generation/root record identity, keeping the ordinal only as the displayed step number.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 99a5af9. Step keys are now step:<message>:<origin>:<anchor record id> (the step's generation, or its first root), with the ordinal kept only for the displayed number, so a fold survives an older page inserting an earlier model call. Covered by model.test.ts "keeps a step key stable when an older page adds an earlier model call", which fails with ordinal keys.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99a5af943b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| for (const [origin, roots] of rootsByOrigin) { | ||
| roots.sort(compare); |
There was a problem hiding this comment.
When a generation and its following tool start within the same millisecond, the reader's Date(...).toISOString() normalization gives them identical times, so this sort falls back to their opaque observation IDs. If the tool ID sorts first, groupSteps treats it as a leading tool-only step and puts the generation in the next step, inflating the step count and shifting message previews to the wrong records. Use causal information or a deterministic generation-before-following-tool tie-break instead of observation ID order.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 62915e5. Ties at one millisecond are now broken causally (wrapper, then model call, then tool, then event) before falling back to the id, in the one comparator that orders roots, siblings and the sequence. Covered by model.test.ts "orders a model call before the tool it asked for when both start in the same millisecond", which fails with the kind tie-break removed.
| if (steps.length === 0 && message?.text && message.error !== true) { | ||
| return { steps: [{ text: compact(message.text), toolCalls: [] }], fromText: true }; | ||
| } | ||
| return { steps, fromText: false }; |
There was a problem hiding this comment.
When an agent uses hide_sequential_outputs and has multiple sequential generations without a retained tool-call boundary, applyHideSequentialOutputsFilter removes the intermediate content before persistence and leaves only the final TEXT part. Fresh evidence beyond the earlier flat-text case is that this branch returns that retained content with fromText: false, so previewFor treats it as steps[0] and displays the final answer beside the first generation rather than the last. Mark final-only filtered content as final-aligned, or suppress the preview when its step cannot be recovered.
AGENTS.md reference: AGENTS.md:L31-L33
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 62915e5. finalOnly now covers both shapes: flat text and content reduced to a single text-only round, which hide_sequential_outputs leaves behind. Either previews the last model call only. Covered by "attaches a message that kept only its final text to the last model call" (both cases), which fails with the detection removed.
| /** Hangs every shown record from its nearest shown ancestor; the full mode shows them all. */ | ||
| function resolveViewTree(nodes: Map<string, TraceNode>, mode: TraceMode): void { | ||
| for (const node of nodes.values()) { | ||
| node.shown = mode === 'full' || isSimpleRecord(node.record); |
There was a problem hiding this comment.
When a title run is represented only by a successful CHAIN observation—the shape normalized as obs-title in packages/api/src/langfuse/reader.spec.ts—isSimpleRecord returns false here, so the default mode hides the record and stepRoots never creates the advertised Title row. Switching to All spans does not restore the label because full mode omits step headers, leaving the backend's new origin: 'title' marker without a visible effect for this supported shape. Include title-origin roots in the title group even when their kind is a generic span.
AGENTS.md reference: AGENTS.md:L31-L33
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 62915e5. Records carrying origin: 'title' are listed in the simple mode whatever their kind, so a title run recorded as a plain chain span gets its Title step. Covered by "shows a title run recorded as a plain span as its own Title step", which fails with the origin rule removed.
| </span> | ||
| <Icon aria-hidden="true" className="size-3.5 shrink-0 text-text-secondary" /> | ||
| <span className="min-w-[3ch] truncate">{record.name}</span> | ||
| <span className="min-w-[3ch] shrink-0 truncate">{record.name}</span> |
There was a problem hiding this comment.
When a record has a name wider than the first grid column, such as a long MCP tool name, shrink-0 prevents this flex item from becoming narrower than its intrinsic content, so truncate cannot constrain it. The name then collapses the newly added preview to zero width and can overflow into the duration column; remove shrink-0 or give the name a bounded width so both the name and preview remain readable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 62915e5. The name no longer refuses to shrink: it is bounded to 60% of the label column and truncates, and the preview keeps a minimum width, so a long MCP tool name and its preview both stay readable. Layout only, so no jsdom test; verified by reading the flex rules.
…, and truncating long names
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62915e538d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return (left?.start ?? 0) - (right?.start ?? 0) || a.localeCompare(b); | ||
| return ( | ||
| (left?.start ?? 0) - (right?.start ?? 0) || | ||
| KIND_ORDER[left?.record.kind ?? 'span'] - KIND_ORDER[right?.record.kind ?? 'span'] || |
There was a problem hiding this comment.
When a tool from one round and the following model call start within the same persisted millisecond, this unconditional kind ordering moves the generation before the tool. For roots ordered as generation1, tool, generation2, the model becomes generation1, generation2, tool, so groupSteps assigns the tool to the second step and previewFor shifts its arguments away from the model call that requested it. The tie-break needs round/call identity or another causal signal that distinguishes a tool following the current generation from one preceding the next generation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Rejected. In the agent loop a tool starts only after the model call that requested it has ended, and the next model call starts only after that tool has ended, so a tool and the following model call can share a start millisecond only if the tool ran for zero milliseconds. A model call and the tool it requested sharing a millisecond is the only tie the loop can produce, and generation-before-tool is the causal order for it. The list read carries no call identity to do better, and origin/stepId are not on observations; a zero-duration tool colliding with the next model call is not a case worth a second matching mechanism.
| return preview.text || undefined; | ||
| } | ||
| /** Same-name calls that started in the same millisecond have no reliable order to match by. */ | ||
| const ambiguous = step.rootIds.some((id) => { |
There was a problem hiding this comment.
When a completed step contains many root tool calls and the user types in the trace search field, flattenRows evaluates labelsFor and therefore previewFor for every record. Each tool preview scans the entire step.rootIds here, scans it again in toolOrdinal, and filters the full preview call list, making search quadratic in the step size; at the supported 10,000-record limit, a large parallel-tool step can perform hundreds of millions of comparisons per search update and freeze the viewer. Build a per-step lookup once, or otherwise avoid rescanning the step for each row.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5b87449. Previews are now built once per model and message set by buildPreviewIndex, one pass over each step's roots (calls grouped by name, ordinals and same-millisecond twins counted as it goes), and rows and search read the resulting map. Nothing rescans a step per row. Existing preview coverage runs through the index; preview.test.ts also pins the page-boundary rule through it.
| continue; | ||
| } | ||
| const step = current == null || afterToolCall ? begin() : current; | ||
| step.text = compact(`${step.text} ${textOf(part.text)}`); |
There was a problem hiding this comment.
When a multi-agent response stores adjacent TEXT parts from different agentId values—either a sequential handoff or agents sharing a parallel groupId—this loop concatenates them into one preview because it only starts a new round after a tool call. The trace records those agents as separate generation roots and therefore separate steps, but finalOnly then attaches the combined output only to the last generation, leaving earlier generations blank and attributing their text to the wrong agent. Split sequential agent outputs using their metadata and suppress or explicitly correlate previews for parallel lanes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5b87449. A text or tool-call part whose agentId differs from the current round's starts a new round (a handoff), and any part carrying a groupId marks the message parallel, for which no previews are shown, since lanes cannot be told apart by order. Covered by preview.test.ts "splits a handoff to another agent into its own round and withholds parallel lanes", which fails with either rule removed.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b87449a65
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const part of parts) { | ||
| if (part.type === ContentTypes.TOOL_CALL) { |
There was a problem hiding this comment.
When the trace is opened during an active streamed response, the cached message.content array can contain holes because client/src/hooks/SSE/useContentHandler.ts writes parts directly at provider-supplied indexes. Iterating a sparse array yields undefined, so the immediate part.type access throws and crashes the Trace Viewer until the message is compacted or another update fills the slot. Skip nullish parts before dispatching on their type.
AGENTS.md reference: AGENTS.md:L31-L33
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e10cd24. buildMessagePreview skips nullish parts before dispatching on their type, since useContentHandler writes parts at provider indexes and a streaming message can hold holes. Covered by preview.test.ts "skips the holes a streaming message leaves in its content", which throws without the guard.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e10cd24f54
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (part.type !== ContentTypes.TEXT) { | ||
| continue; |
There was a problem hiding this comment.
When an agent automatically compacts context and then performs a tool-using round, useStepHandler.ts persists a ContentTypes.SUMMARY part for the summarizer model call, but this branch discards it without reserving a step. The Langfuse trace still contains that summarizer generation, so subsequent tool/text previews are indexed one step early: tool arguments can appear beside the summarizer and the final generation can receive the preceding round's text. Represent each successful summary as an empty preview round, or correlate the remaining parts to trace steps by run-step identity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 708b0e5. A SUMMARY part now opens a sealed, empty round, so the summarizer's model call keeps its place in the numbering and the rounds after it stay aligned; the summary itself previews nothing. It reserves a round for every summary part, failed ones included, since the summarizer was still called. Covered by preview.test.ts "reserves a round for a compaction summary, which is a model call of its own", which fails with the round removed.
| if (kind === 'generation' || (kind === 'tool' && current == null)) { | ||
| groups.push([...leading, id]); | ||
| leading = []; | ||
| } else if (current == null) { | ||
| leading.push(id); | ||
| } else { | ||
| current.push(id); |
There was a problem hiding this comment.
For a parallel multi-agent turn whose flattened roots are ordered as generation A, generation B, tool A, tool B, each generation starts a new group and both tools are consequently assigned to B's step. This shape occurs when each lane's generation and tool are siblings beneath separate hidden agent wrappers, so simple mode reports A as having no tools while B owns both, producing incorrect step headers, durations, and collapse grouping even though parallel previews are withheld. Preserve the lane/ancestor relationship when grouping concurrent roots instead of assigning every later tool to the last globally ordered generation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 708b0e5. A root's lane is the wrapper it sits under one level below the turn's structural root; a tool now joins the latest step in its own lane and falls back to the latest step only when no step shares its lane, which leaves single-agent turns exactly as before. Covered by model.test.ts "keeps a tool with the model call of its own lane when agents run in parallel", which fails with lane targeting removed.
…ools with their own model call
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 708b0e5186
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (afterToolCall) { | ||
| begin(); | ||
| } | ||
| continue; |
There was a problem hiding this comment.
When a sequential handoff's new agent starts with a THINK part after the previous agent's tool call, this branch opens a round because afterToolCall is true but never calls handoff(part), leaving roundAgent set to the old agent. The following text from the new agent is then interpreted as another handoff and opens an extra round, so the new generation receives an empty preview while its text is left in an unused later slot. Fresh evidence beyond the earlier adjacent-text handoff is that useStepHandler.ts stamps agentId and groupId onto THINK parts via getStepMetadata; process that metadata here as is already done for text and tool parts.
AGENTS.md reference: AGENTS.md:L31-L33
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2031e4f. The reasoning branch now records the handoff before deciding, so an agent that takes over by thinking first opens exactly one round and its text lands in it. Covered by preview.test.ts "recognizes a handoff whose new agent starts by reasoning", which fails with the handoff call removed from that branch.
| const own = [...groups].reverse().find((group) => group.lane === lane); | ||
| (own ?? current).rootIds.push(id); |
There was a problem hiding this comment.
For a long agent turn containing many sequential generations and tools, every tool reaching this branch clones and reverses the entire accumulated groups array before examining it. This makes groupSteps quadratic even for the common single-lane pattern, and the viewer rebuilds this model whenever records or display mode change; with traceViewer.maxRecords allowing 10,000 records per page and older pages accumulating, the repeated array allocations can noticeably stall or freeze the viewer. Track the latest group per lane in a Map (or at least check current.lane first) so grouping remains linear.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2031e4f. The latest group per lane is kept in a map updated when a group is created, so a tool's lookup is constant and groupSteps stays linear in the roots; behaviour is unchanged and the existing lane test still passes.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2031e4f30f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if (current == null) { | ||
| leading.push(id); | ||
| } else { | ||
| (latestByLane.get(lane) ?? current).rootIds.push(id); |
There was a problem hiding this comment.
When pagination leaves a lane's generation on the older page, but another parallel lane already has a group, a leading tool from the incomplete lane reaches this fallback and is appended to current, which belongs to the other lane. For loaded roots such as generation A, tool B with generation B still beyond the page boundary, the viewer therefore attributes tool B to A and changes the step's duration, tool summary, and collapse grouping until older records load. Fresh evidence beyond the earlier parallel-lane case is the ?? current fallback specifically when that lane has no loaded generation; retain a per-lane leading group instead.
AGENTS.md reference: AGENTS.md:L31-L33
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 29d3657. A tool whose lane has no model call loaded yet now leads a step of its own lane instead of joining another lane's step, and the lane key was tightened so records directly under the turn's root, or with no parent, share one lane (single-agent turns group exactly as before). Covered by model.test.ts "keeps a tool whose lane has no loaded model call in a step of its own lane", which fails with the fallback restored.
| if (steps.length === 0 && message?.text && message.error !== true) { | ||
| return { steps: [{ text: compact(message.text), toolCalls: [] }], finalOnly: true, parallel }; | ||
| } | ||
| const finalOnly = steps.length === 1 && steps[0].toolCalls.length === 0 && steps[0].text !== ''; |
There was a problem hiding this comment.
When a multi-generation run fails or is cancelled after a later model call starts, its persisted content can contain one earlier TEXT part followed by an ERROR part or an unfinished marker. Because non-text parts are ignored and this condition does not check either terminal state, that earlier text is marked finalOnly; buildPreviewIndex then attaches it to the last, failed generation instead of the generation that produced it. Fresh evidence beyond the earlier flat error-text fix is that structured content still takes this branch, so failed or unfinished messages need ordinal alignment or suppressed final alignment.
AGENTS.md reference: AGENTS.md:L31-L33
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 29d3657. A message with error, unfinished, or an ERROR part is never marked final-only, so its surviving text stays with the model call that wrote it (ordinal alignment) rather than the last, failed one. Covered by preview.test.ts "never marks the surviving text of a failed or unfinished run as final", which fails with the check removed.
| const roots = visible ? turn.rootIds.filter((id) => visible.has(id)) : turn.rootIds; | ||
| return visible && roots.length === 0 ? [] : [{ turn, roots }]; | ||
| const groups = groupsOf(turn); | ||
| return visible && groups.length === 0 ? [] : [{ turn, groups }]; |
There was a problem hiding this comment.
When a running or early-cancelled trace contains only successful/running agent or chain wrappers, simple mode hides every record and produces no stepKeys, but this condition retains the turn whenever no search/window filter is active. Since Viewer checks the raw model.nodes.size, it then renders an expandable “0 steps” tree item with no children instead of an appropriate empty/running state, and keyboard or pointer expansion only toggles invisible state. Drop turns with no groups in simple mode, make them non-expandable, or surface the wrapper needed to explain the run.
AGENTS.md reference: AGENTS.md:L31-L33
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 29d3657. A turn with no model call, tool or failure yet lists its structural roots (the running wrapper) in the simple mode, so the ledger shows what is running instead of an empty expandable row; once work arrives the wrappers drop out again. Covered by model.test.ts "lists the wrappers of a turn that has no model call, tool or failure yet", which fails with the fallback removed.
…o final alignment for runs that ended early
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29d3657598
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| const callsByName = new Map<string, string[]>(); | ||
| for (const call of round.toolCalls) { | ||
| callsByName.set(call.name, [...(callsByName.get(call.name) ?? []), call.args]); |
There was a problem hiding this comment.
When a step contains many calls to the same tool, this spread copies the entire growing bucket for every call, making preview indexing O(n²); at the supported 10,000-record scale, one large parallel-tool step copies roughly 50 million array entries and can stall the viewer before rendering or search begins. Fresh evidence after the prior indexing fix is that constructing the index itself still performs these repeated copies; mutate a newly created per-name array or append to the existing one instead. CLAUDE.mdL165-L170
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e452759. The per-name bucket is created once and appended to, so building the index is linear in a step's calls. Behaviour unchanged; the existing preview coverage runs through the index.
| const compare = byStart(nodes); | ||
| const rootsByOrigin = new Map<TraceStep['origin'], string[]>(); | ||
| for (const id of rootIds) { | ||
| const origin = nodes.get(id)?.record.origin ?? 'run'; |
There was a problem hiding this comment.
During a rolling or mixed-version deployment, the new frontend can receive trace records from a pre-change backend that never emits origin; this fallback then classifies every title-trace observation as response-run work. For a traced response with an automatic title generation, the viewer reports an extra numbered response step instead of a Title step, and a finalOnly answer can be attached to that title generation because it is now the turn's last run step. Gate the new grouping on an advertised backend capability or otherwise preserve the old-record distinction before defaulting missing origins to run. CLAUDE.mdL130-L132
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Rejected. The origin marker is emitted by the same deployment's server, so a frontend without it only exists during a rollout window. In that window the title generation is listed as a numbered step, which is exactly how every record was listed before this change, and a final-only preview may sit one row off until the backend catches up. That is a transient cosmetic degradation, not data loss or a wrong action, and it does not justify a capability handshake on the trace contract; the feature is also unreleased, so no shipped frontend meets an older backend.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e452759ffd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Please review the current PR head a6f37fc. Confirm that this exact commit is the reviewed commit and ignore findings that apply only to earlier heads. I addressed all four latest findings together: canonical parallel-lane detection, steer boundaries, unloaded-parent lane identity, and suppression of ambiguous ordinal previews when round counts differ. I also checked reasoning/summary/steer transitions and unfinished flat-text handling. The Trace and lane suites pass (125 tests), using a temporary local source mapping for data-provider because the shared package builds are stale. ESLint, Prettier, and diff checks pass. Full client typechecking was attempted but local dependency/build mismatches prevent a clean result; no diagnostics in the four changed files with the source mapping. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6f37fcc21
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Please review the current PR head 2ed874e. Confirm that this exact commit is the reviewed commit and ignore findings that apply only to earlier heads. I cached nearest step ancestors across both projections, cached lane branches, and prevented overlapping full-mode roots from recounting the same subtree. Both modes have a 3,000-node reversed-input failure-cascade regression with a deterministic bound on record inspection and exact record counts. All 128 Trace/lane tests pass; ESLint, Prettier, and diff checks pass. Local full-client tsc still hits dependency/build mismatches; source-mapped tsc reports no diagnostics in the changed files. The previous head passed CI client typechecking. The nested unloaded-wrapper finding is rejected: with only |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
The trace viewer added in #15869 lists every Langfuse observation as recorded, so a user reads
AgentGraph,chain,tool-dispatchandcheckpointbefore reaching a model call, a turn with four model calls and six tools is one flat tree, and the overview draws real durations, where a 40 ms tool call is invisible beside a 30 s generation. This change makes the ledger read as what the response did.By default the ledger now shows only model calls and tools (plus anything that failed), each hung from its nearest shown ancestor, and groups a response into steps: each model call starts one and the tools that ran after it belong to it. A step header reads
Step 2 · 12.3s · web_search×2, a folded response reads3 steps · 5 tool calls, and responses older than the newest arrive folded. Rows carry a one-line preview beside the name: the text the step wrote, or the arguments the tool was called with. The overview defaults to a sequence scale, one equal block per record in three lanes (model calls, tools, other), so a quick call stays visible; the previous time scale is one toggle away, as is the full span tree.The previews come from the chat's own messages, which the viewer already has in the query cache underneath it. They are the assistant text and tool-call arguments the user already sees in the chat, so they need no new Langfuse read and stay outside the
showInputOutputgate, which still guards system prompts and full input. Records of the turn's title run are labelledTitlerather than counted as a step; the reader now marks them withorigin: 'title', which it always knew from the trace id.How it works
The projection, as the model builds it for one response:
A window (
{ start, end }) is now a span on the active scale: record positions on the sequence scale, epoch milliseconds on the time scale.clampWindowwidens a window narrower than the minimum around its own centre, so zooming at the minimum no longer drifts right.Change Type
Testing
Focused suites, all passing locally:
client:Chat/Trace/__tests__/{model,preview,Viewer,Surface,format}(85). New coverage: the simple-mode roll-up and re-parenting, a failed span staying visible, step grouping (leading tools, no-model-call turn, title run apart), first-call tool-name order (verified to fail with the ordering fix removed), sequence numbering and record-range focus, full-mode rows, minimum-span zoom, the All spans and Duration toggles, previews from the messages cache with no messages request, and older responses folding as they load.packages/api:src/langfuse/reader.spec.ts(60) andsrc/traces/handlers.spec.tsfor theoriginmarker.npx tsc --noEmitpasses forpackages/api, and forclientreports only the three pre-existingcomposerSubmitClasseserrors from the local@librechat/clientbuild, none in the Trace files. Prettier, ESLint andsort-imports --checkpass on every changed file.Not run:
api/server/routes/__tests__/traces.spec.js, which loads the built@librechat/apibundle rather than the source under change.Current-head verification for the preview alignment fixes:
git diff --checkon the four changed files.npx tsc --noEmitinclient; local dependency/build mismatches block a clean result. A second pass mapped data-provider to current source and reported no diagnostics in the four changed files. CI passed the full client typecheck at a6f37fc; the latest head is checked independently.Test Configuration:
Plus Langfuse tracing configured, as for #15869. The
All spansandDurationtoggles persist per browser.Checklist