-
Notifications
You must be signed in to change notification settings - Fork 61
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
feat(react-spa): New modal when user switches from child to self #16951
Conversation
WalkthroughThis pull request introduces a new React functional component, Changes
Possibly related PRs
Suggested labels
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
warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. 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
Documentation and Community
|
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: 2
🧹 Outside diff range and nitpick comments (10)
libs/react-spa/bff/src/lib/BffSessionExpiredModal.tsx (1)
16-24
: Consider implementing i18n support for better reusabilityWhile the comment explains why translations are not available, consider implementing a fallback mechanism or default language configuration to make the component more reusable across different applications.
Consider:
- Creating a constants file for default strings
- Adding a prop for optional translation overrides
- Implementing a simple fallback mechanism
+ type Translations = { + title?: string; + prefixText?: string; + actionText?: string; + postfixText?: string; + } type BffSessionExpiredModalProps = { onLogin(): void; + translations?: Translations; }libs/react-spa/bff/src/lib/BffNewUserSameSessionModal.tsx (1)
1-25
: Consider enhancing component configurabilitySince this is a reusable library component, consider making the modal more configurable:
- Allow customization of action button styles
- Support custom content rendering
- Add event tracking capabilities for analytics
This would improve reusability across different NextJS applications within the system.
libs/react-spa/bff/src/lib/BffProblemTemplateModal.tsx (3)
1-2
: Consider making CSS more modularThe
fullScreen
CSS import fromErrorScreen.css
seems to be shared with an error screen component. Consider moving shared styles to a common styles file or using a more specific CSS module for this component.
4-12
: Enhance type documentation for better reusabilityThe type definition is well-structured, but could benefit from JSDoc documentation explaining its purpose and usage patterns.
+/** + * Props for the BffProblemTemplateModal component + * @property title - The main heading of the modal + * @property action - Configuration for the action button and surrounding text + */ export type BffProblemTemplateModalProps = { title: string action: { prefixText: string text: string postfixText: string onClick(): void } }
17-43
: Consider performance and accessibility improvementsThe component implementation is clean and well-structured, but could benefit from the following enhancements:
- Memoize the component to prevent unnecessary re-renders:
-export const BffProblemTemplateModal = ({ +export const BffProblemTemplateModal = React.memo(({ title, action: { prefixText, text, postfixText, onClick }, -}: BffProblemTemplateModalProps) => ( +}: BffProblemTemplateModalProps) => { + const handleClick = React.useCallback(() => { + onClick(); + }, [onClick]); + + return ( <Box display="flex" justifyContent="center" alignItems="center" padding={[0, 6]} className={fullScreen} > <ProblemTemplate variant="warning" expand title={title} + role="alertdialog" + aria-modal="true" message={ <> {prefixText}{' '} - <Button variant="text" onClick={onClick}> + <Button variant="text" onClick={handleClick}> {text} </Button>{' '} {postfixText} </> } /> </Box> -) + ); +}); + +BffProblemTemplateModal.displayName = 'BffProblemTemplateModal';
- Add prop-types validation for non-TypeScript consumers:
import PropTypes from 'prop-types'; BffProblemTemplateModal.propTypes = { title: PropTypes.string.isRequired, action: PropTypes.shape({ prefixText: PropTypes.string.isRequired, text: PropTypes.string.isRequired, postfixText: PropTypes.string.isRequired, onClick: PropTypes.func.isRequired, }).isRequired, };libs/react-spa/bff/src/lib/bff.utils.ts (3)
45-50
: Consider using explicit Boolean conversionWhile the double negation (
!!
) works, consider using a more explicit boolean conversion for better readability.- return !!(oldSid && newSid && oldSid !== newSid) + return Boolean(oldSid && newSid && oldSid !== newSid)
52-62
: Consider adding null/undefined checks for profile propertiesThe function looks good and aligns with the PR objectives. However, consider adding null/undefined checks for the profile properties to make it more robust.
export const isNewUserWithSameSession: UserCheckFn = (oldUser, newUser) => { const isSameSession = !isNewSession(oldUser, newUser) if (isSameSession) { - return oldUser.profile.nationalId !== newUser.profile.nationalId + return oldUser.profile?.nationalId != null && + newUser.profile?.nationalId != null && + oldUser.profile.nationalId !== newUser.profile.nationalId } return false }
40-62
: Well-structured utility functions with good reusabilityThe implementation follows good practices for reusable utilities:
- Pure functions that are easy to test and maintain
- Effective TypeScript usage
- Clear separation of concerns
- Tree-shakeable exports
libs/react-spa/bff/src/lib/BffProvider.tsx (2)
51-58
: Consider extracting event handling logic.While the logic is correct, consider extracting the event handling into a separate function for better maintainability:
+ const handleNewSession = (userInfo: UserInfo) => { + if (isNewSession(state.userInfo, userInfo)) { + setSessionExpiredScreen(true) + } else if (isNewUserWithSameSession(state.userInfo, userInfo)) { + setNewUserScreen(true) + } + } const { postMessage } = useBffBroadcaster((event) => { if (isLoggedIn && event.data.type === BffBroadcastEvents.NEW_SESSION) { - if (isNewSession(state.userInfo, event.data.userInfo)) { - setSessionExpiredScreen(true) - } else if (isNewUserWithSameSession(state.userInfo, event.data.userInfo)) { - setNewUserScreen(true) - } + handleNewSession(event.data.userInfo) } else if (event.data.type === BffBroadcastEvents.LOGOUT) {
231-233
: Simplify the reload handler.The reload handler can be simplified by directly passing
window.location.reload
:<BffNewUserSameSessionModal - onReload={() => { - window.location.reload() - }} + onReload={window.location.reload} />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
libs/react-spa/bff/src/lib/BffNewUserSameSessionModal.tsx
(1 hunks)libs/react-spa/bff/src/lib/BffProblemTemplateModal.tsx
(1 hunks)libs/react-spa/bff/src/lib/BffProvider.tsx
(4 hunks)libs/react-spa/bff/src/lib/BffSessionExpiredModal.tsx
(2 hunks)libs/react-spa/bff/src/lib/bff.utils.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
libs/react-spa/bff/src/lib/BffNewUserSameSessionModal.tsx (1)
Pattern libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/react-spa/bff/src/lib/BffProblemTemplateModal.tsx (1)
Pattern libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/react-spa/bff/src/lib/BffProvider.tsx (1)
Pattern libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/react-spa/bff/src/lib/BffSessionExpiredModal.tsx (1)
Pattern libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/react-spa/bff/src/lib/bff.utils.ts (1)
Pattern libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (8)
libs/react-spa/bff/src/lib/BffSessionExpiredModal.tsx (1)
1-1
: LGTM: Clean import statement
The import statement follows best practices for tree-shaking and bundling.
libs/react-spa/bff/src/lib/BffNewUserSameSessionModal.tsx (2)
1-1
: LGTM! Clean import statement.
The import is specific and follows good tree-shaking practices.
13-25
: LGTM! Clean component implementation with a note about i18n.
The component is well-structured and follows React best practices. The comment about translation limitations is helpful.
Consider documenting a future TODO to implement translations once user locale becomes available.
libs/react-spa/bff/src/lib/BffProblemTemplateModal.tsx (1)
17-43
: LGTM! The component meets reusability requirements
The component is well-designed for reuse across NextJS apps:
- Uses shared UI components from island-ui/core
- Implements proper TypeScript types
- Has a focused, single responsibility
- Follows proper tree-shaking practices with named exports
libs/react-spa/bff/src/lib/bff.utils.ts (1)
40-41
: LGTM! Well-defined TypeScript type alias
The UserCheckFn
type alias improves code maintainability and type safety by providing a clear contract for user comparison functions.
libs/react-spa/bff/src/lib/BffProvider.tsx (3)
6-6
: LGTM! Well-structured imports and state management.
The new imports and state management follow good practices:
- Modular component imports support tree-shaking
- Utility functions are properly segregated
- TypeScript types are properly utilized
Also applies to: 12-16, 35-35
228-237
: LGTM! Clean modal integration.
The render logic for the new user screen is well-structured and follows React best practices.
228-237
: Verify the user experience flow.
Please ensure that:
- The modal appears at the right moment during user switching
- Users understand why they need to reload the page
- The reload action maintains the user's context appropriately
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16951 +/- ##
==========================================
- Coverage 36.41% 36.41% -0.01%
==========================================
Files 6898 6900 +2
Lines 144874 144893 +19
Branches 41404 41411 +7
==========================================
+ Hits 52755 52758 +3
- Misses 92119 92135 +16
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 19 Total Test Services: 0 Failed, 18 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (3) |
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)
libs/react-spa/bff/src/lib/BffProvider.tsx (2)
51-58
: Consider simplifying the nested conditions.While the logic is correct, the nested conditions could be simplified for better readability.
Consider this alternative structure:
- if (isLoggedIn && event.data.type === BffBroadcastEvents.NEW_SESSION) { - if (isNewSession(state.userInfo, event.data.userInfo)) { - setSessionExpiredScreen(true) - } else if (isNewUserWithSameSession(state.userInfo, event.data.userInfo)) { - setNewUserScreen(true) - } + if (!isLoggedIn || event.data.type !== BffBroadcastEvents.NEW_SESSION) { + return; + } + + if (isNewSession(state.userInfo, event.data.userInfo)) { + setSessionExpiredScreen(true); + return; + } + + if (isNewUserWithSameSession(state.userInfo, event.data.userInfo)) { + setNewUserScreen(true); + }
Line range hint
1-258
: Well-structured provider implementation.The BffProvider maintains good separation of concerns and follows React best practices:
- Proper TypeScript usage throughout the component
- Clear state management and event handling
- Reusable design that can be shared across different NextJS apps
- Effective error handling and loading states
Consider adding JSDoc comments for the new functionality to improve documentation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
libs/react-spa/bff/src/lib/BffNewUserSameSessionModal.tsx
(1 hunks)libs/react-spa/bff/src/lib/BffProvider.tsx
(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/react-spa/bff/src/lib/BffNewUserSameSessionModal.tsx
🧰 Additional context used
📓 Path-based instructions (1)
libs/react-spa/bff/src/lib/BffProvider.tsx (1)
Pattern libs/**/*
: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (3)
libs/react-spa/bff/src/lib/BffProvider.tsx (3)
6-6
: LGTM! Import changes follow best practices.
The imports are properly structured for effective tree-shaking and maintain type safety.
Also applies to: 12-16
35-36
: LGTM! State management follows React best practices.
The new state variable is well-named and properly typed through inference.
228-231
: LGTM! Render logic maintains consistency.
The new modal rendering condition follows the established pattern and maintains the component's reusability across different NextJS apps.
} | ||
|
||
/** | ||
* Checks if the user is a new user with the same session |
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.
I'm concerned about this "new user with the same session" as IDS should be making new sessions.
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.
Yes exactly
New BFF modal for user switch
What
New modal when user switches from child to self. This is more relevant when mínarsíður makes the bff transition.
Why
When switching from child to user the other tabs where not detecting it.
I added a new modal telling the user that new user switch had happened.
Screenshots / Gifs
Checklist:
Summary by CodeRabbit