E2606 Final Project Frontend PR#178
Open
nadkarnik wants to merge 44 commits into
Open
Conversation
…Topics). Tried to keep GUI consistency with User import/export interface.
…ill add project_topic seeds to db).
…t_participants route relies on assignments now. Participants views load properly and also populate correctly into teams. Additionally permitted more context to be passed from frontend to backend so system can know what assignment's teams are being imported/exported.
…or-imp-exp-teams-topics Kdnadkar/frontend changes for imp exp teams topics
…opulating-correctly-frontend Kdnadkar/fixing teams not populating correctly frontend
…ownload fixing duplicate downloads of csv export
…ation Integrate graph export into export with switch
…uestionnaire-import Add questionnaire import button
…ng-spammed-GETs Added timer to ImportModal
…lank_header_import Add option to download sample csv when importing
…_changes Mskamat/final changes
…xport-frontend Nadkarnik/questionnaire import export frontend
…ades-export added frontend components for grades export
…t-participant-export Nadkarnik/frontend for assignment participant export
Add context parameters to Import and Export modals in TopicsTab
📝 WalkthroughWalkthroughAdds context-aware Import/Export modals (multi-file exports, synthetic Team fields), a GradesExportModal and grades loading, moves team editing to server-driven CRUD, enriches participant import/export and routing (assignment participants rendered with type="assignments"), and introduces questionnaire package import/export with selection support. Changes
Sequence DiagramssequenceDiagram
participant User
participant UI as CreateTeams UI
participant Server
participant State as Component State
User->>UI: Trigger team action (add/remove/rename/delete)
UI->>Server: POST/PATCH/DELETE /teams...
Server-->>UI: Operation result
alt success
UI->>Server: GET /teams
Server-->>UI: Teams data
UI->>Server: GET /participants/assignment/:id
Server-->>UI: Participant data
UI->>State: Update teams, recompute unassigned, reset UI expansion
else error
Server-->>UI: Error
UI->>User: Show error
end
sequenceDiagram
participant User
participant ViewScores as ViewScores Page
participant Server
participant Parser as CSV Parser
participant Graph as Chart Component
ViewScores->>Server: GET /grades/{assignmentId}/export?include_email=true
Server-->>ViewScores: CSV text
ViewScores->>Parser: Parse CSV → rows[]
Parser-->>ViewScores: Normalized rows with numeric grades
ViewScores->>Graph: Render chart with numeric grades
User->>ViewScores: Click "Export Grades"
ViewScores->>Server: GET /grades/{assignmentId}/export (blob)
Server-->>ViewScores: CSV blob
ViewScores->>User: Trigger file download (timestamped name)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 0/3 reviews remaining, refill in 46 minutes and 5 seconds. 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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Modals/ImportModal.tsx (1)
241-261:⚠️ Potential issue | 🟠 MajorGuard the file picker against empty selections.
on_file_changedcan now receivenullwhen the user cancels or clears the file input, but the handler still treats the value as aFileand calls.text()unconditionally. That will throw and leave the modal in a broken state.🛠️ Suggested fix
- const on_file_changed = async (incomingFile: File) => { - setFile(incomingFile); + const on_file_changed = async (incomingFile: File | null) => { + if (!incomingFile) { + setFile(null); + setCsvData([]); + setCsvHeaders([]); + setSelectedFields([]); + return; + } + + setFile(incomingFile); if (availableFields.length === 0) return;Also applies to: 564-568
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Modals/ImportModal.tsx` around lines 241 - 261, The on_file_changed handler currently assumes incomingFile is a File and calls incomingFile.text(), which throws if the file picker returns null; update on_file_changed to first guard that incomingFile is non-null and is an instance of File (or check typeof incomingFile?.text === 'function') and return early if not, so the modal state is not modified; apply the same null-check pattern to the other file handler (the similar block around lines 564-568) to avoid calling .text() on null and to avoid setting state when no file was selected.
🧹 Nitpick comments (1)
src/pages/Courses/Course.tsx (1)
183-183: Fix typo in inline comment for clarity.Line 183 uses “doenot”; consider correcting it to “does not”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Courses/Course.tsx` at line 183, Update the inline comment in the Course component that currently reads "Admin doenot have the option to create a course but he can create an assignment" to correct the typo by changing "doenot" to "does not" so the comment reads "Admin does not have the option to create a course but he can create an assignment"; locate this comment inside the Course component in Course.tsx and replace the misspelled word only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Modals/ExportModal.tsx`:
- Around line 185-199: The external_fields from exportResponse.data are
currently assigned raw causing duplicate participant columns; run them through
normalizeTeamFieldList like mandatory_fields and optional_fields before calling
setExternalFields and before composing fields for setAllFields and
setSelectedFields (i.e., replace direct use of
exportResponse.data.external_fields with a normalizedExternalFields variable
produced by normalizeTeamFieldList), so expandTeamFieldSelection and other
consumers see normalized team field names.
In `@src/components/Modals/GradesExportModal.tsx`:
- Around line 134-137: The auto-close timer created with setTimeout after a
successful export (where setStatus("") and onHide() are called) must be tracked
and cleared to avoid race conditions; add a ref (e.g., exportTimeoutRef) to
store the timeout id when you call setTimeout in the GradesExportModal, clear
any existing timeout before creating a new one, add an effect that watches the
show prop (or whatever prop controls opening) to clear the ref/timeout whenever
the modal reopens, and add a cleanup effect (return function in useEffect) to
clear the timeout on unmount so stale timers cannot call onHide on a new
session.
In `@src/pages/Assignments/AssignmentEditor.tsx`:
- Around line 55-63: The buildTopicSettings helper currently hardcodes five
fields so user edits are not persisted; update buildTopicSettings to read all
seven flags from the assignment payload (e.g. map
assignment.allow_topic_suggestions, assignment.enable_bidding,
assignment.enable_authors_review, assignment.allow_reviewer_choice,
assignment.allow_bidding_for_reviewers, assignment.allow_bookmarks,
assignment.advertising_for_partners_allowed) into the TopicSettings return
object so the UI-controlled checkboxes reflect persisted values, and also update
the initial topicSettings state initialization (the state created around the
TopicsTab/topicSettings usage) to use this same buildTopicSettings(assignment)
instead of hardcoded defaults; alternatively, if backend fields do not exist,
remove the five uncontrolled checkbox UI controls in TopicsTab to avoid
mismatched state.
In `@src/pages/Assignments/CreateTeams.tsx`:
- Around line 1374-1383: The fetched Teams mapping for CreateTeams loses mentor
data for MentoredTeam entries, so populate the mentor field when mapping backend
payload to Team objects: in the mapping that creates fetchedTeams (the arrow
producing { id, name, members }), add mentor: team.mentor (or map it via
mapUserToParticipant(team.mentor, team.name) if the mentor needs the same shape)
for entries where team.type === 'MentoredTeam', ensuring mapUserToParticipant is
used to shape the mentor consistently and preserving mentor so downstream code
(Lines referencing mentor banner and member filtering) works correctly.
- Around line 1315-1323: The code is incorrectly relying on (loader as any).id;
change assignmentId to derive from route params with a loader fallback: import
and call useParams() (or the appropriate route params helper) to get the
assignment id param (e.g., const { id: routeId } = useParams()), then set const
assignmentId = String(routeId || loader.id || '') and update usages that assume
assignmentId (lines referencing assignmentId, initial sync code, and functions
handling new/imported teams) to use this variable; alternatively, if you prefer
the loader approach make id required on the LoaderPayload type and propagate
that change so assignmentId can safely use loader.id directly. Ensure the change
is applied where assignmentId is declared and where it's consumed (e.g., code
around assignmentId, ctxType/ctxName usage, initial sync and team create/import
handlers).
- Around line 1548-1557: The deleteAllTeams callback uses Promise.all which
aborts on the first failure and prevents UI refresh; change it to use
Promise.allSettled over teams.map(team =>
axiosClient.delete(`/teams/${team.id}`)), inspect the settled results to log or
surface any individual failures (e.g., filter results with status ===
'rejected'), and ensure refreshAssignmentTeams() is called in a finally block so
the UI refreshes regardless of partial success or failure; keep existing error
logging but move it to handle rejected results from Promise.allSettled while
still returning after the confirmation check.
In `@src/pages/Assignments/ViewScores.tsx`:
- Around line 47-54: The current parseGradesCsv function splits csvText by
newlines which breaks rows that contain quoted multiline fields (e.g., comment),
so replace the naive split with a quote-aware row splitter: implement a small
scanner inside parseGradesCsv that iterates csvText character-by-character,
toggles an inQuotes flag when encountering double quotes (and handles escaped
quotes), accumulates characters into a buffer, and only treats '\n' or '\r\n' as
row delimiters when not inQuotes; then parse the first buffered row with
parseCsvLine to get headers and map subsequent buffered rows with parseCsvLine
to values. Keep references to parseGradesCsv and parseCsvLine so the change is
localized to that function.
In `@src/pages/Questionnaires/Questionnaire.tsx`:
- Around line 119-135: selectAllQuestionnaires and the onToggleAll logic
currently compare against tableData.length which is incorrect when some rows
have non-numeric IDs or stale selections; change both to derive a selectableIds
list/set from tableData (e.g., map questionnaire.id and filter to numeric IDs)
and use selectableIds.length (or selectableIds.size) for the select-all
comparison and for the conditional in setSelectedQuestionnaireIds in
onToggleAll, and when selecting all return a new Set(selectableIds) so only
valid numeric IDs are tracked (update references: selectAllQuestionnaires,
selectedQuestionnaireIds, setSelectedQuestionnaireIds, onToggleAll, and
tableData).
In `@src/pages/Questionnaires/QuestionnairePackageExportModal.tsx`:
- Around line 56-60: The payload construction silently falls back to {
export_all: true } when scope === "selected" but selectedQuestionnaires contains
no numeric IDs; change the logic so you compute const selectedIds =
selectedQuestionnaires.map(q => q.id).filter(id => typeof id === "number") and
if scope === "selected" then if selectedIds.length === 0 prevent submit and
surface a validation error (or force the user to change scope) instead of
building an export_all payload; otherwise set payload = { questionnaire_ids:
selectedIds }. Keep the existing checks for canExportSelected but ensure the
empty-ID case does not fall back to export_all.
In `@src/pages/Questionnaires/QuestionnairePackageImportModal.tsx`:
- Around line 107-124: Reset the duplicate-action state immediately when opening
the modal to avoid stale options: call setDuplicateActions([]) and
setDuplicateAction("") before initiating axiosClient.get in
QuestionnairePackageImportModal (where the current reset block and
axiosClient.get("/questionnaire_packages/config") live) so the UI is cleared
while the config loads (and remains cleared if the fetch fails).
- Around line 230-241: The current try/catch wraps both the actual upload
(axiosClient.post("/questionnaire_packages/import", ...)) and the post-import
callback onImported(), so if onImported() throws you incorrectly report an
import failure; fix by isolating the network call and its success handling from
the refresh: await the axiosClient.post call and setStatus/preview immediately
on success, then call onImported() in a separate try/catch (or .catch) that
handles refresh errors without changing the import success message (e.g., log or
set a different status), and only report import failure from the catch that
surrounds axiosClient.post; keep calls to setPreview(null) and onHide()
consistent so they run on successful import regardless of refresh outcome.
In `@src/pages/Users/UserEditor.tsx`:
- Around line 225-228: Update the form field components (FormInput, FormSelect,
FormCheckBox) to compute their invalid state using submitCount as well as
touched: replace existing isInvalid logic with something like isInvalid =
(form.touched[field.name] || form.submitCount > 0) && !isValid so validation
errors show after a submit attempt; also add error rendering to
FormCheckboxGroup (it currently lacks any error display) using the same
isInvalid/ error message logic so checkbox groups display errors when
form.submitCount > 0 or the field is touched.
In `@src/pages/Users/userUtil.ts`:
- Around line 77-100: Add a JSDoc block above the parseFullName function
describing the supported input formats and edge cases: state that it accepts
"lastName, firstName" (comma-delimited) and a whitespace-delimited fallback,
note that comma parsing stops at the first comma (names containing commas will
be truncated), explain the whitespace heuristic (last token is treated as
lastName, rest as firstName) and that this may mis-handle non-Western naming
conventions, and document its behavior for empty/whitespace input (returns empty
firstName/lastName). Also reference the parseFullName symbol so reviewers and
callers (e.g., participantUtil usage) know which function the docs describe.
---
Outside diff comments:
In `@src/components/Modals/ImportModal.tsx`:
- Around line 241-261: The on_file_changed handler currently assumes
incomingFile is a File and calls incomingFile.text(), which throws if the file
picker returns null; update on_file_changed to first guard that incomingFile is
non-null and is an instance of File (or check typeof incomingFile?.text ===
'function') and return early if not, so the modal state is not modified; apply
the same null-check pattern to the other file handler (the similar block around
lines 564-568) to avoid calling .text() on null and to avoid setting state when
no file was selected.
---
Nitpick comments:
In `@src/pages/Courses/Course.tsx`:
- Line 183: Update the inline comment in the Course component that currently
reads "Admin doenot have the option to create a course but he can create an
assignment" to correct the typo by changing "doenot" to "does not" so the
comment reads "Admin does not have the option to create a course but he can
create an assignment"; locate this comment inside the Course component in
Course.tsx and replace the misspelled word only.
🪄 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: 12f89623-272a-4103-a198-40793b9376bb
📒 Files selected for processing (21)
src/App.tsxsrc/components/Modals/ExportModal.tsxsrc/components/Modals/GradesExportModal.tsxsrc/components/Modals/ImportModal.test.tsxsrc/components/Modals/ImportModal.tsxsrc/hooks/useAPI.tssrc/pages/Assignments/AssignmentEditor.tsxsrc/pages/Assignments/CreateTeams.tsxsrc/pages/Assignments/ViewScores.tsxsrc/pages/Assignments/tabs/TopicsTab.tsxsrc/pages/Courses/Course.tsxsrc/pages/Courses/CourseAssignments.tsxsrc/pages/Participants/Participant.tsxsrc/pages/Participants/ParticipantEditor.tsxsrc/pages/Questionnaires/Questionnaire.tsxsrc/pages/Questionnaires/QuestionnaireColumns.tsxsrc/pages/Questionnaires/QuestionnairePackageExportModal.tsxsrc/pages/Questionnaires/QuestionnairePackageImportModal.tsxsrc/pages/Users/User.tsxsrc/pages/Users/UserEditor.tsxsrc/pages/Users/userUtil.ts
Comment on lines
+185
to
+199
| const normalizedMandatoryFields = normalizeTeamFieldList(exportResponse.data.mandatory_fields); | ||
| const normalizedOptionalFields = normalizeTeamFieldList(exportResponse.data.optional_fields); | ||
|
|
||
| setMandatoryFields(normalizedMandatoryFields); | ||
| setOptionalFields(normalizedOptionalFields); | ||
| setExternalFields(exportResponse.data.external_fields); | ||
|
|
||
| const fields = [ | ||
| ...exportResponse.data.mandatory_fields, | ||
| ...exportResponse.data.optional_fields, | ||
| ...normalizedMandatoryFields, | ||
| ...normalizedOptionalFields, | ||
| ...exportResponse.data.external_fields | ||
| ] | ||
| ]; | ||
|
|
||
| setAllFields(fields) | ||
| setSelectedFields(exportResponse.data.mandatory_fields) | ||
| setAllFields(Array.from(new Set(fields))); | ||
| setSelectedFields(normalizedMandatoryFields); |
There was a problem hiding this comment.
Normalize external_fields for Team exports too.
Only mandatory_fields and optional_fields go through normalizeTeamFieldList. If the export config exposes any participant_* external fields, the modal will show raw participant_* entries beside synthetic participant_ids, and expandTeamFieldSelection can emit duplicate participant columns. Apply the same normalization before storing externalFields and building allFields.
Possible fix
- setExternalFields(exportResponse.data.external_fields);
+ const normalizedExternalFields = normalizeTeamFieldList(
+ exportResponse.data.external_fields,
+ );
+ setExternalFields(normalizedExternalFields);
const fields = [
...normalizedMandatoryFields,
...normalizedOptionalFields,
- ...exportResponse.data.external_fields
+ ...normalizedExternalFields,
];📝 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 normalizedMandatoryFields = normalizeTeamFieldList(exportResponse.data.mandatory_fields); | |
| const normalizedOptionalFields = normalizeTeamFieldList(exportResponse.data.optional_fields); | |
| setMandatoryFields(normalizedMandatoryFields); | |
| setOptionalFields(normalizedOptionalFields); | |
| setExternalFields(exportResponse.data.external_fields); | |
| const fields = [ | |
| ...exportResponse.data.mandatory_fields, | |
| ...exportResponse.data.optional_fields, | |
| ...normalizedMandatoryFields, | |
| ...normalizedOptionalFields, | |
| ...exportResponse.data.external_fields | |
| ] | |
| ]; | |
| setAllFields(fields) | |
| setSelectedFields(exportResponse.data.mandatory_fields) | |
| setAllFields(Array.from(new Set(fields))); | |
| setSelectedFields(normalizedMandatoryFields); | |
| const normalizedMandatoryFields = normalizeTeamFieldList(exportResponse.data.mandatory_fields); | |
| const normalizedOptionalFields = normalizeTeamFieldList(exportResponse.data.optional_fields); | |
| setMandatoryFields(normalizedMandatoryFields); | |
| setOptionalFields(normalizedOptionalFields); | |
| const normalizedExternalFields = normalizeTeamFieldList( | |
| exportResponse.data.external_fields, | |
| ); | |
| setExternalFields(normalizedExternalFields); | |
| const fields = [ | |
| ...normalizedMandatoryFields, | |
| ...normalizedOptionalFields, | |
| ...normalizedExternalFields | |
| ]; | |
| setAllFields(Array.from(new Set(fields))); | |
| setSelectedFields(normalizedMandatoryFields); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Modals/ExportModal.tsx` around lines 185 - 199, The
external_fields from exportResponse.data are currently assigned raw causing
duplicate participant columns; run them through normalizeTeamFieldList like
mandatory_fields and optional_fields before calling setExternalFields and before
composing fields for setAllFields and setSelectedFields (i.e., replace direct
use of exportResponse.data.external_fields with a normalizedExternalFields
variable produced by normalizeTeamFieldList), so expandTeamFieldSelection and
other consumers see normalized team field names.
Comment on lines
+134
to
+137
| setTimeout(() => { | ||
| setStatus(""); | ||
| onHide(); | ||
| }, 1200); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify timer usage and cleanup in the modal.
rg -n -C2 'setTimeout|clearTimeout|useRef|autoCloseTimerRef' src/components/Modals/GradesExportModal.tsxRepository: expertiza/reimplementation-front-end
Length of output: 214
🏁 Script executed:
cat -n src/components/Modals/GradesExportModal.tsxRepository: expertiza/reimplementation-front-end
Length of output: 9942
Clear the auto-close timer when the modal reopens or unmounts to prevent race conditions.
The setTimeout at lines 134–137 is created after a successful export but never cleared. If a user closes and reopens the modal within 1200ms, the old timer can unexpectedly call onHide() while the new session is active. This can be fixed by tracking the timeout in a useRef, clearing it in the show dependency effect, and adding a cleanup effect on unmount.
Proposed fix (track + clear timeout)
-import React, { memo, useEffect, useState } from "react";
+import React, { memo, useEffect, useRef, useState } from "react";
...
const [isExporting, setIsExporting] = useState(false);
const [status, setStatus] = useState("");
const [includeEmail, setIncludeEmail] = useState(false);
+ const autoCloseTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
useEffect(() => {
if (!show) return;
+ if (autoCloseTimerRef.current) {
+ clearTimeout(autoCloseTimerRef.current);
+ autoCloseTimerRef.current = null;
+ }
setStatus("");
setIncludeEmail(false);
}, [show]);
+
+ useEffect(
+ () => () => {
+ if (autoCloseTimerRef.current) clearTimeout(autoCloseTimerRef.current);
+ },
+ []
+ );
...
- setTimeout(() => {
+ autoCloseTimerRef.current = setTimeout(() => {
setStatus("");
+ autoCloseTimerRef.current = null;
onHide();
}, 1200);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Modals/GradesExportModal.tsx` around lines 134 - 137, The
auto-close timer created with setTimeout after a successful export (where
setStatus("") and onHide() are called) must be tracked and cleared to avoid race
conditions; add a ref (e.g., exportTimeoutRef) to store the timeout id when you
call setTimeout in the GradesExportModal, clear any existing timeout before
creating a new one, add an effect that watches the show prop (or whatever prop
controls opening) to clear the ref/timeout whenever the modal reopens, and add a
cleanup effect (return function in useEffect) to clear the timeout on unmount so
stale timers cannot call onHide on a new session.
Comment on lines
+55
to
+63
| const buildTopicSettings = (assignment?: any): TopicSettings => ({ | ||
| allowTopicSuggestions: false, | ||
| enableBidding: false, | ||
| enableAuthorsReview: true, | ||
| allowReviewerChoice: true, | ||
| allowBookmarks: Boolean(assignment?.allow_bookmarks), | ||
| allowBiddingForReviewers: false, | ||
| allowAdvertiseForPartners: Boolean(assignment?.advertising_for_partners_allowed), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'allowTopicSuggestions|enableBidding|enableAuthorsReview|allowReviewerChoice|allowBookmarks|allowBiddingForReviewers|allowAdvertiseForPartners|allow_bookmarks|advertising_for_partners_allowed' src/pages/AssignmentsRepository: expertiza/reimplementation-front-end
Length of output: 15734
Mapping incomplete: five topic settings are hardcoded defaults and not persisted.
The buildTopicSettings helper only reads allow_bookmarks and advertising_for_partners_allowed from the assignment object. The remaining five fields (allowTopicSuggestions, enableBidding, enableAuthorsReview, allowReviewerChoice, allowBiddingForReviewers) are hardcoded to static defaults. Since the UI in TopicsTab renders editable checkboxes for all seven settings, user changes to these five fields are lost on page reload or when the loader refreshes the assignment data—only the hardcoded defaults are restored. The same issue applies to the initial state initialization at line 172-180.
Either persist all seven settings to the backend, or remove the UI controls for the five settings that are not synchronized.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Assignments/AssignmentEditor.tsx` around lines 55 - 63, The
buildTopicSettings helper currently hardcodes five fields so user edits are not
persisted; update buildTopicSettings to read all seven flags from the assignment
payload (e.g. map assignment.allow_topic_suggestions, assignment.enable_bidding,
assignment.enable_authors_review, assignment.allow_reviewer_choice,
assignment.allow_bidding_for_reviewers, assignment.allow_bookmarks,
assignment.advertising_for_partners_allowed) into the TopicSettings return
object so the UI-controlled checkboxes reflect persisted values, and also update
the initial topicSettings state initialization (the state created around the
TopicsTab/topicSettings usage) to use this same buildTopicSettings(assignment)
instead of hardcoded defaults; alternatively, if backend fields do not exist,
remove the five uncontrolled checkbox UI controls in TopicsTab to avoid
mismatched state.
Comment on lines
+1315
to
+1323
| const assignmentId = String((loader as any)?.id || ''); | ||
|
|
||
| // Context | ||
| const ctxType = (contextType || loader.contextType || 'assignment') as ContextType; | ||
| const ctxName = contextName || loader.contextName || 'Program'; | ||
|
|
||
| // Initial data | ||
| const baseTeams = loader.initialTeams || sampleTeams; | ||
| const baseUnassigned = loader.initialUnassigned || sampleUnassigned; | ||
| const baseTeams = loader.initialTeams || []; | ||
| const baseUnassigned = loader.initialUnassigned || []; |
There was a problem hiding this comment.
Stop relying on (loader as any).id for the assignment scope.
LoaderPayload still does not declare id, but Line 1366, Line 1537, and Lines 1753-1759 now assume assignmentId is always present. If the loader ever returns only the typed fields, the initial sync becomes a no-op and new/imported teams lose their assignment context. Please derive this from the route params or make id a required loader field.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Assignments/CreateTeams.tsx` around lines 1315 - 1323, The code is
incorrectly relying on (loader as any).id; change assignmentId to derive from
route params with a loader fallback: import and call useParams() (or the
appropriate route params helper) to get the assignment id param (e.g., const {
id: routeId } = useParams()), then set const assignmentId = String(routeId ||
loader.id || '') and update usages that assume assignmentId (lines referencing
assignmentId, initial sync code, and functions handling new/imported teams) to
use this variable; alternatively, if you prefer the loader approach make id
required on the LoaderPayload type and propagate that change so assignmentId can
safely use loader.id directly. Ensure the change is applied where assignmentId
is declared and where it's consumed (e.g., code around assignmentId,
ctxType/ctxName usage, initial sync and team create/import handlers).
Comment on lines
+1374
to
+1383
| const fetchedTeams: Team[] = (Array.isArray(teamsResponse.data) ? teamsResponse.data : []) | ||
| .filter((team: any) => | ||
| String(team.assignment_id ?? team.parent_id) === assignmentId && | ||
| ['AssignmentTeam', 'MentoredTeam'].includes(team.type), | ||
| ) | ||
| .map((team: any) => ({ | ||
| id: team.id, | ||
| name: team.name, | ||
| members: (team.users || []).map((user: any) => mapUserToParticipant(user, team.name)), | ||
| })); |
There was a problem hiding this comment.
Preserve mentor data when hydrating MentoredTeams.
This mapper drops mentor entirely. After the first refresh, team.mentor is always undefined, so Line 1654 never renders the mentor banner and Line 1638 can no longer filter mentor users out of the member list. MentoredTeams need the mentor field populated from the backend payload here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Assignments/CreateTeams.tsx` around lines 1374 - 1383, The fetched
Teams mapping for CreateTeams loses mentor data for MentoredTeam entries, so
populate the mentor field when mapping backend payload to Team objects: in the
mapping that creates fetchedTeams (the arrow producing { id, name, members }),
add mentor: team.mentor (or map it via mapUserToParticipant(team.mentor,
team.name) if the mentor needs the same shape) for entries where team.type ===
'MentoredTeam', ensuring mapUserToParticipant is used to shape the mentor
consistently and preserving mentor so downstream code (Lines referencing mentor
banner and member filtering) works correctly.
Comment on lines
+107
to
+124
| setImportMode("csv"); | ||
| setPackageFile(null); | ||
| setQuestionnaireFile(null); | ||
| setItemsFile(null); | ||
| setQuestionAdvicesFile(null); | ||
| setTemplateNames([]); | ||
| setCsvHeaderRequirements({}); | ||
| setPreview(null); | ||
| setStatus(""); | ||
|
|
||
| axiosClient.get("/questionnaire_packages/config") | ||
| .then((response) => { | ||
| const actions = response.data.available_actions_on_dup || []; | ||
| setDuplicateActions(actions); | ||
| setDuplicateAction(actions[0] || ""); | ||
| setTemplateNames(response.data.available_templates || []); | ||
| setCsvHeaderRequirements(response.data.csv_header_requirements || {}); | ||
| }) |
There was a problem hiding this comment.
Reset duplicate-action state on modal open to avoid stale options.
If config load fails, previous duplicateActions / duplicateAction remain visible and can be submitted. Clear them before fetching config.
Proposed fix
setQuestionAdvicesFile(null);
+ setDuplicateActions([]);
+ setDuplicateAction("");
setTemplateNames([]);
setCsvHeaderRequirements({});
setPreview(null);
setStatus("");📝 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
| setImportMode("csv"); | |
| setPackageFile(null); | |
| setQuestionnaireFile(null); | |
| setItemsFile(null); | |
| setQuestionAdvicesFile(null); | |
| setTemplateNames([]); | |
| setCsvHeaderRequirements({}); | |
| setPreview(null); | |
| setStatus(""); | |
| axiosClient.get("/questionnaire_packages/config") | |
| .then((response) => { | |
| const actions = response.data.available_actions_on_dup || []; | |
| setDuplicateActions(actions); | |
| setDuplicateAction(actions[0] || ""); | |
| setTemplateNames(response.data.available_templates || []); | |
| setCsvHeaderRequirements(response.data.csv_header_requirements || {}); | |
| }) | |
| setImportMode("csv"); | |
| setPackageFile(null); | |
| setQuestionnaireFile(null); | |
| setItemsFile(null); | |
| setQuestionAdvicesFile(null); | |
| setDuplicateActions([]); | |
| setDuplicateAction(""); | |
| setTemplateNames([]); | |
| setCsvHeaderRequirements({}); | |
| setPreview(null); | |
| setStatus(""); | |
| axiosClient.get("/questionnaire_packages/config") | |
| .then((response) => { | |
| const actions = response.data.available_actions_on_dup || []; | |
| setDuplicateActions(actions); | |
| setDuplicateAction(actions[0] || ""); | |
| setTemplateNames(response.data.available_templates || []); | |
| setCsvHeaderRequirements(response.data.csv_header_requirements || {}); | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Questionnaires/QuestionnairePackageImportModal.tsx` around lines
107 - 124, Reset the duplicate-action state immediately when opening the modal
to avoid stale options: call setDuplicateActions([]) and setDuplicateAction("")
before initiating axiosClient.get in QuestionnairePackageImportModal (where the
current reset block and axiosClient.get("/questionnaire_packages/config") live)
so the UI is cleared while the config loads (and remains cleared if the fetch
fails).
Comment on lines
+230
to
+241
| try { | ||
| const response = await axiosClient.post("/questionnaire_packages/import", formData, { | ||
| headers: { "Content-Type": "multipart/form-data" }, | ||
| timeout: 120000 | ||
| }); | ||
|
|
||
| setStatus(response.data.message); | ||
| setPreview(null); | ||
| await onImported(); | ||
| setTimeout(() => onHide(), 800); | ||
| } catch (error: any) { | ||
| setStatus(error?.response?.data?.error || error?.message || "Failed to import questionnaire template package."); |
There was a problem hiding this comment.
Don’t report import failure when only post-import refresh fails.
await onImported() is in the same try as the import request. If refresh throws, the catch path reports import failure even when import already succeeded.
Proposed fix
const onImport = async () => {
@@
try {
const response = await axiosClient.post("/questionnaire_packages/import", formData, {
headers: { "Content-Type": "multipart/form-data" },
timeout: 120000
});
setStatus(response.data.message);
setPreview(null);
- await onImported();
+ try {
+ await onImported();
+ } catch (refreshError: any) {
+ setStatus(`${response.data.message} Imported successfully, but refresh failed.`);
+ }
setTimeout(() => onHide(), 800);
} catch (error: any) {
setStatus(error?.response?.data?.error || error?.message || "Failed to import questionnaire template package.");
} finally {
setIsImporting(false);
}
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Questionnaires/QuestionnairePackageImportModal.tsx` around lines
230 - 241, The current try/catch wraps both the actual upload
(axiosClient.post("/questionnaire_packages/import", ...)) and the post-import
callback onImported(), so if onImported() throws you incorrectly report an
import failure; fix by isolating the network call and its success handling from
the refresh: await the axiosClient.post call and setStatus/preview immediately
on success, then call onImported() in a separate try/catch (or .catch) that
handles refresh errors without changing the import success message (e.g., log or
set a different status), and only report import failure from the catch that
surrounds axiosClient.post; keep calls to setPreview(null) and onHide()
consistent so they run on successful import regardless of refresh outcome.
Comment on lines
225
to
+228
| <Button | ||
| variant="outline-success" | ||
| type="submit" | ||
| disabled={!(formik.isValid && formik.dirty) || formik.isSubmitting} | ||
| disabled={!formik.dirty || formik.isSubmitting} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in $(fd -a 'Form(Input|Select).*tsx?$' src components); do
echo "=== $f ==="
rg -n -C 3 'submitCount|touched|errors|meta\.error|Formik' "$f" || true
doneRepository: expertiza/reimplementation-front-end
Length of output: 2630
🏁 Script executed:
#!/bin/bash
# Read the UserEditor.tsx file to see the submit button and form configuration
head -300 src/pages/Users/UserEditor.tsx | tail -100Repository: expertiza/reimplementation-front-end
Length of output: 3831
🏁 Script executed:
#!/bin/bash
# Check the form setup around line 225-228 and look for Formik configuration
sed -n '200,280p' src/pages/Users/UserEditor.tsxRepository: expertiza/reimplementation-front-end
Length of output: 1545
🏁 Script executed:
#!/bin/bash
# Search for validateOnChange in UserEditor.tsx
rg -n 'validateOnChange|Formik' src/pages/Users/UserEditor.tsx | head -20Repository: expertiza/reimplementation-front-end
Length of output: 315
🏁 Script executed:
#!/bin/bash
# Check if there are any other form components besides FormInput and FormSelect
fd -type f \( -name '*.tsx' -o -name '*.ts' \) src/components/Form | head -20Repository: expertiza/reimplementation-front-end
Length of output: 251
🏁 Script executed:
#!/bin/bash
# Find and check FormCheckBoxGroup
fd -type f \( -name '*FormCheckBox*' \)Repository: expertiza/reimplementation-front-end
Length of output: 251
🏁 Script executed:
#!/bin/bash
# Search for FormCheckBoxGroup to see how it handles validation feedback
rg -n 'form\.touched|submitCount|form\.errors' src/components/Form/FormCheckBoxGroup.tsx 2>/dev/null || echo "File not found in expected location"Repository: expertiza/reimplementation-front-end
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Find all Form* components and their validation logic
fd -type f -name '*Form*.tsx' src/components/Form/ -exec basename {} \;Repository: expertiza/reimplementation-front-end
Length of output: 251
🏁 Script executed:
#!/bin/bash
# Check for any submitCount usage in the entire form components directory
rg -r 'submitCount' src/components/Form/Repository: expertiza/reimplementation-front-end
Length of output: 62
🏁 Script executed:
#!/bin/bash
# Find FormCheckBoxGroup file
find src -name '*CheckBox*' -o -name '*Checkbox*'Repository: expertiza/reimplementation-front-end
Length of output: 202
🏁 Script executed:
#!/bin/bash
# Search for FormCheckBoxGroup usage and definition
rg -l 'FormCheckBoxGroup' src/Repository: expertiza/reimplementation-front-end
Length of output: 191
🏁 Script executed:
#!/bin/bash
# Check FormCheckBoxGroup validation logic
cat src/components/Form/FormCheckBoxGroup.tsxRepository: expertiza/reimplementation-front-end
Length of output: 1883
🏁 Script executed:
#!/bin/bash
# Also check FormCheckBox for completeness
cat src/components/Form/FormCheckBox.tsxRepository: expertiza/reimplementation-front-end
Length of output: 1462
Add submitCount-based error visibility to form components.
With validateOnChange={false}, the submit button enables as soon as the form is dirty, even if fields are invalid. While Formik will block the submit, form controls currently only show validation errors when touched=true, not on form submission. This means clicking submit without touching individual fields produces no visible error feedback, creating a no-op UX.
The form uses FormInput, FormSelect, FormCheckBox, and FormCheckboxGroup. Of these, FormCheckboxGroup has no error display at all. Update all components to also show errors when form.submitCount > 0:
const isInvalid = (form.touched[field.name] || form.submitCount > 0) && !isValid;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Users/UserEditor.tsx` around lines 225 - 228, Update the form field
components (FormInput, FormSelect, FormCheckBox) to compute their invalid state
using submitCount as well as touched: replace existing isInvalid logic with
something like isInvalid = (form.touched[field.name] || form.submitCount > 0) &&
!isValid so validation errors show after a submit attempt; also add error
rendering to FormCheckboxGroup (it currently lacks any error display) using the
same isInvalid/ error message logic so checkbox groups display errors when
form.submitCount > 0 or the field is touched.
Comment on lines
+77
to
+100
| const parseFullName = (fullName: string) => { | ||
| const normalizedFullName = (fullName || "").trim(); | ||
| if (!normalizedFullName) { | ||
| return { firstName: "", lastName: "" }; | ||
| } | ||
|
|
||
| if (normalizedFullName.includes(",")) { | ||
| const [lastName = "", firstName = ""] = normalizedFullName.split(",", 2); | ||
| return { | ||
| firstName: firstName.trim(), | ||
| lastName: lastName.trim(), | ||
| }; | ||
| } | ||
|
|
||
| const parts = normalizedFullName.split(/\s+/); | ||
| if (parts.length === 1) { | ||
| return { firstName: parts[0], lastName: "" }; | ||
| } | ||
|
|
||
| return { | ||
| firstName: parts.slice(0, -1).join(" "), | ||
| lastName: parts[parts.length - 1], | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden \
--glob '!**/node_modules/**' \
--glob '!**/dist/**' \
--glob '!**/build/**' \
'\bfull_name\b|parseFullName|transformUserResponse' .Repository: expertiza/reimplementation-front-end
Length of output: 9181
Add documentation clarifying the supported full_name formats and acknowledge the edge case handling.
The parseFullName function correctly handles the primary "lastName, firstName" format used throughout the codebase (as seen in userUtil.ts:66 and participantUtil.ts:68). However, add a JSDoc comment explicitly documenting:
- The expected formats: comma-delimited ("lastName, firstName") and whitespace-delimited fallback
- The limitation that comma-delimited parsing truncates at the first comma—names containing commas will be truncated
- That whitespace-delimited parsing uses a heuristic (last token = lastName) which may not work for non-Western naming conventions
This defensive dual-format support is reasonable, but consumers should understand the assumptions and edge cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Users/userUtil.ts` around lines 77 - 100, Add a JSDoc block above
the parseFullName function describing the supported input formats and edge
cases: state that it accepts "lastName, firstName" (comma-delimited) and a
whitespace-delimited fallback, note that comma parsing stops at the first comma
(names containing commas will be truncated), explain the whitespace heuristic
(last token is treated as lastName, rest as firstName) and that this may
mis-handle non-Western naming conventions, and document its behavior for
empty/whitespace input (returns empty firstName/lastName). Also reference the
parseFullName symbol so reviewers and callers (e.g., participantUtil usage) know
which function the docs describe.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/pages/Assignments/ViewScores.tsx (1)
49-51:⚠️ Potential issue | 🟠 MajorQuoted multiline CSV rows are still parsed incorrectly.
At Line 50, splitting by newline before record parsing still corrupts valid CSV rows when
commentcontains embedded line breaks.Proposed fix
+const parseCsvRows = (csvText: string): string[] => { + const rows: string[] = []; + let current = ""; + let inQuotes = false; + + for (let i = 0; i < csvText.length; i += 1) { + const ch = csvText[i]; + const next = csvText[i + 1]; + + if (ch === '"' && inQuotes && next === '"') { + current += '""'; + i += 1; + continue; + } + + if (ch === '"') { + inQuotes = !inQuotes; + current += ch; + continue; + } + + if ((ch === "\n" || ch === "\r") && !inQuotes) { + if (ch === "\r" && next === "\n") i += 1; + if (current.trim().length > 0) rows.push(current); + current = ""; + continue; + } + + current += ch; + } + + if (current.trim().length > 0) rows.push(current); + return rows; +}; + const parseGradesCsv = (csvText: string): IGradeRow[] => { - const lines = csvText.split(/\r?\n/).filter((line) => line.trim().length > 0); + const lines = parseCsvRows(csvText); if (lines.length <= 1) return [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Assignments/ViewScores.tsx` around lines 49 - 51, The current parseGradesCsv function splits csvText by newline into lines (the lines variable) before parsing, which breaks quoted CSV fields that contain embedded newlines; instead, stop pre-splitting the input and parse the entire csvText as a whole using a CSV-aware parser or a record-splitting routine that respects quoted fields. Update parseGradesCsv to remove the csvText.split(...).filter(...) step and either feed csvText directly into a CSV parser (e.g., PapaParse or the project's CSV utility) or replace it with a small state-machine helper (e.g., parseRecordsAllowingQuotedNewlines) that iterates characters and only treats CR/LF as record separators when not inside quotes; keep using parseGradesCsv, header parsing, and IGradeRow mapping logic unchanged once records are correctly produced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/Assignments/ViewScores.tsx`:
- Around line 112-117: chartData mapping is converting empty grade strings to 0
via Number(row.grade), causing missing grades to show as real zeros; update the
useMemo for chartData (the chartData constant) to treat empty/null/undefined
grades as missing before mapping — e.g. filter out rows where row.grade is "" or
null/undefined (or parse to NaN) and only map rows with a finite numeric score
(keep the existing Number.isFinite check) so only real numeric grades become
plotted values.
---
Duplicate comments:
In `@src/pages/Assignments/ViewScores.tsx`:
- Around line 49-51: The current parseGradesCsv function splits csvText by
newline into lines (the lines variable) before parsing, which breaks quoted CSV
fields that contain embedded newlines; instead, stop pre-splitting the input and
parse the entire csvText as a whole using a CSV-aware parser or a
record-splitting routine that respects quoted fields. Update parseGradesCsv to
remove the csvText.split(...).filter(...) step and either feed csvText directly
into a CSV parser (e.g., PapaParse or the project's CSV utility) or replace it
with a small state-machine helper (e.g., parseRecordsAllowingQuotedNewlines)
that iterates characters and only treats CR/LF as record separators when not
inside quotes; keep using parseGradesCsv, header parsing, and IGradeRow mapping
logic unchanged once records are correctly produced.
🪄 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: c22bf12c-a6f6-4a14-a71c-7a170f9c8ad0
📒 Files selected for processing (3)
src/components/Modals/ExportModal.tsxsrc/components/Modals/ImportModal.tsxsrc/pages/Assignments/ViewScores.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Modals/ExportModal.tsx
- src/components/Modals/ImportModal.tsx
Comment on lines
+112
to
+117
| const chartData = useMemo( | ||
| () => | ||
| grades | ||
| .map((row) => ({ student: row.username, score: Number(row.grade) })) | ||
| .filter((row) => Number.isFinite(row.score)), | ||
| [grades] |
There was a problem hiding this comment.
Empty grades are being graphed as zero.
At Line 115, Number(row.grade) converts "" to 0, so missing grades appear as real zero scores in the graph.
Proposed fix
const chartData = useMemo(
() =>
grades
- .map((row) => ({ student: row.username, score: Number(row.grade) }))
+ .map((row) => {
+ const raw = row.grade.trim();
+ return {
+ student: row.username,
+ score: raw.length === 0 ? NaN : Number(raw),
+ };
+ })
.filter((row) => Number.isFinite(row.score)),
[grades]
);📝 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 chartData = useMemo( | |
| () => | |
| grades | |
| .map((row) => ({ student: row.username, score: Number(row.grade) })) | |
| .filter((row) => Number.isFinite(row.score)), | |
| [grades] | |
| const chartData = useMemo( | |
| () => | |
| grades | |
| .map((row) => { | |
| const raw = row.grade.trim(); | |
| return { | |
| student: row.username, | |
| score: raw.length === 0 ? NaN : Number(raw), | |
| }; | |
| }) | |
| .filter((row) => Number.isFinite(row.score)), | |
| [grades] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Assignments/ViewScores.tsx` around lines 112 - 117, chartData
mapping is converting empty grade strings to 0 via Number(row.grade), causing
missing grades to show as real zeros; update the useMemo for chartData (the
chartData constant) to treat empty/null/undefined grades as missing before
mapping — e.g. filter out rows where row.grade is "" or null/undefined (or parse
to NaN) and only map rows with a finite numeric score (keep the existing
Number.isFinite check) so only real numeric grades become plotted values.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Modals/ExportModal.tsx`:
- Around line 179-185: The modal's useEffect resets API state but leaves local
selection/order which can cause stale selectedFields/allFields to be posted;
update the opening flow in the useEffect (or inside resetExportState) to also
reset local export UI state (e.g., clear selectedFields, allFields, and any
fieldOrder) so they are empty when show becomes true, ensure this reset happens
before calling fetchConfig and before any export action can be triggered (or
disable the export button until config is loaded), and reference the existing
symbols useEffect, hasHandledExportResponse, resetExportState, fetchConfig,
selectedFields, and allFields when making the change.
In `@src/components/Modals/ImportModal.tsx`:
- Around line 193-195: The ImportModal currently calls fetchUsers unscoped
(fetchUsers({ url: "/users" })) which loads all users and exposes sensitive
fields in the preview; change ImportModal to extract the set of usernames from
the parsed import rows and call the scoped user lookup instead (e.g.,
fetchUsers({ url: `/users?names=${encodedCommaList}` }) or a batch lookup
endpoint such as `/users/batch` with the usernames payload), then map returned
users into the preview rendering logic so email/institution/parent/notification
fields are only shown for those matched usernames; update any error/empty
handling in the ImportModal preview code to handle missing users gracefully.
- Around line 147-161: fetchConfig is unstable because it depends directly on
the object identity of contextParams, causing refetch loops when parents pass
inline object literals; to fix, compute a stable serialized query string (e.g.,
via useMemo or equivalent) by iterating the same keys/values and filtering out
undefined/null/"" (the same logic currently in fetchConfig), expose that
serialized string (e.g., contextQuery) and then change fetchConfig to depend on
contextQuery instead of contextParams, build the URL using contextQuery and call
fetchImports({ url }) as before (keep using modelClass and fetchImports); ensure
the effect that depends on fetchConfig now sees a stable dependency.
🪄 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: f0f3025e-ccd8-425d-a4c5-4cdf1822f81f
📒 Files selected for processing (2)
src/components/Modals/ExportModal.tsxsrc/components/Modals/ImportModal.tsx
Comment on lines
179
to
+185
| useEffect(() => { | ||
| if (!show) return; | ||
|
|
||
| fetchConfig() | ||
| }, [show]); | ||
| hasHandledExportResponse.current = false; | ||
| resetExportState(true, true); | ||
| fetchConfig(); | ||
| }, [fetchConfig, resetExportState, show]); |
There was a problem hiding this comment.
Reset local field state when opening to prevent stale exports
At Line 179-185, API state is reset, but local selections/order are preserved across openings. If the user clicks export before fresh config loads, stale selectedFields/allFields can be posted.
Suggested fix
useEffect(() => {
if (!show) return;
hasHandledExportResponse.current = false;
resetExportState(true, true);
+ setMandatoryFields([]);
+ setOptionalFields([]);
+ setExternalFields([]);
+ setAllFields([]);
+ setSelectedFields([]);
+ setStatus('');
fetchConfig();
}, [fetchConfig, resetExportState, show]);📝 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
| useEffect(() => { | |
| if (!show) return; | |
| fetchConfig() | |
| }, [show]); | |
| hasHandledExportResponse.current = false; | |
| resetExportState(true, true); | |
| fetchConfig(); | |
| }, [fetchConfig, resetExportState, show]); | |
| useEffect(() => { | |
| if (!show) return; | |
| hasHandledExportResponse.current = false; | |
| resetExportState(true, true); | |
| setMandatoryFields([]); | |
| setOptionalFields([]); | |
| setExternalFields([]); | |
| setAllFields([]); | |
| setSelectedFields([]); | |
| setStatus(''); | |
| fetchConfig(); | |
| }, [fetchConfig, resetExportState, show]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Modals/ExportModal.tsx` around lines 179 - 185, The modal's
useEffect resets API state but leaves local selection/order which can cause
stale selectedFields/allFields to be posted; update the opening flow in the
useEffect (or inside resetExportState) to also reset local export UI state
(e.g., clear selectedFields, allFields, and any fieldOrder) so they are empty
when show becomes true, ensure this reset happens before calling fetchConfig and
before any export action can be triggered (or disable the export button until
config is loaded), and reference the existing symbols useEffect,
hasHandledExportResponse, resetExportState, fetchConfig, selectedFields, and
allFields when making the change.
Comment on lines
147
to
+161
| const fetchConfig = useCallback(async () => { | ||
| try { | ||
| await fetchImports({ url: `/import/${modelClass}` }); | ||
| const params = new URLSearchParams(); | ||
| Object.entries(contextParams || {}).forEach(([key, value]) => { | ||
| if (value !== undefined && value !== null && value !== "") { | ||
| params.append(key, String(value)); | ||
| } | ||
| }); | ||
|
|
||
| const url = params.toString() ? `/import/${modelClass}?${params.toString()}` : `/import/${modelClass}`; | ||
| await fetchImports({ url }); | ||
| } catch (err) { | ||
| console.error("Error fetching import config:", err); | ||
| } | ||
| }, [fetchImports, modelClass]); | ||
| }, [contextParams, fetchImports, modelClass]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify call sites passing inline contextParams objects and current effect deps.
rg -n --type=tsx -C2 'contextParams=\{\{'
rg -n --type=tsx -C3 'const fetchConfig = useCallback|}, \[show, fetchConfig, fetchUsers, modelClass\]\);' src/components/Modals/ImportModal.tsxRepository: expertiza/reimplementation-front-end
Length of output: 136
🏁 Script executed:
#!/bin/bash
# Search for contextParams usage as inline objects in tsx files
rg -n 'contextParams=\{\{' --glob='*.tsx' -C2
rg -n 'contextParams=' --glob='*.tsx' -C1 | head -50Repository: expertiza/reimplementation-front-end
Length of output: 2907
🏁 Script executed:
#!/bin/bash
# Read the ImportModal.tsx file to check dependency arrays
wc -l src/components/Modals/ImportModal.tsxRepository: expertiza/reimplementation-front-end
Length of output: 122
🏁 Script executed:
#!/bin/bash
# Look for fetchConfig useCallback and surrounding effect dependencies
rg -n 'const fetchConfig|useCallback|useEffect' src/components/Modals/ImportModal.tsx -B1 -A3Repository: expertiza/reimplementation-front-end
Length of output: 1613
🏁 Script executed:
#!/bin/bash
# Read ImportModal.tsx to see the full fetchConfig and surrounding effects
sed -n '147,210p' src/components/Modals/ImportModal.tsxRepository: expertiza/reimplementation-front-end
Length of output: 2193
🏁 Script executed:
#!/bin/bash
# Verify the full dependency array in the effect and check Participant.tsx for context
sed -n '187,198p' src/components/Modals/ImportModal.tsxRepository: expertiza/reimplementation-front-end
Length of output: 362
🏁 Script executed:
#!/bin/bash
# Check how participantContextParams is set in Participant.tsx (from earlier search result)
rg -n 'participantContextParams' src/pages/Participants/Participant.tsx -B5 -A2 | head -40Repository: expertiza/reimplementation-front-end
Length of output: 800
🏁 Script executed:
#!/bin/bash
# Confirm the issue by checking if assignmentId changes frequently in parent components
rg -n 'assignmentId' src/pages/Assignments/CreateTeams.tsx | head -10
rg -n 'const assignmentId' src/pages/Assignments/CreateTeams.tsx -A1Repository: expertiza/reimplementation-front-end
Length of output: 721
🏁 Script executed:
#!/bin/bash
# Check the fetchImports function to understand what it does with the URL
rg -n 'fetchImports' src/components/Modals/ImportModal.tsx -B2 -A2
rg -n 'const fetchImports|fetchImports:' src/components/Modals/ImportModal.tsx -A5Repository: expertiza/reimplementation-front-end
Length of output: 721
🏁 Script executed:
#!/bin/bash
# Check how fetchImports is passed as prop to understand impact
rg -n 'fetchImports' src/pages/Participants/Participant.tsx -B2 -A2 | head -20
rg -n 'fetchImports' src/pages/Assignments/CreateTeams.tsx -B2 -A2 | head -20Repository: expertiza/reimplementation-front-end
Length of output: 62
🏁 Script executed:
#!/bin/bash
# Verify the effect dependency chain and confirm the refetch loop risk
sed -n '138,165p' src/components/Modals/ImportModal.tsxRepository: expertiza/reimplementation-front-end
Length of output: 1213
Avoid refetch loops from unstable contextParams object identity.
The fetchConfig callback depends directly on contextParams (line 161), and the effect at lines 187–198 depends on fetchConfig. When components like CreateTeams.tsx (line 1753) or TopicsTab.tsx (lines 677, 683) pass inline object literals (e.g., contextParams={{ assignment_id: assignmentId }}), the contextParams reference changes on every parent re-render. This causes fetchConfig to recreate, which re-triggers the effect and refetches while the modal is open. Only Participant.tsx avoids this by memoizing participantContextParams.
Memoize the query string separately to stabilize the dependency:
Proposed fix
+ const contextQuery = useMemo(() => {
+ const params = new URLSearchParams();
+ Object.entries(contextParams || {})
+ .sort(([a], [b]) => a.localeCompare(b))
+ .forEach(([key, value]) => {
+ if (value !== undefined && value !== null && value !== "") {
+ params.append(key, String(value));
+ }
+ });
+ return params.toString();
+ }, [contextParams]);
+
const fetchConfig = useCallback(async () => {
try {
- const params = new URLSearchParams();
- Object.entries(contextParams || {}).forEach(([key, value]) => {
- if (value !== undefined && value !== null && value !== "") {
- params.append(key, String(value));
- }
- });
-
- const url = params.toString() ? `/import/${modelClass}?${params.toString()}` : `/import/${modelClass}`;
+ const url = contextQuery ? `/import/${modelClass}?${contextQuery}` : `/import/${modelClass}`;
await fetchImports({ url });
} catch (err) {
console.error("Error fetching import config:", err);
}
- }, [contextParams, fetchImports, modelClass]);
+ }, [contextQuery, fetchImports, modelClass]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Modals/ImportModal.tsx` around lines 147 - 161, fetchConfig is
unstable because it depends directly on the object identity of contextParams,
causing refetch loops when parents pass inline object literals; to fix, compute
a stable serialized query string (e.g., via useMemo or equivalent) by iterating
the same keys/values and filtering out undefined/null/"" (the same logic
currently in fetchConfig), expose that serialized string (e.g., contextQuery)
and then change fetchConfig to depend on contextQuery instead of contextParams,
build the URL using contextQuery and call fetchImports({ url }) as before (keep
using modelClass and fetchImports); ensure the effect that depends on
fetchConfig now sees a stable dependency.
Comment on lines
+193
to
+195
| if (modelClass === "AssignmentParticipant") { | ||
| fetchUsers({ url: "/users" }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current unscoped user fetch and the breadth of user fields consumed in preview.
rg -n --type=tsx -C3 'fetchUsers\(\{ url: "/users" \}\)' src/components/Modals/ImportModal.tsx
rg -n --type=tsx -C2 'full_name|email|parent|institution|email_on_review_of_review' src/components/Modals/ImportModal.tsx
# Check whether a scoped user-lookup endpoint pattern already exists in frontend calls.
rg -n --type=ts --type=tsx -C2 '"/users[^"]*"'
rg -n --type=ts --type=tsx -C2 'lookup|resolve|user_name|usernames'Repository: expertiza/reimplementation-front-end
Length of output: 192
🏁 Script executed:
#!/bin/bash
# Read the ImportModal.tsx file and check the specified line ranges
echo "=== Lines 193-195 (fetchUsers call) ==="
sed -n '193,195p' src/components/Modals/ImportModal.tsx
echo -e "\n=== Lines 391-402 (preview fields usage) ==="
sed -n '391,402p' src/components/Modals/ImportModal.tsx
echo -e "\n=== Broader context around line 194 ==="
sed -n '190,200p' src/components/Modals/ImportModal.tsx
echo -e "\n=== Searching for fetchUsers calls in ImportModal ==="
grep -n "fetchUsers" src/components/Modals/ImportModal.tsx
echo -e "\n=== Searching for user fields (email, institution, parent) ==="
grep -n -E "email|institution|parent|full_name" src/components/Modals/ImportModal.tsx | head -20Repository: expertiza/reimplementation-front-end
Length of output: 2213
🏁 Script executed:
#!/bin/bash
# Understand how preview is built and what data drives the user lookup
echo "=== Search for preview data transformation/processing ==="
grep -n "preview\|Preview" src/components/Modals/ImportModal.tsx | head -20
echo -e "\n=== Find where preview rows are constructed (likely around line 391) ==="
sed -n '370,410p' src/components/Modals/ImportModal.tsx
echo -e "\n=== Look for the import file processing and username extraction ==="
grep -n -E "processFile|parseFile|column|userName|user_name" src/components/Modals/ImportModal.tsx | head -25
echo -e "\n=== Check what columns are extracted from import ==="
sed -n '300,370p' src/components/Modals/ImportModal.tsxRepository: expertiza/reimplementation-front-end
Length of output: 5297
🏁 Script executed:
#!/bin/bash
# Check for alternative user lookup endpoints and patterns
echo "=== Search for other /users endpoint patterns ==="
grep -r -n "/users" src/ --include="*.ts" --include="*.tsx" | grep -v node_modules | head -20
echo -e "\n=== Look for username-based lookups or filters ==="
grep -r -n "username\|usernames\|lookup\|filter" src/ --include="*.ts" --include="*.tsx" | grep -i "user" | head -15
echo -e "\n=== Check if backend API docs or constants show other endpoints ==="
find src -name "*.ts" -o -name "*.tsx" | xargs grep -l "api\|endpoint\|API" | head -5Repository: expertiza/reimplementation-front-end
Length of output: 4808
Scope user lookups by import usernames instead of loading all users.
Line 194 fetches /users without scoping, then lines 391–402 expose rich profile fields (email, institution, parent, notification preferences) for every preview row. For assignment imports that need only a handful of users, this unnecessarily loads and displays sensitive data for the entire user list—a privacy and scalability risk.
The codebase already uses scoped user endpoints elsewhere (e.g., /users/${userId}/managed, /users/role/{role}), so a similar pattern—such as /users?names=... or a batch lookup by submitted usernames—would be straightforward to implement and more appropriate here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Modals/ImportModal.tsx` around lines 193 - 195, The
ImportModal currently calls fetchUsers unscoped (fetchUsers({ url: "/users" }))
which loads all users and exposes sensitive fields in the preview; change
ImportModal to extract the set of usernames from the parsed import rows and call
the scoped user lookup instead (e.g., fetchUsers({ url:
`/users?names=${encodedCommaList}` }) or a batch lookup endpoint such as
`/users/batch` with the usernames payload), then map returned users into the
preview rendering logic so email/institution/parent/notification fields are only
shown for those matched usernames; update any error/empty handling in the
ImportModal preview code to handle missing users gracefully.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/pages/Questionnaires/QuestionnairePackageImportModal.tsx (2)
235-246:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSeparate import success from post-import refresh errors.
await onImported()is in the same try/catch as the upload. If refresh fails, the UI reports the import itself as failed.Proposed fix
try { const response = await axiosClient.post("/questionnaire_packages/import", formData, { headers: { "Content-Type": "multipart/form-data" }, timeout: 120000 }); setStatus(response.data.message); setPreview(null); - await onImported(); + try { + await onImported(); + } catch { + setStatus(`${response.data.message} Imported successfully, but refreshing the list failed.`); + } setTimeout(() => onHide(), 800); } catch (error: any) { setStatus(error?.response?.data?.error || error?.message || "Failed to import questionnaire template package."); } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Questionnaires/QuestionnairePackageImportModal.tsx` around lines 235 - 246, The try/catch currently groups the upload (axiosClient.post) and the post-import refresh (await onImported()), causing refresh failures to be reported as upload failures; split them by keeping the upload and its success handling (setStatus, setPreview, setTimeout(() => onHide())) inside the first try and immediately after a successful upload call onImported() inside a separate try/catch so upload success is always reported via setStatus(response.data.message) while errors from onImported() are handled/logged separately (e.g., setStatus to a different message or console.error) without overwriting the upload success; reference axiosClient.post, setStatus, setPreview, onImported, and onHide when making the change.
109-118:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset duplicate-action state before loading config.
Old duplicate action options can persist if config fetch fails, leaving stale choices visible.
Proposed fix
setQuestionnaireFile(null); setItemsFile(null); setQuestionAdvicesFile(null); + setDuplicateActions([]); + setDuplicateAction(""); setTemplateNames([]); setCsvHeaderRequirements({}); setPreview(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Questionnaires/QuestionnairePackageImportModal.tsx` around lines 109 - 118, Reset the duplicate-action state along with the other imports before attempting to load the config to avoid showing stale choices; add calls to clear the duplicate-action state (e.g. call setDuplicateAction(null) and setDuplicateActionOptions([]) or whatever the component's duplicate-action setter names are) in the same block where setImportMode, setPackageFile, setQuestionnaireFile, setItemsFile, setQuestionAdvicesFile, setTemplateNames, setCsvHeaderRequirements, setPreview and setStatus are called so duplicate options are cleared prior to the config fetch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/pages/Questionnaires/QuestionnairePackageImportModal.tsx`:
- Around line 235-246: The try/catch currently groups the upload
(axiosClient.post) and the post-import refresh (await onImported()), causing
refresh failures to be reported as upload failures; split them by keeping the
upload and its success handling (setStatus, setPreview, setTimeout(() =>
onHide())) inside the first try and immediately after a successful upload call
onImported() inside a separate try/catch so upload success is always reported
via setStatus(response.data.message) while errors from onImported() are
handled/logged separately (e.g., setStatus to a different message or
console.error) without overwriting the upload success; reference
axiosClient.post, setStatus, setPreview, onImported, and onHide when making the
change.
- Around line 109-118: Reset the duplicate-action state along with the other
imports before attempting to load the config to avoid showing stale choices; add
calls to clear the duplicate-action state (e.g. call setDuplicateAction(null)
and setDuplicateActionOptions([]) or whatever the component's duplicate-action
setter names are) in the same block where setImportMode, setPackageFile,
setQuestionnaireFile, setItemsFile, setQuestionAdvicesFile, setTemplateNames,
setCsvHeaderRequirements, setPreview and setStatus are called so duplicate
options are cleared prior to the config fetch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 17c9ca60-1017-441a-a33b-9f94ad964eb2
📒 Files selected for processing (4)
src/pages/Questionnaires/Questionnaire.tsxsrc/pages/Questionnaires/QuestionnaireColumns.tsxsrc/pages/Questionnaires/QuestionnairePackageExportModal.tsxsrc/pages/Questionnaires/QuestionnairePackageImportModal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/Questionnaires/QuestionnaireColumns.tsx
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
2 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.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes