mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-09-01 03:27:01 +00:00
⏱️ feat: Show Run-Step Durations On Tool Cards (#14892)
* ⏱️ feat: Show Run-Step Durations On Tool Cards Surfaces how long each tool call took, derived from the `closed_at` / `created_at` pair already carried by `on_run_step_closed` — the same event #14871 and #14873 use for the terminal status. No new event, no new SDK surface. The duration is stamped onto the content part at the same three sites as `runStepStatus`, so it survives a reload and a resumable reconnect rather than living only on the live React message: - `callbacks.js`, on the aggregated part before the event is forwarded - `RedisJobStore`, in the host-authored replay reconstruction branch - `useStepHandler`, on the live message Rendering lands in the shared `ProgressText`, which nine tool cards already use, rather than in each card: one place decides whether a duration is shown and how it reads, and the cards only forward the number. That keeps this from adding a tenth independent state derivation to a component family whose label/announcement/progress split is already the subject of AI-1810. The value is deliberately absent rather than zero whenever it would be a guess — no `created_at`, non-finite input, or a negative elapsed time from two clocks that disagree, which is now reachable because a step can be opened in one process and closed in another after a checkpoint resume. Sub-second durations are suppressed as noise, and it renders only on a settled, non-error card, where the slot is not already carrying the cancelled icon or the error suffix. For assistive technology the compact form (`3.5s`) is hidden and paired with a spoken equivalent ("took 3.5 seconds"), both inside the button, so the accessible name carries the duration without an `aria-live` region re-announcing it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * 🎨 style: Sort Imports In Touched Files The import-sort gate runs against the files a PR changes, so pre-existing drift in `ProgressText.tsx` and `RedisJobStore.ts` surfaced on this branch. Both were already unsorted on `dev`; this is the sorter's output, with no semantic change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * 🐛 fix: Accept Partial Timestamps In Run-Step Duration Helper `getReportableRunStepDurationMs` declared its parameter as `Pick<RunStepClosedEvent, 'created_at' | 'closed_at'>`, where `closed_at` is required. That contradicted the function's own purpose: every guard inside it exists precisely to handle stamps that may be missing. The Redis replay branch reconstructs closures from persisted JSON and holds nothing stronger than "might be a number", so it failed to typecheck against the narrower signature. Widened to an exported `RunStepTimestamps` shape with both stamps optional, rather than asserting at the call site — an assertion would move the decision about what is trustworthy somewhere it cannot be enforced, which is the thing the helper exists to centralize. Callers holding a fully-typed event still pass, since a required field satisfies an optional one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * 🐛 fix: Suppress Duration When Failure Arrives As errorSuffix Alone At every call site `error` carries cancellation while failure travels through `errorSuffix` with `error` false, so gating the duration on `!error` alone rendered "· 3.5s" beside "· failed" — and announced it. The gate now checks both terminal-failure channels. The original test pinned only the `error: true` path, which is why this survived; the failed-via-suffix path is now pinned separately, both the visible and the announced half. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * 🧩 refactor: Persist Raw Run-Step Durations, Threshold At Render Only The three stamp sites filtered through the 1-second reportability threshold before persisting, baking a presentation rule into stored data: a 900ms step stored nothing, making "fast" indistinguishable from "not derivable" and unrecoverable if the display rule ever changes. Stamp sites now persist the raw `getRunStepDurationMs` value — absent only when genuinely not derivable — and the renderer alone decides what is worth showing, which `ProgressText` already did. Rendering is unchanged. `getReportableRunStepDurationMs` is removed; it existed only to serve the write-time filter, and a test now pins that sub-threshold durations survive to storage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * 🐛 fix: Suppress Duration On Backgrounded Bash And Code Cards A backgrounded call's run step closes when dispatch returns the handle, so the stamped duration is the dispatch time. Rendering it beside "Running/Finished in background" misstated a detached task's runtime as seconds — and violated the "settled card only" rule, since the card is still tracking the detached run. Scope is exactly the two cards that parse background handles. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * 🌍 fix: Format The Sub-10s Decimal For The Active Locale The fractional seconds value was interpolated as a raw JS number, which hardcodes the en-US decimal point into every language — "1.4s" where the locale writes "1,4 s" — and translators cannot fix a number formatted in code. The value is now formatted via Intl.NumberFormat with i18n.language, following MessageTimestamp's pattern of threading the language into the util; plural-key selection stays on the numeric value. A malformed language tag falls back to the plain number. Also documents the two accepted limits of the derivation, so they read as decisions rather than oversights: positive clock skew is undetectable from a single stamp pair, and the value is wall-clock elapsed, so a step held open across a suspension (checkpoint resume, HITL approval wait) includes that time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * 🐛 fix: Persist A Durable `backgrounded` Marker Through Harvest; Localize Minute Digits Codex round 3, both findings confirmed. **Background origin survived only as transient state.** The dispatch handle in `tool_call.output` and the live status-marker attachment are both gone once the harvester patches the settled task's stdout over the handle — so the round-2 suppression (`backgroundHandle == null`) came back on after harvest or reload, showing dispatch time as the task's runtime. Following the same rule as e4bd15d (persist facts, decide at render): the harvest patch now stamps `backgrounded: true` onto the tool call in the same atomic write that erases the handle — on the heal path too, which re-applies over full-row saves that reverted the part. The cards gate on handle-or-marker; the dispatch duration itself stays stored. **Minute-branch digits bypassed locale formatting.** The seconds branch went through Intl.NumberFormat while minutes interpolated raw numbers, so Arabic/Persian locales flipped to ASCII digits above one minute. All interpolated values now flow through the (renamed) formatDurationValue; an ar-EG test pins the localized digits. data-schemas cannot be installed in this environment (same npm ci 403 as packages/api), so message.ts/harvest.ts are syntax-checked with resolution off and otherwise verified by review; CI runs their real typecheck and suites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * 🧪 test: Assert The `markBackgrounded` Stamp In Harvest Expectations The successful-harvest test's exact `toHaveBeenCalledWith` object did not include the newly forwarded `markBackgrounded`, so the API suite would fail on it. All three harvest-call expectations now assert `markBackgrounded: true` — the exact-object one of necessity, the two `objectContaining` ones deliberately, since the durable stamp (on the best-effort file-failure path and the reapply heal alike) is now part of the behavior under test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 * 🎨 style: Wrap Harvest Spec Expectation Per Prettier Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014vLhxCFMYkCaTsoFTiAjJ5 --------- Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
parent
d79d1ff76a
commit
fb8ae881cf
24 changed files with 625 additions and 4 deletions
|
|
@ -64,6 +64,7 @@ describe('createBackgroundCodeResultHandler', () => {
|
|||
agentId: 'agent_a',
|
||||
output: 'stdout:\nhello',
|
||||
attachments: [{ file_id: 'f1', filename: 'plot.png', toolCallId: 'call_code' }],
|
||||
markBackgrounded: true,
|
||||
});
|
||||
expect(result).toEqual({
|
||||
attachments: [{ file_id: 'f1', filename: 'plot.png', toolCallId: 'call_code' }],
|
||||
|
|
@ -156,7 +157,11 @@ describe('createBackgroundCodeResultHandler', () => {
|
|||
const result = await handler(baseParams);
|
||||
|
||||
expect(updateToolCallResult).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ output: 'stdout:\nhello', attachments: [] }),
|
||||
expect.objectContaining({
|
||||
output: 'stdout:\nhello',
|
||||
attachments: [],
|
||||
markBackgrounded: true,
|
||||
}),
|
||||
);
|
||||
expect(result).toEqual({ attachments: [] });
|
||||
});
|
||||
|
|
@ -180,6 +185,9 @@ describe('createBackgroundCodeResultHandler', () => {
|
|||
toolCallId: 'call_code',
|
||||
output: 'stdout:\nhello',
|
||||
attachments: [{ file_id: 'f1' }],
|
||||
/** The heal path must re-stamp the marker: the full-row save it
|
||||
* repairs reverted the whole patched part, marker included. */
|
||||
markBackgrounded: true,
|
||||
}),
|
||||
);
|
||||
expect(result).toEqual({ attachments: [{ file_id: 'f1' }] });
|
||||
|
|
|
|||
|
|
@ -8,6 +8,7 @@ const {
|
|||
FileContext,
|
||||
ErrorTypes,
|
||||
UsageEvents,
|
||||
getRunStepDurationMs,
|
||||
} = require('librechat-data-provider');
|
||||
const {
|
||||
GraphEvents,
|
||||
|
|
@ -460,6 +461,17 @@ function getDefaultHandlers({
|
|||
const part = typeof index === 'number' ? contentParts[index] : undefined;
|
||||
if (part?.type === ContentTypes.TOOL_CALL && part.tool_call) {
|
||||
part.tool_call.runStepStatus = data.status;
|
||||
/**
|
||||
* The raw derivable duration, left unset rather than zeroed when
|
||||
* the event cannot support a trustworthy one — no `created_at`,
|
||||
* or clocks that disagree. Whether it is *worth showing* is the
|
||||
* renderer's call; persisting the fact unfiltered keeps that
|
||||
* threshold adjustable without data loss.
|
||||
*/
|
||||
const durationMs = getRunStepDurationMs(data);
|
||||
if (durationMs != null) {
|
||||
part.tool_call.runStepDurationMs = durationMs;
|
||||
}
|
||||
}
|
||||
}
|
||||
await emitForJob({ event, data });
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue