refactor: migrate live quiz evaluation to new database structure #4355
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request involve significant updates to several components related to quiz management and evaluation in the frontend application. Key modifications include the restructuring of status handling in Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (20)
🔥 Files not summarized due to errors (1)
💤 Files with no reviewable changes (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
🔇 Additional comments (16)apps/frontend-manage/src/pages/sessions/[id]/evaluation.tsx (1)
The migration to the new database structure has introduced new components. Let's verify that all imports are being used and remove any legacy imports. ✅ Verification successfulAll imports are actively used in the component Based on the search results, every imported module and component is being used within the file:
There are no unused imports from the previous implementation that need to be removed. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for unused imports from the previous implementation
rg --type typescript "import.*from.*@klicker-uzh/graphql" -A 5 apps/frontend-manage/src/pages/sessions/[id]/evaluation.tsx | grep -v "GetLiveQuizEvaluationDocument"
Length of output: 201 Script: #!/bin/bash
# Search for all imports in the evaluation.tsx file
rg "^import.*from" apps/frontend-manage/src/pages/sessions/[id]/evaluation.tsx
# Search for usage of each imported module/component in the file
rg "GetLiveQuizEvaluationDocument|ActivityEvaluation|Layout|Loader|UserNotification" apps/frontend-manage/src/pages/sessions/[id]/evaluation.tsx
Length of output: 1222 apps/frontend-manage/src/components/evaluation/navigation/EvaluationNavigation.tsx (3)
The new props are properly typed with clear optionality markers, following TypeScript best practices. Line range hint The changes effectively integrate new evaluation types and feature flags while maintaining component cohesion and type safety. The modifications align well with the PR's objective of migrating to a new database structure.
Please ensure that the StackNavigation component is properly updated to handle these new props. Let's verify the implementation: apps/frontend-manage/src/components/evaluation/ElementChart.tsx (2)Line range hint The
The import path for ✅ Verification successfulBased on the shell script results, I can generate the final response as we have comprehensive information about the import changes: Import path changes for TextSizeType are consistent across the codebase All imports of 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for any remaining imports from the old path and verify new imports
# Test 1: Search for old import path
echo "Checking for old imports..."
rg "from '\.\./sessions/evaluation/constants'" -l
# Test 2: Search for new import path usage
echo "Checking new imports..."
rg "from '\./textSizes'" -l
# Test 3: Search for TextSizeType usage to ensure all occurrences are updated
echo "Checking TextSizeType usage..."
rg "TextSizeType" -A 2
Length of output: 6074 apps/frontend-manage/src/components/evaluation/ActivityEvaluation.tsx (1)
The new imports and type definitions are properly organized and provide good type safety for the new features. Also applies to: 7-9, 14-16, 21-22, 28-29 packages/graphql/src/schema/evaluation.ts (2)
The new imports are correctly added and necessary for the feedback-related schema extensions.
The implementation looks correct, but let's ensure the referenced types are properly defined. ✅ Verification successfulLet me gather more specific information about these types. Let me gather more specific information about the implementation of these types. Let me check one final piece of information about the IFeedback interface. Based on the gathered information, I can now provide the final response: Referenced feedback types are properly defined and implemented The implementation is correct with both types properly defined:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify that the referenced types exist and are properly defined
# Check for FeedbackRef and ConfusionTimestepRef definitions
echo "Checking for FeedbackRef and ConfusionTimestepRef definitions..."
rg -l "FeedbackRef.*=.*builder\.(objectRef|enumType)" --type ts
rg -l "ConfusionTimestepRef.*=.*builder\.(objectRef|enumType)" --type ts
# Check for IFeedback interface definition
echo "Checking for IFeedback interface definition..."
rg -l "interface.*IFeedback.*{" --type ts
# Check for ConfusionTimestep type usage
echo "Checking for ConfusionTimestep type usage..."
rg -l "type.*ConfusionTimestep.*=" --type ts
Length of output: 750 Script: #!/bin/bash
# Check for FeedbackRef definition and its context
echo "=== Checking FeedbackRef definition and context ==="
rg "FeedbackRef.*=.*builder\." -B 2 -A 2 --type ts
# Check for ConfusionTimestepRef definition
echo "=== Checking ConfusionTimestepRef definition ==="
rg "ConfusionTimestepRef" -B 2 -A 2 --type ts
# Check for ConfusionTimestep type definition
echo "=== Checking ConfusionTimestep type definition ==="
rg "type.*ConfusionTimestep.*=" -B 2 -A 2 --type ts
# Check for any Feedback related types/interfaces
echo "=== Checking Feedback related types ==="
rg "interface.*Feedback|type.*Feedback" --type ts
Length of output: 230752 Script: #!/bin/bash
# Check for FeedbackRef implementation and its interface
echo "=== Checking FeedbackRef implementation ==="
rg "FeedbackRef.*implement" -B 2 -A 5 --type ts
# Check for ConfusionTimestepRef implementation
echo "=== Checking ConfusionTimestepRef implementation ==="
rg "ConfusionTimestepRef.*implement" -B 2 -A 5 --type ts
# Check for IFeedback interface definition
echo "=== Checking IFeedback interface definition ==="
rg "interface.*IFeedback.*{" -A 5 --type ts
# Check for imports in evaluation.ts
echo "=== Checking imports in evaluation.ts ==="
rg "import.*from.*" packages/graphql/src/schema/evaluation.ts
Length of output: 2208 Script: #!/bin/bash
# Check for IFeedback interface with fixed regex
echo "=== Checking IFeedback interface definition ==="
rg "export.*interface.*IFeedback" -A 5 --type ts
# Check for DB.Feedback type
echo "=== Checking DB.Feedback type ==="
rg "type.*Feedback.*=" -A 5 packages/types/src/index.ts
Length of output: 753 packages/graphql/src/schema/query.ts (2)
The removal of
While the implementation looks correct, please ensure:
Let's verify the HMAC validation implementation and type compatibility: packages/graphql/src/services/liveQuizzes.ts (1)
The evaluation results are returned without validation. Consider:
Let's verify the usage of computeStackEvaluation: packages/graphql/src/ops.ts (4)
The
The argument types are properly structured with consistent use of String input types and optional HMAC parameter. Line range hint The GraphQL documents are well-structured with:
The new fields are properly typed and the inclusion of HMAC suggests secure access control. Let's verify the HMAC implementation. ✅ Verification successfulHMAC implementation is secure and properly validated The HMAC implementation follows security best practices:
The implementation provides a secure way to authenticate access to quiz evaluations and leaderboards. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify HMAC implementation and usage
# Look for HMAC generation and validation logic
# Search for HMAC-related code
echo "Searching for HMAC implementation..."
rg -l "HMAC|hmac" --type ts --type tsx
# Search for crypto-related imports
echo "Checking crypto imports..."
rg -l "crypto|createHmac" --type ts --type tsx
Length of output: 291 Script: #!/bin/bash
# Let's try again without tsx extension and with proper context
echo "Searching for HMAC implementation..."
rg -l "HMAC|hmac" --type ts
echo "Checking crypto imports..."
rg -l "crypto|createHmac" --type ts
# Let's also search for specific HMAC generation patterns
echo "Searching for HMAC generation patterns..."
ast-grep --pattern 'createHmac($$$)'
# Search for validation patterns
echo "Searching for HMAC validation patterns..."
rg -B 3 -A 3 "\.hmac|HMAC" --type ts
Length of output: 29459 Script: #!/bin/bash
# Let's examine the HMAC implementation in liveQuizzes.ts
echo "Checking HMAC implementation details..."
rg -A 10 "getLiveQuizHMAC" packages/graphql/src/services/liveQuizzes.ts
# Let's also check where the HMAC is validated
echo "Checking HMAC validation..."
rg -B 3 -A 10 "hmac.*process\.env\.APP_SECRET" packages/graphql/src/services/liveQuizzes.ts
Length of output: 1463 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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (19)
packages/graphql/src/graphql/ops/QGetLiveQuizEvaluation.graphql (3)
1-2: LGTM! Consider adding documentation for HMAC usage.The query structure is well-defined with proper parameter typing. However, it would be helpful to document the purpose and requirements of the optional HMAC parameter.
Consider adding a comment explaining when and how the HMAC parameter should be used:
# Query to fetch live quiz evaluation data # @param id - Unique identifier for the quiz evaluation # @param hmac - Optional HMAC for secure access (required for <specify use case>)
3-6: Consider adding essential evaluation metadata fields.The basic evaluation fields might be missing crucial information needed for proper evaluation management and display.
Consider adding these commonly needed fields:
id name displayName description + status + startedAt + endedAt + duration
27-31: Consider adding aggregation fields for confusion metrics.The confusion feedback structure could benefit from aggregated statistics for better analysis.
Consider adding these fields:
confusionFeedbacks { speed difficulty createdAt + averageSpeed + averageDifficulty + totalResponses }apps/frontend-manage/src/components/courses/LiveQuizElement.tsx (1)
126-144: Enhance semantic structure with HTML5 elementsThe layout structure is clean, but could benefit from semantic HTML elements for better accessibility and SEO.
Consider this improvement:
-<div className="border-uzh-grey-80 w-full rounded border border-solid p-2"> +<article className="border-uzh-grey-80 w-full rounded border border-solid p-2"> <div className="flex w-full flex-row justify-between"> - <div className="flex-1"> + <header className="flex-1"> <div className="flex flex-row gap-2"> - <Ellipsis maxLength={50} className={{ markdown: 'text-base font-bold' }}> + <h2><Ellipsis maxLength={50} className={{ markdown: 'text-base font-bold' }}> {quiz.name} - </Ellipsis> + </Ellipsis></h2> </div> <div className="mb-1 text-sm italic">packages/graphql/src/schema/evaluation.ts (1)
156-163: LGTM: Schema implementation follows best practices.The implementation correctly exposes the new feedback fields with proper type references and nullability. Consider documenting the purpose of these fields using GraphQL descriptions for better schema documentation.
Add descriptions to the new fields for better API documentation:
feedbacks: t.expose('feedbacks', { type: [FeedbackRef], nullable: true, + description: 'Collection of general feedback provided during the activity', }), confusionFeedbacks: t.expose('confusionFeedbacks', { type: [ConfusionTimestepRef], nullable: true, + description: 'Timeline of confusion feedback collected during the activity', }),apps/frontend-manage/src/components/courses/PracticeQuizElement.tsx (2)
Line range hint
168-177: Consider adding error handling for deletion modal interactions.While the deletion implementation is well-structured, consider adding error handling for modal interactions to gracefully handle potential failures during the deletion process.
const deletionItem = { label: ( <div className="flex cursor-pointer flex-row items-center gap-1 text-red-600"> <FontAwesomeIcon icon={faTrashCan} className="w-[1.2rem]" /> <div>{t('manage.course.deletePracticeQuiz')}</div> </div> ), - onClick: () => setDeletionModal(true), + onClick: () => { + try { + setDeletionModal(true) + } catch (error) { + // Handle any potential errors in modal interaction + console.error('Failed to open deletion modal:', error) + // Consider showing a user-friendly error message + } + }, data: { cy: `delete-practice-quiz-${practiceQuiz.name}` }, }
Line range hint
179-329: Consider breaking down the component for better maintainability.The component has grown to handle multiple responsibilities, which could make maintenance more challenging. Consider:
- Extracting status-specific action menus into separate components
- Creating a dedicated hook for managing modal state and interactions
- Moving access link logic into a separate service
This would improve code organization and make the component more maintainable.
Example refactor for status-specific actions:
// components/PracticeQuizActions/DraftActions.tsx function DraftActions({ practiceQuiz, onDelete }) { return ( <> <PublishPracticeQuizButton practiceQuiz={practiceQuiz} /> <Dropdown items={[ // Draft-specific actions ]} /> </> ) } // PracticeQuizElement.tsx function PracticeQuizElement() { // ... existing code ... return ( <div> {practiceQuiz.status === PublicationStatus.Draft && ( <DraftActions practiceQuiz={practiceQuiz} onDelete={() => setDeletionModal(true)} /> )} {/* Other status-specific components */} </div> ) }packages/graphql/src/services/liveQuizzes.ts (2)
1692-1694: Improve authentication condition readability.The authentication condition is complex and could be more readable. Consider extracting the logic into a separate function or simplifying the condition.
-if ((!ctx.user?.sub && typeof hmac !== 'string') || hmac == '') { +const isAuthenticated = Boolean(ctx.user?.sub) +const hasValidHmac = typeof hmac === 'string' && hmac !== '' +if (!isAuthenticated && !hasValidHmac) { return null }
1760-1792: Consider extracting active block processing logic.The active block processing logic is complex and could be extracted into a separate function for better maintainability and testability.
async function processActiveBlock( liveQuiz: LiveQuiz & { activeBlock: ElementBlock & { elements: ElementInstance[] } }, ctx: Context ): Promise<(ElementBlock & { elements: ElementInstance[] }) | undefined> { if (!liveQuiz.activeBlockId || !liveQuiz.activeBlock) { return undefined } const activeInstanceIds = liveQuiz.activeBlock.elements.map( (instance) => instance.id ) const cachedResults = await getCachedBlockResults({ ctx, quizId: liveQuiz.id, blockId: liveQuiz.activeBlockId, activeInstanceIds, }) if (!cachedResults) { return undefined } const { instanceResults } = await processCachedData({ cachedResults, activeBlock: liveQuiz.activeBlock, }) return { ...liveQuiz.activeBlock, elements: liveQuiz.activeBlock.elements.map((instance) => ({ ...instance, anonymousResults: instanceResults[instance.id]?.anonymousResults ?? instance.anonymousResults, })), } }packages/graphql/src/services/stacks.ts (3)
2997-2998: Consider using optional chaining for property access.The current implementation uses type checking with the
inoperator. While this works, using optional chaining would make the code more concise and maintainable.- stackName: 'displayName' in stack ? stack.displayName : null, - stackDescription: 'description' in stack ? stack.description : null, + stackName: (stack as ElementStack)?.displayName ?? null, + stackDescription: (stack as ElementStack)?.description ?? null,
Line range hint
2779-2819: Improve type safety in numerical statistics computation.The function has been updated to accept a more specific type for results, but there are a few improvements that could be made:
- The return type should be explicitly defined
- The spread operator with
sdreturns a number array, which might not be intendedfunction computeNumericalStatistics( results: { value: number count: number correct?: boolean | null }[] +): { + max: number + mean: number + median: number + min: number + q1: number + q3: number + sd: number +} | null { const valueArray = results.reduce<number[]>((acc, { count, value }) => { const elements = Array(count).fill(value) return acc.concat(elements) }, []) return valueArray.length > 0 ? { max: max(valueArray), mean: mean(valueArray), median: median(valueArray), min: min(valueArray), q1: quantileSeq(valueArray, 0.25) as number, q3: quantileSeq(valueArray, 0.75) as number, - sd: std(valueArray) as number[], + sd: std(valueArray) as number, } : null }
2950-2950: Remove debug console.log statement.The console.log statement appears to be left from debugging and should be removed.
- console.log('COMPUTING INSTANCE EVALUATION')packages/graphql/src/public/server.json (1)
130-130: Well-structured schema changes for quiz evaluation.The new fragment
EvaluationResultsand queryGetLiveQuizEvaluationprovide a comprehensive evaluation structure with several strengths:
- Robust support for multiple question types (choices, free text, numerical, flashcard, content)
- Detailed analytics including total answers, anonymous responses, and type-specific metrics
- Security consideration through HMAC parameter for evaluation access control
Consider implementing caching for the evaluation results, especially for quizzes with many participants, to optimize performance and reduce database load.
packages/graphql/src/ops.schema.json (2)
123-142: Add descriptions for the confusionFeedbacks field and its type.Consider adding descriptive comments to improve API documentation and help consumers understand the purpose of this field.
{ "name": "confusionFeedbacks", - "description": null, + "description": "List of confusion feedback data points collected during the live quiz session", "args": [], "type": { "kind": "LIST",
167-186: Add descriptions for the feedbacks field and its type.Consider adding descriptive comments to improve API documentation and help consumers understand the purpose of this field.
{ "name": "feedbacks", - "description": null, + "description": "Collection of feedback responses submitted during the live quiz", "args": [], "type": { "kind": "LIST",packages/graphql/src/ops.ts (1)
4388-4388: Consider extracting common fields into shared fragments.The GraphQL documents are well-structured but contain significant duplication in field selections across different element types (ChoicesElement, FreeElement, etc.). Consider extracting common fields into shared fragments to improve maintainability.
Example refactor:
+ const CommonElementFields = gql` + fragment CommonElementFields on ElementInstanceEvaluation { + id + type + name + content + explanation + hasSampleSolution + hasAnswerFeedbacks + } + `; + const CommonResultsFields = gql` + fragment CommonResultsFields on ElementResults { + totalAnswers + anonymousAnswers + } + `; export const GetLiveQuizEvaluationDocument = { "kind": "Document", "definitions": [{ // ... existing definitions "selectionSet": { "selections": [ - { "kind": "Field", "name": { "kind": "Name", "value": "id" } }, - { "kind": "Field", "name": { "kind": "Name", "value": "type" } }, - // ... other repeated fields + { "kind": "FragmentSpread", "name": { "kind": "Name", "value": "CommonElementFields" } }, + { "kind": "FragmentSpread", "name": { "kind": "Name", "value": "CommonResultsFields" } }, // ... element-specific fields ] } }] }apps/frontend-manage/src/pages/sessions/[id]/evaluation.tsx (3)
Line range hint
15-22: Ensure 'hmac' parameter is defined before using in query variablesTo prevent potential runtime errors when
router.query.hmacis undefined, adjust theskipcondition to include!router.query.hmac.Apply this diff to adjust the
skipcondition:pollInterval: 5000, - skip: !router.query.id, + skip: !router.query.id || !router.query.hmac,
36-45: Wrap 'no data' return statement with<Layout>for consistencyFor consistent layout rendering and user experience, consider wrapping the return statement in the
if (!data)block with the<Layout>component, similar to theloadinganderrorstates.Apply this diff:
if (!data) { - return ( - <div className="flex h-full w-full flex-col items-center justify-center"> + return ( + <Layout> + <div className="flex h-full w-full flex-col items-center justify-center"> <UserNotification className={{ root: 'max-w-[80%] text-lg lg:max-w-[60%] 2xl:max-w-[50%]', }} message={t('manage.evaluation.evaluationNotYetAvailable')} /> - </div> - ) + </div> + </Layout> + )
49-49: Address the TODO: Add feedbacks and leaderboard toActivityEvaluationThe TODO comment indicates that feedbacks, confusion feedbacks, and leaderboard data should be added to the
ActivityEvaluationcomponent props. Would you like assistance in implementing this functionality?
🛑 Comments failed to post (4)
packages/graphql/src/graphql/ops/QGetLiveQuizEvaluation.graphql (2)
10-26: 🛠️ Refactor suggestion
Add user attribution and pagination for feedback responses.
The feedback structure is comprehensive but could benefit from additional fields and pagination support.
Consider these improvements:
feedbacks { id isPublished isPinned isResolved content votes + userId + userDisplayName resolvedAt createdAt - responses { + responses(first: Int, after: String) { + pageInfo { + hasNextPage + endCursor + } id createdAt content + userId + userDisplayName positiveReactions negativeReactions } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.feedbacks { id isPublished isPinned isResolved content votes userId userDisplayName resolvedAt createdAt responses(first: Int, after: String) { pageInfo { hasNextPage endCursor } id createdAt content userId userDisplayName positiveReactions negativeReactions } }
33-40: 🛠️ Refactor suggestion
Add pagination and additional statistics to leaderboard.
The leaderboard query should include pagination for scalability and additional statistics for richer display.
Consider these improvements:
- liveQuizLeaderboard(quizId: $id) { + liveQuizLeaderboard(quizId: $id, first: Int, after: String) { + pageInfo { + hasNextPage + endCursor + } id participantId rank username avatar score + correctAnswers + totalAnswers + accuracyPercentage }Committable suggestion skipped: line range outside the PR's diff.
apps/frontend-manage/src/components/courses/LiveQuizElement.tsx (1)
147-284: 🛠️ Refactor suggestion
Reduce code duplication in dropdown implementations
The dropdown implementation is repeated across different quiz states with similar configurations and actions.
Consider extracting the common dropdown logic into a reusable function:
const getDropdownItems = (status: PublicationStatus) => { const commonItems = [ getAccessLink({ href, setCopyToast, t, name: quiz.name }), ...(dataUser?.userProfile?.catalyst ? [getLTIAccessLink({ href, setCopyToast, t, name: quiz.name })] : []), getActivityDuplicationAction({ id: quiz.id, text: t('manage.sessions.duplicateSession'), wizardMode: WizardMode.LiveQuiz, router, data: { cy: `duplicate-live-quiz-${quiz.name}` }, }), ] const deleteAction = { label: ( <div className="flex cursor-pointer flex-row items-center gap-2 text-red-600"> <FontAwesomeIcon icon={faTrashCan} /> <div>{t('manage.sessions.deleteSession')}</div> </div> ), onClick: () => setDeletionModal(true), data: { cy: `delete-live-quiz-${quiz.name}` }, } switch (status) { case PublicationStatus.Draft: case PublicationStatus.Scheduled: return [...commonItems, deleteAction] case PublicationStatus.Published: return commonItems case PublicationStatus.Ended: return [deleteAction, ...commonItems] default: return commonItems } }Then use it in the render:
<Dropdown data={{ cy: `live-quiz-actions-${quiz.name}` }} className={{ item: 'p-1 hover:bg-gray-200', viewport: 'bg-white', }} trigger={t('manage.course.otherActions')} items={getDropdownItems(quiz.status)} triggerIcon={faHandPointer} />packages/graphql/src/services/liveQuizzes.ts (1)
1749-1758:
⚠️ Potential issueEnhance security measures in HMAC verification.
Several security improvements can be made:
- Use strict equality (
===) for HMAC comparison- Validate environment variable presence
- Consider using a timing-safe comparison function
if (typeof hmac === 'string') { + if (!process.env.APP_SECRET) { + throw new Error('APP_SECRET environment variable is not configured') + } + const hmacEncoder = createHmac('sha256', process.env.APP_SECRET as string) hmacEncoder.update(liveQuiz.namespace + liveQuiz.id) const quizHmac = hmacEncoder.digest('hex') // evaluate whether the hashed liveQuiz.namespace and liveQuiz.id equals the hmac - if (quizHmac !== hmac) { + if (!timingSafeEqual(Buffer.from(quizHmac), Buffer.from(hmac))) { return null } }Don't forget to import the required function:
import { timingSafeEqual } from 'crypto'
…e quiz evaluation
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
new-live-quiz-evaluation
|
| Run status |
|
| Run duration | 11m 14s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
140
|
| View all changes introduced in this branch ↗︎ | |
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
apps/frontend-manage/src/components/evaluation/hooks/useVisibleStacks.tsx (1)
23-26: Consider enhancing type safety and maintainabilityWhile the conditional translation key selection works, there are a few improvements that could make the code more robust:
- Consider using a constant or enum for 'LiveQuiz' to prevent typos
- Consider extracting translation keys as constants
- Consider handling all possible types explicitly for better maintainability
Here's a suggested improvement:
+const EVALUATION_TYPES = { + LIVE_QUIZ: 'LiveQuiz' as const, +} as const; + +const TRANSLATION_KEYS = { + BLOCK: 'shared.generic.blockN', + STACK: 'shared.generic.stackN', +} as const; // In the useMemo -label: t( - type === 'LiveQuiz' ? 'shared.generic.blockN' : 'shared.generic.stackN', - { number: String(index + 1) } -), +label: t( + type === EVALUATION_TYPES.LIVE_QUIZ + ? TRANSLATION_KEYS.BLOCK + : TRANSLATION_KEYS.STACK, + { number: String(index + 1) } +),apps/frontend-manage/src/components/evaluation/navigation/EvaluationNavigation.tsx (1)
Line range hint
8-17: Consider adding JSDoc documentationTo improve maintainability, consider adding JSDoc documentation to the interface explaining the purpose and possible values of the
typeprop.interface EvaluationNavigationProps { stacks: StackEvaluation[] stackInstanceMap: Record<number, { label: string; value: number }[]> activeStack: ActiveStackType setActiveStack: (stack: ActiveStackType) => void activeInstance: number setActiveInstance: (instance: number) => void numOfInstances: number + /** Type of activity evaluation (e.g., 'Synchronous' or 'Asynchronous') */ type: ActivityEvaluationType }apps/frontend-manage/src/components/evaluation/navigation/StackNavigation.tsx (2)
28-28: Consider adding prop validation and documentation.While the type is correctly added, consider enhancing the implementation with:
- PropTypes validation for runtime type checking
- JSDoc documentation to describe the expected values and purpose of the
typeprop+ import PropTypes from 'prop-types' + /** + * @param {Object} props + * @param {ActivityEvaluationType} props.type - The type of activity evaluation + */ function StackNavigation({ stacks, activeStack, setActiveStack, setActiveInstance, stackInstanceMap, type, }: StackNavigationProps) {
Line range hint
38-120: Enhance accessibility with ARIA labels and keyboard navigation.The navigation implementation is solid, but consider improving accessibility:
- Add ARIA labels to navigation buttons
- Implement keyboard navigation support (left/right arrows)
<Button basic + aria-label="Navigate to previous stack" onClick={() => { const newActiveStack = typeof activeStack === 'number' ? Math.max(activeStack - 1, 0) : 0 setActiveStack(newActiveStack) setActiveInstance(stackInstanceMap[newActiveStack][0].value) }} disabled={ stacks.length <= 2 * width + 1 || (typeof activeStack === 'number' && activeStack - width <= 0) } data={{ cy: 'evaluate-previous-block' }} > // Add similar aria-label to the next button and stack selection buttons + // Add keyboard navigation + useEffect(() => { + const handleKeyDown = (e: KeyboardEvent) => { + if (e.key === 'ArrowLeft' && !prevButtonDisabled) { + handlePrevClick() + } else if (e.key === 'ArrowRight' && !nextButtonDisabled) { + handleNextClick() + } + } + document.addEventListener('keydown', handleKeyDown) + return () => document.removeEventListener('keydown', handleKeyDown) + }, [activeStack, stacks.length])apps/frontend-manage/src/components/evaluation/ActivityEvaluation.tsx (3)
28-30: Consider adding JSDoc comments for better documentation.The props interface is well-structured, but adding JSDoc comments would improve code maintainability by documenting the purpose of each prop.
interface ActivityEvaluationProps { activityName: string stacks: StackEvaluation[] + /** Optional array of user feedbacks for live quizzes */ feedbacks?: Feedback[] | null + /** Optional array of confusion timesteps for live quizzes */ confusionFeedbacks?: ConfusionTimestep[] | null + /** Type of activity evaluation, defaults to 'Asynchronous' */ type?: ActivityEvaluationType }
48-49: TODO comment needs more context.The TODO comment about PPT integration lacks implementation details. Consider adding more specific requirements and acceptance criteria.
Would you like me to help create a GitHub issue to track this TODO with detailed requirements for the PowerPoint integration feature?
Line range hint
130-171: Consider refactoring to reduce code duplication.The feedback and confusion sections share similar structure and could be refactored into a reusable component to improve maintainability.
Consider extracting a common component:
interface FeedbackSectionProps<T> { isVisible: boolean data: T[] | null | undefined emptyMessage: string renderContent: (data: T[]) => React.ReactNode } function FeedbackSection<T>({ isVisible, data, emptyMessage, renderContent }: FeedbackSectionProps<T>) { if (!isVisible || data === null) return null; return ( <div className="overflow-y-auto"> <div className="border-t p-4"> <div className="mx-auto max-w-5xl text-xl"> {data && data.length > 0 ? ( renderContent(data) ) : ( <UserNotification className={{ message: 'text-lg' }} type="warning" message={emptyMessage} /> )} </div> </div> </div> ); }Usage:
<FeedbackSection isVisible={type === 'LiveQuiz' && activeStack === 'feedbacks'} data={feedbacks} emptyMessage={t('manage.evaluation.noFeedbacksYet')} renderContent={(data) => ( <EvaluationFeedbacks feedbacks={data} sessionName={activityName} /> )} />packages/graphql/src/services/liveQuizzes.ts (2)
1688-1694: Enhance input validation robustness.Consider combining the empty string check with the type check for better validation.
-if ((!ctx.user?.sub && typeof hmac !== 'string') || hmac == '') { +if (!ctx.user?.sub && (!hmac || typeof hmac !== 'string' || hmac === '')) { return null }
1794-1813: Document return type and address TODOs.The function has pending TODOs and lacks return type documentation. Consider:
- Implementing leaderboard functionality
- Adding logic for conditional feedback return
- Adding JSDoc documentation for the return type
Would you like me to help implement the leaderboard functionality or create GitHub issues to track these TODOs?
🛑 Comments failed to post (2)
packages/graphql/src/services/liveQuizzes.ts (2)
1749-1759:
⚠️ Potential issueAdd validation for APP_SECRET environment variable.
The HMAC verification assumes APP_SECRET is defined. Missing validation could lead to security issues if the environment variable is not set.
if (typeof hmac === 'string') { + if (!process.env.APP_SECRET) { + throw new Error('APP_SECRET environment variable is not configured') + } const hmacEncoder = createHmac('sha256', process.env.APP_SECRET as string) hmacEncoder.update(liveQuiz.namespace + liveQuiz.id) const quizHmac = hmacEncoder.digest('hex')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if (typeof hmac === 'string') { if (!process.env.APP_SECRET) { throw new Error('APP_SECRET environment variable is not configured') } const hmacEncoder = createHmac('sha256', process.env.APP_SECRET as string) hmacEncoder.update(liveQuiz.namespace + liveQuiz.id) const quizHmac = hmacEncoder.digest('hex') // evaluate whether the hashed liveQuiz.namespace and liveQuiz.id equals the hmac if (quizHmac !== hmac) { return null } }
1760-1792:
⚠️ Potential issueAdd error handling for cache operations.
The cache operations could fail silently. Consider adding proper error handling.
const cachedResults = await getCachedBlockResults({ ctx, quizId: id, blockId: liveQuiz.activeBlockId, activeInstanceIds, -}) +}).catch((error) => { + console.error('Failed to get cached block results:', error) + return null +})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// load results from active block as well let activeBlockWithResults: | (ElementBlock & { elements: ElementInstance[] }) | undefined if (liveQuiz.activeBlockId && liveQuiz.activeBlock) { const activeInstanceIds = liveQuiz.activeBlock.elements.map( (instance) => instance.id ) const cachedResults = await getCachedBlockResults({ ctx, quizId: id, blockId: liveQuiz.activeBlockId, activeInstanceIds, }).catch((error) => { console.error('Failed to get cached block results:', error) return null }) if (cachedResults) { const { instanceResults } = await processCachedData({ cachedResults, activeBlock: liveQuiz.activeBlock, }) activeBlockWithResults = { ...liveQuiz.activeBlock, elements: liveQuiz.activeBlock.elements.map((instance) => ({ ...instance, anonymousResults: instanceResults[instance.id]?.anonymousResults ?? instance.anonymousResults, })), } } }
|
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (16)
apps/frontend-manage/src/pages/sessions/[id]/evaluation.tsx (6)
Line range hint
14-23: Consider optimizing polling strategy.The current implementation polls every 5 seconds, which might create unnecessary load on both client and server. Consider implementing:
- WebSocket connection for real-time updates
- Dynamic polling intervals based on activity status
- Manual refresh option as a fallback
Would you like assistance in implementing any of these alternatives?
24-30: Enhance loading state with contextual information.While the loader works, consider providing more context to improve user experience.
<Layout> - <Loader /> + <div className="flex flex-col items-center gap-4"> + <Loader /> + <div className="text-lg">{t('manage.evaluation.loadingEvaluation')}</div> + </div> </Layout>
32-48: Enhance error messaging with specific error details.The current error handling could be more informative to help users understand and resolve issues.
-if (error && !data) { - return <Layout>{t('shared.generic.systemError')}</Layout> +if (error && !data) { + return ( + <Layout> + <UserNotification + type="error" + message={t('shared.generic.systemError')} + description={error.message} + /> + </Layout> + ) }
49-52: Remove outdated TODO comment.The TODO comment on line 49 can be removed as feedbacks, confusion feedbacks, and leaderboard are already implemented in the ActivityEvaluation props.
54-61: Consider adding TypeScript interface for evaluation data.While the implementation works, adding a dedicated interface for the evaluation data structure would improve type safety and documentation.
interface LiveQuizEvaluation { displayName: string results: Array<any> // Replace 'any' with actual type feedbacks?: Array<any> confusionFeedbacks?: Array<any> leaderboard?: Array<any> }
Line range hint
63-82: Add proper typing for locale parameter.Replace the
anytype with proper typing for better type safety.-export async function getStaticProps({ locale }: any) { +export async function getStaticProps({ locale }: { locale: string }) {apps/frontend-manage/src/components/evaluation/navigation/EvaluationNavigation.tsx (1)
49-59: Consider improving the fallback case for better semantics and accessibility.While the ternary operator improves readability, the empty
<div />fallback could be enhanced.Consider this improvement:
{typeof activeStack === 'number' ? ( <InstanceNavigation stack={stacks[activeStack]} activeInstance={activeInstance ?? 0} setActiveInstance={setActiveInstance} numOfInstances={numOfInstances} instanceSelection={stackInstanceMap[activeStack]} /> ) : ( - <div /> + <div aria-hidden="true" className="flex-1" /> )}apps/frontend-manage/src/components/evaluation/ElementChart.tsx (1)
Line range hint
28-76: Consider refactoring the chart type selection logic.While the current implementation works correctly, the nested if-else structure could be refactored to improve maintainability and readability.
Consider using a switch statement or an object map for chart type handling:
- if (chartType === ChartType.TABLE) { - return ( - <ElementTableChart - instance={instanceEvaluation} - showSolution={showSolution} - textSize={textSize.text} - /> - ) - } else if ( - chartType === ChartType.HISTOGRAM && - instanceEvaluation.__typename === 'NumericalElementInstanceEvaluation' - ) { - // ... histogram logic ... - } else if (chartType === ChartType.WORD_CLOUD) { - // ... word cloud logic ... - } else if (chartType === ChartType.BAR_CHART) { - // ... bar chart logic ... - } else { - return <div>{t('manage.evaluation.noChartsAvailable')}</div> - } + const chartComponents = { + [ChartType.TABLE]: () => ( + <ElementTableChart + instance={instanceEvaluation} + showSolution={showSolution} + textSize={textSize.text} + /> + ), + [ChartType.HISTOGRAM]: () => { + if (instanceEvaluation.__typename !== 'NumericalElementInstanceEvaluation') { + return null; + } + const responses = instanceEvaluation.results.responseValues.map( + (response) => ({ + value: response.value, + count: response.count, + }) + ); + return ( + <ElementHistogram + type={instanceEvaluation.type} + responses={responses} + solutionRanges={instanceEvaluation.results.solutionRanges} + statistics={instanceEvaluation.statistics} + minValue={instanceEvaluation.results.minValue} + maxValue={instanceEvaluation.results.maxValue} + showSolution={showSolution} + showStatistics={showStatistics} + textSize={textSize.text} + /> + ); + }, + [ChartType.WORD_CLOUD]: () => ( + <ElementWordcloud + instance={instanceEvaluation} + showSolution={showSolution} + textSize={{ min: textSize.min, max: textSize.max }} + /> + ), + [ChartType.BAR_CHART]: () => ( + <ElementBarChart + instance={instanceEvaluation} + showSolution={showSolution} + textSize={textSize} + /> + ), + }; + + const ChartComponent = chartComponents[chartType]; + return ChartComponent ? ( + <ChartComponent /> + ) : ( + <div>{t('manage.evaluation.noChartsAvailable')}</div> + );This refactoring:
- Improves code organization with a clear mapping of chart types to their components
- Makes it easier to add new chart types
- Reduces nesting and improves readability
apps/frontend-manage/src/components/evaluation/ActivityEvaluation.tsx (3)
34-37: Consider stricter null handling in props.The optional props (
feedbacks,confusionFeedbacks,leaderboard) allow bothundefinedandnull. Consider using only one of these to simplify null checks in the component.interface ActivityEvaluationProps { activityName: string stacks: StackEvaluation[] - feedbacks?: Feedback[] | null - confusionFeedbacks?: ConfusionTimestep[] | null - leaderboard?: LeaderboardCombinedEntry[] | null + feedbacks?: Feedback[] + confusionFeedbacks?: ConfusionTimestep[] + leaderboard?: LeaderboardCombinedEntry[] type?: ActivityEvaluationType }Also applies to: 40-47
56-57: TODO comment needs more details.The TODO comment about PPT integration lacks specificity. Consider adding more details about:
- The expected behavior
- Required parameters from PPT
- Implementation approach
Would you like me to help create a GitHub issue to track this enhancement?
Line range hint
116-184: Consider extracting feature rendering logic into separate components.The conditional rendering logic for leaderboard, feedbacks, and confusion features follows a similar pattern and could be extracted into separate components to improve maintainability and reduce duplication.
Example refactor:
interface FeatureProps { isVisible: boolean data: any[] // Use proper type sessionName?: string emptyMessage: string } const FeatureContainer: React.FC<FeatureProps> = ({ isVisible, data, emptyMessage, children, }) => { if (!isVisible) return null; return ( <div className="overflow-y-auto"> <div className="border-t p-4"> <div className="mx-auto max-w-5xl text-xl"> {data.length > 0 ? ( children ) : ( <UserNotification className={{ message: 'text-lg' }} type="warning" message={emptyMessage} /> )} </div> </div> </div> ); }; // Usage: <FeatureContainer isVisible={type === 'LiveQuiz' && activeStack === 'leaderboard'} data={leaderboard ?? []} emptyMessage={t('manage.evaluation.noSignedInStudents')} > <Leaderboard leaderboard={leaderboard} podiumImgSrc={{ rank1: Rank1Img, rank2: Rank2Img, rank3: Rank3Img, }} /> </FeatureContainer>packages/graphql/src/schema/evaluation.ts (2)
17-18: Add documentation for the new feedback fields.While the types are properly defined, please add JSDoc comments explaining:
- The purpose and usage of
feedbacks- The purpose and usage of
confusionFeedbacks- The relationship between these fields and live quiz evaluation
This documentation will help other developers understand the schema changes.
17-18: Consider the impact of optional feedback fields on queries.The addition of optional feedback fields to
ActivityEvaluationintroduces new considerations:
- Query Optimization: Clients should use fragments or specify exactly which feedback fields they need to avoid over-fetching data.
- Backward Compatibility: The optional nature of these fields maintains compatibility with existing queries.
- Error Handling: Clients should handle cases where these fields are null or undefined.
Consider documenting these considerations in the schema or adding examples in the README.
Also applies to: 155-162
packages/graphql/src/services/liveQuizzes.ts (1)
1696-1743: Consider optimizing the database query.The query includes extensive related data that might not be needed in all cases. Consider:
- Implementing field selection based on GraphQL query fields
- Splitting the query into smaller chunks that can be loaded on demand
packages/graphql/src/public/server.json (1)
130-130: LGTM! Consider adding documentation for the evaluation schema.The new evaluation schema is well-structured with proper type safety, comprehensive result handling for different question types, and good security practices. The fragment reuse and type definitions follow best practices.
Consider adding documentation comments to describe:
- The purpose and usage of the
EvaluationResultsfragment- The authentication requirements for
GetLiveQuizEvaluation- The statistical calculations for numerical evaluations
Add documentation using GraphQL descriptions:
""" Fragment containing evaluation results for different types of quiz elements. Supports choices, free text, numerical, flashcard, and content elements. """ fragment EvaluationResults on ActivityEvaluation { # ... existing fields } """ Fetches evaluation results, feedbacks, and leaderboard data for a live quiz. Requires quiz ID and optional HMAC authentication. """ query GetLiveQuizEvaluation($id: String!, $hmac: String) { # ... existing fields }packages/graphql/src/ops.ts (1)
4373-4373: Consider splitting the large evaluation query.The
GetLiveQuizEvaluationDocumentis quite extensive. Consider splitting it into smaller, focused queries for better maintainability:
- Separate queries for basic evaluation data
- Dedicated query for feedback data
- Dedicated query for confusion feedback data
Example structure:
export const GetBasicLiveQuizEvaluationDocument = /* ... basic fields ... */ export const GetLiveQuizFeedbackDocument = /* ... feedback fields ... */ export const GetLiveQuizConfusionDocument = /* ... confusion fields ... */
🛑 Comments failed to post (3)
packages/graphql/src/services/liveQuizzes.ts (3)
1688-1694: 🛠️ Refactor suggestion
Enhance authentication check robustness.
The authentication check can be improved in several ways:
- Use strict equality (
===) for empty string comparison- Consider throwing a GraphQLError for unauthorized access instead of returning null
- Add input validation for the HMAC format
Apply this diff to improve the authentication check:
export async function getLiveQuizEvaluation( { id, hmac }: { id: string; hmac?: string | null }, ctx: Context ) { - if ((!ctx.user?.sub && typeof hmac !== 'string') || hmac == '') { + if (!ctx.user?.sub) { + if (typeof hmac !== 'string' || hmac === '' || !/^[a-f0-9]{64}$/i.test(hmac)) { + throw new GraphQLError('Unauthorized access') + } + } - return null - }Committable suggestion skipped: line range outside the PR's diff.
1749-1758:
⚠️ Potential issueAdd fallback for missing APP_SECRET environment variable.
The HMAC validation could fail if APP_SECRET is not set. Add a check to ensure the secret is available.
Apply this diff to add the check:
if (typeof hmac === 'string') { + const appSecret = process.env.APP_SECRET + if (!appSecret) { + throw new Error('APP_SECRET environment variable is not configured') + } - const hmacEncoder = createHmac('sha256', process.env.APP_SECRET as string) + const hmacEncoder = createHmac('sha256', appSecret) hmacEncoder.update(liveQuiz.namespace + liveQuiz.id) const quizHmac = hmacEncoder.digest('hex')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if (typeof hmac === 'string') { const appSecret = process.env.APP_SECRET if (!appSecret) { throw new Error('APP_SECRET environment variable is not configured') } const hmacEncoder = createHmac('sha256', appSecret) hmacEncoder.update(liveQuiz.namespace + liveQuiz.id) const quizHmac = hmacEncoder.digest('hex') // evaluate whether the hashed liveQuiz.namespace and liveQuiz.id equals the hmac if (quizHmac !== hmac) { return null } }
1760-1792: 🛠️ Refactor suggestion
Add error handling for cache processing.
The cache processing could fail silently. Consider adding proper error handling and logging.
Apply this diff to improve error handling:
if (cachedResults) { + try { const { instanceResults } = await processCachedData({ cachedResults, activeBlock: liveQuiz.activeBlock, }) activeBlockWithResults = { ...liveQuiz.activeBlock, elements: liveQuiz.activeBlock.elements.map((instance) => ({ ...instance, anonymousResults: instanceResults[instance.id]?.anonymousResults ?? instance.anonymousResults, })), } + } catch (error) { + console.error('Failed to process cached results:', error) + // Continue without cached results + } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// load results from active block as well let activeBlockWithResults: | (ElementBlock & { elements: ElementInstance[] }) | undefined if (liveQuiz.activeBlockId && liveQuiz.activeBlock) { const activeInstanceIds = liveQuiz.activeBlock.elements.map( (instance) => instance.id ) const cachedResults = await getCachedBlockResults({ ctx, quizId: id, blockId: liveQuiz.activeBlockId, activeInstanceIds, }) if (cachedResults) { try { const { instanceResults } = await processCachedData({ cachedResults, activeBlock: liveQuiz.activeBlock, }) activeBlockWithResults = { ...liveQuiz.activeBlock, elements: liveQuiz.activeBlock.elements.map((instance) => ({ ...instance, anonymousResults: instanceResults[instance.id]?.anonymousResults ?? instance.anonymousResults, })), } } catch (error) { console.error('Failed to process cached results:', error) // Continue without cached results } } }
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores