Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 56 additions & 5 deletions packages/opencode/src/cli/cmd/tui/routes/session/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
For,
Match,
on,
onCleanup,
onMount,
Show,
Switch,
Expand All @@ -33,6 +34,7 @@ import { Prompt, type PromptRef } from "@tui/component/prompt"
import type { AssistantMessage, Part, ToolPart, UserMessage, TextPart, ReasoningPart } from "@opencode-ai/sdk/v2"
import { useLocal } from "@tui/context/local"
import { Locale } from "@/util/locale"
import { SLOW_TOOL_MS, formatElapsed, type Elapsed } from "./timing"
import type { Tool } from "@/tool/tool"
import type { ReadTool } from "@/tool/read"
import type { WriteTool } from "@/tool/write"
Expand Down Expand Up @@ -152,7 +154,7 @@ export function Session() {
const [sidebarOpen, setSidebarOpen] = createSignal(false)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH · code-reviewer] The default value for 'timestamps' was changed from 'hide' to 'show'. While the PR claims this is intentional and user preferences persist, the CLAUDE.md in the project root (if present) may explicitly require backward-compatible defaults for UI settings unless explicitly approved by product. This change alters user experience without a documented product decision.

💡 Suggestion: Verify that a product decision or CLAUDE.md rule explicitly permits flipping the default from hide to show. If not, revert to 'hide' and document the change in a follow-up PR with product sign-off.

Confidence: 70/100

const [conceal, setConceal] = createSignal(true)
const [showThinking, setShowThinking] = kv.signal("thinking_visibility", true)
const [timestamps, setTimestamps] = kv.signal<"hide" | "show">("timestamps", "hide")
const [timestamps, setTimestamps] = kv.signal<"hide" | "show">("timestamps", "show")
const [showDetails, setShowDetails] = kv.signal("tool_details_visibility", true)
const [showAssistantMetadata, setShowAssistantMetadata] = kv.signal("assistant_metadata_visibility", true)
const [showScrollbar, setShowScrollbar] = kv.signal("scrollbar_visible", true)
Expand Down Expand Up @@ -615,12 +617,12 @@ export function Session() {
},
},
{
title: showTimestamps() ? "Hide timestamps" : "Show timestamps",
title: showTimestamps() ? "Hide tool timing" : "Show tool timing",
value: "session.toggle.timestamps",
category: "Session",
slash: {
name: "timestamps",
aliases: ["toggle-timestamps"],
aliases: ["toggle-timestamps", "timing", "toggle-timing"],
},
onSelect: (dialog) => {
setTimestamps((prev) => (prev === "show" ? "hide" : "show"))
Expand Down Expand Up @@ -1690,6 +1692,25 @@ function ToolTitle(props: { fallback: string; when: any; icon: string; children:
)
}

// Elapsed time (ms) for a tool part. Returns undefined for "pending" (no start yet),
// ticks at 1Hz while "running", freezes to end-start on "completed"/"error".
function useElapsed(part: () => ToolPart | undefined) {
const [now, setNow] = createSignal(Date.now())
createEffect(() => {
if (part()?.state.status !== "running") return
const id = setInterval(() => setNow(Date.now()), 1000)
onCleanup(() => clearInterval(id))
})
return createMemo<Elapsed | undefined>(() => {
const p = part()
if (!p) return undefined
const s = p.state
if (s.status === "pending") return undefined
if (s.status === "running") return { start: s.time.start, ms: now() - s.time.start, running: true }
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Clamp elapsed to zero to prevent negative duration on first render. The now signal is seeded at mount time but s.time.start may be newer if the tool starts later, yielding a negative ms until the next interval tick. Use Math.max(0, now() - s.time.start).

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/cli/cmd/tui/routes/session/index.tsx, line 1709:

<comment>Clamp elapsed to zero to prevent negative duration on first render. The `now` signal is seeded at mount time but `s.time.start` may be newer if the tool starts later, yielding a negative `ms` until the next interval tick. Use `Math.max(0, now() - s.time.start)`.</comment>

<file context>
@@ -1690,6 +1692,25 @@ function ToolTitle(props: { fallback: string; when: any; icon: string; children:
+    if (!p) return undefined
+    const s = p.state
+    if (s.status === "pending") return undefined
+    if (s.status === "running") return { start: s.time.start, ms: now() - s.time.start, running: true }
+    return { start: s.time.start, ms: s.time.end - s.time.start, running: false }
+  })
</file context>
Suggested change
if (s.status === "running") return { start: s.time.start, ms: now() - s.time.start, running: true }
if (s.status === "running") return { start: s.time.start, ms: Math.max(0, now() - s.time.start), running: true }

return { start: s.time.start, ms: s.time.end - s.time.start, running: false }
Comment on lines +1698 to +1710
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Prevent negative elapsed values on pending→running transition.

now may be older than s.time.start right after a tool starts, so the first rendered duration can go negative until the first 1s tick.

Suggested fix
 function useElapsed(part: () => ToolPart | undefined) {
   const [now, setNow] = createSignal(Date.now())
   createEffect(() => {
     if (part()?.state.status !== "running") return
+    setNow(Date.now())
     const id = setInterval(() => setNow(Date.now()), 1000)
     onCleanup(() => clearInterval(id))
   })
   return createMemo<Elapsed | undefined>(() => {
     const p = part()
     if (!p) return undefined
     const s = p.state
     if (s.status === "pending") return undefined
-    if (s.status === "running") return { start: s.time.start, ms: now() - s.time.start, running: true }
-    return { start: s.time.start, ms: s.time.end - s.time.start, running: false }
+    if (s.status === "running") return { start: s.time.start, ms: Math.max(0, now() - s.time.start), running: true }
+    return { start: s.time.start, ms: Math.max(0, s.time.end - s.time.start), running: false }
   })
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/opencode/src/cli/cmd/tui/routes/session/index.tsx` around lines 1698
- 1710, The elapsed can be negative on the pending→running transition because
now() may be older than s.time.start; fix by ensuring computed ms is never
negative: in the createMemo for Elapsed (the block using part(), s.time.start
and now()), clamp ms to Math.max(0, now() - s.time.start) for the running case
(or alternatively update the now signal when a part's status changes to
"running" inside the createEffect to setNow(Date.now())); modify the logic in
the createMemo/createEffect that references now, part(), and s.time.start so the
first rendered duration is non-negative.

})
}

function InlineTool(props: {
icon: string
iconColor?: RGBA
Expand Down Expand Up @@ -1729,6 +1750,14 @@ function InlineTool(props: {
error()?.includes("user dismissed"),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW · code-reviewer] The useElapsed hook calls clearInterval(id) on cleanup, but does not check if id is defined before clearing. If the effect runs and immediately exits (e.g., if part() is undefined), id may be undefined, leading to a potential runtime warning or error in some environments.

💡 Suggestion: Wrap clearInterval(id) in a conditional: if (id) clearInterval(id).

Confidence: 85/100

)

const elapsed = useElapsed(() => props.part)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: InlineTool spinner branch omits elapsed timing display

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/cli/cmd/tui/routes/session/index.tsx, line 1753:

<comment>InlineTool spinner branch omits elapsed timing display</comment>

<file context>
@@ -1729,6 +1750,14 @@ function InlineTool(props: {
       error()?.includes("user dismissed"),
   )
 
+  const elapsed = useElapsed(() => props.part)
+  const timingVisible = createMemo(() => ctx.showTimestamps() && !!elapsed())
+  const timingColor = createMemo(() => {
</file context>

const timingVisible = createMemo(() => ctx.showTimestamps() && !!elapsed())
const timingColor = createMemo(() => {
const e = elapsed()
if (e?.running && e.ms >= SLOW_TOOL_MS) return theme.warning
return theme.textMuted
})

return (
<box
marginTop={margin()}
Expand Down Expand Up @@ -1771,6 +1800,12 @@ function InlineTool(props: {
<Show fallback={<>~ {props.pending}</>} when={props.complete}>
<span style={{ fg: props.iconColor }}>{props.icon}</span> {props.children}
</Show>
<Show when={timingVisible()}>
<span style={{ fg: timingColor() }}>
{" · "}
{formatElapsed(elapsed()!)}
</span>
</Show>
</text>
</Match>
</Switch>
Expand All @@ -1788,10 +1823,18 @@ function BlockTool(props: {
part?: ToolPart
spinner?: boolean
}) {
const ctx = use()
const { theme } = useTheme()
const renderer = useRenderer()
const [hover, setHover] = createSignal(false)
const error = createMemo(() => (props.part?.state.status === "error" ? props.part.state.error : undefined))
const elapsed = useElapsed(() => props.part)
const timingVisible = createMemo(() => ctx.showTimestamps() && !!elapsed())
const timingColor = createMemo(() => {
const e = elapsed()
if (e?.running && e.ms >= SLOW_TOOL_MS) return theme.warning
return theme.textMuted
})
return (
<box
border={["left"]}
Expand All @@ -1814,11 +1857,19 @@ function BlockTool(props: {
when={props.spinner}
fallback={
<text paddingLeft={3} fg={theme.textMuted}>
{props.title}
<span>{props.title}</span>
<Show when={timingVisible()}>
<span style={{ fg: timingColor() }}>
{" "}
{formatElapsed(elapsed()!)}
</span>
</Show>
</text>
}
>
<Spinner color={theme.textMuted}>{props.title.replace(/^# /, "")}</Spinner>
<Spinner color={theme.textMuted}>
{props.title.replace(/^# /, "") + (timingVisible() ? ` ${formatElapsed(elapsed()!)}` : "")}
</Spinner>
</Show>
{props.children}
<Show when={error()}>
Expand Down
44 changes: 42 additions & 2 deletions packages/opencode/src/cli/cmd/tui/routes/session/sidebar.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { useSync } from "@tui/context/sync"
import { createMemo, For, Show, Switch, Match } from "solid-js"
import { createEffect, createMemo, createSignal, For, onCleanup, Show, Switch, Match } from "solid-js"
import { createStore } from "solid-js/store"
import { useTheme } from "../../context/theme"
import { Locale } from "@/util/locale"
import path from "path"
import type { AssistantMessage } from "@opencode-ai/sdk/v2"
import type { AssistantMessage, ToolPart } from "@opencode-ai/sdk/v2"
import { Global } from "@/global"
import { Installation } from "@/installation"
import { useKeybind } from "../../context/keybind"
Expand Down Expand Up @@ -62,6 +62,39 @@ export function Sidebar(props: { sessionID: string; overlay?: boolean }) {
}
})

// altimate_change start - total tool time
// Flatten every tool part in this session so we can sum execution time across
// completed/error tools and live-tick any tool that's still running.
const toolParts = createMemo<ToolPart[]>(() => {
const out: ToolPart[] = []
for (const message of messages()) {
const parts = sync.data.part[message.id]
if (!parts) continue
for (const part of parts) {
if (part.type === "tool") out.push(part)
}
}
return out
})
const hasRunningTool = createMemo(() => toolParts().some((p) => p.state.status === "running"))
const [nowToolTime, setNowToolTime] = createSignal(Date.now())
createEffect(() => {
if (!hasRunningTool()) return
const id = setInterval(() => setNowToolTime(Date.now()), 1000)
onCleanup(() => clearInterval(id))
})
const toolTime = createMemo(() => {
let ms = 0
const tick = nowToolTime()
for (const part of toolParts()) {
const s = part.state
if (s.status === "running") ms += tick - s.time.start
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Clamp to zero to prevent transient negative tool-time contribution. nowToolTime() is seeded at sidebar mount; if a tool starts later, tick - s.time.start can be negative until the interval fires. Use Math.max(0, tick - s.time.start).

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/cli/cmd/tui/routes/session/sidebar.tsx, line 91:

<comment>Clamp to zero to prevent transient negative tool-time contribution. `nowToolTime()` is seeded at sidebar mount; if a tool starts later, `tick - s.time.start` can be negative until the interval fires. Use `Math.max(0, tick - s.time.start)`.</comment>

<file context>
@@ -62,6 +62,39 @@ export function Sidebar(props: { sessionID: string; overlay?: boolean }) {
+    const tick = nowToolTime()
+    for (const part of toolParts()) {
+      const s = part.state
+      if (s.status === "running") ms += tick - s.time.start
+      else if (s.status === "completed" || s.status === "error") ms += s.time.end - s.time.start
+    }
</file context>
Suggested change
if (s.status === "running") ms += tick - s.time.start
if (s.status === "running") ms += Math.max(0, tick - s.time.start)

else if (s.status === "completed" || s.status === "error") ms += s.time.end - s.time.start
}
Comment on lines +80 to +93
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Seed the running-time ticker immediately to avoid stale/negative totals.

When a tool starts after sidebar mount, nowToolTime() can be stale until the first interval tick, which can yield negative elapsed math (tick - start) transiently.

Suggested fix
   const [nowToolTime, setNowToolTime] = createSignal(Date.now())
   createEffect(() => {
     if (!hasRunningTool()) return
+    setNowToolTime(Date.now())
     const id = setInterval(() => setNowToolTime(Date.now()), 1000)
     onCleanup(() => clearInterval(id))
   })
   const toolTime = createMemo(() => {
     let ms = 0
     const tick = nowToolTime()
     for (const part of toolParts()) {
       const s = part.state
-      if (s.status === "running") ms += tick - s.time.start
-      else if (s.status === "completed" || s.status === "error") ms += s.time.end - s.time.start
+      if (s.status === "running") ms += Math.max(0, tick - s.time.start)
+      else if (s.status === "completed" || s.status === "error") ms += Math.max(0, s.time.end - s.time.start)
     }
     return ms
   })
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/opencode/src/cli/cmd/tui/routes/session/sidebar.tsx` around lines 80
- 93, The running-time ticker can be stale until the first interval tick; inside
the createEffect that watches hasRunningTool(), immediately seed nowToolTime()
(call setNowToolTime(Date.now())) when a running tool is detected before
creating the setInterval, then create the interval as before and keep the
onCleanup(clearInterval) logic; update the createEffect block around
hasRunningTool(), nowToolTime, setNowToolTime, and the interval id to ensure the
first tick uses a fresh timestamp.

return ms
})
Comment on lines +86 to +95
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[🟠 MEDIUM] If the sidebar is mounted significantly earlier than a tool starts executing, the nowToolTime signal will hold an outdated timestamp (from component mount). When a new tool starts, toolTime will instantly re-evaluate, but because setInterval hasn't ticked yet, tick - s.time.start will result in a negative value.

This will either temporarily hide the tool time from the UI (if total time drops ≤ 0) or display an incorrect value for the first second of execution.

We can fix this by ensuring tick combines the reactive trigger with the actual current time, and guarding against negative execution times.

Suggested change:

Suggested change
const toolTime = createMemo(() => {
let ms = 0
const tick = nowToolTime()
for (const part of toolParts()) {
const s = part.state
if (s.status === "running") ms += tick - s.time.start
else if (s.status === "completed" || s.status === "error") ms += s.time.end - s.time.start
}
return ms
})
const toolTime = createMemo(() => {
let ms = 0
const tick = Math.max(nowToolTime(), Date.now())
for (const part of toolParts()) {
const s = part.state
if (s.status === "running") ms += Math.max(0, tick - s.time.start)
else if (s.status === "completed" || s.status === "error") ms += s.time.end - s.time.start
}
return ms
})

// altimate_change end

const directory = useDirectory()
const kv = useKV()

Expand Down Expand Up @@ -107,6 +140,13 @@ export function Sidebar(props: { sessionID: string; overlay?: boolean }) {
<text fg={theme.textMuted}>{context()?.tokens ?? 0} tokens</text>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[CRITICAL · code-reviewer] The toolTime calculation uses Date.now() as a live tick for running tools, but does not account for the possibility that multiple tools may be running simultaneously and their start times may be from different epochs. This could lead to incorrect cumulative timing if tools start at different times and the live tick advances uniformly.

💡 Suggestion: Instead of using a single nowToolTime signal for all tools, compute elapsed time per tool individually using their own start time and the current time, avoiding shared tick logic that assumes synchronized timing.

Confidence: 90/100

<text fg={theme.textMuted}>{context()?.percentage ?? 0}% used</text>
<text fg={theme.textMuted}>{cost()} spent</text>
{/* altimate_change start - total tool time */}
<Show when={toolTime() > 0}>
<text fg={theme.textMuted}>
{Locale.duration(toolTime())} in tools{hasRunningTool() ? " (running)" : ""}
</text>
</Show>
{/* altimate_change end */}
</box>
{/* altimate_change start - trace section */}
<box>
Expand Down
17 changes: 17 additions & 0 deletions packages/opencode/src/cli/cmd/tui/routes/session/timing.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { Locale } from "@/util/locale"

// Threshold above which a still-running tool is rendered in the warning color.
export const SLOW_TOOL_MS = 30_000

export interface Elapsed {
start: number
ms: number
running: boolean
}

// Single source of truth for the "HH:MM:SS · running 12.3s" suffix shown
// next to a tool call. Separator/prefix lives at the call site because it
// differs by surface (inline vs block).
export function formatElapsed(e: Elapsed): string {
return `${Locale.clockTime(e.start)} · ${e.running ? "running " : ""}${Locale.duration(e.ms)}`
}
5 changes: 5 additions & 0 deletions packages/opencode/src/util/locale.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ export namespace Locale {
return date.toLocaleTimeString(undefined, { timeStyle: "short" })
}

export function clockTime(input: number): string {
const date = new Date(input)
return date.toLocaleTimeString(undefined, { hour12: false, hour: "2-digit", minute: "2-digit", second: "2-digit" })
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: clockTime may render midnight as 24:00:00 because hour12: false alone does not guarantee a 0-23 hour cycle in Node.js ICU. Add hourCycle: "h23" to enforce standard 24-hour format.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/util/locale.ts, line 13:

<comment>`clockTime` may render midnight as `24:00:00` because `hour12: false` alone does not guarantee a 0-23 hour cycle in Node.js ICU. Add `hourCycle: "h23"` to enforce standard 24-hour format.</comment>

<file context>
@@ -8,6 +8,11 @@ export namespace Locale {
 
+  export function clockTime(input: number): string {
+    const date = new Date(input)
+    return date.toLocaleTimeString(undefined, { hour12: false, hour: "2-digit", minute: "2-digit", second: "2-digit" })
+  }
+
</file context>

}

export function datetime(input: number): string {
const date = new Date(input)
const localTime = time(input)
Expand Down
15 changes: 15 additions & 0 deletions packages/opencode/test/util/locale.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,21 @@ describe("Locale.duration", () => {
})
})

describe("Locale.clockTime", () => {
test("renders HH:MM:SS in 24-hour form", () => {
// 2024-01-15T03:04:05 in the host's local time, since the helper formats local time.
const d = new Date(2024, 0, 15, 3, 4, 5).getTime()
const out = Locale.clockTime(d)
expect(out).toMatch(/^\d{2}:\d{2}:\d{2}$/)
expect(out).toBe("03:04:05")
})

test("zero-pads single digit components", () => {
const d = new Date(2024, 0, 15, 9, 0, 7).getTime()
expect(Locale.clockTime(d)).toBe("09:00:07")
})
})

describe("Locale.truncateMiddle", () => {
test("returns original if short enough", () => {
expect(Locale.truncateMiddle("hello", 35)).toBe("hello")
Expand Down
Loading