Peter/get org update #220
Peter/get org update #220
Conversation
WalkthroughThe changes in this pull request introduce new utility functions, Changes
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
src/utils/generateUserOrganizationsObject.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-jest". (The package "eslint-plugin-jest" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-jest" was referenced from the config file in ".eslintrc.json". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (8)
src/utils/generateOrganizationObject.ts (1)
11-28: LGTM: Properties object is well-structured. Consider refactoring for improved maintainability.The
propertiesobject is well-structured with consistent naming and appropriate use of optional chaining. The fallback logic, checkingidTokenbeforeaccessToken, ensures the most relevant information is used.To improve maintainability and reduce repetition, consider refactoring the properties assignment using a helper function or a loop. Here's a suggested refactor:
const propertyKeys = [ 'city', 'industry', 'postcode', 'state_region', 'street_address', 'street_address_2' ] as const; const getPropertyValue = (key: typeof propertyKeys[number]) => idToken.organization_properties?.[`kp_org_${key}`]?.v || accessToken.organization_properties?.[`kp_org_${key}`]?.v; return { orgCode: accessToken.org_code, orgName: accessToken.org_name, properties: Object.fromEntries( propertyKeys.map(key => [key, getPropertyValue(key)]) ) };This refactored version reduces code duplication and makes it easier to add or modify properties in the future.
tests/frontend/generate-organization-object.test.ts (2)
4-136: LGTM: Comprehensive test data setup.The test data is well-structured and covers various scenarios, including tokens with and without properties. This will help ensure robust testing of the
generateOrganizationObjectfunction.Consider adding a comment explaining the purpose of the empty objects in
orgProperties(lines 18-19) to clarify if they are intentional for testing edge cases.
179-198: Good edge case coverage, but consider refining the expected output.This test case effectively covers the scenario where both tokens lack properties. However, there are a few points to consider:
The test name has a minor typo: "when not properties" should be "when no properties".
The expected output includes all possible properties with undefined values. Consider if it would be more appropriate to omit undefined properties or return an empty object for the
propertiesfield.Fix the typo in the test name:
- test('should generate org object with no properties when not properties in access token or id token', () => { + test('should generate org object with no properties when no properties in access token or id token', () => {
- Discuss with the team whether returning undefined properties or an empty object for
propertiesis the desired behavior when no properties are present in either token.tests/frontend/generate-user-object.test.ts (4)
5-286: LGTM: Comprehensive test data constants.The constants provide a good range of test scenarios, including cases with and without properties. This allows for thorough testing of the
generateUserObjectfunction.Consider using a factory function or object spread to reduce duplication between the full and "without properties" versions of the tokens. This could make the test data more maintainable.
306-329: LGTM: Good edge case coverage.This test case effectively checks the behavior of
generateUserObjectwhen the ID token lacks properties, ensuring the function handles partial data correctly.Consider adding an assertion for the
propertiesobject to explicitly check that it contains the expected keys, even if some values areundefined. This would make the test more robust against unintended changes in thegenerateUserObjectfunction.
350-363: LGTM: Excellent coverage of minimal data scenario.This test case effectively checks the behavior of
generateUserObjectwhen both tokens lack properties, ensuring the function can handle scenarios with minimal available data.Consider adding an assertion to explicitly check that the
propertieskey is not present in the returned object when both tokens lack properties. This would make the test more precise in verifying the expected behavior.
1-363: Overall: Excellent test coverage and structure.This test suite for
generateUserObjectis well-designed and comprehensive. It covers the happy path and various edge cases, providing good confidence in the function's behavior under different scenarios.Consider the following suggestions to further improve the test suite:
- Add a test case for invalid input, such as malformed tokens, to ensure proper error handling.
- Use parameterized tests to reduce code duplication and make it easier to add more test cases in the future.
- Consider testing with real-world data samples, if available, to ensure the function works correctly with actual token structures from your authentication system.
These improvements would enhance the robustness and maintainability of the test suite.
src/session/getOrganization.ts (1)
11-16: Add a description to the JSDoc comment forgetOrganizationFactory.The JSDoc comment is currently missing a description. Providing a clear explanation of the function's purpose will enhance readability and help other developers understand its usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- src/session/getOrganization.js (0 hunks)
- src/session/getOrganization.ts (1 hunks)
- src/utils/generateOrganizationObject.ts (1 hunks)
- tests/frontend/generate-organization-object.test.ts (1 hunks)
- tests/frontend/generate-user-object.test.ts (1 hunks)
- tests/frontend/utils.test.ts (0 hunks)
💤 Files with no reviewable changes (2)
- src/session/getOrganization.js
- tests/frontend/utils.test.ts
🧰 Additional context used
🔇 Additional comments (11)
src/utils/generateOrganizationObject.ts (4)
1-1: LGTM: Import statement is correct and necessary.The import statement correctly brings in the required types
KindeAccessTokenandKindeIdTokenfrom the parent directory.
3-6: LGTM: Function signature is well-defined and exported.The
generateOrganizationObjectfunction is correctly exported and has a clear, descriptive name. The parametersidTokenandaccessTokenare appropriately typed withKindeIdTokenandKindeAccessTokenrespectively.
7-10: LGTM: Main object structure is well-organized.The function returns an object with a clear structure, including
orgCode,orgName, and a nestedpropertiesobject. The use ofaccessTokenfororgCodeandorgNameis appropriate.
1-31: Overall, well-implemented function with room for minor improvements.The
generateOrganizationObjectfunction is well-structured and effectively combines information from bothidTokenandaccessTokento create a comprehensive organization object. The implementation is solid and achieves its purpose.To further enhance this implementation, consider:
- Refactoring the properties assignment to reduce repetition and improve maintainability.
- Adding input validation and error handling to make the function more robust.
These improvements will make the code more maintainable and resilient while preserving its current functionality.
tests/frontend/generate-organization-object.test.ts (4)
1-3: LGTM: Imports are correct and necessary.The imports for the
generateOrganizationObjectfunction and the required types are properly set up.
138-163: LGTM: Well-structured test cases.These test cases effectively cover the scenarios where:
- Both tokens contain properties
- The ID token lacks properties but the access token has them
The expected outputs are clearly defined and match the likely intended behavior of
generateOrganizationObject.
164-177: LGTM: Good coverage of access token without properties.This test case effectively covers the scenario where the access token lacks properties but the ID token contains them. It ensures that the
generateOrganizationObjectfunction can handle this situation correctly.
1-198: Overall, excellent test coverage and structure.This test file provides comprehensive coverage for the
generateOrganizationObjectfunction, including various scenarios and edge cases. The test data is well-structured and reusable, which contributes to the clarity and maintainability of the tests.Key strengths:
- Covers all major scenarios (both tokens with properties, one token without properties, both tokens without properties).
- Uses Jest testing patterns effectively.
- Test data is comprehensive and mimics real-world token structures.
Minor improvements have been suggested in previous comments, but these do not detract from the overall quality of the test suite.
tests/frontend/generate-user-object.test.ts (3)
1-2: LGTM: Imports are correct and necessary.The import statements are appropriate for the test file. They import the function being tested and the necessary types.
287-305: LGTM: Comprehensive happy path test.This test case effectively checks the basic functionality of
generateUserObject, ensuring all expected properties are correctly generated when both tokens have full data.
330-348: LGTM: Thorough edge case testing.This test case effectively verifies the behavior of
generateUserObjectwhen the access token lacks properties. It ensures that the function can still generate a complete user object using data from the ID token when the access token is missing information.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/utils/generateOrganizationObject.ts (1)
17-40: LGTM: Object construction is well-implemented. Consider minor refactoring.The object construction logic is sound and follows the requirements. The fallback mechanism ensures data is retrieved from the most appropriate source.
For improved readability and maintainability, consider extracting the property mapping into a separate function. This would make the main function more concise and easier to understand at a glance.
Here's a suggested refactoring:
const getPropertyValue = (property: string, idToken: KindeIdToken, accessToken: KindeAccessToken) => { return idToken.organization_properties?.[`kp_org_${property}`]?.v || accessToken.organization_properties?.[`kp_org_${property}`]?.v; }; export const generateOrganizationObject = ( idToken: KindeIdToken, accessToken: KindeAccessToken ) => { // ... (existing validation code) return { orgCode: accessToken.org_code, orgName: accessToken.org_name, properties: { city: getPropertyValue('city', idToken, accessToken), industry: getPropertyValue('industry', idToken, accessToken), postcode: getPropertyValue('postcode', idToken, accessToken), state_region: getPropertyValue('state_region', idToken, accessToken), street_address: getPropertyValue('street_address', idToken, accessToken), street_address_2: getPropertyValue('street_address_2', idToken, accessToken) } }; };This refactoring reduces repetition and makes the main function more concise.
src/handlers/setup.ts (2)
18-19: LGTM: AccessToken retrieval and type annotation added.The changes for retrieving and decoding the
accessTokenare correct. The addition of theKindeAccessTokentype annotation improves type safety.Consider combining the retrieval and decoding steps for better readability:
const accessToken: KindeAccessToken = jwtDecode( await routerClient.sessionManager.getSessionItem('access_token') );Also applies to: 24-24
21-22: LGTM: IdToken retrieval and type annotation added.The changes for retrieving and decoding the
idTokenare correct. The addition of theKindeIdTokentype annotation improves type safety.Similar to the
accessToken, consider combining the retrieval and decoding steps for better readability:const idToken: KindeIdToken = jwtDecode( await routerClient.sessionManager.getSessionItem('id_token') );Also applies to: 26-26
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/handlers/setup.ts (3 hunks)
- src/session/getOrganization.ts (1 hunks)
- src/utils/generateOrganizationObject.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/session/getOrganization.ts
🧰 Additional context used
🔇 Additional comments (7)
src/utils/generateOrganizationObject.ts (3)
1-1: LGTM: Import statement is correct.The import statement correctly imports the necessary types from the relative path.
3-6: LGTM: Function signature is well-defined.The function name is descriptive, and the parameters are clearly typed, following TypeScript best practices.
7-16: Great job implementing input validation!The input validation addresses the concerns raised in the previous review. It checks for the presence of both tokens and validates the structure of the
accessToken. The error messages are clear and informative.src/handlers/setup.ts (4)
5-5: LGTM: Import statement forgenerateOrganizationObjectadded.The import statement for
generateOrganizationObjecthas been correctly added, aligning with the PR objectives.
Line range hint
13-110: LGTM: Overall changes tosetupfunction are well-implemented.The modifications to the
setupfunction are consistent with the PR objectives. The addition of token retrieval, decoding, and the use ofgenerateOrganizationObjecthave been integrated seamlessly while maintaining the existing error handling and session management logic. The function remains clear and maintainable.
Line range hint
1-110: Overall assessment: Changes are well-implemented and align with PR objectives.The modifications to
src/handlers/setup.tshave been implemented effectively. The addition ofgenerateOrganizationObject, along with the improved type annotations for tokens, enhances the functionality and type safety of the code. The overall structure of thesetupfunction has been maintained, ensuring continued robustness in error handling and session management.Minor suggestions for code readability have been provided, but these are not critical. The changes successfully achieve the goal of updating the organization object generation process as outlined in the PR objectives.
81-81: LGTM: Organization object generation updated.The change to use
generateOrganizationObject(idToken, accessToken)aligns with the PR objectives and encapsulates the organization object creation logic, improving maintainability.To ensure the
generateOrganizationObjectfunction is implemented correctly, let's verify its implementation:✅ Verification successful
Verified:
generateOrganizationObjectfunction is correctly implemented.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of generateOrganizationObject function # Test: Check the implementation of generateOrganizationObject ast-grep --lang typescript --pattern $'export function generateOrganizationObject(idToken: KindeIdToken, accessToken: KindeAccessToken) { $$$ }'Length of output: 147
Script:
#!/bin/bash # Description: Verify the implementation of generateOrganizationObject function with different declaration styles # Test: Check for traditional function declaration ast-grep --lang typescript --pattern $'function generateOrganizationObject(idToken: KindeIdToken, accessToken: KindeAccessToken) { $$$ }' # Test: Check for arrow function declaration ast-grep --lang typescript --pattern $'const generateOrganizationObject = (idToken: KindeIdToken, accessToken: KindeAccessToken) => { $$$ }'Length of output: 3353
…ter/get-org-update
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/session/getOrganization.ts (2)
25-25: Remove unnecessary type assertion onidTokenString.After confirming
idTokenStringis notnull, TypeScript infers it asstring, so theas stringassertion is redundant.Apply this diff:
- const idToken = jwtDecoder<KindeIdToken>(idTokenString as string); + const idToken = jwtDecoder<KindeIdToken>(idTokenString);
33-33: Remove unnecessary type assertion onaccessTokenString.After confirming
accessTokenStringis notnull, TypeScript infers it asstring, so theas stringassertion is redundant.Apply this diff:
- const accessToken = jwtDecoder<KindeAccessToken>(accessTokenString as string); + const accessToken = jwtDecoder<KindeAccessToken>(accessTokenString);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/handlers/setup.ts (2 hunks)
- src/session/getOrganization.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
src/handlers/setup.ts (3)
5-5: LGTM: New import statement is well-placed and follows conventions.The import of
generateOrganizationObjectis correctly placed with other utility imports and follows the existing naming convention. This addition supports good modularity by keeping the organization object generation logic in a separate file.
Line range hint
1-114: Overall assessment: Good refactoring that improves code structure.The changes in this file effectively refactor the organization object generation, aligning with the PR objectives. The modification improves code modularity and consistency without disrupting the existing functionality. The error handling and other parts of the
setupfunction remain unchanged, which is appropriate.To ensure the refactoring is complete:
- Verify that all necessary organization-related claims are properly handled in the new
generateOrganizationObjectfunction.- Consider adding or updating tests to cover the new implementation, as mentioned in the PR objectives.
To confirm that tests have been added or updated, you can run:
✅ Verification successful
Tests for
generateOrganizationObjecthave been successfully added.The addition of
tests/frontend/generate-organization-object.test.tsand the recent commits confirm that the necessary tests are in place, ensuring comprehensive coverage of the refactoredgenerateOrganizationObjectfunction.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for new or updated tests related to organization object generation # Test: Look for test files related to organization object generation fd -e test.ts generate-organization-object # Test: Check for recent changes in test files git log -p --since="1 week ago" -- '*.test.ts'Length of output: 26427
81-81: Approve change with suggestion: Verify claims usage in new implementation.The replacement of the inline organization object with a call to
generateOrganizationObject(idToken, accessToken)is a good refactoring that improves maintainability and consistency with the user object generation.However, it's important to ensure that all necessary organization-related claims (e.g.,
org_code,org_name,organization_properties) are still being utilized in the new implementation.Please verify that
generateOrganizationObjectuses all relevant organization claims. You can run the following script to check the implementation:
…ss/kinde-auth-nextjs into peter/get-org-update
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (10)
src/session/getIdToken.js (1)
Line range hint
23-28: Consider enhancing error handlingThe current error handling silently returns null in production, which could make debugging authentication issues difficult. Consider:
- Adding error codes or types to distinguish between different failure scenarios
- Logging errors to an error monitoring service in production
} catch (err) { if (config.isDebugMode) { console.error(err); } - return null; + // Add error type checking + const errorType = err instanceof TypeError ? 'TOKEN_PARSE_ERROR' : 'SESSION_ERROR'; + // Still return null but with more context + return { + error: errorType, + token: null + }; }src/utils/generateUserOrganizationsObject.ts (1)
22-28: Improve type safety in organization mapping.The current mapping implementation could be more robust by ensuring org objects are properly typed and validated.
Consider this improvement:
return { orgCodes, - orgs: orgs.map((org) => ({ - code: org?.id, - name: org?.name - })) + orgs: orgs.map((org) => { + if (!org?.id || !org?.name) { + throw new Error('Invalid organization data structure'); + } + return { + code: org.id, + name: org.name + }; + }) };src/utils/getClaim.ts (2)
1-7: Consider using path aliases for importsInstead of using relative imports (
../../types), consider using path aliases (e.g.,@types) to improve maintainability and prevent issues when moving files.
21-38: Document token precedence logicThe function checks idToken before accessToken, but this precedence isn't documented.
Add JSDoc comments to explain the token precedence:
+/** + * Retrieves a claim from either the ID token or access token. + * Checks in the following order: + * 1. ID token with original claim name + * 2. ID token with Hasura claim name + * 3. Access token with original claim name + * 4. Access token with Hasura claim name + * + * @param {GetClaimParams} params - The parameters object + * @returns {any | null} The claim value or null if not found + */ export const getClaim = ({accessToken, claim, idToken}: GetClaimParams) => {src/session/getOrganization.ts (2)
8-12: Well-structured factory function with proper typing!The factory pattern is well-implemented here, allowing for flexible usage in both API and client-side contexts through optional req/res parameters.
Consider documenting the following usage patterns in the README:
- API route usage:
getOrganizationFactory(req, res)()- Client-side usage:
getOrganizationFactory()()
14-16: Consider strengthening session management.The session manager usage could be more robust by checking its initialization status.
Consider adding validation:
try { const session = await sessionManager(req, res); + if (!session) { + throw new Error('Failed to initialize session manager'); + }src/utils/generateOrganizationObject.ts (1)
4-7: Add JSDoc documentation for better maintainability.The function signature is well-defined with proper typing. Consider adding JSDoc documentation to describe the purpose, parameters, and return value.
+/** + * Generates an organization object from Kinde tokens + * @param idToken - The Kinde ID token containing user claims + * @param accessToken - The Kinde access token containing organization claims + * @returns The organization object or null if organization data is not present + */ export const generateOrganizationObject = ( idToken: KindeIdToken, accessToken: KindeAccessToken ): KindeOrganization | null => {src/config/index.ts (1)
28-33: LGTM! Well-structured token management enhancement.The new token-related properties and getters align well with the PR's objective of reading from both access and ID tokens for organization object generation. The naming is consistent and initialization is proper.
Consider documenting the following aspects:
- The difference between encoded vs raw token properties
- When to use each getter method
- The lifecycle of these tokens in the state
This will help future maintainers understand the token management strategy.
types.d.ts (1)
306-306: Consider using string[] instead of [] for better flexibility.The union type addition
{permissions: []; orgCode: null}is good for handling the initial/empty state. However, using an empty tuple type[]is more restrictive than necessary. Consider usingstring[]instead to maintain consistency with theKindePermissionstype and allow for future extensibility:- permissions: KindePermissions | {permissions: []; orgCode: null}; + permissions: KindePermissions | {permissions: string[]; orgCode: null}; - getPermissions: () => KindePermissions | {permissions: []; orgCode: null}; + getPermissions: () => KindePermissions | {permissions: string[]; orgCode: null};Also applies to: 340-340
src/session/getUserOrganizations.ts (1)
13-25: Refactor token retrieval and decoding to eliminate duplicationThe code for retrieving and decoding the
id_tokenandaccess_tokenis duplicated. Refactoring this logic into a reusable helper function will improve maintainability and reduce redundancy.Here's a suggested refactor:
+const getDecodedToken = async <T>(tokenName: string): Promise<T> => { + const tokenString = await session.getSessionItem(tokenName); + if (!tokenString) { + throw new Error(`${tokenName.replace('_', ' ')} is missing`); + } + return jwtDecoder<T>(tokenString as string); +}; -const idTokenString = await session.getSessionItem('id_token'); -if (!idTokenString) { - throw new Error('ID token is missing'); -} -const idToken = jwtDecoder<KindeIdToken>(idTokenString as string); -const accessTokenString = await session.getSessionItem('access_token'); -if (!accessTokenString) { - throw new Error('Access token is missing'); -} -const accessToken = jwtDecoder<KindeAccessToken>( - accessTokenString as string -); +const idToken = await getDecodedToken<KindeIdToken>('id_token'); +const accessToken = await getDecodedToken<KindeAccessToken>('access_token');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
src/config/index.ts(2 hunks)src/session/getIdToken.js(1 hunks)src/session/getOrganization.ts(1 hunks)src/session/getUserOrganizations.ts(1 hunks)src/utils/generateOrganizationObject.ts(1 hunks)src/utils/generateUserOrganizationsObject.ts(1 hunks)src/utils/getClaim.ts(1 hunks)types.d.ts(2 hunks)
🔇 Additional comments (13)
src/session/getIdToken.js (2)
Line range hint 1-28: Verify JWT token handling security
Since this code handles authentication tokens, let's verify the security aspects:
- Token validation
- Safe JWT decoding
- Proper token storage
#!/bin/bash
# Description: Check JWT handling security
# Check JWT decoder implementation
rg -A 10 'jwtDecoder'
# Check for token validation
ast-grep --pattern 'verify.*token'
# Look for token storage implementation in sessionManager
rg -A 5 'setSessionItem.*token'21-21: Verify the necessity of double await
The change from await sessionManager(req, res).getSessionItem('id_token') to await (await sessionManager(req, res)).getSessionItem('id_token') suggests that sessionManager now returns a Promise. While this fixes potential undefined method calls, the double await pattern could impact performance.
Let's verify the sessionManager implementation:
✅ Verification successful
Let me gather more specific information about the sessionManager implementation.
Based on the results, I can now generate the final response:
Double await is necessary - implementation verified
The double await pattern is correct and necessary because:
sessionManageris an async factory function that returns a Promise (confirmed insrc/session/sessionManager.js:export const sessionManager = async (req, res) => {)- The returned session manager object has
getSessionItemmethod that is also async (used consistently across codebase)
Therefore:
- First await: Resolves the Promise returned by sessionManager to get the session manager instance
- Second await: Resolves the Promise returned by getSessionItem to get the token value
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check sessionManager implementation to verify if double await is necessary
# Search for sessionManager implementation and exports
ast-grep --pattern 'export const sessionManager = $$$'
# Search for sessionManager class or interface definition
ast-grep --pattern 'class SessionManager {
$$$
}'
# Search for getSessionItem implementation
rg -A 5 'getSessionItem.*id_token'
Length of output: 14012
Script:
#!/bin/bash
# Search for sessionManager implementation
rg -A 10 "export.*sessionManager"
# Search for any Session class or interface definitions
ast-grep --pattern 'class $SESSION {
$$$
}'
# Search for Session type definitions
rg -A 5 "type.*Session"
Length of output: 8690
src/utils/generateUserOrganizationsObject.ts (1)
1-2: LGTM! Imports are well-organized.
The necessary types and utilities are properly imported.
src/utils/getClaim.ts (1)
1-38: Verify function usage in generateOrganizationObject
The implementation aligns with PR objectives, correctly handling both access and ID tokens.
Let's verify the usage:
✅ Verification successful
Implementation correctly serves its purpose in the codebase
The getClaim function is properly implemented and used as intended:
- Used in
generateOrganizationObjectto extractorg_code,org_name, andorganization_properties - Used in
generateUserOrganizationsObjectto extractorg_codesandorganizations - Handles both token types (access and ID) as required
- Supports both regular and Hasura-prefixed claims
- Implementation matches the type definitions in
types.d.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of getClaim in generateOrganizationObject
# Search for imports of getClaim
echo "Checking imports:"
rg "import.*getClaim.*from.*utils/getClaim"
# Search for usage of getClaim
echo "\nChecking usage:"
rg "getClaim\(\{.*\}\)"
Length of output: 515
Script:
#!/bin/bash
# Let's verify the implementation of generateOrganizationObject and generateUserOrganizationsObject
# Check generateOrganizationObject implementation
echo "Checking generateOrganizationObject implementation:"
ast-grep --pattern 'export const generateOrganizationObject = $$$'
# Check generateUserOrganizationsObject implementation
echo "\nChecking generateUserOrganizationsObject implementation:"
ast-grep --pattern 'export const generateUserOrganizationsObject = $$$'
# Check the types being used
echo "\nChecking type definitions:"
rg "type.*Organization" -A 5
rg "interface.*Organization" -A 5
Length of output: 6357
src/session/getOrganization.ts (2)
1-6: LGTM! Imports are well-organized and properly typed.
All necessary dependencies are imported with appropriate typing.
31-31: Verify organization object generation.
The generateOrganizationObject function is called with decoded tokens, but we should verify its implementation matches the PR objectives about reading from both tokens.
✅ Verification successful
Organization object generation implementation verified successfully
The implementation correctly reads from both tokens:
generateOrganizationObjectproperly validates both tokens and their structure- The
getClaimhelper function checks for claims in both tokens with proper fallback:- First checks idToken with standard claim name
- Then checks idToken with Hasura claim name
- Then checks accessToken with standard claim name
- Finally checks accessToken with Hasura claim name
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the generateOrganizationObject implementation
ast-grep --pattern 'export function generateOrganizationObject($_: KindeIdToken, $_: KindeAccessToken) {
$$$
}'
Length of output: 115
Script:
#!/bin/bash
# Let's try a broader search for the function implementation
rg "generateOrganizationObject" -A 10
Length of output: 7670
Script:
#!/bin/bash
# Let's get the complete implementation of generateOrganizationObject
cat src/utils/generateOrganizationObject.ts
Length of output: 1314
Script:
#!/bin/bash
# Let's check the getClaim implementation to ensure it properly reads from both tokens
cat src/utils/getClaim.ts
Length of output: 815
src/utils/generateOrganizationObject.ts (3)
1-2: LGTM! Clean and focused imports.
The imports are well-organized and all types and utilities are used within the function.
8-19: LGTM! Robust input validation with clear error messages.
The validation logic effectively handles:
- Required token presence
- Token structure validation
- Support for both standard and Hasura token formats
4-39: Verify integration with dependent modules.
According to the summary, this function is used in setup.ts and getOrganization.ts. Let's verify the integration.
✅ Verification successful
Let me verify the implementation details in the test file and the actual usage in the dependent modules to ensure proper integration.
Integration with dependent modules is properly implemented
The verification shows that:
- The function is correctly imported and used in both
setup.tsandgetOrganization.ts - Comprehensive test coverage exists in
generate-organization-object.test.tscovering various scenarios - The function is used consistently with proper error handling in both modules:
getOrganization.tsuses it within a try-catch block, returning null on errorssetup.tsintegrates it as part of the response object with proper token validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of generateOrganizationObject
# Check usage in setup.ts and getOrganization.ts
echo "Checking function usage in setup.ts and getOrganization.ts:"
rg -A 3 "generateOrganizationObject" "src/handlers/setup.ts" "src/session/getOrganization.ts"
# Check for any other potential usages
echo -e "\nChecking for other usages:"
rg -l "generateOrganizationObject" --type ts
Length of output: 1483
Script:
#!/bin/bash
# Check implementation details
echo "Checking test implementation:"
cat "tests/frontend/generate-organization-object.test.ts"
echo -e "\nChecking usage in getOrganization.ts:"
cat "src/session/getOrganization.ts"
echo -e "\nChecking usage in setup.ts:"
cat "src/handlers/setup.ts"
Length of output: 9380
src/config/index.ts (2)
3-3: LGTM! Good type safety improvement.
Adding explicit type annotation with KindeState improves type safety and makes the code more maintainable.
Also applies to: 5-5
11-11: Verify null handling in consumers of permissions and userOrganizations.
The change from empty arrays to null for permissions and userOrganizations could impact code that expects these properties to be arrays. Let's verify that all consumers properly handle null values.
Also applies to: 13-13
✅ Verification successful
Let me gather more context about the usage of these properties by checking their initialization and assignments.
Let me check the actual implementations where these properties are used, particularly focusing on the files we found in the previous searches.
No issues with changing permissions and userOrganizations to null
After analyzing the codebase, the change from empty arrays to null is safe because:
-
In
protect.js, permissions are only accessed after being fetched throughgetPermissions(), which returns the actual permissions from the server, not the initial state. -
In
AuthProvider.jsx, permissions are accessed through the setup response which provides properly structured data, not the initial state. ThegetPermissionfunction usespermissions.permissionsonly after the data is fetched from the server. -
In
setup.ts, both properties are properly initialized with actual data from the server before being used, overwriting the initial null values.
The initial state values being null don't impact the runtime behavior as they're immediately replaced with proper data structures during authentication.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct access to permissions and userOrganizations
# to ensure proper null checks are in place
echo "Checking permissions usage:"
rg -A 2 "\.permissions[^=]" --type ts
echo "Checking userOrganizations usage:"
rg -A 2 "\.userOrganizations[^=]" --type ts
# Check for array operations that might fail with null
echo "Checking for potential array operations:"
ast-grep --pattern 'permissions.$$$'
ast-grep --pattern 'userOrganizations.$$$'
Length of output: 647
Script:
#!/bin/bash
# Check for assignments and initialization of permissions and userOrganizations
echo "Checking permissions assignments:"
rg "permissions\s*=|permissions:\s*" --type ts -A 2
echo "Checking userOrganizations assignments:"
rg "userOrganizations\s*=|userOrganizations:\s*" --type ts -A 2
# Check for null checks
echo "Checking null checks:"
rg "(\?\.|\|\|)\s*(permissions|userOrganizations)" --type ts -A 2
Length of output: 2955
Script:
#!/bin/bash
# Check the implementation files where permissions are used
echo "Checking protect.js implementation:"
cat src/handlers/protect.js
echo "Checking AuthProvider.jsx implementation:"
cat src/frontend/AuthProvider.jsx
# Check the setup handler implementation
echo "Checking setup.ts implementation:"
cat src/handlers/setup.ts
Length of output: 14967
types.d.ts (1)
306-306: Verify the impact on consuming code.
The type changes for permissions and getPermissions() might require updates in code that consumes these types. Let's verify the usage to ensure type compatibility.
Also applies to: 340-340
✅ Verification successful
No breaking changes detected in consuming code
The type change for permissions is safe as the consuming code:
- Only accesses the
permissionsarray through the.permissionsproperty - Uses null-safe access with optional chaining (
?.) - Performs permission checks through
.some()array method which works with both type variants
The codebase shows no direct usage of getPermissions() method, making this change low risk.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usage of permissions property and getPermissions method
# to verify type compatibility with the new union type.
echo "Searching for direct permissions property access..."
rg -t typescript "\.permissions\b" --no-heading
echo -e "\nSearching for getPermissions method calls..."
rg -t typescript "\.getPermissions\(\)" --no-heading
Length of output: 392
Script:
#!/bin/bash
# Search for permissions property access and getPermissions calls using ripgrep
# without file type restriction since typescript extension isn't recognized
echo "Searching for direct permissions property access..."
rg "\.permissions\b" --no-heading
echo -e "\nSearching for getPermissions method calls..."
rg "\.getPermissions\(\)" --no-heading
Length of output: 1564
src/session/getUserOrganizations.ts (1)
27-27: LGTM
The use of generateUserOrganizationsObject with the decoded tokens is appropriate and correctly implemented.
| } catch (error) { | ||
| if (config.isDebugMode) { | ||
| console.error(error); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling granularity.
The current catch-all error handling might mask important issues by returning null for all error cases.
Consider this enhanced error handling:
} catch (error) {
+ // Handle known error cases
+ if (error instanceof Error) {
+ if (error.message.includes('token is missing')) {
+ if (config.isDebugMode) {
+ console.error('Authentication tokens not found:', error);
+ }
+ return null;
+ }
+ if (error.message.includes('invalid')) {
+ if (config.isDebugMode) {
+ console.error('Token validation failed:', error);
+ }
+ return null;
+ }
+ }
+ // Log unexpected errors regardless of debug mode
+ console.error('Unexpected error in getOrganization:', error);
if (config.isDebugMode) {
- console.error(error);
+ console.debug('Detailed error:', error);
}
return null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| if (config.isDebugMode) { | |
| console.error(error); | |
| } | |
| return null; | |
| } | |
| } catch (error) { | |
| // Handle known error cases | |
| if (error instanceof Error) { | |
| if (error.message.includes('token is missing')) { | |
| if (config.isDebugMode) { | |
| console.error('Authentication tokens not found:', error); | |
| } | |
| return null; | |
| } | |
| if (error.message.includes('invalid')) { | |
| if (config.isDebugMode) { | |
| console.error('Token validation failed:', error); | |
| } | |
| return null; | |
| } | |
| } | |
| // Log unexpected errors regardless of debug mode | |
| console.error('Unexpected error in getOrganization:', error); | |
| if (config.isDebugMode) { | |
| console.debug('Detailed error:', error); | |
| } | |
| return null; | |
| } |
| const orgProperties = getClaim({ | ||
| accessToken, | ||
| idToken, | ||
| claim: 'organization_properties' | ||
| }); | ||
|
|
||
| return { | ||
| orgCode: getClaim({accessToken, idToken, claim: 'org_code'}) as string, | ||
| orgName: getClaim({accessToken, idToken, claim: 'org_name'}) as string, | ||
| properties: { | ||
| city: orgProperties?.kp_org_city?.v, | ||
| industry: orgProperties?.kp_org_industry?.v, | ||
| postcode: orgProperties?.kp_org_postcode?.v, | ||
| state_region: orgProperties?.state_region?.v, | ||
| street_address: orgProperties?.street_address?.v, | ||
| street_address_2: orgProperties?.street_address_2?.v | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving type safety and consistency.
A few suggestions for improvement:
- Replace type assertions with runtime validation
- Add null check for orgProperties
- Use consistent property naming convention
const orgProperties = getClaim({
accessToken,
idToken,
claim: 'organization_properties'
});
+const orgCode = getClaim({accessToken, idToken, claim: 'org_code'});
+const orgName = getClaim({accessToken, idToken, claim: 'org_name'});
+
+if (typeof orgCode !== 'string' || typeof orgName !== 'string') {
+ return null;
+}
+
return {
- orgCode: getClaim({accessToken, idToken, claim: 'org_code'}) as string,
- orgName: getClaim({accessToken, idToken, claim: 'org_name'}) as string,
+ orgCode,
+ orgName,
properties: {
+ // Only include properties if orgProperties exists
+ ...(orgProperties && {
city: orgProperties?.kp_org_city?.v,
industry: orgProperties?.kp_org_industry?.v,
postcode: orgProperties?.kp_org_postcode?.v,
- state_region: orgProperties?.state_region?.v,
- street_address: orgProperties?.street_address?.v,
- street_address_2: orgProperties?.street_address_2?.v
+ stateRegion: orgProperties?.state_region?.v,
+ streetAddress: orgProperties?.street_address?.v,
+ streetAddress2: orgProperties?.street_address_2?.v
+ })
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const orgProperties = getClaim({ | |
| accessToken, | |
| idToken, | |
| claim: 'organization_properties' | |
| }); | |
| return { | |
| orgCode: getClaim({accessToken, idToken, claim: 'org_code'}) as string, | |
| orgName: getClaim({accessToken, idToken, claim: 'org_name'}) as string, | |
| properties: { | |
| city: orgProperties?.kp_org_city?.v, | |
| industry: orgProperties?.kp_org_industry?.v, | |
| postcode: orgProperties?.kp_org_postcode?.v, | |
| state_region: orgProperties?.state_region?.v, | |
| street_address: orgProperties?.street_address?.v, | |
| street_address_2: orgProperties?.street_address_2?.v | |
| } | |
| }; | |
| const orgProperties = getClaim({ | |
| accessToken, | |
| idToken, | |
| claim: 'organization_properties' | |
| }); | |
| const orgCode = getClaim({accessToken, idToken, claim: 'org_code'}); | |
| const orgName = getClaim({accessToken, idToken, claim: 'org_name'}); | |
| if (typeof orgCode !== 'string' || typeof orgName !== 'string') { | |
| return null; | |
| } | |
| return { | |
| orgCode, | |
| orgName, | |
| properties: { | |
| // Only include properties if orgProperties exists | |
| ...(orgProperties && { | |
| city: orgProperties?.kp_org_city?.v, | |
| industry: orgProperties?.kp_org_industry?.v, | |
| postcode: orgProperties?.kp_org_postcode?.v, | |
| stateRegion: orgProperties?.state_region?.v, | |
| streetAddress: orgProperties?.street_address?.v, | |
| streetAddress2: orgProperties?.street_address_2?.v | |
| }) | |
| } | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/handlers/setup.ts(2 hunks)types.d.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- types.d.ts
🔇 Additional comments (3)
src/handlers/setup.ts (3)
5-6: LGTM! Clean import organization.
The new imports are well-organized and properly grouped with related functionality.
60-62: Verify potential breaking changes in the organization object structure.
The new implementation might return a different organization object structure. Ensure this change is backward compatible or properly documented.
Let's check for existing usage patterns:
#!/bin/bash
# Description: Find usage patterns of the organization object
# Search for property access on the organization object
echo "Checking organization object usage patterns:"
rg -t typescript -t javascript "organization\." --glob '!{test,tests,__tests__}/**'60-62: Verify error handling in the new utility functions.
Ensure that the utility functions properly handle and propagate errors for invalid tokens or missing claims.
Let's verify the error handling implementation:
✅ Verification successful
Let me try a different approach to verify the error handling in these utility functions.
Error handling is properly implemented in both utility functions
Both utility functions have appropriate error handling:
-
generateOrganizationObject:- Validates presence of both tokens
- Checks for required token structure
- Throws descriptive error messages
- Uses type assertions safely with nullability checks
-
generateUserOrganizationsObject:- Validates presence of both tokens
- Uses safe property access with optional chaining
- Returns strongly typed results
The implementation follows good practices for error handling and null safety.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling in the utility functions
# Search for error handling in generateOrganizationObject
echo "Checking error handling in generateOrganizationObject:"
ast-grep --pattern 'if (!$_) { throw $$ }' src/utils/generateOrganizationObject.ts
# Search for error handling in generateUserOrganizationsObject
echo "Checking error handling in generateUserOrganizationsObject:"
ast-grep --pattern 'if (!$_) { throw $$ }' src/utils/generateUserOrganizationsObject.ts
Length of output: 527
Script:
#!/bin/bash
# Let's first check if these files exist and examine their content
echo "Checking for utility files:"
fd "generate(Organization|UserOrganizations)Object\.ts$"
echo -e "\nChecking content of generateOrganizationObject.ts:"
fd "generateOrganizationObject\.ts$" -x cat {}
echo -e "\nChecking content of generateUserOrganizationsObject.ts:"
fd "generateUserOrganizationsObject\.ts$" -x cat {}
# Let's also look for try-catch patterns
echo -e "\nChecking for try-catch blocks:"
rg "try\s*{" -A 5 --glob "**/generate*.ts"
Length of output: 2730
Explain your changes
generateOrganizationObjectnow reads from both accesstoken and idtoken when populating the objectutils.test.tsintogenerate-user-object.test.ts+generate-organization-object.test.tsSuppose there is a related issue with enough detail for a reviewer to understand your changes fully. In that case, you can omit an explanation and instead include either “Fixes #XX” or “Updates #XX” where “XX” is the issue number.
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.