feat: phase 4.1 + 4.2 — multi-class compare statistics & activity dashboard#145
Open
NesiciCoding wants to merge 4 commits into
Open
feat: phase 4.1 + 4.2 — multi-class compare statistics & activity dashboard#145NesiciCoding wants to merge 4 commits into
NesiciCoding wants to merge 4 commits into
Conversation
…hboard - Statistics: new Compare tab with grouped avg bar chart, per-criterion gap chart, multi-class trend overlay, and rule-based Insights panel (struggling class, weak criterion, divergence thresholds) - Statistics: test results now tracked — student view shows submitted/graded test scores; class view shows per-test average bar chart - Statistics: track/year filter dropdowns scope the class selector - Activity Dashboard: new /activity-dashboard page — rubric/test/essay × class grid with Link/Unlink, Assign All, and Open actions - New utilities: classComparisonAggregator.ts, activityDashboardAggregator.ts (43 unit tests each, including stress-test datasets) - New component: MultiClassTrendChart (Recharts LineChart, one line per class) - e2e: 22-statistics-compare.spec.ts seeds 3 classes (A 100%, B 62%, C 25%) and asserts bar chart, class names, bar count, insights panel, trend line - i18n: all 5 locales updated (statistics.compare.*, activityDashboard.*) - Docs/README/LandingPage updated per CLAUDE.md documentation rule Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughAdds two new features: a Compare mode in ChangesStatistics Compare Mode
Activity Dashboard
Sequence Diagram(s)sequenceDiagram
actor Teacher
participant StatisticsPage
participant compareClasses
participant buildMultiClassTrend
participant getInsights
participant MultiClassTrendChart
Teacher->>StatisticsPage: Select "Compare" mode, pick rubric + classes
StatisticsPage->>compareClasses: rubrics, studentRubrics, selectedClassIds, gradeScale
compareClasses-->>StatisticsPage: ClassComparisonResult[] (avg, median, criterionAvg)
StatisticsPage->>buildMultiClassTrend: selectedClassIds, rubrics, studentRubrics, classNames
buildMultiClassTrend-->>StatisticsPage: MultiTrendPoint[] (time-ordered per-class scores)
StatisticsPage->>getInsights: ClassComparisonResult[], rubricCriteria
getInsights-->>StatisticsPage: Insight[] (struggling / weak_criterion / divergence)
StatisticsPage->>MultiClassTrendChart: data, classIds, classNames
MultiClassTrendChart-->>Teacher: Renders multi-series line chart
Teacher->>StatisticsPage: Click Insights expander
StatisticsPage-->>Teacher: Shows collapsible insight messages
sequenceDiagram
actor Teacher
participant ActivityDashboardPage
participant getActivityRows
participant buildDashboardMatrix
participant useApp
Teacher->>ActivityDashboardPage: Navigate to /activity-dashboard
ActivityDashboardPage->>useApp: rubrics, tests, essayAssignments, classes, students, studentRubrics, studentTests
ActivityDashboardPage->>getActivityRows: rubrics, tests, essayAssignments
getActivityRows-->>ActivityDashboardPage: ActivityRow[] (deduplicated by teacherKey for essays)
ActivityDashboardPage->>buildDashboardMatrix: activities, classes, students, studentRubrics, studentTests, essayAssignments
buildDashboardMatrix-->>ActivityDashboardPage: matrix[activityId][classId] = CellData
ActivityDashboardPage-->>Teacher: Renders activity × class grid with submittedCount badges
Teacher->>ActivityDashboardPage: Click Link/Unlink on rubric cell
ActivityDashboardPage->>useApp: updateClass with toggled rubricIds
Teacher->>ActivityDashboardPage: Click Assign on essay cell
ActivityDashboardPage->>useApp: createEssayAssignment for each unassigned student
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
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 |
…ent lint rule Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
46-56: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUpdate the Features section to describe the Statistics Compare mode and Activity Dashboard.
The Routes table (lines 151–152) was correctly updated with both new routes, but per the coding guideline, the Features section must also reflect major new capabilities. Currently:
- Line 47 (Statistics dashboard) does not mention the new multi-class comparison, insights panel, or trend visualization added in phase 4.1.
- Activity Dashboard (new in phase 4.2) is not described anywhere in the Features section (lines 5–74).
Expand the "5. Analytics & Reporting" section to highlight:
- Multi-class rubric comparison (grouped averages, criterion gaps, trend overlay, insights panel with struggling-class and weak-criteria detection)
- Activity Dashboard (grid view of rubrics/tests/essays against classes with link/unlink and assign actions)
This ensures README documentation is complete and discoverable for new users.
Also applies to: 151-152
🤖 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 `@README.md` around lines 46 - 56, The Analytics & Reporting section in the Features section of README.md is incomplete. The Statistics dashboard bullet point (line 47) needs to be expanded to include the new multi-class comparison capabilities such as grouped averages, criterion gaps, trend overlay, and insights panel with struggling-class and weak-criteria detection. Additionally, add a new bullet point to describe the Activity Dashboard feature (new in phase 4.2) which provides a grid view of rubrics/tests/essays against classes with link/unlink and assign actions. Ensure these additions appear in the Features section (lines 5-74) to maintain documentation consistency with the Routes table that already lists these new routes.Source: Coding guidelines
🤖 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.
Inline comments:
In `@e2e/specs/22-statistics-compare.spec.ts`:
- Around line 76-95: The test `class averages reflect grade data (A > B > C)`
currently only verifies that class labels appear and that three bars exist, but
does not validate the actual average ordering. Add explicit assertions after the
bars check to verify either the displayed numeric values (Class A should be 100,
Class B should be between them, Class C should be 25) or the bar
heights/geometry to confirm they are in descending order, ensuring the test
actually guards against incorrect average computation or bar ordering.
- Around line 16-53: The trend chart requires at least 2 rubrics to render
because multiTrendData returns an array of trend points per rubric, and the
chart gates on checks for data.length >= 2. Currently, the beforeEach fixture
only seeds one rubric (cmp-rubric), so the chart will not render and the
assertion at line 123 checking for visible recharts-line will fail. Add a second
rubric to the beforeEach fixture using buildRubric (similar to how cmp-rubric is
created) and include it in the rm_rubrics array passed to seedStorage, ensuring
the trend chart has sufficient data to render.
In `@src/pages/ActivityDashboardPage.tsx`:
- Line 100: Remove all explanatory comments that describe what the code does
rather than why it exists, as they violate the repo's comment policy.
Specifically, remove the comments at lines 100, 133, 195, 218, 244, 276, and 289
(such as {/* Filter bar */}) since their meaning is already clear from the code
identifiers and structure. Keep only comments that explain non-obvious design
decisions or reasoning that cannot be inferred from reading the code itself.
- Around line 122-126: The option labels (VMBO-BB, VMBO-KB, VMBO-TL, HAVO, VWO)
in the select dropdown are hardcoded English strings that bypass the i18n
translation system. To fix this, first import the useTranslation hook at the top
of ActivityDashboardPage.tsx if not already present, then add translation keys
for each VO track label to src/locales/en.json (for example, voTracks.vmboBb,
voTracks.vmboKb, voTracks.vmboTl, voTracks.havo, voTracks.vwo), and finally
replace each hardcoded option label with a call to the t() function using the
corresponding key from your translation object.
In `@src/pages/DocsPage.tsx`:
- Around line 244-252: User-visible English strings are hardcoded directly in
the DocsPage.tsx configuration objects instead of being moved to translation
keys. Extract all hardcoded strings including the descriptions for the dashboard
entries and the label 'Activity Dashboard' from lines 244-252 and similar
sections, add them as key-value pairs to src/locales/en.json with descriptive
key names, then replace the hardcoded strings in the configuration objects with
references to those keys using the useTranslation hook. Ensure the component
imports and uses the useTranslation hook if not already present, and reference
the translation keys consistently throughout the affected sections.
- Line 251: Replace the raw hex color values `#0ea5e9` used in the style object
(appearing at line 251 and line 1007 in DocsPage.tsx) with a global CSS custom
property variable instead. Either map this color to an existing global CSS
variable like `--accent` if it matches the design intent, or define a new CSS
custom property centrally in your global CSS file and reference it using the
var() function to maintain theme consistency across the application.
In `@src/pages/LandingPage.tsx`:
- Line 47: The TEACHER_FEATURES.desc property contains hardcoded user-facing
text that violates localization guidelines. Extract this text and move it to a
new key in src/locales/en.json (e.g., something like
teacherFeaturesDescription), then import useTranslation hook at the top of the
LandingPage.tsx component if not already present, and replace the hardcoded desc
value with a call to t() using the new locale key. This ensures the landing page
content remains fully localized and consistent with the project's translation
standards.
In `@src/pages/StatisticsPage.tsx`:
- Around line 610-614: The hardcoded English text in the StatisticsPage
component violates localization guidelines. Replace all hardcoded user-visible
strings with localized keys using the t() function from useTranslation.
Specifically, replace the education track labels in the option elements
(VMBO-BB, VMBO-KB, VMBO-TL, HAVO, VWO) and the string "(max 4)" that is directly
appended at line 701 with appropriate t(...) calls referencing new locale keys
that should be added to src/locales/en.json. Ensure useTranslation is imported
and properly used throughout StatisticsPage.
- Around line 69-77: When the filteredClasses changes due to filterTrack or
filterYear updates in the useMemo hook, you need to synchronize the
selectedClassId and compareClassIds state to remove any class IDs that are no
longer present in the filtered list. Add a useEffect that watches the
filteredClasses dependency and verifies that selectedClassId and compareClassIds
only contain IDs that exist in filteredClasses. If selectedClassId or any IDs in
compareClassIds are no longer in filteredClasses, update these state variables
to remove the invalid IDs or reset them appropriately. This ensures that charts
only display classes that match the current filter criteria.
In `@src/utils/activityDashboardAggregator.test.ts`:
- Around line 243-265: Add a new test case within the 'matrix structure'
describe block that tests the scenario where a rubric and test share the same
ID. The test should create two activities with the same id value but different
kind values (e.g., both with id 'duplicate'), call buildDashboardMatrix with
these activities, and then assert that both entries remain independently
addressable by verifying that the matrix contains both activity entries and they
have not overwritten each other. This regression test ensures that the
buildDashboardMatrix function correctly handles cross-kind duplicate IDs without
silently losing data.
In `@src/utils/activityDashboardAggregator.ts`:
- Around line 3-15: Move the three type definitions ActivityKind, ActivityRow,
and CellData from src/utils/activityDashboardAggregator.ts to src/types/index.ts
to maintain centralized domain type definitions. After moving them, import these
types back into activityDashboardAggregator.ts from src/types/index.ts. This
ensures all domain-level shapes are defined in the centralized types location
and remain consistent across the codebase.
- Around line 52-53: The matrix initialization using matrix[activity.id] as the
key causes collisions when different activity kinds share the same ID. Replace
the single activity.id key with a composite key that combines both the activity
kind and ID (for example, creating a unique key string from both values) in the
activityDashboardAggregator.ts file at the matrix initialization point and any
other locations where matrix keys are accessed. Additionally, update all
corresponding lookups in src/pages/ActivityDashboardPage.tsx to use the same
composite key format to maintain consistency across both files.
- Around line 58-59: The variables submittedCount and isLinked are initialized
with default values on lines 58-59 but are unconditionally overwritten before
being used anywhere, triggering the no-useless-assignment lint error. Remove the
initial assignments of submittedCount = 0 and isLinked = false, and instead
declare these variables only when they are actually assigned with their final
values in the conditional branches that follow. This ensures each variable is
assigned exactly once with its meaningful value.
In `@src/utils/classComparisonAggregator.ts`:
- Around line 21-24: The Insight interface currently includes a hardcoded
message string field, but messages are being built as English text in the
classComparisonAggregator at lines 126, 134, and 159. Replace the message field
in the Insight interface with translationKey and params fields (or similar
structure), then update all three locations where insights are created to return
the translation key and any necessary parameters instead of building English
strings. The actual localization should be done in StatisticsPage using the
useTranslation hook and t(...) function to translate based on the key and
params.
- Around line 38-42: The studentRubrics filtering operation and similar
operations throughout the aggregators repeatedly call students.find() in nested
loops, causing linear lookups that scale poorly with large cohorts. Pre-index
the students array into a Map keyed by student ID before entering the filter
operations (at line 38-42 and similar locations at lines 82-86 and 99-103), then
replace each students.find((s) => s.id === sr.studentId) call with a direct Map
lookup to achieve O(1) performance instead of O(n) for each iteration.
---
Outside diff comments:
In `@README.md`:
- Around line 46-56: The Analytics & Reporting section in the Features section
of README.md is incomplete. The Statistics dashboard bullet point (line 47)
needs to be expanded to include the new multi-class comparison capabilities such
as grouped averages, criterion gaps, trend overlay, and insights panel with
struggling-class and weak-criteria detection. Additionally, add a new bullet
point to describe the Activity Dashboard feature (new in phase 4.2) which
provides a grid view of rubrics/tests/essays against classes with link/unlink
and assign actions. Ensure these additions appear in the Features section (lines
5-74) to maintain documentation consistency with the Routes table that already
lists these new routes.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c2bd0457-fbb7-4e30-8521-6b60116ba48e
📒 Files selected for processing (19)
.gitignoreREADME.mde2e/specs/22-statistics-compare.spec.tssrc/App.tsxsrc/components/Layout/Sidebar.tsxsrc/components/Statistics/MultiClassTrendChart.tsxsrc/locales/de.jsonsrc/locales/en.jsonsrc/locales/es.jsonsrc/locales/fr.jsonsrc/locales/nl.jsonsrc/pages/ActivityDashboardPage.tsxsrc/pages/DocsPage.tsxsrc/pages/LandingPage.tsxsrc/pages/StatisticsPage.tsxsrc/utils/activityDashboardAggregator.test.tssrc/utils/activityDashboardAggregator.tssrc/utils/classComparisonAggregator.test.tssrc/utils/classComparisonAggregator.ts
Comment on lines
+76
to
+95
| test('class averages reflect grade data (A > B > C)', async ({ appPage }) => { | ||
| await appPage.goto('/#/statistics'); | ||
| await appPage.getByRole('button', { name: /compare/i }).click(); | ||
|
|
||
| await appPage.getByLabel('Class A').check(); | ||
| await appPage.getByLabel('Class B').check(); | ||
| await appPage.getByLabel('Class C').check(); | ||
|
|
||
| // Recharts renders tick labels with the avg value — Class A should show 100 | ||
| // and Class C should show 25. We check for the presence of "100" and "25" | ||
| // somewhere in the chart region (the tooltip formatter shows "X%"). | ||
| // Simpler: just assert all three class names appear in the results section. | ||
| await expect(appPage.getByText('Class A')).toBeVisible({ timeout: 5_000 }); | ||
| await expect(appPage.getByText('Class B')).toBeVisible(); | ||
| await expect(appPage.getByText('Class C')).toBeVisible(); | ||
|
|
||
| // With 3 classes rendered, we should have 3 bar cells | ||
| const bars = appPage.locator('.recharts-bar-rectangle'); | ||
| await expect(bars).toHaveCount(3, { timeout: 5_000 }); | ||
| }); |
Contributor
There was a problem hiding this comment.
This test does not actually validate “A > B > C” averages.
The assertions only verify labels and bar count; incorrect average computation/order would still pass. Please assert the displayed values/order (or bar geometry) explicitly so the test guards the stated behavior.
🤖 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 `@e2e/specs/22-statistics-compare.spec.ts` around lines 76 - 95, The test
`class averages reflect grade data (A > B > C)` currently only verifies that
class labels appear and that three bars exist, but does not validate the actual
average ordering. Add explicit assertions after the bars check to verify either
the displayed numeric values (Class A should be 100, Class B should be between
them, Class C should be 25) or the bar heights/geometry to confirm they are in
descending order, ensuring the test actually guards against incorrect average
computation or bar ordering.
Comment on lines
+244
to
+252
| description: 'Grade distributions, per-criterion performance, class comparison, and trend analysis.', | ||
| color: '#64748b', | ||
| }, | ||
| { | ||
| path: '/activity-dashboard', | ||
| label: 'Activity Dashboard', | ||
| description: 'Grid of all rubrics, tests, and essays vs classes — link, assign, and monitor coverage at a glance.', | ||
| color: '#0ea5e9', | ||
| }, |
Contributor
There was a problem hiding this comment.
Move new/updated docs copy to translation keys instead of inline English.
Line 244, Line 249, Line 250, Line 995, and Line 1009+ introduce user-visible English strings directly in TSX/config objects. This bypasses locale files and will leave these new docs updates untranslated.
As per coding guidelines, "All user-visible strings must use the useTranslation hook and a key from src/locales/en.json. Never hardcode English text in JSX."
Also applies to: 995-1005, 1007-1018
🤖 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/DocsPage.tsx` around lines 244 - 252, User-visible English strings
are hardcoded directly in the DocsPage.tsx configuration objects instead of
being moved to translation keys. Extract all hardcoded strings including the
descriptions for the dashboard entries and the label 'Activity Dashboard' from
lines 244-252 and similar sections, add them as key-value pairs to
src/locales/en.json with descriptive key names, then replace the hardcoded
strings in the configuration objects with references to those keys using the
useTranslation hook. Ensure the component imports and uses the useTranslation
hook if not already present, and reference the translation keys consistently
throughout the affected sections.
Source: Coding guidelines
Comment on lines
+3
to
+15
| export type ActivityKind = 'rubric' | 'test' | 'essay'; | ||
|
|
||
| export interface ActivityRow { | ||
| kind: ActivityKind; | ||
| id: string; // rubricId, testId, or teacherKey | ||
| name: string; | ||
| } | ||
|
|
||
| export interface CellData { | ||
| submittedCount: number; | ||
| totalStudents: number; | ||
| isLinked: boolean; // rubric in Class.rubricIds; essay has ≥1 assignment in this class | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Move dashboard domain types to src/types/index.ts.
ActivityKind, ActivityRow, and CellData are domain-level shapes but are defined in this utility module. Centralizing them in src/types/index.ts keeps contracts consistent across page, utils, and tests.
As per coding guidelines, "All domain types must be defined in src/types/index.ts. Do not create ad-hoc inline types for data shapes already defined there."
🤖 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/utils/activityDashboardAggregator.ts` around lines 3 - 15, Move the three
type definitions ActivityKind, ActivityRow, and CellData from
src/utils/activityDashboardAggregator.ts to src/types/index.ts to maintain
centralized domain type definitions. After moving them, import these types back
into activityDashboardAggregator.ts from src/types/index.ts. This ensures all
domain-level shapes are defined in the centralized types location and remain
consistent across the codebase.
Source: Coding guidelines
Comment on lines
+52
to
+53
| matrix[activity.id] = {}; | ||
| for (const cls of classes) { |
Contributor
There was a problem hiding this comment.
Prevent matrix key collisions across activity kinds.
Using matrix[activity.id] can overwrite data when different kinds share the same ID (e.g., rubric id="r1" and test id="r1"). That corrupts dashboard cells and actions for one row with another row’s data.
💡 Suggested direction
+const getActivityKey = (activity: ActivityRow) => `${activity.kind}:${activity.id}`;
for (const activity of activities) {
- matrix[activity.id] = {};
+ const activityKey = getActivityKey(activity);
+ matrix[activityKey] = {};
for (const cls of classes) {
...
- matrix[activity.id][cls.id] = { submittedCount, totalStudents, isLinked };
+ matrix[activityKey][cls.id] = { submittedCount, totalStudents, isLinked };
}
}Then update lookups in src/pages/ActivityDashboardPage.tsx to use the same composite key.
Also applies to: 84-84
🤖 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/utils/activityDashboardAggregator.ts` around lines 52 - 53, The matrix
initialization using matrix[activity.id] as the key causes collisions when
different activity kinds share the same ID. Replace the single activity.id key
with a composite key that combines both the activity kind and ID (for example,
creating a unique key string from both values) in the
activityDashboardAggregator.ts file at the matrix initialization point and any
other locations where matrix keys are accessed. Additionally, update all
corresponding lookups in src/pages/ActivityDashboardPage.tsx to use the same
composite key format to maintain consistency across both files.
Comment on lines
+21
to
+24
| export interface Insight { | ||
| kind: 'struggling' | 'weak_criterion' | 'divergence'; | ||
| message: string; | ||
| } |
Contributor
There was a problem hiding this comment.
Replace hardcoded insight messages with translation keys/params.
Line 126, Line 134, and Line 159 build end-user English text in a utility module. This makes insights non-localizable and leaks presentation concerns into domain logic. Return a translation key + params from this aggregator, then localize in StatisticsPage using t(...).
Suggested refactor (utility-side contract)
export interface Insight {
kind: 'struggling' | 'weak_criterion' | 'divergence';
- message: string;
+ messageKey: string;
+ messageParams?: Record<string, string | number>;
}
...
insights.push({
kind: 'struggling',
- message: `${r.className} may need targeted support — class average: ${r.average.toFixed(1)}%.`,
+ messageKey: 'statistics.insights.struggling',
+ messageParams: { className: r.className, average: r.average.toFixed(1) },
});
...
insights.push({
kind: 'weak_criterion',
- message: `"${c.title}" is a consistent weak point in ${r.className} (${cAvg.toFixed(1)}% vs class avg ${r.average.toFixed(1)}%).`,
+ messageKey: 'statistics.insights.weak_criterion',
+ messageParams: {
+ criterion: c.title,
+ className: r.className,
+ criterionAvg: cAvg.toFixed(1),
+ classAvg: r.average.toFixed(1),
+ },
});
...
insights.push({
kind: 'divergence',
- message: `"${maxGapCritTitle}" shows the biggest gap: ${maxGapHigh} is ${maxGap.toFixed(0)} pp ahead of ${maxGapLow}.`,
+ messageKey: 'statistics.insights.divergence',
+ messageParams: {
+ criterion: maxGapCritTitle,
+ highClass: maxGapHigh,
+ gap: maxGap.toFixed(0),
+ lowClass: maxGapLow,
+ },
});As per coding guidelines, “All user-visible strings must use the useTranslation hook and a key from src/locales/en.json.”
Also applies to: 124-127, 133-135, 157-160
🤖 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/utils/classComparisonAggregator.ts` around lines 21 - 24, The Insight
interface currently includes a hardcoded message string field, but messages are
being built as English text in the classComparisonAggregator at lines 126, 134,
and 159. Replace the message field in the Insight interface with translationKey
and params fields (or similar structure), then update all three locations where
insights are created to return the translation key and any necessary parameters
instead of building English strings. The actual localization should be done in
StatisticsPage using the useTranslation hook and t(...) function to translate
based on the key and params.
Source: Coding guidelines
Comment on lines
+38
to
+42
| const srs = studentRubrics.filter( | ||
| (sr) => | ||
| sr.rubricId === rubricId && | ||
| students.find((s) => s.id === sr.studentId)?.classId === classId | ||
| ); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Pre-index students by id to remove repeated linear lookups.
students.find(...) is called in nested loops in both aggregators (Line 41, Line 85, Line 102). This scales poorly with large cohorts and repeated recomputations in compare mode.
Low-impact optimization
export function compareClasses(
...
): ClassComparisonResult[] {
+ const studentClassById = new Map(students.map((s) => [s.id, s.classId]));
return classIds
.map((classId) => {
...
const srs = studentRubrics.filter(
(sr) =>
sr.rubricId === rubricId &&
- students.find((s) => s.id === sr.studentId)?.classId === classId
+ studentClassById.get(sr.studentId) === classId
); export function buildMultiClassTrend(
...
): MultiTrendPoint[] {
+ const studentClassById = new Map(students.map((s) => [s.id, s.classId]));
const relevantRubrics = rubrics
.filter((r) =>
classIds.some((classId) =>
studentRubrics.some(
(sr) =>
sr.rubricId === r.id &&
- students.find((s) => s.id === sr.studentId)?.classId === classId
+ studentClassById.get(sr.studentId) === classId
)
)
)
...
sr.rubricId === r.id &&
- students.find((s) => s.id === sr.studentId)?.classId === classId
+ studentClassById.get(sr.studentId) === classId
)Also applies to: 82-86, 99-103
🤖 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/utils/classComparisonAggregator.ts` around lines 38 - 42, The
studentRubrics filtering operation and similar operations throughout the
aggregators repeatedly call students.find() in nested loops, causing linear
lookups that scale poorly with large cohorts. Pre-index the students array into
a Map keyed by student ID before entering the filter operations (at line 38-42
and similar locations at lines 82-86 and 99-103), then replace each
students.find((s) => s.id === sr.studentId) call with a direct Map lookup to
achieve O(1) performance instead of O(n) for each iteration.
… types, Map optimization, filter sync useEffect - Composite key `kind:id` in dashboard matrix to prevent collision when rubric and test share the same id - Move ActivityKind/ActivityRow/CellData/Insight to src/types/index.ts - Insight interface uses messageKey+messageParams for i18n; StatisticsPage renders via t() - Pre-index students with Map for O(1) lookup in compareClasses and buildMultiClassTrend - Replace hardcoded VO track options with VO_TRACKS/VO_TRACK_LABELS in ActivityDashboardPage and StatisticsPage - useEffect for filter sync placed after state declarations to avoid temporal dead zone - Add 3-class compare e2e fixture with 2 rubrics to satisfy trend chart ≥2 points guard - Update all tests to use composite matrix keys and messageKey/messageParams assertions Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
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.
Summary
Class.voTrackandClass.year/activity-dashboard): rubric × test × essay row grid against class columns with Link/Unlink, Assign All, and Open cell actionsclassComparisonAggregator.ts+activityDashboardAggregator.ts, each with 43 unit tests (incl. stress datasets)MultiClassTrendChart(Recharts LineChart, one<Line>per class with auto-colour cycle)22-statistics-compare.spec.tsseeds 3 classes at distinct performance levels and asserts bar chart, class names, bar count, insights panel, and trend linestatistics.compare.*,statistics.insights.*,statistics.filters.*,activityDashboard.*)Test plan
npm run typecheck— zero errorsnpm test -- --run— all unit tests pass (incl. 86 new tests)npx playwright test e2e/specs/22-statistics-compare.spec.ts— 5 tests × 3 browsers pass🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests