E2608. Hierarchical list display#167
Open
veera2508 wants to merge 10 commits intoexpertiza:mainfrom
Open
Conversation
…ty by preventing refetch
feat(create-teams): load assignment teams and unassigned participants…
[Imp] Frontend changes to render Questionnaire Type and Heirarchy
Author
|
There are some files which should not be modified in the final PR. Once a review is done, we will revert those back. This is because those changes are required to run it on docker on the VCL. |
📝 WalkthroughWalkthroughThis pull request refactors data loading patterns and component data flow. Route loaders are updated to use more specific data fetchers ( Changes
Sequence DiagramsequenceDiagram
participant Router as Route Loader
participant API as API Layer
participant Component as Questionnaire Component
participant SubTable as Child Table
rect rgba(100, 150, 200, 0.5)
Note over Router,SubTable: Old Flow (Flat List)
Router->>API: Fetch /questionnaires
API-->>Router: QuestionnaireResponse[]
Component->>Component: Set as flat table data
Component->>SubTable: None (no expansion)
end
rect rgba(150, 200, 100, 0.5)
Note over Router,SubTable: New Flow (Hierarchical)
Router->>API: Fetch /questionnaires/hierarchical
API-->>Router: QuestionnaireTypeGroup[]<br/>(grouped by type)
Component->>Component: Transform to tableData<br/>(type + count)
Component->>Component: Set selectedQuestionnaire<br/>on row click
Component->>SubTable: renderSubComponent with<br/>group.questionnaires
SubTable-->>Component: Displays nested items
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/Questionnaires/QuestionnaireItemsFieldArray.tsx (1)
369-387:⚠️ Potential issue | 🟡 MinorMissing
type="button"on the fallback remove button.This button lacks
type="button"unlike all other remove buttons in this component. Without it, the button defaults totype="submit", which could cause unintended form submission.🐛 Proposed fix
<Button variant="link" + type="button" onClick={() => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Questionnaires/QuestionnaireItemsFieldArray.tsx` around lines 369 - 387, The fallback remove Button currently lacks an explicit type and may act as a submit button; update the Button (the one rendering the delete-icon and using values.items[index], setFieldValue(`items[${index}]._destroy`, true) and remove(index)) to include type="button" so it does not trigger form submission.
🧹 Nitpick comments (7)
src/pages/Courses/CourseAssignments.tsx (1)
49-74: Consider memoizing handler functions to avoid unnecessary re-renders.The handler functions (
onEditHandle,onDeleteHandle, etc.) are recreated on every render since they're not wrapped inuseCallback. This causes theactionHandlersuseMemo (which lists them as dependencies) to recompute on every render, negating its memoization benefit.♻️ Example fix for one handler
- const onEditHandle = (row: TRow<IAssignmentResponse>) => { - const from = `${location.pathname}${location.search}${location.hash || ""}`; - navigate(`/assignments/edit/${row.original.id}`, { state: { from } }); - }; + const onEditHandle = useCallback((row: TRow<IAssignmentResponse>) => { + const from = `${location.pathname}${location.search}${location.hash || ""}`; + navigate(`/assignments/edit/${row.original.id}`, { state: { from } }); + }, [location.pathname, location.search, location.hash, navigate]);Apply the same pattern to
onDeleteHandle,onAddParticipantHandle, etc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Courses/CourseAssignments.tsx` around lines 49 - 74, The handler functions (onEditHandle, onDeleteHandle, onAddParticipantHandle, onCreateTeamsHandle, onAssignReviewersHandle, onViewSubmissionsHandle, onViewScoresHandle, onViewReportsHandle) are recreated every render which forces actionHandlers useMemo to recompute; wrap each handler in React.useCallback and provide stable dependency arrays (e.g., include navigate and location for onEditHandle, include setShowDeleteConfirmation for onDeleteHandle, and include navigate for the others) so the handlers remain stable and actionHandlers can actually memoize.src/pages/Courses/Course.tsx (1)
47-62: Fetch condition may cause excessive API calls.The condition
!showDeleteConfirmation.visible || !showCopyConfirmation.visibleevaluates totruewhenever at least one modal is closed (i.e., almost always). Combined with the dependency array including both modal visibility states, this triggers fetches on every modal state change.If the intent is to refetch after closing a modal, consider restructuring to explicitly trigger on modal close rather than relying on visibility changes.
♻️ Possible approach: fetch only on mount or explicit refresh
useEffect(() => { - // Ensure the API fetch happens unless modals are active - if (!showDeleteConfirmation.visible || !showCopyConfirmation.visible) { - fetchCourses({ url: `/courses` }); - fetchInstitutions({ url: `/institutions` }); - fetchInstructors({ url: `/users` }); - } + fetchCourses({ url: `/courses` }); + fetchInstitutions({ url: `/institutions` }); + fetchInstructors({ url: `/users` }); }, [ fetchCourses, fetchInstitutions, fetchInstructors, location, - showDeleteConfirmation.visible, auth.user.id, - showCopyConfirmation.visible, ]);If refetch after delete/copy is needed, handle it via callbacks (as you've done for assignments) rather than visibility-triggered effects.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Courses/Course.tsx` around lines 47 - 62, The current useEffect fetches whenever any modal visibility flips because it uses || with modal visibilities and includes those visibilities in the dependency array; change the condition to require both modals to be closed (use && for showDeleteConfirmation.visible and showCopyConfirmation.visible) so fetches only run when neither modal is open, and remove showDeleteConfirmation.visible and showCopyConfirmation.visible from the effect dependency array to avoid modal toggles retriggering fetches; additionally, instead of relying on visibility changes to refetch, call fetchCourses/fetchInstitutions/fetchInstructors explicitly from the modal close/success handlers (e.g., the delete/copy success callbacks) to refresh data after an operation.src/pages/Questionnaires/QuestionnaireUtils.tsx (1)
166-172: Consider adding error handling toloadQuestionnaireHierarchy.The loader lacks error handling. If the API call fails, the error will propagate unhandled. Consider wrapping in try-catch or letting React Router's errorElement handle it.
♻️ Suggested improvement
export async function loadQuestionnaireHierarchy() { + try { const response = await axiosClient.get(`/questionnaires/hierarchical`); return response.data.map((group: QuestionnaireTypeGroup) => ({ ...group, questionnaires: group.questionnaires.map((q: any) => transformQuestionnaireResponse(q)), })); + } catch (error) { + console.error("Error loading questionnaire hierarchy:", error); + throw error; // Let React Router errorElement handle it + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Questionnaires/QuestionnaireUtils.tsx` around lines 166 - 172, The loader function loadQuestionnaireHierarchy lacks error handling for the axiosClient.get call; wrap the await axiosClient.get('/questionnaires/hierarchical') and subsequent mapping in a try-catch inside loadQuestionnaireHierarchy, catch network/response errors, log or process the error (using your logger or console), and rethrow a controlled error (or throw a Response for React Router errorElement) so upstream can handle it; ensure you still map group.questionnaires with transformQuestionnaireResponse on success and do not swallow the original error details.src/pages/Questionnaires/QuestionnaireItemsFieldArray.tsx (2)
127-145: Extract duplicated remove button into a reusable component.The remove button with its onClick handler is duplicated 7 times across different question type branches. Consider extracting it to a local
RemoveItemButtoncomponent to reduce duplication and simplify maintenance.♻️ Suggested extraction
const RemoveItemButton = ({ index, values, setFieldValue, remove }: { index: number; values: any; setFieldValue: (field: string, value: any) => void; remove: (index: number) => void; }) => ( <div className="ms-auto"> <OverlayTrigger overlay={<Tooltip>Remove Item</Tooltip>}> <Button variant="link" type="button" onClick={() => { const item = values.items[index]; if (item.id) { setFieldValue(`items[${index}]._destroy`, true); } else { remove(index); } }} aria-label="Remove Item" className="p-0" > <img src={"/assets/images/delete-icon-24.png"} alt="remove item" style={{ width: "20px", height: "20px" }} /> </Button> </OverlayTrigger> </div> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Questionnaires/QuestionnaireItemsFieldArray.tsx` around lines 127 - 145, Extract the duplicated remove-button JSX into a local React component named RemoveItemButton and replace the 7 inline occurrences with it; the component should accept props (index: number, values: any, setFieldValue: (field: string, value: any) => void, remove: (index: number) => void) and implement the same onClick logic used currently (read item from values.items[index], if item.id setFieldValue(`items[${index}]._destroy`, true) else call remove(index)), render the same Button and img markup and preserve aria-label/className; update callers to pass index, values, setFieldValue and remove so behavior is identical.
35-43: Consider extracting the duplicatenormalizedQuestionTypefunction.This normalization function is duplicated - a similar inverse mapping exists in
QuestionnaireForm.tsx(lines 81-89). Consider extracting this toQuestionnaireUtils.tsxalongside the existing type mappings to maintain a single source of truth for question type normalization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Questionnaires/QuestionnaireItemsFieldArray.tsx` around lines 35 - 43, Extract the duplicated normalization logic by moving the normalizedQuestionType function (used in QuestionnaireItemsFieldArray.tsx) and the inverse mapping in QuestionnaireForm.tsx into a shared utility module (e.g., QuestionnaireUtils.tsx) alongside the existing type mappings; create a single exported function name (normalizedQuestionType) and, if needed, an inverse helper (denormalizeQuestionType) in that module, update both QuestionnaireItemsFieldArray.tsx and QuestionnaireForm.tsx to import and use these utilities instead of local copies, and ensure you export the mappings so other components can reference the single source of truth.src/pages/Questionnaires/QuestionnaireForm.tsx (1)
72-89: Consider reusing normalization logic.The type normalization here (lines 84-88) performs the inverse of
normalizedQuestionType()inQuestionnaireItemsFieldArray.tsx. These could be consolidated into shared utilities inQuestionnaireUtils.tsx.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Questionnaires/QuestionnaireForm.tsx` around lines 72 - 89, The item type normalization in this component (see normalizedItemTypes and fallbackItemTypes) duplicates the inverse mapping already implemented by normalizedQuestionType() in QuestionnaireItemsFieldArray.tsx; extract a single normalization utility (e.g., normalizeQuestionType) into a shared module (QuestionnaireUtils.tsx) that handles the mappings ("TextArea" <-> "Text area", "TextField" <-> "Text field", "MultipleChoiceRadio" <-> "Multiple choice", etc.), replace the local mapping in QuestionnaireForm.tsx with a call to that utility, and update QuestionnaireItemsFieldArray.tsx to use the same utility so both places reference the single source of truth.src/pages/Questionnaires/QuestionnaireEditor.tsx (1)
11-32: Consider consolidating type mappings inQuestionnaireUtils.tsx.This
typeMapis the inverse ofquestionnaireTypeMapinQuestionnaireUtils.tsx. Consider exporting a single bidirectional mapping or helper functions fromQuestionnaireUtils.tsxto avoid maintaining two separate mappings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Questionnaires/QuestionnaireEditor.tsx` around lines 11 - 32, The displayQuestionnaireType function duplicates the inverse of questionnaireTypeMap in QuestionnaireUtils.tsx; move the canonical mapping and/or a helper that returns display names into QuestionnaireUtils.tsx (e.g., export a getDisplayName or a bidirectional mapping like questionnaireTypeMap and questionnaireDisplayMap) and update displayQuestionnaireType to import and reuse that helper/map (referencing displayQuestionnaireType and questionnaireTypeMap) so you maintain a single source of truth and handle unknown keys by falling back to the raw questionnaireType or empty string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 39: The package.json currently includes both "sass" and "sass-embedded";
remove the "sass" dependency and keep only "sass-embedded" so Vite uses the
supported embedded implementation. Edit package.json to delete the "sass" entry
from the dependencies (leaving "sass-embedded": "^1.98.0") and then run package
manager cleanup (npm/yarn/pnpm install or prune) to ensure only sass-embedded is
installed.
In `@src/pages/Assignments/AssignmentDelete.tsx`:
- Around line 46-49: The effect watching deletedAssignment?.status references
onSuccess but does not include it in the dependency array; update the useEffect
dependency array to include onSuccess (i.e., [deletedAssignment?.status,
dispatch, onClose, assignmentData.name, onSuccess]) to avoid stale closure bugs,
or if onSuccess is expected to be stable, ensure it is memoized (wrap in
useCallback where it is defined) and then include it in the dependency list used
in the AssignmentDelete useEffect.
In `@src/pages/Questionnaires/QuestionnaireEditor.tsx`:
- Around line 34-55: In mapFetchedItemToFormItem, the size parsing assumes a
comma-separated "width,height" string and can leave height undefined for inputs
like "60"; update the parsing logic for item.size to handle strings and numbers
robustly: when typeof item.size === "string" split by "," then trim and filter
out empty parts, if the result has two parts assign width=parts[0],
height=parts[1]; if it has one part assign width=parts[0] and height="" (or vice
versa per expected semantics); also handle numeric item.size by converting to
string or treating it as a single width; then use these safe width/height values
in textarea_width and textarea_height assignments in mapFetchedItemToFormItem.
In `@src/pages/Questionnaires/QuestionnaireUtils.tsx`:
- Line 119: Remove the debug console.log statements that print "Original Form
Values:" and any other console.log usages at the noted locations in
QuestionnaireUtils.tsx; locate the console.log("Original Form Values:", values)
and the console.log at line ~136 (both inside the same helper/function that
handles form values) and delete those lines so no debug logging remains in the
production code.
---
Outside diff comments:
In `@src/pages/Questionnaires/QuestionnaireItemsFieldArray.tsx`:
- Around line 369-387: The fallback remove Button currently lacks an explicit
type and may act as a submit button; update the Button (the one rendering the
delete-icon and using values.items[index],
setFieldValue(`items[${index}]._destroy`, true) and remove(index)) to include
type="button" so it does not trigger form submission.
---
Nitpick comments:
In `@src/pages/Courses/Course.tsx`:
- Around line 47-62: The current useEffect fetches whenever any modal visibility
flips because it uses || with modal visibilities and includes those visibilities
in the dependency array; change the condition to require both modals to be
closed (use && for showDeleteConfirmation.visible and
showCopyConfirmation.visible) so fetches only run when neither modal is open,
and remove showDeleteConfirmation.visible and showCopyConfirmation.visible from
the effect dependency array to avoid modal toggles retriggering fetches;
additionally, instead of relying on visibility changes to refetch, call
fetchCourses/fetchInstitutions/fetchInstructors explicitly from the modal
close/success handlers (e.g., the delete/copy success callbacks) to refresh data
after an operation.
In `@src/pages/Courses/CourseAssignments.tsx`:
- Around line 49-74: The handler functions (onEditHandle, onDeleteHandle,
onAddParticipantHandle, onCreateTeamsHandle, onAssignReviewersHandle,
onViewSubmissionsHandle, onViewScoresHandle, onViewReportsHandle) are recreated
every render which forces actionHandlers useMemo to recompute; wrap each handler
in React.useCallback and provide stable dependency arrays (e.g., include
navigate and location for onEditHandle, include setShowDeleteConfirmation for
onDeleteHandle, and include navigate for the others) so the handlers remain
stable and actionHandlers can actually memoize.
In `@src/pages/Questionnaires/QuestionnaireEditor.tsx`:
- Around line 11-32: The displayQuestionnaireType function duplicates the
inverse of questionnaireTypeMap in QuestionnaireUtils.tsx; move the canonical
mapping and/or a helper that returns display names into QuestionnaireUtils.tsx
(e.g., export a getDisplayName or a bidirectional mapping like
questionnaireTypeMap and questionnaireDisplayMap) and update
displayQuestionnaireType to import and reuse that helper/map (referencing
displayQuestionnaireType and questionnaireTypeMap) so you maintain a single
source of truth and handle unknown keys by falling back to the raw
questionnaireType or empty string.
In `@src/pages/Questionnaires/QuestionnaireForm.tsx`:
- Around line 72-89: The item type normalization in this component (see
normalizedItemTypes and fallbackItemTypes) duplicates the inverse mapping
already implemented by normalizedQuestionType() in
QuestionnaireItemsFieldArray.tsx; extract a single normalization utility (e.g.,
normalizeQuestionType) into a shared module (QuestionnaireUtils.tsx) that
handles the mappings ("TextArea" <-> "Text area", "TextField" <-> "Text field",
"MultipleChoiceRadio" <-> "Multiple choice", etc.), replace the local mapping in
QuestionnaireForm.tsx with a call to that utility, and update
QuestionnaireItemsFieldArray.tsx to use the same utility so both places
reference the single source of truth.
In `@src/pages/Questionnaires/QuestionnaireItemsFieldArray.tsx`:
- Around line 127-145: Extract the duplicated remove-button JSX into a local
React component named RemoveItemButton and replace the 7 inline occurrences with
it; the component should accept props (index: number, values: any,
setFieldValue: (field: string, value: any) => void, remove: (index: number) =>
void) and implement the same onClick logic used currently (read item from
values.items[index], if item.id setFieldValue(`items[${index}]._destroy`, true)
else call remove(index)), render the same Button and img markup and preserve
aria-label/className; update callers to pass index, values, setFieldValue and
remove so behavior is identical.
- Around line 35-43: Extract the duplicated normalization logic by moving the
normalizedQuestionType function (used in QuestionnaireItemsFieldArray.tsx) and
the inverse mapping in QuestionnaireForm.tsx into a shared utility module (e.g.,
QuestionnaireUtils.tsx) alongside the existing type mappings; create a single
exported function name (normalizedQuestionType) and, if needed, an inverse
helper (denormalizeQuestionType) in that module, update both
QuestionnaireItemsFieldArray.tsx and QuestionnaireForm.tsx to import and use
these utilities instead of local copies, and ensure you export the mappings so
other components can reference the single source of truth.
In `@src/pages/Questionnaires/QuestionnaireUtils.tsx`:
- Around line 166-172: The loader function loadQuestionnaireHierarchy lacks
error handling for the axiosClient.get call; wrap the await
axiosClient.get('/questionnaires/hierarchical') and subsequent mapping in a
try-catch inside loadQuestionnaireHierarchy, catch network/response errors, log
or process the error (using your logger or console), and rethrow a controlled
error (or throw a Response for React Router errorElement) so upstream can handle
it; ensure you still map group.questionnaires with
transformQuestionnaireResponse on success and do not swallow the original error
details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 71bdde17-2c0d-432b-b3ac-5e5c560013a6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
.gitignorepackage.jsonsrc/App.tsxsrc/pages/Assignments/AssignmentDelete.tsxsrc/pages/Assignments/AssignmentUtil.tssrc/pages/Courses/Course.tsxsrc/pages/Courses/CourseAssignments.tsxsrc/pages/Questionnaires/Questionnaire.tsxsrc/pages/Questionnaires/QuestionnaireEditor.tsxsrc/pages/Questionnaires/QuestionnaireForm.tsxsrc/pages/Questionnaires/QuestionnaireItemsFieldArray.tsxsrc/pages/Questionnaires/QuestionnaireUtils.tsxsrc/utils/interfaces.ts
| "recharts": "^2.15.4", | ||
| "redux-persist": "^6.0.0", | ||
| "sass": "^1.62.1", | ||
| "sass-embedded": "^1.98.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
`For Vite projects, verify:
- compiler selection order when both
sassandsass-embeddedare installed - whether differing versions between the two can produce different CSS output
- any recommended practice from Sass/Vite docs for keeping both dependencies in sync`
💡 Result:
In Vite projects, when both sass and sass-embedded are installed, Vite prioritizes sass-embedded by first attempting to load it via loadSassPackage, which tries sass-embedded before falling back to sass. This behavior is implemented in Vite's source code and documented in official configuration notes stating "Uses sass-embedded if installed, otherwise uses sass." Differing versions between sass and sass-embedded can potentially produce different CSS output if the packages are not kept in sync, as they share the same underlying Dart Sass implementation but may differ if versions diverge. However, Sass maintainers ensure both packages are released simultaneously with identical version numbers for feature parity. Minor differences might arise from limited API support (e.g., sass-embedded lacks some legacy options like precision), but for standard modern usage, output should be identical. Vite and Sass docs recommend installing only sass-embedded (not both) for best performance: npm add -D sass-embedded. Official Vite docs explicitly state "For the best performance, it's recommended to install the sass-embedded package." Sass docs position sass-embedded as a drop-in replacement. No docs suggest keeping both in sync; instead, uninstall sass and use sass-embedded alone to avoid conflicts and ensure consistency.
Citations:
- 1: feat(css): support sass compiler api and sass-embedded package vitejs/vite#17754
- 2: vitejs/vite@3bfe5c5
- 3: vitejs/vite@1025bb6
- 4: https://vitejs.dev/config/shared-options.html
- 5: https://vite.dev/config/shared-options.html
- 6: https://www.npmjs.com/package/sass-embedded
- 7: https://sass-lang.com/blog/embedded-sass-is-live
- 8: https://github.com/sass/embedded-host-node/blob/main/README.md
- 9: https://www.oddbird.net/2024/08/14/sass-compiler/
- 10: https://vite.dev/guide/features
Remove the sass dependency; keep only sass-embedded for Sass compilation.
Having both sass and sass-embedded installed is not recommended. Vite prioritizes sass-embedded when both are present, and official Vite and Sass documentation explicitly recommend installing only sass-embedded for best performance and consistency. Sass maintains version parity between the packages, so alignment is unnecessary; instead, remove the redundant sass dependency entirely.
♻️ Proposed update
- "sass": "^1.62.1",
"sass-embedded": "^1.98.0",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 39, The package.json currently includes both "sass" and
"sass-embedded"; remove the "sass" dependency and keep only "sass-embedded" so
Vite uses the supported embedded implementation. Edit package.json to delete the
"sass" entry from the dependencies (leaving "sass-embedded": "^1.98.0") and then
run package manager cleanup (npm/yarn/pnpm install or prune) to ensure only
sass-embedded is installed.
Comment on lines
+46
to
49
| if (onSuccess) onSuccess(); | ||
| onClose(); | ||
| } | ||
| }, [deletedAssignment?.status, dispatch, onClose, assignmentData.name]); |
There was a problem hiding this comment.
Missing onSuccess in useEffect dependency array.
The onSuccess callback is used inside the effect but not listed in the dependency array. This can lead to stale closure bugs if the callback reference changes between renders.
🔧 Proposed fix
- }, [deletedAssignment?.status, dispatch, onClose, assignmentData.name]);
+ }, [deletedAssignment?.status, dispatch, onClose, onSuccess, assignmentData.name]); 📝 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.
Suggested change
| if (onSuccess) onSuccess(); | |
| onClose(); | |
| } | |
| }, [deletedAssignment?.status, dispatch, onClose, assignmentData.name]); | |
| if (onSuccess) onSuccess(); | |
| onClose(); | |
| } | |
| }, [deletedAssignment?.status, dispatch, onClose, onSuccess, assignmentData.name]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Assignments/AssignmentDelete.tsx` around lines 46 - 49, The effect
watching deletedAssignment?.status references onSuccess but does not include it
in the dependency array; update the useEffect dependency array to include
onSuccess (i.e., [deletedAssignment?.status, dispatch, onClose,
assignmentData.name, onSuccess]) to avoid stale closure bugs, or if onSuccess is
expected to be stable, ensure it is memoized (wrap in useCallback where it is
defined) and then include it in the dependency list used in the AssignmentDelete
useEffect.
Comment on lines
+34
to
+55
| const mapFetchedItemToFormItem = (item: any) => { | ||
| const size = typeof item.size === "string" ? item.size.split(",").map((value: string) => value.trim()) : []; | ||
| const [width, height] = size; | ||
|
|
||
| return { | ||
| id: item.id, | ||
| txt: item.txt, | ||
| question_type: item.question_type, | ||
| weight: item.weight ?? "", | ||
| alternatives: item.alternatives ? item.alternatives.split("|").join(", ") : "", | ||
| min_label: item.min_label ?? "", | ||
| max_label: item.max_label ?? "", | ||
| textarea_width: item.question_type === "Criterion" || item.question_type === "TextArea" ? width ?? "" : "", | ||
| textarea_height: item.question_type === "Criterion" || item.question_type === "TextArea" ? height ?? "" : "", | ||
| textbox_width: item.question_type === "TextField" ? item.size ?? "" : "", | ||
| col_names: item.col_names ?? "", | ||
| row_names: item.row_names ?? "", | ||
| seq: item.seq, | ||
| break_before: item.break_before, | ||
| _destroy: item._destroy || false, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Handle edge cases in size parsing.
The size string splitting on line 35 assumes comma-separated format but doesn't validate the input format. If item.size is a single number string (e.g., "60"), the destructuring will leave height as undefined.
🛡️ Suggested defensive improvement
const mapFetchedItemToFormItem = (item: any) => {
- const size = typeof item.size === "string" ? item.size.split(",").map((value: string) => value.trim()) : [];
- const [width, height] = size;
+ const size = typeof item.size === "string"
+ ? item.size.split(",").map((value: string) => value.trim())
+ : [];
+ const width = size[0] ?? "";
+ const height = size[1] ?? "";📝 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.
Suggested change
| const mapFetchedItemToFormItem = (item: any) => { | |
| const size = typeof item.size === "string" ? item.size.split(",").map((value: string) => value.trim()) : []; | |
| const [width, height] = size; | |
| return { | |
| id: item.id, | |
| txt: item.txt, | |
| question_type: item.question_type, | |
| weight: item.weight ?? "", | |
| alternatives: item.alternatives ? item.alternatives.split("|").join(", ") : "", | |
| min_label: item.min_label ?? "", | |
| max_label: item.max_label ?? "", | |
| textarea_width: item.question_type === "Criterion" || item.question_type === "TextArea" ? width ?? "" : "", | |
| textarea_height: item.question_type === "Criterion" || item.question_type === "TextArea" ? height ?? "" : "", | |
| textbox_width: item.question_type === "TextField" ? item.size ?? "" : "", | |
| col_names: item.col_names ?? "", | |
| row_names: item.row_names ?? "", | |
| seq: item.seq, | |
| break_before: item.break_before, | |
| _destroy: item._destroy || false, | |
| }; | |
| }; | |
| const mapFetchedItemToFormItem = (item: any) => { | |
| const size = typeof item.size === "string" | |
| ? item.size.split(",").map((value: string) => value.trim()) | |
| : []; | |
| const width = size[0] ?? ""; | |
| const height = size[1] ?? ""; | |
| return { | |
| id: item.id, | |
| txt: item.txt, | |
| question_type: item.question_type, | |
| weight: item.weight ?? "", | |
| alternatives: item.alternatives ? item.alternatives.split("|").join(", ") : "", | |
| min_label: item.min_label ?? "", | |
| max_label: item.max_label ?? "", | |
| textarea_width: item.question_type === "Criterion" || item.question_type === "TextArea" ? width ?? "" : "", | |
| textarea_height: item.question_type === "Criterion" || item.question_type === "TextArea" ? height ?? "" : "", | |
| textbox_width: item.question_type === "TextField" ? item.size ?? "" : "", | |
| col_names: item.col_names ?? "", | |
| row_names: item.row_names ?? "", | |
| seq: item.seq, | |
| break_before: item.break_before, | |
| _destroy: item._destroy || false, | |
| }; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Questionnaires/QuestionnaireEditor.tsx` around lines 34 - 55, In
mapFetchedItemToFormItem, the size parsing assumes a comma-separated
"width,height" string and can leave height undefined for inputs like "60";
update the parsing logic for item.size to handle strings and numbers robustly:
when typeof item.size === "string" split by "," then trim and filter out empty
parts, if the result has two parts assign width=parts[0], height=parts[1]; if it
has one part assign width=parts[0] and height="" (or vice versa per expected
semantics); also handle numeric item.size by converting to string or treating it
as a single width; then use these safe width/height values in textarea_width and
textarea_height assignments in mapFetchedItemToFormItem.
| Quiz: "QuizQuestionnaire", | ||
| }; | ||
|
|
||
| console.log("Original Form Values:", values); |
There was a problem hiding this comment.
Remove debug console.log statements.
These console.log statements (lines 119 and 136) appear to be debugging artifacts and should be removed before merging.
🧹 Proposed fix
- console.log("Original Form Values:", values);
const questionnaire: QuestionnaireRequest = {
name: values.name,
...
};
- console.log("Transformed Questionnaire Request:", questionnaire);
return { questionnaire };Also applies to: 136-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Questionnaires/QuestionnaireUtils.tsx` at line 119, Remove the
debug console.log statements that print "Original Form Values:" and any other
console.log usages at the noted locations in QuestionnaireUtils.tsx; locate the
console.log("Original Form Values:", values) and the console.log at line ~136
(both inside the same helper/function that handles form values) and delete those
lines so no debug logging remains in the production code.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
All details can be found here: https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2026_-_E2608._Hierarchical_list_display
Hosted at: http://152.7.177.90:3000/
Summary by CodeRabbit
Release Notes
New Features
Chores