refactor(ui): addresses PR review findings across date display PR

wgordon17 · wgordon17 · commit 55927580bcf9 · 2026-03-31T13:55:40.000-04:00
- Extracts RepoGitHubLink shared component, replacing duplicated anchor markup in IssuesTab, PullRequestsTab, and ActionsTab

- Moves clockInterval inside onMount to match coordinator lifecycle pattern

- Promotes depDashboard from string enum in tabFilters to top-level boolean (hideDepDashboard), eliminating resetAllTabFilters preserve hack

- Expands void refreshTick comments to explain SolidJS dependency tracking mechanism

- Normalizes Date.parse() usage in formatDuration for consistency with relativeTime/shortRelativeTime

- Adds tests for mixed valid/invalid dates, 360-day boundary, external link rendering, clock cleanup specificity, and hideDepDashboard lifecycle
diff --git a/src/app/components/dashboard/ActionsTab.tsx b/src/app/components/dashboard/ActionsTab.tsx
@@ -11,7 +11,7 @@ import type { FilterChipGroupDef } from "../shared/FilterChips";
 import ChevronIcon from "../shared/ChevronIcon";
 import ExpandCollapseButtons from "../shared/ExpandCollapseButtons";
 import RepoLockControls from "../shared/RepoLockControls";
-import ExternalLinkIcon from "../shared/ExternalLinkIcon";
+import RepoGitHubLink from "../shared/RepoGitHubLink";
 import { orderRepoGroups } from "../../lib/grouping";
 import { createReorderHighlight } from "../../lib/reorderHighlight";
 import { createFlashDetection } from "../../lib/flashDetection";
@@ -320,16 +320,7 @@ export default function ActionsTab(props: ActionsTabProps) {
                       </span>
                     </Show>
                   </button>
-                  <a
-                    href={`https://github.com/${repoGroup.repoFullName}/actions`}
-                    target="_blank"
-                    rel="noopener noreferrer"
-                    class="opacity-0 group-hover/repo-header:opacity-100 focus-visible:opacity-100 transition-opacity text-base-content/40 hover:text-primary px-1"
-                    title={`Open ${repoGroup.repoFullName} actions on GitHub`}
-                    aria-label={`Open ${repoGroup.repoFullName} actions on GitHub`}
-                  >
-                    <ExternalLinkIcon />
-                  </a>
+                  <RepoGitHubLink repoFullName={repoGroup.repoFullName} section="actions" />
                   <RepoLockControls tab="actions" repoFullName={repoGroup.repoFullName} />
                 </div>
                 <Show when={!isExpanded() && peekUpdates().get(repoGroup.repoFullName)}>
diff --git a/src/app/components/dashboard/DashboardPage.tsx b/src/app/components/dashboard/DashboardPage.tsx
@@ -240,6 +240,8 @@ export default function DashboardPage() {
     updateViewState({ lastActiveTab: tab });
   }
 
+  const [clockTick, setClockTick] = createSignal(0);
+
   onMount(() => {
     if (!_coordinator()) {
       _setCoordinator(createPollCoordinator(() => config.refreshInterval, pollFetch));
@@ -306,20 +308,19 @@ export default function DashboardPage() {
       });
     }
 
+    // Wall-clock tick keeps relative time displays fresh between full poll cycles.
+    const clockInterval = setInterval(() => setClockTick((t) => t + 1), 60_000);
+
     onCleanup(() => {
       _coordinator()?.destroy();
       _setCoordinator(null);
       _hotCoordinator()?.destroy();
       _setHotCoordinator(null);
       clearHotSets();
+      clearInterval(clockInterval);
     });
   });
 
-  // Wall-clock tick keeps relative time displays fresh between full poll cycles.
-  const [clockTick, setClockTick] = createSignal(0);
-  const clockInterval = setInterval(() => setClockTick((t) => t + 1), 60_000);
-  onCleanup(() => clearInterval(clockInterval));
-
   const refreshTick = createMemo(() => (dashboardData.lastRefreshedAt?.getTime() ?? 0) + clockTick());
 
   const tabCounts = createMemo(() => ({
diff --git a/src/app/components/dashboard/IssuesTab.tsx b/src/app/components/dashboard/IssuesTab.tsx
@@ -1,6 +1,6 @@
 import { createEffect, createMemo, createSignal, For, Show } from "solid-js";
 import { config, type TrackedUser } from "../../stores/config";
-import { viewState, setSortPreference, setTabFilter, resetTabFilter, resetAllTabFilters, ignoreItem, unignoreItem, toggleExpandedRepo, setAllExpanded, pruneExpandedRepos, pruneLockedRepos, type IssueFilterField } from "../../stores/view";
+import { viewState, updateViewState, setSortPreference, setTabFilter, resetTabFilter, resetAllTabFilters, ignoreItem, unignoreItem, toggleExpandedRepo, setAllExpanded, pruneExpandedRepos, pruneLockedRepos, type IssueFilterField } from "../../stores/view";
 import type { Issue, RepoRef } from "../../services/api";
 import ItemRow from "./ItemRow";
 import UserAvatarBadge, { buildSurfacedByUsers } from "../shared/UserAvatarBadge";
@@ -18,7 +18,7 @@ import { deriveInvolvementRoles } from "../../lib/format";
 import { groupByRepo, computePageLayout, slicePageGroups, orderRepoGroups } from "../../lib/grouping";
 import { createReorderHighlight } from "../../lib/reorderHighlight";
 import RepoLockControls from "../shared/RepoLockControls";
-import ExternalLinkIcon from "../shared/ExternalLinkIcon";
+import RepoGitHubLink from "../shared/RepoGitHubLink";
 
 export interface IssuesTabProps {
   issues: Issue[];
@@ -120,7 +120,7 @@ export default function IssuesTab(props: IssuesTabProps) {
         if (tabFilter.comments === "none" && issue.comments > 0) return false;
       }
 
-      if (tabFilter.depDashboard === "hide" && issue.title === "Dependency Dashboard") return false;
+      if (viewState.hideDepDashboard && issue.title === "Dependency Dashboard") return false;
 
       if (tabFilter.user !== "all") {
         // Items from monitored repos bypass the surfacedBy filter (all activity is shown)
@@ -248,12 +248,11 @@ export default function IssuesTab(props: IssuesTabProps) {
         />
         <button
           onClick={() => {
-            const next = viewState.tabFilters.issues.depDashboard === "show" ? "hide" : "show";
-            setTabFilter("issues", "depDashboard", next);
+            updateViewState({ hideDepDashboard: !viewState.hideDepDashboard });
             setPage(0);
           }}
-          class={`btn btn-xs rounded-full ${viewState.tabFilters.issues.depDashboard === "show" ? "btn-primary" : "btn-ghost text-base-content/50"}`}
-          aria-pressed={viewState.tabFilters.issues.depDashboard === "show"}
+          class={`btn btn-xs rounded-full ${!viewState.hideDepDashboard ? "btn-primary" : "btn-ghost text-base-content/50"}`}
+          aria-pressed={!viewState.hideDepDashboard}
           title="Toggle visibility of Dependency Dashboard issues"
         >
           Show Dep Dashboard
@@ -349,16 +348,7 @@ export default function IssuesTab(props: IssuesTabProps) {
                           </span>
                         </Show>
                       </button>
-                      <a
-                        href={`https://github.com/${repoGroup.repoFullName}/issues`}
-                        target="_blank"
-                        rel="noopener noreferrer"
-                        class="opacity-0 group-hover/repo-header:opacity-100 focus-visible:opacity-100 transition-opacity text-base-content/40 hover:text-primary px-1"
-                        title={`Open ${repoGroup.repoFullName} issues on GitHub`}
-                        aria-label={`Open ${repoGroup.repoFullName} issues on GitHub`}
-                      >
-                        <ExternalLinkIcon />
-                      </a>
+                      <RepoGitHubLink repoFullName={repoGroup.repoFullName} section="issues" />
                       <RepoLockControls tab="issues" repoFullName={repoGroup.repoFullName} />
                     </div>
                     <Show when={isExpanded()}>
diff --git a/src/app/components/dashboard/ItemRow.tsx b/src/app/components/dashboard/ItemRow.tsx
@@ -35,7 +35,9 @@ export default function ItemRow(props: ItemRowProps) {
     return { createdTitle, updatedTitle, diffMs };
   });
 
-  // void refreshTick — Date.now() is not reactive in SolidJS
+  // Reading props.refreshTick registers it as a SolidJS reactive dependency,
+  // forcing this memo to re-evaluate when the tick changes. Date.now() alone
+  // is not tracked by SolidJS's dependency system.
   const dateDisplay = createMemo(() => {
     void props.refreshTick;
     const created = shortRelativeTime(props.createdAt);
diff --git a/src/app/components/dashboard/PullRequestsTab.tsx b/src/app/components/dashboard/PullRequestsTab.tsx
@@ -22,7 +22,7 @@ import { groupByRepo, computePageLayout, slicePageGroups, orderRepoGroups } from
 import { createReorderHighlight } from "../../lib/reorderHighlight";
 import { createFlashDetection } from "../../lib/flashDetection";
 import RepoLockControls from "../shared/RepoLockControls";
-import ExternalLinkIcon from "../shared/ExternalLinkIcon";
+import RepoGitHubLink from "../shared/RepoGitHubLink";
 
 export interface PullRequestsTabProps {
   pullRequests: PullRequest[];
@@ -490,16 +490,7 @@ export default function PullRequestsTab(props: PullRequestsTabProps) {
                           </span>
                         </Show>
                       </button>
-                      <a
-                        href={`https://github.com/${repoGroup.repoFullName}/pulls`}
-                        target="_blank"
-                        rel="noopener noreferrer"
-                        class="opacity-0 group-hover/repo-header:opacity-100 focus-visible:opacity-100 transition-opacity text-base-content/40 hover:text-primary px-1"
-                        title={`Open ${repoGroup.repoFullName} pull requests on GitHub`}
-                        aria-label={`Open ${repoGroup.repoFullName} pull requests on GitHub`}
-                      >
-                        <ExternalLinkIcon />
-                      </a>
+                      <RepoGitHubLink repoFullName={repoGroup.repoFullName} section="pulls" />
                       <RepoLockControls tab="pullRequests" repoFullName={repoGroup.repoFullName} />
                     </div>
                     <Show when={!isExpanded() && peekUpdates().get(repoGroup.repoFullName)}>
diff --git a/src/app/components/dashboard/WorkflowRunRow.tsx b/src/app/components/dashboard/WorkflowRunRow.tsx
@@ -134,7 +134,9 @@ export default function WorkflowRunRow(props: WorkflowRunRowProps) {
 
   const createdTitle = createMemo(() => `Created: ${new Date(props.run.createdAt).toLocaleString()}`);
 
-  // void refreshTick — Date.now() is not reactive in SolidJS
+  // Reading props.refreshTick registers it as a SolidJS reactive dependency,
+  // forcing this memo to re-evaluate when the tick changes. Date.now() alone
+  // is not tracked by SolidJS's dependency system.
   const timeLabel = createMemo(() => {
     void props.refreshTick;
     return relativeTime(props.run.createdAt);
diff --git a/src/app/components/shared/RepoGitHubLink.tsx b/src/app/components/shared/RepoGitHubLink.tsx
@@ -0,0 +1,27 @@
+import ExternalLinkIcon from "./ExternalLinkIcon";
+
+const sectionLabels = {
+  issues: "issues",
+  pulls: "pull requests",
+  actions: "actions",
+} as const;
+
+export default function RepoGitHubLink(props: {
+  repoFullName: string;
+  section: "issues" | "pulls" | "actions";
+}) {
+  const label = () => sectionLabels[props.section];
+
+  return (
+    <a
+      href={`https://github.com/${props.repoFullName}/${props.section}`}
+      target="_blank"
+      rel="noopener noreferrer"
+      class="opacity-0 group-hover/repo-header:opacity-100 focus-visible:opacity-100 transition-opacity text-base-content/40 hover:text-primary px-1"
+      title={`Open ${props.repoFullName} ${label()} on GitHub`}
+      aria-label={`Open ${props.repoFullName} ${label()} on GitHub`}
+    >
+      <ExternalLinkIcon />
+    </a>
+  );
+}
diff --git a/src/app/lib/format.ts b/src/app/lib/format.ts
@@ -63,7 +63,7 @@ export function labelTextColor(hexColor: string): string {
 export function formatDuration(startedAt: string, completedAt: string | null): string {
   if (!startedAt) return "--";
   if (!completedAt) return "--";
-  const diffMs = new Date(completedAt).getTime() - new Date(startedAt).getTime();
+  const diffMs = Date.parse(completedAt) - Date.parse(startedAt);
   if (diffMs <= 0) return "--";
   const totalSec = Math.floor(diffMs / 1000);
   const h = Math.floor(totalSec / 3600);
diff --git a/src/app/stores/view.ts b/src/app/stores/view.ts
@@ -9,7 +9,6 @@ const IssueFiltersSchema = z.object({
   role: z.enum(["all", "author", "assignee"]).default("all"),
   comments: z.enum(["all", "has", "none"]).default("all"),
   user: z.enum(["all"]).or(z.string()).default("all"),
-  depDashboard: z.enum(["hide", "show"]).default("hide"),
 });
 
 const PullRequestFiltersSchema = z.object({
@@ -64,15 +63,16 @@ export const ViewStateSchema = z.object({
     })
     .default({ org: null, repo: null }),
   tabFilters: z.object({
-    issues: IssueFiltersSchema.default({ role: "all", comments: "all", user: "all", depDashboard: "hide" }),
+    issues: IssueFiltersSchema.default({ role: "all", comments: "all", user: "all" }),
     pullRequests: PullRequestFiltersSchema.default({ role: "all", reviewDecision: "all", draft: "all", checkStatus: "all", sizeCategory: "all", user: "all" }),
     actions: ActionsFiltersSchema.default({ conclusion: "all", event: "all" }),
   }).default({
-    issues: { role: "all", comments: "all", user: "all", depDashboard: "hide" },
+    issues: { role: "all", comments: "all", user: "all" },
     pullRequests: { role: "all", reviewDecision: "all", draft: "all", checkStatus: "all", sizeCategory: "all", user: "all" },
     actions: { conclusion: "all", event: "all" },
   }),
   showPrRuns: z.boolean().default(false),
+  hideDepDashboard: z.boolean().default(true),
   expandedRepos: z.object({
     issues: z.record(z.string(), z.boolean()).default({}),
     pullRequests: z.record(z.string(), z.boolean()).default({}),
@@ -122,11 +122,12 @@ export function resetViewState(): void {
     ignoredItems: [],
     globalFilter: { org: null, repo: null },
     tabFilters: {
-      issues: { role: "all", comments: "all", user: "all", depDashboard: "hide" },
+      issues: { role: "all", comments: "all", user: "all" },
       pullRequests: { role: "all", reviewDecision: "all", draft: "all", checkStatus: "all", sizeCategory: "all", user: "all" },
       actions: { conclusion: "all", event: "all" },
     },
     showPrRuns: false,
+    hideDepDashboard: true,
     expandedRepos: { issues: {}, pullRequests: {}, actions: {} },
     lockedRepos: { issues: [], pullRequests: [], actions: [] },
   });
@@ -224,9 +225,7 @@ export function resetAllTabFilters(
   setViewState(
     produce((draft) => {
       if (tab === "issues") {
-        const depDashboard = draft.tabFilters.issues.depDashboard;
         draft.tabFilters.issues = IssueFiltersSchema.parse({});
-        draft.tabFilters.issues.depDashboard = depDashboard;
       } else if (tab === "pullRequests") {
         draft.tabFilters.pullRequests = PullRequestFiltersSchema.parse({});
       } else {
diff --git a/tests/components/DashboardPage.test.tsx b/tests/components/DashboardPage.test.tsx
@@ -188,14 +188,15 @@ describe("DashboardPage — clock tick", () => {
     spy.mockRestore();
   });
 
-  it("clears the interval on unmount", () => {
+  it("clears the clock interval on unmount", () => {
     const setSpy = vi.spyOn(globalThis, "setInterval");
     const clearSpy = vi.spyOn(globalThis, "clearInterval");
     const { unmount } = render(() => <DashboardPage />);
-    const clockCall = setSpy.mock.calls.find(([, ms]) => ms === 60_000);
-    expect(clockCall).toBeDefined();
+    const clockCallIdx = setSpy.mock.calls.findIndex(([, ms]) => ms === 60_000);
+    expect(clockCallIdx).not.toBe(-1);
+    const clockIntervalId = setSpy.mock.results[clockCallIdx].value;
     unmount();
-    expect(clearSpy).toHaveBeenCalled();
+    expect(clearSpy).toHaveBeenCalledWith(clockIntervalId);
     setSpy.mockRestore();
     clearSpy.mockRestore();
   });
diff --git a/tests/components/IssuesTab.test.tsx b/tests/components/IssuesTab.test.tsx
@@ -602,12 +602,12 @@ describe("IssuesTab", () => {
     screen.getByText("Normal issue");
   });
 
-  it("shows Dependency Dashboard issues when toggle is active", () => {
+  it("shows Dependency Dashboard issues when hideDepDashboard is false", () => {
     const issues = [
       makeIssue({ id: 1, title: "Dependency Dashboard" }),
       makeIssue({ id: 2, title: "Normal issue" }),
     ];
-    viewStore.setTabFilter("issues", "depDashboard", "show");
+    viewStore.updateViewState({ hideDepDashboard: false });
     setAllExpanded("issues", ["owner/repo"], true);
     render(() => <IssuesTab issues={issues} userLogin="" />);
     screen.getByText("Dependency Dashboard");
@@ -634,4 +634,14 @@ describe("IssuesTab", () => {
     await user.click(screen.getByText("Show Dep Dashboard"));
     expect(screen.queryByText("Dependency Dashboard")).toBeNull();
   });
+
+  it("renders repo header link to GitHub issues", () => {
+    const issues = [makeIssue({ id: 1 })];
+    setAllExpanded("issues", ["owner/repo"], true);
+    render(() => <IssuesTab issues={issues} userLogin="" />);
+    const link = screen.getByLabelText("Open owner/repo issues on GitHub");
+    expect(link.getAttribute("href")).toBe("https://github.com/owner/repo/issues");
+    expect(link.getAttribute("target")).toBe("_blank");
+    expect(link.getAttribute("rel")).toBe("noopener noreferrer");
+  });
 });
diff --git a/tests/components/ItemRow.test.tsx b/tests/components/ItemRow.test.tsx
@@ -235,7 +235,7 @@ describe("ItemRow", () => {
     expect(container.querySelector(`time[datetime="${createdAt}"]`)!.textContent).toBe("3d");
   });
 
-  it("suppresses update display when dates are invalid", () => {
+  it("suppresses update display when both dates are invalid", () => {
     const { container } = render(() => (
       <ItemRow {...defaultProps} createdAt="not-a-date" updatedAt="also-invalid" />
     ));
@@ -244,6 +244,22 @@ describe("ItemRow", () => {
     expect(container.querySelector("time")!.textContent).toBe("");
   });
 
+  it("suppresses update display when createdAt is valid but updatedAt is invalid", () => {
+    const { container } = render(() => (
+      <ItemRow {...defaultProps} createdAt="2026-03-30T10:00:00Z" updatedAt="not-a-date" />
+    ));
+    expect(container.querySelector('span[aria-hidden="true"]')).toBeNull();
+    expect(container.querySelectorAll("time").length).toBe(1);
+  });
+
+  it("suppresses update display when updatedAt is valid but createdAt is invalid", () => {
+    const { container } = render(() => (
+      <ItemRow {...defaultProps} createdAt="not-a-date" updatedAt="2026-03-30T11:30:00Z" />
+    ));
+    expect(container.querySelector('span[aria-hidden="true"]')).toBeNull();
+    expect(container.querySelectorAll("time").length).toBe(1);
+  });
+
   it("renders correct datetime attributes on time elements", () => {
     const { container } = render(() => <ItemRow {...defaultProps} />);
     const timeEls = container.querySelectorAll("time");
diff --git a/tests/lib/format.test.ts b/tests/lib/format.test.ts
@@ -136,6 +136,16 @@ describe("shortRelativeTime", () => {
     expect(shortRelativeTime(isoString)).toBe("1y");
   });
 
+  it("returns '11mo' for 359 days ago (just before year boundary)", () => {
+    const isoString = new Date(MOCK_NOW - 359 * 24 * 60 * 60 * 1000).toISOString();
+    expect(shortRelativeTime(isoString)).toBe("11mo");
+  });
+
+  it("returns '1y' for 360 days ago (exact year boundary at 12×30)", () => {
+    const isoString = new Date(MOCK_NOW - 360 * 24 * 60 * 60 * 1000).toISOString();
+    expect(shortRelativeTime(isoString)).toBe("1y");
+  });
+
   it("returns compact years for 400 days ago", () => {
     const isoString = new Date(MOCK_NOW - 400 * 24 * 60 * 60 * 1000).toISOString();
     expect(shortRelativeTime(isoString)).toBe("1y");
diff --git a/tests/stores/view.test.ts b/tests/stores/view.test.ts
