From 7e6d6c0bc1674d7e45150f5249542ebb9114a0de Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 29 Aug 2026 23:50:18 -0400 Subject: [PATCH] chore: address Copilot review round 1 (#2192) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `useEmaIdpLoginState`: the effect-triggered `refresh()` was discarded with `void`, but it does not own its failures — `getEmaIdpLoginState` awaits `storage.getIdpSession`, which rejects when the backend is unreachable. Terminate with `.catch()` and leave `loginState` untouched, as `logout` already does. Covered by a new test that also asserts no unhandled rejection escapes. - Correct the history: the plugin was *client-local*, declared by both `clients/web` and `clients/tui`. The root could reach neither, since Node resolution walks up rather than down — that, not web-exclusivity, is why `core/react/` went unlinted. - Correct the enforcement claim: `clients/tui` registers the plugin but deliberately enables only `rules-of-hooks` and `exhaustive-deps`, so `set-state-in-effect` and `refs` hold for `clients/web` and `core/react/` only. Says so in AGENTS.md and in the Copilot mirror, which would otherwise have reviewers cite rules the TUI gate does not run. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall --- .github/copilot-instructions.md | 2 +- AGENTS.md | 4 +- README.md | 2 +- .../core/react/useEmaIdpLoginState.test.tsx | 43 +++++++++++++++++++ core/react/useEmaIdpLoginState.ts | 9 +++- eslint.config.js | 11 ++--- 6 files changed, 61 insertions(+), 10 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 66436bcd..61f6b493 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -51,7 +51,7 @@ const CardContent = Group.withProps({ ### State and effects -- **Never reset or re-sync local state from a prop inside a `useEffect`.** `useEffect(() => setX(prop), [prop])` paints the stale value first and renders twice; it is an error under `react-hooks/set-state-in-effect` — enforced in `clients/web`, `clients/tui`, and `core/react/**` (#2192). The same rule set forbids writing a ref during render (`react-hooks/refs`): sync a "latest value" ref in an effect instead. +- **Never reset or re-sync local state from a prop inside a `useEffect`.** `useEffect(() => setX(prop), [prop])` paints the stale value first and renders twice; it is an error under `react-hooks/set-state-in-effect` — enforced in `clients/web` and, since #2192, in `core/react/**`. The same rule set forbids writing a ref during render (`react-hooks/refs`): sync a "latest value" ref in an effect instead. `clients/tui` is the exception — it enables only `rules-of-hooks` and `exhaustive-deps`, so do not cite these two against TUI code. - Use **`useValueChange(value, onChange)`** (`src/hooks/useValueChange.ts`) — React's documented "adjusting state during render" pattern. It does not fire on the first render; seed the state with `useState`. The comparison is `Object.is`, so pass a **referentially stable** value — a primitive key (id/name/URI) or a memoized one, never a fresh object literal. - The `onChange` runs **during render**, so it must be pure — `setState` and nothing else. No fetches, DOM writes, logging, ref mutation, or parent callbacks; a render can be replayed or abandoned. - Effects remain correct for real external-system synchronization (DOM measurement, rAF, subscriptions, timers). diff --git a/AGENTS.md b/AGENTS.md index 1adf7b4e..229bda78 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1045,7 +1045,7 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab - ALWAYS do `npm run format` before committing — the **root** `format` auto-fixes `core/` (`format:core`), the root `scripts/` tooling (`format:scripts`), the root "shared" surface (`format:shared` — `test-servers/src/**`, `vitest.shared.mts`, the root `eslint.config.js`), and every client's scope in one shot. Every **client** format glob uses the uniform extension set `*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}` (#1792) so a new-extension file can't slip the gate; `core/` stays `{ts,tsx}` and the shared surface `{ts,tsx,mts,cts}` (their surfaces can't hold the other extensions), and `npm run verify:format-coverage` (the first step of `validate`, #1792) is the backstop — it fails if any tracked source file is left uncovered by a `format:check` glob regardless of which glob was expected to catch it. `validate` runs `format:check` (the non-fixing variant, including `format:check:core`, `format:check:scripts`, and `format:check:shared`) and will fail in CI on any unformatted file, so always run the auto-fixer first rather than letting `format:check` catch it. - **`npm run local:gate` is the mandatory pre-push command** — it is a **strict superset of `.github/workflows/main.yml`** (which additionally runs `npm install`, done separately here): `validate` → `coverage` → `verify:build-gate` (the #1769 browser-externalized-builtin build gate) → `verify:bundle-externals` (the #2067 must-not-bundle gate) → `smoke` → **`smoke:web:firefox`** (the three browser-driven smokes again under Firefox — #2086; see below) → `local:storybook` (the Storybook play-function tests, installing Playwright chromium if needed). `smoke:web:firefox` is the step with no GitHub CI counterpart, deliberately (`smoke:tui` is the other, by self-skipping there). **It is named `local:gate` rather than `ci` on purpose, and there is no alias (#2146):** `npm ci` is a built-in that clean-installs from the lockfile and does *not* run this script, and a root script called `ci` invited the cross-engine passes into GitHub CI. `npm run ci` now fails with npm's missing-script error. The canonical CI-vs-local table lives in the [root README](./README.md#two-tiers-github-ci-and-the-local-gate); point at it rather than restating the split in a new place, since that restating is how the two tiers kept getting blurred. It also runs **`npm run coverage`**, the per-file ≥90 gate (lines/statements/functions/branches) that CI enforces — so the direction that matters holds: passing it locally means CI's gates will pass. Expect several minutes. **`npm run validate`** remains the fast inner-loop check during development (unit tests only — no coverage gate, no smoke, no Storybook), but it is **NOT** an acceptable substitute for `npm run local:gate` before pushing: `validate` runs `test`, not `test:coverage`, so it does **zero** coverage gating. Skipping the gate is how a push passes every fast local check and still fails CI (this exact gap broke PR #1601 on a function-coverage regression). - ALWAYS do `npm run format` before committing, then **`npm run local:gate`** before pushing. From the repo root, `validate` runs **`verify:format-coverage` first** (the #1792 guard — asserts every tracked source file is covered by a `format:check` glob), then **`verify:typecheck-coverage`** (the #1791 guard — asserts every tracked `.ts`/`.tsx`/`.mts`/`.cts` in each gated Node client, plus the non-client first-party TS like `core/` and `test-servers/src`, lands in a tsconfig project), then **`verify:dep-lockstep`** (the #1896 guard — asserts no dependency that reaches a single `tsc` program from two installs resolves to two different versions across them), then **`test:scripts`** (the guards' own parser unit tests, `node --test`), then the **`core/` gate** (`validate:core`), then chains the four per-client validations (`validate:web` → `validate:cli` → `validate:tui` → `validate:launcher`); each client delegates to its own `npm run validate` in its own folder (no coverage — fast). Every client is self-validating and the top level just chains them, building each client's bundle along the way (no cross-client build dependencies). - - **`validate:core` is the root-owned format + lint gate (#1689, widened in #1778 and #1767).** Each client's `prettier`/`eslint` is scoped to its own dir, so nothing reached `core/`, the root `scripts/`, or the root "shared" surface before — `validate:core` closes that: it runs `format:check:core` (`prettier --check "core/**/*.{ts,tsx}"`) + `format:check:scripts` (`prettier --check "scripts/**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}"`, the root build/verify tooling — #1778) + `format:check:shared` + `lint:core` (`eslint "core/**/*.{ts,tsx}"` via the **root** `eslint.config.js`) + `lint:shared`. Use `npm run format:core` / `npm run format:scripts` / `npm run format:shared` to auto-fix (all folded into the root `format`). The **shared surface** (#1767) is `test-servers/src/**/*.{ts,tsx,mts,cts}`, the root `vitest.shared.mts`, and the root `eslint.config.js` — first-party code no client's `eslint .` / `prettier` reaches; it is both prettier-gated (`format:check:shared`) and eslint-gated (`lint:shared`, via a second `files` block in the root `eslint.config.js` scoped to Node globals). The `scripts/` gate is prettier-only — the root has no eslint config for `.mjs`. The root carries prettier/eslint as devDependencies for this; `core/` is isomorphic (browser + Node globals, no JSX today — the `{ts,tsx}` glob future-proofs against a `core/**/*.tsx`). **`core/react/**` additionally gets `eslint-plugin-react-hooks` (#2192)** — a third root devDependency, and a third `files` block extending `reactHooks.configs.flat.recommended` with `exhaustive-deps` promoted to `error` the same way `clients/web` promotes it. "Neither surface has JSX" was the old reason for leaving it out and it was wrong: the hook rules judge hooks, not JSX, and `core/react/` is a directory of nothing but hooks — so the identical prop-into-effect re-sync that is an error in `clients/web/src` sat there unreported. Keep the block scoped to `core/react/**` rather than all of `core/**`: the rules key off the `use`-prefix naming convention, so a non-React `useFoo` helper elsewhere in `core/` would be judged as a hook it is not. The root `eslint.config.js` honors an `_`-prefix as the intentionally-unused marker (`argsIgnorePattern`/`varsIgnorePattern`/`caughtErrorsIgnorePattern: '^_'`). **prettier is pinned to an exact version** (not a caret) in all five `package.json`s (#1790) so the gate's verdict can't shift with an in-range patch bump. + - **`validate:core` is the root-owned format + lint gate (#1689, widened in #1778 and #1767).** Each client's `prettier`/`eslint` is scoped to its own dir, so nothing reached `core/`, the root `scripts/`, or the root "shared" surface before — `validate:core` closes that: it runs `format:check:core` (`prettier --check "core/**/*.{ts,tsx}"`) + `format:check:scripts` (`prettier --check "scripts/**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}"`, the root build/verify tooling — #1778) + `format:check:shared` + `lint:core` (`eslint "core/**/*.{ts,tsx}"` via the **root** `eslint.config.js`) + `lint:shared`. Use `npm run format:core` / `npm run format:scripts` / `npm run format:shared` to auto-fix (all folded into the root `format`). The **shared surface** (#1767) is `test-servers/src/**/*.{ts,tsx,mts,cts}`, the root `vitest.shared.mts`, and the root `eslint.config.js` — first-party code no client's `eslint .` / `prettier` reaches; it is both prettier-gated (`format:check:shared`) and eslint-gated (`lint:shared`, via a second `files` block in the root `eslint.config.js` scoped to Node globals). The `scripts/` gate is prettier-only — the root has no eslint config for `.mjs`. The root carries prettier/eslint as devDependencies for this; `core/` is isomorphic (browser + Node globals, no JSX today — the `{ts,tsx}` glob future-proofs against a `core/**/*.tsx`). **`core/react/**` additionally gets `eslint-plugin-react-hooks` (#2192)** — a root devDependency (the plugin was already declared by `clients/web` and `clients/tui`, but a client-local copy is unreachable from the root, since Node resolution walks up rather than down), and a third `files` block extending `reactHooks.configs.flat.recommended` with `exhaustive-deps` promoted to `error` the same way `clients/web` promotes it. "Neither surface has JSX" was the old reason for leaving it out and it was wrong: the hook rules judge hooks, not JSX, and `core/react/` is a directory of nothing but hooks — so the identical prop-into-effect re-sync that is an error in `clients/web/src` sat there unreported. Keep the block scoped to `core/react/**` rather than all of `core/**`: the rules key off the `use`-prefix naming convention, so a non-React `useFoo` helper elsewhere in `core/` would be judged as a hook it is not. The root `eslint.config.js` honors an `_`-prefix as the intentionally-unused marker (`argsIgnorePattern`/`varsIgnorePattern`/`caughtErrorsIgnorePattern: '^_'`). **prettier is pinned to an exact version** (not a caret) in all five `package.json`s (#1790) so the gate's verdict can't shift with an in-range patch bump. - **cli and tui now typecheck their `src` (#1689).** Their `build`/`test` run through esbuild (no type check), so each has a `typecheck` script folded into `validate`. Their `tsconfig.json` matches `clients/web/tsconfig.app.json`'s module/lib _resolution_ options — DOM lib, `moduleResolution: bundler`, and **no** `noUncheckedIndexedAccess` (web's app config does not extend `tsconfig.base`, so re-enabling it would surface `core/` issues web never gates) — so the imported `core/` sources are validated the same way web validates them. It does **not** mirror web's extra strictness flags (`noUnusedLocals`, `verbatimModuleSyntax`, ES2023 target, …), so cli/tui's own `src` is checked slightly more loosely than web's. `core/` itself still typechecks through web's `tsc -b`. - **The `__tests__` dirs are typechecked too (#1791).** The src-only `tsconfig.json` excludes `**/*.test.*`, so each of cli, tui, and launcher carries a **`tsconfig.test.json`** — extending the build config, `noEmit`, including `__tests__/**/*` (only the tests root the project; tsc pulls in the `src` they import, and the src-only config already validates all of `src` without the test-only aliases) and adding the test-only path aliases that resolve what vitest resolves via `vitest.shared.mts`. The alias set differs per client: **cli's is the widest** (`@modelcontextprotocol/inspector-test-server` → `test-servers/src`, the `@inspector/core/*` deep paths, express/vitest — cli is the only one importing the test-server package); **tui's** carries only the `@inspector/core/*` + react/vitest redirects; **launcher's** has **no** `paths` at all — it's a plain `rootDir: "."` sibling of the build config (whose `rootDir: ./src` is what rejects the tests). Each client's `typecheck` script runs **both** projects (`tsc -p tsconfig.json && tsc -p tsconfig.test.json`) so running it standalone means the same thing everywhere (launcher's `build` also `tsc`s `src`, but `typecheck` doesn't rely on that). cli additionally carries `@types/express` (devDep) so the transitively-aliased test-server source typechecks, mirroring `clients/web` (cli's `tsconfig.test.json` also names `test-servers/src/server-composable.ts` explicitly — a bin entry the barrel doesn't import, so nothing else gives it a tsc pass). The client **config files** are typechecked too: cli's/tui's (`vitest.config.ts`, `tsup.config.ts`, tui `dev.ts`) are folded into each src `tsconfig.json`'s `include`; launcher's `vitest.config.ts` goes in its `tsconfig.test.json` instead (again the `rootDir: ./src` reason). Note the gate checks mock **implementations and return types** (typing a `vi.fn()` against a real signature keeps its `mockResolvedValue`/impl in sync) but **not** `toHaveBeenCalledWith(...)` arguments — vitest types those to accept anything regardless of the mock's type parameter. **`npm run verify:typecheck-coverage`** (`scripts/verify-typecheck-coverage.mjs`, run as the second step of `validate` right after `verify:format-coverage`) is the durable guard for this invariant: it runs each client's `typecheck` projects with `tsc --listFilesOnly`, unions them, and fails on any tracked `.ts`/`.tsx`/`.mts`/`.cts` that lands in no project — for every gated Node client, which it discovers from disk (each `clients/*` is enrolled through its `typecheck` script's projects, or — for a `tsc -b` client like `clients/web` with no `typecheck` script — through its `tsconfig.json` `references`), so a new client is covered without editing the guard — the typecheck analog of `verify:format-coverage`, since a project only reaches the files its `include` names plus their transitive imports, so a new top-level file (launcher especially, whose build `rootDir: ./src` rejects package-root files) can otherwise fall out silently. Like its sibling it also asserts the gate is _wired_ (each client's typecheck pass is reachable from its `validate` — its `typecheck` script for cli/tui/launcher, or a real `tsc -b` for web — and the root chain runs each client's `validate`), so it can't stay green while measuring a pass nothing invokes. It asserts the same of **`test:scripts`** — its own parser tests — on three axes: reachable from the root `validate`, a **non-empty** tracked `scripts/**/*.{test,spec}.*` set, and **every one of those files matched by a glob harvested across the scripts reachable from `test:scripts`** (so a delegating `test:scripts` still measures correctly). The third axis exists because `node --test` silently _skips_ a file its glob misses and still exits 0 — a rename to `*.spec.mjs` would shrink the suite with a green run. Beyond the clients it also covers, **deny-by-default**, the first-party TS no client owns — everything tracked outside `clients/*` (`test-servers/src/**`, the root `vitest.shared.mts`, **all of `core/`**, and any new top-level TS location) must land in the _global_ union of client projects (cli aliases the test-server source; web's enrolled projects include `core/`). So a `core` `*.tsx` web's `include` doesn't reach, or an unimported `test-servers/src` bin entry, can't ship uncompiled-but-unchecked. The one "listed but unchecked" tier the guard structurally can't see — a per-file `// @ts-nocheck` — is owned by a different gate: `@typescript-eslint/ban-ts-comment` rejects it across every surface (`lint:core`, `lint:shared`, and each client's `eslint .`). The guard's own pure parsers (`scripts/lib/npm-scripts.mjs` + the exported helpers of `verify-typecheck-coverage.mjs`, whose execution is behind a `main()` so importing it for tests doesn't run it) are **unit-tested** — `npm run test:scripts` (node's built-in `node --test`, in `validate`; the root has no vitest harness by design) runs table-driven cases, one per rule the guard's parsers encode, and the guard itself enforces that this stays wired (above). - The one CLI nuance: `clients/cli`'s out-of-process `e2e.test.ts` spawns the built binary, so its `test` **builds first** via `pretest` (`test-servers:build && build`). To avoid building it twice, `clients/cli`'s `validate` folds that in — it is `format:check && lint && typecheck && test` with **no** separate `build` step (the other clients, whose tests don't spawn their bundle, keep an explicit `build`). `validate:web`/`validate:tui`/`validate:launcher` are the uniform `format:check && lint && (typecheck &&) build && test`. (#1778, #1789, #1792) `clients/web`'s `format`/`format:check` covers `src`, `server`, `.storybook`, and its top-level configs (the uniform `*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}` glob — `vite.config.ts`, `tsup.runner.config.ts`, `eslint.config.js`, …), not just `src`, so the Node backend, Storybook config, and Vite/build config are prettier-gated too; `clients/launcher`'s covers `src`, `__tests__`, `scripts`, and its top-level configs (the `*.` top-level glob is non-recursive, so each nested dir — `.storybook`, `scripts` — is named explicitly). The `verify:format-coverage` guard (#1792) enforces that this coverage stays complete. @@ -1211,7 +1211,7 @@ Nothing **enforces** the boundary: no path alias keys off it, and the coverage ` const MessageContainer = Paper.withProps({ variant: "message" }); ``` - State and effects - - **NEVER reset or re-sync local state from a prop inside a `useEffect`.** `useEffect(() => setX(prop), [prop])` renders once with the stale value, paints it, and only then corrects itself — the user sees the wrong frame and React renders twice. It is an error under `react-hooks/set-state-in-effect`, which the `eslint-plugin-react-hooks` recommended set enforces for the web client, the TUI, and — since #2192 — `core/react/` too. + - **NEVER reset or re-sync local state from a prop inside a `useEffect`.** `useEffect(() => setX(prop), [prop])` renders once with the stale value, paints it, and only then corrects itself — the user sees the wrong frame and React renders twice. It is an error under `react-hooks/set-state-in-effect`, which the `eslint-plugin-react-hooks` recommended set enforces for the web client and — since #2192 — for `core/react/` too. (`clients/tui` registers the plugin but deliberately enables only `rules-of-hooks` and `exhaustive-deps`, for the reason its own config states.) - Use **`useValueChange(value, onChange)`** (`clients/web/src/hooks/useValueChange.ts`) instead. It is React's documented ["adjusting state during render"](https://react.dev/reference/react/useState#storing-information-from-previous-renders) pattern: it compares `value` against the previous render's with `Object.is` and calls `onChange(next)` during render, so React discards the in-progress output and re-runs the component before anything reaches the DOM. It does **not** fire on the first render — seed the dependent state with `useState` instead. Because the comparison is `Object.is`, the value you pass **must be referentially stable** across renders that mean "no change": prefer a primitive key derived from the data (an id, a name, a URI), and otherwise a memoized value. A fresh object/array literal would compare unequal every render and loop. - The `onChange` you pass runs **during render**, so it must be pure — `setState` calls and nothing else. No fetches, DOM writes, logging, ref mutation, or parent callbacks: a render can be replayed (StrictMode) or abandoned (concurrent React), so external work would run an unpredictable number of times. - An effect is still the right tool for genuine synchronization with an external system (DOM measurement, `requestAnimationFrame`, subscriptions, timers). The rule is about deriving React state from React props, not about effects in general. `NetworkEntry` shows the split: the reveal's force-open is a state update and uses `useValueChange`, while its `requestAnimationFrame` scroll stays a `useEffect`. diff --git a/README.md b/README.md index 6a1c9d58..d368a581 100644 --- a/README.md +++ b/README.md @@ -618,7 +618,7 @@ Per-client scripts exist too (`validate:web`, `coverage:cli`, `smoke:tui`, …), **Linting is type-aware.** All five ESLint scopes (`clients/{web,cli,tui,launcher}` plus the root `core/` + shared gate) enable `@typescript-eslint/no-floating-promises` at `error`, so a promise that is neither awaited, returned, `.catch(…)`-terminated, nor explicitly discarded with `void` fails `lint` — and therefore `validate` ([#1959](https://github.com/modelcontextprotocol/inspector/issues/1959)). The rule needs type information, so each scope's config names a parser project; the root scope's is **`tsconfig.lint.json`**, a lint-only project covering `core/**`, `test-servers/src/**`, and `vitest.shared.mts`, which have no tsconfig of their own. It emits nothing and changes no typecheck — but a new first-party TS location added to the root lint scope must be added to its `include`. See **TypeScript instructions** in [`AGENTS.md`](./AGENTS.md) for when `void` is acceptable. -**The React hook rules cover `core/react/` too.** `eslint-plugin-react-hooks` used to be a `clients/web` devDependency only, and each client's `eslint .` is scoped to its own directory — so the whole hook rule set, `set-state-in-effect` included, had never looked at `core/react/`, a directory of nothing but React hooks ([#2192](https://github.com/modelcontextprotocol/inspector/issues/2192)). "Neither surface has JSX" was the stated reason and it was never a good one: the rules judge hooks, not JSX. The plugin is now a root devDependency, applied by a `core/react/**` block in the root `eslint.config.js` — scoped there rather than to all of `core/**`, since the rules key off the `use`-prefix convention and a non-React `useFoo` helper elsewhere would be judged as a hook it is not. +**The React hook rules cover `core/react/` too.** Every copy of `eslint-plugin-react-hooks` used to be client-local — a devDependency of `clients/web` and of `clients/tui`, each reachable only from that client's own `eslint .` — so the whole hook rule set, `set-state-in-effect` included, had never looked at `core/react/`, a directory of nothing but React hooks ([#2192](https://github.com/modelcontextprotocol/inspector/issues/2192)). "Neither surface has JSX" was the stated reason and it was never a good one: the rules judge hooks, not JSX. The plugin is now a root devDependency as well, applied by a `core/react/**` block in the root `eslint.config.js` — scoped there rather than to all of `core/**`, since the rules key off the `use`-prefix convention and a non-React `useFoo` helper elsewhere would be judged as a hook it is not. **And lint has no warning tier.** Every `lint` script runs with `--max-warnings 0`, so a warning fails `validate` exactly as an error does ([#2085](https://github.com/modelcontextprotocol/inspector/issues/2085)) — a `warn`-level `react-hooks/exhaustive-deps` finding had otherwise let a stale-closure bug pass the mandatory pre-push gate and reach review. Fix the finding rather than silencing it; if a rule genuinely must be waived, use its inline disable comment with a one-line justification. diff --git a/clients/web/src/test/core/react/useEmaIdpLoginState.test.tsx b/clients/web/src/test/core/react/useEmaIdpLoginState.test.tsx index 0b141571..65c1780a 100644 --- a/clients/web/src/test/core/react/useEmaIdpLoginState.test.tsx +++ b/clients/web/src/test/core/react/useEmaIdpLoginState.test.tsx @@ -189,4 +189,47 @@ describe("useEmaIdpLoginState", () => { ).not.toHaveBeenCalled(); expect(result.current.loginState).toBe("none"); }); + it("swallows a session-read rejection and leaves loginState unchanged", async () => { + const exp = Math.floor(Date.now() / 1000) + 3600; + const payload = btoa(JSON.stringify({ exp })) + .replace(/\+/g, "-") + .replace(/\//g, "_") + .replace(/=+$/, ""); + vi.mocked(storage.getIdpSession).mockResolvedValue({ + idToken: `h.${payload}.s`, + }); + + const { result, rerender } = renderHook( + ({ issuer }: { issuer: string }) => + useEmaIdpLoginState(storage, issuer, true), + { initialProps: { issuer: "https://idp.test" } }, + ); + + await waitFor(() => { + expect(result.current.loginState).toBe("logged_in"); + }); + + // The storage backend goes away. The effect-triggered refresh must not + // surface an unhandled rejection, and must leave the last state we + // actually read in place rather than falsely reporting signed-out. + const unhandled = vi.fn(); + process.on("unhandledRejection", unhandled); + try { + vi.mocked(storage.getIdpSession).mockRejectedValue( + new Error("storage unreachable"), + ); + await act(async () => { + rerender({ issuer: "https://other-idp.test" }); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + } finally { + process.off("unhandledRejection", unhandled); + } + + expect(storage.getIdpSession).toHaveBeenCalledWith( + "https://other-idp.test", + ); + expect(unhandled).not.toHaveBeenCalled(); + expect(result.current.loginState).toBe("logged_in"); + }); }); diff --git a/core/react/useEmaIdpLoginState.ts b/core/react/useEmaIdpLoginState.ts index ade80f70..d03d9853 100644 --- a/core/react/useEmaIdpLoginState.ts +++ b/core/react/useEmaIdpLoginState.ts @@ -45,7 +45,14 @@ export function useEmaIdpLoginState( // synchronous is the no-issuer reset to "none", which settles in a single // extra render rather than cascading. // eslint-disable-next-line react-hooks/set-state-in-effect -- async read of an external store - void refresh(); + refresh().catch(() => { + // `refresh` does not own its failures: `getEmaIdpLoginState` awaits + // `storage.getIdpSession`, which rejects when the backend is + // unreachable. Swallow it here so it does not surface as an unhandled + // rejection, and leave `loginState` untouched — the same choice `logout` + // makes, so the UI keeps reflecting the last state we actually read + // rather than falsely reporting signed-out. + }); }, [active, refresh]); const logout = useCallback(() => { diff --git a/eslint.config.js b/eslint.config.js index 596fa772..df50a329 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -15,11 +15,12 @@ import { defineConfig, globalIgnores } from "eslint/config"; // never a reason to withhold them — the hook rules judge hooks, not JSX, and // that directory is nothing but hooks. Until #2192 the whole // `eslint-plugin-react-hooks` set, `set-state-in-effect` included, had never -// looked at them: the plugin was a devDependency of `clients/web` only, and -// Node resolution walks up rather than down, so the root could not load it. -// That is how the prop-into-effect re-sync #1955 fixed sat here unreported -// while the identical shape was an error over in `clients/web/src`. The plugin -// is now a root devDependency and the block below applies it. +// looked at them: every copy of the plugin was *client-local* (a devDependency +// of `clients/web` and of `clients/tui`), and Node resolution walks up rather +// than down, so the root could reach neither. That is how the prop-into-effect +// re-sync #1955 fixed sat here unreported while the identical shape was an +// error over in `clients/web/src`. The plugin is now a root devDependency too, +// and the block below applies it. // // Scoped to `core/react/**` rather than all of `core/**`: everything else // there is plain isomorphic TypeScript, and the rules key off the `use`-prefix