E2609: Calibration tab UI & comparison report#177
Open
ruju4a wants to merge 15 commits into
Open
Conversation
…nt-end into rujuta merge
Integrating add participant changes with report changes
📝 WalkthroughWalkthroughAdds an assignment calibration feature: new route and page for calibration review, backend-driven participant management in the AssignmentEditor, normalization utilities, visualization components (stacked and distribution charts), a rubric detail panel, supporting hook for instructor demo, and associated tests and test setup changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant Backend
participant NormalizeFn as normalizeCalibrationReport()
participant CalibrationReview as CalibrationReview Component
participant Charts as Chart Components
User->>Browser: Navigate to calibration review route
Browser->>Backend: GET /assignments/{id}/reports/calibration/{mapId}
Backend-->>Browser: CalibrationReportResponse
Browser->>NormalizeFn: normalizeCalibrationReport(response)
NormalizeFn-->>Browser: NormalizedCalibrationReport
Browser->>CalibrationReview: set state (report, selectedReviewerId)
CalibrationReview->>Charts: render comparison + rubric components
Charts-->>Browser: display charts and rubric detail
User->>CalibrationReview: change reviewer selection
CalibrationReview->>Charts: re-render with reviewer-specific rows
Charts-->>Browser: update displayed rubric details
sequenceDiagram
participant User
participant AssignmentEditor as AssignmentEditor Component
participant Backend
participant ParticipantTable as Participant Table
User->>AssignmentEditor: Enter username + click Add (or press Enter)
AssignmentEditor->>Backend: POST /assignments/:id/review_mappings/calibration_participants { username }
Backend-->>AssignmentEditor: 2xx success
AssignmentEditor->>AssignmentEditor: show success alert
AssignmentEditor->>Backend: GET /assignments/:id/review_mappings/calibration_participants (refresh)
Backend-->>AssignmentEditor: updated participants list
AssignmentEditor->>ParticipantTable: update rows state
ParticipantTable-->>User: show new participant row
User->>ParticipantTable: Click "Remove" on a row
ParticipantTable->>Backend: DELETE /assignments/:id/review_mappings/calibration_participants/{participant_id}
Backend-->>ParticipantTable: success
ParticipantTable->>AssignmentEditor: trigger refresh
AssignmentEditor->>ParticipantTable: update rows state
User->>ParticipantTable: Click "Begin" on not_started row
ParticipantTable->>Backend: POST mock instructor endpoint via hook
Backend-->>ParticipantTable: success
ParticipantTable->>AssignmentEditor: refresh participants
AssignmentEditor->>ParticipantTable: show updated status (View/Edit)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ruju4a
changed the title
Integrate calibration tab and report page changes
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/pages/Assignments/components/CalibrationRubricDistributionChart.tsx (1)
24-25: Consider making the chart container width responsive.A fixed
width: 300can cause overflow in smaller containers.width: "100%"withmaxWidth: 300preserves layout better.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Assignments/components/CalibrationRubricDistributionChart.tsx` around lines 24 - 25, The chart container in CalibrationRubricDistributionChart currently uses a fixed width (the outer div with data-testid `rubric-distribution-${row.itemId}`); change its inline style from width: 300 to a responsive style like width: "100%" and maxWidth: 300 so the chart scales down in smaller containers while preserving max width. Update the outer div's style object accordingly (keep the existing data-testid and inner height div unchanged).src/pages/Assignments/__tests__/CalibrationStackedChart.test.tsx (1)
45-46: Make numeric assertions less brittle.
getByText("4")/getByText("5")can become ambiguous as UI grows. Prefer scoping to the instructor table (e.g.,within(...)) or asserting full row content.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Assignments/__tests__/CalibrationStackedChart.test.tsx` around lines 45 - 46, The numeric assertions using screen.getByText("4") and screen.getByText("5") are brittle and can match unrelated elements; scope them to the instructor table or assert the full row content instead: locate the instructor table (e.g., using screen.getByRole('table', { name: /instructor/i }) or a container query), then use within(table).getByText(...) or within(row).toHaveTextContent('Instructor Name 4 5') to assert the exact row content; replace the two screen.getByText calls with scoped within(...) lookups for the instructor table/row to make the test robust.
🤖 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 583-587: In AssignmentEditor.tsx inside the rows.map block (where
personName is computed), stop prefixing the fallback with "Team_": change the
participant_name assignment from row.team_name || `Team_${personName}` to use
the real participant name as the fallback (e.g., row.team_name || personName) so
that when team_name is absent the UI shows the actual username/handle/full_name
used in searches and actions.
- Around line 1308-1315: The Add and Remove calibration Buttons in
AssignmentEditor.tsx are missing an explicit type and will default to submitting
the Formik form; update the <Button> for handleAddCalibrationParticipant (the
one using calibrationUsername and disabled={!calibrationUsername.trim()}) and
any corresponding calibration Remove button to include type="button" so clicks
don't trigger form submit; locate the Button components in the AssignmentEditor
component and add the type prop to each calibration action Button.
In `@src/pages/Assignments/calibrationReportNormalize.ts`:
- Around line 101-116: latestStudentResponsesForMaps is currently choosing the
newest response even if it's an autosaved draft; update the selection logic to
ignore drafts by only considering responses with is_submitted === true (e.g.,
filter responses to responses.filter(r => r.is_submitted) before the reduce, or
add a guard inside the reduce to skip when response.is_submitted is false) while
keeping the same reduce-by-map and the existing sort by reviewer_name so
submitted calibrations replace drafts in the reviewer dropdown and rubric detail
panel.
In `@src/pages/Assignments/CalibrationReview.tsx`:
- Around line 56-58: The conditional that chooses reviewer-specific rows
incorrectly treats a reviewer id of 0 as falsy; update the check in the
expression that uses normalizedReport and selectedReviewerId so it uses an
explicit null check (selectedReviewerId !== null) instead of a truthy check,
keeping the same fallback to normalizedReport.rubricDetailRows when the
reviewer-specific map
(normalizedReport.rubricDetailRowsByReviewer[selectedReviewerId]) is missing;
modify the expression referencing normalizedReport, selectedReviewerId,
rubricDetailRowsByReviewer, and rubricDetailRows accordingly.
- Around line 20-43: The useEffect's loadReport can have out-of-order responses
that overwrite state; prevent this by tracking and validating the active request
before calling setReport/setError/setLoading: create a per-effect signal (either
an AbortController passed into axiosClient.get or an incrementing requestId
stored in a ref) inside the useEffect, attach it to the axios request, and in
the response and catch handlers verify the signal matches (or that
!signal.aborted) before calling setReport, setError or setLoading; ensure you
abort/mark the signal stale in the cleanup return of the useEffect so previous
requests cannot update state (update code in the useEffect surrounding
loadReport, and references to loadReport, axiosClient.get, setReport, setError,
setLoading).
- Around line 127-131: In CalibrationReview.tsx, the hyperlinks.map render is
using backend-provided links directly; add a validation step (e.g., create a
helper validateUrl or isSafeProtocol) that uses the URL constructor or regex to
allow only safe protocols (http:, https:, mailto:) and rejects others; update
the mapping in the CalibrationReview component to either filter out unsafe links
or render them with a safe fallback (e.g., plain text or href="#" and no target)
when validateUrl(link) returns false, ensuring the anchor tag's href is never
set to an unsafe scheme.
---
Nitpick comments:
In `@src/pages/Assignments/__tests__/CalibrationStackedChart.test.tsx`:
- Around line 45-46: The numeric assertions using screen.getByText("4") and
screen.getByText("5") are brittle and can match unrelated elements; scope them
to the instructor table or assert the full row content instead: locate the
instructor table (e.g., using screen.getByRole('table', { name: /instructor/i })
or a container query), then use within(table).getByText(...) or
within(row).toHaveTextContent('Instructor Name 4 5') to assert the exact row
content; replace the two screen.getByText calls with scoped within(...) lookups
for the instructor table/row to make the test robust.
In `@src/pages/Assignments/components/CalibrationRubricDistributionChart.tsx`:
- Around line 24-25: The chart container in CalibrationRubricDistributionChart
currently uses a fixed width (the outer div with data-testid
`rubric-distribution-${row.itemId}`); change its inline style from width: 300 to
a responsive style like width: "100%" and maxWidth: 300 so the chart scales down
in smaller containers while preserving max width. Update the outer div's style
object accordingly (keep the existing data-testid and inner height div
unchanged).
🪄 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: 0458fe60-ab79-48cc-bf73-b12289bdbedc
📒 Files selected for processing (11)
src/App.tsxsrc/pages/Assignments/AssignmentEditor.tsxsrc/pages/Assignments/CalibrationReview.tsxsrc/pages/Assignments/__tests__/CalibrationRubricDetailPanel.test.tsxsrc/pages/Assignments/__tests__/CalibrationStackedChart.test.tsxsrc/pages/Assignments/__tests__/calibrationReportNormalize.test.tssrc/pages/Assignments/calibrationReportNormalize.tssrc/pages/Assignments/components/CalibrationRubricDetailPanel.tsxsrc/pages/Assignments/components/CalibrationRubricDistributionChart.tsxsrc/pages/Assignments/components/CalibrationStackedChart.tsxsrc/test/setup.ts
Comment on lines
+583
to
+587
| rows.map((row: any) => { | ||
| const personName = row.full_name || row.username || row.handle || `Participant ${row.participant_id}`; | ||
| return { | ||
| participant_id: row.participant_id, | ||
| participant_name: row.team_name || `Team_${personName}`, |
There was a problem hiding this comment.
Use the real participant name as the fallback.
The column is labeled “Participant name”, but this fallback turns alice into Team_alice whenever team_name is absent. That makes the list diverge from the searched username and is confusing in remove/report actions.
Suggested fix
- participant_name: row.team_name || `Team_${personName}`,
+ participant_name: row.team_name || personName,📝 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
| rows.map((row: any) => { | |
| const personName = row.full_name || row.username || row.handle || `Participant ${row.participant_id}`; | |
| return { | |
| participant_id: row.participant_id, | |
| participant_name: row.team_name || `Team_${personName}`, | |
| rows.map((row: any) => { | |
| const personName = row.full_name || row.username || row.handle || `Participant ${row.participant_id}`; | |
| return { | |
| participant_id: row.participant_id, | |
| participant_name: row.team_name || personName, |
🤖 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 583 - 587, In
AssignmentEditor.tsx inside the rows.map block (where personName is computed),
stop prefixing the fallback with "Team_": change the participant_name assignment
from row.team_name || `Team_${personName}` to use the real participant name as
the fallback (e.g., row.team_name || personName) so that when team_name is
absent the UI shows the actual username/handle/full_name used in searches and
actions.
Comment on lines
+1308
to
+1315
| <Button | ||
| variant="outline-secondary" | ||
| onClick={handleAddCalibrationParticipant} | ||
| disabled={!calibrationUsername.trim()} | ||
| style={{ height: 38, lineHeight: 1, padding: '0 1rem', boxSizing: 'border-box' }} | ||
| > | ||
| Add | ||
| </Button> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l src/pages/Assignments/AssignmentEditor.tsxRepository: expertiza/reimplementation-front-end
Length of output: 128
🏁 Script executed:
# Check the code around lines 1308-1315
sed -n '1295,1320p' src/pages/Assignments/AssignmentEditor.tsxRepository: expertiza/reimplementation-front-end
Length of output: 1901
🏁 Script executed:
# Check the code around lines 1437-1443
sed -n '1425,1450p' src/pages/Assignments/AssignmentEditor.tsxRepository: expertiza/reimplementation-front-end
Length of output: 2211
🏁 Script executed:
# Find the Form wrapper context - search for Formik Form
rg "from.*formik|import.*Formik|<Form" src/pages/Assignments/AssignmentEditor.tsx -A 2 -B 2 | head -50Repository: expertiza/reimplementation-front-end
Length of output: 2795
🏁 Script executed:
# Check Button component import and implementation
rg "Button" src/pages/Assignments/AssignmentEditor.tsx | head -5Repository: expertiza/reimplementation-front-end
Length of output: 417
🏁 Script executed:
rg "handleAddCalibrationParticipant" src/pages/Assignments/AssignmentEditor.tsx -B 5 -A 5Repository: expertiza/reimplementation-front-end
Length of output: 1590
🏁 Script executed:
rg "variant=\"outline-secondary\"" src/pages/Assignments/AssignmentEditor.tsx -B 3 -A 3 | head -100Repository: expertiza/reimplementation-front-end
Length of output: 3226
🏁 Script executed:
# Search for the exact Button code around line 1308-1315
sed -n '1300,1325p' src/pages/Assignments/AssignmentEditor.tsxRepository: expertiza/reimplementation-front-end
Length of output: 1838
🏁 Script executed:
rg "handleRemoveCalibrationParticipant" src/pages/Assignments/AssignmentEditor.tsx -B 3 -A 3Repository: expertiza/reimplementation-front-end
Length of output: 732
🏁 Script executed:
# Check the exact lines 1437-1443
sed -n '1430,1450p' src/pages/Assignments/AssignmentEditor.tsxRepository: expertiza/reimplementation-front-end
Length of output: 1793
🏁 Script executed:
# Search for both Button components to verify if they're inside Form context
sed -n '1270,1450p' src/pages/Assignments/AssignmentEditor.tsx | grep -A 200 "Calibration"Repository: expertiza/reimplementation-front-end
Length of output: 11128
🏁 Script executed:
# Check the overall structure to confirm Form context
sed -n '1200,1330p' src/pages/Assignments/AssignmentEditor.tsx | grep -E "<Form>|<Formik|<Tab|calibration" | head -20Repository: expertiza/reimplementation-front-end
Length of output: 1379
🏁 Script executed:
# Verify the second button location and form context
sed -n '1380,1460p' src/pages/Assignments/AssignmentEditor.tsx | grep -E "<Form>|</Form>|<Tab|variant=" | head -30Repository: expertiza/reimplementation-front-end
Length of output: 254
🌐 Web query:
react-bootstrap Button component default type attribute behavior
💡 Result:
The react-bootstrap Button component does not set a default value for the HTML 'type' attribute. When used as a element (the default), it renders without an explicit type="...", so browsers default to type="submit" if the Button is inside a element. This can cause unintended form submissions for non-submit buttons. To avoid this, explicitly set type="button" (or "submit", "reset" as needed) on the Button component. This behavior comes from the Button source code, which spreads all props (including 'type' if provided) to the underlying element via useButtonProps from @restart/ui and passes ...props. No default type is set in ButtonProps or the implementation. Official docs do not document a default type, and examples omit it.
Citations:
- 1: https://react-bootstrap.github.io/docs/components/buttons/
- 2: https://github.com/react-bootstrap/react-bootstrap/blob/master/src/Button.tsx
- 3: fix(Button): Use type attribute if specified react-bootstrap/react-bootstrap#5208
Add type="button" to calibration action buttons inside the Formik Form.
Both the Add and Remove calibration buttons currently lack an explicit type attribute. Since react-bootstrap's Button component does not set a default type, browsers will treat these as type="submit" when inside a form element, risking unintended assignment save submissions when users click Add/Remove.
Suggested fix
<Button
+ type="button"
variant="outline-secondary"
onClick={handleAddCalibrationParticipant}
disabled={!calibrationUsername.trim()}
style={{ height: 38, lineHeight: 1, padding: '0 1rem', boxSizing: 'border-box' }}
>
Add
</Button> <Button
+ type="button"
variant="outline-danger"
size="sm"
onClick={() => handleRemoveCalibrationParticipant(row.original.participant_id)}
>
Remove
</Button>🤖 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 1308 - 1315, The Add
and Remove calibration Buttons in AssignmentEditor.tsx are missing an explicit
type and will default to submitting the Formik form; update the <Button> for
handleAddCalibrationParticipant (the one using calibrationUsername and
disabled={!calibrationUsername.trim()}) and any corresponding calibration Remove
button to include type="button" so clicks don't trigger form submit; locate the
Button components in the AssignmentEditor component and add the type prop to
each calibration action Button.
Comment on lines
+101
to
+116
| const latestStudentResponsesForMaps = (responses: CalibrationResponse[]) => | ||
| Object.values( | ||
| responses.reduce<Record<number, CalibrationResponse>>((latestByMap, response) => { | ||
| const current = latestByMap[response.map_id]; | ||
|
|
||
| if (!current || new Date(response.updated_at).getTime() > new Date(current.updated_at).getTime()) { | ||
| latestByMap[response.map_id] = response; | ||
| } | ||
|
|
||
| return latestByMap; | ||
| }, {}) | ||
| ).sort((left, right) => { | ||
| const leftName = left.reviewer_name ?? ""; | ||
| const rightName = right.reviewer_name ?? ""; | ||
| return leftName.localeCompare(rightName); | ||
| }); |
There was a problem hiding this comment.
Exclude drafts when selecting the latest student response.
latestStudentResponsesForMaps currently picks the newest record even when is_submitted is false. A newer autosaved draft would then replace the actual submitted calibration in the reviewer dropdown and rubric detail panel.
Suggested fix
const latestStudentResponsesForMaps = (responses: CalibrationResponse[]) =>
Object.values(
- responses.reduce<Record<number, CalibrationResponse>>((latestByMap, response) => {
+ responses
+ .filter((response) => response.is_submitted)
+ .reduce<Record<number, CalibrationResponse>>((latestByMap, response) => {
const current = latestByMap[response.map_id];
if (!current || new Date(response.updated_at).getTime() > new Date(current.updated_at).getTime()) {
latestByMap[response.map_id] = response;
}
return latestByMap;
- }, {})
+ }, {})
).sort((left, right) => {📝 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 latestStudentResponsesForMaps = (responses: CalibrationResponse[]) => | |
| Object.values( | |
| responses.reduce<Record<number, CalibrationResponse>>((latestByMap, response) => { | |
| const current = latestByMap[response.map_id]; | |
| if (!current || new Date(response.updated_at).getTime() > new Date(current.updated_at).getTime()) { | |
| latestByMap[response.map_id] = response; | |
| } | |
| return latestByMap; | |
| }, {}) | |
| ).sort((left, right) => { | |
| const leftName = left.reviewer_name ?? ""; | |
| const rightName = right.reviewer_name ?? ""; | |
| return leftName.localeCompare(rightName); | |
| }); | |
| const latestStudentResponsesForMaps = (responses: CalibrationResponse[]) => | |
| Object.values( | |
| responses | |
| .filter((response) => response.is_submitted) | |
| .reduce<Record<number, CalibrationResponse>>((latestByMap, response) => { | |
| const current = latestByMap[response.map_id]; | |
| if (!current || new Date(response.updated_at).getTime() > new Date(current.updated_at).getTime()) { | |
| latestByMap[response.map_id] = response; | |
| } | |
| return latestByMap; | |
| }, {}) | |
| ).sort((left, right) => { | |
| const leftName = left.reviewer_name ?? ""; | |
| const rightName = right.reviewer_name ?? ""; | |
| return leftName.localeCompare(rightName); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Assignments/calibrationReportNormalize.ts` around lines 101 - 116,
latestStudentResponsesForMaps is currently choosing the newest response even if
it's an autosaved draft; update the selection logic to ignore drafts by only
considering responses with is_submitted === true (e.g., filter responses to
responses.filter(r => r.is_submitted) before the reduce, or add a guard inside
the reduce to skip when response.is_submitted is false) while keeping the same
reduce-by-map and the existing sort by reviewer_name so submitted calibrations
replace drafts in the reviewer dropdown and rubric detail panel.
Comment on lines
+20
to
+43
| useEffect(() => { | ||
| const loadReport = async () => { | ||
| if (!assignmentId || !mapId) { | ||
| setError("Missing assignment or calibration map id."); | ||
| setLoading(false); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| setLoading(true); | ||
| setError(null); | ||
| const response = await axiosClient.get<CalibrationReportResponse>( | ||
| `/assignments/${assignmentId}/reports/calibration/${mapId}` | ||
| ); | ||
| setReport(response.data); | ||
| } catch (err: any) { | ||
| setError(err?.response?.data?.error || "Unable to load calibration report"); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }; | ||
|
|
||
| loadReport(); | ||
| }, [assignmentId, mapId]); |
There was a problem hiding this comment.
Prevent stale state from out-of-order fetch completions.
If route params change quickly, an older request can resolve last and overwrite the newer report/error state.
🔧 Suggested fix
useEffect(() => {
+ let isCurrent = true;
const loadReport = async () => {
if (!assignmentId || !mapId) {
- setError("Missing assignment or calibration map id.");
- setLoading(false);
+ if (isCurrent) {
+ setError("Missing assignment or calibration map id.");
+ setLoading(false);
+ }
return;
}
try {
- setLoading(true);
- setError(null);
+ if (isCurrent) {
+ setLoading(true);
+ setError(null);
+ }
const response = await axiosClient.get<CalibrationReportResponse>(
`/assignments/${assignmentId}/reports/calibration/${mapId}`
);
- setReport(response.data);
+ if (isCurrent) {
+ setReport(response.data);
+ }
} catch (err: any) {
- setError(err?.response?.data?.error || "Unable to load calibration report");
+ if (isCurrent) {
+ setError(err?.response?.data?.error || "Unable to load calibration report");
+ }
} finally {
- setLoading(false);
+ if (isCurrent) {
+ setLoading(false);
+ }
}
};
loadReport();
+ return () => {
+ isCurrent = false;
+ };
}, [assignmentId, mapId]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Assignments/CalibrationReview.tsx` around lines 20 - 43, The
useEffect's loadReport can have out-of-order responses that overwrite state;
prevent this by tracking and validating the active request before calling
setReport/setError/setLoading: create a per-effect signal (either an
AbortController passed into axiosClient.get or an incrementing requestId stored
in a ref) inside the useEffect, attach it to the axios request, and in the
response and catch handlers verify the signal matches (or that !signal.aborted)
before calling setReport, setError or setLoading; ensure you abort/mark the
signal stale in the cleanup return of the useEffect so previous requests cannot
update state (update code in the useEffect surrounding loadReport, and
references to loadReport, axiosClient.get, setReport, setError, setLoading).
Comment on lines
+56
to
+58
| normalizedReport && selectedReviewerId | ||
| ? normalizedReport.rubricDetailRowsByReviewer[selectedReviewerId] ?? normalizedReport.rubricDetailRows | ||
| : normalizedReport?.rubricDetailRows ?? []; |
There was a problem hiding this comment.
Use an explicit null check for selectedReviewerId.
The current truthy check skips reviewer-specific rows when id is 0. Use selectedReviewerId !== null for correctness.
🔧 Suggested fix
const rubricDetailRows =
- normalizedReport && selectedReviewerId
+ normalizedReport && selectedReviewerId !== null
? normalizedReport.rubricDetailRowsByReviewer[selectedReviewerId] ?? normalizedReport.rubricDetailRows
: normalizedReport?.rubricDetailRows ?? [];📝 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
| normalizedReport && selectedReviewerId | |
| ? normalizedReport.rubricDetailRowsByReviewer[selectedReviewerId] ?? normalizedReport.rubricDetailRows | |
| : normalizedReport?.rubricDetailRows ?? []; | |
| const rubricDetailRows = | |
| normalizedReport && selectedReviewerId !== null | |
| ? normalizedReport.rubricDetailRowsByReviewer[selectedReviewerId] ?? normalizedReport.rubricDetailRows | |
| : normalizedReport?.rubricDetailRows ?? []; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Assignments/CalibrationReview.tsx` around lines 56 - 58, The
conditional that chooses reviewer-specific rows incorrectly treats a reviewer id
of 0 as falsy; update the check in the expression that uses normalizedReport and
selectedReviewerId so it uses an explicit null check (selectedReviewerId !==
null) instead of a truthy check, keeping the same fallback to
normalizedReport.rubricDetailRows when the reviewer-specific map
(normalizedReport.rubricDetailRowsByReviewer[selectedReviewerId]) is missing;
modify the expression referencing normalizedReport, selectedReviewerId,
rubricDetailRowsByReviewer, and rubricDetailRows accordingly.
Comment on lines
+127
to
+131
| {hyperlinks.map((link, idx) => ( | ||
| <li key={`${link}-${idx}`}> | ||
| <a href={link} target="_blank" rel="noopener noreferrer"> | ||
| {link} | ||
| </a> |
There was a problem hiding this comment.
Validate submitted hyperlink protocols before rendering anchor tags.
Rendering backend-provided links directly allows unsafe schemes (e.g., javascript:), which is a security risk on click.
🔒 Suggested fix
{hyperlinks.map((link, idx) => (
- <li key={`${link}-${idx}`}>
- <a href={link} target="_blank" rel="noopener noreferrer">
- {link}
- </a>
- </li>
+ <li key={`${link}-${idx}`}>
+ {(() => {
+ try {
+ const parsed = new URL(link);
+ const isSafe = parsed.protocol === "http:" || parsed.protocol === "https:";
+ return isSafe ? (
+ <a href={parsed.toString()} target="_blank" rel="noopener noreferrer">
+ {link}
+ </a>
+ ) : (
+ <span>{link}</span>
+ );
+ } catch {
+ return <span>{link}</span>;
+ }
+ })()}
+ </li>
))}📝 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
| {hyperlinks.map((link, idx) => ( | |
| <li key={`${link}-${idx}`}> | |
| <a href={link} target="_blank" rel="noopener noreferrer"> | |
| {link} | |
| </a> | |
| {hyperlinks.map((link, idx) => ( | |
| <li key={`${link}-${idx}`}> | |
| {(() => { | |
| try { | |
| const parsed = new URL(link); | |
| const isSafe = parsed.protocol === "http:" || parsed.protocol === "https:"; | |
| return isSafe ? ( | |
| <a href={parsed.toString()} target="_blank" rel="noopener noreferrer"> | |
| {link} | |
| </a> | |
| ) : ( | |
| <span>{link}</span> | |
| ); | |
| } catch { | |
| return <span>{link}</span>; | |
| } | |
| })()} | |
| </li> | |
| ))} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Assignments/CalibrationReview.tsx` around lines 127 - 131, In
CalibrationReview.tsx, the hyperlinks.map render is using backend-provided links
directly; add a validation step (e.g., create a helper validateUrl or
isSafeProtocol) that uses the URL constructor or regex to allow only safe
protocols (http:, https:, mailto:) and rejects others; update the mapping in the
CalibrationReview component to either filter out unsafe links or render them
with a safe fallback (e.g., plain text or href="#" and no target) when
validateUrl(link) returns false, ensuring the anchor tag's href is never set to
an unsafe scheme.
Added tests for calibration report ui
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/pages/Assignments/hooks/useCalibrationInstructorDemo.ts (1)
70-84: Document the stability requirement foronSuccess.The
onSuccesscallback is included in the dependency array. If passed an unstable function reference (one not wrapped inuseCallback), this effect could fire unexpectedly on every render after a successful response. The current usage inAssignmentEditor.tsxis safe sincerefreshCalibrationParticipantsis memoized, but consider documenting this expectation in the JSDoc or interface.Suggested documentation update
interface UseCalibrationInstructorDemoOptions { assignmentId: number | string | null | undefined; - onSuccess: () => void; // called after a successful seed so the table refreshes + /** + * Called after a successful seed so the table refreshes. + * Must be a stable reference (e.g., wrapped in useCallback) to avoid + * re-triggering the effect on every render. + */ + onSuccess: () => void; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Assignments/hooks/useCalibrationInstructorDemo.ts` around lines 70 - 84, The effect watching beginCalibrationResponse currently includes onSuccess in its dependency array; document that onSuccess must be a stable (memoized) callback to avoid the effect re-running unexpectedly — update the JSDoc or the hook interface for useCalibrationInstructorDemo to state that onSuccess should be wrapped with useCallback (or otherwise be stable), and reference the onSuccess parameter and the useEffect that reads beginCalibrationResponse so callers (e.g., AssignmentEditor.tsx which passes refreshCalibrationParticipants) know to memoize their handler.src/pages/Assignments/AssignmentEditor.tsx (2)
1348-1354: Use React Router'sLinkinstead of<a>for SPA navigation.Using raw
<a href>tags for internal routes causes full page reloads instead of client-side navigation. Since these point to routes within the app (/assignments/edit/.../calibration/...), they should use React Router'sLinkoruseNavigate.Suggested fix
+import { Link } from "react-router-dom"; // ... in the cell renderer: - return ( - <span> - <a style={linkStyle} href={`${reviewBase}/view`}>View</a> - <span style={{ margin: '0 0.4rem', color: '#986633' }}>|</span> - <a style={linkStyle} href={`${reviewBase}/edit`}>Edit</a> - </span> - ); + return ( + <span> + <Link style={linkStyle} to={`${reviewBase}/view`}>View</Link> + <span style={{ margin: '0 0.4rem', color: '#986633' }}>|</span> + <Link style={linkStyle} to={`${reviewBase}/edit`}>Edit</Link> + </span> + );🤖 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 1348 - 1354, The current JSX in AssignmentEditor.tsx renders internal navigation using raw <a> tags (see the span that returns View/Edit with linkStyle and reviewBase), which causes full page reloads; replace these <a href={`${reviewBase}/view`}> and <a href={`${reviewBase}/edit`}> with React Router <Link> components (or useNavigate) to enable client-side routing, preserving the same linkStyle and URL paths (e.g., <Link to={`${reviewBase}/view`} style={linkStyle}> and similarly for edit) and ensure you import Link from react-router-dom at the top of the file.
1359-1371: Use React Router'sLinkfor the "View review report" link.Same issue—this internal route should use
Linkfor client-side navigation.Suggested fix
- return ( - <a style={{ color: '#986633', textDecoration: 'none' }} href={href}> - View review report - </a> - ); + return ( + <Link style={{ color: '#986633', textDecoration: 'none' }} to={href}> + View review report + </Link> + );🤖 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 1359 - 1371, The cell renderer for the "Report" column currently returns an <a> tag using the computed href (mapId -> `/assignments/edit/${assignmentData.id}/calibration/${mapId}` or '#'); replace that anchor with React Router's Link component (import { Link } from 'react-router-dom') and use the Link's "to" prop instead of href, preserving the inline styles and text ("View review report"); ensure the fallback when mapId is falsy either renders a disabled/non-clickable element or omits the Link (keep existing visual style) so client-side navigation is used for valid internal routes.
🤖 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/__tests__/CalibrationReview.test.tsx`:
- Around line 72-74: The test currently waits for screen.getByText(/calibration
report/i) which matches both the heading and the loading message; replace this
with an explicit role-based wait so the test waits for the actual heading.
Update the assertion that uses waitFor + screen.getByText to instead await
screen.findByRole("heading", { name: /calibration report/i }) (or assert its
presence with expect(await screen.findByRole(...)).toBeInTheDocument()) to
ensure you wait specifically for the heading element.
In `@src/pages/Assignments/AssignmentEditor.tsx`:
- Line 1291: In AssignmentEditor.tsx remove the erroneous CSS declaration
display: 'ruby' from the inline style on the div (the <div style={{ display:
'ruby', marginTop: '30px' }}> element) — either delete the display property
entirely or replace it with the intended value (e.g., 'block' or leave only
marginTop) so the calibration table layout is not affected.
- Around line 1372-1399: The cell renderer in AssignmentEditor (the cell: ({ row
}) => ... block rendering row.original.submitted_content?.hyperlinks and files)
currently binds user-provided URLs directly to <a href={item}> which allows
javascript: URIs; update the hyperlink and file href construction to validate
and sanitize the URL before rendering: attempt to parse with the URL constructor
and only allow safe protocols (e.g. http:, https:, mailto:), otherwise treat as
unsafe and either drop the link or render it as plain text; for non-string file
objects use item.path only after the same check; also add safe anchor attributes
like rel="noopener noreferrer" and target="_blank" if opening external links.
---
Nitpick comments:
In `@src/pages/Assignments/AssignmentEditor.tsx`:
- Around line 1348-1354: The current JSX in AssignmentEditor.tsx renders
internal navigation using raw <a> tags (see the span that returns View/Edit with
linkStyle and reviewBase), which causes full page reloads; replace these <a
href={`${reviewBase}/view`}> and <a href={`${reviewBase}/edit`}> with React
Router <Link> components (or useNavigate) to enable client-side routing,
preserving the same linkStyle and URL paths (e.g., <Link
to={`${reviewBase}/view`} style={linkStyle}> and similarly for edit) and ensure
you import Link from react-router-dom at the top of the file.
- Around line 1359-1371: The cell renderer for the "Report" column currently
returns an <a> tag using the computed href (mapId ->
`/assignments/edit/${assignmentData.id}/calibration/${mapId}` or '#'); replace
that anchor with React Router's Link component (import { Link } from
'react-router-dom') and use the Link's "to" prop instead of href, preserving the
inline styles and text ("View review report"); ensure the fallback when mapId is
falsy either renders a disabled/non-clickable element or omits the Link (keep
existing visual style) so client-side navigation is used for valid internal
routes.
In `@src/pages/Assignments/hooks/useCalibrationInstructorDemo.ts`:
- Around line 70-84: The effect watching beginCalibrationResponse currently
includes onSuccess in its dependency array; document that onSuccess must be a
stable (memoized) callback to avoid the effect re-running unexpectedly — update
the JSDoc or the hook interface for useCalibrationInstructorDemo to state that
onSuccess should be wrapped with useCallback (or otherwise be stable), and
reference the onSuccess parameter and the useEffect that reads
beginCalibrationResponse so callers (e.g., AssignmentEditor.tsx which passes
refreshCalibrationParticipants) know to memoize their handler.
🪄 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: a2c13014-0e5c-4b44-96d5-99b679db0de0
📒 Files selected for processing (4)
src/pages/Assignments/AssignmentEditor.tsxsrc/pages/Assignments/__tests__/CalibrationReview.test.tsxsrc/pages/Assignments/__tests__/calibrationReportNormalize.test.tssrc/pages/Assignments/hooks/useCalibrationInstructorDemo.ts
Comment on lines
+72
to
+74
| await waitFor(() => | ||
| expect(screen.getByText(/calibration report/i)).toBeInTheDocument() | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify overlapping text that makes the current wait condition ambiguous.
rg -n 'Loading calibration report|getByText\(/calibration report|Calibration Report' \
src/pages/Assignments/CalibrationReview.tsx \
src/pages/Assignments/__tests__/CalibrationReview.test.tsxRepository: expertiza/reimplementation-front-end
Length of output: 418
Wait for the heading element to avoid matching the loading text.
/calibration report/i matches both "Calibration Report" (the actual heading) and "Loading calibration report..." (the loading state), causing the wait to complete too early. Use findByRole("heading") instead, which waits specifically for the heading to appear.
Proposed fix
await waitFor(() =>
- expect(screen.getByText(/calibration report/i)).toBeInTheDocument()
+ await screen.findByRole("heading", { name: /calibration report/i });📝 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
| await waitFor(() => | |
| expect(screen.getByText(/calibration report/i)).toBeInTheDocument() | |
| ); | |
| await screen.findByRole("heading", { name: /calibration report/i }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Assignments/__tests__/CalibrationReview.test.tsx` around lines 72 -
74, The test currently waits for screen.getByText(/calibration report/i) which
matches both the heading and the loading message; replace this with an explicit
role-based wait so the test waits for the actual heading. Update the assertion
that uses waitFor + screen.getByText to instead await
screen.findByRole("heading", { name: /calibration report/i }) (or assert its
presence with expect(await screen.findByRole(...)).toBeInTheDocument()) to
ensure you wait specifically for the heading element.
| <h3 style={{ marginTop: '1.5rem' }}>Select participants for submitting calibration artifacts</h3> | ||
|
|
||
| <div> | ||
| <div style={{ display: 'ruby', marginTop: '30px' }}> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if display: ruby is used intentionally elsewhere in the codebase
rg "display:\s*['\"]?ruby" --type=ts --type=tsx --type=css --type=scss || echo "No other occurrences found"Repository: expertiza/reimplementation-front-end
Length of output: 153
display: ruby appears to be a typo.
display: ruby is a CSS value for East Asian Ruby annotation layout and has no other occurrences in the codebase. In the context of a calibration table, this is almost certainly unintended and should be removed.
Suggested fix
- <div style={{ display: 'ruby', marginTop: '30px' }}>
+ <div style={{ marginTop: '30px' }}>📝 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
| <div style={{ display: 'ruby', marginTop: '30px' }}> | |
| <div style={{ marginTop: '30px' }}> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Assignments/AssignmentEditor.tsx` at line 1291, In
AssignmentEditor.tsx remove the erroneous CSS declaration display: 'ruby' from
the inline style on the div (the <div style={{ display: 'ruby', marginTop:
'30px' }}> element) — either delete the display property entirely or replace it
with the intended value (e.g., 'block' or leave only marginTop) so the
calibration table layout is not affected.
Comment on lines
+1372
to
+1399
| { | ||
| cell: ({ row }) => ( | ||
| <> | ||
| <div>Hyperlinks:</div> | ||
| <div style={{ display: 'flex', flexDirection: 'column', gap: '5px' }}> | ||
| {(row.original.submitted_content?.hyperlinks || []).map((item: any, index: number) => ( | ||
| <a style={{ color: '#986633', textDecoration: 'none' }} key={`hl-${index}`} href={item}>{item}</a> | ||
| ))} | ||
| </div> | ||
| <div style={{ marginTop: '10px', display: 'flex', flexDirection: 'column' }}>Files:</div> | ||
| <div style={{ display: 'flex', flexDirection: 'column', gap: '5px' }}> | ||
| {(row.original.submitted_content?.files || []).map((item: any, index: number) => { | ||
| // Files may arrive as bare strings (report shape) or as | ||
| // { id, name, path, submitted_at, submitted_by } objects | ||
| // (participant-list shape). Handle both. | ||
| const href = typeof item === 'string' ? item : item.path; | ||
| const label = typeof item === 'string' ? item : (item.name || item.path); | ||
| return ( | ||
| <a | ||
| style={{ color: '#986633', textDecoration: 'none' }} | ||
| key={`file-${index}`} | ||
| href={href} | ||
| > | ||
| {label} | ||
| </a> | ||
| ); | ||
| })} | ||
| </div> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's URL validation/sanitization happening elsewhere for submitted hyperlinks
rg -n "hyperlink|submitted_content" --type=ts -A 5 -B 5 | head -100Repository: expertiza/reimplementation-front-end
Length of output: 5591
🏁 Script executed:
# 1. Check the exact code at the specified lines in AssignmentEditor.tsx
wc -l src/pages/Assignments/AssignmentEditor.tsxRepository: expertiza/reimplementation-front-end
Length of output: 128
🏁 Script executed:
# 2. Search for sanitization libraries or URL validation in the project
rg -i "dompurify|sanitize|xss|validator" --type=ts,tsx,jsonRepository: expertiza/reimplementation-front-end
Length of output: 116
🏁 Script executed:
# 3. Check if there's any validation on hyperlinks in the submitted_content service
cat -n src/services/SubmittedContentService.tsRepository: expertiza/reimplementation-front-end
Length of output: 9930
🏁 Script executed:
# Read the specific lines from AssignmentEditor.tsx
sed -n '1372,1399p' src/pages/Assignments/AssignmentEditor.tsxRepository: expertiza/reimplementation-front-end
Length of output: 2033
🏁 Script executed:
# Fix the syntax and search for sanitization libraries
rg -i "dompurify|sanitize|xss" --type typescript | head -50Repository: expertiza/reimplementation-front-end
Length of output: 115
🏁 Script executed:
# Check package.json for sanitization dependencies
cat package.json | grep -A 20 -B 5 "dependencies"Repository: expertiza/reimplementation-front-end
Length of output: 842
🏁 Script executed:
# Search for any URL validation or sanitization on hyperlinks
rg "javascript:|href.*validate|sanitize.*url" --type typescriptRepository: expertiza/reimplementation-front-end
Length of output: 115
🏁 Script executed:
# Show full package.json dependencies
cat package.jsonRepository: expertiza/reimplementation-front-end
Length of output: 2246
🏁 Script executed:
# Search for sanitization/validation with correct syntax
rg -i "dompurify|sanitize|xss" --type ts --type tsx | head -50Repository: expertiza/reimplementation-front-end
Length of output: 108
🏁 Script executed:
# Search for any URL validation patterns or href handling
rg "href|javascript:" --type ts --type tsx -A 2 -B 2 | head -100Repository: expertiza/reimplementation-front-end
Length of output: 108
🏁 Script executed:
# Search for sanitization with just -t ts (without tsx)
rg -i "dompurify|sanitize.*url|xss" -t ts | head -30Repository: expertiza/reimplementation-front-end
Length of output: 62
🏁 Script executed:
# Check if there's any backend code in this repo that validates hyperlinks
find . -name "*.rb" -o -name "*.py" 2>/dev/null | head -20Repository: expertiza/reimplementation-front-end
Length of output: 62
🏁 Script executed:
# Look at how other links are handled in the codebase
rg "href=" -t ts -B 2 -A 2 | head -60Repository: expertiza/reimplementation-front-end
Length of output: 4223
Sanitize hyperlinks to prevent javascript: URL injection.
User-submitted hyperlinks are rendered directly as <a href={item}>. The backend validation uses new URL() which accepts javascript: URLs. A malicious payload like javascript:alert(1) would execute when clicked.
Suggested defensive check
{(row.original.submitted_content?.hyperlinks || []).map((item: any, index: number) => {
+ // Basic sanity check: only render http/https URLs
+ const isValidUrl = typeof item === 'string' && /^https?:\/\//i.test(item);
+ if (!isValidUrl) return null;
return (
<a style={{ color: '#986633', textDecoration: 'none' }} key={`hl-${index}`} href={item}>{item}</a>
);
})}🤖 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 1372 - 1399, The
cell renderer in AssignmentEditor (the cell: ({ row }) => ... block rendering
row.original.submitted_content?.hyperlinks and files) currently binds
user-provided URLs directly to <a href={item}> which allows javascript: URIs;
update the hyperlink and file href construction to validate and sanitize the URL
before rendering: attempt to parse with the URL constructor and only allow safe
protocols (e.g. http:, https:, mailto:), otherwise treat as unsafe and either
drop the link or render it as plain text; for non-string file objects use
item.path only after the same check; also add safe anchor attributes like
rel="noopener noreferrer" and target="_blank" if opening external links.
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
1 participant
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.
Calibration tab (assignment editor)
Begin/View | Edit),View review reportlink, submitted content (hyperlinks + files), andRemoveactionCalibration report page (
CalibrationReview)GET /assignments/:id/reports/calibration/:map_idCalibrationStackedChart(Recharts stacked bars: agree / near / disagree per rubric item)CalibrationRubricDetailPanel; per-student dropdown; per-item score diff cards with instructor vs student score, comments, and mini class-distribution chartUtilities
calibrationReportNormalize.ts— transforms raw report JSON into chart-ready structuresCode hygiene
useCalibrationInstructorDemo(gitignored), keeping productionAssignmentEditorcleanSummary by CodeRabbit
New Features
Tests