[Netmanager-app]improved on the grids hooks #2544
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new React component, Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant AnalyticsPage as Analytics Index
participant Loader as AnalyticsSkeleton
participant Data as Data Fetcher
User->>AnalyticsPage: Request analytics dashboard
AnalyticsPage->>Data: Trigger data fetch
Data-->>AnalyticsPage: Data Loading (pending)
AnalyticsPage->>Loader: Render skeleton UI for loading state
Data-->>AnalyticsPage: Data available
AnalyticsPage->>User: Render complete analytics dashboard
Possibly related PRs
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
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 (5)
src/netmanager-app/components/Analytics/analytics-skeleton.tsx (1)
44-44: Minor cleanup: Remove trailing empty line.There's an empty line at the end of the file that could be removed for consistency.
} - 44src/netmanager-app/core/hooks/useGrids.ts (1)
23-33: Type safety reduced in query callbacks.The onSuccess and onError callbacks now use less specific types (
anyandErrorinstead of specific types likeAxiosError<ErrorResponse>). While this works functionally, it reduces type safety which could lead to runtime errors if the response structure changes.- onSuccess: (data: any) => { + onSuccess: (data: { grids: Grid[] }) => { dispatch(setGrids(data.grids)); }, - onError: (error: Error) => { + onError: (error: AxiosError<ErrorResponse>) => { - dispatch(setError(error.message)); + dispatch(setError(error.response?.data?.message || error.message)); },src/netmanager-app/components/Analytics/index.tsx (3)
19-19: Commented import should be enabled or removed.The AnalyticsSkeleton component is imported but commented out. Either use it to replace the simple loading spinner or remove the import if it's not needed.
-// import { AnalyticsSkeleton } from "./analytics-skeleton" +import { AnalyticsSkeleton } from "./analytics-skeleton"Then replace the simple loading spinner with the skeleton UI:
- if (isLoading) { - return ( - <div className="flex items-center justify-center min-h-screen"> - <Loader2 className="w-8 h-8 animate-spin" /> - </div> - ); - } + if (isLoading) { + return <AnalyticsSkeleton /> + }
70-71: Use optional chaining for improved safety.The static analysis tool correctly identified that line 70 would benefit from using optional chaining instead of the current approach.
- : (cohorts && cohorts[0]) || null, + : cohorts?.[0] || null,🧰 Tools
🪛 Biome (1.9.4)
[error] 70-70: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
74-90: Remove or uncomment the transformation code.This commented-out code block for data transformation should either be:
- Implemented if it's still needed
- Removed if it's obsolete
- Documented with a comment explaining why it's preserved
Consider removing it if it's no longer needed:
- // useEffect(()=>{ - // if(mapReadings) { - // const values = transformDataToGeoJson( - // mapReadings, - // { - // longitude: 'Longitude', - // latitude: 'Latitude' - // }, - // (feature) => [ - // feature.siteDetails && feature.siteDetails.approximate_longitude, - // feature.siteDetails && feature.siteDetails.approximate_latitude - // ] - // ); - - // setTransformedReadings(values); - // } - // }, [mapReadings]);Or if you're keeping it for reference, add a clear explanation:
+ // NOTE: This transformation is temporarily disabled until we implement the new map component // useEffect(()=>{ // if(mapReadings) { // ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/netmanager-app/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
src/netmanager-app/components/Analytics/analytics-skeleton.tsx(1 hunks)src/netmanager-app/components/Analytics/index.tsx(4 hunks)src/netmanager-app/components/Organization/create-group.tsx(1 hunks)src/netmanager-app/core/hooks/useGrids.ts(2 hunks)src/netmanager-app/core/hooks/useGroups.ts(1 hunks)src/netmanager-app/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/netmanager-app/package.json
- src/netmanager-app/components/Organization/create-group.tsx
- src/netmanager-app/core/hooks/useGroups.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/netmanager-app/components/Analytics/index.tsx
[error] 70-70: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
src/netmanager-app/components/Analytics/analytics-skeleton.tsx (1)
1-42: Well-structured skeleton component implementation.This new AnalyticsSkeleton component provides an excellent visual placeholder during data loading states. The grid-based layout effectively mirrors the actual dashboard structure, creating a smooth transition between loading and loaded states. The responsive design considerations (using sm/lg breakpoints) will ensure consistent user experience across device sizes.
src/netmanager-app/core/hooks/useGrids.ts (2)
10-10: LGTM: Import statement updated appropriately.The import statement has been simplified to include only the necessary Redux actions.
35-39: Improved return object with loading and error states.The hook now properly returns
isLoadinganderrorstates, which improves the API and makes it consistent with other hooks. This is a good improvement that gives consumers more information about the request status.src/netmanager-app/components/Analytics/index.tsx (3)
8-8: LGTM: Added Loader2 import for loading states.Adding the Loader2 component from lucide-react improves visual feedback during loading states.
36-41: Good consolidation of loading states.Well done on consolidating the loading states from multiple hooks. This creates a single source of truth for the component's loading state.
161-165: Great improvement to loading UI in Download button.The addition of a loading spinner and text change during download operations provides excellent visual feedback to users. This follows best practices for interactive UI elements.
| @@ -16,24 +16,21 @@ interface ErrorResponse { | |||
| } | |||
|
|
|||
| // Hook to get the grid summary | |||
| export const useGrids = (networkId: string) => { | |||
| export const useGrids = () => { | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Hook signature simplified - verify impact on consumers.
The function signature has been changed to remove the networkId parameter. This simplifies usage but may affect any components that were passing a non-active network ID.
🏁 Script executed:
#!/bin/bash
# Check for any instances where useGrids is called with parameters
rg "useGrids\(\S" --type tsx --type tsLength of output: 68
Below is a revised verification step to further check for any consumers that might still be passing parameters to the updated hook signature:
Next Steps:
- Please run the script above so we can confirm if any components are still calling
useGridswith an argument. - If the output shows any instances, those usages will need to be updated to remove the obsolete networkId parameter.
- If nothing is returned, it’s still a good idea to do a manual scan—especially in TSX files—to ensure there are no unintended regressions.
This additional verification is important because the earlier command failed due to the unrecognized file type "tsx". Once you’ve checked that no consumers pass parameters, the hook signature change should be safe.
Thanks for double-checking!
🏁 Script executed:
#!/bin/bash
# Check for any instances where useGrids is called with non-empty arguments
# (i.e. where there's something between the parentheses other than just whitespace).
rg "useGrids\\(\\s*[^)]" -g "*.ts" -g "*.tsx"Length of output: 285
Action Required: Update Consumer Usage of useGrids
It appears that although the hook signature was simplified (removing the networkId parameter), there is still a consumer in the codebase invoking it with an argument:
- File:
src/netmanager-app/components/export-data/ExportForm.tsx
Usage:const { grids } = useGrids(activeNetwork?.net_name || "");
Please remove the argument from this call to align with the updated hook signature. Verify that this change doesn't affect the component's logic or any dependent behavior.
| if (isLoading) { | ||
| return ( | ||
| <div className="flex items-center justify-center min-h-screen"> | ||
| <Loader2 className="w-8 h-8 animate-spin" /> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using the AnalyticsSkeleton component.
You've created a nice AnalyticsSkeleton component but are using a simple spinner here. For better user experience, consider using the skeleton UI which would provide a closer representation of the content being loaded.
- if (isLoading) {
- return (
- <div className="flex items-center justify-center min-h-screen">
- <Loader2 className="w-8 h-8 animate-spin" />
- </div>
- );
- }
+ if (isLoading) {
+ return <AnalyticsSkeleton />
+ }📝 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 (isLoading) { | |
| return ( | |
| <div className="flex items-center justify-center min-h-screen"> | |
| <Loader2 className="w-8 h-8 animate-spin" /> | |
| </div> | |
| ); | |
| } | |
| if (isLoading) { | |
| return <AnalyticsSkeleton /> | |
| } |
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
New Features
Chores