[Hotfix] Fix current org change #2402
Conversation
|
Warning Rate limit exceeded@Codebmk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request introduces modifications to several components related to active group management and loading states. The changes primarily focus on enhancing the conditional rendering and data fetching logic across Changes
Sequence DiagramsequenceDiagram
participant User
participant OrganizationDropdown
participant ActiveGroupHook
participant DataComponents
User->>OrganizationDropdown: Select Organization
OrganizationDropdown->>ActiveGroupHook: Update Active Group
ActiveGroupHook-->>DataComponents: Trigger Loading State
DataComponents->>DataComponents: Render Loading Skeletons
ActiveGroupHook->>DataComponents: Provide Active Group Data
DataComponents->>DataComponents: Render Actual Content
Possibly related PRs
Suggested Reviewers
Poem
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: 2
🧹 Nitpick comments (2)
platform/src/core/hooks/useGetActiveGroupId.jsx (1)
39-41: Consistent string comparison.
Here, both sides use lowercase, which is good. However, ensure lines 17-19 match this convention for uniformity.platform/src/common/components/Dropdowns/OrganizationDropdown.jsx (1)
62-62: Consider enhancing user feedback on errors.
Right now, errors are only logged to the console. Providing a user-facing error or a toast could improve the experience ifupdateUserPreferencesfails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
platform/src/common/components/AQNumberCard/index.jsx(1 hunks)platform/src/common/components/Dropdowns/OrganizationDropdown.jsx(2 hunks)platform/src/common/components/Modal/dataDownload/modules/DataDownload.jsx(2 hunks)platform/src/core/hooks/useGetActiveGroupId.jsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test-analytics-platform-changes
- GitHub Check: build-push-deploy-analytics-platform-preview
🔇 Additional comments (14)
platform/src/core/hooks/useGetActiveGroupId.jsx (8)
1-1: Import statement looks good.
5-5: State initialization is straightforward.
Using a null default foractiveGroupis clear and easy to maintain.
10-12: Potential loading state ambiguity.
This approach setsloadingto false as soon asuserInfochanges, which may cause UI flicker ifuserInfoisn’t fully retrieved. Consider verifying that all relevant data is available before settingloadingto false.
15-16: Reactivating loading while searching is appropriate.
This ensures the UI reflects the new fetching operation in progress.
21-21: No issues with setting the active group.
The fallback logic below handles scenarios where no matching group is found.
23-23: Dependency array usage.
Properly referenceschartData?.organizationName.
29-30: Optional chaining is well-implemented.
This prevents runtime errors whenactiveGroupis null.
37-37: Organizational name check is valid.
Helps ensure we only proceed ifchartData.organizationNameis truthy.platform/src/common/components/Dropdowns/OrganizationDropdown.jsx (1)
89-89: Callback closure is correct.
No issues with the dependency array or function references.platform/src/common/components/AQNumberCard/index.jsx (1)
333-333: Unified loading condition is sensible.
Displaying a skeleton when eitherloadingorisFetchingActiveGroupis true prevents partial rendering.platform/src/common/components/Modal/dataDownload/modules/DataDownload.jsx (4)
61-61: Tracking active group loading is a valuable addition.
UsingisFetchingActiveGrouphere keeps load states in sync across components.
132-133: Guard clause prevents partial data fetch.
Avoids callingfetchSitesSummaryprematurely if the active group is still being determined.
136-138: Consistent case handling for group name.
Lowercasing the organization string for fetching is a solid approach to avoid mismatch errors.
141-141: Effect closure is correct.
The hook’s dependencies are properly accounted for.
| const matchingGroup = userInfo?.groups?.find( | ||
| (group) => group.grp_title.toLowerCase() === chartData?.organizationName, | ||
| ); |
There was a problem hiding this comment.
Case-insensitive matching discrepancy.
Currently, only group.grp_title is converted to lowercase. This may fail if chartData.organizationName contains uppercase letters.
Apply this fix to unify case handling:
- (group) => group.grp_title.toLowerCase() === chartData?.organizationName,
+ (group) => group.grp_title.toLowerCase() === chartData?.organizationName?.toLowerCase(),📝 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 matchingGroup = userInfo?.groups?.find( | |
| (group) => group.grp_title.toLowerCase() === chartData?.organizationName, | |
| ); | |
| const matchingGroup = userInfo?.groups?.find( | |
| (group) => group.grp_title.toLowerCase() === chartData?.organizationName?.toLowerCase(), | |
| ); |
| // Immediately update organization name | ||
| dispatch(setOrganizationName(group.grp_title)); | ||
| localStorage.setItem('activeGroup', JSON.stringify(group)); | ||
|
|
||
| // Dispatch preferences update asynchronously |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Synchronizing local and remote states.
Immediately updating local storage and Redux could lead to mismatches if the update request fails. Consider deferring local storage updates until the dispatch call succeeds, or handle failures more robustly.
|
New next-platform changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
platform/src/core/hooks/useGetActiveGroupId.jsx (2)
1-9: Consider adding TypeScript or PropTypes for better type safety.The hook handles complex state management and Redux interactions. Adding type definitions would improve maintainability and catch potential issues early.
Line range hint
1-89: Consider implementing a more robust state management pattern.The current implementation mixes multiple concerns:
- Loading state management
- Group matching logic
- Fallback handling
- Redux state access
Consider splitting this into smaller, more focused hooks:
useGroupLoadingfor loading stateuseGroupMatcherfor group matching logicuseGroupFallbackfor fallback handlingThis would improve testability and maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
platform/src/core/hooks/useGetActiveGroupId.jsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-push-deploy-analytics-platform-preview
🔇 Additional comments (1)
platform/src/core/hooks/useGetActiveGroupId.jsx (1)
17-21:⚠️ Potential issueAdd null checks to prevent potential runtime errors.
The group matching logic could throw if
chartData.organizationNameis undefined.const matchingGroup = userInfo?.groups?.find( (group) => group.grp_title.toLowerCase() === - chartData?.organizationName.toLowerCase(), + chartData?.organizationName?.toLowerCase() ?? '', );Likely invalid or redundant comment.
| id: activeGroup?.id || null, | ||
| title: activeGroup?.grp_title || null, |
There was a problem hiding this comment.
Inconsistent ID property access across the hook.
The hook uses both _id and id properties when returning group data. This inconsistency could lead to bugs.
- id: activeGroup?.id || null,
+ id: activeGroup?._id || null,Also, consider consolidating the return object creation into a helper function to ensure consistency:
const createReturnObject = (group, loading, userInfo) => ({
loading,
id: group?._id || null,
title: group?.grp_title || null,
userID: userInfo?._id || null,
groupList: userInfo?.groups || [],
});Also applies to: 49-52
| useEffect(() => { | ||
| setLoading(false); | ||
| }, [userInfo]); |
There was a problem hiding this comment.
Potential race condition in loading state management.
The first useEffect blindly sets loading to false whenever userInfo changes, which could interfere with the loading state managed by the second useEffect. Consider removing this useEffect and handling loading state only in the group matching logic.
- useEffect(() => {
- setLoading(false);
- }, [userInfo]);| if (chartData.organizationName) { | ||
| const storedGroupInUserGroups = userInfo.groups.find( | ||
| (group) => group._id === activeGroupFromStorage._id, | ||
| (group) => | ||
| group.grp_title.toLowerCase() === | ||
| chartData.organizationName.toLowerCase(), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate group matching logic detected.
The case-insensitive group matching logic is duplicated. Consider extracting it into a helper function.
+ const findGroupByOrgName = (groups, orgName) =>
+ groups?.find(group =>
+ group.grp_title.toLowerCase() === orgName?.toLowerCase()
+ );
- const storedGroupInUserGroups = userInfo.groups.find(
- (group) =>
- group.grp_title.toLowerCase() ===
- chartData.organizationName.toLowerCase(),
- );
+ const storedGroupInUserGroups = findGroupByOrgName(
+ userInfo.groups,
+ chartData.organizationName
+ );📝 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.
| if (chartData.organizationName) { | |
| const storedGroupInUserGroups = userInfo.groups.find( | |
| (group) => group._id === activeGroupFromStorage._id, | |
| (group) => | |
| group.grp_title.toLowerCase() === | |
| chartData.organizationName.toLowerCase(), | |
| ); | |
| const findGroupByOrgName = (groups, orgName) => | |
| groups?.find(group => | |
| group.grp_title.toLowerCase() === orgName?.toLowerCase() | |
| ); | |
| if (chartData.organizationName) { | |
| const storedGroupInUserGroups = findGroupByOrgName( | |
| userInfo.groups, | |
| chartData.organizationName | |
| ); |
|
New next-platform changes available for preview here |
|
New next-platform changes available for preview here |
|
New next-platform changes available for preview here |
Summary of Changes (What does this PR do?)
Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Screenshots (optional)
Summary by CodeRabbit
Bug Fixes
Refactor