refactor: centralize hooks and add reusable useFetchData hook (#1867)
#2690
Conversation
…labs#1867) Moved role-based logic into a single reusable hook `useHasRole`. Refactored `useIsAdmin` and `useIsSuperAdmin` to use `useHasRole` internally. Ensures consistent logic for role checks across components. Preserves separate named hooks for component-level clarity. Reduces code duplication and improves maintainability. Closes: bluewave-labs#1867
AbhishekG-Codes
requested review from
ajhollid,
mohicody,
Br0wnHammer,
Owaiseimdad,
karenvicent and
shanikauwu1
as code owners
WalkthroughThis set of changes introduces a new generic Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant useFetchChecksTeam
participant useFetchData
participant networkService
Component->>useFetchChecksTeam: Call with params
useFetchChecksTeam->>useFetchData: Pass requestFn, deps, enabled, shouldRun
useFetchData->>networkService: Fetch data (based on requestFn)
networkService-->>useFetchData: Return response
useFetchData-->>useFetchChecksTeam: Return [data, count, isLoading, networkError]
useFetchChecksTeam-->>Component: Return hook result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Why did the Canadian developer refuse to use absolute imports? Because he thought it was too "American" to take shortcuts—he'd rather take the scenic route, eh! Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (4)
client/src/Pages/Uptime/Monitors/Components/SearchComponent/index.jsx (1)
1-6: Unify React imports for tidier codeYou can fold the two separate React imports into one:
-import { useState } from "react"; -import { useEffect, useRef } from "react"; +import { useState, useEffect, useRef } from "react";Purely cosmetic, but it trims a line and keeps things grouped—like Canadians and Americans sharing the same hockey rink, everyone gets along when they sit together.
client/src/Pages/StatusPage/Create/index.jsx (1)
13-14: Consider a path alias to replace the'../../../'drill-downThree
../hops work today, but they tend to break the moment someone moves the file. Setting up a webpack/TS path alias (e.g.@hooks/useMonitorsFetch) would make the import stable and far easier to read. It also keeps IDE refactors happier than a Canadian in a maple-syrup store—and that’s saying something.client/src/Hooks/useFetchData.js (2)
23-28: Consider making response structure handling more generic.The current implementation has hardcoded logic for handling
res.data.data.checkswhich makes the hook less reusable. This assumes a specific API response structure that might not apply to all use cases.Consider making this more flexible by accepting an optional
dataTransformerfunction:export const useFetchData = ({ requestFn, enabled = true, deps = [], shouldRun = true, + dataTransformer, // optional function to transform response data }) => { // ... existing code ... try { setIsLoading(true); const res = await requestFn(); - // Support both list and summary - if (res.data?.data?.checks) { - setData(res.data.data.checks); - setCount(res.data.data.checksCount); - } else { - setData(res.data.data); - } + if (dataTransformer) { + const transformed = dataTransformer(res); + setData(transformed.data); + setCount(transformed.count); + } else { + // Support both list and summary (backward compatibility) + if (res.data?.data?.checks) { + setData(res.data.data.checks); + setCount(res.data.data.checksCount); + } else { + setData(res.data.data); + } + } } catch (error) {
40-40: Consider using object return instead of array destructuring.Returning an array
[data, count, isLoading, networkError]makes the hook less self-documenting and more prone to ordering mistakes. Like a Canadian trying to order Tim Hortons in America - you might get the wrong thing in the wrong order! 🍁Consider returning an object for better developer experience:
- return [data, count, isLoading, networkError]; + return { data, count, isLoading, networkError };This would make consuming code more readable and less error-prone:
// Instead of remembering order: [data, count, loading, error] const [data, count, isLoading, networkError] = useFetchData({...}); // More self-documenting: const { data, count, isLoading, networkError } = useFetchData({...});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
client/src/Hooks/checkHooks.js(1 hunks)client/src/Hooks/editUser.js(1 hunks)client/src/Hooks/useCreateStatusPage.jsx(1 hunks)client/src/Hooks/useFetchData.js(1 hunks)client/src/Hooks/useMonitorsFetch.jsx(1 hunks)client/src/Hooks/useStatusPagesFetch.jsx(1 hunks)client/src/Hooks/useValidatePassword.jsx(1 hunks)client/src/Pages/Infrastructure/Details/Components/AreaChartBoxes/InfraAreaChart.jsx(1 hunks)client/src/Pages/Infrastructure/Details/Components/AreaChartBoxes/index.jsx(1 hunks)client/src/Pages/Infrastructure/Details/Components/BaseContainer/index.jsx(1 hunks)client/src/Pages/Infrastructure/Details/Components/GaugeBoxes/index.jsx(1 hunks)client/src/Pages/Infrastructure/Details/Components/StatusBoxes/index.jsx(1 hunks)client/src/Pages/StatusPage/Create/index.jsx(1 hunks)client/src/Pages/Uptime/Monitors/Components/SearchComponent/index.jsx(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: ceskyDJ
PR: bluewave-labs/Checkmate#2410
File: client/src/Pages/Settings/SettingsAbout.jsx:23-25
Timestamp: 2025-06-08T14:56:37.007Z
Learning: User ceskyDJ appropriately manages PR scope by focusing on i18n changes and not addressing unrelated pre-existing technical debt in the same PR, demonstrating good software development practices.
client/src/Hooks/checkHooks.js (1)
Learnt from: Jesulayomy
PR: #2664
File: client/src/Pages/Uptime/Create/index.jsx:92-96
Timestamp: 2025-07-24T17:52:55.506Z
Learning: In the Uptime monitor components, the formatAndSet function was deprecated due to rendering issues caused by state mutations. The current approach stores intervals internally in milliseconds (API format) but converts for display using const displayInterval = monitor?.interval / MS_PER_MINUTE || 1; and converts user input back to milliseconds using value = value * MS_PER_MINUTE in the onChange handler.
client/src/Pages/Infrastructure/Details/Components/AreaChartBoxes/InfraAreaChart.jsx (1)
Learnt from: Jesulayomy
PR: #2664
File: client/src/Pages/Uptime/Create/index.jsx:92-96
Timestamp: 2025-07-24T17:52:55.506Z
Learning: In the Uptime monitor components, the formatAndSet function was deprecated due to rendering issues caused by state mutations. The current approach stores intervals internally in milliseconds (API format) but converts for display using const displayInterval = monitor?.interval / MS_PER_MINUTE || 1; and converts user input back to milliseconds using value = value * MS_PER_MINUTE in the onChange handler.
client/src/Pages/Infrastructure/Details/Components/AreaChartBoxes/index.jsx (1)
Learnt from: Jesulayomy
PR: #2664
File: client/src/Pages/Uptime/Create/index.jsx:92-96
Timestamp: 2025-07-24T17:52:55.506Z
Learning: In the Uptime monitor components, the formatAndSet function was deprecated due to rendering issues caused by state mutations. The current approach stores intervals internally in milliseconds (API format) but converts for display using const displayInterval = monitor?.interval / MS_PER_MINUTE || 1; and converts user input back to milliseconds using value = value * MS_PER_MINUTE in the onChange handler.
client/src/Hooks/useMonitorsFetch.jsx (1)
Learnt from: Jesulayomy
PR: #2664
File: client/src/Pages/Uptime/Create/index.jsx:92-96
Timestamp: 2025-07-24T17:52:55.506Z
Learning: In the Uptime monitor components, the formatAndSet function was deprecated due to rendering issues caused by state mutations. The current approach stores intervals internally in milliseconds (API format) but converts for display using const displayInterval = monitor?.interval / MS_PER_MINUTE || 1; and converts user input back to milliseconds using value = value * MS_PER_MINUTE in the onChange handler.
client/src/Pages/Uptime/Monitors/Components/SearchComponent/index.jsx (1)
Learnt from: Jesulayomy
PR: #2664
File: client/src/Pages/Uptime/Create/index.jsx:92-96
Timestamp: 2025-07-24T17:52:55.506Z
Learning: In the Uptime monitor components, the formatAndSet function was deprecated due to rendering issues caused by state mutations. The current approach stores intervals internally in milliseconds (API format) but converts for display using const displayInterval = monitor?.interval / MS_PER_MINUTE || 1; and converts user input back to milliseconds using value = value * MS_PER_MINUTE in the onChange handler.
client/src/Pages/Infrastructure/Details/Components/GaugeBoxes/index.jsx (1)
Learnt from: Jesulayomy
PR: #2664
File: client/src/Pages/Uptime/Create/index.jsx:92-96
Timestamp: 2025-07-24T17:52:55.506Z
Learning: In the Uptime monitor components, the formatAndSet function was deprecated due to rendering issues caused by state mutations. The current approach stores intervals internally in milliseconds (API format) but converts for display using const displayInterval = monitor?.interval / MS_PER_MINUTE || 1; and converts user input back to milliseconds using value = value * MS_PER_MINUTE in the onChange handler.
client/src/Pages/StatusPage/Create/index.jsx (1)
Learnt from: Jesulayomy
PR: #2664
File: client/src/Pages/Uptime/Create/index.jsx:92-96
Timestamp: 2025-07-24T17:52:55.506Z
Learning: In the Uptime monitor components, the formatAndSet function was deprecated due to rendering issues caused by state mutations. The current approach stores intervals internally in milliseconds (API format) but converts for display using const displayInterval = monitor?.interval / MS_PER_MINUTE || 1; and converts user input back to milliseconds using value = value * MS_PER_MINUTE in the onChange handler.
client/src/Pages/Infrastructure/Details/Components/StatusBoxes/index.jsx (1)
Learnt from: Jesulayomy
PR: #2664
File: client/src/Pages/Uptime/Create/index.jsx:92-96
Timestamp: 2025-07-24T17:52:55.506Z
Learning: In the Uptime monitor components, the formatAndSet function was deprecated due to rendering issues caused by state mutations. The current approach stores intervals internally in milliseconds (API format) but converts for display using const displayInterval = monitor?.interval / MS_PER_MINUTE || 1; and converts user input back to milliseconds using value = value * MS_PER_MINUTE in the onChange handler.
🧬 Code Graph Analysis (1)
client/src/Hooks/useFetchData.js (5)
client/src/Hooks/useCertificateFetch.jsx (1)
isLoading(13-13)client/src/Hooks/checkHooks.js (8)
isLoading(51-51)isLoading(73-73)requestFn(12-12)requestFn(26-26)requestFn(40-40)deps(10-10)deps(24-24)deps(38-38)client/src/Hooks/useCreateStatusPage.jsx (2)
isLoading(6-6)networkError(7-7)client/src/Hooks/useStatusPagesFetch.jsx (2)
isLoading(9-9)networkError(10-10)client/src/Hooks/useMonitorsFetch.jsx (2)
isLoading(10-10)networkError(11-11)
🔇 Additional comments (18)
client/src/Pages/Uptime/Monitors/Components/SearchComponent/index.jsx (1)
4-4: Import path correction looks goodThe updated relative path climbs 5 levels, which correctly matches the folder depth of this component. Nothing else to tweak here—ship it!
client/src/Pages/Infrastructure/Details/Components/BaseContainer/index.jsx (1)
14-14: LGTM! Hook centralization looks good, eh!The import path update correctly reflects the movement of
useHardwareUtilsto the centralized Hooks directory. This change aligns perfectly with the PR objectives of organizing hooks for better reusability - much like how Canadians organize their hockey sticks in a central location for the whole team to use!client/src/Pages/Infrastructure/Details/Components/AreaChartBoxes/InfraAreaChart.jsx (1)
7-7: Consistent import path update - approved!The import path change maintains consistency with the hook centralization effort. Like Americans organizing their tools in a centralized garage, this keeps everything neat and accessible!
client/src/Pages/Infrastructure/Details/Components/AreaChartBoxes/index.jsx (1)
14-14: Another clean import path update!The centralized hook import is implemented correctly and maintains the existing functionality. This refactoring is going smoother than a Canadian apologizing for bumping into a door!
client/src/Pages/Infrastructure/Details/Components/StatusBoxes/index.jsx (1)
8-8: Perfectly consistent with the refactoring pattern!The import path update follows the same centralization pattern as the other components. This level of consistency would make even the most meticulous American tax accountant proud!
client/src/Pages/Infrastructure/Details/Components/GaugeBoxes/index.jsx (1)
8-8: ✅ useHardwareUtils hook verified in centralized directory
- Confirmed
client/src/Hooks/useHardwareUtils.jsxexists and exports the hook.- Import in
client/src/Pages/Infrastructure/Details/Components/GaugeBoxes/index.jsxnow resolves correctly.All set—this refactoring is more organized than a line at Tim Hortons!
client/src/Hooks/useValidatePassword.jsx (1)
2-2: Import-path shortened; sanity-check that the file really movedThe new relative import jumps up a single level (
../Validation/validation). If theValidationfolder now lives alongsideHooksthis is perfect; if it remained at project root three levels up you’ll trip the bundler faster than a Canadian saying “sorry”. Double-check the folder layout or runtsc --noEmitto be sure.client/src/Hooks/useMonitorsFetch.jsx (1)
2-5: Updated imports look cleaner; verify resolve
networkServiceandcreateToastnow come from one-level-up paths. Looks good provided:
client/src/main.(js|ts)really exportsnetworkService.client/src/Utils/toastUtilsexists after the shuffle.A quick
npm run lintwill catch any dangling paths—saves more time than an American at a drive-thru.client/src/Hooks/editUser.js (1)
2-5: Path simplification across utilitiesNice consolidation. Just confirm that
../Utils/*and../Validation/validationare now colocated withHooks; otherwise imports will 404 harder than a maple-syrup recipe on an American BBQ site.client/src/Hooks/useCreateStatusPage.jsx (1)
2-3: Consistent path usage; quick smoke-test advisedSame note as above—run a build/test pass to ensure
../mainand../Utils/toastUtilsresolve correctly after the refactor. Better to catch it now than later when the Yankees (CI) start complaining.client/src/Hooks/useStatusPagesFetch.jsx (1)
2-4: Import tweaks mirror the new folder structureEverything reads fine; compile once to confirm nothing is missing from
../mainor../Utils/toastUtils. Think of it as checking your passport before crossing that friendly northern border.client/src/Hooks/useFetchData.js (3)
4-9: LGTM! Well-designed hook interface.The parameter structure with sensible defaults makes this hook flexible and reusable. The
shouldRunparameter adds a nice extra layer of conditional control beyond the basicenabledflag.
15-18: Good guard clause implementation.The early return prevents unnecessary API calls when conditions aren't met, which is efficient and prevents potential race conditions.
38-38: Dependency arrays usage reviewed – no infinite loops detectedI checked all
useFetchDatacalls inclient/src/Hooks/checkHooks.js:
- Lines 9–17:
const config = { ...params }; const deps = Object.values(config);- Lines 23–31:
const config = { ...params }; const deps = Object.values(config);- Lines 37–46:
const deps = [updateTrigger];React compares each dependency by value (
Object.is), so even thoughObject.values(config)returns a new array, its primitive elements stay the same unless parameters actually change. The explicit[updateTrigger]is also correct. No risk of infinite re-renders here—no changes required.We’re safer than an American at a Canadian hockey game, eh?
client/src/Hooks/checkHooks.js (4)
5-5: LGTM! Clean import of the new centralized hook.Good to see the new
useFetchDatahook being imported and ready to reduce code duplication across the hooks.
37-47: Well-implemented refactoring with stable dependencies.This hook maintains its original signature and uses a simple
[updateTrigger]dependency array, which is much more stable thanObject.values(). The refactoring looks clean and maintains the same functionality.
50-96: Good decision to leave these hooks unchanged.The
useResolveIncidentanduseAcknowledgeCheckshooks serve different purposes (mutation operations vs data fetching) and appropriately weren't refactored to useuseFetchData. This shows good judgment in the refactoring scope - like Americans knowing when not to add maple syrup to everything! 🇺🇸
8-19: No breaking changes fromuseFetchChecksTeamsignature updateAll existing calls to
useFetchChecksTeamalready pass a single parameter object, so the switch from destructured args toparamsis safe.• client/src/Pages/Incidents/Components/IncidentTable/index.jsx
– InvokesuseFetchChecksTeam({ status, sortOrder, limit, dateRange, filter })Looks like both our American and Canadian friends are already speaking the same “params” language—no updates needed!
client/src/Hooks/checkHooks.js
Outdated
| const useFetchChecksByMonitor = (params) => { | ||
| const config = { ...params }; | ||
| const deps = Object.values(config); | ||
|
|
||
| const config = { | ||
| monitorId, | ||
| type, | ||
| status, | ||
| sortOrder, | ||
| limit, | ||
| dateRange, | ||
| filter, | ||
| ack, | ||
| page, | ||
| rowsPerPage, | ||
| }; | ||
|
|
||
| try { | ||
| setIsLoading(true); | ||
| const res = await networkService.getChecksByMonitor(config); | ||
| setChecks(res.data.data.checks); | ||
| setChecksCount(res.data.data.checksCount); | ||
| } catch (error) { | ||
| setNetworkError(true); | ||
| createToast({ body: error.message }); | ||
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| }; | ||
|
|
||
| fetchChecks(); | ||
| }, [ | ||
| monitorId, | ||
| type, | ||
| status, | ||
| sortOrder, | ||
| limit, | ||
| dateRange, | ||
| filter, | ||
| ack, | ||
| page, | ||
| rowsPerPage, | ||
| enabled, | ||
| updateTrigger, | ||
| ]); | ||
|
|
||
| return [checks, checksCount, isLoading, networkError]; | ||
| const requestFn = () => networkService.getChecksByMonitor(config); | ||
|
|
||
| return useFetchData({ | ||
| requestFn, | ||
| enabled: config.enabled, | ||
| deps, | ||
| shouldRun: !!config.type, // only run if 'type' is truthy | ||
| }); | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Same signature and dependency issues as useFetchChecksTeam.
This hook has the same potential problems: breaking signature change and unstable dependencies from Object.values(config). However, the shouldRun: !!config.type logic is a nice addition for conditional fetching.
Check usage of this hook as well:
Apply the same dependency stabilization fix as suggested for useFetchChecksTeam.
🏁 Script executed:
#!/bin/bash
# Description: Find all usages of useFetchChecksByMonitor
# Expected: Verify current usage patterns and potential breaking changes
rg -A 5 -B 2 "useFetchChecksByMonitor" --type js --type jsx client/src/Length of output: 99
Let’s broaden the search across all file types—no provincial dialects needed, eh? Americans and Canadians both agree we shouldn’t miss any usage.
🏁 Script executed:
#!/bin/bash
# Search all occurrences of useFetchChecksByMonitor in client/src
rg -n -A 5 -B 2 useFetchChecksByMonitor client/src/Length of output: 3824
Stabilize dependencies in useFetchChecksByMonitor hook
The hook’s deps = Object.values(config) can reorder or pick up extra keys over time, leading to spurious re-fetches. Let’s explicitly declare the fields in a fixed order to match its callers and prevent breaking signature changes.
• File: client/src/Hooks/checkHooks.js (around lines 22–34)
• Change: Replace the generic Object.values(config) with an explicit array of all possible params.
- const deps = Object.values(config);
+ const deps = [
+ config.monitorId,
+ config.type,
+ config.status, // may be undefined in some callers
+ config.sortOrder,
+ config.limit,
+ config.dateRange, // only used by Uptime Details
+ ];This aligns with usages in both Uptime Details (which passes dateRange) and IncidentTable (which passes status). Now Americans and Canadians alike will see predictable fetch behavior—eh?
📝 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 useFetchChecksByMonitor = (params) => { | |
| const config = { ...params }; | |
| const deps = Object.values(config); | |
| const config = { | |
| monitorId, | |
| type, | |
| status, | |
| sortOrder, | |
| limit, | |
| dateRange, | |
| filter, | |
| ack, | |
| page, | |
| rowsPerPage, | |
| }; | |
| try { | |
| setIsLoading(true); | |
| const res = await networkService.getChecksByMonitor(config); | |
| setChecks(res.data.data.checks); | |
| setChecksCount(res.data.data.checksCount); | |
| } catch (error) { | |
| setNetworkError(true); | |
| createToast({ body: error.message }); | |
| } finally { | |
| setIsLoading(false); | |
| } | |
| }; | |
| fetchChecks(); | |
| }, [ | |
| monitorId, | |
| type, | |
| status, | |
| sortOrder, | |
| limit, | |
| dateRange, | |
| filter, | |
| ack, | |
| page, | |
| rowsPerPage, | |
| enabled, | |
| updateTrigger, | |
| ]); | |
| return [checks, checksCount, isLoading, networkError]; | |
| const requestFn = () => networkService.getChecksByMonitor(config); | |
| return useFetchData({ | |
| requestFn, | |
| enabled: config.enabled, | |
| deps, | |
| shouldRun: !!config.type, // only run if 'type' is truthy | |
| }); | |
| }; | |
| const useFetchChecksByMonitor = (params) => { | |
| const config = { ...params }; | |
| const deps = [ | |
| config.monitorId, | |
| config.type, | |
| config.status, // may be undefined in some callers | |
| config.sortOrder, | |
| config.limit, | |
| config.dateRange, // only used by Uptime Details | |
| ]; | |
| const requestFn = () => networkService.getChecksByMonitor(config); | |
| return useFetchData({ | |
| requestFn, | |
| enabled: config.enabled, | |
| deps, | |
| shouldRun: !!config.type, // only run if 'type' is truthy | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In client/src/Hooks/checkHooks.js around lines 22 to 34, replace the dynamic
deps array created by Object.values(config) with an explicit array listing all
expected parameter keys in a fixed order. This prevents re-fetches caused by
changing object key order or extra keys. Identify all possible params used by
callers (e.g., type, enabled, dateRange, status) and list them explicitly in the
deps array to stabilize the hook's dependencies.
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
Auto Pull Request Review from LlamaPReview
1. Overview
1.1 Core Changes
- Primary purpose and scope: Centralize data fetching logic into reusable
useFetchDatahook and refactor existing hooks to use this abstraction - Key components modified:
checkHooks.js,useFetchData.js, and multiple hook consumers - Cross-component impacts: Affects all components using data fetching hooks, standardizes API interaction patterns
- Business value alignment: Improves maintainability, reduces code duplication, and establishes consistent patterns for future development
1.2 Technical Architecture
- System design modifications: Introduces centralized data fetching layer that abstracts common API interaction patterns
- Component interaction changes: Hooks now depend on the new
useFetchDataabstraction rather than implementing their own fetching logic - Integration points impact: Standardizes how components interact with network services
- Dependency changes and implications: Reduces direct dependencies on networkService across components
2. Critical Findings
2.1 Must Fix (P0🔴)
Issue: Unstable dependency arrays in hook implementations
- Analysis Confidence: High
- Impact: Causes infinite re-renders and performance degradation due to new array references on each render
- Resolution: Replace
Object.values(config)with explicit primitive dependency lists in all hook implementations
Issue: Missing request cancellation mechanism
- Analysis Confidence: High
- Impact: Potential memory leaks and state updates on unmounted components
- Resolution: Implement AbortController pattern in
useFetchDatawith proper cleanup
Issue: Error handling regression in centralized hook
- Analysis Confidence: High
- Impact: Loss of error context from network service responses
- Resolution: Restore original error handling behavior that checks for response data structure
2.2 Should Fix (P1🟡)
Issue: Type safety concerns in response handling
- Analysis Confidence: High
- Impact: Runtime errors if API response structure changes unexpectedly
- Suggested Solution: Add response validation layer that verifies structure before processing
Issue: Missing test coverage for critical hook
- Analysis Confidence: High
- Impact: Increased risk of regressions in core data fetching abstraction
- Suggested Solution: Implement test cases for success/failure states, loading transitions, and dependency behaviors
Issue: Hardcoded response paths in data mapping
- Analysis Confidence: Medium
- Impact: Potential breaking changes if API response structures evolve
- Suggested Solution: Make response mapping configurable through hook parameters
2.3 Consider (P2🟢)
Area: Request cancellation coordination
- Analysis Confidence: Medium
- Improvement Opportunity: Implement request deduplication for identical in-flight requests to improve performance
Area: Response caching layer
- Analysis Confidence: Medium
- Improvement Opportunity: Add LRU cache option to avoid duplicate network requests and implement stale-while-revalidate pattern
Area: TypeScript migration
- Analysis Confidence: Low
- Improvement Opportunity: Add JSDoc types as interim solution and plan for full TypeScript conversion
2.4 Summary of Action Items
- Critical: Fix dependency arrays and add request cancellation (P0)
- High Priority: Implement response validation and add test coverage (P1)
- Medium Priority: Consider adding caching layer and request deduplication (P2)
- Long-term: Plan for TypeScript migration and performance monitoring
3. Technical Analysis
3.1 Code Logic Analysis
📁 client/src/Hooks/checkHooks.js - useFetchChecksTeam
- Submitted PR Code:
const useFetchChecksTeam = (params) => {
const config = { ...params };
const deps = Object.values(config); // Safe enough for now
const requestFn = () => networkService.getChecksByTeam(config);
return useFetchData({
requestFn,
enabled: config.enabled,
deps,
});
};- Analysis:
- Current dependency array implementation creates new references on each render
- Error handling loses context from network service responses
- No request cancellation mechanism
- Business logic remains consistent but with technical risks
- LlamaPReview Suggested Improvements:
const useFetchChecksTeam = (params) => {
// Stable dependencies
const deps = [
params.status,
params.sortOrder,
params.limit,
params.dateRange,
params.filter,
params.ack,
params.page,
params.rowsPerPage,
params.enabled,
params.updateTrigger
];
return useFetchData({
requestFn: () => networkService.getChecksByTeam(params),
deps,
responseMapper: (data) => ({
data: data.checks,
count: data.checksCount
}),
errorHandler: (error) =>
createToast({ body: error?.response?.data?.msg || error.message })
});
};- Improvement rationale:
- Prevents infinite re-renders with stable dependencies
- Restores proper error handling context
- Provides configurable response mapping
- Maintains consistent behavior with original implementation
📁 client/src/Hooks/useFetchData.js - useFetchData
- Submitted PR Code:
export const useFetchData = ({
requestFn,
enabled = true,
deps = [],
shouldRun = true,
}) => {
const [data, setData] = useState(undefined);
const [count, setCount] = useState(undefined);
const [isLoading, setIsLoading] = useState(false);
const [networkError, setNetworkError] = useState(false);
useEffect(() => {
const fetchData = async () => {
if (!enabled || !shouldRun) return;
try {
setIsLoading(true);
const res = await requestFn();
if (res.data?.data?.checks) {
setData(res.data.data.checks);
setCount(res.data.data.checksCount);
} else {
setData(res.data.data);
}
} catch (error) {
setNetworkError(true);
createToast({ body: error.message });
} finally {
setIsLoading(false);
}
};
fetchData();
}, deps);
return [data, count, isLoading, networkError];
};- Analysis:
- Missing request cancellation mechanism
- No response validation layer
- Hardcoded response paths
- Error handling loses context
- Business logic provides good abstraction but needs safety improvements
- LlamaPReview Suggested Improvements:
export const useFetchData = (config) => {
const [state, setState] = useState({
data: undefined,
count: undefined,
isLoading: false,
networkError: false
});
useEffect(() => {
const controller = new AbortController();
const fetchData = async () => {
try {
setState(prev => ({ ...prev, isLoading: true }));
const res = await config.requestFn({ signal: controller.signal });
const validatedData = validateResponse(res, config.responseSchema);
setState(prev => ({
...prev,
...mapResponse(validatedData, config.responseMapper),
networkError: false
}));
} catch (error) {
if (error.name !== 'AbortError') {
handleError(error, config.errorHandler);
setState(prev => ({ ...prev, networkError: true }));
}
} finally {
if (!controller.signal.aborted) {
setState(prev => ({ ...prev, isLoading: false }));
}
}
};
if (config.enabled && config.shouldRun) {
fetchData();
}
return () => controller.abort();
}, useDeepCompareMemo(config.deps));
return [state.data, state.count, state.isLoading, state.networkError];
};
// Helper functions would be defined elsewhere
function validateResponse(res, schema) { /* ... */ }
function mapResponse(data, mapper) { /* ... */ }
function handleError(error, handler) { /* ... */ }- Improvement rationale:
- Adds proper request cancellation
- Implements response validation
- Provides configurable error handling
- Uses safe dependency comparison
- Maintains all original functionality while being more robust
3.2 Key Quality Aspects
- System scalability considerations: The new hook architecture improves scalability by reducing code duplication and providing a consistent pattern for data fetching
- Performance bottlenecks and optimizations: Current implementation could benefit from request deduplication and response caching
- Testing strategy and coverage: Critical need for comprehensive test coverage of the new hook abstraction
- Documentation needs: Should document the new hook's contract, parameters, and expected behaviors
4. Overall Evaluation
- Technical assessment: Strong architectural improvement with some critical technical debt that needs addressing
- Business impact: Positive long-term maintainability benefits with some short-term risk from the identified issues
- Risk evaluation: Medium risk due to P0 issues that could cause performance problems and error handling regressions
- Notable positive aspects and good practices: Excellent architectural direction, good code organization improvements
- Implementation quality: Good conceptual implementation with some technical shortcomings in the current version
- Final recommendation: Request Changes - The architectural direction is excellent but critical technical issues need addressing before merging
💡 LlamaPReview Community
Have feedback on this AI Code review tool? Join our GitHub Discussions to share your thoughts and help shape the future of LlamaPReview.
There was a problem hiding this comment.
I approve of the idea of writing a more generic reusable hook, but why go through the extra trouble fo them wrapping that generic hook in another hook?
const useFetchChecksTeam = (params) => {
const config = { ...params };
const deps = Object.values(config); // Safe enough for now
const requestFn = () => networkService.getChecksByTeam(config);
return useFetchData({
requestFn,
enabled: config.enabled,
deps,
});
};
This seems redundant to me, why not just call useFetchData directly?
Additionally, accepting arbitrary params without any JSdoc makes using these hooks more error prone in my opinion. We should define what parameters we can accept and enforce as best we can so users of the hook don't have to work too hard.
If we're going to refactor the hooks, we may as well do it thoroughly and remove the wrapper hooks entirely unless there's a compelling reason not to.
If there is such a reason, please do let me know and we can discuss!
|
You're right that useFetchChecksTeam essentially wraps useFetchData, and that may seem redundant at first glance. But, I added this wrapper intentionally to maintain semantic clarity and abstraction at the call site. This also gives us flexibility going forward, for example, if the data transformation, error handling, or side-effects specific to this fetch change in the future, we can manage that within the wrapper without touching consuming components. That said, I do agree it’s important not to over-abstract it. If the main idea is to call useFetchData directly and remove these wrappers for simplicity, I’m happy to update accordingly. Also noted on the JSDoc comment, I’ll add proper documentation to clarify expected params and improve dev experience. Thanks again for pointing that out..... |
|
@AbhishekG-Codes I believe Alex's suggestion is valid. We can directly call the wrapper hooks with the dependency array where the function needs to be executed, which simplifies the process. Additionally, sending the dependencies as an array appears to be a good approach. However, we should include a dependencies checker function that can serve as an extra validation layer to ensure that the hooks do not re-render infinitely due to a specific property. Also, please make sure to run |
|
@AbhishekG-Codes any update? |
Thanks for the follow-up @Br0wnHammer! I’m currently wrapping up the suggested changes:
Will push the changes and update here by EOD. Appreciate the patience! |
|
Refactored hooks to accept dependency arrays as suggested. Added a dependency checker for validation. Also ran npm run format in the client directory as requested. Ready for review. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
client/src/Pages/Incidents/Components/IncidentTable/index.jsx (1)
88-88: Avoid reassigning the isLoading parameter.Reassigning function parameters is generally discouraged. It's like telling a Canadian that hockey is just "okay" - technically possible but not recommended! 🏒
Consider using a different variable name:
-isLoading = isLoadingTeam || isLoadingMonitor; +const isLoadingData = isLoadingTeam || isLoadingMonitor || isLoading;Then update the loading check on line 168:
-if (isLoading || resolveLoading) return <TableSkeleton />; +if (isLoadingData || resolveLoading) return <TableSkeleton />;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
client/src/Hooks/checkHooks.js(1 hunks)client/src/Hooks/useFetchData.js(1 hunks)client/src/Pages/Incidents/Components/IncidentTable/index.jsx(2 hunks)client/src/Pages/Incidents/index.jsx(2 hunks)client/src/Pages/Uptime/Details/index.jsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/src/Hooks/useFetchData.js
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ceskyDJ
PR: bluewave-labs/Checkmate#2410
File: client/src/Pages/Settings/SettingsAbout.jsx:23-25
Timestamp: 2025-06-08T14:56:37.007Z
Learning: User ceskyDJ appropriately manages PR scope by focusing on i18n changes and not addressing unrelated pre-existing technical debt in the same PR, demonstrating good software development practices.
📚 Learning: in the uptime monitor components, the `formatandset` function was deprecated due to rendering issues...
Learnt from: Jesulayomy
PR: bluewave-labs/Checkmate#2664
File: client/src/Pages/Uptime/Create/index.jsx:92-96
Timestamp: 2025-07-24T17:52:55.506Z
Learning: In the Uptime monitor components, the `formatAndSet` function was deprecated due to rendering issues caused by state mutations. The current approach stores intervals internally in milliseconds (API format) but converts for display using `const displayInterval = monitor?.interval / MS_PER_MINUTE || 1;` and converts user input back to milliseconds using `value = value * MS_PER_MINUTE` in the onChange handler.
Applied to files:
client/src/Pages/Uptime/Details/index.jsxclient/src/Pages/Incidents/index.jsxclient/src/Pages/Incidents/Components/IncidentTable/index.jsxclient/src/Hooks/checkHooks.js
🧬 Code Graph Analysis (1)
client/src/Pages/Incidents/Components/IncidentTable/index.jsx (1)
client/src/Hooks/useFetchData.js (2)
useFetchData(46-115)useFetchData(46-115)
🔇 Additional comments (2)
client/src/Pages/Incidents/index.jsx (1)
18-19: LGTM! Clean refactoring to generic hook.The migration from
useFetchChecksSummaryByTeamIdto the genericuseFetchDatahook is well implemented. The dependency array with just[updateTrigger]is stable and won't cause unnecessary re-renders - unlike what happens when Americans try to understand Canadian politeness, this won't loop infinitely! 🍁Also applies to: 43-46
client/src/Hooks/checkHooks.js (1)
54-54: Export statement correctly updated after hook removal.Good job cleaning up the exports after removing the data fetching hooks. The remaining action hooks (
useResolveIncidentanduseAcknowledgeChecks) appropriately stay in this file since they handle state mutations rather than data fetching.
AbhishekG-Codes
force-pushed
the
chore/refactor-hooks-organization-1867
branch
from
0116ab5 to
a40de13
Compare
Thanks for the feedback! @Br0wnHammer I've already addressed the following as part of the latest changes:
|
|
Hey @AbhishekG-Codes , After looking at this further I'm not convinced this is the direction we want to be taking this in. We are actively trying to move business logic out of the UI components, and having to import and use the network service in UI components feels like a step backwards to me. I don't want devs to have to do this in the UI component, they should simply be able to use the hook as they do now and get a value back. I'm all for refactoring the hooks to make them more reusable and predictable, and certainly for moving them to the correct directory, but we should aim to decrease complexity in the UI components. |
Describe your changes
Refactored the folder structure to improve code organization and maintainability.
changes were made to the logic or functionality of the application, moved repeated logic to single file called ./Hooks/useFetchData.js and was used in ./Hooks/checkHooks.js
Fixes #1867
Please ensure all items are checked off before requesting a review. "Checked off" means you need to add an "x" character between brackets so they turn into checkmarks.
(Do not skip this or your PR will be closed) I deployed the application locally.
yes. only folder structure and moving hook to centralized file for reuse.
(Do not skip this or your PR will be closed) I have performed a self-review and testing of my code.
yes
I have included the issue # in the PR.
yes
I have added i18n support to visible strings (N/A - no UI strings were touched).
I have not included any files that are not related to my pull request, including package-lock and package-json if dependencies have not changed.
I didn't use any hardcoded values.
I made sure font sizes, color choices etc are all referenced from the theme. (N/A)
My PR is granular and targeted to one specific feature (folder structure refactor).
yes
I ran
npm run formatin server and client directories, which automatically formats your code.yes
I took a screenshot or a video and attached to this PR if there is a UI change. (N/A)
(N/A)
Summary by CodeRabbit
New Features
Refactor
Style
Bug Fixes