E2614 Role Based Reviewing#180
Open
veera2508 wants to merge 11 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements per-role teammate review rows, editable per-duty "Max members" with PATCH-backed updates, optimistic add/remove of assignment duties with rollback, fetches accessible duties in update mode, and adds participant duty update APIs and UI for changing member roles. (28 words) ChangesAssignment duties, rubric, and team role updates
Sequence DiagramsequenceDiagram
participant User
participant Editor as AssignmentEditor
participant TeamView as StudentTeamView
participant API as Backend
participant State as Local/Formik State
User->>Editor: Edit "Max members" for a duty
Editor->>State: Optimistically update `assignmentDuties.max_members_for_duty`
Editor->>API: PATCH /assignments/{id}/duties/{dutyId}/limit
alt Success
API-->>Editor: 200 OK
Editor->>User: Show success alert
else Failure
API-->>Editor: Error
Editor->>State: Revert `assignmentDuties`
Editor->>User: Show error alert
end
User->>Editor: Add or remove assigned duty
Editor->>State: Optimistically mutate `assignmentDuties`
Editor->>API: POST/DELETE /assignments/{id}/duties
alt Success
API-->>Editor: 200 OK
Editor->>State: Refresh duties and regenerate rubric rows
else Failure
API-->>Editor: Error
Editor->>State: Revert `assignmentDuties`
Editor->>User: Show error alert
end
User->>TeamView: Change member role via dropdown
TeamView->>API: PATCH /participants/{id}/duty
TeamView->>TeamView: Disable select while saving
alt Success
API-->>TeamView: 200 OK
TeamView->>User: Show success toast and refresh team
else Failure
API-->>TeamView: Error
TeamView->>User: Show error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Microsoft Presidio Analyzer (2.2.362)src/pages/Assignments/AssignmentEditor.tsxMicrosoft Presidio Analyzer failed to scan this file Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 5
🤖 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/AssignmentEditor.test.tsx`:
- Around line 98-112: The test "adds teammate review rows per role when
role-based review is enabled" races because AssignmentEditor copies
assignment_duties into local state inside a useEffect; change the assertions to
use async queries so the test waits for the effect to finish—update the it block
in AssignmentEditor.test.tsx to await screen.findByText("Teammate Review for
Frontend") and await screen.findByText("Teammate Review for Backend") (or wrap
the synchronous getByText checks in await waitFor(...)) so the role-based rows
are asserted after the component's effect has populated state.
- Around line 114-154: The tests in AssignmentEditor.test.tsx use outdated
labels and miss setting is_role_based when exercising role-based UI: update the
positive test case (the "shows role assignment controls with existing roles"
setup of loaderData and baseAssignment) to include is_role_based: true (in
addition to review_rubric_varies_by_role: true and assignment_duties) and then
update the assertions to match the current UI contract (replace old text/button
label expectations with the current accessible labels used by AssignmentEditor,
e.g., the current "assign" button text, the role name "Frontend", the actual
toggle/checkbox label, and the current add/remove/create button labels) so the
test validates the real rendered controls; do the inverse for the negative case
("hides role assignment controls when role-based review is disabled") by keeping
is_role_based false and asserting those controls are not present.
In `@src/pages/Assignments/AssignmentEditor.tsx`:
- Around line 170-172: The useEffect in AssignmentEditor reads
assignmentData.assignment_duties directly which can throw when loader data is
absent; update the effect to guard access by either depending on assignmentData
(e.g., useEffect(..., [assignmentData])) or using optional chaining in the
dependency array (assignmentData?.assignment_duties) and check before setting
state (if (assignmentData?.assignment_duties) setAssignmentDuties(...); else
setAssignmentDuties([])); update the effect that calls setAssignmentDuties so it
never dereferences assignmentData when it's undefined and uses
setAssignmentDuties with a safe fallback.
- Around line 290-307: The three useEffect blocks that call
fetchAccessibleDuties and react to
accessibleDutiesResponse/accessibleDutiesError are duplicates and cause
duplicate network calls and alerts; remove these duplicate effects (the
useEffect that checks mode === "update" and calls fetchAccessibleDuties, the
useEffect that setsAccessibleDuties from accessibleDutiesResponse, and the
useEffect that dispatches alertActions.showAlert on accessibleDutiesError) and
rely on the single canonical fetch + response mapping + error handling code
already implemented later; reference the hooks/idents involved (mode,
fetchAccessibleDuties, accessibleDutiesResponse, accessibleDutiesError,
setAccessibleDuties, dispatch, alertActions.showAlert) to locate and delete the
redundant blocks.
🪄 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: 31759c44-eb31-4ffe-97d6-0394181652b9
📒 Files selected for processing (2)
src/pages/Assignments/AssignmentEditor.test.tsxsrc/pages/Assignments/AssignmentEditor.tsx
Comment on lines
+98
to
+112
| it("adds teammate review rows per role when role-based review is enabled", () => { | ||
| loaderData = { | ||
| ...baseAssignment, | ||
| review_rubric_varies_by_role: true, | ||
| assignment_duties: [ | ||
| { duty_id: 1, duty_name: "Frontend", max_members_for_duty: 1 }, | ||
| { duty_id: 2, duty_name: "Backend", max_members_for_duty: 1 }, | ||
| ], | ||
| }; | ||
|
|
||
| render(<AssignmentEditor mode="update" />); | ||
|
|
||
| expect(screen.getByText("Teammate Review for Frontend")).toBeInTheDocument(); | ||
| expect(screen.getByText("Teammate Review for Backend")).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
Use async queries for effect-driven role rows.
assignment_duties is copied into local state in useEffect, so synchronous getByText can race. Prefer findByText/waitFor here.
Suggested fix
-render(<AssignmentEditor mode="update" />);
-
-expect(screen.getByText("Teammate Review for Frontend")).toBeInTheDocument();
-expect(screen.getByText("Teammate Review for Backend")).toBeInTheDocument();
+render(<AssignmentEditor mode="update" />);
+
+expect(await screen.findByText("Teammate Review for Frontend")).toBeInTheDocument();
+expect(await screen.findByText("Teammate Review for Backend")).toBeInTheDocument();📝 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
| it("adds teammate review rows per role when role-based review is enabled", () => { | |
| loaderData = { | |
| ...baseAssignment, | |
| review_rubric_varies_by_role: true, | |
| assignment_duties: [ | |
| { duty_id: 1, duty_name: "Frontend", max_members_for_duty: 1 }, | |
| { duty_id: 2, duty_name: "Backend", max_members_for_duty: 1 }, | |
| ], | |
| }; | |
| render(<AssignmentEditor mode="update" />); | |
| expect(screen.getByText("Teammate Review for Frontend")).toBeInTheDocument(); | |
| expect(screen.getByText("Teammate Review for Backend")).toBeInTheDocument(); | |
| }); | |
| it("adds teammate review rows per role when role-based review is enabled", async () => { | |
| loaderData = { | |
| ...baseAssignment, | |
| review_rubric_varies_by_role: true, | |
| assignment_duties: [ | |
| { duty_id: 1, duty_name: "Frontend", max_members_for_duty: 1 }, | |
| { duty_id: 2, duty_name: "Backend", max_members_for_duty: 1 }, | |
| ], | |
| }; | |
| render(<AssignmentEditor mode="update" />); | |
| expect(await screen.findByText("Teammate Review for Frontend")).toBeInTheDocument(); | |
| expect(await screen.findByText("Teammate Review for Backend")).toBeInTheDocument(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Assignments/AssignmentEditor.test.tsx` around lines 98 - 112, The
test "adds teammate review rows per role when role-based review is enabled"
races because AssignmentEditor copies assignment_duties into local state inside
a useEffect; change the assertions to use async queries so the test waits for
the effect to finish—update the it block in AssignmentEditor.test.tsx to await
screen.findByText("Teammate Review for Frontend") and await
screen.findByText("Teammate Review for Backend") (or wrap the synchronous
getByText checks in await waitFor(...)) so the role-based rows are asserted
after the component's effect has populated state.
Comment on lines
+114
to
+154
| it("shows role assignment controls with existing roles", () => { | ||
| loaderData = { | ||
| ...baseAssignment, | ||
| review_rubric_varies_by_role: true, | ||
| assignment_duties: [ | ||
| { duty_id: 1, duty_name: "Frontend", max_members_for_duty: 2 }, | ||
| ], | ||
| }; | ||
|
|
||
| render(<AssignmentEditor mode="update" />); | ||
|
|
||
| fireEvent.click(screen.getByRole("tab", { name: "Review strategy" })); | ||
|
|
||
| expect(screen.getByText("Assign existing role")).toBeInTheDocument(); | ||
| expect(screen.getByRole("button", { name: "Create a new role" })).toBeInTheDocument(); | ||
| expect(screen.getByText("Frontend")).toBeInTheDocument(); | ||
| expect(screen.getByLabelText("Is role based?")).toBeInTheDocument(); | ||
| expect(screen.getByRole("button", { name: "Add Role to Assignment" })).toBeInTheDocument(); | ||
| expect(screen.queryByRole("button", { name: "Create Role" })).not.toBeInTheDocument(); | ||
| expect(screen.getByRole("button", { name: "Remove" })).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("hides role assignment controls when role-based review is disabled", () => { | ||
| loaderData = { | ||
| ...baseAssignment, | ||
| review_rubric_varies_by_role: false, | ||
| assignment_duties: [ | ||
| { duty_id: 1, duty_name: "Frontend", max_members_for_duty: 2 }, | ||
| ], | ||
| }; | ||
|
|
||
| render(<AssignmentEditor mode="update" />); | ||
|
|
||
| fireEvent.click(screen.getByRole("tab", { name: "Review strategy" })); | ||
|
|
||
| expect(screen.queryByText("Assign existing role")).not.toBeInTheDocument(); | ||
| expect(screen.queryByRole("button", { name: "Add Role to Assignment" })).not.toBeInTheDocument(); | ||
| expect(screen.queryByRole("button", { name: "Create a new role" })).not.toBeInTheDocument(); | ||
| expect(screen.queryByRole("button", { name: "Create Role" })).not.toBeInTheDocument(); | ||
| expect(screen.queryByRole("button", { name: "Remove" })).not.toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
Role-controls tests are misaligned with current UI contract.
These assertions target old text/button labels and omit is_role_based: true in the positive case, so they either fail incorrectly or pass vacuously without validating the feature.
Suggested test update
loaderData = {
...baseAssignment,
+ is_role_based: true,
review_rubric_varies_by_role: true,
assignment_duties: [
{ duty_id: 1, duty_name: "Frontend", max_members_for_duty: 2 },
],
};
fireEvent.click(screen.getByRole("tab", { name: "Review strategy" }));
-expect(screen.getByText("Assign existing role")).toBeInTheDocument();
-expect(screen.getByRole("button", { name: "Create a new role" })).toBeInTheDocument();
+expect(screen.getByText("Select roles(duties):")).toBeInTheDocument();
expect(screen.getByText("Frontend")).toBeInTheDocument();
expect(screen.getByLabelText("Is role based?")).toBeInTheDocument();
-expect(screen.getByRole("button", { name: "Add Role to Assignment" })).toBeInTheDocument();
-expect(screen.queryByRole("button", { name: "Create Role" })).not.toBeInTheDocument();
-expect(screen.getByRole("button", { name: "Remove" })).toBeInTheDocument();
+expect(screen.getByRole("button", { name: "Add" })).toBeInTheDocument();
+expect(screen.getByRole("button", { name: "Delete Duty" })).toBeInTheDocument();📝 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
| it("shows role assignment controls with existing roles", () => { | |
| loaderData = { | |
| ...baseAssignment, | |
| review_rubric_varies_by_role: true, | |
| assignment_duties: [ | |
| { duty_id: 1, duty_name: "Frontend", max_members_for_duty: 2 }, | |
| ], | |
| }; | |
| render(<AssignmentEditor mode="update" />); | |
| fireEvent.click(screen.getByRole("tab", { name: "Review strategy" })); | |
| expect(screen.getByText("Assign existing role")).toBeInTheDocument(); | |
| expect(screen.getByRole("button", { name: "Create a new role" })).toBeInTheDocument(); | |
| expect(screen.getByText("Frontend")).toBeInTheDocument(); | |
| expect(screen.getByLabelText("Is role based?")).toBeInTheDocument(); | |
| expect(screen.getByRole("button", { name: "Add Role to Assignment" })).toBeInTheDocument(); | |
| expect(screen.queryByRole("button", { name: "Create Role" })).not.toBeInTheDocument(); | |
| expect(screen.getByRole("button", { name: "Remove" })).toBeInTheDocument(); | |
| }); | |
| it("hides role assignment controls when role-based review is disabled", () => { | |
| loaderData = { | |
| ...baseAssignment, | |
| review_rubric_varies_by_role: false, | |
| assignment_duties: [ | |
| { duty_id: 1, duty_name: "Frontend", max_members_for_duty: 2 }, | |
| ], | |
| }; | |
| render(<AssignmentEditor mode="update" />); | |
| fireEvent.click(screen.getByRole("tab", { name: "Review strategy" })); | |
| expect(screen.queryByText("Assign existing role")).not.toBeInTheDocument(); | |
| expect(screen.queryByRole("button", { name: "Add Role to Assignment" })).not.toBeInTheDocument(); | |
| expect(screen.queryByRole("button", { name: "Create a new role" })).not.toBeInTheDocument(); | |
| expect(screen.queryByRole("button", { name: "Create Role" })).not.toBeInTheDocument(); | |
| expect(screen.queryByRole("button", { name: "Remove" })).not.toBeInTheDocument(); | |
| }); | |
| it("shows role assignment controls with existing roles", () => { | |
| loaderData = { | |
| ...baseAssignment, | |
| is_role_based: true, | |
| review_rubric_varies_by_role: true, | |
| assignment_duties: [ | |
| { duty_id: 1, duty_name: "Frontend", max_members_for_duty: 2 }, | |
| ], | |
| }; | |
| render(<AssignmentEditor mode="update" />); | |
| fireEvent.click(screen.getByRole("tab", { name: "Review strategy" })); | |
| expect(screen.getByText("Select roles(duties):")).toBeInTheDocument(); | |
| expect(screen.getByText("Frontend")).toBeInTheDocument(); | |
| expect(screen.getByLabelText("Is role based?")).toBeInTheDocument(); | |
| expect(screen.getByRole("button", { name: "Add" })).toBeInTheDocument(); | |
| expect(screen.getByRole("button", { name: "Delete Duty" })).toBeInTheDocument(); | |
| }); | |
| it("hides role assignment controls when role-based review is disabled", () => { | |
| loaderData = { | |
| ...baseAssignment, | |
| review_rubric_varies_by_role: false, | |
| assignment_duties: [ | |
| { duty_id: 1, duty_name: "Frontend", max_members_for_duty: 2 }, | |
| ], | |
| }; | |
| render(<AssignmentEditor mode="update" />); | |
| fireEvent.click(screen.getByRole("tab", { name: "Review strategy" })); | |
| expect(screen.queryByText("Assign existing role")).not.toBeInTheDocument(); | |
| expect(screen.queryByRole("button", { name: "Add Role to Assignment" })).not.toBeInTheDocument(); | |
| expect(screen.queryByRole("button", { name: "Create a new role" })).not.toBeInTheDocument(); | |
| expect(screen.queryByRole("button", { name: "Create Role" })).not.toBeInTheDocument(); | |
| expect(screen.queryByRole("button", { name: "Remove" })).not.toBeInTheDocument(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Assignments/AssignmentEditor.test.tsx` around lines 114 - 154, The
tests in AssignmentEditor.test.tsx use outdated labels and miss setting
is_role_based when exercising role-based UI: update the positive test case (the
"shows role assignment controls with existing roles" setup of loaderData and
baseAssignment) to include is_role_based: true (in addition to
review_rubric_varies_by_role: true and assignment_duties) and then update the
assertions to match the current UI contract (replace old text/button label
expectations with the current accessible labels used by AssignmentEditor, e.g.,
the current "assign" button text, the role name "Frontend", the actual
toggle/checkbox label, and the current add/remove/create button labels) so the
test validates the real rendered controls; do the inverse for the negative case
("hides role assignment controls when role-based review is disabled") by keeping
is_role_based false and asserting those controls are not present.
Comment on lines
+170
to
+172
| useEffect(() => { | ||
| setAssignmentDuties(assignmentData.assignment_duties || []); | ||
| }, [assignmentData.assignment_duties]); |
There was a problem hiding this comment.
Guard loader data access before reading assignment_duties.
This effect can throw when loader data is absent (e.g., create flow), because assignmentData.assignment_duties is read eagerly in the dependency list and body.
Suggested fix
-useEffect(() => {
- setAssignmentDuties(assignmentData.assignment_duties || []);
-}, [assignmentData.assignment_duties]);
+useEffect(() => {
+ setAssignmentDuties(assignmentData?.assignment_duties || []);
+}, [assignmentData?.assignment_duties]);🤖 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 170 - 172, The
useEffect in AssignmentEditor reads assignmentData.assignment_duties directly
which can throw when loader data is absent; update the effect to guard access by
either depending on assignmentData (e.g., useEffect(..., [assignmentData])) or
using optional chaining in the dependency array
(assignmentData?.assignment_duties) and check before setting state (if
(assignmentData?.assignment_duties) setAssignmentDuties(...); else
setAssignmentDuties([])); update the effect that calls setAssignmentDuties so it
never dereferences assignmentData when it's undefined and uses
setAssignmentDuties with a safe fallback.
Comment on lines
+290
to
+307
| useEffect(() => { | ||
| if (mode === "update") { | ||
| fetchAccessibleDuties({ url: "/duties/accessible_duties", method: HttpMethod.GET }); | ||
| } | ||
| }, [mode, fetchAccessibleDuties]); | ||
|
|
||
| useEffect(() => { | ||
| if (accessibleDutiesResponse?.data) { | ||
| setAccessibleDuties(accessibleDutiesResponse.data || []); | ||
| } | ||
| }, [accessibleDutiesResponse]); | ||
|
|
||
| useEffect(() => { | ||
| if (accessibleDutiesError) { | ||
| dispatch(alertActions.showAlert({ variant: "danger", message: accessibleDutiesError })); | ||
| } | ||
| }, [accessibleDutiesError, dispatch]); | ||
|
|
There was a problem hiding this comment.
Remove duplicated accessible-duties effects to avoid duplicate requests/alerts.
These new effects duplicate behavior already handled later (fetch + response mapping + error alert), which can cause redundant network calls and repeated error toasts.
Suggested fix
-useEffect(() => {
- if (mode === "update") {
- fetchAccessibleDuties({ url: "/duties/accessible_duties", method: HttpMethod.GET });
- }
-}, [mode, fetchAccessibleDuties]);
-
-useEffect(() => {
- if (accessibleDutiesResponse?.data) {
- setAccessibleDuties(accessibleDutiesResponse.data || []);
- }
-}, [accessibleDutiesResponse]);
-
-useEffect(() => {
- if (accessibleDutiesError) {
- dispatch(alertActions.showAlert({ variant: "danger", message: accessibleDutiesError }));
- }
-}, [accessibleDutiesError, dispatch]);
+// Keep a single fetch+response+error flow (the existing refreshAccessibleDuties path below).🤖 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 290 - 307, The three
useEffect blocks that call fetchAccessibleDuties and react to
accessibleDutiesResponse/accessibleDutiesError are duplicates and cause
duplicate network calls and alerts; remove these duplicate effects (the
useEffect that checks mode === "update" and calls fetchAccessibleDuties, the
useEffect that setsAccessibleDuties from accessibleDutiesResponse, and the
useEffect that dispatches alertActions.showAlert on accessibleDutiesError) and
rely on the single canonical fetch + response mapping + error handling code
already implemented later; reference the hooks/idents involved (mode,
fetchAccessibleDuties, accessibleDutiesResponse, accessibleDutiesError,
setAccessibleDuties, dispatch, alertActions.showAlert) to locate and delete the
redundant blocks.
Comment on lines
+1114
to
+1121
| <input | ||
| type="number" | ||
| min={1} | ||
| defaultValue={duty.max_members_for_duty ?? 1} | ||
| style={{ width: '100px' }} | ||
| onBlur={(e) => handleUpdateRoleLimit(duty.duty_id ?? duty.id, Number(e.target.value))} | ||
| disabled={!id} | ||
| /> |
There was a problem hiding this comment.
max_members_for_duty input should be controlled by state.
Using defaultValue here makes the field drift from assignmentDuties after async updates/reverts. This is especially visible when PATCH fails or backend normalizes the value.
Suggested fix
-<input
- type="number"
- min={1}
- defaultValue={duty.max_members_for_duty ?? 1}
- style={{ width: '100px' }}
- onBlur={(e) => handleUpdateRoleLimit(duty.duty_id ?? duty.id, Number(e.target.value))}
- disabled={!id}
-/>
+<input
+ type="number"
+ min={1}
+ value={duty.max_members_for_duty ?? 1}
+ style={{ width: '100px' }}
+ onChange={(e) => {
+ const next = Math.max(1, Number(e.target.value) || 1);
+ const dutyId = duty.duty_id ?? duty.id;
+ setAssignmentDuties((prev) =>
+ prev.map((d: any) =>
+ (d.duty_id ?? d.id) === dutyId ? { ...d, max_members_for_duty: next } : d
+ )
+ );
+ }}
+ onBlur={(e) => handleUpdateRoleLimit(duty.duty_id ?? duty.id, Number(e.target.value))}
+ disabled={!id}
+/>📝 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
| <input | |
| type="number" | |
| min={1} | |
| defaultValue={duty.max_members_for_duty ?? 1} | |
| style={{ width: '100px' }} | |
| onBlur={(e) => handleUpdateRoleLimit(duty.duty_id ?? duty.id, Number(e.target.value))} | |
| disabled={!id} | |
| /> | |
| <input | |
| type="number" | |
| min={1} | |
| value={duty.max_members_for_duty ?? 1} | |
| style={{ width: '100px' }} | |
| onChange={(e) => { | |
| const next = Math.max(1, Number(e.target.value) || 1); | |
| const dutyId = duty.duty_id ?? duty.id; | |
| setAssignmentDuties((prev) => | |
| prev.map((d: any) => | |
| (d.duty_id ?? d.id) === dutyId ? { ...d, max_members_for_duty: next } : d | |
| ) | |
| ); | |
| }} | |
| onBlur={(e) => handleUpdateRoleLimit(duty.duty_id ?? duty.id, Number(e.target.value))} | |
| disabled={!id} | |
| /> |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/AssignmentEditor.tsx`:
- Around line 854-855: The new FormCheckbox "review_rubric_varies_by_role"
(controlId/name review_rubric_varies_by_role) is never read when generating
per-role teammate review rows; update the code that builds those rows (the block
that iterates roles near the per-role review rows creation, also the similar
block at the 928-940 area) to first read the form value (e.g., via your form
state/getValues or values.review_rubric_varies_by_role) and only generate
per-role rows when that boolean is true; if false, generate the single shared
review/rubric rows instead. Ensure you reference the same form key
review_rubric_varies_by_role and apply the conditional to both places that
create role-specific review rows.
- Around line 430-444: The current try/catch using Promise.all over
selectedDutyIds will revert UI state on any single POST failure; instead use
Promise.allSettled to call addAssignmentDuty for each selectedDutyId, then
process results: collect fulfilled responses and merge only those successful
duties into the current assignment state (do not overwrite with
previousAssignmentDuties), call refreshAssignmentDuties or update
setAssignmentDuties/setSelectedDutyIds to reflect the successful writes, and
surface or log failed dutyIds so the UI can show errors; reference
addAssignmentDuty, selectedDutyIds, refreshAssignmentDuties,
setAssignmentDuties, setSelectedDutyIds, previousAssignmentDuties, and
previousSelectedDutyIds when making these changes.
🪄 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: 8ebd0125-02e8-4e64-b647-5cea5cff8b72
📒 Files selected for processing (1)
src/pages/Assignments/AssignmentEditor.tsx
Comment on lines
+430
to
+444
| try { | ||
| await Promise.all( | ||
| selectedDutyIds.map((dutyId) => | ||
| addAssignmentDuty({ | ||
| url: `/assignments/${id}/duties`, | ||
| method: "POST", | ||
| data: { duty_id: dutyId }, | ||
| }) | ||
| ) | ||
| ); | ||
| refreshAssignmentDuties(); | ||
| } catch (error) { | ||
| setAssignmentDuties(previousAssignmentDuties); | ||
| setSelectedDutyIds(previousSelectedDutyIds); | ||
| } |
There was a problem hiding this comment.
Handle partial duty-add failures without reverting successful writes.
Promise.all rejects on the first failed POST, but earlier requests may already have succeeded. Reverting to previousAssignmentDuties/previousSelectedDutyIds in catch can leave the UI out of sync with persisted duties until the next manual refresh.
Suggested fix
- try {
- await Promise.all(
- selectedDutyIds.map((dutyId) =>
- addAssignmentDuty({
- url: `/assignments/${id}/duties`,
- method: "POST",
- data: { duty_id: dutyId },
- })
- )
- );
- refreshAssignmentDuties();
- } catch (error) {
- setAssignmentDuties(previousAssignmentDuties);
- setSelectedDutyIds(previousSelectedDutyIds);
- }
+ const results = await Promise.allSettled(
+ selectedDutyIds.map((dutyId) =>
+ addAssignmentDuty({
+ url: `/assignments/${id}/duties`,
+ method: "POST",
+ data: { duty_id: dutyId },
+ })
+ )
+ );
+
+ const failedDutyIds = results.flatMap((result, index) =>
+ result.status === "rejected" ? [selectedDutyIds[index]] : []
+ );
+
+ setSelectedDutyIds(failedDutyIds);
+ refreshAssignmentDuties();🤖 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 430 - 444, The
current try/catch using Promise.all over selectedDutyIds will revert UI state on
any single POST failure; instead use Promise.allSettled to call
addAssignmentDuty for each selectedDutyId, then process results: collect
fulfilled responses and merge only those successful duties into the current
assignment state (do not overwrite with previousAssignmentDuties), call
refreshAssignmentDuties or update setAssignmentDuties/setSelectedDutyIds to
reflect the successful writes, and surface or log failed dutyIds so the UI can
show errors; reference addAssignmentDuty, selectedDutyIds,
refreshAssignmentDuties, setAssignmentDuties, setSelectedDutyIds,
previousAssignmentDuties, and previousSelectedDutyIds when making these changes.
Add role column and student based selector boxes
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Student` Teams/StudentTeamView.tsx:
- Around line 168-179: The handleRoleChange function is bypassing the existing
update flow by calling fetchTeam() and setting a one-off banner/toast; instead
let the updateDuty call drive the downstream effects already wired to
updateDutyResponse. Remove the manual fetchTeam() and the
setShowAlert/setToastMessage calls in handleRoleChange (keep
setRoleSavingParticipantId and the try/finally around updateDuty), so
updateDuty(participantId, selectedDutyId) alone updates updateDutyResponse and
triggers the existing refetch/banner logic defined elsewhere.
🪄 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: 559d026f-02a9-4c49-aada-100b436cffbc
📒 Files selected for processing (2)
src/hooks/useStudentTeam.tssrc/pages/Student Teams/StudentTeamView.tsx
Comment on lines
+168
to
+179
| const handleRoleChange = async (participantId: number, value: string) => { | ||
| const selectedDutyId = value ? Number(value) : null; | ||
| setRoleSavingParticipantId(participantId); | ||
| try { | ||
| await updateDuty(participantId, selectedDutyId); | ||
| fetchTeam(); | ||
| setShowAlert(false); | ||
| setToastMessage("Role updated successfully."); | ||
| } finally { | ||
| setRoleSavingParticipantId(null); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Route role updates through the existing reset flow.
This handler sets a one-off success banner and refetches immediately, but updateDutyResponse is already used below as the refetch trigger. As written, the banner never auto-clears and the stale response can keep retriggering refetches on later actions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Student` Teams/StudentTeamView.tsx around lines 168 - 179, The
handleRoleChange function is bypassing the existing update flow by calling
fetchTeam() and setting a one-off banner/toast; instead let the updateDuty call
drive the downstream effects already wired to updateDutyResponse. Remove the
manual fetchTeam() and the setShowAlert/setToastMessage calls in
handleRoleChange (keep setRoleSavingParticipantId and the try/finally around
updateDuty), so updateDuty(participantId, selectedDutyId) alone updates
updateDutyResponse and triggers the existing refetch/banner logic defined
elsewhere.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/Assignments/AssignmentEditor.tsx (1)
915-938:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRender either the shared teammate-review rows or the per-role rows, not both.
When
review_rubric_varies_by_roleis enabled, this still keeps the generic"Teammate review:"row and then appends role-specific rows. That duplicates questionnaire/weight inputs for the same section, and it can also keep role rows visible afteris_role_basedis turned off on an existing assignment.Suggested fix
-{ - id: (formik.values.number_of_review_rounds ?? 0) + 1, - title: "Teammate review:", - questionnaire_options: [{ label: 'Review with Github metrics', value: 'Review with Github metrics' }], - questionnaire_type: 'dropdown', - weight_index: 101, -}, -{ - id: (formik.values.number_of_review_rounds ?? 0) + 1, - title: "Add tag prompts", - questionnaire_type: 'tag_prompts', -}, -...(formik.values.review_rubric_varies_by_role && assignedRoleDuties.length > 0 - ? assignedRoleDuties.flatMap((roleName: string, index: number) => { - const roleRowId = 10_000 + index; - return [{ - id: roleRowId, - title: `Teammate Review for ${roleName}`, - questionnaire_options: [{ label: 'Review with Github metrics', value: 'Review with Github metrics' }], - questionnaire_type: 'dropdown', - weight_index: 20_000 + index, - }]; - }) - : []), +...( + formik.values.is_role_based && + formik.values.review_rubric_varies_by_role && + assignedRoleDuties.length > 0 + ? assignedRoleDuties.flatMap((roleName: string, index: number) => { + const roleRowId = 10_000 + index; + return [{ + id: roleRowId, + title: `Teammate Review for ${roleName}`, + questionnaire_options: [{ label: 'Review with Github metrics', value: 'Review with Github metrics' }], + questionnaire_type: 'dropdown', + weight_index: 20_000 + index, + }]; + }) + : [ + { + id: (formik.values.number_of_review_rounds ?? 0) + 1, + title: "Teammate review:", + questionnaire_options: [{ label: 'Review with Github metrics', value: 'Review with Github metrics' }], + questionnaire_type: 'dropdown', + weight_index: 101, + }, + { + id: (formik.values.number_of_review_rounds ?? 0) + 1, + title: "Add tag prompts", + questionnaire_type: 'tag_prompts', + }, + ] +),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/Assignments/AssignmentEditor.tsx` around lines 915 - 938, The current mapper always includes the generic "Teammate review:" row and then appends per-role rows when formik.values.review_rubric_varies_by_role is true, causing duplicate inputs and stale role rows; change the array construction so the generic teammate row (id/title "Teammate review:") is only added when formik.values.review_rubric_varies_by_role is false, and when true only produce the per-role rows using assignedRoleDuties (use the existing roleRowId/weight_index scheme 10000+/20000+), and ensure toggling review_rubric_varies_by_role to false stops/filters out those per-role rows (i.e., build the final rows array conditionally rather than always appending both).
♻️ Duplicate comments (4)
src/pages/Assignments/AssignmentEditor.tsx (4)
430-444:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t roll back successful duty adds on the first failed POST.
Promise.allrejects on the first failure, but earlier requests may already have persisted. Resetting topreviousAssignmentDutiesandpreviousSelectedDutyIdscan make the UI stale and re-offer duties that were actually added.Suggested fix
-try { - await Promise.all( - selectedDutyIds.map((dutyId) => - addAssignmentDuty({ - url: `/assignments/${id}/duties`, - method: "POST", - data: { duty_id: dutyId }, - }) - ) - ); - refreshAssignmentDuties(); -} catch (error) { - setAssignmentDuties(previousAssignmentDuties); - setSelectedDutyIds(previousSelectedDutyIds); -} +const results = await Promise.allSettled( + selectedDutyIds.map((dutyId) => + addAssignmentDuty({ + url: `/assignments/${id}/duties`, + method: "POST", + data: { duty_id: dutyId }, + }) + ) +); + +const failedDutyIds = results.flatMap((result, index) => + result.status === "rejected" ? [selectedDutyIds[index]] : [] +); + +setSelectedDutyIds(failedDutyIds); +refreshAssignmentDuties();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/Assignments/AssignmentEditor.tsx` around lines 430 - 444, The current try/catch around Promise.all with addAssignmentDuty causes a full rollback if any POST fails; change to use Promise.allSettled for selectedDutyIds.map(...) so you can inspect each result, keep the successfully added duties in state (or call refreshAssignmentDuties for the successful ones) and only revert UI state or re-enable selection for the dutyIds that actually failed; reference addAssignmentDuty, selectedDutyIds, previousAssignmentDuties, previousSelectedDutyIds, and refreshAssignmentDuties to locate where to switch to Promise.allSettled, filter results by status === "fulfilled"/"rejected", and update state accordingly instead of resetting everything on the first error.
1112-1119:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the max-members field controlled so async reconciliation is visible.
Using
defaultValuehere means the input stops followingassignmentDutiesafter first render. If the PATCH fails or the backend normalizes the value, the table state changes but the field keeps showing the stale typed value.Suggested fix
<input type="number" min={1} - defaultValue={duty.max_members_for_duty ?? 1} + value={duty.max_members_for_duty ?? 1} style={{ width: '100px' }} + onChange={(e) => { + const next = Math.max(1, Number(e.target.value) || 1); + const dutyId = duty.duty_id ?? duty.id; + setAssignmentDuties((prev) => + prev.map((d: any) => + (d.duty_id ?? d.id) === dutyId + ? { ...d, max_members_for_duty: next } + : d + ) + ); + }} onBlur={(e) => handleUpdateRoleLimit(duty.duty_id ?? duty.id, Number(e.target.value))} disabled={!id} />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/Assignments/AssignmentEditor.tsx` around lines 1112 - 1119, The number input uses defaultValue so it becomes uncontrolled and won't reflect async updates from assignmentDuties; change it to a controlled input by binding value to duty.max_members_for_duty (or a derived value from assignmentDuties) and call handleUpdateRoleLimit on blur (or onChange to update local state) so the UI reflects backend reconciliation; update the input to use value={duty.max_members_for_duty ?? 1} and ensure handleUpdateRoleLimit(duty.duty_id ?? duty.id, Number(e.target.value)) is invoked appropriately and that any local state update follows the PATCH response to keep the field in sync.
290-306:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the duplicated accessible-duty effects.
These three effects duplicate the later
refreshAccessibleDuties+ response/error handling path. In update mode this issues the same GET twice and can show the same error alert twice.Suggested fix
-useEffect(() => { - if (mode === "update") { - fetchAccessibleDuties({ url: "/duties/accessible_duties", method: HttpMethod.GET }); - } -}, [mode, fetchAccessibleDuties]); - -useEffect(() => { - if (accessibleDutiesResponse?.data) { - setAccessibleDuties(accessibleDutiesResponse.data || []); - } -}, [accessibleDutiesResponse]); - -useEffect(() => { - if (accessibleDutiesError) { - dispatch(alertActions.showAlert({ variant: "danger", message: accessibleDutiesError })); - } -}, [accessibleDutiesError, dispatch]); +// Keep the single refreshAccessibleDuties/accessibleDutiesResponse flow below.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/Assignments/AssignmentEditor.tsx` around lines 290 - 306, The three useEffect blocks that call fetchAccessibleDuties and handle accessibleDutiesResponse/accessibleDutiesError are duplicating the later refreshAccessibleDuties logic and causing double requests/alerts; remove these three useEffect blocks (the ones referencing fetchAccessibleDuties, accessibleDutiesResponse, and accessibleDutiesError) and instead trigger refreshAccessibleDuties when mode === "update" so only the single refreshAccessibleDuties path updates state and dispatches alerts (keep the existing handlers that setAccessibleDuties and dispatch alertActions.showAlert tied to the refreshAccessibleDuties response/error).
170-172:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard
assignmentDatabefore readingassignment_duties.This still dereferences
assignmentDatain both the effect body and the dependency array. In create flow or any route without loader data, render will throw before the fallback can run.Suggested fix
useEffect(() => { - setAssignmentDuties(assignmentData.assignment_duties || []); -}, [assignmentData.assignment_duties]); + setAssignmentDuties(assignmentData?.assignment_duties || []); +}, [assignmentData?.assignment_duties]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/Assignments/AssignmentEditor.tsx` around lines 170 - 172, The effect currently dereferences assignmentData in both the body and dependency array; guard against undefined by referencing assignmentData safely and checking before using it. Update the useEffect that calls setAssignmentDuties so it reads assignmentData?.assignment_duties (or checks if assignmentData exists) in the body and uses a safe dependency like assignmentData?.assignment_duties or assignmentData in the deps, and only call setAssignmentDuties when assignmentData is defined; target the useEffect surrounding setAssignmentDuties and the assignmentData/assignment_duties references.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/pages/Assignments/AssignmentEditor.tsx`:
- Around line 915-938: The current mapper always includes the generic "Teammate
review:" row and then appends per-role rows when
formik.values.review_rubric_varies_by_role is true, causing duplicate inputs and
stale role rows; change the array construction so the generic teammate row
(id/title "Teammate review:") is only added when
formik.values.review_rubric_varies_by_role is false, and when true only produce
the per-role rows using assignedRoleDuties (use the existing
roleRowId/weight_index scheme 10000+/20000+), and ensure toggling
review_rubric_varies_by_role to false stops/filters out those per-role rows
(i.e., build the final rows array conditionally rather than always appending
both).
---
Duplicate comments:
In `@src/pages/Assignments/AssignmentEditor.tsx`:
- Around line 430-444: The current try/catch around Promise.all with
addAssignmentDuty causes a full rollback if any POST fails; change to use
Promise.allSettled for selectedDutyIds.map(...) so you can inspect each result,
keep the successfully added duties in state (or call refreshAssignmentDuties for
the successful ones) and only revert UI state or re-enable selection for the
dutyIds that actually failed; reference addAssignmentDuty, selectedDutyIds,
previousAssignmentDuties, previousSelectedDutyIds, and refreshAssignmentDuties
to locate where to switch to Promise.allSettled, filter results by status ===
"fulfilled"/"rejected", and update state accordingly instead of resetting
everything on the first error.
- Around line 1112-1119: The number input uses defaultValue so it becomes
uncontrolled and won't reflect async updates from assignmentDuties; change it to
a controlled input by binding value to duty.max_members_for_duty (or a derived
value from assignmentDuties) and call handleUpdateRoleLimit on blur (or onChange
to update local state) so the UI reflects backend reconciliation; update the
input to use value={duty.max_members_for_duty ?? 1} and ensure
handleUpdateRoleLimit(duty.duty_id ?? duty.id, Number(e.target.value)) is
invoked appropriately and that any local state update follows the PATCH response
to keep the field in sync.
- Around line 290-306: The three useEffect blocks that call
fetchAccessibleDuties and handle accessibleDutiesResponse/accessibleDutiesError
are duplicating the later refreshAccessibleDuties logic and causing double
requests/alerts; remove these three useEffect blocks (the ones referencing
fetchAccessibleDuties, accessibleDutiesResponse, and accessibleDutiesError) and
instead trigger refreshAccessibleDuties when mode === "update" so only the
single refreshAccessibleDuties path updates state and dispatches alerts (keep
the existing handlers that setAccessibleDuties and dispatch
alertActions.showAlert tied to the refreshAccessibleDuties response/error).
- Around line 170-172: The effect currently dereferences assignmentData in both
the body and dependency array; guard against undefined by referencing
assignmentData safely and checking before using it. Update the useEffect that
calls setAssignmentDuties so it reads assignmentData?.assignment_duties (or
checks if assignmentData exists) in the body and uses a safe dependency like
assignmentData?.assignment_duties or assignmentData in the deps, and only call
setAssignmentDuties when assignmentData is defined; target the useEffect
surrounding setAssignmentDuties and the assignmentData/assignment_duties
references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 86ecb31e-cee3-479f-b8a7-6b44854dd976
📒 Files selected for processing (1)
src/pages/Assignments/AssignmentEditor.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
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.
Summary by CodeRabbit
New Features
Tests