FEN-229 Make the JWT profile customizable #256
Conversation
tsl-ps2
requested review from
anicioalexandre,
deboracosilveira and
priscilladeroode
as code owners
|
WalkthroughThis update introduces a more flexible and extensible approach to handling user profile data within the authentication and profile modules. The core changes include the addition of customizable utility functions for transforming user and profile data, the introduction of a generic profile hook, and the expansion of type definitions to support arbitrary profile properties. Several components and hooks now accept optional transformation functions as props or options, allowing for tailored profile data extraction and manipulation. Type safety and modularity are improved throughout, with explicit support for custom and minimal profile representations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant useLogin
participant getCustomProfile
participant setCurrentProfile
User->>useLogin: Initiate login
useLogin->>getCustomProfile: Transform User to CustomProfile
getCustomProfile-->>useLogin: Return CustomProfile
useLogin->>setCurrentProfile: Save CustomProfile
sequenceDiagram
participant ProfileSettingsComponent
participant getMinimalProfile
participant updateProfileIfActive
ProfileSettingsComponent->>getMinimalProfile: Transform Profile data
getMinimalProfile-->>ProfileSettingsComponent: Return MinimalProfile
ProfileSettingsComponent->>updateProfileIfActive: Update profile with MinimalProfile
Poem
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
packages/authentication/modules/access/index.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/authentication/.eslintrc.js
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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. 🪧 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: 1
🔭 Outside diff range comments (1)
packages/authentication/modules/profile/useCurrentProfile/index.ts (1)
48-60:⚠️ Potential issueParameter type of
saveCurrentProfileis too wide
saveCurrentProfilecurrently requiresCustomProfile | null, yet we call it withTProfile | null.
WhenTProfileis narrower thanCustomProfile(e.g. plainMinimalProfile), TypeScript will error because the argument is not guaranteed to satisfy the index signature ofRecord<string, any>.- const saveCurrentProfile = (newProfile: CustomProfile | null) => { + const saveCurrentProfile = (newProfile: TProfile | null) => {This simple change aligns the helper with the public contract of the hook and removes the need for unsafe casts in the caller.
🧹 Nitpick comments (5)
packages/authentication/modules/access/useLogin/utils.ts (1)
5-6: Consider adding error handling for missing environment variable.While the optional chaining (
?.) protects against undefinedNEXT_PUBLIC_API_BASE_URL, if it's undefined, this would result in concatenating "undefined" with the image path, creating an invalid URL.- const baseUrl = process.env.NEXT_PUBLIC_API_BASE_URL?.replace('/v1', '') + const baseUrl = process.env.NEXT_PUBLIC_API_BASE_URL?.replace('/v1', '') || ''packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx (1)
113-114: Consider adding a null check before applying transformation.While the outer
if (profile)check prevents calling the transformation on null profiles, including an explicit null check before using the result would make the code more defensive.- const minimalProfileData = getMinimalProfile(profile) - updateProfileIfActive(minimalProfileData) + const minimalProfileData = getMinimalProfile(profile) + if (minimalProfileData) { + updateProfileIfActive(minimalProfileData) + }packages/authentication/modules/profile/useCurrentProfile/index.ts (3)
26-26:profileAtomshould storeTProfile, not alwaysCustomProfileBecause the hook is now generic, keeping the atom fixed to
CustomProfileforces a down‑cast every time we read or write the value.
Parameterising the atom eliminates unnecessary casts and prevents accidental property leakage.-export const profileAtom = atom<CustomProfile | null>(initialProfile) +export const profileAtom = atom<TProfile | null>(initialProfile)Note: this requires
profileAtomto be declared inside the generic hook or moved to a factory helper, otherwiseTProfileis out of scope.
64-67:updateProfileIfActivemutates the current profile but widens the typeSpreading two generics (
currentProfileandnewProfile) producesTProfile & Partial<TProfile>, which TypeScript simplifies to an anonymous object.
Subsequently passing that object tosaveCurrentProfile(after the fix above) may fail to satisfyTProfileif optional fields become required again.Consider enforcing the return type explicitly:
saveCurrentProfile({ ...currentProfile, ...newProfile } as TProfile)or, even better, use a utility like
deepMerge<TProfile>()that preserves the original type.
70-78:typeof window === typeof undefinedis a fragile SSR checkThe conventional pattern is
typeof window === 'undefined'.
Comparing totypeof undefinedworks but is obscure and might confuse future maintainers.- const isSSR = typeof window === typeof undefined + const isSSR = typeof window === 'undefined'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/authentication/index.ts(1 hunks)packages/authentication/modules/access/useLogin/index.ts(3 hunks)packages/authentication/modules/access/useLogin/types.ts(2 hunks)packages/authentication/modules/access/useLogin/utils.ts(1 hunks)packages/authentication/modules/profile/index.ts(0 hunks)packages/authentication/modules/profile/useCurrentProfile/index.ts(3 hunks)packages/authentication/types/profile.ts(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx(2 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/types.ts(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/utils.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/authentication/modules/profile/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (6)
packages/components/modules/profiles/web/ProfileSettingsComponent/types.ts (1)
packages/authentication/types/profile.ts (1)
CustomProfile(8-8)
packages/authentication/modules/access/useLogin/utils.ts (2)
packages/authentication/types/user.ts (1)
User(3-15)packages/authentication/types/profile.ts (1)
CustomProfile(8-8)
packages/authentication/modules/access/useLogin/types.ts (2)
packages/authentication/types/user.ts (1)
User(3-15)packages/authentication/types/profile.ts (1)
CustomProfile(8-8)
packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx (2)
packages/components/modules/profiles/web/ProfileSettingsComponent/types.ts (1)
ProfileSettingsComponentProps(8-11)packages/components/modules/profiles/web/ProfileSettingsComponent/utils.ts (1)
getMinimalProfile(3-8)
packages/authentication/modules/access/useLogin/index.ts (1)
packages/authentication/modules/access/useLogin/utils.ts (1)
getCustomProfile(4-17)
packages/authentication/modules/profile/useCurrentProfile/index.ts (4)
packages/utils/types/server.ts (1)
ServerSideRenderingOption(1-8)packages/utils/functions/cookie/index.ts (1)
getCookie(13-25)packages/authentication/types/profile.ts (2)
CustomProfile(8-8)MinimalProfile(1-6)packages/authentication/modules/profile/useCurrentProfile/constants.ts (1)
CURRENT_PROFILE_KEY(1-1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (12)
packages/authentication/types/profile.ts (1)
8-8: Good extension of MinimalProfile to support additional properties.The new
CustomProfiletype allows for adding arbitrary properties to the base profile, enabling more flexible profile data handling across the application. Consider whether usingRecord<string, any>provides the right balance between flexibility and type safety for your use case.packages/components/modules/profiles/web/ProfileSettingsComponent/utils.ts (1)
3-8: Well-structured utility function for profile extraction.This utility function nicely encapsulates the logic for extracting minimal profile data from a larger profile object. The use of nullish coalescing operators (
??) provides safe handling of optional fields, and the function's structure makes it easy to understand and maintain.packages/authentication/modules/access/useLogin/types.ts (1)
11-12: Good addition of customizable profile transformation.Adding the optional
getCustomProfilefunction parameter allows consumers to customize how user data is transformed into profile data, improving the hook's flexibility while maintaining backward compatibility.Also applies to: 22-22
packages/authentication/index.ts (1)
18-18: Good addition to the public API exports.Exporting the profile types makes the new
CustomProfiletype and other profile-related types available to consumers of the package, completing the implementation of the customizable profile feature.packages/authentication/modules/access/useLogin/utils.ts (2)
4-17: Nice implementation of the customizable profile utility function.The function efficiently handles both relative and absolute image URLs, ensuring consistent profile image paths across the application. The approach of extracting this logic into a separate utility makes the code more modular and reusable.
5-5: Follow up on the TODO comment.There's a comment about handling absolute image paths on the backend. Consider creating a ticket to track this improvement if it's not already being addressed.
This client-side URL manipulation might be unnecessary if the backend could consistently provide absolute image URLs.
packages/components/modules/profiles/web/ProfileSettingsComponent/types.ts (1)
1-11: Good addition of customizable profile transformation function.Adding the optional
getMinimalProfileprop creates a flexible extension point that allows consumers to customize how profile data is transformed and used, which aligns well with the JWT profile customization objective.packages/authentication/modules/access/useLogin/index.ts (3)
35-35: Clear import with descriptive name.Good practice to import the default implementation with a descriptive name that indicates its purpose.
45-45: Good implementation of optional parameter with default.The addition of the optional
getCustomProfileparameter with a default implementation follows good design patterns for customizable behavior.
64-65: Excellent refactoring to use the custom profile transformer.Replacing the inline logic with a call to the configurable
getCustomProfilefunction improves modularity and makes the code more maintainable. This change aligns well with the PR's objective of making JWT profiles customizable.packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx (2)
39-44: Good implementation of customizable profile transformation.The import of the default implementation and providing it as a default value for the optional prop follows best practices for creating extensible components.
111-116:✅ Verification successful
Verify the useEffect dependency array consistency.
While the code now correctly uses the customizable profile transformation function, the dependency array lists individual profile properties that might not match what
getMinimalProfileactually uses.If
getMinimalProfileaccesses more properties than those listed in the dependency array, consider simplifying to just use[profile, getMinimalProfile, updateProfileIfActive]to avoid potential stale closures.
🏁 Script executed:
#!/bin/bash # Check what properties getMinimalProfile accesses in its implementation rg "getMinimalProfile.+\{" -A 10 "packages/components/modules/profiles/web/ProfileSettingsComponent/utils.ts"Length of output: 319
Dependency array correctly reflects
getMinimalProfileusageThe effect lists precisely the properties (
id,name,urlPath?.path,image?.url) thatgetMinimalProfilereads, so there’s no mismatch between what’s accessed and what’s watched. You can keep the array as‑is or, if you prefer a more concise form, replace it with a single dependency onprofile(the importedgetMinimalProfileis static and doesn’t need to be listed).
| export const getProfileFromCookie = ({ noSSR = true }: ServerSideRenderingOption = {}) => { | ||
| const settings = | ||
| getCookie<MinimalProfile | undefined>(CURRENT_PROFILE_KEY, { noSSR, parseJSON: true }) ?? null | ||
| getCookie<CustomProfile | undefined>(CURRENT_PROFILE_KEY, { noSSR, parseJSON: true }) ?? null | ||
|
|
||
| return settings |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Cookie‑fetch should respect the generic profile (TProfile) you advertise
getProfileFromCookie is hard‑coded to return CustomProfile, yet the public hook advertises a generic TProfile.
If a caller narrows TProfile to something stricter than CustomProfile, we silently cast the broader cookie value down to the narrower type, losing type safety.
- const settings =
- getCookie<CustomProfile | undefined>(CURRENT_PROFILE_KEY, { noSSR, parseJSON: true }) ?? null
+ const settings =
+ getCookie<TProfile | undefined>(CURRENT_PROFILE_KEY, { noSSR, parseJSON: true }) ?? nullApplying the same generic here ensures compile‑time guarantees that the cookie really contains the profile shape the consumer expects.
📝 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.
| export const getProfileFromCookie = ({ noSSR = true }: ServerSideRenderingOption = {}) => { | |
| const settings = | |
| getCookie<MinimalProfile | undefined>(CURRENT_PROFILE_KEY, { noSSR, parseJSON: true }) ?? null | |
| getCookie<CustomProfile | undefined>(CURRENT_PROFILE_KEY, { noSSR, parseJSON: true }) ?? null | |
| return settings | |
| export const getProfileFromCookie = ({ noSSR = true }: ServerSideRenderingOption = {}) => { | |
| const settings = | |
| getCookie<TProfile | undefined>(CURRENT_PROFILE_KEY, { noSSR, parseJSON: true }) ?? null | |
| return settings | |
| } |
anicioalexandre
force-pushed
the
fen-229-permissions-baseapp
branch
from
fa8f3c4 to
ed61088
Compare
|
This allows us to customize the MinimalProfile.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor