Feat/006 jan 19 create use project risks #496
Conversation
WalkthroughThis pull request introduces a new custom hook Changes
Sequence DiagramsequenceDiagram
participant Component
participant useProjectRisks
participant API
Component->>useProjectRisks: Initialize with projectId
useProjectRisks->>API: Fetch project risks
API-->>useProjectRisks: Return risks data
useProjectRisks->>useProjectRisks: Categorize risks
useProjectRisks-->>Component: Return risks data, loading state, error
Possibly related PRs
Suggested labels
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: 3
🧹 Nitpick comments (6)
Clients/src/application/hooks/useProjectRisks.tsx (2)
15-42: Consider using more specific types for certain fields.The interface uses string type for fields that could benefit from more specific types:
- Date fields like
deadlineanddate_of_assessmentcould useDate- Boolean fields like
risk_approvalcould useboolean- Numeric fields like
project_idcould usenumberexport interface ProjectRisk { id: number; project_id: number; risk_name: string; risk_owner: string; ai_lifecycle_phase: string; risk_description: string; risk_category: string; impact: string; assessment_mapping: string; controls_mapping: string; likelihood: string; severity: string; risk_level_autocalculated: string; review_notes: string; mitigation_status: string; current_risk_level: string; - deadline: string; + deadline: Date; mitigation_plan: string; implementation_strategy: string; mitigation_evidence_document: string; likelihood_mitigation: string; risk_severity: string; final_risk_level: string; - risk_approval: string; + risk_approval: boolean; approval_status: string; - date_of_assessment: string; + date_of_assessment: Date; }
95-101: Add explicit return type for better type safety.Define an interface for the hook's return type to ensure consistent usage across components.
+interface UseProjectRisksReturn { + projectRisks: ProjectRisk[]; + loadingProjectRisks: boolean; + error: string | boolean; + projectRisksSummary: RiskData; +} -const useProjectRisks = ({ id }: { id?: number }) => { +const useProjectRisks = ({ id }: { id?: number }): UseProjectRisksReturn => {Clients/src/presentation/pages/ProjectView/index.tsx (2)
16-20: Consider using nullish coalescing for projectId.The current implementation uses logical OR which could ignore valid project ID "0".
-const projectId = searchParams.get("projectId") || "1" +const projectId = searchParams.get("projectId") ?? "1"
37-42: Improve loading and error states UI.Consider using a proper loading spinner and error component for better user experience.
-return <Typography>Loading project risks...</Typography>; +return <CircularProgress sx={{ m: 2 }} />; -return <Typography>Error fetching project risks. {errorFetchingProjectRisks}</Typography>; +return ( + <Alert severity="error" sx={{ m: 2 }}> + Error fetching project risks: {errorFetchingProjectRisks} + </Alert> +);Clients/src/presentation/pages/ProjectView/Overview/index.tsx (2)
12-18: Consider moving RiskData type to a shared types file.The RiskData type is used across multiple components and should be in a shared location.
Consider moving it to
src/types/risk.types.ts.
30-30: Destructure only required props.The component only uses projectRisksSummary but destructures vendorRisks as well.
-const Overview: FC<OverviewProps> = memo(({ projectRisksSummary }) => { +const Overview: FC<OverviewProps> = memo(({ projectRisksSummary, vendorRisks }) => {Also applies to: 147-147
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Clients/src/application/hooks/useProjectRisks.tsx(1 hunks)Clients/src/presentation/components/Risks/index.tsx(1 hunks)Clients/src/presentation/pages/ProjectView/Overview/index.tsx(4 hunks)Clients/src/presentation/pages/ProjectView/RisksView/index.tsx(2 hunks)Clients/src/presentation/pages/ProjectView/index.tsx(5 hunks)
🔇 Additional comments (3)
Clients/src/presentation/pages/ProjectView/RisksView/index.tsx (1)
96-98: LGTM!The reformatting of
handleOpenOrClosemaintains the same functionality while improving code consistency.Clients/src/presentation/pages/ProjectView/index.tsx (1)
63-65: Verify empty array for vendorRisks.The Overview component receives an empty array for vendorRisks while the actual vendorRisks data is available in the project prop.
✅ Verification successful
Empty array for vendorRisks is correct
The Overview component expects an array of individual risks (RiskData[]), while the vendorRisks from project prop is a summary object containing aggregated statistics (veryHighRisks, highRisks, etc.). These serve different purposes in the UI, so passing an empty array is appropriate in this case.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if vendorRisks is used elsewhere in the codebase rg -A 5 "vendorRisks"Length of output: 13941
Clients/src/presentation/components/Risks/index.tsx (1)
4-4: LGTM!The import path update is correct and aligns with the new location of the RiskData type.
| @@ -167,7 +167,7 @@ const RisksView: FC<RisksViewProps> = memo( | |||
| data={tableData} | |||
| table="risksTable" | |||
| paginated | |||
| label={`${title} risks`} | |||
| label={`${title} risk`} | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Keep consistent pluralization in labels.
The label has been changed to singular form (${title} risk), but this creates inconsistency with:
- The title above the table which uses plural form (
{title} risks) - The table's content which displays multiple risks
- label={`${title} risk`}
+ label={`${title} risks`}📝 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.
| label={`${title} risk`} | |
| label={`${title} risks`} |
| const projectRisksSummary = projectRisks.reduce((acc, risk) => { | ||
| const riskLevel = risk.current_risk_level.charAt(0).toLowerCase() + risk.current_risk_level.slice(1); | ||
| const key = `${riskLevel.replace(/\s/g, '')}Risks` as keyof typeof acc; | ||
| acc[key] = acc[key] + 1; | ||
|
|
||
| return acc; | ||
| }, { | ||
| veryHighRisks: 0, | ||
| highRisks: 0, | ||
| mediumRisks: 0, | ||
| lowRisks: 0, | ||
| veryLowRisks: 0, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for risk levels and use an enum for type safety.
The current implementation assumes risk levels will always match expected values. Consider using an enum and validation to make this more robust.
+enum RiskLevel {
+ VeryHigh = 'Very High',
+ High = 'High',
+ Medium = 'Medium',
+ Low = 'Low',
+ VeryLow = 'Very Low'
+}
+const isValidRiskLevel = (level: string): level is RiskLevel => {
+ return Object.values(RiskLevel).includes(level as RiskLevel);
+}
const projectRisksSummary = projectRisks.reduce((acc, risk) => {
- const riskLevel = risk.current_risk_level.charAt(0).toLowerCase() + risk.current_risk_level.slice(1);
- const key = `${riskLevel.replace(/\s/g, '')}Risks` as keyof typeof acc;
+ if (!isValidRiskLevel(risk.current_risk_level)) {
+ console.warn(`Invalid risk level: ${risk.current_risk_level}`);
+ return acc;
+ }
+ const key = `${risk.current_risk_level.replace(/\s/g, '').toLowerCase()}Risks` as keyof typeof acc;
acc[key] = acc[key] + 1;
return acc;
}, {Committable suggestion skipped: line range outside the PR's diff.
| const useProjectRisks = ({ id }: { id?: number }) => { | ||
| const [projectRisks, setProjectRisks] = useState<ProjectRisk[]>([]); | ||
| const [loadingProjectRisks, setLoadingProjectRisks] = useState<boolean>(true); | ||
| const [error, setError] = useState<string | boolean>(false); | ||
|
|
||
| useEffect(() => { | ||
| const controller = new AbortController(); | ||
| const signal = controller.signal; | ||
| const routeUrl = id ? `/projectRisks/${id}` : '/projectRisks' | ||
|
|
||
| const updateProjectRisks = async () => { | ||
| setLoadingProjectRisks(true); | ||
| try { | ||
| const response = await getEntityById({ | ||
| routeUrl, | ||
| signal, | ||
| }) | ||
| if(response.data) { | ||
| setProjectRisks(Array.isArray(response.data) ? response.data : [response.data]) | ||
| } | ||
| } catch (err) { | ||
| if (err instanceof Error){ | ||
| setError(`Request failed: ${err.message}`) | ||
| } else { | ||
| setError(`Request failed`) | ||
| } | ||
| } finally { | ||
| setLoadingProjectRisks(false) | ||
| } | ||
| } | ||
| updateProjectRisks(); | ||
| return () => { | ||
| controller.abort(); | ||
| }; | ||
| }, [id]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and type safety.
Consider the following improvements:
- Add type validation for the API response
- Handle network errors specifically
- Add retry logic for transient failures
const updateProjectRisks = async () => {
setLoadingProjectRisks(true);
try {
const response = await getEntityById({
routeUrl,
signal,
})
- if(response.data) {
+ if (!response.data) {
+ throw new Error('No data received from API');
+ }
+ // Validate response type
+ if (!Array.isArray(response.data) && typeof response.data !== 'object') {
+ throw new Error('Invalid response format');
+ }
setProjectRisks(Array.isArray(response.data) ? response.data : [response.data])
- }
} catch (err) {
+ // Handle network errors
+ if (err instanceof TypeError && err.message === 'Failed to fetch') {
+ setError('Network error: Please check your connection');
+ return;
+ }
if (err instanceof Error){
setError(`Request failed: ${err.message}`)
} else {
setError(`Request failed`)
}
} finally {
setLoadingProjectRisks(false)
}
}📝 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 useProjectRisks = ({ id }: { id?: number }) => { | |
| const [projectRisks, setProjectRisks] = useState<ProjectRisk[]>([]); | |
| const [loadingProjectRisks, setLoadingProjectRisks] = useState<boolean>(true); | |
| const [error, setError] = useState<string | boolean>(false); | |
| useEffect(() => { | |
| const controller = new AbortController(); | |
| const signal = controller.signal; | |
| const routeUrl = id ? `/projectRisks/${id}` : '/projectRisks' | |
| const updateProjectRisks = async () => { | |
| setLoadingProjectRisks(true); | |
| try { | |
| const response = await getEntityById({ | |
| routeUrl, | |
| signal, | |
| }) | |
| if(response.data) { | |
| setProjectRisks(Array.isArray(response.data) ? response.data : [response.data]) | |
| } | |
| } catch (err) { | |
| if (err instanceof Error){ | |
| setError(`Request failed: ${err.message}`) | |
| } else { | |
| setError(`Request failed`) | |
| } | |
| } finally { | |
| setLoadingProjectRisks(false) | |
| } | |
| } | |
| updateProjectRisks(); | |
| return () => { | |
| controller.abort(); | |
| }; | |
| }, [id]) | |
| const useProjectRisks = ({ id }: { id?: number }) => { | |
| const [projectRisks, setProjectRisks] = useState<ProjectRisk[]>([]); | |
| const [loadingProjectRisks, setLoadingProjectRisks] = useState<boolean>(true); | |
| const [error, setError] = useState<string | boolean>(false); | |
| useEffect(() => { | |
| const controller = new AbortController(); | |
| const signal = controller.signal; | |
| const routeUrl = id ? `/projectRisks/${id}` : '/projectRisks' | |
| const updateProjectRisks = async () => { | |
| setLoadingProjectRisks(true); | |
| try { | |
| const response = await getEntityById({ | |
| routeUrl, | |
| signal, | |
| }) | |
| if (!response.data) { | |
| throw new Error('No data received from API'); | |
| } | |
| // Validate response type | |
| if (!Array.isArray(response.data) && typeof response.data !== 'object') { | |
| throw new Error('Invalid response format'); | |
| } | |
| setProjectRisks(Array.isArray(response.data) ? response.data : [response.data]) | |
| } catch (err) { | |
| // Handle network errors | |
| if (err instanceof TypeError && err.message === 'Failed to fetch') { | |
| setError('Network error: Please check your connection'); | |
| return; | |
| } | |
| if (err instanceof Error){ | |
| setError(`Request failed: ${err.message}`) | |
| } else { | |
| setError(`Request failed`) | |
| } | |
| } finally { | |
| setLoadingProjectRisks(false) | |
| } | |
| } | |
| updateProjectRisks(); | |
| return () => { | |
| controller.abort(); | |
| }; | |
| }, [id]) |
Describe your changes
Issue number
#392
Please ensure all items are checked off before requesting a review:
Screenshot
Summary by CodeRabbit
New Features
useProjectRisksto fetch and manage project risks dataBug Fixes
Refactor