Skip to content

Commit 02cea2d

Browse files
committed
fix: addresses PR review findings for date display
- adds 60s wall-clock tick for relative time freshness (cor-1) - memoizes WorkflowRunRow tooltip with Created prefix (cq-1, cq-2) - renames hasUpdate to shouldShowUpdated (cq-5) - simplifies duplicate reactivity comments (cq-4) - switches ItemRow test locators to querySelector (gh-5) - fixes double-mock pattern with vi.mocked (qa-4) - adds 2mo, invalid date, and title attribute tests (qa-1, qa-2, qa-3)
1 parent 64aae0f commit 02cea2d

File tree

6 files changed

+58
-37
lines changed

6 files changed

+58
-37
lines changed

src/app/components/dashboard/DashboardPage.tsx

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -314,7 +314,12 @@ export default function DashboardPage() {
314314
});
315315
});
316316

317-
const refreshTick = createMemo(() => dashboardData.lastRefreshedAt?.getTime() ?? 0);
317+
// Wall-clock tick keeps relative time displays fresh between full poll cycles.
318+
const [clockTick, setClockTick] = createSignal(0);
319+
const clockInterval = setInterval(() => setClockTick((t) => t + 1), 60_000);
320+
onCleanup(() => clearInterval(clockInterval));
321+
322+
const refreshTick = createMemo(() => (dashboardData.lastRefreshedAt?.getTime() ?? 0) + clockTick());
318323

319324
const tabCounts = createMemo(() => ({
320325
issues: dashboardData.issues.length,

src/app/components/dashboard/ItemRow.tsx

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,7 @@ export default function ItemRow(props: ItemRowProps) {
3535
return { createdTitle, updatedTitle, diffMs };
3636
});
3737

38-
// Reactive date display — re-run on refreshTick to keep relative times current.
39-
// Date.now() is not reactive in SolidJS; refreshTick is the explicit invalidation signal.
38+
// void refreshTick — Date.now() is not reactive in SolidJS
4039
const dateDisplay = createMemo(() => {
4140
void props.refreshTick;
4241
const created = shortRelativeTime(props.createdAt);
@@ -46,7 +45,7 @@ export default function ItemRow(props: ItemRowProps) {
4645
return { created, updated, createdLabel, updatedLabel };
4746
});
4847

49-
const hasUpdate = createMemo(() => {
48+
const shouldShowUpdated = createMemo(() => {
5049
const { diffMs } = staticDateInfo();
5150
if (diffMs <= 60_000) return false;
5251
const { created, updated } = dateDisplay();
@@ -138,7 +137,7 @@ export default function ItemRow(props: ItemRowProps) {
138137
>
139138
{dateDisplay().created}
140139
</time>
141-
<Show when={hasUpdate()}>
140+
<Show when={shouldShowUpdated()}>
142141
<span aria-hidden="true">{"\u00B7"}</span>
143142
<time
144143
datetime={props.updatedAt}

src/app/components/dashboard/WorkflowRunRow.tsx

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -132,8 +132,9 @@ export default function WorkflowRunRow(props: WorkflowRunRowProps) {
132132
const paddingClass = () =>
133133
props.density === "compact" ? "py-1.5 px-3" : "py-2.5 px-4";
134134

135-
// Re-run on refreshTick to keep relative time current.
136-
// Date.now() is not reactive in SolidJS; refreshTick is the explicit invalidation signal.
135+
const createdTitle = createMemo(() => `Created: ${new Date(props.run.createdAt).toLocaleString()}`);
136+
137+
// void refreshTick — Date.now() is not reactive in SolidJS
137138
const timeLabel = createMemo(() => {
138139
void props.refreshTick;
139140
return relativeTime(props.run.createdAt);
@@ -182,7 +183,7 @@ export default function WorkflowRunRow(props: WorkflowRunRowProps) {
182183
<time
183184
class="text-xs text-base-content/40 shrink-0"
184185
datetime={props.run.createdAt}
185-
title={new Date(props.run.createdAt).toLocaleString()}
186+
title={createdTitle()}
186187
>
187188
{timeLabel()}
188189
</time>

tests/components/ItemRow.test.tsx

Lines changed: 38 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -45,11 +45,11 @@ describe("ItemRow", () => {
4545
});
4646

4747
it("renders relative time for createdAt", () => {
48-
render(() => <ItemRow {...defaultProps} />);
48+
const { container } = render(() => <ItemRow {...defaultProps} />);
4949
// Should show compact format like "2h"
50-
const timeEl = screen.getByTitle(`Created: ${new Date(defaultProps.createdAt).toLocaleString()}`);
51-
expect(timeEl).toBeDefined();
52-
expect(timeEl.textContent).toBe("2h");
50+
const timeEl = container.querySelector(`time[datetime="${defaultProps.createdAt}"]`);
51+
expect(timeEl).not.toBeNull();
52+
expect(timeEl!.textContent).toBe("2h");
5353
});
5454

5555
it("renders children slot when provided", () => {
@@ -177,8 +177,12 @@ describe("ItemRow", () => {
177177
it("shows both dates when updatedAt meaningfully differs from createdAt", () => {
178178
const { container } = render(() => <ItemRow {...defaultProps} />);
179179
// createdAt=2h ago → "2h", updatedAt=30m ago → "30m"
180-
expect(screen.getByTitle(`Created: ${new Date(defaultProps.createdAt).toLocaleString()}`).textContent).toBe("2h");
181-
expect(screen.getByTitle(`Updated: ${new Date(defaultProps.updatedAt).toLocaleString()}`).textContent).toBe("30m");
180+
const created = container.querySelector(`time[datetime="${defaultProps.createdAt}"]`);
181+
const updated = container.querySelector(`time[datetime="${defaultProps.updatedAt}"]`);
182+
expect(created!.textContent).toBe("2h");
183+
expect(created!.getAttribute("title")).toBe(`Created: ${new Date(defaultProps.createdAt).toLocaleString()}`);
184+
expect(updated!.textContent).toBe("30m");
185+
expect(updated!.getAttribute("title")).toBe(`Updated: ${new Date(defaultProps.updatedAt).toLocaleString()}`);
182186
// Middle dot separator is a <span> with aria-hidden
183187
const dot = container.querySelector('span[aria-hidden="true"]');
184188
expect(dot).not.toBeNull();
@@ -191,7 +195,7 @@ describe("ItemRow", () => {
191195
<ItemRow {...defaultProps} createdAt={sameDate} updatedAt={sameDate} />
192196
));
193197
expect(container.querySelector('span[aria-hidden="true"]')).toBeNull();
194-
expect(screen.queryByTitle(`Updated: ${new Date(sameDate).toLocaleString()}`)).toBeNull();
198+
expect(container.querySelectorAll("time").length).toBe(1);
195199
});
196200

197201
it("shows single date when updatedAt is within 60s of createdAt", () => {
@@ -202,9 +206,9 @@ describe("ItemRow", () => {
202206
updatedAt="2026-03-30T11:59:30Z"
203207
/>
204208
));
205-
// Only one time span — no dot separator span
209+
// Only one time element — no dot separator
206210
expect(container.querySelector('span[aria-hidden="true"]')).toBeNull();
207-
expect(screen.queryByTitle(`Updated: ${new Date("2026-03-30T11:59:30Z").toLocaleString()}`)).toBeNull();
211+
expect(container.querySelectorAll("time").length).toBe(1);
208212
});
209213

210214
it("shows single date when updatedAt is exactly 60s after createdAt", () => {
@@ -226,9 +230,18 @@ describe("ItemRow", () => {
226230
const { container } = render(() => (
227231
<ItemRow {...defaultProps} createdAt={createdAt} updatedAt={updatedAt} />
228232
));
229-
// diff > 60s but both show "3d" — no dot separator span
233+
// diff > 60s but both show "3d" — no dot separator
234+
expect(container.querySelector('span[aria-hidden="true"]')).toBeNull();
235+
expect(container.querySelector(`time[datetime="${createdAt}"]`)!.textContent).toBe("3d");
236+
});
237+
238+
it("suppresses update display when dates are invalid", () => {
239+
const { container } = render(() => (
240+
<ItemRow {...defaultProps} createdAt="not-a-date" updatedAt="also-invalid" />
241+
));
230242
expect(container.querySelector('span[aria-hidden="true"]')).toBeNull();
231-
expect(screen.getByTitle(`Created: ${new Date(createdAt).toLocaleString()}`).textContent).toBe("3d");
243+
expect(container.querySelectorAll("time").length).toBe(1);
244+
expect(container.querySelector("time")!.textContent).toBe("");
232245
});
233246

234247
it("renders correct datetime attributes on time elements", () => {
@@ -240,35 +253,32 @@ describe("ItemRow", () => {
240253
});
241254

242255
it("shows verbose aria-label for created and updated spans", () => {
243-
render(() => <ItemRow {...defaultProps} />);
244-
const createdSpan = screen.getByTitle(`Created: ${new Date(defaultProps.createdAt).toLocaleString()}`);
245-
const updatedSpan = screen.getByTitle(`Updated: ${new Date(defaultProps.updatedAt).toLocaleString()}`);
246-
expect(createdSpan.getAttribute("aria-label")).toMatch(/^Created 2 hours? ago$/);
247-
expect(updatedSpan.getAttribute("aria-label")).toMatch(/^Updated 30 minutes? ago$/);
256+
const { container } = render(() => <ItemRow {...defaultProps} />);
257+
const created = container.querySelector(`time[datetime="${defaultProps.createdAt}"]`);
258+
const updated = container.querySelector(`time[datetime="${defaultProps.updatedAt}"]`);
259+
expect(created!.getAttribute("aria-label")).toMatch(/^Created 2 hours? ago$/);
260+
expect(updated!.getAttribute("aria-label")).toMatch(/^Updated 30 minutes? ago$/);
248261
});
249262

250263
it("refreshTick forces time display update", () => {
251264
const [tick, setTick] = createSignal(0);
252265
let mockNow = MOCK_NOW;
253-
vi.spyOn(Date, "now").mockImplementation(() => mockNow);
266+
vi.mocked(Date.now).mockImplementation(() => mockNow);
254267

255-
// createdAt is 2h before MOCK_NOW → displays "2h"
256-
// updatedAt is 30m before MOCK_NOW → displays "30m"
257-
render(() => (
258-
<ItemRow
259-
{...defaultProps}
260-
refreshTick={tick()}
261-
/>
268+
const { container } = render(() => (
269+
<ItemRow {...defaultProps} refreshTick={tick()} />
262270
));
263-
expect(screen.getByTitle(`Created: ${new Date(defaultProps.createdAt).toLocaleString()}`).textContent).toBe("2h");
264-
expect(screen.getByTitle(`Updated: ${new Date(defaultProps.updatedAt).toLocaleString()}`).textContent).toBe("30m");
271+
const created = container.querySelector(`time[datetime="${defaultProps.createdAt}"]`);
272+
const updated = container.querySelector(`time[datetime="${defaultProps.updatedAt}"]`);
273+
expect(created!.textContent).toBe("2h");
274+
expect(updated!.textContent).toBe("30m");
265275

266276
// Advance mock time by 3 hours and bump refreshTick
267277
mockNow = MOCK_NOW + 3 * 60 * 60 * 1000;
268278
setTick(1);
269279

270-
expect(screen.getByTitle(`Created: ${new Date(defaultProps.createdAt).toLocaleString()}`).textContent).toBe("5h");
280+
expect(created!.textContent).toBe("5h");
271281
// updatedAt was 30m before MOCK_NOW; after +3h it is 3h30m ago → Math.floor(210/60) = 3 → "3h"
272-
expect(screen.getByTitle(`Updated: ${new Date(defaultProps.updatedAt).toLocaleString()}`).textContent).toBe("3h");
282+
expect(updated!.textContent).toBe("3h");
273283
});
274284
});

tests/components/WorkflowRunRow.test.tsx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,12 +36,13 @@ describe("WorkflowRunRow", () => {
3636
const timeEl = container.querySelector("time");
3737
expect(timeEl).not.toBeNull();
3838
expect(timeEl!.getAttribute("datetime")).toBe(createdAt);
39+
expect(timeEl!.getAttribute("title")).toBe(`Created: ${new Date(createdAt).toLocaleString()}`);
3940
expect(timeEl!.textContent).toMatch(/2 hours? ago/);
4041
});
4142

4243
it("updates time display when refreshTick changes", () => {
4344
let mockNow = MOCK_NOW;
44-
vi.spyOn(Date, "now").mockImplementation(() => mockNow);
45+
vi.mocked(Date.now).mockImplementation(() => mockNow);
4546
const createdAt = new Date(MOCK_NOW - 2 * 60 * 60 * 1000).toISOString();
4647
const run = makeWorkflowRun({ createdAt });
4748
const [tick, setTick] = createSignal(0);

tests/lib/format.test.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,11 @@ describe("shortRelativeTime", () => {
121121
expect(shortRelativeTime(isoString)).toBe("1mo");
122122
});
123123

124+
it("returns '2mo' for 2 months ago", () => {
125+
const isoString = new Date(MOCK_NOW - 2 * 30 * 24 * 60 * 60 * 1000).toISOString();
126+
expect(shortRelativeTime(isoString)).toBe("2mo");
127+
});
128+
124129
it("returns '11mo' for 11 months ago", () => {
125130
const isoString = new Date(MOCK_NOW - 11 * 30 * 24 * 60 * 60 * 1000).toISOString();
126131
expect(shortRelativeTime(isoString)).toBe("11mo");

0 commit comments

Comments
 (0)