317 iterating compliance store in db #336
Conversation
MuhammadKhalilzadeh
added
frontend
WalkthroughThe pull request introduces several changes across multiple components and files in the codebase. Key modifications include updates to dropdown options in the Changes
Possibly related PRs
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: 23
🧹 Outside diff range and nitpick comments (16)
Servers/controllers/controlCategory.ctrl.ts (1)
28-28: Simplify boolean condition checksYou can simplify
if (MOCKDATA_ON === true)toif (MOCKDATA_ON).Apply this diff:
- if (MOCKDATA_ON === true) { + if (MOCKDATA_ON) {Also applies to: 65-65, 85-85, 109-109
Servers/controllers/project.ctrl.ts (1)
241-241: Avoid usingconsole.login production codeUsing
console.logcan clutter the console and may expose sensitive data. Use a proper logging library with appropriate log levels.Remove or replace
console.logstatements:- console.log("subcontrolToSave : ", subcontrolToSave);Also applies to: 283-283
Servers/routes/project.route.ts (2)
9-9: Ensure consistent route organization and uncomment authenticationThe authentication middleware is commented out. If authentication is required, consider uncommenting it. Additionally, the new route
/saveControlsshould be appropriately placed and documented.Uncomment
authenticateJWTif needed and organize the routes for better readability.Also applies to: 21-21
Line range hint
263-263: Fix incorrect HTTP method commentIn line 21, the comment indicates
// POSTs, but the route for/saveControlsis added after this comment. Ensure that comments accurately reflect the code structure.Move the comment if necessary to include the new POST route.
Servers/utils/controlCategory.util.ts (1)
1-52: Consider implementing database transactionsFor data consistency, consider implementing database transactions when multiple operations need to be performed atomically. This is especially important in a compliance system.
Would you like me to provide an example implementation using transactions?
Servers/utils/subControl.utils.ts (2)
Line range hint
42-52: Validate input parameters before database operationAdd validation for required fields and data types before executing the database query to prevent invalid data from being stored.
+ // Validate required fields + if (!controlId || !subcontrol.status || !subcontrol.owner) { + throw new Error('Missing required fields: controlId, status, and owner are mandatory'); + } + + // Validate date format + if (subcontrol.dueDate && !(subcontrol.dueDate instanceof Date)) { + throw new Error('Invalid date format for dueDate'); + } + const result = await pool.query( `INSERT INTO subcontrols (...) VALUES (...) RETURNING *`, [ controlId, subcontrol.status, // ... ] );
Line range hint
20-109: Consider implementing database constraintsTo ensure data integrity, consider adding database constraints for:
- Foreign key relationship between subcontrols and controls
- Required fields (status, owner)
- Valid status values using ENUM or CHECK constraints
Would you like me to provide the SQL statements for adding these constraints?
Clients/src/presentation/pages/ComplianceTracker/controls.req.structure.ts (1)
41-43: Replace hardcoded IDs with placeholder comments.Using specific IDs in example data could be misleading. Consider using more generic placeholders.
- projectId: 1234, // id of the project that the control category is related to - controlCategoryTitle: "AI literacy", - controlCategoryId: 1, // After control Category is created id will be saved in a const + projectId: 0, // Example project ID + controlCategoryTitle: "Example Category", + controlCategoryId: 0, // Example category IDClients/src/presentation/components/Table/AccordionTable/index.tsx (3)
Line range hint
97-108: Remove console.log and update OnSave implementation.The OnSave callback contains a console.log statement and a commented-out function call.
- OnSave={() => { - console.log("Save clicked"); - // fetchComplianceTracker(); - }} + OnSave={async () => { + try { + await fetchComplianceTracker(); + } catch (error) { + // Handle error appropriately + console.error('Failed to save:', error); + } + }}
Line range hint
166-166: Update useMemo dependencies for tableBody.The useMemo dependencies array is missing the newly added controlCategory prop.
- [rows, modalOpen, selectedRow, getProgressColor, theme.palette.grey, id] + [rows, modalOpen, selectedRow, getProgressColor, theme.palette.grey, id, controlCategory]
Line range hint
13-32: Consider splitting component responsibilities.The AccordionTable component handles multiple responsibilities including table rendering, modal management, and progress calculation. Consider splitting these into separate components for better maintainability.
Would you like me to provide an example of how to split this component into smaller, more focused components?
Servers/controllers/control.ctrl.ts (2)
Line range hint
124-138: Consider wrapping DB operations in a transaction.Multiple database operations are performed in sequence. If any operation fails, the database could be left in an inconsistent state.
Consider wrapping the operations in a transaction:
- mainLoop: for (const controlGroup of controlGroups) { + const transaction = await prisma.$transaction(async (tx) => { + mainLoop: for (const controlGroup of controlGroups) { // ... existing code ... + } + });
Line range hint
124-143: Consider optimizing nested loops performance.The current implementation uses triple-nested loops which could be inefficient for large datasets.
Consider using batch operations or reducing nesting:
+ const flattenedControls = controlGroups.flatMap(group => + group.control.flatMap(ctrl => + ctrl.controls.map(control => ({ + control, + subControls: control.subControls + })) + ) + ); + + for (const { control, subControls } of flattenedControls) { + // Process control and subControls + }Servers/controllers/assessment.ctrl.ts (2)
Line range hint
229-270: Add error handling in non-mock path.The non-mock path lacks try-catch blocks and proper error handling.
Add error handling:
} else { + try { // first get all assessments const assessments = await getAllAssessmentsQuery(); // ... existing code ... + return res.status(200).json(STATUS_CODE[200]({ message: "Answers saved" })); + } catch (error) { + return res.status(500).json(STATUS_CODE[500]((error as Error).message)); + } }
Line range hint
229-270: Add input validation.The function lacks validation for required fields and data structure.
Add validation before processing:
if (!req.body.topic || !req.body.subtopic || !Array.isArray(req.body.subtopic)) { return res.status(400).json(STATUS_CODE[400]({ message: "Invalid request body structure" })); }Clients/src/presentation/components/Modals/Controlpane/NewControlPane.tsx (1)
51-51: Add prop validation and documentationThe new props lack proper documentation and validation.
Add prop types validation and JSDoc comments:
+/** + * @param {Object} props + * @param {string} props.numbering - The control numbering in format X.Y + * @param {string} props.controlCategory - The category of the control + */ const NewControlPane = ({ id, numbering, isOpen, handleClose, title, content, subControls, controlCategory, OnSave, }: { id: string; - numbering: string; + numbering: string; // Format: X.Y where X and Y are numbers isOpen: boolean; handleClose: () => void; title: string; content: string; subControls: any[]; - controlCategory: string; + controlCategory: string; // Must be one of the valid control categories OnSave?: (state: State) => void; }) => { + // Validate numbering format + if (!/^\d+\.\d+$/.test(numbering)) { + console.warn('Invalid numbering format. Expected X.Y where X and Y are numbers'); + }Also applies to: 57-57, 61-61, 67-67
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
Clients/src/presentation/components/Inputs/Dropdowns/index.tsx(3 hunks)Clients/src/presentation/components/Inputs/Field/index.tsx(4 hunks)Clients/src/presentation/components/Modals/ComplianceFeedback/ComplianceFeedback.tsx(1 hunks)Clients/src/presentation/components/Modals/Controlpane/NewControlPane.tsx(5 hunks)Clients/src/presentation/components/Table/AccordionTable/index.tsx(3 hunks)Clients/src/presentation/pages/ComplianceTracker/NewComplianceTracker.tsx(1 hunks)Clients/src/presentation/pages/ComplianceTracker/controls.req.structure.ts(1 hunks)Servers/controllers/assessment.ctrl.ts(1 hunks)Servers/controllers/control.ctrl.ts(1 hunks)Servers/controllers/controlCategory.ctrl.ts(1 hunks)Servers/controllers/project.ctrl.ts(2 hunks)Servers/controllers/subcontrol.ctrl.ts(1 hunks)Servers/mocks/controlCategory.mock.data.ts(1 hunks)Servers/mocks/tools/control.mock.db.ts(1 hunks)Servers/mocks/tools/controlCategory.mock.db.ts(1 hunks)Servers/models/controlCategory.model.ts(1 hunks)Servers/routes/controlCategory.route.ts(1 hunks)Servers/routes/project.route.ts(2 hunks)Servers/utils/controlCategory.util.ts(1 hunks)Servers/utils/subControl.utils.ts(1 hunks)
🔇 Additional comments (11)
Servers/controllers/project.ctrl.ts (1)
263-263:
Fix incorrect property assignment
In the real database operation, the projectId is incorrectly set to controlCategoryId. This should be controlCategoryId instead.
Apply this diff to correct the assignment:
- projectId: controlCategoryId, // now must be replaced with controlCategoryId
+ controlCategoryId: controlCategoryId,Likely invalid or redundant comment.
Servers/models/controlCategory.model.ts (1)
1-5: LGTM!
The definition of the ControlCategory type is clear and follows TypeScript conventions.
Servers/mocks/tools/control.mock.db.ts (1)
13-24:
Multiple issues in control creation logic
- Confusing property mapping:
projectIdis being used forcontrolCategoryId - Deprecated
controlGroupfield is still included - Deep nesting in
newControl.controlstructure is error-prone - Lacks type safety
+interface CreateControlInput {
+ status: string;
+ approver: string;
+ riskReview: string;
+ owner: string;
+ reviewer: string;
+ description: string;
+ date: Date;
+ controlCategoryId: number;
+}
-export const createMockControl = (newControl: any): object => {
+export const createMockControl = (input: CreateControlInput): Control => {
const newControlToSave = {
id: mockControls.length + 1,
- status: newControl.control.status,
- approver: newControl.control.approver,
- riskReview: newControl.control.riskReview,
- owner: newControl.control.owner,
- reviewer: newControl.control.reviewer,
- implementationDetails: newControl.control.description,
- dueDate: newControl.control.date,
- projectId: newControl.controlCategoryId, // it will be controlCategoryId
- controlGroup: newControl.controlGroup, // This field must be removed in anywhere
+ status: input.status,
+ approver: input.approver,
+ riskReview: input.riskReview,
+ owner: input.owner,
+ reviewer: input.reviewer,
+ implementationDetails: input.description,
+ dueDate: input.date,
+ controlCategoryId: input.controlCategoryId,
};Let's verify the removal of controlGroup field across the codebase:
Servers/utils/subControl.utils.ts (1)
20-34: 🛠️ Refactor suggestion
Replace console.log with proper logging
Production code should use a proper logging system instead of console.log for better observability and log management.
+import { Logger } from '../utils/logger'; // Implement or import a logger
+
export const createNewSubcontrolQuery = async (
controlId: number,
subcontrol: {
status: string;
approver: string;
riskReview: string;
owner: string;
reviewer: string;
dueDate: Date;
implementationDetails: string;
evidence: string;
attachment: string;
feedback: string;
}
): Promise<Subcontrol> => {
- console.log("createNewSubcontrol", subcontrol);
+ Logger.info('Creating new subcontrol', { controlId, ...subcontrol });Likely invalid or redundant comment.
Clients/src/presentation/components/Modals/ComplianceFeedback/ComplianceFeedback.tsx (1)
50-57: 🛠️ Refactor suggestion
Review content transformation logic
The current implementation has potential issues:
- Prepending a space (
" " + content) seems unnecessary and could cause formatting issues - Removing paragraph tags might break rich text formatting, especially if the content is meant to be rendered as HTML later
Consider this alternative implementation:
- target: {
- value:
- " " +
- content
- .replace(/^<p>/, "")
- .replace(/<\/p>$/, "")
- .trim(),
+ target: {
+ value: content.trim()
+ },Let's verify how this content is used downstream:
✅ Verification successful
Keep the current content transformation logic
After analyzing the code, the current implementation should be maintained because:
- The component uses a
RichTextEditorcomponent that provides HTML content - The space prefix and p-tag removal are intentional as the content is used in a form context where the transformed value is stored in the parent component's state (
NewControlPane.tsx) for both evidence and feedback fields - The transformation is consistent with how the data is handled in the parent component, which expects plain text input values
- The content is properly managed in the parent component's state through the
onChangehandlers for both evidence and feedback sections - The transformation maintains consistency between the rich text editor's HTML output and the form's expected plain text input
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of this transformed content
rg -l "ComplianceFeedback.*onChange"
Length of output: 38
Script:
#!/bin/bash
# Let's try a broader search to understand the component and its usage
rg -A 5 "ComplianceFeedback"
# Also search for the handleContentChange function
ast-grep --pattern 'handleContentChange = ($$$) => { $$$ }'
# Let's also check if there are any HTML renderers or rich text editors in use
rg -l "dangerouslySetInnerHTML|contentEditable"
Length of output: 815
Script:
#!/bin/bash
# Let's look at the ComplianceFeedback component implementation
cat "Clients/src/presentation/components/Modals/ComplianceFeedback/ComplianceFeedback.tsx"
# Also check NewControlPane for how ComplianceFeedback is used
cat "Clients/src/presentation/components/Modals/Controlpane/NewControlPane.tsx"
Length of output: 13463
Clients/src/presentation/pages/ComplianceTracker/NewComplianceTracker.tsx (1)
86-86: Verify AccordionTable's handling of controlCategory prop
The addition of controlCategory prop improves context passing, but we should verify its implementation in AccordionTable.
Let's check the AccordionTable implementation:
✅ Verification successful
controlCategory prop is properly implemented in AccordionTable
The AccordionTable component correctly:
- Accepts controlCategory as a required string prop in its interface
- Passes it down to the NewControlPane component when a row is clicked
- Uses proper TypeScript typing and prop drilling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how controlCategory is used in AccordionTable
ast-grep --pattern 'interface $interface {
$$$
controlCategory?: string;
$$$
}'
# Check for any potential prop drilling
rg -A 5 "controlCategory.*={.*}"
Length of output: 246
Script:
#!/bin/bash
# Search for AccordionTable component definition and props interface
ast-grep --pattern 'interface $interface {
$$$
}'
# Search for controlCategory usage
rg "controlCategory" -A 3
# Find AccordionTable component definition
ast-grep --pattern 'const AccordionTable = ($props) => {
$$$
}'
Length of output: 15477
Script:
#!/bin/bash
# Let's find the AccordionTable component and its props interface more specifically
ast-grep --pattern 'interface $interface {
$$$
controlCategory: string;
$$$
}'
# Find the component implementation
ast-grep --pattern 'const AccordionTable: React.FC<$_> = ({
$$$
controlCategory,
$$$
}) => {
$$$
}'
# Check for any usage of controlCategory in AccordionTable component
rg -l "controlCategory" "Clients/src/presentation/components/Table/AccordionTable/"
Length of output: 345
Script:
#!/bin/bash
# Let's examine the full content of AccordionTable to see how controlCategory is used
cat Clients/src/presentation/components/Table/AccordionTable/index.tsx
Length of output: 5490
Servers/controllers/control.ctrl.ts (1)
125-126: LGTM! Parameter passing improvement.
The separation of controlId and subControl parameters improves code clarity and maintainability.
Clients/src/presentation/components/Inputs/Field/index.tsx (3)
101-103: LGTM! Improved error state styling.
The border styling for error states now properly uses theme values and maintains consistency.
222-222: LGTM! Typography size adjustment.
The font size adjustment for error messages improves readability.
165-165: Verify overflow behavior for multiline inputs.
The addition of overflowY: "auto" might affect the appearance of multiline inputs.
✅ Verification successful
The overflow behavior for multiline inputs is correctly implemented
Looking at the Field component implementation, I can see that:
- The component handles multiline inputs specifically when
type="description"(line 151) - For description type, it sets
multiline={true}androws={4}(lines 151-152) - The
overflowY: "auto"style is applied to the input element, which is the correct behavior for multiline inputs as it allows scrolling when content exceeds the 4-row height
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of Field component with description type
ast-grep --pattern 'Field type="description"'
Length of output: 47
Script:
#!/bin/bash
# Search for Field component usage with multiline or textarea attributes
rg -U "Field.*?(multiline|textarea)" -A 3
# Also search for Field component with type prop
rg -U "Field.*?type=" -A 3
# Look for the Field component implementation to understand its behavior
ast-grep --pattern 'const Field = $_'
Length of output: 16174
Clients/src/presentation/components/Modals/Controlpane/NewControlPane.tsx (1)
213-213: Verify numbering format consistency
The numbering format is used in multiple places. Ensure it follows a consistent pattern across the application.
Also applies to: 279-279
✅ Verification successful
Numbering format is consistent across the codebase
The numbering format follows a consistent hierarchical pattern using dot notation:
- Base level:
{numbering} - Nested level:
${numbering}.${id}
This is consistently implemented in both the AccordionTable and NewControlPane components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other uses of numbering prop in the codebase
rg -l "numbering.*string" src/
rg "numbering\}" src/
Length of output: 146
Script:
#!/bin/bash
# Search for other uses of numbering prop in the codebase
rg -l "numbering.*string" Clients/src/
rg "numbering\}" Clients/src/
# Also search for similar patterns to understand the format
rg -A 2 "numbering" Clients/src/
Length of output: 2033
| const controlCategories = await getAllControlCategoriesQuery(); | ||
| return res.status(200).json(controlCategories); | ||
| } catch (error) { | ||
| return res.status(500).json(STATUS_CODE[500]((error as Error).message)); |
There was a problem hiding this comment.
Avoid exposing internal error messages to clients
Returning detailed error messages to clients can reveal sensitive information. Instead, log the error on the server and return a generic message.
Apply this diff to improve error handling:
- return res.status(500).json(STATUS_CODE[500]((error as Error).message));
+ console.error(error);
+ return res.status(500).json(STATUS_CODE[500]("Internal Server Error"));Also applies to: 54-54, 74-74, 99-99, 119-119
| const controlCategoryId = parseInt(req.params.id); | ||
| if (MOCKDATA_ON === true) { |
There was a problem hiding this comment.
Validate and sanitize input parameters
Ensure that controlCategoryId parsed from req.params.id is a valid number before proceeding.
Add validation to handle invalid IDs:
const controlCategoryId = parseInt(req.params.id);
+ if (isNaN(controlCategoryId)) {
+ return res.status(400).json(STATUS_CODE[400]("Invalid control category ID"));
+ }Also applies to: 83-84, 107-108
| const controlCategory = getMockControlCategoryById(controlCategoryId); | ||
| return res.status(200).json(controlCategory); | ||
| } |
There was a problem hiding this comment.
Handle cases where resources are not found
When fetching, updating, or deleting a control category, return a 404 status if the resource does not exist.
Add checks to handle not-found scenarios:
// After fetching the control category
if (!controlCategory) {
return res.status(404).json(STATUS_CODE[404]("Control Category not found"));
}Also applies to: 50-52, 86-90, 92-96, 110-112, 114-116
| res: Response | ||
| ): Promise<any> { | ||
| try { | ||
| const newControlCategory: ControlCategory = req.body; |
There was a problem hiding this comment.
Validate request body data
Ensure required fields are present in req.body before creating or updating a control category.
Add validation for required fields:
const newControlCategory: ControlCategory = req.body;
+ if (!newControlCategory.projectId || !newControlCategory.name) {
+ return res.status(400).json(STATUS_CODE[400]("Missing required fields: projectId and name"));
+ }Also applies to: 84-84
| const control: any = await createNewControlQuery({ | ||
| // controlCategoryId: controlCategory.id, | ||
| projectId: controlCategoryId, // now must be replaced with controlCategoryId | ||
| // title: req.body.control.title, | ||
| status: req.body.control.status, | ||
| approver: req.body.control.approver, | ||
| riskReview: req.body.control.riskReview, | ||
| owner: req.body.control.owner, | ||
| reviewer: req.body.control.reviewer, | ||
| dueDate: req.body.control.date, | ||
| implementationDetails: req.body.control.description, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistency in property names
There is inconsistency in property names between mock data and real database operations, such as contrlTitle vs. controlTitle. Also, some properties are commented out or may not match the expected database schema.
Review and align the property names to match the database schema and ensure all necessary fields are included.
| const controlIdFK = req.body.controlId; | ||
| const newSubcontrol = await createNewSubcontrolQuery( | ||
| controlIdFK, | ||
| subcontrol | ||
| ); |
There was a problem hiding this comment.
Add type checking and validation for controlIdFK.
The extraction of controlIdFK lacks type checking and validation, which could lead to runtime errors.
- const controlIdFK = req.body.controlId;
+ const controlIdFK = Number(req.body.controlId);
+ if (!controlIdFK || isNaN(controlIdFK)) {
+ return res.status(400).json(STATUS_CODE[400]('Invalid control ID'));
+ }
const newSubcontrol = await createNewSubcontrolQuery(
controlIdFK,
subcontrol
);📝 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 controlIdFK = req.body.controlId; | |
| const newSubcontrol = await createNewSubcontrolQuery( | |
| controlIdFK, | |
| subcontrol | |
| ); | |
| const controlIdFK = Number(req.body.controlId); | |
| if (!controlIdFK || isNaN(controlIdFK)) { | |
| return res.status(400).json(STATUS_CODE[400]('Invalid control ID')); | |
| } | |
| const newSubcontrol = await createNewSubcontrolQuery( | |
| controlIdFK, | |
| subcontrol | |
| ); |
| @@ -219,6 +219,7 @@ export async function saveAnswers(req: Request, res: Response): Promise<any> { | |||
| createMockQuestion(subtopicId, question); | |||
| } | |||
| } | |||
| res.status(200).json(STATUS_CODE[200]({ message: "Answers saved" })); | |||
There was a problem hiding this comment.
Fix potential "headers already sent" error.
The code attempts to send multiple responses in the mock data path.
Remove the redundant status setting:
res.status(200).json(STATUS_CODE[200]({ message: "Answers saved" }));
} catch (error) {
return res.status(500).json(STATUS_CODE[500]((error as Error).message));
}
- res.status(200);Also applies to: 224-226
| @@ -14,6 +14,7 @@ import DropDowns from "../../Inputs/Dropdowns"; | |||
| import { useState } from "react"; | |||
| import AuditorFeedback from "../ComplianceFeedback/ComplianceFeedback"; | |||
| import { Dayjs } from "dayjs"; | |||
| import { apiServices } from "../../../../infrastructure/api/networkServices"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper error handling and loading state management
The new API integration requires better error handling and user feedback mechanisms.
Consider implementing:
- Loading state during API calls
- Error message display to users
- Success notifications
import { useState } from "react";
+import { useState } from "react";
+const [isLoading, setIsLoading] = useState(false);
+const [error, setError] = useState<string | null>(null);Committable suggestion skipped: line range outside the PR's diff.
| const handleSave = async () => { | ||
| const controlToSave = { | ||
| controlCategoryTitle: controlCategory, | ||
| control: state, | ||
| }; | ||
| console.log(controlToSave); | ||
|
|
||
| try { | ||
| const response = await apiServices.post( | ||
| "/projects/saveControls", | ||
| controlToSave | ||
| ); | ||
| console.log("Controls saved successfully:", response); | ||
| } catch (error) { | ||
| console.error("Error saving controls:", error); | ||
| } |
There was a problem hiding this comment.
Enhance error handling and add loading state in handleSave
The current implementation needs improvements in error handling and user feedback.
Apply these improvements:
const handleSave = async () => {
+ setIsLoading(true);
+ setError(null);
const controlToSave = {
controlCategoryTitle: controlCategory,
control: state,
};
- console.log(controlToSave);
try {
const response = await apiServices.post(
"/projects/saveControls",
controlToSave
);
- console.log("Controls saved successfully:", response);
+ // Add type safety for response
+ if (!response.ok) {
+ throw new Error('Failed to save controls');
+ }
+ // Show success notification
+ enqueueSnackbar('Controls saved successfully', { variant: 'success' });
} catch (error) {
- console.error("Error saving controls:", error);
+ setError(error instanceof Error ? error.message : 'An error occurred');
+ enqueueSnackbar('Failed to save controls', { variant: 'error' });
} finally {
+ setIsLoading(false);
}
if (OnSave) {
OnSave(state);
}
};Committable suggestion skipped: line range outside the PR's diff.
| const response = await apiServices.post( | ||
| "/projects/saveControls", | ||
| controlToSave | ||
| ); |
There was a problem hiding this comment.
💡 Codebase verification
Authentication middleware is disabled for sensitive endpoint
The API endpoint /projects/saveControls has its authentication middleware commented out, potentially exposing sensitive compliance data. Additionally, there's no visible input validation for the control data being saved.
Key issues found:
- Authentication is disabled:
router.post("/saveControls", /*authenticateJWT,*/ saveControls); - No input validation before saving control data
- Raw user input is directly passed to the API endpoint
🔗 Analysis chain
Verify API endpoint security and data sanitization
The component handles sensitive compliance data and makes API calls with user input.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for API endpoint security configuration
rg -A 5 "/projects/saveControls" src/
# Check for data sanitization
rg -l "sanitize|validate" src/infrastructure/api/
Length of output: 198
Script:
#!/bin/bash
# Search for API endpoint security configuration and data validation
# First, find the correct path structure
fd -t f saveControls
fd -t f apiServices
# Then search for API endpoint and validation patterns
rg -A 5 "saveControls"
rg -A 5 "apiServices.post"
# Look for validation or sanitization patterns
rg -A 5 "controlToSave"
Length of output: 7052
There was a problem hiding this comment.
Actionable comments posted: 24
🧹 Outside diff range and nitpick comments (10)
Servers/controllers/control.ctrl.ts (2)
Line range hint
124-138: Add transaction management for atomic operationsThe nested loops perform multiple database operations that should be atomic. If any operation fails, all changes should be rolled back.
} else { + const client = await pool.connect(); + try { + await client.query('BEGIN'); let flag = true; mainLoop: for (const controlGroup of controlGroups) { // ... existing code ... } if (flag) { + await client.query('COMMIT'); return res.status(201).json(STATUS_CODE[201]({})); } + await client.query('ROLLBACK'); return res.status(400).json(STATUS_CODE[400]({})); + } catch (error) { + await client.query('ROLLBACK'); + throw error; + } finally { + client.release(); + } }
Line range hint
124-138: Improve error handling with specific messagesThe current implementation uses a boolean flag that doesn't provide specific error information. Consider capturing and returning specific error messages.
- let flag = true; + const errors = []; mainLoop: for (const controlGroup of controlGroups) { for (const ctrl of controlGroup.control) { for (const control of ctrl.controls) { const controlId = control.id; for (const subControl of control.subControls) { const newSubControl = await createNewSubcontrolQuery( controlId, subControl ); if (!newSubControl) { - flag = false; - break mainLoop; + errors.push(`Failed to create subcontrol for control ${controlId}`); + continue; } } const newControl = await createNewControlQuery(control); if (!newControl) { - flag = false; - break mainLoop; + errors.push(`Failed to create control ${control.id}`); + continue; } } } } - if (flag) { + if (errors.length === 0) { return res.status(201).json(STATUS_CODE[201]({})); } - return res.status(400).json(STATUS_CODE[400]({})); + return res.status(400).json(STATUS_CODE[400]({ errors }));Servers/controllers/controlCategory.ctrl.ts (1)
28-28: Simplify boolean comparisonComparing a boolean to
trueis redundant. You can simplify the conditionif (MOCKDATA_ON === true)toif (MOCKDATA_ON).Apply this diff to fix the issue:
- if (MOCKDATA_ON === true) { + if (MOCKDATA_ON) {Servers/controllers/project.ctrl.ts (2)
241-241: Removeconsole.logstatements in production code
console.logstatements can expose sensitive data and clutter the console output. Consider removing them or using a proper logging mechanism with appropriate log levels.Apply this diff to fix the issue:
- console.log("subcontrolToSave : ", subcontrolToSave);
262-262: Remove commented-out codeCommented-out code can clutter the codebase and reduce readability. Consider removing it if it's no longer needed.
Apply this diff to fix the issue:
- // controlCategoryId: controlCategory.id,Servers/mocks/controlCategory.mock.data.ts (1)
3-110: Consider using an enum or constants for category namesThe category names are repeated across projects and could benefit from centralization to ensure consistency and ease of maintenance.
Example implementation:
export enum ControlCategoryNames { AI_LITERACY = "AI literacy", TRANSPARENCY = "Transparency and provision of information to deployers", // ... other categories } export const ControlCategories: ControlCategory[] = [ { id: 1, projectId: 1, name: ControlCategoryNames.AI_LITERACY }, { id: 2, projectId: 1, name: ControlCategoryNames.TRANSPARENCY }, // ... rest of the categories ];Clients/src/presentation/pages/ComplianceTracker/NewComplianceTracker.tsx (1)
Line range hint
31-42: Add loading and error states for better UXThe
fetchComplianceTrackerfunction should handle loading states and display appropriate UI feedback during API calls.Consider adding loading and error states:
const NewComplianceTracker = () => { + const [isLoading, setIsLoading] = useState(false); + const [error, setError] = useState<string | null>(null); const [expanded, setExpanded] = useState<number | false>(false); const { setDashboardValues } = useContext(VerifyWiseContext); const fetchComplianceTracker = async () => { + setIsLoading(true); + setError(null); try { const response = await getAllEntities({ routeUrl: "/controls" }); console.log("Response:", response); setDashboardValues((prevValues: any) => ({ ...prevValues, compliance: response.data, })); } catch (error) { console.error("Error fetching compliance tracker:", error); + setError("Failed to fetch compliance data"); } finally { + setIsLoading(false); } };Clients/src/presentation/pages/ComplianceTracker/controls.req.structure.ts (1)
49-55: Define proper enums for status fieldsThe status fields use string literals which could lead to inconsistencies.
export enum ControlStatus { InProgress = 'IN_PROGRESS', Done = 'DONE', Waiting = 'WAITING' } export enum RiskReviewStatus { Acceptable = 'ACCEPTABLE_RISK', Residual = 'RESIDUAL_RISK', Unacceptable = 'UNACCEPTABLE_RISK' }Servers/controllers/assessment.ctrl.ts (1)
Line range hint
222-226: Fix duplicate response sendingThere's a potential issue where multiple responses could be sent:
- Line 222 sends a 200 response with a message
- Line 226 sends another 200 response without a body
This could lead to the error: "Cannot set headers after they are sent to the client"
Apply this fix:
res.status(200).json(STATUS_CODE[200]({ message: "Answers saved" })); } catch (error) { return res.status(500).json(STATUS_CODE[500]((error as Error).message)); } - res.status(200);Clients/src/presentation/components/Modals/Controlpane/NewControlPane.tsx (1)
51-67: Consider adding default values for new required props.The new props
numberingandcontrolCategoryare required but lack default values. This could cause runtime issues if these props are accidentally omitted.Consider adding default values:
const NewControlPane = ({ id, numbering = '', isOpen, handleClose, title, content, subControls, controlCategory = 'default', OnSave, }: { id: string; numbering: string; isOpen: boolean; handleClose: () => void; title: string; content: string; subControls: any[]; controlCategory: string; OnSave?: (state: State) => void; })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
Clients/src/presentation/components/Inputs/Dropdowns/index.tsx(3 hunks)Clients/src/presentation/components/Inputs/Field/index.tsx(4 hunks)Clients/src/presentation/components/Modals/ComplianceFeedback/ComplianceFeedback.tsx(1 hunks)Clients/src/presentation/components/Modals/Controlpane/NewControlPane.tsx(5 hunks)Clients/src/presentation/components/Table/AccordionTable/index.tsx(3 hunks)Clients/src/presentation/pages/ComplianceTracker/NewComplianceTracker.tsx(1 hunks)Clients/src/presentation/pages/ComplianceTracker/controls.req.structure.ts(1 hunks)Servers/controllers/assessment.ctrl.ts(1 hunks)Servers/controllers/control.ctrl.ts(1 hunks)Servers/controllers/controlCategory.ctrl.ts(1 hunks)Servers/controllers/project.ctrl.ts(2 hunks)Servers/controllers/subcontrol.ctrl.ts(1 hunks)Servers/mocks/controlCategory.mock.data.ts(1 hunks)Servers/mocks/tools/control.mock.db.ts(1 hunks)Servers/mocks/tools/controlCategory.mock.db.ts(1 hunks)Servers/models/controlCategory.model.ts(1 hunks)Servers/routes/controlCategory.route.ts(1 hunks)Servers/routes/project.route.ts(2 hunks)Servers/utils/controlCategory.util.ts(1 hunks)Servers/utils/subControl.utils.ts(1 hunks)
🔇 Additional comments (17)
Servers/utils/subControl.utils.ts (2)
20-34: LGTM! Good separation of controlId parameter
The refactoring to separate controlId from the subcontrol object improves the function's interface by making the relationship between controls and subcontrols more explicit. The TypeScript types are well-defined and maintain type safety.
Line range hint 42-52: Verify database schema constraints
The SQL parameters are correctly ordered to match the columns. However, we should ensure that the database has appropriate foreign key constraints for the controlId relationship.
Servers/controllers/control.ctrl.ts (1)
125-126: LGTM! Correct usage of separated controlId parameter
The changes correctly adapt to the new createNewSubcontrolQuery signature by passing controlId as a separate parameter.
Servers/controllers/controlCategory.ctrl.ts (6)
74-74: Avoid exposing internal error messages to the client
83-83: Specify radix when using parseInt
84-84: Validate input data to prevent unexpected errors
98-98: Avoid exposing internal error messages to the client
107-107: Specify radix when using parseInt
118-118: Avoid exposing internal error messages to the client
Servers/controllers/project.ctrl.ts (1)
283-283: Remove console.log statements in production code
Servers/models/controlCategory.model.ts (1)
1-5: Type definition for ControlCategory is well-defined
The type definition correctly specifies the required and optional properties for ControlCategory.
Servers/routes/project.route.ts (1)
21-21: New route /saveControls added successfully
The route /saveControls has been correctly added to handle saving controls.
Clients/src/presentation/components/Modals/ComplianceFeedback/ComplianceFeedback.tsx (1)
Line range hint 8-8: Verify file size constant usage
Ensure that MAX_FILE_SIZE from constants matches any server-side upload limits.
Clients/src/presentation/pages/ComplianceTracker/NewComplianceTracker.tsx (1)
86-86: LGTM: controlCategory prop addition
The addition of controlCategory prop to AccordionTable component aligns with the PR objective of iterating compliance store in the database.
Clients/src/presentation/components/Table/AccordionTable/index.tsx (2)
26-26: LGTM: Type-safe prop addition
The controlCategory prop is properly typed as a string and added to both the destructured props and the interface.
Also applies to: 31-31
97-98: Verify the impact of id prop splitting
The separation of id and numbering props might affect how controls are identified in the system. While the change looks logical, we should ensure this doesn't break any existing functionality.
Also applies to: 108-108
✅ Verification successful
The separation of id and numbering props is correctly implemented
Looking at the NewControlPane component implementation, both id and numbering props are properly defined in the interface and used consistently throughout the component:
- The
idprop is used for unique identification in state management and DOM elements - The
numberingprop is used for display purposes in the UI - This separation of concerns is maintained consistently in the modal, tabs, and dropdown components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other uses of NewControlPane to verify consistent prop usage
ast-grep --pattern 'NewControlPane $$$'
# Search for any direct references to the id prop
rg "id={.*?}" --type tsx
Length of output: 163
Script:
#!/bin/bash
# Search for other uses of NewControlPane to verify consistent prop usage
ast-grep --pattern 'NewControlPane'
# Search for any direct references to the id prop with proper regex escaping
rg "id=\{[^}]+\}" --type tsx -A 2
# Search for numbering prop usage
rg "numbering=\{[^}]+\}" --type tsx -A 2
Length of output: 622
Script:
#!/bin/bash
# Let's check the NewControlPane component definition to understand the props
cat Clients/src/presentation/components/Modals/Controlpane/NewControlPane.tsx
# Search for id and numbering props with correct file type
rg "id=\{[^}]+\}" -t typescript -t typescriptreact -A 2
rg "numbering=\{[^}]+\}" -t typescript -t typescriptreact -A 2
Length of output: 10153
Clients/src/presentation/components/Inputs/Field/index.tsx (1)
101-103: LGTM: Improved error border handling
The error border styling is now more explicit and consistent with the theme system.
| const controlIdFK = req.body.controlId; | ||
| const newSubcontrol = await createNewSubcontrolQuery( | ||
| controlIdFK, | ||
| subcontrol | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant controlId from subcontrol object
Since controlId is now passed separately as controlIdFK, it should be removed from the subcontrol object to avoid potential inconsistencies.
- const subcontrol: {
- controlId: number; // Remove this
+ const subcontrol: {
status: string;
approver: string;
// ... other fields
} = req.body;Committable suggestion skipped: line range outside the PR's diff.
Add validation for controlIdFK
The controlIdFK is extracted from req.body but lacks validation. Consider adding these checks:
- Ensure
controlIdFKis a valid number - Verify that
controlIdFKexists in the controls table before creating the subcontrol
- const controlIdFK = req.body.controlId;
+ const controlIdFK = parseInt(req.body.controlId);
+ if (isNaN(controlIdFK)) {
+ return res.status(400).json(STATUS_CODE[400]('Invalid controlId'));
+ }
+ // Verify control exists
+ const control = await getControlByIdQuery(controlIdFK);
+ if (!control) {
+ return res.status(404).json(STATUS_CODE[404]('Control not found'));
+ }
const newSubcontrol = await createNewSubcontrolQuery(
controlIdFK,
subcontrol
);Committable suggestion skipped: line range outside the PR's diff.
| res: Response | ||
| ): Promise<any> { | ||
| try { | ||
| const newControlCategory: ControlCategory = req.body; |
There was a problem hiding this comment.
Validate input data to prevent unexpected errors
Directly assigning req.body to newControlCategory without validation may lead to issues if required fields are missing or invalid. Implement input validation to ensure all necessary fields are present and correctly formatted.
| res: Response | ||
| ): Promise<any> { | ||
| try { | ||
| const controlCategoryId = parseInt(req.params.id); |
There was a problem hiding this comment.
Specify radix when using parseInt
Using parseInt without specifying the radix can lead to unexpected results. Always specify the radix parameter to ensure consistent behavior.
Apply this diff to fix the issue:
- const controlCategoryId = parseInt(req.params.id);
+ const controlCategoryId = parseInt(req.params.id, 10);📝 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 controlCategoryId = parseInt(req.params.id, 10); |
| ); | ||
| return res.status(200).json(controlCategory); | ||
| } catch (error) { | ||
| return res.status(500).json(STATUS_CODE[500]((error as Error).message)); |
There was a problem hiding this comment.
Avoid exposing internal error messages to the client
Returning detailed error messages to the client can expose sensitive information. Consider logging the error internally and returning a generic message.
Apply this diff to fix the issue:
- return res.status(500).json(STATUS_CODE[500]((error as Error).message));
+ console.error(error);
+ return res.status(500).json(STATUS_CODE[500]("Internal Server 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.
| return res.status(500).json(STATUS_CODE[500]((error as Error).message)); | |
| console.error(error); | |
| return res.status(500).json(STATUS_CODE[500]("Internal Server Error")); |
| const control: any = createMockControl({ | ||
| controlCategoryId: controlCategory.id, | ||
| control: { | ||
| contrlTitle: req.body.control.controlTitle, |
There was a problem hiding this comment.
Fix typo in property name contrlTitle
There's a typo in the property name contrlTitle. It should be controlTitle.
Apply this diff to fix the issue:
- contrlTitle: req.body.control.controlTitle,
+ controlTitle: req.body.control.controlTitle,📝 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.
| contrlTitle: req.body.control.controlTitle, | |
| controlTitle: req.body.control.controlTitle, |
| export const controlsReqStructure = { | ||
| projectId: 1234, // id of the project that the control category is related to | ||
| controlCategoryTitle: "AI literacy", | ||
| controlCategoryId: 1, // After control Category is created id will be saved in a const | ||
| control: { | ||
| controlCategoryId: 1, // The control category id that the control is related to | ||
| controlId: 1, // After control is created id will be saved in a const | ||
| controlTitle: "AI Literacy and Responsible AI Training", | ||
| controlDescription: "Develop the AI literacy ...", | ||
| status: "selected option", | ||
| approver: "selected option", | ||
| riskReview: "selected option", | ||
| owner: "selected option", | ||
| reviewer: "selected option", | ||
| date: "selected date", | ||
| description: "provided description", | ||
| subcontrols: [ | ||
| { | ||
| controlId: 1, // The control id that the subcontrol is related to | ||
| subControlTitle: | ||
| "We ensure executive leadership takes responsibility for decisions related to AI risks", | ||
| subControlDescription: | ||
| "Leadership is accountable for oversight and strategic decisions regarding AI risks, ensuring alignment with compliance.", | ||
| status: "selected option", | ||
| approver: "selected option", | ||
| riskReview: "selected option", | ||
| owner: "selected option", | ||
| reviewer: "selected option", | ||
| date: "selected date", | ||
| description: "provided description", | ||
| evidence: "provided evidence", | ||
| evidenceFiles: [], | ||
| feedback: "provided feedback", | ||
| feedbackFiles: [], | ||
| }, | ||
| ], | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety with TypeScript interfaces
The request structure uses primitive types and contains example values. Let's improve type safety and separate concerns.
- Create proper TypeScript interfaces:
interface SubControl {
controlId: number;
subControlTitle: string;
subControlDescription: string;
status: ControlStatus;
approver: string;
riskReview: RiskReviewStatus;
owner: string;
reviewer: string;
date: string;
description: string;
evidence: string;
evidenceFiles: Array<unknown>; // Define proper type
feedback: string;
feedbackFiles: Array<unknown>; // Define proper type
}
interface Control {
controlCategoryId: number;
controlId: number;
controlTitle: string;
controlDescription: string;
status: ControlStatus;
approver: string;
riskReview: RiskReviewStatus;
owner: string;
reviewer: string;
date: string;
description: string;
subcontrols: SubControl[];
}
interface ControlsRequest {
projectId: number;
controlCategoryTitle: string;
controlCategoryId: number;
control: Control;
}
export const controlsReqStructure: ControlsRequest;- Move example values to a separate file:
// controls.examples.ts
export const exampleControlsRequest: ControlsRequest = {
projectId: 1234,
controlCategoryTitle: "AI literacy",
// ... rest of the example
};| @@ -143,6 +143,7 @@ const Field = forwardRef( | |||
| </Typography> | |||
| )} | |||
| <TextField | |||
| className="field-input" | |||
There was a problem hiding this comment.
💡 Codebase verification
Add accessibility enhancements for scrollable content
The review comment is valid. The component uses overflowY: "auto" in the input props but lacks proper accessibility considerations:
- No max-height constraint is defined to prevent excessive scrolling
- Missing ARIA attributes for scrollable content
- No visible scrolling indicators for keyboard users
Specific locations that need attention:
- Line 146: Add
aria-multiline="true"whentype === "description" - Line 165: Add
maxHeightconstraint in theinputProps.sxobject - Consider adding
role="textbox"witharia-multiline="true"for better screen reader support
🔗 Analysis chain
Consider accessibility implications of overflow behavior
While adding overflowY: "auto" improves the visual experience, ensure that the content remains accessible when scrolling.
Consider adding:
- A max-height constraint to prevent excessive scrolling
- Aria attributes for better screen reader support
Also applies to: 165-165
The previous search for TextField didn't yield results, so let's look for the specific class name and related accessibility attributes to better understand the current implementation and verify the overflow concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other TextField components to verify consistent overflow handling
ast-grep --pattern 'TextField $$$'
Length of output: 36
Script:
#!/bin/bash
# Search for the Field component implementation and its usage of overflow properties
rg -U "className=\"field-input\"" -A 5 -B 5
# Search for any aria attributes related to overflow or scrolling
rg "aria-" --type tsx
# Look for style definitions related to field-input
rg "field-input" --type css --type scss
Length of output: 1142
Script:
#!/bin/bash
# Search for aria attributes without file type restriction
rg "aria-"
# Search for field-input class styles without file type restriction
rg "\.field-input"
# Look for the complete Field component implementation to understand the context
rg -U "export.*Field.*" -A 20 Clients/src/presentation/components/Inputs/Field/index.tsx
Length of output: 2304
Script:
#!/bin/bash
# Get the full content of the Field component file to understand the complete implementation
cat Clients/src/presentation/components/Inputs/Field/index.tsx
# Look for any CSS/SCSS files in the component directory
fd -e css -e scss . Clients/src/presentation/components/Inputs/Field/
Length of output: 7544
Script:
#!/bin/bash
# Let's check the CSS file to see any existing overflow or accessibility-related styles
cat Clients/src/presentation/components/Inputs/Field/index.css
Length of output: 941
| @@ -191,7 +210,7 @@ const NewControlPane = ({ | |||
| }} | |||
| > | |||
| <Typography fontSize={16} fontWeight={600} sx={{ textAlign: "left" }}> | |||
| {id} {title} | |||
| {numbering} {title} | |||
There was a problem hiding this comment.
Add input validation and improve accessibility for numbering display.
The numbering display needs validation and security improvements:
- No validation of numbering format
- Potential XSS vulnerability
- Missing accessibility attributes
Consider these improvements:
- <Typography fontSize={16} fontWeight={600} sx={{ textAlign: "left" }}>
- {numbering} {title}
- </Typography>
+ <Typography
+ fontSize={16}
+ fontWeight={600}
+ sx={{ textAlign: "left" }}
+ aria-label={`Control ${numbering}: ${title}`}
+ >
+ {sanitizeInput(numbering)} {sanitizeInput(title)}
+ </Typography>
// Add utility function at the top of the file
+ const sanitizeInput = (input: string) => {
+ // Basic XSS prevention
+ return input.replace(/[<>]/g, '');
+ };
// Add prop validation
+ if (!/^\d+(\.\d+)*$/.test(numbering)) {
+ console.warn(`Invalid numbering format: ${numbering}. Expected format: "1.2.3"`);
+ }Apply similar changes to the subcontrol title display at line 279.
Also applies to: 279-279
| OnSave, | ||
| }: { | ||
| id: string; | ||
| numbering: string; | ||
| isOpen: boolean; | ||
| handleClose: () => void; | ||
| title: string; | ||
| content: string; | ||
| subControls: any[]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define proper TypeScript interface for subControls.
Using any[] for subControls loses TypeScript's type safety benefits and could lead to runtime errors.
Add a proper interface:
interface SubControl {
id: string;
title: string;
description: string;
subControlId?: string;
}
// Update props interface
interface NewControlPaneProps {
id: string;
numbering: string;
isOpen: boolean;
handleClose: () => void;
title: string;
content: string;
subControls: SubControl[];
controlCategory: string;
OnSave?: (state: State) => void;
}| const handleSave = async () => { | ||
| const controlToSave = { | ||
| controlCategoryTitle: controlCategory, | ||
| control: state, | ||
| }; | ||
| console.log(controlToSave); | ||
|
|
||
| try { | ||
| const response = await apiServices.post( | ||
| "/projects/saveControls", | ||
| controlToSave | ||
| ); | ||
| console.log("Controls saved successfully:", response); | ||
| } catch (error) { | ||
| console.error("Error saving controls:", error); | ||
| } |
There was a problem hiding this comment.
Enhance error handling and user feedback in save operation.
The current implementation has several areas for improvement:
- No loading state during API call
- Missing user feedback for success/error cases
- Modal stays open after successful save
- Potential race condition between API call and OnSave callback
Consider this improved implementation:
+ const [isSaving, setIsSaving] = useState(false);
const handleSave = async () => {
+ setIsSaving(true);
const controlToSave = {
controlCategoryTitle: controlCategory,
control: state,
};
try {
const response = await apiServices.post(
"/projects/saveControls",
controlToSave
);
- console.log("Controls saved successfully:", response);
+ if (OnSave) {
+ OnSave(state);
+ }
+ handleClose();
+ // Consider using a toast notification system
+ alert('Changes saved successfully');
} catch (error) {
- console.error("Error saving controls:", error);
+ // Consider using a toast notification system
+ alert('Failed to save changes: ' + error.message);
} finally {
+ setIsSaving(false);
}
- if (OnSave) {
- OnSave(state);
- }
};Also update the Save button to show loading state:
<Button
variant="contained"
onClick={handleSave}
+ disabled={isSaving}
sx={{
...buttonStyle,
width: 68,
"&:hover": {
backgroundColor: "#175CD3 ",
},
}}
disableRipple
>
- Save
+ {isSaving ? 'Saving...' : 'Save'}
</Button>Committable suggestion skipped: line range outside the PR's diff.
@HarshP4585
The iteration through controls is mostly done.
Please go through the code and fix parts that do not match on the server-side
Summary by CodeRabbit
Release Notes
New Features
controlCategoryproperty to various components for improved data handling.Bug Fixes
Documentation
Chores