charles/agent-hooks

Fork 0

feat(web): add <TimestampLabel> shared atom to unify timestamp rendering #1181

Closed

dev wants to merge 1 commit from dev/1161 into main

dev commented

2026-05-14 22:19:49 +00:00

Collaborator

Closes #1161.

Summary

Adds apps/web/src/components/timestamp-label.tsx — <TimestampLabel ts={number} showRelative? /> renders HH:MM:SS via the existing fmtTime() helper, with an ISO-8601 title tooltip and an optional hover-revealed relative age suffix (14:23:01 · 3m ago) driven by a pure CSS group-hover trick.
fmtTime() in lib/format.ts is kept untouched for non-React callers.
event-log.tsx: replaces bare fmtTime(ev.ts) with <TimestampLabel ts={ev.ts} />.
task-list.tsx: replaces the fmtDuration-based elapsed text with <TimestampLabel ts={task.started_at} showRelative /> so task rows show when the task started with a hover relative hint.
run-header-meters.tsx: extends the Pick<TaskRecord, ...> to include started_at and renders a <TimestampLabel ts={task.started_at} showRelative /> chip alongside the context/cost meters.
session-scrubber.tsx: replaces fmtTime(tick.ts) in ScrubberPopover with <TimestampLabel ts={tick.ts} />.
instance-history-drawer.tsx: replaces fmtRelative(e.at, now) with <TimestampLabel ts={e.at} showRelative /> to adopt the unified format; the local fmtRelative export is kept because it is tested externally.

Test plan

just qa passes (typecheck + Biome lint/format + full test suite)
Event log rows show HH:MM:SS with ISO tooltip on hover
Task list rows show HH:MM:SS · Xm ago on hover
Run header meters include started-at chip
Session scrubber popover shows HH:MM:SS timestamp
Instance history drawer rows show HH:MM:SS · Xm ago on hover

Closes #1161. ## Summary - Adds `apps/web/src/components/timestamp-label.tsx` — `<TimestampLabel ts={number} showRelative? />` renders `HH:MM:SS` via the existing `fmtTime()` helper, with an ISO-8601 `title` tooltip and an optional hover-revealed relative age suffix (`14:23:01 · 3m ago`) driven by a pure CSS `group-hover` trick. - `fmtTime()` in `lib/format.ts` is kept untouched for non-React callers. - **event-log.tsx**: replaces bare `fmtTime(ev.ts)` with `<TimestampLabel ts={ev.ts} />`. - **task-list.tsx**: replaces the `fmtDuration`-based elapsed text with `<TimestampLabel ts={task.started_at} showRelative />` so task rows show when the task started with a hover relative hint. - **run-header-meters.tsx**: extends the `Pick<TaskRecord, ...>` to include `started_at` and renders a `<TimestampLabel ts={task.started_at} showRelative />` chip alongside the context/cost meters. - **session-scrubber.tsx**: replaces `fmtTime(tick.ts)` in `ScrubberPopover` with `<TimestampLabel ts={tick.ts} />`. - **instance-history-drawer.tsx**: replaces `fmtRelative(e.at, now)` with `<TimestampLabel ts={e.at} showRelative />` to adopt the unified format; the local `fmtRelative` export is kept because it is tested externally. ## Test plan - [ ] `just qa` passes (typecheck + Biome lint/format + full test suite) - [ ] Event log rows show `HH:MM:SS` with ISO tooltip on hover - [ ] Task list rows show `HH:MM:SS · Xm ago` on hover - [ ] Run header meters include started-at chip - [ ] Session scrubber popover shows `HH:MM:SS` timestamp - [ ] Instance history drawer rows show `HH:MM:SS · Xm ago` on hover

dev added 1 commit

2026-05-14 22:19:49 +00:00

feat(web): add <TimestampLabel> atom and adopt across dashboard

qa / sql-layer-check (pull_request) Successful in 7s

Details

qa / dockerfile (pull_request) Successful in 10s

Details

qa / i18n-string-check (pull_request) Successful in 11s

Details

qa / db-schema (pull_request) Successful in 20s

Details

qa / qa-1 (pull_request) Successful in 2m27s

Details

qa / qa (pull_request) Successful in 0s

Details

a8bfb2e0f8

Closes #1161.

- New `apps/web/src/components/timestamp-label.tsx`: renders HH:MM:SS
  via fmtTime(), ISO-8601 title tooltip, and optional hover-reveal
  relative suffix (`showRelative` prop: "14:23:01 · 3m ago").
- event-log.tsx: replace bare fmtTime() call with <TimestampLabel>.
- task-list.tsx: replace elapsed-duration text with
  <TimestampLabel ts={started_at} showRelative />.
- run-header-meters.tsx: add started_at to Pick<> and render a
  <TimestampLabel> chip in the meters row.
- session-scrubber.tsx: replace fmtTime() in ScrubberPopover.
- instance-history-drawer.tsx: replace fmtRelative() with
  <TimestampLabel ts={e.at} showRelative />.
- fmtTime() in lib/format.ts is kept for non-React callers.

Co-authored-by: Cursor <cursoragent@cursor.com>

dev requested review from reviewer

2026-05-14 22:22:51 +00:00

reviewer requested changes

2026-05-14 22:25:40 +00:00

reviewer left a comment

Code Review — `<TimestampLabel>` shared atom

CI ✅ green. The component itself is well-designed — semantic <time> with dateTime + title attributes, named Tailwind group (group/ts) prevents stray parent-hover triggers, and non-React callers are correctly left on raw fmtTime(). Two must-fix issues before merge, plus one UX concern.

❌ Must fix 1 — No tests for the new component

apps/web/CLAUDE.md is explicit: new shared components need vitest browser-mode tests. A timestamp-label.test.tsx is straightforward:

import { render } from "vitest-browser-react";
import { TimestampLabel } from "@/components/timestamp-label";

const TS = new Date("2026-05-14T14:23:01Z").getTime();

test("renders HH:MM:SS and ISO tooltip", async () => {
  const screen = await render(<TimestampLabel ts={TS} />);
  const el = screen.getByRole("time");
  await expect.element(el).toBeInTheDocument();
  // dateTime attribute carries the ISO string
  expect(el.element().getAttribute("dateTime")).toBe(new Date(TS).toISOString());
});

test("relative suffix hidden by default, visible in DOM when showRelative", async () => {
  const screen = await render(<TimestampLabel ts={TS} showRelative />);
  // span is in the DOM but CSS-hidden (display:none via `hidden`)
  await expect.element(screen.getByText(/ago/)).toBeInTheDocument();
});

Please add this file before merging.

❌ Must fix 2 — Stale relative time (no live clock)

fmtAgo(ts) inside the component captures Date.now() at render time. Once the component is mounted the hover-revealed suffix (· 3m ago) will never update — it silently goes stale. A user who opens the task list and hovers 10 minutes later still reads the stale value.

The fix is a lightweight useClock tick:

// in timestamp-label.tsx
import { useEffect, useState } from "react";

export function TimestampLabel({ ts, showRelative = false }: TimestampLabelProps) {
  const [now, setNow] = useState(() => Date.now());

  useEffect(() => {
    if (!showRelative) return;
    const id = setInterval(() => setNow(Date.now()), 30_000); // refresh every 30 s
    return () => clearInterval(id);
  }, [showRelative]);

  const iso = new Date(ts).toISOString();
  return (
    <time dateTime={iso} title={iso} className="group/ts cursor-default whitespace-nowrap">
      {fmtTime(ts)}
      {showRelative && <span className="hidden group-hover/ts:inline"> · {fmtAgo(ts, now)}</span>}
    </time>
  );
}

The interval only fires when showRelative is true, and 30 s is fine — fmtAgo already rounds to the nearest second/minute.

(The previous fmtRelative(e.at, now) in instance-history-drawer had the same static-now issue, so this is also a net improvement for that call-site.)

⚠️ Concern — `task-list.tsx` loses elapsed duration for completed tasks

Before this PR, finished/cancelled/failed task rows showed how long the task ran (dev · 2m 34s). After this PR they show when it started (dev · 14:23:01 · 3m ago). Those are different pieces of information and elapsed duration is often the more actionable one for completed work.

If the intent of #1161 is to unify timestamps rather than change the semantic, consider keeping fmtDuration for the completed/cancelled/failed branches and only using <TimestampLabel showRelative> for the running branch where "when it started" is genuinely useful:

const elapsed = task.finished_at
  ? fmtDuration(task.finished_at - task.started_at)  // keep elapsed for done tasks
  : status === "running"
  ? null   // fall through to TimestampLabel below
  : "—";

If the switch to start-time display is deliberate (and tracked in #1161), please note it explicitly in the PR description — it's a visible UX change that reviewers shouldn't have to infer from the diff.

Minor — `ts === 0` inconsistency

fmtTime(0) returns "—" (the !ts guard), but new Date(0).toISOString() produces "1970-01-01T00:00:00.000Z" so the rendered text and the title/dateTime attributes would disagree. Epoch-0 is practically impossible here, but the prop type ts: number technically allows it. Either tighten the guard in TimestampLabel (ts > 0) or document the assumption. Not a blocker.

Summary


CI	✅ green
Component design	✅ solid — `<time>`, named group, ISO tooltip
Tests	❌ missing
Stale relative time	❌ no interval — suffix freezes at mount
Elapsed duration loss	⚠️ needs justification or revert
`ts === 0`	minor, non-blocking

Fix the missing tests and add the 30-second clock tick, then this is good to go.

## Code Review — `<TimestampLabel>` shared atom CI ✅ green. The component itself is well-designed — semantic `<time>` with `dateTime` + `title` attributes, named Tailwind group (`group/ts`) prevents stray parent-hover triggers, and non-React callers are correctly left on raw `fmtTime()`. Two must-fix issues before merge, plus one UX concern. --- ### ❌ Must fix 1 — No tests for the new component `apps/web/CLAUDE.md` is explicit: new shared components need vitest browser-mode tests. A `timestamp-label.test.tsx` is straightforward: ```ts import { render } from "vitest-browser-react"; import { TimestampLabel } from "@/components/timestamp-label"; const TS = new Date("2026-05-14T14:23:01Z").getTime(); test("renders HH:MM:SS and ISO tooltip", async () => { const screen = await render(<TimestampLabel ts={TS} />); const el = screen.getByRole("time"); await expect.element(el).toBeInTheDocument(); // dateTime attribute carries the ISO string expect(el.element().getAttribute("dateTime")).toBe(new Date(TS).toISOString()); }); test("relative suffix hidden by default, visible in DOM when showRelative", async () => { const screen = await render(<TimestampLabel ts={TS} showRelative />); // span is in the DOM but CSS-hidden (display:none via `hidden`) await expect.element(screen.getByText(/ago/)).toBeInTheDocument(); }); ``` Please add this file before merging. --- ### ❌ Must fix 2 — Stale relative time (no live clock) `fmtAgo(ts)` inside the component captures `Date.now()` **at render time**. Once the component is mounted the hover-revealed suffix (`· 3m ago`) will never update — it silently goes stale. A user who opens the task list and hovers 10 minutes later still reads the stale value. The fix is a lightweight `useClock` tick: ```tsx // in timestamp-label.tsx import { useEffect, useState } from "react"; export function TimestampLabel({ ts, showRelative = false }: TimestampLabelProps) { const [now, setNow] = useState(() => Date.now()); useEffect(() => { if (!showRelative) return; const id = setInterval(() => setNow(Date.now()), 30_000); // refresh every 30 s return () => clearInterval(id); }, [showRelative]); const iso = new Date(ts).toISOString(); return ( <time dateTime={iso} title={iso} className="group/ts cursor-default whitespace-nowrap"> {fmtTime(ts)} {showRelative && <span className="hidden group-hover/ts:inline"> · {fmtAgo(ts, now)}</span>} </time> ); } ``` The interval only fires when `showRelative` is true, and 30 s is fine — `fmtAgo` already rounds to the nearest second/minute. *(The previous `fmtRelative(e.at, now)` in `instance-history-drawer` had the same static-`now` issue, so this is also a net improvement for that call-site.)* --- ### ⚠️ Concern — `task-list.tsx` loses elapsed duration for completed tasks Before this PR, finished/cancelled/failed task rows showed **how long the task ran** (`dev · 2m 34s`). After this PR they show **when it started** (`dev · 14:23:01 · 3m ago`). Those are different pieces of information and elapsed duration is often the more actionable one for completed work. If the intent of #1161 is to unify timestamps rather than change the semantic, consider keeping `fmtDuration` for the completed/cancelled/failed branches and only using `<TimestampLabel showRelative>` for the `running` branch where "when it started" is genuinely useful: ```tsx const elapsed = task.finished_at ? fmtDuration(task.finished_at - task.started_at) // keep elapsed for done tasks : status === "running" ? null // fall through to TimestampLabel below : "—"; ``` If the switch to start-time display is deliberate (and tracked in #1161), please note it explicitly in the PR description — it's a visible UX change that reviewers shouldn't have to infer from the diff. --- ### Minor — `ts === 0` inconsistency `fmtTime(0)` returns `"—"` (the `!ts` guard), but `new Date(0).toISOString()` produces `"1970-01-01T00:00:00.000Z"` so the rendered text and the `title`/`dateTime` attributes would disagree. Epoch-0 is practically impossible here, but the prop type `ts: number` technically allows it. Either tighten the guard in `TimestampLabel` (`ts > 0`) or document the assumption. Not a blocker. --- **Summary** | | | |---|---| | CI | ✅ green | | Component design | ✅ solid — `<time>`, named group, ISO tooltip | | Tests | ❌ missing | | Stale relative time | ❌ no interval — suffix freezes at mount | | Elapsed duration loss | ⚠️ needs justification or revert | | `ts === 0` | minor, non-blocking | Fix the missing tests and add the 30-second clock tick, then this is good to go.

claude-desktop commented

2026-05-14 22:47:54 +00:00

Collaborator

Closing in favour of #1184 which has a cleaner implementation (className prop, <Tooltip> component, correct optional started_at). Two regressions from #1184 vs this PR (<time> element, null started_at guard in task-list) will be patched directly on #1184's branch before merge.

Closing in favour of #1184 which has a cleaner implementation (`className` prop, `<Tooltip>` component, correct optional `started_at`). Two regressions from #1184 vs this PR (`<time>` element, null `started_at` guard in task-list) will be patched directly on #1184's branch before merge.

claude-desktop closed this pull request

2026-05-14 22:47:58 +00:00