enhance(apps/analytics): add overview of element feedbacks on activities and instances #4401
Conversation
…es on instances and activities
…otes illustration
Current Aviator status
This PR was merged manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time.
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
|
Caution Review failedThe pull request is closed. Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ast-grep (0.31.1)apps/frontend-manage/src/components/analytics/performance/useElementFeedbackSearch.tsAn unexpected error occurred while running ast-grep. 📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces comprehensive enhancements to the learning analytics system across multiple packages and applications. The changes primarily focus on adding new feedback tracking mechanisms for activities and instances, updating import paths, and refactoring component names. The modifications span frontend management, PWA applications, GraphQL schema, internationalization, and type definitions, creating a more robust and detailed analytics infrastructure. Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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: 3
🧹 Nitpick comments (27)
apps/frontend-manage/src/pages/analytics/index.tsx (1)
6-7: Consider the trade-offs of relative vs. absolute importsWhile relative imports work, consider the maintainability aspects:
- Absolute imports (with path aliases) can make the codebase more maintainable during file relocations
- Relative imports can make it harder to move files around without breaking imports
Consider using TypeScript path aliases (e.g.,
@/components/...) for a balance between explicit paths and maintainability.packages/graphql/src/ops.ts (1)
49-56: LGTM! Consider adding JSDoc comments.The new type definitions are well-structured and consistent. The feedback types appropriately use float values for vote rates, allowing for percentage calculations.
Consider adding JSDoc comments to document the purpose of each type and any important constraints on the rate values. Example:
+/** + * Represents feedback data for a course activity + * @property upvoteRate - The rate of upvotes (0.0 to 1.0) + * @property downvoteRate - The rate of downvotes (0.0 to 1.0) + */ export type ActivityFeedback = { __typename?: 'ActivityFeedback'; activityName: Scalars['String']['output']; // ... };Also applies to: 379-385, 892-900
apps/frontend-manage/src/pages/analytics/[courseId]/quizzes.tsx (5)
1-2: Use smaller or direct imports from your GraphQL library.While importing from "@klicker-uzh/graphql/dist/ops" might be correct for your setup, consider whether you can import the generated documents more directly (e.g., via "@klicker-uzh/graphql") to avoid potential path-churn if the library folder structure evolves.
26-34: Consider letting the router handle the route-based gating.Currently, your loading state covers both the "loading" variable from Apollo and the absence of "courseId". This is fine, but an alternative is to check the route param in a higher-level route guard. That might simplify your component logic if you have repeated patterns across other pages.
36-44: Improve error handling messages or actions.You display a generic error view, which is helpful. You could provide more details or a retry option, especially if the error might be intermittent or user-correctable.
50-59: Separate layout styling from data.The display of the course name and total participants is clear and well styled. Consider placing them in a small, dedicated component if you plan to reuse this info or layout in other analytics pages.
61-64: Encapsulate feedback logic if it expands later.The “ActivityInstanceFeedbacksPlot” is directly injected here. If you plan to extend the feedback logic for quizzes further, you might want a dedicated wrapper or container for all feedback components under a “quiz feedback” umbrella.
apps/frontend-manage/src/components/analytics/quiz/ActivityInstanceFeedbacksPlot.tsx (5)
1-2: Reduce import from FontAwesome.You are importing icons from the FontAwesome package. If the bundle size is of any concern, consider using tree-shaking or selectively importing icons from a narrower source (e.g., '@fortawesome/free-solid-svg-icons/faX') to cut down on potential overhead.
34-39: Consolidate default filters.You are defining “defaultFilters” as a constant with multiple fields. This is fine, but ensuring each enumerated filter type is typed or tested (especially if new filters appear) will help reduce future mistakes.
42-55: Potential duplication of state.You have separate search terms for activity (“activitySearchTerm”) and instance (“instanceSearchTerm”). Since the type
('activity' | 'instance')is already tracked, you might consolidate them into a single searchTerm with conditional usage, or into a dictionary keyed by type. This can reduce code repetition.
78-104: Avoid inline styles in className if you can.Your usage of “py-0.25” is presumably from a tailwind or system library. Double-check that fractional units do behave consistently across browsers. Also, consider a design token approach if used widely.
130-174: Provide more descriptive user messages or a link.When no entries are present, you notify the user to manage filters. Directly linking them to documentation or providing a brief explanation of typical filter usage might further improve user experience.
apps/frontend-manage/src/components/analytics/StackedBarChartLabel.tsx (1)
1-27: Add input validation and improve accessibilityWhile the implementation is solid, consider these improvements:
- Add validation for the
valueprop to handle negative numbers or NaN- Add
aria-labelfor better accessibilityfunction StackedBarChartLabel({ value, x, width, y, height, }: { value: number x: number y: number width: number height: number }) { + const displayValue = Math.max(0, Number.isFinite(value) ? value : 0) return ( <text x={x + width / 2} y={y + height / 2 + 1} fill="white" fontSize={14} textAnchor="middle" dominantBaseline="middle" className="font-bold" + aria-label={`${displayValue}%`} > - {value} % + {displayValue} % </text> ) }apps/frontend-manage/src/components/analytics/quiz/QuizAnalyticsNavigation.tsx (1)
8-11: Consider extracting URL paths to constantsThe hardcoded URL paths could be moved to constants or a routing configuration for better maintainability.
+const ANALYTICS_ROUTES = { + PERFORMANCE: (courseId: string) => `/analytics/${courseId}/performance`, + ACTIVITY: (courseId: string) => `/analytics/${courseId}/activity`, +} as const function QuizAnalyticsNavigation({ courseId }: { courseId: string }) { return ( <AnalyticsNavigation - hrefLeft={`/analytics/${courseId}/performance`} + hrefLeft={ANALYTICS_ROUTES.PERFORMANCE(courseId)} labelLeft={<PerformanceDashboardLabel />} - hrefRight={`/analytics/${courseId}/activity`} + hrefRight={ANALYTICS_ROUTES.ACTIVITY(courseId)} labelRight={<ActivityDashboardLabel />} /> ) }apps/frontend-manage/src/components/analytics/quiz/useElementFeedbackSearch.ts (1)
15-25: Add error handling and consider performance optimizationsThe search implementation could be improved with:
- Validation for empty or invalid feedback arrays
- Configuration options for search settings (e.g., exact match, case sensitivity)
- Pagination or virtualization for large datasets
return useMemo(() => { + // Validate input arrays + if (!Array.isArray(activityFeedbacks) || !Array.isArray(instanceFeedbacks)) { + throw new Error('Invalid feedback arrays provided') + } const activitySearch = new JsSearch.Search('activityName') + // Configure search settings + activitySearch.searchIndex = new JsSearch.UnorderedSearchIndex() + activitySearch.tokenizer = new JsSearch.StopWordsTokenizer( + new JsSearch.SimpleTokenizer() + ) activitySearch.addIndex('activityName') activitySearch.addDocuments(activityFeedbacks) const instanceSearch = new JsSearch.Search('elementName') + instanceSearch.searchIndex = new JsSearch.UnorderedSearchIndex() + instanceSearch.tokenizer = new JsSearch.StopWordsTokenizer( + new JsSearch.SimpleTokenizer() + ) instanceSearch.addIndex('elementName') instanceSearch.addDocuments(instanceFeedbacks) return { activitySearch, instanceSearch } }, [activityFeedbacks, instanceFeedbacks])apps/frontend-manage/src/components/analytics/ActivityTypeFilter.tsx (1)
Line range hint
6-35: Consider extracting activity type options to a constant.The implementation looks good, but the activity type options could be extracted to improve maintainability.
Consider refactoring the items array:
+ const ACTIVITY_TYPE_OPTIONS = [ + { value: 'all', label: 'manage.analytics.allActivityTypes' }, + { value: ActivityType.PracticeQuiz, label: 'shared.generic.practiceQuizzes' }, + { value: ActivityType.MicroLearning, label: 'shared.generic.microlearnings' }, + ] as const function ActivityTypeFilter({...}) { const t = useTranslations() return ( <SelectField label={t('manage.analytics.activityType')} - items={[ - { value: 'all', label: t('manage.analytics.allActivityTypes') }, - { value: ActivityType.PracticeQuiz, label: t('shared.generic.practiceQuizzes') }, - { value: ActivityType.MicroLearning, label: t('shared.generic.microlearnings') }, - ]} + items={ACTIVITY_TYPE_OPTIONS.map(({ value, label }) => ({ + value, + label: t(label) + }))} ... /> ) }apps/frontend-manage/src/components/analytics/quiz/ElementFeedbackBarChart.tsx (1)
18-19: Consider extracting percentage calculations.These calculations could be reused elsewhere in the analytics module.
Consider creating a utility function:
export const calculatePercentage = (rate: number): number => Math.round(rate * 100)apps/frontend-manage/src/components/analytics/performance/PerformanceRatesBarChart.tsx (1)
Line range hint
31-64: Consider memoizing the bar components for better performance.Since the bar components are conditionally rendered based on rate values, consider memoizing them to prevent unnecessary re-renders.
+ import { useMemo } from 'react' function PerformanceRatesBarChart({ title, rates, colors, }: PerformanceRatesBarChartProps) { const roundedErrorRate = Math.round(rates.incorrectRate * 100) const roundedPartialRate = Math.round(rates.partialRate * 100) const roundedCorrectRate = 100 - roundedErrorRate - roundedPartialRate + const errorBar = useMemo(() => + roundedErrorRate > 0 && ( + <Bar + dataKey="incorrectRate" + stackId="1" + fill={colors.incorrect} + label={(props) => ( + <StackedBarChartLabel {...props} value={roundedErrorRate} /> + )} + /> + ), + [roundedErrorRate, colors.incorrect]) // Similar memoization for partial and correct barsapps/frontend-manage/src/components/analytics/quiz/useElementFeedbackFilters.ts (3)
31-48: Consider adding error handling for search operations.The search operation could potentially throw errors if the search index is not properly initialized or if the data structure is unexpected.
- return activitySearch.search(sanitizedSearchTerm) as ActivityFeedback[] + try { + const searchResults = activitySearch.search(sanitizedSearchTerm) + return searchResults as ActivityFeedback[] + } catch (error) { + console.error('Search operation failed:', error) + return typeFiltered + }
10-30: Improve type safety with discriminated unions.Consider using a discriminated union type for better type safety and to avoid type assertions.
type FilterParams = | { type: 'activity' activityFeedbacks: ActivityFeedback[] activityType: ActivityType | 'all' activitySearchTerm: string activitySearch: Search } | { type: 'instance' instanceFeedbacks: InstanceFeedback[] elementType: ElementType | 'all' instanceSearchTerm: string instanceSearch: Search }
35-42: Consider memoizing filter predicates.The filter predicates could be memoized to prevent recreating functions on each render.
+ const typeFilterPredicate = useMemo(() => + (feedback: ActivityFeedback) => { + if (activityType === 'all') return true + return feedback.activityType === activityType + }, + [activityType]) - const typeFiltered = rawData.filter((feedback) => { - if (activityType === 'all') { - return true - } - return feedback.activityType === activityType - }) + const typeFiltered = rawData.filter(typeFilterPredicate)apps/frontend-manage/src/pages/analytics/[courseId]/performance.tsx (1)
Line range hint
41-46: Consider more granular error handling.The error state could be more specific about the type of error encountered (network error, data not found, etc.).
- if (course === null || typeof course === 'undefined' || error) { + if (error) { + return ( + <AnalyticsErrorView + title={t('manage.analytics.performanceDashboard')} + navigation={navigation} + error={error} + message={t('manage.analytics.errors.query')} + /> + ) + } + + if (course === null || typeof course === 'undefined') { return ( <AnalyticsErrorView title={t('manage.analytics.performanceDashboard')} navigation={navigation} + message={t('manage.analytics.errors.noData')} /> ) }apps/frontend-pwa/src/pages/course/[courseId]/microlearning/[id]/[ix].tsx (1)
14-14: Consider using path alias instead of deep relative pathDeep relative paths (
../../../../../) are harder to maintain and more prone to break during refactoring. Consider maintaining the alias import:-import PreviewMessage from '../../../../../components/common/PreviewMessage' +import PreviewMessage from '~/components/common/PreviewMessage'packages/types/src/index.ts (2)
408-415: Consider adding validation constraints and documentation for feedback rates.The
InstanceFeedbacktype looks good but could benefit from additional constraints and documentation:
- Add JSDoc comments explaining how upvote/downvote rates are calculated
- Consider adding min/max constraints (e.g., 0-1 or 0-100) for rate fields
- Consider adding a timestamp field for temporal analysis
+/** Represents feedback metrics for a specific instance + * @property upvoteRate - Rate of upvotes (range: 0-1) + * @property downvoteRate - Rate of downvotes (range: 0-1) + */ export type InstanceFeedback = { id: number activityType: ActivityType instanceName: string instanceType: ElementType upvoteRate: number downvoteRate: number + createdAt?: Date }
417-423: Ensure consistent type usage between InstanceFeedback and ActivityFeedback.The
ActivityFeedbacktype uses string for ID whileInstanceFeedbackuses number. Consider using consistent ID types unless there's a specific reason for the difference.export type ActivityFeedback = { - id: string + id: number activityType: ActivityType activityName: string upvoteRate: number downvoteRate: number }packages/graphql/src/schema/analytics.ts (2)
271-282: Add field descriptions for better GraphQL schema documentation.Consider adding descriptions to the GraphQL fields to improve schema documentation and API usability.
export const InstanceFeedback = builder.objectType(InstanceFeedbackRef, { + description: 'Feedback metrics for a specific instance', fields: (t) => ({ - id: t.exposeInt('id'), + id: t.exposeInt('id', { + description: 'Unique identifier for the instance feedback' + }), activityType: t.expose('activityType', { type: ActivityType }), instanceName: t.exposeString('instanceName'), instanceType: t.expose('instanceType', { type: ElementType }), - upvoteRate: t.exposeFloat('upvoteRate'), + upvoteRate: t.exposeFloat('upvoteRate', { + description: 'Rate of upvotes (range: 0-1)' + }), downvoteRate: t.exposeFloat('downvoteRate'), }), })
296-316: Consider adding pagination support for feedback collections.The
CourseQuizAnalyticstype returns arrays of feedbacks which might grow large. Consider implementing pagination for better performance and scalability.Example implementation:
interface ICourseQuizAnalytics { name: string totalParticipants: number - instanceFeedbacks: InstanceFeedbackType[] - activityFeedbacks: ActivityFeedbackType[] + instanceFeedbacks: { + nodes: InstanceFeedbackType[] + pageInfo: { + hasNextPage: boolean + endCursor: string + } + } + activityFeedbacks: { + nodes: ActivityFeedbackType[] + pageInfo: { + hasNextPage: boolean + endCursor: string + } + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
apps/frontend-manage/src/components/analytics/ActivityTypeFilter.tsx(2 hunks)apps/frontend-manage/src/components/analytics/AnalyticsErrorView.tsx(1 hunks)apps/frontend-manage/src/components/analytics/AnalyticsLoadingView.tsx(1 hunks)apps/frontend-manage/src/components/analytics/AnalyticsSearchField.tsx(2 hunks)apps/frontend-manage/src/components/analytics/ElementTypeFilter.tsx(2 hunks)apps/frontend-manage/src/components/analytics/StackedBarChartLabel.tsx(1 hunks)apps/frontend-manage/src/components/analytics/performance/PerformanceRates.tsx(4 hunks)apps/frontend-manage/src/components/analytics/performance/PerformanceRatesBarChart.tsx(4 hunks)apps/frontend-manage/src/components/analytics/quiz/ActivityInstanceFeedbacksPlot.tsx(1 hunks)apps/frontend-manage/src/components/analytics/quiz/ElementFeedbackBarChart.tsx(1 hunks)apps/frontend-manage/src/components/analytics/quiz/QuizAnalyticsNavigation.tsx(1 hunks)apps/frontend-manage/src/components/analytics/quiz/useElementFeedbackFilters.ts(1 hunks)apps/frontend-manage/src/components/analytics/quiz/useElementFeedbackSearch.ts(1 hunks)apps/frontend-manage/src/pages/analytics/[courseId]/activity.tsx(1 hunks)apps/frontend-manage/src/pages/analytics/[courseId]/performance.tsx(1 hunks)apps/frontend-manage/src/pages/analytics/[courseId]/quizzes.tsx(1 hunks)apps/frontend-manage/src/pages/analytics/index.tsx(1 hunks)apps/frontend-pwa/src/pages/course/[courseId]/index.tsx(1 hunks)apps/frontend-pwa/src/pages/course/[courseId]/microlearning/[id]/[ix].tsx(1 hunks)apps/frontend-pwa/src/pages/course/[courseId]/microlearning/[id]/evaluation.tsx(1 hunks)apps/frontend-pwa/src/pages/course/[courseId]/microlearning/[id]/index.tsx(1 hunks)apps/frontend-pwa/src/pages/group/[groupId]/activity/[activityId].tsx(1 hunks)apps/frontend-pwa/src/pages/index.tsx(1 hunks)packages/graphql/src/graphql/ops/QGetCourseQuizAnalytics.graphql(1 hunks)packages/graphql/src/ops.schema.json(4 hunks)packages/graphql/src/ops.ts(7 hunks)packages/graphql/src/public/client.json(1 hunks)packages/graphql/src/public/schema.graphql(4 hunks)packages/graphql/src/public/server.json(1 hunks)packages/graphql/src/schema/analytics.ts(2 hunks)packages/graphql/src/schema/query.ts(2 hunks)packages/graphql/src/services/analytics.ts(2 hunks)packages/i18n/messages/de.ts(1 hunks)packages/i18n/messages/en.ts(1 hunks)packages/types/src/index.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
✅ Files skipped from review due to trivial changes (7)
- apps/frontend-manage/src/pages/analytics/[courseId]/activity.tsx
- apps/frontend-manage/src/components/analytics/AnalyticsLoadingView.tsx
- apps/frontend-manage/src/components/analytics/AnalyticsErrorView.tsx
- apps/frontend-pwa/src/pages/index.tsx
- apps/frontend-pwa/src/pages/course/[courseId]/index.tsx
- apps/frontend-manage/src/components/analytics/AnalyticsSearchField.tsx
- apps/frontend-pwa/src/pages/course/[courseId]/microlearning/[id]/evaluation.tsx
🔇 Additional comments (35)
apps/frontend-manage/src/pages/analytics/index.tsx (1)
Line range hint 9-32: LGTM! Clean and well-structured component
The component follows React best practices:
- Proper data fetching with Apollo Client
- Clean loading state handling
- Consistent layout structure
Let's verify that the router removal didn't impact any navigation features:
✅ Verification successful
Navigation functionality is properly implemented and unaffected by router changes
The router usage in the analytics section is working as expected:
- The main Analytics page correctly renders the course list without router dependency
- Navigation is properly handled through the DashboardButtons component using Next.js router
- All navigation paths (
/analytics/${courseId}/[activity|performance|quizzes]) are correctly structured
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining router usage in analytics-related components
# that might have been affected by the router removal
# Search for router-related patterns in analytics components
rg -t tsx -t ts "useRouter|router\." "apps/frontend-manage/src/components/analytics"
# Check for any navigation-related onClick handlers that might need the router
ast-grep --pattern 'onClick={$_}' "apps/frontend-manage/src/components/analytics"
Length of output: 194
Script:
#!/bin/bash
# Let's fix the previous script and broaden the search
# Search for router-related patterns in analytics components and pages
rg "useRouter|router\." apps/frontend-manage/src/pages/analytics
rg "useRouter|router\." apps/frontend-manage/src/components
# Check for any navigation-related onClick handlers in analytics components
rg "onClick" apps/frontend-manage/src/pages/analytics
rg "onClick" apps/frontend-manage/src/components/analytics
# Check if CourseDashboardList has any navigation logic
rg -A 5 "CourseDashboardList" apps/frontend-manage/src
Length of output: 12068
Script:
#!/bin/bash
# Let's check the CourseDashboardList component implementation for any navigation logic
cat apps/frontend-manage/src/components/analytics/overview/CourseDashboardList.tsx
# Also check DashboardButtons implementation since it handles navigation
cat apps/frontend-manage/src/components/analytics/overview/DashboardButtons.tsx
Length of output: 2935
packages/graphql/src/ops.ts (2)
3714-3716: LGTM! Query variables are properly typed.
The query variables type is correctly defined with a required courseId parameter.
4231-4231: LGTM! Verify the query implementation.
The GraphQL document is well-structured and includes all necessary fields. The query follows the same patterns as other queries in the file.
Let's verify that the query implementation matches the schema:
✅ Verification successful
LGTM! The query implementation matches the schema definition.
The verification confirms that:
- The
getCourseQuizAnalyticsfield is properly defined in the schema with correct arguments and return type - The
CourseQuizAnalyticstype exists in the schema with all the required fields - The query implementation in
ops.tsmatches the schema definition - The resolver is properly implemented in
schema/query.tsandservices/analytics.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the getCourseQuizAnalytics field exists in the schema
ast-grep --pattern 'type Query {
$$$
getCourseQuizAnalytics$_($_): CourseQuizAnalytics
$$$
}'
# Verify the return type exists in the schema
ast-grep --pattern 'type CourseQuizAnalytics {
$$$
activityFeedbacks: [ActivityFeedback!]!
instanceFeedbacks: [InstanceFeedback!]!
$$$
}'
Length of output: 249
Script:
#!/bin/bash
# Let's try to find the schema file first
fd "schema.graphql|schema.gql"
# Search for type definitions in any GraphQL files
rg "type (Query|CourseQuizAnalytics|ActivityFeedback|InstanceFeedback)" -A 5 --type graphql
# Search for resolver implementation
rg "getCourseQuizAnalytics" -A 5 --type typescript
Length of output: 287
Script:
#!/bin/bash
# Now that we found the schema file, let's examine its contents
cat packages/graphql/src/public/schema.graphql | grep -A 10 "type Query" || true
cat packages/graphql/src/public/schema.graphql | grep -A 10 "type CourseQuizAnalytics" || true
# Let's also search for the resolver implementation without file type restriction
rg "getCourseQuizAnalytics" -A 5
Length of output: 27599
apps/frontend-manage/src/pages/analytics/[courseId]/quizzes.tsx (2)
7-10: Check updated import paths.
You switched to relative paths for these components, which aligns them with the updated directory structure. Ensure consistency across your codebase so all references to analytics components match these relative paths.
16-20: Validate that courseId is always defined before making the query.
Using "skip: !courseId" avoids extraneous fetch calls, which is good. Just double-check that no code attempts to render data that might be undefined if the skip logic is triggered.
apps/frontend-manage/src/components/analytics/quiz/ActivityInstanceFeedbacksPlot.tsx (1)
11-12: Re-check your Recharts usage for zero-state or error states.
While you do handle empty results (no entries), ensure that any Recharts usage also gracefully handles unexpected feedback data (e.g., null or undefined). This is often addressed with fallback logic or optional chaining.
packages/graphql/src/services/analytics.ts (2)
282-339: Check for concurrency in feedback aggregation.
The aggregateInstanceFeedbacks function looks correct for synchronous collection. However, if there's any chance that data about stacks or elements might be handled in parallel in the future (e.g., Promise.all for performance), be mindful that reassigning accumulators or mutating shared objects might cause race conditions. For now, it’s fine if everything is synchronous.
390-482: Ensure participant privacy.
When returning feedback info at the activity or instance level, confirm that no personally identifiable data (like user IDs) are included. Everything looks aggregated, but keep an eye on additional expansions or fields that might leak PII.
packages/graphql/src/graphql/ops/QGetCourseQuizAnalytics.graphql (1)
1-23: GraphQL query shape looks good.
You’ve introduced a clear query with typed variables. Ensure the schema definitions in your GraphQL server (for getCourseQuizAnalytics) are kept consistent. Also, verify you’re returning only what the frontend needs to limit overfetching.
apps/frontend-manage/src/components/analytics/StackedBarChartLabel.tsx (1)
1-29: LGTM! Well-structured component
The component is well-implemented with proper typing and positioning calculations.
apps/frontend-manage/src/components/analytics/quiz/QuizAnalyticsNavigation.tsx (1)
1-16: LGTM! Clean navigation implementation
The component is well-structured and properly integrates with the analytics navigation system.
apps/frontend-manage/src/components/analytics/quiz/useElementFeedbackSearch.ts (1)
1-28: LGTM! Well-implemented search hook
The hook effectively implements search functionality with proper memoization and type safety.
apps/frontend-manage/src/components/analytics/ElementTypeFilter.tsx (1)
Line range hint 6-31: LGTM! Verify component renaming impact.
The component renaming and implementation look good. The code is well-structured with proper typing and translations.
Let's verify all usages of the old component name have been updated:
✅ Verification successful
Component renaming is properly implemented
The component renaming from PerformanceElementTypeFilter to ElementTypeFilter has been correctly implemented. All imports and usages in the codebase are using the new name consistently. The component is properly used in both PerformanceRates.tsx and ActivityInstanceFeedbacksPlot.tsx with correct props.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old component name
rg "PerformanceElementTypeFilter"
# Find all imports of ElementTypeFilter to verify correct usage
rg "import.*ElementTypeFilter"
Length of output: 336
Script:
#!/bin/bash
# Check the content of files importing ElementTypeFilter to verify correct usage
rg -A 5 "ElementTypeFilter" apps/frontend-manage/src/components/analytics/performance/PerformanceRates.tsx apps/frontend-manage/src/components/analytics/quiz/ActivityInstanceFeedbacksPlot.tsx
Length of output: 3089
apps/frontend-manage/src/components/analytics/quiz/ElementFeedbackBarChart.tsx (2)
4-11: LGTM! Props interface is well-defined.
The interface clearly defines the expected props with appropriate types.
27-55: Verify chart responsiveness.
The chart uses ResponsiveContainer which is good, but we should ensure it behaves well at different viewport sizes.
Let's check for any related responsive layout issues:
✅ Verification successful
Chart responsiveness appears to be properly implemented
The implementation shows good responsive design practices:
- The chart is wrapped in a
ResponsiveContainerwithwidth="100%"and fixedheight={35} - The parent component uses a flex layout with proper constraints (
max-h-[13rem]andoverflow-y-scroll) - Similar responsive chart patterns are consistently used across other analytics components
- The chart is vertically compact (35px height) and horizontally fluid, which is appropriate for its use case as a list item
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related responsive container usages and potential issues
rg -A 3 "ResponsiveContainer.*width=\"100%\""
# Look for any reported issues with similar chart implementations
rg -l "BarChart.*layout=\"vertical\"" | xargs rg "TODO|FIXME|BUG"
Length of output: 3729
Script:
#!/bin/bash
# Let's check for any media queries or responsive styles applied to these charts
rg -A 5 "ElementFeedbackBarChart|StackedProgress|PerformanceRatesBarChart" "apps/frontend-manage/src/**/*.{css,scss,styled.ts,styles.ts}"
# Check for any container components or wrappers that might affect the chart's responsiveness
ast-grep --pattern 'const $_ = styled.$_`
$$$
@media $$ {
$$$
}
$$$
`'
# Look for any test files that might verify responsive behavior
fd -e test.tsx -e spec.tsx | xargs rg "ElementFeedbackBarChart|StackedProgress|PerformanceRatesBarChart"
Length of output: 2522
Script:
#!/bin/bash
# Let's check the parent components to understand the layout context
rg -B 5 -A 10 "<ElementFeedbackBarChart"
# Check if there are any width/height constraints from parent components
ast-grep --pattern 'const $_ = styled.div`
$$$
width: $_
$$$
`' apps/frontend-manage/src/components/analytics/quiz/
# Look for any flex or grid container usage that might affect the chart
rg "display: (flex|grid)" apps/frontend-manage/src/components/analytics/quiz/
Length of output: 2300
apps/frontend-manage/src/components/analytics/performance/PerformanceRatesBarChart.tsx (1)
2-2: LGTM! Good refactoring of the label component.
The extraction of StackedBarChartLabel improves code reusability and maintainability.
Also applies to: 39-39, 49-49, 59-59
apps/frontend-manage/src/pages/analytics/[courseId]/performance.tsx (1)
7-13: LGTM! Import paths are consistently updated.
The relative import paths are correctly updated and maintain consistency with the project structure.
apps/frontend-manage/src/components/analytics/performance/PerformanceRates.tsx (3)
15-18: LGTM! Component renames improve reusability
The renaming of components from specific prefixes (e.g., Performance) to more generic names improves their reusability across different contexts while maintaining their functionality.
82-84: LGTM! Button padding adjustment
The padding adjustment improves the button's visual consistency.
110-117: LGTM! Consistent usage of renamed components
The implementation correctly uses the renamed components (ActivityTypeFilter, AnalyticsSearchField, ElementTypeFilter) with consistent props.
Also applies to: 126-134
apps/frontend-pwa/src/pages/group/[groupId]/activity/[activityId].tsx (1)
25-25: LGTM: Import path update.
The change from alias path to relative path is consistent with standard practices.
packages/graphql/src/public/client.json (1)
123-123: LGTM!
The hash entry for GetCourseQuizAnalytics is correctly added and follows the established pattern.
packages/graphql/src/schema/query.ts (2)
17-17: LGTM!
The CourseQuizAnalytics type is correctly imported alongside other analytics types.
785-794: LGTM!
The getCourseQuizAnalytics query field is well-structured:
- Properly secured with user authentication
- Follows the established pattern of other analytics queries
- Correctly delegates to the analytics service
packages/graphql/src/public/schema.graphql (4)
27-33: LGTM!
The ActivityFeedback type is well-defined with appropriate fields for tracking feedback metrics at the activity level.
331-336: LGTM!
The CourseQuizAnalytics type is well-structured:
- Includes both activity and instance level feedbacks
- Uses non-null lists to ensure data consistency
- Provides course name and total participants for context
811-818: LGTM!
The InstanceFeedback type is well-defined:
- Captures both instance metadata and feedback metrics
- Uses appropriate enums for activity and element types
1348-1348: LGTM!
The query field definition matches its implementation in query.ts and follows GraphQL naming conventions.
packages/i18n/messages/en.ts (1)
1877-1879: LGTM! New translation keys added for analytics features.
The new translation keys are properly structured and follow consistent naming patterns for feedback-related metrics.
packages/graphql/src/public/server.json (1)
123-123: LGTM! New GraphQL query added for course quiz analytics.
The query is well-structured with proper parameter validation and returns comprehensive feedback metrics at both instance and activity levels.
packages/i18n/messages/de.ts (1)
1891-1893: LGTM! The translations are accurate and consistent.
The new German translations for feedback analytics are well-structured and maintain consistency with the existing translations.
Let's verify the consistency with the English translations:
✅ Verification successful
German translations are accurate and consistent with English counterparts
The verification confirms that:
- The English translations exist in
en.tswith matching keys - German translations accurately convey the meaning:
- "Negative Feedback Overview" → "Übersicht Negative Bewertungen"
- "Upvotes" → "Positive Bewertungen"
- "Downvotes" → "Negative Bewertungen"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the English translations exist and match the structure
# Look for the corresponding keys in the English translation file
rg "negativeFeedbackOverview|upvotes|downvotes" packages/i18n/messages/en.ts
Length of output: 196
packages/graphql/src/ops.schema.json (4)
249-340: Well-structured ActivityFeedback type definition!
The ActivityFeedback type is well-designed with:
- Clear and descriptive field names
- Appropriate use of non-null constraints
- Good type choices for the fields (String for id, Float for rates)
3862-3953: Well-designed CourseQuizAnalytics type!
The CourseQuizAnalytics type effectively aggregates quiz analytics with:
- Comprehensive feedback arrays (activityFeedbacks and instanceFeedbacks)
- Proper non-null constraints
- Clear field naming
9025-9132: Well-structured InstanceFeedback type definition!
The InstanceFeedback type is well-designed with:
- Appropriate fields for instance-level feedback
- Consistent use of non-null constraints
- Clear separation from ActivityFeedback type
19873-19901: Well-defined getCourseQuizAnalytics query!
The query is properly structured with:
- Required courseId parameter
- Appropriate return type (CourseQuizAnalytics)
- Consistent with GraphQL best practices
apps/frontend-manage/src/components/analytics/quiz/ElementFeedbackBarChart.tsx
Outdated
Show resolved
Hide resolved
| import Layout from '../../../../../components/Layout' | ||
| import PreviewMessage from '../../../../../components/common/PreviewMessage' | ||
| import MicroLearningSubscriber from '../../../../../components/microLearning/MicroLearningSubscriber' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using path aliases instead of deep relative paths
Multiple deep relative paths (../../../../../) make the code harder to maintain. Consider using path aliases for all these imports:
-import Layout from '../../../../../components/Layout'
-import PreviewMessage from '../../../../../components/common/PreviewMessage'
-import MicroLearningSubscriber from '../../../../../components/microLearning/MicroLearningSubscriber'
+import Layout from '~/components/Layout'
+import PreviewMessage from '~/components/common/PreviewMessage'
+import MicroLearningSubscriber from '~/components/microLearning/MicroLearningSubscriber'Would you like me to create a GitHub issue to track the systematic implementation of path aliases across the codebase?
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/graphql/src/services/analytics.ts (1)
341-388: 🛠️ Refactor suggestionConsider weighted averaging and simplify the implementation.
The current implementation uses unweighted averaging, which might not represent the true feedback distribution. Additionally, the implementation could be simplified.
Consider this refactoring:
function aggregateActivityFeedbacks({ instanceFeedbacks, activityType, activityId, activityName, }: { instanceFeedbacks: InstanceFeedback[] activityType: ActivityType activityId: string activityName: string }) { if (instanceFeedbacks.length === 0) { return undefined } + // Calculate total votes across all instances for weighted averaging + const totalVotes = instanceFeedbacks.reduce((sum, feedback) => sum + feedback.totalVotes, 0) + + // Skip if no votes + if (totalVotes === 0) return undefined + + // Calculate weighted averages + const { upvoteRate, downvoteRate } = instanceFeedbacks.reduce( + (acc, feedback) => ({ + upvoteRate: acc.upvoteRate + (feedback.upvoteRate * feedback.totalVotes / totalVotes), + downvoteRate: acc.downvoteRate + (feedback.downvoteRate * feedback.totalVotes / totalVotes), + }), + { upvoteRate: 0, downvoteRate: 0 } + ) return { id: activityId, activityType, activityName, upvoteRate, downvoteRate, - count: instanceFeedbacks.length, }
🧹 Nitpick comments (2)
packages/graphql/src/services/analytics.ts (2)
282-339: Consider improving readability and robustness of the feedback aggregation.The function could benefit from the following improvements:
- Add input validation for null/undefined stacks
- Simplify the nested data transformations
- Make the division by zero handling more explicit
Consider this refactoring:
function aggregateInstanceFeedbacks({ stacks, activityType, }: { stacks: (ElementStack & { elements: (ElementInstance & { feedbacks: ElementFeedback[] })[] })[] activityType: ActivityType }): InstanceFeedback[] { + if (!stacks?.length) return [] + + // First, flatten the structure to get all elements with their feedbacks + const elements = stacks.flatMap(stack => stack.elements) + + // Then, process each element's feedbacks + return elements + .map(element => { + const votes = element.feedbacks.reduce( + (acc, feedback) => ({ + upvotes: acc.upvotes + (feedback.upvote ? 1 : 0), + downvotes: acc.downvotes + (feedback.downvote ? 1 : 0), + }), + { upvotes: 0, downvotes: 0 } + ) + + const totalVotes = votes.upvotes + votes.downvotes + + // Skip elements with no votes + if (totalVotes === 0) return null + + return { + id: element.id, + instanceName: element.elementData.name, + instanceType: element.elementData.type, + activityType, + ...votes, + totalVotes, + upvoteRate: votes.upvotes / totalVotes, + downvoteRate: votes.downvotes / totalVotes, + } + }) + .filter((feedback): feedback is InstanceFeedback => feedback !== null)
390-484: Consider optimizing the implementation and adding error handling.The function could benefit from the following improvements:
- Add error handling for the prisma query
- Abstract the repeated logic for quizzes and micro learnings
- Consider optimizing the prisma query by selecting only needed fields
Consider this refactoring:
export async function getCourseQuizAnalytics( { courseId }: { courseId: string }, ctx: ContextWithUser ) { + try { const course = await ctx.prisma.course.findUnique({ where: { id: courseId, ownerId: ctx.user.sub }, include: { participations: { + select: { id: true }, // Only need count + }, practiceQuizzes: { include: { stacks: { include: { elements: { include: { feedbacks: true, }, }, }, }, }, orderBy: { createdAt: 'desc' }, }, // ... similar optimization for microLearnings }, }) if (!course) { return null } + // Abstract the feedback aggregation logic + const aggregateFeedbacks = ( + activities: typeof course.practiceQuizzes | typeof course.microLearnings, + activityType: ActivityType + ) => { + const instanceFeedbacks: InstanceFeedback[] = [] + const activityFeedbacks: ActivityFeedback[] = [] + + activities.forEach((activity) => { + const activityInstanceFeedbacks = aggregateInstanceFeedbacks({ + stacks: activity.stacks, + activityType, + }) + instanceFeedbacks.push(...activityInstanceFeedbacks) + + const activityFeedback = aggregateActivityFeedbacks({ + instanceFeedbacks: activityInstanceFeedbacks, + activityType, + activityId: activity.id, + activityName: activity.name, + }) + + if (activityFeedback) { + activityFeedbacks.push(activityFeedback) + } + }) + + return { instanceFeedbacks, activityFeedbacks } + } + // Aggregate feedbacks for both types of activities + const quizFeedbacks = aggregateFeedbacks( + course.practiceQuizzes, + ActivityType.PRACTICE_QUIZ + ) + const microFeedbacks = aggregateFeedbacks( + course.microLearnings, + ActivityType.MICRO_LEARNING + ) return { name: course.name, totalParticipants: course.participations.length, - instanceFeedbacks, - activityFeedbacks, + instanceFeedbacks: [ + ...quizFeedbacks.instanceFeedbacks, + ...microFeedbacks.instanceFeedbacks, + ], + activityFeedbacks: [ + ...quizFeedbacks.activityFeedbacks, + ...microFeedbacks.activityFeedbacks, + ], } + } catch (error) { + console.error('Error in getCourseQuizAnalytics:', error) + throw error + } }
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/graphql/src/ops.ts (1)
4212-4212: Consider using GraphQL fragments for better maintainability.The
GetCoursePerformanceAnalyticsDocumentquery is quite large. Consider extracting common field selections into reusable fragments, particularly for the feedback-related fields and performance metrics. This would improve maintainability and reduce duplication.Example structure:
fragment FeedbackFields on ActivityFeedback { __typename id activityType upvoteRate downvoteRate feedbackCount } # Then in the query: activityFeedbacks { ...FeedbackFields activityName }apps/frontend-manage/src/components/analytics/performance/ActivityInstanceFeedbacksPlot.tsx (1)
150-159: Consider extracting the feedback title resolution into a helper function.
Currently, you're switching on __typename in-line to pick between activityName and instanceName. Extracting this logic into a helper function or a small mapping could simplify readability and provide flexibility if new feedback types are introduced in the future.apps/frontend-manage/src/components/analytics/performance/useElementFeedbackFilters.ts (2)
10-30: Consider adding explicit return type annotation.The function's return type should be explicitly defined for better type safety and documentation.
-function useElementFeedbackFilters({ +function useElementFeedbackFilters({ type, activityFeedbacks, instanceFeedbacks, activityType, elementType, activitySearchTerm, instanceSearchTerm, activitySearch, instanceSearch, -}: { +}: { type: 'activity' | 'instance' activityFeedbacks: ActivityFeedback[] instanceFeedbacks: InstanceFeedback[] activityType: ActivityType | 'all' elementType: ElementType | 'all' activitySearchTerm: string instanceSearchTerm: string activitySearch: Search instanceSearch: Search -}) { +}): ActivityFeedback[] | InstanceFeedback[] {
52-70: Extract search term sanitization to a utility function.The search term sanitization logic is duplicated. Consider extracting it to a reusable function.
+const sanitizeSearchTerm = (term: string): string => term?.trim().toLowerCase() ?? '' + } else { + if (!instanceFeedbacks?.length) return [] const rawData = instanceFeedbacks // filter by element type const typeFiltered = rawData.filter((feedback) => { if (elementType === 'all') { return true } return feedback.instanceType === elementType }) // filter by search word - const sanitizedSearchTerm = instanceSearchTerm.trim().toLowerCase() - if (sanitizedSearchTerm) { - return instanceSearch.search(sanitizedSearchTerm) as InstanceFeedback[] + const sanitizedSearchTerm = sanitizeSearchTerm(instanceSearchTerm) + if (sanitizedSearchTerm && instanceSearch) { + const searchResults = instanceSearch.search(sanitizedSearchTerm) + return searchResults.length ? searchResults as InstanceFeedback[] : typeFiltered }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/frontend-manage/src/components/analytics/performance/ActivityInstanceFeedbacksPlot.tsx(1 hunks)apps/frontend-manage/src/components/analytics/performance/ElementFeedbackBarChart.tsx(1 hunks)apps/frontend-manage/src/components/analytics/performance/useElementFeedbackFilters.ts(1 hunks)apps/frontend-manage/src/components/analytics/performance/useElementFeedbackSearch.ts(1 hunks)apps/frontend-manage/src/pages/analytics/[courseId]/performance.tsx(2 hunks)apps/frontend-manage/src/pages/analytics/[courseId]/quizzes.tsx(1 hunks)packages/graphql/src/graphql/ops/QGetCoursePerformanceAnalytics.graphql(1 hunks)packages/graphql/src/ops.schema.json(4 hunks)packages/graphql/src/ops.ts(5 hunks)packages/graphql/src/public/client.json(1 hunks)packages/graphql/src/public/schema.graphql(3 hunks)packages/graphql/src/public/server.json(1 hunks)packages/graphql/src/schema/analytics.ts(3 hunks)packages/graphql/src/services/analytics.ts(4 hunks)packages/types/src/index.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
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/graphql/src/public/client.json
- apps/frontend-manage/src/pages/analytics/[courseId]/performance.tsx
- apps/frontend-manage/src/pages/analytics/[courseId]/quizzes.tsx
- packages/graphql/src/public/server.json
- packages/graphql/src/ops.schema.json
🔇 Additional comments (10)
packages/graphql/src/ops.ts (2)
49-57: LGTM! Well-structured feedback type definitions.
The ActivityFeedback and InstanceFeedback types are well-defined with consistent naming and proper type annotations for all fields.
Also applies to: 887-896
372-375: LGTM! Consistent type extensions.
The new feedback-related fields are properly integrated into the CoursePerformanceAnalytics type, maintaining consistency with the existing structure.
packages/graphql/src/graphql/ops/QGetCoursePerformanceAnalytics.graphql (1)
56-74: Verify usage of instanceType in downstream code.
If "instanceType" is not used anywhere else, consider removing it from the query to reduce payload size. Otherwise, ensure you are leveraging it downstream.
apps/frontend-manage/src/components/analytics/performance/ElementFeedbackBarChart.tsx (1)
22-23: Verify that feedback rate is indeed a ratio instead of a percentage.
Rounding “downvoteRate” and “upvoteRate” after multiplying by 100 is correct for fractional rates. Ensure the source data is indeed 0–1 to avoid incorrect percentage displays.
packages/types/src/index.ts (1)
408-425: LGTM! Well-structured type definitions.
The new types InstanceFeedback and ActivityFeedback are well-defined and consistent with the existing type patterns in the codebase.
packages/graphql/src/schema/analytics.ts (1)
236-261: LGTM! Well-structured GraphQL schema changes.
The new GraphQL types and interface updates are consistent with the type definitions and follow the established patterns in the codebase.
Also applies to: 270-271, 293-298
packages/graphql/src/services/analytics.ts (3)
116-174: Consider weighted averaging for more accurate feedback representation.
The implementation correctly aggregates instance feedbacks, but consider weighing the feedback rates based on the number of votes to give more weight to instances with higher participation.
Line range hint 426-499: LGTM! Well-structured implementation.
The function properly handles error cases and efficiently combines both performance and feedback analytics.
204-209:
Fix weighted average calculation.
The current implementation of weighted average is incorrect. When combining rates, it's using the count of instances instead of the total votes.
Apply this fix:
- acc.upvoteRate =
- (acc.upvoteRate * acc.feedbackCount + instanceFeedback.upvoteRate) /
- (acc.feedbackCount + 1)
- acc.downvoteRate =
- (acc.downvoteRate * acc.feedbackCount + instanceFeedback.downvoteRate) /
- (acc.feedbackCount + 1)
+ const totalVotesSoFar = acc.feedbackCount * instanceFeedback.feedbackCount
+ const newTotalVotes = totalVotesSoFar + instanceFeedback.feedbackCount
+ acc.upvoteRate =
+ (acc.upvoteRate * totalVotesSoFar +
+ instanceFeedback.upvoteRate * instanceFeedback.feedbackCount) /
+ newTotalVotes
+ acc.downvoteRate =
+ (acc.downvoteRate * totalVotesSoFar +
+ instanceFeedback.downvoteRate * instanceFeedback.feedbackCount) /
+ newTotalVotesLikely invalid or redundant comment.
packages/graphql/src/public/schema.graphql (1)
27-34: LGTM! Well-defined types.
The new ActivityFeedback and InstanceFeedback types are well-structured and align perfectly with their implementation.
Also applies to: 807-815
apps/frontend-manage/src/components/analytics/performance/useElementFeedbackSearch.ts
Outdated
Show resolved
Hide resolved
apps/frontend-manage/src/components/analytics/performance/useElementFeedbackFilters.ts
Show resolved
Hide resolved
|
Summary by CodeRabbit
New Features
ActivityInstanceFeedbacksPlotandElementFeedbackBarChart.QuizAnalyticsNavigation, for improved course analytics navigation.instanceFeedbacksandactivityFeedbackshave been added to enhance performance analytics.Bug Fixes
Documentation
Chores