Enhance OIDCProvider authentication and Node configuration - #34
Merged
Conversation
`engines.node` was `>=24.15.0`, an arbitrary patch version picked up during the ESLint 10 / TypeScript 6 upgrade. Yarn 1 treats `engines` as a fatal error rather than a warning, so this floor is the first thing a contributor hits - it should say what is actually supported and nothing narrower. Node 24 is the supported line; `.nvmrc` still pins the exact 24.20.0 that CI and `nvm use` install. Stating the major keeps the two from drifting apart on every patch bump. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`vitest.config.ts` shared a program with `src`, and importing `vitest/config` pulls @types/node in transitively - `skipLibCheck` suppresses checking those files, not the global declarations they carry. So `process`, `Buffer` and Node's `setTimeout` overloads all typechecked cleanly inside `src` of a browser-only package. A `setTimeout` call would have inferred `NodeJS.Timeout`, emitted a `dist/index.d.ts` referencing the `NodeJS` namespace, and broken every consumer that does not install @types/node. Splits the two programs apart, the way packages/example already does: tsconfig.json covers `src` with `types: ["vitest/globals"]`, and a new tsconfig.node.json covers `vitest.config.ts` with `types: ["node"]`. tsconfig.json stays a real program rather than a `files: []` solution file, because bunchee reads it for declaration emit. Also declares @types/node, which until now resolved only by hoisting from the example app, and drops `eslint.config.js` from `include` - without `allowJs` that glob never matched anything. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`eslint.config.js` was listed in `include`, but neither base.json nor this config enables `allowJs`, so the file was never part of the program. No error was raised, which left it reading as type coverage that did not exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`npm install --global yarn` ran one step *after* `setup-node`, which had already shelled out to whatever `yarn` was on the runner to locate the cache directory. The install could never help the step that needed it, and the job worked only because the GitHub images happen to preinstall Yarn 1.22.x. It also ignored `packageManager`, pulling whatever the latest 1.22.x happened to be. `corepack enable` honours the pinned yarn@1.22.18 instead. setup-node v7 has no `corepack` input, so the cache probe still uses the preinstalled Yarn - harmless, since every Yarn 1.x shares the same ~/.cache/yarn/v6 directory. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`snyk.advanced.autoSelectOrganization` was committed local editor state. Nothing else in the repo uses Snyk, and shared settings apply to every contributor's workspace. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`RedirectCallback` typed `appState` as required, but the provider calls
`onRedirectCallback(token?.state)` — which is `undefined` whenever sign-in
was started without state, i.e. a plain `loginWithRedirect()`. A consumer
writing the type-legal `({ returnTo }) => ...` therefore hit a destructuring
TypeError on a path the types said was safe.
The README and the internal `defaultOnRedirectCallback` already treated the
argument as optional; the exported type now matches them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… program `"types": ["vitest/globals"]` injected `describe`/`it`/`expect`/`vi` as ambient globals across all of `src`, shipped sources included — partly undoing the guard the surrounding commits were adding. A source file calling a bare `expect()` passed both `tsc --noEmit` and `eslint`, then shipped as a `ReferenceError` in a consumer's bundle. Every test file already imports these from `"vitest"` explicitly, so the ambient declarations bought nothing. `globals: true` stays in `vitest.config.ts`, so the runtime behaviour is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`actions/setup-node` resolves the `cache: yarn` key and path by shelling out to `yarn`, and it did so before `corepack enable` ran — so the probe used whatever ambient Yarn the runner image happens to preinstall rather than the `yarn@1.22.18` pinned in `packageManager`. That worked only by accident of `ubuntu-latest` still shipping Yarn 1.x; the day it stops, setup-node fails with "Unable to locate executable file: yarn" before Corepack gets a chance. Node is now installed first, Corepack enabled against that version, and a second setup-node runs the cache probe. The second call re-resolves an already-downloaded version, so it costs a cache hit rather than a download. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s in OIDCProvider
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Error callback URLs remain uncleared, and event listeners can briefly invoke stale handlers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Enhances OIDC error handling and event subscriptions while refining Node.js, TypeScript, and CI configuration.
Changes:
- Handles authorization errors and redirect failures.
- Stabilizes OIDC event subscriptions and expands tests.
- Separates browser/Node TypeScript configuration and updates CI setup.
File summaries
| File | Description |
|---|---|
.github/workflows/build.yml |
Enables Corepack before Yarn caching. |
.vscode/settings.json |
Removes Snyk workspace setting. |
package.json |
Broadens Node 24 compatibility. |
packages/example/src/App.tsx |
Displays authentication errors. |
packages/example/tsconfig.node.json |
Narrows Node configuration inputs. |
packages/oidc-provider/package.json |
Adds Node types and split typechecking. |
packages/oidc-provider/tsconfig.json |
Limits browser typechecking to source files. |
packages/oidc-provider/tsconfig.node.json |
Adds configuration-file typechecking. |
packages/oidc-provider/src/oidc-provider.tsx |
Stabilizes event listeners and handles optional redirect state. |
packages/oidc-provider/src/use-event-callback.ts |
Adds a stable event callback hook. |
packages/oidc-provider/src/utils.ts |
Recognizes OIDC error responses. |
packages/oidc-provider/src/with-authentication-required.tsx |
Surfaces redirect failures without retry loops. |
packages/oidc-provider/src/__tests__/oidc-provider.test.tsx |
Tests errors and stable subscriptions. |
packages/oidc-provider/src/__tests__/utils.test.ts |
Tests error response detection. |
packages/oidc-provider/src/__tests__/with-authentication-required.test.tsx |
Tests redirect failure behavior. |
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…le handler identity
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.