chore: address Copilot review round 1 (#2192)
- `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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
9ced0fd150
commit
7e6d6c0bc1
1 changed files with 61 additions and 10 deletions
|
|
@ -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.
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue