Fix type errors for Node 24 compatibility#54
Merged
domleboss97 merged 2 commits intomainfrom Feb 24, 2026
Merged
Conversation
WalkthroughIntroduces a FetchLike type and updates DID resolver types and JWKS initialization, replaces direct global.fetch assignment with Vitest stubbing and cleanup in tests, adds explicit MiddlewareHandler generics and SignedPayloadEnv typing to middleware, and bumps Node.js from 22 to 24 in GitHub Actions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
venables
approved these changes
Feb 24, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/actions/setup/action.yaml (1)
11-11: LGTM — Node 24 is Active LTS.Node 24 (
Krypton) entered Active LTS on 2025-10-28, and the latest patch as of February 2026 is 24.13.1, so"24"is a well-supported version string foractions/setup-node. The version bump is consistent with the PR's stated goal of Node 24 compatibility.One optional consideration: Node 22 is still in Maintenance LTS until April 2027. If the library is intended to remain compatible with Node 22, adding a matrix build (e.g.,
[22, 24]) would ensure regressions on older supported runtimes are caught in CI. If Node 22 is being intentionally dropped, a note in the changelog/README would help consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/setup/action.yaml at line 11, The workflow currently pins actions/setup-node to node-version: "24" which is fine; if you intend to keep Node 22 compatibility add a matrix in the workflow (e.g., matrix.node-version: [22, 24]) so CI runs against both runtimes and update the actions/setup-node usage to use matrix.node-version, otherwise add a short changelog/README note stating that Node 22 support is dropped; modify the workflow's node-version key or add matrix.node-version and adjust any step references (actions/setup-node) accordingly.tools/api-utils/src/middleware/signed-payload-validator.ts (1)
38-45:"json"output key is hardcoded regardless of thetargetparameter.The return type
{ out: { json: ValidatedSignedPayload<T> } }will misrepresent the validated output whentargetis anything other than"json"(e.g."form","query"). If this validator is only ever used with"json", consider narrowing thetargetparameter type accordingly. Otherwise, maketargeta generic type parameter so the output key matches:♻️ Proposed refactor for target-aware output typing
-export const signedPayloadValidator = <T>( - target: keyof ValidationTargets, +export const signedPayloadValidator = <T, TTarget extends keyof ValidationTargets>( + target: TTarget, schema: v.GenericSchema<unknown, T>, ): MiddlewareHandler< Env, string, - { out: { json: ValidatedSignedPayload<T> } } + { out: { [K in TTarget]: ValidatedSignedPayload<T> } } > =>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/api-utils/src/middleware/signed-payload-validator.ts` around lines 38 - 45, The return type of signedPayloadValidator incorrectly hardcodes the output key "json"; update the function signature to make the target parameter type-aware: change the parameter to a generic like TTarget extends keyof ValidationTargets and accept target: TTarget (or if this validator truly only handles JSON, narrow target to "json"), then update the MiddlewareHandler return type from { out: { json: ValidatedSignedPayload<T> } } to use the generic key (e.g. { out: { [K in TTarget]: ValidatedSignedPayload<T> } }), ensuring ValidationTargets and any callers are updated to satisfy the new generic where needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/actions/setup/action.yaml:
- Line 11: The workflow currently pins actions/setup-node to node-version: "24"
which is fine; if you intend to keep Node 22 compatibility add a matrix in the
workflow (e.g., matrix.node-version: [22, 24]) so CI runs against both runtimes
and update the actions/setup-node usage to use matrix.node-version, otherwise
add a short changelog/README note stating that Node 22 support is dropped;
modify the workflow's node-version key or add matrix.node-version and adjust any
step references (actions/setup-node) accordingly.
In `@tools/api-utils/src/middleware/signed-payload-validator.ts`:
- Around line 38-45: The return type of signedPayloadValidator incorrectly
hardcodes the output key "json"; update the function signature to make the
target parameter type-aware: change the parameter to a generic like TTarget
extends keyof ValidationTargets and accept target: TTarget (or if this validator
truly only handles JSON, narrow target to "json"), then update the
MiddlewareHandler return type from { out: { json: ValidatedSignedPayload<T> } }
to use the generic key (e.g. { out: { [K in TTarget]: ValidatedSignedPayload<T>
} }), ensuring ValidationTargets and any callers are updated to satisfy the new
generic where needed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/actions/setup/action.yamlpackages/did/src/did-resolvers/get-did-resolver.tspackages/did/src/did-resolvers/web-did-resolver.test.tspackages/did/src/did-resolvers/web-did-resolver.tspackages/did/src/types.tstools/api-utils/src/middleware/signed-payload-validator.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/api-utils/src/middleware/signed-payload-validator.ts`:
- Around line 47-53: Add a runtime guard inside the async validator callback to
detect if c.get("resolver") returns undefined and throw a clear, descriptive
Error (e.g. "missing DID resolver in context") before continuing; specifically,
in the validator(target, async (value, c) => { ... }) block where you call const
didResolver = c.get("resolver"), check didResolver and throw if falsy so you
avoid opaque failures while preserving the existing dev-mode bypass path and
subsequent try/catch flow.
Comment on lines
+47
to
+53
| ): MiddlewareHandler< | ||
| SignedPayloadEnv, | ||
| string, | ||
| { out: { json: ValidatedSignedPayload<T> } } | ||
| > => | ||
| validator(target, async (value, c): Promise<ValidatedSignedPayload<T>> => { | ||
| const didResolver = c.get("resolver") as Resolvable | undefined | ||
| const didResolver = c.get("resolver") |
There was a problem hiding this comment.
Add a runtime guard for missing resolver to avoid opaque failures.
Even with env typing, a misconfigured context can still yield undefined at runtime; throwing a clear error inside the try preserves the dev-mode bypass path.
🧯 Suggested guard
- validator(target, async (value, c): Promise<ValidatedSignedPayload<T>> => {
- const didResolver = c.get("resolver")
-
- try {
+ validator(target, async (value, c): Promise<ValidatedSignedPayload<T>> => {
+ const didResolver = c.get("resolver")
+
+ try {
+ if (!didResolver) {
+ throw new Error("Missing DID resolver in context")
+ }
const data = v.parse(signedPayloadSchema, value)
const { parsed, body } = await validatePayload(
data.payload,
schema,
didResolver,
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/api-utils/src/middleware/signed-payload-validator.ts` around lines 47 -
53, Add a runtime guard inside the async validator callback to detect if
c.get("resolver") returns undefined and throw a clear, descriptive Error (e.g.
"missing DID resolver in context") before continuing; specifically, in the
validator(target, async (value, c) => { ... }) block where you call const
didResolver = c.get("resolver"), check didResolver and throw if falsy so you
avoid opaque failures while preserving the existing dev-mode bypass path and
subsequent try/catch flow.
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
2 participants
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.
static property in Node 24
Summary by CodeRabbit
Chores
Refactor
Tests