chore(apps/analytics): make sure that analytics pages do not break for empty courses #4407
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a comprehensive enhancement to the user experience in the analytics sections of the KlickerUZH application. The changes focus on improving data visualization and user feedback by adding conditional rendering mechanisms across multiple components. When no activity or performance data is available, components now display a Changes
Possibly related PRs
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
![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
🧹 Nitpick comments (13)
apps/frontend-manage/src/components/analytics/activity/DailyActivityTimeSeries.tsx (1)
21-34: Consider extracting date formatting logic into a utility function.The date formatting logic could be moved to a reusable utility function to improve maintainability and reusability across components.
+ // utils/formatDate.ts + export const formatDateToGB = (date: Date): string => { + return date.toLocaleDateString('en-GB', { + day: '2-digit', + month: '2-digit', + year: 'numeric', + }).replace(/\//g, '-') + } // In component - date: date - .toLocaleDateString('en-GB', { - day: '2-digit', - month: '2-digit', - year: 'numeric', - }) - .replace(/\//g, '-'), + date: formatDateToGB(date),apps/frontend-manage/src/components/analytics/activity/DailyActivityPlot.tsx (1)
52-59: Consider simplifying the activity check using array methods.The current implementation using multiple
&&operators could be simplified using array methods for better maintainability.- const noActivity = - activeDays.monday === 0 && - activeDays.tuesday === 0 && - activeDays.wednesday === 0 && - activeDays.thursday === 0 && - activeDays.friday === 0 && - activeDays.saturday === 0 && - activeDays.sunday === 0 + const noActivity = Object.values(activeDays).every((value) => value === 0)apps/frontend-manage/src/components/analytics/performance/ActivityProgressPlot.tsx (2)
Line range hint
23-47: Consider moving chart-related constants outside the component.The
chartColorsobject andProgressLegendcomponent could be moved outside to prevent recreation on each render.+ const CHART_COLORS = { + started: '#4ade80', + completed: '#15803d', + repeated: '#064e3b', + } + const ProgressLegend = ({ t }: { t: any }) => ( + <Legend + payload={[ + { + value: t('manage.analytics.started'), + color: CHART_COLORS.started, + type: 'rect', + }, + { + value: t('manage.analytics.completed'), + color: CHART_COLORS.completed, + type: 'rect', + }, + { + value: t('manage.analytics.repeated'), + color: CHART_COLORS.repeated, + type: 'rect', + }, + ]} + wrapperStyle={{ bottom: 0, right: 0 }} + /> + ) function ActivityProgressPlot({ activityProgresses, participants, }: { activityProgresses: ActivityProgress[] participants: number }) { const t = useTranslations() - const chartColors = { ... } - const ProgressLegend = () => ( ... )
54-96: Consider simplifying the conditional rendering structure.The nested conditional structure could be simplified to improve readability while maintaining the same functionality.
- {pqProgresses.length > 0 || mlProgresses.length > 0 ? ( - <div className="flex flex-col gap-6"> - {pqProgresses.length > 0 && ( - <div> - <div className="relative flex flex-row"> - <H4>{t('shared.generic.practiceQuizzes')}</H4> - <ProgressLegend /> - </div> - {pqProgresses.map((progress, idx) => ( - <StackedProgress - key={`activity-progress-pq-${idx}`} - progress={progress} - participants={participants} - colors={chartColors} - showScale={idx === pqProgresses.length - 1} - /> - ))} - </div> - )} - {mlProgresses.length > 0 && ( - <div> - <div className="relative flex flex-row"> - <H4>{t('shared.generic.microlearnings')}</H4> - <ProgressLegend /> - </div> - {mlProgresses.map((progress, idx) => ( - <StackedProgress - key={`activity-progress-ml-${idx}`} - progress={progress} - participants={participants} - colors={chartColors} - showScale={idx === mlProgresses.length - 1} - /> - ))} - </div> - )} - </div> - ) : ( + {pqProgresses.length === 0 && mlProgresses.length === 0 ? ( <UserNotification message={t('manage.analytics.noAsynchronousActivityProgressData')} type="info" /> + ) : ( + <div className="flex flex-col gap-6"> + {[ + { data: pqProgresses, title: t('shared.generic.practiceQuizzes'), type: 'pq' }, + { data: mlProgresses, title: t('shared.generic.microlearnings'), type: 'ml' }, + ].map( + ({ data, title, type }) => + data.length > 0 && ( + <div key={type}> + <div className="relative flex flex-row"> + <H4>{title}</H4> + <ProgressLegend t={t} /> + </div> + {data.map((progress, idx) => ( + <StackedProgress + key={`activity-progress-${type}-${idx}`} + progress={progress} + participants={participants} + colors={CHART_COLORS} + showScale={idx === data.length - 1} + /> + ))} + </div> + ) + )} + </div> )}apps/frontend-manage/src/components/analytics/activity/WeeklyActivityTimeSeries.tsx (1)
41-69: Consider extracting the data transformation logic.The nested ternary operations and complex data mapping logic could be moved to a separate utility function for better maintainability.
Consider this refactor:
+ const transformActivityData = ( + activity: ParticipantActivityTimestamp[], + courseParticipants: number, + secondActivity: ParticipantActivityTimestamp[], + secondParticipants: number, + hasComparison: boolean + ) => { + if (hasComparison && secondParticipants > 0) { + return activity.map((item, idx) => ({ + date: t('manage.analytics.weekN', { number: idx + 1 }), + activeParticipants: (item.activeParticipants / courseParticipants) * 100, + activeParticipantsReference: secondActivity.length > idx + ? (secondActivity[idx].activeParticipants / secondParticipants) * 100 + : undefined, + })) + } + + return activity.map((item) => { + const date = new Date(item.date) + return { + date: date.toLocaleDateString('en-GB', { + day: '2-digit', + month: '2-digit', + year: 'numeric', + }).replace(/\//g, '-'), + activeParticipants: (item.activeParticipants / courseParticipants) * 100, + } + }) + } <ActivityTimeSeriesPlot singleCourse={typeof courseComparison === 'undefined'} - activityData={ - typeof courseComparison !== 'undefined' && - secondParticipants > 0 - ? activity.map((item, idx) => ({ - date: t('manage.analytics.weekN', { number: idx + 1 }), - activeParticipants: - (item.activeParticipants / courseParticipants) * 100, - activeParticipantsReference: - secondActivity.length > idx - ? (secondActivity[idx].activeParticipants / - secondParticipants) * - 100 - : undefined, - })) - : activity.map((item) => { - const date = new Date(item.date) - return { - date: date - .toLocaleDateString('en-GB', { - day: '2-digit', - month: '2-digit', - year: 'numeric', - }) - .replace(/\//g, '-'), - activeParticipants: - (item.activeParticipants / courseParticipants) * 100, - } - }) - } + activityData={transformActivityData( + activity, + courseParticipants, + secondActivity, + secondParticipants, + typeof courseComparison !== 'undefined' + )}apps/frontend-manage/src/components/analytics/activity/TotalStudentActivityPlot.tsx (1)
108-183: Consider adding row count and pagination controls configuration.The DataTable is well configured, but could benefit from explicit pagination controls for better user experience with large datasets.
Add these configurations to the DataTable:
<DataTable isPaginated isResetSortingEnabled + pageSize={10} + showRowCount + paginationControls={{ + showFirstLastPageButtons: true, + rowsPerPageOptions: [10, 25, 50], + }} columns={[apps/frontend-manage/src/components/analytics/performance/TotalStudentPerformancePlot.tsx (1)
176-181: Consider using an enum mapping object for performance levels.The current nested ternary operation for mapping performance levels could be replaced with a more maintainable object mapping.
Consider this refactor:
+ const PERFORMANCE_LEVEL_MAPPING = { + [PerformanceLevel.High]: 3, + [PerformanceLevel.Medium]: 2, + [PerformanceLevel.Low]: 1, + } as const; data={participantPerformance.map((entry, ix) => ({ ...entry, student: t('manage.analytics.studentN', { number: ix + 1 }), - performanceLevelNumber: - entry.totalPerformance === PerformanceLevel.High - ? 3 - : entry.totalPerformance === PerformanceLevel.Medium - ? 2 - : 1, + performanceLevelNumber: PERFORMANCE_LEVEL_MAPPING[entry.totalPerformance], }))}packages/i18n/messages/en.ts (1)
1909-1920: LGTM! Consider adding placeholders for dynamic content.The new analytics messages are well-structured and provide clear feedback for empty states. They follow a consistent pattern across different analytics scenarios.
Consider adding placeholders for dynamic content (e.g., course name, date ranges) to make messages more specific:
- noWeeklyActivityData: 'Until now, no weekly activity data is available for this course.', + noWeeklyActivityData: 'Until now, no weekly activity data is available for course "{name}".',apps/frontend-manage/src/pages/analytics/[courseId]/quizzes/index.tsx (3)
Line range hint
41-42: Consider optimizing search implementationThe current search implementation might trigger unnecessary re-renders on every keystroke. Consider adding debounce to improve performance.
Here's a suggested implementation using debounce:
- const [practiceSearch, setPracticeSearch] = useState('') - const [microSearch, setMicroSearch] = useState('') + const [practiceSearch, setPracticeSearch] = useState('') + const [microSearch, setMicroSearch] = useState('') + const debouncedSetPracticeSearch = useCallback( + debounce((value: string) => setPracticeSearch(value), 300), + [] + ) + const debouncedSetMicroSearch = useCallback( + debounce((value: string) => setMicroSearch(value), 300), + [] + )Don't forget to add the imports:
import { debounce } from 'lodash' import { useCallback } from 'react'Also applies to: 51-52
Line range hint
89-94: Enhance search field accessibilityThe search fields could benefit from additional accessibility attributes to improve the experience for screen reader users.
Consider adding these accessibility enhancements:
<TextField placeholder={t('manage.analytics.searchPracticeQuizzes')} className={{ input: 'my-2 h-8' }} value={practiceSearch} onChange={(newValue) => setPracticeSearch(newValue)} + aria-label={t('manage.analytics.searchPracticeQuizzes')} + aria-describedby="practice-search-results" /> + <div id="practice-search-results" className="sr-only"> + {filteredPracticeQuizzes?.length ?? 0} {t('shared.generic.results')} + </div>Apply similar changes to the microlearnings search field as well.
Also applies to: 116-121
Line range hint
183-196: Add type safety and documentation to static generation functionsWhile the implementation is correct, it could benefit from improved type safety and documentation.
Consider these enhancements:
- export async function getStaticProps({ locale }: GetStaticPropsContext) { + export async function getStaticProps({ + locale = 'en' + }: GetStaticPropsContext): Promise<{ + props: { messages: Record<string, string> } + }> { return { props: { messages: (await import(`@klicker-uzh/i18n/messages/${locale}`)).default, }, } } + // Using blocking fallback to ensure SEO-friendly rendering + // and handle dynamic course IDs without generating paths upfront export function getStaticPaths() { return { paths: [], fallback: 'blocking', } }packages/graphql/src/services/analytics.ts (2)
480-482: Fix indentation of the if condition.The indentation of the if condition appears to be inconsistent with the surrounding code.
Apply this diff to fix the indentation:
- if ( - course.practiceQuizzes.length === 0 && - course.microLearnings.length === 0 - ) { + if ( + course.practiceQuizzes.length === 0 && + course.microLearnings.length === 0 + ) {
483-492: LGTM! Good handling of empty courses.The change improves the handling of empty courses by returning a valid response structure with empty arrays instead of null. This ensures that the UI components receive consistent data structures, preventing potential rendering issues.
This is a good example of the Robustness Principle (Postel's Law): be conservative in what you send, be liberal in what you accept. By returning a consistent structure even for empty courses, you're making it easier for client code to handle the response.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/frontend-manage/src/components/analytics/activity/DailyActivityPlot.tsx(2 hunks)apps/frontend-manage/src/components/analytics/activity/DailyActivityTimeSeries.tsx(2 hunks)apps/frontend-manage/src/components/analytics/activity/TotalStudentActivityPlot.tsx(2 hunks)apps/frontend-manage/src/components/analytics/activity/WeeklyActivityTimeSeries.tsx(2 hunks)apps/frontend-manage/src/components/analytics/performance/ActivityProgressPlot.tsx(2 hunks)apps/frontend-manage/src/components/analytics/performance/TotalStudentPerformancePlot.tsx(2 hunks)apps/frontend-manage/src/pages/analytics/[courseId]/activity.tsx(1 hunks)apps/frontend-manage/src/pages/analytics/[courseId]/quizzes/index.tsx(2 hunks)packages/graphql/src/services/analytics.ts(1 hunks)packages/i18n/messages/de.ts(1 hunks)packages/i18n/messages/en.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/frontend-manage/src/pages/analytics/[courseId]/activity.tsx
🔇 Additional comments (10)
apps/frontend-manage/src/components/analytics/activity/DailyActivityTimeSeries.tsx (1)
2-2: LGTM! Empty state handling is implemented correctly.
The conditional rendering based on activity.length is appropriate, and the UserNotification component is used consistently with other analytics components.
Also applies to: 18-42
apps/frontend-manage/src/components/analytics/activity/DailyActivityPlot.tsx (1)
64-95: LGTM! Conditional rendering is implemented correctly.
The chart is appropriately hidden when there's no activity data, showing a user-friendly notification instead.
apps/frontend-manage/src/components/analytics/performance/ActivityProgressPlot.tsx (1)
54-96: LGTM! Empty state handling is implemented correctly.
The component appropriately handles cases where there's no progress data for both practice quizzes and microlearnings.
apps/frontend-manage/src/components/analytics/activity/WeeklyActivityTimeSeries.tsx (1)
6-6: LGTM! Proper empty state handling added.
The addition of UserNotification for empty states aligns with the PR objective of preventing analytics pages from breaking for empty courses.
Also applies to: 37-88
apps/frontend-manage/src/components/analytics/activity/TotalStudentActivityPlot.tsx (1)
4-4: LGTM! Proper empty state handling added.
The addition of UserNotification for empty states and the conditional rendering of the visualization components is well implemented.
Also applies to: 43-190
apps/frontend-manage/src/components/analytics/performance/TotalStudentPerformancePlot.tsx (1)
7-7: LGTM! Proper empty state handling added.
The addition of UserNotification for empty states and the conditional rendering of the visualization components is well implemented.
Also applies to: 43-195
packages/i18n/messages/de.ts (1)
1924-1935: LGTM! Translations are accurate and natural.
The German translations accurately convey the same meaning as their English counterparts while maintaining natural German language flow.
apps/frontend-manage/src/pages/analytics/[courseId]/quizzes/index.tsx (2)
130-130: LGTM: Consistent styling improvements for empty state notifications
The addition of w-full class ensures notifications span the full width of their containers, providing better visual alignment and consistency when displaying empty states.
Also applies to: 157-157
129-131: Verify empty state handling across the application
While the empty state handling looks good here, let's ensure consistency across other analytics pages.
Also applies to: 156-158
✅ Verification successful
Let me gather more information about empty state handling across the application to make a comprehensive assessment.
Based on the comprehensive search results, I can now provide a final response:
Empty state styling is consistently implemented across analytics pages
The search results show that the empty state handling in the analytics pages is already consistent:
- The
UserNotificationcomponent in the quizzes analytics page uses the same pattern as other analytics components - All analytics-related empty states use the "info" type consistently
- The
classNameprop withw-fullis appropriately used where full-width notifications are needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other UserNotification components in analytics pages
# that might need similar styling updates
echo "Searching for UserNotification components in analytics pages..."
rg -l "UserNotification" "apps/frontend-manage/src/pages/analytics"
echo "Checking for consistent className usage..."
ast-grep --pattern 'UserNotification $props' "apps/frontend-manage/src/pages/analytics"
Length of output: 455
Script:
#!/bin/bash
# Search for empty state messages and their styling patterns across the application
echo "Searching for empty state related translations..."
rg "noPractice|noData|empty" "apps/frontend-manage/src/pages"
echo "Checking UserNotification usage in the entire frontend-manage..."
rg -A 2 "UserNotification" "apps/frontend-manage/src"
Length of output: 39067
packages/graphql/src/services/analytics.ts (1)
475-477: LGTM! Good separation of error cases.
The null check is well-placed and correctly handles the case where no course is found, improving error handling.
|
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation