Fix undefined subcontrols #1321
Conversation
HarshP4585
requested review from
charurajput16,
samuel-coutinho and
MuhammadKhalilzadeh
HarshP4585
added
bug
WalkthroughThe changes refactor the Changes
Sequence Diagram(s)sequenceDiagram
participant UI as ControlsTable
participant API as API Server
participant DB as Database
UI->>API: Fetch controls by category ID and projectFrameworkId
API->>DB: Query controls list with projectFrameworkId filter
DB-->>API: Return controls
UI->>API: On control row click, fetch detailed control by ID and projectFrameworkId
API->>DB: Query detailed control with subcontrol counts
DB-->>API: Return detailed control data
API-->>UI: Return detailed control data
UI->>UI: Pass detailed control as `data` prop to NewControlPane modal
UI->>UI: Render modal with control details
Assessment against linked issues
Poem
✨ 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 (
|
|
@HarshP4585 - Thank you Harsh! This is working now. |
| let _response = [] | ||
| for (let control of response) { | ||
| const subControlsResponse = await getEntityById({ | ||
| routeUrl: `eu-ai-act/controlById?controlId=${control.id}&projectFrameworkId=${projectFrameworkId}`, | ||
| }); | ||
| _response.push({...subControlsResponse.data, ...control}); | ||
| } | ||
| setControls(_response); |
There was a problem hiding this comment.
We have a few issues to address here. First, we are making multiple asynchronous requests inside a for loop, which results in sequential execution. This means that the next fetch will not start until the previous one has completed, significantly slowing down performance.
To improve this, we should create a specific endpoint that retrieves all the data from the controls and subcontrols in a single call.
Additionally, it would be beneficial to move the logic for calling the API to a dedicated hook, which we can name useControls.
There was a problem hiding this comment.
@samuel-coutinho Updated the route to get the controls by control category id and changes the UI code to only fetch the subControls if the modal is open.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Clients/src/presentation/pages/ComplianceTracker/1.0ComplianceTracker/ControlsTable.tsx(4 hunks)Servers/controllers/eu.ctrl.ts(4 hunks)Servers/utils/eu.utils.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Clients/src/presentation/pages/ComplianceTracker/1.0ComplianceTracker/ControlsTable.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
Servers/utils/eu.utils.ts (3)
Servers/database/db.ts (1)
sequelize(82-82)Servers/models/EU/controlEU.model.ts (1)
ControlEU(14-29)Servers/models/EU/subControlEU.model.ts (1)
SubcontrolEU(11-27)
🔇 Additional comments (9)
Servers/controllers/eu.ctrl.ts (3)
9-9: Import statement updated to include new utility function.The import now includes
getControlStructByControlCategoryIdForAProjectQuery, which is used in the updatedgetControlsByControlCategoryIdfunction. This aligns with the project-specific control querying functionality being added.
295-296: Improved type consistency with explicit integer parsing.Converting string numbers to integers using
parseInt()ensures consistent numeric types in the API response, which prevents potential type-related issues on the frontend.
316-317: Improved type consistency with explicit integer parsing.Similar to the assessment progress endpoint, explicitly parsing string numbers to integers ensures data type consistency in the API response.
Servers/utils/eu.utils.ts (6)
125-139: Added helper function for subcontrol calculations.This new utility function retrieves the count of total subcontrols and completed subcontrols for a given control. It properly encapsulates database query logic that's reused in multiple functions.
158-160: Enhanced control data with subcontrol counts.The
getControlByIdForProjectQueryfunction now augments the control data with parsed integer properties for subcontrol counts. This provides important metadata that helps the frontend display accurate progress information.
273-292: Added new function to get controls filtered by project framework.This function retrieves controls for a specific control category and project framework, and enriches each control with subcontrol counts. The implementation correctly joins the control structure with actual control data and includes ownership information.
614-616: Improved update function to avoid empty update queries.Adding an early return when there are no fields to update avoids running an unnecessary database query with an empty SET clause.
639-639: Updated function return type for consistency.Changed the return type from
Promise<SubcontrolEU | null>toPromise<SubcontrolEU>to match the actual behavior where the function never returns null (it returns the input object when there's nothing to update).
695-695: Fixed early return to maintain type consistency.Changed the early return from
nulltosubcontrol as SubcontrolEUto ensure the function always returns a SubcontrolEU object, which is consistent with the updated return type on line 639.
| const controlCategoryId = parseInt(req.params.id); | ||
| const controls = await getControlStructByControlCategoryIdQuery(controlCategoryId); | ||
| const projectFrameworkId = parseInt(req.query.projectFrameworkId as string); | ||
| const controls = await getControlStructByControlCategoryIdForAProjectQuery(controlCategoryId, projectFrameworkId); | ||
| return res.status(200).json(controls); |
There was a problem hiding this comment.
Added project framework filtering to control retrieval.
The function now accepts a projectFrameworkId query parameter and uses it to filter controls by both category and project framework. This resolves the issue where sub-controls were undefined due to missing project context.
You should add validation for the projectFrameworkId parameter to handle cases where it might be missing or invalid:
try {
const controlCategoryId = parseInt(req.params.id);
const projectFrameworkId = parseInt(req.query.projectFrameworkId as string);
+ if (isNaN(projectFrameworkId)) {
+ return res.status(400).json(STATUS_CODE[400]("Invalid projectFrameworkId"));
+ }
const controls = await getControlStructByControlCategoryIdForAProjectQuery(controlCategoryId, projectFrameworkId);
return res.status(200).json(controls);
} catch (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.
| const controlCategoryId = parseInt(req.params.id); | |
| const controls = await getControlStructByControlCategoryIdQuery(controlCategoryId); | |
| const projectFrameworkId = parseInt(req.query.projectFrameworkId as string); | |
| const controls = await getControlStructByControlCategoryIdForAProjectQuery(controlCategoryId, projectFrameworkId); | |
| return res.status(200).json(controls); | |
| try { | |
| const controlCategoryId = parseInt(req.params.id); | |
| const projectFrameworkId = parseInt(req.query.projectFrameworkId as string); | |
| if (isNaN(projectFrameworkId)) { | |
| return res.status(400).json(STATUS_CODE[400]("Invalid projectFrameworkId")); | |
| } | |
| const controls = await getControlStructByControlCategoryIdForAProjectQuery( | |
| controlCategoryId, | |
| projectFrameworkId | |
| ); | |
| return res.status(200).json(controls); | |
| } catch (error) { | |
| next(error); | |
| } |
| @@ -125,16 +130,9 @@ const ControlsTable: React.FC<ControlsTableProps> = ({ | |||
| setLoading(true); | |||
| try { | |||
| const response = await getEntityById({ | |||
| routeUrl: `/eu-ai-act/controls/byControlCategoryId/${controlCategoryId}`, | |||
| routeUrl: `/eu-ai-act/controls/byControlCategoryId/${controlCategoryId}?projectFrameworkId=${projectFrameworkId}`, | |||
There was a problem hiding this comment.
Good work. Now we could move the logic inside this useEffect to its hook, useControls, and call it here to obtain controls.
Screen.Recording.2025-05-10.at.12.07.06.AM.mov |
Describe your changes
Write your issue number after "Fixes "
Fixes #1320
Please ensure all items are checked off before requesting a review:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor