Fetch frameworks from backend #1312
Conversation
charurajput16
added
frontend
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant UI as ProjectFrameworks Component
participant Hook as useFrameworks Hook
participant Repo as getAllFrameworks
participant API as Backend API
UI->>Hook: useFrameworks()
activate Hook
Hook->>Repo: getAllFrameworks()
activate Repo
Repo->>API: GET /frameworks (with auth token)
API-->>Repo: Frameworks data
deactivate Repo
Repo-->>Hook: Frameworks data
Hook-->>UI: { frameworks, loading, error, refreshFrameworks }
deactivate Hook
UI->>UI: Render tabs and content based on frameworks data
Assessment against linked issues
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. 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 (8)
Clients/src/application/repository/entity.repository.ts (1)
290-297: Function implementation looks good, but JSDoc is missing.The implementation follows the repository pattern established in the codebase, but unlike other functions in this file, it's missing JSDoc documentation.
Add JSDoc documentation for consistency:
+/** + * Fetches all frameworks from the API. + * + * @param {RequestParams} params - The parameters for the request. + * @returns {Promise<any>} A promise that resolves to the list of frameworks. + * @throws Will throw an error if the request fails. + */ export async function getAllFrameworks({ authToken = getAuthToken(), }: RequestParams): Promise<any> { const response = await apiServices.get("/frameworks", { headers: { Authorization: `Bearer ${authToken}` }, }); return response.data; }Clients/src/application/hooks/useFrameworks.ts (2)
20-26: Consider more robust response validation.The current validation only checks if
response?.dataexists, but doesn't verify that it's an array of Framework objects.try { setLoading(true); const response = await getAllFrameworks({ routeUrl: "/frameworks" }); - if (response?.data) { + if (response?.data && Array.isArray(response.data)) { setFrameworks(response.data); setError(null); } else { - throw new Error("Invalid response format"); + throw new Error("Invalid response format: expected an array of frameworks"); } } catch (err) {
17-34: Consider adding pagination support for scalability.If the number of frameworks grows significantly, fetching all at once might become inefficient.
For better scalability, consider adding pagination support to the hook:
- Add pagination parameters to the hook interface
- Pass these parameters to the API call
- Return pagination metadata along with the frameworks
This would make the hook more future-proof as the application scales.
Clients/src/presentation/pages/ProjectView/ProjectFrameworks/index.tsx (5)
71-79: Show a proper loading state instead of “Coming Soon” while fetchingWhen
frameworksare still loading,currentFrameworkisundefined,
which currently yields the “Coming Soon” placeholder – a confusing message
for users who are simply waiting for data.- if (!currentFramework) { - return <ComingSoonMessage />; - } + if (loading) { + return ( + <VWSkeleton variant="rectangular" width="100%" height={400} /> + ); + } + + if (!currentFramework) { + return <ComingSoonMessage />; + }
80-80: Replace magic number with a named constantHard-coding
1makes the intent opaque and fragile. Extract a constant
(or better, fetch a flag from the backend) so the value lives in one place.+// Temporary until the backend delivers a “primary” flag +const EU_AI_ACT_ID = 1; ... - const isEUAIAct = currentFramework.id === 1; // EU AI Act framework ID + const isEUAIAct = currentFramework.id === EU_AI_ACT_ID;
116-131: Use semantic, focus-able elements for framework tabsRendering tabs as bare
<Box>elements hurts accessibility (no keyboard
focus, no ARIA semantics). Wrap them inButtonBase(or reuseTab)
to gain keyboard navigation and screen-reader support.-import { Box, Button, Tab, Typography, Stack, Alert } from '@mui/material'; +import { Box, Button, Tab, Typography, Stack, Alert, ButtonBase } from '@mui/material'; ... -<Box - key={fw.id} - onClick={() => handleFrameworkChange(fw.id)} - sx={getFrameworkTabStyle(isActive, idx === frameworks.length - 1)} -> - {fw.name} -</Box> +<ButtonBase + key={fw.id} + onClick={() => handleFrameworkChange(fw.id)} + sx={getFrameworkTabStyle(isActive, idx === frameworks.length - 1)} + disableRipple +> + {fw.name} +</ButtonBase>
133-135: Wire up the “Add new framework” buttonThe button is currently inert and may confuse users.
-<Button variant="contained" sx={addButtonStyle}> +<Button + variant="contained" + sx={addButtonStyle} + onClick={handleAddFramework} // TODO: implement +> Add new framework </Button>Consider disabling the button or adding a tooltip until the handler is
implemented.
158-162: Avoid rendering the same content twice
renderFrameworkContent()is executed for both panels on every render,
doubling work (API calls, heavy components, etc.). Compute once and pass
the result to the active panel:-<TabPanel value="compliance" sx={tabPanelStyle}> - {renderFrameworkContent()} -</TabPanel> -<TabPanel value="assessment" sx={tabPanelStyle}> - {renderFrameworkContent()} -</TabPanel> +{['compliance', 'assessment'].map(value => ( + <TabPanel key={value} value={value} sx={tabPanelStyle}> + {frameworkContent} + </TabPanel> +))}const frameworkContent = useMemo(renderFrameworkContent, [ project, currentFramework, tracker, loading, ]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Clients/src/application/hooks/useFrameworks.ts(1 hunks)Clients/src/application/repository/entity.repository.ts(1 hunks)Clients/src/domain/types/Framework.ts(1 hunks)Clients/src/presentation/pages/Home/1.0Home/index.tsx(5 hunks)Clients/src/presentation/pages/ProjectView/ProjectFrameworks/index.tsx(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
Clients/src/application/repository/entity.repository.ts (1)
Clients/src/infrastructure/api/networkServices.ts (1)
apiServices(63-206)
Clients/src/application/hooks/useFrameworks.ts (2)
Clients/src/domain/types/Framework.ts (1)
Framework(1-8)Clients/src/application/repository/entity.repository.ts (1)
getAllFrameworks(290-297)
Clients/src/presentation/pages/Home/1.0Home/index.tsx (1)
Clients/src/application/hooks/useFetchProjects.ts (1)
useProjectData(6-22)
🔇 Additional comments (8)
Clients/src/domain/types/Framework.ts (1)
2-7: Good update to accommodate backend data structure.The type changes properly align with the dynamic framework data being fetched from the backend. Changing
idfrom string to number matches the numeric IDs used in the framework system, and the additional required fields (name,description,created_at) provide better type safety for the fetched data.Clients/src/presentation/pages/Home/1.0Home/index.tsx (5)
15-15: Removed unused import.The unnecessary import space was properly removed.
33-36: Clean approach to disable code by commenting.Commenting out the state declarations is a clean way to remove the feature while preserving the code for potential future reference or rollback.
45-45: Updated destructuring to remove unused loading state.The loading state is no longer being used since the progress trackers were removed, so it was correctly removed from the destructuring.
68-75: Clean approach to disable API calls by commenting.Commenting out the API calls is consistent with removing the feature while preserving the code structure.
105-112: Consistently disabled all related API calls.All related API calls were properly commented out, maintaining consistency in the codebase.
Clients/src/application/hooks/useFrameworks.ts (2)
5-10: Well-defined interface for hook return values.The interface clearly defines the return structure of the hook, making it easy to understand and use.
12-46: Well-implemented custom hook with proper state management.This hook follows React best practices:
- Uses appropriate useState hooks for data, loading, and error states
- Implements useCallback for the fetch function to prevent unnecessary re-renders
- Properly handles loading states, errors, and success scenarios
- Returns a clean interface with the data and a refresh function
| export async function getAllFrameworks({ | ||
| authToken = getAuthToken(), | ||
| }: RequestParams): Promise<any> { | ||
| const response = await apiServices.get("/frameworks", { | ||
| headers: { Authorization: `Bearer ${authToken}` }, | ||
| }); | ||
| return response.data; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing error handling in getAllFrameworks function.
The function doesn't have any error handling, unlike other similar functions in this file that use try/catch blocks to log errors and rethrow them.
Add error handling for consistency with other repository functions:
export async function getAllFrameworks({
authToken = getAuthToken(),
}: RequestParams): Promise<any> {
+ try {
const response = await apiServices.get("/frameworks", {
headers: { Authorization: `Bearer ${authToken}` },
});
return response.data;
+ } catch (error) {
+ console.error("Error fetching frameworks:", error);
+ throw error;
+ }
}📝 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.
| export async function getAllFrameworks({ | |
| authToken = getAuthToken(), | |
| }: RequestParams): Promise<any> { | |
| const response = await apiServices.get("/frameworks", { | |
| headers: { Authorization: `Bearer ${authToken}` }, | |
| }); | |
| return response.data; | |
| } | |
| export async function getAllFrameworks({ | |
| authToken = getAuthToken(), | |
| }: RequestParams): Promise<any> { | |
| try { | |
| const response = await apiServices.get("/frameworks", { | |
| headers: { Authorization: `Bearer ${authToken}` }, | |
| }); | |
| return response.data; | |
| } catch (error) { | |
| console.error("Error fetching frameworks:", error); | |
| throw error; | |
| } | |
| } |
| // Set initial framework when frameworks are loaded | ||
| useEffect(() => { | ||
| if (frameworks.length > 0 && !selectedFrameworkId) { | ||
| setSelectedFrameworkId(frameworks[0].id); | ||
| } | ||
| }, [frameworks, selectedFrameworkId]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle framework list changes & invalid selections
useEffect only sets the initial framework when nothing is selected.
If the frameworks list refreshes (e.g. after refreshFrameworks) and the
previously-selected framework is no longer present, selectedFrameworkId
will keep pointing to a non-existing entry, leaving the UI in an
inconsistent state.
-useEffect(() => {
- if (frameworks.length > 0 && !selectedFrameworkId) {
- setSelectedFrameworkId(frameworks[0].id);
- }
-}, [frameworks, selectedFrameworkId]);
+useEffect(() => {
+ if (frameworks.length === 0) return;
+
+ // If nothing is selected **or** the selected framework disappeared,
+ // default to the first framework.
+ const frameworkExists = frameworks.some(fw => fw.id === selectedFrameworkId);
+ if (selectedFrameworkId === null || !frameworkExists) {
+ setSelectedFrameworkId(frameworks[0].id);
+ }
+}, [frameworks, selectedFrameworkId]);📝 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.
| // Set initial framework when frameworks are loaded | |
| useEffect(() => { | |
| if (frameworks.length > 0 && !selectedFrameworkId) { | |
| setSelectedFrameworkId(frameworks[0].id); | |
| } | |
| }, [frameworks, selectedFrameworkId]); | |
| // Set initial framework when frameworks are loaded | |
| useEffect(() => { | |
| if (frameworks.length === 0) return; | |
| // If nothing is selected **or** the selected framework disappeared, | |
| // default to the first framework. | |
| const frameworkExists = frameworks.some(fw => fw.id === selectedFrameworkId); | |
| if (selectedFrameworkId === null || !frameworkExists) { | |
| setSelectedFrameworkId(frameworks[0].id); | |
| } | |
| }, [frameworks, selectedFrameworkId]); |
MuhammadKhalilzadeh
left a comment
MuhammadKhalilzadeh
left a comment
There was a problem hiding this comment.
There are conflicts here @charurajput16 ❌
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
Clients/src/presentation/pages/ProjectView/ProjectFrameworks/index.tsx (1)
141-157:⚠️ Potential issueInconsistent variable usage
The code references
currentValueandcurrentTabswhich aren't defined in the provided code. These may be relics from the previous implementation or they exist elsewhere in the file. Please ensure these variables are properly defined.-<TabContext value={currentValue}> +<TabContext value={tracker}>-onChange={(_, v) => setCurrentValue(v)} +onChange={(_, v) => setTracker(v)}-{currentTabs.map((tab) => ( +{trackerTabs.map((tab) => (
♻️ Duplicate comments (1)
Clients/src/presentation/pages/ProjectView/ProjectFrameworks/index.tsx (1)
56-61: Handle framework list changes & invalid selections
useEffectonly sets the initial framework when nothing is selected.
If the frameworks list refreshes (e.g. afterrefreshFrameworks) and the
previously-selected framework is no longer present,selectedFrameworkId
will keep pointing to a non-existing entry, leaving the UI in an
inconsistent state.-useEffect(() => { - if (frameworks.length > 0 && !selectedFrameworkId) { - setSelectedFrameworkId(frameworks[0].id); - } -}, [frameworks, selectedFrameworkId]); +useEffect(() => { + if (frameworks.length === 0) return; + + // If nothing is selected **or** the selected framework disappeared, + // default to the first framework. + const frameworkExists = frameworks.some(fw => fw.id === selectedFrameworkId); + if (selectedFrameworkId === null || !frameworkExists) { + setSelectedFrameworkId(frameworks[0].id); + } +}, [frameworks, selectedFrameworkId]);
🧹 Nitpick comments (1)
Clients/src/presentation/pages/ProjectView/ProjectFrameworks/index.tsx (1)
82-82: Replace magic number with a constantUsing
1as a magic number for the EU AI Act framework ID reduces code readability and maintainability.- const isEUAIAct = currentFramework.id === 1; // EU AI Act framework ID + const EU_AI_ACT_FRAMEWORK_ID = 1; + const isEUAIAct = currentFramework.id === EU_AI_ACT_FRAMEWORK_ID;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Clients/src/presentation/pages/ProjectView/ProjectFrameworks/index.tsx(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Frontend Checks
Clients/src/presentation/pages/ProjectView/ProjectFrameworks/index.tsx
[error] 20-20: TypeScript error TS6133: 'ISO42001Annex' is declared but its value is never read.
🔇 Additional comments (7)
Clients/src/presentation/pages/ProjectView/ProjectFrameworks/index.tsx (7)
11-11: Good job implementing the dynamic framework loading!The addition of
useFrameworkshook is a great improvement to fetch frameworks from the backend instead of hardcoding them.
28-49: Nice reusable component for unavailable frameworksThe
ComingSoonMessagecomponent is well-structured with proper styling and informative messaging to let users know when a framework is not yet implemented.
64-67: Good use of memoization for performance optimizationUsing
useMemoto cache the current framework is a good practice for performance optimization, preventing unnecessary lookups during re-renders.
73-97: Well-structured conditional rendering logicThe
renderFrameworkContentfunction effectively centralizes the logic for rendering different framework content based on the selected framework, improving code organization and maintainability.
99-110: Good error handling with user-friendly recoveryExcellent implementation of error handling with a clear error message and a retry button, providing users with a good fallback experience.
118-133: Good implementation of loading state and dynamic framework tabsThe conditional rendering of skeleton loaders during loading and dynamic mapping of frameworks is well-implemented. This improves user experience by providing visual feedback during loading.
159-164: Clean code with DRY implementationUsing the
renderFrameworkContentfunction for both tab panels follows the DRY principle and ensures consistent rendering logic.
| import ISO42001Annex from "../../ISO/Annex"; | ||
| import ISO42001Clauses from "../../ISO/Clause"; |
There was a problem hiding this comment.
Remove unused imports
The ISO42001Annex import is declared but never used, as indicated by the pipeline failure. Remove this unused import and verify if ISO42001Clauses is also unused.
-import ISO42001Annex from "../../ISO/Annex";
-import ISO42001Clauses from "../../ISO/Clause";Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Frontend Checks
[error] 20-20: TypeScript error TS6133: 'ISO42001Annex' is declared but its value is never read.
|
@MuhammadKhalilzadeh - We can skip this PR, as I have handled issues in #1319 |
Describe your changes
Write your issue number after "Fixes "
Fixes #1308
Please ensure all items are checked off before requesting a review:
Summary by CodeRabbit