-
Couldn't load subscription status.
- Fork 8
fix(types): support organizations {id,name}[] in getClaim #178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe getClaim function in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/utils/token/getClaim.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/utils/token/getClaim.ts (1)
lib/utils/token/index.ts (1)
getClaim(11-11)
🔇 Additional comments (1)
lib/utils/token/getClaim.ts (1)
10-10: LGTM! Organizations claim type support added correctly.The extension of the
Vgeneric to include{ id: string; name: string }[]properly supports the organizations claim format. The change is backward compatible.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/utils/token/getClaim.ts (1)
8-8: JSDoc improvement addresses past feedback.The updated
@returnsannotation now correctly documents the full return structure{ name, value }instead of just the value type, resolving the previous review concern.However, the JSDoc hardcodes the default V union. When callers specify a custom type (e.g.,
getClaim<T, boolean>), the JSDoc will still show the full union, which may be misleading.Consider a more generic JSDoc that defers to TypeScript's type system:
-* @returns { Promise<{ name: keyof T; value: string | number | string[] | { id: string; name: string }[] } | null> } +* @returns {Promise<{ name: keyof T; value: V } | null>} Object containing the claim name and value (based on generic type V), or null if claims unavailable
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/utils/token/getClaim.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/utils/token/getClaim.ts (1)
lib/utils/token/index.ts (1)
getClaim(11-11)
🔇 Additional comments (1)
lib/utils/token/getClaim.ts (1)
10-10: LGTM! Appropriate type expansion for organizations claim.The expanded V generic correctly adds support for
{ id: string; name: string }[]to handle the organizations claim format while maintaining backward compatibility with existing claim types.
Explain your changes
Extend V union to include { id, name }[] for the organizations claim. Update JSDoc @returns to match TypeScript signature.
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.