enhance(apps/analytics): add course data comparison for weekly student activity chart #4394
Conversation
|
Warning Rate limit exceeded@sjschlapbach has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 45 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. 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several new components and modifications to existing ones within the analytics section of the frontend application. Key changes include the addition of Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
packages/graphql/src/ops.ts (1)
4091-4091: Consider adding a comment describing the query's purpose.While the implementation is correct and follows the established patterns, adding a brief JSDoc comment describing the query's purpose would improve maintainability.
+/** + * Retrieves weekly activity data for a course, including total participants + * and activity timestamps. + */ export const GetCourseWeeklyActivityDocument = {"kind":"Document",...packages/graphql/src/schema/analytics.ts (1)
86-102: LGTM! Consider adding input validation.The new types for weekly course activities are well-structured and consistent with the existing pattern. The reuse of
ParticipantActivityTimestampmaintains consistency.Consider adding validation to ensure
totalParticipantsis non-negative andweeklyActivityarray is not empty:export const WeeklyCourseActivities = builder.objectType( WeeklyCourseActivitiesRef, { fields: (t) => ({ - totalParticipants: t.exposeInt('totalParticipants'), + totalParticipants: t.int({ + resolve: (parent) => { + if (parent.totalParticipants < 0) return 0 + return parent.totalParticipants + }, + }), weeklyActivity: t.expose('weeklyActivity', { type: [ParticipantActivityTimestamp], + validate: { minItems: 1 }, }), }), } )apps/frontend-manage/src/components/analytics/activity/WeeklyActivityTimeSeries.tsx (2)
45-72: Extract data transformation logic.The complex data transformation logic in the render method makes the component harder to maintain and test.
Consider extracting the transformation logic into a separate utility function:
const transformActivityData = ( activity: ParticipantActivityTimestamp[], courseParticipants: number, courseComparison?: { activity: ParticipantActivityTimestamp[], participants: number } ) => { if (courseComparison) { return activity.map((item, idx) => ({ date: `Week ${idx + 1}`, activeParticipants: (item.activeParticipants / courseParticipants) * 100, activeParticipantsReference: courseComparison.activity[idx] ? (courseComparison.activity[idx].activeParticipants / courseComparison.participants) * 100 : undefined, })) } return activity.map((item) => ({ date: new Date(item.date).toLocaleDateString('en-GB', { day: '2-digit', month: '2-digit', year: 'numeric', }).replace(/\//g, '-'), activeParticipants: (item.activeParticipants / courseParticipants) * 100, })) }
77-82: Consider fallback UI for Suspense boundary.The Suspense wrapper is missing a fallback UI which could lead to a jarring user experience during loading.
Add a fallback:
- <Suspense> + <Suspense fallback={<div className="w-full lg:w-1/4 animate-pulse h-32" />}> <SuspendedCourseComparison courseComparison={courseComparison} setCourseComparison={setCourseComparison} /> </Suspense>apps/frontend-manage/src/pages/analytics/[courseId]/activity.tsx (1)
72-75: LGTM! Consider adding prop type validation.The integration of the new WeeklyActivityTimeSeries component looks good. Props are passed correctly and the layout is maintained.
Consider adding runtime validation for the activity data to ensure type safety:
import { z } from 'zod' const activitySchema = z.array(z.object({ date: z.date(), activeParticipants: z.number().min(0) })) // Validate before passing to component const validatedActivity = activitySchema.safeParse(course.weeklyActivity) if (!validatedActivity.success) { console.error('Invalid activity data:', validatedActivity.error) return null }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
apps/frontend-manage/src/components/analytics/activity/ActivityTimeSeriesPlot.tsx(2 hunks)apps/frontend-manage/src/components/analytics/activity/DailyActivityTimeSeries.tsx(1 hunks)apps/frontend-manage/src/components/analytics/activity/SuspendedCourseComparison.tsx(1 hunks)apps/frontend-manage/src/components/analytics/activity/WeeklyActivityTimeSeries.tsx(1 hunks)apps/frontend-manage/src/pages/analytics/[courseId]/activity.tsx(2 hunks)packages/graphql/src/graphql/ops/QGetCourseWeeklyActivity.graphql(1 hunks)packages/graphql/src/ops.schema.json(2 hunks)packages/graphql/src/ops.ts(5 hunks)packages/graphql/src/public/client.json(1 hunks)packages/graphql/src/public/schema.graphql(2 hunks)packages/graphql/src/public/server.json(1 hunks)packages/graphql/src/schema/analytics.ts(1 hunks)packages/graphql/src/schema/query.ts(2 hunks)packages/graphql/src/services/analytics.ts(1 hunks)packages/i18n/messages/de.ts(2 hunks)packages/i18n/messages/en.ts(1 hunks)
🔥 Files not summarized due to errors (1)
- packages/graphql/src/ops.ts: Error: Server error: no LLM provider could handle the message
🔇 Additional comments (13)
packages/graphql/src/ops.ts (2)
2148-2150: LGTM! Well-structured type definitions.
The type definitions are clean, follow TypeScript best practices, and use proper GraphQL types (Maybe, InputMaybe). The field names are clear and descriptive.
Also applies to: 2551-2555
3594-3600: LGTM! Clean query definition.
The query definition follows GraphQL naming conventions and properly uses types. The structure is clear and consistent with other queries in the codebase.
packages/graphql/src/graphql/ops/QGetCourseWeeklyActivity.graphql (1)
1-9: LGTM!
The GraphQL query is well-defined and aligns with the expected schema structure.
packages/graphql/src/schema/query.ts (2)
14-18: LGTM: Import addition is properly organized
The WeeklyCourseActivities import is correctly grouped with other analytics-related imports.
761-770: Verify error handling for missing courseId parameter
The query field is well-structured and properly secured with user authentication. However, since courseId is optional, we should verify how AnalyticsService handles the case when it's not provided.
✅ Verification successful
Let me search for the implementation of getCourseWeeklyActivity in the AnalyticsService.
Let me try a different search pattern to locate the implementation.
Let me try one final search pattern to locate the service implementation.
I see the analytics service is likely in packages/graphql/src/services/analytics.ts. Let me check its implementation.
Error handling for missing courseId parameter is properly implemented
The implementation in analytics.ts correctly handles the case when courseId is not provided by returning null. This is consistent with the GraphQL field definition where the return type is marked as nullable. The function also includes proper authorization checks by verifying course ownership through ownerId: ctx.user.sub.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how AnalyticsService handles missing courseId
# Look for error handling in analytics service
rg -A 5 "getCourseWeeklyActivity.*ctx"
Length of output: 366
Script:
#!/bin/bash
# Search for the implementation of getCourseWeeklyActivity
ast-grep --pattern 'getCourseWeeklyActivity($$$) {
$$$
}'
# Also try with ripgrep to catch any other implementations
rg -A 10 "getCourseWeeklyActivity.*=.*"
Length of output: 103
Script:
#!/bin/bash
# Search for AnalyticsService class and its methods
rg -A 20 "class AnalyticsService"
# Also search for any file containing AnalyticsService
fd "analytics" --type f --exec rg -l "AnalyticsService"
Length of output: 89
Script:
#!/bin/bash
# Search for files with 'analytics' in their name
fd "analytics" --type f
# Search for any exports or declarations related to AnalyticsService
rg "export.*AnalyticsService|const.*AnalyticsService|class.*AnalyticsService" -A 10
Length of output: 1757
Script:
#!/bin/bash
# Check the implementation of getCourseWeeklyActivity in the analytics service
cat packages/graphql/src/services/analytics.ts
Length of output: 2364
packages/graphql/src/public/schema.graphql (1)
1244-1244: LGTM: Type definition is well-structured
The WeeklyCourseActivities type is properly defined with:
- Non-nullable fields ensuring data consistency
- Reuse of existing ParticipantActivityTimestamp type
- Clear integration with the new query field
The schema changes align well with the query implementation in query.ts.
Also applies to: 1479-1481
packages/i18n/messages/en.ts (1)
1827-1831: LGTM! Localization strings for course comparison feature are well structured.
The new strings provide clear and consistent translations for the course comparison feature. The descriptions are user-friendly and maintain consistency between English and German translations.
packages/graphql/src/ops.schema.json (2)
18746-18770: LGTM! Well-structured query field addition
The new getCourseWeeklyActivity query field is well-defined with appropriate parameter and return type to support weekly activity data retrieval.
21994-22045: LGTM! Well-designed type definition
The new WeeklyCourseActivities type is well-structured with:
- Clear field naming
- Proper nullability constraints
- Good reuse of existing
ParticipantActivityTimestamptype for consistency
packages/graphql/src/public/client.json (1)
124-124: LGTM: New operation hash added correctly
The new hash entry for GetCourseWeeklyActivity follows the established pattern and uses a valid SHA-256 hash.
packages/graphql/src/public/server.json (1)
124-124: LGTM: New GraphQL query properly structured
The GetCourseWeeklyActivity query is well-defined with proper types, optional parameters, and follows GraphQL best practices.
packages/i18n/messages/de.ts (2)
1839-1842: LGTM: German translations added correctly
The new translations for course comparison functionality are clear, properly formatted, and correctly use string parameters where needed.
1904-1904: LGTM: Week indicator translation added
The weekN translation is properly formatted and correctly uses the {number} parameter.
apps/frontend-manage/src/components/analytics/activity/ActivityTimeSeriesPlot.tsx
Show resolved
Hide resolved
apps/frontend-manage/src/components/analytics/activity/DailyActivityTimeSeries.tsx
Show resolved
Hide resolved
apps/frontend-manage/src/components/analytics/activity/SuspendedCourseComparison.tsx
Show resolved
Hide resolved
apps/frontend-manage/src/components/analytics/activity/WeeklyActivityTimeSeries.tsx
Outdated
Show resolved
Hide resolved
apps/frontend-manage/src/components/analytics/activity/WeeklyActivityTimeSeries.tsx
Show resolved
Hide resolved
|
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
activity-comparison
|
| Run status |
|
| Run duration | 12m 17s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
148
|
| View all changes introduced in this branch ↗︎ | |
Summary by CodeRabbit
Release Notes
New Features
DailyActivityTimeSeriesandWeeklyActivityTimeSeriescomponents for enhanced visualization of daily and weekly course activity data.SuspendedCourseComparisoncomponent for comparing course activities.GetCourseWeeklyActivityfor retrieving weekly activity data.Enhancements
ActivityTimeSeriesPlotcomponent based on course comparisons.Bug Fixes
ActivityDashboardcomponent.