From 579bb7de8f1fa7c99811452b953f51cc220396e4 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Tue, 28 Jul 2026 23:40:43 +0800 Subject: [PATCH] fix(console,runner): render the approvals inbox against one ticking clock, and lint both packages (#2927) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `apps/console` and `packages/runner` had no `lint` script, so `turbo run lint` skipped them silently and their 17 ESLint errors had never been seen. Both now carry `"lint": "eslint ."`, and the `DEBT` list in check-lint-coverage.mjs is empty — 45/45 workspace packages are linted. Read one by one, the 17 were not one class of problem: 8x react-hooks/purity — real. The approvals inbox read `Date.now()` mid-render for every age tint, "5m ago" label and SLA chip. Render must be pure: the output depended on when React happened to render, so it disagreed with itself under StrictMode's double render and then froze — an inbox left open kept saying "just now" and an SLA countdown never counted down. The page now renders against a single `now` held in state and advanced once a minute (the finest granularity it displays), so render is a pure function of props+state and the figures tick. `sla_due_at` also goes through a parse guard now: a due date the backend sends in a shape `Date.parse` can't read rendered as "SLA NaNh left"; it renders nothing. 1x react-hooks/static-components — real. `StatusBadge` was declared inside `ApprovalsInboxPage`, so it was a brand-new component type every render and React remounted every status chip in the table on each re-render. Hoisted to module scope with the translated label passed as a prop. 6x react-hooks/static-components — false positives (3 settings pages, 3 in the runner's LayoutRenderer). All six render the result of `getIcon`/`getLazyIcon`, which memoises per name in a module-level cache: the reference is stable and nothing is created during render. They carry the same targeted disable + justification the repo already uses at a dozen icon-registry sites, and both resolvers now document the cache. Verified rather than assumed — typing into a settings field keeps focus and every character, so nothing was being reset. 2 minor — a dead `token` initializer on the auth-preflight path (read, not blind -deleted: no intended write was missing) and a `prefer-const`. Closes #2927. Refs #2911, #2923. Co-Authored-By: Claude --- .../lint-console-runner-react-correctness.md | 44 ++++++ apps/console/package.json | 1 + apps/console/src/lib/auth-preflight.ts | 6 +- .../src/pages/settings/SettingsField.tsx | 5 +- .../src/pages/settings/SettingsHub.tsx | 1 + .../src/pages/settings/SettingsView.tsx | 5 +- .../src/pages/system/ApprovalsInboxPage.tsx | 137 ++++++++++++------ apps/console/src/sdui-workbench-preview.tsx | 2 +- apps/console/src/utils/getIcon.ts | 8 + packages/runner/package.json | 1 + packages/runner/src/LayoutRenderer.tsx | 16 +- scripts/check-lint-coverage.mjs | 18 +-- 12 files changed, 185 insertions(+), 59 deletions(-) create mode 100644 .changeset/lint-console-runner-react-correctness.md diff --git a/.changeset/lint-console-runner-react-correctness.md b/.changeset/lint-console-runner-react-correctness.md new file mode 100644 index 000000000..03ec3917c --- /dev/null +++ b/.changeset/lint-console-runner-react-correctness.md @@ -0,0 +1,44 @@ +--- +"@object-ui/console": patch +"@object-ui/runner": patch +--- + +fix(console,runner): the approvals inbox renders against one ticking clock, and both packages now run ESLint + +`apps/console` and `packages/runner` had no `lint` script, so `turbo run lint` +skipped them silently and their 17 ESLint **errors** had never been seen +(#2923 declared them as DEBT; this closes the gap). Both now carry +`"lint": "eslint ."` and the `DEBT` list in `scripts/check-lint-coverage.mjs` +is empty — every workspace package is linted. + +What the errors actually were, once read one by one: + +- **8x `react-hooks/purity` — real, and user-visible.** The approvals inbox + read `Date.now()` mid-render for every age tint, "5m ago" label and SLA chip. + Render must be pure: the output depended on when React happened to render, so + it disagreed with itself under StrictMode's double render and then **froze** — + an inbox left open kept saying "just now" and an SLA countdown never counted + down. The page now renders against a single `now` held in state and advanced + once a minute (the finest granularity anything here displays), so render is a + pure function of props+state *and* the figures actually tick. +- Alongside that, `sla_due_at` is now parsed through a guard. A due date the + backend sends in a shape `Date.parse` can't read used to render as + "SLA NaNh left"; it now renders nothing. +- **1x `react-hooks/static-components` — real.** `StatusBadge` was declared + inside `ApprovalsInboxPage`, making it a brand-new component type on every + render, so React unmounted and remounted every status chip in the table each + time the page re-rendered. Hoisted to module scope, with the translated label + passed as a prop. +- **6x `react-hooks/static-components` — false positives** (3 in the console's + settings pages, 3 in the runner's `LayoutRenderer`). All six render the result + of `getIcon`/`getLazyIcon`, which memoises per name in a module-level cache — + the component reference is stable across renders and nothing is created during + render. The rule cannot see through the call, so these carry the same targeted + `eslint-disable-next-line` + justification the repo already uses at a dozen + icon-registry sites, and the resolvers themselves now say so in a comment. + (Verified rather than assumed: typing into a settings field keeps focus and + every character, so no state was ever being reset there.) +- **2 minor.** A dead `token` initializer on the console's auth preflight path + (`no-useless-assignment` — read, not blind-deleted: no intended write was + missing, every path out of the try/catch either assigns or returns), and a + `prefer-const` in the SDUI workbench preview. diff --git a/apps/console/package.json b/apps/console/package.json index 2abfc75e9..d1350e510 100644 --- a/apps/console/package.json +++ b/apps/console/package.json @@ -46,6 +46,7 @@ "build:plugin": "tsc -p tsconfig.plugin.json", "build": "tsc && vite build && pnpm build:plugin", "type-check": "tsc --noEmit", + "lint": "eslint .", "build:analyze": "pnpm build && echo 'Bundle analysis available at dist/stats.html'", "preview": "vite preview", "test": "vitest run", diff --git a/apps/console/src/lib/auth-preflight.ts b/apps/console/src/lib/auth-preflight.ts index 51c4553ef..5dfa86de8 100644 --- a/apps/console/src/lib/auth-preflight.ts +++ b/apps/console/src/lib/auth-preflight.ts @@ -36,10 +36,14 @@ const ACTIVE_ORG_KEY = 'auth-active-organization-id'; export async function preflightAuth(authBaseUrl: string): Promise { if (typeof window === 'undefined') return; - let token: string | null = null; + // No initializer: every path out of the try/catch below either assigns + // `token` or returns, so a `= null` seed would be dead (no-useless-assignment). + let token: string | null; try { token = localStorage.getItem(TOKEN_KEY); } catch { + // Storage blocked (Safari private mode / partitioned iframe) — nothing to + // validate, and nothing we could purge either. return; } if (!token) return; diff --git a/apps/console/src/pages/settings/SettingsField.tsx b/apps/console/src/pages/settings/SettingsField.tsx index eada45df8..b108c5c59 100644 --- a/apps/console/src/pages/settings/SettingsField.tsx +++ b/apps/console/src/pages/settings/SettingsField.tsx @@ -219,7 +219,10 @@ export function SettingsField(props: SettingsFieldProps) { ) : null} diff --git a/apps/console/src/pages/settings/SettingsHub.tsx b/apps/console/src/pages/settings/SettingsHub.tsx index e116a6af4..4b1a5bac8 100644 --- a/apps/console/src/pages/settings/SettingsHub.tsx +++ b/apps/console/src/pages/settings/SettingsHub.tsx @@ -43,6 +43,7 @@ function SettingCard({ m, onOpen }: { m: SettingsManifest; onOpen: () => void }) >
+ {/* eslint-disable-next-line react-hooks/static-components -- getIcon returns a module-cached stable component per name, not one created during render */} {m.beta ? ( diff --git a/apps/console/src/pages/settings/SettingsView.tsx b/apps/console/src/pages/settings/SettingsView.tsx index 233bb0e90..16116dcbd 100644 --- a/apps/console/src/pages/settings/SettingsView.tsx +++ b/apps/console/src/pages/settings/SettingsView.tsx @@ -158,7 +158,10 @@ export function SettingsView() {
- {Icon ? : null} + {Icon ? ( + // eslint-disable-next-line react-hooks/static-components -- getIcon returns a module-cached stable component per name, not one created during render + + ) : null}

{title}

diff --git a/apps/console/src/pages/system/ApprovalsInboxPage.tsx b/apps/console/src/pages/system/ApprovalsInboxPage.tsx index c9e344f77..01dc8bf1f 100644 --- a/apps/console/src/pages/system/ApprovalsInboxPage.tsx +++ b/apps/console/src/pages/system/ApprovalsInboxPage.tsx @@ -121,6 +121,21 @@ const STATUS_CLASSES: Record = { returned: 'border-violet-200 bg-violet-50 text-violet-700 dark:border-violet-500/30 dark:bg-violet-500/10 dark:text-violet-400', }; +/** + * Status chip. Module scope on purpose: declared inside `ApprovalsInboxPage` + * it was a brand-new component type on every render, so React unmounted and + * remounted every badge in the table each time the page re-rendered + * (`react-hooks/static-components`). The translated label is passed in + * because `statusLabel` closes over the page's `t`. + */ +function StatusBadge({ status, label }: { status: string; label: string }) { + return ( + + {label} + + ); +} + function formatDate(s: string | null | undefined): string { if (!s) return '—'; try { return new Date(s).toLocaleString(); } catch { return s; } @@ -214,19 +229,24 @@ function submittedAt(r: ApprovalRequestRow): string | undefined { return r.submitted_at || r.created_at || undefined; } -/** Hours a pending request has been waiting; null when no timestamp. */ -function waitingHours(r: ApprovalRequestRow): number | null { +/** + * Hours a pending request has been waiting at instant `now`; null when no + * timestamp. `now` is a parameter rather than a `Date.now()` read so callers + * on the render path stay pure (`react-hooks/purity`) — see the `now` state in + * {@link ApprovalsInboxPage}. + */ +function waitingHours(r: ApprovalRequestRow, now: number): number | null { const s = submittedAt(r); if (!s) return null; const t = Date.parse(s); if (Number.isNaN(t)) return null; - return (Date.now() - t) / 36e5; + return (now - t) / 36e5; } /** Aging tint: quiet under a day, amber 1–3 days, red beyond 3 days. */ -function agingClass(r: ApprovalRequestRow): string { +function agingClass(r: ApprovalRequestRow, now: number): string { if (r.status !== 'pending') return 'text-muted-foreground'; - const h = waitingHours(r); + const h = waitingHours(r, now); if (h == null) return 'text-muted-foreground'; if (h > 72) return 'text-red-600 dark:text-red-400 font-medium'; if (h > 24) return 'text-amber-600 dark:text-amber-400'; @@ -239,6 +259,19 @@ function compactDuration(ms: number): string { return h < 48 ? `${h}h` : `${Math.round(h / 24)}d`; } +/** + * SLA position at instant `now`, or null when there is nothing to show — no + * due date, or one the backend sent in a shape `Date.parse` can't read (which + * previously rendered as "SLA NaNh left"). Takes `now` for the same purity + * reason as {@link waitingHours}. + */ +function slaState(dueAt: string | null | undefined, now: number): { overdue: boolean; ms: number } | null { + if (!dueAt) return null; + const due = Date.parse(dueAt); + if (Number.isNaN(due)) return null; + return { overdue: due < now, ms: Math.abs(now - due) }; +} + const PAYLOAD_SYSTEM_KEYS = new Set([ 'id', 'created_at', 'updated_at', 'created_by', 'updated_by', 'organization_id', ]); @@ -354,12 +387,31 @@ export function ApprovalsInboxPage() { [t], ); + /** + * The one clock this page renders against. + * + * Every age/SLA figure below used to call `Date.now()` mid-render, which is + * impure (`react-hooks/purity`): the output depended on when React happened + * to render, so it disagreed with itself under StrictMode's double render and + * froze between renders — an inbox left open showed "just now" indefinitely. + * Holding the instant in state and advancing it on a timer makes render a + * pure function of props+state *and* makes the countdowns actually tick. + * + * A minute is the finest granularity anything here displays ("5m ago", + * "36h", "3d"), so that is the tick rate. + */ + const [now, setNow] = useState(() => Date.now()); + useEffect(() => { + const id = setInterval(() => setNow(Date.now()), 60_000); + return () => clearInterval(id); + }, []); + /** Localized relative time, e.g. "5m ago" / "5 分钟前". */ const formatRelative = useCallback((s: string | null | undefined): string => { if (!s) return '—'; const ts = Date.parse(s); if (Number.isNaN(ts)) return s; - const sec = Math.round((Date.now() - ts) / 1000); + const sec = Math.round((now - ts) / 1000); if (sec < 45) return tr('justNow', 'just now'); const min = Math.round(sec / 60); if (min < 60) return tr('minutesAgo', '{{count}}m ago', { count: min }); @@ -368,7 +420,7 @@ export function ApprovalsInboxPage() { const day = Math.round(hr / 24); if (day < 30) return tr('daysAgo', '{{count}}d ago', { count: day }); try { return new Date(s).toLocaleDateString(language); } catch { return s; } - }, [tr, language]); + }, [tr, language, now]); const statusLabel = useCallback((status: string): string => { switch (status) { @@ -397,13 +449,14 @@ export function ApprovalsInboxPage() { return err?.message || fallback; }, [tr]); - function StatusBadge({ status }: { status: string }) { - return ( - - {statusLabel(status)} - - ); - } + /** SLA chip copy for a {@link slaState} result. */ + const slaLabel = useCallback( + (sla: { overdue: boolean; ms: number }): string => + sla.overdue + ? tr('slaOverdue', 'SLA overdue {{dur}}', { dur: compactDuration(sla.ms) }) + : tr('slaRemaining', 'SLA {{dur}} left', { dur: compactDuration(sla.ms) }), + [tr], + ); const recordHref = useCallback((r: ApprovalRequestRow): string => { const app = appName || 'setup'; @@ -1387,23 +1440,24 @@ export function ApprovalsInboxPage() {
)} - + {formatRelative(submittedAt(r))} - {r.status === 'pending' && r.sla_due_at && ( -
- {Date.parse(r.sla_due_at) < Date.now() - ? tr('slaOverdue', 'SLA overdue {{dur}}', { dur: compactDuration(Date.now() - Date.parse(r.sla_due_at)) }) - : tr('slaRemaining', 'SLA {{dur}} left', { dur: compactDuration(Date.parse(r.sla_due_at) - Date.now()) })} -
- )} + {r.status === 'pending' && (() => { + const sla = slaState(r.sla_due_at, now); + return sla ? ( +
+ {slaLabel(sla)} +
+ ) : null; + })()} @@ -1420,7 +1474,7 @@ export function ApprovalsInboxPage() {
{processLabel(r)}
- +
{r.record_title || formatIdentity(r.record_id)} @@ -1442,7 +1496,7 @@ export function ApprovalsInboxPage() { {submitterDisplay(r)} )} - + {formatRelative(submittedAt(r))}
@@ -1585,7 +1639,7 @@ export function ApprovalsInboxPage() {
{/* Status strip */}
- + {(selected.round ?? 1) > 1 && ( {tr('roundChip', 'Round {{n}}', { n: selected.round })} @@ -1598,18 +1652,19 @@ export function ApprovalsInboxPage() { {selected.completed_at && ( · {tr('completedAt', 'Completed {{when}}', { when: formatRelative(selected.completed_at) })} )} - {selected.status === 'pending' && selected.sla_due_at && ( - - {Date.parse(selected.sla_due_at) < Date.now() - ? tr('slaOverdue', 'SLA overdue {{dur}}', { dur: compactDuration(Date.now() - Date.parse(selected.sla_due_at)) }) - : tr('slaRemaining', 'SLA {{dur}} left', { dur: compactDuration(Date.parse(selected.sla_due_at) - Date.now()) })} - - )} + {selected.status === 'pending' && (() => { + const sla = slaState(selected.sla_due_at, now); + return sla ? ( + + {slaLabel(sla)} + + ) : null; + })()}
{/* Business summary card */} diff --git a/apps/console/src/sdui-workbench-preview.tsx b/apps/console/src/sdui-workbench-preview.tsx index 0d67f5413..346a031d9 100644 --- a/apps/console/src/sdui-workbench-preview.tsx +++ b/apps/console/src/sdui-workbench-preview.tsx @@ -22,7 +22,7 @@ if (new URLSearchParams(location.search).has('disable')) { } // ---- in-memory adapter (just enough for ListView + ObjectForm) ---- -let store: any[] = [ +const store: any[] = [ { id: '1', name: 'Apollo Migration', status: 'active', health: 'green', budget: 120000, owner: 'Dana Lee' }, { id: '2', name: 'Billing Revamp', status: 'planned', health: 'yellow', budget: 80000, owner: 'Sam Ortiz' }, { id: '3', name: 'Mobile App v2', status: 'active', health: 'red', budget: 210000, owner: 'Priya N.' }, diff --git a/apps/console/src/utils/getIcon.ts b/apps/console/src/utils/getIcon.ts index 5e26f2376..5ecf90624 100644 --- a/apps/console/src/utils/getIcon.ts +++ b/apps/console/src/utils/getIcon.ts @@ -20,6 +20,14 @@ function toKebab(name: string): string { const cache = new Map(); +/** + * Resolve a Lucide icon component by name. + * + * The result is memoised per name in the module-level `cache`, so call sites + * get a *stable* component reference across renders — nothing is created during + * render. `react-hooks/static-components` cannot see through the call, so the + * JSX sites that render the result carry a targeted disable pointing back here. + */ export function getIcon(name?: string): React.ElementType { if (!name) return Database; const cached = cache.get(name); diff --git a/packages/runner/package.json b/packages/runner/package.json index 9c8d80312..7c9649d77 100644 --- a/packages/runner/package.json +++ b/packages/runner/package.json @@ -17,6 +17,7 @@ "dev": "vite", "build": "vite build", "type-check": "tsc --noEmit", + "lint": "eslint .", "preview": "vite preview", "test": "vitest run" }, diff --git a/packages/runner/src/LayoutRenderer.tsx b/packages/runner/src/LayoutRenderer.tsx index 6e41eb472..24f5f6d83 100644 --- a/packages/runner/src/LayoutRenderer.tsx +++ b/packages/runner/src/LayoutRenderer.tsx @@ -45,6 +45,11 @@ interface LayoutRendererProps { // Helper to resolve icon from string name (e.g. "bar-chart" -> "BarChart") // Delegates to the shared lazy icon resolver so we don't ship the entire // lucide-react namespace for the few icons rendered by user schemas. +// +// `getLazyIcon` memoises per name in a module-level cache, so the component +// this returns is a stable reference across renders — not a component created +// during render. `react-hooks/static-components` cannot see through the call, +// hence the targeted disables at the JSX sites below. const getIcon = (name?: string) => { if (!name) return null; return getLazyIcon(name); @@ -66,7 +71,10 @@ const NavItem = ({ item, currentPath, isSidebarOpen, onNavigate, level = 0 }: an
- {Icon && } + {Icon && ( + // eslint-disable-next-line react-hooks/static-components -- getLazyIcon returns a module-cached stable component per name, not one created during render + + )} {item.label} @@ -102,7 +110,10 @@ const NavItem = ({ item, currentPath, isSidebarOpen, onNavigate, level = 0 }: an : 'text-muted-foreground hover:bg-muted hover:text-foreground' } ${isSidebarOpen ? 'px-3' : 'justify-center px-2'} ${level > 0 && isSidebarOpen ? 'pl-10' : ''}`} > - {Icon && } + {Icon && ( + // eslint-disable-next-line react-hooks/static-components -- getLazyIcon returns a module-cached stable component per name, not one created during render + + )} {item.label} @@ -164,6 +175,7 @@ export const LayoutRenderer = ({ app, children, currentPath, onNavigate }: Layou >
{LogoIcon ? ( + // eslint-disable-next-line react-hooks/static-components -- getLazyIcon returns a module-cached stable component per name, not one created during render ) : app.logo ? ( {app.title} diff --git a/scripts/check-lint-coverage.mjs b/scripts/check-lint-coverage.mjs index 5d36b6d1e..a4b64ff52 100644 --- a/scripts/check-lint-coverage.mjs +++ b/scripts/check-lint-coverage.mjs @@ -31,18 +31,12 @@ const root = resolve(dirname(fileURLToPath(import.meta.url)), ".."); // // Every entry is real debt: fix the errors, add `"lint": "eslint ."`, then // delete the entry. -const DEBT = { - "@object-ui/console": { - errors: 14, - issue: 2927, - note: "8x react-hooks/purity (Date.now during render) + 4x react-hooks/static-components (components created during render reset state) + no-useless-assignment + prefer-const", - }, - "@object-ui/runner": { - errors: 3, - issue: 2927, - note: "3x react-hooks/static-components in LayoutRenderer", - }, -}; +// +// Empty since #2927 closed the last two (`@object-ui/console`, 14 errors, and +// `@object-ui/runner`, 3). Every workspace package now runs ESLint. Adding an +// entry here is a deliberate admission that a package ships known errors — do +// it with an issue number, and treat it as temporary. +const DEBT = {}; // ── Collect workspace packages ─────────────────────────────────────────────── const GROUPS = ["packages", "apps", "examples"];