-
Notifications
You must be signed in to change notification settings - Fork 24
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
Fix: coalesce nulls when Hasura claims don't exist #217
Conversation
WalkthroughThe pull request introduces changes to two files: Changes
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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
🧹 Outside diff range and nitpick comments (2)
src/session/getUserOrganizations.ts (1)
23-29
: Overall improvement in null handling with a suggestion for error loggingThese changes significantly improve the robustness of the
getUserOrganizationsFactory
function by ensuring thathasuraOrgCodes
andhasuraOrganizations
are always arrays, even when the corresponding claims are null or undefined. This directly addresses the issue described in the PR objectives and prevents exceptions when spreading these values.While the changes are excellent, consider enhancing the error logging in the catch block. Currently, it only logs the error in debug mode. It might be beneficial to log specific information about which claim (if any) was null or undefined, to aid in future debugging or monitoring.
For example:
} catch (error) { if (config.isDebugMode) { console.debug('getUserOrganization error:', error); if (error instanceof TypeError) { console.debug('Possible null claim value. hasuraOrgCodes:', hasuraOrgCodes, 'hasuraOrganizations:', hasuraOrganizations); } } return null; }This additional logging could provide more context about the nature of any errors that occur, while still maintaining the current behavior of returning null on error.
src/frontend/KindeBrowserClient.js (1)
26-26
: Approve change with suggestion for improvementThe change to initialize
userOrganizations
asnull
aligns well with the PR objective of handling cases where Hasura claims don't exist. This allows for a clear distinction between "no data fetched yet" (null) and "fetched but empty" (empty array).However, to ensure robustness, consider updating the
getUserOrganizations
function to handle thenull
case:const getUserOrganizations = () => { - return state.userOrganizations; + return state.userOrganizations ?? []; };This change ensures that consumers of
getUserOrganizations
always receive an array, maintaining backwards compatibility while allowing for the newnull
initial state.Additionally, consider adding a comment explaining the rationale behind using
null
instead of an empty array for initial state:userOrganizations: null // Initialized as null to distinguish between "no data fetched" and "fetched but empty"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/frontend/KindeBrowserClient.js (1 hunks)
- src/session/getUserOrganizations.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
src/session/getUserOrganizations.ts (2)
23-23
: Excellent addition of null coalescing operatorThe addition of
?? []
is a great improvement. It ensures thathasuraOrgCodes
will always be an array, even if the claim value is null or undefined. This change directly addresses the issue mentioned in the PR objectives and prevents potential exceptions when spreading this value later in the function.
29-29
: Consistent null handling for hasuraOrganizationsThis change mirrors the improvement made to
hasuraOrgCodes
, ensuring thathasuraOrganizations
is always an array. This consistency in handling potential null values from claim data is commendable and further strengthens the function's resilience.
Explain your changes
When Hasura claims are not included in the token the resultant
null
claim causesgetUserOrganizations()
to returnnull
due to an exception trying to spread thenull
.Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.