feat: support new case study elements on activity evaluation for asynchronous activities with scatter plots #4492
Conversation
…nfusions with participant evaluation types
… and implement backend computation logic
|
Warning Rate limit exceeded@sjschlapbach has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request refactors evaluation data handling throughout the codebase. It updates several React components and shared hooks to replace legacy “element instance evaluation” type checks and interface definitions with new “activity evaluation” types. In addition, the GraphQL schema and services are revised, with new functions and interfaces added to support case study evaluations, and test data updated accordingly. These changes ensure consistency in conditional rendering and type validation across frontend components, GraphQL definitions, and seed data without altering external functionality. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
dosubot
bot
added
size:XL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (4)
apps/frontend-manage/src/components/evaluation/ElementChart.tsx (1)
14-14: Update the type ofinstanceEvaluationprop to align with new activity evaluation types.The prop type still uses the legacy
ElementInstanceEvaluationtype while the implementation has been updated to use activity evaluation types.- instanceEvaluation: ElementInstanceEvaluation + instanceEvaluation: ActivityEvaluationDataapps/frontend-manage/src/components/evaluation/ElementEvaluation.tsx (1)
15-15: Update the type ofcurrentInstanceprop to align with new activity evaluation types.The prop type still uses the legacy
ElementInstanceEvaluationtype while the implementation has been updated to use activity evaluation types.- currentInstance: ElementInstanceEvaluation + currentInstance: ActivityEvaluationDatapackages/shared-components/src/charts/ElementBarChart.tsx (2)
25-25: Update the type ofinstanceprop to align with new activity evaluation types.The prop type still uses the legacy
ElementInstanceEvaluationtype while the implementation has been updated to use activity evaluation types.- instance: ElementInstanceEvaluation + instance: ActivityEvaluationData
98-99: Animation workaround detected in ElementBarChart.tsx requires further investigation.The code explicitly disables animation on the Bar component using the hack:
// HACK: don't animate as it causes labels to disappear isAnimationActive={false}Our review of the context (lines 80–120) confirms that this workaround is still in place to prevent disappearing labels. No alternative solution or additional context addressing the underlying issue is present elsewhere in the repository. This indicates that the hack is a known, temporary fix for an unresolved issue with label rendering during animations.
• Action Item: Investigate if this issue has been resolved in the underlying charting library (likely Recharts) or if a custom solution can safely re-enable animations without causing labels to vanish.
🔗 Analysis chain
Consider addressing the animation HACK.
The comment indicates a workaround for disappearing labels. This should be properly investigated and fixed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar animation-related issues or workarounds rg -A 2 "isAnimationActive.*false.*// ?(HACK|TODO|FIXME)"Length of output: 59
Script:
#!/bin/bash # Extract additional context around the hack in ElementBarChart.tsx to better understand its usage. sed -n '80,120p' packages/shared-components/src/charts/ElementBarChart.tsxLength of output: 1344
🧹 Nitpick comments (2)
packages/graphql/src/schema/evaluation.ts (1)
141-174: Add JSDoc comments to document the complex structure.While the interface structure is well-organized, adding JSDoc comments would improve understanding of:
- The relationship between cases, items, and criteria
- The purpose of each nesting level
- The meaning of statistics in this context
Example improvement:
+/** + * Represents evaluation results for a case study element. + * The structure follows a hierarchical pattern: + * - Cases: Different scenarios for evaluation + * - Items: Individual elements to be evaluated + * - Criteria: Specific aspects to evaluate for each item + */ export interface ICaseStudyElementEvaluationResults { totalAnswers: number anonymousAnswers: numberpackages/prisma/src/data/data/TEST.ts (1)
493-584: LGTM! Comprehensive test data for single-criterion case study.The new test case provides good coverage for single-criterion evaluation scenarios. The sample solutions are well-defined and cover both general and gourmet cases with realistic value ranges.
However, consider adding test cases for edge cases:
- Minimum/maximum boundary values (1 and 10)
- Equal min/max values
- Values near the step boundaries
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
packages/graphql/src/graphql/ops/FEvaluationResults.graphqlis excluded by!**/**/graphql/ops/**packages/graphql/src/ops.schema.jsonis excluded by!**/**/ops.schema.jsonpackages/graphql/src/ops.tsis excluded by!**/**/ops.tspackages/graphql/src/public/client.jsonis excluded by!**/**/public/**packages/graphql/src/public/schema.graphqlis excluded by!**/**/public/**packages/graphql/src/public/server.jsonis excluded by!**/**/public/**
📒 Files selected for processing (18)
apps/frontend-manage/src/components/evaluation/ElementChart.tsx(1 hunks)apps/frontend-manage/src/components/evaluation/ElementEvaluation.tsx(4 hunks)apps/frontend-manage/src/components/evaluation/elements/CTEvaluation.tsx(1 hunks)apps/frontend-manage/src/components/evaluation/elements/ChoicesEvaluation.tsx(2 hunks)apps/frontend-manage/src/components/evaluation/elements/ChoicesSidebar.tsx(2 hunks)apps/frontend-manage/src/components/evaluation/elements/FCEvaluation.tsx(1 hunks)apps/frontend-manage/src/components/evaluation/elements/FTEvaluation.tsx(2 hunks)apps/frontend-manage/src/components/evaluation/elements/FTSidebar.tsx(2 hunks)apps/frontend-manage/src/components/evaluation/elements/NREvaluation.tsx(2 hunks)apps/frontend-manage/src/components/evaluation/elements/NRSidebar.tsx(2 hunks)apps/frontend-manage/src/components/evaluation/elements/SEEvaluation.tsx(1 hunks)packages/graphql/src/schema/evaluation.ts(16 hunks)packages/graphql/src/services/stacks.ts(5 hunks)packages/prisma/src/data/data/TEST.ts(1 hunks)packages/shared-components/src/charts/ElementBarChart.tsx(1 hunks)packages/shared-components/src/charts/ElementWordcloud.tsx(1 hunks)packages/shared-components/src/hooks/useEvaluationBarChartData.ts(1 hunks)packages/shared-components/src/hooks/useEvaluationTableData.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run
🔇 Additional comments (26)
apps/frontend-manage/src/components/evaluation/elements/NRSidebar.tsx (3)
1-1: LGTM! Import change aligns with the refactoring effort.The updated import of
NumericalActivityEvaluationDatafrom the GraphQL operations is consistent with the broader initiative to replace legacy evaluation types.
27-136: LGTM! Component implementation remains robust.The component's usage of the instance prop remains unchanged and includes proper null checks and fallbacks for all accessed fields. The implementation demonstrates good defensive programming practices.
17-25: Verify type compatibility with component usage.The interface update from
NumericalElementInstanceEvaluationtoNumericalActivityEvaluationDatalooks good, but let's verify that all required fields are still available in the new type.✅ Verification successful
Verified type compatibility with component usage.
The new type,
NumericalActivityEvaluationData, includes all the required fields (such ascontent,explanation,hasAnswerFeedbacks,results, and optionallystatistics) expected by the component. Its usage throughout the codebase (e.g., checks againstinstance.__typename === 'NumericalActivityEvaluationData'and mapping overresults.responseValues) confirms that the transition fromNumericalElementInstanceEvaluationis compatible.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the structure of both types to ensure compatibility # Search for type definitions ast-grep --pattern 'type NumericalElementInstanceEvaluation { $$$ }' ast-grep --pattern 'type NumericalActivityEvaluationData { $$$ }'Length of output: 142
Script:
#!/bin/bash # Gather detailed context for both type names to manually verify their structure and compatibility. echo "Searching for occurrences of 'NumericalElementInstanceEvaluation' with context:" rg -n "NumericalElementInstanceEvaluation" -C 3 echo "------------------------------------------------------------------" echo "Searching for occurrences of 'NumericalActivityEvaluationData' with context:" rg -n "NumericalActivityEvaluationData" -C 3Length of output: 105666
packages/graphql/src/services/stacks.ts (4)
3410-3493: Check correctness merging with nullish coalescing.
When merging thecorrectfield with the nullish coalescing operator (??), if an existing correctness value isfalse, the new correctness value won't override it. Consider verifying whether a logical OR might better represent the intended combined correctness.
3632-3655: Implementation appears consistent.
The computed evaluation structure for case study elements aligns well with the rest of the file.
3702-3705: Function definition is clear.
No issues found in the function signature or structure.
3775-3785: Seamless extension for CASE_STUDY handling.
Inclusion ofcomputeCaseStudyEvaluationmatches the existing evaluation logic.apps/frontend-manage/src/components/evaluation/elements/CTEvaluation.tsx (2)
1-1: Import update is correct.
Switching toContentActivityEvaluationDataaligns with the new activity-based model.
6-6: Updated interface type.
ReplacingContentElementInstanceEvaluationwithContentActivityEvaluationDataaccurately reflects the new activity evaluation structure.apps/frontend-manage/src/components/evaluation/elements/FCEvaluation.tsx (2)
1-1: Import update is consistent.
UsingFlashcardActivityEvaluationDatasupports the ongoing migration to activity-based evaluations.
6-6: Interface property realigned.
Shifting fromFlashcardElementInstanceEvaluationtoFlashcardActivityEvaluationDatakeeps it consistent with the new data model.apps/frontend-manage/src/components/evaluation/elements/SEEvaluation.tsx (1)
1-1: LGTM! Type update aligns with activity evaluation refactoring.The change from
SelectionElementInstanceEvaluationtoSelectionActivityEvaluationDatais consistent with the PR objective of refactoring evaluation data handling.Also applies to: 7-7
packages/shared-components/src/hooks/useEvaluationBarChartData.ts (1)
13-13: LGTM! Type check update aligns with activity evaluation refactoring.The change from
ChoicesElementInstanceEvaluationtoChoicesActivityEvaluationDatais consistent with the PR objective of refactoring evaluation data handling.apps/frontend-manage/src/components/evaluation/elements/FTSidebar.tsx (1)
1-1: LGTM! Type update aligns with activity evaluation refactoring.The change from
FreeElementInstanceEvaluationtoFreeTextActivityEvaluationDatais consistent with the PR objective of refactoring evaluation data handling.Also applies to: 10-10
packages/shared-components/src/charts/ElementWordcloud.tsx (1)
28-28: LGTM! Type check updates align with activity evaluation refactoring.The changes from
NumericalElementInstanceEvaluationtoNumericalActivityEvaluationDataand fromFreeElementInstanceEvaluationtoFreeTextActivityEvaluationDataare consistent with the PR objective of refactoring evaluation data handling.Also applies to: 33-33
apps/frontend-manage/src/components/evaluation/elements/ChoicesEvaluation.tsx (1)
1-1: LGTM! Clean type migration.The changes correctly update the import and prop type to use the new
ChoicesActivityEvaluationDatatype, aligning with the broader refactoring effort.Also applies to: 16-16
packages/shared-components/src/hooks/useEvaluationTableData.ts (2)
22-24: Address TODO comment about numerical evaluation.The comment indicates a potential issue with duplicate numerical values being treated differently. This should be investigated and fixed to ensure accurate evaluation results.
Would you like me to help investigate and propose a fix for the duplicate numbers issue?
11-11: LGTM! Consistent type migration.The type checks have been correctly updated to use the new activity evaluation data types while maintaining the same data mapping logic.
Also applies to: 22-22, 35-35, 46-46
apps/frontend-manage/src/components/evaluation/elements/NREvaluation.tsx (1)
1-1: LGTM! Clean type migration.The changes correctly update the import and prop type to use the new
NumericalActivityEvaluationDatatype while maintaining the component's functionality.Also applies to: 17-17
apps/frontend-manage/src/components/evaluation/elements/FTEvaluation.tsx (1)
1-1: LGTM! Clean type migration.The changes correctly update the import and prop type to use the new
FreeTextActivityEvaluationDatatype while maintaining the component's functionality.Also applies to: 17-17
apps/frontend-manage/src/components/evaluation/ElementChart.tsx (1)
54-55: LGTM! Type check updated correctly.The type check has been properly updated to use the new activity evaluation type, maintaining consistency with the broader refactoring effort.
apps/frontend-manage/src/components/evaluation/ElementEvaluation.tsx (1)
43-83: LGTM! Type checks updated consistently.All conditional type checks have been properly updated to use the new activity evaluation types, maintaining consistency across different evaluation types.
apps/frontend-manage/src/components/evaluation/elements/ChoicesSidebar.tsx (1)
1-1: LGTM! Import and type definitions updated correctly.The changes consistently update both the import statement and the interface definition to use the new activity evaluation type.
Also applies to: 11-11
packages/shared-components/src/charts/ElementBarChart.tsx (1)
119-131: LGTM! Type check updated correctly.The type check has been properly updated to use the new activity evaluation type, maintaining consistency with the broader refactoring effort.
packages/graphql/src/schema/evaluation.ts (2)
67-70: LGTM! Improved naming convention enhances code clarity.The renaming of interfaces from
*ElementInstanceEvaluationto*ActivityEvaluationDatabetter reflects their purpose and aligns with the domain language. The consistent naming pattern across all evaluation types improves code maintainability.Also applies to: 91-105, 119-122, 136-139, 176-179, 189-192, 199-202
423-514: LGTM! Well-structured GraphQL type implementation.The GraphQL type definitions for case study evaluations are complete and follow the established patterns. The field definitions correctly match the TypeScript interfaces, and the builder pattern is used consistently.
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
async-activity-evaluation
|
| Run status |
|
| Run duration | 22m 42s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
325
|
| View all changes introduced in this branch ↗︎ | |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (13)
packages/shared-components/src/constants.ts (1)
124-130: Consider reordering chart types for consistency.For consistency with other element types in
ACTIVE_CHART_TYPES, consider listing the histogram before the scatter plot, as histogram is a more common visualization type that appears first in other element type entries.[ElementType.CaseStudy]: [ - { label: 'manage.evaluation.scatterPlot', value: ChartType.SCATTER }, - { - label: 'manage.evaluation.histogram', - value: ChartType.HISTOGRAM, - }, + { + label: 'manage.evaluation.histogram', + value: ChartType.HISTOGRAM, + }, + { label: 'manage.evaluation.scatterPlot', value: ChartType.SCATTER }, ],apps/frontend-manage/src/components/evaluation/elements/CSEvaluationHistogram.tsx (1)
10-18: Props interface is well-structured but could be enhanced.The interface is comprehensive but could benefit from JSDoc documentation explaining the purpose of each prop.
Consider adding JSDoc comments:
interface CSEvaluationHistogramProps { + /** Evaluation results object containing the data to be visualized */ results: CSResultsEvaluationObject + /** Array of case study information */ cases: CaseStudyElementResultCaseInfo[] + /** Array of item information for the case study */ items: CaseStudyElementResultItemInfo[] + /** Text size configuration for the chart */ textSize: TextSizeType + /** Type of chart to be rendered */ chartType: ChartType + /** Flag to control solution visibility */ showSolution: boolean + /** Type of activity evaluation */ type: ActivityEvaluationType }apps/frontend-manage/src/components/evaluation/elements/QuestionCollapsible.tsx (1)
67-82: Enhance button accessibility.The collapse/expand button could benefit from improved accessibility attributes.
Add ARIA attributes to improve accessibility:
<Button className={{ root: twMerge( 'hover:bg-primary-20 hidden h-4 w-full rounded-none border-0 text-center text-xs shadow-none hover:bg-none md:block print:hidden', questionCollapsed && 'bg-gradient-to-b from-white to-slate-100' ), }} onClick={() => setQuestionCollapsed(!questionCollapsed)} data={{ cy: 'toggle-question-collapse-evaluation' }} + aria-expanded={!questionCollapsed} + aria-label={questionCollapsed ? 'Expand question' : 'Collapse question'} >apps/frontend-manage/src/components/evaluation/elements/CSTwoDimScatterPlot.tsx (2)
60-60: Optimize criteria lookup performance.Multiple
findoperations on criteria array could be optimized.Consider creating a criteria lookup map:
+const criteriaMap = useMemo( + () => Object.fromEntries(criteria.map((c) => [c.id, c])), + [criteria] +) // Then replace find operations with map lookups -value: criteria.find((c) => c.id === xCriterion)?.name, +value: criteriaMap[xCriterion]?.name, -value: criteria.find((c) => c.id === yCriterion)?.name, +value: criteriaMap[yCriterion]?.name,Also applies to: 71-71
78-92: Enhance tooltip accessibility.The tooltip could benefit from ARIA attributes and keyboard navigation support.
Consider adding accessibility attributes:
<div className="rounded-md border-2 border-black bg-white p-2" + role="tooltip" + aria-live="polite" >apps/frontend-manage/src/components/evaluation/elements/CSEvaluation.tsx (4)
15-23: Add documentation for the complex type definition.Consider adding JSDoc comments to explain the structure and purpose of the CSResultsEvaluationObject type, as it represents a complex nested data structure.
+/** + * Represents the hierarchical structure of case study evaluation results + * @property caseId - Unique identifier for a case + * @property itemId - Unique identifier for an item within a case + * @property criterionId - Unique identifier for a criterion within an item + */ export type CSResultsEvaluationObject = { [caseId: string]: { [itemId: string]: {
44-65: Improve readability of the complex data transformation.Consider extracting the nested reduce operations into separate helper functions for better readability and maintainability.
+const transformCriteria = (criteria: any[]) => + criteria.reduce((critAcc, critResult) => { + critAcc[critResult.criterionId] = critResult + return critAcc + }, {}) +const transformItems = (items: any[]) => + items.reduce((itemAcc, itemResult) => { + itemAcc[String(itemResult.itemId)] = transformCriteria(itemResult.criteria) + return itemAcc + }, {}) const resultsObject = useMemo( () => instanceEvaluation.results.caseResults.reduce<CSResultsEvaluationObject>( (caseAcc, caseResult) => { - caseAcc[caseResult.caseId] = caseResult.items.reduce< - CSResultsEvaluationObject[string] - >((itemAcc, itemResult) => { - itemAcc[String(itemResult.itemId)] = itemResult.criteria.reduce< - CSResultsEvaluationObject[string][string] - >((critAcc, critResult) => { - critAcc[critResult.criterionId] = critResult - return critAcc - }, {}) - return itemAcc - }, {}) + caseAcc[caseResult.caseId] = transformItems(caseResult.items) return caseAcc }, {} ), [instanceEvaluation.results.caseResults] )
69-114: Enhance tab accessibility.Consider adding ARIA labels and keyboard navigation improvements to the tabs for better accessibility.
<Tabs defaultValue="instructions"> <Tabs.TabList className={{ root: 'h-8 px-0 py-0', }} + aria-label="Case study navigation" > <Tabs.Tab value="instructions" className={{ label: textSize.text, root: 'px-0 py-0', }} + aria-controls="instructions-panel" >
115-138: Add explicit handling for unsupported chart types.Consider adding an else case to handle unsupported chart types gracefully.
{chartType === ChartType.SCATTER && ( <CSEvaluationScatter evaluationId={instanceEvaluation.id} results={resultsObject} cases={instanceEvaluation.cases} items={instanceEvaluation.items} criteria={instanceEvaluation.criteria} textSize={textSize} chartType={chartType} showSolution={showSolution} type={type} /> )} + {chartType !== ChartType.HISTOGRAM && chartType !== ChartType.SCATTER && ( + <UserNotification + type="warning" + message={t('manage.evaluation.unsupportedChartType')} + /> + )}packages/graphql/src/services/stacks.ts (1)
3518-3541: Consider adding input validation for numerical statistics.The
computeNumericalStatisticsfunction should validate its input to prevent potential issues with empty or invalid arrays.function computeNumericalStatistics( results: { value: number count: number correct?: boolean | null }[] ) { + if (!Array.isArray(results) || results.length === 0) { + return null; + } + const valueArray = results.reduce<number[]>((acc, { count, value }) => { + if (typeof value !== 'number' || typeof count !== 'number' || count <= 0) { + return acc; + } const elements = Array(count).fill(value) return acc.concat(elements) }, []) return valueArray.length > 0apps/frontend-manage/src/components/evaluation/elements/CSEvaluationScatter.tsx (2)
103-107: Address TODO comments and implement 1-dimensional plot.The TODO comments and placeholder implementation for the 1-dimensional plot need to be addressed.
Would you like me to help implement the 1-dimensional plot component using Recharts?
71-81: Consider memoizing criteria initialization.The criteria initialization effect runs on every criteria change. Consider using
useMemofor better performance, especially with large datasets.- useEffect(() => { + const initialCriteria = useMemo(() => { if (criteria.length > 0) { - setXCriterion(String(criteria[0].id)) + return { + x: String(criteria[0].id), + y: criteria.length > 1 ? String(criteria[1].id) : null, + } } - if (criteria.length > 1) { - setYCriterion(String(criteria[1].id)) - } else { - setYCriterion(null) + return { + x: null, + y: null, } - }, [criteria]) + }, [criteria]) + useEffect(() => { + setXCriterion(initialCriteria.x) + setYCriterion(initialCriteria.y) + }, [initialCriteria])apps/frontend-manage/src/components/evaluation/elements/CSEvaluationScatterSidebar.tsx (1)
49-56: Consider extracting case selection logic.The case selection logic in the onCheck handler could be simplified and extracted for better maintainability.
+ const toggleCaseSelection = (caseId: string) => { + setSelectedCases((prev) => + prev.includes(caseId) + ? prev.filter((id) => id !== caseId) + : [...prev, caseId] + ) + } <Checkbox checked={selectedCases.includes(caseItem.id)} - onCheck={() => - setSelectedCases((prev) => - prev.includes(caseItem.id) - ? prev.filter((id) => id !== caseItem.id) - : [...prev, caseItem.id] - ) - } + onCheck={() => toggleCaseSelection(caseItem.id)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
packages/graphql/src/graphql/ops/FEvaluationResults.graphqlis excluded by!**/**/graphql/ops/**packages/graphql/src/ops.schema.jsonis excluded by!**/**/ops.schema.jsonpackages/graphql/src/ops.tsis excluded by!**/**/ops.tspackages/graphql/src/public/client.jsonis excluded by!**/**/public/**packages/graphql/src/public/schema.graphqlis excluded by!**/**/public/**packages/graphql/src/public/server.jsonis excluded by!**/**/public/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
apps/auth/package.json(1 hunks)apps/backend-docker/package.json(1 hunks)apps/docs/package.json(1 hunks)apps/frontend-control/package.json(1 hunks)apps/frontend-manage/package.json(1 hunks)apps/frontend-manage/src/components/evaluation/ElementEvaluation.tsx(5 hunks)apps/frontend-manage/src/components/evaluation/elements/CSEvaluation.tsx(1 hunks)apps/frontend-manage/src/components/evaluation/elements/CSEvaluationHistogram.tsx(1 hunks)apps/frontend-manage/src/components/evaluation/elements/CSEvaluationScatter.tsx(1 hunks)apps/frontend-manage/src/components/evaluation/elements/CSEvaluationScatterSidebar.tsx(1 hunks)apps/frontend-manage/src/components/evaluation/elements/CSTwoDimScatterPlot.tsx(1 hunks)apps/frontend-manage/src/components/evaluation/elements/QuestionCollapsible.tsx(2 hunks)apps/frontend-manage/src/components/evaluation/hooks/useCaseStudyScatterData.tsx(1 hunks)apps/frontend-pwa/package.json(1 hunks)apps/func-incoming-responses/package.json(1 hunks)apps/func-response-processor/package.json(1 hunks)apps/office-addin/package.json(1 hunks)packages/graphql/src/schema/evaluation.ts(16 hunks)packages/graphql/src/services/stacks.ts(5 hunks)packages/i18n/messages/de.ts(2 hunks)packages/i18n/messages/en.ts(2 hunks)packages/markdown/package.json(1 hunks)packages/prisma/src/data/data/TEST.ts(1 hunks)packages/shared-components/package.json(1 hunks)packages/shared-components/src/constants.ts(3 hunks)packages/transactional/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (10)
- packages/shared-components/package.json
- apps/frontend-pwa/package.json
- apps/office-addin/package.json
- apps/func-incoming-responses/package.json
- apps/func-response-processor/package.json
- packages/transactional/package.json
- apps/backend-docker/package.json
- apps/frontend-manage/package.json
- apps/auth/package.json
- apps/frontend-control/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/prisma/src/data/data/TEST.ts
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
- GitHub Check: Analyze (javascript)
- GitHub Check: cypress-run
- GitHub Check: build
- GitHub Check: check
- GitHub Check: SonarCloud
🔇 Additional comments (20)
apps/docs/package.json (1)
25-25: Dependency Version Upgrade for @uzh-bf/design-system
ThedevDependenciesfield now correctly specifies version"3.0.0-alpha.56"for@uzh-bf/design-system. Please verify that this upgrade is compatible across modules and that all necessary regression tests have been executed.packages/markdown/package.json (1)
43-43: Peer Dependency Update for @uzh-bf/design-system
The peer dependency version for@uzh-bf/design-systemhas been updated to"3.0.0-alpha.56", ensuring consistency with the rest of the repository. Make sure that consumers of this package are aware of this update to avoid any version mismatch issues.packages/shared-components/src/constants.ts (1)
85-85: LGTM!The new
SCATTERchart type and its corresponding label follow consistent naming patterns.Also applies to: 94-94
apps/frontend-manage/src/components/evaluation/elements/CSEvaluationHistogram.tsx (1)
20-30: Component implementation is incomplete.The component currently only renders a placeholder text "CASE STUDY HISTOGRAM EVALUATION" despite receiving comprehensive props for histogram visualization. Consider implementing the histogram visualization using the provided props.
Would you like assistance in implementing the histogram visualization using a charting library like recharts?
apps/frontend-manage/src/components/evaluation/hooks/useCaseStudyScatterData.tsx (1)
83-93: Potential edge case in axis limits calculation.The code assumes the first case and item will always have valid min/max values.
Consider adding validation:
return { scatterData: data, - xLower: results[selectedCases[0]][items[0].id][xCriterion]?.min, - xUpper: results[selectedCases[0]][items[0].id][xCriterion]?.max, + xLower: results[selectedCases[0]]?.[items[0].id]?.[xCriterion]?.min ?? 0, + xUpper: results[selectedCases[0]]?.[items[0].id]?.[xCriterion]?.max ?? 100, yLower: yCriterion - ? results[selectedCases[0]][items[0].id][yCriterion]?.min + ? results[selectedCases[0]]?.[items[0].id]?.[yCriterion]?.min ?? 0 : 0, yUpper: yCriterion - ? results[selectedCases[0]][items[0].id][yCriterion]?.max + ? results[selectedCases[0]]?.[items[0].id]?.[yCriterion]?.max ?? 100 : 0, }apps/frontend-manage/src/components/evaluation/ElementEvaluation.tsx (3)
5-5: LGTM!Clean import of the new CSEvaluation component.
36-44: LGTM!Good conditional rendering to handle case study content differently. The content prop is correctly passed to QuestionCollapsible.
46-96: LGTM!Clean implementation of type checks for different activity evaluation types. The CSEvaluation component is properly integrated with all required props.
packages/graphql/src/schema/evaluation.ts (4)
67-70: LGTM! Well-structured interfaces for activity evaluation data.The interfaces are well-designed with clear separation of concerns:
- Base interface extends
IElementInstanceEvaluation- Consistent naming convention across all activity types
- Proper type definitions for results and statistics
Also applies to: 91-105, 119-122, 136-139, 171-187
141-169: LGTM! Comprehensive case study evaluation results interface.The
ICaseStudyElementEvaluationResultsinterface is well-structured with:
- Clear separation of case, item, and criteria data
- Proper handling of optional fields
- Support for statistics and responses
259-271: LGTM! Well-implemented statistics object reference.The
Statisticsobject reference implementation:
- Exposes all required statistical fields
- Uses appropriate float type for numerical values
- Maintains consistency with other object references
664-677: LGTM! Updated union type with case study support.The
ElementInstanceEvaluationunion type:
- Includes all activity evaluation data types
- Maintains consistent naming convention
- Properly handles type resolution
packages/graphql/src/services/stacks.ts (2)
3409-3488: LGTM! Well-implemented case study results combination.The
combineCaseStudyResultsfunction:
- Properly merges results and anonymous results
- Handles optional fields appropriately
- Maintains type safety
3627-3664: LGTM! Comprehensive case study evaluation computation.The
computeCaseStudyEvaluationfunction:
- Properly structures case study data
- Maintains consistency with other evaluation types
- Handles all required fields
packages/i18n/messages/en.ts (4)
1687-1687: LGTM: New scatter plot visualization label added.The addition of the
scatterPlotlabel enables support for scatter plot visualization in the UI.
1701-1702: LGTM: Added axis labels for scatter plot visualization.The addition of
criterionXAxisandcriterionYAxislabels provides clear axis labeling for scatter plot visualizations.
1703-1703: LGTM: Added aggregation label.The addition of the
aggregationlabel supports data aggregation functionality in the UI.
1704-1705: LGTM: Added validation message for case study evaluation.The addition of
caseStudySelectCasesCriteriaprovides a clear validation message requiring at least one case and corresponding criteria for displaying the evaluation.packages/i18n/messages/de.ts (2)
1711-1727: LGTM! The new translations for scatter plot and criteria are accurate.The German translations for scatter plot visualization and axis criteria are clear, concise, and consistent with technical terminology.
1728-1729: LGTM! The case study selection criteria translation provides clear user guidance.The German translation effectively communicates the requirement to select at least one case and corresponding criteria to view the evaluation.
apps/frontend-manage/src/components/evaluation/hooks/useCaseStudyScatterData.tsx
Outdated
Show resolved
Hide resolved
apps/frontend-manage/src/components/evaluation/elements/CSTwoDimScatterPlot.tsx
Show resolved
Hide resolved
apps/frontend-manage/src/components/evaluation/elements/CSEvaluationScatter.tsx
Outdated
Show resolved
Hide resolved
apps/frontend-manage/src/components/evaluation/elements/CSEvaluationScatter.tsx
Show resolved
Hide resolved
apps/frontend-manage/src/components/evaluation/elements/CSEvaluationScatterSidebar.tsx
Show resolved
Hide resolved
apps/frontend-manage/src/components/evaluation/elements/CSEvaluationScatterSidebar.tsx
Outdated
Show resolved
Hide resolved
…ensional scatter plotting
dosubot
bot
added
size:XXL
sjschlapbach
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
apps/frontend-manage/src/components/evaluation/elements/CSTwoDimScatterPlot.tsx (1)
102-104:⚠️ Potential issueFix type safety issue in shape prop.
The shape prop uses
anytype which should be avoided for better type safety.-shape={(props: any) => ( +shape={(props: { cx: number; cy: number; fill: string }) => ( <circle cx={props.cx} cy={props.cy} r={6} fill={props.fill} /> )}apps/frontend-manage/src/components/evaluation/elements/CSEvaluationScatterSidebar.tsx (1)
143-143:⚠️ Potential issueAdd validation for aggregation type.
The aggregation type change handler doesn't validate the input value against the enum.
-onChange={(value) => setAggregationType(value as AggregationType)} +onChange={(value) => { + if (Object.values(AggregationType).includes(value as AggregationType)) { + setAggregationType(value as AggregationType) + } +}}
🧹 Nitpick comments (8)
apps/frontend-manage/src/components/evaluation/elements/CSOneDimScatterPlot.tsx (3)
39-39: Consider using dynamic height based on data points.The fixed height of "50%" might not be optimal for all data sets.
-<ResponsiveContainer width="99%" height="50%"> +<ResponsiveContainer width="99%" height={Math.max(300, selectedCases.length * 50)}>
39-39: Consider making the height prop configurable.The height is hardcoded to "50%", which might not be suitable for all use cases.
-<ResponsiveContainer width="99%" height="50%"> +<ResponsiveContainer width="99%" height={height ?? "50%"}>Add height to the props interface:
height?: string | number
88-92: Optimize data transformation.The data transformation in the scatter component can be memoized to prevent unnecessary recalculations on re-renders.
+const transformedData = useMemo( + () => + scatterData[caseId].map((data) => ({ + ...data, + x: data.x, + caseId: index, + })), + [scatterData, caseId, index] +) <Scatter key={caseId} name={cases[caseIx]?.name} - data={scatterData[caseId].map((data) => ({ - ...data, - x: data.x, - caseId: index, - }))} + data={transformedData} fill={CHART_COLORS[caseIx % 12]}apps/frontend-manage/src/components/evaluation/elements/CSEvaluation.tsx (3)
44-65: Consider extracting results transformation logic.The complex nested reduce operations could be moved to a separate utility function for better maintainability and reusability.
+const transformResults = (caseResults: CaseStudyElementResults['caseResults']) => + caseResults.reduce<CSResultsEvaluationObject>( + (caseAcc, caseResult) => { + caseAcc[caseResult.caseId] = caseResult.items.reduce< + CSResultsEvaluationObject[string] + >((itemAcc, itemResult) => { + itemAcc[String(itemResult.itemId)] = itemResult.criteria.reduce< + CSResultsEvaluationObject[string][string] + >((critAcc, critResult) => { + critAcc[critResult.criterionId] = critResult + return critAcc + }, {}) + return itemAcc + }, {}) + return caseAcc + }, + {} + ) const resultsObject = useMemo( - () => - instanceEvaluation.results.caseResults.reduce<CSResultsEvaluationObject>( - // ... existing reduce logic - ), + () => transformResults(instanceEvaluation.results.caseResults), [instanceEvaluation.results.caseResults] )
45-65: Simplify nested reduce operations for better maintainability.The triple-nested reduce operations make the code hard to follow. Consider breaking it down into smaller, more manageable functions.
const resultsObject = useMemo(() => { + const createCriteriaMap = (criteria) => + criteria.reduce((acc, crit) => { + acc[crit.criterionId] = crit; + return acc; + }, {}); + + const createItemsMap = (items) => + items.reduce((acc, item) => { + acc[String(item.itemId)] = createCriteriaMap(item.criteria); + return acc; + }, {}); + + return instanceEvaluation.results.caseResults.reduce((acc, caseResult) => { + acc[caseResult.caseId] = createItemsMap(caseResult.items); + return acc; + }, {}); }, [instanceEvaluation.results.caseResults]);
45-65: Simplify nested reduce operations.The nested reduce operations can be made more readable by extracting helper functions and using object spread.
const resultsObject = useMemo( () => - instanceEvaluation.results.caseResults.reduce<CSResultsEvaluationObject>( - (caseAcc, caseResult) => { - caseAcc[caseResult.caseId] = caseResult.items.reduce< - CSResultsEvaluationObject[string] - >((itemAcc, itemResult) => { - itemAcc[String(itemResult.itemId)] = itemResult.criteria.reduce< - CSResultsEvaluationObject[string][string] - >((critAcc, critResult) => { - critAcc[critResult.criterionId] = critResult - return critAcc - }, {}) - return itemAcc - }, {}) - return caseAcc - }, - {} - ), + instanceEvaluation.results.caseResults.reduce((caseAcc, caseResult) => ({ + ...caseAcc, + [caseResult.caseId]: caseResult.items.reduce((itemAcc, itemResult) => ({ + ...itemAcc, + [String(itemResult.itemId)]: itemResult.criteria.reduce( + (critAcc, critResult) => ({ + ...critAcc, + [critResult.criterionId]: critResult, + }), + {} + ), + }), {}), + }), {} as CSResultsEvaluationObject), [instanceEvaluation.results.caseResults] )apps/frontend-manage/src/components/evaluation/elements/CSEvaluationScatterSidebar.tsx (2)
50-56: Consider using Set for better performance.When dealing with case selection, using a Set instead of an array would provide better performance for includes/filter operations.
-selectedCases: string[] +selectedCases: Set<string> -checked={selectedCases.includes(caseItem.id)} +checked={selectedCases.has(caseItem.id)} onCheck={() => setSelectedCases((prev) => - prev.includes(caseItem.id) - ? prev.filter((id) => id !== caseItem.id) - : [...prev, caseItem.id] + prev.has(caseItem.id) + ? new Set([...prev].filter((id) => id !== caseItem.id)) + : new Set([...prev, caseItem.id]) )
49-56: Optimize case selection state updates.The case selection logic could be simplified using Set operations for better performance.
-onCheck={() => - setSelectedCases((prev) => - prev.includes(caseItem.id) - ? prev.filter((id) => id !== caseItem.id) - : [...prev, caseItem.id] - ) -} +onCheck={() => { + setSelectedCases((prev) => { + const set = new Set(prev); + set.has(caseItem.id) ? set.delete(caseItem.id) : set.add(caseItem.id); + return Array.from(set); + }); +}}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/frontend-manage/src/components/evaluation/elements/CSEvaluation.tsx(1 hunks)apps/frontend-manage/src/components/evaluation/elements/CSEvaluationScatter.tsx(1 hunks)apps/frontend-manage/src/components/evaluation/elements/CSEvaluationScatterSidebar.tsx(1 hunks)apps/frontend-manage/src/components/evaluation/elements/CSOneDimScatterPlot.tsx(1 hunks)apps/frontend-manage/src/components/evaluation/elements/CSTwoDimScatterPlot.tsx(1 hunks)apps/frontend-manage/src/components/evaluation/hooks/useCaseStudyScatterData.tsx(1 hunks)packages/prisma/src/data/data/TEST.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/frontend-manage/src/components/evaluation/hooks/useCaseStudyScatterData.tsx
- packages/prisma/src/data/data/TEST.ts
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: cypress-run
- GitHub Check: SonarCloud
- GitHub Check: build
- GitHub Check: check
- GitHub Check: build
- GitHub Check: build
- GitHub Check: test
🔇 Additional comments (10)
apps/frontend-manage/src/components/evaluation/elements/CSTwoDimScatterPlot.tsx (2)
102-104: Fix type safety issue in shape prop.The shape prop uses
anytype which should be avoided for type safety.
102-104: Fix type safety issue in shape prop.The shape prop uses
anytype which should be avoided for type safety.apps/frontend-manage/src/components/evaluation/elements/CSEvaluationScatterSidebar.tsx (2)
143-143: Consider adding validation for aggregation type.The aggregation type change handler doesn't validate the input value against the enum.
143-143: Consider adding validation for aggregation type.The aggregation type change handler doesn't validate the input value against the enum.
apps/frontend-manage/src/components/evaluation/elements/CSEvaluationScatter.tsx (6)
1-24: LGTM! Well-organized imports.The imports are logically grouped and all dependencies are properly scoped.
25-39: LGTM! Clear and well-structured type definitions.The enum and type definitions are clear, with proper typing for optional values.
41-51: LGTM! Well-defined props interface.The props interface is comprehensive and properly typed.
105-109: Panel size persistence is not implemented.The
autoSaveIdprop suggests panel sizes should persist, but there's no visible mechanism that actually saves or restores panel sizes across sessions.
177-177: LGTM! Proper export statement.The default export is correctly defined.
76-78:⚠️ Potential issueAdd validation for initial case selection.
The initial case selection assumes
cases[0]exists. This could lead to runtime errors if the cases array is empty.Apply this diff to add validation:
- setSelectedCases([cases[0].id]) + setSelectedCases(cases.length > 0 ? [cases[0].id] : [])Likely invalid or redundant comment.
apps/frontend-manage/src/components/evaluation/elements/CSOneDimScatterPlot.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/shared-components/src/evaluation/SCEvaluation.tsx (2)
18-33: Add safety check for evaluation.choices.Consider adding a null check before mapping over evaluation.choices to handle potential undefined cases gracefully.
- {evaluation.choices?.map((choice) => ( + {evaluation.choices?.length > 0 ? ( + evaluation.choices.map((choice) => ( <Progress // ... existing props /> - ))} + )) + ) : ( + <div className="text-gray-500">{t('common.noData')}</div> + )}
34-34: Consider using a more type-safe approach for the formatter.While the type assertion improves clarity, consider using a type guard for better runtime safety.
- formatter={(v) => (v as number).toFixed() + '%'} + formatter={(v) => (typeof v === 'number' ? v.toFixed() : '0') + '%'}This approach:
- Provides runtime type checking
- Handles non-number cases gracefully
- Is more type-safe than type assertion
packages/shared-components/src/evaluation/MCKPRIMEvaluation.tsx (2)
34-38: Improve percentage calculation logic.The current percentage calculation could be simplified and made more robust:
- Using a fallback of 1 for
numAnswerswhen it's 0 could lead to misleading 100% results- The nested ternary could be simplified
Consider this alternative implementation:
- value={ - choice.count - ? (choice.count / (evaluation.numAnswers ?? 1)) * 100 - : 0 - } + value={evaluation.numAnswers ? (choice.count ?? 0) / evaluation.numAnswers * 100 : 0}
40-40: Consider a more type-safe approach instead of type assertion.While the type assertion works, we can make this more type-safe since we know the value should always be a number based on the calculation.
Here are two alternative approaches:
- formatter={(v) => (v as number).toFixed() + '%'} + formatter={(v: number) => v.toFixed() + '%'}Or even better, use type inference:
- formatter={(v) => (v as number).toFixed() + '%'} + formatter={(v) => Number(v).toFixed() + '%'}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/shared-components/src/evaluation/MCKPRIMEvaluation.tsx(1 hunks)packages/shared-components/src/evaluation/SCEvaluation.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: check
- GitHub Check: build
- GitHub Check: SonarCloud
- GitHub Check: build
- GitHub Check: Analyze (javascript)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
- GitHub Check: cypress-run
🔇 Additional comments (2)
packages/shared-components/src/evaluation/SCEvaluation.tsx (1)
1-9: LGTM! Clean imports and well-defined Props interface.The component's dependencies and type definitions are clear and appropriate.
packages/shared-components/src/evaluation/MCKPRIMEvaluation.tsx (1)
1-1: Consider updating the evaluation type to align with new activity evaluation types.Based on the broader changes in the codebase moving from element instance evaluations to activity evaluations, the
ChoicesInstanceEvaluationtype might need to be updated to maintain consistency.Let's verify the type changes in the codebase:
Also applies to: 7-9
✅ Verification successful
Update the evaluation type to use the new activity evaluation types.
The shell script results confirm that many components now rely on the updated evaluation types (e.g., using ChoicesActivityEvaluationData rather than ChoicesInstanceEvaluation). In order to maintain consistency across the codebase, please update the import and any related usage in packages/shared-components/src/evaluation/MCKPRIMEvaluation.tsx (and the corresponding lines in 7–9) to align with the new activity evaluation types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for new activity evaluation types that might be relevant ast-grep --pattern 'type $name = { $$$ choices?: $$$ $$$ }' # Check for other components using the new types rg "ChoicesInstanceEvaluation|ActivityEvaluation" -A 5Length of output: 72818
|
Summary by CodeRabbit
Summary by CodeRabbit
New Features
CSEvaluation,CSEvaluationHistogram,CSEvaluationScatter, andCSOneDimScatterPlot.Bug Fixes
Chores
@uzh-bf/design-systemacross multiple packages.